review: re-review accountability, code-rendered from the reconciler keep-list #244

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

Open

jwbron wants to merge 55 commits into main from jwbron/review-rereview-accountability

.changeset/review-live-ab-ci.md

-Original file 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.

.changeset/review-live-ab-runner.md

-Original file 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.

.changeset/review-live-producer-staging.md

-Original file 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).

.changeset/review-out-artifact-upload.md

-Original file line number
+Diff line change
@@ -0,0 +1,5 @@
+    ---
+    "review": patch
+    ---
+    Fix the out/ run-artifact upload, which failed on every run under gh-aw v0.81.6. The orchestrator passed the absolute path `/tmp/gh-aw/review/out/` exactly as instructed, but gh-aw's upload_artifact tool stages an uploaded directory under its basename and records only the staging-relative name (`out`), and the safe_outputs job then filters the staged files (`out/<agent>.json`) against `allowed-paths` with a fully anchored matcher; the absolute pattern `/tmp/gh-aw/review/out/**` therefore matched nothing, every run annotated `ERR_VALIDATION: upload_artifact: no files matched the selection criteria`, and no sub-agent outputs or `pre-existing.json` reached the run artifacts, blocking post-hoc claim auditing, the live counters, and the A/B runner, which all read them. `allowed-paths` now lists the staging-relative `out/**` (plus the absolute form, since the filter is an OR, in case a future gh-aw matches original paths). Step 9 also copies `claims.json` into `out/` before uploading, so the artifact carries the validator's input alongside its verdicts (`claim-validator.json`) and the provenance gate's set-asides (`pre-existing.json`).

.changeset/review-rereview-accountability.md

-Original file line number
+Diff line change
@@ -0,0 +1,5 @@
+    ---
+    "review": minor
+    ---
+    Re-review accountability, code-rendered. Observed on the review-v1.4.0 re-run lifecycle (Khan/webapp#40730): run 2 resolved the fixed threads and said nothing about the three blocking threads it left open, under a bare "Changes requested" body; run 3 approved with an empty body while resolving 11 threads. The verdict body now accounts for every prior thread, rendered deterministically from the `thread-reconciler`'s keep/resolve lists by the new `lib/rereview.ts` (never composed by the model): each still-unaddressed prior thread is enumerated as a link to its earlier comment ("as of \<sha\>", blocking first, with a verbatim excerpt of the earlier comment's first line), with the resolved count stated; an approval that resolved the last open threads says every prior thread is resolved. `threads.json` staging gains a `url` field (the thread's first comment `html_url`) to power the links, `renderReviewBody` gains a `rereviewSection` slot, and the redundant-approval skip treats a non-empty section as content. Fail-open: a missing or unparseable staging input renders an empty section, leaving the body exactly as before.

.changeset/review-trial-skill.md

-Original file 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.

.claude/skills/review-trial/SKILL.md

-Original file 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)
+. **Consumer repo** (e.g. `Khan/webapp`) and the **seeded branch** in it.
+. **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").
+. **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`).
+. **Lifecycle plan** (optional): the push-2 content (fixes mixed with fresh
+       seeds, plus their defect-table rows) and the push-3 content (everything
+       fixed).
+. **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.

.github/workflows/review-eval-ab.yml

-Original file 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]
+        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 = "<!-- 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,
+                  });
+                }

.gitignore

-Original file line number
+Diff line change
@@ Expand Up / @@ -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

package.json

-Original file line number
+Diff line change
@@ Expand Up / @@ -10,6 +10,7 @@ @@
             "build": "tsc -p actions/tsconfig.json"
         },
         "devDependencies": {
+            "@anthropic-ai/claude-agent-sdk": "^0.3.205",
             "@changesets/cli": "^2.29.8",
             "@khanacademy/eslint-config": "^0.1.0",
             "@swc-node/register": "^1.11.1",
@@ Expand Down @@

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

review: re-review accountability, code-rendered from the reconciler keep-list #244

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Uh oh!

Uh oh!

Uh oh!

review: re-review accountability, code-rendered from the reconciler keep-list #244

Are you sure you want to change the base?

Uh oh!

review: re-review accountability, code-rendered from the reconciler keep-list #244

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Uh oh!

Uh oh!