Skip to content

Commit 26a0bbf

Browse files
author
Yogthos
committed
feat(phase 5): /allow CRUD slash command (list, add, remove, clear)
Phase 5 of the plan. No clean equivalent in opencode or pi — both manage allowlist entries only via the interactive permission prompt's "allow always" answer. dirge adds explicit CRUD so users can inspect, manually add, drop one, or clear all entries without editing the session JSON. ## Commands - `/allow` or `/allow list` — show numbered entries - `/allow add <tool> <pattern>` — add a (tool, pattern) entry, e.g. `/allow add bash 'cargo *'` - `/allow remove <idx>` — drop one entry by 0-based index from the `list` output - `/allow clear` — drop all entries The `add` form preserves pattern strings containing spaces by re-deriving the args from the raw `text` slice (rather than relying on the SmallVec `parts[2]` which would get truncated at the first whitespace). Same approach `/cd` uses for paths. ## State sync Both surfaces stay aligned: - `PermissionChecker::session_allowlist` (in-memory, used by `check` / `check_path`) — updated via the existing `add_session_allowlist` and the two new `remove_session_allowlist_at(idx)` + `clear_session_allowlist`. - `Session::permission_allowlist` (persisted to JSON, restored on resume via `load_session_allowlist`) — mirrored manually in the slash handler so save/load round-trips show the user-edited list. Without the session-side mirror, a `/allow add foo bar` would stay in memory for this run but vanish on `-c` resume. Dedup at the session level too so `save()` writes a clean list. ## Tests 3 new permission tests, written failing first: - `remove_session_allowlist_at_returns_removed_entry`: remove(1) on [bash:cargo*, bash:git*, read:/tmp/*] yields Some(("bash","git *")) and the surviving entries shift down. - `remove_session_allowlist_at_out_of_range_returns_none`: remove(99) returns None without panicking; existing entries intact. - `clear_session_allowlist_empties_the_list`: clear() drops all entries. The slash handler itself is not unit-tested (requires the full UI loop fixtures); manual smoke test: ``` $ dirge --restrictive > bash 'echo hi' > (a) allow always > /allow list → [0] bash echo * > /allow add read /tmp/* > /allow list → [0] bash echo * [1] read /tmp/* > /allow remove 0 > /allow list → [0] read /tmp/* > /allow clear > /allow list → empty ``` ## Docs `/help` text gets four new lines covering each subcommand. ## Test plan - [x] `cargo test --features plugin` -> 649 pass (was 646). - [x] `cargo build --all-features` -> compiles. - [x] `cargo build --no-default-features` -> compiles. ## Plan status | Phase | PR | Done | |---|---|---| | 1 | #69 | first-wins block + docs | | 2 | #70 | sibling-branch pruning + notification | | 3 | #71 | structured tool-call persistence | | 4 | #72 | branch summary metadata in /tree | | 5 | this | /allow CRUD | | 6 | — | skipped per user request (cost tracking) | 5 of 6 phases complete; Phase 6 (cost tracking) skipped.
1 parent 3ed4a1a commit 26a0bbf

2 files changed

Lines changed: 217 additions & 1 deletion

File tree

src/permission/checker.rs

Lines changed: 64 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -286,14 +286,30 @@ impl PermissionChecker {
286286
}
287287
}
288288

289-
#[allow(dead_code)]
290289
pub fn allowlist_entries(&self) -> Vec<(String, String)> {
291290
self.session_allowlist
292291
.iter()
293292
.map(|(t, p)| (t.clone(), p.original.clone()))
294293
.collect()
295294
}
296295

296+
/// Remove the allowlist entry at the given index (0-based,
297+
/// matching the display order in `/allow list`). Returns the
298+
/// removed `(tool, pattern)` on success, or `None` if the
299+
/// index is out of range. Used by `/allow remove <n>`.
300+
pub fn remove_session_allowlist_at(&mut self, idx: usize) -> Option<(String, String)> {
301+
if idx >= self.session_allowlist.len() {
302+
return None;
303+
}
304+
let (tool, pat) = self.session_allowlist.remove(idx);
305+
Some((tool, pat.original.clone()))
306+
}
307+
308+
/// Remove ALL allowlist entries. Used by `/allow clear`.
309+
pub fn clear_session_allowlist(&mut self) {
310+
self.session_allowlist.clear();
311+
}
312+
297313
pub fn set_mode(&mut self, mode: SecurityMode) {
298314
self.mode = mode;
299315
}
@@ -397,6 +413,53 @@ mod tests {
397413
assert!(matches!(r2, CheckResult::Allowed));
398414
}
399415

