chore: audit fixes (5 rounds — CRITICAL/HIGH/MEDIUM/LOW) (#54)

* fix(audit r1): path traversal, compress crashes, question permission

Round 1 of the audit fixes — security + crashes.

## Path traversal in session storage (CRITICAL)

`save_session` / `load_session` / `delete_session` interpolated
`session.id` directly into the `{id}.json` filename. Session ids
are normally UUIDs but they round-trip through JSON on disk; a
tampered-with file with `id: "../../etc/passwd"` could escape the
sessions directory on save or read arbitrary files on load.

New `validate_session_id` gate: accepts `[A-Za-z0-9._-]+` only,
explicitly rejects `.`, `..`, slashes, backslashes. Tested with
the usual escape attempts.

## Compress bounds + leaf-tracking (HIGH)

`Session::compress(_, first_kept_index, _)` did `messages.drain(..first_kept_index)`
with no bounds check — an out-of-range index from a buggy caller
panicked. Now clamped to `messages.len()` so misuse degrades to
"summarize everything" instead of crashing the agent.

Branched-session compaction also had a latent leaf-tracking bug:
if `tree.leaf_id` pointed at a branch leaf that was in the dropped
set (e.g. user forked, then compressed the alternate branch), the
leaf id was left dangling. New post-prune check re-anchors the leaf
to the first kept message (or the summary if everything was dropped).

## QuestionTool routes through permission (HIGH)

`QuestionTool::call` injected user input into the LLM's tool result
without any permission check, unlike `TaskTool` / `WriteTool` /
`BashTool` etc. Added `permission: Option<PermCheck>` +
`ask_tx: Option<AskSender>` to the struct, new `.with_permission()`
builder, and a `check_perm(&self.permission, &self.ask_tx, "question", &summary)`
at the top of `call`. Wired through `builder.rs` so production
paths get the gate; existing tests construct without permission
and continue to pass.

## Test plan

- [x] 2 new tests in `session::storage::tests` (UUID accept,
      traversal reject).
- [x] `cargo test --features plugin` -> 606 pass, 0 fail.
- [x] Both build profiles -> 0 warnings.

* fix(audit r2): on-complete dispatch, turn-hook slots, harness-response, warn unknown ops

Round 2 — silent failures.

## on-complete now fires (CRITICAL)

`on-complete` was in `HOOK_NAMES` (so plugins defining it got
auto-aliased on load) but no host site dispatched. Plugins that
defined `on-complete` ran forever without ever seeing the event.

Now fires from `AgentEvent::Done` right after `on-response` + the
pending-prompt + store_response sequence, so plugins observe a
fully-complete turn (response stored, pending prompts processed).

## Turn-hook slot reset (HIGH)

`on-turn-start` / `on-turn-end` bypassed `dispatch_tool_hook`'s
slot-clear step, so a plugin calling `(harness/block ...)` from
inside a turn hook would leave the slot set and spuriously block
the *first* tool of the next turn. Both turn hooks now reset
`harness-block` / `harness-mutate-input` / `harness-replace-result`
explicitly after the dispatch returns.

## harness-response cleared after store (HIGH)

`mgr.store_response()` wrote `harness-response` and left it set
indefinitely. Plugins reading the var in a later hook saw stale
text from previous turns. Now cleared with
`(set harness-response nil)` immediately after store, matching the
pattern other slots already use.

## drain_tree_ops warns on unknown op verbs (MEDIUM)

`parse_tree_op_line` silently dropped lines it didn't recognize
(forward-compat — good), but with no diagnostic a typo'd op verb
in a plugin would fail with no trace. Added a `tracing::warn!`
at `dirge::plugin` target so confused plugin authors can spot the
typo in the log.

## Test plan

- [x] `cargo test --features plugin` -> 606 pass, 0 fail.
- [x] Both build profiles -> 0 warnings.

* fix(audit r3): markdown theme sweep, cursor flicker, avatar idle, atomic init

Round 3 — theme + UI polish.

## Markdown theme sweep (CRITICAL)

`src/ui/markdown.rs` had 13 hardcoded color literals
(`Color::Cyan`, `Color::DarkYellow`, `Color::DarkGrey`) used for
headings, code blocks, blockquotes, and bullets. Under the
phosphor theme these forced cyan + yellow accents that broke
palette coherence. Swept to `theme::header()`, `theme::tool()`,
and `theme::dim()` so palette swaps cascade through markdown
rendering.

`bullet_prefix(col: Color)` matched on `Color::DarkGrey` as a
sentinel for "this is a blockquote." With the dim color now
themed away from DarkGrey, the sentinel was broken. Refactored
to `bullet_prefix(in_blockquote: bool)` — explicit, theme-safe.

## Cursor flicker on the right (HIGH)

`draw_bottom` called `draw_panel` while the cursor was visible —
the panel's MoveTo loop walked the hardware cursor across the
right-hand panel one cell at a time, visibly flickering. Now
hides the cursor BEFORE panel + avatar paints, places it at the
final input position, then re-shows.

## Avatar resets to Idle on user submit (MEDIUM)

After a turn completed the avatar stuck on Done forever — the
next user prompt didn't visually "wake" it. Now all three
user-message commit sites set the avatar to Idle so the brief
moment between submit and first agent token shows a neutral
face that transitions to Thinking/Speaking naturally.

## Atomic back-compat init (MEDIUM)

`ensure_message_store_initialized()` and
`ensure_tree_initialized()` were called as a pair in every
mutation method; a panic between them could leave the session
half-initialized (tree rebuilt but store empty, or vice versa).
New `ensure_back_compat_initialized()` runs both in one call.
All five mutation sites (add_message, pop_last_message,
switch_to_leaf, fork_at, compress) updated.

## Test plan

- [x] `cargo test --features plugin` -> 606 pass, 0 fail.
- [x] Both build profiles -> 0 warnings.

* fix(audit r4): permission gates, schema corrections, single-compaction record

Round 4 — schema + permission hygiene.

## TaskStatusTool routes through permission

`task_status` was the only background-task tool that didn't call
`check_perm`. The wait=true path can hold the parent turn open for
up to 10 minutes; both wait=true and wait=false leak subagent
state to the caller LLM. Now gated identically to `task`.

## PlanEnterTool / PlanExitTool stay un-gated (documented)

These tools surface a user-confirmation dialog via `plan_tx` /
`PlanSwitchResponse::{Accepted,Rejected}` — that IS the gate. A
second `check_perm` call would double-ask the user. Added a
comment in both `call` methods so future readers don't try to
"fix" the missing check.

## WebFetch max_chars: number -> integer

Schema declared `"type": "number"` for what's a `usize` in code.
LLM-emitted floats would round-trip as 0 (when fractional) or
panic in `.chars().take(n)`. Now declared as integer with a
minimum of 1.

## LSP workspaceSymbol query: schema now documents the contract

Schema marks `query` as a regular optional field, but the call
path errors when query is missing AND operation == workspaceSymbol.
Added that constraint to the parameter description so the LLM
knows when to pass it (the runtime check stays as the source of
truth).

## Compress now replaces the compactions list instead of appending

`Compaction::first_kept_index` is meaningful only for the *latest*
compaction record — keeping a list of records from earlier
compresses left stale `first_kept_index` values that no longer
matched the post-drain message indices. The LLM context already
folds older summaries into the new summary via `previous_summary`
(captured in `slash.rs:109` before compress runs), so dropping
the historical list is lossless. Fixes the multi-compaction
accounting drift flagged in the audit.

## Test plan

- [x] `cargo test --features plugin` -> 606 pass, 0 fail.
- [x] Both build profiles -> 0 warnings.

* fix(audit r5): glob cache, fork docs, switch-session diagnostics, pop fallback

Round 5 — low hygiene.

## GlobTool gets the dual-constructor pattern

Other read tools (Read/Grep/FindFiles/ListDir) follow a
`new(perm, ask_tx) / with_cache(perm, ask_tx, cache)` pattern.
`GlobTool` had only `new`, so glob calls re-walked the filesystem
every invocation even when bash/write/edit cache-clears
explicitly invalidated other tools' caches.

Added `with_cache(perm, ask_tx, cache)` and wired it through
`builder.rs`. Reuses the same `ToolCache` so a `bash`/`write`
mutation clears glob results too.

## pop_last_message: tree-corruption fallback

If `tree.entries` somehow lacks the popped message's id (data
corruption, external mutation), the old code wiped `tree.leaf_id`
to None, leaving the tree dangling on branched sessions. New
fallback uses the previous message's id from the linear cache so
the leaf stays anchored to a real node.

