contract: publish findings with identity, severity and head binding

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 ""),

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"
 

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"] == []

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