fix: address workflow manifest review findings
This commit is contained in:
@@ -50,3 +50,43 @@ and scope audit.
|
||||
## Concerns
|
||||
|
||||
None identified.
|
||||
|
||||
## Task 2 Review Fixes
|
||||
|
||||
### Findings Addressed
|
||||
|
||||
1. Reference validation now runs against operations in original source order,
|
||||
and reports `$.methods[index]...` paths before deterministic lexical sorting.
|
||||
A regression test uses the source order `workflow.zeta.run`, then
|
||||
`workflow.alpha.inspect`, and asserts the exact later ref path.
|
||||
2. Success result validation now checks raw mapping keys before `_schema()`
|
||||
removes `title`, so a valid `$ref` plus an extra `title` is rejected.
|
||||
3. The malformed-contract mutation table now uses the typed
|
||||
`DocumentMutation` Protocol instead of an `Any` mutation annotation.
|
||||
|
||||
### TDD Evidence
|
||||
|
||||
- Review regression tests were run before implementation: 2 failed as
|
||||
intended, one for the title-stripping acceptance and one for the sorted
|
||||
operation path.
|
||||
- After implementation, all 41 focused normalization tests passed.
|
||||
|
||||
### Fix Verification
|
||||
|
||||
```text
|
||||
.venv\Scripts\python.exe -m pytest tests\wf_contract_manifest\test_normalize.py -n 0 -q
|
||||
41 passed in 0.13s
|
||||
|
||||
.venv\Scripts\ruff.exe check src\wf_contract_manifest tests\wf_contract_manifest
|
||||
All checks passed!
|
||||
|
||||
.venv\Scripts\basedpyright.exe --level error src\wf_contract_manifest tests\wf_contract_manifest
|
||||
0 errors, 0 warnings, 0 notes
|
||||
|
||||
git diff --check
|
||||
passed (only Git line-ending warnings)
|
||||
```
|
||||
|
||||
### Fix Concerns
|
||||
|
||||
None identified.
|
||||
|
||||
@@ -83,7 +83,7 @@ def _validate_references(
|
||||
) -> None:
|
||||
values: list[tuple[str, JsonValue]] = []
|
||||
for operation_index, operation in enumerate(manifest["operations"]):
|
||||
operation_path = f"$.operations[{operation_index}]"
|
||||
operation_path = f"$.methods[{operation_index}]"
|
||||
for parameter_index, parameter in enumerate(operation["params"]):
|
||||
values.append(
|
||||
(
|
||||
@@ -176,15 +176,19 @@ def manifest_from_openrpc(document: Mapping[str, object]) -> ContractManifest:
|
||||
result = _mapping(method.get("result"), result_path)
|
||||
if "schema" not in result:
|
||||
raise ManifestError(f"{result_path}.schema", "expected an object")
|
||||
result_schema = _schema(result["schema"], f"{result_path}.schema")
|
||||
if set(result_schema) != {"$ref"} or not (
|
||||
isinstance(result_schema["$ref"], str)
|
||||
and result_schema["$ref"].startswith("#/components/schemas/")
|
||||
raw_result_schema = result["schema"]
|
||||
if isinstance(raw_result_schema, Mapping) and (
|
||||
set(raw_result_schema) != {"$ref"}
|
||||
or not (
|
||||
isinstance(raw_result_schema.get("$ref"), str)
|
||||
and raw_result_schema["$ref"].startswith("#/components/schemas/")
|
||||
)
|
||||
):
|
||||
raise ManifestError(
|
||||
f"{result_path}.schema",
|
||||
"success result must reference a named schema component",
|
||||
)
|
||||
result_schema = _schema(raw_result_schema, f"{result_path}.schema")
|
||||
errors: list[JsonSchema] = []
|
||||
for error_index, error_value in enumerate(
|
||||
_list(method.get("errors"), f"{method_path}.errors")
|
||||
@@ -216,7 +220,7 @@ def manifest_from_openrpc(document: Mapping[str, object]) -> ContractManifest:
|
||||
manifest: ContractManifest = {
|
||||
"manifest_version": 1,
|
||||
"source": {"format": "openrpc", "openrpc_version": openrpc_version},
|
||||
"operations": sorted(operations, key=lambda operation: operation["method"]),
|
||||
"operations": operations,
|
||||
"components": {"schemas": normalized_schemas, "errors": normalized_errors},
|
||||
}
|
||||
component_index = {
|
||||
@@ -224,4 +228,7 @@ def manifest_from_openrpc(document: Mapping[str, object]) -> ContractManifest:
|
||||
"errors": set(manifest["components"]["errors"]),
|
||||
}
|
||||
_validate_references(manifest, component_index)
|
||||
manifest["operations"] = sorted(
|
||||
manifest["operations"], key=lambda operation: operation["method"]
|
||||
)
|
||||
return manifest
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
from __future__ import annotations
|
||||
|
||||
from copy import deepcopy
|
||||
from typing import Any
|
||||
from typing import Any, Protocol
|
||||
|
||||
import pytest
|
||||
|
||||
@@ -10,6 +10,10 @@ from wf_contract_manifest import ManifestError, manifest_from_openrpc
|
||||
from .fixtures import synthetic_openrpc_document
|
||||
|
||||
|
||||
class DocumentMutation(Protocol):
|
||||
def __call__(self, document: dict[str, Any]) -> object: ...
|
||||
|
||||
|
||||
def assert_manifest_error(
|
||||
document: dict[str, Any], path: str, message: str
|
||||
) -> None:
|
||||
@@ -214,6 +218,17 @@ def test_rejects_invalid_result_schema_shape() -> None:
|
||||
)
|
||||
|
||||
|
||||
def test_rejects_extra_raw_success_result_schema_keys() -> None:
|
||||
document = synthetic_openrpc_document()
|
||||
document["methods"][0]["result"]["schema"]["title"] = "extra"
|
||||
|
||||
assert_manifest_error(
|
||||
document,
|
||||
"$.methods[0].result.schema",
|
||||
"success result must reference a named schema component",
|
||||
)
|
||||
|
||||
|
||||
def test_rejects_invalid_method_error_schema_shape() -> None:
|
||||
document = synthetic_openrpc_document()
|
||||
document["methods"][1]["errors"][0] = []
|
||||
@@ -278,7 +293,7 @@ def test_rejects_invalid_method_error_schema_shape() -> None:
|
||||
],
|
||||
)
|
||||
def test_rejects_malformed_openrpc_contracts(
|
||||
mutate: Any, path: str, message: str
|
||||
mutate: DocumentMutation, path: str, message: str
|
||||
) -> None:
|
||||
document = synthetic_openrpc_document()
|
||||
mutate(document)
|
||||
@@ -317,6 +332,23 @@ def test_rejects_unsupported_or_dangling_references(
|
||||
assert exc_info.value.message == message
|
||||
|
||||
|
||||
def test_reports_reference_errors_at_original_method_path() -> None:
|
||||
document = synthetic_openrpc_document()
|
||||
assert [method["name"] for method in document["methods"]] == [
|
||||
"workflow.zeta.run",
|
||||
"workflow.alpha.inspect",
|
||||
]
|
||||
document["methods"][0]["errors"][0] = {
|
||||
"$ref": "#/components/errors/Missing"
|
||||
}
|
||||
|
||||
with pytest.raises(ManifestError) as exc_info:
|
||||
manifest_from_openrpc(document)
|
||||
|
||||
assert exc_info.value.path == "$.methods[0].errors[0].$ref"
|
||||
assert exc_info.value.message == "dangling local reference"
|
||||
|
||||
|
||||
def test_accepts_nested_schema_and_error_component_references() -> None:
|
||||
document = synthetic_openrpc_document()
|
||||
document["components"]["schemas"]["AlphaResult"]["properties"]["linked"] = {
|
||||
|
||||
Reference in New Issue
Block a user