diff --git a/docs/kit-friction-log.md b/docs/kit-friction-log.md index a20b3c5..b4c96be 100644 --- a/docs/kit-friction-log.md +++ b/docs/kit-friction-log.md @@ -11,41 +11,19 @@ > > Tracker board: https://github.com/topij/agentic-dev-kit/issues -## 2026-07-25 — inbox +## 2026-07-25 — Backlog migrated to GitHub Issues + +Two H-severity entries were **removed from this file** and filed as +[#26](https://github.com/topij/agentic-dev-kit/issues/26) (fallback review needs to be a +*panel*) and [#27](https://github.com/topij/agentic-dev-kit/issues/27) (a receipt +survives a redesign its reviewer never saw). #27's cheap half shipped in PR #29; the +issue stays open for the shape-change half. -- **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. +Also closed this session, so their inbox entries below are **done**, kept only for the +trail: **#19** (premature receipt — closed by PR #25) and **#10** (lane-worktree gate +failure — closed by PR #28). -- **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. +## 2026-07-25 — inbox - **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 @@ -67,3 +45,56 @@ reasons teaches agents to ignore a red gate. Already filed as issue #10 — raising severity here because three independent occurrences in one session makes it a pattern, not an incident. + +- **A fix round on gate logic is where the next bug comes from — every time (severity: M, pattern).** + Seven review rounds on PR #25. Every one found something real, and **rounds 3 through 7 + each found a defect introduced by the previous round's fix**: an incomplete poison-clock + fix that still wedged on a *parseable* future date (R3); a section-scoping fix applied + to 1 of 3 guards in the same function (R4); a replacement warning message that walked + inline-list adopters into the corruption the deleted mechanism used to cause (R5); a + style detection that missed a real flow spelling (R6); and a list spelling promoted to + "supported" that the kit's own reader cannot parse (R7). Session-wide: **13 rounds + across three PRs, all 13 with findings, 7 of them self-inflicted by the prior fix.** + `safety-critical-changes.md` rule 3 **already** says "Re-review after every fix round + until a full pass finds nothing new" and that "fix rounds on gate logic routinely + introduce their own regressions" — so the floor is written and was followed. What this + session adds is different, and is what should graduate: (a) a *base rate* — 13/13 rounds + with findings means "until a pass finds nothing new" may never terminate, so the rule + needs a stopping criterion it currently lacks; and (b) the criterion that actually got + used, which is **blast radius, not round count** — a merge gate and a + reported-never-gating display field cannot share a stopping point. + +- **Reading the code is not the same as running it, and the gap is not small (severity: M).** + Three defects this session were invisible to careful reading and obvious on execution: + CodeRabbit's pending check reports `startedAt: 0001-01-01T00:00:00Z`, so an + "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); making `append_to_section` + return non-zero looked plainly correct and aborted `init.sh` under `set -eu` on any + config missing an optional section; and `kitconfig` silently resolves a next-line flow + list to `{}`. **Candidate graduation:** the review-panel prompt (#26) should require + the lens to *execute* the changed paths and to mutation-test new branches — mutation + is what proved **five** properties across the session were unpinned despite tests that + named them (on #29: anchored author matching, newest-review-per-bot, the `bots=` + threading; on #25: the `init.sh` list-style branch and `grep -qi`'s + case-insensitivity). Three of those five are #29's. + +- **Narrative surfaces drift from the diff, and nobody re-reads them (severity: M).** + #25's PR body needed three corrections (a stale test count, and two descriptions of a + design the diff had replaced); #29's asserted an anchored-match property no test pinned; + and **this very wrap-up** was fact-checked and came back with 13 issues, three of them + HIGH — including a number that a review round on #25 had *explicitly corrected* + ("four ways to corrupt" → three) and which I reintroduced while writing up the lesson + that PR bodies keep drifting. Every instance was caught by a review pass, never by the + author. **Proposed fix:** `wrap-up` should fact-check the handoff against `git log` / + `gh` before committing — the handoff is read at the start of every future session, so a + wrong number there propagates further than a wrong PR body. Filing this at M rather + than L because the failure recurred *inside the document describing it*. + +- **The cockpit bundled wrap-up narrative edits into a lane branch, and only the hook caught it (severity: L, but the guard worked).** + While waiting on CI for PR #29 I updated `kit-handoff.md` and `kit-friction-log.md`, then + `git add -A` swept them into the lane commit. `pre-push` refused, named both files, and + said where the lane's handoff belongs instead. Recording it because it is the **positive** + case this log rarely captures: a fail-closed guard firing on its author, with a message + that made the fix obvious. Worth keeping in mind when weighing whether a guard is worth + its friction — this one cost ten seconds and prevented a narrative-file conflict with the + wrap-up PR. No change proposed. diff --git a/docs/kit-handoff.md b/docs/kit-handoff.md index f4e4174..1df6381 100644 --- a/docs/kit-handoff.md +++ b/docs/kit-handoff.md @@ -14,10 +14,105 @@ > 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 — Phase 3a shipped: the watch-loop predicate and the merge gate -are now separate, and the schema change that separates them is purely additive. +Last updated: 2026-07-25 — Phase 3b shipped: the queued-vs-unavailable ambiguity is +closed on both surfaces, and the receipt now says what it does *not* cover. -## Latest session — 2026-07-25 +## Latest 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). + +______________________________________________________________________ + +## Earlier session — 2026-07-25 (Phase 3a) **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. @@ -101,10 +196,8 @@ 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. -▶ 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. +✔ Done — shipped as PR #25 (see the Phase 3b block above). The design constraint held: +neither surface's fix touches `converged`. ______________________________________________________________________