From 906f9acfc63fbbedf1247262fa84d5559e3173a0 Mon Sep 17 00:00:00 2001 From: CodeWhale Bot Date: Tue, 15 Sep 2026 23:05:32 -0700 Subject: [PATCH 1/4] perf(tui): stop re-parsing the whole tool-argument buffer per delta (#6213 T4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every `InputJsonDelta` ran `parse_tool_input` over the entire accumulated buffer, and `parse_tool_input` starts with the full `arg_repair` ladder (`dispatch.rs:531-538`). Mid-stream the buffer almost never parses strictly, so each delta fell through to the repair stages, which copy the whole buffer — O(n²) work per tool call, for a value nothing read. Nothing reads `tool_state.input` between the first delta and finalization. Inside the stream loop the field is written at the mirror, logged at `ContentBlockStop`, then unconditionally overwritten by `finalize_streamed_tool_input`; every other `.input` in this file belongs to a different binding outside the loop. The mirror's one historical consumer was a block whose `ContentBlockStop` never arrived, whose partial parse then reached dispatch and executed (#5986). That was closed by the post-loop drain, which runs the same finalization gate over every unstopped block — the mirror was simply left behind by that fix. The drain, not the mirror, is what makes a truncated tool call safe, and it is also why no mid-stream parse is needed at all. The `ContentBlockStop` log printed `Current input` *before* finalization, so it showed the mirror's last partial parse. With the mirror gone it would have shown the `ContentBlockStart` placeholder, which is worse than nothing — dropped. The authoritative value is still logged two lines later by `finalize_streamed_tool_input` as `final input`. Three comments asserted the mirror exists and are corrected with it, per the repo rule that a stale comment is a defect. Behaviour preserved: after finalization `tool_state.input` derives solely from `input_buffer`, except when the buffer is empty, where the `ContentBlockStart` value survives — the non-streaming and text-parsed-tool-call paths rely on that and are untouched. `structure_synthesized` is still rejected in exactly one place. Verification (macOS aarch64): cargo check -p codewhale-tui --all-features --locked Finished `dev` profile in 1m 21s sh scripts/with-hermetic-test-home.sh cargo test -p codewhale-tui --lib \ --all-features --locked -j 5 -- --test-threads=2 tool_call_without_block_stop test core::engine::tests::complete_tool_call_without_block_stop_still_dispatches ... ok test core::engine::tests::truncated_tool_call_without_block_stop_never_dispatches ... ok test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 12807 filtered out cargo fmt --all -- --check (clean) No new test: the removed behaviour has no observable. The two tests above already pin the contract that makes the removal safe. Signed-off-by: CodeWhale Bot Co-Authored-By: Claude Opus 5 (1M context) --- crates/tui/src/core/engine/turn_loop.rs | 45 ++++++++++--------------- 1 file changed, 18 insertions(+), 27 deletions(-) diff --git a/crates/tui/src/core/engine/turn_loop.rs b/crates/tui/src/core/engine/turn_loop.rs index f97f8c69b7..92c733c518 100644 --- a/crates/tui/src/core/engine/turn_loop.rs +++ b/crates/tui/src/core/engine/turn_loop.rs @@ -5118,20 +5118,13 @@ impl Engine { tool_state.name, partial_json, tool_state.input_buffer )); } - // Mid-stream mirror of a partial buffer. The - // argument text is *expected* to be incomplete - // here, so `structure_synthesized` is ignored on - // purpose; ContentBlockStop below is where an - // unfinished argument becomes an error. - if let Some(parsed) = parse_tool_input(&tool_state.input_buffer) { - tool_state.input = parsed.value.clone(); - if crate::logging::is_verbose() { - crate::logging::info(format!( - "Tool '{}' input parsed: {:?}", - tool_state.name, parsed.value - )); - } - } + // The buffer is the only mid-stream state: nothing + // reads `tool_state.input` before finalization, so + // there is no mirror parse here. Running the + // `arg_repair` ladder per delta re-scanned the whole + // accumulated buffer O(n²) times per tool call to + // produce a value that `finalize_streamed_tool_input` + // unconditionally overwrote (#6213 T4). } } }, @@ -5174,8 +5167,8 @@ impl Engine { && let Some(tool_state) = tool_uses.get_mut(tool_idx) { crate::logging::info(format!( - "Tool '{}' block stop. Buffer: '{}', Current input: {:?}", - tool_state.name, tool_state.input_buffer, tool_state.input + "Tool '{}' block stop. Buffer: '{}'", + tool_state.name, tool_state.input_buffer )); self.finalize_streamed_tool_input(tool_state).await; @@ -5232,14 +5225,12 @@ impl Engine { } } // A stream cut at the provider's output limit ends without the - // closing ContentBlockStop for whatever block was in flight. Those - // blocks' inputs still hold the mid-stream mirror's best-effort - // parse, which ignores `structure_synthesized` by design — left - // as-is, a truncated tool call reaches dispatch through - // `tool.input` and executes (#5986). Every block that never - // stopped goes through the same finalization gate a normal - // ContentBlockStop applies, and is announced with the same - // finalized input. + // closing ContentBlockStop for whatever block was in flight. Before + // this drain existed a truncated tool call reached dispatch through + // `tool.input` and executed (#5986). Every block that never stopped + // goes through the same finalization gate a normal ContentBlockStop + // applies, and is announced with the same finalized input — which is + // also why no mid-stream parse is needed (#6213 T4). for tool_idx in std::mem::take(&mut current_tool_indices).into_values() { let Some(tool_state) = tool_uses.get_mut(tool_idx) else { continue; @@ -5284,9 +5275,9 @@ impl Engine { /// execute a truncated tool call (#5986). Called for a tool block that /// closes normally (`ContentBlockStop`) and again after the stream ends /// for blocks whose Stop never arrived — a provider cutting the stream - /// at its output limit omits the closing event, while the mid-stream - /// mirror deliberately ignores `structure_synthesized` because partial - /// text is the normal state mid-stream. + /// at its output limit omits the closing event. This is the only place + /// the accumulated buffer is parsed, and the only place + /// `structure_synthesized` is rejected. async fn finalize_streamed_tool_input(&self, tool_state: &mut ToolUseState) { if tool_state.input_buffer.trim().is_empty() { crate::logging::warn(format!( From 5d08e45b2809318b87a1174ced62def857d7378c Mon Sep 17 00:00:00 2001 From: CodeWhale Bot Date: Tue, 15 Sep 2026 23:09:56 -0700 Subject: [PATCH 2/4] fix(fleet): stop a default role blocking a prompt-only agent spawn (#6244) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `agent(action=start, prompt=...)` with no `type`, `role` or `profile` failed at the tool boundary with Fleet member selector `role:general` is ambiguous even though the caller never asked for a role. `resolve_spawn_route_profile` fell back to `request.agent_type.as_str()`, and `request.agent_type` defaults to `FleetRole::Worker`, whose `as_str()` is `"general"` — so Codewhale synthesized `role:general`, handed it to the pin resolver, and turned its own default into a hard refusal whenever two saved members happened to share that role. The reporter then burned three further tool calls guessing schemas. Refusing is right when the caller *did* name a role, type or profile: silently picking one of several providers for them would be the worse failure, and `saved_role_ambiguity_fails_unless_a_higher_priority_pin_selects_the_route` pins that. The bug is only that a Codewhale-supplied default was treated as a caller request. So the ambiguity is now interpreted, not just propagated: when the role came from the default, an ambiguous pin means "no usable pin" and the spawn falls through to the session/operator route it would have taken anyway. Every other selector error, and every ambiguity on a role the caller actually wrote, still refuses exactly as before. `request.agent_type_explicit` (mod.rs:1886, set at 14374) already records "the caller named a type or role", so no new state was needed. `resolve_pinned_role_profile` returned `anyhow::Result`, which flattened the typed `FleetSelectorError::Ambiguous` the caller now needs to see. It returns the concrete error instead — that deletes an anyhow round-trip rather than adding machinery, since `FleetSelectorError` is the only error it can produce. The refusal message also named the candidate members without saying how to pass one back; it now names the argument (`profile`). Existing assertions match on `"is ambiguous"` and are unaffected. Verification (macOS aarch64): cargo check -p codewhale-tui --all-features --locked (clean) cargo fmt --all -- --check (clean) sh scripts/with-hermetic-test-home.sh cargo test -p codewhale-tui --lib \ --all-features --locked -j 5 -- --test-threads=2 \ prompt_only_spawn saved_role_ambiguity pinned_role_lookup test fleet::worker_runtime::tests::pinned_role_lookup_ignores_builtins_and_rejects_semantic_ambiguity ... ok test tools::subagent::tests::prompt_only_spawn_survives_an_ambiguous_default_role ... ok test tools::subagent::tests::saved_role_ambiguity_fails_unless_a_higher_priority_pin_selects_the_route ... ok test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 12807 filtered out The new test was confirmed to fail without the fix: forcing `role_was_requested` back to `true` reproduces the original refusal — test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 12809 filtered out Signed-off-by: CodeWhale Bot Co-Authored-By: Claude Opus 5 (1M context) --- crates/tui/src/fleet/identity.rs | 2 +- crates/tui/src/fleet/worker_runtime.rs | 10 ++--- crates/tui/src/tools/subagent/mod.rs | 29 +++++++++---- crates/tui/src/tools/subagent/tests.rs | 58 ++++++++++++++++++++++++++ 4 files changed, 86 insertions(+), 13 deletions(-) diff --git a/crates/tui/src/fleet/identity.rs b/crates/tui/src/fleet/identity.rs index 70f19a5093..764dc9d61f 100644 --- a/crates/tui/src/fleet/identity.rs +++ b/crates/tui/src/fleet/identity.rs @@ -280,7 +280,7 @@ pub enum FleetSelectorError { path: String, }, #[error( - "Fleet member selector `{selector}` is ambiguous; choose one member explicitly: {candidates}" + "Fleet member selector `{selector}` is ambiguous; choose one member explicitly by passing `profile` as one of: {candidates}" )] Ambiguous { selector: String, diff --git a/crates/tui/src/fleet/worker_runtime.rs b/crates/tui/src/fleet/worker_runtime.rs index b0fded556c..77f9ae285c 100644 --- a/crates/tui/src/fleet/worker_runtime.rs +++ b/crates/tui/src/fleet/worker_runtime.rs @@ -23,7 +23,7 @@ use codewhale_protocol::fleet::{ }; use serde::{Deserialize, Serialize}; -use super::identity::resolve_member_in_profiles; +use super::identity::{FleetSelectorError, resolve_member_in_profiles}; use super::profile::{ AgentProfile, FleetDelegationHints, FleetLoadout, FleetProfile, FleetProfilePermissions, FleetRole as FleetProfileRole, FleetSlot, ProfileOrigin, canonical_public_role_name, @@ -935,7 +935,7 @@ pub(crate) fn append_agent_profile_prompt(prompt: &mut String, agent_profile: &A pub(crate) fn resolve_pinned_role_profile( agent_profiles: &[AgentProfile], role: &str, -) -> Result> { +) -> Result, FleetSelectorError> { let pinned = agent_profiles .iter() .filter(|profile| { @@ -949,11 +949,11 @@ pub(crate) fn resolve_pinned_role_profile( }) .cloned() .collect::>(); - Ok(resolve_member_in_profiles( + resolve_member_in_profiles( &pinned, &format!("role:{}", canonical_public_role_name(role)), - )? - .cloned()) + ) + .map(|member| member.cloned()) } /// Compare only the known route pair; never infer a provider from a wire id's diff --git a/crates/tui/src/tools/subagent/mod.rs b/crates/tui/src/tools/subagent/mod.rs index e02b896fc3..88b031a047 100644 --- a/crates/tui/src/tools/subagent/mod.rs +++ b/crates/tui/src/tools/subagent/mod.rs @@ -14927,13 +14927,28 @@ fn resolve_spawn_route_profile( if member.is_some() || configured_manual_spawn_model(runtime, request)?.is_some() { return Ok(member); } - let role = request - .assignment - .role - .as_deref() - .unwrap_or_else(|| request.agent_type.as_str()); - let member = crate::fleet::worker_runtime::resolve_pinned_role_profile(roster.members(), role) - .map_err(|error| ToolError::invalid_input(error.to_string()))?; + // A role the caller never wrote is Codewhale's own default + // (`FleetRole::Worker` -> "general"), not a request. Refusing an ambiguous + // pin is right when the caller named a role, type or profile — it stops us + // silently choosing one of several providers for them. But for a + // prompt-only `agent(action=start, ...)` it turned our default into a hard + // failure whenever two members happened to share role `general`, blocking + // the spawn at the tool boundary over a selector the caller never asked + // for (#6244). In that case an ambiguous pin means "no usable pin": fall + // through to the session/operator route the spawn would have taken anyway. + let requested_role = request.assignment.role.as_deref(); + let role = requested_role.unwrap_or_else(|| request.agent_type.as_str()); + let role_was_requested = requested_role.is_some() || request.agent_type_explicit; + let member = + match crate::fleet::worker_runtime::resolve_pinned_role_profile(roster.members(), role) { + Ok(member) => member, + Err(crate::fleet::identity::FleetSelectorError::Ambiguous { .. }) + if !role_was_requested => + { + None + } + Err(error) => return Err(ToolError::invalid_input(error.to_string())), + }; let Some(member) = member else { return Ok(None); }; diff --git a/crates/tui/src/tools/subagent/tests.rs b/crates/tui/src/tools/subagent/tests.rs index 1c1c6a5e82..177f5153cd 100644 --- a/crates/tui/src/tools/subagent/tests.rs +++ b/crates/tui/src/tools/subagent/tests.rs @@ -4825,6 +4825,64 @@ fn saved_role_ambiguity_fails_unless_a_higher_priority_pin_selects_the_route() { assert_eq!(explicit.agent_type, FleetRole::Reviewer); } +/// A prompt-only spawn must not be refused over a role the caller never wrote. +/// +/// `request.agent_type` defaults to `FleetRole::Worker`, whose `as_str()` is +/// `"general"`, so the route lookup synthesized `role:general` for a call that +/// named no type, role or profile. Two saved members sharing role `general` then +/// made every bare `agent(action=start, ...)` fail at the tool boundary (#6244). +/// The refusal is still correct when the caller *did* ask — that half is pinned +/// by `saved_role_ambiguity_fails_unless_a_higher_priority_pin_selects_the_route`. +#[test] +fn prompt_only_spawn_survives_an_ambiguous_default_role() { + let root = tempdir().unwrap(); + for id in ["general-a", "general-b"] { + std::fs::write( + root.path().join(format!("{id}.toml")), + format!( + "id = '{id}'\nbase_role = 'general'\nprovider = 'deepseek'\nmodel = 'deepseek-v4-flash'\n", + ), + ) + .unwrap(); + } + let profiles = crate::fleet::profile::load_agent_profiles_from_dir(root.path()).unwrap(); + assert_eq!(profiles.len(), 2); + let roster = FleetRoster::from_members(profiles); + let runtime = stub_runtime(); + + let mut request = parse_spawn_request(&json!({"prompt":"do the thing"})).unwrap(); + assert!( + !request.agent_type_explicit, + "a prompt-only spawn must not report an explicit type" + ); + assert_eq!(request.assignment.role, None); + + let member = resolve_spawn_route_profile(&runtime, &mut request, &roster) + .expect("an ambiguous default role must not block a prompt-only spawn"); + assert!( + member.is_none(), + "no member is pinned, so the spawn falls through to the session route" + ); + assert_eq!( + request.profile, None, + "an unusable pin must not stamp a member" + ); + + // The same roster still refuses when the caller actually named the role. + let mut asked = parse_spawn_request(&json!({"prompt":"do the thing", "type":"general"})) + .expect("an explicit general type parses"); + if asked.agent_type_explicit { + let error = resolve_spawn_route_profile(&runtime, &mut asked, &roster) + .expect_err("an explicitly requested ambiguous role must still refuse"); + let message = error.to_string(); + assert!(message.contains("ambiguous"), "{message}"); + assert!( + message.contains("profile"), + "the refusal must say how to choose one: {message}" + ); + } +} + #[test] fn providerless_spawn_model_gate_rejects_known_foreign_route_before_spawn() { let runtime = stub_runtime_for_provider("moonshot"); From 270c02593c7b943e04cd6257e6466b1ecb01f50f Mon Sep 17 00:00:00 2001 From: CodeWhale Bot Date: Tue, 15 Sep 2026 23:29:13 -0700 Subject: [PATCH 3/4] perf(tui): reuse one buffer set across a tool_search catalog scan (#6213 T5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `tool_search_haystack` built a lowercased `name\ndescription\ninput_schema` blob per deferred tool with `format!`, which copied all three pieces a second time into the concatenation and then dropped the result. The bm25 scorer separately re-lowered `tool.name` once per query term for a value that does not vary across terms. One reused `ToolSearchScratch` replaces both: the buffers keep their capacity across the scan instead of reallocating per tool, the concatenation copy is gone, and the name is lowered once per tool rather than once per term. This is the same precomputed-index idiom `CachedFallback` already uses for the static core-action fallbacks in this file, so it adds no new pattern. Two things the issue asked for that this deliberately does NOT do: - **No cache across queries.** The issue asks to stop re-serialising per query. A sound key does exist — `tool_set_identity` (crates/core/src/prefix_cache.rs) is an allocation-free order-sensitive digest over the catalog — but the catalog at the call site is a turn-scoped `Vec` that is *mutated in place* mid-turn by `replace_runtime_mcp_tools`, so pointer identity is not a key, and computing a content digest to avoid a content scan is circular at this size. `tool_search` also runs a handful of times per turn beside a multi-second provider request; it is not a per-frame or per-token path. A cache here would add an invalidation rule to defend and buy nothing measurable. - **No per-`char` lowercasing.** It would allocate less, but `chars().flat_map(char::to_lowercase)` is not the same function as `str::to_lowercase` — they differ on Greek final sigma. On a path this cold, matching the original exactly is worth more than the last allocation. Verification (macOS aarch64): cargo check -p codewhale-tui --all-features --locked (clean) cargo fmt --all -- --check (clean) sh scripts/with-hermetic-test-home.sh cargo test -p codewhale-tui --lib \ --all-features --locked -j 5 -- --test-threads=2 tool_catalog:: test result: ok. 13 passed; 0 failed; 0 ignored; 0 measured; 12797 filtered out No new test: this is an allocation change with no observable difference. The 13 tests above cover the catalog surface it sits behind. Signed-off-by: CodeWhale Bot Co-Authored-By: Claude Opus 5 (1M context) --- crates/tui/src/core/engine/tool_catalog.rs | 70 ++++++++++++++++++---- 1 file changed, 58 insertions(+), 12 deletions(-) diff --git a/crates/tui/src/core/engine/tool_catalog.rs b/crates/tui/src/core/engine/tool_catalog.rs index 92eefb135c..020aaec124 100644 --- a/crates/tui/src/core/engine/tool_catalog.rs +++ b/crates/tui/src/core/engine/tool_catalog.rs @@ -734,13 +734,56 @@ pub(crate) fn active_tools_for_request( Some(tools) } -fn tool_search_haystack(tool: &Tool) -> String { - format!( - "{}\n{}\n{}", - tool.name.to_lowercase(), - tool.description.to_lowercase(), - tool.input_schema.to_string().to_lowercase() - ) +/// Reusable scratch for one `tool_search` catalog scan. +/// +/// Each deferred tool needs a lowercased `name\ndescription\ninput_schema` blob +/// that is compared once and dropped. Building it with `format!` also copied all +/// three pieces a second time into the concatenation, and the bm25 scorer then +/// re-lowered `tool.name` once per query term for a value that does not vary +/// across terms. Reusing one set of buffers across the scan removes the +/// concatenation copy and the per-term lowering, and keeps the buffers' capacity +/// instead of reallocating per tool (#6213 T5). +/// +/// This is the same precomputed-index idiom `CachedFallback` already uses for +/// the static core-action fallbacks in this file; it is not a new pattern. +/// +/// Lowercasing deliberately stays `str::to_lowercase`, matching the original +/// exactly. A per-`char` fold would allocate less but is not the same function — +/// it differs on Greek final sigma — and this path runs a handful of times per +/// turn beside a multi-second provider call, so it is not worth a semantic +/// change. +#[derive(Default)] +struct ToolSearchScratch { + /// `tool.name`, lowercased. Loop-invariant across query terms, so the bm25 + /// scorer reads this instead of re-lowering the name once per term. + name_lower: String, + /// Compact JSON of `tool.input_schema`, before lowercasing. + schema_json: String, + /// The match target: `name\ndescription\nschema`, all lowercased. + hay: String, +} + +impl ToolSearchScratch { + fn load(&mut self, tool: &Tool) { + use std::fmt::Write as _; + + self.name_lower.clear(); + self.name_lower.push_str(&tool.name.to_lowercase()); + + self.schema_json.clear(); + // `Value`'s `Display` is what `to_string()` calls, so this is the same + // text without materializing an owned copy first. Infallible for a + // `String` sink; a formatting error could only shorten the schema, + // which weakens matching and never breaks correctness. + let _ = write!(self.schema_json, "{}", tool.input_schema); + + self.hay.clear(); + self.hay.push_str(&self.name_lower); + self.hay.push('\n'); + self.hay.push_str(&tool.description.to_lowercase()); + self.hay.push('\n'); + self.hay.push_str(&self.schema_json.to_lowercase()); + } } fn catalog_contains_tool(catalog: &[Tool], name: &str) -> bool { @@ -820,6 +863,7 @@ fn discover_tools_with_regex( .map_err(|err| ToolError::invalid_input(format!("Invalid regex query: {err}")))?; let mut matches = Vec::new(); + let mut scratch = ToolSearchScratch::default(); for tool in catalog { // tool_search loads definitions omitted from the current request. An // eager tool is already present, so returning it as a cache candidate @@ -828,8 +872,8 @@ fn discover_tools_with_regex( if !tool.defer_loading.unwrap_or(false) || is_tool_search_tool(&tool.name) { continue; } - let hay = tool_search_haystack(tool); - if regex.is_match(&hay) { + scratch.load(tool); + if regex.is_match(&scratch.hay) { matches.push(tool.name.clone()); } if matches.len() >= max_results { @@ -850,17 +894,19 @@ fn discover_tools_with_bm25_like(catalog: &[Tool], query: &str, max_results: usi } let mut scored: Vec<(i64, String)> = Vec::new(); + let mut scratch = ToolSearchScratch::default(); for tool in catalog { if !tool.defer_loading.unwrap_or(false) || is_tool_search_tool(&tool.name) { continue; } - let hay = tool_search_haystack(tool); + scratch.load(tool); let mut score = 0i64; for term in &terms { - if hay.contains(term) { + if scratch.hay.contains(term) { score += 1; } - if tool.name.to_lowercase().contains(term) { + // Loop-invariant: lowered once by `load`, not once per term. + if scratch.name_lower.contains(term) { score += 2; } } From 7c8fe7ec9b1b64bdf11266d283961823042dd69d Mon Sep 17 00:00:00 2001 From: CodeWhale Bot Date: Tue, 15 Sep 2026 23:29:37 -0700 Subject: [PATCH 4/4] fix(tui): open a clicked path:line through the suspend path, once (#6235) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Open in editor" from the transcript context menu built its own `Command` and `.spawn()`ed it detached while the TUI still held raw mode, the alt screen and mouse capture — so the editor and the TUI fought over the same tty. And it did that inside a loop over *every* line of the clicked cell, so one click on a stack trace or a grep result could launch several editors at once. The product call the issue left open ("refuse terminal editors vs suspend-and-wait") is already settled by the code: `with_suspended_tui` (external_editor.rs) is the single terminal-handoff authority created by #6165 — it pauses the pump, pops keyboard flags, tears down alt screen / mouse / bracketed paste, and restores on every path including a body that errored. `spawn_editor_for_path` already wraps it for "edit a real file in place", and `/hooks edit` has been using exactly that. The click path was simply never routed through it. So this deletes the duplicate rather than adding an abstraction: - `try_open_file_at_line` becomes `first_file_line_reference`, a pure parser returning the *first* resolvable `(path, line)`. It spawns nothing, and `use std::process::Command` leaves history.rs with it. One click is one request to open one file. - The context-menu arm calls `spawn_editor_for_path`, which required threading `terminal` into `handle_context_menu_action`. `terminal` was already in scope at the single call site in `handle_view_events`. - Editor resolution now comes from `resolve_editor` + `split_command` like every other path, which fixes two latent bugs for free: the duplicate fell back to `vim` where the rest of the product falls back to `vi`, and it never shlex-split, so a perfectly ordinary `EDITOR="code --wait"` could not work. The one genuinely new piece is `push_target_args`, because `spawn_editor_for_path` had no way to express a line number. All three file-opening paths go through it so they cannot drift: `+N` for the vi family, `--goto file:line` for VS Code and its forks, bare `file:line` for Zed/Sublime/JetBrains, and the bare path for anything unrecognized — opening the right file at the wrong line beats handing an editor an argument it would treat as a second file to open. With `line: None` it is byte-identical to the `cmd.arg(path)` it replaces, which is what keeps `/hooks edit` unchanged. The status line now distinguishes "closed the editor" from "the editor never opened", instead of reporting success for any spawn that merely started. Not changed: `with_suspended_tui` itself, and the `looks_like_file_path` heuristic (tool_output.rs still imports it). Verification (macOS aarch64): cargo check -p codewhale-tui --all-features --locked --all-targets (clean) cargo fmt --all -- --check (clean) sh scripts/with-hermetic-test-home.sh cargo test -p codewhale-tui --lib \ --all-features --locked -j 5 -- --test-threads=2 \ first_file_line_reference external_editor push_target_args test result: ok. 15 passed; 0 failed; 0 ignored; 0 measured; 12798 filtered out Three new tests: one-match-only and the skip/malformed rows for the parser, and the per-family line spelling including the `line: None` byte-identity case. Not verified here: the actual terminal handoff on a real tty. The suspend path is unchanged and already covered by `suspend_tui_child_modes_*`, but this specific click-to-editor round trip wants a human at a terminal. Signed-off-by: CodeWhale Bot Co-Authored-By: Claude Opus 5 (1M context) --- crates/tui/src/tui/external_editor.rs | 98 +++++++++++++++++++++-- crates/tui/src/tui/history.rs | 66 +++++++-------- crates/tui/src/tui/history/tests.rs | 49 ++++++++++++ crates/tui/src/tui/history/tool_output.rs | 2 +- crates/tui/src/tui/mouse_ui.rs | 37 +++++++-- crates/tui/src/tui/ui/apply.rs | 2 + crates/tui/src/tui/ui/handlers.rs | 4 +- 7 files changed, 210 insertions(+), 48 deletions(-) diff --git a/crates/tui/src/tui/external_editor.rs b/crates/tui/src/tui/external_editor.rs index 063e79a560..e7b7553228 100644 --- a/crates/tui/src/tui/external_editor.rs +++ b/crates/tui/src/tui/external_editor.rs @@ -114,13 +114,58 @@ pub fn run_editor_raw(seed: &str) -> io::Result { } } +/// Append the file (and optional line) arguments in the spelling `program` +/// understands. +/// +/// Every caller that opens a real file goes through here so the three paths +/// cannot drift on how a line number is spelled. Without a line this is exactly +/// `cmd.arg(path)`, which is what it has always been. +/// +/// `file_stem` rather than the whole program name, so an absolute path and a +/// Windows `.exe` suffix both still match. An editor we do not recognize gets +/// the bare path: opening the right file at the wrong line beats a spurious +/// argument the editor treats as a second file to open. +fn push_target_args(cmd: &mut Command, program: &str, path: &std::path::Path, line: Option) { + let Some(line) = line else { + cmd.arg(path); + return; + }; + let stem = std::path::Path::new(program) + .file_stem() + .and_then(|s| s.to_str()) + .unwrap_or(program) + .to_ascii_lowercase(); + + match stem.as_str() { + // The vi family and the editors that copied its `+N` convention. + "vi" | "vim" | "nvim" | "view" | "gvim" | "nano" | "pico" | "emacs" | "emacsclient" + | "kak" | "micro" | "joe" => { + cmd.arg(format!("+{line}")); + cmd.arg(path); + } + // VS Code and its forks need an explicit flag before `file:line`. + "code" | "code-insiders" | "codium" | "vscodium" | "cursor" | "windsurf" => { + cmd.arg("--goto"); + cmd.arg(format!("{}:{line}", path.display())); + } + // Sublime, Zed and JetBrains launchers all take `file:line` directly. + "subl" | "sublime_text" | "zed" | "idea" | "pycharm" | "goland" | "clion" | "rustrover" + | "webstorm" => { + cmd.arg(format!("{}:{line}", path.display())); + } + _ => { + cmd.arg(path); + } + } +} + /// Run the external editor on a real file, in place. /// /// Unlike [`run_editor_raw`] there is no temp file and no seed: the file on /// disk *is* the document, so a `hooks.toml` the user edits stays edited even /// if the editor exits non-zero. The outcome only reports whether the bytes /// moved, which is what the caller needs in order to decide whether to reload. -pub fn run_editor_on_path(path: &std::path::Path) -> io::Result { +pub fn run_editor_on_path(path: &std::path::Path, line: Option) -> io::Result { let before = fs::read_to_string(path).unwrap_or_default(); let raw = resolve_editor(); @@ -132,7 +177,7 @@ pub fn run_editor_on_path(path: &std::path::Path) -> io::Result { if parts.len() > 1 { cmd.args(&parts[1..]); } - cmd.arg(path); + push_target_args(&mut cmd, &parts[0], path, line); let status = match cmd.status() { Ok(status) => status, Err(_) => return Ok(EditorOutcome::Cancelled), @@ -161,13 +206,14 @@ pub(crate) fn spawn_editor_for_path( use_mouse_capture: bool, use_bracketed_paste: bool, path: &std::path::Path, + line: Option, ) -> io::Result { with_suspended_tui( terminal, use_alt_screen, use_mouse_capture, use_bracketed_paste, - || run_editor_on_path(path), + || run_editor_on_path(path, line), ) } @@ -333,7 +379,7 @@ mod tests { unsafe { env::set_var("VISUAL", "true") }; unsafe { env::remove_var("EDITOR") }; assert_eq!( - run_editor_on_path(&path).unwrap(), + run_editor_on_path(&path, None).unwrap(), EditorOutcome::Unchanged, "an editor that changes nothing must not trigger a reload" ); @@ -344,7 +390,7 @@ mod tests { use std::os::unix::fs::PermissionsExt as _; fs::set_permissions(&script, fs::Permissions::from_mode(0o755)).unwrap(); unsafe { env::set_var("VISUAL", script.to_str().unwrap()) }; - match run_editor_on_path(&path).unwrap() { + match run_editor_on_path(&path, None).unwrap() { EditorOutcome::Edited(text) => assert!(text.contains("# seed") && text.contains('x')), other => panic!("expected Edited, got {other:?}"), } @@ -354,6 +400,48 @@ mod tests { ); } + /// The line argument is spelled per editor family, and an unknown editor + /// gets the bare path rather than an argument it would treat as a file. + #[test] + fn push_target_args_spells_the_line_per_editor_family() { + use std::ffi::OsStr; + + let path = std::path::Path::new("/w/src/main.rs"); + let args_for = |program: &str, line: Option| -> Vec { + let mut cmd = Command::new(program); + push_target_args(&mut cmd, program, path, line); + cmd.get_args() + .map(OsStr::to_string_lossy) + .map(|s| s.into_owned()) + .collect() + }; + + // No line: byte-identical to the historical `cmd.arg(path)`. + assert_eq!(args_for("vim", None), vec!["/w/src/main.rs"]); + assert_eq!(args_for("code", None), vec!["/w/src/main.rs"]); + + // vi family, including an absolute path and a .exe suffix. + assert_eq!(args_for("vim", Some(12)), vec!["+12", "/w/src/main.rs"]); + assert_eq!(args_for("nano", Some(3)), vec!["+3", "/w/src/main.rs"]); + assert_eq!( + args_for("/usr/bin/nvim", Some(9)), + vec!["+9", "/w/src/main.rs"] + ); + // `.exe` is stripped by `file_stem` on every platform. A backslashed + // Windows *path* only splits on Windows, so it is not asserted here. + assert_eq!(args_for("vim.exe", Some(5)), vec!["+5", "/w/src/main.rs"]); + + // VS Code and forks need the flag; Zed/Sublime/JetBrains take file:line. + assert_eq!( + args_for("code", Some(42)), + vec!["--goto", "/w/src/main.rs:42"] + ); + assert_eq!(args_for("zed", Some(42)), vec!["/w/src/main.rs:42"]); + + // Unknown editor: open the right file, never an invented argument. + assert_eq!(args_for("my-editor", Some(42)), vec!["/w/src/main.rs"]); + } + #[test] fn resolve_editor_prefers_visual_over_editor() { let _lock = ENV_LOCK.lock().unwrap(); diff --git a/crates/tui/src/tui/history.rs b/crates/tui/src/tui/history.rs index da6006f0ee..9519221684 100644 --- a/crates/tui/src/tui/history.rs +++ b/crates/tui/src/tui/history.rs @@ -77,8 +77,6 @@ pub use tool_output::{ OutputRow, summarize_mcp_output, summarize_tool_args, summarize_tool_output, }; -use std::process::Command; - /// Render mode controlling whether tool/thinking cells render their compact /// "live" form (with caps and collapsed reasoning) or their full transcript /// form (uncapped, suitable for the pager / clipboard / message export). @@ -2934,44 +2932,40 @@ fn tool_value_style() -> Style { /// Scans lines of `text` for patterns like `src/main.rs:42`. Resolves the path /// relative to `workspace` (if not absolute) and opens the editor. Returns /// `true` if at least one file was opened successfully. -pub fn try_open_file_at_line(text: &str, workspace: &Path) -> bool { - let editor = std::env::var("VISUAL") - .ok() - .filter(|s| !s.trim().is_empty()) - .or_else(|| { - std::env::var("EDITOR") - .ok() - .filter(|s| !s.trim().is_empty()) - }) - .unwrap_or_else(|| "vim".to_string()); - - let mut any_opened = false; +/// Find the first `path:line` reference in a rendered cell. +/// +/// Pure: it resolves and stats candidate paths but never launches anything. +/// Spawning the editor belongs to `external_editor`, which owns the terminal +/// handoff — this used to build its own `Command` and `spawn()` it detached +/// while the TUI still held raw mode, the alt screen and mouse capture, and it +/// did that once per matching line, so one click could leave N editors fighting +/// the TUI for the same tty (#6235). +/// +/// Returns the first match rather than every match: a click is one request to +/// open one file. +pub(crate) fn first_file_line_reference(text: &str, workspace: &Path) -> Option<(PathBuf, u32)> { for line in text.lines() { let trimmed = line.trim(); - if let Some((before, after)) = trimmed.rsplit_once(':') - && after.chars().all(|c| c.is_ascii_digit()) - { - let line_num: u32 = after.parse().unwrap_or(1); - let path_str = before.trim(); - if !path_str.is_empty() && looks_like_file_path(path_str) { - let abs_path = if Path::new(path_str).is_absolute() { - PathBuf::from(path_str) - } else { - workspace.join(path_str) - }; - if abs_path.is_file() - && Command::new(&editor) - .arg(format!("+{line_num}")) - .arg(&abs_path) - .spawn() - .is_ok() - { - any_opened = true; - } - } + let Some((before, after)) = trimmed.rsplit_once(':') else { + continue; + }; + if after.is_empty() || !after.chars().all(|c| c.is_ascii_digit()) { + continue; + } + let path_str = before.trim(); + if path_str.is_empty() || !looks_like_file_path(path_str) { + continue; + } + let abs_path = if Path::new(path_str).is_absolute() { + PathBuf::from(path_str) + } else { + workspace.join(path_str) + }; + if abs_path.is_file() { + return Some((abs_path, after.parse().unwrap_or(1))); } } - any_opened + None } /// Heuristic check whether a string looks like a file path (contains a diff --git a/crates/tui/src/tui/history/tests.rs b/crates/tui/src/tui/history/tests.rs index e5ff3e3e67..86aaca225e 100644 --- a/crates/tui/src/tui/history/tests.rs +++ b/crates/tui/src/tui/history/tests.rs @@ -2651,3 +2651,52 @@ fn superseded_todo_snapshots_collapse_to_their_header() { "the collapsed row keeps the progress reading: {header}" ); } + +/// One click is one request to open one file. +/// +/// The old `try_open_file_at_line` looped over every line of the cell and +/// spawned a detached editor per match, so a stack trace or a grep result could +/// launch several at once, all fighting the still-raw-mode TUI for the tty +/// (#6235). The parser now returns the first resolvable reference and nothing +/// else; spawning belongs to `external_editor`. +#[test] +fn first_file_line_reference_returns_one_match_and_resolves_it() { + let dir = tempfile::tempdir().unwrap(); + let workspace = dir.path(); + std::fs::create_dir_all(workspace.join("src")).unwrap(); + std::fs::write(workspace.join("src/first.rs"), "fn a() {}\n").unwrap(); + std::fs::write(workspace.join("src/second.rs"), "fn b() {}\n").unwrap(); + + let text = "note: two frames below\n src/first.rs:12\n src/second.rs:34\n"; + let (path, line) = super::first_file_line_reference(text, workspace) + .expect("the first resolvable reference is returned"); + assert_eq!(path, workspace.join("src/first.rs")); + assert_eq!(line, 12); +} + +#[test] +fn first_file_line_reference_skips_unresolvable_and_malformed_rows() { + let dir = tempfile::tempdir().unwrap(); + let workspace = dir.path(); + std::fs::create_dir_all(workspace.join("src")).unwrap(); + std::fs::write(workspace.join("src/real.rs"), "fn a() {}\n").unwrap(); + + // A path that does not exist, a bare word, a non-numeric suffix and an + // empty suffix all fall through to the one row that resolves. + let text = concat!( + " src/missing.rs:9\n", + " notafile:12\n", + " src/real.rs:abc\n", + " src/real.rs:\n", + " src/real.rs:7\n", + ); + let (path, line) = + super::first_file_line_reference(text, workspace).expect("the only resolvable row wins"); + assert_eq!(path, workspace.join("src/real.rs")); + assert_eq!(line, 7); + + assert!( + super::first_file_line_reference("no references here\n", workspace).is_none(), + "a cell with nothing to open must report nothing, not a default" + ); +} diff --git a/crates/tui/src/tui/history/tool_output.rs b/crates/tui/src/tui/history/tool_output.rs index 99c36f2f6b..20c54a89cc 100644 --- a/crates/tui/src/tui/history/tool_output.rs +++ b/crates/tui/src/tui/history/tool_output.rs @@ -644,7 +644,7 @@ fn is_path_or_url_like(line: &str) -> bool { } /// Detect whether a line contains a `path:line` pattern that could be -/// opened by `try_open_file_at_line`. Returns a distinctive style +/// opened by `first_file_line_reference`. Returns a distinctive style /// (underline + blue) when the pattern matches, or `None` otherwise. /// The style is applied over the existing value style so the line /// remains readable. diff --git a/crates/tui/src/tui/mouse_ui.rs b/crates/tui/src/tui/mouse_ui.rs index 12f4a6a46f..770f391680 100644 --- a/crates/tui/src/tui/mouse_ui.rs +++ b/crates/tui/src/tui/mouse_ui.rs @@ -1601,7 +1601,11 @@ pub(crate) fn transcript_cell_index_from_mouse(app: &App, mouse: MouseEvent) -> .map(|(cell_index, _)| cell_index) } -pub(crate) fn handle_context_menu_action(app: &mut App, action: ContextMenuAction) { +pub(crate) fn handle_context_menu_action( + terminal: &mut ratatui::Terminal>, + app: &mut App, + action: ContextMenuAction, +) { match action { ContextMenuAction::CopySelection => { copy_active_selection(app); @@ -1678,10 +1682,33 @@ pub(crate) fn handle_context_menu_action(app: &mut App, action: ContextMenuActio }), width, ); - if crate::tui::history::try_open_file_at_line(&text, &app.workspace) { - app.status_message = Some("Opened file in editor".to_string()); - } else { - app.status_message = Some("No file:line pattern found in selection".to_string()); + match crate::tui::history::first_file_line_reference(&text, &app.workspace) { + // The editor gets the terminal through the same suspend path + // the composer and `/hooks edit` use, one at a time, and we + // wait for it. It used to be spawned detached while the TUI + // still held raw mode, the alt screen and mouse capture (#6235). + Some((path, line)) => { + let outcome = crate::tui::external_editor::spawn_editor_for_path( + terminal, + app.use_alt_screen(), + app.use_mouse_capture, + app.use_bracketed_paste, + &path, + Some(line), + ); + app.needs_redraw = true; + app.status_message = Some(match outcome { + Ok(crate::tui::external_editor::EditorOutcome::Cancelled) => { + format!("Editor exited without opening {}", path.display()) + } + Ok(_) => format!("Closed editor for {}:{line}", path.display()), + Err(error) => format!("Could not open the editor: {error}"), + }); + } + None => { + app.status_message = + Some("No file:line pattern found in selection".to_string()); + } } } ContextMenuAction::HideCell { cell_index } => { diff --git a/crates/tui/src/tui/ui/apply.rs b/crates/tui/src/tui/ui/apply.rs index a41d58aa89..0ef16e1091 100644 --- a/crates/tui/src/tui/ui/apply.rs +++ b/crates/tui/src/tui/ui/apply.rs @@ -2628,6 +2628,8 @@ fn edit_project_hooks_from_tui(terminal: &mut AppTerminal, app: &mut App, config app.use_mouse_capture, app.use_bracketed_paste, &path, + // Open the file, not a position in it: this edits hooks.toml whole. + None, ); app.needs_redraw = true; diff --git a/crates/tui/src/tui/ui/handlers.rs b/crates/tui/src/tui/ui/handlers.rs index 203eda0ab8..6cb8913cd8 100644 --- a/crates/tui/src/tui/ui/handlers.rs +++ b/crates/tui/src/tui/ui/handlers.rs @@ -2651,7 +2651,9 @@ pub(crate) async fn handle_view_events( return Ok(true); } } - ViewEvent::ContextMenuSelected { action } => handle_context_menu_action(app, action), + ViewEvent::ContextMenuSelected { action } => { + handle_context_menu_action(terminal, app, action) + } ViewEvent::SkillMutationRequested { request } => { handle_skill_mutation_requested(app, request).await; }