fix(audit r2): permission rules + markdown tables + /retry + bold streaming + atomic save + UX polish - #56
Merged
Merged
Conversation
added 4 commits
May 20, 2026 19:24
…tables
Round 1 of the follow-up audit fixes.
## PermissionConfig: apply_patch, lsp, question (CRITICAL/HIGH)
Three tools called `check_perm(..., "apply_patch", _)` /
`check_perm(..., "lsp", _)` / `check_perm(..., "question", _)`
but their keys were never registered in `PermissionConfig`. Users
could only allow/deny them via the global `*` default — no
per-pattern rules.
Each added as `Option<ToolPerm>` on `PermissionConfig` and wired
into `checker.rs`'s tool-rule map. Users can now write rules like:
"apply_patch": { "**/*.rs": "allow", "**": "ask" }
"lsp": "allow"
"question": "ask"
## Markdown tables now render (CRITICAL)
`Tag::Table` / `TableHead` / `TableRow` / `TableCell` had empty
match arms; pipe-delimited tables in agent replies silently
dropped. `pulldown_cmark::Parser::new` doesn't emit table events
by default either — needs `Options::ENABLE_TABLES`.
Now enables tables and renders as box-drawn cards:
│ col1 │ col2 │ col3 │
├──────┼──────┼──────┤
│ a │ b │ c │
│ d │ e │ f │
Column widths computed from longest cell content, capped to fit
inside `max_width` so a runaway cell can't break alignment.
Header row paints in `theme::header()`, body in `theme::agent()`,
separator in `theme::dim()`. Long cells truncate with `…` to
preserve the right border.
## Test plan
- [x] `cargo test --features plugin` -> 612 pass, 0 fail.
- [x] Both build profiles -> 0 warnings.
Round 2 — HIGH fixes. ## /retry pops the previous assistant response Previously /retry only restored the last user prompt into the editor and left the failed assistant reply in the session. On retry the agent saw its own bad answer as context. Now calls `undo_last(session)` first to pop the trailing assistant message (plus the user message if `undo_last`'s pair logic fires), then restores the user prompt to the editor and re-renders. ## write_line + write now apply Bold for bright colors `render_viewport` applied `Attribute::Bold` to bright tones for the phosphor glow effect; the streaming paths (`write_line` and `write`) didn't. Streamed agent text rendered flat until the next full repaint shifted it to bold. Both streaming paths now check `theme::is_bright` and wrap output in `Bold` / `NormalIntensity`, matching the viewport's per-row paint. ## apply_patch CRLF normalization on update `apply_patch`'s `update` operation did a raw substring match on the file's on-disk bytes. The LLM almost always emits `\n` for line breaks even when the file is CRLF on disk, so the substring match failed on Windows-style files. `edit.rs` already had this normalization; copying the same pattern here: 1. Read original. 2. If CRLF detected, build a `\n`-normalized working copy for matching. 3. Normalize `old_text` and `new_text` to `\n` for the match + replace. 4. Re-apply CRLF on write-back so the file's line endings aren't silently changed. ## apply_patch clears cache once after the batch Per-op `cache.clear()` was wasteful — a 5-op batch cleared 5 times. Now the cache is cleared exactly once after all ops in the batch finish (success or partial failure). ## Test plan - [x] `cargo test --features plugin` -> 612 pass, 0 fail. - [x] Both build profiles -> 0 warnings.
…lback split
Round 3 — MEDIUM fixes.
## Atomic session save
`save_session` did `fs::write(path, json)` directly. A crash mid-
write left a truncated `.json` that `find_recent_sessions` would
skip silently — the user would notice their session was missing
without knowing why. Two concurrent dirge processes saving the
same id could also interleave bytes.
Now write-temp-then-rename:
1. Write to `dir/.{id}.json.tmp`.
2. `sync_all()` (best-effort; non-fatal on unsupported FS).
3. `rename(tmp, target)`.
4. Clean up the tmp if rename fails.
Rename is atomic on every OS we target as long as both paths
live on the same filesystem (they do — same dir).
## Auto-compact failure is visible now
The old auto-compact error was a single dim red line that
scrolled past unnoticed. Users kept typing into an over-full
context and got mysterious context-length errors next turn.
Replaced with a framed alert:
╭─ ⚠ AUTO-COMPACT FAILED ─...─╮
│ cause: <error>
│ context is over the threshold — replies may start
│ hitting context-length errors. Try /compress
│ manually, /clear to start fresh, or restart with
│ a larger context_window in config.
╰─...─╯
Same style as the permission alert; impossible to miss.
The success-path banner also got a soft accent treatment
(`▒░ auto-compacting context ░▒`) so the user sees auto-compact
running rather than wondering why the next prompt is slow.
## set_label uses ensure_back_compat_initialized
Every other mutation method (`add_message`, `pop_last_message`,
`switch_to_leaf`, `fork_at`, `compress`) calls
`ensure_back_compat_initialized`. `set_label` was still using the
older split call. Switched for consistency so future label-aware
code reading `message_store` doesn't trip on an uninitialized store.
## Bash segment splitting without `semantic-bash` feature
Without the tree-sitter feature, complex compound commands like
`safe_cmd && rm -rf /` were checked as a single string against
the bash permission rules. If `safe_cmd && rm` didn't match any
deny pattern, the dangerous part squeaked through.
Now does a best-effort coarse split on `&&` / `;` / `||` and
checks each segment separately. Command substitution / subshell
constructs (`$(...)`, backticks, `<(...)`, `>(...)`) still need
the full parser, so when one is detected we fall back to
whole-command check — that surfaces the unfamiliar form before
any segment runs, letting the user explicitly allow or deny.
## Test plan
- [x] `cargo test --features plugin` -> 612 pass, 0 fail.
- [x] Both build profiles -> 0 warnings.
…on alert Round 4 — LOW fixes (final audit round). ## Prompt + input + search bar get Bold-glow The bottom row (prompt indicator, input text, search bar) rendered without `Attribute::Bold` for their bright colors. Chat above bloomed; the bottom area looked dimmer. Each site now checks `theme::is_bright` and wraps the write in Bold / NormalIntensity, matching the viewport's per-row paint. Status row stays unbolded — `theme::dim()` is dim by design (two-tone phosphor depth) so `is_bright` correctly returns false. ## Avatar updates instantly on permission alert Setting the avatar state to `Alert` happened before the alert box rendered, but the bottom-row repaint that actually shows the new face waited for the next event (typically the user's keystroke answering the prompt). The face still showed the in-flight tool (Reading/Writing/Bash) while the alert was up. Now `draw_bottom` runs explicitly right after the state change so the `(O_O)` Alert face appears at the same moment the alert box does. ## Test plan - [x] `cargo test --features plugin` -> 612 pass, 0 fail. - [x] Both build profiles -> 0 warnings. Note: the previously-flagged "grep.rs glob_to_regex doesn't escape dots" finding turned out to be a false positive — line 43 of `grep.rs` does push `\\.` for `.` correctly. Skipped.
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
…eaming + atomic save + UX polish (dirge-code#56) * fix(audit): permission rules for apply_patch/lsp/question + markdown tables Round 1 of the follow-up audit fixes. ## PermissionConfig: apply_patch, lsp, question (CRITICAL/HIGH) Three tools called `check_perm(..., "apply_patch", _)` / `check_perm(..., "lsp", _)` / `check_perm(..., "question", _)` but their keys were never registered in `PermissionConfig`. Users could only allow/deny them via the global `*` default — no per-pattern rules. Each added as `Option<ToolPerm>` on `PermissionConfig` and wired into `checker.rs`'s tool-rule map. Users can now write rules like: "apply_patch": { "**/*.rs": "allow", "**": "ask" } "lsp": "allow" "question": "ask" ## Markdown tables now render (CRITICAL) `Tag::Table` / `TableHead` / `TableRow` / `TableCell` had empty match arms; pipe-delimited tables in agent replies silently dropped. `pulldown_cmark::Parser::new` doesn't emit table events by default either — needs `Options::ENABLE_TABLES`. Now enables tables and renders as box-drawn cards: │ col1 │ col2 │ col3 │ ├──────┼──────┼──────┤ │ a │ b │ c │ │ d │ e │ f │ Column widths computed from longest cell content, capped to fit inside `max_width` so a runaway cell can't break alignment. Header row paints in `theme::header()`, body in `theme::agent()`, separator in `theme::dim()`. Long cells truncate with `…` to preserve the right border. ## Test plan - [x] `cargo test --features plugin` -> 612 pass, 0 fail. - [x] Both build profiles -> 0 warnings. * fix(audit r2): /retry, streaming bold, apply_patch cache + CRLF Round 2 — HIGH fixes. ## /retry pops the previous assistant response Previously /retry only restored the last user prompt into the editor and left the failed assistant reply in the session. On retry the agent saw its own bad answer as context. Now calls `undo_last(session)` first to pop the trailing assistant message (plus the user message if `undo_last`'s pair logic fires), then restores the user prompt to the editor and re-renders. ## write_line + write now apply Bold for bright colors `render_viewport` applied `Attribute::Bold` to bright tones for the phosphor glow effect; the streaming paths (`write_line` and `write`) didn't. Streamed agent text rendered flat until the next full repaint shifted it to bold. Both streaming paths now check `theme::is_bright` and wrap output in `Bold` / `NormalIntensity`, matching the viewport's per-row paint. ## apply_patch CRLF normalization on update `apply_patch`'s `update` operation did a raw substring match on the file's on-disk bytes. The LLM almost always emits `\n` for line breaks even when the file is CRLF on disk, so the substring match failed on Windows-style files. `edit.rs` already had this normalization; copying the same pattern here: 1. Read original. 2. If CRLF detected, build a `\n`-normalized working copy for matching. 3. Normalize `old_text` and `new_text` to `\n` for the match + replace. 4. Re-apply CRLF on write-back so the file's line endings aren't silently changed. ## apply_patch clears cache once after the batch Per-op `cache.clear()` was wasteful — a 5-op batch cleared 5 times. Now the cache is cleared exactly once after all ops in the batch finish (success or partial failure). ## Test plan - [x] `cargo test --features plugin` -> 612 pass, 0 fail. - [x] Both build profiles -> 0 warnings. * fix(audit r3): atomic save, auto-compact UX, set_label init, bash fallback split Round 3 — MEDIUM fixes. ## Atomic session save `save_session` did `fs::write(path, json)` directly. A crash mid- write left a truncated `.json` that `find_recent_sessions` would skip silently — the user would notice their session was missing without knowing why. Two concurrent dirge processes saving the same id could also interleave bytes. Now write-temp-then-rename: 1. Write to `dir/.{id}.json.tmp`. 2. `sync_all()` (best-effort; non-fatal on unsupported FS). 3. `rename(tmp, target)`. 4. Clean up the tmp if rename fails. Rename is atomic on every OS we target as long as both paths live on the same filesystem (they do — same dir). ## Auto-compact failure is visible now The old auto-compact error was a single dim red line that scrolled past unnoticed. Users kept typing into an over-full context and got mysterious context-length errors next turn. Replaced with a framed alert: ╭─ ⚠ AUTO-COMPACT FAILED ─...─╮ │ cause: <error> │ context is over the threshold — replies may start │ hitting context-length errors. Try /compress │ manually, /clear to start fresh, or restart with │ a larger context_window in config. ╰─...─╯ Same style as the permission alert; impossible to miss. The success-path banner also got a soft accent treatment (`▒░ auto-compacting context ░▒`) so the user sees auto-compact running rather than wondering why the next prompt is slow. ## set_label uses ensure_back_compat_initialized Every other mutation method (`add_message`, `pop_last_message`, `switch_to_leaf`, `fork_at`, `compress`) calls `ensure_back_compat_initialized`. `set_label` was still using the older split call. Switched for consistency so future label-aware code reading `message_store` doesn't trip on an uninitialized store. ## Bash segment splitting without `semantic-bash` feature Without the tree-sitter feature, complex compound commands like `safe_cmd && rm -rf /` were checked as a single string against the bash permission rules. If `safe_cmd && rm` didn't match any deny pattern, the dangerous part squeaked through. Now does a best-effort coarse split on `&&` / `;` / `||` and checks each segment separately. Command substitution / subshell constructs (`$(...)`, backticks, `<(...)`, `>(...)`) still need the full parser, so when one is detected we fall back to whole-command check — that surfaces the unfamiliar form before any segment runs, letting the user explicitly allow or deny. ## Test plan - [x] `cargo test --features plugin` -> 612 pass, 0 fail. - [x] Both build profiles -> 0 warnings. * fix(audit r4): bold streaming for prompt/input/search, avatar redraw on alert Round 4 — LOW fixes (final audit round). ## Prompt + input + search bar get Bold-glow The bottom row (prompt indicator, input text, search bar) rendered without `Attribute::Bold` for their bright colors. Chat above bloomed; the bottom area looked dimmer. Each site now checks `theme::is_bright` and wraps the write in Bold / NormalIntensity, matching the viewport's per-row paint. Status row stays unbolded — `theme::dim()` is dim by design (two-tone phosphor depth) so `is_bright` correctly returns false. ## Avatar updates instantly on permission alert Setting the avatar state to `Alert` happened before the alert box rendered, but the bottom-row repaint that actually shows the new face waited for the next event (typically the user's keystroke answering the prompt). The face still showed the in-flight tool (Reading/Writing/Bash) while the alert was up. Now `draw_bottom` runs explicitly right after the state change so the `(O_O)` Alert face appears at the same moment the alert box does. ## Test plan - [x] `cargo test --features plugin` -> 612 pass, 0 fail. - [x] Both build profiles -> 0 warnings. Note: the previously-flagged "grep.rs glob_to_regex doesn't escape dots" finding turned out to be a false positive — line 43 of `grep.rs` does push `\\.` for `.` correctly. Skipped. --------- Co-authored-by: Yogthos <yogthos@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second sweep of audit fixes — 4 rounds, 15 of 16 outstanding findings resolved.
Round 1 — CRITICAL/HIGH
apply_patch,lsp,questionadded to `PermissionConfig` so per-pattern rules workRound 2 — HIGH
Round 3 — MEDIUM
Round 4 — LOW
Skipped
Totals