fix(ce-resolve-pr-feedback): judge findings centrally before dispatching fixers (#1015)

Author: tmchow

docs/skills/ce-resolve-pr-feedback.md

@@ -2,7 +2,7 @@
 
 > Evaluate, fix, and reply to PR review feedback in parallel. Fix what's real; don't churn on what isn't.
 
-`ce-resolve-pr-feedback` is the **incoming-feedback resolution** skill. After your PR gets review comments, this skill fetches all unresolved threads, classifies them as new vs already-handled, dispatches parallel agents to validate each finding and fix what's genuinely correct (or reply with reasoning), commits and pushes, then posts replies and resolves threads via GitHub's GraphQL API. It judges every item on its merits — regardless of source (human or bot) or form (inline thread, formal review body, or top-level comment) — and defaults to fixing, diverting only when reading the code trips a concrete signal (the finding's wrong, the fix would harm, it buys nothing, or the risk can't be bounded).
+`ce-resolve-pr-feedback` is the **incoming-feedback resolution** skill. After your PR gets review comments, this skill fetches all unresolved threads, classifies them as new vs already-handled, then judges every finding centrally — in the one context that holds all threads at once — and fans out parallel subagents only to *implement* the fixes it has approved. It commits and pushes, then posts replies and resolves threads via GitHub's GraphQL API. It judges every item on its merits — regardless of source (human or bot) or form (inline thread, formal review body, or top-level comment) — and defaults to fixing, diverting only when reading the code trips a concrete signal (the finding's wrong, the fix would harm, it buys nothing, or the risk can't be bounded). The central judgment is what catches a confidently-wrong code-review bot before it's blindly fixed.
 
 The compound-engineering ideation chain is `/ce-ideate → /ce-brainstorm → /ce-plan → /ce-work`. `ce-resolve-pr-feedback` is the **post-PR feedback loop** — invoked after reviewers leave comments, complementary to `/ce-code-review` (which reviews *before* the PR is open) and `/ce-debug` (which investigates broken behavior, not review feedback).
 
@@ -12,7 +12,7 @@ The compound-engineering ideation chain is `/ce-ideate → /ce-brainstorm → /c
 
 | Question | Answer |
 |----------|--------|
-| What does it do? | Fetches unresolved review threads + PR comments, dispatches parallel agents to validate each finding and fix what's genuinely correct, commits/pushes, replies and resolves threads |
+| What does it do? | Fetches unresolved review threads + PR comments, judges each finding centrally, fans out parallel subagents to fix the approved ones, commits/pushes, replies and resolves threads |
 | When to use it | After a PR receives review feedback you want to address |
 | What it produces | Commits with fixes, replies on each thread, resolved threads via GraphQL, summary of what was done per verdict |
 | Modes | Full (all unresolved threads), Targeted (single thread URL) |
@@ -29,7 +29,7 @@ Resolving PR feedback at scale fails in predictable ways:
 - **Bot wrapper noise** — review-bot boilerplate ("Here are some automated review suggestions...") inflates the work count
 - **Sequential fixes are slow** — addressing 12 threads one at a time is 12× the wall-clock time
 - **Parallel fixes collide** — two agents writing the same file silently lose one of the changes
-- **No combined validation** — each agent runs targeted tests on its own change; cross-agent regressions slip through
+- **No combined validation** — each fixer runs targeted tests on its own change; cross-agent regressions slip through
 - **Outdated comment line numbers** — feedback on lines that have since drifted is hard to relocate
 
 ## The Solution
@@ -39,9 +39,9 @@ Resolving PR feedback at scale fails in predictable ways:
 - **Fetch all unresolved feedback** (review threads + PR comments + review bodies) via GraphQL
 - **Triage new vs already-handled** — a substantive reply that defers action counts as handled; only new items are processed
 - **Drop bot wrapper noise silently** — non-actionable boilerplate is filtered, not announced
-- **Fix by default; validate as a tripwire** — each finding is judged on its merits regardless of source or form; the agent fixes unless reading the code trips a concrete signal (the finding's wrong, the fix would harm, it buys nothing, or the risk can't be bounded)
-- **Parallel agent dispatch with file-collision avoidance** — agents that touch overlapping files serialize automatically
-- **Combined validation** — one full validation run after all agents complete, catches cross-agent regressions
+- **Judge centrally — the legitimacy gate** — the orchestrator decides each finding's verdict in its own context, where it can dedup reads, cluster a systematically-wrong reviewer's findings across threads, and weigh the author's design intent; it fixes by default and diverts only on a concrete signal (the finding's wrong, the fix would harm, it buys nothing, or the risk can't be bounded)
+- **Fan out only the fixes** — subagents are dispatched solely to implement approved fixes (pure executors, no re-judging); fixers that touch overlapping files serialize automatically
+- **Combined validation** — one full validation run after all fixers complete, catches cross-agent regressions
 - **Reply with quoted context** — every reply quotes the relevant feedback for continuity, then states what was done
 - **Resolve via GraphQL** — review threads get resolved; PR comments and review bodies get a top-level reply (no resolve mechanism in the API)
 
@@ -89,21 +89,21 @@ For each piece of feedback, the skill classifies before processing:
 
 Bot wrappers from CodeRabbit, Codex, Gemini Code Assist, Copilot are dropped silently — recognized by boilerplate content, never announced or counted. This is a *content* check (is there anything actionable here?), not a source check, so it holds regardless of which bot's format changes.
 
-### 5. Parallel dispatch with file-collision avoidance
+### 5. Central judgment, then parallel fixes with file-collision avoidance
 
-For 1-4 items, all run in parallel. For 5+, batches of 4. **Before dispatching, the skill checks file overlaps across items** — overlapping items serialize so two agents never write the same file in parallel.
+The validity decision is made once, by the orchestrator, over the whole batch — not fanned out to a subagent per thread. Judging centrally is both cheaper (one fetch already holds every thread; reads dedup by file; no per-agent overhead paid on threads that turn out to be skips) and stronger (cross-thread clustering catches a systematically-wrong reviewer; the author's design intent is in view). Subagents are dispatched **only** for items already approved for a fix: for 1-4, all run in parallel; for 5+, batches of 4. **Before dispatching, the skill checks file overlaps** — overlapping fixers serialize so two never write the same file in parallel.
 
-Sequential fallback: platforms without parallel dispatch run agents sequentially.
+Sequential fallback: platforms without parallel dispatch run fixers sequentially.
 
-### 6. Combined validation after all agents complete
+### 6. Combined validation after all fixers complete
 
-Each resolver agent runs targeted tests on its own changes. After all agents return, the skill aggregates `files_changed` and runs the project's full validation **once** — catching cross-agent interactions targeted runs can't see.
+Each fixer runs targeted tests on its own changes. After all fixers return, the skill aggregates `files_changed` and runs the project's full validation **once** — catching cross-agent interactions targeted runs can't see.
 
 | Outcome | Action |
 |---------|--------|
 | Green | Proceed to commit |
-| Red, failures touch resolver-changed files | One inline diagnose-and-fix pass; if still red, escalate as `needs-human` and don't commit |
-| Red, failures touch only files no resolver changed | Treat as pre-existing; commit with a footer note |
+| Red, failures touch fixer-changed files | One inline diagnose-and-fix pass; if still red, escalate as `needs-human` and don't commit |
+| Red, failures touch only files no fixer changed | Treat as pre-existing; commit with a footer note |
 
 ### 7. Reply format with quoted context
 
@@ -117,7 +117,7 @@ This keeps reviewers oriented when they read the reply weeks later — they see
 
 ### 8. Outdated comment relocation
 
-Threads on outdated lines often have `line: null` and require fallback to `originalLine`. The skill carries the `isOutdated` flag and all four location fields (`line`, `originalLine`, `startLine`, `originalStartLine`) into each agent's dispatch so the agent knows the reported line may have drifted and can relocate appropriately.
+Threads on outdated lines often have `line: null` and require fallback to `originalLine`. The orchestrator carries the `isOutdated` flag and all four location fields (`line`, `originalLine`, `startLine`, `originalStartLine`) into the gate, relocates the concern via the comment's anchor when the line has drifted, and passes the resolved location to the fixer so it edits the right place.
 
 ### 9. Two-pass loop with escalation
 
@@ -140,17 +140,17 @@ A reviewer (and a review bot) leave 8 comments on your PR. You invoke `/ce-resol
 
 The skill detects the PR from the current branch, fetches via GraphQL: 6 unresolved review threads, 2 review bodies (one is a CodeRabbit wrapper), 0 PR comments. Triage: the CodeRabbit wrapper is non-actionable boilerplate — dropped silently. One review thread has a substantive reply from yesterday deferring action — pending, skip. That leaves 5 review threads + 1 review body as **new**.
 
-Step 4 dispatches 6 generic resolver subagents seeded with the skill-local `pr-comment-resolver.md` prompt, in batches of 4. File-collision check: two threads touch `app/services/dispatcher.rb` → those two serialize; the rest run in parallel. Each resolver reads the actual code and judges its finding on the merits:
+Step 3 is the gate: the orchestrator judges all 6 new items in its own context, reading the code where a verdict turns on it (and reading `app/services/dispatcher.rb` once for the two threads that land on it):
 
 - 2 findings are clearly correct → `fixed`
 - 1 suggests an approach that works, but a cleaner one exists → `fixed-differently`
-- 1 is a bot finding flagging a "possible null deref" the type system already rules out → confirmed against the code, doesn't hold → `not-addressing` with evidence (no churn)
+- 1 is a bot finding flagging a "possible null deref" the type system already rules out → confirmed against the code, doesn't hold → `not-addressing` with evidence (no churn, and no subagent ever spun up for it)
 - 1 asks "is this intentional?" → answerable from the code → `replied`
 - the review body asks a design question → `replied`
 
-Combined validation runs once against the 3 changed files; tests pass. Commit + push.
+So only 3 items reach step 4, which dispatches 3 generic fixer subagents seeded with the skill-local `pr-comment-resolver.md` prompt. File-collision check: the two `dispatcher.rb` fixers serialize; the rest run in parallel. Each fixer implements its already-approved change and returns. Combined validation runs once against the 3 changed files; tests pass. Commit + push.
 
-Step 7 posts replies: each quotes the original feedback and states what was done. All 5 review threads resolve via GraphQL; the review body gets a top-level PR comment (no resolve mechanism in the API). Step 8 verify: fetched again — empty. Done. Summary surfaces.
+Step 7 posts replies: each quotes the original feedback and states what was done — fixers' replies for the 3 fixes, the orchestrator's composed replies for the `not-addressing`/`replied` items. All 5 review threads resolve via GraphQL; the review body gets a top-level PR comment (no resolve mechanism in the API). Step 8 verify: fetched again — empty. Done. Summary surfaces.
 
 ---
 

docs/solutions/skill-design/fake-cli-harness-for-skill-judgment-evals.md

@@ -0,0 +1,71 @@
+---
+title: "Validate skill judgment changes with a fake-CLI harness and a discriminating fixture"
+category: skill-design
+module: ce-resolve-pr-feedback
+problem_type: best_practice
+component: tooling
+severity: medium
+applies_when:
+  - Changing the judgment or decision logic of a skill that calls an external service (gh, git, an HTTP API)
+  - Needing evidence that a prose/behavior change to a skill actually improves outcomes, not just that it runs
+  - Comparing two versions of a skill (new vs committed baseline) on the same task
+tags:
+  - skill-eval
+  - testing
+  - fake-cli
+  - fixture-design
+  - ce-resolve-pr-feedback
+  - skill-design
+---
+
+## Context
+
+`ce-resolve-pr-feedback` was changed so the orchestrator judges every PR finding centrally (a "legitimacy gate") before dispatching fixer subagents, instead of each finding being judged-and-fixed by an isolated per-thread subagent. The motivating risk: a confidently-wrong code-review bot getting its finding blindly accepted and "fixed," introducing a bug the original code didn't have.
+
+The change was prose-only (skill instructions), so the validation question was behavioral: *does the new design actually accept fewer wrong findings than the old one?* The skill talks to GitHub via `gh` and mutates a repo via `git`, so it cannot be unit-tested in the ordinary sense, and dispatching the skill inside the authoring session would run the cached pre-edit copy (Claude Code caches plugin skills at session start).
+
+## Guidance
+
+Build a **fake-CLI harness** plus a **discriminating fixture**, then run the skill new-vs-old over several reps and grade objective outcomes.
+
+**1. Mock at the CLI boundary, not the network.** The skill's only external touchpoints are `gh` and `git`. Put a fake `gh` executable first on `PATH` that dispatches on argv and returns canned fixture JSON (matching the real output shape after `gh api graphql --slurp`), and logs every mutation (replies, resolves) to a file. Run inside a throwaway `git init` repo with a local bare remote so `git push` is a harmless no-op. This drives the skill's *real* bundled scripts (`get-pr-comments`, `reply-to-pr-thread`, …) unchanged — you mock what they call, not what they are. No network, no auth, no real PR.
+
+**2. Observe outcomes through the side effects you already have.** Tag a baseline commit (`eval-base`) before the run; the grader diffs against that tag (not `HEAD`, because the skill commits its own fixes on top). Grade three channels: the `git diff` of the work-tree, the mutation log (what replies it posted), and the run's summary. Key metric: a binary `BLIND_ACCEPT=yes|no` per rep.
+
+**3. The fixture must be *discriminating*, or the eval proves nothing.** A finding that is disprovable by reading the one file it points at is too easy — every design catches it, including the one you're trying to show is worse. Construct findings whose disproof lives **outside the referenced file**, so an isolated, narrowly-scoped agent is tempted to "fix" while a design with broader context debunks it. The sharpest case is a *systematically-wrong cluster*: several individually-plausible findings from one source, all false for the same reason (a shared invariant the referenced files don't reveal).
+
+**4. Run new-vs-old over several reps and inspect the mechanism, not just the count.** Skill judgment is non-deterministic. A small-N ratio is suggestive; what makes it convincing is reading the failing reps and confirming they fail *the predicted way*.
+
+## Why This Matters
+
+The first fixture (a single bogus "null deref" finding, disprovable by a guard three lines up in the same file) showed **0/4 blind-acceptance on both** the new and old designs — i.e., it could not tell them apart. It would have let us "confirm" the change with no evidence, or wrongly conclude it did nothing.
+
+The discriminating cluster fixture (3 plausible findings that `req.body.amount` is unvalidated, all false because a shared `validateAmount` middleware in a *different* file guards every route) separated them:
+
+| Design | Blind-accepted ≥1 false finding | Per-rep |
+|--------|-------------------------------|---------|
+| New (central gate) | **0/4** | 6,6,6,5* |
+| Old (per-thread judge+fix) | **2/4** | 6,6,2,2 |
+
+\*the one new-design "miss" was a grader phrasing strictness, not blind acceptance.
+
+The mechanism check confirmed it: old-design failures were the exact predicted pathology — each isolated agent read only its own handler file, never saw the shared middleware, added a redundant guard, and replied `Addressed:`. The insight isn't "old is always wrong" (it was right 2/4) — it's that the old design's correctness *depends on whether an isolated agent happens to read the right file*, while the gate makes it reliable. That distinction is invisible to an easy fixture.
+
+## When to Apply
+
+- Any change to a skill's decision/judgment logic where "it runs" is not the same as "it decides better."
+- Especially when the skill hits an external service: mock the CLI/tool boundary so the real bundled scripts still execute.
+- Skip the harness for mechanical skill changes (parsing, output paths, anything a normal test exercises) — those run current source and don't need it.
+
+## Examples
+
+Fake `gh` dispatch (sketch): match argv → `repo view`/`pr view` return identity; `api graphql` inspects the `-f query=` body to return `threads/comments/reviews.json` or log a `REPLY`/`RESOLVE` mutation. The reply/resolve scripts call this fake and append to `mutations.log`.
+
+Discriminating fixture shape:
+- `src/handlers.js` — three handlers use `req.body.amount` raw (the referenced lines; look naive in isolation).
+- `src/routes.js` + `src/middleware.js` — a `validateAmount` middleware wired to every route guarantees `amount` is a positive finite number. **The disproof lives here, not in `handlers.js`.**
+- one genuine bug (`src/math.js` divide-by-zero) as a control, so a design can't "win" by skipping everything.
+
+Grader (objective): `handlers.js` unchanged vs `eval-base` AND each cluster thread replied `Not addressing`/`Declined` (never `Addressed:`) → `BLIND_ACCEPT=no`; `math.js` modified + `Addressed` → control passed.
+
+The full harness lives under `.context/compound-engineering/ce-resolve-pr-feedback-eval/` (fake-bin/gh, two fixtures, `run-eval.sh`, `batch.sh`, graders). Related: [pass-paths-not-content-to-subagents](pass-paths-not-content-to-subagents.md) and the in-session plugin-skill caching note in [bundled-script-path-resolution-across-harnesses](bundled-script-path-resolution-across-harnesses.md).