fix: bound recursive authoring schema traversal
This commit is contained in:
@@ -58,3 +58,36 @@ uv run ruff format --check src/wf_api/authoring_contracts.py src/wf_api/models/a
|
|||||||
|
|
||||||
None for the Task 1 scope. Runtime-context analysis and persisted workspace or
|
None for the Task 1 scope. Runtime-context analysis and persisted workspace or
|
||||||
capability loading remain intentionally deferred to Tasks 2 and 3.
|
capability loading remain intentionally deferred to Tasks 2 and 3.
|
||||||
|
|
||||||
|
## Round 1 Fix
|
||||||
|
|
||||||
|
The review identified that recursive schema walking was not bounded even
|
||||||
|
though individual `$ref` chains were bounded. A self-referential local
|
||||||
|
definition repeatedly expanded `_append_schema_options` until
|
||||||
|
`RecursionError`.
|
||||||
|
|
||||||
|
Added the regression test
|
||||||
|
`test_schema_path_options_stops_expanding_recursive_local_definition` before
|
||||||
|
changing production code. The RED run failed with `RecursionError` in
|
||||||
|
`_append_schema_options` after repeatedly traversing the self-reference.
|
||||||
|
|
||||||
|
The fix tracks active local `$ref` definitions per traversal branch. A repeated
|
||||||
|
definition is emitted as a selectable path but is not expanded again. Distinct
|
||||||
|
active references are also capped using the existing
|
||||||
|
`_MAX_LOCAL_SCHEMA_REFERENCE_DEPTH` limit from `schema_projection`.
|
||||||
|
|
||||||
|
Round 1 verification:
|
||||||
|
|
||||||
|
```text
|
||||||
|
uv run pytest tests/wf_api/test_authoring_contracts.py tests/wf_api/test_schema_projection.py -q
|
||||||
|
49 passed
|
||||||
|
|
||||||
|
uv run basedpyright --level error src/wf_api/authoring_contracts.py src/wf_api/models/authoring_contracts.py
|
||||||
|
0 errors, 0 warnings, 0 notes
|
||||||
|
|
||||||
|
uv run ruff check src/wf_api/authoring_contracts.py src/wf_api/models/authoring_contracts.py src/wf_api/models/__init__.py tests/wf_api/test_authoring_contracts.py
|
||||||
|
All checks passed!
|
||||||
|
|
||||||
|
uv run ruff format --check src/wf_api/authoring_contracts.py src/wf_api/models/authoring_contracts.py src/wf_api/models/__init__.py tests/wf_api/test_authoring_contracts.py
|
||||||
|
4 files already formatted
|
||||||
|
```
|
||||||
|
|||||||
@@ -54,6 +54,7 @@ def schema_path_options(
|
|||||||
origin=origin,
|
origin=origin,
|
||||||
uses=normalized_uses,
|
uses=normalized_uses,
|
||||||
options=options,
|
options=options,
|
||||||
|
active_references=frozenset(),
|
||||||
)
|
)
|
||||||
return options
|
return options
|
||||||
|
|
||||||
@@ -127,6 +128,7 @@ def _append_schema_options(
|
|||||||
origin: AuthoringPathOrigin,
|
origin: AuthoringPathOrigin,
|
||||||
uses: list[AuthoringPathUse],
|
uses: list[AuthoringPathUse],
|
||||||
options: list[AuthoringPathOptionPayload],
|
options: list[AuthoringPathOptionPayload],
|
||||||
|
active_references: frozenset[str],
|
||||||
) -> None:
|
) -> None:
|
||||||
fragment = schema_fragment_at_location(schema, location)
|
fragment = schema_fragment_at_location(schema, location)
|
||||||
resolved = _resolve_local_reference(schema, fragment)
|
resolved = _resolve_local_reference(schema, fragment)
|
||||||
@@ -164,6 +166,16 @@ def _append_schema_options(
|
|||||||
|
|
||||||
if _is_array_schema(resolved_child):
|
if _is_array_schema(resolved_child):
|
||||||
continue
|
continue
|
||||||
|
reference = _direct_local_reference(child_fragment)
|
||||||
|
next_active_references = active_references
|
||||||
|
if reference is not None:
|
||||||
|
# A repeated definition means this branch is recursive. Keep the
|
||||||
|
# repeated path as a selectable option, but do not expand it again.
|
||||||
|
if reference in active_references:
|
||||||
|
continue
|
||||||
|
next_active_references = active_references | {reference}
|
||||||
|
if len(next_active_references) > _MAX_LOCAL_SCHEMA_REFERENCE_DEPTH:
|
||||||
|
continue
|
||||||
_append_schema_options(
|
_append_schema_options(
|
||||||
schema,
|
schema,
|
||||||
location=child_location,
|
location=child_location,
|
||||||
@@ -171,6 +183,7 @@ def _append_schema_options(
|
|||||||
origin=origin,
|
origin=origin,
|
||||||
uses=uses,
|
uses=uses,
|
||||||
options=options,
|
options=options,
|
||||||
|
active_references=next_active_references,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
@@ -229,3 +242,12 @@ def _description_for(
|
|||||||
def _is_array_schema(schema: Mapping[str, Any]) -> bool:
|
def _is_array_schema(schema: Mapping[str, Any]) -> bool:
|
||||||
schema_type = schema.get("type")
|
schema_type = schema.get("type")
|
||||||
return schema_type == "array"
|
return schema_type == "array"
|
||||||
|
|
||||||
|
|
||||||
|
def _direct_local_reference(schema: Mapping[str, Any]) -> str | None:
|
||||||
|
reference = schema.get("$ref")
|
||||||
|
if isinstance(reference, str) and (
|
||||||
|
reference.startswith("#/$defs/") or reference.startswith("#/definitions/")
|
||||||
|
):
|
||||||
|
return reference
|
||||||
|
return None
|
||||||
|
|||||||
@@ -194,6 +194,28 @@ def test_schema_path_options_resolves_local_definition_metadata() -> None:
|
|||||||
]
|
]
|
||||||
|
|
||||||
|
|
||||||
|
def test_schema_path_options_stops_expanding_recursive_local_definition() -> None:
|
||||||
|
options = schema_path_options(
|
||||||
|
{
|
||||||
|
"type": "object",
|
||||||
|
"properties": {"node": {"$ref": "#/$defs/Node"}},
|
||||||
|
"$defs": {
|
||||||
|
"Node": {
|
||||||
|
"type": "object",
|
||||||
|
"properties": {"child": {"$ref": "#/$defs/Node"}},
|
||||||
|
}
|
||||||
|
},
|
||||||
|
},
|
||||||
|
root="input",
|
||||||
|
uses=["step_input"],
|
||||||
|
)
|
||||||
|
|
||||||
|
assert [option["path"] for option in options] == [
|
||||||
|
"input.node",
|
||||||
|
"input.node.child",
|
||||||
|
]
|
||||||
|
|
||||||
|
|
||||||
def test_schema_path_options_returns_empty_schema_for_unconstrained_property() -> None:
|
def test_schema_path_options_returns_empty_schema_for_unconstrained_property() -> None:
|
||||||
options = schema_path_options(
|
options = schema_path_options(
|
||||||
{
|
{
|
||||||
|
|||||||
Reference in New Issue
Block a user