kit_doctor + /upgrade: make an already-adopted install diagnosable#16
Merged
Conversation
…able Phase 2. Re-running ./init.sh (landed in #8) already handles the config half of an upgrade. This is the engine half, which was the part nothing could see. The problem, measured rather than assumed. Surveying the real installs turned up four distinct shapes, none of them versioned at the engine level: · an ancestor with no config surface at all · a v1-schema repo with 2 of 6 engines and a paths.engines value pointing at a directory containing no engine — so every workflow's <engine-dir>/… reference resolved to nothing, silently, because nothing validates that value · a correctly namespaced v2 repo whose pr_watch.py was 3 features behind · the template itself Nothing could tell an older engine from a hand-patched one, which is what makes "just copy the new files in" unsafe. scripts/kit_doctor.py — read-only drift report against a released kit-manifest.json (path -> sha256). Per kit-owned file: unchanged / differs / missing / unknown-version. Plus the four installation-level checks nothing else performs: schema generation, whether paths.engines actually holds engines, whether the pre-push hook is installed rather than merely shipped, and whether the narrative docs were rendered. Two deliberate design calls: · `differs` does NOT claim a cause. A hash mismatch cannot distinguish "older version" from "hand-edited", and labelling an old file "locally modified" sends someone hunting for edits they never made — this was caught by running the tool against a real adopter, which reported 4 files as locally-modified when they were almost certainly just old. It now narrows by schema version and leaves the call to the operator. · paths.engines indirection is handled by remapping the manifest's kit-layout prefix, so a repo with engines under scripts/devkit/ compares correctly against an unmodified manifest. Adopter-owned paths are never compared — they are supposed to differ, and reporting them would bury the signal. That list is explicit (not inferred) so an installer can consume the same boundary. /upgrade (.claude/commands/upgrade.md) — drives the sequence: detect which of the four shapes this repo is, migrate config via init.sh, then refresh per file keyed on kit_doctor's states. Routes a config-less repo to /adopt instead of guessing a config into existence. Names the case worth routing UPSTREAM: a local edit that is genuinely ahead of the kit should become a kit PR, not be overwritten. Also closes #5: check_doc_budget.py and archive_plan_sessions.py now read config through the stdlib kitconfig instead of PyYAML, so every shipped engine is dependency-free — including the SessionStart hook path added in #13. This is the same invariant kit_doctor asserts: an engine an adopter must edit (or install a dependency for) can never be safely replaced. CI gates the manifest against the tree, since a stale manifest silently degrades every adopter's report to unknown-version. 184 tests pass. Verified against both real adopter repos read-only: brain's missing-engine breakage and OpenKitchen's 4 drifted files are both correctly diagnosed, with the engines-dir failure called out explicitly. Closes #5.
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds ChangesKit drift detection and upgrade
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AdopterRepository
participant KitDoctor
participant KitManifest
participant DevModelConfig
AdopterRepository->>KitDoctor: Run inspection CLI
KitDoctor->>DevModelConfig: Load kit and engine path settings
KitDoctor->>KitManifest: Read expected file hashes
KitDoctor->>AdopterRepository: Hash remapped kit-owned files
KitDoctor-->>AdopterRepository: Return drift report and exit status
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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 |
topij
marked this pull request as ready for review
July 25, 2026 05:59
Found by the fallback review pass on this PR. `.git/hooks` is not the only place git reads hooks from — core.hooksPath overrides it, and pre-commit plus several monorepo layouts set it. Checking only `.git/hooks` reports a correctly-installed hook as missing, so the report would tell an adopter to re-run an install that had already worked. This is the same bug class fixed in init.sh for the WRITE side earlier in this stack, reintroduced on the READ side. Read via a plain .git/config scan rather than shelling out to git, keeping kit_doctor subprocess-free. Also strip a trailing slash from paths.engines before remapping.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/archive_plan_sessions.py (1)
274-289: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winRemove the dangling
yaml.YAMLErrorfrom the except tuple.
kitconfig.load_config()is stdlib-only, so PyYAML exceptions no longer apply here, andyamlis not imported. A config-resolution failure in this block will raiseNameErrorwhile checking the handler tuple instead of catching the expectedFileNotFoundError/KeyError.🐛 Proposed fix
except ( FileNotFoundError, KeyError, OSError, TypeError, ValueError, - yaml.YAMLError, ) as exc:🤖 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/archive_plan_sessions.py` around lines 274 - 289, Remove yaml.YAMLError from the exception tuple in the configured_paths error handler, leaving the existing standard exception types unchanged so failures are caught without referencing the unavailable yaml symbol.
🧹 Nitpick comments (3)
scripts/check_doc_budget.py (1)
48-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale reference to
devmodel_config.Comment still points readers to
devmodel_config.pyfor adjusting budgets, but the import above it now reads fromkitconfig.✏️ Proposed fix
-# Discover the repo root by walking up for a `.git` marker (via devmodel_config) +# Discover the repo root by walking up for a `.git` marker (via kitconfig)🤖 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/check_doc_budget.py` around lines 48 - 50, Update the repository-root discovery comment near the `kitconfig` import to remove the stale `devmodel_config` reference and point readers to `kitconfig` instead, without changing the discovery logic..github/workflows/test.yml (1)
32-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo-pyyaml re-run only covers
test_kitconfig.py.
kit_doctor.py,check_doc_budget.py, andarchive_plan_sessions.pyalso declaredependencies = []and rely onkitconfig, but none of their tests run after pyyaml is uninstalled — so a regression that reintroduces a pyyaml dependency in one of them wouldn't be caught here.♻️ Proposed fix
- name: Engines must work without PyYAML # The stdlib config reader exists so engines carry no third-party # dependency. Prove it in an env where PyYAML genuinely is not installed. run: | python -m pip uninstall -y pyyaml - python -m pytest scripts/tests/test_kitconfig.py -q + python -m pytest scripts/tests -q🤖 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 @.github/workflows/test.yml around lines 32 - 37, Expand the “Engines must work without PyYAML” workflow step to run the test suites for kit_doctor.py, check_doc_budget.py, and archive_plan_sessions.py in addition to test_kitconfig.py after uninstalling pyyaml, preserving the existing no-dependency validation.scripts/tests/test_kit_doctor.py (1)
321-400: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo coverage for
main()'s error/exit-code paths.Missing config, missing manifest, and malformed manifest JSON are all documented exit-2 cases but none are exercised here — including the malformed-config path flagged in
kit_doctor.py's review.🤖 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_kit_doctor.py` around lines 321 - 400, Add tests in the test suite for main() covering each documented exit-code-2 failure path: missing configuration, malformed configuration, missing manifest, and malformed manifest JSON. Assert both the exit code and the relevant error behavior, using the existing test fixtures and invocation patterns.
🤖 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.
Inline comments:
In @.claude/commands/upgrade.md:
- Around line 64-70: Move the branch-creation step from Step 3 to before the
Step 2 migration command in the upgrade instructions. Ensure ./init.sh runs only
after the upgrade branch has been created, while preserving the existing branch
workflow and idempotent migration guidance.
- Around line 64-73: Update the Step 2 migration command to execute the freshly
fetched kit migrator at /tmp/agentic-dev-kit/init.sh against the target
repository, rather than the repository’s potentially stale ./init.sh.
Alternatively, explicitly refresh the local init.sh before running it, while
preserving the documented idempotent migration behavior.
In `@README.md`:
- Around line 136-138: Update the README command example to replace the Unicode
em dash note with a valid shell comment, ensuring the alternative python
invocation passes no unintended arguments; preserve the existing uv invocation
and explanatory note.
In `@scripts/check_doc_budget.py`:
- Around line 46-51: Update main() to catch KeyError alongside FileNotFoundError
and ValueError when loading or validating configuration, so a missing
doc_budgets key follows the existing graceful error message and exit-2 path
instead of producing an unhandled traceback. Keep the existing get(config,
"doc_budgets") behavior unchanged.
---
Outside diff comments:
In `@scripts/archive_plan_sessions.py`:
- Around line 274-289: Remove yaml.YAMLError from the exception tuple in the
configured_paths error handler, leaving the existing standard exception types
unchanged so failures are caught without referencing the unavailable yaml
symbol.
---
Nitpick comments:
In @.github/workflows/test.yml:
- Around line 32-37: Expand the “Engines must work without PyYAML” workflow step
to run the test suites for kit_doctor.py, check_doc_budget.py, and
archive_plan_sessions.py in addition to test_kitconfig.py after uninstalling
pyyaml, preserving the existing no-dependency validation.
In `@scripts/check_doc_budget.py`:
- Around line 48-50: Update the repository-root discovery comment near the
`kitconfig` import to remove the stale `devmodel_config` reference and point
readers to `kitconfig` instead, without changing the discovery logic.
In `@scripts/tests/test_kit_doctor.py`:
- Around line 321-400: Add tests in the test suite for main() covering each
documented exit-code-2 failure path: missing configuration, malformed
configuration, missing manifest, and malformed manifest JSON. Assert both the
exit code and the relevant error behavior, using the existing test fixtures and
invocation patterns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 621c35ee-2d6b-4219-834b-749cd263ade6
📒 Files selected for processing (9)
.claude/commands/upgrade.md.github/workflows/test.ymlREADME.mdkit-manifest.jsonscripts/archive_plan_sessions.pyscripts/check_doc_budget.pyscripts/kit_doctor.pyscripts/lib/kitconfig.pyscripts/tests/test_kit_doctor.py
Comment on lines
+64
to
+70
| ## Step 2 — Migrate the config (idempotent) | ||
|
|
||
| ```bash | ||
| ./init.sh | ||
| ``` | ||
|
|
||
| This is the supported config upgrade path, and it is safe to re-run any number of times. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Create the upgrade branch before running migration.
Step 2 mutates configuration, hooks, and documentation, but the branch is created only in Step 3. This contradicts the “runs on a branch” guarantee and can leave migration changes on the operator’s current branch. Move the branch-creation step before ./init.sh.
Also applies to: 82-86
🤖 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 @.claude/commands/upgrade.md around lines 64 - 70, Move the branch-creation
step from Step 3 to before the Step 2 migration command in the upgrade
instructions. Ensure ./init.sh runs only after the upgrade branch has been
created, while preserving the existing branch workflow and idempotent migration
guidance.
Comment on lines
+64
to
+73
| ## Step 2 — Migrate the config (idempotent) | ||
|
|
||
| ```bash | ||
| ./init.sh | ||
| ``` | ||
|
|
||
| This is the supported config upgrade path, and it is safe to re-run any number of times. | ||
| It only ever **adds** missing keys, never guesses over an existing value; it probes | ||
| `paths.engines` from where engines actually are rather than defaulting; it stamps | ||
| `kit.version`; it installs the pre-push hook as a shim (honoring `core.hooksPath`); and |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Run the fetched kit’s migrator, not a potentially stale local ./init.sh.
This procedure fetches the current kit but then invokes the target repository’s existing init.sh before refreshing it. An older adopted installation may therefore miss the new schema migrations while appearing to complete successfully. Run /tmp/agentic-dev-kit/init.sh against this repository, or refresh init.sh before invoking it.
🤖 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 @.claude/commands/upgrade.md around lines 64 - 73, Update the Step 2
migration command to execute the freshly fetched kit migrator at
/tmp/agentic-dev-kit/init.sh against the target repository, rather than the
repository’s potentially stale ./init.sh. Alternatively, explicitly refresh the
local init.sh before running it, while preserving the documented idempotent
migration behavior.
Comment on lines
+136
to
+138
| ```sh | ||
| uv run scripts/kit_doctor.py # or: python scripts/kit_doctor.py — stdlib only | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a shell comment instead of an em dash.
python scripts/kit_doctor.py — stdlib only passes the Unicode em dash and words as arguments, so copying this alternative can fail argument parsing. Use python scripts/kit_doctor.py # stdlib only or move the note outside the code block.
🤖 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 `@README.md` around lines 136 - 138, Update the README command example to
replace the Unicode em dash note with a valid shell comment, ensuring the
alternative python invocation passes no unintended arguments; preserve the
existing uv invocation and explanatory note.
Comment on lines
+46
to
+51
| from kitconfig import get, load_config, repo_root # noqa: E402 | ||
|
|
||
| # Discover the repo root by walking up for a `.git` marker (via devmodel_config) | ||
| # rather than assuming a fixed `scripts/<script>.py` depth — so this keeps working | ||
| # when the kit is vendored under a nested dir (e.g. scripts/devkit/). | ||
| REPO_ROOT = _repo_root() | ||
| REPO_ROOT = repo_root() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
main() doesn't catch KeyError from the new fail-loud get().
kitconfig.get(config, "doc_budgets") (line 76) has no default, so per its documented contract it raises KeyError when the key is absent — but main()'s except (FileNotFoundError, ValueError) (line 150) doesn't include KeyError. A config missing doc_budgets now crashes with an unhandled traceback instead of the intended graceful error: ... + exit 2, directly undercutting the PR's goal (and issue #5's explicit requirement) of warning and exiting cleanly on missing/unreadable config.
🛡️ Proposed fix
try:
statuses = evaluate(args.root, args.config)
- except (FileNotFoundError, ValueError) as exc:
+ except (FileNotFoundError, ValueError, KeyError) as exc:
print(f"error: {exc}", file=sys.stderr)
return 2🤖 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/check_doc_budget.py` around lines 46 - 51, Update main() to catch
KeyError alongside FileNotFoundError and ValueError when loading or validating
configuration, so a missing doc_budgets key follows the existing graceful error
message and exit-2 path instead of producing an unhandled traceback. Keep the
existing get(config, "doc_budgets") behavior unchanged.
topij
added a commit
that referenced
this pull request
Jul 25, 2026
* fix: address CodeRabbit's post-merge findings on #16 The review landed AFTER the merge — I treated a queued bot as an unavailable one and recorded the fallback receipt too early. Details in the PR body. All four findings were valid; two are real logic holes in the /upgrade skill. 1. (major) /upgrade mutated before branching. Step 2 ran init.sh — which rewrites config, installs hooks, and renders docs — while the branch was not created until Step 3, so the migration could land on whatever branch the operator was on, contradicting the skill's own "runs on a branch" guarantee. Branch creation moved ahead of the first mutation; the duplicate step in Step 3 replaced with a confirmation. 2. (major) /upgrade ran a stale migrator. It fetched the current kit and then invoked the TARGET repo's existing init.sh — the old one, without the new schema migrations — so an upgrade would report success while applying none of them. The exact silent-no-op class this stack has been removing. init.sh is now refreshed from the fetched kit before being run, with a chmod +x note since a copy can drop the bit. 3. (minor) An em dash inside a shell code block in README became an argument if copy-pasted. Replaced with a real shell comment. 4. (major) check_doc_budget.py crashed with an unhandled KeyError on a config with no doc_budgets section, instead of the intended one-line error + exit 2. CodeRabbit attributed this to the new fail-loud get(); that attribution is wrong — devmodel_config.get was equally fail-loud and main() already caught only (FileNotFoundError, ValueError), so the gap predates the kitconfig switch. Valid bug either way, now caught, with a subprocess-level regression test asserting exit 2 and no traceback. Manifest regenerated (the CI gate caught the stale hash, as designed). * fix(upgrade): copy templates before running init.sh Caught by the fallback review pass on this PR. init.sh resolves docs/templates/*.tmpl relative to the WORKING DIRECTORY, so Step 2 ran the refreshed migrator before Step 4 had copied the templates in — it prints 'note: template … missing — skipped' and seeds nothing. Harmless noise for a repo whose narrative docs are already in use (they would be left untouched regardless), but a PARTIALLY-adopted repo missing one of the four docs would silently not get it seeded — and partial adoptions are the common shape, not the exception. Verified both directions: templates absent -> four 'skipped' notes and no seeding; templates present -> seeded. Also corrected the claim that running the fetched kit's init.sh in place is 'equivalent' — every path it reads resolves against the working directory, so it needs the templates here either way. --------- Co-authored-by: Topi Jarvinen <topi.jarvinen@gmail..com>
This was referenced Jul 25, 2026
topij
added a commit
that referenced
this pull request
Jul 25, 2026
* chore: wrap up — the kit now practises Principle #1 on itself Session wrap-up, and the resolution of the dogfooding gap the assessment found: this repo's own handoff and friction log were unrendered skeletons, so the kit was not doing the one thing Principle #1 requires. It could not simply be fixed by filling them in. docs/handoff.md and docs/friction-log.md are the SKELETONS shipped to adopters — writing real session blocks there would ship this repo's narrative into every adopting repo and destroy the 'devkit-template: unrendered' marker the seeding logic depends on. So the kit's own files are docs/kit-*.md, with the indirection explained at the config keys. An adopter's config keeps the plain names; only the template repo needs this. - config paths + doc_budgets repointed to docs/kit-*.md - docs/kit-handoff.md: first real session block (six PRs, 83 -> 196 tests, what was decided, what was learned, and the Phase 3 next step) - docs/kit-friction-log.md: the friction that is NOT yet issue-shaped, plus a note that most of this session's friction routed straight to GitHub Issues — which is Principle #2's prescribed routing, not a neglected inbox - kit_doctor: kit-*.md added to ADOPTER_OWNED so they are never compared - the marker test split in two: shipped skeletons asserted by LITERAL filename (reading paths.handoff would now check the kit's live plan instead), plus a new test asserting the kit's own plan is real — no marker, no placeholder dates Filed while wrapping up: #18 (the cp -r quickstart can't tell kit-owned from adopter-owned; the manifest already encodes the boundary, nothing consumes it for installing — landing that would let kit-*.md go back to the plain names) and #19 (a receipt can be recorded while the primary bot is merely queued, which is how #16's findings landed post-merge). 196 tests pass. check_doc_budget and kit_doctor both clean: the kit's own narrative docs now report 'in use' rather than 'still an unrendered template'. * docs: correct getting-started's stale seeding description Found by the fallback review pass on this PR. getting-started.md still described init.sh as seeding the narrative docs 'only if they don't already exist' — the rule #8 replaced, and the one that could never fire (the kit ships those files, so a copy-in always landed them first). Swept the other docs/handoff.md references while I was there and deliberately left them: README's what's-inside table, parallel-dev.md, adopt.md, and the two flywheel skills all describe what an ADOPTER gets, where the plain filenames are correct. Only this repo's own config uses kit-*.md. --------- Co-authored-by: Topi Jarvinen <topi.jarvinen@gmail..com>
topij
added a commit
that referenced
this pull request
Jul 25, 2026
* fix(pr-watch): split `done` into watch-convergence and merge-authorization `decide_done` meant two things at once: "is there more for me to fix?" (what the poll/fix/mark-seen loop needs) and "is this authorized to merge?" (what `dev_session.sh merge` needs). Merge authorization was folded into `done` because that was the only hook `cmd_merge` had. The conflation has two costs. A watch loop that has genuinely finished reports `done: false` forever until someone records a review receipt — so any caller that watches to convergence without recording one wedges, and the operator is pressured into recording a receipt early just to terminate the loop (#19, where a receipt was recorded against a merely-queued CodeRabbit on #16 and four valid findings landed after the merge). Split the predicate: decide_done(checks, new_items, *, settling) # watch loop decide_mergeable(done, *, merge_blockers, review_evidence) # merge gate `build_report` emits both; `cmd_merge` now gates on `mergeable`, reading the flag the engine computes rather than re-deriving the predicate in bash — a restated contract is the drift that bit `parallel.md` last session. The gate is authorization-preserving, not merely still-passing: `mergeable` == the pre-split `done` expression across all 32 input combinations, pinned by test. Missing/false `mergeable` fails CLOSED, so an older or foreign `pr_watch` that predates the split cannot authorize a merge on the old semantics. Per `.claude/rules/safety-critical-changes.md` this is operator-merge class and needs dual-lens review before merge. Claude-Session: https://claude.ai/code/session_01V27wAgECMEApqAvmJMFyqm * docs(config): correct the require_ci comment to name `mergeable` The comment claimed `done` requires a current-head review receipt. After the predicate split that is `mergeable`'s job — `done` is watch-convergence only. Found by sweeping every remaining reference to the old semantics; both runtime adapters were already clean because they delegate to the shared workflow doc rather than restating it. Claude-Session: https://claude.ai/code/session_01V27wAgECMEApqAvmJMFyqm * refactor(pr-watch): make the predicate split purely additive Adversarial review of the previous commit found a fail-open. Engine upgrades are per-file (`/upgrade` Step 3 treats `missing` as a supported state — "a sized-down adoption omits engines deliberately"), so a repo can run the new `pr_watch.py` against an older `dev_session.sh`. That gate reads `done` — and with `done` redefined to mean watch-convergence, it would have authorized merges on PRs with no review receipt at all. Redefining a field that a safety gate reads is unsafe regardless of how the new meaning is documented, because the old reader never sees the documentation. So nothing changes meaning; the schema only grows: converged = all_green && !new_items && !settling (new: watch loop) mergeable = converged && !blockers && review_evidence (new: merge gate) done = mergeable (UNCHANGED) `decide_done` keeps its original signature and semantics, now composed from the two new predicates. An older `dev_session.sh` reading `done` still gates on merge authorization; a newer one reads `mergeable` and fails closed when the key is absent, so both skew directions are safe. Claude-Session: https://claude.ai/code/session_01V27wAgECMEApqAvmJMFyqm * docs(pr-watch): update the exit-code note to name the current predicates Claude-Session: https://claude.ai/code/session_01V27wAgECMEApqAvmJMFyqm * fix(dev-session): declare `mergeable` local; pin `converged` alongside `mergeable` Addresses CodeRabbit's review of a3a5f91. 1. `cmd_merge` still declared `local report done …` after the gate variable was renamed, so `done` was declared-unused and `mergeable` leaked to global scope. Not exploitable (the `read` assigns it fresh every call) but wrong. 2. The test nitpick asked for explicit `done` assertions in four scenarios. Its literal form no longer applies — the rework made `done` an alias of `mergeable`, so asserting `done is True` on a MERGED or BLOCKED PR would now assert the opposite of the intent. Honoured the underlying point instead: those scenarios now pin `converged` alongside `mergeable`, so a regression that re-couples watch convergence to merge authorization fails. Claude-Session: https://claude.ai/code/session_01V27wAgECMEApqAvmJMFyqm * fix(pr-watch): bool-type the merge predicate; correct two docstrings Findings from the fallback independent review pass (CodeRabbit's check reads "Review rate limited", and its only completed review predates the additive rework, so the configured fallback ran per the review doctrine). 1. `decide_mergeable` returned its last operand, so a truthy non-bool `review_evidence` would propagate into the report. `dev_session.sh merge` tests the JSON value with `is True` — such a value fails closed, but confusingly, and serializes as e.g. `"mergeable": 1`. Coerced to bool and pinned by test. 2. `decide_done`'s docstring claimed the compatibility guarantee for itself. The thing protecting an older `dev_session.sh` is the report's `done` KEY — that gate shells out to `--json` and never imports this module, and the function now has no in-engine caller. As written, someone could delete the key as a redundant alias, see the function's promise still standing, and conclude the protection survived. It would not. 3. `build_report`'s settling note still pointed at `decide_done`; settling is a convergence concern. Claude-Session: https://claude.ai/code/session_01V27wAgECMEApqAvmJMFyqm --------- Co-authored-by: Topi Jarvinen <topi.jarvinen@gmail..com>
topij
added a commit
that referenced
this pull request
Jul 25, 2026
…) (#25) * Resolve queued-vs-unavailable on both surfaces (#19 + #23) Two issues, one predicate. A configured review bot's status check is excluded from the blocking tally so a bot that never reports cannot wedge the watch loop — but that exclusion made an *unavailable* bot indistinguishable from one that reviewed and found nothing (#23), and a merely *queued* bot indistinguishable from a finished one (#19). Neither could be fixed by letting that check block, because the exclusion is the anti-wedge property. The way out is that nothing here touches `decide_converged`. The anti-wedge property lives entirely in that predicate and is left alone; every new signal feeds the merge gate, which already requires someone to explicitly record a review receipt. So the gate can wait longer, but the poll/fix/ack loop can always finish. `summarize_review_bots` resolves each `review.bots` entry to: - unavailable — an `unavailable_markers` hit on a comment body OR on the bot's status-check DESCRIPTION (#23; `gh pr view --json statusCheckRollup` carries no description at all, hence the second `gh pr checks` fetch). Never blocks; it is the action signal to run the fallback, and it survives `--mark-seen` so the gap stays readable at merge time. It also cancels the pending block below — a bot that announced it is not reviewing will never leave pending. - pending — the bot's check is non-terminal and no outage was announced. Blocks `mergeable` and makes `--record-review` refuse (#19: on #16 a receipt was taken against a queued CodeRabbit and four valid findings landed post-merge), until it ages past `review.bot_pending_grace_minutes` (default 15) — the bound that keeps a dead bot from wedging the gate. Also here because the fix does not work without them: - `"review rate limited"` added to `unavailable_markers`. The mechanism alone detects nothing: on #22 the check description used that wording while #24's comment said "review limit reached", an hour apart from the same bot. - kitconfig now parses decimal scalars, gated on a literal `.` so `nan`/`inf`/ `1e5` stay strings as PyYAML resolves them. Without it a `2.5` grace silently fell back to the default. - `_load_review_config` returns a NamedTuple; the shape grew, and positional unpacking makes every future growth a breaking change for every reader. - `safety-critical-changes.md` now matches `pr_watch.py`. It computes the gate `dev_session.sh` merely re-checks, and was matching only the latter. Blockers are strictly additive: `done` (the legacy alias an older `dev_session.sh` reads) can only go true→false, so per-file engine skew makes merges wait, never fire early. Tests 204 → 217. Closes #19 Closes #23 Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp * Give the grace clock a source that actually exists Found by running the first cut against this PR: CodeRabbit's pending status context reports `startedAt: 0001-01-01T00:00:00Z` — the zero time, i.e. no timestamp. So the age fallback ("unmeasurable ⇒ fail open, don't block") was not an edge case at all: it was the *only* path CodeRabbit ever took, and the #19 guard printed "age unmeasurable, NOT blocking" against a live review-in-progress. The guard was dead code for the one bot it was written for. The clock now falls back to when this engine first saw the bot pending, persisted per PR. Because it is our clock and only ever advances, every pending bot reaches the grace bound — so the fail-open branch is gone entirely rather than merely narrowed, and there is no unmeasurable case left to wedge on. Stored as `bot_pending_since: {"head": sha, "bots": {bot: iso}}` — self- describing rather than scoped by the sibling `state["head"]`. That field is the false-settle guard's input; making `record_review` maintain it just to scope this clock would put two intents on one key, and that key decides whether a just-pushed commit may settle. `record_review` persists the first sighting BEFORE refusing. Without it a cold `--record-review` restarts the clock at zero on every retry, so the refusal could never expire and the override would be the only way through — a wedge dressed as a guard. Verified live on this PR: the merge gate now reports `review bot coderabbit has not reported yet (check CodeRabbit pending 0.0m < 15m grace)`. Tests 217 → 221. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp * Address CodeRabbit's findings on 233fdd0 All four valid, including one real bug in my own migration. **init.sh corrupted an inline `unavailable_markers` list.** The awk insert anchored on `^ unavailable_markers:`, which also matches an inline flow list (`unavailable_markers: ["a", "b"]`) — equally valid YAML and equally supported by kitconfig. It then appended a ` - ` line under a key that already has a scalar value, producing a malformed mapping, and wrote it straight over the adopter's config. Now only a bare block-list key is migrated; an inline list falls through to the existing by-hand warning. Verified against both shapes. **`fetch_check_details` degraded silently.** Both new guards depend on that fetch, so an older `gh` rejecting one of the requested `--json` fields would disable #19's and #23's detection with no trace — "checked and clean" indistinguishable from "never checked", the failure mode that cost this repo three silent-no-op bugs in one session. It now warns on stderr, once per process rather than per poll (a warning repeated every round of a watch loop is skimmed past exactly like silence). **The PyYAML parity claim is now checked, not restated.** The float-gating docstring argued "PyYAML resolves these as strings" while asserting only the expected values — which would pass just as happily if the reference resolver disagreed with every one of them. Compared against `yaml.safe_load` directly. **`.tmp.$$`** for the migration temp file, matching `append_to_section`. Tests 221 → 222. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp * Address the dual-lens review: three real defects, all in the new code Two fresh-context passes over the diff (adversarial + general-correctness) found almost disjoint sets, which is the doctrine's claim about why a gate needs both. **A stale outage comment permanently cancelled the pending block — #19 walking back in through the door built to close it.** `collect_comments` returns the whole PR history, unscoped by head or age, so one transient rate limit on commit 1 put the bot in `unavailable_bots` for the rest of the PR and waved through every later queued review. Rate limits are transient by construction, which makes "this bot was rate-limited earlier" the ORDINARY state of a later poll, not a corner case. Now only a CHECK-surface outage cancels: a check describes the bot's state now, a comment describes the past. Comment outages are still reported — that is #23's action signal — they just cannot speak for the present. **A corrupt or foreign `bot_pending_since` value wedged the merge gate forever.** `age = parse(stored) or 0.0` pinned an unparseable value at the maximally- blocking age *and* wrote it back, so every later poll re-read the same poison. Reachable from a corrupt state file, an older engine, or a richer future format. Anything unreadable is now replaced rather than coerced, so the window stays bounded whatever wrote the state. **The init.sh migration silently orphaned every existing marker on a 2-space block list.** The guard checked the key line's indent while the awk anchored on 4-space items, so a config formatted with dashes at the key's own indent (what several YAML formatters emit, and what kitconfig explicitly supports) got the new marker inserted at the wrong depth — dropping all the originals, while the post-condition grep still passed and `mv`'d it over the adopter's config. That is fail-OPEN on the very markers this PR exists to extend. Now indent-aware, verified against 4-space, 2-space and inline shapes. Also: `review_bots.signal` makes a failed check-read machine-readable (the merge gate consumes stdout; a stderr warning is not a guard on an autonomous path); the override is recorded on the receipt; comment authors match anchored rather than by substring (that input is attacker-controlled on a public repo, check names are not); the grace bound compares the exact age, not the rounded display value; `render` no longer claims "past the grace" for a check cancelled by an outage; kitconfig's float rule is YAML 1.1's own (signed exponent required) with its three remaining divergences pinned as divergences; and init.sh's idempotency grep is scoped to the marker block so it cannot silently skip both the migration and its warning. Tests 222 → 229. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp * Fix the regressions the fix round introduced (re-review #2) The doctrine's "re-review after every fix round, not one pass" earning its keep: a third fresh-context pass found that two of the three fixes were incomplete and that the round introduced a new data-loss path. **`record_review` computed the bot signal and threw it away.** When the check read fails there are no blockers to raise, so the receipt was taken with the #19 guard simply switched off — and unlike the explicit override, which the same round had just made auditable, this left no trace at all. The commit message even justified `signal` as machine-readable evidence while no consumer read it. Now recorded on the receipt as `bot_signal`, surfaced by the renderer. Still not a refusal: an old `gh`, or a PR with no checks, is an environment problem, and refusing would turn it into a wedge whose only exit is the override flag. **The poison-clock fix missed the parseable half.** A *future-dated* `bot_pending_since` survives `_age_minutes`, gets clamped to age 0 by `max(0.0, …)`, and is re-persisted verbatim — so the block lasts until real time catches up with the stamp. Measured at 30 days on a copied state file. Anything beyond a small skew tolerance is now treated as unusable and restamped, so the window is bounded whatever wrote it. The previous test only covered unparseable values, which is why this survived. **The new `review.bots` migration lost adopter values.** `grep -q '^ bots:'` misses a 4-space-indented `review:` block and then appends a duplicate at 2 spaces, which the reader resolves last-key-wins — silently dropping a `[bugbot, coderabbit]` down to `[coderabbit]`. It also reported success when it had written nothing, and was satisfied by an unrelated `bots:` under any other section. **The marker migration still no-opped silently, and could write to the wrong section.** Scoping the *idempotency grep* to the block left the outer key anchor whole-file, so: a tab- or 4-space-indented key skipped both migration and warning; a same-named list under an earlier section got the marker while the success message claimed `review.unavailable_markers` had it; a comment or blank line mid-list truncated the idempotency view and re-inserted a marker already present; and a file with no trailing newline failed the `wc -l` post-condition. Both migrations now go through `section_range` / `section_lines`, which bound every read and write to the `review:` section at any indent. Every whole-file `grep '^ key:'` in a migration is a latent bug in two directions at once — it misses the key at another indent and matches a same-named key elsewhere — and this change had shipped one of each. Also: kitconfig's float pattern is now transcribed from PyYAML's resolver rather than approximated. Hoisting the sign out of the alternation diverged on `+.5`, `-.5` and `-.5e+3` — the exact "float() is too loose" class the previous commit claimed to have eliminated. Parity verified over 24 forms; the two remaining gaps (`.inf`/`.nan`, sexagesimal) are pinned as gaps. Plus the blocker string no longer reads `pending 15.0m < 15m grace`, and three docs describing anchored author matching as substring are corrected. Migrations verified against seven real config shapes (4-space, 2-space, inline flow, comment-mid-list, empty list, no-trailing-newline, same-named key under another section), each round-tripped through the real kitconfig. Tests 229 → 231. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp * Stop doing surgery on adopters' config, and test what's left (re-review #3) Third pass. It found that 8a1d5df applied its own section-scoping fix to ONE of three guards in the same function, that the marker insertion had a third corruption mode, and that the highest-blast-radius code in the diff had no tests at all. All three are the same underlying mistake: shell text-munging on a file the adopter owns, defended one special case at a time. **The marker migration is gone.** Three review rounds produced three distinct ways for list surgery to corrupt a config — wrong indent orphaned every existing entry; a whole-file key anchor wrote into a same-named list under another section; and inserting before "the first line that is not an item" splices a multi-line item in half, yielding YAML PyYAML refuses to load *entirely*. Each passed the migration's own post-conditions and printed success. The payoff was one string in a list. `init.sh` now detects the gap and prints exactly what to add — a trade that stops being worth defending after the third corruption. **Guards are section-scoped everywhere now, not in one place.** A whole-file `grep '^ key:'` fails in two directions at once: it misses the key when the adopter's section uses another indent, and it is satisfied by a same-named key elsewhere. `migrate_kit_schema` had one of each; `migrate_runtime_schema` still had three more, which is why a 4-space config was non-idempotent — every run re-appended `fallback_commands`. **Per-key review migration.** One `noise_markers` guard used to gate a block defining five keys, so an adopter with `unavailable_markers` but no `noise_markers` got a second definition of it — and both readers resolve last-key-wins, silently replacing their list with the kit's defaults. **`append_to_section` re-indents to the section it is writing into.** Appending a 2-space block to a 4-space section produced a mapping with two indent levels: PyYAML fails on the WHOLE file, kitconfig tolerates it and applies last-key-wins. Note what did NOT ship: making this function return non-zero on "section not found" seemed like the obvious way to stop callers reporting false success — under `set -eu`, with callers that don't test its status, it aborted init.sh on the first config missing any optional section. Caught by running it. **Engine fixes.** The observed clock now wins over a check's own stamp whenever we have one: preferring the check let the age REGRESS when a future-dated stamp slid inside the skew tolerance, restarting a window that had already expired and making merge_blockers non-monotonic in wall-clock time. And `bot_signal` is recorded only for `unavailable`, not for `skipped` — "you configured no bots" was being reported as "the guard did not run", putting a permanent false warning on every receipt a bot-less adopter takes. **Tests, finally.** Eight config shapes (2-space, 4-space, flush-indent, inline flow, decoy section, header with trailing space, multi-line item, no trailing newline) × corruption, idempotency, exactly-once, and the instruct-when-it-can't path. Every corruption above fails at least one. The kitconfig-vs-PyYAML disagreement on multi-line scalars is asserted as a known reader limitation rather than skipped, so closing it later fails loudly. Verified the shipped config is unchanged relative to main (both reformat the same 5 comment-alignment lines via the pre-existing set_field). Tests 231 → 249. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp * Address CodeRabbit's review of the current head Its first pass covered `233fdd0`; this is its first look at the actual design, after the grace clock and three rounds of fallback-panel fixes. All three findings valid. **The "ACTION NEEDED" message walked inline-list adopters into the corruption it replaced.** Dropping the list surgery left an instruction that always prints `- "review rate limited"` — a block item. An adopter whose `unavailable_markers` is a flow list (`["a", "b"]`) who follows that literally hangs a block item off a flow scalar and breaks their config. The step is read-only, so it could not corrupt anything itself; it would just have talked someone else into doing it. Now detects the style and says "add the string inside the brackets" instead. **`zip(pending, exact_ages, strict=True)`** — the lists are appended in lockstep, so a later edit adding a `continue` between them would pair each entry with the wrong age, silently, and only for a bot with more than one pending check. **The no-PyYAML type test covered 3 of 21 tokens**, which inverts the invariant its own neighbour states ("kept as one list so the parity check cannot quietly cover a smaller set"). The parity test skips precisely where the engines run — a bare env with no PyYAML — so those three were the only coverage that environment had. Now parametrized over the full set with a declared expected type, plus a guard that the two sets stay equal, so adding a token cannot leave it untested where it matters most. Tests 249 → 270. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp * Cover the branch a mutation proved was dead-codeable (re-review #4) **The style-detection branch had no test, proven by mutation.** Hardwiring the condition to a constant — so inline-list adopters get the corrupting `- ` advice — still passed the whole suite. The only assertion checked the block direction. Sharpest part: `_MIGRATION_SHAPES` already contained an `inline_flow` fixture and the commit message advertised the path as covered; the fixture was there, the assertion was never written. Now parametrized per style, and the same mutation fails. **Style detection missed a real flow spelling.** `head -n 1 | grep '\['` only sees the key line, so a flow list whose value sits on the FOLLOWING line (`unavailable_markers:\n ["a", "b"]`) read as block style and got the corrupting advice. Presence of `- ` items is the discriminator now, which also gives the empty-key case its own answer: no value means the engine defaults apply, and those already contain the marker, so asking for it is noise. **A marker named only in a trailing comment counted as present.** The grep ran over raw lines, and the kit's own shipped config carries a `# the status-check wording of …` comment on that exact line — so an adopter with the phrase in a comment and not in the list would be told nothing. Matched against comment-stripped values now. **The no-PyYAML type assertions covered 21 of 25 tokens** while the guard's docstring claimed they covered the same set as the parity check. The parity test walks two collections; the guard compared against one. The four missing tokens are the divergences — exactly where this reader departs from PyYAML on purpose, so an accidental change there looks like a fix. Both collections are covered now, and the divergent tokens' string-identity assertion moved out from behind the `importorskip` it never needed. Tests 270 → 281. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp * Don't give confident advice about a list the reader can't read Round 6 found that the previous round promoted a list spelling to "supported" in the instruction table without checking whether the kit's own reader parses it. `kitconfig` — what every engine uses — resolves a flow list whose brackets don't close on the key line to `{}`, and a multi-line one to the string `"["`: unavailable_markers: ["a", "b"] -> ['a', 'b'] unavailable_markers:\n ["a", "b"] -> {} unavailable_markers: [\n "a",\n "b"\n] -> '[' So an adopter in either of the latter shapes has NO markers in effect at all, and "add the string inside the brackets" is advice that fixes nothing. Those shapes now get told what is actually wrong — the whole list is inert — which is considerably bigger news than one missing marker. The style detection is unchanged for the shapes that do work. Two smaller ones from the same pass: - **Comment-stripping was quote-unaware**, so a marker containing an issue number (`- "tracked in #23: review rate limited"`) was truncated before the match and the adopter was asked for a marker they already had. Now scans character-wise and only cuts a `#` outside quotes. - **`grep -qi`'s case-insensitivity had no test** — dropping `-i` passed all 281 — which is the same untested-branch class the previous round existed to close. Also corrected in the PR body: the baseline is 202 tests, not 204 (measured at the merge-base); "four ways to corrupt" is three, matching what init.sh's own comment enumerates — the fifth round's finding was about the replacement message, not surgery; and the migration matrix is 8 shapes × 2 tests plus a separate 5-row instruction table, not 8 × 4. Tests 281 → 285. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp --------- Co-authored-by: Topi Jarvinen <topi.jarvinen@gmail..com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 2.
./init.sh(from #8) already handles the config half of an upgrade. This is the engine half — the part nothing could see.Why
Surveying the real installs turned up four shapes, none versioned at the engine level:
paths.enginespointing at a directory containing no engine — so every workflow's<engine-dir>/…reference resolved to nothing, silentlypr_watch.pywas 3 features behindNothing could distinguish an older engine from a hand-patched one, which is what makes "copy the new files in" unsafe.
What
scripts/kit_doctor.py— read-only drift report againstkit-manifest.json(path → sha256). Per kit-owned file:unchanged/differs/missing/unknown-version, plus the four installation checks nothing else performs: schema generation, whetherpaths.enginesactually holds engines, whether the hook is installed rather than merely shipped, and whether the narrative docs were rendered./upgrade— drives shape detection → config migration → per-file refresh keyed on those states. Routes a config-less repo to/adoptrather than guessing a config into existence, and names the case worth routing upstream: a local edit genuinely ahead of the kit should become a kit PR, not be overwritten.Two design calls worth reviewing
differsdeliberately does not claim a cause. A hash mismatch can't distinguish "older version" from "hand-edited". I only learned this by running the tool against a real adopter: it reported 4 files aslocally-modifiedwhen they were almost certainly just old. Telling someone that sends them hunting for edits they never made. It now narrows by schema version and leaves the call to the operator.paths.enginesindirection is remapped, so a repo with engines underscripts/devkit/compares correctly against an unmodified manifest — otherwise every namespaced adopter reads as a wholesalemissing.Also closes #5
check_doc_budget.pyandarchive_plan_sessions.pynow read config through the stdlibkitconfiginstead of PyYAML, so every shipped engine is dependency-free — including theSessionStarthook path added in #13. Same invariantkit_doctorasserts: an engine you must edit, or install a dependency for, can never be safely replaced.Verification
unknown-version)brain's missing-engine breakage andOpenKitchen's 4 drifted files are both correctly diagnosed, with the engines-dir failure called out explicitlySummary by CodeRabbit
New Features
Documentation
Quality Improvements