diff --git a/CHANGELOG.md b/CHANGELOG.md index 30eafe8250..042e913712 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,15 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- **Ground Noema formal-review coordinates in the one structured-output request.** + The trusted exact-line validator remains fail-closed, but the model now also receives a + compact, deterministic JSON manifest of every allowed path, side, and inclusive changed-line + range alongside the `response_format` schema. This prevents the model from guessing + coordinates out of whole-file context, as observed in `ContextualWisdomLab/bandscope#1122` + job `99792663163`, while preserving rejection of any reviewed line, adversarial probe, or + finding outside the parsed unified diff. (Grounds the single gateway-owned request rather + than a local repair retry -- Noema's own repository-owned retry was removed separately, see + "Noema single-request gateway ownership" above.) - **Catch scheduler target-list drift before it silently fails an hourly heartbeat.** `hourly-review-repair.yml`'s per-cron `target_repository` matrix and the `OPENCODE_REPOSITORY_DISPATCH_TARGETS` repository variable (which gates `ALLOWED_TARGET_REPOSITORIES` in `pr-review-merge-scheduler.yml`/`pr-review-fix-scheduler.yml`) are two independently hand-maintained lists with no structural link -- three repositories (`governance-risk-compliance`, `nonnest2`, `quarantine-sandbox-runtime`) were added to the hourly matrix without a corresponding variable update, so their hourly heartbeat failed closed with "target repository is not allowlisted" until each was found and fixed the same day. Added `scripts/ci/opencode_repository_dispatch_targets.json`, a hand-maintained mirror of the variable's live value, and a new contract test (`test_every_hourly_caller_target_is_in_the_dispatch_targets_mirror`) asserting every hourly-caller target is present in it, so a future PR that repeats the omission fails at review time instead of at the next silent hourly failure. See `docs/doctoring/scheduler-target-list-drift-20260902.md`. - **Fix a stale `test_strix_quick_gate.sh` assertion left broken by the `#1630` scheduler-cadence lengthening.** `pr-review-merge-scheduler.yml`'s repository-local diff --git a/docs/superpowers/plans/2026-09-01-noema-exact-changed-line-grounding.md b/docs/superpowers/plans/2026-09-01-noema-exact-changed-line-grounding.md new file mode 100644 index 0000000000..9403db25d2 --- /dev/null +++ b/docs/superpowers/plans/2026-09-01-noema-exact-changed-line-grounding.md @@ -0,0 +1,58 @@ +# Noema Exact Changed-Line Grounding Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Keep Noema's exact changed-side validator fail-closed while giving both the first LLM request and its one repair retry an authoritative, compact coordinate manifest. + +**Architecture:** Parse the bounded unified diff once, group exact `(path, line, side)` locations by file and side, and encode consecutive lines as inclusive ranges in compact JSON. Put that manifest in the prompt as the sole authority for `reviewed_lines`, adversarial probes, and finding coordinates; retain the existing deterministic post-response validator unchanged. + +**Tech Stack:** Python 3.14, pytest, GitHub Actions, OpenAI-compatible chat-completion transport. + +**Spec:** Live incident `ContextualWisdomLab/bandscope#1122`, Actions job `99792663163` (`Noema reviewed line 3 is not an exact changed-side line`). + +## Global Constraints + +- Do not coerce, snap, filter, or silently discard invalid model coordinates. +- Do not downgrade a formal verdict to a comment to obtain a green check. +- Keep retry count, reviewer identity, provider routing, merge authority, and current-head validation unchanged. +- The manifest must be deterministic, UTF-8 safe, and compact enough not to duplicate every changed line as a JSON object. + +--- + +### Task 1: Reproduce the ungrounded retry + +**Files:** +- Test: `tests/test_noema_review_gate.py` + +**Interfaces:** +- Consumes: `changed_diff_locations()` and `call_llm()`. +- Produces: regressions for range compression and for manifest presence on both initial and repair requests. + +- [ ] Add a mixed LEFT/RIGHT multi-file diff fixture and exact manifest expectation. +- [ ] Add a two-response LLM fixture whose first verdict fails specifically at reviewed-line item 3. +- [ ] Run the focused test before implementation and require the expected missing-interface failure. + +### Task 2: Ground formal review coordinates + +**Files:** +- Modify: `scripts/ci/noema_review_gate.py` +- Modify: `CHANGELOG.md` + +**Interfaces:** +- Produces: `_compact_line_ranges(lines: Sequence[int]) -> str` and `changed_line_manifest(locations: Sequence[tuple[str, int, str]]) -> str`. + +- [ ] Compress sorted line numbers into inclusive ranges. +- [ ] Serialize one deterministic JSON entry per path with only populated LEFT/RIGHT sides. +- [ ] Include the authoritative manifest in every request and point repair guidance at it. +- [ ] Leave `validate_substantive_verdict()` unchanged. + +### Task 3: Verify and publish + +**Files:** +- Remove: `.github/workflows/noema-coordinate-grounding-writer.yml` +- Remove: `scripts/ci/_temporary_apply_noema_coordinate_grounding.py` + +- [ ] Run the focused RED test and confirm the expected failure. +- [ ] Run the focused Noema suite after implementation. +- [ ] Run the full pytest/coverage/docstring/compile/diff gates. +- [ ] Preserve separate test-first and implementation commits and remove one-shot writer files. diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index ce90b8bc84..e4dbcaa1b6 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -531,6 +531,38 @@ def changed_diff_locations(diff: str) -> set[tuple[str, int, str]]: return locations +def _compact_line_ranges(lines: Sequence[int]) -> str: + """Compress sorted line numbers into comma-separated inclusive ranges.""" + if not lines: + return "" + ranges: list[str] = [] + start = previous = lines[0] + for line in lines[1:]: + if line == previous + 1: + previous = line + continue + ranges.append(str(start) if start == previous else f"{start}-{previous}") + start = previous = line + ranges.append(str(start) if start == previous else f"{start}-{previous}") + return ",".join(ranges) + + +def changed_line_manifest(locations: Sequence[tuple[str, int, str]]) -> str: + """Serialize exact changed-side locations as compact deterministic JSON.""" + grouped: dict[str, dict[str, set[int]]] = {} + for path, line, side in locations: + grouped.setdefault(path, {"LEFT": set(), "RIGHT": set()})[side].add(line) + entries: list[dict[str, str]] = [] + for path in sorted(grouped): + entry = {"path": path} + for side in ("LEFT", "RIGHT"): + lines = sorted(grouped[path][side]) + if lines: + entry[side] = _compact_line_ranges(lines) + entries.append(entry) + return json.dumps(entries, ensure_ascii=False, separators=(",", ":")) + + def parse_diff_path(raw: str, prefix: str) -> str: """Decode a Git unified-diff path, including C-quoted UTF-8 paths.""" value = raw.split("\t", 1)[0] @@ -1427,10 +1459,12 @@ def call_llm( ) reject_private_llm_url(api_url) + changed_locations = sorted(changed_diff_locations(diff)) allowed_locations = [ {"path": path, "line": line, "side": side} - for path, line, side in sorted(changed_diff_locations(diff)) + for path, line, side in changed_locations ] + location_manifest = changed_line_manifest(changed_locations) location_example = allowed_locations[0] if allowed_locations else { "path": "path", "line": 0, "side": "RIGHT" } @@ -1441,6 +1475,9 @@ def call_llm( "You are Noema, an independent pull request reviewer for ContextualWisdomLab.", "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.", + "Authoritative exact changed-side coordinate manifest (inclusive line ranges):", + location_manifest, + "For reviewed_lines.path, adversarial_validation.probes.path, and findings.file, copy a manifest path and side exactly and choose an integer line inside that side's ranges. This manifest, not additional context or visual line counting, is the sole coordinate authority.", "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.", "Use request_changes only for blocking, concrete issues. A generic no-issues statement is not review evidence.", f"Repository: {repo}", diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index ba65ba6b1f..bb070f1ad8 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -2362,3 +2362,145 @@ def test_parse_args_and_main(monkeypatch): noema.main( ["--repo", "owner/repo", "--pr-number", "9", "--expected-head", "A" * 40] ) + + + +def _coordinate_grounding_diff() -> str: + """Return a multi-file diff with sparse LEFT/RIGHT changed coordinates.""" + return """diff --git a/a.py b/a.py +index 1111111..2222222 100644 +--- a/a.py ++++ b/a.py +@@ -1,6 +1,7 @@ + one +-old-two ++new-two ++new-three + four +-old-five ++new-five + six +diff --git a/new.py b/new.py +new file mode 100644 +index 0000000..3333333 +--- /dev/null ++++ b/new.py +@@ -0,0 +1,3 @@ ++alpha ++beta ++gamma +""" + + +def test_compact_line_ranges_handles_empty_single_sparse_and_contiguous_values(): + """The manifest must compress coordinates without losing sparse boundaries.""" + assert noema._compact_line_ranges([]) == "" + assert noema._compact_line_ranges([9]) == "9" + assert noema._compact_line_ranges([1, 2, 4, 6, 7]) == "1-2,4,6-7" + + +def test_changed_line_manifest_compacts_exact_coordinates(): + """One path entry must preserve every parsed side-specific changed line.""" + diff = _coordinate_grounding_diff() + + manifest = json.loads( + noema.changed_line_manifest(sorted(noema.changed_diff_locations(diff))) + ) + + assert manifest == [ + {"path": "a.py", "LEFT": "2,4", "RIGHT": "2-3,5"}, + {"path": "new.py", "RIGHT": "1-3"}, + ] + assert noema.changed_line_manifest([]) == "[]" + + +def test_call_llm_grounds_its_single_request_in_authoritative_manifest(monkeypatch): + """Guard the BandScope #1122 root cause: the model's one request must carry + every changed-side coordinate, not just the raw diff and a schema example. + + Noema is single-request only (contextual-orchestrator owns repair/failover, + see "Noema single-request gateway ownership" in CHANGELOG.md), so unlike an + earlier draft of this fix there is no local repair retry to also ground -- + proving fail-closed rejection of an invented coordinate is already covered + generically by validate_substantive_verdict's own direct tests above; this + test only needs to prove the manifest reaches the one request Noema sends. + """ + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") + diff = _coordinate_grounding_diff() + valid = { + "decision": "approve", + "summary": "Both changed lines are independently reviewed.", + "reviewed_lines": [ + {"path": "a.py", "line": 2, "side": "RIGHT", "analysis": "Replacement keeps the value explicit."}, + {"path": "a.py", "line": 3, "side": "RIGHT", "analysis": "The inserted line is independently reviewed."}, + ], + "adversarial_validation": { + "status": "passed", + "residual_risk": "The fixture does not execute product code.", + "probes": [ + { + "path": "a.py", + "line": 2, + "side": "RIGHT", + "hypothesis": "The replacement can erase the expected value.", + "attack_or_counterexample": "Trace the changed assignment through its caller.", + "evidence": "The caller still receives the explicit replacement value.", + "outcome": "falsified", + }, + { + "path": "a.py", + "line": 3, + "side": "RIGHT", + "hypothesis": "The inserted line can change ordering semantics.", + "attack_or_counterexample": "Compare execution order before and after the insertion.", + "evidence": "The insertion runs in the intended sequence.", + "outcome": "falsified", + }, + ], + }, + "findings": [], + } + requests = [] + + class Response: + """Return one OpenAI-compatible response envelope.""" + + def __enter__(self): + return self + + def __exit__(self, *args): + return None + + def read(self): + return json.dumps( + {"choices": [{"message": {"content": json.dumps(valid)}}]} + ).encode() + + def open_response(_opener, request, **_kwargs): + """Capture the single request payload while serving the deterministic response.""" + requests.append(request) + return Response() + + monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr(headRefOid="head")) + + verdict = noema.call_llm( + "owner/repo", + 7, + make_pr(headRefOid="head"), + diff, + False, + "head", + changed_paths=("a.py", "new.py"), + ) + + assert verdict["decision"] == "approve" + assert len(requests) == 1 + prompt = json.loads(requests[0].data)["messages"][1]["content"] + expected_manifest = noema.changed_line_manifest( + sorted(noema.changed_diff_locations(diff)) + ) + assert "Authoritative exact changed-side coordinate manifest" in prompt + assert expected_manifest in prompt + assert "sole coordinate authority" in prompt