From db53efba199af42fc7e8d9ea1d4b6773fd7345ae Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 19:09:05 +0000 Subject: [PATCH] refactor(replay): make the daemon's artifact set the run's one ledger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Artifact-path accumulation was double-written after the P5 extraction: the engine's step loop kept its own `Set` while `runReplayScriptFile` kept an outer `Set` that only its exception handler read, and `dispatchStep` wrote both — two mutable collections with no single owner, kept in sync by hand. The daemon's `Set` is now the run's only ledger. `dispatchStep` remains its sole writer and returns its CONTENTS (cumulative for the run, not just the step's own entries); the engine drops its `Set` for a plain `readonly string[]` re-bound to whatever the capability last returned. `AdReplayRunOutcome.artifactPaths` stays a façade field — it is wire-relevant, the daemon's success response reports it — but is now a projection of what the capability handed back rather than an independent accumulation. The exception path is preserved byte-identically. The two old sets differed in exactly one way: the engine's also absorbed a divergence build's own fresh capture, which the daemon's never saw. Writing those into the ledger would change what the catch block reports when `handleActionFailure` itself throws, so they stay out of it and reach `handleActionFailure` through a derived union (`mergeArtifactPaths`) instead — a value, not a write. A failing step always ends the run, so nothing downstream observes that union. Adds a counterfactual test at the ledger's one independent observation point: a mid-loop throw (an unresolved `${VAR}` on step 3) after two artifact-producing steps must report exactly those two artifacts. Dropping the `dispatchStep` write fails it; returning per-step entries instead of the ledger fails its companion completed-run assertion — verified in both directions, then restored. The P5 `declaredScriptPlatform` duplicate this follow-up was also meant to unwind landed inside #1555 itself (`resolveDeclaredScriptPlatform`, owned by packages/ad-script, consumed by both the engine's inspect/digest path and the daemon's `readScriptReplaySelection`), so there is nothing left to dedupe. Gates: typecheck / lint / format:check / check:layering (56) / check:replay-compat (12 digest-pinned entries) / vitest packages src/daemon (255 files, 2179 tests) — all green. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JrwynLjFHMfz42PFBzoEwK --- .../src/internal/runtime-port-types.ts | 13 +- packages/ad-replay/src/internal/step-loop.ts | 48 +++++++- ...ion-replay-runtime-artifact-ledger.test.ts | 115 ++++++++++++++++++ .../session-replay-runtime-engine-adapter.ts | 21 +++- src/daemon/handlers/session-replay-runtime.ts | 9 +- 5 files changed, 191 insertions(+), 15 deletions(-) create mode 100644 src/daemon/handlers/__tests__/session-replay-runtime-artifact-ledger.test.ts diff --git a/packages/ad-replay/src/internal/runtime-port-types.ts b/packages/ad-replay/src/internal/runtime-port-types.ts index edda39fcd..40d85c04b 100644 --- a/packages/ad-replay/src/internal/runtime-port-types.ts +++ b/packages/ad-replay/src/internal/runtime-port-types.ts @@ -50,7 +50,12 @@ export type AdReplayStepFailure = Readonly<{ readonly artifactPaths: readonly string[]; }>; -/** `verify-dispatch.ts`'s per-dispatch result: pass, or a neutral failure (never a wire response). */ +/** + * `verify-dispatch.ts`'s per-dispatch result: pass, or a neutral failure + * (never a wire response). An `ok` outcome's `artifactPaths` is the run's + * whole artifact ledger as of this step, threaded straight through from + * `dispatchStep` — see `AdReplayStepRuntime.dispatchStep`. + */ export type AdReplayStepOutcome = | Readonly<{ readonly status: 'ok'; readonly artifactPaths: readonly string[] }> | Readonly<{ readonly status: 'failed'; readonly failure: AdReplayStepFailure }>; @@ -243,6 +248,12 @@ export type AdReplayStepRuntime = Readonly<{ * actually gets sent; `action` is threaded alongside it only for * daemon-owned, non-interpolation decisions (e.g. a recorded-input * variable heuristic read off the ORIGINAL fill text). + * + * Sole writer of the run's artifact ledger, and the reason the engine keeps + * no accumulator of its own: the incoming `artifactPaths` is the PRE-step + * ledger, and every returned `artifactPaths` is the ledger AFTER this + * step's entries were recorded — cumulative for the run, not just this + * step's (#1478 P5 follow-up; see `./step-loop.ts`'s header). */ dispatchStep( action: SessionAction, diff --git a/packages/ad-replay/src/internal/step-loop.ts b/packages/ad-replay/src/internal/step-loop.ts index 3fbd57182..d191f01df 100644 --- a/packages/ad-replay/src/internal/step-loop.ts +++ b/packages/ad-replay/src/internal/step-loop.ts @@ -68,6 +68,26 @@ import type { * computed ONCE per run and threaded to each build-failure/`handleActionFailure` * capability as an explicit `scrubVars` argument rather than recomputed per * call site — the daemon never holds a `ReplayVarScope` value at all. + * + * #1478 P5 follow-up (one daemon-owned artifact ledger): artifact-path + * accumulation used to be DOUBLE-WRITTEN — `dispatchStep` added each step's + * entries to the daemon's own `Set` (`runReplayScriptFile`'s, read by its + * catch block so a mid-loop throw still reports what was collected) AND + * returned them for this loop to add to a second `Set` of its own. Two + * mutable collections, kept in sync by hand, with no single owner. The + * daemon's `Set` is now the run's ONE ledger: `dispatchStep` writes it and + * returns its contents, and `artifactPaths` below is just the latest such + * return value — re-bound, never mutated. `AdReplayRunOutcome.artifactPaths` + * stays a façade field (it is wire-relevant: the daemon's success response + * reports it), but it is now a projection of what the capability handed back + * rather than an independently accumulated set. + * + * The ledger deliberately does NOT carry a divergence build's own artifacts + * (`buildTargetBindingFailure` and friends produce a fresh capture): those + * reach `handleActionFailure` through `mergeArtifactPaths` as a derived + * value. Writing them into the ledger instead would change what the daemon's + * catch block reports when `handleActionFailure` itself throws — the one + * observable difference between the two old sets, preserved exactly. */ /** @@ -109,7 +129,10 @@ export async function runAdReplay( // in place as each step resolves (tracks which builtins actually expanded, // for `collectReplayScrubbableVarValues`), never rebuilt mid-run. const scope = buildReplayVarScope(request.varSources); - const artifactPaths = new Set(); + // The run's artifact ledger AS THIS ENGINE SEES IT: a plain value re-bound + // to whatever `dispatchStep` last returned, never a collection this module + // mutates — see the module header's ledger note. + let artifactPaths: readonly string[] = []; const snapshotDiagnosticSamples: SnapshotTimingSample[] = []; const terminalCloseIndex = resolveSuppressedTerminalCloseIndex(actions); let replayed = 0; @@ -145,18 +168,17 @@ export async function runAdReplay( action, resolvedAction, index, - [...artifactPaths], + artifactPaths, ); snapshotDiagnosticSamples.push(...runtime.diagnosticsSince(sampleStart)); if (stepOutcome.status === 'ok') { - stepOutcome.artifactPaths.forEach((entry) => artifactPaths.add(entry)); + artifactPaths = stepOutcome.artifactPaths; continue; } - stepOutcome.failure.artifactPaths.forEach((entry) => artifactPaths.add(entry)); const failure = await runtime.handleActionFailure({ action, index, - artifactPaths: [...artifactPaths], + artifactPaths: mergeArtifactPaths(artifactPaths, stepOutcome.failure.artifactPaths), snapshotDiagnosticSamples, scrubVars, }); @@ -170,6 +192,22 @@ export async function runAdReplay( }; } +/** + * The one place this engine combines artifact paths: a failing step's OWN + * artifacts (a divergence build's fresh capture, which the daemon ledger does + * not carry — see the module header) unioned onto the ledger, for + * `handleActionFailure` alone. Deliberately a derived value, not a write: the + * ledger stays the daemon's, and a failing step always ends the run, so + * nothing downstream ever observes this union. + */ +function mergeArtifactPaths( + ledger: readonly string[], + failureArtifactPaths: readonly string[], +): readonly string[] { + if (failureArtifactPaths.length === 0) return ledger; + return [...new Set([...ledger, ...failureArtifactPaths])]; +} + /** `resolveReplayAction`'s `loc` for one step — `actionSourcePaths[index]` when the step came from a `runFlow` include, else the top-level plan's own resolved path. */ function resolveActionLoc( request: AdReplayRunRequest, diff --git a/src/daemon/handlers/__tests__/session-replay-runtime-artifact-ledger.test.ts b/src/daemon/handlers/__tests__/session-replay-runtime-artifact-ledger.test.ts new file mode 100644 index 000000000..c939f5775 --- /dev/null +++ b/src/daemon/handlers/__tests__/session-replay-runtime-artifact-ledger.test.ts @@ -0,0 +1,115 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { test } from 'vitest'; +import { runReplayScriptFile } from '../session-replay-runtime.ts'; +import { SessionStore } from '../../session-store.ts'; +import { makeIosSession } from '../../../__tests__/test-utils/session-factories.ts'; +import { + baseReplayRequest as baseReq, + writeReplayFile, +} from './session-replay-runtime.fixtures.ts'; + +/** + * #1478 P5 follow-up (one daemon-owned artifact ledger): artifact-path + * accumulation used to be double-written — the engine's step loop kept its own + * `Set` alongside this handler's, the two kept in sync by hand. The daemon's is + * now the single ledger; `dispatchStep` writes it and returns its contents, and + * the engine only threads that value. + * + * The ledger's one INDEPENDENT observation point is `runReplayScriptFile`'s + * catch block: on a mid-loop throw there is no run outcome to read artifacts + * from, so what the failure reports comes from the ledger and nothing else. + * That makes this the counterfactual test for the threading — break the ledger + * (drop the `dispatchStep` write, or return only the step's own entries so the + * ledger stops accumulating) and this goes red while the ordinary + * success/divergence paths stay green, because those read the engine's threaded + * value instead. + */ +test('a mid-loop throw reports exactly the artifacts of the steps that ran before it', async () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-replay-artifact-ledger-')); + const sessionStore = new SessionStore(path.join(root, 'sessions')); + const sessionName = 'default'; + sessionStore.set(sessionName, makeIosSession(sessionName, { appBundleId: 'com.example.app' })); + + // Two real files, so `collectReplayActionArtifactPaths`' existence check + // keeps them (it drops any candidate path that is not a file on disk). + const firstArtifact = path.join(root, 'step-1.png'); + const secondArtifact = path.join(root, 'step-2.png'); + fs.writeFileSync(firstArtifact, 'artifact'); + fs.writeFileSync(secondArtifact, 'artifact'); + + // Steps 1 and 2 each produce an artifact; step 3 interpolates a variable + // nothing defines, so `resolveReplayAction` throws INVALID_ARGS from inside + // the step loop — after two steps have already written the ledger, and + // before step 3 dispatches anything of its own. + const filePath = writeReplayFile(root, [ + 'click "First"', + 'click "Second"', + 'click "${UNDEFINED_VAR}"', + ]); + + const dispatched: string[] = []; + const response = await runReplayScriptFile({ + req: baseReq({ positionals: [filePath] }), + sessionName, + logPath: path.join(root, 'daemon.log'), + sessionStore, + invoke: async (req) => { + const label = req.positionals?.[0] ?? ''; + dispatched.push(label); + if (label === 'First') return { ok: true, data: { path: firstArtifact } }; + if (label === 'Second') return { ok: true, data: { path: secondArtifact } }; + throw new Error(`unexpected dispatch of ${label}`); + }, + }); + + // The throw really did land mid-loop: step 3 never reached dispatch. + assert.deepEqual(dispatched, ['First', 'Second']); + assert.equal(response.ok, false); + if (response.ok) return; + assert.equal(response.error.code, 'INVALID_ARGS'); + assert.match(response.error.message, /UNDEFINED_VAR/); + // Exactly the two artifacts, in dispatch order — no more (step 3 produced + // none), no fewer (the ledger accumulated across steps, not just the last). + assert.deepEqual(response.error.details?.artifactPaths, [firstArtifact, secondArtifact]); +}); + +/** + * The ledger's other half: a run that COMPLETES reports the same accumulation + * through the engine's threaded value (`AdReplayRunOutcome.artifactPaths`), + * which must agree with what the ledger holds. Pins that threading the + * capability's return value — rather than accumulating engine-side — still + * yields every step's artifacts, not just the final step's. + */ +test('a completed run reports every step’s artifacts through the threaded ledger value', async () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-replay-artifact-ledger-ok-')); + const sessionStore = new SessionStore(path.join(root, 'sessions')); + const sessionName = 'default'; + sessionStore.set(sessionName, makeIosSession(sessionName, { appBundleId: 'com.example.app' })); + + const firstArtifact = path.join(root, 'step-1.png'); + const secondArtifact = path.join(root, 'step-2.png'); + fs.writeFileSync(firstArtifact, 'artifact'); + fs.writeFileSync(secondArtifact, 'artifact'); + + const filePath = writeReplayFile(root, ['click "First"', 'click "Second"']); + + const response = await runReplayScriptFile({ + req: baseReq({ positionals: [filePath] }), + sessionName, + logPath: path.join(root, 'daemon.log'), + sessionStore, + invoke: async (req) => { + const label = req.positionals?.[0] ?? ''; + if (label === 'First') return { ok: true, data: { path: firstArtifact } }; + if (label === 'Second') return { ok: true, data: { path: secondArtifact } }; + return { ok: true, data: {} }; + }, + }); + + assert.equal(response.ok, true); + if (!response.ok) return; + assert.deepEqual(response.data?.artifactPaths, [firstArtifact, secondArtifact]); +}); diff --git a/src/daemon/handlers/session-replay-runtime-engine-adapter.ts b/src/daemon/handlers/session-replay-runtime-engine-adapter.ts index 34b9250af..06a2684a7 100644 --- a/src/daemon/handlers/session-replay-runtime-engine-adapter.ts +++ b/src/daemon/handlers/session-replay-runtime-engine-adapter.ts @@ -93,7 +93,14 @@ import type { ReplayTestAttemptStepSink } from '@agent-device/replay-test'; export function createAdReplayStepRuntime(params: { ctx: ReplayStepContext; req: DaemonRequest; - /** The outer exception-reporting mirror (see `runReplayScriptFile`'s catch block). */ + /** + * The run's ONE artifact ledger, owned by `runReplayScriptFile`. `dispatchStep` + * is its only writer, and returns its contents for the engine to thread as a + * plain value — the engine keeps no accumulator of its own (#1478 P5 + * follow-up; see `@agent-device/ad-replay`'s `step-loop.ts` header). Also what + * `runReplayScriptFile`'s catch block reports, so a mid-loop throw still names + * the artifacts collected up to that point. + */ artifactPaths: Set; onStep: ReplayTestAttemptStepSink | undefined; armSaveScript: () => void; @@ -218,10 +225,14 @@ export function createAdReplayStepRuntime(params: { invoke: ctx.invoke, }); lastResponse = response; - const entries = collectReplayActionArtifactPaths(response); - entries.forEach((entry) => artifactPaths.add(entry)); - if (response.ok) return { status: 'ok', artifactPaths: entries }; - return classifyReplayDispatchFailure(response, guard, entries); + // The run's one artifact ledger: this step's entries are written into + // it here, and its CONTENTS (not just this step's entries) are what the + // engine gets back to thread as its own `artifactPaths` value — see + // `createAdReplayStepRuntime`'s `artifactPaths` parameter. + collectReplayActionArtifactPaths(response).forEach((entry) => artifactPaths.add(entry)); + const ledger = [...artifactPaths]; + if (response.ok) return { status: 'ok', artifactPaths: ledger }; + return classifyReplayDispatchFailure(response, guard, ledger); }, async buildRecordedUnverifiableFailure(action, index, stepArtifactPaths, scrubVars) { diff --git a/src/daemon/handlers/session-replay-runtime.ts b/src/daemon/handlers/session-replay-runtime.ts index c2947561f..36df7e4aa 100644 --- a/src/daemon/handlers/session-replay-runtime.ts +++ b/src/daemon/handlers/session-replay-runtime.ts @@ -54,10 +54,11 @@ export async function runReplayScriptFile(params: ReplayScriptFileParams): Promi const startedAt = Date.now(); const keepSession = req.flags?.replayKeepSession === true; let resolved = ''; - // The one accumulator `createAdReplayStepRuntime`'s adapter mutates as it - // dispatches/builds each step's failure (via `collectReplayActionArtifactPaths`), - // so a mid-loop exception still reports the artifacts collected up to that - // point. + // The run's ONE artifact ledger (#1478 P5 follow-up): `createAdReplayStepRuntime`'s + // `dispatchStep` is its only writer and hands its contents back to the engine, + // which threads them as a plain value rather than accumulating a second set of + // its own. Read below by the catch block, so a mid-loop exception still reports + // the artifacts collected up to that point. const artifactPaths = new Set(); // #1478 P4b: the one locked coordinator this request reaches the repair // transaction and resume watermark through.