refactor: error-emission architecture base (PR 1 of 3)

[ChrisHuie]

Summary

• Establishes the AdCP spec 3.0.6 two-layer error envelope across MCP, A2A, and REST. _impl raises typed AdCPError; boundary translators run build_two_layer_error_envelope() so both
  adcp_error.code and errors[0].code populate — storyboard runners (@adcp/sdk@6.11.0+) no longer synthesize MCP_ERROR from missing layers.
• 4 structural guards establish a shrink-only ratchet: 82 Pattern A Error(code=...) sites and 126 raise ValueError sites capped per-file. PR 2 drains the caps; PR 3 covers async/submitted
  lifecycle.
• A2A audit-gap fix at adcp_a2a_server.py:1397 — _log_a2a_operation(success=False) now fires on AdCPError before re-raise. Behavior change: previously-silent failures reach audit log + Slack
  on merge.

Planning artifact: .claude/notes/error-emission-architecture/PLAN.md (8 decisions documented).

What's in this PR

Substrate (src/):

• core/exceptions.py — build_two_layer_error_envelope(); 7 typed subclasses (AdCPMediaBuyNotFoundError, AdCPPackageNotFoundError, AdCPCreativeRejectedError, AdCPBudgetExceededError,
  AdCPBudgetTooLowError, AdCPCapabilityNotSupportedError, AdCPProductUnavailableError); context param on AdCPError.
• core/tool_error_logging.py — AdCPToolError(ToolError) carries envelope; MCP wire text is JSON-parseable.
• core/context_manager.py — ContextManager.fail_step(step_id, *, exc, error_message). Persisted workflow_step.response_data is byte-identical to wire response (same builder).
• a2a_server/adcp_a2a_server.py — _adcp_to_a2a_error embeds envelope in data; audit-gap fix.
• app.py + routes/api_v1.py — REST boundary uses envelope.

Structural guards (tests/unit/):

• test_architecture_no_error_construction_in_impl.py — 82 Pattern A sites across 11 files (cap shrink-only).
• test_architecture_no_value_error_in_impl.py — 126 ValueError sites across 23 files.
• test_architecture_error_envelope_two_layer.py — AST guard verifying each of 3 boundary translators calls the envelope builder.
• _per_file_cap_guard.py — shared helper.

Drive-by hygiene (commit 2):

• Pre-commit mypy hook adcp pin 3.2.0 → 4.3.0 (was producing phantom AccountReference / get_adcp_spec_version errors).
• Drop 2 obsolete # type: ignore[arg-type] at src/app.py:44-45. Net src/ type:ignore count 60 → 59.
• Resync .type-ignore-baseline 42 → 59 (was stale; main had drifted via PRs bypassing the hook).
• Stale sync-creatives-request schema-mismatch allowlist entry fixed (account → account_id per adcp 4.3 spec evolution).

Coordination with in-flight PRs

