Skip to content

fix(server): echo request context on AdcpError error envelopes#560

Merged
bokelley merged 2 commits intomainfrom
bokelley/issue-557-adcperror-context-echo
May 4, 2026
Merged

fix(server): echo request context on AdcpError error envelopes#560
bokelley merged 2 commits intomainfrom
bokelley/issue-557-adcperror-context-echo

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

@bokelley bokelley commented May 4, 2026

Closes #557

Summary

Fixes a context-passthrough gap on the AdcpError raise path. The success path runs inject_context(raw_params, response) so a request's context extension echoes back to the buyer. The error path built the structured adcp_error envelope without doing the same — buyers lost correlation IDs and idempotency hints across the raise-AdcpError boundary, and three storyboard runners were failing on this assertion.

What changed

  • build_mcp_error_result(exc, params=None) now accepts the raw request dict. When supplied, it runs inject_context on the structuredContent dict so context lands as a sibling of adcp_error, mirroring the success path's wire shape.
  • MCP catch sites (src/adcp/server/serve.py) pass kwargs through on both branches — the ADCPError catch and the decisioning-AdcpError re-projection.
  • ADCPAgentExecutor._send_adcp_error(...) now accepts params and runs inject_context on the DataPart payload before sending the failed task.
  • execute() in the A2A executor passes the parsed params into _send_adcp_error.

The change is backwards-compatible: both new params are optional. Existing callers (proxies, custom transports invoking build_mcp_error_result directly) keep working without context echo until they wire it through.

Tests

  • tests/test_mcp_structured_error.py — added TestBuildMcpErrorResultContextEcho (4 unit tests) covering: no params, params without context, params with context, sibling-not-nested placement; plus an end-to-end test_context_echo_round_trips_through_register_tool.
  • tests/test_a2a_structured_error.py — added 3 executor-level tests covering: echo on raise, no-context omission, sibling-not-nested placement.
  • All 29 MCP + 14 A2A structured-error tests pass; wider regression run on tests/test_translate.py + tests/test_a2a_server.py + tests/test_server_helpers.py (128 tests) green.

Test plan

  • pytest tests/test_mcp_structured_error.py tests/test_a2a_structured_error.py — pass
  • pytest tests/test_translate.py tests/test_a2a_server.py tests/test_server_helpers.py — pass
  • ruff check — pass
  • pre-commit (black, ruff, mypy, bandit) — all hooks pass
  • Storyboard runners pick up the fix on the 3 previously-failing assertions

🤖 Generated with Claude Code

bokelley and others added 2 commits May 4, 2026 06:04
…#557)

The success path runs inject_context(raw_params, response) so a
request's context extension echoes back to the buyer. The error path
built the structured adcp_error envelope without doing the same — buyers
lost correlation IDs and idempotency hints across the raise-AdcpError
boundary.

- build_mcp_error_result now accepts optional params and runs
  inject_context on the structuredContent dict alongside adcp_error.
- _serve_*_call_tool catches pass kwargs (the raw request dict).
- ADCPAgentExecutor._send_adcp_error accepts params and runs
  inject_context on the DataPart payload.
- Tests cover all three shapes: no params, params without context,
  params with context — plus end-to-end through both transports.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…h error path)

Code-review found the original PR fixed only the A2A error path while
the success path (_send_result) silently dropped the request's
``context`` extension. The asymmetry — context echoed on errors but
not on successes — would have surprised buyers. Closing the gap so the
context-passthrough contract holds across both A2A outcomes, matching
MCP's mcp_tools.py:2030 behaviour.

Also from review:
- ``build_mcp_error_result(exc, *, params=None)`` is now keyword-only
  to lock the API shape (no risk of meaning-shift if a future positional
  arg is added).
- Test added for the 64KB context size cap on the error path —
  buyer-controlled context cannot amplify error response size.
- Test added for A2A success-path echo + no-context omission.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bokelley
Copy link
Copy Markdown
Contributor Author

bokelley commented May 4, 2026

Updated based on expert review (code-reviewer + ad-tech-protocol-expert):

Expanded scope: code review caught that the A2A success path (_send_result) also skips inject_context — the original PR would have left a strange asymmetry (context echoed on A2A errors, dropped on A2A successes). Fixed in the same PR since it's the same bug class and a one-line addition; updated docstrings/tests accordingly.

Other review fixes:

  • build_mcp_error_result(exc, *, params=None) is now keyword-only — locks the API shape against future positional drift.
  • Added 64KB-cap drop test on the error path so the security note about buyer-controlled context size is enforced by a test, not just the docstring.
  • Added A2A success-path echo + no-context-omission tests.

Open question for reviewer: ad-tech-protocol-expert flagged that transport-errors.mdx couldn't be fetched to confirm normative wire shape (sibling vs. nested). The PR matches the success-path convention (context at structuredContent / DataPart top level, not inside adcp_error), which is the safest reading. If upstream spec disagrees we'd want to align there before merging.

@bokelley
Copy link
Copy Markdown
Contributor Author

bokelley commented May 4, 2026

On the open wire-shape question: ran a second pass with ad-tech-protocol-expert specifically on sibling vs. nested placement. Verdict: sibling is correct — proceed with confidence.

Three reasons:

  1. inject_context() is a top-level key injector by design (response["context"] = ctx). Calling it on {"adcp_error": {...}} is exactly the intended usage — there's no alternative injection point, and it guards against double-injection.
  2. The success-path convention is {"products": [...], "context": {...}}. Placing context alongside adcp_error rather than inside it keeps the envelope shape symmetric across both outcomes: context is always a sibling of the payload key, never nested within it.
  3. adcp_error is a typed schema object with defined fields (code, message, recovery, field, suggestion, retry_after, details). Injecting context inside it would mutate that schema to carry an opaque, variable-size blob — the schema-fidelity violation is the nesting case, not the sibling case.

