refactor(prepare-pr): split rationale out and fix six self-contradictions

[iamwhatever]

Problem / Motivation

prepare-pr is one of the most-loaded skills in the repo, and its SKILL.md had grown to 68,690 bytes of mixed content: normative instructions, script-behaviour documentation, and post-mortem evidence all on one plane with no separation.

Six places contradicted themselves outright, so an agent's behaviour depended on which paragraph it happened to weight:

1. monitor_start arming. "The acknowledgement is not evidence the loop armed — and it cannot be" was followed four sentences later by "a plain requested acknowledgement is the success signal, so ending the turn on it is correct."
2. Whether cron may drive the loop. Phase 3 step 5 said to prefer the pr_watch script cron for a pure-watch stretch; the closing section said "Do not hand the iterate-on-review-feedback loop to a cron job." The two were separated only by the undefined terms "pure-watch" and "iterate-on-review-feedback".
3. Four unaligned iteration numbers. Outer cap 10, Phase-2 inner cap 10, escalate at ~3 stalled rounds, max_cycles=80. Escalation fires at 3, which makes 10 unreachable in any healthy run — yet 10 appeared six times and was written into the monitor_start message template as a target.
4. A self-negating freeze. "Once the PR is open, the diff is FROZEN — but the freeze is about SCOPE, never about correctness" negated its own headline in the same sentence, and sat under Phase 0 (Preflight) while describing post-open behaviour.
5. Mode selection had no precedence. The prepare-only and full-loop trigger lists overlapped ("update the PR" vs "make it green") with no rule for a request carrying both, so "push this and make it green" had no defined mode.
6. A distinction with no operational difference. ~1,100 characters argued rebut vs push back, then stated both record as the single rebutted disposition and that push-back "is not a fifth disposition" — a taxonomy discussion about text that produces identical actions.

Why it matters

Every byte here is injected on every invocation of the skill, and this skill runs on most PRs in the repo. Three costs compound:

• Contradictions produce nondeterminism where the skill is load-bearing. test: validate CI workflows on KiroCrew #1 is the worst: reading it as "not evidence" fires the wait fallback on every successful arm, reinstating the exact 2-hour turn timeout the monitor_start branch exists to remove. refactor: remove dead legacy compatibility shims #3 means the runaway backstop reads as a target, so a loop can burn ten rounds without the operator learning it should have escalated at three.
• Rationale crowds out instruction. Roughly 55–60% of the file was prose whose removal leaves behaviour identical: incident anecdotes ("the 2026-07-31 clobber where a force-push replayed 114 duplicate commits", "101 runs over 25 hours with 23 approval blocks"), script internals the agent only reads exit codes for, and rules restated three or four times each (answer-every-concern ×3, the disposition taxonomy ×4, proportionality ×4, the single_commit skip ×4).
• The body and the frontmatter disagreed on which phrasings mean prepare-only, and the frontmatter is what decides whether the skill loads at all.

What changed (motivation → approach → change)

Root cause: the file had no layering. A rule, the reason for the rule, and the evidence that motivated it were all written at the same level, so de-duplication was impossible without deciding what each sentence was for.

Approach: split by function, following the precedent references/gate-floor.md already set in this same skill — and which test_gate_rationale_reference_exists_and_is_pointed_at explicitly endorses ("the reasons each gate is shaped the way it is moved to a reference file so they do not dilute the operational instructions on every skill load"). SKILL.md keeps only what the loop executes; the reasons move to a sibling reference read on demand, when an agent needs to justify a deviation rather than on every load.

Change:

• SKILL.md 68,690 → 38,054 bytes (−45% of per-invocation load).
• New references/rationale.md (14 KB) carrying the incident evidence, script internals, and design history — including why each of the six contradictions was resolved the way it was.
• All six contradictions fixed:
  • Arming is now one rule with two branches: a synchronous refusal → fall back to the in-turn wait loop and say no loop is running; any other reply, including a bare requested → treat as armed and end the turn.
  • One cron rule in one place, with the exemption's precondition stated: pr_watch only when you have nothing to answer (no open concern, no bot post still expected), because it reads no comment bodies.
  • Escalate at 3 is the operative limit; 10 is named a runaway backstop, not a target, and a loop reaching it "has already missed an escalation trigger."
  • The freeze moved to Phase 3 and reworded as the three push justifications, dropping the self-negation.
  • A mode decision table plus an explicit precedence rule (ship beats stop beats default; a stop signal only wins when it is the whole ask), and the body list now matches the frontmatter's instead of offering a narrower one.
  • Proportionality collapsed to "two questions, two outcomes"; the taxonomy argument moved to rationale.md.
