fix(rules): allow flag-only negation to pass with empty command tokens#179
fix(rules): allow flag-only negation to pass with empty command tokens#179
Conversation
…emain Flag-only negation patterns (e.g. `!-o|--output`) scan all command tokens for forbidden flags using order-independent matching. When the command had zero tokens after the program name (e.g. bare `sort`), the early `cmd_tokens.is_empty()` guard returned false for all negations indiscriminately, even though an absent flag trivially satisfies a flag-only negation. Additionally, flag-only negations were consuming one positional token on success (advancing to `cmd_tokens[1..]`), which is incorrect since they perform a whole-list scan rather than matching a specific position. Split the negation handling into two branches: - Flag-only: allow empty tokens (flag absent = pass), do not consume - Positional: preserve existing behavior (empty = fail, consume one) Co-Authored-By: Claude Opus 4.6 <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #179 +/- ##
==========================================
+ Coverage 89.86% 89.92% +0.05%
==========================================
Files 38 38
Lines 7212 7225 +13
==========================================
+ Hits 6481 6497 +16
+ Misses 731 728 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The patch coverage was low because the positional negation branch and the flag-only negation empty-tokens branch inside extract_placeholder_all were not exercised by existing tests. Add three extract_placeholder_cases: positional negation with tokens, positional negation with empty tokens, and flag-only negation with empty tokens. Co-Authored-By: Claude Opus 4.6 <[email protected]>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…tests The flag-only negation rejection branch (line 519) and the positional negation rejection branch (line 534) were not covered. Add test cases where the forbidden flag/token is present so the negation fails. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The Negation descriptions in the architecture and pattern-syntax docs did not reflect that flag-only negation does not consume a positional token and passes when no command tokens remain. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a bug in the rule engine's pattern matching for flag-only negations. Previously, rules using flag-only negations would incorrectly reject commands that had no arguments, even if the forbidden flag was absent. The update modifies the matching logic to correctly interpret the absence of command tokens as the absence of the negated flag, allowing such commands to pass. This change improves the accuracy and consistency of rule evaluation, particularly for commands with minimal arguments. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a bug where flag-only negations failed on commands with no arguments and ensures they don't consume a command token. This improvement enhances the accuracy of the pattern matching engine, and no new security vulnerabilities were introduced. I've also suggested some refactorings to reduce code duplication in the negation handling logic, which would improve maintainability.
| PatternToken::Negation(inner) => { | ||
| if cmd_tokens.is_empty() { | ||
| return false; | ||
| } | ||
| // Flag-only negations (all inner elements start with `-`, excluding | ||
| // bare `--`) use order-independent matching: scan the entire command | ||
| // token list. Other negations check only the positional token. | ||
| let negation_passed = if is_flag_only_negation(inner) { | ||
| !cmd_tokens | ||
| if is_flag_only_negation(inner) { | ||
| // Flag-only negations scan the entire command token list for | ||
| // the forbidden flag (order-independent). When no tokens remain | ||
| // the flag is trivially absent, so the negation passes. | ||
| let negation_passed = !cmd_tokens | ||
| .iter() | ||
| .any(|t| match_flag_token_with_equals(inner, t, definitions)) | ||
| } else { | ||
| !match_single_token(inner, cmd_tokens[0], definitions) | ||
| }; | ||
| if negation_passed { | ||
| match_tokens_core(rest, &cmd_tokens[1..], definitions, steps, captures) | ||
| .any(|t| match_flag_token_with_equals(inner, t, definitions)); | ||
| if negation_passed { | ||
| // Flag-only negations do not consume a positional token. | ||
| match_tokens_core(rest, cmd_tokens, definitions, steps, captures) | ||
| } else { | ||
| false | ||
| } | ||
| } else { | ||
| false | ||
| if cmd_tokens.is_empty() { | ||
| return false; | ||
| } | ||
| let negation_passed = !match_single_token(inner, cmd_tokens[0], definitions); | ||
| if negation_passed { | ||
| match_tokens_core(rest, &cmd_tokens[1..], definitions, steps, captures) | ||
| } else { | ||
| false | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for handling flag-only and positional negations has some duplication. You can refactor this to reduce redundancy and make the distinction between the two cases (the number of tokens consumed) more explicit. This will improve maintainability.
PatternToken::Negation(inner) => {
let (negation_passed, consumed) = if is_flag_only_negation(inner) {
// Flag-only negations scan the entire command token list.
// If no tokens remain, the flag is trivially absent.
// They do not consume a positional token.
let passed = !cmd_tokens
.iter()
.any(|t| match_flag_token_with_equals(inner, t, definitions));
(passed, 0)
} else {
// Positional negations check only the next token.
if cmd_tokens.is_empty() {
return false;
}
let passed = !match_single_token(inner, cmd_tokens[0], definitions);
(passed, 1)
};
if negation_passed {
match_tokens_core(rest, &cmd_tokens[consumed..], definitions, steps, captures)
} else {
false
}
}| PatternToken::Negation(inner) => { | ||
| if cmd_tokens.is_empty() { | ||
| return Ok(()); | ||
| } | ||
| let negation_passed = if is_flag_only_negation(inner) { | ||
| !cmd_tokens | ||
| if is_flag_only_negation(inner) { | ||
| let negation_passed = !cmd_tokens | ||
| .iter() | ||
| .any(|t| match_flag_token_with_equals(inner, t, definitions)) | ||
| .any(|t| match_flag_token_with_equals(inner, t, definitions)); | ||
| if negation_passed { | ||
| extract_placeholder_all( | ||
| rest, | ||
| cmd_tokens, | ||
| definitions, | ||
| steps, | ||
| captured, | ||
| all_candidates, | ||
| )?; | ||
| } | ||
| } else { | ||
| !match_single_token(inner, cmd_tokens[0], definitions) | ||
| }; | ||
| if negation_passed { | ||
| extract_placeholder_all( | ||
| rest, | ||
| &cmd_tokens[1..], | ||
| definitions, | ||
| steps, | ||
| captured, | ||
| all_candidates, | ||
| )?; | ||
| if cmd_tokens.is_empty() { | ||
| return Ok(()); | ||
| } | ||
| let negation_passed = !match_single_token(inner, cmd_tokens[0], definitions); | ||
| if negation_passed { | ||
| extract_placeholder_all( | ||
| rest, | ||
| &cmd_tokens[1..], | ||
| definitions, | ||
| steps, | ||
| captured, | ||
| all_candidates, | ||
| )?; | ||
| } | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Similar to match_tokens_core, the logic for PatternToken::Negation here has some duplicated code. Refactoring this would improve clarity and maintainability by making the difference in token consumption explicit.
PatternToken::Negation(inner) => {
let (negation_passed, consumed) = if is_flag_only_negation(inner) {
let passed = !cmd_tokens
.iter()
.any(|t| match_flag_token_with_equals(inner, t, definitions));
(passed, 0)
} else {
if cmd_tokens.is_empty() {
return Ok(());
}
let passed = !match_single_token(inner, cmd_tokens[0], definitions);
(passed, 1)
};
if negation_passed {
extract_placeholder_all(
rest,
&cmd_tokens[consumed..],
definitions,
steps,
captured,
all_candidates,
)?;
}
Ok(())
}
…into single match_engine Two nearly-identical recursive matching functions (match_tokens_core for boolean matching, extract_placeholder_all for placeholder extraction) duplicated all PatternToken variant handling. Bug fixes applied to one function were frequently missed in the other (PR #175, #179, #180). Merge both into a single `match_engine` function parameterized by an optional `extract` tuple. Each PatternToken variant's logic now exists in exactly one place. Also introduce a flag normalization layer (`split_flag_equals`, `flag_aliases_match_token`) to centralize `--flag=value` splitting, replacing the ad-hoc `=`-joined handling scattered across match_flag_token_with_equals, FlagWithValue, and optional_flags_absent. Co-Authored-By: Claude Opus 4.6 <[email protected]>
1 participant
Why
!-o|--output|--compress-program) incorrectly returnedfalsewhen the command had zero tokens after the program namesort !-o|--output|--compress-program *did not matchsort(no arguments), resulting in ask instead of allowcat *matchedcat(no arguments), so the behavior was inconsistentWhat
cmd_tokens.is_empty()guard returned false for all negations without distinguishing flag-only from positional