Skip to content

test(signing): vendor the complete AdCP 3.1.8 request-signing vector set and pin it against drift - #980

Merged
bokelley merged 6 commits into
adcontextprotocol:mainfrom
KonstantinMirin:fix/vendor-complete-signing-vectors
Jul 29, 2026
Merged

test(signing): vendor the complete AdCP 3.1.8 request-signing vector set and pin it against drift#980
bokelley merged 6 commits into
adcontextprotocol:mainfrom
KonstantinMirin:fix/vendor-complete-signing-vectors

Conversation

@KonstantinMirin

@KonstantinMirin KonstantinMirin commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Fixes #975.

What was wrong

The vendored request-signing conformance vectors were an incomplete copy of the spec set, and the loaders could not tell. Both graders globbed the vector directory and asserted only that the result was non-empty (test_verifier_vectors.py:77-78, test_canonicalization.py:24-31), so any non-empty subset passed and a missing vector was indistinguishable from a complete set.

Against AdCP 3.1.8 — the version src/adcp/ADCP_VERSION already targets — the vendored copy was missing 4 of 12 positive vectors, 8 of 28 negative vectors, and canonicalization.json entirely. The vendored README.md documents canonicalization.json in its file-layout tree and devotes a section to it; the file had never been copied in.

Three further vectors (016, 017, 020) had silently drifted from upstream. 017 is the consequential one: it was vendored at a revision that revokes test-ed25519-2026, the primary signing key, rather than the dedicated test-revoked-2026 keypair the spec now uses — and keys.json had never been refreshed to carry that key.

What this PR does

Vendors the complete set. All 12 missing files plus canonicalization.json, and re-syncs the three drifted vectors, keys.json and README.md. The tree is now byte-identical to dist/compliance/3.1.8/test-vectors/request-signing/ at spec tag v3.1.8 — verified file-by-file in both directions.

Replaces the non-empty assertion with a completeness pin. This is the actual fix; adding 12 files without it would only reset the clock.

  • tests/conformance/signing/vectors.py — one shared loader, replacing the two per-module glob helpers. Parametrization is gated on the pin, so a missing vector fails at collection time rather than quietly shrinking the graded set.
  • vector_manifest.json — SHA-256 of every vendored file. Fails on a missing file, an unexpected extra file, and on content drift. The pin is over content rather than filenames precisely because a filename-only manifest would not have caught what happened to 016/017/020. Regenerate with python -m tests.conformance.signing.vectors --write.
  • test_vector_completeness.py — grades the pin, and ties it to get_adcp_spec_version() so bumping the targeted spec version without re-vendoring fails loudly instead of leaving the SDK graded against a stale contract. Also checks per-directory counts, contiguous vector numbering, and that every jwks_ref kid resolves in keys.json (that last one is what the stale 017 would have tripped).

Wires in canonicalization.json. It earns its own grader rather than being redundant with the per-vector expected_signature_base path: it exercises canonicalize_target_uri / canonicalize_authority over 31 cases, including 6 malformed-authority rejections that no signed vector covers at all. The spec's README calls it "the fastest way to surface cross-implementation divergence".

The new vectors surface 12 real conformance failures

As expected, grading the previously-ungraded vectors turns the suite red. Nothing was weakened to get green — the failures are marked strict-xfail with the issue each one tracks, so a fix XPASSes and forces the entry to be retired rather than leaving a stale exemption behind.

They are filed separately rather than fixed here, since each is a production behavior change in a security-critical verifier and several change error codes that consumers may key on:

Verification

  • Vendored tree confirmed byte-identical to spec v3.1.8, both directions (nothing missing, nothing extra).
  • Signing conformance suite: 459 passed, 9 skipped, 12 xfailed.
  • Full suite: 5911 passed, 41 skipped, 13 xfailed, 0 failed.
  • ruff check src/ and black clean; mypy on the changed files introduces no new errors (the 3 remaining bare-dict warnings in test_verifier_vectors.py pre-date this branch, and make typecheck covers src/adcp/ only).

The completeness check was mutation-verified — a guard that cannot fail is the same defect in a new costume, and this issue exists because an assertion that could not fail was trusted. Each mutation was applied, confirmed red, and reverted:

