From 11fa857263694905bf0800339d06a7245cdb7920 Mon Sep 17 00:00:00 2001 From: tzhouam Date: Mon, 21 Sep 2026 19:56:02 +0800 Subject: [PATCH] contract: publish findings with identity, severity and head binding The review result crossed the boundary as prose plus an anchor-only audit trail, so a consumer could not say whether a blocker on this head is the same blocker it saw on the last one. The review bot's ready gate needs exactly that: it carries open blocker/major findings forward and may only page a maintainer once none remain. Adds a findings list, one entry per published finding, each with a finding_id that is stable across heads (file plus the whole normalized text, deliberately not the line, so an unrelated edit above a finding does not lose it), the severity it was actually published with, its anchor, and the head it was found on. Duplicate identities are kept rather than collapsed, because two findings the pipeline chose to publish separately are two findings. No join with the disposition audit: those records are keyed by anchor, an anchor is not unique between a withheld and a published candidate, and anchor resolution can move a line after they are written, so a join would mislabel findings rather than inform. The aggregate finding_dispositions field is unchanged; STRICT_API_VERSION goes to 1.2.0 so consumers can detect the addition. Scope note: the RFC also asks for a per-finding recheck, so that a carried blocker retires only on an explicit fixed verdict and silence never reads as a fix. That needs the review step to be handed the carried findings and the model to answer about each one; neither exists yet, and disposition_records discards head_recheck today. Emitting carried_from / head_recheck / findings_missing_carried now would be fields no producer fills, so they wait for the slice that wires them. Co-Authored-By: Claude Opus 5 (1M context) --- src/infermatrix_copilot/contract.py | 82 +++++++++++++ src/infermatrix_copilot/sdk/v1/models.py | 2 +- test/test_contract.py | 142 +++++++++++++++++++++++ test/test_sdk_v1.py | 2 +- 4 files changed, 226 insertions(+), 2 deletions(-) diff --git a/src/infermatrix_copilot/contract.py b/src/infermatrix_copilot/contract.py index d90618131..c7d07e80e 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 90c84c6a3..60727ead8 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 db71a9d02..1da083900 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 270f4b51f..e5378d264 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