Skip to content

reported_findings is now a delegation: one selection, two names, six call sites #140

Description

@rodrigorjsf

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

  1. 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.
  2. 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.
  3. 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.
  4. 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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or requestneeds-triageTriage: issue was just created; untriaged

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions