From 8e6ef508df6fff36ac681ea96b2cc01f749b67d8 Mon Sep 17 00:00:00 2001 From: Magnus Hedemark Date: Wed, 7 Oct 2026 13:15:12 -0400 Subject: [PATCH] fix: preserve review follow-up scope and compact coverage reports --- docs/CURRENT-STATUS.md | 20 ++++- docs/experiments/pr305-v16-2026-10-07.md | 52 ++++++++++++ src/pr_review_harness/engine.py | 7 +- src/pr_review_harness/report.py | 50 +++++++++-- tests/test_engine.py | 55 ++++++++++++ tests/test_report.py | 102 ++++++++++++++++++++++- 6 files changed, 275 insertions(+), 11 deletions(-) create mode 100644 docs/experiments/pr305-v16-2026-10-07.md diff --git a/docs/CURRENT-STATUS.md b/docs/CURRENT-STATUS.md index 5b5434b..ddc5d55 100644 --- a/docs/CURRENT-STATUS.md +++ b/docs/CURRENT-STATUS.md @@ -1,6 +1,22 @@ -# Current evidence update — 2026-10-07 (source cutoff `6f67aea`; latest trial runtime `4fbc3b0`) +# Current evidence update — 2026-10-07 (source cutoff `1d4903d`; latest trial runtime `0379e20`) -## Current observation +## Latest v16 PR305 observation + +Source PRs [#217](https://github.com/groktopus/codereview/pull/217) and [#218](https://github.com/groktopus/codereview/pull/218) are merged. The current caller is `1d4903dab34b0c51e8f3805803872c3b98e2a96c`; it pins the reusable runtime to `0379e2001eebc07981aa99553e832ea342d781a7`. Both PRs passed their enabled CI checks and their post-merge runs succeeded. The caller keeps `legacy-v14` as the default and offers an explicit closed `bounded-production-v16` option. + +The authorized v16 run [37655393958](https://github.com/groktopus/codereview/actions/runs/37655393958) reviewed SlopSearX PR #305 at base `00accc58a42eaa470e12831498b572cab2483981` and head `90165dd65be3595006171d7abef34a082c90a71d`. A live GitHub API comparison returned ahead 1 / behind 0, merge-base equal to the API base, and 12 changed files; the result freshness was `CURRENT`. Its profile binding selected `bounded-production-v16` (`699f93fd…`), limits-v3 (`ec191dc…`), and a four-candidate claim-assessment cap. The retained report and artifact hashes are recorded in [the dated PR305 evidence note](experiments/pr305-v16-2026-10-07.md). + +The workflow completed successfully, but the review result was `INCOMPLETE / PARTIAL`: 27 of 42 required rows were complete, 15 partial, and none not started. The partial rows were 11 changed-unit rows with `CONTEXT_GAP_UNRESOLVED`, two with `PARTIAL_REVIEW_COVERAGE`, and two portal check rows with `CHECK_RESULT_UNAVAILABLE`. The seven retrieval records comprise five truncated IPC envelopes, one retrieved `engines/__init__.py` whose follow-up failed `invalid_context_followup_obligation_binding`, and one symbol lookup rejected by `symbol_lookup_not_in_allowlist_contract`; these are observed alongside the row reasons, not assumed to map one-to-one. The 12 snapshot gaps were optional and are separate. The run emitted no findings or claim candidates, so LLM claim reconciliation and native TypeSafe Choice claim assessment were `NOT_EXERCISED`. One TypeSafe System 1 advisory summary was received with `UNRESOLVED`; it is not a claim assessment or truth signal. No prompt-injection diagnostic appeared (`NOT_EXERCISED`). + +Sixteen unique HTTP 200 receipts were retained: 15 primary OpenAI-compatible calls and one TypeSafe advisory call, each with one attempt. Usage was known for the 15 primary calls (285,570 prompt tokens and 8,386 completion tokens) and unknown for the advisory; billing and estimated cost remain `UNKNOWN`. The ledger reserved 16/32 provider calls and 1,250,619/2,500,000 aggregate input bytes: 1,116,935 provider-request bytes, 112,000 bytes for seven local context retrievals, and 21,684 bytes for two local checks. Aggregate output reservation was 288,000/512,000 bytes, comprising 256,000 provider-output bytes and 32,000 local-check output bytes. It made no target-code execution or publication; optional publication-intake artifact steps were skipped. + +A separate provider-free preview used this run's result with the private COMMENT-only publication policy. The protected consumer maps `INCOMPLETE` with `policy_valid=true` to a COMMENT event; this result's 62,777-byte rendered body exceeded the 60,000-byte hard cap, so preview stopped at the body-size check. Nothing was published. This is a concrete report-size boundary for this run, not evidence of semantic quality or a new acceptance threshold. + +The target activation packet remains private and unapplied. The installed target workflow/policy baseline is still `2abbf4924609c29f486d4a3550c26878e3d74dad`. A successful read-only GitHub secrets listing showed only `LLM_API_KEY` and `JEV_API_KEY` absent. No activation, target write, provider setting change, publication, or cutover is evidenced here. + +## Prior current observation retained (source cutoff `6f67aea`; trial runtime `4fbc3b0`) + +### Current observation at that cutoff PR #207 merged at `4fbc3b0a4e45596c3c73685edc8be575e2173e27` after 23 enabled checks passed, the disabled probe skipped, independent review cleared 112 tests, and root verification passed 124 tests. Reviewed and merged source files were identical. Post-merge run [37612949630](https://github.com/groktopus/codereview/actions/runs/37612949630) completed successfully. Source/package verification does not establish review quality. diff --git a/docs/experiments/pr305-v16-2026-10-07.md b/docs/experiments/pr305-v16-2026-10-07.md new file mode 100644 index 0000000..3a7afa0 --- /dev/null +++ b/docs/experiments/pr305-v16-2026-10-07.md @@ -0,0 +1,52 @@ +# SlopSearX PR305 v16 bounded review — 2026-10-07 + +## Scope and binding + +This note records one authorized, read-only v16 review run. It is operational evidence from one natural PR, not an accuracy, reliability, or replacement assessment. + +Source PR #217 merged the reusable runtime at `0379e2001eebc07981aa99553e832ea342d781a7`; caller PR #218 merged at `1d4903dab34b0c51e8f3805803872c3b98e2a96c`. Both passed enabled CI and post-merge runs. The caller defaults to `legacy-v14` and exposes only the closed `legacy-v14` and `bounded-production-v16` choices. + +Run [37655393958](https://github.com/groktopus/codereview/actions/runs/37655393958), attempt 1, selected target `magnus919/SlopSearX` PR #305. Its live API identity was base `00accc58a42eaa470e12831498b572cab2483981`, head `90165dd65be3595006171d7abef34a082c90a71d`; the compare API reported ahead 1, behind 0, merge-base equal to base, and 12 changed files. The report marked freshness `CURRENT`. + +The retained profile binding names `bounded-production-v16`, profile `profiles/slopsearx-v16-bounded-production-candidate.json` SHA-256 `699f93fd7bc78d310de2455cad7402a1c3c8cf5ad61c3d4cbb77bb38da1189ed`, limits `profiles/ordinary-review-limits-v3.json` SHA-256 `ec191dcde1672b4f86aac24ee6412752cd9a08871d1aba1b332d11341b6fd2a6`, and claim cap 4. The call ceiling was 32, aggregate provider input 2,500,000 bytes, per-task input 96,000 bytes, aggregate output 512,000 bytes, and retries 0. + +## Observed outcome + +The Actions run concluded successfully; the review result was `INCOMPLETE`, coverage `PARTIAL`, and merge eligibility `NOT_EVALUATED`. There were 42 required rows: 27 `COMPLETE`, 15 `PARTIAL`, and 0 `NOT_STARTED`. The 15 partial rows were 11 changed-unit rows with `CONTEXT_GAP_UNRESOLVED`, two changed-unit rows with `PARTIAL_REVIEW_COVERAGE`, and two portal checks with `CHECK_RESULT_UNAVAILABLE`. The 12 snapshot gaps were optional and are not included in the required-row count. + +Seven requested context-retrieval records remained unresolved: + +- Five retrievals ended `PARTIAL / ipc_envelope_truncated`, returning 14,282–14,632 bytes in envelopes capped at 16,000 bytes. +- One `engines/__init__.py` retrieval returned 1,688 bytes of a 2,734-byte envelope, but follow-up validation failed with `invalid_context_followup_obligation_binding`. +- One symbol-only request for the Exa adapter implementation returned no bytes and ended `UNRESOLVED / symbol_lookup_not_in_allowlist_contract`. + +These retrievals consumed seven of eight context-retrieval reservations and zero provider calls. The follow-up binding rejection is a concrete runtime protocol defect; the truncated and unsupported-symbol records remain explicit partial/unresolved evidence. No retry occurred. + +The run emitted zero findings and zero claim candidates. LLM claim reconciliation and native TypeSafe Choice claim assessment were `NOT_EXERCISED`; a four-candidate cap is not evidence that any candidate was assessed. The distinct System 1 advisory assessment was received from TypeSafe, used the Noul primitive, and recommended `UNRESOLVED` at probability 0.67; that uncalibrated advisory is not a claim verdict. No prompt-injection diagnostic appeared in the retained report (`NOT_EXERCISED`). + +## Calls, usage, and effects + +There were 16 unique provider HTTP receipts: 15 OpenAI-compatible primary review requests and one TypeSafe advisory request. Every receipt had HTTP 200 and one attempt. Usage was reported for the 15 primary calls: 285,570 prompt tokens and 8,386 completion tokens (293,956 total). Advisory usage was unknown. Billing and estimated cost were `UNKNOWN`; zero-valued billing fields are accompanied by `cost_billing_known=false` and do not establish zero spend. + +The ledger’s 1,250,619/2,500,000 aggregate input reservation comprises 1,116,935 provider-request bytes, 112,000 bytes reserved for seven local context retrievals, and 21,684 bytes for two local checks. The 288,000/512,000 aggregate output reservation already includes 256,000 provider-output bytes and 32,000 local-check output bytes; it does not include another 32,000 bytes on top. There were no budget breaches. The two local check rows were unavailable in the external GitHub check inventory, so they remain partial. + +The reusable workflow's permissions were read-only for contents, checks, and pull requests. It reviewed a bare target object store and did not execute target code. Optional publication-intake artifact steps were skipped; no target review was published. A provider-free preview against the private COMMENT-only policy mapped `INCOMPLETE` with `policy_valid=true` to COMMENT, then rejected the 62,777-byte rendered report at the hard 60,000-byte body limit. This preview had no external effect and establishes only the current report-size boundary. + +## Artifact integrity + +The retained artifact was downloaded to `/private/tmp/codereview-v16-pr305-37655393958-artifacts/pr-review-305-37655393958-1`. Its manifest inventory verified all five retained files: + +| File | SHA-256 | +|---|---| +| `profile-binding.json` | `5c643f03c06256aaac06a27f40fa2611ade205ed78ebe4655f596432160338ac` | +| `recovery-manifest.json` | `8eee8ac86a078505d993243b05490d5d1f41256eaa58cfebce8b3f9e3b813211` | +| `review-result.json` | `dc8872865a1aeb8ad2b7e1654027e62294ef8f6439c2cf57286df7f2340136aa` | +| `review/pr-305-37655393958.json` | `2f87d5a426d8a1b304ce9c810341fb817eeeff7415eaaa91da01c37fd4bddbf4` | +| `review/pr-305-37655393958.md` | `feee9e068c06999f53b8b432c46a36efdf238ec8c09681503a3026f08d0175b6` | +| `recovery-inputs.json` | `a614b7424d43efcc52eacef249c1ef8f43423fc35feec6fe6c1aeee3a5585773` | + +The top-level manifest SHA-256 is `8eee8ac86a078505d993243b05490d5d1f41256eaa58cfebce8b3f9e3b813211`. It binds the run, attempt, caller/harness SHA, target PR, base/head, profile version/hash, and snapshot ID. The manifest records provider-configuration hashes; the checkpoint also retains non-secret endpoint, model, and adapter identity plus credential-reference names, not credential values. + +## Limitations + +This run does not establish semantic accuracy, native claim agreement, blocker truth, prompt-injection resistance, or production quality. Zero candidates means claim stages were not exercised, not that the changes were safe. The review remained incomplete, usage/billing was partly unknown, and the private preview did not publish. No human-label, second-project, or new numeric-quality gate is introduced by this note. diff --git a/src/pr_review_harness/engine.py b/src/pr_review_harness/engine.py index a4e0d65..13dfa39 100644 --- a/src/pr_review_harness/engine.py +++ b/src/pr_review_harness/engine.py @@ -2645,13 +2645,16 @@ def run_followup(task: dict, evidence: list[dict]) -> dict: if retrieved and retrieved.get("status") == "RESOLVED" and record.get("retrieved_evidence_ids"): followup_id = f"{task['task_id']}:followup:{index}" followup_obligation_id = f"context:{record['proposal_id']}" + followup_unit_ids = ( + [target_unit] if target_unit else list(task.get("unit_ids", task.get("scope_unit_ids", []))) + ) followup_task = { **task, "task_id": followup_id, "obligation_id": followup_obligation_id, "obligation_ids": [followup_obligation_id], - "unit_ids": [target_unit] if target_unit else list(task_units), - "scope_unit_ids": [target_unit] if target_unit else list(task_units), + "unit_ids": followup_unit_ids, + "scope_unit_ids": followup_unit_ids, "evidence_ids": list( dict.fromkeys(list(task.get("evidence_ids", [])) + record["retrieved_evidence_ids"]) ), diff --git a/src/pr_review_harness/report.py b/src/pr_review_harness/report.py index 186ce31..8e1be5e 100644 --- a/src/pr_review_harness/report.py +++ b/src/pr_review_harness/report.py @@ -2,8 +2,6 @@ from __future__ import annotations -import html -import json from typing import Any from urllib.parse import quote, urlparse @@ -18,6 +16,11 @@ def safe(value: Any) -> str: value = value.replace(char, "\\" + char) return value + def string_values(value: Any) -> list[str]: + if not isinstance(value, list): + return [] + return [item for item in value if isinstance(item, str) and item] + def source_link(path: str, side: str, line: int | None) -> str: repository = result.get("repository_url") or result.get("snapshot", {}).get("repository_url") revision = ( @@ -162,10 +165,47 @@ def source_link(path: str, side: str, line: int | None) -> str: lines.append( f"{unresolved_findings} unverified claims remain in the durable result and are not presented as recommendations." ) - lines.extend(("", "
", f"Full coverage ledger ({len(coverage_rows)} obligations)", "")) + evidence_ids = sorted( + { + evidence_id + for row in coverage_rows + for evidence_id in string_values(row.get("evidence_refs")) + } + ) + evidence_labels = {evidence_id: f"E{index:04d}" for index, evidence_id in enumerate(evidence_ids, start=1)} + lines.extend( + ( + "", + "
", + f"Coverage ledger ({len(coverage_rows)} obligations)", + "", + "| Required | Obligation | Lens | Scope units | State | Reason | Tasks | Context gaps | Evidence refs |", + "| --- | --- | --- | --- | --- | --- | --- | --- | --- |", + ) + ) for row in coverage_rows: - serialized = json.dumps(row, ensure_ascii=False, sort_keys=True, separators=(",", ":")) - lines.extend(("
", html.escape(serialized, quote=True), "
")) + row_evidence_ids = string_values(row.get("evidence_refs")) + evidence_refs = ", ".join( + evidence_labels[evidence_id] + for evidence_id in row_evidence_ids + if evidence_id in evidence_labels + ) + cells = ( + safe(row.get("required", True)), + safe(row.get("obligation_id", "")), + safe(row.get("lens", "")), + safe(", ".join(string_values(row.get("scope_unit_ids")))), + safe(row.get("state", "")), + safe(row.get("reason_code", "")), + safe(", ".join(string_values(row.get("task_ids")))), + safe(", ".join(string_values(row.get("context_gap_ids")))), + safe(evidence_refs), + ) + lines.append("| " + " | ".join(cells) + " |") + if evidence_labels: + lines.extend(("", "Evidence reference index:")) + for evidence_id in evidence_ids: + lines.append(f"- {evidence_labels[evidence_id]} = {safe(evidence_id)}") lines.extend(("", "
")) return "\n".join(lines).rstrip() + "\n" diff --git a/tests/test_engine.py b/tests/test_engine.py index 080b126..97e8944 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -571,6 +571,30 @@ def review(self, task, evidence, limits): } +class ExternalPathContextFollowupProvider(ContextFollowupProvider): + def review(self, task, evidence, limits): + if not task.get("context_gap_followup_for"): + result = super().review(task, evidence, limits) + result["context_gap_proposals"][0]["target"]["target_path"] = "engines/__init__.py" + return result + + self.calls += 1 + retrieved_id = next(item["evidence_id"] for item in evidence if item.get("path") == "engines/__init__.py") + notes = [] + for unit_id in task["unit_ids"]: + binding = next(row for row in task["unit_evidence_bindings"] if row["unit_id"] == unit_id) + notes.append( + { + "unit_id": unit_id, + "state": "COVERED", + "reason_code": "reviewed_with_external_context", + "evidence_refs": [retrieved_id, *binding["evidence_ids"]], + "coverage_basis": "STATIC_REVIEW", + } + ) + return {"finding_candidates": [], "context_gap_proposals": [], "coverage_notes": notes} + + class RequiredContextClosureFixtureProvider(ContextFollowupProvider): """Exercise the real engine closure reducer with bounded synthetic notes.""" @@ -1934,6 +1958,37 @@ def test_retrieval_requires_bounded_followup_that_cites_retrieved_evidence(tmp_p assert context_coverage["closure_diagnostics"]["coverage_note_failure_counts"]["MATCH"] >= 1 +def test_external_path_followup_preserves_multi_unit_parent_scope_order(tmp_path): + snapshot = make_snapshot(units=2) + unit_by_id = {unit["unit_id"]: unit for unit in snapshot["inventory"]} + parent_order = list(unit_by_id) + assert len(set(parent_order)) == len(parent_order) + snapshot["inventory"] = [unit_by_id[unit_id] for unit_id in parent_order] + prof = {**profile(), "retrieval_context_patterns": ["engines/*.py"]} + + result = run( + tmp_path, + snap=snapshot, + prof=prof, + provider=ExternalPathContextFollowupProvider(), + context_retriever=ResolvedContextRetriever(), + ) + + gap = result["context_gaps"][0] + assert gap["retrieval_status"] == "RESOLVED" + assert "followup_error" not in gap + assert gap["status"] == "RESOLVED_BY_FOLLOWUP" + followup = result["ledger"]["dynamic_tasks"][0] + assert followup["unit_ids"] == parent_order + metadata = followup["context_followup"] + obligation = next( + item for item in result["ledger"]["dynamic_obligations"] if item["obligation_id"] == metadata["followup_obligation_id"] + ) + assert metadata["scope_unit_ids"] == parent_order + assert obligation["scope_unit_ids"] == parent_order + assert result["budget"]["followup_tasks_reserved"] == 1 + + @pytest.mark.parametrize( ("followup_state", "mutation", "expected_note_code", "expected_coverage"), [ diff --git a/tests/test_report.py b/tests/test_report.py index 2eee1e9..ab1a60e 100644 --- a/tests/test_report.py +++ b/tests/test_report.py @@ -316,11 +316,109 @@ def test_partial_report_keeps_canonical_blocker_and_full_coverage_ledger_visible assert r"PR review coverage-report-test: REQUEST\_CHANGES" in rendered assert "0 complete, 11 partial, 0 not started across 11 obligations." in rendered assert "3 more unresolved obligations are listed in the full coverage ledger below." in rendered - assert "Full coverage ledger (11 obligations)" in rendered - assert "obligation-<script>alert(1)</script>" in rendered + assert "Coverage ledger (11 obligations)" in rendered + assert "Coverage ledger (11 obligations)" in rendered + assert r"obligation-\alert\(1\)\" in rendered assert "task-secret-looking-but-not-secret" in rendered assert "## Blockers" in rendered and "Reject invalid caller input" in rendered assert rendered.count("## ") == 4 + assert "
" not in rendered
+
+
+def test_compact_ledger_preserves_all_obligations_and_stable_evidence_mapping():
+    result = blocker_result()
+    result["coverage_ledger"] = []
+    states = ("COMPLETE", "PARTIAL", "NOT_STARTED")
+    lenses = ("correctness", "tests", "security", "project_specific")
+    for index in range(48):
+        refs = [f"ev-{(index + offset) % 53:03d}" for offset in range(4)]
+        result["coverage_ledger"].append(
+            {
+                "obligation_id": f"unit:unit-{index:03d}:lens:{lenses[index % len(lenses)]}",
+                "obligation_kind": "CHANGED_UNIT_LENS" if index % 3 else "REQUIRED_CONTEXT",
+                "required": index % 7 != 0,
+                "scope_unit_ids": [f"unit-{index:03d}"],
+                "lens": lenses[index % len(lenses)],
+                "task_ids": [f"task-{index:03d}:chunk-{index % 2 + 1}"],
+                "state": states[index % len(states)],
+                "reason_code": "PARTIAL_REVIEW_COVERAGE" if index % 3 else "café_context_gap_",
+                "context_gap_ids": [f"gap-{index:03d}"] if index % 3 else [],
+                "evidence_refs": refs,
+            }
+        )
+
+    rendered = render_report(result)
+    evidence_ids = sorted(
+        {
+            evidence_id
+            for row in result["coverage_ledger"]
+            for evidence_id in row["evidence_refs"]
+        }
+    )
+    header = [
+        "Required",
+        "Obligation",
+        "Lens",
+        "Scope units",
+        "State",
+        "Reason",
+        "Tasks",
+        "Context gaps",
+        "Evidence refs",
+    ]
+    table_rows = {}
+    in_table = False
+    for line in rendered.splitlines():
+        if line.startswith("| Required | Obligation |"):
+            in_table = True
+            continue
+        if in_table and line.startswith("| --- |"):
+            continue
+        if in_table and line.startswith("| "):
+            cells = [cell.strip() for cell in line.strip("|").split("|")]
+            table_rows[cells[1]] = dict(zip(header, cells, strict=True))
+            continue
+        if in_table:
+            break
+    evidence_index = {
+        alias: evidence_id
+        for line in rendered.splitlines()
+        if line.startswith("- E")
+        for alias, evidence_id in [line[2:].split(" = ", 1)]
+    }
+    assert evidence_index == {
+        f"E{position:04d}": evidence_id
+        for position, evidence_id in enumerate(evidence_ids, start=1)
+    }
+
+    def safe_cell(value):
+        value = str(value if value is not None else "").replace("\r", " ").replace("\n", " ")
+        for char in ("|", "<", ">", "`", "[", "]", "(", ")", "#", "*", "_", "!"):
+            value = value.replace(char, "\\" + char)
+        return value
+
+    assert len(rendered.encode("utf-8")) < 60_000
+    assert "Coverage ledger (48 obligations)" in rendered
+    assert "Coverage ledger (48 obligations)" in rendered
+    assert "
" not in rendered
+    assert r"café\_context\_gap\_\" in rendered
+    assert "| Required | Obligation | Lens | Scope units | State | Reason | Tasks | Context gaps | Evidence refs |" in rendered
+    assert "## Blockers" in rendered and "Reject invalid caller input" in rendered
+    for row in result["coverage_ledger"]:
+        rendered_row = table_rows[safe_cell(row["obligation_id"])]
+        assert rendered_row["Required"] == safe_cell(row["required"])
+        assert rendered_row["Lens"] == safe_cell(row["lens"])
+        assert rendered_row["Scope units"] == safe_cell(", ".join(row["scope_unit_ids"]))
+        assert rendered_row["State"] == safe_cell(row["state"])
+        assert rendered_row["Reason"] == safe_cell(row["reason_code"])
+        assert rendered_row["Tasks"] == safe_cell(", ".join(row["task_ids"]))
+        assert rendered_row["Context gaps"] == safe_cell(", ".join(row["context_gap_ids"]))
+        rendered_refs = [
+            evidence_index[alias]
+            for alias in rendered_row["Evidence refs"].split(", ")
+            if alias
+        ]
+        assert rendered_refs == row["evidence_refs"]
 
 
 def test_report_rejects_malformed_coverage_ledger_instead_of_rendering_partial_inventory():