Repository navigation
test(arch): forbid one-character method names, close the ArchUnit module gap, and gate inflight notes that describe their own PR - #326
Merged
Conversation
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 |
✅ SpotBugs ReportNo bugs found (new bugs only — baseline from base branch excluded). |
🧪🔒 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 |
astubbs
marked this pull request as draft
August 22, 2026 22:54
Owner
Author
|
@codex review this |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e09620026f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
astubbs
marked this pull request as ready for review
August 24, 2026 01:42
Owner
Author
|
@claude review this Small PR, four independent commits. Where to look, in order:
Weight naming and representation at least as heavily as correctness. That is the whole motivation for commit 2: Verified locally before asking: all seven |
|
Claude finished @astubbs's task in 5m 2s —— View job Review complete
Confirmed HEAD is 1.
2.
3. The three new 4. Docs-only commits ( On the "weight naming/representation" ask: nothing in this diff has the Overall: LGTM, matching astubbs's own review. No blocking issues found; the two Codex P2s are legitimate but narrow-scope follow-up candidates, not defects in what's here. |
astubbs
changed the title
4 tasks done
astubbs
added a commit
that referenced
this pull request
Aug 24, 2026
…is correct #326 hit this and the workaround was reverted, so the finding needs a home before it is lost. The engine's only baseline is the base branch computed live, which means identical files are accepted or rejected purely by which side of a merge they sit on: four existing ArchUnit wrappers pass at ~84% because they are not an increase, while three identical new ones fail at 84.3%. Recorded as `bug` / `misdirection` rather than a task, because a check whose verdict depends on merge timing rather than on the code is an instrument that misreports - and this repo ranks signal integrity above severity precisely because you cannot judge code through instruments that lie. The note carries why each obvious workaround is wrong, since all four are tempting and three were seriously considered: excluding the example directories also hides the five app tests that are the likeliest real duplication; `only_code` raises the wrappers' similarity rather than lowering it; lowering the threshold blinds everything; and making the check advisory downgrades an engine with a demonstrated catch - it found the 83% drain-control-arm duplication on #325 that the density-based clone engines could not have. Names the better fix shape too: an accepted-pairs baseline rather than a path ignore, so a NEW pair between the same files still fails instead of the file being silenced forever.
astubbs
force-pushed
the
compound/naming-and-method
branch
2 times, most recently
from
August 24, 2026 03:03
f226e58 to
a328ff7
Compare
astubbs
added a commit
that referenced
this pull request
Aug 24, 2026
…and reported a pass Six-reviewer pass over the two new scripts. The headline is that the gate was green in CI for the wrong reason, and the fourth self-test bug of the same shape was hiding underneath it. **In CI, the PR-number arm never ran.** repo-hygiene.yml has no `env:` block, so `gh` on the runner is unauthenticated and exits 4; `2>/dev/null || true` swallows that, `$pr` stays empty, and the `[ -n "$pr" ]` guard skips the whole `#NNN` search. The gate then printed its success line and exited 0 having checked only the branch NAME. That disables its own motivating incident - a note citing #1 by number, with no branch name in it - and three of the four red self-test cases were exercising a path that does not run where the gate blocks merges. The event payload already carries the number, so the fix needs no token. The success line now names the arms that ran: "branch name only - NO PR NUMBER RESOLVED" is not the same statement as a clean full run and must not look like one. **Mention was not distinguished from use, a bug this repo has already paid for.** `.github/scripts/issue-ref-gate.js` records that a doc line quoting its block markers opened a real unclosed block, and that five files - including the convention's defining doc - made themselves permanently self-exempt. This gate copied that convention's spelling while its header advertised "one convention to remember rather than two", and copied the bug with it: prose containing `post-merge: exempt-file` silenced a whole note. All four marker tests (file, block, line, line-above) now read a code-span-stripped view, as the JS gate does. Branch names and PR numbers are still matched on the raw file - a mention inside backticks is a real mention; only markers are instructions. **The branch match was an unanchored substring search.** `grep -F "$branch"` fired on any line containing the name, so on `fix/909` a note mentioning `builds/fix/9090/output.log` failed the gate, and on `feat/857-fix` so did `feat/857-fix-followup`. Both reproduced. The convention here is `bugs/857-...`, `fix/909-...`, so short names nest inside longer ones by design and the false positive blocks an unrelated PR. The PR-number arm had boundaries for exactly this reason; the branch arm did not. **Two silent holes closed:** a PR cited by URL (`.../pull/326`) - what `gh` prints, what a paste carries, and already live in this directory - was invisible to a `#`-anchored regex; and `git ls-files` reads the index, so a note you had written but not staged was invisible to the local run this gate's header advertises. That is the commonest way to trip it. **A wrong marker is now an error, not a no-op.** The sibling gate spells all four of its markers `exempt`, so `post-merge: exempt` arrives here by muscle memory and matched nothing - a line whose author believed it was handled. **The fourth self-test bug, and the change that should stop the fifth.** Deleting the GITHUB_HEAD_REF preference from the gate passed 17/17, while on a detached HEAD - what actions/checkout produces on a pull_request event - the mutant exits 0 forever, checking nothing. The root cause of all four bugs is that a case asserted only an EXIT CODE: a gate crashing on every input scored four `ok`s. Red cases now require the gate to say what it caught, the CI branch-resolution path has its own cases, and the bar is recorded in the header as a MUTATION MATRIX rather than a green run - green proved nothing all four times. 31 cases, and the thirteen mutants they kill are named. Verified: each of the thirteen goes red. The comment claiming the glob is "non-recursive by design" was backwards - a git pathspec `*` matches `/` - and is corrected rather than deleted; the flatness rule it cited is real, it just was not load-bearing. The header's quotation from branch-package-rename-sweep.md no longer greps against that file, which has since been repaired, so it now points at the history holding it per docs/citations.md. Reviewed by six lenses: correctness, adversarial, project-standards, testing, maintainability, learnings. The adversarial lens ran IN-PROCESS, not cross-model - no different-provider CLI on this host - so its agreement with correctness on the CI finding is not independent corroboration. Both findings were reproduced by hand before being applied. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019infZnCpPvX8c6QWFC1uLC
astubbs
force-pushed
the
compound/naming-and-method
branch
from
August 24, 2026 03:48
2257f12 to
95960ac
Compare
astubbs
changed the title
…ng out of ArchUnit silently Two gaps, both invisible in the same way: nothing went red, so nothing looked wrong. **One-character method names.** `d(...)` in `KeyOrderLedgerIT` was the only single-letter method in the codebase - the identifier every fixture in that file was built from, and its least informative name. It survived three Claude review rounds and a Fable simplify-and-review pass whose maintainability reviewer returned findings on that exact file; a human reading the code found it. Naming is the class of defect automated review is weakest at, and the class a trivial mechanical rule catches perfectly. The rule lives in `TestConventionRules`, so it applies to every module at once, and it is proven red against a deliberately broken control that it names by file and line. ArchUnit rather than a `bin/check-*.sh`: it already runs in every module's normal test suite, so there is no new script and no new CI job, and the failure reaches the developer who wrote it. SpotBugs was the other candidate and is the wrong tool - it detects bugs, not naming. **Three modules had no ArchUnit at all.** `example-core`, `example-vertx` and `example-reactor` all have test sources and no `TestConventionsArchTest`, so every rule in the shared library silently did not run there. A module that opts out is not reported as unprotected; it is reported as nothing, which reads exactly like passing. Files added, and `EveryModuleWiresUpArchUnitTest` now fails the build when a module with test sources lacks one - covering modules that do not exist yet, the half an audit cannot reach. That sentinel accepts a module by its WIRING, not its filename: a placeholder or annotation-stripped wrapper would otherwise satisfy it while ArchUnit discovered nothing, preserving the exact silent opt-out it exists to prevent. Found by Codex review, and it is an instance of the rule this same PR adds to `docs/agent-harness.md` - a check can be right about WHAT to look for and wrong about its SCOPE, with its red control passing throughout. It locates the reactor root by build content - the aggregator pom's `<module>` entries - rather than by `.git`, which aborts the test outright in a source archive or exported tree. That also keeps the property that a git WORKTREE stores `.git` as a FILE, so an `isDirectory` probe would walk past the worktree root and silently audit the main checkout instead. Inheriting the wrapper from the root pom was the obvious alternative and is worse: surefire's `dependenciesToScan` pulls the WHOLE test-jar, so core's 54 test classes would run in all five modules, and scoping that back needs global `<includes>` that constrain normal test selection everywhere. The reasoning is recorded on the test so it is not re-derived.
…is disputed Four reviewers deadlocked on one test - two wanting it deleted, two wanting it hardened, the fixes mutually exclusive - and nobody acted for hours. Three mutants over the guards it claimed to protect settled it in a few builds: it killed none of them, including the scenario its own javadoc named. `docs/testing.md` had nothing on the technique, so the next deadlock gets argued from opinion again. Written as OPTIONAL and says so in its first line. Made routine it would be a tax on every test review; the trigger is a dispute, or a slow test defended on the grounds that it "covers" something. Scoped honestly, after review caught the write-up overclaiming: a finite hand-picked mutant set shows a test is redundant FOR THE GUARDS YOU MUTATED, which is not the same as covering nothing - it may uniquely assert an output, an ordering property, or a race none of the mutations disturbs. The table is evidence for a deletion argument, never the whole of one. Carries the two things that decide whether the result can be trusted: fix the test's own bugs before measuring, or you measure a broken test - that one bypassed in its own setup the guard it was verifying - and watch for equivalent mutants, since a mutant changing no observable behaviour is evidence of nothing. Also names the shape to suspect first: a `@RepeatedTest` in a gating lane, which is what you write when you cannot force a race and are hoping to draw it.
…ooking at the right thing
Every gate that misbehaved across this work failed on SCOPE, never on logic. `pre-commit-gate.sh`
fired on read-only commands that were not commits. `check-pr-ready.sh` read only the first matching
`pr-<n>-*.md` when two existed. The `inflight-state` regex was greedy, so the gate and the session
index disagreed about the same note. `chaos-test.sh`'s EXIT trap swallowed the exit code and would
have rendered a real chaos RED as green.
Not one was wrong about WHAT to check, and every one passed its negative control - because a red
control only demonstrates the gate can fire.
So rule 3 now asks for a second control: the near-miss that must stay GREEN, the thing one character
away from what you are catching. `bin/test-check-agent-hooks.sh` already carries these where they were
learned the hard way ("a commit MESSAGE mentioning push does not fire"), and they exist because the
first drafts matched substrings and blocked them. The rule asks for it while the gate is new, rather
than after it has been routed around.
This PR then proved the rule against itself: the module-coverage sentinel it adds accepted a wrapper
by filename, which is a check right about what and wrong about scope, and its red control passed the
whole time. A reviewer caught it two commits later.
…dings this work turned up **The thirteenth sighting, with its seed.** #325 merged one minute after its last check landed, while the chaos evidence was still being extracted, so the run went unrecorded - and CI artifacts expire. By that ledger's own argument the seed is the asset: a sighting without one is an anecdote. It earns its place beyond the tally, because this was the FIRST full chaos storm run against the ordering ledger and stall probe #325 added and **none of them fired** - zero `INSTANCE_STALL`, zero `LEDGER_KEY_ORDER`, zero `LEDGER_KEY_CONCURRENCY`, across 21 violations that were all the pre-existing `CLASS2_STALL` timing bound. A new detector's likeliest failure is crying wolf; this is the first evidence these two do not. Recorded as UNRESOLVED, not contention - the discriminator is an uncontended replay of that seed and nobody has run one. **A register of the other compounding candidates**, each with the incident behind it, including what was checked and found ALREADY covered - that half is what stops a proposal arriving a third time. It also records two patterns no single incident makes visible: enforcement here is concentrated at commit and merge time while the failures concentrated in claims made in chat, and a rule that lands only in an unmerged PR does not bind with nothing able to tell you. **The similarity engine cannot accept duplication that is correct.** Its only baseline is the base branch computed live, so identical files are accepted or rejected by which side of a merge they sit on: four existing ArchUnit wrappers pass at ~84% because they are not an increase, while the three identical ones this PR adds fail at 84.3%. Filed as `bug`/`misdirection` rather than a task, because a check whose verdict depends on merge timing rather than on the code is an instrument that misreports. The note records why all four obvious workarounds are wrong - including that making the check advisory would downgrade an engine with a demonstrated catch - and names the better fix: an accepted-pairs baseline rather than a path ignore, so a NEW pair between the same files still fails.
…ctly AFTER the merge A docs/inflight/ note is a claim about NOW - the directory's contract is transient cross-branch state, and a note is deleted when its work lands. So a sentence in one describing your own open PR is a claim your merge falsifies: accurate when written, accurate at review, wrong seconds after the merge button - by which time the person who could cheaply fix it has moved on, and the fix is too small to justify its own PR, so it rots. It happened in exactly this directory. branch-package-rename-sweep.md, a live 38-branch worklist, told readers #1 was on the too-old list "until its branch was reset onto renamed master and repurposed as documentation" - written while #1 was open. It merged, its branch was deleted, and a live worklist was left asserting an arrangement that no longer held. The PR did not stop existing - PRs are permanent and citing one is fine forever. What rotted is the BRANCH (deleted) and the TENSE. That is why the gate looks nowhere else: in docs/solutions/, dated docs/plans/ or CHANGELOG.adoc a branch or PR reference records what already happened and stays correct, so demanding a marker there would be a tax on writing history correctly. The rule is not "do not mention your own PR" - cross-referencing is often exactly right - but "write the mention as it will read AFTER this lands", confirmed by a human with `post-merge: checked` on or above the line, `checked-begin`/`checked-end` around a paragraph, or `post-merge: exempt-file` for a note that is legitimately about in-flight state. It cannot tell whether a rewritten sentence is true after the merge; no grep can. It forces someone to look at every self-reference at the one moment fixing it is cheap. WHAT IT LOOKS AT, and the four ways a gate like this checks less than it claims: - **Both halves run in CI.** repo-hygiene.yml passes PR_NUMBER from the event payload. Without it `gh` on a runner is unauthenticated and exits 4, the script's fallback swallows that, and the whole `#NNN` arm is skipped while the gate still exits 0 - which would disable its own motivating incident, a note citing #1 by NUMBER with no branch name in it. The success line names which arms ran, so a branch-only run cannot be mistaken for a clean full one. - **Mention is not use.** All four marker tests read a code-span-stripped view, as .github/scripts/issue-ref-gate.js does. That gate records the incident this one would otherwise have repeated: a doc line quoting the block markers opened a real unclosed block, and five files - including the convention's defining doc - made themselves permanently self-exempt. Branch names and PR numbers are matched on the raw file: a mention inside backticks is a real mention, only markers are instructions. An unrecognised `post-merge: exempt...` is a hard error rather than a no-op, because the sibling gate spells all four of ITS markers `exempt` and the habit transfers. - **Matching is bounded, and covers what a reference really looks like.** Branch names nest by convention here (`bugs/857-...`, `fix/909-...`), so an unanchored match fires on `builds/fix/9090/output.log` and blocks an unrelated PR. A PR cited by URL counts as much as `#NNN` - it is what `gh` prints - and untracked files are searched, because the note you just wrote about your own branch is the commonest way to trip this and `git ls-files` alone cannot see it. - **The self-test's bar is a mutation matrix, not a green run.** This suite was green while blind four times, each time because a case asserted only an EXIT CODE: a gate crashing on every input scored four `ok`s. Red cases now require the gate to say what it caught, the detached-HEAD branch resolution CI actually uses has its own cases, and the header names the thirteen mutants the 31 cases kill. A new case earns its place by killing one none of the others do. Docs are deliberately untouched: docs/agent-harness.md's rule 3 (a red control proves a gate can fire, never that it is looking at the right thing) already owns the principle, and this is its worked example. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019infZnCpPvX8c6QWFC1uLC
astubbs
force-pushed
the
compound/naming-and-method
branch
from
August 24, 2026 04:01
95960ac to
9d5179f
Compare
This was referenced Aug 6, 2026
6 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Things this repo learned the hard way, turned into mechanism — plus the review and dependency work that landing it turned up. Each commit is independent and separately revertable; merge strategy is rebase-merge.
The mechanism
1. One-character method names are now an ArchUnit rule, and no module can silently opt out.
d(...)inKeyOrderLedgerITwas the only single-letter method in the codebase — the identifier every fixture in that file was built from. It survived three Claude review rounds and a Fable simplify-and-review pass whose maintainability reviewer returned findings on that exact file. A human reading the code found it.Wiring it up exposed a second gap: three of the five example modules had no ArchUnit at all.
example-core,example-vertxandexample-reactorall have test sources and noTestConventionsArchTest, so every shared rule silently did not run there. A module that opts out isn't reported as unprotected — it's reported as nothing, which reads exactly like passing. Files added, plusEveryModuleWiresUpArchUnitTest, which fails the build when a module with test sources lacks one — covering modules that don't exist yet.Root-pom inheritance was the obvious alternative and is worse: surefire's
dependenciesToScanpulls the whole test-jar, so core's 54 test classes would run in all five modules. That reasoning is recorded on the test.2. A red control proves a gate can fire, not that it is looking at the right thing. Every gate that misbehaved during this work failed on scope, never on logic — a pre-commit gate firing on non-commit commands, a checker reading only the first of two matching notes, a greedy regex, an EXIT trap swallowing a real failure. Not one was wrong about what to check, and every one passed its negative control.
docs/agent-harness.mdrule 3 now also asks for the near-miss that must stay green.3. How to settle whether a test earns its place — optional, for when it's disputed. Four reviewers deadlocked on one test, two wanting it deleted and two wanting it hardened, and nobody acted. Three mutants over the guards it claimed to protect settled it.
docs/testing.mdhad nothing on the technique.4. The thirteenth confluentinc#857 sighting, with its seed. Recorded because CI artifacts expire and, by that ledger's own argument, the seed is the asset. It earns its place beyond the tally: this was the first full chaos storm run against the ordering ledger and stall probe added in #325, and none of them fired — zero
INSTANCE_STALL, zeroLEDGER_KEY_ORDER, zeroLEDGER_KEY_CONCURRENCY. A new detector's likeliest failure is crying wolf; this is the first evidence these two do not. Marked unresolved, not contention — nobody has run the discriminating replay.What review and CI added
5. Three Codex findings, all P2, all valid — and the first is an instance of the rule commit 2 adds.
EveryModuleWiresUpArchUnitTestaccepted a wrapper by filename, so a placeholder or annotation-stripped file satisfied it while ArchUnit discovered nothing there — preserving the exact silent opt-out the test exists to prevent. It now requires@AnalyzeClassesandArchTests.in(TestConventionRules.class). A check right about what and wrong about scope, whose red control passed throughout, written two commits before a reviewer caught it.Also: root detection required
.git, which aborts the test in a source archive — it now locates the reactor by the aggregator pom's<module>entries, which also keeps the property that a git worktree stores.gitas a file. And the mutation-matrix write-up overclaimed: a hand-picked mutant set shows redundancy for those guards, not that a test covers nothing.6.
reactor-core3.8.6 -> 3.8.7 landed separately and is no longer part of this PR. It was cherry-picked to master while this was open (0e21b903d), so the rebase onto master dropped this branch's identical commit as already-upstream. Recorded here because the description claimed it as a commit of this PR; it is now inherited, not contributed.7. A gate for inflight notes that describe their own open PR.
bin/check-branch-self-reference.shfails when adocs/inflight/note mentions the current branch or PR without being written as it will read after the merge, markedpost-merge: checked. The rule is not "do not cite your own PR" - it is that the tense has to survive the merge button. It happened here:branch-package-rename-sweep.mddescribed #1's branch as currently repurposed, then #1 merged and the branch was deleted, leaving a live worklist asserting an arrangement that had ended. Scoped todocs/inflight/only, because everywhere else a branch reference records what already happened and stays correct.A six-lens review of it found the gate was passing in CI having checked half of what it claimed: no
PR_NUMBERin the workflow, soghon the runner was unauthenticated, the failure was swallowed, and the whole#NNNarm was skipped - disabling the gate's own motivating incident, which is a note citing a PR by number. Also fixed: prose quoting a marker invoked it (the bug.github/scripts/issue-ref-gate.jsalready records for its own markers), an unanchored branch match that fired onfix/9090when the branch wasfix/909, a PR cited by URL being invisible, and unstaged notes being invisible to the local run. The self-test's bar is now a mutation matrix rather than a green run - it was green while blind four times - with 31 cases and the thirteen mutants they kill named in the header.8.
dups: similarityfails, and it is the only red check. The three new ArchUnit wrappers flag at 84.3% against core's. Excluding the example modules was tried here and reverted: each wrapper shares a directory with that module's real app test, and the five example apps solve one problem five ways, so examples are the most likely place for genuine duplication. The engine's only baseline is the base branch computed live, so the four wrappers already on master pass at ~84% while three identical new ones fail - same code, opposite verdicts, decided by merge timing. Recorded as abug/misdirectionnote with all four workarounds and why each is wrong; the named fix is an accepted-pairs baseline in the tool, which is this project's own fork. This needs a merge decision, not a code change.Checklist
docs/testing.md,docs/agent-harness.md,docs/inflight/bug-857-family.md, plus a candidates registerdocs/features/—N/A - no user-facing behaviour; test-only rules, docs, a ledger entry, and a test-scope dependency bumpVerified locally: all
bin/check-*.shgates, the new gate's 31-case self-test and its 13-mutant matrix (all killed),test-check-agent-hooks.sh,test-check-inflight-tags.sh, the ArchUnit rules in all nine modules, negative controls for both new rules including an unwired-wrapper control, and the OSS Index audit against the bumped reactor tree.