fix(daemon): keep close-time script-save failures from leaking the session/device claim (#1392)

* fix(daemon): keep close-time script-save failures from leaking the session/device claim

A close-time script write (implicit from `open --save-script`, or this
close's own `--save-script`) that refuses to publish (e.g. a no-clobber
target-exists AppError) threw uncaught out of `handleCloseCommand`,
skipping lease release, device-claim release, and `sessionStore.delete`
entirely — while the `close` action had already been recorded with no
rollback.

Live-repro'd over the real CLI against an Android emulator: this single
gap explained both symptoms split out of #1384 into #1391 — a lingering
`DEVICE_IN_USE` claim after a failed `close`, and a published `.ad`
rewritten with duplicated trailing `close` lines when the same close was
retried (each attempt re-recorded a `close` action on top of the one
never rolled back from the prior failure).

Catch the write failure, roll back the just-recorded `close` action
(mirroring the existing repair-armed commit-failure pattern), and let
teardown (lease release, device-claim clear, session delete) complete
regardless — exactly as an ordinary platform-close failure already
doesn't block them. The failure is still surfaced to the caller, but
after teardown, with a corrected hint: retrying the same close is no
longer meaningful since the session is now gone.

Fixes #1391

* refactor(daemon): shrink handleCloseCommand/runSessionCloseTeardown under fallow's complexity gate

CI's fallow code-quality check flagged handleCloseCommand (126 lines,
19 cyclomatic / 16 cognitive) and runSessionCloseTeardown (73 lines) as
exceeding the large-function/high-complexity thresholds after the
prior commit's fix.

Extract runCloseTeardownAndRelease (teardown + lease release + claim
clear + delete + ordered error surfacing) and buildCloseSuccessResponse
(final response shaping) out of handleCloseCommand, and
finalizeOrdinaryCloseScript out of runSessionCloseTeardown. No behavior
change — same control flow, split into named, independently-readable
steps; fallow now reports 0 complexity findings for this diff.

* fix(daemon): preserve the write error's structured details in the close-time save failure

Review feedback on #1392 (thymikee): toOrdinaryCloseSaveScriptFailure
rebuilt the AppError from only the original message, dropping its
machine-readable details.reason ("script_target_exists"), details.path,
and cause. A caller dispatching on those fields (or reading the CLI's
--json error.details) lost them even though the underlying write
failure carried them.

Preserve the original error's details/cause, overriding only the
close-specific hint and retriable:false. Extends the #1391 regression
test to assert the routed close response still carries reason/path.

* refactor(daemon): drop the vestigial close-time rollback, add router-level #1391 coverage

Review feedback on #1392 (thymikee), P2 items:

- The close-time save-script failure's session.actions rollback
  (finalizeOrdinaryCloseScript) was left over from an earlier design
  where a failed save could keep the session alive for retry. It
  never does now — runCloseTeardownAndRelease always tears the
  session down regardless of the outcome — so there is no surviving
  session for a later write to duplicate the close action on. Drop
  the rollback; the durable events.ndjson entry (which the rollback
  never touched anyway) and the in-memory action now agree, both
  accurately recording that the close happened.
- Add a request-router-level regression (request-router-typed-error.test.ts,
  alongside the existing repair-close BLOCKER 2 test it mirrors) proving
  the normalized JSON error shape a real client sees: top-level
  retriable:false, details.reason/path preserved, and the session torn
  down — not just that handleCloseCommand throws the right AppError
  when called directly.

* test(daemon): assert the durable close event survives a failed close-time save

Review feedback on #1392 (thymikee), final P2 item: the previous commit
removed the actions rollback because there's no surviving session to
duplicate the close action on, but nothing actually asserted the
durable events.ndjson action.recorded:close event stays put. Flush and
read it back so a future rollback or event-order change can't silently
recreate the in-memory/durable mismatch the removed rollback used to
paper over asymmetrically.

* test(daemon): assert the retained session's in-memory close action, not just the durable event

Review feedback on #1392 (thymikee): the durable-event assertion alone
doesn't catch a reintroduced session.actions.length = actionsBeforeClose
rollback, because that event is queued (and durable) before the write
even attempts — a regression there would leave the assertion passing
while silently reintroducing the in-memory/durable mismatch.

Retain the session object past handleCloseCommand (store.delete only
drops the map entry, not the object a local variable still points at)
and assert its actions array contains exactly one close entry, matching
the durable event count. Verified by temporarily reintroducing the old
rollback locally: this assertion fails (0 !== 1) where the prior
durable-only check did not, then reverted.

* refactor(daemon): model repair close retry as receipt

* refactor(daemon): merge blockingError to state, not explain, the save-script exclusion

Following up on the comment-trimming pass already on this branch: the
device-claim condition (!platformCloseError && !cleanupAggregate) and
the two-line throw sequence right below it both needed a paragraph
explaining why saveScriptError is excluded from one but not the other.

Merge platformCloseError and cleanupAggregate into a single named
blockingError — its name now states the exclusion the comment used to
argue for, and the throw sequence collapses from two ifs to one.
Trimmed the remaining long docblocks in this file the same way: state
what's non-obvious in 1-3 lines instead of re-deriving it in prose.

* refactor(daemon): clarify close script finalization

scripts/help-conformance-cases.mjs

@@ -155,6 +155,41 @@ export const CASES = [
       { id: 'noRawAdbKeyevent', pattern: /\badb\s+shell\s+input\b/i },
     ],
   },
