Skip to content

fix(pipeline-cli): the subprocess-budget guard scans every workspace member, not just its own package (#4858) - #5014

Merged
usirin merged 1 commit into
mainfrom
usirin/4858-subprocess-budget-workspace-scope-844FF9E6
Aug 9, 2026
Merged

fix(pipeline-cli): the subprocess-budget guard scans every workspace member, not just its own package (#4858)#5014
usirin merged 1 commit into
mainfrom
usirin/4858-subprocess-budget-workspace-scope-844FF9E6

Conversation

@usirin

@usirin usirin commented Aug 9, 2026

Copy link
Copy Markdown
Member

The guard that keeps subprocess-spawning tests off vitest's 5-second default only ever looked inside its own package. It ran on every PR, it passed every time, and it had never read a line of the other 28 workspace packages — where six suites were spawning real child processes with no budget at all, on the job whose timeouts eject a PR from the merge queue instead of just reddening it. This widens the guard's scan to the whole workspace, budgets those six suites, and collapses five scattered copies of the timeout constant down to one per package.

Fixes #4858

What changed

The guard now derives its scope instead of assuming it. packages/pipeline-cli/src/subprocess-budget.test.ts used to compute SRC from import.meta.url, which pinned it to packages/pipeline-cli/src/. It now walks up to pnpm-workspace.yaml, expands the declared packages: globs through the existing parseWorkspacePackageGlobs seam, and keeps the members the packages unit tests job actually runs (pnpm --filter './packages/**' --filter @kampus/infra run test). Scope follows the workspace declaration, so a new package cannot fall outside it by being forgotten.

Three assertions make an empty or collapsed scope red, per ADR 0092:

Six packages/pipeline-crew-mcp suites joined the tier. The issue listed three; a full-tree scan found six files / 14 describes with no budget:

file describes
packages/pipeline-crew-mcp/src/bin.test.ts 1
packages/pipeline-crew-mcp/src/crew/channel-server.socket.test.ts 1
packages/pipeline-crew-mcp/src/crew/tracker.rendezvous.socket.test.ts 1
packages/pipeline-crew-mcp/src/standup/ensure-tracker.test.ts 2
packages/pipeline-crew-mcp/src/tracker/rendezvous.test.ts 3
packages/pipeline-crew-mcp/src/tracker/server.test.ts 4

One constant per package, not five per-file copies. packages/fabrika-cli/src/ carried five local re-declarations at three different numbers (20s / 30s / 60s), and packages/pipeline-crew-mcp/ had none. Both packages now have a src/test-budget.ts on the same 60s ceiling as packages/pipeline-cli/src/test-budget.ts, and the guard asserts they agree — so the copies are provably identical rather than merely intended to be. It is one module per package because these members have no dependency edge between them; a test importing another package's source would invent one. .patterns/subprocess-test-budget.md records that reasoning.

Four per-test timeout literals removed. A trailing }, 30_000) on an it overrides the suite budget downward, which the pattern doc already bans and nothing enforced. Three sat in packages/pipeline-cli/src/tools/worktree-sweep/command.hook.test.ts (30s under a 60s suite) and one in packages/pipeline-crew-mcp/src/bin.test.ts; two more multi-line forms were in the crew-mcp suites above. A new assertion keeps them out.

Mutation evidence

Every mutant was checked for which assertion killed it, not merely that something went red. Restored to green between each.

# mutation result assertion that fired
1 drop {timeout} from ensure-tracker.test.ts's second describe 1 failed / 5 passed declares a {timeout: SUBPROCESS_TEST_TIMEOUT_MS} on every describe — named packages/pipeline-crew-mcp/src/standup/ensure-tracker.test.ts, which is the proof the scan leaves its own package
2 re-declare the constant locally in wire.cli.test.ts 1 failed / 5 passed imports the budget instead of re-declaring itpackages/fabrika-cli/src/wire/wire.cli.test.ts: declares SUBPROCESS_TEST_TIMEOUT_MS locally
3 drift pipeline-crew-mcp/src/test-budget.ts to 30_000 1 failed / 5 passed keeps every package's test-budget.ts on the one canonical ceilingexports 30000
4a empty scopeGUARDED_WORKSPACE_ROOTS = [] 4 failed / 2 passed derives a non-empty workspace scope (expected 0 to be greater than 0), plus the zero-tier-files, span, and budget-module asserts
4b a guarded root that matches no member ("tools") 1 failed / 5 passed every guarded root contributesexpected [ 'tools' ] to deeply equal []
5 scope collapsed back to packages/pipeline-cli only (the #4858 regression) 1 failed / 5 passed scans beyond its own packageexpected 1 to be greater than 1. Note the other five stayed green: this is the mutant the pre-#4858 guard could not detect
6 plant }, 30_000); on an it in bin.test.ts 1 failed / 6 passed writes no per-test timeout literal

Two false positives were caught while writing assertion 6 and fixed rather than tolerated: a first draft flagged total_tokens: 31_000 in a spawn-guard fixture, and a second flagged setInterval(() => {}, 1000) inside a spawned child's inline source. The shipped pattern keys on a numeric argument after a callback's closing brace at the start of a line, which biome's formatting guarantees for a real per-test override.

Scope resolution is physical (realpathSync before the walk up to pnpm-workspace.yaml), so a symlinked entry point cannot fold .. past the repo root into a false pass.

The blocking route, checked rather than assumed

No CI change was needed, and I verified that rather than assuming it:

  • .github/workflows/ci.ymlci-required declares needs: [changes, check, unit, packages-tests, actionlint, integration, e2e], so packages-tests is a genuine predecessor.
  • ci-required is one of three required status-check contexts on the main ruleset (17377992), alongside scan changed files for leaks and validate skill frontmatter.
  • packages-tests runs when packages_required is true, which this diff sets, and unconditionally on merge_group.

That is also why the guard's scope stops at packages/ + infra/: guarding a member this job never runs would be an assertion with no teeth. apps/* is out of tier for a separate reason recorded in the pattern doc — the only apps/web project whose tests spawn is integration, and apps/web/vitest.config.ts sets its testTimeout at the project level.