One scoping note the expert raised (not a blocker): verify params in _send_adcp_error is the parsed wire-request dict that carries the context field, not a reduced/re-projected view. Checked the diff — execute() passes params directly through, so this is already correct.

The transport-errors.mdx fetch failure remains unresolved, but the symmetry argument is strong enough that this shouldn't block merge — if the spec later specifies nesting (which would be unusual given how context-passthrough is defined), it'd be a one-line fix.


Triaged by Claude Code. Session: https://claude.ai/code/session_01NAhicH2jcMyXpJe8E8gHsX


Generated by Claude Code

@bokelley
Copy link
Copy Markdown
Contributor Author

bokelley commented May 4, 2026

Two pre-merge follow-ups noted by @bokelley (filed as no-triage):

Shipping this PR now and tracking those two separately.

@bokelley bokelley merged commit 2e351b0 into main May 4, 2026
16 checks passed
bokelley added a commit that referenced this pull request May 4, 2026
Code-reviewer caught that the original assertion (INTERNAL_ERROR +
context echoed) would pass even if the dispatcher wrap step were
skipped. Adds details.caused_by.type == 'ValueError' assertion which
is set only by _internal_error_details inside the wrap path.

Other review nits: executor.shutdown(wait=True) for cleanup; trim
PR-#560 history from docstring per CLAUDE.md; rename for symmetry.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bokelley added a commit that referenced this pull request May 4, 2026
…non-AdcpError (#568)

* test(server): regression test for context-echo on dispatcher-wrapped non-AdcpError (closes #562)

PR #560 added context-echo on the AdcpError raise path. This test
verifies the implicitly-wrapped path: a raw ValueError from a
DecisioningPlatform method goes through _invoke_platform_method's
except-Exception clause which wraps to AdcpError(INTERNAL_ERROR),
which then projects through serve.py's catch + build_mcp_error_result
with params=kwargs and echoes the request context.

If a future refactor lets a non-AdcpError exception escape the
dispatch wrap, this test fails first.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test: strengthen #562 regression assertion to pin the wrap step

Code-reviewer caught that the original assertion (INTERNAL_ERROR +
context echoed) would pass even if the dispatcher wrap step were
skipped. Adds details.caused_by.type == 'ValueError' assertion which
is set only by _internal_error_details inside the wrap path.

Other review nits: executor.shutdown(wait=True) for cleanup; trim
PR-#560 history from docstring per CLAUDE.md; rename for symmetry.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bokelley added a commit that referenced this pull request May 4, 2026
…text (#569)

* fix(decisioning): TaskHandoff registry.fail and registry.complete echo request context (closes #563)

PR #560 fixed context-echo on the synchronous AdcpError raise path.
Background tasks (TaskHandoff) route through a separate path:
registry.complete on success and registry.fail on AdcpError or
wrapped INTERNAL_ERROR. Neither echoed the request's context
extension, so a buyer polling tasks/get on a deferred task lost
their correlation IDs across the handoff boundary.

_project_handoff now accepts request_params (the original Pydantic
model). Both _fail and the success branch apply inject_context to
the wire dict before passing to the registry. Symmetric across:
- sync MCP / A2A success path (mcp_tools.py / a2a_server._send_result)
- sync AdcpError path (serve.py via build_mcp_error_result)
- bg task success (registry.complete)
- bg task failure incl. wrapped INTERNAL_ERROR (registry.fail)

A new _to_request_dict helper coerces Pydantic to dict for
inject_context, with empty-dict fall-through on coercion failure.

4 new tests in test_decisioning_dispatch.py cover success-path echo,
AdcpError echo, wrapped-INTERNAL_ERROR echo (RuntimeError → wrap →
echo), and no-request-params no-op. 42 dispatch tests green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(decisioning): #569 expert-review — context lands at TasksGetResponse top level

Protocol-expert flagged the original PR put context inside result/error
on the registry record. Per schemas/cache/core/tasks_get_response.json
context is a top-level sibling of result/error/history. Spec-incorrect
shape would have surfaced as result.context / error.context on
tasks/get reads instead of top-level context.

Refactor:
- TaskRecord gains a request_context field; to_dict surfaces it at
  the top level under context key.
- TaskRegistry.issue accepts request_context kwarg (additive,
  default None — backwards-compatible for adopters who don't
  thread context).
- _project_handoff and _project_workflow_handoff (the second gap
  reviewer flagged — workflow path also issues tasks) extract
  context from request_params and pass to issue() once at
  issue-time. Terminal-state helpers (_fail, registry.complete) no
  longer touch context.
- _to_request_dict renamed to _extract_request_context. model_dump
  failures now log a warning (was silent before).

Tests assert top-level rec[context] (not rec[result][context] or
rec[error][context]) per the spec shape.

129 tests across registry/idempotency/dispatch green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(decisioning): forward-compat **_extra on TaskRegistry.issue (#569 nit)

Reviewer flagged that adding the request_context kwarg breaks custom
TaskRegistry impls in the wild — Python doesn't silently absorb
unknown kwargs at the call site. Adds **_extra: Any to both the
Protocol declaration and InMemoryTaskRegistry.issue so the framework
can introduce new optional kwargs in future versions without
breaking adopters who haven't upgraded their custom impls.

Custom impls MUST include **_extra: Any on their issue() signature.
Documented the contract in the Protocol docstring; InMemoryTaskRegistry
logs unrecognized kwargs at DEBUG so adopters notice they've fallen
behind without noisy warnings.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(server): AdcpError raise path bypasses context-echo on MCP and A2A error envelopes

1 participant