fix: normalize draft semantic revision precedence
This commit is contained in:
@@ -712,7 +712,13 @@ class WorkflowDraftAuthoringApi:
|
|||||||
routes: dict[str, str],
|
routes: dict[str, str],
|
||||||
) -> dict[str, Any]:
|
) -> dict[str, Any]:
|
||||||
"""Atomically set routes for one step, preserving unspecified outcomes."""
|
"""Atomically set routes for one step, preserving unspecified outcomes."""
|
||||||
workspace = self.drafts._draft_store().get_workspace(workspace_id)
|
checked = self._workspace_if_revision_matches(
|
||||||
|
workspace_id=workspace_id,
|
||||||
|
revision=revision,
|
||||||
|
)
|
||||||
|
if isinstance(checked, dict):
|
||||||
|
return checked
|
||||||
|
workspace = checked
|
||||||
draft_routes = workspace.draft.get("routes", {})
|
draft_routes = workspace.draft.get("routes", {})
|
||||||
if not isinstance(draft_routes, dict):
|
if not isinstance(draft_routes, dict):
|
||||||
raise ValueError("draft routes must be an object")
|
raise ValueError("draft routes must be an object")
|
||||||
@@ -721,13 +727,7 @@ class WorkflowDraftAuthoringApi:
|
|||||||
raise ValueError(f"routes for step {step_id!r} must be an object")
|
raise ValueError(f"routes for step {step_id!r} must be an object")
|
||||||
merged = {**existing, **routes}
|
merged = {**existing, **routes}
|
||||||
if merged == existing:
|
if merged == existing:
|
||||||
checked = self._workspace_if_revision_matches(
|
return summarize_draft_workspace(workspace)
|
||||||
workspace_id=workspace_id,
|
|
||||||
revision=revision,
|
|
||||||
)
|
|
||||||
if isinstance(checked, dict):
|
|
||||||
return checked
|
|
||||||
return summarize_draft_workspace(checked)
|
|
||||||
return await self.drafts.patch_draft_workspace(
|
return await self.drafts.patch_draft_workspace(
|
||||||
workspace_id=workspace_id,
|
workspace_id=workspace_id,
|
||||||
revision=revision,
|
revision=revision,
|
||||||
@@ -749,15 +749,15 @@ class WorkflowDraftAuthoringApi:
|
|||||||
target: str,
|
target: str,
|
||||||
) -> dict[str, Any]:
|
) -> dict[str, Any]:
|
||||||
"""Update the target for multiple (step, outcome) pairs atomically."""
|
"""Update the target for multiple (step, outcome) pairs atomically."""
|
||||||
|
checked = self._workspace_if_revision_matches(
|
||||||
|
workspace_id=workspace_id,
|
||||||
|
revision=revision,
|
||||||
|
)
|
||||||
|
if isinstance(checked, dict):
|
||||||
|
return checked
|
||||||
|
workspace = checked
|
||||||
if not branches:
|
if not branches:
|
||||||
checked = self._workspace_if_revision_matches(
|
return summarize_draft_workspace(workspace)
|
||||||
workspace_id=workspace_id,
|
|
||||||
revision=revision,
|
|
||||||
)
|
|
||||||
if isinstance(checked, dict):
|
|
||||||
return checked
|
|
||||||
return summarize_draft_workspace(checked)
|
|
||||||
workspace = self.drafts._draft_store().get_workspace(workspace_id)
|
|
||||||
draft_routes = workspace.draft.get("routes", {})
|
draft_routes = workspace.draft.get("routes", {})
|
||||||
if not isinstance(draft_routes, dict):
|
if not isinstance(draft_routes, dict):
|
||||||
raise ValueError("draft routes must be an object")
|
raise ValueError("draft routes must be an object")
|
||||||
@@ -786,13 +786,7 @@ class WorkflowDraftAuthoringApi:
|
|||||||
}
|
}
|
||||||
)
|
)
|
||||||
if not patch:
|
if not patch:
|
||||||
checked = self._workspace_if_revision_matches(
|
return summarize_draft_workspace(workspace)
|
||||||
workspace_id=workspace_id,
|
|
||||||
revision=revision,
|
|
||||||
)
|
|
||||||
if isinstance(checked, dict):
|
|
||||||
return checked
|
|
||||||
return summarize_draft_workspace(checked)
|
|
||||||
return await self.drafts.patch_draft_workspace(
|
return await self.drafts.patch_draft_workspace(
|
||||||
workspace_id=workspace_id,
|
workspace_id=workspace_id,
|
||||||
revision=revision,
|
revision=revision,
|
||||||
@@ -808,7 +802,13 @@ class WorkflowDraftAuthoringApi:
|
|||||||
outcome: str,
|
outcome: str,
|
||||||
) -> dict[str, Any]:
|
) -> dict[str, Any]:
|
||||||
"""Remove one route; missing routes are revision-checked no-ops."""
|
"""Remove one route; missing routes are revision-checked no-ops."""
|
||||||
workspace = self.drafts._draft_store().get_workspace(workspace_id)
|
checked = self._workspace_if_revision_matches(
|
||||||
|
workspace_id=workspace_id,
|
||||||
|
revision=revision,
|
||||||
|
)
|
||||||
|
if isinstance(checked, dict):
|
||||||
|
return checked
|
||||||
|
workspace = checked
|
||||||
draft_routes = workspace.draft.get("routes", {})
|
draft_routes = workspace.draft.get("routes", {})
|
||||||
if not isinstance(draft_routes, dict):
|
if not isinstance(draft_routes, dict):
|
||||||
raise ValueError("draft routes must be an object")
|
raise ValueError("draft routes must be an object")
|
||||||
@@ -816,13 +816,7 @@ class WorkflowDraftAuthoringApi:
|
|||||||
if not isinstance(step_routes, dict):
|
if not isinstance(step_routes, dict):
|
||||||
raise ValueError(f"routes for step {step_id!r} must be an object")
|
raise ValueError(f"routes for step {step_id!r} must be an object")
|
||||||
if outcome not in step_routes:
|
if outcome not in step_routes:
|
||||||
checked = self._workspace_if_revision_matches(
|
return summarize_draft_workspace(workspace)
|
||||||
workspace_id=workspace_id,
|
|
||||||
revision=revision,
|
|
||||||
)
|
|
||||||
if isinstance(checked, dict):
|
|
||||||
return checked
|
|
||||||
return summarize_draft_workspace(checked)
|
|
||||||
return await self.drafts.patch_draft_workspace(
|
return await self.drafts.patch_draft_workspace(
|
||||||
workspace_id=workspace_id,
|
workspace_id=workspace_id,
|
||||||
revision=revision,
|
revision=revision,
|
||||||
@@ -845,18 +839,18 @@ class WorkflowDraftAuthoringApi:
|
|||||||
step_id: str,
|
step_id: str,
|
||||||
) -> dict[str, Any]:
|
) -> dict[str, Any]:
|
||||||
"""Remove a step and its own route map; inbound routes are left explicit."""
|
"""Remove a step and its own route map; inbound routes are left explicit."""
|
||||||
workspace = self.drafts._draft_store().get_workspace(workspace_id)
|
checked = self._workspace_if_revision_matches(
|
||||||
|
workspace_id=workspace_id,
|
||||||
|
revision=revision,
|
||||||
|
)
|
||||||
|
if isinstance(checked, dict):
|
||||||
|
return checked
|
||||||
|
workspace = checked
|
||||||
steps = workspace.draft.get("steps", {})
|
steps = workspace.draft.get("steps", {})
|
||||||
if not isinstance(steps, dict):
|
if not isinstance(steps, dict):
|
||||||
raise ValueError("draft steps must be an object")
|
raise ValueError("draft steps must be an object")
|
||||||
if step_id not in steps:
|
if step_id not in steps:
|
||||||
checked = self._workspace_if_revision_matches(
|
return summarize_draft_workspace(workspace)
|
||||||
workspace_id=workspace_id,
|
|
||||||
revision=revision,
|
|
||||||
)
|
|
||||||
if isinstance(checked, dict):
|
|
||||||
return checked
|
|
||||||
return summarize_draft_workspace(checked)
|
|
||||||
patch = [
|
patch = [
|
||||||
{
|
{
|
||||||
"op": "remove",
|
"op": "remove",
|
||||||
@@ -889,7 +883,13 @@ class WorkflowDraftAuthoringApi:
|
|||||||
"""Remove selected local input/output bindings from one draft step."""
|
"""Remove selected local input/output bindings from one draft step."""
|
||||||
if not inputs and not outputs:
|
if not inputs and not outputs:
|
||||||
raise ValueError("pass at least one input or output binding to remove")
|
raise ValueError("pass at least one input or output binding to remove")
|
||||||
workspace = self.drafts._draft_store().get_workspace(workspace_id)
|
checked = self._workspace_if_revision_matches(
|
||||||
|
workspace_id=workspace_id,
|
||||||
|
revision=revision,
|
||||||
|
)
|
||||||
|
if isinstance(checked, dict):
|
||||||
|
return checked
|
||||||
|
workspace = checked
|
||||||
step = draft_step(workspace.draft, step_id)
|
step = draft_step(workspace.draft, step_id)
|
||||||
current_inputs = step.get("input", [])
|
current_inputs = step.get("input", [])
|
||||||
current_outputs = step.get("output", [])
|
current_outputs = step.get("output", [])
|
||||||
@@ -914,13 +914,7 @@ class WorkflowDraftAuthoringApi:
|
|||||||
item for item in current_outputs if item.get("source") not in output_sources
|
item for item in current_outputs if item.get("source") not in output_sources
|
||||||
]
|
]
|
||||||
if next_inputs == current_inputs and next_outputs == current_outputs:
|
if next_inputs == current_inputs and next_outputs == current_outputs:
|
||||||
checked = self._workspace_if_revision_matches(
|
return summarize_draft_workspace(workspace)
|
||||||
workspace_id=workspace_id,
|
|
||||||
revision=revision,
|
|
||||||
)
|
|
||||||
if isinstance(checked, dict):
|
|
||||||
return checked
|
|
||||||
return summarize_draft_workspace(checked)
|
|
||||||
patch: list[dict[str, Any]] = []
|
patch: list[dict[str, Any]] = []
|
||||||
if next_inputs != current_inputs:
|
if next_inputs != current_inputs:
|
||||||
patch.append(
|
patch.append(
|
||||||
|
|||||||
@@ -1802,6 +1802,75 @@ async def test_handle_draft_no_change_still_checks_revision(tmp_path: Path) -> N
|
|||||||
assert result["diagnostics"][0]["code"] == "revision_conflict"
|
assert result["diagnostics"][0]["code"] == "revision_conflict"
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
@pytest.mark.parametrize(
|
||||||
|
"operation",
|
||||||
|
["branch", "handle", "remove_route", "remove_step", "remove_binding"],
|
||||||
|
)
|
||||||
|
async def test_route_and_remove_edits_stale_revision_wins_over_semantic_errors(
|
||||||
|
tmp_path: Path,
|
||||||
|
operation: str,
|
||||||
|
) -> None:
|
||||||
|
artifact_store = FileWorkflowArtifactStore(
|
||||||
|
tmp_path / f"draft_route_remove_stale_{operation}"
|
||||||
|
)
|
||||||
|
api, _service, authoring = _draft_api(artifact_store, register_echo=True)
|
||||||
|
draft = _echo_draft()
|
||||||
|
if operation in {"branch", "handle", "remove_route"}:
|
||||||
|
draft["routes"] = "not-an-object"
|
||||||
|
elif operation == "remove_step":
|
||||||
|
draft["steps"] = "not-an-object"
|
||||||
|
await api.create_draft_workspace(workspace_id="draft_ws", draft=draft)
|
||||||
|
before = await api.get_draft_workspace(
|
||||||
|
workspace_id="draft_ws",
|
||||||
|
include_draft=True,
|
||||||
|
)
|
||||||
|
|
||||||
|
if operation == "branch":
|
||||||
|
result = await authoring.branch_draft(
|
||||||
|
workspace_id="draft_ws",
|
||||||
|
revision=2,
|
||||||
|
step_id="echo",
|
||||||
|
routes={"error": "__end__"},
|
||||||
|
)
|
||||||
|
elif operation == "handle":
|
||||||
|
result = await authoring.handle_draft(
|
||||||
|
workspace_id="draft_ws",
|
||||||
|
revision=2,
|
||||||
|
branches=[RouteSource(step_id="echo", outcome="error")],
|
||||||
|
target="__end__",
|
||||||
|
)
|
||||||
|
elif operation == "remove_route":
|
||||||
|
result = await authoring.remove_draft_route(
|
||||||
|
workspace_id="draft_ws",
|
||||||
|
revision=2,
|
||||||
|
step_id="echo",
|
||||||
|
outcome="ok",
|
||||||
|
)
|
||||||
|
elif operation == "remove_step":
|
||||||
|
result = await authoring.remove_draft_step(
|
||||||
|
workspace_id="draft_ws",
|
||||||
|
revision=2,
|
||||||
|
step_id="echo",
|
||||||
|
)
|
||||||
|
else:
|
||||||
|
result = await authoring.remove_draft_binding(
|
||||||
|
workspace_id="draft_ws",
|
||||||
|
revision=2,
|
||||||
|
step_id="missing",
|
||||||
|
inputs=("text",),
|
||||||
|
)
|
||||||
|
|
||||||
|
after = await api.get_draft_workspace(
|
||||||
|
workspace_id="draft_ws",
|
||||||
|
include_draft=True,
|
||||||
|
)
|
||||||
|
assert result["status"] == "conflict"
|
||||||
|
assert result["revision"] == before["revision"]
|
||||||
|
assert result["diagnostics"][0]["code"] == "revision_conflict"
|
||||||
|
assert after == before
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_add_step_from_capability_infers_single_outcome_route(
|
async def test_add_step_from_capability_infers_single_outcome_route(
|
||||||
tmp_path: Path,
|
tmp_path: Path,
|
||||||
@@ -2066,6 +2135,37 @@ async def test_remove_draft_binding_removes_input_and_output_bindings(
|
|||||||
]
|
]
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_remove_draft_binding_envelope_error_precedes_revision_check(
|
||||||
|
tmp_path: Path,
|
||||||
|
) -> None:
|
||||||
|
artifact_store = FileWorkflowArtifactStore(
|
||||||
|
tmp_path / "draft_remove_binding_envelope_precedence"
|
||||||
|
)
|
||||||
|
api, _service, authoring = _draft_api(artifact_store, register_echo=True)
|
||||||
|
await api.create_draft_workspace(workspace_id="draft_ws", draft=_echo_draft())
|
||||||
|
before = await api.get_draft_workspace(
|
||||||
|
workspace_id="draft_ws",
|
||||||
|
include_draft=True,
|
||||||
|
)
|
||||||
|
|
||||||
|
with pytest.raises(
|
||||||
|
ValueError,
|
||||||
|
match="pass at least one input or output binding to remove",
|
||||||
|
):
|
||||||
|
await authoring.remove_draft_binding(
|
||||||
|
workspace_id="draft_ws",
|
||||||
|
revision=2,
|
||||||
|
step_id="echo",
|
||||||
|
)
|
||||||
|
|
||||||
|
after = await api.get_draft_workspace(
|
||||||
|
workspace_id="draft_ws",
|
||||||
|
include_draft=True,
|
||||||
|
)
|
||||||
|
assert after == before
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_remove_missing_draft_element_is_noop(tmp_path: Path) -> None:
|
async def test_remove_missing_draft_element_is_noop(tmp_path: Path) -> None:
|
||||||
artifact_store = FileWorkflowArtifactStore(tmp_path / "remove_noop")
|
artifact_store = FileWorkflowArtifactStore(tmp_path / "remove_noop")
|
||||||
|
|||||||
Reference in New Issue
Block a user