fix(cli): report the findings that actually drove the risk score

[wernerkasselman-au]

What this fixes

Four call sites select findings with filtered_findings or findings. That
idiom over-reports in two distinct ways, and both are reachable on main
today.

1. The falsy fallback. report returns filtered_findings as a real
list, and an empty one is a real answer: the meta-analyzer filtered every
finding out, or a baseline suppressed them all. The or treats [] as
absent and falls through to the raw pre-filter findings, so a skill that
scores 0 is reported alongside a non-zero finding count, disagreeing with
its own risk score and report body.

2. The unsubtracted partition. report returns filtered_findings as
the full pre-partition set and suppressed_findings as the excluded subset:

active_findings, suppressed = partition_findings(selected_findings, baseline, ...)
findings_for_scoring = deduplicate(active_findings)
risk_score, ... = _compute_risk_score(findings_for_scoring, ...)
sarif_report = _build_sarif(active_findings, suppressed, ...)
return {..., "filtered_findings": selected_findings, "suppressed_findings": suppressed}

Score, dedup, and SARIF all come from active_findings. Counting
filtered_findings counts findings the report itself excluded.

The change

Adds suppression.effective_findings(), the inverse of the existing
partition_findings() and placed next to it, and routes all four sites
through it:

Site	Impact
cli.py recursive summary table	Findings column disagreed with the Score column
cli.py combined recursive JSON	finding_count disagreed with risk_score
cli.py skillspector baseline	Fingerprinted raw findings the scan had already filtered out, writing a baseline for findings it never reported
mcp_server.py scan_skill verdict	Serialises this list straight to a calling agent, so a suppressed finding leaking in tells that agent a skill is dirtier than the score it is gating on

It falls back to the raw findings list only when filtered_findings is
absent or malformed, and deliberately does not subtract there: raw findings
are not the population that produced suppressed_findings.

Tests

15 new tests. 10 unit-cover every branch of effective_findings (empty
filtered, partial and full suppression, no-baseline pass-through, raw
fallback without subtraction, malformed input, empty result). 5 pin the
behaviour at the call sites: the recursive summary row, the combined JSON
count, and the two MCP verdict paths. The summary row and the JSON count
are pinned separately on purpose, since they are independent lines and a
regression in one is invisible to a test covering the other.

I verified these tests actually catch the bug rather than passing by
accident: reverting only the src/ change and re-running leaves all five
behavioural tests failing, each at the site it targets, and they pass again
with the fix restored.

pytest -q                       # 2088 passed, 17 skipped, 4 xfailed
ruff check src/ tests/          # All checks passed
ruff format --check src/ tests/ # 165 files already formatted

mypy reports the same 117 pre-existing errors in 21 files before and
after this change, so nothing new is introduced there.

Scope

Deliberately narrow, and independent of the open discussion on #201 and
#205 about whether --baseline should be supported in recursive mode at
all. This corrects how findings are counted and reported wherever the
partition already exists; it does not change what gets suppressed. The
review on #205 flagged this same falsy pattern as worth fixing in a
follow-up, which is what this is.

[wernerkasselman-au]

Hi Keshav,

You asked me to address review comments and resolve merge conflicts on #205 and
#19, and I have not simply done that on either of them, so let me set out where
each one landed and what I am asking for.

