diff --git a/strix/fix/__init__.py b/strix/fix/__init__.py index 44310b235..59c4c7e64 100644 --- a/strix/fix/__init__.py +++ b/strix/fix/__init__.py @@ -20,6 +20,7 @@ from strix.fix.contracts import ( RepairStatus, RepositoryTestPlan, ReproductionSpec, + ReviewConcern, SourceIdentity, SourceIdentityKind, VerificationDecision, @@ -61,6 +62,7 @@ __all__ = [ "RepairStatus", "RepositoryTestPlan", "ReproductionSpec", + "ReviewConcern", "SourceIdentity", "SourceIdentityKind", "VerificationDecision", diff --git a/strix/fix/contracts.py b/strix/fix/contracts.py index df7c796ab..bcef944fb 100644 --- a/strix/fix/contracts.py +++ b/strix/fix/contracts.py @@ -291,6 +291,11 @@ class VerificationHarness(ContractModel): content: str +class ReviewConcern(ContractModel): + kind: Literal["repair_needed", "customer_prerequisite", "optional_follow_up"] + summary: str = Field(min_length=1) + + class VerifierResult(ContractModel): decision: VerificationDecision summary: str @@ -309,6 +314,8 @@ class VerifierResult(ContractModel): review_basis: Literal["execution", "code_review"] | None = None regression_test_valid: bool = False unit_test_coverage_valid: bool = False + # None preserves historical reviews; new reviewers classify every concern. + concerns: list[ReviewConcern] | None = None class RepairOutcome(ContractModel): diff --git a/strix/fix/prepare.py b/strix/fix/prepare.py index da505547c..6e94970fd 100644 --- a/strix/fix/prepare.py +++ b/strix/fix/prepare.py @@ -421,6 +421,10 @@ def _verification_passes(checks: list[CheckResult], verifier: VerifierResult) -> and any( item.purpose == "regression" and item.status is CheckStatus.PASSED for item in checks ) + and any( + item.purpose == "unit" and item.required and item.status is CheckStatus.PASSED + for item in checks + ) and len({item.environment_id for item in checks if item.environment_id}) <= 1 and len({item.source_digest for item in checks if item.source_digest}) <= 1 and verifier.decision is VerificationDecision.VERIFIED @@ -439,7 +443,49 @@ def _test_plan_gaps(repair: RepairOutcome, manifest: list[FileManifestEntry]) -> return ["The repair must supply a regression test and the repository unit-test commands."] changed = {entry.path for entry in manifest if entry.operation != "delete"} missing = [path for path in plan.regression_files if path not in changed] - return [f"Regression test must be added or updated in the patch: {path}" for path in missing] + gaps = [f"Regression test must be added or updated in the patch: {path}" for path in missing] + if not plan.unit_tests: + gaps.append("Supply the customer's existing unit-test suite for the affected code.") + if plan.no_unit_tests_reason: + gaps.append(plan.no_unit_tests_reason) + return gaps + + +def _resolve_review(verifier: VerifierResult) -> VerifierResult: + """Derive the outcome from actionable concerns, retaining legacy record support.""" + if verifier.concerns is None: + return verifier + repairs = [c.summary for c in verifier.concerns if c.kind == "repair_needed"] + prerequisites = [c.summary for c in verifier.concerns if c.kind == "customer_prerequisite"] + notes = [c.summary for c in verifier.concerns if c.kind == "optional_follow_up"] + approved = ( + verifier.security_invariant_closed + and verifier.regression_test_valid + and verifier.unit_test_coverage_valid + ) + decision = ( + VerificationDecision.REJECTED + if repairs + else VerificationDecision.INCONCLUSIVE + if prerequisites or not approved + else VerificationDecision.VERIFIED + ) + return verifier.model_copy( + update={ + "decision": decision, + "repairable": bool(repairs), + "gaps": [*repairs, *prerequisites], + "notes": list(dict.fromkeys([*verifier.notes, *notes])), + "blocker": PreparationBlocker( + kind=BlockerKind.EXTERNAL_CONFIGURATION, + summary="A required customer prerequisite is missing.", + user_action="\n".join(prerequisites), + details=prerequisites, + ) + if prerequisites + else None, + } + ) def _test_commands(repair: RepairOutcome) -> list[CommandSpec]: @@ -562,6 +608,10 @@ async def prepare_fix( # noqa: PLR0915 checks = record.checks record.workspace_digest = await _workspace_digest(workspace) manifest, _, _ = await build_git_manifest(workspace) + can_retry = ( + attempt < resolved_policy.max_repair_attempts + and record.repair.status is not RepairStatus.BLOCKED + ) if not manifest: if record.repair.blocker: return await finish( @@ -570,14 +620,18 @@ async def prepare_fix( # noqa: PLR0915 blocker=record.repair.blocker, gaps=record.repair.gaps, ) + record.repair.gaps.append( + "No changed files were found. Use the file tools to implement the fix and " + "regression test, then submit their validation commands." + ) + if can_retry: + continue return await finish( - PreparationState.FAILED, "The repair did not change repository source." + PreparationState.FAILED, + "The repair did not change repository source.", + gaps=record.repair.gaps, ) await manifest_builder(workspace) # Save the patch before any test can fail. - can_retry = ( - attempt < resolved_policy.max_repair_attempts - and record.repair.status is not RepairStatus.BLOCKED - ) plan_gaps = _test_plan_gaps(record.repair, manifest) if plan_gaps: record.repair.gaps = list(dict.fromkeys([*record.repair.gaps, *plan_gaps])) @@ -585,7 +639,7 @@ async def prepare_fix( # noqa: PLR0915 continue return await finish( PreparationState.BLOCKED, - "The patch is saved, but its regression-test handoff is incomplete.", + "The patch is saved, but its required test handoff is incomplete.", blocker=record.repair.blocker, gaps=record.repair.gaps, ) @@ -625,7 +679,7 @@ async def prepare_fix( # noqa: PLR0915 gaps=[f"{item.name}: {item.status}" for item in failed], ) # Review the exact repair and its native test results. No second harness is required. - verifier = await verify(context, checks) + verifier = _resolve_review(await verify(context, checks)) record.verifier = verifier if record.workspace_digest != await _workspace_digest(workspace): return await finish( @@ -640,6 +694,13 @@ async def prepare_fix( # noqa: PLR0915 "The independent reviewer found a repair or regression-test defect.", gaps=verifier.gaps, ) + if verifier.blocker: + return await finish( + PreparationState.BLOCKED, + verifier.blocker.summary, + blocker=verifier.blocker, + gaps=verifier.gaps, + ) if record.repair.status is RepairStatus.BLOCKED: return await finish( PreparationState.BLOCKED, @@ -647,12 +708,15 @@ async def prepare_fix( # noqa: PLR0915 blocker=record.repair.blocker, gaps=record.repair.gaps, ) - if _verification_passes(checks, verifier) and not record.repair.gaps: + # Typed review explicitly resolves the repair agent's observations. Historical + # free-text gaps remain blocking until a reviewer classifies them. + repair_gaps = record.repair.gaps if verifier.concerns is None else [] + if _verification_passes(checks, verifier) and not repair_gaps: return await finish( PreparationState.READY, "Tests and required checks passed; independent review approved a draft PR.", ) - gaps = list(dict.fromkeys([*record.repair.gaps, *verifier.gaps])) + gaps = list(dict.fromkeys([*repair_gaps, *verifier.gaps])) if not gaps: gaps = ["Independent review could not confirm the repair and native-test coverage."] return await finish( diff --git a/tests/test_fix_preparation.py b/tests/test_fix_preparation.py index 0985dfe34..a0bb10cb8 100644 --- a/tests/test_fix_preparation.py +++ b/tests/test_fix_preparation.py @@ -25,6 +25,7 @@ from strix.fix.contracts import ( RepairStatus, RepositoryTestPlan, ReproductionSpec, + ReviewConcern, SourceIdentity, SourceIdentityKind, VerificationDecision, @@ -93,7 +94,11 @@ def _workspace(tmp_path: Path) -> tuple[Path, str]: _git(workspace, "config", "user.email", "test@example.com") _git(workspace, "config", "user.name", "Test") (workspace / "app.py").write_text("def result():\n return 'unsafe'\n", encoding="utf-8") - _git(workspace, "add", "app.py") + (workspace / "test_existing.py").write_text( + "import unittest\nfrom app import result\nclass Existing(unittest.TestCase):\n" + " def test_result_type(self): self.assertIsInstance(result(), str)\n" + ) + _git(workspace, "add", "app.py", "test_existing.py") _git(workspace, "commit", "-m", "initial") return workspace, _git(workspace, "rev-parse", "HEAD") @@ -179,7 +184,11 @@ async def _noop_repair( regression_test=CommandSpec( name="regression", argv=[sys.executable, "-m", "unittest", "test_app"] ), - no_unit_tests_reason="The fixture has no prior unit suite.", + unit_tests=[ + CommandSpec( + name="existing suite", argv=[sys.executable, "-m", "unittest", "test_existing"] + ) + ], ), status=RepairStatus.COMPLETE, summary="The repository fix is ready for independent evaluation.", @@ -586,6 +595,90 @@ async def test_exhausted_repair_can_be_validated_from_saved_work(tmp_path: Path) assert result.state is PreparationState.READY +@pytest.mark.asyncio +@pytest.mark.parametrize("recover", [True, False]) +async def test_empty_completion_gets_actionable_bounded_retry( + tmp_path: Path, recover: bool +) -> None: + workspace, commit = _workspace(tmp_path) + + async def repair(context, checks): + if context.attempt == 2: + assert "No changed files" in context.feedback[0].repair.gaps[0] + if recover: + return await _noop_repair(context, checks) + return RepairOutcome(status=RepairStatus.COMPLETE, summary="Claimed completion") + + result = await prepare_fix( + _request(_candidate(commit)), workspace, repair=repair, verify=_verified + ) + assert result.attempts == 2 + assert result.state is (PreparationState.READY if recover else PreparationState.FAILED) + if not recover: + assert not result.final_file_manifest + assert "No changed files" in result.gaps[0] + + +@pytest.mark.asyncio +async def test_missing_customer_suite_cannot_be_waived_with_an_explanation(tmp_path: Path) -> None: + workspace, commit = _workspace(tmp_path) + + async def repair(context, checks): + outcome = await _noop_repair(context, checks) + outcome.test_plan.unit_tests = [] + outcome.test_plan.no_unit_tests_reason = "No existing suite was found." + return outcome + + async def review(*_args): + pytest.fail("A reviewer cannot waive required customer unit tests") + + result = await prepare_fix( + _request(_candidate(commit)), workspace, repair=repair, verify=review + ) + assert result.state is PreparationState.BLOCKED + assert result.final_file_manifest + assert any("existing unit-test suite" in gap for gap in result.gaps) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("kind", ["repair_needed", "customer_prerequisite", "optional_follow_up"]) +async def test_controller_derives_review_action_and_resolves_repair_observations( + tmp_path: Path, kind: str +) -> None: + workspace, commit = _workspace(tmp_path) + + async def repair(context, checks): + outcome = await _noop_repair(context, checks) + outcome.gaps = ["An additional deployment check could help."] + if context.attempt == 2: + assert context.feedback[0].verifier.gaps == ["Inspect the companion configuration."] + return outcome + + async def review(context, checks): + result = await _verified(context, checks) + # The controller must derive the outcome even if the proposed decision says verified. + result.concerns = ( + [] + if context.attempt == 2 + else [ReviewConcern(kind=kind, summary="Inspect the companion configuration.")] + ) + return result + + result = await prepare_fix( + _request(_candidate(commit)), workspace, repair=repair, verify=review + ) + assert result.attempts == (2 if kind == "repair_needed" else 1) + if kind == "customer_prerequisite": + assert result.state is PreparationState.BLOCKED + assert result.blocker.user_action == "Inspect the companion configuration." + else: + assert result.state is PreparationState.READY + assert not result.gaps + assert not result.verifier.gaps + if kind == "optional_follow_up": + assert result.verifier.notes == ["Inspect the companion configuration."] + + @pytest.mark.asyncio @pytest.mark.parametrize( "defect",