fix: 避免 && 短路时误用未执行命令的退出码语义 - #1347
Conversation
📝 WalkthroughWalkthroughThe BashTool now uses conservative default exit-code semantics for ambiguous ChangesBash command semantics
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There 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 `@packages/builtin-tools/src/tools/BashTool/commandSemantics.ts`:
- Around line 123-125: Update command-semantics selection around
splitCommandWithOperators and splitCommand_DEPRECATED so parse-fallback results
are detected before choosing a command. Propagate parse status or an equivalent
quote-aware ambiguity signal, ensuring fallback commands containing operators
such as && are not classified by their first token; preserve correct exit-code
failure handling and add a regression test for the rg ${var + expr} && echo
case.
🪄 Autofix
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: ce43b9a4-6384-459b-adc2-0ed4a79dadb3
📒 Files selected for processing (2)
packages/builtin-tools/src/tools/BashTool/__tests__/commandSemantics.test.tspackages/builtin-tools/src/tools/BashTool/commandSemantics.ts
| if (splitCommandWithOperators(command).includes('&&')) { | ||
| return '' | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A25 -B8 \
'parseResult\.success|return \[commandOriginalJoined\]' \
src/utils/bash/commands.ts
rg -n -A30 -B8 \
'heuristicallyExtractBaseCommand|splitCommand_DEPRECATED' \
packages/builtin-tools/src/tools/BashTool/commandSemantics.tsRepository: claude-code-best/claude-code
Length of output: 11736
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- command splitter implementation ---'
sed -n '80,255p' src/utils/bash/commands.ts
printf '%s\n' '--- semantic tests and call sites ---'
rg -n -C 8 \
'interpretCommandResult|heuristicallyExtractBaseCommand|splitCommandWithOperators|splitCommand_DEPRECATED' \
packages/builtin-tools/src src --glob '*.{ts,tsx}' \
| head -n 500
printf '%s\n' '--- parser dependency and relevant test files ---'
rg -n -C 5 \
'splitCommandWithOperators|parse failed|malformed|Bad substitution|&&' \
src packages/builtin-tools --glob '*.{test,spec}.{ts,tsx}' --glob '*.ts' \
| head -n 500Repository: claude-code-best/claude-code
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- legacy splitter ---'
sed -n '251,365p' src/utils/bash/commands.ts
printf '%s\n' '--- command semantics files and tests ---'
fd -i 'commandSemantics' packages/builtin-tools/src
fd -i 'test' packages/builtin-tools/src/tools/BashTool | rg 'Semantics|command|BashTool'
printf '%s\n' '--- direct semantic references ---'
rg -n -C 12 \
'interpretCommandResult|COMMAND_SEMANTICS|splitCommandWithOperators' \
packages/builtin-tools/src/tools/BashTool/commandSemantics.ts \
packages/builtin-tools/src/tools/BashTool --glob '*.{test,spec}.{ts,tsx}'
printf '%s\n' '--- shell parser implementation ---'
fd -i 'shellQuote' src
rg -n -C 12 \
'function tryParseShellCommand|export function tryParseShellCommand|Bad substitution' \
src/utils/bashRepository: claude-code-best/claude-code
Length of output: 31607
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- remainder of legacy splitter ---'
sed -n '340,410p' src/utils/bash/commands.ts
printf '%s\n' '--- complete Bash command semantics tests ---'
cat -n packages/builtin-tools/src/tools/BashTool/__tests__/commandSemantics.test.ts
printf '%s\n' '--- package metadata for shell-quote ---'
rg -n -C 3 '"shell-quote"|packageManager|bun' package.json bun.lock package-lock.json yarn.lock pnpm-lock.yaml 2>/dev/null | head -n 120 || true
printf '%s\n' '--- parser result type and error handling ---'
sed -n '1,75p' src/utils/bash/shellQuote.tsRepository: claude-code-best/claude-code
Length of output: 17098
🌐 Web query:
shell-quote 1.8.3 parse "Bad substitution" ${var + expr}
💡 Result:
The "Bad substitution" error in the shell-quote library—including version 1.8.3 and others—occurs because the library's internal tokenizer is not a full-featured Bash interpreter [1][2][3]. The library only supports simple parameter expansion in the format of $VARNAME or ${VARNAME} [2][4][5]. It does not support complex Bash features such as: * Arithmetic expansion (e.g., $((...))) [1][5] * Command substitution (e.g., $(...)) [1][5] * Parameter expansion with modifiers (e.g., ${var:-default}, ${var/a/b}) [1][5] * Array indexing (e.g., ${arr[i]}) [1][6] The specific error "Bad substitution" is triggered when the parser encounters whitespace inside a ${...} block or other complex syntax it cannot resolve [1][2][5]. Because the library uses a simplified, regex-based tokenizer, expressions like ${var + expr} contain characters (such as spaces or operators) that the parser does not recognize as valid for a simple variable name, leading it to throw an error rather than ignoring or evaluating the expression [1][2][7]. If your application requires evaluation of complex Bash syntax, the recommended approach is to avoid using shell-quote for parsing such commands and instead execute the commands directly via a shell (e.g., using Node.js child_process.execFile or spawn) [1]. If you must use the library for validation or sanitization, common workarounds in existing projects include catching the "Bad substitution" error and treating the entire command string as an opaque block, or skipping the parser for complex commands [8][7].
Citations:
- 1: shell-quote fails to parse Bash array indexing with nested parameter expansion ljharb/shell-quote#19
- 2: https://github.com/ljharb/shell-quote/
- 3: https://github.laiyagushi.com/ljharb/shell-quote
- 4: https://www.npmjs.com/package/shell-quote
- 5: https://libraries.io/npm/shell-quote
- 6: Command validation fails with 'Bad substitution' error on shell array indexing RooCodeInc/Roo-Code#3529
- 7: fix(bash): share parser analysis across checks Gitlawb/openclaude#1735
- 8: https://github.com/wk42worldworld/cybercode/blob/main/src/utils/bash/commands.ts
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
commands = Path("src/utils/bash/commands.ts").read_text()
semantics = Path(
"packages/builtin-tools/src/tools/BashTool/commandSemantics.ts"
).read_text()
# Model only the documented parse-failure branches and the exact operations
# shown in the reviewed source. This does not execute repository code.
parse_failed = "rg ${var + expr} && echo"
fallback = [parse_failed] # splitCommandWithOperators on !parseResult.success
operator_present = "&&" in fallback
legacy_segments = [part for part in fallback if part not in {
"&&", "||", ";", "|", "&", ">", ">>", ">&"
}]
base_command = legacy_segments[-1].strip().split()[0]
semantic = "rg" if base_command == "rg" else "default"
print({
"source_has_parse_failure_fallback": "return [commandOriginalJoined]" in commands,
"source_checks_exact_operator_token": ".includes('&&')" in semantics,
"fallback_result": fallback,
"exact_operator_detected": operator_present,
"legacy_segments": legacy_segments,
"base_command": base_command,
"selected_semantic": semantic,
"exit_1_is_error": semantic == "default",
})
PY
printf '%s\n' '--- splitter tests for quoted and escaped operators ---'
rg -n -C 8 \
'splitCommandWithOperators|escaped.*&&|quoted.*&&|&&.*quoted|&&.*escaped' \
src packages --glob '*.{test,spec}.{ts,tsx}' | head -n 300Repository: claude-code-best/claude-code
Length of output: 6641
Handle parse-fallback commands before selecting command semantics.
On parse failure, splitCommandWithOperators returns the complete command as one element. For rg ${var + expr} && echo, the && check misses the operator, and splitCommand_DEPRECATED selects rg. Exit code 1 is then treated as “No matches found” instead of a command failure. Propagate parse status or a quote-aware ambiguity signal, and add a regression test.
🤖 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 `@packages/builtin-tools/src/tools/BashTool/commandSemantics.ts` around lines
123 - 125, Update command-semantics selection around splitCommandWithOperators
and splitCommand_DEPRECATED so parse-fallback results are detected before
choosing a command. Propagate parse status or an equivalent quote-aware
ambiguity signal, ensuring fallback commands containing operators such as && are
not classified by their first token; preserve correct exit-code failure handling
and add a regression test for the rg ${var + expr} && echo case.
1 participant
Summary
&&list。rg、管道和纯||的既有特殊语义。rg/diff短路回归及false || rg控制组。这样可以避免
test-command && rg ...中测试失败时，把根本未执行的rg当成“无匹配”。Test plan
bun test packages/builtin-tools/src/tools/BashTool/__tests__/commandSemantics.test.ts（14 pass）bun run typecheckgit diff --check关联 issue
Closes #1344Summary by CodeRabbit
&&and||.||.