Skip to content

fix(storyboard): refs_resolve follow-ups (#710, #711, #712, #714)#717

Merged
bokelley merged 4 commits into
mainfrom
bokelley/pr-followups
Apr 21, 2026
Merged

fix(storyboard): refs_resolve follow-ups (#710, #711, #712, #714)#717
bokelley merged 4 commits into
mainfrom
bokelley/pr-followups

Conversation

@bokelley

Copy link
Copy Markdown
Contributor

Summary

Bundle of four refs_resolve validator fixes surfaced by security/protocol review on adcontextprotocol/adcp#2601. Each fix closes a dedicated issue and ships with its own changeset.

  • refs_resolve $agent_url should canonicalize transport path before scope compare #710 — Canonicalize \$agent_url by stripping transport suffixes (/mcp, /a2a, /sse, /.well-known/agent[-card].json) while preserving subpath. Mirrors SingleAgentClient.computeBaseUrl. Previously, the scope compare degraded to a silent no-op on every MCP/A2A agent because the runner's target URL never equalled the bare agent URL on refs. Path-preserving matters: AdCP agent_url can legitimately live under a subpath (https://publisher.com/.well-known/adcp/sales) — origin-only would false-positive sibling agents on shared hosts.

  • refs_resolve: warn when scope filter excludes 100% of source refs #711 — Emit scope_excluded_all_refs meta-observation when a scope filter partitions 100% of source refs out. Catches the silent-no-op smell even when the underlying comparison is broken. Suppressed under on_out_of_scope: 'ignore' (which explicitly opts out of scope warnings).

  • refs_resolve: pagination of list_creative_formats target #712 — Detect paginated current-step targets (pagination.has_more: true) and demote unresolved refs to unresolved_with_pagination observations instead of failing the check. Stops false positives against conformant paginating sellers. target_paginated meta-observation surfaces the partition decision. The spec-level fix (compliance mode requires seller returns all product-referenced formats in a single response) should follow in the AdCP repo.

  • refs_resolve: observations payload hygiene (userinfo strip, size caps, escaping) #714 — Harden grader-visible payloads: strip userinfo via WHATWG URL parsing plus a regex fallback for credential-shaped substrings in non-URL fields; replace non-http(s) schemes (blocks javascript: / data: / file: stored-XSS vectors) with a placeholder; code-point-safe truncate at 512 chars; cap observations at 50 with observations_truncated marker; use hasOwnProperty throughout to prevent storyboard authors from drawing match keys from Object.prototype.

Expert review notes

Routed through code-reviewer, security-reviewer, and ad-tech-protocol-expert before pushing. Key incorporated feedback:

  • Protocol — flagged that origin-only canonicalization (my first draft) contradicts the AdCP schema's own agent_url examples (core/format-id.json shows https://publisher.com/.well-known/adcp/sales). Rewrote to path-preserving + transport-suffix-stripping.
  • Protocol — flagged that #712 needs to demote failure → observation, not just annotate. Implemented.
  • Security — non-URL userinfo substrings, javascript: scheme survival, key in ref prototype access, pre-truncate ordering. All addressed with dedicated tests.
  • Code review — surrogate-pair-safe truncation; positional assertion on meta-observation ordering; stale comment in types.ts. All addressed.

Observation vocabulary

This PR adds three observation kinds the runner emits on refs_resolve results: scope_excluded_all_refs, target_paginated, unresolved_with_pagination, plus observations_truncated marker. The runner-output-contract at compliance/cache/latest/universal/runner-output-contract.yaml is synced from upstream and doesn't yet enumerate an observation taxonomy — documenting these kinds in the source contract belongs in a follow-up PR against adcontextprotocol/adcp.

Test plan

  • node --test test/lib/storyboard-validations-refs-resolve.test.js — 39 tests, all pass (18 pre-existing + 21 new covering canonicalization, scope warnings, pagination demotion, and each hardening case)
  • npm test — full suite 4965 pass / 0 fail / 6 skipped
  • npm run build clean

Changesets

Four patch-level changesets, one per closed issue — lets each fix land with its own CHANGELOG entry in the next release PR.

Closes #710, closes #711, closes #712, closes #714.

🤖 Generated with Claude Code

bokelley and others added 2 commits April 21, 2026 11:06
Bundle four refs_resolve validator fixes surfaced during adcp#2601
verification:

- #710 Canonicalize `$agent_url` by stripping transport suffixes (/mcp,
  /a2a, /sse, /.well-known/agent[-card].json) while preserving subpath
  so sibling agents on shared hosts stay distinguishable. Mirrors the
  SDK's computeBaseUrl. Previously the check was a silent no-op on
  MCP/A2A agents because the runner's target URL never matched a ref's
  bare agent URL.

- #711 Emit `scope_excluded_all_refs` meta-observation when the scope
  filter partitions 100% of source refs out — catches silent no-ops
  without changing pass/fail semantics. Suppressed under
  `on_out_of_scope: 'ignore'`.

- #712 Detect paginated current-step targets and demote unresolved refs
  to `unresolved_with_pagination` observations instead of failing the
  check. Stops false-positives against conformant paginating sellers
  until the spec-level "compliance mode returns all" rule lands.

- #714 Harden grader-visible payloads against hostile agent responses:
  strip userinfo (URL parse + regex fallback for non-URL fields),
  reject non-http schemes, code-point-safe truncate at 512 chars, cap
  observations at 50 with truncation marker, use hasOwnProperty
  throughout.

Tests cover each fix plus the security hardening cases flagged in
review (surrogate-pair safety, prototype-key guarding, meta-observation
ordering under the cap, context-vs-current-step pagination scoping,
ignore-mode suppression).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI failures on PR #717:

- Code Quality: prettier formatting on the two refs_resolve files
  edited in this branch.
- Test & Build: generated tool types out of sync with upstream `latest`
  schema bundle, which added five new ListScenarios values
  (seed_product, seed_pricing_option, seed_creative, seed_plan,
  seed_media_buy) and refined the scenarios description. Drift is
  unrelated to this PR — first push to main since the upstream change.

Re-ran `prettier --write`, `npm run sync-schemas`, `npm run
generate-types`, and `npm run generate-wellknown-schemas`. Tests pass
locally (4965/0/6).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bokelley

Copy link
Copy Markdown
Contributor Author

Downstream review came back as ship it with three follow-ups. Filed:

Two non-PR items the reviewer is taking themselves:

Plus one nit deemed not worth fixing here: `CREDENTIAL_SHAPE_RE` doesn't catch `scheme://@host` (bare `@` with empty user). Edge case; mentioned for completeness.

bokelley and others added 2 commits April 21, 2026 11:16
CI ci:docs-check failed because docs/llms.txt and docs/TYPE-SUMMARY.md
still carry the v5.7.0 generated stamp; the version.ts drift correction
in the prior commit bumped the library version stamp to v5.8.0.

Re-ran `npm run generate-agent-docs`. Diff is stamp-only (date +
version line); no content drift.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Conflict: src/lib/types/schemas.generated.ts — header timestamp only;
resolved by regenerating. Auto-merge combined overlapping fixes to
src/lib/server/test-controller.ts (main added `| SeedScenario` to the
exhaustiveness guard via #716; this branch narrowed `ControllerScenario`
to exclude `SeedScenario`). Both survive; functionally equivalent with
one redundant exclusion.
@bokelley bokelley merged commit bdebac9 into main Apr 21, 2026
14 checks passed
@github-actions github-actions Bot mentioned this pull request Apr 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant