fix: address workflow authoring review findings
This commit is contained in:
@@ -53,11 +53,15 @@ def _graph_parts(path: str) -> tuple[str, tuple[str, ...]]:
|
||||
|
||||
|
||||
def _local_field(path: str) -> str:
|
||||
raw = path.removeprefix("local.")
|
||||
parsed = LocalPath.parse(raw)
|
||||
if len(parsed.parts) != 1:
|
||||
parts = _local_parts(path)
|
||||
if len(parts) != 1:
|
||||
raise ValueError("local path must name one capability field")
|
||||
return parsed.parts[0]
|
||||
return parts[0]
|
||||
|
||||
|
||||
def _local_parts(path: str) -> tuple[str, ...]:
|
||||
"""Parse a CLI local-root path as the rootless core LocalPath value."""
|
||||
return LocalPath.parse(path.removeprefix("local.")).parts
|
||||
|
||||
|
||||
class WorkflowDraftAuthoringApi:
|
||||
@@ -193,7 +197,7 @@ class WorkflowDraftAuthoringApi:
|
||||
source_root, source_parts = (
|
||||
_graph_parts(source_path)
|
||||
if not source_path.startswith("local.")
|
||||
else ("local", LocalPath.parse(source_path).parts)
|
||||
else ("local", _local_parts(source_path))
|
||||
)
|
||||
if target_path.startswith("output."):
|
||||
# GraphSourcePath excludes output targets, but output fields still
|
||||
@@ -205,7 +209,7 @@ class WorkflowDraftAuthoringApi:
|
||||
raise ValueError("output path must name a field, such as output.result")
|
||||
elif target_path.startswith("local."):
|
||||
target_root = "local"
|
||||
target_parts = LocalPath.parse(target_path).parts
|
||||
target_parts = _local_parts(target_path)
|
||||
else:
|
||||
target_root, target_parts = _graph_parts(target_path)
|
||||
|
||||
@@ -271,10 +275,12 @@ class WorkflowDraftAuthoringApi:
|
||||
allow_existing_equivalent=True,
|
||||
)
|
||||
|
||||
current_output_map = self.drafts._step_output_map(
|
||||
workspace_id=workspace_id, step_id=step_id
|
||||
)
|
||||
previous_state_path = current_output_map.get(local_field)
|
||||
output_map = {
|
||||
**self.drafts._step_output_map(
|
||||
workspace_id=workspace_id, step_id=step_id
|
||||
),
|
||||
**current_output_map,
|
||||
local_field: state_path_str,
|
||||
}
|
||||
|
||||
@@ -284,7 +290,15 @@ class WorkflowDraftAuthoringApi:
|
||||
b
|
||||
for b in existing_output
|
||||
if not (
|
||||
isinstance(b, dict) and b.get("target") == output_target_str
|
||||
isinstance(b, dict)
|
||||
and (
|
||||
b.get("target") == output_target_str
|
||||
or b.get("path") == state_path_str
|
||||
or (
|
||||
previous_state_path is not None
|
||||
and b.get("path") == previous_state_path
|
||||
)
|
||||
)
|
||||
)
|
||||
]
|
||||
else:
|
||||
@@ -667,6 +681,14 @@ class WorkflowDraftAuthoringApi:
|
||||
raise ValueError(f"input bindings for step {step_id!r} must be a list")
|
||||
if not isinstance(current_outputs, list):
|
||||
raise ValueError(f"output bindings for step {step_id!r} must be a list")
|
||||
if not all(isinstance(item, dict) for item in current_inputs):
|
||||
raise ValueError(
|
||||
f"input binding entries for step {step_id!r} must be objects"
|
||||
)
|
||||
if not all(isinstance(item, dict) for item in current_outputs):
|
||||
raise ValueError(
|
||||
f"output binding entries for step {step_id!r} must be objects"
|
||||
)
|
||||
input_targets = set(inputs)
|
||||
output_sources = set(outputs)
|
||||
next_inputs = [
|
||||
|
||||
@@ -535,7 +535,7 @@ def _draft_repair_hint(
|
||||
target_field = details.get("target_field")
|
||||
if not isinstance(source_path, str) or not isinstance(target_field, str):
|
||||
return None
|
||||
if source_path.startswith("input."):
|
||||
if source_path.startswith(("input.", "state.")):
|
||||
return (
|
||||
f"wf draft bind {workspace_id} --revision {revision} "
|
||||
f"--step {step_id} --from {source_path} --to local.{target_field}"
|
||||
|
||||
@@ -154,8 +154,10 @@ EXPLAIN_CARDS: tuple[ExplainCard, ...] = (
|
||||
"A draft patch changed output bindings without changing the matching schema.",
|
||||
],
|
||||
how_to_fix=[
|
||||
"For capability output to state, prefer `wf draft bind --from local.FIELD --to state.FIELD`.",
|
||||
"For capability output to state, prefer `wf draft bind <workspace_id> --revision N --step STEP --from local.FIELD --to state.FIELD`.",
|
||||
"To publish one capability output, use the same bind command with `--to output.FIELD`.",
|
||||
"For multiple output bindings, use `wf draft set-output --merge` when preserving existing mappings.",
|
||||
"For an existing state-to-public-output projection, use `wf draft set-workflow-output <workspace_id> --revision N --merge --map state.FIELD=FIELD`.",
|
||||
"Read any `repair_hint` returned by `wf draft validate` before writing JSON Patch.",
|
||||
"Run `wf draft validate <workspace_id>` after the edit.",
|
||||
],
|
||||
|
||||
@@ -51,6 +51,10 @@ _SEARCH_ALWAYS_VISIBLE_TOOL_NAMES = [
|
||||
"wf.workflow.set_step_input_map",
|
||||
"wf.workflow.set_step_output_map",
|
||||
"wf.workflow.set_workflow_output_map",
|
||||
"wf.workflow.bind",
|
||||
"wf.workflow.remove_draft_route",
|
||||
"wf.workflow.remove_draft_step",
|
||||
"wf.workflow.remove_draft_binding",
|
||||
"wf.workflow.create_minimal_draft_workspace",
|
||||
"wf.workflow.create_artifact_from_workspace",
|
||||
"wf.workflow.create_wrapper_from_workspace",
|
||||
|
||||
@@ -35,6 +35,7 @@ DraftPathMap = Annotated[
|
||||
)
|
||||
),
|
||||
]
|
||||
NonEmptyString = Annotated[str, Field(min_length=1)]
|
||||
DraftInputBindings = Annotated[
|
||||
list[InputBinding],
|
||||
Field(
|
||||
@@ -216,7 +217,9 @@ class SetStepInputMapRequest(BaseModel):
|
||||
|
||||
workspace_id: WorkspaceId
|
||||
revision: int = Field(ge=1, description="Expected current workspace revision.")
|
||||
step_id: str = Field(description="Draft step id whose input map should change.")
|
||||
step_id: NonEmptyString = Field(
|
||||
description="Draft step id whose input map should change."
|
||||
)
|
||||
input_map: DraftPathMap
|
||||
merge: bool = Field(
|
||||
default=False,
|
||||
@@ -232,7 +235,9 @@ class SetStepOutputMapRequest(BaseModel):
|
||||
|
||||
workspace_id: WorkspaceId
|
||||
revision: int = Field(ge=1, description="Expected current workspace revision.")
|
||||
step_id: str = Field(description="Draft step id whose output map should change.")
|
||||
step_id: NonEmptyString = Field(
|
||||
description="Draft step id whose output map should change."
|
||||
)
|
||||
output_map: DraftPathMap
|
||||
merge: bool = Field(
|
||||
default=False,
|
||||
@@ -263,9 +268,13 @@ class BindDraftRequest(BaseModel):
|
||||
|
||||
workspace_id: WorkspaceId
|
||||
revision: int = Field(ge=1, description="Expected current workspace revision.")
|
||||
step_id: str = Field(description="Capability-backed draft step id.")
|
||||
source_path: str = Field(description="Source path, for example input.x or local.y.")
|
||||
target_path: str = Field(description="Target path, for example local.x or state.y.")
|
||||
step_id: NonEmptyString = Field(description="Capability-backed draft step id.")
|
||||
source_path: NonEmptyString = Field(
|
||||
description="Source path, for example input.x or local.y."
|
||||
)
|
||||
target_path: NonEmptyString = Field(
|
||||
description="Target path, for example local.x or state.y."
|
||||
)
|
||||
|
||||
|
||||
class AddStepFromCapabilityRequest(BaseModel):
|
||||
@@ -273,13 +282,14 @@ class AddStepFromCapabilityRequest(BaseModel):
|
||||
|
||||
workspace_id: WorkspaceId
|
||||
revision: int = Field(ge=1, description="Expected workspace revision.")
|
||||
step_id: str = Field(description="New draft step id.")
|
||||
capability_name: str = Field(description="Qualified capability name.")
|
||||
step_id: NonEmptyString = Field(description="New draft step id.")
|
||||
capability_name: NonEmptyString = Field(description="Qualified capability name.")
|
||||
route_from_step: str | None = Field(
|
||||
default=None,
|
||||
min_length=1,
|
||||
description="Optional existing step whose outcome should route to the new step.",
|
||||
)
|
||||
route_from_outcome: str = Field(
|
||||
route_from_outcome: NonEmptyString = Field(
|
||||
default="ok",
|
||||
description="Outcome on route_from_step that should route to the new step.",
|
||||
)
|
||||
@@ -306,8 +316,12 @@ class RemoveDraftRouteRequest(BaseModel):
|
||||
|
||||
workspace_id: WorkspaceId
|
||||
revision: int = Field(ge=1, description="Expected current workspace revision.")
|
||||
step_id: str = Field(description="Draft step id whose route should be removed.")
|
||||
outcome: str = Field(description="Outcome label to remove from the step route map.")
|
||||
step_id: NonEmptyString = Field(
|
||||
description="Draft step id whose route should be removed."
|
||||
)
|
||||
outcome: NonEmptyString = Field(
|
||||
description="Outcome label to remove from the step route map."
|
||||
)
|
||||
|
||||
|
||||
class RemoveDraftStepRequest(BaseModel):
|
||||
@@ -315,7 +329,7 @@ class RemoveDraftStepRequest(BaseModel):
|
||||
|
||||
workspace_id: WorkspaceId
|
||||
revision: int = Field(ge=1, description="Expected current workspace revision.")
|
||||
step_id: str = Field(description="Draft step id to remove.")
|
||||
step_id: NonEmptyString = Field(description="Draft step id to remove.")
|
||||
|
||||
|
||||
class RemoveDraftBindingRequest(BaseModel):
|
||||
@@ -323,7 +337,9 @@ class RemoveDraftBindingRequest(BaseModel):
|
||||
|
||||
workspace_id: WorkspaceId
|
||||
revision: int = Field(ge=1, description="Expected current workspace revision.")
|
||||
step_id: str = Field(description="Draft step id whose bindings should be removed.")
|
||||
step_id: NonEmptyString = Field(
|
||||
description="Draft step id whose bindings should be removed."
|
||||
)
|
||||
inputs: list[str] = Field(
|
||||
default_factory=list,
|
||||
description="Local input target names to remove.",
|
||||
|
||||
Reference in New Issue
Block a user