fix: gate task_shell_start behind allow_shell like exec_shell

[HUQIANTAO]

Problem

In Agent mode with default config, exec_shell is blocked (allow_shell defaults to false) but task_shell_start bypasses the same gate entirely. Since task_shell_start delegates directly to ExecShellTool, this creates an inconsistent security boundary.

The model tries exec_shell first (prompted by base.md), fails, then falls back to task_shell_start — wasting tokens and providing an unrestricted path to shell execution.

Fix

Split TaskShellStartTool and TaskShellWaitTool out of with_runtime_task_tools() into a new with_runtime_task_shell_tools() method. Gate both behind the allow_shell check in with_agent_tools() and the feature-flag gate in tool_setup.rs.

Also updates MODES.md to clarify that Agent mode shell access requires allow_shell = true.

Changes

• registry.rs: Split shell task tools, gate behind allow_shell
• tool_setup.rs: Add shell task tools to feature-flag gate
• MODES.md: Clarify Agent mode shell requirements
• Added 2 unit tests

Closes #2303

Greptile Summary

This PR fixes a security inconsistency where task_shell_start and task_shell_wait bypassed the allow_shell gate that already blocked exec_shell, creating an unintended shell execution path in Agent mode.

• tools/registry.rs: TaskShellStartTool and TaskShellWaitTool are split out of with_runtime_task_tools() into a new with_runtime_task_shell_tools() method, then conditionally wired into with_agent_tools() only when allow_shell is true. Two unit tests cover both branches.
• tool_setup.rs: The now-redundant feature-flag block that re-registered shell tools from outside with_agent_tools is removed, eliminating the pre-existing double-registration of exec_shell.
• docs/MODES.md: Agent-mode description updated to explicitly list all three gated tools and the allow_shell = true requirement.

Confidence Score: 5/5

Safe to merge. The shell-tool gating logic is correctly consolidated into with_agent_tools(), Plan mode is unaffected by its separate builder path, and YOLO mode correctly forces allow_shell=true at the session layer.

The fix is narrowly scoped — it moves two tool registrations into a guarded branch and removes a now-redundant block — and the two new unit tests directly exercise both allow_shell branches. All shell execution paths now converge on the same gate.

No files require special attention.

Important Files Changed

Filename	Overview
crates/tui/src/tools/registry.rs	Core fix: TaskShellStartTool/TaskShellWaitTool moved to new with_runtime_task_shell_tools() and gated behind allow_shell in with_agent_tools(); two unit tests added covering both true/false cases.
crates/tui/src/core/engine/tool_setup.rs	Removes the now-redundant feature-flag block that called with_shell_tools() a second time, eliminating double-registration; shell tool gating is now owned entirely by with_agent_tools().
crates/tui/src/lsp/registry.rs	Formatting-only change: test assertions for detect_language condensed onto single lines; no functional change.
docs/MODES.md	Agent-mode bullet updated to name all three gated tools (exec_shell, task_shell_start, task_shell_wait) and state the allow_shell = true requirement.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[build_turn_tool_registry_builder] -->|mode == Plan| B[read-only builder path\nwith_runtime_read_only_task_tools]
    A -->|mode == Agent / YOLO| C[with_agent_tools allow_shell]
    C --> D[with_file_tools\nwith_search_tools\nwith_runtime_task_tools\n... shared tools]
    C -->|allow_shell == false| E[builder returned\nno shell tools]
    C -->|allow_shell == true| F[with_shell_tools\nregisters exec_shell]
    F --> G[with_runtime_task_shell_tools\nregisters task_shell_start\ntask_shell_wait]
    B --> H[No shell tools ever registered]
    E --> H

Loading

Comments Outside Diff (1)

1. crates/tui/src/core/engine/tool_setup.rs, line 84-89 (link)

   Double-registration of shell task tools

   build_turn_tool_registry_builder calls with_agent_tools(self.session.allow_shell) at line 58, which — after this PR — already invokes with_runtime_task_shell_tools() when allow_shell is true. The feature-flag block below then calls with_runtime_task_shell_tools() a second time (same for with_shell_tools()). Both registrations land in the builder's Vec, so register_all later hits HashMap::insert twice for task_shell_start and task_shell_wait, emitting a tracing::warn!("Overwriting existing tool: {}") for each. The same double-registration already existed for exec_shell pre-PR, so this is consistent, but the fix could instead drop the with_runtime_task_shell_tools() call from this block (letting with_agent_tools own it) to avoid the warning.

   

_{Reviews (3): Last reviewed commit: "style: format shell gating tests" | Re-trigger Greptile}

[gemini-code-assist]

Code Review

This pull request refactors the registration of shell-related task tools (task_shell_start and task_shell_wait) to ensure they are properly gated behind the allow_shell configuration. It introduces with_runtime_task_shell_tools to separate these from general runtime task tools, adds corresponding unit tests, and updates the documentation. A security review comment points out a potential bypass of the Feature::ShellTool feature flag when allow_shell is enabled, suggesting combining the gates to prevent redundant or unauthorized registration of shell tools.

[gemini-code-assist]

Security Gate Bypass / Redundant Registration

There is a security and correctness issue here: the feature flag check self.config.features.enabled(Feature::ShellTool) is completely bypassed when self.session.allow_shell is true.

Why this happens:
At line 58, with_agent_tools(self.session.allow_shell) is called. If self.session.allow_shell is true, with_agent_tools unconditionally registers both with_shell_tools() and with_runtime_task_shell_tools(), regardless of whether Feature::ShellTool is enabled. This makes the check in lines 85-89 redundant and ineffective at disabling shell tools when the feature flag is off.

How to fix:

1. Update line 58 to pass the combined gate:

   .with_agent_tools(self.session.allow_shell && self.config.features.enabled(Feature::ShellTool))

2. Remove this redundant block entirely (lines 84-89).

[Hmbown]

Thanks @HUQIANTAO — this is the right security/UX direction for #2303. task_shell_start should not be the back door around the same allow_shell boundary that blocks exec_shell.

I’m not merging this exact revision yet because it has two tiny cleanup items before it is release-safe: avoid double-registering task_shell_start / task_shell_wait in the engine builder path, and mention task_shell_wait alongside task_shell_start in docs/MODES.md. Once those are patched, the focused gate tests should be enough to land it quickly.

[HUQIANTAO]

Fixed both items:

1. Removed the redundant shell-tools gate in tool_setup.rs to avoid double-registration
2. Added task_shell_wait to MODES.md alongside task_shell_start

[HUQIANTAO]

Hi @Hmbown, both cleanup items are patched:

1. Removed the redundant shell-tools gate in tool_setup.rs to avoid double-registration of task_shell_start / task_shell_wait
2. Added task_shell_wait to MODES.md alongside task_shell_start

The two gate tests pass. Could you take another look when you get a chance? Thanks!

[Hmbown]

Thanks for the quick patch, @HUQIANTAO. I pushed one formatting-only follow-up so CI/lint has the exact rustfmt shape it wants. Local verification on a current-main merge simulation is green:\n\n- CARGO_TARGET_DIR=/Volumes/VIXinSSD/codewhale/target-pr2384 cargo test -p codewhale-tui agent_tools_with_allow_shell -- --nocapture\n- CARGO_TARGET_DIR=/Volumes/VIXinSSD/codewhale/target-pr2384-check cargo check -p codewhale-tui --all-features --locked\n\nThis now looks mergeable to me once the hosted checks settle.