feat(server): auto-unwrap thrown adcpError envelopes in dispatcher#743
Merged
Conversation
Handlers that `throw adcpError(...)` instead of `return adcpError(...)` used to surface as `SERVICE_UNAVAILABLE: Tool X handler threw: [object Object]` — the thrown value is a plain object, not an Error, so err.message is undefined and String(err) yields the literal. The dispatcher now detects the envelope shape (isError + content + structuredContent.adcp_error.code) and returns it directly, preserving the typed code / field / suggestion as if the handler had used `return`. Idempotency claims are released on unwrap (same as any thrown path) so retries proceed normally. Non-envelope throws (TypeError, strings, custom errors) still go through the SERVICE_UNAVAILABLE path with the underlying cause exposed when exposeErrorDetails is on. Driver: matrix v8 showed this pattern persisting across fresh-Claude builds even when skill examples use `return`. Fixing it at the dispatcher closes the class of bugs instead of chasing it through every skill-corpus update.
Code review: assert message:string in isThrownAdcpError so malformed envelopes fall through to SERVICE_UNAVAILABLE instead of producing a half-formed response. Security review: log message + stack on unwrap so forensic review sees what was thrown — previously only the code was captured, which made the "dependency trojaned an envelope" attack model invisible. Follow-up filed as #744 for the broader question of whether recovery:terminal envelopes should cache-on-error instead of release — applies to both return and throw paths, not caused by this PR.
This was referenced Apr 21, 2026
bokelley
added a commit
that referenced
this pull request
Apr 21, 2026
Addresses the audit question in #744: should `recovery: terminal` errors cache on replay, or keep release-all-errors? Audit conclusion: keep release-all-errors (most AdCP terminals are state-dependent; caching would return stale errors post-remediation). Document the handler-author implication that #743 widened the surface for. - skills/build-seller-agent: new "mutate last" paragraph in the idempotency section, with broken-vs-correct budget-approval example and guidance for partial-write paths. - skills/build-creative-agent: swap the now-stale "throw surfaces as SERVICE_UNAVAILABLE" rationale (invalidated by #743's auto-unwrap) for the still-true claim-release rationale. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Handlers that write `throw adcpError(...)` instead of `return adcpError(...)` used to surface as:
because the thrown value is a plain object, not an `Error` — so `err.message` is undefined and `String(err)` yields the `[object Object]` literal.
The dispatcher now detects the envelope shape (`isError: true` + `content: [...]` + `structuredContent.adcp_error.code`) and returns it directly, preserving the typed code / field / suggestion as if the handler had written `return`. A `logger.warn` still fires so agent authors see they should switch to `return`, but buyers stop paying for the mistake.
Why this lives in the SDK, not the skill corpus: matrix v8 showed this pattern persisting across fresh-Claude builds even when every skill example uses `return`. Closing the class of bugs at the dispatcher is a one-time fix vs. an N-skill ongoing drift tax.
Behavior
Test plan