diff --git a/docs/adr/0012-interactive-replay.md b/docs/adr/0012-interactive-replay.md index d5882087b..74151fe9b 100644 --- a/docs/adr/0012-interactive-replay.md +++ b/docs/adr/0012-interactive-replay.md @@ -576,10 +576,12 @@ 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=`, **and for an ordinary (non-repair) `open`/`close + --save-script` recording whose target already exists** — the writer entry point and publish primitive + are shared, so the refusal is uniform, not repair-only (see "Scope" below). 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 +793,37 @@ 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=` -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=` 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=` — 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. + +**Scope: this refusal is uniform across repair AND ordinary recording, not repair-only.** Everything +above is written in the context of decision 6's repair-armed heal, but `publishHealedScriptAtomically` +(`src/daemon/session-script-writer.ts`) is the single publish primitive `SessionScriptWriter.write` calls +for every `--save-script` target, with no repair-armed-vs-ordinary branch — a repair-armed heal +(`saveScriptBoundary` set) and an ordinary, non-repair `open --save-script`/`close --save-script` +recording (`saveScriptBoundary` absent) publish through the exact same exclusive `linkSync`. Before this +decision, ordinary recording published via a separate atomic rename-replace path +(`publishOverwriteAtomically`, since removed), silently overwriting an existing target; that overwrite +path is gone. An ordinary recording whose target already exists is now refused exactly like a healed +repair publish would be: `EEXIST` surfaces as the same `AppError`, the existing file is left +byte-for-byte unchanged, and the caller must remove it or choose a different `--save-script=`. +There is no `--force`/`--overwrite` escape hatch for either path today; that is tracked as a future +follow-up (#1258), 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 +944,10 @@ 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, and uniformly for an + ordinary non-repair recording's target too, since the publish primitive is shared — never overwriting + one; see "Scope" under Decision 6 above). ## Migration progress diff --git a/src/daemon/__tests__/request-router-repair-expired.test.ts b/src/daemon/__tests__/request-router-repair-expired.test.ts new file mode 100644 index 000000000..5da899fb1 --- /dev/null +++ b/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 }); +}); diff --git a/src/daemon/__tests__/request-router-typed-error.test.ts b/src/daemon/__tests__/request-router-typed-error.test.ts index cff9d44b8..9bbd2f09f 100644 --- a/src/daemon/__tests__/request-router-typed-error.test.ts +++ b/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(); +}); diff --git a/src/daemon/__tests__/session-script-writer.test.ts b/src/daemon/__tests__/session-script-writer.test.ts index a28e01d19..56565f213 100644 --- a/src/daemon/__tests__/session-script-writer.test.ts +++ b/src/daemon/__tests__/session-script-writer.test.ts @@ -1,8 +1,8 @@ -import { test, expect } from 'vitest'; +import { test, expect, vi } from 'vitest'; import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; -import { SessionScriptWriter } from '../session-script-writer.ts'; +import { HEAL_COMPLETE_SENTINEL, SessionScriptWriter } from '../session-script-writer.ts'; import { recordActionEntry } from '../session-action-recorder.ts'; import { makeIosSession } from '../../__tests__/test-utils/session-factories.ts'; import { parseReplayScriptDetailed } from '../../replay/script.ts'; @@ -30,6 +30,10 @@ test('write() slices session.actions from saveScriptBoundary onward, excluding p const session = makeIosSession('default', { recordSession: true, saveScriptBoundary: 2, + // Fix 2: a repair-armed write only publishes once explicitly finalized + // (`close --save-script`) — set here to isolate THIS test's own concern + // (boundary slicing), covered separately below. + saveScriptComplete: true, actions: [ action({ command: 'open', positionals: ['Demo'] }), action({ command: 'click', positionals: ['label="Old"'] }), @@ -66,6 +70,7 @@ test('a boundary-sliced script still strips diagnostic snapshot actions', () => const session = makeIosSession('default', { recordSession: true, saveScriptBoundary: 1, + saveScriptComplete: true, actions: [ action({ command: 'open', positionals: ['Demo'] }), action({ command: 'snapshot', positionals: [] }), @@ -90,6 +95,7 @@ test('a recorded ref that resolved to a selectorChain writes a clean selector li const session = makeIosSession('default', { recordSession: true, saveScriptBoundary: 0, + saveScriptComplete: true, actions: [ action({ command: 'press', @@ -111,12 +117,17 @@ test('a recorded ref that never resolved to a selectorChain throws instead of em const session = makeIosSession('default', { recordSession: true, saveScriptBoundary: 0, + saveScriptComplete: true, actions: [action({ command: 'press', positionals: ['@e7'] })], }); const scriptPath = path.join(root, 'sessions', 'default', 'expected-not-written.ad'); - expect(() => writer.write(session)).toThrow(/never resolved to a selector/); - // Fail loud, not a swallowed { written: false } — no file was produced. + // BLOCKER 2: a repair commit failure is SURFACED via the result (never + // swallowed into a bare `{written:false}`), not thrown — so close/teardown + // can report it and keep the session for retry. + const result = writer.write(session); + expect(result.written).toBe(false); + expect(result.written === false && result.error?.message).toMatch(/never resolved to a selector/); expect(fs.existsSync(scriptPath)).toBe(false); expect(fs.readdirSync(path.join(root, 'sessions')).length).toBe(0); }); @@ -127,10 +138,13 @@ test('a bare-@ref fill action also fails loud, not just click-like commands', () const session = makeIosSession('default', { recordSession: true, saveScriptBoundary: 0, + saveScriptComplete: true, actions: [action({ command: 'fill', positionals: ['@e9', 'hello'] })], }); - expect(() => writer.write(session)).toThrow(/never resolved to a selector/); + const result = writer.write(session); + expect(result.written).toBe(false); + expect(result.written === false && result.error?.message).toMatch(/never resolved to a selector/); }); test('a bare @ref later in the same session (after a resolved earlier action) still fails loud, writing nothing', () => { @@ -139,6 +153,7 @@ test('a bare @ref later in the same session (after a resolved earlier action) st const session = makeIosSession('default', { recordSession: true, saveScriptBoundary: 0, + saveScriptComplete: true, actions: [ action({ command: 'open', positionals: ['Demo'] }), action({ @@ -150,7 +165,9 @@ test('a bare @ref later in the same session (after a resolved earlier action) st ], }); - expect(() => writer.write(session)).toThrow(/never resolved to a selector/); + const result = writer.write(session); + expect(result.written).toBe(false); + expect(result.written === false && result.error?.message).toMatch(/never resolved to a selector/); expect(fs.readdirSync(path.join(root, 'sessions')).length).toBe(0); }); @@ -174,58 +191,293 @@ test('an ordinary (non-repair-armed) recording keeps the existing bare-ref fallb }); // --- ADR 0012 decision 6 (P2): the default `.healed.ad` sibling is never -// silently clobbered — a human must review each healed diff before promoting. --- +// silently clobbered — a human must review each healed diff before promoting. +// The publish primitive refuses ANY pre-existing target, complete or +// partial, uniformly (no lock, no lease, no overwrite). --- + +// (a) publishing to an ABSENT target succeeds. +test('write() publishes cleanly when the target does not exist yet', () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-script-writer-absent-')); + const writer = new SessionScriptWriter(path.join(root, 'sessions')); + const healedPath = path.join(root, 'flows', 'login.healed.ad'); + + const session = makeIosSession('default', { + recordSession: true, + saveScriptBoundary: 0, + saveScriptComplete: true, + saveScriptPath: healedPath, + saveScriptDefaultedHealedPath: true, + actions: [action({ command: 'click', positionals: ['id="new"'] })], + }); -test('write() refuses to clobber an existing DEFAULT .healed.ad (no explicit --save-script=)', () => { + const result = writer.write(session); + expect(result.written).toBe(true); + expect(result.written && result.path).toBe(healedPath); + const script = fs.readFileSync(healedPath, 'utf8'); + expect(script).toContain(HEAL_COMPLETE_SENTINEL); + const parsed = parseReplayScriptDetailed(script); + expect(parsed.actions.map((a) => a.positionals[0])).toEqual(['id="new"']); +}); + +// (b) publishing when a COMPLETE sentinel-marked artifact exists is REFUSED, +// bytes unchanged. +test('write() refuses to clobber an existing COMPLETE DEFAULT .healed.ad', () => { const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-script-writer-clobber-')); const writer = new SessionScriptWriter(path.join(root, 'sessions')); const healedPath = path.join(root, 'flows', 'login.healed.ad'); fs.mkdirSync(path.dirname(healedPath), { recursive: true }); - // A prior, unreviewed healed script already sits at the default sibling path. - fs.writeFileSync(healedPath, 'context platform=ios device="x"\nclick id="old"\n'); + // A prior, unreviewed, COMPLETE healed script already sits at the default + // sibling path — the publish primitive refuses ANY pre-existing target, so + // this is refused regardless of the sentinel. + fs.writeFileSync( + healedPath, + `context platform=ios device="x"\nclick id="old"\n${HEAL_COMPLETE_SENTINEL}\n`, + ); const before = fs.readFileSync(healedPath, 'utf8'); const session = makeIosSession('default', { recordSession: true, saveScriptBoundary: 0, + saveScriptComplete: true, saveScriptPath: healedPath, saveScriptDefaultedHealedPath: true, actions: [action({ command: 'click', positionals: ['id="new"'] })], }); - expect(() => writer.write(session)).toThrow(/already exists/); - // Fail loud — the prior unreviewed diff is untouched. + // BLOCKER 2c: a no-clobber refusal is surfaced via the result's error (a + // distinct "already exists" message), not thrown; the prior complete diff is + // untouched. + const result = writer.write(session); + expect(result.written).toBe(false); + expect(result.written === false && result.error?.message).toMatch(/already exists/); expect(fs.readFileSync(healedPath, 'utf8')).toBe(before); }); -test('write() DOES overwrite when the caller passed an explicit --save-script= (not defaulted)', () => { +test('write() now refuses to clobber a stale PARTIAL (non-sentinel) .healed.ad at the default path too (behavior change: no more auto-overwrite)', () => { + const root = fs.mkdtempSync( + path.join(os.tmpdir(), 'agent-device-script-writer-clobber-partial-'), + ); + const writer = new SessionScriptWriter(path.join(root, 'sessions')); + const healedPath = path.join(root, 'flows', 'login.healed.ad'); + fs.mkdirSync(path.dirname(healedPath), { recursive: true }); + // A partial left over from a diverged-and-abandoned repair (pre-Fix-2 bug, + // or any other incomplete write) — no completeness sentinel. The lock/lease + // machinery used to distinguish this from a COMPLETE artifact and silently + // overwrite it; the simplified publish primitive refuses ANY pre-existing + // target uniformly, complete or partial alike. + fs.writeFileSync(healedPath, 'context platform=ios device="x"\nclick id="stale-partial"\n'); + const before = fs.readFileSync(healedPath, 'utf8'); + + const session = makeIosSession('default', { + recordSession: true, + saveScriptBoundary: 0, + saveScriptComplete: true, + saveScriptPath: healedPath, + saveScriptDefaultedHealedPath: true, + actions: [action({ command: 'click', positionals: ['id="new"'] })], + }); + + const result = writer.write(session); + expect(result.written).toBe(false); + expect(result.written === false && result.error?.message).toMatch(/already exists/); + expect(fs.readFileSync(healedPath, 'utf8')).toBe(before); +}); + +// The lock/lease machinery previously here (acquireLease/releaseLease/ +// stealExpiredLease/the three-writer interleaving) is gone: the publish +// primitive is now a single exclusive `linkSync`, which already decides a +// concurrent race correctly without any lock — first writer wins, the loser +// sees `EEXIST` and is refused. +test('two writers racing on the SAME ABSENT target: exactly one linkSync wins, the other is refused (no lock involved)', () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-script-writer-race-')); + const writer = new SessionScriptWriter(path.join(root, 'sessions')); + const healedPath = path.join(root, 'flows', 'login.healed.ad'); + fs.mkdirSync(path.dirname(healedPath), { recursive: true }); + + const sessionA = makeIosSession('writer-a', { + recordSession: true, + saveScriptBoundary: 0, + saveScriptComplete: true, + saveScriptPath: healedPath, + saveScriptDefaultedHealedPath: true, + actions: [action({ command: 'click', positionals: ['id="from-a"'] })], + }); + const sessionB = makeIosSession('writer-b', { + recordSession: true, + saveScriptBoundary: 0, + saveScriptComplete: true, + saveScriptPath: healedPath, + saveScriptDefaultedHealedPath: true, + actions: [action({ command: 'click', positionals: ['id="from-b"'] })], + }); + + // Force a genuine interleaving deterministically: just before writer A's + // own final `linkSync` into `healedPath` runs, drive writer B's ENTIRE + // publish to completion first — both writers started against the same + // absent target, but B's own `linkSync` gets there first. + const realLinkSync = fs.linkSync; + let triggeredCompetingWriter = false; + let resultB: ReturnType | undefined; + const linkSpy = vi + .spyOn(fs, 'linkSync') + .mockImplementation((existingPath: fs.PathLike, newPath: fs.PathLike) => { + if (!triggeredCompetingWriter && newPath === healedPath) { + triggeredCompetingWriter = true; + resultB = writer.write(sessionB); + } + return realLinkSync(existingPath, newPath); + }); + + const resultA = writer.write(sessionA); + linkSpy.mockRestore(); + + expect(resultB).toBeDefined(); + // Exactly one writer wins (its own linkSync creates the file), the other's + // subsequent linkSync sees EEXIST and is cleanly refused — never both + // silently succeeding, never a torn/mixed file. + const outcomes = [resultA, resultB!]; + const wins = outcomes.filter((r) => r.written); + const losses = outcomes.filter((r) => !r.written); + expect(wins).toHaveLength(1); + expect(losses).toHaveLength(1); + expect(losses[0]!.written === false && losses[0]!.error?.message).toMatch(/already exists/); + + const finalScript = fs.readFileSync(healedPath, 'utf8'); + expect(finalScript).toContain(HEAL_COMPLETE_SENTINEL); + const parsed = parseReplayScriptDetailed(finalScript); + const winnerLabel = resultB!.written ? 'id="from-b"' : 'id="from-a"'; + expect(parsed.actions.map((a) => a.positionals[0])).toEqual([winnerLabel]); +}); + +// BLOCKER 4: the no-clobber protection applies to an EXPLICIT +// `--save-script=` target identically to the default healed sibling — +// an explicit target is caller-DIRECTED (which path to use), never +// caller-AUTHORIZED to silently destroy an unreviewed prior healed diff +// sitting there. +test('write() refuses to clobber an existing COMPLETE artifact at an EXPLICIT --save-script= target too', () => { + const root = fs.mkdtempSync( + path.join(os.tmpdir(), 'agent-device-script-writer-explicit-complete-clobber-'), + ); + const writer = new SessionScriptWriter(path.join(root, 'sessions')); + const explicitOut = path.join(root, 'flows', 'promoted.ad'); + fs.mkdirSync(path.dirname(explicitOut), { recursive: true }); + fs.writeFileSync( + explicitOut, + `context platform=ios device="x"\nclick id="old"\n${HEAL_COMPLETE_SENTINEL}\n`, + ); + const before = fs.readFileSync(explicitOut, 'utf8'); + + const session = makeIosSession('default', { + recordSession: true, + saveScriptBoundary: 0, + saveScriptComplete: true, + saveScriptPath: explicitOut, + // No saveScriptDefaultedHealedPath: this is an explicit, caller-directed + // target — the protection must apply here too, not just the default path. + actions: [action({ command: 'click', positionals: ['id="new"'] })], + }); + + const result = writer.write(session); + expect(result.written).toBe(false); + expect(result.written === false && result.error?.message).toMatch(/already exists/); + // The prior complete diff at the explicit target is untouched. + expect(fs.readFileSync(explicitOut, 'utf8')).toBe(before); +}); + +// (d) an explicit --save-script= to an existing file is REFUSED +// identically to the default path — behavior change: this used to succeed +// (a non-sentinel/partial file was freely overwritable at an explicit path). +test('write() now refuses an explicit --save-script= pointing at an existing (non-sentinel) file too', () => { const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-script-writer-explicit-out-')); const writer = new SessionScriptWriter(path.join(root, 'sessions')); const outPath = path.join(root, 'flows', 'explicit.ad'); fs.mkdirSync(path.dirname(outPath), { recursive: true }); fs.writeFileSync(outPath, 'context platform=ios device="x"\nclick id="old"\n'); + const before = fs.readFileSync(outPath, 'utf8'); const session = makeIosSession('default', { recordSession: true, saveScriptBoundary: 0, + saveScriptComplete: true, saveScriptPath: outPath, // No saveScriptDefaultedHealedPath: the caller directed this path explicitly. actions: [action({ command: 'click', positionals: ['id="new"'] })], }); + const result = writer.write(session); + expect(result.written).toBe(false); + expect(result.written === false && result.error?.message).toMatch(/already exists/); + expect(fs.readFileSync(outPath, 'utf8')).toBe(before); +}); + +// (e) the refuse-on-exist publish primitive is shared with ORDINARY +// (non-repair) recording too — no `saveScriptBoundary` at all, i.e. a plain +// `open --save-script`/`close --save-script` session, never `replay +// --save-script`. Before the maintainer-approved uniform simplification, +// this path published via a separate atomic rename-replace and silently +// overwrote an existing target; that overwrite path is gone (see the ADR's +// "Scope" note under decision 6). This is the coverage gap the reviewer +// flagged on #1235: only repair-armed (`saveScriptBoundary` set, even to 0) +// sessions were exercised above. +test('an ordinary (non-repair) recording now refuses an existing target too (behavior change: no more rename-replace overwrite)', () => { + const root = fs.mkdtempSync( + path.join(os.tmpdir(), 'agent-device-script-writer-ordinary-clobber-'), + ); + const writer = new SessionScriptWriter(path.join(root, 'sessions')); + const outPath = path.join(root, 'flows', 'existing.ad'); + fs.mkdirSync(path.dirname(outPath), { recursive: true }); + fs.writeFileSync(outPath, 'context platform=ios device="x"\nclick id="old"\n'); + const before = fs.readFileSync(outPath, 'utf8'); + + const session = makeIosSession('default', { + recordSession: true, + // No saveScriptBoundary: an ordinary open/close --save-script recording, + // never armed via `replay --save-script` — this is NOT a repair. + saveScriptPath: outPath, + actions: [action({ command: 'click', positionals: ['id="new"'] })], + }); + + // Unlike a repair-armed write (which returns `{written:false, error}`), an + // ordinary (non-repair-armed) write's catch block RETHROWS an AppError + // (`write()`: `if (repairArmed) return {...}; if (error instanceof + // AppError) throw error;`) — so refusal here surfaces as a thrown error, + // not a returned result. + expect(() => writer.write(session)).toThrow(/already exists/); + // The pre-existing target is left byte-for-byte unchanged — refused, not + // clobbered. + expect(fs.readFileSync(outPath, 'utf8')).toBe(before); +}); + +// (f) confirm the absent-target case still succeeds for ordinary recording — +// the refusal is scoped to an EXISTING target, not a blanket block. +test('an ordinary (non-repair) recording still publishes cleanly when its target does not exist yet', () => { + const root = fs.mkdtempSync( + path.join(os.tmpdir(), 'agent-device-script-writer-ordinary-absent-'), + ); + const writer = new SessionScriptWriter(path.join(root, 'sessions')); + const outPath = path.join(root, 'flows', 'fresh.ad'); + + const session = makeIosSession('default', { + recordSession: true, + // No saveScriptBoundary: ordinary recording, not a repair. + saveScriptPath: outPath, + actions: [action({ command: 'click', positionals: ['id="new"'] })], + }); + const result = writer.write(session); expect(result.written).toBe(true); + expect(result.written && result.path).toBe(outPath); const parsed = parseReplayScriptDetailed(fs.readFileSync(outPath, 'utf8')); expect(parsed.actions.map((a) => a.positionals[0])).toEqual(['id="new"']); + // Ordinary recording never carries the repair-only completeness sentinel. + expect(fs.readFileSync(outPath, 'utf8')).not.toContain(HEAL_COMPLETE_SENTINEL); }); -test('close --save-script= clears the defaulted marker, so an explicit overwrite of an existing file SUCCEEDS', () => { +test('close --save-script= clears the defaulted marker, and a write to that (absent) explicit path succeeds', () => { const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-script-writer-close-explicit-')); const writer = new SessionScriptWriter(path.join(root, 'sessions')); const defaultedHealed = path.join(root, 'flows', 'login.healed.ad'); const explicitOut = path.join(root, 'flows', 'promoted.ad'); - fs.mkdirSync(path.dirname(explicitOut), { recursive: true }); - fs.writeFileSync(explicitOut, 'context platform=ios device="x"\nclick id="old"\n'); // The repair defaulted to `.healed.ad` (marker set). const session = makeIosSession('default', { @@ -236,9 +488,11 @@ test('close --save-script= clears the defaulted marker, so an exp actions: [action({ command: 'click', positionals: ['id="new"'] })], }); - // `close --save-script=` re-points the path AND - // clears the marker (regression: it used to retain the marker and wrongly - // refuse the explicit overwrite). + // `close --save-script=` re-points the path AND clears the + // marker (regression: it used to retain the marker and wrongly refuse the + // explicit target). The marker no longer affects the publish decision at + // all (refusal is now uniform), but a redirected session must still + // publish cleanly to its own (absent) explicit target. recordActionEntry(session, { command: 'close', positionals: [], @@ -246,6 +500,10 @@ test('close --save-script= clears the defaulted marker, so an exp }); expect(session.saveScriptDefaultedHealedPath).toBe(false); expect(session.saveScriptPath).toBe(explicitOut); + // `recordActionEntry` is the low-level action recorder `close`'s handler + // calls on its way to setting the finalize signal (Fix 2) — set here to + // isolate this test's own concern (defaulted-marker clearing). + session.saveScriptComplete = true; const result = writer.write(session); expect(result.written).toBe(true); @@ -253,3 +511,123 @@ test('close --save-script= clears the defaulted marker, so an exp const parsed = parseReplayScriptDetailed(fs.readFileSync(explicitOut, 'utf8')); expect(parsed.actions.some((a) => a.positionals[0] === 'id="new"')).toBe(true); }); + +// --- ADR 0012 decision 6, R7 + commit semantics (Fix 2, C2): a repair-armed +// write COMMITS only when the transaction is COMPLETE (ARMED -> COMPLETE -> +// COMMITTED); an incomplete transaction ABORTS (publishes no prefix), and a +// committed one is an idempotent no-op. --- + +test('C2 abort-before-complete: a repair-armed but NOT-complete write discards — no file, no prefix', () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-script-writer-incomplete-')); + const writer = new SessionScriptWriter(path.join(root, 'sessions')); + const session = makeIosSession('default', { + recordSession: true, + saveScriptBoundary: 0, + // No saveScriptComplete: the plan never ran to its last executable step + // (a `close`/`close --save-script` reached after a divergence, a daemon + // teardown, or an idle-reap of an in-flight repair). + actions: [action({ command: 'click', positionals: ['id="save"'] })], + }); + + const result = writer.write(session); + expect(result).toEqual({ written: false }); + expect(fs.existsSync(path.join(root, 'sessions'))).toBe(false); + // Not committed — teardown will tombstone it (C5a). + expect(session.saveScriptCommitted).toBeFalsy(); +}); + +test('C2 commit-when-complete: a repair-armed COMPLETE write publishes and marks the session COMMITTED', () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-script-writer-complete-')); + const writer = new SessionScriptWriter(path.join(root, 'sessions')); + const outPath = path.join(root, 'flows', 'flow.healed.ad'); + fs.mkdirSync(path.dirname(outPath), { recursive: true }); + const session = makeIosSession('default', { + recordSession: true, + saveScriptBoundary: 0, + saveScriptComplete: true, + saveScriptPath: outPath, + actions: [action({ command: 'click', positionals: ['id="save"'] })], + }); + + const result = writer.write(session); + expect(result.written).toBe(true); + expect(fs.readFileSync(outPath, 'utf8')).toContain(HEAL_COMPLETE_SENTINEL); + expect(session.saveScriptCommitted).toBe(true); +}); + +test('C2 idempotent post-commit: a second write on a COMMITTED session no-ops (no re-publish, no error)', () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-script-writer-idempotent-')); + const writer = new SessionScriptWriter(path.join(root, 'sessions')); + const outPath = path.join(root, 'flows', 'flow.healed.ad'); + fs.mkdirSync(path.dirname(outPath), { recursive: true }); + const session = makeIosSession('default', { + recordSession: true, + saveScriptBoundary: 0, + saveScriptComplete: true, + saveScriptPath: outPath, + actions: [action({ command: 'click', positionals: ['id="save"'] })], + }); + + expect(writer.write(session).written).toBe(true); + const firstContent = fs.readFileSync(outPath, 'utf8'); + const firstMtime = fs.statSync(outPath).mtimeMs; + + // Mutate actions to prove a re-publish WOULD change the file if it happened. + session.actions.push(action({ command: 'click', positionals: ['id="other"'] })); + const second = writer.write(session); + expect(second).toEqual({ written: false }); + // The published artifact is untouched — the committed transaction never + // re-writes (no duplicate, no corruption). + expect(fs.readFileSync(outPath, 'utf8')).toBe(firstContent); + expect(fs.statSync(outPath).mtimeMs).toBe(firstMtime); +}); + +test('write() still emits an ordinary (non-repair) recording on close without --save-script, unaffected by the commit gate', () => { + const root = fs.mkdtempSync( + path.join(os.tmpdir(), 'agent-device-script-writer-ordinary-unfinalized-'), + ); + const writer = new SessionScriptWriter(path.join(root, 'sessions')); + const session = makeIosSession('default', { + recordSession: true, + // No saveScriptBoundary: an ordinary `open --save-script` recording, not + // a repair — the Fix 2 gate only applies to repair-armed sessions. + actions: [action({ command: 'click', positionals: ['id="save"'] })], + }); + + const { parsed } = writeAndParse(writer, session); + expect(parsed.actions.map((a) => a.command)).toEqual(['click']); +}); + +test('write() never appends the completeness sentinel to an ordinary (non-repair) recording', () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-script-writer-no-sentinel-')); + const writer = new SessionScriptWriter(path.join(root, 'sessions')); + const session = makeIosSession('default', { + recordSession: true, + actions: [action({ command: 'click', positionals: ['id="save"'] })], + }); + + const { script } = writeAndParse(writer, session); + expect(script).not.toContain(HEAL_COMPLETE_SENTINEL); +}); + +test('write() publishes atomically: no stray temp file survives a successful repair write', () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-script-writer-atomic-')); + const writer = new SessionScriptWriter(path.join(root, 'sessions')); + const outPath = path.join(root, 'flows', 'atomic.healed.ad'); + fs.mkdirSync(path.dirname(outPath), { recursive: true }); + const session = makeIosSession('default', { + recordSession: true, + saveScriptBoundary: 0, + saveScriptComplete: true, + saveScriptPath: outPath, + actions: [action({ command: 'click', positionals: ['id="save"'] })], + }); + + const result = writer.write(session); + expect(result.written).toBe(true); + // The only file left in the destination directory is the published script + // itself — the temp hard-link was cleaned up after the exclusive `linkSync` + // published the target, not left behind. + expect(fs.readdirSync(path.dirname(outPath))).toEqual([path.basename(outPath)]); + expect(fs.readFileSync(outPath, 'utf8')).toContain(HEAL_COMPLETE_SENTINEL); +}); diff --git a/src/daemon/__tests__/session-store.test.ts b/src/daemon/__tests__/session-store.test.ts index 149a30ff1..fff7f7ba5 100644 --- a/src/daemon/__tests__/session-store.test.ts +++ b/src/daemon/__tests__/session-store.test.ts @@ -7,6 +7,8 @@ import { SessionStore } from '../session-store.ts'; import type { SessionState } from '../types.ts'; import { buildRequestFinishedEvent } from '../session-event-log.ts'; import type { TargetAnnotationV1 } from '../../replay/target-identity.ts'; +import { HEAL_COMPLETE_SENTINEL } from '../session-script-writer.ts'; +import { parseReplayScriptDetailed } from '../../replay/script.ts'; type RecordActionEntry = Parameters[1]; @@ -656,3 +658,130 @@ test('writeSessionLog never fabricates a target-v1 annotation for actions record const script = writeScript(fixture); assert.equal(/agent-device:target-v1/.test(script), false); }); + +// --- ADR 0012 decision 6, R7 (C5a): repair tombstones turn a post-reap +// SESSION_NOT_FOUND into REPAIR_SESSION_EXPIRED with re-run guidance. --- + +test('writeRepairTombstone/readRepairTombstone round-trips owner + source path', () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-tombstone-')); + const store = new SessionStore(path.join(root, 'sessions')); + const session = makeSession('default'); + session.saveScriptBoundary = 0; + session.repairSourcePath = '/flows/login.ad'; + + store.writeRepairTombstone(session); + const tombstone = store.readRepairTombstone('default'); + assert.ok(tombstone); + assert.equal(tombstone?.owner, 'default'); + assert.equal(tombstone?.sourcePath, '/flows/login.ad'); + assert.ok((tombstone?.expiresAt ?? 0) > Date.now()); +}); + +test('readRepairTombstone returns undefined once the tombstone has expired', () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-tombstone-expiry-')); + const store = new SessionStore(path.join(root, 'sessions')); + const session = makeSession('default'); + // TTL 0 => expiresAt <= now => already stale. + store.writeRepairTombstone(session, 0); + assert.equal(store.readRepairTombstone('default'), undefined); +}); + +test('clearRepairTombstone removes a tombstone (a fresh replay --save-script clears the key)', () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-tombstone-clear-')); + const store = new SessionStore(path.join(root, 'sessions')); + const session = makeSession('default'); + store.writeRepairTombstone(session); + assert.ok(store.readRepairTombstone('default')); + + store.clearRepairTombstone('default'); + assert.equal(store.readRepairTombstone('default'), undefined); +}); + +// --- ADR 0012 decision 6 (BLOCKER 2): a COMPLETE transaction's commit can +// still FAIL at idle-reap/daemon-shutdown teardown (no-clobber refusal, a +// bare-@ref failure, or a filesystem error) — that failure must be preserved, +// not lost behind a generic "reaped before it was finalized" tombstone. --- + +test('BLOCKER 2: finalizeRepairTeardown of a COMPLETE transaction whose commit FAILS preserves the failure in a distinct tombstone, not a generic expiry', () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-teardown-commit-fail-')); + const store = new SessionStore(path.join(root, 'sessions')); + const healedPath = path.join(root, 'flow.healed.ad'); + // A prior COMPLETE (sentinel-marked) healed artifact already sits at the + // default sibling path — teardown's auto-commit attempt must refuse to + // clobber it, exactly like an explicit close's commit would. + fs.writeFileSync( + healedPath, + `context platform=ios device="x"\nclick id="old"\n${HEAL_COMPLETE_SENTINEL}\n`, + ); + const before = fs.readFileSync(healedPath, 'utf8'); + + const session = makeSession('default'); + session.recordSession = true; + session.saveScriptBoundary = 0; + session.saveScriptComplete = true; + session.saveScriptPath = healedPath; + session.saveScriptDefaultedHealedPath = true; + session.repairSourcePath = '/flows/login.ad'; + session.actions = [{ ts: 1, command: 'open', positionals: ['Demo'], flags: {} }]; + + // Idle-reap/shutdown teardown (never routes through close's handler). + store.finalizeRepairTeardown(session); + + // The prior complete artifact is untouched — teardown's failed commit + // never clobbers it. + assert.equal(fs.readFileSync(healedPath, 'utf8'), before); + // Never committed (the write failed), so the ordinary success bookkeeping + // never ran. + assert.notEqual(session.saveScriptCommitted, true); + + const tombstone = store.readRepairTombstone('default'); + assert.ok(tombstone, 'expected a tombstone to preserve the failed-commit outcome'); + // BLOCKER 2: distinguishable from a plain "reaped before it was finalized" + // tombstone — it carries the real commit failure. + assert.ok(tombstone?.commitFailure, 'expected the tombstone to carry the commit failure'); + assert.match(tombstone!.commitFailure!.message, /already exists/); + assert.equal(tombstone?.sourcePath, '/flows/login.ad'); +}); + +// --- ADR 0012 decision 6 (BLOCKER 3): idle-reap/shutdown auto-commit must +// record the same synthetic terminal `close` an explicit close records, so +// the committed healed .ad is self-contained (fresh-replayable) exactly like +// an explicit `close --save-script` commit. --- + +test('BLOCKER 3: finalizeRepairTeardown auto-commit records a terminal close, producing a self-contained, fresh-replayable healed .ad', () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-teardown-autocommit-close-')); + const store = new SessionStore(path.join(root, 'sessions')); + const healedPath = path.join(root, 'flow.healed.ad'); + + const session = makeSession('default'); + session.recordSession = true; + session.saveScriptBoundary = 0; + session.saveScriptComplete = true; + session.saveScriptPath = healedPath; + session.saveScriptDefaultedHealedPath = true; + session.actions = [ + { ts: 1, command: 'open', positionals: ['Demo'], flags: {} }, + { ts: 2, command: 'click', positionals: ['id="save-v2"'], flags: {} }, + ]; + + // The source plan's terminal `close` was already skipped-while-armed + // (Fix 3) — `session.actions` never gained one. Idle-reap/shutdown teardown + // must synthesize it itself before auto-committing. + store.finalizeRepairTeardown(session); + + assert.equal(session.saveScriptCommitted, true); + assert.equal(store.readRepairTombstone('default'), undefined); + const script = fs.readFileSync(healedPath, 'utf8'); + assert.ok(script.includes(HEAL_COMPLETE_SENTINEL)); + const parsed = parseReplayScriptDetailed(script); + // Self-contained: the auto-committed artifact ends with its OWN terminal + // close, exactly like an explicit close's commit — never a healed script + // that a fresh replay would run off the end of. + assert.deepEqual( + parsed.actions.map((a) => a.command), + ['open', 'click', 'close'], + ); + assert.deepEqual(parsed.actions[2]?.positionals, []); + const bareRefs = parsed.actions.flatMap((a) => a.positionals.filter((p) => p.startsWith('@'))); + assert.deepEqual(bareRefs, []); +}); diff --git a/src/daemon/client/daemon-client-lifecycle.ts b/src/daemon/client/daemon-client-lifecycle.ts index ced977199..101014f9a 100644 --- a/src/daemon/client/daemon-client-lifecycle.ts +++ b/src/daemon/client/daemon-client-lifecycle.ts @@ -2,11 +2,12 @@ import fs from 'node:fs'; import net from 'node:net'; import os from 'node:os'; import path from 'node:path'; -import { AppError } from '../../kernel/errors.ts'; -import type { DaemonRequest } from '../types.ts'; +import { AppError, normalizeError } from '../../kernel/errors.ts'; +import type { DaemonRequest, DaemonResponse } from '../types.ts'; import { runCmdDetachedMonitored, type ExecDetachedExit } from '../../utils/exec.ts'; import { findProjectRoot, readVersion } from '../../utils/version.ts'; import { emitDiagnostic } from '../../utils/diagnostics.ts'; +import { findUnrecoveredRepairCommitFailure } from '../session-store.ts'; import { resolveDaemonPaths, resolveDaemonServerMode, @@ -305,17 +306,45 @@ async function startLocalDaemon(settings: DaemonClientSettings): Promise, daemon: EnsuredDaemon, settings: DaemonClientSettings, -): Promise { + response: DaemonResponse | undefined, +): Promise { if ( !isOneShotReplayCommand(req.command) || (!daemon.startedByClient && !settings.ownedStateDir) || - isRemoteDaemon(daemon.info) + isRemoteDaemon(daemon.info) || + // ADR 0012 decision 6, R7 (Fix 1, C1): a repair-armed `--save-script` + // replay that comes back as a HELD divergence must keep its owning daemon + // (and the session on it) addressable for the agent's corrective press + + // `replay --from`/`close` — tearing it down here is what turns a + // recoverable divergence into a later bare SESSION_NOT_FOUND. The daemon + // then bounds the held session's own lifetime via idle-reap (writing a + // `REPAIR_SESSION_EXPIRED` tombstone on reap), so an abandoned repair still + // cannot leak indefinitely; this only stops the ONE-SHOT-COMMAND teardown + // below from racing ahead of that window. + isHeldRepairDivergence(response) ) { - return; + return response; } const result = { @@ -325,6 +354,7 @@ export async function cleanupDaemonAfterRequest( removedStateDir: false, error: undefined as string | undefined, }; + let surfacedResponse = response; try { await stopDaemonProcessForTakeover(daemon.info); @@ -340,8 +370,17 @@ export async function cleanupDaemonAfterRequest( result.removedLock = lockExists && !fs.existsSync(settings.paths.lockPath); if (settings.ownedStateDir) { - fs.rmSync(settings.paths.baseDir, { recursive: true, force: true }); - result.removedStateDir = !fs.existsSync(settings.paths.baseDir); + // `stopDaemonProcessForTakeover` above waits for the (real) daemon + // process to actually exit, which only happens AFTER its shutdown + // handler finishes `finalizeRepairTeardown` for every session — so by + // now any commit-failure tombstone it would leave is already on disk. + const unrecovered = findUnrecoveredRepairCommitFailure(settings.paths.sessionsDir); + if (unrecovered) { + surfacedResponse = surfaceUnrecoveredRepairCommitFailure(response, unrecovered); + } else { + fs.rmSync(settings.paths.baseDir, { recursive: true, force: true }); + result.removedStateDir = !fs.existsSync(settings.paths.baseDir); + } } } @@ -350,6 +389,87 @@ export async function cleanupDaemonAfterRequest( phase: 'daemon_replay_cleanup', data: result, }); + return surfacedResponse; +} + +/** + * ADR 0012 decision 6 (BLOCKER 2, third follow-up): converts an unrecovered + * shutdown-time commit failure into the response the CALLER actually sees. + * The original response may have been a genuine SUCCESS — the replay/plan + * itself completed with no divergence, only the deferred healed-script + * publish failed afterward at teardown — so there is no existing error to + * attach a hint to (unlike `attachRepairSessionAddressHint`, which only ever + * runs on an already-`ok:false` divergence): this REPLACES the response with + * the same `REPAIR_COMMIT_FAILED` error the daemon's own + * `repairExpiredIfTombstoned` (request-router.ts) would surface to a + * follow-up request on this session — a one-shot command has no follow-up + * request to receive it, so the client raises it here instead. An existing + * `ok:false` response (e.g. the platform close itself failed for a different, + * more specific reason) is returned unchanged. + */ +function surfaceUnrecoveredRepairCommitFailure( + response: DaemonResponse | undefined, + unrecovered: NonNullable>, +): DaemonResponse { + if (response && !response.ok) return response; + const { sessionName, tombstone } = unrecovered; + const reRun = tombstone.sourcePath + ? `re-run: replay ${tombstone.sourcePath} --save-script` + : 're-run your replay