test: the report must ask cb_verdict, not merely have it (#531 follow-up) - #538
Conversation
…cts (#537) Adversarial review by @ChronicallyJD, all three findings reproduced here before being fixed. The first should have blocked the merge and did. 1. THE FIX WAS REMOVABLE WITHOUT A RED. Measured, not reasoned: delete the pgc_start_log_report call from pgc_setup -> 70 checks, PASSED revert the summary path to a hardcoded pattern -> 70 checks, PASSED assert a squatter unconditionally -> FAILED, 2 checks The 16 checks fed the three functions fixtures and proved their arithmetic. Nothing asserted the failure path calls any of them, so the entire contribution of this branch could be deleted and the suite still reported green. That is the same gap #538 found in #532's bench guards -- the same mistake, twice, the second time inside a fix for an issue about a message asserting something the code has not established. Six call-site checks now, over source text, which is the weaker kind and is labelled as such. The failure path needs a cluster that will not start, which this suite cannot stand up; a weak check on the call site beats none. Both greps are premised on the path still existing, or they approve nothing. Rows 1 and 2 above are now red. Row 3 already was. 2. pgc_fatal_pattern's docstring claimed "one definition, used by both the start-failure path and the summary path". pgc_start_log_report never called it; it hardcoded FATAL:|PANIC:. So the comment asserting the drift had been fixed was itself untrue, committed in the fix for #537. The divergence is right and argued below it. There are now two NAMED patterns, pgc_fatal_pattern and pgc_start_fatal_pattern, so the difference is greppable rather than two literals in two places, and a check asserts each path asks its own. 3. _sawforeign was sticky: set on any attempt, never cleared. One squatter on attempt 1 followed by seven genuine start failures printed the squatter verdict for all eight -- #537's own defect narrowed rather than removed, and reachable, since escaping a port collision is what the retry loop is for. It is a count now, with three cases: none, all, and the mixed one that a flag cannot express. The mixed case has its own check and fails when the old any-nonzero behaviour is restored. One check of my own was wrong rather than the code: a grep for the inline verdict matched two unrelated lines about the previously installed .so (#513). Tightened to the start-failure text, with the reason recorded, since that is the same prefix-matching trap this suite already guards for suite names. harness_selftest 70 checks to 78. End-to-end re-verified with a genuinely broken .so after the pattern change. Gate: PG17 assert 132 ran PASS, PG19 assert 137 ran with only temporal, btree_gist absent from this container, identical on main. Refs #537
jd asked for this. It is the one section of this repository that tells every
future agent how to argue, so it was deliberately left unwritten until he did:
the other agent declined to write it on a peer's request, correctly, on the
grounds that a good argument is what a bad request would also look like.
Four additions and one widening, all from defects committed on 2026-08-09 rather
than from principle.
WIDENED. "Prove the guard by removal" now says prove it by removal and not only
guards, with the one-line form up front:
Can I delete this change and still be green?
The bullet said "guard" and fired reliably for guards. Two changes shipped that
day whose entire contribution could be deleted with the suite still green (#538,
and the first version of #537's own fix), and neither was a guard, which is
exactly why the line stayed quiet. The rule failed by its own trigger.
ADDED. A suite that sources a helper cannot see whether anything calls it.
Feeding a function fixtures proves its arithmetic and nothing else, so the
caller can be deleted with every check passing. Assert the call site; a source
grep is the weaker kind and is still worth writing.
ADDED. Before believing a check, make it say the other thing. Two mirror-image
failures: a check too TIGHT to fail approves anything, a check too LOOSE to
believe condemns anything. The loose kind cost as much diagnosis time that day as
the tight kind, and one instance was a grep matching a message that merely began
the same way.
ADDED. When a rule does not fire, fix its trigger, not your discipline. Guidance
can be correct, specific and silent because its trigger is narrower than its
content. The widened bullet above is the worked example. "Consult it more
carefully" is not a fix.
Also removed a number from the suite-count bullet. It read "it is 132 today",
which was 137 by the time anyone noticed, sitting one clause after the command
that computes it. Quoting a weekly-changing count beside its own query is the
drift this file warns about, committed in the file that warns about it.
Two corrections to my own first draft of this text, both the defects it
describes. I wrote "both authors had read this line", which asserts something
about other people's reading that I cannot establish, in the section about not
asserting what has not been established. And both changes were in fact the same
author's. The sentence now states what happened instead.
CONTEXT.md is not covered by docs_style, which checks docs/*.md and README.md,
because it is internal engineering guidance like design/. docs_style and
harness_selftest both pass.
…own rule (#545) Review by @ChronicallyJD. Every one is the file not meeting the standard it states, which is the only kind worth reporting in a change about how to argue. CITED THE REMEDY AS THE DEFECT. The removal bullet listed #538 as a change that shipped deletable. #538 is the change that CAUGHT it; the one that shipped deletable was #532. Corrected, with #538 named as the catcher so the pair is still findable. THE PERSON-CLAIM SURVIVED ONE PARAGRAPH DOWN. I cut "Both authors had read this line" from the removal bullet, said so in the commit message, and left "it stayed quiet for two people writing fixes" in the trigger bullet, which asserts the same unestablished thing in different words and is wrong the same way: both changes were one author's. It now reads "for both changes above" and claims nothing about anyone. This is the finding worth keeping. A corrected sentence is not a corrected belief, and the second instance is where you stop looking because the first one felt like the fix. That is the same shape as the trigger bullet itself: knowing the rule is not what makes it fire. THE ONE UNCITED CLAIM. The loose-grep example carried no issue number while #532, #537, #487 and zonemap_cost all do, in a section about not making unverifiable statements. Now cites #537, and #513 for the message it collided with. Refs #545
…st (#545) Proposed by @ChronicallyJD, and their wording rather than mine. I had offered "a corrected sentence is not a corrected belief, and the second instance is where you stop looking because the first one felt like the fix". True as a diagnosis, and wrong as a bullet: its operative content is "notice that the correction felt like completion", which is an instruction to notice harder. It cannot be checked and it cannot fire, and it would have sat one paragraph below "Consult it more carefully is not a fix" -- this section failing its own standard a fourth time, in the sentence added to record the third. The mechanical form earns its place: search for the same claim elsewhere, grep for the phrasing you just removed. Ten seconds, names the tool, fails visibly when skipped. Same properties as "can I delete this change and still be green". The bullet then caught a fourth instance of its own class before this commit landed, which is why the example in it is two failures rather than one. Verifying every issue number in the added text -- which is what the bullet demands -- found that the loose-grep example cited #513 for a message introduced by #508. The previous commit had corrected exactly this defect, #538 cited where #532 belonged, four lines away. The first correction felt like completion, again, in the edit that recorded the first correction feeling like completion. The psychology stays here in the history, where the story of how it happened is findable, and the bullet carries only the part a reader can execute. Refs #545
ChronicallyJD
left a comment
There was a problem hiding this comment.
Reviewed by running it. The premise is right and the seam is the right one to
close. One finding, and it is the same class this PR exists to fix, one level
up: the check can be satisfied without the behaviour being present.
Verified
45 -> 49 checks, and it catches what it names. Restoring the #531 defect
verbatim in bench/run_clickbench.sh:
FAIL ratio()'s output reaches printf and nothing else: got [2] want [0]
FAIL and the verdict is asked of cb_verdict rather than recomputed inline: got [no] want [yes]
The premise on r[12]=$(ratio is the right one and it is doing real work: rename
the variable and the premise fails rather than the check silently approving.
The finding: a mention satisfies it, so the defect can return
grep -c 'cb_verdict' counts any occurrence, including one in a comment. I
removed the call, decided the verdict inline on $c and $h directly, and left
one comment line mentioning the name:
# a future refactor might reinstate cb_verdict here
if [ "$(awk -v x="$c" -v y="$h" 'BEGIN { print (x < y) ? 1 : 0 }')" = 1 ]; then
wins=$((wins + 1)); else losses=$((losses + 1)); fiResult: checks run: 49, bench_guards.sh: PASSED.
Both checks are bypassed at once, because deciding on $c and $h rather than
on $r1 also satisfies "ratio's output reaches printf and nothing else". So the
verdict is computed inline again, the report is back to #531's shape in
everything but the rounding, and the suite approves it.
This is not hypothetical in the way it might look. The likely path is not
somebody being clever, it is somebody refactoring the loop, leaving a TODO
mentioning cb_verdict, and the guard staying green.
The fix is one character
Grep for a call rather than a mention. Measured on the same bypassed tree:
loose grep -c 'cb_verdict' -> 1 approves
tight grep -c 'cb_verdict "' -> 0 rejects
and on the good tree, tight -> 1 still approves
cb_verdict " is the shape a call takes here and a mention almost never does.
Same form as the call-site checks in #544, which land on pgc_start_log_report "
for exactly this reason.
What I could not fault
Both premises are load-bearing rather than decorative, and the "the alternative
is not a better test; it is no test" note is the right framing for admitting a
source-text check. The four checks are the correct four for this seam. This is a
one-character change from being airtight, and the seam it closes is one I left
open twice.
…pt#531 follow-up) commandprompt#532 added cb_verdict, cb_warm_spread and cb_band and fifteen checks over their arithmetic. None of them proves that bench/run_clickbench.sh asks any of them anything: test/bench_guards.sh sources bench/cb_guards.sh and never reads the benchmark, so the call site is invisible to the suite. Measured rather than assumed. Restoring the commandprompt#531 defect verbatim at the call site, r1=$(ratio "$c" "$h") if [ "$(awk -v r="$r1" 'BEGIN { print (r < 1) ? 1 : 0 }')" = 1 ] ... leaves the suite reporting 45 checks PASSED. With these four checks it reports 49 and FAILS, and passes again when the call site is restored. These are checks over source text, which is the weaker kind, and they are here because the behaviour they guard needs the 15 GB download and the tuned cluster this suite exists to do without. The premise checks in front of the negative one are not decoration: rename r1 and "no ratio value decides anything" would pass against a report that had gone back to deciding on one under another name. Gated: bench_guards 45 -> 49 with the removal proof both ways, and harness_selftest 54/54 on pg18a and on pg19a.
1fe4717 to
aaffd16
Compare
|
Finding accepted and fixed at Same tree, verdict recomputed inline on
And you were right that both checks fall together: deciding on Now matched as a call. Re-gated: The comment carries the measurement and the realistic path you named — not somebody being clever, somebody refactoring the loop and leaving a This is the same defect this PR exists to fix, one level up, in the fix itself. I wrote a check that a mention satisfies, in a PR whose argument is that a suite can approve a report which no longer does the thing. Worth stating plainly rather than quietly correcting: the seam was right and my assertion at it was not, and it took someone running the bypass to show that. |
#532 fixed the win count and added
cb_verdict,cb_warm_spreadandcb_bandwith fifteen checks over their arithmetic. This adds the check nobody has: that
the report asks them.
The gap, proved by removal
test/bench_guards.shsourcesbench/cb_guards.shand never readsbench/run_clickbench.sh, so the call site is invisible to it. Restoring the#531 defect verbatim:
The second row is the point. The fix was merged and the harness could have been
reverted to the exact defect it fixed without a single check reddening.
What the four checks are
Two premises and two assertions.
ratio()'s output must reachprintfandnothing else, and the verdict must come from
cb_verdict.The premises are load-bearing rather than decoration. Rename
r1and a bare"no ratio value decides anything" passes against a report that has gone back to
deciding on one under another name — the grep finds nothing, and nothing is what
it approves. So the suite first asserts that the report still captures
ratio()into a display variable at all.
These check source text, which is the weaker kind of check. They are here
because the behaviour they guard needs the 15 GB download and the tuned cluster
that
bench_guardsexists to do without. The alternative is not a better test;it is no test, which is what the 45 amounted to at this seam.
They live in the suite rather than in
bench/cb_guards.shon purpose: thatfile's contract is "Nothing here touches a database or the filesystem", and a
guard that reads a source file would break it.
Gate
bench_guardson mainbench_guardson this branchharness_selftestpg18aharness_selftestpg19aThe pg19a number needed #536 dealt with first: the run initially failed every
cluster start because the build had reused pg18a's objects.
make cleanand arebuild for pg19a alone, and it passes. Nothing in that is caused by this branch
— the control on unpatched main fails identically — but it is why the gate took
two passes, and it is filed as #536.
Closes nothing on its own; it is the follow-up #532 wanted. See also #537 for
why diagnosing that gate failure took six probes.