fix(runner): reset alias cache at phase boundaries (#1657)#1658
Merged
bokelley merged 1 commit intoMay 11, 2026
Conversation
$generate:uuid_v4#alias was sharing its WeakMap cache entry across
all phases within a single executeStoryboardPass call. When two
independent setup phases both used the same alias name, the second
phase received the same UUID as the first — hitting the seller's
idempotency cache and returning stale state from the prior group.
Fix: at the start of each non-skipped phase, replace `context` with
a spread clone (`context = { ...context }`). The new object identity
drops the WeakMap alias cache entry without losing any $context.*
values (plain spread properties survive). forwardAliasCache in
executeStep still propagates aliases step-to-step within a phase.
Regression test added: phase-boundary reset in
test/lib/storyboard-context-generate.test.js (#1657).
https://claude.ai/code/session_01TQwo3hTJAPCeA2Qxa42wCK
4 tasks
bokelley
added a commit
that referenced
this pull request
May 11, 2026
* chore: bump AdCP pin to 3.0.11 Picks up adcontextprotocol/adcp#4365 (v3.0.11): collapses universal/idempotency.yaml's key_reuse_conflict phase into replay_same_payload as a fourth step. The conflict step shares $generate:uuid_v4#replay_key with the replay steps by design — same key, different body — to assert IDEMPOTENCY_CONFLICT against the cache entry the replay step populated. Companion to #1658 (closed #1657): with the runner's phase-boundary alias-cache reset in place, the conflict step must live in the same phase as the replay steps. A separate phase would mint a fresh UUID and the seller would treat the call as new, masking the conflict assertion entirely. 3.0.11 restructures the storyboard so the runner fix is safe. No schema shape changes; regen diff is metadata only (adcp_version strings + entity-hydration source version comment). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: regenerate core.generated and manifest.generated for 3.0.11 CI ci:schema-check caught two generated files I missed on the initial bump commit. Pure metadata refresh — version strings + source-path comments + generated_at timestamps — no field changes. 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 was referenced May 11, 2026
Open
bokelley
added a commit
that referenced
this pull request
May 16, 2026
…bel canonical/reference TrackResult views (#1674) (#1792) * feat(testing): bake source coordinates into every AdvisoryObservation (#1746) Adds required `source: ObservationSource` to `AdvisoryObservation`. The discriminated-union shape carries the rule code plus storyboard/step coordinates when applicable: - storyboard_step — storyboard_id + step_id (most evaluator rules) - storyboard — storyboard_id (rollups across scenarios) - profile — derived from capability profile (no coordinates) - probe — network probe outside the storyboard pipeline Every emission site in comply.ts is populated: 17 in collectObservations, 4 in complyImpl (tool-discovery), 2 in detectAuthRejection. Regression test exercises a representative cross-section and structurally validates source.kind/code plus coordinates when the kind requires them — a future push() without source fails the build. Text output gains a `↳ source: (code · storyboard_id/step_id)` line under each advisory; JSON includes the field by structural recursion. Triagers can grep the storyboard YAML directly from a failing run. Closes #1746. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(testing): label canonical vs reference TrackResult views (#1674) ComplianceResult.tested_tracks is a .filter() of .tracks — same JS object references, so JSON.stringify serializes every scenario twice. Triagers grepping --json output wasted hours on false "duplicate execution" hypotheses (#1658, salesagent#331). Conservative fix: every TrackResult carries _view = 'canonical' | 'reference'. Consumers wanting a deduplicated view filter on _view === 'canonical' or iterate `tracks` and ignore `tested_tracks`. CI pipelines should keep reading buildComplianceSummary() / --summary-output for the stable dedupe-by-design contract. The duplication remains; the breaking type-split that removes it (a narrower TestedTrackEntry omitting scenarios/skipped_scenarios) is tracked at #1791 for bundling with the AdCP 3.1 adoption umbrella (#1580). Both construction sites updated (complyImpl + runWithDegradedProfile). Shallow-copy on the tested_tracks side preserves nested scenarios reference identity — no deep clone, memory cost stays flat. Regression test pins the canonical/reference labeling, shallow-copy invariant, and the documented dedup recipe. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(testing): clarify ObservationSource JSDoc — composite storyboard_id + kebab-case code Two non-blocking notes from PR #1792 expert review: - source.storyboard_id is sourced from TestResult.scenario which is ${storyboard_id}/${phase_id} (not the bare storyboard ID). Document that the field is composite — still greppable against storyboard YAML, just with extra phase-level specificity. - source.code is intentionally kebab-case to match storyboard step-id conventions in compliance YAML, even though the rest of the SDK surface is snake_case. Document the divergence so adopters greppable- searching reports know which casing to look for. 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
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
Closes #1657.
$generate:uuid_v4#aliasplaceholders were sharing their WeakMap cache entry across all phases within a singleexecuteStoryboardPasscall. When two independent setup phases both used the same alias name (e.g.,#setup), the second phase received the same UUID as the first — hitting the seller's idempotency cache and returning stale state from the prior group.Root cause: the alias cache (a
WeakMap<StoryboardContext, Record<string, string>>) is keyed on object identity. Within a pass,contextwas the same object across phases because the runner only created a fresh identity at pass start (line 1292:let context = { ...options.context }). Step-to-step,forwardAliasCacheexplicitly propagates the cache — that's correct. But no equivalent reset happened at phase boundaries, so aliases bled across them.Fix: at the entry of each non-skipped phase, replace
contextwith a spread clone:The new object identity drops the WeakMap alias cache entry — no alias carried forward — while all
$context.*values (plain spread properties) survive intact.forwardAliasCacheinexecuteStepstill propagates aliases step-to-step within the new phase, so within-phase replay continuity is unaffected.Placement is after the
shouldSkipPhase+continueblock — phases that skip execution have no steps, so there is no alias to reset.Changed files
src/lib/testing/storyboard/runner.tscontext = { ...context }at phase boundary entrytest/lib/storyboard-context-generate.test.jsphase-boundary reset: alias does not bleed from phase 1 setup into phase 2 setup (#1657).changeset/fix-phase-boundary-alias-reset.mdWhat was tested
aliased placeholders resolve to the SAME UUID within a context,bare (no-alias) placeholders each resolve to a FRESH UUID,forwardAliasCache propagates aliases across a shallow-cloned context,shallow-cloned context WITHOUT forwardAliasCache gets a fresh cache— all remain green.forwardAliasCache) and cross-phase alias gets a fresh UUID (noforwardAliasCacheat phase boundary).npm run format:checkpasses.Pre-PR review
Code reviewer (independent agent): no blockers. Confirmed:
shouldSkipPhaseskip, before any step runs).context = { ...context }correctly creates a new WeakMap key → fresh alias cache on nextinjectContextcall.forwardAliasCachecall sites inrunner.tsare accounted for; none missed.Compatibility
create_media_buy_async_submittedfixture alias#media_buy_seller_create_submitted_create_media_buyis scoped to a single phase; there are no multi-phase shared aliases in the fixture corpus.forwardAliasCachecan be called explicitly at the phase boundary — the mechanism is already there; the default is now correctly opt-in.Triage-managed PR — opened by claude-code triage agent for issue #1657.
https://claude.ai/code/session_01TQwo3hTJAPCeA2Qxa42wCK
Generated by Claude Code