## fork_at: documented root behaviour

`fork_at` at the conversation root clears `messages` and sets
`leaf_id = None` — useful but surprising. Docstring now spells
this out, including the note that sibling branches survive.

## switch-session: ambiguity error lists candidate ids

`prefix 'ab' matches 5 sessions` was un-debuggable. Now lists
the first 3 matching session ids (short form) so the plugin
author / user can pick a longer prefix immediately.

## Test plan

- [x] `cargo test --features plugin` -> 606 pass, 0 fail.
- [x] Both build profiles -> 0 warnings.

---------

Co-authored-by: Yogthos <yogthos@gmail.com>

Author: yogthos

src/agent/builder.rs

@@ -194,7 +194,11 @@ pub async fn build_agent_inner<M: CompletionModel + 'static>(
                 ask_tx.clone(),
                 cache.clone(),
             )),
-            Box::new(tools::GlobTool::new(permission.clone(), ask_tx.clone())),
+            Box::new(tools::GlobTool::with_cache(
+                permission.clone(),
+                ask_tx.clone(),
+                cache.clone(),
+            )),
             Box::new(tools::ListDirTool::with_cache(
                 permission.clone(),
                 ask_tx.clone(),
@@ -217,8 +221,11 @@ pub async fn build_agent_inner<M: CompletionModel + 'static>(
             )),
         ];
 