• feat: wire buying_mode and refine through get_products (#1272) #1274 (buying_mode/refine): merge anytime, zero conflict.
• feat: round-trip property_list and add collection_list on media-buy (#1275) #1276 (property/collection lists): allowlist gains 1 Pattern A site if it lands first.
• feat: implement update_media_buy cancellation per AdCP 3.0.6 (#1261) #1262 (cancel, draft): 4 Pattern A sites — added to PR 2 cleanup scope after merge.

Spec & storyboard compliance

• AdCP spec 3.0.6 normative two-layer envelope (CHANGELOG entry 91b6e2c).
• Storyboard runner pinned at @adcp/sdk@6.11.0 (matches PR feat: wire buying_mode and refine through get_products (#1272) #1274 CI).
• Unblocks media_buy_seller/invalid_transitions raise-path codes; return-path codes wait for PR 2.

Test plan

• make quality clean (verified locally: 4481 passing, 19 xfailed, 0 failures).
• ./run_all_tests.sh — all 5 suites pass.
• Storyboard smoke: docker compose up -d then npx -y @adcp/sdk@6.11.0 storyboard run mcp http://localhost:8000/mcp/ --auth test-token --scenario media_buy_seller/invalid_transitions. Verify
  errors[0].code and adcp_error.code both surface real codes (MEDIA_BUY_NOT_FOUND, PACKAGE_NOT_FOUND, NOT_CANCELLABLE); zero MCP_ERROR synthesis.
• Operations review for Slack volume jump from A2A audit-gap fix.
• Cap counts unchanged (82 Pattern A, 126 ValueError).
• Coverage doesn't decrease.

Sequence

PR	Scope	Status
PR 1 (this PR)	Substrate + structural guards	✅ ready
PR 2	Pattern A cleanup sweep (~82 + 126 sites)	upcoming
PR 3	Async/submitted lifecycle + Response3 envelope	upcoming

[KonstantinMirin]

The design is right, the execution isn't there yet.

The core idea — typed exceptions, build_two_layer_error_envelope as single source of truth, AST guards enforcing it — is exactly what we need. But this is PR 1 of 3, the foundation everything else stacks on. It has to be airtight, and right now it's not.

The PR contradicts its own architecture in several places:

1. The A2A explicit-skill loop adds a proper except AdCPError handler with the two-layer envelope (line 629), then the except Exception catchall directly below it (line 638) produces {"error": str(e)} — a flat dict with no envelope at all. The architecture establishes the envelope as canonical, and the very code that introduces it has a fallthrough that ignores it.

2. _handle_tool_error was refactored to use the envelope builder, but still hardcodes status_code=500 for every error. A VALIDATION_ERROR (400) or AUTH_REQUIRED (401) gets served as 500 through REST. adcp_error_handler in app.py does this correctly — the pattern exists, it just wasn't applied here.

3. _envelope_to_adcp_error was extracted as the shared helper for reconstructing errors from envelopes. parse_rest_error — in the same file, modified in the same PR — inlines the same three-tier fallback logic instead of calling it. This is the kind of duplication we've been actively eliminating.

4. Context serialization appears three different ways in exceptions.py: to_dict(), to_adcp_error(), and build_two_layer_error_envelope() each serialize self.context with different behavior (exclude_none=True in one, not the others). One module, one PR, three divergent implementations of the same operation.

The three transport boundaries aren't symmetric:

The structural guard verifies all three transports call build_two_layer_error_envelope. But around that call, each boundary does something different:

• MCP catches ValueError/PermissionError and translates them. A2A does too. REST doesn't.
• MCP logs to activity feed + audit. A2A logs to audit only. REST logs to neither.
• A2A uses logger.error for business validation failures. MCP uses logger.warning for the same errors.

For an architecture PR that's supposed to make error handling consistent across transports, this is a significant gap.

What I'd like to see before this is done:

• The A2A catchall should produce an envelope (or convert to AdCPError and re-raise into the handler above it)
• _handle_tool_error should use the actual status code, not hardcode 500
• parse_rest_error should call _envelope_to_adcp_error
• Context serialization should be one method, not three expressions
• The three transport boundaries should handle the same exception types, log at the same level, and log to the same destinations — or the asymmetry should be documented as intentional with a reason

The structural guards and the per-file cap ratchet are well done. The exception hierarchy design is clean. This just needs another pass on the execution to match the quality of the design.

[KonstantinMirin]

Code Review: PR #1306 — Error Emission Architecture

Overall this is a well-designed architecture. build_two_layer_error_envelope() as a single source of truth enforced by AST structural guards, cross-transport roundtrip tests across all 11 subclasses, DRY-extracted per-file ratchet guards, and unified reconstruction via _envelope_to_adcp_error. Strong work.

---

Blockers

1. A2A _handle_explicit_skill ValueError/PermissionError asymmetry

src/a2a_server/adcp_a2a_server.py:1475-1489

AdCPError propagates to the dispatcher (-> failed Task with envelope DataPart), but ValueError and PermissionError are translated to JSON-RPC A2AError via _adcp_to_a2a_error(). Same semantic error class, different wire outcome. MCP and REST both treat these identically through the envelope path.

This contradicts the PR's own B4 contract: "AdCP-domain errors are async-task failures, not JSON-RPC transport errors."

Fix — wrap and re-raise instead of calling _adcp_to_a2a_error:

except ValueError as e:
    raise AdCPValidationError(str(e)) from e
except PermissionError as e:
    raise AdCPAuthorizationError(str(e)) from e

The outer dispatcher already handles both via _build_failed_skill_result. The catch-all except Exception can also be removed — the dispatcher's own except Exception covers it.

Flagged independently by 3 of 5 review agents — strongest cross-agent signal.

2. DRY violation: with_error_logging async/sync wrappers

src/core/tool_error_logging.py:236-253 vs :262-279

The 15-line exception-handling block (context-extraction loop over args/kwargs, _extract_tenant_and_principal, _log_tool_error, _translate_to_tool_error) is copy-pasted verbatim. Only difference: await tool_func(...) vs tool_func(...).

Fix — extract shared helper:

def _handle_tool_exception(tool_func, e, args, kwargs) -> NoReturn:
    context = None
    for arg in args:
        if isinstance(arg, FastMCPContext) or hasattr(arg, "tenant_id"):
            context = arg
            break
    for v in kwargs.values():
        if isinstance(v, FastMCPContext) or hasattr(v, "tenant_id"):
            context = v
            break
    tenant_id, principal_id = _extract_tenant_and_principal(context) if context else (None, None)
    _log_tool_error(tool_func.__name__, e, tenant_id, principal_id)
    _translate_to_tool_error(e)

---

Major

3. _handle_tool_error plain-ToolError fallback always returns HTTP 500

src/routes/api_v1.py:66-71

synthetic = AdCPError(error_message) uses status_code=500 (class default). Only error_code is overridden, not status_code. A plain ToolError("VALIDATION_ERROR", ...) would produce HTTP 500 + VALIDATION_ERROR — the status/code disagree and a buyer agent may retry a correctable error.

Limited blast radius since AdCPToolError now handles the primary path, but the mismatch is real for any plain ToolError that reaches this fallback.

Fix: Add a simple ERROR_CODE_TO_STATUS lookup for known codes (VALIDATION_ERROR->400, AUTH_REQUIRED->401, NOT_FOUND->404).

4. to_adcp_error() context location differs from envelope builder

src/core/exceptions.py:220-242

to_adcp_error() puts context in details["context"] (nested), while build_two_layer_error_envelope() puts it at top-level context. The test explicitly asserts this asymmetry. Not a bug — but now that the envelope builder is canonical, to_adcp_error() is effectively legacy.

Fix: Add deprecation note to to_adcp_error() docstring.

5. Lazy import + redundant recovery="correctable" in _assignments.py

src/core/tools/creatives/_assignments.py:77-82

AdCPValidationError is already imported at top level from the same module — no circular dependency risk. The explicit recovery="correctable" is redundant (it's the class default) and would silently prevent a future default change from taking effect here.

Fix: Move import to top level, drop the kwarg.

6. REST routes still catch ToolError defensively

src/routes/api_v1.py — every endpoint has try/except ToolError. Both paths produce the same wire shape, but the dual-path is unnecessary now that the app-level AdCPError handler exists. New routes must copy the pattern for no reason.

7. Test precision gaps

• test_error_format_consistency.py:28-60 — catches (AdCPValidationError, ToolError) with only assert len(str(error)) > 0. Now that boundary behavior is architecturally defined, pin the expected type.
• test_error_paths.py:356-399 — bare except Exception: pass makes the test vacuous. The targeted TestImportValidation tests already cover the import regression this was guarding.

8. Envelope assertion helpers duplicated across test files

_assert_two_layer_envelope (test_adcp_exceptions.py) and _assert_rest_envelope / _assert_mcp_envelope (test_error_boundary_translation.py) verify the same shape. A spec change to the envelope requires updating 3 helpers. Extract shared assert_envelope_shape() into a test helper module.

---

Nits

File	Note
tool_error_logging.py:141,158,174	f-string logging — prefer %s formatting
tests/harness/_base.py:73	_CODE_TO_CLASS dict relies on insertion order for AUTH_REQUIRED — consider explicit override
test_architecture_no_error_construction_in_impl.py:68	Cross-file import from sibling guard — consider moving to shared module
exceptions.py:448-463	UNSUPPORTED_FEATURE intentional spec divergence — track with issue number
conftest.py:575	60s startup wait justified by CI evidence but root cause unaddressed

---

What's working well

• Single source of truth for wire shape via build_two_layer_error_envelope(), enforced by AST structural guard with 1-level transitive call analysis
• Cross-transport roundtrip tests across all 11 AdCPError subclasses through MCP, A2A, and REST
• Per-file ratchet guards with DRY-extracted _per_file_cap_guard.py — caps-only-shrink is a great pattern
• _envelope_to_adcp_error unifies REST and A2A reconstruction — proven byte-identical by roundtrip tests
• AdCPToolError design carrying envelope + status_code eliminates 4xx-as-5xx mislabeling
• ContextManager.fail_step byte-identical-by-construction invariant with matching test
• _serialize_context shallow-copy defense against aliasing footguns, with explicit mutation test

[KonstantinMirin]

Code Review: PR #1306 — Final Round

All prior feedback (2 blockers + 6 majors) verified as resolved. The two-layer envelope architecture is solid — single source of truth via build_two_layer_error_envelope(), cross-transport symmetry fully achieved, DRY agent found zero violations and documented 9 improvements.

Four remaining items.

Major

1. _ERROR_CODE_TO_STATUS has phantom entries and missing wire codes

src/routes/api_v1.py:57-80

Three agents flagged this independently. The map mixes pre-translation codes, wire codes, and phantom codes:

Entry	Problem
CAPABILITY_NOT_SUPPORTED: 422	Not a wire code — the actual code is UNSUPPORTED_FEATURE
GONE: 410	Pre-translation code — the wire code is INVALID_STATE
RATE_LIMIT_EXCEEDED: 429	Pre-translation code — the wire code is RATE_LIMITED

Missing entries: UNSUPPORTED_FEATURE: 422, INVALID_STATE: 410, RATE_LIMITED: 429, INVALID_REQUEST: 400.

Low blast radius since AdCPToolError bypasses the map entirely, but a safety net with wrong entries is worse than documenting it as intentionally incomplete.

2. _adcp_to_a2a_error is dead code

src/a2a_server/adcp_a2a_server.py:143-166

Defined and updated (now builds two-layer envelope in its data dict) but never called from any production code path after the A2A refactor. _handle_explicit_skill now re-raises AdCPError directly. Either remove or mark # NOTE: currently unused.

3. extract_error_info widens RecoveryHint to str | None

src/core/tool_error_logging.py:109-110

AdCPError.recovery is RecoveryHint (a Literal), never None. The return type tuple[str, str, str | None] forces a type: ignore[assignment] downstream in _handle_tool_error. Using RecoveryHint | None as the third element would eliminate the suppression.

4. Lazy import of AdCPValidationError in media_buy_list.py

src/core/tools/media_buy_list.py:258

Inside an except ValidationError block. The module already imports from src.core.exceptions at top level. No circular dependency risk. Move to top-level imports for consistency with the rest of the PR.

What's working well

• Single source of truth for wire shape via build_two_layer_error_envelope() — enforced by AST structural guard with transitive call analysis
• A2A asymmetry eliminated — ValueError/PermissionError now wrap-and-re-raise to typed AdCPError subclasses, catch-all removed
• REST defensive catches removed — all 10+ per-route try/except ToolError blocks replaced by global exception handlers
• DRY: _handle_tool_exception extracts the 15-line shared error handling path from sync/async wrappers
• DRY: assert_envelope_shape consolidates 4 per-boundary assertion helpers into one canonical helper
• Per-file ratchet guards with caps-only-shrink enforcement for both Pattern A and ValueError migration
• _envelope_to_adcp_error unifies REST and A2A reconstruction — proven byte-identical by roundtrip tests