fix: collapse duplicate prompt grants on allow always#698
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughTOML allow-entry upserts now normalize ChangesAllow-entry prompt normalization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has required the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/seal-tui/src/toml_editor/fs.rs`:
- Around line 27-30: The prompt-stripping path in fs_entry_without_prompt should
preserve the same no-default-files fallback as the prompt=false builder path.
Update normalize_allow_fs_prompt_false, add_allow_fs_path, and
flip_allow_fs_prompt_false so matched shorthand entries in sections without
default_files still serialize through the explicit files = ["*"] fallback
instead of becoming a bare string; keep the prompt=false upsert behavior
consistent with the normal path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 47dcda6b-e592-42a7-87c7-4bc182cf1dae
📒 Files selected for processing (3)
crates/seal-tui/src/toml_editor/commands.rscrates/seal-tui/src/toml_editor/fs.rscrates/seal-tui/src/toml_editor/tests.rs
| if !prompt && normalize_allow_fs_prompt_false(paths, pattern, &defaults) { | ||
| return Ok(()); | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the no-default-files fallback when stripping prompt.
For matched shorthand entries, fs_entry_without_prompt now writes a bare string. In sections without default_files, that bypasses the existing prompt=false builder path that emits the explicit files = ["*"] fallback, so add_allow_fs_path(..., false) and flip_allow_fs_prompt_false can serialize a different rule than the normal prompt=false upsert.
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
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/seal-tui/src/toml_editor/fs.rs` around lines 27 - 30, The
prompt-stripping path in fs_entry_without_prompt should preserve the same
no-default-files fallback as the prompt=false builder path. Update
normalize_allow_fs_prompt_false, add_allow_fs_path, and
flip_allow_fs_prompt_false so matched shorthand entries in sections without
default_files still serialize through the explicit files = ["*"] fallback
instead of becoming a bare string; keep the prompt=false upsert behavior
consistent with the normal path.
Greptile SummaryThis PR fixes a bug where clicking "Allow always" could leave the TOML config with both a silent grant and a
Confidence Score: 4/5Safe to merge; the targeted bug is correctly fixed, no data is lost, and the new tests verify the intended behaviour. Two non-blocking rough edges noted. The core logic is sound for the common cases. There are two residual observations: the commands normalizer does not deduplicate identical extra-policy entries (the FS normalizer does), and the
Important Files Changed
Reviews (1): Last reviewed commit: "fix: collapse duplicate prompt grants on..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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; | ||
| } |
There was a problem hiding this comment.
normalize_allow_command_prompt_false correctly collapses duplicate simple entries (only command/prompt keys) by tracking kept_simple and removing extras, but the command_entry_has_extra_policy branch replaces every matching entry individually without any deduplication guard. If the array contains two identical entries such as { command = "curl:*", domains = ["x.com"], prompt = true } twice, both have their prompt key removed but both remain — producing [{ command = "curl:*", domains = ["x.com"] }, { command = "curl:*", domains = ["x.com"] }].
This differs from normalize_allow_fs_prompt_false, which removes all entries after the first unconditionally. A test covering flip_prompt_false on duplicate identical extra-policy entries would make the intended behaviour explicit.
| 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(()); |
There was a problem hiding this comment.
(true, false) arm is now unreachable when prompt = false
normalize_allow_fs_prompt_false is called just above with the same defaults slice and the same paths array; it scans every entry using fs_entry_path_matches. If it returns true (at least one match), the function has already returned early. If it returns false, no entry matched — so the subsequent for loop can also never find a match via fs_entry_path_matches, and the (true, false) arm here can never fire. The only effect of this loop on the prompt = false path is to fall through to push_preserving_format, which happens anyway. The (true, false) branch and the loop itself could be guarded by if prompt (or removed for the !prompt case) to make it clear this scan only matters when prompt = true.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/seal-tui/src/toml_editor/fs.rs">
<violation number="1" location="crates/seal-tui/src/toml_editor/fs.rs:27">
P3: The `(true, false)` arm in the `for` loop is unreachable when `prompt = false`. `normalize_allow_fs_prompt_false` is called just above with the same array and matching logic (`fs_entry_path_matches`). If it returns `true`, the function already returned. If it returns `false`, no entry matched — so this loop also cannot find a match via `fs_entry_path_matches`, making the `(true, false)` branch dead code on the `!prompt` path. Consider guarding this loop with `if prompt` or removing the unreachable arm to clarify intent.</violation>
<violation number="2" location="crates/seal-tui/src/toml_editor/fs.rs:319">
P1: `fs_entry_without_prompt` always collapses non-`files` entries to a bare string, but when `section_default_files` is empty, the normal upsert path emits `{ path = "...", files = ["*"] }` to ensure the grant is effective. This means `normalize_allow_fs_prompt_false` can produce a different (potentially less permissive) rule than the standard `prompt=false` upsert when there are no default files configured for the section. Pass `section_default_files` to `fs_entry_without_prompt` and use `build_fs_entry_value` when defaults are empty.</violation>
</file>
<file name="crates/seal-tui/src/toml_editor/commands.rs">
<violation number="1" location="crates/seal-tui/src/toml_editor/commands.rs:241">
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 have `prompt` removed but both remain in the output — producing two identical `{ command = "curl:*", domains = ["x.com"] }` entries. This differs from `normalize_allow_fs_prompt_false` which removes all entries after the first unconditionally. Consider tracking whether an extra-policy entry was already kept and removing subsequent identical ones.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| if found { | ||
| paths.remove(i); | ||
| } else { | ||
| let replacement = fs_entry_without_prompt(value, pattern); |
There was a problem hiding this comment.
P1: fs_entry_without_prompt always collapses non-files entries to a bare string, but when section_default_files is empty, the normal upsert path emits { path = "...", files = ["*"] } to ensure the grant is effective. This means normalize_allow_fs_prompt_false can produce a different (potentially less permissive) rule than the standard prompt=false upsert when there are no default files configured for the section. Pass section_default_files to fs_entry_without_prompt and use build_fs_entry_value when defaults are empty.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/seal-tui/src/toml_editor/fs.rs, line 319:
<comment>`fs_entry_without_prompt` always collapses non-`files` entries to a bare string, but when `section_default_files` is empty, the normal upsert path emits `{ path = "...", files = ["*"] }` to ensure the grant is effective. This means `normalize_allow_fs_prompt_false` can produce a different (potentially less permissive) rule than the standard `prompt=false` upsert when there are no default files configured for the section. Pass `section_default_files` to `fs_entry_without_prompt` and use `build_fs_entry_value` when defaults are empty.</comment>
<file context>
@@ -292,33 +289,54 @@ pub(super) fn flip_fs_prompt_in_array(
+ if found {
+ paths.remove(i);
+ } else {
+ let replacement = fs_entry_without_prompt(value, pattern);
+ paths.replace(i, replacement);
+ found = true;
</file context>
| } | ||
|
|
||
| found = true; | ||
| if command_entry_has_extra_policy(value) { |
There was a problem hiding this comment.
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 have prompt removed but both remain in the output — producing two identical { command = "curl:*", domains = ["x.com"] } entries. This differs from normalize_allow_fs_prompt_false which 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
Check if this issue is valid — if so, understand the root cause and fix it. At crates/seal-tui/src/toml_editor/commands.rs, line 241:
<comment>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 have `prompt` removed but both remain in the output — producing two identical `{ command = "curl:*", domains = ["x.com"] }` entries. This differs from `normalize_allow_fs_prompt_false` which removes all entries after the first unconditionally. Consider tracking whether an extra-policy entry was already kept and removing subsequent identical ones.</comment>
<file context>
@@ -224,16 +220,57 @@ fn ensure_command_domains_array<'a>(doc: &'a mut DocumentMut, side: &str) -> Res
+ }
+
+ found = true;
+ if command_entry_has_extra_policy(value) {
+ let replacement = command_entry_without_prompt(value);
+ patterns.replace(i, replacement);
</file context>
| 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.
P3: The (true, false) arm in the for loop is unreachable when prompt = false. normalize_allow_fs_prompt_false is called just above with the same array and matching logic (fs_entry_path_matches). If it returns true, the function already returned. If it returns false, no entry matched — so this loop also cannot find a match via fs_entry_path_matches, making the (true, false) branch dead code on the !prompt path. Consider guarding this loop with if prompt or removing the unreachable arm to clarify intent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/seal-tui/src/toml_editor/fs.rs, line 27:
<comment>The `(true, false)` arm in the `for` loop is unreachable when `prompt = false`. `normalize_allow_fs_prompt_false` is called just above with the same array and matching logic (`fs_entry_path_matches`). If it returns `true`, the function already returned. If it returns `false`, no entry matched — so this loop also cannot find a match via `fs_entry_path_matches`, making the `(true, false)` branch dead code on the `!prompt` path. Consider guarding this loop with `if prompt` or removing the unreachable arm to clarify intent.</comment>
<file context>
@@ -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(());
+ }
</file context>
1 participant
Pull request
Summary
Fixes a bug where choosing "Allow always" could leave duplicate entries in the TOML config — one silent grant and one with
prompt = true. The fix collapses all matching entries into a single silent grant, droppingprompt = truewhile preserving any other policy fields on inline tables.Changes
normalize_allow_command_prompt_falseto handle deduplication of command allow entries, replacing the earlier single-match-and-return logicnormalize_allow_fs_prompt_falseto do the same for filesystem path entriesflip_prompt_in_arrayandflip_fs_prompt_in_arraynow delegate to these shared normalization functionsupsert_allow_commandandupsert_allow_fs_pathcall the normalization path before falling through to append, preventing silent grants from being added alongside existing prompted onescommand/prompt) are preserved with only thepromptkey removed rather than collapsed to a bare stringTest plan
add_allow_removes_later_prompt_duplicate_when_silent_exists_firstfor commandsflip_prompt_false_removes_later_prompt_duplicate_when_silent_exists_firstfor commandsflip_prompt_false_collapses_multiple_prompt_duplicatesfor commandsadd_allow_fs_path_removes_later_prompt_duplicate_when_silent_exists_firstfor FS pathsflip_allow_fs_prompt_false_removes_later_prompt_duplicate_when_silent_exists_firstfor FS pathsflip_allow_fs_prompt_false_collapses_multiple_prompt_duplicatesfor FS paths