feat: probe-first idempotency/revision/delivery concurrency subsystem (rebuild for #1546) - #1689
Draft
numarasSigmaSoftware wants to merge 160 commits into
Draft
Conversation
…tokens (prebid#1512) AdCP SDK clients inject the version-negotiation envelope fields (adcp_version / adcp_major_version) on every request, and some runners send "Bearer <token>" inside x-adcp-auth. Both made this agent uncallable by conformant clients: FastMCP's strict per-tool arg-validation rejected the undeclared envelope fields, and the raw bearer-wrapped credential failed the DB token lookup. Spec grounding (AdCP 3.1.0-beta.3, get_adcp_capabilities.mdx "Version Negotiation"): - Clients pin a major via adcp_major_version; a seller that cannot speak the pinned major MUST reject with VERSION_UNSUPPORTED (error-compliance storyboard, unsupported-major probe). - The advertised major_versions must reflect what the build actually speaks — now derived from the installed adcp SDK spec pin instead of a hardcoded 3 (src/core/adcp_version.py). - Envelope fields (context, ext, push_notification_config, idempotency_key, revision) are protocol framing a client may send on any request; tools that declare them still receive them, tools that don't must tolerate them (protocol-envelope, "always permitted" request fields). Changes: - src/core/adcp_version.py: single source of truth for the AdCP major this build speaks (parsed from adcp.get_adcp_spec_version()) and validate_adcp_major_version() negotiation guard. - MCP RequestCompatMiddleware: reject unsupported majors with a VERSION_UNSUPPORTED wire envelope, then strip negotiation fields and undeclared envelope fields before dispatch (all environments). - A2A parity: _handle_explicit_skill runs the same major validation before dispatch, rendered as a VERSION_UNSUPPORTED failed-Task envelope. - AdCPVersionUnsupportedError (400, terminal) + SPEC_CODES moved into src/core/exceptions.py as the single source of truth for spec-required codes not yet in the SDK's STANDARD_ERROR_CODES. - normalize_adcp_auth_token(): trims and strips a case-insensitive "Bearer " prefix from x-adcp-auth across all three auth paths (get_principal_from_context, resolve_identity, UnifiedAuthMiddleware). - tool_error_logging: _translate_to_tool_error made public (translate_to_tool_error) for the middleware boundary; guard tests updated. Closes part of prebid#1512; addresses prebid#1247 item 1. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
) The A2A dispatch validated the AdCP major but left the version-negotiation fields (adcp_version / adcp_major_version) and undeclared envelope framing in `parameters`. Handlers that `model_validate` the full dict against a strict (extra="forbid" in dev/CI) request model — get_media_buys and update_performance_index, whose models declare neither field — then rejected conformant SDK clients with extra_forbidden, breaking the cross-transport parity the PR claims. The MCP middleware already strips these before dispatch. - _handle_explicit_skill: validate the major, then strip negotiation fields FIRST — before the idempotency payload is captured — so the canonical request hash stays identical to the MCP path (which strips before the tool builds its payload), and before any handler's strict model_validate. - _validate_envelope_tolerant: shared helper that strips undeclared AdCP envelope fields against each model before model_validate, mirroring the MCP middleware's schema-aware envelope strip (Step 4). Applied at all 4 strict-validation sites. - Fix stale _translate_to_tool_error references (renamed to the public translate_to_tool_error) in comments/docstrings. Tests: unit A2A-dispatch regression (get_media_buys with supported major + ext no longer rejects) and an end-to-end on_message_send regression that produces get_media_buys_result with the fix and error_result without it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rebid#1512) The FUTURE_TOP_LEVEL_FIELD fixture used adcp_major_version: 5 as a stand-in for "an unknown future top-level field the server should accept/strip in production." This PR gave adcp_major_version dedicated negotiation semantics: the middleware validates it and rejects an unsupported major with VERSION_UNSUPPORTED in ALL environments (spec-mandated), so it is no longer a silently-accepted forward-compat field. test_production_accepts_payload [future_top_level] therefore failed on the CI Unit Tests job (which runs tests/harness/, unlike `make quality`'s tests/unit/-only scope). Replace the fixture value with a genuinely-unknown field (experimental_capability) so the test again exercises production stripping / dev rejection of unknown top-level fields. Version negotiation is covered by test_mcp_compat_middleware.py and test_adcp_version.py. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…sing (prebid#1546 review) Address KonstantinMirin's review on prebid#1546 — wire the derived BR-UC-010 VERSION_UNSUPPORTED scenarios and close the implementation gaps they grade. Version negotiation (spec: core/version-envelope.json + error-details/version-unsupported.json at v3.1.0-beta.3): - validate_adcp_version_pins() (was validate_adcp_major_version) now negotiates the string adcp_version pin as well as the deprecated int, rejecting majors above the native major. Pre-3.0 pins stay honored — the version_compat layer deliberately serves them (see module docstring). - VERSION_UNSUPPORTED details now carry the REQUIRED supported_versions[] (SDK-pin derived), deprecated supported_majors[], advisory build_version, the echoed buyer pin, and a suggestion. - REST parity: a router-level dependency on /api/v1 validates the RAW body/query pin before Pydantic parsing, so *Body model defaults never trigger a rejection; GET /capabilities accepts query-param pins. - BR-UC-010 wiring: test_uc010_discover_seller_capabilities.py binds the feature; the four version-negotiation scenarios run green across MCP/A2A/REST via CapabilitiesEnv (new harness env); Then-steps assert on the real wire envelope. Remaining UC-010 scenarios xfail at the fixture (UC-002/UC-018 convention). Bearer consolidation (DRY + divergence fix): - One primitive: http_utils.parse_bearer_authorization() + extract_auth_token(); auth.py, resolved_identity.py, and UnifiedAuthMiddleware all route through it — the four hand-rolled parses and the duplicated extraction routine are gone. - Fixes the whitespace divergence (" Bearer <token>" now extracts identically everywhere), pinned by TestPaddedAuthorizationConsistency. - Wire-level: test_bearer_across_transports.py drives the REAL REST ingress (no dep overrides) with canonical/padded/lowercase-scheme bearer forms. - normalize_adcp_auth_token docstring corrected ("Bearer " -> "Bearer") and the inaccurate "left for schema validation" comment removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nsport idempotency (prebid#1546 review round 2) Verified-review follow-ups on the version-negotiation slice: - A2A captures raw_wire_payload BEFORE stripping negotiation fields, so the idempotency canonical hash matches MCP (captured pre-strip in MCPAuthMiddleware) and REST (raw body bytes); the SDK canonicalizer's exclusion list is closed, so adcp_version participates in the hash. - VERSION_UNSUPPORTED recovery is "correctable" per enums/error-code.json enumMetadata ("re-pin to a release in supported_versions and retry"), not "terminal"; BDD + unit assertions updated to pin the spec value. - The request's context object rides on AdCPVersionUnsupportedError so the error envelope echoes it (error-compliance storyboard grades field_present: context and unchanged correlation_id on error responses). - Version rejections now hit record_boundary_error on MCP (middleware, best-effort identity from auth state) and A2A (raise site) — previously only REST recorded them; the A2A dispatcher comment claiming otherwise was stale. - Major pins validate by MEMBERSHIP in the supported set: below-native majors reject like above-native ones (get_adcp_capabilities.mdx: "validates against its major_versions and returns VERSION_UNSUPPORTED if not in range"); this build serves 3.x shapes only, so accepting a 2-pin while advertising [3] was self-inconsistent. - Capabilities advertise adcp.supported_versions + build_version (advisory at 3.1, required at 3.2), reusing the negotiation helpers. - The echoed pin in error details is capped at 64 chars (buyer-controlled, reflected). - Dead post-strip adcp_version read removed from the A2A get_products handler (apply_version_compat is a no-op for dict payloads). - Envelope-tolerance comments cite the real grounding (additionalProperties: true + security.mdx § Idempotency) instead of the nonexistent spec term "always permitted request fields"; stale negotiation-follow-up comment updated. - Weak-mock allowlist shrunk: test_rest_applies_version_compat now uses assert_called_once_with. make quality green (5195 unit tests); BDD UC-010 18 passed / 548 xfailed; touched integration suites (a2a skill invocation, bearer across transports, mcp error envelope) 30 passed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…, BDD grading)
Addresses five of the round-3 review items:
1. DRY — no new clones from the consolidation:
- Extract capabilities._build_adcp_block(); both response paths call it so
the version/idempotency envelope is declared once.
- Add tool_error_logging.record_boundary_error_for_identity(); route the
MCP middleware + the three A2A identity-based boundary sites through it
(removes the hand-rolled getattr(identity,…) or "anonymous" projection).
3. A2A logs dropped negotiation/envelope fields at DEBUG, parity with the MCP
RequestCompatMiddleware — the strip is now observable on every transport.
5. BDD grades the behavior this PR changed, against production:
- New @T-UC-010-v31-version-unsupported-cross-major outline pins the
below-native ("2.0") rejection alongside the above-native ("4.0") control,
dispatched per-transport (6 green).
- details-bounds rows now assert on production's real wire envelope
(supported_versions present + non-empty) instead of validating a mutated
copy against the SDK model.
6. Malformed SDK spec pin surfaces as a typed AdCPConfigurationError (500) via
one _spec_major_minor() helper both siblings share, never a bare ValueError.
7. Debug-log the silently-tolerated unparseable version pin; document why the
pre-3.0 needs_v2_compat comparison is still load-bearing (unpinned "1.0.0"
default) rather than deleting a directly-unit-tested branch.
Gates: make quality green (5197 passed); UC-010 negotiation BDD 24 green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ransport (prebid#1546) Addresses the last two round-3 review items. Item 2 — cross-transport error precedence. Canonical decision: AUTH before VERSION (an unauthenticated caller is not told what versions the agent supports — VERSION_UNSUPPORTED discloses supported_versions). Made uniform: - REST: version validation moves from a blanket router dependency (which ran before auth) to per-route _version_after_resolve / _version_after_require deps that chain the route's own auth dep as a sub-dependency, so require_auth rejects first on auth-required routes. - MCP: MCPAuthMiddleware now rejects a principal-less identity (missing token), not only an invalid one, so the request never reaches the version gate on a missing token — the missing-vs-invalid flip is gone. - A2A: already AUTH-first (on_message_send enforces auth before dispatch); covered by the new compound-error test. Pinned by tests/integration/test_auth_version_precedence.py (bad/missing token + unsupported pin → AUTH on REST, A2A, and the MCP middleware chain). Item 4 — bearer normalization graded on the wire for all three transports. test_bearer_across_transports.py now grades canonical/padded/lowercase-scheme forms end-to-end past the real extract_auth_token seam: - A2A: TestClient POST /a2a runs the SAME UnifiedAuthMiddleware as REST plus the A2A context-builder + real _get_auth_token (no mock). - MCP: resolve_identity_from_context with a raw Authorization: Bearer header (via get_http_headers) → extract_auth_token → DB lookup. Gates: make quality green (5198 passed); integration auth/bearer/precedence + UC-010 BDD green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…prebid#1546 review) P2 — REST get_products v2 compatibility was a no-op. /api/v1/products dumped the response to a dict before calling apply_version_compat, which short-circuits on a dict (legacy pass-through) and never derives the v2 pricing fields. Pass the response MODEL instead, so an unpinned legacy client (Body "1.0.0" default) actually gets is_fixed / rate / price_guidance.floor. The test now asserts the real REST JSON (compat fields present when unpinned, absent for a v3 pin), not merely that the helper was called. Shared product-response builder extracted to tests/helpers/adcp_factories.make_get_products_response_with_pricing (DRY; baseline unchanged at tests 89). Should-fix — malformed version pins now reject instead of being silently stripped. version-envelope.json constrains adcp_version to ^\d+\.\d+(-[a-zA-Z0-9.-]+)?$ and types adcp_major_version as an integer, and the spec prose defines a fallback only for an OMITTED pin. So a present-but-garbage value ("banana", non-int major) is a malformed request: validate_adcp_version_pins now raises AdCPValidationError (VALIDATION_ERROR, correctable) with a truncated echo, replacing the round-3 debug-log-and-tolerate. This strengthens the version contract rather than erasing a claim the client made. Supersedes the earlier tolerate behavior; the echo-truncation helper is shared with the VERSION_UNSUPPORTED builder. Gates: make quality green (5200 passed); UC-010 BDD + integration bearer/ precedence/products-auth green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…otiation # Conflicts: # tests/bdd/conftest.py
…eware (prebid#1546) CI regression surfaced after merging main: auth-required MCP tools returned an auth error with no error_code on the wire (UC-011 sync_accounts BDD + test_mcp_error_envelope). A raw AdCPError raised from MCPAuthMiddleware bypasses the tool wrapper's with_error_logging, so it never became a two-layer envelope — the same reason RequestCompatMiddleware translates its own version error. Wrap the identity resolution + missing-token guard in the middleware and route an AdCPError through record_boundary_error_for_identity + translate_to_tool_error, so both invalid and missing tokens surface AUTH_TOKEN_INVALID with code + recovery + suggestion, identically to the version-negotiation path. Unit tests updated to assert the translated AdCPToolError envelope (code AUTH_TOKEN_INVALID) rather than the raw exception. Gates: make quality green (5251 passed); UC-011 MCP auth + test_mcp_error_envelope + auth/version precedence green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…1512) Self-caught follow-ups from a self-review of the round-4 changes: - Pin AUTH-before-VERSION as a wired BDD scenario across MCP/A2A/REST (BR-UC-011 account-validation): a caller with a bad token AND an unsupported version pin gets a terminal AUTH_TOKEN_INVALID envelope and is never told the seller's supported_versions. Asserts on the real wire envelope; the integration tests remain as fast controls. - Unify the four "Dropped … fields" debug logs behind one _log_dropped_fields() helper in request_compat.py, so MCP and A2A emit the same message shape keyed by the tool/skill name (no more for/from and model-class-name divergence). - Reference the shared _BEARER_FORMS/_BEARER_IDS constants in the REST bearer test instead of re-inlining the literals. - Reword the SPEC_CODES comment to separate the pinned-spec codes (VERSION_UNSUPPORTED, BILLING_NOT_SUPPORTED) from the project-specific AUTH_TOKEN_INVALID (grounded in BR-UC-011, not the AdCP 3.1 error enum). - Extract _reject_at_mcp_boundary() to remove the duplicated MCP rejection tail, and hoist _DEFAULT_PROTOCOLS/_DEFAULT_SPECIALISMS in capabilities.py to finish the response-construction dedupe. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
# Conflicts: # run_all_tests.sh
…re helper (prebid#1512) Self-caught follow-up on the round-4 A2A boundary-error work: - The new `except AdCPAuthenticationError` handler in on_message_send hand-rolled record_boundary_error(..., tenant_id="unknown", principal_id="unknown"). Because record_boundary_error early-returns only on a *falsy* tenant_id, the truthy "unknown" drove activity-feed + audit writes under a fabricated tenant for an unauthenticated caller — the exact opposite of the intended "no tenant → skip the sinks" behavior. Route through record_boundary_error_for_identity("a2a", ..., identity) like the three sibling dispatch sites: at an auth failure identity is None, so tenant_id degrades to None and the sinks are correctly skipped. Regression test updated to pin this (mutation-verified: the old call attempts a real audit write). - health.py: drop "immutable" from TestingAdCPVersionPolicy's docstring — the SalesAgentBaseModel it extends is not frozen, so instances are mutable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ity-aware sink (prebid#1512) Self-caught while re-reviewing the boundary-error consolidation: the generic `except Exception` handler in `on_message_send` was left with the old hand-rolled projection — `err_tenant_id = (identity.tenant_id or "unknown") if identity else "unknown"` then `record_boundary_error(..., tenant_id="unknown")` — even though the byte-adjacent `AdCPAuthenticationError` branch directly above was converted to `record_boundary_error_for_identity`. `record_boundary_error` only early-returns on a *falsy* tenant_id, so a truthy "unknown" drives the activity-feed + audit writes under a fabricated tenant. This handler is reachable with `identity` still None (e.g. a non-auth exception raised during identity resolution, before it is assigned), so a caller with no resolved tenant would wrongly hit those tenant-scoped sinks — the exact bug the auth branch was fixed to avoid, one clause away. Route it through `record_boundary_error_for_identity` too, so tenant_id degrades to None and the sinks are correctly skipped. Add a regression test that drives the generic handler with an unresolved (None) identity and pins the identity-aware call; mutation-verified (reverting the fix fails the test). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ng prerelease (prebid#1512) The SDK spec pin 3.1.0-beta.3 was normalized to a bare "3.1" for the wire, dropping the required -beta.3 prerelease. core/version-envelope.json (v3.1.0-beta.3) is explicit: full-semver meta values MUST normalize to release-precision (patch dropped, prerelease PRESERVED) and "meta-field values are NOT valid wire values" — so the correct advertised value is "3.1-beta.3", not "3.1". Advertising the bare stable "3.1" both claimed a release this build does not implement and made the seller reject a correct "3.1-beta.3" buyer pin with VERSION_UNSUPPORTED. Fix: capture the prerelease segment from the SDK build version and route every advertised form through a single shared conversion (_release_precision_wire_value) behind supported_adcp_versions(), from which advertisement, request validation, capabilities, and the VERSION_UNSUPPORTED error details all derive. Only the never-negotiated PATCH digit is dropped. Tests: add an exact 3.1.0-beta.3 -> 3.1-beta.3 normalization test and make the derived-value test assert prerelease preservation. Downshift tests that relied on a stable "3.1" being advertised now pin a stable set explicitly (a stable pin never downshifts onto a prerelease — already locked by test_stable_pin_never_downshifts_onto_prerelease); the MCP/A2A negotiation-strip tests use the real supported value instead of a hardcoded "3.1". Spec-grounding: core/version-envelope.json @ v3.1.0-beta.3 (authoritative, cross-checked against the installed adcp==5.7.0 SDK pin). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…y SSRF (prebid#1512) An auth-optional A2A discovery request (e.g. get_adcp_capabilities) resolves an anonymous identity SUCCESSFULLY, and on_message_send then persisted any supplied push_notification_config before the skill ran. The end-of-processing status webhook (and the failure-webhook path) would later POST to that stored URL with no SSRF validation — so an unauthenticated caller could drive an outbound request to an internal/cloud-metadata endpoint (e.g. http://169.254.169.254/latest/meta-data). The prior regression only covered a rejected-auth request dropping its callback, not this successful-anonymous sibling. Fix (both controls, per review): - Gate persistence with _validate_push_callback(): an anonymous caller cannot register a callback at all, and any callback URL must pass the shared WebhookURLValidator SSRF check (loopback, link-local/metadata, RFC-1918) before it is stored — the same guard the media-buy delivery path uses. A rejection leaves the config unstored, so the later webhook has no target. - Re-validate the URL at delivery time in _send_protocol_webhook as defense-in-depth against DNS-rebinding / TOCTOU and any callback that reached storage through another path. Tests: end-to-end anonymous-discovery-with-callback is refused and never persisted; authenticated caller still cannot register an SSRF URL; a safe URL is accepted; delivery skips an SSRF target. Mutation-verified (removing either control fails the corresponding test). Incidental: aligns one a2a integration negotiation test to the release-precision supported value (3.1-beta.3) rather than a hardcoded "3.1", a ripple from the version-normalization fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ebid#1512) ERROR_CODE_MAPPING rewrote BILLING_NOT_SUPPORTED -> UNSUPPORTED_FEATURE, which contradicted the adjacent SPEC_CODES comment ("all three pass through translate_error_code() unchanged"). BILLING_NOT_SUPPORTED is a distinct pinned-spec code (a member of the adcp SDK ErrorCode enum, buyer-visible via SPEC_CODES, mandated by BR-RULE-059) and the pinned sync-accounts-response fixture expects the buyer to see it verbatim. Collapsing it to the generic UNSUPPORTED_FEATURE erases the billing-specific meaning. The mapping was also a latent trap: BILLING_NOT_SUPPORTED is currently only emitted as a SyncAccountsResponse.errors[] advisory (which bypasses translate_error_code), so the mapping had no live effect — but any future raise of AdCPError(code="BILLING_NOT_SUPPORTED") would have been silently rewritten. Remove the obsolete mapping so it passes through unchanged, and add an invariant test that every SPEC_CODES member survives translate_error_code verbatim. Mutation-verified (re-adding the mapping fails the new test). Spec-grounding: adcp==5.7.0 ErrorCode enum defines BILLING_NOT_SUPPORTED and UNSUPPORTED_FEATURE as distinct members; version-envelope pin 3.1.0-beta.3. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…y default (prebid#1512) REST version negotiation validated the buyer's query/body pins but then discarded the result: get_products serialized the response through apply_version_compat() using only GetProductsBody.adcp_version, whose default is "1.0.0". A v3 client pinning by query (?adcp_version=3.1-beta.3), by body adcp_major_version, or by query adcp_major_version never populated that body field, so it was silently served the legacy v2 shape (is_fixed / rate). Only a body adcp_version string pin worked. Fix: _validate_version_pins now derives the effective negotiated release from the merged pins (query + body, adcp_version or the deprecated integer major, the latter projected to MAJOR.0) and stashes it on request.state; get_products gates compat on that, falling back to the body default only when the buyer sent no pin at all (preserving the unpinned-legacy v2 default). Reuses the existing REST conflict guard, so a query pin that disagrees with a body pin is rejected with VALIDATION_ERROR before any response is built. Tests: real TestClient coverage for query release pin, body major pin, query major pin (all must serve clean v3), conflicting query/body pins (rejected), and matching query+body pins (accepted, clean). Mutation-verified (reverting to body.adcp_version fails the query/major-pin tests). Scope: this fixes the response-compatibility gating (the demonstrated v2-leak). Universal response adcp_version echo is graded advisory/ungraded by the pinned 3.1 migration table and remains a separate deferred residual per src/core/adcp_version.py. Spec-grounding: core/version-envelope.json @ v3.1.0-beta.3 ("On a response: the release the seller actually served"). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… reach capabilities (prebid#1512) capabilities imported supported_adcp_versions / adcp_major_version / adcp_build_version by name, binding a private copy at module load. A version policy override applied at src.core.adcp_version.* (the in-process BDD harness patch, or a testing policy) never reached that copy, so validate_adcp_version_pins negotiated one supported set while _build_adcp_block advertised the SDK default — a probe configuring ["3.0","3.1"] saw both in negotiation but ["3.1-beta.3"] in capabilities. Resolve all three through the module attribute (src.core.adcp_version.*) so a single canonical patch point governs both advertisement and negotiation. Add a harness test that drives the real _build_adcp_block under a policy override and asserts the ADVERTISED versions (not merely absence of an error); the prior test only proved the patch took on the module. Mutation-verified (the by-name import advertises the stale default and fails the new test). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…equest.state (prebid#1512) The Blocker-4 fix initially read the negotiated version off request.state, which required adding a raw Request parameter to the get_products route handler — a violation caught by the test_rest_depends_auth structural guard (test_no_route_has_request_parameter): route handlers must resolve everything via FastAPI Depends, never a raw Request. Rework: _validate_version_pins / _version_after_resolve now RETURN the effective negotiated release, and get_products injects it as a `negotiated_version: str | None = Depends(_version_after_resolve)` parameter. Dependencies may still take Request (only route handlers may not). Same validation, same VERSION_UNSUPPORTED gate, same auth-before-version ordering — no request.state, no Request in the handler signature. All REST product version tests and the guard pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…1512) A capabilities request carrying `context` returned `context=None` — the response model declares the field but neither the impl nor the wrappers set it, so the buyer's context silently vanished. The version-negotiation storyboard grades an unchanged context echo, and the error path (_version_unsupported_error) already echoes it, so the success path diverged. Fix (part of Blocker 3 "envelope tolerance", context portion): - _get_adcp_capabilities_impl echoes `req.context` on BOTH response paths (minimal no-tenant and full-tenant). - The MCP wrapper and A2A raw function now accept and forward `context` into the request. Declaring `context` on the MCP wrapper also keeps the envelope-tolerance middleware from stripping it (it strips only ADCP_ENVELOPE_FIELDS the tool does not declare); the A2A path already preserves context (it strips only negotiation fields), so the A2A skill handler forwards parameters["context"]. Tests: impl echoes context on the minimal path and the full-tenant path. Mutation-verified (dropping context from either response construction fails the corresponding test). Deferred (tracked separately): revision->CONFLICT optimistic concurrency (needs a MediaBuy.revision column migration) and idempotency replay dedup, per the review triage. Spec-grounding: core/version-envelope.json @ v3.1.0-beta.3 (context echoed on responses); adcp==5.7.0 GetAdcpCapabilitiesResponse.context field. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…A2A (prebid#1512) The buyer's idempotency_key was discarded and replaced with a fresh server UUID on every sync_accounts call: the MCP wrapper did not declare idempotency_key (so the envelope-tolerance middleware stripped it) and unconditionally generated str(uuid.uuid4()); the A2A skill handler built the request without the key at all. Two retries carrying the same client key therefore became two distinct requests — the exact behavior AdCP's idempotency contract exists to prevent. Fix (part of Blocker 3 "envelope tolerance", idempotency portion): - The MCP sync_accounts wrapper declares idempotency_key (so it is not stripped) and forwards it verbatim, synthesizing a UUID only when the client omits one. - The A2A _handle_sync_accounts_skill forwards parameters["idempotency_key"] (the A2A path strips only negotiation fields, so it survives), same omit-only fallback. Tests: MCP wrapper and A2A handler forward the client key into the SyncAccountsRequest; a fresh UUID is synthesized only when omitted. Mutation-verified (reverting either path to always-generate fails the forward tests). Scope: this restores the client key end-to-end (the deleted semantic the review flagged). Full idempotency replay-dedup for sync_accounts is not yet wired (_sync_accounts_impl does not consult the key) and is deferred with the revision->CONFLICT concurrency work per the review triage. Spec-grounding: adcp==5.7.0 UpdateMediaBuyRequest/SyncAccountsRequest.idempotency_key ("resending with the same idempotency_key guarantees the update is applied at most once"); version-envelope pin 3.1.0-beta.3. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nvelope (prebid#1512) Second-round review follow-ups on the round-1 callback guard. B1 — SSRF still bypassable: the guard validated the URL string once, but the outbound POST followed redirects (requests default), so a validated public URL could 302 the POST to cloud-metadata/private, and plain HTTP was allowed in production. Fixes: - protocol_webhook_service outbound POST now sends allow_redirects=False, so a post-validation redirect is never followed (closes the demonstrated vector for all callers). - New WebhookURLValidator.validate_callback_url is the single env-gated gate used at both registration and delivery: production requires HTTPS and blocks loopback/localhost; non-production relaxes both (un-breaks the E2E localhost webhook receiver — the round-1 regression). SSRF range checks (link-local/ metadata 169.254.169.254, RFC-1918) apply in every environment. - Residual (noted in code): connect-time peer/IP pinning against DNS rebinding needs a custom transport; out of scope here. B2 — callback validation surfaced as INTERNAL_ERROR and broke E2E: the guard raised AdCPValidationError at persistence, BEFORE the per-skill translation seam, so on_message_send emitted a JSON-RPC -32603 whose data the A2A v0.3 adapter drops (data: null); its localhost block also failed 5 E2E tests. Fix: catch the AdCPValidationError at the callback-validation call site and return a FAILED Task carrying the two-layer envelope DataPart (buyer-correctable VALIDATION_ERROR) — the same shape as a failed skill — scoped so the outer handler (and an existing NL test that relies on InternalError) is unchanged. The callback stays unstored, so no webhook can be driven. Tests: env-gated validate_callback_url (HTTPS-in-prod, localhost-in-test, metadata-always-blocked); outbound POST asserts allow_redirects=False; the anonymous-callback unit test now asserts a FAILED task (not InternalError); and a raw A2A-wire integration test asserts the VALIDATION_ERROR envelope on the artifact. All mutation-verified. Spec-grounding: pinned 3.1.0-beta.3 security rules (production HTTPS, redirects disabled, SSRF range validation) + the A2A two-layer error-envelope contract. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… loud (prebid#1512) update-media-buy-request.json (v3.1.0-beta.3) requires a supplied revision mismatch to return CONFLICT. No tool implements that (there is no MediaBuy.revision column; the compare-and-increment feature is tracked in prebid#1607), yet revision sat in ADCP_ENVELOPE_FIELDS and was silently stripped — dropping the buyer's optimistic- concurrency guard and performing a STALE update the buyer believed was protected. Remove revision from the tolerated envelope set so it is retained and rejected by strict per-tool validation instead of silently succeeding — the reviewer's explicit interim. Caveat (documented in code + prebid#1607): production uses extra="ignore", so a complete fail-loud/CONFLICT across all environments is part of the prebid#1607 feature. Test: request_compat no longer strips revision (mutation-verified — re-adding it to ADCP_ENVELOPE_FIELDS fails the test). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rebid#1512) The impl/MCP/A2A capabilities and products paths already echo the request context, but the REST boundary dropped it: GetProductsBody had no context field and the handler never forwarded it to create_get_products_request. Per the pinned version-negotiation storyboard (POST-S9), the application context MUST be echoed unchanged on the response. Add context to GetProductsBody and forward it; _get_products_impl already echoes req.context, so REST was the only gap. GET /capabilities carries no body and is graded "when possible" (POST-F3), so it is structurally N/A — left as-is with a note. Test: real TestClient POST /api/v1/products with context echoes it verbatim on the wire (mutation-verified — dropping the forward fails it). Spec-grounding: core/version-envelope.json @ v3.1.0-beta.3 (context echoed on responses). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…oks deliver (prebid#1512) The localhost-only seam from the previous commit was insufficient: the E2E runner exposes its webhook receiver via the compose alias ADCP_WEBHOOK_HOST=tests, which resolves to a private RFC-1918 (172.x) address. The SSRF guard blocks RFC-1918, so it rejected the legitimate callback and 5 E2E webhook tests failed with "Expected at least one webhook delivery" (the same 5 the reviewer saw at exact-head — the round-1 guard regressed them). Add an `allow_private` seam to check_url_ssrf that separates: - ALWAYS blocked (every env): link-local / cloud-metadata (169.254.x, fe80::, metadata.google.internal, instance-data) — the credential-exfiltration surface. - Blocked only without allow_private: RFC-1918 private, loopback, and localhost/Docker aliases — legitimate targets for a trusted test/dev receiver. validate_callback_url passes allow_private=True in non-production, so the compose/ local receiver is reachable while metadata stays blocked in every environment. Production and the other check_url_ssrf callers (property-list, signals) keep the default allow_private=False, so their behavior is unchanged. Tests: test mode allows a private compose host and localhost; metadata IP and the GCP metadata hostname stay blocked even in test mode. Mutation-verified. E2E now 91 passed / 0 failed (was 86/5) — the previously-red webhook-delivery cases pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…and connection pinning (prebid#1512) Replace the over-broad "any non-production allows private targets" gate with a dedicated ADCP_ALLOW_PRIVATE_WEBHOOKS opt-in (set only by the E2E harness), so a real staging/dev deployment serving buyers still blocks private/internal callbacks. - url_validator: add resolve_and_validate_target() — resolve the hostname once, validate EVERY A/AAAA record (closes the single-record / IPv6 / DNS-rebinding multi-record bypass), and return a single validated IP to pin. - protocol_webhook_service: pin delivery to the validated IP via a _PinningHTTPAdapter mounted on the long-lived pooled session — keeps connection pooling and session.post mockability while binding SNI + cert verification to the original hostname (closes the validate-then-reconnect gap). Redirects disabled; only 2xx counts as delivered (a 3xx refused-redirect is a failed delivery, not success). - webhook_validator: HTTPS required whenever private targets are not permitted. Verified: make quality (5330), ssrf/webhook unit (96), e2e webhook delivery 10/10 (loopback + container), bdd uc004 (408). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rebid#1512) Self-caught follow-ups to the idempotency_key work, which had only landed on the MCP and A2A wrappers: - REST /accounts/sync silently dropped a buyer-supplied idempotency_key: SyncAccountsBody had no such field, so pydantic's extra="ignore" discarded it and req.idempotency_key was always None on REST. Declare the field and forward it, synthesizing a UUID only when the client omits one — matching the MCP/A2A siblings so a retry carrying the same key is not fabricated a fresh UUID. Adds REST coverage to test_sync_accounts_idempotency (mutation-verified). - Drop a redundant function-local `import uuid` in _handle_sync_accounts_skill; the module already imports uuid at top level. - Document why REST GET /capabilities forwards no context (no request body, so the echo would be an inherent no-op) rather than leaving it an unexplained asymmetry against the MCP/A2A capabilities siblings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… into version-negotiation branch
Resolves 18 conflicts composing upstream's UC-002/003 validation wiring +
canonical error-code reconciliation with this branch's version-negotiation,
envelope-tolerance, SSRF, and idempotency work:
- exceptions.py: adopt canonical AUTH_REQUIRED (correctable + suggestion
mechanism); keep the SPEC_CODES passthrough mechanism reduced to
{BILLING_NOT_SUPPORTED, VERSION_UNSUPPORTED}; preserve
AdCPVersionUnsupportedError and the BILLING un-mapping.
- A2A/REST boundaries: nest envelope-tolerant validation inside the
adcp_validation_boundary wrappers; keep upstream's pre-auth
InvalidRequestError (two-layer envelope) rejection; preserve
idempotency_key, context forwarding, SSRF validation, and version pins.
- api_v1.py: SalesAgentBaseModel bodies via a shared _VersionedBody base that
declares both negotiation pins (strict dev bodies must not 400 the
deprecated integer pin the raw-body negotiator reads).
- Harness: upstream's helper structure + WireAuth/GET/restore-in-finally;
bdd conftest taken from upstream with the UC-010 wiring replayed.
- Error tests updated to the canonical codes; .duplication-baseline
regenerated.
Verified: unit+harness 5629 passed; mypy 282 files clean; integration error/
account slices 45/45; e2e webhook 10/10; bdd uc004 459 passed.
Note: the local type-ignore-baseline guard flags 67 vs our fork's stale
origin/main (42); 67 IS upstream's committed baseline, so the guard passes
against prebid/salesagent main.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…A's missing-token rejection Two constants that disagreed, and one boundary that recorded nothing. **The deadline cancelled the retries it was supposed to bound.** `WEBHOOK_DELIVERY_DEADLINE_SECONDS` was a hand-picked 12.0 wrapping a loop that legitimately runs 36-38s: 3 attempts x 10s POST timeout, plus 2-3s and 4-5s exponential backoff. The SECOND attempt alone exceeded it. So the TimeoutError branch was not an outlier — it was the normal outcome for any slow endpoint, and it killed the retries that exist to tolerate exactly that. All four callers discard the returned bool, so the only trace was one log line. Nothing pinned the relation; the sole existing reference monkeypatches the constant to 0.05. The deadline is now DERIVED from the retry policy, and the policy constants live beside it, because they are one decision. Deliberate trade-off: a slow target occupies a bulkhead worker for up to ~38s instead of 12s. That is the cost of the retry policy actually running — the old value bought worker turnover by making retries a no-op. The comment says to tighten the RETRY POLICY if that occupancy is too high, and not to re-cap the deadline below the budget, which would just restore the silent cancellation. **A2A's missing-token rejection left no trace.** It raises `InvalidRequestError`, an `A2AError` subclass, and `except A2AError: raise` re-raises it without reaching `record_boundary_error_for_identity`. Measured over the real transports: A2A 0, REST 1, MCP 1. The gap is inside A2A too — the sibling INVALID-token path a few lines down does record. `record_boundary_error`'s own docstring states all three boundaries delegate to it so severity, activity feed and audit stay in lockstep: a claimed invariant with no oracle. That change collided with a DELIBERATE assertion in `test_auth_failure_never_sends_or_retains_attacker_push_callback`, which required that nothing be recorded, on the grounds that an unauthenticated sender must not drive tenant-scoped sinks. Both hold at once: `record_boundary_error` returns early when `tenant_id` is None, so recording with identity=None emits the operator-facing WARNING while the activity feed and audit log stay silent. So that test is STRENGTHENED, not relaxed. It now patches the SINKS and lets the telemetry helper run for real, asserting `activity_feed.log_error` and `get_audit_logger` are never called. The old assertion graded "we avoided calling the helper"; this grades the property that actually matters — no tenant-scoped sink fires for an attacker. Removing the tenant guard in `record_boundary_error` reddens it (the rejection logs to a fabricated "unknown" tenant); the old assertion would not have noticed. The deadline oracle is behavioral, not a source scan: the real POST call must carry WEBHOOK_POST_TIMEOUT_SECONDS. My first version used `inspect.getsource`, which this repo bans by lint (TID251) for exactly the reason it was wrong. Mutation-verified: restoring the 12.0 deadline reddens the relation oracle (assert 12.0 >= 38.0); drifting the service POST timeout to 25.0 reddens the payload test; removing the A2A recording reddens with "recorded 0 boundary errors; REST and MCP each record 1"; removing the tenant guard reddens the attacker test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…on version Two self-inflicted regressions the full in-network suite caught, both from the previous commit's version work. **The agent card conflated two different versions.** One local `protocol_version` fed BOTH the extension URI and the advertised `adcp_version`, so pointing it at `wire_adcp_version()` moved the URI too and broke the live agent-card e2e tests (`assert '3.1' == '3.1.1'`). They are not the same value: - the URI is a schema PATH (`dist/schemas/3.1.1/...`), which exists at patch precision, and - `params.adcp_version` is a NEGOTIATION value a buyer pins on its next request, where the wire envelope is release precision. Advertising the patch version told buyers to pin "3.1.1", which this agent's own `_RELEASE_PIN_RE` then rejects with VERSION_UNSUPPORTED — the same defect as the delivery-webhook payload, in the one place a buyer is most likely to read it from. Both e2e tests now assert the URI carries the schema version AND that the advertised version is release-precision and one we accept inbound, instead of pinning both to the SDK spec pin. **Setting ADCP_ALLOW_PRIVATE_WEBHOOKS on the compose test runner was wrong.** It was added to preserve in-process localhost webhook behaviour after the two SSRF gates converged on that flag. But the in-process suites assert the PRODUCTION default — the callback-policy tests pin `_validate_callback_url_with_policy(..., allow_private=False)` — so the flag flipped 16 of them. Verified both directions: 16 failed with the variable set, 95 passed with it unset. Only the SERVER needs the opt-in, because only the server dials the compose-network receiver, and it already had it. `make quality` did not catch either: it runs unit-only, and the unit env has no ADCP_ALLOW_PRIVATE_WEBHOOKS, so the 16 failures existed solely inside the in-network runner. The e2e arm it never runs at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ded seams **Idempotency was described backwards at seven sites.** The wire ships `IdempotencyUnsupported(supported=False)`; six parameter docstrings and the Spec-Grounding-Gate note said this seller "advertises idempotency support". Zero hits at merge-base, so the drift entered here — and `tests/harness/media_buy_create.py` states it correctly, so the PR contradicted itself. The note mattered most: it is the artifact a reviewer reads to check the claim against the spec, and it described the applicability guard as asserting `supported: true` when it asserts `False`. No wire leak — buyer-facing parameters all use the neutral `RawIdempotencyKey` alias — but a reviewer checking the gate would have been reading the opposite of the truth. A guard now keys on the SHIPPED discriminant: if the wire says unsupported, no source file may claim support. If this seller ever advertises support for real, the guard inverts with it rather than going stale. Text-matching is the right shape because the defect IS the text; the wire side is already pinned by the sibling test. **The detached-ORM seam had no test.** `list_active_delivery_targets` returns a plain dataclass so the outbound worker can read `url` and `authentication_token` after the session closes. Reverting it to ORM rows left `tests/unit` at 6110 passed, and nothing imported `PushNotificationConfigUoW` at all. The new test drives the exact access pattern `webhook_delivery_service` uses, and pins the mechanism (not SQLAlchemy-mapped) as well as the symptom. The mutation reproduces the real production failure: `DetachedInstanceError`, raised in a background delivery thread. **The E2E control secret passed while empty.** Every assertion was a substring presence check, all satisfied by `ADCP_TEST_CONTROL_TOKEN=""` — an empty secret leaves the gated test-control endpoints reachable with no credential. The value is now graded by executing the runner's own generation line: non- empty, hex, >=32 chars, and different across runs. The file already uses that idiom for `_resolve()` and `record_gate_failure`. NITs, each a small correctness fix rather than a style change: - `_CONTEXT_UNSET` is now exported. It is compared by IDENTITY across modules (a private copy silently disables context derivation), so it is a contract, not an internal. This one is mine — the cross-module import came from my merge resolution earlier in this branch. - `_AttemptOutcome` dispatch gets an explicit sentinel: a member added later fell through and was RETRIED, which is the wrong default for anything not explicitly retryable. `ValueError`, not `AssertionError` — the import-usage guard flags the latter, and growing its allowlist to keep a stylistic choice would be the wrong trade. - `bounded_executor`'s over-release guard no longer `raise`s. It runs from a Future done-callback, where CPython swallows exceptions into the concurrent.futures logger — a fail-loud guard that could never reach a caller and read like one. It now logs at ERROR and clamps, so an over-release cannot silently widen the bulkhead it exists to bound. Mutation-verified: re-inverting one docstring reddens the posture guard; returning ORM rows reddens the snapshot tests with DetachedInstanceError; generating an empty token reddens the secret test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…er that exists Review follow-up. The original defect was a docstring claiming "a DENY-RAW rule, not a name allowlist" over code that was an allowlist — which is how sixteen raw-URL sites passed it. The strengthening in 8e4026b added url/webhook_url/callback_url/endpoint to the recognised set, which makes it a BETTER allowlist, not deny-by-default, and left the same claim standing. That reproduces the original bug one layer up. Measured, not assumed — only one of the three branches is deny-by-default: bare url (listed) hits=1 bare cb (UNLISTED) hits=0 bare dest (UNLISTED) hits=0 bare endpoint (listed) hits=1 attr .secret (UNLISTED) hits=0 unknown call hits=1 - ast.Call — DENY-BY-DEFAULT. An unenumerated call is flagged. - ast.Attribute — ALLOWLIST. `config.url` caught, `config.secret` not. - ast.Name — ALLOWLIST. `url`/`endpoint` caught, `cb`/`dest` not. The hybrid stays: deny-by-default on bare names would flag every name in every logger call. That is a legitimate engineering choice; claiming it is something stronger is not. The docstring now states the shape per branch, names the residual gap, and says why the allowlist branches cannot be deny-by-default. Three gap cases are added to the meta-test table as EXPECTED-0 with a comment saying they expect zero because that is what the matcher does, not because the shapes are safe. That makes the gap visible and testable rather than implied, and means widening a name set flips a documented expectation instead of silently changing behaviour. Mutation-verified: adding "cb" to _URL_NAMES reddens the pinned gap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rency Re-stacks this branch on its parent's current head instead of merging main directly. prebid#1546 had already merged upstream prebid#1868 (SDK-tree schema pinning) and resolved the 25 files it conflicts with, so taking that resolution once is cheaper and less divergent than repeating it here. Resolutions that were not mechanical: - Adopted 1546's `mcp_result()` seam in the MCP wrappers, keeping this branch's `raw_wire_payload` threading through to the `_impl` calls. - `wire_adcp_version()` is now the single source for the advertised version on the outbound delivery payload. - Moved 1546's `_json_safe_atom` into application_context_values.py, where this branch does the detaching, and applied it at both scalar sites. Without it a non-finite float or datetime in `context` raises inside an exception handler and the boundary emits no envelope at all. - Kept the idempotency-posture docstrings that describe advertised support: this branch ships `supported=True`, and 1546's posture guard keys on the shipped discriminant, so it early-returns here. - `_supports_reporting_frequency` logged buyer-controlled `raw_frequency` unscrubbed while scrubbing its neighbour — the extraction had reintroduced the exact miss 1546 fixed inline. Scrub restored inside the helper. - Cyclic `context`: serialization now breaks the cycle and emits the rest rather than dropping the whole context, matching the contract that lands on main with 1546. `validate_application_context` still rejects a cyclic context at the request boundary, so buyers keep the VALIDATION_ERROR. - A stray fragment of 1546's `_AttemptOutcome` dispatch auto-merged into this branch's inline retry loop, referencing an `outcome` that does not exist there; it raised NameError on every failed delivery. Removed. Ruff ignores F821 in this repo, so only a unit test caught it. - The retry loop's hardcoded `timeout=10.0` now uses `WEBHOOK_POST_TIMEOUT_SECONDS`, which the delivery deadline is derived from. Gates: make quality green (ruff, mypy 301 files, duplication 33/72/0 unchanged, 6282 unit passed / 0 failed). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…o fixed sites
The previous round reported the idempotency posture corrected at all seven
sites. It was six. The seventh —
`.claude/notes/pr1546-adcp-3.1.1-grounding.md:238`, the Spec-Grounding-Gate
record and the site called most consequential — was edited but never
committed: that commit staged `src/` and `tests/` only, so `.claude/notes/`
was silently excluded. The file has been byte-untouched this whole time,
still reading "the live `supported: true` discriminant matches the enforced
replay window" against a guard that asserts the opposite.
The guard could not have caught it, twice over, and both are the same
mistake — fitting the check to the sites that had just been fixed:
1. It scanned `src/` only, and the note is not in `src/`.
2. After widening the scan root, it STILL passed against the real note,
because the claim strings were the phrasings used in the source
docstrings. The note spells the same inversion differently: "the live
`supported: true` discriminant".
So the matcher is now patterns over substrings, keyed on assertions about
THIS seller's live posture. Descriptions of the schema union ("`supported:
true` requires replay_ttl_seconds") and of the rejected alternative
("`supported: true` claims every mutating call is safe to retry blind") are
legitimate and deliberately do not match.
`docs/releases/2.0.0.md` is EXCLUDED, deliberately, and this is not an
allowlist of inconvenient hits. It says 2.0.0 "advertises idempotency support
... with a 24-hour replay window", which is true of 2.0.0: that release
shipped 2026-07-01, and the flip to supported=false is 853706e, dated
2026-07-24, on this branch. A release note records what a past release did.
Forcing an edit there would falsify the changelog.
Mutation-verified against the real thing: restoring the note to the text
actually committed at HEAD reddens the guard, reporting
`.claude/notes/pr1546-adcp-3.1.1-grounding.md:238` — the exact line cited in
review. Before the pattern change, that same mutation left it green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… escape Two ways last round's guard could be defeated without touching a scenario. **The union was a hand-enumeration.** It listed four `*_WIRED` set names, with the completeness rule stated only in its docstring — "Add new *_WIRED sets here when you add them" — and no test behind the sentence. A fifth set would simply not be guarded. Same shape as the defects the guard was written for, which is the recurring criticism of this round: the check was fitted to the sites that existed when it was written. The union is now derived from this module's globals, and `test_wired_union_is_derived_not_enumerated` grades the derivation. Mutation-verified: a new `_UC099_NEW_FEATURE_WIRED` set joins the union with no edit anywhere. Coverage limits are now stated instead of implied — the lesson from the posture guard, which claimed more than it checked. `"context" in marker_names` and `_is_brand_shorthand_media_buy(...)` also bind an env in `_harness_env` and are NOT `*_WIRED` sets, so unbinding a `@context` step still yields xfails rather than failures; UC-003's `_UC003_TARGETING_OVERLAY` is function-local and cannot be discovered from module globals. Those are separate binding mechanisms, not missing entries, and promoting them is how they would join. **The guard's own remedy text reopened dormancy.** It offered "or remove the tag from the wired set". For UC-010 that is harmless — the catch-all in `_harness_env` raises. For UC-002 the catch-all is `pytest.xfail`, so the same one-line edit turns the guard green and silently restores the dormancy it exists to prevent. That difference is not something a reader should have to know before following the advice, so the clause is gone and the message now says what to do instead. Backing that up, `test_wired_sets_never_shrink` pins membership shrink-only: growth is free (wiring more scenarios is the goal), removal must edit a recorded baseline in the same commit. Mutation-verified: deleting `T-UC-002-v31-idempotency-replay` reddens it. The baseline was generated from the live sets after an initial hand-written version guessed four tag names wrong — recording reality, not recollection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both defects are this branch's own; merging prebid#1546 (and through it prebid#1868's SDK-tree schema pin) is what made them fail rather than what caused them. `test_push_notification_target_snapshot.py`, added by prebid#1546 to grade the detached-ORM seam, drives `list_active_delivery_targets`. This branch's subsystem replaced that method with `claim_delivery_targets`, which returns the same frozen `PushNotificationTarget`. Neither file conflicted, so the merge paired prebid#1546's test with this branch's repository and the whole media-buy integration shard failed on AttributeError. The test now drives `claim_delivery_targets` and seeds a media buy for its FK. Mutation-verified: returning ORM rows from the repository still reddens both tests with DetachedInstanceError, which is the production failure they exist to catch. `32f55e19a` seeded `reporting_capabilities` without `date_range_support` in the CI database and in the UC-004 BDD product fixture. The pinned SDK requires that field, so every read of `prod_display_premium` failed Product validation and `get_products` answered SERVICE_UNAVAILABLE — 14 of the 17 E2E failures at cdb42d8, from one partial dict. Both seeds now carry it. Swept the pattern rather than the cited sites: the one other partial block, src/adapters/broadstreet/managers/inventory.py:281, is from prebid#1013 with no commits in this branch's range and feeds admin suggestions rather than a validated Product. Left alone, tracked separately. Gates: make quality green (mypy 301 files, duplication 33/72/0 unchanged, 6282 unit passed / 0 failed); integration media-buy shard 434 passed / 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… "already sanitizes"
The scan set was a `src/services/*webhook*.py` glob plus three named files.
That criterion is self-limiting: a module which sanitizes nothing is
structurally exempt, so the completeness test could only ever pull in modules
that were already partly safe. Demonstrated in review — adding a raw-URL
logger call to `order_approval_service.py` left the file at 48 passed. Four
modules import the URL helpers and sat outside the set:
`order_approval_service.py`, `context_manager.py`, `creatives/_sync.py`,
`media_buy_create.py`.
The set is now derived: any module under `src/` that mentions the URL helpers
or the scrubber is scanned.
Widening surfaces 48 PRE-EXISTING violations across four modules this PR does
not otherwise touch (media_buy_create.py alone has 36 raw `{media_buy_id}`
interpolations). Fixing those is separable from fixing the criterion, and
sweeping 48 sites through untouched files was declined as scope earlier in
this PR. They are recorded as a per-module RATCHET — the repo's established
pattern for this exact situation (.duplication-baseline, .type-ignore-baseline,
.ruff-complexity-baseline) — not an allowlist:
- every module NOT in the dict is held at ZERO, so the modules this PR touched
cannot regress, and any newly-scanned module starts at zero too;
- counts may only fall. `test_the_ratchet_never_loosens` fails if a module
gets cleaner without its budget dropping, which is how a ratchet quietly
becomes an allowlist, and fails if a listed module leaves the scan set.
prebid#1953 tracks the order_approval_service.py and core/webhook_delivery.py sites.
Also fixes the `%`-operator arm, which modelled only a scalar right-hand side,
so every multi-value form — the commonest shape in this codebase's log calls —
was invisible. `logger.error("%s %s", url, exc)` now reports 2 hits; it
reported 0.
Mutation-verified: adding one raw-URL logger call to order_approval_service.py
now reddens with "9 raw channels, ratchet allows 8". Before this change the
same edit left the guard green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…sion Two homes for one number. `_wait_before_retry` hardcoded `(2**attempt) + random.uniform(0, 1)`; `_worst_case_delivery_seconds` re-expressed the identical formula to derive the deadline from it. A derivation that re-implements what it derives from is not a derivation: widening the REAL backoff to `3**attempt + uniform(0, 5)` — 52s against a 38s deadline, reinstating the original defect this round was meant to close — left 92 tests passing. `WEBHOOK_RETRY_BACKOFF_MAX_JITTER_SECONDS` had zero production readers, which was the tell. `webhook_retry_delay_seconds(attempt, *, jitter=None)` is now the single definition. The loop sleeps it; the deadline sums it with `jitter` pinned to the maximum so the budget is worst-case rather than a sample. The deadline also covers ADMISSION and DNS now, not just execution. The bulkhead starts the clock BEFORE granting a permit and there are only WEBHOOK_DELIVERY_MAX_WORKERS of them, so an execution-only budget cancels the 5th concurrent target's retries — the same defect, moved from the retry loop to the queue. `_enqueue_and_deliver_target`'s docstring already claimed the deadline "covers admission", so the execution-only figure made that claim false. 38.0s -> 78.0s (own retries + worst-case permit wait + DNS timeout). Two oracles, because one number pinned in one place was exactly the gap: - `test_the_deadline_moves_when_the_REAL_backoff_moves` patches the shared delay and asserts the derived budget follows it. Asserting the NUMBER cannot catch two-homes drift; asserting the DEPENDENCY can. - `test_the_service_sleeps_the_shared_delay` covers the other direction — the loop must call the shared definition, not its own copy. Mutation-verified with the reviewer's exact edit: widening the real backoff to `3**attempt + uniform(0, 5)` while leaving the derivation alone now reddens. Fallout handled rather than papered over: moving the jitter took the last use of `random` out of webhook_delivery_service, and two harnesses patched `src.services.webhook_delivery_service.random.uniform`. Both now patch `src.core.security.webhook_http.random.uniform` — the patch target follows the code, which is the point of having one home. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…l default Patching this fixture field-by-field as CI names them does not converge. The block was hand-written as a two-key subset; the pinned SDK requires six, so adding `date_range_support` in the previous commit just moved the error on to `expected_delay_minutes`, `timezone` and `available_metrics`. Product validation then failed at read time and the transports answered SERVICE_UNAVAILABLE — which is not the rejection these credential-length scenarios assert on, so all three transports failed for a reason unrelated to credential length. It now starts from `_default_reporting_capabilities()` — the same default the Product field itself uses — and overrides only the two values the scenario cares about. A future required field arrives through the default instead of through a CI failure. Mutation-verified: restoring the two-key subset reddens all three transports; deriving from the default greens them. The rest of UC-004's failures are the pre-existing delivery cluster (18 x "No webhook POST was made" and its downstream circuit-breaker/backoff/probe assertions), unchanged by this. Gates: make quality green (mypy 301 files, duplication 33/72/0 unchanged, 6282 unit passed / 0 failed); uc004 credential-length scenarios 3/3 on a2a, mcp and rest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
**The "total" log helper stopped being total.** Folding `scrub_control_chars`
into `webhook_url_for_log` put a `urlparse` behind a docstring promising
"never raw", and `urlparse` RAISES on some malformed input rather than
degrading: `urlparse("https://[fe80::1")` is `ValueError: Invalid IPv6 URL`.
The helper is installed at fourteen delivery-service sites, several inside
`except` handlers where the `scrub_control_chars` it replaced was total by
construction — so a raise there replaces the delivery failure being logged
with an unrelated one. Reachability is bounded (registration rejects such
URLs), but a helper called from exception handlers has to be total by
construction, not by its callers' good behaviour. The parse is wrapped; the
placeholder is pinned. Writing the mutation surfaced a second raising input
the review did not list — `https://[]` gives `ValueError: '' does not appear
to be an IPv4 or IPv6 address` — which the same wrap covers.
**The last-resort envelope guard sat one line below the call that raises.**
`build_two_layer_error_envelope` walks the buyer's context and is the likelier
of the two calls to fail, but it was ABOVE the `try`, so the guard covered
only `JSONResponse` and the failure it exists to prevent — a raise inside the
exception handler, hence no response at all — still had an open path. Both
calls are now inside.
Moving it exposed a second bug in my own fix: the `except` popped `context`
from `envelope`, which is unbound when the BUILDER is what raised. The retry
now distinguishes the two cases (clear the context and rebuild vs pop and
re-encode) and ends in a terminal net built from scalar attributes, so it
cannot depend on anything that has already failed twice.
**The agent-card comment justified the URI with something false.** It said the
URI is held at patch precision because it "is a schema PATH … which exists at
patch precision". `dist/schemas/3.1.1/protocols` returns 404 and
`adcp-extension.json` was removed upstream in v3. I had verified
`dist/schemas/3.1.1/core/context.json` and generalised without checking — an
inference presented as fact, and I wrote a test assertion pinning it.
The URI is a legacy extension IDENTIFIER, an opaque match key held at its
historical value so existing clients keep matching; the variable is renamed to
say so. The SPLIT itself remains correct and is spec-grounded — only
`params.adcp_version` is a negotiation value, and it is release precision per
core/version-envelope.json. The comment and the assertion message now say
that, and the note gains the Spec-Grounding-Gate section this split never had,
including that the URI is ungraded and that its divergence from the pinned
guide's identifier is wire-affecting and tracked separately.
Mutation-verified: unwrapping the urlparse reddens the totality oracle on four
inputs; moving the builder back above the try reddens the new builder-raise
test (it produces no response at all, which is the original failure mode).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lly reads
The 42 UC-004 delivery failures were five defects stacked so that each hid the
next. Production code is unchanged; every fix is in the test setup or in a
scenario that contradicted the pinned spec.
1. No POST was attempted at all (18). Reporting webhooks are a media-buy-scoped
channel: `_send_webhook_enhanced` resolves the typed `reporting_webhook`
persisted on the media buy and returns False, silently, when it is absent.
The Given steps seeded a PushNotificationConfig row — the task-status
channel — and two of them never created the media buy they name at all,
which went unnoticed while the old lookup keyed on tenant+principal rather
than the buy. The webhook config is now written onto the media buy's
raw_request, re-runnably, because a scenario's credential Given lands after
the one that names the scheme.
2. The report was read at the wrong level (12). The body on the wire is the
AdCP webhook envelope; `notification_type`, `sequence_number` and
`media_buy_deliveries` live under `result`. Reading them off the envelope
yielded None rather than failing, so the steps reported a missing field
instead of a mis-addressed read. Confirmed against the pinned spec:
"Delivery-report content lives under `result`; it is not valid as the
top-level POST body by itself." One unwrap now serves every per-call
consumer, with a separate raw-body accessor for envelope-level fields.
3. Delivery targeted the wrong media buy (15). Five When steps called the
harness with its default `mb_001` while the Givens create `mb-001`.
4. The 4xx no-retry scenario graded the one 4xx the spec exempts. AdCP 3.1.1,
Persistent channel contract / Auth renewal: a 401 from the receiver SHOULD
be treated as transient and retried on the standard schedule. A
reporting_webhook is a persistent channel, so 401 cannot grade
BR-RULE-029 INV-4; the scenario now drives 403.
5. HMAC (3). The scenario demanded an ISO `X-ADCP-Timestamp`, but the legacy
profile specifies Unix seconds and signs `{unix_timestamp}.{raw_json_body}`,
so an ISO value could never verify. The check also re-serialized the decoded
payload instead of signing the bytes on the wire — the exact failure the
spec calls out. It now signs the actual POST body.
Both scenario corrections carry the verbatim spec quote and a LOCAL DIVERGENCE
marker so a merge-mode regeneration does not silently revert them.
Three latent defects surfaced on the way, none of them on the original list:
the circuit-breaker-open step passed vacuously (its `False` came from the
missing media buy, not from suppression); the auth-log step reported a missing
harness capability whenever the capture was merely empty; and
`_ensure_media_buy_in_db` created a tenant without checking the DB, unlike its
sibling, so any caller running after the env seeded its own collided on the PK.
Every fix mutation-verified against production: reverting the 403 carve-out,
and signing with a different secret, each redden the scenarios that claim to
grade them.
Gates: uc004 module 464 passed / 0 failed (was 42 failed); make quality green
(mypy 301 files, duplication 33/72/0 unchanged, 6282 unit passed / 0 failed).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…aths are scrubbed
**Item 7 — A2A's boundary record wrote a log line, not a record.** It passed
identity=None, and `record_boundary_error` returns before the activity feed
and the audit row without a tenant. So "recorded" meant nothing durable and
the in-code claim that the three boundaries stay in lockstep was still false.
REST resolves a header-only tenant for exactly this case
(`_best_effort_rest_identity`), and A2A already had the same capability twenty
lines below — an unauthenticated DISCOVERY request resolves its tenant from
headers with no token. `_best_effort_a2a_identity` mirrors REST's contract,
including degrading to None on any failure so observability cannot shadow the
buyer's error.
Author decision, taken deliberately: a header-resolved tenant is NOT an
authorization decision. It scopes WHERE the refusal is recorded; the caller is
still unauthenticated and still refused.
The test that pinned the absence is handled honestly rather than deleted. It
still asserts `assert_not_called` — but the REASON changed and now says so:
that request carries no headers, so there is no tenant to resolve, not "the
boundary passes None on principle". The parity it used to contradict is graded
by a new test with a resolvable tenant, which asserts the exact activity-feed
and audit calls. Mutation-verified: reverting to identity=None reddens it.
**Item 10 — an unrecognised auth scheme delivered unauthenticated, silently.**
Both dispatches fell through with no `else`. Fixing the case-sensitivity
instance last round left the shape, which is why it recurred. Both now warn,
naming the scheme and the supported set. A falsy `authentication_type` (no
auth configured) is a different, legitimate case and stays quiet. The two
paths support genuinely different sets — delivery does HMAC+Bearer because
only it signs; approval does Bearer+Basic — so they are named rather than
merged.
**Item 8 — buyer-controlled `request.url.path` reached loggers unscrubbed.**
`urlsplit` strips CR/LF, but VT, FF, U+2028 and NEL survive and each splits a
log line — the class this repo's own TestControlCharClassCompleteness derives
from `splitlines()` and names log forging as the risk. Scrubbing `operation`
inside `record_boundary_error` closes all four of its logger calls at one
seam; the two new `src/app.py` sites are scrubbed directly.
Widening those two files' imports pulled them into the log-scrub scan set,
which immediately flagged three PRE-EXISTING raw `{e}` sites in them. Fixed,
not ratcheted — three lines in files this commit already edits, and adding
them to the ratchet when the fix is that small is the move the ratchet exists
to prevent.
Nits, each a correctness fix rather than a tidy-up:
- the unhandled-outcome guard now logs and treats the outcome as terminal
instead of raising. The raise was caught by `except Exception` in
`send_delivery_webhook` and relabelled a generic failure, so the fail-loud
guard never reached a caller.
- `_AsyncAdmissionGate`'s clamp gets a test that drives it; swapping a raise
for a clamp with nothing exercising it just relocates untested code.
Mutation-verified: removing the clamp inflates capacity to 3 against a
capacity of 2.
- `validate_webhook_url_registration`'s docstring named ADCP_TESTING after the
body moved to the shared predicate (fail-closed drift, but the same
prose-vs-code gap this PR keeps finding).
- the agent-card local import states its reason; the A2A README no longer
documents one version driving both values.
- the in-process agent-card assertion pinned the URI as a LITERAL. Rebuilding
it from `get_adcp_spec_version()` — production's own call — asserted
production against production, so it could not fail.
- the three UC-003 `revision` rows are annotated in the feature, and the
recorded xfail reason no longer says merely "not yet wired": those rows
encode the post-prebid#1689 contract and contradict shipped behavior, so wiring
them turns them RED. Graduation is gated on prebid#1607/prebid#1689, not harness work.
A structural guard also caught the durable-row test using
`assert_called_once()` + `call_args`; it now uses `assert_called_once_with`
with the full expected call.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…pping
Three unrelated causes behind the remaining red, each introduced by this branch.
**list_accounts filters never reached the endpoint (20 BDD failures).** Every
failure was `[rest-*]`; MCP and A2A passed, because their dispatchers unpack a
typed `req` into flat arguments. `BaseTestEnv.build_rest_body` does the same —
but `AccountListEnv` overrode it with a flat-fields-only version that silently
dropped `req`, so REST POSTed an empty body and the endpoint answered with the
unfiltered, unpaginated account list. The scenarios then graded 200 accounts
against an expected 100, and an unfiltered `active` against an expected
`closed`. The override is load-bearing — the read-idempotency and
validation-parity tests pass flat boundary fields — so it now serves both call
shapes instead of one.
**on_get_task / on_cancel_task stopped sending `data` (2 E2E failures).**
`_require_owned_task` replaced `_get_task_or_raise` when task lookup became
durable and ownership-scoped, and its raises carry only a message. The test
that failed exists to pin both halves; its docstring says so explicitly:
asserting the message alone would let `data={"task_id": ...}` be deleted with
the suite still green, because the id appears in the message either way. All
three live raises carry the structured payload again. `_get_task_or_raise` had
no callers left and is removed; two docstrings still described it as the shared
path and now name `_require_owned_task`.
That test also predates the identity resolution the durable path added, so it
was reaching for a database it never had. It stubs the identity and the UoW
rather than acquiring one: this is the fast smoke check on the raise, and a
real database would grade persistence, not the error shape.
**The e2e control token was never generated in CI (1 E2E failure).** The
per-run secret gating the stack's setup controls is created by
run_all_tests.sh, which the e2e job does not use — it invokes pytest directly,
so `os.environ["ADCP_TEST_CONTROL_TOKEN"]` raised KeyError. The job now
generates its own before the stack comes up. The in-network job needs no change
because it runs through run_all_tests.sh.
Mutation-verified: dropping `data` again reddens both task-not-found cases.
The REST fix is a straight before/after on the same selection — 10 failed, then
39 passed.
Gates: uc011 module + all five integration files using AccountListEnv, 250
passed / 0 failed; TestA2ARequestHandler 6 passed with DATABASE_URL unset (the
CI condition); make quality green (mypy 301 files, duplication 33/72/0
unchanged, 6282 unit passed / 0 failed).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…m [order R1-2]
…block [order R1-4]
…ment reference [order R1-6]
…hrough Three references still pointed at `_get_task_or_raise`, which the previous commit deleted once `_require_owned_task` took over — including one describing it as "the shared" path both entry points route through. A reader following those would look for a method that no longer exists, and the docstring is the artifact that explains why the test pins `data` at all. Verified separately: the full in-network BDD suite passes on this tree — 428 passed, 0 failed, 34 xpassed (test-results/innet_130826_1030/bdd_e2e.json, non-green 0), including the two uc006 account-resolution scenarios that failed in CI. That failure is not reproducible here and no mechanism was found in the diff, so this push is also the fresh in-network run that discriminates a CI-side flake from a real regression. Gates: make quality green (mypy 301 files, duplication 33/72/0 unchanged, 6282 unit passed / 0 failed). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…R1-3 follow-on] Two edits the per-order commits did not carry. **The shrink-only baseline never gained the hoisted sets.** R1-3 hoisted `_UC003_MANUAL_APPROVAL_WIRED` and `_UC003_REQUIRED_IDEMPOTENCY_WIRED` to module level so `_wired_scenario_tags()` can see them, but `_WIRED_BASELINE` still listed only the original four. So the two newly guarded sets could be deleted again with nothing objecting — the guard would simply stop seeing them, which is the failure mode the baseline exists to prevent. Found by mutation, not by review: renaming `_UC003_REQUIRED_IDEMPOTENCY_WIRED` back to a non-`_WIRED` name left `test_wired_sets_never_shrink` GREEN. With the baseline corrected the same mutation reddens with "wired sets removed wholesale: ['_UC003_REQUIRED_IDEMPOTENCY_WIRED']". This is the second time this round a supposedly-applied edit turned out not to be in the tree, and both times the mutation caught it rather than a re-read. **`_safe_request_path` is registered as a sanitizing call.** R1-1 introduced it to keep the terminal net's log line from reading the buyer-controlled path unguarded. The log-scrub matcher's Call branch is deny-by-default, so an unrecognised call is flagged even when it sanitizes — the branch behaving correctly. The helper scrubs internally and returns a placeholder when the read fails, so it belongs in `_SANITIZING_OR_SAFE_CALLS` rather than being worked around at the call site. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…he guard's reach R1-B5. The stack base was 14 commits stale, six of them touching files this branch rewrites, so a merge at integration time would have resolved in favour of this side and reverted them silently. Merged now instead, with the four verified-absent fixes confirmed present afterwards: webhook_retry_delay_seconds, _minimal_envelope, _safe_request_path, _safe_status_code. `_wait_before_retry` takes prebid#1546's single-home delay, so the deadline and the sleep no longer derive the same formula independently. Two conflicts resolved in favour of this side, each checked rather than assumed: - prebid#1546 warns and then sends UNAUTHENTICATED when a configured scheme is unusable. This side's validate_webhook_auth_selector REFUSES both causes it separated — an unsupported scheme, and a recognised scheme with missing or under-length credentials. Refusing is stronger than warning, but those refusals were graded nowhere, so the fix's intent is preserved as a test that pins the two causes to distinct messages. - prebid#1546's _AttemptOutcome guard has no subject here: this side classifies each attempt inline and defines no `outcome`. Re-extracting the enum is tracked separately (R1-S3), not silently dropped. R1-B6, found by adopting prebid#1546's widened log-scrub criterion — derived from "handles buyer URLs" rather than "already calls the scrubber", which is the stronger rule and reaches modules that sanitize nothing. It went red on this tree. Measured with the guard's own oracle: media_buy_create.py 36 -> 39 raw buyer channels creatives/_sync.py 2 -> 3 This branch added four raw buyer-channel log interpolations AND removed the scrubber call that kept media_buy_create.py inside the scan set, so three of them became invisible to the guard that exists to catch exactly that. All five sites are redact_idempotency_key() as a positional logger arg — a truncation, not an escape: it keeps whatever the buyer put in the key. Wrapping them in scrub_control_chars fixes the interpolation and restores scan set membership, because membership is keyed on the marker. Counts are now 35 and 2, and the ratchet is LOWERED 36 -> 35 as its own rule requires. No ratchet was raised, and media_buy_create.py was not dropped from the scan set — that would have hidden 35 sites rather than 3. Refuted from the same finding: the branch is NOT 1 commit behind main. `git rev-list --count HEAD..upstream/main` is 0; dd0bee9 is on the fork's main, never on prebid/salesagent. Gates: make quality green (mypy 301 files, duplication 33/72/0 unchanged, 6304 unit passed / 0 failed); scrub guard 49 passed; signing-config 10 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
R1-B1. Anonymous reads persisted (tenant, NULL, NULL). The lookup index is NULLS NOT DISTINCT, so that is not a weaker scope — it is ONE row-space and one insert-rate bucket shared by every unauthenticated caller of a tenant. Keys are client-chosen and the tenant is addressable via x-adcp-tenant, so a second anonymous caller reusing a key either collides (IDEMPOTENCY_CONFLICT) or is handed the first caller's cached envelope, and one caller's read burst rate-limits every other anonymous reader tenant-wide. Grounded before choosing, not after. AdCP 3.1.1 security.mdx: "Keys are scoped per (authenticated agent, account) — they have no meaning across agents on the same seller, across accounts under the same agent, or across sellers", and "Every piece of state — media buys, creatives, idempotency cache entries, session IDs, governance tokens — is scoped to the account that owns it." An unauthenticated caller supplies neither half of that tuple. That rules out both alternatives. A discriminator would have to invent an identity the spec does not define, from forgeable material. Silently ignoring the key is the one thing the spec forbids outright: a seller that accepts a supplied key MUST apply the replay contract. Refusing is what is left, and it is refused before any durable row is reserved and before the read runs. The integration test grades the absence of the row, not just the error. The row is what made cross-caller replay reachable, so asserting only the error code would leave the mechanism in place with a green suite. Mutation-verified: disabling the guard reddens both the unit test and the wire test. test_read_replay_never_executes_work was anonymous only incidentally — its subject is that a replay skips work() — so it gains a principal rather than an inverted assertion. Gates: make quality green (mypy 301 files, duplication 33/72/0 unchanged, 6304 unit passed / 0 failed); read-idempotency orchestration 5 passed; transport persistence 15 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tripping The branch went CONFLICTING against main while main advanced 7 commits, and a DIRTY merge state means GitHub cannot build the synthetic merge ref — so every `pull_request` workflow was silently SKIPPED. The two blockers pushed before this were graded by nothing; the checks page showed a near-empty run rather than a red one. A branch that is slightly ahead of its parent and tested beats a pristine one that CI cannot see, so main is merged here rather than waiting for prebid#1546 to take it. Two conflicts, both test-only, resolved toward whichever side is the stronger contract: - test_list_creatives_concept_filter.py takes MAIN's version: it is parametrized across every wire where this branch's was REST-only, and each field names its own drop message instead of a shared "concept" label. 32 pass, up from the REST-only subset. - tests/helpers/__init__.py is a union — both `__all__` entries are live. One contract collision needed a decision rather than a side. Main's prebid#1616 added a CodeQL CRLF test asserting buyer ids are STRIPPED; this branch had already moved `log_safe` to ESCAPE, delegating to scrub_control_chars so the control-char defense has one home. Escaping is stronger on both axes: it neutralizes every forge-capable character rather than only CR/LF, and it keeps the id readable instead of mangling it into a different value. The property the CodeQL finding actually requires — the suffix stays a single line — is asserted either way and is untouched. The expected value now reflects the escaping contract, with the reason recorded at the assertion. Note for the reviewer: `check-pr-title` failed on the previous head with `##[error]No server is currently available` — GitHub's own 503 inside the action, not the title, which satisfies the workflow's `feat:` type and `^.+$` subject pattern. Gates: make quality green (mypy 301 files, duplication 33/72/0 unchanged, 6341 unit passed / 0 failed); concept-filter 32 passed; blob coercers 37 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The run at fb6d8eb had 10 jobs fail, every one of them before reaching a test: 8 with 'Failed to download archive / Too Many Requests' fetching an action from codeload, check-pr-title with 'No server is currently available' from the API, and the Summary rollup of those. GitHub declared a Partial System Outage at 13:40Z with an approximate 50 percent error rate on archive downloads and 20 percent on API traffic; the 20 jobs that did download their actions all passed. Nothing in the tree changed. This empty commit exists only to get one honest reading of B1, B5, B6 and the main merge together, now that the incident is mitigated, before any further work stacks on top of commits CI has never graded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
GitHub reports All Systems Operational; the two remaining red checks are the
original jobs from the outage window and have not re-executed. Both failed
before doing any work: CodeQL died in codeql-action/init ('Encountered an error
while trying to determine feature enablement: HttpError: No server is currently
available'), and check-pr-title died on the API call it makes to read the
title. The title itself is conformant — type 'feat' is in the workflow's
configured list and the subject satisfies subjectPattern ^.+$.
The substantive suite already passed at this tree: 28 of 30 checks green,
including Unit, all five Integration shards, both BDD shards, BDD In-Network,
E2E, Admin, Smoke, Quality Gate and Migration Roundtrip.
Fork PRs cannot use 'gh run rerun', so a push is the only way to re-trigger.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Third data point on a signature that has now appeared twice in the in-network job and never locally: a typed error arriving as INTERNAL_ERROR / HTTP 500 at account resolution. It hit uc006 on one run and uc002 on another, and the same tree passes the full in-network suite locally both times (428 passed, 0 failed, non-green 0 in test-results/*/bdd_e2e.json), with 486 uc002 tests among them. No mechanism exists in the diff. The envelope-fallback explanation was ruled out because the first occurrence predates that merge. The remaining reading is an environment-dependent failure during account resolution: an unexpected exception there is correctly reported as INTERNAL_ERROR, so whichever scenarios expect a typed domain error fail — different victims, same cause. The previous occurrence cleared on the next run with no code change. If this one clears too, that is three readings pointing at the environment. If the same five fail again, it is real and the next step is capturing the server-side traceback from the in-network stack, which neither run has produced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The durable idempotency/concurrency subsystem, rebuilt as the standalone
change it is — split out of #1546 on 2026-07-30 so that PR stays at its
reviewed surface.
Branch construction: this branch is the split #1546 head (
2c9c308e6)plus the three subsystem commits that briefly lived there
(
04d878fe4/ea56fcf81/32f55e19a, cherry-picked from the pre-splithistory) plus port reconciliation. It is stacked on #1546: until #1546
merges, the diff against main shows both; review here should focus on the
subsystem commits. The previous 2026-07-15 probe-first snapshot of this
branch was superseded and force-replaced.
The subsystem
idempotency_keyacross MCP, A2A, and REST; supplied keys reserve, cache thecanonical typed response, and replay byte-stably with
replayed=true;cross-tool reuse returns
IDEMPOTENCY_CONFLICT; anonymous public readspersist
principal_id=NULLin tenant/account scope.executions release their attempt reservations; consequential provider calls
are preceded by persistent deterministic downstream-operation claims;
reconciliation is explicit (
APPLIEDreconstructs,NOT_APPLIEDinvokesonce,
UNKNOWNfails closed). GAM, Broadstreet, Kevel, Triton, and Mockreconcile via stable provider identifiers; Xandr is rejected before
invocation.
upserts, notification outbox, RFC 9421 default signing of the exact
transmitted bytes, and
idempotency.supported=truewithreplay_ttl_seconds— this branch is what makes the agent-widetruedeclaration honest (feat: AdCP 3.1.1 version negotiation, envelope tolerance, and bearer normalization (#1512) #1546 ships
supported=false+ FIXME(update_media_buy: implement revision → CONFLICT optimistic concurrency (deferred from #1546) #1607) until thislands).
a4d7e8c91f20(attempt principals nullable,operation_classbackfill, downstream-mutation claims) andb5c8f1d20a37(task/webhook reliability state), following
f3a1c92b47defrom feat: AdCP 3.1.1 version negotiation, envelope tolerance, and bearer normalization (#1512) #1546.Verification
At head
cf366f2e2(locally, per-worktree Postgres + pinned creative agent):env -u DATABASE_URL make quality: 6,152 passed / 9 skipped / 26 xfailed(ratchet baselines regenerated for the ported tree; complexity shrank).
transient DB-connection setup error cleared on serial re-run).
Draft: post-push GitHub checks are the CI authority; e2e/admin/BDD suites run
there. Full spec grounding for the graded idempotency behavior is carried in
the subsystem commit messages and
.claude/notes/pr1546-adcp-3.1.1-grounding.md.Closes part of #1512's follow-on (#1247 item 1) and implements #1607.