Uh oh!
There was an error while loading. Please reload this page.
test(style): prove the style gate fires — 25 cases, every rule both ways (backend#1924) - #762
Conversation
…ays (backend#1924) Sweep 6 (backend#1729) measured that 2 of the 24 shell scripts under scripts/ were referenced by no bats suite. gen-manifest.sh got client#707; check-style.sh was the residual, and it now sits inside the REQUIRED "Source-of-truth drift" check — so a rule that silently stopped matching would read as "clean" forever. Shape copied from client#707: every rule is tested BOTH ways — it fires on a violation AND passes on the clean equivalent. One-sided tests are the trap here: a rule whose regex stopped matching anything satisfies every "clean input is clean" assertion while enforcing nothing. Covered: all four rules and each of their documented exemptions, the shared `# style-guard: allow` opt-out, scanner scope (.ps1 IS scanned, scripts/tests IS NOT, non-scripts are not), and both fail-closed exits — a missing tree and a grep internal error must be exit 2, never a clean 0. The brand palette is DERIVED from the script's own `brand=` regex rather than restated, with a floor on the count. Deriving alone would be self-consistent and blind: mutating a tone out of the palette also mutates what the test reads. Mutation MS2 proves the floor closes that. The grep-error path is driven by putting a grep that exits 2 on PATH, so the real `rc >= 2` branch runs, rather than editing the script and testing a copy of the rule (#1729 rule 9). 21 mutations, each reddening its own guard; control 25/25. One found a real hole in this suite before it shipped: rule 4 has TWO exemptions, and a fixture of only `Get-TraceblocClientEnv` satisfies both, so deleting the `[-]Tracebloc` filter changed nothing and the test stayed green. Split into a case that only the leading-dash filter can spare (`Get-Tracebloc`, no following capital); both now redden independently. Also fixes a stale claim this exposed: the header said "Three mechanical checks" while four `report` calls were live — rule 4 (capital-T Tracebloc) arrived later and the count stayed behind. A gate whose own description undercounts it is how a rule gets dropped unnoticed, so the count is now asserted by the suite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
saadqbal
left a comment
There was a problem hiding this comment.
Bugbot's Medium on _brand_rgbs is real, and I reproduced it rather than reasoning about it. Deleting the entire RGB arm from check-style.sh:70 — so brand='#?(01a5cc|91e947|a7ed6c|01637a|578c2b|34b7d6)', rule 1's RGB half completely gone — leaves the test green:
$ bats scripts/tests/check-style.bats -f "RGB triple"
1..1
ok 1 rule 1: EVERY brand RGB triple the script declares is caught
The mechanism is exactly as described. ${re##*38;2;(} is a no-op when the marker is absent, so _brand_rgbs falls through to ${re%%)*} on the untouched pattern and yields the hex vocabulary:
RGB tokens with the arm dropped: [#?(01a5cc 91e947 a7ed6c 01637a 578c2b 34b7d6]
Those get planted as printf '\033[38;2;#?(01a5cc m', which still contains a brand hex, so the surviving hex rule fires, status is 1, the output still says hardcoded brand colour, count reaches 6, and [ -n "$rgbs" ] never fails closed. A test asserting the RGB half is caught passes with the RGB half deleted — which is the one thing this PR exists to rule out.
The fix is to make the token shape part of the contract, not just non-emptiness. Something like:
_brand_rgbs() {
local line re
line="$(grep -m1 '^brand='"$CS")"case"$line"in*'38;2;('*) ;; *) return 1 ;; esac# fail closed: no RGB arm is a finding
re="${line#brand=\'}"; re="${re%\'}"
re="${re##*38;2;(}"printf'%s'"${re%%)*}"| tr '|'''
}and in the test, assert each token actually looks like a triple ([[ "$t" =~ ^[0-9]{1,3};[0-9]{1,3};[0-9]{1,3}$ ]]) so a hex leaking through can't stand in for one.
_brand_hexes has the same shape — ${re#*\#?(} is equally a no-op if #?( ever moves — so it's worth the same guard while you're in there. It's less exposed today only because nothing else would catch its planted fixtures.
Everything else here I'm happy with: 25 cases with both arms per rule, the common.sh exemption pinned, and the non-brand-hex case proving the rule discriminates rather than matching any hex. Just this one extraction needs to fail closed before it's the proof it says it is.
…(Asad, #762) Bugbot Medium, reproduced before fixing: deleting rule 1's entire RGB arm left `rule 1: EVERY brand RGB triple ... is caught` GREEN. `${re##*38;2;(}` is a no-op when the marker is absent, so _brand_rgbs fell through to `${re%)*}` on the untouched pattern and returned the HEX vocabulary. Those were planted as `printf 38;2;#?(01a5cc m`, which still contains a brand hex, so the surviving HEX rule fired, status was 1, the count floor was met, and the non-emptiness guard never tripped. A test asserting the RGB half is caught passed with the RGB half deleted -- the one thing this PR exists to rule out. Non-emptiness was never the contract. Two changes, because either alone still leaks: both extractors now FAIL CLOSED when their marker is gone, and each caller asserts the token SHAPE -- 6 hex digits, or three numeric fields -- so a stray hex cannot stand in for a triple. _brand_hexes got the same guard: `${re#*\#?(}` is equally a no-op if `#?(` moves. It was less exposed only because nothing else would catch its planted fixtures. Mutation-proved both directions: deleting the RGB arm now reddens the RGB test, and removing the `#?(` marker reddens the hex test. Baseline 25/25. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
commented
Aug 20, 2026
@saadqbal — fixed in bcfbf52, and I reproduced your repro first rather than reasoning about it: deleting the RGB arm did leave Took both halves of your suggestion, because either alone still leaks:
Mutation-proved both directions: deleting the RGB arm reddens the RGB test, removing Re-requesting your review. |
LukasWodka
commented
Aug 20, 2026
bugbot run |
saadqbal
left a comment
There was a problem hiding this comment.
Re-reviewed bcfbf52c. My finding is fixed, and fixed in both places rather than just the one that was demonstrably broken. I re-ran the same mutations against the new code:
1. Delete rule 1's entire RGB arm — the mutation that used to pass:
not ok 2 rule 1: EVERY brand RGB triple the script declares is caught
# `rgbs="$(_brand_rgbs)" || return 1 # fail closed: no `38;2;(` marker is a finding' failed
2. Restructure the hex arm so #?( is gone (#(01a5cc|… instead) — the same class in _brand_hexes, which I'd flagged as latent rather than broken:
not ok 1 rule 1: EVERY brand hex the script declares is caught
# `hexes="$(_brand_hexes)" || return 1 # fail closed: no `#?(` marker is a finding' failed
3. Leak a non-triple into the RGB vocabulary (deadbeef among the triples) — the shape assertion, independently of the marker guard:
not ok 1 rule 1: EVERY brand RGB triple the script declares is caught
# `[[ "$t" =~ ^[0-9]{1,3}\;[0-9]{1,3}\;[0-9]{1,3}$ ]] || return 1' failed
Baseline both green. Belt and braces was the right choice — the marker guard alone would still have let a stray hex stand in for a triple if the arms were ever reordered, and you say exactly that in the comment ("A guard on the extractor alone would still let a stray hex through").
Holding the approval only on CI.Cursor Bugbot, Unit tests and bats (bash unit, mocked) are all still pending as I write this — nothing here needs another round from me, so once those land green I'll approve on my next pass. Flag me if they go red for a reason that isn't obvious.
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit bcfbf52. Configure here.
saadqbal
left a comment
There was a problem hiding this comment.
Approving as promised. Head is still bcfbf52c — the commit I verified last pass — and CI has landed clean (33 passing, 3 skipped, nothing failing or pending) with no open threads.
Recapping what I checked so the record is on the PR rather than only in chat: deleting rule 1's RGB arm reddens EVERY brand RGB triple on the marker guard; restructuring the hex arm so #?( is gone reddens EVERY brand hex on its guard; and leaking a non-triple into the RGB vocabulary reddens on the shape assertion. Baseline green. Both extractors fail closed and both callers assert token shape, which is more than I asked for and the right call — the marker guard alone would still have let a stray hex stand in for a triple if the arms were reordered.
Uh oh!
There was an error while loading. Please reload this page.
#776) Three PRs merged on 2026-08-20 — #762, #763 and tracebloc/release-train#94 — shipped ELEVEN tests that asserted the right property and proved nothing. Every one was written deliberately, reviewed, and green. That is the most frequent finding of the three PRs by a wide margin, more than every production defect in them combined, and the guide said nothing about it. The convention is that a finding recurring across PRs becomes a rule here, so: - the three shapes, ascending in subtlety — unreachable fixture, redundant mechanisms (two paths, one observable), and the inert mutation, where the MUTATION fails to express the defect. The third is the dangerous one: an anchor-resolution check cannot see it, because the anchor resolves perfectly. - a surviving mutation is a defect in the test, never a nuisance to annotate; and a green mutation log is evidence only if the run asserts the mutation APPLIED (backend#1729 rule 5). - a derived vocabulary must fail closed. Deriving beats restating, but a derivation that silently falls through returns the WRONG vocabulary and then agrees with itself — check-style.bats's `_brand_rgbs` fell back to the hex list when rule 1's RGB arm was deleted, so "every RGB triple is caught" passed with the RGB half gone. - the early-close hazard now has a CI gate, with diagnose.sh as the worked example of an instance that is correct AS a pipe. - a corollary on the `scripts/lib/*.sh` non-issue: they set no options but they RUN under both, so errexit/pipefail rules apply to them in full. A guard that asks only "does this file set the options" reads the whole lib tree as safe — the bug #763 fixed. - never resolve a review thread on "the reported case now passes": a fix for one spelling routinely leaves its sibling broken, and a resolved thread reads as handled to the next person. backend#1729 already required mutation-proving, deriving the input domain, and never testing a copy of the rule. All three were FOLLOWED in these PRs and the tests were still vacuous eleven times — the existing rules say to mutation-test but not what a surviving mutation means, nor that a fixture can be too thin for the property to be observable. That is the gap. Every claim verified against the tree before committing; two drafting errors caught that way (diagnose.sh carries no marker — the guard reads `set +e` — and `mutation-markers` is a release-train target, not a client one). Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Fixes tracebloc/backend#1924
Why
Sweep 6 (backend#1729) measured that of the 24 shell scripts under
scripts/, exactly two were referenced by no bats suite.gen-manifest.shgot #707;check-style.shis the residual.The stake is deliberately lower than the manifest generator's — an inert style guard costs drifting wording, not unverified code running privileged steps — but the reason to cover it is the same, and it got sharper on 2026-08-19: this gate now runs inside the required
Source-of-truth driftcheck. An uncovered gate is a gate nobody can prove fires, and a rule that silently stopped matching would read as "clean" forever.Shape
From #707: every rule is tested both ways — fires on a violation, passes on the clean equivalent. One-sided tests are the trap here. A rule whose regex stopped matching anything satisfies every "clean input is clean" assertion while enforcing nothing.
25 cases covering:
workspace, barecurl, capital-TTracebloc) and each documented exemption — the colour engine, the DNS-1123 sanitiser identifiers, comments,--tlsv1.2lines,has curl/command -v curl, thecurl … | shone-liner, and both PascalCase forms# style-guard: allowopt-out, on every rule.ps1is scanned (the gate is cross-platform),scripts/tests/is not (or fixtures would self-trip it), non-scripts are notTwo deliberate choices worth review:
The brand palette is derived, not restated (#1729 rule 1/6) — parsed out of the script's own
brand=regex, so a newly added tone is tested automatically. Deriving alone would be self-consistent and blind, though: mutating a tone out of the palette also mutates what the test reads. A floor on the count closes that, and mutation MS2 proves it.The grep-error path is driven by putting a
grepthat exits 2 onPATH, so the realrc >= 2branch runs — rather than editing the script, which would test a copy of the rule instead of the rule (#1729 rule 9).Mutation proof
21 mutations, each reddening its own guard; restored control 25/25.
-idropped (case-insensitivity lost)\bcurl\bweakened /--tlsv1.2,has curl,curl | shexemptions removedTracebloc[A-Z]exemption removed[-]Traceblocexemption removed# style-guard: allowignored--exclude-dir=testsdropped--include=*.ps1droppedOne mutation found a real hole in this suite before it shipped. Rule 4 has two separate exemptions, and a fixture of only
Get-TraceblocClientEnvsatisfies both — so deleting the[-]Traceblocfilter changed nothing and the test stayed green. Split out a case that only the leading-dash filter can spare (Get-Tracebloc, no following capital); both now redden independently. That is the value of asserting the mutation actually applied rather than trusting a green log.One stale claim fixed
The header read "Three mechanical checks" while four
reportcalls were live — rule 4 arrived later and the count stayed behind. A gate whose own description undercounts it is how a rule gets dropped unnoticed, so the count is now asserted by the suite rather than trusted.Verification
bats scripts/tests/check-style.bats→ 25/25bats scripts/tests/bats-hygiene.bats→ 0 offenders (the new suite satisfies the org's|| return 1convention)check-style.shon the real tree → clean;bash -n,shellcheck -S error,gen-manifest --check,check-facts --check,check-drift.sh→ all cleanMakefile's bats count is derived fromgrep '^@test', so nothing needed hand-updating.🤖 Generated with Claude Code
Note
Low Risk
Test-only plus comment/header fixes on the style guard; no runtime installer or CI behavior change beyond documenting four rules.
Overview
Adds
scripts/tests/check-style.bats(25 cases) so the requiredcheck-style.shgate inmake driftis proven to fire and pass on clean input—each of the four rules is tested both ways, plus documented exemptions,# style-guard: allow, scanner scope (.ps1vsscripts/tests/), and fail-closed exits 1 vs 2.Brand-colour coverage parses the script’s
brand=regex instead of duplicating the palette; a test also checks the header’s “Four mechanical checks” claim matches the number ofreportcalls.check-style.shheader is corrected from “Three” to Four mechanical checks and notes that the bats suite asserts that count (backend#1924).Reviewed by Cursor Bugbot for commit b8e272d. Bugbot is set up for automated code reviews on this repo. Configure here.