Verification

  • pnpm --filter './packages/**' --filter @kampus/infra run test (the job's exact command): green — pipeline-cli 3036, fabrika-cli 1254, pipeline-crew-mcp 469, all other packages green.
  • pnpm typecheck: 30/30 tasks green.
  • pnpm lint:worktree: clean.

§CP

pipeline-cli cp-classify classify returns control-plane [path-match]packages/pipeline-cli/src/subprocess-budget.test.ts matches the live CONTROL_PLANE_RE. This banks for a human merge rather than auto-shipping.

Deviations

Class: narrowed or changed a prior decision.

  • Said: AC 3 asks the packages/fabrika-cli/src/ re-declarations to either import a shared constant or have the pattern doc justify a package-local budget — "one source, not three copies."
  • Did: Gave the package one src/test-budget.ts at 60_000. That raises the 20_000 ceiling PR fix(fabrika-cli): cut the excess-operand CLI suite to 5 spawns and take the network out of it #4857 measured for excess-operand.cli.test.ts, and the same for wire.cli.test.ts.
  • Why: Five copies at three numbers cannot collapse without picking one, and only the maximum is safe — a timeout is an upper bound, so raising it cannot introduce a false red, while lowering deterministic-shell-observer.unit.test.ts from 60s could. fix(fabrika-cli): cut the excess-operand CLI suite to 5 spawns and take the network out of it #4857's actual fix was removing a git fetch from a unit test and cutting twelve spawns to five; neither is touched here, and its file still runs in 1.7s locally.
  • Disposition: For the reviewer to judge. If the measured 20s sizing should govern the package, the one-line change is the value in packages/fabrika-cli/src/test-budget.ts — the guard then holds all five files to it.

Class: fixed more than the issue named.

  • Said: Three unbudgeted pipeline-crew-mcp files, and the fabrika-cli re-declarations.
  • Did: Six unbudgeted files (triage flagged its list as a floor from code search, not a full-tree grep — the grep finds six), plus four per-test timeout literals, three of which are inside packages/pipeline-cli and were invisible to the old guard.
  • Why: The widened guard reds on all of them, so leaving any would have shipped a red gate.
  • Disposition: No action needed.

Class: declined a plausible wider fix.

  • Said: Nothing in the issue about apps/**.
  • Did: Left apps/* out of the guard's scope.
  • Why: Its two spawning files are in the integration project, which already carries a project-level 120s testTimeout, and that project runs in a different job than the one this guard blocks through.
  • Disposition: Recorded in .patterns/subprocess-test-budget.md as out-of-tier by construction, so a later reader does not read it as an oversight.

Class: chose not to add a CI surface.

  • Said: Nothing explicit.
  • Did: Added no workflow job and did not touch .github/workflows/ or packages/pipeline-cli/src/registry.ts.
  • Why: The guard already blocks through packages unit testsci-required, verified above. A second surface would be ceremony over an existing route.
  • Disposition: No action needed.

(repair round 1) Class: fixed more than the issue named.

  • Said: The five fabrika-cli re-declarations that existed when this PR was authored.
  • Did: Collapsed a sixth — packages/fabrika-cli/src/eval/stage-vocabulary.cli.test.ts, which landed on main from chore(fabrika-cli): re-key the eval harness's write-code stage to build (#4978) #5032 while this PR sat in review and declared its own SUBPROCESS_TEST_TIMEOUT_MS = 30_000. It now imports from packages/fabrika-cli/src/test-budget.ts like the other five.
  • Why: The merge queue ejected this PR on exactly that file: the widened guard scanned it and red. The guard is correct — the new file is the drift it exists to catch — so the fix is the file, not the guard. No guard assertion was weakened; all seven still pass.
  • Disposition: No action needed.

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

No preview deploy

  • No preview deploy for this PR — its diff touches no deploy-relevant path, so no preview stack was minted and e2e is not applicable. (f01b593)
  • web — Stage pr-5014 torn down.

@usirin

usirin commented Aug 9, 2026

Copy link
Copy Markdown
Member Author

review-code: advisory — blocking-set PR (§CP — approval-gated)

Every acceptance criterion on #4858 verified against the PR head. This carries no first-line @ <sha> by design (ADR 0111/0151): a §CP PR is never auto-mergeable off a gate verdict, and the reviewed head is bound in the body line below.

Reviewed-head: @ 9db1ae6

§CP derivation, not inherited from the lane. pipeline-cli cp-classify classify over the live changed-file set returns control-plane [path-match]packages/pipeline-cli/src/subprocess-budget.test.ts matches the live CONTROL_PLANE_RE. Corroborated against live CODEOWNERS on main: the /packages/pipeline-cli/src/* row is within-segment, and that file is a direct child of src/. This banks for a human @kamp-us/control-plane approval; it does not auto-ship.

Acceptance criteria

  • [PASS] Every test file in packages/** that imports node:child_process is covered by one guard, not one guard per package. The scan root is derived, not assumed: repoRoot() walks up to pnpm-workspace.yaml under realpathSync, parseWorkspacePackageGlobs expands the declared packages: globs, and GUARDED_WORKSPACE_ROOTS = ["packages", "infra"] keeps the members the blocking job runs. My own full-tree grep at head finds 32 test files importing node:child_process under packages/ — all inside the derived scope, none under infra/, and none written in a multi-line import form that the CHILD_PROCESS_IMPORT statement regex would miss. One guard file, resident in pipeline-cli, reaching every member.
  • [PASS] The pipeline-crew-mcp suites declare the shared budget at the suite. The issue named three; my own grep confirms six files, and all six carry {timeout: SUBPROCESS_TEST_TIMEOUT_MS} on every describe at head. Treating the issue list as a floor was correct — the issue itself flagged it as best-effort code search, not a census.
  • [PASS] The packages/fabrika-cli/src/ re-declarations resolve to one source. The criterion offers two routes: import a shared constant, or have the pattern doc justify a package-local budget. This PR takes both — packages/fabrika-cli/src/test-budget.ts and packages/pipeline-crew-mcp/src/test-budget.ts as the one place per package, with .patterns/subprocess-test-budget.md recording why it is per-package (workspace members have no dependency edge, so importing across would invent one). The guard proves the copies identical rather than merely intending it. See the deviation section below for the value chosen.
  • [PASS] The guard still fails closed on zero scope (ADR 0092) at its widened scope. Reproduced, not taken on report — see the mutation table.
  • [PASS] .patterns/subprocess-test-budget.md describes the scope the guard actually enforces. Verdicted under review-doc in a separate comment.

Mutation evidence — re-run independently, not accepted on report

Every mutant below was applied by me to a throwaway worktree at the PR head, run, and reverted; the baseline is 7 passed / 7. Each is recorded with the assertion that fired, because a mutant that dies for the wrong reason proves nothing.

# mutation result assertion that fired
1 drop {timeout} from the 2nd describe in ensure-tracker.test.ts 1 failed / 6 passed declares a {timeout: SUBPROCESS_TEST_TIMEOUT_MS} on every describe — named packages/pipeline-crew-mcp/src/standup/ensure-tracker.test.ts
2 re-declare the constant locally in wire.cli.test.ts 1 failed / 6 passed imports the budget instead of re-declaring itpackages/fabrika-cli/src/wire/wire.cli.test.ts: declares SUBPROCESS_TEST_TIMEOUT_MS locally
3 drift pipeline-crew-mcp/src/test-budget.ts to 30_000 1 failed / 6 passed keeps every package test-budget.ts on the one canonical ceilingexports 30000
4a empty scopeGUARDED_WORKSPACE_ROOTS = [] 4 failed / 3 passed derives a non-empty workspace scope, finds subprocess-spawning test files at all, and keeps every test-budget.ts on the ceiling, all three at expected 0 to be greater than 0; plus scans beyond its own package at expected 0 to be greater than 1
4b a guarded root matching no member ("tools") 1 failed / 6 passed every guarded root contributesexpected [ 'tools' ] to deeply equal []
5 scope collapsed back to packages/pipeline-cli only 1 failed / 6 passed scans beyond its own packageexpected 1 to be greater than 1; the other six stayed green
6 plant }, 30_000); on an it in bin.test.ts 1 failed / 6 passed writes no per-test timeout literalpackages/pipeline-crew-mcp/src/bin.test.ts: }, 30_000)

Three findings worth stating explicitly:

Paths were resolved physically throughout (cd -P / pwd -P, plus the guard own realpathSync before the walk to pnpm-workspace.yaml), so no symlinked .. folding is standing behind a pass.

One count correction, non-blocking. The PR body mutation table reports "/5 passed" for mutants 1, 2, 3, 4b and 5, and "/6 passed" for mutant 6, against a file that has seven it blocks. Those totals are consistent with the earlier mutants having been run before the per-test-literal assertion was added — that is, mutants 1 through 5 were not re-run against the final file. I re-ran all six against the shipped head myself and every one still dies for its stated reason, so the conclusion stands; but the table as written is not a record of the shipped file.

The blocking route — verified, not inherited

A guard that nothing in CI invokes is the defect class this drain has hit repeatedly, so I checked the whole chain at head rather than reading the claim:

  • .github/workflows/ci.ymlci-required declares needs: [changes, check, unit, packages-tests, actionlint, integration, e2e]. packages-tests is a genuine predecessor.
  • Ruleset 17377992 on main lists exactly three required status-check contexts: scan changed files for leaks, validate skill frontmatter, ci-required. Read live via REST.
  • packages-tests (job name packages unit tests) gates on needs.changes.outputs.packages_required == true, which resolves to github.event_name == merge_group || steps.filter.outputs.packages == true. This diff is entirely packages/** plus one .patterns/ doc, so the path filter fires on the PR, and the merge_group arm fires unconditionally on the batch.
  • The job runs pnpm --filter ./packages/** --filter @kampus/infra run test, which includes packages/pipeline-cli — so the guard executes on the blocking path.

Empirically confirmed: packages unit tests is success on this head. No CI change was needed, and declining to add a second surface is the right call.

Deviation: collapsing fabrika-cli to 60s raises the 20s ceiling #4857 measured — accepted

I read #4857 rather than accepting the disclosure at face value. It is a real deviation and the call is legitimate either way; I am accepting it, for reasons that outlive the disclosure:

My call: accept 60s. If the measured 20s should govern the package instead, it is a one-line change to packages/fabrika-cli/src/test-budget.ts and the guard then holds all five files to it — but I do not think that is worth a repair round.

Non-blocking observations

  • The guard scope slightly over-reaches the job it mirrors. Its docblock says GUARDED_WORKSPACE_ROOTS mirrors what packages unit tests runs, but that job selects only infra/ci-credentials via --filter @kampus/infra, while the guard scans every infra/* member — infra/depo included. This is the safe direction (over-guarding, not under-guarding) and has no live effect: neither infra member has a spawning test today. Worth softening the comment at some point; not worth a round-trip.
  • Tier membership is a direct node:child_process import. A test that spawns through an imported helper is not detected. packages/fabrika-cli/src/eval/deterministic-shell-observer.unit.test.ts is exactly that shape: it carries the budget because the author added it, not because the guard would have demanded it. This is inherited from the Three subprocess-spawning pipeline-cli tests time out at vitest's 5s default under load #4014 membership rule, unchanged and unwidened by this PR, and the acceptance criterion is written to that same rule — so it is not a regression. Naming it so a later reader does not mistake the tier for complete-by-construction against all spawning tests.

CI at head

Read live from REST check-runs on 9db1ae65. All three required contexts are success (ci-required, scan changed files for leaks, validate skill frontmatter), packages unit tests is success, and no check on the head is failing, errored, or still in progress. The skipped contexts (deploy, e2e, integration, lint workflow YAML) are legitimate not-applicable skips for a packages-only diff.

Verdict

All five acceptance criteria PASS. The regression this PR exists to prevent is genuinely detected — I reproduced the collapse mutant and watched it die on the one assertion built for it — and the zero-scope floor holds at the widened scope. Recorded as an advisory: the merge is authorized by a @kamp-us/control-plane approval at this head, not by this comment.

@usirin

usirin commented Aug 9, 2026

Copy link
Copy Markdown
Member Author

review-doc: advisory — blocking-set PR (§CP — approval-gated)

The docs class of this mixed diff is one file: .patterns/subprocess-test-budget.md. Verdicted in its own comment so its marker anchors to a first line (per the gate-verdict contract, markers are never stacked). No first-line @ <sha> by design (ADR 0111/0151); the reviewed head is bound below.

Reviewed-head: @ 9db1ae6

Class set for this PR. pipeline-cli class-probe classify --namespaces over the live 16-file changed set returns has-code, has-docsreview-code + review-doc. No skills/** or agents/** path, and no apps/web/src/ path, so review-skill and review-design are correctly absent. §CP is control-plane [path-match] on packages/pipeline-cli/src/subprocess-budget.test.ts.

The one acceptance criterion this gate owns

  • [PASS] .patterns/subprocess-test-budget.md describes the scope the guard actually enforces (it previously said "a pipeline-cli test"). The opening line now reads "every workspace member the merge-queue-gating packages unit tests job runs — everything under packages/ plus infra/ — not one package", and The guard section describes the derived scope, the four things it reds on, and the ADR 0092 empty-and-collapsed floor. I checked each claim against the code at head rather than against the prose.

Claim-by-claim check against the source

doc claim verified against result
scope derived from pnpm-workspace.yaml, kept to the members the job runs repoRoot() + parseWorkspacePackageGlobs + GUARDED_WORKSPACE_ROOTS in the guard accurate
reds on: a suite without the budget, a per-test timeout literal, a locally re-declared constant, a package budget off the canonical value the four corresponding it blocks; each mutation-killed in the review-code comment accurate
fails closed on an empty scope and a collapsed one (ADR 0092) mutants 4a and 5, reproduced by me accurate
one constant per package because members have no dependency edge the three src/test-budget.ts modules; no cross-package import exists accurate
a generous ceiling is not a weakened assertion — a timeout is an upper bound reasoning, and it is the same argument I accepted for the 60s deviation sound
apps/* is out of tier by construction: the only spawning apps/web project is integration, which sets testTimeout at the project level apps/web/vitest.config.ts — the integration project carries testTimeout: 120_000 accurate, checked directly

Doc hygiene

  • Right surface. .patterns/ is how the current code is shaped — correct home for a tier rule plus its guard. The why (the Three subprocess-spawning pipeline-cli tests time out at vitest's 5s default under load #4014 profiling) stays a pointer into the canonical test-budget.ts docblock rather than being re-derived here, and the historical drift belongs in the narrative, not in .decisions/. No surface confusion.
  • Links. All relative, all repo-relative, all resolvable: ../packages/pipeline-cli/test-budget.ts, ../packages/pipeline-cli/src/subprocess-budget.test.ts, ../.decisions/0092-gates-fail-closed-on-zero-scope.md, ./effect-testing.md, ./golden-real-payload-fixtures.md. Standard markdown, no wikilinks, no placeholders. The check docs have no dead internal links job is green on this head.
  • No local, home, or sibling-clone paths. Clean.
  • Prose. Tight; the doc gained scope without gaining bulk. The code example was updated with the one clarifying comment it needed (// your own package), which is the right amount given the rule changed from one shared module to one per package.

Two non-blocking nits

  • The doc repeats the issue undercount. It says the guard "had never looked outside packages/pipeline-cli/, where three suites elsewhere had no budget at all (The subprocess-test-budget guard scans only pipeline-cli, so three spawning suites elsewhere carry no budget at all #4858)". This PR itself established the real number is six — the issue flagged its own list as a best-effort code search, and the full-tree grep (which I reproduced) finds six files and 14 describes. The doc is the place a later reader will look for that number, so it should carry the corrected one.
  • *.test.ts undersells the walk. testFilesIn collects .test.ts and .test.tsx. Trivial, but the doc is otherwise precise enough that the omission reads as intentional.

Neither changes the scope the doc describes, so neither blocks the acceptance criterion. Both are a one-line fix if the author is touching the file again.

Verdict

PASS. The doc accurately describes the scope the guard now enforces, every falsifiable claim in it checks out against the source at head, and the surface, links and prose are clean. Recorded as an advisory: the merge is authorized by a @kamp-us/control-plane approval at this head, not by this comment.

@usirin usirin added the status:cp-banked Control-plane PR banked on the board, awaiting a control-plane approval label Aug 9, 2026
@usirin
usirin requested a review from notusirin August 9, 2026 03:55
notusirin
notusirin previously approved these changes Aug 9, 2026
@usirin
usirin added this pull request to the merge queue Aug 9, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 9, 2026
@usirin

usirin commented Aug 9, 2026

Copy link
Copy Markdown
Member Author

ship-it: merge-queue ejection detected — PR #5014 was enqueued but the queue dropped it without merging (still open, no longer queued, not merged). Routing back to repair/re-queue — this is NOT a shipped state.

Evidence (REST, at head 9db1ae6527250c13412bcc4feb5d0f64331d20bd)

  • added_to_merge_queue at 2026-08-09T04:22:25Z, removed_from_merge_queue at 2026-08-09T04:28:23Z — the removal is unpaired (no merged event), and merged: false / merged_at: null / state: open. Per ADR 0132 + merge-queue-classify, an unpaired removal is an ejection, not a landing.
  • No single-parent squash for this PR is reachable on main.

Cause: combined-batch CI failure, not a textual conflict. The batch run on gh-readonly-queue/main/pr-5014-… failed in packages unit testsci-required:

FAIL src/subprocess-budget.test.ts > imports the budget instead of re-declaring it — no per-file copies of the number
AssertionError: expected [ Array(1) ] to deeply equal []
+ "packages/fabrika-cli/src/eval/stage-vocabulary.cli.test.ts: declares SUBPROCESS_TEST_TIMEOUT_MS locally; import it from test-budget.ts"

This is a semantic conflict with a fresher base, exactly the class the merge queue exists to catch. While this PR sat in review, a sibling lane landed packages/fabrika-cli/src/eval/stage-vocabulary.cli.test.ts on main carrying its own local SUBPROCESS_TEST_TIMEOUT_MS. This PR's widened guard now scans that file and reds on it. Both halves are individually green; only the combination is not — which is why the PR's own head CI was fully green (45 contexts, 0 red) and the batch was not.

Repair: rebase onto current main, then replace the local declaration in packages/fabrika-cli/src/eval/stage-vocabulary.cli.test.ts with an import of packages/fabrika-cli/src/test-budget.ts (the same collapse this PR already performs for the other five fabrika-cli suites). Then re-review at the new head and re-ship — the §CP approval and both gate advisories are bound to 9db1ae65… and will be stale after the rebase (ADR 0058).

Merge intent: cleared (no arm left parked — ADR 0198).

…member, not just its own package (#4858)

The guard rooted its scan at its own directory, so it enforced the subprocess timeout tier inside packages/pipeline-cli and reported nothing about anywhere else. Six pipeline-crew-mcp suites ran real spawns against vitest's 5s default on the merge-queue-gating job, where a timeout ejects rather than reds.

Scope is now derived from pnpm-workspace.yaml, kept to the members the 'packages unit tests' job runs, and asserted three ways: non-empty, every guarded root contributed, and the tier spans more than one member. The last is the regression detector — re-narrowing the scan leaves every other assertion green.

Also folded six per-file re-declarations of the constant into one module per package, dropped four per-test timeout literals, and added the assertions that keep both from coming back. The sixth arrived on main while this branch sat in review (stage-vocabulary.cli.test.ts, from #5032) and is exactly the drift the widened scan is meant to catch.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@usirin
usirin force-pushed the usirin/4858-subprocess-budget-workspace-scope-844FF9E6 branch from 9db1ae6 to f01b593 Compare August 9, 2026 04:45
@usirin

usirin commented Aug 9, 2026

Copy link
Copy Markdown
Member Author

review-code: advisory — blocking-set PR (§CP — approval-gated)

Reviewed-head: @ f01b593

Fresh re-gate at a new head. The branch was rebased and force-pushed after the merge-queue ejection, so every verdict and the approval bound to the old head 9db1ae65 are void under ADR 0058. Nothing from that round was inherited — the diff was re-verified from scratch against f01b5935. The stale advisories and the stale @9db1ae65 approval are left untouched on this PR as historical record; they authorize nothing.

§CP classification (re-derived, not inherited). cp-classify classify against the live CONTROL_PLANE_RE returns control-plane [path-match]packages/pipeline-cli/src/subprocess-budget.test.ts matches the ^packages/pipeline-cli/src/[^/]+$ branch, and live .github/CODEOWNERS assigns /packages/pipeline-cli/src/* to @kamp-us/control-plane. So this PASS takes the advisory form (no bindable first-line @ <sha>; head bound above per ADR 0111/0151). A failing criterion would have been a bindable FAIL instead (ADR 0226) — there is none.

Acceptance criteria (issue #4858)

  • [PASS] Every packages/** test file importing node:child_process is covered by one guard, not one guard per package. The scan scope is derived from pnpm-workspace.yaml's declared members, filtered to the roots the merge-queue-gating packages unit tests job actually runs (packages, infra), then walked for *.test.ts / *.test.tsx. Scoping to that job is the right boundary — guarding a member the blocking job never runs would be an assertion with no teeth. Proven live by mutant 1 below: collapsing the member list back to packages/pipeline-cli reds, so the widened scan is load-bearing rather than incidental.
  • [PASS] The three packages/pipeline-crew-mcp suites declare the shared budget at the suite. src/tracker/rendezvous.test.ts, src/crew/tracker.rendezvous.socket.test.ts and src/standup/ensure-tracker.test.ts each import SUBPROCESS_TEST_TIMEOUT_MS from that package's src/test-budget.ts and carry it as the describe options object on every suite. Independently confirmed by the guard's own describe-coverage assertion running green over the real tree.
  • [PASS] The packages/fabrika-cli/src/ local re-declarations are collapsed to one source, or the pattern doc records why a package-local budget is correct. Both arms are satisfied. The six per-file const SUBPROCESS_TEST_TIMEOUT_MS copies are gone; each package now has exactly one src/test-budget.ts. The criterion explicitly permits a package-local budget provided the doc records why, and .patterns/subprocess-test-budget.md does: workspace members have no dependency edge between them, so importing another package's source would invent one. The copies are not on trust — the guard's seventh assertion reds if any package's module drifts off the canonical value (mutant 6 below).
  • [PASS] The guard still fails closed on zero scope (ADR 0092) at its widened scope. Two distinct zero-scope arms are asserted directly, not implied: a non-empty derived member list and every guarded root having contributed members, plus a non-empty spawning-file set and a non-empty budget-module set. Mutant 2 (empty roots) and the bonus mutant (a root that contributes nothing) both red on exactly these.
  • [PASS] .patterns/subprocess-test-budget.md describes the scope the guard actually enforces. The doc no longer says "a pipeline-cli test": it states the tier covers every member the packages unit tests job runs (packages/ plus infra/), describes the derived scope and all four red conditions, and records both fail-closed arms with the The subprocess-test-budget guard scans only pipeline-cli, so three spawning suites elsewhere carry no budget at all #4858 history. Its apps/*-is-out-of-tier claim is grounded, not asserted — apps/web/vitest.config.ts sets testTimeout: 120_000 at the project level on integration, the only apps/web project whose tests spawn.

The guard file has zero diff this round

Diffing 9db1ae65 against f01b5935 over packages/pipeline-cli/src/subprocess-budget.test.ts returns empty. The repair did not touch the guard. "Weaken the guard to make the batch pass" was the tempting wrong fix and it was not taken — the offending test file was fixed instead. All seven assertions still run and still pass, including the three that make the scan honest (non-empty scope + every-root-contributed, non-empty spawning set, and the #4858 scope-collapse detector).

Mutation harness — six mutants, each dying for its intended reason

Run against a throwaway worktree at f01b5935, paths resolved physically (cd -P / pwd -P). Baseline: 7/7 green.

# mutation result killing assertion
1 scan scope collapsed back to packages/pipeline-cli only (the #4858 regression) 1 failed, 6 passed scans beyond its own package — a scope collapsed back to one member reds (#4858)expected 1 to be greater than 1
2 GUARDED_WORKSPACE_ROOTS = [] (empty scope) 4 failed, 3 passed derives a non-empty workspace scope…expected 0 to be greater than 0, plus the two other zero-scope arms and the budget-module count
3 restore the #5032 local re-declaration in stage-vocabulary.cli.test.ts 1 failed, 6 passed imports the budget instead of re-declaring it — offender list length 1, naming exactly that file
4 strip the {timeout: SUBPROCESS_TEST_TIMEOUT_MS} option off a spawning suite 1 failed, 6 passed declares a {timeout: SUBPROCESS_TEST_TIMEOUT_MS} on every describe in every such file
5 add a trailing per-test literal }, 30_000) to an it 1 failed, 6 passed writes no per-test timeout literal — the suite budget is the only ceiling
6 drift packages/fabrika-cli/src/test-budget.ts to 30_000 1 failed, 6 passed keeps every package's test-budget.ts on the one canonical ceilingexports 30000
bonus add a guarded root that contributes no members 1 failed, 6 passed …and every guarded root contributes (ADR 0092)expected [ 'reports' ] to deeply equal []

Mutant 1 is the load-bearing one and it behaves exactly as required: it reds exactly one assertion and leaves the other six green. That is the proof the old, package-rooted guard could not have caught this class — every other property it asserts stays true within pipeline-cli, so "the tier spans more than one member" had to be asserted directly. Mutant 2 confirms the ADR 0092 arm: on an empty scope the three content assertions go vacuously green, and the zero-scope assertions are precisely what stops that from reading as a pass. Every mutant was reverted and the worktree verified clean before this verdict.

Re-declaration count against current live main

Re-checked against main @ 668d352e, not against the rebase base — main moved again after the rebase (one commit, #5035, keying review post's upsert on the carrier). Result:

The merge-queue ejection is therefore explained and closed: head CI was green at the old head, the batch reded because the widened guard correctly caught a file that landed on main mid-review. The guard was working; the repair fixed the offending file.

CI and typecheck at this head

  • Head CI: green over 45 contexts, read live from REST check-runs at f01b5935, latest-per-context. No failing, no running, no wedged. The blocking packages unit tests job is success, as are lint / format / typecheck and unit + client tests. The read succeeded, so this is an observed green, not an assumed one.
  • pnpm typecheck forced uncached: 30 successful, 30 total / 0 cached, 30 total in 19.1s. Not a FULL TURBO cache replay.

Non-blocking notes (no action required to merge)

  • The docblock in packages/fabrika-cli/src/test-budget.ts still says it "collapses five per-file copies". With chore(fabrika-cli): re-key the eval harness's write-code stage to build (#4978) #5032's file folded in it is now six — the same off-by-one the commit message was corrected for. Cosmetic and non-load-bearing (the guard, not the prose, enforces the count), so it does not fail a criterion. Worth a one-word fix if the branch is touched again for any other reason; not worth a round-trip on its own.
  • The 20s → 60s ceiling collapse is settled ground from the prior round and was not re-litigated: a timeout is an upper bound, so raising one cannot introduce a false red.

Method

Head pinned from the live PR read at the start of this run. The head was materialized read-only into a per-run ref and an isolated throwaway worktree; no working tree was ever checked out or switched. All GitHub reads via gh api REST with --paginate. No file in the repository was edited by this gate.

Verdict-written: 2026-08-09T04:59:07Z

@usirin

usirin commented Aug 9, 2026

Copy link
Copy Markdown
Member Author

review-doc: advisory — blocking-set PR (§CP — approval-gated)

Reviewed-head: @ f01b593

Fresh re-gate at a new head. The branch was rebased and force-pushed after the merge-queue ejection, so the prior review-doc verdict and the approval bound to 9db1ae65 are void under ADR 0058. Nothing was inherited; the doc surface was re-read from scratch at f01b5935. This PR is §CP by path (packages/pipeline-cli/src/subprocess-budget.test.ts), so this PASS takes the advisory form — head bound above, not on the first line (ADR 0111/0151).

Doc surface in scope: .patterns/subprocess-test-budget.md (the only prose artifact in the 17-file diff; the other 16 are code, gated separately under review-code).

Criteria

  • [PASS] The doc describes the scope the guard actually enforces (issue The subprocess-test-budget guard scans only pipeline-cli, so three spawning suites elsewhere carry no budget at all #4858's fifth criterion). The old text scoped the tier to "a pipeline-cli test". The new opening states it covers every workspace member the merge-queue-gating packages unit tests job runs — everything under packages/ plus infra/ — and "The guard" section replaces the old "scans every src/**/*.test.ts" with the actual mechanism: scope derived from pnpm-workspace.yaml, filtered to the members that job runs, walked for every *.test.ts. Verified against the guard source at this head; doc and code agree.
  • [PASS] Every red condition the guard enforces is documented. The doc now names all four (a suite without the budget, a per-test timeout literal, a locally re-declared constant, a package budget off the canonical value) plus both fail-closed arms — empty scope and collapsed scope. That matches the seven assertions one-for-one, with no documented rule the code does not enforce and no enforced rule the doc omits.
  • [PASS] The rule change is stated, not implied. The "One shared constant" bullet becomes "One constant per package… all on the same 60s ceiling", naming packages/pipeline-cli/src/test-budget.ts as canonical and giving the reason a reader would otherwise ask about: workspace members have no dependency edge, so a cross-package import would invent one, and the guard makes the copies provably identical. The example import line is annotated "your own package's" so a reader copying it does not reach across packages. This is the arm issue The subprocess-test-budget guard scans only pipeline-cli, so three spawning suites elsewhere carry no budget at all #4858's third criterion explicitly allows.
  • [PASS] Claims are grounded, per the CLAUDE.md rule on falsifiable platform claims. Two checked: (a) the apps/*-is-out-of-tier claim — apps/web/vitest.config.ts does set testTimeout: 120_000 at the project level on integration, the only apps/web project whose tests spawn; (b) the scope-boundary claim — .github/workflows/ci.yml's packages unit tests job is what the guard's roots mirror, and it is a blocking route via ci-required's needs:. Neither is asserted from intuition.
  • [PASS] The history is recorded where a future reader will hit it. The doc explains why both fail-closed arms exist — the guard spent its first life rooted at its own package, ran, passed, and had never looked — which is the non-obvious part a reader would otherwise strip as redundant. Cites The subprocess-test-budget guard scans only pipeline-cli, so three spawning suites elsewhere carry no budget at all #4858 and ADR 0092 by link.
  • [PASS] Doc hygiene. Standard markdown links with real resolvable relative paths, no wikilinks, no placeholders. No home-directory, machine-local, or sibling-clone paths. English technical prose, consistent with the surrounding .patterns/ corpus. Lands on the right surface: this is how the current code is shaped, which is .patterns/, not .decisions/ or reports/.

Non-blocking note

The doc is accurate as written. One cosmetic inconsistency lives in code, not here, and is recorded on the review-code verdict: packages/fabrika-cli/src/test-budget.ts's docblock still says "five per-file copies" where it is now six. No doc change is required for it.

Method

Head pinned from the live PR read at the start of this run, materialized read-only into a per-run ref and an isolated throwaway worktree — no checkout, no working-tree switch. All GitHub reads via gh api REST with --paginate. No file in the repository was edited by this gate.

Verdict-written: 2026-08-09T04:59:18Z

@usirin
usirin added this pull request to the merge queue Aug 9, 2026
Merged via the queue into main with commit 5e1259d Aug 9, 2026
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status:cp-banked Control-plane PR banked on the board, awaiting a control-plane approval

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The subprocess-test-budget guard scans only pipeline-cli, so three spawning suites elsewhere carry no budget at all

2 participants