subprocess-safety

Subprocess Safety

Read, in order:

1. AGENTS.md (Invariant 3: subprocesses)
2. docs/engineering-invariants.md (subsection 3)
3. .agents/skills/writing-tests/SKILL.md if tests are touched
4. .opencode/skills/generated/mock-to-internals-migration/SKILL.md if converting mock.module to _internals

Codex-specific execution notes:

• This skill consolidates AGENTS.md Invariant 3 into an actionable checklist.
• The canonical spawn shape and six required properties are non-negotiable per AGENTS.md.
• The CI quality job enforces these via scripts/check-invariants.sh (Check 1: subprocess timeout).
• Violations are advisory in CI but blocking in code review.

When to use this skill

• You are adding, modifying, or reviewing a subprocess call (bunSpawn, spawn, spawnSync, child_process.execFile, etc.)
• You are writing or updating tests that exercise subprocess-dependent code
• A PR review flags a subprocess call missing timeout, cwd, or cleanup

Scope

This skill applies to all files that spawn child processes:

• src/utils/git*.ts
• src/hooks/*.ts
• src/tools/*.ts
• src/services/*.ts
• src/plugins/*.ts
• src/index.ts (init-path subprocesses)
• Any test file (tests/**) that stubs or exercises subprocess code

Canonical spawn shape

Every subprocess call MUST follow this pattern:

const PER_CALL_TIMEOUT_MS = 10_000; // module-level constant (choose an appropriate value)

const proc = bunSpawn(['git', '-C', dir, 'rev-parse', '--show-toplevel'], {
  stdin: 'ignore',
  cwd: dir,
  timeout: PER_CALL_TIMEOUT_MS,
  // stdout/stderr: piped, bounded, or ignored
});
try {
  const result = await proc;
  // process result
} finally {
  proc.kill(); // best-effort cleanup
}

Six required properties

execFile callback vs execFileSync distinction

child_process.execFile (callback form) and child_process.execFileSync have different default stdio behavior:

Key differences from the canonical spawn pattern:

1. proc.kill() in finally (line 69): Applicable to callback-form execFile. The function returns a ChildProcess reference (matching the canonical spawn pattern per Node.js docs). The child reference enables kill() before that point for timeout safety, and failing to call proc.kill() in finally can leave orphaned children when combined with an outer withTimeout. The timeout option triggers internal SIGTERM, but is not a substitute for explicit kill in finally — always kill the child in finally.

2. stdin: 'ignore' (line 66): Technically default-safe for callback execFile (stdin is piped, not inherited). However, always add stdio: ['ignore', 'pipe', 'pipe'] for defense-in-depth and consistency with execFileSync calls. Note: Bun's TypeScript definitions do not include stdio in ExecFileOptions — use execOpts as any when passing stdio to callback-form execFile.

3. execFileSync should always use stdio: ['ignore', 'pipe', 'pipe'] to prevent the stdin-inheritance hang on Windows/Bun (v7.3.3).

Windows-specific notes

• .cmd extensions: npm/bun binaries on Windows are .cmd wrappers. Resolve the executable path explicitly using which/where or the project's cross-platform helper. Do NOT enable shell: true or shell-mediated execution to work around PATH resolution.
• PATH differences: cmd.exe and PowerShell resolve PATH differently. Test on Windows, not just macOS/Linux.
• child_process.spawn('bin', ...) does not behave identically to running under cmd.exe. Use array-form args and explicit cwd.
• fs.renameSync cannot overwrite existing directories on Windows. Use a remove-then-rename pattern or fs.rename with error handling.

gh CLI Subprocess Patterns

The gh CLI is a common subprocess in this repo (scripts/release-notes-fragments.mjs, CI workflows). It follows the same six required properties as all subprocesses, plus several gh-specific patterns.

gh api --paginate requires --slurp

Bug pattern (PR #1762 F-002): gh api --paginate without --slurp produces concatenated JSON arrays on stdout. JSON.parse() can only parse the first array — subsequent arrays cause a parse error or are silently lost.

Correct pattern:

const raw = execFileSync('gh', ['api', '--paginate', '--slurp', 'repos/.../pulls', ...], {
  encoding: 'utf8',
  timeout: 30_000,
  maxBuffer: 16 * 1024 * 1024,
  stdio: ['ignore', 'pipe', 'pipe'], // required for execFileSync (AGENTS.md §3)
});
// --slurp wraps paginated results as [[page1], [page2], ...]
const pages = JSON.parse(raw);
const allItems = pages.flat(); // flatten to single array

Without --slurp: stdout is [item1, item2][item3, item4] — invalid JSON after the first array. This is a silent data loss bug that only manifests when results span multiple pages (>30 items by default).

stdin: 'ignore' for gh calls

gh subprocess calls must include stdin: 'ignore' (or stdio: ['ignore', 'pipe', 'pipe'] for execFileSync). This is the same invariant as all subprocesses (AGENTS.md §3). For example, scripts/release-notes-fragments.mjs defines ghJson() and ghText() helpers using execFileSync — these must include stdio: ['ignore', 'pipe', 'pipe'] per the six required properties. A PR review (pre-merge) identified this gap.

Number.isInteger() for API response validation

When validating integer IDs from API responses (PR numbers, issue numbers, run IDs), use Number.isInteger(), not Number.isFinite(). Number.isFinite() accepts floats like 1.5, which are never valid IDs.

// Correct
function isValidPrNumber(n) {
  return Number.isInteger(n) && n > 0;
}

// Wrong — accepts 1.5, NaN, Infinity
function isValidPrNumber(n) {
  return Number.isFinite(n) && n > 0;
}

Note: This is a stricter pattern. Some existing code uses Number.isFinite() after parseInt() — while technically safe for parsed integers, Number.isInteger() is the correct guard for all ID validation going forward.

maxBuffer for large API responses

gh api can return large payloads. Set maxBuffer: 16 * 1024 * 1024 (16 MiB) to prevent silent truncation. This is especially important for --paginate calls that aggregate multiple pages.

Note: maxBuffer is specific to Node.js child_process.execFile/execFileSync. For Bun's bunSpawn, use the equivalent output bounding option.

Testing pattern: _internals DI seam, NOT mock.module

mock.module(...) leaks across test files in Bun's shared test-runner process. Use dependency injection instead:

// --- source file (e.g. src/utils/gitignore-warning.ts) ---
import { bunSpawn } from './bun-compat';

export const _internals: { bunSpawn: typeof bunSpawn } = { bunSpawn };

// In production code, call _internals.bunSpawn(...) instead of bunSpawn(...)

// --- test file ---
import { _internals } from '../../src/utils/gitignore-warning';
const real = _internals.bunSpawn;
beforeEach(() => { _internals.bunSpawn = stub; });
afterEach(() => { _internals.bunSpawn = real; });

For the full migration protocol, load the mock-to-internals-migration skill.

Verification grep

After changing any file with subprocess calls, run:

grep -n "bunSpawn\|spawn(\|spawnSync(" src/<changed>/*.ts

Every match MUST have all of:

1. timeout set to a concrete millisecond value
2. stdin: 'ignore' (unless intentionally interactive; note: callback-form execFile uses stdio: ['ignore', 'pipe', 'pipe'] instead)
3. cwd or git -C <directory> for explicit working directory
4. proc.kill() in a finally block or equivalent cleanup path (exception: callback-form execFile manages cleanup internally via timeout option)

Historical failures

• v7.0.3 (#704): repo-graph Desktop hang -- unbounded filesystem scan on plugin init. No timeout, no kill path. Result: "no agents in TUI/GUI" with no error message.
• v7.3.3 (#732): Git-hygiene startup regression -- ensureSwarmGitExcluded called git without timeout, stdin, or kill. Result: same silent failure on Windows.

Both caused OpenCode to silently drop the plugin manifest. Users saw no agents and no error. Every subprocess call is a potential repeat of these failures unless all six properties are enforced.