Let review modes run read-only bash (git diff/log/grep)

.beads/issues.jsonl

@@ -134,6 +134,7 @@
 {"_type":"issue","id":"dirge-86e","title":"ANSI injection in permission ALERT prompt","description":"ask_req.tool / ask_req.input rendered un-sanitized at mod.rs:2584-2585. Reopen path already sanitizes — asymmetric. Sec impl: ANSI at the permission-decision moment.","status":"closed","priority":1,"issue_type":"bug","assignee":"Yogthos","owner":"yogthos@gmail.com","created_at":"2026-05-21T22:17:34Z","created_by":"Yogthos","updated_at":"2026-05-21T22:26:37Z","started_at":"2026-05-21T22:17:42Z","closed_at":"2026-05-21T22:26:37Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}
 {"_type":"issue","id":"dirge-9f1","title":"Chat history ignores 120-col content_width cap","description":"max_line_width and wrap_line use raw content_cols, so on wide terminals scrollback overflows the centered band into divider/panel margin.","status":"closed","priority":1,"issue_type":"bug","assignee":"Yogthos","owner":"yogthos@gmail.com","created_at":"2026-05-21T22:17:33Z","created_by":"Yogthos","updated_at":"2026-05-21T22:26:36Z","started_at":"2026-05-21T22:17:42Z","closed_at":"2026-05-21T22:26:36Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}
 {"_type":"issue","id":"dirge-woq","title":"R1: fix 3 critical plugin bugs (FFI panic, dialog deadlock, init hang)","description":"From the plugin subsystem audit: (1) wrap JanetCFunctions in catch_unwind so Rust panics don't unwind across the C-FFI boundary into Janet; (2) cancel send_dialog's reply_rx.recv() on worker shutdown so the worker thread doesn't block forever when the UI exits mid-dialog; (3) add timeout to the init handshake so a worker panic before init_tx.send() doesn't hang the main thread. Also: (4) bounds-assert wrap_string's i32 cast for the unlikely \u003e2GB case, (5) make take_string_slot atomic to close the race window, (6) don't eat unrelated user events in the dialog arm.","status":"closed","priority":1,"issue_type":"bug","owner":"yogthos@gmail.com","created_at":"2026-05-20T14:59:57Z","created_by":"Yogthos","updated_at":"2026-05-20T15:30:28Z","started_at":"2026-05-20T15:00:10Z","closed_at":"2026-05-20T15:30:28Z","dependency_count":0,"dependent_count":1,"comment_count":0}
+{"_type":"issue","id":"dirge-k265","title":"Review modes deny all bash, blocking read-only git inspection","description":"Review mode (prompts/review.md deny_tools:[bash]) terminally denies the whole bash tool at PromptDenyPolicy precedence #1, so read-only 'git diff'/'git log' can't run and there's no way to inspect the diff. Also, read-only git prompts in every session because Operation::Execute is never builtin-allowed. Fix: (1) add a conservative read-only-git allow to BuiltinAllowPolicy (git diff/log/status/show/blame/... on a non-complex single segment, rejecting shell metachars, code-executing git global flags like -c, and write flags like --output) so observation is transparent like Operation::Read; (2) drop 'bash' from review.md deny_tools so effectful bash hits the Ask path (human/approval_provider) instead of a hard wall. Plan mode's hard deny is unchanged.","notes":"Root cause: review.md / review-security.md deny_tools include 'bash', which PromptDenyPolicy (#1) enforces as a terminal whole-tool Deny — pre-empting the existing default_bash_rules (#5) that already auto-allow git status/log/diff/show and read-only inspection tools. Fix is minimal: drop 'bash' from both review prompts' deny_tools. No engine change needed (initial BuiltinAllow read-only-git idea was redundant with default_bash_rules and reverted). Effectful bash in review still governed by the Ask path (default rules + approval_provider); plan mode keeps its full bash deny. Caveat: default_bash_rules also allow git add/commit + cargo/make/test runners in review mode now — tightening that to strictly read-only needs sub-tool granularity (not built).","status":"in_progress","priority":2,"issue_type":"feature","assignee":"Yogthos","owner":"yogthos@gmail.com","created_at":"2026-07-06T14:57:14Z","created_by":"Yogthos","updated_at":"2026-07-06T15:09:27Z","started_at":"2026-07-06T14:57:22Z","dependency_count":0,"dependent_count":0,"comment_count":0}
 {"_type":"issue","id":"dirge-2emd","title":"Editor follow-along: open files dirge reads/edits in an external editor (issue #572)","description":"GH #572 (ericschmar): let the user's editor follow along with the files dirge accesses. Adaptation of the external-editor feature (#592): opt-in config editor_open_command (template with {path}/{line}); when set, dirge launches it DETACHED (non-blocking, opens in Zed/VS Code/etc. window) each time it reads/edits a file, passing the resolved abs path + line. Like Zed's AI panel. Terminal/editor-agnostic, no click/OSC8 dependency. Interactive TUI only. Dedup consecutive identical launches.","status":"closed","priority":2,"issue_type":"feature","assignee":"Yogthos","owner":"yogthos@gmail.com","created_at":"2026-07-06T02:46:36Z","created_by":"Yogthos","updated_at":"2026-07-06T03:03:41Z","started_at":"2026-07-06T02:47:41Z","closed_at":"2026-07-06T03:03:41Z","close_reason":"Opt-in editor follow-along (GH #572): editor_open_command config template ({path}/{line}); on each read/edit tool call dirge resolves the abs path and launches the command detached (setsid, null stdio, reaped in a thread so no zombies), deduped against the last launch. Pure helpers build_editor_open_argv + file_target_for_tool (22 unit tests). Best-effort, opt-in, TUI-only. Implemented via dirge delegation; reviewed (added child-reaping), fmt/clippy/635 UI tests pass.","dependency_count":0,"dependent_count":0,"comment_count":0}
 {"_type":"issue","id":"dirge-rwru","title":"Agent halts mid-work after an unresolved tool failure; needs manual user nudge","description":"dirge's loop re-prompts only while the model keeps emitting tool calls. When the model ends a turn text-only (stop_reason=Stop) right after a tool call was rejected (tool_input_invalid / repair-exhausted) or otherwise errored, the run breaks and waits for the user. Every continuation nudge in poll_finalization_follow_up is conditional/one-shot (hook/verifier/critic/code-review/goal/todo), so a plain session has nothing to push the model to finish. The tool-result feedback path itself is sound (error result IS fed back, terminate=None, escalation falls back cleanly) — the gap is the absence of a general 'you stopped with an unresolved tool failure, finish it' nudge. Fix (#1, targeted+bounded): add a finalization-poll source that fires ONCE when the finalizing turn made no tool call AND the immediately-preceding tool-result group has \u003e=1 is_error, nudging the model to complete/retry or state why it can't.","notes":"Investigation: critic (run_critic) ALREADY covers case 2 (ambiguous 'is task complete' -\u003e VERDICT COMPLETE/INCOMPLETE -\u003e [critic] nudge), but gated on critic_provider, fires once/run, fail-open calibrated. GAP is case 1: a definitive failure-stop (tool call rejected/errored, model then stops text-only) is not deterministically caught. FIX = new deterministic finalization-poll source ordered BEFORE the critic: fires when the tail of new_messages is [ToolResult group with \u003e=1 is_error][final Assistant with no tool calls], injects a bounded [resume]-tagged nudge to complete/retry the failed step or explain why not. Anti-loop: only fires when the error group is immediately before the stopped turn (false once the model replies text-only) + a counter cap. Always-on + deterministic (no LLM). Case 2 unchanged.","status":"closed","priority":2,"issue_type":"bug","assignee":"Yogthos","owner":"yogthos@gmail.com","created_at":"2026-07-05T16:25:36Z","created_by":"Yogthos","updated_at":"2026-07-05T16:47:06Z","started_at":"2026-07-05T16:32:11Z","closed_at":"2026-07-05T16:47:06Z","close_reason":"Added a deterministic resume-after-failure nudge (case 1). Pure last_action_failed_and_stopped() helper fires when a tool-error group immediately precedes a stopped (no-tool-call) Assistant turn; new bounded (MAX_RESUME_NUDGE=3) finalization source ordered after hook/before verifier injects a [resume] nudge to complete or explain. Anti-loop: stops matching once the model replies text-only. Case 2 (ambiguous completeness) unchanged — stays the LLM critic's job. Implemented via dirge delegation; reviewed, escalation integration test updated for the intended extra continuation, fmt/clippy/608 agent_loop tests pass.","dependency_count":0,"dependent_count":0,"comment_count":0}
 {"_type":"issue","id":"dirge-173j","title":"Mouse/selection break: recover terminal dropped to main screen (Ctrl+L full re-assert)","status":"closed","priority":2,"issue_type":"bug","owner":"yogthos@gmail.com","created_at":"2026-07-04T03:24:50Z","created_by":"Yogthos","updated_at":"2026-07-04T03:25:31Z","started_at":"2026-07-04T03:25:20Z","closed_at":"2026-07-04T03:25:31Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}

docs/features.md

@@ -23,7 +23,7 @@ headline differentiators, see the top-level [README](../README.md).
 - **Side panels**: optional gutters auto-shown at ≥152 cols. Toggle both together with `/panel`, or pick panes individually with `/display` (e.g. `/display main`, `/display main|right`, `/display left|main|right`); set a startup default with the `display` config key. A hidden panel's gutter is reclaimed by the conversation, which widens to use the freed space rather than leaving it blank. The **right** sidebar shows system load (CPU/MEM), MCP/LSP server status, pending todos, and recently-modified files. The **left** gutter shows live session vitals when idle — a context-window fill gauge (with a "compaction soon" cue near the auto-fold threshold), a recent-tool-activity ticker, and a git working-tree snapshot (branch + staged/unstaged/untracked + last commit) — and switches to per-subagent status rows when background subagents are running.
 - **Mid-execution interjection**: type while the agent is running to queue a follow-up message — the runner stops at the next tool-result boundary so it's picked up promptly instead of waiting for the whole multi-turn run. `Alt+X` drops queued messages (leaving the run going), `Ctrl+C` cancels both the run and the queue.
 - **Prompts system**: switch between system prompt modes at runtime (`code`, `plan`, `review`, `debug`, etc.). See [prompts.md](prompts.md).
-- **Per-prompt tool restrictions**: each prompt (`prompts/<name>.md`) can declare a `deny_tools` frontmatter list. The permission checker refuses those tools while the prompt is active — a real security boundary, not a prose gate. `plan`, `review`, and `review-security` ship with `edit`, `write`, `apply_patch`, `bash`, and `webfetch` denied. See [prompts.md](prompts.md).
+- **Per-prompt tool restrictions**: each prompt (`prompts/<name>.md`) can declare a `deny_tools` frontmatter list. The permission checker refuses those tools while the prompt is active — a real security boundary, not a prose gate. `plan` ships with `edit`, `write`, `apply_patch`, `bash`, and `webfetch` denied (a full read-only lock). `review` and `review-security` deny the file-mutating tools + `webfetch` but keep `bash` so the reviewer can inspect read-only (`git diff`, `git log`, `grep`) — effectful commands still hit the normal approval prompt. See [prompts.md](prompts.md).
 - **Agent profiles** (opt-in): named personas that bundle a *system prompt*, a *model*, and a *tool policy*, defined in `.dirge/agents/<name>.md`, `~/.config/dirge/agents/<name>.md`, or `config.json` `agents` (layered project > global > config). Switch the active persona mid-session with `/agent <name>` — it applies the profile's prompt + `deny_tools`/`allow_tools` (at the permission layer) and swaps the model (same-client), so the right model-for-the-job is one keystroke away. `/agent off` reverts; `/agents` lists profiles plus the built-in role routing. The `task` tool can also spawn a **subagent under a profile** via `task(prompt=…, agent="<name>")` — the subagent runs on that profile's model + system prompt, so work fans out to specialized personas. Fully opt-in — no profiles defined means unchanged behavior, and the built-in critic/role routing is untouched. See [agents.md](agents.md).
 - **Ask-vs-proceed calibration**: the base prompt gives concrete signals for when to ask the user versus proceed — ask only when a wrong guess is costly/irreversible, can't be inferred from the code, and has genuinely divergent interpretations; otherwise proceed with the most reasonable interpretation and state the assumption. Reduces both over-asking and silently-wrong work.
 - **Progress updates**: for multi-step tool runs the agent gives a brief up-front plan and terse one-line progress notes between major steps, so long runs are steerable — scoped to progress *during* the run, leaving the terse final reply unchanged.

docs/prompts.md

@@ -10,12 +10,12 @@ restrictions enforced at the permission layer. Switch at runtime with
 |--------|-------------|
 | **`code`** (default) | Coding mode with full tool access, TDD workflow |
 | **`plan`** | Planning-only mode — `edit`/`write`/`apply_patch`/`bash`/`webfetch` are denied at the permission layer (via `deny_tools` frontmatter). Plan is delivered as the chat reply; the user saves it to disk if desired. |
-| **`review`** | Code review mode — same deny list as plan; findings delivered in chat |
+| **`review`** | Code review mode — `edit`/`write`/`apply_patch`/`webfetch` denied, but `bash` stays so the reviewer can inspect read-only (`git diff`, `git log`, `grep`); effectful commands still prompt. Findings delivered in chat |
 | **`debug`** | Debug mode — finds root cause before proposing fixes |
 | **`ask`** | Read-only mode — `edit`/`write`/`apply_patch`/`bash`/`webfetch` denied via deny_tools |
 | **`brainstorm`** | Design-only mode — explores ideas and presents designs without code |
 | **`frontend-design`** | Frontend design mode — distinctive, production-grade UI |
-| **`review-security`** | Security review mode — same deny list as plan/review; finds exploitable vulnerabilities |
+| **`review-security`** | Security review mode — same deny list as `review` (`bash` kept for read-only inspection); finds exploitable vulnerabilities |
 | **`simplify`** | Code simplification mode — refines for clarity without changing behavior |
 | **`write-prompt`** | Prompt writing mode — creates and optimizes agent prompts |
 | **`default`** | Default system prompt — the base built-in prompt |

prompts/review-security.md

@@ -1,13 +1,13 @@
 ---
-deny_tools: [edit, write, apply_patch, bash, webfetch]
+deny_tools: [edit, write, apply_patch, webfetch]
 description: Read-only security review — surface HIGH-confidence vulnerabilities
 critic: false
 ---
 ## Security Review Mode
 
 You are in **security review mode**. Identify exploitable security vulnerabilities in code. Report only HIGH CONFIDENCE findings after thorough investigation.
 
-`edit`, `write`, `apply_patch`, `bash`, and `webfetch` are denied. Deliver findings as your chat reply.
+`edit`, `write`, `apply_patch`, and `webfetch` are denied — don't change files. Read-only inspection with `bash` (e.g. `git diff`, `git log`, `grep`) is fine; effectful commands will ask for approval before running. Deliver findings as your chat reply.
 
 **Announce at start:** "I'm using the security review prompt. I will systematically review the code for vulnerabilities."
 

prompts/review.md

@@ -1,13 +1,13 @@
 ---
-deny_tools: [edit, write, apply_patch, bash, webfetch]
+deny_tools: [edit, write, apply_patch, webfetch]
 description: Read-only code review — analyze and critique, do not change files
 critic: false
 ---
 ## Code Review Mode
 
 You are in **code review mode**. Review code for correctness, design, testing, and long-term impact. Provide actionable, constructive feedback.
 
-`edit`, `write`, `apply_patch`, `bash`, and `webfetch` are denied in this mode. Deliver findings as your chat reply.
+`edit`, `write`, `apply_patch`, and `webfetch` are denied in this mode — don't change files. Read-only inspection with `bash` (e.g. `git diff`, `git log`, `git show`, `grep`) is fine; effectful commands will ask for approval before running. Deliver findings as your chat reply.
 
 **Announce at start:** "I'm using the code review prompt. I will review the changes systematically."
 

src/context/prompts.rs

@@ -510,6 +510,44 @@ mod tests {
         }
     }
 
+    /// dirge-k265: review modes deny the file-mutating tools + webfetch,
+    /// but must NOT deny `bash` — a whole-tool bash deny (precedence #1)
+    /// pre-empts the read-only-git default rules, so the reviewer couldn't
+    /// even run `git diff`. Effectful bash is instead governed by the normal
+    /// Ask path (default_bash_rules + approval_provider), not a hard wall.
+    #[test]
+    fn review_prompts_deny_writes_but_allow_bash_inspection() {
+        for file in ["review.md", "review-security.md"] {
+            let raw = EMBEDDED
+                .get_file(file)
+                .and_then(|f| f.contents_utf8())
+                .unwrap_or_else(|| panic!("embedded {file} present"));
+            let p = parse_frontmatter(raw);
+            for required in ["edit", "write", "apply_patch", "webfetch"] {
+                assert!(
+                    p.deny_tools
+                        .iter()
+                        .any(|d| d.eq_ignore_ascii_case(required)),
+                    "{file} must deny {required:?}; deny_tools = {:?}",
+                    p.deny_tools,
+                );
+            }
+            assert!(
+                !p.deny_tools.iter().any(|d| d.eq_ignore_ascii_case("bash")),
+                "{file} must NOT deny bash (blocks read-only git); deny_tools = {:?}",
+                p.deny_tools,
+            );
+            for d in &p.deny_tools {
+                assert!(
+                    crate::agent::tools::BUILTIN_TOOL_NAMES
+                        .iter()
+                        .any(|k| k.eq_ignore_ascii_case(d)),
+                    "{file} deny_tools entry {d:?} isn't a known built-in (would warn at load)",
+                );
+            }
+        }
+    }
+
     #[test]
     fn frontmatter_tolerates_quoted_tool_names_and_whitespace() {
         let raw = "---\ndeny_tools: [ \"edit\" , 'write' ,  bash ]\n---\nbody\n";

src/permission/engine/policies.rs

@@ -354,7 +354,10 @@ impl BuiltinAllowPolicy {
 
             // Execute / Network / Mcp / Agent (recursive task) / Plugin
             // are never builtin-allowed; Other (unknown) falls to
-            // configured rules or the default.
+            // configured rules or the default. Safe-bash defaults
+            // (git status/log/diff, cargo, test runners, …) live in
+            // `default_bash_rules()` as configured rules, deliberately
+            // distinct from this policy.
             Operation::Execute
             | Operation::Network
             | Operation::Mcp