wehub-resource-sync
72 lines
6.6 KiB
Markdown
72 lines
6.6 KiB
Markdown
---
|
|
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), the in-session plugin-skill caching note in [bundled-script-path-resolution-across-harnesses](bundled-script-path-resolution-across-harnesses.md), and the sibling prose-injection variant [paired-old-vs-new-injection-skill-evals](paired-old-vs-new-injection-skill-evals.md) (injects SKILL.md excerpts into blind subagents instead of mocking a CLI boundary).
|