Wrap-up: Phase 3a — split the watch predicate from the merge gate#24
Merged
Conversation
…erge gate Records the sequencing decision and the two corrections it took, resolves the previous block's now-superseded `Next:` (it proposed a flag whose purpose disappeared once the predicates were split), and adds two friction entries about the fallback review pass standing in for a rate-limited primary reviewer. Claude-Session: https://claude.ai/code/session_01V27wAgECMEApqAvmJMFyqm
|
Warning Review limit reached
Next review available in: 50 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 (5)
📝 WalkthroughWalkthroughThe PR documents review receipt failure modes and Phase 3a handoff decisions, clarifies merge-gate semantics, expands predicate-alias test coverage, and updates the script checksum in the manifest. ChangesReview gate updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Findings from a cold-context subagent review of #24 — the mechanism proposed in this PR's own friction entry, tried on this PR. - `dev_session.sh` still described the REJECTED first design at the merge gate: "`done` is the watch-loop predicate ... deliberately true on a PR that carries no review receipt". In the merged code `done` is an unchanged alias of `mergeable`, so that is false and the following sentence is circular. The Python docstrings were updated during the rework; this bash comment was not. The session's headline lesson is "documentation does not reach a stale reader" — and it shipped one inside the gate it was written about. - The handoff and friction log described self-review as satisfying "the doctrine's floor". There is no such floor: rule 2 says a single-lens verdict is "not a green light", and rule 3 wants re-review to convergence, which the fallback's same-pass approve did not meet. Recorded as a rule violation. - Both friction entries now carry a proposed fix, per wrap-up.md; a misquote of safety-critical-changes.md is corrected (the repo's own lesson is that a doc quoting a contract must quote it, not restate it); the "Phase 3" vocabulary is defined so its justification is checkable from this repo; and the superseded `Next:` marker no longer uses a relative reference that breaks once archive_plan_sessions.py demotes the block. Also requested a CodeRabbit pass over #22's merged design — the one concrete action #22 generated, which was recorded only in a PR comment. Claude-Session: https://claude.ai/code/session_01V27wAgECMEApqAvmJMFyqm
Second lens (adversarial, cold context) over #24's own diff. It confirmed the rewritten merge-gate comment is accurate, then found nine issues in the wrap-up. Addressed: - The PR description still said the fallback "met the doctrine's floor" — the exact framing the previous commit retracted from both files. Rewritten, along with two other false body claims ("docs-only"; stale budget numbers). - The handoff claimed the alias equivalence is "pinned across the whole boolean input space". The exhaustive test covers `decide_done` — the function with no in-engine caller — while the report KEY that provides the protection was pinned by four examples. That is the same function-vs-key confusion listed two bullets later as a finding. Claim corrected; the key's test broadened from 4 cases to a 24-case matrix over converged/blocked/receipt state. - "was rejected in review" read as though an independent reviewer caught the fail-open. It was my own adversarial re-read; CodeRabbit's pass on that commit raised a `local` nit and a test nitpick. Attributed. - The outstanding CodeRabbit review of #22's merged design lived only in a PR comment. Now an explicit open item in the handoff, with the unfiled H-severity friction entries. - `▶ Next:` was used to mark a SUPERSEDED item; `session-start` reads every `Next:` trail, so that surfaces a resolved item as actionable. Now `✔`. - A third quote-vs-restate slip: the `/upgrade` paraphrase inverted its verb. - `dev_session.sh` gave naming clarity as the reason to read `mergeable`. The real reason is that it is absent from any pre-split engine, so a version skew fails closed — a safety property, not a style preference. - Friction entry's "severity raised from M" had no referent, and cited this PR's own review as corroboration without saying so. Both stated plainly. Claude-Session: https://claude.ai/code/session_01V27wAgECMEApqAvmJMFyqm
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/kit-friction-log.md`:
- Around line 36-46: Update the safety-critical PR heading in
docs/kit-friction-log.md to say “without any independent review” instead of
claiming there was no review of the final design. Preserve the surrounding
explanation that the fallback reviewed the current head and satisfied the
receipt while the primary review was stale.
🪄 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: 116c7898-2f79-4816-b698-e984751c478a
📒 Files selected for processing (5)
docs/kit-friction-log.mddocs/kit-handoff.mdkit-manifest.jsonscripts/dev_session.shscripts/tests/test_pr_watch.py
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/kit-handoff.md
Comment on lines
+36
to
+46
| - **A safety-critical PR merged without any review of its final design (severity: H).** | ||
| Also #22: CodeRabbit's only completed review was bound to the first commit; the design then | ||
| changed materially (the fail-open rework), and the rate limit never lifted. The receipt | ||
| mechanism records *that a review happened at this head* — satisfied by the fallback — with | ||
| no notion of "the primary reviewer saw an earlier, materially different design." | ||
| **Proposed fix:** invalidate a receipt when the diff changes *shape* rather than only when | ||
| the head moves — e.g. require a fresh receipt when a later push touches a file the recorded | ||
| review never saw. Cheaper interim: have `pr_watch` surface the primary bot's last-reviewed | ||
| SHA next to the current head, so the gap is visible at merge time instead of reconstructible | ||
| only from the PR thread. Related to #23 but distinct: #23 is about detecting the outage, | ||
| this is about what a receipt should mean when the outage outlasts a redesign. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Say “without any independent review” here.
The heading says no review covered the final design, but the entry states that the fallback reviewed the current head and satisfied the receipt. The actual gap was the absence of an independent/primary review, so the current wording overstates the incident.
Proposed wording
-- **A safety-critical PR merged without any review of its final design (severity: H).**
+- **A safety-critical PR merged without any independent review of its final design (severity: H).**📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - **A safety-critical PR merged without any review of its final design (severity: H).** | |
| Also #22: CodeRabbit's only completed review was bound to the first commit; the design then | |
| changed materially (the fail-open rework), and the rate limit never lifted. The receipt | |
| mechanism records *that a review happened at this head* — satisfied by the fallback — with | |
| no notion of "the primary reviewer saw an earlier, materially different design." | |
| **Proposed fix:** invalidate a receipt when the diff changes *shape* rather than only when | |
| the head moves — e.g. require a fresh receipt when a later push touches a file the recorded | |
| review never saw. Cheaper interim: have `pr_watch` surface the primary bot's last-reviewed | |
| SHA next to the current head, so the gap is visible at merge time instead of reconstructible | |
| only from the PR thread. Related to #23 but distinct: #23 is about detecting the outage, | |
| this is about what a receipt should mean when the outage outlasts a redesign. | |
| - **A safety-critical PR merged without any independent review of its final design (severity: H).** | |
| Also `#22`: CodeRabbit's only completed review was bound to the first commit; the design then | |
| changed materially (the fail-open rework), and the rate limit never lifted. The receipt | |
| mechanism records *that a review happened at this head* — satisfied by the fallback — with | |
| no notion of "the primary reviewer saw an earlier, materially different design." | |
| **Proposed fix:** invalidate a receipt when the diff changes *shape* rather than only when | |
| the head moves — e.g. require a fresh receipt when a later push touches a file the recorded | |
| review never saw. Cheaper interim: have `pr_watch` surface the primary bot's last-reviewed | |
| SHA next to the current head, so the gap is visible at merge time instead of reconstructible | |
| only from the PR thread. Related to `#23` but distinct: `#23` is about detecting the outage, | |
| this is about what a receipt should mean when the outage outlasts a redesign. |
🤖 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 `@docs/kit-friction-log.md` around lines 36 - 46, Update the safety-critical PR
heading in docs/kit-friction-log.md to say “without any independent review”
instead of claiming there was no review of the final design. Preserve the
surrounding explanation that the fallback reviewed the current head and
satisfied the receipt while the primary review was stale.
…and stale config vocabulary Round-3 convergence pass (cold context, regression-focused) plus CodeRabbit's review of the current head — which agreed on the first item. - **I overstated my own failure.** Both files said the final commit of #22 was "reviewed by nothing". The fallback pass did see `32f3e4f` (it posted seconds after that commit, naming it); what never happened is an INDEPENDENT review, and a rule-3 re-review after the fixes. Accuracy cuts both ways — an overstated violation is still an inaccurate handoff. - **The previous round's fix to `dev_session.sh` overshot.** It claimed `done` is a key "an older engine might carry with different semantics". Every pre-split kit `pr_watch` emitted `done` with exactly this merge-authorization meaning — that is this PR's own thesis — so both keys are safe against a kit version skew. Rewritten to state the real reason: `mergeable` is the precise name, `done` the one retained for compatibility. This is the pattern rule 3 exists for: my fix made an accurate comment inaccurate. - `kit-handoff.md` pointed at friction entries "below" that live in a different file — added by the same commit that removed a relative reference elsewhere. - `config/dev-model.yaml` still described `unavailable_markers` and `informational_checks` in terms of `done`. Post-split the load-bearing property is that informational checks never block `converged` (the anti-wedge property). Commit 74da7c8 claimed to have swept every stale reference; it had not. Claude-Session: https://claude.ai/code/session_01V27wAgECMEApqAvmJMFyqm
Owner
Author
|
Fixed in the latest push — and you were right, which is worth stating plainly since it corrects an error in my direction rather than the usual one.
Two notes for the record:
Also fixed this round, from a cold-context review pass: a regression my own previous fix introduced into the |
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.
Session wrap-up for 2026-07-25 (second session).
Not docs-only. Alongside the narrative files this fixes a stale comment on the merge gate in
scripts/dev_session.sh(withkit-manifest.jsonrehashed) and broadens one test. See "Found in review" below.Handoff
Records Phase 3a and the two corrections the design took:
doneconflating two predicates; splitting them removed the need for it.converged/mergeablesplit,donekept as an unchanged alias. Tests 196 → 202.local-declaration nit and a test nitpick.Also resolves the previous session block's
▶ Next:, which still described the superseded ordering, and defines the "Phase 3 / cs-toolkit back-port" vocabulary that previously existed nowhere outside this file — making the design's stated justification unverifiable from this repo.Friction
Two entries at H, both with proposed fixes. They are issue-shaped and flagged in the handoff as unfiled —
triage-friction-logshould graduate them:safety-critical-changes.mdrule 2 is explicit that a single-lens verdict is "not a green light"; rule 3 wants re-review to convergence, which Split the watch-loop predicate from the merge gate (additively) #22's same-pass approve did not meet. On the record: Split the watch-loop predicate from the merge gate (additively) #22 merged with no review pass over its final design. Proposed fix is a panel of fresh-context reviewers with distinct lenses and an honest receipt source.Found in review
This PR was reviewed by the panel mechanism it proposes — two cold-context subagents, one correctness-lensed, one adversarial. They overlapped on nothing.
The correctness lens found that
scripts/dev_session.shstill described the rejected first design at the merge gate: "doneis the watch-loop predicate … deliberately true on a PR that carries no review receipt". In the merged codedoneis an alias ofmergeable, so that was false and the next clause circular. It shipped tomainin #22 and three of my own passes read past it. Fixed here.The adversarial lens then found nine issues in the wrap-up itself, including:
decide_done— the function with no in-engine caller — while the report key that actually provides the protection was pinned by four examples. Same function-vs-key confusion the session lists as a finding. Corrected, and the key's test broadened to a 24-case matrix.▶ Next:sigil used to mark a superseded item —session-startreads everyNext:trail, so that would surface a resolved item as actionable. Now✔ Superseded./upgradein a way that inverted its verb), in a session whose own recorded lesson is that a doc quoting a contract must quote it.For the operator
Two things
wrap-up.mdasks me not to do unilaterally, flagged rather than assumed:archive_plan_sessions.pydemotes the block). The workflow authorizes a▶ Next:line on the latest block only.Both are revertable if you'd rather they weren't there.
Budgets at HEAD: handoff 182/400, friction log 67/150 — no archive sweep needed. 202 tests pass.
Summary by CodeRabbit
Documentation
Reliability