Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 33 additions & 4 deletions crates/tui/src/core/engine/dispatch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 } => {
Expand All @@ -117,17 +126,37 @@ 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!(
"Tool '{tool_name}' is not available: {message}. Check mode, feature flags, or tool name."
)
}
}
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."
)
}
}
}
}

Expand Down
27 changes: 27 additions & 0 deletions crates/tui/src/core/engine/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
34 changes: 32 additions & 2 deletions crates/tui/src/tools/subagent/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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()
}
}
Comment on lines +5633 to +5652

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This catch-all match arm can be simplified and made more idiomatic by using a match guard. This keeps the code flatter and avoids nesting if/else inside the match arm.

        _ if {
            let lower = err.to_ascii_lowercase();
            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)"
            )
        }
        _ => err.to_string(),

}
}

Expand Down
23 changes: 23 additions & 0 deletions crates/tui/src/tools/subagent/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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]
Expand Down
Loading