Skip to content

fix: harden 3.1 digest attestations and codegen#2052

Open
bokelley wants to merge 14 commits into
mainfrom
resolve-more-3-1
Open

fix: harden 3.1 digest attestations and codegen#2052
bokelley wants to merge 14 commits into
mainfrom
resolve-more-3-1

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

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

@bokelley bokelley changed the title Harden 3.1 digest attestations and codegen fix: harden 3.1 digest attestations and codegen May 27, 2026
@bokelley bokelley marked this pull request as ready for review May 27, 2026 05:10
aao-ipr-bot[bot]
aao-ipr-bot Bot previously approved these changes May 27, 2026
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

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-620promoteConditionalParamProperties 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-437projectRecordedCall 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-471collectMatchedStringLeafDigests 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-105request.* / 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)

  1. 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.
  2. 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.
  3. 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.

@bokelley bokelley force-pushed the resolve-more-3-1 branch from 8872789 to 11e57ac Compare May 27, 2026 05:20
aao-ipr-bot[bot]
aao-ipr-bot Bot previously approved these changes May 27, 2026
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

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 stringstring | 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.
  • canonicalPayloadBytesparseJsonStringPayloadForScan 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)

  1. 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.
  2. 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).
  3. 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[bot]
aao-ipr-bot Bot previously approved these changes May 27, 2026
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

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)

  1. 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.

  2. 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.

  3. 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.

  4. JCS: non-finite numbernot_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.

  5. 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)

  1. 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.

  2. 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.

  3. 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.

Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

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:

  1. JSON.parse(payload) succeeds (V8 parses thousands of levels).
  2. redactSecrets walks to depth 256 and returns the deeper subtree verbatim, including any authorization / token / cookie keys past depth 256.
  3. assertJsonDepth(redacted) throws RangeError at line 590.
  4. The bare catch at line 596 swallows it.
  5. 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-627promoteConditionalParamProperties 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-994supported_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)

  1. 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.
  2. 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.
  3. .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[bot]
aao-ipr-bot Bot previously approved these changes May 27, 2026
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

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)

  1. 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.
  2. 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.
  3. 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().
  4. /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.
  5. 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.
  6. 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)

  1. 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.
  2. 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.
  3. 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).

Comment thread src/lib/upstream-recorder/recorder.ts Fixed
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

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

  1. 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.
  2. 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.
  3. 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-63RawRecordedCall.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:339since_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-414RecordedCall.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-590canonicalCodegenJson 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-117validateIdentifierPathSyntax 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-131validateUpstreamIdentifierPaths 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[bot]
aao-ipr-bot Bot previously approved these changes May 27, 2026
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

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-336total++ 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-581computePayloadDigestSha256 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)

  1. 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.
  2. 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."
  3. 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[bot]
aao-ipr-bot Bot previously approved these changes May 27, 2026
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

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 contributescontributes_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)

  1. 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[bot]
aao-ipr-bot Bot previously approved these changes May 27, 2026
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

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)

  1. 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_lengthbyteLengthOf(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.

  2. 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.

  3. 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.

  4. 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.

  5. 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.

  6. /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.

  7. 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.
  8. 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)

  1. 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.

  2. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants