From d184142377f912906d0ff747ab28b32231c75df1 Mon Sep 17 00:00:00 2001 From: Jonathan Singer Date: Tue, 29 Sep 2026 17:59:58 -0400 Subject: [PATCH] Let fix reviewer own validation and completion --- strix/fix/prepare.py | 51 ++++++++--------------------------- tests/test_fix_preparation.py | 42 ++++++++++++++++++++++------- 2 files changed, 44 insertions(+), 49 deletions(-) diff --git a/strix/fix/prepare.py b/strix/fix/prepare.py index 9c1a0afb2..e13aba8da 100644 --- a/strix/fix/prepare.py +++ b/strix/fix/prepare.py @@ -405,34 +405,6 @@ def _repair_outcome(value: RepairOutcome | None) -> RepairOutcome: ) -def validation_gaps( - checks: list[CheckResult], source_digest: str | None, requested: list[CommandSpec] -) -> list[str]: - """Check execution facts only; agents judge coverage and the security fix.""" - gaps: list[str] = [] - if not source_digest: - gaps.append("No source digest was recorded for the prepared patch.") - for purpose in ("regression", "unit"): - if not any(item.purpose == purpose and item.required for item in checks): - gaps.append(f"Run and record the required {purpose} tests.") # noqa: PERF401 - for item in checks: - if not item.required: - continue - if item.source_digest != source_digest or not item.environment_id: - gaps.append(f"Rerun {item.name}: its results are not for the current patch.") - elif item.status is not CheckStatus.PASSED or item.exit_code != 0 or item.tests_passed == 0: - gaps.append(f"Resolve and rerun {item.name}: {item.status}.") - if len({item.environment_id for item in checks if item.required}) > 1: - gaps.append("Required checks refer to different execution environments.") - for command in requested: - if command.required and not any( - item.required and item.argv == command.argv and item.cwd == command.cwd - for item in checks - ): - gaps.append(f"Run the requested check: {command.name}.") # noqa: PERF401 - return gaps - - async def prepare_fix( # noqa: PLR0915 - thin orchestration and cleanup request: FixPreparationRequestV1, workspace: Path, @@ -480,7 +452,7 @@ async def prepare_fix( # noqa: PLR0915 - thin orchestration and cleanup reproduction=next((c for c in checks if c.purpose == "regression"), None), ) - async def execute() -> FixPreparationResultV1: # noqa: PLR0912 + async def execute() -> FixPreparationResultV1: # noqa: PLR0911 - explicit terminal outcomes nonlocal checks, verifier, repair_turns, review_turns if cancelled(): raise PreparationCancelledError @@ -520,10 +492,10 @@ async def prepare_fix( # noqa: PLR0915 - thin orchestration and cleanup gaps=record.repair.gaps, ) if not manifest: - record.repair.gaps.append( - "No changed files were found. Implement the fix and regression test." + return await finish( + PreparationState.BLOCKED, + "Repair completed without a deliverable patch.", ) - continue if cancelled(): raise PreparationCancelledError # Review can investigate even incomplete validation and run the missing checks itself. @@ -545,21 +517,20 @@ async def prepare_fix( # noqa: PLR0915 - thin orchestration and cleanup continue if verifier.decision is not VerificationDecision.VERIFIED: return await finish(PreparationState.BLOCKED, verifier.summary, gaps=verifier.gaps) - gaps = validation_gaps(checks, record.repair.source_digest, request.checks) + # Test selection, failures, reruns and coverage belong to the reviewer. + # Only the artifact identity is checked here; it never starts another repair. if ( record.workspace_digest != await _workspace_digest(workspace) or verifier.source_digest != record.repair.source_digest ): - gaps.append( - "The deliverable changed during review. Inspect the diff, clean up temporary " - "files, and rerun affected checks before requesting review again." + return await finish( + PreparationState.BLOCKED, + "The deliverable changed during review; " + "the approved patch cannot be delivered.", ) - if gaps: - record.repair.gaps.extend(gaps) - continue return await finish( PreparationState.READY, - "Required tests passed and independent review approved the draft PR.", + "Independent review approved the draft PR. See the review for validation results.", ) return await finish( PreparationState.BLOCKED, diff --git a/tests/test_fix_preparation.py b/tests/test_fix_preparation.py index b856dfaef..6cb0f40f2 100644 --- a/tests/test_fix_preparation.py +++ b/tests/test_fix_preparation.py @@ -535,7 +535,7 @@ async def test_review_can_request_more_than_two_repairs(tmp_path: Path) -> None: @pytest.mark.asyncio @pytest.mark.parametrize("defect", ["missing_unit", "failed", "empty", "stale", "missing_request"]) -async def test_approval_cannot_waive_required_execution_evidence( +async def test_agent_approval_owns_test_evidence_without_controller_retries( tmp_path: Path, defect: str ) -> None: workspace, commit = _workspace(tmp_path) @@ -559,9 +559,12 @@ async def test_approval_cannot_waive_required_execution_evidence( request = _request(_candidate(commit)) request.max_agent_turns = 2 result = await prepare_fix(request, workspace, repair=repair, verify=_verified) - assert result.state is PreparationState.BLOCKED + # Deliberately scripted approval: test policy is the reviewer's responsibility. + # This tests routing, not whether a real reviewer ought to approve this evidence. + assert result.state is PreparationState.READY assert result.final_file_manifest - assert result.attempt_history[0].repair.gaps + assert result.attempts == 1 + assert not result.attempt_history[0].repair.gaps @pytest.mark.asyncio @@ -640,7 +643,9 @@ async def test_interruptions_preserve_partial_patch_without_approval( @pytest.mark.asyncio -async def test_review_mutation_returns_to_repair_instead_of_destroying_run(tmp_path: Path) -> None: +async def test_review_mutation_blocks_delivery_without_controller_repair_loop( + tmp_path: Path, +) -> None: workspace, commit = _workspace(tmp_path) async def review(context, checks): @@ -650,16 +655,35 @@ async def test_review_mutation_returns_to_repair_instead_of_destroying_run(tmp_p return result async def repair(context, checks): - if context.attempt > 1: - assert "deliverable changed" in " ".join(context.feedback[-2].repair.gaps) - (workspace / "review.tmp").unlink() + assert context.attempt == 1 return await _noop_repair(context, checks) result = await prepare_fix( _request(_candidate(commit)), workspace, repair=repair, verify=review ) - assert result.state is PreparationState.READY - assert result.attempts == 2 + assert result.state is PreparationState.BLOCKED + assert "deliverable changed" in result.stop_reason + assert result.attempts == 1 + assert result.final_file_manifest + + +@pytest.mark.asyncio +async def test_empty_deliverable_does_not_start_another_repair(tmp_path: Path) -> None: + workspace, commit = _workspace(tmp_path) + + async def repair(context, _checks): + assert context.attempt == 1 + return RepairOutcome(status=RepairStatus.COMPLETE, summary="Done.") + + async def review(*_args): + raise AssertionError("An empty artifact cannot be delivered") + + result = await prepare_fix( + _request(_candidate(commit)), workspace, repair=repair, verify=review + ) + assert result.state is PreparationState.BLOCKED + assert result.attempts == 1 + assert "without a deliverable patch" in result.stop_reason @pytest.mark.parametrize(