diff --git a/.changeset/shell-projection-subprocess-migration.md b/.changeset/shell-projection-subprocess-migration.md new file mode 100644 index 000000000..d4ccfc33e --- /dev/null +++ b/.changeset/shell-projection-subprocess-migration.md @@ -0,0 +1,5 @@ +--- +type: Changed +pr: 3476 +--- +Migrate all subprocess call sites to the shell-command-projection seam (`execGit`, `execNpm`, `execTool`, `probeTty`). Removes scattered platform-conditional logic and normalizes subprocess error handling. Local `execGit` wrapper removed from `core.cjs`; `verify.cjs` and `worktree-safety.cjs` migrated to the seam's `(args, opts)` signature. See #3466. diff --git a/get-shit-done/bin/check-latest-version.cjs b/get-shit-done/bin/check-latest-version.cjs index bf2d54193..78b3e109f 100755 --- a/get-shit-done/bin/check-latest-version.cjs +++ b/get-shit-done/bin/check-latest-version.cjs @@ -20,7 +20,7 @@ * Text Matching on Test Outputs". */ -const cp = require('node:child_process'); +const { execNpm } = require('./lib/shell-command-projection.cjs'); // Hardcoded. Do not parameterise — the whole point of this script is that // the package name is not a runtime choice for the caller. @@ -36,19 +36,24 @@ const SEMVER_RE = /^\d+\.\d+\.\d+(?:[-+][0-9A-Za-z.-]+)?$/; /** * Pure-ish: takes an injected spawn function so tests don't actually run npm. - * In production, defaults to cp.spawnSync('npm', ...). + * In production, defaults to execNpm() from the shell-projection seam. */ function checkLatestVersion(opts = {}) { - const defaultSpawn = () => cp.spawnSync('npm', ['view', PACKAGE_NAME, 'version'], { - encoding: 'utf8', - stdio: ['ignore', 'pipe', 'pipe'], - shell: process.platform === 'win32', // npm is npm.cmd on Windows - // Bound the registry call so a hung network/registry doesn't block the - // entire /gsd-update workflow indefinitely (#2993 CR). 15s is generous - // for `npm view version`; on timeout, spawnSync returns with - // signal !== null and the existing failure path emits FAIL_NPM_FAILED. - timeout: 15_000, - }); + // Default path routes through the shell-projection seam (execNpm owns the + // Windows shell-flag policy and timeout default). The injection point + // remains spawnSync-shaped for test compatibility — the adapter below + // translates { exitCode } → { status } so the consumer logic is unchanged. + // Bounded at 15s so a hung registry doesn't block /gsd-update (#2993 CR). + const defaultSpawn = () => { + const r = execNpm(['view', PACKAGE_NAME, 'version'], { timeout: 15_000 }); + return { + status: r.exitCode, + stdout: r.stdout, + stderr: r.stderr, + signal: r.signal, + error: r.error, + }; + }; const spawn = opts.spawn || defaultSpawn; const r = spawn(); diff --git a/get-shit-done/bin/lib/commands.cjs b/get-shit-done/bin/lib/commands.cjs index 79af406e3..1fe4704a9 100644 --- a/get-shit-done/bin/lib/commands.cjs +++ b/get-shit-done/bin/lib/commands.cjs @@ -3,8 +3,8 @@ */ const fs = require('fs'); const path = require('path'); -const { execSync } = require('child_process'); -const { safeReadFile, loadConfig, isGitIgnored, execGit, normalizePhaseName, comparePhaseNum, getArchivedPhaseDirs, generateSlugInternal, getMilestoneInfo, getMilestonePhaseFilter, resolveModelInternal, stripShippedMilestones, extractCurrentMilestone, toPosixPath, output, error, findPhaseInternal, extractOneLinerFromBody, getRoadmapPhaseInternal } = require('./core.cjs'); +const { execGit } = require('./shell-command-projection.cjs'); +const { safeReadFile, loadConfig, isGitIgnored, normalizePhaseName, comparePhaseNum, getArchivedPhaseDirs, generateSlugInternal, getMilestoneInfo, getMilestonePhaseFilter, resolveModelInternal, stripShippedMilestones, extractCurrentMilestone, toPosixPath, output, error, findPhaseInternal, extractOneLinerFromBody, getRoadmapPhaseInternal } = require('./core.cjs'); const { planningDir, planningPaths } = require('./planning-workspace.cjs'); const { extractFrontmatter } = require('./frontmatter.cjs'); const { MODEL_PROFILES } = require('./model-profiles.cjs'); @@ -302,12 +302,12 @@ function cmdCommit(cwd, message, files, raw, amend, noVerify) { } } if (branchName) { - const currentBranch = execGit(cwd, ['rev-parse', '--abbrev-ref', 'HEAD']); + const currentBranch = execGit(['rev-parse', '--abbrev-ref', 'HEAD'], { cwd }); if (currentBranch.exitCode === 0 && currentBranch.stdout.trim() !== branchName) { // Create branch if it doesn't exist, or switch to it if it does - const create = execGit(cwd, ['checkout', '-b', branchName]); + const create = execGit(['checkout', '-b', branchName], { cwd }); if (create.exitCode !== 0) { - execGit(cwd, ['checkout', branchName]); + execGit(['checkout', branchName], { cwd }); } } } @@ -327,29 +327,34 @@ function cmdCommit(cwd, message, files, raw, amend, noVerify) { } // Default mode (staging all of .planning/): stage the deletion so // removed planning files are not left dangling in the index. - execGit(cwd, ['rm', '--cached', '--ignore-unmatch', file]); + execGit(['rm', '--cached', '--ignore-unmatch', file], { cwd }); } else { - execGit(cwd, ['add', file]); + execGit(['add', file], { cwd }); } } // Commit (--no-verify skips pre-commit hooks, used by parallel executor agents) const commitArgs = amend ? ['commit', '--amend', '--no-edit'] : ['commit', '-m', message]; if (noVerify) commitArgs.push('--no-verify'); - const commitResult = execGit(cwd, commitArgs); + const commitResult = execGit(commitArgs, { cwd }); if (commitResult.exitCode !== 0) { if (commitResult.stdout.includes('nothing to commit') || commitResult.stderr.includes('nothing to commit')) { const result = { committed: false, hash: null, reason: 'nothing_to_commit' }; output(result, raw, 'nothing'); return; } - const result = { committed: false, hash: null, reason: 'nothing_to_commit', error: commitResult.stderr }; - output(result, raw, 'nothing'); + const result = { + committed: false, + hash: null, + reason: 'commit_failed', + error: commitResult.stderr || commitResult.stdout, + }; + output(result, raw, 'failed'); return; } // Get short hash - const hashResult = execGit(cwd, ['rev-parse', '--short', 'HEAD']); + const hashResult = execGit(['rev-parse', '--short', 'HEAD'], { cwd }); const hash = hashResult.exitCode === 0 ? hashResult.stdout : null; const result = { committed: true, hash, reason: 'committed' }; output(result, raw, hash || 'committed'); @@ -395,11 +400,11 @@ function cmdCommitToSubrepo(cwd, message, files, raw) { // Stage files (strip sub-repo prefix for paths relative to that repo) for (const file of repoFiles) { const relativePath = file.slice(repo.length + 1); - execGit(repoCwd, ['add', relativePath]); + execGit(['add', relativePath], { cwd: repoCwd }); } // Commit - const commitResult = execGit(repoCwd, ['commit', '-m', message]); + const commitResult = execGit(['commit', '-m', message], { cwd: repoCwd }); if (commitResult.exitCode !== 0) { if (commitResult.stdout.includes('nothing to commit') || commitResult.stderr.includes('nothing to commit')) { repos[repo] = { committed: false, hash: null, files: repoFiles, reason: 'nothing_to_commit' }; @@ -410,7 +415,7 @@ function cmdCommitToSubrepo(cwd, message, files, raw) { } // Get hash - const hashResult = execGit(repoCwd, ['rev-parse', '--short', 'HEAD']); + const hashResult = execGit(['rev-parse', '--short', 'HEAD'], { cwd: repoCwd }); const hash = hashResult.exitCode === 0 ? hashResult.stdout : null; repos[repo] = { committed: true, hash, files: repoFiles }; } @@ -914,14 +919,14 @@ function cmdStats(cwd, format, raw) { // Git stats let gitCommits = 0; let gitFirstCommitDate = null; - const commitCount = execGit(cwd, ['rev-list', '--count', 'HEAD']); + const commitCount = execGit(['rev-list', '--count', 'HEAD'], { cwd }); if (commitCount.exitCode === 0) { gitCommits = parseInt(commitCount.stdout, 10) || 0; } - const rootHash = execGit(cwd, ['rev-list', '--max-parents=0', 'HEAD']); + const rootHash = execGit(['rev-list', '--max-parents=0', 'HEAD'], { cwd }); if (rootHash.exitCode === 0 && rootHash.stdout) { const firstCommit = rootHash.stdout.split('\n')[0].trim(); - const firstDate = execGit(cwd, ['show', '-s', '--format=%as', firstCommit]); + const firstDate = execGit(['show', '-s', '--format=%as', firstCommit], { cwd }); if (firstDate.exitCode === 0) { gitFirstCommitDate = firstDate.stdout || null; } @@ -990,9 +995,9 @@ function cmdCheckCommit(cwd, raw) { } // commit_docs is false — check if any .planning/ files are staged - try { - const staged = execSync('git diff --cached --name-only', { cwd, encoding: 'utf-8' }).trim(); - const planningFiles = staged.split('\n').filter(f => f.startsWith('.planning/') || f.startsWith('.planning\\')); + const stagedResult = execGit(['diff', '--cached', '--name-only'], { cwd }); + if (stagedResult.exitCode === 0) { + const planningFiles = stagedResult.stdout.split('\n').filter(f => f.startsWith('.planning/') || f.startsWith('.planning\\')); if (planningFiles.length > 0) { error( @@ -1001,9 +1006,8 @@ function cmdCheckCommit(cwd, raw) { `\n\nTo unstage: git reset HEAD ${planningFiles.join(' ')}` ); } - } catch { - // git diff --cached failed (no staged files or not a git repo) — allow } + // exitCode !== 0 → no staged files or not a git repo — allow output({ allowed: true, reason: 'no_planning_files_staged' }, raw, 'allowed'); } diff --git a/get-shit-done/bin/lib/core.cjs b/get-shit-done/bin/lib/core.cjs index 78faac1ee..f2c95e475 100644 --- a/get-shit-done/bin/lib/core.cjs +++ b/get-shit-done/bin/lib/core.cjs @@ -5,7 +5,7 @@ const fs = require('fs'); const os = require('os'); const path = require('path'); -const { execSync, execFileSync, spawnSync } = require('child_process'); +const { execGit } = require('./shell-command-projection.cjs'); const { MODEL_PROFILES, AGENT_TO_PHASE_TYPE, VALID_PHASE_TYPES, AGENT_DEFAULT_TIERS, VALID_AGENT_TIERS, nextTier } = require('./model-profiles.cjs'); const { MODEL_ALIAS_MAP, RUNTIME_PROFILE_MAP, KNOWN_RUNTIMES, RUNTIMES_WITH_REASONING_EFFORT } = require('./model-catalog.cjs'); const { @@ -593,23 +593,16 @@ const _gitIgnoredCache = new Map(); function isGitIgnored(cwd, targetPath) { const key = cwd + '::' + targetPath; if (_gitIgnoredCache.has(key)) return _gitIgnoredCache.get(key); - try { - // --no-index checks .gitignore rules regardless of whether the file is tracked. - // Without it, git check-ignore returns "not ignored" for tracked files even when - // .gitignore explicitly lists them — a common source of confusion when .planning/ - // was committed before being added to .gitignore. - // Use execFileSync (array args) to prevent shell interpretation of special characters - // in file paths — avoids command injection via crafted path names. - execFileSync('git', ['check-ignore', '-q', '--no-index', '--', targetPath], { - cwd, - stdio: 'pipe', - }); - _gitIgnoredCache.set(key, true); - return true; - } catch { - _gitIgnoredCache.set(key, false); - return false; - } + // --no-index checks .gitignore rules regardless of whether the file is tracked. + // Without it, git check-ignore returns "not ignored" for tracked files even when + // .gitignore explicitly lists them — a common source of confusion when .planning/ + // was committed before being added to .gitignore. + // Array args (via the seam) prevent shell interpretation of special characters in + // file paths — avoids command injection via crafted path names. + const result = execGit(['check-ignore', '-q', '--no-index', '--', targetPath], { cwd }); + const ignored = result.exitCode === 0; + _gitIgnoredCache.set(key, ignored); + return ignored; } // ─── Markdown normalization ───────────────────────────────────────────────── @@ -724,39 +717,6 @@ function normalizeMd(content) { // Default timeout for worktree-related git subprocess calls (matches worktree-safety.cjs). // Prevents `git worktree list --porcelain` and similar calls from blocking the parent -// process indefinitely when git is stalled (locked index, hung remote, NFS mount freeze). -// Callers can override via an options bag if needed. -const DEFAULT_GIT_TIMEOUT_MS = 10000; - -/** - * Execute a git command with a bounded timeout. - * - * Return shape: { exitCode, stdout, stderr, timedOut, error } - * - timedOut: true when spawnSync reports SIGTERM + ETIMEDOUT — callers must - * branch on this to surface a structured warning (PRED.k302). - * - error: spawnSync error object or null - * - * Backward-compatible: existing callers that only read exitCode/stdout/stderr - * continue to work unchanged. - */ -function execGit(cwd, args, options = {}) { - const timeout = options.timeout ?? DEFAULT_GIT_TIMEOUT_MS; - const result = spawnSync('git', args, { - cwd, - stdio: 'pipe', - encoding: 'utf-8', - timeout, - }); - const timedOut = result.signal === 'SIGTERM' && result.error?.code === 'ETIMEDOUT'; - return { - exitCode: result.status ?? 1, - stdout: (result.stdout ?? '').toString().trim(), - stderr: (result.stderr ?? '').toString().trim(), - timedOut, - error: result.error ?? null, - }; -} - // ─── Common path helpers ────────────────────────────────────────────────────── /** @@ -765,8 +725,10 @@ function execGit(cwd, args, options = {}) { * Returns the main worktree path, or cwd if not in a worktree. */ function resolveWorktreeRoot(cwd) { + // Omit execGit so worktree-safety uses its own execGitDefault — that wrapper + // delegates to the seam and derives the `timedOut` field that pruneResult + // branches on below. const context = resolveWorktreeContext(cwd, { - execGit, existsSync: fs.existsSync, }); return context.effectiveRoot; @@ -798,9 +760,9 @@ function pruneOrphanedWorktrees(repoRoot) { const plan = planWorktreePrune( repoRoot, { allowDestructive: false }, - { execGit, parseWorktreePorcelain } + { parseWorktreePorcelain } ); - const pruneResult = executeWorktreePrunePlan(plan, { execGit }); + const pruneResult = executeWorktreePrunePlan(plan); if (pruneResult && pruneResult.timedOut) { // AC2: surface structured warning instead of silently swallowing the timeout. // Uses process.stderr.write to match the [gsd-tools] WARNING prefix style. @@ -1980,7 +1942,6 @@ module.exports = { safeReadFile, loadConfig, isGitIgnored, - execGit, normalizeMd, escapeRegex, normalizePhaseName, diff --git a/get-shit-done/bin/lib/graphify.cjs b/get-shit-done/bin/lib/graphify.cjs index 721166eb7..d9c1ed1ba 100644 --- a/get-shit-done/bin/lib/graphify.cjs +++ b/get-shit-done/bin/lib/graphify.cjs @@ -2,8 +2,8 @@ const fs = require('fs'); const path = require('path'); -const childProcess = require('child_process'); -const { atomicWriteFileSync, execGit } = require('./core.cjs'); +const { execTool, execGit } = require('./shell-command-projection.cjs'); +const { atomicWriteFileSync } = require('./core.cjs'); // ─── Config Gate ───────────────────────────────────────────────────────────── @@ -58,15 +58,13 @@ const GRAPHIFY_REASON = Object.freeze({ function execGraphify(cwd, args, options = {}) { const timeout = options.timeout ?? 30000; - const result = childProcess.spawnSync('graphify', args, { + const result = execTool('graphify', args, { cwd, - stdio: 'pipe', - encoding: 'utf-8', timeout, env: { ...process.env, PYTHONUNBUFFERED: '1' }, }); - // ENOENT -- graphify binary not found on PATH + // ENOENT — seam normalizes to exitCode 127. Surface as typed reason. if (result.error && result.error.code === 'ENOENT') { return { exitCode: 127, @@ -76,23 +74,22 @@ function execGraphify(cwd, args, options = {}) { }; } - // Timeout -- subprocess killed via SIGTERM + // Timeout — seam exposes signal; spawnSync sets SIGTERM when killed by timeout. if (result.signal === 'SIGTERM') { return { exitCode: 124, - stdout: (result.stdout ?? '').toString().trim(), + stdout: result.stdout, stderr: 'graphify timed out after ' + timeout + 'ms', reason: GRAPHIFY_REASON.TIMEOUT, timeout_ms: timeout, }; } - const exitCode = result.status ?? 1; return { - exitCode, - stdout: (result.stdout ?? '').toString().trim(), - stderr: (result.stderr ?? '').toString().trim(), - reason: exitCode === 0 ? GRAPHIFY_REASON.OK : GRAPHIFY_REASON.EXIT_NONZERO, + exitCode: result.exitCode, + stdout: result.stdout, + stderr: result.stderr, + reason: result.exitCode === 0 ? GRAPHIFY_REASON.OK : GRAPHIFY_REASON.EXIT_NONZERO, }; } @@ -105,11 +102,7 @@ function execGraphify(cwd, args, options = {}) { * @returns {{ installed: boolean, message?: string }} */ function checkGraphifyInstalled() { - const result = childProcess.spawnSync('graphify', ['--help'], { - stdio: 'pipe', - encoding: 'utf-8', - timeout: 5000, - }); + const result = execTool('graphify', ['--help'], { timeout: 5000 }); if (result.error) { return { @@ -134,18 +127,13 @@ function checkGraphifyInstalled() { */ function checkGraphifyVersion() { // Strategy 1: try `graphify --version` directly (2s timeout -- fast path) - const versionResult = childProcess.spawnSync('graphify', ['--version'], { - stdio: 'pipe', - encoding: 'utf-8', - timeout: 2000, - }); + const versionResult = execTool('graphify', ['--version'], { timeout: 2000 }); let versionStr = null; - if (!versionResult.error && versionResult.status === 0) { - const raw = (versionResult.stdout || '').trim(); + if (!versionResult.error && versionResult.exitCode === 0) { // graphify --version may emit "graphify 0.4.23" or just "0.4.23" - const match = raw.match(/(\d+\.\d+(?:\.\d+)*)/); + const match = versionResult.stdout.match(/(\d+\.\d+(?:\.\d+)*)/); if (match) { versionStr = match[1]; } @@ -153,17 +141,13 @@ function checkGraphifyVersion() { // Strategy 2: fall back to python3 importlib.metadata if (!versionStr) { - const pyResult = childProcess.spawnSync('python3', [ + const pyResult = execTool('python3', [ '-c', 'from importlib.metadata import version; print(version("graphifyy"))', - ], { - stdio: 'pipe', - encoding: 'utf-8', - timeout: 5000, - }); + ], { timeout: 5000 }); - if (!pyResult.error && pyResult.status === 0 && pyResult.stdout && pyResult.stdout.trim()) { - versionStr = pyResult.stdout.trim(); + if (!pyResult.error && pyResult.exitCode === 0 && pyResult.stdout) { + versionStr = pyResult.stdout; } } @@ -370,7 +354,7 @@ const COMMIT_HASH_RE = /^[0-9a-f]{4,40}$/i; * success, or null when cwd is not a git repo / `git` is not on PATH. */ function readGitHead(cwd) { - const r = execGit(cwd, ['rev-parse', 'HEAD']); + const r = execGit(['rev-parse', 'HEAD'], { cwd }); if (r.exitCode !== 0) return null; return r.stdout.trim() || null; } @@ -381,7 +365,7 @@ function readGitHead(cwd) { * or the cwd is not a git repo. */ function countCommitsBetween(cwd, from, to) { - const r = execGit(cwd, ['rev-list', '--count', `${from}..${to}`]); + const r = execGit(['rev-list', '--count', `${from}..${to}`], { cwd }); if (r.exitCode !== 0) return null; const n = parseInt(r.stdout.trim(), 10); return Number.isFinite(n) ? n : null; diff --git a/get-shit-done/bin/lib/init.cjs b/get-shit-done/bin/lib/init.cjs index ee7a4427f..b64bb2171 100644 --- a/get-shit-done/bin/lib/init.cjs +++ b/get-shit-done/bin/lib/init.cjs @@ -4,7 +4,7 @@ const fs = require('fs'); const path = require('path'); -const { execSync } = require('child_process'); +const { execGit } = require('./shell-command-projection.cjs'); const { loadConfig, resolveModelInternal, findPhaseInternal, getRoadmapPhaseInternal, pathExistsInternal, generateSlugInternal, getMilestoneInfo, getMilestonePhaseFilter, stripShippedMilestones, extractCurrentMilestone, normalizePhaseName, toPosixPath, output, error, checkAgentsInstalled, phaseTokenMatches } = require('./core.cjs'); const { planningPaths, planningDir, planningRoot } = require('./planning-workspace.cjs'); const { maskIfSecret } = require('./secrets.cjs'); @@ -1511,11 +1511,8 @@ function detectChildRepos(dir) { const fullPath = path.join(dir, entry.name); const gitDir = path.join(fullPath, '.git'); if (fs.existsSync(gitDir)) { - let hasUncommitted = false; - try { - const status = execSync('git status --porcelain', { cwd: fullPath, encoding: 'utf8', timeout: 5000 }); - hasUncommitted = status.trim().length > 0; - } catch { /* best-effort */ } + const statusResult = execGit(['status', '--porcelain'], { cwd: fullPath, timeout: 5000 }); + const hasUncommitted = statusResult.exitCode === 0 && statusResult.stdout.length > 0; repos.push({ name: entry.name, path: fullPath, has_uncommitted: hasUncommitted }); } } @@ -1530,11 +1527,8 @@ function cmdInitNewWorkspace(cwd, raw) { const childRepos = detectChildRepos(cwd); // Check if git worktree is available - let worktreeAvailable = false; - try { - execSync('git --version', { encoding: 'utf8', timeout: 5000, stdio: 'pipe' }); - worktreeAvailable = true; - } catch { /* no git at all */ } + const gitVersion = execGit(['--version'], { timeout: 5000 }); + const worktreeAvailable = gitVersion.exitCode === 0; const result = { default_workspace_base: defaultBase, @@ -1634,12 +1628,10 @@ function cmdInitRemoveWorkspace(cwd, name, raw) { for (const repo of repos) { const repoPath = path.join(wsPath, repo.name); if (!fs.existsSync(repoPath)) continue; - try { - const status = execSync('git status --porcelain', { cwd: repoPath, encoding: 'utf8', timeout: 5000, stdio: 'pipe' }); - if (status.trim().length > 0) { - dirtyRepos.push(repo.name); - } - } catch { /* best-effort */ } + const statusResult = execGit(['status', '--porcelain'], { cwd: repoPath, timeout: 5000 }); + if (statusResult.exitCode === 0 && statusResult.stdout.length > 0) { + dirtyRepos.push(repo.name); + } } const result = { diff --git a/get-shit-done/bin/lib/planning-workspace.cjs b/get-shit-done/bin/lib/planning-workspace.cjs index 61e9af52e..8bb9e8a49 100644 --- a/get-shit-done/bin/lib/planning-workspace.cjs +++ b/get-shit-done/bin/lib/planning-workspace.cjs @@ -11,7 +11,7 @@ const fs = require('fs'); const os = require('os'); const path = require('path'); const crypto = require('crypto'); -const { execFileSync } = require('child_process'); +const { probeTty } = require('./shell-command-projection.cjs'); const { isValidActiveWorkstreamName } = require('./workstream-name-policy.cjs'); const WORKSTREAM_SESSION_ENV_KEYS = [ @@ -92,16 +92,11 @@ function probeControllingTtyToken() { return cachedControllingTtyToken; } - try { - const ttyPath = execFileSync('tty', [], { - encoding: 'utf-8', - stdio: ['inherit', 'pipe', 'ignore'], - }).trim(); - if (ttyPath && ttyPath !== 'not a tty') { - const token = sanitizeWorkstreamSessionToken(ttyPath.replace(/^\/dev\//, '')); - if (token) cachedControllingTtyToken = `tty-${token}`; - } - } catch {} + const ttyPath = probeTty(); + if (ttyPath) { + const token = sanitizeWorkstreamSessionToken(ttyPath.replace(/^\/dev\//, '')); + if (token) cachedControllingTtyToken = `tty-${token}`; + } return cachedControllingTtyToken; } diff --git a/get-shit-done/bin/lib/shell-command-projection.cjs b/get-shit-done/bin/lib/shell-command-projection.cjs index 82e1ad6c0..6497ab21a 100644 --- a/get-shit-done/bin/lib/shell-command-projection.cjs +++ b/get-shit-done/bin/lib/shell-command-projection.cjs @@ -2,7 +2,10 @@ const path = require('path'); const fs = require('fs'); -const { spawnSync, execFileSync } = require('child_process'); +// Use non-destructured access so test-time mock.method(childProcess, 'spawnSync') +// can intercept calls from this seam — destructured imports capture references +// at load time and become un-mockable. +const childProcess = require('child_process'); /** * Shell Command Projection Module @@ -358,18 +361,30 @@ function formatSdkPathDiagnostic({ shimDir, platform, runDir }) { function _spawnResult(result, program) { if (result.error && result.error.code === 'ENOENT') { - return { exitCode: 127, stdout: '', stderr: `${program}: not found` }; + return { exitCode: 127, stdout: '', stderr: `${program}: not found`, signal: null, error: result.error }; } return { exitCode: result.status ?? 1, stdout: (result.stdout ?? '').toString().trim(), stderr: (result.stderr ?? '').toString().trim(), + signal: result.signal ?? null, + error: result.error ?? null, }; } function execGit(args, opts = {}) { - const result = spawnSync('git', args, { + // Non-interactive defaults: a hung credential prompt or terminal-input + // probe must surface as a timeout, not block the tool forever. Callers + // can override via opts.env. + const env = { + ...process.env, + GIT_TERMINAL_PROMPT: '0', + GCM_INTERACTIVE: 'never', + ...(opts.env || {}), + }; + const result = childProcess.spawnSync('git', args, { cwd: opts.cwd, + env, encoding: 'utf-8', stdio: 'pipe', timeout: opts.timeout ?? 10_000, @@ -378,7 +393,7 @@ function execGit(args, opts = {}) { } function execNpm(args, opts = {}) { - const result = spawnSync('npm', args, { + const result = childProcess.spawnSync('npm', args, { cwd: opts.cwd, shell: process.platform === 'win32', encoding: 'utf-8', @@ -389,8 +404,9 @@ function execNpm(args, opts = {}) { } function execTool(program, args, opts = {}) { - const result = spawnSync(program, args, { + const result = childProcess.spawnSync(program, args, { cwd: opts.cwd, + env: opts.env ? { ...process.env, ...opts.env } : undefined, encoding: 'utf-8', stdio: 'pipe', timeout: opts.timeout ?? 30_000, @@ -402,7 +418,7 @@ function probeTty(opts = {}) { const platform = opts.platform ?? process.platform; if (platform === 'win32') return null; try { - const ttyPath = execFileSync('tty', [], { + const ttyPath = childProcess.execFileSync('tty', [], { encoding: 'utf-8', stdio: ['inherit', 'pipe', 'ignore'], }).trim(); diff --git a/get-shit-done/bin/lib/verify.cjs b/get-shit-done/bin/lib/verify.cjs index 2acaa0b1e..bd3ce7293 100644 --- a/get-shit-done/bin/lib/verify.cjs +++ b/get-shit-done/bin/lib/verify.cjs @@ -5,7 +5,8 @@ const fs = require('fs'); const path = require('path'); const os = require('os'); -const { safeReadFile, loadConfig, normalizePhaseName, escapeRegex, execGit, findPhaseInternal, getMilestoneInfo, stripShippedMilestones, extractCurrentMilestone, output, error, checkAgentsInstalled, CONFIG_DEFAULTS, inspectWorktreeHealth } = require('./core.cjs'); +const { safeReadFile, loadConfig, normalizePhaseName, escapeRegex, findPhaseInternal, getMilestoneInfo, stripShippedMilestones, extractCurrentMilestone, output, error, checkAgentsInstalled, CONFIG_DEFAULTS, inspectWorktreeHealth } = require('./core.cjs'); +const { execGit } = require('./shell-command-projection.cjs'); const { planningDir } = require('./planning-workspace.cjs'); const { extractFrontmatter, parseMustHavesBlock } = require('./frontmatter.cjs'); const { writeStateMd } = require('./state.cjs'); @@ -68,8 +69,8 @@ function cmdVerifySummary(cwd, summaryPath, checkFileCount, raw) { let commitsExist = false; if (hashes.length > 0) { for (const hash of hashes.slice(0, 3)) { - const result = execGit(cwd, ['cat-file', '-t', hash]); - if (result.exitCode === 0 && result.stdout === 'commit') { + const result = execGit(['cat-file', '-t', hash], { cwd }); + if (result.exitCode === 0 && result.stdout.trim() === 'commit') { commitsExist = true; break; } @@ -265,7 +266,7 @@ function cmdVerifyCommits(cwd, hashes, raw) { const valid = []; const invalid = []; for (const hash of hashes) { - const result = execGit(cwd, ['cat-file', '-t', hash]); + const result = execGit(['cat-file', '-t', hash], { cwd }); if (result.exitCode === 0 && result.stdout.trim() === 'commit') { valid.push(hash); } else { @@ -1226,7 +1227,7 @@ function cmdVerifySchemaDrift(cwd, phaseArg, skipFlag, raw) { } // Also check git commit messages for push evidence - const gitLog = execGit(cwd, ['log', '--oneline', '--all', '-50']); + const gitLog = execGit(['log', '--oneline', '--all', '-50'], { cwd }); if (gitLog.exitCode === 0) { executionLog += '\n' + gitLog.stdout; } @@ -1288,7 +1289,7 @@ function cmdVerifyCodebaseDrift(cwd, raw) { const lastMapped = drift.readMappedCommit(structurePath); // Verify we're inside a git repo and resolve the diff range. - const revProbe = execGit(cwd, ['rev-parse', 'HEAD']); + const revProbe = execGit(['rev-parse', 'HEAD'], { cwd }); if (revProbe.exitCode !== 0) { emit({ skipped: true, @@ -1307,11 +1308,11 @@ function cmdVerifyCodebaseDrift(cwd, raw) { base = EMPTY_TREE; } else { // Verify the commit is reachable; if not, fall back to EMPTY_TREE. - const verify = execGit(cwd, ['cat-file', '-t', base]); + const verify = execGit(['cat-file', '-t', base], { cwd }); if (verify.exitCode !== 0) base = EMPTY_TREE; } - const diff = execGit(cwd, ['diff', '--name-status', base, 'HEAD']); + const diff = execGit(['diff', '--name-status', base, 'HEAD'], { cwd }); if (diff.exitCode !== 0) { emit({ skipped: true, diff --git a/get-shit-done/bin/lib/worktree-safety.cjs b/get-shit-done/bin/lib/worktree-safety.cjs index 2cd23fe80..9e641618a 100644 --- a/get-shit-done/bin/lib/worktree-safety.cjs +++ b/get-shit-done/bin/lib/worktree-safety.cjs @@ -6,7 +6,7 @@ const fs = require('fs'); const path = require('path'); -const { spawnSync } = require('child_process'); +const { execGit: execGitSeam } = require('./shell-command-projection.cjs'); // Default timeout for worktree-related git subprocess calls. // 10 s is generous enough for normal git operations on large repos while still @@ -15,42 +15,17 @@ const { spawnSync } = require('child_process'); const DEFAULT_GIT_TIMEOUT_MS = 10000; /** - * Execute a git command with a bounded timeout. + * Execute a git command via the shell-projection seam, with a derived + * `timedOut` field. Tests inject mocks via deps.execGit using the new + * (args, opts) shape — see worktree-safety-policy.test.cjs. * - * Return shape: { exitCode, stdout, stderr, timedOut, error } - * - exitCode: process exit status (null when killed by signal) - * - timedOut: true when spawnSync reports SIGTERM + ETIMEDOUT — callers must - * branch on this to surface a structured warning instead of - * silently treating the empty output as success (PRED.k302) - * - error: the Error object from spawnSync when the process could not start - * or was killed; null otherwise - * - * Backward-compatible: existing callers that only read exitCode/stdout/stderr - * continue to work unchanged. + * Return shape: { exitCode, stdout, stderr, timedOut, error, signal } + * - timedOut: true when spawnSync reports SIGTERM + ETIMEDOUT */ -function execGitDefault(cwd, args, options = {}) { - const timeout = options.timeout ?? DEFAULT_GIT_TIMEOUT_MS; - const result = spawnSync('git', args, { - cwd, - stdio: 'pipe', - encoding: 'utf-8', - timeout, - env: { - ...process.env, - GIT_TERMINAL_PROMPT: '0', - GCM_INTERACTIVE: 'never', - }, - }); - // spawnSync sets signal='SIGTERM' and error.code='ETIMEDOUT' when the timeout - // fires and the subprocess is killed. +function execGitDefault(args, opts = {}) { + const result = execGitSeam(args, { ...opts, timeout: opts.timeout ?? DEFAULT_GIT_TIMEOUT_MS }); const timedOut = result.signal === 'SIGTERM' && result.error?.code === 'ETIMEDOUT'; - return { - exitCode: result.status ?? 1, - stdout: (result.stdout ?? '').toString().trim(), - stderr: (result.stderr ?? '').toString().trim(), - timedOut, - error: result.error ?? null, - }; + return { ...result, timedOut }; } function parseWorktreePorcelain(porcelain) { @@ -82,7 +57,7 @@ function parseWorktreeListPaths(porcelain) { function readWorktreeList(repoRoot, deps = {}) { const execGit = deps.execGit || execGitDefault; - const listResult = execGit(repoRoot, ['worktree', 'list', '--porcelain']); + const listResult = execGit(['worktree', 'list', '--porcelain'], { cwd: repoRoot }); if (listResult.timedOut) { // AC2 / AC4: surface timeout as a distinct reason so callers can emit a // structured warning rather than silently treating the failure as a generic @@ -127,8 +102,8 @@ function resolveWorktreeContext(cwd, deps = {}) { }; } - const gitDir = execGit(cwd, ['rev-parse', '--git-dir']); - const commonDir = execGit(cwd, ['rev-parse', '--git-common-dir']); + const gitDir = execGit(['rev-parse', '--git-dir'], { cwd }); + const commonDir = execGit(['rev-parse', '--git-common-dir'], { cwd }); if (gitDir.exitCode !== 0 || commonDir.exitCode !== 0) { return { effectiveRoot: cwd, @@ -203,7 +178,7 @@ function executeWorktreePrunePlan(plan, deps = {}) { }; } - const result = execGit(plan.repoRoot, ['worktree', 'prune']); + const result = execGit(['worktree', 'prune'], { cwd: plan.repoRoot }); if (result.timedOut) { // AC4: surface timedOut as a first-class field so callers (e.g. // pruneOrphanedWorktrees in core.cjs) can log a structured WARNING rather @@ -436,8 +411,8 @@ function executeWorktreeWaveCleanupPlan(plan, deps = {}) { stderr: '', }; - const branchCheck = execGit(plan.repoRoot, ['-C', entry.worktree_path, 'rev-parse', '--abbrev-ref', 'HEAD']); - if (!gitResultOk(branchCheck) || branchCheck.stdout !== entry.branch) { + const branchCheck = execGit(['-C', entry.worktree_path, 'rev-parse', '--abbrev-ref', 'HEAD'], { cwd: plan.repoRoot }); + if (!gitResultOk(branchCheck) || branchCheck.stdout.trim() !== entry.branch) { result.status = 'blocked'; result.reason = 'branch_mismatch'; result.stderr = branchCheck?.stderr || ''; @@ -447,8 +422,8 @@ function executeWorktreeWaveCleanupPlan(plan, deps = {}) { break; } - const mergeBase = execGit(plan.repoRoot, ['merge-base', 'HEAD', entry.branch]); - if (!gitResultOk(mergeBase) || mergeBase.stdout !== entry.expected_base) { + const mergeBase = execGit(['merge-base', 'HEAD', entry.branch], { cwd: plan.repoRoot }); + if (!gitResultOk(mergeBase) || mergeBase.stdout.trim() !== entry.expected_base) { result.status = 'blocked'; result.reason = 'base_mismatch'; result.stderr = mergeBase?.stderr || ''; @@ -458,7 +433,7 @@ function executeWorktreeWaveCleanupPlan(plan, deps = {}) { break; } - const deletions = execGit(plan.repoRoot, ['diff', '--diff-filter=D', '--name-only', `HEAD...${entry.branch}`]); + const deletions = execGit(['diff', '--diff-filter=D', '--name-only', `HEAD...${entry.branch}`], { cwd: plan.repoRoot }); if (!gitResultOk(deletions)) { result.status = 'blocked'; result.reason = 'deletion_check_failed'; @@ -478,7 +453,7 @@ function executeWorktreeWaveCleanupPlan(plan, deps = {}) { break; } - const worktreeStatus = execGit(plan.repoRoot, ['-C', entry.worktree_path, 'status', '--porcelain', '--untracked-files=all']); + const worktreeStatus = execGit(['-C', entry.worktree_path, 'status', '--porcelain', '--untracked-files=all'], { cwd: plan.repoRoot }); if (!gitResultOk(worktreeStatus) || worktreeStatus.stdout) { result.status = 'blocked'; result.reason = 'worktree_dirty'; @@ -489,7 +464,7 @@ function executeWorktreeWaveCleanupPlan(plan, deps = {}) { break; } - const merge = execGit(plan.repoRoot, ['merge', entry.branch, '--no-ff', '--no-edit', '-m', `chore: merge executor worktree (${entry.branch})`]); + const merge = execGit(['merge', entry.branch, '--no-ff', '--no-edit', '-m', `chore: merge executor worktree (${entry.branch})`], { cwd: plan.repoRoot }); if (!gitResultOk(merge)) { result.status = 'blocked'; result.reason = 'merge_failed'; @@ -500,7 +475,7 @@ function executeWorktreeWaveCleanupPlan(plan, deps = {}) { break; } - const remove = execGit(plan.repoRoot, ['worktree', 'remove', entry.worktree_path, '--force']); + const remove = execGit(['worktree', 'remove', entry.worktree_path, '--force'], { cwd: plan.repoRoot }); if (!gitResultOk(remove)) { result.status = 'blocked'; result.reason = 'worktree_remove_failed'; @@ -511,7 +486,7 @@ function executeWorktreeWaveCleanupPlan(plan, deps = {}) { break; } - const branchDelete = execGit(plan.repoRoot, ['branch', '-D', entry.branch]); + const branchDelete = execGit(['branch', '-D', entry.branch], { cwd: plan.repoRoot }); if (!gitResultOk(branchDelete)) { result.status = 'warning'; result.reason = 'branch_delete_failed'; diff --git a/tests/bug-3281-worktree-git-timeout.test.cjs b/tests/bug-3281-worktree-git-timeout.test.cjs index 53187a6a1..3e3127da2 100644 --- a/tests/bug-3281-worktree-git-timeout.test.cjs +++ b/tests/bug-3281-worktree-git-timeout.test.cjs @@ -35,7 +35,7 @@ const WORKTREE_SAFETY_PATH = path.join( * - not throw */ function makeTimeoutStub() { - return function stubTimedOutExecGit(_cwd, _args) { + return function stubTimedOutExecGit(_args, _opts) { return { exitCode: null, stdout: '', diff --git a/tests/bug-3384-worktree-cleanup-manifest.test.cjs b/tests/bug-3384-worktree-cleanup-manifest.test.cjs index 2f8524866..51674adb3 100644 --- a/tests/bug-3384-worktree-cleanup-manifest.test.cjs +++ b/tests/bug-3384-worktree-cleanup-manifest.test.cjs @@ -87,8 +87,8 @@ describe('bug #3384: worktree cleanup is manifest-scoped and fail-closed', () => }; const result = executeWorktreeWaveCleanupPlan(plan, { - execGit: (cwd, args) => { - calls.push({ cwd, args }); + execGit: (args, opts) => { + calls.push({ cwd: opts.cwd, args }); const key = args.join(' '); if (key === '-C /repo/.claude/worktrees/agent-a1 rev-parse --abbrev-ref HEAD') { return { exitCode: 0, stdout: 'worktree-agent-a1', stderr: '' }; @@ -141,7 +141,7 @@ describe('bug #3384: worktree cleanup is manifest-scoped and fail-closed', () => }; const result = executeWorktreeWaveCleanupPlan(plan, { - execGit: (_cwd, args) => { + execGit: (args) => { const key = args.join(' '); if (key === '-C /repo/.claude/worktrees/agent-a1 rev-parse --abbrev-ref HEAD') { return { exitCode: 0, stdout: 'worktree-agent-a1', stderr: '' }; @@ -184,7 +184,7 @@ describe('bug #3384: worktree cleanup is manifest-scoped and fail-closed', () => }; const result = executeWorktreeWaveCleanupPlan(plan, { - execGit: (_cwd, args) => { + execGit: (args) => { calls.push(args.join(' ')); const key = args.join(' '); if (key === '-C /repo/.claude/worktrees/agent-a1 rev-parse --abbrev-ref HEAD') { diff --git a/tests/worktree-safety-policy.test.cjs b/tests/worktree-safety-policy.test.cjs index 770c904fb..f0d093b3a 100644 --- a/tests/worktree-safety-policy.test.cjs +++ b/tests/worktree-safety-policy.test.cjs @@ -27,7 +27,7 @@ describe('worktree-safety policy module', () => { test('resolveWorktreeContext maps linked worktree to common-dir parent', () => { const context = resolveWorktreeContext('/repo/wt', { existsSync: () => false, - execGit: (_, args) => { + execGit: (args) => { if (args[1] === '--git-dir') return { exitCode: 0, stdout: '.git/worktrees/wt', stderr: '' }; if (args[1] === '--git-common-dir') return { exitCode: 0, stdout: '../.git', stderr: '' }; return { exitCode: 1, stdout: '', stderr: '' }; @@ -50,7 +50,7 @@ describe('worktree-safety policy module', () => { test('resolveWorktreeContext keeps cwd for main worktree checkout', () => { const context = resolveWorktreeContext('/repo/main', { existsSync: () => false, - execGit: (_, args) => { + execGit: (args) => { if (args[1] === '--git-dir') return { exitCode: 0, stdout: '.git', stderr: '' }; if (args[1] === '--git-common-dir') return { exitCode: 0, stdout: '.git', stderr: '' }; return { exitCode: 1, stdout: '', stderr: '' }; @@ -127,8 +127,8 @@ describe('worktree-safety policy module', () => { const result = executeWorktreePrunePlan( { repoRoot: '/repo/main', action: 'metadata_prune_only', reason: 'worktrees_present' }, { - execGit: (cwd, args) => { - calls.push({ cwd, args }); + execGit: (args, opts) => { + calls.push({ cwd: opts.cwd, args }); return { exitCode: 0, stdout: '', stderr: '' }; }, }