ci-secure: cover coverage-gap dedupe with tests (issue #36)

Confidence Score: 4/5

The PR appears safe to merge, with only a misleading helper comment that should be corrected for maintainability.

The tests seed the scanner’s dropped-match queue and directly validate deduplication across all three output buckets; the only accepted concern is contradictory documentation around why queue clearing is disabled.

Files Needing Attention: skills/ci-secure/tests/test_scan.py

Important Files Changed

_{Reviews (1): Last reviewed commit: "Test cross-bucket dedupe scoping for cov..." | Re-trigger Greptile}

@claude review

Claude finished @yonasb's task in 3m 8s —— View job

Review: coverage-gap dedupe tests (issue #36)

• Gather context (diff, scan() bucketing loop, report.py consumers)
• Analyze the new tests for correctness and teeth
• Check repo conventions (CLAUDE.md: changelog, install surface)
• Post review

Verdict: looks good — approve-shaped, no blocking issues. The tests target the right seam (skills/ci-secure/scripts/scan.py:5464-5475), the mutation checks in the PR description are the right way to prove teeth, and test_dedupe_is_scoped_per_bucket_not_across_buckets closes a genuinely non-obvious gap (kind is not part of the dedupe key, so the same (file, reason) legitimately lands in two buckets). Comments below are polish, ranked.

Note: I could not run pytest in this environment (command not permitted), so this is a static review plus the CI result — I did not independently re-run the 6 passing tests or the mutation checks.

1. The stale Greptile comment is already fixed

Greptile's inline suggestion targets the #NOTE: This just makes sure the Dropped matches are empty… line, which commit c382334 removed. The surviving _NoClearList docstring (skills/ci-secure/tests/test_scan.py:2158-2164) says the opposite and is correct — entries seeded before the call survive into the loop. Nothing to do here.

2. All four tests bypass the detectors — the premise itself is untested (low, worth considering)

The header comment at skills/ci-secure/tests/test_scan.py:2145-2148 asserts the motivating fact:

several detectors record the same (file, reason) pair once per match, so a file with two matching steps queues the identical entry twice

No test demonstrates that. All four seed _DROPPED_MATCHES by hand, so they pin the bucketing loop but not the reason the loop needs a dedupe. This matters because most gap(...) reasons already embed a line number (scan.py:3722, 3870, 3974, 4036), which makes them naturally distinct; the ones that can genuinely repeat are the job-keyed P14.24/P14.25 unanchored records (scan.py:2749, scan.py:4067) and the here-doc gaps (scan.py:3425, 3449), which key only on the here-doc marker. If a future refactor adds line numbers everywhere, the dedupe becomes dead code and these tests still pass green — protecting code that no longer matters.

One end-to-end case — a real workflow that provokes a genuine repeat through a real scan() — would be worth more than the three seeded ones, and would keep the header comment honest. Not a merge blocker; the seeded tests are still a strict improvement over zero coverage.

Fix this →

3. Assert the untouched buckets stay empty (nit)

test_repeated_coverage_gap_entries_collapse_per_file (line 2203) and test_dedupe_is_scoped_per_bucket_not_across_buckets (line 2263) only check the buckets they seeded. Adding assert result["suppressed_findings"] == [] to the cross-bucket test would also catch a mutation that fans one entry into every bucket — currently that variant slips past, since both asserted buckets would still read [reason].

4. Whole-bucket equality is sensitive to unrelated detector changes (nit, trade-off)

The assertions compare the full bucket, so if the benign ok.yml fixture ever starts producing a gap of its own, all four tests fail with a confusing diff rather than a clear signal. Filtering to the seeded reason (or Counter(reasons)[reason] == 1 — Counter is already imported at line 23) would isolate the behavior under test. Counter-argument: exact equality also catches unexpected pollution, which is arguably the point. Your call — I'd leave it.

5. Type annotation consistency (nit)

_bucket_gaps (line 2170) takes a bare monkeypatch and queue: list[dict], while the tests below annotate pytest.MonkeyPatch. monkeypatch: pytest.MonkeyPatch and queue: list[dict[str, str]] (matching _DROPPED_MATCHES's own annotation at scan.py:798) would match the surrounding file.

Repo conventions

• CHANGELOG — not required here. CLAUDE.md: "Pure-docs / test-only refactors that don't change behavior can be noted briefly or skipped." This adds no behavior. ✅
• Install surface — the change is inside skills/ci-secure/tests/, which is part of the shipped tree already; no maintainer infra leaked in. ✅
• Single file, +128/−0 — no production code touched, so the blast radius is the test suite only. ✅

One observation about what the tests now pin (not a defect in this PR)

The dedupe collapses on (workflow_file, reason), and report.py:2381 renders len(dropped_matches) as "N run: step(s) in M workflow(s)". So two distinct unscanned steps that happen to produce byte-identical reasons — e.g. two unclosed here-docs opened with the same marker in one job (scan.py:3425) — collapse to one, and the headline under-counts the gap. That's pre-existing behavior, and this PR is right to pin it as-is; flagging only because these tests now make it the documented contract, so a future fix would have to change them deliberately. · Branch issue-36-coverage-gap-dedupe-test