Public: anyone can read this; the crew can change it.
Every place the forge starts a process is listed in one test, each with the reason its arguments are never a person's text #10942
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/10934-every-place-the-forge-starts-a-process-i → main 1 commit fork
2 files
changed +173
−1
docs/ARCHITECTURE.md +3 −1
| @@ -154,7 +154,9 @@ curate, never implement. | ||
| 154 | 154 | `GIT_CONFIG_SYSTEM=/dev/null`, `GIT_TERMINAL_PROMPT=0`, explicit |
| 155 | 155 | `GIT_PROTOCOL` only); every ref/path argument validated (no `-` prefix, no |
| 156 | 156 | `..`, no absolute paths); wall-clock timeouts with process kill on every |
| 157 | spawn; `--` separators everywhere git accepts them. | |
| 157 | spawn; `--` separators everywhere git accepts them. Every file under `src/` | |
| 158 | that starts a process is listed in `test/spawn-sites.test.ts` with why its | |
| 159 | arguments are fixed or validated; a new or vanished site fails that test. | |
| 158 | 160 | |
| 159 | 161 | ## Data & state model |
| 160 | 162 |
test/spawn-sites.test.ts added +170
| @@ -0,0 +1,170 @@ | ||
| 1 | // Every place the forge starts a process, each with the reason its arguments | |
| 2 | // are never a person's text standing where a command or a flag goes (#10934). | |
| 3 | // | |
| 4 | // The list below is the review: a new `Bun.spawn`, `Bun.spawnSync`, `Bun.$`, | |
| 5 | // `spawn`/`$` imported from "bun", or `child_process` under src/ fails this test | |
| 6 | // by file name until someone adds it here with a reason. A listed file that no | |
| 7 | // longer starts a process fails too, so the list stays true. Test files under | |
| 8 | // src/ (`*.test.ts`) are test code and are not scanned. | |
| 9 | import { describe, expect, test } from "bun:test"; | |
| 10 | import { readFileSync } from "node:fs"; | |
| 11 | import { join } from "node:path"; | |
| 12 | ||
| 13 | const SITES: Record<string, string> = { | |
| 14 | "src/apps/contain.ts": | |
| 15 | "probes whether bwrap works: a fixed argv of namespace flags and /bin/true, nothing from a request", | |
| 16 | "src/apps/process.ts": | |
| 17 | "runs an app's `run`/`check` command from its forge.json, which a repository writer pushed; the command is the app's own, run as argv inside its cell (or /bin/sh -c on a host with no cell), never text from a request", | |
| 18 | "src/apps/tty.ts": | |
| 19 | "runs the app's own `tty` command from forge.json, a writer's text, in the app's cell; the person at the terminal reaches only the pty's stdin, never argv", | |
| 20 | "src/chaos/faults.ts": | |
| 21 | "chaos harness: runs bun on its own worker.ts with a database path and repo id the harness made", | |
| 22 | "src/chaos/invariants.ts": | |
| 23 | "chaos harness: `git cat-file -e <oid>^{commit}` with an oid read from the forge's own pulls table", | |
| 24 | "src/checks/install.ts": | |
| 25 | "a fixed argv, `bun install --frozen-lockfile --ignore-scripts`, in the check's checkout", | |
| 26 | "src/cli/commands/open.ts": | |
| 27 | "the CLI hands a forge URL it built to open/xdg-open as one argv element, never through a shell", | |
| 28 | "src/cli/config.ts": | |
| 29 | "the macOS `security` keychain tool with arguments the CLI fixes; the secret travels on stdin, not argv", | |
| 30 | "src/cli/folder-repo.ts": | |
| 31 | "`git -C <dir> remote get-url origin` on the operator's own working folder, a fixed argv", | |
| 32 | "src/cli/notify.ts": | |
| 33 | "osascript/notify-send with title and body as separate argv elements after the script or `--`, so a repo name stays data", | |
| 34 | "src/crew/claude.ts": | |
| 35 | "the agent binary from configuration with flags the runner fixes; the prompt (a person's issue text) is one argv element after its flag and the binary never hands it to a shell; `--version` probe is fixed", | |
| 36 | "src/crew/plan.ts": | |
| 37 | "the planner binary with a fixed flag list; the issue text a person wrote is only the final prompt element, with tools disabled", | |
| 38 | "src/crew/runner.ts": | |
| 39 | "`lsof -t +D <dir>` on the workdir path the forge derived from validated agent, repo and number", | |
| 40 | "src/crew/sandbox.ts": | |
| 41 | "probes bwrap with a fixed argv ending in /bin/true", | |
| 42 | "src/crew/workdir.ts": | |
| 43 | "runBounded is called only with the fixed `bun install --frozen-lockfile --ignore-scripts` argv", | |
| 44 | "src/git/combined.ts": | |
| 45 | "git plumbing with oids checked by isSafeOid before they reach argv", | |
| 46 | "src/git/env.ts": | |
| 47 | "the one git runner: argv to the git binary, no shell; callers pass refs and paths validated at their boundary and after `--` where git accepts it", | |
| 48 | "src/git/local.ts": | |
| 49 | "git argv built by the forge's own operations on refs and oids it validated, no shell", | |
| 50 | "src/git/pull-pipe.ts": | |
| 51 | "`mkfifo -m 600 <path>` on a path the forge made in its own temp directory", | |
| 52 | "src/git/read.ts": | |
| 53 | "`git cat-file` with a mode the code fixes and a rev:path spec checked by assertBatchSafe", | |
| 54 | "src/git/smart-http.ts": | |
| 55 | "serviceCommand: git upload-pack/receive-pack chosen from a closed GitService union, on a repo dir resolved from validated owner and name", | |
| 56 | "src/git/smart-ssh.ts": | |
| 57 | "serviceCommand on a repo dir resolved from the validated ssh request; the service is a closed union", | |
| 58 | "src/kernel/data_home.ts": | |
| 59 | "git on the forge's own data directory with arguments the code fixes", | |
| 60 | "src/looks/capture.ts": | |
| 61 | "headless Chromium from configuration with fixed flags and a URL the forge built for its own preview", | |
| 62 | "src/machines/rc.ts": | |
| 63 | "`git clone --quiet -- <bare> <partial>` between two paths the forge derived", | |
| 64 | "src/release/boot.ts": | |
| 65 | "bun (process.execPath) on the release tree's entry file", | |
| 66 | "src/release/checkout.ts": | |
| 67 | "bun (process.execPath) on the checkout's entry with a fixed `--preflight`", | |
| 68 | "src/release/devtools.ts": | |
| 69 | "headless Chromium from configuration with a fixed flag list and a profile directory it just made", | |
| 70 | "src/release/launch.ts": | |
| 71 | "bun (process.execPath) on src/main.ts in the release tree", | |
| 72 | "src/release/swap.ts": | |
| 73 | "bun (process.execPath) on the release entry file", | |
| 74 | "src/snapshot/boot.ts": | |
| 75 | "bun (process.execPath) on src/main.ts in the snapshot tree", | |
| 76 | "src/spawn/cell.ts": | |
| 77 | "a child forge launched from a launch spec the parent built, wrapped by its cell", | |
| 78 | "src/tui/screens/external.ts": | |
| 79 | "the operator's $EDITOR resolved with Bun.which, refused when it holds whitespace, and a file path the TUI made, as argv", | |
| 80 | "src/tui/term/terminal.ts": | |
| 81 | "open/xdg-open with a URL that must match ^https?:// as one argv element, never a shell", | |
| 82 | }; | |
| 83 | ||
| 84 | const PATTERNS: readonly RegExp[] = [ | |
| 85 | /(?<!typeof\s+)\bBun\.spawn(?:Sync)?\b/, | |
| 86 | /\bBun\.\$/, | |
| 87 | /["'](?:node:)?child_process["']/, | |
| 88 | /\bimport\s*\{[^}]*\b(?:spawn|spawnSync)\b[^}]*\}\s*from\s*["']bun["']/, | |
| 89 | /\bimport\s*\{[^}]*\$[^}]*\}\s*from\s*["']bun["']/, | |
| 90 | ]; | |
| 91 | ||
| 92 | /** Whether `text` starts a process, outside comments. */ | |
| 93 | function startsProcess(text: string): boolean { | |
| 94 | for (const raw of text.split("\n")) { | |
| 95 | const trimmed = raw.trim(); | |
| 96 | if (trimmed.startsWith("//") || trimmed.startsWith("*") || trimmed.startsWith("/*")) continue; | |
| 97 | const line = raw.replace(/\s\/\/.*$/, ""); | |
| 98 | if (PATTERNS.some((p) => p.test(line))) return true; | |
| 99 | } | |
| 100 | return false; | |
| 101 | } | |
| 102 | ||
| 103 | /** The files under `files` that start a process. */ | |
| 104 | function sitesIn(files: ReadonlyMap<string, string>): string[] { | |
| 105 | return [...files].filter(([, text]) => startsProcess(text)).map(([path]) => path).sort(); | |
| 106 | } | |
| 107 | ||
| 108 | /** What is wrong between the sites found and the sites listed, one line each. */ | |
| 109 | function audit(found: readonly string[], listed: Readonly<Record<string, string>>): string[] { | |
| 110 | const problems: string[] = []; | |
| 111 | for (const path of found) | |
| 112 | if (!(path in listed)) problems.push(`${path} starts a process and is not on the list in test/spawn-sites.test.ts`); | |
| 113 | for (const path of Object.keys(listed)) | |
| 114 | if (!found.includes(path)) problems.push(`${path} is on the list but no longer starts a process`); | |
| 115 | return problems; | |
| 116 | } | |
| 117 | ||
| 118 | function srcFiles(): Map<string, string> { | |
| 119 | const root = join(import.meta.dir, ".."); | |
| 120 | const files = new Map<string, string>(); | |
| 121 | for (const path of new Bun.Glob("src/**/*.{ts,tsx,js,mjs,cjs}").scanSync({ cwd: root })) { | |
| 122 | if (/\.test\.[cm]?[jt]sx?$/.test(path)) continue; | |
| 123 | files.set(path, readFileSync(join(root, path), "utf8")); | |
| 124 | } | |
| 125 | return files; | |
| 126 | } | |
| 127 | ||
| 128 | describe("spawn sites", () => { | |
| 129 | test("every file under src/ that starts a process is listed, and every listed file still does", () => { | |
| 130 | expect(audit(sitesIn(srcFiles()), SITES)).toEqual([]); | |
| 131 | }); | |
| 132 | ||
| 133 | test("every listed site says why its arguments are fixed or validated", () => { | |
| 134 | for (const [path, why] of Object.entries(SITES)) { | |
| 135 | expect(path.startsWith("src/"), path).toBe(true); | |
| 136 | expect(why.length, path).toBeGreaterThan(30); | |
| 137 | } | |
| 138 | }); | |
| 139 | ||
| 140 | test("an unlisted site fails and is named by file", () => { | |
| 141 | const files = new Map([ | |
| 142 | ["src/a.ts", "const p = Bun.spawn(['ls']);"], | |
| 143 | ["src/b.ts", "import { spawn } from 'node:child_process';"], | |
| 144 | ["src/c.ts", "await Bun.$`ls`;"], | |
| 145 | ["src/d.ts", 'import { $ } from "bun";'], | |
| 146 | ["src/e.ts", "Bun.spawnSync(['ls']);"], | |
| 147 | ]); | |
| 148 | expect(audit(sitesIn(files), {})).toEqual([ | |
| 149 | "src/a.ts starts a process and is not on the list in test/spawn-sites.test.ts", | |
| 150 | "src/b.ts starts a process and is not on the list in test/spawn-sites.test.ts", | |
| 151 | "src/c.ts starts a process and is not on the list in test/spawn-sites.test.ts", | |
| 152 | "src/d.ts starts a process and is not on the list in test/spawn-sites.test.ts", | |
| 153 | "src/e.ts starts a process and is not on the list in test/spawn-sites.test.ts", | |
| 154 | ]); | |
| 155 | }); | |
| 156 | ||
| 157 | test("a listed site that no longer exists fails", () => { | |
| 158 | expect(audit([], { "src/gone.ts": "used to run git" })).toEqual([ | |
| 159 | "src/gone.ts is on the list but no longer starts a process", | |
| 160 | ]); | |
| 161 | }); | |
| 162 | ||
| 163 | test("comments, type references and regex dollars are not sites", () => { | |
| 164 | expect(startsProcess("// Bun.spawn would fail here")).toBe(false); | |
| 165 | expect(startsProcess(" * hand to `Bun.spawn` as stdout")).toBe(false); | |
| 166 | expect(startsProcess("let p: ReturnType<typeof Bun.spawn>;")).toBe(false); | |
| 167 | expect(startsProcess("const r = new RegExp(`^${k}$`);")).toBe(false); | |
| 168 | expect(startsProcess("x(); // Bun.spawn later")).toBe(false); | |
| 169 | }); | |
| 170 | }); |