test: detach the suite from the ambient repo's config (closes #48)#49
Conversation
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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughTests now isolate review behavior from ambient repository configuration by pinning dynamically loaded ChangesConfiguration-isolated test coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/tests/test_pr_followup_hook.py (1)
126-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the mocked loader result within its declared contract.
scripts/hooks/pr_followup_hook.py:48-102returns lists and a stringpanel_source, but this fixture supplies tuples andNone. Use valid list/string placeholders so future reminder changes cannot fail this bot-isolation test for unrelated type-shape reasons.Proposed fix
- lambda: (("zzz-sentinel-bot",), "/code-review", "scripts", (), None), + lambda: (["zzz-sentinel-bot"], "/code-review", "scripts", [], "test-source"),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/test_pr_followup_hook.py` around lines 126 - 130, Update the _load_review_config mock in the test to return the declared contract shape: lists for the bot and reviewer collections and a string placeholder for panel_source, while preserving the existing sentinel bot and other fixture values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/tests/test_pr_followup_hook.py`:
- Around line 126-130: Update the _load_review_config mock in the test to return
the declared contract shape: lists for the bot and reviewer collections and a
string placeholder for panel_source, while preserving the existing sentinel bot
and other fixture values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c86bf7c-c116-4c38-a099-b36b92af082d
📒 Files selected for processing (2)
scripts/tests/test_pr_followup_hook.pyscripts/tests/test_pr_watch.py
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
|
Thanks — valid finding, applied in 75306b0. Checked it against the source rather than taking it on trust, and the substance holds. Used Re-verified after the change:
Worth noting for the record: this was the one substantive finding on the PR, and it landed on the fixture rather than the change itself — which is a reasonable outcome for a diff whose entire purpose is making fixtures stop depending on ambient state. |
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
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
Review record — fallback panel, 2 roundsCodeRabbit reviewed the first commit (one valid nitpick, applied in 75306b0), then went rate-limited on every subsequent head — detected on both the comment surface ( Round 1 — 2 HIGH, both real, both mine: the regression test guarded 2 of 7 pinned fields (missing Round 2 — found the fix to round 1 was itself wrong: the new guard-on-the-guard failed open (skip predicate computed with the function under test), "EVERY config-derived global" was a hand-written list of 7, and the marker axes still reddened a legitimate adopter. Plus the correctness lens caught that the PR body still carried a claim round 1 had proven false — because I'd fixed the commit message and not the body. All addressed in 29da37d. The adversarial lens re-verified the core mechanism independently and reported it could not break the pin sweep. Stopping criterion applied — blast radius, not round countPer Recording honestly what that does not cover: 29da37d itself has not been through a panel round. The counter-argument for continuing is real — round 2 found a MED/HIGH inside round 1's fix, so fix rounds on this file have a demonstrated defect rate. Stopping here is a judgment that the blast radius does not justify a fourth round, made by the operator, not a claim that the code is now known-clean. VerificationSix config shapes, all with Per-field sweep of Receipt: |
… filed (#52) Session block for the adopter-upgrade session. Five PRs merged across two repos: OpenKitchen #256 (pre-v2 install to schema v2), #257 (review.bots emptied), #258 (doc sync); kit #43 (panel isolation doctrine) and #49 (test suite detached from ambient config, closing #48). The upgrade worked. What it mostly produced was evidence about the kit: 17 issues, and the recurring shape is that the kit's quality mechanisms each cover a different subset of the kit and nothing checks they agree — KIT_OWNED tracks 24 of 37 shipped files, the manifest tracks no test files, the suite covers no lines of pre-push. That gap is the root cause of #36, #40, #41 and #51, and #47 is the check that closes the class. Also recorded because it casts doubt backwards: #50 — a mutation preserving source LENGTH leaves stale bytecode Python treats as valid, so a git-clean restore does not restore. Mutation evidence gathered earlier in the session is suspect, including on the already-merged #256. Ran archive_plan_sessions.py --keep 4 (447 -> 377 lines); the 2026-07-25 block moved to kit-handoff-history.md verbatim. No friction-log entry: the log is AT its 150-line budget with an explicit "sweep required before the next entry" tripwire, and this session's friction was routed straight to Issues — the Principle #2 routing, not a neglected inbox. CodeRabbit found one real issue in review: the archive pointer references a section that exists nowhere and claims a folding step the script never performs. Filed as #53 rather than fixed here — the text is engine-generated, so a hand-edit would be overwritten by the next sweep and reproduces in every adopter. Next session: cs-toolkit Phase 1 only (additive install, zero behaviour change). Deferring the pr_watch swap (nightly fixer in active development) and the pre-push swap (blocked on #46). Claude-Session: https://claude.ai/code/session_01V27wAgECMEApqAvmJMFyqm
1 participant
Closes #48. Found by a real adopter making a one-value config change (topij/OpenKitchen#257), which it blocked.
The bug
pr_watchresolves its review config at import time into module-level constants:_load_pr_watch()executes the module, so ~32 tests exercised bot behaviour through those constants and silently required the surrounding repo to configure a review bot.OpenKitchen has CodeRabbit installed but on a plan where it never returns a verdict (#45), so
review.bots: []is the truthful value. Setting it turned 32 kit-owned tests red — on assertions about engine behaviour. Reproduced here: onmain,review.bots: []gives 33 failed (the 33rd is a followup-hook test OpenKitchen does not vendor); on this branch, 323 passed.Why it mattered
The kit's founding invariant is "engines are kit-owned; config is adopter-owned." The converse has to hold too, and didn't: a legitimate adopter value broke kit-owned tests, and the only in-repo fix was editing them — which makes them un-upgradable, the exact failure the kit exists to prevent.
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.
The fix
_load_pr_watch()pins the loaded module to the engine defaults, via_load_review_config'sFileNotFoundErrorbranch (returns defaults, stays quiet)._load_pr_watch(pin_defaults=False)exists for tests whose subject is the ambient config. Pinning is the default because an ambient-reading test fails invisibly here and only reddens in somebody else's repo.Review history — three rounds, and the first two cuts were wrong
CodeRabbit reviewed the first commit (one valid nitpick, applied), then went rate-limited — detected on both the comment and status-check surfaces, which is exactly the #23 machinery working. So the fallback panel carried rounds 2 and 3.
Round 1 found two HIGH issues, both real:
_REVIEW_BOTS— the field that caused the bug — was unguarded: deleting its pin left the suite green here while restoring the entire failure for an adopter.test_shipped_config_preserves_the_engine_defaults_behavior, which took its module from the pinned loader and so asserted that the defaults classify like the defaults. It was not even one of the 32 broken tests — it was healthy coverage taken out as collateral.Round 2 found the fix to round 1 was also wrong:
if pin_defaults:toif True:gave "322 passed, 1 skipped" while the regression its docstring claims to prevent became reachable.botsaxis only.Corrections to earlier versions of this description
Recorded rather than quietly edited, because the same drift recurred:
Verification — the property, not a green run
All runs with
PYTHONDONTWRITEBYTECODE=1and__pycache__cleared, per #50: during this PR a same-length mutation left stale bytecode that survived agit-clean restore, so the suite ran mutant code for several minutes.review.bots: []review.require_ci: falsereview.informational_checks: []review.noise_markers: []Per-field mutation sweep of
_pin_engine_defaults: 7/7 killed. Adding an 8thReviewConfigfield now fails until the map is extended.Known residual, deliberately not fixed
Removing
review.fallback_panel, or the wholereview:section, failstest_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.shstate thatinit.shrepairs — a different question from "a legitimate steady-state adopter config must not break kit tests".Scope
Tests only — no engine, no config, no doctrine.
kit_doctor:24 unchanged, 0 differ, 0 missing, 0 unknown(test files are outsideKIT_OWNED, which is #47's subject).Unblocks
topij/OpenKitchen#257, once these tests are refreshed there.
https://claude.ai/code/session_01V27wAgECMEApqAvmJMFyqm
Summary by CodeRabbit