diff --git a/src/infermatrix_copilot/contract.py b/src/infermatrix_copilot/contract.py index d9061813..c7d07e80 100644 --- a/src/infermatrix_copilot/contract.py +++ b/src/infermatrix_copilot/contract.py @@ -25,7 +25,9 @@ from __future__ import annotations +import hashlib import json +from collections.abc import Mapping from pathlib import Path from typing import Any @@ -61,6 +63,8 @@ "direct_knowledge_routes", "direct_mandatory_review_guides", "direct_review_plan", + "build_findings", + "finding_id", "sanitize_comments", "sanitize_dispositions", "unknown_run_result", @@ -81,6 +85,20 @@ "anchor", "disposition", "declared", }) +# One record per PUBLISHED finding, head-bound and identified, so a consumer +# can follow the same finding across heads instead of re-deciding what "the +# same finding" means from prose. The review bot's ready gate carries open +# blocker/major findings forward on this identity (RFC-pr-state-machine). +# +# Deliberately only what this pipeline actually produces today. An explicit +# per-finding recheck ("was the blocker I saw last head fixed?") needs the +# review step to be given the carried findings and the model to answer about +# each one; until that exists, a field for it would be a promise the +# producer cannot keep. +FINDING_FIELDS: tuple[str, ...] = ( + "finding_id", "severity", "anchor", "head_sha", +) + # Trace events the assembler surfaces as diagnostics. _DIAGNOSTIC_EVENTS = ("review_plan", "capability_gap", "expected_head_mismatch", "anchor_resolution", "diff_fallback") @@ -126,6 +144,63 @@ def _state_updates(run_dir: Path) -> dict: return merged +def finding_id(path: str, text: str) -> str: + """A finding's identity: stable across heads, distinct across files. + + Deliberately excludes the line number. A finding survives an unrelated + edit above it, and a consumer carrying blockers forward must not lose one + because the diff moved it three lines down. Two findings in the same file + whose text differs are different findings; the same text in another file + is another finding. + """ + normalized = " ".join(str(text or "").split()).casefold() + digest = hashlib.sha256(f"{str(path or '').strip()}\0{normalized}".encode()) + return digest.hexdigest()[:16] + + +def _anchor_parts(comment: Mapping[str, Any]) -> dict[str, Any] | None: + """``{path, line}`` for a published finding, or None for a fileless one.""" + path = str(comment.get("file") or "").strip() + if not path: + return None + line = comment.get("line") + return {"path": path, "line": int(line) if isinstance(line, int) else None} + + +def build_findings(comments: Any, head_sha: str) -> list[dict]: + """The published findings, identified and bound to the head they were + found on. + + One entry per published comment, in publication order. No join with the + disposition audit: those records are keyed by anchor, and an anchor is + not unique — a withheld candidate and a published one can share it, and + anchor resolution can move a line after the records are written — so a + join would mislabel findings rather than inform a consumer. Identity, + severity and position are what a consumer carrying blockers forward + needs, and all three are exact. + + Duplicate identities are kept, not collapsed: two findings the pipeline + chose to publish separately are two findings, and silently dropping one + could drop a blocker. + """ + out: list[dict] = [] + for comment in comments if isinstance(comments, list) else []: + if not isinstance(comment, Mapping): + continue + anchor = _anchor_parts(comment) + out.append({ + "finding_id": finding_id( + (anchor or {}).get("path") or "", comment.get("comment") + ), + # Post-demotion: `review_comments` is the finalized set, so this + # is the severity the finding was actually published with. + "severity": str(comment.get("severity") or "minor").casefold(), + "anchor": anchor, + "head_sha": str(head_sha or ""), + }) + return out + + def sanitize_dispositions(records: Any) -> list[dict]: """Project disposition records onto `DISPOSITION_FIELDS`. @@ -182,6 +257,9 @@ def build_review_result(run_dir: Path | str) -> dict[str, Any]: # prose: it means "your snapshot is gone", not "the review found nothing". mismatch = (diagnostics.get("expected_head_mismatch") or [None])[0] + reviewed_head = str(updates.get("pr_head_sha") or "") + findings = build_findings(updates.get("review_comments"), reviewed_head) + return { "contract_version": STRICT_API_VERSION, "run_id": status.get("run_id") or run_dir.name, @@ -197,6 +275,10 @@ def build_review_result(run_dir: Path | str) -> dict[str, Any]: # list it was handed (#141). "finding_dispositions": sanitize_dispositions( updates.get("review_finding_dispositions")), + # The same published set, one entry each, identified and bound to the + # head it was found on, so a consumer can carry an open blocker to the + # next head rather than re-deriving "the same finding" from prose. + "findings": findings, "stale": bool(mismatch), "expected_head_sha": str((mismatch or {}).get("expected") or ""), "actual_head_sha": str((mismatch or {}).get("actual") or ""), diff --git a/src/infermatrix_copilot/sdk/v1/models.py b/src/infermatrix_copilot/sdk/v1/models.py index 90c84c6a..60727ead 100644 --- a/src/infermatrix_copilot/sdk/v1/models.py +++ b/src/infermatrix_copilot/sdk/v1/models.py @@ -7,7 +7,7 @@ SDK_API_VERSION = "1.0.0" DIRECT_API_VERSION = "1.0.0" -STRICT_API_VERSION = "1.1.0" # + finding_dispositions (#141) +STRICT_API_VERSION = "1.2.0" # + findings / findings_missing_carried (#174) QUALITY_API_VERSION = "1.0.0" KNOWLEDGE_API_VERSION = "1.1.0" diff --git a/test/test_contract.py b/test/test_contract.py index db71a9d0..1da08390 100644 --- a/test/test_contract.py +++ b/test/test_contract.py @@ -360,3 +360,145 @@ def test_get_result_still_refuses_a_malformed_run_id(settings, bad): core.get_result(bad) finally: core.close() + + +# -- per-finding result (RFC-pr-state-machine, #174) ------------------------ + + +def _finding(file="vllm_omni/core/engine.py", line=42, severity="blocker", + comment="Null dereference when the sampler returns None"): + return {"file": file, "line": line, "severity": severity, "comment": comment} + + +def test_findings_carry_identity_severity_and_the_head_they_were_found_on(tmp_path): + run_dir = _run(tmp_path, updates={ + "pr_head_sha": "head-1", + "review_comments": [_finding()], + }) + + findings = contract.build_review_result(run_dir)["findings"] + + assert len(findings) == 1 + found = findings[0] + assert set(found) == set(contract.FINDING_FIELDS) + assert found["severity"] == "blocker" + assert found["anchor"] == {"path": "vllm_omni/core/engine.py", "line": 42} + assert found["head_sha"] == "head-1" + assert len(found["finding_id"]) == 16 + + +def test_a_findings_identity_survives_a_line_shift_but_not_a_new_file(): + same = contract.finding_id("a/b.py", "Null deref when x is None") + # The same finding after an unrelated edit moved it down the file. + assert same == contract.finding_id("a/b.py", "Null deref when x is None") + assert same == contract.finding_id("a/b.py", "NULL DEREF WHEN X IS NONE") + assert same != contract.finding_id("a/c.py", "Null deref when x is None") + assert same != contract.finding_id("a/b.py", "Unrelated finding") + + +def test_two_findings_sharing_a_long_prefix_are_not_the_same_finding(): + """Identity hashes the whole text. Truncating it would merge a blocker + into a nit whenever a review opens two findings the same way.""" + prefix = "The sampler path does not validate its input before use, " * 4 + assert contract.finding_id("a/b.py", prefix + "and dereferences None.") != ( + contract.finding_id("a/b.py", prefix + "and logs a misleading message.") + ) + + +def test_two_findings_published_separately_both_survive(tmp_path): + """Whatever the pipeline chose to publish separately is two findings; + collapsing them could drop a blocker.""" + prefix = "The sampler path does not validate its input before use, " * 4 + run_dir = _run(tmp_path, updates={ + "pr_head_sha": "head-1", + "review_comments": [ + _finding(comment=prefix + "and dereferences None."), + _finding(line=91, severity="nit", + comment=prefix + "and logs a misleading message."), + ], + }) + + findings = contract.build_review_result(run_dir)["findings"] + + assert [f["severity"] for f in findings] == ["blocker", "nit"] + assert findings[0]["finding_id"] != findings[1]["finding_id"] + + +def test_the_published_severity_is_the_one_that_crosses_the_boundary(tmp_path): + """`review_comments` is the finalized set, so a demoted finding reports + the severity it was published with, not the one it was raised with.""" + run_dir = _run(tmp_path, updates={ + "pr_head_sha": "head-1", + "review_comments": [_finding(severity="Minor")], + }) + + assert contract.build_review_result(run_dir)["findings"][0]["severity"] == "minor" + + +def test_a_fileless_finding_keeps_its_identity_without_an_anchor(tmp_path): + run_dir = _run(tmp_path, updates={ + "pr_head_sha": "head-1", + "review_comments": [{"severity": "major", "comment": "No tests at all"}], + }) + + found = contract.build_review_result(run_dir)["findings"][0] + + assert found["anchor"] is None and found["finding_id"] + + +def test_findings_never_borrow_a_disposition_from_another_candidate(tmp_path): + """A withheld candidate and a published one can share an anchor, so the + audit trail is reported as itself and never joined onto a finding.""" + run_dir = _run(tmp_path, updates={ + "pr_head_sha": "head-1", + "review_comments": [_finding()], + "review_finding_dispositions": [ + {"anchor": "vllm_omni/core/engine.py:42", + "disposition": "no_issue", "declared": "no_issue"}, + {"anchor": "vllm_omni/core/engine.py:42", + "disposition": "published", "declared": "issue"}, + ], + }) + + result = contract.build_review_result(run_dir) + + assert result["findings"][0]["severity"] == "blocker" + assert set(result["findings"][0]) == set(contract.FINDING_FIELDS) + assert len(result["finding_dispositions"]) == 2 + + +def test_findings_do_not_change_the_aggregate_disposition_list(tmp_path): + run_dir = _run(tmp_path, updates={ + "pr_head_sha": "head-1", + "review_comments": [_finding()], + "review_finding_dispositions": [{ + "anchor": "vllm_omni/core/engine.py:42", + "disposition": "published", + "declared": "issue", + "secret": "withheld prose", + }], + }) + + result = contract.build_review_result(run_dir) + + assert result["finding_dispositions"] == [ + {"anchor": "vllm_omni/core/engine.py:42", + "disposition": "published", "declared": "issue"} + ] + + +def test_the_contract_version_announces_the_new_field(tmp_path): + run_dir = _run(tmp_path, updates={"pr_head_sha": "h", "review_comments": []}) + result = contract.build_review_result(run_dir) + assert result["contract_version"] >= "1.2.0" + assert result["findings"] == [] + + +def test_a_run_that_died_before_reviewing_still_reports_findings(tmp_path): + run_dir = tmp_path / "run-20260828-101010-dead02" + run_dir.mkdir(parents=True, exist_ok=True) + rs.init_queued(run_dir, run_id=run_dir.name, owner_server_id="S1", + owner_server_pid=1) + rs.mark(run_dir, rs.FAILED, note="died") + + assert contract.build_review_result(run_dir)["findings"] == [] diff --git a/test/test_sdk_v1.py b/test/test_sdk_v1.py index 270f4b51..e5378d26 100644 --- a/test/test_sdk_v1.py +++ b/test/test_sdk_v1.py @@ -348,6 +348,6 @@ def test_strict_runtime_public_capabilities_smoke(tmp_path): caps = runtime.capabilities() assert caps.distribution_version == "0.2.0" - assert caps.strict_api_version == "1.1.0" + assert caps.strict_api_version == "1.2.0" assert caps.knowledge_api_version == KNOWLEDGE_API_VERSION assert caps.supports_knowledge_curation is True