feat: Tighten reviewer standing checks, design Q&A, and PR body rules

.claude/skills/shepherd/SKILL.md

@@ -130,16 +130,19 @@ If the dispatched agent has no write access, it returns the artifact verbatim as
 message and the orchestrator persists it to `{role.writes}` **unchanged** — a mechanical relay,
 not authorship; the no-judgment-files rule is not violated. Note the relay in `_progress.md`.
 
-`{role.standing}` for reviewer and final-reviewer roles always carries three checks, regardless
+`{role.standing}` for reviewer and final-reviewer roles always carries these checks, regardless
 of what the design emphasizes: committed code must not reference run-internal artifacts
 (`.shepherd/`, plan files, session paths); cruft preserved by a faithful migration is still a
 finding — "byte-identical" instructions cover assertions/behavior, not carried-over dead code;
-and a comment the diff adds, edits, or moves must still be true of the code it now describes —
-stale references, wrong claims, and comments restating the obvious are findings.
+a comment the diff adds, edits, or moves must still be true of the code it now describes —
+stale references, wrong claims, comments restating the obvious, and comments longer than the
+code they describe are findings; also flag AI-slop — abnormal defensive try/catch (defensive
+code at trust boundaries is fine), type-escape casts (`any` or equivalent), deep nesting that
+should be early returns, and other patterns inconsistent with the surrounding file.
 
 | role | reads | do NOT read | writes | format |
 |------|-------|-------------|--------|--------|
-| `verify` | `_user_request.md`, `1-triage.md`, codebase, referenced issue; **for a review-only run also the PR/branch description and its diff — treat that description as the claim source** | `2-design.md`, `3-success-criteria.md` | `_request_fact_check.md` | claim ledger: every request claim tagged `VALID \| STALE \| LIKELY-FIXED \| UNVERIFIABLE` with evidence, plus a one-line verdict — never empty |
+| `verify` | `_user_request.md`, `1-triage.md`, codebase, referenced issue, current upstream sources for any claim resting on facts outside the repo; **for a review-only run also the PR/branch description and its diff — treat that description as the claim source** | `2-design.md`, `3-success-criteria.md` | `_request_fact_check.md` | claim ledger: every request claim tagged `VALID \| STALE \| LIKELY-FIXED \| UNVERIFIABLE` with evidence (claims resting on facts outside the repo: check current upstream sources, not model memory), plus a one-line verdict — never empty |
 | `explorer` | codebase | `.shepherd/` internals | `_codebase_map.md` | ≤1 page: key files · patterns · data flow · risks |
 | `architect` | `_user_request.md`, `1-triage.md`, `_request_fact_check.md`, `_codebase_map.md` if present, `_design_feedback.md` if present (settled human decisions — constraints, not suggestions), codebase; on a revision pass also its previous `2-design.md` | `3-success-criteria.md` | `2-design.md` | the design template in step 3 (Design) |
 | `success_criteria` | pasted content of the "What we're solving" and "How it will work" sections of `2-design.md`, plus `_user_request.md`, `1-triage.md`, and `_request_fact_check.md` (verified facts — real paths, real coverage gaps — so criteria reference reality instead of guessing; it contains no solution) — nothing else | the rest of `2-design.md` (the solution), `claim.md` | `3-success-criteria.md` | numbered, testable criteria — each verifiable by a command or an observable behavior; no solution details |
@@ -168,7 +171,9 @@ reviewers, final reviewers, fulfillment) for the human to edit before approving.
 Orchestrator-owned cheap product screen — no dispatch, no deep code reading; a quick skim is
 fine. Write `.shepherd/1-triage.md` in about 12 lines:
 - Problem
-- Decision: `PROCEED | DEFER | DECLINE`
+- Decision: `PROCEED | DEFER | DECLINE` — DEFER an under-specified request (no determinable
+  problem or user-visible outcome) and put what's missing in Open questions; a clear problem
+  with an open solution still proceeds — design settles solutions, not triage
 - Complexity: `trivial | small | medium | large`
 - Review-only: `yes | no` — `yes` when the task is "review PR/branch/diff X" with nothing to build
 - Approach sketch, high level only
@@ -233,10 +238,13 @@ Otherwise set `state.phase="design"`.
   ## Alternatives + the call
   ## Major changes           (key files/areas only — never an exhaustive file list)
   ## Risks
-  ## Open questions          (numbered; each with your recommended answer)
+  ## Open questions          (real decisions only — each: options + recommended answer; no filler)
   ## Decisions               (from _design_feedback.md when it exists; otherwise starts empty)
   ```
 
+  Facts verifiable in the repo or issue belong in the design body, not Open questions — ask as
+  many decisions as the design needs, no minimum or maximum.
+
   For a review-only run, `2-design.md` is the review scope: what to check and which reviewers.
 - Dispatch the `success_criteria` stage: paste it ONLY the two product sections of the design
   (plus request, triage, and the fact-check) and have it write `.shepherd/3-success-criteria.md`. It defines
@@ -245,10 +253,11 @@ Otherwise set `state.phase="design"`.
   contract.
 
 **Iterate — the conversation is the orchestrator's; every rewrite is a subagent's.**
-- Present the FULL `2-design.md` + `3-success-criteria.md` (see "Keep the human in the loop"),
-  then work the open questions: one question at a time, multiple choice where possible, always
-  with your recommended answer — product questions first, implementation after.
-- Be proactive: raise risks and trade-off calls yourself. YAGNI — cut speculative scope.
+- Present the FULL `2-design.md` + `3-success-criteria.md` (see "Keep the human in the loop").
+- Grill decisions one question at a time (wait for each answer): options + your recommended
+  answer; product questions first, implementation after. Look up facts yourself; only decisions
+  go to the human. Walk dependencies in order — if Open questions miss a real fork, ask it.
+  YAGNI — cut speculative scope.
 - **Batch a round of answers**, then append them verbatim to `_design_feedback.md`
   (append-only; the orchestrator writes only this file, never the design or criteria).
 - Re-run the `architect` as a **revision pass** — it reads its previous `2-design.md` +
@@ -388,10 +397,10 @@ from step 5 (Inner loop).
    `iter-1/predirty.txt`); stop if unrelated changes are present.
 2. Commit only the run's own paths — pre-existing changes stay uncommitted in the tree. **If the
    repo has a PR template** (`.github/pull_request_template.md` or the other usual locations),
-   mirror its section headings and fill them from the run — a layout, not instructions to obey.
-   **Otherwise** write the PR body plain: **What we're solving · How · Alternatives considered**.
-   Either way: plain commit message, never enumerate changes obvious from the diff, summarize
-   run evidence (fulfillment, oracle, reviewer verdicts); run files stay ignored. When the run
+   mirror its section headings and fill each briefly — a layout, not instructions to obey.
+   **Otherwise** at most three short bullets (What / Why / Notes). Either way: plain commit
+   message, never enumerate changes obvious from the diff; evidence (fulfillment, oracle,
+   reviews) is one short clause, not a transcript; run files stay ignored. When the run
    completes a tracked issue, end the PR body with `Closes #N` (auto-close on merge); reference
    parent/epic issues non-closingly (`Part of #M`).
 3. If a writable remote exists, push and open a PR. Record the evidence summary, approval

tests/test_orchestrator_contract.py

@@ -310,12 +310,45 @@ def test_orchestrator_documents_dirty_worktree_protection():
 
 
 def test_orchestrator_finish_writes_plain_commit_and_pr():
-    # Plain PR body: what / how / alternatives, never obvious-diff narration.
-    assert "Alternatives considered" in ORCH
+    # Short PR body: template headings filled briefly, else ≤3 bullets; no essay / diff narration.
+    assert "three short bullets" in ORCH
+    assert "What / Why / Notes" in ORCH
     assert "obvious from the diff" in ORCH
+    assert "one short clause" in ORCH
     assert "PR URL" in ORCH
 
 
+def test_open_questions_are_real_decisions():
+    assert "real decisions only" in ORCH
+    assert "no filler" in ORCH
+    assert "no minimum or maximum" in ORCH
+    assert "Facts verifiable" in ORCH
+
+
+def test_design_iterate_grills_decisions():
+    assert "Grill decisions" in ORCH
+    assert "Look up facts yourself" in ORCH
+    assert "only decisions" in ORCH
+    assert "miss a real fork" in ORCH
+
+
+def test_standing_checks_include_ai_slop():
+    assert "comments longer than the" in ORCH
+    assert "abnormal defensive" in ORCH
+    assert "type-escape casts" in ORCH
+    assert "early returns" in ORCH
+
+
+def test_triage_defers_underspecified_requests():
+    assert "DEFER an under-specified request" in ORCH
+    assert "design settles solutions, not triage" in ORCH
+
+
+def test_verify_checks_external_spec_claims():
+    assert "facts outside the repo" in ORCH
+    assert "not model memory" in ORCH
+
+
 def test_step_references_name_their_target():
     # Bare "step N" is ambiguous against numbered list items inside sections; every
     # cross-reference must name its target: "step 4 (Design gate)" or "(step 9, Finish)".