fix: audit batch — 10 verified bugs (security, plugin, skill, UI)

plugins/session_tree.janet

@@ -13,19 +13,27 @@
 #   /fresh            — persist the current session and start a new
 #                        one in place. Keeps the model/provider.
 
-(def hooks [])
+# Register the hook so dirge actually dispatches it. Bare names in
+# this vector get auto-aliased to `<stem>-<hook>` (so this resolves
+# to `session_tree-on-message-update`) — dirge's hook surface uses
+# `on-message-update`, not `on-message`. The earlier version of this
+# plugin declared `(def hooks [])` and defined `on-message`, so the
+# function was loaded but never fired, leaving /label permanently
+# stuck on "no entry yet".
+(def hooks ["on-message-update"])
 
-# Track the most-recent entry-id we've seen across on-message hooks
-# so /label can attach to it without the user having to type a uuid.
+# Track the most-recent entry-id we've seen across on-message-update
+# hooks so /label can attach to it without the user typing a uuid.
 (var last-entry-id nil)
 
-(defn on-message [ctx]
-  # Plugin receives the just-recorded entry id via ctx — we stash it
-  # so the label command has something to target. (If your harness
-  # passes ids differently, adapt this getter.)
-  (when-let [id (get ctx :id)]
-    (when (string? id)
-      (set last-entry-id id))))
+(defn on-message-update [ctx]
+  # Plugin receives the in-progress turn id via ctx; we stash it
+  # so the label command has something to target. The dirge hook
+  # surface ships `:index` (turn ordinal) and `:partial`; if a
+  # future host bumps the context to include `:id`, this picks it
+  # up automatically.
+  (when-let [id (or (get ctx :id) (get ctx :index))]
+    (set last-entry-id (string id))))
 
 (defn label-handler [args]
   (cond

plugins/workflow.janet

@@ -2,8 +2,19 @@
 # Drives architect → implementor → review phases automatically
 # Inversion of control: harness drives the model, not vice versa
 
-# Declare which hooks this plugin subscribes to
-(def hooks ["on-init" "on-prompt" "on-response" "on-tool-start"])
+# Declare which hooks this plugin subscribes to. The `workflow-on-*`
+# functions below are auto-aliased from these bare names (e.g.
+# "on-init" → `workflow-on-init`). The earlier version of this
+# plugin only listed four hooks, so `workflow-on-tool-end`,
+# `workflow-on-error`, and `workflow-on-complete` were defined but
+# never dispatched.
+(def hooks ["on-init"
+            "on-prompt"
+            "on-response"
+            "on-tool-start"
+            "on-tool-end"
+            "on-error"
+            "on-complete"])
 
 (var phase :idle)
 

src/agent/tools/bash.rs

@@ -342,7 +342,7 @@ fn quote_aware_split(command: &str) -> Vec<&str> {
         }
 
         if !in_single && !in_double {
-            // Check for `&&` and `||` (2-byte) BEFORE single-byte `;`.
+            // Check for `&&` and `||` (2-byte) BEFORE single-byte `;`/`|`.
             if i + 1 < bytes.len()
                 && ((b == b'&' && bytes[i + 1] == b'&') || (b == b'|' && bytes[i + 1] == b'|'))
             {
@@ -357,6 +357,18 @@ fn quote_aware_split(command: &str) -> Vec<&str> {
                 start = i;
                 continue;
             }
+            // Pipe `|` (single-byte) — must be checked AFTER `||`
+            // above. Without this, a command like `safe_cmd | rm
+            // -rf /` was treated as one segment and only `safe_cmd`'s
+            // permission rule applied; the destructive RHS rode in
+            // unchecked. The semantic-bash tree-sitter path correctly
+            // splits pipelines; this fallback didn't.
+            if b == b'|' {
+                push_segment(command, start, i, &mut segments);
+                i += 1;
+                start = i;
+                continue;
+            }
         }
 
         i += 1;
@@ -501,6 +513,28 @@ mod tests {
         assert_eq!(segments[3], "cmd4");
     }
 
+    /// Regression: bare `|` pipes must split into segments. Before
+    /// this, a command like `safe_cmd | rm -rf /` was treated as
+    /// one unit and only `safe_cmd`'s permission rule applied.
+    #[test]
+    fn quote_aware_split_splits_on_bare_pipe() {
+        let segments = quote_aware_split("safe_cmd | rm -rf /tmp/x");
+        assert_eq!(segments.len(), 2);
+        assert_eq!(segments[0].trim(), "safe_cmd");
+        assert_eq!(segments[1].trim(), "rm -rf /tmp/x");
+    }
+
+    /// `||` must NOT also match the single-`|` arm (already covered
+    /// by the existing `||` test, but pin the interaction here too).
+    #[test]
+    fn quote_aware_split_or_and_pipe_distinct() {
+        let segments = quote_aware_split("a || b | c");
+        assert_eq!(segments.len(), 3, "got {segments:?}");
+        assert_eq!(segments[0].trim(), "a");
+        assert_eq!(segments[1].trim(), "b");
+        assert_eq!(segments[2].trim(), "c");
+    }
+
     /// Empty / whitespace-only segments dropped.
     #[test]
     fn quote_aware_split_drops_empty_segments() {

src/agent/tools/grep.rs

@@ -183,6 +183,18 @@ impl Tool for GrepTool {
 
             let path_str = entry.path().to_string_lossy().to_string();
 
+            // Skip files larger than 10 MiB so a single huge file
+            // can't blow up the process memory. Anything bigger
+            // than this is realistically not source code the LLM
+            // would want grepped. `tokio::fs::read` previously
+            // pulled the whole file into RAM unconditionally.
+            const MAX_GREP_FILE_BYTES: u64 = 10 * 1024 * 1024;
+            if let Ok(meta) = tokio::fs::metadata(entry.path()).await
+                && meta.len() > MAX_GREP_FILE_BYTES
+            {
+                continue;
+            }
+
             match tokio::fs::read(entry.path()).await {
                 Ok(data) => {
                     if Self::is_binary(&data) {

src/agent/tools/read.rs

@@ -9,6 +9,88 @@ use crate::agent::tools::{AskSender, PermCheck, ReadArgs, ToolError, check_perm_
 #[cfg(feature = "lsp")]
 use crate::lsp::manager::{LspManager, TouchMode};
 
+/// Reject these extensions outright as binary — matches opencode's
+/// `read.ts` extension list. Sampling-based detection below catches
+/// anything not on this list (custom compiled artifacts, etc.).
+fn is_binary_extension(path: &str) -> bool {
+    let lower = path.to_ascii_lowercase();
+    let ext = match lower.rsplit_once('.') {
+        Some((_, e)) => e,
+        None => return false,
+    };
+    matches!(
+        ext,
+        "zip"
+            | "tar"
+            | "gz"
+            | "tgz"
+            | "bz2"
+            | "xz"
+            | "7z"
+            | "rar"
+            | "exe"
+            | "dll"
+            | "so"
+            | "dylib"
+            | "class"
+            | "jar"
+            | "war"
+            | "wasm"
+            | "doc"
+            | "docx"
+            | "xls"
+            | "xlsx"
+            | "ppt"
+            | "pptx"
+            | "odt"
+            | "ods"
+            | "odp"
+            | "pdf"
+            | "bin"
+            | "dat"
+            | "obj"
+            | "o"
+            | "a"
+            | "lib"
+            | "pyc"
+            | "pyo"
+            | "png"
+            | "jpg"
+            | "jpeg"
+            | "gif"
+            | "webp"
+            | "bmp"
+            | "ico"
+            | "mp3"
+            | "mp4"
+            | "mov"
+            | "avi"
+            | "ogg"
+            | "wav"
+            | "flac"
+    )
+}
+
+/// Sample-based binary detection — opencode `read.ts:187-198`.
+/// Any null byte → binary. Otherwise count "non-printable" bytes
+/// (outside `\t\n\r` and printable ASCII range); if more than 30%
+/// of the sample, treat as binary.
+fn is_binary_content(sample: &[u8]) -> bool {
+    if sample.is_empty() {
+        return false;
+    }
+    let mut non_printable = 0usize;
+    for &b in sample {
+        if b == 0 {
+            return true;
+        }
+        if b < 9 || (b > 13 && b < 32) {
+            non_printable += 1;
+        }
+    }
+    (non_printable * 100) / sample.len() > 30
+}
+
 pub struct ReadTool {
     pub permission: Option<PermCheck>,
     pub ask_tx: Option<AskSender>,
@@ -114,7 +196,34 @@ impl Tool for ReadTool {
         let offset = args.offset.unwrap_or(1).max(1) - 1;
         let limit = args.limit.unwrap_or(2000);
 
-        use tokio::io::AsyncBufReadExt;
+        use tokio::io::{AsyncBufReadExt, AsyncReadExt};
+
+        // Binary file detection — refuse before streaming so we
+        // don't shovel multi-MB of corrupted UTF-8 into LLM
+        // context. Pattern matches opencode `read.ts:153-198`:
+        // reject by extension OR by sampling the first 4 KiB
+        // for null bytes / non-printable density. The agent gets
+        // a clear "Cannot read binary file" hint instead of
+        // garbled output.
+        if is_binary_extension(args.path.as_str()) {
+            return Err(ToolError::Msg(format!(
+                "Cannot read binary file: {} (use bash with a hex/xxd tool if you really need bytes)",
+                args.path,
+            )));
+        }
+        {
+            let mut sniffer = tokio::fs::File::open(&args.path).await?;
+            let mut sample = vec![0u8; 4096];
+            let n = sniffer.read(&mut sample).await?;
+            sample.truncate(n);
+            if is_binary_content(&sample) {
+                return Err(ToolError::Msg(format!(
+                    "Cannot read binary file: {} (null bytes / high non-printable density detected)",
+                    args.path,
+                )));
+            }
+        }
+
         let file = tokio::fs::File::open(&args.path).await?;
         let reader = tokio::io::BufReader::new(file);
         let mut lines = reader.lines();
@@ -216,6 +325,36 @@ mod tests {
     use super::*;
     use crate::agent::tools::ReadArgs;
 
+    /// Binary detection by extension — pdf/exe/o/zip/etc.
+    #[test]
+    fn test_is_binary_extension_known() {
+        assert!(is_binary_extension("foo.pdf"));
+        assert!(is_binary_extension("a.tar.gz"));
+        assert!(is_binary_extension("dir/lib.so"));
+        assert!(is_binary_extension("PHOTO.JPG"));
+        assert!(is_binary_extension("class.pyc"));
+        assert!(!is_binary_extension("source.rs"));
+        assert!(!is_binary_extension("script.py"));
+        assert!(!is_binary_extension("README.md"));
+        assert!(!is_binary_extension("noext"));
+    }
+
+    /// Sample-based binary detection — null byte triggers, plain
+    /// UTF-8 doesn't, 30%-non-printable triggers.
+    #[test]
+    fn test_is_binary_content_null_byte() {
+        assert!(is_binary_content(b"hello\x00world"));
+        assert!(!is_binary_content(b"hello world"));
+        assert!(!is_binary_content(b"")); // empty isn't binary
+        // Mostly non-printable → binary.
+        let blob: Vec<u8> = (0..100).map(|_| 0x01u8).collect();
+        assert!(is_binary_content(&blob));
+        // UTF-8 multi-byte (Japanese) — should NOT trip the
+        // non-printable heuristic since multi-byte UTF-8 bytes
+        // are all >= 128.
+        assert!(!is_binary_content("こんにちは世界".as_bytes()));
+    }
+
     /// Verifies the line-numbering format used in read output.
     /// The model sees this format and must strip "NNN: " prefixes when passing text to edit.
     #[test]

src/extras/mcp/tool.rs

@@ -74,7 +74,30 @@ impl ToolDyn for McpTool {
                 .await
                 .map_err(|e| ToolError::ToolCallError(Box::new(McpToolError(e.to_string()))))?;
 
-            let arguments: Option<JsonObject> = serde_json::from_str(&args).unwrap_or_default();
+            // Malformed JSON used to silently default to `None` via
+            // `unwrap_or_default()` — the MCP server got an empty
+            // argument set and the agent saw a confusing "missing
+            // required field" error from the server instead of a
+            // dirge-side parse error. Surface the parse failure
+            // distinctly so the agent can fix its tool call.
+            //
+            // Empty / whitespace-only args is treated as the explicit
+            // no-arguments case (matches rig's default tool-call
+            // shape when the LLM omits the arguments object).
+            let trimmed = args.trim();
+            let arguments: Option<JsonObject> = if trimmed.is_empty() {
+                None
+            } else {
+                match serde_json::from_str::<JsonObject>(trimmed) {
+                    Ok(obj) => Some(obj),
+                    Err(e) => {
+                        return Err(ToolError::ToolCallError(Box::new(McpToolError(format!(
+                            "MCP tool {}::{}: malformed JSON arguments ({e}). Got: {trimmed:.200}",
+                            server_name, tool_name,
+                        )))));
+                    }
+                }
+            };
             let params = arguments
                 .map(|a| CallToolRequestParams::new(tool_name.clone()).with_arguments(a))
                 .unwrap_or_else(|| CallToolRequestParams::new(tool_name.clone()));

src/semantic/adapters/python.rs

@@ -166,7 +166,14 @@ impl PythonAdapter {
                     if let Some(node) = func_node {
                         if let Some(name_node) = node.child_by_field_name("name") {
                             let name = self.node_text(name_node, source).to_string();
-                            let is_exported = !name.starts_with('_');
+                            // Dunder methods (`__init__`, `__call__`, `__repr__`, …)
+                            // are part of Python's public protocol; they look
+                            // "private" by the leading-underscore heuristic but
+                            // are externally callable. Treat them as exported.
+                            // Single-underscore names (`_helper`, `_internal`)
+                            // stay non-exported.
+                            let is_dunder = name.starts_with("__") && name.ends_with("__");
+                            let is_exported = is_dunder || !name.starts_with('_');
                             let range = self.make_range(child);
                             let signature = self.signature_from_node(node, source);
                             symbols.push(Symbol {

src/skill.rs

@@ -60,7 +60,14 @@ pub fn discover_skills(cwd: &Path) -> Vec<Skill> {
                 }
                 if let Ok(content) = std::fs::read_to_string(&skill_md) {
                     if let Some(skill) = parse_skill(&content, &path) {
-                        map.entry(skill.name.clone()).or_insert(skill);
+                        // README contract: "Project skills override
+                        // global skills by name." Globals are iterated
+                        // first (line 34), so use `insert` (last-write-
+                        // wins) — `or_insert` kept the global value
+                        // and silently dropped the project override.
+                        if !skill.name.is_empty() {
+                            map.insert(skill.name.clone(), skill);
+                        }
                     }
                 }
             }
@@ -107,6 +114,16 @@ fn parse_skill(content: &str, dir_path: &Path) -> Option<Skill> {
         parse_frontmatter(&frontmatter, dir_name)
     };
 
+    // A frontmatter `name:` with an empty value would parse to "" and
+    // then any subsequent `skill <empty>` call would silently match
+    // the first such entry. Fall back to the directory name in that
+    // case so every skill has a usable handle.
+    let name = if name.trim().is_empty() {
+        dir_name.to_string()
+    } else {
+        name
+    };
+
     Some(Skill {
         name,
         description,