From 81e0e9acb9ea9179625d0bddf7ae613fd681d955 Mon Sep 17 00:00:00 2001 From: Jack Felke Date: Mon, 9 Mar 2026 10:47:44 -0700 Subject: [PATCH 1/2] fix: eliminate shell syntax in git.run() calls across 8 tools (#172) Replace shell-piped commands passed to git.run() (which uses execFileSync without a shell) with proper array-arg calls and Node.js equivalents: - verify-completion: use execFileSync for tsc/build, readFileSync for pkg - token-audit: use fs.statSync/readFileSync instead of wc, openSync for tail - session-handoff: use which+execFileSync instead of command -v, direct gh call - audit-workspace: array args for git diff, git ls-files for test file count - scope-work: filter git ls-files in JS instead of piping through grep - enrich-agent-task: JS filtering/readFileSync instead of grep/head pipes - sequence-tasks: array args for git ls-files, JS slice - sharpen-followup: array args for git status Removes dead shellEscape helpers where no longer needed. Fixes #172 --- src/tools/audit-workspace.ts | 5 +-- src/tools/enrich-agent-task.ts | 40 ++++++++++++++-------- src/tools/scope-work.ts | 10 ++---- src/tools/sequence-tasks.ts | 2 +- src/tools/session-handoff.ts | 14 ++++++-- src/tools/sharpen-followup.ts | 2 +- src/tools/token-audit.ts | 36 +++++++++++--------- src/tools/verify-completion.ts | 61 ++++++++++++++++++++++++---------- 8 files changed, 108 insertions(+), 62 deletions(-) diff --git a/src/tools/audit-workspace.ts b/src/tools/audit-workspace.ts index d4306bd..6f5b5b8 100644 --- a/src/tools/audit-workspace.ts +++ b/src/tools/audit-workspace.ts @@ -36,7 +36,7 @@ export function registerAuditWorkspace(server: McpServer): void { {}, async () => { const docs = findWorkspaceDocs(); - const recentFiles = run("git diff --name-only HEAD~10 2>/dev/null || echo ''").split("\n").filter(Boolean); + const recentFiles = run(["diff", "--name-only", "HEAD~10"]).split("\n").filter(Boolean); const sections: string[] = []; // Doc freshness @@ -75,7 +75,8 @@ export function registerAuditWorkspace(server: McpServer): void { // Check for gap trackers or similar tracking docs const trackingDocs = Object.entries(docs).filter(([n]) => /gap|track|progress/i.test(n)); if (trackingDocs.length > 0) { - const testFilesCount = parseInt(run("find tests -name '*.spec.ts' -o -name '*.test.ts' 2>/dev/null | wc -l").trim()) || 0; + const testFilesList = run(["ls-files", "tests/**/*.spec.ts", "tests/**/*.test.ts"]); + const testFilesCount = testFilesList ? testFilesList.split("\n").filter(Boolean).length : 0; sections.push(`## Tracking Docs\n${trackingDocs.map(([n]) => { const age = docStatus.find(d => d.name === n)?.ageHours ?? "?"; return `- .claude/${n} — last updated ${age}h ago`; diff --git a/src/tools/enrich-agent-task.ts b/src/tools/enrich-agent-task.ts index 236edfa..c73d7b0 100644 --- a/src/tools/enrich-agent-task.ts +++ b/src/tools/enrich-agent-task.ts @@ -8,8 +8,8 @@ import { execFileSync } from "child_process"; import { join, basename } from "path"; import { createHash } from "crypto"; -/** Sanitize user input for safe use in shell commands */ -function shellEscape(s: string): string { +/** Sanitize user input for safe path matching */ +function sanitizeArea(s: string): string { return s.replace(/[^a-zA-Z0-9_\-./]/g, ""); } @@ -21,39 +21,51 @@ function detectPackageManager(): string { return "npm"; } +/** Get all git-tracked files */ +function getAllFiles(): string[] { + return run(["ls-files"]).split("\n").filter(Boolean); +} + /** Find files in a target area using git-tracked files (project-agnostic) */ function findAreaFiles(area: string): string { if (!area) return getDiffFiles("HEAD~3"); - const safeArea = shellEscape(area); + const safeArea = sanitizeArea(area); // If area looks like a path, search directly if (area.includes("/")) { - return run(`git ls-files -- '${safeArea}*' 2>/dev/null | head -20`); + const matched = getAllFiles().filter(f => f.startsWith(safeArea)).slice(0, 20); + return matched.join("\n") || getDiffFiles("HEAD~3"); } // Search for area keyword in git-tracked file paths - const files = run(`git ls-files 2>/dev/null | grep -i '${safeArea}' | head -20`); - if (files && !files.startsWith("[command failed")) return files; - - // Fallback to recently changed files - return getDiffFiles("HEAD~3"); + const pattern = new RegExp(safeArea, "i"); + const matched = getAllFiles().filter(f => pattern.test(f)).slice(0, 20); + return matched.length > 0 ? matched.join("\n") : getDiffFiles("HEAD~3"); } /** Find related test files for an area */ function findRelatedTests(area: string): string { - if (!area) return run("git ls-files 2>/dev/null | grep -E '\\.(spec|test)\\.(ts|tsx|js|jsx)$' | head -10"); + const testPattern = /\.(spec|test)\.(ts|tsx|js|jsx)$/; + const allFiles = getAllFiles(); + const testFiles = allFiles.filter(f => testPattern.test(f)); - const safeArea = shellEscape(area.split(/\s+/)[0]); - const tests = run(`git ls-files 2>/dev/null | grep -E '\\.(spec|test)\\.(ts|tsx|js|jsx)$' | grep -i '${safeArea}' | head -10`); - return tests || run("git ls-files 2>/dev/null | grep -E '\\.(spec|test)\\.(ts|tsx|js|jsx)$' | head -10"); + if (!area) return testFiles.slice(0, 10).join("\n"); + + const safeArea = sanitizeArea(area.split(/\s+/)[0]); + const areaPattern = new RegExp(safeArea, "i"); + const matched = testFiles.filter(f => areaPattern.test(f)).slice(0, 10); + return matched.length > 0 ? matched.join("\n") : testFiles.slice(0, 10).join("\n"); } /** Get an example pattern from the first matching file */ function getExamplePattern(files: string): string { const firstFile = files.split("\n").filter(Boolean)[0]; if (!firstFile) return "no pattern available"; - return run(`head -30 '${shellEscape(firstFile)}' 2>/dev/null || echo 'could not read file'`); + try { + const content = readFileSync(join(PROJECT_DIR, firstFile), "utf-8"); + return content.split("\n").slice(0, 30).join("\n"); + } catch { return "could not read file"; } } // --------------------------------------------------------------------------- diff --git a/src/tools/scope-work.ts b/src/tools/scope-work.ts index 9b5d971..d647b16 100644 --- a/src/tools/scope-work.ts +++ b/src/tools/scope-work.ts @@ -17,11 +17,6 @@ const STOP_WORDS = new Set([ "like", "some", "each", "only", "need", "want", "please", "update", "change", ]); -/** Shell-escape a string for use inside single quotes */ -function shellEscape(s: string): string { - return s.replace(/'/g, "'\\''"); -} - /** Safely parse git porcelain status lines */ function parsePortelainFiles(porcelain: string): string[] { if (!porcelain.trim()) return []; @@ -127,8 +122,9 @@ export function registerScopeWork(server: McpServer): void { .filter((k) => k.length > 2) .slice(0, 5); if (grepTerms.length > 0) { - const pattern = shellEscape(grepTerms.join("|")); - matchedFiles = run(`git ls-files | head -500 | grep -iE '${pattern}' | head -30`); + const allFiles = run(["ls-files"]).split("\n").filter(Boolean).slice(0, 500); + const pattern = new RegExp(grepTerms.join("|"), "i"); + matchedFiles = allFiles.filter(f => pattern.test(f)).slice(0, 30).join("\n"); } // Check which relevant dirs actually exist (with path traversal protection) diff --git a/src/tools/sequence-tasks.ts b/src/tools/sequence-tasks.ts index 22dea23..31c6871 100644 --- a/src/tools/sequence-tasks.ts +++ b/src/tools/sequence-tasks.ts @@ -90,7 +90,7 @@ export function registerSequenceTasks(server: McpServer): void { // For locality: infer directories from path-like tokens in task text if (strategy === "locality") { // Use git ls-files with a depth limit instead of find for performance - const gitFiles = run("git ls-files 2>/dev/null | head -1000"); + const gitFiles = run(["ls-files"]).split("\n").filter(Boolean).slice(0, 1000).join("\n"); const knownDirs = new Set(); for (const f of gitFiles.split("\n").filter(Boolean)) { const parts = f.split("/"); diff --git a/src/tools/session-handoff.ts b/src/tools/session-handoff.ts index d199462..bfc9abe 100644 --- a/src/tools/session-handoff.ts +++ b/src/tools/session-handoff.ts @@ -1,6 +1,7 @@ import { z } from "zod"; import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { existsSync, readFileSync } from "fs"; +import { execFileSync } from "child_process"; import { join } from "path"; import { run, getBranch, getRecentCommits, getStatus } from "../lib/git.js"; import { readIfExists, findWorkspaceDocs } from "../lib/files.js"; @@ -8,8 +9,10 @@ import { STATE_DIR, now } from "../lib/state.js"; /** Check if a CLI tool is available */ function hasCommand(cmd: string): boolean { - const result = run(`command -v ${cmd} 2>/dev/null`); - return !!result && !result.startsWith("[command failed"); + try { + execFileSync("which", [cmd], { encoding: "utf-8", stdio: ["pipe", "pipe", "pipe"] }); + return true; + } catch { return false; } } export function registerSessionHandoff(server: McpServer): void { @@ -44,7 +47,12 @@ export function registerSessionHandoff(server: McpServer): void { // Only try gh if it exists if (hasCommand("gh")) { - const openPRs = run("gh pr list --state open --json number,title,headRefName 2>/dev/null || echo '[]'"); + let openPRs = "[]"; + try { + openPRs = execFileSync("gh", ["pr", "list", "--state", "open", "--json", "number,title,headRefName"], { + encoding: "utf-8", timeout: 10000, stdio: ["pipe", "pipe", "pipe"], + }).trim(); + } catch { /* gh not available or not in a repo */ } if (openPRs && openPRs !== "[]") { sections.push(`## Open PRs\n\`\`\`json\n${openPRs}\n\`\`\``); } diff --git a/src/tools/sharpen-followup.ts b/src/tools/sharpen-followup.ts index db5acaa..0065458 100644 --- a/src/tools/sharpen-followup.ts +++ b/src/tools/sharpen-followup.ts @@ -87,7 +87,7 @@ export function registerSharpenFollowup(server: McpServer): void { // Gather context to resolve ambiguity const contextFiles: string[] = [...(previous_files ?? [])]; const recentChanged = getRecentChangedFiles(); - const porcelainOutput = run("git status --porcelain 2>/dev/null"); + const porcelainOutput = run(["status", "--porcelain"]); const untrackedOrModified = parsePortelainFiles(porcelainOutput); const allKnownFiles = [...new Set([...contextFiles, ...recentChanged, ...untrackedOrModified])].filter(Boolean); diff --git a/src/tools/token-audit.ts b/src/tools/token-audit.ts index b7aad2c..35dd5a7 100644 --- a/src/tools/token-audit.ts +++ b/src/tools/token-audit.ts @@ -4,14 +4,9 @@ import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { run } from "../lib/git.js"; import { readIfExists, findWorkspaceDocs, PROJECT_DIR } from "../lib/files.js"; import { loadState, saveState, now, STATE_DIR } from "../lib/state.js"; -import { readFileSync, existsSync, statSync } from "fs"; +import { readFileSync, existsSync, statSync, openSync, readSync, closeSync } from "fs"; import { join } from "path"; -/** Shell-escape a filename for safe interpolation */ -function shellEscape(s: string): string { - return s.replace(/'/g, "'\\''"); -} - /** * Grade thresholds rationale: * - A (0-10): Minimal waste — small diffs, targeted reads, lean context @@ -39,8 +34,8 @@ export function registerTokenAudit(server: McpServer): void { let wasteScore = 0; // 1. Git diff size & dirty file count - const diffStat = run("git diff --stat --no-color 2>/dev/null"); - const dirtyFiles = run("git diff --name-only 2>/dev/null"); + const diffStat = run(["diff", "--stat", "--no-color"]); + const dirtyFiles = run(["diff", "--name-only"]); const dirtyList = dirtyFiles.split("\n").filter(Boolean); const dirtyCount = dirtyList.length; @@ -62,9 +57,11 @@ export function registerTokenAudit(server: McpServer): void { const largeFiles: string[] = []; for (const f of dirtyList.slice(0, 30)) { - // Use shell-safe quoting instead of interpolation - const wc = run(`wc -l < '${shellEscape(f)}' 2>/dev/null`); - const lines = parseInt(wc) || 0; + let lines = 0; + try { + const content = readFileSync(join(PROJECT_DIR, f), "utf-8"); + lines = content.split("\n").length; + } catch { /* skip unreadable files */ } estimatedContextTokens += lines * AVG_LINE_BYTES * AVG_TOKENS_PER_BYTE; if (lines > 500) { largeFiles.push(`${f} (${lines} lines)`); @@ -80,8 +77,8 @@ export function registerTokenAudit(server: McpServer): void { // 3. CLAUDE.md bloat check const claudeMd = readIfExists("CLAUDE.md", 1); if (claudeMd !== null) { - const stat = run(`wc -c < '${shellEscape("CLAUDE.md")}' 2>/dev/null`); - const bytes = parseInt(stat) || 0; + let bytes = 0; + try { bytes = statSync(join(PROJECT_DIR, "CLAUDE.md")).size; } catch { /* ignore */ } if (bytes > 5120) { patterns.push(`CLAUDE.md is ${(bytes / 1024).toFixed(1)}KB — injected every session, burns tokens on paste`); recommendations.push("Trim CLAUDE.md to essentials (<5KB). Move reference docs to files read on-demand"); @@ -137,9 +134,16 @@ export function registerTokenAudit(server: McpServer): void { } // Read with size cap: take the tail if too large - const raw = stat.size <= MAX_TOOL_LOG_BYTES - ? readFileSync(toolLogPath, "utf-8") - : run(`tail -c ${MAX_TOOL_LOG_BYTES} '${shellEscape(toolLogPath)}'`); + let raw: string; + if (stat.size <= MAX_TOOL_LOG_BYTES) { + raw = readFileSync(toolLogPath, "utf-8"); + } else { + const fd = openSync(toolLogPath, "r"); + const buf = Buffer.alloc(MAX_TOOL_LOG_BYTES); + readSync(fd, buf, 0, MAX_TOOL_LOG_BYTES, stat.size - MAX_TOOL_LOG_BYTES); + closeSync(fd); + raw = buf.toString("utf-8"); + } const lines = raw.trim().split("\n").filter(Boolean); totalToolCalls = lines.length; diff --git a/src/tools/verify-completion.ts b/src/tools/verify-completion.ts index 732532f..67f4240 100644 --- a/src/tools/verify-completion.ts +++ b/src/tools/verify-completion.ts @@ -2,7 +2,8 @@ import { z } from "zod"; import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { run, getStatus } from "../lib/git.js"; import { PROJECT_DIR } from "../lib/files.js"; -import { existsSync } from "fs"; +import { existsSync, readFileSync } from "fs"; +import { execFileSync } from "child_process"; import { join } from "path"; /** Detect package manager from lockfiles */ @@ -34,11 +35,24 @@ function detectTestRunner(): string | null { /** Check if a build script exists in package.json */ function hasBuildScript(): boolean { try { - const pkg = JSON.parse(run("cat package.json 2>/dev/null")); + const pkg = JSON.parse(readFileSync(join(PROJECT_DIR, "package.json"), "utf-8")); return !!pkg?.scripts?.build; } catch { return false; } } +/** Run an arbitrary command (non-git) safely, returning stdout or empty string on error */ +function execCmd(cmd: string, args: string[], opts: { timeout?: number } = {}): string { + try { + return execFileSync(cmd, args, { + cwd: PROJECT_DIR, + encoding: "utf-8", + timeout: opts.timeout || 30000, + maxBuffer: 1024 * 1024, + stdio: ["pipe", "pipe", "pipe"], + }).trim(); + } catch { return ""; } +} + export function registerVerifyCompletion(server: McpServer): void { server.tool( "verify_completion", @@ -55,7 +69,9 @@ export function registerVerifyCompletion(server: McpServer): void { const checks: { name: string; passed: boolean; detail: string }[] = []; // 1. Type check (single invocation, extract both result and count) - const tscOutput = run(`${pm === "npx" ? "npx" : pm} tsc --noEmit 2>&1 | tail -20`); + const tscBin = pm === "npx" ? "npx" : pm; + const tscArgs = pm === "npx" ? ["tsc", "--noEmit"] : ["exec", "tsc", "--noEmit"]; + const tscOutput = execCmd(tscBin, tscArgs, { timeout: 60000 }); const errorLines = tscOutput.split("\n").filter(l => /error TS\d+/.test(l)); const typePassed = errorLines.length === 0; checks.push({ @@ -80,39 +96,45 @@ export function registerVerifyCompletion(server: McpServer): void { // 3. Tests if (!skip_tests) { const runner = detectTestRunner(); - const changedFiles = run("git diff --name-only HEAD~1 2>/dev/null").split("\n").filter(Boolean); + const changedFiles = run(["diff", "--name-only", "HEAD~1"]).split("\n").filter(Boolean); let testCmd = ""; + // Build test command args (run via execCmd, not git run) + let testBin = ""; + let testArgs: string[] = []; + if (runner === "playwright") { - const runnerCmd = `${pm === "npx" ? "npx" : `${pm} exec`} playwright test`; + testBin = pm === "npx" ? "npx" : pm; + const baseArgs = pm === "npx" ? ["playwright", "test"] : ["exec", "playwright", "test"]; if (test_scope && test_scope !== "all") { - testCmd = test_scope.endsWith(".spec.ts") || test_scope.endsWith(".test.ts") - ? `${runnerCmd} ${test_scope} --reporter=line 2>&1 | tail -20` - : `${runnerCmd} --grep "${test_scope}" --reporter=line 2>&1 | tail -20`; + testArgs = test_scope.endsWith(".spec.ts") || test_scope.endsWith(".test.ts") + ? [...baseArgs, test_scope, "--reporter=line"] + : [...baseArgs, "--grep", test_scope, "--reporter=line"]; } else { - // Auto-detect from changed files const changedTests = changedFiles.filter(f => /\.(spec|test)\.(ts|tsx|js)$/.test(f)).slice(0, 5); if (changedTests.length > 0) { - testCmd = `${runnerCmd} ${changedTests.join(" ")} --reporter=line 2>&1 | tail -20`; + testArgs = [...baseArgs, ...changedTests, "--reporter=line"]; } } } else if (runner === "vitest" || runner === "jest") { - const runnerCmd = `${pm === "npx" ? "npx" : `${pm} exec`} ${runner}`; + testBin = pm === "npx" ? "npx" : pm; + const baseArgs = pm === "npx" ? [runner, "--run"] : ["exec", runner, "--run"]; if (test_scope && test_scope !== "all") { - testCmd = `${runnerCmd} --run ${test_scope} 2>&1 | tail -20`; + testArgs = [...baseArgs, test_scope]; } else { const changedTests = changedFiles.filter(f => /\.(spec|test)\.(ts|tsx|js)$/.test(f)).slice(0, 5); if (changedTests.length > 0) { - testCmd = `${runnerCmd} --run ${changedTests.join(" ")} 2>&1 | tail -20`; + testArgs = [...baseArgs, ...changedTests]; } } } else if (test_scope) { - // No recognized runner but scope given — try npm test - testCmd = `${pm} test 2>&1 | tail -20`; + testBin = pm; + testArgs = ["test"]; } - if (testCmd) { - const testResult = run(testCmd, { timeout: 120000 }); + if (testBin && testArgs.length > 0) { + const fullOutput = execCmd(testBin, testArgs, { timeout: 120000 }); + const testResult = fullOutput.split("\n").slice(-20).join("\n"); const testPassed = /pass/i.test(testResult) && !/fail/i.test(testResult); checks.push({ name: "Tests", @@ -130,7 +152,10 @@ export function registerVerifyCompletion(server: McpServer): void { // 4. Build check (only if build script exists and not skipped) if (!skip_build && hasBuildScript()) { - const buildCheck = run(`${pm === "npx" ? "npm run" : pm} build 2>&1 | tail -10`, { timeout: 60000 }); + const buildBin = pm === "npx" ? "npm" : pm; + const buildArgs = pm === "npx" ? ["run", "build"] : ["build"]; + const fullBuild = execCmd(buildBin, buildArgs, { timeout: 60000 }); + const buildCheck = fullBuild.split("\n").slice(-10).join("\n"); const buildPassed = !/\b[Ee]rror\b/.test(buildCheck) || /Successfully compiled/.test(buildCheck); checks.push({ name: "Build", From f83809dc51aeb852d23b6b3f6dffeead4871db46 Mon Sep 17 00:00:00 2001 From: Jack Felke Date: Mon, 9 Mar 2026 11:45:35 -0700 Subject: [PATCH 2/2] fix: remove unused imports and fix prefer-const lint error - Remove unused 'run' import from session-handoff.ts - Remove unused 'loadState' import from token-audit.ts - Remove unused 'existsSync' and 'join' imports from sequence-tasks.ts - Remove unused 'testCmd' variable from verify-completion.ts (prefer-const error) Fixes CI lint failure on PR #185 --- src/tools/sequence-tasks.ts | 3 +-- src/tools/session-handoff.ts | 2 +- src/tools/token-audit.ts | 2 +- src/tools/verify-completion.ts | 2 -- 4 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/tools/sequence-tasks.ts b/src/tools/sequence-tasks.ts index 31c6871..38d0491 100644 --- a/src/tools/sequence-tasks.ts +++ b/src/tools/sequence-tasks.ts @@ -4,8 +4,7 @@ import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { run } from "../lib/git.js"; import { now } from "../lib/state.js"; import { PROJECT_DIR } from "../lib/files.js"; -import { existsSync } from "fs"; -import { join, resolve } from "path"; +import { resolve } from "path"; type Cat = "schema" | "config" | "api" | "ui" | "test" | "other"; diff --git a/src/tools/session-handoff.ts b/src/tools/session-handoff.ts index bfc9abe..f5e06bc 100644 --- a/src/tools/session-handoff.ts +++ b/src/tools/session-handoff.ts @@ -3,7 +3,7 @@ import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { existsSync, readFileSync } from "fs"; import { execFileSync } from "child_process"; import { join } from "path"; -import { run, getBranch, getRecentCommits, getStatus } from "../lib/git.js"; +import { getBranch, getRecentCommits, getStatus } from "../lib/git.js"; import { readIfExists, findWorkspaceDocs } from "../lib/files.js"; import { STATE_DIR, now } from "../lib/state.js"; diff --git a/src/tools/token-audit.ts b/src/tools/token-audit.ts index 35dd5a7..c14b399 100644 --- a/src/tools/token-audit.ts +++ b/src/tools/token-audit.ts @@ -3,7 +3,7 @@ import { z } from "zod"; import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { run } from "../lib/git.js"; import { readIfExists, findWorkspaceDocs, PROJECT_DIR } from "../lib/files.js"; -import { loadState, saveState, now, STATE_DIR } from "../lib/state.js"; +import { saveState, now, STATE_DIR } from "../lib/state.js"; import { readFileSync, existsSync, statSync, openSync, readSync, closeSync } from "fs"; import { join } from "path"; diff --git a/src/tools/verify-completion.ts b/src/tools/verify-completion.ts index 67f4240..7524691 100644 --- a/src/tools/verify-completion.ts +++ b/src/tools/verify-completion.ts @@ -97,8 +97,6 @@ export function registerVerifyCompletion(server: McpServer): void { if (!skip_tests) { const runner = detectTestRunner(); const changedFiles = run(["diff", "--name-only", "HEAD~1"]).split("\n").filter(Boolean); - let testCmd = ""; - // Build test command args (run via execCmd, not git run) let testBin = ""; let testArgs: string[] = [];