code review

This commit is contained in:
lda
2026-06-02 09:52:24 +07:00 Verified
parent e6bb5d6514
commit 10eaaec737
16 changed files with 147 additions and 74 deletions
@@ -397,7 +397,7 @@ _local_path_payload
_state_path_payload
```
Keep `_escape_json_pointer` if `set_draft_route` delegation still needs no local use. Remove it only if no remaining references exist.
Remove `_escape_json_pointer` only if no remaining references exist in `handlers.py`.
Do not remove from `handlers.py` yet unless `rg` proves there are no remaining
callers:
@@ -409,6 +409,7 @@ src/wf_cli/
- `WfMcpService.__post_init__` creates default `FileWorkflowArtifactStore`, `FileDraftWorkspaceStore`, `FileRunStore` if not provided. These are protocol-neutral stores.
- The stores are created from `_store_root(self.store)` which uses the MCP `Store` root.
- After extraction, store creation should be the caller's responsibility (config-driven), not `WfMcpService`'s.
- Next extraction slice must define the store initialization boundary: which caller constructs `FileWorkflowArtifactStore`, `FileDraftWorkspaceStore`, and `FileRunStore`; how config/API callers inject protocol-neutral store implementations; and which tests must explicitly construct stores instead of relying on `WfMcpService.__post_init__`.
### Naming Confusion
@@ -446,6 +447,7 @@ src/wf_cli/
8. **Run full test suite** — all existing tests must pass with import-only changes.
9. **Update `CliContext`** to use `WorkflowApi` instead of `WorkflowSurfaceHandlers`.
10. **Add integration test** for `WorkflowApi` with a mock backend (not `WfMcpService`).
11. **Define store ownership** before extracting service construction: remove implicit workflow-store creation from `WfMcpService.__post_init__`, add config/API hooks for injected stores, and update tests to pass stores explicitly.
### Dependency Graph After Extraction
@@ -15,7 +15,7 @@ The current suspicion:
Write your findings to:
```text
random shit/wf_api_extraction_map.md
docs/superpowers/research/2026-06-01-wf-api-extraction-map.md
```
Keep it structured and link-heavy. Do not paste huge code blocks. Use file paths, symbol names, and short notes.
@@ -50,3 +50,12 @@ that decides what content types are acceptable.
If a result exposes a convenience `text` field, inspect the capability schema
and wrapper hints before using it. Do not assume every content block is text.
Incorrect:
```json
{"source": {"root": "local", "parts": ["content"]}, "target": {"root": "state", "parts": ["summary"]}}
```
Correct: filter `content` to text blocks, extract each `text`, then combine or
select the value before writing it to a string state field.
+1 -1
View File
@@ -74,7 +74,7 @@ class WorkflowArtifactApi:
query=query,
)
]
entries.sort(key=lambda entry: str(entry["name"]))
entries.sort(key=lambda entry: str(entry.get("name", "")))
return paged_list_payload("nodes", entries, cursor=cursor, limit=limit)
async def save_artifact(self, artifact: dict[str, Any]) -> dict[str, Any]:
+2 -1
View File
@@ -306,6 +306,7 @@ def _bounded_trace_example(
trace_count: int,
) -> NextActionPatchExample:
"""Return a safe read_run_trace request; never suggest full trace reads."""
limit = max(1, min(25, trace_count))
return NextActionPatchExample(
description=(
"Read a bounded debug trace slice. Increase start/limit only when needed."
@@ -315,7 +316,7 @@ def _bounded_trace_example(
"run_id": run_id,
"trace_range": {
"start": 0,
"limit": 25,
"limit": limit,
},
},
)
+8 -3
View File
@@ -2,11 +2,16 @@ from __future__ import annotations
from typing import Any, TypeAlias
from pydantic import TypeAdapter, ValidationError
from wf_artifacts import WorkflowCapabilityRef
from wf_platform import CapabilityRef
WorkflowSurfaceCapabilityId: TypeAlias = CapabilityRef | WorkflowCapabilityRef
_CAPABILITY_REF_ADAPTER = TypeAdapter(CapabilityRef)
_WORKFLOW_CAPABILITY_REF_ADAPTER = TypeAdapter(WorkflowCapabilityRef)
def parse_workflow_surface_capability_id(
value: str | dict[str, Any],
@@ -19,10 +24,10 @@ def parse_workflow_surface_capability_id(
"""
if isinstance(value, dict):
if "artifact_id" in value and "version" in value:
return WorkflowCapabilityRef._validate(value)
return CapabilityRef._validate(value)
return _WORKFLOW_CAPABILITY_REF_ADAPTER.validate_python(value)
return _CAPABILITY_REF_ADAPTER.validate_python(value)
try:
return WorkflowCapabilityRef.parse(value)
except ValueError:
except TypeError, ValueError, ValidationError:
return CapabilityRef.parse(value)
+38 -37
View File
@@ -57,6 +57,7 @@ class WorkflowRunApi:
workflow_input: dict[str, Any],
trace_range: TraceRangeLike | None = None,
) -> dict[str, Any]:
trace_values = _trace_range_values(trace_range)
deployment, artifact, diagnostics, tree = (
self.deployments.deployment_validation(deployment_id)
)
@@ -96,22 +97,7 @@ class WorkflowRunApi:
error=run.error,
output=run.output,
trace_count=len(run.trace),
trace=(
[
asdict(entry)
for entry in run.trace[
trace_range.start : trace_range.start + trace_range.limit
]
]
if trace_range is not None
else None
),
trace_start=trace_range.start if trace_range is not None else None,
trace_limit=trace_range.limit if trace_range is not None else None,
trace_truncated=(
trace_range is not None
and len(run.trace) > trace_range.start + trace_range.limit
),
**_trace_slice_fields(run, trace_values),
)
async def resume_run(
@@ -123,6 +109,7 @@ class WorkflowRunApi:
trace_range: TraceRangeLike | None = None,
) -> dict[str, Any]:
"""Resume one durable interrupted deployment run."""
trace_values = _trace_range_values(trace_range)
record, stopped_run = restore_interrupted_run(self._run_store(), run_id)
environment = record.environment
diagnostics = validate_pinned_resume_environment(
@@ -176,22 +163,7 @@ class WorkflowRunApi:
error=run.error,
output=run.output,
trace_count=len(run.trace),
trace=(
[
asdict(entry)
for entry in run.trace[
trace_range.start : trace_range.start + trace_range.limit
]
]
if trace_range is not None
else None
),
trace_start=trace_range.start if trace_range is not None else None,
trace_limit=trace_range.limit if trace_range is not None else None,
trace_truncated=(
trace_range is not None
and len(run.trace) > trace_range.start + trace_range.limit
),
**_trace_slice_fields(run, trace_values),
)
async def inspect_run(self, *, run_id: str) -> dict[str, Any]:
@@ -219,9 +191,9 @@ class WorkflowRunApi:
trace_range: TraceRangeLike,
) -> dict[str, Any]:
"""Return only a caller-bounded debug trace slice from a stopped run."""
trace_values = _trace_range_values(trace_range)
record, run = load_stored_run(self._run_store(), run_id)
environment = record.environment
end = trace_range.start + trace_range.limit
return _run_payload(
deployment=environment.deployment,
artifact=environment.root_artifact,
@@ -230,13 +202,42 @@ class WorkflowRunApi:
resume_readiness=record.resume_readiness.value,
diagnostics=record.diagnostics,
trace_count=len(run.trace),
trace=[asdict(entry) for entry in run.trace[trace_range.start : end]],
trace_start=trace_range.start,
trace_limit=trace_range.limit,
trace_truncated=len(run.trace) > end,
**_trace_slice_fields(run, trace_values),
)
def _trace_range_values(
trace_range: TraceRangeLike | None,
) -> tuple[int, int] | None:
"""Validate protocol-level trace ranges before they reach Python slicing."""
if trace_range is None:
return None
start = trace_range.start
limit = trace_range.limit
if start < 0:
raise ValueError("trace_range.start must be >= 0")
if limit <= 0:
raise ValueError("trace_range.limit must be > 0")
return start, limit
def _trace_slice_fields(
run: RunState,
trace_range: tuple[int, int] | None,
) -> dict[str, Any]:
"""Return bounded trace payload fields, or no trace fields when omitted."""
if trace_range is None:
return {}
start, limit = trace_range
end = start + limit
return {
"trace": [asdict(entry) for entry in run.trace[start:end]],
"trace_start": start,
"trace_limit": limit,
"trace_truncated": len(run.trace) > end,
}
def _run_payload(
*,
deployment: WorkflowDeployment,
+5 -7
View File
@@ -110,18 +110,16 @@ def wrapper_hints_for_capability(
output_schema, output_properties
)
output_map = {name: f"state.{name}" for name in sorted(output_map_properties)}
state_schema = {
"type": "object",
"properties": {
name: schema for name, schema in sorted(output_map_properties.items())
},
}
wrapper_output_schema = {
mapped_output_schema = {
"type": "object",
"properties": {
name: schema for name, schema in sorted(output_map_properties.items())
},
}
# The minimal wrapper stores mapped outputs in state and returns the same
# fields. Split this later only when wrappers support separate return shape.
state_schema = mapped_output_schema
wrapper_output_schema = mapped_output_schema
missing_decisions = _missing_decisions_for_output(hint_output_schema)
outcome_candidates = _boolean_outcome_candidates(output_properties)
if outcome_candidates:
@@ -30,7 +30,6 @@ from .core import WfMcpService
LIVE_SOURCE_CHECK_TIMEOUT_SECONDS = 8.0
_LIVE_SOURCE_CHECK_FAILURES = (
KeyError,
TimeoutError,
OSError,
anyio.ClosedResourceError,
@@ -80,6 +79,16 @@ async def live_source_diagnostics(
continue
try:
connection = service.connections.get(source_id)
except KeyError as exc:
diagnostics.append(
_source_unreachable_diagnostic(
logical_ref=logical_ref,
source_id=source_id,
exc=exc,
)
)
continue
try:
adapter = require_adapter(connection, service.adapters)
auth = service.load_auth(source_id)
await asyncio.wait_for(
@@ -88,24 +97,38 @@ async def live_source_diagnostics(
)
except _LIVE_SOURCE_CHECK_FAILURES as exc:
diagnostics.append(
DependencyDiagnostic(
severity=DiagnosticSeverity.ERROR,
code="source_unreachable",
_source_unreachable_diagnostic(
logical_ref=logical_ref,
bound_source=source_id,
message=(
f"Live check for upstream source {source_id!r} failed: "
f"{type(exc).__name__}: {exc}"
),
repair_hint=(
"Start or reconnect the source, fix its transport/auth "
"configuration, or bind this deployment to another source."
),
source_id=source_id,
exc=exc,
)
)
return diagnostics
def _source_unreachable_diagnostic(
*,
logical_ref: str,
source_id: str,
exc: BaseException,
) -> DependencyDiagnostic:
"""Build a liveness diagnostic without catching unrelated probe bugs."""
return DependencyDiagnostic(
severity=DiagnosticSeverity.ERROR,
code="source_unreachable",
logical_ref=logical_ref,
bound_source=source_id,
message=(
f"Live check for upstream source {source_id!r} failed: "
f"{type(exc).__name__}: {exc}"
),
repair_hint=(
"Start or reconnect the source, fix its transport/auth "
"configuration, or bind this deployment to another source."
),
)
__all__ = [
"LIVE_SOURCE_CHECK_TIMEOUT_SECONDS",
"live_source_diagnostics",
+1 -1
View File
@@ -16,11 +16,11 @@ from wf_api.artifacts import WorkflowArtifactApi
from wf_api.capabilities import WorkflowCapabilityApi
from wf_api.deployments import WorkflowDeploymentApi
from wf_api.drafts import WorkflowDraftApi
from wf_api.listing import paged_list_payload
from wf_api.models import RawWorkflowPlan
from wf_api.runs import WorkflowRunApi
from ..broker.service.workflow_operation_context import context_from_service
from wf_api.listing import paged_list_payload
from .models import TraceRange
if TYPE_CHECKING:
+3 -4
View File
@@ -2,6 +2,8 @@ from __future__ import annotations
import asyncio
import pytest
from wf_artifacts import FileWorkflowArtifactStore
from wf_api.capabilities import WorkflowCapabilityApi
from wf_mcp.broker import WfMcpService
@@ -91,11 +93,8 @@ def test_inspect_capability_raises_on_unknown() -> None:
)
api, _service = _capability_api(artifact_store, register_echo=True)
try:
with pytest.raises(KeyError, match="no.such.capability"):
asyncio.run(api.inspect_capability(qualified_name="no.such.capability"))
assert False, "expected KeyError"
except KeyError as exc:
assert "no.such.capability" in str(exc)
def test_call_capability_node_spec_success() -> None:
+1 -1
View File
@@ -286,7 +286,7 @@ def test_validate_draft_workspace_refreshes_status() -> None:
assert fetched["status"] == "invalid"
def test_create_minimal_draft_workspace_with_error_route() -> None:
def test_create_minimal_draft_workspace_minimal_success_path() -> None:
artifact_store = FileWorkflowArtifactStore(
local_temp_root() / "drafts_minimal_workspace"
)
+2 -4
View File
@@ -15,15 +15,13 @@ def test_wf_api_has_no_wf_mcp_imports() -> None:
tree = ast.parse(py_file.read_text(encoding="utf-8"), filename=str(py_file))
for node in ast.walk(tree):
if isinstance(node, ast.ImportFrom) and node.module is not None:
if node.module.startswith("wf_mcp") or node.module.startswith(
"wf_mcp."
):
if node.module == "wf_mcp" or node.module.startswith("wf_mcp."):
violations.append(
f"{module}:{node.lineno}: from {node.module} import ..."
)
elif isinstance(node, ast.Import):
for alias in node.names:
if alias.name.startswith("wf_mcp"):
if alias.name == "wf_mcp" or alias.name.startswith("wf_mcp."):
violations.append(
f"{module}:{node.lineno}: import {alias.name}"
)
+25
View File
@@ -3,6 +3,8 @@ from __future__ import annotations
import asyncio
from pathlib import Path
import pytest
from wf_artifacts import FileWorkflowArtifactStore, WorkflowDeployment
from wf_api.runs import WorkflowRunApi
from wf_mcp.broker import WfMcpService
@@ -159,6 +161,29 @@ def test_run_api_inspect_and_bounded_trace() -> None:
assert trace["trace_count"] == summary["trace_count"]
def test_run_api_rejects_invalid_trace_range_before_store_lookup() -> None:
root = local_temp_root() / "run_api_invalid_trace_range"
service, _ = _service_with_echo(root)
context = context_from_service(service)
api = WorkflowRunApi(context)
with pytest.raises(ValueError, match="trace_range.start"):
asyncio.run(
api.read_run_trace(
run_id="missing",
trace_range=SimpleTraceRange(start=-1, limit=1),
)
)
with pytest.raises(ValueError, match="trace_range.limit"):
asyncio.run(
api.read_run_trace(
run_id="missing",
trace_range=SimpleTraceRange(start=0, limit=0),
)
)
def test_run_api_handler_delegation_matches() -> None:
root = local_temp_root() / "run_api_delegation"
service, _ = _service_with_echo(root)
@@ -130,6 +130,18 @@ def test_next_actions_from_failed_run_recommends_bounded_trace() -> None:
assert dumped["patch_examples"][0]["tool"] == NextActionTool.READ_RUN_TRACE.value
assert dumped["patch_examples"][0]["request"]["run_id"] == "run_123"
assert dumped["patch_examples"][0]["request"]["trace_range"]["start"] == 0
assert dumped["patch_examples"][0]["request"]["trace_range"]["limit"] == 12
def test_next_actions_from_failed_run_caps_large_trace_example() -> None:
actions = NextActions.from_run_result(
run_id="run_123",
status="failed",
trace_count=100,
diagnostics=[],
)
dumped = actions.model_dump(mode="json")
assert dumped["patch_examples"][0]["request"]["trace_range"]["limit"] == 25