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

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