fix(tui): hide shell prompt guidance when shell is disabled#2638
fix(tui): hide shell prompt guidance when shell is disabled#2638reidliu41 wants to merge 1 commit into
Conversation
Thread allow_shell into system prompt composition and remove shell-only guidance when shell tools are not available. This keeps the prompt aligned with the runtime tool catalog and prevents the model from trying exec_shell or task_shell_* after allow_shell = false.
|
Thanks @reidliu41 for taking the time to contribute. This repository is currently observing a maintainer-managed contribution gate in dry-run mode, so this pull request is staying open. When enforcement is enabled, pull requests from contributors who are not listed in Please read |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to dynamically omit shell-only workflows and guidance from system prompts when shell tools are unavailable (controlled by the allow_shell configuration). It adds the allow_shell field to PromptSessionContext, implements helper functions to filter out shell-related guidance and markdown sections (such as ### exec_shell), and adds corresponding unit tests. A critical issue was identified in the remove_markdown_section helper function, where searching only for the next \n### heading can cause it to miss higher-level headings (like # or ## ), potentially resulting in the accidental deletion of unrelated sections. A code suggestion was provided to correctly find the boundary of the section by searching for any equal or higher-level heading.
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.
| let end = prompt[after_heading..] | ||
| .find("\n### ") | ||
| .map(|offset| after_heading + offset) | ||
| .unwrap_or(prompt.len()); |
There was a problem hiding this comment.
If the next heading after the target section is of a higher level (e.g., ## or # ), find("\n### ") will fail to match it. It will instead continue searching and potentially match a ### heading much further down the document (or reach the end of the document if none exists), resulting in the accidental deletion of unrelated sections (such as ## Environment or ## Authority Recap).
To fix this, we should search for the next heading of equal or higher level (i.e., # , ## , or ### ) to correctly identify the boundary of the level 3 section.
| let end = prompt[after_heading..] | |
| .find("\n### ") | |
| .map(|offset| after_heading + offset) | |
| .unwrap_or(prompt.len()); | |
| let end = ["\n# ", "\n## ", "\n### "] | |
| .iter() | |
| .filter_map(|sep| prompt[after_heading..].find(sep)) | |
| .min() | |
| .map(|offset| after_heading + offset) | |
| .unwrap_or(prompt.len()); |
| fn remove_shell_tool_guidance(prompt: &str) -> String { | ||
| let prompt = prompt | ||
| .lines() | ||
| .filter(|line| !is_shell_disabled_prompt_line(line)) | ||
| .collect::<Vec<_>>() | ||
| .join("\n"); | ||
| let prompt = remove_markdown_section(&prompt, "### `exec_shell`"); | ||
| let prompt = prompt.replace( | ||
| "; for GitHub issue/PR/release triage, prefer the native `gh ... --json` CLI through shell because it is authenticated, structured, and reproducible; `github_issue_context` / `github_pr_context` are read-only fallbacks when the CLI route is unavailable;", | ||
| "; for GitHub issue/PR/release triage, use `github_issue_context` / `github_pr_context` as read-only routes when shell tools are unavailable;", | ||
| ); | ||
| prompt.replace( | ||
| "Use deterministic Python inside RLM for exact counts and structured aggregation; use `grep_files` or `exec_shell` directly when that is the clearest deterministic check.", | ||
| "Use deterministic Python inside RLM for exact counts and structured aggregation; use `grep_files` directly when that is the clearest deterministic check.", | ||
| ) | ||
| } | ||
|
|
||
| fn is_shell_disabled_prompt_line(line: &str) -> bool { | ||
| line.starts_with("- Arithmetic, math, calculations → `exec_shell`") | ||
| || line.starts_with("- Hashes, encodings, checksums → `exec_shell`") | ||
| || line.starts_with("- Current time, date, timezone → `exec_shell`") | ||
| || line | ||
| .starts_with("- System state: OS, CPU, memory, disk, ports, processes → `exec_shell`") | ||
| || line.starts_with("- **Shell**:") | ||
| } |
There was a problem hiding this comment.
Fragile exact-string filtering
remove_shell_tool_guidance relies on exact starts_with prefixes and verbatim replace substrings to strip shell references. If any of those strings in the base prompt are ever rephrased, reordered, or split across lines, the filter silently becomes a no-op and shell guidance will leak back into shell-disabled prompts without any compile-time or test failure at the point of change. The existing tests catch a regression today, but only if run after touching the prompt text. Consider a more structure-aware approach (e.g., tagging removable blocks at definition time rather than matching them at composition time).
|
Thanks @reidliu41 for the focused shell-disabled guidance fix. The v0.8.52 release repair is now published across GitHub/Cargo/npm, so this can move into the v0.8.53 review sweep instead of being lost in the release cleanup. Sorry the queue was noisy today; I am tracking the post-release PR pass in #2645. |
Summary
Fixes #2637.
When
allow_shell = false, the runtime tool catalog correctly removes shelltools such as
exec_shellandtask_shell_start, but the system prompt stilladvertised those tools in the Toolbox and mandatory tool-use guidance.
That mismatch could cause the model to try calling shell tools that are not
available, wasting a turn with a missing-tool error.
This PR threads shell availability into prompt composition and removes
shell-only guidance when shell tools are disabled, while keeping non-shell
tool guidance intact.
Testing
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-featurescargo test --workspace --all-featuresChecklist
Greptile Summary
This PR fixes a mismatch between the runtime tool catalog and the system prompt when
allow_shell = false: the prompt was advertisingexec_shell/task_shell_startworkflows even though those tools had been removed at runtime, wasting a turn when the model tried to call them. The fix threadsallow_shellthroughPromptSessionContextinto prompt composition and strips shell-only guidance from the base prompt before serving it.PromptSessionContextgains a newallow_shellfield (defaulting totrue) that is populated from the active config or session state at all three call sites inengine.rsandui.rs.compose_prompt_with_approval_model_and_shellfunction conditionally invokesremove_shell_tool_guidance, which removes specific lines, theexec_shellmarkdown section, and two inline sentence fragments that reference shell tools; two new tests verify the filtering on and off.Confidence Score: 4/5
Safe to merge; the fix correctly threads shell availability into prompt composition and is backed by targeted tests.
The private
compose_mode_prompt_with_approval_and_modelfunction has no callers after the refactor and will produce anunusedcompiler warning. The string-based filtering inremove_shell_tool_guidanceis functional and tested, but is tightly coupled to exact prompt wording — a future prompt edit could silently re-expose shell guidance without a failing build. Neither issue affects runtime correctness today.crates/tui/src/prompts.rs — dead private function and fragile string-matching logic worth cleaning up before the prompt text evolves further.
Important Files Changed
allow_shelltoPromptSessionContext, introducesremove_shell_tool_guidance+remove_markdown_sectionhelpers, and wires shell-aware prompt composition. A private helper (compose_mode_prompt_with_approval_and_model) is now dead code; the string-based filtering approach is tested but fragile.allow_shellintoPromptSessionContext; one usesconfig.allow_shell(initial setup) and the other usessession.allow_shell(prompt refresh) — both are kept in sync and are correct.dispatch_user_messageupdated to propagateapp.allow_shellintoPromptSessionContext; straightforward and consistent with the other two call sites.Sequence Diagram
sequenceDiagram participant UI as ui.rs (dispatch_user_message) participant Engine as engine.rs participant Prompts as prompts.rs UI->>Engine: "PromptSessionContext { allow_shell: app.allow_shell }" Engine->>Prompts: system_prompt_for_mode_with_context_skills_session_and_approval(ctx) Prompts->>Prompts: compose_prompt_with_approval_model_and_shell(allow_shell) alt "allow_shell && mode != Plan" Prompts->>Prompts: "render_base_prompt_for_tool_availability(shell_available=true)" Note over Prompts: Full prompt with exec_shell / task_shell_start guidance else shell disabled or Plan mode Prompts->>Prompts: "render_base_prompt_for_tool_availability(shell_available=false)" Prompts->>Prompts: remove_shell_tool_guidance(prompt) Note over Prompts: Strip Shell lines, exec_shell section, inline refs end Prompts-->>Engine: SystemPrompt (shell guidance conditionally removed) Engine-->>UI: Updated system prompt stored in sessionComments Outside Diff (1)
crates/tui/src/prompts.rs, line 890-902 (link)compose_mode_prompt_with_approval_and_modelhas no remaining call sites — the only caller (system_prompt_for_mode_with_context_skills_session_and_approval) was switched directly tocompose_prompt_with_approval_model_and_shellin this PR. The private function is now unreachable and the Rust compiler will emit anunusedwarning. It can be removed entirely.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (1): Last reviewed commit: "fix(tui): hide shell prompt guidance whe..." | Re-trigger Greptile