From e202368303f3e6763cd0d8dbb1e4da4276c71d4b Mon Sep 17 00:00:00 2001 From: raeedz Date: Sat, 11 Jul 2026 15:09:10 -0700 Subject: [PATCH 1/2] Feat: codex spinner working --- src-tauri/resources/goonware-codex-hook.sh | 22 +- src-tauri/resources/goonware-gemini-hook.sh | 1 + src-tauri/src/agent_hooks.rs | 436 +++++++++++++++++--- src/lib/claudeUsage.test.ts | 27 ++ src/lib/claudeUsage.ts | 24 ++ src/terminal/AgentChrome.tsx | 86 +++- src/terminal/BlockTerminal.tsx | 109 +++-- 7 files changed, 577 insertions(+), 128 deletions(-) diff --git a/src-tauri/resources/goonware-codex-hook.sh b/src-tauri/resources/goonware-codex-hook.sh index 32d8574..bcbfbe2 100644 --- a/src-tauri/resources/goonware-codex-hook.sh +++ b/src-tauri/resources/goonware-codex-hook.sh @@ -14,10 +14,13 @@ ls /tmp/goonware-agent-*.sock >/dev/null 2>&1 || exit 0 GOONWARE_SID="${GOONWARE_SESSION_ID:-${GLI_SESSION_ID:-$RLI_SESSION_ID}}" [ -z "$GOONWARE_SID" ] && exit 0 -# Codex's hook protocol is similar to Claude's but emits fewer events -# (only SessionStart / UserPromptSubmit / Stop are reliably wired). -# That means we have no SessionEnd signal — the Rust side compensates -# with PID-based liveness monitoring, for which it needs the codex +# Codex's hook protocol mirrors Claude's, but only SessionStart / +# UserPromptSubmit / Stop are guaranteed on every Codex build — the +# richer events (PreToolUse / PostToolUse / Notification / PreCompact +# / SessionEnd) fire on newer CLIs and are forwarded verbatim when +# they do; the Rust classifier handles them all. Codex still has no +# reliable SessionEnd signal, so the Rust side compensates with +# PID-based liveness monitoring, for which it needs the codex # process id. We walk up the parent process tree looking for "codex" # so the Rust side has a PID to watch. /usr/bin/python3 -c " @@ -69,6 +72,14 @@ def codex_pid(): pid = info['ppid'] return None +# Same envelope shape as goonware-claude-hook.sh: +# - 'aux' carries Notification's sub-classifier (notification_type) +# so the Rust side can tell idle_prompt (→ Idle) from a real +# question (→ Waiting). +# - 'prompt' carries the user's typed text for UserPromptSubmit so +# the tab-subtitle summarizer works for codex tabs too. Captured +# here (inside codex's process tree) so Goonware never has to read +# ~/.codex/sessions/*.jsonl — same TCC rationale as Claude. out = { 'provider': 'codex', 'session_id': payload.get('session_id', ''), @@ -76,7 +87,8 @@ out = { 'cwd': payload.get('cwd', ''), 'event': payload.get('hook_event_name', ''), 'tool': payload.get('tool_name', ''), - 'aux': '', + 'aux': payload.get('notification_type', ''), + 'prompt': payload.get('prompt', '') or '', 'goonware_session_id': '$GOONWARE_SID', 'goonware_instance_id': os.environ.get('GOONWARE_INSTANCE_ID', ''), } diff --git a/src-tauri/resources/goonware-gemini-hook.sh b/src-tauri/resources/goonware-gemini-hook.sh index fd89cf0..d71cd16 100644 --- a/src-tauri/resources/goonware-gemini-hook.sh +++ b/src-tauri/resources/goonware-gemini-hook.sh @@ -71,6 +71,7 @@ out = { 'event': payload.get('hook_event_name', ''), 'tool': payload.get('tool_name', ''), 'aux': payload.get('notification_type', ''), + 'prompt': payload.get('prompt', '') or '', 'goonware_session_id': '$GOONWARE_SID', 'goonware_instance_id': os.environ.get('GOONWARE_INSTANCE_ID', ''), } diff --git a/src-tauri/src/agent_hooks.rs b/src-tauri/src/agent_hooks.rs index d4a9175..1436c62 100644 --- a/src-tauri/src/agent_hooks.rs +++ b/src-tauri/src/agent_hooks.rs @@ -26,14 +26,19 @@ //! //! Each provider installs differently: //! * Claude → `~/.claude/settings.json` (hooks block per event name). -//! * Codex → `~/.codex/hooks.json` + `codex_hooks = true` flag in `~/.codex/config.toml`. +//! * Codex → `~/.codex/hooks.json` + `hooks = true` under `[features]` in `~/.codex/config.toml`. //! * Gemini → `~/.gemini/settings.json` (hooks block per event name). //! -//! Codex's hook coverage is the thinnest — only SessionStart / -//! UserPromptSubmit / Stop fire reliably. There's no SessionEnd, so the -//! Rust side does PID-based liveness monitoring: every 2s, walk all -//! known Codex sessions and `kill(pid, 0)`. After two consecutive -//! misses, synthesize a SessionEnd to evict the session from the map. +//! Codex's GUARANTEED hook coverage is the thinnest — only +//! SessionStart / UserPromptSubmit / Stop fire on every build. The +//! installer registers the full Claude-equivalent roster anyway +//! (tool events, Notification, compaction, SessionEnd); newer Codex +//! CLIs fire them and get full parity, older ones silently ignore +//! the extra registrations. Because SessionEnd can't be relied on, +//! the Rust side also does PID-based liveness monitoring: every 2s, +//! walk all known Codex sessions and `kill(pid, 0)`. After two +//! consecutive misses, synthesize a SessionEnd to evict the session +//! from the map. use std::collections::HashMap; use std::fs; @@ -192,11 +197,21 @@ struct HookEnvelope { aux: String, /// The CLI process id we should watch for liveness. All three /// providers populate this now (Claude/Gemini for parity with - /// Codex's PID watchdog). `alias = "codex_process_id"` keeps a - /// stale hook script (left on disk from a prior Goonware build) - /// parseable during the upgrade window. - #[serde(default, alias = "codex_process_id")] + /// Codex's PID watchdog). + /// + /// TWO separate fields, NOT a serde alias. The codex hook script + /// deliberately sends BOTH names (`agent_process_id` for current + /// builds, `codex_process_id` for older ones), and serde treats + /// an alias receiving both spellings as a `duplicate field` + /// DECODE ERROR — which silently rejected every real codex + /// envelope and killed the spinner for codex entirely (the + /// "decode failed: duplicate field `agent_process_id`" lines in + /// the app log). Read through {@agent_pid} which merges the two. + #[serde(default)] agent_process_id: Option, + /// Legacy wire name for the same PID — see `agent_process_id`. + #[serde(default)] + codex_process_id: Option, /// True iff this envelope is from a Goonware helper-agent invocation /// (commit-message draft, PR description, etc.). Belt-and-braces: /// the hook script already exits early when `GOONWARE_HELPER_AGENT` is @@ -242,6 +257,17 @@ struct HookEnvelope { prompt: String, } +impl HookEnvelope { + /// The liveness-watchdog PID, whichever wire name it arrived + /// under. Scripts may send `agent_process_id`, the legacy + /// `codex_process_id`, or both — both spellings are first-class + /// fields (NOT serde aliases) precisely so a script sending both + /// can't trip serde's duplicate-field decode error. + fn agent_pid(&self) -> Option { + self.agent_process_id.or(self.codex_process_id) + } +} + /// Drop envelopes that shouldn't move the spinner at all. Three rules: /// 1. `goonware_helper=true` — internal helper-agent one-shot, never a /// user-visible turn. @@ -371,17 +397,46 @@ fn classify_event( }, Provider::Codex => match event { - // Codex emits these three reliably. The fourth state - // (Ended) is synthesized by the PID monitor, not by the - // CLI itself. + // Codex emits SessionStart / UserPromptSubmit / Stop on + // every build; the richer events below fire on newer CLIs + // (the installer registers them all — older Codex builds + // simply never send them). Ended is still synthesized by + // the PID monitor when SessionEnd doesn't arrive. "UserPromptSubmit" => Some((SessionStatus::Working, true)), - "SessionStart" => Some((SessionStatus::Idle, false)), - "Stop" => Some((SessionStatus::Idle, false)), + + // Same in-turn gate as Claude: tool events refresh the + // Working state (and last_tool → "Codex is using X") only + // inside a user turn; startup housekeeping is ignored. + // SubagentStop is in-turn, not turn-end — same reasoning + // as the Claude arm above. + "PreToolUse" | "PostToolUse" | "PostToolUseFailure" | "SubagentStart" + | "SubagentStop" => { + if in_user_turn { + Some((SessionStatus::Working, true)) + } else { + None + } + } + + "PreCompact" => Some((SessionStatus::Compacting, in_user_turn)), + "PostCompact" => Some((SessionStatus::Idle, in_user_turn)), + + "PermissionRequest" => Some((SessionStatus::Waiting, in_user_turn)), + // Codex's notification taxonomy isn't fully documented; // mirror Claude's "Notification = the user is being // asked something" heuristic so codex's permission / - // tool-confirm prompts also stop the spinner. - "Notification" => Some((SessionStatus::Waiting, in_user_turn)), + // tool-confirm prompts also stop the spinner. Honor the + // idle_prompt carve-out when the CLI sends it (the hook + // script forwards notification_type as aux). + "Notification" => match aux { + "idle_prompt" => Some((SessionStatus::Idle, false)), + _ => Some((SessionStatus::Waiting, in_user_turn)), + }, + + "SessionStart" => Some((SessionStatus::Idle, false)), + "Stop" => Some((SessionStatus::Idle, false)), + "SessionEnd" => Some((SessionStatus::Ended, false)), _ => None, }, @@ -630,7 +685,7 @@ fn handle_connection(mut stream: UnixStream, app: &AppHandle) { status, last_event: envelope.event.clone(), last_tool: envelope.tool.clone(), - agent_process_id: envelope.agent_process_id, + agent_process_id: envelope.agent_pid(), updated_at_ms: now_ms(), } } else { @@ -672,7 +727,7 @@ fn handle_connection(mut stream: UnixStream, app: &AppHandle) { status, last_event: envelope.event.clone(), last_tool: envelope.tool.clone(), - agent_process_id: envelope.agent_process_id, + agent_process_id: envelope.agent_pid(), updated_at_ms: now_ms(), }); entry.cwd = envelope.cwd.clone(); @@ -684,8 +739,8 @@ fn handle_connection(mut stream: UnixStream, app: &AppHandle) { // Keep the most recent non-None pid. Each hook fire // includes the PID; this is just defensive in case some // event omits it. - if envelope.agent_process_id.is_some() { - entry.agent_process_id = envelope.agent_process_id; + if envelope.agent_pid().is_some() { + entry.agent_process_id = envelope.agent_pid(); } entry.updated_at_ms = now_ms(); // Any successful event proves the agent is alive — drop @@ -977,7 +1032,7 @@ fn install_codex_hooks() { eprintln!("[goonware-hooks] write codex config.toml failed: {e}"); } else { eprintln!( - "[goonware-hooks] enabled codex_hooks in {}", + "[goonware-hooks] enabled [features].hooks in {}", config_path.display() ); } @@ -987,16 +1042,37 @@ fn upsert_codex_hooks_json(mut root: Value, script_path: &Path) -> Value { let command = script_path.to_string_lossy().into_owned(); let hook_entry = json!([{"type": "command", "command": command}]); let with_matcher = json!([{"matcher": "startup|resume", "hooks": hook_entry}]); + // Tool events take a tool-name matcher; "*" = every tool, same as + // the Claude installer's PreToolUse/PostToolUse registration. + let with_star_matcher = json!([{"matcher": "*", "hooks": hook_entry}]); let without_matcher = json!([{"hooks": hook_entry}]); let with_timeout = json!([{"hooks": [{"type": "command", "command": command, "timeout": 30}]}]); - // Codex's reliable hook surface is small — these three events - // cover spinner-on / spinner-off / fresh-start. + // Codex's GUARANTEED hook surface is small — SessionStart / + // UserPromptSubmit / Stop cover spinner-on / spinner-off / + // fresh-start on every Codex build. The rest of the roster is + // registered for parity with Claude: newer Codex CLIs fire + // Notification (permission / question prompts must park the + // spinner), PreToolUse / PostToolUse ("Codex is using X" + the + // mid-turn Working refresh), PreCompact / PostCompact (the + // Compacting state), and SessionEnd (instant eviction instead of + // waiting on the PID watchdog). Codex silently ignores event + // names it doesn't know, so registering them on an older CLI is + // harmless — the classifier just never sees them. let events: &[(&str, &Value)] = &[ ("SessionStart", &with_matcher), ("UserPromptSubmit", &without_matcher), + ("PreToolUse", &with_star_matcher), + ("PostToolUse", &with_star_matcher), + ("PermissionRequest", &with_star_matcher), + ("Notification", &without_matcher), + ("PreCompact", &without_matcher), + ("PostCompact", &without_matcher), + ("SubagentStart", &with_star_matcher), + ("SubagentStop", &with_star_matcher), ("Stop", &with_timeout), + ("SessionEnd", &without_matcher), ]; if !root.is_object() { @@ -1046,52 +1122,87 @@ fn upsert_codex_hooks_json(mut root: Value, script_path: &Path) -> Value { root } -/// Ensure `codex_hooks = true` exists under `[features]`. Preserves -/// the rest of config.toml byte-for-byte. Lightweight string editing -/// is enough — no need to pull in a full TOML parser. +/// Ensure `hooks = true` exists under `[features]`, and migrate away +/// the pre-0.144 `codex_hooks` flag. Codex 0.144 renamed the feature +/// flag: `codex_hooks` is deprecated — hooks.json entries DON'T run +/// under it anymore, and its mere presence makes codex print a red +/// deprecation banner on every launch. (This was the "spinner never +/// runs for codex" bug: our hooks were registered but the feature +/// gate no longer honored the old flag, so no event ever fired.) +/// Preserves the rest of config.toml byte-for-byte. Lightweight +/// string editing is enough — no need to pull in a full TOML parser. +/// Section-aware: only lines inside `[features]` are touched, so a +/// hypothetical `hooks` key in another table survives. fn upsert_codex_feature_flag(existing: &str) -> String { - let target_line = "codex_hooks = true"; - - // 1. If a `codex_hooks = ...` line already exists, rewrite it. - if existing - .lines() - .any(|l| l.trim_start().starts_with("codex_hooks")) - { - let mut out = String::with_capacity(existing.len()); - for line in existing.split_inclusive('\n') { - let stripped = line.trim_start(); - if stripped.starts_with("codex_hooks") { + let target_line = "hooks = true"; + + // Pass 1: walk sections; inside [features], drop the deprecated + // codex_hooks line and rewrite any existing hooks line in place. + let mut out = String::with_capacity(existing.len() + target_line.len() + 1); + let mut in_features = false; + let mut has_features_header = false; + let mut wrote_flag = false; + for line in existing.split_inclusive('\n') { + let trimmed = line.trim(); + if trimmed.starts_with('[') { + in_features = trimmed == "[features]"; + if in_features { + has_features_header = true; + } + out.push_str(line); + continue; + } + if in_features { + let key = trimmed + .split(['=', ' ', '\t']) + .next() + .unwrap_or(""); + if key == "codex_hooks" { + // Deprecated name — dropping it silences the red + // banner; the modern flag below keeps hooks enabled. + continue; + } + if key == "hooks" { out.push_str(target_line); out.push('\n'); - } else { - out.push_str(line); + wrote_flag = true; + continue; } } + out.push_str(line); + } + if wrote_flag { return out; } - // 2. If `[features]` exists, insert immediately after the header. - if existing.contains("[features]") { - let mut out = String::with_capacity(existing.len() + target_line.len() + 1); - for line in existing.split_inclusive('\n') { - out.push_str(line); - if line.trim() == "[features]" { - out.push_str(target_line); - out.push('\n'); + // Pass 2: no hooks line existed. Insert right after the + // [features] header when there is one. + if has_features_header { + let src = out; + let mut rebuilt = String::with_capacity(src.len() + target_line.len() + 1); + for line in src.split_inclusive('\n') { + rebuilt.push_str(line); + if !wrote_flag && line.trim() == "[features]" { + if !line.ends_with('\n') { + rebuilt.push('\n'); + } + rebuilt.push_str(target_line); + rebuilt.push('\n'); + wrote_flag = true; } } - return out; + return rebuilt; } - // 3. Neither exists — append a fresh `[features]` block. - let mut out = existing.to_string(); - if !out.is_empty() && !out.ends_with('\n') { - out.push('\n'); + // Pass 3: no [features] section at all — append a fresh block. + let mut appended = out; + if !appended.is_empty() && !appended.ends_with('\n') { + appended.push('\n'); } - out.push_str("\n[features]\n"); - out.push_str(target_line); - out.push('\n'); - out + appended.push_str("\n[features]\n"); + appended.push_str(target_line); + appended.push('\n'); + appended } /* ---------- Gemini ---------- */ @@ -1359,8 +1470,62 @@ mod tests { classify_in_turn(Provider::Codex, "Stop", ""), Some(SessionStatus::Idle) ); - // Codex never emits SessionEnd — PID monitor synthesizes it. - assert_eq!(classify_in_turn(Provider::Codex, "SessionEnd", ""), None); + // Newer Codex CLIs DO emit SessionEnd — instant eviction, + // no waiting on the PID monitor. (Older builds still rely on + // the watchdog to synthesize it.) + assert_eq!( + classify_in_turn(Provider::Codex, "SessionEnd", ""), + Some(SessionStatus::Ended) + ); + } + + /// Codex tool events mirror Claude's: Working inside a user turn + /// (keeps the spinner lit and last_tool fresh → "Codex is using + /// X"), dropped outside one so startup housekeeping can't flash + /// the spinner. + #[test] + fn classify_codex_tool_events_gated_on_turn() { + for ev in ["PreToolUse", "PostToolUse", "PostToolUseFailure"] { + assert_eq!( + classify_in_turn(Provider::Codex, ev, ""), + Some(SessionStatus::Working), + "codex {ev} mid-turn must keep Working" + ); + assert_eq!( + classify_event(Provider::Codex, ev, "", false), + None, + "codex {ev} outside a turn must be dropped" + ); + } + } + + /// Codex compaction gets the same distinct state as Claude/Gemini, + /// preserving the surrounding turn flag in both directions. + #[test] + fn classify_codex_compaction_preserves_turn() { + assert_eq!( + classify_event(Provider::Codex, "PreCompact", "", true), + Some((SessionStatus::Compacting, true)) + ); + assert_eq!( + classify_event(Provider::Codex, "PreCompact", "", false), + Some((SessionStatus::Compacting, false)) + ); + assert_eq!( + classify_event(Provider::Codex, "PostCompact", "", true), + Some((SessionStatus::Idle, true)) + ); + } + + /// idle_prompt is Codex noting the user has gone quiet — not a + /// question. Same carve-out as Claude: park to Idle AND clear the + /// turn so a stray later tool event can't re-light the spinner. + #[test] + fn classify_codex_idle_prompt_clears_turn() { + assert_eq!( + classify_event(Provider::Codex, "Notification", "idle_prompt", true), + Some((SessionStatus::Idle, false)) + ); } #[test] @@ -1659,23 +1824,59 @@ mod tests { fn upsert_codex_feature_flag_into_empty() { let out = upsert_codex_feature_flag(""); assert!(out.contains("[features]")); - assert!(out.contains("codex_hooks = true")); + assert!(out.contains("hooks = true")); + } + + /// Codex 0.144 deprecated `codex_hooks` — hooks.json entries no + /// longer run under it and its presence prints a red banner on + /// every codex launch. The upsert must migrate the old flag to + /// the modern `hooks = true`, not keep both. + #[test] + fn upsert_codex_feature_flag_migrates_deprecated_name() { + let prior = "[features]\ncodex_hooks = true\njs_repl = false\n"; + let out = upsert_codex_feature_flag(prior); + assert!(out.contains("hooks = true")); + assert!(!out.contains("codex_hooks")); + assert!(out.contains("js_repl = false")); + } + + /// Both keys present (the state a 0.144 user lands in after + /// following the deprecation banner's advice while our installer + /// keeps re-adding the old one): keep exactly one `hooks = true`. + #[test] + fn upsert_codex_feature_flag_dedupes_both_keys() { + let prior = "[features]\nhooks = true\ncodex_hooks = true\n"; + let out = upsert_codex_feature_flag(prior); + assert_eq!(out.matches("hooks = true").count(), 1); + assert!(!out.contains("codex_hooks")); + } + + /// A `hooks` key in a DIFFERENT table must survive untouched — + /// the migration is scoped to [features]. + #[test] + fn upsert_codex_feature_flag_ignores_other_sections() { + let prior = "[tui]\nhooks = false\n\n[features]\njs_repl = false\n"; + let out = upsert_codex_feature_flag(prior); + assert!(out.contains("[tui]\nhooks = false")); + let features_pos = out.find("[features]").unwrap(); + let flag_pos = out.rfind("hooks = true").unwrap(); + assert!(flag_pos > features_pos); } #[test] fn upsert_codex_feature_flag_idempotent() { let once = upsert_codex_feature_flag(""); let twice = upsert_codex_feature_flag(&once); - assert_eq!(once.matches("codex_hooks = true").count(), 1); - assert_eq!(twice.matches("codex_hooks = true").count(), 1); + assert_eq!(once.matches("hooks = true").count(), 1); + assert_eq!(twice.matches("hooks = true").count(), 1); } #[test] fn upsert_codex_feature_flag_rewrites_existing() { - let prior = "[features]\ncodex_hooks = false\nother = 1\n"; + let prior = "[features]\nhooks = false\nother = 1\n"; let out = upsert_codex_feature_flag(prior); - assert!(out.contains("codex_hooks = true")); - assert!(!out.contains("codex_hooks = false")); + assert!(out.contains("hooks = true")); + assert!(!out.contains("hooks = false")); assert!(out.contains("other = 1")); } @@ -1683,13 +1884,55 @@ mod tests { fn upsert_codex_feature_flag_inserts_under_existing_features() { let prior = "[features]\nother = 1\n"; let out = upsert_codex_feature_flag(prior); - assert!(out.contains("codex_hooks = true")); + assert!(out.contains("hooks = true")); assert!(out.contains("other = 1")); let header_pos = out.find("[features]").unwrap(); - let flag_pos = out.find("codex_hooks = true").unwrap(); + let flag_pos = out.find("hooks = true").unwrap(); assert!(flag_pos > header_pos); } + /// The installer must register every event the Codex arm of + /// classify_event understands — a classified-but-never-installed + /// event is a dead path (the original "codex Notification never + /// parks the spinner" bug). + #[test] + fn codex_hooks_json_registers_full_event_roster() { + let out = upsert_codex_hooks_json( + json!({}), + Path::new("/tmp/goonware-codex-hook.sh"), + ); + let hooks = out + .get("hooks") + .and_then(|h| h.as_object()) + .expect("hooks object"); + for ev in [ + "SessionStart", + "UserPromptSubmit", + "PreToolUse", + "PostToolUse", + "PermissionRequest", + "Notification", + "PreCompact", + "PostCompact", + "SubagentStart", + "SubagentStop", + "Stop", + "SessionEnd", + ] { + assert!(hooks.contains_key(ev), "codex hooks.json missing {ev}"); + } + } + + /// Re-running the installer (every Goonware launch) must not + /// duplicate entries — the retain() strip keys off the script name. + #[test] + fn codex_hooks_json_upsert_is_idempotent() { + let script = Path::new("/tmp/goonware-codex-hook.sh"); + let once = upsert_codex_hooks_json(json!({}), script); + let twice = upsert_codex_hooks_json(once.clone(), script); + assert_eq!(once, twice); + } + #[test] fn running_states_classification() { assert!(SessionStatus::Working.is_running()); @@ -1895,6 +2138,65 @@ mod tests { assert!(!should_drop_envelope_for(&envelope_from_json(empty), "1234")); } + /// The tab-subtitle summarizer (`latest_prompt_for_cwd`) and the + /// Notification idle_prompt/question split both depend on every + /// provider's script forwarding these envelope fields. Claude had + /// them from day one; codex/gemini regressing to a prompt-less + /// envelope silently blanks their tab subtitles. + #[test] + fn hook_scripts_forward_prompt_and_aux() { + for (name, body) in [ + ("claude", CLAUDE_HOOK_SCRIPT), + ("codex", CODEX_HOOK_SCRIPT), + ("gemini", GEMINI_HOOK_SCRIPT), + ] { + assert!( + body.contains("'prompt'"), + "{name} hook script must forward the user's prompt text" + ); + assert!( + body.contains("notification_type"), + "{name} hook script must forward notification_type as aux" + ); + } + } + + /// THE bug that killed the codex spinner: the codex hook script + /// sends BOTH `agent_process_id` and the legacy `codex_process_id` + /// (so old and new Rust builds each find the name they know). + /// When the struct declared the legacy name as a serde ALIAS of + /// the new one, an envelope carrying both spellings failed to + /// decode with `duplicate field agent_process_id` — and every + /// real codex envelope was rejected before classification, so no + /// codex session ever reached the spinner. The two spellings must + /// stay separate struct fields merged via `agent_pid()`. + #[test] + fn envelope_decodes_with_both_pid_spellings() { + let env = envelope_from_json(serde_json::json!({ + "provider": "codex", + "session_id": "s1", + "cwd": "/tmp/x", + "event": "UserPromptSubmit", + "agent_process_id": 4242, + "codex_process_id": 4242, + "goonware_session_id": "w_x", + })); + assert_eq!(env.agent_pid(), Some(4242)); + } + + /// Old scripts (pre-rename) send only the legacy name — the merge + /// accessor must still surface it for the liveness watchdog. + #[test] + fn envelope_pid_falls_back_to_legacy_name() { + let env = envelope_from_json(serde_json::json!({ + "provider": "codex", + "session_id": "s1", + "event": "Stop", + "codex_process_id": 777, + })); + assert_eq!(env.agent_pid(), Some(777)); + } + /// Sanity check on the script-level guard: the bundled hook /// scripts must exit early when `GOONWARE_HELPER_AGENT` is set. The /// script-level skip is the primary defense (no socket write at diff --git a/src/lib/claudeUsage.test.ts b/src/lib/claudeUsage.test.ts index 7e5d862..9c5a9e6 100644 --- a/src/lib/claudeUsage.test.ts +++ b/src/lib/claudeUsage.test.ts @@ -1,5 +1,6 @@ import { describe, expect, test } from "bun:test"; import { + detectAgentBanner, detectClaude, formatDuration, formatTokenCount, @@ -32,6 +33,32 @@ describe("detectClaude", () => { }); }); +describe("detectAgentBanner", () => { + test("classifies the Claude banner", () => { + expect(detectAgentBanner("│ ✻ Welcome to Claude")).toBe("claude"); + expect(detectAgentBanner("Claude Code v1.2.3")).toBe("claude"); + }); + + test("classifies the Codex banner case-insensitively", () => { + expect(detectAgentBanner("OpenAI Codex (v0.42.0)")).toBe("codex"); + expect(detectAgentBanner(">_ openai codex")).toBe("codex"); + }); + + test("classifies the Gemini banner", () => { + expect(detectAgentBanner("Tips for getting started:")).toBe("gemini"); + }); + + test("returns null for plain shell output", () => { + expect(detectAgentBanner("$ ls -la\ntotal 42")).toBe(null); + expect(detectAgentBanner("")).toBe(null); + }); + + test("the bare CLI name is not a banner", () => { + expect(detectAgentBanner("codex")).toBe(null); + expect(detectAgentBanner("gemini")).toBe(null); + }); +}); + describe("formatTokenCount", () => { test("under 1k stays as a plain integer", () => { expect(formatTokenCount(0)).toBe("0"); diff --git a/src/lib/claudeUsage.ts b/src/lib/claudeUsage.ts index 1594386..4b9d77c 100644 --- a/src/lib/claudeUsage.ts +++ b/src/lib/claudeUsage.ts @@ -73,6 +73,14 @@ const CLAUDE_MARKERS = [ "✻ welcome", ]; +/** Codex CLI's startup banner prints "OpenAI Codex (vX.Y.Z)". */ +const CODEX_MARKERS = ["openai codex"]; + +/** Gemini CLI greets with a tips block under its ASCII-art banner. */ +const GEMINI_MARKERS = ["welcome to gemini", "tips for getting started"]; + +export type AgentBannerCli = "claude" | "codex" | "gemini"; + /** * Returns true when `text` contains a confident marker that Claude is * running in this PTY. Used by BlockTerminal for UI-mode switching — @@ -84,6 +92,22 @@ export function detectClaude(text: string): boolean { return CLAUDE_MARKERS.some((m) => lower.includes(m)); } +/** + * Provider-agnostic banner sniff: classify which agent CLI's startup + * banner appears in `text`, or null when none does. Same contract as + * {@link detectClaude} but covers every agent in the roster, so a + * codex/gemini launched through a wrapper script (where the command + * line never says "codex") still flips the pane into agent mode. + */ +export function detectAgentBanner(text: string): AgentBannerCli | null { + if (!text) return null; + const lower = text.toLowerCase(); + if (CLAUDE_MARKERS.some((m) => lower.includes(m))) return "claude"; + if (CODEX_MARKERS.some((m) => lower.includes(m))) return "codex"; + if (GEMINI_MARKERS.some((m) => lower.includes(m))) return "gemini"; + return null; +} + export interface ModelBreakdown { messages: number; input_tokens: number; diff --git a/src/terminal/AgentChrome.tsx b/src/terminal/AgentChrome.tsx index 5e8b700..05a5692 100644 --- a/src/terminal/AgentChrome.tsx +++ b/src/terminal/AgentChrome.tsx @@ -8,6 +8,16 @@ interface Props { * agent that cd'd into a subdirectory still lights this chrome. */ cwd?: string; + /** + * CLI detected from the pane's command line, shown while no hook + * session exists yet (the window between launching the agent and + * its first SessionStart event). Keeping the strip mounted during + * that window keeps its 32px height invariant for the whole + * agent-mode lifetime — the PTY-dimension reserve in BlockTerminal + * never has to reflow when the first event lands. `null` renders a + * generic "agent" badge (aider and friends have no hook system). + */ + pendingCli?: SessionRecord["provider"] | null; } /** @@ -49,11 +59,45 @@ export const AGENT_CHROME_HEIGHT_PX = 32; * Visual reference: Warp's `use_agent_footer` panel * (/tmp/warp-check/app/src/terminal/view/use_agent_footer/mod.rs). */ -export function AgentChrome({ cwd }: Props) { +export function AgentChrome({ cwd, pendingCli }: Props) { const session = useAgentSessionForCwd(cwd ?? ""); - if (!session) return null; - if (session.status === "ended") return null; + if (!session || session.status === "ended") { + // No hook session (yet). When the caller told us which agent is + // launching, hold the strip's slot with a quiet "starting" state + // instead of unmounting — see the pendingCli prop doc. + return ; + } + + return ( + + + + + {session.status === "waiting" && ( + + + permission requested + + )} + + ); +} +/** + * Shared 32px strip shell. BOTH the live and pending states render + * through this so the pinned-height contract lives in exactly one + * place and can't drift between them. + */ +function StripShell({ children }: { children: React.ReactNode }) { return (
- - - - {session.status === "waiting" && ( + {children} +
+ ); +} + +/** + * Pre-session state: the agent was just launched and hasn't fired its + * first hook yet (or has no hook system at all — aider). Occupies the + * same 32px slot so the PTY grid below never reflows when the real + * session record arrives. + */ +function PendingStrip({ cli }: { cli: SessionRecord["provider"] | null }) { + return ( + + {cli ? ( + + ) : ( - - permission requested + agent )} - + starting… + ); } diff --git a/src/terminal/BlockTerminal.tsx b/src/terminal/BlockTerminal.tsx index 18ae77b..dc93440 100644 --- a/src/terminal/BlockTerminal.tsx +++ b/src/terminal/BlockTerminal.tsx @@ -28,7 +28,8 @@ import { setTerminalRunning, clearTerminalRunning, } from "./terminalActivityStore"; -import { detectClaude } from "@/lib/claudeUsage"; +import { detectAgentBanner } from "@/lib/claudeUsage"; +import { AgentChrome, AGENT_CHROME_HEIGHT_PX } from "./AgentChrome"; import { termKillForeground, termResetGrid } from "@/lib/tauri/term"; import { writeClipboardTextWithFallback } from "./clipboardWrite"; import { decideSoftResetAction } from "./softReset"; @@ -375,39 +376,37 @@ export function BlockTerminal({ return () => unregisterTerminalFocus(id); }, [id]); - // While a Claude-Code session is foregrounded, the launch command - // ("claude") tells you nothing about what's actually happening. Ask - // the helper-agent layer to summarize the last 3 turns of the - // transcript — that lands a phrase like "wiring up the OSC 133 - // segmenter" instead. The Rust side caches the result keyed by the - // turn uuids, so polling here is cheap unless a new exchange landed. - // - // Codex / Gemini have their own transcript layouts; for now we only - // run this against Claude transcripts (the only one the helper - // currently knows how to parse). Other CLIs fall back to the launch - // command via the activeCommand path below. - const [claudeSummary, setClaudeSummary] = useState(null); + // While an agent session is foregrounded, the launch command + // ("claude" / "codex" / "gemini") tells you nothing about what's + // actually happening. Poll the hook-fed prompt map for the user's + // latest prompt — that lands a phrase like "wiring up the OSC 133 + // segmenter" instead. The Rust command (`claude_activity_summary`, + // named for its origin) is provider-agnostic: it reads the + // in-memory LATEST_PROMPT_BY_CWD map keyed by cwd, which every + // provider's hook script populates via its UserPromptSubmit + // `prompt` field. No transcript files are read — same TCC-safety + // contract for all three CLIs. + const [agentSummary, setAgentSummary] = useState(null); useEffect(() => { if (!autoSummarize) { - setClaudeSummary(null); + setAgentSummary(null); return; } if (!foregroundIsAgent) { - setClaudeSummary(null); + setAgentSummary(null); return; } if (!cwd) return; - const isClaudeLine = commandLineIsAgent(activeCommand || command) - && detectCliFromCommandLine(activeCommand || command) === "claude"; - if (!isClaudeLine) return; + const cli = detectCliFromCommandLine(activeCommand || command); + if (!cli) return; let cancelled = false; const tick = async () => { try { const summary = await invoke("claude_activity_summary", { projectCwd: cwd, - cli: "claude", + cli, }); - if (!cancelled) setClaudeSummary(summary); + if (!cancelled) setAgentSummary(summary); } catch { // Transient failures keep the last value — better than blanking. } @@ -432,14 +431,14 @@ export function BlockTerminal({ // 2. **Bare CLI launch names don't dispatch either.** When the // tab remounts (e.g. on tab switch), `activeCommand` is // seeded back to the agent's name ("claude" / "codex" / - // "gemini") before `claudeSummary` has had a chance to tick. + // "gemini") before `agentSummary` has had a chance to tick. // Without this guard the stored summary would briefly flip // to "claude" and then get rewritten with the real activity // summary a moment later — a visible flicker the user // specifically called out. Letting the prior real summary // persist until a real new one arrives is the right default. useEffect(() => { - const source = claudeSummary ?? activeCommand; + const source = agentSummary ?? activeCommand; const summary = source.replace(/\s+/g, " ").trim(); if (!summary) return; const lower = summary.toLowerCase(); @@ -447,7 +446,7 @@ export function BlockTerminal({ return; } onActivitySummaryChangeRef.current?.(summary); - }, [activeCommand, claudeSummary]); + }, [activeCommand, agentSummary]); const { blocks, @@ -1425,10 +1424,14 @@ export function BlockTerminal({ // the terminal grid (38px + a 6px breathing strip = 44). const inputChrome = agentMode ? 44 : 80; const liveBlockChrome = 50; - // The agent status strip that used to sit above the canvas was - // removed, so there's no chrome height to reserve here anymore — - // the agent's PTY reclaims that space and fills the pane. - const agentChromeHeight = 0; + // The AgentChrome strip is mounted whenever a known agent CLI + // is foregrounded (same flag as the JSX mount below — keep them + // in sync or the PTY either clips its bottom row under the + // input box or leaves a 32px black stripe). The pendingCli + // fallback inside AgentChrome guarantees the strip renders for + // the whole foregroundIsAgent lifetime, so this reserve is + // exact, not conditional on hook events having arrived. + const agentChromeHeight = foregroundIsAgent ? AGENT_CHROME_HEIGHT_PX : 0; // On a NATIVE agent pane the only real chrome is the AgentChrome strip: // the input bar is hidden and the live block is opacity:0, so reserving // their heights (inputChrome + liveBlockChrome) would shrink Claude's PTY @@ -1622,25 +1625,26 @@ export function BlockTerminal({ // CRITICAL: only sniff while a command is actively running. After // Ctrl+C kills an agent the alacritty grid still holds the agent's // TUI bytes — without this gate, the very next frame after the - // command_running=false transition would re-detect claude from + // command_running=false transition would re-detect the agent from // those leftover bytes and flip foregroundIsAgent back to true, // pinning PromptInput off-screen forever. // // AND: skip the sniff entirely once we know what command is running // and it isn't an agent. The live frame contains the full grid (per - // useTerminalSession's allDirty re-emit), so claude's banner from + // useTerminalSession's allDirty re-emit), so the agent's banner from // the previous run is still painted above the shell prompt when the // user types `ls`. Without this gate, that stale banner trips - // detectClaude on the next `command_running=true` transition, + // detectAgentBanner on the next `command_running=true` transition, // re-arms agent mode, and the prompt input vanishes mid-typing — // exactly the bug the user reported. activeCommand-classification // (line below) already covers the case where the new command IS an // agent, so suppressing the sniff here loses nothing. // - // Scans the full grid (claude's banner paints near the top of the - // initial draw, so a tail-only scan misses it). Bails on the first - // marker hit — the inner loop appends span text and short-circuits - // as soon as detectClaude succeeds. + // Scans the full grid (the banners paint near the top of the + // initial draw, so a tail-only scan misses them). Covers every CLI + // in the roster — claude, codex, gemini — so an agent launched via + // a wrapper script (command line never names the binary) still + // flips the pane into agent mode. useEffect(() => { if (foregroundIsAgent) return; // Suppressed after an explicit force-kill until the next submit — see @@ -1660,9 +1664,12 @@ export function BlockTerminal({ sniffBufferRef.current.length + text.length > 16_384 ? (sniffBufferRef.current + text).slice(-16_384) : sniffBufferRef.current + text; - if (detectClaude(sniffBufferRef.current)) { + const bannerCli = detectAgentBanner(sniffBufferRef.current); + if (bannerCli) { setForegroundIsAgent(true); - if (!claudeDetectedLocal) { + // The detected-callback anchors the Claude 5h-usage pill; the + // other CLIs have no usage surface, so only Claude fires it. + if (bannerCli === "claude" && !claudeDetectedLocal) { setClaudeDetectedLocal(true); onClaudeDetectedRef.current?.(Date.now()); } @@ -1671,10 +1678,11 @@ export function BlockTerminal({ }, [liveFrame, foregroundIsAgent, claudeDetectedLocal, activeCommand]); // Foreground the agent the moment the user runs one from the shell. - // The Claude-only banner sniff above is a slow path that doesn't - // know about codex/aider; this catches every known agent on its - // command line as soon as command_running flips to true. Skipped - // for direct-launch panes (already foregrounded at mount). + // The banner sniff above is a slow path (it waits for the TUI's + // first paint, and doesn't know aider); this catches every known + // agent on its command line as soon as command_running flips to + // true. Skipped for direct-launch panes (already foregrounded at + // mount). useEffect(() => { if (directAgent) return; if (foregroundIsAgent) return; @@ -2382,9 +2390,24 @@ export function BlockTerminal({ )} - {/* The Warp-style agent status strip ("claude is idle ✓") was - removed — it restated what the agent's own TUI already shows - and just ate vertical space above the pane. */} + {/* Warp-style agent status strip: hook-driven spinner + "Codex is + using Read" + the waiting-on-permission pill. An earlier pass + removed it as redundant with the agent's own TUI, but the TUI + shows nothing when the pane is scrolled away from the input + box or the agent silently waits on a permission prompt — the + strip is the one always-visible truth. Mounted ONLY for known + agent CLIs (foregroundIsAgent), never for vim/fzf/alt-screen + — deriveInputMode's agentMode is deliberately NOT the gate + here. computeDims reserves AGENT_CHROME_HEIGHT_PX under the + same flag, and the pendingCli fallback keeps the strip's + 32px present from launch, so the PTY grid never reflows when + the first hook event lands. */} + {foregroundIsAgent && ( + + )} {/* Alt-screen TUIs (vim, htop, claude-in-alt-screen) render on the native Metal surface. The previous React WebGPU CanvasGrid From 6ac46c6f194be68af82dfc41d8c39958e216913a Mon Sep 17 00:00:00 2001 From: raeedz Date: Sat, 11 Jul 2026 15:46:28 -0700 Subject: [PATCH 2/2] Fix session eviction and case-only file renames Preserve live peer sessions, handle root cwd safely, and update README. --- README.md | 3 +- src-tauri/src/agent_hooks.rs | 123 +++++++++++++++++++++++++-- src-tauri/src/fs.rs | 42 ++++++++- src-tauri/src/pr.rs | 62 ++++++++++++-- src/shell/MainColumn.tsx | 50 ----------- src/state/agentActivityStore.test.ts | 43 +++------- src/state/agentActivityStore.ts | 46 +++------- src/state/types.ts | 2 +- src/terminal/BlockTerminal.tsx | 26 +++--- src/todo/TodoView.tsx | 71 +++++++++++----- 10 files changed, 298 insertions(+), 170 deletions(-) diff --git a/README.md b/README.md index 3015179..6b41a6b 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,7 @@ **GPU-accelerated terminal for running CLI coding agents in parallel.** -No chat panel. No wrapper around your agent. Just a fast, dark workspace where every `claude` / `codex` session lives in its own git worktree, every commit can be AI-drafted, and the browser, files, and git panels are one keystroke away. +No chat panel. No wrapper around your agent. Just a fast, dark workspace where every `claude` / `codex` session lives in its own git worktree, every commit can be AI-drafted, and the files and git panels are one keystroke away. > Built with Tauri (Rust) + React/TypeScript. xterm.js + WebGL for the terminal. macOS only in v1. @@ -15,7 +15,6 @@ A native macOS app for orchestrating CLI coding agents the way you already think - **No harness, just a terminal.** Your agent runs in a real PTY with xterm.js + WebGL. Nothing in between you and the model. - **Every agent gets its own git worktree.** Spawn five `claude` sessions on five branches and they never step on each other. - **Live, plain-English summaries.** Every tab carries a one-line summary of what the agent is doing right now, auto-drafted when the PTY goes idle. -- **In-house browser daemon.** A headless Chrome at `127.0.0.1:4000` exposes `/screenshot`, `/navigate`, `/click`, `/type`, `/console/recent`. Faster than Chrome MCP. The same HTTP contract works from any agent terminal. - **AI commits and AI PRs.** Stage changes, hit `⌘⏎`, get a Gemini Flash-Lite draft. `⌘⌥P` drafts a PR title + body and ships it via `gh`. - **Highlight → ask.** Select code, press `⌘L`, get an inline answer in the margin. No side panel. No thread. - **Per-project memory.** `rli-memory add` / `recall` from any pane. Auto-scoped to the active worktree. Multiple agents in parallel panes coordinate without a scratch file. diff --git a/src-tauri/src/agent_hooks.rs b/src-tauri/src/agent_hooks.rs index 1436c62..89d51b1 100644 --- a/src-tauri/src/agent_hooks.rs +++ b/src-tauri/src/agent_hooks.rs @@ -698,16 +698,9 @@ fn handle_connection(mut stream: UnixStream, app: &AppHandle) { if is_new_session { let mut to_remove: Vec = Vec::new(); for (other_key, rec) in sessions.iter() { - if rec.provider != provider { - continue; + if should_evict_stale(rec, provider, &envelope.session_id, &envelope.cwd) { + to_remove.push(other_key.clone()); } - if rec.session_id == envelope.session_id { - continue; - } - if !cwds_overlap(&rec.cwd, &envelope.cwd) { - continue; - } - to_remove.push(other_key.clone()); } for other_key in to_remove { if let Some(mut stale) = sessions.remove(&other_key) { @@ -799,11 +792,61 @@ fn cwds_overlap(a: &str, b: &str) -> bool { if a_trim == b_trim { return true; } + // Root / degenerate case: "/" trims to "". Without this guard the + // prefix below becomes "/" and matches EVERY absolute path — one + // session with cwd "/" would evict all same-provider sessions + // everywhere. Root overlaps only root (handled by the equality + // check above), so bail here. + if a_trim.is_empty() || b_trim.is_empty() { + return false; + } let a_prefix = format!("{}/", a_trim); let b_prefix = format!("{}/", b_trim); b_trim.starts_with(&a_prefix) || a_trim.starts_with(&b_prefix) } +/// Should this existing session record be evicted (with a synthetic +/// Ended emit) when a NEW session key appears for `new_provider` at +/// `new_cwd`? +/// +/// The stale-eviction sweep exists to clear leftover records from a +/// relaunched CLI whose old process never sent SessionEnd. But two +/// LIVE same-provider sessions in one worktree are a legitimate state +/// (two panes running claude side by side), and evicting a live peer +/// caused an eviction ping-pong: each session's next event found its +/// own key missing (its peer had just evicted it), re-registered as +/// "new," and evicted the peer right back — spinner flicker and wiped +/// permission state on every event, forever. +/// +/// So a record whose agent PID is known AND alive is never evicted +/// here; only records with no PID or a dead PID keep the legacy +/// eviction behavior. A never-evicted peer never re-registers as +/// "new," which structurally ends the ping-pong. +fn should_evict_stale( + rec: &SessionRecord, + new_provider: Provider, + new_session_id: &str, + new_cwd: &str, +) -> bool { + if rec.provider != new_provider { + return false; + } + if rec.session_id == new_session_id { + return false; + } + if !cwds_overlap(&rec.cwd, new_cwd) { + return false; + } + // Live peer — coexist instead of evicting. Dead or PID-less + // records fall through and are evicted as before. + if let Some(pid) = rec.agent_process_id { + if pid_alive(pid) { + return false; + } + } + true +} + /// True iff a process with this PID is currently in the kernel's /// process table. Uses `kill(pid, 0)` — the canonical Unix liveness /// idiom. `ESRCH` = dead; `EPERM` = alive but inaccessible (treated @@ -2325,4 +2368,66 @@ mod tests { assert!(!cwds_overlap("/Users/me/proj", "")); assert!(!cwds_overlap("", "")); } + + #[test] + fn cwds_overlap_root_only_matches_root() { + assert!(cwds_overlap("/", "/")); + assert!(!cwds_overlap("/", "/Users/me/proj")); + assert!(!cwds_overlap("/Users/me/proj", "/")); + } + + #[test] + fn stale_eviction_preserves_live_peers_and_rejects_unrelated_records() { + let mut record = SessionRecord { + provider: Provider::Claude, + session_id: "old".into(), + cwd: "/Users/me/proj".into(), + status: SessionStatus::Working, + last_event: "UserPromptSubmit".into(), + last_tool: String::new(), + agent_process_id: Some(std::process::id() as i32), + updated_at_ms: 1, + }; + + assert!(!should_evict_stale( + &record, + Provider::Claude, + "new", + "/Users/me/proj" + )); + assert!(!should_evict_stale( + &record, + Provider::Codex, + "new", + "/Users/me/proj" + )); + assert!(!should_evict_stale( + &record, + Provider::Claude, + "old", + "/Users/me/proj" + )); + assert!(!should_evict_stale( + &record, + Provider::Claude, + "new", + "/Users/me/other" + )); + + record.agent_process_id = None; + assert!(should_evict_stale( + &record, + Provider::Claude, + "new", + "/Users/me/proj/src" + )); + + record.agent_process_id = Some(-1); + assert!(should_evict_stale( + &record, + Provider::Claude, + "new", + "/Users/me/proj" + )); + } } diff --git a/src-tauri/src/fs.rs b/src-tauri/src/fs.rs index c805a74..60d6642 100644 --- a/src-tauri/src/fs.rs +++ b/src-tauri/src/fs.rs @@ -579,7 +579,13 @@ pub fn fs_rename(from: String, to: String) -> Result { if from_path == to_path { return Ok(to); } - if to_path.exists() { + // On macOS's default case-insensitive APFS, a case-only rename + // ('readme.md' → 'README.md') makes `to` resolve to the SOURCE + // file's own inode, so `exists()` is true even though nothing + // distinct would be clobbered. If `from` and `to` are the same + // underlying file, fall through — `fs::rename` performs the + // in-place case change. Genuinely distinct targets still error. + if to_path.exists() && !same_file(from_path, to_path) { let name = to_path .file_name() .map(|n| n.to_string_lossy().into_owned()) @@ -594,6 +600,28 @@ pub fn fs_rename(from: String, to: String) -> Result { Ok(to) } +/// True when `a` and `b` name the same underlying file (same device + +/// inode). Uses `symlink_metadata` so a symlink compares as the link +/// entry itself rather than its target. This is what lets `fs_rename` +/// distinguish a case-only rename on a case-insensitive volume from a +/// genuine collision with a different file. +#[cfg(unix)] +fn same_file(a: &Path, b: &Path) -> bool { + use std::os::unix::fs::MetadataExt; + match (fs::symlink_metadata(a), fs::symlink_metadata(b)) { + (Ok(ma), Ok(mb)) => ma.dev() == mb.dev() && ma.ino() == mb.ino(), + _ => false, + } +} + +/// Non-unix fallback: no inode identity available, so never claim two +/// distinct paths are the same file (preserves the strict no-clobber +/// behavior). The app only ships on macOS, so this is belt-and-braces. +#[cfg(not(unix))] +fn same_file(_a: &Path, _b: &Path) -> bool { + false +} + /// Permanently delete a file or directory. Powers the file tree's /// right-click → Delete; the frontend gates it behind a confirm dialog /// because this is irreversible (no Trash round-trip — moving to @@ -853,6 +881,18 @@ mod tests { .unwrap_err(); assert!(err.contains("already exists"), "got: {err}"); assert_eq!(fs::read_to_string(&other).unwrap(), "keep"); + + // A case-only rename must not be mistaken for a collision on + // case-insensitive filesystems (the default on macOS). + let lower = dir.path().join("case-name.txt"); + let upper = dir.path().join("CASE-NAME.txt"); + fs::write(&lower, b"same file").unwrap(); + fs_rename( + lower.to_string_lossy().into_owned(), + upper.to_string_lossy().into_owned(), + ) + .unwrap(); + assert_eq!(fs::read_to_string(&upper).unwrap(), "same file"); } #[test] diff --git a/src-tauri/src/pr.rs b/src-tauri/src/pr.rs index dd0e710..789713d 100644 --- a/src-tauri/src/pr.rs +++ b/src-tauri/src/pr.rs @@ -736,7 +736,9 @@ async fn enter_review_state(cwd: &str, base: &str) -> Option { /// an ancestor of `head_sha` (the soft reset is still in effect); if /// the user or an agent committed on top, resetting would drop those /// commits from the branch, so we leave everything alone and report -/// false. +/// false. Errs when the ancestry check itself fails (e.g. `head_sha` +/// no longer resolves after a force-push + prune) — callers must treat +/// that as "do not touch the worktree", not as a benign skip. async fn restore_review_state(cwd: &str, head_sha: &str) -> Result { let head = run_git_checked(cwd, &["rev-parse", "HEAD"]) .await? @@ -745,16 +747,28 @@ async fn restore_review_state(cwd: &str, head_sha: &str) -> Result if head == head_sha { return Ok(false); } - let ancestor = Command::new("git") + let out = Command::new("git") .args(["merge-base", "--is-ancestor", "HEAD", head_sha]) .current_dir(cwd) .output() .await - .map_err(|e| format!("spawn git: {e}"))? - .status - .success(); - if !ancestor { - return Ok(false); + .map_err(|e| format!("spawn git: {e}"))?; + match out.status.code() { + // HEAD is an ancestor — the soft reset is still in effect. + Some(0) => {} + // Definitive "not an ancestor": someone committed on top — + // resetting would drop those commits, so leave everything be. + Some(1) => return Ok(false), + // Anything else (e.g. 128: head_sha unresolvable after a + // force-push + prune) means git couldn't answer the question. + // Neither resetting nor pretending "user committed on top" is + // safe, so surface it and let the caller abort. + _ => { + return Err(format!( + "git merge-base --is-ancestor HEAD {head_sha} failed: {}", + String::from_utf8_lossy(&out.stderr).trim() + )); + } } run_git_checked(cwd, &["reset", "--soft", head_sha]).await?; Ok(true) @@ -1824,6 +1838,40 @@ mod tests { assert_eq!(head_after.trim(), new_head, "takeover commit must survive"); } + #[tokio::test] + async fn checkout_return_aborts_when_head_sha_is_unresolvable() { + // A force-push + prune can leave the recorded head sha pointing + // at nothing. That is NOT the "user committed on top" case: + // proceeding would check out the original branch with the whole + // PR diff still staged. The return must abort and leave the + // review checkout exactly as it was. + let (clone, _bare, _pr_head) = build_pr_branch_repo(); + let cwd = clone.path().to_str().unwrap(); + enter_review_state(cwd, "main").await.expect("enter"); + + let bogus = "0123456789abcdef0123456789abcdef01234567"; + let err = pr_checkout_return( + cwd.to_string(), + "main".to_string(), + false, + Some(bogus.to_string()), + ) + .await + .expect_err("unresolvable head sha must abort the return"); + assert!(err.contains(bogus), "error should name the sha: {err}"); + + // Still in the review checkout on the PR branch... + let branch = run_sync(clone.path(), &["symbolic-ref", "--short", "HEAD"]); + assert_eq!(branch.trim(), "pr-branch", "must not switch branches"); + // ...with the PR diff still staged where it belongs — not + // carried onto main. + let staged = run_sync(clone.path(), &["diff", "--cached", "--name-only"]); + assert!( + staged.contains("a.txt") && staged.contains("b.txt"), + "review state must remain intact: {staged}" + ); + } + #[tokio::test] async fn checkout_return_with_head_sha_restores_branch_before_switching() { let (clone, _bare, pr_head) = build_pr_branch_repo(); diff --git a/src/shell/MainColumn.tsx b/src/shell/MainColumn.tsx index 0c23040..2962cd9 100644 --- a/src/shell/MainColumn.tsx +++ b/src/shell/MainColumn.tsx @@ -188,7 +188,6 @@ export function MainColumn() { ); } - /* ------------------------------------------------------------------ Breadcrumb ------------------------------------------------------------------ */ @@ -774,16 +773,6 @@ function TabContent({ draggingTabId: string | null; }) { const split = !!splitTab; - // TEMP DEBUG — remove before commit (live-state mirror for the render - // investigation; a module-level interval at the bottom of this file - // posts it to a local diagnostics listener). - (window as unknown as Record).__paneState = { - wt: worktree.id, - splitTabIdRaw: worktree.splitTabId ?? null, - activeTabId: worktree.activeTabId ?? null, - active: tab ? { id: tab.id, kind: tab.kind } : null, - split: splitTab ? { id: splitTab.id, kind: splitTab.kind } : null, - }; // Terminal-kind tabs go through the always-mounted keepalive // layer; non-terminal kinds (diff, markdown, all-changes, // project-settings) mount on demand. The keepalive layer is @@ -1820,42 +1809,3 @@ function MissingWorktreeView({ ); } - - - -// TEMP DEBUG — remove before commit. Posts a 2s heartbeat of the pane -// layout (state mirror + drop-zone children boxes + visibility) to a -// local diagnostics listener so pane-rendering failures can be caught -// in the exact moment they happen. -{ - const g = window as unknown as { __paneDump?: number; __paneState?: unknown }; - if (g.__paneDump) window.clearInterval(g.__paneDump); - g.__paneDump = window.setInterval(() => { - const z = document.querySelector("[data-tab-drop-zone]"); - const kids = z - ? Array.from(z.children).map((c) => { - const e = c as HTMLElement; - const r = e.getBoundingClientRect(); - const cs = getComputedStyle(e); - return { - rect: { x: r.x, y: r.y, w: r.width, h: r.height }, - vis: cs.visibility, - z: cs.zIndex, - kidCount: e.children.length, - text: (e.textContent ?? "").slice(0, 40), - }; - }) - : null; - const zr = z?.getBoundingClientRect(); - fetch("http://localhost:8787/dump", { - method: "POST", - body: JSON.stringify({ - t: new Date().toISOString(), - win: { w: window.innerWidth, h: window.innerHeight }, - zone: zr ? { x: zr.x, y: zr.y, w: zr.width, h: zr.height } : null, - state: g.__paneState ?? null, - kids, - }), - }).catch(() => {}); - }, 2000); -} diff --git a/src/state/agentActivityStore.test.ts b/src/state/agentActivityStore.test.ts index 9fdf579..bb32018 100644 --- a/src/state/agentActivityStore.test.ts +++ b/src/state/agentActivityStore.test.ts @@ -164,18 +164,15 @@ describe("forceIdleForCwd — Ctrl+C local spinner flip", () => { }); }); -describe("applyRecord — stale-session eviction on new-session edge", () => { +describe("applyRecord — backend-owned stale-session eviction", () => { /** - * The reported "loading box doesn't fire on restart" bug. A - * hard-killed Claude leaves its SessionRecord stuck at `working` in - * the map (SessionEnd never fires). When the user launches a fresh - * Claude in the same pane, the new SessionStart record joins the - * old one — the sidebar spinner reads "ANY working session = on" - * and stays lit even when the new agent is idle. Eviction on the - * new-session edge drops the stale record so the spinner reflects - * just the live agent. + * The frontend cannot tell a stale hard-killed session from a live + * peer in another pane because SessionRecord's wire shape does not + * expose enough trustworthy process-liveness information. It must + * retain both records until the Rust backend emits a synthetic Ended + * event for a session it has proved dead. */ - test("new session_id at same (provider, cwd) evicts the stale one", () => { + test("new session_id at same (provider, cwd) preserves the existing peer", () => { seedSession({ session_id: "old", cwd: "/Users/me/proj", @@ -191,11 +188,11 @@ describe("applyRecord — stale-session eviction on new-session edge", () => { last_tool: "", updated_at_ms: 2000, }); - expect(__internals.sessions.has("claude:old")).toBe(false); + expect(__internals.sessions.has("claude:old")).toBe(true); expect(__internals.sessions.has("claude:new")).toBe(true); }); - test("eviction matches when new session's cwd is a subdirectory of the stale one", () => { + test("preserves an existing peer when the new cwd is a descendant", () => { seedSession({ session_id: "old", cwd: "/Users/me/proj", @@ -211,11 +208,11 @@ describe("applyRecord — stale-session eviction on new-session edge", () => { last_tool: "", updated_at_ms: 2000, }); - expect(__internals.sessions.has("claude:old")).toBe(false); + expect(__internals.sessions.has("claude:old")).toBe(true); expect(__internals.sessions.has("claude:new")).toBe(true); }); - test("eviction matches when stale session's cwd is a subdirectory of the new one", () => { + test("preserves an existing peer when its cwd is a descendant", () => { seedSession({ session_id: "old", cwd: "/Users/me/proj/src", @@ -231,7 +228,7 @@ describe("applyRecord — stale-session eviction on new-session edge", () => { last_tool: "", updated_at_ms: 2000, }); - expect(__internals.sessions.has("claude:old")).toBe(false); + expect(__internals.sessions.has("claude:old")).toBe(true); expect(__internals.sessions.has("claude:new")).toBe(true); }); @@ -278,9 +275,7 @@ describe("applyRecord — stale-session eviction on new-session edge", () => { }); test("re-applying an event for an existing session does NOT trigger self-eviction", () => { - // The eviction is gated on `prev === null` — i.e. the session_id - // is new to the map. Without that gate, the very record we're - // applying would self-evict, blanking the state mid-update. + // Existing records are still updated in place rather than duplicated. seedSession({ session_id: "abc", cwd: "/Users/me/proj", @@ -299,18 +294,6 @@ describe("applyRecord — stale-session eviction on new-session edge", () => { expect(__internals.sessions.get("claude:abc")!.status).toBe("compacting"); }); - // NOTE: multi-pane same-(provider, cwd) is intentionally NOT - // preserved. Two simultaneous claude sessions in the same worktree - // is rare; the much more common failure mode is a stale "working" - // record sitting in the map after a hard-killed agent, which - // accidentally lights the worktree spinner for the new fresh-launch - // session. Treating same-(provider, cwd) as one logical session - // and clobbering the older record on new-session arrival is the - // explicit trade-off here. The Rust-side PID watchdog catches the - // dead-but-not-evicted case for the single-pane scenario, and - // multi-pane users can land in a worktree subdirectory to - // disambiguate (cwd prefix-matching ensures eviction still respects - // the descendant relationship). }); describe("forceEvictForCwd — SIGKILL drops sessions definitively", () => { diff --git a/src/state/agentActivityStore.ts b/src/state/agentActivityStore.ts index 5cfdf1e..fd6b4a2 100644 --- a/src/state/agentActivityStore.ts +++ b/src/state/agentActivityStore.ts @@ -107,15 +107,17 @@ function applyRecord(record: SessionRecord) { } const prev = sessions.get(key); if (!prev) { - // First event for this session_id — a fresh agent process. Evict - // any prior sessions for the same (provider, cwd) so a Claude / - // Gemini that was hard-killed before its Stop / SessionEnd hook - // fired doesn't keep the worktree spinner spinning forever. The - // PID-watchdog would also eventually catch this, but acting on - // the new-session signal makes the spinner restart deterministic: - // by the time the AgentChrome reads the most-recent session, the - // stale "working" record is gone. - evictStaleForCwd(record.provider, record.cwd, record.session_id); + // First event for this session_id — a fresh agent process. Stale + // same-(provider, cwd) cleanup is NOT done here: the frontend has + // no pid knowledge, so a client-side sweep can't tell a dead + // hard-killed session from a live peer running in a sibling pane + // of the same worktree (two agents would mutually evict each + // other's records and the spinner ping-pongs). The Rust backend + // owns that decision — on a new session's first event it evicts + // same-provider overlapping-cwd sessions only when their pid is + // missing or dead, and broadcasts each eviction as a synthetic + // Ended record over agent://session/state, which the `ended` + // branch above handles generically. sessions.set(key, record); notifyAll(); return; @@ -135,32 +137,6 @@ function applyRecord(record: SessionRecord) { notifyAll(); } -/** - * Drop every session for `(provider, cwd)` whose session_id differs - * from `keepSessionId`. Used by applyRecord on the new-session edge - * and by `forceEvictForCwd` on the SIGKILL path. Returns true iff at - * least one session was removed (caller may decide whether to bundle - * the notify with its own — applyRecord does, forceEvictForCwd does - * its own notify). - */ -function evictStaleForCwd( - provider: Provider, - cwd: string, - keepSessionId: string, -): boolean { - let removed = false; - for (const [k, rec] of sessions) { - if (rec.provider !== provider) continue; - if (rec.session_id === keepSessionId) continue; - if (!cwdMatchesWorktree(rec.cwd, cwd) && !cwdMatchesWorktree(cwd, rec.cwd)) { - continue; - } - sessions.delete(k); - removed = true; - } - return removed; -} - async function bootstrap() { if (bootstrapped) return; bootstrapped = true; diff --git a/src/state/types.ts b/src/state/types.ts index 695314c..5c43b09 100644 --- a/src/state/types.ts +++ b/src/state/types.ts @@ -297,7 +297,7 @@ export interface Worktree { /** User-assigned color tag (sidebar accent). */ color?: TagId; - /** User-picked HugeIcons component name (overrides project's choice). */ + /** User-picked picker-registry icon key (overrides project's choice). */ iconName?: string; /** Active PR-review session, if the worktree is checked out to a PR. */ prSession?: PrSession | null; diff --git a/src/terminal/BlockTerminal.tsx b/src/terminal/BlockTerminal.tsx index dc93440..4c65578 100644 --- a/src/terminal/BlockTerminal.tsx +++ b/src/terminal/BlockTerminal.tsx @@ -1740,20 +1740,22 @@ export function BlockTerminal({ // foreground process group via tcgetpgrp(master_fd) and SIGKILLs // it, bypassing whatever signal trap the running process installed. // - // Always force-EVICT here (not just force-idle). A double-tap - // escalation means the user really wants the agent dead — even if - // it traps SIGINT, the foreground process group is about to receive - // SIGKILL. The SessionEnd hook will never fire (the agent process - // is killed before it can run its at-exit handler), so the only way - // to keep the session map from accumulating a stuck "working" - // record is to drop it locally right here. Without this, the next - // time the user runs `claude` in this pane the stale record is - // still there: the sidebar spinner stays on (any working session - // counts) and the per-pane chrome can briefly show the killed - // agent's last status before the new SessionStart record arrives. + // Force-EVICT here (not just force-idle) — but ONLY when the + // foregrounded process is an agent, mirroring the single-Ctrl+C + // gate in onSendBytesVoid. A double-tap escalation means the user + // really wants the foreground process dead — even if it traps + // SIGINT, the foreground process group is about to receive SIGKILL. + // When that process IS an agent, its SessionEnd hook will never + // fire (killed before its at-exit handler runs), so drop its + // session record locally right here; otherwise the next `claude` + // launch in this pane inherits a stuck "working" record and the + // sidebar spinner stays on. When the killed process is NOT an + // agent (e.g. a stuck `npm run dev`), evicting by cwd would wipe + // the live session records of OTHER panes sharing this worktree — + // so the eviction is gated, while the kill itself is not. const onForceKill = useCallback(() => { const path = cwdRef.current; - if (path) forceEvictForCwd(path); + if (path && foregroundIsAgentRef.current) forceEvictForCwd(path); void termKillForeground(ptyId).catch(() => { // Backend may have torn the session down between the read and // the kill (rare race on tab close). Nothing useful to do; the diff --git a/src/todo/TodoView.tsx b/src/todo/TodoView.tsx index 8c2d8de..ca0ce08 100644 --- a/src/todo/TodoView.tsx +++ b/src/todo/TodoView.tsx @@ -1,4 +1,4 @@ -import { useEffect, useRef, useState } from "react"; +import { useEffect, useLayoutEffect, useRef, useState } from "react"; import { useAppDispatch } from "@/state/AppState"; import type { TodoItem, Worktree } from "@/state/types"; @@ -38,14 +38,15 @@ export function TodoView({ worktree }: { worktree: Worktree }) { latest.current = { todos, history }; // Timers for todos flashing green before they drop into History, so we can - // cancel them on unmount and avoid firing against a dead component. + // cancel them on unmount or worktree switch and avoid firing against a + // dead component or archiving one worktree's id into another worktree. const doneTimers = useRef(new Map>()); - useEffect( + useLayoutEffect( () => () => { doneTimers.current.forEach((t) => clearTimeout(t)); doneTimers.current.clear(); }, - [], + [worktree.id], ); useEffect(() => { @@ -91,6 +92,48 @@ export function TodoView({ worktree }: { worktree: Worktree }) { patch({ todos: todos.map((t) => (t.id === id ? { ...t, text } : t)) }); }; + // Move a "done" todo out of the active list and into History. Shared by + // the 900ms flash timer below and the recovery sweep effect. Always reads + // from — and synchronously advances — the `latest` mirror: if another + // archive dispatches before React commits this one back into the + // `worktree` prop, it must see this accumulated state, otherwise its + // shallow-merge patch would clobber `todoHistory` and drop the item we + // just archived. + const archiveDone = (id: string) => { + const { todos: curTodos, history: curHistory } = latest.current; + const item = curTodos.find((t) => t.id === id); + if (!item) return; + const done: TodoItem = { ...item, status: "done", completedAt: Date.now() }; + const nextTodos = curTodos.filter((t) => t.id !== id); + const nextHistory = [done, ...curHistory]; + latest.current = { todos: nextTodos, history: nextHistory }; + dispatch({ + type: "update-worktree", + id: worktree.id, + patch: { + todos: nextTodos, + todoHistory: nextHistory, + }, + }); + }; + + // Recovery sweep. A todo can be stranded in "done" forever: the status + // persists as soon as the checkbox is clicked, but the archive only + // happens on the 900ms timer — which the unmount cleanup above cancels. + // Switch worktree/tab or quit inside that window and the item comes back + // as a read-only, uncycleable "done" row that nothing will ever archive. + // On mount and whenever the worktree changes, archive such items + // immediately; they already missed their flash animation, so there is + // nothing to wait for. Items with a live timer are mid-flash and are + // left to their timer. + useEffect(() => { + for (const t of latest.current.todos) { + if (t.status === "done" && !doneTimers.current.has(t.id)) { + archiveDone(t.id); + } + } + }, [worktree.id]); + // Circle click cycles the state: todo → in_progress → done. Reaching // "done" flips the marker to a green check in place and leaves the row // sitting there for a beat before it drops out of the active list and @@ -120,25 +163,7 @@ export function TodoView({ worktree }: { worktree: Worktree }) { }); const timer = setTimeout(() => { doneTimers.current.delete(id); - const { todos: curTodos, history: curHistory } = latest.current; - const item = curTodos.find((t) => t.id === id); - if (!item) return; - const done: TodoItem = { ...item, status: "done", completedAt: Date.now() }; - const nextTodos = curTodos.filter((t) => t.id !== id); - const nextHistory = [done, ...curHistory]; - // Advance the mirror synchronously. If another todo's timer fires - // before React commits this archive back into the `worktree` prop, it - // must read this accumulated state — otherwise its shallow-merge patch - // would clobber `todoHistory` and drop the item we just archived. - latest.current = { todos: nextTodos, history: nextHistory }; - dispatch({ - type: "update-worktree", - id: worktree.id, - patch: { - todos: nextTodos, - todoHistory: nextHistory, - }, - }); + archiveDone(id); }, 900); doneTimers.current.set(id, timer); };