fix(runner): construct account in applyBrandInvariant when missing#644
Merged
Conversation
3 tasks
) Before: the helper added only top-level `brand` and merged into any existing `account`. When the request-builder produced no `account` (e.g. `get_media_buys`, `get_media_buy_delivery`, `list_creatives`), the outgoing request carried `brand` at the top level — which `SingleAgentClient.adaptRequestForServerVersion` then stripped because those tools' schemas declare `account`, not top-level `brand`. Net effect: the run-scoped brand was lost on the wire for exactly the tools most likely to read tenant-scoped state, breaking session isolation for create→get→update→delete flows on brand-scoped sellers. After: when the request omits `account`, construct one via `resolveAccount(options)` so `account.brand` lands on the wire for tools whose schema accepts `account`. Non-object `account` values (`null`, arrays) still pass through unchanged — the helper only synthesizes when the key is absent, not when it's explicitly malformed. Flipped one test that pinned the buggy behavior; added coverage for the `sandbox` passthrough. Existing wire-level integration test (`runStoryboard: brand invariant on the wire`) continues to pass because `list_creatives` with `auth: 'none'` goes through `rawMcpProbe`, which skips the adapter stripping — so the old behavior happened to work for the test harness but not production.
bokelley
force-pushed
the
bokelley/issue-643
branch
from
95b2b14 to
04d79dc
Compare
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
applyBrandInvariant's silent-rewrite behavior; investigation during a related monkey-patch surfaced a higher-priority correctness bug in the primitive itself. Fixing that first.applyBrandInvariantat `src/lib/testing/storyboard/runner.ts:1120` injects top-level `brand` and merges into any existing `account`. For tools whose schema declares `account` but not top-level `brand` — `get_media_buys`, `get_media_buy_delivery`, `list_creatives`, and several others — the request-builder produces no `account`, so the only thing the invariant sets is top-level `brand`. `SingleAgentClient.adaptRequestForServerVersion` (SingleAgentClient.ts:1129-1177) then strips the unrecognized top-level `brand` against the agent's declared tool schema, and the run-scoped brand is lost on the wire. Create→get→update→delete flows on brand-scoped sellers break because the get/update/delete calls land in the seller's `open:default` session instead of the tenant the create used.Why the existing test suite didn't catch this
The wire-level integration test (`runStoryboard: brand invariant on the wire`) uses `auth: 'none'` steps, which dispatch through `rawMcpProbe` and skip `adaptRequestForServerVersion` entirely. So the invariant appeared to work for the test harness but silently dropped brand on the wire in production (typed-client) paths.
Why the silent-rewrite question in #643 stays open (separately)
The programmatic tenant-routing use case the monkey-patch was serving is legitimate — the lint (adcontextprotocol/adcp#2527) covers storyboard authoring mistakes but not runtime SDK callers passing `options.brand`. Removing `applyBrandInvariant` would break that use case. The silent-rewrite concern (still real) can be revisited in a follow-up once this correctness fix lands.
Test plan
🤖 Generated with Claude Code