diff --git a/.changeset/review-live-ab-ci.md b/.changeset/review-live-ab-ci.md new file mode 100644 index 00000000..e944470c --- /dev/null +++ b/.changeset/review-live-ab-ci.md @@ -0,0 +1,5 @@ +--- +"review": patch +--- + +CI wiring for the live A/B (live A/B plan, phase 4): the `Review Eval A/B` workflow runs on every non-draft PR touching `workflows/review/**` (plus `workflow_dispatch`), executing the arm-to-arm live eval against the PR's merge-base and posting the delta report as a sticky PR comment, a job summary, and an artifact. Per-PR runs cover the smoke-tagged live subset; a `full-eval` label (or the dispatch input) lifts that to every live case, a `skip-live-eval` label opts out, the changeset release branch is excluded, secretless runs (forks) skip green, and a new push cancels a superseded run. The job fails only when the runner's adversarial hard gate fails on the candidate arm. The runner gains `--smoke-only` and writes a markdown sibling of the JSON report for the comment step. diff --git a/.changeset/review-trial-skill.md b/.changeset/review-trial-skill.md new file mode 100644 index 00000000..dd0e9293 --- /dev/null +++ b/.changeset/review-trial-skill.md @@ -0,0 +1,4 @@ +--- +--- + +Docs/tooling only: add the `review-trial` Claude Code skill (`.claude/skills/review-trial/SKILL.md`), packaging the Khan/webapp#40678 seeded-defect live-trial choreography (live A/B plan, phase 5): isolated per-arm PRs with distinct workflow names, run collection, defect-table scoring, corpus export with the private-to-public sanitization gate, and cleanup. Seeds and ground truth stay human-authored; costs are projected and confirmed before any PR is created. diff --git a/.claude/skills/review-trial/SKILL.md b/.claude/skills/review-trial/SKILL.md new file mode 100644 index 00000000..f4c96182 --- /dev/null +++ b/.claude/skills/review-trial/SKILL.md @@ -0,0 +1,135 @@ +--- +name: review-trial +description: Run a seeded-defect live trial of the PR review workflow, comparing reviewer arms on isolated copies of one seeded PR (the Khan/webapp#40678 pattern). Use when evaluating an architecture-class review change, a re-review/lifecycle behavior change, or ground-truthing before graduating a repo to automatic mode. Invoke with a consumer repo, a seeded branch, a defect table, and an arms list. +--- + +# Seeded-defect live trial + +Choreograph the trial pattern from Khan/webapp#40678: one human-seeded PR, +copied into isolated PRs so each reviewer arm reviews identical content +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. + +## What stays human + +Do NOT author seeds or ground truth yourself. The operator supplies both; a +model authoring its own seeds would grade its own homework. If either is +missing, stop and ask; do not improvise a defect table from the branch diff. + +## Required inputs (collect all before doing anything) + +1. **Consumer repo** (e.g. `Khan/webapp`) and the **seeded branch** in it. +2. **The defect table**: one row per seed with `key`, `path`, an approximate + line window, `mechanism` (2-4 keyword/regex alternates describing the + causal defect), intended severity, and any deliberate non-defect traps + (rows the reviewers must NOT flag, with the exculpating evidence named, + e.g. "the wrapper chunks DeleteMulti; see internal/datastore/client.go"). +3. **The arms**: for each, a name plus its reviewer source, one of: + - `repo-default`: whatever reviewer the consumer repo already runs. + - `workflow @ `: the shared review workflow pinned to a tag/branch + (a candidate build or a prior release). + - `hosted`: the Claude Code GitHub app review (`@claude review`). +4. **Lifecycle plan** (optional): the push-2 content (fixes mixed with fresh + seeds, plus their defect-table rows) and the push-3 content (everything + fixed). +5. **Budget approval**: project the cost FIRST and confirm. Measured basis: + roughly $7-10 per workflow-arm run; total is arms x (1 + lifecycle + pushes), hosted-arm runs billed separately by the app. Print the + projection and get an explicit yes before creating any PR. + +## Step 1: isolated arm PRs + +For each arm, create `trial//` from the seeded branch and open a +PR in the consumer repo (title prefixed `[reviewer-trial]`, body stating it +is a trial and will be closed unmerged). One PR per arm; never point two arms +at the same PR, or their comments contaminate each other. + +Per-arm trigger setup: + +- `repo-default`: nothing extra; the repo's reviewer triggers normally. +- `workflow @ ref`: commit the compiled workflow for that ref onto the arm + branch. **Give it a workflow name distinct from the repo's own reviewer + and from every other arm**: same-named gh-aw workflows share a per-PR + concurrency group and silently cancel each other (this ate a run in the + original trial). Then suppress the repo's default reviewer on this PR via + its documented opt-out (`skip-ai-review` label in Khan repos), so exactly + one reviewer runs per PR. +- `hosted`: suppress the default reviewer the same way, then trigger with an + `@claude review` comment. + +Draft PRs generally do not trigger reviewers; open the PRs ready-for-review. + +## Step 2: run and collect + +For each arm: trigger, then watch to completion (`gh run watch` / +`gh pr checks`). Collect, per run: + +- The posted review: verdict, every inline comment (path, line, body), and + the review body (`gh api` on the PR's reviews and review comments). +- Run artifacts when the arm is the shared workflow (findings, claims, + verdict JSONs): `gh run download`. Tolerate the known gh-aw staging-path + artifact bug (an ERR_VALIDATION annotation; the artifacts still upload). +- Cost and wall clock: billed AI credits from the run summary, run duration + from `gh run view`. Hosted-arm cost is app-billed; record it as opaque + unless the operator supplies it. + +Record everything into a working directory as you go; a trial that loses its +transcripts cannot be scored or exported. + +## Step 3: lifecycle (when the plan includes it) + +Push the operator's push-2 content to EVERY arm branch (identical commits), +let each arm re-review, and collect again; repeat for push-3. Score each push +separately: re-review behavior (thread resolution, duplicate suppression, +scoping to new hunks) is half the point of the lifecycle. + +## Step 4: score + +Match each arm's posted comments against the defect table, per push: + +- Deterministic first: same path, line within (or overlapping) the window, + and any mechanism alternate matching the comment text, case-insensitively. + This mirrors `workflows/review/eval/live-match.ts`; one comment satisfies + at most one seed. +- Ambiguous leftovers: judge manually, and mark judged matches in the report + so a reader can audit them. +- Non-defect traps: a comment matching a trap row is a false flag; silence + on a trap is correct suppression and counts FOR the arm. + +Produce the #40678 report shape: a headline table (seeds caught, verdict, +comment count, cost, wall clock, per arm), a defect-by-defect table (one row +per seed, one column per arm), a lifecycle table when applicable, and a short +writeup naming where each margin came from. Report faithfully: misses, +wrong severities, and noise comments all go in, whichever arm they favor. + +## Step 5: export to the corpus + +Every trial compounds the corpus. Emit case skeletons in the live-enabled +format (`workflows/review/eval/corpus/`, layout `/case.json` + `tree/`; +format spec in `corpus/live.ts`): the seeded diff, the post-change tree, +`live.mustCatchSpecs` straight from the defect table, traps as +`mustNotFlagSpecs` on a clean case, and recorded findings taken from the +best arm's artifacts. + +**Sanitization gate**: if the consumer repo is private and the corpus repo +public (Khan/webapp into Khan/actions is exactly this), never copy code, +paths, or identifiers. Author structural rewrites that reproduce each +defect mechanism with generic naming, the way the #40678 seeds landed in +Khan/actions#235. When in doubt, rewrite. + +## Step 6: clean up + +Close every trial PR with a comment linking the report, delete the +`trial//*` branches, and confirm no temporary workflow file survived +onto a long-lived branch. Leave the collected transcripts with the operator +(attach the report to the PR or issue that motivated the trial). + +## Guardrails (recap) + +- Costs projected and approved before the first PR exists. +- Seeds and ground truth are operator-authored, always. +- One arm per PR; distinct workflow names; exactly one reviewer per PR. +- Score before cleanup; export before cleanup; never lose transcripts. +- Faithful reporting, including the arm you expected to win losing. diff --git a/.github/workflows/review-eval-ab.yml b/.github/workflows/review-eval-ab.yml new file mode 100644 index 00000000..9f1c6acd --- /dev/null +++ b/.github/workflows/review-eval-ab.yml @@ -0,0 +1,133 @@ +# Review live A/B — real-model arm-to-arm evals for review-bot changes. +# +# A PR touching workflows/review/** runs the model sub-agents from BOTH the +# base branch's review.md (baseline) and the PR's review.md (candidate) over +# the live-enabled corpus subset, then posts the delta report (five live +# metrics, judge quality, cost, regressions) as a sticky PR comment and job +# summary. See workflows/review/eval/live-ab-plan.md for the full design. +# +# Why the base TIP and not the merge-base: on pull_request events the +# checkout is the PR merge commit, so the candidate tree already contains the +# base tip. Baselining on the same tip means the two arms differ only by the +# PR's own review.md changes; a merge-base baseline would fold any upstream +# review.md movement into the PR's measured delta. +# +# Report-only, with one exception (the playbook's standing rule): the runner +# exits non-zero when the candidate arm mishandles an adversarial-injection +# case, which fails this job. +# +# Cost control: per-PR runs cover only the live cases also tagged `smoke`; +# a `full-eval` label lifts that to every live case. The runner enforces a +# hard USD ceiling per arm and reports skipped cases rather than dying at the +# cap. A new push cancels a superseded run (concurrency below). A +# `skip-live-eval` label skips the job entirely. +# +# NOTE: this workflow's name must stay distinct from any gh-aw workflow a +# consumer runs (same-named workflows share a gh-aw concurrency group per PR +# and cancel each other; see the round-two doc's operational floor). + +name: Review Eval A/B + +on: + pull_request: + types: [opened, synchronize, reopened, ready_for_review] + paths: + - "workflows/review/**" + workflow_dispatch: + inputs: + base_ref: + description: "Baseline ref for review.md (default: the runner's merge-base fallback; PR runs pass the base-branch tip)" + required: false + max_usd: + description: "Total hard budget across both arms" + required: false + default: "40" + full: + description: "Run every live case (not just the smoke subset)" + type: boolean + required: false + default: false + +permissions: + contents: read + pull-requests: write + +concurrency: + group: review-eval-ab-${{ github.event.pull_request.number || github.run_id }} + cancel-in-progress: true + +jobs: + live-ab: + name: Live A/B over the eval corpus + runs-on: ubuntu-latest + # Drafts wait until ready; the changeset release PR matches the path + # filter (it bumps workflows/review/package.json) but changes no + # behavior; skip-live-eval opts a PR out. + if: >- + github.event_name == 'workflow_dispatch' || + (github.event.pull_request.draft == false && + github.head_ref != 'changeset-release/main' && + !contains(github.event.pull_request.labels.*.name, 'skip-live-eval')) + steps: + - uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5 + with: + # The baseline arm reads review.md from the base ref via + # `git show`, so the full history is needed. + fetch-depth: 0 + - uses: ./actions/shared-node-cache + - name: Run the live A/B + env: + ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} + # The tip of the PR's base branch (see the header comment for why + # tip, not merge-base). workflow_dispatch may override it. + BASE_REF: ${{ inputs.base_ref || format('origin/{0}', github.base_ref || 'main') }} + MAX_USD: ${{ inputs.max_usd || '40' }} + FULL_EVAL: ${{ inputs.full == true || contains(github.event.pull_request.labels.*.name, 'full-eval') }} + run: | + if [ -z "$ANTHROPIC_API_KEY" ]; then + echo "ANTHROPIC_API_KEY secret not configured; skipping the live A/B." >&2 + exit 0 + fi + SCOPE="--smoke-only" + if [ "$FULL_EVAL" = "true" ]; then + SCOPE="" + fi + pnpm dlx tsx workflows/review/eval/live-ab.ts \ + --base-ref "$BASE_REF" --max-usd "$MAX_USD" $SCOPE + - name: Upload the report artifact + # always(): a partial or gate-failing run still uploads what it wrote. + if: always() + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 + with: + name: live-ab-report + path: out/live-ab-report.* + if-no-files-found: ignore + - name: Post the sticky PR comment + if: always() && github.event_name == 'pull_request' + uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7 + with: + script: | + const fs = require("fs"); + const path = "out/live-ab-report.md"; + if (!fs.existsSync(path)) { + core.info("no report markdown; nothing to post"); + return; + } + const marker = ""; + const body = `${marker}\n${fs.readFileSync(path, "utf8")}`; + const {owner, repo} = context.repo; + const issue_number = context.payload.pull_request.number; + const comments = await github.paginate( + github.rest.issues.listComments, + {owner, repo, issue_number, per_page: 100}, + ); + const prior = comments.find((c) => c.body?.startsWith(marker)); + if (prior) { + await github.rest.issues.updateComment({ + owner, repo, comment_id: prior.id, body, + }); + } else { + await github.rest.issues.createComment({ + owner, repo, issue_number, body, + }); + } diff --git a/.gitignore b/.gitignore index 70aafff2..fe53efa8 100644 --- a/.gitignore +++ b/.gitignore @@ -6,5 +6,8 @@ actions/**/*.js !actions/secure-network/post.js coverage -.claude +# Local agent state (settings, worktrees) stays untracked, but project +# skills are shared tooling and are committed. +.claude/* +!.claude/skills/ .mcp.json diff --git a/workflows/review/eval/live-ab.ts b/workflows/review/eval/live-ab.ts index b1da3c3d..a8947345 100644 --- a/workflows/review/eval/live-ab.ts +++ b/workflows/review/eval/live-ab.ts @@ -19,6 +19,8 @@ * [--base-ref ] baseline review.md source (default: merge-base * of HEAD and origin/main) * [--cases ] subset of live cases (default: every live case) + * [--smoke-only] only live cases also tagged smoke (the per-PR + * default in CI; a full-eval label lifts it) * [--max-usd ] total hard budget across both arms (default 40) * [--no-judge] skip judge quality scoring * [--stage-root ] staging root (default: a fresh temp dir) @@ -38,7 +40,7 @@ import {tmpdir} from "node:os"; import {dirname} from "node:path"; import {extractAgents} from "./agent-extract"; -import {loadLiveCorpus, type CorpusCase} from "./corpus/loader"; +import {SMOKE_TAG, loadLiveCorpus, type CorpusCase} from "./corpus/loader"; import {aggregate, buildCorpusRequests} from "./judge"; import {liveJudgeModel} from "./judge-live-model"; import { @@ -605,7 +607,11 @@ const main = async (): Promise => { return; } - const allCases = loadLiveCorpus(); + const allCases = loadLiveCorpus().filter( + (c) => + !process.argv.includes("--smoke-only") || + c.tags.includes(SMOKE_TAG), + ); const cases = caseFilter === undefined ? allCases @@ -691,6 +697,8 @@ const main = async (): Promise => { mkdirSync(dirname(outPath), {recursive: true}); writeFileSync(outPath, JSON.stringify(report, null, 2)); const markdown = renderMarkdownReport(report); + // A sibling .md rides along for CI's sticky PR comment. + writeFileSync(outPath.replace(/\.json$/, ".md"), `${markdown}\n`); console.log(markdown); const summaryPath = process.env["GITHUB_STEP_SUMMARY"]; if (summaryPath !== undefined && summaryPath !== "") {