fix: A2A top-level failures return failed Task with AdCP envelope, not JSON-RPC InternalError

[numarasSigmaSoftware]

Problem

The A2A server surfaced application-layer failures on the JSON-RPC transport layer, emitted webhooks that contradicted the AdCP terminal-state rules, leaked raw exception text (credentials/connection strings) onto the wire, and lost the buyer's task identity across the async workflow boundary (poll/cancel/webhook correlation).

Spec Grounding (AdCP 3.1.1; prose tree rendered at dist/docs/3.1.1/)

• building/operating/transport-errors.mdx "Layer Separation": application/task errors (including unknown/unsupported skills, caught after routing) belong in the task response body as a failed Task with the two-layer envelope; JSON-RPC/MethodNotFound is reserved for transport faults and infra-before-dispatch. The table at transport-errors.mdx:11-18 lists internal crash under the transport layer, with no RFC keyword — an earlier revision of this description wrote "(SHOULD-level)", which the spec does not say, and described a split by WHERE the crash surfaces, which the code does not implement: _dispatch_under_sanitize_seam catches only (AdCPError, ValueError, PermissionError), so an untyped skill-handler crash reaches the same transport-layer InternalError as a boundary crash. The inner seam's own docstring states this correctly.
• building/operating/transport-errors.mdx "Security Considerations" § Seller Requirements: error responses MUST NOT include hostnames, database/SQL text, credentials, or upstream internals; suggestion carries generic guidance only. This grounds the boundary sanitizer (safe_adcp_error) including its synthesized suggestion.
• building/by-layer/L0/a2a-guide.mdx "Webhook Trigger Rules": no webhook is sent when the initial response is already terminal; webhooks fire only for non-terminal initial responses and later async transitions, which carry the full Task's artifacts. canceled is listed among the final states ("Cancellation confirmed").
• tasks/get / tasks/cancel semantics are A2A-protocol-native (A2A spec Task Management: TaskNotFoundError, TaskNotCancelableError for terminal tasks; SDK default_request_handler as reference cross-check) — AdCP prose defines no cancel contract of its own.
• building/operating/transport-errors.mdx JSON-RPC error.data: error.data is a sanctioned transport-envelope location and error.data.adcp_error is a MUST-check in the client detection order. This grounds the widened auth emission — every A2A method's auth rejection (message/send, tasks/get, tasks/cancel, the four push-notification-config arms) stays a JSON-RPC error AND attaches the two-layer envelope as data. "Stays on the JSON-RPC wire" and "carries the envelope" are orthogonal.
  Whether the buyer receives that data depends on the method name they call. The app runs with enable_v0_3_compat=True, and dispatch is selected by method name, so the v0.3 aliases reach a2a/compat/v0_3/jsonrpc_adapter.py, whose handle_request has no except A2AError arm — only except Exception -> CoreInternalError(message=str(e)), which takes no data. Measured on an auth rejection: GetTask/CancelTask return -32600 + the envelope; tasks/get/tasks/cancel return -32603 + data: null. The installed adcp client emits the v0.3 names, so that is the common path. Nothing leaks (the flattened message is the already-scrubbed text) but the buyer loses the code they are told to branch on. Tracked in A2A: the a2a-sdk v0.3 compat adapter flattens every typed A2AError to -32603, losing the spec's error codes #1670 and now graded in BOTH directions by _TASK_METHOD_DISPATCH in tests/unit/test_a2a_transport_contract.py — an earlier revision of this description asserted the envelope reached the wire without that qualification, and the test was parametrized over only the two v1.0 names that preserve data.
• Pinned 3.1.1 error-code enum (error-code.json enumMetadata): CONFIGURATION_ERROR is terminal recovery, suggestion "surface to a human at the seller — the buyer cannot resolve a seller-side deployment misconfiguration and MUST NOT auto-retry". Grounds _resolve_a2a_identity's no-tenant branch: an authenticated principal with no resolvable tenant is a seller-side deployment issue, not a buyer credentials problem, so it emits CONFIGURATION_ERROR rather than AUTH_REQUIRED for that specific condition. Note this is a known, currently-tracked divergence from require_tenant (src/core/auth.py), which raises AUTH_REQUIRED for the same underlying "no tenant" condition across its ~18 _impl call sites — reconciling the two is open follow-up work, not resolved by this PR.
• Pinned AdCP 3.1.1 error-code enum, read from the installed adcp SDK's own schema tree (adcp/_schemas/3.1/enums/error-code.json; adcp 6.6.0 -> spec 3.1.1): sellers MUST emit AUTH_MISSING when no standard Authorization header is present and AUTH_INVALID when that header is present but rejected. The implementation applies that split uniformly at A2A, MCP, and REST wire boundaries; direct _impl helpers retain deprecated AUTH_REQUIRED only when credential-presence state is unavailable. (Earlier revisions of this description grounded the split on a separately vendored schema tree — first at the v3.1.1 release tag 467fd93d, a citation that was wrong because the pin was advanced to the tag while only 2 of 245 vendored files had been re-fetched from it, then reverted to 04f59d2d5 with the two auth codes recorded as an explicit, content-checked supplement. fix: pin AdCP schema validation to the installed SDK and fix the wire-serialization bugs it surfaced #1868 has since made the SDK's own tree the single pin — enforced by tests/unit/test_pinned_schema_single_source.py — and that tree ships both codes with enumMetadata byte-identical to what was vendored, so the vendored tree and its supplement are gone and the SDK's enum is what the Spec-Grounding Gate reads.)
• Recorded conflict in the pinned sources: AUTH_REQUIRED is marked Deprecated in the pinned error-code.json enum, while authentication.mdx:290-297 still illustrates the unauthenticated case with it. This PR follows the enum (emitting the AUTH_MISSING/AUTH_INVALID split at the wire boundaries and retaining AUTH_REQUIRED only where credential-presence state is unavailable). Noting the conflict here so the next reader does not have to re-derive which side to take.
• Storyboard status for this split: ungraded — surveyed dist/compliance/3.1.1/universal/error-compliance.yaml, security.yaml and pagination-integrity-list-accounts.yaml; none carries an AUTH_* error-code scenario. Note security.yaml:95 (unauth_rejection) DOES grade the unauthenticated path — http_status_in [401, 403] and on_401_require_header: www-authenticate — so the surface is not entirely ungraded, only the code split is. git grep -in "www-authenticate" -- src/ returns zero at this head and at the base while AdCPAuthenticationError._default_status_code = 401; that gap is tracked separately. The normative enum mandate is graded by the in-repo cross-transport wire tests.

Changes

Error routing (application failures → failed Tasks):

