diff --git a/.changeset/sturdy-dogs-caper.md b/.changeset/sturdy-dogs-caper.md new file mode 100644 index 000000000..fbf0b6cb1 --- /dev/null +++ b/.changeset/sturdy-dogs-caper.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3485 +--- +The /gsd slash command in Pi now visibly renders its output (progress, errors) via Pi's ctx.ui.notify mechanism instead of a return value Pi silently discards. diff --git a/pi/gsd.cjs b/pi/gsd.cjs index 0b107d34d..cf9b4eb3f 100644 --- a/pi/gsd.cjs +++ b/pi/gsd.cjs @@ -320,21 +320,35 @@ module.exports = function gsdPiExtension(pi) { getArgumentCompletions, handler: async (args, ctx) => { const cwd = (ctx && ctx.cwd) || process.cwd(); + // #3456: Pi's command dispatcher awaits the handler and DISCARDS its + // return value (agent-session.ts: `await command.handler(args, ctx); + // return true;`) — #3097's `{ content: [...] }` return was silently + // dropped exactly like the pre-#3097 bare string. The ONLY command- + // output mechanism Pi consumes is `ctx.ui.notify(message, type)` + // (pi docs: extensions.md#piregistercommandname-options), so the output + // is pushed as a side effect on both the success and error paths. + // Guarded so a host ctx without `ui` degrades silently instead of + // crashing the command. + const notify = (ctx && ctx.ui && typeof ctx.ui.notify === 'function') + ? (message, type) => ctx.ui.notify(message, type) + : null; const { family, subcommand, args: rest } = parseGsdCommandArgs(args); let dispatchGsdCommand; try { ({ dispatchGsdCommand } = require(path.join(GSD_CORE, 'bin', 'lib', 'shell-command-projection.cjs'))); } catch (e) { - // #2991: return the structured { content } shape Pi's ExtensionAPI - // displays, not a bare string (which Pi silently drops). - return { content: [{ type: 'text', text: `GSD engine unavailable: ${e && e.message ? e.message : String(e)}` }] }; + if (notify) { + notify(`GSD engine unavailable: ${e && e.message ? e.message : String(e)}`, 'error'); + } + return; } const result = dispatchGsdCommand({ family, subcommand, args: rest, cwd }); - // #2991: match gsd_invoke's proven output shape so Pi actually displays it. const text = result.ok ? result.stdout : `GSD error: ${result.stderr || result.stdout || `dispatch failed (exit ${result.code})`}`; - return { content: [{ type: 'text', text }] }; + if (notify) { + notify(text, result.ok ? 'info' : 'error'); + } }, }); diff --git a/tests/pi-extension-reachability.test.cjs b/tests/pi-extension-reachability.test.cjs index ce6d5f83f..b31a3ea11 100644 --- a/tests/pi-extension-reachability.test.cjs +++ b/tests/pi-extension-reachability.test.cjs @@ -6,8 +6,9 @@ * Proves the pi extension is keystone-WIRED: the registered /gsd command's * `handler(args, ctx)` (pi's REAL ExtensionAPI shape — NOT the `execute(ctx)` * shape the original #1944 cut used) dispatches through gsd-tools.cjs - * (subprocess-reuse — dispatchGsdCommand) and returns real output, not just a - * registration on a mock. This is the "user can invoke X" proof. + * (subprocess-reuse — dispatchGsdCommand) and renders real output through + * `ctx.ui.notify` (#3456 — Pi discards command-handler return values), not + * just a registration on a mock. This is the "user can invoke X" proof. * * Dispatch is exercised with a real read-only family/subcommand * (progress/json) against a real temp project, matching the sibling @@ -59,7 +60,27 @@ test('REACHABILITY: empty args dispatch a working default (gsd-tools.cjs --help) assert.equal(parsed.subcommand, undefined); }); -test('REACHABILITY: the /gsd handler dispatches a real family through gsd-tools.cjs and returns real output (keystone wired)', async () => { +// #3456: Pi's command dispatcher does `await command.handler(args, ctx); +// return true;` — it DISCARDS the handler's return value in any shape. The +// only command-output mechanism Pi consumes is `ctx.ui.notify(message, type)` +// (earendil-works/pi docs: extensions.md#piregistercommandname-options). The +// #3097 tests asserted on the discarded return object, so they passed while +// `/gsd` stayed mute in real Pi. These tests invoke the handler exactly as +// Pi's dispatcher does (await + discard) and assert the notify side effect. +function makePiCtx(dir) { + const notifications = []; + return { + ctx: { + cwd: dir, + ui: { + notify(message, type) { notifications.push({ message, type }); }, + }, + }, + notifications, + }; +} + +test('REACHABILITY: the /gsd handler dispatches a real family through gsd-tools.cjs and renders output via ctx.ui.notify (keystone wired)', async () => { const pi = mockPi(); gsdPiExtension(pi); const dir = createTempDir(); @@ -70,29 +91,46 @@ test('REACHABILITY: the /gsd handler dispatches a real family through gsd-tools. // is withheld (null), breaking this reachability proxy. fs.mkdirSync(path.join(dir, '.planning'), { recursive: true }); fs.writeFileSync(path.join(dir, '.planning', 'ROADMAP.md'), '# Roadmap\n'); - const result = await pi._recorded.commands['gsd'].handler('progress json', { cwd: dir }); - // #2991: handler returns { content: [{ type: 'text', text }] } (Pi's display shape), not a bare string. - assert.ok(result && Array.isArray(result.content) && result.content[0].type === 'text', - `/gsd handler must return Pi's display shape { content: [{ type: 'text', text }] }; got: ${JSON.stringify(result).slice(0, 200)}`); - const parsed = JSON.parse(result.content[0].text); + const { ctx, notifications } = makePiCtx(dir); + // Invoke EXACTLY as Pi's dispatcher does: await, then discard the return. + const returned = await pi._recorded.commands['gsd'].handler('progress json', ctx); + assert.equal(notifications.length, 1, `/gsd must push exactly one ui.notify; got ${notifications.length}`); + assert.equal(notifications[0].type, 'info', 'successful dispatch notifies with type "info"'); + const parsed = JSON.parse(notifications[0].message); assert.equal(typeof parsed.percent, 'number', '/gsd dispatch reached gsd-tools.cjs for real (the engine was reached)'); + assert.equal(returned, undefined, 'the handler return value is the boundary Pi discards — the output must travel via ctx.ui.notify, not a return (#3456)'); } finally { cleanup(dir); } }); -test('REACHABILITY: an unknown family surfaces a clear GSD error, not a throw', async () => { +test('REACHABILITY: an unknown family renders a clear GSD error via ctx.ui.notify, not a throw', async () => { const pi = mockPi(); gsdPiExtension(pi); const dir = createTempDir(); try { - const result = await pi._recorded.commands['gsd'].handler('no-such-family-8675309', { cwd: dir }); - // #2991: handler returns { content: [{ type: 'text', text }] } (Pi's display shape). - assert.ok(result && Array.isArray(result.content) && result.content[0].type === 'text', - `error result must carry Pi's display shape; got: ${JSON.stringify(result).slice(0, 200)}`); - const text = result.content[0].text; - assert.match(text, /GSD error:/); - assert.match(text, /no-such-family-8675309|Unknown command/); + const { ctx, notifications } = makePiCtx(dir); + const returned = await pi._recorded.commands['gsd'].handler('no-such-family-8675309', ctx); + assert.equal(notifications.length, 1, `failed dispatch must push exactly one ui.notify; got ${notifications.length}`); + assert.equal(notifications[0].type, 'error', 'failed dispatch notifies with type "error"'); + assert.match(notifications[0].message, /GSD error:/); + assert.match(notifications[0].message, /no-such-family-8675309|Unknown command/); + assert.equal(returned, undefined, 'the error text must travel via ctx.ui.notify, not the discarded return (#3456)'); + } finally { + cleanup(dir); + } +}); + +test('REACHABILITY: the /gsd handler tolerates a ctx without ui (older Pi host) without throwing', async () => { + const pi = mockPi(); + gsdPiExtension(pi); + const dir = createTempDir(); + try { + fs.mkdirSync(path.join(dir, '.planning'), { recursive: true }); + fs.writeFileSync(path.join(dir, '.planning', 'ROADMAP.md'), '# Roadmap\n'); + // No assert on output — a host without ctx.ui has no renderable channel; + // this only proves the handler does not crash such a host. + await pi._recorded.commands['gsd'].handler('progress json', { cwd: dir }); } finally { cleanup(dir); }