From 3f1615089eb1eb93172d3a5d1d0542d23fe8ba56 Mon Sep 17 00:00:00 2001 From: fluxdiv <156196590+fluxdiv@users.noreply.github.com> Date: Wed, 24 Jun 2026 23:23:05 -0400 Subject: [PATCH] fix: collapse duplicate prompt grants on allow always --- crates/seal-tui/src/toml_editor/commands.rs | 95 +++++++++++------ crates/seal-tui/src/toml_editor/fs.rs | 80 +++++++++------ crates/seal-tui/src/toml_editor/tests.rs | 107 ++++++++++++++++++++ 3 files changed, 222 insertions(+), 60 deletions(-) diff --git a/crates/seal-tui/src/toml_editor/commands.rs b/crates/seal-tui/src/toml_editor/commands.rs index 9cf89ce85..f9b49a265 100644 --- a/crates/seal-tui/src/toml_editor/commands.rs +++ b/crates/seal-tui/src/toml_editor/commands.rs @@ -18,40 +18,36 @@ pub(super) fn upsert_allow_command( ) -> Result<()> { let patterns = ensure_patterns_array(doc, "allow")?; - // Scan for an existing match. The entry can be either a bare - // string or an inline table with `command = ""`. - for i in 0..patterns.len() { - let Some(value) = patterns.get(i) else { - continue; - }; - match extract_command(value) { - Some(existing) if existing == pattern => { - // Replace with the requested form. - if prompt { - // Write an inline table to preserve the prompt flag. - let mut it = InlineTable::new(); - it.insert("command", Value::from(pattern)); - it.insert("prompt", Value::from(true)); - patterns.replace(i, Value::InlineTable(it)); - } else { - // Bare string form — equivalent to `prompt = false`. - patterns.replace(i, Value::from(pattern)); - } + if prompt { + // Prompted allows are only written by callers that explicitly + // want that shape. Preserve the old first-match semantics here; + // Allow-once uses `upsert_allow_command_prompt_only` to avoid + // downgrading silent grants. + for i in 0..patterns.len() { + let Some(value) = patterns.get(i) else { + continue; + }; + if extract_command(value).is_some_and(|existing| existing == pattern) { + let mut it = InlineTable::new(); + it.insert("command", Value::from(pattern)); + it.insert("prompt", Value::from(true)); + patterns.replace(i, Value::InlineTable(it)); return Ok(()); } - _ => {} } - } - // Not found — append. - if prompt { let mut it = InlineTable::new(); it.insert("command", Value::from(pattern)); it.insert("prompt", Value::from(true)); push_preserving_format(patterns, Value::InlineTable(it)); - } else { - push_preserving_format(patterns, Value::from(pattern)); + return Ok(()); + } + + if normalize_allow_command_prompt_false(patterns, pattern) { + return Ok(()); } + + push_preserving_format(patterns, Value::from(pattern)); Ok(()) } @@ -224,16 +220,57 @@ fn ensure_command_domains_array<'a>(doc: &'a mut DocumentMut, side: &str) -> Res /// into a bare string) without adding a new one. /// Returns `true` if an entry was found and modified, `false` otherwise. pub(super) fn flip_prompt_in_array(patterns: &mut Array, pattern: &str) -> Result { - for i in 0..patterns.len() { + Ok(normalize_allow_command_prompt_false(patterns, pattern)) +} + +fn normalize_allow_command_prompt_false(patterns: &mut Array, pattern: &str) -> bool { + let mut found = false; + let mut kept_simple = false; + let mut i = 0; + while i < patterns.len() { let Some(value) = patterns.get(i) else { + i += 1; continue; }; - if extract_command(value).is_some_and(|c| c == pattern) { + if extract_command(value).is_none_or(|c| c != pattern) { + i += 1; + continue; + } + + found = true; + if command_entry_has_extra_policy(value) { + let replacement = command_entry_without_prompt(value); + patterns.replace(i, replacement); + i += 1; + } else if kept_simple { + patterns.remove(i); + } else { patterns.replace(i, Value::from(pattern)); - return Ok(true); + kept_simple = true; + i += 1; + } + } + found +} + +fn command_entry_has_extra_policy(value: &Value) -> bool { + match value { + Value::InlineTable(table) => table + .iter() + .any(|(key, _)| key != "command" && key != "prompt"), + _ => false, + } +} + +fn command_entry_without_prompt(value: &Value) -> Value { + match value { + Value::InlineTable(table) => { + let mut table = table.clone(); + table.remove("prompt"); + Value::InlineTable(table) } + _ => value.clone(), } - Ok(false) } /// Remove any allow-commands entry with `command == pattern`. diff --git a/crates/seal-tui/src/toml_editor/fs.rs b/crates/seal-tui/src/toml_editor/fs.rs index f0b475f24..5024dbdec 100644 --- a/crates/seal-tui/src/toml_editor/fs.rs +++ b/crates/seal-tui/src/toml_editor/fs.rs @@ -24,6 +24,10 @@ pub(super) fn upsert_allow_fs_path( let defaults = section_default_files(doc, "allow", kind); let paths = ensure_fs_paths_array(doc, "allow", kind)?; + if !prompt && normalize_allow_fs_prompt_false(paths, pattern, &defaults) { + return Ok(()); + } + for i in 0..paths.len() { let Some(value) = paths.get(i) else { continue; @@ -32,17 +36,10 @@ pub(super) fn upsert_allow_fs_path( let existing_prompt = fs_entry_prompt(value).unwrap_or(false); match (existing_prompt, prompt) { (true, false) => { - // Flip prompt=true → prompt=false: emit the bare - // scope form, dropping the prompt flag. paths.replace(i, build_fs_entry_value(pattern, false, has_defaults)); } - (false, true) => { - // Requested prompt=true but a stronger (no-prompt) - // entry exists. Never downgrade. - } - _ => { - // Same flag state — idempotent no-op. - } + (false, true) => {} + _ => {} } return Ok(()); } @@ -292,33 +289,54 @@ pub(super) fn flip_fs_prompt_in_array( pattern: &str, section_default_files: &[String], ) -> Result { - for i in 0..paths.len() { + Ok(normalize_allow_fs_prompt_false( + paths, + pattern, + section_default_files, + )) +} + +fn normalize_allow_fs_prompt_false( + paths: &mut Array, + pattern: &str, + section_default_files: &[String], +) -> bool { + let mut found = false; + let mut i = 0; + while i < paths.len() { let Some(value) = paths.get(i) else { + i += 1; continue; }; - if fs_entry_path_matches(value, pattern, section_default_files) { - // Build a no-prompt replacement preserving the entry's - // current shape. For shorthand-with-flags (`{ path, prompt - // = true }`) this collapses back to a bare string; for - // `{ path, files, prompt = true }` it drops just the - // `prompt` key. - let new_val = match value { - Value::InlineTable(t) if t.contains_key("files") => { - let mut t = t.clone(); - t.remove("prompt"); - Value::InlineTable(t) - } - _ => { - // Collapse to shorthand. - let path = fs_entry_path(value).unwrap_or_else(|| pattern.to_string()); - Value::from(path) - } - }; - paths.replace(i, new_val); - return Ok(true); + if !fs_entry_path_matches(value, pattern, section_default_files) { + i += 1; + continue; + } + + if found { + paths.remove(i); + } else { + let replacement = fs_entry_without_prompt(value, pattern); + paths.replace(i, replacement); + found = true; + i += 1; + } + } + found +} + +fn fs_entry_without_prompt(value: &Value, pattern: &str) -> Value { + match value { + Value::InlineTable(table) if table.contains_key("files") => { + let mut table = table.clone(); + table.remove("prompt"); + Value::InlineTable(table) + } + _ => { + let path = fs_entry_path(value).unwrap_or_else(|| pattern.to_string()); + Value::from(path) } } - Ok(false) } pub(super) fn fs_paths_contain( diff --git a/crates/seal-tui/src/toml_editor/tests.rs b/crates/seal-tui/src/toml_editor/tests.rs index 042e3c8b5..788171da6 100644 --- a/crates/seal-tui/src/toml_editor/tests.rs +++ b/crates/seal-tui/src/toml_editor/tests.rs @@ -54,6 +54,58 @@ fn add_allow_is_idempotent_for_bare_string() { assert_eq!(count, 1, "no duplicate; output:\n{out}"); } +#[test] +fn add_allow_removes_later_prompt_duplicate_when_silent_exists_first() { + let toml = r#"schema_version = 1 +[model] +provider = "anthropic" +name = "claude-sonnet-4-6" + +[capabilities.allow.commands] +patterns = ["jj log:*", { command = "jj log:*", prompt = true }, "pwd:*"] +"#; + let out = add_allow_command(toml, "jj log:*").unwrap(); + + assert!(!out.contains("prompt = true"), "{out}"); + assert_eq!(out.matches(r#""jj log:*""#).count(), 1, "{out}"); + assert!(out.contains(r#""pwd:*""#), "{out}"); +} + +#[test] +fn flip_prompt_false_removes_later_prompt_duplicate_when_silent_exists_first() { + let toml = r#"schema_version = 1 +[model] +provider = "anthropic" +name = "claude-sonnet-4-6" + +[capabilities.allow.commands] +patterns = ["jj log:*", { command = "jj log:*", prompt = true }, "pwd:*"] +"#; + let out = flip_allow_prompt_false(toml, "jj log:*").unwrap(); + + assert!(!out.contains("prompt = true"), "{out}"); + assert_eq!(out.matches(r#""jj log:*""#).count(), 1, "{out}"); + assert!(out.contains(r#""pwd:*""#), "{out}"); +} + +#[test] +fn flip_prompt_false_collapses_multiple_prompt_duplicates() { + let toml = r#"schema_version = 1 +[model] +provider = "anthropic" +name = "claude-sonnet-4-6" + +[capabilities.allow.commands] +patterns = [ + { command = "jj log:*", prompt = true }, + { command = "jj log:*", prompt = true }, +] +"#; + let out = flip_allow_prompt_false(toml, "jj log:*").unwrap(); + + assert!(!out.contains("prompt = true"), "{out}"); + assert_eq!(out.matches(r#""jj log:*""#).count(), 1, "{out}"); +} #[test] fn add_allow_removes_matching_deny() { let toml = r#"schema_version = 1 @@ -705,6 +757,61 @@ paths = [{ path = "docs/**", prompt = true }] assert!(!docs.prompt); } +#[test] +fn add_allow_fs_path_removes_later_prompt_duplicate_when_silent_exists_first() { + let base = r#"schema_version = 1 +[model] +provider = "anthropic" +name = "t" + +[capabilities.allow.read] +default_files = ["*.rs"] +paths = ["src/**", { path = "src/**", prompt = true }, "docs/**"] +"#; + let out = add_allow_fs_path(base, FsKind::Read, "src/**", false).unwrap(); + + assert!(!out.contains("prompt = true"), "{out}"); + assert_eq!(out.matches(r#""src/**""#).count(), 1, "{out}"); + assert!(out.contains(r#""docs/**""#), "{out}"); +} + +#[test] +fn flip_allow_fs_prompt_false_removes_later_prompt_duplicate_when_silent_exists_first() { + let base = r#"schema_version = 1 +[model] +provider = "anthropic" +name = "t" + +[capabilities.allow.read] +default_files = ["*.rs"] +paths = ["src/**", { path = "src/**", prompt = true }, "docs/**"] +"#; + let out = flip_allow_fs_prompt_false(base, FsKind::Read, "src/**").unwrap(); + + assert!(!out.contains("prompt = true"), "{out}"); + assert_eq!(out.matches(r#""src/**""#).count(), 1, "{out}"); + assert!(out.contains(r#""docs/**""#), "{out}"); +} + +#[test] +fn flip_allow_fs_prompt_false_collapses_multiple_prompt_duplicates() { + let base = r#"schema_version = 1 +[model] +provider = "anthropic" +name = "t" + +[capabilities.allow.read] +default_files = ["*.rs"] +paths = [ + { path = "docs/**", prompt = true }, + { path = "docs/**", prompt = true }, +] +"#; + let out = flip_allow_fs_prompt_false(base, FsKind::Read, "docs/**").unwrap(); + + assert!(!out.contains("prompt = true"), "{out}"); + assert_eq!(out.matches(r#""docs/**""#).count(), 1, "{out}"); +} #[test] fn add_allow_fs_path_accepts_inline_table_for_narrow_form() { // The TUI's Tab-narrow produces `{ path = "docs", files = ["x.md"] }`.