From ccb7d4e8ff4c81f67f4cc8785e3528ad0807491f Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sun, 19 Jul 2026 16:00:13 -0700 Subject: [PATCH] FN-8367: enforce bounded engine shellouts Enforce bounded synchronous shellout use across the engine. - Audit every production synchronous shellout against a call-site allowlist. - Bound data-dependent git diff commands by timeout and output size. - Document the async shellout invariant and align focused command guards. Files changed: AGENTS.md | 2 +- docs/architecture.md | 1 + .../__tests__/engine-no-blocking-shellout.test.ts | 135 +++++++++++++++++++++ .../user-configured-command-no-execsync.test.ts | 5 +- packages/engine/src/merger-git-parse.ts | 16 ++- .../engine/src/merger-workspace-test-commands.ts | 27 ++++- 6 files changed, 181 insertions(+), 5 deletions(-) Fusion-Task-Id: FN-8367 Fusion-Task-Lineage: 976384e6-f283-4464-9f74-f328f2be3430 Co-authored-by: Fusion (runfusion.ai) --- AGENTS.md | 2 +- docs/architecture.md | 1 + .../engine-no-blocking-shellout.test.ts | 135 ++++++++++++++++++ ...ser-configured-command-no-execsync.test.ts | 5 +- packages/engine/src/merger-git-parse.ts | 16 ++- .../src/merger-workspace-test-commands.ts | 27 +++- 6 files changed, 181 insertions(+), 5 deletions(-) create mode 100644 packages/engine/src/__tests__/engine-no-blocking-shellout.test.ts diff --git a/AGENTS.md b/AGENTS.md index 660238a333..f5601b55ca 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -201,7 +201,7 @@ When you need a Fusion temp artifact, target the known prefix directly and list #### Never use `execSync` for user-configured commands -Run user-configured commands (test/build/workflow scripts) via async `exec` with timeout. `execSync` is only acceptable for short deterministic git plumbing. +Run user-configured commands (test/build/workflow scripts) via async `exec` with timeout. `execSync` is only acceptable for short deterministic git plumbing. `packages/engine/src/__tests__/engine-no-blocking-shellout.test.ts` enforces the engine-wide call-site allowlist for all synchronous shellout primitives. #### Move-Task contract diff --git a/docs/architecture.md b/docs/architecture.md index 7a60313a9d..1107406a22 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -647,6 +647,7 @@ See [Memory Plugin Contract](./memory-plugin-contract.md) for the full plan. ### Sandbox backend seam (FN-4636) - Engine user-configured command runners now route through `packages/engine/src/sandbox/` via a shared `SandboxBackend` abstraction (`resolveSandboxBackend()`), currently implemented only by the transparent `NativeSandboxBackend` passthrough (no behavior change). - The seam now covers both exec-shaped commands (`run`) and spawn-shaped verification commands (`runStreaming`), with `packages/engine/src/verification-utils.ts` delegating `runVerificationCommand`/`execWithProcessGroup` through `runStreaming`. +- **Async shellout invariant (FN-8367):** executor, scheduler, merger, self-healing, and dashboard activity share the engine's Node event loop, so user-configured or potentially long-running work must use bounded async execution. Production `execSync`, `spawnSync`, and `execFileSync` are restricted to the audited short git-plumbing call sites in `packages/engine/src/__tests__/engine-no-blocking-shellout.test.ts`; its call-site-level allowlist is the enforced source of truth. Data-dependent `git diff` is only permitted there when both `timeout` and `maxBuffer` bound it; otherwise it must use async `exec`/`execFile`. - Follow-up chain: FN-4637 (bubblewrap), FN-4638 (sandbox-exec), FN-4639 (settings selection), FN-4640 (run-audit telemetry), FN-4641 (action-gate), FN-4642 (container backends). - FN-4641 adds dedicated `sandbox_provisioning` approval-gate plumbing for first-time backend bootstrap. Backends call `requireSandboxProvisioningApproval()` (`packages/engine/src/sandbox/provisioning-gate.ts`) from `prepare()` when prerequisites are missing, and policy is resolved via `resolveSandboxProvisioningPolicy()` (`packages/core/src/sandbox-provisioning-policy.ts`). Initial callers land in FN-4637/FN-4638/FN-4642. - FN-4642 adds an experimental `ContainerSandboxBackend` (Podman-first, Docker-compatible) plus `buildContainerArgv()` for rootless container runs. It is opt-in only via explicit `resolveSandboxBackend({ backendId: "podman" | "docker" })` and is not wired through settings yet; known prototype limits are no SELinux `:Z` relabel on bind mounts, no filesystem policy beyond cwd bind-mounting, and a fixed default image (`docker.io/library/alpine:3.20`) with override via `FUSION_SANDBOX_CONTAINER_IMAGE`. diff --git a/packages/engine/src/__tests__/engine-no-blocking-shellout.test.ts b/packages/engine/src/__tests__/engine-no-blocking-shellout.test.ts new file mode 100644 index 0000000000..a2e4348064 --- /dev/null +++ b/packages/engine/src/__tests__/engine-no-blocking-shellout.test.ts @@ -0,0 +1,135 @@ +import { readdirSync, readFileSync } from "node:fs"; +import { join, relative } from "node:path"; +import { describe, expect, it } from "vitest"; +import { createSourceFile, forEachChild, isCallExpression, isIdentifier, ScriptTarget } from "typescript"; + +/* +FNXC:EngineAsyncInvariant 2026-07-29-00:00: +The engine's executor, scheduler, merger, self-healing, and dashboard activity +share one Node event loop. User-configured and potentially long-running work +must therefore stay async and bounded. This guard covers execSync, spawnSync, +and execFileSync across production source. + +The allowlist is call-site-level (path, line, and signature), not file-level, +and is the single enforced source of truth for sanctioned short git plumbing. +Data-dependent git diff calls are present only after proving timeout and +maxBuffer bounds in their production modules; new or unbounded sync shellouts +must migrate to bounded async execution instead. +*/ + +type SyncPrimitive = "execSync" | "spawnSync" | "execFileSync"; +type ShelloutSite = { + file: string; + line: number; + primitive: SyncPrimitive; + signature: string; +}; +type AllowlistEntry = ShelloutSite & { reason: string }; + +const SHORT_GIT_PLUMBING = "short deterministic git plumbing"; +const BOUNDED_GIT_DIFF = "bounded data-dependent git diff plumbing"; + +const allowlist: AllowlistEntry[] = [ + { file: "src/review-checkout.ts", line: 35, primitive: "execFileSync", signature: "const topLevel = execFileSync(\"git\", [\"rev-parse\", \"--show-toplevel\"], {", reason: SHORT_GIT_PLUMBING }, + { file: "src/worktree-prune.ts", line: 69, primitive: "execSync", signature: "execSync(\"git worktree prune\", {", reason: SHORT_GIT_PLUMBING }, + { file: "src/merger-git-parse.ts", line: 102, primitive: "execFileSync", signature: "const output = execFileSync(", reason: BOUNDED_GIT_DIFF }, + { file: "src/already-merged-detector.ts", line: 204, primitive: "execSync", signature: "branchTip = execSync(`git rev-parse --verify ${shellQuote(branchName)}`, {", reason: SHORT_GIT_PLUMBING }, + { file: "src/already-merged-detector.ts", line: 223, primitive: "execSync", signature: "execSync(`git merge-base --is-ancestor ${shellQuote(branchTip)} ${shellQuote(baseBranch)}`, {", reason: SHORT_GIT_PLUMBING }, + { file: "src/already-merged-detector.ts", line: 270, primitive: "execSync", signature: "branchTip = execSync(`git rev-parse --verify ${shellQuote(branchName)}`, {", reason: SHORT_GIT_PLUMBING }, + { file: "src/already-merged-detector.ts", line: 345, primitive: "execSync", signature: "execSync(`git rev-parse --verify ${shellQuote(treeBranchName)}`, {", reason: SHORT_GIT_PLUMBING }, + { file: "src/self-healing.ts", line: 4028, primitive: "execSync", signature: "const tipSha = String(execSync(`git rev-parse --verify ${shellQuote(branch)}`, {", reason: SHORT_GIT_PLUMBING }, + { file: "src/self-healing.ts", line: 4034, primitive: "execSync", signature: "const uniqueCommitCount = Number.parseInt(String(execSync(`git rev-list --count ${shellQuote(branch)} --not ${shellQuote(\"main\")}`, {", reason: SHORT_GIT_PLUMBING }, + { file: "src/self-healing.ts", line: 4071, primitive: "execSync", signature: "const branchesRaw = String(execSync(\"git branch --list 'fusion/*'\", {", reason: SHORT_GIT_PLUMBING }, + { file: "src/self-healing.ts", line: 12470, primitive: "execSync", signature: "execSync(`git branch -d ${shellQuote(branch)}`, {", reason: SHORT_GIT_PLUMBING }, + { file: "src/merger-workspace-test-commands.ts", line: 204, primitive: "execSync", signature: "changedFilesOutput = execSync(", reason: BOUNDED_GIT_DIFF }, + { file: "src/merger-workspace-test-commands.ts", line: 301, primitive: "execSync", signature: "changedFilesOutput = execSync(", reason: BOUNDED_GIT_DIFF }, + { file: "src/integration-branch.ts", line: 71, primitive: "execSync", signature: "const stdout = execSync(\"git symbolic-ref --short refs/remotes/origin/HEAD\", {", reason: SHORT_GIT_PLUMBING }, + { file: "src/integration-branch.ts", line: 107, primitive: "execSync", signature: "const stdout = execSync(\"git remote\", {", reason: SHORT_GIT_PLUMBING }, + { file: "src/merger.ts", line: 734, primitive: "execSync", signature: "const output = execSync(command, options);", reason: SHORT_GIT_PLUMBING }, + { file: "src/merger.ts", line: 781, primitive: "execSync", signature: "treeSha = execSync(\"git rev-parse HEAD^{tree}\", { cwd: rootDir, stdio: \"pipe\" })", reason: SHORT_GIT_PLUMBING }, + { file: "src/merger.ts", line: 1388, primitive: "execSync", signature: "execSync(\"git reset --merge\", { cwd: rootDir, stdio: \"pipe\" });", reason: SHORT_GIT_PLUMBING }, + { file: "src/merger.ts", line: 1600, primitive: "execSync", signature: "beforeRaw = execSync(\"git status -z --porcelain\", { cwd: rootDir, stdio: [\"ignore\", \"pipe\", \"ignore\"] }).toString(\"utf-8\");", reason: SHORT_GIT_PLUMBING }, + { file: "src/merger.ts", line: 1612, primitive: "execSync", signature: "afterRaw = execSync(\"git status -z --porcelain\", { cwd: rootDir, stdio: [\"ignore\", \"pipe\", \"ignore\"] }).toString(\"utf-8\");", reason: SHORT_GIT_PLUMBING }, + { file: "src/merger.ts", line: 5754, primitive: "execSync", signature: "execSync(\"git rev-parse --verify REBASE_HEAD\", {", reason: SHORT_GIT_PLUMBING }, + { file: "src/merger.ts", line: 7618, primitive: "execSync", signature: "execSync(`git rev-parse --verify \"${branch}\"`, {", reason: SHORT_GIT_PLUMBING }, + { file: "src/merger.ts", line: 8573, primitive: "execSync", signature: "execSync(\"git reset --merge\", { cwd: rootDir, stdio: \"pipe\" });", reason: SHORT_GIT_PLUMBING }, + { file: "src/merger.ts", line: 8586, primitive: "execSync", signature: "execSync(\"git reset --merge\", { cwd: rootDir, stdio: \"pipe\" });", reason: SHORT_GIT_PLUMBING }, + { file: "src/merger.ts", line: 8598, primitive: "execSync", signature: "execSync(\"git reset --merge\", { cwd: rootDir, stdio: \"pipe\" });", reason: SHORT_GIT_PLUMBING }, + { file: "src/merger.ts", line: 8936, primitive: "execSync", signature: "execSync(\"git reset --merge\", { cwd: rootDir, stdio: \"pipe\" });", reason: SHORT_GIT_PLUMBING }, + { file: "src/merger.ts", line: 8956, primitive: "execSync", signature: "execSync(\"git reset --merge\", { cwd: rootDir, stdio: \"pipe\" });", reason: SHORT_GIT_PLUMBING }, + { file: "src/merger.ts", line: 8965, primitive: "execSync", signature: "execSync(\"git reset --merge\", { cwd: rootDir, stdio: \"pipe\" });", reason: SHORT_GIT_PLUMBING }, + { file: "src/merger.ts", line: 9055, primitive: "execSync", signature: "execSync(\"git reset --merge\", { cwd: rootDir, stdio: \"pipe\" });", reason: SHORT_GIT_PLUMBING }, + { file: "src/merger.ts", line: 9630, primitive: "execSync", signature: "const postPushSha = execSync(\"git rev-parse HEAD\", {", reason: SHORT_GIT_PLUMBING }, + { file: "src/merger.ts", line: 10189, primitive: "execSync", signature: "const squashIsEmpty = execSync(", reason: SHORT_GIT_PLUMBING }, + { file: "src/merger.ts", line: 10223, primitive: "execSync", signature: "const squashIsEmpty = execSync(", reason: SHORT_GIT_PLUMBING }, + { file: "src/merger.ts", line: 10410, primitive: "execSync", signature: "execSync(\"git reset --merge\", { cwd: rootDir, stdio: \"pipe\" });", reason: SHORT_GIT_PLUMBING }, + { file: "src/executor.ts", line: 15748, primitive: "execSync", signature: "execSync(`git merge-base --is-ancestor ${task.baseCommitSha} HEAD`, {", reason: SHORT_GIT_PLUMBING }, +]; + +function scanSource(file: string, source: string): ShelloutSite[] { + // The TypeScript parser excludes comments and quoted literals from call + // expressions, avoiding false positives from documentation or examples. + const sourceFile = createSourceFile(file, source, ScriptTarget.Latest, false); + const sites: ShelloutSite[] = []; + const visit = (node: Parameters[0]): void => { + if (isCallExpression(node) && isIdentifier(node.expression)) { + const primitive = node.expression.text; + if (primitive === "execSync" || primitive === "spawnSync" || primitive === "execFileSync") { + const offset = node.expression.getStart(sourceFile); + const { line } = sourceFile.getLineAndCharacterOfPosition(offset); + sites.push({ + file, + line: line + 1, + primitive, + signature: source.split("\n")[line].trim(), + }); + } + } + forEachChild(node, visit); + }; + visit(sourceFile); + return sites; +} + +function listProductionSource(dir: string): string[] { + return readdirSync(dir, { withFileTypes: true }).flatMap((entry) => { + const path = join(dir, entry.name); + if (entry.isDirectory()) return entry.name === "__tests__" ? [] : listProductionSource(path); + return entry.isFile() && entry.name.endsWith(".ts") && !entry.name.endsWith(".test.ts") && !entry.name.endsWith(".spec.ts") ? [path] : []; + }); +} + +function scanEngineSource(): ShelloutSite[] { + const root = join(process.cwd(), "src"); + return listProductionSource(root).flatMap((path) => scanSource(relative(process.cwd(), path), readFileSync(path, "utf-8"))); +} + +function classifySites(sites: ShelloutSite[]): { unmatched: ShelloutSite[]; stale: AllowlistEntry[] } { + const remaining = new Set(allowlist.map((entry) => `${entry.file}:${entry.line}:${entry.primitive}:${entry.signature}`)); + const unmatched = sites.filter((site) => { + const key = `${site.file}:${site.line}:${site.primitive}:${site.signature}`; + if (!remaining.has(key)) return true; + remaining.delete(key); + return false; + }); + return { unmatched, stale: allowlist.filter((entry) => remaining.has(`${entry.file}:${entry.line}:${entry.primitive}:${entry.signature}`)) }; +} + +describe("engine blocking-shellout static guard", () => { + it("confines every production synchronous shellout to an audited call-site allowlist", () => { + const { unmatched, stale } = classifySites(scanEngineSource()); + expect(unmatched).toEqual([]); + expect(stale).toEqual([]); + }); + + it("flags a synchronous call in a non-allowlisted file", () => { + const { unmatched } = classifySites(scanSource("src/fake-runner.ts", 'const child = execSync("git status");')); + expect(unmatched).toHaveLength(1); + }); + + it("flags an extra synchronous call in an allowlisted file", () => { + const source = readFileSync(join(process.cwd(), "src", "worktree-prune.ts"), "utf-8") + '\nconst child = execSync("git status");\n'; + const { unmatched } = classifySites(scanSource("src/worktree-prune.ts", source)); + expect(unmatched).toHaveLength(1); + }); +}); diff --git a/packages/engine/src/__tests__/user-configured-command-no-execsync.test.ts b/packages/engine/src/__tests__/user-configured-command-no-execsync.test.ts index d92544e650..a019e7b07e 100644 --- a/packages/engine/src/__tests__/user-configured-command-no-execsync.test.ts +++ b/packages/engine/src/__tests__/user-configured-command-no-execsync.test.ts @@ -19,7 +19,10 @@ import { describe, expect, it } from "vitest"; * - packages/engine/src/sandbox/bubblewrap-backend.ts :: BubblewrapBackend.runBwrapSpawn — concrete bubblewrap spawn path uses setTimeout(options.timeoutMs) and options.maxBuffer. * - packages/engine/src/sandbox/sandbox-exec-backend.ts :: SandboxExecBackend.run — macOS isolating backend uses async exec with timeout, maxBuffer, and signal. * - * Explicit exclusions: git-only execSync in merger.ts, self-healing.ts, already-merged-detector.ts, integration-branch.ts, worktree-prune.ts, and executor.ts git merge-base ancestry checks. The guard slices only registry function bodies instead of asserting over whole files. + * Explicit exclusions: audited short git plumbing is enforced call-site-by-call-site + * by engine-no-blocking-shellout.test.ts (including review-checkout.ts and bounded + * data-dependent git diffs). This focused registry slices only user-command + * function bodies instead of asserting over whole files. */ type GuardEntry = { diff --git a/packages/engine/src/merger-git-parse.ts b/packages/engine/src/merger-git-parse.ts index bd31ce2f8e..1fd26390e8 100644 --- a/packages/engine/src/merger-git-parse.ts +++ b/packages/engine/src/merger-git-parse.ts @@ -7,6 +7,9 @@ */ import { execFileSync } from "node:child_process"; +const BOUNDED_GIT_DIFF_TIMEOUT_MS = 5_000; +const BOUNDED_GIT_DIFF_MAX_BUFFER = 10 * 1024 * 1024; + export function parseFailingFilesFromOutput(output: string): string[] { const paths = new Set(); @@ -86,6 +89,11 @@ export function quoteArg(value: string): string { * quoting on Windows cmd.exe, and parse NUL-delimited paths so whitespace/ * newlines in filenames are preserved. Empty array on git errors (unknown). * + * FNXC:EngineAsyncInvariant 2026-07-29-00:00: + * This data-dependent git diff remains short plumbing only with an explicit + * wall-clock timeout and bounded output. Do not remove either bound or add an + * unbounded synchronous shellout on the engine's shared event loop. + * * @internal Exported for testing only. */ export function getBranchChangedFiles(rootDir: string, baseBranch: string, branch: string): string[] { @@ -94,7 +102,13 @@ export function getBranchChangedFiles(rootDir: string, baseBranch: string, branc const output = execFileSync( "git", ["diff", "--name-only", "-z", `${baseBranch}...${headRef}`], - { cwd: rootDir, encoding: "utf-8", stdio: ["ignore", "pipe", "pipe"] }, + { + cwd: rootDir, + encoding: "utf-8", + stdio: ["ignore", "pipe", "pipe"], + timeout: BOUNDED_GIT_DIFF_TIMEOUT_MS, + maxBuffer: BOUNDED_GIT_DIFF_MAX_BUFFER, + }, ); return String(output).split("\0").map((f) => f.trim()).filter(Boolean); } catch { diff --git a/packages/engine/src/merger-workspace-test-commands.ts b/packages/engine/src/merger-workspace-test-commands.ts index c992315152..d741006d48 100644 --- a/packages/engine/src/merger-workspace-test-commands.ts +++ b/packages/engine/src/merger-workspace-test-commands.ts @@ -8,6 +8,9 @@ import { basename, dirname, join } from "node:path"; import { execSync } from "node:child_process"; import { mergerLog } from "./logger.js"; +const BOUNDED_GIT_DIFF_TIMEOUT_MS = 5_000; +const BOUNDED_GIT_DIFF_MAX_BUFFER = 10 * 1024 * 1024; + /** Shell-safe single-argument quoting for command composition. */ function quoteArg(value: string): string { return `'${value.replace(/'/g, "'\\''")}'`; @@ -173,6 +176,10 @@ export function packageNamesForFiles(rootDir: string, files: string[]): string[] * Returns null when scoping cannot be determined (missing git context, no * workspace file, root-only changes, etc.) — callers fall back to `pnpm test`. * + * FNXC:EngineAsyncInvariant 2026-07-29-00:00: + * The branch diff is data-dependent, so its synchronous git-plumbing call is + * allowed only with an explicit wall-clock timeout and bounded captured output. + * * @internal Exported for testing only. */ export function deriveScopedPnpmTestCommand(rootDir: string, baseBranch: string, branch: string): string | null { @@ -196,7 +203,13 @@ export function deriveScopedPnpmTestCommand(rootDir: string, baseBranch: string, try { changedFilesOutput = execSync( `git diff --name-only ${quoteArg(baseBranch)}...${quoteArg(branch)}`, - { cwd: rootDir, stdio: "pipe", encoding: "utf-8" }, + { + cwd: rootDir, + stdio: "pipe", + encoding: "utf-8", + timeout: BOUNDED_GIT_DIFF_TIMEOUT_MS, + maxBuffer: BOUNDED_GIT_DIFF_MAX_BUFFER, + }, ).toString(); } catch { return null; @@ -259,6 +272,10 @@ const TEST_FILE_RE = /\.(test|spec)\.(ts|tsx|js|jsx)$/; * files and test paths come from `git diff`, so every shell argument is quoted * via `quoteArg`. * + * FNXC:EngineAsyncInvariant 2026-07-29-00:00: + * The branch diff is data-dependent, so its synchronous git-plumbing call is + * allowed only with an explicit wall-clock timeout and bounded captured output. + * * @internal Exported for testing only. */ export function deriveFileScopedPnpmTestCommand( @@ -283,7 +300,13 @@ export function deriveFileScopedPnpmTestCommand( try { changedFilesOutput = execSync( `git diff --name-only ${quoteArg(baseBranch)}...${quoteArg(branch)}`, - { cwd: rootDir, stdio: "pipe", encoding: "utf-8" }, + { + cwd: rootDir, + stdio: "pipe", + encoding: "utf-8", + timeout: BOUNDED_GIT_DIFF_TIMEOUT_MS, + maxBuffer: BOUNDED_GIT_DIFF_MAX_BUFFER, + }, ).toString(); } catch { return null;