fix: harden authoring contract inventory
This commit is contained in:
@@ -78,3 +78,45 @@ No Serena configuration was modified.
|
|||||||
target set passes when that isolated test is excluded.
|
target set passes when that isolated test is excluded.
|
||||||
- FastAPI JSON-RPC emits deprecation warnings from the installed
|
- FastAPI JSON-RPC emits deprecation warnings from the installed
|
||||||
`fastapi-jsonrpc` dependency; no new warning class was introduced.
|
`fastapi-jsonrpc` dependency; no new warning class was introduced.
|
||||||
|
|
||||||
|
## Round 1 Review Fixes
|
||||||
|
|
||||||
|
Addressed all three Important findings from `task-3-review.md`.
|
||||||
|
|
||||||
|
### Test-First Evidence
|
||||||
|
|
||||||
|
Each regression was verified RED before its production fix:
|
||||||
|
|
||||||
|
- Invalid persisted workflow schema: `test_inspect_draft_authoring_contract_tolerates_invalid_workflow_schema` initially raised `ValueError` from `schema_path_options` during inventory projection.
|
||||||
|
- Saved wrapper capability: `test_inspect_draft_authoring_contract_resolves_saved_wrapper_capability` initially returned no entry contract because the service only called `get_qualified_spec`.
|
||||||
|
- Explicit empty capability schemas: `test_inspect_draft_authoring_contract_preserves_empty_capability_schemas` was forced back to the pre-fix truthiness resolver and then advertised Pydantic model fields instead of empty projections.
|
||||||
|
|
||||||
|
### Fixes
|
||||||
|
|
||||||
|
- Added per-schema validation at the inventory service boundary. Invalid persisted input, state, or output schemas now produce an empty affected projection and a warning while preserving the other inventory sections.
|
||||||
|
- Added `WorkflowCapabilityApi.resolve_capability_contract` as the shared resolver for live `NodeSpec` and saved wrapper contracts. Draft inventory inspection now resolves wrapper artifacts using the same capability surface and preserves wrapper schemas/outcomes.
|
||||||
|
- Capability schema fallback now uses `is not None`, preserving explicit `{}` input and output contracts.
|
||||||
|
|
||||||
|
### Verification
|
||||||
|
|
||||||
|
```text
|
||||||
|
uv run pytest tests/wf_api/test_drafts_service.py tests/wf_api/test_capability_api.py tests/wf_api/test_authoring_contracts.py -q -k "inspect_draft_authoring_contract or authoring_contract or saved_wrapper"
|
||||||
|
17 passed
|
||||||
|
|
||||||
|
uv run pytest tests/wf_api/test_drafts_service.py tests/wf_transport_rpc_http/test_app.py tests/wf_transport_rpc_http/test_client.py tests/wf_transport_rpc_http/test_openrpc_contract.py -q -k "not reads_admin_state"
|
||||||
|
394 passed, 184 warnings
|
||||||
|
|
||||||
|
uv run ruff check
|
||||||
|
All checks passed
|
||||||
|
|
||||||
|
uv run ruff format --check <touched files>
|
||||||
|
3 files already formatted
|
||||||
|
|
||||||
|
uv run basedpyright --level error src/wf_api/capabilities.py src/wf_api/service.py
|
||||||
|
0 errors, 0 warnings, 0 notes
|
||||||
|
```
|
||||||
|
|
||||||
|
Repository-wide basedpyright still reports 394 pre-existing diagnostics in
|
||||||
|
unrelated examples, CLI, MCP, and test files. The known isolated
|
||||||
|
`test_rpc_workflow_client_reads_admin_state` failure remains excluded from the
|
||||||
|
target command; no admin-event or Serena configuration files were changed.
|
||||||
|
|||||||
@@ -1,6 +1,7 @@
|
|||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
from collections.abc import Mapping, Sequence
|
from collections.abc import Mapping, Sequence
|
||||||
|
from dataclasses import dataclass
|
||||||
from typing import Any, cast
|
from typing import Any, cast
|
||||||
|
|
||||||
from wf_artifacts import (
|
from wf_artifacts import (
|
||||||
@@ -56,6 +57,16 @@ _PROJECT_CREATE_DRAFT_FROM_CAPABILITY = JsonProjector(
|
|||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
@dataclass(frozen=True, slots=True)
|
||||||
|
class ResolvedCapabilityContract:
|
||||||
|
"""Schema and outcome contract shared by node specs and saved wrappers."""
|
||||||
|
|
||||||
|
input_schema: dict[str, Any]
|
||||||
|
output_schema: dict[str, Any]
|
||||||
|
outcomes: tuple[str, ...]
|
||||||
|
description: str | None
|
||||||
|
|
||||||
|
|
||||||
def _schema_field_names(schema: dict[str, Any]) -> list[str]:
|
def _schema_field_names(schema: dict[str, Any]) -> list[str]:
|
||||||
"""Return top-level JSON object property names for compact discovery rows."""
|
"""Return top-level JSON object property names for compact discovery rows."""
|
||||||
properties = schema.get("properties")
|
properties = schema.get("properties")
|
||||||
@@ -166,6 +177,36 @@ class WorkflowCapabilityApi:
|
|||||||
return wrapper_detail
|
return wrapper_detail
|
||||||
raise KeyError(f"unknown workflow capability {qualified_name!r}")
|
raise KeyError(f"unknown workflow capability {qualified_name!r}")
|
||||||
|
|
||||||
|
def resolve_capability_contract(
|
||||||
|
self,
|
||||||
|
qualified_name: str,
|
||||||
|
) -> ResolvedCapabilityContract:
|
||||||
|
"""Resolve authoring schemas for a live node or saved wrapper."""
|
||||||
|
wrapper_artifact = self._wrapper_artifact_for_capability_name(qualified_name)
|
||||||
|
if wrapper_artifact is not None:
|
||||||
|
return ResolvedCapabilityContract(
|
||||||
|
input_schema=wrapper_artifact.input_schema,
|
||||||
|
output_schema=wrapper_artifact.output_schema,
|
||||||
|
outcomes=wrapper_artifact.outcomes,
|
||||||
|
description=wrapper_artifact.description,
|
||||||
|
)
|
||||||
|
|
||||||
|
spec = self.context.specs.get_qualified_spec(qualified_name)
|
||||||
|
return ResolvedCapabilityContract(
|
||||||
|
input_schema=(
|
||||||
|
spec.input_schema_contract
|
||||||
|
if spec.input_schema_contract is not None
|
||||||
|
else spec.input_model.model_json_schema()
|
||||||
|
),
|
||||||
|
output_schema=(
|
||||||
|
spec.output_schema_contract
|
||||||
|
if spec.output_schema_contract is not None
|
||||||
|
else spec.output_model.model_json_schema()
|
||||||
|
),
|
||||||
|
outcomes=spec.outcomes,
|
||||||
|
description=spec.description,
|
||||||
|
)
|
||||||
|
|
||||||
async def call_capability(
|
async def call_capability(
|
||||||
self,
|
self,
|
||||||
*,
|
*,
|
||||||
|
|||||||
+48
-19
@@ -14,6 +14,7 @@ from .authoring_contracts import (
|
|||||||
context_path_options_for_node,
|
context_path_options_for_node,
|
||||||
project_authoring_contract_inventory,
|
project_authoring_contract_inventory,
|
||||||
project_authoring_step_contract,
|
project_authoring_step_contract,
|
||||||
|
schema_path_options,
|
||||||
)
|
)
|
||||||
from .capabilities import WorkflowCapabilityApi
|
from .capabilities import WorkflowCapabilityApi
|
||||||
from .deployments import WorkflowDeploymentApi
|
from .deployments import WorkflowDeploymentApi
|
||||||
@@ -53,9 +54,26 @@ from .operation_context import WorkflowOperationContext
|
|||||||
from .runs import TraceRangeLike, WorkflowRunApi
|
from .runs import TraceRangeLike, WorkflowRunApi
|
||||||
|
|
||||||
|
|
||||||
def _authoring_schema(value: object) -> dict[str, Any]:
|
def _authoring_schema(
|
||||||
"""Return a safe schema object from a possibly invalid persisted draft."""
|
value: object,
|
||||||
return dict(value) if isinstance(value, Mapping) else {}
|
*,
|
||||||
|
field_name: str,
|
||||||
|
root: str,
|
||||||
|
warnings: list[str],
|
||||||
|
) -> dict[str, Any]:
|
||||||
|
"""Return one valid persisted schema, isolating invalid projections."""
|
||||||
|
if not isinstance(value, Mapping):
|
||||||
|
warnings.append(
|
||||||
|
f"{field_name} authoring choices unavailable: expected a schema object"
|
||||||
|
)
|
||||||
|
return {}
|
||||||
|
schema = dict(value)
|
||||||
|
try:
|
||||||
|
schema_path_options(schema, root=root, uses=[])
|
||||||
|
except ValueError as exc:
|
||||||
|
warnings.append(f"{field_name} authoring choices unavailable: {exc}")
|
||||||
|
return {}
|
||||||
|
return schema
|
||||||
|
|
||||||
|
|
||||||
def _authoring_outcomes(value: object) -> list[str]:
|
def _authoring_outcomes(value: object) -> list[str]:
|
||||||
@@ -393,12 +411,8 @@ class WorkflowApi:
|
|||||||
)
|
)
|
||||||
continue
|
continue
|
||||||
try:
|
try:
|
||||||
spec = self.context.specs.get_qualified_spec(capability_name)
|
resolved_contract = self.capabilities.resolve_capability_contract(
|
||||||
input_schema = (
|
capability_name
|
||||||
spec.input_schema_contract or spec.input_model.model_json_schema()
|
|
||||||
)
|
|
||||||
output_schema = (
|
|
||||||
spec.output_schema_contract or spec.output_model.model_json_schema()
|
|
||||||
)
|
)
|
||||||
except (KeyError, TypeError, ValueError) as exc:
|
except (KeyError, TypeError, ValueError) as exc:
|
||||||
if raw_step_id == selected_step_id:
|
if raw_step_id == selected_step_id:
|
||||||
@@ -409,18 +423,18 @@ class WorkflowApi:
|
|||||||
|
|
||||||
description = raw_step.get("desc")
|
description = raw_step.get("desc")
|
||||||
if not isinstance(description, str):
|
if not isinstance(description, str):
|
||||||
description = spec.description
|
description = resolved_contract.description
|
||||||
contract = project_authoring_step_contract(
|
projected_contract = project_authoring_step_contract(
|
||||||
step_id=raw_step_id,
|
step_id=raw_step_id,
|
||||||
label=_step_label(raw_step_id),
|
label=_step_label(raw_step_id),
|
||||||
description=description,
|
description=description,
|
||||||
input_schema=input_schema,
|
input_schema=resolved_contract.input_schema,
|
||||||
output_schema=output_schema,
|
output_schema=resolved_contract.output_schema,
|
||||||
outcomes=spec.outcomes,
|
outcomes=resolved_contract.outcomes,
|
||||||
)
|
)
|
||||||
entry_steps.append(contract)
|
entry_steps.append(projected_contract)
|
||||||
if raw_step_id == selected_step_id:
|
if raw_step_id == selected_step_id:
|
||||||
selected_contract = contract
|
selected_contract = projected_contract
|
||||||
|
|
||||||
context_entries = []
|
context_entries = []
|
||||||
if selected_step_id is not None:
|
if selected_step_id is not None:
|
||||||
@@ -449,9 +463,24 @@ class WorkflowApi:
|
|||||||
workspace_id=workspace_id,
|
workspace_id=workspace_id,
|
||||||
revision=checked.revision,
|
revision=checked.revision,
|
||||||
selected_step_id=selected_step_id,
|
selected_step_id=selected_step_id,
|
||||||
input_schema=_authoring_schema(draft.get("input_schema")),
|
input_schema=_authoring_schema(
|
||||||
state_schema=_authoring_schema(draft.get("state_schema")),
|
draft.get("input_schema"),
|
||||||
output_schema=_authoring_schema(draft.get("output_schema")),
|
field_name="input_schema",
|
||||||
|
root="input",
|
||||||
|
warnings=warnings,
|
||||||
|
),
|
||||||
|
state_schema=_authoring_schema(
|
||||||
|
draft.get("state_schema"),
|
||||||
|
field_name="state_schema",
|
||||||
|
root="state",
|
||||||
|
warnings=warnings,
|
||||||
|
),
|
||||||
|
output_schema=_authoring_schema(
|
||||||
|
draft.get("output_schema"),
|
||||||
|
field_name="output_schema",
|
||||||
|
root="output",
|
||||||
|
warnings=warnings,
|
||||||
|
),
|
||||||
context_entries=context_entries,
|
context_entries=context_entries,
|
||||||
step_input_targets=selected_input_targets,
|
step_input_targets=selected_input_targets,
|
||||||
step_output_sources=selected_output_sources,
|
step_output_sources=selected_output_sources,
|
||||||
|
|||||||
@@ -8,6 +8,7 @@ import pytest
|
|||||||
from pydantic import BaseModel, Field, TypeAdapter, ValidationError
|
from pydantic import BaseModel, Field, TypeAdapter, ValidationError
|
||||||
|
|
||||||
from tests.wf_mcp.test_support import echo_tool
|
from tests.wf_mcp.test_support import echo_tool
|
||||||
|
from tests.wf_mcp.workflow_surface.conftest import echo_artifact
|
||||||
from wf_api.draft_authoring import RouteSource, WorkflowDraftAuthoringApi
|
from wf_api.draft_authoring import RouteSource, WorkflowDraftAuthoringApi
|
||||||
from wf_api.draft_updates import CapabilityStepUpdate
|
from wf_api.draft_updates import CapabilityStepUpdate
|
||||||
from wf_api.drafts import WorkflowDraftApi
|
from wf_api.drafts import WorkflowDraftApi
|
||||||
@@ -174,6 +175,108 @@ async def test_inspect_draft_authoring_contract_projects_selected_capability(
|
|||||||
assert selected["outcomes"] == ["ok"]
|
assert selected["outcomes"] == ["ok"]
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_inspect_draft_authoring_contract_tolerates_invalid_workflow_schema(
|
||||||
|
tmp_path: Path,
|
||||||
|
) -> None:
|
||||||
|
draft_api, _service, authoring = _draft_api(
|
||||||
|
FileWorkflowArtifactStore(tmp_path / "authoring_contract_invalid_schema"),
|
||||||
|
register_echo=True,
|
||||||
|
)
|
||||||
|
draft = _echo_draft()
|
||||||
|
draft["input_schema"] = {"type": 7}
|
||||||
|
draft["state_schema"] = {
|
||||||
|
"type": "object",
|
||||||
|
"properties": {"echoed": {"type": "string"}},
|
||||||
|
}
|
||||||
|
await draft_api.create_draft_workspace(workspace_id="authoring", draft=draft)
|
||||||
|
api = WorkflowApi(authoring.context)
|
||||||
|
|
||||||
|
inventory = await api.inspect_draft_authoring_contract(
|
||||||
|
workspace_id="authoring",
|
||||||
|
revision=1,
|
||||||
|
selected_step_id="echo",
|
||||||
|
)
|
||||||
|
|
||||||
|
assert inventory["readable_sources"]
|
||||||
|
assert all(
|
||||||
|
option["origin"] != "workflow_input" for option in inventory["readable_sources"]
|
||||||
|
)
|
||||||
|
assert {option["path"] for option in inventory["state_targets"]} == {"state.echoed"}
|
||||||
|
assert {option["path"] for option in inventory["workflow_output_targets"]} == {
|
||||||
|
"output.echoed"
|
||||||
|
}
|
||||||
|
assert any("input_schema" in warning for warning in inventory["warnings"])
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_inspect_draft_authoring_contract_resolves_saved_wrapper_capability(
|
||||||
|
tmp_path: Path,
|
||||||
|
) -> None:
|
||||||
|
artifact_store = FileWorkflowArtifactStore(tmp_path / "authoring_contract_wrapper")
|
||||||
|
artifact_store.save_artifact(
|
||||||
|
echo_artifact().model_copy(update={"id": "echo_wrapper", "kind": "wrapper"})
|
||||||
|
)
|
||||||
|
draft_api, _service, authoring = _draft_api(artifact_store)
|
||||||
|
draft = _echo_draft()
|
||||||
|
draft["steps"]["echo"]["use"] = "workflow.echo_wrapper.v1"
|
||||||
|
draft["state_schema"] = {
|
||||||
|
"type": "object",
|
||||||
|
"properties": {"echoed": {"type": "string"}},
|
||||||
|
}
|
||||||
|
await draft_api.create_draft_workspace(workspace_id="authoring", draft=draft)
|
||||||
|
api = WorkflowApi(authoring.context)
|
||||||
|
|
||||||
|
inventory = await api.inspect_draft_authoring_contract(
|
||||||
|
workspace_id="authoring",
|
||||||
|
revision=1,
|
||||||
|
selected_step_id="echo",
|
||||||
|
)
|
||||||
|
|
||||||
|
assert [step["step_id"] for step in inventory["entry_steps"]] == ["echo"]
|
||||||
|
assert {option["path"] for option in inventory["step_input_targets"]} == {
|
||||||
|
"step_input.text"
|
||||||
|
}
|
||||||
|
assert {option["path"] for option in inventory["step_output_sources"]} == {
|
||||||
|
"step_output.echoed"
|
||||||
|
}
|
||||||
|
assert inventory["entry_steps"][0]["outcomes"] == ["completed"]
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_inspect_draft_authoring_contract_preserves_empty_capability_schemas(
|
||||||
|
tmp_path: Path,
|
||||||
|
) -> None:
|
||||||
|
draft_api, service, authoring = _draft_api(
|
||||||
|
FileWorkflowArtifactStore(tmp_path / "authoring_contract_empty_schema")
|
||||||
|
)
|
||||||
|
service.register_connection(
|
||||||
|
ConnectionConfig(id="demo.personal", server="demo", account="personal")
|
||||||
|
)
|
||||||
|
service.register_specs(
|
||||||
|
"demo.personal",
|
||||||
|
replace(echo_tool, input_schema_contract={}, output_schema_contract={}),
|
||||||
|
)
|
||||||
|
draft = _echo_draft()
|
||||||
|
draft["state_schema"] = {
|
||||||
|
"type": "object",
|
||||||
|
"properties": {"echoed": {"type": "string"}},
|
||||||
|
}
|
||||||
|
await draft_api.create_draft_workspace(workspace_id="authoring", draft=draft)
|
||||||
|
api = WorkflowApi(authoring.context)
|
||||||
|
|
||||||
|
inventory = await api.inspect_draft_authoring_contract(
|
||||||
|
workspace_id="authoring",
|
||||||
|
revision=1,
|
||||||
|
selected_step_id="echo",
|
||||||
|
)
|
||||||
|
|
||||||
|
assert inventory["entry_steps"][0]["input_targets"] == []
|
||||||
|
assert inventory["entry_steps"][0]["output_sources"] == []
|
||||||
|
assert inventory["step_input_targets"] == []
|
||||||
|
assert inventory["step_output_sources"] == []
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_inspect_draft_authoring_contract_rejects_unknown_selected_step(
|
async def test_inspect_draft_authoring_contract_rejects_unknown_selected_step(
|
||||||
tmp_path: Path,
|
tmp_path: Path,
|
||||||
|
|||||||
Reference in New Issue
Block a user