docs(prepare-pr): require an explicit reply to every concern before review-ready

[chenmingwei23]

What is the problem?

The prepare-pr skill drives a PR to readiness: passed without ever requiring the author to reply to a concern. Its only reply obligation — Phase 3 step 3 — is scoped to GPT findings (<!-- ai-review-disposition target=gpt -->). Everything else was framed as optional: "Advisory threads may remain open" (review-ready definition) and "Medium/Low findings are advisory … do not widen the PR solely to satisfy advisory feedback" (severity gate).

The gap has teeth because Design Review and UX Review post 🟡 CONCERNS and still report their check as passing. The readiness rollup therefore never surfaces them and no loop step nags about them, so an agent that follows the skill correctly still lands a PR with unanswered concerns on it.

Observed on real PRs: #1432 (Design Review — 🟡 CONCERNS) and #1436 (Design Review — 🟡 CONCERNS + UX Review — 🟡 CONCERNS) both reached review-ready carrying only ai-review-disposition target=gpt comments from the author, with no reply to either concern.

Why this issue matters to the user

A maintainer reading the PR cannot distinguish "the author weighed this and decided it is out of scope" from "the author never looked at it" — silence reads as the second. That costs a review round to recover information the author already had, and it happens on PRs that are otherwise fully green, which is exactly when the author has stopped looking.

How our fix solves it

Symptom: advisory CONCERNS ship unanswered. → Root cause: the skill's only reply rule targets GPT findings, and its severity language conflates "you need not change the code" with "you need not respond" — so nothing in the loop ever asks about a passing-but-concerned verdict. → Change: make answering a concern a readiness condition with its own loop step, and separate the two meanings of "advisory".

Docs-only, one file (src/kiro_crew/builtin_skills/kirocrew-dev/prepare-pr/SKILL.md):

• New Core Concepts bullet — "Answer every concern — MANDATORY, and a readiness condition." Enumerates what counts (non-PASS advisory bot verdicts, Arbiter-escalated items, human comments and inline threads), states that a green check is not a reply, and fixes the three allowed dispositions: fixed (change + SHA), rebutted (evidence, without contorting correct code), accepted-and-deferred (why out of scope + where it goes next). Explicitly forbids one blanket line for a batch, and notes answering is prose-only so it never widens the diff or forces another push.
• Severity gate split — advisory governs whether you must change code, never whether you must reply. Every finding at every severity gets a written disposition.
• Review-ready definition — now includes "every raised concern answered"; advisory findings may remain unfixed, not unanswered.
• New Phase 3 step 4 — an enumerate-and-answer step (with the gh pr view --json comments,reviews command) placed before the poll, so it runs on every iteration rather than being remembered at the end. The old step 4 (poll) becomes step 5; the one stale "Phase 3 step 4" cross-reference is updated.
• Phase 4 convergence — requires that no unanswered concern remains; a green rollup with unanswered CONCERNS is explicitly not converged, and the user-facing report must say how each left-on-purpose item was answered.
• Two new Common mistakes — leaving a CONCERNS verdict unanswered, and answering a batch with one blanket "addressed the review feedback" line.
• Overview — one sentence so a skimmer sees that review-ready includes answered concerns.

What tests we did

• pytest test/test_skills.py test/test_skill_home_paths.py test/test_agent_template_skills.py test/test_ai_review_workflows.py → 182 passed. These cover the skill loader's frontmatter/description parsing, the lazy-load context budget (this file grew ~19 lines, well inside the budget), the agent-template skill listing, and the AI-review workflow contract the skill mirrors.
• No Python/TS/CSS touched, so the lint/type/vitest gates have nothing to bite on; run for completeness via CI.

Manual verification

Re-read the edited Phase 3 against the two PRs that motivated this and confirmed the new step 4 would have caught both: on #1432 and #1436 the gh pr view --json comments,reviews enumeration lists the Design Review/UX Review CONCERNS bodies that the existing GPT-scoped disposition step skips, and Phase 4 would have blocked the "review-ready" report until each had a disposition. No screenshots — no user-visible UI surface changes.

Any other suggestions on the work

The convention is currently enforced by prose only. A natural follow-up is a mechanical check — e.g. teach pr_status.py to count non-PASS bot verdicts that have no author reply and fold that into its exit code, so the loop fails closed instead of relying on the agent remembering to run step 4. Filing that separately rather than widening this docs change.

Fixes #1529

[github-actions]

Opus 5 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 1e9d96b

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

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

[github-actions]

Design Review (Fable 5) — ✅ PASS

Advisory design-level review of 1e9d96b636b781ac3390a631bf264f3f75b908d2 — updated in place on each push; does not block merge.

Design-Verdict: PASS

A documented real gap (evidenced on #1432/#1436), fixed at its actual root — the GPT-scoped reply rule and conflated "advisory" — with the mechanical fail-closed check rightly deferred.

[DESIGN-REVIEWED] 1e9d96b

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/builtin_skills/kirocrew-dev/prepare-pr/SKILL.md:131 -- .body[0:200] omits Design/UX concern details after their generated headers, preventing an individual disposition -> Fix: output the full .body.
[GPT-REVIEWED] 1e9d96b

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

[chenmingwei23]

CI disposition: the two red shards are inherited from main, not from this PR

Backend Tests (3.10, 2) and Backend Tests (Windows) (2) both fail on:

test/test_lazy_data_home_paths.py::TestNoImportTimePathResolution::test_no_module_level_path_constants
AssertionError: Data-home path resolved at import time (issue #874) ...
    apps/builtins/spec_builder/tests/test_routes.py:88  [module] _REAL_STATE_DIR = ..._state_dir()

rebutted — not caused by this PR. This branch changes exactly one file, src/kiro_crew/builtin_skills/kirocrew-dev/prepare-pr/SKILL.md (markdown). The offending file, src/kiro_crew/apps/builtins/spec_builder/tests/test_routes.py, does not exist on this branch at all — it landed on main in #518 (af8774b8b), after this branch's base (95813ee14). CI sees it only because PR checks run on the merge commit with current main.

Both failures are shard 2 on two different platforms, so it is deterministic rather than a flake, and main's own run for af8774b8b was still in progress when this surfaced.

Filed as #1537 for the owner of #518. This PR needs no change; it should go green once main is fixed.

[chenmingwei23]

Update on the comment above: resolved, and no longer applicable to this PR's current state. #1543 fixed the ratchet violation on main (65268a1cc), this branch was rebased onto it (1e9d96b6), and all five inherited reds cleared — the rollup is now 0 failing, which also confirms none of them were ever this PR's. My duplicate fix (#1546) is closed and issue #1537 is closed. Leaving the original comment in place as the triage record rather than editing it.