feat(review-fix-loop): implement reviewer isolation and complete-review orchestration

CHANGELOG.md

@@ -4,8 +4,57 @@ summary: Chronological history of repository and skill changes.
 
 # Changelog
 
-## 2026-07-30 — Implemented the review-fix-loop local execution substrate (common-directory locking, isolated attempts, checkpoint persistence, and recovery), defined the review-fix-loop invocation, checkpoint, and terminal-result contracts, removed the unproven verification-sufficiency pass and its required-evidence field from review-correctness, and simplified the review-fix-loop design around local coordination and Git-native publication safety
+## 2026-07-30 — Implemented the review-fix-loop reviewer isolation and complete-review orchestration and local execution substrate (common-directory locking, isolated attempts, checkpoint persistence, and recovery), defined the review-fix-loop invocation, checkpoint, and terminal-result contracts, removed the unproven verification-sufficiency pass and its required-evidence field from review-correctness, and simplified the review-fix-loop design around local coordination and Git-native publication safety
 
+- fix(review-fix-loop): remove three subsumed/redundant tests and cut
+  `reviewer_orchestration.py`'s docstring/comment density from roughly one line
+  of prose per line of code down to sibling-module levels, replacing restated
+  rationale with single pointers to `references/reviewer-orchestration.md`,
+  closing the two code-simplicity gaps the sixth review-code-change pass on #98
+  found
+- fix(review-fix-loop): correct an off-by-one changelog SHA attribution left by
+  the previous fix cycle's own rebase cleanup — the duplicate-test entry and the
+  `ignored`-comparison entry each carried the other's identity, closing the gap
+  the fifth review-code-change pass on #98 found
+  (`1945f82979bb3a0e6993c0326fdc9caad7391964`)
+- fix(review-fix-loop): rebase onto the merged #97 local-execution substrate,
+  document that a mutation attributable to a review pass must stop the
+  invocation with `blocked/reviewer_integrity_failure` immediately rather than
+  only relying on the `write_isolation`/`converged`-rejection backstop, and
+  correct a changelog SHA left stale by that same rebase, closing the two gaps
+  the fourth review-code-change pass on #98 found; the extracted
+  `review_gate.evaluate_bound` reuse (cycle 1) is kept as the deliberate design
+  after correctness confirmed it changes no accept/reject outcome for
+  `implement-ticket`/`babysit-pr` (`2596f72cd4886b2d5ba385ee33a51353279fe995`)
+- fix(review-fix-loop): remove a byte-identical duplicate test and trim
+  history-narrating/triplicated docstring prose in `reviewer_orchestration.py`
+  and `reviewer-orchestration.md` down to one owner per rationale, closing the
+  two code-simplicity gaps the third review-code-change pass on #98 found
+  (`7663b2cb36320287d9c2c9e820d4fb1745f4c5b2`)
+- fix(review-fix-loop): stop comparing `ignored` worktree state for reviewer
+  mutation (authorized validation commands legitimately create ignored build
+  artifacts, which previously made `converged` unreachable), fail closed instead
+  of silently passing when a before/after snapshot omits a required capture key,
+  and collapse the packet-less `evaluate_review_result` path into the single
+  packet-plus-result evaluator (`review-fix-loop`'s own
+  checkpoint/terminal-result contract never persists one without the other, so
+  no caller can legitimately use the weaker path), closing the two blocking gaps
+  and the one strong-recommendation gap the second review-code-change pass on
+  #98 found (`18991dd231ce5272b9a4b3335529418e6a717057`)
+- fix(review-fix-loop): detect refs mutation (not only `head_sha`) between
+  before/after reviewer snapshots, reconcile the packet/result evaluator with a
+  contract-legal identity-omitting `blocked` result while still binding the
+  packet itself to the current candidate, and extract the canonical
+  `review_gate.evaluate_bound` (bundled into `implement-ticket`, `babysit-pr`,
+  and now `review-fix-loop`) instead of a second candidate-binding
+  implementation, closing the three gaps the first review-code-change pass on
+  #98 found (`a519ae9f4e42551feba08146b465c5c526188e8b`)
+- feat(review-fix-loop): implement reviewer isolation and complete-review
+  orchestration — fixed lens resolution, default fresh-subagent review execution
+  with an explicit in-agent override, before/after mutation detection that fails
+  a cycle closed, checkpoint-shaped review-record construction, and
+  deterministic finding normalization/selection (#98)
+  (`086677ab59b219bccc009b9eb08dc67f3f613758`)
 - feat(review-fix-loop): add `scripts/local_execution.py` implementing #97's
   local execution substrate — non-blocking common-Git-common-directory candidate
   locking (local-ref lock before the optional `update_pr` remote-target lock,
@@ -18,7 +67,7 @@ summary: Chronological history of repository and skill changes.
   interrupted attempt against a checkpoint's own history — with deterministic
   tests against real temporary Git repositories covering contention,
   interruption, stale state, dirty worktrees, promotion races, and cleanup
-  safety
+  safety (`26b4cf47168dc8432f7d6e5e4597439af6391a51`)
 - fix(review-fix-loop): reject `converged` when any `review_records` entry — not
   only the final-head-bound one — recorded a mutation attempt, closing the gap
   the tenth (final) review-code-change pass on #96 found

justfile

@@ -12,7 +12,7 @@ list-skills:
 # each caller that consumes a review-code-change result, so every skill stays
 # self-contained when installed outside this repository.
 sync-contracts:
-  @for skill in review-code-change review-correctness review-code-simplicity review-solution-simplicity implement-ticket babysit-pr; do \
+  @for skill in review-code-change review-correctness review-code-simplicity review-solution-simplicity implement-ticket babysit-pr review-fix-loop; do \
     dest="{{skills_dir}}/$skill/references/review-suite"; \
     mkdir -p "$dest"; \
     cp review-suite/CONTRACT.md "$dest/CONTRACT.md"; \
@@ -21,7 +21,7 @@ sync-contracts:
     cp review-suite/scripts/validate.py "$dest/validate.py"; \
     echo "Synced $dest"; \
   done
-  @for skill in implement-ticket babysit-pr; do \
+  @for skill in implement-ticket babysit-pr review-fix-loop; do \
     scripts_dest="{{skills_dir}}/$skill/scripts"; \
     tests_dest="$scripts_dest/tests"; \
     mkdir -p "$tests_dest"; \

review-suite/scripts/review_gate.py

@@ -12,6 +12,13 @@
 those stay owned by repository-owned `review-code-change`. It only refuses to
 let this caller treat a stale, malformed, non-aggregate, non-clean, or
 wrongly-bound result as publishable evidence.
+
+`evaluate_aggregate` is for a caller that only ever wants to consume `clean`
+evidence (`implement-ticket`, `babysit-pr`). `evaluate_bound` is the same
+schema/semantic validation plus candidate/lens-execution binding without the
+clean-verdict requirement, for a caller that must also react to
+`changes_required` or `blocked` (for example `review-fix-loop`, which runs a
+review pass expecting exactly that most of the time).
 """
 
 from __future__ import annotations
@@ -50,6 +57,78 @@ def _validate_module_path() -> Path:
 VALIDATE_SPEC.loader.exec_module(VALIDATE)
 
 
+def _binding_errors(
+    result: dict[str, Any], expected_head: str, expected_base: str
+) -> list[str]:
+    """Return aggregate-lens and candidate/lens-execution binding errors.
+
+    Assumes `result` already passed schema/semantic validation. Shared by
+    `evaluate_bound` and `evaluate_aggregate` so both stay consistent as this
+    binding logic evolves.
+
+    A `blocked` result may legitimately omit candidate identity entirely (the
+    shared review-suite contract allows this when the caller could not
+    establish it); this only compares identity when the result actually
+    asserts some, so an empty `blocked` candidate is not mistaken for a
+    stale-candidate mismatch.
+    """
+    errors: list[str] = []
+    if result.get("lens") != "aggregate":
+        errors.append(f"lens: expected an aggregate result, got {result.get('lens')!r}")
+
+    candidate = result.get("candidate") or {}
+    if (
+        candidate.get("head_sha") is not None
+        or candidate.get("comparison_base_sha") is not None
+    ):
+        if (
+            candidate.get("head_sha") != expected_head
+            or candidate.get("comparison_base_sha") != expected_base
+        ):
+            errors.append(
+                "candidate: result is not bound to the current candidate "
+                f"(expected head {expected_head} / base {expected_base}, got "
+                f"head {candidate.get('head_sha')!r} / "
+                f"base {candidate.get('comparison_base_sha')!r})"
+            )
+
+    for execution in result.get("lens_executions") or []:
+        if not isinstance(execution, dict):
+            continue
+        if (
+            execution.get("head_sha") != expected_head
+            or execution.get("comparison_base_sha") != expected_base
+        ):
+            errors.append(
+                f"lens_executions: {execution.get('lens')!r} execution is not "
+                "bound to the current candidate"
+            )
+
+    return errors
+
+
+def evaluate_bound(
+    result: dict[str, Any], expected_head: str, expected_base: str
+) -> list[str]:
+    """Return rejection reasons for any raw review-code-change result.
+
+    Unlike `evaluate_aggregate` (for a caller that only ever wants to consume
+    clean, publishable evidence), this accepts any verdict —
+    `changes_required` and `blocked` are the ordinary outcome of most review
+    cycles for a caller that must react to them, not failures of this
+    function. Schema/semantic validation (via the bundled `validate.py`,
+    including its aggregate-clean lens-execution completeness rule) plus
+    binding to `expected_head`/`expected_base` is still fully enforced.
+    """
+    errors = [f"schema: {error}" for error in VALIDATE.validate_result(result)]
+    if errors:
+        # A schema-level rejection already explains why the result is
+        # untrustworthy; do not layer confusing candidate-binding errors on
+        # top of a document that isn't even shape-valid.
+        return errors
+    return _binding_errors(result, expected_head, expected_base)
+
+
 def evaluate_aggregate(
     result: dict[str, Any], expected_head: str, expected_base: str
 ) -> list[str]:
@@ -68,39 +147,14 @@ def evaluate_aggregate(
         # errors on top of a document that isn't even shape-valid.
         return errors
 
-    if result.get("lens") != "aggregate":
-        errors.append(f"lens: expected an aggregate result, got {result.get('lens')!r}")
     if result.get("verdict") != "clean":
         errors.append(
             f"verdict: expected clean, got {result.get('verdict')!r}; "
             "changes_required and blocked results cannot be consumed as "
             "publishable evidence"
         )
 
-    candidate = result.get("candidate") or {}
-    if (
-        candidate.get("head_sha") != expected_head
-        or candidate.get("comparison_base_sha") != expected_base
-    ):
-        errors.append(
-            "candidate: result is not bound to the current candidate "
-            f"(expected head {expected_head} / base {expected_base}, got "
-            f"head {candidate.get('head_sha')!r} / "
-            f"base {candidate.get('comparison_base_sha')!r})"
-        )
-
-    for execution in result.get("lens_executions") or []:
-        if not isinstance(execution, dict):
-            continue
-        if (
-            execution.get("head_sha") != expected_head
-            or execution.get("comparison_base_sha") != expected_base
-        ):
-            errors.append(
-                f"lens_executions: {execution.get('lens')!r} execution is not "
-                "bound to the current candidate"
-            )
-
+    errors.extend(_binding_errors(result, expected_head, expected_base))
     return errors
 
 

review-suite/scripts/tests/test_bundled_contracts.py

@@ -25,6 +25,7 @@
     "review-solution-simplicity",
     "implement-ticket",
     "babysit-pr",
+    "review-fix-loop",
 )
 CANONICAL_FILES = {
     "CONTRACT.md": REVIEW_SUITE / "CONTRACT.md",
@@ -42,7 +43,7 @@
 # `review-code-change` result bundle it (under `scripts/`, not
 # `references/review-suite/`) — unlike CANONICAL_FILES above, which every
 # review lens skill also bundles.
-GATE_BUNDLING_SKILLS = ("implement-ticket", "babysit-pr")
+GATE_BUNDLING_SKILLS = ("implement-ticket", "babysit-pr", "review-fix-loop")
 GATE_CANONICAL_FILES = {
     "scripts/review_gate.py": REVIEW_SUITE / "scripts" / "review_gate.py",
     "scripts/tests/test_review_gate.py": REVIEW_SUITE

review-suite/scripts/tests/test_review_gate.py

@@ -158,6 +158,124 @@ def test_non_aggregate_lens_result_is_rejected(self):
         self.assertTrue(errors)
         self.assertTrue(any("aggregate" in e for e in errors))
 
+
+class EvaluateBoundTests(unittest.TestCase):
+    """`evaluate_bound` accepts any verdict, unlike `evaluate_aggregate`."""
+
+    def test_current_clean_aggregate_is_accepted(self):
+        self.assertEqual([], GATE.evaluate_bound(CLEAN_AGGREGATE, HEAD, BASE))
+
+    def test_changes_required_with_partial_lens_executions_is_accepted(self):
+        changes_required = {
+            "schema_version": "1.4",
+            "lens": "aggregate",
+            "candidate": {"head_sha": HEAD, "comparison_base_sha": BASE},
+            "verdict": "changes_required",
+            "findings": [
+                {
+                    "id": "correctness.missing-null-check",
+                    "lens": "correctness",
+                    "severity": "blocking",
+                    "confidence": "high",
+                    "rule": "demonstrated correctness failure",
+                    "evidence": [{"location": "a.py:10", "detail": "unchecked None"}],
+                    "concern": "crash",
+                    "impact": "500 on empty input",
+                    "proposed_change": "add a guard",
+                    "expected_effect": "no crash",
+                }
+            ],
+            "blocking_reasons": [],
+            # The sequence stops at the first gating finding, so only one
+            # lens ran; evaluate_bound must not require full completeness
+            # for a non-clean verdict.
+            "lens_executions": [
+                {
+                    "lens": "solution_simplicity",
+                    "head_sha": HEAD,
+                    "comparison_base_sha": BASE,
+                    "verdict": "clean",
+                    "freshly_executed": True,
+                }
+            ],
+        }
+        self.assertEqual([], GATE.evaluate_bound(changes_required, HEAD, BASE))
+
+    def test_blocked_result_omitting_candidate_identity_is_accepted(self):
+        blocked = {
+            "schema_version": "1.4",
+            "lens": "aggregate",
+            "candidate": {},
+            "verdict": "blocked",
+            "findings": [],
+            "blocking_reasons": ["review-solution-simplicity is unreadable"],
+        }
+        # Unlike evaluate_aggregate (which always demands clean and would
+        # reject this), evaluate_bound accepts a blocked result that omits
+        # candidate identity entirely, per review-suite/CONTRACT.md's "a
+        # blocked result may omit candidate fields that the caller could not
+        # establish."
+        self.assertEqual([], GATE.evaluate_bound(blocked, HEAD, BASE))
+
+    def test_stale_schema_version_is_rejected(self):
+        stale = copy.deepcopy(CLEAN_AGGREGATE)
+        stale["schema_version"] = "1.2"
+        errors = GATE.evaluate_bound(stale, HEAD, BASE)
+        self.assertTrue(errors)
+
+    def test_clean_missing_one_lens_is_still_rejected(self):
+        incomplete = copy.deepcopy(CLEAN_AGGREGATE)
+        incomplete["lens_executions"] = [
+            execution
+            for execution in incomplete["lens_executions"]
+            if execution["lens"] != "code_simplicity"
+        ]
+        errors = GATE.evaluate_bound(incomplete, HEAD, BASE)
+        self.assertTrue(errors)
+
+    def test_result_bound_to_a_different_head_is_rejected(self):
+        different_head = copy.deepcopy(CLEAN_AGGREGATE)
+        other_head = "3434343434343434343434343434343434343434"
+        different_head["candidate"]["head_sha"] = other_head
+        for execution in different_head["lens_executions"]:
+            execution["head_sha"] = other_head
+        errors = GATE.evaluate_bound(different_head, HEAD, BASE)
+        self.assertTrue(errors)
+        self.assertTrue(any("current candidate" in e for e in errors))
+
+    def test_non_aggregate_lens_result_is_rejected(self):
+        single_lens = {
+            "schema_version": "1.4",
+            "lens": "correctness",
+            "candidate": {"head_sha": HEAD, "comparison_base_sha": BASE},
+            "verdict": "clean",
+            "findings": [],
+            "blocking_reasons": [],
+        }
+        errors = GATE.evaluate_bound(single_lens, HEAD, BASE)
+        self.assertTrue(errors)
+        self.assertTrue(any("aggregate" in e for e in errors))
+
+    def test_blocked_result_with_mismatched_identity_is_still_rejected(self):
+        # A blocked result that DOES assert a candidate identity is held to
+        # the same binding standard as any other verdict.
+        blocked = {
+            "schema_version": "1.4",
+            "lens": "aggregate",
+            "candidate": {
+                "head_sha": "3434343434343434343434343434343434343434",
+                "comparison_base_sha": BASE,
+            },
+            "verdict": "blocked",
+            "findings": [],
+            "blocking_reasons": ["repository identity could not be established"],
+        }
+        errors = GATE.evaluate_bound(blocked, HEAD, BASE)
+        self.assertTrue(errors)
+        self.assertTrue(any("current candidate" in e for e in errors))
+
+
+class ReviewGateCliTests(unittest.TestCase):
     def test_cli_exits_nonzero_and_prints_errors_for_a_rejected_result(self):
         import json
         import subprocess