From 30c287c8fe1d01c56e259e9dc62df2a3dbc2e234 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Mon, 27 Jul 2026 07:30:49 +0300 Subject: [PATCH 1/3] =?UTF-8?q?chore:=20update=20handoff=20=E2=80=94=20#59?= =?UTF-8?q?=20+=20#61.1=20shipped,=20#61.2=20built=20and=20reverted?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Session block for 2026-07-27, six friction entries, and an archive sweep (--keep 4; the default 6 was a no-op at 421/400 lines, which is the logged friction reproducing a third time). The friction worth reading first: · a mutation harness that restores outside `finally` leaves the tree mutated — #50's hazard by a route #50 does not describe, and the .pyc framing is what made a hash-verified restore look sufficient (H); · a negated closing keyword got re-armed while documenting the last one, so the rule needs a mechanical grep in pr-watch/wrap-up rather than a habit (M); · the panel's isolated worktree pointed at the wrong ref for the sixth consecutive launch across two sessions — contract item 7 should invert its default from "verify" to "assume it is wrong and clone the target" (H); · a review bot with an incremental model keeps a stale review across a force-push or rewrite, and nothing warns at push time when re-requesting is still cheap (M). Claude-Session: https://claude.ai/code/session_01PWoYsKTB9fBp4knZbXqbHc --- docs/kit-friction-log.md | 53 ++++++++++++ docs/kit-handoff-history.md | 93 ++++++++++++++++++++ docs/kit-handoff.md | 163 ++++++++++++++---------------------- 3 files changed, 211 insertions(+), 98 deletions(-) diff --git a/docs/kit-friction-log.md b/docs/kit-friction-log.md index 7204614..79dd0ca 100644 --- a/docs/kit-friction-log.md +++ b/docs/kit-friction-log.md @@ -34,6 +34,59 @@ accounted for. Everything above now lives in [`kit-friction-log-archive.md`](kit-friction-log-archive.md). +## 2026-07-27 + +- **A mutation harness that restores outside `finally` leaves the repo mutated.** My + restore line was unreachable when the script died parsing pytest output (a bare + `python3` with no pytest returned no stdout, and `splitlines()[-1]` raised). The + working tree kept a live mutant until `git status` caught it. `#50` and + `fallback-review-panel.md` contract item 5 both warn about mutation testing, but only + about **stale `.pyc`** — this is the same outcome by a different route, and the + `.pyc` framing is what made me think a hash-verified restore was sufficient. **H** — + proposed fix: contract item 5 should say the restore belongs in a `finally` (or a + git-clean check) and that any harness must verify the tree is clean *after* it exits, + not only after a successful run. +- **A negated closing keyword got re-armed while documenting the last one.** Last + session's entry says "never write a closing keyword adjacent to an issue number you + do not intend to close, even negated". This session I wrote a PR body sentence + explaining that I had *removed* a `Closes #61` — and the sentence contained the + literal keyword, which GitHub matches regardless of context. Caught by grepping my own + text, not by remembering the rule. **M** — proposed fix: the rule needs a mechanical + check, not a habit: a `pr-watch` / `wrap-up` step that greps the PR body and every + commit message for `(close|fix|resolve)\w*\s+#\d+` and asks the operator to confirm + each match is intended. A rule that fires only when you remember it does not survive + the session that is busy applying it. +- **A review bot with an incremental model keeps a stale review across a rewrite.** + CodeRabbit reviewed the pre-split head, then declined `@coderabbitai review` + ("does not re-review already reviewed commits") and hit its Fair Usage limit on + `@coderabbitai full review`. The PR was force-pushed and then substantially rewritten, + so the only bot review covered code that no longer existed. `pr_watch`'s + `bots_behind_head` recorded it correctly — the friction is that **nothing warns at + push time**, when re-requesting is still cheap. **M** — proposed fix: have `pr-watch` + surface `covers_head: false` as a distinct, louder line right after a push that + changes the diff shape, rather than only at receipt time. Related: `#27`, `#44`. +- **The panel's worth is disjointness, and this run measured it.** Round 1: the + adversarial lens found the enclosing-repo and `GIT_DIR` regressions; the correctness + lens independently found the tilde disagreement and the misattributed defect. Almost + no overlap, and **four regressions against `main` that CI, 372 tests, my own 19/19 + mutation run, and CodeRabbit all missed**. **No change proposed** — recording it + because `fallback-review-panel.md` argues this from one prior data point, and this is + a second, stronger one worth citing there. +- **The doc-budget remedy no-op reproduced a third time** (see the 2026-07-26 entry + below). `check_doc_budget` reported 421/400; the remedy it names printed *"nothing to + move: 5 session block(s) <= --keep 6"*; `--keep 4` was needed and found by guessing. + **No new fix proposed** — recording the third occurrence, because two of the three + were in *this* repo and the entry below still reads as a one-off. It is not. +- **A review lens's isolated worktree pointed at `main` again — 4 of 4 launches, second + session running.** Every lens detected it and cloned the target itself, because the + prompt required reporting the path and diff stat. This is now the *sixth* consecutive + launch across two sessions where the runtime's isolation was wrong and the mandatory + report was the only thing that caught it. **H** — proposed fix: this is no longer a + caveat, it is the default behaviour. Contract item 7 should stop saying "verify" and + start saying "assume the worktree is wrong; clone the target yourself first", with the + path/diff-stat report as a required field of the lens output (the existing 2026-07-26 + entry proposed the report; this one is about inverting the default). + ## 2026-07-26 - **The doc-budget remedy does not fire at the default `--keep`.** `check_doc_budget` diff --git a/docs/kit-handoff-history.md b/docs/kit-handoff-history.md index 2588e85..99cafe1 100644 --- a/docs/kit-handoff-history.md +++ b/docs/kit-handoff-history.md @@ -4,6 +4,99 @@ Archived session narratives from [`kit-handoff.md`](kit-handoff.md). Keep active and the next step there; this file is append-only history. ## Session log +### 2026-07-25 (Phase 3b) + +**Theme —** Fixed #19 + #23 together, and then spent most of the session discovering +that **the fix rounds were more dangerous than the original diff**. Seven review rounds +on #25 alone; every one found something real; five of them found a defect introduced by +the previous round's fix. + +- **#25 merged — #19 and #23 closed.** `summarize_review_bots` resolves each configured + bot to *unavailable* (outage announced on a comment body **or** a status-check + description — the surface that was invisible) or *pending* (a verdict still coming). + Pending blocks the merge gate until it ages past `review.bot_pending_grace_minutes`; + unavailable never blocks and is the action signal. **Nothing reaches `converged`** — + that is the whole design, and it is what let both be fixed at once. +- **#28 merged — #10 closed.** The state_paths suite failed from inside a lane worktree + because its fixture cleared every sandbox *env* signal and not the *cwd* one. The + issue predicted a per-test audit would be needed; it wasn't — every cwd-sensitive test + already chdirs itself. A mutation pass showed the fix makes the suite *stricter*: + three tests had been passing by accidentally discovering the real repo root. +- **#29 merged — half of #27.** `review_bots.coverage` reports which commit each bot's + last review actually saw, and `--record-review` records it as `bots_behind_head`. + Verified against the real #25, where it reproduces the gap I had had to work out by + hand an hour earlier. #27 stays open for the shape-change half. +- **#26 and #27 filed** from the friction-log inbox, both with concrete sketches. + +**Decided** + +- The anti-wedge property lives in `converged` alone. Every new signal feeds the merge + gate, which already needs an explicit receipt — so a gate can wait, but the poll/fix/ack + loop can always finish. +- **Report, never gate.** All three new fields (`signal`, `bot_signal`/`override`, + `coverage`) make an omission legible at merge time and block nothing. The faithful + version of each risks wedging a repo whose bot is permanently unavailable. +- **Stop patching a mechanism that keeps corrupting.** The `init.sh` marker migration + produced **three** distinct config corruptions across three rounds, each while its own + post-conditions passed and it printed success (the fourth round's finding was about + the *replacement message*, not the surgery). It was deleted rather than patched again; + `init.sh` now detects the gap and prints what to add. + +**Learned** + +- **A fix round on gate logic is where the next bug comes from.** Session-wide: + **13 review rounds across #25, #28 and #29 — all 13 found something.** Seven of those + findings were defects introduced by the *previous round's fix* (five on #25, two on + #29), twice at HIGH. **No round on any PR came back empty**; #29's fourth pass found + no HIGH or MEDIUM but still found four LOW, including a stale comment and a dead- + argument trap. `safety-critical-changes.md` rule 3 already says to treat "the last + round found nothing" as provisional — this session never reached that state at all, + so the practical question is not when the findings stop but what blast radius + justifies stopping anyway. +- **Stopping has to be calibrated to the blast radius, not the round count.** #25 was a + merge gate — worst case, an unreviewed PR lands. #29 is a reported-never-gating display + field — worst case, a wrong warning. Same review doctrine, different stopping points, + and saying which one applies is part of the merge decision. The `never gates` property + is what made that judgment available, and it was *proved* rather than assumed — by a + review pass sweeping report shapes ad hoc, and in-repo by + `test_review_coverage_is_reported_and_never_gates` plus the 32-combination matrix in + `test_done_keeps_its_original_merge_authorization_semantics`. +- **Reading is not running.** Three defects were invisible to careful reading and obvious + on execution — most sharply, CodeRabbit's pending check reports the zero timestamp, so + the "unmeasurable age fails open" branch was not an edge case but the *only* path that + bot ever took. The #19 guard was dead code for its own target, and only polling the + live PR showed it. +- **Mutation testing found what test names asserted and test bodies didn't.** Five + properties across the session were named in tests or claimed in a PR body and pinned by + nothing: on #29, anchored author matching, newest-review-per-bot, and the `bots=` + threading; on #25, the `init.sh` list-style branch and `grep -qi`'s case-insensitivity. + Each was found by breaking the code and watching the suite still pass. +- **Every whole-file `grep '^ key:'` in a config migration is a bug in two directions** + — it misses the key at another indent *and* matches a same-named key under an unrelated + section. This change shipped one of each. +- **Removing a dangerous mechanism does not make its replacement safe.** After deleting + the list surgery, the replacement *message* still told inline-list adopters to add a + block item — walking them into the same corruption by hand. + +**Open, and owned by nothing yet** + +- **#27's other half** — invalidating a receipt when the diff changes *shape*, not just + when the head moves. The cheap half (visibility) shipped; the faithful half runs into + the same wedge tension as #19/#23. +- **#26** — the fallback panel. Run manually ~10 times this session (5 rounds on #25, + 4 on #29, 1 on #28), two fresh-context lenses per round. CodeRabbit completed only 3 + reviews across 17 pushed heads, so the panel carried most — not all — of the load; one + of its 3 was the round that caught the "ACTION NEEDED" bug. Highest-value unbuilt thing + in the tracker. + +▶ Next: **#26** (make the fallback a panel spec). CodeRabbit was rate-limited on nearly +every head all day (3 completed reviews across 17 heads), so the panel carried most of +the review load — and it is still two manual subagent launches per round, ~10 rounds this +session. Two things this session learned belong in its prompt: +require the lens to **execute** the changed paths (three defects were invisible to +reading and obvious on running), and to **mutation-test** new branches (that is what +proved five separate properties were named by tests and pinned by nothing). + ### 2026-07-25 (Phase 3a) **Theme —** Made the Phase 3 sequencing decision, and it changed under scrutiny — twice. diff --git a/docs/kit-handoff.md b/docs/kit-handoff.md index 82de8d3..2bcd977 100644 --- a/docs/kit-handoff.md +++ b/docs/kit-handoff.md @@ -14,10 +14,72 @@ > 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-26 — a second adopter adopted, and the kit's own fixes for -what that found regressed twice before being deleted. +Last updated: 2026-07-27 — two of three adopter-facing `kit_doctor` lies fixed; the +third was built, reviewed, and deleted. + +## Latest session — 2026-07-27 (#59 + #61.1 shipped; #61.2 built and reverted) + +**Theme —** Fixed what the cs-toolkit adoption found in `kit_doctor`. The review panel +ran twice and found **28 defects, four of them regressions against `main`** — my change +making things *worse* than the code it replaced. None were caught by CI, 372 passing +tests, my own mutation runs, or CodeRabbit. + +- **`#65` merged (`a18f085`), closing `#59`.** The engines probe derives from + `KIT_OWNED` instead of naming three files; a quoted `kit.version` no longer crashes + the report, an unreadable one says so instead of advising a migration, and it exits 2 + because CI gates on that code. +- **`#61`'s hook-detection half was deleted, not shipped.** Asking git resolved the + false negatives and introduced worse: answering from an *enclosing* repository when + `--root` was not one, and honoring an inherited `GIT_DIR`. `_hook_dirs` is + byte-identical to `main`; only its docstring changes, to state the gaps as known. +- **`#66`, `#67` filed** — both `init.sh`, both found by the panel while reviewing + something else. -## Latest session — 2026-07-26 (cs-toolkit Phase 1; kit lint; #60 attempted and reverted) +**Decided** + +- **The detector must resolve the same way as the writer.** Probing settled a + disagreement the kit shipped with: `rev-parse --git-path hooks` *does* honor + `core.hooksPath` and tilde-expands it; `git config --get` does not. `init.sh`'s + comment asserts the opposite, and it installs an **inert hook** for a `~`-form path. +- **Blast radius, not round count — and say which you applied, in the PR.** A read-only + report's worst case is a wrong message, so two panel rounds with decaying severity is + proportionate. What that does *not* cover was written down too. +- **Rule 1 applied to a half, not a PR.** Two failed shapes for hook detection ⇒ revert + that half and ship the rest, rather than tightening a third time or holding #59. + +**Learned** + +- **My claims were wrong three times, in the permanent record each time.** A commit + message attributed a defect to the reverted half when it *ships*; a PR table misstated + `main`'s behaviour; and the sentence explaining that I had removed a `Closes #61` + keyword **contained the keyword**, which GitHub matches regardless of context — the + exact trap the friction log recorded last session, re-armed while documenting it. +- **Two of my tests pinned nothing**, including the one whose stated thesis is "don't + restate the list": it re-derived its expectation from the real `KIT_OWNED` with the + prefix filter left out, so deleting that filter left the suite green. +- **A mutation harness must restore in a `finally`.** Mine died parsing pytest output + and left the file mutated — `#50`'s hazard by a route `#50` does not describe. +- **CodeRabbit is incremental**, so a force-pushed or substantially rewritten PR keeps a + stale review and reports nothing new. Its pass covered the pre-split head only; + `bots_behind_head` recorded that rather than waving it through. + +**Open, and owned by nothing yet** + +- **Three `init.sh` defects** — `#62` (unquoted YAML stamping), `#66` (inert `~` hook), + `#67` (the same hardcoded triple `#59` just fixed, where it *writes* bad config). + `init.sh` still has zero test coverage (`#36`). +- **`#61`** — reopened scope: the hook-detection half, with the panel's evidence, the + 9-form parsing table, and the shape a correct fix needs. +- **`#47`** still the highest-leverage unbuilt thing, and it subsumes `#67`. +- **`#50`, `#60`** unchanged. + +▶ Next: **`#67` + `#62`, behind a first `init.sh` fixture test (`#36`)** — three +`init.sh` bugs are now open and the file has no coverage, so the harness is the +unblocking step, not the fixes. `#66` needs the `#61` design call and should follow. + +______________________________________________________________________ + +## Earlier session — 2026-07-26 (cs-toolkit Phase 1; kit lint; #60 attempted and reverted) **Theme —** Adopted the kit into cs-toolkit (its ancestor), then fixed upstream what the adoption found. **Everything the review panel caught had already passed my own @@ -257,101 +319,6 @@ cs-toolkit is the larger job — an *adoption*, not an upgrade, plus a fixer cha ______________________________________________________________________ -## Earlier session — 2026-07-25 (Phase 3b) - -**Theme —** Fixed #19 + #23 together, and then spent most of the session discovering -that **the fix rounds were more dangerous than the original diff**. Seven review rounds -on #25 alone; every one found something real; five of them found a defect introduced by -the previous round's fix. - -- **#25 merged — #19 and #23 closed.** `summarize_review_bots` resolves each configured - bot to *unavailable* (outage announced on a comment body **or** a status-check - description — the surface that was invisible) or *pending* (a verdict still coming). - Pending blocks the merge gate until it ages past `review.bot_pending_grace_minutes`; - unavailable never blocks and is the action signal. **Nothing reaches `converged`** — - that is the whole design, and it is what let both be fixed at once. -- **#28 merged — #10 closed.** The state_paths suite failed from inside a lane worktree - because its fixture cleared every sandbox *env* signal and not the *cwd* one. The - issue predicted a per-test audit would be needed; it wasn't — every cwd-sensitive test - already chdirs itself. A mutation pass showed the fix makes the suite *stricter*: - three tests had been passing by accidentally discovering the real repo root. -- **#29 merged — half of #27.** `review_bots.coverage` reports which commit each bot's - last review actually saw, and `--record-review` records it as `bots_behind_head`. - Verified against the real #25, where it reproduces the gap I had had to work out by - hand an hour earlier. #27 stays open for the shape-change half. -- **#26 and #27 filed** from the friction-log inbox, both with concrete sketches. - -**Decided** - -- The anti-wedge property lives in `converged` alone. Every new signal feeds the merge - gate, which already needs an explicit receipt — so a gate can wait, but the poll/fix/ack - loop can always finish. -- **Report, never gate.** All three new fields (`signal`, `bot_signal`/`override`, - `coverage`) make an omission legible at merge time and block nothing. The faithful - version of each risks wedging a repo whose bot is permanently unavailable. -- **Stop patching a mechanism that keeps corrupting.** The `init.sh` marker migration - produced **three** distinct config corruptions across three rounds, each while its own - post-conditions passed and it printed success (the fourth round's finding was about - the *replacement message*, not the surgery). It was deleted rather than patched again; - `init.sh` now detects the gap and prints what to add. - -**Learned** - -- **A fix round on gate logic is where the next bug comes from.** Session-wide: - **13 review rounds across #25, #28 and #29 — all 13 found something.** Seven of those - findings were defects introduced by the *previous round's fix* (five on #25, two on - #29), twice at HIGH. **No round on any PR came back empty**; #29's fourth pass found - no HIGH or MEDIUM but still found four LOW, including a stale comment and a dead- - argument trap. `safety-critical-changes.md` rule 3 already says to treat "the last - round found nothing" as provisional — this session never reached that state at all, - so the practical question is not when the findings stop but what blast radius - justifies stopping anyway. -- **Stopping has to be calibrated to the blast radius, not the round count.** #25 was a - merge gate — worst case, an unreviewed PR lands. #29 is a reported-never-gating display - field — worst case, a wrong warning. Same review doctrine, different stopping points, - and saying which one applies is part of the merge decision. The `never gates` property - is what made that judgment available, and it was *proved* rather than assumed — by a - review pass sweeping report shapes ad hoc, and in-repo by - `test_review_coverage_is_reported_and_never_gates` plus the 32-combination matrix in - `test_done_keeps_its_original_merge_authorization_semantics`. -- **Reading is not running.** Three defects were invisible to careful reading and obvious - on execution — most sharply, CodeRabbit's pending check reports the zero timestamp, so - the "unmeasurable age fails open" branch was not an edge case but the *only* path that - bot ever took. The #19 guard was dead code for its own target, and only polling the - live PR showed it. -- **Mutation testing found what test names asserted and test bodies didn't.** Five - properties across the session were named in tests or claimed in a PR body and pinned by - nothing: on #29, anchored author matching, newest-review-per-bot, and the `bots=` - threading; on #25, the `init.sh` list-style branch and `grep -qi`'s case-insensitivity. - Each was found by breaking the code and watching the suite still pass. -- **Every whole-file `grep '^ key:'` in a config migration is a bug in two directions** - — it misses the key at another indent *and* matches a same-named key under an unrelated - section. This change shipped one of each. -- **Removing a dangerous mechanism does not make its replacement safe.** After deleting - the list surgery, the replacement *message* still told inline-list adopters to add a - block item — walking them into the same corruption by hand. - -**Open, and owned by nothing yet** - -- **#27's other half** — invalidating a receipt when the diff changes *shape*, not just - when the head moves. The cheap half (visibility) shipped; the faithful half runs into - the same wedge tension as #19/#23. -- **#26** — the fallback panel. Run manually ~10 times this session (5 rounds on #25, - 4 on #29, 1 on #28), two fresh-context lenses per round. CodeRabbit completed only 3 - reviews across 17 pushed heads, so the panel carried most — not all — of the load; one - of its 3 was the round that caught the "ACTION NEEDED" bug. Highest-value unbuilt thing - in the tracker. - -▶ Next: **#26** (make the fallback a panel spec). CodeRabbit was rate-limited on nearly -every head all day (3 completed reviews across 17 heads), so the panel carried most of -the review load — and it is still two manual subagent launches per round, ~10 rounds this -session. Two things this session learned belong in its prompt: -require the lens to **execute** the changed paths (three defects were invisible to -reading and obvious on running), and to **mutation-test** new branches (that is what -proved five separate properties were named by tests and pinned by nothing). - -______________________________________________________________________ - > Older session entries (below the live blocks above) live in [`kit-handoff-history.md`](kit-handoff-history.md). > Active open items from them are folded into the "Open for next session" lists above. From 30ab5739f081807065ed64097161ee2266691de4 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Mon, 27 Jul 2026 07:36:55 +0300 Subject: [PATCH 2/3] docs: fix a cross-reference this sweep broke, and log the reproduction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 2026-07-27 archive run moved the Phase 3b block into the history file, which made a sentence already IN that file — "see the Phase 3b block, still live in kit-handoff.md" — false in a second way: the target is now in the same document as the reference. The friction log already carried the sweep's cross-reference defect from the previous run. That entry proposes warning on (above|below) references inside MOVED blocks; this instance is the mirror case, where the moved block was the reference's target rather than its location, so the check has to cover both. Claude-Session: https://claude.ai/code/session_01PWoYsKTB9fBp4knZbXqbHc --- docs/kit-friction-log.md | 10 ++++++++++ docs/kit-handoff-history.md | 6 +++--- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/docs/kit-friction-log.md b/docs/kit-friction-log.md index 79dd0ca..265cb2d 100644 --- a/docs/kit-friction-log.md +++ b/docs/kit-friction-log.md @@ -72,6 +72,16 @@ Everything above now lives in [`kit-friction-log-archive.md`](kit-friction-log-a mutation run, and CodeRabbit all missed**. **No change proposed** — recording it because `fallback-review-panel.md` argues this from one prior data point, and this is a second, stronger one worth citing there. +- **The archive sweep broke a cross-reference again — and this time it broke one the + PREVIOUS sweep had written.** The 2026-07-26 entry below reports the sweep orphaning + *"see the Phase 3b block above"*. Today's sweep moved Phase 3b itself into the history + file, so the surviving text — *"see the Phase 3b block, still live in + kit-handoff.md"* — became false in a second way: the target is now in the very file + the sentence sits in. Fixed by hand in this commit. **M** — the entry below proposes + warning on `(above|below)`-style references in *moved* blocks; this instance shows the + warning must also cover references **into** a moved block from anywhere in either + document, since the block that moved was the reference's *target*, not its location. + Same family as `#53`. - **The doc-budget remedy no-op reproduced a third time** (see the 2026-07-26 entry below). `check_doc_budget` reported 421/400; the remedy it names printed *"nothing to move: 5 session block(s) <= --keep 6"*; `--keep 4` was needed and found by guessing. diff --git a/docs/kit-handoff-history.md b/docs/kit-handoff-history.md index 99cafe1..0b37ad4 100644 --- a/docs/kit-handoff-history.md +++ b/docs/kit-handoff-history.md @@ -181,9 +181,9 @@ Both times the correction came from asking what a *stale reader* of the mechanis proposed fix, so they are issue-shaped — `triage-friction-log` should graduate them rather than leaving them in the inbox. -✔ Done — shipped as PR #25 (see the Phase 3b block, still live in -[`kit-handoff.md`](kit-handoff.md)). The design constraint held: -neither surface's fix touches `converged`. +✔ Done — shipped as PR #25 (see the Phase 3b block, swept into this file by the +2026-07-27 archive run). The design constraint held: neither surface's fix +touches `converged`. ### 2026-07-25 From f3d4e6ef6e411d33d960adbaa594a638c843da75 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Mon, 27 Jul 2026 07:52:09 +0300 Subject: [PATCH 3/3] docs: fix eight claim errors a review lens found in this handoff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The wrap-up block asserted things the artifacts contradict. Corrected: · `#36` MISCITED. It is pre-push's coverage gap, not init.sh's, and the handoff had promoted that error into its ▶ Next line. No issue tracks init.sh coverage at all — filing one is now the first step, and #67's body (where I first made the error) is corrected too. · "GitHub matches a closing keyword regardless of context" is FALSE, and measuring it changed the proposed fix. closingIssuesReferences is empty for a PR body carrying a backticked `Closes #61`; #63's un-backticked "Does NOT close #60" did fire. Inline code spans are excluded, so the hazard is negated PROSE — and the grep this log proposed would have fired on every legitimate backticked reference. · "372 passing tests missed the four regressions" — the 372-test suite did not exist yet. It contains 15 tests written in RESPONSE to those findings. The suite that missed them had 355, and the mutation run credited alongside it ran against a tree the regressions had already been reverted from. · "four regressions" was asserted and never enumerated; now named. · "sixth consecutive launch" — 4 last session + 5 this session is 9, and the entry contradicting it sat in the same file. · contract item 5 does not warn about stale .pyc (that is #50 alone); it warns about false kills. Both frame the hazard as a wrong RESULT, and neither says the repo may be left broken — which is the actual gap. · a round-2 finding was credited to round 1, in the entry whose whole point is per-round disjointness. It could not have been round 1: the defect was a false sentence in the commit message describing round 1's outcome. · "byte-identical to main" said of a function whose docstring grew 25 lines; the BODY is byte-identical. "#61 reopened" — it was never closed. And the "9-form parsing table" is a 5-row table of 9 forms. Also fixed: a cross-reference in kit-handoff-history.md that this session's own archive sweep invalidated, and which the previous sweep had written. Claude-Session: https://claude.ai/code/session_01PWoYsKTB9fBp4knZbXqbHc --- docs/kit-friction-log.md | 65 ++++++++++++++++++++++++---------------- docs/kit-handoff.md | 45 ++++++++++++++++++---------- 2 files changed, 68 insertions(+), 42 deletions(-) diff --git a/docs/kit-friction-log.md b/docs/kit-friction-log.md index 265cb2d..c3ec722 100644 --- a/docs/kit-friction-log.md +++ b/docs/kit-friction-log.md @@ -39,23 +39,31 @@ Everything above now lives in [`kit-friction-log-archive.md`](kit-friction-log-a - **A mutation harness that restores outside `finally` leaves the repo mutated.** My restore line was unreachable when the script died parsing pytest output (a bare `python3` with no pytest returned no stdout, and `splitlines()[-1]` raised). The - working tree kept a live mutant until `git status` caught it. `#50` and - `fallback-review-panel.md` contract item 5 both warn about mutation testing, but only - about **stale `.pyc`** — this is the same outcome by a different route, and the - `.pyc` framing is what made me think a hash-verified restore was sufficient. **H** — + working tree kept a live mutant until `git status` caught it. Neither existing warning + covers it: `#50` is about stale `.pyc`, and `fallback-review-panel.md` contract item 5 + warns only about **false kills** from a checksum/drift test (it does not mention + `.pyc` at all). Both frame the hazard as "the result may be wrong"; neither says the + repo may be left broken. **H** — proposed fix: contract item 5 should say the restore belongs in a `finally` (or a git-clean check) and that any harness must verify the tree is clean *after* it exits, not only after a successful run. -- **A negated closing keyword got re-armed while documenting the last one.** Last - session's entry says "never write a closing keyword adjacent to an issue number you - do not intend to close, even negated". This session I wrote a PR body sentence - explaining that I had *removed* a `Closes #61` — and the sentence contained the - literal keyword, which GitHub matches regardless of context. Caught by grepping my own - text, not by remembering the rule. **M** — proposed fix: the rule needs a mechanical - check, not a habit: a `pr-watch` / `wrap-up` step that greps the PR body and every - commit message for `(close|fix|resolve)\w*\s+#\d+` and asks the operator to confirm - each match is intended. A rule that fires only when you remember it does not survive - the session that is busy applying it. +- **The closing-keyword rule as written is wrong, and measuring it changed the fix.** + Last session's entry says never to write one "even negated". Acting on that, I rewrote + a PR body sentence that mentioned a backticked `` `Closes #61` ``. Then I measured: + + ``` + PR #68 body contains `Closes #61` (backticked) → closingIssuesReferences: [] + PR #63 body contains "Does NOT close #60" → closingIssuesReferences: [58, 60] + ``` + + **GitHub excludes inline code spans**; what it does not excuse is negating prose. So + the real rule is narrower than the entry below claims, and the obvious mechanical fix + — grep for `(close|fix|resolve)\w*\s+#\d+` — would fire on every legitimate backticked + reference and train people to ignore it. **M** — proposed fix: a `pr-watch` / `wrap-up` + check that strips fenced blocks and inline code **first**, then flags a closing keyword + within a few tokens of a negation (`not`, `n't`, `NOT`, "stays open"), which is the + actual failure mode. Correct the 2026-07-26 entry's rule at the same time; it is + currently stated more strongly than the evidence supports. - **A review bot with an incremental model keeps a stale review across a rewrite.** CodeRabbit reviewed the pre-split head, then declined `@coderabbitai review` ("does not re-review already reviewed commits") and hit its Fair Usage limit on @@ -67,11 +75,15 @@ Everything above now lives in [`kit-friction-log-archive.md`](kit-friction-log-a changes the diff shape, rather than only at receipt time. Related: `#27`, `#44`. - **The panel's worth is disjointness, and this run measured it.** Round 1: the adversarial lens found the enclosing-repo and `GIT_DIR` regressions; the correctness - lens independently found the tilde disagreement and the misattributed defect. Almost - no overlap, and **four regressions against `main` that CI, 372 tests, my own 19/19 - mutation run, and CodeRabbit all missed**. **No change proposed** — recording it - because `fallback-review-panel.md` argues this from one prior data point, and this is - a second, stronger one worth citing there. + lens independently found the `~`-expansion disagreement between `init.sh` and the new + detector. Almost no overlap. Round 2's correctness lens then found the *misattributed + defect* — a false sentence in the commit message written to describe round 1's own + outcome, which could not have existed when round 1 ran. **No change proposed** — + recording it because `fallback-review-panel.md` argues disjointness from one prior + data point, and this is a second, stronger one worth citing there. Note the honest + framing of what the panel beat: the four regressions were missed by CI, by the suite + **as it then stood**, and by the mutation run **at that head** — not by the 372-test + suite quoted elsewhere, which only exists *because* those findings were fixed. - **The archive sweep broke a cross-reference again — and this time it broke one the PREVIOUS sweep had written.** The 2026-07-26 entry below reports the sweep orphaning *"see the Phase 3b block above"*. Today's sweep moved Phase 3b itself into the history @@ -85,13 +97,14 @@ Everything above now lives in [`kit-friction-log-archive.md`](kit-friction-log-a - **The doc-budget remedy no-op reproduced a third time** (see the 2026-07-26 entry below). `check_doc_budget` reported 421/400; the remedy it names printed *"nothing to move: 5 session block(s) <= --keep 6"*; `--keep 4` was needed and found by guessing. - **No new fix proposed** — recording the third occurrence, because two of the three - were in *this* repo and the entry below still reads as a one-off. It is not. -- **A review lens's isolated worktree pointed at `main` again — 4 of 4 launches, second - session running.** Every lens detected it and cloned the target itself, because the - prompt required reporting the path and diff stat. This is now the *sixth* consecutive - launch across two sessions where the runtime's isolation was wrong and the mandatory - report was the only thing that caught it. **H** — proposed fix: this is no longer a + **No new fix proposed** — recording the third occurrence. The entry below already + records two; this makes it three, two of them in *this* repo, which is a pattern + rather than a run of bad luck. +- **A review lens's isolated worktree pointed at the wrong ref again — 5 of 5 this + session, 9 of 9 across two.** (Last session's entry below records 4 of 4; this session + ran 2 lenses × 2 rounds on `#65` plus 1 on `#68`.) Every lens detected it and cloned + the target itself, because the prompt required reporting the path and diff stat. + **H** — proposed fix: this is no longer a caveat, it is the default behaviour. Contract item 7 should stop saying "verify" and start saying "assume the worktree is wrong; clone the target yourself first", with the path/diff-stat report as a required field of the lens output (the existing 2026-07-26 diff --git a/docs/kit-handoff.md b/docs/kit-handoff.md index 2bcd977..cded8c3 100644 --- a/docs/kit-handoff.md +++ b/docs/kit-handoff.md @@ -20,9 +20,12 @@ third was built, reviewed, and deleted. ## Latest session — 2026-07-27 (#59 + #61.1 shipped; #61.2 built and reverted) **Theme —** Fixed what the cs-toolkit adoption found in `kit_doctor`. The review panel -ran twice and found **28 defects, four of them regressions against `main`** — my change -making things *worse* than the code it replaced. None were caught by CI, 372 passing -tests, my own mutation runs, or CodeRabbit. +ran twice; round 1 found **four regressions against `main`** — my change making things +*worse* than the code it replaced: `--root` on a non-repo answering from the *enclosing* +repository, an inherited `GIT_DIR` overriding it, an unreadable manifest version +degrading from a loud crash to a silent `✓`, and `version: 2.0` going from accepted to a +hard `--generate-manifest` failure. **None were caught** by CI, by the suite as it then +stood (355 tests), by my own mutation run at that head, or by CodeRabbit. - **`#65` merged (`a18f085`), closing `#59`.** The engines probe derives from `KIT_OWNED` instead of naming three files; a quoted `kit.version` no longer crashes @@ -30,8 +33,10 @@ tests, my own mutation runs, or CodeRabbit. because CI gates on that code. - **`#61`'s hook-detection half was deleted, not shipped.** Asking git resolved the false negatives and introduced worse: answering from an *enclosing* repository when - `--root` was not one, and honoring an inherited `GIT_DIR`. `_hook_dirs` is - byte-identical to `main`; only its docstring changes, to state the gaps as known. + `--root` was not one, and honoring an inherited `GIT_DIR`. `_hook_dirs`'s **body** is + byte-identical to `main`; the only change is 25 docstring lines stating the gaps as + known — including a false POSITIVE the revert does *not* fix (`.git/hooks` is appended + unconditionally, so a hook there reports installed even when git reads elsewhere). - **`#66`, `#67` filed** — both `init.sh`, both found by the panel while reviewing something else. @@ -49,11 +54,16 @@ tests, my own mutation runs, or CodeRabbit. **Learned** -- **My claims were wrong three times, in the permanent record each time.** A commit - message attributed a defect to the reverted half when it *ships*; a PR table misstated - `main`'s behaviour; and the sentence explaining that I had removed a `Closes #61` - keyword **contained the keyword**, which GitHub matches regardless of context — the - exact trap the friction log recorded last session, re-armed while documenting it. +- **My claims keep being the defect.** A commit message attributed a defect to the + reverted half when it *ships*; a PR table misstated `main`'s behaviour; and the + wrap-up's own handoff block then miscited `#36`, overstated a test count, and got a + GitHub rule backwards — all caught by a review lens, none by me. This is the third + consecutive session where claim-vs-artifact drift is the most common finding. +- **A closing keyword inside backticks does NOT fire.** Measured: + `closingIssuesReferences` is empty for a PR whose body carries a backticked + `` `Closes #61` ``, while `#63`'s *un-backticked* "Does NOT close #60" closed it. So + the hazard is negated **prose**, not any mention — a grep that ignores code spans + would have over-fired on every legitimate reference. - **Two of my tests pinned nothing**, including the one whose stated thesis is "don't restate the list": it re-derived its expectation from the real `KIT_OWNED` with the prefix filter left out, so deleting that filter left the suite green. @@ -67,15 +77,18 @@ tests, my own mutation runs, or CodeRabbit. - **Three `init.sh` defects** — `#62` (unquoted YAML stamping), `#66` (inert `~` hook), `#67` (the same hardcoded triple `#59` just fixed, where it *writes* bad config). - `init.sh` still has zero test coverage (`#36`). -- **`#61`** — reopened scope: the hook-detection half, with the panel's evidence, the - 9-form parsing table, and the shape a correct fix needs. + `init.sh` has no automated test coverage and **no issue tracks that** — `#36` is the + `pre-push` twin, and `#67`'s body miscites it for `init.sh`; both need correcting. +- **`#61`** — still open, never closed: the hook-detection half, with the panel's + evidence, the shape a correct fix needs, and a table of 9 `git config` value forms of + which the current scan misparses 5. - **`#47`** still the highest-leverage unbuilt thing, and it subsumes `#67`. - **`#50`, `#60`** unchanged. -▶ Next: **`#67` + `#62`, behind a first `init.sh` fixture test (`#36`)** — three -`init.sh` bugs are now open and the file has no coverage, so the harness is the -unblocking step, not the fixes. `#66` needs the `#61` design call and should follow. +▶ Next: **file the `init.sh`-coverage issue, then `#67` + `#62` behind it** — three +`init.sh` bugs are open, the file has zero coverage, and nothing tracks that gap, so the +harness is the unblocking step and it needs a ticket of its own first. `#66` needs the +`#61` design call and should follow. ______________________________________________________________________