diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index 4f82281fc3..f1c39a51bd 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -423,6 +423,63 @@ def parse_diff_path(raw: str, prefix: str) -> str: return value.removeprefix(prefix) +def _entry_ordinal(position: int, total: int) -> str: + """Return an unambiguous array-position label for a validated JSON entry. + + ``position`` is the entry's 1-based place in the array being validated — + an array position, not a source-code line number. The historical message + text ("Noema reviewed line N is not an exact changed-side line") read as + if N named literal file line N; it only ever named "the Nth entry" of + ``reviewed_lines``/``probes``, so two failures on entries 1 and 3 of a + 3-entry array could be misread as complaints about file lines 1 and 3 + (see the naruon#1503 investigation this fixes). Every caller splices this + immediately after the fixed ``"Noema reviewed line "``/``"Noema + adversarial probe "`` prefix so ``_stable_failure_diagnostic``'s + trusted-prefix allowlist still recognizes the message as trusted + structural validator output. + """ + return f"entry {position}/{total} (array index {position - 1}, not a source line)" + + +def _format_location(path: Any, line: Any, side: Any) -> str: + """Format one rejected path/line/side citation for a diagnostic message. + + ``repr()`` on each raw value (rather than plain interpolation) keeps a + non-string ``path``, a non-int ``line``, or a ``None`` deliberately + distinguishable in the rendered text instead of silently coercing to a + misleading string. + """ + return f"path={path!r} line={line!r} side={side!r}" + + +def _nearby_changed_locations( + locations: set[tuple[str, int, str]], path: Any, line: Any, *, limit: int = 5 +) -> str: + """Return a short hint of the closest real changed locations sharing ``path``. + + Scoped to ``locations`` entries whose path matches ``path`` exactly, then + sorted nearest-line-first (so a citation just one line off a real changed + line is obviously close, rather than buried in an unsorted dump) and + capped at ``limit`` entries to keep the GitHub Actions ``::error::`` + annotation this feeds into readable. Returns ``""`` — no hint — when + ``path`` is not a string or no changed location shares it; there is + nothing useful to compare against. + """ + if not isinstance(path, str): + return "" + same_path = [location for location in locations if location[0] == path] + if not same_path: + return "" + if isinstance(line, int): + same_path.sort(key=lambda location: (abs(location[1] - line), location[1], location[2])) + else: + same_path.sort(key=lambda location: (location[1], location[2])) + sample = ", ".join(f"{p}:{ln} ({s})" for p, ln, s in same_path[:limit]) + remaining = len(same_path) - limit + more = f", +{remaining} more" if remaining > 0 else "" + return f"; nearest changed lines for {path}: {sample}{more}" + + def validate_substantive_verdict( verdict: dict[str, Any], diff: str, changed_paths: Sequence[str] = () ) -> None: @@ -437,15 +494,22 @@ def validate_substantive_verdict( reviewed_lines = verdict.get("reviewed_lines") if not isinstance(reviewed_lines, list) or not reviewed_lines: raise NoemaModelOutputError("Noema formal verdict requires at least one reviewed changed line") - for index, reviewed in enumerate(reviewed_lines, start=1): + reviewed_total = len(reviewed_lines) + for position, reviewed in enumerate(reviewed_lines, start=1): + entry = _entry_ordinal(position, reviewed_total) if not isinstance(reviewed, dict): - raise NoemaModelOutputError(f"Noema reviewed line {index} must be an object") + raise NoemaModelOutputError(f"Noema reviewed line {entry} must be an object") location = (reviewed.get("path"), reviewed.get("line"), reviewed.get("side")) if location not in locations: - raise NoemaModelOutputError(f"Noema reviewed line {index} is not an exact changed-side line") + path, line, side = location + raise NoemaModelOutputError( + f"Noema reviewed line {entry} cites {_format_location(path, line, side)}, " + f"which is not an exact changed-side line" + f"{_nearby_changed_locations(locations, path, line)}" + ) analysis = reviewed.get("analysis") if not isinstance(analysis, str) or not analysis.strip(): - raise NoemaModelOutputError(f"Noema reviewed line {index} requires concrete analysis") + raise NoemaModelOutputError(f"Noema reviewed line {entry} requires concrete analysis") validation = verdict.get("adversarial_validation") if not isinstance(validation, dict): @@ -465,22 +529,29 @@ def validate_substantive_verdict( confirmed: set[tuple[str, int, str]] = set() identities: set[tuple[Any, ...]] = set() - for index, probe in enumerate(probes, start=1): + probes_total = len(probes) + for position, probe in enumerate(probes, start=1): + entry = _entry_ordinal(position, probes_total) if not isinstance(probe, dict): - raise NoemaModelOutputError(f"Noema adversarial probe {index} must be an object") + raise NoemaModelOutputError(f"Noema adversarial probe {entry} must be an object") location = (probe.get("path"), probe.get("line"), probe.get("side")) if location not in locations: - raise NoemaModelOutputError(f"Noema adversarial probe {index} is not an exact changed-side line") + path, line, side = location + raise NoemaModelOutputError( + f"Noema adversarial probe {entry} cites {_format_location(path, line, side)}, " + f"which is not an exact changed-side line" + f"{_nearby_changed_locations(locations, path, line)}" + ) for field in ("hypothesis", "attack_or_counterexample", "evidence"): value = probe.get(field) if not isinstance(value, str) or not value.strip(): - raise NoemaModelOutputError(f"Noema adversarial probe {index} requires {field}") + raise NoemaModelOutputError(f"Noema adversarial probe {entry} requires {field}") outcome = probe.get("outcome") if outcome not in {"falsified", "confirmed"}: - raise NoemaModelOutputError(f"Noema adversarial probe {index} outcome must be falsified or confirmed") + raise NoemaModelOutputError(f"Noema adversarial probe {entry} outcome must be falsified or confirmed") identity = (*location, probe["hypothesis"].strip().casefold(), probe["attack_or_counterexample"].strip().casefold()) if identity in identities: - raise NoemaModelOutputError(f"Noema adversarial probe {index} duplicates an earlier probe") + 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"]))) diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 378bde85f9..c2bf379d40 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -2618,13 +2618,16 @@ def test_substantive_verdict_fail_closed_boundaries(): assert noema.validate_substantive_verdict({"decision": "comment"}, diff) is None invalid_cases = [ (lambda value: value.pop("reviewed_lines"), "at least one reviewed"), - (lambda value: value.update(reviewed_lines=[None]), "reviewed line 1 must be an object"), + (lambda value: value.update(reviewed_lines=[None]), r"reviewed line entry 1/1 \(array index 0.*must be an object"), (lambda value: value["reviewed_lines"][0].update(analysis=""), "requires concrete analysis"), (lambda value: value.pop("adversarial_validation"), "requires adversarial_validation"), (lambda value: value["adversarial_validation"].update(status="failed"), "status=passed"), (lambda value: value["adversarial_validation"].update(residual_risk=""), "requires residual_risk"), (lambda value: value["adversarial_validation"].update(probes=[]), "at least 2 concrete probe"), - (lambda value: value["adversarial_validation"].update(probes=[None, None]), "probe 1 must be an object"), + ( + lambda value: value["adversarial_validation"].update(probes=[None, None]), + r"adversarial probe entry 1/2 \(array index 0.*must be an object", + ), (lambda value: value["adversarial_validation"]["probes"][0].update(line=2), "not an exact changed-side line"), (lambda value: value["adversarial_validation"]["probes"][0].update(hypothesis=""), "requires hypothesis"), (lambda value: value["adversarial_validation"]["probes"][0].update(attack_or_counterexample=""), "requires attack_or_counterexample"), @@ -2639,6 +2642,128 @@ def test_substantive_verdict_fail_closed_boundaries(): noema.validate_substantive_verdict(candidate, diff) +def test_entry_ordinal_names_an_array_position_not_a_line_number(): + """Regression for naruon#1503: the label must read as an array position.""" + assert noema._entry_ordinal(1, 3) == "entry 1/3 (array index 0, not a source line)" + assert noema._entry_ordinal(3, 3) == "entry 3/3 (array index 2, not a source line)" + + +def test_format_location_reprs_every_raw_field(): + assert noema._format_location("a.py", 3, "RIGHT") == "path='a.py' line=3 side='RIGHT'" + # None/non-string/non-int values stay visibly distinguishable via repr(). + assert noema._format_location(None, "3", 7) == "path=None line='3' side=7" + + +def test_nearby_changed_locations_covers_every_branch(): + locations = { + ("a.py", 1, "RIGHT"), + ("a.py", 5, "RIGHT"), + ("a.py", 10, "LEFT"), + ("b.py", 2, "RIGHT"), + } + # Non-string path: nothing to compare against. + assert noema._nearby_changed_locations(locations, None, 5) == "" + # No changed location shares this path. + assert noema._nearby_changed_locations(locations, "missing.py", 5) == "" + # Int line: sorted nearest-first by distance from the rejected line. + hint = noema._nearby_changed_locations(locations, "a.py", 4) + assert hint == "; nearest changed lines for a.py: a.py:5 (RIGHT), a.py:1 (RIGHT), a.py:10 (LEFT)" + # Non-int line: falls back to ascending (line, side) order instead of distance. + hint_non_int = noema._nearby_changed_locations(locations, "a.py", "not-a-line") + assert hint_non_int == "; nearest changed lines for a.py: a.py:1 (RIGHT), a.py:5 (RIGHT), a.py:10 (LEFT)" + # More same-path locations than the display limit: truncated with a "+N more" tail. + many = {("c.py", line, "RIGHT") for line in range(1, 8)} + hint_many = noema._nearby_changed_locations(many, "c.py", 1, limit=5) + assert hint_many.endswith(", +2 more") + assert hint_many.count("(RIGHT)") == 5 + + +def test_validate_substantive_verdict_reports_rejected_location_and_nearby_hint(): + """The raised message must carry the actual rejected citation, not just a position.""" + diff = """diff --git a/tool.py b/tool.py +--- a/tool.py ++++ b/tool.py +@@ -1,3 +1,3 @@ + keep = 1 +-old = True ++new = True + tail = 2 +""" + verdict = { + "decision": "approve", + "summary": "The replacement keeps the invariant.", + "findings": [], + "reviewed_lines": [ + {"path": "tool.py", "line": 99, "side": "RIGHT", "analysis": "Wrong line cited."} + ], + "adversarial_validation": { + "status": "passed", + "residual_risk": "Callers were not executed.", + "probes": [], + }, + } + with pytest.raises(noema.NoemaModelOutputError) as exc_info: + noema.validate_substantive_verdict(verdict, diff) + message = str(exc_info.value) + assert "reviewed line entry 1/1 (array index 0, not a source line)" in message + assert "path='tool.py' line=99 side='RIGHT'" in message + assert "is not an exact changed-side line" in message + assert "nearest changed lines for tool.py: tool.py:2 (LEFT), tool.py:2 (RIGHT)" in message + + # A citation whose path was never touched by the diff gets no nearby hint. + verdict["reviewed_lines"][0]["path"] = "unrelated.py" + with pytest.raises(noema.NoemaModelOutputError) as exc_info_unrelated: + noema.validate_substantive_verdict(verdict, diff) + unrelated_message = str(exc_info_unrelated.value) + assert "path='unrelated.py'" in unrelated_message + assert "nearest changed lines" not in unrelated_message + + +def test_validate_substantive_verdict_probe_rejection_reports_location_and_hint(): + diff = """diff --git a/tool.py b/tool.py +--- /dev/null ++++ b/tool.py +@@ -0,0 +1 @@ ++new = True +""" + verdict = { + "decision": "approve", + "summary": "The replacement keeps the invariant.", + "findings": [], + "reviewed_lines": [{"path": "tool.py", "line": 1, "side": "RIGHT", "analysis": "Checked."}], + "adversarial_validation": { + "status": "passed", + "residual_risk": "Callers were not executed.", + "probes": [ + { + "path": "tool.py", + "line": 2, + "side": "RIGHT", + "hypothesis": "Off by one.", + "attack_or_counterexample": "Cite the wrong line.", + "evidence": "n/a", + "outcome": "falsified", + }, + { + "path": "tool.py", + "line": 1, + "side": "RIGHT", + "hypothesis": "A distinct second hypothesis.", + "attack_or_counterexample": "Read the correct line.", + "evidence": "The literal is True.", + "outcome": "falsified", + }, + ], + }, + } + with pytest.raises(noema.NoemaModelOutputError) as exc_info: + noema.validate_substantive_verdict(verdict, diff) + message = str(exc_info.value) + assert "adversarial probe entry 1/2 (array index 0, not a source line)" in message + assert "path='tool.py' line=2 side='RIGHT'" in message + assert "nearest changed lines for tool.py: tool.py:1 (RIGHT)" in message + + def test_changed_diff_locations_handles_new_files_and_no_newline_marker(): diff = """diff --git a/new.py b/new.py --- /dev/null