fix(storyboard-runner): preserve sample_request fields in enrichers (#1604)

.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:<step_id>}}` 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`.

src/lib/testing/storyboard/request-builder.ts

@@ -98,6 +98,26 @@ function asNonSentinel(value: unknown, sentinels: Set<string>): 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:<step_id>}}`)
+ * 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<string, unknown>): Record<string, unknown> {
+  const out: Record<string, unknown> = {};
+  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<string, RequestEnricher> = {
   // ── Account & Audience ─────────────────────────────────
 
@@ -299,9 +319,15 @@ const REQUEST_ENRICHERS: Record<string, RequestEnricher> = {
       step.sample_request !== undefined
         ? (injectContext({ ...(step.sample_request as Record<string, unknown>) }, context) as Record<string, unknown>)
         : {};
+    // Drop envelope fields from the local spread — the outer enrichRequest
+    // re-injects them with `runnerVars` so `{{runner.webhook_url:<step_id>}}`
+    // 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<string, RequestEnricher> = {
       };
     }
 
+    // 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<string, RequestEnricher> = {
     // (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<string, unknown>) }, context) as Record<string, unknown>)
+      ? omitEnvelopeFields(
+          injectContext({ ...(step.sample_request as Record<string, unknown>) }, context) as Record<string, unknown>
+        )
       : {};
     const request: Record<string, unknown> = { ...fixtureFields };
     request.account = context.account ?? resolveAccount(options);
@@ -363,9 +405,13 @@ const REQUEST_ENRICHERS: Record<string, RequestEnricher> = {
   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<string, unknown>) }, context) as Record<string, unknown>)
+      ? omitEnvelopeFields(
+          injectContext({ ...(step.sample_request as Record<string, unknown>) }, context) as Record<string, unknown>
+        )
       : {};
     const result: Record<string, unknown> = { ...fixtureFields };
     result.account = context.account ?? resolveAccount(options);
@@ -378,8 +424,12 @@ const REQUEST_ENRICHERS: Record<string, RequestEnricher> = {
   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<string, unknown>) }, context) as Record<string, unknown>)
+      ? omitEnvelopeFields(
+          injectContext({ ...(step.sample_request as Record<string, unknown>) }, context) as Record<string, unknown>
+        )
       : {};
     const result: Record<string, unknown> = { ...fixtureFields };
     result.account = context.account ?? resolveAccount(options);
@@ -828,7 +878,12 @@ const REQUEST_ENRICHERS: Record<string, RequestEnricher> = {
       }
     }
     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<string, unknown>),
+        account,
+      };
     }
     return {
       account,
@@ -855,25 +910,6 @@ const REQUEST_ENRICHERS: Record<string, RequestEnricher> = {
  * 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,

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.