diff --git a/CHANGELOG.md b/CHANGELOG.md index 87c70fe..3831cda 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,8 +4,46 @@ summary: Chronological history of repository and skill changes. # Changelog -## 2026-07-31 — Recorded the first review-fix-loop `update_pr` fix cycle +## 2026-07-31 — Added the review-fix-loop cross-cutting evaluation corpus, recorded the first review-fix-loop `update_pr` fix cycle +- fix(review-fix-loop): configure `user.email`/`user.name` on both git clones + `scripts/evals/corpus.py`'s + `up_sequential_publication_race_second_clone_loses` scenario creates, + mirroring `helpers.init_repo`'s existing convention for every non-cloned + fixture repo — `git clone` never copies a source repository's local git + identity config, and unlike a developer machine a CI runner has no global + identity configured either, so the scenario's own `git commit` call failed + with "Author identity unknown" in GitHub Actions even though every local run + passed; reproduced the exact CI failure locally under a forced no-identity + condition, confirmed the fix resolves it, and confirmed GitHub Actions' own + `ci` check on PR #113 is green +- fix(review-fix-loop): consolidate `scripts/evals/helpers.py`'s five fixtures + that were byte-identical or functionally identical to + `scripts/tests/helpers.py`'s own (`init_repo`, `CLEAN_TEMPLATE`, + `ALWAYS_PASS_VALIDATION`, `finding`, `make_clean_reviewer`, + `fixing_apply_fix`, `accepting_decide`) into imports from that sibling module + instead of a second source of truth, following this repository's own + `carve-changesets` precedent of importing across the `scripts/tests`/ + `scripts/evals` boundary within one skill, and remove one unused reviewer + fixture (`make_expanding_findings_reviewer`) left over from a descoped + scenario, closing the one code-simplicity gap the first review-code-change + pass on #101 found (`81a3078c819d4bc8755a9a796d2fa4c0e7dbf1c4`) +- feat(review-fix-loop): add the cross-cutting, result-blind evaluation corpus + (issue #101, epic #95) covering convergence, repeated findings, + invalid/incomplete reviews, declined findings, budget exhaustion, interruption + and recovery, validation failure, reviewer mutation, and publication races + across both `local_commit` and `update_pr`, plus the fresh-subagent default + and the explicit in-agent override — twenty scenarios in + `scripts/evals/corpus.py`, each driving the real engine against a real + disposable Git repository (and, for `update_pr`, a real disposable bare + remote) and graded in `scripts/evals/grader.py` against independently derived + Git evidence (a real commit count, a real file's content at a real commit, a + real remote ref, a real object's reachability) rather than the returned + terminal-result document's own claims; `scripts/tests/test_evals.py` + demonstrates the grader rejecting both a fabricated convergence claim and a + fixture that cannot actually converge, and runs the whole corpus under + `just test`; `just eval-review-fix-loop` is the standalone entry point + (`cd5b3ee63d89fed305c4e5a3c0f15cb14b84a3c6`) - fix(review-fix-loop): extract the test fixtures shared between `test_local_commit.py` and `test_update_pr.py` (the module loader, a bare local repository, the always-passing validation commands, the @@ -13,6 +51,7 @@ summary: Chronological history of repository and skill changes. sibling `scripts/tests/helpers.py`, matching `carve-changesets/scripts/tests/helpers.py`'s established precedent, closing the one code-simplicity gap the first review-code-change pass on #100 found + (`729135bb11d5bd8f0efa3a66d1c1ab1f978a3f6d`) ## 2026-07-30 — Delivered and evaluated the standalone review-fix-loop `update_pr` workflow, delivered and evaluated the standalone review-fix-loop `local_commit` workflow, 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 diff --git a/justfile b/justfile index 48b2f9d..9a3e239 100644 --- a/justfile +++ b/justfile @@ -110,6 +110,18 @@ test-implement-epic: test-carve-changesets: python3 -m unittest discover -s {{skills_dir}}/carve-changesets/scripts/tests -p 'test_*.py' +test-review-fix-loop: + python3 -m unittest discover -s {{skills_dir}}/review-fix-loop/scripts/tests -p 'test_*.py' + +# Result-blind, deterministic replay of the review-fix-loop cross-cutting +# corpus: drives the real local_commit/update_pr engine against disposable +# Git repositories with scripted reviewer/decide/apply_fix fixtures (no +# subprocess boundary, no model call, no network). Also exercised under +# `just test` via scripts/tests/test_evals.py; this target is the standalone +# entry point for ad hoc runs and per-scenario `--output-dir` reports. +eval-review-fix-loop: + python3 {{skills_dir}}/review-fix-loop/scripts/evals/runner.py + eval-carve-changesets: python3 {{skills_dir}}/carve-changesets/scripts/evals/runner.py --integration-self-test python3 {{skills_dir}}/carve-changesets/scripts/evals/runner.py diff --git a/skills/review-fix-loop/SKILL.md b/skills/review-fix-loop/SKILL.md index 22082ac..3bc4fb6 100644 --- a/skills/review-fix-loop/SKILL.md +++ b/skills/review-fix-loop/SKILL.md @@ -12,7 +12,7 @@ review suite, applies material ticket-scoped fixes, and repeats until review converges or a bounded stop condition is reached. The full design is [`design/review-fix-loop.md`](../../design/review-fix-loop.md). -Five of its children are implemented so far: +Six of its children are implemented so far: - [Issue #96](https://github.com/shaug/agent-scripts/issues/96) (the first of epic [#95](https://github.com/shaug/agent-scripts/issues/95)) defines and @@ -52,6 +52,11 @@ Five of its children are implemented so far: publication race is lost or the remote is unavailable. See [Run the standalone `update_pr` workflow](#run-the-standalone-update_pr-workflow) below. +- [Issue #101](https://github.com/shaug/agent-scripts/issues/101) adds the + cross-cutting, result-blind evaluation corpus that establishes this skill's + behavioral contract across both execution modes from externally observable Git + evidence, not the returned terminal-result document's own claims. See + [Evaluation](#evaluation) below. Use [`scripts/local_commit.py`](scripts/local_commit.py)'s `run_local_commit(...)` to run a complete standalone `local_commit` invocation @@ -275,6 +280,24 @@ unreachable remote, the remote-target lock actually being exercised through `run_update_pr`, and rejection of an invalid invocation or a `local_commit` invocation at the API boundary. +## Evaluation + +[`evals/README.md`](evals/README.md) documents the cross-cutting, result-blind +evaluation corpus in [`scripts/evals/`](scripts/evals): twenty scenarios, +covering convergence, repeated findings, invalid/incomplete reviews, declined +findings, budget exhaustion, interruption and recovery, validation failure, +reviewer mutation, and publication races across both `local_commit` and +`update_pr`, plus the fresh-subagent default and the explicit in-agent override. +Every scenario drives the real engine against a real disposable Git repository +(and, for `update_pr`, a real disposable bare remote) and is graded against +independently derived Git evidence — a real commit count, a real file's content +at a real commit, a real remote ref, a real object's reachability — never the +returned terminal-result document's own claims, so a result that only *asserts* +success cannot pass. No subprocess boundary and no model call is involved, so +the whole corpus runs deterministically under `just test` (via +[`scripts/tests/test_evals.py`](scripts/tests/test_evals.py)) and standalone via +`just eval-review-fix-loop`. + ## Non-goals - Creating, merging, closing, or otherwise managing a pull request. diff --git a/skills/review-fix-loop/evals/README.md b/skills/review-fix-loop/evals/README.md new file mode 100644 index 0000000..639b4b4 --- /dev/null +++ b/skills/review-fix-loop/evals/README.md @@ -0,0 +1,71 @@ +# Review-fix-loop evaluations + +Issue #101's cross-cutting, result-blind evaluation corpus for the standalone +`review-fix-loop` skill. It does not replace +`scripts/tests/test_local_commit.py` or `scripts/tests/test_update_pr.py` — +those remain the capability-owned unit suites for issues #99/#100. This corpus +instead proves the skill's whole behavioral contract across both publication +policies, from externally observable Git evidence rather than the returned +terminal-result document's own claims. + +## Where the corpus lives + +Every other evaluated skill in this repository (`carve-changesets`, +`implement-ticket`, `babysit-pr`) keeps its scenario data in this directory as +`cases.json`/`expectations.json`, because each of their scenarios is a short +natural-language situation description handed to an agent-shaped executor for +judgment. `review-fix-loop` has no equivalent judgment surface to evaluate that +way: its actual behavior is the deterministic Python engine in +`../scripts/local_commit.py` and `../scripts/update_pr.py`, and the three +genuinely host-boundary actions (`reviewer`, `decide`, `apply_fix`) are already +exercised with scripted fixtures by the capability-owned unit suites. A JSON +case format here would either duplicate that fixture plumbing or describe +scenarios too shallowly to build the real Git repositories, disposable bare +remotes, and interleaved-clone races several scenarios require. + +The corpus is therefore a registry of plain Python scenario functions in +`../scripts/evals/corpus.py` (`CORPUS.SCENARIOS`/`CORPUS.SCENARIOS_BY_ID`), each +of which drives the real engine against a real temporary repository and reports +a `checks` mapping of independently-derived Git evidence for +`../scripts/evals/grader.py` to diff. This directory still exists, per the +repository's convention that every evaluated skill keeps its evaluation +documentation and data under `evals/`, and is the place to look for what this +corpus asserts and why. + +## Running it + +```bash +just eval-review-fix-loop # run the whole corpus once +python3 scripts/evals/runner.py --list # list scenario ids +python3 scripts/evals/runner.py --scenario lc_converged_after_one_fix +python3 scripts/evals/runner.py --output-dir /tmp/out # per-scenario JSON reports +``` + +No subprocess boundary and no model call is involved: every scenario is an +in-process, deterministic replay, so the whole corpus is free and safe to run in +repository CI. `scripts/tests/test_evals.py` already runs it under `just test`; +`eval-review-fix-loop` is the standalone entry point for ad hoc runs and +per-scenario reports. + +## Scope + +The corpus mirrors this ticket's own "Scope" bullet list: convergence, repeated +findings, invalid reviews, declined findings, budget exhaustion, interruption, +recovery, validation failure, reviewer mutation, and publication races — across +both `local_commit` and `update_pr` — plus fresh-subagent defaults and the +explicit in-agent override. It deliberately does not add dedicated +`oscillation`/`repeated_failed_attempt` scenarios: those `changes_remaining` +reasons are already covered by `scripts/tests/test_local_commit.py`. + +## Result-blindness + +`grader.grade_case` never trusts a scenario's returned terminal-result document +by itself. Each scenario's `checks` mapping compares the terminal contract's own +declared fields (`terminal_state`, `reason`) *and* a set of facts computed +independently by asking Git directly — a real commit count, a real file's +content at a real commit, a real remote ref, a real object's reachability, a +real dirty-worktree listing — so a result that lies about what happened +disagrees with the evidence rather than being trusted. +`scripts/tests/test_evals.py`'s `SeededFaultDemonstrationTests` demonstrates +this directly: a fabricated "converged" claim with no matching Git evidence, and +a deliberately unfixable fixture declared to converge, are both rejected. diff --git a/skills/review-fix-loop/scripts/evals/__init__.py b/skills/review-fix-loop/scripts/evals/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/skills/review-fix-loop/scripts/evals/corpus.py b/skills/review-fix-loop/scripts/evals/corpus.py new file mode 100644 index 0000000..545dab6 --- /dev/null +++ b/skills/review-fix-loop/scripts/evals/corpus.py @@ -0,0 +1,1160 @@ +"""The reusable, result-blind review-fix-loop evaluation corpus (issue #101). + +Each scenario function below drives the real `local_commit.run_local_commit` +or `update_pr.run_update_pr` entry point end to end against a real temporary +Git repository (and, for `update_pr`, a real disposable local bare +repository), then reports a `checks` mapping of +`name -> (expected, observed)` pairs for `grader.grade_case` to diff. +`observed` values are computed independently of the returned terminal-result +document wherever the design's "Validation strategy" calls for externally +observable evidence — reading `marker.txt`'s content at a specific commit via +`git show`, reading a ref directly via `git rev-parse`/`git ls-remote`, +checking object reachability via `git cat-file -e` — never by re-deriving the +same field the code under test already reported. See `grader.py`'s module +docstring for the full rationale. + +Corpus scope mirrors this ticket's own "Scope" bullet list exactly: +convergence, repeated findings, invalid reviews, declined findings, budget +exhaustion, interruption, recovery, validation failure, reviewer mutation, +publication races, plus fresh-subagent defaults and the explicit in-agent +override. It deliberately does not re-cover `oscillation` or +`repeated_failed_attempt`: those `changes_remaining` reasons are already +exercised by `scripts/tests/test_local_commit.py`, the capability-owned unit +suite this ticket's own body says not to duplicate. + +Every scenario is a plain function `(tmp_dir: Path) -> dict` so `runner.py` +can execute each one inside its own disposable temporary directory and so +`scripts/tests/test_evals.py` can call any single scenario directly for the +seeded-fault demonstration required by this ticket's Validation section. +""" + +from __future__ import annotations + +import sys +from pathlib import Path +from typing import Any, Callable + +HERE = Path(__file__).resolve().parent +if str(HERE) not in sys.path: + sys.path.insert(0, str(HERE)) + +import grader as G # noqa: E402 +import helpers as H # noqa: E402 + +LC = H.LC +UP = H.UP +LE = H.LE +VALIDATE = H.VALIDATE + + +def _schema_check(result: dict[str, Any]) -> tuple[list[str], list[str]]: + return ([], VALIDATE.validate_terminal_result(result)) + + +def _terminal_checks( + result: dict[str, Any], *, terminal_state: str, reason: str | None +) -> dict[str, tuple[Any, Any]]: + return { + "terminal_state": (terminal_state, result.get("terminal_state")), + "reason": (reason, result.get("reason")), + "schema_valid": _schema_check(result), + } + + +def _case( + id: str, + *, + category: str, + policy: str, + result: dict[str, Any], + checks: dict[str, tuple[Any, Any]], +) -> dict[str, Any]: + return { + "id": id, + "category": category, + "policy": policy, + "result": result, + "checks": checks, + } + + +# --------------------------------------------------------------------------- +# local_commit: convergence +# --------------------------------------------------------------------------- + + +def lc_converged_clean_initial(tmp_dir: Path) -> dict[str, Any]: + repo = tmp_dir / "repo" + H.init_repo(repo) + base_sha, head_sha = H.start_candidate(repo, branch="lc/clean", marker="fixed") + invocation = H.make_invocation( + repo, + policy="local_commit", + branch="lc/clean", + base_sha=base_sha, + head_sha=head_sha, + invocation_id="lc-converged-clean-initial", + ) + result = LC.run_local_commit( + invocation, + repo=repo, + reviewer=H.make_clean_reviewer(), + decide=H.accepting_decide, + apply_fix=H.fixing_apply_fix, + ) + branch_head = G.rev_parse(repo, "lc/clean") + checks = _terminal_checks(result, terminal_state="converged", reason=None) + checks.update( + { + "no_new_commit_on_canonical_branch": ( + 0, + G.rev_list_count(repo, head_sha, branch_head), + ), + "marker_at_final_head": ( + H.MARKER_FIXED, + G.show_file(repo, branch_head, "marker.txt"), + ), + "worktree_clean": (True, G.worktree_is_clean(repo)), + } + ) + return _case( + "lc-converged-clean-initial", + category="convergence", + policy="local_commit", + result=result, + checks=checks, + ) + + +def lc_converged_after_one_fix(tmp_dir: Path) -> dict[str, Any]: + repo = tmp_dir / "repo" + H.init_repo(repo) + base_sha, head_sha = H.start_candidate(repo, branch="lc/one-fix", marker="broken") + invocation = H.make_invocation( + repo, + policy="local_commit", + branch="lc/one-fix", + base_sha=base_sha, + head_sha=head_sha, + invocation_id="lc-converged-after-one-fix", + ) + result = LC.run_local_commit( + invocation, + repo=repo, + reviewer=H.make_marker_reviewer(repo), + decide=H.accepting_decide, + apply_fix=H.fixing_apply_fix, + ) + branch_head = G.rev_parse(repo, "lc/one-fix") + checks = _terminal_checks(result, terminal_state="converged", reason=None) + checks.update( + { + "exactly_one_new_commit": ( + 1, + G.rev_list_count(repo, head_sha, branch_head), + ), + "canonical_branch_matches_reported_final_head": ( + result.get("head", {}).get("final"), + branch_head, + ), + "marker_at_final_head": ( + H.MARKER_FIXED, + G.show_file(repo, branch_head, "marker.txt"), + ), + "worktree_clean": (True, G.worktree_is_clean(repo)), + "default_review_independence": ( + "fresh_subagent", + (result.get("review_records") or [{}])[-1].get("review_independence"), + ), + } + ) + return _case( + "lc-converged-after-one-fix", + category="convergence", + policy="local_commit", + result=result, + checks=checks, + ) + + +# --------------------------------------------------------------------------- +# local_commit: repeated findings + budget exhaustion +# --------------------------------------------------------------------------- + + +def lc_budget_exhausted_persistent_finding(tmp_dir: Path) -> dict[str, Any]: + repo = tmp_dir / "repo" + H.init_repo(repo) + base_sha, head_sha = H.start_candidate(repo, branch="lc/budget", marker="broken") + invocation = H.make_invocation( + repo, + policy="local_commit", + branch="lc/budget", + base_sha=base_sha, + head_sha=head_sha, + invocation_id="lc-budget-exhausted", + max_fix_cycles=1, + ) + result = LC.run_local_commit( + invocation, + repo=repo, + reviewer=H.make_marker_reviewer(repo), + decide=H.accepting_decide, + apply_fix=H.make_never_fixing_apply_fix(), + ) + branch_head = G.rev_parse(repo, "lc/budget") + checks = _terminal_checks( + result, terminal_state="changes_remaining", reason="cycle_budget_exhausted" + ) + checks.update( + { + "exactly_one_attempt_committed": ( + 1, + G.rev_list_count(repo, head_sha, branch_head), + ), + "finding_genuinely_unresolved_at_final_head": ( + False, + G.show_file(repo, branch_head, "marker.txt") == H.MARKER_FIXED, + ), + "worktree_clean": (True, G.worktree_is_clean(repo)), + } + ) + return _case( + "lc-budget-exhausted-persistent-finding", + category="budget_exhaustion", + policy="local_commit", + result=result, + checks=checks, + ) + + +# --------------------------------------------------------------------------- +# local_commit: declined findings +# --------------------------------------------------------------------------- + + +def lc_declined_finding_blocked(tmp_dir: Path) -> dict[str, Any]: + repo = tmp_dir / "repo" + H.init_repo(repo) + base_sha, head_sha = H.start_candidate(repo, branch="lc/declined", marker="broken") + invocation = H.make_invocation( + repo, + policy="local_commit", + branch="lc/declined", + base_sha=base_sha, + head_sha=head_sha, + invocation_id="lc-declined-finding", + ) + result = LC.run_local_commit( + invocation, + repo=repo, + reviewer=H.make_marker_reviewer(repo), + decide=H.make_rejecting_decide("not a genuine defect for this scenario"), + apply_fix=H.fixing_apply_fix, + ) + branch_head = G.rev_parse(repo, "lc/declined") + checks = _terminal_checks( + result, terminal_state="blocked", reason="operator_input_required" + ) + checks.update( + { + "no_commit_was_made": (head_sha, branch_head), + "worktree_clean": (True, G.worktree_is_clean(repo)), + } + ) + return _case( + "lc-declined-finding-blocked", + category="declined_findings", + policy="local_commit", + result=result, + checks=checks, + ) + + +def lc_scope_expanding_finding_blocked(tmp_dir: Path) -> dict[str, Any]: + repo = tmp_dir / "repo" + H.init_repo(repo) + base_sha, head_sha = H.start_candidate(repo, branch="lc/scope", marker="broken") + invocation = H.make_invocation( + repo, + policy="local_commit", + branch="lc/scope", + base_sha=base_sha, + head_sha=head_sha, + invocation_id="lc-scope-expanding", + ) + result = LC.run_local_commit( + invocation, + repo=repo, + reviewer=H.make_marker_reviewer(repo), + decide=H.make_scope_expanding_decide(), + apply_fix=H.fixing_apply_fix, + ) + branch_head = G.rev_parse(repo, "lc/scope") + checks = _terminal_checks( + result, terminal_state="blocked", reason="scope_decision_required" + ) + checks["no_commit_was_made"] = (head_sha, branch_head) + return _case( + "lc-scope-expanding-finding-blocked", + category="declined_findings", + policy="local_commit", + result=result, + checks=checks, + ) + + +# --------------------------------------------------------------------------- +# local_commit: validation failure +# --------------------------------------------------------------------------- + + +def lc_validation_unavailable_blocked(tmp_dir: Path) -> dict[str, Any]: + repo = tmp_dir / "repo" + H.init_repo(repo) + base_sha, head_sha = H.start_candidate( + repo, branch="lc/val-unavailable", marker="fixed" + ) + invocation = H.make_invocation( + repo, + policy="local_commit", + branch="lc/val-unavailable", + base_sha=base_sha, + head_sha=head_sha, + invocation_id="lc-validation-unavailable", + ) + result = LC.run_local_commit( + invocation, + repo=repo, + reviewer=H.make_clean_reviewer(), + decide=H.accepting_decide, + apply_fix=H.fixing_apply_fix, + run_validation=H.make_unavailable_validation_runner(), + ) + branch_head = G.rev_parse(repo, "lc/val-unavailable") + checks = _terminal_checks( + result, terminal_state="blocked", reason="validation_unavailable" + ) + checks["no_commit_was_made"] = (head_sha, branch_head) + return _case( + "lc-validation-unavailable-blocked", + category="validation_failure", + policy="local_commit", + result=result, + checks=checks, + ) + + +def lc_validation_failure_not_tractable(tmp_dir: Path) -> dict[str, Any]: + repo = tmp_dir / "repo" + H.init_repo(repo) + base_sha, head_sha = H.start_candidate( + repo, branch="lc/val-intractable", marker="fixed", validation_flag="fail" + ) + invocation = H.make_invocation( + repo, + policy="local_commit", + branch="lc/val-intractable", + base_sha=base_sha, + head_sha=head_sha, + invocation_id="lc-validation-intractable", + validation=H.FLAG_GATED_VALIDATION, + ) + result = LC.run_local_commit( + invocation, + repo=repo, + reviewer=H.make_clean_reviewer(), + decide=H.accepting_decide, + apply_fix=H.fixing_apply_fix, + classify_validation_failure=H.classify_validation_failure_as_intractable, + ) + branch_head = G.rev_parse(repo, "lc/val-intractable") + checks = _terminal_checks( + result, + terminal_state="changes_remaining", + reason="current_candidate_validation_failure", + ) + checks["no_commit_was_made"] = (head_sha, branch_head) + return _case( + "lc-validation-failure-not-tractable", + category="validation_failure", + policy="local_commit", + result=result, + checks=checks, + ) + + +def lc_validation_failure_tractable_converges(tmp_dir: Path) -> dict[str, Any]: + repo = tmp_dir / "repo" + H.init_repo(repo) + base_sha, head_sha = H.start_candidate( + repo, branch="lc/val-tractable", marker="fixed", validation_flag="fail" + ) + invocation = H.make_invocation( + repo, + policy="local_commit", + branch="lc/val-tractable", + base_sha=base_sha, + head_sha=head_sha, + invocation_id="lc-validation-tractable", + validation=H.FLAG_GATED_VALIDATION, + ) + result = LC.run_local_commit( + invocation, + repo=repo, + reviewer=H.make_clean_reviewer(), + decide=H.accepting_decide, + apply_fix=H.make_flag_fixing_apply_fix(), + classify_validation_failure=H.classify_validation_failure_as_tractable, + ) + branch_head = G.rev_parse(repo, "lc/val-tractable") + checks = _terminal_checks(result, terminal_state="converged", reason=None) + checks.update( + { + "exactly_one_new_commit": ( + 1, + G.rev_list_count(repo, head_sha, branch_head), + ), + "validation_flag_actually_repaired": ( + "pass", + G.show_file(repo, branch_head, "validation_flag.txt"), + ), + } + ) + return _case( + "lc-validation-failure-tractable-converges", + category="validation_failure", + policy="local_commit", + result=result, + checks=checks, + ) + + +# --------------------------------------------------------------------------- +# local_commit: invalid/incomplete review + reviewer mutation +# --------------------------------------------------------------------------- + + +def lc_reviewer_mutation_blocked(tmp_dir: Path) -> dict[str, Any]: + repo = tmp_dir / "repo" + H.init_repo(repo) + base_sha, head_sha = H.start_candidate(repo, branch="lc/mutation", marker="broken") + invocation = H.make_invocation( + repo, + policy="local_commit", + branch="lc/mutation", + base_sha=base_sha, + head_sha=head_sha, + invocation_id="lc-reviewer-mutation", + ) + result = LC.run_local_commit( + invocation, + repo=repo, + reviewer=H.make_mutating_reviewer(repo, H.make_marker_reviewer(repo)), + decide=H.accepting_decide, + apply_fix=H.fixing_apply_fix, + ) + branch_head = G.rev_parse(repo, "lc/mutation") + checks = _terminal_checks( + result, terminal_state="blocked", reason="reviewer_integrity_failure" + ) + checks.update( + { + "candidate_never_advanced": (head_sha, branch_head), + "mutation_attempt_preserved_for_inspection": ( + True, + "reviewer-attempted-write.txt" in G.untracked_paths(repo), + ), + } + ) + return _case( + "lc-reviewer-mutation-blocked", + category="reviewer_mutation", + policy="local_commit", + result=result, + checks=checks, + ) + + +def lc_incomplete_review_blocked_verdict(tmp_dir: Path) -> dict[str, Any]: + repo = tmp_dir / "repo" + H.init_repo(repo) + base_sha, head_sha = H.start_candidate( + repo, branch="lc/incomplete", marker="broken" + ) + invocation = H.make_invocation( + repo, + policy="local_commit", + branch="lc/incomplete", + base_sha=base_sha, + head_sha=head_sha, + invocation_id="lc-incomplete-review", + ) + result = LC.run_local_commit( + invocation, + repo=repo, + reviewer=H.make_blocked_reviewer("aggregate lens coverage is incomplete"), + decide=H.accepting_decide, + apply_fix=H.fixing_apply_fix, + ) + branch_head = G.rev_parse(repo, "lc/incomplete") + checks = _terminal_checks( + result, terminal_state="blocked", reason="missing_capability" + ) + checks["candidate_never_advanced"] = (head_sha, branch_head) + return _case( + "lc-incomplete-review-blocked-verdict", + category="invalid_review", + policy="local_commit", + result=result, + checks=checks, + ) + + +def lc_invalid_stale_review_result_blocked(tmp_dir: Path) -> dict[str, Any]: + repo = tmp_dir / "repo" + H.init_repo(repo) + base_sha, head_sha = H.start_candidate( + repo, branch="lc/stale-review", marker="broken" + ) + invocation = H.make_invocation( + repo, + policy="local_commit", + branch="lc/stale-review", + base_sha=base_sha, + head_sha=head_sha, + invocation_id="lc-stale-review-result", + ) + result = LC.run_local_commit( + invocation, + repo=repo, + reviewer=H.make_malformed_reviewer(), + decide=H.accepting_decide, + apply_fix=H.fixing_apply_fix, + ) + branch_head = G.rev_parse(repo, "lc/stale-review") + checks = _terminal_checks( + result, terminal_state="blocked", reason="reviewer_integrity_failure" + ) + checks["candidate_never_advanced"] = (head_sha, branch_head) + return _case( + "lc-invalid-stale-review-result-blocked", + category="invalid_review", + policy="local_commit", + result=result, + checks=checks, + ) + + +# --------------------------------------------------------------------------- +# local_commit: fresh-subagent default vs. explicit in-agent override +# --------------------------------------------------------------------------- + + +def lc_missing_capability_without_override(tmp_dir: Path) -> dict[str, Any]: + repo = tmp_dir / "repo" + H.init_repo(repo) + base_sha, head_sha = H.start_candidate( + repo, branch="lc/no-fresh-host", marker="fixed" + ) + invocation = H.make_invocation( + repo, + policy="local_commit", + branch="lc/no-fresh-host", + base_sha=base_sha, + head_sha=head_sha, + invocation_id="lc-missing-capability", + ) + result = LC.run_local_commit( + invocation, + repo=repo, + reviewer=H.make_clean_reviewer(), + decide=H.accepting_decide, + apply_fix=H.fixing_apply_fix, + host_supports_fresh_subagent=False, + ) + branch_head = G.rev_parse(repo, "lc/no-fresh-host") + checks = _terminal_checks( + result, terminal_state="blocked", reason="missing_capability" + ) + checks["candidate_never_advanced"] = (head_sha, branch_head) + return _case( + "lc-missing-capability-without-override", + category="review_mode", + policy="local_commit", + result=result, + checks=checks, + ) + + +def lc_in_agent_override_bypasses_missing_capability(tmp_dir: Path) -> dict[str, Any]: + repo = tmp_dir / "repo" + H.init_repo(repo) + base_sha, head_sha = H.start_candidate(repo, branch="lc/override", marker="broken") + invocation = H.make_invocation( + repo, + policy="local_commit", + branch="lc/override", + base_sha=base_sha, + head_sha=head_sha, + invocation_id="lc-in-agent-override", + review_execution={ + "mode": "in_agent_override", + "override_authorization": "user-explicit-2026-07-31", + }, + ) + result = LC.run_local_commit( + invocation, + repo=repo, + reviewer=H.make_marker_reviewer(repo), + decide=H.accepting_decide, + apply_fix=H.fixing_apply_fix, + # No host in this process can honor `fresh_subagent`; the override + # must still be honored per design ("An explicit override is always + # honored, regardless of host capability"). + host_supports_fresh_subagent=False, + ) + branch_head = G.rev_parse(repo, "lc/override") + checks = _terminal_checks(result, terminal_state="converged", reason=None) + checks.update( + { + "exactly_one_new_commit": ( + 1, + G.rev_list_count(repo, head_sha, branch_head), + ), + "marker_at_final_head": ( + H.MARKER_FIXED, + G.show_file(repo, branch_head, "marker.txt"), + ), + "review_independence_is_in_agent_override": ( + "in_agent_override", + (result.get("review_records") or [{}])[-1].get("review_independence"), + ), + } + ) + return _case( + "lc-in-agent-override-bypasses-missing-capability", + category="review_mode", + policy="local_commit", + result=result, + checks=checks, + ) + + +# --------------------------------------------------------------------------- +# local_commit: checkpoint mismatch + interruption/recovery +# --------------------------------------------------------------------------- + + +def lc_checkpoint_mismatch_resume_blocked(tmp_dir: Path) -> dict[str, Any]: + repo = tmp_dir / "repo" + H.init_repo(repo) + base_sha, head_sha = H.start_candidate(repo, branch="lc/mismatch", marker="broken") + invocation_id = "lc-checkpoint-mismatch" + invocation = H.make_invocation( + repo, + policy="local_commit", + branch="lc/mismatch", + base_sha=base_sha, + head_sha=head_sha, + invocation_id=invocation_id, + ) + # First run: stops at "decide" (declined finding) so a real checkpoint is + # persisted without ever advancing the canonical head. + LC.run_local_commit( + invocation, + repo=repo, + reviewer=H.make_marker_reviewer(repo), + decide=H.make_rejecting_decide(), + apply_fix=H.fixing_apply_fix, + ) + common_dir = LE.git_common_dir(repo) + checkpoint_path = LC.LE.checkpoint_path(common_dir, invocation_id) + checkpoint = LC.LE.read_checkpoint(checkpoint_path) + # Corrupt the one field `reconcile_checkpoint_for_resume` checks last: a + # real, well-formed, but wrong `current_head`. + tampered = dict(checkpoint) + tampered["current_head"] = base_sha + result = LC.run_local_commit( + invocation, + repo=repo, + reviewer=H.make_marker_reviewer(repo), + decide=H.accepting_decide, + apply_fix=H.fixing_apply_fix, + resume_checkpoint=tampered, + ) + branch_head = G.rev_parse(repo, "lc/mismatch") + checks = _terminal_checks( + result, terminal_state="blocked", reason="checkpoint_mismatch" + ) + checks["candidate_never_advanced"] = (head_sha, branch_head) + return _case( + "lc-checkpoint-mismatch-resume-blocked", + category="recovery", + policy="local_commit", + result=result, + checks=checks, + ) + + +def lc_interrupted_attempt_recovered_without_losing_commit( + tmp_dir: Path, +) -> dict[str, Any]: + repo = tmp_dir / "repo" + H.init_repo(repo) + base_sha, head_sha = H.start_candidate(repo, branch="lc/recovery", marker="broken") + invocation_id = "lc-recovery" + invocation = H.make_invocation( + repo, + policy="local_commit", + branch="lc/recovery", + base_sha=base_sha, + head_sha=head_sha, + invocation_id=invocation_id, + ) + # First run: stops at "decide" (declined) so a real checkpoint exists + # with no promoted commit yet. + LC.run_local_commit( + invocation, + repo=repo, + reviewer=H.make_marker_reviewer(repo), + decide=H.make_rejecting_decide(), + apply_fix=H.fixing_apply_fix, + ) + common_dir = LE.git_common_dir(repo) + checkpoint_path = LC.LE.checkpoint_path(common_dir, invocation_id) + checkpoint = LC.LE.read_checkpoint(checkpoint_path) + + # Simulate a crash between `commit_attempt` and `promote_attempt`: create + # and commit an attempt branch by hand, but never promote it. + attempts_root = LC.LE.default_attempts_root(common_dir) + handle = LC.LE.create_attempt( + repo=repo, + attempts_root=attempts_root, + base_sha=head_sha, + invocation_id=invocation_id, + sequence=1, + ) + (handle.path / "marker.txt").write_text("fixed\n") + interrupted_sha = LC.LE.commit_attempt(handle, "manually interrupted commit") + attempt_branch_ref = f"refs/heads/{handle.branch}" + + result = LC.run_local_commit( + invocation, + repo=repo, + reviewer=H.make_marker_reviewer(repo), + decide=H.accepting_decide, + apply_fix=H.fixing_apply_fix, + resume_checkpoint=checkpoint, + ) + branch_head = G.rev_parse(repo, "lc/recovery") + checks = _terminal_checks(result, terminal_state="converged", reason=None) + checks.update( + { + # Only the fresh, post-recovery attempt was ever promoted onto + # the canonical branch. + "exactly_one_promoted_commit": ( + 1, + G.rev_list_count(repo, head_sha, branch_head), + ), + "marker_at_final_head": ( + H.MARKER_FIXED, + G.show_file(repo, branch_head, "marker.txt"), + ), + # The interrupted commit's object was never lost: its attempt + # branch still points at it even though it was never promoted. + "interrupted_commit_object_preserved": ( + True, + G.object_exists(repo, interrupted_sha), + ), + "interrupted_attempt_branch_still_points_at_it": ( + interrupted_sha, + G.rev_parse(repo, attempt_branch_ref), + ), + "interrupted_attempt_recorded_as_preserved": ( + True, + any( + entry.get("attempt_ref") == handle.branch + for entry in result.get("preserved_failed_attempts", []) + ), + ), + } + ) + return _case( + "lc-interrupted-attempt-recovered-without-losing-commit", + category="recovery", + policy="local_commit", + result=result, + checks=checks, + ) + + +# --------------------------------------------------------------------------- +# update_pr: convergence + publication +# --------------------------------------------------------------------------- + + +def up_converged_published(tmp_dir: Path) -> dict[str, Any]: + repo = tmp_dir / "repo" + H.init_repo(repo) + bare = H.init_bare_remote(tmp_dir) + base_sha, head_sha = H.start_candidate( + repo, branch="up/converge", marker="broken", bare=bare + ) + head_ref = "refs/heads/up/converge" + remote_before = G.ls_remote(bare, head_ref) + invocation = H.make_invocation( + repo, + policy="update_pr", + branch="up/converge", + base_sha=base_sha, + head_sha=head_sha, + invocation_id="up-converged-published", + bare=bare, + ) + result = UP.run_update_pr( + invocation, + repo=repo, + reviewer=H.make_marker_reviewer(repo), + decide=H.accepting_decide, + apply_fix=H.fixing_apply_fix, + ) + branch_head = G.rev_parse(repo, "up/converge") + remote_after = G.ls_remote(bare, head_ref) + checks = _terminal_checks(result, terminal_state="converged", reason=None) + checks.update( + { + "remote_started_at_expected_old_head": (head_sha, remote_before), + "remote_advanced_to_exact_local_head": (branch_head, remote_after), + "marker_at_published_head": ( + H.MARKER_FIXED, + G.show_file(repo, remote_after or "", "marker.txt"), + ), + } + ) + return _case( + "up-converged-published", + category="convergence", + policy="update_pr", + result=result, + checks=checks, + ) + + +# --------------------------------------------------------------------------- +# update_pr: stale target / remote already advanced +# --------------------------------------------------------------------------- + + +def up_remote_advanced_stale_target_blocked(tmp_dir: Path) -> dict[str, Any]: + repo = tmp_dir / "repo" + H.init_repo(repo) + bare = H.init_bare_remote(tmp_dir) + base_sha, head_sha = H.start_candidate( + repo, branch="up/stale", marker="fixed", bare=bare + ) + head_ref = "refs/heads/up/stale" + + # Someone else's clone advances the remote before this invocation runs. + other_clone = tmp_dir / "other-clone" + LE.git("clone", "-q", str(repo), str(other_clone)) + LE.git("checkout", "-q", "up/stale", cwd=other_clone) + (other_clone / "marker.txt").write_text("someone-else-fixed-it\n") + LE.git("add", "-A", cwd=other_clone) + LE.git( + "-c", + "user.email=other@example.com", + "-c", + "user.name=Other", + "commit", + "-q", + "-m", + "a competing fix", + cwd=other_clone, + ) + LE.git("push", str(bare), "up/stale:refs/heads/up/stale", cwd=other_clone) + competing_head = LE.current_head(other_clone) + + invocation = H.make_invocation( + repo, + policy="update_pr", + branch="up/stale", + base_sha=base_sha, + head_sha=head_sha, + invocation_id="up-remote-advanced", + bare=bare, + ) + result = UP.run_update_pr( + invocation, + repo=repo, + reviewer=H.make_clean_reviewer(), + decide=H.accepting_decide, + apply_fix=H.fixing_apply_fix, + ) + remote_after = G.ls_remote(bare, head_ref) + checks = _terminal_checks( + result, terminal_state="blocked", reason="remote_advanced" + ) + checks.update( + { + "remote_untouched_by_the_losing_invocation": (competing_head, remote_after), + "local_converged_commit_not_lost": ( + True, + G.object_exists(repo, result.get("head", {}).get("final", "")), + ), + } + ) + return _case( + "up-remote-advanced-stale-target-blocked", + category="publication_race", + policy="update_pr", + result=result, + checks=checks, + ) + + +def up_sequential_publication_race_second_clone_loses(tmp_dir: Path) -> dict[str, Any]: + """Two independent local clones of the same PR branch/head both converge + locally; the first to publish wins, and the second is deterministically + interleaved (via its own reviewer callback, never real concurrency) to + attempt publication only after the first has already landed.""" + canonical = tmp_dir / "repo" + H.init_repo(canonical) + bare = H.init_bare_remote(tmp_dir) + base_sha, head_sha = H.start_candidate( + canonical, branch="up/race", marker="broken", bare=bare + ) + head_ref = "refs/heads/up/race" + + def _clone_with_identity(name: str) -> Path: + # `git clone` never copies the source repository's local + # `user.name`/`user.email` config, and a CI runner (unlike a + # developer machine) typically has no global git identity + # configured either — the engine's own `commit_attempt` relies on + # ambient git identity exactly as production code does, so each + # clone needs its own explicit local config, matching what + # `H.init_repo` already does for every non-cloned fixture repo. + clone_path = tmp_dir / name + LE.git("clone", "-q", str(canonical), str(clone_path)) + LE.git("checkout", "-q", "up/race", cwd=clone_path) + LE.git("config", "user.email", "test@example.com", cwd=clone_path) + LE.git("config", "user.name", "Eval", cwd=clone_path) + return clone_path + + repo_a = _clone_with_identity("clone-a") + repo_b = _clone_with_identity("clone-b") + + invocation_a = H.make_invocation( + repo_a, + policy="update_pr", + branch="up/race", + base_sha=base_sha, + head_sha=head_sha, + invocation_id="up-race-a", + bare=bare, + ) + invocation_b = H.make_invocation( + repo_b, + policy="update_pr", + branch="up/race", + base_sha=base_sha, + head_sha=head_sha, + invocation_id="up-race-b", + bare=bare, + ) + + base_reviewer_b = H.make_marker_reviewer(repo_b) + result_a_box: dict[str, Any] = {} + + def reviewer_b(**kwargs): + pass_result = base_reviewer_b(**kwargs) + if pass_result.result.get("verdict") == "clean": + # Deterministically interleave: clone A completes its entire + # converge-and-publish run right before clone B's own clean + # review would let it proceed to publish. + result_a_box["result"] = UP.run_update_pr( + invocation_a, + repo=repo_a, + reviewer=H.make_marker_reviewer(repo_a), + decide=H.accepting_decide, + apply_fix=H.fixing_apply_fix, + ) + return pass_result + + def apply_fix_b(*, finding, attempt_path, change_contract, attempt_number): + del finding, change_contract, attempt_number + (attempt_path / "marker.txt").write_text(H.MARKER_FIXED + "\n") + # A distinct commit message from clone A's `fixing_apply_fix`, so the + # two clones' fix commits never collide on identical tree, parent, + # message, *and* same-second timestamp — which git would otherwise + # hash to the exact same commit object, defeating this scenario's + # "the loser's own distinct local commit is not lost" evidence. + return f"fix: resolve {H.FINDING_ID} (clone B)" + + result_b = UP.run_update_pr( + invocation_b, + repo=repo_b, + reviewer=reviewer_b, + decide=H.accepting_decide, + apply_fix=apply_fix_b, + ) + result_a = result_a_box["result"] + + remote_final = G.ls_remote(bare, head_ref) + branch_head_a = G.rev_parse(repo_a, "up/race") + branch_head_b = G.rev_parse(repo_b, "up/race") + + checks: dict[str, tuple[Any, Any]] = { + "winner_terminal_state": ("converged", result_a.get("terminal_state")), + "loser_terminal_state": ("blocked", result_b.get("terminal_state")), + "loser_reason": ("remote_advanced", result_b.get("reason")), + "remote_equals_winners_local_head": (branch_head_a, remote_final), + "remote_does_not_equal_losers_local_head": ( + True, + remote_final != branch_head_b, + ), + # The loser's own local fix commit is not lost, merely unpublished. + "losers_local_commit_exists": (True, branch_head_b != head_sha), + "losers_local_commit_object_preserved": ( + True, + G.object_exists(repo_b, branch_head_b), + ), + "loser_publication_status": ( + "failed", + result_b.get("publication", {}).get("status"), + ), + } + return _case( + "up-sequential-publication-race-second-clone-loses", + category="publication_race", + policy="update_pr", + result=result_b, + checks=checks, + ) + + +# --------------------------------------------------------------------------- +# update_pr: publication target / grant validation (fail closed before any lock) +# --------------------------------------------------------------------------- + + +def up_missing_authority_target_mismatch_blocked(tmp_dir: Path) -> dict[str, Any]: + repo = tmp_dir / "repo" + H.init_repo(repo) + bare = H.init_bare_remote(tmp_dir) + base_sha, head_sha = H.start_candidate( + repo, branch="up/mismatch", marker="fixed", bare=bare + ) + head_ref = "refs/heads/up/mismatch" + invocation = H.make_invocation( + repo, + policy="update_pr", + branch="up/mismatch", + base_sha=base_sha, + head_sha=head_sha, + invocation_id="up-target-mismatch", + bare=bare, + source_repository="someone-else/a-fork", + ) + result = UP.run_update_pr( + invocation, + repo=repo, + reviewer=H.make_clean_reviewer(), + decide=H.accepting_decide, + apply_fix=H.fixing_apply_fix, + ) + remote_after = G.ls_remote(bare, head_ref) + checks = _terminal_checks( + result, terminal_state="blocked", reason="missing_authority" + ) + checks.update( + { + "remote_untouched_before_any_lock": (head_sha, remote_after), + "no_cycle_was_ever_consumed": ( + 0, + result.get("budget", {}).get("consumed_cycles"), + ), + } + ) + return _case( + "up-missing-authority-target-mismatch-blocked", + category="publication_target", + policy="update_pr", + result=result, + checks=checks, + ) + + +def up_mismatched_grant_blocked(tmp_dir: Path) -> dict[str, Any]: + repo = tmp_dir / "repo" + H.init_repo(repo) + bare = H.init_bare_remote(tmp_dir) + base_sha, head_sha = H.start_candidate( + repo, branch="up/bad-grant", marker="fixed", bare=bare + ) + head_ref = "refs/heads/up/bad-grant" + invocation = H.make_invocation( + repo, + policy="update_pr", + branch="up/bad-grant", + base_sha=base_sha, + head_sha=head_sha, + invocation_id="up-bad-grant", + bare=bare, + grants=[ + { + "mechanism_id": "ci-check", + "kind": "external_ci", + "repository": "shaug/agent-scripts", + "ref": "refs/heads/some-other-branch", + "origin_only_evidence": "CI only evaluates the pushed remote ref", + } + ], + ) + result = UP.run_update_pr( + invocation, + repo=repo, + reviewer=H.make_clean_reviewer(), + decide=H.accepting_decide, + apply_fix=H.fixing_apply_fix, + ) + remote_after = G.ls_remote(bare, head_ref) + checks = _terminal_checks( + result, terminal_state="blocked", reason="missing_authority" + ) + checks["remote_untouched_before_any_lock"] = (head_sha, remote_after) + return _case( + "up-mismatched-grant-blocked", + category="publication_target", + policy="update_pr", + result=result, + checks=checks, + ) + + +SCENARIOS: list[Callable[[Path], dict[str, Any]]] = [ + lc_converged_clean_initial, + lc_converged_after_one_fix, + lc_budget_exhausted_persistent_finding, + lc_declined_finding_blocked, + lc_scope_expanding_finding_blocked, + lc_validation_unavailable_blocked, + lc_validation_failure_not_tractable, + lc_validation_failure_tractable_converges, + lc_reviewer_mutation_blocked, + lc_incomplete_review_blocked_verdict, + lc_invalid_stale_review_result_blocked, + lc_missing_capability_without_override, + lc_in_agent_override_bypasses_missing_capability, + lc_checkpoint_mismatch_resume_blocked, + lc_interrupted_attempt_recovered_without_losing_commit, + up_converged_published, + up_remote_advanced_stale_target_blocked, + up_sequential_publication_race_second_clone_loses, + up_missing_authority_target_mismatch_blocked, + up_mismatched_grant_blocked, +] + +SCENARIOS_BY_ID: dict[str, Callable[[Path], dict[str, Any]]] = { + scenario.__name__: scenario for scenario in SCENARIOS +} diff --git a/skills/review-fix-loop/scripts/evals/grader.py b/skills/review-fix-loop/scripts/evals/grader.py new file mode 100644 index 0000000..4458382 --- /dev/null +++ b/skills/review-fix-loop/scripts/evals/grader.py @@ -0,0 +1,87 @@ +"""Result-blind grading primitives for the review-fix-loop evaluation corpus. + +Acceptance criterion: "Evaluators are result-blind and cannot pass solely +because the implementation asserts success." Every function here derives its +verdict from live Git state (or, for locks, live filesystem/`flock` state) — +never merely from the returned terminal-result document's own claims. A +scenario in `corpus.py` may still read `result["head"]["final"]` to know +*which* SHA to independently re-derive evidence about, but the grading +comparison itself is always against a value this module computed by asking +Git directly, so a terminal result that lies about the candidate's state +disagrees with these functions rather than being trusted by them. + +`grade_case` itself is deliberately generic: it does not know about +"convergence" or "budget exhaustion" or any other scenario category. Each +scenario in `corpus.py` supplies its own `checks` mapping of +`name -> (expected, observed)` pairs, where `observed` was computed by one of +this module's independent-evidence helpers (or, for the two fields the +terminal-result contract itself defines, read from the result and compared +against this corpus's own independently decided expectation). `grade_case` +only diffs those pairs and reports exactly which named check failed for which +fixture — satisfying "evaluation output identifies the exact fixture, +observed evidence, and reason for failure." +""" + +from __future__ import annotations + +import sys +from pathlib import Path +from typing import Any + +HERE = Path(__file__).resolve().parent +if str(HERE) not in sys.path: + sys.path.insert(0, str(HERE)) + +from helpers import LE # noqa: E402 + + +def rev_parse(repo: Path, ref: str) -> str | None: + result = LE.git("rev-parse", "--verify", ref, cwd=repo, check=False) + if result.returncode != 0: + return None + return result.stdout.strip() + + +def ls_remote(remote: Path, ref: str) -> str | None: + result = LE.git("ls-remote", str(remote), ref, check=False) + if result.returncode != 0 or not result.stdout.strip(): + return None + return result.stdout.split()[0] + + +def show_file(repo: Path, ref: str, path: str) -> str | None: + result = LE.git("show", f"{ref}:{path}", cwd=repo, check=False) + if result.returncode != 0: + return None + return result.stdout.strip() + + +def object_exists(repo: Path, sha: str) -> bool: + return LE.git("cat-file", "-e", sha, cwd=repo, check=False).returncode == 0 + + +def rev_list_count(repo: Path, old: str, new: str) -> int: + return int(LE.git("rev-list", "--count", f"{old}..{new}", cwd=repo).stdout.strip()) + + +def worktree_is_clean(repo: Path) -> bool: + status = LE.worktree_status(repo) + return not status["staged"] and not status["unstaged"] and not status["untracked"] + + +def untracked_paths(repo: Path) -> list[str]: + return LE.worktree_status(repo)["untracked"] + + +def grade_case(case: dict[str, Any]) -> list[str]: + """Return `f"{case_id}: {check_name}: expected ... observed ..."` failure + strings for every mismatched `checks` entry. Empty means every + independently derived expectation for this fixture held.""" + case_id = case["id"] + failures: list[str] = [] + for name, (expected, observed) in case["checks"].items(): + if expected != observed: + failures.append( + f"{case_id}: {name}: expected {expected!r}, observed {observed!r}" + ) + return failures diff --git a/skills/review-fix-loop/scripts/evals/helpers.py b/skills/review-fix-loop/scripts/evals/helpers.py new file mode 100644 index 0000000..fe4db9a --- /dev/null +++ b/skills/review-fix-loop/scripts/evals/helpers.py @@ -0,0 +1,440 @@ +"""Shared fixture plumbing for review-fix-loop's cross-cutting evaluation +corpus (issue #101). + +This mirrors the module-loading and "no mocked Git state" conventions +`scripts/tests/helpers.py` already established for the capability-owned unit +suites (`test_local_commit.py`, `test_update_pr.py`): every scenario drives +the real `local_commit.run_local_commit` / `update_pr.run_update_pr` entry +points against a real temporary Git repository (and, for `update_pr`, a real +disposable local bare repository used as the publication remote — never this +repository's actual `origin`). + +It imports the identical subset of `scripts/tests/helpers.py`'s own fixtures +(`init_repo`, `CLEAN_TEMPLATE`, `ALWAYS_PASS_VALIDATION`, `finding`, +`make_clean_reviewer`, `fixing_apply_fix`, `accepting_decide`) rather than +redefining them, following this repository's own `carve-changesets` precedent +of one skill's `scripts/tests/`/`scripts/evals/` fixture modules importing +across that boundary +(`skills/carve-changesets/scripts/tests/test_evals.py` imports directly from +its sibling `scripts/evals/`). Every fixture below — +imported or defined here — is duck-typed (attribute access only, never +`isinstance`), matching `scripts/tests/helpers.py`'s own documented +convention: constructing a `ReviewPass`/`FixDecision` from one module's +separately loaded `local_commit.py` is exactly as valid to another module's +separately loaded engine as one built from that engine's own load. Only the +fixtures genuinely specific to this corpus (more reviewer/decide/apply_fix +shapes than any one capability's unit tests need, because this corpus +exercises the full cross-cutting scenario list from +`design/review-fix-loop.md`'s "Validation strategy" section rather than one +module's own contract) are defined here. +""" + +from __future__ import annotations + +import importlib.util +import sys +from pathlib import Path +from typing import Any, Callable + +SKILL_ROOT = Path(__file__).resolve().parents[2] + + +def load_module(name: str, filename: str, *, subdir: str = "scripts"): + spec = importlib.util.spec_from_file_location(name, SKILL_ROOT / subdir / filename) + assert spec and spec.loader + module = importlib.util.module_from_spec(spec) + sys.modules[name] = module + spec.loader.exec_module(module) + return module + + +# One dedicated load, shared by every scenario builder in `corpus.py`. Every +# fixture below is duck-typed (attribute access only), matching the existing +# convention documented in `scripts/tests/helpers.py`. +UP = load_module("review_fix_loop_eval_update_pr", "update_pr.py") +LC = UP.LC +LE = LC.LE +VALIDATE = LC.VALIDATE + +# The capability unit suites' own shared fixture module — see this module's +# docstring for why importing across the tests/evals boundary is safe here. +TESTS = load_module( + "review_fix_loop_eval_tests_helpers", "helpers.py", subdir="scripts/tests" +) +init_repo = TESTS.init_repo +CLEAN_TEMPLATE = TESTS.CLEAN_TEMPLATE +ALWAYS_PASS_VALIDATION = TESTS.ALWAYS_PASS_VALIDATION +FINDING_ID = TESTS.FINDING_ID +finding = TESTS.finding +make_clean_reviewer = TESTS.make_clean_reviewer +fixing_apply_fix = TESTS.fixing_apply_fix +accepting_decide = TESTS.accepting_decide + +MARKER_FIXED = "fixed" +MARKER_BROKEN = "broken" + +# Fails exactly while `validation_flag.txt` reads anything other than `pass`, +# matching `scripts/tests/test_local_commit.py`'s established flag-gated +# validation convention. +FLAG_GATED_VALIDATION = [ + { + "name": "focused unit test", + "command": ( + 'python3 -c "import pathlib,sys; ' + "sys.exit(0 if pathlib.Path('validation_flag.txt').read_text().strip()" + "=='pass' else 1)\"" + ), + "scope": "focused", + }, + {"name": "full repository gate", "command": "true", "scope": "full"}, +] + + +# --------------------------------------------------------------------------- +# Repository + disposable remote fixtures +# --------------------------------------------------------------------------- + + +def init_bare_remote(root: Path, *, name: str = "remote.git") -> Path: + bare = root / name + LE.git("init", "-q", "--bare", str(bare)) + return bare + + +def start_candidate( + repo: Path, + *, + branch: str, + marker: str = MARKER_BROKEN, + validation_flag: str = "pass", + bare: Path | None = None, +) -> tuple[str, str]: + """Create `branch` off `main` with one commit, optionally pushing it to + `bare` at the same ref name. Returns `(base_sha, head_sha)`.""" + base_sha = LE.current_head(repo) + LE.git("checkout", "-q", "-b", branch, cwd=repo) + (repo / "marker.txt").write_text(marker + "\n") + (repo / "validation_flag.txt").write_text(validation_flag + "\n") + LE.git("add", "-A", cwd=repo) + LE.git("commit", "-q", "-m", "start candidate", cwd=repo) + head_sha = LE.current_head(repo) + if bare is not None: + LE.git("push", str(bare), f"{branch}:refs/heads/{branch}", cwd=repo) + return base_sha, head_sha + + +def make_invocation( + repo: Path, + *, + policy: str, + branch: str, + base_sha: str, + head_sha: str, + invocation_id: str, + bare: Path | None = None, + max_fix_cycles: int = 3, + validation: list[dict[str, str]] | None = None, + grants: list[dict[str, str]] | None = None, + review_execution: dict[str, str] | None = None, + head_repository: str = "shaug/agent-scripts", + source_repository: str | None = None, + remote_url: str | None = None, + expected_old_head_sha: str | None = None, +) -> dict[str, Any]: + """Build one schema-shaped invocation document for either publication + policy, mirroring `scripts/tests/test_local_commit.py` and + `scripts/tests/test_update_pr.py`'s own `make_invocation` builders (kept + as one parameterized function here since this corpus, unlike either unit + suite, exercises both policies from a single scenario registry).""" + common_dir = LE.git_common_dir(repo) + diff = LE.git("diff", base_sha, head_sha, cwd=repo).stdout + worktree = LE.worktree_status(repo) + document: dict[str, Any] = { + "schema_version": "1.0", + "invocation_id": invocation_id, + "repository": { + "identity": "shaug/agent-scripts", + "git_common_directory": str(common_dir), + }, + "candidate": { + "branch": branch, + "head_sha": head_sha, + "comparison_base": {"ref": "main", "sha": base_sha}, + "diff": {"format": "unified_diff", "complete": True, "content": diff}, + "worktree": worktree, + "all_changes_committed": True, + }, + "change_contract": { + "goal": "Fix the example.", + "acceptance_criteria": ["marker.txt reads 'fixed'"], + "non_goals": ["Unrelated refactors"], + "preserved_behaviors": ["Existing README content"], + "allowed_remediation_scope": "marker.txt only", + "sources": { + "repository_instructions": [], + "named_documents": [], + "nearby_patterns": [], + }, + }, + "review_execution": review_execution or {"mode": "fresh_subagent"}, + "fix_cycle_budget": {"max_fix_cycles": max_fix_cycles}, + "validation": validation or ALWAYS_PASS_VALIDATION, + } + + if policy == "local_commit": + document["candidate"]["source_unavailable_reason"] = ( + "standalone invocation has no recorded pushable source" + ) + document["publication"] = {"policy": "local_commit"} + return document + + if policy != "update_pr": + raise ValueError(f"unknown policy: {policy!r}") + + assert bare is not None, "update_pr scenarios require a bare remote" + head_ref = f"refs/heads/{branch}" + document["candidate"]["pull_request"] = { + "repository": "shaug/agent-scripts", + "number": 123, + } + document["candidate"]["source_binding"] = { + "repository": source_repository or head_repository, + "remote_url": remote_url or str(bare), + "ref": head_ref, + "observed_object_id": head_sha, + } + publication: dict[str, Any] = { + "policy": "update_pr", + "pull_request": { + "head_repository": head_repository, + "head_ref": head_ref, + "expected_old_head_sha": expected_old_head_sha or head_sha, + "base_ref": "main", + "base_sha": base_sha, + }, + } + if grants is not None: + publication["remote_iteration_grants"] = grants + document["publication"] = publication + return document + + +# --------------------------------------------------------------------------- +# Reviewer fixtures +# --------------------------------------------------------------------------- + + +def _lens_executions(head_sha: str, comparison_base_sha: str) -> list[dict[str, Any]]: + return [ + { + "lens": lens, + "head_sha": head_sha, + "comparison_base_sha": comparison_base_sha, + "verdict": "clean", + "freshly_executed": True, + } + for lens in ("solution_simplicity", "correctness", "code_simplicity") + ] + + +def make_marker_reviewer(repo: Path) -> Callable[..., Any]: + """`clean` iff `marker.txt` reads 'fixed' at the exact reviewed head, + `changes_required` with one blocking finding otherwise. A real function + of real repository content, never a call counter.""" + + def reviewer( + *, packet, briefing, head_sha, comparison_base_sha, independence, sequence + ): + del packet, briefing, independence, sequence + content = LE.git("show", f"{head_sha}:marker.txt", cwd=repo).stdout.strip() + candidate = {"head_sha": head_sha, "comparison_base_sha": comparison_base_sha} + if content == MARKER_FIXED: + result = { + **CLEAN_TEMPLATE, + "candidate": candidate, + "lens_executions": _lens_executions(head_sha, comparison_base_sha), + } + else: + result = { + **CLEAN_TEMPLATE, + "candidate": candidate, + "verdict": "changes_required", + "findings": [finding()], + "lens_executions": [ + { + "lens": "correctness", + "head_sha": head_sha, + "comparison_base_sha": comparison_base_sha, + "verdict": "changes_required", + "freshly_executed": True, + } + ], + "next_action": f"Fix {FINDING_ID}.", + } + return LC.ReviewPass(result=result) + + return reviewer + + +def make_blocked_reviewer(reason: str = "coverage gap") -> Callable[..., Any]: + """The complete aggregate review itself returns `blocked` (design: + "the review pass itself returned blocked"), simulating incomplete review + evidence rather than a normal changes_required/clean verdict.""" + + def reviewer( + *, packet, briefing, head_sha, comparison_base_sha, independence, sequence + ): + del packet, briefing, independence, sequence + candidate = {"head_sha": head_sha, "comparison_base_sha": comparison_base_sha} + result = { + "schema_version": "1.4", + "lens": "aggregate", + "verdict": "blocked", + "candidate": candidate, + "findings": [], + "blocking_reasons": [reason], + "validation_limitations": [], + "next_action": f"Resolve: {reason}.", + } + return LC.ReviewPass(result=result) + + return reviewer + + +def make_malformed_reviewer() -> Callable[..., Any]: + """Returns a result bound to the wrong head — an "incomplete/invalid + review" that `evaluate_review_result` must reject before it can ever + certify anything, regardless of its claimed verdict.""" + + def reviewer( + *, packet, briefing, head_sha, comparison_base_sha, independence, sequence + ): + del packet, briefing, independence, sequence + # Deliberately bound to a fabricated, non-matching head: a stale or + # cross-candidate result. + candidate = { + "head_sha": "0" * 40, + "comparison_base_sha": comparison_base_sha, + } + result = { + **CLEAN_TEMPLATE, + "candidate": candidate, + "lens_executions": _lens_executions("0" * 40, comparison_base_sha), + } + return LC.ReviewPass(result=result) + + return reviewer + + +def make_mutating_reviewer(repo: Path, inner: Callable[..., Any]) -> Callable[..., Any]: + """Wraps `inner` but also writes an untracked file into the canonical + worktree before returning — a reviewer attempting a prohibited mutation. + `_run_engine`'s before/after worktree snapshot around the reviewer call + must detect this regardless of the wrapped verdict.""" + + def reviewer(**kwargs): + (repo / "reviewer-attempted-write.txt").write_text("i should not be here\n") + return inner(**kwargs) + + return reviewer + + +# --------------------------------------------------------------------------- +# Decide fixtures +# --------------------------------------------------------------------------- + + +def make_rejecting_decide( + rationale: str = "not a genuine defect", +) -> Callable[..., Any]: + def decide(*, finding, change_contract, attempt_number): + del change_contract, attempt_number + return LC.FixDecision(disposition="rejected", rationale=rationale) + + return decide + + +def make_scope_expanding_decide() -> Callable[..., Any]: + def decide(*, finding, change_contract, attempt_number): + del change_contract, attempt_number + return LC.FixDecision( + disposition="accepted", + rationale=f"{finding['id']} is real but only fixable outside scope", + expands_scope=True, + ) + + return decide + + +# --------------------------------------------------------------------------- +# Apply-fix fixtures +# --------------------------------------------------------------------------- + + +def make_never_fixing_apply_fix(content: str = "still-broken") -> Callable[..., Any]: + """A deliberately incomplete fix: commits real progress but never writes + the content the reviewer actually requires, so the same finding persists + across every subsequent review. Used for the budget-exhaustion scenario, + and (unmodified) for this ticket's own seeded-faulty-fixture + demonstration.""" + + def apply_fix(*, finding, attempt_path, change_contract, attempt_number): + del finding, change_contract + (attempt_path / "marker.txt").write_text(f"{content}-{attempt_number}\n") + return f"fix: partial attempt {attempt_number}" + + return apply_fix + + +def make_flag_fixing_apply_fix() -> Callable[..., Any]: + """Fixes `validation_flag.txt` (not `marker.txt`) — pairs with a + synthetic validation-failure finding rather than a review finding.""" + + def apply_fix(*, finding, attempt_path, change_contract, attempt_number): + del finding, change_contract, attempt_number + (attempt_path / "validation_flag.txt").write_text("pass\n") + return "fix: repair validation_flag.txt" + + return apply_fix + + +# --------------------------------------------------------------------------- +# Validation-classification fixtures +# --------------------------------------------------------------------------- + + +def classify_validation_failure_as_tractable(*, outcome, invocation): + del invocation + return { + "id": "validation-001", + "lens": "validation", + "severity": "blocking", + "confidence": "high", + "rule": "validation must pass", + "evidence": [{"location": "validation_flag.txt", "detail": outcome.result}], + "concern": "the focused validation command fails", + "impact": "the candidate is not demonstrably correct", + "proposed_change": "repair validation_flag.txt", + "expected_effect": "the focused validation command passes", + } + + +def classify_validation_failure_as_intractable(*, outcome, invocation): + del outcome, invocation + return None + + +def make_unavailable_validation_runner( + reason: str = "the validation environment is unreachable from this sandbox", +) -> Callable[..., Any]: + """A `run_validation` port that unconditionally reports its command as + unavailable — a deterministic stand-in for an environment failure (a + missing tool, an unreachable CI runner) distinct from an ordinary command + failure (`status: "failed"`), which `default_run_validation` only reports + when the command itself cannot even be spawned.""" + + def run_validation(*, name, command, scope, cwd): + del name, command, scope, cwd + return LC.ValidationOutcome(status="unavailable", reason=reason) + + return run_validation diff --git a/skills/review-fix-loop/scripts/evals/runner.py b/skills/review-fix-loop/scripts/evals/runner.py new file mode 100644 index 0000000..ad79e16 --- /dev/null +++ b/skills/review-fix-loop/scripts/evals/runner.py @@ -0,0 +1,117 @@ +#!/usr/bin/env python3 +"""Run the review-fix-loop cross-cutting evaluation corpus (issue #101). + +Every scenario in `corpus.py` drives the real `local_commit`/`update_pr` +engine against a real disposable Git repository (and, for `update_pr`, a +real disposable bare remote) — no subprocess boundary and no model call is +involved, matching `eval-carve-changesets`' +`--integration-self-test` convention: this is an objective, deterministic +replay, not an agent-judgment evaluation. Because the three genuinely +host-boundary actions (`reviewer`, `decide`, `apply_fix`) are supplied here as +small deterministic fixtures (`helpers.py`), the whole corpus is free to run +and safe for repository CI, satisfying this ticket's "deterministic enough +for repository CI" acceptance criterion. + +Grading is result-blind: `grader.grade_case` never trusts a scenario's +returned terminal-result document by itself. Each scenario computes its own +`checks` mapping against live Git state (see `corpus.py`'s module docstring), +and this runner's job is only to execute every scenario, grade it, and print +one JSON report that identifies the exact fixture, the mismatched evidence, +and the failure reason for every failing check. +""" + +from __future__ import annotations + +import argparse +import json +import sys +import tempfile +from pathlib import Path + +HERE = Path(__file__).resolve().parent +if str(HERE) not in sys.path: + sys.path.insert(0, str(HERE)) + +import corpus as CORPUS # noqa: E402 +import grader as GRADER # noqa: E402 + + +def run_scenario(name: str) -> dict: + scenario = CORPUS.SCENARIOS_BY_ID[name] + with tempfile.TemporaryDirectory(prefix=f"review-fix-loop-eval-{name}-") as tmp: + return scenario(Path(tmp)) + + +def run_all(ids: list[str] | None = None) -> tuple[dict[str, dict], list[str]]: + names = ids or list(CORPUS.SCENARIOS_BY_ID) + cases: dict[str, dict] = {} + failures: list[str] = [] + for name in names: + case = run_scenario(name) + cases[case["id"]] = case + failures.extend(GRADER.grade_case(case)) + return cases, failures + + +def build_parser() -> argparse.ArgumentParser: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--scenario", + action="append", + dest="scenarios", + help=( + "Run only this scenario function name (repeatable). Defaults to " + "the complete corpus." + ), + ) + parser.add_argument( + "--list", action="store_true", help="List every scenario id and exit." + ) + parser.add_argument("--output-dir", type=Path) + return parser + + +def write_outputs(output_dir: Path | None, cases: dict[str, dict]) -> None: + if output_dir is None: + return + output_dir.mkdir(parents=True, exist_ok=True) + for case_id, case in cases.items(): + report = { + "id": case["id"], + "category": case["category"], + "policy": case["policy"], + "checks": { + name: {"expected": expected, "observed": observed} + for name, (expected, observed) in case["checks"].items() + }, + } + (output_dir / f"{case_id}.json").write_text( + json.dumps(report, indent=2, sort_keys=True, default=str) + "\n" + ) + + +def main(argv: list[str] | None = None) -> int: + parser = build_parser() + args = parser.parse_args(argv) + + if args.list: + for name in CORPUS.SCENARIOS_BY_ID: + print(name) + return 0 + + cases, failures = run_all(args.scenarios) + write_outputs(args.output_dir, cases) + + failed_ids = {failure.split(":", 1)[0] for failure in failures} + summary = { + "total": len(cases), + "passed": len(cases) - len(failed_ids), + "failed": len(failed_ids), + "failures": failures, + } + print(json.dumps(summary, indent=2, sort_keys=True)) + return 1 if failures else 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/skills/review-fix-loop/scripts/tests/test_evals.py b/skills/review-fix-loop/scripts/tests/test_evals.py new file mode 100644 index 0000000..6c13997 --- /dev/null +++ b/skills/review-fix-loop/scripts/tests/test_evals.py @@ -0,0 +1,234 @@ +"""CI-run tests for review-fix-loop's cross-cutting evaluation corpus +(issue #101). + +Unlike `test_local_commit.py`/`test_update_pr.py` (which own each +capability's own contract assertions), this module tests the *evaluation +harness itself*: that the full result-blind corpus passes against the real +engine, that it covers the scope this ticket's body requires, and — the +ticket's own required demonstration — that the grader actually rejects a +fabricated success claim and a fixture that cannot really converge. It never +duplicates a `test_local_commit.py`/`test_update_pr.py` assertion; it proves +the *corpus* is trustworthy, matching `carve-changesets/scripts/tests/ +test_evals.py`'s own precedent for testing an eval harness under `just test`. +""" + +from __future__ import annotations + +import importlib.util +import sys +import tempfile +import unittest +from pathlib import Path + +SKILL_ROOT = Path(__file__).resolve().parents[2] +EVALS_DIR = SKILL_ROOT / "scripts" / "evals" +if str(EVALS_DIR) not in sys.path: + sys.path.insert(0, str(EVALS_DIR)) + + +def _load(name: str, filename: str): + spec = importlib.util.spec_from_file_location(name, EVALS_DIR / filename) + assert spec and spec.loader + module = importlib.util.module_from_spec(spec) + sys.modules[name] = module + spec.loader.exec_module(module) + return module + + +RUNNER = _load("review_fix_loop_eval_runner", "runner.py") +CORPUS = _load("review_fix_loop_eval_corpus", "corpus.py") +GRADER = _load("review_fix_loop_eval_grader", "grader.py") +HELPERS = _load("review_fix_loop_eval_helpers", "helpers.py") +LC = HELPERS.LC + +EXPECTED_SCENARIO_IDS = { + "lc-converged-clean-initial", + "lc-converged-after-one-fix", + "lc-budget-exhausted-persistent-finding", + "lc-declined-finding-blocked", + "lc-scope-expanding-finding-blocked", + "lc-validation-unavailable-blocked", + "lc-validation-failure-not-tractable", + "lc-validation-failure-tractable-converges", + "lc-reviewer-mutation-blocked", + "lc-incomplete-review-blocked-verdict", + "lc-invalid-stale-review-result-blocked", + "lc-missing-capability-without-override", + "lc-in-agent-override-bypasses-missing-capability", + "lc-checkpoint-mismatch-resume-blocked", + "lc-interrupted-attempt-recovered-without-losing-commit", + "up-converged-published", + "up-remote-advanced-stale-target-blocked", + "up-sequential-publication-race-second-clone-loses", + "up-missing-authority-target-mismatch-blocked", + "up-mismatched-grant-blocked", +} + +EXPECTED_CATEGORIES = { + "convergence", + "budget_exhaustion", + "declined_findings", + "validation_failure", + "reviewer_mutation", + "invalid_review", + "review_mode", + "recovery", + "publication_race", + "publication_target", +} + + +class CorpusShapeTests(unittest.TestCase): + """The corpus itself covers this ticket's required scope.""" + + @classmethod + def setUpClass(cls) -> None: + cls.cases, cls.failures = RUNNER.run_all() + + def test_corpus_covers_every_required_scenario_id(self) -> None: + self.assertEqual(EXPECTED_SCENARIO_IDS, set(self.cases)) + + def test_corpus_covers_every_required_category(self) -> None: + observed = {case["category"] for case in self.cases.values()} + self.assertEqual(EXPECTED_CATEGORIES, observed) + + def test_corpus_exercises_both_publication_policies(self) -> None: + policies = {case["policy"] for case in self.cases.values()} + self.assertEqual({"local_commit", "update_pr"}, policies) + + def test_every_case_carries_more_than_the_terminal_contract_fields(self) -> None: + """Every scenario's `checks` must include at least one + independently-derived Git-evidence entry beyond the terminal + contract's own self-reported `terminal_state`/`reason`/ + `schema_valid` — otherwise a scenario would just be re-parsing the + implementation's own claim.""" + self_report_only = {"terminal_state", "reason", "schema_valid"} + for case_id, case in self.cases.items(): + independent = set(case["checks"]) - self_report_only + self.assertTrue( + independent, f"{case_id} has no independent evidence checks" + ) + + def test_full_corpus_passes_against_the_real_engine(self) -> None: + self.assertEqual([], self.failures, "\n".join(self.failures)) + + def test_failure_messages_identify_fixture_evidence_and_reason(self) -> None: + """A synthetic mismatch (not a real corpus failure) must still read + as `: : expected ... observed ...` — this is + what lets an operator act on a real failure without re-running + anything, per this ticket's "identifies the exact fixture, observed + evidence, and reason for failure" acceptance criterion.""" + case = { + "id": "synthetic-example", + "checks": {"marker_at_final_head": ("fixed", "broken")}, + } + failures = GRADER.grade_case(case) + self.assertEqual( + [ + "synthetic-example: marker_at_final_head: expected 'fixed', " + "observed 'broken'" + ], + failures, + ) + + +class SeededFaultDemonstrationTests(unittest.TestCase): + """This ticket's Validation section requires demonstrating that a seeded + faulty implementation or fixture is rejected, not merely asserted to be. + Both tests below never call the real engine's success path; they prove + the grader's independent Git checks — not the returned document's own + claims — decide the outcome.""" + + def test_grader_rejects_a_fabricated_convergence_claim(self) -> None: + """A schema-shaped result that *claims* `converged` without any + fix cycle ever having run must still be rejected, because the + independent Git evidence (a real commit count and real file + content) disagrees with that claim.""" + with tempfile.TemporaryDirectory() as tmp: + tmp_dir = Path(tmp) + repo = tmp_dir / "repo" + HELPERS.init_repo(repo) + base_sha, head_sha = HELPERS.start_candidate( + repo, branch="lc/seeded-fault", marker="broken" + ) + del base_sha + # A faulty implementation that never actually ran a fix cycle, + # yet reports success. + fabricated_result = { + "terminal_state": "converged", + "head": {"initial": head_sha, "final": head_sha}, + } + branch_head = GRADER.rev_parse(repo, "lc/seeded-fault") + case = { + "id": "seeded-fault-fabricated-convergence", + "checks": { + "terminal_state": ( + "converged", + fabricated_result["terminal_state"], + ), + # The same independent checks a genuine + # lc-converged-after-one-fix case uses. + "exactly_one_new_commit": ( + 1, + GRADER.rev_list_count(repo, head_sha, branch_head), + ), + "marker_at_final_head": ( + "fixed", + GRADER.show_file(repo, branch_head, "marker.txt"), + ), + }, + } + failures = GRADER.grade_case(case) + self.assertTrue( + failures, "the grader must not be fooled by a self-reported success claim" + ) + joined = "\n".join(failures) + self.assertIn("exactly_one_new_commit", joined) + self.assertIn("marker_at_final_head", joined) + + def test_grader_rejects_a_fixture_declared_to_converge_but_cannot(self) -> None: + """A real engine run driven by a deliberately incomplete + `apply_fix` fixture (it never writes the content the reviewer + actually requires) cannot converge; declaring it "should converge" + anyway (as a miswritten fixture might) must fail, proving this + corpus does not rubber-stamp an arbitrary expectation either.""" + with tempfile.TemporaryDirectory() as tmp: + tmp_dir = Path(tmp) + repo = tmp_dir / "repo" + HELPERS.init_repo(repo) + base_sha, head_sha = HELPERS.start_candidate( + repo, branch="lc/broken-fixture", marker="broken" + ) + invocation = HELPERS.make_invocation( + repo, + policy="local_commit", + branch="lc/broken-fixture", + base_sha=base_sha, + head_sha=head_sha, + invocation_id="seeded-fault-broken-fixture", + max_fix_cycles=1, + ) + result = LC.run_local_commit( + invocation, + repo=repo, + reviewer=HELPERS.make_marker_reviewer(repo), + decide=HELPERS.accepting_decide, + # Deliberately incomplete: this fixture can never satisfy + # the reviewer, so the engine can never converge. + apply_fix=HELPERS.make_never_fixing_apply_fix(), + ) + # A miswritten fixture's expectation: "this converges." + case = { + "id": "seeded-fault-broken-fixture", + "checks": { + "terminal_state": ("converged", result.get("terminal_state")) + }, + } + failures = GRADER.grade_case(case) + self.assertTrue(failures) + self.assertIn("terminal_state", failures[0]) + self.assertEqual("changes_remaining", result.get("terminal_state")) + + +if __name__ == "__main__": + unittest.main()