Uh oh!
There was an error while loading. Please reload this page.
docs(bugbot): changing half of a paired construct is a new bug, not a partial fix - #781
Conversation
… partial fix
Three instances in one day, all caught by reviewers rather than by me, and all
on constructs whose halves sat within twenty lines of each other:
client#777 `||` neutralised to \001 for the grep arms, but \001 not added to
the head terminator class -- `producer | head||die` silently
dropped, though develop flagged it. FAIL-OPEN.
client#764 Helm comment stripper taught the chomping opener `{{- /*` and not
the closer `*/ -}}`, so a block using both never terminated and
the stripper ate 23 of 165 lines. FAIL-CLOSED, in a required gate.
client#777 the fix for the first shipped a second change whose mutation
SURVIVED -- inert, reverted.
The org convention is that a finding recurring across PRs becomes a rule here
rather than being re-argued in comments, and this one recurred three times
between two PRs.
Two habits close it, both cheap, both stated in the rule: grep for the sibling
before committing, and for any scanner or matcher diff the whole-tree output
against the base -- if the base flags something you no longer do, that is a
regression no unit test will show you, because the tests only cover the case
you were already thinking about.
Plus the corollary that explains why it kept getting through: a fixture set
that only exercises the form the author had in mind. The k3s suite shipped nine
cases in the SAME commit as a bug none of them could see, because none used the
`*/ -}}` spelling the real template uses.
Every claim re-checked against the tree before committing. check-style,
gen-manifest --check and the early-close gate all clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
saqlainsyed007
left a comment
There was a problem hiding this comment.
Approving. This is exactly the org convention in action — a finding that recurred (three times across client#777/#764 in one day) becomes a rule in .cursor/BUGBOT.md rather than being re-argued in comments. The rule is well-drawn and the two habits it prescribes are the right ones: grep for the sibling half of a paired construct, and diff the whole-tree scanner output against the base (the failure mode unit tests structurally can't show, because they only cover the case you were already thinking about). The corollary about fixture sets that only exercise the author's intended form is the sharp part — it's why the #764 stripper bug shipped in the same commit as the suite that couldn't see it. Accurate, docs-only, no runtime surface. Nice writeup.
Uh oh!
There was an error while loading. Please reload this page.
Follows client#776, which encoded the three shapes of a vacuous test. This encodes a different pattern, found the hard way in the hours since.
The rule
Changing half of a paired construct is a new bug, not a partial fix — and often in the opposite direction from the one being fixed.
Three instances in one day, all caught by reviewers rather than by me, all on constructs whose halves sat within twenty lines of each other:
||neutralised to\001for the grep arms viagrep[^|\001]*, but\001never added to theheadterminator class —producer | head||diesilently dropped, thoughdevelopflagged it{{- /*but not the closer*/ -}}; a block using both never terminated and the stripper ate 23 of 165 linesOnce is bad luck. Three times between two PRs is a pattern, and the org convention is explicit that a recurring finding becomes a rule here rather than being re-argued in comments.
The two habits it prescribes
Both cheap, both would have caught all three:
The corollary — why it kept getting through
A fixture set that only exercises the form the author had in mind.
The
k3s-components-agreementsuite added in client#764 had nine cases, and none used the*/ -}}closer. So the suite shipped in the same commit as a bug none of its fixtures could see. When the thing under test accepts several spellings of one construct, enumerate the spellings from the real input — here, from the template the guard actually reads — not from the example in your head.That is adjacent to client#776's "unreachable fixture" shape but distinct: there the fixture couldn't reach the code path; here it reaches it, in only one of its several legal spellings.
Verification
Docs-only. Every factual claim re-checked against the tree before committing — the line references, the 23-of-165 measurement, and the nine-cases-at-the-time count (that suite is at 11 now, and the text is careful to say what it was at that commit).
check-style.sh,gen-manifest.sh --checkand the early-close gate all clean.🤖 Generated with Claude Code
Note
Low Risk
Docs-only change to the Bugbot review guide; no runtime, security, or CI behavior is modified.
Overview
Adds two Always flag rules to
.cursor/BUGBOT.mdso reviewers treat a one-sided edit of a paired construct as a new bug, not a partial fix.The first rule covers openers/closers, matcher arms, and writer/reader pairs, with three recent examples (fail-open
head||diedrop, fail-closed Helm comment strip, inert mutation). It prescribes grepping the sibling and diffing whole-tree scanner output against the base.The corollary flags fixture suites that only cover the spelling the author had in mind, citing
k3s-components-agreementshipping without a*/ -}}case.Reviewed by Cursor Bugbot for commit 0b8f413. Bugbot is set up for automated code reviews on this repo. Configure here.