diff --git a/scripts/tests/test_kitconfig.py b/scripts/tests/test_kitconfig.py index 109fef8..cba149e 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 @@ -341,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_followup_hook.py b/scripts/tests/test_pr_followup_hook.py index 23a956c..381c257 100644 --- a/scripts/tests/test_pr_followup_hook.py +++ b/scripts/tests/test_pr_followup_hook.py @@ -104,13 +104,72 @@ 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", + # 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() - 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() + + +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() @@ -122,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 253f85f..c437e41 100644 --- a/scripts/tests/test_pr_watch.py +++ b/scripts/tests/test_pr_watch.py @@ -12,13 +12,59 @@ ENGINE_DIR = Path(__file__).resolve().parent.parent -def _load_pr_watch() -> ModuleType: +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. + + 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 + 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(*, 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) + if pin_defaults: + _pin_engine_defaults(module) return module @@ -556,11 +602,22 @@ 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() +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. + # + # `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 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]) @@ -1656,10 +1713,118 @@ def test_configured_unavailable_marker_still_beats_configured_noise( assert pr_watch.is_noise(body) is False +def test_every_config_derived_global_is_pinned() -> None: + """``_pin_engine_defaults`` must overwrite EVERY config-derived global. + + 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: []``. + + So seed every global with a sentinel the defaults cannot equal, then pin. + 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" + + _pin_engine_defaults(pr_watch) + + 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 be wired into ``_load_pr_watch``, and + ``pin_defaults=False`` must genuinely leave the ambient config in place — + 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. + """ + 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 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 == 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: """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. 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" @@ -1676,8 +1841,18 @@ 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 + # `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) def test_normal_coderabbit_walkthrough_remains_noise() -> None: