fix: harden 3.1 digest attestations and codegen by bokelley · Pull Request #2052 · adcontextprotocol/adcp-client

bokelley · 2026-05-27T04:56:46Z

Summary

This PR resolves the 3.1 follow-up issues around digest attestations, upstream traffic validation, and conditional params codegen.

hardens conditional params promotion so root params refinements do not conflict while true promoted-key conflicts still fail loudly
makes digest projection use canonical JSON consistently, parse valid JSON string payloads for both payload digests and identifier proofs, and fail rather than emit non-canonical JSON digests
tightens storyboard upstream traffic grading for mixed raw/digest and raw/non-JSON evidence, including multi-clause payload_must_contain cases and actionable JSON pointers
updates recorder/storyboard docs for digest-mode throwing, malformed JSON strings, and identifier_paths request/response/context prefix rejection
adds regression coverage for the cached 3.1 comply controller schema, digest identifier scans, mixed attestation batches, manual JSON-string payloads, and authoring validation

Validation

npm run ci:quick
expert re-review: JavaScript/protocol, code review, and DX/docs all reported no blockers after follow-up fixes

Follow-up

Split the larger preferred-attestation-mode design question into #2051 so #2050 can close without blocking this hardening pass on a schema/API decision.

Closes #2045
Closes #2046
Closes #2047
Closes #2048
Closes #2049
Closes #2050

aao-ipr-bot

Approving. Clean hardening pass on the 3.1 digest surface — fail-closed on canonicalization, bounded identifier scan, load-time identifier_paths validation, and the not_applicable grading correctly extends spec PR adcontextprotocol/adcp#3987's precedent to mixed raw+digest batches.

Things I checked

scripts/generate-types.ts:575-620 — promoteConditionalParamProperties correctly distinguishes "duplicate identical refinement" (passes) from "conflicting promotion" (throws with allOf[N] index). Root-param refinements are intentionally ignored; new test at test/type-generator-allof-ref-merge.test.js:240-275 pins that contract. Cached 3.1 comply-controller schema round-trips green.
src/lib/upstream-recorder/recorder.ts:404-437 — projectRecordedCall now reports payload_length as the canonical byte stream covered by payload_digest_sha256, not the raw record-time length. That's the only definition that keeps the pair independently verifiable.
src/lib/upstream-recorder/recorder.ts:450-471 — collectMatchedStringLeafDigests is behaviorally equivalent to the prior jsonStringLeafDigests + membership check, but bounded and short-circuiting once all wanted digests are found. Iterative walk, depth-cap-safe.
src/lib/upstream-recorder/recorder.ts:483-518 — JCS via utils/jcs.canonicalize, throws on depth > 256. index.ts:50-56 docstring tells controller handlers to surface as a failed query_upstream_traffic response. Fail-closed beats fail-open.
src/lib/testing/storyboard/validations.ts:2770-2790 — walked the four-quadrant matrix for anyPayloadNotApplicable / hasMeaningfulPayloadPass: two raw matches → pass; mixed raw-pass + digest → not_applicable; raw-JSON inspectable miss + non-JSON raw → fail with pointer at the JSON call. Correct.
src/lib/testing/storyboard/validations.ts:2806-2812 — JSON pointer now prefers the first raw-JSON inspectable miss over the first matched call. Right shape; makes the pointer actionable.
src/lib/testing/storyboard/loader.ts:74-105 — request.* / response.* / context.* prefixes rejected at load time. Closes the silent-skip footgun from #2047. Author-controlled surface, not runner-controlled, so the residual bracket-notation gap (see nit 3) is DX rather than security.
64-digest cap (recorder.ts:445, runner.ts:5159, validations.ts:2826) — overflow surfaces as identifierDigestLimitExceeded with clipped count, runner grades not_applicable with the count in the note instead of silently passing or failing. Matches #2047 acceptance.
src/lib/server/decisioning/from-platform.ts:991 — empty supported_optimization_metrics now omitted from the wire capabilities response. Doc on capabilities.ts:140-165 says it explicitly. Right call: an empty list reads as "no support" rather than the intended "no rollup advertised."
Two changesets present — harden-conditional-param-codegen (patch, codegen-only) and upstream-digest-attestation-hardening (minor, new public export + behavior change). Categories are right.

Follow-ups (non-blocking)

Expand the upstream-digest-attestation-hardening changeset body with two adopter-affecting deltas the current prose buries: (a) RecordedCallBase.host / .path flipped from required to optional in upstream-recorder/types.ts:28-29 — runtime is unaffected because the recorder always populates them from new URL(url), but strict: true consumers of RecordedCall imported from @adcp/sdk/upstream-recorder will now see string | undefined. The shape was already loose through the testing/test-controller re-export, so the public surface ends up consistent — worth saying so. (b) payload_length semantics now meaningfully diverge between raw mode (raw recorded length) and digest mode (canonical byte length for the same call). Doc'd in types.ts:38-43, but worth surfacing in the changeset too.
computePayloadDigestSha256 redaction footgun. src/lib/upstream-recorder/recorder.ts:540-542 ships an unredacted-input helper alongside the recorder. JSDoc says "this helper does not redact for you," but adopters who wire a custom controller may call it on raw outbound bytes. Either rename to computePayloadDigestSha256AfterRedaction to put the warning at the call site, or accept an optional redact?: RegExp defaulting to SECRET_KEY_PATTERN. The latter is the safer default.
assertJsonDepth is recursive, then canonicalize recurses again. recorder.ts:505-518 is one recursion to 256, then jcs.ts:serialize recurses to the same depth on success. Safe at 256 against Node's default stack, but collectMatchedStringLeafDigests is already iterative at 450-471 — converging on iterative for the depth check is defense-in-depth.
Promote the JCS digest test vector to a spec PR. test/lib/upstream-recorder-spec-shape.test.js:198 pins '1456bd286cd759390538b520050a4df59e11fa794dc2cab8333402620f99ce29' for { b: 1.25, é: ['z'], a: { c: true } }. Spec PR adcontextprotocol/adcp#3816 doesn't pin canonicalization; without a shared vector, controllers will drift. Issue #2045 (split out in this PR) is the home for the spec follow-up.
Share IDENTIFIER_DIGEST_LIMIT = 64 between runner.ts:5159 and the implicit cap inside recorder.ts:445's normalizeIdentifierDigests. Drift risk later.

Minor nits (non-blocking)

Bracket-notation paths bypass validateUpstreamIdentifierPaths. src/lib/testing/storyboard/loader.ts:104 splits on /[.[\]]/, so ["request"].x and $..request.x slip past. Author-controlled, not runner-controlled, so it's a DX gap rather than a security hole — but a normalize-then-check pass would close it.
canonicalCodegenJson vs utils/jcs.canonicalize. scripts/generate-types.ts:575-587 ships a local canonicalizer instead of reusing jcs.canonicalize. Acceptable — the JSON Schema sub-objects don't carry the edge cases JCS guards (BigInt, exotic numerics, non-ASCII key ordering); the local helper is isolated to build-time. Worth a one-line code comment noting why JCS isn't reused.
Test gap for conflicting-conditional-refinement of root keys. type-generator-allof-ref-merge.test.js:240-275 covers single-branch drop but not "two conditional branches conflict on a root key, both silently ignored." Documented contract; worth pinning explicitly.

ad-tech-protocol-expert: sound-with-caveats — all seven decisions either align with #3816/#3987 or are the right SDK choice where the spec is silent. code-reviewer: no blockers, six minor follow-ups (rolled into the above). security-reviewer: no HIGH/CRITICAL; two MEDIUMs (depth-check recursion, helper redaction footgun) noted as fast-follows above.

Notable that the largest behavior shift here — not_applicable over passed for mixed raw+digest — is the anti-façade posture, not a softening. A partial-digest seller can't dodge inspection by mixing modes.

LGTM. Follow-ups noted below.

aao-ipr-bot

LGTM. Follow-ups noted below. Hardening pass closing #2045–#2050 lands cleanly: digest projection now redacts and JCS-canonicalizes before hashing (the right shape — recorder is the witness, owns the wire bytes it commits to), payload_length covers the canonical byte stream in digest mode, identifier scanning is bounded at 64 and depth-capped at 256, and the storyboard loader now rejects envelope-prefixed identifier_paths at authoring time instead of letting them resolve vacuously at run time. Two changesets — minor for the new computePayloadDigestSha256 export and the host/path widening, patch for promoteConditionalParamProperties failing loud on conflicts. Both categorizations are correct.

Things I checked

Changeset audit. Two changesets present; categories match wire/API impact. The minor covers a new public export and a type widening (host/path string → string | undefined on upstream-recorder/types.ts:29-32) — the recorder still always emits strings (recorder.ts:164-165 initializes both to ''), so the runtime shape is unchanged; the type change is a relaxation aligning with the looser comply-test-controller-response.json shape. Sound.
Witness-not-translator invariant. Recorder normalizes (URL→host/path, lowercase headers, body decode) and redacts before hashing — both are the recorder's own authoring responsibility on the wire shape it commits to. Nothing is fabricated outside what the 3.1 spec defines. ad-tech-protocol-expert verified against comply-test-controller-response.json > UpstreamTrafficSuccess and confirmed sound.
Discriminated-union conformance. RawRecordedCall requires payload and types payload_digest_sha256?: never; DigestRecordedCall flips it. Matches the RawAttestation / DigestAttestation branches in the cached schema. The additionalProperties: false envelope is honored.
IDENTIFIER_DIGEST_LIMIT = 64 (constants.ts:1) is the spec's normative maxItems: 64 on both identifier_value_digests (request) and identifier_match_proofs (response). Right number.
collectUpstreamIdentifierDigests clipped accounting (runner.ts:5224-5246) only stamps identifierDigestLimitExceeded when clipped > 0. Correct guard.
computePayloadDigestSha256 overload runtime check (recorder.ts:505-510) — verified for all three forms (RegExp, false, options object). The false short-circuit preserves the option for adopters who pre-normalize. The JSDoc warns; the ?? SECRET_KEY_PATTERN correctly distinguishes explicit false from undefined.
canonicalPayloadBytes ↔ parseJsonStringPayloadForScan symmetry. Both fall through to the raw string on JSON.parse failure, so identifier scanning and digest computation see the same view. No drift.
enforceStrictSchema codegen throw. promoteConditionalParamProperties (scripts/generate-types.ts:602-626) now ignores root-params.properties refinements (silent today, was silent before) and throws on truly conflicting promoted keys with a cited path. Six new harness tests pin the matrix.

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

record() with a JSON-string payload skips per-key redaction. recorder.ts:574-581 takes the typeof payload === 'string' branch and goes straight to cap() — redactSecrets is never called. wrapFetch is safe (parses first via readBody); only the manual-instrumentation record({ payload: JSON.stringify({...}), content_type: 'application/json' }) path holds the unredacted string in the buffer. computePayloadDigestSha256 correctly avoids this by routing through normalizeFetchPayloadInput — so the new helper and the recorder produce DIFFERENT digests on the same input when a secret-shaped key is present. Code-reviewer and security-reviewer landed on this independently — when two reviewers converge on the same line, the finding is real, even when it predates this PR. Fix: parse+redact+restringify in the JSON-string branch of normalizeRecordedPayload, symmetric with the form-urlencoded path. Pin both paths with an equality assertion.
Redaction depth cap (32) vs canonicalization depth cap (256). redactSecrets returns the subtree un-rewritten past depth 32; assertJsonDepth then accepts the structure (35 < 256) and hashes it verbatim. Pre-existing, but the new computePayloadDigestSha256 export makes the asymmetry more reachable. Cheapest fix is to raise REDACT_MAX_DEPTH to match MAX_JSON_DEPTH; alternatively, have redactSecrets throw past its cap rather than silently returning the subtree.
validateStoryboardShape not invoked from runStoryboardStep. The new validateUpstreamIdentifierPaths only fires through parseStoryboard / runStoryboard. Programmatic single-step callers bypass it. Add validateStoryboardShape(storyboard) near the top of runStoryboardStep — the helper is idempotent.
purpose field is free-string at recorder emission (types.ts:50-55, recorder.ts:152-159) but the spec restricts it to the enum [platform_primary, measurement, attribution, creative_serving, identity, other] under additionalProperties: false. An adopter classifier returning 'foo' produces an item that fails spec-shape validation. Clamp the classifier return to the enum (or undefined off-enum) before stamping and surface via onError.
NaN/Infinity digest path grades failed instead of not_applicable. Spec normatively requires the runner to grade non-finite-number payloads not_applicable in digest mode. Today: jcs.ts throws → controller emits INTERNAL_ERROR → runner grades failed. Either catch at digest projection and tag the call, or have the runner detect the specific JCS-finite throw and downgrade.
No-op recorder emits empty-string since_timestamp (recorder.ts:633-638) which violates the spec's format: date-time on UpstreamTrafficSuccess.since_timestamp. Adopters who gate enabled correctly never hit it; still worth substituting a sentinel like new Date(0).toISOString().

Minor nits (non-blocking)

Codegen conflict error message names only the losing branch. scripts/generate-types.ts:620-622 cites allOf[${memberIndex}] (the second branch). Track the first-promotion index and include both: …conflicts with allOf[${firstIdx}].then…. Saves a grep on build failure.
maxPayloadBytes: -1 / Infinity test labels are misleading. test/lib/upstream-recorder-spec-shape.test.js:319-328 exercises both, but clamp() (recorder.ts:47-55) collapses both to DEFAULT_MAX_PAYLOAD_BYTES. The parity passes only because both sides land on DEFAULT — not because the label semantics ("disable truncation" / "Infinity bypass") are honored. Either document "both fall to DEFAULT" or exercise maxPayloadBytes: 0, which actually disables truncation per cap() (line 772).
compliance/summary.ts:273 regex change drops trailing-whitespace stripping by reshaping [^;(]+ → [^;(]*[^ ;(]. Pure-whitespace payloads ("missing_required_tool_family: needs ") now fall through to the bare requirement_unmet causal bucket instead of normalizing to "missing_required_tool_family: needs". Likely fine but pin the malformed-input behavior with a unit test if it matters.

Safe to merge.

aao-ipr-bot

LGTM. Closes six 3.1 follow-ups in one pass; the digest pipeline is now coherent end-to-end (recorder normalization → JCS canonicalization → bounded-depth throw → storyboard grading → controller projection) with regression tests pinned at every seam.

Things I checked

Recorder rewrite end-to-end. normalizeRecordedPayload is the new chokepoint for raw + digest; payload_length math reconciles via Buffer.byteLength(_, 'utf8') on both paths (src/lib/upstream-recorder/recorder.ts:148-151, :397-403). readBody correctly clones before formData()/text() so the original Request stream stays intact for originalFetch (recorder.ts:708-718). assertJsonDepth is iterative, throws RangeError, propagates through query() to the test that pins the message text (upstream-recorder-spec-shape.test.js:444-447).
computePayloadDigestSha256 as a public export. The RegExp | false | { ... } overload disambiguation handles all four runtime shapes — including options = null defaulting through options?.redactPattern ?? SECRET_KEY_PATTERN (recorder.ts:517-522). Three-shape API surface adequately tested at upstream-recorder-spec-shape.test.js:329-333, 409-428.
projectRecordedCall payload_length swap to canonical bytes (recorder.ts:398). Only coherent interpretation in digest mode — emitted bytes aren't on the wire, so the canonical byte stream is the only honest referent.
Codegen: conditional-params conflict throws. canonicalCodegenJson (build-time only, deliberately separate from runtime JCS — no drift risk) gives stable deep-equal so identical duplicates pass and real conflicts throw with a pointer to both allOf indices (scripts/generate-types.ts:602-627).
Storyboard loader prefix rejection. validateUpstreamIdentifierPaths rejects request.* / response.* / context.* and normalizes $. prefix in place — idempotent on re-validation through runStoryboardStep (loader.ts:80-137, runner.ts:3203).
Witness invariant. No new fabrication paths. The empty-payload→{} substitution for raw GET/HEAD is the only synthetic shape, and it's a witness substitute for a structurally-required field (recorder.ts:562-567). Disabled-recorder since_timestamp epoch fallback is the right correctness fix — '' was schema-invalid against date-time.
Changesets. Two present, severity right (minor for the digest pipeline, patch for the codegen tightening). Wire-touching changes belong on @adcp/sdk minor.

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

redactSecrets depth-cap leak is asymmetric with digest mode. The redactor returns the subtree verbatim past REDACT_MAX_DEPTH=256 (src/lib/utils/redact-secrets.ts:48) while assertJsonDepth THROWS at the same cap. The docstring's "either redacted and hashable, or rejected" parity claim only holds for digest mode — raw recording will silently store unredacted secrets at depths >256. Net improvement vs. the prior 32-cap (which had the same shape), but the docstring needs to acknowledge the asymmetry, or applyRedactionToPayload should route raw recording through the same depth gate. Pathological depth, sandbox-only recorder, but worth closing.
anyMatchedCallSatisfies sawDigestCall masks raw-call misses on mixed evidence (validations.ts:2948-2975). One missing path on a raw JSON call + one digest sibling = not_applicable. Per-spec semantics should be per-call per-path: the digest sibling doesn't change that the raw call lacks the value. Recommend tightening so sawDigestCall only suppresses when !sawApplicableCall. Worth a clarification PR upstream against adcp#3987 to pin the mixed-evidence rule.
supported_optimization_metrics: [] → omitted is a wire behavior change absent from the changeset prose (from-platform.ts:991). Buyers that read "field present == 3.1 metric-optimization declaration" will now see a flip on adopters who previously emitted []. Capabilities docstring covers it; the changeset doesn't. Add a one-line bullet on the next minor cut.
JCS: non-finite number → not_applicable is SDK convention, not a spec clause. Documented as "matching the 3.1 runner contract" but I can't find it normatively pinned anywhere in schemas/cache/. The grading is defensible (RFC 8785 §3.2.2.3 forbids NaN/Infinity); file a spec-clarification PR upstream so the contract pins it.
IDENTIFIER_DIGEST_LIMIT = 64 is a local heuristic. No maxItems constraint on comply-test-controller-request.json#/identifier_value_digests. Propose the cap upstream so storyboard authors can rely on it portably.

Minor nits (non-blocking)

RecordedCall re-export drops the [key: string]: unknown index signatures the prior test-controller.ts definitions carried (src/lib/upstream-recorder/types.ts:58-90, test-controller.ts:39). Public-surface narrowing — adopters who attached forward-compat custom keys will hit TS errors. One-line [k: string]: unknown add on RecordedCallBase restores the prior latitude; alternatively, call this out in the changeset as a deliberate tightening.
host? / path? documented as "emits empty string on parse failure" but typed optional (types.ts:29-32). Pick one — either non-optional with '' sentinel, or strip to undefined on parse failure. Today's combination forces consumers to handle both.
parseJsonStringPayloadForScan false-negative on malformed JSON strings (recorder.ts:440-448). When the body fails JSON.parse, the whole string is hashed as a single leaf — substring identifiers won't match. Acceptable trade-off, but worth a line on DigestRecordedCall.identifier_match_proofs so adopters debugging found: false don't suspect a recorder bug.

Safe to merge.

aao-ipr-bot

Holding for one Critical. The digest-attestation hardening is the right direction and most of the surface is clean — but the JSON-string branch of normalizeRecordedPayload fails-open where the object branch fails-closed, and the result is unredacted secrets sitting in the recorder buffer.

MUST FIX

src/lib/upstream-recorder/recorder.ts:586-606 — JSON-string payload branch leaks secrets past the depth cap.

The flow for a >256-deep JSON string body:

JSON.parse(payload) succeeds (V8 parses thousands of levels).
redactSecrets walks to depth 256 and returns the deeper subtree verbatim, including any authorization / token / cookie keys past depth 256.
assertJsonDepth(redacted) throws RangeError at line 590.
The bare catch at line 596 swallows it.
Control falls past the form-urlencoded check; line 606 returns cap(payload, maxPayloadBytes) — the original raw JSON string, capped to 65 KB but otherwise untouched. String-mode redaction never re-runs on raw text, so "authorization":"Bearer ..." sits in the buffer in cleartext.

Compare the object-payload branch at lines 609-615: assertJsonDepth(redacted) is outside any try, so the RangeError propagates to buildRecordedCall's try/catch (line 174-208), emits payload_build_failed, and the record is dropped entirely. String branch fails open; object branch fails closed. The file header still claims "plaintext secrets never sit in the recorded-calls buffer in memory, even briefly" — this branch violates that contract.

The fix is to narrow the catch to SyntaxError so only malformed-JSON falls back to the diagnostic raw-string path, and the depth assertion drops the record the same way as the object branch:

if (typeof payload === 'string') {
  if (isJsonContentType(contentType)) {
    let parsed: unknown;
    try {
      parsed = JSON.parse(payload);
    } catch {
      // Malformed JSON: keep the raw body for diagnostics.
      return cap(payload, maxPayloadBytes);
    }
    const redacted = redactSecrets(parsed, redactPattern);
    assertJsonDepth(redacted);  // re-raises out; drops the record at buildRecordedCall
    const json = safeStringify(redacted);
    if (json && maxPayloadBytes > 0 && byteLengthOf(json) > maxPayloadBytes) {
      return `[truncated ${byteLengthOf(json)} bytes]`;
    }
    return redacted;
  }
  // ...
}

Add a regression test against test/lib/upstream-recorder-spec-shape.test.js that records a JSON string body deeper than 256 levels with an authorization key past depth 256 and asserts the record was dropped (and that the raw string never lands in the buffer).

Things I checked

scripts/generate-types.ts:602-627 — promoteConditionalParamProperties throw-on-conflict via canonicalCodegenJson equality. Idempotent redeclaration permitted; conflicts hard-fail with both allOf[] indices named. Test at test/type-generator-allof-ref-merge.test.js:376-395 confirms cached comply-test-controller-request.json is clean today.
RecordedCall lift to src/lib/upstream-recorder/types.ts with test-controller.ts re-export — surface preserved, no importer broken.
RawRecordedCall.payload = {} for undefined bodies + host/path as required strings emitting "" on parse failure — matches the 3.1 UpstreamTrafficSuccess discriminated arms.
payload_length switched to byteLengthOf(canonicalPayloadBytes(...)) for digest mode — the only value a runner can sanity-check against payload_digest_sha256. Test at upstream-recorder-spec-shape.test.js:168-171 locks it.
src/lib/testing/storyboard/loader.ts:80-119 validateUpstreamIdentifierPaths — rejects request.* / response.* / context.* prefixes and bracket / recursive forms before they silently resolve to undefined. runStoryboardStep now runs the same shape validation as full runs.
src/lib/server/decisioning/runtime/from-platform.ts:988-994 — supported_optimization_metrics empty-array → omitted from the wire response. Matches the schema description ("omitting means no specific guarantees declared"); [] would be a different and useless claim.
Cross-principal isolation in recorder.ts:323-324 is unchanged. Digest projection runs on a single matched entry after filtering and cannot leak cross-principal.
Changeset scoping: harden-conditional-param-codegen correctly patch (codegen is build-time, cached schemas don't trip the new throw); upstream-digest-attestation-hardening correctly minor (new export computePayloadDigestSha256, behavior tightening, no removal).

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

JCS non-finite error grading is string-coupled. validations.ts:2682 greps query.payload.error for /JCS: non-finite number\b/, which is the message emitted by this SDK's src/lib/utils/jcs.ts:120. A conforming third-party controller written in Go/Python won't emit that exact string and will fail-grade instead of NA-grade. File against adcontextprotocol/adcp for a normative JCS_NON_FINITE_NUMBER error code so cross-implementation NA parity holds.
computePayloadDigestSha256(..., false) overload is a footgun. Three-form options parameter (RegExp | false | {...}) invites adopters who skim docs to pass false thinking it means "use default redaction" — instead they emit digests over unredacted bytes that won't match the recorder's. Consider a named sentinel like { prenormalized: true } in a follow-up minor.
Malformed-JSON identifier-proof self-suppression (recorder.ts:440-448). An adversarial agent can emit JSON-shaped-but-unparseable bodies to make digest-mode identifier scanning report found: false for every vector. Practically mitigated by pairing identifier checks with attestation_mode_required: 'raw', but worth emitting an onError event when JSON.parse fails on a JSON content-type so adopters can detect the tamper.
identifier_digest_limit_exceeded.clipped counts encounters, not distinct vectors (runner.ts:5239-5242). Track a Set<string> of clipped vectors and increment only on first sight so the clipped: N value the validation surfaces means what its name says.

Minor nits (non-blocking)

validations.ts:2719-2724 comment is stale. Prose still says "the assertion grades not_applicable only when every path-based check downgraded that way," but the code at line 2791-2796 grades NA when any path is NA and no path is missing. The new behavior is the right one (per adcp#3987), so update the comment to match. Add a test for "1 satisfied path + 1 NA path" to lock the intended semantics.
src/lib/upstream-recorder/types.ts:24-25 [key: string]: unknown; on RecordedCallBase. Weakens typo detection for projectRecordedCall — a future payload_lenght would compile. Either drop the index signature (no external importer reads RecordedCallBase directly — the re-export skips it) or add a one-line comment noting it's for spec-additive forward compat.
.changeset/harden-conditional-param-codegen.md prose. Worth one extra line warning adopters who maintain forked schemas that codegen will now hard-fail on conflicting allOf[].then.properties.params.properties.* instead of silently keeping the first branch — that's the behavior change adopters care about.

Once the JSON-string branch is fixed (narrow the catch to SyntaxError + regression test), this is safe to merge.

aao-ipr-bot

LGTM. Follow-ups noted below. Closes six 3.1 follow-up issues without breaking the witness-not-translator invariant — digest projection fails closed, redaction depth aligns with canonicalization depth, identifier-path scope check fires at load and programmatic step entry.

Things I checked

Changesets line up with impact. Two new changesets — upstream-digest-attestation-hardening.md (minor — computePayloadDigestSha256 is a new public export from src/lib/upstream-recorder/index.ts:64) and harden-conditional-param-codegen.md (patch — codegen hard-fail on conflicting promotions). Type matches wire impact.
JCS adoption is fail-closed. recorder.ts:476-479 routes through src/lib/utils/jcs.ts; non-finite numbers throw JCS: non-finite number … rather than emit a non-canonical digest. Storyboard runner detects this on the controller envelope and grades not_applicable at validations.ts:2680-2695.
Depth gate is now consistent. REDACT_MAX_DEPTH = 256 in redact-secrets.ts:39 matches MAX_JSON_DEPTH = 256 in recorder.ts:32. Recorder calls assertJsonDepth(redacted) after redactSecrets (recorder.ts:614 and :630) and converts a deeper structured payload into payload_build_failed — call is dropped rather than stored with unvisited subtrees.
Identifier-proof early termination is sound. collectMatchedStringLeafDigests at recorder.ts:421-442 only short-circuits when matched.size === wantedDigests.size. Verified against the 4100-entry test at test/lib/upstream-recorder-spec-shape.test.js — the missing digest forces full-payload traversal.
Codegen hard-fail handles the right cases. scripts/generate-types.ts:572-621 rejects conflicting sibling allOf[].then.properties.params.properties.* promotions while leaving root-defined params keys untouched (test fixture exercises the cached 3.1 controller-request schema).
identifier_paths scope check fires for both runStoryboard and runStoryboardStep (runner.ts:3203).
identifier_match_proofs cap matches spec. IDENTIFIER_DIGEST_LIMIT = 64 aligns with the 3.1 query_upstream_traffic request schema's documented "Cap of 64 digests per query."
Principal isolation is unchanged. recorder.ts:323-328 still filters on entry.principal === params.principal first; url_parse_failed storing empty host/path cannot widen a cross-principal query.

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

payload_length semantics in digest mode are SDK-defined, not spec-pinned. Digest projection at recorder.ts:402 measures byteLengthOf(payloadBytes) of the canonical bytes — consistent with payload_digest_sha256, but the 3.1 comply-test-controller-response.json doesn't normatively pin whether digest-mode payload_length should report raw emitted bytes or canonical bytes. File a spec issue on adcontextprotocol/adcp to disambiguate before more adopters ship divergent measurements. Compensating not_applicable grading at validations.ts:2686 keeps the blast radius contained today.
JCS throw propagation isn't smoothed. recorder.ts:331 calls projectRecordedCall inside a .map() with no try/catch. A single stored payload with NaN / Infinity / BigInt poisons the whole recorder.query() result for adopters calling it directly — the storyboard runner catches the error envelope, but a direct adopter handler will throw at the wire. Consider wrapping the .map body and emitting payload_build_failed per-entry, or document the contract loudly in recorder.query()'s JSDoc.
Malformed JSON-string payload + application/json content-type stores plaintext. recorder.ts:603-612 returns cap(payload, maxPayloadBytes) verbatim on JSON.parse failure — a truncated body like {"authorization":"Bearer xyz" lands unredacted. Not a regression (pre-diff behavior was the same and worse — no parse path at all), and well-formed JSON strings now redact correctly, but the malformed branch is the residual hole. Consider a regex-fallback redact pass over the raw string before cap().
/JCS: non-finite number\b/ couples the runner to one canonicalizer's error string. validations.ts:2680 matches against query.payload.error from the agent-under-test's controller envelope — agents using a different JCS impl won't downgrade to not_applicable. Lift this into a structured error_code field on the controller envelope (jcs_non_finite_number) and match that instead, or call out the brittleness explicitly in the comment.
validateUpstreamIdentifierPaths mutates caller-owned strings. loader.ts:297 rewrites validation.identifier_paths[pathIndex] in place. runStoryboardStep now re-validates programmatic input (runner.ts:3203), so callers passing their own Storyboard objects will observe $.foo rewritten to foo post-call. Document the mutation contract or clone before mutating.
identifier_paths loader grammar is stricter than the spec defines. Loader at loader.ts:114-118 + the regex at :127-128 reject $.., $["…"], and numeric [0] selectors. No existing storyboard in this tree breaks, but third-party storyboards using those shapes will fail at load. Worth documenting "identifier_paths grammar" so other 3.1 runners narrow the same way.

Minor nits (non-blocking)

computePayloadDigestSha256 options union. recorder.ts:522-549. RegExp | false | { … prenormalized? } is documented but baroque — false is the same as { prenormalized: true }. Soft-deprecate the bare RegExp and false forms in JSDoc (@deprecated pass { redactPattern } / { prenormalized: true }); plan removal at the next major. The runtime branch can stay.
Redundant valueDigest && guard. validations.ts:3050 — the continue at :3047 already short-circuits the no-digest path, so the extra valueDigest && on the conditional below is dead. Cosmetic.
canonicalCodegenJson array-order coupling. scripts/generate-types.ts:~115. Two schema branches authoring semantically-equivalent enums in different array orders (["a","b"] vs ["b","a"]) would now throw as conflicting. Realistic only during a refactor, but consider sorting enum arrays before comparison or noting the limitation in the throw message.

Approving on the strength of the witness-not-translator fail-closed posture plus the test coverage at test/lib/upstream-recorder-spec-shape.test.js (~370 lines pinning JCS digest vectors, malformed-JSON onError surfacing, depth-cap fail-closed for both recording and digesting, identifier-proof early-termination).

aao-ipr-bot

Hardening direction is right and most surface lands clean — but one new line teaches sellers a magic substring that downgrades the anti-façade check, and the wire-doc on query() ships out of sync with what the code does.

Blocking

validations.ts:2682-2697 — controller-authored error string downgrades upstream_traffic from failed to not_applicable. New carve-out grades passed: true, not_applicable: true whenever query.payload.error matches /JCS: non-finite number\b/. Pre-PR, that string came from the recorder's own JCS throw. This PR also moves the recorder to safeProjectRecordedCall, which catches that throw and drops the entry on success: true (recorder.ts:377-388). After this PR the SDK's own loop never emits the string. The only remaining emitter is the seller's query_upstream_traffic handler.

Concrete bypass: an adapter that doesn't actually call upstream returns { success: false, error: "JCS: non-finite number — could not canonicalize" } from every controller invocation. Storyboard upstream_traffic validation grades not_applicable, rollup counts it as passing-or-skipped, anti-façade detection silent. The SDK is the witness; that's the contract. Substring match on adopter-controlled prose is the wrong shape.

Fix path (pick one):
- Drop the carve-out. The SDK's own recorder no longer surfaces this error — the branch is effectively dead unless a seller mints it. Letting it grade fail matches pre-PR behavior on the same input.
- Move the carve-out onto a structured error code field (error_code: "jcs_non_canonicalizable" on the controller-error envelope, gated through toQueryUpstreamTrafficResponse) so the signal is something the seller can't forge from prose.
safeProjectRecordedCall makes the carve-out unreachable on the SDK side without leaving a wire signal. recorder.ts:330-345 — when projection drops every matched entry (NaN, depth>256, etc.), total = 0, truncated = false, items = [], response is success: true. payload_build_failed fires through onError but stays process-local; the runner never sees it. Combined with blocker #1: the only path that surfaces the JCS failure to the runner is the controller author writing the magic string. Pick one:
- Re-throw on per-entry projection failure (matches the doc-comments below in #3 and the changeset's stated "fail-closed" direction for deep JSON, commit 46ab685).
- Emit a structured dropped_total counter on UpstreamRecorderQueryResult / QueryUpstreamTrafficResponse so the runner has something to grade against.
types.ts:253-254 + :361-362 + index.ts:51-55 — three new doc blocks say query() throws on non-canonicalizable digest payloads. Code drops and emits payload_build_failed (changeset agrees). The doc and the behavior were both edited in this PR; pick one and align. Adopters wiring try/catch around query() per the doc will silently degrade.

Follow-ups (non-blocking)

recorder.ts:62-63 — RawRecordedCall.payload doc still says "Decoded JSON object when content_type is JSON-shaped; raw string otherwise." After this PR, malformed JSON under a JSON content type is stored as a redacted string. Amend.
recorder.ts:835-845 redactJsonLikeSecretValues — regex-based scrubber misses unquoted keys ({authorization: 'bearer abc'} — JS-literal-pasted-as-body, which is exactly the malformed-JSON case the path exists for) and nested secret keys past the first sibling (the [^,\s}\]]+ value alternative consumes into an inner {, advancing lastIndex past the nested "password"). Two options: land a streaming JSON-ish tokenizer, or fail-closed by substituting [unparseable JSON N bytes] instead of partial scrub. Storing a partially-scrubbed body in the buffer is in tension with "never store plaintext secrets even briefly."
recorder.ts:339 — since_timestamp fallback echoes matched[0]?.call.timestamp from the pre-drop list. If the projection drops that entry, the response echoes a window header for a record the caller never sees. Use items[0]?.timestamp ?? matched[0]?.call.timestamp ?? ….
recorder.ts:413-414 — RecordedCall.host / .path are now required-as-string with '' fallback on new URL(url) failure. Code does emit url_parse_failed through onError at recorder.ts:182-184, which is the right shape, but the spec text on whether host/path are type: string, string | null, or string, minLength: 1 in comply-test-controller-response.json is worth pinning before the wire shape ossifies. Confirm against the merged spec PR adcontextprotocol/adcp#3816.
generate-types.ts:578-590 — canonicalCodegenJson enum sort works on every valid JSON Schema enum value type but stringifies undefined to literal "undefined". Unreachable for valid schemas; an upstream typo wouldn't be caught. Not worth fixing unless it's fired in practice.
loader.ts:113-117 — validateIdentifierPathSyntax compares first segment case-sensitively against 'request' | 'response' | 'context'. $.REQUEST.foo would pass the loader but resolve nothing at runtime. Low-impact; flag for symmetry.
Per protocol expert: runner-output-contract.yaml v2.0.0 grading rule for "any not_applicable path demotes the validation" — should be cross-checked against the contract text before declaring this the canonical behavior. Validations rewrite at validations.ts:2790-2805 is plausible but isn't grounded in a cited spec line.

Things I checked

Recursive redactSecrets at depth 256 is stack-safe; assertJsonDepth runs after the recursive walk so deeper-than-256 subtrees that pass through redactSecrets unchanged are rejected before storage. Witness-not-translator floor holds for the in-buffer write.
loader.ts:80-131 — validateUpstreamIdentifierPaths rejects request.*, $["request"].*, $..request.*, bracket-quoted, recursive-descent, and the case-sensitive nit above. Programmatic runStoryboardStep now calls validateStoryboardShape (runner.ts:3203) so the gate isn't bypassable.
computePayloadDigestSha256 overload set is sound; legacy RegExp and false forms still accepted; the helper is new so no double-redact hazard from existing adopters.
IDENTIFIER_DIGEST_LIMIT = 64 matches spec text quoted in tools.generated.ts:22497.
supported_optimization_metrics empty-array omission at from-platform.ts:991-994 parallels how 3.1 capability blocks handle presence-vs-empty.
promoteConditionalParamProperties consolidation across generate-types.ts and generate-3-1-beta-types.ts is the right factoring — the beta codegen path will now also throw on conflicting branches, matching the changeset claim.
Changeset shape is correct: one minor for the new export surface (computePayloadDigestSha256, PayloadDigestOptions, RecordedCall re-exports, query() projection contract change), one patch for the codegen hardening, one revision to the pre-existing 3.1 changeset.

Blockers above need either prose or structural fixes. Happy to re-review on push.

aao-ipr-bot

LGTM. Follow-ups noted below. Hardening is in the right place — JCS for payload_digest_sha256, depth-cap on redaction so subtrees beyond 256 throw rather than store unvisited, and a single source of truth on RecordedCall now that test-controller.ts re-exports from upstream-recorder/types.ts instead of duplicating the union.

Things I checked

Changeset-vs-wire-impact: upstream-digest-attestation-hardening.md (minor) matches the new public exports (computePayloadDigestSha256, PayloadDigestOptions) and the digest-mode payload_length semantic change at recorder.ts:421. harden-conditional-param-codegen.md (patch) is right — build-time fail-loud only, no runtime API delta. resolve-3-1-blocked-issues.md correctly extended for the empty-array SOM omission. No changeset gap.
Spec coherence (ad-tech-protocol-expert): SDK agrees with 3.1.0-beta.5 on every load-bearing surface. JCS mandate at comply-test-controller-response.json#/.../payload_digest_sha256; NaN/Infinity → not_applicable in the same schema's JCS edge-case note; identifier_match_proofs/maxItems: 64 matches IDENTIFIER_DIGEST_LIMIT = 64; identifier_paths grammar matches storyboard-schema.yaml (lines 1428-1462). No divergence.
Security posture (security-reviewer): no High/Critical. The assertJsonDepth(redacted) calls at recorder.ts:651, 667 close the "unvisited subtree stored" footgun the 32→256 bump in redact-secrets.ts:39 would otherwise have opened. Multipart Request body now reads via clone.formData() (recorder.ts:769-773) and projects to {name: value} so the redaction walk still sees field names.
recorder.ts:330-336 — total++ only on successful projection, truncated: total > items.length consistent. safeProjectRecordedCall is per-call fail-closed (bad call dropped, excluded from total) and per-query fail-open (others returned). Right shape.
recorder.ts:541-581 — computePayloadDigestSha256 overloads route RegExp, false, and PayloadDigestOptions correctly. prenormalized: true and legacy false produce identical digests; the parity test at upstream-recorder-spec-shape.test.js:336 pins it.
loader.ts:122-131 — identifier-path regex rejects $..request.x, $["request"].x, request[*].x. runStoryboardStep entry at runner.ts:3203 now runs validateStoryboardShape upfront so programmatic callers get the same loud-fail-on-drift gate as runStoryboard.

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

upstream_traffic not_applicable is spoofable through the controller error string. validations.ts:2682 matches /JCS: non-finite number\b/ against query.payload.error, which is composed from the adopter-controlled errResult.error_detail (runner.ts:5186). An adopter being graded can return that exact string from their comply_test_controller and convert a failing upstream_traffic check into passed: true, not_applicable: true. validations_not_applicable propagates onto StoryboardResult and ComplianceResult but is not surfaced on ComplianceSummaryArtifact — which is what most CI/badge tooling consumes. Mitigations: (a) include validations_not_applicable in the summary artifact, or (b) require a typed error code (e.g. error: "non_finite_canonicalization") instead of substring-matching free-text. Trust model attenuates this — the SUT is already trusted to return its own controller responses — but the new regex matcher added a dodge path without making it observable to the gating tooling.
Brittle string-match on JCS: non-finite number. Same regex (validations.ts:2682) silently fails open if jcs.ts:120 ever rephrases the error message. Convert to a typed error class so the downgrade is resilient to future canonicalizer refactors.
safeProjectRecordedCall silent drop loses operator signal. recorder.ts:377-388 catches JCS canonicalization throws and emits payload_build_failed via onError. Adopters who have not wired onError see no breadcrumb when a NaN payload disappears from a digest-mode query — and the storyboard then grades on matched_count mismatch rather than a clear "non_finite" note. Fire a typed digest_canonicalization_failed event, or call out the onError wiring requirement in the digest-mode docstring.
parseJsonStringPayloadForScan runs even when no identifier digests were requested. recorder.ts:399 emits json_payload_parse_failed on every malformed JSON entry in a digest-mode query() whether or not identifier proofs were asked for. Cheap fix: short-circuit when identifierDigests.length === 0.
Pre-existing codegen gap, not this PR. tools.generated.ts:22980-22988 and schemas.generated.ts:7961-7967 emit collapsed RawAttestation / DigestAttestation interfaces — the per-branch payload/digest fields are lost. SDK adopters using upstream-recorder/types.ts see the right discriminated union; only direct consumers of the generated types hit the collapse. Separate codegen issue.

Minor nits (non-blocking)

loader.ts:113 case-sensitive prefix gate. Request.foo / Response.foo / Context.foo (capital) bypass the prefix check. The resolver walks the payload directly with no {request,response,context} envelope so there's no false-positive echo risk — but the asymmetry with the i-flagged SECRET_KEY_PATTERN is cosmetic. One-line .toLowerCase() closes it.
redactJsonLikeSecretValues produces non-JSON output on malformed-JSON arrays. recorder.ts:835-905 — {"token": [1,2,3], ... (malformed) emits {"token": [redacted],2,3], ... because skipJsonLikeValue does not track bracket depth. The secret-named key is correctly redacted; what leaks past is array elements that were not the secret. Diagnostic-aesthetic, not a security gap. Worth a JSDoc note that the fallback is "best-effort scrub, output is not guaranteed valid JSON."
RecordedCallBase carries [key: string]: unknown (types.ts:26). Type-level forward-compat hatch, but the 3.1 schema has additionalProperties: false on recorded_calls[] items. Adopters who attach arbitrary keys pass tsc and fail wire validation. JSDoc should say the index signature anticipates future spec fields, not adopter-defined ones.

Third "final 3.1" cleanup in a row — worth checking whether the next storyboard release stops feeding new follow-ups, or whether the spec authoring loop is the actual feeder. Not this PR's problem.

Approving on the strength of the JCS conformance and the depth-cap fail-closed posture.

aao-ipr-bot

Approving. Coherent hardening pass closing six 3.1 follow-ups, witness-not-translator preserved end-to-end: digest projection fails closed on non-canonical JSON instead of fabricating, malformed-JSON scrub is best-effort with raw bytes hashed verbatim, identifier overflow grades not_applicable rather than silently dropping vectors.

Things I checked

Both changesets attached. harden-conditional-param-codegen.md is patch (tightens an existing transform with a fail-loud branch). upstream-digest-attestation-hardening.md is minor — correct: computePayloadDigestSha256 ships a new options shape via overloads 1–3, the legacy RegExp and false signatures remain wired through overloads 2 and 3, and UpstreamRecorderErrorEvent adds three additive kinds. Exhaustive switch on the event union will fail to compile, which is the desired outcome for adopters running adopter-side diagnostics.
[key: string]: unknown on RecordedCallBase (types.ts:24-30) is not a new escape — both RawRecordedCall and DigestRecordedCall already carried the index signature in the pre-PR types; this hoists it. projectRecordedCall (recorder.ts:415-430) still constructs the wire payload by whitelisting fields, and the 3.1 additionalProperties: false on recorded_calls[] rejects forged keys at the wire boundary.
Redaction depth 32→256 (redact-secrets.ts:39) is the right shape now that assertJsonDepth (recorder.ts:503-518) runs on the post-redaction tree at the same threshold — the redaction walk and the depth gate run in lockstep, deeper structured payloads are rejected by buildRecordedCall's outer try/catch (recorder.ts:202-207) and surfaced as payload_build_failed.
validateStoryboardShape invoked from runStoryboardStep (runner.ts:3203) — right shape. The validator was already idempotent and the existing contributes→contributes_to mutation means the entry point was always opinionated about input.
Empty supported_optimization_metrics omission (from-platform.ts:988-993) — sound. The 3.1 schema makes the field optional with no minItems; emitting [] would advertise a positive metric-optimization declaration covering zero metrics. Omission carries the correct semantic.
Codegen throw on conflicting promoted params (generate-types.ts ~622) — bounded risk. Enum-order tolerance via canonicalCodegenJson prevents cosmetic false positives; the cached 3.1.0-beta.5 schemas pass the regression test added at test/type-generator-allof-ref-merge.test.js.
payload_must_contain not_applicable grading inverted from "every path" to "any path" (validations.ts:2790-2805). Spec PRs adcp#3816 and adcp#3987 pin per-clause semantics but do not normatively pin the multi-clause grading rule. The new "any" posture is more conservative and matches the anti-façade intent of adcp#3987, but it is a deliberate SDK interpretation, not a bugfix.

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

Doc drift on digest-mode query() semantics. Three doc blocks still claim digest-mode query() "throws" when JCS canonicalization fails: src/lib/upstream-recorder/index.ts:51-55, src/lib/upstream-recorder/types.ts:257-259, src/lib/upstream-recorder/types.ts:366-367. The new behavior in safeProjectRecordedCall (recorder.ts:382-388) is drop-the-entry + emit digest_canonicalization_failed via onError. Adopters writing try/catch on query() expecting a throw will end up with a silent drop. Update the three JSDoc passages to match the catch-and-drop reality.
prenormalized: true is a digest-oracle footgun. computePayloadDigestSha256(payload, ct, { prenormalized: true }) skips redaction entirely and hashes raw bytes. The digest is one-way, but an adopter who mis-reads the JSDoc and forwards raw fetch bodies will publish digests covering Authorization: Bearer … — and the buyer-visible digest becomes a low-entropy pre-image confirmation oracle against guessable bearer tokens. Mitigation: rename to unsafeSkipRedaction, emit a one-shot warning on first use, or have the helper scan for SECRET_KEY_PATTERN-shaped keys when prenormalized: true and throw on detection.
No cycle guard on redactSecrets; computePayloadDigestSha256 has no public try/catch. A cyclic input expands during redaction into a 256-deep linear chain whose leaf is the original cyclic node; assertJsonDepth then throws RangeError. Inside record() this is caught and emitted as payload_build_failed — safe. The public digest helper has no surrounding try/catch, so a cyclic payload becomes an uncaught RangeError propagating into adopter query_upstream_traffic handlers, surfacing as 5xx instead of 4xx. Add a WeakSet cycle guard in redactSecrets and wrap the helper's depth-overrun in a typed PayloadDigestError.
PayloadDigestOptions does not key-align with createUpstreamRecorder options. The changeset narrative says "pass the same custom options used by createUpstreamRecorder()" but the helper exposes redactPattern / maxPayloadBytes / prenormalized while the recorder uses different field names. Either alias the helper's keys or add a one-line mapping table in JSDoc.
payload_must_contain "any not_applicable downgrades" posture deserves a spec-repo issue. The PR ships the SDK's interpretation; file against adcontextprotocol/adcp to ratify the multi-clause grading rule so other implementations converge instead of mirroring the SDK after the fact.
IDENTIFIER_DIGEST_LIMIT = 64 is runner-side, not spec-pinned. Consider making it configurable for high-cardinality audience sync flows where 64 vectors will pinch. Document this is a runner bound (not a spec value) on the storyboard identifier_paths JSDoc.
runStoryboardStep validation has no opt-out. Negative-test harnesses that intentionally construct malformed storyboards now throw at the entry point. Add a { skipValidation?: boolean } option if/when an adopter files for one — don't pre-build it.

Minor nits (non-blocking)

Storyboard runner.ts:3203 — add a one-line regression test for the new programmatic-entry validation. The changeset calls the behavior change out, but locking the contract via a test prevents a future refactor from silently weakening it.

Approving on the strength of the test coverage (1247 lines of new tests against ~733 lines of new src), the witness-not-translator posture held end-to-end, and clean expert verdicts (code-reviewer Major is doc-only, security findings are dev footguns rather than buyer-attacker paths). The payload_must_contain inversion is the load-bearing judgment call — it ships correctly, but file the spec issue.

aao-ipr-bot

Approving. The hardening lands the witness-not-translator posture in the places that mattered: redaction now walks to the same 256-cap the canonicalizer asserts at, raw recording fails closed on overdeep payloads instead of buffering subtrees verbatim, digest projection drops the bad entry instead of poisoning the whole query(), and storyboard identifier paths refuse the four request/response/context-prefixed shapes that previously misgraded.

Things I checked

Changeset audit. Three changesets, types align with impact: upstream-digest-attestation-hardening.md is minor (new exports PayloadDigestError, computePayloadDigestSha256, PayloadDigestOptions), harden-conditional-param-codegen.md is patch (build-time only — codegen hard-fails on conflicting promoted params keys), and resolve-3-1-blocked-issues.md correctly amends prose for the empty-rollup omission. No manual package.json version edit.
Witness invariant on safeProjectRecordedCall (recorder.ts:387-398). Per-entry try/catch around projection. The matched.filter still runs first, so cross-principal isolation is preserved; only the failing entry gets dropped, with digest_canonicalization_failed surfaced through onError. No silent fabrication: failing entries don't get placeholder digests.
payload_length in digest mode (recorder.ts:434). Now byteLengthOf(payloadBytes) — the canonical bytes the digest covers. This is the only self-consistent posture: length and digest agree, attestation verifiers can pre-size for re-hash. types.ts:46-52 docs the raw-vs-digest split correctly.
assertJsonDepth post-redact (recorder.ts:725, 741). Redaction-then-depth-check. Old 32-deep-passthrough is closed; payloads >256 throw RangeError which buildRecordedCall catches and surfaces as payload_build_failed, no record stored. security-reviewer confirmed the closed exfiltration vector.
Cross-principal isolation preserved. query() filters by entry.principal === params.principal before projection. safeProjectRecordedCall cannot escape the filter.
Codegen conflict detection (scripts/generate-types.ts:582-628). canonicalCodegenJson deterministically serializes; enum-order normalization at keyHint === 'enum' accepts reordered enums but still throws on conflicting subsets (the JSON strings differ when lengths differ). Conflict error names both allOf indices.
Identifier-path validation robustness (loader.ts:80-131). Rejects Request.* (case), $.request.* ($-prefix), bracket-quoted forms, recursive descent, numeric indexes. Trim happens before validation but the stored path is not mutated.
validateStoryboardShape ordering in runStoryboardStep (runner.ts:3203). Shape gate runs before applyStoryboardVersionOptions — no prior consumer of malformed storyboard.requires.
Disabled-recorder since_timestamp. Now '1970-01-01T00:00:00.000Z' (recorder.ts:795) — RFC 3339 valid; "" would have failed any format: date-time validator.

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

payload_length digest-mode change is under-disclosed. The changeset prose calls it "now spell out the existing attestation semantics" — but recorder.ts:434 actually changes the value (call.payload_length → byteLengthOf(payloadBytes)). Adopters who shipped digest mode against the prior payload_length will see different numbers. Worth a one-line "BEHAVIOR CHANGE" callout, or a note in the migration doc.
findUnredactedSecretPath is recursive (recorder.ts:634-657), not iterative. The post-redact assertJsonDepth gate uses an explicit stack at 256 cap, but the prenormalized-safety walker uses Node's call stack. A 10k-deep prenormalized payload trips a RangeError: Maximum call stack size exceeded before the depth cap fires. Convert to the same iterative pattern. Low-severity because the path is adopter-opt-in ({ prenormalized: true }), but worth aligning with the rest of the file.
isUnredactedSecretScalar (recorder.ts:659-661) misses non-scalar redacted markers. Only '[redacted]' is treated as safely-redacted; null / true / false slip past. Adopters who pre-redact with null (a plausible custom posture) get hashed-over-plaintext. Document the literal '[redacted]' requirement in the prenormalized: true JSDoc, or widen the predicate.
redactJsonLikeSecretValues is best-effort by design but the doc comment could be louder. skipJsonLikeValue doesn't walk into object/array values — when the secret value is {"nested":"..."}, the outer password: redacts but the trailing {...} lands verbatim. Header notes "not guaranteed to be valid JSON" but doesn't call out that nested-object secret values can survive scrub. Mostly diagnostic-noise concern; the buffer ceiling still bounds blast radius.
MAX_JSON_DEPTH = 256 is duplicated between recorder.ts:32 and redact-secrets.ts:39 (REDACT_MAX_DEPTH). The two are documented to stay aligned. Export one constant and import it in the other file.
/JCS: non-finite number\b/ regex (validations.ts:2682) string-matches a downstream library's error message. A future @trust0/ridb-canonicalize (or whichever JCS lib) patch that changes the prefix flips the regrade from not_applicable back to failed. Consider a structured error code from the recorder, or pin the exact string in a unit test.
Confirm against published 3.1 schema:
- Does comply-test-controller-response.json set minLength: 1 on RecordedCall.host / RecordedCall.path? "" on URL parse failure is sound posture if the schema allows empty strings; unsound if it doesn't.
- Does the same schema set additionalProperties: false? The new [key: string]: unknown index signature on RecordedCallBase is TS-only forward-compat; the comment claims wire-side rejection. Verify.
- Does the spec actually constrain identifier_paths to the dotted+[*] grammar loader.ts:127 enforces, or is the SDK stricter than spec? Either way, the fail-closed posture is defensible — but the loader error message should cite the spec section.
Spec-silent on purpose enum (recorder.ts:175). The classifier now enforces six values and emits classifier_invalid_purpose for the rest. types.ts:56-60 says "when the spec adopts that field" — so this is fail-closed ahead of spec. Defensible, but flag as preview in the changeset, not GA.

Minor nits (non-blocking)

Three-overload computePayloadDigestSha256 signature is awkward. RegExp and false legacy forms plus the canonical PayloadDigestOptions object plus the no-args path is a lot of surface. The @deprecated JSDoc on the legacy forms is the right move; consider removing them in the next major.
Enum-sort rationale. scripts/generate-types.ts:583 sorts enum items lexicographically on their JSON-stringified form. Mixed-type enums like [1, "1"] sort consistently because JSON.stringify runs first. No correctness bug, but a one-line comment on the rationale would save the next reader a minute of head-scratching.

Notable: the PR closes six follow-up issues with one bundled hardening pass. The matching test scaffolding (JCS parity sweep across JSON, form-urlencoded, FormData, Buffer, URLSearchParams, plus maxPayloadBytes boundaries at 0, -1, Infinity) is the right shape for this kind of change.

Safe to merge.

aao-ipr-bot

Approving. Hardening pass holds the witness-not-translator line — every digest-mode field on projectRecordedCall derives deterministically from the recording, and uncanonicalizable entries are dropped through onError instead of synthesized.

Things I checked

projectRecordedCall (src/lib/upstream-recorder/recorder.ts:400-440) is witness-clean. identifier_match_proofs only emits when the controller actually has digests to match; no synthesized empty arrays. payload_length and payload_digest_sha256 share the same canonicalPayloadBytes() output (lines 406-407, 433-434), so the documented BEHAVIOR CHANGE in digest mode is internally coherent.
safeProjectRecordedCall (recorder.ts:387-398) drops uncanonicalizable entries and fires digest_canonicalization_failed. total counts only canonicalizable entries; truncated semantics still match the field doc.
IDENTIFIER_DIGEST_LIMIT = 64 is the spec-bound identifier_value_digests[] cap (verified against src/lib/types/v3-1-beta/tools.generated.ts:22497), not an internal limit. Removing the prior 64-leaf payload-scan cap fixes a real false-negative on large payloads — the new test at test/lib/upstream-recorder-spec-shape.test.js covers 4100 entries.
assertPrenormalizedPayloadSafe (recorder.ts:615-664) fails closed on unredacted secret-shaped keys, hostile cycles terminate via iterative WeakSet, error path emits a developer-facing key chain without adopter values.
redactSecrets depth cap 32 → 256 with WeakSet cycle protection (src/lib/utils/redact-secrets.ts:50-78). Depth-257 path: assertJsonDepth throws → buildRecordedCall catches → payload_build_failed event with a static RangeError message; the buffer never holds the entry. No plaintext leak.
promoteConditionalParamProperties (scripts/generate-types.ts:606-631) hard-fails on conflicting allOf promotions, accepts enum reorders via canonicalCodegenJson. The new test pinning the cached 3.1 comply-test-controller-request.json as a non-conflicting baseline is the right regression anchor.
Two changesets present and categorized correctly: harden-conditional-param-codegen patch (build-time hard-fail, only triggers on actually-conflicting forked schemas), upstream-digest-attestation-hardening minor. minor is defensible — no green-path code is removed; request.* identifier paths never resolved meaningfully against sample_request before, so the load-time rejection is hardening of an authoring error.
Compliance summary's validations_not_applicable is additive, omitted when zero (src/lib/testing/compliance/summary.ts:106-110). Badge consumers can now distinguish clean passes from coverage downgrades.

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

payload_must_contain grading shift is spec-unverifiable from this checkout. The new "any NA → whole-assertion NA" rule (src/lib/testing/storyboard/validations.ts:2790-2805) is more conservative than the prior "all NA → NA" rule. The runner-output-contract.yaml referenced at recorder.ts:81-83 lives in adcontextprotocol/adcp, not this tree. If the spec says "any actionable pass → passed," this over-grades NA. Conservative-incorrect, not security-relevant — but worth confirming against adcp#3816 / #3987 before another adopter inherits the rule. The identifier-path branch correctly guards via hasMeaningfulPayloadPass; the payload-must-contain branch does not.
identifier_paths root scope is SDK-chosen, not spec-defined. Rejecting request.* / response.* / context.* at load time is a stricter rule than adcp#3816 requires — the spec defines the wire shape (identifier_value_digests[] to the controller), not the storyboard authoring syntax. The new prose at src/lib/testing/storyboard/types.ts:1038-1064 calls this out. Either fold the SDK choice into a spec PR or keep documenting it as a runner-side constraint.

Minor nits (non-blocking)

Stringly-typed JCS error detection. validations.ts:2682 matches /JCS: non-finite number\b/ against the controller's error string. Same repo, so the coupling is enforceable, but everywhere else this PR moves toward typed sentinels (PayloadDigestError, UpstreamRecorderErrorEvent.kind). A kind: 'jcs_non_finite' discriminator on the controller error path would let the validator branch on class identity instead of agent-controlled error text.
computePayloadDigestSha256({ redactPattern: false, prenormalized: false }) is an undocumented bypass shape. That options object routes to the redact branch but with redactPattern === false, skipping both assertPrenormalizedPayloadSafe and redaction. Nothing exercises it today, but the type accepts it. Either reject redactPattern: false without prenormalized: true explicitly, or fold both legacy false and redactPattern: false into the prenormalized path.
Identifier-path syntax regex is rebuilt per call. loader.ts:127 constructs pathPattern via new RegExp(...) on each validation. Hoist to a module-level const.

One observation on the surrounding shape: this is the third "final" hardening pass on the digest path in as many commits (fix: address final argus digest follow-ups, fix: close final upstream attestation nits, fix: close final argus follow-ups). The work is real and the diffs are clean — but if a fourth "final" lands next week, it's worth asking whether the spec contract under it is still settling rather than the SDK.

LGTM. Follow-ups noted below.

aao-ipr-bot

Not ready. Three independent paths bypass the prenormalized: true safety scan on computePayloadDigestSha256, and the same helper validates against a narrower pattern than the recorder it claims symmetry with.

The witness-not-translator invariant says the digest must attest what the recorder would have stored. Right now an adopter can hold prenormalized: true while the bytes hashed contain plaintext secrets the scanner never saw.

Blockers

Form-urlencoded duplicate-key bypass. assertPrenormalizedPayloadSafe at src/lib/upstream-recorder/recorder.ts:636-637 does Object.fromEntries(new URLSearchParams(payload)), which collapses duplicates to last-wins. A body authorization=plaintext-secret&authorization=%5Bredacted%5D scans clean (the map shows {authorization: \"[redacted]\"}), but assertPrenormalizedPayloadSafe returns the original payload string verbatim and canonicalPayloadBytes hashes the unredacted bytes. The digest is now an attestation over a payload the safety scan never inspected. Iterate params.entries() and fail closed on any duplicate at a secret-shaped key.
Malformed-JSON-string prenormalized bypass. recorder.ts:628-646. When JSON.parse throws on a JSON-content-type string, scanTarget becomes the raw string, findUnredactedSecretPath skips non-objects at L653, and the unparsed body is returned and hashed verbatim. The recording path has redactJsonLikeSecretValues as a last-resort scrub at L738; the prenormalized path has nothing. Either run the same scrub or throw PayloadDigestError for a malformed JSON body in prenormalized mode.
assertPrenormalizedPayloadSafe uses the default pattern, not the caller's. recorder.ts:604 passes SECRET_KEY_PATTERN instead of the resolved redactPattern. Adopters who extended the pattern with vendor secret keys (x_vendor_token, etc.) get a narrower safety scan than the recorder applies at recording time. The whole API contract here is symmetry — createUpstreamRecorder({ redactPattern }) ↔ computePayloadDigestSha256(..., { redactPattern }) — and the safety scan currently breaks it silently.

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

isSafelyRedactedSecretValue accepts true/false as redaction sentinels. recorder.ts:675-677. An adopter writing { authorization: false } to mean "no auth attached" satisfies the prenormalized contract with an out-of-band convention the spec doesn't define. Restrict to '[redacted]' and null; document the narrowing in the changeset.
redactJsonLikeSecretValues leaks array-element secrets in malformed JSON. recorder.ts:987-997. skipJsonLikeValue stops at the first , inside \"password\":[\"s1\",\"s2\"], so \"s2\"]} is re-scanned and \"s2\" is emitted verbatim. Block comment at L925-931 documents this, but the function is reached on every malformed JSON body in the recording path, not just diagnostics. Scan with bracket-depth tracking, or substitute \"[redacted]\" for any bracket/brace-shaped value.
total reflects kept items, not matched items. recorder.ts:341-353. safeProjectRecordedCall drops uncanonicalizable entries silently; the controller response shows truncated: false, total: N with no on-wire signal that anything was elided. Surface a dropped counter alongside onError so runner-side diagnostics aren't onError-only.
ad-tech-protocol-expert was blocked on schemas/cache/3.1.0-beta.5/**. That directory isn't synced in this worktree, so three SDK-vs-spec items couldn't be graded against authority: payload_length digest-mode semantics at types.ts:46-52, empty host/path strings at types.ts:36-38, and IDENTIFIER_DIGEST_LIMIT = 64 at constants.ts:1. SDK source is internally consistent; before re-requesting review, run npm run sync-schemas so the second pass can grade against the cached spec.
supported_optimization_metrics empty-array logger. src/lib/server/decisioning/runtime/from-platform.ts around the new omit-empty branch — when an adopter explicitly passes [] to opt out, the log line still says "using explicit override". Guard on explicitSom.length > 0.
Conditional codegen enum-equivalence test. scripts/generate-types.ts:572-590. The enum sort comparator stringifies-then-sorts, which is correct for mixed-type enums like [1, \"1\"] and [true, \"true\"] but worth a fixture to lock the behavior against future refactors.

Things I checked

Three changesets present (harden-conditional-param-codegen.md patch, upstream-digest-attestation-hardening.md minor, resolve-3-1-blocked-issues.md modified). Types match impact — minor correctly covers the new public exports (computePayloadDigestSha256, PayloadDigestError, PayloadDigestOptions) and the digest-mode payload_length behavior change. No manual package.json version edit.
Largest non-generated file rule: read src/lib/upstream-recorder/recorder.ts end to end (522 net lines).
RecordedCall / RawRecordedCall / DigestRecordedCall moved from test-controller.ts to upstream-recorder/types.ts and re-exported. No circular import — test-controller.ts imports from ../upstream-recorder/types.
validations_not_applicable summary field gated behind truthy check (summary.ts:106-109). Badge consumers keying on field presence stay backward-compatible.
validateUpstreamIdentifierPaths at loader.ts:83-133 rejects request.* / response.* / context.* (case-insensitive), bracket-quoted, recursive descent, and numeric-index forms. runStoryboardStep now invokes validateStoryboardShape for programmatic callers.
Empty supported_optimization_metrics arrays omitted from the wire response (from-platform.ts:991-994).
redactSecrets depth cap 32 → 256 with WeakSet cycle protection; default-param seen is freshly allocated per top-level call and propagated through recursion. Correctly scoped.
safeProjectRecordedCall drop-on-error path emits digest_canonicalization_failed and preserves the rest of the query. NaN-payload test confirms behavior.

Three independent ways past the same guardrail in one PR is noteworthy. Fix the prenormalized scanner so the digest cannot attest bytes the scanner never inspected, then this is good.

aao-ipr-bot · 2026-05-27T08:46:51Z

⚠️ Argus review could not complete

The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final gh pr review). A human reviewer should take this PR.

View workflow run

_{This is an automated message from the Argus AI review workflow.}

bokelley · 2026-05-27T08:57:20Z

Heads-up from triage: issue #2056 ("Expose dropped digest projection count in upstream recorder query diagnostics") proposes adding a dropped_count to UpstreamRecorderQueryResult + toQueryUpstreamTrafficResponse to surface how many entries were elided by the fail-closed canonicalization path this PR introduces.

The scope is small (one new optional field on the result type + noop-recorder update; wire projection gated on the schema allowing it) and the diff naturally belongs here before merge — same file, same author, same feature area. Worth folding if the review iteration permits; otherwise #2056 resurfaces when this PR closes.

Generated by Claude Code

Add `dropped_count: number` to `UpstreamRecorderQueryResult` to surface the number of digest-mode entries silently dropped by canonicalization failure. The counter is explicitly gated on `attestationMode === 'digest'` so raw-mode queries always return 0 structurally, not as a side-effect of throw absence. Wire projection onto `toQueryUpstreamTrafficResponse` / `UpstreamTrafficSuccess` is intentionally withheld — the spec wire schema does not yet define this field. The counter is a diagnostic surface only; `onError` with `digest_canonicalization_failed` remains the primary observable. Noop recorder returns `dropped_count: 0`. Folds issue #2056 (Expose dropped digest projection count in upstream recorder query diagnostics) into this PR before merge. Refs #2056

bokelley · 2026-05-27T09:13:05Z

Folded #2056 into this PR. Two commits pushed to resolve-more-3-1:

feat(upstream-recorder): expose dropped_count in query result — adds dropped_count: number to UpstreamRecorderQueryResult; the counter increments only when attestationMode === 'digest' (explicit guard, not a side-effect of throw absence), so raw-mode queries are structurally guaranteed to return 0.
style: apply prettier formatting — Prettier formatting fix the pre-push hook caught.

Wire projection onto toQueryUpstreamTrafficResponse / UpstreamTrafficSuccess is intentionally withheld — the existing deepEqual key-order test pins the wire shape, and the spec hasn't defined the field. The existing validateUpstreamTrafficSuccess schema test is extended to assert dropped_count is absent from the wire shape.

Tests added (3): NaN-payload digest drop (covers the onError correlation), raw-mode zero invariant (records a NaN entry to make the title accurate), and noop recorder.

Changeset added: .changeset/upstream-recorder-dropped-count.md (minor — additive optional field).

Applied by Claude Code

Generated by Claude Code

bokelley · 2026-05-27T09:14:25Z

Acknowledged. The dropped_count design looks well-reasoned — explicit attestationMode === 'digest' guard rather than throw-absence inference keeps the raw-mode zero invariant structural, and withholding wire projection until the spec defines the field is the right call (the deepEqual key-order test as a wire-shape pin is a good safety net). CI is in progress on the new commits; no action from me until checks complete.

Triaged by Claude Code. Session: https://claude.ai/code/session_014MDNQAfQaNpCVWZ2QrUjHW

Generated by Claude Code

aao-ipr-bot

LGTM. Strict improvement on every axis — the malformed-JSON path that previously stored secret strings verbatim now redacts scalar values, the digest pipeline gates non-canonicalizable payloads instead of producing wrong hashes, and the codegen conflict check fails loud on the pattern that silently first-wins'd before.

Things I checked

assertJsonDepth fires after redactSecrets in every JSON / structured branch (recorder.ts:746, 762) — record-time and digest-projection both reject deeper trees before any non-canonical digest could form.
computePayloadDigestSha256 runtime branching at recorder.ts:587-606: all three signatures (object form, bare RegExp, bare false) land in the correct branch. Bare-false routes to assertPrenormalizedPayloadSafe via the redactPattern === false check; the L598-L606 cross-field guard only fires for the object form, but the bare-false path is already safe because it triggers the scanner.
Cross-principal isolation: query hard-filters by entry.principal === params.principal at recorder.ts:334 before any projection. safeProjectRecordedCall only ever sees same-principal entries.
dropped_count is on the adopter-facing UpstreamRecorderQueryResult, NOT on the wire UpstreamTrafficSuccess. Right call — don't ship a field the spec schema hasn't blessed. Wire projection explicitly gated on schema adoption (types.ts:296-307).
RawRecordedCall / DigestRecordedCall discriminated-union shape matches the cached 3.1 comply-test-controller-response.json branch-required fields; the RecordedCall re-export from test-controller.ts collapses the prior duplicate definition without changing wire semantics.
Four changesets present and audited against wire impact: harden-conditional-param-codegen (patch — internal codegen behavior), resolve-3-1-blocked-issues (minor + BREAKING — pre-mode beta rolling into 8.0.0-beta.x, so the minor type is correct), upstream-digest-attestation-hardening (minor), upstream-recorder-dropped-count (minor).

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

redactJsonLikeSecretValues leaks nested secret values inside malformed JSON. recorder.ts:931-1003. For input like {"token": {"inner": "real-secret-xxx" (truncated upstream JSON), the substitution emits "[redacted]" at token but skipJsonLikeValue stops at the first , } ] or whitespace — it does not balance-skip a { value. The outer loop then walks into the object body and copies the inner string verbatim. Reachable when an upstream returns truncated JSON (server crash mid-write, proxy buffer cut, oversize stream); plaintext lands in RecordedCall.payload shipped to compliance runners via query_upstream_traffic. The function comment at L932-L937 documents the limitation — documenting a leak isn't fixing it. Fix: balance-skip { and [ (depth counter + string/escape state) in skipJsonLikeValue, emit onError when the scrub fires so adopters know storage was lossy. Strict-improvement-over-status-quo (prior code stored malformed JSON verbatim with no scrub), so non-blocking — but worth landing before the next beta.
Codegen conflict check normalizes enum but not required. scripts/generate-types.ts:577-595. canonicalCodegenJson sorts only enum arrays; required: [a,b] vs required: [b,a] are semantically equal in JSON Schema but would now throw Conflicting conditional params property. Zero upstream schemas hit this today; fix is one line (if (keyHint === 'enum' || keyHint === 'required') items.sort()).
redactSecrets callers outside the recorder skip assertJsonDepth. ~11 sites in storyboard/runner.ts walk attacker-derived response bodies. The recorder gates post-redaction; runner callers don't. CPU bound on a hostile 256-deep tree is 8× looser than the prior 32-deep cap. Either pre-gate at every caller, or have redactSecrets substitute [depth-exceeded] at the cap instead of passing the tree through unchanged.
computePayloadDigestSha256 overload surface. Three signatures plus a cross-field runtime invariant (redactPattern: false requires prenormalized: true). Drop redactPattern: false from the public type, keep only { prenormalized: true }, coerce bare RegExp and bare false internally. Single published overload. Agents reading the .d.ts will otherwise pick the deprecated form.
Soft-deprecation silence. @deprecated JSDoc on the bare-RegExp / bare-false overloads only fires in IDEs that surface it. A one-time console.warn per process on first deprecated-form call catches the upgrade-from-beta path.
PayloadDigestError.cause: unknown shadows Node 16+ built-in. recorder.ts:43. Use super(message, cause !== undefined ? { cause } : undefined) and drop the explicit field.
validateStoryboardShape newly called from runStoryboardStep. storyboard/runner.ts:3203. Programmatic callers building partial Storyboard literals without phases will now throw TypeError at entry instead of mid-run. Right behavior — but runStoryboardStep's JSDoc should call out the shape contract so the failure mode is obvious from the call site, not from the loader stack.

Minor nits (non-blocking)

Doc the undefined → {} raw-payload fabrication. recorder.ts:711-716. The 3.1 schema requires payload on the raw branch, so GET/HEAD bodies emit {} instead of being omitted. Comment is there; adopter-facing docs should call it out — a reviewer chasing a Content-Length: 0 discrepancy will land on this and assume witness violation.
Object.fromEntries(new URLSearchParams(payload)) collapses duplicate keys to the last value. recorder.ts:642. Not a correctness issue for findUnredactedSecretPath (any secret-shaped key triggers the throw), but worth a one-line comment so a future reader doesn't assume the scanner sees every original entry.

Bundling codegen + sponsored-intelligence + digest hardening + summary cleanup closes six issues in one merge. The upstream-digest-attestation-hardening changeset still reads as eight paragraph-length topics under one minor bump — split-on-next-bundle worth considering, so adopters can cherry-pick which behavior change affects them without reading all of it.

Safe to merge.

aao-ipr-bot

Approving on the strength of the codegen conflict detector plus the fail-closed digest projection. The witness invariant holds — every drop path emits an onError event instead of fabricating a sentinel digest, and toQueryUpstreamTrafficResponse correctly omits the SDK-internal dropped_count from the wire shape.

Things I checked

computePayloadDigestSha256 overload matrix at src/lib/upstream-recorder/recorder.ts:576-660. Bare-RegExp and bare-false legacy forms route to the same normalize-or-prenormalized branches as the options form. The redactPattern: false, prenormalized: undefined combo throws at L625 — won't silently bypass the safety scan.
Prenormalized scan floor. prenormalizedSafetyPattern at L606-612 defaults to SECRET_KEY_PATTERN. Adopters cannot narrow below the canonical floor; the worst case is over-flagging on a key the adopter already redacted under a broader pattern. Right shape.
Digest projection fail-closed. safeProjectRecordedCall at L397-408 drops the entry on canonicalization throw and surfaces digest_canonicalization_failed via onError. A buyer querying the controller sees fewer rows rather than a forged digest. Storyboard runner grades the resulting absence as missing, which is the correct AdCP 3.1 reading.
Identifier-proof DFS at L463-484 returns only matched digest set — no plaintext, no path, no positional info. Depth-capped at MAX_JSON_DEPTH=256. Early-exit on matched.size === wantedDigests.size. Clean.
Form-urlencoded duplicate-secret rejection at L685-706. Enumerates Array.from(URLSearchParams) to preserve duplicates, then dup-check before value-shape check. Last-wins maps can't prove every original value was inspected — right two-pass order.
Wire shape parity in test/lib/upstream-recorder-spec-shape.test.js:715-779: assert.ok(!Object.prototype.hasOwnProperty.call(wireShape, 'dropped_count')). Spec drift caught at the wire boundary.
Codegen conflict detector at scripts/generate-types.ts:606-631. Reordered-enum equivalence allows refactors; conflicting promoted shapes throw with the originating allOf[] index. JSON Schema semantics require allOf branches to be jointly satisfiable — previous silent-first-wins was masking the bug.
identifier_paths prefix rejection in src/lib/testing/storyboard/loader.ts rejects request./response./context. — identifier scanning must target the wire-sent payload, not runner-local context. Right narrowing, caught at load time.
Changesets present for all three behavior categories. Two minor (additive PayloadDigestOptions + dropped_count field), one patch (codegen) — appropriate impact mapping.

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

dropped_count is not wired into runner grading. grep -rn dropped_count src/lib/testing returns zero hits. The PR description's "tightens storyboard upstream traffic grading for mixed raw/digest" still derives missing from empty recorded_calls, not from dropped_count. The field is adopter-observable via query() and through onError events, which is useful, but the grading wire-through described in prose doesn't exist yet. Either wire dropped_count into the storyboard verdict (so canonicalization failures grade attestation_unavailable instead of collapsing into missing), or update the changeset prose to "adopter-observable" only.
RecordedCallBase[key: string]: unknown comment is misleading against the current cache. src/lib/upstream-recorder/types.ts:25-30 says adopter-defined fields are "still rejected by the current 3.1 wire schema," but UpstreamTrafficSuccessSchema in src/lib/types/schemas.generated.ts uses .passthrough() — additional fields propagate at runtime. The footgun is real for adopters running closed validators downstream. Tighten the comment to "MAY be enforced by closed adopter validators" or close the schema upstream.
Malformed-JSON tokenizer has a narrow residual leak. redactJsonLikeSecretValues at recorder.ts:986-1077 correctly scrubs the common cases (the new test at test/lib/upstream-recorder-spec-shape.test.js proves it for bracket-shaped values), but a body like {"foo":"unterminated, "authorization":"actual-bearer"} loses parser sync after the first unterminated string and dumps the bearer verbatim. This path is strictly better than pre-PR (the previous code returned malformed JSON unredacted, no scrub at all) — so it's not a regression. But fail-closed is the witness invariant: when pattern.test(body) is true on a body that JSON.parse rejected, consider substituting [unredactable malformed json N bytes] rather than emitting a best-effort scrub. The buffer never holding plaintext bearers is worth the diagnostic loss.
redactSecrets DAG re-walk. src/lib/utils/redact-secrets.ts:75,88 — seen.delete(value) after the recursive call means a shared subtree referenced from N siblings is walked N times. Real cycles still get caught. Crafted DAGs with diamond fan-out under MAX_JSON_DEPTH=256 can balloon work exponentially. Lower priority than #3, but consider keeping seen.add and dropping seen.delete so shared subtrees collapse to [Circular] after the first walk.
findUnredactedSecretPath has no depth cap. recorder.ts:708-733 is iterative so it won't blow the stack, but a deeply-nested adversarial prenormalized payload could exhaust memory. canonicalPayloadBytes's assertJsonDepth (L519) is the next gate, so the attack window is narrow. Worth applying MAX_JSON_DEPTH here for consistency.

Minor nits (non-blocking)

Single global production warning at recorder.ts:888-901. warnedAboutProduction is module-scoped. Multi-tenant servers with N misconfigured tenants get exactly one warning total. Per-recorder dedup would surface the issue per construction.
Codegen changeset bump level at .changeset/harden-conditional-param-codegen.md. The new throwing failure mode is a behavior change for adopters with forked schemas. patch defends as "fixing latent bug" — borderline minor. Live with it.

The expert pass converged: code-reviewer, security-reviewer, and the protocol expert all read this as a hardening pass that does what it claims, with the same three observability deltas above. Notable that two of them independently caught the dropped_count-vs-runner-grading prose drift — worth noting for future "what did the prose claim" sanity checks.

Approved.

bokelley changed the title ~~Harden 3.1 digest attestations and codegen~~ fix: harden 3.1 digest attestations and codegen May 27, 2026

bokelley marked this pull request as ready for review May 27, 2026 05:10

bokelley added 5 commits May 27, 2026 01:18

fix: harden conditional params codegen

4575053

fix: harden upstream digest attestations

2b12b03

test: cover 3.1 harness nits

fb1da75

docs: clarify 3.1 digest attestations

4e9ab53

fix: address expert 3.1 review findings

11e57ac

aao-ipr-bot Bot previously approved these changes May 27, 2026

View reviewed changes

bokelley force-pushed the resolve-more-3-1 branch from 8872789 to 11e57ac Compare May 27, 2026 05:20

fix: address 3.1 attestation review nits

76366ce

bokelley dismissed aao-ipr-bot[bot]’s stale review via 76366ce May 27, 2026 05:53

aao-ipr-bot Bot previously approved these changes May 27, 2026

View reviewed changes

fix: close 3.1 attestation follow-up nits

80d8b6d

bokelley dismissed aao-ipr-bot[bot]’s stale review via 80d8b6d May 27, 2026 06:30

aao-ipr-bot Bot previously approved these changes May 27, 2026

View reviewed changes

fix: close final 3.1 review follow-ups

26ecd28

bokelley dismissed aao-ipr-bot[bot]’s stale review via 26ecd28 May 27, 2026 06:48

bokelley mentioned this pull request May 27, 2026

Clarify mixed raw/digest payload_must_contain evidence semantics adcontextprotocol/adcp#5070

Open

aao-ipr-bot Bot requested changes May 27, 2026

View reviewed changes

fix: fail closed for deep json string attestations

46ab685

aao-ipr-bot Bot previously approved these changes May 27, 2026

View reviewed changes

This was referenced May 27, 2026

Clarify digest-mode upstream_traffic payload_length semantics adcontextprotocol/adcp#5072

Closed

Document portable identifier_paths grammar for upstream_traffic adcontextprotocol/adcp#5073

Closed

fix: close final 3.1 approval follow-ups

4592330

bokelley dismissed aao-ipr-bot[bot]’s stale review via 4592330 May 27, 2026 07:26

github-advanced-security AI found potential problems May 27, 2026

View reviewed changes

Comment thread src/lib/upstream-recorder/recorder.ts Fixed

fix: avoid regex fallback in json redaction

85901aa

aao-ipr-bot Bot requested changes May 27, 2026

View reviewed changes

aao-ipr-bot Bot previously approved these changes May 27, 2026

View reviewed changes

bokelley mentioned this pull request May 27, 2026

Preserve RawAttestation/DigestAttestation branch fields in generated types #2053

Closed

fix: close final argus follow-ups

a874199

bokelley dismissed aao-ipr-bot[bot]’s stale review via a874199 May 27, 2026 07:48

aao-ipr-bot Bot previously approved these changes May 27, 2026

View reviewed changes

fix: close final upstream attestation nits

d808294

bokelley dismissed aao-ipr-bot[bot]’s stale review via d808294 May 27, 2026 08:00

aao-ipr-bot Bot previously approved these changes May 27, 2026

View reviewed changes

bokelley mentioned this pull request May 27, 2026

Remove legacy computePayloadDigestSha256 overloads in the next major #2054

Open

fix: address final argus digest follow-ups

24fb747

bokelley dismissed aao-ipr-bot[bot]’s stale review via 24fb747 May 27, 2026 08:21

aao-ipr-bot Bot previously approved these changes May 27, 2026

View reviewed changes

bokelley mentioned this pull request May 27, 2026

Use typed controller error metadata for non-finite JCS digest downgrades #2055

Closed

fix: close digest helper option nits

57b6e84

bokelley dismissed aao-ipr-bot[bot]’s stale review via 57b6e84 May 27, 2026 08:38

aao-ipr-bot Bot requested changes May 27, 2026

View reviewed changes

bokelley mentioned this pull request May 27, 2026

Expose dropped digest projection count in upstream recorder query diagnostics #2056

Closed

claude added 2 commits May 27, 2026 09:08

style: apply prettier formatting to test file

e3acc8e

aao-ipr-bot Bot previously approved these changes May 27, 2026

View reviewed changes

fix: harden prenormalized digest scanning

5e4eac2

bokelley dismissed aao-ipr-bot[bot]’s stale review via 5e4eac2 May 27, 2026 09:27

aao-ipr-bot Bot approved these changes May 27, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into pr-2052-update-2052

9b7450b

bokelley merged commit 80f255b into main May 27, 2026
16 checks passed

bokelley deleted the resolve-more-3-1 branch May 27, 2026 09:53

Uh oh!

Conversation

bokelley commented May 27, 2026

Summary

Validation

Follow-up

Uh oh!

aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

Things I checked

Follow-ups (non-blocking)

Minor nits (non-blocking)

Uh oh!

aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

Things I checked

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

Minor nits (non-blocking)

Uh oh!

aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

Things I checked

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

Minor nits (non-blocking)

Uh oh!

aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

MUST FIX

Things I checked

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

Minor nits (non-blocking)

Uh oh!

aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

Things I checked

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

Minor nits (non-blocking)

Uh oh!

Uh oh!

aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

Blocking

Follow-ups (non-blocking)

Things I checked

Uh oh!

aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

Things I checked

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

Minor nits (non-blocking)

Uh oh!

aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

Things I checked

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

Minor nits (non-blocking)

Uh oh!

aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

Things I checked

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

Minor nits (non-blocking)

Uh oh!

aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

Things I checked

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

Minor nits (non-blocking)

Uh oh!

aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

Blockers

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

Things I checked

Uh oh!

aao-ipr-bot Bot commented May 27, 2026

Uh oh!

bokelley commented May 27, 2026

Uh oh!

bokelley commented May 27, 2026