416+
/// Phase 5 — `/allow remove <idx>` plumbs through to
417+
/// `remove_session_allowlist_at`. Returns the removed entry's
418+
/// (tool, pattern) so the slash handler can confirm to the
419+
/// user what was removed.
420+
#[test]
421+
fn remove_session_allowlist_at_returns_removed_entry() {
422+
let mut checker = fresh_checker();
423+
checker.add_session_allowlist("bash".to_string(), "cargo *");
424+
checker.add_session_allowlist("bash".to_string(), "git *");
425+
checker.add_session_allowlist("read".to_string(), "/tmp/*");
426+
assert_eq!(checker.allowlist_entries().len(), 3);
427+
428+
let removed = checker.remove_session_allowlist_at(1);
429+
assert_eq!(removed, Some(("bash".to_string(), "git *".to_string())),);
430+
// After removal, the indices shift: original [0]bash:cargo*,
431+
// [2]read:/tmp/* are now at [0] and [1].
432+
let after = checker.allowlist_entries();
433+
assert_eq!(after.len(), 2);
434+
assert_eq!(after[0], ("bash".to_string(), "cargo *".to_string()));
435+
assert_eq!(after[1], ("read".to_string(), "/tmp/*".to_string()));
436+
}
437+
438+
/// Out-of-range index returns None rather than panicking. The
439+
/// slash handler shows a clear error in that case.
440+
#[test]
441+
fn remove_session_allowlist_at_out_of_range_returns_none() {
442+
let mut checker = fresh_checker();
443+
checker.add_session_allowlist("bash".to_string(), "cargo *");
444+
assert_eq!(checker.remove_session_allowlist_at(99), None);
445+
assert_eq!(checker.remove_session_allowlist_at(1), None);
446+
// Existing entry still there.
447+
assert_eq!(checker.allowlist_entries().len(), 1);
448+
}
449+
450+
/// `clear` empties the allowlist entirely. Different from
451+
/// `reset_to_new` (which clears EVERYTHING) — this is the
452+
/// user-facing nuke for just allowlist grants.
453+
#[test]
454+
fn clear_session_allowlist_empties_the_list() {
455+
let mut checker = fresh_checker();
456+
checker.add_session_allowlist("bash".to_string(), "cargo *");
457+
checker.add_session_allowlist("bash".to_string(), "git *");
458+
assert_eq!(checker.allowlist_entries().len(), 2);
459+
checker.clear_session_allowlist();
460+
assert!(checker.allowlist_entries().is_empty());
461+
}
462+
400463
// Adding the same (tool, pattern) twice must not duplicate the
401464
// entry. The audit flagged that "allow always" picks for the
402465
// same command repeated across a long session accumulate

