mirror of
https://github.com/usestrix/strix.git
synced 2026-10-01 02:03:55 +00:00
Let fix reviewer own validation and completion
This commit is contained in:
parent
43391ebefa
commit
d184142377
2 changed files with 44 additions and 49 deletions
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue