diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index a133bc3f30..b5a7d92b66 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -119,6 +119,7 @@ "attack_or_counterexample": {"type": "string"}, "evidence": {"type": "string"}, "outcome": {"type": "string", "enum": ["falsified", "confirmed"]}, + "finding_index": {"type": ["integer", "null"], "minimum": 0}, }, "required": [ "path", @@ -128,6 +129,7 @@ "attack_or_counterexample", "evidence", "outcome", + "finding_index", ], } _NOEMA_FINDING_SCHEMA: dict[str, Any] = { @@ -641,6 +643,9 @@ def validate_substantive_verdict( raise NoemaModelOutputError( f"Noema adversarial validation requires at least {required_probes} concrete probe(s)" ) + findings = verdict.get("findings") + if not isinstance(findings, list): + raise NoemaModelOutputError("Noema formal verdict requires findings") confirmed: set[tuple[str, int, str]] = set() identities: set[tuple[Any, ...]] = set() @@ -666,6 +671,35 @@ def validate_substantive_verdict( raise NoemaModelOutputError( f"Noema adversarial probe {entry} outcome must be falsified or confirmed" ) + if "finding_index" not in probe: + raise NoemaModelOutputError( + f"Noema adversarial probe {entry} requires finding_index" + ) + finding_index = probe["finding_index"] + if outcome == "confirmed": + if type(finding_index) is not int or finding_index < 0 or finding_index >= len(findings): + raise NoemaModelOutputError( + f"Noema adversarial probe {entry} finding_index must reference a published finding" + ) + finding = findings[finding_index] + if not isinstance(finding, dict): + raise NoemaModelOutputError( + f"Noema adversarial probe {entry} finding_index must reference a published finding" + ) + finding_location = ( + finding.get("file"), + finding.get("line"), + finding.get("side"), + ) + if finding_location != location: + raise NoemaModelOutputError( + f"Noema adversarial probe {entry} finding_index location must match the probe location" + ) + confirmed.add((str(probe["path"]), int(probe["line"]), str(probe["side"]))) + elif finding_index is not None: + raise NoemaModelOutputError( + f"Noema falsified adversarial probe {entry} finding_index must be null" + ) identity = ( *location, probe["hypothesis"].strip().casefold(), @@ -674,21 +708,13 @@ def validate_substantive_verdict( if identity in identities: raise NoemaModelOutputError(f"Noema adversarial probe {entry} duplicates an earlier probe") identities.add(identity) - if outcome == "confirmed": - confirmed.add((str(probe["path"]), int(probe["line"]), str(probe["side"]))) if decision == "approve" and confirmed: raise NoemaModelOutputError("Noema approve cannot contain a confirmed adversarial probe") - if decision == "request_changes": - finding_locations = { - (str(finding.get("file") or ""), finding.get("line"), str(finding.get("side") or "")) - for finding in verdict.get("findings") or [] - if isinstance(finding, dict) - } - if not confirmed or not confirmed.intersection(finding_locations): - raise NoemaModelOutputError( - "Noema request_changes requires a confirmed probe on a published finding" - ) + if decision == "request_changes" and not confirmed: + raise NoemaModelOutputError( + "Noema request_changes requires a confirmed probe on a published finding" + ) def truncate_text(text: str, limit: int) -> str: @@ -1403,6 +1429,34 @@ def _format_gateway_error_telemetry(telemetry: dict[str, str | int]) -> str: ) +def _interleave_locations_by_path( + locations: Sequence[tuple[str, int, str]], +) -> list[tuple[str, int, str]]: + """Order changed locations round-robin across paths. + + The bounded JSON keeps a prefix of this order, so alphabetical ordering + would starve alphabetically-last paths (e.g. ``tests/``) under truncation + while letting them approve blind. Round-robin degrades evenly instead: + every path keeps its earliest lines first. Deterministic: paths in sorted + order, lines in input order. + """ + groups: dict[str, list[tuple[str, int, str]]] = {} + for location in locations: + groups.setdefault(location[0], []).append(location) + paths = sorted(groups) + ordered: list[tuple[str, int, str]] = [] + active = [(path, 0) for path in paths] + while active: + next_active: list[tuple[str, int]] = [] + for path, index in active: + ordered.append(groups[path][index]) + next_index = index + 1 + if next_index < len(groups[path]): + next_active.append((path, next_index)) + active = next_active + return ordered + + def _bounded_allowed_locations_json(allowed_locations: Sequence[dict[str, Any]]) -> str: """Serialize the largest location prefix that fits the prompt byte budget.""" total_count = len(allowed_locations) @@ -1546,12 +1600,24 @@ def call_llm( allowed_locations = [ {"path": path, "line": line, "side": side} - for path, line, side in sorted(changed_diff_locations(diff)) + for path, line, side in _interleave_locations_by_path( + sorted(changed_diff_locations(diff)) + ) ] location_example = allowed_locations[0] if allowed_locations else { "path": "path", "line": 0, "side": "RIGHT" } allowed_locations_json = _bounded_allowed_locations_json(allowed_locations) + allowed_locations_envelope = json.loads(allowed_locations_json) + if allowed_locations_envelope["truncated"]: + retained_locations = allowed_locations_envelope["locations"] + print( + "::warning::Noema changed-location context truncated " + f"total_locations={allowed_locations_envelope['total_count']} " + f"retained_locations={len(retained_locations)} " + f"total_paths={len({location['path'] for location in allowed_locations})} " + f"retained_paths={len({location['path'] for location in retained_locations})}" + ) prompt = { "role": "user", "content": "\n".join( @@ -1560,6 +1626,7 @@ def call_llm( "Review the PR diff plus the additional changed-file and review-thread context for correctness, security, maintainability, and behavioral regressions.", "Return only JSON with the declared response_format schema.", "Every formal verdict must cite exact changed-side lines. APPROVE requires falsifying concrete regression hypotheses; source or test changes require at least two distinct probes and other changes require at least one. REQUEST_CHANGES requires a confirmed probe at a finding location.", + "Every adversarial probe must include finding_index. A confirmed probe must set finding_index to the zero-based index of the published finding at the same path/line/side; a falsified probe must set finding_index to null.", "Use only path, line, and side tuples listed in the bounded allowed-locations JSON below. If it is truncated, omit a formal verdict for any location not listed instead of guessing.", f"Allowed changed-side locations: {allowed_locations_json}", f"Location shape example: {json.dumps(location_example, separators=(',', ':'))}", diff --git a/tests/test_noema_changed_location_truncation.py b/tests/test_noema_changed_location_truncation.py new file mode 100644 index 0000000000..e9bbb5e243 --- /dev/null +++ b/tests/test_noema_changed_location_truncation.py @@ -0,0 +1,120 @@ +"""Regression tests for bounded Noema changed-location prompt context.""" + +import json + +from scripts.ci import noema_review_gate as noema +from tests.test_noema_review_gate import make_pr + + +def test_call_llm_reports_allowed_location_truncation(monkeypatch, capsys): + """Report when the prompt receives only a bounded subset of changed lines.""" + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example.test/chat") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "secret") + monkeypatch.setattr(noema, "validate_substantive_verdict", lambda *_args: None) + monkeypatch.setattr( + noema, + "changed_diff_locations", + lambda _diff: { + (f"src/{prefix}.py", line, "RIGHT") + for prefix in ("a", "z") + for line in range(1, 500) + }, + ) + + class Response: + """Deterministic context-managed gateway response for this regression.""" + + def __enter__(self): + """Return this response to the context-managed caller.""" + return self + + def __exit__(self, *args): + """Propagate exceptions raised while processing the response.""" + return False + + def read(self): + """Return a valid non-blocking Noema verdict as encoded JSON.""" + return json.dumps( + { + "choices": [{ + "message": { + "content": '{"decision":"comment","summary":"checked","findings":[]}' + } + }] + } + ).encode() + + class Opener: + """Return the deterministic response without making network I/O.""" + + def open(self, request): + """Discard the prepared request and return the fixture response.""" + del request + return Response() + + monkeypatch.setattr(noema.urllib.request, "build_opener", lambda *_args: Opener()) + + noema.call_llm("owner/repo", 1, make_pr(), "diff", False, "head") + + output = capsys.readouterr().out + assert "::warning::Noema changed-location context truncated" in output + assert "total_locations=998" in output + assert "retained_locations=" in output + assert "total_paths=2" in output + assert "retained_paths=2" in output + + +def _long_path_locations(prefix: str, count: int) -> list[tuple[str, int, str]]: + """Build budget-sized locations whose paths sort under one prefix.""" + return [ + (f"src/{prefix}-{'가' * 80}.py", index + 1, "RIGHT") for index in range(count) + ] + + +def test_truncation_keeps_alphabetically_last_paths() -> None: + """Truncation must degrade evenly instead of starving last-sorted paths.""" + ordered = sorted( + _long_path_locations("a", 400) + _long_path_locations("z", 400) + ) + interleaved = noema._interleave_locations_by_path(ordered) + + plain = json.loads(noema._bounded_allowed_locations_json([ + {"path": path, "line": line, "side": side} + for path, line, side in ordered + ])) + fair = json.loads(noema._bounded_allowed_locations_json([ + {"path": path, "line": line, "side": side} + for path, line, side in interleaved + ])) + + assert plain["truncated"] is True + assert fair["truncated"] is True + assert {loc["path"] for loc in plain["locations"]} == { + f"src/a-{'가' * 80}.py" + } + assert {loc["path"] for loc in fair["locations"]} == { + f"src/a-{'가' * 80}.py", + f"src/z-{'가' * 80}.py", + } + + +def test_interleave_preserves_single_path_order() -> None: + """One path interleaves to itself, keeping existing order contracts.""" + ordered = [("tool.py", 292, "LEFT"), ("tool.py", 295, "RIGHT")] + assert noema._interleave_locations_by_path(ordered) == ordered + + +def test_interleave_preserves_uneven_path_groups() -> None: + """A short path is emitted once while a longer path keeps its order.""" + ordered = [ + ("a.py", 1, "RIGHT"), + ("a.py", 2, "RIGHT"), + ("a.py", 3, "RIGHT"), + ("z.py", 8, "RIGHT"), + ] + assert noema._interleave_locations_by_path(ordered) == [ + ("a.py", 1, "RIGHT"), + ("z.py", 8, "RIGHT"), + ("a.py", 2, "RIGHT"), + ("a.py", 3, "RIGHT"), + ] diff --git a/tests/test_noema_review_finding_probe_binding_contract.py b/tests/test_noema_review_finding_probe_binding_contract.py new file mode 100644 index 0000000000..470aba5bfd --- /dev/null +++ b/tests/test_noema_review_finding_probe_binding_contract.py @@ -0,0 +1,150 @@ +"""Regression contract for request-changes finding/probe binding. + +A formal request-changes verdict is not structurally reviewable when findings and confirmed +adversarial probes are only parallel arrays. The response schema must make the relationship explicit +so the model and contextual-orchestrator repair pass can reason about the same contract that the local +validator enforces. +""" + +from __future__ import annotations + +import copy + +import pytest + +from scripts.ci import noema_review_gate as noema + + +def _material_diff() -> str: + """Return a two-probe material Python diff with stable changed-side locations.""" + return """diff --git a/app.py b/app.py +--- a/app.py ++++ b/app.py +@@ -1 +1,2 @@ +-old ++new ++other +""" + + +def _request_changes_verdict() -> dict[str, object]: + """Return the smallest valid request-changes verdict for the binding contract.""" + return { + "decision": "request_changes", + "summary": "The first changed line violates the reviewed contract.", + "reviewed_lines": [ + { + "path": "app.py", + "line": 1, + "side": "RIGHT", + "analysis": "The changed value needs a blocking correction.", + } + ], + "adversarial_validation": { + "status": "failed", + "residual_risk": "The published finding remains until the change is corrected.", + "probes": [ + { + "path": "app.py", + "line": 1, + "side": "RIGHT", + "hypothesis": "The first change breaks the contract.", + "attack_or_counterexample": "Exercise the changed branch directly.", + "evidence": "The counterexample confirms the blocking defect.", + "outcome": "confirmed", + "finding_index": 0, + }, + { + "path": "app.py", + "line": 2, + "side": "RIGHT", + "hypothesis": "The second change introduces the same defect.", + "attack_or_counterexample": "Exercise the second changed line independently.", + "evidence": "The second hypothesis is falsified.", + "outcome": "falsified", + "finding_index": None, + }, + ], + }, + "findings": [ + { + "severity": "high", + "file": "app.py", + "line": 1, + "side": "RIGHT", + "message": "Correct the blocking contract violation.", + } + ], + } + + +def test_probe_schema_carries_explicit_published_finding_binding() -> None: + """Every probe must expose a schema-declared finding binding coordinate.""" + schema = noema._noema_verdict_json_schema(required_probes=2) + probe_schema = schema["properties"]["adversarial_validation"]["properties"]["probes"]["items"] + + assert "finding_index" in probe_schema["properties"] + assert "finding_index" in probe_schema["required"] + assert probe_schema["properties"]["finding_index"] == { + "type": ["integer", "null"], + "minimum": 0, + } + + +def test_request_changes_prompt_explains_confirmed_probe_binding() -> None: + """The model prompt must explain how the schema link is populated for each probe outcome.""" + source = noema.__loader__.get_source(noema.__name__) + assert source is not None + assert "confirmed probe must set finding_index" in source + assert "falsified probe must set finding_index to null" in source + + +def test_validator_accepts_confirmed_probe_bound_to_same_location_finding() -> None: + """A confirmed probe may bind only to the published finding at its exact location.""" + noema.validate_substantive_verdict(_request_changes_verdict(), _material_diff()) + + +@pytest.mark.parametrize("binding", [None, True, "0", -1, 1]) +def test_validator_rejects_invalid_confirmed_probe_binding(binding: object) -> None: + """Confirmed probes fail closed on null, bool, non-int, negative, or out-of-range bindings.""" + verdict = _request_changes_verdict() + verdict["adversarial_validation"]["probes"][0]["finding_index"] = binding # type: ignore[index] + + with pytest.raises(RuntimeError, match="finding_index"): + noema.validate_substantive_verdict(verdict, _material_diff()) + + +def test_validator_rejects_missing_confirmed_probe_binding() -> None: + """The deterministic backstop rejects a confirmed probe that omits the required binding.""" + verdict = _request_changes_verdict() + del verdict["adversarial_validation"]["probes"][0]["finding_index"] # type: ignore[index] + + with pytest.raises(RuntimeError, match="finding_index"): + noema.validate_substantive_verdict(verdict, _material_diff()) + + +def test_validator_rejects_confirmed_binding_to_different_finding_location() -> None: + """The referenced finding must share the confirmed probe's exact changed-side location.""" + verdict = _request_changes_verdict() + verdict["findings"][0]["line"] = 2 # type: ignore[index] + + with pytest.raises(RuntimeError, match="finding_index.*location"): + noema.validate_substantive_verdict(verdict, _material_diff()) + + +def test_validator_rejects_falsified_probe_with_finding_binding() -> None: + """Falsified probes cannot claim ownership of a published blocking finding.""" + verdict = _request_changes_verdict() + verdict["adversarial_validation"]["probes"][1]["finding_index"] = 0 # type: ignore[index] + + with pytest.raises(RuntimeError, match="falsified.*finding_index"): + noema.validate_substantive_verdict(verdict, _material_diff()) + + +def test_validator_rejects_falsified_probe_without_explicit_null_binding() -> None: + """The local validator mirrors the schema requirement instead of treating omission as null.""" + verdict = _request_changes_verdict() + del verdict["adversarial_validation"]["probes"][1]["finding_index"] # type: ignore[index] + + with pytest.raises(RuntimeError, match="finding_index"): + noema.validate_substantive_verdict(verdict, _material_diff())