fix: address semantic draft review nits
This commit is contained in:
@@ -1,4 +1,4 @@
|
||||
# Structured Refs
|
||||
# Structural Refs
|
||||
|
||||
Qualified names are display strings. They are not authoritative identifiers.
|
||||
|
||||
|
||||
@@ -0,0 +1,17 @@
|
||||
from __future__ import annotations
|
||||
|
||||
|
||||
def short_model_name(model: str) -> str:
|
||||
"""Return compact model labels for dense trial tables and session titles."""
|
||||
name = model.rsplit("/", 1)[-1]
|
||||
for suffix in ("-v4-flash-free", "-v2.5-free", "-3-ultra-free"):
|
||||
name = name.replace(suffix, "")
|
||||
return name
|
||||
|
||||
|
||||
def short_challenge_name(challenge: str) -> str:
|
||||
"""Return compact challenge labels without changing stored challenge ids."""
|
||||
return {
|
||||
"browser_click": "browser",
|
||||
"report_workflow": "report",
|
||||
}.get(challenge, challenge)
|
||||
@@ -14,6 +14,10 @@ import yaml
|
||||
|
||||
ROOT = Path(__file__).resolve().parents[2]
|
||||
|
||||
from examples.agent_challenges.names import ( # noqa: E402
|
||||
short_challenge_name,
|
||||
short_model_name,
|
||||
)
|
||||
from examples.agent_challenges.opencode_io import ( # noqa: E402
|
||||
build_opencode_command,
|
||||
opencode_text_results,
|
||||
@@ -44,13 +48,10 @@ def _opencode_trial_title(
|
||||
*, challenge_id: str, model: str, profile: str, index: int
|
||||
) -> str:
|
||||
"""Build a compact OpenCode session title for crowded trial matrices."""
|
||||
challenge_name = {
|
||||
"browser_click": "browser",
|
||||
"report_workflow": "report",
|
||||
}.get(challenge_id, challenge_id)
|
||||
model_name = model.rsplit("/", 1)[-1].replace("-v4-flash-free", "")
|
||||
model_name = model_name.replace("-v2.5-free", "").replace("-3-ultra-free", "")
|
||||
return f"{challenge_name} {model_name} {profile} {index:03d}"
|
||||
return (
|
||||
f"{short_challenge_name(challenge_id)} "
|
||||
f"{short_model_name(model)} {profile} {index:03d}"
|
||||
)
|
||||
|
||||
|
||||
@dataclass(slots=True)
|
||||
|
||||
@@ -6,6 +6,8 @@ from dataclasses import asdict, dataclass
|
||||
from pathlib import Path
|
||||
from typing import Any
|
||||
|
||||
from examples.agent_challenges.names import short_challenge_name, short_model_name
|
||||
|
||||
ROOT = Path(__file__).resolve().parents[2]
|
||||
DEFAULT_CHALLENGES_ROOT = ROOT / "examples" / "agent_challenges"
|
||||
|
||||
@@ -26,20 +28,6 @@ class TrialSummary:
|
||||
notes: str
|
||||
|
||||
|
||||
def _short_model(model: str) -> str:
|
||||
name = model.rsplit("/", 1)[-1]
|
||||
for suffix in ("-v4-flash-free", "-v2.5-free", "-3-ultra-free"):
|
||||
name = name.replace(suffix, "")
|
||||
return name
|
||||
|
||||
|
||||
def _short_challenge(challenge: str) -> str:
|
||||
return {
|
||||
"browser_click": "browser",
|
||||
"report_workflow": "report",
|
||||
}.get(challenge, challenge)
|
||||
|
||||
|
||||
def _string(value: object, default: str = "") -> str:
|
||||
return value if isinstance(value, str) else default
|
||||
|
||||
@@ -83,8 +71,8 @@ def load_trial_summary(path: Path) -> TrialSummary:
|
||||
tokens = _dict(evidence.get("tokens"))
|
||||
self_report = _dict(data.get("agent_self_report"))
|
||||
|
||||
challenge = _short_challenge(_string(identity.get("challenge_id")))
|
||||
model = _short_model(_string(identity.get("model")))
|
||||
challenge = short_challenge_name(_string(identity.get("challenge_id")))
|
||||
model = short_model_name(_string(identity.get("model")))
|
||||
manual = _string(manual_audit.get("official_outcome"), "pending")
|
||||
notes = _string(manual_audit.get("notes"))
|
||||
|
||||
|
||||
@@ -4,6 +4,10 @@ from collections.abc import Sequence
|
||||
from dataclasses import dataclass
|
||||
from typing import Any
|
||||
|
||||
from wf_artifacts.draft_workspaces.models import (
|
||||
WorkflowDraftWorkspace,
|
||||
summarize_draft_workspace,
|
||||
)
|
||||
from wf_core.models.steps import (
|
||||
InputBinding,
|
||||
OutputBinding,
|
||||
@@ -45,6 +49,31 @@ class WorkflowDraftAuthoringApi:
|
||||
self.context = context
|
||||
self.drafts = drafts
|
||||
|
||||
def _workspace_if_revision_matches(
|
||||
self,
|
||||
*,
|
||||
workspace_id: str,
|
||||
revision: int,
|
||||
) -> WorkflowDraftWorkspace | dict[str, Any]:
|
||||
"""Load a workspace for no-op edits while still enforcing optimistic locks."""
|
||||
workspace = self.drafts._draft_store().get_workspace(workspace_id)
|
||||
if workspace.revision == revision:
|
||||
return workspace
|
||||
return {
|
||||
**summarize_draft_workspace(workspace),
|
||||
"status": "conflict",
|
||||
"diagnostics": [
|
||||
{
|
||||
"code": "revision_conflict",
|
||||
"path": "revision",
|
||||
"message": (
|
||||
f"workspace {workspace.id!r} is at revision "
|
||||
f"{workspace.revision}, not {revision}"
|
||||
),
|
||||
}
|
||||
],
|
||||
}
|
||||
|
||||
def _outcomes_for_capability(self, qualified_name: str) -> tuple[str, ...] | None:
|
||||
try:
|
||||
spec = self.context.specs.get_qualified_spec(qualified_name)
|
||||
@@ -317,9 +346,13 @@ class WorkflowDraftAuthoringApi:
|
||||
raise ValueError(f"routes for step {step_id!r} must be an object")
|
||||
merged = {**existing, **routes}
|
||||
if merged == existing:
|
||||
return await self.drafts.get_draft_workspace(
|
||||
checked = self._workspace_if_revision_matches(
|
||||
workspace_id=workspace_id,
|
||||
revision=revision,
|
||||
)
|
||||
if isinstance(checked, dict):
|
||||
return checked
|
||||
return summarize_draft_workspace(checked)
|
||||
return await self.drafts.patch_draft_workspace(
|
||||
workspace_id=workspace_id,
|
||||
revision=revision,
|
||||
@@ -342,9 +375,13 @@ class WorkflowDraftAuthoringApi:
|
||||
) -> dict[str, Any]:
|
||||
"""Update the target for multiple (step, outcome) pairs atomically."""
|
||||
if not branches:
|
||||
return await self.drafts.get_draft_workspace(
|
||||
checked = self._workspace_if_revision_matches(
|
||||
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", {})
|
||||
if not isinstance(draft_routes, dict):
|
||||
@@ -374,9 +411,13 @@ class WorkflowDraftAuthoringApi:
|
||||
}
|
||||
)
|
||||
if not patch:
|
||||
return await self.drafts.get_draft_workspace(
|
||||
checked = self._workspace_if_revision_matches(
|
||||
workspace_id=workspace_id,
|
||||
revision=revision,
|
||||
)
|
||||
if isinstance(checked, dict):
|
||||
return checked
|
||||
return summarize_draft_workspace(checked)
|
||||
return await self.drafts.patch_draft_workspace(
|
||||
workspace_id=workspace_id,
|
||||
revision=revision,
|
||||
|
||||
@@ -53,13 +53,13 @@ def state_root_field(value: str) -> str:
|
||||
|
||||
|
||||
def _local_path_payload(value: str) -> str:
|
||||
return LocalPath._serialize(LocalPath.parse(value))
|
||||
return str(LocalPath.parse(value))
|
||||
|
||||
|
||||
def _graph_path_payload(value: str | GraphSourcePath) -> str:
|
||||
path = value if isinstance(value, GraphSourcePath) else GraphSourcePath.parse(value)
|
||||
return GraphSourcePath._serialize(path)
|
||||
return str(path)
|
||||
|
||||
|
||||
def _state_path_payload(value: str) -> str:
|
||||
return StatePath._serialize(StatePath.parse(value))
|
||||
return str(StatePath.parse(value))
|
||||
|
||||
@@ -32,10 +32,15 @@ def project_output_property_to_state_schema(
|
||||
raise ValueError(f"output field {output_field!r} is not a JSON Schema object")
|
||||
|
||||
projected = deepcopy(state_schema)
|
||||
state_type = projected.get("type")
|
||||
if state_type is not None and state_type != "object":
|
||||
raise ValueError("state_schema must be an object schema")
|
||||
projected.setdefault("type", "object")
|
||||
properties = projected.setdefault("properties", {})
|
||||
if not isinstance(properties, dict):
|
||||
raise ValueError("state_schema.properties must be an object")
|
||||
if state_field in properties:
|
||||
raise ValueError(f"state field {state_field!r} already exists")
|
||||
properties[state_field] = deepcopy(output_property)
|
||||
|
||||
_merge_definition_block(projected, output_schema, "$defs")
|
||||
|
||||
@@ -8,7 +8,7 @@ from wf_artifacts import ArtifactKind
|
||||
from .artifacts import WorkflowArtifactApi
|
||||
from .capabilities import WorkflowCapabilityApi
|
||||
from .deployments import WorkflowDeploymentApi
|
||||
from .draft_authoring import WorkflowDraftAuthoringApi
|
||||
from .draft_authoring import DraftOutcomeRef, WorkflowDraftAuthoringApi
|
||||
from .drafts import WorkflowDraftApi
|
||||
from .models import RawWorkflowPlan
|
||||
from .operation_context import WorkflowOperationContext
|
||||
@@ -436,8 +436,6 @@ class WorkflowApi:
|
||||
branches: list[dict[str, str]],
|
||||
target: str,
|
||||
) -> dict[str, Any]:
|
||||
from .draft_authoring import DraftOutcomeRef
|
||||
|
||||
refs = [
|
||||
DraftOutcomeRef(step_id=b["step_id"], outcome=b["outcome"])
|
||||
for b in branches
|
||||
|
||||
@@ -12,18 +12,39 @@ from wf_cli.io import CliInputError, emit_json, parse_bindings, parse_json_value
|
||||
from wf_cli.remote_errors import run_cli_operation
|
||||
|
||||
|
||||
def _parse_map_flags(values: list[str] | None) -> dict[str, str]:
|
||||
def _parse_assignment_flags(
|
||||
values: list[str] | None,
|
||||
*,
|
||||
option_name: str,
|
||||
expected: str,
|
||||
) -> dict[str, str]:
|
||||
parsed: dict[str, str] = {}
|
||||
for item in values or []:
|
||||
source, separator, target = item.partition("=")
|
||||
if separator != "=" or not source or not target:
|
||||
raise typer.BadParameter("--map must use source=target")
|
||||
raise typer.BadParameter(f"{option_name} must use {expected}")
|
||||
if source in parsed:
|
||||
raise typer.BadParameter(f"duplicate --map for {source!r}")
|
||||
raise typer.BadParameter(f"duplicate {option_name} for {source!r}")
|
||||
parsed[source] = target
|
||||
return parsed
|
||||
|
||||
|
||||
def _parse_map_flags(values: list[str] | None) -> dict[str, str]:
|
||||
return _parse_assignment_flags(
|
||||
values,
|
||||
option_name="--map",
|
||||
expected="source=target",
|
||||
)
|
||||
|
||||
|
||||
def _parse_route_flags(values: list[str] | None) -> dict[str, str]:
|
||||
return _parse_assignment_flags(
|
||||
values,
|
||||
option_name="--route",
|
||||
expected="OUTCOME=TARGET",
|
||||
)
|
||||
|
||||
|
||||
app = typer.Typer(
|
||||
name="draft",
|
||||
help="Create, inspect, patch, validate, and save draft workflows.",
|
||||
@@ -373,15 +394,7 @@ def add_step_from_capability(
|
||||
"""
|
||||
input_map = _parse_map_flags(input_mapping)
|
||||
bind_outputs = _parse_map_flags(output_mapping)
|
||||
routes: dict[str, str] = {}
|
||||
if route:
|
||||
for r in route:
|
||||
key, _, value = r.partition("=")
|
||||
if not key or not value:
|
||||
raise typer.BadParameter(
|
||||
f"invalid route: {r!r} (expected OUTCOME=TARGET)"
|
||||
)
|
||||
routes[key] = value
|
||||
routes = _parse_route_flags(route)
|
||||
context = load_cli_context(ctx)
|
||||
emit_json(
|
||||
run_cli_operation(
|
||||
@@ -418,15 +431,7 @@ def branch_draft(
|
||||
] = None,
|
||||
) -> None:
|
||||
"""Branch multiple outcome routes on a single step atomically."""
|
||||
routes: dict[str, str] = {}
|
||||
if route:
|
||||
for r in route:
|
||||
key, _, value = r.partition("=")
|
||||
if not key or not value:
|
||||
raise typer.BadParameter(
|
||||
f"invalid route: {r!r} (expected OUTCOME=TARGET)"
|
||||
)
|
||||
routes[key] = value
|
||||
routes = _parse_route_flags(route)
|
||||
context = load_cli_context(ctx)
|
||||
emit_json(
|
||||
run_cli_operation(
|
||||
|
||||
@@ -280,11 +280,11 @@ class AddStepFromCapabilityRequest(BaseModel):
|
||||
"require explicit routes."
|
||||
),
|
||||
)
|
||||
input_map: dict[str, str] = Field(
|
||||
input_map: DraftPathMap = Field(
|
||||
default_factory=dict,
|
||||
description="Graph source path to node-local target field.",
|
||||
)
|
||||
bind_outputs: dict[str, str] = Field(
|
||||
bind_outputs: DraftPathMap = Field(
|
||||
default_factory=dict,
|
||||
description="Node-local output field to state path with schema projection.",
|
||||
)
|
||||
|
||||
@@ -274,7 +274,7 @@ def test_interrupt_node_rejects_mixed_old_and_new_binding_styles():
|
||||
)
|
||||
|
||||
|
||||
def test_foreach_node_serializes_over_path_as_structural_json():
|
||||
def test_foreach_node_serializes_over_path_as_canonical_string():
|
||||
node = ForeachNode.model_validate(
|
||||
{
|
||||
"id": "each_item",
|
||||
|
||||
@@ -906,6 +906,26 @@ async def test_branch_draft_no_change_when_routes_unchanged(tmp_path: Path) -> N
|
||||
assert after == before
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_branch_draft_no_change_still_checks_revision(tmp_path: Path) -> None:
|
||||
artifact_store = FileWorkflowArtifactStore(tmp_path / "drafts_branch_noop_stale")
|
||||
api, _service, authoring = _draft_api(artifact_store, register_echo=True)
|
||||
await api.create_draft_workspace(
|
||||
workspace_id="noop_ws",
|
||||
draft=_echo_draft(),
|
||||
)
|
||||
|
||||
result = await authoring.branch_draft(
|
||||
workspace_id="noop_ws",
|
||||
revision=2,
|
||||
step_id="echo",
|
||||
routes={"ok": "__end__"},
|
||||
)
|
||||
|
||||
assert result["status"] == "conflict"
|
||||
assert result["diagnostics"][0]["code"] == "revision_conflict"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_handle_draft_empty_branches_noop(tmp_path: Path) -> None:
|
||||
artifact_store = FileWorkflowArtifactStore(tmp_path / "drafts_handle_noop")
|
||||
@@ -927,6 +947,26 @@ async def test_handle_draft_empty_branches_noop(tmp_path: Path) -> None:
|
||||
assert after == before
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_handle_draft_no_change_still_checks_revision(tmp_path: Path) -> None:
|
||||
artifact_store = FileWorkflowArtifactStore(tmp_path / "drafts_handle_noop_stale")
|
||||
api, _service, authoring = _draft_api(artifact_store, register_echo=True)
|
||||
await api.create_draft_workspace(
|
||||
workspace_id="noop_ws",
|
||||
draft=_echo_draft(),
|
||||
)
|
||||
|
||||
result = await authoring.handle_draft(
|
||||
workspace_id="noop_ws",
|
||||
revision=2,
|
||||
branches=[],
|
||||
target="fail",
|
||||
)
|
||||
|
||||
assert result["status"] == "conflict"
|
||||
assert result["diagnostics"][0]["code"] == "revision_conflict"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_add_step_from_capability_infers_single_outcome_route(
|
||||
tmp_path: Path,
|
||||
|
||||
@@ -67,6 +67,35 @@ def test_project_output_property_rejects_conflicting_defs() -> None:
|
||||
)
|
||||
|
||||
|
||||
def test_project_output_property_rejects_non_object_state_schema() -> None:
|
||||
with pytest.raises(ValueError, match="state_schema must be an object schema"):
|
||||
project_output_property_to_state_schema(
|
||||
state_schema={"type": "array", "items": {"type": "string"}},
|
||||
output_schema={
|
||||
"type": "object",
|
||||
"properties": {"after": {"type": "object"}},
|
||||
},
|
||||
output_field="after",
|
||||
state_field="after",
|
||||
)
|
||||
|
||||
|
||||
def test_project_output_property_rejects_existing_state_field() -> None:
|
||||
with pytest.raises(ValueError, match="state field 'after' already exists"):
|
||||
project_output_property_to_state_schema(
|
||||
state_schema={
|
||||
"type": "object",
|
||||
"properties": {"after": {"type": "string"}},
|
||||
},
|
||||
output_schema={
|
||||
"type": "object",
|
||||
"properties": {"after": {"type": "object"}},
|
||||
},
|
||||
output_field="after",
|
||||
state_field="after",
|
||||
)
|
||||
|
||||
|
||||
def test_project_output_property_rejects_invalid_output_schema() -> None:
|
||||
with pytest.raises(ValueError, match="output_schema is not valid JSON Schema"):
|
||||
project_output_property_to_state_schema(
|
||||
|
||||
@@ -148,15 +148,59 @@ def test_wf_draft_bind_output_to_state_help_explains_composed_edit() -> None:
|
||||
result = runner.invoke(app, ["draft", "bind-output-to-state", "--help"])
|
||||
|
||||
assert result.exit_code == 0
|
||||
assert "state schema" in result.output
|
||||
assert "output binding" in result.output
|
||||
assert "validate" in result.output
|
||||
output = " ".join(result.output.split())
|
||||
assert "state schema" in output
|
||||
assert "output binding" in output
|
||||
assert "validate" in output
|
||||
|
||||
|
||||
def test_wf_draft_add_step_from_capability_help_explains_explicit_wiring() -> None:
|
||||
result = runner.invoke(app, ["draft", "add-step-from-capability", "--help"])
|
||||
|
||||
assert result.exit_code == 0
|
||||
assert "--from-step" in result.output
|
||||
assert "--bind-output" in result.output
|
||||
assert "does not guess" in result.output
|
||||
output = " ".join(result.output.split())
|
||||
assert "--from-step" in output
|
||||
assert "--bind-output" in output
|
||||
assert "does not guess" in output
|
||||
|
||||
|
||||
def test_wf_draft_route_flags_reject_duplicate_outcomes() -> None:
|
||||
add_result = runner.invoke(
|
||||
app,
|
||||
[
|
||||
"draft",
|
||||
"add-step-from-capability",
|
||||
"ws",
|
||||
"--revision",
|
||||
"1",
|
||||
"--step",
|
||||
"call",
|
||||
"--capability",
|
||||
"demo.call",
|
||||
"--route",
|
||||
"ok=call",
|
||||
"--route",
|
||||
"ok=__end__",
|
||||
],
|
||||
)
|
||||
branch_result = runner.invoke(
|
||||
app,
|
||||
[
|
||||
"draft",
|
||||
"branch",
|
||||
"ws",
|
||||
"--revision",
|
||||
"1",
|
||||
"--step",
|
||||
"call",
|
||||
"--route",
|
||||
"ok=call",
|
||||
"--route",
|
||||
"ok=__end__",
|
||||
],
|
||||
)
|
||||
|
||||
assert add_result.exit_code == 2
|
||||
assert branch_result.exit_code == 2
|
||||
assert "duplicate --route for 'ok'" in add_result.output
|
||||
assert "duplicate --route for 'ok'" in branch_result.output
|
||||
|
||||
Reference in New Issue
Block a user