fix(replay): repair-transaction lifecycle — keep-alive, no-partial-emit, close-as-lifecycle, atomic publish

docs/adr/0012-interactive-replay.md

@@ -576,10 +576,10 @@ Implementation is not accepted on benchmark evidence alone. Required automated c
   `COMPLETE` transaction committing (not tombstoning) — and is idempotent with no re-publish once
   `COMMITTED`; a terminal-close test proving an armed replay reaching the terminal source `close`
   **skips** it without deleting the session (C4); an atomic-publication test proving the temp file is
-  created in the target's own directory and the no-clobber is race-safe (create-exclusive/rename-if-absent);
-  and a no-clobber test proving an existing *complete* (sentinel-marked) healed artifact is protected
-  while an incomplete/partial one is
-  overwritable.
+  created in the target's own directory and published via a single exclusive `linkSync`
+  (create-if-absent, first writer wins); and a no-clobber test proving publication refuses ANY
+  pre-existing target — complete or partial alike, byte-for-byte unchanged — for both the default healed
+  sibling and an explicit `--save-script=<path>`.
 
 Extend the settle benchmark (`~/.agent-device-bench/rnnav-matrix.py` pattern, external harness) with a
 replay arm only after these contracts pass: measure clean replay and one induced divergence repaired
@@ -791,19 +791,23 @@ double-commit nor trip the no-clobber guard).
 
 **Atomic, race-safe publication.** The commit serializes the healed slice (ending with a serialized
 `close` line so the artifact is self-contained) to a temp file created **in the same directory as the
-final target** — atomic `rename` requires the same filesystem, and an explicit `--save-script=<path>`
-target may live on a different mount than any process-wide temp dir — then atomically renames it into
-place. A reader therefore never observes a half-written healed script, and an aborted repair leaves no
-partial behind. The no-clobber guard is enforced **race-safely** by the atomic primitive itself
-(create-exclusive / rename-if-absent), not by a check-then-write.
-
-**No-clobber applies only to a COMPLETE healed artifact.** The guard protects a *complete, review-worthy*
-artifact only. An incomplete or partial file is always overwritable by a fresh repair that reaches
-`COMMITTED`. Completeness is established by a sentinel written **only on commit** — a trailing
-`# agent-device:heal-complete` marker — or an equivalent structural check; the atomic temp→publish flow is
-what makes that sentinel trustworthy (a partial never reaches the published path carrying the sentinel).
-Auto-versioned output names (e.g. `.healed.2.ad`) are explicitly **out of scope** here — a separate
-naming change, not part of this decision.
+final target** — an explicit `--save-script=<path>` target may live on a different mount than any
+process-wide temp dir, and `linkSync` requires the same filesystem — then publishes with a single
+exclusive `linkSync(temp, target)`: create-if-absent, first writer wins. A reader therefore never
+observes a half-written healed script, and an aborted repair leaves no partial behind.
+
+**Publication refuses ANY pre-existing target — complete or partial, default or explicit path alike.**
+An earlier design distinguished a *complete, review-worthy* artifact (protected) from an incomplete or
+partial one (silently overwritable), enforced through a publish lock. That distinction added a whole
+class of lock/lease/reclaim races for a case that is, in practice, a degenerate state: a partial healed
+file left behind by an aborted or reaped repair. The simpler, adopted design collapses this: the atomic
+`linkSync` itself decides the winner (`EEXIST` iff a file already sits at the target, regardless of its
+contents), and the caller must explicitly clear the way — remove the existing file, or pass a different
+`replay --save-script=<path>` — rather than have it silently replaced. No lock, no lease, no steal, no
+overwrite. The `# agent-device:heal-complete` trailing sentinel remains — it still marks a healed `.ad` as
+a complete, review-worthy repair artifact for any other reader — but it no longer participates in the
+publish decision. Auto-versioned output names (e.g. `.healed.2.ad`) are explicitly **out of scope** here —
+a separate naming change, not part of this decision.
 
 **Repair-session tombstone (R7 ownership).** When a bounded-expiry escape hatch idle-reaps (or a daemon
 shutdown tears down) a repair-armed transaction that has **not** reached `COMPLETE`, the teardown aborts