mutation result
delete negative/026-non-ascii-host.json collection error, naming the missing file
delete canonicalization.json collection error
edit a vector in place (content drift) 1 failed — vendored vectors differ from their pinned content
add an unexpected negative/029-not-in-spec.json collection error
add a ledger entry for a passing vector 1 failed (strict xfail → XPASS), confirming the ledger self-retires

No production code is touched; this change is vectors plus the guard that keeps them complete.

Context

Found while adopting adcp.signing in a seller implementation that delegates canonicalization entirely to the SDK. Because the SDK owns canonicalization on behalf of its consumers, these gaps were untested in every downstream implementation too, not just in this suite.

aao-ipr-bot[bot]
aao-ipr-bot Bot previously approved these changes Jul 28, 2026

@aao-ipr-bot aao-ipr-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vendors the missing 13 files and, more importantly, replaces the non-empty assertion with a content-hash completeness pin — the guard is the fix, the vectors are just the payload. Right shape: a conformance suite that grades "whatever is on disk" is graded against an attacker-chosen subset, and fail-closed at collection time beats fail-open at runtime.

Things I checked

  • Test-only. All 23 files under tests/; no source under src/adcp/ touched. Triage: skip all experts.
  • Version pin is live, not decorative. src/adcp/ADCP_VERSION is 3.1.1, get_adcp_spec_version() (src/adcp/__init__.py:684) reads it, and test_manifest_pins_the_spec_version_the_sdk_targets ties the two — a spec bump without a re-vendor fails loudly instead of grading against a stale contract.
  • Collection-time gating is load-bearing. load_vector_set (tests/conformance/signing/vectors.py:40-64) raises on missing or unexpected filenames, so parametrization can't silently shrink — a dropped vector takes the suite red at collection, not quietly out of the graded set. The manifest (test_vendored_tree_matches_manifest_exactly) then closes the third direction with content hashes, which is what caught the drifted 016/017/020 a filename-only pin would have missed.
  • Public API is real. canonicalize_target_uri / canonicalize_authority exist in src/adcp/signing/canonical.py:81,94; the new imports resolve.
  • Strict-xfail direction is correct. test_canonicalization_case asserts pytest.raises(ValueError) for the 5 malformed-authority reject cases in KNOWN_CANONICALIZATION_GAPS — SDK currently accepts them, so the raise doesn't fire, the test fails, and the strict xfail passes; a fix XPASSes and forces retirement. Same shape for the four KNOWN_VERIFIER_GAPS. No exemption outlives its bug.
  • Test-plan honesty: verification section is complete and mutation-verified (delete a vector → collection error; edit in place → content-drift failure; add a passing vector to a ledger → XPASS). The guard is demonstrably falsifiable, which is the whole point given how this started.

