Skip to content

feat: resolve 3.1 blocked SDK issues#2040

Merged
bokelley merged 7 commits into
mainfrom
resolve-3-1-blocked-issues
May 27, 2026
Merged

feat: resolve 3.1 blocked SDK issues#2040
bokelley merged 7 commits into
mainfrom
resolve-3-1-blocked-issues

Conversation

@bokelley

Copy link
Copy Markdown
Contributor

Summary

  • resolve the 3.1 capability projection follow-ups for conversion tracking, optimization metric rollups, and sponsored-intelligence platform validation
  • align upstream-recorder/query_upstream_traffic with raw/digest attestation fields and identifier proofs
  • add required_any_of_tools storyboard gating, skip-cause aggregation, and generated type/doc updates
  • refresh generated registry types to satisfy schema generation

Verification

  • experts re-reviewed DX, JavaScript/protocol, protocol, and code-review findings with no remaining blockers
  • npm run build
  • npm run typecheck
  • npm run typecheck:examples
  • npm run lint (warnings only)
  • focused regression set: 49 pass
  • direct node suite: 10843 pass / 0 fail / 7 skipped
  • eslint-plugin workspace tests: 30 pass
  • slow CLI timeout files pass individually with 120s timeout

Note: local aggregate npm test hit 60s whole-file timeouts under load for two CLI test files on one run; both files pass individually and the direct node suite passed in the captured run.

@bokelley bokelley changed the title Resolve 3.1 blocked SDK issues feat: resolve 3.1 blocked SDK issues May 27, 2026

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM. Five distinct unblocks shipped under one changeset is more surface than I'd usually fold together, but each is contained, the expert passes (DX, protocol, code, security) converge on sound-with-caveats, and the test additions actually exercise the new behavior rather than rubber-stamping it.

Things I checked

  • Changeset is right shape. .changeset/resolve-3-1-blocked-issues.mdminor against 8.1.0-beta.13 fits the new public surface (productCatalog field on capabilities, RequiredToolFamily, raw/digest discriminated RecordedCall, sponsored-intelligence as a first-class specialism, required_any_of_tools storyboard gate). The companion edit to .changeset/1853-1818-capability-projection.md keeps the 1818 narrative coherent — explicit acknowledgement that the supported_targets portion now ships here rather than being deferred.
  • supported_targets preservation matches spec verbatim. src/lib/types/v3-1-beta/tools.generated.ts:19950 says verbatim "When omitted, only target-less event goals are guaranteed; sellers MAY accept specific target kinds but buyers should not rely on it." The changeset language mirrors it; from-platform.ts:1010 passes through without injecting a default. No translator behavior added.
  • SOM rollup honors the spec contract. Spec at tools.generated.ts:19909 requires sellers keep supported_optimization_metrics synced with the catalog union. rollupOptimizationMetricsFromProducts mechanizes it. Explicit declaration wins at from-platform.ts:990 — right precedence.
  • Runner-side digest computation is authoritative, not controller-trusted. runner.ts:5203-5224 resolves identifier paths against runner-known plaintext (requestPayload / sampleRequest), computes SHA-256 in-process, sends digest to controller, and validations.ts:3010-3017 pairs runner's known digest against controller's found. Controller cannot smuggle a "yes" past the runner.
  • attestation_mode_required: 'raw' downgrade-to-not_applicable at validations.ts:2740-2750 correctly refuses to silently pass a raw-introspection assertion against a digest-only attestation. Fail-closed.
  • Sandbox-authority gate not loosened. src/lib/server/test-controller.ts deltas are typed-params widening and dead-comment cleanup; comply-controller.ts:557-575 routing through sandboxGate is untouched. Phase-2 of #1435 still load-bearing.
  • Identifier-proof field shape matches the cached 3.1 spec (identifier_value_sha256 + found), not the digest_sha256 + path shape my notes had wrong. Confirmed against tools.generated.ts:23127-23135 and upstream-recorder/types.ts:59-67.
  • SI specialism wiring is symmetric. RequiredPlatformsFor (platform.ts:362), SPECIALISM_REQUIREMENTS (validate-platform.ts:56), camelCase platform methods (initiateSession/sendMessage/terminateSession → wire si_*) all line up. examples/hello_si_adapter_brand.ts:543 now claims the specialism instead of carrying the prior workaround comment — right direction.
  • Raw-mode redaction covers the canonical set. recorder.ts:8 → redact-secrets.ts:27: authorization|cookie|set-cookie|api-key|bearer|token|access_token|refresh_token|session_id|session_token|secret|credentials|password|offering_token. Form-urlencoded bodies hit redactFormUrlEncoded; binary bodies are marker-substituted (no raw bytes in the record buffer).

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

  • Pin the canonical-JSON form for payload_digest_sha256 upstream. recorder.ts:466 uses lexicographic sorted-key JSON before SHA-256; the cached 3.1 spec is silent on canonicalization. Today this is benign because the runner compares identifier digests on string leaves, not the payload digest. The day any consumer cross-checks payload_digest_sha256 between two compliant controllers, they'll diverge. File against adcontextprotocol/adcp referencing the empty field-level prose at the digest field — RFC 8785 (JCS) is the obvious citation.
  • Tighten the digest-mode warning to call out low-entropy identifier vectors. recorder.ts:563 warns about the in-memory buffer; it should also call out that unsalted SHA-256 over short numeric IDs / ISO dates / country codes / boolean-as-string is an offline preimage oracle. The spec already mandates "synthetic vectors only" but the SDK doesn't enforce it. Two options: gate attestation_mode: 'digest' projection behind an explicit allowDigestProjection: true recorder option, or move to HMAC with a per-session key (runner controls both sides, so a session-scoped salt is free and breaks dictionary precomputation).
  • Cap total leaf count in jsonStringLeafDigests. recorder.ts:444 caps depth at 256 but not total visited nodes; a 16K-leaf body inside the recorder amplifies per-request CPU. The runner-side collector caps at 64; the recorder-side digest set should similarly cap and emit a [truncated digest set] marker.
  • payload_length and payload_digest_sha256 measure different byte streams. Length is computed at record-time over safeStringify(redacted) (insertion-order); the digest is computed over canonicalJsonStringify (sorted-key). For non-trivial objects the two disagree. No verifier cares today; flag for awareness if one ever does.
  • Non-string identifier vectors silently drop in digest mode. runner.ts:5218 skips non-string leaves. If a storyboard's identifier_paths resolves to a number/boolean, the controller receives no digest, returns no proof, and validations.ts:3010-3017 marks missing rather than not_applicable. Either signal not_applicable for non-string leaves, or force attestation_mode_required: 'raw' when any identifier vector is non-string.
  • DigestRecordedCall.identifier_match_proofs defined twicesrc/lib/server/test-controller.ts:255-258 and src/lib/upstream-recorder/types.ts:62-65. Drift surface. Re-export one from the other.
  • Generated RawAttestation / DigestAttestation Zod/TS shapes are stubs — only the attestation_mode discriminator survives codegen; branch-specific required fields are dropped, passthrough() masks the gap. Pre-existing, not introduced here, but the new promoteConditionalParamProperties helper in scripts/generate-types.ts:575 is scoped to params.allOf[].then.properties.params and won't fix it — separate codegen follow-up.
  • required_any_of_tools skip emits passed: true. runner.ts:1254-1305 returns overall_passed: true on a requirement_unmet skip. skipped_count: 1 + skip_reason: 'requirement_unmet' are correct, but rollups that only consult overall_passed will read a tool-missing agent as compliant for storyboards testing absent tools. Expose effective_passed excluding requirement_unmet skips, or document the rollup contract.
  • Skip-cause aggregation test is synthetic. test/lib/compliance-summary.test.js:508-549 hand-builds a result shape; the regex prefix extraction in summary.ts:272-278 will silently rot if the runner's warning string format ever drifts. Add an integration-shaped test that pipes runner output into the summary.

Minor nits (non-blocking)

  1. Empty payload: {} for undefined-body GETs. recorder.ts:138-141applyRedactionToPayload(undefined) now returns { value: {}, bytes: 2 } rather than { value: undefined, bytes: 0 }, materializing an object the adopter never supplied. Load-bearing because the 3.1 schema requires payload in raw mode (test/lib/upstream-recorder-spec-shape.test.js:91-100 makes this explicit). Defensible but worth a one-line comment at the branch explaining the spec constraint, so the next reader doesn't read it as drift from the witness-not-translator rule.
  2. Explicit empty supported_optimization_metrics: [] coerced to undefined. from-platform.ts:991 collapses somCandidate?.length === 0 into "omit field." Wire-equivalent today, but an adopter explicitly declaring "no metrics supported" gets normalized to "field absent." At minimum, log when explicitSom?.length === 0 so the coercion is visible.

