From 8c4f2964c56997f6865cb23004004f7def6aa20b Mon Sep 17 00:00:00 2001 From: Jack Felke Date: Wed, 4 Mar 2026 18:17:01 -0700 Subject: [PATCH] fix: replace shell syntax in run() calls with proper APIs Fixes remaining tools from #89 that passed shell operators (pipes, redirects, ||, &&) to run() which uses execFileSync (no shell). Changes: - Add shell() helper in git.ts for commands needing shell features - Add getTrackedFiles() and getTrackedFilesInPath() to replace 'git ls-files | grep | head' patterns with Node.js filtering - token-audit: replace wc/tail with fs.readFileSync/statSync/readSync - verify-completion: use shell() for tsc/build/test commands with pipes - enrich-agent-task: use getTrackedFiles() instead of shell pipes - scope-work: use getTrackedFiles() with RegExp filtering - session-handoff: use shell() for 'command -v' and 'gh pr list' Closes #89 --- src/lib/git.ts | 48 +++++++++++++++++++++++++++++++++- src/tools/enrich-agent-task.ts | 30 +++++++++++++-------- src/tools/scope-work.ts | 6 ++--- src/tools/session-handoff.ts | 6 ++--- src/tools/token-audit.ts | 36 +++++++++++++------------ src/tools/verify-completion.ts | 15 ++++++----- 6 files changed, 100 insertions(+), 41 deletions(-) diff --git a/src/lib/git.ts b/src/lib/git.ts index a32ee3c..f38b248 100644 --- a/src/lib/git.ts +++ b/src/lib/git.ts @@ -1,4 +1,4 @@ -import { execFileSync } from "child_process"; +import { execFileSync, execSync } from "child_process"; import { PROJECT_DIR } from "./files.js"; import type { RunError } from "../types.js"; @@ -35,6 +35,52 @@ function gitCmd(cmdStr: string, opts?: { timeout?: number }): string { return run(cmdStr.split(/\s+/), opts); } +/** + * Run an arbitrary shell command (with shell: true). + * Use this for non-git commands or commands requiring pipes/redirects. + * Callers are responsible for sanitizing any interpolated values. + */ +export function shell(cmd: string, opts: { timeout?: number; cwd?: string } = {}): string { + try { + return execSync(cmd, { + cwd: opts.cwd || PROJECT_DIR, + encoding: "utf-8", + timeout: opts.timeout || 10000, + maxBuffer: 1024 * 1024, + stdio: ["pipe", "pipe", "pipe"], + }).trim(); + } catch (e: any) { + if (e.killed === true || e.signal === "SIGTERM") { + return `[timed out after ${opts.timeout || 10000}ms]`; + } + const output = e.stdout?.trim() || e.stderr?.trim(); + if (output) return output; + return `[command failed: ${cmd} (exit ${e.status ?? "?"})]`; + } +} + +/** + * Get git-tracked files, optionally filtered. Uses execFileSync (no shell). + * Replaces patterns like: run("git ls-files | grep ... | head ...") + */ +export function getTrackedFiles(opts: { pattern?: RegExp; limit?: number } = {}): string[] { + const raw = run(["ls-files"]); + if (raw.startsWith("[")) return []; + let files = raw.split("\n").filter(Boolean); + if (opts.pattern) files = files.filter(f => opts.pattern!.test(f)); + if (opts.limit) files = files.slice(0, opts.limit); + return files; +} + +/** + * Get git-tracked files matching a path prefix. + */ +export function getTrackedFilesInPath(prefix: string, limit = 20): string[] { + const result = run(["ls-files", "--", `${prefix}*`]); + if (result.startsWith("[")) return []; + return result.split("\n").filter(Boolean).slice(0, limit); +} + /** Get the current branch name. */ export function getBranch(): string { return run(["branch", "--show-current"]); diff --git a/src/tools/enrich-agent-task.ts b/src/tools/enrich-agent-task.ts index 236edfa..7dca2d8 100644 --- a/src/tools/enrich-agent-task.ts +++ b/src/tools/enrich-agent-task.ts @@ -1,6 +1,6 @@ import { z } from "zod"; import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; -import { run, getDiffFiles } from "../lib/git.js"; +import { run, getDiffFiles, getTrackedFiles, getTrackedFilesInPath } from "../lib/git.js"; import { PROJECT_DIR } from "../lib/files.js"; import { getConfig, type RelatedProject } from "../lib/config.js"; import { existsSync, readFileSync } from "fs"; @@ -25,16 +25,16 @@ function detectPackageManager(): string { function findAreaFiles(area: string): string { if (!area) return getDiffFiles("HEAD~3"); - const safeArea = shellEscape(area); - // If area looks like a path, search directly if (area.includes("/")) { - return run(`git ls-files -- '${safeArea}*' 2>/dev/null | head -20`); + return getTrackedFilesInPath(area, 20).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; + const safeArea = shellEscape(area); + const pattern = new RegExp(safeArea, "i"); + const files = getTrackedFiles({ pattern, limit: 20 }); + if (files.length > 0) return files.join("\n"); // Fallback to recently changed files return getDiffFiles("HEAD~3"); @@ -42,18 +42,26 @@ function findAreaFiles(area: string): string { /** 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 allTests = getTrackedFiles({ pattern: testPattern, limit: 100 }); + + if (!area) return allTests.slice(0, 10).join("\n"); - 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"); + const safeArea = shellEscape(area.split(/\s+/)[0]).toLowerCase(); + const areaTests = allTests.filter(f => f.toLowerCase().includes(safeArea)).slice(0, 10); + return (areaTests.length > 0 ? areaTests : allTests.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..ebc2335 100644 --- a/src/tools/scope-work.ts +++ b/src/tools/scope-work.ts @@ -1,7 +1,7 @@ // CATEGORY 1: scope_work — Plans import { z } from "zod"; import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; -import { run, getBranch, getRecentCommits, getStatus } from "../lib/git.js"; +import { run, getBranch, getRecentCommits, getStatus, getTrackedFiles } from "../lib/git.js"; import { readIfExists, findWorkspaceDocs, PROJECT_DIR } from "../lib/files.js"; import { searchSemantic } from "../lib/timeline-db.js"; import { getRelatedProjects } from "../lib/config.js"; @@ -127,8 +127,8 @@ 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 pattern = new RegExp(grepTerms.join("|"), "i"); + matchedFiles = getTrackedFiles({ pattern, limit: 30 }).join("\n"); } // Check which relevant dirs actually exist (with path traversal protection) diff --git a/src/tools/session-handoff.ts b/src/tools/session-handoff.ts index d199462..e75d5b0 100644 --- a/src/tools/session-handoff.ts +++ b/src/tools/session-handoff.ts @@ -2,13 +2,13 @@ import { z } from "zod"; import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { existsSync, readFileSync } from "fs"; import { join } from "path"; -import { run, getBranch, getRecentCommits, getStatus } from "../lib/git.js"; +import { run, getBranch, getRecentCommits, getStatus, shell } from "../lib/git.js"; import { readIfExists, findWorkspaceDocs } from "../lib/files.js"; 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`); + const result = shell(`command -v ${cmd} 2>/dev/null`); return !!result && !result.startsWith("[command failed"); } @@ -44,7 +44,7 @@ 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 '[]'"); + const openPRs = shell("gh pr list --state open --json number,title,headRefName 2>/dev/null || echo '[]'"); if (openPRs && openPRs !== "[]") { sections.push(`## Open PRs\n\`\`\`json\n${openPRs}\n\`\`\``); } 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..f3b24bf 100644 --- a/src/tools/verify-completion.ts +++ b/src/tools/verify-completion.ts @@ -1,8 +1,8 @@ import { z } from "zod"; import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; -import { run, getStatus } from "../lib/git.js"; +import { run, getStatus, shell } from "../lib/git.js"; import { PROJECT_DIR } from "../lib/files.js"; -import { existsSync } from "fs"; +import { existsSync, readFileSync } from "fs"; import { join } from "path"; /** Detect package manager from lockfiles */ @@ -34,7 +34,8 @@ 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 raw = readFileSync(join(PROJECT_DIR, "package.json"), "utf-8"); + const pkg = JSON.parse(raw); return !!pkg?.scripts?.build; } catch { return false; } } @@ -55,7 +56,7 @@ 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 tscOutput = shell(`${pm === "npx" ? "npx" : pm} tsc --noEmit 2>&1 | tail -20`); const errorLines = tscOutput.split("\n").filter(l => /error TS\d+/.test(l)); const typePassed = errorLines.length === 0; checks.push({ @@ -80,7 +81,7 @@ 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 = ""; if (runner === "playwright") { @@ -112,7 +113,7 @@ export function registerVerifyCompletion(server: McpServer): void { } if (testCmd) { - const testResult = run(testCmd, { timeout: 120000 }); + const testResult = shell(testCmd, { timeout: 120000 }); const testPassed = /pass/i.test(testResult) && !/fail/i.test(testResult); checks.push({ name: "Tests", @@ -130,7 +131,7 @@ 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 buildCheck = shell(`${pm === "npx" ? "npm run" : pm} build 2>&1 | tail -10`, { timeout: 60000 }); const buildPassed = !/\b[Ee]rror\b/.test(buildCheck) || /Successfully compiled/.test(buildCheck); checks.push({ name: "Build",