From 7ed62e849a74a9357604afcde11817b86b66cf04 Mon Sep 17 00:00:00 2001 From: Jack Felke Date: Thu, 19 Mar 2026 21:16:28 -0700 Subject: [PATCH] fix(#302): remove shell syntax from run() calls in 4 more tools Replace shell syntax (pipes, redirects, 2>/dev/null) passed to run() with proper array args and Node.js equivalents: - token-audit: use Node fs for line/byte counting and tail reads - enrich-agent-task: filter git ls-files output in JS instead of grep/head pipes - clarify-intent: use execFileSync for tsc, Node fs for finding test files - audit-workspace: use array args for git diff, Node fs for counting test files scan-sessions.ts was already clean (uses fs directly, not run()). Closes the remaining files from #302. --- src/tools/audit-workspace.ts | 23 +++++++++++++-- src/tools/clarify-intent.ts | 43 +++++++++++++++++++++++++--- src/tools/enrich-agent-task.ts | 49 +++++++++++++++++++++----------- src/tools/token-audit.ts | 51 ++++++++++++++++++++++++++-------- 4 files changed, 132 insertions(+), 34 deletions(-) diff --git a/src/tools/audit-workspace.ts b/src/tools/audit-workspace.ts index d4306bd..7426bcd 100644 --- a/src/tools/audit-workspace.ts +++ b/src/tools/audit-workspace.ts @@ -1,6 +1,8 @@ import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { run } from "../lib/git.js"; -import { readIfExists, findWorkspaceDocs } from "../lib/files.js"; +import { readIfExists, findWorkspaceDocs, PROJECT_DIR } from "../lib/files.js"; +import { readdirSync } from "fs"; +import { join } from "path"; /** Extract top-level work areas from file paths generically */ function detectWorkAreas(files: string[]): Set { @@ -36,7 +38,8 @@ 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 diffResult = run(["diff", "--name-only", "HEAD~10"]); + const recentFiles = diffResult.startsWith("[") ? [] : diffResult.split("\n").filter(Boolean); const sections: string[] = []; // Doc freshness @@ -75,7 +78,21 @@ 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; + // Count test files using Node.js fs + let testFilesCount = 0; + const countTestFiles = (dir: string, depth = 0): void => { + if (depth > 4) return; + try { + for (const entry of readdirSync(dir, { withFileTypes: true })) { + if (entry.isDirectory() && !entry.name.startsWith(".") && entry.name !== "node_modules") { + countTestFiles(join(dir, entry.name), depth + 1); + } else if (entry.isFile() && /\.(spec|test)\.(ts|tsx|js|jsx)$/.test(entry.name)) { + testFilesCount++; + } + } + } catch { /* dir may not exist */ } + }; + countTestFiles(join(PROJECT_DIR, "tests")); 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/clarify-intent.ts b/src/tools/clarify-intent.ts index 32efa3a..a46d89a 100644 --- a/src/tools/clarify-intent.ts +++ b/src/tools/clarify-intent.ts @@ -4,10 +4,31 @@ import { run, getBranch, getStatus, getRecentCommits, getDiffFiles, getStagedFil import { findWorkspaceDocs, PROJECT_DIR } from "../lib/files.js"; import { searchSemantic } from "../lib/timeline-db.js"; import { getRelatedProjects } from "../lib/config.js"; -import { existsSync, readFileSync } from "fs"; +import { existsSync, readFileSync, readdirSync, statSync } from "fs"; +import { execFileSync } from "child_process"; import { join, basename, resolve } from "path"; import { loadAllContracts, searchContracts, formatContracts } from "../lib/contracts.js"; +/** Recursively find test files using Node.js fs */ +function findTestFiles(dir: string, maxDepth: number, limit: number, depth = 0): string[] { + const results: string[] = []; + if (depth > maxDepth || results.length >= limit) return results; + try { + const entries = readdirSync(dir, { withFileTypes: true }); + for (const entry of entries) { + if (results.length >= limit) break; + const fullPath = join(dir, entry.name); + if (entry.isDirectory() && !entry.name.startsWith(".") && entry.name !== "node_modules") { + results.push(...findTestFiles(fullPath, maxDepth, limit - results.length, depth + 1)); + } else if (entry.isFile() && /\.spec\.ts$/.test(entry.name)) { + // Return relative to PROJECT_DIR + results.push(fullPath.replace(PROJECT_DIR + "/", "")); + } + } + } catch { /* directory may not exist */ } + return results; +} + /** Parse test failures from common report formats without fragile shell pipelines */ function getTestFailures(): string { // Try playwright JSON report @@ -152,10 +173,24 @@ export function registerClarifyIntent(server: McpServer): void { let hasTestFailures = false; if (!area || area.includes("test") || area.includes("fix") || area.includes("ui") || area.includes("api")) { - const typeErrors = run("pnpm tsc --noEmit 2>&1 | grep -c 'error TS' || echo '0'"); - hasTypeErrors = parseInt(typeErrors, 10) > 0; + let typeErrorCount = 0; + try { + execFileSync("pnpm", ["tsc", "--noEmit"], { + cwd: PROJECT_DIR, + encoding: "utf-8", + timeout: 30000, + stdio: ["pipe", "pipe", "pipe"], + }); + } catch (e: any) { + const output = (e.stdout || "") + (e.stderr || ""); + const matches = output.match(/error TS/g); + typeErrorCount = matches ? matches.length : 0; + } + const typeErrors = String(typeErrorCount); + hasTypeErrors = typeErrorCount > 0; - const testFiles = run("find tests -name '*.spec.ts' -maxdepth 4 2>/dev/null | head -20"); + // Find test files using Node.js fs instead of shell find + const testFiles = findTestFiles(join(PROJECT_DIR, "tests"), 4, 20).join("\n"); const failingTests = getTestFailures(); hasTestFailures = failingTests !== "all passing" && failingTests !== "no test report found"; diff --git a/src/tools/enrich-agent-task.ts b/src/tools/enrich-agent-task.ts index 236edfa..ac63685 100644 --- a/src/tools/enrich-agent-task.ts +++ b/src/tools/enrich-agent-task.ts @@ -8,11 +8,6 @@ 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 { - return s.replace(/[^a-zA-Z0-9_\-./]/g, ""); -} - /** Detect package manager from lockfiles */ function detectPackageManager(): string { if (existsSync(join(PROJECT_DIR, "pnpm-lock.yaml"))) return "pnpm"; @@ -25,16 +20,23 @@ function detectPackageManager(): string { function findAreaFiles(area: string): string { if (!area) return getDiffFiles("HEAD~3"); - const safeArea = shellEscape(area); + // Get all tracked files and filter in JS + const allFiles = run(["ls-files"]); + if (allFiles.startsWith("[")) return getDiffFiles("HEAD~3"); + + const fileList = allFiles.split("\n").filter(Boolean); + const areaLower = area.toLowerCase(); - // If area looks like a path, search directly + let matches: string[]; if (area.includes("/")) { - return run(`git ls-files -- '${safeArea}*' 2>/dev/null | head -20`); + // If area looks like a path, match prefix + matches = fileList.filter(f => f.startsWith(area)); + } else { + // Search for area keyword in file paths + matches = fileList.filter(f => f.toLowerCase().includes(areaLower)); } - // 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; + if (matches.length > 0) return matches.slice(0, 20).join("\n"); // Fallback to recently changed files return getDiffFiles("HEAD~3"); @@ -42,18 +44,33 @@ 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 allFiles = run(["ls-files"]); + if (allFiles.startsWith("[")) return ""; + + const testPattern = /\.(spec|test)\.(ts|tsx|js|jsx)$/; + const testFiles = allFiles.split("\n").filter(f => testPattern.test(f)); + + if (!area) return testFiles.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 areaKeyword = area.split(/\s+/)[0].toLowerCase(); + const areaTests = testFiles.filter(f => f.toLowerCase().includes(areaKeyword)); + if (areaTests.length > 0) return areaTests.slice(0, 10).join("\n"); + + return 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 fullPath = join(PROJECT_DIR, firstFile); + const content = readFileSync(fullPath, "utf-8"); + const lines = content.split("\n").slice(0, 30); + return lines.join("\n"); + } catch { + return "could not read file"; + } } // --------------------------------------------------------------------------- diff --git a/src/tools/token-audit.ts b/src/tools/token-audit.ts index b7aad2c..1465666 100644 --- a/src/tools/token-audit.ts +++ b/src/tools/token-audit.ts @@ -5,11 +5,43 @@ 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 { execFileSync } from "child_process"; import { join } from "path"; -/** Shell-escape a filename for safe interpolation */ -function shellEscape(s: string): string { - return s.replace(/'/g, "'\\''"); +/** Safe line count using Node.js (no shell needed) */ +function countLines(filePath: string): number { + try { + const fullPath = join(PROJECT_DIR, filePath); + const content = readFileSync(fullPath, "utf-8"); + return content.split("\n").length; + } catch { + return 0; + } +} + +/** Safe byte count using Node.js (no shell needed) */ +function countBytes(filePath: string): number { + try { + const fullPath = join(PROJECT_DIR, filePath); + return statSync(fullPath).size; + } catch { + return 0; + } +} + +/** Read tail of a file up to maxBytes */ +function readTail(filePath: string, maxBytes: number): string { + try { + const stat = statSync(filePath); + const fd = require("fs").openSync(filePath, "r"); + const start = Math.max(0, stat.size - maxBytes); + const buf = Buffer.alloc(Math.min(maxBytes, stat.size)); + require("fs").readSync(fd, buf, 0, buf.length, start); + require("fs").closeSync(fd); + return buf.toString("utf-8"); + } catch { + return ""; + } } /** @@ -39,8 +71,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 +94,7 @@ 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; + const lines = countLines(f); estimatedContextTokens += lines * AVG_LINE_BYTES * AVG_TOKENS_PER_BYTE; if (lines > 500) { largeFiles.push(`${f} (${lines} lines)`); @@ -80,8 +110,7 @@ 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; + const bytes = countBytes("CLAUDE.md"); 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"); @@ -139,7 +168,7 @@ 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)}'`); + : readTail(toolLogPath, MAX_TOOL_LOG_BYTES); const lines = raw.trim().split("\n").filter(Boolean); totalToolCalls = lines.length;