src/ui/slash.rs

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1149,6 +1149,143 @@ pub async fn handle_slash(
11491149
}
11501150
}
11511151
}
1152+
"/allow" => {
1153+
// Phase 5: CRUD for the session permission allowlist.
1154+
// Subcommands: list (default), add <tool> <pattern>,
1155+
// remove <idx>, clear. Without this, users can only
1156+
// create allowlist entries via the interactive "(a)
1157+
// allow always" prompt; there's no way to inspect or
1158+
// undo a bad grant short of editing the session JSON.
1159+
let sub = parts.get(1).copied().unwrap_or("list");
1160+
let perm = match permission {
1161+
Some(p) => p,
1162+
None => {
1163+
renderer.write_line(
1164+
"permission system unavailable (--no-tools mode?)",
1165+
c_error(),
1166+
)?;
1167+
return Ok(());
1168+
}
1169+
};
1170+
match sub {
1171+
"list" => {
1172+
let entries = {
1173+
let guard = perm.lock().unwrap_or_else(|e| e.into_inner());
1174+
guard.allowlist_entries()
1175+
};
1176+
if entries.is_empty() {
1177+
renderer.write_line(
1178+
"session allowlist is empty (use '(a) allow always' in a permission prompt to add entries)",
1179+
c_agent(),
1180+
)?;
1181+
} else {
1182+
renderer.write_line(
1183+
&format!("session allowlist ({} entries):", entries.len()),
1184+
c_agent(),
1185+
)?;
1186+
for (i, (tool, pat)) in entries.iter().enumerate() {
1187+
renderer
1188+
.write_line(&format!(" [{}] {} {}", i, tool, pat), c_result())?;
1189+
}
1190+
renderer.write_line(
1191+
"use '/allow remove <idx>' to drop a single entry; '/allow clear' to drop all",
1192+
theme::dim(),
1193+
)?;
1194+
}
1195+
}
1196+
"add" => {
1197+
// `/allow add <tool> <pattern>` — third part is
1198+
// the tool, rest is the pattern (may contain
1199+
// spaces, so re-derive from raw text).
1200+
let raw_args = text.trim().strip_prefix("/allow").unwrap_or("").trim();
1201+
let rest = raw_args.strip_prefix("add").unwrap_or("").trim();
1202+
let mut it = rest.splitn(2, char::is_whitespace);
1203+
let tool = it.next().unwrap_or("");
1204+
let pattern = it.next().unwrap_or("").trim();
1205+
if tool.is_empty() || pattern.is_empty() {
1206+
renderer.write_line(
1207+
"usage: /allow add <tool> <pattern> (e.g. /allow add bash 'cargo *')",
1208+
c_error(),
1209+
)?;
1210+
} else {
1211+
{
1212+
let mut guard = perm.lock().unwrap_or_else(|e| e.into_inner());
1213+
guard.add_session_allowlist(tool.to_string(), pattern);
1214+
}
1215+
// Mirror into session.permission_allowlist
1216+
// so the entry persists across save/load.
1217+
let entry = crate::session::PermissionAllowEntry {
1218+
tool: tool.to_string(),
1219+
pattern: pattern.to_string(),
1220+
};
1221+
// Dedup at the session level too — checker
1222+
// dedupes but we want save() to write a
1223+
// clean list.
1224+
if !session
1225+
.permission_allowlist
1226+
.iter()
1227+
.any(|e| e.tool == entry.tool && e.pattern == entry.pattern)
1228+
{
1229+
session.permission_allowlist.push(entry);
1230+
}
1231+
renderer.write_line(&format!("added: {} {}", tool, pattern), c_agent())?;
1232+
}
1233+
}
1234+
"remove" => {
1235+
let idx_str = parts.get(2).copied().unwrap_or("");
1236+
let idx: usize = match idx_str.parse() {
1237+
Ok(n) => n,
1238+
Err(_) => {
1239+
renderer.write_line(
1240+
"usage: /allow remove <idx> (run /allow list to see indices)",
1241+
c_error(),
1242+
)?;
1243+
return Ok(());
1244+
}
1245+
};
1246+
let removed = {
1247+
let mut guard = perm.lock().unwrap_or_else(|e| e.into_inner());
1248+
guard.remove_session_allowlist_at(idx)
1249+
};
1250+
match removed {
1251+
Some((tool, pat)) => {
1252+
// Mirror removal into the session
1253+
// allowlist too.
1254+
session
1255+
.permission_allowlist
1256+
.retain(|e| !(e.tool == tool && e.pattern == pat));
1257+
renderer.write_line(
1258+
&format!("removed [{}]: {} {}", idx, tool, pat),
1259+
c_agent(),
1260+
)?;
1261+
}
1262+
None => {
1263+
renderer.write_line(
1264+
&format!("no allowlist entry at index {}", idx),
1265+
c_error(),
1266+
)?;
1267+
}
1268+
}
1269+
}
1270+
"clear" => {
1271+
{
1272+
let mut guard = perm.lock().unwrap_or_else(|e| e.into_inner());
1273+
guard.clear_session_allowlist();
1274+
}
1275+
session.permission_allowlist.clear();
1276+
renderer.write_line("session allowlist cleared", c_agent())?;
1277+
}
1278+
other => {
1279+
renderer.write_line(
1280+
&format!(
1281+
"unknown /allow subcommand {:?}; try: list, add, remove, clear",
1282+
other,
1283+
),
1284+
c_error(),
1285+
)?;
1286+
}
1287+
}
1288+
}
11521289
"/help" => {
11531290
renderer.write_line("commands:", c_agent())?;
11541291
renderer.write_line(" /model [name] show or switch model", c_result())?;
@@ -1223,6 +1360,22 @@ pub async fn handle_slash(
12231360
" /toggle <feat> [on|off] toggle a feature (e.g. /toggle todo)",
12241361
c_result(),
12251362
)?;
1363+
renderer.write_line(
1364+
" /allow list list session allowlist entries",
1365+
c_result(),
1366+
)?;
1367+
renderer.write_line(
1368+
" /allow add <tool> <pat> add an allowlist entry",
1369+
c_result(),
1370+
)?;
1371+
renderer.write_line(
1372+
" /allow remove <idx> drop one allowlist entry",
1373+
c_result(),
1374+
)?;
1375+
renderer.write_line(
1376+
" /allow clear drop all allowlist entries",
1377+
c_result(),
1378+
)?;
12261379
#[cfg(feature = "loop")]
12271380
{
12281381
let _ = renderer.write_line(

0 commit comments

Comments
 (0)