Conversation
github-actions
Bot
added
the
maintainer:e2e:ok
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe change adds PowerShell parsing and shell-aware command authorization. It supports recursive validation of wrappers, evaluators, substitutions, and dynamic targets. It also adds runtime compatibility checks, dependency resolution tests, integration coverage, and documentation. ChangesPowerShell shell parsing and authorization
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Contributor
WalkthroughBefore this PR, PowerShell commands were still flowing through bash-oriented or generic shell validation paths, so PowerShell-specific syntax, wrapper forms, and AST-level security issues could be missed or misclassified. After this PR, the core shell-validation path is shell-aware: it resolves the active shell type, loads PowerShell grammar when needed, extracts PowerShell AST details, handles PowerShell wrapper payloads, and applies the correct allowlisting and error diagnostics. The CLI prompt-processing layer also threads the resolved shell type through so permission checks use the right grammar instead of assuming bash-style parsing. Release NotesNew Features
Bug Fixes
Tests
Documentation
Refactor
Chore
Changes
Sequence DiagramsequenceDiagram
participant U as User/CLI
participant SP as ShellProcessor
participant SU as ShellUtils
participant P as ShellParser
participant PA as PowerShellAST
participant TU as ToolUtils
participant A as CoreShellToolHostAdapter
U->>SP: Invoke command with shell injection
SP->>SP: Extract injections (!{...})
SP->>SP: Resolve commands with shell escaping
SP->>SU: checkCommandPermissions(command, config, allowlist)
SU->>SU: Resolve shell type (bash/cmd/powershell)
SU->>P: parseShellCommand(command)
P->>P: Initialize tree-sitter parser with bash grammar
alt PowerShell command
P->>PA: Parse with PowerShell grammar
PA->>PA: Validate AST for security issues
PA-->>P: Return parsed AST
end
P-->>SU: Return command details
SU->>TU: isShellInvocationAllowlisted(command, allowedTools)
TU->>TU: Split commands and check patterns
TU-->>SU: Return allowlist decision
SU-->>SP: Return permission result
alt Allowed
SP->>A: executeShellCommand(resolvedCommand)
A->>A: Execute via ShellExecutionService
A-->>SP: Return execution result
SP-->>U: Return processed prompt with output
else Denied
SP-->>U: Throw permission error
end
Magnitude🎯 3 (L) RelatedNo related items found. Pre-merge Checks
Walkthrough generated by LLxprt PR Review. Planner issue: #2256 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (6)
scripts/tests/issue-3181-pwsh-resolution.bun.test.ts (2)
141-175: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueIsolate the negative control from the ambient environment.
The test proves that resolution fails when the package is absent. Node also consults
NODE_PATHand everynode_modulesdirectory on the path from the temp directory to the filesystem root. An inheritedNODE_PATHthat contains the workspace would make the resolution succeed and the negative control fail.Pass an explicit
envthat clearsNODE_PATHto theexecFileSynccall.💚 Proposed change
const stdout = execFileSync('node', [scriptPath], { encoding: 'utf8', timeout: 10_000, + env: { ...process.env, NODE_PATH: '' }, });🤖 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 `@scripts/tests/issue-3181-pwsh-resolution.bun.test.ts` around lines 141 - 175, Update the execFileSync call in the “resolution fails when tree-sitter-pwsh is absent” test to pass an explicit environment with NODE_PATH cleared, while preserving the existing process environment needed by the test. Keep the negative-control setup and assertion unchanged.
44-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the module-level resolution into the tests.
Lines 49 and 58 resolve
tree-sitter-pwshat import time. If the package is missing or the workspace link is broken, the import throws and every test in this file reports a raw module-resolution error. The failure then does not identify the missing dependency as the cause.Resolve inside
beforeAllor inside the test that needs the path, and assert a clear message.🤖 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 `@scripts/tests/issue-3181-pwsh-resolution.bun.test.ts` around lines 44 - 63, Move the module-level tree-sitter-pwsh resolution using localRequire.resolve for realWasmPath and realPwshPackageJsonPath into beforeAll or the specific test that needs them. Catch resolution failures and assert or report a clear missing-dependency message so the test fails diagnostically instead of during module import.packages/core/src/utils/powershell-ast.ts (2)
228-233: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider a depth cap for nested wrapper expansion.
Termination relies on the strict-substring invariant at Line 736. That invariant bounds depth by command length, so a deeply nested payload such as repeated
iex '...'causes one full parse per level. The cost grows quadratically with command length. A small explicit depth limit that returnsexpressionDetailwhen exceeded would make the bound independent of input length and keep the fail-closed result.🤖 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/core/src/utils/powershell-ast.ts` around lines 228 - 233, Cap recursive nested wrapper expansion in extractPwshWrapperPayloadDetails with a small explicit depth limit; when the limit is exceeded, return expressionDetail instead of parsing another payload. Thread the depth through the current wrapper/evaluator expansion path while preserving existing literal parsing and unresolved-expression behavior below the cap.
430-455: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winFail closed on unrecognized string children. The current grammar handles
"$env:x"and"$(cmd)"throughexpandable_string_literal, so both returnnullbefore the fallback. Returnnullwhen no recognized child matches instead of decodingstringNode.text, to prevent future interpolation node types from being treated as static text.🤖 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/core/src/utils/powershell-ast.ts` around lines 430 - 455, The extractPwshStaticStringContent function should fail closed when none of its named children match the recognized static string node types. Replace the final fallback that decodes stringNode.text with a null return, while preserving the existing handling for verbatim and non-interpolated expandable string children.scripts/bun-native-modules-smoke.ts (1)
171-200: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd deterministic cleanup for the web-tree-sitter objects. Call
tree.delete()andparser.delete()in afinallyblock. The existingcreateRequire(import.meta.url)correctly supportsrequire.resolve.🤖 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 `@scripts/bun-native-modules-smoke.ts` around lines 171 - 200, The checkTreeSitterPowerShell function must deterministically release the parser and parsed tree resources. Declare parser and tree outside the try block as needed, then call tree.delete() when a tree was created and parser.delete() in a finally block, ensuring cleanup occurs on both success and failure while preserving the existing validation and error reporting.packages/core/src/utils/shell-parser.ts (1)
364-369: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReset the parser after parse exceptions.
web-tree-sitterdocuments that a parser resumes after callback failures untilreset()is called. Reset the shared parser before returning from the catch path.🤖 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/core/src/utils/shell-parser.ts` around lines 364 - 369, Update the parse exception catch path to call reset() on the shared web-tree-sitter parser before returning null. Keep the existing conditional debug logging unchanged and ensure the reset occurs for every parse failure.
🤖 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 `@docs/shell-replacement.md`:
- Line 81: Update the literal call-target example in the documentation to use
escaped backslashes, so the Windows path renders as C:\tools\tool.exe rather
than containing tab characters; leave the surrounding normalization explanation
unchanged.
In `@packages/core/src/utils/powershell-ast.ts`:
- Around line 626-657: Update wrapper flag detection in findFlagElementIndex and
extractPwshBareWrapperPayload to accept unambiguous PowerShell parameter
prefixes, so forms such as -Comm and -Com resolve to -Command while preserving
exact handling for cmd and bash flags. Add -EncodedCommand handling only if its
base64 payload can be decoded safely; otherwise document the unsupported
recursive decoding limitation in docs/shell-replacement.md.
- Around line 929-938: Update findFirstErrorNode to detect missing nodes via
root.isMissing instead of comparing root.type to 'MISSING', while retaining the
existing ERROR-type check and descendant traversal behavior.
- Around line 271-281: Update the command extraction logic near the existing
command_name handling to also recognize path_command_name nodes. Normalize the
path-qualified command name with normalizePwshCommandName and construct
canonicalText so allowlist and blocklist checks use the canonical executable
name rather than the raw path.
- Around line 549-571: Update classifyPwshLauncherArgument to handle
concatenated_command_argument nodes by extracting their string_literal child
with extractPwshStaticStringContent; classify statically extracted content as a
normalized static command detail, and return dynamicDetail for interpolated
arguments so launcher targets remain in blocklist and allowlist checks.
In `@packages/core/src/utils/powershell-parse-result.ts`:
- Line 38: Update the errorReason message in the PowerShell parse-result
handling to name the declared parser dependency, tree-sitter-pwsh, instead of
powershell-tree-sitter; preserve the existing syntax-error context and position
details.
In `@packages/core/src/utils/shell-parser-node-smoke.test.ts`:
- Around line 124-158: Move the JSON parsing and result assertions out of the
try/catch surrounding execFileSync, leaving that catch responsible only for
process-spawn or execution failures. Preserve the existing initOk,
bashAvailable, and pwshAvailable expectations after successful execution, while
retaining the current spawn-error diagnostics in the catch.
In `@packages/core/src/utils/shell-parser-pwsh.test.ts`:
- Around line 19-26: Use isParserAvailable('powershell') rather than
initializeParser() as the PowerShell availability guard. In
packages/core/src/utils/shell-parser-pwsh.test.ts#L19-L26, set parserReady after
awaiting initialization, remove the throwing beforeAll, and reuse the flag at
lines 319, 372, and 489; update
packages/core/src/utils/shell-utils.powershell.test.ts#L26-L26 and
packages/core/src/utils/shell-utils.powershell-wrappers.test.ts#L24-L24 to
import and use the same guard. In
packages/core/src/utils/shell-utils.pwshUnavailable.test.ts#L134-L160, guard the
parser reset/init lifecycle consistency suite with PowerShell availability or
assert only Bash availability. In
packages/core/src/tools-adapters/CoreShellToolHostAdapter.test.ts#L113-L116,
import and capture the availability flag, then include it in the Windows-only
suite condition at line 227.
In `@packages/core/src/utils/shell-parser.ts`:
- Around line 500-507: Correct the `canonicalText` JSDoc example so the Windows
path uses the literal escaped `\tools` sequence rather than a tab character,
matching the corresponding `shell-parser-pwsh` test example. Change only the
documentation example.
- Around line 1128-1142: Update parsePwshCommandDetails and the recursive
payload-parser flow passed to buildPwshCommandParseResult to track recursion
depth and enforce a constant maximum of at least 16 levels. When the limit is
exceeded, return an unresolved/null parse result so permission checks remain
fail-closed, while preserving existing parsing behavior within the allowed depth
and the per-call timeout.
In `@packages/core/src/utils/shell-utils.powershell-wrappers.test.ts`:
- Around line 499-502: Update the explanatory comment in the test case around
isCommandAllowed to match the command under test: describe powershell -Command
"rm ""-rf"" /tmp" decoding to rm "-rf" /tmp. Do not change the test command or
assertion.
In `@packages/core/src/utils/shell-utils.powershell.test.ts`:
- Around line 349-358: Remove the stale comment claiming the current PowerShell
implementation returns false in the malformed-tree test around
detectCommandSubstitution. Rename the describe block currently labeled
“PowerShell parser unavailable” to reflect that it verifies multiline PowerShell
does not use the Bash diagnostic.
In `@packages/core/src/utils/shell-utils.test.ts`:
- Around line 542-562: PowerShell integration suites are gated by generic parser
initialization instead of PowerShell grammar availability. In
packages/core/src/utils/shell-utils.test.ts#542-562, define and use a shared
pwshAvailable constant from isParserAvailable('powershell') to replace the
parserInitialized gate; in packages/core/src/utils/shell-utils.test.ts#651-731,
gate the isShellInvocationAllowlisted: PowerShell shell-aware suite with the
same pwshAvailable condition.
In `@packages/core/src/utils/shell-utils.ts`:
- Around line 183-192: Update the documentation and behavior contract for
splitCommands, getCommandRoots, and detectCommandSubstitution to match their
actual omitted-argument default: Bash. Change the comments from “platform shell”
to “bash” unless intentionally routing these functions through resolveShellType;
preserve checkCommandPermissions’ existing platform-shell default.
- Around line 841-857: Update the parser-result handling for the `all` and
`none` branches so empty or unresolved normalized command lists produce a hard
denial via `getStrictAllowlistDenial`; do not allow `.filter(Boolean)` to return
an empty list. Ensure dynamic targets such as `& $cmd` and empty targets such as
`& ''` remain denied, while preserving recursive parsed-command checks for
resolvable commands and the existing fallback behavior.
In `@packages/core/src/utils/tool-utils.ts`:
- Around line 179-220: Update the resolveAllowlistCommands documentation to
describe the actual fail-closed behavior: dynamic or expression details are
normalized from detail.canonicalText ?? detail.text and may retain non-empty
command text, but fail closed because that text does not match any specific
allowlist pattern. Do not claim they are represented by empty-name details.
---
Nitpick comments:
In `@packages/core/src/utils/powershell-ast.ts`:
- Around line 228-233: Cap recursive nested wrapper expansion in
extractPwshWrapperPayloadDetails with a small explicit depth limit; when the
limit is exceeded, return expressionDetail instead of parsing another payload.
Thread the depth through the current wrapper/evaluator expansion path while
preserving existing literal parsing and unresolved-expression behavior below the
cap.
- Around line 430-455: The extractPwshStaticStringContent function should fail
closed when none of its named children match the recognized static string node
types. Replace the final fallback that decodes stringNode.text with a null
return, while preserving the existing handling for verbatim and non-interpolated
expandable string children.
In `@packages/core/src/utils/shell-parser.ts`:
- Around line 364-369: Update the parse exception catch path to call reset() on
the shared web-tree-sitter parser before returning null. Keep the existing
conditional debug logging unchanged and ensure the reset occurs for every parse
failure.
In `@scripts/bun-native-modules-smoke.ts`:
- Around line 171-200: The checkTreeSitterPowerShell function must
deterministically release the parser and parsed tree resources. Declare parser
and tree outside the try block as needed, then call tree.delete() when a tree
was created and parser.delete() in a finally block, ensuring cleanup occurs on
both success and failure while preserving the existing validation and error
reporting.
In `@scripts/tests/issue-3181-pwsh-resolution.bun.test.ts`:
- Around line 141-175: Update the execFileSync call in the “resolution fails
when tree-sitter-pwsh is absent” test to pass an explicit environment with
NODE_PATH cleared, while preserving the existing process environment needed by
the test. Keep the negative-control setup and assertion unchanged.
- Around line 44-63: Move the module-level tree-sitter-pwsh resolution using
localRequire.resolve for realWasmPath and realPwshPackageJsonPath into beforeAll
or the specific test that needs them. Catch resolution failures and assert or
report a clear missing-dependency message so the test fails diagnostically
instead of during module import.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c950e595-7e76-46c2-b700-6a7994ed0cbc
⛔ Files ignored due to path filters (4)
bun.lockis excluded by!**/*.lock,!**/*.lockdev-docs/bun.mdis excluded by!dev-docs/**package-lock.jsonis excluded by!**/package-lock.json,!package-lock.jsonproject-plans/issue3181/PLAN.mdis excluded by!project-plans/**
📒 Files selected for processing (25)
docs/shell-replacement.mdpackages/cli/package.jsonpackages/cli/src/services/prompt-processors/shellProcessor.test.tspackages/cli/src/services/prompt-processors/shellProcessor.tspackages/core/package.jsonpackages/core/src/tools-adapters/CoreShellToolHostAdapter.test.tspackages/core/src/tools-adapters/CoreShellToolHostAdapter.tspackages/core/src/utils/powershell-ast.tspackages/core/src/utils/powershell-parse-result.tspackages/core/src/utils/shell-parser-node-smoke.test.tspackages/core/src/utils/shell-parser-pwsh.test.tspackages/core/src/utils/shell-parser.tspackages/core/src/utils/shell-utils.detectSubstitution.test.tspackages/core/src/utils/shell-utils.multiline.test.tspackages/core/src/utils/shell-utils.parserUnavailable.test.tspackages/core/src/utils/shell-utils.powershell-wrappers.test.tspackages/core/src/utils/shell-utils.powershell.test.tspackages/core/src/utils/shell-utils.pwshUnavailable.test.tspackages/core/src/utils/shell-utils.shellReplacement.test.tspackages/core/src/utils/shell-utils.test.tspackages/core/src/utils/shell-utils.tspackages/core/src/utils/tool-utils.tsscripts/bun-native-modules-smoke.tsscripts/tests/bun-workspaces.test.tsscripts/tests/issue-3181-pwsh-resolution.bun.test.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
OpenCodeReview — automatic reviews suspendedAutomatic OCR reviews are suspended for this PR after 2 of 2 automatic reviews. To get more reviews you can:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/utils/powershell-ast.ts (1)
297-303: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winNormalize the bare
command_namebefore wrapper classification.A path-qualified
powershell.exeis not classified as a wrapper, so its nested-Commandpayload is not validated. UsenormalizePwshCommandNamein the barecommand_namebranch and add a regression test for a path-qualified wrapper.🤖 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/core/src/utils/powershell-ast.ts` around lines 297 - 303, Update getPwshCommandName to pass the bare command_name text through normalizePwshCommandName before returning it, ensuring path-qualified PowerShell executables are recognized by wrapper classification and their nested -Command payload is validated. Add a regression test covering a path-qualified wrapper.
🧹 Nitpick comments (1)
packages/core/src/utils/powershell-ast-security.test.ts (1)
22-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe hard throw makes every
skipIfguard dead code.Lines 24-26 throw when the PowerShell grammar is unavailable, so module evaluation stops. The
describe.skipIf(!pwshAvailable)guards at Lines 52, 123, 241, 277, 301, 364, and 399 can never take the skip branch. Choose one policy: keep the throw and drop the guards, or drop the throw and let the guards skip the suites.🤖 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/core/src/utils/powershell-ast-security.test.ts` around lines 22 - 26, Remove the module-level PowerShell availability throw after initializeParser and retain the pwshAvailable checks so each describe.skipIf guard can skip its suite when the grammar is unavailable. Keep parser initialization and the existing availability detection unchanged.
🤖 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/core/src/utils/powershell-wrapper-payload.ts`:
- Around line 142-156: Update isWrapperFlagMatch for the pwsh category to
require text to begin with '-' before performing the command-prefix comparison;
do not strip an optional dash and accept bare tokens. Preserve exact matches
through flags.has(lowered), while ensuring positional arguments such as command
are not treated as wrapper flags by findFlagElementIndex or
extractPwshBareWrapperPayload.
---
Outside diff comments:
In `@packages/core/src/utils/powershell-ast.ts`:
- Around line 297-303: Update getPwshCommandName to pass the bare command_name
text through normalizePwshCommandName before returning it, ensuring
path-qualified PowerShell executables are recognized by wrapper classification
and their nested -Command payload is validated. Add a regression test covering a
path-qualified wrapper.
---
Nitpick comments:
In `@packages/core/src/utils/powershell-ast-security.test.ts`:
- Around line 22-26: Remove the module-level PowerShell availability throw after
initializeParser and retain the pwshAvailable checks so each describe.skipIf
guard can skip its suite when the grammar is unavailable. Keep parser
initialization and the existing availability detection unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d9b7393a-c948-48e8-9aa4-f6d205cb99df
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lock,!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json,!package-lock.json
📒 Files selected for processing (16)
docs/shell-replacement.mdpackage.jsonpackages/core/src/tools-adapters/CoreShellToolHostAdapter.test.tspackages/core/src/utils/powershell-ast-security.test.tspackages/core/src/utils/powershell-ast.tspackages/core/src/utils/powershell-parse-result.tspackages/core/src/utils/powershell-wrapper-payload.tspackages/core/src/utils/shell-parser-node-smoke.test.tspackages/core/src/utils/shell-parser-pwsh.test.tspackages/core/src/utils/shell-parser.tspackages/core/src/utils/shell-utils.powershell-wrappers.test.tspackages/core/src/utils/shell-utils.powershell.test.tspackages/core/src/utils/shell-utils.pwshUnavailable.test.tspackages/core/src/utils/shell-utils.test.tspackages/core/src/utils/shell-utils.tspackages/core/src/utils/tool-utils.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/core/src/utils/powershell-parse-result.ts
- packages/core/src/utils/shell-utils.powershell.test.ts
- packages/core/src/tools-adapters/CoreShellToolHostAdapter.test.ts
- packages/core/src/utils/shell-utils.powershell-wrappers.test.ts
- packages/core/src/utils/shell-utils.pwshUnavailable.test.ts
- packages/core/src/utils/tool-utils.ts
- docs/shell-replacement.md
- packages/core/src/utils/shell-utils.test.ts
- packages/core/src/utils/shell-parser.ts
- packages/core/src/utils/shell-utils.ts
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.
TLDR
Fixes Windows shell-command false rejections by validating PowerShell commands with a PowerShell grammar instead of always parsing them as Bash.
The validator now uses the same resolved shell as execution, so valid constructs such as:
reach normal permission evaluation instead of failing with
Command rejected because it could not be parsed safely.Important review points:
tree-sitter-pwshbecause that grammar reproducibly crashes Node 24 during shutdown; Node PowerShell validation fails closed with a truthful parser-unavailable diagnostic.cmd.exekeeps the legacy Bash-parser fallback because no dedicated cmd grammar is available.Dive Deeper
Root cause
Execution was already shell-aware, but validation, command-root extraction, splitting, and substitution detection always used
tree-sitter-bash. Valid PowerShell control flow, assignments, script blocks, member expressions, redirects, call operators, and multiline commands could therefore produce BashERROR/MISSINGnodes and be hard-denied before execution.Implementation
tree-sitter-pwshWASM under Bun and keeps Node on the Bash-only initialization path.allowedToolschecks, and CLI prompt processing.$()subexpressions, literal call/dot-source targets, invocation expressions, wrappers, evaluators, and process launchers.$()is substitution; backticks and@()are not Bash-style command substitution.none,allowlist, andallmodes.Invoke-Expression, andStart-Processaliases, including literal call-operator forms and executable suffixes.tree-sitter-pwshinstall scripts untrusted because only the published WASM asset is consumed.tree-sitter-pwsha direct core and CLI dependency and verifies strict non-hoisted CLI WASM resolution.The project plan at
project-plans/issue3181/PLAN.mddocuments the reproduction corpus, parser evaluation, security policy, runtime constraint, and verification history.Security boundary
This preserves fail-closed behavior for malformed syntax, unavailable PowerShell structural parsing, dynamic strict-allowlist targets, invocation expressions, and unresolved static wrapper payloads. Static nested payloads are recursively parsed with strict-decrease termination.
A blocklist alone is not represented as a complete sandbox for dynamically generated command text; the documentation states this limitation explicitly.
Reviewer Test Plan
On Windows/Bun, run:
Confirm the exact issue command passes
ShellToolvalidation:Confirm malformed PowerShell such as
Get-ChildItem |receives a PowerShell-specific hard denial.Confirm nested blocklisted commands are denied through script blocks,
$(),iex,pwsh -Command, literal&wrappers, andStart-Process.Confirm strict allowlists reject dynamic call targets and .NET/member invocation expressions.
Run
bun scripts/bun-native-modules-smoke.tsandbun test scripts/tests/issue-3181-pwsh-resolution.bun.test.ts.Run
bun test packages/core/src/utils/shell-parser-node-smoke.test.tsto verify the production parser initializes Bash only and exits cleanly under Node.Local verification:
npm run format: passed.npm run typecheck: passed.npm run build: passed.bun scripts/bun-native-modules-smoke.ts: passed, with the expected Windows-only PTY-adapter skip.git diff --check: passed.READYwith no findings.Local environment limitations, reported rather than hidden:
npm run testreached an unchanged Windows cancellation-race test and stalled; focused affected suites are green.npm run lintexited without diagnostics on this Windows machine, and a direct default-heap full ESLint run exhausted V8 memory; explicit changed/new-file ESLint is green.stepfun-37profile is absent.Testing Matrix
Linked issues / bugs
Fixes #3181
Summary by CodeRabbit
New Features
Documentation