Finding
skillspector.nodes.report.reported_findings is now a one-line delegation to
skillspector.suppression.effective_findings. Two names for one answer survive the fix in #130,
and the fork keeps both.
$ grep -n 'def reported_findings' -A20 src/skillspector/nodes/report.py
1506:def reported_findings(result: Mapping[str, object]) -> list[Finding]:
"""The fork's name for :func:`skillspector.suppression.effective_findings`.
…
The name stays because five CLI call sites and the MCP server read findings
through it, and because the report node is where a reader looks for what a
report contains. …
"""
1523: return effective_findings(result)
The six call sites, all of them the fork's own:
| Site |
What it counts |
cli.py:351 |
the advisory (unscored-rule) count |
cli.py:2199 |
_combined_skill_entry's finding_count |
cli.py:2569 |
a multi-skill summary row |
cli.py:3005 |
a summary table |
cli.py:3166 |
a summary table |
mcp_server.py:144 |
the MCP tool's verdict payload |
Reason
#130 found the two functions disagreeing on the raw-findings fallback: the fork's subtracted
suppressed_findings, upstream's deliberately did not. Upstream's semantics won, so the fork's
function became a delegation rather than a second implementation — and the divergence the issue was
filed about is gone.
What is left is a naming duplicate, and #130's brief sequenced its removal after the defect
fix on purpose. Deleting a public-ish name and repointing six call sites is a mechanical change with
its own review surface; folding it into the change that fixed a live over-count would have buried the
part a reviewer needs to check.
The docstring's own reason for keeping the name — "the report node is where a reader looks for what
a report contains" — is the argument to weigh here rather than assume. It is not obviously wrong.
Impact
No user-visible behaviour. Both names return the same list, and a test pins that. The cost is to
readers and to future contributors: two names for one selection is exactly the shape that let three
consumers disagree with the report in the first place (#119, #122, #130), and the invariant
effective_findings' docstring states — "exactly one function in the package decides which findings
a finished scan reports" — is true of the behaviour but not of the vocabulary.
Gotchas
mcp_server.py imports it, so this is not confined to cli.py. Check whether the mcp extra's
own tests import the name too.
- The docstring at the delegation site carries reasoning that must survive the deletion, not just
the call. It records what #130 measured and what upstream 73dd1f1 changed about the keys it
reads. effective_findings already carries most of it; diff the two before removing either.
CONTEXT.md may name one of the two spellings. A rename that leaves the glossary behind is a
vocabulary defect the guard tests will not catch, because neither name is a framework vocabulary
spelling.
- This is not a fingerprint or snapshot change, so the usual "prove a committed baseline still
suppresses what it suppressed" check does not apply — but a test that imports reported_findings
by name will fail loudly, which is the point.
- Do not "simplify" by making
effective_findings import from report: report already imports
from suppression, and reversing it is a cycle.
Options
- Delete
reported_findings, repoint the six call sites at effective_findings. One name, and
the vocabulary matches the invariant the docstring claims. Cost: six edits plus the import lines,
and any external caller of the fork's name breaks — this fork is installed, so that is a real if
small surface. Run this first if the name is judged to have no independent value.
- Keep it and mark it explicitly as an alias — a module-level assignment rather than a function,
with the reasoning consolidated in one place. Cheapest, keeps the readers' entry point at the
report node, and stops the two docstrings drifting apart. Loses nothing except the single-name
goal.
- Delete it the other way round — move the implementation to
report and have suppression
delegate. Rejected on sight: report imports suppression, so this inverts the dependency and
the selection would live in the module that renders rather than the one that decides.
- Do nothing. The current state. Behaviour is correct and the duplication is documented at both
sites.
I would settle 1 against 2 by asking whether anything outside this repository calls
reported_findings. If nothing does, 1; if the fork's own MCP consumers might, 2 is the honest
compromise.
Provenance
Deferred deliberately during #130's implementation and recorded in the commit body there. Filed
per CLAUDE.md's standing rule that an accepted deferral belongs in the tracker rather than in a
commit message a reader has to go looking for.
Finding
skillspector.nodes.report.reported_findingsis now a one-line delegation toskillspector.suppression.effective_findings. Two names for one answer survive the fix in#130,and the fork keeps both.
The six call sites, all of them the fork's own:
cli.py:351cli.py:2199_combined_skill_entry'sfinding_countcli.py:2569cli.py:3005cli.py:3166mcp_server.py:144Reason
#130found the two functions disagreeing on the raw-findingsfallback: the fork's subtractedsuppressed_findings, upstream's deliberately did not. Upstream's semantics won, so the fork'sfunction became a delegation rather than a second implementation — and the divergence the issue was
filed about is gone.
What is left is a naming duplicate, and
#130's brief sequenced its removal after the defectfix on purpose. Deleting a public-ish name and repointing six call sites is a mechanical change with
its own review surface; folding it into the change that fixed a live over-count would have buried the
part a reviewer needs to check.
The docstring's own reason for keeping the name — "the report node is where a reader looks for what
a report contains" — is the argument to weigh here rather than assume. It is not obviously wrong.
Impact
No user-visible behaviour. Both names return the same list, and a test pins that. The cost is to
readers and to future contributors: two names for one selection is exactly the shape that let three
consumers disagree with the report in the first place (
#119,#122,#130), and the invarianteffective_findings' docstring states — "exactly one function in the package decides which findingsa finished scan reports" — is true of the behaviour but not of the vocabulary.
Gotchas
mcp_server.pyimports it, so this is not confined tocli.py. Check whether themcpextra'sown tests import the name too.
the call. It records what
#130measured and what upstream73dd1f1changed about the keys itreads.
effective_findingsalready carries most of it; diff the two before removing either.CONTEXT.mdmay name one of the two spellings. A rename that leaves the glossary behind is avocabulary defect the guard tests will not catch, because neither name is a framework vocabulary
spelling.
suppresses what it suppressed" check does not apply — but a test that imports
reported_findingsby name will fail loudly, which is the point.
effective_findingsimport fromreport:reportalready importsfrom
suppression, and reversing it is a cycle.Options
reported_findings, repoint the six call sites ateffective_findings. One name, andthe vocabulary matches the invariant the docstring claims. Cost: six edits plus the import lines,
and any external caller of the fork's name breaks — this fork is installed, so that is a real if
small surface. Run this first if the name is judged to have no independent value.
with the reasoning consolidated in one place. Cheapest, keeps the readers' entry point at the
report node, and stops the two docstrings drifting apart. Loses nothing except the single-name
goal.
reportand havesuppressiondelegate. Rejected on sight:
reportimportssuppression, so this inverts the dependency andthe selection would live in the module that renders rather than the one that decides.
sites.
I would settle 1 against 2 by asking whether anything outside this repository calls
reported_findings. If nothing does, 1; if the fork's own MCP consumers might, 2 is the honestcompromise.
Provenance
Deferred deliberately during
#130's implementation and recorded in the commit body there. Filedper
CLAUDE.md's standing rule that an accepted deferral belongs in the tracker rather than in acommit message a reader has to go looking for.