fix(rules): evaluate subshell-wrapped arguments through the wrapper path#297
Merged
fix(rules): evaluate subshell-wrapped arguments through the wrapper path#297
Conversation
intent(rules/command_parser): fix `time (lefthook run pre-commit 2>&1 | tail -40)`
and similar wrapper-over-subshell forms falling through to the default
action because neither the wrapper pattern nor the compound-command
path could see the subshell argument as a single unit
learned(rules/command_parser): tree-sitter-bash parses `time (...)` as a
single `command` node with the subshell as a named child, so
`extract_commands_with_metadata` emits exactly one `time (...)` string
and the compound guard never splits it — the fix has to live at the
tokenize layer that `parse_command` uses for wrapper placeholder
extraction
decision(rules/command_parser): teach `tokenize` to keep `(...)`, `$(...)`,
backtick substitutions, and `${...}` as single atoms (with nesting and
quoted-content awareness) instead of special-casing `time` — the fix is
then uniform across every wrapper (`sudo (...)`, `env (...)`, etc.) and
matches how shell word-splitting actually treats these groupings
rejected(rules/command_parser): handling the subshell inside
`collect_commands` by synthesising separate `time` + inner sub-commands
— it forces a wrapper-vs-compound coupling into the AST walker and
still leaves `parse_command`'s whitespace tokenizer broken for any
other caller
intent(docs/releases): replace the #000 placeholder now that the PR exists
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #297 +/- ##
==========================================
+ Coverage 89.55% 89.57% +0.01%
==========================================
Files 52 52
Lines 11015 11080 +65
==========================================
+ Hits 9865 9925 +60
- Misses 1150 1155 +5
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:
|
![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.
![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.
…ading subshells
intent(rules/command_parser): stop `read_balanced_group` from being
tripped by a `)` embedded inside a backtick substitution, a nested
`$(...)`, or a `${...}` parameter expansion — before this change,
inputs like `time (echo ${FOO:-)} done)` silently lost the trailing
content of the outer subshell
learned(rules/command_parser): the outer-group reader only looked at
plain paren nesting and quoted strings, so any inner construct that
can legitimately contain an unmatched `)` acted as a false terminator;
the `` `...` `` / `$(...)` / `${...}` cases all need the same
skip-over treatment used by the top-level tokenizer
…-sitter AST
intent(rules/command_parser): let wrapper placeholder extraction see a
whole subshell as a single `<cmd>` argument, so `time (lefthook run
pre-commit 2>&1 | tail -40)` routes through the `time <cmd>` wrapper
path and gets its body split into sub-commands instead of falling
through to `defaults.action`
decision(rules/command_parser): walk the tree-sitter-bash AST when the
input parses as a single `command` node and emit one token per named
child, preserving `subshell` / `command_substitution` /
`process_substitution` verbatim; run everything else through the
existing whitespace tokenizer as a fallback. This keeps `(...)` and
`$(...)` fully symmetric at the wrapper layer and avoids re-inventing
shell quoting in a character-level state machine
rejected(rules/command_parser): adding a `(...)` / `$(...)` / `${...}`
special case to the whitespace tokenizer — it duplicated tree-sitter's
balancing and quoting logic and drifted further every time a new
construct came up; the AST walk is the same rule tree-sitter already
applies to every other form of compound extraction
learned(rules/command_parser): the real asymmetry that made `time (...)`
fail was that `collect_commands`'s `command` branch recursed into
`command_substitution` children but not `subshell` children. Adding
`subshell` to the recursion list makes the compound extractor treat
both the same, and letting `parse_command` consult the AST instead of
whitespace tokens is what actually unblocks the wrapper path
fohte
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
time (ls | tail -40), where a wrapper's argument is a bare subshell, are not parsed as a wrapped formWhat
<cmd>placeholder