Repository navigation
ci: make the PR's CI comments say what they mean, and survive their own failures - #407
Conversation
…when the verdict does Review asked for four things. Two were already true, and saying so is the useful half of the answer: the check already runs on every push, because it is a step in the performance job; and it already updates its comment in place rather than appending, deliberately, because a PR with fifteen throughput comments is a PR where nobody reads the throughput comment. The other two were real gaps. THE COMMENT NOW CARRIES ITS OWN READINGS, SO THE NEXT PUSH CAN SHOW A DELTA Each report ends with one line of machine-readable state - status, rate, ratio, share. The workflow reads it back off its own previous comment and renders "since the previous push: rate X -> Y (+n%)". Reading our own last comment is the only store available: no series is kept anywhere, by an explicit earlier decision, and updating in place otherwise destroys the only record of what the last push measured. It is labelled a delta and not a verdict. This test's run-to-run spread on one unchanged commit is wide enough that a single push of difference is noise far more often than it is a change. A STATUS CHANGE NOW POSTS A NEW COMMENT INSTEAD OF SILENTLY EDITING AN OLD ONE Editing a comment thirty scrolls up produces no notification and no visible change to anyone reading the PR top to bottom, so a run that went green to red announced itself by rewriting something nobody was looking at. On a status change the old comment is retired - its marker renamed so later pushes stop targeting it, its heading marked superseded - and a fresh one posted at the bottom. Number-only movement still updates in place; only the VERDICT earns a new comment. AND THE "NO COMPARABLE REFERENCE" MESSAGE SAID LESS THAN IT KNEW It reads as "no baseline exists" and has been read that way. Baselines usually do exist - they just ran a different set of tests, most often because the PR itself enabled or disabled some. The message now says how many were found and that a matrix change is usually the PR's own doing. THE TEST FOR THIS IS A RUNTIME TEST, AFTER THE FIRST ONE PASSED ON SIX CORRUPTED MESSAGES The status started as a second argument to writeReport. Inserting it meant finding each call's closing paren, and the report bodies are template literals containing escaped backticks - so all six insertions landed INSIDE the prose, producing "control classes (`...`, 'no-control') produced a time" and an empty payload. The self-test written to prevent exactly that passed anyway, because it grepped for the literal rather than checking where it was. So the status is now set into a variable on the line before each call, which cannot go wrong that way, and the test RUNS the reporter and reads the payload back the way the workflow does. Verified by reintroducing the corruption: the old test stayed green, this one fails with "expected no-control, got unset". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
… and show incomparable readings Two follow-ups from review, both about the comment being less useful than the data behind it. AN IN-PLACE UPDATE LOOKS DEAD The body could be identical to last week's and nothing on it said a run had happened - no timestamp, no commit, no link. A reader cannot tell a fresh verdict from a stale one, and the honest reaction to that is to stop trusting the comment. Every report now carries the head sha it ran from, a link to the run, and the time, which is what makes "updated in place" legible as "updated". And when a status change retires a comment, the retired one now links FORWARD to its replacement. Renaming its marker stops later pushes targeting it, but a reader who lands on the stale one from a notification or a scroll was left with no way on. SHOW THE INCOMPARABLE READINGS, FLAGGED PER ROW Refusing to compute a verdict from a run with a different workload is right. Refusing to SHOW the numbers is a separate decision nobody asked for, and it sends the reader to the job logs to find out roughly where they stand. The candidates' rates are now listed beside this run's, and every borrowed row carries "different test set - not a control" in its own column rather than in a footnote - so a number cannot be lifted out of the table without its warning attached. A sentence under it says a difference against those rows measures nothing. NOT COVERED BY A TEST, AND SAYING SO The runtime self-test exercises the no-control path, which is the cheapest one to reach with a fixture. The incomparable path needs baseline artifacts from several runs to reach, so the new table is verified by parse and by reading, not by execution. That is weaker and it is the reason this note is here rather than a claim that it is tested. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its |
|
🟢 Throughput — OKThis branch measured about 6% slower than master, on the one test this measures. That is INSIDE this test's own run-to-run spread of about 17%, so read it as a reading and not as a result - re-running the same commit moves it by about as much.
Allowable range 🟢 ≥ 0.70 · 🟡 0.50–0.70 (about a 30% loss) · 🔴 < 0.50 (about a 50% loss) What the numbers mean, and what they cannot tell youThe one that gets misread. Why a shape and not a rate. A rate depends on which runner you drew. A shape does not: every test here processes a fixed number of records, so a runner twice as slow doubles the subject and the controls together and leaves their ratio alone. That is the whole trick, and it is why the reported rate is shown last and labelled as this machine only. Reading the comparison. By conservation, not by correction. Every test in this lane processes a fixed number of records, so within one run the ratio of one test's time to another's is invariant under machine speed — a runner twice as slow doubles both terms and leaves the ratio alone. There is no machine-index correction to be wrong, because nothing needed correcting. Per-method times, not class times. A class time is Reference is the median of 4 recent What this still cannot do. It removes machine-to-machine variance. It does not remove this test's own run-to-run variance, measured at about 30% on a single unchanged commit while its controls stayed within 5%. That is a property of the test, not of the comparison, and no arithmetic here can touch it — which is why the reference is a median and the bounds are deliberately coarse. 🟡 means look at this; only 🔴 is outside the measured spread. Since the previous push: ratio 0.998 -> 0.939, share 1.856 -> 1.973, rate 67546 -> 59231 (-12.3%). One push of difference sits inside this test's measured spread - read it as movement, not as a result. Updated for |
…ing the opposite A reviewer read "Subject share, this run: 1.713" as "this build is 1.7x faster than master". That is a fair reading of those words and it is wrong by a factor of twenty: 1.713 is the subject's time divided by the CONTROL tests' time in the SAME run, and the actual comparison with master was 1.081 - about 8% faster. A report whose headline number is routinely read as twenty times its true value is not a labelling problem, it is a broken report. THE ANSWER NOW COMES FIRST, IN A SENTENCE "This branch is about 8% faster than master, on the one test this measures." Nobody should have to divide two ratios to learn whether their branch got slower. EVERY ROW IS NAMED IN WORDS, AND SAYS WHAT IT IS FOR "Subject share" and "Reference share" were the method's vocabulary, not the reader's. The table now has a Meaning column: the comparison says which direction is better, the share row says outright that it is NOT a speed, and the reported rate says it is this machine only. The comparison is first because it is the only row that mentions master. THE EMPTY FIRST ROW WAS A REAL BUG, IN FIVE PLACES Every table opened with `| | |` - a header row of two empty cells, which GitHub renders as a blank band above the data. It looked like a rendering fault because it was one. All five now carry column names. THE EXPLANATION LEADS WITH THE MISREADING The collapsed section explained the derivation before saying what the numbers mean. It now opens with the one that gets misread, states why a shape rather than a rate (a rate depends on which runner you drew; a shape does not), and how to read the comparison - including that the 0.50 failing bound means taking twice as long relative to its controls, which is not a small regression. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
…ritten for
Review of this branch, cross-checked against an independent model, found the replacement self-test
carrying four versions of the same mistake it was written to remove. Running the reporter is not the
same as checking the run.
THE ASSERTION FOR THE ORIGINAL BUG COULD NOT FAIL
`text.includes('control classes (`')` was true whether or not the status literal had been injected
into the prose, because the corruption landed at the CLOSING paren and left that opening text intact.
Both arms of a control experiment - the clean report, and the report carrying the exact string the
last commit quotes - pass it. So the assertion written to catch that bug agreed with the bug.
It now splits the payload off and asserts the status literal appears NOWHERE in the prose a human
reads. Same experiment: clean passes, corrupted fails.
THE SPAWN RESULT WAS THROWN AWAY
A reporter that wrote the right report and then returned the wrong exit code passed everything. The
workflow branches on that code, and the four-value contract (0 pass, 1 violation, 2 cannot run,
3 nothing in scope) was asserted nowhere at all. Exit 2 is now pinned for the no-control path.
A STALE REPORT SCORED AS A FRESH ONE
Nothing removed `target/throughput-report.md` before the spawn, so a reporter that wrote nothing
passed against whatever was already on disk. Verified: sabotaging the write while leaving a passing
report in place was green before, red now.
THE FIXTURE WAS ONLY HALF THE INPUT
The reporter also globs `**/target/failsafe-reports/`. A developer holding real failsafe XML gives it
a control class, which takes it off the no-control path and out to `gh run list` - a networked,
credentialed run wearing this test's name. The precondition is asserted and fails loudly rather than
passing through a different path, because a skip is not a pass.
AND `finally` DOES NOT RUN ON Ctrl-C
Node terminates on SIGINT with no listener installed, so interrupting the test left the developer's
real files replaced by the fixture - the exact loss the save/restore exists to prevent. Handlers now
restore and re-raise with the default disposition.
Each of the four was red-proofed by reintroducing its defect: all four go red, and the control arm
with the reporter unmutated stays green and byte-identical.
Also records why the `pc-throughput-data` marker is hand-written in three places rather than shared -
the workflow reader cannot import a module, and the test's parser is independent on purpose, since a
test parsing with the producer's own code would agree with it by construction. `grep -rn
pc-throughput-data` is named as the list to change, because nothing enforces that the three agree.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
…t as no change Review found the new logic defeats itself on the one run where it matters, and the case is live on three open PRs today. `statusChanged = prev && cur && prev.status !== cur.status` is falsy whenever `prev` is null - which is every throughput comment posted before the payload existed. #405, #400 and #29 each carry a marker-bearing comment with no payload right now. So the first green-to-red on any of them would have been applied by silently editing a comment far up the page: precisely the failure this change was written to remove, on the first run after it merges. Now `cur && (!prev || prev.status !== cur.status)`. Unknown is not the same as unchanged. The cost is one extra comment per PR carrying a legacy one, once, which is the right way round. TWO HARDENING FIXES FOUND ALONGSIDE IT The comment lookup is paginated. listComments returns 30 per page, so on a busy PR the report scrolls off page one, every push posts a fresh comment instead of updating, and the delta disappears - the fifteen-comments problem returning by a different route. And it is restricted to comments this workflow could have written. The found comment stopped being decoration when its payload began steering the update-versus-post decision: it is an input now, so anyone able to comment could otherwise plant a marker. The status is also stripped to letters before it reaches a comment body, where `rate` and `ratio` were already type-checked and it was not. Found by ce-code-review, verified here before acting rather than taken on the report's word: the legacy-comment case was confirmed by querying the three PRs for marker comments lacking a payload. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
|
@claude review this Two passes have already run on this branch (ce-simplify, then ce-code-review with a cross-model adversarial arm), and their findings are either applied or listed below. Please focus on what those could not settle rather than re-deriving them. The workflow behaviour is the part nothing local can verify. Known and unfixed, in the order I would weigh them:
Coverage is narrower than the body first claimed and the body has been corrected: the runtime self-test reaches the Five decisions are deliberate and defended in the code comments: the status is set by variable assignment rather than passed as an argument (passing it corrupted all six messages); the self-test runs the reporter rather than grepping it (the grep version passed on those six); update-in-place with a new comment only on status change; the verdict is advisory, not blocking; and incomparable readings are shown with a per-row caveat rather than hidden. |
|
Claude finished @astubbs's task in 3m 32s —— View job Review — the workflow-behaviour findings
As asked, I focused on the workflow behaviour list rather than re-deriving what Confirmed, in the order you weighed them:
Nothing additional found. I checked for a concurrent-run variant of #1 (two workflow runs racing on the same comment list) — The five deliberate decisions you listed (status-by-assignment, reporter-run self-test, update-in-place with new-comment-on-status-change, advisory verdict, incomparable-but-shown readings) all read as correctly implemented and match their code comments — no objection to any of them. |
Four fixes to the step that POSTS the report, all about the step misbehaving rather than about what it measures. Raised on this PR's review and each confirmed against the code before being taken. RETIRE BEFORE CREATING. On a status change the step created the new comment and only then rewrote the old one's marker to `(superseded)`. There is no transaction across two API calls, so a failure between them left TWO comments carrying the live marker - and because the lookup paginates oldest-first and takes the first match, every later run would latch onto the STALE one and update it forever while the new report sat orphaned. Silent, self-perpetuating, and worst on exactly the status change the mechanism exists to announce. Reversed, the failure mode becomes zero live markers and the next run simply posts a fresh one: degraded, obvious, self-healing. Verified by running the step's script against a stubbed API with `createComment` throwing - zero live markers remain, where the old order left two. The forward link to the new comment costs a third write, so it is now best-effort and last. By the time it runs the marker state is already correct either way, so losing it costs the reader a direct link, never correctness. CONTINUE-ON-ERROR, which the sibling step already had and this one did not. It makes one or two GitHub API writes inside the REQUIRED Performance Tests job, so a rate limit, a 500 or a script exception failed the required check for a reason unrelated to the lane or the verdict. The comment is the nice-to-have; the job's own result is not. THE DELTA NOW RENDERS `share`. It was already in the payload and was the one number left out - the raw dimensionless shape the whole method rests on, where `ratio` is derived from it and `rate` is explicitly not comparable across runners. The parts are also reordered to match the report's own table: most machine-independent first, the machine-only rate last. THE FRESHNESS STAMP LINKS THE SHA, at seven characters rather than nine. GitHub abbreviates to seven everywhere else on the same page, so the old form showed a second, different-looking abbreviation of the same commit and made a reader stop to check it was even the same one. The link points at `/pull/N/commits/<sha>`, checked to resolve and to keep the PR framing, and that is the real answer to "is this comment current?" - the commit page carries GitHub's own localised, relative timestamp, so nobody has to do arithmetic on a UTC string. A static comment body cannot produce local time itself, and that was measured rather than assumed: rendering candidates through the /markdown API in this repo's comment context, GitHub's sanitiser strips `<time>`, `<time-ago>` and `<local-time>` to their text, and the REST API exposes no user timezone. The UTC timestamp therefore stays, and stays plain, because it is the one thing the links cannot do - show at a glance that a run happened without navigating anywhere. One finding worth leaving behind for whoever revisits this: `<relative-time datetime=...>` DOES survive that sanitiser with its attribute intact, and being a custom element GitHub registers page-wide it would upgrade inside user content and render reader-local, with the UTC text as a graceful fallback. It was built, then dropped: the link already answers the question, and this line's whole job is to be short.
Three report-correctness fixes and the tests for them, all raised on this PR's review and confirmed against the code. THE `no-subject` SENTINEL WAS UNHANDLED, and it was the only one of the three that was. `verdictFor` returns it when this run produced no per-method time for the subject class, and nothing between that call and the report template guarded it: the destructure left `ratio` undefined, building the template threw on `ratio.toFixed(3)`, and Node exits 1 for an uncaught throw - which is this script's own code for VIOLATION. So a subject test that never ran was reported as a REGRESSION, and did it while writing no report at all, breaking the file's own "EVERY EXIT WRITES A REPORT" rule by the single exit that never reached `writeReport`. Now guarded beside its two neighbours, and deliberately AFTER the control guard: when both are missing, the broader fact is the useful one, and naming the subject alone would send the reader to look at one test. THE HEADLINE STATED A DIFFERENCE FLATLY AGAINST AN UNDISCLOSED NOISE FLOOR. "This branch is about 8% faster than master" sat two paragraphs above the same comment's own statement that this test moves 13-17% run to run, so the report contradicted itself and the headline won, because the headline is what gets read. It now says which of the two it is. The verb changed with it: "measured about 8% faster" is a fact about this run, "is about 8% faster" is a claim about the branch, and only the first one is true. `NOISE_FLOOR` is the measured share spread from the table already in the library, rounded up rather than down. THE PRINTED RATIO COULD CONTRADICT ITS OWN ICON. `toFixed(3)` rounds, and at a bound the rounding crosses it: 0.4996 printed as `0.500` beside a red icon whose bound the very same comment prints as "< 0.50". A reader doing the honest thing, checking the number against the stated range, got the opposite answer from the icon. The displayed value now widens its precision until it lands in the same band as the raw one, so the two cannot disagree. Away from a bound nothing changes and three places stays the normal case. The headline and the display helper both live in lib/throughput-verdict.mjs, beside the bounds and the measurement table they depend on, so a bound cannot move without its caveat - and so both can be asserted without a run. COVERAGE. The runtime self-test reached exactly one of the reporter's `writeReport` paths. It now reaches two: `no-subject` is driven end to end by giving the reporter a control time and no subject time, which clears the earlier guards and stops before the networked reference lookup, so the case stays hermetic and needs no credentials. Its exit code AND its report are both asserted, because either alone would have passed against some version of the bug the guard exists for, and its fixture is registered with the restore list so an interrupted run cannot leave a stray failsafe report behind. Still not covered, and stated rather than glossed: the main verdict path. It needs `gh run list` and downloaded artifacts, so reaching it offline means putting a seam into production code - a design call left open rather than made here.
…the solved one Seen on this branch's Integration Tests. The branch carries one workflow and two Node scripts and no Java at all, so nothing in it is reachable from the code under test: this is master state, not the branch's. Recorded rather than diagnosed, per the ledger's own rule - the evidence expires with the logs, and a sighting has to be written down before the branch that saw it merges. IT WEARS A SOLVED BUG'S SIGNATURE FROM THE WRONG SIDE, which is the part worth writing down. The assertion is the one solved on 2026-08-05 as a nudge race - `checkHowManyRecordsWithKeyPresent` expected 2, got 1 - and that diagnosis was the reader stopping early and missing the compaction records past its window. Here the scan is holding the compactor and it is the ORIGINAL that is absent, which a short window cannot produce. Left as a hypothesis, not a diagnosis: one sighting is not a rate, and it has not been reproduced. Kept as its own row and section rather than folded into the `[2] earliest` entry that carries the same assertion text on a different parameter, because whether the two share a cause is precisely what is open.
All six are now fixed, in 1. Create-then-retire has no compensation — fixed by reversing the order. The retire happens first, so a failure between the two writes leaves zero live markers and the next run posts a fresh comment, instead of two live markers with the stale one winning forever. Verified rather than reasoned about: the step's script was run against a stubbed API with 2. Missing 3. Delta omits 4. Headline asserts a flat percentage against an undisclosed noise floor — the headline now discloses the floor in the same sentence as the number, and the verb changed with it. "measured about 8% faster" is a fact about this run; "is about 8% faster" was a claim about the branch, and only the first is true. 5. Rounded ratio can contradict its icon — the printed value now widens its precision until it lands in the same verdict band as the raw ratio, so 6. On the coverage gap. The runtime self-test now reaches two Also on this push, and not from the review: the freshness stamp now links the sha — seven characters, matching GitHub's own abbreviation everywhere else on the page — at Unrelated to this PR: None of the five deliberate decisions were touched. |
Comment only - no behaviour change. The stamp still links
`/pull/N/commits/<sha>`, and this records the measurement that says it should,
so the next person does not spend the afternoon rediscovering it.
THE WISH. The reader is already on the PR conversation, so the sha should scroll
them to the commit's row there rather than navigate to the commits tab.
`#commits-pushed-<7char>` looks exactly like the anchor for that job.
IT NEVER RESOLVES FOR THE SHA THIS STAMP PRINTS, and the reason is two separate
facts, either of which alone would sink it. Read off the rendered PR page by
parsing its timeline items in DOM order and listing which commits each contains:
* The anchor keys on the push group's OLDEST commit. This stamp prints
`head.sha`, the newest. On a group of more than one they are never the same.
* A single-commit group - the one case where oldest and newest coincide - has
no anchor at all. It renders as a lone row badged `octicon-git-commit` with
no `id`, where a multi-commit group renders as `octicon-repo-push` and
carries one.
So the case that would have worked is precisely the case GitHub does not
anchor. On this branch: two `repo-push` groups of two commits, one of three, and
one commit sitting alone and anchorless between them. Eight commits, three
anchors, and the head sha among them ZERO times.
There is also no per-commit anchor to fall back on. The only fragment targets a
PR conversation emits are `#commits-pushed-<7char>`, `#issuecomment-<id>`,
`#pullrequestreview-<id>`, `#ref-pullrequest-<id>` and the issue itself; the
commit rows inside a push group carry no `id`, so the row the stamp names is not
addressable.
DERIVING THE OLDEST COMMIT INSTEAD was rejected on three counts. The issues
timeline API returns `committed` events with no push grouping, so it is not
recoverable at stamp time; `github.event.before` exists on `synchronize` but not
on `opened`; and grouping is GitHub's rendering choice rather than a record of
pushes - consecutive pushes can be coalesced - so the key is not stable for a
push after the fact. It would also make the link target a different commit from
the sha printed beside it, reintroducing in a worse form the
two-abbreviations-of-one-commit confusion the seven-character change exists to
remove.
WHY NOT SHIP IT ANYWAY on the grounds that an unknown fragment is harmless. It
is harmless, and it is also unconditional: the link would land the reader at the
top of a long PR every single time while claiming to point at a commit. A link
that always leaves the page but always arrives beats one that never leaves and
never arrives.
…g a human's
All three faults are PRE-EXISTING ON MASTER and none is introduced by this PR.
They are fixed here rather than on their own branch because it is the same
defect class, in the same file, as the throughput step's lookup a few hundred
lines above - and a separate branch would buy a second review cycle and a second
CI run to change two lines.
UNPAGINATED `listComments`, WHICH IS LATENT RATHER THAN VISIBLE - and the reason
why is the part worth keeping. The issue-comments API returns 30 per page,
OLDEST FIRST, so a sticky comment born EARLY sits on page one forever and keeps
being found. It only breaks when the comment is born LATE: created once the PR
already has 30+ comments it lands on page two, `find` never sees it again, and
every later push takes the create branch. So it does not miss once and recover -
it duplicates on EVERY push, and each duplicate is itself on page two. Measured
exposure, so as not to overstate it: the busiest PRs in this repo today are 13,
11, 11, 10 and 9 comments, all comfortably above the fold. Nobody has met this,
and on current traffic nobody would.
THE LOOKUP COULD OVERWRITE A HUMAN'S COMMENT, which is the worse of the two and
was not on the list. `includes('SpotBugs Report')` matches ANY comment carrying
that string - including a person quoting the report - and `find` takes the
OLDEST match. So somebody who wrote "the SpotBugs Report shows 318, is that
right?" before the bot first posted would have their comment silently replaced
by the next push. Restricting to a Bot author is what makes the lookup mean "the
comment this workflow wrote" rather than "any comment on the subject". Fixed
here because it is one clause in a line already being edited for the pagination,
and leaving a known comment-destroying bug in code being touched anyway is not a
defensible place to draw a scope line - but it is the one change here nobody
asked for, so it is easy to drop if that call goes the other way.
Both halves verified by running the step's script against a stubbed API: a
human-only mention now CREATEs instead of overwriting; a human followed by the
bot updates the BOT's comment; bot-only updates; empty creates. All four
paginate at 100 per page.
MISSING `continue-on-error`, the same omission the throughput step had. This step
summarises analysis that has already run and already posted its annotations, so
a rate limit or a 500 while commenting would fail the lane for something that is
not the lane's business.
This was the only remaining unpaginated `listComments` in the tree; swept and
confirmed, including `bin/quarantine-lane-report.sh`, whose own sticky-comment
upsert already passes `--paginate`.
astubbs
changed the title
|
@claude review this Second round. The first was against The three I would most like argued with:
Deliberate, defended in code comments, please do not recommend reversing without engaging the reason: the status is set by variable assignment rather than passed as an argument (passing it corrupted all six messages); the self-test runs the reporter rather than grepping it (the grep version passed on those six corrupted messages); update-in-place with a new comment only on a status change; the verdict is advisory rather than blocking; incomparable readings are shown with a per-row caveat rather than hidden; and the stamp links the commits tab rather than a PR-timeline anchor, because Known coverage gap: the runtime self-test reaches 2 of the reporter's 7 write paths. The main verdict path needs the network, and adding a test seam to production code was left as a design call. |
|
Claude finished @astubbs's task in 4m 2s —— View job Review — second round, weighing only the new material
1.
|
…exist The status-change path retires the old comment BEFORE creating the new one, and that order is the safety property: a failure between the two writes leaves ZERO live markers and the next push posts a fresh comment, rather than two live markers with the stale one winning forever. The retirement text, though, named a PLACE - "Superseded by a newer report further down this conversation" - and that write happens before the create. This step carries `continue-on-error: true` by design, so a failed `createComment` is swallowed and the promise is simply never kept. A reader then scrolls down looking for a report that is not there. WHAT MAKES IT WORTH FIXING is that the state is permanent, not transient. The retired marker `<!-- pc-throughput-report (superseded) -->` is not a substring of the live marker the lookup searches for, so no later run ever finds this comment again to correct it. The next push does self-heal a LIVE comment - `existing` is falsy, so it takes the create-fresh branch - but the orphaned false pointer is never touched by anything, ever. `retiredBody` now takes the whole trailing note rather than a noun slotted into a fixed "Superseded by X" sentence, because the two writes cannot honestly promise the same thing. The pre-create note claims only what is already true at that moment: this reading is superseded, and a fresh report should follow for this push. The post-create note, written once `created` demonstrably exists, still links straight at it. THE `continue-on-error` TRADE IS UNCHANGED AND DELIBERATE. This is a REPORTING step inside the REQUIRED Performance Tests job, so a rate limit, a 500 or a script exception must not take the required check down for a reason unrelated to the lane or the verdict. Reversing that to make the create loud would trade a misleading comment for a red required check, which is the worse failure. The wording was the defect, not the ordering and not the swallow. Found by the second `@claude review` round on this PR, which traced the failure path through the code rather than reasoning about it abstractly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
…ver reaches `displayRatio` widens precision until the printed value lands in the same verdict band as the raw one, so a rounded number can never contradict the icon beside it. Every case the suite already pins - 0.4996, 0.6996, 0.98042, exactly 0.50 - resolves on the [3, 4, 5, 6, 9] ladder. None of them reaches the fallback underneath it. THE FALLBACK IS REACHABLE, AND IT IS LOAD-BEARING. A ratio a hair under a bound, here 0.5 - 1e-12, rounds to "0.500000000" at EVERY rung: each one reads `flag` against a raw `fail`, the loop finds no agreeing rung and returns nothing. `String(ratio)` then carries the invariant on its own, and does so correctly, because it yields the shortest representation that round-trips the double exactly - so the printed text always parses back into its own band, at any distance from the bound. VERIFIED IN BOTH DIRECTIONS RATHER THAN ASSERTED. Replacing the fallback with a final `ratio.toFixed(9)` - the plausible tidy-up, since the ladder already ends at 9 - fails exactly the two new assertions and nothing else: "expected 0.499999999999, got 0.500000000" and "expected fail, got flag". Restoring it returns the suite to green. Without that control arm a new test here would be indistinguishable from one that cannot fail, which is precisely the trap this PR already fell into once, when a grep-based self-test passed on all six corrupted messages. The test also pins the intermediate fact - that all five rungs disagree with the raw band - so a future reader can see WHY the fallback is entered, not just that it is. `displayRatio`'s logic is unchanged. The second `@claude review` round traced these boundaries by hand, found no correctness bug, and identified the untested ladder-exhausted path as the one remaining gap; this closes it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
|
Round 2 handled. One finding fixed, one gap closed, one recommendation accepted as-is. Replying here rather than in-thread because this round opened no inline review threads - the findings live in a top-level comment body, so there is nothing to reply into or resolve. 1.
|
The Chaos Pain Suite went red on this branch's CI with the confluentinc#857 revoke-path discriminator: `ChaosRevokeUnderWorkDrainIT.revokeUnderDrainingStopsStaysProtocolHonest` killed by the `unclassified failure cause` SLO, with the poll thread found BLOCKED on an `AtomicBoolean` monitor held by its own control thread, inside `onPartitionsRevoked` -> `commitOffsetsThatAreReady`. Same arm, same SLO, same monitor and holder pair as the fifth capture. RECORDED FOR THE SEED, NOT FOR THE SIGNATURE. The signature is already documented five times over and the entry says so outright: a seventh capture is worth less than one replay of any of the six, and the file's standing ask is a replay with #29's `tryLock()` applied, which nobody has run. This exists because the seed (`1075685698131288762`) dies with the log, and the ledger's own rule is that a sighting is written down before the branch that saw it merges. TWO THINGS IT DOES ADD. The line numbers moved again - `:1780` / `:580` against the fifth capture's `:1589` / `:552` and the first four's `:1585` / `:548` - so that capture's advice to match by method and monitor rather than by line is now load-bearing rather than precautionary; a filer going by line number would have missed this one. And it is the cheapest not-PR-introduced control arm in the file: this branch's diff is one workflow file, three Node scripts and one markdown note, with no Java at all, so there is no compiled change the chaos suite could reach. Every earlier capture had to argue the branch was not the cause by resolving frames or naming subsystems; here the category of change excludes it. Also noted as a small addition to the rate question, and explicitly not as a rate: the same lane passed on four earlier heads of this branch and failed on the fifth, within about three hours, none of them carrying Java. Not this PR's defect, and not fixed here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
…st seam The reporter has EIGHT `writeReport` exits, and the split that matters is not covered-versus-uncovered but local-versus-networked. Exactly three are decided from local files before any network call - `no-rate`, `no-control`, `no-subject` - and the other five (`bootstrapping`, `check-failed`, `no-reference`, `incomparable`, and the verdict itself) sit behind `gh run list` and an artifact download. The self-test already drove two of the three local ones. This adds the third, which costs nothing: a summary file that exists and is non-blank but carries no usable `recordsPerSecond`. That is deliberately not the same as a missing or blank summary, which exits NOTHING_IN_SCOPE several lines earlier under a different contract - conflating the two would assert the wrong exit code and pass for the wrong reason. WHAT THIS CHANGES ABOUT THE OPEN QUESTION. The coverage gap was recorded as needing a design call about putting a test seam into production code. That is still true, but it is now scoped to five specific networked paths rather than to "the rest of them", and no free coverage is being left on the table while that call is pending. Verified in both directions rather than asserted, in the way this file's own history demands: deleting the `reportStatus = 'no-rate'` assignment - the exact bug class that motivated running the reporter instead of grepping it - fails the new assertion with `expected no-rate, got unset`, and nothing else moves. Restoring it returns the suite to green with the reporter byte-identical. Also wraps the one assertion line added by the previous commit that ran to 149 characters, against `.editorconfig`'s 120. Naming the intermediate reads better than the one-liner did, and the file's longest line is back to what it was before this branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
… head The entry was written while the failing head was the newest one, so it could only say the lane had failed. The next head that completed passed the same lane, which is the half that makes "intermittent" mean anything: the failure did not reproduce on a re-run of nearly the same tree, neither of which carried any Java. Also sharpens the disclaimer rather than just updating the tally. The previous wording called it "one branch's run of five", which invites the reader to treat it as a denominator. It is a handful of runs gathered while doing something else, not an experiment anybody designed, and the file's standing ask is unchanged and unaffected: replay one of the six seeds with #29's `tryLock()` applied. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
astubbs
changed the title
Four commits, none touching this branch's files. Read before merging, per AGENTS.md: 1001c69 ci: PR CI comments say what they mean, and survive their own failures (#407) 000de5d ci(lincheck): run the lane that has never run, and flip the arm it found dark (#404) b36ad94 fix(core): stop transactional batches failing and reprocessing records that succeeded (#257) 625a8c1 docs(solutions) #183: a defect matches a report on direction and on the reporter's build The package-rename rule is still live on master, and does not bite here: this branch touches no Java and no package paths, and the sweep grep finds io.confluent only in the rename, copyright and mutation scripts that carry the pattern as data. Worth recording rather than passing over: #404 lands the 2026-09-01 sightings into docs/inflight/test-load-tightness-flakes.md, so the committedOffsetRemoved flake this PR's CI hit is now tracked on master rather than only on sixteen unmerged branches. That was the worked example in docs/inflight-tool.md - the example stands as the record of what the tool found on the day, and the ledger has since caught up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UTX8obQMsjs9kq2rkpU5cZ
…s-bug Master gained one commit since this branch last merged it - #407, the CI comment work - and the only files touched on both sides are the two sighting registers. BOTH SIDES' SIGHTINGS SURVIVED, WHICH IS THE ONLY THING THAT MATTERED HERE These are append-style registers, so a resolution that picked a side would have silently dropped somebody's evidence. Checked rather than assumed after the merge: #407's sixth 857 capture and its seed are present, its committedOffsetRemoved sighting is present, and this branch's own fifth open item and its withdrawal of the "never a product defect" claim are both still there. ONE INHERITED LINE NEEDED AN ATTESTATION #407's capture entry proposes replaying a seed with this branch's tryLock() applied - a sentence that reads correctly after the merge, but which the self-reference gate cannot know is safe. Marked, and the reference fully qualified while there. The marker had to move: it sat above the paragraph's first line, and the gate's single-line form covers only the line immediately after it, which was not the one carrying the reference. The paragraph is rewrapped so the reference sits directly under its marker. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UH2q829FDKQcqQseE8Cp7q
Two commits: #29's confluentinc#857 revoke-path deadlock fix, and #407's CI comment repairs. No Java overlap again - #29 works on the commit lock and the revoke path, this branch on offset decoding - but one inherited argument lands squarely on a note this PR ships, and that is the part worth reading. #29 GIVES A MECHANISM TO A NOTE THAT ONLY HAD A SYMPTOM docs/inflight/core-fail-policy-escapes-the-rebalance-callback.md recorded that FAIL stops the consumer by throwing out of onPartitionsAssigned, and framed the problem as SURFACING: an opaque "User rebalance callback throws an error" instead of PC's own fatal path. #29 puts a never-throws contract on the revoke side of exactly that hazard - PartitionStateManager#deregisterPartitionCounters, grep it for `never-throws contract`. Its reasoning is that the callback runs on the broker-poll thread inside poll(), so an escape kills that thread, which is the only producer of commit responses, so every later commit blocks until it times out. It names that as the confluentinc#857 family's worst failure shape. onPartitionsAssigned is the same thread and the same wrapper. So the open question is no longer "is the message clear?" but "is a FAIL stop a stop at all, or a hang wearing one?" - and this branch's own test does not answer it: failPolicyStopsRatherThanDiscardingForeignMetadata calls wm.onPartitionsAssigned directly and asserts the throw, never reaching a real poll loop. The note now says so and names the experiment that would settle it. Not settled here: FAIL is opt-in since the default moved to IGNORE, and a clean fatal exit reaches beyond the offsets package into the engine's error handling. THE CONFLICT, AND A CLAIM OF MINE IT KILLED One conflict: both sides added a committedOffsetRemoved[1] latest sighting on the same day, #407's and this branch's. Merged into one row with both jobs cited, because they are the same test and the same parameter. #407's captured WHICH record survived - the compactor, not the original - and the 2026-08-05 nudge-race mechanism cannot leave that behind. This branch's note had called its own sighting a RECURRENCE of that solved mechanism, on the strength of a matching signature: same test, same parameter, same expected 2 got 1, same clean probe. That reading is withdrawn, and the withdrawal is written where the row is read rather than quietly dropped, because matching a signature is not classifying a failure and this is the worked example. The #207 sighting is now unclassifiable: which record survived was never captured, and its log has since expired - GitHub returned zero bytes for it. The ledger's own rule about evidence expiring with the logs, paid in full. What survives is the branch clearance on mechanism, which never depended on the signature: this test removes the committed offset entirely, so there is no metadata for an offset-decoding change to reach.
…ticky-comment machinery (#409) WHAT WAS WRONG A quarantined test that starts PASSING is the most consequential thing the lane report can say: it means the fix landed, and its `@Quarantined` annotation plus its `docs/quarantined-tests.md` entry must both be deleted. That transition was announced by silently editing a comment far up the page - no notification, no visible change to anyone reading the PR top to bottom. The most important message was the one least likely to be seen. THE FIX The report now embeds a machine-readable payload, reads its own previous comment back to render a DELTA naming which test moved, and posts a FRESH comment when any row's outcome changes - retiring the old one first and linking it forward. Every comment is stamped with the head sha, a commit link, the run and the time, so an in-place update is visibly fresh rather than looking dead. It has since fired in production on a real transition, retiring two comments and chaining the forward links correctly. SHARED, NOT COPIED The five mechanics behind that are domain-free, and all five already existed for the throughput comment. They now live in one module with two thin consumers, replacing ~198 lines of inline `github-script` JS in a workflow and a ~25-line `gh api` upsert in shell. That shell copy carried two of the three defects #407 had already fixed for throughput - including a `gh api` failure under `set -e` that could red a job whose lane had run fine. Measured like-for-like the modules are more CODE than they replace, and the reason is that it was never a like-for-like extraction: two unlike implementations became one core plus two vocabularies, and the quarantine side GAINED the delta, the retirement and the stamping it never had. CODECOV AS A QUERYABLE TEST HISTORY `bin/inflight.mjs codecov` exposes per-test outcome and wall-clock per commit - `codecov test <name>` for "which commit did this start failing at", `codecov flaky` for the sighting evidence a quarantine entry needs, `codecov slow` for what owns the CI wall-clock. Unauthenticated, because the repo is public, so it works from a fresh sandbox with no setup step to forget. It answered its first real question during this branch's own CI: an integration test went red, and `prior-art` plus `codecov test` established a known tracked flake with a rate - one failure in seven runs, on a commit that deletes a file - without re-running a single build. That sighting is recorded, and it is the first entry in that ledger carrying a rate rather than a single job log. WHAT REVIEW FOUND, AND WHY IT MATTERS MORE THAN THE FEATURE Seven local reviewer lenses, an independent cross-model pass on Codex, and a Claude review. The local lenses share this model and found nothing of the class Codex found: seven ways the new flake tool could exit 0 with an apparently complete "no flakes" - in the exact situation a reader uses it to justify REMOVING a quarantine. A truncated walk printing a clean negative; an empty corpus reported as an absence of flakes rather than a failure to read it; a skipped test rendered as a failure; branch identity omitted so a PR-only failure read as master-state, which is the distinction the quarantine rule turns on. All fixed, each with a test that was red-proofed by reverting its guard. Two of this branch's own defects were the same shape and are worth naming: a flag's VALUE was read as the search term, and the fix for that dropped the query whenever the flag was absent. Both silent, both plausible, both caught by review rather than by use. KNOWN AND DELIBERATELY NOT FIXED HERE The two `codecov/project/*` gates fail on branches that change no Java. Proven from the upload logs: the CLI does not expand `**`, so `files:` matches nothing and a tree-wide fallback uploads everything - on master that gives BOTH flags the whole tree, while a PR's suite job has only its own half. The delta measures that. It is the same inert-`files:` defect this change fixes for the test-results upload; four instances remain. Not fixed here because it alters what a required merge gate measures, and that should be the only thing in its diff. Also filed rather than fixed: emptying the lane leaves its last report live saying ACTION REQUIRED, because every step after the empty check is gated on the lane being non-empty. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two commits - #400's in-flight tooling front door and #409's quarantine status-change report. One Java file between them, a script test, so nothing here reaches this branch's code. THE CONFLICT: THREE SIGHTINGS, NOT TWO, AND BOTH SIDES HAD IT WRONG test-load-tightness-flakes.md again, and again because both sides appended a committedOffsetRemoved[1] latest sighting to the same row. This branch's row said two sightings and named #407 and #207; master's said two and named #407 and #409. Neither is right on its own: there are three, and dropping either side's would lose the evidence the ledger exists to keep. Resolved to one row citing all three with what each contributed, which is the part that matters: #407 captured WHICH record survived - the compactor, not the original - and that rules out the 2026-08-05 nudge-race mechanism; #409 attached a RATE; #207 captured neither. Both prose sections are kept because they say different things - one withdraws a claim, the other adds the rate - and neither restates the other. A RECOVERY ATTEMPTED, AND RECORDED AS FAILED #400 landed `node bin/inflight.mjs codecov test <name>`, which reads per-commit outcomes from history that outlives a CI log - exactly the gap this branch recorded when the #207 sighting's log returned zero bytes. Ran against this test it covers only the branch #409 ran on, so that sighting stays unclassifiable. Written into the note as an attempt that does not reach it, so the next reader does not run the same query hoping for a different answer. INHERITED, AND IT INDICTS HOW THIS SESSION SEARCHED #400 adds a rule worth acting on rather than filing: grep and find read the WORKING TREE, and roughly two thirds of this repo's docs exist only on branches that have not merged - so a working-tree search returns a false negative carrying the authority of a completed check, and `bin/inflight.mjs prior-art` is what searches every ref. The prior-art checks in this session used `grep -rln` over docs/, which is precisely the shape that rule names. Unit lane green across all modules; bin/check-all.sh 15/15.
Four commits, no Java touching this branch's paths. Three conflicts, all the same shape as before - two writers appending sightings to the same ledgers - and this time both writers were recording the SAME two flakes, so neither side's count was right and taking either whole would have lost real observations. MdcContextPropagationTest: master had three sightings (one local on #262, two on #416's CI across consecutive heads); this branch had a fourth, its own local suite, and a fifth that matters more than the rest - MASTER'S OWN `full build` at ebd8bcb. Unioned to five, with the master one called out, because it is the one that stops being a test problem: `build` is master's only coverage uploader, so its failure is why #414's merged glob fix has not taken effect and why every open PR still sees red per-flag gates. committedOffsetRemoved[1] latest: master had #407, #409 and #416; this branch had #407, #207 and #409. Union is four, and the row now says which of them captured what - #407 the surviving record, #409 a rate, the other two neither. Master also re-added a processInKeyOrder row at two sightings where this branch already carries the six-sighting version from an earlier merge; its copy is dropped rather than landing a duplicate row under one test name. bin/check-all.sh 15/15, core compiles. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01228AtRiHyTzeH7SjcXzgNj
…before re-running it The sharded lane's first pull_request run went red on two tests this branch does not touch, and both already have ledgers. Recording them before the re-run, per the rule that a seed dies with its log. ChaosChurnStormIT, INSTANCE_STALL/NO_WORK_COMPLETED, seed 4788502970202706178, on shard 4/4 - one fork on its own VM, the gate's configuration exactly. That is the fifth gating firing of this detector on this class since 2026-08-25 and the fourth in two days, on three seeds; the ledger now says the rate, not any seed, is the property worth measuring. PartitionStateCommittedOffsetIT.committedOffsetRemoved[1] latest, expected 2 got 1 at forkCount=4, the fifth sighting; the survivor's value length says it was the compactor again, the shape #407 captured, so it is the unsolved mechanism rather than the solved 2026-08-05 one. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Follow-up to #401, which began as review of the throughput comment it posts and grew into the comment-posting steps around it.
Three strands, and the third was not planned: the comments now say what they mean (a reviewer read
1.713as "1.7x faster than master" when the real figure was1.081), they survive their own failures (a failed write no longer fails a required job, or leaves a PR with two live markers), and the SpotBugs comment stops destroying human comments - its lookup matched any comment containing "SpotBugs Report", oldest first, so a human quoting the bot had their comment silently overwritten.What review asked for, and what was already true
Two of the four asks were already implemented, which is worth stating rather than silently "fixing":
The real gaps
The comment could not show a delta, because it kept no readings. Each report now ends with one machine-readable line — status, rate, ratio, share — and the workflow reads it back off its own previous comment to render "since the previous push: ratio X → Y, share X → Y, rate X → Y (+n%)". Reading our own last comment is the only store available: no series is kept anywhere, by an explicit earlier decision, and updating in place otherwise destroys the only record of what the last push measured.
It is labelled a delta and not a verdict. This test's run-to-run spread on one unchanged commit is wide enough that a single push of difference is noise far more often than it is a change.
A status change edited a comment nobody was looking at. Editing something thirty scrolls up produces no notification and no visible change to a reader going top to bottom, so a run that went green→red announced itself silently. Now a fresh comment is posted at the bottom and the old one is retired — marker renamed so later pushes stop targeting it, and a link forward so a reader who lands on the stale one is not stranded. Number-only movement still updates in place; only the verdict earns a new comment.
The retire happens BEFORE the create, and the order is the safety property. There is no transaction across two API calls, so one can fail after the other succeeded. Creating first and failing to retire leaves two comments carrying the live marker — and since the lookup paginates oldest-first and takes the first match, every later run latches onto the stale one forever while the new report sits orphaned. Retiring first fails the other way: no live marker at all, so the next run simply posts a fresh one. The forward link costs a third write and is therefore best-effort and last, since by then the marker state is already correct.
An in-place update looked dead. The body could be identical to last week's with nothing to say a run had happened. Every report now carries a stamp: the head SHA as a link to that commit in this PR, a link to the run, and the time. The SHA link is the part that matters — a static comment body cannot render the reader's local time (GitHub's sanitiser strips
<time>,<time-ago>and<local-time>, and the REST API exposes no user timezone), but the commit page it links to carries GitHub's own localised relative timestamp. Nobody has to do arithmetic on a UTC string; they click. It is abbreviated to seven characters to match GitHub's own abbreviation everywhere else on the page."No comparable reference" read as "no baseline exists". Baselines usually do exist; they just ran a different set of tests, most often because the PR itself enabled or disabled some. The message now says how many were found and that a matrix change is usually the PR's own doing — and the readings are shown, with "different test set - not a control" in its own column on every borrowed row, so a number cannot be lifted out of the table without its warning.
The report must not contradict itself
Three fixes from the
@claude reviewround, all about the report misleading a reader who does exactly the right thing with it.The headline stated a difference flatly against an undisclosed noise floor. "This branch is about 8% faster than master" sat two paragraphs above the same comment's own statement that this test moves 13-17% run to run — so the report contradicted itself and the headline won, because the headline is what gets read. It now discloses the floor in the same sentence, and the verb changed with it: "measured about 8% faster" is a fact about this run; "is about 8% faster" was a claim about the branch.
The printed ratio could contradict its own icon.
toFixed(3)rounds, and at a bound the rounding crosses it:0.4996printed as0.500beside a 🔴 whose bound the very same comment prints as "< 0.50". The displayed value now widens its precision until it lands in the same verdict band as the raw one. Away from a bound nothing changes.A subject test that did not run was reported as a regression.
verdictFor'sno-subjectsentinel had no guard: the destructure leftratioundefined, the template threw onratio.toFixed(3), and Node exits 1 for an uncaught throw — which is this script's own code for VIOLATION. It failed loudly for the wrong reason and wrote no report at all, breaking the file's own "every exit writes a report" rule by the single exit that never reachedwriteReport. Pre-existing, taken here because the fix is a ten-line guard mirroring the two beside it.The posting step could fail the required check
It makes one or two GitHub API writes inside the REQUIRED Performance Tests job and, unlike its sibling, had no
continue-on-error— so a rate limit, a 500 or a script exception failed the check for a reason unrelated to the lane or the verdict. The comment is the nice-to-have; the job's own result is not.One more comment step, same defect class
Riding in rather than taking its own branch, because it is the same file and the same reasoning. All of it is pre-existing on master; none is introduced here.
The SpotBugs summary step called
listCommentsunpaginated. That is latent rather than live, and the reason is worth stating: the API returns 30 per page oldest first, so a sticky comment born early stays on page one forever and keeps working. It only breaks when the comment is born late — created once a PR already has 30+ comments, it lands on page two,findnever sees it again, and every later push takes the create branch. So it would not miss once and recover; it would duplicate on every push. Measured exposure, so as not to overstate it: the busiest PRs in this repo today are 13, 11, 11, 10 and 9 comments, all comfortably above the fold.It also matched any comment containing "SpotBugs Report" — including a human quoting it — and took the oldest match, so somebody who asked a question about the report before the bot first posted would have had their comment silently overwritten. It now requires a Bot author, the same way the throughput lookup does.
And it posted with no
continue-on-error, the same omission fixed above.This was the only remaining unpaginated
listCommentsin the tree;bin/quarantine-lane-report.shalready passes--paginate.A mistake worth reading, because the test missed it
The status began as a second argument to
writeReport. Inserting it meant locating each call's closing paren, and the report bodies are template literals containing escaped backticks — so all six insertions landed inside the prose, producingcontrol classes (`...`, 'no-control') produced a timeand an empty payload.The self-test written to prevent exactly that passed on all six, because it grepped for the literal rather than checking where it was.
So the status is now set into a variable on the line before each call, which cannot fail that way, and the test runs the reporter and reads the payload back as the workflow does. Verified in both directions: reintroducing the corruption fails it with
expected no-control, got unset; the previous test stayed green.Coverage, stated rather than implied
The reporter has eight
writeReportexits, and the split that matters is not covered-versus-uncovered but local versus networked. Exactly three are decided from local files before any network call —no-rate,no-control,no-subject— and the runtime test now drives all three, each reached by shaping only the files on disk so the case stays hermetic. Exit code and report are asserted every time, because either alone would pass against some version of the bug the guard exists for.The other five —
bootstrapping,check-failed,no-reference,incomparable, and the main verdict path — sit behindgh run listand an artifact download. Reaching them offline means putting a test seam into production code, and that is a design call left open rather than made here. What has changed is its scope: it is a question about those five specifically, not about "the rest of them", and no free coverage is being left on the table while it is pending.The two pure helpers the verdict path depends on (the headline sentence and the ratio display) are unit-tested directly — including
displayRatio's ladder-exhausted fallback, the one branch of it no other case reaches — so the findings from both review rounds are covered even though the path that renders them is not. The incomparable path is still verified by parse and by reading, not by execution. That is weaker, and it is why this section exists.The second review round asked for one thing
It did not ask for the design to change.
continue-on-errorswallowing a failed create after theold comment has been retired is the right trade, and the reviewer explicitly declined to reverse it.
The wording written into the retired comment was the defect: it named a PLACE - "Superseded by a
newer report further down this conversation" - and that write happens before the create that may
never succeed.
The false pointer is permanent, not transient, which is what made it worth fixing rather than
noting. The retired marker
<!-- pc-throughput-report (superseded) -->is not a substring of thelive marker the lookup searches for, so no later run ever finds that comment again to correct it.
The next push heals the live comment -
existingcomes back falsy, so it takes the create-freshbranch - and leaves the orphan untouched forever.
retiredBodynow takes the whole trailing note rather than a noun slotted into a fixed"Superseded by X" sentence, because the two writes cannot honestly promise the same thing. The
pre-create note claims only what is already true when it is written - this reading is superseded, a
fresh report should follow for this push - and names no place. The post-create note, written once
the new comment demonstrably exists, still links straight at it.
The round's other two questions came back as no-change, which is the useful outcome for both.
displayRatio's boundary behaviour was traced by hand and found correct - including the adversarialcase that exhausts the entire
[3, 4, 5, 6, 9]ladder and falls through toString(ratio), whichholds because that is the shortest representation round-tripping the double exactly. That fallback
was the one branch of the function no test reached, so it is now pinned, verified in both
directions: replacing it with a final
toFixed(9)fails exactly the two new assertions andnothing else. The SpotBugs author guard was judged better bundled than split, and stays bundled.
One sighting, riding along
The
Chaos Pain Suitewent red on one head of this branch carrying the confluentinc#857 revoke-pathdiscriminator - the poll thread found BLOCKED on a monitor held by its own control thread, inside
onPartitionsRevoked->commitOffsetsThatAreReady. Not this PR's defect and not fixed here:the diff has no Java in it at all, and the lane passed again on the next head that completed.
It is the sixth capture of a signature already documented five times, so it is recorded for its
seed, which dies with the log, and for the two things it does add. The frame line numbers moved
again -
:1780/:580against the fifth capture's:1589/:552- which turns that capture's"match by method and monitor, not by line" advice from precautionary into load-bearing. And because
this branch carries no compiled change the chaos suite could reach, it is the cheapest
not-PR-introduced control arm in that file: every earlier capture had to argue the branch was
innocent, this one does not.
Checklist
docs/features/— N/A - CI reporting, not a library featuredocs/inflight/working note started at the PR's first commit — N/A - a scoped follow-up to ci(perf): a throughput check derived by conservation, against recent master runs #401, fully described here and in the commit bodiesce-simplifyandce-code-reviewlocally — N/A - asking for@claude review thison the PR instead