Repository navigation
Three readers of one contract: the transitive finding count over-reports, and two named readers disagree #130
Description
Finding
Three readers now answer the same question — "which findings does a finished scan's report
actually carry?" — with different code, one behavioural difference between two of them, and an
outright over-count in the third. This was filed describing only the first two; triage measured the
rest and widened it.
Two are named functions:
reported_findings,src/skillspector/nodes/report.py:1513(fork-authored, Advisory spec-conformance findings render asConfidence: 0%instead of saying they are not scored #119/A baseline is silently bound to the --spec-checks mode it was taken in; the document records no mode and scan cannot warn #122 era).effective_findings,src/skillspector/suppression.py:507(upstream,33b3930).
Both select filtered_findings by presence rather than truthiness, and both subtract the
suppressed_findings partition. They diverge on exactly one input: when filtered_findings is
absent or malformed and the raw findings list is used instead.
reported_findingsfalls back tofindingsand still subtractssuppressed_findings.effective_findingsreturns the raw list without subtracting, and its docstring says why:
"raw findings are not the population that producedsuppressed_findings".
Reproduce with a result mapping that has no filtered_findings key, a findings list of one
Finding, and a suppressed_findings entry wrapping that same Finding:
reported_findings -> []
effective_findings -> ['TM1']
The third reader is unnamed and inline: the transitive merge path in the CLI computes
transitive_finding_count over _coerce_findings_list(report_result.get("filtered_findings")) and
subtracts nothing. filtered_findings is the pre-suppression partition, so every
baseline-suppressed transitive finding is counted into a number the report publishes:
transitive_finding_count as the merge path computes it : 2
what the report actually carries : 1
Reachable from an ordinary invocation — --transitive with a --baseline that accepts a transitive
finding — not only from a hand-assembled state.
Call-site inventory, measured rather than recalled: reported_findings has six call sites (five
in the CLI, one in the MCP server), effective_findings has one — _multi_skill_public_record_count,
in the same CLI module. So a single recursive scan counts its findings one way for the summary
table's finding_count and another way for its public-record budget.
Reason
The two were written independently for the same defect, on the two sides of a fork that had not
synced. The 2.9.6 upstream sync (#TBD) merged both into the tree and kept both, because collapsing
them is a behaviour decision and the merge's job was to be attributable, not to redesign. The four
reported_findings call sites (CLI summary tables, skillspector baseline, the MCP verdict, the
repository-scan digest) were left pointing at the fork's function so the merge's snapshot ledger
stayed readable.
Impact
The two named functions differ only on a branch a real scan cannot reach: the report node always
writes filtered_findings, so the fallback is exercised by unit tests alone.
The third reader is a live defect. transitive_finding_count reaches the JSON report and the
recursive combined payload, so a consumer gating on it sees more transitive findings than the report
lists, by exactly the number a baseline suppressed. Silent — the count and the findings array
disagree with nothing to flag it.
The real cost is drift. effective_findings is upstream's and will keep evolving; reported_findings
is the fork's and is what the CLI and the MCP server call. The next upstream change to the
selection semantics lands in one of them and not the other, and the two readers disagree exactly the
way reported_findings' own docstring warns about: "three consumers reading graph state three ways
is how they came to disagree with the report in the first place."
Gotchas
- Deleting
reported_findingsand repointing its six call sites is not a no-op: it changes the
fallback branch's answer, and the CLI stream tests have five cases that go through it. - The third reader cannot simply call the survivor: it filters on
source_urlafter selecting, so
the fix is to select first and filter second, not to swap one expression for another. - The fork's function carries a long docstring that is the record of Advisory spec-conformance findings render as
Confidence: 0%instead of saying they are not scored #119/A baseline is silently bound to the --spec-checks mode it was taken in; the document records no mode and scan cannot warn #122. Whichever function
survives has to carry that reasoning, or the next reader re-derives it. - A Behavior Snapshot will not catch a regression here.
filtered_findingsis always present in a
real scan, so the snapshot corpus never exercises the branch the two disagree on.
Options
- Fix the over-count first, on its own: route the transitive count through whichever reader
survives, selecting before filtering onsource_url. It is the only part with user-visible
impact and it does not depend on the naming question below. - Make
reported_findingsdelegate toeffective_findingsand keep the fork's name and
docstring. Smallest diff; adopts upstream's fallback semantics, so the five stream tests have to
be re-read first. Cost: ~30 minutes. - Delete
reported_findings, repoint the six call sites. Cleaner, but loses the fork's
docstring and touches the MCP server, whose comment names the fork's function by name. - Keep both and add a test asserting they agree on every input the graph can produce. Cheapest
to write and the weakest: it documents the duplication rather than removing it.
Option 0 first — it is the defect a user can observe. Then 1, which settles the semantics question
with the smallest reversible change; 2 is the cleanup that follows once nothing depends on the
fork's name.
Activity
[-]Two readers of one contract: reported_findings and effective_findings diverge on the raw-findings fallback[/-][+]Three readers of one contract: the transitive finding count over-reports, and two named readers disagree[/+]rodrigorjsf commentedon Aug 22, 2026
Agent Brief
Category: bug
Summary: One reader decides which findings a finished scan reports; today three do, and the one the transitive count uses over-reports by the baseline-suppressed set
Current behavior:
Three expressions answer "which findings does a finished scan's report carry", and they do not agree.
reported_findings(fork-authored, in the report node) — selectsfiltered_findingsby presence, subtracts thesuppressed_findingspartition, and also subtracts when it falls back to the rawfindingslist.effective_findings(upstream, in the suppression module) — the same selection, but its raw-findings fallback returns the list unsubtracted, deliberately: raw findings are not the population that producedsuppressed_findings.transitive_finding_countover the coercedfiltered_findingslist and subtracts nothing before counting entries carrying asource_url.The first two differ only on a branch a real scan cannot reach, because the report node always writes
filtered_findings. The third is a live defect:filtered_findingsis the pre-suppression partition, so with--transitiveand a--baselinethat accepts a transitive finding,transitive_finding_countexceeds the number of transitive findings the report lists, by exactly the suppressed count. Measured: 2 published against 1 reported.The two named readers are also both live in the CLI at once — the recursive summary table's
finding_countuses one, and the recursive public-record budget uses the other, on the same result.Desired behavior:
One reader owns the selection semantics. Every consumer that counts or lists a finished scan's findings — the CLI summary tables, the recursive public-record budget, the baseline generator, the MCP verdict payload, the repository-scan digest, and the transitive count — goes through it, and agrees with what the report renders.
transitive_finding_countequals the number of findings the report carries that have asource_url. A baseline that suppresses a transitive finding lowers both the count and the list, together.Key interfaces:
reported_findings(result) -> list[Finding]andeffective_findings(result) -> list[Finding]— same signature, same intent. One survives; the other is either deleted or becomes a one-line delegation. Whichever survives keeps the reasoning currently carried inreported_findings' docstring (issues Advisory spec-conformance findings render asConfidence: 0%instead of saying they are not scored #119 and A baseline is silently bound to the --spec-checks mode it was taken in; the document records no mode and scan cannot warn #122), because it is the record of why selection is by presence rather than by truthiness.transitive_finding_count— currently selects and filters in one expression. It must select through the surviving reader first, then filter onsource_url; swapping one expression for another will not work, because the filter is applied to the selection._coerce_findings_list— the defensive coercion around a raw state read. Keep it where a raw state key genuinely has to be read; it is not itself the defect.Acceptance criteria:
filtered_findingsholds one kept and one baseline-suppressed transitive finding,transitive_finding_countis1, not2.finding_countand the recursive public-record budget count the same result identically.make test-unit,make lint,make format-checkpass, andmake update-snapshotsleaves the tree clean.Out of scope:
filtered_findingsorsuppressed_findings. This issue is about how consumers read those two keys, not about the partition itself.suppressed_findingsorfiltered_findingsin graph state.fix(cli): count only the transitive findings the report carries (#141)
rodrigorjsf commentedon Aug 22, 2026
Closed by #141, squash-merged as
e47ec3f. This issue's central measurement no longer reproduces,and that is the first thing a later reader needs.
The premise is falsified
The body states the over-count is "Reachable from an ordinary invocation" and measures "2 published
against 1 reported". That was true when this was filed. It is not true on the current tree: upstream
73dd1f1("Security fixes",NVIDIA/SkillSpector#393) narrowed what the report node writes intofiltered_findingsto the kept side alone, so the state the transitive count reads no longercarries the suppressed findings it was over-counting.
The fix still landed, and the contract test still earns its place — but it has to induce the
pre-
73dd1f1shape through a spy, because that shape is what the transitive merge path hands overtoday even though the report node no longer produces it. So the correct framing is a latent contract
defect held shut by an upstream change, not a bug a user hits this afternoon. Anyone re-deriving the
original repro will fail to reproduce it and should not conclude the fix was unnecessary.
What shipped, in the order this issue asked for
The brief sequenced the over-count fix first and the redundancy second, and that ordering held. The
transitive count now counts only the findings the report carries, proven by a failing test written
first. Separately, exactly one function in the package decides which findings a finished Scan
reports: this issue measured the fork's
reported_findingsand upstream'seffective_findingsdiverging on the raw-
findingsfallback — the fork's subtractedsuppressed_findings, upstream'sdeliberately did not — and upstream's semantics won, per this fork's standing rule that upstream
owns the truth about the core.
Three things review caught that this issue did not anticipate
The acceptance criterion's own test was true of itself. The assertion for the summary/budget
agreement compared
_multi_skill_public_record_count(result) - 1againstlen(reported_findings(result))— and that right-hand side was character-for-character theproduction expression at the call site the criterion names. An adversarial critic proved it by
inlining a raw
filtered_findingsread at_combined_skill_entry'sfinding_count, the exactregression this issue exists to prevent, and watching every case stay green. It now routes through
the entry builder and derives the suppressed record count from the state rather than hard-coding it.
The premise survived in four docstrings, while the commit correcting it claimed there were three
and that all three were done.
A contract document gained a rule the tree does not keep.
docs/DEVELOPMENT.md's newfiltered_findingsrow read "Read it throughsuppression.effective_findings, never directly" —contradicted by three legitimate mid-graph reads — and described only what the report node writes,
while the transitive merge writes the pre-partition merged population into the same key beforehand.
Both ends are now qualified.
Deferred, and filed
reported_findingsis now a one-line delegation, so one selection carries two names across six callsites. Its removal was deliberately sequenced after the defect fix and is #140, which also
records the argument for keeping the name.