6.6 KiB
title, category, module, problem_type, component, severity, applies_when, tags
| title | category | module | problem_type | component | severity | applies_when | tags | |||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Validate skill judgment changes with a fake-CLI harness and a discriminating fixture | skill-design | ce-resolve-pr-feedback | best_practice | tooling | medium |
|
|
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 usereq.body.amountraw (the referenced lines; look naive in isolation).src/routes.js+src/middleware.js— avalidateAmountmiddleware wired to every route guaranteesamountis a positive finite number. The disproof lives here, not inhandlers.js.- one genuine bug (
src/math.jsdivide-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, the in-session plugin-skill caching note in bundled-script-path-resolution-across-harnesses, and the sibling prose-injection variant paired-old-vs-new-injection-skill-evals (injects SKILL.md excerpts into blind subagents instead of mocking a CLI boundary).