diff --git a/.superpowers/sdd/python-workflow-client/task-6-report.md b/.superpowers/sdd/python-workflow-client/task-6-report.md index 64e94999..68353634 100644 --- a/.superpowers/sdd/python-workflow-client/task-6-report.md +++ b/.superpowers/sdd/python-workflow-client/task-6-report.md @@ -38,3 +38,11 @@ references are converted to `InvalidResponse` with operation context, and run decoding accepts truthful inspect/start/resume operation names. Fixes committed in the follow-up review commit for this report. + +## Review fix round 2 + +The sole discovered deployment path now rechecks the inspected artifact +identity, deployment creation acknowledgements and inspected ids are checked +before validation, and run-start responses enforce deployment plus artifact +identity. Missing-run errors preserve server text, including operation-aware +start interrupt decoding. diff --git a/src/wf_client/deployments.py b/src/wf_client/deployments.py index 1383d04c..7388b262 100644 --- a/src/wf_client/deployments.py +++ b/src/wf_client/deployments.py @@ -120,6 +120,18 @@ class Deployment: f"match requested {self.deployment_id!r}" ), ) + if ( + decoded.artifact_id != self.artifact_id + or decoded.artifact_version != self.artifact_version + ): + raise InvalidResponse( + operation="workflow.runs.start", + details=( + f"start result for {self.deployment_id!r} targets artifact " + f"{decoded.artifact_id!r} version {decoded.artifact_version}, " + f"expected {self.artifact_id!r} version {self.artifact_version}" + ), + ) if decoded.run_id is None or decoded.status in {"unrunnable", "rejected"}: raise DeploymentNotRunnable( deployment_id=self.deployment_id, @@ -127,7 +139,11 @@ class Deployment: outcome=decoded.outcome, error=decoded.error, ) - return _run_from_decoded(self._port, decoded) + return _run_from_decoded( + self._port, + decoded, + operation="workflow.runs.start", + ) def _decode_summaries(payload: object) -> list[dict[str, Any]]: @@ -222,4 +238,15 @@ async def run_artifact( f"match requested {matches[0]['id']!r}" ), ) + if ( + deployment.artifact_id != artifact.artifact.id + or deployment.artifact_version != artifact.artifact.version + ): + raise InvalidResponse( + operation="workflow.deployments.inspect", + details=( + f"deployment {matches[0]['id']!r} does not target artifact " + f"{artifact.artifact.id!r} version {artifact.artifact.version}" + ), + ) return await deployment.run(workflow_input) diff --git a/src/wf_client/runs.py b/src/wf_client/runs.py index f1da7b7b..4b6f87b6 100644 --- a/src/wf_client/runs.py +++ b/src/wf_client/runs.py @@ -81,6 +81,7 @@ def _run_from_decoded( deployment_id=decoded.deployment_id, diagnostics=decoded.diagnostics, outcome=decoded.outcome, + error=decoded.error, ) return Run( _port=port, diff --git a/src/wf_client/workflows.py b/src/wf_client/workflows.py index 2add4036..bed92300 100644 --- a/src/wf_client/workflows.py +++ b/src/wf_client/workflows.py @@ -128,7 +128,7 @@ class WorkflowArtifact: """Save, inspect, and validate a deployment for this artifact version.""" from .deployments import Deployment - await self._port.save_deployment( + saved = await self._port.save_deployment( { "id": deployment_id, "artifact_id": self.artifact.id, @@ -137,10 +137,27 @@ class WorkflowArtifact: "drift_policy": drift_policy, } ) + if not isinstance(saved, Mapping) or saved.get("deployment_id") != deployment_id: + saved_id = saved.get("deployment_id") if isinstance(saved, Mapping) else None + raise InvalidResponse( + operation="workflow.deployments.save", + details=( + f"saved deployment id {saved_id!r} does not match requested " + f"{deployment_id!r}" + ), + ) deployment = Deployment.from_payload( self._port, await self._port.inspect_deployment(deployment_id=deployment_id), ) + if deployment.deployment_id != deployment_id: + raise InvalidResponse( + operation="workflow.deployments.inspect", + details=( + f"inspected deployment {deployment.deployment_id!r} does not " + f"match requested {deployment_id!r}" + ), + ) if ( deployment.artifact_id != self.artifact.id or deployment.artifact_version != self.artifact.version diff --git a/tests/wf_client/test_deployments.py b/tests/wf_client/test_deployments.py index c79e7c39..6a868dc5 100644 --- a/tests/wf_client/test_deployments.py +++ b/tests/wf_client/test_deployments.py @@ -42,6 +42,13 @@ class _FakePort: self.list_result: dict[str, Any] = {"deployments": []} self.inspect_artifact_id = "report" self.inspect_artifact_version = 1 + self.inspect_deployment_id: str | None = None + self.save_result: dict[str, Any] = { + "deployment_id": "report.production", + "artifact_id": "report", + "artifact_version": 1, + "saved": True, + } self.validation_result: dict[str, Any] = { "deployment_id": "report.production", "artifact_id": "report", @@ -82,12 +89,12 @@ class _FakePort: async def save_deployment(self, deployment: dict[str, Any]) -> object: self.calls.append(("save_deployment", {"deployment": deployment})) - return {"deployment_id": deployment["id"], "saved": True} + return self.save_result async def inspect_deployment(self, *, deployment_id: str) -> object: self.calls.append(("inspect_deployment", {"deployment_id": deployment_id})) return { - "id": deployment_id, + "id": self.inspect_deployment_id or deployment_id, "artifact_id": self.inspect_artifact_id, "artifact_version": self.inspect_artifact_version, "bindings": [ @@ -177,6 +184,44 @@ async def test_explicit_artifact_run_rejects_deployment_for_another_artifact() - assert not any(call[0] == "run_deployment" for call in port.calls) +@pytest.mark.asyncio +async def test_artifact_deploy_rejects_wrong_created_deployment_id() -> None: + artifact = _artifact() + port = cast(_FakePort, artifact._port) + port.save_result["deployment_id"] = "other.deployment" + with pytest.raises(InvalidResponse, match="save"): + await artifact.deploy("report.production") + + +@pytest.mark.asyncio +async def test_artifact_deploy_rejects_wrong_inspected_deployment_id() -> None: + artifact = _artifact() + port = cast(_FakePort, artifact._port) + port.inspect_deployment_id = "other.deployment" + with pytest.raises(InvalidResponse, match="inspect"): + await artifact.deploy("report.production") + + +@pytest.mark.asyncio +async def test_discovered_deployment_rechecks_inspected_artifact_identity() -> None: + artifact = _artifact() + port = cast(_FakePort, artifact._port) + port.list_result = { + "deployments": [ + { + "id": "report.production", + "artifact_id": "report", + "artifact_version": 1, + "binding_count": 0, + "drift_policy": "block", + } + ] + } + port.inspect_artifact_id = "other" + with pytest.raises(InvalidResponse, match="does not target artifact"): + await artifact.run({}) + + @pytest.mark.asyncio async def test_deployment_validation_rejects_identity_mismatch() -> None: artifact = _artifact() @@ -197,6 +242,49 @@ async def test_deployment_run_rejects_mismatched_start_deployment() -> None: await deployment.run({}) +@pytest.mark.asyncio +async def test_deployment_run_rejects_mismatched_start_artifact() -> None: + artifact = _artifact() + deployment = await artifact.deploy("report.production") + port = cast(_FakePort, deployment._port) + port.run_result["artifact_id"] = "other" + with pytest.raises(InvalidResponse, match="workflow.runs.start"): + await deployment.run({}) + + +@pytest.mark.asyncio +async def test_start_malformed_interrupt_reports_start_operation() -> None: + artifact = _artifact() + deployment = await artifact.deploy("report.production") + port = cast(_FakePort, deployment._port) + port.run_result["interrupt"] = { + "id": "interrupt-1", + "frame_id": "root", + "node_id": "approve", + "kind": "approval", + "payload": {}, + "resumable": True, + "route": { + "frame_id": "child", + "node_id": "approve", + "scope_id": "scope", + "lineage_id": "lineage", + "parent_frame_id": "root", + "workflow_ref": { + "name": "local", + "artifact_id": "invalid", + "version": 1, + }, + }, + "outcomes": ["submitted"], + "request_schema": {"type": "object"}, + "resume_schema": {"type": "object"}, + "typed": False, + } + with pytest.raises(InvalidResponse, match="workflow.runs.start"): + await deployment.run({}) + + @pytest.mark.asyncio async def test_deployment_run_preserves_server_error_and_diagnostics() -> None: artifact = _artifact() diff --git a/tests/wf_client/test_runs.py b/tests/wf_client/test_runs.py index c9479c96..0b0b96dc 100644 --- a/tests/wf_client/test_runs.py +++ b/tests/wf_client/test_runs.py @@ -5,7 +5,7 @@ from typing import Any, cast import pytest from wf_client import App, Run, WorkflowClientPort -from wf_client.errors import InvalidResponse +from wf_client.errors import DeploymentNotRunnable, InvalidResponse def _payload( @@ -150,6 +150,15 @@ async def test_malformed_interrupt_route_is_invalid_response() -> None: Run.from_payload(cast(WorkflowClientPort, _Port()), payload) +@pytest.mark.asyncio +async def test_missing_run_id_preserves_server_error() -> None: + payload = _payload(run_id=None, status="failed") + payload["error"] = "server refused to start" + with pytest.raises(DeploymentNotRunnable) as captured: + Run.from_payload(cast(WorkflowClientPort, _Port()), payload) + assert captured.value.error == "server refused to start" + + @pytest.mark.asyncio async def test_non_resumable_run_is_rejected_before_io() -> None: port = _Port()