+  {
+    id: 'ios-system-ui-widget-flow',
+    docs: ['--help:first30', 'ios-system-ui'],
+    task: 'On an iOS simulator, add the Calendar widget from SpringBoard, capture visual evidence of the placed widget, return to the installed app com.example.calendar, and close. Plan commands only.',
+    expectations: ['validPlanCommands', 'fullPrefix', 'usesSnapshotI', 'opensAndCloses'],
+    matchers: [
+      {
+        id: 'bindsSessionToSpringBoard',
+        pattern: /\bagent-device\s+open\s+com\.apple\.springboard\b[^\n]*--platform\s+ios\b/i,
+      },
+      {
+        id: 'entersEditModeWithCoordinateLongpress',
+        pattern: /\bagent-device\s+longpress\s+-?\d+(?:\.\d+)?\s+-?\d+(?:\.\d+)?\b/i,
+      },
+      {
+        id: 'refreshesSnapshotAfterEnteringEditMode',
+        pattern: /\blongpress\b[\s\S]*\n[^\n]*\bsnapshot\s+-i\b/i,
+      },
+      {
+        id: 'usesScreenshotForSparseGalleryResult',
+        pattern:
+          /\bagent-device\s+screenshot\b[\s\S]*\n[^\n]*\bagent-device\s+press\s+-?\d+(?:\.\d+)?\s+-?\d+(?:\.\d+)?\b/i,
+      },
+      {
+        id: 'returnsToAppUnderTest',
+        pattern: /\bagent-device\s+open\s+com\.example\.calendar\b[^\n]*--platform\s+ios\b/i,
+      },
+    ],
+    forbidden: [
+      {
+        id: 'noRawSimulatorControl',
+        pattern: /(?:^|\n)(?:xcrun\s+simctl|idb|maestro)\b/i,
+      },
+    ],
+  },
   {
     id: 'web-managed-backend-loop',
     docs: ['--help:first30', 'web'],

scripts/layering/session-state.ts

@@ -57,7 +57,7 @@ export const SESSION_STATE_FIELD_OWNERS: Readonly<Record<string, readonly string
     'src/daemon/handlers/session-script-publication.ts',
   ],
   recordSession: [
-    'src/daemon/handlers/session-close.ts',
+    'src/daemon/handlers/session-close-script.ts',
     'src/daemon/handlers/session-open.ts',
     'src/daemon/handlers/session-replay-runtime.ts',
     'src/daemon/handlers/session-script-publication.ts',
@@ -81,8 +81,7 @@ export const SESSION_STATE_FIELD_OWNERS: Readonly<Record<string, readonly string
   saveScriptCommitted: ['src/daemon/session-script-writer.ts'],
   repairSourcePath: ['src/daemon/handlers/session-replay-runtime.ts'],
   pendingRecordAndHeal: ['src/daemon/handlers/session-replay-resume.ts'],
-  repairPlatformCloseSucceeded: ['src/daemon/handlers/session-close.ts'],
-  repairPlatformCloseIdentity: ['src/daemon/handlers/session-close.ts'],
+  repairPlatformCloseReceipt: ['src/daemon/handlers/session-close.ts'],
 
   trace: ['src/daemon/handlers/record-trace.ts'],
   recording: ['src/daemon/handlers/record-trace-recording.ts'],

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

@@ -1,4 +1,5 @@
 import { test, expect, vi, beforeEach } from 'vitest';
+import fs from 'node:fs';
 import os from 'node:os';
 import path from 'node:path';
 
@@ -167,3 +168,40 @@ test('BLOCKER 2 (second follow-up): a repair-close platform-close failure surfac
   // The session is retained (not torn down), addressable for the retry.
   expect(sessionStore.get('typed-error')).toBeDefined();
 });
+
+// Unlike the handler-level test in session-device-claims.test.ts, this goes
+// through the real router boundary, so it also covers normalizeError's
+// details.retriable hoisting.
+test('#1391: an ordinary close-time script-save failure surfaces details.reason/path and retriable:false through the router, and the session is torn down', async () => {
+  const { sessionStore, handler } = makeHandler();
+  const session = makeIosSession('typed-error');
+  session.recordSession = true;
+  const targetPath = path.join(
+    os.tmpdir(),
+    `agent-device-router-typed-error-${Date.now()}-${Math.random().toString(36).slice(2)}.ad`,
+  );
+  fs.writeFileSync(targetPath, 'pre-existing\n');
+  session.saveScriptPath = targetPath;
+  sessionStore.set('typed-error', session);
+
+  try {
+    // Untargeted close: no positionals, so no platform close is dispatched
+    // (`shouldDispatchPlatformClose`) — isolates the script-save failure from
+    // any platform-close error, matching BLOCKER 2's own targeted-vs-untargeted
+    // distinction above.
+    const response = await handler(request('close'));
+
+    expect(response.ok).toBe(false);
+    if (response.ok) return;
+    expect(response.error.code).toBe('COMMAND_FAILED');
+    expect(response.error.retriable).toBe(false);
+    expect(response.error.details?.reason).toBe('script_target_exists');
+    expect(response.error.details?.path).toBe(targetPath);
+    // Unlike the repair-armed case above, an ordinary session's teardown
+    // never withholds on a failed script save — it is always torn down.
+    expect(sessionStore.get('typed-error')).toBeUndefined();
+    expect(mockDispatch).not.toHaveBeenCalled();
+  } finally {
+    fs.rmSync(targetPath, { force: true });
+  }
+});

src/daemon/handlers/__tests__/session-close-script.test.ts

