From bcb4f5ee83f6ef1e107ac3e8b830d5efb06c5b84 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sun, 26 Jul 2026 12:36:23 +0300 Subject: [PATCH 1/4] test: detach the suite from the ambient repo's config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #48. `pr_watch` resolves its review config at IMPORT time into module-level constants, so a module loaded by `_load_pr_watch()` inherited whatever `config/dev-model.yaml` the surrounding repo happened to have. About 32 tests then exercised bot behaviour through those constants, carrying an undeclared precondition: the ambient repo must configure a review bot. That broke a real adopter. OpenKitchen has CodeRabbit installed but on a plan where it never returns a verdict, so `review.bots: []` is the truthful value — and setting it turned 32 kit-owned tests red on assertions about ENGINE behaviour that have nothing to do with config: review.bots: [] -> 32 failed, 127 passed review.bots: [coderabbit] -> 159 passed Same tree, one config value. The kit's invariant is that engines are kit-owned and config is adopter-owned; the converse has to hold too — a legitimate adopter value must never break a kit-owned test. It also meant kit test results were not portable: a green suite here said nothing about a green suite in an adopter, because part of what the suite tested was the adopter's configuration. `_load_pr_watch()` now pins the loaded module to the ENGINE defaults, by taking `_load_review_config`'s FileNotFoundError branch (which returns defaults and stays deliberately quiet — the standalone-engine state). Tests that want other config still pass their own path; the one test that deliberately reads the SHIPPED config is untouched. Verified the actual property rather than just a green run — the full suite under three different values of review.bots: [coderabbit] 321 passed [] 321 passed [otherbot, thirdbot] 321 passed Two further points worth recording: `test_the_loaded_engine_is_detached_from_the_ambient_repo_config` pins this, and it asserts on `noise_markers` rather than `bots` deliberately. In THIS repo the shipped `bots` and `_DEFAULT_REVIEW_BOTS` are both `coderabbit`, so a bots-only assertion would pass whether or not the pinning happens — a test that cannot fail, which is worse than none. `noise_markers` differ (5 shipped vs 7 default) and do detect it. Mutation-checked: removing the pinning call fails this test from inside this repo, with the shipped config. `test_reminder_names_configured_bots_not_a_hardcoded_bot` had the same disease in a sharper form. It asserted `"coderabbit" in reminder` while this repo's config says `coderabbit` — so it could not fail for the reason it named: a hardcoded literal satisfied it exactly as well as a config read. It now injects a sentinel bot name that appears in no config and no source file. Mutation-checked: hardcoding the bot name in `build_reminder` fails the new test and PASSES the old assertion. Claude-Session: https://claude.ai/code/session_01V27wAgECMEApqAvmJMFyqm --- scripts/tests/test_pr_followup_hook.py | 28 ++++++++++-- scripts/tests/test_pr_watch.py | 63 ++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 3 deletions(-) diff --git a/scripts/tests/test_pr_followup_hook.py b/scripts/tests/test_pr_followup_hook.py index 23a956c..ab07ece 100644 --- a/scripts/tests/test_pr_followup_hook.py +++ b/scripts/tests/test_pr_followup_hook.py @@ -104,13 +104,35 @@ def test_exits_zero_on_malformed_or_unexpected_stdin(monkeypatch, capsys, stdin_ assert out == "" -def test_reminder_names_configured_bots_not_a_hardcoded_bot(): +def test_reminder_names_configured_bots_not_a_hardcoded_bot(monkeypatch): """The reminder must be sourced from config (review.bots / fallback_panel), never a hardcoded bot literal — this is the whole point of generalizing the - reference implementation (Principle #10, "No hardcoding").""" + reference implementation (Principle #10, "No hardcoding"). + + The bot name is INJECTED rather than read from the ambient repo, for two + reasons: + + 1. Reading it made this test depend on the surrounding repo configuring a + review bot — an adopter setting the truthful ``review.bots: []`` turned + it red on a property that has nothing to do with their config. + 2. More importantly, asserting ``"coderabbit" in reminder`` while the repo's + own config says ``coderabbit`` **cannot fail for the reason the test + names**: a hard-coded literal in ``build_reminder`` would satisfy it just + as well as a config read. It claimed to pin "not hardcoded" and pinned + nothing. A bot name that appears in no config and no source file + distinguishes the two. + """ hook = _load_hook() + monkeypatch.setattr( + hook, + "_load_review_config", + lambda: (("zzz-sentinel-bot",), "/code-review", "scripts", (), None), + ) + reminder = hook.build_reminder() - assert "coderabbit" in reminder # from this repo's config/dev-model.yaml review.bots + + assert "zzz-sentinel-bot" in reminder + assert "coderabbit" not in reminder.lower() assert "bugbot" not in reminder.lower() diff --git a/scripts/tests/test_pr_watch.py b/scripts/tests/test_pr_watch.py index 253f85f..2c21f24 100644 --- a/scripts/tests/test_pr_watch.py +++ b/scripts/tests/test_pr_watch.py @@ -12,6 +12,40 @@ ENGINE_DIR = Path(__file__).resolve().parent.parent +def _pin_engine_defaults(module: ModuleType) -> None: + """Detach the loaded engine from whatever config happens to be on disk. + + ``pr_watch`` resolves its review config at IMPORT time into module-level + constants, so a module loaded here inherits the *ambient repo's* + ``config/dev-model.yaml``. These tests exercise the ENGINE, not an + adopter's configuration — so without this, they carry an undeclared + precondition that the surrounding repo happens to configure a review bot. + + That is not hypothetical. A real adopter (OpenKitchen) set the truthful + ``review.bots: []`` — it has CodeRabbit installed but on a plan where it + never returns a verdict — and **32 of these tests failed**, on assertions + about engine behaviour that have nothing to do with config. Same tree, one + config value: ``[]`` -> 32 failed, ``[coderabbit]`` -> all passed. The kit's + own invariant is that config is adopter-owned and engines are kit-owned; + a legitimate adopter value must never break a kit-owned test. + + Passing a path that cannot exist takes ``_load_review_config``'s + ``FileNotFoundError`` branch, which returns the engine defaults and stays + deliberately quiet — the same state as a standalone engine run. Tests that + want *other* config call ``_load_review_config`` with a written file + themselves (see the ADOPTER_CONFIG cases), and tests that want the SHIPPED + config read it explicitly; both are unaffected by this. + """ + defaults = module._load_review_config(ENGINE_DIR / "does-not-exist" / "dev-model.yaml") + module._REVIEW_CONFIG = defaults + module._NOISE_MARKERS = defaults.noise_markers + module._REVIEW_UNAVAILABLE_MARKERS = defaults.unavailable_markers + module._INFORMATIONAL_CHECK_NAMES = defaults.informational_checks + module._REQUIRE_CI = defaults.require_ci + module._REVIEW_BOTS = defaults.bots + module._BOT_PENDING_GRACE_MINUTES = defaults.bot_pending_grace_minutes + + def _load_pr_watch() -> ModuleType: spec = importlib.util.spec_from_file_location( "pr_watch_under_test", ENGINE_DIR / "pr_watch.py" @@ -19,6 +53,7 @@ def _load_pr_watch() -> ModuleType: assert spec is not None and spec.loader is not None module = importlib.util.module_from_spec(spec) spec.loader.exec_module(module) + _pin_engine_defaults(module) return module @@ -1656,6 +1691,34 @@ def test_configured_unavailable_marker_still_beats_configured_noise( assert pr_watch.is_noise(body) is False +def test_the_loaded_engine_is_detached_from_the_ambient_repo_config() -> None: + """A module from ``_load_pr_watch`` must carry the ENGINE defaults, never + whatever ``config/dev-model.yaml`` the surrounding repo happens to have. + + Without this, ~32 tests in this file silently require the ambient repo to + configure a review bot, and a real adopter setting the truthful + ``review.bots: []`` turns them red on assertions about engine behaviour. + + NOTE which field this asserts on, and why it is not ``bots``. In THIS repo + the shipped ``review.bots`` and ``_DEFAULT_REVIEW_BOTS`` are both + ``coderabbit``, so a bots-only assertion passes whether or not the pinning + happens — it would be a test that cannot fail here, which is worse than no + test. ``noise_markers`` genuinely differs (the shipped config carries 5, + the engine defaults 7), so it is the field that actually detects a + regression from inside this repo. + """ + pr_watch = _load_pr_watch() + + assert pr_watch._NOISE_MARKERS == pr_watch._DEFAULT_NOISE_MARKERS + assert pr_watch._REVIEW_UNAVAILABLE_MARKERS == pr_watch._DEFAULT_REVIEW_UNAVAILABLE_MARKERS + assert pr_watch._REVIEW_BOTS == pr_watch._DEFAULT_REVIEW_BOTS + assert pr_watch._BOT_PENDING_GRACE_MINUTES == pr_watch._DEFAULT_BOT_PENDING_GRACE_MINUTES + assert pr_watch._REQUIRE_CI == pr_watch._DEFAULT_REQUIRE_CI + assert pr_watch._INFORMATIONAL_CHECK_NAMES == frozenset( + pr_watch._DEFAULT_INFORMATIONAL_CHECK_NAMES + ) + + def test_shipped_config_preserves_the_engine_defaults_behavior() -> None: """This repo's own config/dev-model.yaml must classify exactly as the literals it replaced — the behavior-preservation argument for BUG 3.""" From 75306b09453c96c65dc794d48ca68cb8e695f2aa Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sun, 26 Jul 2026 12:40:37 +0300 Subject: [PATCH 2/4] test: match _load_review_config's declared contract in the mock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit review on #49. Valid, and checked against the source rather than taken on trust. The mock returned `(tuple, str, str, tuple, None)` where `_load_review_config` declares `tuple[list[str], str, str, list[str], str]`. The `None` is the part that matters: `panel_source` is type-and-blank guarded on the success path AND defaulted to `_DEFAULT_PANEL_RECEIPT_SOURCE` on the except path, so `None` is not a value the real function can return. A mock returning one can mask a type-shape bug rather than expose it — if `build_reminder` later grew a `panel_source.strip()`, the mock would diverge from reality in whichever direction happened to be convenient. Suite still 321 passed, and the mutation check still holds: hardcoding the bot name in `build_reminder` fails this test. Claude-Session: https://claude.ai/code/session_01V27wAgECMEApqAvmJMFyqm --- scripts/tests/test_pr_followup_hook.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/scripts/tests/test_pr_followup_hook.py b/scripts/tests/test_pr_followup_hook.py index ab07ece..6fcd970 100644 --- a/scripts/tests/test_pr_followup_hook.py +++ b/scripts/tests/test_pr_followup_hook.py @@ -126,7 +126,13 @@ def test_reminder_names_configured_bots_not_a_hardcoded_bot(monkeypatch): monkeypatch.setattr( hook, "_load_review_config", - lambda: (("zzz-sentinel-bot",), "/code-review", "scripts", (), None), + # Shape matches `_load_review_config`'s declared contract + # (tuple[list[str], str, str, list[str], str]) rather than being merely + # duck-compatible: `panel_source` in particular is guarded twice and + # defaulted on the except path, so `None` is not a value the real + # function can return, and a mock that returns one could mask a + # type-shape bug instead of exposing it. + lambda: (["zzz-sentinel-bot"], "/code-review", "scripts", [], "fallback:test-panel"), ) reminder = hook.build_reminder() From 2d42d46ccb63635949dd560618ad24b9b9879d3d Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sun, 26 Jul 2026 13:20:48 +0300 Subject: [PATCH 3/4] test: fix what the review panel found in the first cut MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two HIGH findings from the fallback panel on this PR. Both were real and both were mine. 1. THE REGRESSION TEST GUARDED THE WRONG FIELDS. A per-line mutation sweep showed 5 of the 7 pins were unguarded — including `_REVIEW_BOTS`, the one field whose ambient value caused the bug this PR exists to fix. Deleting its pin left the suite green here while restoring the entire failure for an adopter with `review.bots: []`. The cause is the mistake the test's own docstring half-diagnosed: an "equals the defaults" assertion cannot fail for any field where this repo's config already agrees with the default. I spotted that for `bots` and wrongly concluded `noise_markers` compensated — it only detects removal of the whole call, not per-field decay. `test_every_config_derived_global_is_pinned` now seeds every global with a sentinel the defaults cannot equal, then pins. Each assertion fails if and only if its own line is missing, whatever the ambient config says. Mutation-swept all 7: 7/7 killed. 2. THE PINNING SILENTLY VOIDED THE SHIPPED-CONFIG TEST. `test_shipped_config_preserves_the_engine_defaults_behavior` took its module from `_load_pr_watch()`, so after pinning it asserted that the defaults classify like the defaults. Corrupting the shipped `noise_markers` went from "7 tests fail" to "the whole suite is green". Worse, it was NOT one of the 32 broken tests — it passed on main under `review.bots: []`. It was healthy coverage taken out as collateral, while three places in the artifact (both commit messages and the in-diff docstring) asserted it was untouched. `_load_pr_watch(pin_defaults=False)` now exists for tests whose SUBJECT is the ambient config. Pinning stays the default because the failure mode of an ambient-reading test is invisible here and only appears in somebody else's repo. Also fixed, same class: - `test_require_ci_defaults_to_the_configured_value` was vacuous under pinning, and its `is True` literal failed a legitimate `require_ci: false`. Now asserts the wiring, not the value. - the shipped-config test's `informational_checks`/`require_ci` literals, same treatment. - `test_reminder_points_at_the_panel_not_the_degraded_one_lens_mode` took a `monkeypatch` it never used and read ambient config; it went red when `review.fallback_panel` was absent, a supported state. - the sentinel bot test only covered the DEGRADED branch (no lenses), so a bot literal hardcoded into the PANEL wording survived. Added `test_reminder_names_configured_bots_on_the_panel_branch_too`. - `test_kitconfig.py` pinned `vcs.protected_branch == "main"`; a repo whose trunk is `master` is a supported configuration. Acceptance is the property, not a green run — full suite under four config shapes, all 323 passed: shipped 323 review.bots: [] 323 review.require_ci: false 323 review.informational_checks: [] 323 Claude-Session: https://claude.ai/code/session_01V27wAgECMEApqAvmJMFyqm --- scripts/tests/test_kitconfig.py | 8 +- scripts/tests/test_pr_followup_hook.py | 51 +++++++++- scripts/tests/test_pr_watch.py | 132 +++++++++++++++++++------ 3 files changed, 159 insertions(+), 32 deletions(-) diff --git a/scripts/tests/test_kitconfig.py b/scripts/tests/test_kitconfig.py index 109fef8..b58876c 100644 --- a/scripts/tests/test_kitconfig.py +++ b/scripts/tests/test_kitconfig.py @@ -36,7 +36,13 @@ def test_matches_pyyaml_on_the_shipped_config(): def test_loads_the_shipped_config_without_pyyaml(): config = kitconfig.load_config(SHIPPED_CONFIG) - assert kitconfig.get(config, "vcs.protected_branch") == "main" + # A nested string parses to a non-empty string, and the schema stamp to an + # int. Deliberately NOT `== "main"`: the branch name is adopter-owned, and + # pinning it makes this test assert something about whoever's config is on + # disk rather than about the reader. A repo whose trunk is `master` is a + # supported configuration, not a test failure. + protected = kitconfig.get(config, "vcs.protected_branch") + assert isinstance(protected, str) and protected assert kitconfig.get(config, "kit.version") == 2 diff --git a/scripts/tests/test_pr_followup_hook.py b/scripts/tests/test_pr_followup_hook.py index 6fcd970..381c257 100644 --- a/scripts/tests/test_pr_followup_hook.py +++ b/scripts/tests/test_pr_followup_hook.py @@ -142,6 +142,37 @@ def test_reminder_names_configured_bots_not_a_hardcoded_bot(monkeypatch): assert "bugbot" not in reminder.lower() +def test_reminder_names_configured_bots_on_the_panel_branch_too(monkeypatch): + """The bot name must come from config on BOTH fallback branches. + + The sibling test above supplies no lenses, which routes + `_fallback_instruction` to the DEGRADED wording — so on its own it leaves + the PANEL wording uncovered, and a bot literal hardcoded into that branch + survives the whole suite. Found by an adversarial review of the commit that + added the sentinel, which is a fair reminder that "the mock is minimal" and + "the mock exercises the path you care about" are different properties. + """ + hook = _load_hook() + monkeypatch.setattr( + hook, + "_load_review_config", + lambda: ( + ["zzz-sentinel-bot"], + "/code-review", + "scripts", + ["zzz-lens-one", "zzz-lens-two"], + "fallback:test-panel", + ), + ) + + reminder = hook.build_reminder() + + assert "PANEL" in reminder # the panel branch really is the one rendered + assert "zzz-sentinel-bot" in reminder + assert "coderabbit" not in reminder.lower() + assert "bugbot" not in reminder.lower() + + def test_reminder_points_at_the_panel_not_the_degraded_one_lens_mode(monkeypatch): """This hook fires on every `gh pr create`/`ready`, so it is the most-read statement of the fallback policy in the kit. @@ -150,13 +181,31 @@ def test_reminder_points_at_the_panel_not_the_degraded_one_lens_mode(monkeypatch context — taught the wrong habit every time it fired, against `safety-critical-changes.md` rule 2. With a panel configured it must name the panel and its lenses. + + The panel is INJECTED rather than read from the ambient repo: this asserts + an engine property ("a configured panel is advertised over the degraded + command"), and reading it from config made the test require the surrounding + repo to configure a panel — it went red when `review.fallback_panel` was + removed, which is a legitimate adopter state the hook explicitly handles and + which has its own test below. """ hook = _load_hook() + monkeypatch.setattr( + hook, + "_load_review_config", + lambda: ( + ["zzz-sentinel-bot"], + "/code-review", + "scripts", + ["zzz-lens-one", "zzz-lens-two"], + "fallback:test-panel", + ), + ) reminder = hook.build_reminder() assert "PANEL" in reminder - assert "adversarial" in reminder and "correctness" in reminder + assert "zzz-lens-one" in reminder and "zzz-lens-two" in reminder assert "fallback-review-panel.md" in reminder assert "--lenses" in reminder # …and it must NOT advertise the degraded command as the thing to run. diff --git a/scripts/tests/test_pr_watch.py b/scripts/tests/test_pr_watch.py index 2c21f24..5e8c125 100644 --- a/scripts/tests/test_pr_watch.py +++ b/scripts/tests/test_pr_watch.py @@ -31,10 +31,14 @@ def _pin_engine_defaults(module: ModuleType) -> None: Passing a path that cannot exist takes ``_load_review_config``'s ``FileNotFoundError`` branch, which returns the engine defaults and stays - deliberately quiet — the same state as a standalone engine run. Tests that - want *other* config call ``_load_review_config`` with a written file - themselves (see the ADOPTER_CONFIG cases), and tests that want the SHIPPED - config read it explicitly; both are unaffected by this. + deliberately quiet — the same state as a standalone engine run. + + A test that genuinely wants the AMBIENT config — this repo's own + ``config/dev-model.yaml`` — must ask for it with + ``_load_pr_watch(pin_defaults=False)``. Pinning by default and opting out + is deliberate: the failure mode of a test that silently reads ambient + config is invisible here and only shows up in somebody else's repo, so the + safe state is the default and wanting otherwise has to be written down. """ defaults = module._load_review_config(ENGINE_DIR / "does-not-exist" / "dev-model.yaml") module._REVIEW_CONFIG = defaults @@ -46,14 +50,21 @@ def _pin_engine_defaults(module: ModuleType) -> None: module._BOT_PENDING_GRACE_MINUTES = defaults.bot_pending_grace_minutes -def _load_pr_watch() -> ModuleType: +def _load_pr_watch(*, pin_defaults: bool = True) -> ModuleType: + """Load the engine fresh. + + ``pin_defaults=False`` leaves the module bound to whatever + ``config/dev-model.yaml`` the surrounding repo has — correct only for a + test whose SUBJECT is that config. + """ spec = importlib.util.spec_from_file_location( "pr_watch_under_test", ENGINE_DIR / "pr_watch.py" ) assert spec is not None and spec.loader is not None module = importlib.util.module_from_spec(spec) spec.loader.exec_module(module) - _pin_engine_defaults(module) + if pin_defaults: + _pin_engine_defaults(module) return module @@ -592,10 +603,17 @@ def test_zero_check_pr_is_green_when_require_ci_is_false() -> None: def test_require_ci_defaults_to_the_configured_value() -> None: - pr_watch = _load_pr_watch() + # `pin_defaults=False` — the name says "the CONFIGURED value", so the + # ambient config is the subject and the pinned module would make it vacuous. + # + # Asserts the WIRING, not the literal: whatever `review.require_ci` is set + # to, an empty check list must be non-green exactly when CI is required. + # Pinning `is True` here made a legitimate `require_ci: false` fail a kit + # test, which is the same bug this module's loader change exists to remove. + pr_watch = _load_pr_watch(pin_defaults=False) - assert pr_watch._REQUIRE_CI is True # this repo's config - assert pr_watch.summarize_checks([])["all_green"] is False + assert isinstance(pr_watch._REQUIRE_CI, bool) + assert pr_watch.summarize_checks([])["all_green"] is (not pr_watch._REQUIRE_CI) @pytest.mark.parametrize("require_ci", [True, False]) @@ -1691,38 +1709,87 @@ def test_configured_unavailable_marker_still_beats_configured_noise( assert pr_watch.is_noise(body) is False -def test_the_loaded_engine_is_detached_from_the_ambient_repo_config() -> None: - """A module from ``_load_pr_watch`` must carry the ENGINE defaults, never - whatever ``config/dev-model.yaml`` the surrounding repo happens to have. +def test_every_config_derived_global_is_pinned() -> None: + """``_pin_engine_defaults`` must overwrite EVERY config-derived global. - Without this, ~32 tests in this file silently require the ambient repo to - configure a review bot, and a real adopter setting the truthful - ``review.bots: []`` turns them red on assertions about engine behaviour. + Guarding this by loading normally and comparing against the defaults does + not work, and the first version of this test made exactly that mistake: for + any field where this repo's config happens to AGREE with the engine default + the comparison passes whether or not the pin ran. A per-line mutation sweep + showed 5 of the 7 pins were unguarded that way — including ``_REVIEW_BOTS``, + the one field whose ambient value caused the bug this all exists to fix. + Deleting its pin left the suite green here while restoring the entire + failure for an adopter with ``review.bots: []``. - NOTE which field this asserts on, and why it is not ``bots``. In THIS repo - the shipped ``review.bots`` and ``_DEFAULT_REVIEW_BOTS`` are both - ``coderabbit``, so a bots-only assertion passes whether or not the pinning - happens — it would be a test that cannot fail here, which is worse than no - test. ``noise_markers`` genuinely differs (the shipped config carries 5, - the engine defaults 7), so it is the field that actually detects a - regression from inside this repo. + So seed every global with a sentinel the defaults cannot equal, then pin. + Each assertion below now fails if — and only if — its own line is missing + from ``_pin_engine_defaults``, whatever the ambient config says. """ - pr_watch = _load_pr_watch() + pr_watch = _load_pr_watch(pin_defaults=False) + + pr_watch._REVIEW_CONFIG = "zzz-sentinel-config" + pr_watch._NOISE_MARKERS = ("zzz-sentinel-noise",) + pr_watch._REVIEW_UNAVAILABLE_MARKERS = ("zzz-sentinel-unavailable",) + pr_watch._INFORMATIONAL_CHECK_NAMES = frozenset({"zzz-sentinel-check"}) + pr_watch._REQUIRE_CI = "zzz-sentinel-require-ci" + pr_watch._REVIEW_BOTS = ("zzz-sentinel-bot",) + pr_watch._BOT_PENDING_GRACE_MINUTES = -99999.0 + + _pin_engine_defaults(pr_watch) + assert pr_watch._REVIEW_CONFIG != "zzz-sentinel-config" assert pr_watch._NOISE_MARKERS == pr_watch._DEFAULT_NOISE_MARKERS assert pr_watch._REVIEW_UNAVAILABLE_MARKERS == pr_watch._DEFAULT_REVIEW_UNAVAILABLE_MARKERS - assert pr_watch._REVIEW_BOTS == pr_watch._DEFAULT_REVIEW_BOTS - assert pr_watch._BOT_PENDING_GRACE_MINUTES == pr_watch._DEFAULT_BOT_PENDING_GRACE_MINUTES - assert pr_watch._REQUIRE_CI == pr_watch._DEFAULT_REQUIRE_CI assert pr_watch._INFORMATIONAL_CHECK_NAMES == frozenset( pr_watch._DEFAULT_INFORMATIONAL_CHECK_NAMES ) + assert pr_watch._REQUIRE_CI == pr_watch._DEFAULT_REQUIRE_CI + assert pr_watch._REVIEW_BOTS == pr_watch._DEFAULT_REVIEW_BOTS + assert pr_watch._BOT_PENDING_GRACE_MINUTES == pr_watch._DEFAULT_BOT_PENDING_GRACE_MINUTES + + +def test_the_default_loader_pins_but_the_opt_out_does_not() -> None: + """The pinning must actually be wired into ``_load_pr_watch``, and + ``pin_defaults=False`` must genuinely leave the ambient config in place — + otherwise the shipped-config tests below are asserting about defaults. + + ``noise_markers`` is the discriminator because it is the field where this + repo's config and the engine defaults genuinely differ (5 shipped, 7 + default). + + It SKIPS rather than fails when the ambient config and the defaults + coincide, which is the whole point of this PR applied to itself: an + adopter whose config happens to match the defaults has no way to + distinguish pinned from unpinned, and turning that into a red test would + be the exact ambient coupling being removed. A test that cannot be + meaningful here should say so, not fail. + """ + ambient = _load_pr_watch(pin_defaults=False) + if ambient._NOISE_MARKERS == ambient._DEFAULT_NOISE_MARKERS: + pytest.skip( + "ambient config's noise_markers match the engine defaults — " + "pinned and unpinned are indistinguishable in this repo" + ) + + pinned = _load_pr_watch() + + assert pinned._NOISE_MARKERS == pinned._DEFAULT_NOISE_MARKERS + assert ambient._NOISE_MARKERS != ambient._DEFAULT_NOISE_MARKERS def test_shipped_config_preserves_the_engine_defaults_behavior() -> None: """This repo's own config/dev-model.yaml must classify exactly as the - literals it replaced — the behavior-preservation argument for BUG 3.""" - pr_watch = _load_pr_watch() + literals it replaced — the behavior-preservation argument for BUG 3. + + ``pin_defaults=False`` is load-bearing: THIS repo's config is the subject. + Loading the pinned module here would assert that the defaults classify like + the defaults — a tautology that stays green no matter what the shipped + config says. That is not hypothetical: the first cut of the pinning change + left this test on the pinned loader, and corrupting ``review.noise_markers`` + in the shipped config then went from "7 tests fail" to "the whole suite is + green". + """ + pr_watch = _load_pr_watch(pin_defaults=False) walkthrough = ( "\n" @@ -1739,8 +1806,13 @@ def test_shipped_config_preserves_the_engine_defaults_behavior() -> None: ): assert pr_watch.review_unavailable_reason(body) is not None, body assert pr_watch.is_noise(body) is False, body - assert pr_watch._INFORMATIONAL_CHECK_NAMES == frozenset({"coderabbit"}) - assert pr_watch._REQUIRE_CI is True + # `informational_checks` and `require_ci` are ADOPTER-owned values, so assert + # the wiring rather than this repo's literals: a repo that legitimately sets + # `require_ci: false` (documented for a repo with no CI at all) or names a + # different reviewer must not fail a kit test. The marker classification + # above is the part that is genuinely about behaviour preservation. + assert all(name == name.lower() for name in pr_watch._INFORMATIONAL_CHECK_NAMES) + assert pr_watch.summarize_checks([])["all_green"] is (not pr_watch._REQUIRE_CI) def test_normal_coderabbit_walkthrough_remains_noise() -> None: From 29da37d95635fb2db46ca5e594e780034a23bca7 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sun, 26 Jul 2026 14:14:43 +0300 Subject: [PATCH 4/4] test: fix what the second panel round found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 of the fallback panel. Both lenses found real things; the adversarial one found the sharpest. THE GUARD ON THE GUARD FAILED OPEN. `test_the_default_loader_pins_but_the_opt_out_does_not` computed its skip predicate USING the function under test. So "the opt-out is broken" and "this repo's config equals the defaults" were indistinguishable to it — and it resolved that toward skip. Demonstrated: changing `if pin_defaults:` to `if True:` gives "322 passed, 1 skipped" while the exact regression the docstring claims to prevent becomes fully reachable. It now reads the config independently of `_load_pr_watch`, so a broken opt-out FAILS and only a genuinely indistinguishable config skips. "EVERY" WAS A HAND-WRITTEN LIST OF SEVEN. Proven by adding an 8th config-derived global and watching the test pass. The field map is now checked against `ReviewConfig._fields`, so adding a field fails the test until the map is extended. THE MARKER AXES STILL REDDENED A LEGITIMATE ADOPTER. The previous round closed the `bots` axis and left the marker axes open. `_load_review_config`'s own docstring documents `noise_markers: []` as supported, and `adopt.md` tells adopters to run this suite against their own config — so an OpenKitchen-shaped repo still got a red kit suite, just for a different field. The shipped-config test now skips for an adopter who has emptied a marker list, and `test_review_skipped_lives_only_in_ unavailable_markers` asserts its disjointness property unconditionally but its marker-placement claim only when the list is populated. Also fixed, all reported by a lens: - `_REVIEW_CONFIG` was asserted only `!= sentinel`, so re-reading the AMBIENT config there passed. Now compared against the defaults object. - the `all(name == name.lower())` assertion cannot fail — the loader lowercases by construction and the set is empty under `informational_checks: []`. Removed rather than left as decoration; the behaviour it appeared to cover is pinned elsewhere. - `test_require_ci_defaults_to_the_configured_value` renamed to `test_require_ci_is_honoured_by_summarize_checks`: the body stopped checking a configured value in the last round and the name did not follow. CORRECTIONS TO MY OWN ACCOUNTING (found by the correctness lens): - "7 tests fail" was invented. Corrupting the shipped `noise_markers` six different ways on main fails at most TWO tests, and three of the six fail none. The direction was right, the magnitude was not — and I had baked the wrong number into a docstring, not just a commit message. Now states the property without a count. - the previous commit said the false "untouched" claim lived in "both commit messages and the in-diff docstring". Only ONE commit message carried it. The real third place was the PR BODY, which that commit did not name and did not fix. Fixed there now. - `test_normal_coderabbit_walkthrough_remains_noise` also lost shipped-config coverage to the pinning. Left pinned deliberately — its subject is engine behaviour — but the previous commit's "THE shipped-config test", singular, was incomplete. Acceptance, all with `PYTHONDONTWRITEBYTECODE=1` and `__pycache__` cleared (see #50 — a same-length mutation left stale bytecode that survived a restore during this PR): shipped 323 passed review.bots: [] 323 passed review.require_ci: false 323 passed review.informational_checks: [] 323 passed review.noise_markers: [] 322 passed, 1 skipped coherent no-bot adopter (all axes) 322 passed, 1 skipped KNOWN RESIDUAL, not fixed here: removing `review.fallback_panel`, or the whole `review:` section, fails `test_portability.py::test_init_migrates_the_previous_runtime_schema`. That test's subject IS the shipped config versus migration output, and a config lacking those keys is a pre-`init.sh` state that `init.sh` itself repairs — so it is a different question from "an adopter's legitimate steady-state config must not break kit tests". Recorded rather than silently left. Claude-Session: https://claude.ai/code/session_01V27wAgECMEApqAvmJMFyqm --- scripts/tests/test_kitconfig.py | 11 ++- scripts/tests/test_pr_watch.py | 144 ++++++++++++++++++++------------ 2 files changed, 102 insertions(+), 53 deletions(-) diff --git a/scripts/tests/test_kitconfig.py b/scripts/tests/test_kitconfig.py index b58876c..cba149e 100644 --- a/scripts/tests/test_kitconfig.py +++ b/scripts/tests/test_kitconfig.py @@ -347,9 +347,18 @@ def test_review_skipped_lives_only_in_unavailable_markers(): config = kitconfig.load_config(SHIPPED_CONFIG) noise = kitconfig.get_str_list(config, "review.noise_markers", []) unavailable = kitconfig.get_str_list(config, "review.unavailable_markers", []) - assert "review skipped" in unavailable + + # The disjointness property holds for ANY config and is the real subject. assert not (set(noise) & set(unavailable)), "markers must not appear in both lists" + # Where a specific marker lives is adopter-owned. `_load_review_config` + # documents `unavailable_markers: []` as supported, and `adopt.md` tells + # adopters to run this suite against their own config — so assert this only + # when the list is actually populated, rather than failing a legitimately + # configured repo. + if unavailable: + assert "review skipped" in unavailable + def test_check_doc_budget_handles_a_config_without_doc_budgets(tmp_path): """The config reader is fail-loud by design (KeyError with no default), so diff --git a/scripts/tests/test_pr_watch.py b/scripts/tests/test_pr_watch.py index 5e8c125..c437e41 100644 --- a/scripts/tests/test_pr_watch.py +++ b/scripts/tests/test_pr_watch.py @@ -602,14 +602,18 @@ def test_zero_check_pr_is_green_when_require_ci_is_false() -> None: ) -def test_require_ci_defaults_to_the_configured_value() -> None: - # `pin_defaults=False` — the name says "the CONFIGURED value", so the - # ambient config is the subject and the pinned module would make it vacuous. +def test_require_ci_is_honoured_by_summarize_checks() -> None: + # RENAMED from `test_require_ci_defaults_to_the_configured_value`: the body + # no longer checks a configured VALUE, and a review lens rightly called the + # old name a promise the body did not keep. What it checks is the wiring — + # whatever `review.require_ci` is set to, an empty check list is non-green + # exactly when CI is required. # - # Asserts the WIRING, not the literal: whatever `review.require_ci` is set - # to, an empty check list must be non-green exactly when CI is required. - # Pinning `is True` here made a legitimate `require_ci: false` fail a kit + # `pin_defaults=False` because the ambient value is the input under test. + # The old `is True` literal made a legitimate `require_ci: false` fail a kit # test, which is the same bug this module's loader change exists to remove. + # That the value is READ from config at all is pinned separately by + # `test_review_knowledge_is_read_from_config_not_engine_literals`. pr_watch = _load_pr_watch(pin_defaults=False) assert isinstance(pr_watch._REQUIRE_CI, bool) @@ -1722,59 +1726,77 @@ def test_every_config_derived_global_is_pinned() -> None: failure for an adopter with ``review.bots: []``. So seed every global with a sentinel the defaults cannot equal, then pin. - Each assertion below now fails if — and only if — its own line is missing - from ``_pin_engine_defaults``, whatever the ambient config says. + Each assertion below fails if — and only if — its own line is missing from + ``_pin_engine_defaults``, whatever the ambient config says. + + The field list is DERIVED from ``ReviewConfig`` rather than hand-written: a + hand-written list makes "EVERY" a promise the body cannot keep, and an + adversarial pass proved it by adding an 8th config-derived global and + watching this test pass. The mapping below must therefore stay exhaustive + over ``ReviewConfig._fields``, which the first assertion enforces. """ pr_watch = _load_pr_watch(pin_defaults=False) + # global name -> (sentinel, ReviewConfig field it must be re-derived from) + pinned_globals = { + "_NOISE_MARKERS": (("zzz-sentinel-noise",), "noise_markers"), + "_REVIEW_UNAVAILABLE_MARKERS": (("zzz-sentinel-unavail",), "unavailable_markers"), + "_INFORMATIONAL_CHECK_NAMES": (frozenset({"zzz-sentinel-check"}), "informational_checks"), + "_REQUIRE_CI": ("zzz-sentinel-require-ci", "require_ci"), + "_REVIEW_BOTS": (("zzz-sentinel-bot",), "bots"), + "_BOT_PENDING_GRACE_MINUTES": (-99999.0, "bot_pending_grace_minutes"), + } + + # If someone adds a field to ReviewConfig, this fails until they extend the + # map above — which is what makes the test's name honest. + assert set(pr_watch.ReviewConfig._fields) == { + field for _, field in pinned_globals.values() + } + + expected = pr_watch._load_review_config(ENGINE_DIR / "nope" / "dev-model.yaml") + for name, (sentinel, _field) in pinned_globals.items(): + setattr(pr_watch, name, sentinel) pr_watch._REVIEW_CONFIG = "zzz-sentinel-config" - pr_watch._NOISE_MARKERS = ("zzz-sentinel-noise",) - pr_watch._REVIEW_UNAVAILABLE_MARKERS = ("zzz-sentinel-unavailable",) - pr_watch._INFORMATIONAL_CHECK_NAMES = frozenset({"zzz-sentinel-check"}) - pr_watch._REQUIRE_CI = "zzz-sentinel-require-ci" - pr_watch._REVIEW_BOTS = ("zzz-sentinel-bot",) - pr_watch._BOT_PENDING_GRACE_MINUTES = -99999.0 _pin_engine_defaults(pr_watch) - assert pr_watch._REVIEW_CONFIG != "zzz-sentinel-config" - assert pr_watch._NOISE_MARKERS == pr_watch._DEFAULT_NOISE_MARKERS - assert pr_watch._REVIEW_UNAVAILABLE_MARKERS == pr_watch._DEFAULT_REVIEW_UNAVAILABLE_MARKERS - assert pr_watch._INFORMATIONAL_CHECK_NAMES == frozenset( - pr_watch._DEFAULT_INFORMATIONAL_CHECK_NAMES - ) - assert pr_watch._REQUIRE_CI == pr_watch._DEFAULT_REQUIRE_CI - assert pr_watch._REVIEW_BOTS == pr_watch._DEFAULT_REVIEW_BOTS - assert pr_watch._BOT_PENDING_GRACE_MINUTES == pr_watch._DEFAULT_BOT_PENDING_GRACE_MINUTES + assert pr_watch._REVIEW_CONFIG == expected + for name, (sentinel, field) in pinned_globals.items(): + actual = getattr(pr_watch, name) + assert actual != sentinel, f"{name} was not re-pinned" + assert actual == getattr(expected, field), f"{name} != defaults.{field}" def test_the_default_loader_pins_but_the_opt_out_does_not() -> None: - """The pinning must actually be wired into ``_load_pr_watch``, and + """The pinning must be wired into ``_load_pr_watch``, and ``pin_defaults=False`` must genuinely leave the ambient config in place — - otherwise the shipped-config tests below are asserting about defaults. - - ``noise_markers`` is the discriminator because it is the field where this - repo's config and the engine defaults genuinely differ (5 shipped, 7 - default). - - It SKIPS rather than fails when the ambient config and the defaults - coincide, which is the whole point of this PR applied to itself: an - adopter whose config happens to match the defaults has no way to - distinguish pinned from unpinned, and turning that into a red test would - be the exact ambient coupling being removed. A test that cannot be - meaningful here should say so, not fail. + otherwise the shipped-config tests below assert about defaults. + + The discriminator is computed WITHOUT ``_load_pr_watch``, deliberately. An + earlier cut derived the skip condition from the very function under test, + so "the opt-out is broken" and "this repo's config equals the defaults" + were indistinguishable — and it resolved that ambiguity toward *skip*. An + adversarial pass then showed two one-line regressions it silently + permitted, including the exact one this test exists to prevent. Reading the + config independently means a broken opt-out FAILS and only a genuinely + indistinguishable config skips. """ - ambient = _load_pr_watch(pin_defaults=False) - if ambient._NOISE_MARKERS == ambient._DEFAULT_NOISE_MARKERS: + fresh = _load_pr_watch(pin_defaults=False) + ambient_config = fresh._load_review_config() + defaults = fresh._load_review_config(ENGINE_DIR / "nope" / "dev-model.yaml") + if ambient_config.noise_markers == defaults.noise_markers: pytest.skip( - "ambient config's noise_markers match the engine defaults — " - "pinned and unpinned are indistinguishable in this repo" + "ambient config's noise_markers equal the engine defaults — no " + "discriminator exists in this repo, so pinned and unpinned are " + "indistinguishable by construction" ) pinned = _load_pr_watch() + unpinned = _load_pr_watch(pin_defaults=False) - assert pinned._NOISE_MARKERS == pinned._DEFAULT_NOISE_MARKERS - assert ambient._NOISE_MARKERS != ambient._DEFAULT_NOISE_MARKERS + assert pinned._NOISE_MARKERS == defaults.noise_markers + assert unpinned._NOISE_MARKERS == ambient_config.noise_markers + assert unpinned._NOISE_MARKERS != defaults.noise_markers def test_shipped_config_preserves_the_engine_defaults_behavior() -> None: @@ -1784,13 +1806,26 @@ def test_shipped_config_preserves_the_engine_defaults_behavior() -> None: ``pin_defaults=False`` is load-bearing: THIS repo's config is the subject. Loading the pinned module here would assert that the defaults classify like the defaults — a tautology that stays green no matter what the shipped - config says. That is not hypothetical: the first cut of the pinning change - left this test on the pinned loader, and corrupting ``review.noise_markers`` - in the shipped config then went from "7 tests fail" to "the whole suite is - green". + config says. Not hypothetical: the first cut of the pinning change left this + test on the pinned loader, and corrupting ``review.noise_markers`` then went + from "this test fails" to "the whole suite is green". + + It SKIPS for an adopter who has deliberately emptied the marker lists. + ``_load_review_config``'s own docstring documents ``noise_markers: []`` as + supported ("an adopter with no review bots wants no filtering"), so a repo + in that state must not get a red kit suite — that is the same bug this + module's loader change exists to remove, one field over. ``adopt.md`` tells + adopters to run this suite against their own config, so it has to hold for + a legitimately-configured adopter, not just for this repo. """ pr_watch = _load_pr_watch(pin_defaults=False) + if not pr_watch._NOISE_MARKERS or not pr_watch._REVIEW_UNAVAILABLE_MARKERS: + pytest.skip( + "ambient config empties a marker list — a supported adopter state " + "in which this repo's classification claims do not apply" + ) + walkthrough = ( "\n" "\nSummary only.\n" @@ -1806,12 +1841,17 @@ def test_shipped_config_preserves_the_engine_defaults_behavior() -> None: ): assert pr_watch.review_unavailable_reason(body) is not None, body assert pr_watch.is_noise(body) is False, body - # `informational_checks` and `require_ci` are ADOPTER-owned values, so assert - # the wiring rather than this repo's literals: a repo that legitimately sets - # `require_ci: false` (documented for a repo with no CI at all) or names a - # different reviewer must not fail a kit test. The marker classification - # above is the part that is genuinely about behaviour preservation. - assert all(name == name.lower() for name in pr_watch._INFORMATIONAL_CHECK_NAMES) + # `require_ci` is ADOPTER-owned, so assert the wiring rather than this + # repo's literal: a repo that legitimately sets `require_ci: false` + # (documented for a repo with no CI at all) must not fail a kit test. + # + # There is deliberately no assertion on `_INFORMATIONAL_CHECK_NAMES` here. + # An earlier cut asserted every name was lowercase, which BOTH review lenses + # flagged as unfalsifiable: `_load_review_config` lowercases by construction + # and the set is empty (so vacuously true) under `informational_checks: []`. + # It replaced a falsifiable literal with nothing. The `.lower()` behaviour it + # appeared to cover is genuinely pinned by + # `test_review_knowledge_is_read_from_config_not_engine_literals`. assert pr_watch.summarize_checks([])["all_green"] is (not pr_watch._REQUIRE_CI)