From d1fd84a574b3d837bad3346d607bcdd6311b2030 Mon Sep 17 00:00:00 2001 From: Yogthos Date: Mon, 6 Jul 2026 11:09:37 -0400 Subject: [PATCH] Let review modes run read-only bash (git diff/log/grep) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit review and review-security carried deny_tools:[…, bash, …], and a whole-tool bash deny is terminal at PromptDenyPolicy (precedence #1) — so it pre-empted the default_bash_rules that already auto-allow git status/log/diff/show and read-only inspection. The reviewer couldn't even see the diff it was asked to review. Drop bash from both review prompts' deny_tools. Read-only git and the other pre-trusted commands now flow through the normal engine (default rules → allow; effectful commands → Ask/approval_provider); plan mode keeps its full read-only lock. No engine change — an initial BuiltinAllow read-only-git carve-out was redundant with default_bash_rules and dropped; only a clarifying comment remains. Regression test pins that both review prompts deny the file-mutating tools + webfetch but keep bash. --- .beads/issues.jsonl | 1 + docs/features.md | 2 +- docs/prompts.md | 4 ++-- prompts/review-security.md | 4 ++-- prompts/review.md | 4 ++-- src/context/prompts.rs | 38 +++++++++++++++++++++++++++++++ src/permission/engine/policies.rs | 5 +++- 7 files changed, 50 insertions(+), 8 deletions(-) diff --git a/.beads/issues.jsonl b/.beads/issues.jsonl index 079c4fe9..9ac4f83f 100644 --- a/.beads/issues.jsonl +++ b/.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} diff --git a/docs/features.md b/docs/features.md index 114bad4a..f8522b94 100644 --- a/docs/features.md +++ b/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/.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/.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/.md`, `~/.config/dirge/agents/.md`, or `config.json` `agents` (layered project > global > config). Switch the active persona mid-session with `/agent ` — 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="")` — 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. diff --git a/docs/prompts.md b/docs/prompts.md index 81a11c0f..9129abf5 100644 --- a/docs/prompts.md +++ b/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 | diff --git a/prompts/review-security.md b/prompts/review-security.md index f7b1d67f..c2648e1a 100644 --- a/prompts/review-security.md +++ b/prompts/review-security.md @@ -1,5 +1,5 @@ --- -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 --- @@ -7,7 +7,7 @@ critic: false 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." diff --git a/prompts/review.md b/prompts/review.md index c09616ac..79be80da 100644 --- a/prompts/review.md +++ b/prompts/review.md @@ -1,5 +1,5 @@ --- -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 --- @@ -7,7 +7,7 @@ critic: false 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." diff --git a/src/context/prompts.rs b/src/context/prompts.rs index 8e124f8e..4e2d1afc 100644 --- a/src/context/prompts.rs +++ b/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"; diff --git a/src/permission/engine/policies.rs b/src/permission/engine/policies.rs index 1df7906e..6439ea7f 100644 --- a/src/permission/engine/policies.rs +++ b/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