fix: harden 3.1 digest attestations and codegen#2052
Conversation
There was a problem hiding this comment.
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—promoteConditionalParamPropertiescorrectly distinguishes "duplicate identical refinement" (passes) from "conflicting promotion" (throws withallOf[N]index). Root-param refinements are intentionally ignored; new test attest/type-generator-allof-ref-merge.test.js:240-275pins that contract. Cached 3.1 comply-controller schema round-trips green.src/lib/upstream-recorder/recorder.ts:404-437—projectRecordedCallnow reportspayload_lengthas the canonical byte stream covered bypayload_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—collectMatchedStringLeafDigestsis behaviorally equivalent to the priorjsonStringLeafDigests+ 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 viautils/jcs.canonicalize, throws on depth > 256.index.ts:50-56docstring tells controller handlers to surface as a failedquery_upstream_trafficresponse. Fail-closed beats fail-open.src/lib/testing/storyboard/validations.ts:2770-2790— walked the four-quadrant matrix foranyPayloadNotApplicable/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 asidentifierDigestLimitExceededwithclippedcount, runner gradesnot_applicablewith the count in the note instead of silently passing or failing. Matches #2047 acceptance. src/lib/server/decisioning/from-platform.ts:991— emptysupported_optimization_metricsnow omitted from the wire capabilities response. Doc oncapabilities.ts:140-165says 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) andupstream-digest-attestation-hardening(minor, new public export + behavior change). Categories are right.
Follow-ups (non-blocking)
- Expand the
upstream-digest-attestation-hardeningchangeset body with two adopter-affecting deltas the current prose buries: (a)RecordedCallBase.host/.pathflipped from required to optional inupstream-recorder/types.ts:28-29— runtime is unaffected because the recorder always populates them fromnew URL(url), butstrict: trueconsumers ofRecordedCallimported from@adcp/sdk/upstream-recorderwill now seestring | undefined. The shape was already loose through thetesting/test-controllerre-export, so the public surface ends up consistent — worth saying so. (b)payload_lengthsemantics now meaningfully diverge between raw mode (raw recorded length) and digest mode (canonical byte length for the same call). Doc'd intypes.ts:38-43, but worth surfacing in the changeset too. computePayloadDigestSha256redaction footgun.src/lib/upstream-recorder/recorder.ts:540-542ships 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 tocomputePayloadDigestSha256AfterRedactionto put the warning at the call site, or accept an optionalredact?: RegExpdefaulting toSECRET_KEY_PATTERN. The latter is the safer default.assertJsonDepthis recursive, thencanonicalizerecurses again.recorder.ts:505-518is one recursion to 256, thenjcs.ts:serializerecurses to the same depth on success. Safe at 256 against Node's default stack, butcollectMatchedStringLeafDigestsis 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:198pins'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 = 64betweenrunner.ts:5159and the implicit cap insiderecorder.ts:445'snormalizeIdentifierDigests. Drift risk later.
Minor nits (non-blocking)
- Bracket-notation paths bypass
validateUpstreamIdentifierPaths.src/lib/testing/storyboard/loader.ts:104splits on/[.[\]]/, so["request"].xand$..request.xslip 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. canonicalCodegenJsonvsutils/jcs.canonicalize.scripts/generate-types.ts:575-587ships a local canonicalizer instead of reusingjcs.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-275covers 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.
8872789 to
11e57ac
Compare
There was a problem hiding this comment.
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
minorcovers a new public export and a type widening (host/pathstring→string | undefinedonupstream-recorder/types.ts:29-32) — the recorder still always emits strings (recorder.ts:164-165initializes both to''), so the runtime shape is unchanged; the type change is a relaxation aligning with the loosercomply-test-controller-response.jsonshape. 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-expertverified againstcomply-test-controller-response.json > UpstreamTrafficSuccessand confirmed sound. - Discriminated-union conformance.
RawRecordedCallrequirespayloadand typespayload_digest_sha256?: never;DigestRecordedCallflips it. Matches theRawAttestation/DigestAttestationbranches in the cached schema. TheadditionalProperties: falseenvelope is honored. IDENTIFIER_DIGEST_LIMIT = 64(constants.ts:1) is the spec's normativemaxItems: 64on bothidentifier_value_digests(request) andidentifier_match_proofs(response). Right number.collectUpstreamIdentifierDigestsclipped accounting (runner.ts:5224-5246) only stampsidentifierDigestLimitExceededwhenclipped > 0. Correct guard.computePayloadDigestSha256overload runtime check (recorder.ts:505-510) — verified for all three forms (RegExp,false, options object). Thefalseshort-circuit preserves the option for adopters who pre-normalize. The JSDoc warns; the?? SECRET_KEY_PATTERNcorrectly distinguishes explicitfalsefromundefined.canonicalPayloadBytes↔parseJsonStringPayloadForScansymmetry. Both fall through to the raw string on JSON.parse failure, so identifier scanning and digest computation see the same view. No drift.enforceStrictSchemacodegen throw.promoteConditionalParamProperties(scripts/generate-types.ts:602-626) now ignores root-params.propertiesrefinements (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-581takes thetypeof payload === 'string'branch and goes straight tocap()—redactSecretsis never called.wrapFetchis safe (parses first viareadBody); only the manual-instrumentationrecord({ payload: JSON.stringify({...}), content_type: 'application/json' })path holds the unredacted string in the buffer.computePayloadDigestSha256correctly avoids this by routing throughnormalizeFetchPayloadInput— 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 ofnormalizeRecordedPayload, symmetric with the form-urlencoded path. Pin both paths with an equality assertion.- Redaction depth cap (32) vs canonicalization depth cap (256).
redactSecretsreturns the subtree un-rewritten past depth 32;assertJsonDepththen accepts the structure (35 < 256) and hashes it verbatim. Pre-existing, but the newcomputePayloadDigestSha256export makes the asymmetry more reachable. Cheapest fix is to raiseREDACT_MAX_DEPTHto matchMAX_JSON_DEPTH; alternatively, haveredactSecretsthrow past its cap rather than silently returning the subtree. validateStoryboardShapenot invoked fromrunStoryboardStep. The newvalidateUpstreamIdentifierPathsonly fires throughparseStoryboard/runStoryboard. Programmatic single-step callers bypass it. AddvalidateStoryboardShape(storyboard)near the top ofrunStoryboardStep— the helper is idempotent.purposefield 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]underadditionalProperties: false. An adopter classifier returning'foo'produces an item that fails spec-shape validation. Clamp the classifier return to the enum (orundefinedoff-enum) before stamping and surface viaonError.- NaN/Infinity digest path grades
failedinstead ofnot_applicable. Spec normatively requires the runner to grade non-finite-number payloadsnot_applicablein digest mode. Today:jcs.tsthrows → controller emitsINTERNAL_ERROR→ runner gradesfailed. 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'sformat: date-timeonUpstreamTrafficSuccess.since_timestamp. Adopters who gateenabledcorrectly never hit it; still worth substituting a sentinel likenew Date(0).toISOString().
Minor nits (non-blocking)
- Codegen conflict error message names only the losing branch.
scripts/generate-types.ts:620-622citesallOf[${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/Infinitytest labels are misleading.test/lib/upstream-recorder-spec-shape.test.js:319-328exercises both, butclamp()(recorder.ts:47-55) collapses both toDEFAULT_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 exercisemaxPayloadBytes: 0, which actually disables truncation percap()(line 772).compliance/summary.ts:273regex change drops trailing-whitespace stripping by reshaping[^;(]+→[^;(]*[^ ;(]. Pure-whitespace payloads ("missing_required_tool_family: needs ") now fall through to the barerequirement_unmetcausal 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.
There was a problem hiding this comment.
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.
normalizeRecordedPayloadis the new chokepoint for raw + digest;payload_lengthmath reconciles viaBuffer.byteLength(_, 'utf8')on both paths (src/lib/upstream-recorder/recorder.ts:148-151,:397-403).readBodycorrectly clones beforeformData()/text()so the originalRequeststream stays intact fororiginalFetch(recorder.ts:708-718).assertJsonDepthis iterative, throwsRangeError, propagates throughquery()to the test that pins the message text (upstream-recorder-spec-shape.test.js:444-447). computePayloadDigestSha256as a public export. TheRegExp | false | { ... }overload disambiguation handles all four runtime shapes — includingoptions = nulldefaulting throughoptions?.redactPattern ?? SECRET_KEY_PATTERN(recorder.ts:517-522). Three-shape API surface adequately tested atupstream-recorder-spec-shape.test.js:329-333, 409-428.projectRecordedCallpayload_lengthswap 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 bothallOfindices (scripts/generate-types.ts:602-627). - Storyboard loader prefix rejection.
validateUpstreamIdentifierPathsrejectsrequest.*/response.*/context.*and normalizes$.prefix in place — idempotent on re-validation throughrunStoryboardStep(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-recordersince_timestampepoch fallback is the right correctness fix —''was schema-invalid againstdate-time. - Changesets. Two present, severity right (minor for the digest pipeline, patch for the codegen tightening). Wire-touching changes belong on
@adcp/sdkminor.
Follow-ups (non-blocking — file as issues)
-
redactSecretsdepth-cap leak is asymmetric with digest mode. The redactor returns the subtree verbatim pastREDACT_MAX_DEPTH=256(src/lib/utils/redact-secrets.ts:48) whileassertJsonDepthTHROWS 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, orapplyRedactionToPayloadshould route raw recording through the same depth gate. Pathological depth, sandbox-only recorder, but worth closing. -
anyMatchedCallSatisfiessawDigestCallmasks 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 sosawDigestCallonly 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_applicableis SDK convention, not a spec clause. Documented as "matching the 3.1 runner contract" but I can't find it normatively pinned anywhere inschemas/cache/. The grading is defensible (RFC 8785 §3.2.2.3 forbidsNaN/Infinity); file a spec-clarification PR upstream so the contract pins it. -
IDENTIFIER_DIGEST_LIMIT = 64is a local heuristic. NomaxItemsconstraint oncomply-test-controller-request.json#/identifier_value_digests. Propose the cap upstream so storyboard authors can rely on it portably.
Minor nits (non-blocking)
-
RecordedCallre-export drops the[key: string]: unknownindex signatures the priortest-controller.tsdefinitions 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]: unknownadd onRecordedCallBaserestores 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 toundefinedon parse failure. Today's combination forces consumers to handle both. -
parseJsonStringPayloadForScanfalse-negative on malformed JSON strings (recorder.ts:440-448). When the body failsJSON.parse, the whole string is hashed as a single leaf — substring identifiers won't match. Acceptable trade-off, but worth a line onDigestRecordedCall.identifier_match_proofsso adopters debuggingfound: falsedon't suspect a recorder bug.
Safe to merge.
There was a problem hiding this comment.
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).redactSecretswalks to depth 256 and returns the deeper subtree verbatim, including anyauthorization/token/cookiekeys past depth 256.assertJsonDepth(redacted)throwsRangeErrorat line 590.- The bare
catchat 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—promoteConditionalParamPropertiesthrow-on-conflict viacanonicalCodegenJsonequality. Idempotent redeclaration permitted; conflicts hard-fail with bothallOf[]indices named. Test attest/type-generator-allof-ref-merge.test.js:376-395confirms cachedcomply-test-controller-request.jsonis clean today.RecordedCalllift tosrc/lib/upstream-recorder/types.tswithtest-controller.tsre-export — surface preserved, no importer broken.RawRecordedCall.payload = {}for undefined bodies +host/pathas required strings emitting""on parse failure — matches the 3.1UpstreamTrafficSuccessdiscriminated arms.payload_lengthswitched tobyteLengthOf(canonicalPayloadBytes(...))for digest mode — the only value a runner can sanity-check againstpayload_digest_sha256. Test atupstream-recorder-spec-shape.test.js:168-171locks it.src/lib/testing/storyboard/loader.ts:80-119validateUpstreamIdentifierPaths— rejectsrequest.*/response.*/context.*prefixes and bracket / recursive forms before they silently resolve toundefined.runStoryboardStepnow runs the same shape validation as full runs.src/lib/server/decisioning/runtime/from-platform.ts:988-994—supported_optimization_metricsempty-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-324is unchanged. Digest projection runs on a single matched entry after filtering and cannot leak cross-principal. - Changeset scoping:
harden-conditional-param-codegencorrectlypatch(codegen is build-time, cached schemas don't trip the new throw);upstream-digest-attestation-hardeningcorrectlyminor(new exportcomputePayloadDigestSha256, behavior tightening, no removal).
Follow-ups (non-blocking — file as issues)
- JCS non-finite error grading is string-coupled.
validations.ts:2682grepsquery.payload.errorfor/JCS: non-finite number\b/, which is the message emitted by this SDK'ssrc/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 againstadcontextprotocol/adcpfor a normativeJCS_NON_FINITE_NUMBERerror 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 passfalsethinking 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 reportfound: falsefor every vector. Practically mitigated by pairing identifier checks withattestation_mode_required: 'raw', but worth emitting anonErrorevent whenJSON.parsefails on a JSON content-type so adopters can detect the tamper. identifier_digest_limit_exceeded.clippedcounts encounters, not distinct vectors (runner.ts:5239-5242). Track aSet<string>of clipped vectors and increment only on first sight so theclipped: Nvalue the validation surfaces means what its name says.
Minor nits (non-blocking)
validations.ts:2719-2724comment is stale. Prose still says "the assertion gradesnot_applicableonly 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;onRecordedCallBase. Weakens typo detection forprojectRecordedCall— a futurepayload_lenghtwould compile. Either drop the index signature (no external importer readsRecordedCallBasedirectly — the re-export skips it) or add a one-line comment noting it's for spec-additive forward compat..changeset/harden-conditional-param-codegen.mdprose. Worth one extra line warning adopters who maintain forked schemas that codegen will now hard-fail on conflictingallOf[].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.
There was a problem hiding this comment.
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 —computePayloadDigestSha256is a new public export fromsrc/lib/upstream-recorder/index.ts:64) andharden-conditional-param-codegen.md(patch — codegen hard-fail on conflicting promotions). Type matches wire impact. - JCS adoption is fail-closed.
recorder.ts:476-479routes throughsrc/lib/utils/jcs.ts; non-finite numbers throwJCS: non-finite number …rather than emit a non-canonical digest. Storyboard runner detects this on the controller envelope and gradesnot_applicableatvalidations.ts:2680-2695. - Depth gate is now consistent.
REDACT_MAX_DEPTH = 256inredact-secrets.ts:39matchesMAX_JSON_DEPTH = 256inrecorder.ts:32. Recorder callsassertJsonDepth(redacted)afterredactSecrets(recorder.ts:614and:630) and converts a deeper structured payload intopayload_build_failed— call is dropped rather than stored with unvisited subtrees. - Identifier-proof early termination is sound.
collectMatchedStringLeafDigestsatrecorder.ts:421-442only short-circuits whenmatched.size === wantedDigests.size. Verified against the 4100-entry test attest/lib/upstream-recorder-spec-shape.test.js— themissingdigest forces full-payload traversal. - Codegen hard-fail handles the right cases.
scripts/generate-types.ts:572-621rejects conflicting siblingallOf[].then.properties.params.properties.*promotions while leaving root-definedparamskeys untouched (test fixture exercises the cached 3.1 controller-request schema). identifier_pathsscope check fires for bothrunStoryboardandrunStoryboardStep(runner.ts:3203).identifier_match_proofscap matches spec.IDENTIFIER_DIGEST_LIMIT = 64aligns with the 3.1query_upstream_trafficrequest schema's documented "Cap of 64 digests per query."- Principal isolation is unchanged.
recorder.ts:323-328still filters onentry.principal === params.principalfirst;url_parse_failedstoring emptyhost/pathcannot widen a cross-principal query.
Follow-ups (non-blocking — file as issues)
payload_lengthsemantics in digest mode are SDK-defined, not spec-pinned. Digest projection atrecorder.ts:402measuresbyteLengthOf(payloadBytes)of the canonical bytes — consistent withpayload_digest_sha256, but the 3.1comply-test-controller-response.jsondoesn't normatively pin whether digest-modepayload_lengthshould report raw emitted bytes or canonical bytes. File a spec issue onadcontextprotocol/adcpto disambiguate before more adopters ship divergent measurements. Compensatingnot_applicablegrading atvalidations.ts:2686keeps the blast radius contained today.- JCS throw propagation isn't smoothed.
recorder.ts:331callsprojectRecordedCallinside a.map()with no try/catch. A single stored payload withNaN/Infinity/BigIntpoisons the wholerecorder.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.mapbody and emittingpayload_build_failedper-entry, or document the contract loudly inrecorder.query()'s JSDoc. - Malformed JSON-string payload +
application/jsoncontent-type stores plaintext.recorder.ts:603-612returnscap(payload, maxPayloadBytes)verbatim onJSON.parsefailure — 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 beforecap(). /JCS: non-finite number\b/couples the runner to one canonicalizer's error string.validations.ts:2680matches againstquery.payload.errorfrom the agent-under-test's controller envelope — agents using a different JCS impl won't downgrade tonot_applicable. Lift this into a structurederror_codefield on the controller envelope (jcs_non_finite_number) and match that instead, or call out the brittleness explicitly in the comment.validateUpstreamIdentifierPathsmutates caller-owned strings.loader.ts:297rewritesvalidation.identifier_paths[pathIndex]in place.runStoryboardStepnow re-validates programmatic input (runner.ts:3203), so callers passing their ownStoryboardobjects will observe$.foorewritten tofoopost-call. Document the mutation contract or clone before mutating.identifier_pathsloader grammar is stricter than the spec defines. Loader atloader.ts:114-118+ the regex at:127-128reject$..,$["…"], 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)
computePayloadDigestSha256options union.recorder.ts:522-549.RegExp | false | { … prenormalized? }is documented but baroque —falseis the same as{ prenormalized: true }. Soft-deprecate the bareRegExpandfalseforms in JSDoc (@deprecated pass { redactPattern } / { prenormalized: true }); plan removal at the next major. The runtime branch can stay.- Redundant
valueDigest &&guard.validations.ts:3050— thecontinueat:3047already short-circuits the no-digest path, so the extravalueDigest &&on the conditional below is dead. Cosmetic. canonicalCodegenJsonarray-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).
There was a problem hiding this comment.
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 gradespassed: true, not_applicable: truewheneverquery.payload.errormatches/JCS: non-finite number\b/. Pre-PR, that string came from the recorder's own JCS throw. This PR also moves the recorder tosafeProjectRecordedCall, which catches that throw and drops the entry onsuccess: true(recorder.ts:377-388). After this PR the SDK's own loop never emits the string. The only remaining emitter is the seller'squery_upstream_traffichandler.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 gradesnot_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
failmatches 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 throughtoQueryUpstreamTrafficResponse) so the signal is something the seller can't forge from prose.
- 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
-
safeProjectRecordedCallmakes 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 issuccess: true.payload_build_failedfires throughonErrorbut 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_totalcounter onUpstreamRecorderQueryResult/QueryUpstreamTrafficResponseso the runner has something to grade against.
-
types.ts:253-254+:361-362+index.ts:51-55— three new doc blocks sayquery()throws on non-canonicalizable digest payloads. Code drops and emitspayload_build_failed(changeset agrees). The doc and the behavior were both edited in this PR; pick one and align. Adopters wiringtry/catcharoundquery()per the doc will silently degrade.
Follow-ups (non-blocking)
recorder.ts:62-63—RawRecordedCall.payloaddoc 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-845redactJsonLikeSecretValues— 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{, advancinglastIndexpast 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_timestampfallback echoesmatched[0]?.call.timestampfrom the pre-drop list. If the projection drops that entry, the response echoes a window header for a record the caller never sees. Useitems[0]?.timestamp ?? matched[0]?.call.timestamp ?? ….recorder.ts:413-414—RecordedCall.host/.pathare now required-as-string with''fallback onnew URL(url)failure. Code does emiturl_parse_failedthroughonErroratrecorder.ts:182-184, which is the right shape, but the spec text on whetherhost/patharetype: string,string | null, orstring, minLength: 1incomply-test-controller-response.jsonis worth pinning before the wire shape ossifies. Confirm against the merged spec PR adcontextprotocol/adcp#3816.generate-types.ts:578-590—canonicalCodegenJsonenum sort works on every valid JSON Schema enum value type but stringifiesundefinedto 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—validateIdentifierPathSyntaxcompares first segment case-sensitively against'request' | 'response' | 'context'.$.REQUEST.foowould 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-2805is plausible but isn't grounded in a cited spec line.
Things I checked
- Recursive
redactSecretsat depth 256 is stack-safe;assertJsonDepthruns 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—validateUpstreamIdentifierPathsrejectsrequest.*,$["request"].*,$..request.*, bracket-quoted, recursive-descent, and the case-sensitive nit above. ProgrammaticrunStoryboardStepnow callsvalidateStoryboardShape(runner.ts:3203) so the gate isn't bypassable.computePayloadDigestSha256overload set is sound; legacyRegExpandfalseforms still accepted; the helper is new so no double-redact hazard from existing adopters.IDENTIFIER_DIGEST_LIMIT = 64matches spec text quoted intools.generated.ts:22497.supported_optimization_metricsempty-array omission atfrom-platform.ts:991-994parallels how 3.1 capability blocks handle presence-vs-empty.promoteConditionalParamPropertiesconsolidation acrossgenerate-types.tsandgenerate-3-1-beta-types.tsis the right factoring — the beta codegen path will now also throw on conflicting branches, matching the changeset claim.- Changeset shape is correct: one
minorfor the new export surface (computePayloadDigestSha256,PayloadDigestOptions,RecordedCallre-exports,query()projection contract change), onepatchfor 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.
There was a problem hiding this comment.
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-modepayload_lengthsemantic change atrecorder.ts:421.harden-conditional-param-codegen.md(patch) is right — build-time fail-loud only, no runtime API delta.resolve-3-1-blocked-issues.mdcorrectly 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 atcomply-test-controller-response.json#/.../payload_digest_sha256; NaN/Infinity →not_applicablein the same schema's JCS edge-case note;identifier_match_proofs/maxItems: 64matchesIDENTIFIER_DIGEST_LIMIT = 64;identifier_pathsgrammar matchesstoryboard-schema.yaml(lines 1428-1462). No divergence. - Security posture (
security-reviewer): no High/Critical. TheassertJsonDepth(redacted)calls atrecorder.ts:651, 667close the "unvisited subtree stored" footgun the 32→256 bump inredact-secrets.ts:39would otherwise have opened. MultipartRequestbody now reads viaclone.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.lengthconsistent.safeProjectRecordedCallis per-call fail-closed (bad call dropped, excluded fromtotal) and per-query fail-open (others returned). Right shape.recorder.ts:541-581—computePayloadDigestSha256overloads routeRegExp,false, andPayloadDigestOptionscorrectly.prenormalized: trueand legacyfalseproduce identical digests; the parity test atupstream-recorder-spec-shape.test.js:336pins it.loader.ts:122-131— identifier-path regex rejects$..request.x,$["request"].x,request[*].x.runStoryboardStepentry atrunner.ts:3203now runsvalidateStoryboardShapeupfront so programmatic callers get the same loud-fail-on-drift gate asrunStoryboard.
Follow-ups (non-blocking — file as issues)
upstream_trafficnot_applicableis spoofable through the controller error string.validations.ts:2682matches/JCS: non-finite number\b/againstquery.payload.error, which is composed from the adopter-controllederrResult.error_detail(runner.ts:5186). An adopter being graded can return that exact string from theircomply_test_controllerand convert a failingupstream_trafficcheck intopassed: true, not_applicable: true.validations_not_applicablepropagates ontoStoryboardResultandComplianceResultbut is not surfaced onComplianceSummaryArtifact— which is what most CI/badge tooling consumes. Mitigations: (a) includevalidations_not_applicablein 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 ifjcs.ts:120ever rephrases the error message. Convert to a typed error class so the downgrade is resilient to future canonicalizer refactors. safeProjectRecordedCallsilent drop loses operator signal.recorder.ts:377-388catches JCS canonicalization throws and emitspayload_build_failedviaonError. Adopters who have not wiredonErrorsee no breadcrumb when a NaN payload disappears from a digest-mode query — and the storyboard then grades onmatched_countmismatch rather than a clear "non_finite" note. Fire a typeddigest_canonicalization_failedevent, or call out theonErrorwiring requirement in the digest-mode docstring.parseJsonStringPayloadForScanruns even when no identifier digests were requested.recorder.ts:399emitsjson_payload_parse_failedon every malformed JSON entry in a digest-modequery()whether or not identifier proofs were asked for. Cheap fix: short-circuit whenidentifierDigests.length === 0.- Pre-existing codegen gap, not this PR.
tools.generated.ts:22980-22988andschemas.generated.ts:7961-7967emit collapsedRawAttestation/DigestAttestationinterfaces — the per-branch payload/digest fields are lost. SDK adopters usingupstream-recorder/types.tssee the right discriminated union; only direct consumers of the generated types hit the collapse. Separate codegen issue.
Minor nits (non-blocking)
loader.ts:113case-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 thei-flaggedSECRET_KEY_PATTERNis cosmetic. One-line.toLowerCase()closes it.redactJsonLikeSecretValuesproduces non-JSON output on malformed-JSON arrays.recorder.ts:835-905—{"token": [1,2,3], ...(malformed) emits{"token": [redacted],2,3], ...becauseskipJsonLikeValuedoes 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."RecordedCallBasecarries[key: string]: unknown(types.ts:26). Type-level forward-compat hatch, but the 3.1 schema hasadditionalProperties: falseonrecorded_calls[]items. Adopters who attach arbitrary keys passtscand 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.
There was a problem hiding this comment.
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.mdispatch(tightens an existing transform with a fail-loud branch).upstream-digest-attestation-hardening.mdisminor— correct:computePayloadDigestSha256ships a new options shape via overloads 1–3, the legacyRegExpandfalsesignatures remain wired through overloads 2 and 3, andUpstreamRecorderErrorEventadds three additivekinds. Exhaustiveswitchon the event union will fail to compile, which is the desired outcome for adopters running adopter-side diagnostics. [key: string]: unknownonRecordedCallBase(types.ts:24-30) is not a new escape — bothRawRecordedCallandDigestRecordedCallalready 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.1additionalProperties: falseonrecorded_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 bybuildRecordedCall's outer try/catch (recorder.ts:202-207) and surfaced aspayload_build_failed. validateStoryboardShapeinvoked fromrunStoryboardStep(runner.ts:3203) — right shape. The validator was already idempotent and the existingcontributes→contributes_tomutation means the entry point was always opinionated about input.- Empty
supported_optimization_metricsomission (from-platform.ts:988-993) — sound. The 3.1 schema makes the field optional with nominItems; 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
canonicalCodegenJsonprevents cosmetic false positives; the cached 3.1.0-beta.5 schemas pass the regression test added attest/type-generator-allof-ref-merge.test.js. payload_must_containnot_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-modequery()"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 insafeProjectRecordedCall(recorder.ts:382-388) is drop-the-entry + emitdigest_canonicalization_failedviaonError. Adopters writing try/catch onquery()expecting a throw will end up with a silent drop. Update the three JSDoc passages to match the catch-and-drop reality. prenormalized: trueis 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 rawfetchbodies will publish digests coveringAuthorization: Bearer …— and the buyer-visible digest becomes a low-entropy pre-image confirmation oracle against guessable bearer tokens. Mitigation: rename tounsafeSkipRedaction, emit a one-shot warning on first use, or have the helper scan forSECRET_KEY_PATTERN-shaped keys whenprenormalized: trueand throw on detection.- No cycle guard on
redactSecrets;computePayloadDigestSha256has no public try/catch. A cyclic input expands during redaction into a 256-deep linear chain whose leaf is the original cyclic node;assertJsonDepththen throwsRangeError. Insiderecord()this is caught and emitted aspayload_build_failed— safe. The public digest helper has no surrounding try/catch, so a cyclic payload becomes an uncaughtRangeErrorpropagating into adopterquery_upstream_traffichandlers, surfacing as 5xx instead of 4xx. Add aWeakSetcycle guard inredactSecretsand wrap the helper's depth-overrun in a typedPayloadDigestError. - PayloadDigestOptions does not key-align with
createUpstreamRecorderoptions. The changeset narrative says "pass the same custom options used bycreateUpstreamRecorder()" but the helper exposesredactPattern/maxPayloadBytes/prenormalizedwhile 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 againstadcontextprotocol/adcpto ratify the multi-clause grading rule so other implementations converge instead of mirroring the SDK after the fact.IDENTIFIER_DIGEST_LIMIT = 64is 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 storyboardidentifier_pathsJSDoc.runStoryboardStepvalidation 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.
There was a problem hiding this comment.
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.mdis minor (new exportsPayloadDigestError,computePayloadDigestSha256,PayloadDigestOptions),harden-conditional-param-codegen.mdis patch (build-time only — codegen hard-fails on conflicting promotedparamskeys), andresolve-3-1-blocked-issues.mdcorrectly amends prose for the empty-rollup omission. No manualpackage.jsonversion edit. - Witness invariant on
safeProjectRecordedCall(recorder.ts:387-398). Per-entry try/catch around projection. Thematched.filterstill runs first, so cross-principal isolation is preserved; only the failing entry gets dropped, withdigest_canonicalization_failedsurfaced throughonError. No silent fabrication: failing entries don't get placeholder digests. payload_lengthin digest mode (recorder.ts:434). NowbyteLengthOf(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-52docs the raw-vs-digest split correctly.assertJsonDepthpost-redact (recorder.ts:725, 741). Redaction-then-depth-check. Old 32-deep-passthrough is closed; payloads >256 throwRangeErrorwhichbuildRecordedCallcatches and surfaces aspayload_build_failed, no record stored.security-reviewerconfirmed the closed exfiltration vector.- Cross-principal isolation preserved.
query()filters byentry.principal === params.principalbefore projection.safeProjectRecordedCallcannot escape the filter. - Codegen conflict detection (
scripts/generate-types.ts:582-628).canonicalCodegenJsondeterministically serializes; enum-order normalization atkeyHint === 'enum'accepts reordered enums but still throws on conflicting subsets (the JSON strings differ when lengths differ). Conflict error names bothallOfindices. - Identifier-path validation robustness (
loader.ts:80-131). RejectsRequest.*(case),$.request.*($-prefix), bracket-quoted forms, recursive descent, numeric indexes. Trim happens before validation but the stored path is not mutated. validateStoryboardShapeordering inrunStoryboardStep(runner.ts:3203). Shape gate runs beforeapplyStoryboardVersionOptions— no prior consumer of malformedstoryboard.requires.- Disabled-recorder
since_timestamp. Now'1970-01-01T00:00:00.000Z'(recorder.ts:795) — RFC 3339 valid;""would have failed anyformat: date-timevalidator.
Follow-ups (non-blocking — file as issues)
-
payload_lengthdigest-mode change is under-disclosed. The changeset prose calls it "now spell out the existing attestation semantics" — butrecorder.ts:434actually changes the value (call.payload_length→byteLengthOf(payloadBytes)). Adopters who shipped digest mode against the priorpayload_lengthwill see different numbers. Worth a one-line "BEHAVIOR CHANGE" callout, or a note in the migration doc. -
findUnredactedSecretPathis recursive (recorder.ts:634-657), not iterative. The post-redactassertJsonDepthgate uses an explicit stack at 256 cap, but the prenormalized-safety walker uses Node's call stack. A 10k-deep prenormalized payload trips aRangeError: Maximum call stack size exceededbefore 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/falseslip past. Adopters who pre-redact withnull(a plausible custom posture) get hashed-over-plaintext. Document the literal'[redacted]'requirement in theprenormalized: trueJSDoc, or widen the predicate. -
redactJsonLikeSecretValuesis best-effort by design but the doc comment could be louder.skipJsonLikeValuedoesn't walk into object/array values — when the secret value is{"nested":"..."}, the outerpassword: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 = 256is duplicated betweenrecorder.ts:32andredact-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 fromnot_applicableback tofailed. 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.jsonsetminLength: 1onRecordedCall.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]: unknownindex signature onRecordedCallBaseis TS-only forward-compat; the comment claims wire-side rejection. Verify. - Does the spec actually constrain
identifier_pathsto the dotted+[*]grammarloader.ts:127enforces, 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.
- Does
-
Spec-silent on
purposeenum (recorder.ts:175). The classifier now enforces six values and emitsclassifier_invalid_purposefor the rest.types.ts:56-60says "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
computePayloadDigestSha256signature is awkward.RegExpandfalselegacy forms plus the canonicalPayloadDigestOptionsobject plus the no-args path is a lot of surface. The@deprecatedJSDoc on the legacy forms is the right move; consider removing them in the next major. -
Enum-sort rationale.
scripts/generate-types.ts:583sorts enum items lexicographically on their JSON-stringified form. Mixed-type enums like[1, "1"]sort consistently becauseJSON.stringifyruns 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.
Summary
This PR resolves the 3.1 follow-up issues around digest attestations, upstream traffic validation, and conditional params codegen.
paramspromotion so rootparamsrefinements do not conflict while true promoted-key conflicts still fail loudlypayload_must_containcases and actionable JSON pointersidentifier_pathsrequest/response/context prefix rejectionValidation
npm run ci:quickFollow-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