From bba4aa24dc529f969b02eecc768ef24aff64787e Mon Sep 17 00:00:00 2001 From: Jonathan Singer Date: Wed, 30 Sep 2026 23:21:24 -0400 Subject: [PATCH] fix: require reviewed current patches and enforce fix network isolation --- docs/fix-preparation.md | 10 ++-- strix/fix/prepare.py | 7 ++- strix/fix/runtime.py | 7 ++- strix/fix/scan.py | 3 + strix/interface/fix_cli.py | 12 +++- strix/runtime/backends.py | 2 + strix/runtime/docker_client.py | 11 +++- strix/runtime/session_manager.py | 20 ++++++- tests/test_fix_cli.py | 59 ++++++++++++++++++-- tests/test_fix_completion.py | 19 ++++--- tests/test_fix_network.py | 96 ++++++++++++++++++++++++++++++++ tests/test_fix_preparation.py | 50 +++++++++++++++-- tests/test_scan_fixes.py | 45 +++++++++++++++ 13 files changed, 311 insertions(+), 30 deletions(-) create mode 100644 tests/test_fix_network.py diff --git a/docs/fix-preparation.md b/docs/fix-preparation.md index fff812618..cf7182bcd 100644 --- a/docs/fix-preparation.md +++ b/docs/fix-preparation.md @@ -17,16 +17,18 @@ - `strix/fix/scan.py`: supplies the finding/worktree, deduplicates requests, preserves turn counts, exports completion, and cleans up. It replays persisted findings on resume. Terminal failures are reported truthfully; an explicit retry starts a fresh attempt using the remaining turn allowance. - `strix/runtime/agent_session.py`: borrows the scan sandbox with a worktree-specific filesystem root and process ownership. All scan agents get a process scope. Use `stop_process` or Ctrl-C on an owned tool session; broad shell kill commands are rejected. This prevents accidental interference, not hostile code escaping an OS security boundary. - `strix/agents/prompts/fix.jinja`: the single Fix assignment; shared workspace guidance is in `fix_workspace.jinja`. -- `strix/fix/runtime.py`: uses `build_strix_agent`, `run_agent_loop`, native tools, persisted sessions, and usage hooks. There is no separate reviewer or custom conversation loop. -- `strix/fix/prepare.py`: checks source identity and the completed patch, then exports successful artifacts. +- `strix/fix/runtime.py`: uses `build_strix_agent`, `run_agent_loop`, native tools, persisted sessions, and usage hooks for repair and independent review. +- `strix/fix/prepare.py`: checks source identity and the completed patch, then exports artifacts only after independent approval of the final source digest. Scan delivery also rechecks that the finding is still current. - Pro supplies progress/result callbacks. The app registers the inline attempt, stores successful artifacts privately, and creates draft PRs using its existing repository integration. Neither starts another fix sandbox. -The `single_agent` result contract exposes the agent's limitations in both `completion.gaps` and top-level `gaps` for compatible readers. It records the Fix agent's completion, command history, final file manifest, and source digest. It does not claim independent verification. Commands include diagnostic failures and superseded attempts; the agent's final summary explains which tests passed and any optional follow-ups. +The result exposes the agent's limitations in both `completion.gaps` and top-level `gaps` for compatible readers. Ready results use `agent_review` and record the repair completion, independent review, command history, final file manifest, and approved source digest. Commands include diagnostic failures and superseded attempts; the Markdown result includes both repair and reviewer summaries. ## Standalone OSS command `strix fix --finding findings.json --finding-id FINDING_ID --repo /path/to/repo` -The standalone command uses the same single-agent implementation in its own sandbox, because no live scan exists to borrow. It preserves the supplied checkout and writes private outputs outside the repository by default. Only successful runs export a patch and archive. `--max-agent-turns` and the legacy `--max-repair-turns` can lower the turn cap; they cannot raise it above 300. Old request fields are accepted for compatibility, but reviewer limits no longer control a second agent. +The standalone command uses the same repair and review workflow in its own sandbox, because no live scan exists to borrow. It preserves the supplied checkout and writes private outputs outside the repository by default. Only successful runs export a patch and archive. `--max-agent-turns` and the legacy `--max-repair-turns` can lower the repair turn cap; they cannot raise it above 300. + +Standalone fix requests default to `network_allowed=false`, enforced with Docker's `network_mode="none"`. Use `--allow-network` or explicitly set `network_allowed=true` in request JSON to use `STRIX_DOCKER_SANDBOX_NETWORK` (Docker's default network when unset). Offline sandboxes skip host-side proxy bootstrap and cannot reuse a session with a different network policy; unsupported backends reject offline requests. Fixes running inside a live scan inherit that scan's network policy. Uploaded archives and ambiguous multiple-repository findings cannot currently start an automatic worktree fix: the candidate must identify one Git source and its exact revision. diff --git a/strix/fix/prepare.py b/strix/fix/prepare.py index f6e361b78..cf4318b9c 100644 --- a/strix/fix/prepare.py +++ b/strix/fix/prepare.py @@ -290,7 +290,10 @@ async def prepare_fix( # noqa: PLR0911, PLR0912 if completion.source_digest != await workspace_digest(workspace): return await finish(PreparationState.BLOCKED, "Source changed after completion.") if verify is None: - return await finish(PreparationState.READY, completion.summary) + return await finish( + PreparationState.BLOCKED, + "Independent verification is required before delivery.", + ) before_review = await workspace_digest(workspace) attempt = FixPreparationAttempt( attempt=1, @@ -304,6 +307,8 @@ async def prepare_fix( # noqa: PLR0911, PLR0912 context.feedback.append(attempt) verifier = await verify(context, attempt.checks) attempt.verifier = verifier + if cancelled(): + raise PreparationCancelledError # noqa: TRY301 after_review = await workspace_digest(workspace) if after_review != before_review: return await finish( diff --git a/strix/fix/runtime.py b/strix/fix/runtime.py index 4899f27fb..d91365161 100644 --- a/strix/fix/runtime.py +++ b/strix/fix/runtime.py @@ -719,12 +719,15 @@ class ManagedIndependentVerifier(_FixAgent): async def _create_command_sandbox( sandbox_id: str, + *, + network_allowed: bool = False, ) -> BaseSandboxSession: settings = load_settings() bundle = await session_manager.create_or_reuse( sandbox_id, image=settings.runtime.image, local_sources=[], + network_allowed=network_allowed, ) return cast("BaseSandboxSession", bundle["session"]) @@ -933,7 +936,9 @@ async def run_isolated_fix_preparation( f"fix-preparation-{request.finding_id}-" f"{request.candidate.digest()[:12]}-{attempt_digest}" ) - sandbox_session = await _create_command_sandbox(sandbox_id) + sandbox_session = await _create_command_sandbox( + sandbox_id, network_allowed=request.network_allowed + ) environment = _RuntimeEnvironment( workspace=mirror, sandbox_session=sandbox_session, diff --git a/strix/fix/scan.py b/strix/fix/scan.py index 4a1278c27..91b15ea6c 100644 --- a/strix/fix/scan.py +++ b/strix/fix/scan.py @@ -329,6 +329,9 @@ class ScanFixes: prepared = await finish_native_fix( request, environment, hooks, result, session, artifact ) + if not self._current(finding_id, digest): + prepared = None + raise ValueError("The finding changed or was withdrawn during Fix.") # noqa: TRY301 prepared.elapsed_seconds = time.monotonic() - started_at delivered = await self._emit( "finished", diff --git a/strix/interface/fix_cli.py b/strix/interface/fix_cli.py index 81714fbc3..eff2f2d40 100644 --- a/strix/interface/fix_cli.py +++ b/strix/interface/fix_cli.py @@ -49,6 +49,12 @@ def _parser() -> argparse.ArgumentParser: parser.add_argument("--max-review-turns", type=int, help=argparse.SUPPRESS) parser.add_argument("--max-budget", type=float, help="Combined LLM cost budget in USD.") parser.add_argument("--timeout", type=int, help="Whole-job timeout in seconds.") + parser.add_argument( + "--allow-network", + action="store_true", + default=None, + help="Allow sandbox network access using the operator-configured Docker network.", + ) return parser @@ -80,7 +86,6 @@ def _load_request(args: argparse.Namespace) -> FixPreparationRequestV1: scan_id=str(finding.get("scan_id") or "local"), finding_id=str(finding.get("id") or args.finding_id or uuid.uuid4().hex), candidate=candidate, - network_allowed=True, ) if ( request.candidate.source_identity is None @@ -95,6 +100,7 @@ def _load_request(args: argparse.Namespace) -> FixPreparationRequestV1: "max_review_turns": args.max_review_turns, "max_budget_usd": args.max_budget, "timeout_seconds": args.timeout, + "network_allowed": args.allow_network, }.items() if value is not None } @@ -116,10 +122,10 @@ def _summary(result: FixPreparationResultV1) -> str: lines = ["# Fix preparation", "", f"Status: {result.state.value}", "", result.stop_reason] if result.completion: lines.extend(["", "## Fix", "", result.completion.summary]) - elif result.verifier: - lines.extend(["", "## Review", "", result.verifier.summary]) elif result.attempt_history: lines.extend(["", "## Repair", "", result.attempt_history[-1].repair.summary]) + if result.verifier: + lines.extend(["", "## Review", "", result.verifier.summary]) lines.extend(["", "## Recorded commands", "", "Includes diagnostic and superseded attempts."]) lines.extend( f"- {check.name}: {check.status.value}; exit code {check.exit_code}." diff --git a/strix/runtime/backends.py b/strix/runtime/backends.py index ec49f7a7d..ce19ca6a8 100644 --- a/strix/runtime/backends.py +++ b/strix/runtime/backends.py @@ -23,6 +23,7 @@ async def _docker_backend( manifest: Manifest, exposed_ports: tuple[int, ...], bind_mounts: list[dict[str, Any]] | None = None, + network_allowed: bool = True, ) -> tuple[Any, Any]: """Bring up a session backed by the local Docker daemon. @@ -44,6 +45,7 @@ async def _docker_backend( client = StrixDockerSandboxClient(docker.from_env()) client.strix_bind_mounts = bind_mounts or [] + client.network_allowed = network_allowed options = DockerSandboxClientOptions(image=image, exposed_ports=exposed_ports) session = await client.create(options=options, manifest=manifest) await session.start() diff --git a/strix/runtime/docker_client.py b/strix/runtime/docker_client.py index 969267d53..09afa8e5c 100644 --- a/strix/runtime/docker_client.py +++ b/strix/runtime/docker_client.py @@ -59,7 +59,11 @@ def _sandbox_network() -> str | None: return value or None -def _apply_sandbox_network(create_kwargs: dict[str, Any]) -> None: +def _apply_sandbox_network(create_kwargs: dict[str, Any], *, network_allowed: bool = True) -> None: + if not network_allowed: + create_kwargs["network_mode"] = "none" + create_kwargs.pop("ports", None) + return network = _sandbox_network() if network: create_kwargs["network"] = network @@ -162,6 +166,7 @@ class StrixDockerSandboxSession(DockerSandboxSession): class StrixDockerSandboxClient(DockerSandboxClient): + network_allowed: bool = True # Host directories to bind-mount into the container, set by the docker # backend before ``create()``. Each item is ``{source, target, read_only}``. strix_bind_mounts: list[dict[str, Any]] | None = None @@ -232,7 +237,7 @@ class StrixDockerSandboxClient(DockerSandboxClient): extra_hosts = create_kwargs.setdefault("extra_hosts", {}) extra_hosts["host.docker.internal"] = "host-gateway" - _apply_sandbox_network(create_kwargs) + _apply_sandbox_network(create_kwargs, network_allowed=self.network_allowed) _apply_resource_limits(create_kwargs) _apply_log_limits(create_kwargs) _apply_run_labels(create_kwargs) @@ -268,7 +273,7 @@ class StrixDockerSandboxClient(DockerSandboxClient): async def create(self, **kwargs: Any) -> SandboxSession: session = await super().create(**kwargs) - network = _sandbox_network() + network = _sandbox_network() if self.network_allowed else None inner = session._inner if network and isinstance(inner, DockerSandboxSession): inner.__class__ = StrixDockerSandboxSession diff --git a/strix/runtime/session_manager.py b/strix/runtime/session_manager.py index a8447ebbe..b10a46649 100644 --- a/strix/runtime/session_manager.py +++ b/strix/runtime/session_manager.py @@ -262,6 +262,7 @@ async def create_or_reuse( local_sources: list[dict[str, Any]], extra_files: list[dict[str, Any]] | None = None, status_sink: StatusSink | None = None, + network_allowed: bool = True, ) -> dict[str, Any]: """Return the existing session bundle for ``scan_id`` or create a new one. @@ -280,11 +281,15 @@ async def create_or_reuse( cached = _SESSION_CACHE.get(scan_id) if cached is not None: + if cached.get("network_allowed", True) != network_allowed: + raise ValueError("Cannot reuse a sandbox with a different network policy.") logger.info("Reusing existing sandbox session for scan %s", scan_id) return cached backend_name = load_settings().runtime.backend backend = get_backend(backend_name) + if not network_allowed and backend_name != "docker": + raise ValueError("Network isolation is only supported by the Docker backend.") if backend_supports_bind_mounts(backend_name): bind_mounts = build_bind_mounts(local_sources) @@ -327,8 +332,9 @@ async def create_or_reuse( client, session = await backend( image=image, manifest=manifest, - exposed_ports=(_CONTAINER_CAIDO_PORT,), + exposed_ports=(_CONTAINER_CAIDO_PORT,) if network_allowed else (), bind_mounts=bind_mounts, + **({"network_allowed": False} if not network_allowed else {}), ) if extra_file_archive is not None: @@ -339,6 +345,17 @@ async def create_or_reuse( await _discard_session(client, session) raise + if not network_allowed: + # Offline command sandboxes have no reachable proxy endpoint to bootstrap. + bundle = { + "client": client, + "session": session, + "caido_client": None, + "network_allowed": False, + } + _SESSION_CACHE[scan_id] = bundle + return bundle + report("Setting up the proxy") caido_endpoint = await session.resolve_exposed_port(_CONTAINER_CAIDO_PORT) scheme = "https" if caido_endpoint.tls else "http" @@ -364,6 +381,7 @@ async def create_or_reuse( "client": client, "session": session, "caido_client": caido_client, + "network_allowed": True, } _SESSION_CACHE[scan_id] = bundle logger.info("Sandbox session for scan %s ready and cached", scan_id) diff --git a/tests/test_fix_cli.py b/tests/test_fix_cli.py index eb0c513bb..2d6786bdd 100644 --- a/tests/test_fix_cli.py +++ b/tests/test_fix_cli.py @@ -54,11 +54,48 @@ def test_cli_role_budget_overrides(tmp_path: Path) -> None: assert (loaded.repair_turn_limit, loaded.review_turn_limit) == (300, 250) -def _local_runtime(monkeypatch: pytest.MonkeyPatch, tmp_path: Path, model: ScriptedModel) -> None: +@pytest.mark.parametrize("allow_network", [False, True]) +@pytest.mark.parametrize("input_kind", ["finding", "request"]) +def test_cli_network_access_requires_opt_in( + tmp_path: Path, allow_network: bool, input_kind: str +) -> None: + request = _request("a" * 40) + assert request.network_allowed is False + path = tmp_path / "input.json" + path.write_text( + request.model_dump_json() + if input_kind == "request" + else json.dumps({"fix_candidate": request.candidate.model_dump(mode="json")}) + ) + argv = [f"--{input_kind}", str(path), "--repo", str(tmp_path)] + if allow_network: + argv.append("--allow-network") + assert ( + fix_cli._load_request(fix_cli._parser().parse_args(argv)).network_allowed is allow_network + ) + + +def test_cli_preserves_explicit_request_network_policy(tmp_path: Path) -> None: + request = _request("a" * 40) + request.network_allowed = True + path = tmp_path / "request.json" + path.write_text(request.model_dump_json()) + args = fix_cli._parser().parse_args(["--request", str(path), "--repo", str(tmp_path)]) + assert fix_cli._load_request(args).network_allowed is True + + +def _local_runtime( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + model: ScriptedModel, + *, + allow_network: bool = False, +) -> None: root = tmp_path / "execution" / "source" original_environment = fix_runtime._RuntimeEnvironment - async def sandbox(_sandbox_id: str) -> LocalSandbox: + async def sandbox(_sandbox_id: str, *, network_allowed: bool) -> LocalSandbox: + assert network_allowed is allow_network return LocalSandbox(root.parent) async def noop(*_args: Any) -> None: @@ -82,8 +119,9 @@ def _local_runtime(monkeypatch: pytest.MonkeyPatch, tmp_path: Path, model: Scrip @pytest.mark.parametrize("blocked", [False, True]) +@pytest.mark.parametrize("allow_network", [False, True]) def test_cli_runs_shared_workflow_and_preserves_original_checkout( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch, blocked: bool + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, blocked: bool, allow_network: bool ) -> None: workspace, _ = _workspace(tmp_path) commit = existing_suite(workspace) @@ -94,12 +132,13 @@ def test_cli_runs_shared_workflow_and_preserves_original_checkout( review = ( [shell("exit 1"), finish("blocked", "Required tests need a customer database.")] if blocked - else [*suite_commands(), finish("approved", "Existing and regression tests passed.")] + else [*suite_commands(), finish("done", "Existing and regression tests passed.")] ) model = ScriptedModel( - [*patch(), *(review if blocked else [*suite_commands(), finish("done", "tests passed")])] + [*patch(), *(review if blocked else [*suite_commands(), finish("done", "tests passed")])], + review=[] if blocked else review, ) - _local_runtime(monkeypatch, tmp_path, model) + _local_runtime(monkeypatch, tmp_path, model, allow_network=allow_network) output = tmp_path / "result.json" code = fix_cli.run_fix( @@ -112,6 +151,7 @@ def test_cli_runs_shared_workflow_and_preserves_original_checkout( str(workspace), "--output", str(output), + *(["--allow-network"] if allow_network else []), ] ) @@ -124,6 +164,10 @@ def test_cli_runs_shared_workflow_and_preserves_original_checkout( ".md" ).read_text() if not blocked: + assert ( + "## Review\n\nExisting and regression tests passed." + in output.with_suffix(".md").read_text() + ) with zipfile.ZipFile(output.with_suffix(".zip")) as artifact: assert "files/tests/test_security.py" in artifact.namelist() assert "tool-results.jsonl" in artifact.namelist() @@ -320,6 +364,7 @@ def test_summary_includes_followups_from_repair_and_reviewer() -> None: source_identity=request.candidate.source_identity, candidate=request.candidate, candidate_digest=request.candidate.digest(), + completion=attempt.repair, attempt_history=[attempt], verifier=VerifierResult( decision=VerificationDecision.VERIFIED, @@ -328,6 +373,8 @@ def test_summary_includes_followups_from_repair_and_reviewer() -> None: ), ) summary = fix_cli._summary(result) + assert "## Fix\n\nPatched." in summary + assert "## Review\n\nApproved." in summary assert "re-run the nightly suite" in summary assert "rotate the leaked token" in summary diff --git a/tests/test_fix_completion.py b/tests/test_fix_completion.py index b585e272b..07fefee51 100644 --- a/tests/test_fix_completion.py +++ b/tests/test_fix_completion.py @@ -74,12 +74,15 @@ def suite_commands() -> list[Any]: class ScriptedModel(Model): def __init__(self, repair: list[Any], review: list[Any] | None = None) -> None: - self.responses = {"repair": repair, "review": review or []} + self.responses = { + "repair": repair, + "review": review if review is not None else [finish("done", "Independently approved.")], + } self.inputs: dict[str, list[Any]] = {"repair": [], "review": []} self.tools: set[str] = set() async def get_response(self, **kwargs: Any) -> ModelResponse: - role = "review" if "Independently review" in kwargs["system_instructions"] else "repair" + role = "review" if "Independently verify" in kwargs["system_instructions"] else "repair" self.inputs[role].append(list(kwargs["input"])) self.tools.update(t.name for t in kwargs["tools"]) assert self.responses[role], f"Unexpected additional {role} turn" @@ -164,13 +167,15 @@ async def scenario( @pytest.mark.asyncio -async def test_single_agent_implements_runs_both_test_suites_and_exports(tmp_path, monkeypatch): +async def test_agent_implements_runs_both_test_suites_and_exports_after_review( + tmp_path, monkeypatch +): model = ScriptedModel([*patch(), *suite_commands(), finish("done", "Both suites passed")]) result, _env = await scenario(tmp_path, monkeypatch, model) assert result.state is PreparationState.READY, result.model_dump_json() - assert result.validation_mode == "single_agent" - assert not model.inputs["review"] - assert result.verifier is None + assert result.validation_mode == "agent_review" + assert model.inputs["review"] + assert result.verifier.summary == "Independently approved." assert result.completion.turns_used == 5 assert all("Ran 1 test" in c.output for c in result.checks[-2:]) assert {"exec_command", "apply_patch", "agent_finish"} <= model.tools @@ -189,7 +194,7 @@ async def test_agent_corrects_failed_test_in_same_conversation(tmp_path, monkeyp assert result.state is PreparationState.READY assert any(c.exit_code != 0 for c in result.checks) assert all(c.exit_code == 0 for c in result.checks[-2:]) - assert not model.inputs["review"] + assert model.inputs["review"] @pytest.mark.asyncio diff --git a/tests/test_fix_network.py b/tests/test_fix_network.py new file mode 100644 index 000000000..8220d9914 --- /dev/null +++ b/tests/test_fix_network.py @@ -0,0 +1,96 @@ +"""Fix requests enforce network policy at container creation and session reuse.""" + +from __future__ import annotations + +from types import SimpleNamespace +from typing import TYPE_CHECKING, Any +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from strix.fix import runtime as fix_runtime +from strix.runtime import session_manager +from strix.runtime.docker_client import StrixDockerSandboxClient + + +if TYPE_CHECKING: + from agents.sandbox.manifest import Manifest + + +@pytest.mark.parametrize("network_allowed", [False, True]) +@pytest.mark.parametrize("operator_network", ["", "operator-network"]) +async def test_fix_network_policy_reaches_docker( + monkeypatch: pytest.MonkeyPatch, network_allowed: bool, operator_network: str +) -> None: + docker = MagicMock() + session = SimpleNamespace( + start=AsyncMock(), + resolve_exposed_port=AsyncMock( + return_value=SimpleNamespace(tls=False, host="localhost", port=48080) + ), + ) + settings = SimpleNamespace(runtime=SimpleNamespace(backend="docker", image="test-image")) + monkeypatch.setattr(fix_runtime, "load_settings", lambda: settings) + monkeypatch.setattr(session_manager, "load_settings", lambda: settings) + monkeypatch.setattr(session_manager, "_SESSION_CACHE", {}) + bootstrap = AsyncMock() + monkeypatch.setattr(session_manager, "bootstrap_caido", bootstrap) + monkeypatch.setenv("STRIX_DOCKER_SANDBOX_NETWORK", operator_network) + monkeypatch.setattr("docker.from_env", lambda: docker) + monkeypatch.setattr(StrixDockerSandboxClient, "image_exists", lambda *_args: True) + + async def create(self: StrixDockerSandboxClient, *, options: Any, manifest: Manifest) -> Any: + await self._create_container( + options.image, manifest=manifest, exposed_ports=options.exposed_ports + ) + return session + + monkeypatch.setattr(StrixDockerSandboxClient, "create", create) + result = await fix_runtime._create_command_sandbox("fix", network_allowed=network_allowed) + assert result is session + session.start.assert_awaited_once() + kwargs = docker.containers.create.call_args.kwargs + if not network_allowed: + assert kwargs["network_mode"] == "none" + assert "network" not in kwargs + assert "ports" not in kwargs + session.resolve_exposed_port.assert_not_awaited() + bootstrap.assert_not_awaited() + else: + assert "network_mode" not in kwargs + if operator_network: + assert kwargs["network"] == operator_network + assert "ports" not in kwargs + else: + assert "network" not in kwargs + assert kwargs["ports"] == {"48080/tcp": ("127.0.0.1", None)} + await session_manager._SESSION_CACHE["fix"]["caido_client"].aclose() + + assert ( + await fix_runtime._create_command_sandbox("fix", network_allowed=network_allowed) is session + ) + with pytest.raises(ValueError, match="different network policy"): + await fix_runtime._create_command_sandbox("fix", network_allowed=not network_allowed) + docker.containers.create.assert_called_once() + + +async def test_fix_sandbox_defaults_to_no_network(monkeypatch: pytest.MonkeyPatch) -> None: + create = AsyncMock(return_value={"session": object()}) + monkeypatch.setattr(session_manager, "create_or_reuse", create) + await fix_runtime._create_command_sandbox("fix") + assert create.call_args.kwargs["network_allowed"] is False + + +async def test_network_isolation_rejects_unsupported_backend( + monkeypatch: pytest.MonkeyPatch, +) -> None: + settings = SimpleNamespace(runtime=SimpleNamespace(backend="custom", image="test-image")) + backend = AsyncMock() + monkeypatch.setattr(session_manager, "_SESSION_CACHE", {}) + monkeypatch.setattr(session_manager, "load_settings", lambda: settings) + monkeypatch.setattr(session_manager, "get_backend", lambda _name: backend) + with pytest.raises(ValueError, match="only supported by the Docker backend"): + await session_manager.create_or_reuse( + "fix", image="test-image", local_sources=[], network_allowed=False + ) + backend.assert_not_awaited() diff --git a/tests/test_fix_preparation.py b/tests/test_fix_preparation.py index 41452ad58..4994f276c 100644 --- a/tests/test_fix_preparation.py +++ b/tests/test_fix_preparation.py @@ -368,10 +368,17 @@ def test_candidate_keeps_full_finding_without_inventing_reproduction() -> None: @pytest.mark.asyncio -async def test_agent_tests_are_reused_without_controller_execution(tmp_path): +async def test_unreviewed_completion_never_exports_a_patch(tmp_path): workspace, commit = _workspace(tmp_path) - result = await prepare_fix(_request(_candidate(commit)), workspace, repair=_noop_repair) - assert result.state is PreparationState.READY + export = AsyncMock() + result = await prepare_fix( + _request(_candidate(commit)), workspace, repair=_noop_repair, manifest_builder=export + ) + assert result.state is PreparationState.BLOCKED + assert "Independent verification is required" in result.stop_reason + assert not result.final_file_manifest + assert result.artifact_ref is None + export.assert_not_awaited() assert result.validation_mode == "single_agent" assert result.completion.status == RepairStatus.COMPLETE assert result.verifier is None @@ -379,6 +386,34 @@ async def test_agent_tests_are_reused_without_controller_execution(tmp_path): assert "Ran 1 test" in result.checks[0].output +@pytest.mark.asyncio +async def test_cancellation_during_review_never_exports_a_patch(tmp_path): + workspace, commit = _workspace(tmp_path) + stopped = False + + async def verify(context, _checks): + nonlocal stopped + stopped = True + return VerifierResult( + decision=VerificationDecision.VERIFIED, + summary="Approved.", + source_digest=await workspace_digest(context.workspace), + ) + + export = AsyncMock() + result = await prepare_fix( + _request(_candidate(commit)), + workspace, + repair=_noop_repair, + verify=verify, + cancelled=lambda: stopped, + manifest_builder=export, + ) + assert result.state is PreparationState.FAILED + assert "cancelled" in result.stop_reason + export.assert_not_awaited() + + @pytest.mark.asyncio async def test_ready_requires_independent_verifier_approval(tmp_path): workspace, commit = _workspace(tmp_path) @@ -514,6 +549,13 @@ async def test_completion_limitations_are_preserved_at_top_level(tmp_path): completion = await _noop_repair(context, checks) return completion.model_copy(update={"gaps": ["External integration was not exercised."]}) - result = await prepare_fix(_request(_candidate(commit)), workspace, repair=agent) + async def verify(context, _checks): + return VerifierResult( + decision=VerificationDecision.VERIFIED, + summary="Approved with the recorded limitation.", + source_digest=await workspace_digest(context.workspace), + ) + + result = await prepare_fix(_request(_candidate(commit)), workspace, repair=agent, verify=verify) assert result.state is PreparationState.READY assert result.gaps == result.completion.gaps == ["External integration was not exercised."] diff --git a/tests/test_scan_fixes.py b/tests/test_scan_fixes.py index 2a8cb0006..d2bdd1c76 100644 --- a/tests/test_scan_fixes.py +++ b/tests/test_scan_fixes.py @@ -188,6 +188,51 @@ async def test_finding_revision_invalidates_active_completion(tmp_path): assert not fixes._current("finding", digest) +@pytest.mark.parametrize("change", ["revised", "withdrawn", "unconfirmed"]) +async def test_finding_changed_before_delivery_discards_reviewed_patch( + tmp_path, monkeypatch, change +): + fixes, report, _, _, reports, context, _ = setup(tmp_path) + callback = None + + async def spawn(**kwargs): + nonlocal callback + callback = kwargs["on_complete"] + fixes.coordinator.runtimes["fix"] = SimpleNamespace( + task=asyncio.create_task(asyncio.sleep(0)) + ) + return {"success": True, "agent_id": "fix"} + + async def finish_preparation(request, _env, _hooks, _result, _session, artifact): + artifact.write_bytes(b"reviewed patch") + if change == "revised": + report["fix_candidate"]["security_invariant"] = "Revised attack" + elif change == "withdrawn": + reports.clear() + else: + report["validation_status"] = "unconfirmed" + return scan_module.FixPreparationResultV1( + state="ready", + stop_reason="Approved.", + source_identity=request.candidate.source_identity, + candidate=request.candidate, + candidate_digest=request.candidate.digest(), + artifact_ref=str(artifact), + ) + + monkeypatch.setattr(scan_module, "finish_native_fix", finish_preparation) + fixes.sink = AsyncMock(return_value=True) + await fixes.spawn( + "finding", spawn, parent_ctx=context.context, name="Fix", task="Repair", skills=[] + ) + with pytest.raises(ValueError, match="changed or was withdrawn"): + await callback(None, None) + assert fixes.records["finding"]["status"] == "stopped" + assert "artifact" not in fixes.records["finding"] + assert not list((tmp_path / "state/fixes").glob("*/prepared-fix.zip")) + assert all(call.args[2:] == (None, None) for call in fixes.sink.await_args_list) + + @pytest.mark.asyncio async def test_worktree_process_cleanup_never_terminates_parent_sessions(tmp_path): parent = LocalSandbox(tmp_path)