Add scenario inheritance via the extends field#298
Conversation
omarespejel
left a comment
There was a problem hiding this comment.
I mirrored this into my fork for extra CodeRabbit/Qodo review. The feature direction looks useful, but I think a few parser-boundary issues should be fixed before merge.
extendspermits path traversal.
The code rejects absolute paths, but ../ traversal still works after .resolve(), so extends: "../parent.json" can escape the scenario directory and read files outside the intended scenario tree. If scenario files can come from external repos, examples, or CI artifacts, this becomes an unexpected local file-read surface.
I would constrain resolved parent paths to the scenario directory or an explicit scenario root, and add a regression where extends: "../parent.json" is rejected when it escapes the allowed root.
schema_versionshould reject bools and floats explicitly.
Right now True, 1.0, and 2.0 can pass membership checks because Python treats them as equal to integers. This matters more now because extends requires schema v2. I would add a strict guard everywhere scenario schema versions are checked: isinstance(value, int) and not isinstance(value, bool), then check membership in the supported versions.
extends: nullis silently treated like the field is absent.
Because the code uses payload.get("extends"), explicit null is indistinguishable from a missing key and gets silently stripped. Since an explicit invalid field should fail fast, I would distinguish missing key from present null and raise WorldForgeError for null/non-string values.
Smaller note: some new inheritance error messages include resolved absolute paths. It may be better to keep errors relative/sanitized where possible, since these can surface through CLI output.
Address review feedback on AbdelStark#298: - Reject any `..` segment in the `extends` reference string before path resolution so a child cannot escape its own scenario directory via `extends: "../parent.json"`. Two regressions cover the direct and sibling-traversal shapes. - Replace the lenient `payload.get("extends")` check with `"extends" not in payload` so an explicit `extends: null` is treated as malformed instead of silently absent. - Add `_check_schema_version` that rejects `bool` and any non-`int` value (`True`, `2.0`, `"2"`) before the supported-version membership check. The strict typing also runs inside `_resolve_extends_chain` before the inheritance-min-version comparison so `1.0` can no longer bypass the gate via Python int-equal semantics. - Drop the resolved absolute parent path from inheritance error messages and reference the relative `extends` value instead. - Docs: scenarios.md validation table covers the new traversal, null-extends, and strict-typing rules; the resolution-rules bullet notes the `..` rejection.
|
Thanks for the review @omarespejel. All three concerns addressed in 52bc044:
Smaller note also addressed: dropped the resolved absolute parent path from inheritance error messages and now reference the relative |
Schema version 2 adds a single optional top-level `extends` string so a child scenario can reuse a base file's setup. Inheritance is resolved at load time before scenario or matrix parsing. - Replace-at-top-level merge with the child winning on conflict. - Cycle detection (self-cycle and chains) and `SCENARIO_MAX_EXTENDS_DEPTH` guard report typed `WorldForgeError` instances with the offending chain. - Absolute paths, non-string values, empty `extends`, and v1 schemas using `extends` all raise typed errors. - `parse_scenario` and `parse_scenario_matrix` reject unresolved `extends` with a message pointing callers at `load_scenario(<path>)`. - New example gallery under `examples/scenarios/inheritance/` ships a base plus two children covering action overrides and added objects. Existing schema version 1 scenarios continue to validate and run unchanged.
Address review feedback on AbdelStark#298: - Reject any `..` segment in the `extends` reference string before path resolution so a child cannot escape its own scenario directory via `extends: "../parent.json"`. Two regressions cover the direct and sibling-traversal shapes. - Replace the lenient `payload.get("extends")` check with `"extends" not in payload` so an explicit `extends: null` is treated as malformed instead of silently absent. - Add `_check_schema_version` that rejects `bool` and any non-`int` value (`True`, `2.0`, `"2"`) before the supported-version membership check. The strict typing also runs inside `_resolve_extends_chain` before the inheritance-min-version comparison so `1.0` can no longer bypass the gate via Python int-equal semantics. - Drop the resolved absolute parent path from inheritance error messages and reference the relative `extends` value instead. - Docs: scenarios.md validation table covers the new traversal, null-extends, and strict-typing rules; the resolution-rules bullet notes the `..` rejection.
52bc044 to
baf9a37
Compare
Summary
Adds scenario inheritance to the JSON-native scenario format (closes #285 / WF-FEAT3-006). Schema version 2 introduces a single optional top-level
extendsstring that points at a relative parent file. Inheritance is resolved at load time before scenario or matrix parsing, and existing v1 scenarios continue to validate and run unchanged.world.objects, the child copies the fullworldblock. Single parent only.WorldForgeErrorfor missing parents, absolute paths, non-stringextends, empty strings, v1 schema usingextends, self-cycles, transitive cycles, and chains beyondSCENARIO_MAX_EXTENDS_DEPTH = 8.load_scenario(<path>)andload_scenario_matrix(<path>)resolveextends(the path is needed to interpret the relative ref).parse_scenarioandparse_scenario_matrixreject unresolvedextendswith a message pointing callers at the loaders.examples/scenarios/inheritance/ships a base plus two children covering action-override and added-object patterns. All three passworldforge scenario runend-to-end.SCENARIO_SUPPORTED_SCHEMA_VERSIONS,SCENARIO_EXTENDS_MIN_SCHEMA_VERSION, andSCENARIO_MAX_EXTENDS_DEPTHadded toworldforgere-exports.docs/src/scenarios.mdgains a Scenario Inheritance section with merge rules, validation, and matrix interaction.docs/src/artifact-schemas.mdrow updated to mention the new constants.The one ScenarioResult snapshot test in
tests/test_scenarios.pywas updated for the bumped runtimeschema_version.Test plan
uv run ruff check src tests examples scriptsuv run ruff format --check src tests examples scriptsuv run python scripts/generate_provider_docs.py --checkuv run mkdocs build --strictuv run pytest tests/test_scenario_inheritance.py tests/test_scenarios.py tests/test_docs_site.pyuv run pytest(1352 passed, 2 skipped)bash scripts/test_package.sh(1243 passed, 86 skipped)uv run worldforge scenario run examples/scenarios/inheritance/child-a.jsonuv run worldforge scenario run examples/scenarios/inheritance/child-b.json