From 49f67932c81345dc0cbf7e68d4bc26631d21d430 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 12:43:34 +0300 Subject: [PATCH 1/4] =?UTF-8?q?chore:=20update=20handoff=20=E2=80=94=20Pha?= =?UTF-8?q?se=203a=20split=20the=20watch=20predicate=20from=20the=20merge?= =?UTF-8?q?=20gate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/kit-friction-log.md | 20 +++++++++++++ docs/kit-handoff.md | 65 +++++++++++++++++++++++++++++++++++----- 2 files changed, 78 insertions(+), 7 deletions(-) diff --git a/docs/kit-friction-log.md b/docs/kit-friction-log.md index 3667b14..0739980 100644 --- a/docs/kit-friction-log.md +++ b/docs/kit-friction-log.md @@ -13,6 +13,26 @@ ## 2026-07-25 — inbox +- **The fallback review pass has no independence when the cockpit authored the PR (severity: M).** + On #22 CodeRabbit was rate-limited, so `review.fallback_commands` ran — but the agent + running it was the same one that wrote the diff. It did find three real issues, so the + floor held; what it cannot provide is the *adversarial disjointness* the dual-lens rule in + `safety-critical-changes.md` is built on ("an adversarial pass and a general-correctness + pass routinely find **disjoint** holes"). Self-review collapses both lenses into one + perspective, on the exact code that perspective just produced. Not issue-shaped yet + because the fix is unclear — options include requiring a fresh-context reviewer for the + fallback, or blocking the merge until the primary bot recovers when the change is + safety-critical class. Worth watching for a second occurrence before deciding. + +- **A safety-critical PR merged without the primary reviewer ever seeing the final design (severity: M).** + Also #22: CodeRabbit's only completed review covered the first commit, and the design then + changed materially (the fail-open rework). The rate limit never lifted, so the merged code + carries one review — of a version that no longer exists. The receipt mechanism records + *that a review happened at this head*, which was satisfied by the fallback; it has no + notion of "the primary reviewer reviewed an earlier, materially different design." Related + to #23 but distinct: #23 is about detecting the outage, this is about what a receipt should + mean when the outage persists across a redesign. + - **The `cp -r` quickstart can't distinguish kit-owned from adopter-owned files (severity: M).** Any file the kit tracks lands in an adopter's repo, which is why this repo's own narrative docs had to be renamed `kit-*.md` rather than simply filled in. `kit-manifest.json` now diff --git a/docs/kit-handoff.md b/docs/kit-handoff.md index 5e34141..2bbc5e5 100644 --- a/docs/kit-handoff.md +++ b/docs/kit-handoff.md @@ -14,11 +14,64 @@ > Older session blocks graduate to [`kit-handoff-history.md`](kit-handoff-history.md) once > this file crosses its line budget (`scripts/check_doc_budget.py`). -Last updated: 2026-07-25 — first full assessment-and-remediation pass; the kit now fixes -its own adoption path, and can diagnose an installed copy. +Last updated: 2026-07-25 — Phase 3a shipped: the watch-loop predicate and the merge gate +are now separate, and the schema change that separates them is purely additive. ## Latest session — 2026-07-25 +**Theme —** Made the Phase 3 sequencing decision, and it changed under scrutiny — twice. +Both times the correction came from asking what a *stale reader* of the mechanism would do. + +- **The blocking problem was not the porting order.** `decide_done` conflated "is there + more for me to fix?" with "is this authorized to merge?", because `cmd_merge` had no + other hook — it re-polls `pr_watch --json` and gates on `done`. That conflation, not the + sequence of ports, is what would have wedged cs-toolkit's nightly fixer (its Step 6.2 + watches to green-and-clean and records no receipt). Fixing it removed a whole phase from + the plan. +- **#22 merged.** `converged` (watch loop) and `mergeable` (merge gate) are now distinct; + `dev_session.sh merge` gates on `mergeable`. Tests 196 → 202. +- **The first cut of #22 failed open, and was rejected in review.** It redefined `done` to + mean watch-convergence. Because `/upgrade` refreshes engines **per file** (`missing` is a + supported state — a sized-down adoption installs some engines on purpose), a new + `pr_watch.py` can run against an older `dev_session.sh` whose gate reads `done` — which + would then have authorized merges on PRs with no review receipt at all. +- **So the schema only grows.** `done` stays an unchanged alias of `mergeable`. Both skew + directions fail closed; the equivalence is pinned across the whole boolean input space + rather than by example. +- **CodeRabbit was rate-limited**, so the configured fallback pass ran instead. It found + three further issues, including a docstring that claimed a compatibility guarantee the + *function* doesn't provide — the report **key** does. + +**Decided** + +- Enforce at the merge point, not at `done`. A watch loop asking "anything left to fix?" + should never be answered "no" only once a review receipt exists. +- A field that a safety gate reads may be added to, never redefined. +- #19 and #23 get designed together — they are the same ambiguity on two surfaces, and + both run into the informational-check exclusion being load-bearing against wedging. + +**Learned** + +- **Documentation does not reach a stale reader.** Redefining `done` was safe by every + local reading of the new code and unsafe in fact, because the component that would have + been wrong is the one that never sees the new docs. Version skew is not hypothetical + here: per-file engine upgrades are a supported, documented workflow. +- **An unavailable reviewer can be indistinguishable from a clean one.** CodeRabbit's + rate-limit arrived as a status-check *description* on a check classified informational, + so nothing surfaced it (#23). The doctrine's "a blocked bot is an action signal" rule can + only fire if the outage is detected. +- Reviewing one's own PR satisfies the doctrine's floor, not its intent — recorded in the + friction log rather than treated as a solved problem. + +▶ Next: fix **#19 + #23 together** (Phase 3b) — the queued-vs-unavailable ambiguity, on +both the comment and status-check surfaces. Design constraint: the informational-check +exclusion must keep preventing a wedge on a bot that never reports, so neither can be fixed +by simply letting that check block. Wants CodeRabbit actually available for review. + +______________________________________________________________________ + +## Earlier session — 2026-07-25 + **Theme —** Assessed the kit against its own ten principles, then fixed what the assessment found. Six PRs merged; tests 83 → 188. The recurring shape: **the kit had written down rules it was itself violating.** @@ -83,11 +136,9 @@ written down rules it was itself violating.** queued, and its four valid findings landed after the merge. `decide_done` can't tell the two apart. -▶ Next: decide the **Phase 3 sequencing** for the cs-toolkit back-port — porting the review -receipts as-is would break the nightly fixer (nothing there records a receipt, so -`decide_done` would report `done=False` forever on every PR it opens). Proposed order: -additive merge gate first → receipts behind a flag → wire the fixer's own `/code-review` -step to record → flip the flag. +✔ Resolved in the session above. The proposed order (merge gate → receipts behind a flag → +wire the fixer → flip) was replaced: the flag existed to defer a breakage caused by `done` +conflating two predicates, so splitting them removed the need for it. ______________________________________________________________________ From 2a9f692566adc86e5e3913ce60f61a04e685df98 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 12:53:16 +0300 Subject: [PATCH 2/4] fix: correct a stale merge-gate comment and the wrap-up's review claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/kit-friction-log.md | 41 +++++++++++++++++++++++----------------- docs/kit-handoff.md | 37 +++++++++++++++++++++++++----------- kit-manifest.json | 2 +- scripts/dev_session.sh | 10 ++++++---- 4 files changed, 57 insertions(+), 33 deletions(-) diff --git a/docs/kit-friction-log.md b/docs/kit-friction-log.md index 0739980..6355417 100644 --- a/docs/kit-friction-log.md +++ b/docs/kit-friction-log.md @@ -13,25 +13,32 @@ ## 2026-07-25 — inbox -- **The fallback review pass has no independence when the cockpit authored the PR (severity: M).** +- **The fallback review pass has no independence when the cockpit authored the PR (severity: H).** On #22 CodeRabbit was rate-limited, so `review.fallback_commands` ran — but the agent - running it was the same one that wrote the diff. It did find three real issues, so the - floor held; what it cannot provide is the *adversarial disjointness* the dual-lens rule in - `safety-critical-changes.md` is built on ("an adversarial pass and a general-correctness - pass routinely find **disjoint** holes"). Self-review collapses both lenses into one - perspective, on the exact code that perspective just produced. Not issue-shaped yet - because the fix is unclear — options include requiring a fresh-context reviewer for the - fallback, or blocking the merge until the primary bot recovers when the change is - safety-critical class. Worth watching for a second occurrence before deciding. + running it was the same one that wrote the diff. It found three real issues, yet + `safety-critical-changes.md` rule 2 is explicit that this is not a pass: "A single-lens + 'converged' verdict is an incomplete review, not a green light." Rule 3 also went unmet — + the fallback's own approve was written in the same pass that produced the final commit, so + nothing reviewed `32f3e4f`. **Proposed fix:** make `review.fallback_commands` a *panel* + spec rather than an inline command — one fresh-context subagent per lens + (adversarial/bypass-focused + general-correctness, the two the doctrine says find + **disjoint** holes), each handed the raw diff with no framing from the author, and a + distinct receipt source (`fallback:panel`) so the audit trail does not read as a primary + review. Demonstrated the same day: a cold subagent on #24 found a stale merge-gate comment + that three self-review passes had walked past. Severity raised from M — this is a + documented rule being violated, not a soft gap. -- **A safety-critical PR merged without the primary reviewer ever seeing the final design (severity: M).** - Also #22: CodeRabbit's only completed review covered the first commit, and the design then - changed materially (the fail-open rework). The rate limit never lifted, so the merged code - carries one review — of a version that no longer exists. The receipt mechanism records - *that a review happened at this head*, which was satisfied by the fallback; it has no - notion of "the primary reviewer reviewed an earlier, materially different design." Related - to #23 but distinct: #23 is about detecting the outage, this is about what a receipt should - mean when the outage persists across a redesign. +- **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. - **The `cp -r` quickstart can't distinguish kit-owned from adopter-owned files (severity: M).** Any file the kit tracks lands in an adopter's repo, which is why this repo's own narrative diff --git a/docs/kit-handoff.md b/docs/kit-handoff.md index 2bbc5e5..f00cf64 100644 --- a/docs/kit-handoff.md +++ b/docs/kit-handoff.md @@ -22,12 +22,19 @@ are now separate, and the schema change that separates them is purely additive. **Theme —** Made the Phase 3 sequencing decision, and it changed under scrutiny — twice. Both times the correction came from asking what a *stale reader* of the mechanism would do. +> **What "Phase 3" and "the cs-toolkit back-port" mean.** cs-toolkit +> (`/Users/topi/Coding/in-parallel/cs-toolkit`, a separate private repo) is where this kit's +> mechanisms originated; the kit generalized them, and the back-port is returning the +> improved versions. Phase 3 is the review-receipt + merge-gate slice of that. The vocabulary +> has never been written down outside this handoff, which made the claim below unverifiable +> from inside this repo — recorded here so the next session doesn't have to reconstruct it. + - **The blocking problem was not the porting order.** `decide_done` conflated "is there more for me to fix?" with "is this authorized to merge?", because `cmd_merge` had no other hook — it re-polls `pr_watch --json` and gates on `done`. That conflation, not the - sequence of ports, is what would have wedged cs-toolkit's nightly fixer (its Step 6.2 - watches to green-and-clean and records no receipt). Fixing it removed a whole phase from - the plan. + sequence of ports, is what would have wedged cs-toolkit's nightly fixer — its per-lane + review step (`.claude/commands/nightly-fixer.md` Step 6.2 in that repo) watches to + green-and-clean and records no receipt. Fixing it removed a whole phase from the plan. - **#22 merged.** `converged` (watch loop) and `mergeable` (merge gate) are now distinct; `dev_session.sh merge` gates on `mergeable`. Tests 196 → 202. - **The first cut of #22 failed open, and was rejected in review.** It redefined `done` to @@ -44,8 +51,8 @@ Both times the correction came from asking what a *stale reader* of the mechanis **Decided** -- Enforce at the merge point, not at `done`. A watch loop asking "anything left to fix?" - should never be answered "no" only once a review receipt exists. +- Enforce at the merge point, never at `converged`. A watch loop asking "anything left to + fix?" should never be answered "no" only once a review receipt exists. - A field that a safety gate reads may be added to, never redefined. - #19 and #23 get designed together — they are the same ambiguity on two surfaces, and both run into the informational-check exclusion being load-bearing against wedging. @@ -60,8 +67,15 @@ Both times the correction came from asking what a *stale reader* of the mechanis rate-limit arrived as a status-check *description* on a check classified informational, so nothing surfaced it (#23). The doctrine's "a blocked bot is an action signal" rule can only fire if the outage is detected. -- Reviewing one's own PR satisfies the doctrine's floor, not its intent — recorded in the - friction log rather than treated as a solved problem. +- **#22 merged without satisfying review rules 2 or 3, and that should be recorded as a + violation rather than a compromise.** The doctrine has no "floor" the author's own pass + can meet: rule 2 says a single-lens verdict is "not a green light", and rule 3 wants + re-review until a pass finds nothing new — but the fallback's approve was written in the + same pass that produced `32f3e4f`, so the final commit was reviewed by nothing. + CodeRabbit's only review was bound to the first commit, before the redesign. +- **A cold-context subagent reviewer found what three self-review passes missed** — a stale + comment on the merge gate itself, describing the design that was rejected. Authorship + anchoring, not capability, is what self-review cannot escape. ▶ Next: fix **#19 + #23 together** (Phase 3b) — the queued-vs-unavailable ambiguity, on both the comment and status-check surfaces. Design constraint: the informational-check @@ -126,7 +140,7 @@ written down rules it was itself violating.** required. Corrected in #21: marking ready is the lane's, landing it is the cockpit's. **The lesson isn't "check for drift" — it's that finding two sources doesn't tell you which one is right, and I picked by proximity rather than by testing either against the - baseline." + baseline.** - **A guard that fails open must be loud.** Three separate silent-no-op bugs this session (`origin/main`, the uninstalled hook, `paths.engines`). Silence is indistinguishable from "checked and clean". @@ -136,9 +150,10 @@ written down rules it was itself violating.** queued, and its four valid findings landed after the merge. `decide_done` can't tell the two apart. -✔ Resolved in the session above. The proposed order (merge gate → receipts behind a flag → -wire the fixer → flip) was replaced: the flag existed to defer a breakage caused by `done` -conflating two predicates, so splitting them removed the need for it. +▶ Next: superseded by the 2026-07-25 Phase 3a session (PR #22). The proposed order (merge +gate → receipts behind a flag → wire the fixer → flip) was replaced: the flag existed to +defer a breakage caused by `done` conflating two predicates, so splitting them removed the +need for it. ______________________________________________________________________ diff --git a/kit-manifest.json b/kit-manifest.json index 41a24ec..3bf542e 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -59,7 +59,7 @@ }, "scripts/dev_session.sh": { "role": "engine", - "sha256": "52d962af8088e70fd7b4682d67ec7d6d4354c70f3d8db49b724d4c42c04d98f6" + "sha256": "6d7b3f6ada3322362f0f593eb77a05bc3bac23e0ebe19db9ed7115df5779e9b7" }, "scripts/hooks/pre-push": { "role": "hook", diff --git a/scripts/dev_session.sh b/scripts/dev_session.sh index 92bd926..f17d5ab 100755 --- a/scripts/dev_session.sh +++ b/scripts/dev_session.sh @@ -746,10 +746,12 @@ cmd_merge() { report="$(GH_REPO="$repo_nwo" DEVKIT_STATE_ROOT="$session_dir/state" \ uv run "$SCRIPT_DIR/pr_watch.py" "$pr" --json)" \ || _die "pr-watch failed for PR #$pr" - # Gate on `mergeable`, NOT `done`. `done` is the watch-loop predicate - # ("anything left to fix?") and is deliberately true on a green, comment-clean - # PR that carries no review receipt. `mergeable` is the merge-gate predicate: - # done AND no deterministic merge blocker AND a receipt bound to this head. + # Gate on `mergeable`: converged AND no deterministic merge blocker AND a + # review receipt bound to this head. The report's `done` is an unchanged + # alias of `mergeable`, so either key would be correct today; `mergeable` is + # named because it says what this gate means. The key that must NEVER gate a + # merge is `converged` — the watch-loop predicate ("anything left to fix?"), + # which is deliberately true on a green, comment-clean PR carrying no receipt. # Read the flag pr_watch computes — never re-derive it here, or this gate # becomes a second copy of the contract that can drift from the engine's. IFS=$'\t' read -r mergeable validated_pr validated_base validated_head <<< "$(printf '%s' "$report" | python3 -c ' From c503f4e3522253538955add006d220e05c631658 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 14:03:40 +0300 Subject: [PATCH 3/4] fix: correct claims the adversarial review pass found in the wrap-up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/kit-friction-log.md | 11 +++++++--- docs/kit-handoff.md | 40 ++++++++++++++++++++++++++-------- kit-manifest.json | 2 +- scripts/dev_session.sh | 10 +++++---- scripts/tests/test_pr_watch.py | 35 ++++++++++++++++++++--------- 5 files changed, 71 insertions(+), 27 deletions(-) diff --git a/docs/kit-friction-log.md b/docs/kit-friction-log.md index 6355417..72b42a0 100644 --- a/docs/kit-friction-log.md +++ b/docs/kit-friction-log.md @@ -24,9 +24,14 @@ (adversarial/bypass-focused + general-correctness, the two the doctrine says find **disjoint** holes), each handed the raw diff with no framing from the author, and a distinct receipt source (`fallback:panel`) so the audit trail does not read as a primary - review. Demonstrated the same day: a cold subagent on #24 found a stale merge-gate comment - that three self-review passes had walked past. Severity raised from M — this is a - documented rule being violated, not a soft gap. + review. **Evidence, with the conflict of interest stated:** the panel was trialled on the + wrap-up PR that carries this entry (#24), so this is self-reported. What it produced is + checkable in that PR's diff — a correctness lens found a merge-gate comment in + `dev_session.sh` describing the *rejected* design, shipped to `main` in #22 and missed by + the author's three passes over it; a second adversarial lens then found nine further + issues in the wrap-up, overlapping the first on none of them. Filed at **H** rather than + the M this would have got before that trial: a documented rule is being violated, not a + soft gap. - **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 diff --git a/docs/kit-handoff.md b/docs/kit-handoff.md index f00cf64..2f85d5d 100644 --- a/docs/kit-handoff.md +++ b/docs/kit-handoff.md @@ -31,20 +31,26 @@ Both times the correction came from asking what a *stale reader* of the mechanis - **The blocking problem was not the porting order.** `decide_done` conflated "is there more for me to fix?" with "is this authorized to merge?", because `cmd_merge` had no - other hook — it re-polls `pr_watch --json` and gates on `done`. That conflation, not the + other hook — it re-polled `pr_watch --json` and gated on `done`. That conflation, not the sequence of ports, is what would have wedged cs-toolkit's nightly fixer — its per-lane review step (`.claude/commands/nightly-fixer.md` Step 6.2 in that repo) watches to green-and-clean and records no receipt. Fixing it removed a whole phase from the plan. - **#22 merged.** `converged` (watch loop) and `mergeable` (merge gate) are now distinct; `dev_session.sh merge` gates on `mergeable`. Tests 196 → 202. -- **The first cut of #22 failed open, and was rejected in review.** It redefined `done` to +- **The first cut of #22 failed open, and my own adversarial re-read caught it — not + CodeRabbit, whose pass on that commit raised only a `local`-declaration nit and a test + nitpick.** It redefined `done` to mean watch-convergence. Because `/upgrade` refreshes engines **per file** (`missing` is a - supported state — a sized-down adoption installs some engines on purpose), a new + supported state — "a sized-down adoption omits engines deliberately (one surveyed repo + installs 2 of 6 on purpose)"), a new `pr_watch.py` can run against an older `dev_session.sh` whose gate reads `done` — which would then have authorized merges on PRs with no review receipt at all. -- **So the schema only grows.** `done` stays an unchanged alias of `mergeable`. Both skew - directions fail closed; the equivalence is pinned across the whole boolean input space - rather than by example. +- **So the schema only grows.** `done` stays an unchanged alias of `mergeable`, and both + skew directions fail closed. Note what is pinned where: the *function* `decide_done` is + held to the pre-split expression across all 32 boolean inputs, but the thing that actually + protects an older `dev_session.sh` is the report **key**, and that is pinned by a matrix of + report shapes rather than exhaustively. Worth keeping straight — the same function-vs-key + confusion is the next bullet's finding. - **CodeRabbit was rate-limited**, so the configured fallback pass ran instead. It found three further issues, including a docstring that claimed a compatibility guarantee the *function* doesn't provide — the report **key** does. @@ -74,8 +80,24 @@ Both times the correction came from asking what a *stale reader* of the mechanis same pass that produced `32f3e4f`, so the final commit was reviewed by nothing. CodeRabbit's only review was bound to the first commit, before the redesign. - **A cold-context subagent reviewer found what three self-review passes missed** — a stale - comment on the merge gate itself, describing the design that was rejected. Authorship - anchoring, not capability, is what self-review cannot escape. + comment on the merge gate itself (`dev_session.sh` `cmd_merge`), describing the design that + was rejected. It shipped to `main` in #22 and was fixed in the wrap-up PR (#24), so the + artifact is visible only in that diff. Authorship anchoring, not capability, is what + self-review cannot escape. +- A second, adversarially-prompted subagent pass then found nine further issues in the + wrap-up itself — including this handoff misattributing a test-coverage claim, and the PR + description still carrying the "floor" framing the diff had already retracted. The two + lenses overlapped on nothing, which is the doctrine's disjointness claim holding up. + +**Open, and owned by nothing yet** + +- **#22's merged design has still never had an independent review.** CodeRabbit's only pass + covered the first commit; the request for a pass over the final design (posted on #22) + was refused for rate limits again. Re-request it — this is a safety-critical merge gate + running on a two-lens subagent panel and the author's own reads. +- The two H-severity friction entries below (fallback independence; a receipt outliving the + design it reviewed) are unfiled. Both now carry a proposed fix, so they are issue-shaped — + `triage-friction-log` should graduate them rather than leaving them in the inbox. ▶ Next: fix **#19 + #23 together** (Phase 3b) — the queued-vs-unavailable ambiguity, on both the comment and status-check surfaces. Design constraint: the informational-check @@ -150,7 +172,7 @@ written down rules it was itself violating.** queued, and its four valid findings landed after the merge. `decide_done` can't tell the two apart. -▶ Next: superseded by the 2026-07-25 Phase 3a session (PR #22). The proposed order (merge +✔ Superseded by the Phase 3a session (PR #22). The proposed order (merge gate → receipts behind a flag → wire the fixer → flip) was replaced: the flag existed to defer a breakage caused by `done` conflating two predicates, so splitting them removed the need for it. diff --git a/kit-manifest.json b/kit-manifest.json index 3bf542e..fe0c8f9 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -59,7 +59,7 @@ }, "scripts/dev_session.sh": { "role": "engine", - "sha256": "6d7b3f6ada3322362f0f593eb77a05bc3bac23e0ebe19db9ed7115df5779e9b7" + "sha256": "3c24f80286c2de30bebf4886e5334e3afd2a9ef9b6464c495bc7863de68ceace" }, "scripts/hooks/pre-push": { "role": "hook", diff --git a/scripts/dev_session.sh b/scripts/dev_session.sh index f17d5ab..42007f6 100755 --- a/scripts/dev_session.sh +++ b/scripts/dev_session.sh @@ -748,10 +748,12 @@ cmd_merge() { || _die "pr-watch failed for PR #$pr" # Gate on `mergeable`: converged AND no deterministic merge blocker AND a # review receipt bound to this head. The report's `done` is an unchanged - # alias of `mergeable`, so either key would be correct today; `mergeable` is - # named because it says what this gate means. The key that must NEVER gate a - # merge is `converged` — the watch-loop predicate ("anything left to fix?"), - # which is deliberately true on a green, comment-clean PR carrying no receipt. + # alias of `mergeable` today, but read `mergeable`: it is absent from any + # pr_watch predating the split, so an engine/gate version skew fails CLOSED + # here, whereas `done` is the key an older or foreign engine might carry with + # different semantics. The key that must NEVER gate a merge is `converged` — + # the watch-loop predicate ("anything left to fix?"), which is deliberately + # true on a green, comment-clean PR carrying no receipt. # Read the flag pr_watch computes — never re-derive it here, or this gate # becomes a second copy of the contract that can drift from the engine's. IFS=$'\t' read -r mergeable validated_pr validated_base validated_head <<< "$(printf '%s' "$report" | python3 -c ' diff --git a/scripts/tests/test_pr_watch.py b/scripts/tests/test_pr_watch.py index 511d069..534104d 100644 --- a/scripts/tests/test_pr_watch.py +++ b/scripts/tests/test_pr_watch.py @@ -97,17 +97,32 @@ def original_done(checks, new_items, merge_blockers, review_evidence, settling): def test_report_done_is_always_identical_to_mergeable() -> None: - """The alias must never drift from the field it aliases.""" - pr_watch = _load_pr_watch() - receipt = {"head": "abc123", "source": "fallback:codex"} + """The alias must never drift from the field it aliases. - for report in ( - pr_watch.build_report(_green_view(), [], set()), - pr_watch.build_report(_green_view(), [], set(), review_receipt=receipt), - pr_watch.build_report(_green_view(isDraft=True), [], set()), - pr_watch.build_report(_green_view(state="MERGED"), [], set()), - ): - assert report["done"] is report["mergeable"] + The report **key** — not :func:`decide_done`, which has no in-engine caller — + is what keeps an older `dev_session.sh` gating on merge authorization. So it + is the key that needs the coverage: walk a matrix of report shapes spanning + converged/not, blocked/clean, and receipt/none, rather than a few examples. + """ + pr_watch = _load_pr_watch() + current = {"head": "abc123", "source": "fallback:codex"} + stale = {"head": "older", "source": "fallback:codex"} + comment = {"id": "c1", "author": {"login": "someone"}, "body": "please fix"} + + views = ( + _green_view(), + _green_view(isDraft=True), + _green_view(state="MERGED"), + _green_view(mergeStateStatus="BLOCKED"), + _green_view(reviewDecision="CHANGES_REQUESTED"), + _green_view(statusCheckRollup=[{"name": "t", "conclusion": "FAILURE"}]), + _green_view(statusCheckRollup=[]), + _green_view(comments=[comment]), + ) + for view in views: + for receipt in (None, current, stale): + report = pr_watch.build_report(view, [], set(), review_receipt=receipt) + assert report["done"] is report["mergeable"], (view, receipt) def test_predicates_are_strictly_bool_typed() -> None: From 09a327c7643bd76510076078b8bce971264e5897 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 14:13:13 +0300 Subject: [PATCH 4/4] fix: correct an overstatement, a regression from the last fix round, and stale config vocabulary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- config/dev-model.yaml | 13 +++++++------ docs/kit-friction-log.md | 4 +++- docs/kit-handoff.md | 10 ++++++---- kit-manifest.json | 2 +- scripts/dev_session.sh | 9 +++++---- 5 files changed, 22 insertions(+), 16 deletions(-) diff --git a/config/dev-model.yaml b/config/dev-model.yaml index ec4f9b2..cce3f64 100644 --- a/config/dev-model.yaml +++ b/config/dev-model.yaml @@ -103,8 +103,8 @@ review: - "actionable comments posted: 0" - "" # Bodies signalling the reviewer could not run. Deliberately NOT noise: these - # surface and block `done`, because a blocked bot is an action signal (run the - # configured fallback), never a silent review waiver. + # surface and block `converged`, because a blocked bot is an action signal (run + # the configured fallback), never a silent review waiver. unavailable_markers: - "bugbot needs on-demand usage enabled" - "review limit reached" @@ -112,10 +112,11 @@ review: - "couldn't start this review" - "review skipped" - "no review credits" - # Status checks that are advisory only and must never block `done`. A review - # bot's check can sit PENDING forever after a trivial follow-up commit; its real - # findings arrive as comments (which DO block). Matched case-insensitively - # against the check name. + # Status checks that are advisory only and must never block `converged` — this + # is the anti-wedge property: a review bot's check can sit PENDING forever after + # a trivial follow-up commit, and the watch loop must still be able to finish. + # Its real findings arrive as comments (which DO block). Matched + # case-insensitively against the check name. informational_checks: [coderabbit] # Whether a PR must have at least one real (non-informational) CI check before # pr_watch will report green. True is the safe default: it stops an autonomous diff --git a/docs/kit-friction-log.md b/docs/kit-friction-log.md index 72b42a0..a20b3c5 100644 --- a/docs/kit-friction-log.md +++ b/docs/kit-friction-log.md @@ -19,7 +19,9 @@ `safety-critical-changes.md` rule 2 is explicit that this is not a pass: "A single-lens 'converged' verdict is an incomplete review, not a green light." Rule 3 also went unmet — the fallback's own approve was written in the same pass that produced the final commit, so - nothing reviewed `32f3e4f`. **Proposed fix:** make `review.fallback_commands` a *panel* + no *independent* pass ever covered `32f3e4f` — the fallback did see that code (it posted + seconds after the commit, naming it), but a self-review re-reading its own fixes is not a + rule-3 re-review. **Proposed fix:** make `review.fallback_commands` a *panel* spec rather than an inline command — one fresh-context subagent per lens (adversarial/bypass-focused + general-correctness, the two the doctrine says find **disjoint** holes), each handed the raw diff with no framing from the author, and a diff --git a/docs/kit-handoff.md b/docs/kit-handoff.md index 2f85d5d..f4e4174 100644 --- a/docs/kit-handoff.md +++ b/docs/kit-handoff.md @@ -77,7 +77,8 @@ Both times the correction came from asking what a *stale reader* of the mechanis violation rather than a compromise.** The doctrine has no "floor" the author's own pass can meet: rule 2 says a single-lens verdict is "not a green light", and rule 3 wants re-review until a pass finds nothing new — but the fallback's approve was written in the - same pass that produced `32f3e4f`, so the final commit was reviewed by nothing. + same pass that produced `32f3e4f`, so no *independent* review ever covered the final + commit — the fallback saw that code, but only as the author re-reading their own fixes. CodeRabbit's only review was bound to the first commit, before the redesign. - **A cold-context subagent reviewer found what three self-review passes missed** — a stale comment on the merge gate itself (`dev_session.sh` `cmd_merge`), describing the design that @@ -95,9 +96,10 @@ Both times the correction came from asking what a *stale reader* of the mechanis covered the first commit; the request for a pass over the final design (posted on #22) was refused for rate limits again. Re-request it — this is a safety-critical merge gate running on a two-lens subagent panel and the author's own reads. -- The two H-severity friction entries below (fallback independence; a receipt outliving the - design it reviewed) are unfiled. Both now carry a proposed fix, so they are issue-shaped — - `triage-friction-log` should graduate them rather than leaving them in the inbox. +- The two H-severity entries in [`kit-friction-log.md`](kit-friction-log.md) (fallback + independence; a receipt outliving the design it reviewed) are unfiled. Both now carry a + proposed fix, so they are issue-shaped — `triage-friction-log` should graduate them rather + than leaving them in the inbox. ▶ Next: fix **#19 + #23 together** (Phase 3b) — the queued-vs-unavailable ambiguity, on both the comment and status-check surfaces. Design constraint: the informational-check diff --git a/kit-manifest.json b/kit-manifest.json index fe0c8f9..97e3592 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -59,7 +59,7 @@ }, "scripts/dev_session.sh": { "role": "engine", - "sha256": "3c24f80286c2de30bebf4886e5334e3afd2a9ef9b6464c495bc7863de68ceace" + "sha256": "50dd24f5506fc40c55a7007d3e78395beea69b184194faa2fa3c47e883b5d486" }, "scripts/hooks/pre-push": { "role": "hook", diff --git a/scripts/dev_session.sh b/scripts/dev_session.sh index 42007f6..fd5875c 100755 --- a/scripts/dev_session.sh +++ b/scripts/dev_session.sh @@ -748,10 +748,11 @@ cmd_merge() { || _die "pr-watch failed for PR #$pr" # Gate on `mergeable`: converged AND no deterministic merge blocker AND a # review receipt bound to this head. The report's `done` is an unchanged - # alias of `mergeable` today, but read `mergeable`: it is absent from any - # pr_watch predating the split, so an engine/gate version skew fails CLOSED - # here, whereas `done` is the key an older or foreign engine might carry with - # different semantics. The key that must NEVER gate a merge is `converged` — + # alias of `mergeable`, and every pre-split pr_watch emitted `done` with this + # same merge-authorization meaning — so either key is safe against a kit + # version skew, and `mergeable` additionally fails CLOSED when absent. Read + # `mergeable` because it is the precise name and is not the one retained for + # backward compatibility. The key that must NEVER gate a merge is `converged` — # the watch-loop predicate ("anything left to fix?"), which is deliberately # true on a green, comment-clean PR carrying no receipt. # Read the flag pr_watch computes — never re-derive it here, or this gate