Repository navigation
ci(citations): read citations inside .java files, not only prose formats - #374
Conversation
Javadoc and comments cite `docs/...` paths as prose exactly the way markdown does, and nothing checked them: `CITING_FILE` listed `.md`/`.adoc`/`.txt`/`.html`, so the gate could not see a .java file at all. #342 carries the live example - a javadoc citing `docs/inflight/perf-throughput-regression-since-0-3.md`, a note that exists only on another branch, which no gate could have flagged. WHY THE OLD EXCLUSION DOES NOT SURVIVE READING. The comment said "a .java file's imports are the compiler's problem, not this gate's". TOKEN requires at least two `/`-separated segments ending in a real extension; a Java import is a DOTTED package name - `bz.stub.parallelconsumer.state.ShardKey` - with no `/` in it, so it can never match. Verified before relying on it, and pinned by two tests (single imports, and a whole file of them) because it is the entire reason this is safe to turn on. The exclusion bought nothing and cost the javadoc. `.html` IS THE PRECEDENT: same failure, same fix. It was excluded, a rename stranded a path inside one, and the gate was blind because of the extension rather than the content (#323 review). WHAT THE WHOLE-TREE RUN SURFACED. Three findings across 283 java files, all false positives, none a broken citation: - `docs/x.md`, twice, inside QuarantineRegistryScriptTest's embedded fixture source. Fixed by TIGHTENING THE MATCHER rather than exempting: a one-character LOWERCASE filename is an illustrative name, the way this module's own header writes "a pointer for docs/a.md says nothing about docs/b.md". Lowercase is load-bearing - a java file is named for its class, so `.../internal/A.java` is a plausible real path while `docs/a.md` is not a plausible real document. Anchored at the filename, so a short directory segment is untouched. No tracked file in the tree has a single-character stem. - `module/src/test-integration/java/SomeQuarantinedIT.java` in CheckQuarantineOwnersScriptTest - a path inside the temporary fixture repo the test builds, not a path in this one. Nothing textually separates it from the REAL repo paths in the same file (`bin/check-quarantine-owners.sh`), so exempting the file would have thrown those away. It gets the gate's own documented paragraph marker with a reason, which is line-scoped and costs nothing else. Code paths are checked too, deliberately: `@Quarantined(tracking = "docs/...")` and `REPO_ROOT.resolve("bin/...")` are repo citations that go silently stale on a rename, and they are the majority of what a java file cites. RED CONTROL. A gate that passes everything is indistinguishable from one that is not looking, so the no was constructed rather than assumed: a .java file citing `docs/inflight/perf-throughput-regression-since-0-3.md` was added to the tree and `bin/check-file-refs.sh` exited 1 naming the file, the line and the path; removing it returned the run to clean. The unit suite asserts the same shape directly. `.sh` is the one remaining gap and is not closed here - it wants a pass over the shell corpus, not a third one-off extension. docs/inflight/ci-copyright-gate-review-leftovers.md now records shell as the open half of the class rather than claiming java is still excluded.
|
You have reached your Codex usage limits. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
@claude review this |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Claude finished @astubbs's task in 4m 22s —— View job Review complete ✅
Note on verification: this environment's tool allowlist is master's copy of the workflow, so FindingsThe central safety claim holds up. The new So the added
Minor, non-blocking observation: the first The Overall this is a well-verified, narrowly-scoped change: the risky part (would enabling |
✅ 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 |
|
Review of #374 caught a second `file-refs: N/A` marker in CheckQuarantineOwnersScriptTest that excuses no finding: in the setup fixture the path is built from two separate `resolve()` calls - `"module/src/test-integration/java"` has no file extension and `"SomeQuarantinedIT.java"` has one segment - so neither is a citation and the gate never fired there. Only the single-line `fixture.resolve("module/src/test-integration/java/SomeQuarantinedIT.java")` in `qualifiedFixedByDisagreeingWithRegistryOwnerIsFlagged` needs one, and keeps it. An unused escape is worse than none: it reads as a live exemption, so the next reader trusts a silence that was never there, and it is the first step in the drift that makes a gate's exemption list meaningless. Verified by removing it - `bin/check-file-refs.sh` stays clean.
|
Addressed the review's minor observation in cdfb054: the first Only the single-line |
…tive flag The `.java` change in this branch rewrote PLACEHOLDER from a `/.../i` literal into `new RegExp(a + b)` to add the one-character-lowercase-filename rule. That call takes no flags argument, so the word list silently stopped being case-insensitive: `bin/check-FOO.sh` and `docs/Bar.md` in a worked example became real citations and would be checked against the tree. Nothing went red, because no case in the 56-check suite used a capitalised placeholder word. It is a NARROWING of the exemption, so the gate still says no to a dangling citation - but it is a new false-positive class nobody asked for, introduced purely by the refactor. Two named regexes rather than one alternation, because their flags genuinely differ and JavaScript has no inline flag syntax: the word list must be case-insensitive, and the one-character rule must NOT be - `.../internal/A.java` is a plausible real path while `docs/a.md` is not a plausible real document. A concatenated regex cannot express that, and makes the loss invisible. Two assertions pin the case-insensitivity, and they are load-bearing: removing the `/i` again fails the suite. Also folded in why a single DIGIT stem is on the same rule (no java class starts with one), and hoisted the history-pointer regex out of its per-line loop - it was recompiled once per line, which cost nothing while only prose was scanned and is a per-line allocation across the whole java tree now that `.java` is a citing file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… once This branch's `Chaos Pain Suite` went red on ChaosRevokeUnderWorkIT.revokeUnderWorkStaysProtocolHonest with the family's BLOCKED-on-monitor discriminator: the poll thread stopped on the `synchronized (commitCommand)` acquisition inside commitOffsetsThatAreReady, reached from onPartitionsRevoked, with the monitor held by that instance's own control thread. Frame for frame it is the two captures already recorded on 2026-08-26, on a branch carrying no main Java at all. The new fact is not a third occurrence but a simultaneous one. A second PR's chaos job tripped the same discriminator on a different scenario arm and a different seed within two minutes. Both prior entries argued the box was not a suspect because sibling chaos jobs were green; that argument is no longer needed either way, because the lane moved off the shared highcpu box to ubuntu-latest earlier the same day, so each chaos job now has its own VM and co-residency between the two runs is structurally impossible. Contention on one loaded machine cannot have produced both. Recorded with both seeds, and with the corroborating capture explicitly marked weaker: that branch edits AbstractParallelEoSStreamProcessor, so its shifted line numbers make it corroboration of the monitor pair rather than a second not-PR-introduced control arm. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts: # docs/inflight/bug-857-family.md
…tten here Ten commits, including the #322 module-ownership split and #374's citation gate. Only `bug-857-family.md` conflicted; `PCModule` and `WorkManager` merged clean and their suites pass. THE CONFLICT IS THE INTERESTING PART, AND MASTER WINS IT. Both sides added a "fourth capture" of the commit-deadlock discriminator, hours apart, neither knowing about the other. Master's is better and is taken wholesale: it pairs a genuinely clean capture from #374 - a branch carrying only a gate script, its test and markdown, so it cannot reach the chaos engine - with the one recorded here, and correctly downgrades this one to corroborating. The reason is one this branch had missed while arguing the opposite: because it edits `AbstractParallelEoSStreamProcessor`, its frames read `:1621` and `:555` rather than `:1585` and `:548`, so it cannot serve as a not-PR-introduced control arm even though it changes no `synchronized` block. Master also carries an argument this branch did not have - the chaos suite moved to a per-job `ubuntu-latest` VM the same day, so co-residency between the two red runs is structurally impossible, which retires the ambient explanation far more firmly than the same-run sibling greens that were argued here. Everything of substance from the entry written here survives in master's: run, seed, instance, holder, and why the frames differ. One clause is added, for the fact master's text does not carry - the corroborating capture's ARM. It is the drain stop-mode under the eager assignor, which no capture above covers, so between them the cycle has now been seen under both assignors and both stop-modes. That claim does not depend on the capture being a clean control, which is why it is worth keeping when the rest of the entry is not. Also merged, and worth knowing here because it endorses a change made on this branch earlier today: #322's `docs/inflight/AGENTS.md` rewrite now names four outcomes for a note whose work lands, with deletion LAST and "migrate what outlives the work to its durable owner" first. That is what the handoff extraction did - the two documents it created are its first customers under a rule that did not exist when they were written. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
… as corroboration not a control `ChaosRevokeUnderWorkDrainIT` went red on this branch's CI with the BLOCKED-on-monitor discriminator: the poll thread parked acquiring the `commitCommand` monitor, held by that instance's own control thread, reached through `commitOffsetsThatAreReady` from `onPartitionsRevoked`. Same monitor, same holder, same method pair as the five captures above it. FILED AS WEAKER EVIDENCE ON PURPOSE. The frames read `:1639` / `:573` rather than master's `:1585` / `:548`, and this time the shift is the branch's own - this work edits `AbstractParallelEoSStreamProcessor` to wrap the pool submit in an MDC scope, which moves everything below it. Resolved against this branch's copy the pair is identical, matched by method and monitor the way the fifth capture instructs rather than by line number. But a branch that edits the file cannot rule itself out the way #346 and #374 could, so this adds another arm-and-holder match and no independent evidence about cause. The fourth capture already draws that distinction for #267 and this follows it. IT IS NOT THE FLAKE, AND THE ENTRY SAYS SO. The same branch produced a `ZOMBIE_MEMBER` capture on an earlier head, recorded below as a sighting with its own standing seed-dependent explanation. Different probe, different failure mode. Reading any red `Chaos Pain Suite` on this branch as "the flake" would merge the two, and only the monitor-and-holder match separates them - the discriminator is the live defect. Seed `818084281700661522`, recorded with its replay command. RETRIEVAL DECIDED WHAT THIS ENTRY COULD SAY, so it is recorded too. The job log is large, and `gh-run-view-log-truncation.md` documents three occasions where a truncated fetch produced a published-and-wrong diagnosis - one of them to a reader who had that document open. The autopsy here came from the uploaded `chaos-suite-reports-*` artifact, route 1 in that file, which survives server-side stream truncation because the autopsy is `system-out` inside the failsafe XML. Seed and both frames came out intact. Also repaired: the `ZOMBIE_MEMBER` sighting below lost its opening `post-merge: checked-begin` when this file was rebuilt from master's copy during an earlier merge, leaving an unmatched `checked-end` and three self-references the gate could no longer see as attested. Begin and end markers now balance.
Description
.github/scripts/file-ref-gate.jscould not see a.javafile at all -CITING_FILElisted.md/.adoc/.txt/.html. Javadoc and comments citedocs/...paths as prose exactly the waymarkdown does, and nothing checked any of them. #342 flagged the gap and
carries the live example: a javadoc citing
docs/inflight/perf-throughput-regression-since-0-3.md,a note that exists only on another branch.
.htmlis the precedent - same failure, same fix. It was excluded too, a rename stranded a pathinside one, and the gate was blind because of the extension rather than the content
(#323 review).
The argument for the exclusion does not survive reading the code
The comment said "a
.javafile's imports are the compiler's problem, not this gate's".TOKENrequires at least two
/-separated segments ending in a real extension; a Java import is a dottedpackage name -
bz.stub.parallelconsumer.state.ShardKey- with no/in it, so it can nevermatch. Verified before relying on it, and now pinned by two tests (single imports, and a whole file
of them), because it is the entire reason this is safe to turn on.
What the whole-tree run surfaced
Three findings across 283
.javafiles. All three are false positives - no genuinely brokencitation exists in a
.javafile today, so the change is pure upside plus the narrowing below.docs/x.mdx2 inQuarantineRegistryScriptTest, inside embedded fixture sourcedocs/a.mdsays nothing aboutdocs/b.md". Lowercase is load-bearing: a java file is named for its class, so.../internal/A.javais a plausible real path whiledocs/a.mdis not a plausible real document. Anchored at the filename, so a short directory segment is untouched. No tracked file in the tree has a single-character stem.module/src/test-integration/java/SomeQuarantinedIT.javainCheckQuarantineOwnersScriptTestbin/check-quarantine-owners.sh,bin/check-quarantine-registry.sh), so anEXEMPT_PATHSentry would have thrown those away - which is exactly how a gate rots.EXEMPT_PATHSis unchanged.Code paths are checked too, deliberately
@Quarantined(tracking = "docs/...")andREPO_ROOT.resolve("bin/...")are repo citations that gosilently stale on a rename, and they are the majority of what a
.javafile cites. Restricting thescan to comment lines would have been simpler and would have thrown all of that away.
Red control
A gate that passes everything is indistinguishable from one that is not looking, so the no was
constructed rather than assumed: a
.javafile citingdocs/inflight/perf-throughput-regression-since-0-3.mdwas added to the tree andbin/check-file-refs.shexited 1, naming the file, the line and the path; removing it returned therun to clean. The unit suite asserts the same shape directly.
Not closed here
.shis the one remaining extension where prose cites repo paths.docs/inflight/ci-copyright-gate-review-leftovers.mdnow records shell as the open half of theclass rather than claiming java is still excluded. It wants a pass over the shell corpus, not a
third one-off extension.
Verification
node .github/scripts/file-ref-gate.test.js- 56 assertions pass (was 45)bin/test-check-file-refs.sh- all cases passbin/check-file-refs.sh- cleanbin/check-all.sh- 15 ran, 15 passed, 0 failedChecklist
docs/inflight/ci-copyright-gate-review-leftovers.md; the rule itself lives in the module's own header, which is updated.github/scripts/file-ref-gate.test.jsgains a java section: imports do not trip it, a resolving javadoc citation passes, a dangling one fails, code paths are checked, the line opt-out works in.java, and the one-character-filename narrowing is pinned in both directions@codex reviewand@claude review thison the PR instead