Repository navigation
fix(core): the claim register refuses to certify a run whose filters deselected its proofs - #443
Conversation
…deselected its proofs Top item of the review recorded in docs/inflight/next-transactional-register-hardening.md, from #262. TransactionalClaimCoverageTest certifies that every documented transactional guarantee has a test proving it. It reads compiled @ProvesClaim annotations via ArchUnit, so it could not tell a proof that ran and passed from one that was never selected - and pom.xml documented an override that selected none of them. Reproduced with a control arm, one term changed: # default excluded.groups ParallelConsumerOptionsTest 9 TransactionalBulkCommitTest 4 ProducerManagerTest 11 TransactionalClaimCoverageTest 4 green BUILD SUCCESS # -Dexcluded.groups=transactions,performance,chaos,quarantined,lincheck ParallelConsumerOptionsTest 4 TransactionalClaimCoverageTest 4 green BUILD SUCCESS (the other two never appear) Twenty of twenty-eight tests, including every tagged proof, did not execute, while the register reported every claim covered, every parked claim explained and every sentence intact. The gate's selection criteria and the proofs' selection criteria were disjoint, which was the whole bug. A test JVM cannot see its own launcher's tag filters - surefire and failsafe pass groups/excludedGroups to the JUnit Platform, not to the tests. The pom now forwards the same two properties the filters are configured from, on both plugins, and RunTagFilter reads them. everyCoveredClaimMustHaveAProofThisRunCanSelect then fails, naming each claim and the proofs this run dropped. WHY A REFUSAL RATHER THAN A TAG. The recorded option was to tag the register @tag("transactions") so excluding the tag excluded the now-meaningless report. Rejected: it buys silence where the run needs an explanation, it is a proxy that had already gone stale - ParallelEoSStreamProcessorTest's proof, added in #429, carries no tag at all - and it would take the two source-drift checks, which depend on no test running, down with it. Nothing in the repo passes -Dexcluded.groups=transactions; every real invocation uses the default list, an empty value, or quarantined, so the loud failure breaks no workflow. WHY IT IS JUDGED PER CLAIM. A claim with two proofs, one tagged, is still proven in a run that drops the tagged one; failing there would be the register reporting a gap it does not have. OFFSET_AND_RECORDS_ATOMIC is the live case. AN ABSENT FILTER IS A DEFECT, NOT "NOTHING EXCLUDED". Reading a dropped systemPropertyVariables block as an empty filter would make the gate pass without having checked anything - the same failure, one level up and about itself. RunTagFilter raises instead, detected with a marker surefire sets rather than one this repo sets, since ours would vanish with the block. Verified by deleting the block: both the gate and its test go red with actionable messages. Outside Maven - an IDE, and pitest's minions, which run bz.stub.parallelconsumer.* with neither the properties nor the marker - absent does mean unfiltered. Tags resolve through JUnit's own AnnotationSupport, so meta-annotated and inherited cases come out as the launcher computes them - @Quarantined is a @tag("quarantined") carrier, not a @tag. Defect-class sweep: the only other gate asserting pom tag wiring is QuarantinedAnnotationContractTest#bothSurefireAndFailsafeBindTheGroupProperties, from an identical past P1 where only failsafe was wired. It named a gap the dynamic guard cannot cover - forwarding added to one plugin only, which no surefire run would notice - so RunTagFilterTest guards that too. No other ArchUnit-based gate in the tree makes a coverage claim. The note's item 1 is retired and its reasoning migrated to docs/solutions/; its item 2 carried an instruction to fix a gap "while #261 is still open", and that PR merged on 2026-08-14, so the routing advice is corrected. Verified: unit suite green (core 694 tests, 0 failures); both arms of the reproduction re-run against the final code; deleting the forwarding block verified to go red rather than silently green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…green-without-proofs
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
[superseded - a quarantined test changed outcome] 🧪🔒 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 Updated for Superseded by a newer quarantine lane report. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #443 +/- ##
============================================
+ Coverage 81.86% 82.27% +0.40%
- Complexity 1466 1472 +6
============================================
Files 95 95
Lines 5133 5133
Branches 500 500
============================================
+ Hits 4202 4223 +21
+ Misses 732 715 -17
+ Partials 199 195 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
🟢 Throughput — OKThis branch measured about 2% faster 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 9 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. Runs used: 5706686, c668acb, fb5ea93, 6573781, c30aaee, ca9c21b, 77fbba8, 7a8a7f0, 2e2705a Since the previous push: ratio 0.876 -> 1.018, share 1.708 -> 1.58, rate 90266 -> 68401 (-24.2%). One push of difference sits inside this test's measured spread - read it as movement, not as a result. Updated for |
|
@claude review this Steer: the load-bearing claim is that |
|
Claude finished @astubbs's task in 6m 22s —— View job Review:
|
|
@claude review this Where the effort is best spent, since the mechanical parts are already covered by the green lanes:
Two things worth verifying rather than assuming, both claimed in the description: that the |
|
Claude finished @astubbs's task in 5m 26s —— View job Review
Findings, by your numbered points1. The central judgement call — re-derived, holds. I grepped every workflow and every 2. Documented flag now fails the build — confirmed, and confirmed correctly. I ran it for real: 3. 4. Per-claim, not per-method — confirmed with a real example, not just the described one. Traced Base-commit reproduction — verified structurally rather than by re-running the old build. Other observations (non-blocking)
lgtm — I'd merge this once astubbs's own "lgtm" review stands; nothing here contradicts it. |
bin/check-pr-analysis-surfaces.sh separates findings on lines this diff wrote
from findings merely in files it touched. Three of the former, no inherited
ones, both real:
- everyCoveredClaimMustHaveAProofThisRunCanSelect looked the deselected map
up twice, containsKey then get. Now one get, null-checked - and the null
branch carries the reason it is passed over rather than reported, which
the containsKey form left implicit: a claim with NO proof at all belongs to
everyClaimWeSayIsCoveredHasATestReferencingIt, and reporting it here would
say the same thing twice in two different vocabularies.
- effectiveTagsOf allocated its set before knowing how many tags it would
hold. Both annotation lists are now resolved first and the set is sized
from them, which also names the two sources.
Behaviour is unchanged and re-verified rather than assumed: the default arm
stays green, the transactions-excluded arm still fails naming the same claims,
and OFFSET_AND_RECORDS_ATOMIC is still absent from that list - the untagged
proof in ParallelEoSStreamProcessorTest keeps it covered, which is the
per-claim judgement the earlier per-method revision got wrong.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014H4PsgR1bTfa8NuwxQ5D3o
Two findings from the review on #443. THE TAG RESOLUTION HAD NO TEST. effectiveTagsOf claims to resolve tags "the way the launcher does" - through meta-annotations and superclasses - and nothing exercised either path. It could not: the only meta-annotated tag carrier in the tree is @Quarantined, and a claim proof carrying it is forbidden outright by claimProofsMustLiveWhereATestRunnerWillFindThem, so no real input reaches the case. Swapping AnnotationSupport for a hand-rolled isAnnotatedWith(Tag.class) would have passed the entire suite while making everyCoveredClaimMustHaveAProofThisRunCanSelect blind to a proof deselected through a carrier - this PR's own failure class, one layer down in the gate that fixes it. The reflective half is now split from the ArchUnit wrapper so a fixture can drive it, and the new test covers a declared tag, a meta-annotated one, and an inherited one. It is red under the hand-rolled read, which is how the meta-annotation assertion was confirmed to be load-bearing rather than decorative. NOT USING @Quarantined FOR THE FIXTURE, though the review suggested it. bin/check-quarantine-registry.sh fails any class carrying that annotation without an entry in docs/quarantined-tests.md, so a fixture would read as an undocumented quarantine and could only be answered with a registry entry for a test that is not quarantined - satisfying one gate by lying to another. A local carrier proves the property that would actually regress; the reasoning is in the fixture's javadoc, where the next person to reach for @Quarantined will find it. A COUNT OF TWO IS NOT TWO PLUGINS. bothSurefireAndFailsafeForwardTheFiltersToTheTestJvm counted occurrences across the whole pom, so two copies inside surefire and none in failsafe satisfied it - the exact state its own message says it forbids. The review called this non-blocking; taken anyway, because a gate that cannot tell the state it forbids from the state it requires is what this PR is about. It now slices each plugin's declaration and asserts per plugin, and is red on that pathological edit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014H4PsgR1bTfa8NuwxQ5D3o
…green-without-proofs
|
Thanks both - two findings acted on, and the substantive one was a good catch. The meta-annotation coverage gap - fixed
Correct, and it is this PR's own failure class one layer down in the gate that fixes it - a claim asserted by reasoning with nothing red to defend it. I did not use Proven red rather than assumed: swapping the resolution to The pom occurrence count - also fixed, despite being called non-blocking
Taking this one anyway, because a gate that cannot distinguish the state it forbids from the state it requires is precisely what this PR is about; leaving that shape in the test that guards the fix would be inconsistent. It now slices each plugin's own declaration and asserts containment per plugin. Proven red on the exact pathological case: two copies inside surefire and none in failsafe leaves the whole-file count at two, which the old assertion passed and the new one fails, naming Two things I am not changing
Also noted and not acted on: the quarantine lane reports |
✅ 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 |
[superseded - a quarantined test changed outcome] 🧪🔒 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 Since the previous push: Updated for Superseded by a newer quarantine lane report. |
[superseded - a quarantined test changed outcome] 🧪🔒 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 No quarantined test changed outcome since the previous push. Updated for Superseded by a newer quarantine lane report. |
bin/check-pr-analysis-surfaces.sh reported the previous commit's own fix as a new finding on a line it wrote: assembling "<artifactId>" + id + "</artifactId>" puts a variable inside markup, which SpotBugs reads as building XML from untrusted input. Not a real injection - the input is a literal from the line above - but a suppression is a claim the next reader has to take on trust, and the concatenation bought nothing. Both declarations are now written out whole, which also makes the match exact rather than merely likely. Still red on the case it exists for: two forwarding blocks inside surefire and none in failsafe leaves a whole-file count at two, and the assertion names maven-failsafe-plugin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014H4PsgR1bTfa8NuwxQ5D3o
…green-without-proofs
…green-without-proofs
…either goes #442's bin/lib/compiled-classes.mjs and this branch's TransactionalClaimCoverageTest.effectiveTagsOf both answer "which tags would JUnit apply to this test?", and both had to get the same three cases right - method, class, and a tag reached only through a meta-annotation such as @Quarantined. They arrived independently, days apart, and agree. Recorded rather than queued, because consolidating them is the wrong move: one reads javap output from Node before any JVM starts, the other resolves annotations inside a running test JVM through JUnit's own AnnotationSupport. Neither can call the other, and a shared derivation would be a third implementation rather than one fewer. What the entry asks for instead is that a change to either is applied to both or explained - the rule is JUnit's, so it moves only when JUnit's does, and a divergence is silent in both directions. Also repairs a citation this branch broke itself: the inflight note named the new check by the name it carried before review renamed it. Caught by the merge checklist's rename sweep rather than by anything that goes red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014H4PsgR1bTfa8NuwxQ5D3o
🧪🔒 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 Since the previous push: Updated for |
…this branch's sighting #433 and #443. One conflict, the RegistrationRaceStaleResidentIT ledger row, and both sides had reached "13 seen" while counting different sets - so either taken whole would have dropped a sighting under an authoritative-looking number, for the third time on this file. Master's side is the base and deserves to be: it carries the #442 and #438 sightings and, more importantly, a WITHIN-branch control this branch had no way to know about - #433 ran the same content twice, green then red after a re-cut that changed only the commit split, so one tree produced both outcomes and the tree itself is ruled out, leaving the runner. That is a stronger claim than the cross-branch controls it joins. Added to it: this branch's 2026-09-04 #444 sighting with its same-head re-run, and one clause noting that sighting is another of the same-day cross-branch kind. 14 seen. Recorded because the first attempt at this resolution asserted on an anchor that did not exist, wrote nothing, and the file was then staged WITH its conflict markers - caught by grepping the markers rather than trusting the exit code. The second attempt carries an anchor-free fallback that announces itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WErnxQd9Ew57F9SsqzdPU5
1 participant
Description
Top item of the review recorded in
docs/inflight/next-transactional-register-hardening.md, whichcame out of #262.
TransactionalClaimCoverageTestcertifies that every documented transactional guarantee has a testproving it. It reads compiled
@ProvesClaimannotations via ArchUnit, so it cannot distinguish aproof that ran and passed from one that was never selected - and
pom.xml's own help textdocumented an override that selected none of them.
Reproduce on the base commit, one term changed between the two arms:
The second arm never runs
ProducerManagerTestorTransactionalBulkCommitTestat all and cutsParallelConsumerOptionsTestdown to its untagged methods, while the register reports every claimcovered, every parked claim explained and every sentence intact. A fully green report over a run
that verified nothing. The register's selection criteria and the proofs' selection criteria were
disjoint, which is the whole bug.
The fix
A test JVM cannot see its own launcher's tag filters - surefire and failsafe pass
groups/excludedGroupsto the JUnit Platform, not to the tests.pom.xmlnow forwards the sametwo properties the filters are configured from, on both plugins, and
RunTagFilterreads them.everyCoveredClaimMustHaveAProofThisRunCanSelectthen fails, naming each claim and the proofs thisrun dropped.
Tags resolve through JUnit's own
AnnotationSupport, so meta-annotated and inherited cases come outas the launcher computes them -
@Quarantinedis a@Tag("quarantined")carrier, not a@Tag.Three decisions worth arguing with
Why a refusal rather than tagging the register. The recorded option was to give the register
@Tag("transactions")so that excluding the tag excluded the now-meaningless report. Rejected onthree grounds: it buys silence where the run needs an explanation; it is a proxy that had already
gone stale, because
ParallelEoSStreamProcessorTest's proof - added in#429 - carries no tag at all, so one tag cannot express the condition; and
it would take the two source-drift checks, which depend on no test running, down with it. Nothing in
the repo passes
-Dexcluded.groups=transactions- every real invocation uses the default list, anempty value, or
quarantined- so the loud failure breaks no workflow. That grep is what made thechoice safe, and it is the check to redo if you disagree.
A documented flag now fails the build. The pom's help text is updated to say so. This is the
behaviour change most worth reviewing.
An absent property is a defect, not "nothing excluded". Reading a dropped
systemPropertyVariablesblock as an empty filter would make the gate pass without having checkedanything - the same failure, one level up and about itself.
RunTagFilterraises instead, detectingthe case with a marker surefire sets rather than one this repo sets, since ours would vanish along
with the block whose loss it is meant to detect. Outside Maven - an IDE, and pitest's minion JVMs,
which run
bz.stub.parallelconsumer.*with neither the properties nor the marker - absent genuinelydoes mean unfiltered.
Judged per claim, not per method
A claim with two proofs, one tagged, is still proven in a run that drops the tagged one; failing
there would be the register reporting a gap it does not have.
OFFSET_AND_RECORDS_ATOMICis thelive case, and it correctly stays out of the failure list.
Defect-class sweep
QuarantinedAnnotationContractTest#bothSurefireAndFailsafeBindTheGroupPropertiesis the only othergate asserting the pom's tag wiring, and it exists because of an identical past P1 where only
failsafe was wired. It named a gap the dynamic guard cannot cover: forwarding added to one plugin
only, which no surefire run would notice.
RunTagFilterTestnow guards that too - verified bydeleting failsafe's block alone, where the register stayed green and only the new guard fired. No
other ArchUnit-based gate in the tree makes a coverage claim.
What review added
Two findings, both taken:
The register's own tag reading was asserted and never checked.
effectiveTagsOfclaims toresolve tags the way the launcher does - through meta-annotations and superclasses - and nothing
exercised either path, because nothing could: the tree's only meta-annotated carrier is
@Quarantined, and a claim proof carrying it is forbidden outright byclaimProofsMustLiveWhereATestRunnerWillFindThem. SwappingAnnotationSupportfor a hand-rolledisAnnotatedWith(Tag.class)would have passed the whole suite while blinding the new gate - thisPR's own failure class, one layer down in the gate that fixes it. Now covered by a fixture, red under
exactly that hand-rolled read.
The review suggested a
@Quarantinedfixture; that is not what landed.bin/check-quarantine-registry.shfails any class carrying the annotation without an entry indocs/quarantined-tests.md, so a fixture would read as an undocumented quarantine and could only beanswered by registering a test that is not quarantined - satisfying one gate by lying to another. A
local carrier proves the property that would actually regress; the reasoning is in the fixture's
javadoc.
A count of two is not two plugins.
bothSurefireAndFailsafeForwardTheFiltersToTheTestJvmcountedoccurrences across the whole pom, so two copies inside surefire and none in failsafe satisfied it -
the exact state its own message forbids. Marked non-blocking by the reviewer and taken anyway,
because a gate that cannot distinguish the state it forbids from the state it requires is what this
PR is about. Now sliced per plugin, red on that edit.
bin/check-pr-analysis-surfaces.shalso reported findings on lines this diff wrote - acontainsKeyfollowed by
get, two unpresized collections, and then a new one that the fix for the secondfinding above introduced by building markup around a variable. All are gone; the last only surfaced
because the script was re-run after fixing rather than trusting the aggregate
static: spotbugstick,which stayed green throughout.
Verification
Both arms of the reproduction re-run against the final code. Deleting the forwarding block verified
to go red rather than silently green, in both the gate and its own test. Full unit suite green on the
merged tree, and
bin/check-all.shgreen.Not verified locally: the integration lane, which needs Docker - CI covers it.
Note handling
docs/inflight/next-transactional-register-hardening.mdkeeps its remaining items and drops thisone, with the reasoning migrated to
docs/solutions/workflow-issues/. While there, its item on thetwo duplicate ITs carried an instruction to fix a gap "while #261 is still
open"; that PR merged, so the routing advice is corrected.
Checklist
docs/testing.mdgains a section on the register, plus adocs/solutions/write-updocs/features/- N/A - a test-infrastructure gate, no user-facing featureRunTagFilterTest, and the new gate inTransactionalClaimCoverageTestdocs/inflight/working note (pr-/branch-) started at the PR's first commit - N/A - the item this resolves is already tracked indocs/inflight/next-transactional-register-hardening.md, which this PR updates; a second note would duplicate itce-simplifyandce-code-reviewlocally - N/A - neither was run locally;@claude review thiswas requested on this PR instead and returned, and its findings are in "What review added" above.🤖 Generated with Claude Code
https://claude.ai/code/session_014H4PsgR1bTfa8NuwxQ5D3o