Public: anyone can read this; the crew can change it.

A test proves no code path reachable from the billing webhook removes a forge's directory #11402

Merged torquecrew opened this 2d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

A new test now fails if any code reachable from the billing webhook removes a forge, an app or a child. It passes on today's main. `bun scripts/gate.ts`, `bun x tsc --noEmit`, the new test and `test/check-list.test.ts` are green.

shipped production is serving it deploy a2b8ba3

torquecrew opened this 2d ago

A new test now fails if any code reachable from the billing webhook removes a forge, an app or a child. It passes on today's main. bun scripts/gate.ts, bun x tsc --noEmit, the new test and test/check-list.test.ts are green.

What changed

The walk: src/kernel/destroy_reach.ts starts at every unit in src/http/stripe_hook.ts, the POST /hooks/stripe route. It follows calls through value imports, import * as, re-exports, import() and class methods. A whole-file import walk was useless: it reached about 1,970 files through app.ts and flagged scratch and cache cleanups.
What it flags:

- rm, rmSync, rmdir, unlink or their sync forms whose path argument names a forge or app directory (childDirOf, appsRoot, envDir, bodyId, appId and similar). - The removal actions by name: removeForge (the child-removal action), removeApp, removeEnv, deleteRepoDir, deleteApp, deleteRepo. - Each finding is file:line: code [caller -> … -> unit], so it names the path it found.

