From 7e3f8f68fb642f016a534db2070a2fd79f27f0bd Mon Sep 17 00:00:00 2001 From: sleegme <100679818+sleegme@users.noreply.github.com> Date: Thu, 23 Jul 2026 13:40:03 +0900 Subject: [PATCH] fix(validate): harden route-case boolean validation (#14) Issue #14: mutates_artifact and redesign_authorized were treated as runtime truthy checks without strict boolean type validation. Mutating routes (preserve, expand, create, redesign, translate) were also not required to declare mutates_artifact: true. validated_contract: - mutates_artifact must be a real bool for every route case - redesign_authorized must be a real bool for every route case - preserve/expand/create/redesign/translate require mutates_artifact: true - critique/brand-check/profile require mutates_artifact: false - redesign still requires redesign_authorized: true (unchanged) - errors identify the case id plus the offending field or route contract Add regression coverage for string/integer rejections, each mutating route rejecting mutates_artifact: false, each non-mutating route rejecting mutates_artifact: true, and committed-fixture pass-through. Red+green: full suite moves from 31 to 39 tests. --- scripts/validate_project.py | 29 +++++++-- tests/test_project.py | 119 ++++++++++++++++++++++++++++++++++++ 2 files changed, 144 insertions(+), 4 deletions(-) diff --git a/scripts/validate_project.py b/scripts/validate_project.py index 2dbf4cd..1bd3fca 100644 --- a/scripts/validate_project.py +++ b/scripts/validate_project.py @@ -183,11 +183,34 @@ def validate_route_cases() -> None: require(route in ROUTES, f"unknown route in {case['id']}: {route}") require( isinstance(case["meaningful_design_exists"], bool), - f"invalid meaningful_design_exists in {case['id']}", + f"{case['id']} meaningful_design_exists must be a bool", + ) + require( + isinstance(case["mutates_artifact"], bool), + f"{case['id']} mutates_artifact must be a bool, got {type(case['mutates_artifact']).__name__}", + ) + require( + isinstance(case["redesign_authorized"], bool), + f"{case['id']} redesign_authorized must be a bool, got {type(case['redesign_authorized']).__name__}", ) counts[route] += 1 + mutating_routes = {"preserve", "expand", "create", "redesign", "translate"} + non_mutating_routes = {"critique", "brand-check", "profile"} + if route in mutating_routes: + require( + case["mutates_artifact"] is True, + f"{case['id']} {route} route contract requires mutates_artifact: true", + ) + elif route in non_mutating_routes: + require( + case["mutates_artifact"] is False, + f"{case['id']} {route} route contract requires mutates_artifact: false", + ) if route == "redesign": - require(case["redesign_authorized"] is True, f"{case['id']} lacks redesign authorization") + require( + case["redesign_authorized"] is True, + f"{case['id']} {route} route contract requires redesign_authorized: true", + ) if route == "create": require( case["meaningful_design_exists"] is False, @@ -198,8 +221,6 @@ def validate_route_cases() -> None: case["meaningful_design_exists"] is True, f"{case['id']} requires a meaningful existing design", ) - if route in {"critique", "brand-check", "profile"}: - require(case["mutates_artifact"] is False, f"{case['id']} must not mutate design") require(all(counts[route] >= 5 for route in ROUTES), f"insufficient route coverage: {counts}") diff --git a/tests/test_project.py b/tests/test_project.py index b2b395b..0663c61 100644 --- a/tests/test_project.py +++ b/tests/test_project.py @@ -128,6 +128,125 @@ def test_project_archive_contains_source_without_git_metadata(self) -> None: self.assertFalse(any("/.git/" in name or "/dist/" in name for name in names)) +def _base_routing_cases() -> list[dict[str, object]]: + """A synthetic routing-cases fixture that satisfies every existing invariant. + + Each of the eight routes appears six times, with the field values that the + repository's own routing-cases.json already uses. The fixture is intentionally + self-contained so that the Issue #14 regression tests can mutate one field at + a time without reaching into the real committed fixture. + """ + route_values: dict[str, dict[str, object]] = { + "preserve": {"meaningful_design_exists": True, "mutates_artifact": True, "redesign_authorized": False}, + "expand": {"meaningful_design_exists": True, "mutates_artifact": True, "redesign_authorized": False}, + "create": {"meaningful_design_exists": False, "mutates_artifact": True, "redesign_authorized": False}, + "redesign": {"meaningful_design_exists": True, "mutates_artifact": True, "redesign_authorized": True}, + "critique": {"meaningful_design_exists": True, "mutates_artifact": False, "redesign_authorized": False}, + "brand-check": {"meaningful_design_exists": True, "mutates_artifact": False, "redesign_authorized": False}, + "translate": {"meaningful_design_exists": True, "mutates_artifact": True, "redesign_authorized": False}, + "profile": {"meaningful_design_exists": True, "mutates_artifact": False, "redesign_authorized": False}, + } + cases: list[dict[str, object]] = [] + for route, values in route_values.items(): + for index in range(6): + cases.append( + { + "id": f"{route.upper()[:3]}.{index}", + "prompt": f"synthetic prompt {route} {index}", + "expected_route": route, + "basis": "synthetic basis", + **values, + } + ) + return cases + + +class RouteCaseValidationTests(unittest.TestCase): + """Issue #14: route-case boolean fields and route contracts are strictly enforced.""" + + def _run_with_cases(self, cases: list[dict[str, object]]) -> None: + path = validate_project.ROOT / "evals" / "routing-cases.json" + backup = path.read_text(encoding="utf-8") + try: + path.write_text(json.dumps(cases, ensure_ascii=False, indent=2) + "\n", encoding="utf-8") + validate_project.validate_route_cases() + finally: + path.write_text(backup, encoding="utf-8") + + def _expect_failure(self, cases: list[dict[str, object]]) -> str: + path = validate_project.ROOT / "evals" / "routing-cases.json" + backup = path.read_text(encoding="utf-8") + try: + path.write_text(json.dumps(cases, ensure_ascii=False, indent=2) + "\n", encoding="utf-8") + with self.assertRaises(validate_project.ValidationError) as ctx: + validate_project.validate_route_cases() + return str(ctx.exception) + finally: + path.write_text(backup, encoding="utf-8") + + def test_committed_routing_cases_fixture_is_valid(self) -> None: + validate_project.validate_route_cases() + + def test_synthetic_base_fixture_is_valid(self) -> None: + self._run_with_cases(_base_routing_cases()) + + def test_string_mutates_artifact_is_rejected(self) -> None: + cases = _base_routing_cases() + cases[0]["mutates_artifact"] = "true" + message = self._expect_failure(cases) + self.assertIn(cases[0]["id"], message) + self.assertIn("mutates_artifact", message) + self.assertIn("bool", message) + + def test_integer_mutates_artifact_is_rejected(self) -> None: + cases = _base_routing_cases() + cases[0]["mutates_artifact"] = 1 + message = self._expect_failure(cases) + self.assertIn(cases[0]["id"], message) + self.assertIn("mutates_artifact", message) + self.assertIn("bool", message) + + def test_string_redesign_authorized_is_rejected(self) -> None: + cases = _base_routing_cases() + cases[0]["redesign_authorized"] = "false" + message = self._expect_failure(cases) + self.assertIn(cases[0]["id"], message) + self.assertIn("redesign_authorized", message) + self.assertIn("bool", message) + + def test_integer_redesign_authorized_is_rejected(self) -> None: + cases = _base_routing_cases() + cases[0]["redesign_authorized"] = 0 + message = self._expect_failure(cases) + self.assertIn(cases[0]["id"], message) + self.assertIn("redesign_authorized", message) + self.assertIn("bool", message) + + def test_mutating_routes_reject_mutates_artifact_false(self) -> None: + mutating = ["preserve", "expand", "create", "redesign", "translate"] + for route in mutating: + with self.subTest(route=route): + cases = _base_routing_cases() + target = next(case for case in cases if case["expected_route"] == route) + target["mutates_artifact"] = False + message = self._expect_failure(cases) + self.assertIn(target["id"], message) + self.assertIn(route, message) + self.assertIn("mutates_artifact", message) + + def test_non_mutating_routes_reject_mutates_artifact_true(self) -> None: + non_mutating = ["critique", "brand-check", "profile"] + for route in non_mutating: + with self.subTest(route=route): + cases = _base_routing_cases() + target = next(case for case in cases if case["expected_route"] == route) + target["mutates_artifact"] = True + message = self._expect_failure(cases) + self.assertIn(target["id"], message) + self.assertIn(route, message) + self.assertIn("mutates_artifact", message) + + class GradedResultIntegrityTests(unittest.TestCase): """Issue #13: committed graded result cases must exactly match recomputed grading."""