[codex] Recover ACP runtime errors from snapshots

src-tauri/src/acp/session_state.rs

@@ -287,15 +287,15 @@ pub struct SessionState {
     /// "init complete" without waiting for an event that already fired.
     pub selectors_ready: bool,
 
-    /// Most recent `AcpEvent::Error` payload, or `None` if no error has
-    /// landed since the connection started. The probe path reads this
-    /// after `wait_for_session_options` errors so it can fold the
-    /// agent's own error message into the returned `AcpError` instead
-    /// of surfacing a generic "connection not found" once the
-    /// connection task has cleaned up its map entry.
+    /// Most recent unresolved `AcpEvent::Error` payload. Cleared when a new
+    /// prompt starts, matching the frontend reducer's live-event behavior. The
+    /// probe path reads this after `wait_for_session_options` errors so it can
+    /// fold the agent's own error message into the returned `AcpError` instead
+    /// of surfacing a generic "connection not found" once the connection task
+    /// has cleaned up its map entry.
     ///
-    /// Not exposed on `to_snapshot()` today — chat-side error UX already
-    /// flows through the live `AcpEvent::Error` channel.
+    /// Exposed on `to_snapshot()` so clients that reconnect after missing the
+    /// live `AcpEvent::Error` can still surface the latest agent failure.
     pub last_error: Option<SessionLastError>,
 
     /// Single-fire signal that fires when `SessionStarted` applies (i.e.
@@ -502,6 +502,12 @@ impl SessionState {
                 }
             }
             AcpEvent::StatusChanged { status } => {
+                if matches!(status, ConnectionStatus::Prompting) {
+                    // Match the live frontend reducer: a new prompt starts a
+                    // new error scope, so stale recoverable errors must not be
+                    // resurrected by a later snapshot attach.
+                    self.last_error = None;
+                }
                 self.status = status.clone();
             }
             AcpEvent::SessionModes { modes } => {
@@ -1145,6 +1151,7 @@ impl SessionState {
             selectors_ready: self.selectors_ready,
             config_stale: self.config_stale,
             config_stale_kind: self.config_stale_kind,
+            last_error: self.last_error.clone(),
             event_seq: self.event_seq,
         }
     }
@@ -1236,6 +1243,10 @@ pub struct LiveSessionSnapshot {
     /// byte-identical with the pre-feature wire shape.
     #[serde(default, skip_serializing_if = "Option::is_none")]
     pub config_stale_kind: Option<ConfigStaleKind>,
+    /// Most recent agent/runtime error for this live connection. Omitted when
+    /// no error has occurred so older clients and common snapshots stay small.
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub last_error: Option<SessionLastError>,
     pub event_seq: u64,
 }
 
@@ -1492,6 +1503,44 @@ mod tests {
         );
     }
 
