fix(docs): remove invented signing error-code table, point at the taxonomy - #6078
Conversation
There was a problem hiding this comment.
Corrects a broken normative contract: the old seven-code table graded against nothing and failed all 28 negative vectors for a correct implementer. Fix is right because the replacement subset has a definition — the codes the negative vectors grade — so it is re-derivable rather than arbitrary.
Things I checked
- Table == vector codes. Pulled
expected_outcome.error_codefrom all 28 files underdist/compliance/3.1.1/test-vectors/request-signing/negative/. 15 distinct values. The 15 table rows match them exactly — no code in the table absent from the vectors, none exercised by a vector missing from the table. - Wording faithfully lifted from
security.mdx. Each row maps to a verifier-checklist step (security.mdx:1224–1246):request_signature_params_incompleteparam list matches step 2,window_invalid"validity > 5 min" matches step 5'sexpires − created > 300 s,components_unexpectedmatches step 6's forbidden-content-digestbranch. Pages agree in name and meaning, so they can't drift. - The five "not exercised" codes named inline all exist:
request_target_uri_malformed(step 10),request_body_malformed(step 14),request_signature_revocation_stale(step 9 / security.mdx:1387),request_signature_jwks_unavailable(:1393),request_signature_jwks_untrusted(:1394). - Both cross-reference anchors resolve —
#transport-error-taxonomyand#discovering-an-agents-signing-keys-via-brand_json_url; therequest_signature_brand_*/request_signature_key_origin_*codes live in the discovery table (security.mdx:1119–1127), matching the pointer. expired_signatureclaim confirmed against vectors, not assumed —003-expired-signature,004-window-too-long, and013-expires-le-createdall expectrequest_signature_window_invalid. One window predicate, one code.- No wire surface touched.
docs/building/by-layer/L1/request-signing.mdxonly — nodist/, nostatic/schemas/source/, no vector or runner.docs/building/is not inPROTOCOL_SCOPED_PATHS, so no changeset is required (MUST FIX #6 scopes todocs/reference/, notdocs/building/).
Follow-ups (non-blocking — already filed)
- The hardcoded
compliance/cache/3.0.0/path and the--vectorflag that may not exist in the SDK CLI (#6077). Correct call to defer rather than guess the replacement — a cosmetic version bump would have hidden the flag question. - The structural gap: no linter validates an error code named in prose against the taxonomy or vectors, which is why the wrong table survived (#6075). Until that closes, a wrong code in prose reads identically to a right one.
The 025-jwk-alg-crv-mismatch row is broader than its current security.mdx sentence, by the vector's own $comment — widening the row to match graded behavior is correct, and the note that security.mdx's row lags is the right place to leave it.
LGTM. Follow-ups noted below.
…onomy The `### Error codes` table listed seven code names — `missing_signature`, `invalid_signature`, `expired_signature`, `replayed_nonce`, `revoked_key`, `unknown_key`, `unsupported_algorithm` — that appear nowhere in the 3.1.1 compliance tree. The real codes are all `request_signature_*`. `universal/signed-requests.yaml` grades the code byte-for-byte against each vector's `expected_outcome.error_code`, so an implementer following this page fails all 28 negative vectors while correctly implementing what the docs told them: right status, right header, right rejection reason, wrong string. Delete the table rather than correct it. It was a hand-made summary of an enum whose canonical table — `security.mdx#transport-error-taxonomy`, 18 `request_signature_*` codes plus `request_target_uri_malformed` and `request_body_malformed`, each with retry semantics — the page already linked on the very next line. Re-typing that enum with the right names would recreate the second hand-maintained copy that drifts the moment a code is added, which is how this went wrong the first time. Keep the framing sentence, which is normative and correct. Add the byte-for-byte grading rule, which belongs in a Testing section and is stated nowhere else on the page. Extend the pointer to cover the discovery-phase `request_signature_brand_*` / `request_signature_key_origin_*` codes, which the single previous link did not reach. Docs prose only. No schema, vector, runner, or generated artifact changes.
084f95d to
ffa9194
Compare
|
Revised the approach — force-pushed. @aao-secretariat, your approval above was against the previous version and no longer covers what is on the branch. Previously this PR replaced the seven wrong codes with a corrected 15-row table scoped to "the codes the negative vectors grade". It now deletes the table and lets the existing link carry the codes. The argument that changed it: the line immediately below the table already pointed at Net is now −12/+4. Kept: the framing sentence, the byte-for-byte grading rule (owned by Re-review appreciated. |
There was a problem hiding this comment.
Deleting a hand-maintained enum that contradicted the canonical taxonomy on the very next line is the right fix, not retyping it. The only copy that can't drift is the one that doesn't exist.
Things I checked
- Both pointer anchors resolve.
#transport-error-taxonomy→security.mdx:1369(#### Transport error taxonomy).#discovering-an-agents-signing-keys-via-brand_json_url→security.mdx:1096. Therequest_signature_brand_*codes live in the table atsecurity.mdx:1119-1125; therequest_signature_key_origin_*/request_signature_brand_*short-circuit is cited atsecurity.mdx:1236. Both links land on real content. - The seven names were phantom. Swept
docs/,static/schemas/source/,static/compliance/source/—missing_signature,expired_signature,replayed_nonce,revoked_key,unknown_key,unsupported_algorithmappear only in the deleted table (request-signing.mdx:734-740). The revocation vector017-key-revoked.jsongradesrequest_signature_*, notrevoked_key— confirming the table described a protocol the conformance suite doesn't implement. expired_signaturecollapse is correct. No separate expiry code exists; window predicates graderequest_signature_window_invalid, consistent with the removal.- No wire surface touched. Single file, net −8, prose only. No
dist/, no schema, no vector, no runner. Changeset MUST FIX doesn't apply —docs/building/is outside the protocol-scoped path list, and the PR'scheck-changeset-protocol-scope.cjsoutput confirms it. - The kept additions can't drift. The byte-for-byte grading rule and the second
brand_json_urlpointer are both non-enumerative — neither restates a code list, so neither reintroduces the anti-pattern being removed.
Follow-ups (non-blocking — file as issues)
- The hardcoded
compliance/cache/3.0.0/atrequest-signing.mdx:711,719is correctly deferred to #6077 — this repo'sgrading.mdxdocumentsverify-vectorreading from stdin with no--vectorflag, so the snippet's shape is unresolvable from the diff. Leaving it rather than guessing at a half-fix is the right call. - Adjacent count drift (
39/27vs the 28 negative files) sits in #6074's territory, not this PR's. Noted, not owned here.
Clean deletion of a load-bearing-but-wrong summary, replaced with a pointer at the source of truth. The revision from a corrected-table to a delete is the stronger version of the fix — a definition makes a table verifiable, not self-updating.
Approving on the strength of the verified anchors plus the confirmed phantom sweep.
Fixes #6076. Instance 3 of #6075.
The defect
### Error codeson the primary L1 request-signing page listed seven code names —missing_signature,invalid_signature,expired_signature,replayed_nonce,revoked_key,unknown_key,unsupported_algorithm. None of them exists anywhere in the 3.1.1 compliance tree.universal/signed-requests.yamlgrades the code byte-for-byte against each vector'sexpected_outcome.error_code. So an implementer following this page fails all 28 negative vectors while having correctly implemented what the docs told them — right status, right header, right rejection reason, wrong string. The docs and the conformance suite describe two different protocols.Why delete rather than correct
The line immediately below the table already read:
So the table was a hand-made summary that both duplicated and contradicted a canonical table the page linked on the very next line.
security.mdx#transport-error-taxonomycarries 18request_signature_*codes plusrequest_target_uri_malformedandrequest_body_malformed, each with retry semantics, and theWWW-Authenticateheader rules including the no-realmconstraint — none of which a seven-row summary can carry.Retyping that enum with the right names would fix today's symptom and rebuild the mechanism. A corrected table is still a second hand-maintained copy of an 18-row enum; it goes stale the moment a code is added, which is exactly how this one went wrong. That is the anti-pattern filed as #6075 — docs restating machine-readable spec facts by hand with nothing checking they agree — and the fix for an instance of it should not be a fresh instance of it.
I initially wrote a corrected 15-row table scoped to "the codes the negative vectors grade", on the theory that a subset with a definition is re-derivable rather than arbitrary. That was wrong, and this PR was revised. A definition makes a table verifiable, not self-updating — a 29th negative vector introducing a new code staleness it just the same. The only copy that cannot drift is the one that does not exist.
Reviewers: @aao-secretariat's approval above predates this revision and was given against the corrected-table version. Re-review appreciated.
What is kept
The framing sentence, which is normative and correct.
Two additions, both non-enumerative, so neither can drift the way a code list does:
universal/signed-requests.yaml, stated nowhere on this page, and precisely the fact a## Testingsection should carry — it tells an implementer why the exact string matters, which is what the old table failed to convey while getting the strings wrong.brand_json_urldiscovery section. The discovery-phaserequest_signature_brand_*andrequest_signature_key_origin_*codes live in a differentsecurity.mdxsection that the single previous link did not reach.Net −12/+4.
Establishing the canonical set
Even though nothing is transcribed, the sources were reconciled to confirm the pointer aims at a complete list:
security.mdxis the only complete list — 27 concreterequest_signature_*codes across#transport-error-taxonomyand thebrand_json_urldiscovery table. Both are now linked.dist/compliance/3.1.1/test-vectors/request-signing/negative/carry 15 distinct codes, a strict subset of the 27. Verified programmatically; no disagreement in either direction.dist/schemas/3.1.1/enums/error-code.jsonhas 92 values and zerorequest_signature_*.That last one looks like a conflict but is not: these are HTTP transport-layer codes carried in
WWW-Authenticate, not AdCP protocol error codes, so their absence from the protocol enum is correct rather than drift. The only schema naming any of them isget-adcp-capabilities-response.json, in prose, for 3.On the mapping specifically:
expired_signaturewas confirmed against the vectors, not assumed —003-expired-signature,004-window-too-long, and013-expires-le-createdall expectrequest_signature_window_invalid. One window predicate, one code; there is no separate "expired" code.Why pointing beats summarizing here, concretely
security.mdxdefinesrequest_target_uri_malformedas covering empty authority, bare IPv6, IPv6 zone identifier, and raw non-ASCII host. There are open conformance bugs against the Python SDK for not implementing exactly that (adcontextprotocol/adcp-client-python#977, #978). The spec is clear and an implementation still diverged — so implementers are already getting the taxonomy wrong from an authoritative source. A summary on a second page competing with that source makes it worse, not better. Note also thatrequest_target_uri_malformedis one of the codes no seven-row summary would ever have included.Not fixed here — the
compliance/cache/3.0.0/path### Conformance vectorshardcodescompliance/cache/3.0.0/at lines 711 and 719, the second as a copy-pasteable--vectorargument. That path is the SDK's bundled cache whose version segment is read fromnode_modules/@adcp/sdk/ADCP_VERSION(seescripts/overlay-compliance-cache.sh) — a moving path frozen as a literal.I could not establish the correct form from this repo, so I left it rather than guess. This repo's own CLI reference contradicts the snippet:
docs/building/verification/grading.mdxdocumentssigning verify-vectoras reading a vector from stdin, with no--vectorflag, referring to the set by its published URL/compliance/latest/test-vectors/request-signing/rather than anode_modulespath. The CLI ships in@adcp/sdk, so which form is right is not decidable here. Correcting only the version string would leave a command whose flag may not exist — a cosmetic fix that makes a broken snippet look maintained. Filed as #6077. It also sits on the one line #6074 edits, so deferring keeps both PRs conflict-free.Sweep
Swept all non-
dist/prose and source for the seven names. The wrong table existed in exactly one place — all seven names appeared once each, together, in it.The only other hit anywhere is
tests/billing/webhook-security.test.ts:47,const invalidSignature = 'invalid_signature'— a Stripe billing-webhook test mock, unrelated domain, deliberately unchanged.No other invented signing codes exist. Cross-checked both canonical taxonomies against all 55 negative test-vector files under
static/compliance/source/test-vectors/{request-signing,webhook-signing}/— everyerror_codematches one of them byte-for-byte. The webhook taxonomy'swebhook_*prefix is a documented deliberate parallel, not drift. Every otherSignature error=site in the repo uses a<code>placeholder or${err.code}from the SDK, so none could carry a wrong name.Relationship to #6074
Separate PR by design. #6074 corrects stale counts in the adjacent subsection and is approved and green; this corrects the normative contract and deserves its own review rather than riding inside a counts PR.
git merge-treereports a clean merge in either order, so neither blocks the other. #6073 untouched, #6071 left open.Verification
git diff origin/main --name-only→docs/building/by-layer/L1/request-signing.mdxonly. Nodist/path touched; no schema, vector, runner, or generated artifact changed.#transport-error-taxonomywas already used by the line replaced, andsecurity.mdxself-links#discovering-an-agents-signing-keys-via-brand_json_url.Changeset: not required
docs/building/is not inPROTOCOL_SCOPED_PATHS, sochangeset-check.ymlskips the changesets status step. #6074 needed one because it also touchedstatic/compliance/source/, which is protocol-scoped; this is docs prose only.Why nothing caught this
scripts/lint-error-codes.cjsandscripts/lint-error-code-drift.cjsscanstatic/compliance/source/andstatic/schemas/source/enums/error-code.json. Neither scansdocs/. No check validates an error code named in prose against anything — the structural gap #6075 proposes closing. Deleting the copy is the version of that fix available today: prose that points cannot drift, so there is nothing left for a linter to check here.