@@ -924,8 +928,8 @@ each states its dependencies explicitly.
    regression proving the session is NOT deleted when the terminal `close` is reached; the
    `REPAIR_SESSION_EXPIRED` tombstone for an incomplete-transaction reap/shutdown (keyed by session key,
    owner + bounded expiry, cleared by a fresh `replay --save-script`); and race-safe atomic publication
-   (temp file in the target's own directory, create-exclusive/rename-if-absent no-clobber against the
-   `# agent-device:heal-complete` sentinel).
+   (temp file in the target's own directory, published via a single exclusive `linkSync` that refuses ANY
+   pre-existing target — complete or partial, default or explicit path alike — never overwriting one).
 
 ## Migration progress
 

src/daemon/__tests__/request-router-repair-expired.test.ts

@@ -0,0 +1,163 @@
+/**
+ * ADR 0012 decision 6, R7 (C5a): when a repair session was reaped before it was
+ * finalized, the request router rewrites the resulting `SESSION_NOT_FOUND` into
+ * a `REPAIR_SESSION_EXPIRED` recovery error with actionable re-run guidance,
+ * rather than leaking a bare SESSION_NOT_FOUND. Any other error, or the absence
+ * of a live tombstone, passes through unchanged.
+ */
+import { test, expect, vi } from 'vitest';
+import fs from 'node:fs';
+import os from 'node:os';
+import path from 'node:path';
+import { getResolveTargetDeviceMock } from './request-router-dispatch-mocks.ts';
+
+vi.mock('../device-ready.ts', () => ({ ensureDeviceReady: vi.fn(async () => {}) }));
+
+import { createRequestHandler } from '../request-router.ts';
+import type { DaemonRequest, SessionState } from '../types.ts';
+import type { DeviceInfo } from '../../kernel/device.ts';
+import { LeaseRegistry } from '../lease-registry.ts';
+import { makeSessionStore } from '../../__tests__/test-utils/store-factory.ts';
+import { parseReplayInput } from '../../compat/replay-input.ts';
+import { computeReplayPlanDigest } from '../../replay/plan-digest.ts';
+import { readEffectiveReplayPlanDigestMetadata } from '../handlers/session-replay-runtime-plan.ts';
+
+const mockResolveTargetDevice = vi.mocked(getResolveTargetDeviceMock());
+
+function makeHandler(prefix: string) {
+  const sessionStore = makeSessionStore(prefix);
+  const handler = createRequestHandler({
+    logPath: path.join(os.tmpdir(), 'daemon.log'),
+    token: 'test-token',
+    sessionStore,
+    leaseRegistry: new LeaseRegistry(),
+    trackDownloadableArtifact: () => 'artifact-id',
+  });
+  return { sessionStore, handler };
+}
+
+function tombstonedSession(name: string): SessionState {
+  return {
+    name,
+    device: { platform: 'apple', id: 'sim-1', name: 'iPhone', kind: 'simulator', booted: true },
+    createdAt: Date.now(),
+    actions: [],
+    saveScriptBoundary: 0,
+    repairSourcePath: '/flows/login.ad',
+  };
+}
+
+function closeRequest(session: string): DaemonRequest {
+  return { token: 'test-token', session, command: 'close', positionals: [], flags: {} };
+}
+
+test('a command that finds no session but hits a live repair tombstone gets REPAIR_SESSION_EXPIRED', async () => {
+  const { sessionStore, handler } = makeHandler('agent-device-router-repair-expired-');
+  // The repair session was reaped (idle-reap) leaving a tombstone; the store
+  // has no live session by that name.
+  sessionStore.writeRepairTombstone(tombstonedSession('repair-x'));
+
+  const response = await handler(closeRequest('repair-x'));
+
+  expect(response.ok).toBe(false);
+  if (response.ok) return;
+  expect(response.error.code).toBe('REPAIR_SESSION_EXPIRED');
+  // Re-run guidance carries the original script path from the tombstone.
+  expect(response.error.message).toMatch(/replay \/flows\/login\.ad --save-script/);
+});
+
+test('without a tombstone, a missing session still returns a plain SESSION_NOT_FOUND', async () => {
+  const { handler } = makeHandler('agent-device-router-no-tombstone-');
+  const response = await handler(closeRequest('never-existed'));
+
+  expect(response.ok).toBe(false);
+  if (response.ok) return;
+  expect(response.error.code).toBe('SESSION_NOT_FOUND');
+});
+
+// ADR 0012 decision 6 (BLOCKER 2): a tombstone left by a COMPLETE transaction
+// whose commit FAILED at teardown must surface a distinct, actionable
+// REPAIR_COMMIT_FAILED — never the generic "reaped before it was finalized"
+// REPAIR_SESSION_EXPIRED, which would misleadingly suggest the transaction
+// never completed at all.
+test('a command hitting a commit-failure tombstone gets REPAIR_COMMIT_FAILED with the real cause, not a generic REPAIR_SESSION_EXPIRED', async () => {
+  const { sessionStore, handler } = makeHandler('agent-device-router-commit-failed-');
+  sessionStore.writeRepairTombstone(tombstonedSession('repair-commit-fail'), undefined, {
+    code: 'COMMAND_FAILED',
+    message: 'A prior healed script already exists at /flows/login.healed.ad; ...',
+  });
+
+  const response = await handler(closeRequest('repair-commit-fail'));
+
+  expect(response.ok).toBe(false);
+  if (response.ok) return;
+  expect(response.error.code).toBe('REPAIR_COMMIT_FAILED');
+  expect(response.error.code).not.toBe('REPAIR_SESSION_EXPIRED');
+  expect(response.error.message).toMatch(/already exists/);
+  // Still carries the actionable re-run guidance.
+  expect(response.error.message).toMatch(/replay \/flows\/login\.ad --save-script/);
+});
+
+test('an expired tombstone does not shadow a missing session', async () => {
+  const { sessionStore, handler } = makeHandler('agent-device-router-expired-tombstone-');
+  // TTL 0 => already stale.
+  sessionStore.writeRepairTombstone(tombstonedSession('repair-y'), 0);
+
+  const response = await handler(closeRequest('repair-y'));
+
+  expect(response.ok).toBe(false);
+  if (response.ok) return;
+  expect(response.error.code).toBe('SESSION_NOT_FOUND');
+});
+
+// ADR 0012 decision 6, R7 (BLOCKER 1): the tombstone translation also covers the
+// `replay --from` continuation path — a reaped repair session's continuation
+// gets REPAIR_SESSION_EXPIRED, not a REPLAY_DIVERGENCE that slipped past it.
+test('a replay --from continuation on a reaped repair session gets REPAIR_SESSION_EXPIRED (not REPLAY_DIVERGENCE)', async () => {
+  // Device resolution/readiness is mocked so the router reaches the replay
+  // handler's missing-session preflight fast and deterministically.
+  const iosDevice: DeviceInfo = {
+    platform: 'apple',
+    id: 'sim-1',
+    name: 'iPhone',
+    kind: 'simulator',
+    booted: true,
+  };
+  mockResolveTargetDevice.mockResolvedValue(iosDevice);
+  const { sessionStore, handler } = makeHandler('agent-device-router-from-expired-');
+  const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-router-from-script-'));
+  const scriptPath = path.join(root, 'flow.ad');
+  fs.writeFileSync(scriptPath, 'open "Demo"\nclick id="a"\n');
+
+  // Compute the plan digest exactly as runReplayScriptFile does (a real agent
+  // takes it from the divergence report's resume.planDigest).
+  const flags = { platform: 'ios' as const };
+  const parsed = parseReplayInput(fs.readFileSync(scriptPath, 'utf8'), flags, {
+    sourcePath: scriptPath,
+  });
+  const digest = computeReplayPlanDigest({
+    actions: parsed.actions,
+    actionLines: parsed.actionLines,
+    actionSourcePaths: parsed.actionSourcePaths,
+    metadata: readEffectiveReplayPlanDigestMetadata(flags),
+  });
+
+  // The repair session was reaped, leaving a tombstone; no live session exists.
+  sessionStore.writeRepairTombstone(tombstonedSession('repair-from'));
+
+  const response = await handler({
+    token: 'test-token',
+    session: 'repair-from',
+    command: 'replay',
+    positionals: [scriptPath],
+    flags: { ...flags, replayFrom: 2, replayPlanDigest: digest },
+  });
+
+  expect(response.ok).toBe(false);
+  if (response.ok) return;
+  expect(response.error.code).toBe('REPAIR_SESSION_EXPIRED');
+  expect(response.error.code).not.toBe('REPLAY_DIVERGENCE');
+  expect(response.error.message).toMatch(/replay \/flows\/login\.ad --save-script/);
+
+  fs.rmSync(root, { recursive: true, force: true });
+});