Follow-ups (non-blocking — file as issues)

  • The 12 conformance failures (#976#979) are real production behavior in a security-critical verifier — no step-1 strict-parse stage, canonical.py never calling the canonicalize_host() the rest of adcp.signing already wires in, trailing empty query dropped. Correctly filed separately rather than smuggled into a "test" PR. The 021 duplicate-sig1-label case resolving to the second label is the one worth prioritizing — it's covered-component smuggling, not a cosmetic code mismatch.

Minor nits (non-blocking)

  1. _MANIFEST mutation guard. _write_manifest does dict(_MANIFEST) (shallow) then reassigns files, so the module-level _MANIFEST["files"] is safe — but only by construction. A comment there would keep a future edit from aliasing it. tests/conformance/signing/vectors.py:82.

An assertion that could not fail was trusted for 12 vectors; the response is a guard that was made to fail five different ways before being trusted. That is the correct lesson to draw. LGTM.

@KonstantinMirin

Copy link
Copy Markdown
Collaborator Author

Thanks for the review — addressed the _write_manifest nit in ac1e9a2 with a comment spelling out that the shallow copy must replace files rather than mutate it, since _MANIFEST["files"] is what VECTOR_MANIFEST is bound to and mutating it in place would rewrite the pin the tests grade against.

Also dropped a private _pytest.mark.ParameterSet import I'd used for the parametrization type hints (f062247) — it was the only _pytest import in the repo and bought nothing, since make typecheck covers src/adcp/ only.

Re-verified after both: signing conformance suite 459 passed / 12 xfailed, full suite 5911 passed / 0 failed, and python -m tests.conformance.signing.vectors --write regenerates the manifest byte-identically.

Agreed on prioritising #976 — the 021 duplicate-sig1 case is the one with real teeth. It isn't just a code mismatch: the verifier resolves the duplicate to the second label, which covers fewer components, so the observable failure is request_signature_components_incomplete: required covered component missing: @authority. That's the smuggling path working end to end, and the wrong-code symptom is what makes it easy to misread as a benign parse difference.

@aao-ipr-bot aao-ipr-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean fix, and the right shape: the defect here was a guard that could not fail, so the fix is a guard that fails closed on the exact thing that drifted. Pinning content hashes rather than filenames is the load-bearing decision — a filename-only manifest would have sailed past what happened to 016/017/020.

Triage: test-only (23 files, all under tests/, no source touched) — skips all experts per the skip-everything list. Verified by hand instead.

Things I checked

  • No production surface moves. Every changed path is under tests/conformance/. adcp.signing.canonical and the verifier are imported, not modified; get_adcp_spec_version() is consumed, not changed. Nothing on the adcp.* wire changes, so test(signing): is the correct conventional-commit scope.
  • Completeness pin actually fails closed. load_vector_set (vectors.py:107) raises AssertionError on missing or unexpected, and it's called inside the @pytest.mark.parametrize(...) argument — so a dropped vector is a collection error, not a silently smaller graded set. That's the whole point of the PR and it lands.
  • Content-drift arm is real. on_disk_files() hashes over VECTORS_DIR.rglob and test_vendored_tree_matches_manifest_exactly diffs it three ways (missing / unexpected / drifted). VECTORS_DIR (.../vectors/request-signing/) and MANIFEST_PATH (.../signing/) are different directories, so the manifest doesn't pin itself — no self-reference footgun.
  • Manifest matches the vendored tree. 12 positive + 28 negative + README.md + keys.json + canonicalization.json = 43 entries. Counts, contiguous numbering (1..12, 1..28), and the spec-version tie to get_adcp_spec_version() (3.1.1, confirmed against src/adcp/ADCP_VERSION) all line up.
  • canonicalization.json is internally consistent with its grader. Fetched the vendored file: 31 cases, 6 reject. KNOWN_CANONICALIZATION_GAPS xfails 5 of the 6 malformed-authority cases; the sixth (malformed-ipv6-missing-closing-bracket) is not xfailed, which means the SDK must reject it today — consistent with test_canonicalization_case asserting pytest.raises(ValueError) on the non-gap reject path.
  • strict-xfail ledgers are honest. Both KNOWN_VERIFIER_GAPS and KNOWN_CANONICALIZATION_GAPS are strict=True with an issue number per entry, so a fix XPASSes and forces retirement rather than leaving a stale exemption. Every referenced issue (#976#979) is cited with the specific error-code or canonicalization gap it tracks.
  • Test-plan honesty: the mutation table is the part I'd normally flag as missing. Five mutations applied, each confirmed red and reverted — including a passing-vector ledger entry to prove the strict-xfail self-retires. Full suite reported 5911 passed / 0 failed. Nothing was weakened to get green; the new failures are xfailed against filed issues.

Follow-ups (non-blocking — file as issues)

  • The 12 surfaced conformance failures (#976#979) are the actual work — several change verifier error codes consumers may key on (021 duplicate-sig1 smuggling is the sharp one). Filed separately, correctly kept out of this PR since they're production behavior changes in a security-critical verifier.

Minor nits (non-blocking)

  1. --write doesn't refresh spec_version/source. _write_manifest does dict(_MANIFEST) and only rewrites files (vectors.py:143), so on a spec bump the documented "update ADCP_VERSION, then regenerate the pin" leaves spec_version stale until hand-edited. test_manifest_pins_the_spec_version_the_sdk_targets catches it loudly, which is the intended failure — but the re-pinning docstring could say the version line is a hand-edit, not a regen output, so nobody chases a red bar expecting --write to have fixed it.

LGTM. Follow-ups noted below.

aao-ipr-bot[bot]
aao-ipr-bot Bot previously approved these changes Jul 28, 2026

@aao-ipr-bot aao-ipr-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean fix. The bug was a guard that could not fail — a glob asserting only non-empty — and the fix replaces a hollow invariant with a content pin, which is the right shape: a filename-only manifest would not have caught what happened to 016/017/020.

Things I checked

  • Collection-time gate is load-bearing. load_vector_set (vectors.py:438-461) raises an explicit AssertionError — not a bare assert — so the graded set fails to collect under python -O too, rather than silently shrinking. The parametrize decorators in test_verifier_vectors.py:296 and test_canonicalization.py:79 are gated on it, so a missing vector goes red at collection, not by quietly grading fewer cases. That is the actual defect from #975 closed.
  • The pin is over content, not names. vector_manifest.json carries SHA-256 of all 42 entries; test_vendored_tree_matches_manifest_exactly splits missing / unexpected / drifted (test_vector_completeness.py:144-170). Drift detection is what catches the 016/017/020 re-sync class.
  • 017 is coherent end-to-end. keys.json:803-816 adds the dedicated test-revoked-2026 keypair (adcp_use: request-signing so step-8 passes and step-9 fires), and negative/017-key-revoked.json repoints jwks_ref and revocation_list.revoked_kids off the primary test-ed25519-2026 onto it. test_every_key_referenced_by_a_vector_exists_in_keys_json (test_vector_completeness.py:213-221) is the grader that would have tripped the stale state — good that it exists.
  • Nothing was weakened to get green. The 12 conformance failures are strict-xfail with issue citations (test_verifier_vectors.py:252-267, test_canonicalization.py:29-45), so a fix XPASSes and forces retirement. canonicalization.json carries 6 reject cases; exactly 5 are in KNOWN_CANONICALIZATION_GAPSmalformed-ipv6-missing-closing-bracket (canonicalization.json:767-773) is absent from the gap list because the SDK already rejects an unclosed bracket. Consistent, not a gap in the gap list.
  • Spec-version tie. test_manifest_pins_the_spec_version_the_sdk_targets binds the pin to get_adcp_spec_version(), so a spec bump without re-vendoring fails loudly instead of grading against a stale contract.
  • _write_manifest re-pin path replaces document[\"files\"] rather than mutating _MANIFEST[\"files\"] (vectors.py:470-477) — VECTOR_MANIFEST is bound to that dict, so mutating in place would rewrite the pin the tests grade against. The comment says exactly this.
  • Conventional-commit scope test(signing): is correct — no production code touched, no semver signal owed.

Minor nits (non-blocking)

  1. canonicalization.json self-declares \"version\": \"3.0\" (canonicalization.json:555) while everything else pins 3.1.1. It's vendored verbatim so the hash pin is honest and this is upstream's field, not yours — flagging only so a future reader doesn't mistake it for a re-vendor miss.

One dry note: the completeness check was itself mutation-tested. The guard now has a guard — which is the correct response to a bug whose root cause was trusting an assertion nobody had ever watched fail.

LGTM.

@KonstantinMirin

Copy link
Copy Markdown
Collaborator Author

CI note: 19/20 checks pass. The one red check, AdCP storyboard runner — v3 reference seller (translator), is not caused by this PR — it is broken on main. Filed as #981.

I checked rather than assumed, because it did fail three times running here and other open PRs show green on that job:

So the boot regression landed between 07-27 19:24Z (last green) and 07-28 20:35Z, in a window with no CI runs. Details and a suggestion for making the step self-diagnosing (it currently discards the seller's output on timeout, which is why the log shows no cause) are in #981.

Everything this PR actually exercises is green: all four Python versions, Postgres conformance, schema validation, downstream import smoke, and the other five storyboard runners.

aao-ipr-bot[bot]
aao-ipr-bot Bot previously approved these changes Jul 29, 2026

@aao-ipr-bot aao-ipr-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Fixes a fail-open guard by making the vendored vector set the thing under test, not just the graded input — a missing vector now fails at collection instead of passing as a non-empty subset.

Things I checked

  • The actual fix. load_vector_set (vectors.py) gates parametrization on the manifest and raises with named missing/unexpected files, replacing the old assert result non-empty check in test_verifier_vectors.py and test_canonicalization.py. A dropped vector takes the suite red at collection, not silently shrinks the graded set.
  • Content pin, not filename pin. vector_manifest.json pins SHA-256 per file; test_vendored_tree_matches_manifest_exactly fails on missing, extra, and drift. This is what catches 016/017/020 — a filename-only manifest would not have. Right shape.
  • The reject-branch fix (commit 7f72e87). test_canonicalization_case now asserts excinfo.value.code == case["expected_error_code"], not a bare pytest.raises(ValueError). Real catch: https://[::1/p is refused inside urlsplit() with a code-less ValueError, so the bare assertion passed on someone else's exception while five sibling cases sat in the ledger. All 6 malformed-authority cases (malformed-port-without-hostmalformed-ipv6-zone-identifier, canonicalization.json:755-808) now xfail together under #978 and retire together.
  • strict-xfail self-retires. Every gap is xfail(strict=True) mapped to its issue (#976#979) in KNOWN_CANONICALIZATION_GAPS / KNOWN_VERIFIER_GAPS; a fix XPASSes and forces the entry out. The mutation table in the PR body verifies the guard can actually fail in all five directions — that is the part I'd normally ask for and rarely get.
  • Counts + numbering pinned literally: 12 positive, 28 negative, 31 canonicalization (6 reject / 25 non-reject), contiguous 001..NNN. test_every_key_referenced_by_a_vector_exists_in_keys_json ties jwks_ref kids to keys.json — the check the stale 017 would have tripped, now that test-revoked-2026 is vendored in.
  • No production surface. Nothing under src/adcp/ changed; no public export, signature, or Pydantic shape touched. test(signing): prefix is correct — no semver signal owed.
  • Private _pytest.mark.ParameterSet import removed (commit b9de10d); both param builders return list[Any]. No private pytest internals on the import path.

Follow-ups (non-blocking — file as issues)

  • The 12 newly-graded failures (#976#979) are real verifier gaps in a security-critical path, not test noise. Filing them separately is the right call — several change error codes consumers key on — but #976 (021 duplicate sig1 label silently resolving to the second, covering fewer components) and #977 (no UTS-46 A-label conversion; canonical.py never calls the canonicalize_host() the SDK already ships) are the wire-security ones and worth prioritizing over the malformed-authority set.

Minor nits (non-blocking)

  1. Fixture-internal version tag diverges from the pin. canonicalization.json:658 carries "version": "3.0" / "spec_reference": "#adcp-rfc-9421-profile" while the manifest pins spec_version: 3.1.1. The file is vendored verbatim and hash-pinned, so this is upstream's field, not yours to edit here — worth a note upstream that the fixture's own version tag lags the release it ships under.

LGTM. Follow-ups noted below.

@bokelley bokelley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current main now targets AdCP 3.1.8 (#992), but this branch pins VECTOR_SET_SPEC_VERSION to 3.1.1. I merged the full stack into current main and ran tests/conformance/signing: 482 passed, 1 failed at test_manifest_pins_the_spec_version_the_sdk_targets (3.1.8 != 3.1.1).\n\nPlease rebase onto current main and update vector_manifest.json's spec_version/source to 3.1.8. I compared dist/compliance/3.1.1/test-vectors/request-signing with 3.1.8 at tag v3.1.8; the trees are byte-identical, so the file hashes/payload do not need to change.

…nd pin it

The vendored request-signing conformance vectors were an incomplete copy of
the spec set, and the loaders could not tell. Both graders globbed the vector
directory and asserted only that the result was non-empty
(test_verifier_vectors.py, test_canonicalization.py), so any non-empty subset
passed and a missing vector was indistinguishable from a complete set.

Against AdCP 3.1.1 -- the version src/adcp/ADCP_VERSION already targets -- the
vendored copy was missing:

  - 4 of 12 positive vectors (009/010 percent-encoding edges, 011/012 IPv6
    authority)
  - 8 of 28 negative vectors (021-028)
  - canonicalization.json entirely, despite the vendored README documenting
    it and devoting a section to it

Three further vectors (016, 017, 020) had silently drifted from their upstream
contents. 017 is the notable one: it had been vendored at a revision that
revoked test-ed25519-2026, the primary signing key, rather than the dedicated
test-revoked-2026 keypair the spec now uses -- and keys.json had never been
refreshed to carry that key.

Changes:

  - Vendor all 12 missing files plus canonicalization.json, and re-sync the
    three drifted vectors, keys.json and README.md. The tree is now
    byte-identical to dist/compliance/3.1.1/test-vectors/request-signing/.
  - Add tests/conformance/signing/vectors.py: one shared loader whose
    parametrization is gated on a completeness pin, replacing the per-module
    glob-and-assert-non-empty helpers.
  - Add vector_manifest.json, pinning the SHA-256 of every vendored file. It
    fails on a missing file, an unexpected extra file, and on content drift.
    A filename-only manifest would not have caught what happened to 016/017/020,
    so the pin is over content, not names.
  - Add test_vector_completeness.py, which also ties the pin to
    get_adcp_spec_version(), so bumping the targeted spec version without
    re-vendoring fails loudly instead of leaving the SDK graded against a
    stale contract.
  - Wire canonicalization.json into test_canonicalization.py. It earns its own
    grader: it exercises canonicalize_target_uri/canonicalize_authority over
    31 cases including 6 malformed-authority rejections that no signed vector
    covers at all.

The newly graded vectors surface 12 real conformance failures, marked
strict-xfail with the gap each one tracks so a fix XPASSes and forces the
entry to be retired. They fall into two groups:

  - The verifier has no step-1 strict-parse stage, so malformed or ambiguous
    input flows on to a later check and is rejected under the wrong error code
    (negative 021, 022, 023, 026). Conformance grades the code byte-for-byte,
    so "rejected anyway" is not a pass.
  - @target-uri canonicalization applies no UTS-46 A-label conversion, drops a
    trailing empty query, and accepts five malformed authority shapes the spec
    requires be rejected. adcp.signing ships canonicalize_host(), but
    canonical.py never calls it.

Each group is filed separately; this change deliberately adds no production
behavior, only the vectors and the guard that keeps them complete.
The reject branch asserted a bare ValueError -- but urlsplit raises exactly
that for `https://[::1/p`, so `malformed-ipv6-missing-closing-bracket` passed
on someone else's exception while five sibling cases sat in the xfail ledger.
Six reject cases ship in the data; only five were recorded as gaps.

No case asserted `expected_error_code` at all, so the vector set could not
detect its own incompleteness -- the defect this PR exists to close.

Capture the exception and assert the code the vector ships. All six reject
cases now xfail together under adcontextprotocol#978 and retire together when the typed error
lands.
Review follow-up. main moved to 3.1.8 (adcontextprotocol#992) while this branch pinned 3.1.1,
so test_manifest_pins_the_spec_version_the_sdk_targets failed once the two met.

Only spec_version and source change: the 3.1.1 and 3.1.8
dist/compliance/.../test-vectors/request-signing trees are byte-identical, and
regenerating the manifest reproduces all 43 file hashes unchanged. That is the
pin doing its job -- it forced an explicit re-vendor decision on a spec bump
rather than letting the old set ride silently.

Note for the next bump: `python -m tests.conformance.signing.vectors --write`
regenerates `files` only and carries spec_version/source forward, so those two
fields need editing before the regen, not by it.

Refs adcontextprotocol#975.

@aao-ipr-bot aao-ipr-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. This turns a guard that could not fail into one that fails closed — a non-empty glob told you nothing, a content-hash manifest tells you exactly what drifted.

The premise is the fix, not the 12 new files. Adding vectors without the completeness pin would only reset the clock; the pin over content (not filenames) is what would have caught 016/017/020. Right shape.

Things I checked

  • Test-only diff. All 23 files under tests/; no src/adcp/ touched (gh pr diff --name-only | grep ^src/ → none). No production behavior change, so this is vectors plus the guard, as claimed.
  • The spec-version pin actually holds. test_manifest_pins_the_spec_version_the_sdk_targets asserts get_adcp_spec_version() == VECTOR_SET_SPEC_VERSION. ADCP_VERSION on main is 3.1.8 (src/adcp/ADCP_VERSION); manifest spec_version is 3.1.8. Green. The last commit re-pinned from 3.1.1→3.1.8 precisely because main moved under the branch — that is the pin doing its job, forcing an explicit re-vendor decision on a spec bump instead of riding the stale set silently.
  • Fail-at-collection, not fail-quiet. load_vector_set (tests/conformance/signing/vectors.py:452 region) raises AssertionError on any missing/unexpected filename, and it's invoked inside the @parametrize decorator, so a dropped vector takes the suite red at collection rather than shrinking the graded set. That is the whole point of #975 and it's wired correctly.
  • The _write_manifest aliasing footgun is handled. vectors.py shallow-copies _MANIFEST and replaces files rather than mutating in place — VECTOR_MANIFEST is bound to _MANIFEST["files"], and mutating it would rewrite the pin the tests grade against. Comment names the hazard. Good.
  • Reject cases assert the code, not the refusal. test_canonicalization_case captures the exception and asserts excinfo.value.code == expected_error_code. The malformed-ipv6-missing-closing-bracket note is the sharp one: urlsplit() raises a bare ValueError with no code, so pytest.raises(ValueError) alone graded nothing — five siblings sat in the ledger while one passed on someone else's exception. Now all six retire together under #978.
  • Strict xfail ledgers are self-retiring. KNOWN_VERIFIER_GAPS and KNOWN_CANONICALIZATION_GAPS are strict=True, so a verifier fix XPASSes and forces the entry out rather than leaving a stale exemption. Each is mapped to a filed issue (#976#979).
  • Fixture counts match the pin. 12 positive + 28 negative in the manifest; canonicalization.json carries 6 reject: true cases, matching the test_canonicalization_fixture_is_vendored_and_populated assertion of 6 reject / 25 non-reject.
  • Test plan is honest. Verification section is complete, and the completeness check was mutation-verified (delete/edit/add each confirmed red, then reverted). A guard proven able to fail is the right bar for an issue that exists because a guard couldn't.

Follow-ups (non-blocking — file as issues)

  • The four filed gaps (#976 duplicate Signature-Input label / wrong error code, #977 UTS-46 A-label, #978 malformed-authority rejection + request_target_uri_malformed, #979 trailing empty query) are real verifier defects in a security-critical path. #976's 021 — silently resolving a duplicate sig1 to the second label and covering fewer components — is the covered-component smuggling case the rejection rule exists to prevent. Worth prioritizing over the canonicalization niceties. Correctly filed separately since each changes an error code consumers may key on.

Minor nits (non-blocking)

  1. --write carries spec_version/source forward. _write_manifest regenerates files only, so the next bumper has to hand-edit spec_version and source before the regen, not by it. That's documented in the last commit body but not in the vectors.py re-pin docstring, which only mentions updating ADCP_VERSION. One line in the docstring would keep the two in sync.

LGTM. Follow-ups noted below.

@KonstantinMirin

Copy link
Copy Markdown
Collaborator Author

Rebased onto current main and re-pinned at 3.1.8 in d719b5c6.

Only spec_version and source changed — regenerating the manifest reproduces all 43 file hashes identically, which independently confirms your byte-identical check of the two vector trees.

Worth noting this is the pin doing exactly what it was added for. It didn't detect a content drift; it forced an explicit re-vendor decision on a spec bump instead of letting the old vector set ride silently under a new version number. That silent-carry is the failure mode #975 was filed about.

One trap for the next bump, which cost me a cycle: python -m tests.conformance.signing.vectors --write regenerates files only and carries spec_version/source forward unchanged, so running it alone will never bump the version. Those two fields need editing first, then the regen confirms the hashes. The file's "do not hand-edit" banner reads as covering the whole document — I've noted the distinction in the commit message.

The three dependent PRs (#985, #987, #993) are restacked on this base and gated green.

make ci-local: 5994 passed, 0 failed.

@KonstantinMirin KonstantinMirin changed the title test(signing): vendor the complete 3.1.1 request-signing vector set and pin it against drift test(signing): vendor the complete AdCP 3.1.8 request-signing vector set and pin it against drift Jul 29, 2026

@bokelley bokelley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the rebased 3.1.8 pin. The manifest version/source now match the SDK target, the vector payload hashes remain unchanged as expected, and CI is green. My previous request is fully addressed.

@bokelley
bokelley enabled auto-merge (squash) July 29, 2026 22:16
@bokelley
bokelley merged commit cbcd4c3 into adcontextprotocol:main Jul 29, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Vendored request-signing conformance vectors are incomplete, and the loaders cannot detect it

2 participants