Uh oh!
There was an error while loading. Please reload this page.
fix(commands): correct multi-line quoted command parsing, auto-approval, and malformed-command error surfacing - #483
Conversation
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMasks top-level quoted spans and heredocs, detects unterminated quote regions, splits on unquoted newlines only after masking, restores placeholders per-line, and updates pattern extraction so multi-line quoted arguments and heredocs remain atomic for auto-approval and UI selectors. ChangesMulti-line quoted command parsing and auto-approval
Sequence Diagram(s)sequenceDiagram
participant Input as Raw command text
participant Parser as parseCommand
participant Auto as getCommandDecision
participant UI as extractPatternsFromCommand
Input->>Parser: maskTopLevelQuotes -> detect unterminated? -> split on unquoted newlines -> restore placeholders per-line
Parser-->>Auto: sub-commands (heredoc/quote-aware)
Auto->>Auto: evaluate leading words vs allowlist per sub-command
Auto-->>UI: decision + patterns
UI->>Parser: reuse parseCommand for pattern extraction
UI-->>Input: render selector spans (only leading patterns)
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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/shared/__tests__/parse-command.spec.ts (1)
60-68: ⚡ Quick winAdd ANSI-C escaped-apostrophe coverage to lock the multiline fix.
The ANSI-C block should also assert
$'...\'...\n...'stays a single command; this is the key edge case for the top-level masking path.Suggested test
describe("ANSI-C quoting ($'...')", () => { it("does not leak a placeholder for a $'...' multi-line argument", () => { const input = "sh -c $'echo 1\necho 2'" const result = parseCommand(input) // The placeholder used internally must never appear in the output. expect(result.join(" ")).not.toContain("SQUOTE") expect(result.join(" ")).not.toContain("__") }) ++ it("keeps ANSI-C strings with escaped apostrophes and newlines as one command", () => {+ const input = "sh -c $'echo it\\'s ok\necho done'"+ expect(parseCommand(input)).toEqual([input])+ }) })As per coding guidelines,
**/{__tests__,tests,test}/**/*.{test,spec}.{ts,tsx,js}should use package-local unit tests for parsing logic and edge-case behavior.🤖 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 `@src/shared/__tests__/parse-command.spec.ts` around lines 60 - 68, Add a unit test to the existing "ANSI-C quoting ($'...')" suite in src/shared/__tests__/parse-command.spec.ts that exercises an ANSI‑C string containing an escaped apostrophe and a newline (e.g. input like "sh -c $'echo \\'1\\'\necho 2'") and asserts parseCommand returns a single multi-line argument (join(" ") does not split it) and that the internal placeholder tokens ("SQUOTE" and "__") do not appear in the output; update the it(...) block or add a new it(...) alongside the existing test referencing parseCommand to lock the multiline masking/fix for the escaped‑apostrophe edge case.
🤖 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 `@src/shared/parse-command.ts`:
- Around line 47-53: The top-level masking regex in parse-command.ts (the masked
variable created by command.replace) doesn't handle ANSI-C $'...' quoting or
escaped apostrophes, causing premature termination and unmasked newlines; update
the replace logic to first recognize $'...'(ANSI-C) as a single token and to
make the single-quote branch escape-aware (accept backslash-escaped characters
inside single quotes) before falling back to double-quote handling, preserving
the existing topLevelQuotes.push and replacement pattern (__TOPLEVEL_QUOTE_n__)
so newlines inside those quoted blocks remain masked and the subsequent lines =
masked.split(...) call no longer splits inside ANSI-C or escaped-single-quote
strings.
---
Nitpick comments:
In `@src/shared/__tests__/parse-command.spec.ts`:
- Around line 60-68: Add a unit test to the existing "ANSI-C quoting ($'...')"
suite in src/shared/__tests__/parse-command.spec.ts that exercises an ANSI‑C
string containing an escaped apostrophe and a newline (e.g. input like "sh -c
$'echo \\'1\\'\necho 2'") and asserts parseCommand returns a single multi-line
argument (join(" ") does not split it) and that the internal placeholder tokens
("SQUOTE" and "__") do not appear in the output; update the it(...) block or add
a new it(...) alongside the existing test referencing parseCommand to lock the
multiline masking/fix for the escaped‑apostrophe edge case.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 19a3d17f-83ae-474d-adb6-22d1cb4b3ff0
📒 Files selected for processing (5)
.changeset/fix-multiline-quoted-command-parsing.mdsrc/core/auto-approval/__tests__/commands.spec.tssrc/shared/__tests__/parse-command.spec.tssrc/shared/parse-command.tswebview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx
Uh oh!
There was an error while loading. Please reload this page.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| expect(result).toEqual(["sh -c 'echo 1\necho 2'"]) | ||
| expect(result[0]).toContain("\n") | ||
| }) | ||
| }) |
There was a problem hiding this comment.
What does parseCommand return for an unclosed quote like sh -c 'echo test (no closing quote)? The top-level regex '[^']*' requires a closing quote to match, so this input passes through unmasked and an embedded newline would still split — is that the intended contract? Worth a test to pin the behavior.
There was a problem hiding this comment.
Valid concern - regex based parser is not adequate to catch malformed inputs (which LLMs tend to generate when complex quoting is involved). I am reviewing and will re-submit this with a more robust parser.
There was a problem hiding this comment.
FYI - related yolo -c utility https://github.com/awschmeder/yolo-cli
There was a problem hiding this comment.
@edelauna I updated the PR adding a quote-syntax checker that detects if the command contains unterminated quotes and prevents it from reaching the regex, returning them as a single token for user approval.
if (findUnterminatedQuote(command) !== null) {
return [command]
}
A future PR should consider blocking commands that have an unterminated quote with an error message sent back to the agent for correction.
LLMs occasionally generate improper quote syntax in deeply nested quote sequences. In singular commands the shell will catch the parse error, but in a multipart command it can result in partial execution of the command sequence before the shell hits a parse error, which could have unintended side effects and creates more work for the agent to recover from.
a6527d8 to
10e8e56CompareThere 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 `@src/shared/parse-command.ts`:
- Around line 200-206: The current cross-line regex in parseCommand that builds
masked (replacing top-level quotes and pushing them into topLevelQuotes) is
comment-blind and can hide newlines inside comments; replace that regex with a
small state-machine that mirrors findUnterminatedQuote's rules: iterate
characters, maintain flags inSingleQuote, inDoubleQuote, inComment, and
escapeNext, treat '#' as starting a comment only when not in a quote and end
comments at EOL, push each full quoted span into topLevelQuotes and emit a
placeholder like __TOPLEVEL_QUOTE_n__ only when the quote is outside comments,
and copy raw characters otherwise; this preserves real newline separators for
masked.split(...) and fixes the command-splitting bug seen by
getCommandDecision.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c6d51868-5333-4af3-baf4-d7c75dbc19e9
📒 Files selected for processing (5)
.changeset/fix-multiline-quoted-command-parsing.mdsrc/core/auto-approval/__tests__/commands.spec.tssrc/shared/__tests__/parse-command.spec.tssrc/shared/parse-command.tswebview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- .changeset/fix-multiline-quoted-command-parsing.md
- src/core/auto-approval/tests/commands.spec.ts
- webview-ui/src/components/chat/tests/CommandExecution.spec.tsx
Uh oh!
There was an error while loading. Please reload this page.
awschmeder
commented
Jun 7, 2026
Regex parsing: current state after this changeTo clarify scope in response to the review note about cross-line regex masking: this PR converts the single hazardous cross-line quote-masking step to a state machine, but does not eliminate all regex parsing. Summary below. Converted to the state-machine parser
This was the only quote-aware, cross-line regex, and it was the source of the reported defect. Regex still in use (intentional, unchanged by this PR)These all run after the cross-line newline split (i.e. on a single line at a time) or operate on non-shell content, so the comment-blind cross-line hazard does not apply to them:
Out of scopeConverting the per-line tokenizers in |
awschmeder
commented
Jun 7, 2026
Shell Quoting Syntax Error -- Manual Test ResultsTests were run using Tests 1-9: Parse-Time RejectionIn all single-invocation cases,
Observation: Regardless of quoting style or compound operator used, Tests 10-11: Partial ExecutionTwo patterns were found that allow the first command to execute before the quoting error is Test 10 -- |
| Pattern | Execution behavior |
|---|---|
Single sh -c with any quoting style | Parse-time rejection -- nothing executes |
Compound operators (&&, ;, |) inside single sh -c | Parse-time rejection -- nothing executes |
eval with deferred broken string (ANSI-C outer quoting) | Partial execution -- first command runs, eval fails at runtime |
Two separate sh -c calls joined by ; | Partial execution -- first call completes, second fails at parse time |
The only reliable way to get partial execution with a quoting syntax error is to either defer
the broken string to a runtime evaluator (eval, bash -c, etc.) or split the commands into
separate shell invocations.
All sequences were correctly parsed and matched when 'sh -c' is on the Execute auto-approve list.
awschmeder
commented
Jun 7, 2026
Prompt for ZooCode: Paste this into the chat for manual testing Manual Test Procedure -- Multiline Command ParsingRun the following shell commands one at a time using your shell execution tool. All commands use Part 1: Unquoted newlines (should split)1. Three echo commands on separate lines split into three entriesecho a
echo b
echo cExpected: each line executes independently, producing output: Part 2: POSIX single-quoted multi-line argument (should NOT split)2. Newline inside single quotes is part of the argumentsh -c 'echo aecho b'Expected: both 3. Operators inside single quotes are literalsh -c 'echo hello && echo world'Expected: the Part 3: ANSI-C quoting ($'...') (should NOT split)4. ANSI-C quoted multi-line argument stays as one sh -c callsh -c $'echo a\necho b'Expected: 5. ANSI-C with escaped apostrophe and embedded newlinesh -c $'echo it\'s ok\necho done'Expected: syntax error, no output. Why: the ANSI-C string expands to the two-line literal script Part 4: Double-quoted multi-line argument (should NOT split)6. Newline inside double quotes is part of the argumentsh -c "echo aecho b"Expected: Part 5: Heredoc -- unquoted delimiter (should NOT split body)7. Unquoted delimiter heredocsh <<EOFecho helloecho worldEOFExpected: 8. Single-quoted delimiter heredoc (body is literal, no expansion)sh << 'EOF'echo helloecho worldEOFExpected: 9. Double-quoted delimiter heredoc (body allows expansion)sh << "EOF"echo helloecho worldEOFExpected: 10. <<- heredoc strips leading tabs from body and terminatorsh <<-EOF echo hello echo worldEOFExpected: 11. Multi-line heredoc body -- all body lines execute under one sh callsh << 'EOF'echo line1echo line2echo line3EOFExpected: 12. Command after heredoc terminator runs as a separate commandsh <<EOFecho insideEOFecho outsideExpected: ( 13. # comment inside heredoc body is preserved as literal textsh << 'EOF'# this is a commentecho helloEOFExpected: the comment line is passed to Part 6: Locale quoting ($"...") (should NOT split, $ prefix preserved)Note: 14. Locale-quoted string -- no leading $ in outputecho$"hello world"Expected: The 15. Operators inside locale-quoted string are not shell operatorssh -c 'echo $"hello && world"'Expected: The Part 7: Malformed / unterminated quotes (should not execute partial commands)16. Unterminated single quote -- shell should report a syntax error, not execute partial contentsh -c 'echo helloExpected: the shell reports an unmatched quote error. No partial output from 17. Unterminated heredoc -- bash executes the body at EOF (no error)sh <<EOFecho hello(Submit the input without an Expected on bash (all versions, non-interactive): Why: bash in non-interactive script execution treats end-of-file as an implicit Note -- intentional parser divergence: the Zoo Code Summary
|
2535cbf to
28c751bCompareThere was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/shared/parse-command.ts (1)
118-155: 💤 Low valueHeredoc tab-stripping is inconsistent with
maskTopLevelQuotes.Line 123 advances past
-for<<-but doesn't record whether tab-stripping should apply. Line 143 then unconditionally strips leading tabs from every potential terminator line. However,maskTopLevelQuotesat line 316 correctly only strips tabs whenstripTabsis true.For input like
cat << EOF\nhello\n\tEOF,findUnterminatedQuotewould consider\tEOFa valid terminator (incorrectly), whilemaskTopLevelQuoteswould not (correctly). The end result is still correct becausemaskTopLevelQuoteshandles it, but the inconsistency could cause confusion during maintenance.Suggested fix for consistency
if (char === "<" && command[i + 1] === "<") { const heredocOpenIndex = i i += 2 // skip << - if (command[i] === "-") i++ // optional - for <<-+ const stripTabs = command[i] === "-"+ if (stripTabs) i++ // optional - for <<- // Skip horizontal whitespace between << and the delimiter word.- // Strip leading tabs for <<- heredocs (terminator may be indented).- const line = command.slice(lineStart, i).replace(/^\t*/, "")+ // Strip leading tabs only for <<- heredocs (terminator may be indented).+ const rawLine = command.slice(lineStart, i)+ const line = stripTabs ? rawLine.replace(/^\t*/, "") : rawLine🤖 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 `@src/shared/parse-command.ts` around lines 118 - 155, In findUnterminatedQuote, record whether the heredoc opener used the '-' variant by setting a stripTabs boolean when you detect the optional '-' after '<<' (similar to maskTopLevelQuotes), pass or use that flag when processing terminator lines, and only apply the line.replace(/^\t*/, "") tab-stripping when stripTabs is true; keep the rest of the heredoc scanning and the parseHeredocDelimiter call unchanged so terminator matching behavior is consistent with maskTopLevelQuotes.
🤖 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 `@src/shared/parse-command.ts`:
- Around line 167-176: The locale-quote branch in parseCommand sets inDouble but
doesn't record that the opener was a locale quote, so when unterminated the
function returns quoteType "double" instead of "locale"; add a boolean flag
(e.g., localeQuote or inLocale) set when detecting the $"..." branch (alongside
inDouble and openIndex), mirror the existing singleIsAnsiC pattern used for
ANSI-C single quotes, and update the final quoteType selection logic to return
"locale" when that flag is set (ensure the flag is cleared/handled just like the
ANSI-C flag during termination/escape handling in parseCommand).
In `@webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx`:
- Around line 662-665: The test collects span text into fragments but only
checks exact equality, so heredoc internals can leak inside longer span strings;
update the assertions after the fragments =
Array.from(selector.querySelectorAll("span")).map(...) line to assert that none
of the fragment strings include the substrings "EOF", "echo", or "hello" (e.g.,
replace the .not.toContain checks with per-fragment substring checks like
ensuring fragments.some(f => f.includes("...")) is false for each of those
substrings) so any substring leak inside a span will fail.
---
Nitpick comments:
In `@src/shared/parse-command.ts`:
- Around line 118-155: In findUnterminatedQuote, record whether the heredoc
opener used the '-' variant by setting a stripTabs boolean when you detect the
optional '-' after '<<' (similar to maskTopLevelQuotes), pass or use that flag
when processing terminator lines, and only apply the line.replace(/^\t*/, "")
tab-stripping when stripTabs is true; keep the rest of the heredoc scanning and
the parseHeredocDelimiter call unchanged so terminator matching behavior is
consistent with maskTopLevelQuotes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b9fa94c1-407f-4243-a538-fabe5445f98c
📒 Files selected for processing (6)
.changeset/fix-multiline-quoted-command-parsing.mdsrc/shared/__tests__/parse-command.spec.tssrc/shared/parse-command.tswebview-ui/src/components/chat/__tests__/CommandExecution.spec.tsxwebview-ui/src/utils/__tests__/command-parser.spec.tswebview-ui/src/utils/command-parser.ts
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…or auto-approval parseCommand split on every newline before any quote handling, so newlines inside a quoted argument (e.g. a multi-line script passed to sh -c) were treated as separate commands. Single-quoted and ANSI-C ($'...') strings were also not fully masked, leaking placeholders and bogus sub-commands. This defeated allowlist auto-approval and produced a noisy command-pattern breakdown in the UI. Mask quoted strings (single, double, ANSI-C) before splitting on unquoted newlines, using a single left-to-right alternation so a quote of one style inside the other does not start a spurious match. Genuine unquoted newlines still split into separate sub-commands.
Handle $'...' (ANSI-C) quoting in parseCommand's pre-split masking so an escaped apostrophe inside the quoted body no longer terminates the match early and leak an embedded newline, which would split a single command into bogus sub-commands. Add a regression test covering an escaped apostrophe plus newline inside an ANSI-C argument.
Mock shell-quote parse() to throw and assert the fallback path restores the ANSI-C single-quote placeholder, closing the patch-coverage gap on the parse- failure branch.
Add findUnterminatedQuote, a quote-aware state-machine scanner that detects a command containing an unclosed quote (a shell syntax error, common in LLM-generated commands with nested quotes). parseCommand now returns such input as a single opaque token instead of splitting on embedded newlines, so a line intended to live inside the unclosed quote cannot surface as an independently auto-approvable sub-command.
The scanner returns { quoteType, openIndex } to support a future execution-layer rejection that surfaces a located error to the model; that pre-execution rejection is intentionally deferred to a follow-up.Replace the cross-line quote-masking regex in parseCommand() with a state machine (maskTopLevelQuotes) that mirrors findUnterminatedQuote. A quote inside a # comment no longer pairs with a quote on a later line, so a comment can no longer hide a real newline separator and merge two distinct commands.
…; fix pattern extractor - parseCommand: mask heredocs (<<, <<-, all delimiter quoting styles) as single atomic tokens before newline splitting; unterminated heredocs returned as opaque token - parseCommand: add locale-quote ($"...") support alongside existing ANSI-C ($'...') - findUnterminatedQuote: extend QuoteType with "locale" and "heredoc" variants - extractPatternsFromCommand (webview): pre-split via parseCommand before shell-quote tokenization, preventing spurious EOF/body-line/operator tokens in allow/deny selector - Update changeset to cover all three fix areas
…minatedQuote - Add explicit <<< passthrough in maskTopLevelQuotes: emit all three < chars verbatim and advance i by 3 so the second < does not re-trigger the heredoc branch on the next iteration - Same fix in findUnterminatedQuote for the same root cause - Add herestring test suite covering single-line, multi-command split, and single-quoted/ANSI-C quoted multiline word cases
- findUnterminatedQuote: track stripTabs for <<- so tab-stripping only applies when the heredoc opener used <<- (consistent with maskTopLevelQuotes) - findUnterminatedQuote: add doubleIsLocale flag so an unterminated $"..." region returns quoteType "locale" instead of "double" - findUnterminatedQuote: add tests for unterminated locale quote and balanced <<- with indented terminator - CommandExecution: exclude multi-line opaque tokens (heredoc bodies, unterminated quotes) from the raw-command pattern set so body-line words never surface as independently approvable patterns - CommandExecution.spec: strengthen fragment assertions to use per-fragment substring checks, exposing the CommandExecution leak bug
85b3adb to
f522392Compareawschmeder
commented
Jun 7, 2026
Part 8: Herestring (<<<) -- manual test procedureAdded in the latest commit: herestring ( Bug that was fixed: both Fix: explicit Test 18: Simple herestring -- treated as one commandsh <<<'echo hello'Expected: ( Test 19: Herestring followed by a second command -- must split into two commandssh <<<'echo hello'echodoneExpected: two separate commands execute: The parser must NOT consume Test 20: Herestring with a single-quoted multiline word -- one commandsh <<<'echo line1echo line2'Expected: the quoted string contains a literal newline; Test 21: Herestring with an ANSI-C quoted multiline word -- one commandsh <<<$'echo line1\necho line2'Expected: |
awschmeder
commented
Jun 7, 2026
Shell Quoting Test ResultsSummaryAll 21 tests PASSED ✓ Part 1: Unquoted newlines (should split)Test 1: Three echo commands on separate linesStatus: PASS ✓ Command: echo a
echo b
echo cExpected Output: Actual Output: Part 2: POSIX single-quoted multi-line argument (should NOT split)Test 2: Newline inside single quotes is part of the argumentStatus: PASS ✓ Command: sh -c 'echo aecho b'Expected Output: Actual Output: Test 3: Operators inside single quotes are literalStatus: PASS ✓ Command: sh -c 'echo hello && echo world'Expected Output: Actual Output: Part 3: ANSI-C quoting ($'...') (should NOT split)Test 4: ANSI-C quoted multi-line argument stays as one sh -c callStatus: PASS ✓ Command: sh -c $'echo a\necho b'Expected Output: Actual Output: Test 5: ANSI-C with escaped apostrophe and embedded newlineStatus: PASS ✓ Command: sh -c $'echo it\'s ok\necho done'Expected Output: syntax error, no output Actual Output: Part 4: Double-quoted multi-line argument (should NOT split)Test 6: Newline inside double quotes is part of the argumentStatus: PASS ✓ Command: sh -c "echo aecho b"Expected Output: Actual Output: Part 5: Heredoc -- unquoted delimiter (should NOT split body)Test 7: Unquoted delimiter heredocStatus: PASS ✓ Command: sh <<EOFecho helloecho worldEOFExpected Output: Actual Output: Test 8: Single-quoted delimiter heredoc (body is literal, no expansion)Status: PASS ✓ Command: sh << 'EOF'echo helloecho worldEOFExpected Output: Actual Output: Test 9: Double-quoted delimiter heredoc (body allows expansion)Status: PASS ✓ Command: sh << "EOF"echo helloecho worldEOFExpected Output: Actual Output: Test 10: <<- heredoc strips leading tabs from body and terminatorStatus: PASS ✓ Command: sh <<-EOF echo hello echo worldEOFExpected Output: Actual Output: Test 11: Multi-line heredoc body -- all body lines execute under one sh callStatus: PASS ✓ Command: sh << 'EOF'echo line1echo line2echo line3EOFExpected Output: Actual Output: Test 12: Command after heredoc terminator runs as a separate commandStatus: PASS ✓ Command: sh <<EOFecho insideEOFecho outsideExpected Output: Actual Output: Test 13: # comment inside heredoc body is preserved as literal textStatus: PASS ✓ Command: sh << 'EOF'# this is a commentecho helloEOFExpected Output: Actual Output: Part 6: Locale quoting ($"...") (should NOT split, $ prefix preserved)Test 14: Locale-quoted string -- no leading $ in outputStatus: PASS ✓ Command: echo$"hello world"Expected Output: Actual Output: Test 15: Operators inside locale-quoted string are not shell operatorsStatus: PASS ✓ Command: sh -c 'echo $"hello && world"'Expected Output: Actual Output: Part 7: Malformed / unterminated quotes (should not execute partial commands)Test 16: Unterminated single quote -- shell should report a syntax errorStatus: PASS ✓ Command: sh -c 'echo helloExpected Output: Syntax error, no partial output Actual Output: Test 17: Unterminated heredoc -- bash executes the body at EOF (no error)Status: PASS ✓ Command: sh <<EOFecho hello(submitted without EOF terminator line) Expected Output: Actual Output: Part 8: Herestring (<<<) -- single-line stdin redirectTest 18: Simple herestring -- treated as one commandStatus: PASS ✓ Command: sh <<<'echo hello'Expected Output: Actual Output: Test 19: Herestring followed by a second commandStatus: PASS ✓ Command: sh <<<'echo hello'echodoneExpected Output: Actual Output: Test 20: Herestring with a single-quoted multiline wordStatus: PASS ✓ Command: sh <<<'echo line1echo line2'Expected Output: Actual Output: Test 21: Herestring with an ANSI-C quoted multiline wordStatus: PASS ✓ Command: sh <<<$'echo line1\necho line2'Expected Output: Actual Output: Final Test Summary
Total: 21/21 PASSED ✓ |
awschmeder
commented
Jun 7, 2026
@coderabbitai review |
✅ Action performedReview finished.
|
edelauna
left a comment
There was a problem hiding this comment.
Seems like a lot was added since last review, some additional comments.
| it("auto-approves an ANSI-C quoted ($'...') multi-line argument when the wrapper prefix is allowed", () => { | ||
| const ansiC = "sh -c $'echo 1\necho 2'" | ||
| expect(getCommandDecision(ansiC, ["sh"])).toBe("auto_approve") | ||
| }) |
There was a problem hiding this comment.
The new suite covers well-formed multi-line commands but does not test the core safety boundary this PR introduces: that an unterminated-quote command is NOT auto-approved even when its prefix is allowlisted. Could we add a case like getCommandDecision("sh -c 'echo a\nrm -rf /", ["sh"]) asserting "ask_user", and one with ["*"]? That would anchor the protection against regression.
There was a problem hiding this comment.
I widened the PR scope to include triggering a toolError on malformed commands so they will no longer execute.
| it("ignores a quote that appears inside a comment", () => { | ||
| expect(findUnterminatedQuote("echo hi # it's fine")).toBeNull() | ||
| }) |
There was a problem hiding this comment.
Is there a test for a properly closed quote followed by a # comment containing an unmatched quote — e.g. sh -c 'cmd' #it's here? The comment-handling path in scanTopLevelQuotes was written for this case but I don't see it exercised in the findUnterminatedQuote suite.
There was a problem hiding this comment.
Added to test coverage.
| if (char === '"') { | ||
| // Double quote: escape-aware, ends at the next unescaped ". | ||
| const start = i |
There was a problem hiding this comment.
Both scanTopLevelQuotes and parseCommandLine implement the shell quoting state machine independently (and a third time in the regex chain below). Any fix to escape handling or a new quote style needs to land in multiple places — is a consolidation being tracked, or is there a reason they need to stay separate?
There was a problem hiding this comment.
Agreed, not DRY... new commit consolidates parsing into one function and improves error detection.
| // Mask quoted strings (single and double) so their contents -- including | ||
| // operators like &&, |, ; and any embedded newlines -- are not treated as | ||
| // command separators. A single left-to-right scan with an alternation is used | ||
| // so that whichever quote opens first wins, preventing a quote of one style |
There was a problem hiding this comment.
Is arrayIndexing ever populated? I can't find anything that pushes into it or emits an __ARRAY_N__ placeholder — if that pattern never appears in the masked string, this restore branch is dead. And if a command literally contained __ARRAY_0__ the substitution would silently return undefined.
There was a problem hiding this comment.
Looks like it was just dead code and never implemented. Latest commit removes it.
There was a problem hiding this comment.
Actually... good point about hthe plaseholder substition - its worse than that as parseCommand will break if the input contains any of the placeholder tokens. The latest commit closes the gap by pre-escaping any token text and de-escaping it after parsing.
…ve dead arrayIndexing
- Unify findUnterminatedQuote and maskTopLevelQuotes under a single
scanTopLevelQuotes state machine -- one pass, no duplicate quoting
logic between the two functions
- Change parseCommand return type from string[] to ParseResult
{ commands: string[]; parseError: UnterminatedQuote | null } so
callers can distinguish a parse error from a normal single-command
result without a separate findUnterminatedQuote call
- getCommandDecision reads parseError from parseCommand instead of
calling findUnterminatedQuote independently; returns the new
malformed_command CommandDecision variant for shell syntax errors
- Remove dead arrayIndexing bucket and __ARRAY_N__ restore (never
populated; caused undefined return when input contained the literal
string __ARRAY_0__)
- Add safety-boundary test: unterminated-quote commands return
malformed_command even when prefix is allowlisted, wildcard, or
the exact command string is on the allowlist
- Add findUnterminatedQuote test: closed quote followed by # comment
with apostrophe returns null (not an open region)
- Update all parseCommand call sites to destructure .commands…rmed command as toolError in ExecuteCommandTool
…ror card for malformed commands
awschmeder
commented
Jun 10, 2026
Addressing @edelauna's review threads: Re: Safety boundary tests Re: Comment + apostrophe test Re: DRY -- duplicated state machines Re: Dead |
awschmeder
commented
Jun 10, 2026
shell-parsing-manual-test-2026-06-10-small2.mp4(re-uploaded with better resolution)... |
Commands containing text like __QUOTE_0__ or __SQUOTE_0__ would be silently corrupted by restorePlaceholders() -- the restore regexes would match the literal tokens and substitute array entries (or 'undefined') in their place. Fix: pre-escape __ -> \x00 in parseCommand before any masking begins, then post-unescape \x00 -> __ across all output commands at the return. \x00 (null byte, U+0000) is safe as a sentinel because the OS terminates command strings at the first \x00, so it can never appear in real shell command text. Adds one regression test covering all eight internal placeholder namespaces.
e3346e5 to
e24e3c0Compareawschmeder
commented
Jun 11, 2026
@coderabbitai review |
✅ Action performedReview finished.
|
edelauna
left a comment
There was a problem hiding this comment.
Thanks for this contribution!
Uh oh!
There was an error while loading. Please reload this page.
…al, and malformed-command error surfacing (#483) * fix(commands): treat multi-line quoted argument as a single command for auto-approval parseCommand split on every newline before any quote handling, so newlines inside a quoted argument (e.g. a multi-line script passed to sh -c) were treated as separate commands. Single-quoted and ANSI-C ($'...') strings were also not fully masked, leaking placeholders and bogus sub-commands. This defeated allowlist auto-approval and produced a noisy command-pattern breakdown in the UI. Mask quoted strings (single, double, ANSI-C) before splitting on unquoted newlines, using a single left-to-right alternation so a quote of one style inside the other does not start a spurious match. Genuine unquoted newlines still split into separate sub-commands. * fix(commands): mask ANSI-C quoted strings in top-level newline split Handle $'...' (ANSI-C) quoting in parseCommand's pre-split masking so an escaped apostrophe inside the quoted body no longer terminates the match early and leak an embedded newline, which would split a single command into bogus sub-commands. Add a regression test covering an escaped apostrophe plus newline inside an ANSI-C argument. * test(commands): cover shell-quote parse-failure fallback restoration Mock shell-quote parse() to throw and assert the fallback path restores the ANSI-C single-quote placeholder, closing the patch-coverage gap on the parse- failure branch. * fix(commands): reject unterminated-quote commands from auto-approval Add findUnterminatedQuote, a quote-aware state-machine scanner that detects a command containing an unclosed quote (a shell syntax error, common in LLM-generated commands with nested quotes). parseCommand now returns such input as a single opaque token instead of splitting on embedded newlines, so a line intended to live inside the unclosed quote cannot surface as an independently auto-approvable sub-command. The scanner returns { quoteType, openIndex } to support a future execution-layer rejection that surfaces a located error to the model; that pre-execution rejection is intentionally deferred to a follow-up. * fix(commands): make top-level quote masking comment-aware Replace the cross-line quote-masking regex in parseCommand() with a state machine (maskTopLevelQuotes) that mirrors findUnterminatedQuote. A quote inside a # comment no longer pairs with a quote on a later line, so a comment can no longer hide a real newline separator and merge two distinct commands. * fix(commands): add heredoc and locale-quote support to command parser; fix pattern extractor - parseCommand: mask heredocs (<<, <<-, all delimiter quoting styles) as single atomic tokens before newline splitting; unterminated heredocs returned as opaque token - parseCommand: add locale-quote ($"...") support alongside existing ANSI-C ($'...') - findUnterminatedQuote: extend QuoteType with "locale" and "heredoc" variants - extractPatternsFromCommand (webview): pre-split via parseCommand before shell-quote tokenization, preventing spurious EOF/body-line/operator tokens in allow/deny selector - Update changeset to cover all three fix areas * fix(commands): handle herestring (<<<) in parse-command and findUnterminatedQuote - Add explicit <<< passthrough in maskTopLevelQuotes: emit all three < chars verbatim and advance i by 3 so the second < does not re-trigger the heredoc branch on the next iteration - Same fix in findUnterminatedQuote for the same root cause - Add herestring test suite covering single-line, multi-command split, and single-quoted/ANSI-C quoted multiline word cases * fix(commands): address CodeRabbit review comments - findUnterminatedQuote: track stripTabs for <<- so tab-stripping only applies when the heredoc opener used <<- (consistent with maskTopLevelQuotes) - findUnterminatedQuote: add doubleIsLocale flag so an unterminated $"..." region returns quoteType "locale" instead of "double" - findUnterminatedQuote: add tests for unterminated locale quote and balanced <<- with indented terminator - CommandExecution: exclude multi-line opaque tokens (heredoc bodies, unterminated quotes) from the raw-command pattern set so body-line words never surface as independently approvable patterns - CommandExecution.spec: strengthen fragment assertions to use per-fragment substring checks, exposing the CommandExecution leak bug * refactor(commands): consolidate quote scanners; add ParseResult; remove dead arrayIndexing - Unify findUnterminatedQuote and maskTopLevelQuotes under a single scanTopLevelQuotes state machine -- one pass, no duplicate quoting logic between the two functions - Change parseCommand return type from string[] to ParseResult { commands: string[]; parseError: UnterminatedQuote | null } so callers can distinguish a parse error from a normal single-command result without a separate findUnterminatedQuote call - getCommandDecision reads parseError from parseCommand instead of calling findUnterminatedQuote independently; returns the new malformed_command CommandDecision variant for shell syntax errors - Remove dead arrayIndexing bucket and __ARRAY_N__ restore (never populated; caused undefined return when input contained the literal string __ARRAY_0__) - Add safety-boundary test: unterminated-quote commands return malformed_command even when prefix is allowlisted, wildcard, or the exact command string is on the allowlist - Add findUnterminatedQuote test: closed quote followed by # comment with apostrophe returns null (not an open region) - Update all parseCommand call sites to destructure .commands * feat(commands): add message field to UnterminatedQuote; surface malformed command as toolError in ExecuteCommandTool * feat(commands): add error status to CommandExecutionStatus; render error card for malformed commands * i18n: add malformedCommand translation to all 17 non-English locales * fix: guard parseCommand against literal placeholder token collisions Commands containing text like __QUOTE_0__ or __SQUOTE_0__ would be silently corrupted by restorePlaceholders() -- the restore regexes would match the literal tokens and substitute array entries (or 'undefined') in their place. Fix: pre-escape __ -> \x00 in parseCommand before any masking begins, then post-unescape \x00 -> __ across all output commands at the return. \x00 (null byte, U+0000) is safe as a sentinel because the OS terminates command strings at the first \x00, so it can never appear in real shell command text. Adds one regression test covering all eight internal placeholder namespaces.
…al, and malformed-command error surfacing (Zoo-Code-Org#483) * fix(commands): treat multi-line quoted argument as a single command for auto-approval parseCommand split on every newline before any quote handling, so newlines inside a quoted argument (e.g. a multi-line script passed to sh -c) were treated as separate commands. Single-quoted and ANSI-C ($'...') strings were also not fully masked, leaking placeholders and bogus sub-commands. This defeated allowlist auto-approval and produced a noisy command-pattern breakdown in the UI. Mask quoted strings (single, double, ANSI-C) before splitting on unquoted newlines, using a single left-to-right alternation so a quote of one style inside the other does not start a spurious match. Genuine unquoted newlines still split into separate sub-commands. * fix(commands): mask ANSI-C quoted strings in top-level newline split Handle $'...' (ANSI-C) quoting in parseCommand's pre-split masking so an escaped apostrophe inside the quoted body no longer terminates the match early and leak an embedded newline, which would split a single command into bogus sub-commands. Add a regression test covering an escaped apostrophe plus newline inside an ANSI-C argument. * test(commands): cover shell-quote parse-failure fallback restoration Mock shell-quote parse() to throw and assert the fallback path restores the ANSI-C single-quote placeholder, closing the patch-coverage gap on the parse- failure branch. * fix(commands): reject unterminated-quote commands from auto-approval Add findUnterminatedQuote, a quote-aware state-machine scanner that detects a command containing an unclosed quote (a shell syntax error, common in LLM-generated commands with nested quotes). parseCommand now returns such input as a single opaque token instead of splitting on embedded newlines, so a line intended to live inside the unclosed quote cannot surface as an independently auto-approvable sub-command. The scanner returns { quoteType, openIndex } to support a future execution-layer rejection that surfaces a located error to the model; that pre-execution rejection is intentionally deferred to a follow-up. * fix(commands): make top-level quote masking comment-aware Replace the cross-line quote-masking regex in parseCommand() with a state machine (maskTopLevelQuotes) that mirrors findUnterminatedQuote. A quote inside a # comment no longer pairs with a quote on a later line, so a comment can no longer hide a real newline separator and merge two distinct commands. * fix(commands): add heredoc and locale-quote support to command parser; fix pattern extractor - parseCommand: mask heredocs (<<, <<-, all delimiter quoting styles) as single atomic tokens before newline splitting; unterminated heredocs returned as opaque token - parseCommand: add locale-quote ($"...") support alongside existing ANSI-C ($'...') - findUnterminatedQuote: extend QuoteType with "locale" and "heredoc" variants - extractPatternsFromCommand (webview): pre-split via parseCommand before shell-quote tokenization, preventing spurious EOF/body-line/operator tokens in allow/deny selector - Update changeset to cover all three fix areas * fix(commands): handle herestring (<<<) in parse-command and findUnterminatedQuote - Add explicit <<< passthrough in maskTopLevelQuotes: emit all three < chars verbatim and advance i by 3 so the second < does not re-trigger the heredoc branch on the next iteration - Same fix in findUnterminatedQuote for the same root cause - Add herestring test suite covering single-line, multi-command split, and single-quoted/ANSI-C quoted multiline word cases * fix(commands): address CodeRabbit review comments - findUnterminatedQuote: track stripTabs for <<- so tab-stripping only applies when the heredoc opener used <<- (consistent with maskTopLevelQuotes) - findUnterminatedQuote: add doubleIsLocale flag so an unterminated $"..." region returns quoteType "locale" instead of "double" - findUnterminatedQuote: add tests for unterminated locale quote and balanced <<- with indented terminator - CommandExecution: exclude multi-line opaque tokens (heredoc bodies, unterminated quotes) from the raw-command pattern set so body-line words never surface as independently approvable patterns - CommandExecution.spec: strengthen fragment assertions to use per-fragment substring checks, exposing the CommandExecution leak bug * refactor(commands): consolidate quote scanners; add ParseResult; remove dead arrayIndexing - Unify findUnterminatedQuote and maskTopLevelQuotes under a single scanTopLevelQuotes state machine -- one pass, no duplicate quoting logic between the two functions - Change parseCommand return type from string[] to ParseResult { commands: string[]; parseError: UnterminatedQuote | null } so callers can distinguish a parse error from a normal single-command result without a separate findUnterminatedQuote call - getCommandDecision reads parseError from parseCommand instead of calling findUnterminatedQuote independently; returns the new malformed_command CommandDecision variant for shell syntax errors - Remove dead arrayIndexing bucket and __ARRAY_N__ restore (never populated; caused undefined return when input contained the literal string __ARRAY_0__) - Add safety-boundary test: unterminated-quote commands return malformed_command even when prefix is allowlisted, wildcard, or the exact command string is on the allowlist - Add findUnterminatedQuote test: closed quote followed by # comment with apostrophe returns null (not an open region) - Update all parseCommand call sites to destructure .commands * feat(commands): add message field to UnterminatedQuote; surface malformed command as toolError in ExecuteCommandTool * feat(commands): add error status to CommandExecutionStatus; render error card for malformed commands * i18n: add malformedCommand translation to all 17 non-English locales * fix: guard parseCommand against literal placeholder token collisions Commands containing text like __QUOTE_0__ or __SQUOTE_0__ would be silently corrupted by restorePlaceholders() -- the restore regexes would match the literal tokens and substitute array entries (or 'undefined') in their place. Fix: pre-escape __ -> \x00 in parseCommand before any masking begins, then post-unescape \x00 -> __ across all output commands at the return. \x00 (null byte, U+0000) is safe as a sentinel because the OS terminates command strings at the first \x00, so it can never appear in real shell command text. Adds one regression test covering all eight internal placeholder namespaces.
Related GitHub Issue
Closes: #484
Description
A single command that wraps a multi-line script in a quoted argument (e.g.
sh -c '...') was misclassified as a multi-statement sequence, which defeated allowlist auto-approval and produced a noisy command-pattern breakdown in the UI. Malformed commands (unterminated quotes) were silently passed to the terminal, risking partial execution of the well-formed prefix before the shell aborted.Root causes fixed:
parseCommandsplit on every newline before any quote handling, so newlines inside a quoted argument were treated as command separators. Each script line became a bogus sub-command. Because those fragments matched neither the allowlist nor the denylist,getCommandDecisionfell through toask_usereven when the wrapper command (e.g.sh -c) was allowlisted.$'...') leaked an internal placeholder (SQUOTE_0__) becauseshell-quoteparsed$__SQUOTE_0__as a variable expansion and corrupted the marker.How (key implementation details):
In
src/shared/parse-command.ts:$'...') at the top ofparseCommand()before splitting on newlines, so newlines embedded in a quoted argument stay within their command. Genuine unquoted newlines still split into separate sub-commands, preserving per-statement safety checks.scanTopLevelQuotesstate machine (left-to-right alternation, escape-aware,#-comment-aware). BothfindUnterminatedQuoteandmaskTopLevelQuotesdelegate to this one scanner -- eliminating the duplicated state machines that existed in the initial implementation.parseCommandLineremains separate: it operates on already-masked single lines and usesshell-quotefor operator tokenization, a different concern.findUnterminatedQuote()returns{ quoteType, openIndex, message } | null.parseCommand()returns any input with an unterminated quote as a single opaque token, so a fragment can never be auto-approved in isolation.arrayIndexing/__ARRAY_N__restore branch -- it was never populated and silently returnedundefinedon any match.${...}patterns are handled by theparameterExpansionsbucket, which is populated and restored correctly.In
src/core/auto-approval/andsrc/tools/execute-command/:getCommandDecisionreturns a dedicated"malformed_command"result (distinct from"ask_user") whenfindUnterminatedQuotefires, anchoring the safety boundary against regression.ExecuteCommandToolsurfaces a malformed-command as atoolErrorwith a human-readable message locating the unterminated quote, so the agent receives structured feedback instead of a silent shell syntax error.In
webview-ui/:CommandExecutionStatusgains anerrorstate andCommandExecutionrenders a visible error indicator (e.g. the red dot) for malformed commands, replacing the silent no-op the UI previously showed.A single fix at the shared
parseCommandsource corrects both the backend auto-approval decision and the webview pattern breakdown, since both consume this function.Reviewers should note: plain POSIX single-quote handling is intentionally not escape-aware (POSIX single quotes treat backslash literally and always terminate on an apostrophe); only the ANSI-C
$'...'form is escape-aware. ThefindUnterminatedQuotereturn type carriesquoteType,openIndex, andmessageto support located malformed-command error messages.Roadmap alignment: Reliability First -- ensures command execution and auto-approval are consistently reliable for wrapped multi-line scripts, and provides clear, actionable feedback for malformed input rather than silent partial execution.
Test Procedure
Automated (all affected suites pass):
cd src && npx vitest run shared/__tests__/parse-command.spec.ts core/auto-approval/__tests__/commands.spec.tscd webview-ui && npx vitest run src/components/chat/__tests__/CommandExecution.spec.tsxCoverage added:
src/shared/__tests__/parse-command.spec.ts: single/double quoted multi-line payloads, escaped inner quotes, mixed quote styles, ANSI-C$'...'(no placeholder leak), ANSI-C escaped-apostrophe + newline, shell-quote parse-failure fallback restoration, newline preservation, genuine multi-statement splitting, malformed unterminated-quote safe path (single/double/ANSI-C and an embedded-newline case), fullfindUnterminatedQuotesuite (balanced input, each unterminated style with reported index, backslash-escaped quotes outside strings,#comments, mixed quote styles, ANSI-C/double-quote escape handling, apostrophe inside a#comment after a closed quoted argument).src/core/auto-approval/__tests__/commands.spec.ts: wrapped multi-line scripts auto-approve when the wrapper prefix is allowlisted; genuine multi-statement scripts still require every statement to be allowlisted; unterminated-quote commands return"malformed_command"regardless of allowlist (including["*"]).webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx: pattern selector no longer surfaces stray script-line fragments; error card renders for malformed commands.Manual: verified in the Extension Development Host that a well-formed
sh -c '...'multi-line wrapper auto-approves, a malformed unterminated-quote command shows an error card and atoolError(no standalone fragment, no silent partial execution), and genuine multi-statement input still requires each statement allowlisted.Pre-Submission Checklist
Screenshots / Videos
A video of the manual test confirmation is provided in the attached comments.
Documentation Updates
Additional Notes
The fix is a single change at the shared
parseCommandsource, so both the backend auto-approval decision and the webview pattern breakdown are corrected together. The malformed-command error card andtoolErrorsurfacing are additive: they give the agent and user explicit, located feedback instead of a silent shell abort.