feat(review-fix-loop): implement reviewer isolation and complete-review orchestration - #110
Merged
Merged
Conversation
…ew orchestration - Add `scripts/reviewer_orchestration.py`: fixed lens resolution, rejection of an incomplete/misbound review-code-change result (both a lens-completeness check via `evaluate_review_result` and a packet-evidence check via `evaluate_review_pair`), default fresh-subagent review-execution resolution with an explicit in-agent override and no automatic fallback, reviewer identity generation, before/after worktree mutation detection, checkpoint- shaped review-record construction that fails a cycle closed on any detected mutation, and deterministic finding normalization/selection. - Add `references/reviewer-orchestration.md`, implementing design's "Review execution" and "Reviewer write prevention" sections and workflow step 3. - Bundle the canonical review-suite contract into `references/review-suite/` (added review-fix-loop to `just sync-contracts` and to `review-suite/scripts/tests/test_bundled_contracts.py`'s `BUNDLING_SKILLS`), matching every other review-code-change consumer. - Update `SKILL.md` (frontmatter tool grants, description, a new "Run a complete review" section, and the non-goals list) and add `agents/ claude-code.md`/`agents/openai.yaml` adapters, mirroring review-code-change and implement-ticket's conventions. - Add `scripts/tests/test_reviewer_orchestration.py` (52 new tests). - Issue #98 (child of epic #95): reviewer isolation and complete-review orchestration is the next rollout step after the merged contract leaf (#96), and is required before either fix-cycle child (#99 local_commit, #100 update_pr) can run a trustworthy review. Refs #98
## Summary - `detect_worktree_mutation` now compares local `refs` (excluding `refs/remotes/*`) between before/after snapshots, not only `head_sha` and the tracked/staged/unstaged/untracked/ignored path lists — a reviewer that runs `git stash` or force-moves a branch without touching HEAD or any tracked path is now caught. - `evaluate_review_pair` no longer rejects a contract-legal `blocked` result that omits candidate identity already present in the packet (it stopped reusing the bundled `validate_pair`'s stricter packet/result identity check for that reason); it now additionally binds the *packet's own* candidate identity to the current cycle's expected head/base directly. - Extracted canonical `review_gate.evaluate_bound` (schema validation plus candidate/lens-execution binding, generalized to accept any verdict) in `review-suite/scripts/review_gate.py`, reused by `evaluate_aggregate` unchanged; `reviewer_orchestration.py` now delegates to it instead of a second, independently-maintained binding implementation. Added `review-fix-loop` to the `review_gate.py`/`test_review_gate.py` bundling loop (`just sync-contracts`, `GATE_BUNDLING_SKILLS`), matching `implement-ticket`/`babysit-pr`. - Updated `SKILL.md` and `references/reviewer-orchestration.md` to match: refs capture, preferring `evaluate_review_pair` (packet-bearing) over `evaluate_review_result`, and the shared `review_gate` dependency. ## Why - First review-code-change pass on #98 found: refs-only mutation went undetected (acceptance criterion 6 unproven for that class), a contract-legal blocked result was rejected on the packet-bearing path (SKILL.md's own recommended path), and the binding logic duplicated the canonical, drift-tested `review_gate` implementation used by two other consumers. Refs #98
… incomplete snapshots, collapse dual evaluators - `detect_worktree_mutation` no longer compares `ignored` worktree state (still required to be captured, per design, via `REQUIRED_SNAPSHOT_KEYS`, but never compared): authorized recorded validation commands legitimately create or change ignored build artifacts (`__pycache__/`, `.ruff_cache/`, `.venv/`), so comparing it made every review pass that ran validation falsely report a mutation and made `converged` permanently unreachable. - `detect_worktree_mutation` now raises `ValueError` when either snapshot is missing a required capture key (`head_sha`, `refs`, or any worktree category), instead of silently treating an uncaptured dimension as unchanged. - Collapsed the packet-less `evaluate_review_result(result, ...)` path into the single mandatory `evaluate_review_result(packet, result, ...)`: review-fix-loop's own checkpoint/terminal-result contract never persists a raw packet or raw result without the other, so the weaker path had no legitimate caller and could otherwise let a `clean` verdict with actually-failed packet validation slip through by omission. `build_review_record`'s `packet` parameter is now required accordingly. - Backfilled the two prior commits' SHAs into the changelog and removed a fabricated changelog entry from the prior fix-cycle commit. - Second review-code-change pass on #98 found: the `ignored`-comparison defect made `converged` unreachable in this repository's own worktree (every validation command run creates `__pycache__/`); a mutation-detection function silently passing on missing capture keys defeats the point of requiring the capture; and the packet-less evaluator path was unreachable by any real caller and duplicated the packet-plus-result path's logic. Refs #98
…ng prose ## Summary - Removed a byte-identical duplicate test (`test_changes_required_with_partial_lens_executions_is_still_accepted` duplicated `test_current_changes_required_with_partial_lens_executions_is_accepted`). - Trimmed `evaluate_review_result`'s docstring and the `COMPARED_WORKTREE_CATEGORIES` module comment to state current behavior once instead of narrating a removed earlier revision and repeating the `ignored`-is-captured-but-uncompared rationale in full three times; the Python module comment and `detect_worktree_mutation`'s docstring now point to `references/reviewer-orchestration.md`'s "Reviewer write prevention" tier 4 as the one full explanation. ## Why - Third review-code-change pass on #98 (code-simplicity lens): a duplicate test and history-narrating/triplicated rationale prose (0.50 docstring/ comment density vs. 0.11-0.12 for sibling `validate.py` modules) with no behavioral change needed. Refs #98
…ngelog SHA ## Summary - Rebased onto origin/main (now including the merged #97 local-execution substrate at 26b4cf4), resolving conflicts in `CHANGELOG.md` and `SKILL.md` by combining both children's content. - Documented that a mutation attributable to a review pass must stop the invocation immediately with `blocked/reviewer_integrity_failure`, rather than relying solely on the `write_isolation: "violated"` / `_check_converged_requires_clean_evidence` backstop to catch it later. Design assigns this judgment to "the phase that observed it" — this is that phase's explicit routing instruction, in both `references/reviewer-orchestration.md` and `SKILL.md`. - Corrected a changelog SHA left stale by the rebase (a rebased commit's own hash changes, so a prior cycle's backfilled SHA referencing the pre-rebase hash was wrong). - Confirmed (no code change) that the review_gate.evaluate_bound extraction from cycle 1 is the deliberate design going forward: correctness verified it changes no accept/reject outcome for implement-ticket/babysit-pr. ## Why - Fourth review-code-change pass on #98: sibling ticket #97 merged to main mid-review, requiring a rebase; the same pass also found the mutation-stop routing was implicit rather than explicit, and that the rebase itself introduced a stale changelog SHA. Refs #98
## Summary - The previous fix cycle's rebase cleanup left two adjacent changelog entries carrying each other's SHA: the "remove a byte-identical duplicate test" entry (commit 7663b2c) was stamped with the next, older commit's hash, and the "stop comparing ignored worktree state" entry (commit 18991dd) carried no SHA at all. Corrected both. ## Why - Fifth review-code-change pass on #98 found the off-by-one; a two-token attribution correction with no code, test, or schema impact. Refs #98
… tests ## Summary - Removed three subsumed/redundant tests: `test_override_is_honored_even_when_fresh_subagent_would_be_available` (strict subset of `test_explicit_override_is_honored_and_recorded`), `test_different_sequence_yields_a_different_identity` (entailed by `test_default_identity_matches_example_convention`, which already pins both exact values), and `test_identical_refs_report_no_mutation` (identical to `test_identical_snapshots_report_no_mutation` once `refs` is part of the shared `CLEAN_STATE` fixture). - Cut `reviewer_orchestration.py`'s docstring/comment prose from roughly parity with its own code length down to sibling-module levels: every function docstring now states its rationale once, briefly, and points to `references/reviewer-orchestration.md` for the full explanation instead of restating it (previously repeated across the module docstring, inline comments, and multiple function docstrings, including one circular in-file pointer pair). ## Why - Sixth review-code-change pass on #98 (code_simplicity, run for the first time since the rebase onto #97): both findings were test/documentation hygiene only, no behavioral change. Refs #98
6 tasks
11 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements complete-review orchestration for
review-fix-loop, with freshread-only reviewer subagents by default and an explicit in-agent override —
the "reviewer isolation and complete-review orchestration" child of epic
#95, building on the merged contract leaf (#96) and the merged local
execution substrate (#97).
scripts/reviewer_orchestration.py(dependency-free, bundles the canonicalreview-suite contract and
review_gate.py, kept in sync viajust sync-contracts):resolve_review_lenses()— the fixed, non-selectable lens set acomplete review must cover.
resolve_review_execution_mode()—fresh_subagentby default, anexplicit
in_agent_overrideonly when authorized,missing_capabilitywith no automatic fallback when a host can't spawn a fresh context.
evaluate_review_result()— rejects an incomplete or misbound rawreview-code-change packet/result pair (lens completeness for
clean,packet-evidence completeness for any verdict).
detect_worktree_mutation()— before/after snapshot comparison(
head_sha, localrefsexcludingrefs/remotes/*,tracked/staged/unstaged/untracked paths;
ignoredis captured but nevercompared, since authorized validation commands legitimately touch it),
failing closed on an incomplete snapshot.
build_review_record()— builds one checkpoint-shapedreview_recordsentry, forcing
write_isolation: "violated"on any detected mutation soit fails that cycle closed.
normalize_findings()/select_next_finding()— deterministic findingordering for later fix-cycle selection and checkpointing.
build_reviewer_briefing()/REVIEWER_PROHIBITIONS— the literalinstruction text prohibiting worktree mutation and implementation that
travels with every review pass.
references/reviewer-orchestration.md— the full behavioral contract thismodule implements, plus a documented
blocked/reviewer_integrity_failurestop instruction for an attributable reviewer mutation.
skills/review-fix-loop/agents/claude-code.mdandagents/openai.yaml—new discovery adapters.
review_gate.evaluate_boundin canonicalreview-suite/scripts/review_gate.py(schema validation pluscandidate/lens-execution binding, generalized to accept any verdict),
reused unchanged by the existing
evaluate_aggregate; addedreview-fix-loopto thereview-suite/review_gate.pybundling loops soit shares the same drift-tested contract copies as
implement-ticketandbabysit-pr.SKILL.mdadditively (frontmatter tool grants, a new "Run acomplete review" section, narrowed non-goals) and
CHANGELOG.md.skills/review-fix-loop/scripts/tests/test_reviewer_orchestration.pyandthe bundled
test_review_gate.py.Non-goals (explicitly out of scope, per design)
already merged, and Deliver and evaluate standalone review-fix-loop local_commit #99/Deliver and evaluate review-fix-loop update_pr #100).
"Decide"/"Fix" steps, a later child).
Test plan
python3 -m unittest discover -s skills/review-fix-loop/scripts/tests -p 'test_*.py'— 232 tests passjust test— all skill suites andreview-suite/scripts/tests(318 tests) passjust lint— ruff, mdformat,skills-ref validate(all 9 skills), plugin packaging validation passjust format— no diffsreview-code-changereview: clean aggregate at headfd690248670c6acecfb2d335e70e347d4d4390de—solution_simplicity,correctness, andcode_simplicityall fresh and clean for this exact head/base (7 review cycles; earlier cycles found and fixed refs-mutation detection gaps, a contract-legal blocked-result rejection, duplicated binding logic, anignored-worktree false-positive, fail-open snapshot validation, a dead packet-less evaluator path, a rebase-introduced changelog SHA issue, and doc/test verbosity — all resolved)Refs #98