Split the watch-loop predicate from the merge gate (additively)#22
Merged
Conversation
…ation `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
|
Warning Review limit reached
Next review available in: 2 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 (7)
📝 WalkthroughWalkthroughThe PR separates watch-loop convergence ( ChangesMerge readiness predicate split
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/dev_session.sh (1)
731-731: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
mergeableisn't declaredlocal; leftoverdoneis now dead.Line 731 still declares
done(the pre-split read target) instead ofmergeable. Sincemergeableis populated viareadwithout a precedinglocal mergeable, bash makes it an implicit global;doneis now unused.🔧 Proposed fix
- local report done validated_pr validated_base validated_head + local report mergeable validated_pr validated_base validated_headAlso applies to: 749-760
🤖 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/dev_session.sh` at line 731, Update the local variable declaration near the mergeability read to replace the unused done variable with mergeable, ensuring mergeable is explicitly local before it is populated. Apply the same correction to the related declarations and read handling in the mergeable logic around the referenced later section, without changing other variables.
🧹 Nitpick comments (1)
scripts/tests/test_pr_watch.py (1)
152-154: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRetain explicit
doneassertions in these scenarios.These tests now mostly verify only
mergeable, so a regression that re-couples watch convergence to merge authorization could pass.
scripts/tests/test_pr_watch.py#L152-L154: assert both unknown and merged reports remaindone is True.scripts/tests/test_pr_watch.py#L197-L201: assert all unstable variants remaindone is True, while only mergeability varies.scripts/tests/test_pr_watch.py#L273-L278: assertstale["done"]andcurrent["done"]are also true.scripts/tests/test_pr_watch.py#L530-L536: assert all zero-check reports aredone is True.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/test_pr_watch.py` around lines 152 - 154, Strengthen the PR watch tests by explicitly asserting done is True in every specified scenario: unknown and merged reports at scripts/tests/test_pr_watch.py lines 152-154, all unstable variants at lines 197-201, stale and current reports at lines 273-278, and all zero-check reports at lines 530-536. Keep the existing mergeable and merge_blockers assertions unchanged so done remains independently verified from merge authorization.
🤖 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.
Outside diff comments:
In `@scripts/dev_session.sh`:
- Line 731: Update the local variable declaration near the mergeability read to
replace the unused done variable with mergeable, ensuring mergeable is
explicitly local before it is populated. Apply the same correction to the
related declarations and read handling in the mergeable logic around the
referenced later section, without changing other variables.
---
Nitpick comments:
In `@scripts/tests/test_pr_watch.py`:
- Around line 152-154: Strengthen the PR watch tests by explicitly asserting
done is True in every specified scenario: unknown and merged reports at
scripts/tests/test_pr_watch.py lines 152-154, all unstable variants at lines
197-201, stale and current reports at lines 273-278, and all zero-check reports
at lines 530-536. Keep the existing mergeable and merge_blockers assertions
unchanged so done remains independently verified from merge authorization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4facac1b-4fcb-46d2-a0bc-d92fe0c1c25d
📒 Files selected for processing (6)
docs/agentic-dev-kit/workflows/pr-watch.mdkit-manifest.jsonscripts/dev_session.shscripts/pr_watch.pyscripts/tests/test_portability.pyscripts/tests/test_pr_watch.py
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
topij
changed the title
done into watch-convergence and merge-authorization
added 2 commits
July 25, 2026 11:48
…e `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
Owner
Author
|
Thanks — both handled in 3d61512.
Test nitpick — accepted in intent, diverged in form. Worth flagging explicitly: this review ran against So The underlying point was right, so I applied it under the current naming: those four scenarios now pin Re-review against the current head would be useful — the design changed materially since the commit you reviewed. |
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
Owner
Author
Fallback independent review passRun per Caveat stated plainly: I authored this PR, so this is a weaker lens than a genuinely independent reviewer. It satisfies the doctrine's floor, not its ideal. A CodeRabbit pass over the final design once its limit resets would still be worth having. Findings — 3, all fixed in 32f3e4f
Assessed and found sound
Verdict: approve. 202 tests pass. Merge class remains operator-merge; merging on the operator's explicit instruction in-session. |
Owner
Author
|
@coderabbitai full review Requesting a pass over the final merged design. Your earlier review was bound to Merged as |
|
Your plan includes PR reviews subject to rate limits. More reviews will be available in 50 minutes. |
topij
pushed a commit
that referenced
this pull request
Jul 25, 2026
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
topij
pushed a commit
that referenced
this pull request
Jul 25, 2026
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
topij
pushed a commit
that referenced
this pull request
Jul 25, 2026
…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
topij
added a commit
that referenced
this pull request
Jul 25, 2026
* chore: update handoff — Phase 3a split the watch predicate from the merge 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 * fix: correct a stale merge-gate comment and the wrap-up's review claims 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 * fix: correct claims the adversarial review pass found in the wrap-up 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 * fix: correct an overstatement, a regression from the last fix round, 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 --------- 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 was referenced Jul 25, 2026
topij
pushed a commit
that referenced
this pull request
Jul 25, 2026
Third verification pass on this branch, third round of real findings — and both HIGH ones were introduced by the previous commit. **`bots_behind_head` was never recorded under `--allow-pending-bot-review`.** `behind` was populated inside `if not allow_pending_bot`, so the override path fetched the reviews and discarded them. That override IS the #22/#25 scenario: a bot queued or rate-limited through a redesign, merged on a fallback receipt. Combined with the second bug below it was a total blind spot — neither the poll nor the receipt said the reviewer's last review was of an old commit. Coverage now comes off the `pr view` snapshot before the branch, independent of the check fetch. **The pending-deference swallowed the case it was written to protect.** It deferred to *any* pending entry, but a pending check that has aged past the grace window or been cancelled by an announced outage is the engine saying "this verdict is not coming" — the reviewer-went-away case. So after ~15 minutes of polling any stuck bot, and on the #22 two-check outage shape, the coverage warning went quiet precisely when it should speak. Only an actively *blocking* pending entry defers now. Both were invisible to the two lenses that reviewed the commit introducing them, and both are now pinned by mutation. Also: `bots=bots` threading was correct and pinned by nothing (dropping it passed all 297, since every other test uses the default list where scoped and unscoped are identical); a `.get` on a key the next lookup indexes directly; an over-long docstring line; and the doc claimed `--record-review` "records the same thing" as the poll, which is no longer true — the receipt deliberately does not defer to a pending bot. Tests 297 → 300. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp
topij
added a commit
that referenced
this pull request
Jul 25, 2026
* Surface which commit each bot's last review actually saw The cheap half of #27. A receipt binds to the head and a push invalidates it. That answers "was this exact code reviewed" — but not "by whom, and how much of it did they see". A bot can review commit 1, go rate-limited through a material redesign, and the merge proceed on a fallback receipt taken at commit 5. #22 shipped a fail-open rework the primary reviewer never saw; #25 repeated it smaller, three commits behind. Both times the gap was real, both times it was only reconstructible by reading the PR thread afterwards, and both times someone had to notice it by hand. Now `pr_watch` reads it off the reviews it ALREADY fetches — no extra `gh` call: ⚠ review coverage: coderabbit's last review was of 954b93f, not the current head — a receipt taken now does not mean it saw this design Verified against the real #25, where it reproduces exactly the gap I had to work out manually before merging it an hour ago. **Reported, never gating.** The faithful fix — invalidate a receipt when the diff changes *shape*, not merely when the head moves — is the other half of #27 and stays open, because it risks becoming a wedge on a repo whose bot is permanently unavailable. Making the gap visible is the part that is unambiguously safe, and it is what would have changed the operator's judgment on both #22 and #25. Malformed input is dropped rather than reported as coverage of an unknown commit, and the function cannot raise — it runs on the ordinary poll path. Tests 286 → 289. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp * Pin the two properties a mutation pass proved were unpinned The adversarial review ran 12 mutants; 9 died, 2 were real gaps, 1 equivalent. **`anchored=True` had no test.** Flipping it to `anchored=False` passed all 289 — and with that flip `xcoderabbit` reviewing the head reads as CodeRabbit having covered it, suppressing the exact warning this feature exists to raise. It is the one property here with an actual adversary (anyone may review on a public repo), and the PR body asserted it while nothing checked it. **Newest-per-bot was untested independent of array order.** Replacing the whole comparison with `if True:` — last-in-array wins, timestamps ignored — also passed. The only multi-review fixture listed its reviews ascending, so the two rules were indistinguishable. Now pinned with a descending fixture and a shuffled one. Also pinned the tie-break direction that matters: an undated review must never displace a dated one, since an undated review at the head would otherwise set `covers_head` and hide the gap. **A non-string `oid` crashed `render` on the poll path.** `isinstance(commit, dict)` validates the container, not the value, so `{"oid": 12345}` passed the filter and then died on `sha[:7]` — in the function whose stated job is tolerating malformed input, and in the test that claimed to cover it. Now type-checked. Same for a non-string `submittedAt`, which raised TypeError against a sibling review's string. All four mutants now die. Corrected three claims that overstated the code: the docstring said "one entry per bot that has reviewed at all" (a bot whose reviews carry no usable SHA gets no entry — under-reporting, deliberately, but not what it said); "the function cannot raise" was absolute and wasn't; and the lexicographic timestamp compare's dependence on gh's exact serialization was undocumented. The doc's "known gaps" list now includes coverage's own blind spot — a bot that never reviewed produces no entry and no warning. Tests 289 → 293. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp * Fix a bug the first lens missed, and put the warning where it claims to speak The general-correctness lens, run because the doctrine requires both and the adversarial pass had noted it was only one. It found a real bug that pass missed — which is the disjointness claim holding up again. **The `str()` coercion I added last commit was actively wrong.** It stopped the TypeError and, in doing so, rendered garbage as a string sorting ABOVE every real timestamp (`"20260725" > "2026-07-25T…"`). So a malformed review at the head displaced the real dated one and set `covers_head: True` — suppressing the warning, in exactly the direction the docstring beside it calls "the direction that matters". Type-checking instead of coercing is equally crash-proof and sorts garbage to the bottom with the undated ones. The sharpest part: `test_a_non_string_sha_or_timestamp_cannot_break_the_poll` feeds precisely that input and asserts only that it does not raise. It walked over the hole. **`coverage` was a pass-through that arrived empty where it mattered most.** `record_review` calls `summarize_review_bots` without it, so `coverage` was `[]` on the receipt path — "no data" indistinguishable from "every bot is current", on the one path whose entire subject is what a receipt covers. Now computed inside the function like every other key, and recorded on the receipt as `bots_behind_head` next to `override` and `bot_signal`. Its own message says "a receipt taken now does not mean it saw this design"; it was printing everywhere except where a receipt is taken. **It now defers to a bot that is mid-review.** A bot reviewing a just-pushed head is behind it by construction, and the pending line already says a verdict is coming — so the warning was firing on every poll of the healthy window, training the reader to skim past the case it exists for. **Two mutants that survived the first pass now die.** Rendering the line unconditionally, and wiring the wrong head into the coverage call, both passed 292 tests: nothing drove `build_report` -> `render` with a bot AT the head and asserted silence. Selectivity is the whole value of the warning. Also: a comment describing a guard the previous commit deleted (the repo's named failure mode, again); `coverage` missing from both report-schema docstrings; the doc paragraph sitting under a sentence that said every signal feeds the merge gate, which this one does not. Tests 293 → 297. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp * The override path was silent about exactly what it exists to record Third verification pass on this branch, third round of real findings — and both HIGH ones were introduced by the previous commit. **`bots_behind_head` was never recorded under `--allow-pending-bot-review`.** `behind` was populated inside `if not allow_pending_bot`, so the override path fetched the reviews and discarded them. That override IS the #22/#25 scenario: a bot queued or rate-limited through a redesign, merged on a fallback receipt. Combined with the second bug below it was a total blind spot — neither the poll nor the receipt said the reviewer's last review was of an old commit. Coverage now comes off the `pr view` snapshot before the branch, independent of the check fetch. **The pending-deference swallowed the case it was written to protect.** It deferred to *any* pending entry, but a pending check that has aged past the grace window or been cancelled by an announced outage is the engine saying "this verdict is not coming" — the reviewer-went-away case. So after ~15 minutes of polling any stuck bot, and on the #22 two-check outage shape, the coverage warning went quiet precisely when it should speak. Only an actively *blocking* pending entry defers now. Both were invisible to the two lenses that reviewed the commit introducing them, and both are now pinned by mutation. Also: `bots=bots` threading was correct and pinned by nothing (dropping it passed all 297, since every other test uses the default list where scoped and unscoped are identical); a `.get` on a key the next lookup indexes directly; an over-long docstring line; and the doc claimed `--record-review` "records the same thing" as the poll, which is no longer true — the receipt deliberately does not defer to a pending bot. Tests 297 → 300. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp * Tidy what the first clean pass found Fourth verification pass on this branch, and the first that could not break the code — no HIGH or MEDIUM. Four LOW items: - **Coverage was computed twice on the normal path**, and `reviews=`/`head=` on `record_review`'s `summarize_review_bots` call had gone dead when the direct call was hoisted above the override branch. Harmless today (pure, no I/O) but a latent divergence trap: the two calls resolve their bot list differently, so the receipt and the poll would silently disagree the moment this function takes a `bots` argument — the exact class `test_coverage_honours_a_non_default_review_bots_list` was just written to pin. - **A comment quoted a message string that does not exist** in the tree. Same stale-comment class this repo keeps finding, three lines below the block the previous commit edited. - `record_review`'s docstring enumerated two of the three keys the receipt can carry; `bots_behind_head` was missing. - Noted why `.get("blocking")` sits next to direct indexing: a pending entry missing that key falls out of the deference set and the warning fires, which is the safe direction. Tests unchanged at 300. 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.
Why
decide_donemeant two things at once:dev_session.sh mergeneeds.Merge authorization got folded into
donebecause that was the only hookcmd_mergehad (it re-pollspr_watch --jsonand gates ondone; it never checked the receipt itself).Two costs:
This also unblocks the Phase 3 back-port: cs-toolkit's nightly fixer watches to green-and-clean in Step 6.2 and records no receipt, so porting the conflated predicate would strike every lane at the wall-clock cap.
What
The schema only grows — no existing field changes meaning:
decide_donekeeps its original signature and semantics, now composed from the two new predicates.cmd_mergereadsmergeable, taking the flag the engine computes rather than re-deriving the predicate in bash — a restated contract is exactly the drift that bitparallel.mdlast session.Render names the converged-but-unauthorized state instead of letting it read as clearance:
Why additive, specifically
The first cut of this PR redefined
doneto mean watch-convergence. My own adversarial pass found that fails open. Engine upgrades are per-file —/upgradeStep 3 treatsmissingas a supported state ("a sized-down adoption omits engines deliberately; one surveyed repo installs 2 of 6 on purpose") — so a repo can run the newpr_watch.pyagainst an olderdev_session.sh. That gate readsdone, and under the redefinition it would have beentruefor a green PR with no review receipt, silently merging unreviewed work.Redefining a field a safety gate reads is unsafe however well the new meaning is documented, because the old reader never sees the documentation. Hence the additive shape. Both skew directions are now safe:
pr_watchpr_watchdev_session.sh(readsdone)done==mergeable→ safedev_session.sh(readsmergeable)Safety
.claude/rules/safety-critical-changes.mdapplies (dev_session.shmerge gate).doneis provably unchanged.test_done_keeps_its_original_merge_authorization_semanticspins it against a local re-implementation of the original expression across all 32 input combinations, and pinsmergeableto the same value.test_report_done_is_always_identical_to_mergeable.mergeablefalse or absent refuses, including when it contradictsdone— proving the gate doesn't fall back (test_self_merge_refuses_a_report_that_is_done_but_not_mergeable).cmd_mergeportability tests exercise the real bash path and caught the change when it landed.201 tests pass (was 196).
Review notes
Per doctrine rules 2–3 this needs dual-lens review (adversarial/bypass + general-correctness) to convergence, and it is operator-merge class.
The lens I'd most want: any input where
mergeableis true but the originaldoneexpression would have been false — that's the only way this could have widened the gate.