Repository navigation
docs(solutions): name the check-that-never-ran class, and two siblings - #282
Merged
Merged
Conversation
… running
Seven checks here have reported success in a state where they could not
run: the OSS Index audit 401ing on every module behind `fail=false`, the
same plugin still green with `fail=true`, the review action skipping and
exiting zero, the review action posting nothing for months, the copyright
scanner soft-exiting on a shallow clone, Maven degrading a version-less
plugin to a warning, and GitHub's managed dependency submission failing on
every run including master.
`docs/investigating.md` already carries the operative rule - a guard nobody
has seen fail is decoration - and `docs/ci.md` and `docs/issue-references.md`
each state a version of it for their own gate. This write-up names the class
those three are instances of, so the next one is recognised rather than
rediscovered.
The actionable half is guard design. A log-string guard rots silently the
day the vendor rewords the message, which is the same class of failure it
exists to catch, so it may be a leg but never the only leg. The shipped
audit guard pairs it with a structural leg: on a 401 the plugin still writes
its report file, and writes `{ }`, so a non-empty `reports` map is proof the
scan happened and file existence proves nothing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
…y yes A positive result gets scrutinised; a negative one gets acted on, because it is a stopping condition. Three negatives here were artefacts of the instrument rather than facts: a warm OSS Index cache answering a reachability experiment against a bogus URL and an expired token, a two-dot `git diff` rendering master's newer commits as the branch's own deletions, and `gh` answering the prior-art search from upstream. The two-dot/three-dot trap had no home anywhere in the tree. Two dots compare endpoints, three compare against the merge base, and neither is the right default - `docs/solutions/.../read-the-commits-you-inherit` wants two dots for exactly the reason a branch diff wants three. The trap is reaching for a diff without deciding which question is being asked. `docs/investigating.md` already requires instrumentation be shown to have reached the run; this generalises that to every negative, and gives the six prior-art checks in AGENTS.md a positive control each. The `gh` case is cited rather than restated - it is already written up in full. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
Four failures that looked independent are one misunderstanding: in a multi-module reactor, something being declared is not the same as it being resolvable at the moment something asks for it. A plugin version declared inside a profile is not resolvable to a child module that consumes it version-less, and Maven degrades the malformed model to a warning. A module declared in the reactor is not resolvable as a jar during `validate`, because `validate` builds nothing - which is why the audit mojo needs `test-compile`, and why GitHub's managed dependency submission cannot work here at all: its probe is a bare `mvn validate` at the repo root and is not editable in `.github/workflows`. An artifact published to a repository is not resolvable unless that repository is also declared for reading - our snapshots return HTTP 200 and the root pom never declared where to find them. Also records the reasoning for the fix that was NOT applied: declaring the snapshot repository would satisfy one auxiliary probe at the cost of letting `-pl` without `-am` silently resolve a stale published snapshot instead of failing loudly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Claude finished @astubbs's task in 3m 5s —— View job Reviewing PR #282
|
✅ 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). |
![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: f57a3a8028
ℹ️ 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".
Codex review found seven claims that overreached their own evidence. The silent-green doc counted seven checks as having "reported success" when its own table classifies the seventh as fail-closed-and-ignored, so the opening now splits six-that-passed from one-that-failed-unread. It called both guards "shipped" while saying two paragraphs later that one is unmerged. It advertised three fail-closed legs without carrying the warm-cache precondition up from section 5, where a cached report lets leg 2 pass on a run that never contacted the service. And its sigpipe one-liner said the pipeline inverts "exactly when it matches", which a small reproducer refutes: bin/check-shell-sigpipe.sh documents that the producer needs more than a pipe buffer still to write. docs/inflight/ci-ossindex-audit-dead.md still told implementers that failing on the "Failed to fetch component-reports" log line "is enough" - the exact instruction the new guidance rejects. It now names that as one leg and cross-references the structural one. The Maven doc said test-compile leaves the reactor with jars built. It does not; both jar goals run in package. What it does is compile the earlier modules so the reactor answers a sibling from its output directory. The negative-results doc listed a git-diff false positive among its negatives. It is the mirror image of the class rather than an instance, and is now labelled as such and kept, because the defect and the cure are the same in both directions. Its two gh positive controls also now pass -R, and the upstream-mirror probe is limit-proof rather than trusting a default page of 30 - a pagination artefact would have been this document committing its own error. Not changed: "including on the default branch" in the frontmatter, which the review asked to withdraw as unsupported. The Automatic Dependency Submission workflow's run history shows every run a failure, master included, from 2026-04-21 through 2026-08-11. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
|
Claude finished @astubbs's task in 4m 16s —— View job Reviewing PR #282
SummaryVerified the doc content against the actual repo rather than just scanning prose: every relative markdown link across the three new docs resolves correctly, the Maven One real, well-evidenced factual bug was found and posted inline:
No other correctness, consistency, or convention issues survived verification. (Note: this pass focused on factual accuracy of the new docs; it did not re-litigate the Codex bot's earlier stylistic/precision comments on this PR.) |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review disputed the claim that the managed dependency-submission workflow failed on the default branch, and an earlier validator pass had withdrawn it for the same reason: nobody had gone and looked at the run history. Looking settles it. Every run of workflow 261174653 is a failure, with 20+ on master between 2026-04-21 and 2026-08-11. Both documents now say "every run it ever made, master included" rather than naming a sample of three branches, and the Maven one carries the gh api query that answers the question, since this is the second time it has been asked and the first two answers were both derived rather than measured. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
Reviewing PR #282
|
Review caught two references calling #281 open. It merged as f89f27f at 02:46Z, twelve minutes before the previous commit on this branch landed, so the reference was stale when it was written rather than drifting later. Swept the same class across every file this PR touches. One more instance: the ossindex note still described the CVE triage write-up as in flight on an unpushed branch, when it is on master as docs/inflight/deps-cve-backlog.md. Everything else checks out - #259 and #278 are marked merged and are, #279 is marked open and is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
#281 touched the same paragraph of docs/inflight/ci-ossindex-audit-dead.md and made the same correction a better way: instead of relabelling the triage write-up as merged, it describes what the write-up contains and drops the branch-state sentence altogether, so there is no state left to go stale. Taking that side whole. The log-string-guard bullet earlier in the file is a separate hunk and merged clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
|
Claude finished @astubbs's task in 2m 40s —— View job Reviewing PR #282
SummaryThis is a re-review after two prior rounds (Codex's 9 comments + my own 1 finding) were all addressed by the author in
No new findings survive this pass — nothing to post inline. The one thing I could not independently re-verify from this sandbox (network access to |
4 tasks done
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 116cfed65c
ℹ️ 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".
Third review round, three findings, all of them real. The class description said every instance "never looked", which its own table contradicts twice: Maven model validation did look, found the missing plugin version, and downgraded it to a warning, and the review action completed a review it never posted. Both are looked-then-declined- to-enforce, not non-execution. Separated the two shapes and moved the diagnostic from "did it run" to "did its verdict reach anything that acts on it", which is the question that catches both. The reactor trap counted four example modules consuming the core test jar. There are five, plus parallel-consumer-reactor, which was missing from the named list entirely - eight modules, now enumerated rather than counted, since counting is what went wrong. The changelog-gate quote was attributed to docs/issue-references.md. It lives in docs/releasing.md:95-100, and the quote was slightly off as well. Both fixed. Checked the other citations in the same paragraph - docs/investigating.md:27-28 and docs/ci.md:63-70 both say what they are quoted as saying. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
|
Claude finished @astubbs's task in 2m 2s —— View job Reviewing PR #282
SummaryThe branch had already moved to
Also re-verified the other two citations in that same paragraph that the commit message claimed to have double-checked ( No new findings survive this pass — nothing to post inline. This closes out the third review round (Codex's initial 9 + my 1 finding, then Codex's 3 more) with every raised point addressed and independently confirmed against the repo state, not just the author's description of the fix. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6eb07c4ba
ℹ️ 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".
docs/ci.md still said the grep -q pipeline "reports failure exactly when it matches". The learning added in this PR now says why that is too strong, so the topic doc was left contradicting it - the same drift the ossindex in-flight note had, one round earlier. Swept the other three statements of this mechanism. bin/check-shell- sigpipe.sh and .github/workflows/repo-hygiene.yml both open with the same loose sentence but qualify it in the very next one, which is the shape docs/ci.md now has too. No other instance in the tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
|
Claude finished @astubbs's task in 3m 6s —— View job Reviewing PR #282
|
astubbs
added a commit
that referenced
this pull request
Aug 11, 2026
Two conflicts, both in files this branch rewrites, and both needed a decision rather than a resolution. .github/workflows/claude-code-review.yml - master (#273) added bin/check-docs-data.sh and bin/test-check-docs-data.sh to the reviewer's allowlist; this branch removes the reviewer from that file entirely. Taking this branch's side resolves the conflict and silently drops those grants, which is exactly the loss this branch predicted for #279's OSS Index grants and wrote into docs/inflight/ci-review-agent.md. So both grants - all four entries, with master's rationale for the one that writes - are carried into claude-code-review-dispatch.yml, where the reviewer now lives. Both scripts exist after this merge, so the grants are live rather than inert. docs/inflight/ci-review-agent.md - master added a finding that claude.yml passes no --allowed-tools, so an `@claude` review cannot run any of this repo's scripts, proven in-session on #273. The finding is kept: it is evidence, and it corrects an earlier wrong conclusion. Its recommendation is not. "Mirror the bin/check-* grants into claude.yml" rested on `@claude` being the only way to review a PR that edits claude-code-review.yml, which held only while that file invoked the action. It no longer does, so the recommendation is recorded as retired by this branch rather than left standing as outstanding work - and doing it now would widen a trigger anyone can fire on a public repo, for no benefit. docs/ci.md merged cleanly - master's edit landed in the repo-hygiene bullet, this branch's in the reviewer bullet and the new review section. One duplication removed by hand: master's #282 published docs/solutions/workflow-issues/a-check-that-reports-success-without-having-run.md, which names the reviewer's workflow-validation skip as one instance of that class, so the paragraph here now cites it instead of restating the class and carrying a count that would go stale. Re-verified after merging rather than assumed: the gate self-tests, actionlint on all three workflows, check-action-versions, check-shell-sigpipe and its self-test, shellcheck, check-issue-refs, and the newly inherited check-docs-data pair. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
astubbs
added a commit
that referenced
this pull request
Aug 11, 2026
Brings in #282 (three learning documents naming the "check that reports success without having run" class), #273 (the documentation-as-data corpus and its `docs data: audit` gate) and #224 (the surefire retry removal). One conflict, in claude-code-review.yml: both sides appended to the single --allowedTools line. Resolved as a union - master's four bin/check-docs-data.sh grants plus this branch's actionlint, ossindex and cve-exclusion grants, each kept in the position it held on its own side. Also named the two new self-tests in the prose above it, which enumerates the granted verification scripts and had never listed them. Read what came in rather than trusting the clean merge on the two shared docs: docs/ci.md - #282 rewrote the sigpipe sentence in the repo-hygiene entry this branch also edits. Git merged them without complaint, but the result had drifted: "Both are required status checks" was written when the workflow had two jobs and this branch adds a third. Now "Two of the three", which is what it always meant. The new cve-exclusions bullet also cites #282's a-check-that-reports-success-without-having-run.md, since the rule it states - ask what a check does when it CANNOT decide, and make that state distinguishable from pass - is exactly what the undated/unparseable/ future-dated failures implement, and this branch applies it to the audit's own escape hatch. bin/check-cve-exclusions.sh's header cites it too. docs/inflight/ci-ossindex-audit-dead.md - #282 edited it and this branch deliberately holds no delta of its own, to avoid the cross-PR collision an earlier revert already backed out of. Confirmed still true after the merge: `git diff origin/master...HEAD` on that path is empty. #282's documents cite bin/check-ossindex-audit.sh, bin/test-check-ossindex-audit.sh and docs/inflight/ci-ossindex-lane-reassessment.md as "not yet merged". They become plain references when this PR lands; removing those annotations is a follow-up on master, not this branch's business. Re-verified on the merged tree rather than assuming: both audit self-tests, the exclusion-expiry check, sigpipe, action versions, issue refs, the new docs-data gate, actionlint, `mvn validate`, and a cold-cache live authenticated OSS Index scan - 9 modules, 214 components, 0 vulnerable, 7 suppressed, guard exit 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
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
Three durable learnings from a session that started with a Maven warning and ended up naming a class of defect the repo has now hit seven times.
docs/solutions/workflow-issues/a-check-that-reports-success-without-having-run.mddocs/solutions/workflow-issues/negative-results-need-an-instrument-that-could-have-said-yes.mddocs/solutions/build-errors/maven-multi-module-plugin-and-resolution-traps.mddocs/inflight/ci-ossindex-audit-dead.mddocs/ci.mdgrep -qpipeline fails "exactly when it matches", which the first document now explains is too strongWhy the first one is a class and not an anecdote
Seven instances, all in this repo. Six reported success; the seventh reported failure on every run and nobody looked, which is the same outcome by a different route. They come in two shapes: most never looked, while two looked and then declined to enforce what they found. So the diagnostic is not "did it run" - that clears both of the second kind - but did its verdict reach anything that acts on it.
ossindex-maven-plugingot HTTP 401 on every module while configuredfail=false, so the vulnerability scan reported success by not running.fail=truedoes not fix that - it covers "vulnerable components were found", not "the request failed". Verified: forced on with no credentials, the 401 is a[WARNING]and the build reportsBUILD SUCCESS. The plugin exposes no setting that makes an unreachable scanner fatal.claude-code-actionrefuses to run when its workflow differs from the default-branch copy and reports that skip as success.--commentwas never passed.bin/check-copyright-headers.shexits 0 with only a warning on a shallow clone.master, and went unnoticed because it is not a required check.The rule: for any check, ask what it does when it cannot run, and make that state distinguishable from pass.
The actionable half is guard design. For the audit, the obvious guard - grep the log for
Failed to fetch component-reports- was rejected during implementation, because a log-string guard dies the day the vendor rewords the message, which is the same rot it exists to catch. The guard that was built is structural: every exportedossindex-report.jsonmust have a non-emptyreportsmap, because on a 401 the plugin still writes the file but writes{ }. That leg is fail-closed only on a cold client cache, which is why the job must never cache the report directory. And it was proven by making it fail - expired token producedBUILD SUCCESS,maven exit=0, guard exit 1. A guard nobody has watched fail is not a guard.The other two
The second document covers conclusions produced by an instrument that could not have answered: the OSS Index client's on-disk cache returning full results against a bogus URL and an expired token; two-dot vs three-dot
git diffrendering a behind-branch's inherited commits as phantom deletions. Two of those are negatives; the diff one is explicitly labelled as the mirror image, a false positive from the same defect, and kept because the cure is identical in both directions. It cites rather than restatescompound-tooling-breaks-in-worktrees-and-forks-2026-08-07.md, which already covers thegh-resolves-upstream case including its fix.The third found a spine while being written - declared is not resolvable - which unifies the profile-scoped plugin version,
validatecompiling nothing so a sibling cannot be resolved from the reactor, and publishing snapshots that no<repositories>entry can read. That last one includes the fix that was considered and rejected: declaring the snapshot repo would change the whole project's resolution to satisfy one auxiliary probe.Provenance
Each document had an independent validator pass before landing, which earned its keep: one caught an internal contradiction (a trap asserted a test-jar "is not published" when it is - the real cause was the repository not being declared for reading).
One of those withdrawals turned out to be the thing that was wrong. Validation had withdrawn the claim that the managed dependency-submission workflow failed on
master, judging the evidence insufficient; review then asked for the residue of that claim to be removed from the frontmatter too. Checking it directly settled it the other way -gh api repos/astubbs/parallel-consumer/actions/workflows/261174653/runsreturns every run asfailure, with 20+ of them onmasterbetween 2026-04-21 and 2026-08-11. The claim stands, is now evidenced, and the earlier withdrawal was a false negative of exactly the kind the second document is about.Three cited paths -
bin/check-ossindex-audit.sh,bin/test-check-ossindex-audit.shanddocs/inflight/ci-ossindex-lane-reassessment.md- land with #279 and are annotated in prose as not yet merged. Nodepends ongate: that PR's review check is deliberately red (it edits the reviewer workflow), so gating this behind it would trade a short-lived inaccuracy for an indefinite block.Checklist
CHANGELOG.adoccorrectly untouched: perAGENTS.mda PR never adds an entry, and captured learnings are invisible to users and operators.N/A. Prose. The claims in it were verified against the repo and against live runs while the work was being done, and each document had an independent validator pass.N/A. No workflow, runner or permissions files touched.