Skip to content

fix: 避免 && 短路时误用未执行命令的退出码语义 #1347

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ai-yang wants to merge 1 commit into
base: main
Choose a base branch
from
+43 −1
Open

fix: 避免 && 短路时误用未执行命令的退出码语义 #1347

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions packages/builtin-tools/src/tools/BashTool/__tests__/commandSemantics.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,34 @@ describe('interpretCommandResult', () => {
expect(result.message).toBe('No matches found')
})

test('does not use an unexecuted rg command for && short-circuit semantics', () => {
const result = interpretCommandResult(
'cd /definitely-missing && rg pattern .',
1,
'',
'cd: /definitely-missing: No such file or directory',
)
expect(result.isError).toBe(true)
expect(result.message).toBe('Command failed with exit code 1')
})

test('does not use an unexecuted diff command for && short-circuit semantics', () => {
const result = interpretCommandResult(
"node -e 'process.exit(1)' && diff a.txt b.txt",
1,
'',
'',
)
expect(result.isError).toBe(true)
expect(result.message).toBe('Command failed with exit code 1')
})

test('keeps fallback semantics for a command reached through ||', () => {
const result = interpretCommandResult('false || rg pattern .', 1, '', '')
expect(result.isError).toBe(false)
expect(result.message).toBe('No matches found')
})

// ─── rg (ripgrep) semantics ──────────────────────────────────────
test('rg exit 1 means no matches (not error)', () => {
const result = interpretCommandResult('rg pattern', 1, '', '')
Expand Down
16 changes: 15 additions & 1 deletion packages/builtin-tools/src/tools/BashTool/commandSemantics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@
* For example, grep returns 1 when no matches are found, which is not an error condition.
*/

import { splitCommand_DEPRECATED } from 'src/utils/bash/commands.js'
import {
splitCommand_DEPRECATED,
splitCommandWithOperators,
} from 'src/utils/bash/commands.js'

export type CommandSemantic = (
exitCode: number,
Expand Down Expand Up @@ -110,6 +113,17 @@ function extractBaseCommand(command: string): string {
* May get it super wrong - don't depend on this for security
*/
function heuristicallyExtractBaseCommand(command: string): string {
// With an && list, the syntactically last command may never have run. For
// example, `cd /missing && rg needle .` exits 1 from `cd`, but applying rg's
// "1 = no matches" semantics would turn that real failure into success.
// The executor currently only exposes the aggregate exit code, so fall back
// to the conservative default whenever an AND-list makes the executed
// command ambiguous. Pipes remain safe: without pipefail their last command
// determines the aggregate status and is always executed.
if (splitCommandWithOperators(command).includes('&&')) {
return ''
}
Comment on lines +123 to +125

@coderabbitai coderabbitai Bot Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.ts

Repository: 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 500

Repository: 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/bash

Repository: 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.ts

Repository: 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:

🏁 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 300

Repository: 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.


const segments = splitCommand_DEPRECATED(command)

// Take the last command as that's what determines the exit code
Expand Down