From 7eccbf3d1f8f9b1bca1257da5289d414734d7a36 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Sat, 18 Apr 2026 18:05:09 -0400 Subject: [PATCH 1/2] fix: enforce brand/account invariant across storyboard run (#579) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sellers that scope session state by brand lost state across create→get when the runner sent inconsistent brand fields — a list created in `open:acmeoutdoor.example` was invisible to a follow-up get that landed in `open:default` or `open:test.example`. Merge `options.brand` into every outgoing request after builder / sample_request resolution so every step in a run shares one session. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../brand-invariant-storyboard-runner.md | 11 ++++ src/lib/testing/client.ts | 8 ++- src/lib/testing/storyboard/runner.ts | 43 +++++++++++++ test/lib/storyboard-brand-invariant.test.js | 63 +++++++++++++++++++ 4 files changed, 123 insertions(+), 2 deletions(-) create mode 100644 .changeset/brand-invariant-storyboard-runner.md create mode 100644 test/lib/storyboard-brand-invariant.test.js diff --git a/.changeset/brand-invariant-storyboard-runner.md b/.changeset/brand-invariant-storyboard-runner.md new file mode 100644 index 000000000..daf8cbdce --- /dev/null +++ b/.changeset/brand-invariant-storyboard-runner.md @@ -0,0 +1,11 @@ +--- +'@adcp/client': patch +--- + +Storyboard runner: enforce a brand/account invariant on every outgoing request. + +Sellers that scope session state by brand (required for per-tenant isolation) derive a session key from `brand.domain`. Before this fix, a storyboard's `create_*` step could send one brand while the follow-up `get_*` / `update_*` / `delete_*` / `validate_*` step sent another — or omitted brand entirely and hit the default `test.example`. The list created in one session was then invisible to the lookup in a different session, surfacing as `NOT_FOUND` across `property_governance`, `collection_governance`, `media_buy_seller`, and any storyboard that exercises stateful CRUD. + +The runner now merges `options.brand` into every request after builder / `sample_request` resolution, overriding any conflicting brand and filling in `account.brand` when the request carries an `account` object. A storyboard run now lands in one session regardless of per-tool authorship. Storyboards that don't configure a brand (e.g. `universal/security.yaml` probes) pass through unchanged. + +Fixes #579. diff --git a/src/lib/testing/client.ts b/src/lib/testing/client.ts index f81243847..834cc4c2a 100644 --- a/src/lib/testing/client.ts +++ b/src/lib/testing/client.ts @@ -19,8 +19,6 @@ import type { TestOptions, TestStepResult, AgentProfile, TaskResult, Logger } fr import { TOOL_RESPONSE_SCHEMAS } from '../utils/response-schemas'; import { parseCapabilitiesResponse } from '../utils/capabilities'; -const DEFAULT_BRAND_REF: BrandReference = { domain: 'test.example' }; - /** * Extract a principal identifier from TestOptions auth. * For bearer auth this is the token; for basic auth this is the username; @@ -38,9 +36,15 @@ export function resolveAuthPrincipal(options: TestOptions): string | undefined { } } +const DEFAULT_BRAND_REF: BrandReference = { domain: 'test.example' }; + /** * Resolve the brand reference to use for a test call. * Prefers the new brand field, falls back to converting a legacy brand_manifest. + * + * The runner enforces a storyboard-run-scoped brand invariant on every + * outgoing request, so a missing brand yields a stable default that stays + * consistent across create/get/update/delete within a single run. */ export function resolveBrand(options: TestOptions): BrandReference { return ( diff --git a/src/lib/testing/storyboard/runner.ts b/src/lib/testing/storyboard/runner.ts index e1dbd9fcb..3cc678006 100644 --- a/src/lib/testing/storyboard/runner.ts +++ b/src/lib/testing/storyboard/runner.ts @@ -12,6 +12,7 @@ import { executeStoryboardTask } from './task-map'; import { extractContext, injectContext, applyContextOutputs, applyContextInputs } from './context'; import { runValidations, type ValidationContext } from './validations'; import { buildRequest, hasRequestBuilder } from './request-builder'; +import { resolveBrand } from '../client'; import { PROBE_TASKS, probeProtectedResourceMetadata, @@ -355,6 +356,13 @@ async function executeStep( request = applyContextInputs(request, step.context_inputs, context); } + // Brand/account is a storyboard-run-scoped invariant: every step in a run + // targets the same brand, so every outgoing request's brand context must + // match the options. Enforcing this here (after builder + sample_request) + // prevents session-key divergence across create/get/update/delete steps + // when individual builders or sample_request YAML omit brand. + request = applyBrandInvariant(request, options); + // Detect unresolved $context placeholders — a prior step likely failed // and didn't produce the expected output. Skip rather than sending garbage. const unresolvedVars = findUnresolvedContextVars(request); @@ -660,6 +668,41 @@ function evalContributesIf(expr: string | undefined, priorStepResults: Map, + options: StoryboardRunOptions +): Record { + // Only force the invariant when the caller has actually supplied a brand. + // Storyboards that don't exercise brand-scoped tools (e.g. security + // probes) legitimately run without one and should pass through unchanged. + if (!options.brand && !options.brand_manifest) return request; + const brand = resolveBrand(options); + const result: Record = { ...request, brand }; + const existingAccount = request.account; + if (existingAccount && typeof existingAccount === 'object' && !Array.isArray(existingAccount)) { + result.account = { ...(existingAccount as Record), brand }; + } + return result; +} + // ──────────────────────────────────────────────────────────── // Helpers // ──────────────────────────────────────────────────────────── diff --git a/test/lib/storyboard-brand-invariant.test.js b/test/lib/storyboard-brand-invariant.test.js new file mode 100644 index 000000000..912fc3445 --- /dev/null +++ b/test/lib/storyboard-brand-invariant.test.js @@ -0,0 +1,63 @@ +/** + * Storyboard runner: brand/account invariant. + * + * Issue #579 — sellers that scope session state by brand lost cross-step + * state when a create step sent `brand: acmeoutdoor.example` but a follow-up + * get/update/delete step either omitted brand or let it default to + * `test.example`. The runner now overrides brand on every outgoing request + * after builder / sample_request resolution so a storyboard run lands in one + * session, regardless of per-tool authorship. + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert'); + +const { applyBrandInvariant } = require('../../dist/lib/testing/storyboard/runner.js'); + +const BRAND = { domain: 'acmeoutdoor.example' }; + +describe('applyBrandInvariant', () => { + test('injects brand when the request omits it', () => { + const result = applyBrandInvariant({ list_id: 'pl-1' }, { brand: BRAND }); + assert.deepStrictEqual(result.brand, BRAND); + assert.strictEqual(result.list_id, 'pl-1'); + }); + + test('overrides a conflicting brand so every step shares one session', () => { + const result = applyBrandInvariant({ brand: { domain: 'other.example' }, list_id: 'pl-1' }, { brand: BRAND }); + assert.deepStrictEqual(result.brand, BRAND); + }); + + test('fills in account.brand when the request carries an account', () => { + const result = applyBrandInvariant( + { account: { operator: 'acmeoutdoor.example' }, list_id: 'pl-1' }, + { brand: BRAND } + ); + assert.deepStrictEqual(result.account, { operator: 'acmeoutdoor.example', brand: BRAND }); + }); + + test('overrides a conflicting account.brand', () => { + const result = applyBrandInvariant( + { account: { brand: { domain: 'other.example' }, operator: 'other.example' } }, + { brand: BRAND } + ); + assert.deepStrictEqual(result.account.brand, BRAND); + }); + + test('passes through when no brand is configured (e.g. security probes)', () => { + const input = { list_id: 'pl-1' }; + const result = applyBrandInvariant(input, {}); + assert.strictEqual(result, input); + }); + + test('leaves non-object account values alone', () => { + const result = applyBrandInvariant({ account: null }, { brand: BRAND }); + assert.strictEqual(result.account, null); + }); + + test('does not mutate the input request', () => { + const input = { account: { operator: 'x' }, list_id: 'pl-1' }; + applyBrandInvariant(input, { brand: BRAND }); + assert.deepStrictEqual(input, { account: { operator: 'x' }, list_id: 'pl-1' }); + }); +}); From 8926bdb58e3c75b7e3ac6393f51ffc4344414577 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Sat, 18 Apr 2026 18:12:45 -0400 Subject: [PATCH 2/2] test: integration test for brand invariant on the wire Adds a runStoryboard-level test that stands up a mock MCP endpoint and asserts every tool call receives the same brand regardless of whether the step authored brand via sample_request (with a different value), omitted brand entirely, or relied on the builder. This is the regression test that would have caught #579 at the call-site level. Also fills three unit-test gaps (brand_manifest-only, array account, undefined account) and documents that injecting brand into the {account_id} branch of AccountReference passes schema validation but is semantically redundant. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lib/testing/storyboard/runner.ts | 5 + test/lib/storyboard-brand-invariant.test.js | 119 +++++++++++++++++++- 2 files changed, 122 insertions(+), 2 deletions(-) diff --git a/src/lib/testing/storyboard/runner.ts b/src/lib/testing/storyboard/runner.ts index 3cc678006..0078a451e 100644 --- a/src/lib/testing/storyboard/runner.ts +++ b/src/lib/testing/storyboard/runner.ts @@ -685,6 +685,11 @@ function evalContributesIf(expr: string | undefined, priorStepResults: Map, diff --git a/test/lib/storyboard-brand-invariant.test.js b/test/lib/storyboard-brand-invariant.test.js index 912fc3445..864b8e105 100644 --- a/test/lib/storyboard-brand-invariant.test.js +++ b/test/lib/storyboard-brand-invariant.test.js @@ -9,10 +9,11 @@ * session, regardless of per-tool authorship. */ -const { describe, test } = require('node:test'); +const { describe, test, it } = require('node:test'); const assert = require('node:assert'); +const http = require('http'); -const { applyBrandInvariant } = require('../../dist/lib/testing/storyboard/runner.js'); +const { applyBrandInvariant, runStoryboard } = require('../../dist/lib/testing/storyboard/runner.js'); const BRAND = { domain: 'acmeoutdoor.example' }; @@ -50,14 +51,128 @@ describe('applyBrandInvariant', () => { assert.strictEqual(result, input); }); + test('resolves brand from brand_manifest when brand is not set', () => { + const result = applyBrandInvariant( + { list_id: 'pl-1' }, + { brand_manifest: { name: 'Acme', url: 'https://acmeoutdoor.example' } } + ); + assert.deepStrictEqual(result.brand, { domain: 'acmeoutdoor.example' }); + }); + test('leaves non-object account values alone', () => { const result = applyBrandInvariant({ account: null }, { brand: BRAND }); assert.strictEqual(result.account, null); }); + test('leaves array account values alone (malformed but possible)', () => { + const account = []; + const result = applyBrandInvariant({ account }, { brand: BRAND }); + assert.strictEqual(result.account, account); + }); + + test('does not add account when the request has no account', () => { + const result = applyBrandInvariant({ list_id: 'pl-1' }, { brand: BRAND }); + assert.strictEqual(result.account, undefined); + }); + test('does not mutate the input request', () => { const input = { account: { operator: 'x' }, list_id: 'pl-1' }; applyBrandInvariant(input, { brand: BRAND }); assert.deepStrictEqual(input, { account: { operator: 'x' }, list_id: 'pl-1' }); }); }); + +// ──────────────────────────────────────────────────────────── +// Integration: runner call-site regression +// ──────────────────────────────────────────────────────────── + +/** + * Reproduces the wire-level signature of issue #579: three steps in a run + * that each try to set a different brand (via sample_request, via context, + * and via omission). Before the fix, the outgoing MCP `tools/call` would + * carry three different brand domains. After the fix, all three must + * converge on `options.brand`. This catches regressions that move or drop + * the `applyBrandInvariant` call in `executeStep` even when the helper + * itself still behaves correctly. + */ +describe('runStoryboard: brand invariant on the wire', () => { + it('sends options.brand on every step regardless of sample_request authorship', async () => { + const seen = []; + const server = http.createServer(async (req, res) => { + const chunks = []; + for await (const c of req) chunks.push(c); + const rpc = JSON.parse(Buffer.concat(chunks).toString('utf8')); + seen.push({ name: rpc.params.name, args: rpc.params.arguments }); + res.writeHead(401, { 'content-type': 'application/json', 'www-authenticate': 'Bearer realm="x"' }); + res.end('{}'); + }); + await new Promise(r => server.listen(0, r)); + const agentUrl = `http://127.0.0.1:${server.address().port}/mcp`; + try { + const storyboard = { + id: 'brand_invariant_sb', + version: '1.0.0', + title: 'Brand invariant', + category: 'compliance', + summary: '', + narrative: '', + agent: { interaction_model: '*', capabilities: [] }, + caller: { role: 'buyer_agent' }, + phases: [ + { + id: 'p', + title: 'crud', + steps: [ + { + id: 's1_sample_with_different_brand', + title: 'sample_request carries a different brand', + task: 'list_creatives', + auth: 'none', + expect_error: true, + sample_request: { + account: { brand: { domain: 'other.example' }, operator: 'other.example' }, + }, + validations: [{ check: 'http_status_in', allowed_values: [401], description: '' }], + }, + { + id: 's2_sample_omits_brand', + title: 'sample_request omits brand entirely', + task: 'list_creatives', + auth: 'none', + expect_error: true, + sample_request: { list_id: 'pl-1' }, + validations: [{ check: 'http_status_in', allowed_values: [401], description: '' }], + }, + { + id: 's3_builder_path', + title: 'builder constructs account from options', + task: 'list_creatives', + auth: 'none', + expect_error: true, + validations: [{ check: 'http_status_in', allowed_values: [401], description: '' }], + }, + ], + }, + ], + }; + await runStoryboard(agentUrl, storyboard, { + protocol: 'mcp', + allow_http: true, + brand: BRAND, + agentTools: ['list_creatives'], + _profile: { name: 'Test', tools: ['list_creatives'] }, + _client: { getAgentInfo: async () => ({ name: 'Test', tools: [{ name: 'list_creatives' }] }) }, + }); + + assert.strictEqual(seen.length, 3, `expected 3 tool calls, got ${seen.length}`); + for (const call of seen) { + assert.deepStrictEqual(call.args.brand, BRAND, `step ${call.name} brand diverged`); + if (call.args.account && typeof call.args.account === 'object' && !Array.isArray(call.args.account)) { + assert.deepStrictEqual(call.args.account.brand, BRAND, 'account.brand diverged'); + } + } + } finally { + server.close(); + } + }); +});