-
Notifications
You must be signed in to change notification settings - Fork 1
review: per-PR live A/B workflow (phase 4) #237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 40 commits
2d746ac
6727eb6
5c00cf3
52073ef
93770e6
30e7527
19f06cb
d1b5288
254dfc4
917cc11
0ee295a
58cd4ed
b19b08d
da1115c
a6883be
0d02672
93d8dec
2812679
2ce35a0
c0fece2
e02ac40
d225bd4
cbc838d
63097f1
9012508
25133b4
0a3d212
a547972
391151b
d2c4c70
996766f
fb81be8
a659be8
b7c3786
193ae69
7a2065c
80da666
82bc5ed
7f2fc3a
9cc1a89
205d7c7
6328aa7
9baccb7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "review": minor | ||
| --- | ||
|
|
||
| The live A/B runner (live A/B plan, phase 3). `eval/live-match.ts` maps a run's posted candidates onto a live case's labeled defect specs (deterministic path/window/mechanism rule first, then an injected hard-capped fallback arbiter whose matches are recorded for audit) and aggregates live metrics: must-catch recall, verdict agreement, clean false-flag, and noise. `eval/live-ab.ts` runs the model sub-agents from two review.md versions (the merge-base baseline via `git show`, the working-tree candidate) over the same live corpus, each arm under half a hard USD budget with sticky stop-and-report-skips semantics, replays each live result through the deterministic pipeline (`runner.ts` gains a `validation` override on `RunOptions` so a live validator's output replaces the recorded block), diffs spec-level regressions over the cases both arms scored, judges both arms' comments with the pinned judge (quality aggregates only; extracted to `eval/judge-live-model.ts`, now shared with `live-judge.ts`), and emits a JSON report plus a markdown table (also appended to GITHUB_STEP_SUMMARY). Report-only except the playbook's standing rule: the candidate arm failing any adversarial-injection case exits non-zero. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "review": minor | ||
| --- | ||
|
|
||
| Live-enabled corpus cases (live A/B eval plan, phase 1). The eval corpus gains an opt-in `live` block carrying what a REAL model run needs and the deterministic replay does not: the PR context (`prContext`, with the description treated as untrusted author text), a post-change file tree on disk next to the case (`<id>/case.json` + `<id>/tree/`, coexisting with the flat `<id>.json` layout), and labeled defect specs (`mustCatchSpecs` / `mustNotFlagSpecs`: path, line window, mechanism keyword alternates) so a live run's model-chosen finding ids can be matched to ground truth. A live case must carry the `live` tag, a cleanly-parseable diff, and every non-removed changed file in its tree; the loader enforces all of it and `loadLiveCorpus()` returns the subset. Ten cases are converted with hand-authored real diffs and trees (five smoke incidents, both clean cases, one adversarial injection whose payload lives in the diff, one golden holdout, one synthetic mutation); their recorded line anchors now point at the authored defect lines, and the deterministic suite runs them unchanged, provenance gate included. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "review": minor | ||
| --- | ||
|
|
||
| The live producer (live A/B plan, phase 2): everything needed to run the REAL model sub-agents over a live-enabled corpus case. `eval/agent-extract.ts` parses `review.md`'s `## agent:` sections into name/description/model/prompt as pure data (string in, no fs), so an A/B can extract the baseline arm from a `git show` of the merge-base; parsing is strict and lists every malformed section at once, and an integration test runs it over the real `review.md`. `eval/live-stage.ts` materializes the production `/tmp/gh-aw/review/` staging layout for a case (pr-context.json, full/pr/full-stripped diffs, files.json with hasPatch, review-files.json, provenance.json, routing.json, out/, and the post-change checkout) and rewrites extracted prompts to point at it. `eval/live-producer.ts` dispatches the default finders plus the router's lenses through an injected runner seam, maps all three output contracts (label-shape reviewers, structured-schema lenses, the claim-validator's three-state verdicts) into the exact `RecordedFinding`/`CaseVerification` shapes the deterministic runner consumes, stages `claims.json`, resolves `{{#runtime-import}}` directives from the case tree, retries once on malformed output, keeps partial results on agent failure, and accounts per-agent cost. `eval/live-runner.ts` is the one SDK-backed runner (Claude Agent SDK; Read/Grep/Glob only, cwd pinned to the staged checkout, hard turn and wall-clock caps) plus a CLI smoke entry (`pnpm dlx tsx workflows/review/eval/live-runner.ts --case <id>`, requires ANTHROPIC_API_KEY). |
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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, | ||||||
| }); | ||||||
| } | ||||||
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.