@@ -0,0 +1,105 @@
+import fs from 'node:fs';
+import os from 'node:os';
+import path from 'node:path';
+import { afterEach, expect, test, vi } from 'vitest';
+import { AppError } from '../../../kernel/errors.ts';
+import { makeIosSession } from '../../../__tests__/test-utils/session-factories.ts';
+import { SessionStore } from '../../session-store.ts';
+import type { DaemonRequest } from '../../types.ts';
+import {
+  buildRetriableRepairCloseFailureResponse,
+  commitRepairScriptBeforeClose,
+  finalizeOrdinaryCloseScript,
+} from '../session-close-script.ts';
+
+const roots: string[] = [];
+
+afterEach(() => {
+  vi.restoreAllMocks();
+  for (const root of roots.splice(0)) fs.rmSync(root, { recursive: true, force: true });
+});
+
+function setup(name: string) {
+  const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-session-close-script-'));
+  roots.push(root);
+  const sessionStore = new SessionStore(path.join(root, 'sessions'));
+  const session = makeIosSession(name, { appBundleId: 'com.example.app' });
+  sessionStore.set(name, session);
+  const req: DaemonRequest = {
+    token: 'token',
+    session: name,
+    command: 'close',
+    positionals: [],
+    flags: {},
+  };
+  return { req, session, sessionStore };
+}
+
+test('failed repair publication removes only its synthetic close before retry', () => {
+  const { req, session, sessionStore } = setup('repair');
+  session.saveScriptBoundary = 0;
+  session.saveScriptComplete = true;
+  session.recordSession = true;
+  const failure = new AppError('COMMAND_FAILED', 'publish failed');
+  vi.spyOn(sessionStore, 'writeSessionLog').mockReturnValue({ written: false, error: failure });
+
+  expect(commitRepairScriptBeforeClose(sessionStore, session, req)).toEqual({
+    kind: 'failed',
+    error: failure,
+  });
+  expect(session.actions).toEqual([]);
+});
+
+test('repair close failure keeps normalized metadata and is explicitly retriable', () => {
+  const { session } = setup('repair-error');
+  session.saveScriptPath = '/tmp/repaired.ad';
+  const failure = new AppError('COMMAND_FAILED', 'publish failed', {
+    reason: 'target-exists',
+    hint: 'Choose another path.',
+    logPath: '/tmp/daemon.log',
+  });
+
+  expect(buildRetriableRepairCloseFailureResponse(session, failure)).toEqual({
+    ok: false,
+    error: {
+      code: 'COMMAND_FAILED',
+      message: 'publish failed',
+      hint: 'Choose another path.',
+      logPath: '/tmp/daemon.log',
+      details: {
+        reason: 'target-exists',
+        session: 'repair-error',
+        savedScript: '/tmp/repaired.ad',
+      },
+      retriable: true,
+    },
+  });
+});
+
+test('ordinary publication failure retains its close action after making the error non-retriable', () => {
+  const { req, session, sessionStore } = setup('ordinary');
+  const failure = new AppError('COMMAND_FAILED', 'target exists', {
+    reason: 'target-exists',
+    hint: 'Retry close.',
+  });
+  vi.spyOn(sessionStore, 'writeSessionLog').mockImplementation(() => {
+    throw failure;
+  });
+
+  const result = finalizeOrdinaryCloseScript({
+    req: { ...req, flags: { saveScript: true } },
+    session,
+    sessionStore,
+    platformCloseError: undefined,
+  });
+
+  expect(session.actions.map(({ command }) => command)).toEqual(['close']);
+  expect(result).toMatchObject({
+    code: 'COMMAND_FAILED',
+    message: 'The session was closed, but its script was not saved: target exists',
+    details: {
+      reason: 'target-exists',
+      retriable: false,
+    },
+  });
+});

src/daemon/handlers/__tests__/session-device-claims.test.ts

@@ -35,6 +35,8 @@ import { SessionStore } from '../../session-store.ts';
 import { handleCloseCommand } from '../session-close.ts';
 import { handleOpenCommand } from '../session-open.ts';
 import type { DeviceInfo } from '../../../kernel/device.ts';
+import type { SessionState } from '../../types.ts';
+import { AppError } from '../../../kernel/errors.ts';
 
 const mockDispatch = vi.mocked(dispatchCommand);
 const mockResolveTargetDevice = vi.mocked(resolveTargetDevice);
@@ -245,3 +247,80 @@ test('local close clears its matching advisory claim after teardown', async () =
   assert.equal(response.ok, true);
   assert.deepEqual(inspectDeviceClaims({ serial: android.id }), []);
 });
+
+test('#1391: a close-time script save failure still clears the advisory claim and deletes the session', async () => {
+  const { store, stateDir } = setup();
+  const acquired = await acquireAdvisoryDeviceClaim({
+    device: android,
+    session: 'close-save-script-failure',
+    workspace: process.cwd(),
+    stateDir,
+  });
+  assert.ok(acquired.ownership);
+  const targetPath = path.join(stateDir, 'already-published.ad');
+  fs.writeFileSync(targetPath, 'pre-existing\n');
+  // Retained directly (not just looked up via `store`) so it stays inspectable
+  // after `handleCloseCommand` deletes it from the store — `store.delete` only
+  // drops the map entry, it doesn't touch the object this variable still
+  // points at.
+  const session: SessionState = {
+    name: 'close-save-script-failure',
+    device: android,
+    deviceClaim: acquired.ownership,
+    createdAt: Date.now(),
+    actions: [],
+    recordSession: true,
+    saveScriptPath: targetPath,
+  };
+  store.set('close-save-script-failure', session);
+  mockDispatch.mockResolvedValue({});
+
+  // Like the platform-close-error tests above, a failed close-time save is
+  // thrown (not returned as `{ok:false}`) — `handleCloseCommand` never
+  // converts its own errors; the request-router does that for real requests.
+  let thrown: unknown;
+  try {
+    await handleCloseCommand({
+      req: {
+        command: 'close',
+        token: 'test',
+        session: 'close-save-script-failure',
+        positionals: [],
+        flags: {},
+      },
+      sessionName: 'close-save-script-failure',
+      logPath: path.join(stateDir, 'daemon.log'),
+      sessionStore: store,
+      leaseRegistry: new LeaseRegistry(),
+    });
+  } catch (error) {
+    thrown = error;
+  }
+
+  // The failed save is surfaced, but never at the cost of leaking the session
+  // or the device claim (#1391 item 1) — and the target is left untouched
+  // rather than rewritten (#1391 item 2).
+  assert.ok(thrown instanceof AppError, 'expected close to throw an AppError');
+  const error = thrown as AppError;
+  assert.equal(error.code, 'COMMAND_FAILED');
+  assert.match(error.message, /session was closed, but its script was not saved/);
+  assert.equal(error.details?.retriable, false);
+  // The original write failure's machine-readable details survive the
+  // close-specific hint/retriable override, so a caller dispatching on them
+  // (e.g. `reason === 'script_target_exists'`) still can.
+  assert.equal(error.details?.reason, 'script_target_exists');
+  assert.equal(error.details?.path, targetPath);
+  assert.equal(store.get('close-save-script-failure'), undefined);
+  assert.deepEqual(inspectDeviceClaims({ serial: android.id }), []);
+  assert.equal(fs.readFileSync(targetPath, 'utf8'), 'pre-existing\n');
+
+  assert.deepEqual(
+    session.actions.map((action) => action.command),
+    ['close'],
+  );
+  await store.flushEvents('close-save-script-failure');
+  const durableCloseEvents = store
+    .readEvents('close-save-script-failure')
+    .events.filter((event) => event.kind === 'action.recorded' && event.command === 'close');
+  assert.equal(durableCloseEvents.length, session.actions.length);
+});

src/daemon/handlers/__tests__/session-replay-repair-transaction.test.ts

