fix(review-fix-loop): unify duplicated JSON-schema validation engine - #116
Merged
Conversation
## Summary - Extend the canonical `review-suite/scripts/validate.py`'s `_is_type`/ `validate_schema` to support `"type": "integer"` and `minimum`/`maximum` numeric checks, as a backward-compatible superset (no current `review-packet`/`review-result` schema field uses `integer`, so this is behavior-preserving for every existing consumer). - Refresh every bundled `references/review-suite/validate.py` copy via `just sync-contracts` so the extended engine reaches every consuming skill. - Update `skills/review-fix-loop/scripts/validate.py` to import `_path`/`_is_type`/`validate_schema` from its bundled `references/review-suite/validate.py` copy via the same `importlib.util.spec_from_file_location` pattern `review_gate.py` already uses for `evaluate_bound`, instead of hand-duplicating them. Only its own schema-specific `validate_invocation`/`validate_checkpoint`/ `validate_terminal_result` and cross-document checks remain in this file. - Add generic-engine unit tests (integer type, minimum, maximum) to `review-suite/scripts/tests/test_contracts.py` and a regression test in `skills/review-fix-loop/scripts/tests/test_validate.py` proving `max_fix_cycles` non-integer rejection now flows through the shared engine. ## Why An exact-head closeout review of epic #95 found that `skills/review-fix-loop/scripts/validate.py` (added by #96) contained a near-verbatim copy of the generic engine already canonical in `skills/review-suite/scripts/validate.py`, extended only with `"integer"` type support and `minimum`/`maximum` numeric checks. Unlike every other bundled contract copy in this repository, this duplicated engine had no drift-detection test, so a future fix to shared JSON-schema-subset semantics applied to one copy would silently not apply to the other. All 273 pre-existing review-fix-loop tests and 318 pre-existing review-suite tests continue to pass unchanged, plus the 9 new tests this change adds (274 and 326 respectively). Fixes #115
11 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Unifies the duplicated generic JSON-schema-subset validation engine
(
_path/_is_type/validate_schema) soreview-fix-loop's schema validatorreuses the canonical implementation instead of hand-duplicating it:
review-suite/scripts/validate.py's_is_type/validate_schemato support"type": "integer"andminimum/maximumnumeric checks, as a backward-compatible superset. No current
review-packet/review-resultschema field usesinteger, so this isbehavior-preserving for every existing consumer (proved by a new test that
re-runs every existing fixture packet/result through the extended engine and
asserts identical outcomes).
references/review-suite/validate.pycopy viajust sync-contractsso the extended engine reaches every consuming skill(
babysit-pr,implement-ticket,review-code-change,review-code-simplicity,review-correctness,review-fix-loop,review-solution-simplicity).skills/review-fix-loop/scripts/validate.pyto import_path/_is_type/validate_schemafrom its bundledreferences/review-suite/validate.pycopy via the sameimportlib.util.spec_from_file_locationpatternreview_gate.pyalreadyuses for
evaluate_bound, instead of hand-duplicating them. Only its ownschema-specific
validate_invocation/validate_checkpoint/validate_terminal_resultand cross-document checks remain in this file.behavior-preservation) to
review-suite/scripts/tests/test_contracts.pyand a regression test in
skills/review-fix-loop/scripts/tests/test_validate.pyprovingmax_fix_cyclesnon-integer rejection now flows through the shared engine.This is a follow-up ticket for epic #95, discovered during the epic's own
closeout review:
skills/review-fix-loop/scripts/validate.py(added by #96)contained a near-verbatim copy of the generic engine already canonical in
review-suite/scripts/validate.py. Unlike every other bundled contract copy inthis repository (kept in sync via
just sync-contractsandtest_bundled_contracts.py), this duplicated engine had no drift-detectiontest.
Non-goals
review-fix-loop's public validation semantics(
validate_invocation,validate_checkpoint,validate_terminal_result, orthe cross-document checks) — this is a pure internal deduplication.
review-packet/review-resultschemas or their existingconsumers.
Acceptance criteria
review-suite/scripts/validate.py's generic engine supports"integer"type and
minimum/maximumchecks without changing behavior for anyexisting schema.
skills/review-fix-loop/scripts/validate.pyno longer hand-duplicates_path/_is_type/validate_schema; it imports and reuses the canonicalengine.
review-fix-looptests (273) andreview-suitetests(318) continue to pass unchanged in behavior.
integer/minimum/maximumfor at least onereview-fix-loopschemafield (
max_fix_cycles).Validation
just format— pass, 1 file reformatted (new test wrapped by ruff), nobehavior change.
just lint— pass (ruff, mdformat,skills-ref validateon every skill,validate_plugins.py).just test— full repository suite, pass, 0 failures.python3 -m unittest discover -s skills/review-fix-loop/scripts/tests -p 'test_*.py'— 274 tests (273 pre-existing + 1 new), pass.python3 -m unittest discover -s review-suite/scripts/tests -p 'test_*.py'— 326 tests (318 pre-existing + 8 new), pass.
Review
One full
review-code-changepass (fresh, isolated, read-only reviewer givenonly raw evidence) returned a clean aggregate verdict on the first pass — no
blocking or strong-recommendation findings from solution-simplicity,
correctness, or code-simplicity. The reviewer independently confirmed: the
canonical engine's additions are structurally inert for every current
review-packet/review-result field (grepped both schemas for
integer/minimum/maximum, found none); the deleted review-fix-loop engine code wasbyte-for-byte identical in logic to the canonical engine before this change
(zero behavior drift); all 7 bundled copies are byte-identical to canonical;
and both
invocation.schema.json'smax_fix_cyclesandterminal-result.schema.json'soriginal_max_fix_cyclesdeclare{"type": "integer", "minimum": 1, "maximum": 10}, so the new regression testgenuinely exercises the new shared code path.
Guardrail evidence
Diff is 169 insertions / 72 deletions across 12 files — well within
carve-changesets' "a few hundred new or changed lines" preferred range. Onecohesive deliverable (a single internal deduplication), largely mechanical
(bundled-copy resync via
just sync-contracts), reviewed clean as one unit onthe first pass. Ordinary single-PR path, no decomposition needed.
Fixes #115
Refs #95