fix(fabrika-cli): cut the excess-operand CLI suite to 5 spawns and take the network out of it - #4857
Conversation
…ke the network out of it (#4847) `excess-operand.cli.test.ts` spawned 12 subprocesses against a 30s per-test ceiling, and one of them — the variadic case — ran `adr resolve` inside this repo, where the verb does a real `git fetch origin main` before reading the base ref. That fetch is the 15.4s green baseline and the 41.3s timeout that ejected PR #4835 from the merge queue. A bigger ceiling would only have moved the threshold; the fetch is the cost. - The variadic case now runs from a scratch directory that is not a git repository, so `adr resolve` refuses at its first git read in milliseconds. It asserts the same thing plus the exit code, because the excess-operand check runs at parse time — before the verb reaches git — so a regressed catch-all still seats a usage error here. - The matrix drops to 5 representative invocations. Message wording and cross-group generality are covered in 19ms by `excess-operand.unit.test.ts`; help-exits-0 at three depths is covered by `unknown-subcommand.cli.test.ts`. What only a subprocess proves — the exit status — is kept, including the #4828 repro and the hidden catch-all staying out of `--help`. - `SUBPROCESS_TEST_TIMEOUT_MS` drops 30s to 20s, sized against the ~2.3s per-spawn CI baseline and stated against the 2.69x contention factor the issue measured. Local: 5.66s to 1.72s of test time, variadic case 1922ms to 349ms. Falsifiability re-proven both ways against injected regressions.
No preview deploy
|
|
review-code: PASS @ a77e817 — merge-ready Gated fresh against issue #4847. Class: has-code (1 changed file, Run-evidence bundle: PRESENT for head Acceptance criteria
The coder's three load-bearing claims, independently falsified1. The The 2. Timings reproduce in ratio. Re-measured on this machine (warm local git, fast link, so absolute numbers differ from the coder's — the ratio is the claim): refusal path 3. Falsifiability re-run here, both injections, on the head tree. Not accepted from the report:
4. CI evidence checked against the run, not the report — see AC 2. All 47 check runs at this head are 5. PR body intact after the reported blanking. 8,177 chars, all four Sub-gates
Non-blocking observations (no action required for this PR)
Reviewed at head Verdict-written: 2026-08-03T08:54:03Z |
1 participant
A test in
packages/fabrika-cliwas spawning twelve real subprocesses to check one guard, and one of those subprocesses fetched from GitHub every time it ran. That fetch took 15 seconds on a quiet runner and 41 seconds under merge-queue load, where it blew a 30-second ceiling and ejected an unrelated docs-only PR from the queue. This cuts the file to five spawns, none of which touch the network, and re-sizes the ceiling against what the job actually costs.PR #4835 is blocked on this landing. It is a docs-only diff that has now been ejected from the merge queue twice by this test, and nothing in its own diff touches
packages/fabrika-cli.The diagnosis, and why the fix is not a bigger number
The issue title frames this as "~15s against a 30s ceiling" — a headroom problem. It is not, and I checked before touching the constant.
fabrika adr resolve— the invocation in the timing-out case — callsloadMerged, which callsfetchAndResolve, which runs a realgit fetch --quiet origin mainagainst this repository before it reads anything (packages/fabrika-cli/src/io/git.ts,packages/fabrika-cli/src/adr/base-ref.ts). Measured on one machine:adr next extratoken(no git)adr resolve 0164 0023(fetchesorigin/main)adr resolve …from a non-repo cwdThat is a network round-trip inside a unit-tier test. It is the part with unbounded variance, it is why one test cost 15.4s of the file's 40.9s green baseline, and it is why the same test cost 41.3s under contention. Raising
SUBPROCESS_TEST_TIMEOUT_MSwould have left the fetch in place and moved the threshold — the partial mitigation the repo's standing rule says not to ship.What changed
The variadic case no longer reaches the network. It runs from a scratch directory that is not a git repository, so
adr resolverefuses at its first git read. The assertion is not weakened — it is strengthened. The excess-operand check runs at parse time, insideleafCommand's handler wrapper, before the verb reaches git, so a regressed catch-all still printsunexpected operandand seats exit1here. The case now also assertsBASE_UNFETCHABLE(3), which positively proves the verb ran rather than merely proving a string was absent;--repo owner/nameis what keeps the refusal on 3 instead of the ambiguous 1 the origin-remote lookup returns.Twelve spawns become five. Each spawn is a cold node+TS load of
bin.ts, ~2.3s on CI, so spawn count is the file's cost. Dropped, and where the coverage still lives:excess-operand.unit.test.ts—excessRefusal(…, ["a","b"])adrexcess-operand.unit.test.ts— its coverage guard walks every registered leaf--dir <dir> extra--helpandadr --helpexit 0unknown-subcommand.cli.test.ts— asserts this at four depths alreadyKept, because only a process proves them: the #4828 repro (exit 1, empty stdout, token and path named), refusal past a fixed arity with nothing written to the scratch dir, variadic absorption, a fixed-arity verb succeeding at its declared arity, and the hidden catch-all staying out of
--help.SUBPROCESS_TEST_TIMEOUT_MS: 30s to 20s, with a comment stating what it is sized for — about 8x the ~2.3s per-spawn CI baseline, against the 2.69x contention factor the issue measured between two runs 24 minutes apart.Evidence
Local, same machine, before and after:
The improvement on that case is larger on CI than locally, because the fetch here hits a warm local git over a fast link — 15.4s on the runner versus 1.9s here.
Falsifiability re-proven in both directions, by injecting regressions into
excess-operand.tsand reverting:code 1, not 3), as do the fixed-arity cases. 3 of 5 fail.excess.length === 0guard disarmed, restoring the fabrika: a leaf verb silently discards an undeclared extra token and exits 0 #4828 defect exactly: both refusal cases red. 2 of 5 fail.Neither injected defect can pass this file.
pnpm --filter './packages/**' --filter @kampus/infra run testexits 0 (fabrika-cli: 41 files, 554 tests).pnpm typecheckandpnpm lint:worktreeclean.Acceptance criteria
packages unit testsrun is the CI evidence, and its measured file duration gets appended to this body once it reports.packages/fabrika-cli/— done, no workflow change; see the mapping table.pnpm --filter './packages/**' --filter @kampus/infra run testgreen — done, exit 0.Fixes #4847
Deviations
1. Declined the in-passing
ci.ymlcomment correction (deferred sibling defect).packages-testsheader ("fast (~7s, ~500 tests)") "worth correcting in passing, not a separate unit"..github/workflows/ci.ymluntouched..github/**is control-plane (ADR 0053), so touching it moves this PR out of the autonomous merge lane and onto a human merge. This PR exists to unblock PR docs(release): the release runbook for builders and the release-path pattern doc (#4805) #4835 from the merge queue now; a one-line comment fix is not worth that latency.2. Narrowed the fix to the one file the issue scopes (sibling left for follow-up).
excess-operand.cli.test.ts.unknown-subcommand.cli.test.tsalone, which has the same 12-spawn / fixed-30s shape in the same gating job.3. Lowered the ceiling rather than raising it.
4. Strengthened the timing-out test's assertion while shrinking the file.
expect(run.code).toBe(BASE_UNFETCHABLE)to the variadic case, which previously asserted only that a string was absent.not.toContainalso passes if the process dies for an unrelated reason. Binding the exit code to the source constant makes "the verb ran and absorbed its operands" the thing actually proven.Update 2026-08-03 — the CI run log AC 2 asks for
This PR's own
packages unit testsjob (check run91636514592, heada77e817) reports:Against the 40,890ms green baseline recorded in the issue, on the same job: 40.9s -> 13.4s, a 3.05x reduction, with the network fetch gone. That works out to ~2.7s per spawn, which is the figure the new 20s ceiling was sized against — about 7.4x headroom on the slowest single case, clear of the 2.69x contention factor.