Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 81 additions & 10 deletions scripts/ci/noema_review_gate.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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):
Expand All @@ -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"])))
Expand Down
129 changes: 127 additions & 2 deletions tests/test_noema_review_gate.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand All @@ -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
Expand Down
Loading