From d0bedc945bae53a1a9486f6bd45e831720949bfb Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 18 Jul 2026 07:50:15 +0000 Subject: [PATCH 1/2] Encode review practices mined from apify-core/worker/crawlee PR feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mined ~790 review threads across 56 discussion-heavy PRs in apify-core, apify-worker, crawlee, and crawlee-python, cross-referenced against this repo's open issues/PRs (#7-#12) to avoid duplication, and folded in published loop-design guidance (judge self-preference bias, evidence- grounded critique). New rules cover only gaps not already raised: - Standing checks grow from three to six: evidence-grounded findings (unverifiable suspicion is a question, not a finding; referenced identifiers must exist), public-surface minimality (new exports need a consumer or a visibility reason), and doc sync for public changes (README/guides/upgrading notes updated in the same diff). - Implementer: diff stays scoped to the design (incidental hunks are reverted or get a one-line rationale in claim.md); non-obvious mechanisms, suppressions, and magic constants get why-comments. - Architect: search before inventing (name the existing helpers the design builds on); multi-step writes state failure ordering in Risks. - Model tiering: the review panel never resolves entirely onto the implementer's model (self-preference bias). - Finish: PR body scales with the diff — mechanism and known limitations for large changes, one-line whys for incidental ones. Each rule is pinned by a contract test; SKILL.md stays at 444/450 lines. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_016G4uCkx2btpqfGtJRAPJKd --- .claude/skills/shepherd/SKILL.md | 42 ++++++++++++++++++++--------- tests/test_orchestrator_contract.py | 33 +++++++++++++++++++++++ 2 files changed, 63 insertions(+), 12 deletions(-) diff --git a/.claude/skills/shepherd/SKILL.md b/.claude/skills/shepherd/SKILL.md index c294869..6d76928 100644 --- a/.claude/skills/shepherd/SKILL.md +++ b/.claude/skills/shepherd/SKILL.md @@ -130,12 +130,22 @@ 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 -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. +`{role.standing}` for reviewer and final-reviewer roles always carries six 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; +- 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; +- every finding carries evidence checked against the repository — a file:line, a command run, a + cited doc — and identifiers the diff relies on (env vars, config keys, paths) must exist; + a suspicion no check could ground goes in as a question, not a finding — unverified review + claims cause over-engineering and erode trust; +- a new public/exported symbol, option, or env var with no consumer in the diff needs an explicit + reason to be public (`@internal`, private, or a visibility note) — surface added only "for + testing" or "for later" is a finding; +- a diff that adds, renames, or removes a public symbol, option, or env var must update every doc + that teaches it (README, guides, upgrading notes, examples) — an unsynced doc is a finding. | role | reads | do NOT read | writes | format | |------|-------|-------------|--------|--------| @@ -143,7 +153,7 @@ stale references, wrong claims, and comments restating the obvious are findings. | `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 | -| `implementer` | `2-design.md`, `3-success-criteria.md`, `_request_fact_check.md`, `_codebase_map.md` if present, all prior `iter-*/review-*.md` + `final-review-*.md` + `fulfillment.md` | — | source edits + `iter-N/claim.md` | what done · every finding fixed, none skipped or deferred · for a behavior change, add a regression test — ideally shown red before the fix and green after, with the red→green noted in `claim.md` — never weaken/delete tests | +| `implementer` | `2-design.md`, `3-success-criteria.md`, `_request_fact_check.md`, `_codebase_map.md` if present, all prior `iter-*/review-*.md` + `final-review-*.md` + `fulfillment.md` | — | source edits + `iter-N/claim.md` | what done · every finding fixed, none skipped or deferred · for a behavior change, add a regression test — ideally shown red before the fix and green after, with the red→green noted in `claim.md` — never weaken/delete tests · keep the diff scoped to the design: a hunk the design doesn't call for is reverted or gets a one-line rationale in `claim.md` · non-obvious mechanisms, suppressions, and magic constants get a why-comment, with provenance for ported logic | | `reviewer` | pasted content of `2-design.md`, `3-success-criteria.md`, `iter-N/diff.patch`, `iter-N/test-results.txt`, plus the repository itself (working tree, git history, read-only commands) — no other `.shepherd/` files | `claim.md`, peer reviewers' output | `iter-N/review-.md` | first line `VERDICT: PASS\|FAIL` (PASS = zero findings), then findings tagged `blocker\|major\|minor\|nit` | | `final_reviewer` | same as reviewer, but judging the post-fix integrated state: interactions with unchanged code, consumer/contract impact, doc/AGENTS staleness — not a second pass over the patch | `claim.md`, peer reviewers' output | `iter-N/final-review-.md` | same verdict format as reviewer | | `fulfillment` | pasted content of `3-success-criteria.md`, `iter-N/diff.patch`, `iter-N/test-results.txt`, `iter-N/claim.md`, plus the working tree (may run the non-mutating check a criterion names) | `2-design.md` solution details, review files | `iter-N/fulfillment.md` | first line `VERDICT: PASS\|FAIL`, then each criterion `MET \| NOT MET` with evidence | @@ -156,7 +166,9 @@ explicit name (`opus`, `sonnet`, `haiku`) is used verbatim. Resolve `"auto"` as: `explorer`, `success_criteria`, `fulfillment`, `reviewer` → `sonnet`; `architect` → `opus` (`sonnet` for a revision pass — it folds feedback into an existing design without re-exploring); `final_reviewer` → `opus` (`sonnet` for `trivial`/`small`). `sonnet` is the floor for review — -never `haiku`. Pre-gate stages (verify, explorer, architect, success_criteria) resolve `"auto"` +never `haiku`. The panel never resolves entirely onto the implementer's model: keep at least one +reviewer — and one final reviewer when the panel has any — on a different model, because a judge +sharing the generator's model favors its output (self-preference bias). Pre-gate stages (verify, explorer, architect, success_criteria) resolve `"auto"` at dispatch time from this table. At step 4 (Design gate), record all picks in `_panel.json`: the pre-gate ones as the record of what ran, the post-gate ones (implementer, reviewers, final reviewers, fulfillment) for the human to edit before approving. @@ -222,9 +234,12 @@ Otherwise set `state.phase="design"`. the files and the change is mechanical or localized (deletion, rename, inlining) — note why in `_progress.md`. - Dispatch the `architect` stage to write `.shepherd/2-design.md`. ~1 page, no code blocks, no - file:line dumps. Product first, implementation second. A design that unifies a - style/format/template must pin it with one fully-worked example (a complete sentence or - instance showing placement and punctuation), not only named parts: + file:line dumps. Product first, implementation second. The design names the existing helpers + and patterns it builds on — search before inventing; a new helper, config option, or endpoint + gets one line on why nothing existing fits. Multi-step writes state their failure ordering + under Risks (crash mid-way, concurrent writer, fallback when a new path replaces an old one). + A design that unifies a style/format/template must pin it with one fully-worked example (a + complete sentence or instance showing placement and punctuation), not only named parts: ``` ## What we're solving (product: the problem and who hits it) @@ -391,7 +406,10 @@ from step 5 (Inner loop). 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 + run evidence (fulfillment, oracle, reviewer verdicts); run files stay ignored. Scale the body + to the diff: a large or subtle change states its mechanism and known limitations, and any + incidental change carries its one-line why — thin bodies on big diffs shift the cost onto + reviewers. 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..aefdc82 100644 --- a/tests/test_orchestrator_contract.py +++ b/tests/test_orchestrator_contract.py @@ -331,3 +331,36 @@ def test_step_headings_declare_their_phase(): assert len(headings) == 9 missing = [h for h in headings if "`phase=" not in h] assert missing == [], f"headings without a phase annotation: {missing}" + + +def test_standing_checks_require_evidence_grounded_findings(): + # Ungrounded review claims cause over-engineering; unverifiable suspicion is a question. + assert "always carries six checks" in ORCH + assert "every finding carries evidence" in ORCH + assert "a question, not a finding" in ORCH + + +def test_standing_checks_cover_public_surface_and_doc_sync(): + # New public surface needs a consumer or a visibility reason; public changes update docs. + assert "no consumer in the diff" in ORCH + assert "an unsynced doc is a finding" in ORCH + + +def test_implementer_keeps_diff_scoped_and_explains_nonobvious_code(): + # Unexplained incidental hunks and missing why-comments are the top mined review complaints. + assert "reverted or gets a one-line rationale" in ORCH + assert "get a why-comment" in ORCH + + +def test_architect_reuses_before_inventing_and_states_failure_ordering(): + assert "search before inventing" in ORCH + assert "crash mid-way, concurrent writer" in ORCH + + +def test_review_panel_never_matches_implementer_model_entirely(): + assert "never resolves entirely onto the implementer's model" in ORCH + assert "self-preference bias" in ORCH + + +def test_pr_body_scales_with_diff(): + assert "mechanism and known limitations" in ORCH From f5b32a6de113649f6ba7702ab9f917c56f9fd7cf Mon Sep 17 00:00:00 2001 From: Jiri Spilka Date: Fri, 24 Jul 2026 23:23:08 +0200 Subject: [PATCH 2/2] refactor: enhance clarity of SKILL.md regarding findings and reviewer processes --- .claude/skills/shepherd/SKILL.md | 45 ++++++++++++++------------- tests/test_orchestrator_contract.py | 47 ++++++++--------------------- 2 files changed, 37 insertions(+), 55 deletions(-) diff --git a/.claude/skills/shepherd/SKILL.md b/.claude/skills/shepherd/SKILL.md index cddb9fb..a87c850 100644 --- a/.claude/skills/shepherd/SKILL.md +++ b/.claude/skills/shepherd/SKILL.md @@ -156,10 +156,10 @@ sides, and a rewritten path must be exercised against the input classes the old an untested new arm is a finding; a behavior delta versus design or base that the design leaves unstated is a finding, including a changed helper whose default/no-arg semantics silently invert; and whatever the diff names, places, or exports must follow the repo's stated conventions doc, -with no heavier import than its role needs; every finding carries evidence checked against the -repository, relied-on identifiers must exist, and ungrounded suspicion is a question, not a finding. -A new public/exported symbol, option, or env var with no consumer in the diff needs a reason to be -public (`@internal`, private, or a visibility note); testing/later-only surface is a finding; public changes update all teaching docs; an unsynced doc is a finding. +with a new module importing no heavier layer than its role needs. Every finding carries evidence +checked against the repository, and every identifier it relies on must exist; a suspicion you +cannot ground is a question, not a finding — it belongs in your `Questions:` section, never as a +finding and never dropped. | role | reads | do NOT read | writes | format | |------|-------|-------------|--------|--------| @@ -167,22 +167,24 @@ public (`@internal`, private, or a visibility note); testing/later-only surface | `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 | -| `implementer` | `2-design.md`, `3-success-criteria.md`, `_request_fact_check.md`, `_codebase_map.md` if present, all prior `iter-*/review-*.md` + `final-review-*.md` + `fulfillment.md` | — | source edits + `iter-N/claim.md` | what done · every finding fixed, none skipped or deferred · for a behavior change, add a regression test — ideally shown red before the fix and green after, with the red→green noted in `claim.md` — never weaken/delete tests · keep the diff scoped to the design: a hunk the design doesn't call for is reverted or gets a one-line rationale in `claim.md` · non-obvious mechanisms, suppressions, and magic constants get a why-comment, with provenance for ported logic | -| `reviewer` | pasted content of `2-design.md`, `3-success-criteria.md`, `iter-N/diff.patch`, `iter-N/test-results.txt`, plus the repository itself (working tree, git history, read-only commands) — no other `.shepherd/` files | `claim.md`, peer reviewers' output | `iter-N/review-.md` | first line `VERDICT: PASS\|FAIL` (PASS = zero findings, `pre-existing`-tagged ones excepted), then findings tagged `blocker\|major\|minor\|nit`; a defect in adjacent code that predates the diff carries the extra tag `pre-existing` — reported, never silenced, routed to step 7 (Fulfillment + create-PR confirm) | +| `implementer` | `2-design.md`, `3-success-criteria.md`, `_request_fact_check.md`, `_codebase_map.md` if present, all prior `iter-*/review-*.md` + `final-review-*.md` + `fulfillment.md` | — | source edits + `iter-N/claim.md` | what done · every finding fixed, none skipped or deferred · for a behavior change, add a regression test — ideally shown red before the fix and green after, with the red→green noted in `claim.md` — never weaken/delete tests | +| `reviewer` | pasted content of `2-design.md`, `3-success-criteria.md`, `iter-N/diff.patch`, `iter-N/test-results.txt`, plus the repository itself (working tree, git history, read-only commands) — no other `.shepherd/` files | `claim.md`, peer reviewers' output | `iter-N/review-.md` | first line `VERDICT: PASS\|FAIL` (PASS = zero findings, `pre-existing`-tagged ones excepted), then findings tagged `blocker\|major\|minor\|nit`; a defect in adjacent code that predates the diff carries the extra tag `pre-existing` — reported, never silenced, routed to step 7 (Fulfillment + create-PR confirm); then a `Questions:` section — every suspicion you could not ground, or `none` — which never blocks PASS and is surfaced to the human at step 7 (Fulfillment + create-PR confirm) | | `final_reviewer` | same as reviewer, but judging the post-fix integrated state: interactions with unchanged code, consumer/contract impact, doc/AGENTS staleness — not a second pass over the patch | `claim.md`, peer reviewers' output | `iter-N/final-review-.md` | same verdict format as reviewer | | `fulfillment` | pasted content of `3-success-criteria.md`, `iter-N/diff.patch`, `iter-N/test-results.txt`, `iter-N/claim.md`, plus the working tree (may run the non-mutating check a criterion names) | `2-design.md` solution details, review files | `iter-N/fulfillment.md` | first line `VERDICT: PASS\|FAIL`, then each criterion `MET \| NOT MET` with evidence | | `followups` | pasted content of the design's "Scope split" section and of every `iter-*/review-*.md` + `final-review-*.md` (all iterations) | `3-success-criteria.md`, `claim.md` | `iter-N/followups.md` | ledger: item · origin (`scope-split` \| review file) · proposed disposition `fix-here \| issue \| pr-note \| drop` · one-line why; never empty — "none" explicitly | ### Model tiering -`"auto"` picks by role and tier; an explicit `opus`, `sonnet`, or `haiku` is used verbatim. -Resolve implementer → `haiku` (`sonnet` for `medium`/`large`); verify, explorer, criteria, -fulfillment, followups, reviewer → `sonnet`; architect → `opus` (`sonnet` on revision); -final reviewer → `opus` (`sonnet` for `trivial`/`small`). Review uses `sonnet` or higher. -The panel never resolves entirely onto the implementer's model: keep at least one reviewer — -and one final reviewer when present — different to limit self-preference bias. Resolve pre-gate -`"auto"` picks at dispatch; at step 4 (Design gate), `_panel.json` is the record of what ran plus -post-gate picks (implementer, reviewers, final reviewers, fulfillment, followups) for human edit. +`"auto"` (the shipped default) lets the orchestrator pick a model per role and triage tier; an +explicit name (`opus`, `sonnet`, `haiku`) is used verbatim. Resolve `"auto"` as: `implementer` → +`haiku` (`sonnet` for `medium`/`large` — a subtle change is not transcription); `verify`, +`explorer`, `success_criteria`, `fulfillment`, `followups`, `reviewer` → `sonnet`; `architect` → `opus` +(`sonnet` for a revision pass — it folds feedback into an existing design without re-exploring); +`final_reviewer` → `opus` (`sonnet` for `trivial`/`small`). `sonnet` is the floor for review — +never `haiku`. Pre-gate stages (verify, explorer, architect, success_criteria) resolve `"auto"` +at dispatch time from this table. At step 4 (Design gate), record all picks in +`_panel.json`: the pre-gate ones as the record of what ran, the post-gate ones (implementer, +reviewers, final reviewers, fulfillment, followups) for the human to edit before approving. ## Procedure @@ -249,9 +251,10 @@ Otherwise set `state.phase="design"`. implementer reuse it. Skip for `trivial`/`small`, or when the verify fact-check already maps the files and the change is mechanical or localized (deletion, rename, inlining) — note why in `_progress.md`. -- Dispatch the `architect` stage to write `.shepherd/2-design.md`: ~1 page, no code blocks or file:line dumps. - Product first, implementation second; search before inventing by naming reused helpers or why none fits. - Risks cover failure ordering (crash mid-way, concurrent writer, replacement fallback); unified styles/templates need a worked example: +- Dispatch the `architect` stage to write `.shepherd/2-design.md`. ~1 page, no code blocks, no + file:line dumps. Product first, implementation second. A design that unifies a + style/format/template must pin it with one fully-worked example (a complete sentence or + instance showing placement and punctuation), not only named parts: ``` ## What we're solving (product: the problem and who hits it) @@ -420,7 +423,8 @@ disposition per item. When limits are exhausted, or the human disputes a criterion itself, ask the human: accept with the exception recorded, extend the limit, or abandon. - When fulfillment passes: no plan mode. Summarize in chat — the fulfillment table, the - followups ledger verbatim, oracle status, reviewer verdicts, fixed findings, `git diff --stat`. + followups ledger verbatim, oracle status, reviewer verdicts, every reviewer `Questions:` entry + verbatim, fixed findings, `git diff --stat`. The human dispositions each ledger item: `fix-here` reopens the inner loop; `issue` is created only now, on this approval; `pr-note` lands in the PR body; `drop` is recorded in `_progress.md`. Never silent; never an issue without approval. When every fix-here item is @@ -452,9 +456,8 @@ from step 5 (Inner loop). repo has a PR template** (`.github/pull_request_template.md` or the other usual locations), 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 is one short clause, not a - transcript; run files stay ignored. Large/subtle changes state mechanism and known limitations; - incidental changes carry a one-line why. When the run + 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`). Approved `pr-note` items land as a short Follow-ups list in the body. Every number or factual claim in the body (test counts, diff --git a/tests/test_orchestrator_contract.py b/tests/test_orchestrator_contract.py index e4dd3a8..a721121 100644 --- a/tests/test_orchestrator_contract.py +++ b/tests/test_orchestrator_contract.py @@ -15,7 +15,8 @@ def test_orchestrator_reads_config_and_registry(): def test_orchestrator_skill_stays_compact(): # Keep the orchestrator readable, but do not force removal of operational guidance. - assert len(ORCH.splitlines()) <= 500 + # A readability guard, not a budget: never pay for a new rule by deleting a rule's rationale. + assert len(ORCH.splitlines()) <= 510 def test_orchestrator_documents_per_reviewer_files(): @@ -379,39 +380,6 @@ def test_step_headings_declare_their_phase(): assert missing == [], f"headings without a phase annotation: {missing}" -def test_standing_checks_require_evidence_grounded_findings(): - # Ungrounded review claims cause over-engineering; unverifiable suspicion is a question. - assert "always carries these checks" in ORCH - assert "every finding carries evidence" in ORCH - assert "a question, not a finding" in ORCH - - -def test_standing_checks_cover_public_surface_and_doc_sync(): - # New public surface needs a consumer or a visibility reason; public changes update docs. - assert "no consumer in the diff" in ORCH - assert "an unsynced doc is a finding" in ORCH - - -def test_implementer_keeps_diff_scoped_and_explains_nonobvious_code(): - # Unexplained incidental hunks and missing why-comments are the top mined review complaints. - assert "reverted or gets a one-line rationale" in ORCH - assert "get a why-comment" in ORCH - - -def test_architect_reuses_before_inventing_and_states_failure_ordering(): - assert "search before inventing" in ORCH - assert "crash mid-way, concurrent writer" in ORCH - - -def test_review_panel_never_matches_implementer_model_entirely(): - assert "never resolves entirely onto the implementer's model" in ORCH - assert "self-preference bias" in ORCH - - -def test_pr_body_scales_with_diff(): - assert "mechanism and known limitations" in ORCH - - def test_standing_checks_cover_new_arms_and_disclosure(): assert "these three checks" in ORCH assert re.search(r"test-exercised on both\s+sides", ORCH) @@ -456,6 +424,17 @@ def test_gate_panel_lens_fit_nudge(): assert "never edits the panel itself" in ORCH +def test_reviewer_questions_slot_keeps_ungrounded_suspicion_visible(): + # Findings must be grounded, but grounding must not become a silent-drop channel: PASS is + # defined as zero findings, so an ungroundable suspicion needs its own slot in the reviewer + # format and a route to the human, or the zero-findings convergence rule is trivially gamed. + assert "every identifier it relies on must exist" in ORCH + assert "a question, not a finding" in ORCH + assert "`Questions:` section" in ORCH + assert "never blocks PASS" in ORCH + assert "every reviewer `Questions:` entry" in ORCH + + def test_light_rereview_for_trivial_fix_here(): assert "delta-focused panel reviewer plus a fulfillment-delta check" in ORCH assert "picks the mode" in ORCH