fix: consolidate shellEscape to canonical lib/shell.ts#126
Open
TerminalGravity wants to merge 5 commits intomainfrom
Open
fix: consolidate shellEscape to canonical lib/shell.ts#126TerminalGravity wants to merge 5 commits intomainfrom
TerminalGravity wants to merge 5 commits intomainfrom
Conversation
Covers LanceDB native binary failures, CLAUDE_PROJECT_DIR config, vector search not returning results, MCP handshake debugging, and performance tips. Links from README nav bar.
- examples/.preflight/config.yml: profile, related projects, thresholds, embeddings - examples/.preflight/triage.yml: strictness, always_check/skip/cross-service keywords - examples/.preflight/README.md: setup instructions and env var fallback reference - README.md: link to examples from Configuration Reference section
… tool files Adds src/lib/shell.ts with shell() (uses execSync with shell) and shellEscape() for safe interpolation. Migrates all tool files that were passing shell syntax (pipes, redirects, ||, &&) to run() (which uses execFileSync without shell) to use shell() instead. Simple git commands converted to array args where possible. Fixes #110
… and scope-work - enrich-agent-task.ts had a weak shellEscape that stripped chars instead of quoting, which could mangle paths with spaces. Now uses shellEscape() from lib/shell.ts for proper single-quote wrapping, and a separate sanitizePattern() for grep pattern sanitization. - scope-work.ts had a duplicate shellEscape definition. Replaced with import from lib/shell.ts for consistency. - Also switched from string interpolation inside quotes to shellEscape() calls for grep arguments, preventing potential injection if sanitization is bypassed. Closes #110 (all files now use shell() or proper escaping).
TerminalGravity
commented
Mar 6, 2026
Collaborator
Author
TerminalGravity
left a comment
TerminalGravity
left a comment
There was a problem hiding this comment.
This is the right direction — single canonical shell escape in lib/shell.ts. However, there are now 5 open PRs all addressing shell syntax (#119, #120, #123, #124, #125, #126). Recommend:
- Pick this one (#126) as the canonical fix since it consolidates properly
- Close #119, #123, #124, #125 as superseded
- Make sure #120's specific file fixes are captured here
Otherwise we'll end up with merge conflicts across all of them.
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.
Problem
Two tool files had their own local
shellEscapeimplementations instead of using the canonical one fromlib/shell.ts:enrich-agent-task.ts— stripped non-alphanumeric chars (weak, could mangle legitimate paths with spaces)scope-work.ts— duplicated single-quote escaping logicThis inconsistency is a latent bug: the stripping approach silently corrupts paths containing spaces or special characters.
Fix
shellEscapefrom../lib/shell.jsin both filessanitizePattern()inenrich-agent-task.ts(still used for grep pattern safety, distinct from path quoting)shellEscape()callsshellEscapedefinition fromscope-work.tsBuild clean, all 43 tests pass.
Closes #110