fix: enforce lifecycle response identities
This commit is contained in:
@@ -38,3 +38,11 @@ references are converted to `InvalidResponse` with operation context, and run
|
|||||||
decoding accepts truthful inspect/start/resume operation names.
|
decoding accepts truthful inspect/start/resume operation names.
|
||||||
|
|
||||||
Fixes committed in the follow-up review commit for this report.
|
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.
|
||||||
|
|||||||
@@ -120,6 +120,18 @@ class Deployment:
|
|||||||
f"match requested {self.deployment_id!r}"
|
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"}:
|
if decoded.run_id is None or decoded.status in {"unrunnable", "rejected"}:
|
||||||
raise DeploymentNotRunnable(
|
raise DeploymentNotRunnable(
|
||||||
deployment_id=self.deployment_id,
|
deployment_id=self.deployment_id,
|
||||||
@@ -127,7 +139,11 @@ class Deployment:
|
|||||||
outcome=decoded.outcome,
|
outcome=decoded.outcome,
|
||||||
error=decoded.error,
|
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]]:
|
def _decode_summaries(payload: object) -> list[dict[str, Any]]:
|
||||||
@@ -222,4 +238,15 @@ async def run_artifact(
|
|||||||
f"match requested {matches[0]['id']!r}"
|
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)
|
return await deployment.run(workflow_input)
|
||||||
|
|||||||
@@ -81,6 +81,7 @@ def _run_from_decoded(
|
|||||||
deployment_id=decoded.deployment_id,
|
deployment_id=decoded.deployment_id,
|
||||||
diagnostics=decoded.diagnostics,
|
diagnostics=decoded.diagnostics,
|
||||||
outcome=decoded.outcome,
|
outcome=decoded.outcome,
|
||||||
|
error=decoded.error,
|
||||||
)
|
)
|
||||||
return Run(
|
return Run(
|
||||||
_port=port,
|
_port=port,
|
||||||
|
|||||||
@@ -128,7 +128,7 @@ class WorkflowArtifact:
|
|||||||
"""Save, inspect, and validate a deployment for this artifact version."""
|
"""Save, inspect, and validate a deployment for this artifact version."""
|
||||||
from .deployments import Deployment
|
from .deployments import Deployment
|
||||||
|
|
||||||
await self._port.save_deployment(
|
saved = await self._port.save_deployment(
|
||||||
{
|
{
|
||||||
"id": deployment_id,
|
"id": deployment_id,
|
||||||
"artifact_id": self.artifact.id,
|
"artifact_id": self.artifact.id,
|
||||||
@@ -137,10 +137,27 @@ class WorkflowArtifact:
|
|||||||
"drift_policy": drift_policy,
|
"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(
|
deployment = Deployment.from_payload(
|
||||||
self._port,
|
self._port,
|
||||||
await self._port.inspect_deployment(deployment_id=deployment_id),
|
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 (
|
if (
|
||||||
deployment.artifact_id != self.artifact.id
|
deployment.artifact_id != self.artifact.id
|
||||||
or deployment.artifact_version != self.artifact.version
|
or deployment.artifact_version != self.artifact.version
|
||||||
|
|||||||
@@ -42,6 +42,13 @@ class _FakePort:
|
|||||||
self.list_result: dict[str, Any] = {"deployments": []}
|
self.list_result: dict[str, Any] = {"deployments": []}
|
||||||
self.inspect_artifact_id = "report"
|
self.inspect_artifact_id = "report"
|
||||||
self.inspect_artifact_version = 1
|
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] = {
|
self.validation_result: dict[str, Any] = {
|
||||||
"deployment_id": "report.production",
|
"deployment_id": "report.production",
|
||||||
"artifact_id": "report",
|
"artifact_id": "report",
|
||||||
@@ -82,12 +89,12 @@ class _FakePort:
|
|||||||
|
|
||||||
async def save_deployment(self, deployment: dict[str, Any]) -> object:
|
async def save_deployment(self, deployment: dict[str, Any]) -> object:
|
||||||
self.calls.append(("save_deployment", {"deployment": deployment}))
|
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:
|
async def inspect_deployment(self, *, deployment_id: str) -> object:
|
||||||
self.calls.append(("inspect_deployment", {"deployment_id": deployment_id}))
|
self.calls.append(("inspect_deployment", {"deployment_id": deployment_id}))
|
||||||
return {
|
return {
|
||||||
"id": deployment_id,
|
"id": self.inspect_deployment_id or deployment_id,
|
||||||
"artifact_id": self.inspect_artifact_id,
|
"artifact_id": self.inspect_artifact_id,
|
||||||
"artifact_version": self.inspect_artifact_version,
|
"artifact_version": self.inspect_artifact_version,
|
||||||
"bindings": [
|
"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)
|
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
|
@pytest.mark.asyncio
|
||||||
async def test_deployment_validation_rejects_identity_mismatch() -> None:
|
async def test_deployment_validation_rejects_identity_mismatch() -> None:
|
||||||
artifact = _artifact()
|
artifact = _artifact()
|
||||||
@@ -197,6 +242,49 @@ async def test_deployment_run_rejects_mismatched_start_deployment() -> None:
|
|||||||
await deployment.run({})
|
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
|
@pytest.mark.asyncio
|
||||||
async def test_deployment_run_preserves_server_error_and_diagnostics() -> None:
|
async def test_deployment_run_preserves_server_error_and_diagnostics() -> None:
|
||||||
artifact = _artifact()
|
artifact = _artifact()
|
||||||
|
|||||||
@@ -5,7 +5,7 @@ from typing import Any, cast
|
|||||||
import pytest
|
import pytest
|
||||||
|
|
||||||
from wf_client import App, Run, WorkflowClientPort
|
from wf_client import App, Run, WorkflowClientPort
|
||||||
from wf_client.errors import InvalidResponse
|
from wf_client.errors import DeploymentNotRunnable, InvalidResponse
|
||||||
|
|
||||||
|
|
||||||
def _payload(
|
def _payload(
|
||||||
@@ -150,6 +150,15 @@ async def test_malformed_interrupt_route_is_invalid_response() -> None:
|
|||||||
Run.from_payload(cast(WorkflowClientPort, _Port()), payload)
|
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
|
@pytest.mark.asyncio
|
||||||
async def test_non_resumable_run_is_rejected_before_io() -> None:
|
async def test_non_resumable_run_is_rejected_before_io() -> None:
|
||||||
port = _Port()
|
port = _Port()
|
||||||
|
|||||||
Reference in New Issue
Block a user