refactor(replay): make the daemon's artifact set the run's one ledger (#1478 P5 follow-up)

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,

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<string>();
+  // 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,

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]);
+});

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<string>;
   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) {

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<string>();
   // #1478 P4b: the one locked coordinator this request reaches the repair
   // transaction and resume watermark through.