Repository navigation
ci(scan): the CVE scan is its own check again - red on a finding, and not required - #489
Conversation
…ls, and the schedule still gates A new OSS Index advisory against an unchanged dependency (CVE-2026-59296 on micrometer-core) turned every open PR red in the same hour, on the required scan: repo context, in branches that had not touched a dependency. A required check that goes red for something no PR can fix blocks the release burn-down rather than protecting it. Owner decision, 2026-09-09: findings are advisory on the PR lane. What changes: the whole-tree CVE scan step now captures the checker's verdict instead of dying on it. Exit 1, the scan could not be proven to have run, still fails the step and the job - a scan that did not happen is not degraded into a pass. Exit 2, the tree has a finding, is handed to a new advisory step that renders red in the step list, annotates the run with a warning, and carries continue-on-error on the STEP so the required context stays green; a job-level flag would have made a broken scan green too, which is the mutation lane's precedent and the reason it is step-level there. What does not change: dependency-audit.yml, the scheduled and dispatched lane, still fails on a finding; the did-it-actually-scan guard, the two distinct exit codes, and the excludeVulnerabilityIds escape hatch with its dated retirement conditions are all as they were. The reasoning this overrides - that a solo maintainer reads the PR gate and not a scheduled alert - is kept in the checker's header and named in the workflow comment, so the next reader sees a decision, not an oversight. The finding itself is not triaged here: micrometer-core is held on the 1.13 line for family alignment with the prometheus registry (see the pom comment), so an exclusion with a retirement condition or a family bump is a separate change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xoi3HYae8pjsEatuNFKieD
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #489 +/- ##
============================================
+ Coverage 82.50% 83.02% +0.52%
- Complexity 1563 1570 +7
============================================
Files 96 96
Lines 5383 5383
Branches 537 537
============================================
+ Hits 4441 4469 +28
+ Misses 747 722 -25
+ Partials 195 192 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🟢 Throughput — OKThis branch measured about 7% slower than master, on the one test this measures. That is INSIDE this test's own run-to-run spread of about 17%, so read it as a reading and not as a result - re-running the same commit moves it by about as much.
Allowable range 🟢 ≥ 0.70 · 🟡 0.50–0.70 (about a 30% loss) · 🔴 < 0.50 (about a 50% loss) What the numbers mean, and what they cannot tell youThe one that gets misread. Why a shape and not a rate. A rate depends on which runner you drew. A shape does not: every test here processes a fixed number of records, so a runner twice as slow doubles the subject and the controls together and leaves their ratio alone. That is the whole trick, and it is why the reported rate is shown last and labelled as this machine only. Reading the comparison. By conservation, not by correction. Every test in this lane processes a fixed number of records, so within one run the ratio of one test's time to another's is invariant under machine speed — a runner twice as slow doubles both terms and leaves the ratio alone. There is no machine-index correction to be wrong, because nothing needed correcting. Per-method times, not class times. A class time is Reference is the median of 10 recent What this still cannot do. It removes machine-to-machine variance. It does not remove this test's own run-to-run variance, measured at about 30% on a single unchanged commit while its controls stayed within 5%. That is a property of the test, not of the comparison, and no arithmetic here can touch it — which is why the reference is a median and the bounds are deliberately coarse. 🟡 means look at this; only 🔴 is outside the measured spread. Runs used: a055248, 65e11e3, e05b399, ee3d8a9, 890aa53, fd3f913, 21e8728, 9302a89, c2d35c0, ad482ec Since the previous push: ratio 1.172 -> 0.927, share 1.571 -> 1.987, rate 75437 -> 58981 (-21.8%). One push of difference sits inside this test's measured spread - read it as movement, not as a result. Updated for |
|
|
@claude review this |
|
Claude finished @astubbs's task in 4m 0s —— View job Review
What I checked mechanically, tracing execution rather than trusting the description:
One thing worth reconciling, not blocking: The PR body cites "the mutation lane" only for the step-vs-job-level choice of Everything else checks out: the exit-code plumbing is correct, the required-check-stays-green mechanism ( |
…ot required The first cut of this change kept the CVE scan inside the required scan: repo job and downgraded a finding to a continue-on-error step. Its own run showed why that is the wrong shape: the step exited 2, the check ticked green, and nothing in the PR's checks list said a finding existed. A GitHub job emits one check run, so "red but not blocking" cannot be a step of a required job at all. So the CVE lane is a job again - cve, producing the deps: whole-tree CVE scan context on every PR - with the checker's verdict failing it exactly as before, and that context deliberately absent from the master ruleset. The finding shows as a red check the maintainer sees; the merge is not held for something no PR can fix. The job holds contents: read only, since PR-authored build code runs in it, and keeps the !cancelled() edge on prepare-deps so a cache blip degrades it to a colder cache rather than deleting the signal. scan: repo loses its Maven, its cache edge and fifteen minutes of timeout, and its header no longer describes CVE steps it does not have. docs/ci.md's not-required table gains the row and its reason, so nobody re-adds the context as an oversight; the ruleset-edits note records the second un-fold beside the mutation lane's; the checker's header says where each verdict lands. The scheduled lane is unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xoi3HYae8pjsEatuNFKieD
astubbs
changed the title
|
Thanks - the one point worth reconciling was right, and the second cut ( The review read the first cut, where a So the CVE scan is its own |
… 1.16.7 (#493) The whole-tree CVE scan reported CVE-2026-59296 against io.micrometer:micrometer-core:1.13.15, which parallel-consumer-core declares at compile scope. It is real - Spring/VMware assigned, published 2026-08-21, CWE-117 - and it is not reachable here, so it joins the standing excludeVulnerabilityIds group the other two micrometer advisories already sit in. Why an unchanged tree went red. Each of the three micrometer advisory pages carries the same history line: "2026-09-07: Updated affected version ranges to include unsupported 1.10.x - 1.13.x versions". The 1.13 line was out of support and simply unenumerated before that; the code was always affected. This is precisely the case the scheduled lane exists for. The same finding is what prompted #489, which moved the scan out of the required scan: repo context into its own non-required job - it turned every open PR red at once, in branches that had touched no dependency. Why it is unreachable, on two legs that fail independently. The advisory needs all three of: a vulnerable emitter, the Datadog/Etsy StatsD registry or LoggingMeterRegistry; newline-delimited metric output; and meters instrumented with user-controlled, unvalidated input. The emitter is absent - micrometer-registry-statsd is not a dependency of any module and LoggingMeterRegistry is never constructed anywhere in the tree; main code builds only CompositeMeterRegistry plus PrometheusMeterRegistry in example-metrics. The tainted input is absent too - every tag value PC emits is structurally incapable of carrying a line terminator: Kafka topic names, which the broker validates to [a-zA-Z0-9._-]; partition numbers through String.valueOf(int); an offset-encoding enum name; and a pcinstance tag that is a caller string or a UUID. Record keys, headers and payloads never reach tag data. Stated rather than glossed: a caller supplies the registry and commonTags, so an application that hands PC a StatsD registry AND passes attacker-controlled strings as commonTags is vulnerable - but that is the advisory's own antipattern, committed in the application, and 1.16.7 fixes it there too. The correction is worth more than the new id. Reading the three fix tables side by side showed the recorded "minimum public fix is 1.15.12" is true only for CVE-2026-40984. Spring publishes some of these patches to Central and some only to the commercial repository: for CVE-2026-59295 and CVE-2026-59296 the OSS fixes are 1.16.7 and 1.17.1, while 1.15.13, 1.14.17 and 1.13.19 are Enterprise Support Only, and 1.15.0-1.15.12 is inside both advisories' affected range. Central stops at 1.13.15 / 1.14.14 / 1.15.12, all affected. So the migration target moves from "1.15.x" to "1.16.7 or later", and the pom, deps-cve-backlog.md and deps-deferred-majors.md all said 1.15.12. Had that gone uncorrected, the prometheus migration would have landed on 1.15.12 and left two of the three advisories standing while reading as done. A fourth stale micrometer version is fixed for the same reason: the version property's own comment said "Held at 1.13.0" while the value was 1.13.15, and it now names the 1.13 LINE, which is what the prometheus pin actually constrains. Routes rejected, in the preference order the triage asks for. A patch bump on the 1.13 line does not exist: 1.13.19 is Enterprise Support Only and Central's 1.13 line ends at 1.13.15. A family bump to 1.16.7 is now the only fix, and the recorded blocker still stands - the io.micrometer.prometheus to io.micrometer.prometheusmetrics rename breaks examples/metrics/CoreApp.java, and micrometer-registry-prometheus is pinned to 1.12.x there. It was rejected rather than merely deferred: micrometer-core is a compile-scope dependency of the published parallel-consumer-core, so moving it three minors is a change consumers see, and pairing that with an example migration inside a branch triaging an unreachable advisory would produce a diff nobody would review as either. The entry is standing rather than TEMPORARY-SINCE deliberately - 1.16.7 was on Central before this triage, so nothing upstream is awaited; what blocks us is our own deferred migration, and dating it would expire the marker for a reason it cannot describe. Proved with a control arm rather than a green run. With the entry, the workflow's own invocation against real credentials on a cleared client cache gives 9 module reports, 211 components, 0 vulnerable, 7 excluded, exit 0. Re-running with only CVE-2026-59296 removed from the pom - a re-run, because the guard reads the exclusions the plugin resolved into the exported report and not the pom - gives exactly the predicted flip: excluded=6, one vulnerable component, top CVSS 6.3, exit 2, naming micrometer-core across all seven modules. CI agrees from the other side: deps: whole-tree CVE scan is green on this PR. The defect-class sweep changed the migration target. Asking whether any other dependency held back by a family-alignment constraint carries open advisories turned up one row - this one - but reading the published poms across the micrometer lines answered a question the exclusion list had left open-ended. The LatencyUtils entry retires when "micrometer-core drops the dependency", and micrometer-core:1.17.0 already did: still declared in 1.16.7, gone in 1.17.0 and 1.17.1 (HdrHistogram is not dropped and stays). So the three micrometer advisories alone would settle for 1.16.7, but going one line further to 1.17.x takes CVE-2026-82596 with them for the same prometheus migration work - four ids rather than three. deps-deferred-majors.md already named 1.17.x; this is the reason to hold that target. Nothing was ever going to tell us, since CVE-2026-82596 is VulDB-assigned and not in GHSA, so no Dependabot alert can fire. Every other held-back row was checked against the raw unfiltered report and dismissed: the jackson pair is a deliberate anti-alignment and its one advisory is still the same over-broad range, wiremock/byte-buddy appear in the report not at all, and kafka, vertx, mutiny, testcontainers and junit are held by baselines rather than by intra-family alignment. One CI flake was recorded rather than re-run into silence. ManagedPCInstanceLifecycleTest.rapidToggleShouldNotCreateDuplicateInstances went red at repetition 1 of 5 with consumeCount 0, the exact signature the register already carried three sightings of, with repetitions 2 through 5 green on the same commit; it passed on the next run with no retry and no change to the test. The fourth sighting earns its place twice: this branch cannot have caused it provably rather than arguably, since one excludeVulnerabilityId element in a validate-phase plugin that is skipped by default cannot reach a Kafka integration test and the rest of the diff is markdown - where previous sightings had to be cleared by reading the branch's Java - and it is the second of four sightings with no .github/ change, which drops the "branch just took a lane change" condition the first two shared and leaves the fixed-sleep-under-load defect carrying all four. Not quarantined: four sightings is still not a rate. Class-file majors re-checked on the new targets: 1.16.7 and 1.17.1 are both 52, so the Java 8 baseline is not a second blocker. No dependency version changed here - the diff is a plugin configuration id and prose. Co-authored-by: Claude Opus (1M context) <noreply@anthropic.com>
…producer-fencing-brainstorm Brings master as of #496 up through the stack, for the CVE exclusion (#493) and the scan split (#489) that turned the required check red on every rung. Two docs conflicts, resolved by hand: - The flake ledger keeps every row: master's updated ManagedPCInstanceLifecycleTest count and this branch's three rows master never carried. - docs/refactoring.md: master added a bullet asking this branch to swap its replay loop to register-then-publish when it lands. The swap is already on this branch (c0a1ef3, pinned by the completion-on-publish case in PartitionStateAbortedTransactionReplayTest), so the bullet is retired here rather than carried to master as a stale instruction. The two bullets beside it stand. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VrpH51xNDodaajE4P2nhFg
1 participant
Serves the v6 burn-down on #475 (no single issue).
Description
A new OSS Index advisory against an unchanged dependency (
CVE-2026-59296onmicrometer-core) turned every open PR red in the same hour on the requiredscan: repocontext - #480 and #488 among them, neither of which touched a dependency. A required check that goes red for something no PR can fix blocks the release burn-down rather than protecting it. Owner decision, 2026-09-09: a CVE finding must turn its check red, and must not block the merge.What changes. The whole-tree CVE scan is its own job again -
cve, producing thedeps: whole-tree CVE scancheck on every PR - and that context is deliberately not in the master ruleset. The checker's verdict fails the job exactly as before: exit 1 (the scan could not be proven to have run) and exit 2 (the tree has a finding) both go red. The red shows in the PR's checks list where the maintainer sees it; the merge is not held.Why a job, not a step. The first cut of this PR kept the scan inside
scan: repoand downgraded a finding to acontinue-on-errorstep. Its own run showed the problem: the step exited 2, the required check ticked green, and nothing in the checks list said a finding existed. A GitHub job emits one check run, so "red but not blocking" cannot live inside a required job at all.Knock-on tidy-ups.
scan: repoloses its Maven setup, itsneeds: prepare-depsedge and fifteen minutes of timeout, and its header no longer describes CVE steps it does not have. The new job holdscontents: readonly, since PR-authored build code runs in it, and keeps!cancelled()on the cache edge so a transient cache failure degrades it to a colder cache rather than deleting the signal.What does not change.
dependency-audit.yml(schedule and dispatch) still fails on a finding. The did-it-actually-scan guard, the two distinct exit codes, theDossindex.skip=falsetwo-places invariant, and theexcludeVulnerabilityIdsescape hatch with its dated retirement conditions are untouched.The reasoning this overrides is recorded, not deleted.
bin/check-ossindex-audit.sh's header argues that on a tree whose known debt is excluded with reasons, a finding is by construction unlooked-at, and that the PR gate is the only channel a solo maintainer reliably reads. Both still hold; the job's comment names them as overridden, the header says where each verdict now lands,docs/ci.md's not-required table carries the row so nobody re-adds the context as an oversight, anddocs/inflight/ci-fewer-jobs-ruleset-edits.mdrecords this un-fold beside the mutation lane's.Not done here: the finding itself.
micrometer-coreis held on the 1.13 line for family alignment with the prometheus registry (pom comment), so an exclusion with a retirement condition or a family bump is a separate change.Verification. The workflow parses;
bin/check-all.shgreen. The live proof is this PR's own run: the finding is present on master, sodeps: whole-tree CVE scanshould appear as a red check whilescan: repoand the merge state stay green.Checklist
docs/ci.md(lane description, dependency-audit section, not-required table), the checker's header, the ruleset-edits notedocs/features/- N/A - CI lane behaviour, no library featuredocs/inflight/working note (pr-/branch-) started at the PR's first commit - N/A - a CI change fully described by the commit bodies and the workflow comment; the existing ruleset-edits note carries the durable recordce-simplifyandce-code-reviewlocally - N/A - config-only change;@claude review thiscan be asked on the PR if wanted🤖 Generated with Claude Code
https://claude.ai/code/session_01Xoi3HYae8pjsEatuNFKieD