fix(pipeline-cli): the subprocess-budget guard scans every workspace member, not just its own package (#4858) - #5014
Conversation
No preview deploy
|
|
review-code: advisory — blocking-set PR (§CP — approval-gated) Every acceptance criterion on #4858 verified against the PR head. This carries no first-line Reviewed-head: @ 9db1ae6 §CP derivation, not inherited from the lane. Acceptance criteria
Mutation evidence — re-run independently, not accepted on reportEvery 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.
Three findings worth stating explicitly:
Paths were resolved physically throughout ( 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 The blocking route — verified, not inheritedA 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:
Empirically confirmed: Deviation: collapsing fabrika-cli to 60s raises the 20s ceiling #4857 measured — acceptedI 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 Non-blocking observations
CI at headRead live from REST check-runs on VerdictAll 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 |
|
review-doc: advisory — blocking-set PR (§CP — approval-gated) The docs class of this mixed diff is one file: Reviewed-head: @ 9db1ae6 Class set for this PR. The one acceptance criterion this gate owns
Claim-by-claim check against the source
Doc hygiene
Two non-blocking nits
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. VerdictPASS. 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 |
|
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
Cause: combined-batch CI failure, not a textual conflict. The batch run on 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 Repair: rebase onto current 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>
9db1ae6 to
f01b593
Compare
|
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 §CP classification (re-derived, not inherited). Acceptance criteria (issue #4858)
The guard file has zero diff this roundDiffing Mutation harness — six mutants, each dying for its intended reasonRun against a throwaway worktree at
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 Re-declaration count against current live
|
|
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 Doc surface in scope: Criteria
Non-blocking noteThe doc is accurate as written. One cosmetic inconsistency lives in code, not here, and is recorded on the MethodHead 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 Verdict-written: 2026-08-09T04:59:18Z |
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.tsused to computeSRCfromimport.meta.url, which pinned it topackages/pipeline-cli/src/. It now walks up topnpm-workspace.yaml, expands the declaredpackages:globs through the existingparseWorkspacePackageGlobsseam, and keeps the members thepackages unit testsjob 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:
packages/pipeline-clileaves every other assertion in the file green — the tier really is complete in there — so "we are looking past our own package" has to be asserted directly, not inferred.Six
packages/pipeline-crew-mcpsuites joined the tier. The issue listed three; a full-tree scan found six files / 14describes with no budget:packages/pipeline-crew-mcp/src/bin.test.tspackages/pipeline-crew-mcp/src/crew/channel-server.socket.test.tspackages/pipeline-crew-mcp/src/crew/tracker.rendezvous.socket.test.tspackages/pipeline-crew-mcp/src/standup/ensure-tracker.test.tspackages/pipeline-crew-mcp/src/tracker/rendezvous.test.tspackages/pipeline-crew-mcp/src/tracker/server.test.tsOne constant per package, not five per-file copies.
packages/fabrika-cli/src/carried five local re-declarations at three different numbers (20s / 30s / 60s), andpackages/pipeline-crew-mcp/had none. Both packages now have asrc/test-budget.tson the same 60s ceiling aspackages/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.mdrecords that reasoning.Four per-test timeout literals removed. A trailing
}, 30_000)on anitoverrides the suite budget downward, which the pattern doc already bans and nothing enforced. Three sat inpackages/pipeline-cli/src/tools/worktree-sweep/command.hook.test.ts(30s under a 60s suite) and one inpackages/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.
{timeout}fromensure-tracker.test.ts's second describedeclares a {timeout: SUBPROCESS_TEST_TIMEOUT_MS} on every describe— namedpackages/pipeline-crew-mcp/src/standup/ensure-tracker.test.ts, which is the proof the scan leaves its own packagewire.cli.test.tsimports the budget instead of re-declaring it—packages/fabrika-cli/src/wire/wire.cli.test.ts: declares SUBPROCESS_TEST_TIMEOUT_MS locallypipeline-crew-mcp/src/test-budget.tsto30_000keeps every package's test-budget.ts on the one canonical ceiling—exports 30000GUARDED_WORKSPACE_ROOTS = []derives a non-empty workspace scope(expected 0 to be greater than 0), plus the zero-tier-files, span, and budget-module asserts"tools")every guarded root contributes—expected [ 'tools' ] to deeply equal []packages/pipeline-clionly (the #4858 regression)scans beyond its own package—expected 1 to be greater than 1. Note the other five stayed green: this is the mutant the pre-#4858 guard could not detect}, 30_000);on anitinbin.test.tswrites no per-test timeout literalTwo false positives were caught while writing assertion 6 and fixed rather than tolerated: a first draft flagged
total_tokens: 31_000in a spawn-guard fixture, and a second flaggedsetInterval(() => {}, 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 (
realpathSyncbefore the walk up topnpm-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.yml—ci-requireddeclaresneeds: [changes, check, unit, packages-tests, actionlint, integration, e2e], sopackages-testsis a genuine predecessor.ci-requiredis one of three required status-check contexts on themainruleset (17377992), alongsidescan changed files for leaksandvalidate skill frontmatter.packages-testsruns whenpackages_requiredis true, which this diff sets, and unconditionally onmerge_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 onlyapps/webproject whose tests spawn isintegration, andapps/web/vitest.config.tssets itstestTimeoutat 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 classifyreturnscontrol-plane [path-match]—packages/pipeline-cli/src/subprocess-budget.test.tsmatches the liveCONTROL_PLANE_RE. This banks for a human merge rather than auto-shipping.Deviations
Class: narrowed or changed a prior decision.
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."src/test-budget.tsat60_000. That raises the20_000ceiling PR fix(fabrika-cli): cut the excess-operand CLI suite to 5 spawns and take the network out of it #4857 measured forexcess-operand.cli.test.ts, and the same forwire.cli.test.ts.deterministic-shell-observer.unit.test.tsfrom 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 agit fetchfrom a unit test and cutting twelve spawns to five; neither is touched here, and its file still runs in 1.7s locally.packages/fabrika-cli/src/test-budget.ts— the guard then holds all five files to it.Class: fixed more than the issue named.
pipeline-crew-mcpfiles, and thefabrika-clire-declarations.packages/pipeline-cliand were invisible to the old guard.Class: declined a plausible wider fix.
apps/**.apps/*out of the guard's scope.integrationproject, which already carries a project-level 120stestTimeout, and that project runs in a different job than the one this guard blocks through..patterns/subprocess-test-budget.mdas out-of-tier by construction, so a later reader does not read it as an oversight.Class: chose not to add a CI surface.
.github/workflows/orpackages/pipeline-cli/src/registry.ts.packages unit tests→ci-required, verified above. A second surface would be ceremony over an existing route.(repair round 1) Class: fixed more than the issue named.
fabrika-clire-declarations that existed when this PR was authored.packages/fabrika-cli/src/eval/stage-vocabulary.cli.test.ts, which landed onmainfrom chore(fabrika-cli): re-key the eval harness'swrite-codestage tobuild(#4978) #5032 while this PR sat in review and declared its ownSUBPROCESS_TEST_TIMEOUT_MS = 30_000. It now imports frompackages/fabrika-cli/src/test-budget.tslike the other five.