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.

Author: thymikee

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,44 @@ 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();
 });
+
+// #1391 P2: the handler-level test (`session-device-claims.test.ts`) proves
+// `toOrdinaryCloseSaveScriptFailure` builds the right `AppError`, but calls
+// `handleCloseCommand` directly — it never exercises `normalizeError`/
+// `enrichDaemonError`'s own hoisting of `details.retriable` to the TOP-level
+// wire field, the same layer BLOCKER 2's test above exists to catch a
+// regression in. Exercised through the REAL router boundary so a regression
+// in either layer is caught, unlike the direct-call test.
+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/session-close.ts

@@ -251,11 +251,15 @@ async function runSessionCloseTeardown(params: {
  * call site's own comment for why a repair-armed session skips this entirely.
  *
  * #1391: the write can refuse to publish (no-clobber target-exists, or any
- * other fs `AppError`). Roll back the just-recorded `close` action on failure
- * — mirroring `commitRepairBeforeClose`'s own rollback — so neither the
- * session/device claim leaks (item 1) nor an unrecorded `close` survives for a
- * later successful write to duplicate (item 2); the caller still completes
- * teardown regardless of the outcome returned here.
+ * other fs `AppError`). Unlike a repair-armed commit failure — which keeps
+ * its session alive so `commitRepairBeforeClose` must roll back its
+ * just-recorded `close` action to keep a later retry from duplicating it —
+ * this caller (`runSessionCloseTeardown`) always completes teardown and
+ * deletes the session regardless of the outcome returned here, so there is
+ * no surviving session for a retry to duplicate anything on. No rollback:
+ * the recorded `close` action (and its durable `events.ndjson` entry) stay
+ * exactly as they are — an accurate record that the close itself happened,
+ * independent of whether the script also got saved.
  */
 function finalizeOrdinaryCloseScript(params: {
   req: DaemonRequest;
@@ -264,7 +268,6 @@ function finalizeOrdinaryCloseScript(params: {
   platformCloseError: unknown;
 }): AppError | undefined {
   const { req, session, sessionStore, platformCloseError } = params;
-  const actionsBeforeClose = session.actions.length;
   if (!platformCloseError) {
     recordSessionAction(sessionStore, session, req, 'close', {
       session: session.name,
@@ -278,7 +281,6 @@ function finalizeOrdinaryCloseScript(params: {
     sessionStore.writeSessionLog(session, { force: resolveEffectiveSaveScriptForce(req, session) });
     return undefined;
   } catch (error) {
-    session.actions.length = actionsBeforeClose;
     return toOrdinaryCloseSaveScriptFailure(error);
   }
 }