-        let question_tool = question_tx
-            .map(|tx| Box::new(tools::QuestionTool::new(tx)) as Box<dyn rig::tool::ToolDyn>);
+        let question_tool = question_tx.map(|tx| {
+            Box::new(
+                tools::QuestionTool::new(tx).with_permission(permission.clone(), ask_tx.clone()),
+            ) as Box<dyn rig::tool::ToolDyn>
+        });
 
         let plan_tools = plan_tx.map(|tx| {
             let enter =
@@ -279,8 +286,10 @@ pub async fn build_agent_inner<M: CompletionModel + 'static>(
                 pm,
                 store.clone(),
             ));
-            let status_tool =
-                Box::new(tools::TaskStatusTool::new(store)) as Box<dyn rig::tool::ToolDyn>;
+            let status_tool = Box::new(
+                tools::TaskStatusTool::new(store)
+                    .with_permission(permission.clone(), ask_tx.clone()),
+            ) as Box<dyn rig::tool::ToolDyn>;
             builder = builder.tools(hookify(vec![task_tool, status_tool]));
         }
 

src/agent/tools/glob.rs

@@ -5,16 +5,42 @@ use serde::Deserialize;
 use std::path::Path;
 
 use crate::agent::tools::MAX_FIND_RESULTS;
+use crate::agent::tools::cache::ToolCache;
 use crate::agent::tools::{AskSender, PermCheck, ToolError, check_perm};
 
 pub struct GlobTool {
     pub permission: Option<PermCheck>,
     pub ask_tx: Option<AskSender>,
+    pub cache: Option<ToolCache>,
 }
 
 impl GlobTool {
+    /// Construct without a cache. Retained for parity with other
+    /// tools (Read/Grep/FindFiles/ListDir) and exercised by unit
+    /// tests; production paths use `with_cache`.
+    #[allow(dead_code)]
     pub fn new(permission: Option<PermCheck>, ask_tx: Option<AskSender>) -> Self {
-        Self { permission, ask_tx }
+        Self {
+            permission,
+            ask_tx,
+            cache: None,
+        }
+    }
+
+    /// Builder that matches the dual-constructor pattern used by
+    /// Read/Grep/FindFiles/ListDir. Same `ToolCache` is shared
+    /// across tools so a `bash`/`write` that mutates the filesystem
+    /// invalidates glob results too via `cache.clear()`.
+    pub fn with_cache(
+        permission: Option<PermCheck>,
+        ask_tx: Option<AskSender>,
+        cache: ToolCache,
+    ) -> Self {
+        Self {
+            permission,
+            ask_tx,
+            cache: Some(cache),
+        }
     }
 }
 
