Repository navigation
Conversation
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Hi @Cid-oe, thank you for your contribution to SkillSpector — we really appreciate the time you put into this! A few items need attention before it can be merged; details below.
Wraps effective_findings(result) in _expand_occurrences() in the baseline command. This is the same approach as #631 (opened two days earlier), but with a function-local import and no test. The approach is incomplete, and the lint and DCO Check jobs fail. Competing PR #637 fixes the same root cause completely by fingerprinting the pre-deduplication findings the report node partitions, so I'd recommend consolidating there.
Findings
- [Blocking]
src/skillspector/cli.py:3185: the expanded occurrences keep the representative'scontext,code_snippetandconfidence(report.py:255-275;deduplicate.py:84-86leaves these out of the compaction key).finding_fingerprinthashes all three (suppression.py:236,247-248). For context-bearing rules, the non-representative occurrences therefore get fingerprints the next scan never computes. #633's repro (AR2 inSKILL.mdandreferences/notes.md) still reports one occurrence after the baseline is regenerated. The RP1 demo in #630 only works because RP1 carries no context. Expected: fingerprint the exact listpartition_findingsevaluates (the kept findings before deduplication), as #637 does. - [Blocking]
src/skillspector/cli.py:3183: thelintjob fails atmake format-check.ruff format --checkreportsWould reformat: src/skillspector/cli.pybecause the function-local import needs a blank line after it. Please import at module level next toreport(cli.py:63) and runmake format, or drop the import if you switch approach. - [Blocking]
DCO Checkfails because commit26fe5f15has noSigned-off-by:trailer. Its author email is alsocid@example.com. Please configure your real name and email, rungit commit --amend --reset-author -s, and force-push. - [Blocking] There is no regression test. A change to baseline generation needs a test that fails without the fix. Ideally that is a CLI round trip (
baseline, thenscan --baseline --format json) where a context-bearing finding repeats across lines or files, assertingissues == []and that every occurrence is insuppressed.
Thank you for digging into #630. Since #631 and #637 already address this issue, closing this PR in favor of #637 would avoid three parallel fixes.
Tests/CI: lint (format-check) and DCO Check fail. test-unit, docker-smoke and OpenCode TypeScript Tests pass. No tests are added.
Decision: Changes Requested (reviewed head 26fe5f15417871c7ce29248e7eb869c408fd1325)
| from skillspector.nodes.report import _expand_occurrences | ||
| result = graph.invoke(state) | ||
| findings = effective_findings(result) | ||
| findings = _expand_occurrences(effective_findings(result)) |
There was a problem hiding this comment.
Same limitation as #631: _expand_occurrences copies the representative's context, code_snippet and confidence onto every occurrence (report.py:255-275), and finding_fingerprint hashes all three (suppression.py:236,247-248). For context-bearing rules, such as AR2 in #633, the non-representative occurrences never match what partition_findings computes on the next scan, so they stay reported. Please fingerprint the kept, pre-deduplication list the report node partitions, as #637 does with active_findings.
| # output_format is irrelevant here; we consume findings, not report_body. | ||
| state = _scan_state(input_path, FormatChoice.json, no_llm) | ||
| state["baseline_path"] = os.path.abspath(output.expanduser()) | ||
| from skillspector.nodes.report import _expand_occurrences |
There was a problem hiding this comment.
This line is why lint fails at make format-check: ruff format --check wants a blank line after the function-local import (Would reformat: src/skillspector/cli.py). Please import at module level next to report (from skillspector.nodes.report import report, cli.py:63) and run make format, or drop the import if you switch approach.
When `skillspector baseline` evaluates the list of findings to build its suppression fingerprint map, it consumed `effective_findings(result)`, which extracts the `filtered_findings` produced by the report node. However, the report node already deduplicates those findings, folding multiple occurrences into a single finding object. Because baselines require a fingerprint for every unique line/occurrence, fingerprinting the already-folded finding list resulted in only the first occurrence of a finding being successfully added to the baseline, leaving duplicates unsuppressed. This patch wraps `effective_findings` in `_expand_occurrences` when building the baseline dictionary, restoring the 1:1 fingerprint-to-occurrence mapping. Closes NVIDIA#630 Signed-off-by: Siddharth U <cid066a86@gmail.com>
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Hi @Cid-oe, thank you for fixing the sign-off and the formatting so quickly after the last review!
Value and readiness: The new head fixes the two CI failures (lint and DCO Check), but the code change is the same as before. The baseline still fingerprints _expand_occurrences(effective_findings(result)), which misses repeated context-bearing findings, and there is still no regression test. It is not ready. This PR is now superseded. #637 (approved) fixes the root cause, #631 has switched to the same approach, and #657 builds on it. I recommend closing this PR in favor of #637.
Previous findings:
- 1 (expansion copies the representative's evidence, so #633's AR2 repro still leaks): Still open.
src/skillspector/cli.py:3186is unchanged._expand_occurrencesstill copies the representative'scontext,code_snippetandconfidenceonto every occurrence (report.py:255-275), andfinding_fingerprinthashes all three (suppression.py:236,247-248). - 2 (
lintfails atmake format-check): Resolved. Commit7ad581cfadds the blank line after the function-local import, andlintpasses. The private helper is still imported inside the function. That stops mattering if the approach changes. - 3 (
DCO Checkfails, placeholder author email): Resolved.7ad581cfis signed off asSiddharth U <cid066a86@gmail.com>, matching the author, andDCO Checkpasses. - 4 (no regression test): Still open. The PR still changes only
src/skillspector/cli.py(+3/-1).
Material findings
- [Blocker]
src/skillspector/cli.py:3186: same as previous finding 1. Here is a concrete failure. In #633's repro, AR2 fires atSKILL.md:8andreferences/notes.md:3. Each match carries its own ±3-line context. The expandednotes.mdentry getsSKILL.md's context, so its hash never matches whatpartition_findingscomputes on the next scan. That finding stays inissuesand in the risk score, and still trips--fail-on-findings. Expected: fingerprint the kept findings before deduplication, as #637 does withactive_findings. - [Blocker] No test: same as previous finding 4. The fix needs a CLI round trip that fails without it. Run
baseline, thenscan --baseline --format json, on a context-bearing rule that repeats across lines or files. Then assertissues == []and that every occurrence is insuppressed. The test in #637 (tests/unit/test_cli.py, AR2 across two files) would fail against this head.
PIC tradeoffs: None identified. This PR's change is a strict subset of what #637 does, so closing it loses nothing.
Verification and gaps: I compared the new head 7ad581cf with the previously reviewed 26fe5f15. The only source difference is one blank line. The commit was re-authored and signed off, and the base is unchanged (c7958a3). I re-traced the expansion and fingerprint paths at this head. All 6 CI checks pass. Per policy, I did not run the tests locally.
Decision: Changes Requested (reviewed head 7ad581cfb16dcef06f37abdf5a48464717db9044)
|
Closing in favor of #637 as recommended. |
Closes #630
Description
The
skillspector baselinecommand was building its fingerprint map from thefiltered_findingsoutput of the report node. However, the report node deduplicates findings (folding multiple line occurrences into a single finding object) before emitting them tofiltered_findings.Because suppression fingerprints are calculated per-occurrence (per-line), using the deduplicated list meant that only one fingerprint was recorded in the baseline per finding cluster. Subsequent scans would then successfully suppress the first occurrence but continue to report the remaining duplicates.
This PR wraps the
effective_findingspassed tobuild_baseline_dictin_expand_occurrences, ensuring that every individual occurrence is fingerprinted and written to the baseline file.