• on_message_send's outer AdCPError arm returns the failed Task with the processing_error envelope artifact (TextPart + authoritative DataPart); the bare Exception arm re-raises as a sanitized JSON-RPC InternalError, and genuine A2AErrors still re-raise as JSON-RPC.
• Unknown and unimplemented skills raise typed AdCPCapabilityNotSupportedError (UNSUPPORTED_FEATURE/correctable) → surfaced as failed Tasks. MethodNotFoundError is reserved for unknown JSON-RPC methods. The unknown-skill check runs inside the logged boundary so record_boundary_error fires exactly once.
• Multi-skill messages are rejected before any skill runs (typed UNSUPPORTED_FEATURE failed Task): one skill per message until per-skill child Tasks land (A2A Task identity vs workflow-step identity: response Task and webhook Task differ #1614), so a sibling failure can never terminalize a Task that carries an accepted, persisted operation.

Boundary sanitization (one policy across A2A, MCP, REST, and webhook paths):

• safe_adcp_error is the shared scrub for A2A failed-Task / JSON-RPC, MCP, REST, and webhook push boundaries. It is: client-correctable typed errors pass through with their controlled message, EXCEPT AdCPValidationError, whose text is untrusted by default and opts in via _wire_safe_message=True (see "Buyer-visible message change" below); the internal/SERVICE_UNAVAILABLE bucket (base AdCPError, AdCPAdapterError+, AdCPConfigurationError) gets its message and suggestion replaced with static text (code/recovery preserved; suggestion synthesized so the envelope keeps the graded top-level suggestion); untyped exceptions never expose str(exc).
• The JSON-RPC layer uses the sanitized message too — _internal_error_for builds error.message from the scrubbed error, not the original, so a typed adapter error carrying a DB URL is clean in both error.message and error.data.
• Defense-in-depth scrubs at the reachable AdCPAdapterError(str(e)) raise sites (also protects MCP/REST).

Cross-transport authentication and observability hardening:

• A2A, MCP, and REST use one pinned oracle for AUTH_MISSING versus terminal AUTH_INVALID, including exact recovery and suggestion in both envelope layers.
• The A2A/MCP/REST harness can present a real rejected token without injecting a resolved identity; BDD and integration tests exercise production token resolution on every wire.
• Authentication and pre-auth validation failures never derive tenant-visible activity/audit scope from client-controlled Host or x-adcp-tenant hints; a resolved principal is required before tenant-scoped sink writes.
• Auth-optional discovery skills are defined once in the transport-neutral AUTH_OPTIONAL_SKILLS policy and shared by all three boundaries. list_accounts moves auth-optional -> auth-required on the boundaries that previously admitted it unauthenticated: accounts/tasks/list_accounts.mdx opens "Returns all accounts the AUTHENTICATED agent can operate on this vendor agent", and protocol/required-tasks.mdx marks it Conditional for account-id namespaces with require_operator_auth: true. Storyboard status: ungraded for auth — dist/compliance/3.1.1/universal/pagination-integrity-list-accounts.yaml grades pagination, not the auth requirement — so the in-repo cross-transport tests are what hold it.

Webhook / terminal-state contract:

• Immediate terminal responses return synchronously and send no webhook; only non-terminal submitted initial responses notify. The legitimate completed/failed webhook is the async workflow-step transition (context manager).
• _send_push_notifications sends exactly one webhook per step status change — the object-mapping and registered-config rows are opt-in gates, not per-item targets (they previously multiplied identical sends; caught live by the E2E single-completed-webhook pin).
• A single DataPart decoder feeds status detection and the webhook payload, so a webhook preserves all artifacts (duplicate names de-collided, never flattened to {"error": ...}).

Task identity correlation + durable task management (#1544 B6):

• The buyer's outer task_* id is threaded on_message_send → … → _create_media_buy_impl and persisted on the workflow step's request_data.external_task_id; completion webhooks key on it.
• tasks/get treats the persisted step as the source of truth: a non-terminal in-memory task is reconciled against the durable step, so an approval that completed in another process (or before a restart) is visible to the original poll. Terminal durable outcomes rebuild the Task with the stored result artifact.
• tasks/cancel is durable and race-safe: the transition is a single conditional UPDATE (cancel_if_cancellable, WHERE status IN cancellable — the pre-side-effect statuses; excludes approved/in_progress) so a cancel racing an approval can never overwrite a committed decision or strand a real order (zero-row ⇒ TaskNotCancelableError with the fresh status). Terminal in-memory tasks also refuse cancel.
• Durable get/cancel are scoped to tenant AND owning principal (get_by_external_task_id(…, principal_id=…)): a same-tenant sibling principal who learns a task id can neither read its response_data nor cancel its workflow.

Admin approval + policy routes (status mutation moved into the repository):

• Approve/reject no longer write WorkflowStep.status from the route. Every decision goes through one atomic conditional UPDATE in WorkflowRepository (claim_approval / reject_if_approvable / transition_if_nonterminal, all delegating to _atomic_transition), so two concurrent approvers cannot both run the adapter work: the loser gets zero rows → 409, never a silent second execution.
• APPROVABLE_STEP_STATUSES is the one definition of "can be approved" (requires_approval, pending_approval, and the legacy approval alias kept until the alias is normalized away, Normalize legacy 'approval' workflow-step status to canonical 'requires_approval' #1659). The media-buy detail route, the workflow routes and the workflows summary all read it instead of inline literals — a subset literal silently hid or undercounted live approvals.
• The policy review route is gated by require_tenant_access (a server-side membership check, replacing an inline role test that only blocked cross-tenant for one role), filters on step_type == "policy_review" so an arbitrary workflow step cannot be driven terminal through it, and fails loudly on an unknown action instead of falling through.
• The acting user is read one way for audit attribution (session_user_email), so an OAuth session is never recorded as a dict repr.

Tests

• tests/unit/test_a2a_error_routing.py: untyped crash → SERVICE_UNAVAILABLE; typed error keeps its code; auth-extraction failure → failed Task, no webhook; all-skills-failed preserves per-skill envelopes with no webhook; immediate completed → no webhook; multi-skill rejection pin.
• tests/unit/test_error_boundary_translation.py: internal-bucket typed message scrubbed (code/recovery preserved); client-correctable message preserved; full JSON-RPC wire scrub (error.message + error.data both clean for a typed adapter error carrying a secret); typed/untyped envelope shape parity.
• tests/unit/test_a2a_transport_contract.py: table-driven wire assertions across the full dispatch registry, registry↔test bijection, agent-card advertised-subset pin.
• Cross-transport auth tests drive real rejected credentials through A2A/MCP/REST, assert the pinned two-layer code/recovery/suggestion contract, keep anonymous REST validation errors unscoped, and prove the UC-002 no-disclosure path cannot leak account natural keys, ambiguity codes, match counts, or account details.
• tests/unit/test_context_manager_webhook_dedup.py: 2 mappings × 2 configs → exactly 1 send; both gates preserved (mutation-verified against the pre-fix loop).
• Integration (test_a2a_skill_invocation.py): durable get rebuild + tenant isolation + principal isolation (get & cancel); durable cancel persists and is visible to a later poll; terminal step/in-memory refuse cancel; stale in-memory WORKING reconciled to the terminal durable outcome via public on_get_task; cancel/approval race (deterministic TOCTOU interleaving on real PostgreSQL) preserves the terminal decision.
• BDD (@T-UC-002-ext-nl-unsupported) and E2E (test_a2a_webhook_payload_types.py): wire envelope + exactly one completed webhook (grace-window pin that caught the live duplicate); Task/TaskStatusUpdateEvent wire serialization pinned.
• Guard test_architecture_bdd_scenario_merge_durability.py: source-less scenarios must be marked AND registered (bijection scoped to the source-less candidate universe).

Verification

• make quality green; every production fix mutation-verified (revert each → its test reddens).
• Full integration, full BDD (fresh DB), and the E2E webhook file green on the Docker stack.
• Error-path oracles are single-sourced and self-tested: one strict failed-Task reader (artifact name, exactly one DataPart + one TextPart) feeding assert_failed_task_envelope, which delegates the envelope assertion to the canonical assert_envelope_shape so both layers must agree. The secret-leak oracle's token set (_SECRET_TOKENS) was previously the sole, ungraded definition of a leak — a self-test parametrized over that same constant would delete its own case instead of reddening when a token was dropped, silently defanging all ~30 downstream callers. test_secret_scrub_oracle.py closes that by stating the expected leak fragments (_EXPECTED_LEAK_FRAGMENTS) INDEPENDENTLY of _SECRET_TOKENS, so narrowing or emptying the shared set now reddens rather than passing vacuously. Each oracle has known-bad self-tests, so weakening a pin reddens something.

Buyer-visible message change

safe_adcp_error scrubs an AdCPValidationError's message by default, because business
validators frequently interpolate values a raise site never audited. That default is right for
interpolated text and pure collateral damage for a bare string literal: a constant interpolates
nothing, so there is nothing to leak, and genericizing it costs the buyer the one diagnostic they
could act on — for errors whose recovery is correctable, i.e. exactly the ones the buyer is
expected to fix themselves.

31 bare-literal raise sites had accumulated and were all silently downgraded the moment the scrub
landed. Nothing failed: the wire contract stayed valid (code/recovery/status/field all preserved),
only the human-readable half regressed, which no assertion covered. They now carry
_wire_safe_message=True; 39 sites opted in, 0 bare literals remain, and
test_architecture_static_validation_message_opt_in.py holds that at zero with a deliberately
EMPTY allowlist. Interpolated messages stay out of scope by design — those need a per-site audit
and keep scrubbing until it happens.

This changes bytes on the wire. Any consumer string-matching the generic scrub text
("The request could not be validated; review the submitted fields and resubmit.") will now see the
raise site's own message for these 31 conditions. That is the intent, but it is a behavior change,
stated here rather than shipped quietly.

Grounding (dist/schemas/3.1.1/core/error.json): message carries no wording constraint —
"Human-readable error message" is the entire schema description — and the file's own canonical
example echoes the buyer's rejected value in BOTH message and details.rejected_value, documented
there as "echoed for buyer-side diagnostic clarity". error-handling.mdx names only two
MUST-be-generic cases: not-found uniformity (:203), unestablished identity (:416-417), and seller internals (:519). None covers
a static literal.

details is withheld together with message on the scrubbed path, deliberately: both are built by
the same raise site from the same values with the same absent audit, so one flag governs both
channels. Forwarding normalize_to_adcp_error(exc).details there would re-emit the very payload the
branch exists to withhold — on that branch normalize_to_adcp_error returns the same instance it
was given. Both halves of the symmetry now have an oracle.

Also in this round

• Register/deliver agree on the webhook scheme. validate_protocol_webhook_url decided HTTPS
  from ENVIRONMENT == "development" while its two siblings use _require_https()/_strict_mode()
  — the two rules disagree in 7 of 10 environment combinations, always register-permissive /
  deliver-strict. docker-compose.yml never sets ENVIRONMENT, so on the default stack an http://
  reporting webhook registered with a success response and then silently never delivered. It was
  also the only one of the three that ignored ADCP_TESTING. This is the SCHEME axis of the same
  defect _matches_development_test_host already fixed on the HOST axis.
• "Waiting for 0 creative(s)". _approval_creative_gate returns (False, ()) when a buy has no
  creative assignments — unsatisfied precisely BECAUSE nothing is assigned — so the shared message
  told the operator to wait for an empty set. Zero now gets its own copy naming the action.
• _atomic_transition's tenant predicate has a failing oracle. Deleting it reddened nothing
  across the suite: test_architecture_workflow_tenant_isolation matches select(WorkflowStep),
  while that method scopes with select(WorkflowStep.step_id) and writes with update(WorkflowStep).
  It is the single tenant boundary for every conditional transition on the repository. The oracle
  asserts the OTHER tenant's persisted status, not the return value — get_by_step_id is itself
  tenant-scoped, so a cross-tenant UPDATE that applies still returns None. Widening the guard
  matcher is test: workflow tenant-isolation guard misses select(Model.column) and update(Model) — _atomic_transition escaped it #1803.
• The NL A2A step dispatches through the shared harness seam instead of its own handler,
  identity mocks, asyncio.run and TransportResult — four chances to drift, one of which it had
  already taken. The scenario also gained the assertion it was missing: that the failure returns as
  a FAILED Task rather than a JSON-RPC error, which is this PR's actual thesis and which asserting
  the error code alone cannot distinguish.
• Two re-inline guards strengthened. Both derived their fragment from the clause that had
  already drifted, so they caught only re-inlines of the wording that was already correct; and both
  scanned line-by-line, which a formatter-wrapped copy of a >120-char message escapes by
  construction. They now match on the drift-stable prefix, read literals off the AST, and share one
  helper.
• AUTH_OPTIONAL_SKILLS grounding corrected. The comment claimed required-tasks.mdx documents
  all four entries as Required discovery tasks. It documents three; list_authorized_properties was
  REMOVED from the spec in v3 and survives only in a migration note. Membership is unchanged (both
  transports already treated it as public), and the v2-compat lifecycle question is list_authorized_properties is a v2 task the spec removed in v3 — still implemented and still auth-optional #1804.

Follow-ups filed this round: #1803 (guard matcher), #1804 (v2-compat surface).

🤖 Generated with Claude Code

Multi-skill batching

AdCP 3.1.1 is silent on batching multiple application skills in one A2A message, and the 3.1.1 conformance storyboards do not grade it. This PR therefore treats one skill per message as a local side-effect-safety constraint until per-skill child Tasks are available: rejecting a batch before dispatch prevents a sibling failure from terminalizing a Task while accepted work continues.

[KonstantinMirin]

Review: correct fix, right spec grounding — but not mergeable until the behavior is tested at the right altitude

The core change is right and it's grounded. I verified it against the pinned
AdCP 3.1.0-beta.3 building/operating/transport-errors.mdx "Layer Separation"
table: application/task-execution failures belong in the task response body
(failed Task carrying the envelope); JSON-RPC / A2A protocol errors are reserved
for genuine transport faults. Returning the failed Task instead of raising
InternalError is what the spec mandates. Two more things check out: the envelope
shape is consistent by construction (both the per-skill and the new top-level
path route through the single _build_error_envelope →
build_two_layer_error_envelope(normalize_to_adcp_error(exc))), and the
untyped→SERVICE_UNAVAILABLE wire mapping is real (exceptions.py:51,
INTERNAL_ERROR: "SERVICE_UNAVAILABLE").

What's missing is not correctness of the mechanism — it's that the behavior this
PR changes is only covered by new unit tests, and the change touches error
routing, which this repo grades through the wire and grounds in a requirement.
Four points below; the fourth is the blocker.

1. The outer except Exception now absorbs genuine internal crashes, not just typed application errors

on_message_send's outer handler catches everything that isn't an A2AError —
typed AdCPError application failures and unexpected bugs in the wrapper's own
post-success code (artifact extraction / json.loads / webhook, lines 913–949) —
and maps all of them to a SERVICE_UNAVAILABLE failed Task carrying str(exc) on
the buyer wire. The spec's table lists "internal crash" under Transport (→
JSON-RPC), while RATE_LIMITED/CREATIVE_REJECTED-style failures are Application
(→ task body). This isn't a regression — the per-skill path
(_build_failed_skill_result) already did exactly this — so it's internally
consistent. But it's a deliberate choice (raw internal error text reaches the buyer
as an application-layer failure) and it should be stated explicitly in the PR body,
not left implicit.

Ask: one line in the PR description owning the decision. Server-side
observability is preserved via record_boundary_error, so I'd accept it as-is —
just make it explicit.

2. The two failed-Task paths were aligned at the envelope but not at the siblings (DRY)

The envelope body is DRY. The emission around it is duplicated and already
drifting:

• adcp_a2a_server.py:985 drops the failure reason on the webhook. The parallel
  all-skills-failed branch passes error="; ".join(error_messages)
  (:746); the new path calls _send_protocol_webhook(task, status="failed") with
  no error=. A webhook-driven buyer gets a bare failed with no reason, even
  though the reason is sitting in the envelope. This directly contradicts the
  "uniform regardless of which failure path" comment (true only for the artifact).
  This is a real behavior bug, not a nit.
• Artifact naming diverges (processing_error/error_1 at :978 vs
  error_result/skill_result_N at :728), and the new
  extract_processing_error_envelope helper hard-codes name == "processing_error".

Ask: extract a small shared step for the genuinely-identical operation —
"mark task FAILED + send failed webhook with the reason" — used by both :746 and
:985, so the reason is a required parameter and can't silently drift again. Keep
the artifact construction separate (multi-skill partial-failure artifacts vs. a
single synthesized envelope are genuinely different operations — don't over-unify
those).

3. The diff's own defensive change is untested

The identity = None hoist (:587–590) was added specifically to prevent a
NameError in the except path when auth extraction fails before resolution.
Reverting the hoist breaks zero tests (mutation-proven). The safety net has no
regression lock.

Ask: a test where identity resolution raises a non-A2AError, so the except
block runs with identity is None.

4. (blocker) The regression is at the wrong altitude, and the behavior isn't grounded in a scenario

All the new tests are unit tests. This is an error-routing behavior change; the
enforceable regression belongs where the rest of the A2A error contract already
lives — tests/integration/test_a2a_error_responses.py, which drives the real
on_message_send pipeline and asserts the two-layer wire envelope. This PR touched
zero integration/BDD files, and its stated verification (make quality) runs
unit tests only — so the integration suite that owns this behavior never ran.

Note the existing UNSUPPORTED_FEATURE scenarios
(BR-UC-002-create-media-buy.feature, e.g. @T-UC-002-ext-d) do not cover this
change: they fail through the skill-loop except, not the top-level except
this PR modified. Reverting the fix wouldn't break them.

Ask (grounded, source-of-truth first):

1. Add a new scenario (new ID, semantic-merge-safe) to
   BR-UC-002-create-media-buy.feature, grounded in transport-errors.mdx "Layer
   Separation":

   @T-UC-002-ext-nl-unsupported @extension @error @transport-layer-separation
   Scenario: NL media-buy request is unsupported -- returned as a failed task, not a transport error
     Given an authenticated buyer
     When the buyer sends a natural-language "create a media buy" request
     Then the error code should be "UNSUPPORTED_FEATURE"
     And the error recovery should be "correctable"
     # grounded: building/operating/transport-errors.mdx "Layer Separation"
     # drives the top-level on_message_send except -> failed Task envelope, not JSON-RPC

   This reaches the top-level handler (the NL create path raises
   AdCPCapabilityNotSupportedError), reuses the existing Then error code /
   recovery steps, and — because the A2A harness error-reader
   (tests/harness/_base.py:610–629) reads the failed-Task DataPart — it catches
   the regression on the A2A leg today: revert the fix and the assertion fails.

2. Wire it at integration now (real on_message_send), plus the matching case in
   test_a2a_error_responses.py.

3. File a GitHub issue to run the same scenario end-to-end across all transports
   once test: transport-aware BDD harness — retire e2e_rest ledger 312→7 (#1418) #1430 (transport-aware harness) lands, and reference that issue number in a
   code comment at the fix site. The scenario doesn't change when that happens —
   only the transport execution matrix widens.

The ticket

There's no linked issue. This fixes a real latent bug (the handler built the
correct envelope, then discarded it and raised InternalError), but nothing tracks
it and nothing grades it. Please link/file one, and record the spec citation
(transport-errors.mdx "Layer Separation") + "storyboard: ungraded, pending upstream
obligation" per the spec-grounding gate.

Sequencing

Don't block this 5-file fix behind #1430 (235 files). The integration test + the
BDD scenario wired at integration are enforceable today and independent of
#1430; the cross-transport e2e run is the only piece that waits, and it's covered by
the tracking issue above. Net: DRY/webhook fix + hoist test + the grounded scenario
wired at integration → mergeable.

[ChrisHuie]

Re-reviewed at 8fca519. The prior review's blocker is closed: the change is now regression-locked at both the integration and BDD altitude. I verified the locks by reverting the fix — the integration test then errors with InternalError: message processing failed: … and the BDD scenario fails Expected error code 'UNSUPPORTED_FEATURE', got 'InternalError'. The NL create a media buy path raises AdCPCapabilityNotSupportedError from _create_media_buy straight into the top-level except Exception, which is the exact handler this PR changed (the older UNSUPPORTED_FEATURE scenarios go through the skill-loop except and don't cover it). The BDD scenario runs live (PASSED), not xfail. Layer Separation grounding checks out against transport-errors.mdx §Layer Separation, and UNSUPPORTED_FEATURE = correctable is spec-correct. Structural guards (395) and CI are green.

