diff --git a/docs/kit-friction-log.md b/docs/kit-friction-log.md index 7204614..c3ec722 100644 --- a/docs/kit-friction-log.md +++ b/docs/kit-friction-log.md @@ -34,6 +34,82 @@ 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. 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. +- **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 + `@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 `~`-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 + 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. + **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 + 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..0b37ad4 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. @@ -88,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 diff --git a/docs/kit-handoff.md b/docs/kit-handoff.md index 82de8d3..cded8c3 100644 --- a/docs/kit-handoff.md +++ b/docs/kit-handoff.md @@ -14,10 +14,85 @@ > 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; 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 + 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`'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. -## 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 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. +- **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` 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: **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. + +______________________________________________________________________ + +## 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 +332,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.