-
Notifications
You must be signed in to change notification settings - Fork 0
fix: collapse duplicate prompt grants on allow always #698
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 = "<pat>"`. | ||
| 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<bool> { | ||
| 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; | ||
| } | ||
|
Comment on lines
+241
to
+251
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This differs from |
||
| } | ||
| 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`. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: The Prompt for AI agents |
||
| return Ok(()); | ||
| } | ||
|
|
||
|
Comment on lines
+27
to
+30
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Preserve the no-default-files fallback when stripping For matched shorthand entries, Proposed fix- let replacement = fs_entry_without_prompt(value, pattern);
+ let replacement = fs_entry_without_prompt(value, pattern, section_default_files);
paths.replace(i, replacement);
@@
-fn fs_entry_without_prompt(value: &Value, pattern: &str) -> Value {
+fn fs_entry_without_prompt(
+ value: &Value,
+ pattern: &str,
+ section_default_files: &[String],
+) -> 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)
+ if section_default_files.is_empty() {
+ build_fs_entry_value(&path, false, false)
+ } else {
+ Value::from(path)
+ }
}
}
}Also applies to: 292-337 🤖 Prompt for AI Agents |
||
| 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(()); | ||
|
Comment on lines
37
to
44
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| } | ||
|
|
@@ -292,33 +289,54 @@ pub(super) fn flip_fs_prompt_in_array( | |
| pattern: &str, | ||
| section_default_files: &[String], | ||
| ) -> Result<bool> { | ||
| 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); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: Prompt for AI agents |
||
| 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( | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Duplicate entries with extra policy keys (e.g.,
domains) are not deduplicated. If the array contains two identical entries like{ command = "curl:*", domains = ["x.com"], prompt = true }, both will havepromptremoved but both remain in the output — producing two identical{ command = "curl:*", domains = ["x.com"] }entries. This differs fromnormalize_allow_fs_prompt_falsewhich removes all entries after the first unconditionally. Consider tracking whether an extra-policy entry was already kept and removing subsequent identical ones.Prompt for AI agents