Non-blocking, worth a quick pass:

1. tests/unit/test_a2a_nl_auth_redundancy.py:143-144 — the new comment calls UNSUPPORTED_FEATURE (correctable) a "documented intentional divergence." The published spec actually classifies it correctable (only the SDK's STANDARD_ERROR_CODES table says terminal), so there's no divergence — the value is spec-conformant. Suggest rewording to "correctable — matches AdCP error-code.json enumMetadata." (The exceptions.py docstring it points at has the same inversion, but that's out of this PR's scope and already being corrected separately.)

2. tests/a2a_helpers.py:73-77 — make_nl_send_message_request re-implements the Message body that create_a2a_text_message already builds, and the new integration/BDD sites call create_a2a_text_message directly. Consider return SendMessageRequest(message=create_a2a_text_message(text)) and using it at those sites so there's one builder.

3. Optional: the BDD Then steps assert on the reconstructed ctx["error"] rather than the ctx["wire_error_envelope"] the when-step already captures. The two-layer envelope shape is already enforced at the unit + integration altitude, so this is a hardening, not a gap — an extra Then over wire_error_envelope via assert_envelope_shape would close it at the BDD layer too.

Separately (pre-existing, not this PR): the protocol-webhook delivery path (protocol_webhook_service.py:296) posts the push-notification URL without the SSRF validation the application-webhook path uses (webhook_delivery.py:91), and the URL is captured before auth. Worth its own issue.

[numarasSigmaSoftware]

Addressed the three non-blocking items in e6ef756:

1. Comment reword — tests/unit/test_a2a_nl_auth_redundancy.py now says "correctable — matches AdCP error-code.json enumMetadata"; the "documented intentional divergence" phrasing is gone (agreed, the value is spec-conformant).
2. One NL builder — make_nl_send_message_request now returns SendMessageRequest(message=create_a2a_text_message(text)), and the integration test (test_a2a_error_responses.py) and BDD when-step (uc002_create_media_buy.py) call it instead of building the message inline. This also ratcheted the test duplication baseline 89 → 88.
3. BDD wire-envelope hardening — new generic Then step the wire error envelope should carry code "…" with recovery "…" (in then_error.py) asserts on ctx["wire_error_envelope"] via assert_envelope_shape, added to the @T-UC-002-ext-nl-unsupported scenario. Mutation-checked: flipping the expected recovery to terminal fails the scenario.

Verified: make quality (5199 passed), the integration case and the BDD scenario both pass against a real Postgres.

On the pre-existing protocol-webhook SSRF gap (protocol_webhook_service.py:296 vs webhook_delivery.py:91): confirmed — the protocol path has no WebhookURLValidator call at all. Filing it as its own issue.

[KonstantinMirin]

Review — PR #1547

Overview — The core fix is correct and well-grounded: an untyped top-level failure in on_message_send now returns a failed Task carrying the two-layer AdCP envelope instead of raising a JSON-RPC InternalError, exactly as AdCP 3.1.0-beta.3 building/operating/transport-errors.mdx §"Layer Separation" (dist/docs/3.1.0-beta.3/...) mandates. Every prior-round item is resolved — the webhook-reason drop is fixed via a shared _fail_task_with_webhook with a required error=, the identity=None hoist is now regression-locked, and the behavior is graded live at both the integration and BDD altitude (revert-verified by two maintainers). What remains is one within-scope DRY pattern: the "read the failed-Task envelope" operation is now implemented three different ways.

Should fix

Failed-Task envelope read is reimplemented instead of shared (DRY). This PR adds a new reader and a new hand-rolled dispatch that both duplicate logic that already exists, and it splits the read across test layers so the artifact-name contract is pinned in only one of them. Three sites of one pattern:

1. tests/a2a_helpers.py extract_processing_error_envelope re-implements the protobuf decode line json.loads(json_format.MessageToJson(part.data)) that is the entire body of the pre-existing tests/utils/a2a_helpers.py::extract_data_from_artifact. The new helper's real value is its assertions (artifact present + name == "processing_error" + single DataPart); the decode should delegate:

   # extract_processing_error_envelope(task): keep the assertions, then
   return extract_data_from_artifact(task.artifacts[0])

2. The reader is split by layer: unit tests call the strict extract_processing_error_envelope (asserts the processing_error artifact name), while the new integration test and the new BDD when-step read the same wire artifact with the looser extract_data_from_artifact and never assert the artifact-name / single-DataPart shape — so the contract this PR relies on is pinned only at the unit altitude, not at the two altitudes graded as the merge bar. After (1) makes the strict reader delegate, route the new integration/BDD sites through it so all three altitudes pin the same shape.
3. The new BDD when-step (tests/bdd/steps/domain/uc002_create_media_buy.py::when_buyer_sends_nl_a2a_request) hand-rolls the handler build + _get_auth_token/_resolve_a2a_identity injection + asyncio.run(on_message_send(...)) + the TASK_STATE_FAILED → extract_data_from_artifact → _envelope_to_adcp_error reconstruction — which is exactly the failed-Task branch of tests/harness/_base.py::_run_a2a_handler. The harness genuinely has no NL entry point today, so this is gap-driven rather than a lazy copy — but the reconstruction block will drift against the harness reader. Extract a shared _read_failed_a2a_task(task) -> (envelope, error) in tests/harness/_base.py and call it from both _run_a2a_handler and the when-step; ideally give the harness an NL dispatch path so the when-step dispatches through a transport per the project's "BDD dispatches through the harness" convention.

Call to action: land (1) as a trivial delegate, route the new sites through the strict reader for (2), and factor the shared failed-Task reader for (3) — so the next edit to the wire shape changes one place, not three.

Nice to have

• extract_processing_error_envelope and make_nl_send_message_request (a message builder) live in tests/a2a_helpers.py, while the established A2A message builders (create_a2a_text_message, create_a2a_message_with_skill) and extract_data_from_artifact live in tests/utils/a2a_helpers.py. Two importable modules share the basename a2a_helpers, so every import has to disambiguate by path. Co-locating the new NL builder / strict reader with the existing family makes A2A helper construction one grep target.
• extract_processing_error_envelope(task) -> dict is untyped on task and returns a bare dict; def extract_processing_error_envelope(task: Task) -> dict[str, Any] is more precise and matches the rest of the module.
• Untyped internal crashes normalize to base AdCPError → wire SERVICE_UNAVAILABLE with recovery="terminal", whereas that code's canonical recovery in dist/schemas/3.1.0-beta.3/enums/error-code.json is transient. This is not a spec violation — transport-errors.mdx §"Recovery Behavior" makes the explicit recovery field authoritative, and "don't retry a genuine bug" is a defensible per-instance choice — but a one-line note at the base _default_recovery citing that section would record it as deliberate rather than incidental.
• The all-skills-failed branch now routes through _fail_task_with_webhook("; ".join(error_messages)) but has no dedicated assertion that it still forwards the joined reason; the new top-level site is pinned, this leg isn't. A one-line error= assertion in the existing all-skills-failed test closes the single point where a future edit could silently drop that reason.

Notes / prior-review follow-ups

• Prior blocker (wrong altitude / not grounded in a scenario) — resolved: @T-UC-002-ext-nl-unsupported runs live on the A2A leg (not xfail) and the matching integration case drives the real pipeline; both assert on the wire envelope and were revert-verified.
• Webhook drops the failure reason (prior item #2) — resolved: both emission sites route through _fail_task_with_webhook(task, error) with error required.
• Untested identity=None hoist (prior item #3) — resolved: test_auth_extraction_failure_returns_failed_task_before_identity_resolution locks it.
• ChrisHuie's three follow-up items (comment reword, single NL builder, BDD wire-envelope hardening) — resolved in e6ef756a5; the NL builder now delegates to create_a2a_text_message and the duplication baseline ratcheted 89 → 88.
• A2A-only scoping of the BDD scenario is correct here (JSON-RPC-vs-Task is an A2A-only distinction); cross-transport widening is tracked in #1574 / #1430 and not re-raised.
• The protocol-webhook SSRF gap noted earlier is pre-existing and out of this PR's scope; being filed separately.

[numarasSigmaSoftware]

Addressed the round-2 review in 7d540d0.

Should fix — one reader, three altitudes:

1. Delegate the decode — extract_processing_error_envelope keeps the artifact-contract assertions (artifact present, processing_error name, and now explicitly single-DataPart) and delegates the decode to extract_data_from_artifact, per the inline suggestion.
2. Strict reader at all three altitudes — the integration test (test_nl_create_media_buy_unsupported_returns_failed_task_envelope) and the BDD when-step now read the failed Task through the strict reader, so the processing_error artifact-name/single-DataPart contract is pinned at unit, integration, and BDD altitude.
3. Shared _read_failed_a2a_task(task) -> (envelope, error) in tests/harness/_base.py, called by both _run_a2a_handler's failed branch and the when-step. One wrinkle surfaced while factoring: the harness branch reads skill-dispatch failures whose artifact is error_result (not processing_error), so the shared reader stays loose by default with an opt-in expect_processing_error=True used at the outer-handler call sites — asserting the name unconditionally would have retroactively tightened every existing harness A2A error test. The "give the harness an NL dispatch path" ideal is deferred per your call-to-action scoping; the when-step still builds the handler but the failed-Task read now has exactly one implementation.

Nice-to-haves — all four landed:

• The three PR-added helpers (extract_processing_error_envelope, make_nl_send_message_request, make_mock_a2a_identity) moved into tests/utils/a2a_helpers.py with the established builder family; tests/a2a_helpers.py keeps only the pre-existing make_a2a_context.
• Strict reader typed (task: Task) -> dict[str, Any].
• _default_recovery = "terminal" on the AdCPError base now carries a comment citing transport-errors.mdx §"Recovery Behavior" recording the deliberate divergence from error-code.json's transient for SERVICE_UNAVAILABLE.
• test_create_media_buy_validation_error_includes_errors_field (the all-skills-failed leg) now asserts _send_protocol_webhook was awaited with status="failed", error=<the joined per-skill reason>.

Verification: make quality 5199 passed / 8 skipped / 26 xfailed; test_a2a_error_responses.py 23 passed; test_idempotency_wire_matrix -k missing_key 2 passed (harness failed-branch canary); BDD uc002 -k unsupported 1 passed live; duplication ratchet unchanged (src 36 / tests 88 / scripts 0).

[numarasSigmaSoftware]

Follow-up hardening in a5248fe — three precision gaps I caught self-reviewing 7d540d0 before your next pass:

1. _read_failed_a2a_task strict mode couldn't pin "artifact present" — the if task.artifacts: guard sat in front of the expect_processing_error branch, so an artifact-less failed Task silently took the loose fallback instead of tripping the strict reader's first assertion, and the docstring overpromised that leg. Branches reordered so strict mode reaches extract_processing_error_envelope unconditionally; loose-read semantics unchanged (spot-checked both legs: AssertionError vs AdCPError fallback).
2. make_mock_a2a_identity typed -> ResolvedIdentity — it was the one unannotated helper left in the otherwise fully-typed tests/utils/a2a_helpers.py.
3. The "; ".join of webhook reasons is now revert-verifiable — the single-skill error= assertion is degenerate for the join itself (a join of one message equals that message), so test_all_skills_failed_webhook_carries_joined_reasons drives two failed skill invocations in one message and pins the joined reason. Mutation-verified: reverting the join to first-message-only fails it.

make quality 5200 passed / 8 skipped / 26 xfailed; integration file 23 passed; wire-matrix canary 2 passed; BDD uc002 scenario 1 passed.

[ChrisHuie]

Re-reviewed at a5248fe — all prior-round items addressed and diff-verified; the core fix is correct and regression-locked at both the integration and BDD altitude (both revert-verified). No blockers. Items by priority:

SHOULD-FIX — the new BDD scenario is dropped on the next feature recompile. BR-UC-002-create-media-buy.feature is a compiled file (# DO NOT EDIT -- re-run: python scripts/compile_bdd.py --merge). @T-UC-002-ext-nl-unsupported carries no @hand-edited marker and has no adcp-req target — it's the only scenario in the file with a spec-doc upstream_refs (transport-errors.mdx#Layer-Separation) rather than a BR-UC-002-* id — so classify_scenario_pair returns LEGACY-DELETE and the next --merge removes the scenario and its traceability entry, i.e. the BDD lock this PR adds. (The integration lock in test_a2a_error_responses.py is durable regardless.) Fix: add @hand-edited to the scenario, or move it to BR-UC-002-manual-overrides.feature and repoint the traceability adcp_feature.

SHOULD-FIX / FOLD-IN — DRY on the new test helpers.

• make_mock_a2a_identity() is adopted in two files but three byte-identical copies remain: test_a2a_transport_contract.py:25 (where this PR added a new consumer of _MOCK_IDENTITY) and test_a2a_create_media_buy_push_config_validation.py:59,128. Route them through the helper.
• make_nl_send_message_request is aliased _make_nl_request / _make_nl_message in two files; call it directly so one grep finds the family.
• make_mock_a2a_identity returns a real factory-built identity, not a unittest.mock — the name is misleading; consider renaming.

NIT — the strict-mode reorder in the last commit has no test. _read_failed_a2a_task(..., expect_processing_error=True) now raises AssertionError on an artifact-less failed Task, but nothing exercises that leg (reverting the reorder reddens no test). A ~6-line pytest.raises(AssertionError) for the artifact-less strict read (plus a loose-mode counterpart) pins it.

— the outer-handler comment overstates the spec. The comment (and PR body) say application/task-execution failures "MUST" return in the task response body, but the Layer Separation table lists "internal crash" under the transport (JSON-RPC) layer. The typed path (AdCPCapabilityNotSupportedError → UNSUPPORTED_FEATURE/correctable) is spec-conformant; the untyped-internal-crash case is a deliberate choice on genuinely ambiguous ground (the two-layer rule is SHOULD-level). Worth narrowing the comment to typed/application failures and labeling the untyped-crash routing an explicit decision. Confirmed accurate and unchanged: the _default_recovery="terminal" comment on the base AdCPError — the explicit recovery field is authoritative per §Recovery Behavior, so the deliberate deviation from error-code.json's transient is spec-permitted.

Pre-existing, out of scope for this PR (worth a dedicated cleanup). The ~16× inline auth-mock setup in test_a2a_error_responses.py (identity + mocked _get_auth_token/_resolve_a2a_identity + set_current_tenant) wants a shared authed_handler fixture; A2A test-identity construction is duplicated across ~24 files that each define their own _make_identity; and test_a2a_error_responses.py reads envelopes three ways via instance-method shadows of imported utils. The SERVICE_UNAVAILABLE terminal-vs-transient recovery split and raw str(e) on the buyer wire are also pre-existing.

[numarasSigmaSoftware]

Addressed the round-3 items in c51e76c:

SHOULD-FIX — BDD scenario dropped on recompile: went with your first option — @T-UC-002-ext-nl-unsupported now carries @hand-edited (plus a # HAND-EDITED comment explaining why: no adcp-req id until #1574 lands), which classify_scenario_pair maps to LEGACY-PRESERVE. Verified directly with the compiler's own classifier (classify_scenario_pair(scenario, None) == "LEGACY-PRESERVE"). Since a silently-dropped tag would silently drop the scenario again, there's also a new guard — tests/unit/test_bdd_scenario_merge_durability.py — that runs the classifier against the compiled feature on every make quality, with a registry list for future hand-maintained scenarios. (I considered the BR-UC-002-manual-overrides.feature route, but it requires a new scenario id + binding + traceability repoint for the same durability the marker gives.)

SHOULD-FIX — helper DRY: all three byte-identical copies now route through the shared helper (test_a2a_transport_contract.py, both sites in test_a2a_create_media_buy_push_config_validation.py); the _make_nl_request/_make_nl_message aliases are gone (direct calls); and the helper is renamed make_test_a2a_identity per your note — it returns a real factory-built identity, so "mock" was wrong.

NIT — strict-mode reorder untested: added the two direct pins — strict + artifact-less failed Task raises AssertionError (mutation-verified: restoring the guard-first branch order fails it), loose + artifact-less returns the (None, AdCPError) fallback.

Comment/PR-body overstatement: the outer-handler comment now scopes the MUST to typed application failures per the Layer Separation table and explicitly labels the untyped-internal-crash routing a deliberate choice on SHOULD-level ground; the PR body's Spec Grounding paragraph is updated to match. Thanks for confirming the _default_recovery note.

Pre-existing items: filed as #1597 (authed_handler fixture, ~24-file identity duplication, envelope-reader shadows, and the recovery-split / raw-str(e) design questions) so the deferral is tracked rather than silent.

Verification: make quality 5203 passed / 8 skipped / 26 xfailed; test_a2a_error_responses.py 23 passed; BDD uc002 unsupported scenario 1 passed live with the new tag; idempotency wire-matrix canary 2 passed.

[ChrisHuie]

Re-review — verified at head 51d7b331

The fix is correct and the branch is now a clean fast-forward on main (it fully contains origin/main). on_message_send's outer except stores-and-returns the failed Task carrying the two-layer envelope in the processing_error DataPart instead of raising InternalError; genuine A2AErrors still re-raise as JSON-RPC (src/a2a_server/adcp_a2a_server.py:952), typed AdCPErrors keep their own wire code, and untyped exceptions normalize to SERVICE_UNAVAILABLE/terminal. The behavior is regression-locked at three altitudes — unit (test_a2a_error_routing.py), integration (test_a2a_error_responses.py::test_nl_create_media_buy_unsupported_returns_failed_task_envelope), and BDD (test_nl_mediabuy_request_is_unsupported__…) — and re-inserting the pre-fix raise reverts all three to red. The reworded B1 comment matches the spec: the AdCP 3.1.0-beta.3 Layer Separation table lists "internal crash" under the transport layer and the two-layer rule is SHOULD-level, so routing untyped crashes to the failed-Task body is correctly labeled a deliberate choice, not a mandate. Every prior-round item is addressed.

Nothing blocks merge. Six polish items to fold into this PR, then one pre-existing follow-up.

Rename residual — _MOCK_IDENTITY (tests/unit/test_a2a_transport_contract.py:25). The helper is now make_test_a2a_identity, and both test_a2a_error_routing.py and test_a2a_nl_auth_redundancy.py renamed their module var to _TEST_IDENTITY, but this file kept _MOCK_IDENTITY (used at ~10 return_value=_MOCK_IDENTITY patch sites). It's the literal _MOCK_ label on a real factory identity — the exact thing the rename set out to remove.

-_MOCK_IDENTITY = make_test_a2a_identity()
+_TEST_IDENTITY = make_test_a2a_identity()

(plus the ~10 return_value=_MOCK_IDENTITY references in the same file)

The new merge-durability guard (tests/unit/test_bdd_scenario_merge_durability.py). Three small things on the guard itself:

• No test_architecture_ prefix, so it falls outside the tests/unit/test_architecture_*.py guard glob (it still runs via the general unit suite, but isn't catalogued with the other guards). Rename to test_architecture_bdd_scenario_merge_durability.py.
• The failure message names only the tag, but _has_hand_edited_marker also preserves on a # HAND-EDITED comment:

-                f"compile_bdd.py --merge would delete it. Restore the @hand-edited tag."
+                f"compile_bdd.py --merge would delete it. "
+                f"Restore the @hand-edited tag or a # HAND-EDITED comment."

• HAND_MAINTAINED_SCENARIOS is a static list, so a future hand-added scenario that forgets a marker would be dropped by --merge while this guard stays green. A bijection check hardens it with no adcp-req sources: assert every scenario under tests/bdd/features/ carrying @hand-edited/# HAND-EDITED appears in the list, and every list entry still exists and still carries a marker. (Today the mapping is 1:1, so the guard is correct as written — this closes the "added a marked scenario, forgot to register it" gap.)

.duplication-baseline. Committed tests: 88, but the live pylint R0801 count at this head is 87 — the main merge reshaped tests/conftest_db.py/tests/bdd/conftest.py and dropped a block, and the baseline wasn't regenerated. check_code_duplication.py reports -1 fixed and rewrites the file. Regenerate and commit tests: 87 so the next pre-commit run doesn't dirty it.

Two spec-precision comment sweeps.

• _internal_error_for's docstring (src/a2a_server/adcp_a2a_server.py:155-158) still attributes the untyped-normalized fallthrough to "per … Layer Separation … RETURNS a failed Task" without the deliberate-choice hedge the outer comment now carries. Scope the Layer-Separation attribution to typed application failures, or add the same one-clause hedge for the untyped-normalized case.
• The untyped test comment (tests/unit/test_a2a_error_routing.py:68-69) explains the INTERNAL_ERROR → SERVICE_UNAVAILABLE mapping but not that terminal is the deliberate base-class divergence from that code's canonical transient, documented in src/core/exceptions.py. One clause cross-referencing it keeps a future recovery-taxonomy cleanup honest.

Follow-up (pre-existing, not this PR). extract_processing_error_envelope's three artifact-shape asserts (tests/utils/a2a_helpers.py:75-77) are dead-defensive — no test feeds a malformed artifact, so deleting any one reddens nothing (only the artifact-less leg is now oracle'd, via the new A3 tests). It predates this PR and belongs with the test-helper consolidation tracked in #1586, not here.

--- Verified at 51d7b331:

Ran: git merge-base --is-ancestor origin/main HEAD (PR contains main, clean fast-forward, git merge-tree no conflicts); architecture guards 395 passed / 0 failed; new/changed unit tests 73 passed; integration test_a2a_error_responses.py 23 passed; BDD nl-unsupported node passed; Mutation — re-inserted the pre-fix raise → integration + BDD both red; Mutation — reverted the _read_failed_a2a_task branch order → strict reader oracle red, loose green; Mutation — stripped the scenario's @hand-edited/# HAND-EDITED markers → durability guard red ("LEGACY-DELETE"); check_code_duplication.py → tests: 87 live vs 88 committed.

Not run: full e2e/admin suites (rested on green CI); an untyped crash was not executed to observe a real str(e) payload (the pre-existing sink-sanitization item, tracked in #1587); the conformance storyboard YAML was not opened — the untyped path's "ungraded" status rests on the scenario's own note, and the graded typed UNSUPPORTED_FEATURE/correctable path is confirmed conformant.

[numarasSigmaSoftware]

Addressed all six polish items in 4a5ee33d6.

1. _MOCK_IDENTITY rename residual — renamed to _TEST_IDENTITY in tests/unit/test_a2a_transport_contract.py (declaration + the ~9 return_value= sites), finishing the make_test_a2a_identity rename so no _MOCK_ label survives on a real factory identity.

2. Merge-durability guard — three changes:

• Renamed to tests/unit/test_architecture_bdd_scenario_merge_durability.py so it's catalogued in the test_architecture_*.py glob.
• Failure message now names the # HAND-EDITED comment path alongside the @hand-edited tag (both are markers per _has_hand_edited_marker).
• Added a bijection check: every @hand-edited/# HAND-EDITED scenario under tests/bdd/features/ must be registered in HAND_MAINTAINED_SCENARIOS, and every registry entry must still exist and carry a marker. This closes the "added a marked scenario, forgot to register it" gap the classify check can't see. Routed through the shared assert_violations_match_allowlist() helper (the no-handrolled-allowlist-diff meta-guard flagged the inline stale = registry - marked set-diff, and the helper is exactly that bijection). Mutation-verified: emptying the registry while the scenario stays marked on disk turns it red ("new violations (1)").

3. .duplication-baseline — regenerated; tests: 88 → 87. Confirmed your read: the main merge reshaped conftest_db.py/bdd/conftest.py and dropped a block, so check_code_duplication.py reported -1 fixed and rewrote the file.

4. Spec-precision comment sweeps — both:

• _internal_error_for docstring (src/a2a_server/adcp_a2a_server.py): scoped the Layer-Separation attribution to typed application failures and added the deliberate-choice hedge for the untyped-normalized case (SHOULD-level; the table lists internal crashes under the transport layer), matching the outer handler's framing.
• Untyped test comment (tests/unit/test_a2a_error_routing.py): added a clause noting terminal is the deliberate base-class divergence from SERVICE_UNAVAILABLE's canonical transient, cross-referencing the _default_recovery note in src/core/exceptions.py.

Follow-up (not this PR) — left the dead-defensive artifact-shape asserts in extract_processing_error_envelope for the #1586 test-helper consolidation, as you noted.

make quality: 5204 passed, 8 skipped, 26 xfailed. Architecture guards: 363 passed.

[numarasSigmaSoftware]

Addressed both blockers, the should-fix, and the nit in 1bc418518.

[BLOCKER] Skill failures escaping as JSON-RPC — fixed.
Unknown-skill (MethodNotFoundError) and unimplemented-skill (UnsupportedOperationError: approve_creative, get_media_buy_status, optimize_media_buy, create_creative, assign_creative) now raise typed AdCPCapabilityNotSupportedError (UNSUPPORTED_FEATURE/correctable). The message/send method is valid and the failure is inside skill dispatch, so per "Layer Separation" it rides in the task body — the dispatcher wraps each into a failed-skill result, which also fixes the multi-skill hazard (earlier successes are preserved, not discarded by a bubbling A2AError). MethodNotFoundError is now reserved for unknown JSON-RPC methods only (its import is removed since nothing else uses it); the three RPC-method-level UnsupportedOperationErrors (list_tasks / resubscribe / extended-card) stay JSON-RPC. Added table-driven wire assertions across the dispatch registry in test_a2a_transport_contract.py; the old tests at :220/:451 now assert the failed-Task contract. Revert-verified: reverting one stub to UnsupportedOperationError turns the stub-table test red.

[BLOCKER] Second, malformed webhook — fixed.
_fail_task_with_webhook is gone; immediate failure paths now call _mark_task_failed (marks FAILED, no webhook) and return the terminal Task synchronously — per a2a-guide.mdx "no webhook is sent when the initial response is already terminal." _send_protocol_webhook no longer flattens to {"error": str}: for a failed final state it forwards the Task's structured artifact (the two-layer envelope), so a genuinely async failure webhook preserves the artifacts exactly. Added the two tests requested — a zero-delivery test for immediate failure and a serialized-payload test for async failure — both revert-verified (re-adding the webhook / re-flattening the payload each turn red). Updated the integration webhook assertion to assert_not_awaited().

[SHOULD-FIX] Durability guard blind spot — fixed.
Added test_traceability_candidates_are_marked_and_registered, which derives the candidate inventory from the independent bdd-traceability.yaml (scenarios grounded in spec prose — upstream_refs → *.mdx — which have no adcp-req render and would be LEGACY-DELETEd unmarked) and requires each to be both marked and registered. This sees exactly the case the marker-derived checks can't: a scenario missing BOTH a marker and a registry entry. Revert-verified against a spec-grounded orphan row: an unregistered/unmarked candidate now fails the guard.

[NIT] Untyped-crash test overstatement — fixed.
The test_untyped_processing_failure_... docstring now states the injected RuntimeError is an internal crash the spec table classifies as transport-layer, and that returning a failed Task is the project's deliberate uniform-envelope choice, not a mandatory rule — matching the production comment.

Also updated every test that codified the old JSON-RPC/webhook behavior (test_error_format_consistency, test_a2a_handler_correctness, test_a2a_skill_invocation).

Verification: make quality → 5206 passed / 8 skipped / 26 xfailed; a2a integration (43) + BDD nl-unsupported green; duplication baseline unchanged (tests 87); all four new pins mutation-verified.

[numarasSigmaSoftware]

Verified all seven findings against the code and fixed them in acfe455ce.

[BLOCKER] Immediate completed → duplicate webhook — fixed. on_message_send now notifies ONLY for a non-terminal (submitted) initial response; immediate completed/failed return synchronously with no webhook. Confirmed via E2E that the completed Task webhook a buyer receives is the legitimate async workflow-step completion (context manager, task_id=step_*) — on_message_send's was a true duplicate of it, which is what the a2a-guide terminal-state rule forbids. Unit-pinned (test_immediate_completed_task_sends_no_webhook, mutation-verified); the E2E completed/task-field tests now target the async workflow webhook and pass 10/10 on the stack.

[BLOCKER] Mixed submitted+failed dropped the failure envelope — fixed. Confirmed: the submitted scan short-circuited before artifacts were built (returning SUBMITTED + zero artifacts). Artifacts for all results are now built first; status is chosen by precedence failed > submitted > completed (preserve every result), so a mixed batch is terminal-failed with both the UNSUPPORTED_FEATURE envelope and the pending result preserved. Regression-tested in both orders; mutation-verified.

[SHOULD-FIX] 13/18 registry oracle — fixed. Hoisted the dispatch map to _skill_handler_map(); added a registry↔test bijection guard over all 18 skills, drove create_creative/assign_creative with valid params to their terminal UNSUPPORTED_FEATURE branch, and pinned the deliberate agent-card advertised subset (those two are registered but intentionally unadvertised).

[SHOULD-FIX] Unknown skills bypassed observability — fixed. Confirmed the check raised before the logged try. Moved it inside; record_boundary_error now fires exactly once (asserted with assert_called_once_with, mutation-verified).

[SHOULD-FIX] Lossy final webhook + duplicate decoders — fixed. Replaced _first_artifact_data (first DataPart only) with _task_artifacts_data (ALL artifacts), shared by status detection and the webhook payload. The async-failure test now asserts on the real serialized wire Task, not the mocked builder.

[SHOULD-FIX] Guard ignored YAML/JSON-grounded scenarios — fixed. Candidate discovery is now extension-agnostic (any spec-artifact ref — .mdx/.yaml/.yml/.json or URL). Mutation-verified against a webhook-emission.yaml-grounded orphan — the exact case added now goes red.

[NIT→SHOULD-FIX] PR description — updated to match the head: no webhook for immediate terminal, skill failures → typed failed Tasks, current helpers, refreshed spec-grounding + verification.

Verification: make quality unit 5228 passed; a2a integration 43; BDD nl-unsupported; E2E webhook-payload-types 10/10 on the Docker stack; duplication baseline unchanged; every production change mutation-verified (revert → its test reddens).

[numarasSigmaSoftware]

Merged main at 61611c317. The branch was CONFLICTING; it is MERGEABLE again at c0d7d2a71.

The collision was semantic, not textual. #1868 replaced the independently vendored schema fixture tree with the installed SDK's own tree and added tests/unit/test_pinned_schema_single_source.py, which fails if pinned_schema.py resolves anywhere outside the adcp package. This branch had built the opposite: a 246-file snapshot at 04f59d2d5 plus a hand-declared supplement carrying the 3.1.1 AUTH_MISSING/AUTH_INVALID split. So the diff now shows 244 deleted schema files — that deletion is #1868's, arriving through the merge, not a choice made here.

Taking #1868's direction also removed the reason the supplement existed. Measured before migrating:

• recovery is identical across all 66 shared codes (0 divergences)
• suggestion diverges on exactly 4 — MEDIA_BUY_NOT_FOUND, PACKAGE_NOT_FOUND, REQUOTE_REQUIRED, CREDENTIAL_IN_ARGS — and no consumer of these helpers reads any of them
• AUTH_MISSING and AUTH_INVALID exist only in the SDK's enum, with enumMetadata byte-identical to what was vendored

So the SDK tree is not merely an acceptable source for the auth split, it is the only one that can grade it at all. pinned_error_code_metadata() / pinned_error_code_suggestion() now sit on top of #1868's load(); the seven consumers are unchanged. The vendored tree, _manifest.py, and the supplement are gone, which retires the pin-fidelity correction the description had been carrying — that paragraph is rewritten accordingly.

Three resolutions took main over this branch on merit:

• products.py — fix: pin AdCP schema validation to the installed SDK and fix the wire-serialization bugs it surfaced #1868 removed the property-list try/except outright, because it relabelled implementation faults as AdCPValidationError with recovery="transient" against the enum's correctable. This branch had only stopped it interpolating str(e). Removing the handler subsumes the scrub.
• adcp_a2a_server.py — _reconstruct_response_object is deleted: rebuilding an outbound payload handed pydantic before-validators a reference to the dict about to go on the wire (the list_creatives format_id defect). This branch's failure arm survives alongside fix: pin AdCP schema validation to the installed SDK and fix the wire-serialization bugs it surfaced #1868's success arm, so both now read the payload rather than re-derive it.
• test_a2a_skill_invocation.py — kept fix: pin AdCP schema validation to the installed SDK and fix the wire-serialization bugs it surfaced #1868's new test_artifact_text_part_is_the_data_part_message, which grades exactly that resolution, and dropped the artifact loop this branch had already made dead by rejecting multi-skill messages up front.

One guard was deleted rather than repaired: four of test_pinned_schema_provenance.py's five tests graded the supplement and the manifest digests, which no longer exist. The fifth pinned a property that survives — that the auth codes production emits are in the vocabulary the oracle grades against — and moved to test_architecture_error_recovery_enum_conformance.py instead of going out with the file.

The merge surfaced one real defect, in c0d7d2a71. This branch widened _prepare_rest_request to return (client, identity, headers) so a presented-token test drives the real production dependency path; #1868 added CapabilitiesEnv._run_rest_request against the older two-value contract. Both merged cleanly, and every REST capabilities call then raised too many values to unpack. It presented as an assertion about success rather than an arity error, because the harness records the exception into TransportResult.error. Forwarding the headers is the half that matters beyond the arity: a presented token is carried only by those headers, so unpacking correctly while dropping them would send the request unauthenticated and grade the wrong path. Swept the other REST dispatchers — _base and media_buy_dual already unpack three and forward, and media_buy_dual's override delegates to one of them — so this was the only site.

Local verification on c0d7d2a71: make quality 5948 passed · tests/harness 148 passed · integration 2339 passed, 0 failed, 0 errors · BDD 1750 passed, 0 failed (5447 xfailed, 25 xpassed). The integration run's test_creative_agent_live.py errors are the CREATIVE_AGENT_URL env gate, not a regression — all 20 pass with scripts/creative-agent-stack.sh up. e2e and admin were not run locally; CI covers both.

[numarasSigmaSoftware]

Round addressed at fec0a3b82. 11 commits since the reviewed head; five of the should-fix items were already fixed at the time of the review (it was three commits behind), which the ledger below marks explicitly.

Already landed before this round

• a2a_task_id threaded for one skill / false CANCELED — 9eaa7a107, 17a0c2916. _ASYNC_TASK_SKILLS is now {create_media_buy, sync_creatives, update_media_buy}; external_task_id persists through request_metadata on all three; on_cancel_task refuses with "no durable workflow step is associated with this task id" rather than fabricating CANCELED. Your update_media_buy scoping call: it is the third async skill and it had the same gap, so it is in. Unifying the set also turns on push-notification injection for it — deliberate, since the set is defined by what a skill can RETURN and update can answer submitted.
• Approved-execution eligibility across four sites — c456e753e. prepare_media_buy_approval_execution owns the whole decision; all four literals are gone; NOT_EXECUTABLE is now distinct from CLAIM_REFUSED. The four sites were not four slices of one predicate: creatives.py excluded pending_approval deliberately, because a creative approval must not stand in for a human one. That split now lives beside the canonical set with its reasoning and is derived through execution_source_statuses_for(trigger).
• SSRF refusal on delivery writes no record — 9eaa7a107. Writes the delivery-log row and the audit entry before returning, and the validator's reason is no longer discarded.
• Retry exhaustion indistinguishable from nothing-to-do — 9eaa7a107. exhausted_message is required at all three call sites and the exhausting exception is logged with it.
• Webhook host axis — you were right that the round unified the scheme axis and split the host axis. My attempt to close it by unifying onto registration's side was wrong in the dangerous direction and I reverted it: validate_protocol_webhook_url is also the buyer-supplied callback gate in create_media_buy, so relaxing it widened an SSRF boundary — test_create_media_buy_callback_validation passes at c0d7d2a71 and fails at 9eaa7a107. Behaviour is back to the reviewed state. What survives is the docstring correction (the claimed symmetry is false) and a test that characterizes the divergence in both directions: delivery must never be the permissive gate, and with ADCP_TESTING off the two must agree. Closing it properly means tightening REGISTRATION.

Fixed this round

• Creative gate admits a status that cannot exist — dea3df825. You are right that active is not a creative status; the local enum is {processing, approved, rejected, pending_review} and no producer writes it. Removed from both homes (_approval_creative_gate and the retroactive-push gate). The test that supplied the value directly is gone; the row now sits on the refusal side, and restoring active to the gate reddens it.
• Push-notification validator on create, absent on update — 80cde6937. update_media_buy now routes through the same validator, same error shape and field. Deleting the call reddens the new wire test on a2a, mcp and rest.
• Two oracles with no known-bad self-test — 5b9f3a20c. Confirmed both before fixing: weakening the equality pins to truthiness left 159 green, and narrowing the failed-Task surface to the envelope left all 62 routing tests green. Both now have known-bad tests with counter-controls. On the second, the mutation reddens only the new test while all 62 routing tests stay green — which is the point.
• complete_task pre-check narrower than its guard — e4293de69. Derived from TERMINAL_STEP_STATUSES instead of a literal; pending_approval and the legacy approval alias are accepted. Restoring the literal reddens three cases.
• A2A skills drop parameters MCP and REST forward — 1bf9718c0, 00674542d. Larger than the three you named: the update skill forwarded 11 of 18. Now forwards idempotency_key, reporting_webhook, ext, flight_start_date, flight_end_date, currency, pacing, daily_budget. targeting_overlay and creatives are deliberately NOT forwarded — update_media_buy_raw accepts them but drops them before _build_update_request, so forwarding would be silent no-op plumbing, and the REST route omits both for that same documented reason. Same on create for paused. Those exclusions are now the only hand-written list, each entry carrying its reason and asserted to still exist in the live signature; the rest of the test derives from the signature, since a hand-list is how this was missed twice.
• Principal scoping enforced at one of four sites — fec0a3b82. Absorbed rather than deferred. get_by_step_id_or_raise, list_by_tenant and count_by_tenant take a required keyword-only principal_id; _principal_scoped_steps() is the single home for the predicate and get_by_external_task_id was routed through it rather than left as a fourth hand-rolled copy. get_by_step_id is deliberately left unscoped — all 8 callers were inspected and each already establishes authorization before reaching it. Verified with two mutations, because with a required keyword the call-site red is uninformative: dropping the keyword only proves the signature works, so the second mutation removed DBContext.principal_id == principal_id from the query itself, which reddens 5 of 7 isolation tests with data-level failures while the tenant-isolation tests stay green.
• Stale entries adjacent to rewritten lines — 00ca30732. The "no MCP-specific entries remain" comment now describes the one entry that exists; the UC-011 header with zero live entries under it is gone. Two other blocks still carry that local id and have live entries — I did not invent a GitHub number for them; see the follow-up note.
• log_safe applied inconsistently — 475f87bcc. All identifier interpolation in the creative-approval dispatch now goes through log_safe, so no identifier is raw at one site and wrapped at another.
• Dead approval service logs a refused transition as success — a52f1ac15. This one is ours: the round changed both sites from update_status (always wrote) to transition_if_nonterminal (can return None), which is what made the unconditional success log capable of lying. Deletion was preferred but ruled out — test_thread_registry_reaper.py drives the reaper through this module's live accessors — so both sites now branch on None and log the refusal.
• 10 of 13 nits — 6b1a47ef5. Three were not applied and each hit a stated stop condition rather than being skipped silently: the app.py indirection cannot be made guard-visible without growing a shrink-only allowlist; cancel_if_cancellable is answered below; deleting the dead approval service breaks an import outside the change's scope.

One correction

cancel_if_cancellable returning bool — the diagnosis is right, the stated motivation is not, and the motivation is what the fix rests on. The comment says the bool "forces the A2A caller to re-SELECT what the repository already knew." It did not know it. _atomic_transition ends:

if updated is None:
    return None
return self.get_by_step_id(step_id)

The refusal path returns before loading anything, and the caller's re-SELECT is on the refusal branch, after a session.rollback(). Returning the step would yield None there — exactly what False yields — so the re-SELECT cannot be deleted, and deleting it is the change's whole purpose. Removing it another way would need either a change to _atomic_transition (shared with four siblings) or dropping the fresh status from the TaskNotCancelableError, which the contract requires. Declining the consistency-only rename as churn on a security-sensitive CAS; happy to take it if you still want it.

Filed, self-assigned

• Extend boundary completeness guard to A2A skill handlers + extract to_account_reference helper #1259 REOPENED — boundary-completeness guard. It was closed as completed in June, but TOOLS_DIR = Path("src/core/tools") and src/a2a_server/ is not scanned, so its stated goal was never achieved. The parameter-forwarding item above is the third recurrence of the class this issue exists to stop, and its own body says the guard hole is why they keep slipping through.
• BDD merge-durability guard is blind to id-less scenarios (9 unmarked scenarios classify LEGACY-DELETE) #1979 — merge-durability guard blind to id-less scenarios. The fix is verified (keying by id else name, plus merge-set scoping so hand-authored scenarios are not false positives) and the adversarial case is caught. It then reports 9 pre-existing violations — 3 in BR-UC-007, 6 in inventory-profile; 9 and not 13, because the BRAND-SHORTHAND four sit in a hand-authored feature the merger never reaches. Filed rather than absorbed because the feature files are untouched by this PR, and BR-UC-007 is generated from adcp-req.
• BDD error accessors accept a reconstructed error on wire transports (17 scenarios grade nothing) #1980 — BDD error accessors accept a reconstruction on wire transports. Fix verified; it reddens 17 scenarios whose failure text is transport a2a is a wire transport but captured no error envelope. Each needs triage on a question the assertion cannot answer — weak scenario, or transport genuinely dropping the envelope — and the second is a production gap, not a test fix.
• Account resolution leaks existence across principals in a tenant (not-found raised before the access check) #1981 — account resolution raises not-found before the access check on a tenant-only lookup, giving an existence oracle across principals. Pre-existing; the file is untouched by this PR.
• UC-002-EXT-C obligations are stricter than the scenarios that grade them #1982 — UC-002-EXT-C obligations stricter than the scenarios that grade them. Blocked on tooling: the feature is generated from adcp-req, which was not available here, and docs/test-obligations/ is not the compiler input.
• Persisted flight status shares a fabricated read-path resolver call with the buyer-facing display path #1760 — owner assigned; the objection was to unowned deferral, and that is fixed.
• feat: Add idempotency_key support on update_media_buy and sync_creatives #1075 — already open and already assigned; it covers idempotency_key on update_media_buy and sync_creatives exactly. Nothing new filed.

Description corrections

All five accepted; applied to the description in the same push as this reply. The Layer Separation bullet no longer invents "(SHOULD-level)" and no longer describes a crash-location split the code does not implement — _dispatch_under_sanitize_seam catches only (AdCPError, ValueError, PermissionError), so an untyped skill crash goes JSON-RPC like any other, and the inner seam's docstring already said so. The "returns the failed Task" claim is qualified to the AdCPError arm. The ungraded verdict now cites security.yaml alongside error-compliance.yaml; the conclusion holds but the basis was one storyboard, and the missing www-authenticate header is filed. The AUTH_REQUIRED deprecation-vs-authentication.mdx conflict is recorded rather than left for the next reader to re-derive. error-handling.mdx:519 is now cited for the sanitizer, and the deliberate-deviation comment covers both axes. "Two MUST-be-generic cases" is corrected to three.

Verification

make quality 5994 · integration suite on the final tree · per-round order audits clean · no ratchet or allowlist moved, none grew · no new xfail or skip · every commit cites the finding it implements.

Not verified: one make quality run failed once (1 failed / 4340, fail-fast) and I could not reproduce it — 3 full runs and 11 seeded runs since are green at 5994, and the failing seed was lost. The most probable cause is that I started that run while a mutation-testing window was still settling, but I cannot prove it after the fact and am not claiming it as resolved.

[numarasSigmaSoftware]

Re-review addressed at e36919d18. The blocker was reverted rather than fixed forward, and the rest is filed — reasoning below.

Blocker — reverted

You are right, and the failure is worse than the threading being incomplete: _create_sync_workflow_steps creates one step per creative, so one outer id mapped to N steps, and get_by_external_task_id's unordered .first() picked an arbitrary one. Cancel cancelled one of three and reported CANCELED.

Both remedies you offered — resolve the id to the whole set, or a parent step owning children — are new mechanisms, on a PR already carrying six subsystems. So I took a third option: stop threading the id into sync_creatives at all (e36919d18). sync stays in _ASYNC_TASK_SKILLS, so push-notification injection is untouched; only the task-id threading is gone, along with the now-dead external_task_id plumbing through sync_wrappers → _sync → _workflow. With no durable id, the cancel path reaches the refusal branch and the buyer gets an honest refusal instead of a false CANCELED.

Accepted cost, stated so it is not rediscovered later: sync_creatives completion webhooks key on step_id rather than the buyer's task_* id. That is the pre-existing behavior, and it is already graded — test_creative_webhook_correlation.py parametrizes over the id being present and absent.

Mutation-verified against the pre-revert source: the new test fails DID NOT RAISE TaskNotFoundError there, i.e. it directly observed the false CANCELED being returned.

One correction to my own order, found during that proof. I specified the test assert TaskNotCancelableError from the no-durable-step branch. That outcome is unreachable from the real sync path and would have been revert-insensitive — it raises identically with or without the threading. The committed test grades both cancel legs, with the cross-process leg (fresh handler, i.e. after a restart or on another worker) carrying the mutation sensitivity, which is where the false CANCELED was actually reachable.

Worth recording because it is the substantive fact underneath: SyncCreativesResponse.status is hardcoded Literal["completed"], while on_message_send independently reports TASK_STATE_SUBMITTED when any creative is pending_review. The response model and the A2A task state disagree about the same operation. Your "sync still returns TASK_STATE_SUBMITTED" and the schema's "completed" are both true, at different layers. That divergence has no tracker and is arguably the real defect behind this finding.

The rest — filed as #2001, deliberately

Nine of your nine findings target code my remediation added. Last round that number was 1 of 20. My fixes are now producing the next round's findings faster than they retire them, so continuing to fix in-round is the thing most likely to generate a round three.

So #2001 carries all eight remaining items with your evidence intact: the complete_task widening (you are right that the authority for a buyer-facing terminalization is the buyer-cancel sibling, not the repository primitive), the discarded refusal in background_approval_service, the discarded reject_pending_execution CAS result, the three economics params _build_update_request still drops, and the four blind oracles — including the three you caught in tests I had reported as verified:

• the A2A forwarding test asserts at the spy one frame before the drop, so three of its five asserted values die downstream
• the webhook characterization test cannot fail in the direction it names — I measured deliver_ok as False in 0 of 24 combinations
• my own hotfix test does a live DNS lookup, which is precisely the failure its docstring says it exists to prevent

Those three are the ones I would most want re-checked when #2001 lands, because in each case I verified at the layer I changed rather than the layer where the effect has to appear.

Your three withdrawals

Noted and agreed on all three, and the two that shipped are already reverted. On the webhook direction specifically — the asymmetry and the false "vice versa" docstring were real, and the surviving verbatim copy at webhook_validator.py:335 is on #2001.

Trackers

#1803 and #1809 now owned; the eight sibling log_safe sites are recorded on #1809. #1810 closed as superseded by #1979.

#1807 is not closable — I checked before closing. dea3df825 removed two of its three homes (grep -rn '{"approved", "active"}' src/ is now 0), but the third and buyer-visible one survives at creatives.py:154: CreativeAction.failed if c.status != "approved". That is the one the issue is titled for. Commented with the evidence and assigned.

On your creative-status note: agreed, and the local gap is the live part — the pinned wire enum has six values, CreativeStatusEnum carries four, missing suspended and archived. Recorded on #1807.