+    #[test]
+    fn snapshot_carries_last_error_and_clears_on_next_prompt() {
+        let mut s = fresh_state();
+        s.apply_event(&AcpEvent::Error {
+            message: "ACP protocol error: forbidden".into(),
+            agent_type: "claude_code".into(),
+            code: Some("forbidden".into()),
+            terminal: true,
+        });
+
+        let snap = s.to_snapshot();
+        assert_eq!(
+            snap.last_error,
+            Some(SessionLastError {
+                message: "ACP protocol error: forbidden".into(),
+                code: Some("forbidden".into()),
+            })
+        );
+
+        let json = serde_json::to_string(&snap).expect("serialize");
+        let back: LiveSessionSnapshot = serde_json::from_str(&json).expect("deserialize");
+        assert_eq!(back.last_error, snap.last_error);
+
+        let empty_json = serde_json::to_string(&fresh_state().to_snapshot()).expect("serialize");
+        assert!(
+            !empty_json.contains("last_error"),
+            "no-error snapshot must omit last_error"
+        );
+
+        s.apply_event(&AcpEvent::StatusChanged {
+            status: ConnectionStatus::Prompting,
+        });
+        assert!(
+            s.to_snapshot().last_error.is_none(),
+            "new prompts clear stale snapshot-recoverable errors"
+        );
+    }
+
     #[test]
     fn latest_live_reply_prefers_answer_after_last_tool_call() {
         let mut s = fresh_state();

src-tauri/src/commands/acp.rs

@@ -8885,26 +8885,58 @@ wire_api = "chat"
 
     #[test]
     fn kimi_code_skill_storage_spec_targets_kimi_home() {
-        let spec =
-            skill_storage_spec(AgentType::KimiCode).expect("Kimi Code supports skills");
-        assert_eq!(spec.kind, SkillStorageKind::SkillDirectoryOnly);
-        assert_eq!(spec.project_rel_dirs, vec![".kimi-code/skills"]);
-        let expected = crate::parsers::kimi_code::resolve_kimi_code_home_dir().join("skills");
-        assert_eq!(spec.global_dirs, vec![expected]);
+        // `resolve_kimi_code_home_dir()` reads the process-wide `$HOME` (when
+        // `KIMI_CODE_HOME` is unset), and other tests mutate HOME via `temp_env`.
+        // Pin it (and clear `KIMI_CODE_HOME`) so the spec and the expected path
+        // resolve against one consistent home instead of racing a concurrent
+        // HOME-mutating test. Deriving `expected` from the same production helper
+        // keeps it correct on Windows, where `dirs::home_dir()` ignores HOME.
+        let tmp = tempfile::tempdir().expect("tempdir");
+        temp_env::with_vars(
+            [
+                ("HOME", Some(tmp.path())),
+                ("KIMI_CODE_HOME", None::<&std::path::Path>),
+            ],
+            || {
+                let spec =
+                    skill_storage_spec(AgentType::KimiCode).expect("Kimi Code supports skills");
+                assert_eq!(spec.kind, SkillStorageKind::SkillDirectoryOnly);
+                assert_eq!(spec.project_rel_dirs, vec![".kimi-code/skills"]);
+                let expected =
+                    crate::parsers::kimi_code::resolve_kimi_code_home_dir().join("skills");
+                assert_eq!(spec.global_dirs, vec![expected]);
+            },
+        );
     }
 
     #[test]
     fn pi_skill_storage_spec_targets_pi_agent_dir() {
-        let spec = skill_storage_spec(AgentType::Pi).expect("Pi supports skills");
-        // pi's native dir accepts standalone `.md` files, like Codex.
-        assert_eq!(spec.kind, SkillStorageKind::SkillDirectoryOrMarkdownFile);
-        assert_eq!(spec.project_rel_dirs, vec![".pi/skills", ".agents/skills"]);
-        // Native pi dir first (preferred link target), shared store second.
-        let expected = vec![
-            pi_agent_dir().join("skills"),
-            home_dir_or_default().join(".agents").join("skills"),
-        ];
-        assert_eq!(spec.global_dirs, expected);
+        // `pi_agent_dir()` and `home_dir_or_default()` both read the process-wide
+        // `$HOME`, and other tests mutate HOME via `temp_env`. Pin it (and clear
+        // the BYO `PI_CODING_AGENT_DIR` override) so this test serializes against
+        // those mutators through temp_env's shared lock and reads one consistent
+        // home for both the spec and the expected paths. Deriving `expected` from
+        // the same production helpers keeps it correct on Windows, where
+        // `dirs::home_dir()` ignores the pinned HOME.
+        let tmp = tempfile::tempdir().expect("tempdir");
+        temp_env::with_vars(
+            [
+                ("HOME", Some(tmp.path())),
+                ("PI_CODING_AGENT_DIR", None::<&std::path::Path>),
+            ],
+            || {
+                let spec = skill_storage_spec(AgentType::Pi).expect("Pi supports skills");
+                // pi's native dir accepts standalone `.md` files, like Codex.
+                assert_eq!(spec.kind, SkillStorageKind::SkillDirectoryOrMarkdownFile);
+                assert_eq!(spec.project_rel_dirs, vec![".pi/skills", ".agents/skills"]);
+                // Native pi dir first (preferred link target), shared store second.
+                let expected = vec![
+                    pi_agent_dir().join("skills"),
+                    home_dir_or_default().join(".agents").join("skills"),
+                ];
+                assert_eq!(spec.global_dirs, expected);
+            },
+        );
     }
 
     #[test]

src/contexts/acp-connections-context.test.tsx

@@ -136,6 +136,22 @@ beforeEach(() => {
   h.denormalizeSnapshot.mockReset()
   h.denormalizeSnapshot.mockReturnValue({
     connectionId: "owner-conn",
+    status: "connected",
+    sessionId: null,
+    modes: null,
+    configOptions: null,
+    availableCommands: null,
+    usage: null,
+    liveMessage: null,
+    pendingPermission: null,
+    pendingAskQuestion: null,
+    pendingUserMessage: null,
+    promptCapabilities: null,
+    selectorsReady: false,
+    supportsFork: false,
+    configStale: false,
+    configStaleKind: null,
+    lastError: null,
     eventSeq: 0,
     activeDelegations: [],
   })
@@ -498,6 +514,7 @@ describe("AcpConnectionsProvider permission request details", () => {
       supportsFork: false,
       configStale: false,
       configStaleKind: null,
+      lastError: null,
       eventSeq: 5,
       activeDelegations: [],
     })
@@ -998,3 +1015,98 @@ describe("AcpConnectionsProvider Grok cross-agent-type model switch", () => {
     expect(saveConfigPreference).toHaveBeenCalledTimes(1)
   })
 })
+
+describe("HYDRATE_FROM_SNAPSHOT last_error recovery", () => {
+  // Full SnapshotPatch fixture; per-test overrides set connectionId / eventSeq /
+  // lastError. `denormalizeSnapshot` is mocked, so onSnapshot dispatches exactly
+  // this object as `action.patch`.
+  function snapshotPatch(overrides: {
+    eventSeq: number
+    lastError: string | null
+    connectionId?: string
+  }) {
+    return {
+      connectionId: "spawned-conn",
+      status: "connected",
+      sessionId: null,
+      modes: null,
+      configOptions: null,
+      availableCommands: null,
+      usage: null,
+      liveMessage: null,
+      pendingPermission: null,
+      pendingAskQuestion: null,
+      pendingUserMessage: null,
+      promptCapabilities: null,
+      selectorsReady: false,
+      supportsFork: false,
+      configStale: false,
+      configStaleKind: null,
+      backgroundOutstanding: 0,
+      activeDelegations: [],
+      ...overrides,
+    }
+  }
+
+  async function connectOwner(): Promise<AttachHandlers> {
+    h.acpFindConnectionForConversation.mockResolvedValue(null)
+    h.acpGetAgentStatus.mockResolvedValue({
+      agent_type: "claude_code",
+      enabled: true,
+      available: true,
+      installed_version: "1.0.0",
+    })
+    await mountProvider()
+    await act(async () => {
+      await h.actions!.connect(TAB, "claude_code", "/tmp/x", "sess-1", 42)
+    })
+    return latestAttachHandlers()
+  }
+
+  it("recovers last_error from a FRESH snapshot (client missed the live error)", async () => {
+    const handlers = await connectOwner()
+    // A freshly reconnected client (lastAppliedSeq=0) receives a snapshot ahead
+    // of its cursor carrying an error whose live event it never saw. The fresh
+    // path recovers it.
+    h.denormalizeSnapshot.mockReturnValue(
+      snapshotPatch({ eventSeq: 5, lastError: "boom from snapshot" })
+    )
+    hydrateSnapshot(handlers, {
+      event_seq: 5,
+    } as unknown as LiveSessionSnapshot)
+    expect(h.store!.getConnection(TAB)!.error).toBe("boom from snapshot")
+  })
+
+  it("does NOT resurrect a cleared error from a STALE snapshot", async () => {
+    const handlers = await connectOwner()
+    // Live: an error lands, then a new prompt starts and clears it. This also
+    // advances lastAppliedSeq to 2.
+    emitAcpEvent(handlers, {
+      seq: 1,
+      connection_id: "spawned-conn",
+      type: "error",
+      message: "boom",
+      agent_type: "claude_code",
+      code: "runtime_failure",
+    })
+    expect(h.store!.getConnection(TAB)!.error).toBe("boom")
+    emitAcpEvent(handlers, {
+      seq: 2,
+      connection_id: "spawned-conn",
+      type: "status_changed",
+      status: "prompting",
+    })
+    expect(h.store!.getConnection(TAB)!.error).toBeNull()
+
+    // A snapshot generated BEFORE the prompt (eventSeq=1 <= lastAppliedSeq=2)
+    // still carries the old error. Folding it back in would resurrect an error
+    // the current turn already cleared — the stale path must leave error alone.
+    h.denormalizeSnapshot.mockReturnValue(
+      snapshotPatch({ eventSeq: 1, lastError: "boom" })
+    )
+    hydrateSnapshot(handlers, {
+      event_seq: 1,
+    } as unknown as LiveSessionSnapshot)
+    expect(h.store!.getConnection(TAB)!.error).toBeNull()
+  })
+})

src/contexts/acp-connections-context.tsx

@@ -1250,9 +1250,13 @@ function connectionsReducer(
       // Race guard: the snapshot may have been generated BEFORE events
       // that have since arrived and been applied to in-memory state.
       // Mutable fields (status, sessionId, liveMessage, pendingPermission,
-      // usage) are fresher in memory than in the snapshot and must NOT be
-      // overwritten — but the latched/fill-null fields above are still
-      // applied so the once-per-lifetime bits can recover.
+      // usage, error) are fresher in memory than in the snapshot and must NOT
+      // be overwritten — but the latched/fill-null fields above are still
+      // applied so the once-per-lifetime bits can recover. `error` in
+      // particular is cleared on a new prompt (STATUS_CHANGED → prompting), so
+      // folding a stale snapshot's `lastError` back in here would resurrect an
+      // error the current turn already cleared; it is recovered on the fresh
+      // path below instead.
       if (action.patch.eventSeq <= current.lastAppliedSeq) {
         if (
           mergedSelectorsReady === current.selectorsReady &&
@@ -1307,6 +1311,7 @@ function connectionsReducer(
         // recovers the pending-background count the one-shot events won't
         // replay for it (sweep exemption + chip).
         backgroundOutstanding: action.patch.backgroundOutstanding,
+        error: action.patch.lastError,
         lastAppliedSeq: action.patch.eventSeq,
       })
       return next

src/lib/snapshot-denormalize.test.ts

@@ -74,3 +74,26 @@ describe("denormalizeSnapshot — config staleness", () => {
     expect(patch.configStaleKind).toBeNull()
   })
 })
+
+describe("denormalizeSnapshot — last_error", () => {
+  it("carries last_error.message into the patch", () => {
+    const patch = denormalizeSnapshot(
+      baseSnapshot({
+        last_error: {
+          message: " ACP protocol error: Forbidden ",
+          code: "forbidden",
+        },
+      })
+    )
+    expect(patch.lastError).toBe("ACP protocol error: Forbidden")
+    expect(patch.status).toBe("connected")
+  })
+
+  it("defaults lastError to null when the field is absent", () => {
+    const snap = baseSnapshot()
+    delete (snap as { last_error?: unknown }).last_error
+    const patch = denormalizeSnapshot(snap)
+    expect(patch.lastError).toBeNull()
+    expect(patch.status).toBe("connected")
+  })
+})