diff --git a/.changeset/swift-cats-rest.md b/.changeset/swift-cats-rest.md new file mode 100644 index 000000000..6bbb96dee --- /dev/null +++ b/.changeset/swift-cats-rest.md @@ -0,0 +1,9 @@ +--- +'@adcp/sdk': patch +--- + +fix(storyboard-runner): preserve sample_request fields when enriching mutating-tool requests (#1604) + +Fixture-aware enrichers in `src/lib/testing/storyboard/request-builder.ts` rebuilt their request body from scratch and only copied an enumerated set of fields from `step.sample_request` (`start_time`, `end_time`, `packages`). Anything else the storyboard authored at the top level — `total_budget`, `buyer_ref`, `currency`, `reporting_webhook`, scenario-specific extensions — was silently dropped before the request hit the wire. The non-proposal `create_media_buy` path was the immediate trigger; PR #1603's proposal-mode branch already spread the fixture but the structural fix wasn't applied to the rest of the enricher. + +The fix spreads `sample_request` first (after `$context` injection), then layers the runner's substitutions (account, brand, normalised dates, packages with discovery-derived identifiers) on top. Envelope fields (`context`, `ext`, `push_notification_config`, `idempotency_key`) are deliberately omitted from the local spread and re-applied by the outer `enrichRequest` with `runnerVars` so mustache substitutions like `{{runner.webhook_url:}}` expand correctly. The same `omitEnvelopeFields` discipline is now applied uniformly across `create_media_buy`, `update_media_buy`, `get_media_buys`, `get_media_buy_delivery`, and `comply_test_controller`. diff --git a/src/lib/testing/storyboard/request-builder.ts b/src/lib/testing/storyboard/request-builder.ts index d8ed7ad99..a46578915 100644 --- a/src/lib/testing/storyboard/request-builder.ts +++ b/src/lib/testing/storyboard/request-builder.ts @@ -98,6 +98,26 @@ function asNonSentinel(value: unknown, sentinels: Set): string | undefin return sentinels.has(value) ? undefined : value; } +/** + * Envelope fields that live on every AdCP request and are owned by the + * storyboard author — `context.correlation_id`, runner-supplied + * `idempotency_key` aliases, webhook pointers, per-request extensions. + * Fixture-aware enrichers must NOT spread these from their internal + * `injectContext(sample_request, context)` call: that internal call + * doesn't receive `runnerVars`, so mustache tokens (`{{runner.webhook_url:}}`) + * would ship to the wire literally. The outer `enrichRequest` overlays + * envelope fields from a `runnerVars`-aware fixture re-injection. + */ +const ENVELOPE_FIELDS = ['context', 'ext', 'push_notification_config', 'idempotency_key'] as const; + +function omitEnvelopeFields(obj: Record): Record { + const out: Record = {}; + for (const [k, v] of Object.entries(obj)) { + if (!(ENVELOPE_FIELDS as readonly string[]).includes(k)) out[k] = v; + } + return out; +} + const REQUEST_ENRICHERS: Record = { // ── Account & Audience ───────────────────────────────── @@ -299,9 +319,15 @@ const REQUEST_ENRICHERS: Record = { step.sample_request !== undefined ? (injectContext({ ...(step.sample_request as Record) }, context) as Record) : {}; + // Drop envelope fields from the local spread — the outer enrichRequest + // re-injects them with `runnerVars` so `{{runner.webhook_url:}}` + // tokens inside push_notification_config expand. Spreading them here + // would carry through unexpanded mustache strings. + const fixtureBody = omitEnvelopeFields(fixture); + const proposalId: unknown = fixture.proposal_id !== undefined ? fixture.proposal_id : context.proposal_id; if (typeof proposalId === 'string') { - const { packages: _droppedPackages, ...fixtureWithoutPackages } = fixture; + const { packages: _droppedPackages, ...fixtureWithoutPackages } = fixtureBody; return { ...fixtureWithoutPackages, account: fixtureWithoutPackages.account ?? context.account ?? resolveAccount(options), @@ -312,7 +338,19 @@ const REQUEST_ENRICHERS: Record = { }; } + // Spread the fixture (after $context injection) so any sample_request + // fields the enricher doesn't explicitly normalise (total_budget, + // buyer_ref, currency, scenario-specific extensions) reach the wire. + // The enricher then layers its own substitutions on top: packages is + // replaced (the enricher merged samplePackages internally with + // discovery-derived identifiers above), account/brand/start_time/ + // end_time are normalised. Per issue #1604, build-from-scratch dropped + // every fixture field outside this enumerated set — fixture-aware + // enrichers must spread sample_request first, then override. Envelope + // fields are omitted here and re-applied by the outer enrichRequest + // with `runnerVars` so mustache tokens expand correctly. return { + ...fixtureBody, account: context.account ?? resolveAccount(options), brand: resolveBrand(options), start_time: startTime, @@ -329,8 +367,12 @@ const REQUEST_ENRICHERS: Record = { // (otherwise the fixture's account silently routes update writes to a different // partition than the create wrote to, and a subsequent get_media_buys reading // from the create-time partition surfaces stale data — see adcp-client#1505). + // Envelope fields are dropped from the local spread; the outer enrichRequest + // re-injects them with `runnerVars` so mustache substitutions expand. const fixtureFields = step.sample_request - ? (injectContext({ ...(step.sample_request as Record) }, context) as Record) + ? omitEnvelopeFields( + injectContext({ ...(step.sample_request as Record) }, context) as Record + ) : {}; const request: Record = { ...fixtureFields }; request.account = context.account ?? resolveAccount(options); @@ -363,9 +405,13 @@ const REQUEST_ENRICHERS: Record = { get_media_buys(step, context, options) { // Spread fixture fields so storyboards can author filters, status, pagination, etc. // account is always overridden by the harness-resolved value so sandbox routing - // matches create_media_buy's namespace on every round-trip. + // matches create_media_buy's namespace on every round-trip. Envelope fields are + // dropped from the local spread so the outer enrichRequest re-injects them with + // `runnerVars` (mustache substitutions expand against the runner's webhook base). const fixtureFields = step.sample_request - ? (injectContext({ ...(step.sample_request as Record) }, context) as Record) + ? omitEnvelopeFields( + injectContext({ ...(step.sample_request as Record) }, context) as Record + ) : {}; const result: Record = { ...fixtureFields }; result.account = context.account ?? resolveAccount(options); @@ -378,8 +424,12 @@ const REQUEST_ENRICHERS: Record = { get_media_buy_delivery(step, context, options) { // Same account-resolution rule as get_media_buys — fixture fields preserved, // account overridden by harness so sandbox routing matches create_media_buy. + // Envelope fields dropped from the local spread; outer enrichRequest re-applies + // them with `runnerVars` so mustache substitutions expand. const fixtureFields = step.sample_request - ? (injectContext({ ...(step.sample_request as Record) }, context) as Record) + ? omitEnvelopeFields( + injectContext({ ...(step.sample_request as Record) }, context) as Record + ) : {}; const result: Record = { ...fixtureFields }; result.account = context.account ?? resolveAccount(options); @@ -828,7 +878,12 @@ const REQUEST_ENRICHERS: Record = { } } if (step.sample_request) { - return { ...injectContext({ ...step.sample_request }, context), account }; + // Drop envelope fields; outer enrichRequest re-applies them with + // `runnerVars` so mustache substitutions expand correctly. + return { + ...omitEnvelopeFields(injectContext({ ...step.sample_request }, context) as Record), + account, + }; } return { account, @@ -855,25 +910,6 @@ const REQUEST_ENRICHERS: Record = { * empty return is reachable only for read tasks that have no fixture and * no registered enricher (rare). */ -/** - * Envelope fields that live on every AdCP request and are owned by the - * storyboard author — `context.correlation_id`, runner-supplied - * `idempotency_key` aliases, webhook pointers, per-request extensions. - * Fixture-aware enrichers (`create_media_buy`, `comply_test_controller`) - * build their body from scratch and don't re-copy these fields, so the - * outer `enrichRequest` overlays them from sample_request after the - * enricher runs. Non-fixture-aware enrichers get these via the generic - * top-level merge below. - * - * If a future fixture-aware enricher starts emitting an envelope field - * itself (e.g. a scenario where the enricher needs to inject a specific - * `idempotency_key` independent of the fixture), the `=== undefined` - * guard below keeps the enricher's value — intentional, not a bug. - * Fixture envelope fields only flow through for fields the enricher - * didn't set. - */ -const ENVELOPE_FIELDS = ['context', 'ext', 'push_notification_config', 'idempotency_key'] as const; - export function enrichRequest( step: StoryboardStep, context: StoryboardContext, diff --git a/test/lib/request-builder.test.js b/test/lib/request-builder.test.js index 10e9204de..c5a1b3fef 100644 --- a/test/lib/request-builder.test.js +++ b/test/lib/request-builder.test.js @@ -220,6 +220,37 @@ describe('Request Builder', () => { assert.strictEqual(result.packages[0].budget, 5000, 'non-sentinel fixture fields pass through'); }); + test('preserves top-level sample_request fields the enricher does not normalise (#1604)', () => { + // Regression: the non-proposal-mode create_media_buy enricher used to + // build its return value from scratch — only account, brand, + // start_time, end_time, packages — so any other top-level field the + // storyboard authored in sample_request was silently dropped. + // FIXTURE_AWARE_ENRICHERS bypass the outer fixture-overlay merge, so + // the drop wasn't compensated for downstream. The fix spreads + // sample_request first, then overrides only the fields the enricher + // explicitly controls. + const s = step('create_media_buy', { + sample_request: { + start_time: FUTURE_START, + end_time: FUTURE_END, + buyer_ref: 'order-2026-q2', + total_budget: 50000, + currency: 'USD', + reporting_webhook: { url: 'https://buyer.example/wh', authentication: { schemes: ['Bearer'] } }, + packages: [{ product_id: 'p1', budget: 1000, pricing_option_id: 'opt' }], + }, + }); + const result = buildRequest(s, {}, DEFAULT_OPTIONS); + assert.strictEqual(result.buyer_ref, 'order-2026-q2', 'fixture buyer_ref reaches the wire'); + assert.strictEqual(result.total_budget, 50000, 'fixture total_budget reaches the wire'); + assert.strictEqual(result.currency, 'USD', 'fixture currency reaches the wire'); + assert.deepStrictEqual( + result.reporting_webhook, + { url: 'https://buyer.example/wh', authentication: { schemes: ['Bearer'] } }, + 'fixture reporting_webhook reaches the wire' + ); + }); + test('empty fixture package object {} falls back to discovery cleanly', () => { // Some storyboards ship `packages: [{}]` for defaults-only scenarios // where every field should come from discovery / enricher synthesis.