diff --git a/CHANGELOG.md b/CHANGELOG.md index 897d011..bfe5f59 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,8 +4,28 @@ summary: Chronological history of repository and skill changes. # Changelog -## 2026-07-31 — Packaged and documented the standalone review-fix-loop skill, added the review-fix-loop cross-cutting evaluation corpus, recorded the first review-fix-loop `update_pr` fix cycle +## 2026-07-31 — Unified the duplicated JSON-schema validation engine between review-suite and review-fix-loop, packaged and documented the standalone review-fix-loop skill, added the review-fix-loop cross-cutting evaluation corpus, recorded the first review-fix-loop `update_pr` fix cycle +- fix(review-fix-loop): unify the duplicated generic JSON-schema-subset + validation engine (`_path`/`_is_type`/`validate_schema`) with the canonical + `review-suite/scripts/validate.py` copy (issue #115, discovered during epic + #95's closeout review) — extend the canonical engine'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), then refresh every bundled + `references/review-suite/validate.py` copy via `just sync-contracts`; + `skills/review-fix-loop/scripts/validate.py` no longer hand-duplicates + `_path`/`_is_type`/`validate_schema` and instead imports them from its bundled + copy via the same `importlib.util.spec_from_file_location` pattern + `review_gate.py` already uses for `evaluate_bound`, keeping only its own + schema-specific `validate_invocation`/`validate_checkpoint`/ + `validate_terminal_result` and cross-document checks; adds a regression test + proving `max_fix_cycles` non-integer rejection now flows through the shared + engine, plus generic-engine unit tests for `"integer"`/`minimum`/`maximum` in + `review-suite/scripts/tests/test_contracts.py`; 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 - feat(review-fix-loop): package and document the standalone skill for discovery (issue #102, epic #95, the epic's final child) — list `skills/review-fix-loop` in the README's "Current reusable agent skills" section with its @@ -23,6 +43,7 @@ summary: Chronological history of repository and skill changes. that both workflows keep fixes local until convergence, that `update_pr` publishes exactly once, and that every non-converged terminal result reports its retained unpushed commits via `unpushed_commits`/`operator_action` + (`a1623de4d1222d2ae08c53d5e2ee19b7d5693281`) - fix(review-fix-loop): configure `user.email`/`user.name` on both git clones `scripts/evals/corpus.py`'s `up_sequential_publication_race_second_clone_loses` scenario creates, diff --git a/review-suite/scripts/tests/test_contracts.py b/review-suite/scripts/tests/test_contracts.py index 37bb343..9ad988d 100644 --- a/review-suite/scripts/tests/test_contracts.py +++ b/review-suite/scripts/tests/test_contracts.py @@ -582,5 +582,60 @@ def test_a_supplied_field_is_rejected_as_an_unknown_property(self): ) +class GenericEngineIntegerSupportTests(unittest.TestCase): + """#115: the generic JSON-schema-subset engine (`_is_type`/`validate_schema`) + supports `"integer"` type and `minimum`/`maximum` numeric checks as a + backward-compatible superset, so `review-fix-loop`'s validator (whose + schemas use `"type": "integer"` with bounds, e.g. `max_fix_cycles`) can + import this engine instead of hand-duplicating it. No current + `review-packet`/`review-result` schema field uses `integer`, so these + checks are additive and do not change behavior for any existing schema.""" + + def test_is_type_accepts_a_plain_integer(self): + self.assertTrue(VALIDATOR._is_type(3, "integer")) + + def test_is_type_rejects_a_bool_as_integer(self): + # `isinstance(True, int)` is True in Python; a bool must not satisfy + # an integer type check. + self.assertFalse(VALIDATOR._is_type(True, "integer")) + + def test_is_type_rejects_a_non_integer(self): + self.assertFalse(VALIDATOR._is_type(3.5, "integer")) + self.assertFalse(VALIDATOR._is_type("3", "integer")) + + def test_validate_schema_enforces_integer_type(self): + errors = VALIDATOR.validate_schema("3", {"type": "integer"}) + self.assertEqual(["$: expected integer"], errors) + + def test_validate_schema_enforces_minimum(self): + errors = VALIDATOR.validate_schema(0, {"type": "integer", "minimum": 1}) + self.assertEqual(["$: must be >= 1"], errors) + + def test_validate_schema_enforces_maximum(self): + errors = VALIDATOR.validate_schema(11, {"type": "integer", "maximum": 10}) + self.assertEqual(["$: must be <= 10"], errors) + + def test_validate_schema_accepts_an_in_range_integer(self): + errors = VALIDATOR.validate_schema( + 5, {"type": "integer", "minimum": 1, "maximum": 10} + ) + self.assertEqual([], errors) + + def test_existing_packet_and_result_fixtures_are_unaffected(self): + # No current review-packet/review-result schema field uses "integer", + # so adding support for it must not change any existing fixture's + # validation outcome. + manifest = load(ROOT / "fixtures" / "manifest.json") + for entry in manifest: + with self.subTest(entry=entry["name"]): + fixture = ROOT / "fixtures" / entry["name"] + packet = load(fixture / "packet.json") + result = load(fixture / "expected.json") + packet_errors = VALIDATOR.validate_packet(packet) + if entry["packet_valid"]: + self.assertEqual([], packet_errors) + self.assertEqual([], VALIDATOR.validate_result(result)) + + if __name__ == "__main__": unittest.main() diff --git a/review-suite/scripts/validate.py b/review-suite/scripts/validate.py index c88e760..3db2825 100644 --- a/review-suite/scripts/validate.py +++ b/review-suite/scripts/validate.py @@ -94,6 +94,8 @@ def _is_type(value: Any, expected: str) -> bool: return isinstance(value, str) if expected == "boolean": return isinstance(value, bool) + if expected == "integer": + return isinstance(value, int) and not isinstance(value, bool) return False @@ -117,6 +119,11 @@ def validate_schema(value: Any, schema: dict[str, Any], at: str = "$") -> list[s if pattern := schema.get("pattern"): if re.fullmatch(pattern, value) is None: errors.append(f"{at}: does not match {pattern!r}") + if isinstance(value, int) and not isinstance(value, bool): + if "minimum" in schema and value < schema["minimum"]: + errors.append(f"{at}: must be >= {schema['minimum']}") + if "maximum" in schema and value > schema["maximum"]: + errors.append(f"{at}: must be <= {schema['maximum']}") if isinstance(value, list): if len(value) < schema.get("minItems", 0): errors.append(f"{at}: expected at least {schema['minItems']} item(s)") diff --git a/skills/babysit-pr/references/review-suite/validate.py b/skills/babysit-pr/references/review-suite/validate.py index c88e760..3db2825 100644 --- a/skills/babysit-pr/references/review-suite/validate.py +++ b/skills/babysit-pr/references/review-suite/validate.py @@ -94,6 +94,8 @@ def _is_type(value: Any, expected: str) -> bool: return isinstance(value, str) if expected == "boolean": return isinstance(value, bool) + if expected == "integer": + return isinstance(value, int) and not isinstance(value, bool) return False @@ -117,6 +119,11 @@ def validate_schema(value: Any, schema: dict[str, Any], at: str = "$") -> list[s if pattern := schema.get("pattern"): if re.fullmatch(pattern, value) is None: errors.append(f"{at}: does not match {pattern!r}") + if isinstance(value, int) and not isinstance(value, bool): + if "minimum" in schema and value < schema["minimum"]: + errors.append(f"{at}: must be >= {schema['minimum']}") + if "maximum" in schema and value > schema["maximum"]: + errors.append(f"{at}: must be <= {schema['maximum']}") if isinstance(value, list): if len(value) < schema.get("minItems", 0): errors.append(f"{at}: expected at least {schema['minItems']} item(s)") diff --git a/skills/implement-ticket/references/review-suite/validate.py b/skills/implement-ticket/references/review-suite/validate.py index c88e760..3db2825 100644 --- a/skills/implement-ticket/references/review-suite/validate.py +++ b/skills/implement-ticket/references/review-suite/validate.py @@ -94,6 +94,8 @@ def _is_type(value: Any, expected: str) -> bool: return isinstance(value, str) if expected == "boolean": return isinstance(value, bool) + if expected == "integer": + return isinstance(value, int) and not isinstance(value, bool) return False @@ -117,6 +119,11 @@ def validate_schema(value: Any, schema: dict[str, Any], at: str = "$") -> list[s if pattern := schema.get("pattern"): if re.fullmatch(pattern, value) is None: errors.append(f"{at}: does not match {pattern!r}") + if isinstance(value, int) and not isinstance(value, bool): + if "minimum" in schema and value < schema["minimum"]: + errors.append(f"{at}: must be >= {schema['minimum']}") + if "maximum" in schema and value > schema["maximum"]: + errors.append(f"{at}: must be <= {schema['maximum']}") if isinstance(value, list): if len(value) < schema.get("minItems", 0): errors.append(f"{at}: expected at least {schema['minItems']} item(s)") diff --git a/skills/review-code-change/references/review-suite/validate.py b/skills/review-code-change/references/review-suite/validate.py index c88e760..3db2825 100644 --- a/skills/review-code-change/references/review-suite/validate.py +++ b/skills/review-code-change/references/review-suite/validate.py @@ -94,6 +94,8 @@ def _is_type(value: Any, expected: str) -> bool: return isinstance(value, str) if expected == "boolean": return isinstance(value, bool) + if expected == "integer": + return isinstance(value, int) and not isinstance(value, bool) return False @@ -117,6 +119,11 @@ def validate_schema(value: Any, schema: dict[str, Any], at: str = "$") -> list[s if pattern := schema.get("pattern"): if re.fullmatch(pattern, value) is None: errors.append(f"{at}: does not match {pattern!r}") + if isinstance(value, int) and not isinstance(value, bool): + if "minimum" in schema and value < schema["minimum"]: + errors.append(f"{at}: must be >= {schema['minimum']}") + if "maximum" in schema and value > schema["maximum"]: + errors.append(f"{at}: must be <= {schema['maximum']}") if isinstance(value, list): if len(value) < schema.get("minItems", 0): errors.append(f"{at}: expected at least {schema['minItems']} item(s)") diff --git a/skills/review-code-simplicity/references/review-suite/validate.py b/skills/review-code-simplicity/references/review-suite/validate.py index c88e760..3db2825 100644 --- a/skills/review-code-simplicity/references/review-suite/validate.py +++ b/skills/review-code-simplicity/references/review-suite/validate.py @@ -94,6 +94,8 @@ def _is_type(value: Any, expected: str) -> bool: return isinstance(value, str) if expected == "boolean": return isinstance(value, bool) + if expected == "integer": + return isinstance(value, int) and not isinstance(value, bool) return False @@ -117,6 +119,11 @@ def validate_schema(value: Any, schema: dict[str, Any], at: str = "$") -> list[s if pattern := schema.get("pattern"): if re.fullmatch(pattern, value) is None: errors.append(f"{at}: does not match {pattern!r}") + if isinstance(value, int) and not isinstance(value, bool): + if "minimum" in schema and value < schema["minimum"]: + errors.append(f"{at}: must be >= {schema['minimum']}") + if "maximum" in schema and value > schema["maximum"]: + errors.append(f"{at}: must be <= {schema['maximum']}") if isinstance(value, list): if len(value) < schema.get("minItems", 0): errors.append(f"{at}: expected at least {schema['minItems']} item(s)") diff --git a/skills/review-correctness/references/review-suite/validate.py b/skills/review-correctness/references/review-suite/validate.py index c88e760..3db2825 100644 --- a/skills/review-correctness/references/review-suite/validate.py +++ b/skills/review-correctness/references/review-suite/validate.py @@ -94,6 +94,8 @@ def _is_type(value: Any, expected: str) -> bool: return isinstance(value, str) if expected == "boolean": return isinstance(value, bool) + if expected == "integer": + return isinstance(value, int) and not isinstance(value, bool) return False @@ -117,6 +119,11 @@ def validate_schema(value: Any, schema: dict[str, Any], at: str = "$") -> list[s if pattern := schema.get("pattern"): if re.fullmatch(pattern, value) is None: errors.append(f"{at}: does not match {pattern!r}") + if isinstance(value, int) and not isinstance(value, bool): + if "minimum" in schema and value < schema["minimum"]: + errors.append(f"{at}: must be >= {schema['minimum']}") + if "maximum" in schema and value > schema["maximum"]: + errors.append(f"{at}: must be <= {schema['maximum']}") if isinstance(value, list): if len(value) < schema.get("minItems", 0): errors.append(f"{at}: expected at least {schema['minItems']} item(s)") diff --git a/skills/review-fix-loop/references/review-suite/validate.py b/skills/review-fix-loop/references/review-suite/validate.py index c88e760..3db2825 100644 --- a/skills/review-fix-loop/references/review-suite/validate.py +++ b/skills/review-fix-loop/references/review-suite/validate.py @@ -94,6 +94,8 @@ def _is_type(value: Any, expected: str) -> bool: return isinstance(value, str) if expected == "boolean": return isinstance(value, bool) + if expected == "integer": + return isinstance(value, int) and not isinstance(value, bool) return False @@ -117,6 +119,11 @@ def validate_schema(value: Any, schema: dict[str, Any], at: str = "$") -> list[s if pattern := schema.get("pattern"): if re.fullmatch(pattern, value) is None: errors.append(f"{at}: does not match {pattern!r}") + if isinstance(value, int) and not isinstance(value, bool): + if "minimum" in schema and value < schema["minimum"]: + errors.append(f"{at}: must be >= {schema['minimum']}") + if "maximum" in schema and value > schema["maximum"]: + errors.append(f"{at}: must be <= {schema['maximum']}") if isinstance(value, list): if len(value) < schema.get("minItems", 0): errors.append(f"{at}: expected at least {schema['minItems']} item(s)") diff --git a/skills/review-fix-loop/scripts/tests/test_validate.py b/skills/review-fix-loop/scripts/tests/test_validate.py index ff3583c..f3f7e9c 100644 --- a/skills/review-fix-loop/scripts/tests/test_validate.py +++ b/skills/review-fix-loop/scripts/tests/test_validate.py @@ -132,6 +132,17 @@ def test_over_maximum_cycle_budget_rejected(self): errors = VALIDATE.validate_invocation(invocation) self.assertTrue(any("must be <= 10" in error for error in errors), errors) + def test_non_integer_cycle_budget_rejected(self): + # #115: `max_fix_cycles` is `"type": "integer"` with `minimum`/ + # `maximum` bounds. This enforcement now comes from the shared + # `review-suite` generic engine (imported via `validate_schema`) + # rather than a hand-duplicated copy, so a non-integer value must + # still fail closed exactly as it did before deduplication. + invocation = copy.deepcopy(self.local_commit) + invocation["fix_cycle_budget"]["max_fix_cycles"] = 3.5 + errors = VALIDATE.validate_invocation(invocation) + self.assertIn("$.fix_cycle_budget.max_fix_cycles: expected integer", errors) + def test_unsupported_review_only_top_level_field_rejected(self): invocation = copy.deepcopy(self.local_commit) invocation["mode"] = "review_only" diff --git a/skills/review-fix-loop/scripts/validate.py b/skills/review-fix-loop/scripts/validate.py index ad64283..c36c1cc 100644 --- a/skills/review-fix-loop/scripts/validate.py +++ b/skills/review-fix-loop/scripts/validate.py @@ -6,13 +6,23 @@ (`skills/review-code-change/references/review-suite/validate.py`): a skill folder is the unit of distribution, so its validator must work standalone wherever the skill is installed. + +The generic JSON-schema-subset engine (`_path`/`_is_type`/`validate_schema`) +is not hand-duplicated here. It is imported from the bundled +`references/review-suite/validate.py` copy of the canonical +`review-suite/scripts/validate.py` (kept in sync via `just sync-contracts` and +guarded by `review-suite/scripts/tests/test_bundled_contracts.py`), using the +same `importlib` loading pattern `review_gate.py` already uses for +`evaluate_bound`. Only this skill's own schema-specific validators +(`validate_invocation`, `validate_checkpoint`, `validate_terminal_result`, and +the cross-document checks) are defined in this file. """ from __future__ import annotations import argparse +import importlib.util import json -import re import sys from pathlib import Path from typing import Any @@ -20,6 +30,20 @@ HERE = Path(__file__).resolve().parent SCHEMA_DIR = HERE.parent / "references" +_REVIEW_SUITE_VALIDATE_SPEC = importlib.util.spec_from_file_location( + "review_fix_loop_review_suite_validate", + HERE.parent / "references" / "review-suite" / "validate.py", +) +assert _REVIEW_SUITE_VALIDATE_SPEC and _REVIEW_SUITE_VALIDATE_SPEC.loader +_REVIEW_SUITE_VALIDATE = importlib.util.module_from_spec(_REVIEW_SUITE_VALIDATE_SPEC) +_REVIEW_SUITE_VALIDATE_SPEC.loader.exec_module(_REVIEW_SUITE_VALIDATE) + +# Re-exported for any caller that imports this module's generic engine +# directly (mirroring the shape the hand-duplicated functions used to have). +_path = _REVIEW_SUITE_VALIDATE._path +_is_type = _REVIEW_SUITE_VALIDATE._is_type +validate_schema = _REVIEW_SUITE_VALIDATE.validate_schema + SCHEMAS = { "invocation": SCHEMA_DIR / "invocation.schema.json", "checkpoint": SCHEMA_DIR / "checkpoint.schema.json", @@ -57,76 +81,6 @@ BLOCKED_REASONS_IMPLYING_PUBLICATION_FAILED = {"remote_advanced", "publication_failed"} -# --------------------------------------------------------------------------- -# Minimal JSON Schema subset (shared shape with the review-suite validator) -# --------------------------------------------------------------------------- - - -def _path(parent: str, key: object) -> str: - if isinstance(key, int): - return f"{parent}[{key}]" - return f"{parent}.{key}" if parent else str(key) - - -def _is_type(value: Any, expected: str) -> bool: - if expected == "object": - return isinstance(value, dict) - if expected == "array": - return isinstance(value, list) - if expected == "string": - return isinstance(value, str) - if expected == "boolean": - return isinstance(value, bool) - if expected == "integer": - return isinstance(value, int) and not isinstance(value, bool) - return False - - -def validate_schema(value: Any, schema: dict[str, Any], at: str = "$") -> list[str]: - """Validate the JSON Schema subset used by this repository.""" - errors: list[str] = [] - expected_type = schema.get("type") - if expected_type and not _is_type(value, expected_type): - return [f"{at}: expected {expected_type}"] - - if "const" in schema: - const = schema["const"] - # `1 == True` in Python; a boolean constant must reject numeric 1/1.0. - if value != const or isinstance(const, bool) != isinstance(value, bool): - errors.append(f"{at}: expected constant {const!r}") - if "enum" in schema and value not in schema["enum"]: - errors.append(f"{at}: expected one of {schema['enum']!r}") - if isinstance(value, str): - if len(value) < schema.get("minLength", 0): - errors.append(f"{at}: string is too short") - if pattern := schema.get("pattern"): - if re.fullmatch(pattern, value) is None: - errors.append(f"{at}: does not match {pattern!r}") - if isinstance(value, int) and not isinstance(value, bool): - if "minimum" in schema and value < schema["minimum"]: - errors.append(f"{at}: must be >= {schema['minimum']}") - if "maximum" in schema and value > schema["maximum"]: - errors.append(f"{at}: must be <= {schema['maximum']}") - if isinstance(value, list): - if len(value) < schema.get("minItems", 0): - errors.append(f"{at}: expected at least {schema['minItems']} item(s)") - if item_schema := schema.get("items"): - for index, item in enumerate(value): - errors.extend(validate_schema(item, item_schema, _path(at, index))) - if isinstance(value, dict): - properties = schema.get("properties", {}) - for key in schema.get("required", []): - if key not in value: - errors.append(f"{at}: missing required property {key!r}") - if schema.get("additionalProperties") is False: - for key in value.keys() - properties.keys(): - errors.append(f"{_path(at, key)}: unknown property") - for key, child in value.items(): - if key in properties: - errors.extend(validate_schema(child, properties[key], _path(at, key))) - return errors - - def _load_schema(name: str) -> dict[str, Any]: return json.loads(SCHEMAS[name].read_text()) diff --git a/skills/review-solution-simplicity/references/review-suite/validate.py b/skills/review-solution-simplicity/references/review-suite/validate.py index c88e760..3db2825 100644 --- a/skills/review-solution-simplicity/references/review-suite/validate.py +++ b/skills/review-solution-simplicity/references/review-suite/validate.py @@ -94,6 +94,8 @@ def _is_type(value: Any, expected: str) -> bool: return isinstance(value, str) if expected == "boolean": return isinstance(value, bool) + if expected == "integer": + return isinstance(value, int) and not isinstance(value, bool) return False @@ -117,6 +119,11 @@ def validate_schema(value: Any, schema: dict[str, Any], at: str = "$") -> list[s if pattern := schema.get("pattern"): if re.fullmatch(pattern, value) is None: errors.append(f"{at}: does not match {pattern!r}") + if isinstance(value, int) and not isinstance(value, bool): + if "minimum" in schema and value < schema["minimum"]: + errors.append(f"{at}: must be >= {schema['minimum']}") + if "maximum" in schema and value > schema["maximum"]: + errors.append(f"{at}: must be <= {schema['maximum']}") if isinstance(value, list): if len(value) < schema.get("minItems", 0): errors.append(f"{at}: expected at least {schema['minItems']} item(s)")