fix: enforce portable upstream identifier paths

[bokelley]

Fixes #2057.\n\nThis enforces the portable 3.1 upstream_traffic identifier_paths grammar in the storyboard runner, rejects invalid paths as authoring errors, and prevents invalid paths from triggering query_upstream_traffic prefetches. It also bounds wildcard fan-out in the portable resolver and updates the identifier_paths type comment.\n\nTests: npm run build:lib; NODE_ENV=test node --test-timeout=60000 --test-force-exit --test test/lib/storyboard-runner-output-contract-v2.test.js test/lib/storyboard-runner-output-contract-v2-runner.test.js. Pre-push validation also passed.

[aao-ipr-bot]

Right direction — replacing the silent $. strip with hard rejection is the witness-not-translator shape — but two things to fix before merging.

MUST FIX

1. Missing changeset. PR touches src/lib/testing/storyboard/{path,runner,validations,types}.ts — published surface (dist/lib/**/* ships via package.json#files, and bin/adcp.js imports the storyboard runner directly). CLAUDE.md is unambiguous: "ALWAYS create a changeset for ANY library/CLI code change before pushing a PR. This is mandatory." This is a behavior change for adopters whose storyboards currently use $.-prefixed paths (silently normalized today, authoring error after this lands), so the changeset should be minor and call out the break. npm run changeset.

2. Cross-validation suppression in prefetchUpstreamTraffic — src/lib/testing/storyboard/runner.ts:5123. One invalid path in any upstream_traffic check returns undefined from the prefetch for the entire step, so sibling validations with valid paths fall through validateUpstreamTraffic's if (!upstream || !upstream.advertised) branch at validations.ts:2657 and grade not_applicable with the misleading rationale "adopter did not advertise query_upstream_traffic." The invalid path is correctly flagged as authoring error at validations.ts:2631; the valid siblings should still run. Fix: either filter the prefetch input to only the checks whose identifier_paths are all valid, or drop the early-return entirely and let the per-validation authoring-error branch handle the invalid ones (it already does). The author-facing diagnostic stays correct either way; the difference is whether unrelated valid assertions in the same step keep running.

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

• Loader / runner gate disagreement. src/lib/testing/storyboard/loader.ts:128 still silently strips $. at storyboard load time. A storyboard authored with $.audiences[*].add[*].hashed_email passes the loader after silent normalization, then fails at validation with "must be request-payload-relative" — two gates, two opinions, confusing for authors. validateIdentifierPathSyntax should call validatePortableIdentifierPath so the loader and the runner agree on the grammar.
• some(path => validatePortableIdentifierPath(path)) truthiness at runner.ts:5123. Functionally correct (string truthy vs null falsy) but reads inverted at a glance and breaks if the return type ever moves to a discriminated object. validatePortableIdentifierPath(path) !== null makes the intent obvious.
• Test gaps. No boundary at MAX_PATH_LENGTH = 1024 (exactly 1024 ok, 1025 rejected). No resolvePortableIdentifierPathAll test for obj === undefined / empty arrays / missing intermediate keys. No assertion that __proto__ / constructor / prototype are rejected by name (they match the segment regex but should hit FORBIDDEN_KEYS).
• Defense-in-depth comment. FORBIDDEN_KEYS.has(segment.key) at path.ts:201 is unreachable after validation rejects those names, but worth a one-line comment matching walk()'s rationale so a future cleanup doesn't strip it.

Minor nits (non-blocking)

1. JSDoc precision. path.ts:154 says "at most one [*] wildcard suffix per segment." The regex actually forbids [*] anywhere except the suffix. "At most one trailing [*] per segment" reads more precisely.

Things I checked

• Regex /^[A-Za-z_][A-Za-z0-9_]*(?:\[\*\])?$/ correctly rejects audiences[*]foo, audiences[*][*], trailing whitespace, foo$bar, empty segments. Leading $ is caught before the per-segment loop.
• resolvePortableIdentifierPathAll caps wildcard fan-out at every segment via next.length >= RESOLVE_PATH_ALL_MAX inside the inner loop, plus the post-segment .slice(0, RESOLVE_PATH_ALL_MAX). No off-by-one. The cap test at RESOLVE_PATH_ALL_MAX + 5000 items verifies this.
• All ten invalid path forms in storyboard-runner-output-contract-v2.test.js (explicit $., reserved roots, numeric indexes, bracket-quoted keys, empty segments, recursive descent) hit validateUpstreamTraffic's authoring-error early-return — good unhappy-path coverage.
• Spec lineage: portable grammar derives from adcontextprotocol/adcp#3816. The prohibition on numeric indexes, bracket-quoted keys, and recursive descent is spec-grounded; the prohibition on $. explicit roots and request.*/response.*/context.* reserved roots is not obviously spelled out in the spec body but is consistent with "paths resolve against the request payload root."

Request changes pending (1) and (2). Sign off after the changeset lands and prefetch suppression no longer masks valid sibling validations.

[aao-ipr-bot]

LGTM. Follow-ups noted below. Right shape on direction — kills the silent $.-prefix translator at the validator seam (normalizeStoryboardJsonPath was the textbook "translator, not witness" smell), short-circuits prefetch when the controller call would be wasted, and surfaces author bugs loudly with an invalid_identifier_paths field on the result.

Things I checked

• src/lib/testing/storyboard/path.ts:137-180 — new grammar correctly forbids $, leading/trailing/empty segments, bracket-quoted keys, numeric indexes, reserved roots, and FORBIDDEN_KEYS (__proto__/constructor/prototype). Own-property gate and the RESOLVE_PATH_ALL_MAX = 10000 cap match resolvePathAll's posture.
• src/lib/testing/storyboard/runner.ts:5123 — short-circuit returns undefined so the controller is not queried when any path on the step is invalid. Verified by the new e2e test asserting zero comply_test_controller calls.
• src/lib/testing/storyboard/validations.ts:2628-2651 — authoring-error result is constructed BEFORE the !upstream.advertised opt-out branch, so adopters who haven't advertised query_upstream_traffic still see the authoring error (not a silent not_applicable).
• src/lib/testing/storyboard/validations.ts:2783 — call site swapped from resolveJsonPathLite to resolvePortableIdentifierPathAll. Behavior matches for paths that are valid under the new grammar; invalid paths short-circuit two checks earlier so they never reach the resolver.
• normalizeStoryboardJsonPath is fully removed; no callers remain. resolveJsonPathLite correctly stays alive for payload_must_contain.path (agent-payload traversal — different concern).
• Changeset present and patch is defensible (tightening to spec-intent, no new public exports leaving storyboard/index.ts).

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

• Confirm the runtime grammar admits spec-legal dashed identifier keys. PORTABLE_IDENTIFIER_SEGMENT_RE at path.ts:137 is [A-Za-z_][A-Za-z0-9_]* — no dash. The loader at loader.ts:22 is [A-Za-z_][A-Za-z0-9_-]* — dash allowed. If the AdCP 3.1 normative grammar accepts creative-id-style keys, the runtime regex needs the dash back, otherwise spec-legal storyboards grade storyboard authoring error at runtime. Quick spec re-check before adopters with kebab-case identifier keys hit it.
• Update the PR body citation. Body cites adcontextprotocol/adcp#3816, which is the anti-facade contract PR (the identifier_paths replacement for buyer_identifier_echo). The grammar pin lives in a later spec PR — update the reference so issue #2057's lineage is clean.
• Align loader and runtime grammars. Loader accepts $.foo, dashes, and foo[*][*]; runtime rejects all three. Two layers now claim to enforce "portable grammar" with two different definitions. Tighten the loader to the runtime grammar (preferred — fail-load is louder than fail-grade) so authors don't ship YAML that passes sync-schemas and dies in CI.
• Sibling-validation downgrade carries a misleading note. When the prefetch bails on an invalid path (runner.ts:5123), any sibling upstream_traffic validation on the same step with VALID identifier_paths falls into validations.ts:2657 and grades not_applicable with the note "adopter did not advertise query_upstream_traffic" — false, the adopter did advertise. Either bail only when ALL checks on the step are invalid, or thread a bailed_due_to_sibling_authoring_error signal through ctx.upstreamTraffic so siblings emit a distinct note. Cheap follow-up; happy path is unchanged.
• Case sensitivity on the reserved-root check. path.ts:165 does RESERVED_IDENTIFIER_ROOTS.has(firstKey) against lowercase-only entries; loader.ts:116 does firstSegment.toLowerCase() first. Request.foo / RESPONSE.foo slip past the runtime today.
• Test gap on the loader/runtime delta. Add cases for foo-bar, foo[*][*], Request.foo, __proto__.foo, [*], and foo[*]bar (wildcard not at segment end) to storyboard-runner-output-contract-v2.test.js:823. These are exactly the paths adopters migrating from the looser loader grammar will write.
• Test gap on sibling-validation downgrade. No coverage for "one step, two upstream_traffic validations, one with invalid identifier_paths." That's where the misleading-note bug above lives.

Minor nits (non-blocking)

1. Predicate polarity at runner.ts:5123 reads inverted. .some(path => validatePortableIdentifierPath(path)) relies on the validator returning a truthy string for invalid paths — a future reader will see "validate" + .some and assume "any path validates." Either rename to isPortableIdentifierPathInvalid (returns boolean) or write validatePortableIdentifierPath(path) !== null to match the explicit form used at path.ts:189 and validations.ts:2914.
2. path.ts:215 — next.slice(0, RESOLVE_PATH_ALL_MAX) is redundant. Every push site already gates on next.length >= RESOLVE_PATH_ALL_MAX. Drop the slice; keep the .filter(value => value !== undefined).
3. JSDoc on validatePortableIdentifierPath at path.ts:154 should note the string | null polarity explicitly — "returns the error reason as a string for invalid paths; null for valid." Prevents the runner-site predicate confusion from recurring.

Safe to merge once the spec citation is updated. The grammar-coherence and sibling-note follow-ups can ship as separate PRs — neither blocks the fix for #2057.

[aao-ipr-bot]

Approving. Right shape: invalid paths fail closed as authoring errors rather than silently resolving zero vectors — that's witness, not translator. Removing normalizeStoryboardJsonPath and routing both prefetch and validator through the same resolvePortableIdentifierPathAll is the right cleanup.

Things I checked

• Grammar coverage in validatePortableIdentifierPath (src/lib/testing/storyboard/path.ts:154-180). Regex /^[A-Za-z_][A-Za-z0-9_]*(?:\[\*\])?$/ plus the staged checks reject every variant the tests assert: $.foo, request|response|context roots, ["x"], [0], .., leading/trailing ., and forbidden keys (__proto__|constructor|prototype).
• Wildcard fan-out bound. resolvePortableIdentifierPathAll (path.ts:188-219) checks next.length >= RESOLVE_PATH_ALL_MAX before every push, with a per-segment slice(0, RESOLVE_PATH_ALL_MAX). The fan-out test at test/lib/storyboard-runner-output-contract-v2.test.js:899-911 exercises the 10_000 cap with a 15_000-element array.
• Per-check rejection in validateUpstreamTraffic (validations.ts:2628-2651) grades invalid paths passed: false with error: "storyboard authoring error..." — failing, not not_applicable. Anti-façade semantic preserved.
• normalizeStoryboardJsonPath is fully removed — grep confirms no orphan callers.
• New exports (validatePortableIdentifierPath, resolvePortableIdentifierPathAll, PortableIdentifierPathIssue) are module-internal — not re-exported through src/lib/testing/storyboard/index.ts. patch bump is correct.
• Changeset present and descriptively named. actual.invalid_identifier_paths is an additive field on validation_result.actual — compatible with runner-output-contract v2.0.0.
• In-tree fixtures don't trip the new grammar (existing audiences[*].add[*].hashed_email shape passes).

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

• Prefetch short-circuit is over-broad on multi-check steps. runner.ts:5122 uses upstreamChecks.some(...), so a single invalid check on a step skips prefetch for ALL upstream_traffic checks on that step. The valid siblings then grade not_applicable with note "adopter did not advertise query_upstream_traffic" — wrong story when the adopter actually advertised. Per-check rejection at validations.ts:2630 already handles invalid paths individually; the prefetch gate should be every(... === null) semantics (run prefetch unless the step has no work to do because EVERY check is invalid), or just always prefetch and let per-check rejection do its job. Authoring-time scenario, low likelihood.
• Grammar is SDK opinion, not spec-pinned. adcp#3816 defines identifier_paths semantically but doesn't normatively pin the portable grammar enum, reserved-roots list, or rejection of bracket-quoted keys. The runner now grades storyboards a spec-compliant peer runner might accept. The opinion is right (cross-runner portability matters), but file a spec PR against storyboard-schema.yaml so the grammar lands in the contract, not as runner-pinned policy.
• Reserved-roots check is a blanket-ban, not a syntax detector. path.ts:165 rejects any top-level segment whose key matches request|response|context. That's correct for explicit-root syntax (request.body.audiences[*]) but false-positives on a legitimate payload that happens to have a top-level request field. Today's in-tree fixtures don't hit this, but external storyboards might. Consider tightening to "leading $. or explicit-root form" rather than blanket-banning the identifier as a payload key.
• Test gap on sibling scenario. test/lib/storyboard-runner-output-contract-v2-runner.test.js:225-261 covers single-check invalid paths; add a case asserting valid siblings still grade correctly when their step-mate has an authoring bug — that's where the over-broad short-circuit lives.

Minor nits (non-blocking)

1. Truthy-string predicate at runner.ts:5122. path => validatePortableIdentifierPath(path) relies on null vs error-string truthiness. Functionally correct, but validatePortableIdentifierPath(path) !== null reads cleaner and matches the inverse === null form already used in path.ts:189. Pick one shape and stick with it.

LGTM. Follow-ups noted above.