fix(meta): bind assessments to exact findings - #702

[SkillSpector Review]

Hi @Harbor404, thank you for tackling all three meta-review defects from #667 in one coherent change, especially the false "LLM degraded" caused by coverage findings!

Value and readiness: The meta_review_required snapshot fixes the false degraded status: AE1/AE7 coverage findings added during finalization no longer require a meta call. The updated test_reference_destination_kinds expectation matches #667, and analysis_completeness still reports the real coverage gap. The duplicate-location guard is a sound conservative choice. No LLM response can remove a finding or lower its severity or confidence: confirmation uses max(...), and llm-unconfirmed has no scoring consumer in src/. However, the new finding_id binding accepts whatever id the model returns without checking it. That can move an assessment onto a different rule's finding, or drop an answer the model did give. In addition, the new evidence fields copy free-form model text onto every reviewed finding, which changes terminal and Markdown output for every LLM scan. Not ready to merge yet.

Material findings

1. [Blocker] src/skillspector/nodes/meta_analyzer.py:439-442, 472, 493: id binding is trusted without checks, and it is the only binding used when an id is present.

   • An item that carries a finding_id is bound to that finding without checking that the item's pattern_id/start_line match it, or that the id was part of the item's batch. Suppose a batch holds an SC4 finding at line 4 and an AST1 finding at line 9, and the model swaps their ids. The AST1 finding then gets the SC4 assessment's text as its message/explanation/remediation. If the model leaves explanation empty, line 493 builds the default text from assessment.get("pattern_id"), so another rule's default explanation lands on this finding. Main could not mix up rules this way, because its keys included rule_id.
   • An item whose id matches no finding (for example, one hex digit wrong in the 40-character id) hits continue at line 442 and is never indexed by location. The finding it reviewed is then reported as missing, even though the model answered with the correct pattern and line.

   Please index every item both by id and by location. Accept the id only when it belongs to that batch's batch.findings and its rule_id (and start_line, when given) match. Otherwise, fall back to the location path, keeping the duplicate guard. Build default text from f.rule_id. Please add tests for swapped ids, an unknown id with a correct location, and an id from another batch. (Finding ids are per-run uuid4 values, and each batch prompt lists only its own ids. So I see no practical way for a prompt-injected batch to target another batch's finding; the batch check is defense in depth.)

2. [Blocker] meta_analyzer.py:300-305: _review_evidence copies the model's confidence, explanation and remediation into evidence for every reviewed finding. The reports print evidence in full: the terminal output at report.py:1072-1074 (even though message and remediation there are cut to 60 and 150 characters), and Markdown at report.py:1518-1521 (each value inside an inline code span). Every finding in an LLM scan therefore gains an Evidence line that repeats the model's explanation and remediation. Confirmed findings already carry that text as message/explanation/remediation. For disagreed or low-confidence findings, the model's rebuttal, which prompt injection in the scanned skill can steer ("documented installer, safe to run"), now appears under "Evidence" next to a deterministic finding. #667 only asks to distinguish the outcomes, and llm_review_outcome already does that. Please record only llm_review_outcome (and llm_review_confidence, if wanted) and drop the copied explanation and remediation. If the PIC wants the model's reasoning for non-confirmed findings, put it in a clearly labelled field with a length limit.

3. [Non-blocking] meta_analyzer.py:331-334, 872-875, 910-914: the failure paths now add the llm-unconfirmed tag through _mark_review_failed. On main, findings whose review failed stayed untagged. The PR description says the existing tag behaviour is kept, but this changes it. Two effects:

   • Consumers that act on the tag will treat an LLM outage as model disagreement, which is the conflation #667 asks to remove.
   • suppression.finding_fingerprint hashes tags, so a baseline written with --no-llm no longer matches findings from a run whose LLM call failed. On main, those two outputs produce the same fingerprint.

   Please leave failure paths untagged and rely on llm_review_outcome=failed, or document the new meaning of the tag.

4. [Non-blocking] meta_analyzer.py:441 and the location dictionaries: if a finding receives several assessments, the last batch wins. This happens with chunked files: findings_in_range places a finding in every chunk that contains its start line, and chunks overlap by 50 lines. Main kept any confirmation. Now a later chunk without the relevant context can flip a confirmed finding to disagreed. Please merge deterministically (confirmed > low-confidence > disagreed) and add a test.

5. [Non-blocking] cli.py:2252: the transitive merge builds merged_result = {**initial_result, ...}, so meta_review_required reflects only the root scan, while effective_finding_ids is merged from all children. Consider OR-ing the child flags so the field means the same thing in transitive scans.

6. [Non-blocking] Tests and docs:

   • test_finding_clones_preserve_security_metadata_and_confidence changed from evidence == original.evidence to two spot checks. Please assert that the original evidence is preserved as a subset (returned.evidence.items() >= original.evidence.items()).
   • Please add a semantic_runtime test for a legacy result (no flag) whose only findings are tagged coverage.
   • The apply_filter and meta_analyzer docstrings still describe matching by (file, rule_id) only.

PIC tradeoffs: First, should the model's reasoning for disagreed or low-confidence findings appear in reports at all (item 2)? Showing it helps triage, but it puts text a scanned skill can influence next to deterministic findings. Second, should llm-unconfirmed also mean "review failed" (item 3)?

Verification and gaps: I traced apply_filter, every return path of the meta_analyzer node, finalize_inspection_ledger, semantic_runtime._has_effective_findings, the transitive merge in cli.py, evidence rendering in report.py, and suppression.finding_fingerprint. I confirmed that finding ids come from finding-{uuid4().hex} (models.py:118-120) and that _file is set by parse_response, not by the model. Tests were not run locally per policy. CI: all 6 checks green. Overlap: no file overlap with Harbor404's #700, #701 or #703-#706. #703 also changes the finding output shape (models.py/report.py, different fields), so the two output changes should be coordinated.

Decision: Changes Requested (reviewed head b8c17dc42a83d0cee94fc47863e0184d43c6cff5)