Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand Down
55 changes: 55 additions & 0 deletions review-suite/scripts/tests/test_contracts.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
7 changes: 7 additions & 0 deletions review-suite/scripts/validate.py
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand All @@ -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)")
Expand Down
7 changes: 7 additions & 0 deletions skills/babysit-pr/references/review-suite/validate.py
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand All @@ -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)")
Expand Down
7 changes: 7 additions & 0 deletions skills/implement-ticket/references/review-suite/validate.py
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand All @@ -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)")
Expand Down
7 changes: 7 additions & 0 deletions skills/review-code-change/references/review-suite/validate.py
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand All @@ -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)")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand All @@ -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)")
Expand Down
7 changes: 7 additions & 0 deletions skills/review-correctness/references/review-suite/validate.py
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand All @@ -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)")
Expand Down
7 changes: 7 additions & 0 deletions skills/review-fix-loop/references/review-suite/validate.py
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand All @@ -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)")
Expand Down
11 changes: 11 additions & 0 deletions skills/review-fix-loop/scripts/tests/test_validate.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
96 changes: 25 additions & 71 deletions skills/review-fix-loop/scripts/validate.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,44 @@
(`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

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",
Expand Down Expand Up @@ -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())

Expand Down
Loading
Loading