src/daemon/__tests__/request-router-typed-error.test.ts

@@ -122,3 +122,48 @@ test('deterministic errors (INVALID_ARGS) are returned with the default shape 
   expect('supportedOn' in response.error).toBe(false);
   expect(mockDispatch).not.toHaveBeenCalled();
 });
+
+// ADR 0012 decision 6, BLOCKER 2 (second follow-up): a repair-armed `close`
+// whose targeted platform close fails must surface `retriable: true` at the
+// TOP level of the wire error — the location `enrichDaemonError` below and
+// the client actually read (`DaemonError.retriable` in kernel/contracts.ts) —
+// and must preserve the underlying platform error's own diagnosticId/logPath/
+// details rather than discarding them. Exercised through the REAL router
+// boundary (`createRequestHandler`), not just the raw response builder, so a
+// regression in either the handler OR `enrichDaemonError`'s own
+// `error.retriable ?? retriableForErrorCode(error.code)` fallback is caught.
+test('BLOCKER 2 (second follow-up): a repair-close platform-close failure surfaces retriable:true and diagnosticId/logPath/details at the TOP level through the router', async () => {
+  const { sessionStore, handler } = makeHandler();
+  const session = makeIosSession('typed-error');
+  session.recordSession = true;
+  session.saveScriptBoundary = 0;
+  session.saveScriptComplete = true;
+  session.actions = [{ ts: 1, command: 'open', positionals: ['Demo'], flags: {} }];
+  sessionStore.set('typed-error', session);
+
+  // DEVICE_NOT_FOUND is not in `retriableForErrorCode`'s conservative allow
+  // list — if the handler ever regressed to relying on that code-level
+  // fallback instead of forcing `retriable: true` itself, this would catch it.
+  mockDispatch.mockRejectedValueOnce(
+    new AppError('DEVICE_NOT_FOUND', 'device vanished', {
+      diagnosticId: 'diag-router-close-1',
+      logPath: '/tmp/router-close-1.log',
+      someExtra: 'x',
+    }),
+  );
+
+  // A targeted close (an explicit positional app target) is what makes the
+  // repair-armed platform close actually dispatch instead of no-op.
+  const response = await handler(request('close', { positionals: ['com.example.app'] }));
+
+  expect(response.ok).toBe(false);
+  if (response.ok) return;
+  expect(response.error.retriable).toBe(true);
+  expect('retriable' in (response.error.details ?? {})).toBe(false);
+  expect(response.error.diagnosticId).toBe('diag-router-close-1');
+  expect(response.error.logPath).toBe('/tmp/router-close-1.log');
+  expect(response.error.details?.someExtra).toBe('x');
+  expect(response.error.code).toBe('DEVICE_NOT_FOUND');
+  // The session is retained (not torn down), addressable for the retry.
+  expect(sessionStore.get('typed-error')).toBeDefined();
+});