Skip to content

Commit 5594e3d

Browse files
TerminalGravityTerminalGravity
committed
fix: add shell() helper, fix remaining shell syntax in run() calls
Fixes #110. After the execFileSync migration, 8 tool files still passed shell syntax (pipes, redirects, non-git binaries) to run(), which uses execFileSync('git', args) — causing silent failures. Changes: - Add shell() helper in git.ts using execSync for commands that genuinely need shell features (pipes, redirects, non-git binaries) - Convert simple git commands to array args (diff, status) - Route shell-dependent commands (wc, find, cat, head, tail, tsc, command -v, gh, grep pipes) through shell() Affected: verify-completion, token-audit, clarify-intent, session-handoff, audit-workspace, enrich-agent-task, scope-work, sequence-tasks
1 parent 814b247 commit 5594e3d

9 files changed

Lines changed: 61 additions & 35 deletions

‎src/lib/git.ts‎

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { execFileSync } from "child_process";
1+
import { execFileSync, execSync } from "child_process";
22
import { PROJECT_DIR } from "./files.js";
33
import type { RunError } from "../types.js";
44

@@ -30,6 +30,32 @@ export function run(argsOrCmd: string | string[], opts: { timeout?: number } = {
3030
}
3131
}
3232

33+
/**
34+
* Run a shell command string (via /bin/sh). Use for commands that genuinely
35+
* need shell features: pipes, redirects, non-git binaries (find, wc, cat, etc).
36+
* Callers MUST NOT interpolate untrusted input — use shellEscape() or build
37+
* safe command strings with known values only.
38+
*/
39+
export function shell(cmd: string, opts: { timeout?: number } = {}): string {
40+
try {
41+
return execSync(cmd, {
42+
cwd: PROJECT_DIR,
43+
encoding: "utf-8",
44+
timeout: opts.timeout || 10000,
45+
maxBuffer: 1024 * 1024,
46+
stdio: ["pipe", "pipe", "pipe"],
47+
}).trim();
48+
} catch (e: any) {
49+
const timedOut = e.killed === true || e.signal === "SIGTERM";
50+
if (timedOut) {
51+
return `[timed out after ${opts.timeout || 10000}ms]`;
52+
}
53+
const output = e.stdout?.trim() || e.stderr?.trim();
54+
if (output) return output;
55+
return `[shell command failed: ${cmd} (exit ${e.status ?? "?"})]`;
56+
}
57+
}
58+
3359
/** Convenience: run a raw command string (split on spaces). Only for simple, known-safe commands. */
3460
function gitCmd(cmdStr: string, opts?: { timeout?: number }): string {
3561
return run(cmdStr.split(/\s+/), opts);

‎src/tools/audit-workspace.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js";
2-
import { run } from "../lib/git.js";
2+
import { run, shell } from "../lib/git.js";
33
import { readIfExists, findWorkspaceDocs } from "../lib/files.js";
44

55
/** Extract top-level work areas from file paths generically */
@@ -36,7 +36,7 @@ export function registerAuditWorkspace(server: McpServer): void {
3636
{},
3737
async () => {
3838
const docs = findWorkspaceDocs();
39-
const recentFiles = run("git diff --name-only HEAD~10 2>/dev/null || echo ''").split("\n").filter(Boolean);
39+
const recentFiles = run(["diff", "--name-only", "HEAD~10"]).split("\n").filter(Boolean);
4040
const sections: string[] = [];
4141

4242
// Doc freshness
@@ -75,7 +75,7 @@ export function registerAuditWorkspace(server: McpServer): void {
7575
// Check for gap trackers or similar tracking docs
7676
const trackingDocs = Object.entries(docs).filter(([n]) => /gap|track|progress/i.test(n));
7777
if (trackingDocs.length > 0) {
78-
const testFilesCount = parseInt(run("find tests -name '*.spec.ts' -o -name '*.test.ts' 2>/dev/null | wc -l").trim()) || 0;
78+
const testFilesCount = parseInt(shell("find tests -name '*.spec.ts' -o -name '*.test.ts' 2>/dev/null | wc -l").trim()) || 0;
7979
sections.push(`## Tracking Docs\n${trackingDocs.map(([n]) => {
8080
const age = docStatus.find(d => d.name === n)?.ageHours ?? "?";
8181
return `- .claude/${n} — last updated ${age}h ago`;

‎src/tools/clarify-intent.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { z } from "zod";
22
import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js";
3-
import { run, getBranch, getStatus, getRecentCommits, getDiffFiles, getStagedFiles } from "../lib/git.js";
3+
import { run, shell, getBranch, getStatus, getRecentCommits, getDiffFiles, getStagedFiles } from "../lib/git.js";
44
import { findWorkspaceDocs, PROJECT_DIR } from "../lib/files.js";
55
import { searchSemantic } from "../lib/timeline-db.js";
66
import { getRelatedProjects } from "../lib/config.js";
@@ -152,10 +152,10 @@ export function registerClarifyIntent(server: McpServer): void {
152152
let hasTestFailures = false;
153153

154154
if (!area || area.includes("test") || area.includes("fix") || area.includes("ui") || area.includes("api")) {
155-
const typeErrors = run("pnpm tsc --noEmit 2>&1 | grep -c 'error TS' || echo '0'");
155+
const typeErrors = shell("pnpm tsc --noEmit 2>&1 | grep -c 'error TS' || echo '0'");
156156
hasTypeErrors = parseInt(typeErrors, 10) > 0;
157157

158-
const testFiles = run("find tests -name '*.spec.ts' -maxdepth 4 2>/dev/null | head -20");
158+
const testFiles = shell("find tests -name '*.spec.ts' -maxdepth 4 2>/dev/null | head -20");
159159
const failingTests = getTestFailures();
160160
hasTestFailures = failingTests !== "all passing" && failingTests !== "no test report found";
161161

‎src/tools/enrich-agent-task.ts‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { z } from "zod";
22
import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js";
3-
import { run, getDiffFiles } from "../lib/git.js";
3+
import { run, shell, getDiffFiles } from "../lib/git.js";
44
import { PROJECT_DIR } from "../lib/files.js";
55
import { getConfig, type RelatedProject } from "../lib/config.js";
66
import { existsSync, readFileSync } from "fs";
@@ -29,31 +29,31 @@ function findAreaFiles(area: string): string {
2929

3030
// If area looks like a path, search directly
3131
if (area.includes("/")) {
32-
return run(`git ls-files -- '${safeArea}*' 2>/dev/null | head -20`);
32+
return shell(`git ls-files -- '${safeArea}*' 2>/dev/null | head -20`);
3333
}
3434

3535
// Search for area keyword in git-tracked file paths
36-
const files = run(`git ls-files 2>/dev/null | grep -i '${safeArea}' | head -20`);
37-
if (files && !files.startsWith("[command failed")) return files;
36+
const files = shell(`git ls-files 2>/dev/null | grep -i '${safeArea}' | head -20`);
37+
if (files && !files.startsWith("[shell command failed")) return files;
3838

3939
// Fallback to recently changed files
4040
return getDiffFiles("HEAD~3");
4141
}
4242

4343
/** Find related test files for an area */
4444
function findRelatedTests(area: string): string {
45-
if (!area) return run("git ls-files 2>/dev/null | grep -E '\\.(spec|test)\\.(ts|tsx|js|jsx)$' | head -10");
45+
if (!area) return shell("git ls-files 2>/dev/null | grep -E '\\.(spec|test)\\.(ts|tsx|js|jsx)$' | head -10");
4646

4747
const safeArea = shellEscape(area.split(/\s+/)[0]);
48-
const tests = run(`git ls-files 2>/dev/null | grep -E '\\.(spec|test)\\.(ts|tsx|js|jsx)$' | grep -i '${safeArea}' | head -10`);
49-
return tests || run("git ls-files 2>/dev/null | grep -E '\\.(spec|test)\\.(ts|tsx|js|jsx)$' | head -10");
48+
const tests = shell(`git ls-files 2>/dev/null | grep -E '\\.(spec|test)\\.(ts|tsx|js|jsx)$' | grep -i '${safeArea}' | head -10`);
49+
return tests || shell("git ls-files 2>/dev/null | grep -E '\\.(spec|test)\\.(ts|tsx|js|jsx)$' | head -10");
5050
}
5151

5252
/** Get an example pattern from the first matching file */
5353
function getExamplePattern(files: string): string {
5454
const firstFile = files.split("\n").filter(Boolean)[0];
5555
if (!firstFile) return "no pattern available";
56-
return run(`head -30 '${shellEscape(firstFile)}' 2>/dev/null || echo 'could not read file'`);
56+
return shell(`head -30 '${shellEscape(firstFile)}' 2>/dev/null || echo 'could not read file'`);
5757
}
5858

5959
// ---------------------------------------------------------------------------

‎src/tools/scope-work.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// CATEGORY 1: scope_work — Plans
22
import { z } from "zod";
33
import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js";
4-
import { run, getBranch, getRecentCommits, getStatus } from "../lib/git.js";
4+
import { run, shell, getBranch, getRecentCommits, getStatus } from "../lib/git.js";
55
import { readIfExists, findWorkspaceDocs, PROJECT_DIR } from "../lib/files.js";
66
import { searchSemantic } from "../lib/timeline-db.js";
77
import { getRelatedProjects } from "../lib/config.js";
@@ -128,7 +128,7 @@ export function registerScopeWork(server: McpServer): void {
128128
.slice(0, 5);
129129
if (grepTerms.length > 0) {
130130
const pattern = shellEscape(grepTerms.join("|"));
131-
matchedFiles = run(`git ls-files | head -500 | grep -iE '${pattern}' | head -30`);
131+
matchedFiles = shell(`git ls-files | head -500 | grep -iE '${pattern}' | head -30`);
132132
}
133133

134134
// Check which relevant dirs actually exist (with path traversal protection)

‎src/tools/sequence-tasks.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// CATEGORY 6: sequence_tasks — Sequencing
22
import { z } from "zod";
33
import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js";
4-
import { run } from "../lib/git.js";
4+
import { run, shell } from "../lib/git.js";
55
import { now } from "../lib/state.js";
66
import { PROJECT_DIR } from "../lib/files.js";
77
import { existsSync } from "fs";
@@ -90,7 +90,7 @@ export function registerSequenceTasks(server: McpServer): void {
9090
// For locality: infer directories from path-like tokens in task text
9191
if (strategy === "locality") {
9292
// Use git ls-files with a depth limit instead of find for performance
93-
const gitFiles = run("git ls-files 2>/dev/null | head -1000");
93+
const gitFiles = shell("git ls-files 2>/dev/null | head -1000");
9494
const knownDirs = new Set<string>();
9595
for (const f of gitFiles.split("\n").filter(Boolean)) {
9696
const parts = f.split("/");

‎src/tools/session-handoff.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,14 @@ import { z } from "zod";
22
import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js";
33
import { existsSync, readFileSync } from "fs";
44
import { join } from "path";
5-
import { run, getBranch, getRecentCommits, getStatus } from "../lib/git.js";
5+
import { run, shell, getBranch, getRecentCommits, getStatus } from "../lib/git.js";
66
import { readIfExists, findWorkspaceDocs } from "../lib/files.js";
77
import { STATE_DIR, now } from "../lib/state.js";
88

99
/** Check if a CLI tool is available */
1010
function hasCommand(cmd: string): boolean {
11-
const result = run(`command -v ${cmd} 2>/dev/null`);
12-
return !!result && !result.startsWith("[command failed");
11+
const result = shell(`command -v ${cmd} 2>/dev/null`);
12+
return !!result && !result.startsWith("[shell command failed");
1313
}
1414

1515
export function registerSessionHandoff(server: McpServer): void {
@@ -44,7 +44,7 @@ export function registerSessionHandoff(server: McpServer): void {
4444

4545
// Only try gh if it exists
4646
if (hasCommand("gh")) {
47-
const openPRs = run("gh pr list --state open --json number,title,headRefName 2>/dev/null || echo '[]'");
47+
const openPRs = shell("gh pr list --state open --json number,title,headRefName 2>/dev/null || echo '[]'");
4848
if (openPRs && openPRs !== "[]") {
4949
sections.push(`## Open PRs\n\`\`\`json\n${openPRs}\n\`\`\``);
5050
}

‎src/tools/token-audit.ts‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// CATEGORY 5: token_audit — Token Efficiency
22
import { z } from "zod";
33
import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js";
4-
import { run } from "../lib/git.js";
4+
import { run, shell } from "../lib/git.js";
55
import { readIfExists, findWorkspaceDocs, PROJECT_DIR } from "../lib/files.js";
66
import { loadState, saveState, now, STATE_DIR } from "../lib/state.js";
77
import { readFileSync, existsSync, statSync } from "fs";
@@ -39,8 +39,8 @@ export function registerTokenAudit(server: McpServer): void {
3939
let wasteScore = 0;
4040

4141
// 1. Git diff size & dirty file count
42-
const diffStat = run("git diff --stat --no-color 2>/dev/null");
43-
const dirtyFiles = run("git diff --name-only 2>/dev/null");
42+
const diffStat = run(["diff", "--stat", "--no-color"]);
43+
const dirtyFiles = run(["diff", "--name-only"]);
4444
const dirtyList = dirtyFiles.split("\n").filter(Boolean);
4545
const dirtyCount = dirtyList.length;
4646

@@ -63,7 +63,7 @@ export function registerTokenAudit(server: McpServer): void {
6363

6464
for (const f of dirtyList.slice(0, 30)) {
6565
// Use shell-safe quoting instead of interpolation
66-
const wc = run(`wc -l < '${shellEscape(f)}' 2>/dev/null`);
66+
const wc = shell(`wc -l < '${shellEscape(f)}' 2>/dev/null`);
6767
const lines = parseInt(wc) || 0;
6868
estimatedContextTokens += lines * AVG_LINE_BYTES * AVG_TOKENS_PER_BYTE;
6969
if (lines > 500) {
@@ -80,7 +80,7 @@ export function registerTokenAudit(server: McpServer): void {
8080
// 3. CLAUDE.md bloat check
8181
const claudeMd = readIfExists("CLAUDE.md", 1);
8282
if (claudeMd !== null) {
83-
const stat = run(`wc -c < '${shellEscape("CLAUDE.md")}' 2>/dev/null`);
83+
const stat = shell(`wc -c < '${shellEscape("CLAUDE.md")}' 2>/dev/null`);
8484
const bytes = parseInt(stat) || 0;
8585
if (bytes > 5120) {
8686
patterns.push(`CLAUDE.md is ${(bytes / 1024).toFixed(1)}KB — injected every session, burns tokens on paste`);
@@ -139,7 +139,7 @@ export function registerTokenAudit(server: McpServer): void {
139139
// Read with size cap: take the tail if too large
140140
const raw = stat.size <= MAX_TOOL_LOG_BYTES
141141
? readFileSync(toolLogPath, "utf-8")
142-
: run(`tail -c ${MAX_TOOL_LOG_BYTES} '${shellEscape(toolLogPath)}'`);
142+
: shell(`tail -c ${MAX_TOOL_LOG_BYTES} '${shellEscape(toolLogPath)}'`);
143143

144144
const lines = raw.trim().split("\n").filter(Boolean);
145145
totalToolCalls = lines.length;

‎src/tools/verify-completion.ts‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { z } from "zod";
22
import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js";
3-
import { run, getStatus } from "../lib/git.js";
3+
import { run, shell, getStatus } from "../lib/git.js";
44
import { PROJECT_DIR } from "../lib/files.js";
55
import { existsSync } from "fs";
66
import { join } from "path";
@@ -34,7 +34,7 @@ function detectTestRunner(): string | null {
3434
/** Check if a build script exists in package.json */
3535
function hasBuildScript(): boolean {
3636
try {
37-
const pkg = JSON.parse(run("cat package.json 2>/dev/null"));
37+
const pkg = JSON.parse(shell("cat package.json 2>/dev/null"));
3838
return !!pkg?.scripts?.build;
3939
} catch { return false; }
4040
}
@@ -55,7 +55,7 @@ export function registerVerifyCompletion(server: McpServer): void {
5555
const checks: { name: string; passed: boolean; detail: string }[] = [];
5656

5757
// 1. Type check (single invocation, extract both result and count)
58-
const tscOutput = run(`${pm === "npx" ? "npx" : pm} tsc --noEmit 2>&1 | tail -20`);
58+
const tscOutput = shell(`${pm === "npx" ? "npx" : pm} tsc --noEmit 2>&1 | tail -20`);
5959
const errorLines = tscOutput.split("\n").filter(l => /error TS\d+/.test(l));
6060
const typePassed = errorLines.length === 0;
6161
checks.push({
@@ -80,7 +80,7 @@ export function registerVerifyCompletion(server: McpServer): void {
8080
// 3. Tests
8181
if (!skip_tests) {
8282
const runner = detectTestRunner();
83-
const changedFiles = run("git diff --name-only HEAD~1 2>/dev/null").split("\n").filter(Boolean);
83+
const changedFiles = run(["diff", "--name-only", "HEAD~1"]).split("\n").filter(Boolean);
8484
let testCmd = "";
8585

8686
if (runner === "playwright") {
@@ -112,7 +112,7 @@ export function registerVerifyCompletion(server: McpServer): void {
112112
}
113113

114114
if (testCmd) {
115-
const testResult = run(testCmd, { timeout: 120000 });
115+
const testResult = shell(testCmd, { timeout: 120000 });
116116
const testPassed = /pass/i.test(testResult) && !/fail/i.test(testResult);
117117
checks.push({
118118
name: "Tests",
@@ -130,7 +130,7 @@ export function registerVerifyCompletion(server: McpServer): void {
130130

131131
// 4. Build check (only if build script exists and not skipped)
132132
if (!skip_build && hasBuildScript()) {
133-
const buildCheck = run(`${pm === "npx" ? "npm run" : pm} build 2>&1 | tail -10`, { timeout: 60000 });
133+
const buildCheck = shell(`${pm === "npx" ? "npm run" : pm} build 2>&1 | tail -10`, { timeout: 60000 });
134134
const buildPassed = !/\b[Ee]rror\b/.test(buildCheck) || /Successfully compiled/.test(buildCheck);
135135
checks.push({
136136
name: "Build",

0 commit comments

Comments
 (0)