Repository navigation
ci(deps): run the OSS Index audit in one job that can prove it scanned - #279
Conversation
The vulnerability audit has been reporting success without running. `ossindex-maven-plugin`
sat on `validate` with `fail=false`, got HTTP 401 from ossindex.sonatype.org on every
module, and every build stayed green. Credentials now exist as repo secrets; nothing
consumed them.
Adds `.github/workflows/dependency-audit.yml`, job `deps: ossindex audit`.
**The trap this is built around.** The plugin's `fail` parameter covers "vulnerable
components were found", not "the request failed". There is no setting that makes an
unreachable scanner fatal. Verified end to end with a cold client cache and an expired
token: `[WARNING] Failed to fetch component-reports`, HTTP 401, `BUILD SUCCESS`, exit 0.
So a green Maven run is never by itself evidence the audit happened, and a token expiry
six months from now would degrade silently back to green-but-not-scanning.
`bin/check-ossindex-audit.sh` is what makes the job honest. Three fail-closed legs: the
failure line in the log; every exported report must carry a non-empty `reports` map (a 401
leaves `{ }` behind, so "the file exists" proves nothing); and one report per module the
log says it checked. Leg 2 is deliberately structural rather than textual - it is the leg
that still works if Sonatype rewords the warning, which is the realistic way a log-grep
guard rots into the defect it was written against. `bin/test-check-ossindex-audit.sh`
drives all three with negative controls, and CI runs it before the guard, per the house
pattern. Confirmed against the real thing in both directions: the guard turns an
exit-0 BUILD SUCCESS red on an expired token, and passes the authenticated run.
**Reports, does not block - on purpose.** Findings go to the job summary and do not fail
the job; the tree currently has 5 vulnerable components across 12 advisories, top CVSS
9.3, so a blocking job would be red on every PR inside a week and then ignored. Red is
reserved for "this check stopped working", which is rare and therefore still readable.
Making findings fatal is a one-line change once that backlog is triaged.
**Weekly schedule.** The gap nothing else covers: `deps: vulnerabilities` only reviews
dependencies a PR changes, and Dependabot misses transitive components outright -
netty-codec-http, lz4-java and HdrHistogram all carry advisories here with no alert in any
state. A scheduled whole-tree scan is the only thing that sees an unchanged tree acquire a
new advisory. Weekly rather than daily because the answer only moves when the advisory
database does. This is the sole cron lane in the repo and does not contradict "there is no
scheduled build": it re-runs no suite, and what it catches is a function of elapsed time.
**No ordering dependency on #259.** `ossindex.skip`, `ossindex.fail` and
`ossindex.authId` are user properties of the plugin itself, so the command behaves
identically against today's pom (hardcoded `fail=false`, no `authId`) and against the one
that PR leaves behind. The pom is deliberately untouched here.
`test-compile` rather than `validate`, which cost an experiment: the audit mojo resolves
TEST-scope dependencies and the modules depend on `parallel-consumer-core:jar:tests`, so
against a repository with the PC snapshots removed a bare `ossindex:audit` dies at the
second module with "Could not resolve dependencies". The reactor has to have compiled
them. 46s locally for the whole tree.
Also worth knowing for anyone experimenting: the OSS Index client keeps an on-disk report
cache (macOS `~/Library/Application Support/Sonatype/Ossindex`). A warm cache serves full
results even against a bogus base-URL, which invalidated the first two negative controls
run here. Clear it before any reachability experiment, and never cache it in CI.
Permissions are `contents: read` only - findings go to `$GITHUB_STEP_SUMMARY` specifically
so the job never needs `pull-requests: write`, given it runs PR-authored build code beside
the token. Skipped for fork PRs, which get no secrets and would 401 forever.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Dependency ReviewThe following issues were found:
License Issues.github/workflows/dependency-audit.yml
OpenSSF Scorecard
Scanned Files
|
|
Claude finished @astubbs's task in 19m 8s —— View job Review of PR #279 —
|
✅ 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). |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The guard could die in the one direction it exists to prevent: a scan that genuinely ran, reported as an unexplained red. - A coordinate can appear under `vulnerable` with an empty `vulnerabilities` list - an advisory OSS Index has not scored yet. That left the component's advisory map empty, and `max()` on it raised ValueError. Under `set -euo pipefail` that killed the whole guard with a traceback instead of rendering the findings it had just successfully collected. Now `top_of()`, which returns 0.0 for an unscored component. - Advisories were keyed on `id`, so two carrying no id both landed on "?" and the second silently overwrote the first - under-reporting real findings in both the per-component count and the run total. Now keyed positionally when no id is present, which still de-duplicates the same advisory across modules. Both are covered by new self-test cases, and both cases fail against the previous guard - verified with a control arm rather than assumed. Also from the same review pass: - Parse the parser's `key=value` facts in one whitelisted pass instead of six near-identical `sed` calls, and fail loudly when a fact is missing. A key renamed in the python block used to become a silently empty variable, with nothing tying the two halves together. - Document that leg 3 counts every `ossindex-report.json` under the tree with no run-identity check, so a stale one from an earlier or wider run is counted as this run's output. CI is immune (fresh checkout); a local re-run is not, so the documented local flow now clears them first. - The workflow header stated "off by default in the pom" as present fact. It is only true once #259 merges; until then the plugin still runs, 401ing, in every other Maven job. Qualified. - `docs/inflight/ci-review-agent.md` said four reviewer grants were missing. `bin/todo-index.sh` was already granted in bd021cb - both spellings. Three, and recorded so it is not re-added. Adds docs/inflight/ci-ossindex-lane-reassessment.md: GitHub's automatic Maven transitive dependency submission was enabled on this repo on 2026-08-11, which puts this lane's transitive-coverage justification on a timer. The note records the measured pre-flip overlap, the baseline, and the concrete re-measurement to run after the next push to master. Decision for now is to keep the scanner. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
Review of PR #279 (re-run after commit
|
A bare `#259` on an added line fails the PR Checklist issue-ref gate: the fork's numbering sits inside confluentinc's range, so a bare number is ambiguous. The paragraph already names the PR in full two lines above, so this one just drops the number rather than repeating it. 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 9s —— View job Review of PR #279 (re-run after commits
|
|
Thanks - genuinely useful pass. All 14 inline threads are answered and resolved: 8 became code changes in Fixed. Both real bugs were confirmed with a control arm rather than by inspection - each new self-test case fails against the previous guard and passes against the fixed one:
Pushed back, with evidence - The two summary-only findings:
Not addressing here, for the same reason as the assert helpers:
Declining. The premise is stronger than the evidence: Also added Separately, and unrelated to the PR: noted your run-note about background review agents receiving unsolicited messages impersonating the orchestrator and asking them to redirect findings. Declining and reporting through the normal channel was the right call, and it has been passed on to a human. |
The reassessment note tells a future reader to re-measure the dependency graph after the next push to master. That instruction was incomplete: GitHub's managed `Automatic Dependency Submission (Maven)` workflow has failed on every run since the feature was enabled, so the graph will not repopulate and the re-measurement would read as "no change" for entirely the wrong reason. The failure is repo-wide, not this branch's - identical on #259 and #278. Its `validate-project` step cannot resolve `parallel-consumer-core` (or its test-jar) on a clean runner, which is exactly the failure mode dependency-audit.yml already documents and works around by running `test-compile` instead of `validate`. 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 1m 58s —— View job Review of PR #279 (re-run after commit
|
#282) Three learnings from a session that began with a Maven warning and ended up naming a defect this repo has now hit seven times. docs/solutions/workflow-issues/a-check-that-reports-success-without-having-run.md The class. Seven instances, all here: ossindex-maven-plugin 401ing on every module under fail=false; `fail=true` not covering transport failure at all - no setting does; claude-code-action reporting a workflow-validation skip as SUCCESS; the review workflow running green for months while posting nothing, because --comment was never passed; check-copyright-headers.sh exiting 0 on a shallow clone; Maven degrading a malformed model to a warning; and GitHub's managed dependency submission failing on master since April, 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, "grep the log for the vendor's warning string" was rejected during implementation - a log-string guard dies the day the vendor rewords it, which is the same rot it exists to catch. The guard that shipped is structural: the exported report must carry a non-empty reports map, because on a 401 the plugin still writes the file but writes `{ }`. And it was proven by making it fail. A guard nobody has watched fail is not a guard. docs/solutions/workflow-issues/negative-results-need-an-instrument-that-could-have-said-yes.md The investigation-side sibling: negatives produced by an instrument that could not have answered. OSS Index's on-disk cache returning full results against a bogus URL and an expired token, invalidating two negative controls. Two-dot vs three-dot git diff rendering a behind-branch's inherited commits as phantom deletions. Cites rather than restates the existing worktrees-and-forks write-up, which already covers gh resolving to the wrong repo. Before trusting "no findings", "no prior art", "no diff" - prove a positive was reachable. docs/solutions/build-errors/maven-multi-module-plugin-and-resolution-traps.md Declared is not resolvable. One mechanism behind four traps: a plugin version declared only inside a profile but consumed version-less by a child module; `validate` building no artifacts, so a module needing a sibling's jar must resolve it remotely; publishing snapshots that no <repositories> entry can read; and GitHub's managed submission probing with a bare `mvn validate`, which no amount of publishing fixes. Each document had an independent validator pass, which earned its keep. One caught an internal contradiction - a trap asserting a test-jar "is not published" when it is, the real cause being that the repository holding it is not declared for reading. Another withdrew an unsupported claim, and review then reinstated it with evidence the withdrawal had missed: every run of Automatic Dependency Submission is a failure, 20+ of them on master between 2026-04-21 and 2026-08-11. The withdrawal was itself the false negative. Four review rounds corrected real errors: a headline that contradicted its own table, a class description claiming every instance "never looked" when two rows are looked-and-declined-to-enforce, a module count wrong twice over, a quote attributed to the wrong topic doc, and a positive control that could be defeated by gh's default --limit of 30 - the doc committing the very error it warns about. Three cited paths arrive with #279 and are annotated in prose as not yet merged. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The jackson-bom ignore was blanket. It was added to stop an automated jump into 2.21.x, where CVE-2026-54518 stops being a false positive and becomes real - correct intent, wrong scope. Every danger it names is a LINE change, and a blanket ignore also silences patch releases inside the pinned line. That is not hypothetical here: the root pom's two TEMPORARY exclusions (CVE-2026-68497, CVE-2026-19032) retire when jackson-databind 2.18.10 reaches Central, which is a PATCH bump - so the ignore was suppressing the one PR that would have announced the retirement condition. Neither id is in OSV, NVD or GHSA, so no Dependabot alert can arrive instead. The suppression was covering the notification we were relying on, and nothing reports a PR never opened. Both Jackson entries now block minor/major and allow patches, matching the micrometer entry that already did this. jackson-databind gets the same treatment for the same reason and to keep the two siblings the same shape: its recorded hazard - a global pin dragging WireMock onto an incompatible Jackson and breaking VertxTest - is a line change too, while a patch touches one test-scoped dependency in one example module, which is exactly how a future advisory against it would be fixed. The whole-tree CVE scan resolves test scope, so it would report such an advisory and leave it red with no automated route to green. Swept the rest of the list. net.bytebuddy stays blanket, and is now marked as deliberate rather than looking like an oversight beside the two narrowed entries: it is a freeze until wiremock 3.x, so there is no update size we want. astubbs/* (actions) is the same shape. The seven deferred-major entries are already scoped to major only, and each one's block agrees with the pom exclusion that depends on it - micrometer minor stays blocked, which is what CVE-2026-40984's "retire when the prometheus rename is migrated" says, and the 2.21.x line stays blocked, which is what CVE-2026-54518 needs. Recorded the rule above the list so the next entry starts scoped: blanket is for a freeze, anything you expect to move again names the sizes it fears. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
…e read The root pom carries two TEMPORARY exclusions - advisories whose upstream fix is merged but unreleased - and nothing was ever going to tell us when to retire them. Both channels were shut: neither id exists in OSV, NVD or GHSA, so no Dependabot alert can fire, and the jackson-bom ignore had suppressed the 2.18.10 patch bump that is the retirement trigger (fixed in the previous commit). What was left was a comment addressed to whoever happened to read it. bin/check-cve-exclusions.sh parses excludeVulnerabilityIds out of the pom and fails once a temporary entry outlives its window. Undated, unparseable and future-dated markers FAIL rather than reading as "not expired yet" - a date that cannot be read must never evaluate as fresh, which would reproduce inside the audit's own escape hatch the silent-green defect the audit guard exists to prevent. An id with no rationale comment fails too: unclassified silently means standing-forever, the same bug by omission. RECOGNITION: a machine-read marker in the comment that introduces the entry, `TEMPORARY-SINCE: YYYY-MM-DD`, enforced rather than hoped for. The pom stays the single source of truth because excludeVulnerabilityIds accepts nothing but ids - Maven rejects unknown elements in a typed plugin configuration - so a comment is the only carrier that lives with the thing it describes. A sidecar registry would parse more easily and start drifting on the first edit, which is the same defect one level up. WINDOW: 90 days, one quarter, as a named constant the self-test reads rather than restates. Anchored on the cadence that actually retires these entries - the upstream patch line, not our own release cycle, which is far sparser and unrelated to when a fix ships. jackson-databind 2.18.x ran ten releases from 2024-09-27 to 2026-07-08 with gaps of 31, 31, 92, 68, 174, 117, 61, 35 and 40 days, median ~65, so the typical patch lands well inside the window. Expiry is not a missed deadline and the red is always clearable in one reviewable line: retire the entry if the fix shipped, or re-check upstream and re-date or reclassify it. That matters because a red nobody can clear is the failure mode already named here - the audit stayed reporting-only for exactly as long as it had a standing backlog. OWNERSHIP: Repo Hygiene, not the audit job that otherwise reads this list. That job is skipped for fork PRs and dies early on a token expiry or a Sonatype outage, and its "unprovable scan wins" precedence would swallow this - so the list would rot unwatched for exactly as long as the lane was broken. The check needs no JDK, no secrets and no network, which is this workflow's charter. `deps: CVE exclusion expiry` is a NEW job name and is deliberately not added to the master ruleset here; making it required is a separate act. Exit 3, leaving 1 and 2 to keep the meanings check-ossindex-audit.sh gives them (broken lane, real finding). Stale bookkeeping is neither: the scan ran and the tree is clean, only the paperwork rotted. Verified: the self-test's control arm is two fixtures differing by a single day either side of the limit - the fresh one passes, the expired one fails - and the suite was watched to fail both ways by breaking the guard (disabling the undated branch: 2 red; widening the comparison to 10x: 4 red). Live cold -cache authenticated 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
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
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
…d one (#284) Every push spent a full Claude review, most of them on work that was not ready - so "get CI feedback" and "spend a review" were the same action. This splits them. - claude-code-review.yml becomes a cheap always-on GATE. Same job name `claude-review` (a required check, and job names are a ruleset API), on `pull_request`, but with no claude-code-action in it: no JDK, no build, no review. It asserts that a review exists for the CURRENT head and fails if not. Runs in seconds, and its permissions drop from pull-requests:write + id-token:write to read-only. - claude-code-review-dispatch.yml becomes the REVIEWER, on workflow_dispatch with `pr` and `focus` inputs. It keeps the packaged procedure - `plugins: code-review@claude-code-plugins` and `/code-review:code-review <repo>/pull/<n> --comment` - with `focus` appended as a steer that never replaces it, so a requester can aim a review without degrading it to a free-form answer. It checks out refs/pull/<pr>/head, so the workflow comes from the default branch and the code under review from the PR. - claude.yml is byte-identical to master. Continuous coverage does not go away: Codex still auto-reviews every push. What changes is that the Claude pass becomes the deliberate structured review at ready-time, from a different model, instead of a second opinion on unfinished work. STRICT, WITH NO ESCAPE HATCH. The gate requires a review newer than the head. Lenient - any review, ever - would let a review of commit 1 vouch for commits 2..N, which is the defect class this repo has been naming all week. No skip word or label, because a skip is asserted by whoever wants to use it, making the gate exactly as strong as no gate. The honest escape already exists and is loud: merge with the check red, leaving a permanent record. A second-order win, evidenced rather than assumed: PRS TOUCHING THE REVIEWER NO LONGER NEED AN ADMIN BYPASS. claude-code-action's validation guard keys on the INVOKING workflow, so a gate-only claude-code-review.yml has nothing to refuse. The natural experiment is on #279: at one SHA, the pull_request run of a modified claude-code-review.yml was refused while a review of the same SHA through another workflow succeeded. Two defects this PR introduced and then closed, both found in review: - The gate stayed red forever after a successful review - the P1 that broke the whole promise. The reviewer now refreshes the gate's run for the head. - Fork code beside the review credential. The old pull_request trigger was doing unnoticed security work, because GitHub withholds secrets from fork PRs. workflow_dispatch has no such protection while checking out the PR head. Dispatch permission says the DISPATCHER is trusted; it says nothing about the branch under review. Now guarded explicitly. Also hardened: the server-side timestamp is preferred over the contributor- controlled committer date rather than max'd, since a future-dated commit could otherwise hold the check unfixably red; same-second ties fail closed; the run marker uses openssl rand instead of the public, forgeable github.run_id; checkbox scanning implements the full CommonMark closing-fence rule, after a partial version closed early on nested backticks. Every rule is mutation-tested - reverted, and exactly its case goes red - and all three red paths have fired on live PRs: never-reviewed, stale-review, and reviewer-left-its-boxes-unticked. The merge from master rescued two grants that a clean resolution would have silently dropped: #273 had added bin/check-docs-data.sh and its test to the reviewer's allowlist, and this branch removes the reviewer from that file. Carried into the dispatch workflow and verified live rather than inert. KNOWN, AND STATED SO NOBODY DISCOVERS IT LATER: the dispatch route could not be tested before merge. A workflow_dispatch workflow cannot be dispatched unless the trigger exists in the default branch's copy - `gh workflow run` and the REST API both 404 with an explicit --ref. The first dispatch after merge is the test. If track_progress does not author its comment as claude[bot] on a payload with no PR, the gate cannot go green until fixed forward; the known remedy is to gate on a check run against the head SHA, which would also retire the timestamp machinery. The gate fails closed, so the failure mode is blocked-and-loud rather than silently unprotected. Operationally: the auto-review stops, so any open PR whose head moves afterwards needs a deliberate review request before it can merge. docs/ci.md and AGENTS.md carry the agent-facing rule - request when the PR is ready, not on every push, and a red claude-review on an unreviewed PR is the expected state, not a fault. One boundary left open and written down: the gate cannot distinguish a review from any other answer the bot gave on the PR. This PR's own claude-review is red for exactly the reason it documents, and merging it needs an admin bypass - the last time that will be true. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Master landed the rebuilt review system (#287), which moved the reviewer out of the file this branch was editing. All three conflicts are on that seam, and none of them is a mechanical pick. .github/workflows/claude-code-review.yml - took master's side wholesale. Master rewrote it into a gate that invokes no Claude, so the `claude-code-action` step this branch was extending no longer exists in it. THE GRANTS WERE CARRIED, NOT KEPT BY LUCK: bin/check-ossindex-audit.sh, bin/test-check-ossindex-audit.sh, bin/check-cve-exclusions.sh and bin/test-check-cve-exclusions.sh - bare and ./ forms, eight entries - are re-added to BOTH claude-code-review-dispatch.yml and claude.yml, because master's own comment says the two allowlists must stay byte-identical apart from the inline-comment tool and nothing yet enforces it. A grant on one route only is a reviewer that silently cannot run the check it was asked about. All four scripts exist on this branch, so none of the grants is inert. This is the drift docs/inflight/ci-review-agent.md predicted would first bite right here. It also repairs an earlier instance of the same loss: this branch's previous merge (0cfe05b) dropped #273's check-docs-data grants from the allowlist without anyone noticing. Deleting this branch's copy of the list and adding to master's puts them back. docs/ci.md - kept both sides, minus one. This branch's cve-exclusions bullet stays under repo-hygiene; master's "The three claude* workflows" section replaces the old claude-code-review.yml bullet entirely. That bullet restated the review contract, and master marked a single paragraph canonical - so it is dropped rather than merged, and the surviving text points at the canonical statement instead of repeating it. docs/inflight/ci-review-agent.md - took master's rewritten bullets; all four of this branch's were superseded (credential exposure, the integration-test cap and the workflow-validation trap are all restated better there, and "claude.yml grants nothing" is simply no longer true). Master's "the OSS Index audit grants must be carried across at merge" entry is the instruction this commit just carried out, so it is retired and rewritten to keep only what is still open: that no reviewer has yet run any of the four scripts, making the grants asserted capability rather than demonstrated. .github/workflows/README.md is master's new per-workflow index and knew nothing of this branch, so dependency-audit.yml is added to it and repo-hygiene.yml's line now mentions the CVE exclusion expiry job. Verified: both self-tests pass, check-cve-exclusions reports the temporary jackson entries inside their window, actionlint clean on every touched workflow, check-action-versions / check-shell-sigpipe / check-issue-refs and both node gate self-tests green, mvn validate green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
|
@claude review this Focus, in the order it matters:
Both self-tests are runnable and you are granted them: Note on this branch's last commit: it is a merge of master, which had just rebuilt the review system under #287 and moved the reviewer out of |
|
Claude finished @astubbs's task in 3m 22s —— View job Review of PR #279 — focused re-check per @astubbs's three priorities
1. Exit-code precedence (1 beats 2, 3 is separate) — confirmed correct
The WARNING + BUILD SUCCESS scenario specifically: the guard doesn't trust Maven's exit code at all — it never looks at it. It greps the log for the literal captured string ( 2. Exclusion filtering — confirmed correct, and it's per-advisory not per-component
The comment at lines 179-186 states this was "verified on a live authenticated run" (excluding CVE-2026-54518 and observing it still listed in 3.
|
🧪🔒 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 |
…analysis to what it scans testing-evidence.yaml's quality_system listed five gates and neither of the ones this branch adds, so the published evidence described less than the repo does - the drift class the file itself keeps documenting. - whole-tree-cve-scan: the OSS Index audit. Its posture leads with the thing that makes it worth publishing at all - the plugin reports BUILD SUCCESS against an unreachable scanner, so the guard fails an unprovable scan (exit 1) distinctly from a real finding (exit 2), and green means the audit ran rather than that Maven did. Stated boundaries: not yet a required status check, fork PRs skipped for want of credentials, and the exclusion list a maintained artifact rather than evidence of absence. - cve-exclusion-expiry: the 90-day TEMPORARY-SINCE rule. Undated, unparseable and future-dated entries fail rather than reading as not-expired-yet; exit 3 is kept distinct because neither the tree nor the lane is at fault. Records that what expires is the classification, not a claim that an upstream release is late. static-analysis's detects said "dependency vulnerabilities in the PR path", which described dependency-review only. Narrowed to "the dependencies a PR itself adds or changes" - the one clause that distinguishes it from the whole-tree entry, without restating that entry. roadmap.yaml wants neither: its update_policy says an entry closable by a single pull request is too small to be there, and both of these are. module-maturity.yaml is per-module posture, which a CI lane is not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
…scripts The entry said the OSS Index grants were untried. They were tried and refused. On the comment route, run 31547154463 reviewing #279, the reviewer invoked both test scripts bare, as ./, and under sh; every attempt returned "this command requires approval" with no prompt reachable. It verified its own --allowedTools for the route it was on, which rules the workflow allowlist out. So the recorded conclusion changes from "asserted capability, not demonstrated" to something sharper: there is a second approval layer beyond --allowedTools, and it defeats the point of curating the first. The visible cost is already paid - the review of the pull request whose diff is those scripts was a manual trace rather than a run. Kept to what is open and what would settle it; the investigation itself is being run separately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
…s allowlist The entry claimed the reviewer on run 31547154463 was refused four granted scripts by some layer beyond `--allowedTools`, and that the workflow allowlist was ruled out because the reviewer had checked it. Both halves were wrong, and we wrote them. That run was an `issue_comment`, which GitHub runs from the default branch, so the list in force was master's `REVIEW_TOOL_ALLOWLIST` - which has none of the four grants in it. The action prints the resolved list twice, in the step's `claude_args` and again in the SDK's `allowedTools` array, and it is absent from both. The reviewer's self-check cited `claude.yml:60` and `claude-code-review-dispatch.yml:394`, which are the same line numbers in master's copy and the PR's, and asserted the grants were present - so it had read the PR's copy over `gh pr diff` rather than the file underneath it, and mistook "granted in the diff" for "granted in the running copy". The generalisation is worth more than the incident: a grant a PR adds can never take effect on that PR, on any route. `docs/ci.md` already says so under "Editing the reviewer", so this entry points there rather than restating it - the canonical-statement rule from #287. What survives is the one genuinely open thing, that the grants stay unexercised until they merge. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
…anding in
Both routes now carry a line in the system prompt saying where their
configuration came from, because a reviewer that does not know reports the
wrong thing and sounds certain doing it. On the dispatch route: the allowlist
is the default branch's copy of the workflow, so a grant the PR adds is not in
effect and a refusal is expected rather than a defect. On the comment route the
same is true of the allowlist - and of the working tree.
That second half is a defect nobody had written down. `claude.yml` checks out
with no `ref:`, so `refs/pull` never appears in one of its runs: an
`issue_comment` run has master's tree on disk and none of the PR's files.
Verified on run 31547154463, whose checkout resolved to
`git checkout --force -B master refs/remotes/origin/master`. Every execution
grant on that route therefore runs master's code - asked to run
`bin/ci-unit-test.sh` on a PR, it runs master's suite - and the workflow's own
comment claimed the opposite ("BOTH routes run PR-authored code"). Corrected
there, and the fork-head paragraph with it: withholding the allowlist on a fork
was justified by a hazard that is the dispatch route's, since this route never
checks a fork out. The withholding stays, on the two reasons that do hold.
Recorded in docs/ci.md as an open decision with the tradeoff, not as a pending
fix. Adding `ref: refs/pull/<n>/head` would put PR-authored code beside
CLAUDE_CODE_OAUTH_TOKEN on a trigger anyone with write access fires by typing a
comment, which is the credential question in the inflight entry rather than an
answer to it. That is the user's call.
The note is emitted on every path including the fork-head and untrusted-author
carve-outs: it grants nothing, and those are exactly the runs most likely to
misreport. Verified the built `claude_args` stays a single `$GITHUB_OUTPUT`
line, and that the action's tokenizer still parses all 47 grants out of it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
…efore the script
Replaces the enumerated `bin/check-*.sh` and `bin/test-check-*.sh` grants in
both allowlists with four wildcard patterns, in the bare and `./` spellings:
Bash(bin/check-*.sh:*) Bash(./bin/check-*.sh:*)
Bash(bin/test-check-*.sh:*) Bash(./bin/test-check-*.sh:*)
The enumeration had a structural failure built into it. A grant a PR adds can
never apply to that PR - both routes run the default branch's copy of the
workflow - so a per-script grant has to land in an EARLIER PR than the script
it grants. Nobody does that, and the cost is not hypothetical: four check
scripts arrived on this branch, all four were granted here, and this PR's own
reviewer could run none of them. A pattern grants the naming convention once,
ahead of every guard that will ever follow it, and is never inert.
Kept enumerated on purpose, because `Bash(bin/*.sh:*)` would sweep in the
runners this list refuses: ci-unit-test.sh, ci-integration-test.sh,
todo-index.sh, the two `node .github/scripts/*.test.js` gate self-tests,
actionlint, and ./mvnw.
What the pattern accepts, said out loud in the workflow so nobody re-enumerates
it: a PR can add `bin/check-<anything>.sh` and have the dispatch reviewer run
it. That is not a new capability. The dispatch route checks out
`refs/pull/<n>/head`, so `bin/check-copyright-headers.sh` has been executing
whatever the branch says it is for months, the ci-*-test.sh wrappers run the
branch's Java, and `./mvnw` runs the branch's pom. On the comment route the
tree is master's, so the pattern can only ever match a script already on the
default branch. The bound is unchanged: fork heads refused, dispatch requires
write access.
Verified `*` at any position and the `:*` trailing-wildcard form against the
current permissions reference, and that the action's comma-split of
`--allowedTools` still yields all 24/25 entries with the patterns intact.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
|
@claude review this Requesting a fresh review: the last one predates several substantive changes, and the gate is now lenient, so it stays green regardless — the previous review's approval is not evidence about the current head. Please focus on what has changed since:
One thing you will hit and should not report as a defect: the grants this PR adds are not in effect for you. Your allowlist comes from the default branch's copy of the workflow. If a script comes back refused, that is expected — say so plainly rather than concluding the PR is broken, and please tell us which route you ran on and whether the PR's files were actually on disk. |
|
Claude finished @astubbs's task in 4m 15s —— View job Review of PR #279 — fresh pass at commit
|
…tion Reverts the working-tree half of d494a14. The claim was that an `issue_comment` run has master's tree on disk and none of the PR's files, so every execution grant there runs master's code. It is false, and the evidence for it could not have been evidence of anything: the search was for `refs/pull` in a run log, against an action that fetches the head BY BRANCH NAME. A term that can never appear returning zero hits reads exactly like a confirmed absence. Third time today that a search which could not produce a positive was taken for a negative result. `actions/checkout` does leave master's tree - `git checkout --force -B master refs/remotes/origin/master` - and then `claude-code-action` checks the PR out over the top of it. Run 31550657594 shows both, eight seconds apart: the master checkout in "Checkout repository", then "This is an open PR, checking out PR branch...", `* branch ci/ossindex-audit-job -> FETCH_HEAD`, `Switched to a new branch 'ci/ossindex-audit-job'` inside "Run Claude Code". It is `setupBranch` in src/github/operations/branch.ts, and it runs in tag mode only, which is why the dispatch route (agent mode) does its own `refs/pull/<n>/head` checkout and is unaffected either way. So "BOTH routes run PR-authored code" was right, and the section, the tradeoff table and the open decision built on its negation are deleted rather than softened - there is nothing left to decide. The fork-head paragraph is restored to its original reasoning, having been re-derived rather than assumed: `setupBranch` has an explicit cross-repository case that fetches `pull/<n>/head:<branch>`, so a fork's files reach this runner too, and "a fork could replace an allowlisted script" was correct all along. The guard was never in question; only its stated reason was. Untouched, because it is true and separately verified: the WORKFLOW FILE comes from the default branch on this route, so a grant a PR adds is not in effect - that is what run 31547154463 refused the scripts on. The system-prompt note keeps that half and drops the working-tree half, which would otherwise tell a reviewer to disbelieve its own checkout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
Its `posture` said what the check enforces and stopped there, alone among the five quality_system entries in not naming its own blind spot. The script has already written the caveat down - "What it does NOT check is the prose: whether the reason is any good ... is a review question and stays one" - so the entry carries it too: the check reads dates and structure, and green means every exclusion is classified and in date, not that any of them deserves to be there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
…a real instruction The route note added in the previous commit tells the reviewer it can confirm its checkout with `git rev-parse HEAD`. Nothing made that true. The action's tag-mode base list grants `git add`, `git commit`, `git rm` and its push wrapper and no more (src/modes/tag/index.ts), and agent mode - the dispatch route - passes through only what the workflow names, with no base Bash at all (src/modes/agent/index.ts). So the instruction would have been a dead letter pointing at a refusal, which is the same shape of confusion this branch spent the day undoing. Granted on both routes, in step. Read-only, mutates nothing, and it makes the one check that would have caught this whole error affordable: the false "comment route runs master's code" finding came from inferring the tree from the workflow file instead of asking the tree. I claimed in review that a reviewer had run `git rev-parse` unprompted on run 31550657594, and that was wrong - I repeated it rather than checking. The only `git rev-parse` in that log is `actions/checkout`'s own internal call resolving master, and the run's resolved grants contain no such tool. The `2aace088` SHA quoted from it comes from the `pr-context` step's own echo, not from the reviewer inspecting anything. Also corrected in the dispatch workflow: a parenthetical from 2aace08 saying the comment route's `bin/check-*.sh` pattern is "narrower still - that tree is master's". Same false premise, missed in the previous commit's sweep because it sits in the OTHER route's file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
|
@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: 97331c79f6
ℹ️ 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".
`<!-- -->` above an excluded id opened a section, so the id passed and the run reported "every id carries a rationale" - the guard's own invariant satisfied by a comment that says nothing. That is the unreviewed-suppression-becomes-permanent defect the script exists to prevent, wearing the guard's own uniform. A comment now has to carry at least three words to open a section. Below that floor it opens none, and the ids under it are reported unclassified - fail closed, the same way a missing comment already was. A TEMPORARY banner is never dropped for lacking words: it is still parsed for its date and reported as malformed, so the expiry clock cannot be lost by writing less. The floor is not a quality judgement and does not pretend to be. It separates a rationale from a separator rule, an empty comment and a bare placeholder. An unrelated but wordy comment still passes, and that stays a review question - the machine half of the rule is deliberately the half a machine can decide. Three self-test cases, all confirmed red against the old code: an empty comment, a separator rule, and a dated TEMPORARY banner with no prose. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
The fork skip does not cover them. A Dependabot branch lives in this repository, so `head.repo.full_name == github.repository` passes it straight through - but a Dependabot-triggered run resolves `secrets.*` against the Dependabot secret store, not the Actions one, so OSSINDEX_USERNAME and OSSINDEX_TOKEN come back empty and the credential preflight fails deterministically. Every dependency-update PR would have carried a red whole-tree audit, on the PRs least likely to be read closely. The preflight turning a missing secret into a red check is right when the secret is missing by accident. Here it is missing by design, every time, which is the permanent false alarm that teaches people to ignore a check. Gated on `github.actor`, not the PR author, because the actor is what selects the secret store: when a human pushes to a Dependabot branch the run is theirs, the Actions secrets are present, and the scan should run normally. Mirroring the secrets into the Dependabot store was the other option and is worse: it would put the OSS Index token in a second store, reachable by a job that runs `test-compile` over a dependency set an automated process just changed. The scheduled whole-tree scan already covers what those PRs merge, one channel later and without handing the credential around. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
…s done This branch's glob grants falsified bin/AGENTS.md, which still told agents the allowlist "is not a glob" and that a new script stays invisible until someone adds it by hand - binding guidance that would have had the next contributor undo the convention as an oversight. Shrinks bin/AGENTS.md rather than correcting it in place. That file is read by every agent touching the directory, so it should carry only what someone can get wrong by an action taken THERE: naming a script `check-*` or `test-check-*` grants it to the reviewer automatically, so a script that writes, publishes or deploys must not be named that way. Everything else about the allowlist now points at docs/ci.md. Net effect on the file is shorter than master, not longer. The rest moves to docs/ci.md -> "Editing the reviewer", canonical for the review system since #287, whose audience is someone editing the reviewer - rare, deliberate, and already in that file: - the boundary the allowlist actually is. The fork refusal settles whose code runs beside the credential and the review/refresh-gate split settles what the token can reach; neither touches what that code can talk the reviewer into RUNNING, which is the one the allowlist holds. - the naming convention as the new form of that boundary, and why the pattern stops at two prefixes. - what still needs a manual grant: both spellings, both files, and an earlier PR than the script. Drops the bin/test-check-review-posted.sh incident narrative. It was the evidence for a rule that no longer needs arguing - the naming convention now enforces it - and a file every bin/ agent reads should not pay context rent for a settled argument. It survives in git history. Swept the repo for the same staleness by the claim rather than the wording: docs/inflight/ci-review-agent.md, .github/workflows/README.md and both workflows' comments were already correct, and docs/ci.md's pattern bullet already was too. bin/AGENTS.md was the only hit. Also grepped every bold label removed from it - none was cited anywhere; "Scripts that guard other scripts", which is cited twice, is untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
… exit 0 The durable guide still documented the audit guard as reporting-only - `scan ran, found problems -> exit 0, findings rendered (reporting only, deliberately)` - which this branch inverted to exit 2. The scripts cite that guide by name in their headers, so a maintainer following it would have applied the opposite gating semantics to the thing it is the record for. Corrected to the two-reds split, and the reason findings became fatal is kept rather than overwritten: they were NOT fatal originally, deliberately, because a standing backlog would have made the check red on every PR. #281 retired that backlog, which inverts the argument rather than repealing it. The general rule - a red that fires on known debt means nothing - survives intact; what changed is the tree. Retires docs/inflight/ci-ossindex-audit-dead.md. The gap it tracked is what this PR closes, and the inflight convention is to delete the file in the PR that resolves it rather than rewrite it into a DONE narrative - it would otherwise sit on master asserting that #279 is unmerged and that no scheduled whole-tree scan exists, and be inherited as live work by the next agent scanning in-flight state. Its durable half was not deleted with it. The scanner's error profile is the load- bearing part, cited from pom.xml, the guard, and the workflow, so it moves to docs/solutions/security-issues/ with the local-reproduction recipe: one real Dependabot miss, two scanner false positives, one phantom CVE id and two disputed findings, in a single run. Two lessons generalise out of it - an over-claim in your own favour outlives one against you, and two scanners disagreeing is not evidence either is right. The two client traps already had a home in the sibling record and are linked, not copied. All six inbound references re-pointed, and the workflow header's own version of the withdrawn three-component claim corrected with it - leaving it would have recreated the contradiction this commit exists to remove. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
The reassessment note told a future reader to re-enable managed dependency submission after 0.6.0.0 ships, then two paragraphs later established that the managed workflow probes with a bare `mvn validate`, that this cannot resolve reactor siblings whatever is published, and that "waiting for the v6 release does not fix it either, for the same reason" - concluding the setting should stay off. Following the instruction would have restored a permanently red check on every PR and learned nothing. The instruction was written before the root cause was found and never revisited; the trigger at the foot of the note, which requires a working repo-owned submission workflow first, is the version that survived the analysis. Kept as a correction rather than a silent deletion, so the next reader can see the conclusion moved and does not re-derive the abandoned one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
1 participant
Description
Adds
Dependency Audit- a single job,deps: whole-tree CVE scan, that runs the OSS Index dependency scan, can prove it actually scanned, and fails the PR on a finding. Plus the guard that keeps its escape hatch honest,deps: CVE exclusion expiry.Context: the
ossindex-maven-pluginaudit had been returning HTTP 401 on every module for some time while configuredfail=false, so it reported success without ever running. #259 stops it lying by switching it off and visibly off; this PR is the other half - a place where it genuinely runs.Why a guard, and why the obvious guard is not enough
The plugin has no setting that makes an unreachable scanner fatal.
failcovers "vulnerable components were found", not "the request failed" - verified: with an expired token it prints a[WARNING]and reportsBUILD SUCCESS. So a CI job that just runs it and trusts the exit code rebuilds the identical silent-green defect, only somewhere more expensive.The obvious guard - grep the log for
Failed to fetch component-reports- was the original spec here and was rejected during implementation, because a log-string check dies silently the day Sonatype rewords that warning. That is the same class of rot as the bug being fixed.bin/check-ossindex-audit.shtherefore has three fail-closed legs, the load-bearing one being structural rather than textual:ossindex-report.jsonmust have a non-emptyreportsmap - on a 401 the plugin still writes the file, but writes{ };Leg 2 is the one that matters.
bin/test-check-ossindex-audit.sh(run before the guard in CI per the house pattern) includes a case asserting that empty reports fail with no failure line in the log at all - exactly what a log-only guard would wave through.Seen firing, both directions, on real runs: expired token + cold cache →
BUILD SUCCESS,maven exit=0, guard exit 1. Authenticated → exit 0, 214 components across 9 modules scanned.Two reds, and they must not be confused
Distinct exit codes, distinct headings, distinct stderr lines, because they demand opposite responses - exit 1 sends you to the token or the lane; exit 2 says the lane worked and there is something to triage. When both are true, exit 1 wins: findings from a scan that cannot be proven to have happened are evidence of nothing, in either direction.
Findings gate the PR, and they did not always. The first version rendered them and exited 0, because the tree carried a standing backlog and a job that goes red on every PR for known debt is ignored inside a week. #281 retired that backlog - every item is now either fixed or an explicit
excludeVulnerabilityIdsentry in the root pom with a stated retirement condition. That inverts the argument rather than repealing it: on a tree whose known debt is already excluded with reasons, a finding is by construction something nobody has looked at. The general rule survives - a red that fires on known debt means nothing - and what changed is the tree.PR-time rather than the schedule alone, because this repo has one maintainer: a weekly digest nobody is obliged to read is mail, not a control. The schedule still earns its place for what no PR can see - time passing.
The escape hatch for a false positive is the same list: add the id with a reason and a retirement condition. That is a deliberate, reviewable act rather than a flag flip - which is exactly why the list needs its own guard.
deps: CVE exclusion expiry- the escape hatch needs policing tooTwo of the current exclusions (
CVE-2026-68497,CVE-2026-19032, both jackson-databind, example-streams only) are temporary: the upstream fixes are merged on the 2.18 branch, milestone 2.18.10, and there is simply no release carrying them - 2.18.10 is a 404 on Central. There is nothing to bump to.Nothing in this repo was going to tell us when that changed, for two independent reasons, both verified on that pair: neither id exists in OSV, NVD or GHSA, so no Dependabot alert can fire; and the
jackson-bomignore in.github/dependabot.ymlwas written blanket, so it also suppressed the patch bump that is the retirement trigger. That ignore is now scoped to allow patches, which is half the fix.bin/check-cve-exclusions.shis the other half. It parses the root pom and fails once aTEMPORARY-SINCE: YYYY-MM-DDentry is more than 90 days old - and on an undated, unparseable or future-dated marker, which fail rather than reading as not-yet-expired. That last part is the point: a date that cannot be read must never evaluate as "fine", or the guard reproduces inside itself the silent-green defect its neighbour exists to prevent. Every id must also sit under a comment carrying actual rationale prose; an empty comment or a separator rule does not count, and the ids beneath one are reported unclassified.It exits 3, leaving 1 and 2 to keep the meanings the audit guard gives them, and runs in Repo Hygiene rather than beside the scan that reads the same list - because that job is skipped for fork PRs and dies early on a token expiry, which is exactly when an unwatched list rots. No JDK, no secrets, no network.
Expiry asserts the classification expired, never that upstream is late, so the red is always clearable: bump and delete the entry, or re-argue it with a fresh date or a standing reason.
Weekly schedule - and who is skipped
deps: vulnerabilities(dependency-review) only inspects dependencies a PR changes, and Dependabot alerts read from GitHub's dependency graph, which holds what the poms declare rather than what Maven resolves - so a purely transitive component can carry a published advisory and never produce an alert. Measured here rather than assumed:lz4-java,HdrHistogramandnetty-codec-httpwere all absent from the graph, andlz4-javais the confirmed miss (GHSA-xx22-p4ch-683r exists, nothing fired). The wider "three Dependabot blind spots" reading was checked against OSV/GHSA/MITRE and withdrawn - see the scanner record linked below.Weekly, Mondays 06:17 UTC - off-the-hour because GitHub drops runs piled at
:00; daily would produce an identical report ~364 days a year, since the answer only moves when the advisory database does.Two kinds of PR are skipped, and both would otherwise be permanently red:
secrets.*against the Dependabot secret store, not the Actions one, so the credential preflight fails deterministically on every dependency-update PR. Gated ongithub.actor, so a human pushing to a Dependabot branch still gets a real scan. Mirroring the secrets into the Dependabot store was rejected: it would put the token in a second store reachable by a job that runstest-compileover a dependency set an automated process just changed. The scheduled scan covers what those PRs merge.docs/ci.md's "there is no scheduled build, deliberately" is amended rather than contradicted, with the test to apply to any future cron lane: does time alone change the answer? It does here, and for no suite the gate already covers.The managed alternative does not work on a project of this shape
GitHub's automatic Maven transitive dependency submission looked like a live fix for the transitive gap - it was switched on for this repo on 2026-08-11, putting this lane's justification on a timer - and was switched back off the same day. The managed workflow probes with a bare
mvn validateat the repo root, which builds no artifacts, so every reactor module depending on a sibling's jar fails to resolve. That is the same trap this PR's own Maven step documents and works around withtest-compile, and the managed workflow has no such step and cannot be edited in-repo. Snapshots are a red herring and are recorded as an explicit dead end.docs/inflight/ci-ossindex-lane-reassessment.mdrecords the pre-flip baseline, the diagnosis, and the re-measurement to run if a repo-owned submission workflow is ever added. The decision is to keep the scanner, on firmer ground than when that note was written.Two findings that would otherwise have been discovered post-push
mvn validatecannot run the audit on a clean runner. The audit mojo resolves test-scope dependencies, which needparallel-consumer-core:jar:tests; against a repo without the PC snapshots it dies at the second module. The job runstest-compile.Also worth recording:
ossindex.authIdis a plugin user property, so-Dossindex.authId=ossindexbehaves identically with or without #259's pom changes. There is no ordering dependency between these two PRs.Reviewer allowlist: granted by pattern, so the grant can precede the script
bin/check-*.shandbin/test-check-*.share now granted to the review agent as patterns, in both spellings and in bothclaude-code-review-dispatch.ymlandclaude.yml, replacing per-script enumeration.git rev-parseis granted too, so "check which tree you are standing in" is a real instruction rather than advice the reviewer cannot follow.The reason is not convenience. A grant a PR adds never applies to that PR, on either route - the action runs the default branch's copy of the workflow - so a per-script grant has to land in an earlier PR than the script it grants. Nobody keeps that discipline: four check scripts arrived with this PR, all four granted here, and its own reviewer could run none of them. A pattern makes the grant arrive with the convention instead, once, ahead of every script that follows it.
This is the one deliberate widening on the branch, and it is a security judgement, so it is stated plainly. The pattern adds no capability a PR does not already have: the checkout on the dispatch route is
refs/pull/<n>/head, so every already-granted script is the PR's copy -bin/check-copyright-headers.shhas been granted for months and is whatever the branch says it is - andBash(./mvnw:*)already runs the branch's pom. It is a fifth door into a room with four open. The real exposure - PR code executing besideCLAUDE_CODE_OAUTH_TOKEN- predates this and is tracked indocs/inflight/ci-review-agent.md. Both routes are equally in scope:claude-code-actionchecks the PR out on the comment route too (setupBranch), including an explicit fork case, which is why the fork-head withholding of execution tools is load-bearing today rather than belt-and-braces.The new hazard it creates is recorded, because nothing else records it: the naming convention is now a permission boundary. Granting used to be a deliberate act; now naming a script
bin/check-foo.shgrants it. So a script that writes, publishes or deploys must not carry that prefix - the pattern was scoped to two prefixes precisely to keepdeploy.sh,chaos-test.shandsoak-test.shout, and a misnamed script defeats that scoping silently.Documentation, and the records this branch falsified
Several docs asserted things this branch made untrue. Each is corrected where the statement is owned, rather than in a second copy:
bin/AGENTS.mdshrinks by 14 lines and now carries only the footgun reachable from that directory - the naming rule above. The two-boundaries explanation, the manual-grant mechanics and the grant-lands-late rule move todocs/ci.md→ "Editing the reviewer", canonical for the review system per ci(review): make the reviewer run on both routes, take the write token out of it, and ask the gate a simpler question #287. Thetest-check-review-posted.shincident narrative is dropped; it argued a rule the convention now enforces.docs/solutions/workflow-issues/a-check-that-reports-success-without-having-run.mdstill documented findings as exit 0, reporting-only - the opposite of what shipped, in the record the scripts themselves cite.docs/inflight/ci-ossindex-audit-dead.mdis retired (git rm), because the gap it tracked is what this PR closes. Its durable half - the scanner's measured error profile, and how to reproduce a run locally - moves todocs/solutions/security-issues/oss-index-reports-need-reading-before-acting-2026-08-12.md. All six inbound references re-pointed.docs/inflight/ci-ossindex-lane-reassessment.mdtold a reader to re-enable managed submission after v6, two paragraphs before proving it cannot work whatever is released.docs/data/testing-evidence.yamlgains published entries for both new gates, andstatic-analysisis narrowed to what it actually scans.Defect-class sweep
Class: a CI check that reports success without having run. Checked every
continue-on-error,|| true,fail-on-*and skip-guard in.github/workflows/. All are deliberate and documented. The two prior instances of this class are already guarded - the review job bybin/check-review-posted.sh, andbin/check-copyright-headers.sh's shallow-clone silent-skip byCOPYRIGHT_CHECK_REQUIRE_FORK_POINT: "1". No unguarded instances found.A second sweep, for the class a guard whose invariant is satisfiable without meeting it: review found
<!-- -->above an excluded id satisfied "every id carries a rationale". Fixed, with three self-test cases confirmed red against the pre-fix guard. Searched the sibling guards for the same shape - accepting a token's presence as evidence of its content - and found none.Checklist
docs/ci.mdgains the audit lane, the scheduled-build amendment and the allowlist section;bin/AGENTS.mdnarrowed to the naming rule; the durable audit record corrected; one inflight note retired and one corrected;docs/data/testing-evidence.yamlpublishes both gates. See "Documentation" above.bin/test-check-ossindex-audit.shandbin/test-check-cve-exclusions.sh, each run before the guard it protects. The exclusion suite has a matched control pair differing by one day either side of the 90-day limit, so a pass on one and a fail on the other shows the expiry rule is what decides.permissions: contents: readonly; job summary chosen over a PR comment specifically to avoidpull-requests: write. Fork and Dependabot PRs skipped - no credentials either way. Credentials viasetup-javaserver-id/server-username/server-password, the same patternpublish.ymluses. The reviewer-allowlist widening is argued explicitly above rather than left implicit.Still needs to be on master to confirm
Verifiable only from the default branch: the
schedule:trigger (cron fires only there), and the Dependabot skip expression against a real Dependabot PR. The PR-triggered path, the credential preflight,setup-java'ssettings.xmloutput, the repo secrets, cold-cache runtime against the 20-minute timeout and$GITHUB_STEP_SUMMARYrendering are all exercised bydeps: whole-tree CVE scanon this PR's own runs. For a CI-side negative control, pointOSSINDEX_TOKENat a bad value and dispatch - expect red with "OSS Index audit did not run".