From c60aae545226a88d0a47ac8acc2067f13a8bf50a Mon Sep 17 00:00:00 2001 From: jry Date: Fri, 19 Jun 2026 18:49:26 +0800 Subject: [PATCH 1/4] fix: recover ACP errors from session snapshots --- src-tauri/src/acp/session_state.rs | 65 ++++++++++++++++--- src/contexts/acp-connections-context.test.tsx | 17 +++++ src/contexts/acp-connections-context.tsx | 6 +- src/lib/snapshot-denormalize.test.ts | 23 +++++++ src/lib/snapshot-denormalize.ts | 20 +++++- src/lib/types.ts | 8 +++ 6 files changed, 127 insertions(+), 12 deletions(-) diff --git a/src-tauri/src/acp/session_state.rs b/src-tauri/src/acp/session_state.rs index 504b51bdf..ecc78b21e 100644 --- a/src-tauri/src/acp/session_state.rs +++ b/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, /// 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, + /// 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, 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(); diff --git a/src/contexts/acp-connections-context.test.tsx b/src/contexts/acp-connections-context.test.tsx index 1df139436..b22352822 100644 --- a/src/contexts/acp-connections-context.test.tsx +++ b/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: [], }) diff --git a/src/contexts/acp-connections-context.tsx b/src/contexts/acp-connections-context.tsx index b18a51fbb..82354e2e9 100644 --- a/src/contexts/acp-connections-context.tsx +++ b/src/contexts/acp-connections-context.tsx @@ -1246,6 +1246,7 @@ function connectionsReducer( current.availableCommands ?? action.patch.availableCommands const mergedPromptCapabilities = action.patch.promptCapabilities ?? current.promptCapabilities + const recoveredError = current.error ?? action.patch.lastError // Race guard: the snapshot may have been generated BEFORE events // that have since arrived and been applied to in-memory state. @@ -1260,7 +1261,8 @@ function connectionsReducer( mergedModes === current.modes && mergedConfigOptions === current.configOptions && mergedAvailableCommands === current.availableCommands && - mergedPromptCapabilities === current.promptCapabilities + mergedPromptCapabilities === current.promptCapabilities && + recoveredError === current.error ) { return state } @@ -1273,6 +1275,7 @@ function connectionsReducer( promptCapabilities: mergedPromptCapabilities, selectorsReady: mergedSelectorsReady, supportsFork: mergedSupportsFork, + error: recoveredError, }) return next } @@ -1307,6 +1310,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 diff --git a/src/lib/snapshot-denormalize.test.ts b/src/lib/snapshot-denormalize.test.ts index cc2325c0e..f64babb5b 100644 --- a/src/lib/snapshot-denormalize.test.ts +++ b/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") + }) +}) diff --git a/src/lib/snapshot-denormalize.ts b/src/lib/snapshot-denormalize.ts index 87b254efb..9cda13b78 100644 --- a/src/lib/snapshot-denormalize.ts +++ b/src/lib/snapshot-denormalize.ts @@ -24,9 +24,9 @@ import type { /** * Snapshot-derived subset of ConnectionState. Fields not present here - * (pendingQuestion, claudeApiRetry, error, contextKey, agentType, - * workingDir) are frontend-only or set elsewhere and must not be touched - * by HYDRATE_FROM_SNAPSHOT. + * (pendingQuestion, claudeApiRetry, contextKey, agentType, workingDir) are + * frontend-only or set elsewhere and must not be touched by + * HYDRATE_FROM_SNAPSHOT. */ export interface SnapshotPatch { // Carries the snapshot's source connection_id so the reducer can reject @@ -66,6 +66,8 @@ export interface SnapshotPatch { * the one-shot `background_activity` events won't replay. `0` when the * server omitted the field. */ backgroundOutstanding: number + /** Latest ACP runtime error carried by the snapshot. `null` means none. */ + lastError: string | null eventSeq: number /** Live sub-agent delegations carried by the snapshot. Consumed directly at * the attach call sites to re-seed `DelegationProvider` bindings (see @@ -85,6 +87,7 @@ export function denormalizeSnapshot(wire: LiveSessionSnapshot): SnapshotPatch { for (const tc of wire.active_tool_calls) { toolMap.set(tc.id, tc) } + const lastError = normalizeSnapshotLastError(wire.last_error) return { connectionId: wire.connection_id, @@ -123,11 +126,22 @@ export function denormalizeSnapshot(wire: LiveSessionSnapshot): SnapshotPatch { configStale: wire.config_stale ?? false, configStaleKind: wire.config_stale_kind ?? null, backgroundOutstanding: wire.background_outstanding ?? 0, + lastError, eventSeq: wire.event_seq, activeDelegations: wire.active_delegations ?? [], } } +function normalizeSnapshotLastError( + lastError: LiveSessionSnapshot["last_error"] +): string | null { + const message = + lastError && typeof lastError.message === "string" + ? lastError.message.trim() + : "" + return message || null +} + function denormalizeLiveMessage( wire: WireLiveMessage, toolMap: Map diff --git a/src/lib/types.ts b/src/lib/types.ts index a6230623f..c65fd62d0 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -1456,6 +1456,12 @@ export interface FeedbackItem { delivered_at?: string | null } +/** Snapshot of the most recent ACP runtime error. */ +export interface SessionLastError { + message: string + code?: string | null +} + export interface LiveSessionSnapshot { connection_id: string conversation_id: number | null @@ -1502,6 +1508,8 @@ export interface LiveSessionSnapshot { config_stale?: boolean /** Which settings surface drifted; present only while `config_stale`. */ config_stale_kind?: ConfigStaleKind | null + /** Latest agent/runtime error recoverable after reconnect. */ + last_error?: SessionLastError | null event_seq: number } From 62d6cd1018377d4dd25dd3c65a41d89d5341e223 Mon Sep 17 00:00:00 2001 From: xintaofei Date: Mon, 13 Jul 2026 17:50:17 +0800 Subject: [PATCH 2/4] fix(acp): don't recover snapshot error on the stale race-guard path HYDRATE_FROM_SNAPSHOT's stale (event_seq <= lastAppliedSeq) path folded a snapshot's lastError back via `current.error ?? patch.lastError`. Since `error` is a current-state field cleared on a new prompt (STATUS_CHANGED -> prompting), a late stale snapshot generated between an error and the next prompt could resurrect an error the current turn already cleared, leaving a stale banner until the following prompt. Recover the snapshot error only on the fresh path, matching how status / configStale / backgroundOutstanding are already treated (current-state fields, fresh-path only). --- src/contexts/acp-connections-context.tsx | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/contexts/acp-connections-context.tsx b/src/contexts/acp-connections-context.tsx index 82354e2e9..8a19feb47 100644 --- a/src/contexts/acp-connections-context.tsx +++ b/src/contexts/acp-connections-context.tsx @@ -1246,14 +1246,17 @@ function connectionsReducer( current.availableCommands ?? action.patch.availableCommands const mergedPromptCapabilities = action.patch.promptCapabilities ?? current.promptCapabilities - const recoveredError = current.error ?? action.patch.lastError // 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 && @@ -1261,8 +1264,7 @@ function connectionsReducer( mergedModes === current.modes && mergedConfigOptions === current.configOptions && mergedAvailableCommands === current.availableCommands && - mergedPromptCapabilities === current.promptCapabilities && - recoveredError === current.error + mergedPromptCapabilities === current.promptCapabilities ) { return state } @@ -1275,7 +1277,6 @@ function connectionsReducer( promptCapabilities: mergedPromptCapabilities, selectorsReady: mergedSelectorsReady, supportsFork: mergedSupportsFork, - error: recoveredError, }) return next } From a394661b1b95160593019382c86772b28002d03f Mon Sep 17 00:00:00 2001 From: xintaofei Date: Mon, 13 Jul 2026 18:17:29 +0800 Subject: [PATCH 3/4] test(acp): cover snapshot last_error fresh recovery + stale non-resurrection Lock in HYDRATE_FROM_SNAPSHOT error behavior: a fresh snapshot recovers a last_error the client missed live, and a stale snapshot must not resurrect an error the current turn already cleared. The stale case fails against the pre-fix reducer. Fixtures also set backgroundOutstanding for completeness. --- src/contexts/acp-connections-context.test.tsx | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) diff --git a/src/contexts/acp-connections-context.test.tsx b/src/contexts/acp-connections-context.test.tsx index b22352822..ae2bc6070 100644 --- a/src/contexts/acp-connections-context.test.tsx +++ b/src/contexts/acp-connections-context.test.tsx @@ -1015,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 { + 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() + }) +}) From dca1328e29e8435419a6d774da7065b507705583 Mon Sep 17 00:00:00 2001 From: xintaofei Date: Mon, 13 Jul 2026 19:20:22 +0800 Subject: [PATCH 4/4] test(acp): pin HOME in skill-storage spec tests to remove isolation race The pi and kimi skill_storage_spec tests read the process-global $HOME while computing both the spec and the expected paths; a concurrent temp_env HOME-mutating test could flip $HOME between the two reads and fail the assertion. Wrap both in temp_env::with_vars to pin $HOME (and clear the PI_CODING_AGENT_DIR / KIMI_CODE_HOME overrides) so they serialize with those tests and read one consistent home. Expected paths stay derived from the production helpers, keeping them correct on Windows where dirs::home_dir ignores HOME. --- src-tauri/src/commands/acp.rs | 64 ++++++++++++++++++++++++++--------- 1 file changed, 48 insertions(+), 16 deletions(-) diff --git a/src-tauri/src/commands/acp.rs b/src-tauri/src/commands/acp.rs index 3a9b35e81..250d2d232 100644 --- a/src-tauri/src/commands/acp.rs +++ b/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]