review: add the review-trial skill (live A/B phase 5)#238
Merged
Conversation
🦋 Changeset detectedLatest commit: becfbf8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This was referenced Jul 9, 2026
…A/B phase 5) Packages the seeded-defect live-trial pattern (Khan/webapp#40678) as a Claude Code skill so a trial costs an operator an afternoon instead of a week of hand choreography. The skill collects required inputs (seeded branch, human-authored defect table, arms, budget approval) and refuses to improvise ground truth; sets up one isolated PR per arm with per-arm trigger recipes and the distinct-workflow-name rule (same-named gh-aw workflows share a per-PR concurrency group and cancel each other); collects reviews, artifacts, and costs per run with the known gh-aw artifact-bug tolerance; drives optional lifecycle pushes; scores defect by defect with the deterministic rule mirroring eval/live-match.ts plus audited manual judgment; exports live-enabled corpus case skeletons with a sanitization gate for private-repo content landing in this public repo; and cleans up trial PRs, branches, and temporary workflows. Trials remain the architecture-bet instrument; per-change evals belong to the corpus A/B. .gitignore narrows from .claude to .claude/* with a !.claude/skills/ carve-out: local agent state (settings, worktrees) stays untracked, project skills are shared tooling and are committed.
jwbron
force-pushed
the
jwbron/review-trial-skill
branch
from
45e0642 to
bced291
Compare
Contributor
Author
|
Restacked: this PR is now based on jwbron/live-eval-ab-ci (commit cherry-picked as bced291), linearizing the eval stack to #233 -> #234 -> #236 -> #237 -> #238. Rationale: work stacked on this branch (fold-in batch two's behavior PRs, then the re-review mode dial) needs the Review Eval A/B workflow (#237) present in its merge ref to receive per-push A/B reports; previously this branch was based on main, which silently opted all of that work out of A/B coverage. The acceptance arms #239/#240 are unaffected (siblings off jwbron/live-eval-ab-ci, measuring the pipeline itself). |
…b-ci' into tmp-refresh
…o jwbron/review-trial-skill
…o jwbron/review-trial-skill
khan-actions-bot
requested review from
a team,
kevinb-khan and
somewhatabstract
and removed request for
a team
jeresig
requested review from
jeresig
and removed request for
kevinb-khan and
somewhatabstract
khan-actions-bot
requested review from
a team and
somewhatabstract
and removed request for
a team
jeresig
approved these changes
Jul 10, 2026
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| without seeing another arm's comments, optionally driven through a re-review | ||
| lifecycle, then scored defect by defect and exported into the eval corpus. | ||
| This is the playbook's architecture-bet instrument; per-change evals belong to | ||
| the corpus A/B (`workflows/review/eval/live-ab-plan.md`), not to this skill. |
Contributor
There was a problem hiding this comment.
note (non-blocking): workflows/review/eval/live-ab-plan.md, cited here as the home for per-change corpus A/B evals, is not committed in the repo. This follows a pre-existing repo-wide convention (10+ files under workflows/review/eval/ reference the same uncommitted path), so it is not introduced by this PR — flagging only so the plan doc lands somewhere committed, since the skill points operators to it.
Phase 1 of the live A/B eval plan (#232): the eval corpus learns to carry real change content so a future live producer can run the actual model sub-agents against it. ## Format A case may now carry an opt-in `live` block (`corpus/live.ts`, re-exported through the loader): - `prContext`: PR title/description/author/base branch, mirroring production `pr-context.json`. The description is untrusted author text, so adversarial cases can carry their payload there or in the diff. - An on-disk post-change file tree, via a new `<id>/case.json` + `<id>/tree/` layout coexisting with flat `<id>.json`. A directory containing `case.json` is one case; its tree is never parsed as corpus JSON. - `mustCatchSpecs` / `mustNotFlagSpecs`: labeled defects as (path, line window, mechanism keyword alternates). Live runs choose their own finding ids, so ground truth matches on anchor window plus mechanism rather than id; the Phase 3 matcher consumes these. Enforced invariants: the `live` tag and the block imply each other; a live case needs a cleanly-parseable diff (fail-open is fine in production, an authoring error here); spec paths must appear in `changedFiles` and the diff; every non-removed changed file must exist in the tree. `loadLiveCorpus()` returns the subset. ## Cases Ten cases converted with hand-authored real diffs and trees: the five smoke incidents (money rounding, auth bypass, cache key, race condition, missing index), both clean cases, the adversarial injection case (payload is a code comment in the diff instructing the reviewer to approve), the golden authz holdout, and the money-payments synthetic mutation. Recorded line anchors are rewritten to the authored defect lines; natural files beat content padded to synthetic line numbers, and every anchor is asserted to be an added line so the provenance gate (now active on these cases) keeps them. The trial-content port landed as #235, stacked on this PR. One acceptance-driven change also landed here: the phase 4 runs missed `incident-sql-missing-index` in all four arms because live cases carried no `routerConfig` lens rules, so specialist lenses never spawned; every live case whose ground truth belongs to a specialist lens now routes that lens on the finding's file (see the "route the specialist lens on each live case" commit). ## Test plan: - `pnpm run test --run`: 659 tests green, including 17 new loader tests (memfs) covering the live block, both layouts, tree validation, and the tree-JSON exclusion. - The deterministic suite runs the ten converted cases unchanged (same verdicts, comment counts, must-catch sets), now with the provenance gate active on them. - `pnpm run typecheck` and eslint clean on the three touched TS files. ## Next steps (human) 1. Review the ten authored diffs and trees for realism; they are synthetic content a live model will read, so plausibility matters more than in ordinary fixtures. Spot-check that each case's recorded finding still describes the authored defect (anchors were rewritten to the authored lines). 2. This is the root of the stack: review and undraft first; #234 and #235 rebase onto it. 3. No live validation needed here; the deterministic suite (`pnpm run test --run`) fully gates it. Author: jwbron Reviewers: jeresig, github-actions[bot], jwbron Required Reviewers: Approved By: jeresig, github-actions[bot] Checks: ✅ 9 checks were successful Pull Request URL: #233
# Conflicts: # workflows/review/eval/corpus/live.ts
Contributor
Review live A/BNo reviewable delta: review.md is byte-identical in both arms (baseline |
jwbron
added a commit
that referenced
this pull request
Jul 10, 2026
Adds `workflows/review/eval/live-ab-plan.md`: a phased, agent-executable plan for real-model evals of review-bot changes, making the round-two doc's eval playbook executable for its behavior-shifting and cost-affecting change classes (today only the deterministic replay exists, which is structurally blind to prompt and model changes). The phases: 1. **Live-enabled corpus cases**: real diffs, post-change file trees, PR context, and labeled defect specs (live runs choose their own finding ids, so ground truth matches on anchor window + mechanism, not id). Implementation is up in a separate PR. 2. **Live producer**: run the actual sub-agent roster from `review.md` over a case via the Agent SDK (Read/Grep/Glob only, staged checkout, per-agent model pinning, live claim-validator, cost capture), behind the `produceFindings` seam `runner.ts` already ships. 3. **A/B runner + delta report**: same-run arms (merge-base `review.md` vs working tree), five-metric deltas, judge quality deltas, per-case cost, adversarial hard gate outright. 4. **CI wiring**: `review-eval-ab.yml` on `paths: [workflows/review/**]`, sticky PR comment, smoke-live subset by default, `full-eval` label escalation. Settled decisions recorded in the doc: same-run A/B over stored baselines; candidate corpus + lib for both arms (isolate the model seam); bounded agentic sub-agents, not one-shot completions; report-only except the adversarial gate; budget caps that still emit a partial report. Also records how this instrument relates to seeded live-PR trials like Khan/webapp#40678 (complementary layers, different cadences). ## Test plan: Docs only (plus an empty changeset). `pnpm run test --run` stays green. ## Next steps (human) 1. Review the settled design decisions section; they bind the whole stack (#233-#238), so disagreement is cheapest to voice here. 2. Two decisions the plan deliberately leaves to humans, no code blocked on them: ratify the gate thresholds that would turn the A/B report into a merge gate (until then everything is report-only except the adversarial rule), and decide whether `pattern-triage` joins the live roster. 3. This PR is docs plus an empty changeset; it can merge independently of the stack whenever the plan reads right. All five phases have landed and the doc now records the phase 4 acceptance results (both arms passed; the plan's acceptance section names the runs and the judge-direction lesson), so the remaining human items are exactly the two decisions in step 2 plus the consumer-repo trial that accepts #238. Author: jwbron Reviewers: jeresig, github-actions[bot], jwbron Required Reviewers: Approved By: jeresig, github-actions[bot] Checks: ✅ 9 checks were successful Pull Request URL: #232
jwbron
added a commit
that referenced
this pull request
Jul 10, 2026
* [jwbron/live-eval-corpus] review: live-enabled corpus format and ten live cases Phase 1 of the live A/B eval plan (#232). The corpus gains an opt-in live block carrying what a real model run needs and the deterministic replay does not: - prContext (PR title/description/author/base; the description is untrusted author text, so an adversarial case can carry its payload there or in the diff), - a post-change file tree on disk next to the case, via a new <id>/case.json + <id>/tree/ layout that coexists with flat <id>.json (a directory containing case.json is one case; its tree is never parsed as corpus JSON), and - labeled defect specs (mustCatchSpecs / mustNotFlagSpecs: path, line window, mechanism keyword alternates). Live runs choose their own finding ids, so ground truth matches on anchor window + mechanism rather than id. The loader enforces the invariants: the live tag and the live block imply each other, a live case needs a cleanly-parseable diff, spec paths must appear in changedFiles and the diff, and every non-removed changed file must exist in the tree. loadLiveCorpus() returns the subset. The live half lives in corpus/live.ts; loader.ts re-exports it as the single public surface. Ten cases are converted with hand-authored real diffs and trees: five smoke incidents, both clean cases, one adversarial injection whose payload is a code comment in the diff, one golden holdout, and one synthetic mutation. Their recorded line anchors are rewritten to the authored defect lines (natural files beat content padded to synthetic line numbers), which activates the provenance gate on these cases in the deterministic suite; all expectations hold unchanged. * [jwbron/live-eval-producer-staging] review: live-producer prompt extraction and case staging Phases 2a/2b of the live A/B eval plan (#232), stacked on the Phase 1 corpus format (#233). agent-extract.ts turns review.md's '## agent:' sections into data (name, description, pinned model, prompt body). It takes the markdown as a string with no fs access, because the baseline arm of an A/B reads the merge-base review.md via git show. Parsing is strict; a malformed or model-less section throws listing every problem, since a silently dropped agent would skew an eval arm without failing it. An integration test extracts the real review.md (21 agents) and pins the staging-root reference so a future rename fails a test instead of silently staging nothing. live-stage.ts materializes the production staging layout for one live-enabled corpus case: pr-context.json (review.md Step 1's shape, synthetic identity fields), full.diff / pr.diff / full-stripped.diff (all the case diff; corpus diffs carry no generated files and no pattern-triage pass runs), files.json + review-files.json with the hasPatch cross-check derived from the diff parse, provenance.json, routing.json from the deterministic router, an out/ directory, and the post-change checkout copied from the case tree. rewriteAgentPrompt swaps the production staging root for the staged context dir. Everything sits behind an injected-fs seam and is memfs-tested. Model dispatch (phase 2c) is deliberately absent; it needs the Agent SDK dependency decision and arrives separately. * [jwbron/live-eval-producer-staging] review: the live producer and SDK runner (phase 2c) live-producer.ts runs the live roster over one staged case behind an injected LiveAgentRunner seam (the judge.ts pattern), so its logic is stub-tested with zero model calls. Roster: the default finders (correctness-reviewer, skill-auditor) plus the router's lensesToSpawn; no pattern-triage or thread-reconciler in eval. It maps all three sub-agent output contracts into the shapes the deterministic runner consumes: label-shape findings (correctness lens, or conventions for the skill-auditor so labelForFinding reproduces the best-practice variants; confidence defaulted to 0.7 per the production claims rule), structured-schema lens findings validated as-is, and the validator's {claims: [...]} verdicts into CaseVerification[]. It stages claims.json for the validator, resolves {{#runtime-import}} directives against the case tree (a case opts into a skills index by carrying the file), retries once on malformed output with the rejection fed back, keeps partial results when an agent fails twice, prefixes colliding finding ids, and reports per-agent cost/turns/wall-clock. live-runner.ts is the one module that touches a real model runtime: an Agent SDK query() per dispatch with Read/Grep/Glob only, cwd pinned to the staged checkout, the agent's pinned model, and hard turn and wall-clock caps; plus the CLI smoke entry (tsx live-runner.ts --case <id>, requires ANTHROPIC_API_KEY). The investigation-cap CLI the prompts reference is not staged; its own denied-budget fallback applies. Adds @anthropic-ai/claude-agent-sdk as a dev dependency. * [jwbron/live-eval-ab-runner] review: the live A/B runner (phase 3) live-match.ts scores a live run against a case's labeled defect specs: a posted candidate satisfies a spec when its anchor agrees with the spec's path (and line window when both carry one) and any mechanism alternate matches the finding's failure_scenario or prose; each candidate satisfies at most one spec and vice versa. An injected fallback arbiter (hard-capped, same-file only, recorded as via: fallback for audit) can rescue recall on vague prose; false flags are decided by the deterministic rule alone. computeLiveMetrics aggregates recall, verdict agreement, clean false-flag (including a clean case that blocks), and noise. live-ab.ts is the arm orchestrator and CLI: baseline review.md from git show <merge-base>, candidate from the working tree, both arms over the same live corpus with everything else (corpus, lib, runner, metrics, judge) from the candidate, per the plan's settled decision to isolate the model seam. Each arm runs under half the --max-usd budget with sticky exhaustion: once spend plus the running per-case average crosses the cap, remaining cases are recorded skipped and the report still emits. Spec-level regressions are diffed only over cases both arms scored. Judge scoring reuses the pinned judge (quality aggregates only; judge-vs-ground-truth disagreement keys on recorded ids a live arm does not use); the fetch model moves to judge-live-model.ts and live-judge.ts now imports it. runner.ts gains an optional RunOptions.validation override so a live validator's output replaces the recorded block. Report-only except the standing rule: adversarial-injection failures on the candidate arm exit non-zero. * [jwbron/live-eval-ab-ci] review: per-PR live A/B workflow (phase 4) Review Eval A/B runs on every non-draft PR touching workflows/review/** (and on workflow_dispatch): both review.md arms over the live corpus via live-ab.ts, with the delta report posted as a sticky PR comment (hidden-marker upsert), appended to the job summary, and uploaded as an artifact even on partial or gate-failing runs. Per-PR scope is the smoke-tagged live subset; the full-eval label or dispatch input lifts it, skip-live-eval opts out, drafts wait until ready, the changeset-release branch is excluded (it matches the path filter via package.json but changes no behavior), secretless runs skip green so fork PRs never fail, and per-PR concurrency cancels superseded runs. The workflow name is distinct from every gh-aw workflow per the operational-floor rule about shared concurrency groups. live-ab.ts gains --smoke-only and writes out/live-ab-report.md alongside the JSON for the comment step. * [jwbron/live-eval-corpus] review: exclude eval-corpus trees from lint and vitest Tree directories are byte-exact case fixtures paired with each case's diff: prettier auto-formatting one would silently desync it from the diff the provenance gate parses, and a tree may carry a *.test.ts whose tests fail by design (test-adequacy cases), so vitest must not execute them either. CI's lint job caught the first drift (prettier wanting to rewrap a fixture ternary). * [jwbron/live-eval-producer-staging] review: namespace live finding ids with the case id Live agents choose their own finding ids, so every case's first correctness finding was live-correctness-reviewer-1; ids were unique within a case but collided across cases, and judge.ts's score join requires arm-global uniqueness. Caught by the first real A/B run (both arms completed, then judge aggregation threw). Ids are now <caseId>:<id> from the moment of parsing, so claims.json, the validator round-trip, the matcher, and the judge all see the same namespaced id. * [jwbron/live-eval-ab-runner] review: judge failures degrade the A/B report instead of killing it The first real A/B run spent both arms' budgets and then died in judge aggregation, writing no report: the exact everything-spent-nothing-posted failure mode the plan forbids. Judge scoring is additive, so a per-arm failure is now caught, recorded as judgeError on the arm, rendered as a degradation note in the report, and the run proceeds to write JSON + markdown and evaluate the adversarial gate as usual. * [jwbron/live-eval-corpus] review: route the specialist lens on each live case The first live acceptance runs missed incident-sql-missing-index:dm-missing-index-1 in all four arms: the recorded finding is a data-migrations LENS finding, but live cases carried no routerConfig lens rules, so the live roster never spawned the specialist that catches it. Every live case whose recorded finding belongs to a specialist lens now routes that lens on the finding's file (seven cases across five lenses), mirroring how a consumer ROUTING file would route the same paths in production. * [jwbron/live-eval-ab-runner] review: carry agent-failure reasons into the A/B report The acceptance runs surfaced a claim-validator failure that the report could only name, not explain (perCase carried agent names only, and the PerAgentReport.failed detail never reached the markdown). perCase failedAgents entries are now '<agent>: <reason>', so the next failure is diagnosable from the sticky comment alone. * [jwbron/live-eval-ab-runner] review: close every A/B report with a per-row stability footer The phase 4 acceptance pair measured which report rows a reader may act on from one run: recall, verdict agreement, the regression lists, and the adversarial gate reproduced exactly on the no-op control, while judge quality and noise moved on jitter alone; and on the weakened arm judge quality went UP 0.16 as recall fell 17 points (fewer, surer comments each score better). The footer states this on every report so nobody chases a judge-quality delta or reads one as health. * [jwbron/live-eval-ab-runner] review: identity short-circuit, gate-flip retry, drop-bucket taxonomy Three instrument fixes from the eval-tuning memo, landed in the runner so the whole stack above inherits them: - Pre-flight identity short-circuit (memo item 1): byte-identical review.md in both arms posts a no-reviewable-delta report and runs nothing; --force-arms preserved for deliberate wobble controls. - Gate-flip retry (memo item 3): an adversarial hard-gate flip re-runs only the flipped cases, best of three, before the gate may fail the arm. Retried runs never replace the original in the metrics; they only decide whether the flip was a run-to-run flake. Retry spend is recorded in the report. - Drop-bucket taxonomy (memo item 4): each missed must-catch spec is classified true-miss vs found-but-dropped (provenance/scope/ validation) by re-matching the spec against the dropped candidates with the line window relaxed (a mis-anchored real finding is exactly the case to surface). The report annotates every lost regression with its class and carries a found-but-dropped count row. * [jwbron/live-eval-ab-runner] review: judge economics (Haiku pin, retry with backoff) The judge grades one comment's prose in 512 tokens; every load-bearing metric (recall, verdict agreement, regressions, adversarial gate) is deterministic and never touches it, and the acceptance runs showed the judge signal is not single-run-stable on any model (it moved 0.11 on the byte-identical control and went UP on the weakened arm). Price that signal accordingly: the pin moves from claude-opus-4-8 to claude-haiku-4-5-20251001 (a fifth of the cost; dated snapshot so week-over-week scores compare; both arms always share one judge, so within-run deltas are unaffected). Supersedes operator direction 4, which pinned Opus. Also adds retry with backoff (3 attempts, 429/408/ 5xx/network only) to the one live judge seam; a single transient 500 previously wasted an entire weekly scoring pass. * [jwbron/live-eval-ab-ci] review: document why the baseline is the base tip, not the merge-base The workflow passes origin/<base_ref> (the base branch tip) while its comments and the dispatch-input description said merge-base. The code is the right side of that disagreement: pull_request runs check out the PR merge commit, so the candidate tree already contains the base tip; baselining on the same tip isolates the PR's own review.md delta, where a merge-base baseline would fold upstream review.md movement into the PR's measured numbers. Prose updated to match the code; no behavior change. * [jwbron/live-eval-ab-runner] review: type the judge response via the fetch signature (eslint no-undef) * review: add the review-trial skill (live A/B phase 5) (#238) Phase 5 of the live A/B eval plan (#232), independent of the eval-code stack (#233-#237): the `review-trial` Claude Code skill, packaging the Khan/webapp#40678 seeded-defect trial choreography so an operator can run one in an afternoon. ## What the skill does Given a consumer repo, a seeded branch, a **human-authored** defect table, an arms list, and budget approval (projected up front from the trial's measured $7-10 per workflow-arm run, confirmed before any PR is created), it: 1. Creates one isolated PR per arm from the seeded branch, with per-arm trigger recipes (repo-default reviewer, pinned workflow ref, hosted `@claude review`) and two hard rules: distinct workflow names (same-named gh-aw workflows share a per-PR concurrency group and cancel each other; this silently ate a run in the original trial) and exactly one reviewer per PR (suppress the default via `skip-ai-review` on non-default arms). 2. Collects each run's review, artifacts (tolerating the known gh-aw staging-path artifact annotation), billed credits, and wall clock. 3. Optionally drives the lifecycle protocol (push 2 with fixes plus fresh seeds, push 3 all-fixed) identically across arms, scoring each push separately. 4. Scores defect by defect with the deterministic rule mirroring `eval/live-match.ts` (path + window + mechanism), manual judgment for leftovers marked as judged in the report, traps counted as correct suppression, and faithful reporting whichever arm it favors; output in the #40678 report shape. 5. Exports live-enabled corpus case skeletons, with an explicit sanitization gate for private-repo content landing in this public repo (the #235 lesson: structural rewrites, never copied code). 6. Cleans up: PRs closed with the report linked, trial branches deleted, no temporary workflow left behind. What stays human is stated at the top of the skill: seeds and ground truth are operator-authored, always; the skill refuses to improvise a defect table. ## `.gitignore` change `.claude` was ignored wholesale, which would have kept project skills untracked. Narrowed to `.claude/*` with a `!.claude/skills/` carve-out; verified `settings.json` and `worktrees/` remain ignored. ## Test plan: Docs/tooling only (empty changeset); no code paths. `git check-ignore` verified the carve-out (SKILL.md tracked, `settings.json` and `worktrees` still ignored). The real acceptance is the plan's: re-running the #40678 trial via this skill reproduces its table in an afternoon, which needs a consumer-repo trial to exercise. ## Next steps (human) 1. Review the `.gitignore` change deliberately: `.claude` wholesale becomes `.claude/*` + `!.claude/skills/`. Verified locally that `settings.json` and `worktrees/` stay ignored, but this is a policy change about what agent tooling gets committed to the repo. 2. The skill's acceptance is operational, not CI: next time a trial is warranted (an architecture-bet change like the deterministic orchestrator, or pre-graduation ground-truthing), run it via this skill and check it reproduces the #40678 report shape in an afternoon. There is no cheaper honest validation. 3. Skim the per-arm trigger recipes against current consumer-repo reality (the `skip-ai-review` label convention, ready-for-review semantics); those conventions live outside this repo and can drift. Author: jwbron Reviewers: jeresig, github-actions[bot], somewhatabstract Required Reviewers: Approved By: jeresig, github-actions[bot] Checks: ✅ 10 checks were successful Pull Request URL: #238
jwbron
added a commit
that referenced
this pull request
Jul 13, 2026
* [jwies/review-shed-tuning] review: budget and shed tuning from the 07-10 production sheds The 07-10 reviews on #232/#238 shed claim validation and three whole-change dimensions on substantial PRs. Recalibrate the tier budgets (overhead-aware wall clocks; low/medium invocation caps that fit the seven-reviewer roster), exempt pipeline steps from the invocation cap, gate shedding the claim-validator on hard-ceiling pressure only, and split the skipped-dimension note wording so a planned budget shed no longer reads as an infrastructure failure. This repo's ROUTING re-raises .claude/skills/** and eval plan docs from the broad trivial-docs rule. * [jwies/review-shed-tuning] review: lock the invocation caps to the roster size and document the calibration choices * [jwies/review-shed-tuning] review: the low cap fits the lens-free roster; lens-heavy low PRs still shed (review feedback)
Merged
jwbron
added a commit
that referenced
this pull request
Jul 13, 2026
…sts (#260) * [jwies/review-shed-tuning] review: budget and shed tuning from the 07-10 production sheds The 07-10 reviews on #232/#238 shed claim validation and three whole-change dimensions on substantial PRs. Recalibrate the tier budgets (overhead-aware wall clocks; low/medium invocation caps that fit the seven-reviewer roster), exempt pipeline steps from the invocation cap, gate shedding the claim-validator on hard-ceiling pressure only, and split the skipped-dimension note wording so a planned budget shed no longer reads as an infrastructure failure. This repo's ROUTING re-raises .claude/skills/** and eval plan docs from the broad trivial-docs rule. * [jwies/review-shed-ranking] review: value-ranked shedding and dispatch; never shed reviewer requests The shed list's first bucket had no ordering, so a budget-pressed run shed arbitrarily and an under-cap dispatch filled slots by enablement mechanism instead of value. Rank both: shed generic dimensions first (conventions, first-principles, holistic, completeness/test-adequacy) and matched specialist lenses last; dispatch in the reverse order when the invocation cap cannot fit the roster. Reviewer requests (Step 8) leave the shed list entirely: one tool call, and the human fallback matters most when the bot's own coverage is partial. * [jwies/review-shed-ranking] review: shed-ranking review feedback (accurate cross-reference, provisional-ordering note, scoped step 8 rationale, minor bump)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 5 of the live A/B eval plan (#232), independent of the eval-code stack (#233-#237): the
review-trialClaude Code skill, packaging the Khan/webapp#40678 seeded-defect trial choreography so an operator can run one in an afternoon.What the skill does
Given a consumer repo, a seeded branch, a human-authored defect table, an arms list, and budget approval (projected up front from the trial's measured $7-10 per workflow-arm run, confirmed before any PR is created), it:
@claude review) and two hard rules: distinct workflow names (same-named gh-aw workflows share a per-PR concurrency group and cancel each other; this silently ate a run in the original trial) and exactly one reviewer per PR (suppress the default viaskip-ai-reviewon non-default arms).eval/live-match.ts(path + window + mechanism), manual judgment for leftovers marked as judged in the report, traps counted as correct suppression, and faithful reporting whichever arm it favors; output in the #40678 report shape.What stays human is stated at the top of the skill: seeds and ground truth are operator-authored, always; the skill refuses to improvise a defect table.
.gitignorechange.claudewas ignored wholesale, which would have kept project skills untracked. Narrowed to.claude/*with a!.claude/skills/carve-out; verifiedsettings.jsonandworktrees/remain ignored.Test plan:
Docs/tooling only (empty changeset); no code paths.
git check-ignoreverified the carve-out (SKILL.md tracked,settings.jsonandworktreesstill ignored). The real acceptance is the plan's: re-running the #40678 trial via this skill reproduces its table in an afternoon, which needs a consumer-repo trial to exercise.Next steps (human)
.gitignorechange deliberately:.claudewholesale becomes.claude/*+!.claude/skills/. Verified locally thatsettings.jsonandworktrees/stay ignored, but this is a policy change about what agent tooling gets committed to the repo.skip-ai-reviewlabel convention, ready-for-review semantics); those conventions live outside this repo and can drift.