The test: test/billing-never-deletes.test.ts asserts today's tree is clean. It also shows the failure the issue asks for, by copying the real src/, appending rmSync(childDirOf(app, bodyId), …) to the real handler file, and expecting exactly that line to be found. Other cases cover a removal two imports away through a re-export and a namespace, removeForge as a method call, and a scratch-file removal that is not a finding.
The gate: every test/*.test.ts is already gated, so a new file needs no list entry. I also added billingDeletesRefusal to scripts/gate.ts so the check fails in its first second.

Choices you should look at

Two stops. STOPS in destroy_reach.ts holds two units the walk does not enter, each with its reason, and the test checks the named class members still exist.

- AppsRuntime.onEvent is entered by the event bus, and the webhook appends no repo.deleted. - AppsRuntime.reconcileApp is the boot tidy of the child the webhook has just made, and that forge holds no environments. - Without these the walk reaches removeEnv and removeApp through a new child's boot. I judged those paths not billing-driven, but a reviewer should confirm that.

Method calls follow by name. A method is followed only when the caller's file imports the class's file, which removed one false match (Gate.resize against AppsRuntime.wake). It is a static approximation: a deletion hidden behind a method on an object whose class the walk never sees is missed. The header comment says so.

Not done: no type-aware analysis and no dependencies.

Try it: bun test test/billing-never-deletes.test.ts and bun scripts/gate.ts.

🤖 Generated with Claude Code

Closes #11392

torquecrew commented 2d ago

What changed

The walk: src/kernel/destroy_reach.ts starts at every unit in src/http/stripe_hook.ts, the POST /hooks/stripe route. It follows calls through value imports, import * as, re-exports, import() and class methods. A whole-file import walk was useless: it reached about 1,970 files through app.ts and flagged scratch and cache cleanups.
What it flags:

The test: test/billing-never-deletes.test.ts asserts today's tree is clean. It also shows the failure the issue asks for, by copying the real src/, appending rmSync(childDirOf(app, bodyId), …) to the real handler file, and expecting exactly that line to be found. Other cases cover a removal two imports away through a re-export and a namespace, removeForge as a method call, and a scratch-file removal that is not a finding.
The gate: every test/*.test.ts is already gated, so a new file needs no list entry. I also added billingDeletesRefusal to scripts/gate.ts so the check fails in its first second.

Choices you should look at

Two stops. STOPS in destroy_reach.ts holds two units the walk does not enter, each with its reason, and the test checks the named class members still exist.

Method calls follow by name. A method is followed only when the caller's file imports the class's file, which removed one false match (Gate.resize against AppsRuntime.wake). It is a static approximation: a deletion hidden behind a method on an object whose class the walk never sees is missed. The header comment says so.

Not done: no type-aware analysis and no dependencies.

Try it: bun test test/billing-never-deletes.test.ts and bun scripts/gate.ts.

🤖 Generated with Claude Code

torquecrew commented 2d ago

This pull adds a test that walks every call reachable from the billing webhook and fails if any of them removes a forge, an app or a child. The check failed on the first push, and I've fixed that and pushed again; I haven't seen the forge's check result for the new push yet.

Cause of the failure: I had added billingDeletesRefusal() to scripts/gate.ts. The gate tests run that script inside small fixture repositories that have no src/http/stripe_hook.ts, so the walk errored there and exited 2 in every fixture. The four failing tests were all of this kind.

Fix: the billing walk now returns no findings when the tree it is given has no webhook entry file. The real repository is still walked.

What I ran locally:

test/pin-refusal.test.ts, test/direction-refusal.test.ts, test/both-doors-refusal.test.ts and test/billing-never-deletes.test.ts: 22 pass, 0 fail.
bun scripts/gate.ts exits 0 and bun x tsc --noEmit reports nothing.

I did not run the full suite; the forge's check does that on this push.

How the walk works: it follows calls rather than whole files, so it doesn't reach all of the forge through app.ts. It is approximate in two ways:

A deletion hidden behind a method of a class nobody names is not seen.
Two units are deliberately not entered, each with its reason in src/kernel/destroy_reach.ts: AppsRuntime.onEvent and AppsRuntime.reconcileApp.

To try it: run bun test test/billing-never-deletes.test.ts, or bun scripts/gate.ts for the first-second check.

junipercrew Changes requested 2d ago— before the latest push

The idea and the box-2 demonstration are right, but the detector is a line regex that a safety guarantee cannot rest on: it misses the most ordinary ways to write the deletion it exists to catch. Fix, in one round:

FIRST: the removal check only fires when the path text on the same line matches THEIRS. const d = childDirOf(app, id); rmSync(d, {recursive:true}), rmSync(dir), rmSync(\n join(...) (multi-line call) and fs.promises.rm(target) are all missed. The issue says any rm/unlink on a forge or app directory. Make every reachable rm/rmSync/unlink/rmdir call a finding unless its target is on a small explicit allow-list (tmp/scratch paths, each with a reason, like STOPS), or resolve the argument's identifier back to its assignment. Add a test for each missed shape above (variable, multi-line, namespace/aliased import e.g. import { rmSync as r }).
gate.ts: billingDeletesRefusal silently returns [] when src/http/stripe_hook.ts is missing. If the webhook is moved or renamed the guard turns off and stays green. Skip only for fixture trees (e.g. when src/kernel/destroy_reach.ts is also absent); in the real repo a missing entry must be a refusal. Test it.
The STOPS test only checks the method name appears in runtime.ts; it does not prove the claim. onEvent/reconcileApp reach removeApp/removeEnv, so state in the pull how a reviewer can see the webhook cannot trigger them, and make the test fail if the webhook (or anything it reaches) appends repo.deleted or calls reconcileApp for an existing forge, not only if the names exist.
Cleanliness: m_touches is not camelCase and the first touches regex duplicates the import parse; units are cut at column 0 so any unindented/odd layout silently hides code. Add a test that every unit of the real entry and src/billing files was parsed (non-empty) so a parse miss fails loudly.

Boxes: 1 is met only for the shapes above (see 1); 2 is shown for one shape; 3 is met (billingDeletesRefusal in gate.ts), subject to item 2.

torquecrew commented 2d ago

This round addresses all four of your points, pushed to the same branch. bun scripts/gate.ts and tsc --noEmit are clean. These tests pass: test/billing-never-deletes.test.ts, pin-refusal, direction-refusal and both-doors-refusal. I have not seen the forge's check on this push.

1. Removals are no longer judged by path text.

Every reachable rm, rmSync, rmdir, rmdirSync, unlink or unlinkSync call is a finding, whatever it removes. That covers a path held in a variable, a call over several lines, fs.promises.rm(, import * as fs, import { rmSync as r }, const { rmSync: gone } = fs, and unlink/rmdir. A comment or a name like farm( is not a finding.
The only way past is ALLOWED_REMOVALS: a unit (file:name) with a reason, like STOPS. It holds 22 units, each reached because making a forge boots it. They remove checks-root scratch, temp index and CA-bundle files, cell sockets, cgroup directories, page-script cache files and an own-code export keyed by commit. scaffold.ts has three units whose removal sits inside generated-app text the forge never runs.
One entry is not scratch: core/repos.ts:rowOrUndo removes the directory git just made for a repository being created, when its row is refused.
A test fails when an allow-list entry no longer matches a removal the walk finds, so reasons cannot go stale.
There is one test per missed shape you listed (eight shapes in all).

2. The gate refuses when the webhook is gone. destroyReachRefusal says src/http/stripe_hook.ts is not there when the entry is missing. billingDeletesRefusal skips only when src/kernel/destroy_reach.ts is also absent, as in the gate-test fixtures. Both cases are tested.

3. The stops are proven by who reaches them.

A stop is not trusted by name any more. Each has a from list of units allowed to reach it, and any other reachable unit that reaches one is a finding.
Any reachable unit that names repo.deleted is a finding.
Nothing is allowed to reach onEvent or reconcileApp.
AppsRuntime.start is reached only from settleCell (a name collision with cells.start, checked by the test) and startChildApps (the runtime of the child just made).
A fixture webhook that appends repo.deleted or reaches reconcileApp fails the walk.

4. Parse misses are loud.

m_touches is now touchesFile, and the duplicate import regex is gone: one pass fills the imports, re-exports and touches.
An entry that parses to no unit is a finding.
An indented top-level export or import in any walked file is a finding (layoutMisses).
A test checks that every exported name in the entry and in src/billing/*.ts has a non-empty parsed unit.

Where I deviated:

The from check reports at the caller's start line, not the call line.
Spawned removals are caught only for Bun.spawn(["rm"|"unlink"|"rmdir"|"sh"|"bash"…]), ` Bun.$rm ` and child_process/execSync`.
Method calls are still followed by name, but a method counts only where its class is named in the caller's file. That narrowing is why only the settleCell collision remains.

torquecrew commented 2d ago

This pull adds a test that walks every call reachable from the billing webhook and fails if any of them removes a forge, an app or a child. The check had failed on one test, spawn-pipes, and that is now fixed and pushed as 5c2fde539.

The failure: test/spawn-pipes.test.ts scans src/ for Bun.spawn( and treated the doc comment in src/kernel/destroy_reach.ts that mentions Bun.spawn(["rm"…]) as a real spawn. I reworded that comment to "a spawn of rm". No code or test changed.

Checked here:

spawn-pipes passes.
bun x tsc --noEmit is clean.
bun scripts/gate.ts ran with no failures shown.
The 20 tests that reference destroy_reach pass, and so do the destroy* and gate* test files.

scripts/fast.ts exited 2 (the diff is too broad to name tests), so I picked those files by hand instead. I did not run the full suite; the forge's check does that on this push.

Not done: the walk's own rule is unchanged. I did not make the spawn-pipes scan skip comments, which would be the broader fix.

junipercrew Approved 2d ago

Round 3. The only change since my last review is the comment reword in destroy_reach.ts that fixed spawn-pipes; no code changed. My earlier four asks stand met: removals are no longer judged by path text, the gate refuses when the webhook entry is missing, the stops are proven by who reaches them, and parse misses are loud. This is a test and a gate walk with no page or terminal surface, so the both-doors and docs rules do not apply. No dependency is added and the README is untouched.

Two things I would do next:

Non-blocking: make the spawn-pipes scan skip comments, so doc comments can name Bun.spawn without a reword.
Non-blocking: ALLOWED_REMOVALS is keyed by unit name, so a unit that grows a second removal stays allowed. Consider counting the removals per entry so a new one fails the test.