fix(audit r3): system-msg /retry + table breaks + path-tool apply_patch + unique tmp nonce - #57
Merged
Merged
Conversation
…path-tool, unique save nonce, ragged tables
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.
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
…path-tool, unique save nonce, ragged tables (dirge-code#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>
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.
Follow-up audit pass over the recent PRs (granular selection, tables, CRLF apply_patch, atomic save, bold streaming). 3 CRITICAL + 1 HIGH + 1 LOW fixed; 4 audit findings dropped as false positives. 612 tests pass.