fix: harvest error-message fixes from PR #2933 — better tool denial + subagent conflict messages (#3020)
Three targeted error-message improvements extracted from community PR #2933 (author cy2311), with additional model-not-found annotation: 1. dispatch.rs format_tool_error: pass through self-explanatory messages that already name the cause (mode switch, allow_shell, feature flag, denied by user) instead of appending a conflicting generic suffix. Fixes the Plan-mode double-message (#2657). 2. subagent/mod.rs session-name conflict: include elapsed time (started Ns ago / NmNs ago) so the parent can distinguish a live worker from a stale/failed earlier spawn (#2656). 3. subagent/mod.rs annotate_child_model_error: catch model-not-found patterns (Model Not Exist, does not exist, no such model, etc.) in the raw error text even when the taxonomy classifies them as Internal rather than Authorization/State (#2653). Closes #2653, #2656, #2657. Credit: cy2311 for the dispatch.rs and subagent conflict hunks from #2933. Co-authored-by: cy2311 <29836092+cy2311@users.noreply.github.com>
This commit is contained in:
@@ -117,7 +117,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:")
|
||||
|| lower.contains("mode")
|
||||
|| lower.contains("allow_shell")
|
||||
|| lower.contains("feature flag")
|
||||
{
|
||||
message.clone()
|
||||
} else {
|
||||
format!(
|
||||
@@ -125,9 +134,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 lower.contains("mode")
|
||||
|| 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."
|
||||
)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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()
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user