From 16f54fec099ee2fde1279f92ae0415e3afefac6a Mon Sep 17 00:00:00 2001 From: Jiri Spilka Date: Thu, 16 Jul 2026 15:17:59 +0200 Subject: [PATCH 1/2] Tighten reviewer standing checks, design Q&A, and PR body rules Add AI-slop patterns to the engine-independent reviewer standing checks, require Open questions to be real decisions (facts stay in the design body), make the iterate step grill one decision at a time, and cap PR bodies at template-brief or three bullets. Pin the new prose with contract tests. Co-Authored-By: Claude Fable 5 --- .claude/skills/shepherd/SKILL.md | 31 ++++++++++++++++++----------- tests/test_orchestrator_contract.py | 27 +++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 14 deletions(-) diff --git a/.claude/skills/shepherd/SKILL.md b/.claude/skills/shepherd/SKILL.md index c294869..cc523a7 100644 --- a/.claude/skills/shepherd/SKILL.md +++ b/.claude/skills/shepherd/SKILL.md @@ -130,12 +130,15 @@ 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 | |------|-------|-------------|--------|--------| @@ -233,10 +236,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 +251,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 +395,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 diff --git a/tests/test_orchestrator_contract.py b/tests/test_orchestrator_contract.py index 0f37eb8..f2abfd8 100644 --- a/tests/test_orchestrator_contract.py +++ b/tests/test_orchestrator_contract.py @@ -310,12 +310,35 @@ 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_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)". From f09964ea79a629997bc4806a73e173e42f827f42 Mon Sep 17 00:00:00 2001 From: Jiri Spilka Date: Thu, 16 Jul 2026 23:28:08 +0200 Subject: [PATCH 2/2] refactor: clarify verification and triage processes in SKILL.md and add related tests --- .claude/skills/shepherd/SKILL.md | 6 ++++-- tests/test_orchestrator_contract.py | 10 ++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/.claude/skills/shepherd/SKILL.md b/.claude/skills/shepherd/SKILL.md index cc523a7..bafc1ed 100644 --- a/.claude/skills/shepherd/SKILL.md +++ b/.claude/skills/shepherd/SKILL.md @@ -142,7 +142,7 @@ should be early returns, and other patterns inconsistent with the surrounding fi | 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 | @@ -171,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 diff --git a/tests/test_orchestrator_contract.py b/tests/test_orchestrator_contract.py index f2abfd8..6988182 100644 --- a/tests/test_orchestrator_contract.py +++ b/tests/test_orchestrator_contract.py @@ -339,6 +339,16 @@ def test_standing_checks_include_ai_slop(): 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)".