review: per-PR live A/B workflow (phase 4) #237
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 @ <ref>`: 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/<slug>/<arm>` 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 `<id>/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/<slug>/*` 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. |
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
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -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] | ||||||
|
Contributor
There was a problem hiding this comment. suggestion (non-blocking): |
||||||
| 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') }} | ||||||
|
Contributor
There was a problem hiding this comment. suggestion (non-blocking): |
||||||
| 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 \ | ||||||
|
Contributor
There was a problem hiding this comment. thought (non-blocking): The header and PR description say the job fails only when the adversarial gate trips, but the runner also exits non-zero from its top-level catch and the "no case scored on the candidate arm" path ( |
||||||
| --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 | ||||||
|
Contributor
There was a problem hiding this comment. suggestion (non-blocking): The other hand-authored workflows pin
Suggested change
|
||||||
| 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 = "<!-- review-eval-ab -->"; | ||||||
| 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, | ||||||
| }); | ||||||
| } | ||||||
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it seems like we're running this once per PR here in this repo? (maybe twice if we start as a draft?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly more than that: it runs on every push to a non-draft PR touching workflows/review/**, with cancel-in-progress superseding a stale run on a new push. Drafts skip entirely until ready_for_review, so starting as a draft does not add a run. Each run is the smoke subset (7 live cases per arm), measured at ~$7.70 and ~26 minutes in the acceptance runs; skip-live-eval opts a PR out, full-eval widens to the whole live corpus, and the runner enforces a hard USD ceiling either way. If per-push proves too spendy we can gate on a label instead, but per-push is what makes it a tripwire between a prompt edit and production.