fix(testing): honor sample_request in get_rights builder (closes adcp#2846)#792
Merged
Merged
Conversation
…#2846) The get_rights request builder hardcoded query/uses and injected brand_id from the caller's brand.domain. Storyboards declaring scenario-specific query text, uses, or a buyer_brand hit the wire with the generic fallback, and rights-holder rosters rejected the caller-domain brand_id as unknown — so rights[0] was undefined, \$context.rights_id didn't resolve, and downstream acquire_rights steps failed with rights_not_found instead of the error the storyboard was actually asserting (e.g., GOVERNANCE_DENIED in the brand_rights/governance_denied scenario). Delegate to step.sample_request when present, mirroring peer builders (sync_plans, check_governance, list_creative_formats, create_content_standards, etc.). The generic fallback still runs when no sample_request is authored. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Apr 22, 2026
bokelley
added a commit
to adcontextprotocol/adcp
that referenced
this pull request
Apr 22, 2026
…oses #2846) (#2883) Upstream PR adcontextprotocol/adcp-client#792 restored sample_request handling in the storyboard runner's get_rights builder. Before, the builder hardcoded query/uses and derived brand_id from the caller's domain, silently discarding every field the storyboard declared. Rights-holder rosters rejected the caller-domain brand_id as unknown, so rights[0] was undefined, \$context.rights_id didn't resolve, and acquire_rights failed with rights_not_found before the training agent's existing GOVERNANCE_DENIED check in brand-handlers.ts:978 could run. With 5.11.0 the brand_rights/governance_denied scenario passes 5/5 against the training agent. CI floors rebaselined: - legacy: 36→43 clean storyboards, 295→336 passing steps - framework: 21→25 clean storyboards, 241→244 passing steps Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Apr 22, 2026
bokelley
added a commit
that referenced
this pull request
Apr 22, 2026
…803) (#807) * test(testing): schema round-trip invariant for every request builder (#803) Iterate every task in `TOOL_REQUEST_SCHEMAS` (plus the two brand-rights schemas that ship outside the MCP tool surface) with a matching storyboard request builder, build its fallback request, and assert it parses cleanly against the generated Zod schema. Catches the #789 / #792 / #793 / #794 class of drift before it ships — any builder that emits off-spec field names or types now fails CI. Also fixes eight pre-existing fallbacks the invariant surfaced: - `update_media_buy.packages[0].package_id` defaulted - `update_rights` + `creative_approval` use spec `rights_id` (not `rights_grant_id`); `creative_approval` emits `creative_url` + `creative_id` - `sync_creatives` + `buildAssetsForFormat` emit the `asset_type` discriminator and spec-correct video fields - `calibrate_content` + `validate_content_delivery` artifact `assets` is an array of typed assets, not an object map - `activate_signal` defaults `destinations` to a placeholder agent entry - `create_content_standards` + `update_content_standards` align with the current `scope` + `policies` shape - `si_get_offering` + `si_initiate_session` pass `options.si_context` through `intent` (string) instead of `context` (ContextObject); `si_initiate_session` emits the required `intent` Closes #803. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(testing): address review - MUTATING_TASKS, enumerated coverage, si intent default Expert review feedback on PR #807: - Use exported `MUTATING_TASKS` from `src/lib/utils/idempotency.ts` instead of re-implementing Zod shape introspection in the test. Handles the two brand-rights tasks (creative_approval, update_rights) via a small EXTRA_MUTATING set — both are mutating per their generated schemas but intentionally live outside TOOL_REQUEST_SCHEMAS. - Replace ">= 30 covered" floor with an enumerated EXPECTED_COVERED_TASKS list. Adding a new builder/schema pair now forces a conscious list update rather than drifting past a magic number. - `si_initiate_session` intent default is now "Browse available offerings" (a real user-ask) instead of "E2E test session" (test metadata). Intent dispatches meaningfully on SI agents; storyboards override via sample_request for intent-specific paths. - Regenerate docs/llms.txt and docs/TYPE-SUMMARY.md (upstream compliance- cache update landed while this branch was open). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: sync version.ts + package-lock.json to 5.11.0 `sync-version` regenerated `src/lib/version.ts` and the package-lock to match `package.json` (5.11.0). The branch's version.ts had been carried over from a 5.10.0-era rebase point. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Apr 22, 2026
bokelley
added a commit
that referenced
this pull request
Apr 22, 2026
Inverts the runner's request-construction priority. `sample_request` is the authoritative base payload; the per-task enricher (formerly "request builder") fills fields the fixture left unset. Fixture wins every top-level conflict. Previously the builder ran first and ~20 enrichers silently discarded authored fixtures because they didn't opt into a fixture-honoring early return. That false-green failure mode produced five consecutive fallback-shape bugs (#780 / #792 / #793 / #802 / #805). Key pieces: - `enrichRequest` replaces `buildRequest`. Takes optional `runnerVars` so context injection on the fixture can resolve `{{runner.*}}` mustache patterns uniformly with the runner's other injection sites. - `FIXTURE_AWARE_ENRICHERS` escape hatch for two tasks that splice discovery-derived fields INTO nested fixture structures: `create_media_buy` (package-level merge with injected product_id / pricing_option_id) and `comply_test_controller` (forces account.sandbox: true). Envelope fields still flow through from sample_request after these enrichers run. - Load-time hard-fail: mutating tasks (per MUTATING_TASKS) without sample_request throw at parseStoryboard, unless expect_error: true. Error message points at the task, step id, storyboard id, and the concrete author action. - Rename: buildRequest / hasRequestBuilder kept as @deprecated aliases for one release; REQUEST_BUILDERS stays internal. - 73/73 shipping compliance storyboards load cleanly under the validator. 5407/5413 unit tests pass. Strict-vs-lenient run reporting (the fourth proposal in #820) is deferred to a separate issue — reporting is a different subsystem. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bokelley
added a commit
that referenced
this pull request
Apr 22, 2026
…es (#805, #820) (#816) * fix(testing): storyboard check_governance builder emits URI-formatted caller (#805) The `check_governance` builder fallback passed `resolveBrand(options).domain` (a bare hostname) as `caller`. Per governance/check-governance-request.json `caller` is `format: uri`, so strict JSON-schema validators reject the fallback with `-32602 invalid_type`. The generated Zod schemas do not enforce `format` keywords, so the existing Zod round-trip invariant did not catch it. Adds `test/lib/request-builder-jsonschema-roundtrip.test.js` — an AJV- based invariant that validates every builder fallback against the upstream JSON schema, catching `format` violations and strict `additionalProperties` regressions Zod misses. Ships with a small `KNOWN_NONCONFORMING` allowlist documenting six pre-existing fallback bugs unrelated to #805 (format_id.agent_url placeholders; missing required fields on a few builders); a companion guard test fails if any listed task starts passing so the allowlist stays minimal as fixes land. Closes #805 (check_governance half; the creative_approval half was already fixed by prior work on the builder and is now covered by the new invariant). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(testing): close remaining storyboard builder fallback gaps Empties KNOWN_NONCONFORMING in the AJV round-trip invariant by fixing the six builder fallbacks the invariant flagged. Every fallback now satisfies its upstream JSON schema when no sample_request is authored. - build_creative / preview_creative / sync_creatives: share a frozen UNKNOWN_FORMAT_ID with URI-formatted agent_url instead of the bare "unknown" string. Schema core/format-id.json requires format: uri. - update_media_buy: inject account (required per schema) via the same context.account ?? resolveAccount(options) pattern peer builders use. - get_signals: default to a minimal signal_spec so the anyOf [signal_spec | signal_ids] constraint is satisfied when neither options.brief nor sample_request.signal_ids supplied one. - create_content_standards: emit a minimal inline bespoke policies[] entry alongside scope. Schema anyOf: [policies | registry_policy_ids]. The request-builder unit test that asserted get_signals returns {} is updated to match the new contract; all 5482 suite tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(testing): close remaining storyboard builder fallback gaps (part 2) Two bugs the protocol expert caught in the initial PR-816 fix: - check_governance.caller was emitting the BRAND's URL instead of the caller-agent's. Governance agents bind `caller` to agent identity (rate limits, audit, JWS issuer correlation); emitting brand.domain there names the wrong entity and will confuse strict governance agents. Use a stable harness-orchestrator URL and move `brand` into `payload` where the governance rule actually evaluates it. - create_content_standards fallback was emitting enforcement: "must" with a static policy_id — a contamination hazard if a test accidentally hits a shared sandbox (the policy would harden into a deny rule against real content). Downgrade to "should" (warn-only) and make policy_id ephemeral (`e2e-fallback-${Date.now()}`) so stale policies can't match by accident. Both changes keep the fallbacks schema-valid; the AJV round-trip invariant still passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(testing): fixture-authoritative storyboard runner (closes #820) Inverts the runner's request-construction priority. `sample_request` is the authoritative base payload; the per-task enricher (formerly "request builder") fills fields the fixture left unset. Fixture wins every top-level conflict. Previously the builder ran first and ~20 enrichers silently discarded authored fixtures because they didn't opt into a fixture-honoring early return. That false-green failure mode produced five consecutive fallback-shape bugs (#780 / #792 / #793 / #802 / #805). Key pieces: - `enrichRequest` replaces `buildRequest`. Takes optional `runnerVars` so context injection on the fixture can resolve `{{runner.*}}` mustache patterns uniformly with the runner's other injection sites. - `FIXTURE_AWARE_ENRICHERS` escape hatch for two tasks that splice discovery-derived fields INTO nested fixture structures: `create_media_buy` (package-level merge with injected product_id / pricing_option_id) and `comply_test_controller` (forces account.sandbox: true). Envelope fields still flow through from sample_request after these enrichers run. - Load-time hard-fail: mutating tasks (per MUTATING_TASKS) without sample_request throw at parseStoryboard, unless expect_error: true. Error message points at the task, step id, storyboard id, and the concrete author action. - Rename: buildRequest / hasRequestBuilder kept as @deprecated aliases for one release; REQUEST_BUILDERS stays internal. - 73/73 shipping compliance storyboards load cleanly under the validator. 5407/5413 unit tests pass. Strict-vs-lenient run reporting (the fourth proposal in #820) is deferred to a separate issue — reporting is a different subsystem. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * style: apply prettier formatting to request-builder.ts Auto-fix from running `npx prettier --write` after the fixture-authoritative runner refactor landed with some long lines and inconsistent breaks. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
step.sample_requestin theget_rightsrequest builder when present, matching the pattern used bysync_plans,check_governance,list_creative_formats, and other peer builders.query: 'available rights for advertising',uses: ['ai_generated_image'], caller-derivedbrand_id) for storyboards that don't author asample_request.Why
Storyboards declaring a scenario-specific
query,uses,buyer, orbuyer_brandhad those values silently discarded. The hardcoded fallback usedbrand_id: brand.brand_id ?? brand.domain— a caller-domain string that rights-holder rosters treat as unknown. Result:rights[0]was undefined,\$context.rights_iddidn't resolve, and downstreamacquire_rightssteps failed withrights_not_foundinstead of the error the storyboard was actually asserting.This surfaced most visibly in adcontextprotocol/adcontextprotocol#2846: the
brand_rights/governance_deniedscenario assertsGOVERNANCE_DENIED, but the builder quirk caused every run to fail atget_rightsbefore the governance check could fire.Test plan
test/lib/request-builder.test.js—get_rights > honors step.sample_request when presentnode --test test/lib/request-builder.test.js— 33/33 passnpm test— same pass/fail count as main (no new failures introduced)brand_rights/governance_deniedgoes from 4P/1F to 5P/0F after the fix (local build copied into adcp-1's node_modules)🤖 Generated with Claude Code