fix(audit r3): /retry handles system msgs, table breaks, apply_patch path-tool, unique save nonce, ragged tables (#57)

Round of follow-up fixes after auditing the recent PRs (granular
selection, markdown tables, CRLF apply_patch, atomic save, bold
streaming).

## CRITICAL fixes

### /retry now handles system messages
The original /retry used `undo_last` which only pops Assistant/User
pairs. If a system message (compress summary, error note) landed
between the user prompt and the retry trigger, it stayed in the
session and the agent saw it as context. Replaced with a tight
loop that pops messages until the last user message has been
removed too. Works regardless of how many system/assistant
messages were sandwiched between.

### Markdown table cells handle SoftBreak / HardBreak
`Event::SoftBreak` and `Event::HardBreak` pushed `\n` into `acc`
unconditionally. When inside a table cell, this smeared the cell
across multiple lines and broke alignment. Now substitutes a
single space for the break when `in_table`, so the cell's visible
content stays on one row.

### apply_patch / lsp now in `is_path_tool_name`
The tool-rule map registered `apply_patch` and `lsp`, but the
external-directory consultation check used a narrower whitelist
(`read | write | edit | list_dir`). When a config had
`external_directory: { "/tmp/**": "deny" }` and an agent ran
`apply_patch` against `/tmp/...`, the rule was silently ignored.
Both `is_path_tool_name` and the inner `is_path_tool` now use
the same canonical list (DRY: inner calls the public predicate).

## HIGH fix

### Atomic save tmp filename includes per-call nonce
Tmp file was `dir/.{id}.json.tmp` — deterministic. Two concurrent
saves of the same session id (rare, but possible across processes
or via plugin tree-ops) raced on the same tmp file. Now the tmp
name is `dir/.{id}.{pid}-{ns}.json.tmp` so each save writes its
own tmp. Rename collisions remain harmless (last writer wins on
the target; each tmp was complete before rename).

## LOW fix

### Ragged table rows get a minimum column width
When `ncols` was the max across header + rows but some body row
had fewer cells, the missing columns kept `widths[i] = 0`, leaving
the separator line wider than the cells. Each column now gets a
minimum width of 1 char so the right border aligns.

## Skipped (false positives from the audit)

- Bold "leak past colored span" — `ResetColor` does reset attributes
  on the terminals we care about; `NormalIntensity` is belt-and-braces.
  No actual leak in practice.
- Multi-byte UTF-8 grapheme cluster slicing — Rust's `chars()`
  iterator guarantees char-boundary safety; the code is correct.
- `buffer_pos_at` clamp comment — accurate; just terminology nitpick.
- Auto-compact alert "may scroll out of view" — the framed alert
  already stops the eye; further hardening would require a modal
  overlay (bigger UX change).

## Test plan

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

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

Author: yogthos

CONFIG.md

@@ -90,12 +90,27 @@ Accepted top-level keys:
 Permission actions are lowercase strings: `allow`, `ask`, or `deny`. Each tool
 rule can be a single action or an object mapping glob-like patterns to actions.
 Supported permission tool keys are `bash`, `read`, `write`, `edit`, `grep`,
-`find_files`, `list_dir`, and `write_todo_list`. MCP-backed tools are
-checked under `mcp_tool:{server_name}:{tool_name}`. Use `"*"` for the
-default action, `external_directory` for absolute-path rules outside the
-working directory, and `doom_loop` for repeated identical tool calls
-(default: `ask`). If `bash` is omitted, dirge installs its built-in
-safe bash allow/deny rules.
+`find_files`, `list_dir`, `write_todo_list`, `apply_patch`, `lsp`, and
+`question`. MCP-backed tools are checked under
+`mcp_tool:{server_name}:{tool_name}`. Use `"*"` for the default action,
+`external_directory` for absolute-path rules outside the working directory,
+and `doom_loop` for repeated identical tool calls (default: `ask`). If
+`bash` is omitted, dirge installs its built-in safe bash allow/deny rules.
+
+### Mode semantics
+
+- **`standard`** (default): every rule in `permission` is consulted; tools without
+  matching rules fall back to `*` (default `allow`).
+- **`restrictive`**: like `standard`, but any tool whose rule resolves to `allow`
+  via the `*` fallback (no explicit allow rule matched) is converted to `ask`.
+  Explicit `allow` rules still allow. Explicit `deny` rules still deny.
+- **`accept`** (equivalent to `--accept-all`): auto-allows tools whose targets
+  resolve inside the working directory; tools touching paths outside still
+  consult `external_directory` rules.
+- **`yolo`** (equivalent to `--yolo`): bypasses every check. Use with caution.
+
+CLI precedence (high → low): `--yolo` > `--accept-all` > `--restrictive` >
+`default_permission_mode` config > `standard`.
 
 When compiled with MCP support, `mcp_servers` accepts command-based and URL-based
 servers:

src/permission/checker.rs

@@ -29,7 +29,10 @@ pub struct PermissionChecker {
 /// classic glob semantics (one segment, doesn't cross `/`). Everything else
 /// is treated as shell/text where `*` means "any chars including /".
 pub(crate) fn is_path_tool_name(tool: &str) -> bool {
-    matches!(tool, "read" | "write" | "edit" | "list_dir")
+    matches!(
+        tool,
+        "read" | "write" | "edit" | "list_dir" | "apply_patch" | "lsp"
+    )
 }
 
 /// Build a Pattern with the right `*` semantics for the given tool.
@@ -291,7 +294,12 @@ impl PermissionChecker {
     }
 
     fn is_path_tool(&self, tool: &str) -> bool {
-        matches!(tool, "read" | "write" | "edit" | "list_dir")
+        // Must match `is_path_tool_name` — these are the tools that
+        // take a filesystem path as their permission input and need
+        // `external_directory` rule consultation. `apply_patch` and
+        // `lsp` are included because both route filesystem-path
+        // strings through `check_perm_path`.
+        is_path_tool_name(tool)
     }
 
     fn is_external_path(&self, path_str: &str) -> bool {

src/permission/mod.rs

@@ -46,11 +46,28 @@ pub struct PermissionConfig {
     pub doom_loop: Option<Action>,
 }
 
+/// Per-session security mode. Selected via `--yolo` / `--accept-all` /
+/// `--restrictive` CLI flags or the `default_permission_mode` config
+/// key. Mode precedence (high to low): `Yolo > Accept > Restrictive >
+/// Standard`.
 #[derive(Debug, Clone, Copy, PartialEq)]
 pub enum SecurityMode {
+    /// Every rule in `PermissionConfig` is consulted; tools with no
+    /// matching rule fall back to the `*` default action.
     Standard,
+    /// Like `Standard`, but any tool whose rule resolves to `Allow`
+    /// *via the `*` fallback* (no explicit allow rule matched) gets
+    /// upgraded to `Ask`. Explicit allow rules still allow; explicit
+    /// deny rules still deny. The semantic difference from
+    /// `Standard`: "if nothing explicitly approved this, ask the
+    /// user." It does NOT flip every Allow to Ask.
     Restrictive,
+    /// Auto-allows tools whose targets resolve inside the working
+    /// directory; tools touching paths outside `cwd` still consult
+    /// `external_directory` rules. Useful for fast iteration on a
+    /// trusted project.
     Accept,
+    /// Bypasses every check. Use with caution.
     Yolo,
 }
 

src/session/storage.rs

@@ -62,7 +62,21 @@ pub fn save_session(session: &Session) -> anyhow::Result<()> {
     // over the target. A crash mid-write leaves the temp behind but
     // never a truncated `.json`. The rename is atomic on every OS we
     // target. Use the same parent dir so rename stays on one filesystem.
-    let tmp = dir.join(format!(".{}.json.tmp", session.id));
+    //
+    // The tmp filename includes a per-call nonce (pid + nanos) so two
+    // concurrent saves of the same session id don't collide on the
+    // tmp file. Each thread/process writes to its own tmp; the rename
+    // race is harmless (last-writer wins on the target, but neither
+    // tmp is partial because each was fully written before rename).
+    let nonce = format!(
+        "{}-{}",
+        std::process::id(),
+        std::time::SystemTime::now()
+            .duration_since(std::time::UNIX_EPOCH)
+            .map(|d| d.as_nanos())
+            .unwrap_or(0),
+    );
+    let tmp = dir.join(format!(".{}.{}.json.tmp", session.id, nonce));
     {
         use std::io::Write;
         let mut f = std::fs::File::create(&tmp)?;

src/ui/markdown.rs

@@ -94,6 +94,13 @@ fn render_table(
             widths[i] = widths[i].max(cell.chars().count());
         }
     }
+    // Minimum column width — ragged rows (one row has fewer cells
+    // than `ncols`) would otherwise leave `widths[i] = 0` for the
+    // missing columns, breaking the separator line + right-border
+    // alignment. Guarantee at least 1 char per column.
+    for w in widths.iter_mut() {
+        *w = (*w).max(1);
+    }
     // Cap any single column to avoid one runaway cell blowing the
     // line width. Distribute available width: target inner width =
     // max_width - 4 (for outer `| ` + ` |`), minus 3*(ncols-1) for
@@ -404,8 +411,13 @@ pub fn markdown_to_styled(text: &str, max_width: usize) -> Vec<LineEntry> {
                 }
             }
             Event::SoftBreak | Event::HardBreak => {
-                if in_code_block {
-                    acc.push('\n');
+                if in_table {
+                    // Break inside a table cell would smear the cell
+                    // across multiple lines and misalign the row.
+                    // Markdown spec doesn't allow real newlines in
+                    // table cells — substitute a space so the visible
+                    // content stays on one line.
+                    current_cell.push(' ');
                 } else {
                     acc.push('\n');
                 }

src/ui/slash.rs

@@ -1050,11 +1050,21 @@ pub async fn handle_slash(
                 .cloned();
             match last_user {
                 Some(msg) => {
-                    // Pop the trailing assistant response (if any) so a
-                    // /retry doesn't leave the failed reply in the
-                    // session — the agent would otherwise see its own
-                    // bad answer as context on the retry.
-                    let _ = undo_last(session);
+                    // Pop messages until we've removed the last user
+                    // message itself — covers assistant replies AND
+                    // system messages (compress summaries, error
+                    // notices) that landed between the user prompt
+                    // and the current state. `undo_last` only handles
+                    // the Assistant/User pair pattern; using it here
+                    // would leave a trailing System message in the
+                    // session and the agent would see it on retry.
+                    while let Some(last) = session.messages.last() {
+                        let was_user = last.role == MessageRole::User;
+                        session.pop_last_message();
+                        if was_user {
+                            break;
+                        }
+                    }
                     input.buffer = msg.content.clone();
                     input.cursor = msg.content.len();
                     render_session(renderer, session, cli, cfg, context)?;