@@ -95,6 +121,17 @@ impl Tool for GlobTool {
         )
         .await?;
 
+        let cache_key = format!(
+            "glob:{}:{}",
+            args.pattern,
+            args.path.as_deref().unwrap_or("."),
+        );
+        if let Some(ref cache) = self.cache
+            && let Some(cached) = cache.get(&cache_key)
+        {
+            return Ok(cached);
+        }
+
         let re = glob_to_regex(&args.pattern).map_err(|e| ToolError::Msg(e))?;
 
         let root = args
@@ -150,11 +187,15 @@ impl Tool for GlobTool {
         });
 
         let results: Vec<String> = matches.into_iter().map(|(rel, _)| rel).collect();
-        if results.is_empty() {
-            Ok(String::new())
+        let out = if results.is_empty() {
+            String::new()
         } else {
-            Ok(results.join("\n"))
+            results.join("\n")
+        };
+        if let Some(ref cache) = self.cache {
+            cache.set(&cache_key, out.clone());
         }
+        Ok(out)
     }
 }
 

src/agent/tools/lsp.rs

@@ -176,7 +176,7 @@ impl Tool for LspTool {
                     },
                     "query": {
                         "type": "string",
-                        "description": "Search string for workspaceSymbol. Empty string returns all symbols."
+                        "description": "Search string for workspaceSymbol — REQUIRED when operation is 'workspaceSymbol' (pass empty string to list all symbols). Ignored for other operations."
                     }
                 },
                 "required": ["operation", "file_path"]

src/agent/tools/plan.rs

@@ -62,6 +62,12 @@ impl Tool for PlanEnterTool {
     }
 
     async fn call(&self, _args: PlanEnterArgs) -> Result<String, ToolError> {
+        // Note: this tool doesn't go through `check_perm` because the
+        // plan_tx channel itself surfaces a confirmation dialog to the
+        // user — the user has to explicitly Accept or Reject the mode
+        // switch via `PlanSwitchResponse`. Routing through `check_perm`
+        // would double-ask. This matches how `harness/confirm` works
+        // in the plugin layer.
         let (reply_tx, reply_rx) = oneshot::channel();
 
         self.plan_tx

src/agent/tools/question.rs

@@ -3,7 +3,7 @@ use rig::tool::Tool;
 use serde::Deserialize;
 use tokio::sync::{mpsc, oneshot};
 
-use crate::agent::tools::ToolError;
+use crate::agent::tools::{AskSender, PermCheck, ToolError, check_perm};
 
 pub type QuestionSender = mpsc::Sender<QuestionRequest>;
 pub type QuestionReceiver = mpsc::Receiver<QuestionRequest>;
@@ -48,11 +48,30 @@ pub struct QuestionOption {
 
 pub struct QuestionTool {
     pub question_tx: QuestionSender,
+    pub permission: Option<PermCheck>,
+    pub ask_tx: Option<AskSender>,
 }
 
 impl QuestionTool {
     pub fn new(question_tx: QuestionSender) -> Self {
-        Self { question_tx }
+        Self {
+            question_tx,
+            permission: None,
+            ask_tx: None,
+        }
+    }
+
+    /// Builder for the production path: wires the permission checker
+    /// + ask channel so `question` invocations go through the same
+    /// allow/ask/deny rules as every other behaviour-altering tool.
+    pub fn with_permission(
+        mut self,
+        permission: Option<PermCheck>,
+        ask_tx: Option<AskSender>,
+    ) -> Self {
+        self.permission = permission;
+        self.ask_tx = ask_tx;
+        self
     }
 }
 
@@ -116,6 +135,17 @@ impl Tool for QuestionTool {
     }
 
     async fn call(&self, args: QuestionArgs) -> Result<String, ToolError> {
+        // Route through the same permission system as task / write /
+        // bash. `question` rewrites what the LLM sees by injecting
+        // user input — that's behavior-altering and shouldn't bypass
+        // user rules.
+        let summary = args
+            .questions
+            .first()
+            .map(|q| q.question.clone())
+            .unwrap_or_default();
+        check_perm(&self.permission, &self.ask_tx, "question", &summary).await?;
+
         let (reply_tx, reply_rx) = oneshot::channel();
 
         self.question_tx

src/agent/tools/task_status.rs

@@ -3,16 +3,32 @@ use rig::tool::Tool;
 use serde::Deserialize;
 use std::time::Duration;
 
-use crate::agent::tools::ToolError;
 use crate::agent::tools::background::{BackgroundStore, TaskState};
+use crate::agent::tools::{AskSender, PermCheck, ToolError, check_perm};
 
 pub struct TaskStatusTool {
     bg_store: BackgroundStore,
+    permission: Option<PermCheck>,
+    ask_tx: Option<AskSender>,
 }
 
 impl TaskStatusTool {
     pub fn new(bg_store: BackgroundStore) -> Self {
-        Self { bg_store }
+        Self {
+            bg_store,
+            permission: None,
+            ask_tx: None,
+        }
+    }
+
+    pub fn with_permission(
+        mut self,
+        permission: Option<PermCheck>,
+        ask_tx: Option<AskSender>,
+    ) -> Self {
+        self.permission = permission;
+        self.ask_tx = ask_tx;
+        self
     }
 }
 
@@ -52,6 +68,10 @@ impl Tool for TaskStatusTool {
     }
 
     async fn call(&self, args: TaskStatusArgs) -> Result<String, ToolError> {
+        // Same permission gate as `task` — status polling can side-
+        // channel sensitive subagent results, and the wait=true path
+        // can hold the parent turn open for up to 10 minutes.
+        check_perm(&self.permission, &self.ask_tx, "task_status", &args.task_id).await?;
         let wait = args.wait.unwrap_or(false);
 
         if wait {

src/agent/tools/webfetch.rs

@@ -91,7 +91,8 @@ impl Tool for WebFetchTool {
                         "description": "URLs to fetch (may be comma-separated)"
                     },
                     "max_chars": {
-                        "type": "number",
+                        "type": "integer",
+                        "minimum": 1,
                         "description": "Maximum characters to return per URL (default: 3000)"
                     }
                 },

src/plugin/mod.rs

@@ -2063,7 +2063,14 @@ fn parse_tree_op_line(line: &str) -> Option<TreeOp> {
                 Some(TreeOp::SwitchSession { id_prefix: arg1 })
             }
         }
-        _ => None,
+        // Forward compat: a future plugin shipping an op verb we
+        // don't know yet shouldn't poison the rest of the drain. Log
+        // at WARN so a confused plugin author can spot the typo
+        // instead of silently failing.
+        other => {
+            tracing::warn!(target: "dirge::plugin", op = other, "drain_tree_ops: unknown op verb (skipped)");
+            None
+        }
     }
 }