feat(wrapper): add <opts> and <vars> placeholders for wrapper patterns#84
feat(wrapper): add <opts> and <vars> placeholders for wrapper patterns#84
<opts> and <vars> placeholders for wrapper patterns#84Conversation
…er patterns
When a wrapper pattern like `xargs * <cmd>` matched `xargs -I{} echo hello`,
the wildcard `*` consumed 0 tokens (minimum match) and `<cmd>` captured
`-I{} echo hello` as a whole, causing the inner command to be evaluated as
`-I{}` (not a real command) and resulting in "No rules matched".
The root cause was that `extract_placeholder_inner` returned on the first
successful alignment without considering whether the captured command would
actually match any rules. Change the extractor to collect all valid capture
candidates, then let `try_unwrap_wrapper` evaluate each candidate and pick
the one with the highest action priority (Deny > Ask > Allow > Default).
Summary of ChangesHello @fohte, 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 addresses a critical flaw in how wrapper patterns handle the interaction between wildcards and command placeholders. By modifying the pattern matching logic to identify all possible interpretations of the wrapped command and then using the rule engine to prioritize the most relevant one, the system can now correctly parse and apply rules to complex commands like Highlights
Changelog
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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Pass named `captured` variable instead of inline `&mut Vec::new()` - Add comment explaining why empty captures are filtered (patterns without `<cmd>` placeholder produce empty captures on match) - Add comment on non-`<cmd>` placeholder behavior - Improve `extract_placeholder_all` doc comment
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #84 +/- ##
==========================================
+ Coverage 89.09% 89.90% +0.80%
==========================================
Files 29 29
Lines 6025 6129 +104
==========================================
+ Hits 5368 5510 +142
+ Misses 657 619 -38
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 `* <cmd>` combination in wrapper patterns is fundamentally ambiguous
because `*` greedily consumes tokens, making it impossible to
deterministically separate flags from the inner command. `<opts>` solves
this by consuming only hyphen-prefixed flag tokens, leaving the rest for
`<cmd>`.
`consume_opts` treats short single-char flags (e.g., `-n`) as
potentially taking a separate argument, while multi-char flags
(`-I{}`, `-0`, `--verbose`) are self-contained.
fohte
changed the title
<opts> placeholder for deterministic flag consumption in wrapper patterns
Gemini review pointed out that the ordering of candidates (shortest to longest) was not documented. This ordering matters for the rule engine's priority-based selection.
![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.
…r_all Empty captures (from patterns without `<cmd>` or where `<cmd>` matched nothing) were previously filtered out at the call site. Move the guard into `extract_placeholder_all` itself so invalid candidates are never produced, making the logic easier to follow.
Cover previously untested code paths in `extract_placeholder_all`: Literal, Alternation, FlagWithValue, Negation, Wildcard branches, the base-case candidate push when `captured` is non-empty, non-`<cmd>` trailing placeholders, and `<cmd>` in middle position followed by a literal sentinel. Also cover `<opts>` in non-wrapper pattern matching context. Patch coverage improved from 63% to 93%.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
`env <opts> <cmd>` cannot deterministically separate `FOO=bar` variable assignments from the inner command because `<opts>` only consumes hyphen-prefixed flags. `<vars>` consumes consecutive tokens containing `=`, allowing wrapper patterns like `env <opts> <vars> <cmd>` to unambiguously parse `env -i FOO=bar echo hello`. Also update existing `env * <cmd>` test to use `env <opts> <vars> <cmd>` since the new placeholders provide deterministic parsing.
fohte
changed the title
<opts> placeholder for deterministic flag consumption in wrapper patterns<opts> and <vars> placeholders for wrapper patterns
`consume_opts` incorrectly treated `-0` as a short flag that consumes the next token as its argument. Since `-0` is a self-contained boolean flag (like `xargs -0`), only flags whose second character is an ASCII letter should consume a separate argument. Also, `--` (POSIX end-of-options marker) was not handled; it was consumed as a regular flag. Now `--` terminates option scanning and is itself consumed so it does not leak into `<cmd>`.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ume_opts` The doc comment for `consume_opts` was placed directly above `consume_vars`, causing Rust to attach it to the wrong function. Move it to its intended location and update the wording to reflect the ASCII-letter check and `--` handling.
…dcard-greedy # Conflicts: # src/rules/pattern_matcher.rs # src/rules/rule_engine.rs
1 participant
Why
* <cmd>combinations in wrapper patterns (e.g.,xargs * <cmd>) cause*to consume 0 tokens, leaving<cmd>to capture the entire remaining input including flags that are then misinterpreted as the command namexargs -I{} echo helloresults in<cmd>capturing-I{} echo hello, evaluating-I{}as the command name*+<cmd>combination is fundamentally ambiguous — even trying all candidates produces false matches underdefaults.actionsettingsenv * <cmd>has no way to distinguishFOO=barvariable assignment tokens from flagsWhat
<opts>placeholder for wrapper patterns to deterministically consume hyphen-prefixed flag tokens-n) consume the next token as their argument, while multi-char flags (-I{},--verbose) are treated as self-contained<vars>placeholder for wrapper patterns to deterministically consumeKEY=VALUEtokens<opts>asenv <opts> <vars> <cmd>, this correctly parses commands likeenv -i FOO=bar echo helloextract_placeholderto collect all candidates so* <cmd>patterns still work for simpler cases