diff --git a/crates/tui/src/core/engine/dispatch.rs b/crates/tui/src/core/engine/dispatch.rs index 335639c4..1595507d 100644 --- a/crates/tui/src/core/engine/dispatch.rs +++ b/crates/tui/src/core/engine/dispatch.rs @@ -99,6 +99,15 @@ pub(super) fn caller_allowed_for_tool( requested == "direct" } +/// Whole-word check for "mode"/"modes" — a plain `contains("mode")` also +/// matched "model", letting provider model errors skip the actionable-hint +/// suffix (#3020). +fn mentions_mode_word(lower: &str) -> bool { + lower + .split(|ch: char| !ch.is_ascii_alphanumeric()) + .any(|word| word == "mode" || word == "modes") +} + pub(super) fn format_tool_error(err: &ToolError, tool_name: &str) -> String { match err { ToolError::InvalidInput { message } => { @@ -117,7 +126,16 @@ pub(super) fn format_tool_error(err: &ToolError, tool_name: &str) -> String { ), ToolError::NotAvailable { message } => { let lower = message.to_ascii_lowercase(); - if lower.contains("current tool catalog") || lower.contains("did you mean:") { + // #3020: Pass through self-explanatory messages that already name the + // cause (mode switch, allow_shell, feature flag). Avoids appending a + // conflicting "Check mode, feature flags" suffix on top of + // "switch to Agent, Goal, or YOLO mode" which confuses the model. + if lower.contains("current tool catalog") + || lower.contains("did you mean:") + || mentions_mode_word(&lower) + || lower.contains("allow_shell") + || lower.contains("feature flag") + { message.clone() } else { format!( @@ -125,9 +143,20 @@ pub(super) fn format_tool_error(err: &ToolError, tool_name: &str) -> String { ) } } - ToolError::PermissionDenied { message } => format!( - "Tool '{tool_name}' was denied: {message}. Adjust approval mode or request permission." - ), + ToolError::PermissionDenied { message } => { + let lower = message.to_ascii_lowercase(); + // #3020: Pass through messages that already name the denial cause. + if mentions_mode_word(&lower) + || lower.contains("allow_shell") + || lower.contains("denied by user") + { + message.clone() + } else { + format!( + "Tool '{tool_name}' was denied: {message}. Adjust approval mode or request permission." + ) + } + } } } diff --git a/crates/tui/src/core/engine/tests.rs b/crates/tui/src/core/engine/tests.rs index 95e752be..2a591acc 100644 --- a/crates/tui/src/core/engine/tests.rs +++ b/crates/tui/src/core/engine/tests.rs @@ -486,6 +486,33 @@ fn tool_error_messages_include_actionable_hints() { let timeout = ToolError::Timeout { seconds: 5 }; let formatted = format_tool_error(&timeout, "exec_shell"); assert!(formatted.contains("timed out")); + + // #3020: Plan-mode denials already explain the fix — pass through + // verbatim, with no conflicting "Adjust approval mode" suffix. + let plan_denied = ToolError::permission_denied( + "'exec_shell' is not available in Plan mode — switch to Agent, Goal, or YOLO mode to run commands and code.", + ); + let formatted = format_tool_error(&plan_denied, "exec_shell"); + assert_eq!( + formatted, + "'exec_shell' is not available in Plan mode — switch to Agent, Goal, or YOLO mode to run commands and code." + ); + + // Bare denials still get the actionable suffix. + let bare_denied = ToolError::permission_denied("nope"); + let formatted = format_tool_error(&bare_denied, "exec_shell"); + assert!( + formatted.contains("Adjust approval mode or request permission"), + "{formatted}" + ); + + // "model" must not satisfy the "mode" pass-through check. + let model_denied = ToolError::permission_denied("requested model is not allowed"); + let formatted = format_tool_error(&model_denied, "agent_open"); + assert!( + formatted.contains("Adjust approval mode or request permission"), + "{formatted}" + ); } #[test] diff --git a/crates/tui/src/tools/subagent/mod.rs b/crates/tui/src/tools/subagent/mod.rs index 61265544..c9991aa2 100644 --- a/crates/tui/src/tools/subagent/mod.rs +++ b/crates/tui/src/tools/subagent/mod.rs @@ -1496,8 +1496,19 @@ impl SubAgentManager { .values() .find(|existing| existing.session_name == name) { + // #3020: Include elapsed time so the parent can distinguish a + // live worker from a stale/failed earlier spawn (#2656). + let elapsed = existing.started_at.elapsed(); + let since = if elapsed.as_secs() < 120 { + format!("{}s ago", elapsed.as_secs()) + } else { + let mins = elapsed.as_secs() / 60; + let secs = elapsed.as_secs() % 60; + format!("{mins}m{secs}s ago") + }; return Err(anyhow!( - "Sub-agent session name '{name}' is already in use by agent_id '{}' (status: {}). \ + "Sub-agent session name '{name}' is already in use by agent_id '{}' \ + (status: {}, started {since}). \ Reuse that agent_id with agent_eval/agent_close, or open with a different name.", existing.id, subagent_status_name(&existing.status) @@ -5619,7 +5630,26 @@ fn annotate_child_model_error(err: &str, model: &str) -> String { "{err}\n(child model `{model}` may be unavailable under the current access profile — \ retry agent_open with a different `model`, or remove `model` to inherit the parent's)" ), - _ => err.to_string(), + _ => { + // #3020 (#2653): Provider rejections like "Model Not Exist" or + // "does not exist or you do not have access" often classify as + // `Internal` rather than `Authorization`/`State`. Catch these + // patterns in the raw error text and annotate anyway. + let lower = err.to_ascii_lowercase(); + if lower.contains("model not exist") + || lower.contains("model_not_found") + || lower.contains("does not exist") + || lower.contains("no such model") + || lower.contains("invalid model") + { + format!( + "{err}\n(child model `{model}` may be unavailable under the current access profile — \ + retry agent_open with a different `model`, or remove `model` to inherit the parent's)" + ) + } else { + err.to_string() + } + } } } diff --git a/crates/tui/src/tools/subagent/tests.rs b/crates/tui/src/tools/subagent/tests.rs index 113ea47a..85679551 100644 --- a/crates/tui/src/tools/subagent/tests.rs +++ b/crates/tui/src/tools/subagent/tests.rs @@ -1489,6 +1489,12 @@ async fn spawn_duplicate_session_name_error_names_conflicting_agent() { msg.contains("running"), "includes the conflicting status: {msg}" ); + // #3020: elapsed time lets the parent distinguish a live worker from a + // stale earlier spawn. + assert!( + msg.contains("started ") && msg.contains(" ago"), + "includes elapsed time since spawn: {msg}" + ); } #[tokio::test] @@ -2074,6 +2080,23 @@ fn annotate_child_model_error_adds_actionable_hint() { let unrelated = annotate_child_model_error("connection reset by peer", "kimi-k2"); assert_eq!(unrelated, "connection reset by peer"); + + // #3020: provider rejections that classify as Internal (not + // Authorization/State) still get the hint via raw-text matching. + let not_exist = annotate_child_model_error("Model Not Exist", "kimi-k2"); + assert!( + not_exist.contains("retry agent_open"), + "DeepSeek-style rejection gets the hint: {not_exist}" + ); + + let openai_style = annotate_child_model_error( + "The model `gpt-5.5-nano` does not exist or you do not have access to it.", + "gpt-5.5-nano", + ); + assert!( + openai_style.contains("retry agent_open"), + "OpenAI-style rejection gets the hint: {openai_style}" + ); } #[test]