@@ -984,7 +984,7 @@ test('BLOCKER 3 (second follow-up): a retry after a SUCCESSFUL platform close bu
   if (!closeResponse.ok) expect(closeResponse.error.message).toMatch(/already exists/);
   expect(sessionStore.get(sessionName)).toBeDefined();
   expect(fs.readFileSync(healedPath, 'utf8')).toBe(before);
-  expect(sessionStore.get(sessionName)!.repairPlatformCloseSucceeded).toBe(true);
+  expect(sessionStore.get(sessionName)!.repairPlatformCloseReceipt).toBeDefined();
 
   // Retry with an explicit path: the ALREADY-SUCCEEDED platform close must
   // NEVER be dispatched again — a non-idempotent backend could fail (or
@@ -1043,14 +1043,6 @@ test('BLOCKER 3: a competing second writer never overwrites a COMPLETE artifact
   expect(fs.readFileSync(healedPath, 'utf8')).toBe(committed);
 });
 
-// --- ADR 0012 decision 6 (BLOCKER 3, third follow-up): `repairPlatformCloseSucceeded`
-// was session-wide, not bound to WHICH close request actually succeeded. An
-// untargeted close performs NO platform operation (`shouldDispatchPlatformClose`
-// is false with no positional target), yet the prior implementation still set
-// the marker as though a real close succeeded; a retry with a DIFFERENT
-// identity (a target added, or a different target) then wrongly skipped the
-// platform close entirely, committing as though it had run. ---
-
 test('BLOCKER 3 (third follow-up): an untargeted close that performed NO platform operation never lets a targeted retry skip the platform close', async () => {
   const { root, sessionStore, sessionName, logPath, leaseRegistry } = setup(
     'agent-device-repair-transaction-close-identity-untargeted-',
@@ -1078,9 +1070,6 @@ test('BLOCKER 3 (third follow-up): an untargeted close that performed NO platfor
   expect(mockDispatchCommand).not.toHaveBeenCalled();
   expect(sessionStore.get(sessionName)).toBeDefined();
 
-  // Retry WITH a target: the prior session-wide `repairPlatformCloseSucceeded`
-  // flag (set true even though nothing dispatched for the untargeted attempt)
-  // would wrongly skip the platform close here. It must actually run.
   const retry = await handleCloseCommand({
     req: {
       token: 't',
@@ -1126,7 +1115,7 @@ test('BLOCKER 3 (third follow-up): a retry targeting a DIFFERENT app than the su
   });
   expect(first.ok).toBe(false);
   expect(mockDispatchCommand).toHaveBeenCalledTimes(1);
-  expect(sessionStore.get(sessionName)!.repairPlatformCloseSucceeded).toBe(true);
+  expect(sessionStore.get(sessionName)!.repairPlatformCloseReceipt).toBeDefined();
 
   // Retry targets a DIFFERENT app (app-b) — a genuinely different platform
   // operation. The prior session-wide marker would wrongly treat app-b as

src/daemon/handlers/session-close-script.ts

@@ -0,0 +1,103 @@
+import { AppError, normalizeError } from '../../kernel/errors.ts';
+import { successText } from '../../utils/success-text.ts';
+import type { SessionStore } from '../session-store.ts';
+import type { DaemonRequest, DaemonResponse, SessionState } from '../types.ts';
+import { recordSessionAction } from './handler-utils.ts';
+
+export type RepairCloseCommit =
+  | { kind: 'not-armed' }
+  | { kind: 'committed'; path?: string }
+  | { kind: 'aborted' }
+  | { kind: 'failed'; error: AppError };
+
+function shouldOverwriteSavedScript(req: DaemonRequest, session: SessionState): boolean {
+  return Boolean(req.flags?.force || session.saveScriptForce);
+}
+
+export function commitRepairScriptBeforeClose(
+  sessionStore: SessionStore,
+  session: SessionState,
+  req: DaemonRequest,
+): RepairCloseCommit {
+  if (session.saveScriptBoundary === undefined) return { kind: 'not-armed' };
+
+  const actionsBeforeClose = session.actions.length;
+  recordSessionAction(sessionStore, session, req, 'close', {
+    session: session.name,
+    ...successText(`Closed: ${session.name}`),
+  });
+  const result = sessionStore.writeSessionLog(session, {
+    force: shouldOverwriteSavedScript(req, session),
+  });
+  if (result.written) return { kind: 'committed', path: result.path };
+  if (result.error) {
+    // The retained repair session can retry without accumulating synthetic closes.
+    session.actions.length = actionsBeforeClose;
+    return { kind: 'failed', error: result.error };
+  }
+  return session.saveScriptComplete ? { kind: 'committed' } : { kind: 'aborted' };
+}
+
+export function buildRetriableRepairCloseFailureResponse(
+  session: SessionState,
+  error: AppError,
+): DaemonResponse {
+  const normalized = normalizeError(error);
+  return {
+    ok: false,
+    error: {
+      ...normalized,
+      details: {
+        ...normalized.details,
+        session: session.name,
+        ...(session.saveScriptPath ? { savedScript: session.saveScriptPath } : {}),
+      },
+      retriable: true,
+    },
+  };
+}
+
+export function finalizeOrdinaryCloseScript(params: {
+  req: DaemonRequest;
+  session: SessionState;
+  sessionStore: SessionStore;
+  platformCloseError: unknown;
+}): AppError | undefined {
+  const { req, session, sessionStore, platformCloseError } = params;
+  if (!platformCloseError) {
+    recordSessionAction(sessionStore, session, req, 'close', {
+      session: session.name,
+      ...successText(`Closed: ${session.name}`),
+    });
+  }
+  if (req.flags?.saveScript) session.recordSession = true;
+
+  try {
+    sessionStore.writeSessionLog(session, {
+      force: shouldOverwriteSavedScript(req, session),
+    });
+    return undefined;
+  } catch (error) {
+    return toOrdinaryCloseSaveScriptFailure(error);
+  }
+}
+
+function toOrdinaryCloseSaveScriptFailure(error: unknown): AppError {
+  const overrides = {
+    hint: 'Remove the existing target (or pass --force/--overwrite), then re-record with open --save-script.',
+    retriable: false,
+  };
+  if (error instanceof AppError) {
+    return new AppError(
+      'COMMAND_FAILED',
+      `The session was closed, but its script was not saved: ${error.message}`,
+      { ...error.details, ...overrides },
+      error.cause,
+    );
+  }
+  return new AppError(
+    'COMMAND_FAILED',
+    `The session was closed, but its script was not saved: ${normalizeError(error).message}`,
+    overrides,
+  );
+}

src/daemon/handlers/session-close.ts

@@ -23,7 +23,6 @@ import {
 } from './session-device-utils.ts';
 import { errorResponse } from './response.ts';
 import { expireRefFrame } from '../ref-frame.ts';
-import { recordSessionAction } from './handler-utils.ts';
 import { stopSessionRecordingForTeardown } from './record-trace-recording.ts';
 import type { LeaseRegistry } from '../lease-registry.ts';
 import { releaseSessionLease } from '../lease-lifecycle.ts';
@@ -40,6 +39,11 @@ import {
   type SessionCleanupFailure,
 } from '../session-teardown.ts';
 import { clearAdvisoryDeviceClaim } from '../device-claims.ts';
+import {
+  buildRetriableRepairCloseFailureResponse,
+  commitRepairScriptBeforeClose,
+  finalizeOrdinaryCloseScript,
+} from './session-close-script.ts';
 
 async function maybeShutdownSessionTarget(params: {
   device: DeviceInfo;
@@ -52,17 +56,6 @@ async function maybeShutdownSessionTarget(params: {
   return await shutdownDeviceTarget(device);
 }
 
-/**
- * #1258: the effective `--force`/`--overwrite` decision for a `close`-time
- * publish — this close request's own flag OR'd with whatever was persisted
- * on the session at an earlier arm (`open --save-script --force`, or a
- * repair's first `replay --save-script --force`). Either source authorizes
- * overwriting; neither being set keeps the default refuse-on-exist.
- */
-function resolveEffectiveSaveScriptForce(req: DaemonRequest, session: SessionState): boolean {
-  return Boolean(req.flags?.force || session.saveScriptForce);
-}
-
 function shouldRetainAppleRunnerAfterClose(req: DaemonRequest, session: SessionState): boolean {
   return (
     isIosSimulator(session.device) &&
@@ -77,96 +70,6 @@ function shouldStopAppleRunnerBeforeTargetedClose(session: SessionState): boolea
   return isApplePlatform(session.device.platform) && !isIosSimulator(session.device);
 }
 
-/**
- * ADR 0012 decision 6 (BLOCKER 2): outcome of committing a repair transaction
- * at `close` time, BEFORE any destructive teardown. `not-armed` = not a repair
- * session (normal close flow); `committed` = the healed `.ad` was written
- * (`path`) or the transaction was incomplete and intentionally discarded (no
- * `path`) — either way close proceeds and tears the session down; `failed` = a
- * COMPLETE transaction's commit failed (no-clobber / bare-`@ref` / fs error),
- * so the session must be KEPT for retry and the failure surfaced.
- */
-type RepairCloseOutcome =
-  | { kind: 'not-armed' }
-  | { kind: 'committed'; path?: string }
-  | { kind: 'aborted' }
-  | { kind: 'failed'; error: AppError };
-
-function commitRepairBeforeClose(
-  sessionStore: SessionStore,
-  session: SessionState,
-  req: DaemonRequest,
-): RepairCloseOutcome {
-  if (session.saveScriptBoundary === undefined) return { kind: 'not-armed' };
-  // Record the finalize `close` (so the committed healed slice ends with it),
-  // then COMMIT before any destructive teardown. A repair-armed session commits
-  // iff the transaction COMPLETED, regardless of `--save-script` on the close
-  // (C2); `recordSession` is already true from arming.
-  const actionsBeforeClose = session.actions.length;
-  recordSessionAction(sessionStore, session, req, 'close', {
-    session: session.name,
-    ...successText(`Closed: ${session.name}`),
-  });
-  const result = sessionStore.writeSessionLog(session, {
-    force: resolveEffectiveSaveScriptForce(req, session),
-  });
-  if (result.written) return { kind: 'committed', path: result.path };
-  if (result.error) {
-    // The session is kept for retry (BLOCKER 2b): roll back the just-recorded
-    // finalize `close` so a subsequent `close --save-script=<other>` retry does
-    // not accumulate duplicate `close` lines in the healed slice.
-    session.actions.length = actionsBeforeClose;
-    return { kind: 'failed', error: result.error };
-  }
-  if (!session.saveScriptComplete) {
-    return { kind: 'aborted' };
-  }
-  return { kind: 'committed' };
-}
-
-/**
- * ADR 0012 decision 6 (BLOCKER 2b): a commit-failure close response. The session
- * is intentionally NOT torn down (the caller returns before teardown), so the
- * agent can fix the cause and retry `close --save-script`.
- *
- * BLOCKER 2 (second follow-up): routes `error` through the SAME
- * `normalizeError` normalization every other AppError -> DaemonResponse
- * conversion in this codebase uses (see `repairExpiredIfTombstoned` in
- * request-router.ts and the dozens of handler call sites doing
- * `{ ok: false, error: normalizeError(error) }`) — a hand-rolled reshape here
- * previously dropped the underlying platform/commit error's `details`,
- * `diagnosticId`, and `logPath` entirely, and put `retriable` under
- * `error.details.retriable`, a location neither the router's `enrichDaemonError`
- * nor the client reads (both read the TOP-LEVEL `error.retriable` — see
- * `DaemonError` in kernel/contracts.ts). `retriable: true` is still forced
- * unconditionally at the end: the session was preserved specifically so the
- * agent can retry (`close`/`close --save-script=<other>`), which must never
- * be contradicted by the underlying error's own (usually absent) classification.
- */
-function buildRepairCloseFailureResponse(session: SessionState, error: AppError): DaemonResponse {
-  const normalized = normalizeError(error);
-  return {
-    ok: false,
-    error: {
-      ...normalized,
-      details: {
-        ...normalized.details,
-        session: session.name,
-        ...(session.saveScriptPath ? { savedScript: session.saveScriptPath } : {}),
-      },
-      retriable: true,
-    },
-  };
-}
-
-/**
- * ADR 0012 decision 6 (BLOCKER 2, new): normalizes a repair-armed session's
- * FAILED platform close into a distinct, surfaceable AppError, mirroring
- * `toRepairCommitFailure` in `session-script-writer.ts`. An AppError from the
- * platform close (e.g. a device-unavailable failure) already carries its own
- * code/details/hint and passes through unchanged; anything else is wrapped
- * with a clear message so the agent can tell this apart from a write failure.
- */
 function toRepairPlatformCloseFailure(error: unknown): AppError {
   if (error instanceof AppError) return error;
   const detail = error instanceof Error ? error.message : String(error);
@@ -175,10 +78,11 @@ function toRepairPlatformCloseFailure(error: unknown): AppError {
   });
 }
 
-// Runs the failure-isolated resource teardown and the targeted platform close
-// (#1225). Returns the preserved platform-close error (if any); best-effort
-// cleanup failures are pushed into `cleanupFailures`. Never throws for a cleanup
-// step so the caller can make an explicit decision about lease/session commit.
+type SessionCloseTeardownResult = {
+  platformCloseError: unknown;
+  saveScriptError?: AppError;
+};
+
 async function runSessionCloseTeardown(params: {
   req: DaemonRequest;
   session: SessionState;
@@ -187,12 +91,7 @@ async function runSessionCloseTeardown(params: {
   sessionStore: SessionStore;
   cleanupFailures: SessionCleanupFailure[];
   repairArmed: boolean;
-  // ADR 0012 decision 6 (BLOCKER 2): a repair-armed session already dispatched
-  // (and confirmed the success of) its platform close BEFORE this teardown —
-  // see `handleCloseCommand`. Dispatching it again here would be redundant at
-  // best and a double-close at worst, so it is skipped for that case only.
-  skipPlatformClose: boolean;
-}): Promise<unknown> {
+}): Promise<SessionCloseTeardownResult> {
   const { req, session, sessionName, logPath, sessionStore, cleanupFailures, repairArmed } = params;
   const attemptCleanup = async (step: string, run: () => Promise<void>): Promise<void> => {
     try {
@@ -201,43 +100,20 @@ async function runSessionCloseTeardown(params: {
       cleanupFailures.push({ step, error });
     }
   };
-  // Decide runner retention from the PRE-teardown state: an active recording
-  // defeats retention, and `stopBestEffortSessionResources` below finalizes (and
-  // clears) that recording, so the decision must be captured before it runs.
   const retainAppleRunner = shouldRetainAppleRunnerAfterClose(req, session);
   await stopBestEffortSessionResources(session, sessionStore, attemptCleanup);
-  // The targeted platform close is the primary operation, not best-effort cleanup:
-  // its AppError (code/details/hint) is preserved and returned for the caller to
-  // rethrow, and a failed close must not be recorded as `Closed`. Subsequent
-  // resource cleanup still runs regardless.
-  const platformCloseError = params.skipPlatformClose
+  const platformCloseError = repairArmed
     ? undefined
     : await dispatchTargetedPlatformClose({ req, session, logPath });
   await stopOrRetainAppleRunnerAfterClose(retainAppleRunner, session, attemptCleanup);
   await clearSessionRuntimeHints(session, sessionStore, sessionName);
-  // ADR 0012 decision 6 (BLOCKER 2): a repair-armed session already recorded its
-  // finalize `close` and committed (or aborted) its healed `.ad` BEFORE this
-  // teardown (commit-state machine — the single commit path), and only AFTER
-  // its platform close (dispatched above `handleCloseCommand`) was confirmed to
-  // succeed. Only an ordinary (non-repair) session records `close` + writes its
-  // session log here, and — per #1225 — a failed platform close is not recorded
-  // as `Closed`.
-  if (!repairArmed) {
-    if (!platformCloseError) {
-      recordSessionAction(sessionStore, session, req, 'close', {
-        session: session.name,
-        ...successText(`Closed: ${session.name}`),
-      });
-    }
-    if (req.flags?.saveScript) {
-      session.recordSession = true;
-    }
-    sessionStore.writeSessionLog(session, { force: resolveEffectiveSaveScriptForce(req, session) });
-  }
+  const saveScriptError = repairArmed
+    ? undefined
+    : finalizeOrdinaryCloseScript({ req, session, sessionStore, platformCloseError });
   await attemptCleanup('materialized_paths', () =>
     cleanupRetainedMaterializedPathsForSession(sessionName),
   );
-  return platformCloseError;
+  return { platformCloseError, saveScriptError };
 }
 
 type CleanupRunner = (step: string, run: () => Promise<void>) => Promise<void>;
@@ -247,11 +123,7 @@ async function stopBestEffortSessionResources(
   sessionStore: SessionStore,
   attemptCleanup: CleanupRunner,
 ): Promise<void> {
-  // Finalize any still-active recording first so closing a session mid-recording
-  // (without an explicit `record stop`) cannot leak the recorder process; must
-  // run before the Apple runner is stopped below since overlay finalization
-  // consults the runner. `shouldRetainAppleRunnerAfterClose` then observes the
-  // now-cleared `session.recording`.
+  // Recording overlay finalization needs the Apple runner.
   await attemptCleanup('recording', () => stopSessionRecordingForTeardown(session));
   await attemptCleanup('app_log', () => stopSessionAppLog(session));
   await attemptCleanup('audio_probe', async () => {
@@ -265,22 +137,7 @@ async function stopBestEffortSessionResources(
   );
 }
 
-/**
- * ADR 0012 decision 6 (BLOCKER 3, third follow-up): identifies WHICH close
- * request's platform close succeeded — not merely THAT one did. Only the
- * request's TARGET (`positionals`) can change what `dispatchTargetedPlatformClose`
- * actually does: `shouldDispatchPlatformClose` decides purely from
- * `hasCloseTarget(req)` (plus the `web` special case, constant for a given
- * session), and the dispatch itself is `dispatchCommand(device, 'close',
- * req.positionals, ...)`. `close`'s only other flags (`shutdown`, `saveScript`
- * — see `closeCliSchema`) feed the post-teardown shutdown and the commit path
- * respectively, never this call, so they carry no identity here. Binding the
- * marker to this identity means an untargeted close's "succeeded" (a no-op,
- * since `shouldDispatchPlatformClose` was false) can never be misread as "the
- * platform close for THIS target already ran" by a later retry that adds or
- * changes the target — that retry's identity differs, so it re-dispatches.
- */
-function repairPlatformCloseIdentity(req: DaemonRequest): string {
+function buildRepairPlatformCloseReceipt(req: DaemonRequest): string {
   return JSON.stringify(req.positionals ?? []);
 }
 
@@ -296,29 +153,26 @@ async function prepareRepairClose(params: {
 }): Promise<RepairClosePreparation> {
   const { req, session, logPath, sessionStore } = params;
   const repairArmed = session.saveScriptBoundary !== undefined;
-  const closeIdentity = repairPlatformCloseIdentity(req);
-  if (
-    repairArmed &&
-    !(session.repairPlatformCloseSucceeded && session.repairPlatformCloseIdentity === closeIdentity)
-  ) {
+  const closeReceipt = buildRepairPlatformCloseReceipt(req);
+  if (repairArmed && session.repairPlatformCloseReceipt !== closeReceipt) {
     const platformCloseError = await dispatchTargetedPlatformClose({ req, session, logPath });
     if (platformCloseError) {
       return {
-        response: buildRepairCloseFailureResponse(
+        response: buildRetriableRepairCloseFailureResponse(
           session,
           toRepairPlatformCloseFailure(platformCloseError),
         ),
       };
     }
-    session.repairPlatformCloseSucceeded = true;
-    session.repairPlatformCloseIdentity = closeIdentity;
+    session.repairPlatformCloseReceipt = closeReceipt;
   }
-  const repairCommit = commitRepairBeforeClose(sessionStore, session, req);
+  const repairCommit = commitRepairScriptBeforeClose(sessionStore, session, req);
   if (repairCommit.kind === 'failed') {
-    return { response: buildRepairCloseFailureResponse(session, repairCommit.error) };
+    return {
+      response: buildRetriableRepairCloseFailureResponse(session, repairCommit.error),
+    };
   }
-  session.repairPlatformCloseSucceeded = false;
-  session.repairPlatformCloseIdentity = undefined;
+  session.repairPlatformCloseReceipt = undefined;
   return {
     repairArmed,
     ...(repairCommit.kind === 'committed' && repairCommit.path
@@ -450,73 +304,99 @@ export async function handleCloseCommand(params: {
   if (req.internal?.closeAppOnly === true) {
     return await closeAppWithoutEndingSession({ req, session, logPath });
   }
-  // ADR 0012 decision 6 (BLOCKER 2): for a repair-armed session, the platform
-  // close must run and SUCCEED before anything is committed or torn down —
-  // otherwise a committed healed `.ad` could claim a successful `close` that
-  // never actually happened on the device. On failure, return without
-  // touching the session at all (mirrors the commit-failure path below): it
-  // stays addressable so the agent can fix the cause and retry.
-  //
-  // BLOCKER 3: a PRIOR close attempt on this same session may already have
-  // dispatched the platform close and confirmed its success, then failed to
-  // commit (the session is retained for exactly that retry — see below). A
-  // retry must never re-dispatch a (possibly non-idempotent) platform close
-  // against an already-closed target; `repairPlatformCloseSucceeded` +
-  // `repairPlatformCloseIdentity` together record that the platform-level
-  // close already happened FOR THIS EXACT request identity, so a same-identity
-  // retry consumes it and goes straight to the commit instead. A retry whose
-  // identity DIFFERS (third follow-up: e.g. untargeted -> targeted, or a
-  // changed target) never matches — the marker only ever attests to the
-  // identity it was recorded under, so the platform close runs (again).
   const repair = await prepareRepairClose({ req, session, logPath, sessionStore });
   if ('response' in repair) return repair.response;
-  // Resource teardown is failure-isolated: a rejected step is collected instead of
-  // short-circuiting the rest, so every subsequent resource (and the runner stop)
-  // is still attempted. The provider lease is committed only after that teardown,
-  // and a failed provider release keeps the session retryable.
+  const closed = await runCloseTeardownAndRelease({
+    req,
+    session,
+    sessionName,
+    logPath,
+    sessionStore,
+    leaseRegistry,
+    leaseLifecycleProvider,
+    repairArmed: repair.repairArmed,
+  });
+  if (closed.kind === 'response') return closed.response;
+  const shutdownResult = await maybeShutdownSessionTarget({
+    device: session.device,
+    shutdownRequested: req.flags?.shutdown,
+  });
+  return buildCloseSuccessResponse({
+    session,
+    repair,
+    requestedSaveScript: Boolean(req.flags?.saveScript),
+    shutdownResult,
+    providerData: closed.providerData,
+  });
+}
+
+type SessionCloseFinalization =
+  | { kind: 'response'; response: DaemonResponse }
+  | { kind: 'closed'; providerData?: Record<string, unknown> };
+
+// Everything between a settled repair decision and a success response:
+// failure-isolated resource teardown, provider lease release, then (only
+// once both are known) the device-claim clear and session delete. A failed
+// lease release keeps the session retryable instead (`{kind:'response'}`).
+async function runCloseTeardownAndRelease(params: {
+  req: DaemonRequest;
+  session: SessionState;
+  sessionName: string;
+  logPath: string;
+  sessionStore: SessionStore;
+  leaseRegistry: LeaseRegistry;
+  leaseLifecycleProvider: LeaseLifecycleProvider | undefined;
+  repairArmed: boolean;
+}): Promise<SessionCloseFinalization> {
+  const {
+    req,
+    session,
+    sessionName,
+    logPath,
+    sessionStore,
+    leaseRegistry,
+    leaseLifecycleProvider,
+  } = params;
   const cleanupFailures: SessionCleanupFailure[] = [];
-  const platformCloseError = await runSessionCloseTeardown({
+  const { platformCloseError, saveScriptError } = await runSessionCloseTeardown({
     req,
     session,
     sessionName,
     logPath,
     sessionStore,
     cleanupFailures,
-    repairArmed: repair.repairArmed,
-    // The platform close for a repair-armed session already ran (and was
-    // confirmed to succeed) above, before the commit — never dispatch it twice.
-    skipPlatformClose: repair.repairArmed,
+    repairArmed: params.repairArmed,
   });
   const leaseRelease = await releaseProviderLeaseForClose({
     session,
     leaseRegistry,
     leaseLifecycleProvider,
   });
-  if (leaseRelease.response) return leaseRelease.response;
+  if (leaseRelease.response) return { kind: 'response', response: leaseRelease.response };
   const cleanupAggregate = reportSessionCleanupFailures({
     sessionName,
     phase: 'session_close_cleanup_failed',
     failures: cleanupFailures,
   });
-  if (!platformCloseError && !cleanupAggregate) {
+  const deviceClaimBlockingError = platformCloseError ?? cleanupAggregate;
+  if (!deviceClaimBlockingError) {
     await clearAdvisoryDeviceClaim(session.deviceClaim);
   }
   sessionStore.delete(sessionName);
-  // The platform-close failure is the primary error: rethrow it with its original
-  // code/details/hint intact. The cleanup aggregate has already been emitted as a
-  // diagnostic above so per-resource failures stay visible.
-  if (platformCloseError) throw platformCloseError;
-  if (cleanupAggregate) throw cleanupAggregate;
-  const shutdownResult = await maybeShutdownSessionTarget({
-    device: session.device,
-    shutdownRequested: req.flags?.shutdown,
-  });
-  // ADR 0012 decision 6 (BLOCKER 2a): positively report the committed healed
-  // artifact path so the agent learns the repair published (and where) without
-  // an extra round-trip.
-  const savedScript = repair.healedScriptPath ? { savedScript: repair.healedScriptPath } : {};
-  const text = `Closed: ${session.name}`;
-  if (repair.aborted && req.flags?.saveScript) {
+  if (deviceClaimBlockingError) throw deviceClaimBlockingError;
+  if (saveScriptError) throw saveScriptError;
+  return { kind: 'closed', providerData: leaseRelease.providerData };
+}
+
+function buildCloseSuccessResponse(params: {
+  session: SessionState;
+  repair: Extract<RepairClosePreparation, { repairArmed: boolean }>;
+  requestedSaveScript: boolean;
+  shutdownResult: DeviceTargetShutdownResult | undefined;
+  providerData: Record<string, unknown> | undefined;
+}): DaemonResponse {
+  const { session, repair, requestedSaveScript, shutdownResult, providerData } = params;
+  if (repair.aborted && requestedSaveScript) {
     return {
       ok: false,
       error: {
@@ -527,29 +407,24 @@ export async function handleCloseCommand(params: {
       },
     };
   }
-
+  // ADR 0012 decision 6 (BLOCKER 2a): positively report the committed healed
+  // artifact path so the agent learns the repair published (and where) without
+  // an extra round-trip.
+  const savedScript = repair.healedScriptPath ? { savedScript: repair.healedScriptPath } : {};
+  const provider = providerData ? { provider: providerData } : {};
+  const text = `Closed: ${session.name}`;
   if (shutdownResult) {
     return {
       ok: true,
       data: withSuccessText(
-        {
-          session: session.name,
-          shutdown: shutdownResult,
-          ...savedScript,
-          ...(leaseRelease.providerData ? { provider: leaseRelease.providerData } : {}),
-        },
+        { session: session.name, shutdown: shutdownResult, ...savedScript, ...provider },
         text,
       ),
     };
   }
   return {
     ok: true,
-    data: {
-      session: session.name,
-      ...successText(text),
-      ...savedScript,
-      ...(leaseRelease.providerData ? { provider: leaseRelease.providerData } : {}),
-    },
+    data: { session: session.name, ...successText(text), ...savedScript, ...provider },
   };
 }
 

src/daemon/session-store.ts

@@ -146,7 +146,7 @@ export class SessionStore {
    * teardown finalize step for a session (idle-reap or daemon shutdown).
    *
    * BLOCKER 3: unlike the explicit `close --save-script` path
-   * (`commitRepairBeforeClose`), teardown never runs `close`'s handler — but
+   * (`session-close-script.ts`), teardown never runs `close`'s handler — but
    * the source plan's terminal `close` was already skipped-while-armed (Fix
    * 3), so a COMPLETE transaction's auto-commit here must record the same
    * synthetic finalize `close` first, or the auto-committed healed `.ad`
@@ -187,8 +187,8 @@ export class SessionStore {
   }
 
   /**
-   * BLOCKER 3: mirrors `commitRepairBeforeClose`'s finalize-`close` recording
-   * (session-close.ts) for the auto-commit teardown path, which never routes
+   * BLOCKER 3: mirrors the explicit close script's finalize-`close` recording
+   * (`session-close-script.ts`) for the auto-commit path, which never routes
    * through `close`'s handler. Only recorded when this teardown is actually
    * about to attempt a commit (COMPLETE, not yet COMMITTED) — an aborted
    * (incomplete) transaction's write is a no-op regardless, so there is

src/daemon/types.ts

@@ -406,36 +406,8 @@ export type SessionState = {
    * (nothing to tombstone) from an aborted/reaped one.
    */
   saveScriptCommitted?: boolean;
-  /**
-   * ADR 0012 decision 6 (BLOCKER 3, second follow-up): set the moment a
-   * repair-armed `close`'s targeted platform close returns SUCCESS, cleared
-   * once the transaction commits/tears down (never lingers past a single
-   * close attempt's outcome). If the subsequent commit then FAILS (no-clobber
-   * refusal, a bare-`@ref` failure, or an fs error) the session is retained
-   * for retry — a later `close`/`close --save-script=<other>` on the SAME
-   * session must consume this flag and skip re-dispatching the platform
-   * close rather than invoking a (possibly non-idempotent) backend a second
-   * time against an already-closed target.
-   *
-   * BLOCKER 3 (third follow-up): this flag alone is session-wide, not bound
-   * to WHICH close request succeeded — see `repairPlatformCloseIdentity`,
-   * which must ALSO match the retry's own request identity before a retry is
-   * allowed to skip the platform close.
-   */
-  repairPlatformCloseSucceeded?: boolean;
-  /**
-   * ADR 0012 decision 6 (BLOCKER 3, third follow-up): the identity
-   * (`repairPlatformCloseIdentity` in session-close.ts — currently just the
-   * request's target/positionals, the only thing that changes what the
-   * platform close actually dispatches) of the close request whose platform
-   * close last succeeded. A retry only gets to skip re-dispatching the
-   * platform close when BOTH `repairPlatformCloseSucceeded` is true AND this
-   * matches the retry's own identity — an untargeted close that performed NO
-   * platform operation (so `repairPlatformCloseSucceeded` is trivially true)
-   * must never let a later targeted (or differently-targeted) retry skip the
-   * platform close it never actually ran.
-   */
-  repairPlatformCloseIdentity?: string;
+  /** Target identity of a successful repair close awaiting script commit. */
+  repairPlatformCloseReceipt?: string;
   /**
    * ADR 0012 decision 6, R7 (C5a): the original replay input path of an armed
    * repair, stashed so an idle-reap tombstone can hand the agent an actionable