Skip to content

fix: add shell() helper, fix run() misuse across 13 tools - #63

Closed
TerminalGravity wants to merge 1 commit into
mainfrom
fix/shell-commands-in-run
Closed

fix: add shell() helper, fix run() misuse across 13 tools#63
TerminalGravity wants to merge 1 commit into
mainfrom
fix/shell-commands-in-run

Conversation

@TerminalGravity

Copy link
Copy Markdown
Collaborator

Problem

run() in src/lib/git.ts uses execFileSync('git', args) without a shell, but 13 tool files were passing shell syntax (pipes, redirects, &&, ||) and non-git commands (cat, find, wc, head, tail, tsc, gh) through it. These calls silently fail or produce wrong results.

Fix

  • Added shell() export to src/lib/git.ts — uses execSync with a real shell
  • Migrated all 13 affected tools to use shell() or run([...args]) array form

Build clean, all 43 tests pass.

run() uses execFileSync('git', args) without a shell, but 13 tool files
were passing shell syntax (pipes, redirects, &&, ||) and non-git commands
(cat, find, wc, head, tail, command, tsc, gh) through it. These calls
would silently fail or produce wrong results since execFileSync without
shell:true treats pipes/redirects as literal arguments.

Added shell() to git.ts that uses execSync with a real shell for commands
that need pipes, redirects, or non-git binaries. Migrated all misused
run() calls to either:
- shell() for commands needing shell features
- run([...args]) array form for pure git commands (no shell needed)

Affected tools: audit-workspace, checkpoint, clarify-intent,
enrich-agent-task, scope-work, sequence-tasks, session-handoff,
session-health, sharpen-followup, token-audit, verify-completion,
what-changed

@TerminalGravity TerminalGravity left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Reviewed — this looks solid. The silent failures from passing shell syntax through execFileSync were a real footgun. The shell() helper is clean: good timeout handling, proper error messages, sensible buffer limit. One thought: consider a brief JSDoc note about not passing user-controlled strings to avoid injection. Ready to merge.

@TerminalGravity

Copy link
Copy Markdown
Collaborator Author

Closing — superseded by newer PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant