triage: accept findings on the record, and gate on the rest - #19
Merged
Conversation
Closes the oldest item in Known gaps. The two halves ship together because either one alone is worse than neither. --fail-on by itself turns a single unfixable transitive advisory into a build that is red every week forever, since the scheduled scan exists precisely to catch advisories published against code that has not changed. A job that is always red gets `|| true` appended or gets deleted, and both outcomes are what the exit-code design was built to prevent. So gating only becomes safe once there is a way to say "we have looked at this one". .icebergsca.toml records that decision. Every constraint in core/triage.py follows from this being the one feature whose job is to remove findings, which makes it the one most able to undermine the rest of the tool: - A suppressed finding stays in ScanReport.findings, marked. It is never filtered out, so table.py's empty-findings branch cannot print "no known vulnerabilities" for a scan where everything was accepted. There is a test asserting exactly that, and it fails if the filtering is ever moved upstream. - A reason is required, and an expiry always exists, defaulting to 90 days rather than being optional, so no entry becomes permanent by accident. Past it the finding returns; expiry never fails a scan by itself. - Matching is exact on advisory-or-alias plus package, so a rule can never grow to swallow a second, unrelated finding. The negative tests are the point. - parse_ignore_file is strict where every other parser here is forgiving. A third-party manifest is not ours to police; the file that decides which vulnerabilities you stop seeing is a different case, so an unknown key is a ConfigError rather than a silent skip. - A rule that matches nothing, or has expired, becomes a warning — a typo suppresses nothing and looks identical to a rule that is working. Gating checks completeness before severity, so it can never pass a scan that failed to look anything up, and counts only unsuppressed findings. It exits 3 rather than reusing 1: a pipeline that cannot tell "the scanner broke" from "the scanner found something" ends up treating both as noise. SARIF uses its own suppressions property, so GitHub shows an accepted finding as dismissed rather than closing it as fixed. CycloneDX gets analysis.detail only — state and justification are closed enums of security claims that a free-text reason cannot back, and asserting one would invent an analysis nobody performed. Verified end to end against a local OSV stub: a finding appears, is accepted by a rule written as the CVE where OSV answered with a GHSA, stays visible in every format while dropping out of the counts and the gate, and returns with a warning once the entry expires. JSON schema moves to 1.1 — additive only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01By6nqX2dcU351oZn6y6Xoq
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
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.
Closes #11 — the oldest item in Known gaps.
The two halves ship together because either one alone is worse than neither.
--fail-onby itself turns a single unfixable transitive advisory into a build that is red every week forever, since the scheduled scan exists precisely to catch advisories published against code that has not changed. A job that is always red gets|| trueappended, or gets deleted — both the outcomes the exit-code design was built to prevent. Gating only becomes safe once there is a way to say "we have looked at this one".The ignore file
.icebergsca.tomlin the project root, overridable with--ignore-file:Every constraint in
core/triage.pyfollows from this being the one feature whose job is to remove findings, which makes it the one most able to undermine the rest of the tool:ScanReport.findings, marked. It is never filtered out, sotable.py's empty-findings branch cannot print "no known vulnerabilities" for a scan where everything was accepted.test_a_fully_suppressed_report_never_reads_as_cleanasserts exactly that, and fails if the filtering is ever moved upstream — I checked by making that change.GHSA-aaaadoes not reachGHSA-aaaa-bbbb, and a rule for one package does not touch the same CVE on another. The negative tests are the point.parse_ignore_fileis strict where every other parser here is forgiving. A third-party manifest is not ours to police and skipping what we cannot read is right there. The file that decides which vulnerabilities you stop seeing is the opposite case, so an unknown key is aConfigError, not a silent skip.Gating
--fail-on <severity>checks completeness before severity, so it can never pass a scan that failed to look anything up, and counts only unsuppressed findings.It exits 3 rather than reusing 1. A pipeline that cannot tell "the scanner broke" from "the scanner found something" ends up treating both as noise, which is the same failure
|| truerepresents.Output
SARIF uses its own
suppressionsproperty, so GitHub shows an accepted finding as dismissed rather than closing it as fixed, and the rule entry survives. JSON gainsfindings[].suppression,summary.suppressed, and a top-levelsuppressedaudit trail. The table dims accepted rows, counts them separately, and explains the marker. CSV appends two columns.Verification
api.osv.devis blocked from this environment, so the end-to-end run used a local OSV stub with only the upstream URL redirected — the rest of the CLI path is real:--fail-on highexits 3.The incomplete-scan case was checked against the actually blocked endpoint:
--fail-on criticalexits 1, not 0 and not 3 — the false-clean this feature could most easily have introduced.ruff check,ruff format --check,mypy(strict) and 454 tests all pass, in 2.2s.Judgement calls worth a look
suppressedarray lists the rules, not a second copy of the findings. The issue asked for an array; duplicating finding objects across two arrays invites double-counting, and lets a consumer readfindingsalone and never learn some were accepted. Per-finding state lives onfindings[].suppression, where it cannot be missed; the array carries what is found nowhere else — which entry accepted what.analysis.detailonly, nostate.not_affectedis a security claim the tool cannot back from a free-text reason, andimpactAnalysisJustificationis a closed enum that free text does not fit. Recorded in Known gaps, with an optional validatedstatekey on the entry as the natural follow-up.1.1. Additive only, so pinned consumers keep working.summary.findingsandby_severitynow count active findings, which is unchanged for any scan with no ignore file.Generated by Claude Code