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 3667b14..a20b3c5 100644 --- a/docs/kit-friction-log.md +++ b/docs/kit-friction-log.md @@ -13,6 +13,40 @@ ## 2026-07-25 — inbox +- **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 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 + 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 + distinct receipt source (`fallback:panel`) so the audit trail does not read as a primary + 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 + 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 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..f4e4174 100644 --- a/docs/kit-handoff.md +++ b/docs/kit-handoff.md @@ -14,11 +14,102 @@ > 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. + +> **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-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 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 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`, 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. + +**Decided** + +- 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. + +**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. +- **#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 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 + 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 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 +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.** @@ -73,7 +164,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". @@ -83,11 +174,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. -▶ 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. +✔ 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 41a24ec..97e3592 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -59,7 +59,7 @@ }, "scripts/dev_session.sh": { "role": "engine", - "sha256": "52d962af8088e70fd7b4682d67ec7d6d4354c70f3d8db49b724d4c42c04d98f6" + "sha256": "50dd24f5506fc40c55a7007d3e78395beea69b184194faa2fa3c47e883b5d486" }, "scripts/hooks/pre-push": { "role": "hook", diff --git a/scripts/dev_session.sh b/scripts/dev_session.sh index 92bd926..fd5875c 100755 --- a/scripts/dev_session.sh +++ b/scripts/dev_session.sh @@ -746,10 +746,15 @@ 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`, 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 # 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: