Skip to content

spec(storyboards): training-agent 5.7 audit fixes (#2627, #2628, #2629)#2631

Merged
bokelley merged 3 commits into
mainfrom
bokelley/fix-storyboard-issues
Apr 21, 2026
Merged

spec(storyboards): training-agent 5.7 audit fixes (#2627, #2628, #2629)#2631
bokelley merged 3 commits into
mainfrom
bokelley/fix-storyboard-issues

Conversation

@bokelley

@bokelley bokelley commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

Summary

Four issues filed from the training-agent 5.7 adoption storyboard audit. Three fix spec/storyboard content; one is upstream-only.

  • Closes brand_rights storyboard conflates advertiser brand_id with talent brand_id #2627brand-rights/index.yaml > get_rights.sample_request was passing the advertiser brand_id (captured from get_brand_identity, e.g., acme_outdoor) into get_rights.brand_id, which per the request schema filters by rights-holder brand (e.g., talent like daan_janssen). Two fields, same name, different entities. Dropped the mis-assignment and added buyer_brand so compatibility filtering works as specified. Narrative now calls out the two brand_id semantics explicitly. Dead context_outputs capture removed after review.

  • Closes Storyboard contradiction: double-cancel. state_machine permits idempotent-accept, invalid_transitions requires NOT_CANCELLABLE #2628state-machine.yaml > recancel_buy said "either return an error or accept idempotently" with no assertions, while invalid_transitions.yaml > second_cancel already required NOT_CANCELLABLE. No single implementation could pass both. Aligned recancel_buy with the NOT_CANCELLABLE vector — matches the §128/§129 resolution being finalized in spec: pre-GA tightening — re-cancel error code (#2617) + request body well-formedness (#2523) #2619 (cancellation-specific code takes precedence over generic terminal-state INVALID_STATE). Narrative for the terminal_enforcement phase now clarifies that pause/resume on canceled buys still return INVALID_STATE so implementers don't over-generalize.

  • Closes schema_validation storyboard: past-start any_of branches reported as step failures #2629schema-validation.yaml past-start handling has two optional: true branches (past_start_reject_path, past_start_adjust_path) both contributing to past_start_handled. A conformant agent picks one path; the other's expect_error/field_present assertion fails because the agent took the opposite behavior, and the runner surfaces the non-taken branch's failure in the summary. Added a new canonical skip reason peer_branch_taken in runner-output-contract.yaml (distinct from not_applicable — keeps coverage gaps separable from runtime branch routing in dashboards). Added the per-step grading rule in storyboard-schema.yaml with authoring constraint (branch-set peers MUST be optional: true) and runner detection rule. Cross-referenced from docs/contributing/storyboard-authoring.md.

#2626 — filed upstream, no spec change required

The sales_catalog_driven storyboard's sample_request already sends the spec-conformant fields (performance_index, metric_type, feedback_source). The bogus feedback.satisfaction payload comes from a hardcoded provide_performance_feedback override in @adcp/client's src/lib/testing/storyboard/request-builder.js that bypasses the storyboard. Filed at adcontextprotocol/adcp-client#689; commented on #2626 to track.

Expert review applied

Second commit (d98f21b) addresses feedback from protocol / product / docs / code reviews:

  • Restored idempotency_key on recancel_buy — without it, a conformant agent could reject for missing-key rather than NOT_CANCELLABLE, failing the step for the wrong reason. Matches the pattern used by schema-validation.create_buy_past_start_reject.
  • Distinct peer_branch_taken skip reason (not overloading not_applicable) — separates coverage gaps from runtime branch routing for downstream tooling.
  • Removed dead brand_id context_outputs capture in brand-rights (no consumer after the buyer_brand fix).
  • Defined "aggregation flag" on first use, split authoring vs. runner guidance in the schema comment, canonicalized the peer_branch_taken detail shape for downstream parsing.
  • Authoring constraint added: every phase in a branch set MUST be optional: true.

Follow-ups filed (out of scope for this PR)

Test plan

  • npm run build:compliance succeeds — compiled storyboards emit to dist/compliance/latest/
  • npm run test:unit (precommit) — 631 tests pass (both commits)
  • npm run typecheck — clean
  • npm run test:error-codes — no new warnings (existing duplicate-key warning on runner-output-contract.yaml > summary is pre-existing)
  • npm run test:storyboard-scoping — passes
  • Spec review: confirm the NOT_CANCELLABLE alignment is consistent with spec: pre-GA tightening — re-cancel error code (#2617) + request body well-formedness (#2523) #2619's §128/§129 resolution
  • Spec review: confirm peer_branch_taken as a distinct skip reason is the right direction vs. overloading not_applicable

🤖 Generated with Claude Code

bokelley and others added 2 commits April 21, 2026 05:54
- brand-rights get_rights: drop mis-assigned brand_id (advertiser vs
  rights-holder conflation); use buyer_brand for compatibility filtering
  (#2627)
- state-machine recancel_buy: align with NOT_CANCELLABLE vector from
  invalid_transitions.second_cancel; removes prior "either error or accept"
  contradiction (#2628, tracks #2619)
- storyboard-schema + runner-output-contract: clarify per-step grading in
  any_of branch sets — non-chosen branches grade not_applicable, not failed
  (#2629)

#2626 filed upstream at adcp-client#689 — storyboard sample_request is
already spec-correct; the bogus feedback.satisfaction payload comes from a
hardcoded override in @adcp/client request-builder.js.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Restore idempotency_key on state-machine.recancel_buy — per code review,
  without the key a conformant agent could reject for missing-key rather
  than NOT_CANCELLABLE, failing the step for the wrong reason (same
  pattern as schema-validation.create_buy_past_start_reject).

- Promote peer_branch_taken to a distinct skip reason in
  runner-output-contract.yaml (separate from not_applicable). Per protocol
  + product review, conflating coverage gaps with runtime branch routing
  hurts dashboard filters and masks real regressions in the accept-branch.
  Detail shape now canonicalized for downstream parsing.

- Remove dead brand_id context_outputs capture in brand-rights/index.yaml
  (no longer consumed after the buyer_brand fix).

- Tighten state-machine.terminal_enforcement narrative so implementers
  don't over-generalize NOT_CANCELLABLE to non-cancellation transitions
  (pause/resume on canceled buys still return INVALID_STATE).

- Define "aggregation flag" on first use; add authoring constraint that
  all branch-set peers MUST be optional:true; split runner detection from
  authoring guidance in storyboard-schema.yaml.

- Cross-reference the grading rule from docs/contributing/storyboard-
  authoring.md so human authors understand why their reject-branch steps
  grade peer_branch_taken rather than failed.

Follow-ups filed separately (not in this PR):
- Explicit branch_set: field as first-class schema construct
- Known-ambiguities entries for all four bugs
- Pre-merge storyboard contradiction lint
- Compatibility-filtering validation in brand_rights
- second_cancel idempotency_key propagation

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

github-actions Bot commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

Schema Link Check Results

Commit: 38a5407 - Merge remote-tracking branch 'origin/main' into bokelley/fix-storyboard-issues

⚠️ Warnings (schema not yet released)

These schemas exist in source but haven't been released yet. The links will be broken until the next version is published:

  • https://adcontextprotocol.org/schemas/v3/enums/specialism.json
    • Schema exists in latest (source) but not yet released in v3
    • Action: This link will work after next 3.x release is published

To fix: Either:

  1. Wait for the next release and merge this PR after the release is published
  2. Use latest instead of a version alias if you need the link to work immediately (note: latest is the development version and may change)
  3. Coordinate with maintainers to cut a new release before merging

…rd-issues

# Conflicts:
#	static/compliance/source/protocols/media-buy/state-machine.yaml
@bokelley
bokelley merged commit 9fe9c49 into main Apr 21, 2026
18 checks passed
bokelley added a commit that referenced this pull request Apr 21, 2026
…el idempotency (#2635, #2637) (#2644)

* spec(storyboards): Bucket A audit follow-ups — known-ambiguities + second_cancel idempotency

- second_cancel (#2637): add idempotency_key to
  scenarios/invalid_transitions.yaml > double_cancel/second_cancel.
  Same pattern as the recancel_buy fix in #2631 — without the key, a
  conformant agent could reject for missing-key (per
  universal/idempotency.yaml) instead of NOT_CANCELLABLE, failing the
  step for the wrong reason.

- known-ambiguities (#2635): four new entries in known-ambiguities.mdx
  covering the PR #2631 audit findings (brand_id conflation, re-cancel
  error code, branch-set grading, SDK request-builder override). Gives
  implementers hitting pre-fix symptoms on older SDK versions a search
  target pointing at the canonical resolution.

#2636 (brand_rights compatibility-filtering assertion) dropped from
Bucket A after discovering two missing primitives: no cross-step
comparison validation exists in the storyboard schema, and no
conflicting-brand test-kit fixture exists. Filed as follow-ups #2642
(primitive) and #2643 (fixture); #2636 stays open blocked on both.

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

* docs(known-ambiguities): apply docs-expert review — removal trigger, label normalization, search targets

- Preamble now explains entries persist until the fix ships in a tagged
  SDK / spec release (not when the issue closes), resolving the
  inconsistency between "#2603 still listed" and the prior "entries are
  removed when issues close" line.
- Normalize the #2603 entry's "Gap closed by" label to "Resolution" so
  all 8 entries use one vocabulary.
- Add the `past_start_handled` aggregation flag as a search target in
  the branch-set entry (implementers grep for the flag name, not just
  phase names).
- Replace ambiguous "pre-fix runners" / "pre-fix the storyboard" with
  "before the #NNNN fix" so implementers know which SDK version carries
  the change.

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

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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