Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 64 additions & 33 deletions docs/kit-friction-log.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
107 changes: 100 additions & 7 deletions docs/kit-handoff.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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`.

______________________________________________________________________

Expand Down
Loading