fix: harvest error-message fixes from PR #2933 — better tool denial + subagent conflict messages#3041
Conversation
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Code Review
This pull request improves error handling and reporting for tools and sub-agents. Specifically, it passes through self-explanatory tool errors to avoid confusing suffix instructions, adds elapsed time to sub-agent session name conflict errors, and annotates raw provider rejections for missing models. The review feedback highlights that checking for the substring "mode" can lead to false positives (matching "model" or "modify") and suggests using word boundaries instead. Additionally, it recommends using a match guard to simplify the nested conditional logic in the child model error annotation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // "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") |
There was a problem hiding this comment.
Using lower.contains("mode") can lead to false positives because it matches common words like "model" (e.g., "Model Not Exist" or "does not exist or you do not have access") and "modify"/"modification". Since these words are extremely common in LLM/TUI contexts, they will inadvertently bypass the suffix formatting.
To prevent this, check for "mode" as a distinct word or with common boundaries (like spaces or hyphens).
| || lower.contains("mode") | |
| || lower.contains(" mode") | |
| || lower.contains("mode ") | |
| || lower.contains("mode-") |
| ToolError::PermissionDenied { message } => { | ||
| let lower = message.to_ascii_lowercase(); | ||
| // #3020: Pass through messages that already name the denial cause. | ||
| if lower.contains("mode") |
There was a problem hiding this comment.
Using lower.contains("mode") can lead to false positives because it matches common words like "model" and "modify"/"modification". Since these words are extremely common in LLM/TUI contexts, they will inadvertently bypass the suffix formatting.
To prevent this, check for "mode" as a distinct word or with common boundaries (like spaces or hyphens).
| if lower.contains("mode") | |
| if lower.contains(" mode") | |
| || lower.contains("mode ") | |
| || lower.contains("mode-") |
| _ => { | ||
| // #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() | ||
| } | ||
| } |
There was a problem hiding this comment.
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(),… 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>
c5549e4 to
c98b7ea
Compare
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
…ough verbatim, bare/model denials get the suffix; Model-Not-Exist + OpenAI-style rejections annotated; conflict error includes elapsed time; tighten mode-word predicate so 'model' no longer matches Co-Authored-By: Claude <noreply@anthropic.com> https://claude.ai/code/session_018zaP8vUfTAsrE38L6h6fw5
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Summary
Closes #3020, #2653, #2656, #2657.
Three targeted error-message improvements extracted from community
PR #2933 (author cy2311), with additional model-not-found annotation
for subagent child-model errors.
Changes
1.
dispatch.rs— Pass through self-explanatory tool errors (#2657)format_tool_errornow passes through messages that already name thecause ("switch to Agent, Goal, or YOLO mode", "enable allow_shell",
"denied by user") instead of appending a conflicting generic suffix.
Fixes the Plan-mode double-message where the model sees both "switch
to Agent mode" AND "Adjust approval mode or request permission."
2.
subagent/mod.rs— Elapsed time in session-name conflict (#2656)The session-name conflict error now includes "started Ns ago" (or
"NmNs ago" for >120s) so the parent can distinguish a live worker
from a stale/failed earlier spawn.
3.
subagent/mod.rs— Model-not-found annotation (#2653)annotate_child_model_errornow catches model-not-found patterns("Model Not Exist", "does not exist", "no such model", etc.) even
when the taxonomy classifies them as Internal rather than
Authorization/State.
Verification
cargo check— cleancargo test -p codewhale-tui -- tool_error dispatch annotate_child— 60/60 passCredit: cy2311 for the dispatch.rs and subagent conflict hunks from #2933.
Co-authored-by: cy2311 cy2311@users.noreply.github.com