feat(prepare-pr): round notes, a third question, and a retrospective instead of a stall

[iamwhatever]

Problem / Motivation

The prepare-pr loop has no memory between rounds. Each round sees this round's findings and fixes them. It never sees that the code the finding sits in was itself added two rounds ago, to fix an earlier finding.

Four PRs show what that does. #8501 ran 15 pushes and grew from 2776 to 5182 added lines. #8530 ran 11 pushes and grew from 1094 to 2894. #7518 ran 22 pushes and grew from 387 to 2635. #7362 ran 24 pushes and grew from 3328 to 10143, then deleted 2802 of them. In every one, the author's own disposition comments say the same thing: "this was my own regression", "introduced by the previous round's fix". One span on #7362 drew a finding in eight rounds. It closed when the guard was deleted, not patched a ninth time.

Two smaller defects sit under that one. The skill says the "disproportional" answer never applies to a security-class finding, so the agent fixes every one of them, even one whose trigger needs 2001 concurrent artifacts. And the dispositions the agent writes never reach the remote reviewer: the GPT lane's ledger keeps only the marker line, > lines, and - **title** bullets from each comment, and the dispositions on #8530 had zero > lines.

Why it matters

A PR that should take two rounds takes twenty. The maintainer babysits it for days. The code that lands is 2x to 6x the size the change needed, and most of the extra is machinery the loop grew to answer its own earlier fixes. The rebuttal rate on those four PRs was between 4.5% and 18%.

What changed (motivation → approach → change)

The loop needs to look back, so it gets a per-PR note it reads before every round and writes after. scripts/round_notes.py keeps that note under <KIROCREW_HOME>/prepare-pr/<owner>-<repo>/<branch>.md, outside the worktree. init copies the PR's intent once, from --intent text only — it reads no file. add records one round: head, ±lines, each finding with its span=, whether it landed in code this PR added earlier, and the disposition. show prints the note plus a span recurrence table and the self-added count. rm deletes it at Phase 4. prune deletes notes untouched for 14 days at Phase 0. The script recommends nothing and gates nothing.

With that memory, SKILL.md changes in four places. The two questions per finding become three: the new one asks whether the finding lands in code this PR added earlier, and if so, writes down both "repair the mechanism" and "remove the mechanism" before choosing. The security hard limits ("never applies to a reachable Critical/High", "never deferrable") are gone; the three questions apply at every severity, and a real hole still fails question 1. "Escalate at 3 stalled rounds" becomes a retrospective every 3rd round or on a span at x3: one Opus sub-agent gets the note and the diff and answers which mechanisms the intent does not need, then the round subtracts before it fixes. The loop stops for the user only on a decision it cannot make.

A new section tells the agent how to write a disposition the remote reviewer can read: rationale in > lines, written at class level so convergence rule 1 covers the next variant, and a pre-drafted /ai-review override sentence when a security-class finding is honestly disproportional.

Tests

test/test_prepare_pr_round_notes.py (10 tests): show exits 20 with no note; init writes outside the worktree and is idempotent; a base branch is refused; add records deltas, mechanisms, and spans and show --json derives recurrence (c9a9b420 x3) and the self-added count; a malformed --finding exits 2; add before init exits 2; rm and prune delete, and prune leaves fresh notes alone; two branches whose names sanitise alike (feat/x, feat-x) get distinct notes; prune tolerates a note a sibling loop removed first; SKILL.md names every subcommand and the third question.

All 187 existing test_prepare_pr_* tests pass unchanged.

Manual verification

Ran the five subcommands against a throwaway git repo with an isolated KIROCREW_HOME; the note body and show summary match the test expectations. flake8, isort, mypy clean on the new script.

Related Issues

no linked issue: the evidence is the four PRs named above, not a filed issue.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (SKILL.md is the documentation)
• No secrets, credentials, or internal references in the diff

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

Design-level review of 28c10e6aa2f879a3235fd9b47fcea49476280779 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Real, well-evidenced problem; the memory design is sound — but the PR quietly removes every hard human-oversight backstop and replaces each with agent self-judgment.

Watch

• The unconditional runaway cap is gone. "10 iterations is an unconditional runaway backstop" becomes "There is no round cap… raise it with monitor_update" whenever the agent judges the PR "still converging". The pathology this PR documents (a 24-push loop) was one where every round looked like progress to the agent; making the same agent the judge of its own budget extension means the retrospective is now the only brake, and if it underperforms, failure is unbounded instead of capped. Keep a raised-but-finite hard ceiling, or require the user's ack to raise max_cycles.
• Three oversight relaxations land in one PR and all lean on one external backstop: security findings become rebuttable-as-disproportional and deferrable locally only because the GPT lane's convergence rule 5 re-raises them server-side. If that server rule ever loosens, nothing local remains; SKILL.md's description of the ledger/convergence contract is prose with no pinning test, so drift is silent.
• Undocumented scope creep: the Phase-0 decision gate drops "or a diff over ~1k lines" — a maintainer sign-off trigger removed with no mention in the description. Name it there or move it to its own PR.

Suggestions

• self-added:yes|no is self-reported per round; git log -S/blame against the round-0 head could derive it mechanically inside round_notes.py add, removing the one input whose silent omission defeats question 3.

[DESIGN-REVIEWED] 28c10e6

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 28c10e6aa2f879a3235fd9b47fcea49476280779 — this comment is updated in place on each push.

Review details

Both candidates rest on the agent feeding round_notes.py input that deviates from the documented format, in an agent-facing bookkeeping helper.

Candidate 1 — raw.split("|") requires exactly 4 parts, so a | inside the free-text label makes len(parts) != 4 → sys.exit(2). Verified in parse_finding (line 478) and the generator in cmd_add (line 515). But (a) requires "the agent writes a pipe into a few-words label" — an if-a-caller-were-to condition — and (c) is a loud exit 2 with a precise error message, retriable, not silent and not data loss. The tool is rejecting malformed input as designed. Below the 80 bar.

Candidate 2 — parse_finding never validates field 4, so a whitespace disposition is written, while FINDING_RE's (?P<disp>[\w-]+) (line 368) cannot match it, so summarise silently drops the finding. The write/read asymmetry is real and the drop is silent. But the disposition vocabulary is a fixed, closed set (fixed, rebutted, accepted-and-deferred, needs-a-decision) — every documented value matches [\w-]+. The defect manifests only "if the agent invents a multi-word disposition," an if-a-caller-were-to condition that does not occur under documented usage. The candidate itself rates confidence "low." Below the 80 bar.

No stronger self-derived finding survives falsification: _append_mechanism, summarise block-splitting, round numbering, delta computation, and prune's empty-dir cleanup all hold for in-practice inputs.

No findings.

[OPUS-REVIEWED] 28c10e6

Verdict parsed from the review's SHA-scoped output markers for commit 28c10e6aa2f879a3235fd9b47fcea49476280779.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 28c10e6aa2f879a3235fd9b47fcea49476280779: <one-sentence reason>

[github-actions]

GPT 5.6 Review — ✅ human override accepted

Human judgment by @iamwhatever overrides the GPT 5.6 finding for 28c10e6aa2f879a3235fd9b47fcea49476280779; the recorded reason is authoritative for this commit.

This comment is updated in place on each push.

The model was not re-run because an authorized human decision supersedes it.

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 28c10e6aa2f879a3235fd9b47fcea49476280779: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 28c10e6aa2f879a3235fd9b47fcea49476280779 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

I've verified the load-bearing claims: the ADJUDICATION LEDGER really keeps only marker/> /title-bullet lines capped at 12 (.github/workflows/codex-review.yml:435-437), convergence rule 5 really refuses deferral on security findings (.github/review-prompts/gpt-round-convergence.md:26-32), and I counted consumers for the new script's surface. Final review follows.

First-Principles-Verdict: CONCERNS

The decision gate quietly loses its "~1k lines" sign-off trigger — an undeclared policy loosening the round-notes fix neither needs nor replaces.

What this change ships

Intent: stop the prepare-pr loop from inflating PRs 2-6x by giving it cross-round memory. A FIX to loop behavior, shipped as feat.

1. Per-PR round note outside the worktree (round_notes.py init/add/show/rm/prune) — justified (four named PRs)
2. Third question per finding: consider removing self-added mechanisms — justified
3. Security hard limits ("never applies to Critical/High") deleted; questions apply at all severities — justified
4. "Never deferrable" replaced by server-asymmetry note — justified, duplicate of server rule 5
5. Disposition-format section (> lines, class-level, pre-drafted override) — justified, ledger verified
6. Stall-escalation replaced by every-3rd-round Opus retrospective — justified
7. Round cap removed; max_cycles is the only budget, self-raised — declared, watch
8. Decision gate no longer requires sign-off for >~1k-line diffs — undeclared, rides along
9. init falls back to git log -1 for intent — undeclared, contradicts description
10. show --json, init --force — zero consumers

Watch

• Item 8: the description's "What changed" lists four SKILL.md changes; this fifth hunk removes a human pre-open gate. Its zero option (keep the trigger) costs the fix nothing, and round notes measure growth during review, not initial size — nothing replaces it. Drop the hunk.
• Item 7: "raise it with monitor_update while the PR is still converging" makes the loop the judge of its own budget. The retrospective is the cause-level fix and the old cap was demonstrably ignored (15-24 pushes), but the only remaining bound is now self-adjusted.
• Item 9: description says intent comes "from --intent text only — it reads no file", yet cmd_init falls back to git log -1; SKILL.md always passes --intent, so the fallback has zero consumers and contradicts the framing.

Subtractions

• Delete the git log -1 fallback in cmd_init (round_notes.py:450-453); make --intent required, matching the description.
• Drop init --force — 0 consumers (grepped --force in SKILL.md: only --force-with-lease; tests never pass it).
• Drop show --json — 0 consumers outside tests (grepped --json in SKILL.md: 5 hits, all gh pr view); plain show prints the same summary.

[FIRST-PRINCIPLES-REVIEWED] 28c10e6

[iamwhatever]

rebutted — span=67f250492cae

• Untrusted finding titles are interpolated into shell commands

Not a defect: the only caller of round_notes.py add is the agent composing the command on its own machine, where it already executes arbitrary shell; a label it writes into its own note adds no capability the caller lacks.
The <label> field is the agent's own few-word tag, not the reviewer's title. SKILL.md at 28c10e6aa now says so in the add step: "never the reviewer's title pasted in: reviewer text is PR-influenced and does not belong on your own command line."
This ruling covers every finding whose chain is "PR-influenced text reaches a round_notes.py argument": the note is author-side bookkeeping written by the process that would be the attacker, and structured stdin for a two-word label is machinery the skill exists to refuse.

Override, if the fence still holds on the new head:
/ai-review override gpt 28c10e6aa: the only caller of round_notes.py add is the agent on its own machine, where it already runs arbitrary shell; the label is agent-authored, not reviewer text, and structured input for a two-word tag is exactly the mechanism this PR argues against.

[iamwhatever]

fixed — span=677a9f6493d3

• relative KIROCREW_HOME resolves from the worktree and can dirty the branch

Legitimate. notes_root() now returns root.resolve() / "prepare-pr" (round_notes.py, 28c10e6aa), so a relative KIROCREW_HOME is anchored before the notes directory is appended and the note can never land inside the checkout.

[iamwhatever]

fixed — span=677a9f6493d3

• duplicate same-span findings in one round make rounds reach three and trigger a false retrospective

Legitimate. summarise() appends a round number to a span once per round (if rno not in entry["rounds"], round_notes.py, 28c10e6aa); two findings on one span in one round count as one hit. Pinned by the extra same-round --finding in test_add_records_rounds_deltas_spans_and_mechanisms, which still expects rounds == [0, 1, 2].

[iamwhatever]

rebutted — span=682fb57e2fa4

• Concurrent loops overwrite round history

Not a defect in this design: one prepare-pr loop owns one PR branch (SKILL.md "Which mechanism drives the loop": one monitor_start per session, one session per PR). Two loops on one branch is the failure the file-overlap gate at Phase 0 exists to stop, not a state the note is asked to survive.
The casualty, if it ever happened, is one entry in an author-side advisory log (round_notes.py:25: "recommends nothing and never gates anything") — never code, never the PR, never the ledger CI reads. The next add re-derives nothing from it; the agent re-enters the lost round from the PR comment thread, which is the durable record.
This ruling covers every finding whose chain is "concurrent writers to the per-branch note": the note has one writer by construction, and a cross-process lock portable across three OSes is ~30 lines of machinery guarding a two-line bookkeeping write — the exact shape this PR argues the loop must learn to refuse.

The adjudicator reached the same conclusion and FLAGged it; its own rationale is the override:
/ai-review override gpt 28c10e6aa2f879a3235fd9b47fcea49476280779: requires two concurrent prepare-pr loops on the same PR branch (contradicting the single-owner-per-PR design) colliding in a once-per-round sub-millisecond read-modify-write window, and the only casualty is one entry in a deliberately non-gating advisory log, never code, the PR, or user data.

[iamwhatever]

/ai-review override gpt 28c10e6: Requires two concurrent prepare-pr loops on the same PR branch (contradicting the single-owner-per-PR design) colliding in a once-per-round sub-millisecond read-modify-write window, and the only casualty is one entry in a deliberately non-gating advisory log (line 25), never code, the PR, or user data.

[github-actions]

Human judgment recorded

@iamwhatever marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 28c10e6aa2f879a3235fd9b47fcea49476280779.

Requires two concurrent prepare-pr loops on the same PR branch (contradicting the single-owner-per-PR design) colliding in a once-per-round sub-millisecond read-modify-write window, and the only casualty is one entry in a deliberately non-gating advisory log (line 25), never code, the PR, or user data.

This decision applies only to this commit. A new push requires a new judgment.