---
name: subprocess-safety
audience: swarm-plugin
description: Guidelines for safe subprocess calls in opencode-swarm. Load before adding, modifying, or reviewing any file that calls spawn, spawnSync, bunSpawn, or child_process. Covers the six required properties, Windows portability, _internals DI seam pattern, and verification grep.
---

# 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 `bun run check:invariants` (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:

```typescript
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

| Property | Required | Rationale |
|----------|----------|-----------|
| Array-form args | Yes | No shell-string commands (injection risk, quoting hell) |
| `cwd` or `git -C` | Yes | Never rely on inherited process.cwd() |
| `stdin: 'ignore'` | Yes | A never-closed stdin pipe under Bun/Windows can block child exit (v7.3.3) |
| `timeout: <ms>` | Yes | No subprocess is "always fast" on every platform |
| stdout/stderr bounded | Yes | Never leave piped stream unattended on long-running child |
| `proc.kill()` in `finally` | Yes | Outer withTimeout lets awaiter proceed but doesn't abort child |

## execFile callback vs execFileSync distinction

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

| API | Default stdin | Risk |
|-----|---------------|------|
| `execFileSync` | `'inherit'` | Child inherits parent stdin — **v7.3.3 vector** on Windows/Bun if stdin is never closed |
| `execFile` (callback) | `'pipe'` | Child gets an internal pipe — lower risk but still not ideal for defense-in-depth |

**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:**
```javascript
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.

```javascript
// 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:

```typescript
// --- 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:

```bash
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.