diff --git a/crates/agent-runtime/src/claude/hooks.rs b/crates/agent-runtime/src/claude/hooks.rs index 3d02f87..0c663d7 100644 --- a/crates/agent-runtime/src/claude/hooks.rs +++ b/crates/agent-runtime/src/claude/hooks.rs @@ -460,7 +460,7 @@ pub fn run_hook_client(socket: &Path) -> i32 { if std::io::stdin().read_to_string(&mut input).is_err() { // Nothing to decide about. Exiting 1 is a non-blocking error, which leaves // the runtime's own flow intact rather than blocking a call Tervin never saw. - eprintln!("Tervin hook: could not read the tool call from stdin."); + eprintln!("{HOOK_STDERR_PREFIX}could not read the tool call from stdin."); return 1; } @@ -470,8 +470,8 @@ pub fn run_hook_client(socket: &Path) -> i32 { // Tervin is gone. Say so loudly rather than failing silently: the user // believes actions are being gated, and right now they are not. eprintln!( - "Tervin hook: could not reach Tervin at {} ({e}). This tool call was \ - NOT checked against Tervin Rules.", + "{HOOK_STDERR_PREFIX}could not reach Tervin at {} ({e}). This tool call \ + was NOT checked against Tervin Rules.", socket.display() ); return 1; @@ -485,7 +485,7 @@ pub fn run_hook_client(socket: &Path) -> i32 { let mut payload = input.trim().replace('\n', " "); payload.push('\n'); if stream.write_all(payload.as_bytes()).is_err() || stream.flush().is_err() { - eprintln!("Tervin hook: could not send the tool call to Tervin."); + eprintln!("{HOOK_STDERR_PREFIX}could not send the tool call to Tervin."); return 1; } @@ -494,12 +494,12 @@ pub fn run_hook_client(socket: &Path) -> i32 { .is_err() || response.trim().is_empty() { - eprintln!("Tervin hook: Tervin did not answer within {HOOK_TIMEOUT_SECS}s."); + eprintln!("{HOOK_STDERR_PREFIX}Tervin did not answer within {HOOK_TIMEOUT_SECS}s."); return 1; } let Ok(value) = serde_json::from_str::(response.trim()) else { - eprintln!("Tervin hook: Tervin's answer could not be read."); + eprintln!("{HOOK_STDERR_PREFIX}Tervin's answer could not be read."); return 1; }; @@ -525,6 +525,18 @@ pub fn run_hook_client(socket: &Path) -> i32 { /// The flag that turns Tervin's own executable into the hook. pub const HOOK_FLAG: &str = "--tervin-hook"; +/// The prefix every message the hook client prints to stderr carries. +/// +/// This is how the normalizer recognises the gate's own failures. The runtime echoes +/// the hook's command line back only when the hook *blocked*; a hook that merely +/// failed carries nothing but its own stderr, so without a marker of its own every +/// gate failure was reported as the user's configuration. +/// +/// It is deliberately the same string a reader sees, rather than a hidden token: the +/// text is already user-facing, and a sentinel nobody can read is one nobody +/// maintains. Denials are exempt — their stderr is fed back to the agent verbatim. +pub const HOOK_STDERR_PREFIX: &str = "Tervin hook: "; + /// Recognise a hook invocation before any UI starts. /// /// The workspace binary doubles as the hook command so the path is exact and no diff --git a/crates/agent-runtime/src/claude/normalize.rs b/crates/agent-runtime/src/claude/normalize.rs index 6f3c0cb..4c97c25 100644 --- a/crates/agent-runtime/src/claude/normalize.rs +++ b/crates/agent-runtime/src/claude/normalize.rs @@ -332,9 +332,15 @@ impl Normalizer { .filter(|s| !s.is_empty()) .map(String::from); - // Tervin's gate identifies itself by the flag in its own command line, which - // the runtime echoes back in the hook's error text. - let is_tervin = value.to_string().contains(crate::claude::hooks::HOOK_FLAG); + // Tervin's gate identifies itself two ways, and it needs both. The runtime + // echoes the hook's command line back only when the hook *blocked*; a hook that + // merely failed carries nothing but its own stderr. Matching on the command line + // alone therefore recognised the gate exactly when it worked and missed it every + // time it broke — so Tervin reported its own failures as the user's hooks. + let is_tervin = value.to_string().contains(crate::claude::hooks::HOOK_FLAG) + || stderr + .as_deref() + .is_some_and(|s| s.contains(crate::claude::hooks::HOOK_STDERR_PREFIX)); self.hook_runs.push(crate::runtime::HookRun { name: name.clone(), @@ -1829,6 +1835,65 @@ mod tests { assert!(n.denials.is_empty(), "the gate records its own denials"); } + #[test] + fn a_gate_that_failed_is_still_recognised_as_tervins_own() { + // The case that was wrong. When the gate blocks, the runtime echoes its command + // line back and the flag is there to find. When the gate merely fails there is + // no command line — only the hook's own stderr — so matching on the flag alone + // recognised the gate exactly when it worked and missed it every time it broke. + // Tervin then showed its own dead socket as a hook the user had configured. + let mut n = normalizer(); + let events = n.ingest(&serde_json::json!({ + "type": "system", "subtype": "hook_response", + "hook_name": "PreToolUse:Bash", "hook_event": "PreToolUse", + "stderr": "Tervin hook: Tervin did not answer within 5s.", + "exit_code": 1, "outcome": "error" + })); + + assert!( + events.is_empty(), + "Tervin's own failure is not a diagnostic about the user's setup: {events:?}" + ); + assert_eq!(n.hook_runs.len(), 1); + assert!( + n.hook_runs[0].is_tervin, + "a gate failure carries no command line, so the stderr prefix has to carry it" + ); + } + + #[test] + fn the_hook_client_and_the_normalizer_agree_on_the_prefix() { + // The two ends drifting apart is what caused the misattribution, and nothing + // else would catch it: each side compiles perfectly well on its own. + let mut n = normalizer(); + n.ingest(&serde_json::json!({ + "type": "system", "subtype": "hook_response", + "hook_name": "PreToolUse:Bash", "hook_event": "PreToolUse", + "stderr": format!( + "{}could not reach Tervin at /run/h.sock (No such file or directory). \ + This tool call was NOT checked against Tervin Rules.", + crate::claude::hooks::HOOK_STDERR_PREFIX + ), + "exit_code": 1, "outcome": "error" + })); + assert!(n.hook_runs[0].is_tervin); + } + + #[test] + fn a_user_hook_that_fails_is_still_reported_as_theirs() { + // The other half of the fix: widening the match must not swallow the user's own + // broken hooks, which are the reason the diagnostic exists at all. + let mut n = normalizer(); + let events = n.ingest(&serde_json::json!({ + "type": "system", "subtype": "hook_response", + "hook_name": "PreToolUse:Bash", "hook_event": "PreToolUse", + "stderr": "my-gate.sh: line 3: jq: command not found", + "exit_code": 127, "outcome": "error" + })); + assert!(!n.hook_runs[0].is_tervin); + assert!(events.iter().any(|e| e.kind() == "diagnostic.detected")); + } + #[test] fn an_error_result_fails_the_thread() { let mut n = normalizer(); diff --git a/crates/agent-runtime/src/profile.rs b/crates/agent-runtime/src/profile.rs index 143f678..fa311ec 100644 --- a/crates/agent-runtime/src/profile.rs +++ b/crates/agent-runtime/src/profile.rs @@ -332,6 +332,53 @@ fn candidates_from_acp_agents() -> Vec { .collect() } +/// How long the user's shell gets to list its aliases before Tervin stops caring. +const ALIAS_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(5); + +/// Run a command, collect its standard output, and give up after `limit`. +/// +/// `std::process::Command` offers no timeout, and the one thing worse than a +/// discovery that finds nothing is a discovery that never returns. Returns `None` +/// if the command could not start, ran out of time, or was killed — all of which +/// mean the same thing to every caller here: no answer. +fn run_briefly(command: &mut std::process::Command, limit: std::time::Duration) -> Option> { + let mut child = command + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::null()) + .spawn() + .ok()?; + + // Drain the pipe on another thread. A child that fills the pipe buffer blocks + // until someone reads it, so waiting for exit without draining would be waiting + // for a child that is itself waiting for us. + let mut pipe = child.stdout.take()?; + let (tx, rx) = std::sync::mpsc::channel(); + std::thread::spawn(move || { + let mut buf = Vec::new(); + let _ = std::io::Read::read_to_end(&mut pipe, &mut buf); + let _ = tx.send(buf); + }); + + let deadline = std::time::Instant::now() + limit; + loop { + match child.try_wait() { + Ok(Some(_)) => break, + Ok(None) if std::time::Instant::now() < deadline => { + std::thread::sleep(std::time::Duration::from_millis(25)); + } + // Out of time, or a child that cannot be waited on at all. Kill it either + // way: whatever the rc files are still doing, nobody is waiting for it now. + _ => { + let _ = child.kill(); + let _ = child.wait(); + return None; + } + } + } + + rx.recv_timeout(limit).ok() +} + /// Ask the user's shell for its aliases and parse agent-launching ones. /// /// Runs `$SHELL -ic alias`, which sources the user's rc files. That is the only @@ -344,15 +391,19 @@ fn candidates_from_aliases() -> Vec { _ => return Vec::new(), }; - let output = std::process::Command::new(&shell) - .args(["-ic", "alias"]) - .stdin(std::process::Stdio::null()) - .output(); - - let Ok(output) = output else { + // An interactive shell runs whatever the user's rc files contain, and some of + // that waits: version managers that hit the network, prompt frameworks, a stray + // `read`. Aliases are a convenience, so a shell that will not answer promptly is + // simply one that offers nothing — never a reason to keep the caller waiting. + let Some(stdout) = run_briefly( + std::process::Command::new(&shell) + .args(["-ic", "alias"]) + .stdin(std::process::Stdio::null()), + ALIAS_TIMEOUT, + ) else { return Vec::new(); }; - let text = String::from_utf8_lossy(&output.stdout); + let text = String::from_utf8_lossy(&stdout); text.lines() .filter_map(parse_alias_line) @@ -569,6 +620,46 @@ fn shell_words_split(input: &str) -> Option> { mod tests { use super::*; + #[test] + fn a_command_that_answers_is_read_in_full() { + let out = run_briefly( + std::process::Command::new("sh").args(["-c", "echo alias-one; echo alias-two"]), + std::time::Duration::from_secs(10), + ) + .expect("a prompt command should produce output"); + let text = String::from_utf8_lossy(&out); + assert!( + text.contains("alias-one") && text.contains("alias-two"), + "{text}" + ); + } + + #[test] + fn a_command_that_hangs_is_abandoned_rather_than_waited_on() { + // The bug this exists to prevent: `$SHELL -ic alias` sources rc files, and an + // rc file that blocks used to block the whole agents view behind it. + let started = std::time::Instant::now(); + let out = run_briefly( + std::process::Command::new("sh").args(["-c", "sleep 30"]), + std::time::Duration::from_millis(300), + ); + assert!(out.is_none(), "a hung command has no answer to give"); + assert!( + started.elapsed() < std::time::Duration::from_secs(5), + "gave up after {:?}, which is not giving up", + started.elapsed() + ); + } + + #[test] + fn a_command_that_does_not_exist_is_not_an_answer() { + assert!(run_briefly( + &mut std::process::Command::new("tervin-no-such-binary-anywhere"), + std::time::Duration::from_secs(5), + ) + .is_none()); + } + #[test] fn parses_a_config_dir_alias() { // The exact shape people actually use for multiple accounts. diff --git a/crates/tervin-app/src/commands.rs b/crates/tervin-app/src/commands.rs index cb85166..c8300a0 100644 --- a/crates/tervin-app/src/commands.rs +++ b/crates/tervin-app/src/commands.rs @@ -782,13 +782,19 @@ pub async fn audit_recent( // ============================================================ agents +/// What the user configured, read from disk and nothing else. +/// +/// Deliberately separate from [`AgentsDiscovery`]. These two answer different +/// questions — "what did I set up?" and "what is on this machine?" — and only the +/// second one has to leave the process to find out. Serving them together meant a +/// profile the user had written by hand could not be shown until every agent binary +/// on the machine had been probed, and could not be shown *at all* if one of those +/// probes failed: the whole command returned an error, the UI had no profiles, and +/// it said "No agent profile configured" to a user with five of them. #[derive(Debug, Serialize)] pub struct AgentsOverview { pub profiles: Vec, pub default_profile: Option, - pub discovered: Vec, - /// Profiles Tervin found but has not adopted. - pub import_candidates: Vec, /// Where the files the UI mentions actually are. /// /// Resolved rather than written into the interface, because the location differs by @@ -798,8 +804,29 @@ pub struct AgentsOverview { pub mcp_path: String, } +/// What Tervin found on the machine. Every field here costs a subprocess. +#[derive(Debug, Serialize)] +pub struct AgentsDiscovery { + pub discovered: Vec, + /// Profiles Tervin found but has not adopted. + pub import_candidates: Vec, +} + +/// The user's own configuration. Cheap, local, and cannot fail on a missing binary. #[tauri::command] pub async fn agents_overview(state: State<'_, Arc>) -> Result { + let profiles = state.profiles.read().clone(); + Ok(AgentsOverview { + default_profile: profiles.default_profile, + profiles: profiles.profiles, + profiles_path: tervin_core::paths::abbreviate(&ProfileConfig::path()), + mcp_path: tervin_core::paths::abbreviate(&agent_runtime::McpConfig::path()), + }) +} + +/// What is installed. Slow and failure-prone by nature, which is why it stands alone. +#[tauri::command] +pub async fn agents_discovery(state: State<'_, Arc>) -> Result { // Snapshot under the lock, then release it: discovery spawns processes and // must not hold the registry lock while it awaits them. let adapters = state.agents.read().snapshot(); @@ -811,8 +838,13 @@ pub async fn agents_overview(state: State<'_, Arc>) -> Result = profiles.profiles.iter().map(|p| p.id.clone()).collect(); + let existing: Vec = state + .profiles + .read() + .profiles + .iter() + .map(|p| p.id.clone()) + .collect(); let import_candidates = blocking(move || { Ok(agent_runtime::profile::import_candidates() @@ -822,13 +854,9 @@ pub async fn agents_overview(state: State<'_, Arc>) -> Result anyhow::Result<()> { commands::audit_recent, // agents commands::agents_overview, + commands::agents_discovery, commands::project_instructions, commands::agents_add_acp, commands::agents_add_local_model, diff --git a/ui/src/components/SettingsPanel.tsx b/ui/src/components/SettingsPanel.tsx index 80e1463..dd98204 100644 --- a/ui/src/components/SettingsPanel.tsx +++ b/ui/src/components/SettingsPanel.tsx @@ -515,6 +515,9 @@ function ShellSection() { function AgentsSection() { const s = useWorkspace(); const agents = s.agents; + // Arrives after the profiles above, and may never arrive at all. Everything read + // from it therefore has to render sensibly while it is still null. + const discovery = s.agentsDiscovery; return (
@@ -565,12 +568,12 @@ function AgentsSection() {
- {(agents?.import_candidates.length ?? 0) > 0 && ( + {(discovery?.import_candidates.length ?? 0) > 0 && ( - {agents!.import_candidates.map((c) => ( + {discovery!.import_candidates.map((c) => (
- {(agents?.discovered ?? []).map((d) => ( + {/* Said rather than shown as an empty list: "nothing installed" and "not + finished looking" are different answers, and only one of them is news. */} + {discovery === null && ( +
+ Looking for installed agents… +
+ )} + {(discovery?.discovered ?? []).map((d) => (
diff --git a/ui/src/lib/agents.store.test.ts b/ui/src/lib/agents.store.test.ts new file mode 100644 index 0000000..6cc04c4 --- /dev/null +++ b/ui/src/lib/agents.store.test.ts @@ -0,0 +1,91 @@ +/** + * What the user configured must survive what Tervin failed to find. + * + * This file exists because of a specific failure. `agents_overview` returned the + * user's profiles and the results of probing the machine for installed agents in one + * call, so a probe that failed failed the whole command — and a user with five + * profiles in `agents.toml` was shown "No agent profile configured". The profiles + * were never the problem; they had been read correctly and then thrown away. + * + * So these assert the seam rather than the symptom: profiles are set from their own + * call, and nothing discovery does afterwards can take them back off the screen. + */ + +import { beforeEach, describe, expect, it, vi } from "vitest"; +import * as api from "./api"; +import { useWorkspace } from "./store"; + +function profile(id: string): api.AgentProfile { + return { + id, + name: id, + runtime_id: "claude-code", + binary: "claude", + args: [], + env: {}, + model: null, + permission_mode: null, + badge: null, + sensitive: false, + }; +} + +const OVERVIEW: api.AgentsOverview = { + profiles: [profile("work"), profile("personal")], + default_profile: "work", + profiles_path: "~/.config/tervin/agents.toml", + mcp_path: "~/.config/tervin/mcp.json", +}; + +beforeEach(() => { + vi.restoreAllMocks(); + useWorkspace.setState({ + agents: null, + agentsDiscovery: null, + activeProfileId: null, + notices: [], + }); +}); + +describe("refreshAgents", () => { + it("keeps the configured profiles when discovery fails", async () => { + vi.spyOn(api, "agentsOverview").mockResolvedValue(OVERVIEW); + vi.spyOn(api, "agentsDiscovery").mockRejectedValue( + new Error("$SHELL -ic alias never returned"), + ); + + await useWorkspace.getState().refreshAgents(); + + const s = useWorkspace.getState(); + expect(s.agents?.profiles.map((p) => p.id)).toEqual(["work", "personal"]); + expect(s.activeProfileId).toBe("work"); + // The failure is reported, not swallowed — it just costs nothing above it. + expect(s.notices.length).toBeGreaterThan(0); + expect(s.agentsDiscovery).toBeNull(); + }); + + it("still asks for discovery when it can succeed", async () => { + const discovery: api.AgentsDiscovery = { discovered: [], import_candidates: [] }; + vi.spyOn(api, "agentsOverview").mockResolvedValue(OVERVIEW); + vi.spyOn(api, "agentsDiscovery").mockResolvedValue(discovery); + + await useWorkspace.getState().refreshAgents(); + + expect(useWorkspace.getState().agentsDiscovery).toEqual(discovery); + expect(useWorkspace.getState().agents?.profiles).toHaveLength(2); + }); + + it("does not probe the machine when the profiles themselves could not be read", async () => { + // Nothing to fill in around, and a second failure would only be noise. + vi.spyOn(api, "agentsOverview").mockRejectedValue(new Error("agents.toml is malformed")); + const discovery = vi.spyOn(api, "agentsDiscovery").mockResolvedValue({ + discovered: [], + import_candidates: [], + }); + + await useWorkspace.getState().refreshAgents(); + + expect(discovery).not.toHaveBeenCalled(); + expect(useWorkspace.getState().notices.length).toBeGreaterThan(0); + }); +}); diff --git a/ui/src/lib/api.ts b/ui/src/lib/api.ts index 96d4d52..b619e4e 100644 --- a/ui/src/lib/api.ts +++ b/ui/src/lib/api.ts @@ -288,16 +288,27 @@ export interface ImportCandidate { source: string; } +/** + * What the user configured. Read from disk, so it always arrives. + * + * Kept apart from {@link AgentsDiscovery} on purpose: probing the machine is slow and + * can fail, and when the two travelled together a failed probe meant a user with five + * configured profiles was told they had none. + */ export interface AgentsOverview { profiles: AgentProfile[]; default_profile: string | null; - discovered: Discovery[]; - import_candidates: ImportCandidate[]; /** Resolved paths, because they differ by platform. Never hard-code them. */ profiles_path: string; mcp_path: string; } +/** What Tervin found installed. Arrives later than {@link AgentsOverview}, or not at all. */ +export interface AgentsDiscovery { + discovered: Discovery[]; + import_candidates: ImportCandidate[]; +} + /** A kind of instruction file, named after the tool that established it. */ export type InstructionKind = | "agents" @@ -710,6 +721,7 @@ export const auditRecent = (limit: number) => // ----------------------------------------------------------------- agents export const agentsOverview = () => invoke("agents_overview"); +export const agentsDiscovery = () => invoke("agents_discovery"); /** Instruction files and MCP config other tools already wrote into this project. */ export const projectInstructions = () => diff --git a/ui/src/lib/store.ts b/ui/src/lib/store.ts index 740b9db..f15ca89 100644 --- a/ui/src/lib/store.ts +++ b/ui/src/lib/store.ts @@ -251,6 +251,8 @@ interface WorkspaceState { blocks: api.BlockSummary[]; blockFilter: api.BlockFilter; agents: api.AgentsOverview | null; + /** Null until discovery answers, and stays null if it never does. */ + agentsDiscovery: api.AgentsDiscovery | null; activeProfileId: string | null; // threads @@ -472,6 +474,7 @@ export const useWorkspace = create((set, get) blocks: [], blockFilter: { limit: 200 }, agents: null, + agentsDiscovery: null, activeProfileId: null, threads: {}, @@ -855,6 +858,10 @@ export const useWorkspace = create((set, get) }, refreshAgents: async () => { + // Two calls, deliberately not one. What the user configured is read from disk and + // is on screen before anything is probed; what is installed takes a subprocess per + // agent and may be slow or fail. Fetched together, one failed probe took the + // profiles down with it and the user was told they had none. try { const agents = await api.agentsOverview(); set((s) => ({ @@ -864,6 +871,14 @@ export const useWorkspace = create((set, get) })); } catch (e) { get().pushNotice(describeError(e)); + return; + } + + try { + set({ agentsDiscovery: await api.agentsDiscovery() }); + } catch (e) { + // Said, not swallowed — but the profiles above stay exactly where they are. + get().pushNotice(describeError(e)); } },