feat: RFC 9421 request signing — key lifecycle, trust root and inbound verifier (#1291) - #1757
Draft
KonstantinMirin wants to merge 315 commits into
Draft
feat: RFC 9421 request signing — key lifecycle, trust root and inbound verifier (#1291)#1757KonstantinMirin wants to merge 315 commits into
KonstantinMirin wants to merge 315 commits into
Conversation
The A2A discovery-skill dispatch conflated "is auth required for this skill set" with "should a presented token be validated" — an invalid token on a discovery-only request (get_adcp_capabilities, list_accounts) was silently swallowed as anonymous instead of rejected, since require_valid_token was set from the skill-based requires_auth boolean rather than from whether a token was actually presented. A presented token must always be validated: absent token -> proceed anonymous (fine); presented-but-invalid token -> reject with AUTH_INVALID (terminal), independent of the requested skill's own auth requirement. Un-xfail the two dormant scenarios this graduates. Also reconcile three xfail-ledger entries left stale by the salesagent-rrz8 MCP-serialization fix (a real regression against this epic's baseline, caught during this bug's sweep verification) — all three scenarios now genuinely pass. salesagent-7moz
…tion required a principal INV-4 (AdCP v3.1.1): capability discovery describes the SELLER, not the caller — response data must not vary by auth state. get_adcp_capabilities gated adapter resolution on principal_id being truthy, so anonymous callers got generic channel/targeting defaults while authenticated callers got adapter-derived values from the tenant's real ad server. Extract resolve_tenant_adapter_type() as the single source of truth for adapter-type resolution (previously get_adapter() and products.py's get_adapter_default_channels() read from two different, potentially divergent sources), and get_adapter_class_for_tenant() to resolve the adapter CLASS without instantiating it — Kevel and TritonDigital require a principal-bound config in __init__ and would crash for a synthetic Principal, so a tenant-only capabilities-read path must never construct an adapter instance at all. Convert get_targeting_capabilities() to a staticmethod across the base class and all overrides, making principal-independence a structural guarantee rather than an unenforced convention. capabilities.py now resolves adapter data unconditionally, regardless of auth state. Also fix a live BDD scenario (@T-UC-010-ext-c-mcp) that had pinned the old buggy behavior as expected, and reconcile the dormant @T-UC-010-auth-data-identity scenario now that it passes for real. salesagent-dn2s
Three genuine issues surfaced by the full saci run after all 5 bugs in this epic landed, none of them incorrect logic in the fixes themselves: 1. _assert_wire_rejection's (uc004_delivery.py) auth-failure exclusion set hardcoded the literal string "AUTH_REQUIRED" to keep an auth failure from masquerading as a legitimate client field rejection. Once salesagent-mkso split that code into AUTH_MISSING/AUTH_INVALID, the stale exclusion let a genuine auth failure (resolve_principal_or_raise emitting AUTH_INVALID for an unresolvable principal) pass an assertion meant to exclude auth failures — flipping a correctly-xfailed ownership scenario (C3, a real unrelated production gap) to XPASS(strict). 2 and 3. Two e2e_rest-only scenarios (BR-UC-010 auth-data-identity, BR-UC-003 principal-not-found) now fail over real HTTP for reasons unrelated to salesagent-dn2s/mkso's actual logic: an e2e tenant-fixture subdomain gap (anonymous requests can't resolve a tenant via the Host-header "default"-subdomain fallback) and a harness-vs-real divergence in how "principal deleted after token issued" is simulated in-process vs resolved over a real request. Both dispatch through real production code on all in-process transports (a2a/mcp/rest) with no issue; only e2e_rest, which this epic's fixes correctly un-xfailed scenarios, exposes them. Ledgered with root-cause analysis; follow-ups filed (salesagent-zna9, salesagent-z9e0). salesagent-jl20
…zation errors Completes the salesagent-mkso AUTH_MISSING/AUTH_INVALID migration for the two axes left on the deprecated AUTH_REQUIRED alias: - Tenant-resolution failures (require_tenant, get_strategy_manager, ensure_tenant_context) now split on whether a credential was actually presented (identity.auth_token): no token at all -> AUTH_MISSING (correctable); a token was presented but the tenant still didn't resolve -> AUTH_INVALID (terminal). AdCPTenantContextError is removed. - AdCPAuthorizationError (ownership mismatches, admin-only actions, account access, brand-manifest policy) now emits PERMISSION_DENIED per v3.1.1 error-code.json -- not AUTHORIZATION_REQUIRED, which is a distinct downstream-platform-authorization gap per the spec's enumDescriptions, not "authenticated caller lacks permission." salesagent-otc5
…lvable Principal INV-4 class bug (same shape as salesagent-dn2s): whether an adapter supports a pricing model is a fact about the SELLER's ad server, not the caller, so it must not depend on identity.principal_id resolving to a real DB Principal. The annotation block was gated on a resolved Principal solely to construct a get_adapter() instance -- but get_supported_pricing_models() has no per-principal state in any of the 4 adapters, exactly like get_targeting_capabilities() before dn2s. A caller whose principal_id doesn't resolve to a DB row (stale/deleted principal, cross-tenant token) previously got full unmasked pricing (BR-RULE-004-01 suppression only checks principal_id, not the resolved object) with zero adapter-support annotation -- worse than both the anonymous and authenticated states. Converts get_supported_pricing_models() to @staticmethod across all 4 adapters and resolves the adapter class tenant-level via the existing get_adapter_class_for_tenant() helper (dn2s), instead of constructing a Principal-bound adapter instance. salesagent-r9rf
…ymous callers _resolve_auth_dep() (the FastAPI dependency backing get_capabilities, get_products, list_creative_formats, and list_authorized_properties) short-circuited to bare None whenever no auth token was presented or a presented token didn't resolve to a principal -- BEFORE ever calling resolve_identity(). This skipped header-based tenant detection (Host / x-adcp-tenant / Apx-Incoming-Host) entirely for anonymous REST discovery callers, in violation of AdCP INV-4 (discovery responses describe the seller, not the caller). MCP and A2A don't have this bug: resolve_identity_from_context() always calls resolve_identity() regardless of token presence. Only observable over e2e_rest because the in-process REST test dispatcher overrides the FastAPI dependency directly with the test's identity object, bypassing _resolve_auth_dep()'s real logic -- which is why in-process rest/mcp/a2a all passed while e2e_rest uniquely failed. Originally filed as an e2e test-fixture gap; tracing the real HTTP headers on the live stack showed the fixture was fine -- the production dependency was the actual bug. _resolve_auth_dep() now always resolves identity from headers, matching MCP/A2A's contract. Downstream code already distinguishes "no credentials" via identity.principal_id (require_principal_id, brand_manifest_policy), not via identity being None. salesagent-zna9
…agent-zna9 The zna9 fix removed the uc010 auth-data-identity entry from tests/bdd/e2e_rest_known_failures.txt but left the paired pin in EXPECTED_LEDGER (tests/unit/test_e2e_rest_ledger_state.py), which asserts the two stay in lockstep -- broke test_ledger_matches_expected_genuine_gaps and test_conftest_loader_reads_this_ledger. Caught during salesagent-z9e0's architect review, which flagged the same pattern for its own ledger edit. salesagent-zna9
…oken resolution tests/harness/_base.py's identity_for() unconditionally passed principal_id to PrincipalFactory.make_identity(), even when the DB token lookup found no matching Principal row -- diverging from production's resolve_identity(), which nulls principal_id whenever the token->principal lookup fails. This let in-process transports (impl/mcp/a2a/rest) see a stale non-empty principal_id for a deleted or never-created principal, reaching update_media_buy's ownership check (AUTH_MISSING wasn't reached) while e2e_rest -- which re-resolves via the real HTTP path -- correctly nulled it and got AUTH_MISSING first. identity_for() now gates the null on the DB lookup genuinely running (self._session bound) and finding no row, distinguishing that from a session-not-bound timing case that must NOT null principal_id. Fixing this surfaced a second, more consequential bug: the shared authenticate_env_as() step helper (reused by uc003/uc006/uc018 -- BDD step text is global across feature files) eagerly accessed env.identity right after switching principals, before later Given steps create the corresponding Principal row. Because identity_for() caches per-protocol, that eager access would permanently poison the cached identity with principal_id=None even after the row is created. Removed the eager access/assertion, restoring the harness's designed lazy-resolution contract. Also fixed two test-setup gaps (missing Principal factory rows that previously worked only because of the phantom-principal_id bug) and one test whose docstring premise was never actually true. salesagent-z9e0
…nblock mypy baseline get_adapter_class() and get_adapter_class_for_tenant() returned bare `type`, so mypy couldn't see get_supported_pricing_models() on the result at src/core/tools/products.py:721 (introduced in 21fa199). Also reformats tests/unit/test_error_envelope.py to match the pinned ruff version.
…oz coverage gap) get_adcp_capabilities already had BDD coverage proving the A2A transport boundary rejects a presented-but-invalid token; list_accounts (the other DISCOVERY_SKILLS member, same shared code path) did not. Adds the sibling scenario, and hoists invalid_token_identity()/anonymous_identity() off CapabilitiesEnv onto BaseTestEnv so AccountListEnv gets them for free instead of duplicating the construction logic.
…ction, wire_absent, field=) Foundational step for the eiww Gherkin/step-def hardening epic (salesagent-rxhy). - wire_field/wire_dict gain dotted-path resolution behind a shared _wire_body guard; wire_field keeps _require's dual assert (absent OR JSON-null both fail). - wire_absent(ctx, path): a present JSON null is NOT absent and FAILS, mirroring the old uc010 _assert_absent semantics. - wire_lookup(ctx, path)/WIRE_MISSING: non-asserting tri-state primitive for the genuinely three-valued oracles (absent-or-false outline columns, conditional Thens) so the uc010 migration need not keep a private resolver. - assert_envelope_shape gains field= (errors[0].field, protocol top level only); TransportResult.assert_wire_error forwards it — one sanctioned error surface, not a parallel wire_error_field helper (per architect review). - _wire_body restores uc010 _wire's error-first diagnostic so an errored scenario reports the error, not a misleading missing-wire message. - Meta-test pins the contract, incl. the null-rejection hard gate; verified by mutation (each semantic caught). Refs salesagent-rxhy
Delete uc010_capabilities.py's private _wire/_at/_require/_assert_absent quartet and the _MISSING sentinel; route its 35 assertion sites onto the canonical _outcome_helpers surface: _require->wire_field (x27), _assert_absent->wire_absent (x6), tri-state sites->wire_lookup/WIRE_MISSING (x2). _error_details stays (reads the error-envelope details block, which no canonical helper exposes). Preserves the null-rejection/null-is-not-absent semantics exactly and strengthens two restructured sites from sentinel comparison to real value assertions. uc010 BDD slice green and non-dormant (38 passed, 0 failed). Refs salesagent-hu31
…-wmx1)
Strengthen the flagged-WEAK generic error steps to grade the wire envelope
through the sanctioned surface, keeping the reconstructed fallback only for the
dispatch-exception path (no TransportResult captured):
- then_validation_error: primary ctx['result'].assert_wire_error('VALIDATION_ERROR').
- then_real_validation_error: wire VALIDATION_ERROR primary + explicit
isinstance(ValidationError) secondary (the type distinction has no wire form).
- then_error_field_with_value: was wire-blind; now reads errors[0] on the wire first.
Broad bdd unchanged vs baseline (1791 passed, 6023 xfailed, 25 xpassed) — additive,
no regressions.
Does NOT delete the _wire_* quartet: empirically the generic error surface serves
145 distinct codes of which 115 (235 assertions) production never emits on the
wire, and assert_wire_error hard-rejects non-canonical codes by design — so the
quartet cannot collapse onto it. _wire_suggestion/_wire_error_object are canonical
getters (delegating to extract_wire_suggestion / errors[0]), not a parallel
mechanism, and stay. Full three-layer error-code drift analysis + per-code
disposition table: .claude/research/salesagent-eiww-errorcode-reconcile.md.
Refs salesagent-wmx1
…cess sweep The salesagent-ot32 wire-access migration shortened uc002_nfr.py above two allowlisted Then steps, shifting then_payload_size_limits 240->235 and then_budget_validated_against_min_order 416->411. Update the line-keyed allowlist entries to match (net-neutral, no growth). Full saci run caught the drift; the BDD slice ot32 ran does not exercise this architecture guard.
…gent-44c8) Foundation for the error-code reconciliation epic. Ties the production wire- emission vocabulary to the same canonical AdCP source the BDD side is guarded against, closing the gap where assert_wire_error guarded only the test side. - New guard tests/unit/test_wire_standard_codes_conformance.py: every WIRE_STANDARD_CODES entry must be a canonical code in the pinned AdCP enum, and every ERROR_CODE_MAPPING target must be a standard code. - Additive pin bump 66->92: the pinned error-code fixture now carries AdCP v3.1.1's full standard enum. All 66 prior entries preserved verbatim (zero recovery-classification changes); 26 v3.1.1 codes added. This makes AUTHORIZATION_REQUIRED and PROPOSAL_NOT_FOUND canonical. - Demote NOT_SUPPORTED: a legacy SDK STANDARD_ERROR_CODES entry AdCP v3.1.1 dropped (zero production raise sites). Excluded from WIRE_STANDARD_CODES via _SPEC_DEMOTED_CODES and mapped NOT_SUPPORTED->UNSUPPORTED_FEATURE (the v3.1.1 canonical for feature-unsupported) at the transport boundary. Guard is red->green. Per-scenario Gherkin rewrites (VALIDATION_ERROR+field, etc.) stay with each use-case batch; 14 v3.1.1 codes for unbuilt features are vocabulary-only (scenarios remain xfail, not faked green).
…ns (salesagent-tmpd)
…ns (salesagent-scgh)
… (salesagent-e4ad)
…sertions (salesagent-8wuu)
…2e_rest The eiww UC-010 batches (chbi/tmpd) wired the capability-degradation scenarios (adapter-unavailable / db-fail / adapter-and-db-fail / degraded-schema-valid / targeting adapter-config partitions). Their Givens inject adapter/DB failure or a specific adapter targeting config via in-process mocks, which are invisible to the real HTTP server — so they run and assert correctly on a2a/mcp/rest but cannot set up the precondition on e2e_rest. Add the 6 exact nodeids to the e2e_rest known-failures ledger and the EXPECTED_LEDGER exact-set pin (7->13) in the same change, per the ledger discipline. Ledger state + fitness guards green (all 6 resolve to collected items).
S1.4 of salesagent-n78j0.
The delivery ACTION lived in the step layer and ran IN-PROCESS on every
transport, so the transport parametrization graded nothing: on e2e_rest the
delivery happened inside the test process against its own patched requests.post
and the live server never sent anything. That is why a whole cluster of webhook
tags sat parked in the ledger.
Now the three halves are env-owned and realized per transport: the ADDRESS on
BaseTestEnv.webhook_destination(), the ACTION on deliver_webhook(), the READ on
deliveries(at_least=) / last_delivery(). Over e2e the action drives the live
server's own admin trigger route, the key is minted INSIDE the container, and
published_jwks() / advertised_webhook_signing() read the SERVED documents rather
than rebuilding them in-process. The ctx.get("transport") branch is gone from
the step layer.
T-UC-004-webhook-9421 is removed from _UC004_E2E_WEBHOOK_INTERNAL_TAGS. The set
enters at 12 and leaves at 11 — it shrank, nothing was added, and no
e2e_unsupported() was introduced.
MUTATION PROOF, both directions, on the tree that ships:
CLEAN innet_210826_0430 543 passed / 0 FAILED / 2033 xf / 18 xp / 2 s
MUTATED innet_210826_0438 542 passed / 1 FAILED / 2033 xf / 18 xp / 2 s
delta, test-by-test: EXACTLY ONE test moved, passed -> failed:
test_rfc_9421_signed_webhook_payload_when_no_authentication_block_is_registered[e2e_rest]
Eight minutes apart, same tree. The mutation replaced _rfc9421_sender's terminal
WebhookSender(...) with _unauthenticated_sender(client): the key is still
resolved and then discarded, so posture reads and the alg/declaration check stay
intact and the ONLY thing removed is the signature. A leg that responds to a
production mutation is exercising production; that is the claim the old
in-process leg could never make.
AN EARLIER PAIR WAS DISCARDED, AND WHY MATTERS MORE THAN THE NUMBERS. Runs
innet_210826_0333/0341 measured the same delta cleanly, but uc004_delivery.py
then moved 406 lines and conftest.py 50 — both INSIDE bdd_e2e's collected
population (tox.ini:181 scopes collection to pytest tests/bdd/). So that pair
stopped grading the shipping tree and could not be cited. This is the same
distinction that made the FIRST mutation attempt worthless: it returned
IDENTICAL results with and without the signing arm, because the run never
observed the mutation at all. A number is not evidence for a claim it does not
cover.
WHAT THE WORK EXPOSED. Making the read honest turned up three scenarios that
were passing while observing nothing — retry, no-webhook-configuration and
circuit-breaker. All three were ALREADY tagged in the ledger: the parking was
honest and the harness was overriding it. xpassed went 20 -> 18 as they were
exposed and NO set grew.
Two structural fixes, each with a named mutation:
- mock["post"]'s GRADING accessors (called / call_count / call_args /
call_args_list / mock_calls / method_calls) now RAISE on e2e while the object
stays SETTABLE, so setup that configures a response still works. Installed
only by _no_in_process_webhook_socket, so _mixins.py's in-process
call_args_list read is untouched. Presence assertions already failed loudly on
e2e; ABSENCE assertions were green by construction. Same object, opposite
honesty — that asymmetry was the class.
- _await_captured_deliveries now REFUSES when the capture URL was never handed
out, instead of returning []. Absence and never-asked were indistinguishable,
so every absence assertion reading an unregistered key was green by
construction. The flag records the HANDING-OUT, never the lazily-minting
webhook_capture_key — a flag on the mint would only ever say "a key exists",
which is the same vacuity one indirection deeper. Fixed client-side: the
capture service legitimately synthesizes {received: []} for any key, because
server-side "minted but nothing delivered" and "never minted" ARE the same
state.
The no-config scenario then failed through three further layers, each a real
seam defect fixed rather than quieted: unobservable -> HTTP 404 "Media buy not
found" (the conditional guarding webhook attachment also guarded MediaBuy
seeding, so the trigger route had no buy) -> decline-treated-as-error (the admin
driver could not express "production declined": in-process a decline is False,
over e2e it RAISED). The driver is now three-way — sent -> True, "Failed to
trigger" -> False, neither flash -> RAISE. The third branch stays strict; it is
what caught the 404.
Its Thens now grade production's own verdict (_send_webhook_enhanced returns
False without a POST when no active PushNotificationConfig exists), pinned with
`is False` rather than truthiness: on the failure path webhook_result is never
assigned, so a truthiness assert would PASS when production CRASHED.
Measured, and it falsified a docstring since corrected: the admin route is NOT
the only sender — the server's own scheduler (DELIVERY_WEBHOOK_INTERVAL=5) also
delivers. That second sender cannot race these assertions precisely because they
read the returned verdict rather than receiver state; re-pointing one back to
receiver absence would reintroduce a flake with no obvious cause.
test_architecture_no_duplicate_admin_auth_block's _ALLOWED_FUNCTION moves from
_admin_post_expecting_flash to _admin_post_rendered_page. This is NOT allowlist
growth: the exemption moved DOWN a layer because the three-way driver reads a
route reporting a verdict and must match more than one flash, and
_admin_post_expecting_flash is now itself a caller of the exempt helper. Exactly
one function may POST the auth path — one name in, one name out.
Review found 19 findings across two cycles; 6 are IN, the rest are filed with
ids: salesagent-xdcf4 (unrouted call_send), gx6vf (stale ledger header), xhnz6
(~8 remaining mock["post"] readers), 3hgrx (UC-004 Whens that never call
production), jke6o. salesagent-jke6o stays blocked behind this.
THIS COMMIT INHERITS ONE DELIBERATE RED, and it is not S1.4's:
FAILED test_a_registration_carrying_webhook_authentication_is_refused_unless_signed[a2a]
That is S1.3's deliverable — SF-4 reproduced as a failing test rather than
prose. S2 resolves it by driving MUST-sign off credential LOCATIONS. Do not
xfail it, do not park a tag, do not "fix" it here. Anyone running the suite on
this commit should expect exactly that one failure and no other.
src/ is untouched: this is a harness change.
Refs: salesagent-n78j0.1.4
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ontrols
S1.5 of salesagent-n78j0. The title said three guards. Two shipped, one was
deliberately dropped, and the drop is the part a reviewer cannot reconstruct
from the diff.
THE DEFECT was never "transport branching" — it is a transport lookup whose
None case is not handled by raising. uc003_update_media_buy.py:1161 is
LEGITIMATELY transport-aware and documents why (on A2A the transport clears
artifacts, so the wire-dict checks are vacuous there); banning the branch would
have deleted a real assertion. Three further sites use .get() correctly, because
they raise on None with a message naming the fix — a blanket ban would have
traded a good diagnostic for a bare KeyError at three places to catch two.
Fixed, both measured: uc003_update_media_buy.py:1161, and then_payload._is_e2e,
where an unset transport yielded False so callers silently took the non-e2e
branch and skipped the e2e assertions entirely (:59, :137, :141). A step that
grades nothing when its setup is missing is the vacuity this epic exists to
remove.
GUARD 1 WAS DROPPED, DELIBERATELY. It could not be written without creating an
allowlist: a third silent site (_outcome_helpers.py:223 is_e2e, ~25 callers
across UC-002/003/006) is too large for this atom, and exempting it would have
shipped a new allowlist — zero entries at HEAD, one on landing, which the
charter forbids outright. The two fixes it would have guarded shipped anyway,
and the third site is filed as its own task carrying a FIXME that states in
terms it is "not suppressed by any allowlist". A dropped guard with its reason
recorded is a stronger artefact than a guard with an exemption row: the row
outlives the reason, as every row in this repo has.
GUARD 2 — Check C now matches the attribute form. uc010_capabilities.py had
moved from ctx.get("wire_error_envelope") to result.wire_error_envelope and kept
the hand-rolled parsing, so the edit moved the violation PAST the guard rather
than fixing it. The new matcher is SEPARATE (_is_wire_envelope_attribute) and
wired into the finder ONLY. Extending _is_ctx_wire_envelope_get would have been
the obvious move and is wrong: it is consumed twice, by the finder AND by the
exemption calculator, so widening it matches the new shape and immediately
EXCUSES it. That was measured, not theorised — a violation injected into the
shared matcher left 9 tests passing.
GUARD 3 — the xfail ledger is frozen as set equality, reusing the mechanism
test_ruff_egress_bans already invented, in the existing
test_e2e_rest_ledger_state rather than a new file. It fails in BOTH directions:
a one-direction pin catches growth but silently permits an UN-GRADED REMOVAL,
which is the failure mode S1.4 nearly shipped. Its extractor rejects a computed
set and a missing binding — the two ways a set-equality pin goes vacuous.
BOTH SHIPPED GUARDS CARRY PERMANENT POSITIVE CONTROLS, not a mutation run once.
test_positive_attribute_form_envelope_parsing_is_detected parses a real
violation and asserts the detector returns True, so the guard cannot silently
stop detecting. test_check_c_matcher_cannot_see_the_attribute_form makes
RE-MERGING the two matchers a test failure, so the exemption-calculator trap
cannot be re-entered by a future edit. A guard verified once proves it worked
at one moment; a permanent control proves it still works.
The uc002_nfr.py entries in test_architecture_bdd_no_request_in_then were
RE-PINNED, not appended: four entries out, four in, count unchanged. Every line
was off by exactly one because uc002_nfr.py gained an import while replacing
hand-rolled envelope walking with the sanctioned helper — it adds no dispatch.
The line-keyed allowlist is fragile by construction (any edit above a violation
looks like a new offence, and the guard's own message invites appending), so
that fragility is filed as salesagent-y4g7e rather than fixed here; it wants
keying on (file, function) like the guards above.
Unit suite on the full tree: 6728 passed, 0 failed — one more than the 6727
baseline, the guards' own tests netting in with nothing broken.
src/ is untouched. No new guard file. No allowlist created or grown.
THIS COMMIT INHERITS ONE DELIBERATE RED, unchanged and not S1.5's:
FAILED test_a_registration_carrying_webhook_authentication_is_refused_unless_signed[a2a]
S1.3's deliverable — SF-4 as a failing test rather than prose. S2 resolves it.
Refs: salesagent-n78j0.1.5
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
S1.3b of salesagent-n78j0 (salesagent-jj90f).
operations.py:296 `return False` is UNCONDITIONAL for every JSON-RPC method
outside {tools/call, message/send, message/stream}. That is not one missing
enumeration entry — it is a DEFAULT-ACCEPT for the entire rest of the surface.
tasks/pushNotificationConfig/set|create is served by OUR handler
(AdCPRequestHandler.on_create_task_push_notification_config,
adcp_a2a_server.py:1279 — qualname confirms ours, not the SDK base), carries an
`authentication` field, persists a config id, and demands a bearer token but
NEVER a signature. So a buyer registers webhook credentials over A2A, unsigned,
and the seller accepts — the same spec-MUST violation as SF-4
(security.mdx @ v3.1.1 :1462-1465), through a second dedicated method.
S1.3's scenario is correctly transport-blind, and the ENV chooses placement, so
its a2a realization put the credential at
params.configuration.task_push_notification_config on message/send. The
tasks/pushNotificationConfig/* family was exercised by no scenario at all. The
scenario's TEXT already claimed more than it graded: "a registration carrying
webhook authentication is refused unless signed" does not say "via
message/send", and this system accepts such registrations in two places.
SHAPE: one leg exercising BOTH locations, the failure naming WHICH was accepted.
This departs from "one request, one outcome" — two dispatches inside one When —
and the step's docstring says so, with the trigger recorded: if a THIRD location
appears, switch to env-level parametrisation rather than stacking dispatches.
NOT a placement MOVE. Relocating the credential from location 1 to location 2
would have yielded a red either way while trading bypass 1's coverage for
bypass 2's — net zero, and it would silently un-grade the bypass S1.3 was
written to catch. Both locations are graded; neither was traded.
The feature file is unchanged and must never learn that A2A has two credential
locations. Placement is the env's business — that is the charter clause about
nothing under tests/bdd/steps/ learning a transport exists.
EVIDENCE — run 4411cd78 -> test-results/210826_0544, created 07:45:36:
9 collected / 8 passed / 1 FAILED
FAILED test_a_registration_carrying_webhook_authentication_is_refused_unless_signed[a2a]
"AT the tasks/pushNotificationConfig/set params: ... the request was ACCEPTED
(is_error=False, payload=TaskPushNotificationConfig(push_notification_config=
PushNotificationConfig(authentication=..., id='pnc_54982f73ef4a44f4', ...),
task_id='task_30e0e4bae762'))"
THE COUNTS ARE UNCHANGED AND THAT IS EXPECTED — 8/1 of 9 before and after. This
is ONE leg that was ALREADY red on location 1; adding location 2 to the same leg
cannot move a total. Do not read the identical numbers as "nothing happened":
the change is inside the message, which now carries a second failure block.
NO MUTATION PROOF, deliberately, and the reason is not expedience. Mutation is
what distinguishes a green from a VACUOUS green. This deliverable is a RED whose
MESSAGE is the evidence, and that message contains server-generated identifiers
— pnc_54982f73ef4a44f4 and task_30e0e4bae762 — which exist ONLY because the
dispatch really reached the server and it really persisted the registration. The
harness cannot fabricate them. A mutation would ask "if I remove location 2 from
the env, does the message stop naming it?", which tests plumbing rather than the
claim. The real discriminator lives at S2's gate: BOTH reds must flip, and a fix
that closes one location and not the other leaves the leg red with the message
naming which.
src/ untouched; ledger and allowlists byte-identical; no new files.
THIS COMMIT LEAVES TWO DELIBERATE REDS IN ONE LEG, both S2's to resolve:
message/send params.configuration.task_push_notification_config (S1.3)
tasks/pushNotificationConfig/set params (this commit)
A correct S2 drives MUST-sign off credential LOCATIONS and closes both. One that
merely enumerates method families would close one and re-open the hole on the
next SDK family — which is exactly what the second red exists to detect. Do not
xfail either, do not park a tag.
Refs: salesagent-jj90f
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…dinality
S2 Fault C of salesagent-n78j0. The one live production risk in this epic;
everything else it has fixed was test quality.
`operation` labels all three signature counters (metrics.py:188/:194/:200) and
was fed VERBATIM from the request body — params.name (operations.py:322) or
data.skill (:282) — off an UNAUTHENTICATED request. UnifiedAuthMiddleware
rejects nothing before the verifier, so an anonymous
POST {"method":"tools/call","params":{"name":"<arbitrary>"}} to /mcp minted one
new Prometheus series per distinct value in a long-running multi-tenant process.
No credential required. All three recorders AND their caller are new in this PR,
so this is a cardinality bomb the PR introduced, not inherited debt.
sanitize_operation() lives beside sanitize_signature_code and is called INSIDE
all three recorders, never at a call site. That is load-bearing rather than
stylistic: there are TWO record_request_unsigned call sites —
request_verifier_middleware.py:352 ("ignored") and :449 ("absent") — so
sanitising at the site the report named would have left the other minting
series. A sanitiser you must remember to call is the same shape as an assertion
you must remember is vacuous.
THE CLOSED SET IS DERIVED FROM PRODUCTION, NEVER HAND-LISTED.
operations.py exports sdk_operation_names() and resolved_operation_names(); the
guard's _sdk_operation_names() now DELEGATES to production and holds no copy.
Hand-listing it, or letting the guard grade its own copy, would have rebuilt
Fault B inside Fault C's fix — Fault B is literally "the guard grades its own
copy", and it is still open two commits away.
THE FOURTH REGISTRY IS THE FIND, and it was not in the brief. The instruction
named three sources: ADCP_TOOL_DEFINITIONS, the _register_tool list, and the
/api/v1 route table. The real union needs a fourth — SKILL_HANDLERS. Without it
approve_creative, create_creative, assign_creative, get_media_buy_status and
optimize_media_buy exist in no other registry and would have collapsed to
"other": the sanitiser would have bounded cardinality CORRECTLY and silently
destroyed the metric for an entire transport, with every test passing. That is
this epic's own disease one more time — a green that means less than it appears
— caught by checking what the closed set had to contain rather than trusting the
list handed over. MCP_TOOL_NAMES is now accumulated BY _register_tool rather
than re-listed beside it, so the registry cannot drift from its own source.
MUTATION, run and reverted — removing sanitize_operation from ONE recorder
reddens BOTH surfaces, which is the point: the behavioural tests must fire, not
only the structural guard. If only the guard had fired, the integration tests
would have been grading the guard's opinion instead of the behaviour.
FAILED ...records_other
FAILED ...ignored_posture...
FAILED test_every_sanitize_operation_call_in_src_is_inside_a_recording_helper
AssertionError: adcp_request_unsigned_total carries the caller's own string as
its 'operation' label:
[(('operation', 'attacker-minted-series-ea9daefca8fe4050a9b599f0b050b8ce'),
('reason', 'absent'))]
plus its reason='ignored' twin — one removal, both call sites. Reverted, 44 pass.
Also here: metrics.py's inline reason ternaries are promoted to named
sanitize_unsigned_reason / sanitize_revocation_unavailable_reason, all five
sanitisers share one _bounded() body, and the module docstring's
"cardinality is deliberately bounded" list finally names `operation` — the one
label it never mentioned, in the module where this PR added four of its bullets.
tests/helpers/signing.py gains seed_principal: NEW shared infrastructure C's own
integration tests need, since the four test_request_signature_* modules address
SIGNING_TENANT_ID / SIGNING_PRINCIPAL_ID directly and no env creates those rows.
It is deliberately distinct from signing_capability.attach_agent_url and its
docstring says why collapsing them would be wrong: that one is for an env that
SIGNS and reuses the env's own tenant/principal so the signer's identity is the
env's (owner decision D3); this one seeds the shared unsigned-suite rows.
Measured, not assumed: duplication is unmoved with it applied.
Full six-suite run innet_210826_0654 — 12648 passed, 1 failed, the single
failure being the deliberate a2a red S2/A removes. unit 6913, integration 2668,
bdd_inprocess 2268, bdd_e2e 543, admin 106, e2e 145, ui 5.
Gate: no TEMPORARY MUTATION tree-wide; no type: ignore[unreachable]; ledgers and
allowlists untouched; git diff src/ exactly these four files. Duplication reads
tests/: 73 against a baseline of 72 — that breach is PRE-EXISTING, bisected to
d4f7ffc (S1.5): 72 at abd6924, 72 at 7efc416, 73 at d4f7ffc. Measured
both stashed and applied, this commit does not move it. It is fixed forward in
its own commit; the baseline is NOT bumped.
Refs: salesagent-n78j0.2
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The DRY ratchet was breached in the tree and had been for three commits. This fixes it at the fault and lowers the baseline to the measured reality. THE ARC, so nobody has to reconstruct it: .duplication-baseline said tests: 72 d4f7ffc (S1.5) pushed it to 73 after this extraction: 68, measured baseline lowered 72 -> 68 to match BISECTED, not guessed — four points in isolated `git archive` trees: abd6924 72 7efc416 72 d4f7ffc 73 <- enters here 9f01c5d 73 +S2/C 73 <- C adds nothing So S1.4, S1.3b and S2/C are all exonerated. Two independent bisects agreed. MY HYPOTHESIS WAS WRONG AND THE REAL ANSWER IS BETTER. The prediction was that S1.5's two NEW AST guard files resembled each other. They do not. The block was shared by FIVE PRE-EXISTING BDD guards — no_dict_registry, no_duplicate_steps, no_pass_steps, no_silent_env, no_trivial_assertions — each carrying its own copy of the same scan: walk tests/bdd/steps, skip underscore files, parse, find step-decorated functions, apply a predicate, format violations. That reframes S1.5's fault. It did not write a fresh duplicate block; it EXTENDED test_architecture_bdd_no_silent_env.py — one of the five — with enough shared shape to push an already-borderline cluster over R0801's threshold. Which is why nobody noticed: every guard looked reasonable on its own, and the cluster only became visible when one more member joined it. The fix extracts the scan into tests/unit/_bdd_guard_helpers.py and has all five import it. Net -62 lines. WHY THE BASELINE IS LOWERED RATHER THAN LEFT AT 72. Raising a baseline hides a violation that exists; lowering one locks in an improvement already measured. They edit the same file and are opposite acts. The charter forbids a ratchet LARGER than it started — 68 < 72 — and leaving it at 72 would ship a floor four above reality, which floors nothing: the next four duplicate blocks would land with the guard still green. The number here is derived from a measurement, which is the difference between this edit and the kind that quietly moves a ratchet. THE GUARDS CAN STILL FAIL, and that was checked rather than assumed. Twelve passing tests prove they RUN, not that they DETECT — an extraction that swallowed detection would look identical. So the violation was injected into the guard most at risk (no_silent_env, the one S1.5 modified): adding `ctx.get("env")` to a step function turned it RED, naming the exact site — FAILED TestBddNoCtxGetEnv::test_no_new_ctx_get_env assert not [('bdd/steps/generic/then_error.py', 'then_error_references_auth')] "The harness env is guaranteed by the autouse fixture. Use ctx["env"]." Reverted; 4 passed, no residue. Measured after, all three scopes unchanged against the new baseline: src/: 35 tests/: 68 scripts/: 0 Fixed forward. d4f7ffc is not rewritten — the honest record of a ratchet breached, detected by measurement, attributed by bisect and fixed at the fault is worth more than a history that looks clean. The duplication check is now part of the standing pre-commit gate, beside the mutation grep. It slipped past twice tonight because the gate enumerated the failures that had already burned us rather than the invariants the charter names — a gate assembled from incidents is always one incident behind. src/ untouched. One deliberate red remains in the tree, unchanged and not this commit's: the a2a leg S2/A resolves. Refs: salesagent-n78j0.1.5 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> ONE OF THE FIVE IS A DEAD GUARD — pre-existing, found while verifying this commit, and NOT caused by the extraction. test_architecture_bdd_no_trivial_assertions cannot fire on anything. Injecting the exact shape its own docstring names — @then("a deliberately vacuous injected probe step") def then_injected_vacuous_probe(ctx: dict) -> None: assert ctx["result"] — reports 0 violations. Mechanism, isolated rather than guessed: iter_bdd_steps DOES see it (758 then-steps scanned) and _assert_is_meaningful correctly returns False on a synthetic parse of the body, but _has_meaningful_assertion_or_delegation falls through to iter_call_expressions(func), which walks func.decorator_list — and the `then("...")` decorator is itself a Call matching none of the exclusions, so it counts as "delegates to a helper". Every @then step is unconditionally meaningful; all 758 are exempt by construction. Verified pre-existing against HEAD: HEAD walks the tree and passes the full FunctionDef WITH decorators to the same predicate, exactly as this commit does via step.node. The extraction preserved the behaviour precisely — it neither caused nor worsened it. So the injection evidence above establishes that no_silent_env still detects. It does NOT establish that all five do, and one provably does not. Filed as salesagent-v6z5c (P1) with the probe as its acceptance test. Deliberately not fixed here: the fix will surface real violations among those 758 steps, which is its own atom with its own evidence, and absorbing it would turn a -62-line extraction into an open-ended change.
…n copy
S2 Fault B of salesagent-n78j0.
ADCP_SURFACE_PREFIXES was defined once, but the SEGMENT-BOUNDARY PREDICATE that
gives it meaning existed four times: production's _is_adcp_surface, operations'
_on_surface (plus the prefixes re-literalled beside it), and the structural
guard at two sites — one a helper, one inlined.
The guard IMPORTED THE CONSTANT AND COPIED THE LOGIC. That half-coupling is what
made it deceptive: a reader checking "does the guard use the real prefixes?"
gets a satisfying yes, while the behaviour under test lives in the predicate the
guard re-implements. So production's boundary could be rewritten and the guard
would keep grading its own intact copy.
Now is_adcp_surface(path) is exported once and every consumer imports it. The
single boundary rule (path == prefix or path.startswith(f"{prefix}/")) lives in
operations.py with /mcpx and /api/v1x named, and _is_adcp_surface's docstring
records that it holds no boundary rule of its own ON PURPOSE — "a second copy
here is what let the segment boundary be rewritten while the structural guard
graded its own intact copy".
THE MUTATION IS THE SPECIFICATION, and both halves were measured on the same
tree. Rewrite _is_adcp_surface to a bare startswith, dropping the boundary:
BEFORE B: all four TestAllowlistTiedToRouteTable tests stay GREEN.
That green WAS the bug — the guard could not fail.
AFTER B: 6 FAILED / 5 passed —
test_every_allowlisted_prefix_matches_a_real_route
test_the_three_adcp_surfaces_are_allowlisted
test_the_allowlist_stops_at_the_segment_boundary[segment]
test_the_allowlist_stops_at_the_segment_boundary[hyphen]
test_the_allowlist_stops_at_the_segment_boundary[dot]
Six failures across three boundary variants — the guard now fails in more ways
than the specification asked for, which is what happens when it stops grading a
copy. Mutation reverted; 11 pass clean on the full file.
A NOTE ON HOW THAT WAS NEARLY MIS-GRADED, because the lesson generalises. A
first reading used `pytest -k "AllowlistTiedToRouteTable"` and got
"7 passed, 4 deselected" — one step from concluding the mutation had not fired.
The -k expression selected the class and DESELECTED its parametrised boundary
cases, which are the entire point. A mutation proof is a claim about a CORPUS,
so any flag that changes the corpus invalidates it: -k, --deselect, -x, or a
path narrower than the guard's own scope. Both halves must run the same corpus
or they are not comparable. Run the file.
This is the third distinct mechanism of guard-that-cannot-fail found in this
epic: a matcher that was also its own exemption calculator (S1.2), a
meaningfulness predicate satisfied by the @then decorator itself
(salesagent-v6z5c, filed), and this one grading a private copy of the rule.
Three routes, one outcome — a green that certifies nothing.
Gate, all five: no TEMPORARY MUTATION tree-wide; no type: ignore[unreachable];
duplication 35 / 68 / 0 all "unchanged" against the baseline lowered in
6619859; ledgers and allowlists untouched; git diff src/ exactly these three
production files.
One deliberate red remains and is not this commit's: the a2a leg, which S2/A
resolves by driving MUST-sign off credential LOCATIONS.
Refs: salesagent-n78j0.2
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… list
S2 Fault A of salesagent-n78j0 — SF-4, the A2A credential-location bypass.
THE ESCALATION NO LONGER TAKES ``method`` AT ALL. That is what makes this
structural rather than a longer list, and it is the sentence to read first:
-def _jsonrpc_payload_forces_signature(method: str, params: Mapping) -> bool:
+def _jsonrpc_payload_forces_signature(params: Mapping) -> bool:
The old body enumerated each transport's TOOL-ARGUMENT envelope — ``params.
arguments`` for ``tools/call``, ``data.input`` / ``data.parameters`` for
``message/send`` — and then ``return False``. That final answer was never "this
method carries no credentials". It was a DEFAULT-ACCEPT over the entire rest of
the JSON-RPC surface. ``tasks/pushNotificationConfig/set`` registers a webhook
AND its credentials with no skill invocation anywhere in sight, is answered by
``on_create_task_push_notification_config`` which PERSISTS them, and inherited
that bypass — as would every method the next SDK release adds.
A method list has a default arm. A default arm on this question is a hole, and
no amount of extending the list removes it. So the question is asked on the
other axis: WHERE can this envelope carry credentials — enumerated as LOCATIONS
(``_CONFIG_LOCATIONS``, ``_application_payloads``) and run against every body
whatever its method. ``src/services/protocol_webhook_service.py`` :7-13 already
wrote that enumeration down, by configuration CHANNEL rather than by method.
Both wire spellings are DERIVED, not typed twice: ``/a2a`` is built with
``enable_v0_3_compat=True``, so ``pushNotificationConfig`` and
``push_notification_config`` are the same location, and a typo in a hand-written
camelCase spelling would fail silently — as an accepted credential registration.
SECOND FILE, AND IT IS PART OF THE FAULT — NOT SCOPE CREEP. The middleware
carried its OWN INLINED COPY of the escalation condition:
- if resolved.signature_forced and context.bucket != "none":
+ if _credentials_force_a_signature(context.posture, resolved):
That ``and context.bucket != "none"`` was a SECOND COPY of the rule — the same
"predicate written in more than one place" shape as Fault B, one module over.
Fixing the axis in ``operations.py`` while leaving that copy behind would have
been site-patching, which is the thing this epic forbids.
The escalation is also bounded by ``posture.supported``
(security.mdx @ v3.1.1 :1465) instead of a per-OPERATION ``bucket != "none"``:
``bucket_for`` grades a PROTOCOL-namespace method against the
``protocol_methods_*`` trio, which defaults to empty, so every JSON-RPC method
on a signing-capable seller landed in ``none`` — the bucket was never the line
:1465 draws. And it is promoted at RESOLUTION time, not on one handler branch,
because a request whose bucket stayed ``none`` is waved through unverified: an
escalation enforced only on the unsigned branch would refuse the honest unsigned
registration and ADMIT the same registration carrying a junk ``Signature``.
THE TWO e2e TESTS TELL THE WHOLE ARC, and it is the epic's headline property:
401 the escalation correctly REFUSING an unsigned registration that
carries webhook credentials (:1462-1465). Proof it now fires.
IntegrityError test isolation — a second tenant claiming the shared constant
``SIGNING_AGENT_HOST``. Proof the request now gets far enough to
provision a counterparty, which it never did while refused.
PASSED signed, against a resolvable counterparty, CREDENTIALS INTACT.
That last state is SF-4 stated POSITIVELY: registering webhook credentials over
A2A WORKS when signed. The first instinct was to strip the ``authentication``
block from both tests — three sibling tests already register without one, so it
matched the local convention. The owner rejected it, and was right: those two
were the ONLY e2e tests putting a CREDENTIALED push_notification_config through
A2A. Stripping them would have shipped a rewritten escalation with zero live
coverage of the path it governs, and the convention is exactly what would have
made the loss invisible. An uncredentialed webhook receiver is also not a shape
production should produce. The 401 was never about the CALLER's identity — the
request still carries its own ``Authorization: Bearer``; it is the spec refusing
a SECRET over an unsigned channel.
The isolation fix sets a UNIQUE dotted ``virtual_host`` rather than touching
``SIGNING_AGENT_HOST`` or ``ensure_declarable_identity_host``: that constant is
shared with the in-process legs (``tests/helpers/signing.py`` :944,
``test_harness_signed_dispatch.py`` :350) and is paired there with a fixed
``SIGNING_TENANT_ID``. It reuses the harness's own ``unique_run_id()`` and the
harness's own "has a dot" predicate, so the shared assignment path never runs
and the signer's identity stays the env's own (owner decision D3).
EVIDENCE, per suite, and NOT as one whole-matrix green:
innet_210826_1046 — e2e 145 passed / 0 failed / 24 skipped / 3 xfailed.
The ONLY suite whose tree changed after 0958. 143 -> 145: the two tests
above now pass and nothing else moved.
innet_210826_0958 — unit 6916/0, integration 2668/0, bdd_inprocess 2269/0,
bdd_e2e 543/0, admin 106/0, ui 5/0. Authoritative for these six, whose tree
did NOT change since. It PREDATES the isolation fix and is NOT authoritative
for e2e, where it read 143/2.
The ``[a2a]`` leg that S1.3 made red and S1.3b doubled is GREEN in bdd_inprocess.
Gate, all five: no TEMPORARY MUTATION tree-wide; no ``type: ignore[unreachable]``
in tests/; duplication 35 / 68 / 0, all three "unchanged"; no ledger or allowlist
touched. The fifth reads TWO files under src/, not one — ``operations.py`` and
``request_verifier_middleware.py`` — for the reason given above; the second is
where the bucket bound and the promotion point live.
Refs: salesagent-n78j0.2
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…cycles gone
S3 of salesagent-n78j0 (salesagent-n78j0.3). Dissolves SF-8/10/11.
THE CAUSE WAS THE LEAF'S LOCATION, NOT THE IMPORT SHAPE. Python executes a
package's ``__init__`` before any of its submodules, so while the value-sets sat
at ``src.core.signing.algorithms``, merely importing one ran the whole facade —
which imports ``keys`` -> ORM, ``request_verifier_middleware`` -> metrics, and
``replay_store``/``revocation`` -> config. Every one of those is a module that
needs a value FROM the leaf. Re-pointing a caller at the submodule could never
have helped; the bead's step 1 as written ("import the leaf directly") is
provably insufficient, and this is why.
THREE CYCLES, WHERE THE COMMENT DOCUMENTED ONE. ``_LAZY_EXPORTS`` (PEP 562)
deferred them to first attribute access, which is why only one was known:
1. database.models <-> facade (documented)
2. src.core.config <-> facade (UNDOCUMENTED — found only once the deferral
came off. ``CACHE_MAX_AGE_SECONDS`` is a dependency-free int that lived in
trust_root.py, which did not even use it; config derives grace_seconds from
it and three signing modules import config back.)
3. src.core.metrics <-> facade (documented as the reason metrics was lazy)
All three are removed at the cause. ``_LAZY_EXPORTS`` and its ``__getattr__``
are DELETED — all 13 entries now eager and typed. A lazy export moved a STATIC
layering fault to first attribute access where nothing checks it: the illegal
edge was invisible to the import graph AND to mypy, which left the layer's most
security-relevant exports untyped at every call site.
WHAT MOVED. ``src/core/signing_contract/`` is the layer's dependency-free leaf:
algorithms, canonical, ``_upstream/`` (vendored), and the operation vocabulary.
559 deleted lines are RELOCATED, not dropped (575 total in the new package).
Its ``__init__`` states the rule it must keep: nothing in there imports
``src.core.signing``, ``.config``, ``.metrics`` or ``.database`` AT MODULE
LEVEL. The vocabulary reads the transport registries through function-local
imports at request time, which is what keeps the graph acyclic while still
DERIVING the bounded label set from the registries that serve traffic.
EXPORTS OPERATIONS, NOT PRIMITIVES PLUS PROSE. ``signing_alg_check_clause()``
and ``signing_purpose_check_clause()`` return whole CHECK bodies;
``models.py`` takes the clause instead of composing it from
``sql_value_list(SIGNING_ALG_VALUES)``, and ``sql_value_list`` is off the
facade. Choosing the column name, the operator and the rendering is no longer a
caller's job — that freedom is what let the ORM constraint and the migration's
be assembled independently.
THE MIGRATION IS BYTE-IDENTICAL, AND THE SHIM IS WHY.
``alembic/versions/e7a2c40b91d5_add_signing_keys_table.py`` imports the old
path. CLAUDE.md says "Never modify existing migrations after commit!", so the
rule is respected ABSOLUTELY rather than reinterpreted: ``git diff alembic/`` is
empty, and ``src/core/signing/algorithms.py`` remains as a FROZEN FORWARDING
SHIM — 41 lines, four re-exports, ZERO logic constructs. It duplicates a NAME,
not a RULE; the moment it computed anything it would be a second definition site
and the fault would be back.
RELATED DEFECT, FILED NOT FIXED (salesagent-89p27): that migration interpolates
LIVE constants into the DDL it emits, so two databases at the same alembic
revision can carry different CHECK constraints with nothing recording the
divergence. Same family as this epic's other faults, one axis over: one rule,
two TIMES rather than two PLACES. Freezing it needs a NEW migration pinning the
values as literals — its own atom, with its own evidence.
GUARD EDITS TRACK MOVED PATHS AND GRANT NO NEW PERMISSION.
* signing_layer_boundary: LAYER_PREFIXES is now two directories, ONE layer.
Both allowlists remain EMPTY.
* no_dark_signing_primitives: SCOPE repointed at the leaf. Scanning the shim
instead would have kept the guard GREEN while it graded nothing — the same
shape as a guard grading its own copy of a rule.
* vendored_provenance: path follows ``_upstream/``; the vendored BYTES are
unchanged (``_upstream/canonical.py`` shows a 0-line diff).
* no_fabricated_example_domain: the SAME THREE entries re-pinned -1 line each
(models.py's import shrank). None added, none fixed.
AND ONE THE GUARDS CAUGHT ON ME: ``pyproject.toml``'s ruff FORMAT exclusion was
scoped to the old ``_upstream`` path, so moving the package silently took the
vendored copies out of its protection and ``ruff format`` reformatted three
verbatim upstream units. ``test_signing_vendored_provenance`` failed on the
hashes, the bytes were restored from HEAD, and the exclusion now follows the
package. A path-scoped exemption is only as good as the path.
EVIDENCE: innet_210826_1308, exit=0, ONE run over ONE stable tree (diff
identical across a 70s gap before launch), so no per-suite attribution is needed:
unit 6916/0 integration 2668/0 bdd_inprocess 2269/0 bdd_e2e 543/0
e2e 145/0 admin 106/0 ui 5/0
An earlier matrix (innet_210826_1234) failed integration on
``test_grace_is_twice_the_published_cache_max_age`` — a REAL defect this change
introduced by relocating ``CACHE_MAX_AGE_SECONDS`` out of trust_root. The test
now takes it from the facade. Integration 2667 -> 2668 is that fix landing; a
local unit run (6738, green both before and after) could never have seen it.
Gate, all five: no TEMPORARY MUTATION tree-wide; no ``type: ignore[unreachable]``
in tests/; duplication 35 / 68 / 0 with ALL THREE scopes printing "(unchanged)";
ledgers and allowlists unchanged or smaller (e2e ledger 17 -> 17, boundary
allowlists still empty, baseline untouched); ``git diff alembic/`` empty.
Refs: salesagent-n78j0.3, salesagent-20rv3, salesagent-89p27
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
One blank line, added by ``ruff format``. 2a61df0 edited this file's header to make LAYER_PREFIXES two directories and ran ``ruff check`` on it but not ``ruff format`` — so the repo-wide format check failed in CI's Quality Gate while every test suite was green. A FIXUP, not an amend. 2a61df0 is already on the PR and has been graded by CI; rewriting a pushed commit to absorb a whitespace change is the same "rewrite history for cosmetics" trade declined for that commit's own message correction (recorded on salesagent-n78j0.2 instead). THE GATE IS SIX CHECKS, NOT FIVE. The five structural checks plus seven green test suites are not the same claim as "the gate CI enforces is green": ``make quality`` runs ruff format, ruff check, mypy AND unit, and it is the authoritative one for lint/type/unit. Verifying suites and skipping the linter, then reporting the gate passed, is a population-vs-claim error in the gate's own definition. Recorded on the epic bead so S4 and S5 inherit it. Verified after the fix: ruff format --check 1459/1459 already formatted; ruff check "All checks passed!"; mypy "Success: no issues found in 322 source files"; unit 6738 passed / 10 skipped / 26 xfailed. Refs: salesagent-n78j0.3 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ical_origin() replaces .session
S4 bullet 1, first half (salesagent-n78j0.4). Lands on its own because it is complete
and independently true; the remaining S4 steps do not gate it.
``SigningKeyRepository.session`` handed out the raw SQLAlchemy session and asked
callers, in fourteen lines of docstring, to use it only for a sibling repository and to
remember that "the no-raw-select guards still apply to whoever borrows it". That is a
PRIMITIVE PLUS PROSE, and the prose was the only thing holding the invariant up.
THREE CALL SITES, ONE OPERATION, WRITTEN OUT THREE TIMES. Each took the borrowed session,
built a TenantConfigRepository on it, read the tenant, and derived the origin —
identically, differing only in how each handled ``None``:
src/core/signing/webhook_sender_factory.py:374 -> repo.canonical_origin()
src/core/helpers/adapter_helpers.py:118 -> repo.canonical_origin()
tests/harness/_mixins.py:1003 -> repo.canonical_origin()
Not one of them wanted a session, and not one wanted a tenant row. All three wanted the
ORIGIN. So the missing thing was never an escape hatch — it was a method.
``canonical_origin()`` does the read on ``self._session`` and returns the canonical agent
URL, ``None`` when the row is absent, which is what all three did by hand. ``.session``
is now DELETED: ``grep -rn "repo\.session"`` over src/ and tests/ returns nothing, so
there is no hatch left to borrow and nothing to route around.
THE INVARIANT IS NOW ENFORCED BY CONSTRUCTION, NOT REQUESTED. The deleted docstring
warned that resolving the origin in a second session "could observe a rotation from one
side and the host from the other". Nothing stopped that, and nothing GRADED it — the
claim had been unenforced prose since prebid#1291. A caller can no longer open a second session
because it never receives the first one.
AND IT IS GRADED, BY VISIBILITY RATHER THAN BY STRUCTURE.
``TestCanonicalOriginReadsTheRepositorysOwnTransaction`` commits a tenant, then mutates
``virtual_host`` and FLUSHES without committing, so the UPDATE exists in this transaction
and nowhere else, then asserts ``canonical_origin()`` returns the new value. Reading the
uncommitted write IS the proof the read happened in that transaction. Nothing inspects
the session, counts ``get_db_session`` calls, or scans source — so unlike a lexical
enclosure scan it cannot be defeated by extracting a helper: the assertion does not care
how many call frames deep the read is.
MUTATION, both directions, whole file (never ``-k``):
shipping tree 11 passed
canonical_origin opens its own session 1 failed, 10 passed
- https://origin-after.example.com
+ https://origin-before.example.com
reverted 11 passed
The failure names the real cause — committed state read from a second transaction — and
the other ten stay green, so the test is specific to the invariant rather than to the
mutation's blast radius.
A NOTE ON WHAT THE DUPLICATION RATCHET DID NOT SAY. Collapsing three identical blocks
into one did NOT move it: 35 / 68 / 0, "(unchanged)", before and after. Each block was
two lines, under pylint R0801's min-similarity-lines, so the ratchet never counted them.
The ratchet is a FLOOR, not a duplication detector: "unchanged" proves the floor was not
breached, never that nothing was duplicated.
Gate, all six: no TEMPORARY MUTATION tree-wide; no ``type: ignore[unreachable]`` in
tests/; duplication 35 / 68 / 0 with all three scopes printing "(unchanged)"; no
allowlist, ledger or baseline file touched (e2e ledger 17); ``git diff alembic/`` empty;
``make quality`` clean (ruff format, ruff check, mypy, unit 6738 passed).
Refs: salesagent-n78j0.4
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…sion, no ORM on the queue
S4 bullet 1, second half, plus its guards (salesagent-n78j0.4). One fault — a POOLED DB
CONNECTION HELD ACROSS ``time.sleep`` AND A POST TO A BUYER-SUPPLIED URL — closed from
both ends, so a hanging receiver can no longer consume a database connection.
TWO NON-PRIMITIVES RODE ON THE QUEUE ENTRY, not one:
webhook_data = {
"config": config, # a live PushNotificationConfig ORM row
"signing_repo": signing_repo,# a repository built on the CALLER's open session
...
}
``signing_repo`` was the connection itself. ``config`` was an ORM instance whose
lazy-loads need a session that may be closed by the time a retry touches it. Production's
own comment said the repository "rides on the entry" for the same reason ``tenant_id``
does — but ``tenant_id`` is a ``str`` and costs nothing, while a repository carries a
session. That comment was the closest thing to a stated justification for the fault, and
it is rewritten here rather than left to explain something that no longer happens.
(i) NO DONATED SESSION. ``repo=`` is gone from ``deliver_adcp_webhook``,
``deliver_adcp_webhook_sync``, ``adcp_webhook_sender`` and ``_signing_repo``.
``_signing_repo(tenant_id)`` always opens its OWN short session, which lives for the key
read and closes with the block. A caller cannot donate a lifetime it has no way to pass —
the defect is UNREPRESENTABLE at that boundary, not merely discouraged.
(ii) NO ORM ON THE QUEUE. ``QueuedWebhook`` is a frozen, slotted dataclass of primitives.
A loop whose only input is primitives cannot lazy-load, cannot touch a session and cannot
hold a connection across a sleep — AT ANY CALL DEPTH, which is the property that matters
here and the one a lexical scan could not express (see below).
``payload`` is ``dict[str, JsonValue]``, NOT ``dict[str, Any]``. ``Any`` is the ABSENCE
of a type rather than an ORM type, and admitting it left exactly the hole this change
closes: an ``Any`` field can be handed an ORM row at runtime. ``pydantic.JsonValue`` is
the recursive JSON type, so that is now a TYPE ERROR at every producer, enforced by mypy.
Deliberately NOT pre-serialized bytes: ``_deliver_with_backoff`` must "serialize,
authenticate and POST as one act, so the bytes signed are the bytes sent" (prebid#1441), and
bytes on the queue would be something the sent body could diverge from.
A NEW STRUCTURAL TYPE, because the concrete one was a real constraint. The sender's
``config`` parameter named ``PushNotificationConfig``, which made "queue a projection
instead of the row" a type error — the only way to satisfy mypy was to keep the ORM
object on the queue. ``WebhookAuthConfig`` is a runtime-checkable Protocol naming the
THREE attributes the sender actually reads (``url``, ``authentication_type``,
``authentication_token`` — :167-168, :183, :321-322, :445, :452). Both the ORM row and
the projection satisfy it structurally, so the boundary declares the SHAPE it needs
instead of a class.
THE LEXICAL GUARD THAT WAS WRITTEN AND DISCARDED, recorded in the guard file so it is not
helpfully re-added: "a ``with get_db_session()`` block must not enclose a sleep or an
outbound POST" was GREEN ON ITS OWN MOTIVATING DEFECT. The session block called
``self._deliver_with_backoff(...)``; the ``time.sleep`` sat one call frame deeper, so a
body-subtree scan saw nothing. Any such scan is defeated by ordinary helper extraction.
Measured on the tree at the time it found 2 violations, NEITHER of them the defect it
existed for — filed instead as GH prebid#2052, since they are outside this PR's touch set.
FOUR MUTATION PROOFS, each red and green, whole file, never ``-k``, and reverted by
CONTENT RESTORE with a verifying diff (never ``git checkout`` — that restores from the
index and silently deletes uncommitted work; it destroyed this change once already):
baseline 14 passed
repo= back on deliver_adcp_webhook_sync 1 failed (i) -> restored
ORM-typed field on QueuedWebhook 1 failed (ii) -> restored
Mapped[...] annotation on QueuedWebhook 1 failed (ii) -> restored
frozen=True dropped 1 failed (ii) -> restored
green again 14 passed
Both guards were RED on the pre-change tree (3 failed) and are green after — so they
grade the production change itself, not only the mutations. Allowlists stay ``set()``.
A RATCHET SHRANK: tests/unit/test_delivery_service_behavioral.py's behavioural-mock cap
goes 6 -> 5, because the retry-loop test now builds a real ``QueuedWebhook`` instead of
mocking a config object.
Gate, all six: no TEMPORARY MUTATION tree-wide; no ``type: ignore[unreachable]`` in
tests/; duplication 35 / 68 / 0 with all three scopes printing "(unchanged)"; no
allowlist, ledger or baseline grown; ``git diff alembic/`` empty; and the CORRECTED sixth
check — ruff format, ruff check, mypy (322 files) and ``pytest tests/unit/ tests/harness/``
= 6919 passed. `make quality` alone would NOT have covered this: Makefile:35/56 run
``pytest tests/unit/ -x``, so tests/harness/ is never gated locally while CI's Unit Tests
job runs both.
Refs: salesagent-n78j0.4
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…it is gone S4, isolated follow-up to 0f1311b (salesagent-n78j0.4). ``_deliver_with_backoff`` ran inside a ``with get_db_session()`` block, and production said why: "Building the repository on THIS session (rather than opening a second one per delivery) is why ``_deliver_with_backoff`` runs inside this block." 0f1311b deleted that reason — ``_signing_repo`` opens its own short session and the queue carries a frozen dataclass of primitives — so the loop had nothing left holding it there. THE MOVE WAS NATURAL: no second, unstated reason surfaced, which was the finding this commit was kept separate to expose had it existed. The method now runs in two phases. PHASE 1, inside the session: read the receiver rows, apply the gates (auth-blocked, circuit breaker, SSRF), project each row to a ``QueuedWebhook`` and enqueue. Nothing there waits on a network. PHASE 2, after the block closes: deliver. ``_deliver_with_backoff`` sleeps between retries and POSTs to a buyer-supplied URL, so a connection held across it would be parked on a third party's latency — a hanging receiver consuming a pooled connection for the whole backoff, which is the exhaustion path this whole bullet exists to close. VERIFIED STRUCTURALLY, not by reading: an AST walk over this module finds ZERO ``with get_db_session()`` blocks whose body reaches ``_deliver_with_backoff`` (it was 1). That check is deliberately not shipped as a guard — a lexical enclosure scan is defeated by ordinary helper extraction and was green on this very defect while the sleep sat one frame deep. The shipped guards are the API-shape pair in 0f1311b, which hold at any call depth. THE STALE JUSTIFICATION IS DELETED WITH THE THING IT JUSTIFIED. A comment explaining why a session is held, surviving after it no longer is, is the same failure mode as prose holding an invariant up: it reads as current, and the next person believes it. Gate, all six: no TEMPORARY MUTATION tree-wide; no ``type: ignore[unreachable]`` in tests/; duplication 35 / 68 / 0, all three scopes printing "(unchanged)"; no allowlist, ledger or baseline touched; ``git diff alembic/`` empty; ruff format, ruff check, mypy (322 files) and ``pytest tests/unit/ tests/harness/`` = 6919 passed. Refs: salesagent-n78j0.4 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…p as one act
S4 (salesagent-n78j0.4). The sibling of ``provision_signing_key``, and it exists for the
reason ``canonical_origin()`` did: a caller should not be able to perform HALF of an
operation. Applied to a mutation this time rather than a read.
Revocation is two effects — stamp ``revoked_at``, drop the cached provider — and they
were two statements at one call site. That shape stays correct only while every future
call site remembers both. Now there is one function and the admin route calls it.
AND A FALSE CLAIM IN PRODUCTION, CORRECTED. The admin route said:
"The cache bust is not optional: the resolved provider is cached for 60 seconds,
so a revoke that skips it keeps signing with the retired key for up to a minute
after the operator was told it was retired."
THAT WAS WRONG, and the same tree contradicted it: ``provider.py`` :66-70 says a revoked
row "is refused BEFORE the cache is consulted (immediate, because the row is always
freshly read)". The provider is right. ``_resolve_cached`` calls ``_select_row`` at :293
and reads the cache at :296, and ``_select_row`` refuses a revoked row on BOTH paths —
``active_at`` excludes it in SQL, the explicit-``kid`` path raises at :243. A revoked key
stops signing IMMEDIATELY.
Two comments in one tree disagreed about whether a 60-second exposure window existed, and
NOTHING GRADED EITHER. That is the live defect here — not the ergonomics. The cache drop
is housekeeping: it evicts decrypted key material that is already unreachable. Keeping it
is right; believing it is the safety mechanism sends the next reader hunting a window
that does not exist.
GRADED NOW, and deliberately WITHOUT calling ``clear_signing_provider_cache()``.
``TestRevocationBeatsTheProviderCache`` resolves the key first (asserting the entry is
genuinely in ``_provider_cache`` and that the TTL is >= 60s, so a pass cannot be explained
by expiry), revokes, then resolves again in the same instant. Clearing the cache first
would have made it pass for the wrong reason — proving the cache can be emptied rather
than that revocation beats it.
THE FIRST MUTATION I TRIED PROVED NOTHING, AND WHY IS WORTH KEEPING. Moving the cache
lookup ABOVE ``_select_row`` left the test GREEN: the cache key is ``(tenant_id,
row.kid)``, so the row must be read to know the key, and on the ``active_at`` path the
ordering is FORCED rather than chosen. The property rests on the row filter, not on
statement order. The real mutation — drop ``revoked_at.is_(None)`` from ``active_at`` —
turns it RED (1 failed / 6 passed). Both were measured; the test's docstring records the
negative result so the next reader does not repeat it.
A NAME COLLISION, RESOLVED IN THE DIRECTION THE GUARD WANTED. The admin route function
was also called ``revoke_signing_key``, so the operation was first imported under an
alias — and ``test_no_dark_signing_primitives`` correctly failed: with the call site
naming the alias, the real symbol appeared only in an import, and that guard has an
explicit ``test_import_alone_is_not_a_production_caller``. The route function is renamed
to ``revoke_signing_key_route`` instead, so the operation is called by its own name. The
URL rule is on the decorator (``/<kid>/revoke``) and unchanged, so no route moves.
Gate, all six: no TEMPORARY MUTATION tree-wide; no ``type: ignore[unreachable]`` in
tests/; duplication 35 / 68 / 0, all three scopes printing "(unchanged)"; no allowlist,
ledger or baseline touched; ``git diff alembic/`` empty; ruff format, ruff check, mypy
(322 files), ``pytest tests/unit/ tests/harness/`` = 6919 passed, and the touched
integration module 7 passed against real Postgres.
Refs: salesagent-n78j0.4
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
92820c5 renamed the admin view function ``revoke_signing_key`` -> ``revoke_signing_key_route`` (the layer gained an operation by the original name, and ``test_no_dark_signing_primitives`` correctly refused an aliased import as a "caller"). Renaming a Flask view function RENAMES ITS ENDPOINT, and ``url_for()`` resolves by endpoint name — so templates/signing_keys_list.html:95 kept asking for ``signing_keys.revoke_signing_key``, which no longer exists. CI went red on Integration (infra) AND E2E Tests from this one line: the infra job runs ``test_form_actions_point_to_valid_endpoints``, and the e2e admin driver follows the POST's redirect and RENDERS this page, so the broken ``url_for`` 500s the request that was checking a flash. THE REASONING ERROR IN 92820c5's MESSAGE, corrected here: it said "the URL rule is on the decorator and unchanged, so the e2e helper's path still resolves." True, and beside the point — the rule string being untouched is exactly why path-based callers kept working while every ``url_for()`` caller broke. The half that was checked could not break. The sweep that missed it DID include ``--include=*.html`` but was scoped to ``src/ tests/``; ``templates/`` is a sibling of both, so it could not have found this file. Re-swept over the whole repo: this was the only stale reference. Every other ``revoke_signing_key`` hit is the layer operation, the ``@log_admin_action`` LABEL (a string, not an endpoint), or ``revoke_signing_key_via_admin`` (path-based, unaffected). Verified both directions: with the stale endpoint present ``test_form_actions_point_to_valid_endpoints`` FAILS; with this fix it passes (4 passed / 1 skipped in 0.13s). The infra marker slice is green — 15 passed, 51 skipped. That test needs NO database despite living under tests/integration/, so it joins the local gate rather than being left to CI by accident (recorded on salesagent-n78j0). Refs: salesagent-n78j0.4 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ne in a route
S4 step 5 (salesagent-n78j0.4). Same property as ``canonical_origin()`` and
``revoke_signing_key()``: the caller gets an OPERATION, not primitives plus the knowledge
of how to combine them.
``_governance_revocations_handler`` assembled five layer decisions inside an HTTP handler
— resolve the signing material, read the revoked rows, reach into config for the
interval, build the payload, sign it. A route that composes a pipeline out of layer
primitives is a second place that pipeline can be composed DIFFERENTLY, and the next
caller needing this document would have had to know the same five steps in the same order.
``publishable_revocation_list(repo, tenant, now=)`` now owns all five and returns
``(document, next_update)`` or ``None``. The handler keeps only HTTP concerns.
``None`` IS A WITHDRAWAL, NOT AN ERROR, and that is preserved verbatim: revoking a
tenant's only key withdraws the document rather than serving one unsigned or signed by a
dead key (security.mdx :1543). "Rotate before revoke" is the operational invariant the
behaviour asserts; the route turns it into a 404 with its own log line.
THE CACHE BOUNDARY, DECIDED DELIBERATELY. The layer returns ``next_update`` as an
INSTANT; the route converts it to ``max-age``. When the document goes stale is a DOMAIN
fact (the revocation interval), so it belongs to the layer; turning an instant into a
cache lifetime is HTTP shaping and stays with the transport. It is also why this document
alone does not publish ``CACHE_MAX_AGE_SECONDS`` like its three siblings — their
freshness is a fixed published TTL, this one's is when the next list is due.
THE MATERIAL-ISOLATION GUARD WAS STRENGTHENED, NOT RELAXED, AND THEN PAIRED.
``test_exactly_one_handler_resolves_signing_material`` asserted that exactly one handler
here resolves private key material. After this change NONE does — the resolution moved
into the layer — so it becomes ``test_no_handler_resolves_signing_material``. The bar
went UP: the three secret-free bootstrap routes still must never touch key material, and
now neither does the fourth.
That alone would have been a WEAKER guard in disguise, because deleting the signing step
entirely would also satisfy it. So it is paired with
``test_exactly_one_handler_publishes_the_revocation_list``, which pins that the handler
still reaches the material-resolving operation. Neither test means much alone; together
they say "the document is still signed, and only here".
Both proved in both directions, content save/restore, never ``git checkout``:
a bootstrap handler resolves material -> no_handler_resolves RED
the revocation handler stops publishing -> exactly_one_publishes RED
restored 4 passed
Gate: no TEMPORARY MUTATION tree-wide; no ``type: ignore[unreachable]`` in tests/;
duplication 35 / 68 / 0, all three scopes "(unchanged)"; no allowlist, ledger or baseline
touched; ``git diff alembic/`` empty; ruff format, ruff check, mypy (322 files);
``pytest tests/unit/ tests/harness/`` = 6920 passed; the two touched integration modules
24 passed against real Postgres; template endpoints 4 passed.
Refs: salesagent-n78j0.4
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…vations
S4 step 6 (salesagent-n78j0.4). Three call sites each opened a unit of work, re-read the
tenant and derived an identity URL by hand. The re-read and the derivation surface are now
shared; the FIELD each caller wants is not, and that distinction is preserved deliberately.
THREE SITES, NOT FOUR — and the fourth is the interesting one. The plan said four. Checked
against the actual pattern rather than a symbol count, only three match "open a UoW ->
re-read the tenant -> derive an identity URL, and nothing else":
src/app.py:445 -> identity.endpoints["a2a"]
src/admin/blueprints/authorized_properties.py -> identity.origin
SigningKeyRepository.canonical_origin -> identity.origin (own session)
``src/core/tools/accounts.py:1443`` is the CLOSEST MISS and is deliberately left alone: it
opens a ``TrustRootUoW``, re-reads the tenant and derives ``agent_endpoint_urls(tenant)``,
but the SAME transaction also carries ``uow.signing_keys`` for ``adcp_challenge_signer``.
Extracting the identity half would either split that transaction or force the helper to
hand a UoW back to its caller. Recorded here because it reads like an oversight otherwise
and the next person will re-add it. ``well_known.py:103`` (re-reads but derives no identity
— it hands ``(uow, tenant, now)`` to a builder) and ``capabilities.py:216`` (same
mixed-UoW shape as accounts) miss for their own reasons.
``src/app.py`` is the sharpest instance of what this dissolves: a DB read in the ASGI
COMPOSITION ROOT, four lines of unit-of-work plumbing to answer "what is this tenant's A2A
URL" while building an agent card.
THE DERIVATIONS DIFFER, SO THE SURFACE EXPOSES BOTH. Two callers want the canonical ORIGIN
and one wants the A2A ENDPOINT. Those are different facts about one identity, not one fact
spelled two ways, so ``AgentIdentity`` carries ``origin`` AND ``endpoints`` and each caller
asks for what it needs. Collapsing them into a single "identity URL" would have had to pick
one and would have lost a distinction the callers depend on.
SO THE HONEST CLAIM IS NARROWER THAN "ONE IMPLEMENTATION". What is now single-sourced is
the RE-READ and the identity surface; the two derivations remain two named functions
(``canonical_agent_url``, ``agent_endpoint_urls``) sharing one surface. That still dissolves
the repetition — no caller re-implements the open/assert/re-read/derive sequence — but it is
not one derivation, and saying so would overstate it.
THE SPLIT IS LOAD-BEARING, AND GRADED. ``agent_identity_for_tenant(tenant)`` is PURE and
opens nothing; ``agent_identity_for_tenant_id(tenant_id)`` is the UoW-opening convenience.
That split exists because ``canonical_origin`` MUST read on its own session — it resolves
the origin inside the transaction that produced the key row it is about to sign with
(92820c5). A helper that opened a UoW for all three would have silently un-proven exactly
what the flush-visibility test proves. Mutation, content save/restore, never git checkout:
canonical_origin delegates to the PURE half 11 passed
canonical_origin uses the UoW-opening half 1 FAILED
TestCanonicalOriginReadsTheRepositorysOwnTransaction — it read committed state
restored 11 passed
Gate, all six: no TEMPORARY MUTATION tree-wide; no ``type: ignore[unreachable]`` in tests/;
duplication 35 / 68 / 0, all three scopes "(unchanged)"; no allowlist, ledger or baseline
touched; ``git diff alembic/`` empty; ruff format, ruff check, mypy (322 files);
``pytest tests/unit/ tests/harness/`` = 6920 passed; template endpoints 4 passed in their
OWN invocation (appending them to the unit session displaces its get_db_session patching —
see salesagent-n78j0); signing-key repository 11 passed against real Postgres.
Refs: salesagent-n78j0.4
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
S4 step 7 (salesagent-n78j0.4). Every repository in this layer takes
``(session, tenant_id)``, so ``_init_repos`` was the same line with a different class,
written out once per repository per unit of work — and ``_clear_repos`` was its mirror,
written out again. That is a shape that should be DATA, not code.
``BaseUoW._REPOSITORIES`` is now a ``{attribute: RepositoryClass}`` declaration, and the
base class drives BOTH hooks from it. The three units of work this epic added declare:
SigningKeyUoW signing_keys
CapabilitiesUoW tenant_config, signing_keys
TrustRootUoW tenant_config, signing_keys, authorized_properties
THE SYMMETRY IS NOW STRUCTURAL, which is the real defect this removes. Construction and
teardown were two hand-written halves that COULD DISAGREE: adding a repository to
``_init_repos`` and forgetting ``_clear_repos`` leaves it — and its session — attached to
a unit of work that has already exited. Derived from one declaration, they cannot.
THE LOUD-FAILURE PROPERTY IS PRESERVED. ``_init_repos`` previously raised
``NotImplementedError``; a naive default of "iterate an empty mapping" would have turned
"forgot to wire the repositories" into a unit of work whose attributes are all ``None`` at
first use. It still raises when a subclass declares nothing AND does not override,
naming the class and both escape routes. Verified directly, not assumed.
EIGHT PRE-EXISTING UNITS OF WORK KEEP THEIR IMPERATIVE HOOKS, DELIBERATELY. Overriding
still works and they are untouched by this PR — migrating them would be the scope creep
the charter forbids, and they can convert when something else touches them. The base
class documents both routes so the coexistence is a stated choice rather than an
inconsistency someone later "tidies".
THE DECLARATION IS LOAD-BEARING, not decorative. Mutation, content save/restore, never
git checkout: truncate ``TrustRootUoW._REPOSITORIES`` to ``tenant_config`` alone and
``tests/integration/test_trust_root_documents.py`` goes 17 passed -> 8 FAILED / 9 passed
(the adagents pin and the JWKS key set lose their repositories). Restored: 17 passed.
Gate, all six: no TEMPORARY MUTATION tree-wide; no ``type: ignore[unreachable]`` in
tests/; duplication 35 / 68 / 0, all three scopes "(unchanged)" — the removed lines were
short enough to sit under R0801's threshold, so this is the floor holding, not a claim
that nothing was deduplicated; no allowlist, ledger or baseline touched; ``git diff
alembic/`` empty; ruff format, ruff check, mypy (322 files); ``pytest tests/unit/
tests/harness/`` = 6920 passed; template endpoints 4 passed in their own invocation; the
three UoWs' integration modules 35 passed against real Postgres.
Refs: salesagent-n78j0.4
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Closes prebid#2055. S4 (salesagent-n78j0.4) — folded in because tests/unit/test_order_approval_service.py is the test module for the service this step rewrites, so it is inside the touch test rather than the excursion it was first filed as. ``capture_outbound_webhooks`` replaces the SOCKET, so its rebind is process-global and recorded every outbound POST in the window — including one from a background thread belonging to a test that had already finished. The helper was structurally unable to tell "my sender delivered twice" from "someone else's delivery landed in my list", and every assertion that COUNTS captures inherited that blindness. IT WAS A REAL DEFECT, NOT A FLAKE, AND THE SUITE HAD ALREADY BENT AROUND IT. The same test carried: # Note: Due to test pollution in full suite, may see 4 calls, but minimum is 3 assert len(captured) <= 4, "... (3 + 1 pollution)" A count assertion can absorb an intruder. A KEY-SET assertion cannot, because the intruder brings its own key — which is why that one kept reddening CI while the widened count stayed green. DIAGNOSED BEFORE FIXING; THE OTHER HYPOTHESIS IS RULED OUT. Two failures, two tests, two seeds: run 32495625542 seed 174142508 test_webhook_notification_sent_on_success 2 == 1 run 32508361535 seed 13760353 test_webhook_retries_on_failure 2 == 1 The second asserts ONE idempotency key across a retry ladder and saw two DISTINCT keys, which is consistent with both a leak AND a genuine double-send. Production settles it: ``order_approval_service`` mints the key at :419, OUTSIDE the retry loop at :423, and passes that same variable at :428 — a retry cannot mint a second key, so "one key per event" holds by construction and the extra key arrived with a delivery the test never made. The arithmetic agrees exactly: 4 captures, 2 distinct keys = 3 retries sharing one key + 1 foreign delivery with its own. SCOPED BY PROVENANCE, NOT BY TIMING. A delivery is OURS when it comes from the thread that opened the block, or from a thread that did not exist when it opened (one the test spawned itself). A thread alive BEFORE the block and not ours belongs to somebody else. Timing-based mitigation would have stayed sensitive to runner speed — which is exactly why this reproduced on CI (~620s) and never locally (~175s). Foreign deliveries are still ANSWERED (200), never dropped mid-flight: the other test's sender must not crash because this one is watching the socket. BOTH HALVES ARE GRADED, because scoping to "the capturing thread only" would have been a silent under-count — several webhook tests deliver from a worker they start themselves. ``test_webhook_capture_scoping.py`` pins the foreign thread is excluded AND the test-spawned thread is included. Mutation, content save/restore, never git checkout: removing the scope check turns the first RED (2 captures, 2 keys — the CI failure reproduced in-process) and leaves the second green. Restored: 2 passed. AND THE TOLERANCE IS GONE. ``test_webhook_retries_on_failure`` now asserts ``len(captured) == 3`` — exactly two refusals and the acceptance. An extra delivery is a defect again rather than an expected nuisance, which is the point: the fix is only worth having if the assertions it protects are allowed to be strict. Gate, all six: no TEMPORARY MUTATION tree-wide; no ``type: ignore[unreachable]`` in tests/; duplication 35 / 68 / 0, all three scopes "(unchanged)"; no allowlist, ledger or baseline touched; ``git diff alembic/`` empty; ruff format, ruff check, mypy (322 files); ``pytest tests/unit/ tests/harness/`` = 6922 passed; template endpoints 4 passed in their own invocation. Refs: salesagent-n78j0.4 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hods Closes prebid#1878. S4 step 9's remaining half (salesagent-n78j0.4). Acceptance, verbatim from the issue: "Both call sites route through repository/UoW methods, no raw get_db_session()/Account(...) kwargs remain at either site; repository-pattern guard passes with no new allowlist entries." All three hold. SITE 1 — src/admin/blueprints/accounts.py assembled its own ``Account(...)`` from raw kwargs while the sync/provisioning arm AND its dry-run preview both went through ``AccountRepository.build_row``. Two callers shared one row description; a third went its own way — exactly the drift prebid#1721 added that builder to stop. It now calls the SAME builder, so three callers share one description. ``build_row`` NEEDED NO WIDENING. Its signature already ends ``created_fields: dict[str, object]`` and its body ends ``**created_fields``, with the comment "Every settable field comes from the one walk in the caller -- naming them here is what let a field be added to the re-sync arm and forgotten at create." ``billing`` / ``payment_terms`` / ``sandbox`` ride there. ONE REAL SHAPE MISMATCH, FOUND BY CHECKING RATHER THAN TRUSTING: the blueprint set ``brand = {"domain": d} if d else None`` while ``build_row`` always built a dict. Only ``name`` is validated on that form, so an empty ``brand_domain`` is reachable, and ``Account.brand`` is ``nullable=True`` — ``{"domain": ""}`` is a DIFFERENT value from ``None``. ``build_row`` now returns ``None`` for a falsy domain, which is a no-op for every existing caller (all pass a domain) and correct for the one that need not. SITE 2 — src/services/order_approval_service.py opened ``get_db_session()`` inline for one repository read. It now uses ``PushNotificationConfigUoW``. That was NOT a drop-in, and the reason is the interesting part: ``BaseUoW.__exit__`` COMMITS on clean exit (uow.py :107-108) and no session sets ``expire_on_commit=False`` (uow.py :379). ``get_db_session`` closes WITHOUT committing. The loader returned a live ORM row and carried a paragraph explaining why that was safe — "Detaching it at the end of the session is safe ... the loaded columns survive". So a straight swap expires the row before ``_approval_webhook_headers`` reads it: measured, both webhook tests went red with 0 of 3 deliveries. That paragraph was a PRIMITIVE PLUS PROSE — correctness resting on a subtle SQLAlchemy behaviour explained in a comment, the shape this epic has been dissolving all through S4. So the row no longer escapes: ``ApprovalWebhookAuth`` is a frozen dataclass of the four fields the callers actually read, projected INSIDE the unit of work. The same move as ``QueuedWebhook`` (0f1311b). The unit of work may now commit and expire whatever it likes, and the paragraph is DELETED because it explains a hazard that no longer exists. It satisfies ``WebhookAuthConfig`` structurally — the Protocol added in 0f1311b — so it is passed as ``config=`` to the delivery boundary unchanged, with ``validation_token`` alongside for the one extra header this service adds. The two tests mocked ``get_db_session`` in that module, which a unit of work bypasses. They now patch ``_load_approval_webhook_config`` — the seam whose ANSWER they care about ("there is a bearer registration" / "there is none") rather than a session it no longer opens. DECLINED, and named so the decline is visible: 9 ``get_db_session`` calls and 5 ``select(SyncJob)`` sites remain in order_approval_service.py. Only the loader is in prebid#1878's scope. The bead pointed this step at "order_approval_service + get_by_sync_id()" and at "the remaining SyncJob sites" — the ISSUE contains neither; it names two sites in two different files, and re-reading it is what found that. Gate, all six: no TEMPORARY MUTATION tree-wide; no ``type: ignore[unreachable]`` in tests/; duplication 35 / 68 / 0, all three scopes "(unchanged)"; NO ALLOWLIST, ledger or baseline touched — repository-pattern and sync-accounts-row-builder guards both green with their allowlists unchanged; ``git diff alembic/`` empty; ruff format, ruff check, mypy (322 files); ``pytest tests/unit/ tests/harness/`` = 6922 passed; template endpoints 4 passed in their own invocation; account/approval integration slice 234 passed against real Postgres. Refs: salesagent-n78j0.4 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
S5, first half (salesagent-n78j0.5). The trust-root documents pointed ``$schema`` at a
version LITERAL, and the test that was supposed to pin it could not see the value.
THE BLINDNESS WAS STRUCTURAL. ``"$schema"`` is a MEMBER of ``_BRAND_AGENT_KEYS`` in
``test_trust_root_documents.py`` — a key-set containment check. It grades that the KEY
EXISTS. A document pointing at the wrong spec version has the key, so the pin was blind to
exactly the drift a ``$schema`` exists to prevent. Fixed by asserting the VALUE, not by
adding another key.
AND THE DRIFT WAS ALREADY THERE. Two WIRE values disagreed with production:
tests/helpers/signing.py:543 .../schemas/v1/brand.json
tests/integration/test_request_signature_discovery.py:190 .../schemas/v1/brand.json
src/core/signing/trust_root.py:51 (production) .../schemas/3.1.1/brand.json
The fixtures built a brand.json claiming v1 while the deployment served 3.1.1, and the
key-set pin could not tell. Both now derive from the pin.
GREPPED BEFORE TOUCHING, because "v1" appears in nine places and only two are wire:
``creatives.py:731``, ``validation_helpers.py:219`` and ``protocol_envelope.py:4``/``:65``
are comments, a docstring and an error MESSAGE; five more are test comments citing schema
paths. ``test_validation_errors.py:123`` asserts on that error message, so changing it
would have broken an unrelated test for no gain. Left alone.
``_SCHEMA_BASE`` is now ``f"...schemas/{get_adcp_spec_version()}"`` — the same derivation
the A2A extension URI already uses (``adcp_a2a_server.py`` :2362), so the two cannot drift.
A SECOND PIN WAS MISSING ENTIRELY. ``tests/unit/test_adcp_spec_version.py`` tied the SDK
to ``EXPECTED_SPEC_VERSION``, but the vendored schema tree beside it —
``tests/fixtures/adcp_schemas_pinned/<version>/`` — carries the version as a DIRECTORY
NAME with nothing checking it. A bump would have moved the SDK and every derived
``$schema`` while our documents were still validated against the previous version's
fixtures. ``test_the_vendored_schema_tree_matches_the_pin`` closes that, beside the guard
whose mechanism it borrows. Mutation, content save/restore: setting EXPECTED_SPEC_VERSION
to "9.9.9" turns BOTH red; restored, both pass.
THE BUMP LIST HAD ITS OWN STALE POINTER (tenth citation defect this epic): step 4 said
``EXPECTED_SPEC_VERSION`` lives in ``tests/unit/test_adcp_spec_version.py``; it lives in
``tests/helpers/adcp_pin.py``, which that test imports. Corrected, the schema-tree
re-vendor added as its own step, and a closing note that the served ``$schema`` values need
no bump action because they now derive.
ONE OF THIS STAGE'S COSTS WAS ALREADY PAID. The bead lists "well_known.py:208 re-parses a
timestamp the builder computed as a datetime and formatted three lines earlier". IT IS
GONE — 86e9632 dissolved it when the pipeline moved into the layer and ``next_update``
became a domain instant while max-age stayed HTTP shaping. Recorded rather than hunted,
because a bead item a previous atom already dissolved is evidence the atoms were cut along
the right seams.
THE SDK-MODEL CONVERSION IS DELIBERATELY NOT IN THIS COMMIT, and it is viable — checked,
not assumed. ``AgentSigningKey`` declares [kid, kty, alg, use, crv, x, y, n, e,
revoked_at] and NOT ``key_ops``/``adcp_use``, which ``_published_jwk`` passes through
verbatim from ``row.public_jwk``; that would have been a silent publication regression
except that the model sets ``extra: allow`` and round-trips both. So the conversion holds
and is its own atom, with brand.json noted as having no model in the pinned set.
Gate, all six: no TEMPORARY MUTATION tree-wide; no ``type: ignore[unreachable]`` in
tests/; duplication 35 / 68 / 0, all three scopes "(unchanged)"; no allowlist, ledger or
baseline touched; ``git diff alembic/`` empty; ruff format, ruff check, mypy (322 files);
``pytest tests/unit/ tests/harness/`` = 6923 passed; template endpoints 4 passed in their
own invocation; the three signing integration modules 30 passed against real Postgres.
Refs: salesagent-n78j0.5
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…models
S5, second half (salesagent-n78j0.5). TWO of the four documents are now schema-valid BY
CONSTRUCTION; the other two are explained rather than half-converted.
build_jwks -> AgentSigningKey CONVERTED
build_brand_json -> BrandDiscovery3 CONVERTED (aliased, see below)
build_adagents_json -> AdcpAgentsAuthorization RESISTS — model rejects a document we serve
build_revocation_list -> adcp.signing.RevocationList RESISTS — a dataclass with no ``version``
O1 ("a third party fetches our JWKS and verifies") only means something if an invalid
document cannot leave this module. For the two converted, it cannot: ``_validated``
round-trips through the pinned model, so a malformed document raises HERE rather than
passing a test that validated a dict assembled two lines earlier.
THE ALIAS IS LOAD-BEARING, AND NOT COSMETIC. ``BrandDiscovery3`` is a POSITIONAL generated
name — "the third oneOf variant in generation order", not "the brand-agent document". It
is imported ``as LibraryBrandAgentDocument`` (CLAUDE.md Pattern #1), deliberately NOT as
``BrandDiscovery``, which is the SDK's own RootModel union wrapper. A regeneration that
adds or reorders variants would silently rebind that name to a different shape;
``extra: forbid`` catches the common case, but a new variant that is a SUPERSET of our
three keys would bind silently. So the BINDING is pinned by field set in
``test_the_brand_agent_variant_is_still_the_one_we_bind`` — the same argument
``_BRAND_AGENT_KEYS`` makes for the document, applied one level up to the model.
Mutation: point the alias at ``BrandDiscovery5`` -> RED.
THE JWKS CONVERSION RESTS ON AN UNDECLARED BEHAVIOUR, NOW PINNED. ``AgentSigningKey``
declares [kid, kty, alg, use, crv, x, y, n, e, revoked_at] and NOT ``key_ops`` or
``adcp_use`` — both of which ``adcp.signing.keygen`` emits and ``_published_jwk`` passes
through verbatim. A naive conversion would have SILENTLY STOPPED PUBLISHING two members of
every JWK. It is safe only because the model sets ``extra: allow``, so
``test_the_published_jwk_model_preserves_members_it_does_not_declare`` pins that
round-trip. Nothing graded it before; an SDK bump to ``forbid`` or ``ignore`` would have
been a published-key regression with no test to catch it.
WHY THE OTHER TWO RESIST:
* ``RevocationList`` is a DATACLASS whose fields are [issuer, updated, next_update,
revoked_kids, revoked_jtis]. Our document also carries ``version``, which the SDK's own
``revocation_fetcher`` validates as a positive int. A dataclass has no ``extra: allow``,
so converting would drop a member the SDK's consumer requires.
* ``AdcpAgentsAuthorization`` REJECTS the empty-authorization document this builder
deliberately emits (``minItems: 1`` on ``authorized_agents``). An empty list asserts NO
sales authorization; fabricating an entry would self-attest an authorization no
publisher granted. Converting would mean inventing one or dropping the document.
AN OPEN QUESTION I AM NOT ANSWERING HERE, because it deserves its own investigation: while
writing a test to GRADE the adagents claim, the pinned schema rejected a minimal
``{$schema, authorized_agents: []}`` document. The existing
``test_adagents_claims_no_authorization_without_a_backing_record`` asserts only HTTP 200
and an empty list — it never validates that document against the schema. So "the model is
stricter than the spec" is NOT established, and the opposite may hold. The test asserting
it was removed rather than shipped on an unverified premise; filed for follow-up.
A RATCHET BREACH FROM e1f7588, FIXED HERE, NOT BASELINED. Quality Gate found
``check_untyped_defs`` at 212 against a baseline of 211:
accounts.py:101: Argument "operator" to "build_row" has incompatible type
"str | None"; expected "str"
The fix is the SIGNATURE, not the call site: ``models.py`` :864 declares
``operator: Mapped[str | None]`` on a NULLABLE column, so ``build_row(operator: str)`` was
always narrower than the row it builds — the sync path just never passed None. Routing the
admin blueprint through the shared builder exposed a pre-existing type lie. The blueprint
still writes ``operator or None`` so an empty form field becomes SQL NULL rather than "";
"fixing" it at the call site would have silently changed what admin create writes. Ratchet
back to 211, unchanged; ``--update-baseline`` NOT run.
Gate: ``make quality`` WHOLE (exit 0 — it runs the untyped-defs ratchet that its
decomposed parts do not, which is how the breach rode in); ``pytest tests/unit/
tests/harness/`` = 6925 passed; template endpoints 4 passed in their own invocation; the
three signing integration modules 30 passed against real Postgres; duplication 35 / 68 / 0
all three "(unchanged)"; no allowlist, ledger or baseline touched; ``git diff alembic/``
empty.
Refs: salesagent-n78j0.5
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
S5 (salesagent-n78j0.5). 157b8ac's model conversion changed PUBLISHED BYTES as an incidental side effect, and left one origin serving two spellings of one format. MEASURED, not reasoned about — all four documents, every timestamp field: JWKS revoked_at trust_root.py:104 -> model ...115782Z CHANGED brand.json last_updated trust_root.py:116 -> model ...115782Z CHANGED adagents.json revoked_at trust_root.py:104, unconverted ...115782+00:00 revocation updated / revocation_list.py:104-105, ...115782+00:00 next_update unconverted THE BAD CASE IS NOT HYPOTHETICAL. ``build_adagents_json`` embeds the same ``_published_jwk`` output while adagents itself resists conversion, so ``signing_keys[].revoked_at`` and brand.json's ``last_updated`` — documents served from the SAME ORIGIN — carried different spellings. That is strictly worse than either choice and it was live. NOTHING IN THE SPEC PICKS ONE. Both are valid RFC 3339 and the pinned schema constrains neither (``adagents.json`` :110-113 is ``{"type": "string", "format": "date-time"}``). So "the schema allows it" is not authority to change published bytes: these are trust-root documents, O1 is "a third party fetches our JWKS and verifies", and a counterparty string-comparing ``revoked_at`` is doing nothing we ever told them not to. The bytes stay what counterparties have been receiving. THE SERIALIZERS CALL ``rfc3339()``. They do NOT re-implement ``isoformat()`` — a second implementation that agrees today is precisely the fault. ``src/core/signing/_rfc3339.py`` exists because this codebase already shipped two formatters that "happen to agree today" (its own docstring), and the conversion re-created that from the other side. Now there is ONE formatter and every document routes through it, so agreement is structural rather than coincidental. ``PublishedSigningKey`` and ``PublishedBrandDocument`` SUBCLASS the pinned models (CLAUDE.md Pattern #1, ``Library*`` alias) rather than patching them, so an SDK bump changes no caller. ``BrandDiscovery3``'s ``extra: forbid`` and the ``$schema`` alias both survive — verified, not assumed. THE REAL DELIVERABLE IS THE BYTE-PINNING TEST, which is what ``_rfc3339``'s docstring wishes it had had in prebid#1291. ``test_published_timestamps_render_one_spelling`` asserts RENDERED BYTES across all four documents. A "parses as a datetime" test sails straight past this — ``...Z`` and ``...+00:00`` are the same instant. The regression was caught by an integration assertion that compared bytes rather than parsing them; that assertion is untouched, because it was correct about what the wire was. MUTATION, content save/restore: drop either ``field_serializer`` and the byte test goes RED on a ``Z``; restored, 5 passed. The integration assertion that first noticed is green again (17 passed). Gate: ``make quality`` WHOLE (exit 0) — ``check_untyped_defs`` 211 UNCHANGED and duplication 35 / 68 / 0 all "(unchanged)"; ``pytest tests/unit/ tests/harness/`` 6926 passed; template endpoints 4 passed in their own invocation; no allowlist, ledger or baseline touched; ``git diff alembic/`` empty. Refs: salesagent-n78j0.5 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
B2, salesagent-n78j0.7. The guard shipped with its allowlist ALREADY POPULATED by a
symbol the same PR introduces — an allowlist larger than it started, which the charter
forbids, and which this guard's own fix hint forbids by name: "Do NOT add an allowlist
entry to make this pass."
PROVENANCE, verified rather than argued: ``git show origin/main:src/core/signing/
provider.py`` -> ABSENT, and the guard file is absent on main too. Symbol new in this PR,
guard new in this PR, allowlist empty -> one entry.
THE ENTRY'S ARGUMENT WAS HALF RIGHT, AND BOTH OF ITS ALTERNATIVES WERE WRONG.
NOT dead code. ``_resolve_signing_provider`` and ``resolve_signing_material`` are
one-line projections of the SAME ``_resolve_cached`` call — identical signatures,
identical cache, identical raises — and ~10 integration tests grade that shared path
THROUGH the provider form, including ``provider.sign()`` and ``provider.key_id()``, i.e.
production's binding of resolved material into an ``InMemorySigningProvider``. Deleting
it would make those tests construct that themselves and grade a TEST-SIDE COPY.
NOT wireable to a production caller either. ``adcp.webhooks.WebhookSender``'s RFC 9421
constructor takes a raw ``PrivateKey``, so C1's outbound boundary consumes
``resolve_signing_material``. Inventing a caller to satisfy a guard would be worse than
the allowlist entry.
WHY THE UNDERSCORE IS NOT GUARD-EVASION, WHICH IS THE ONLY THING THAT MAKES THIS
LEGITIMATE: the symbol was NEVER on the layer's public surface.
``src/core/signing/__init__.py`` neither imports nor exports it — it is absent from
``__all__`` — and that module's docstring states that everything below the package is
PRIVATE to the layer, with callers importing from the facade only. So it was "public"
solely by naming convention inside a private submodule. The leading underscore states a
fact that was already true, and the guard's rule ("a PUBLIC symbol reached only from
tests") correctly stops applying because the symbol was never in its scope — the scope did
not move.
HAD IT BEEN IN ``__all__``, THE SAME RENAME WOULD HAVE BEEN A DODGE. It is worth a future
reader knowing the difference was checked, not assumed.
MUTATION, and the first attempt was MIS-SHAPED — worth recording because it would have
"proved" the guard while proving nothing. Renaming only the definition back to
``resolve_signing_provider`` left the test helper referencing the underscore name, so the
guard saw a public symbol with NO test references and correctly did not flag it: 9 passed.
That is a different state, not the state under test. Reconstructing the TRUE before —
symbol public AND allowlist empty, via content save/restore of provider.py, the test helper
and the guard — gives:
new violations (1): ('src/core/signing/provider.py', 'resolve_signing_provider')
"A public symbol on the signing-credential surface is referenced from tests/ but from
no live module under src/ or scripts/ — it is dark in production."
Restored: 9 passed. A mutation must produce the state you claim to be testing, not merely
a different one.
Gate: ``make quality`` WHOLE (exit 0) — check_untyped_defs 211 unchanged, duplication
35 / 68 / 0, fixme-citation 19 / 119 unchanged; ``pytest tests/unit/ tests/harness/``
6926 passed; template endpoints 4 passed in their own invocation; no allowlist, ledger or
baseline grown — this one SHRANK to frozenset().
Refs: salesagent-n78j0.7
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
D1 (salesagent-n78j0.10, prebid#1879). Completed in this PR on the owner's confirmation of his 2026-08-05 call — "all three should be addressed as part of this PR, not merged around" — rather than deferred. THE FAULT, AND THIS PR ALREADY WROTE THE CURE TWO FILES AWAY. ``signing_contract/algorithms.py`` :84/:96 — ``narrow_alg`` / ``narrow_purpose`` — check membership against an owned value set and raise ``AdCPConfigurationError`` BEFORE casting, precisely because a ``cast`` does not validate. This PR introduced that shape and then violated it at the boundary that decides whether a request is refused. ``cast(BrandAgentType, config.counterparty_agent_type)`` at :922 and :1100. ``SigningConfig.counterparty_agent_type`` is a plain ``str`` in pydantic-settings (``config.py`` :256) and therefore ENV-OVERRIDABLE, while ``async_resolve_agent`` wants the SDK Literal. A cast is a RUNTIME NO-OP, so a typo passed validation, passed the cast, reached the resolver, matched no ``agents[]`` entry in the counterparty's brand.json, and 401'd EVERY SIGNED COUNTERPARTY with nothing naming the cause. ``narrow_agent_type()`` reads the permitted set from ``get_args(BrandAgentType)`` rather than re-typing it, so an SDK that adds an agent type cannot leave a hand-written copy behind. BEHAVIOURAL: a misconfigured deployment now fails fast at the boundary instead of silently refusing correct counterparties. That is the point, and it is tested both ways — removing the membership check turns ``test_a_typo_is_refused_at_the_boundary`` RED. PAIRED WITH A STRUCTURAL TEST, because behavioural coverage structurally cannot see this regression: a reintroduced ``cast`` is a runtime no-op, so it would pass every behavioural test while restoring the silent 401. ``test_neither_call_site_casts`` asserts the ABSENCE of the cast; the behavioural test asserts the PRESENCE of the refusal. Neither alone is a contract. THE ``Any`` WAS NOT HIDING NOTHING — IT WAS HIDING TWO DEFECTS, and both surfaced the moment a real type went in: * ``AnyUrl(jwks_origin)`` exposed a ``str | None`` that mypy could not narrow through the ``anchored`` flag — a malformed or absent URL had been reaching the served capabilities document via ``cast(Any, ...)``, unvalidated. * ``DeclaredBucket`` exposed that the three ``protocol_methods_*`` buckets are generated ``RootModel[str]`` wrappers, NOT ``str | Enum`` — the non-uniformity ``_name`` exists to flatten, which the ``Any`` had made invisible at the signature. Both fixed properly rather than re-widened. That is the argument for this atom, made by the atom itself. THE REST OF THE SCOPE: * ``notification_proof_service``: ``config: NotificationConfig`` at all three sites and all eight ``getattr`` probes DELETED, not defaulted. Each default converted an impossible ``AttributeError`` into a PLAUSIBLE WRONG VALUE inside a challenge document a receiver must byte-match. The only caller already typed it (``accounts.py`` :1532). * ``from_tenant(declared: Any)`` -> ``object``, which forces the ``isinstance`` guard at :311 to be visible to mypy. Under ``Any`` every attribute access past it typechecked whatever the shape — the opposite of what a parse boundary is for. * ``_reject_unbacked`` -> ``_reject_unbacked[T: Enum]``. With ``Iterable[Any]`` on both sides the signature stated neither the element type nor the RELATION, so a protocol list checked against a specialisms dict typechecked cleanly. Bounded to ``Enum`` because the refusal message reads ``.value``. * five partially-annotated signatures (three admin routes, the ``/a2a/`` redirect, the vendored-canonicalizer delegate). *** WHY ``posture_for_tenant(tenant: Tenant | None)`` WAS NOT ADOPTED — DO NOT "FINISH" THIS BY EDITING resolved_identity.py. *** The prescription asked for that signature. It is UNREACHABLE from here: the caller holds no ORM row. ``_detect_tenant_for_posture`` delegates to ``_detect_tenant`` (``src/core/resolved_identity.py`` :71), which returns a dict and is shared with the identity/auth path at :161. Reaching the ORM type would mean either changing that function — which is IDENTITY RESOLUTION, explicitly out of scope ("identity/ACL collapse (prebid#1870 owns it)") — or reading the tenant a SECOND time on the request path, inside the very function whose docstring exists to avoid a second read. The first is barred by the charter; the second is a behaviour change smuggled in as a type fix. ``PostureTenant`` (TypedDict, ``total=False``) states the two keys the posture actually reads, and the raw dict is PROJECTED into it at the signing layer's boundary — the same move as ``QueuedWebhook`` and ``ApprovalWebhookAuth`` earlier in this epic. A cast would assert the shape; the projection carries it. The ORM boundary stays where prebid#1870 will find it. Gate: ``make quality`` WHOLE (exit 0) — check_untyped_defs 211 UNCHANGED (it counts errors inside unchecked bodies, so annotating five signatures exposed none; no rise is what matters), duplication 35 / 68 / 0, fixme-citation 19 / 119; mypy 322 files; ``pytest tests/unit/ tests/harness/`` 6929 passed; template endpoints 4 passed in their own invocation; signing/capabilities/posture/notification integration slice 201 passed against real Postgres. No allowlist, ledger or baseline touched. Refs: salesagent-n78j0.10, prebid#1879 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
_UC004_E2E_WEBHOOK_INTERNAL_TAGS 11 -> 10. The e2e_rest_known_failures.txt
ledger is untouched at 17 — this tag was never a ledger row.
Traced independently of its already-graduated 9421 sibling rather than assumed
to ride along on the shared step-layer fix, per
.claude/rules/workflows/xpass-graduation.md. One link did not match the
hypothesis: _attach_reporting_webhook writes raw_request["reporting_webhook"]
with NO authentication block, and security.mdx @ v3.1.1 :1424 makes that
absence the RFC 9421 selector — so on the harness alone the HMAC arm looks
unreachable over e2e and the xpass looks vacuous. It is not. Production's
_send_report_for_media_buy queries DBPushNotificationConfig by
(principal, tenant, url, is_active), and that row — written by the Given into
the container's own DB — overrides the auth-less raw_request, so
build_webhook_sender takes the LEGACY_HMAC arm and signs via
from_adcp_legacy_hmac.
All three Thens read env.last_delivery(); the last recomputes HMAC-SHA256 over
captured.content, i.e. the bytes the receiver actually got, so it fails when
the bytes signed are not the bytes sent. None reads env.mock["post"], which is
what leaves the rest of this set unobservable over Docker HTTP.
MUTATION EVIDENCE (the run id pair, kept here and not in the file because any
edit to conftest.py voids it — tox.ini :181 collects `pytest tests/bdd/`):
unmutated innet_220826_0145 544 passed / 17 xpassed / 2033 xfailed / 2
skipped / 2596 total, exit 0 — the node is a plain PASS.
mutated innet_220826_0155 543 passed / 1 FAILED / 17 xpassed / 2033
xfailed / 2 skipped, exit 1 — the sole failure is this node.
The mutation blanked the LEGACY_HMAC arm in build_webhook_sender to
_unauthenticated_sender: the delivery still happens, only the signature is
gone. The resulting failure names the received header list —
Expected header 'X-ADCP-Signature' but got: [..., 'x-real-ip',
'x-forwarded-for', 'x-forwarded-proto', ...] host: webhooks.adcp-e2e.dev:8443
— and x-real-ip / x-forwarded-for / x-forwarded-proto are proxy-added by a real
network hop, which an in-process MagicMock cannot manufacture. That is the
vacuity question answered by observation: the delivery is provably server-made,
and the Then grades the SIGNATURE rather than merely observing that something
was posted.
Also in this change, both required by the graduation rather than incidental:
* EXPECTED_WEBHOOK_INTERNAL_TAGS in tests/unit/test_e2e_rest_ledger_state.py
shrinks to match. The first full gate failed on
test_webhook_internal_tags_match_pin because the removal was not mirrored
there — the lock forcing exactly the review step it exists for.
* conftest.py :412 carried a stale, provably false note about this same tag
("Then steps are pending (no-op); test passes trivially"). False since prebid#1291
C1 / salesagent-n78j0.1.4 routed the Thens. Corrected in place rather than
deleted: it was read as current during this inspection and pointed the
opposite way from the tree.
Final gate innet_220826_0235: all seven suites green, exit 0.
Refs: salesagent-n78j0.13
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
_UC004_E2E_WEBHOOK_INTERNAL_TAGS goes 10 -> 9. The e2e_rest ledger (tests/bdd/e2e_rest_known_failures.txt) is NOT touched and stays at 17 — this tag was never a ledger row. Traced independently of the hmac row graduated in 0cebe90, not carried by it. The two share a tag set, a step layer and a harness, and this epic has twice had such neighbours be wrong about each other. THE QUESTION THAT DECIDED IT. This row is structurally weaker than its hmac sibling: hmac has three Thens, one of which recomputes the digest over the received bytes; bearer has ONE Then and no recompute. So the question was not "does the delivery happen" but "does that lone Then grade the token's VALUE, or merely the header's PRESENCE?" — presence-only being the vacuity signature, since it would pass for any Authorization header anything happened to attach. It grades the VALUE, proved by mutation rather than by reading. The mutation shape was chosen to separate the two questions: a WRONG-BUT- PRESENT token ("z"*32 in the LEGACY_BEARER arm), deliberately NOT a removed header — removing the header would only re-prove presence, the half that was never in doubt. The leg went red at exactly the value assertion, having PASSED the presence and "Bearer " prefix checks first: AssertionError: Bearer token mismatch: expected 'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb', got 'zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz' That a PRODUCTION mutation changed what the receiver captured also settles the delivery half: the observation is downstream of production, so the capture is not an in-process mock that production cannot reach. Chain verified end to end: given_bearer_token_valid sets ctx['webhook_bearer_token']='b'*32 -> _auth_scheme_to_db_fields maps scheme 'bearer' onto authentication_type/authentication_token -> _persist_webhook_config_if_needed writes the row -> the live server's _send_report_for_media_buy finds it by (principal, tenant, url, is_active), NOT the auth-less raw_request["reporting_webhook"] -> legacy_auth_mode returns LEGACY_BEARER -> build_webhook_sender takes the from_bearer_token arm. then_bearer_header reads env.last_delivery() (the TLS capture receiver, never env.mock["post"]) and compares against the test's own ctx value, so expected is test-owned and actual is off the wire — not circular. Spec: security.mdx @ v3.1.1 :1424 (dist/docs/3.1.1/building/by-layer/L1/) names Bearer as a legacy mode selected by the PRESENCE of the authentication block, so the scenario is not over-specified against the pin. RUN IDS (cited here, not in the conftest comment: any edit to that file voids the pair, so a citation kept there could never stay valid). unmutated innet_220826_0446 exit=0 bdd_e2e 545p 16xp 2033xf 2s /2596 node PASSED mutated innet_220826_0456 exit=1 bdd_e2e 544p 1F 16xp 2033xf 2s /2596 sole failure is this node full gate innet_220826_0506 exit=0 all 7 suites green (unit 6929p, bdd_inprocess 2269p, bdd_e2e 545p, integration 2670p, e2e 145p, admin 106p, ui 5p) Every report's `created` epoch was checked against its run window. The 544p/17xp -> 545p/16xp transition, totals conserved at 2596, is the graduation signature: the node moved XPASS -> PASS and nothing else moved. Two weaknesses RECORDED IN THE CONFTEST COMMENT, not fixed — widening scope mid-graduation is how a row gets strengthened into passing: * the value assertion is conditional (`if expected_token:`), so it would silently degrade to presence-only if a Given ever stopped setting the token. The branch is live today. * unlike its 9421 twin the scenario asserts no negative, so nothing here would catch a delivery carrying Authorization AND a 9421 Signature, which :1425 forbids. EXPECTED_WEBHOOK_INTERNAL_TAGS in tests/unit/test_e2e_rest_ledger_state.py is mirrored in this same change — that lock failed the first hmac gate for exactly this reason. 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.
Draft. Tracks #1291.
What this delivers
RFC 9421 message signing, end to end: we verify inbound signatures, sign everything we emit, publish a trust root a counterparty can walk, and declare capability postures that match what we actually enforce.
Four outcomes, each checkable by a third party rather than asserted internally:
required_foroperation is refused, over real HTTPSpec grounding
Pinned version AdCP 3.1.1 (
adcp==6.6.0); guardtests/unit/test_adcp_spec_version.py.dist/schemas/3.1.1/bundled/protocol/get-adcp-capabilities-response.json→#/properties/request_signing: "Optional in 3.0 — capability-advertised so counterparties can opt into signing selectively."signed_requests(dist/compliance/3.1.1/universal/signed-requests.yaml) against 40 conformance vectors (12 positive + 28 negative) atdist/compliance/3.1.1/test-vectors/request-signing/, plus the coordination contracttest-kits/signed-requests-runner.yaml. Negative vectors must return401withWWW-Authenticate: Signature error="<code>"matching byte-for-byte.identity.brand_json_urlis load-bearing — schema-mandated the moment any signing posture is declared, which is why trust-root publication is in scope rather than optional.Two upstream spec defects were found and fixed while grounding this: adcontextprotocol/adcp#6071 (vector count) and #6076 (seven documented error codes that do not exist), plus #6075 on the underlying docs/spec drift.
Architecture
src/core/signing/owns the signing contract. Callers depend on it and on nothing beneath it — where the SDK is correct it delegates, where the SDK is wrong it carries the upstream fix verbatim with per-unit provenance pins. A structural guard forbidsadcp.signingimports outside the layer, allowlist empty, mutation-verified. Consequence: a future SDK bump deletes copied functions and changes no caller.Four SDK conformance bugs were found by grading against the real vectors and filed upstream — adcp-client-python #976 / #977 / #978 / #979, all fixed there — along with #975/#980 (the SDK's own vendored vector set was incomplete and its loader could not detect it).
Verification
Full in-network suite: 12,652 passed / 0 failed across unit, integration, bdd_inprocess, bdd_e2e, e2e, admin and ui — run
innet_210826_1308(unit 6916 · integration 2668 · bdd_inprocess 2269 · bdd_e2e 543 · e2e 145 · admin 106 · ui 5). The earlier figure in this section (12,091) came from a superseded run and was not re-derived; it is replaced rather than relabelled.Ratchets all tightened, none grew: duplication 74→73, mypy-untyped-defs 212→211, C901 183→182. These are this PR's ORIGINAL span, and two of them are in a format the tree no longer uses — the duplication baseline has since been split per-scope (
src/tests/scripts), so "73" has no current counterpart. The truth-pass table below measures the LATER span (this epic's own before/after) at the final head. Both are true; they are different spans and, for duplication, different units.mypy-untyped-defs 212→211andC901 183→182are the same numbers seen from the two ends.Signing behaviour is graded over real HTTPS against the live stack — a signature is produced by the server, the buyer's discovery chain is walked from served documents only (capabilities →
brand_json_url→ brand.json → JWKS), and the signature is verified from bytes fetched over the socket. Negative controls included: samekid, swapped key material must fail at the crypto step rather than at key lookup.Known open, tracked
negative/016-replayed-nonce). This branch rebases onto #1802 and re-derives its destination-policy decision there.z6nr.15z6nr.21Deferred with rationale: KMS
SigningProvider, thecanonical.pyseam deletion (fires on a future SDK bump, #1794), and the fabrication siblings found alongsidepiyo(#1845 and three beads).Truth pass (measured at
daabef6c3, not carried from notes)This section is measured at
daabef6c3and goes stale on every push. It has already aged twice in exactly one way — an earlier revision cited137b0e53dwhile one row (the dark-primitives allowlist) was only true ata7de571b3, mixing two measurement points under one sha; a later one citeda7de571b3after D1 had changed the tree beneath it. Any further push must re-measure the WHOLE table, not patch the row that moved. A table that silently ages is worse than one that states its own expiry.Every claim below was re-derived from the tree at this head. Where an earlier note turned out already-true or stale, that is said rather than restated.
Scope. #1291 is explicitly inbound-only; this PR also reaches into the outbound webhook boundary. That was a deliberate keep, not drift: removing it would leave UC-010 rows advertising webhook signing with the enforcement gone — a direct O4 violation — and the extraction is semantic-merge-sized on a stacked branch for near-zero review benefit. A reviewer's frame should include outbound; #1291's text does not yet say so.
xpass posture. 23 xpasses as of
innet_210826_1308(18bdd_e2e, 5bdd_inprocess) — not a graduation backlog to wave through. The mechanism matters:Thensteps readingenv.mock["post"].call_countare transport-blind, andhttpx.MockTransportmakes the logs look like real retries, so a vacuous pass and a real one are not distinguishable from the count alone. Graduation is tracked separately (salesagent-n78j0.13) and is deliberately out of this PR's scope.Deferrals that are owned, and one that is not.
send_signed_challengeopens a plainhttpx.AsyncClientrather than the SDK's IP-pinned transport. Owned: the source states it atwebhook_sender_factory.py:580-586with two issue citations (feat: route all outbound HTTP through one SSRF-guarded seam #1802, Outbound webhook delivery bypasses the SDK's IP pinning — DNS-rebinding TOCTOU on buyer-supplied URLs #1890), a rationale (adopting it changes behaviour for every existing receiver URL), and the named residual risk — the destination is validated once at fire time, not again at socket-open. That TOCTOU is stated, not hidden.Merge hazard to carry forward. After #1802 lands, a textually clean merge can swap a pinned webhook dial for an unpinned one with every signing test still green — the tests grade signing, not which transport opened the socket.
How to read the green. CI ran on this branch for the first time on 2026-08-19; earlier heads carried only
check-pr-titleandipr-check. The 32/32 on this head is a real full-matrix result, not a long-standing one.Ratchets, stated because the charter requires it
daabef6c3check_untyped_defssrc/tests/scripts)check_fixme_citation_countC901/PLR0912/PLR0915/F841type-ignoreadmin_get_db_session/admin_session_addIMPL_SESSION_ALLOWLISTset()set()frozenset()×2 (new, empty)set()— new, and empty as ofa7de571b3; it shipped with one entry through137b0e53de2e_rest_known_failures.txt_UC004_E2E_WEBHOOK_INTERNAL_TAGSOne breach occurred and was fixed at its cause rather than baselined:
e1f7588a2tookcheck_untyped_defsto 212 becausebuild_rowdeclaredoperator: stragainst a nullable column; the signature was widened and--update-baselinewas never run.D1 landed in this PR rather than being deferred (
daabef6c3), on the owner's confirmation of his 2026-08-05 instruction. Two unvalidatedcast(BrandAgentType, …)at the boundary that decides whether a request is refused becamenarrow_agent_type(), which raisesAdCPConfigurationErroron an unresolvable value — thenarrow_alg/narrow_purposeshape this PR had already introduced two files away and then violated. A cast is a runtime no-op, so a typo in an env-overridable setting previously reached the resolver, matched noagents[]entry, and 401'd every signed counterparty with nothing naming the cause.Worth stating because it is the argument for the change rather than a side note: the two new types caught two real defects on contact.
AnyUrl(jwks_origin)exposed astr | Nonethat mypy could not narrow through theanchoredflag — an unvalidated URL had been reaching the served capabilities document viacast(Any, …).DeclaredBucketexposed that the threeprotocol_methods_*buckets are generatedRootModel[str]wrappers, notstr | Enum. TheAnywas not hiding nothing.One prescription in that scope was unachievable and is recorded as such:
posture_for_tenant(tenant: Tenant | None)would require changing_detect_tenantinresolved_identity.py, which is identity resolution and out of scope under #1870. ATypedDictstates the two keys actually read and the raw dict is projected at the signing layer's boundary; the ORM boundary stays where #1870 will find it.