Safe to merge.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM. Clean implementation of the three 3.1 unblocks — query_upstream_traffic digest attestation, the required_any_of_tools gate, and the SI specialism promotion — all derive cleanly from observed data without violating the witness invariant.

Things I checked

  • Witness invariant. projectRecordedCall (src/lib/upstream-recorder/recorder.ts:399-456) and collectUpstreamIdentifierDigests (src/lib/testing/storyboard/runner.ts:5203-5225) only project / hash data already in the recorded payload or the storyboard's sample_request. No fabrication, no normalization, no placeholder substitution. The SHA-256 canonical-JSON projection is a deterministic transform of bytes already on hand.
  • Digest is over the redacted payload, not the original. recorder.ts:215 stores applyRedactionToPayload(...).value and projectRecordedCall reads call.payload from that buffer. No de-redaction path.
  • 64-digest cap enforced at the producer. normalizeIdentifierDigests (recorder.ts:428-437) caps before the per-call leaf walk and dedups. Bounds the membership-bits a runner can extract per call.
  • Discovery fallback respects the opt-out. runner.ts:1817-1837 only fires when !options.agentTools AND clients[0].getAgentInfo is callable AND _profile.tools didn't already populate. The _client-without-tools escape hatch still works.
  • SI specialism wired end-to-end. 'sponsored-intelligence' in AdCPSpecialism (tools.generated.ts:20293), RequiredPlatformsFor (platform.ts:362), validatePlatform SPECIALISM_REQUIREMENTS (validate-platform.ts:58), the compile-time decisioning.type-checks.ts:152 positive test, the runtime test/server-decisioning-from-platform.test.js:3290 negative test, the example claim, and the skill prose all agree. Pre-PR an adopter could claim the specialism without the platform field and silently no-op; the runtime tightening closes that hole without breaking adopters who couldn't dispatch anyway.
  • Encoding parity. All three sha256Hex sites use createHash('sha256').update(value, 'utf8').digest('hex'). Runner-shipped digests match recorder-computed digests byte-for-byte.
  • force_upstream_unavailable wired end-to-end. Enum at inline-enums.generated.ts:137,308, dispatcher at test-controller.ts:566 (SCENARIO_MAP), advertisement at comply-controller.ts:521.
  • Wire-shape pin. test/lib/upstream-recorder-spec-shape.test.js now compiles against the cached schemas/cache/3.1.0-beta.5/compliance/comply-test-controller-response.json instead of the inline fixture — drift surfaces via npm run sync-schemas.
  • Changeset present and correctly typed. .changeset/resolve-3-1-blocked-issues.md is minor; the RecordedCall discriminated-union widening is structurally additive (payload?: never on the digest branch keeps call.payload accessible as unknown for existing consumers), and the SI tightening only narrows a previously-permissive silent path.

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

  • 64-cap silent drop on identifier vectors. When identifier_paths resolves to more than 64 distinct string vectors, collectUpstreamIdentifierDigests truncates but validateUpstreamTraffic still iterates every resolved vector. Past the cap, valueDigest is undefined, sawApplicableCall is true (digest-mode calls were seen), and anyMatchedCallEchoesValue returns { satisfied: false, not_applicable: false } — graded as a missing-identifier failure rather than not_applicable. Either warn at truncation time or grade over-cap vectors as not_applicable. Exactly the silent-failure mode the recorder otherwise avoids.
  • min_count === 0 semantics flip. validations.ts:2760 changes the meaning from "no lower bound" (always-pass) to "must observe exactly zero." Spec-correct and tested, but the changeset prose doesn't call it out. Worth a migration line.
  • payload_digest_sha256 is producer-only / non-portable. canonicalJsonStringify (recorder.ts:466-484) sort-keys and JSON.stringifys — not RFC 8785 / JCS (no number canonicalization, no normative escape rules). identifier_match_proofs are portable because both ends hash raw UTF-8 strings, but payload_digest_sha256 is opaque per-controller. Either align to JCS or document explicitly in the SDK type's JSDoc that consumers must not cross-validate digests across controllers.
  • RecordedCall structural duplication. src/lib/testing/test-controller.ts:243-261 and src/lib/upstream-recorder/types.ts:51-67 both export RawRecordedCall / DigestRecordedCall / RecordedCall but the test-controller version has [key: string]: unknown index signatures and the recorder version doesn't. Consolidate to a single source.
  • promoteConditionalParamProperties codegen helper (scripts/generate-types.ts:575-587 + scripts/generate-3-1-beta-types.ts:275-289) walks only top-level allOf[].then.properties.params. Doesn't recurse into then.allOf, nested if/then/else, or then.oneOf. Document the limitation or extend.
  • Mock-server SI reference. src/lib/mock-server/sponsored-intelligence/seed-data.ts:108 still points at compliance/cache/latest/protocols/sponsored-intelligence/index.yaml. If 3.1.0-beta.5 also publishes specialisms/sponsored-intelligence/, the seed should look at the specialism path.
  • Negative test gaps. No test for malformed identifier_value_digests rejection, orphan match-proofs (controller returns a proof for a digest the runner didn't ship), or a raw-attestation call that erroneously sets payload_digest_sha256. Discriminated-union TS catches the last at compile but no runtime guard.

Approving on the strength of the witness-invariant integrity through the digest projection plus the end-to-end SI specialism wiring.

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM. Five cohesive 3.1 follow-ups behind one changeset, all keyed to the cached schema and pinned by tests against the cached comply-test-controller-response.json instead of the inline fixture. Wire-side payload_length and the attestation_mode discriminator are modeled correctly as a ?: never discriminated union — TS narrowing flows through RecordedCall consumers without losing the runtime invariant.

Things I checked

  • RawRecordedCall | DigestRecordedCall (src/lib/upstream-recorder/types.ts:51-67) — payload_digest_sha256?: never on the raw arm and payload?: never on the digest arm are load-bearing for TS narrowing. Matches the cached 3.1 schema; the upstream-recorder-spec-shape.test.js switch to the cache-backed schema (vs the inline fixture) is the right forcing function.
  • prefetchUpstreamTraffic (src/lib/testing/storyboard/runner.ts:5099-5215) — identifier digests are computed via sha256(plaintext) runner-side and only the hashes leave the process. Plaintext identifiers don't go on the wire. 64-vector cap is a sensible DoS guard. Raw mode is forced whenever payload_must_contain is present, so digest-mode regression for payload-introspection assertions can't sneak in by default.
  • Gate ordering in executeStoryboardPass (src/lib/testing/storyboard/runner.ts:1875-1900) — required_any_of_tools evaluates before requires_capability and the synthetic skip step (buildRequiredAnyOfToolsMissingResult) matches the runner-output-contract v2 vocabulary used by every other requirement_unmet reason. skip-cause aggregation in summary.ts:269-281 extracts the family prefix correctly.
  • SI specialism promotion — 'sponsored-intelligence' appears in the generated AdCPSpecialism union at src/lib/types/tools.generated.ts:20293 and src/lib/types/v3-1-beta/tools.generated.ts:4967. Both the compile-time RequiredPlatformsFor branch (platform.ts:362) and runtime SPECIALISM_REQUIREMENTS entry (validate-platform.ts:56-58) line up. Deleted RequiredPlatformsForProtocols / SupportedProtocol are not referenced elsewhere.
  • productCatalog rollup precedence (from-platform.ts:982-991) — explicit supported_optimization_metrics always wins over the derived catalog. Test pins both arms.
  • Changeset (.changeset/resolve-3-1-blocked-issues.md) — minor bump matches the additive surface under 8.1.0-beta. RecordedCall becoming a discriminated union is a TS-narrowing change for downstream consumers but the runtime shape stays raw-by-default, and we're still in pre-1.0 prerelease.
  • min_count=0 semantics — runner test pins that a raw-required digest downgrade still fails when min_count: 0 is violated (storyboard-runner-output-contract-v2.test.js:230-258). The new rawRequiredDigestDowngrade short-circuit is gated on matched.length > 0, so a zero-match window can't be masked.

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

  1. anyMatchedCallEchoesValue non-string identifier vectors in digest mode. src/lib/testing/storyboard/validations.ts:3007-3018 — when a vector resolves to a non-string, collectUpstreamIdentifierDigests skips it (no digest sent), but the digest-mode branch still flips sawApplicableCall = true and returns { satisfied: false, not_applicable: false }. The vector ends up in missingIdentifierValues and fails the assertion instead of grading not_applicable. Bounded in practice — identifier paths in storyboards resolve to hashed strings — but worth a if (!valueDigest) continue; so the loop treats non-hashable vectors as inapplicable in digest mode.

  2. Document the conditional-params TS widening. scripts/generate-types.ts:572-587 and scripts/generate-3-1-beta-types.ts:275-288 promote allOf[].then.properties.params.properties into the flat base. Fields that were previously discriminated under if: {properties: {request_type: {const: 'single'}}} now ship as flat-optional in the generated TS — Ajv still enforces the conditional at runtime, but TS callers lose the narrowing. Worth a one-liner in the changeset (or migration note) so adopters reading build_creative / sync_creatives params don't think the field is unconditionally optional.

  3. fwLogger.info dev-mode block has no try/catch. src/lib/server/decisioning/runtime/from-platform.ts:1024-1034 — an adopter-supplied logger that throws would crash server boot in dev. Wrap the dev-mode block.

Minor nits (non-blocking)

  1. {} substitution for undefined payloads regardless of content_type. src/lib/upstream-recorder/recorder.ts:137-141 — required by the 3.1 schema's payload required field, but for a GET/HEAD/DELETE with no body the recorded payload is now a 2-byte JSON object that downstream JSON walkers (containsValueAnyDepth, jsonStringLeafDigests) will traverse. Mild witness/translator deviation; consider '' for non-JSON content_types and reserve {} for JSON.

  2. normalizeAgentToolNames collapses [] to undefined. src/lib/testing/storyboard/runner.ts:1318 — an agent that legitimately advertises zero tools is indistinguishable from an un-discovered profile, so required_any_of_tools and required_tools gates silently no-op. Near-zero real-world impact (every agent ships get_adcp_capabilities), but the asymmetry hides a broken-agent category.

  3. somCandidate.length > 0 drops explicit []. src/lib/server/decisioning/runtime/from-platform.ts:986-988 — adopter intent of "I support none of these metrics" projects the same as "not specified." Probably intentional; one-line comment would pin it.

  4. promoteConditionalParamProperties first-wins assumption. scripts/generate-types.ts:572-587 and scripts/generate-3-1-beta-types.ts:275-288 — comment that the function ignores else branches and uses first-promote-wins so a future schema author with two if/then branches defining the same params.X doesn't get silent data loss.

Safe to merge.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Substantively right shape — 3.1 spec alignment lines up with adcp#3816 / adcp#3961 / adcp#4325. Discriminated RecordedCall, SI specialism enforcement, required_any_of_tools gating, productCatalog derivation all check out. Blocking on changeset prose, not code.

Things I checked

  • RecordedCall discriminated union mirrored in src/lib/upstream-recorder/types.ts:51-67 and the public src/lib/testing/test-controller.ts:241-260. Spec-shape test compiles the cached schema directly.
  • SI specialism wired compile-time (RequiredPlatformsFor<'sponsored-intelligence'> in src/lib/server/decisioning/platform.ts:362) and runtime (src/lib/server/decisioning/runtime/validate-platform.ts:58). AdCPSpecialism enum confirmed in cache.
  • productCatalog derivation gates on explicitSom ?? derivedSom, drops empty arrays before emit (src/lib/server/decisioning/runtime/from-platform.ts:984-991).
  • required_any_of_tools loader rejects [] and single-tool families (src/lib/testing/storyboard/loader.ts:78-104); summary aggregates by missing-family name (src/lib/testing/compliance/summary.ts:269-279).
  • New tests cover digest-mode path, raw-required downgrade, family gate, capability projections.

Must fix — changeset ships two breakages as minor

  1. RecordedCall.payload was required; now branch-dependent. Any adopter writing (c: RecordedCall) => c.payload fails tsc post-bump. payload_length is also new+required, so literal constructors (test fixtures, custom controller stores) need updating too. Same shape mirrored in the public RecordedCall at src/lib/testing/test-controller.ts:241.
  2. validatePlatform now throws PlatformConfigError when 'sponsored-intelligence' is in specialisms[] without the sponsoredIntelligence field. Pre-PR the validator short-circuited unknown specialisms (validate-platform.ts:102if (!required) continue).

Either bump the changeset to major or add explicit **BREAKING** bullets the way .changeset/1853-1818-capability-projection.md already does. The current five-line resolve-3-1-blocked-issues.md reads as additive; both surfaces above fail that promise. Either fix unblocks.

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

  • Identifier-digest collection is request-payload-only (src/lib/testing/storyboard/runner.ts:5203-5225). Storyboards using identifier_paths against response or $context.* values end up requesting digest mode with empty digests; the validation grades not_applicable — silent fail-open. Either reject those paths at load or extend the walker.
  • payload_digest_sha256 is computed over canonical-JSON re-serialization (src/lib/upstream-recorder/recorder.ts:466-484) — key-sorted, post-redaction. A third party holding the runner's emitted bytes can't reproduce this digest from the wire form. adcp#3816 doesn't explicitly license canonical form. Pin in the spec or document the choice in the recorder's JSDoc.
  • promoteConditionalParamProperties is first-wins, no conflict detection (scripts/generate-types.ts:575, scripts/generate-3-1-beta-types.ts:275). No keys collide today; a future schema author adding a colliding then.params.properties.x silently loses the second branch's narrowing. Throw on overwrite.
  • Test-timeout bump 60s → 180s in package.json. PR body acknowledges this is a workaround for two CLI files that pass individually. Tripling the global default hides slow tests rather than fixing them — file a perf issue and revert to 60s, or scope per-file.
  • agentTools late discovery on reused clients (runner.ts:~1820) now issues getAgentInfo() when _profile.tools is absent. Previously a no-op. Adds a network round-trip to a path orchestrators chose to avoid re-discovery — worth an opt-out flag.
  • projectRecordedCall drops adopter-set extension fields on the digest path (recorder.ts:399-425). Public-API RecordedCall carries [key: string]: unknown (test-controller.ts:249,260); the digest projection enumerates fields explicitly. Tighten the index signature or spread call.

Minor nits (non-blocking)

  1. productCatalog name is misleading (src/lib/server/decisioning/capabilities.ts:148-156). Sounds like the full product surface; it's actually "thing we rollup metrics from." Rename or document explicitly that only metric_optimization is read.
  2. jsonStringLeafDigests only digests string leaves (recorder.ts:439-456). Numeric identifiers (retail SKUs as ints) won't surface in identifier_match_proofs. Spec text says "string tokens" — defensible, but worth a JSDoc line.
  3. anyMatchedCallEchoesValue mixed raw/digest grading is correct (src/lib/testing/storyboard/validations.ts:3000-3034), but runValidations is exported and direct callers can exercise the mixed-batch path. Add a unit test pinning the grade.

Code is the right shape; the failure mode for adopters is concrete — tsc breaks on the first rebuild after the bump. Bump the changeset to major or add explicit BREAKING bullets and we're done.

@bokelley

Copy link
Copy Markdown
Contributor Author

Triage note — fold candidate from #2046

Issue #2046 ("Document and bound recorder digest attestation edge cases") tracks four follow-ups surfaced during review of this PR: JSDoc on low-entropy/unsalted SHA-256 digest risks, a leaf-count cap on the recorder-side jsonStringLeafDigests projection (matching the runner-side bounded posture), a clarification that payload_length and payload_digest_sha256 may measure different serialized forms, and truncation/documentation-pinned tests.

All four items touch files this PR already modifies — the upstream-recorder, test-controller.ts, and the attestation test suite. Scope is modest (~50–150 lines against the 5k already in the diff).

Suggest folding before merge to keep the recorder's attestation guarantees self-contained. If the diff is already at a size where expansion would materially delay review, a focused follow-up PR immediately after merge is fine — just leave #2046 open until then.


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


Generated by Claude Code

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approving. Closes out 3.1 with the right shape — RecordedCall as a raw/digest discriminated union mirrors adcp#3816, the SI specialism gate becomes mechanically enforced via SPECIALISM_REQUIREMENTS, and the supported_optimization_metrics projection implements the rollup the spec already requires sellers to maintain (witness, not translator — derivation from adopter-supplied productCatalog, no fabrication).

Things I checked

  • Changeset vs. wire impact. .changeset/resolve-3-1-blocked-issues.md is minor with explicit **BREAKING**: prose on RecordedCall and validatePlatform. In pre-release mode (pre.json mode: "pre", currently 8.1.0-beta.13 with 56 changesets accumulating), this is the established convention for this package — the exit-pre bump composes the stack. Not a block; the BREAKING prose carries the adopter signal.
  • Witness invariant. from-platform.ts:982-999 rollup unions productCatalog[].metric_optimization.supported_metrics, sorts, dedups, and omits the field entirely when the result is [] (somCandidate != null && somCandidate.length > 0 ? somCandidate : undefined). No [] on the wire. Spec at tools.generated.ts:20620 describes this as the rollup the seller maintains — the framework is implementing it, not synthesizing it.
  • required_any_of_tools skip-cause aggregation. The synthetic skip from buildRequiredAnyOfToolsMissingResult puts the detail in skip.detail. The bridge at storyboard-tracks.ts:124 copies skip?.detail into warnings[0], and summary aggregator at summary.ts:273 matches ^(missing_required_tool_family: needs [^;(]+) against that. Path is intact end-to-end.
  • Discriminated union exhaustiveness. upstream-recorder/types.ts:51-67 uses payload_digest_sha256?: never / payload?: never correctly. test-controller.ts:213-261 does the same but adds [key: string]: unknown index signatures on each arm — see follow-up #3 below.
  • Schema cache alignment. attestation_mode, identifier_value_digests, payload_digest_sha256, identifier_match_proofs all present in src/lib/types/v3-1-beta/tools.generated.ts:22493-22499 and tools.generated.ts:22346-22352. 'sponsored-intelligence' is in the AdCP 3.1 AdCPSpecialism enum at enums.generated.ts:22.
  • Backward compat. Underscore protocol form 'sponsored_intelligence' is still derived from the registered SI tools via TOOL_PROTOCOL_MAP — 3.0-era consumers keep seeing supported_protocols: ['sponsored_intelligence'] without needing to claim the specialism.
  • Test-plan honesty. PR body is candid that local aggregate npm test hit 60s file-level timeouts under load on two CLI test files; both pass individually. The package.json slow-path split (180s timeout) plus CI timeout-minutes: 15 → 25 is the right fix shape.

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

  1. payload_length vs. canonicalized digest bytes. recorder.ts:420 forwards call.payload_length unchanged into the digest projection, but payload_digest_sha256 at line 401 is computed over canonicalJsonStringify(payload) (sorted-keys canonical form). The two refer to different byte representations — for any object with non-alphabetic keys, the canonical bytes differ in length from JSON.stringify(redacted). Either recompute payload_length from canonicalPayloadBytes(...) in the digest branch, or document that payload_length is the raw-projection length and the digest stands alone.
  2. allIdentifiersNotApplicable erases payload-must-contain credit. validations.ts:2773-2778: a validation that has BOTH payload_must_contain (passing) AND identifier_paths (digest-only matches → not_applicable) currently grades the whole validation not_applicable, swallowing the passing payload checks. Tighten the predicate to require both arms to be unused-or-N/A together.
  3. Index signatures defeat union exhaustiveness in the test-controller mirror. test-controller.ts:249, 260 add [key: string]: unknown to both RawRecordedCall and DigestRecordedCall. A literal carrying both payload_digest_sha256 and payload would satisfy both arms at compile time — upstream-recorder/types.ts doesn't have this issue. Drop the index signatures; UpstreamTrafficSuccessResponse.recorded_calls: ReadonlyArray<unknown> already gives adopters the pass-through.
  4. 64-vector identifier cap silently clips. runner.ts:~5219 (collectUpstreamIdentifierDigests) and recorder.ts:434 (normalizeIdentifierDigests) both cap at 64. A storyboard with >64 identifier_paths-resolved vectors silently drops the overflow on the wire AND fails the corresponding anyMatchedCallEchoesValue checks because valueDigest comes back undefined. Surface a runner notice when the cap clips.
  5. Default attestation-mode policy. runner.ts:5145-5149 falls back to 'digest' whenever there are identifier digests and no raw-introspection peer. Consider explicit opt-in via a storyboard-level preferred_attestation_mode rather than inference — fail-closed default is raw.
  6. jsonStringLeafDigests depth cap is asymmetric with the digest. recorder.ts:439-456 caps the identifier-leaf walk at depth 256; canonicalizeJson (line 474) recurses unbounded. A hostile payload at depth 257 produces a valid digest but escapes identifier matching. Either cap both or document the inversion.

Minor nits (non-blocking)

  1. Regex trailing-space capture. summary.ts:273 matches [^;(]+ and captures the trailing space before (rationale). .trim() on line 275 normalizes it — works, but tighten to [^;(]+?(?=\s*\(|;|$) to keep the capture honest.
  2. host/path strictness asymmetry. upstream-recorder/types.ts:29-30 has both required; test-controller.ts:217-218 has both optional. The cached spec treats them as required. The consumer-side permissive shape is the safe direction, but aligning would let the type system enforce the strict contract.

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

Development

Successfully merging this pull request may close these issues.

1 participant