fix: keep the rest of the command line when expanding a wildcard - #603
fix: keep the rest of the command line when expanding a wildcard#603webpro wants to merge 3 commits into
Conversation
The wildcard expander matched the runner invocation with
/((?:npm|yarn|pnpm|bun) run|node --run|deno task) (\S+)([^&]*)/
and rebuilt the command as `${command} ${script}${args}`. Everything from
the first `&` onward was captured by nothing and dropped, so
concurrently 'npm run build:* && echo done'
ran the two builds and silently discarded `&& echo done`, exiting 0. The
same truncation cut
concurrently 'npm run build:* -- --grep "a & b"'
mid-string, leaving `/bin/sh` with an unterminated quote. The pattern is
also unanchored, so `echo "npm run build:*"` was expanded as if it were a
runner invocation and, finding no script ending in a quote, returned no
commands at all.
Locate the invocation in the parsed command instead, and substitute the
script in place using the source span of the glob, so every other byte of
the command line is preserved. concurrently's own `(!...)` omission
syntax is extglob, which the parser reads as one word, so it needs no
special handling.
gustavohenke
left a comment
There was a problem hiding this comment.
Thanks for finding and fixing this bug.
One aspect that's not touched on here is what's the Windows support.
I see unbash states that powershell/cmd are unsupported, but what would happen if their syntax went through - does it throw, or do we get some partial structure that we can work with? Is some sort of fallback handling necessary?
I also added some comments around cleaning up the code.
| type Word = { value: string; pos: number; end: number }; | ||
| type Runner = { command: string; glob: Word }; |
There was a problem hiding this comment.
Are these not exposed from unbash?
| function findRunner(node: unknown): Runner | undefined { | ||
| if (!node || typeof node !== 'object') { | ||
| return undefined; | ||
| } | ||
|
|
||
| const candidate = node as { type?: string; name?: Word; suffix?: Word[] }; |
There was a problem hiding this comment.
ditto here; are there types exposed from unbash that'd lead to cleaner code?
Good point, and a bit of a rabbit hole. Good learnings for positioning unbash. Overall, this pull request fixes real issues (in environments that use it the most?), but also introduces additional complexity. Below the line is the full AI-generated story. Happy to keep iterating on this PR, but this hopefully explains enough re. responsibilities of concurrently vs unbash and first decide whether it's worth pursuing getting this merged. The Windows concern is handled using the existing The additional name check is needed because For direct runner commands that parse without errors on these shells, the PR identifies the script argument and replaces just that word. Compared with the published implementation:
Unbash owns Bash parsing, source ranges, and diagnostics. Concurrently owns the choice of parser, supported runner discovery, wildcard and omission matching, and what to do with diagnostics. Unbash already returns an AST alongside reported parse errors; this integration falls back to the old recognizer when errors are present instead of transforming that recovered AST. Confirmed Bash parsing defects belong in unbash. The README comparison shows unbash leading the measured parse-throughput benchmarks and supporting Bash constructs that There are several costs and limits to that approach:
concurrently --shell sh 'npm run "test:*-unit(!slow)" && echo done'On the type comments: The 40 common expansion cases run against both paths with explicit expected outputs. Separate cases cover shell selection, source preservation, quoting, omissions, wrappers, and nested invocations. All 726 unit and smoke tests pass, including the build and CJS/ESM checks. Another 126 legacy comparisons match published 10.0.5 exactly, and CLI checks exercise the command-chain and quoting behavior. CMD and PowerShell coverage checks expansion and spawn arguments; native Windows execution was not tested locally. |
57d1d7d to
703014d
Compare
gustavohenke
left a comment
There was a problem hiding this comment.
Thanks for that.
Overall, this pull request fixes real issues (in environments that use it the most?), but also introduces additional complexity [...] first decide whether it's worth pursuing getting this merged.
Agree. The documented bug should be fixed, but maybe not in this shape.
As it stands, this PR makes concurrently both much more robust, but also a bit more brittle.
For example, the allowlist means that standard POSIX would still cause the bug on zsh, since it's not strictly the bash syntax that unbash works with. Maintaining two recognisers is not ideal either.
I feel like you ended up going down the dual recogniser path due to unbash being faithful to bash syntax, so I'm wondering if we can make the fix less architecturally dramatic, while still keeping a single wildcard expansion path. Could we
- remove the shell gating
- use a tokenizer (whichever it is, shell-quote's or unbash's) to find a valid
runner + subcommand + scriptsequence, and replace just that script token - leave as much as possible of the user input unchanged, so that invalid syntax blows up at runtime, using the configured shell
?
| private readonly readDeno = ExpandWildcard.readDeno, | ||
| private readonly readPackage = ExpandWildcard.readPackage, | ||
| ) {} | ||
| shell?: string, |
There was a problem hiding this comment.
Make this the first argument, so you don't need to do the awkward undefined, undefined, shell in lib/concurrently.ts
|
Yes, we could. Let's use the Bash parser ungated, as you suggest. Note that also a Bash tokenizer would not correctly handle every PowerShell/CMD input. unbash doesn't have/expose one (yet). Anyway, all 40 existing main wildcard tests pass unchanged. ✅ This fixes lost command chains, prefixes, wrappers, and quoted ampersands.
The first one concerns recognizing shell-specific prefixes. The quoting and omission failures are bugs in the replacement logic that need fixing within this approach. This all to illustrate the trade-offs. The issues presented feel like edge cases to me. And I think issues like 2 + 3 are fixable in concurrently (in this PR). I'm just not 100% sure how much of a complete picture it paints cross-environment (which is part of the reason why I went with 703014d initially). If this feels like a better direction we can try and bring this one over the finish line. |
The problem
ExpandWildcardfinds the runner invocation with a regex and rebuilds the command fromthe captured pieces:
([^&]*)stops at the first&, and nothing captures what follows, so it is dropped.The pattern is also unanchored, so it matches inside quoted text.
Three things go wrong, all reproducible with the published package:
A chained command is silently discarded.
An
&inside a quoted argument truncates the command mid-string.A runner named inside a quoted argument is expanded as if it were an invocation.
echo "npm run build:*"matches the pattern, finds no script ending in a quote, andreturns no commands at all, so the command disappears rather than running.
The change
Locate the invocation in the parsed command rather than in the raw text, then substitute the script using the source span of the glob:
Every other byte of the command line is preserved by construction, which is what fixes all three cases at once rather than one at a time. The runner lookup walks the parsed command, so a runner name inside a quoted argument is not a runner invocation and an invocation after
cd app &&still is.The
(!...)omission syntax needs no special handling: it is extglob, and the parser readswatch-*(!js)as a single word.About the dependency
This adds unbash, a zero dependency synchronous Bash parser with no WASM and no async initialisation. I wrote it. I also wrote knip, which depends on it, so it already runs against a large amount of real world shell: knip is at about 11.7M downloads a week and unbash at about 8.5M.
Worth noting given the two
shell-quoteadvisory bumps in #591 and #599:shell-quotestays, becauseexpand-arguments.tsuses itsquote(), which is a different job from parsing. This PR does not touch that.Verification
Both reproductions above were confirmed against the built binary before and after.