Skip to content

Commit 9330c46

Browse files
yogthosYogthos
andauthored
fix: audit round 3 — grep hidden files, invalid include glob, chamber centering, perm mode + README docs (#104)
Five verified bugs from the third 6-agent audit pass. ## Bug 1 — grep walks dotfiles (security, F2 carryover) `src/agent/tools/grep.rs:129` set `.hidden(false)` on the ignore walker. Same security issue F2 fixed for find_files / glob / list_dir — `.env`, `.git/` internals, etc. could be matched by a generic regex search and surfaced into LLM context. Added `include_hidden: bool` to `GrepArgs` (defaults false, matching the F2 pattern). Schema documents the flag. Cache key includes `:hidden=<bool>` so the same pattern with different hidden flags doesn't collide. ## Bug 2 — grep invalid include glob silently fell back to `src/agent/tools/grep.rs:121` used `Regex::new(&pattern).unwrap_or_else(|_| Regex::new(".*").unwrap())` — a malformed include like `"[a-z("` silently became match-everything. User's include filter appeared to do nothing. Now surfaces the compile error via `ToolError::Msg` so the LLM sees "Invalid include glob '...': <err>. Use forms like "*.rs" or "*.{ts,tsx}".". ## Bug 3 — default_permission_mode typo silently → Standard `src/main.rs::resolve_mode` matched `default_permission_mode` against "yolo"/"accept"/ "restrictive" and fell through to `SecurityMode::Standard` for anything else. A typo like `"restritctive"` silently ran the agent in standard mode while the user thought they had configured restrictive. Now warns to stderr naming the unknown value + valid options: `warning: unknown default_permission_mode "restritctive" in config; using standard. Valid values: yolo, accept, restrictive, standard.`. Also accepts "standard" explicitly (previously silent default). ## Bug 4 — chamber_row_centered padding off by 2 + char-count vs display-width `src/ui/mod.rs::chamber_row_centered` had TWO stacked bugs: (1) Used `content.chars().count()` instead of display width. The NO-OUTPUT chamber starts with `⚠` (2 cells / 1 char), so centering was off by 1 cell. (2) `pad = inner - (len + 2)` left the row `inner + 2` cells total — but `chamber_row` and `chamber_bottom` produce `inner + 4` cells. The right border was 2 cells to the LEFT of the chamber's / . PR #93's visible chamber-right-border misalignment was this. Fixed to `pad = inner - len` (using display width). Row now matches `inner + 4` cells exactly, so the right border lines up with the chamber's top/bottom corners. ## Bug 5 — README docs gaps for shipped features PR #102 (custom theme JSON) and PR #73 (`/allow` CRUD) were both shipped but never mentioned in README: - Added `/allow <list|add|remove|clear>` row to the slash table. - Added a paragraph under "UI theme" pointing at `docs/THEMES.md` for custom theme JSON. ## Tests 1 new test: - `chamber_row_centered_handles_wide_emoji`: asserts row is exactly `inner + 4` cells wide with a leading-emoji message (regression guard for both bugs). 716 pass (was 715). All build profiles + fmt clean. ## Other audit findings — verified false positives or deferred The 6-agent audit produced ~60 candidate findings. Most were: - Speculative (panic-safety, atomic-ordering, dead-code claims) - Already-documented design choices (subagent isolation, permission lock-while-ask) - Feature requests (hot-reload plugins, harness/register-tool, MCP resources/prompts, etc.) Real bugs above are the ones I could reproduce or verify by reading the actual code. The rest go on the ROADMAP candidates list (or stay as is). Co-authored-by: Yogthos <yogthos@gmail.com>
1 parent ed974fa commit 9330c46

5 files changed

Lines changed: 92 additions & 10 deletions

File tree

README.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,7 @@ dirge --verbose
126126
| `/regen-prompts` | Restore built-in prompts |
127127
| `/mcp` | List MCP servers and tools |
128128
| `/panel [on\|off\|auto]` | Toggle the right-hand info panel (cwd, MCP, LSP, todos, modified files). `auto` shows it when the terminal is at least 100 cols wide. |
129+
| `/allow <list\|add\|remove\|clear>` | Manage the session permission allowlist (see `/help` for argument shapes) |
129130
| `/quit` | Exit dirge |
130131
| `/retry` | Retry last prompt |
131132
| `/help` | Show all commands |
@@ -317,6 +318,8 @@ dirge ships with an 80s-CRT phosphor green palette by default. To opt out, set `
317318

318319
Errors stay red and warnings stay yellow under every theme — those colors are part of the load-bearing semantic contract.
319320

321+
For custom themes, create `~/.config/dirge/<name>.theme.json` with overrides for any subset of the palette (named colors, hex `#rrggbb`, or 256-color indices), then set `theme: "<name>"` in `config.json`. See [`docs/THEMES.md`](docs/THEMES.md) for the full schema and examples.
322+
320323
## Supported providers
321324

322325
- OpenRouter (default)

src/agent/tools/grep.rs

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,10 @@ impl Tool for GrepTool {
8686
"context_lines": {
8787
"type": "integer",
8888
"description": "Number of context lines to show before and after each match (like grep -C)"
89+
},
90+
"include_hidden": {
91+
"type": "boolean",
92+
"description": "Include dotfiles (.env, .gitignore, etc.) in the search. Default false to avoid surfacing secrets and config files."
8993
}
9094
},
9195
"required": ["pattern"]
@@ -97,11 +101,12 @@ impl Tool for GrepTool {
97101
check_perm(&self.permission, &self.ask_tx, "grep", &args.pattern).await?;
98102

99103
let cache_key = format!(
100-
"grep:{}:{}:{}:{}",
104+
"grep:{}:{}:{}:{}:hidden={}",
101105
args.pattern,
102106
args.path.as_deref().unwrap_or("."),
103107
args.include.as_deref().unwrap_or(""),
104108
args.context_lines.unwrap_or(0),
109+
args.include_hidden,
105110
);
106111

107112
if let Some(ref cache) = self.cache {
@@ -116,17 +121,32 @@ impl Tool for GrepTool {
116121
let search_path = args.path.as_deref().unwrap_or(".");
117122
let context = args.context_lines.unwrap_or(0);
118123

119-
let include_re = args.include.as_ref().map(|g| {
120-
let pattern = format!("^(?:{})$", Self::glob_to_regex(g));
121-
Regex::new(&pattern).unwrap_or_else(|_| Regex::new(".*").unwrap())
122-
});
124+
// Validate the include glob and surface compile errors
125+
// instead of the previous silent fallback to `.*` (match
126+
// everything). A user passing `include: "[a-z("` would have
127+
// silently matched every file — the include filter would
128+
// appear to do nothing and they'd never know why.
129+
let include_re = match args.include.as_ref() {
130+
Some(g) => {
131+
let pattern = format!("^(?:{})$", Self::glob_to_regex(g));
132+
Some(Regex::new(&pattern).map_err(|e| {
133+
ToolError::Msg(format!(
134+
"Invalid include glob {g:?}: {e}. Use forms like \"*.rs\" or \"*.{{ts,tsx}}\"."
135+
))
136+
})?)
137+
}
138+
None => None,
139+
};
123140

124141
let walker = WalkBuilder::new(search_path)
125142
.git_ignore(true)
126143
.git_global(true)
127144
.git_exclude(true)
128145
.require_git(false)
129-
.hidden(false)
146+
// F2 carryover: hide dotfiles by default so grep doesn't
147+
// silently surface `.env` / `.git/` internals. Opt-in
148+
// via `include_hidden: true`.
149+
.hidden(!args.include_hidden)
130150
.filter_entry(|entry| {
131151
if entry.file_type().map(|t| t.is_dir()).unwrap_or(false) {
132152
!is_skip_dir(entry.file_name().to_str().unwrap_or(""))

src/agent/tools/mod.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,12 @@ pub struct GrepArgs {
112112
pub path: Option<String>,
113113
pub include: Option<String>,
114114
pub context_lines: Option<usize>,
115+
/// Include dotfiles / hidden files in the search. Default
116+
/// `false` — F2 carryover from find_files/glob/list_dir: grep
117+
/// also walks the filesystem and should not silently surface
118+
/// `.env`, `.git/` internals, etc. by default.
119+
#[serde(default)]
120+
pub include_hidden: bool,
115121
}
116122

117123
#[derive(Deserialize)]

src/main.rs

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,19 @@ fn resolve_mode(cli: &cli::Cli, cfg: &config::Config) -> SecurityMode {
9191
"yolo" => SecurityMode::Yolo,
9292
"accept" => SecurityMode::Accept,
9393
"restrictive" => SecurityMode::Restrictive,
94-
_ => SecurityMode::Standard,
94+
"standard" => SecurityMode::Standard,
95+
other => {
96+
// Unknown value silently mapped to Standard before
97+
// this — a typo like `restritctive` ended up as
98+
// Standard and the user never knew. Warn explicitly
99+
// and name the valid values.
100+
eprintln!(
101+
"warning: unknown default_permission_mode {:?} in config; using standard. \
102+
Valid values: yolo, accept, restrictive, standard.",
103+
other,
104+
);
105+
SecurityMode::Standard
106+
}
95107
}
96108
} else {
97109
SecurityMode::Standard

src/ui/mod.rs

Lines changed: 44 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3245,11 +3245,29 @@ fn close_tool_chamber_if_open(
32453245
/// `│ <content centered to inner> │` — pad text on both sides so
32463246
/// it sits horizontally centered within the chamber inner width.
32473247
fn chamber_row_centered(content: &str, inner: usize) -> String {
3248-
let len = content.chars().count();
3249-
if len + 2 >= inner {
3248+
// Total row width matches `chamber_row`: exactly `inner + 4`
3249+
// terminal cells (`│ ` (2) + inner-cell middle + ` │` (2)).
3250+
// The middle is `inner` cells: left_pad + content + right_pad.
3251+
//
3252+
// TWO bugs were stacked here:
3253+
// (1) Used `chars().count()` instead of display width — the
3254+
// NO-OUTPUT chamber starts with `⚠` (2 cells / 1 char) so
3255+
// centering was off by 1 cell.
3256+
// (2) Subtracted `len + 2` from `inner` for the pad, leaving
3257+
// the row 2 cells short of `inner + 4` total — the right
3258+
// `│` didn't line up under the chamber's top `╮` /
3259+
// bottom `╯`. Correct: pad = inner - len (the middle is
3260+
// `inner` cells wide; subtracting only `len` reserves the
3261+
// rest for padding around it).
3262+
//
3263+
// Anything wider than `inner` falls back to `chamber_row`
3264+
// which truncates with `…` and pads to exactly `inner` cells.
3265+
use unicode_width::UnicodeWidthStr;
3266+
let len = UnicodeWidthStr::width(content);
3267+
if len >= inner {
32503268
return chamber_row(content, inner);
32513269
}
3252-
let pad = inner.saturating_sub(len + 2);
3270+
let pad = inner - len;
32533271
let left = pad / 2;
32543272
let right = pad - left;
32553273
format!("│ {}{}{} │", " ".repeat(left), content, " ".repeat(right))
@@ -3670,6 +3688,29 @@ mod tests {
36703688
assert!(header.ends_with("─╮"));
36713689
}
36723690

3691+
/// Regression: `chamber_row_centered` must use DISPLAY width
3692+
/// not char count. The NO-OUTPUT chamber message starts with
3693+
/// `⚠` (2 cells wide, 1 char). Before this, centering was off
3694+
/// by 1 cell and the right `│` border misaligned with the
3695+
/// chamber's top/bottom borders.
3696+
#[test]
3697+
fn chamber_row_centered_handles_wide_emoji() {
3698+
let row = chamber_row_centered("⚠ tool denied", 40);
3699+
// Row must occupy exactly `inner + 4` display cells
3700+
// (`│ ` + content + padding + ` │` = inner + 4 = 44).
3701+
let row_width = UnicodeWidthStr::width(row.as_str());
3702+
assert_eq!(
3703+
row_width, 44,
3704+
"row must be exactly inner+4 cells wide; got {row_width} for {row:?}",
3705+
);
3706+
// Right border `│` MUST be at the very end (no trailing
3707+
// pad mismatch).
3708+
assert!(
3709+
row.ends_with(" │"),
3710+
"right border missing or padded wrong: {row:?}"
3711+
);
3712+
}
3713+
36733714
/// Self-review bug 1: `apply_patch` arg is `operations`
36743715
/// (array), not a single string. Previously fell through to
36753716
/// "path" lookup which returned empty, degrading the banner

0 commit comments

Comments
 (0)