Skip to content

fix(storyboards): make list_formats_integrity actually send format_ids (closes #2848)#2885

Merged
bokelley merged 3 commits into
mainfrom
bokelley/issue-2848
Apr 22, 2026
Merged

fix(storyboards): make list_formats_integrity actually send format_ids (closes #2848)#2885
bokelley merged 3 commits into
mainfrom
bokelley/issue-2848

Conversation

@bokelley

Copy link
Copy Markdown
Contributor

Summary

Test plan

  • npm run build:compliance — clean
  • npm run typecheck — clean
  • All storyboard lints (scoping, branch-sets, contradictions, context-entity, auth-shape, test-kits, sample-request-schema) — pass
  • ADCP_COMPLIANCE_DIR=dist/compliance/latest run-storyboards.ts --filter media_buy_seller (legacy dispatch) — 14/14 clean, 73 passed
  • TRAINING_AGENT_USE_FRAMEWORK=1 ADCP_COMPLIANCE_DIR=… run-storyboards.ts --filter media_buy_sellerlist_formats_integrity now passes; remaining failures on framework path are pre-existing session-state issues unrelated to media_buy_seller: format_id round-trip (substitution observer) + creative_fate_after_cancellation sync_creatives shape #2848.

Follow-up

🤖 Generated with Claude Code

bokelley and others added 3 commits April 22, 2026 03:37
…mats_integrity (closes #2848)

The @adcp/client (≤5.11) list_creative_formats request builder returns {}
and the runner only forwards envelope fields from sample_request. The
storyboard's `format_ids: ["$context.product_format_id"]` therefore never
reached the wire and the seller answered with its full unfiltered catalog,
making the round-trip assertion fail by coincidence rather than because
the seller substituted formats. Use the runner's context_inputs (applied
after the builder) to inject the captured {agent_url, id} object at
format_ids[0]; document context_inputs in the storyboard schema as a
temporary bridge tied to adcontextprotocol/adcp-client#797.

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

The upstream fix landed (PR #789, closing #780) — issue #797 was a duplicate
and is now closed. Update the storyboard YAML, schema doc, and changeset to
reference the merged PR so the removal trigger is "@adcp/client release that
ships #789" instead of an open issue.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address docs-expert feedback on #2885:
- Lead the context_inputs field doc with the prohibition ("do NOT use
  unless …") instead of the preference, and name the observable symptom
  (unfiltered agent response) an author can diagnose from.
- Add an inline YAML snippet in the schema doc so authors don't have to
  navigate to the media-buy worked example to see the shape.
- Call out that context_inputs silently no-ops on a missing key (distinct
  from $context.<name>'s loud unresolved_substitution), so the default
  path has better failure modes.
- Acknowledge dependency-aware multi-instance dispatch as a legitimate
  non-workaround use so the framing isn't "escape hatch only."
- State type-preservation behavior explicitly.
- Cross-reference from the sample_request field definition and the
  substitution-syntax section so authors land on context_inputs instead
  of scrolling past the field definitions.
- Pin the inline comment to the currently-published @adcp/client 5.10.0
  (not 5.11, which is unreleased) so the version claim doesn't drift
  when 5.11 ships.

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

Copy link
Copy Markdown
Contributor Author

Expert review (ad-tech-protocol-expert + docs-expert) ran on the diff. Addressed in-scope feedback in 4c62d3a93:

  • Reframed context_inputs doc to lead with the prohibition and name the observable symptom an author can diagnose from, instead of framing the field as an SDK-only escape hatch.
  • Added an inline YAML snippet in the schema doc so authors don't have to navigate to the media-buy worked example.
  • Cross-references added from the sample_request: field definition and the substitution-syntax section so authors land on context_inputs: instead of scrolling past the field.
  • Documented the silent-no-op-on-missing-key behavior (distinct from $context.<name>'s loud unresolved_substitution) so the default path's better failure mode is visible.
  • Acknowledged dependency-aware multi-instance dispatch as a legitimate non-workaround use.
  • State type preservation explicitly.
  • Pinned the inline comment to the published @adcp/client 5.10.0 (not ≤5.11, which is unreleased).

Out-of-scope follow-ups flagged by the reviewers (not blockers for #2848; will file separately if confirmed):

  1. Out-of-catalog format probe is missing. The step's narrative at L313–318 calls out the "stale or typo'd format reference in the product catalog" failure mode, but this PR doesn't test it. A second probe asserting list_creative_formats(format_ids: [<unknown>]) returns empty formats[] — not a substitution — would close the proxying-gap side of the invariant.
  2. agent_url echo semantics are underspecified. The seller's filter (training-agent) keys only on format_id.id; the field_value check requires deep equality on {agent_url, id}. For a tier-1 substitution-observer invariant, the spec should say explicitly whether a proxying seller MAY normalize agent_url on a filter lookup or MUST echo verbatim. Worth filing as a spec issue on the main repo.
  3. sample_request.format_ids + context_inputs dual source of truth. Once we bump past the SDK release that ships docs: clarify inline creative handling (Option B) #789, sample_request.format_ids[0]: "$context.product_format_id" and context_inputs both apply (context_inputs writes last, so behavior is unchanged). The inline comment's "drop on SDK bump" is the mitigation; worth capturing as a follow-up issue tagged to the SDK bump so the cleanup doesn't orphan.
  4. CI lint for workaround-comment convention. The "comment your context_inputs with an SDK version and upstream PR" rule is self-policing. A lint step that fails a PR adding context_inputs: without a tracking link near it would make "temporary bridge" enforceable rather than aspirational.

None of these change the correctness of this fix.

@bokelley bokelley merged commit 5d9d286 into main Apr 22, 2026
13 checks passed
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