• Two rules restored that the first pass trimmed along with their surrounding prose: pr_status.py's exit-20 marker semantics (advisory FINDING counts never gate) and the at-most-two-commits allowance.

Review rounds on this PR found two more dropped guards, both fixed here:

• The reviewer-fleet pin (--reviewers / PREPARE_PR_REVIEWERS) had been dropped entirely. resolve_reviewers()'s own docstring is explicit that without it pr_status.py runs discovery mode, where "a lane that never posted is not required" — so a profile declaring gpt+opus could reach exit 0 with no Opus review at all, and Phase 4 arms auto-merge on that. Restored in three places: the script table, the poll invocation, and the monitor_start message template (the template mattered — the loop would otherwise drop the pin on every subsequent round).
• The first-push skip of the clobber check. Dropping "skip the clobber check entirely" for the empty-LEASE_SHA case left a rule that runs git merge-base --is-ancestor against a ref that does not exist yet and reads the failure as a maintainer commit — permanently blocking the first push on any new branch, including this PR's own.

Tests

No new tests. This diff is prose in an agent instruction file; the behaviour it governs is already pinned by existing tests, and those pins are what constrained the rewrite:

• test/test_ai_review_workflows.py::TestPreparePrPreSubmitReview — phase ordering (Reconcile code and description. < Local review — one subagent per profile reviewer < Push only the reviewed commit.), the REVIEWED_SHA fail-closed strings, both reviewer contract paths, Record dispositions. before loop back to Phase 1, and all four disposition names.
• test/test_deferred_disposition_ratchet.py — the vocabulary ratchet, including the per-line rule that no line may offer accepted-and-deferred without needs-a-decision alongside it.
• test/test_prepare_pr_profiles.py — charter budget parity against .github/review-prompts/ (≤5 BLOCKING, ≤6 advisory FINDING, GPT's report-ALL with no numeric cap) and the references/gate-floor.md pointer in both directions.
• test/test_prepare_pr_local_review.py — the extractor stays the default and the hand-written charters stay the fallback.

751 tests across those plus test_prepare_pr_status.py, test_skills.py, test_builtin_skill_packaging.py, test_builtin_skill_sync_safety.py and test_brand_name_gate.py pass on this commit.

Manual verification

N/A for runtime behaviour — no executable code changed. What was verified instead:

• Full backend suite run and diffed against a pristine base worktree at the same SHA: zero new failures. The 103 failures present are identical on both sides (AF_UNIX path too long, test_xdist_host_budget cap, env-dependent test_source_providers); one base flake actually passed on the branch. Comparison done by node id, over whole files rather than parametrized ids.
• Local gates green: brand name, changelog history, docs lint, scrub lint, harness parity, lockdown, run_scoped_tests --test. The brand gate caught two prose KiroCrew → Kiro Crew spellings, fixed.
• Both model-pinned reviewers run against the CI-extracted contracts (gpt-5.6-sol mirroring codex-review.yml, claude-opus-4.8 mirroring claude-review.yml + base-ref AUTOSDE.yaml). GPT found the two dropped guards above; Opus then verified token survival rule-by-rule across both files and returned no BLOCKING, with one low-confidence advisory (the single_commit conditional centralized away from its steps) which is applied as a pointer on each of the three governed steps.

Related Issues

no linked issue: self-initiated refactor of an agent instruction file, no tracked defect.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

Why no screenshot: the diff touches only two markdown agent-instruction files under src/kiro_crew/builtin_skills/; nothing renders in the dashboard.

[iamwhatever]

Both findings from the local gpt-5.6-sol pre-submit pass are fixed. Each was verified against pr_status.py's own code and against origin/main's SKILL.md before being accepted — not taken on the reviewer's word.

• Required reviewer-fleet pin was removed — fixed in f484111cd, carried into the pushed head 3cb63f2ed.

  Profile requires GPT+OPUS -> bare pr_status.py uses discovery mode -> absent OPUS emits no stamp and is not required -> exit 0 can permit Phase 4 and auto-merge without that review.

  Confirmed legitimate and reachable. resolve_reviewers()'s docstring states the mechanism directly: without --reviewers / PREPARE_PR_REVIEWERS it runs discovery mode, and evaluate_reviewer_markers() documents that in discovery mode "a lane that never posted is not required". origin/main's SKILL.md carried the flag; the rewrite had zero mentions of it, so this was a genuine dropped guard rather than a rewording. Fixed in three places, because one would not have held: the scripts table now says "scope AND require the fleet", the Phase 3 poll step invokes pr_status.py <pr#> --reviewers <profile reviewer names> with the discovery-mode hazard stated inline, and the monitor_start message template carries the flag too — without that third edit the loop would have silently dropped the pin on every round after the first.

• First-push clobber check no longer skips an absent remote branch — fixed in f484111cd, carried into the pushed head 3cb63f2ed.

  Fresh feature branch with no origin/<branch> -> rev-parse/merge-base fails -> failure is misclassified as a maintainer commit -> the workflow stops and cannot make its first push.

  Confirmed legitimate, and demonstrably reachable: this PR's own first push is exactly that case. origin/main had "skip the clobber check entirely" for the empty-LEASE_SHA case and the rewrite dropped the clause, leaving a rule that runs git merge-base --is-ancestor against a ref that does not exist and treats the resulting failure as a maintainer commit on the remote. Phase 3 step 1 now states the first-push branch before the check rather than after it, and says why running it anyway misclassifies the failure.

Local gates, the 751 pinning tests, and both push_guard.py modes are green on 3cb63f2ed. The branch was rebased from 98e7fb8bd onto ba1939524 after these fixes — push_guard.py --require-single-on-base caught the stale base and refused the push, which is the guard working as intended; none of main's six new commits touch the files in this diff.

[iamwhatever]

The local claude-opus-4.8 pre-submit pass returned no BLOCKING findings, having verified rule-by-rule that every operational guard in origin/main's SKILL.md survives in either the new SKILL.md or references/rationale.md. It raised one advisory, dispositioned below.

• single_commit=true conditional centralized away from its operational steps — fixed in 3cb63f2ed.

  Phase 1.3/1.4 and Phase 3.1 no longer carry the inline "only when single_commit = true / skip for false" qualifier the original had on each step. Trigger: a single_commit=false profile. Consequence: an agent following Phase 1 literally on a multi-commit branch runs push_guard.py --base, hits exit 40, and STOPs to "diagnose branch history" — unless it applied the centralized skip rule from the profile section.

  Accepted even though the reviewer marked it low confidence and noted it is inert for the only bundled profile (Kiro Crew is single_commit = true). The reasoning: removing that four-times repetition was a stated goal of this PR, but a step that is silently conditional is the same ambiguity class the PR set out to remove, and the fix does not have to reintroduce the repetition. Each of the three governed steps now carries a pointer — (single_commit only — see above) on Phase 1.3 and 1.4, (single_commit only — see the profile section) on Phase 3.1 — which is a cross-reference, not a restatement of the rule. The governing sentence was reworded from "Nothing else in this file repeats that condition" to "The three steps it governs point back here rather than restating it", so it stays accurate now that the pointers exist.

Its two "token dropped" observations were checked and are correctly non-guards: dangerouslySetInnerHTML was only an example inside the surviving "reproduce the grep rules in code-review.yml" rule, and headRefOid survives in rationale.md as part of the DISCARDED re-fetch mechanism.

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Real harm (contradictory per-invocation instructions), fixed at root cause via the test-endorsed gate-floor layering precedent; fully reversible prose with pinned behavior.

Spot-checked the split: the operational halves of every rationale entry survive in SKILL.md (SHA-pinned lease + first-push skip, --reviewers pin in table/poll/template, --limit 500, base-ref profile reads, max_cycles budget semantics), and rationale.md carries only justification. The two dropped-guard regressions found in review were the real risk of a −45% trim, and both were restored plus verified rule-by-rule against the base — the strongest check available for a prose contract not fully pinned by tests.

[DESIGN-REVIEWED] cae8c6f

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] cae8c6f

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

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

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of cae8c6f12aa13c80689abb7228cf7a203d3ef660 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] cae8c6f

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

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of cae8c6f12aa13c80689abb7228cf7a203d3ef660 — 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.

All verifications are done: the gate-floor.md precedent and its endorsing test exist (test/test_prepare_pr_profiles.py:609), the resolve_reviewers discovery-mode hole matches the script's own docstring (pr_status.py:670), and I counted the residual duplication between the two new files. Final review:

First-Principles-Verdict: CONCERNS

The split's own rule — "SKILL.md carries only what the loop executes" — is broken by ~6 rationale sentences now living verbatim in both files.

What this change ships

Intent: make one heavily-loaded skill deterministic and cheap by fixing its self-contradictions and moving rationale out of the per-invocation load — a FIX.

1. Skill load nearly halved (68.7KB → 38KB) — justified, measured per-invocation cost
2. New references/rationale.md with evidence/design history — justified, precedent pinned by test_prepare_pr_profiles.py:609
3. monitor_start arming: one rule, bare requested = armed — justified, cause-level
4. One cron rule; pr_watch only with nothing to answer — justified, cause-level
5. Escalate-at-3 operative; 10 renamed a runaway backstop — justified
6. Freeze rule moved to Phase 3 as three push justifications — justified
7. Mode table with ship > stop > default precedence — justified, closes an undefined case
8. Proportionality collapsed to two questions, two outcomes — justified
9. Poll and monitor template now always pin --reviewers — declared; derived from resolve_reviewers() docstring (pr_status.py:670)
10. First-push clobber-check skip made explicit — justified fix

Watch

• Grepped both new files: at least 6 "why" sentences ship in both — backstop-not-target (SKILL.md:196 / rationale.md:24), drop-the-lane (164/67), camo-blocked (392/161), arming "result is processed" (341), max_cycles poll-budget (343), pr_watch reads-no-bodies (461). Divergence between duplicate statements is the exact defect class this PR fixes six instances of.
• The root cause ("no layering") has 1 counted unfixed sibling: babysit/SKILL.md is 40,351 bytes and carries the same 101-runs anecdote. Genuinely out of scope — accepted-and-deferred.

Subtractions

• Delete the duplicated why-clauses from SKILL.md lines 164, 196, 343, 392, 461 (keep the rule; the reason already lives in rationale.md), or delete the matching rationale.md paragraph where the in-line why is operationally load-bearing (the arming rule at SKILL.md:341).

[FIRST-PRINCIPLES-REVIEWED] cae8c6f

[iamwhatever]

Rebased onto main HEAD 17dfa28ce (was 26 behind). The Frontend Lint failure was inherited, not introduced.

Frontend Lint & Type Check (jscpd) — inherited, cleared by rebase. jscpd found 2 clone pairs at 0.01% over a threshold: 0 gate. Both pairs live in website/scripts/capture-*.mjs; this PR touches two markdown files under src/kiro_crew/builtin_skills/kirocrew-dev/prepare-pr/ and nothing else, so neither pair is mine:

• capture-aws-control.mjs [22:1-39:16] ↔ capture-hero-art-proxy.mjs [30:1-48:4] — fixed on main by fix(ci): dedupe capture-aws-control server, unbreak jscpd gate on main #6434 (dedupe capture-aws-control's hand-rolled server via serveDist).
• capture-chatpane-queue-edit.mjs [103:5-117:5] ↔ capture-chatpane-upload-error.mjs [112:5-126:5] — fixed on main by fix(ci): dedupe the split-pane chat harness page wiring #6445 (dedupe the split-pane chat harness page wiring), which folded both into prepareSplitChatPage.

Both fixes are ancestors of current main, so the rebase is the whole fix. Verified against the rebased tree rather than assumed: jscpd . from website/ reports Found 0 clones, 0 duplicated lines across all 2,357 files, exit 0.

PR Readiness was downstream of that lint failure and should clear with it. No content change — the diff is byte-identical to 3cb63f2ed.

[chenmingwei23]

Tier 1 auto-approve: refactor (2 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: refactor (skill doc reorg, no behaviour change) - split rationale out of prepare-pr SKILL.md.