ci: run tests, typecheck and lint on pull requests and pushes to main #80
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "ci/73-72-run-tests-and-browser-guard"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #73.
This changes CI only — no application code, so merging deploys nothing new. But it lands on
main, which firesbuild-publish.ymlanddeploy-store.yml, so a merge is still a production release of both apps.The gap
No workflow ran any test suite, on any trigger.
Both matches are
runs-on: ubuntu-la**test**. Around 91 tests inapps/store, 280 inapps/managementand thepackages/catalogsuite executed only on a developer's or agent's machine. A green run meant "the container built" or "wrangler accepted the upload" — never that a test passed.It has already cost real work: a 623-line regression guard was deleted from PR #67, and one of the three decisive arguments was that it would never have executed. #72 is blocked on this issue for the same reason.
What ships
.forgejo/workflows/checks.yml—pull_request,push: [main],workflow_dispatch,workflow_call— with three jobs: test (matrixed per package), typecheck (per app), lint..forgejo/scripts/biome-gate.sh— the lint step, and the part worth reading, because a naive lint step here is permanently green for two independent reasons:npx biomeresolves to an unrelated registry package namedbiome(0.3.3), which checks nothing, prints nothing, and exits 0. There is no biome innode_modules; the only one is the nix binary onPATH. So the gate asserts the output containsChecked N filesand fails hard if that line is absent regardless of exit code — which is exactly where a silentnpxrun dies.Found N errors/warnings/infosand compares against a recorded baseline of 34 errors / 12 warnings / 3 infos, failing when any count grows and reporting when one shrinks so the baseline can be lowered.Baselining rather than fixing 46 pre-existing findings is the recorded choice #73 asked for. It makes the gate useful immediately without a 46-finding cleanup blocking it, and it cannot silently absorb new findings.
Verified — the probe PR is the evidence
#74 is a throwaway PR that exists solely to prove the checks fire on a pull request and can each be made to fail. It is not for merging and should be closed with this.
From
/actions/tasks, two full rounds on headf8e41833, eventpull_request:test @garage/catalogtest garagetest garage-storetypecheck apps/managementtypecheck apps/storelint (biome)probeSo: the workflow fires on a pull request, each job runs independently, and every check has been observed failing on purpose — the acceptance criterion, met by measurement rather than by reading the YAML. Runs 988–993 are the earlier round.
Locally, at this tree, unpiped:
And the gate was made to fail on purpose: with an unfinished script present in the tree the same command reported
errors 36 (baseline 34)/warnings 15 (baseline 12)and exited 1, naming the growth and refusing to raise the baseline. That unfinished script is #72's partial work and is not in this PR — see below.Deviations
workflow_callis exposed so the deploy workflows can depend on this one, but they are not wired to it in this PR. #73 asks for that decision to be recorded: todaypush: mainstill deploys unconditionally, so a merge that breaks the tests still ships to production for both apps. Gating the deploy is the more valuable half and it is deliberately left as a separate change, because it makes a flaky check block releases and that trade wants its own review. Say if you would rather it were wired here.wip/72-rendered-cascade-partial(commit55d750f, do not merge) so the work is not lost — it fails this very gate, catches none of attack 28's six variants, and is not mutation-verified. #72 stays open and now has a concrete starting point.45cf89901cae2786bd5aReview —
ae2786b(first verdict on this PR)Verdict: approve with nits. 0 blockers, 2 should-fix, 3 nits.
Round 1 posted nothing, so I assumed nothing was confirmed and re-derived everything from scratch in a throwaway worktree at
ae2786b. Every check below was made to fail on purpose at least once, and every mutation was diffed or re-measured after applying it rather than assumed — two of my own probes silently no-oped (python3is absent on this box; asedrange never matchedBASELINE=$() and would have produced confident nonsense. The gate was always run unpiped with$?read directly.1. The gate is no longer payable — the centre of the review holds
The original cancellation attack is dead. Fixed one
formatfinding (components/ui/alert.tsx) and added a realdebugger;in the same tree. Measured delta via biome's JSON reporter:format 18→17,noDebugger 0→1. The aggregate severity line came out— byte-identical to a clean baseline run, which is exactly what paid off the old severity gate. Per-rule,
EXIT=1, reporting both halves:debugger;alone:EXIT=1, and the measured histogram delta was onlynoDebugger +1, so nothing else moved to explain it.New rule at count 0 cannot slip in: absent-from-table with any count > 0 fails hard. Rule rename fails closed — I renamed a table key and got
FAIL: new rule lint/style/useTemplateplusRATCHET: …useTemplateRenamed is gone,EXIT=1. A biome upgrade cannot read as "gone + new" and pass.Parse correctness. The
checktrailer is genuinely a banner, not a finding: its body is "Some errors were emitted while running checks", and the JSON reporter returns 0 diagnostics with categorycheck. Dropping it is right. I also cross-checked the script's own text parse against--reporter=json— identical, 13 rules.format/parse/deserialize(noline:col) and lint rules (with it) all key correctly, andFIXABLEis stripped in the right order (box rule first, then trailing marker).On the same-rule residual — I agree with your ruling, and it is honestly stated. Confirmed: format-fix one file + break formatting in another gives a measured histogram of "no change" and
EXIT=0. But the fungible pool is dominated byformat(18 of 47), where a swap is cosmetic; every behavioural rule sits at 1–4, so paying one off means fixing a same-kind finding in the same rule, which is a real improvement; and the dangerous rules (noDebuggeret al.) are absent from the table entirely, so any new one fails hard.<path> <rule>fingerprints would churn on every rename and get deleted the first time they block one. Rule-level is the correct rung and the upgrade path is recorded.2. The summary-parsing hole is closed, and a clean tree passes
Against stub binaries pinned to
Version: 2.4.16so the real resolution path was exercised:npx biomeimpostor)EXIT=1— noChecked N filesFound 34 problems.EXIT=1— summary line not understoodChecked 12 filesEXIT=1— below floorChecked 240 files, noFoundlineEXIT=0, PASSED with all 13 RATCHET linesThe
pipefailfix is real and load-bearing. I ran theRULESpipeline on empty input both ways: with the|| trueguards it survives (exit 0, empty result); without them it dies (exit 1). I audited every other pipeline for the same death —FILES/count()aresed | head(both exit 0, andFILESis guarded by the precedinggrep -q), bothgrep -ccalls carry|| true, and the two unguarded pipelines are reachable only on branches where a match provably exists. No other empty-grep deaths.Truncation direction is correct. 210 synthetic findings pushed past
--max-diagnostics=200; it failed loudly rather than grading a sample:summary says 257 findings (242/12/3)vsdiagnostic headers say 200,EXIT=1. Neither of biome's extra lines ("The number of diagnostics exceeds…", "Diagnostics not shown: 57.") starts withFound, so they don't trip the summary assertion — the reconcile catches it instead.Trap 1's premise is factually true. Registry check:
biomelatest is0.3.3, described as "A simple way to manage environment variables on a per-project basis". Exactly as documented.One scoping note, not a defect: the
Checked N filesassertion is satisfied by any binary that prints the line. A stub that claims 240 files and checked nothing passes (EXIT=0). That's fine against the known impostor, which prints nothing — but it means the assertion is a smoke test, not proof of work. It matters only in combination with should-fix #1 below.3. Baseline re-measured — exact
Re-measured independently on the rebased tree with
--reporter=json, then diffed against theBASELINEtable: matches finding-for-finding, 13 rules / 47 findings, and the gate emits zero RATCHET lines (240 files, floor 200). The staleness is genuinely fixed, not merely rebased over.4. The workflow fires, and each check can fail
It fired on its own PR — run
181, eventpull_request, all six jobssuccess:test @garage/catalog,test garage,test garage-store,typecheck apps/store,typecheck apps/management,lint (biome). (The previous head45cf8990ran all six too.) No blocker here.workflow_callis proven. Tasks 994–1000 all carryworkflow_id=probe-call.ymlatrun_number=120, yet that run contains the six job names that exist only inchecks.yml, alongside the caller jobprobe— andprobewent red when they did. So Forgejo resolveduses: ./.forgejo/workflows/checks.ymland propagated failure to the caller, which is precisely what a deploy gate needs. Your "two-line edit" claim is measurement-backed.Probe attribution is sound. I diffed
f8e41833againstmain: three single-purpose sabotage files, all underapps/store—ci-probe-fail.test.ts(assert.equal(1,2)),ci-probe-lint.ts(const lintProbe = 1;+debugger;),ci-probe-types.ts(const broken: number = "not a number"). The observed pattern matches file location exactly:typecheck apps/storered /apps/managementgreen,test garage-storered /garage+@garage/cataloggreen,lintred. Those three greens are the controls that rule out an incidental cause — a broken install or bad Node would have reddened them too. Each failure is attributable to the sabotage it claims.Caveat worth recording: the probe branch carries the older 172-line gate and 195-line
checks.yml, not the 314/209-line versions shipping here. So the probe proves the job wiring can go red; that the shipped per-rule gate goes red is what I proved myself in §1. Also, log bodies are not retrievable on this forge —actions/{jobs,runs,tasks}/…/logsall 404 and the run's artifact list is empty — so my forge evidence is job-level conclusions plus the diff, not log text. I'm not claiming to have read the logs.5. The deferred deploy gate — I agree it should not have been wired here
Correcting the comment rather than the wiring was the right call, and for a stronger reason than "it deserves its own review": this gate's baseline went stale once inside a week, and a stale table's failure mode is a red gate on an unchanged tree. Wiring that onto a path that currently ships on every
mainpush converts lint-baseline drift into a release outage. The task list corroborates the noise argument directly — I can see the long run ofworkflow_dispatchdeploys of the single unchanged commitd0b58259, manycancelled.But the deferral leaves a real, currently-true production risk: a merge that breaks the tests still deploys both apps. That is now documented in a YAML comment only. It needs a tracking issue, otherwise the comment is where the risk goes to die. And see should-fix #1 — I would not wire a deploy gate onto this script until shrinkage is fatal.
6. Also verified
testscript —garage,garage-store,@garage/catalog— and all three are in the matrix. Rootgarage-monorepo(turbo run test) is the aggregate you deliberately excluded. Exactly twotsconfig.jsonfiles exist (apps/store,apps/management); both are covered, andpackages/cataloggenuinely has none, as the comment says.@garage/catalog24/24,garage370/370 (17 files),garage-store110/110. All green, matching the stated counts exactly.pnpm --filterguard works in both directions. The hazard is real —pnpm --filter does-not-exist testprints "No projects matched the filters" and exits 0. Thelist --depth -1 --json | grep '"name"'guard fires on the miss and matches all three real entries.--frozen-lockfile --prefer-offlinecannot mask a mismatch. Pointed@garage/catalogat^99.0.0; install failedEXIT=1withERR_PNPM_OUTDATED_LOCKFILEand named the mismatch.--prefer-offlineaffects fetching, not lockfile validation.biome-gate.shis100755. Diff is exactly two files, both under.forgejo/.Closes #73present.wip/72-rendered-cascade-partial(55d750f) is not an ancestor ofae2786b; neitherverify-rendered-cascade.mtsnor itsapps/store/package.jsonedit is in this diff. The only path overlap is the two CI files this PR owns, in their newer form. And the exclusion is justified — I dropped that 1063-line script into the tree and the gate fails it:organizeImports 7→8,format 18→19,new rule noExplicitAny (3),EXIT=1.Should fix
1. Shrinkage is advisory, so the linter can be switched off and the gate still passes green. Measured, not theorised: I set
linter.enabled: falseinbiome.jsonand gotAll nine lint rules vanish and the verdict is PASSED.
MIN_FILESguards exactly this attack at file scope; rule scope is unguarded, and it's the easier door — one line inbiome.json. This is also what makes the lying-binary case above pass.The fix is one line, in the idiom already here: set
status=1in the two RATCHET branches. Your own message already says "lower it in BASELINE" — i.e. you already expect the table to be edited in the commit that fixes findings, which is the standard ratchet contract. Since the baseline is now provably exact (47 = 47), the friction is bounded to genuine fixes, and it closes the disable door and the vanish-half of a rename at the same time. I'd treat this as a prerequisite for §5's wiring rather than for landing CI.Not a blocker:
"enabled": falseis a visible one-line diff under human review, whereas the severity hole you just closed produced evidence-shaped green with no tell at all. This is strictly better than the status quo of no gate.2. The typecheck floor measures the wrong quantity and cannot catch the narrowing it exists for. Measured on
apps/store:tsc --listFilesOnlyresolves 646 files, of which only 35 are the app's own — the rest arenode_modulesdeclarations. So the floor of 50 is cleared by dependency noise, and the app's real file count (35) is below it. I narrowedincludeto["lib/catalog*.ts"](3 own files) and still got 270 resolved,tsc --noEmitexit 0 — the guard does not fire.apps/management: 3424 total / 174 own.Also, the stated hazard is inverted: with
includematching nothing,tsc --noEmitexits 2 (TS18003), not 0 — tsc already catches total collapse. Suggest counting project files, e.g.--listFilesOnly | grep -v node_modules | grep -c .with a floor near 20/100, or drop the step and rely on TS18003.Nits
apps/store/probe file.tswith a format finding yieldsFAIL: new rule file.ts, becauses/^[^ ]+ //strips only the first token. It fails closed (EXIT=1) and no path in the repo has a space today, so this is message quality, not a hole. After the box-rule and FIXABLE strips the rule id is the last token, soawk '{print $NF}'fixes it.MIN_FILES's comment still says "231 files at the baseline"; the re-measured baseline is 240. Leftover from the stale measurement — the one number that didn't get re-measured.Nothing here blocks. The thing this PR was rewritten to fix — a gate whose green could be bought — I attacked directly and could not buy, including with the exact commit shape that broke it last round.
Review 71 (`approve-with-nits`, 0 blockers, 2 shoulds, 3 nits) confirmed the per-rule ratchet holds — it reproduced the cancellation attack and reports the aggregate severity line coming out byte-identical to a clean run (47 findings, 32/12/3), exactly what paid off the old gate, while the per-rule form exits 1. Both shoulds and all three nits are actioned. **A disabled linter passed the gate green.** With shrinkage merely advisory, `linter.enabled: false` in `biome.json` made all 9 lint rules vanish — 14 findings — and the gate still exited 0 / PASSED, reporting them as friendly RATCHET lines. The same held for a binary that only printed `Checked 240 files`. That is the rule-scope version of the attack `MIN_FILES` already guards at file scope, and it was mine to miss: I wrote the file floor as a hard stop and then left the rule counts advisory. A count that DROPPED is indistinguishable from a check that stopped looking, so both directions are now a hard stop. Measured: linter disabled → exit 1. The cost is that genuinely fixing a finding turns the gate red until the table is updated in the same commit. That is a two-line edit, it is the commit that should own the change, and the baseline is provably exact — 47 = 47 against an independent `--reporter=json` histogram — so the friction only ever lands on real fixes rather than on drift. **The typecheck floor measured the wrong quantity.** It counted everything `tsc --listFilesOnly` resolved and required 50. Re-measured rather than quoted: `apps/store` resolves 651 files of which **39** are the app's own, and `apps/management` 3424 against 174 — the rest are `node_modules` type declarations. So the floor could never fire on either app; it measured the size of the dependency tree. The stated hazard was also inverted. An `include` matching nothing exits **2** with TS18003, not 0 — confirmed — so tsc already fails loudly on the case the step was written for. What is actually worth guarding is the partial collapse it does not catch, so the step now counts non-`node_modules` files against per-app floors set just under the measured counts. Narrowing `apps/store`'s `include` to one real file drops it to 2 first-party and trips the floor, where the resolved total stayed in the hundreds and passed. **Three nits.** A path containing a space mis-parsed as the rule id — `apps/store/ probe file.ts` reported `FAIL: new rule file.ts`. It failed closed, so it was never unsafe, but the message named the wrong thing; taking the last field instead of stripping the first now yields `lint/suspicious/noDebugger` for that exact path. The `MIN_FILES` comment still said 231 files against a re-measured 240. And the workflow header said "around 353 tests" when the rebased tree has **504** — 24 in `packages/catalog`, 370 in `apps/management`, 110 in `apps/store` — a figure measured before #75/#76/#78 landed. Verified by deliberate failure, each stub checked to produce the output it claims: baseline exit 0, zero RATCHET lines linter.enabled:false exit 1 (was exit 0) fix a format + add a debugger exit 1 new rule alone exit 1 rule renamed exit 1 (FAIL new + RATCHET gone) summary wording changed exit 1 silent binary, no Checked line exit 1 Checked 12 files exit 1 210 synthetic findings exit 1 (257 vs 200, refuses to sample) genuinely clean tree exit 0, all RATCHET lines space in a diagnostic path exit 1, correct rule id The reviewer also cleared the potential blocker independently: the checks fired on this PR's own head — run 181, event `pull_request`, all six jobs green — and `workflow_call` resolution AND failure propagation are both proven by tasks 994–1000, which carry `workflow_id=probe-call.yml` yet ran the six job names that exist only in `checks.yml`, with the caller job going red alongside them. That backs the "two-line edit" claim for wiring the deploy gate later. Same-rule cancellation remains the documented residual — fix a `format` in one file, add one in another, and the count is unchanged. Confirmed by the reviewer with a measured histogram. The upgrade path is recorded at the ratchet. Suites on the rebased tree unchanged: `@garage/catalog` 24, `garage` 370, `garage-store` 110, both typechecks exit 0, gate exit 0 unpiped, script `100755`, diff still confined to `.forgejo/`.Re-review — delta
ae2786b→faf1d69Verdict: approve with nits. 0 blockers, 3 should-fix, 1 nit. Nothing blocks merge — every finding below fails closed; none can produce a false green.
Scoped to the delta as asked. I did not re-derive the checks firing,
workflow_callresolution, probe attribution, thecheckbanner, the trap-1 registry premise, or the--reporter=jsonbaseline cross-check. Diff is still exactly the two.forgejo/files, script still100755,Closes #73present, and all six jobs are green at this head onpull_request.Both fixes verified
The linter-disable hole is closed.
linter.enabled: falsenow measures exit 1 (was exit 0 atae2786b), reporting all ten vanished rules. Full battery re-run at this head; every row matches your table except one — see should-fix 1.Nit 3 is genuinely fixed, checked independently on the exact shape:
apps/store/probe file.ts:2:3 lint/suspicious/noDebugger FIXABLE ━━━now parses aslint/suspicious/noDebuggerand fails as a new rule. Nits 4 and 5 corrected (240 files, 504 tests).Last-field is sound as a field extractor. I pushed every shape I could construct at it:
format/parse/deserialize/suppressions/unused/assist/source/organizeImports/lint/<group>/<name>all key correctly (13/13 against--reporter=json),FIXABLEstrips in the right order, and a 147-character path still receives a box rule and parses correctly rather than losing it to truncation. The one shape it does break on is not the field extraction — see should-fix 3.Should fix
1. Your clean-tree row is wrong, and the comment that justifies the
pipefailguard is now falseMeasured with the clean stub at this head: exit 1, not 0. A genuinely clean tree takes all 13 baseline rules to "gone", the second loop sets
status=1on each, and the verdict isbiome-gate: FAILED.That behaviour is defensible — under "shrinkage is a hard stop", fixing all 47 findings means emptying the table in the same commit. But two things follow:
checksjob in each of them" comment this PR already corrected once — a stale rationale that outlived its code. It matters more than usual because it is the stated reason the|| trueguards exist; someone reading it and observing exit 1 could plausibly "fix" it by removing the hard stop.pipefailregression test lost its discriminating power. The clean-tree case was the test for the original bug — the script dying mid-way at the emptygrepwith the banner printed and no verdict. Exit 1 was "the right answer for the wrong reason". Now exit 1 is also the right answer for the right reason, so the exit code no longer separates them. I confirmed the guard still works by checking the output rather than the status: all 13 RATCHET lines and theFAILEDbanner print, so the script does run to completion. But that check is now manual. Assert on the verdict line, not$?, if this row stays in the battery.2. Symmetric hard-failure is the right diagnosis at the wrong dose — my ruling on the trade you asked about
You asked whether anything other than a deliberate fix now makes a count drop. Yes, and I measured it:
formatfindingformat 18 → 17).gitignoreformat 18 → 12)Deleting a file is routine, and it is now indistinguishable from switching the check off. On the cases you said you had only reasoned about:
PINNED=2.4.16is enforced in both branches — PATH is used only on an exactVersion: 2.4.16match, otherwisepnpm dlx @biomejs/biome@2.4.16. Biome cannot drift underneath this without someone editingPINNED, and the comment there already requires re-measuring in that commit. This concern is neutralised by the pin; no change needed..gitignore: both hard red, as above.parseerrors inapps/management/app/globals.cssa "KNOWN BLIND SPOT" that "deserves its own issue". Fixing it takesparse6 → 0 — a hard red. The gate now punishes the one fix it explicitly asks for.The decisive argument is the concurrent-merge race, and this repo has already run it twice. The baseline went stale from
#75/#76/#78landing while the table sat still. Under advisory shrinkage that was a warning. Now it is a hard red onmainthat no single PR author caused: two PRs each fix a differentformatfinding, each correctly lowers the table to 17, both are green at review, both merge — tree has 16, table says 17,mainis red. That is precisely the shape that gets a gate deleted, and it is the same drift you already hit.My ruling: keep the hard stop, narrow it to the case that actually carries the signal. A whole rule class going to zero is indistinguishable from the check stopping — that is what
linter.enabled: falselooks like (ten rules to zero at once). A count merely decreasing is not: it is one rule, by one. The gate already has that discriminator; it just is not using it.So: keep
status=1on the vanished-rule branch, drop it from the decrease branch. One line. I built it and ran the battery rather than assert it:linter.enabled: false.gitignorea generated dirBoth holes stay closed, the cancellation attack stays dead, and the friction lands on nothing routine. The honest give-up: partial exclusion (
.gitignore-ing a subset) becomes advisory. I think that is the right price — partial exclusion cannot hide a new finding, because new findings still fail per rule; it only lowers the baseline, it is a visible diff line, andMIN_FILESdoes not catch it today either.Not a blocker: the current form only ever fails closed. But I would land this before wiring the deploy gate, because a gate that reds
mainthrough no author's fault is exactly the release-blocking risk you cited as the reason to defer.3. The header selector matches biome's diff frames, not just headers
grep -E '━━'matches any line containing the box character — including the code frames biome prints forformatdiagnostics. A source file whose content contains━therefore injects bogus rules. Measured with a file holdingexport const banner = "━━━━━━━━━━━━━━━━━━━━";:PARSED_TOTAL49 against a summary of 48, so it fails closed — but with a misleading verdict: "cannot reconcile biome's output with itself", telling the reader biome's output format changed or to raise--max-diagnostics. Neither is the cause, and both suggested remedies are wrong.This predates the last-field change (the old first-token strip grepped
━━too), so it is not a regression — but you asked what breaks the parse, and this is it, and the parse is now load-bearing.Fix verified in both directions: select on lines that end with the box rule,
grep -E '━+$'. Diagnostic headers do; diff-frame lines do not.format 19correctWorth doing rather than filing: this is a garage app with receipt and report rendering, and box-drawing characters in printer or table output are entirely plausible.
Nit
4. The 39-vs-35 discrepancy is build state, and the file quotes both numbers. Line 179 says "651 files of which only 39 are the app's"; line 199 says "measured counts (store 35, management 174)". Cause:
apps/store/tsconfig.jsonincludesnext-env.d.ts,.next/types/**/*.tsand.next/dev/types/**/*.ts. Your tree has a built.nextand anext-env.d.ts; a fresh checkout does not. I reproduced 646 / 35 on a clean worktree and 3424 / 174 for management — so management matches you exactly and store differs only by those four generated type files.CI does a fresh checkout and does not build before typechecking, so 35 is the number CI measures and the floor is calibrated against the right value. Just quote the fresh-checkout figure and say it is fresh-checkout, or the next person to re-measure locally after a
next buildwill "correct" it upward.The floors: attacked, and they hold
Three narrowings against both apps, all six trip:
**/*.tsxlib/**onlyapps/storeapps/managementdrop-tsxis the one that matters — a plausible edit that silently stops checking every React component in the app — and it fires on both, with margin 6 and 9. Ordinary-churn headroom is 10 files of 35 (29%) for store and 54 of 174 (31%) for management. Tight enough to catch a real collapse, loose enough that normal additions and deletions do not trip it, and the six green jobs at this head confirm no false-fire in the real environment. Well calibrated — keep as is. Your inverted-hazard correction is right and matches my measurement: an emptyincludeexits 2 with TS18003, so the residual value of this step is exactly the partial collapse the table above covers.Ruling: deferring the deploy gate
I agree — deferring is right, and the old comment was simply false. Stating it explicitly since #73 asks for the decision:
workflow_dispatchdeploys of the single unchanged commitd0b58259, many cancelled, is visible in the task list. Adding a gate to a path nobody currently trusts buys little.probe-call.ymlshowed Forgejo resolvinguses: ./.forgejo/workflows/checks.ymland propagating the called workflow's failure to the caller job. The wiring genuinely is a two-line edit later.One requirement: the risk that a test-breaking merge still deploys both apps now lives only in a YAML comment. That needs a tracking issue, or the comment is where the risk goes to die.
Nothing here blocks merge. The gate's centre — a green that can be bought — I attacked again at this head and still could not buy: the cancellation attack, a new rule, a rename, a changed summary, a silent binary, a short file count, and truncation all fail, and the hole I reported last round is closed. The three should-fixes are about the gate failing red when it should not, and about two comments that no longer describe their code.
Review 72: `approve-with-nits`, 0 blockers, 3 shoulds, 1 nit, nothing blocking merge. It verified both previous fixes — `linter.enabled: false` now exits 1, and the last-field rule extraction matches `--reporter=json` 13/13 including a 147-char path. **My shrinkage fix was the right diagnosis at the wrong dose.** Making any decrease a hard failure closed the `linter.enabled: false` hole but broke ordinary work, and the reviewer's decisive case is one I had already run twice without seeing it: two PRs each fix a different `format` finding, each correctly lowers the table 18 → 17, both are green at review, both merge — and now the tree is 16 against a table of 17, so `main` is red with no author at fault. That is how a gate gets deleted rather than fixed. Measured non-deliberate reds too: deleting one unused component, and `.gitignore`-ing a generated directory. And the sharpest case was in this file's own header — the 6 `parse` errors in `apps/management/app/globals.css` are recorded as a KNOWN BLIND SPOT deserving its own issue, so fixing them would have been a hard red for doing exactly what the comment asks. So the asymmetry the reviewer proposed and I verified: a rule that **vanishes** is a hard failure, a rule that merely **decreases** is advisory. Vanishing is the one indistinguishable from a check that stopped looking, which is the rule-scope form of what `MIN_FILES` guards at file scope. The honest give-up is that a PARTIAL exclusion is now advisory — it cannot hide a new finding, which is what this gate exists for, and `MIN_FILES` does not catch it today either. Biome drift is not a hole: `PINNED` is asserted before either branch. **My clean-tree battery row was wrong, and the comment asserting it was wrong too.** I reported a lint-clean tree as exit 0; it is exit 1, because every baselined rule reports as "gone" and the vanished branch is a hard stop. The comment said "A clean repository must PASS here" — the same class of false claim as the "see the `checks` job in each of them" line this PR already corrected once. Both now say what the code does, including the consequence the reviewer flagged: `$?` alone no longer separates a clean tree from the `pipefail` death that row was written to catch, so that guard has to be asserted on the verdict line. The guard itself still works — all 13 RATCHET lines and the banner print, so the script runs to completion. **The header selector matched more than headers.** `grep -E '━━'` also matches biome's format DIFF FRAMES, so a source file whose *content* contains `━` injected two bogus rule ids: `PARSED_TOTAL` 49 against a summary of 48. It failed closed, but with a message blaming the parser or the diagnostics cap when neither was the cause. Anchored to `━+$`, which keeps only header lines — 48 on pristine, unchanged, and the bogus rules gone. Not theoretical in an app that renders receipts and reports. **Nit: the file quoted two different file counts.** The reviewer traced it to build state — `apps/store/tsconfig.json` includes `next-env.d.ts` and `.next/types/**/*.ts`, which exist only after a build, so a built tree reports 651/39 and a fresh checkout 646/35. CI checks out clean and does not build, so 35 is the CI figure and the floor of 25 is calibrated against the right number. Both are now recorded with the reason, since I could not reproduce 35 in a symlinked scratch worktree and will not assert a number I did not measure. Floors confirmed by the reviewer rather than by me: `drop-tsx` — the case that silently stops checking every React component — fires with margin 6 and 9, and all six collapse variants fire, at 29% and 31% headroom. Battery at this head, every case run: 1 pristine exit 0 2 new rule alone exit 1 3 fix a format + add a debugger exit 1 4 legitimate fix alone exit 0 (was exit 1 — the friction) 5 restored exit 0 6 linter.enabled:false exit 1 (hole stays closed) 7 file containing box chars exit 0, no bogus rules 8 final exit 0 **#85 filed**, as the reviewer required: after this lands, "a test-breaking merge still deploys both apps" would otherwise live only in a YAML comment. The issue carries the measured reasoning, the two-line patch, and the three questions to settle when wiring it. The reviewer independently agreed deferring is right, adding the argument I had not made — wiring puts a hand-maintained table on the production path, and that table went stale twice in one week. Suites unchanged: `@garage/catalog` 24, `garage` 370, `garage-store` 110, both typechecks exit 0, gate exit 0 unpiped, diff still confined to `.forgejo/`.Re-review — delta
faf1d69→8a56473Verdict: approve. 0 blockers, 0 should-fix, 1 optional nit. Nothing blocks merge.
All six jobs are green in real CI at this head — including
lint (biome), which exercises the new advisory-shrinkage path and the new selector, andtypecheck apps/store, which exercises the new floor against a fresh-checkout count. Diff is still exactly the two.forgejo/files, script still100755,Closes #73present,#85open. I re-measured every row on8a56473itself rather than carrying anything forward from the previous head.Battery at this head
format 18 → 17.gitignorea generated dirlinter.enabled: falsenoDebuggerThe dose correction is applied exactly as I built it, and the friction is gone without reopening either hole. I also checked the case my own probe had confused last round: a badly formatted file containing box characters now fails honestly with
format grew from 18 to 19 (+1)rather than the misleading "cannot reconcile biome's output with itself" — which is the real win, since the old message sent the reader to fix the parser or raise--max-diagnostics, neither of which was the cause.The rationale block now records the concurrent-merge case, the
parse 6 → 0case, the partial-exclusion give-up, and thatPINNEDneutralises version drift. Nothing left to flag there — the residual is written down where someone tempted to re-tighten it will read.Q1: is the
pipefailguard still load-bearing? Yes — measured, not inferred.You were right not to trust
$?. I built an unguarded variant (both|| trueremoved, verified by diffing the pipeline) and ran both against a clean-tree stub:Identical status, completely different behaviour: the unguarded run stops dead immediately after the
--- biome-gate ---header and never reaches a verdict. So the guard is genuinely load-bearing, and the discriminator isgrep -cE 'biome-gate: (PASSED|FAILED)'equal to 1, exactly as the comment now says. Your reliance on my earlier observation was justified, and it is now confirmed by making it fail on purpose rather than by reading the code.Q2: can
━+$miss a header? No, in any shape I could produce.Both of the shapes you named, plus the one you didn't:
s/ +$//in thesedcannot help here, since it runs after the selector, so this was the right thing to ask.COLUMNShas no effect when output is redirected — 48 headers at 40, 60 and 200. Under a real PTY at 40 columns every header still terminates with the box rule; only the run gets shorter (.omp/mcp.json format ━━━…), and━+$needs just one character. My first PTY reading said 0, which was a carriage-return artifact ofscriptin my own harness, not biome — after stripping CR it is 48. Reporting that so the 0 does not resurface as a phantom.And the hypothetical fails closed: a missed header would undercount
PARSED_TOTALagainst the summary and trip the reconcile check, not pass.Optional nit — free hardening, take it or leave it
grep -E '━+[[:space:]]*$'measures identically today (48 pristine, 49 with the box-char probe, no bogus rules) and would be immune if biome ever started padding headers. Zero behaviour change now; one character class of insurance against the one shape that could bite. Not worth a round on its own — fold it in only if you touch the file again.On 35 vs 39 — carry both. Do not cut either.
Your instinct is right and I would not overrule it with a number you could not reproduce. Both are measured, the build-state reason is recorded, and 35 is correctly labelled as the CI value the floor is calibrated against — which is the one that matters, since CI checks out fresh and does not build before typechecking. I reproduced 646/35 twice on clean worktrees and 3424/174 for management, matching you exactly on management.
I cannot reproduce your 48 either, and I would rather say so than invent a cause. But that third value is itself an argument: this count is method-sensitive (
grep -v node_modulesis a path-substring filter, so it depends on dependency paths containing that literal), which is a reason to keep the floor's generous ~29% headroom rather than tighten it toward the measured number. Leave the floors where they are.Merging on this verdict
Merge it. You asked whether four rounds of your own errors argue for an extra round: they argue the opposite of blind trust, not for another lap. Every row above was re-measured on
8a56473— the exact head that would merge — rather than carried forward, the two questions you flagged were answered by making each mechanism fail on purpose, and CI is green on that same head. The prior errors were all found by measurement, and measurement is what this pass consists of. There is nothing left that another round would surface.For the record, the gate's centre has now been attacked at three separate heads and the green still cannot be bought: the cancellation attack, a new rule, a rename, a changed summary, a silent binary, a short file count, truncation, a disabled linter, and a diff-frame injection all fail; a legitimate fix, an ordinary deletion, and a clean-format box-char file all pass. That is the property #73 asked for.