#375 and #377 are the two that are ready. Both are rebased on current
main (I merged 5680c2c into #375 an hour ago, so neither is behind), both
are MERGEABLE, and both are sitting at action_required waiting on a fork
workflow approval, which is the one thing I cannot do from my side. If you can
approve the runs on those two, the checks should go green.

• fix(cli): report the findings that actually drove the risk score #375 fixes a live counting bug: four call sites select findings with
  result.get("filtered_findings") or result.get("findings"), and since
  report() returns filtered_findings as the full pre-partition set (kept
  plus baseline-suppressed) while scoring, dedup, and SARIF all use
  active_findings, counting the former both over-counts suppressed findings
  and, because an empty list is falsy, falls through to the raw pre-filter list
  whenever the meta-analyzer filtered everything out. You flagged that same
  pattern as a follow-up candidate in your 27 June review here. It also reaches
  skillspector baseline (which was fingerprinting findings the scan never
  reported) and the MCP scan_skill verdict, which serialises the list straight
  to a calling agent, so an agent gating on risk_score was being handed a
  dirtier findings array than the score it was gating on.
• fix(build-context): bound the per-file read into file_cache #377 is the one piece of Bound build_context file reads and fail closed on oversized inputs #19 that is still true on current main.
  _read_file_cache() calls _read_text_no_follow(), which does an unbounded
  source.read(), on every discovered component, so a local directory scan
  materialises each file whole before any analyzer looks at it.
  INGEST_MAX_BYTES does not reach it (that budget is scoped to remote and
  archive ingest, and a local directory comes in through
  validate_local_input_path(), which does no sizing), and MAX_FILE_BYTES in
  that module is only used inside _is_valid_oms_signature() and for a
  size_bytes metadata field. It follows your ledger convention rather than my
  original one: it stats before reading, reuses the stat already taken for the
  S_ISREG check so it costs no extra syscall, and emits
  LedgerOutcome.SKIPPED with LedgerReason.SIZE_LIMIT instead of raising.

#19 I closed myself. It was based on a5092dd, main has since put roughly
+1,700/-170 into the same six files, and more to the point you answered the
original concern a better way than I had: the inspection ledger records an
oversized file as explicitly not-analyzed and counts it against
analysis_completeness, which gets the honesty guarantee without letting one
large file abort a whole scan. Rebasing it would have meant re-imposing my
abort policy over a mechanism you have already shipped.

#205 is the one I would like a decision on rather than a rebase. I can
resolve the conflict mechanically, but 34f6030 (#308) made recursive plus
--baseline an explicit exit 2, and _scan_multi_skill() no longer takes
baseline or show_suppressed at all, so rebasing this PR would re-enable a
feature you deliberately removed, as a side effect of a conflict resolution.
That is not a call I want to make on your behalf. For what it is worth I think
main is right, and for a reason from my own review thread on that PR:
fingerprints hash a path relative to each sub-skill root, so an identical
finding at the same relative path in a sibling sub-skill gets suppressed too,
and a path-scoped rule cannot target one sub-skill in recursive mode, which
makes recursive plus baseline unsound rather than merely missing. Either I
close #205 and raise the fingerprint scoping as its own issue (#201 would also
want retitling, since the silent drop it reports is now a loud exit 2), or, if
you do want the feature, the scoping fix goes first as its own PR and #205
closes in favour of it.

Two things I should be straight about on my own verification. I ran the full
suite (2210 passed, 17 skipped, 4 xfailed on #375), ruff check and
format-check, and confirmed the DCO sign-off on every commit individually,
including the merge commit. I could not run the Docker job, since Docker is not
installed on the machine I am working from, so
I ran the three assertions tests/docker/smoke.sh actually makes (the version
call, the local fixture scan to JSON, and the GitHub URL scan with the README
component assertion) against a native install instead, and they pass. That
covers the application behaviour but not the image build, so please treat that
job as unverified on my side.

One housekeeping note on #375: I force-pushed it once after the merge, to tidy
a commit message and to collapse the two consecutive merge commits into one, so
the SHAs have moved since your last look. The tree is byte identical either way
(I checked the tree hash rather than trusting the diff), so the change you are
reviewing has not moved, only the history around it.

Happy to take any of this differently, and if you would rather I fold #377 into
something else, or split it further, just say and I will rework it.

Thanks,
Werner

[wernerkasselman-au]

Correcting one thing I said above, in the direction of more confidence rather
than less: I have now run the Docker job, so please disregard the caveat that
it was unverified on my side.

I said Docker was not installed here, which was true, but I had only checked
for Docker itself rather than for any OCI builder, and podman 5.4.2 is
installed. Running tests/docker/smoke.sh unmodified against a podman-backed
docker (with --security-opt label=disable injected, since this host applies
SELinux labelling to the filesystem the checkout sits on, which a GitHub runner
does not) gives exit 0 on both PRs:

• the image builds from the pinned python:3.12-slim-bookworm digest
• skillspector --version reports v2.9.4 and git --version reports 2.39.5
  from inside the image
• the local tests/fixtures/safe_skill scan writes valid JSON
• the octocat/Hello-World scan exits 0, writes valid JSON, and satisfies the
  README component assertion

The only difference from CI is the SELinux flag and the runtime being podman
rather than Docker, so if either of those matters to you then treat the result
as indicative rather than authoritative, but the image and the four assertions
themselves are exercised.

Werner