feat: MCP response + error extraction per AdCP spec#195
Merged
Conversation
Closes #193. Implements the normative algorithms from the AdCP spec: - `docs/building/implementation/mcp-response-extraction.mdx` §Extraction Algorithm (success path) - `docs/building/implementation/transport-errors.mdx` §Client Detection Order paths 1 + 5 (MCP tool-level error path) ## Problem Before this PR the MCP adapter required `structuredContent` and raised when absent. The AdCP reference training agent (and any server using older MCP tooling) returns the AdCP payload as JSON inside a `TextContent` item with no `structuredContent` — a pattern explicitly permitted by the spec. Our SDK was spec-noncompliant and could not talk to the reference implementation end-to-end. ## Implementation New module-level helpers in `src/adcp/protocols/mcp.py`: - `extract_adcp_success(result)` — implements the 4-step spec algorithm: (1) short-circuit on `isError`; (2) prefer `structuredContent` when it's a non-array object that is not `adcp_error`-only; (3) iterate `content[]` `type='text'` items within the 1MB size cap, `json.loads`, accept the first non-array object that is not `adcp_error`-only; (4) return None. - `extract_adcp_error(result)` — implements §Client Detection Order paths 1 (`structuredContent.adcp_error`) and 5 (text fallback with `adcp_error` key). Only fires on `isError=true` responses. Validates the error via `_validate_adcp_error`: dict, non-empty string `code` ≤ 64 chars, total serialized size ≤ 4KB. `_call_mcp_tool` refactored to call these helpers. The success path now accepts text-JSON responses; the error path now extracts `adcp_error` singletons per the AdCP MCP binding. Idempotency integration: the error path checks `adcp_error.code` directly against the idempotency code set, raising `IdempotencyConflictError` / `IdempotencyExpiredError` before constructing a generic `TaskResult(failed)`. FastMCP-style text-only idempotency errors still work via `raise_for_idempotency_text`. Size-limit guards applied consistently on both paths (success + error) before `json.loads` to prevent DoS via multi-MB text blobs. ## Testing - **New test file** `tests/test_mcp_extraction.py` (60 tests) — replays the 16 upstream success vectors and the MCP subset of 29 upstream error vectors, bundled in `tests/fixtures/mcp_extraction/`. Plus edge cases for oversized-text handling, `adcp_error`-only guards, array rejection, and a DoS regression test asserting the 1MB pre-parse cap on the error path. - **Existing MCP protocol tests** updated: `test_call_tool_missing_structured_content` renamed to `test_call_tool_no_structured_adcp_data` with the new error message; added `test_call_tool_text_json_fallback` covering the reference- agent shape. - **Existing idempotency test** `test_structured_conflict_raises` updated to use the spec-canonical `{"adcp_error": {...}}` singleton shape instead of the pre-#2315 `{"errors": [...]}` array shape. Total: 1339 passing, 8 skipped. mypy clean (643 files). ruff clean. ## Live verification Probed `test-agent.adcontextprotocol.org/mcp/` without any monkey-patch: - Phase 1 capability discovery: ✅ seller declares `replay_ttl_seconds=86400` - Phase 5 `create_media_buy` with fresh keys: ✅ distinct `media_buy_id` per call, SDK-generated idempotency_keys flow through to the wire and surface on `result.idempotency_key` Phases 3 and 4 still fail because of the training-agent spec-enforcement gap (adcp#2346), not an SDK issue. ## Behavioral note Plain-text responses with no JSON and no `structuredContent` previously raised `ValueError("did not return structuredContent")` internally (caught and returned as `TaskResult(failed)` with that message). They now return a `TaskResult(failed)` with message `"... returned no structured AdCP data. Neither structuredContent nor content[].text yielded a parseable non- adcp_error JSON object."`. No callers match on either string (grepped). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Ignores the Claude Code harness's scheduled-task lock and session state so they don't end up tracked by accident. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bokelley
added a commit
that referenced
this pull request
Apr 19, 2026
…X polish Addresses expert review findings from the initial PR #196 round. ## P0 fixes **Wire translation (spec-blocking).** `IdempotencyConflictError` raised from `@idempotency.wrap` previously bubbled as a generic Python exception → FastMCP's "Tool execution failed" / A2A blanket Exception → no `IDEMPOTENCY_CONFLICT` on the wire. Fixed on both transports: - MCP: `serve.py _register_tool` wraps the tool `fn()` in `try/except ADCPError → translate_error(exc, "mcp")`, raising a `ToolError` that FastMCP surfaces as `is_error=true` with the code in text (our own client's text-mode extraction from PR #195 recovers the code). - A2A: `ADCPAgentExecutor.execute` catches `ADCPError` before the blanket `Exception` handler and routes to a new `_send_adcp_error` helper that emits a `TaskState.failed` with a `DataPart({"adcp_error": {...}})` per `transport-errors.mdx §A2A Binding`, including `recovery="terminal"` for IDEMPOTENCY_CONFLICT / IDEMPOTENCY_EXPIRED. - Added `IDEMPOTENCY_CONFLICT` and `IDEMPOTENCY_EXPIRED` to `STANDARD_ERROR_CODES` with `recovery="terminal"` so `translate_error` looks up the right recovery classification. **Capabilities helper.** `capabilities_response()` now accepts an `idempotency=` kwarg that nests the declaration under `adcp.idempotency`. The docstring quickstart used `caps.adcp.idempotency = ...` which raised `AttributeError` since `capabilities_response` returns a plain dict. Fixed by updating both the helper and the docstring example to pass `idempotency=store.capability()`. ## P1 fixes **Discoverability.** Re-exported `IdempotencyStore` and `MemoryBackend` from `adcp.server` so sellers can `from adcp.server import IdempotencyStore, MemoryBackend` alongside `ADCPHandler`. Matches the existing convention for every other seller-facing type. **README.** Added a "Building a seller: idempotency middleware" section adjacent to the buyer-side section. Covers `@idempotency.wrap`, the `capability()` declaration, the IDEMPOTENCY_CONFLICT wire behavior, backend status (MemoryBackend ships, PgBackend is scaffolded), and the atomicity caveat. ## P2 fixes **Test ergonomics.** `IdempotencyStore` and `MemoryBackend` now accept an optional `clock: Callable[[], float]` so TTL tests don't have to monkeypatch `time.time`. `MemoryBackend.clear()` added for fixture resets. Regression test `TestTTLExpiry.test_cached_response_expires_after_ttl` migrated to use the injected clock. **PgBackend scaffold.** Docstring now leads with `.. warning::`, the `NotImplementedError` message includes the full issue URL, and the schema sketch notes `COLLATE "C"` (to prevent case-insensitive collation collapsing distinct tenants) and recommends row-level security on `principal_id` as a defense-in-depth layer. **Quickstart docstring.** `adcp.server.idempotency.__init__` quickstart now shows `params.idempotency_key` and `ToolContext(caller_identity=...)` explicitly so first-time users don't silently fall into the no-dedup fallthrough path. ## New tests - `TestWireTranslation.test_mcp_conflict_translates_to_tool_error` — asserts an IdempotencyConflictError from `@idempotency.wrap` surfaces as a `ToolError` with `IDEMPOTENCY_CONFLICT` in the text. - `TestWireTranslation.test_a2a_conflict_emits_failed_task_with_adcp_error` — asserts `_send_adcp_error` emits a `TaskState.failed` with a `DataPart({"adcp_error": {"code": "IDEMPOTENCY_CONFLICT", "recovery": "terminal"}})`. - `TestCapability.test_capabilities_response_accepts_idempotency` — asserts the new kwarg nests under `adcp.idempotency`. - `TestCapability.test_server_reexports` — locks the `adcp.server` re-exports so future refactors can't silently drop them. Total: 1488 passing (from 1483). mypy clean. ruff clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Closes #193.
Why
The MCP adapter required
structuredContenton successful tool responses and raised when absent. The AdCP reference training agent (and older MCP servers generally) returns the AdCP payload as JSON inside aTextContentitem with nostructuredContent— a pattern explicitly permitted by the spec. Our SDK was spec-noncompliant and could not talk to the reference implementation end-to-end. Issue #193 was originally filed as a "convenience fallback"; it turned out to be a real spec-compliance bug.What
Implements the two normative algorithms from upstream AdCP docs:
mcp-response-extraction.mdx§Extraction Algorithm (success path)transport-errors.mdx§Client Detection Order paths 1 + 5 (MCP tool-level error path)Two new helpers in
src/adcp/protocols/mcp.py:extract_adcp_success(result)— 4-step spec algorithm: short-circuit onisError→ preferstructuredContent(non-array, notadcp_error-only) → text fallback with 1MB size cap,json.loads, accept first non-array non-adcp_error-only object → return None.extract_adcp_error(result)— paths 1+5 from the detection-order spec.structuredContent.adcp_errorthen text fallback. Validates: dict, non-emptycodestring ≤ 64 chars, total serialized size ≤ 4KB._call_mcp_toolnow uses these. Idempotency integration: we checkadcp_error.codedirectly against the idempotency code set and raise typed exceptions before building a genericTaskResult(failed). FastMCP-style text-only idempotency errors still work viaraise_for_idempotency_text.DoS protection: 1MB pre-parse cap applied consistently on both success and error paths.
Testing
tests/test_mcp_extraction.py— 60 new tests:tests/fixtures/mcp_extraction/)adcp_error-only guards, array/primitive rejection, multi-item iteration, non-text content, size-limit DoS regressiontests/test_protocols.py— renamedtest_call_tool_missing_structured_content→test_call_tool_no_structured_adcp_data; addedtest_call_tool_text_json_fallbackcovering the reference-agent shape.tests/test_idempotency.py— updatedtest_structured_conflict_raisesto use the spec-canonical{"adcp_error": {...}}singleton (was{"errors": [...]}).Total: 1339 passing, 8 skipped. mypy clean (643 files). ruff clean.
Live verification
Probed
test-agent.adcontextprotocol.org/mcp/with no monkey-patch:replay_ttl_seconds=86400create_media_buywith fresh keys ✅ — distinctmedia_buy_idper call, SDK-generated idempotency keys flow to the wire and surface onresult.idempotency_keyPhases 3 (replay) and 4 (conflict) continue to fail because the training agent doesn't enforce its own declared semantics — that's adcp#2346, not an SDK issue.
Behavioral note
Plain-text responses with no JSON and no
structuredContentpreviously surfaced an error"MCP tool X did not return structuredContent...". They now surface"... returned no structured AdCP data. Neither structuredContent nor content[].text yielded a parseable non-adcp_error JSON object.". Grepped internal + external code: no callers match on either string.Expert review
One Should-Fix from the code-reviewer applied in this PR: 1MB pre-parse size cap on the error path's text fallback (previously only success path had it) — a real DoS vector. Regression test added.
Test plan
pytest tests/ --no-cov -q— 1339 passedmypy src/adcp/— 0 errorsruff check— clean🤖 Generated with Claude Code