Skip to content

Commit 0cec6fe

Browse files
TerminalGravityTerminalGravity
committed
fix: replace shell syntax in run() calls across 6 tools (#172)
Tools fixed: - verify-completion: use fs.readFileSync for package.json, shellRun() for non-git commands (tsc, test runners, build), array args for git diff - token-audit: use fs for line/byte counting, Node.js fd ops for tail, array args for git diff - session-handoff: use execFileSync for 'which' and 'gh' instead of run() with shell operators - audit-workspace: array args for git diff, Node.js fs for test file counting instead of find|wc - scope-work: filter git ls-files in JS instead of shell grep pipes, remove unused shellEscape - enrich-agent-task: filter git ls-files in JS, use fs.readFileSync for file head instead of shell head command - sharpen-followup: array args for all git commands, remove 2>/dev/null - sequence-tasks: array args for git ls-files, JS slice instead of shell head All non-git shell commands now use either Node.js fs APIs or explicit sh -c invocation via shellRun(). git commands use array args with run(). Closes #172
1 parent b0495e7 commit 0cec6fe

8 files changed

Lines changed: 119 additions & 57 deletions

‎src/tools/audit-workspace.ts‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js";
22
import { run } from "../lib/git.js";
3-
import { readIfExists, findWorkspaceDocs } from "../lib/files.js";
3+
import { readIfExists, findWorkspaceDocs, PROJECT_DIR } from "../lib/files.js";
4+
import { readdirSync, statSync } from "fs";
5+
import { join } from "path";
46

57
/** Extract top-level work areas from file paths generically */
68
function detectWorkAreas(files: string[]): Set<string> {
@@ -36,7 +38,8 @@ export function registerAuditWorkspace(server: McpServer): void {
3638
{},
3739
async () => {
3840
const docs = findWorkspaceDocs();
39-
const recentFiles = run("git diff --name-only HEAD~10 2>/dev/null || echo ''").split("\n").filter(Boolean);
41+
const diffResult = run(["diff", "--name-only", "HEAD~10"]);
42+
const recentFiles = diffResult.startsWith("[") ? [] : diffResult.split("\n").filter(Boolean);
4043
const sections: string[] = [];
4144

4245
// Doc freshness
@@ -75,7 +78,20 @@ export function registerAuditWorkspace(server: McpServer): void {
7578
// Check for gap trackers or similar tracking docs
7679
const trackingDocs = Object.entries(docs).filter(([n]) => /gap|track|progress/i.test(n));
7780
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;
81+
// Count test files using Node.js fs instead of shell find|wc
82+
let testFilesCount = 0;
83+
try {
84+
const testsDir = join(PROJECT_DIR, "tests");
85+
const countTests = (dir: string): number => {
86+
let count = 0;
87+
for (const entry of readdirSync(dir, { withFileTypes: true })) {
88+
if (entry.isDirectory()) count += countTests(join(dir, entry.name));
89+
else if (/\.(spec|test)\.(ts|tsx|js|jsx)$/.test(entry.name)) count++;
90+
}
91+
return count;
92+
};
93+
testFilesCount = countTests(testsDir);
94+
} catch { /* tests dir may not exist */ }
7995
sections.push(`## Tracking Docs\n${trackingDocs.map(([n]) => {
8096
const age = docStatus.find(d => d.name === n)?.ageHours ?? "?";
8197
return `- .claude/${n} — last updated ${age}h ago`;

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

Lines changed: 26 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -25,35 +25,47 @@ function detectPackageManager(): string {
2525
function findAreaFiles(area: string): string {
2626
if (!area) return getDiffFiles("HEAD~3");
2727

28-
const safeArea = shellEscape(area);
28+
const allFiles = run(["ls-files"]);
29+
if (allFiles.startsWith("[")) return getDiffFiles("HEAD~3");
2930

30-
// If area looks like a path, search directly
31+
const fileList = allFiles.split("\n").filter(Boolean);
32+
33+
// If area looks like a path, filter by prefix
3134
if (area.includes("/")) {
32-
return run(`git ls-files -- '${safeArea}*' 2>/dev/null | head -20`);
35+
const safeArea = shellEscape(area);
36+
const matched = fileList.filter(f => f.startsWith(safeArea)).slice(0, 20);
37+
return matched.length > 0 ? matched.join("\n") : getDiffFiles("HEAD~3");
3338
}
3439

35-
// 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;
38-
39-
// Fallback to recently changed files
40-
return getDiffFiles("HEAD~3");
40+
// Search for area keyword in file paths
41+
const safeArea = shellEscape(area).toLowerCase();
42+
const matched = fileList.filter(f => f.toLowerCase().includes(safeArea)).slice(0, 20);
43+
return matched.length > 0 ? matched.join("\n") : getDiffFiles("HEAD~3");
4144
}
4245

4346
/** Find related test files for an area */
4447
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");
48+
const allFiles = run(["ls-files"]);
49+
if (allFiles.startsWith("[")) return "";
50+
51+
const testPattern = /\.(spec|test)\.(ts|tsx|js|jsx)$/;
52+
const testFiles = allFiles.split("\n").filter(f => testPattern.test(f));
4653

47-
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");
54+
if (!area) return testFiles.slice(0, 10).join("\n");
55+
56+
const safeArea = shellEscape(area.split(/\s+/)[0]).toLowerCase();
57+
const areaTests = testFiles.filter(f => f.toLowerCase().includes(safeArea)).slice(0, 10);
58+
return areaTests.length > 0 ? areaTests.join("\n") : testFiles.slice(0, 10).join("\n");
5059
}
5160

5261
/** Get an example pattern from the first matching file */
5362
function getExamplePattern(files: string): string {
5463
const firstFile = files.split("\n").filter(Boolean)[0];
5564
if (!firstFile) return "no pattern available";
56-
return run(`head -30 '${shellEscape(firstFile)}' 2>/dev/null || echo 'could not read file'`);
65+
try {
66+
const content = readFileSync(join(PROJECT_DIR, firstFile), "utf-8");
67+
return content.split("\n").slice(0, 30).join("\n");
68+
} catch { return "could not read file"; }
5769
}
5870

5971
// ---------------------------------------------------------------------------

‎src/tools/scope-work.ts‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,6 @@ const STOP_WORDS = new Set([
1717
"like", "some", "each", "only", "need", "want", "please", "update", "change",
1818
]);
1919

20-
/** Shell-escape a string for use inside single quotes */
21-
function shellEscape(s: string): string {
22-
return s.replace(/'/g, "'\\''");
23-
}
24-
2520
/** Safely parse git porcelain status lines */
2621
function parsePortelainFiles(porcelain: string): string[] {
2722
if (!porcelain.trim()) return [];
@@ -127,8 +122,13 @@ export function registerScopeWork(server: McpServer): void {
127122
.filter((k) => k.length > 2)
128123
.slice(0, 5);
129124
if (grepTerms.length > 0) {
130-
const pattern = shellEscape(grepTerms.join("|"));
131-
matchedFiles = run(`git ls-files | head -500 | grep -iE '${pattern}' | head -30`);
125+
// Filter git-tracked files in JS instead of shell pipes
126+
const allTracked = run(["ls-files"]);
127+
if (!allTracked.startsWith("[")) {
128+
const re = new RegExp(grepTerms.join("|"), "i");
129+
matchedFiles = allTracked.split("\n").filter(Boolean).slice(0, 500)
130+
.filter(f => re.test(f)).slice(0, 30).join("\n");
131+
}
132132
}
133133

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

‎src/tools/sequence-tasks.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,8 @@ 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 allFiles = run(["ls-files"]);
94+
const gitFiles = allFiles.startsWith("[") ? "" : allFiles.split("\n").slice(0, 1000).join("\n");
9495
const knownDirs = new Set<string>();
9596
for (const f of gitFiles.split("\n").filter(Boolean)) {
9697
const parts = f.split("/");

‎src/tools/session-handoff.ts‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,18 @@
11
import { z } from "zod";
22
import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js";
33
import { existsSync, readFileSync } from "fs";
4+
import { execFileSync } from "child_process";
45
import { join } from "path";
56
import { run, getBranch, getRecentCommits, getStatus } from "../lib/git.js";
67
import { readIfExists, findWorkspaceDocs } from "../lib/files.js";
78
import { STATE_DIR, now } from "../lib/state.js";
89

910
/** Check if a CLI tool is available */
1011
function hasCommand(cmd: string): boolean {
11-
const result = run(`command -v ${cmd} 2>/dev/null`);
12-
return !!result && !result.startsWith("[command failed");
12+
try {
13+
execFileSync("which", [cmd], { encoding: "utf-8", stdio: ["pipe", "pipe", "pipe"] });
14+
return true;
15+
} catch { return false; }
1316
}
1417

1518
export function registerSessionHandoff(server: McpServer): void {
@@ -44,7 +47,12 @@ export function registerSessionHandoff(server: McpServer): void {
4447

4548
// Only try gh if it exists
4649
if (hasCommand("gh")) {
47-
const openPRs = run("gh pr list --state open --json number,title,headRefName 2>/dev/null || echo '[]'");
50+
let openPRs = "[]";
51+
try {
52+
openPRs = execFileSync("gh", ["pr", "list", "--state", "open", "--json", "number,title,headRefName"], {
53+
encoding: "utf-8", timeout: 10000, stdio: ["pipe", "pipe", "pipe"],
54+
}).trim();
55+
} catch { /* gh not available or not in a repo */ }
4856
if (openPRs && openPRs !== "[]") {
4957
sections.push(`## Open PRs\n\`\`\`json\n${openPRs}\n\`\`\``);
5058
}

‎src/tools/sharpen-followup.ts‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -27,15 +27,15 @@ function parsePortelainFiles(output: string): string[] {
2727
/** Get recently changed files, safe for first commit / shallow clones */
2828
function getRecentChangedFiles(): string[] {
2929
// Try HEAD~1..HEAD, fall back to just staged, then unstaged
30-
const commands = [
31-
"git diff --name-only HEAD~1 HEAD 2>/dev/null",
32-
"git diff --name-only --cached 2>/dev/null",
33-
"git diff --name-only 2>/dev/null",
30+
const commands: string[][] = [
31+
["diff", "--name-only", "HEAD~1", "HEAD"],
32+
["diff", "--name-only", "--cached"],
33+
["diff", "--name-only"],
3434
];
3535
const results = new Set<string>();
36-
for (const cmd of commands) {
37-
const out = run(cmd);
38-
if (out) out.split("\n").filter(Boolean).forEach((f) => results.add(f));
36+
for (const args of commands) {
37+
const out = run(args);
38+
if (out && !out.startsWith("[")) out.split("\n").filter(Boolean).forEach((f) => results.add(f));
3939
if (results.size > 0) break; // first successful source is enough
4040
}
4141
return [...results];
@@ -87,7 +87,7 @@ export function registerSharpenFollowup(server: McpServer): void {
8787
// Gather context to resolve ambiguity
8888
const contextFiles: string[] = [...(previous_files ?? [])];
8989
const recentChanged = getRecentChangedFiles();
90-
const porcelainOutput = run("git status --porcelain 2>/dev/null");
90+
const porcelainOutput = run(["status", "--porcelain"]);
9191
const untrackedOrModified = parsePortelainFiles(porcelainOutput);
9292

9393
const allKnownFiles = [...new Set([...contextFiles, ...recentChanged, ...untrackedOrModified])].filter(Boolean);

‎src/tools/token-audit.ts‎

Lines changed: 21 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,9 @@ import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js";
44
import { run } 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";
7-
import { readFileSync, existsSync, statSync } from "fs";
7+
import { readFileSync, existsSync, statSync, openSync, readSync, closeSync } from "fs";
88
import { join } from "path";
99

10-
/** Shell-escape a filename for safe interpolation */
11-
function shellEscape(s: string): string {
12-
return s.replace(/'/g, "'\\''");
13-
}
14-
1510
/**
1611
* Grade thresholds rationale:
1712
* - A (0-10): Minimal waste — small diffs, targeted reads, lean context
@@ -39,8 +34,8 @@ export function registerTokenAudit(server: McpServer): void {
3934
let wasteScore = 0;
4035

4136
// 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");
37+
const diffStat = run(["diff", "--stat", "--no-color"]);
38+
const dirtyFiles = run(["diff", "--name-only"]);
4439
const dirtyList = dirtyFiles.split("\n").filter(Boolean);
4540
const dirtyCount = dirtyList.length;
4641

@@ -62,9 +57,12 @@ export function registerTokenAudit(server: McpServer): void {
6257
const largeFiles: string[] = [];
6358

6459
for (const f of dirtyList.slice(0, 30)) {
65-
// Use shell-safe quoting instead of interpolation
66-
const wc = run(`wc -l < '${shellEscape(f)}' 2>/dev/null`);
67-
const lines = parseInt(wc) || 0;
60+
// Count lines using Node.js fs instead of shell wc
61+
let lines = 0;
62+
try {
63+
const content = readFileSync(join(PROJECT_DIR, f), "utf-8");
64+
lines = content.split("\n").length;
65+
} catch { /* file may not exist or be binary */ }
6866
estimatedContextTokens += lines * AVG_LINE_BYTES * AVG_TOKENS_PER_BYTE;
6967
if (lines > 500) {
7068
largeFiles.push(`${f} (${lines} lines)`);
@@ -80,8 +78,8 @@ export function registerTokenAudit(server: McpServer): void {
8078
// 3. CLAUDE.md bloat check
8179
const claudeMd = readIfExists("CLAUDE.md", 1);
8280
if (claudeMd !== null) {
83-
const stat = run(`wc -c < '${shellEscape("CLAUDE.md")}' 2>/dev/null`);
84-
const bytes = parseInt(stat) || 0;
81+
let bytes = 0;
82+
try { bytes = statSync(join(PROJECT_DIR, "CLAUDE.md")).size; } catch { /* ignore */ }
8583
if (bytes > 5120) {
8684
patterns.push(`CLAUDE.md is ${(bytes / 1024).toFixed(1)}KB — injected every session, burns tokens on paste`);
8785
recommendations.push("Trim CLAUDE.md to essentials (<5KB). Move reference docs to files read on-demand");
@@ -139,7 +137,16 @@ export function registerTokenAudit(server: McpServer): void {
139137
// Read with size cap: take the tail if too large
140138
const raw = stat.size <= MAX_TOOL_LOG_BYTES
141139
? readFileSync(toolLogPath, "utf-8")
142-
: run(`tail -c ${MAX_TOOL_LOG_BYTES} '${shellEscape(toolLogPath)}'`);
140+
: (() => {
141+
// Read the tail of the file using Node.js fs
142+
const fd = openSync(toolLogPath, "r");
143+
const buf = Buffer.alloc(MAX_TOOL_LOG_BYTES);
144+
const fileSize = statSync(toolLogPath).size;
145+
const start = Math.max(0, fileSize - MAX_TOOL_LOG_BYTES);
146+
readSync(fd, buf, 0, MAX_TOOL_LOG_BYTES, start);
147+
closeSync(fd);
148+
return buf.toString("utf-8");
149+
})();
143150

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

‎src/tools/verify-completion.ts‎

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,8 @@ import { z } from "zod";
22
import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js";
33
import { run, getStatus } from "../lib/git.js";
44
import { PROJECT_DIR } from "../lib/files.js";
5-
import { existsSync } from "fs";
5+
import { existsSync, readFileSync } from "fs";
6+
import { execFileSync } from "child_process";
67
import { join } from "path";
78

89
/** Detect package manager from lockfiles */
@@ -34,11 +35,28 @@ function detectTestRunner(): string | null {
3435
/** Check if a build script exists in package.json */
3536
function hasBuildScript(): boolean {
3637
try {
37-
const pkg = JSON.parse(run("cat package.json 2>/dev/null"));
38+
const raw = readFileSync(join(PROJECT_DIR, "package.json"), "utf-8");
39+
const pkg = JSON.parse(raw);
3840
return !!pkg?.scripts?.build;
3941
} catch { return false; }
4042
}
4143

44+
/** Run a shell command string (for non-git commands that need pipes/shell features) */
45+
function shellRun(cmd: string, opts: { timeout?: number } = {}): string {
46+
try {
47+
return execFileSync("sh", ["-c", cmd], {
48+
cwd: PROJECT_DIR,
49+
encoding: "utf-8",
50+
timeout: opts.timeout || 10000,
51+
maxBuffer: 1024 * 1024,
52+
stdio: ["pipe", "pipe", "pipe"],
53+
}).trim();
54+
} catch (e: any) {
55+
if (e.killed) return `[timed out]`;
56+
return e.stdout?.trim() || e.stderr?.trim() || `[command failed: ${cmd}]`;
57+
}
58+
}
59+
4260
export function registerVerifyCompletion(server: McpServer): void {
4361
server.tool(
4462
"verify_completion",
@@ -55,7 +73,7 @@ export function registerVerifyCompletion(server: McpServer): void {
5573
const checks: { name: string; passed: boolean; detail: string }[] = [];
5674

5775
// 1. Type check (single invocation, extract both result and count)
58-
const tscOutput = run(`${pm === "npx" ? "npx" : pm} tsc --noEmit 2>&1 | tail -20`);
76+
const tscOutput = shellRun(`${pm === "npx" ? "npx" : pm} tsc --noEmit 2>&1 | tail -20`);
5977
const errorLines = tscOutput.split("\n").filter(l => /error TS\d+/.test(l));
6078
const typePassed = errorLines.length === 0;
6179
checks.push({
@@ -80,15 +98,15 @@ export function registerVerifyCompletion(server: McpServer): void {
8098
// 3. Tests
8199
if (!skip_tests) {
82100
const runner = detectTestRunner();
83-
const changedFiles = run("git diff --name-only HEAD~1 2>/dev/null").split("\n").filter(Boolean);
101+
const changedFiles = run(["diff", "--name-only", "HEAD~1"]).split("\n").filter(Boolean);
84102
let testCmd = "";
85103

86104
if (runner === "playwright") {
87105
const runnerCmd = `${pm === "npx" ? "npx" : `${pm} exec`} playwright test`;
88106
if (test_scope && test_scope !== "all") {
89107
testCmd = test_scope.endsWith(".spec.ts") || test_scope.endsWith(".test.ts")
90108
? `${runnerCmd} ${test_scope} --reporter=line 2>&1 | tail -20`
91-
: `${runnerCmd} --grep "${test_scope}" --reporter=line 2>&1 | tail -20`;
109+
: `${runnerCmd} --grep '${test_scope}' --reporter=line 2>&1 | tail -20`;
92110
} else {
93111
// Auto-detect from changed files
94112
const changedTests = changedFiles.filter(f => /\.(spec|test)\.(ts|tsx|js)$/.test(f)).slice(0, 5);
@@ -112,7 +130,7 @@ export function registerVerifyCompletion(server: McpServer): void {
112130
}
113131

114132
if (testCmd) {
115-
const testResult = run(testCmd, { timeout: 120000 });
133+
const testResult = shellRun(testCmd, { timeout: 120000 });
116134
const testPassed = /pass/i.test(testResult) && !/fail/i.test(testResult);
117135
checks.push({
118136
name: "Tests",
@@ -130,7 +148,7 @@ export function registerVerifyCompletion(server: McpServer): void {
130148

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

0 commit comments

Comments
 (0)