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; } } 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!( 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"); 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; }