refactor: replace shell-utils string parsing with tree-sitter AST#68
Open
BingqingLyu wants to merge 6 commits into
Open
refactor: replace shell-utils string parsing with tree-sitter AST#68BingqingLyu wants to merge 6 commits into
BingqingLyu wants to merge 6 commits into
Conversation
- Add sync AST functions to shellAstParser.ts (splitCommandsAST, getCommandRootAST, getCommandRootsAST, detectCommandSubstitutionAST, tokenizeCommandAST, isShellCommandReadOnlySync) - Add ensureParserInitStarted() for fire-and-forget lazy initialization - Migrate shell-utils.ts functions to AST-first with legacy fallback - Migrate rule-parser.ts splitCompoundCommand to AST-first - Migrate shell-semantics.ts extractShellOperations to AST-first - Add BASIC_READ_ONLY_COMMANDS fallback set for isCommandNeedsPermission - Delete deprecated shellReadOnlyChecker.ts and its tests - Add comprehensive tests for all new sync AST functions - Add AST-path integration tests for shell-semantics
# Conflicts: # packages/core/src/permissions/rule-parser.ts # packages/core/src/utils/shell-utils.ts # packages/core/src/utils/shellAstParser.test.ts
- shellAstParser.ts: remove import from deleted shellReadOnlyChecker.ts, replace fallback calls with conservative false (requires permission) - coreToolScheduler.ts: migrate from deleted isShellCommandReadOnly to isShellCommandReadOnlySync from shellAstParser.ts (null → false) - shellAstParser.test.ts: update fallback tests to reflect conservative behavior, remove consistency suite that depended on deleted file
- coreToolScheduler: use isCommandNeedsPermission (three-tier fallback) instead of isShellCommandReadOnlySync ?? false for isConcurrencySafe; this avoids treating all commands as sequential when WASM is still loading - shell-utils: add GIT_READ_ONLY_SUBCOMMANDS set and git subcommand check to isCommandNeedsPermission lightweight fallback, restoring the behavior that shellReadOnlyChecker.ts previously provided for git log/status/diff etc. Fixes: should run read-only shell commands concurrently test
… not ready" This reverts commit 86e83f9.
Owner
Author
Conflict Group 1This PR shares modified functions with 1 other PR(s): #96. These PRs should be reviewed as a batch — merging one may affect the others.
graph LR
PR68["PR #68"]
FpartitionToolCalls_1638["partitionToolCalls<br>coreToolScheduler.ts"]
PR68 -->|modifies| FpartitionToolCalls_1638
PR96["PR #96"]
PR96 -->|modifies| FpartitionToolCalls_1638
Posted by codegraph-ai conflict detection. |
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
2 participants
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
Replace all string-based shell command parsing in
shell-utils.ts,rule-parser.ts, andshell-semantics.tswith tree-sitter AST-first approach, using the existingshellAstParser.tsinfrastructure. This significantly improves parsing robustness for quoted strings, heredocs, nested constructs, and complex compound commands.Screenshots / Video Demo
N/A — no user-facing change. This is an internal refactor of shell command parsing logic.
Dive Deeper
Architecture: Sync/Async Hybrid with Lazy Initialization
The key challenge is that tree-sitter WASM initialization is async (
Parser.init()+Language.load()), but most callers are synchronous. The solution:ensureParserInitStarted()— All sync AST functions call this when the parser isn't ready, triggering a fire-and-forgetinitParser(). The current call falls back to the legacy implementation, but the parser starts loading in the background.*Legacy()functions, ensuring correctness even if the parser fails to load.Changes by File
shellAstParser.tssplitCommandsAST,getCommandRootAST,getCommandRootsAST,detectCommandSubstitutionAST,tokenizeCommandAST,isShellCommandReadOnlySync,ensureParserInitStartedshell-utils.tssplitCommands,getCommandRoot,getCommandRoots,detectCommandSubstitution,isCommandNeedsPermissionto AST-first with legacy fallback. AddedBASIC_READ_ONLY_COMMANDSlightweight fallback set.rule-parser.tssplitCompoundCommandto AST-first with legacy fallbackshell-semantics.tsextractShellOperationsto AST-first tokenization with legacy fallbackshellReadOnlyChecker.tsisShellCommandReadOnlySyncin shellAstParser)shellAstParser.test.tsshell-semantics.test.tsThree-tier fallback for
isCommandNeedsPermissionisShellCommandReadOnlySync) — when parser is readyBASIC_READ_ONLY_COMMANDSset (getCommandRoot+ set lookup) — lightweight sync fallbackReviewer Test Plan
npx vitest run packages/core/src/utils/shellAstParser.test.ts packages/core/src/utils/shell-utils.test.ts packages/core/src/permissions/shell-semantics.test.ts packages/core/src/permissions/permission-manager.test.ts packages/core/src/tools/shell.test.tsnpm run testnpx tsc --noEmit --project packages/core/tsconfig.jsonnpm start, execute shell commands, verify permission prompts work correctly for both read-only and mutating commandsTesting Matrix
Linked issues / bugs
N/A — proactive refactor to improve shell parsing robustness.