[PR #399] [MERGED] feat(cursor-review): optional direct-Anthropic judge via the pinned Claude Code CLI #400

New issue

Closed

opened 2026-10-26 22:39:22 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:39:22 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/399
Author: @mattmillerai
Created: 10/9/2026
Status: ✅ Merged
Merged: 10/9/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-19914-direct-judge

📝 Commits (2)

998545d feat(cursor-review): optional direct-Anthropic judge via the pinned Claude Code CLI (BE-19914)
3880687 fix(cursor-review): run the direct judge in its own job so ANTHROPIC_API_KEY never reaches consolidate

📊 Changes

10 files changed (+1001 additions, -30 deletions)

View changed files

📝 .github/cursor-review/README.md (+3 -1)
📝 .github/cursor-review/catalog-drift.py (+1 -1)
📝 .github/cursor-review/package.json (+1 -1)
📝 .github/cursor-review/tests/test_workflow_job_isolation.py (+8 -6)
➕ .github/cursor-review/tests/test_workflow_judge_direct.py (+414 -0)
📝 .github/cursor-review/tests/test_workflow_panel_integrity.py (+5 -2)
📝 .github/cursor-review/tests/test_workflow_ref_guard_parity.py (+1 -1)
📝 .github/workflow-pins/tests/test_check_workflow_pins.py (+6 -2)
📝 .github/workflows/cursor-review.yml (+559 -15)
📝 docs/callers/cursor-review.md (+3 -1)

📄 Description

Summary

This adds an opt-in direct-Anthropic judge to cursor-review.yml. It is the judge's counterpart to the direct-Anthropic review cells (#396) and reuses their pinned Claude Code CLI, their install and verify steps, and the ANTHROPIC_API_KEY secret.

Inputs: judge_direct_model (default '') and judge_direct_effort (default xhigh). Preflight emits judge_direct=true only when the id is plain ([A-Za-z0-9._-]{1,100}, no whitespace) and the gate's anthropic_key_present is true. In any other case it warns and the Cursor judge runs. An unknown effort warns and falls back to xhigh.
New judge-direct job (GitHub-hosted ubuntu-latest, contents: read, no Cursor CLI and no shell agent). It runs only when consolidate would and judge_direct == 'true':
- It rebuilds the judge input with steps byte-identical to consolidate's: checkout, assets, the reviewed diff, the ledger, the panel findings, Aggregate panel findings and Build judge prompt. A test checks that they stay identical.
- Install Claude Code CLI and Verify Claude Code CLI version are the cells' steps, reading the pin from .github/cursor-review/package.json. They are absorbed and time-boxed (5m and 2m).
- The new Run judge (direct) step does the following inside the PR checkout:
  - runs find . -type l -delete and removes .claude/, .mcp.json and CLAUDE.local.md;
  - runs claude -p on the same /tmp/judge-prompt.txt;
  - uses the same review-output-mcp.py --mode judge server, writing to /tmp/judge-findings.json;
  - keeps the Cursor judge's timeout-minutes and continue-on-error.
  ANTHROPIC_API_KEY is set in this step's env only, together with CLAUDE_CONFIG_DIR=$(mktemp -d -p "$RUNNER_TEMP") and CLAUDE_CODE_DISABLE_NONESSENTIAL_TRAFFIC=1. Every stdin Python in this job runs as python3 -I.
- Exfil guard: Refuse a judge review carrying the API key runs with always(). If the review contains the key (LC_ALL=C grep -qF), it replaces the file with the --init error seed. It then logs only status=<ok|error|?>, N finding(s) and never the review text or the raw result.
- Upload direct judge review uploads /tmp/judge-findings.json as cursor-review-judge-direct. It is not absorbed.
consolidate job: it never references ANTHROPIC_API_KEY, and its runs-on is unchanged from main.
- With the judge direct, Install Cursor agent CLI and Run judge (Cursor) are skipped.
- Download direct judge review / Adopt direct judge review copy the artifact over the seed, but only when judge-direct succeeded and the file is a JSON object. Otherwise the seed stays and Build consolidated findings file takes the existing degraded panel-union path. A panel cell that uploads under that name first makes the real upload 409, which fails the job, so nothing forged is adopted.
- Metadata: the judge record, both the seed and the submitted review, is labelled <model> (direct).
Docs: added rows for both inputs to .github/cursor-review/README.md and docs/callers/cursor-review.md, with the same zero-data-retention note anthropic_direct_model has. Updated the ANTHROPIC_API_KEY row to match.

Judgment calls and deviations from the spec

No --bare. The spec's flag list includes --bare. While #396 was being merged, testing showed that on the pinned CLI (2.1.286) --bare silently drops Grep and Glob from --tools "Read,Grep,Glob", which leaves only Read. So this step mirrors the merged cells instead:
- no --bare;
- --setting-sources "" plus a fresh CLAUDE_CONFIG_DIR cover what --bare would have skipped;
- --disallowedTools "Read(//proc/**),Read(//sys/**)" is a second layer under --restricted, because the key sits in /proc/self/environ;
- the verify step fails by name if the CLI stops advertising any flag this confinement relies on.
A test pins that --bare stays out.
The judge runs in its own job, not inside consolidate. The spec puts the direct judge in consolidate. But Actions sends a job every secret its steps reference, including skipped steps. In consolidate, the key would therefore sit beside the Cursor judge (an unsandboxed shell agent over PR text) for any caller who passes it, including callers who pass it only for the direct cells. A first version moved consolidate to ubuntu-latest whenever the key was passed. That fixed the pool exposure but not the shell agent, and it changed consolidate's runner for those callers, so review led to the separate judge-direct job instead. The cost is a few duplicated steps (pinned byte-identical by a test) plus one artifact hand-off. gate resolves anthropic_key_present only when either direct input is set, and acceptance criterion 1 now holds as written: consolidate follows runs_on exactly as on main.
Install and verify are absorbed (continue-on-error) and time-boxed (5m and 2m) here, unlike in the cells. A judge whose CLI fails to install or verify, or hangs doing it, is skipped. The seed is then posted through the degraded panel-union path, the same outcome as a failed Cursor judge, instead of failing the review and posting nothing.
Mutually exclusive judges. The spec says to run claude -p instead of cursor-agent. With the judge direct, consolidate skips the Cursor judge and its CLI install and adopts the judge-direct review instead. Otherwise judge-direct does not run. A test checks that exactly one judge runs.
catalog-drift.py's LAST_CHECKED_WINDOW goes from 40 to 50. The two new preflight env lines pushed the last checked audit comment past the window, and the real-workflow test caught it.

Tests

New test_workflow_judge_direct.py:
- Preflight, executed: runs the real Define panel models script for the set, unset, missing-key (false and ''), non-plain-id (space, /, more than 100 characters, newline, $(id)) and bad-effort cases. It also checks that the panel list does not depend on the judge input.
- Isolation:
  - consolidate never mentions ANTHROPIC_API_KEY and follows runs_on;
  - judge-direct is hosted and read-only, has consolidate's cap, needs (minus itself) and if: (plus judge_direct), and contains no Cursor agent and no bare python3 -;
  - its rebuilt judge input steps are byte-identical to consolidate's;
  - exactly one judge runs, and adoption requires judge-direct to have succeeded;
  - the upload is not absorbed;
  - the direct judge's cap and absorption match the Cursor step, and the full confinement argv is pinned, with find -type l -delete before claude -p;
  - the CLI install reads the manifest and is time-boxed, and the Cursor install is skipped when the judge runs direct.
- Exfil guard and hand-back, executed together with Configure structured final review, Adopt direct judge review and Build consolidated findings file:
  - a review containing the key becomes the seed, gives degraded=true and posts the panel union;
  - a clean review is adopted unchanged, with degraded=false;
  - a judge that never submitted is degraded;
  - a non-object or missing hand-back leaves the seed;
  - a forged status value is never echoed.
test_workflow_job_isolation.py: judge-direct (hosted) joins the gate and the direct cells as the only ANTHROPIC_API_KEY holders. The runs_on routing test is unchanged from main.
test_workflow_ref_guard_parity.py and test_check_workflow_pins.py: the guard count rises by one for judge-direct's own byte-identical copy of the workflows_ref guard.
The first round's mutation checks (the guard's grep, preflight's judge_direct=true, the Cursor step's if: and the symlink delete) cover code that the restructure did not change.

Residual

Not exercised end to end against the real API. No run of claude -p as the judge, with a real ANTHROPIC_API_KEY on a real PR, was made from here, because this host has no key and no caller. The confinement flags are the ones #396 verified on the pinned 2.1.286 binary, but this specific command line (judge MCP tool cursor_review_submit_final, cwd at the root of the merge-ref checkout) has only been checked structurally and by executing the surrounding steps. The first opted-in caller run should confirm that a direct judge submits (Direct judge: status=ok) and that a review posts un-degraded.
Where the "metadata" names the judge. The posted review body does not name the judge model today, for Cursor or direct. So <model> (direct) is recorded in judge-findings.json's model field, which travels in the cursor-review-consolidated payload artifact, and not in the rendered review text. If the review body itself should show the judge, that needs a post-review.py change.
Caller wiring in downstream repos is out of scope, as the spec says.

Verification

python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py': OK
python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py': OK
check_workflow_pins.py: OK
check_agents_md.py --root .: passed, with 2 pre-existing warnings
check-org-repo-literals.sh: OK
actionlint: only the job.workflow_sha expression findings, which main has too, plus shellcheck info-level notes

Provenance

Authored by: agent-work loop
Verified:
- The cursor-review unit suite is green, including the executed preflight, exfil-guard and hand-back tests and the judge-direct isolation and parity tests.
- The workflow-pins unit suite and check_workflow_pins.py are green.
- The AGENTS.md and org-repo-literal lints pass.
- actionlint reports nothing new compared with main.
- There was no live API run (see Residual).
Deviations:
- No --bare; --setting-sources "" and deny rules are used instead.
- The judge runs in its own judge-direct job instead of inside consolidate.
- The CLI install and verify are absorbed and time-boxed.
- The catalog-drift window was widened.
- All of these are explained above.

🤖 Generated with Claude Code

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/399 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/9/2026 **Status:** ✅ Merged **Merged:** 10/9/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-19914-direct-judge` --- ### 📝 Commits (2) - [`998545d`](https://github.com/Comfy-Org/github-workflows/commit/998545deffe0f5028310ca7b1a4fc8958bf81c6f) feat(cursor-review): optional direct-Anthropic judge via the pinned Claude Code CLI (BE-19914) - [`3880687`](https://github.com/Comfy-Org/github-workflows/commit/3880687bc9fb71e3547a65af4a3c33e16a12b232) fix(cursor-review): run the direct judge in its own job so ANTHROPIC_API_KEY never reaches consolidate ### 📊 Changes **10 files changed** (+1001 additions, -30 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+3 -1) 📝 `.github/cursor-review/catalog-drift.py` (+1 -1) 📝 `.github/cursor-review/package.json` (+1 -1) 📝 `.github/cursor-review/tests/test_workflow_job_isolation.py` (+8 -6) ➕ `.github/cursor-review/tests/test_workflow_judge_direct.py` (+414 -0) 📝 `.github/cursor-review/tests/test_workflow_panel_integrity.py` (+5 -2) 📝 `.github/cursor-review/tests/test_workflow_ref_guard_parity.py` (+1 -1) 📝 `.github/workflow-pins/tests/test_check_workflow_pins.py` (+6 -2) 📝 `.github/workflows/cursor-review.yml` (+559 -15) 📝 `docs/callers/cursor-review.md` (+3 -1) </details> ### 📄 Description ## Summary This adds an opt-in **direct-Anthropic judge** to `cursor-review.yml`. It is the judge's counterpart to the direct-Anthropic review cells (#396) and reuses their pinned Claude Code CLI, their install and verify steps, and the `ANTHROPIC_API_KEY` secret. - **Inputs:** `judge_direct_model` (default `''`) and `judge_direct_effort` (default `xhigh`). Preflight emits `judge_direct=true` only when the id is plain (`[A-Za-z0-9._-]{1,100}`, no whitespace) **and** the gate's `anthropic_key_present` is true. In any other case it warns and the Cursor judge runs. An unknown effort warns and falls back to `xhigh`. - **New `judge-direct` job** (GitHub-hosted `ubuntu-latest`, `contents: read`, no Cursor CLI and no shell agent). It runs only when `consolidate` would and `judge_direct == 'true'`: - It rebuilds the judge input with steps byte-identical to `consolidate`'s: checkout, assets, the reviewed diff, the ledger, the panel findings, `Aggregate panel findings` and `Build judge prompt`. A test checks that they stay identical. - `Install Claude Code CLI` and `Verify Claude Code CLI version` are the cells' steps, reading the pin from `.github/cursor-review/package.json`. They are absorbed and time-boxed (5m and 2m). - The new `Run judge (direct)` step does the following inside the PR checkout: - runs `find . -type l -delete` and removes `.claude/`, `.mcp.json` and `CLAUDE.local.md`; - runs `claude -p` on the same `/tmp/judge-prompt.txt`; - uses the same `review-output-mcp.py --mode judge` server, writing to `/tmp/judge-findings.json`; - keeps the Cursor judge's `timeout-minutes` and `continue-on-error`. `ANTHROPIC_API_KEY` is set in this step's env only, together with `CLAUDE_CONFIG_DIR=$(mktemp -d -p "$RUNNER_TEMP")` and `CLAUDE_CODE_DISABLE_NONESSENTIAL_TRAFFIC=1`. Every stdin Python in this job runs as `python3 -I`. - **Exfil guard:** `Refuse a judge review carrying the API key` runs with `always()`. If the review contains the key (`LC_ALL=C grep -qF`), it replaces the file with the `--init` error seed. It then logs only `status=<ok|error|?>, N finding(s)` and never the review text or the raw result. - `Upload direct judge review` uploads `/tmp/judge-findings.json` as `cursor-review-judge-direct`. It is not absorbed. - **`consolidate` job:** it never references `ANTHROPIC_API_KEY`, and its `runs-on` is unchanged from `main`. - With the judge direct, `Install Cursor agent CLI` and `Run judge` (Cursor) are skipped. - `Download direct judge review` / `Adopt direct judge review` copy the artifact over the seed, but only when `judge-direct` **succeeded** and the file is a JSON object. Otherwise the seed stays and `Build consolidated findings file` takes the existing degraded panel-union path. A panel cell that uploads under that name first makes the real upload 409, which fails the job, so nothing forged is adopted. - **Metadata:** the judge record, both the seed and the submitted review, is labelled `<model> (direct)`. - **Docs:** added rows for both inputs to `.github/cursor-review/README.md` and `docs/callers/cursor-review.md`, with the same zero-data-retention note `anthropic_direct_model` has. Updated the `ANTHROPIC_API_KEY` row to match. ## Judgment calls and deviations from the spec 1. **No `--bare`.** The spec's flag list includes `--bare`. While #396 was being merged, testing showed that on the pinned CLI (2.1.286) `--bare` silently drops Grep and Glob from `--tools "Read,Grep,Glob"`, which leaves only Read. So this step mirrors the merged cells instead: - no `--bare`; - `--setting-sources ""` plus a fresh `CLAUDE_CONFIG_DIR` cover what `--bare` would have skipped; - `--disallowedTools "Read(//proc/**),Read(//sys/**)"` is a second layer under `--restricted`, because the key sits in `/proc/self/environ`; - the verify step fails by name if the CLI stops advertising any flag this confinement relies on. A test pins that `--bare` stays out. 2. **The judge runs in its own job, not inside `consolidate`.** The spec puts the direct judge in `consolidate`. But Actions sends a job every secret its steps reference, including skipped steps. In `consolidate`, the key would therefore sit beside the Cursor judge (an unsandboxed shell agent over PR text) for any caller who passes it, including callers who pass it only for the direct cells. A first version moved `consolidate` to `ubuntu-latest` whenever the key was passed. That fixed the pool exposure but not the shell agent, and it changed `consolidate`'s runner for those callers, so review led to the separate `judge-direct` job instead. The cost is a few duplicated steps (pinned byte-identical by a test) plus one artifact hand-off. `gate` resolves `anthropic_key_present` only when either direct input is set, and acceptance criterion 1 now holds as written: `consolidate` follows `runs_on` exactly as on `main`. 3. **Install and verify are absorbed (`continue-on-error`) and time-boxed (5m and 2m) here, unlike in the cells.** A judge whose CLI fails to install or verify, or hangs doing it, is skipped. The seed is then posted through the degraded panel-union path, the same outcome as a failed Cursor judge, instead of failing the review and posting nothing. 4. **Mutually exclusive judges.** The spec says to run `claude -p` *instead of* `cursor-agent`. With the judge direct, `consolidate` skips the Cursor judge and its CLI install and adopts the `judge-direct` review instead. Otherwise `judge-direct` does not run. A test checks that exactly one judge runs. 5. `catalog-drift.py`'s `LAST_CHECKED_WINDOW` goes from 40 to 50. The two new preflight env lines pushed the `last checked` audit comment past the window, and the real-workflow test caught it. ## Tests - New `test_workflow_judge_direct.py`: - **Preflight, executed:** runs the real `Define panel models` script for the set, unset, missing-key (`false` and `''`), non-plain-id (space, `/`, more than 100 characters, newline, `$(id)`) and bad-effort cases. It also checks that the panel list does not depend on the judge input. - **Isolation:** - `consolidate` never mentions `ANTHROPIC_API_KEY` and follows `runs_on`; - `judge-direct` is hosted and read-only, has `consolidate`'s cap, `needs` (minus itself) and `if:` (plus `judge_direct`), and contains no Cursor agent and no bare `python3 -`; - its rebuilt judge input steps are byte-identical to `consolidate`'s; - exactly one judge runs, and adoption requires `judge-direct` to have succeeded; - the upload is not absorbed; - the direct judge's cap and absorption match the Cursor step, and the full confinement argv is pinned, with `find -type l -delete` before `claude -p`; - the CLI install reads the manifest and is time-boxed, and the Cursor install is skipped when the judge runs direct. - **Exfil guard and hand-back, executed** together with `Configure structured final review`, `Adopt direct judge review` and `Build consolidated findings file`: - a review containing the key becomes the seed, gives `degraded=true` and posts the panel union; - a clean review is adopted unchanged, with `degraded=false`; - a judge that never submitted is degraded; - a non-object or missing hand-back leaves the seed; - a forged `status` value is never echoed. - `test_workflow_job_isolation.py`: `judge-direct` (hosted) joins the gate and the direct cells as the only `ANTHROPIC_API_KEY` holders. The `runs_on` routing test is unchanged from `main`. - `test_workflow_ref_guard_parity.py` and `test_check_workflow_pins.py`: the guard count rises by one for `judge-direct`'s own byte-identical copy of the `workflows_ref` guard. - The first round's mutation checks (the guard's grep, preflight's `judge_direct=true`, the Cursor step's `if:` and the symlink delete) cover code that the restructure did not change. ## Residual - **Not exercised end to end against the real API.** No run of `claude -p` as the judge, with a real `ANTHROPIC_API_KEY` on a real PR, was made from here, because this host has no key and no caller. The confinement flags are the ones #396 verified on the pinned 2.1.286 binary, but this specific command line (judge MCP tool `cursor_review_submit_final`, cwd at the root of the merge-ref checkout) has only been checked structurally and by executing the surrounding steps. The first opted-in caller run should confirm that a direct judge submits (`Direct judge: status=ok`) and that a review posts un-degraded. - **Where the "metadata" names the judge.** The posted review body does not name the judge model today, for Cursor or direct. So `<model> (direct)` is recorded in `judge-findings.json`'s `model` field, which travels in the `cursor-review-consolidated` payload artifact, and not in the rendered review text. If the review body itself should show the judge, that needs a `post-review.py` change. - Caller wiring in downstream repos is out of scope, as the spec says. ## Verification - `python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'`: OK - `python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py'`: OK - `check_workflow_pins.py`: OK - `check_agents_md.py --root .`: passed, with 2 pre-existing warnings - `check-org-repo-literals.sh`: OK - `actionlint`: only the `job.workflow_sha` expression findings, which `main` has too, plus shellcheck info-level notes ## Provenance - **Authored by:** agent-work loop - **Verified:** - The cursor-review unit suite is green, including the executed preflight, exfil-guard and hand-back tests and the `judge-direct` isolation and parity tests. - The workflow-pins unit suite and `check_workflow_pins.py` are green. - The AGENTS.md and org-repo-literal lints pass. - actionlint reports nothing new compared with `main`. - There was no live API run (see Residual). - **Deviations:** - No `--bare`; `--setting-sources ""` and deny rules are used instead. - The judge runs in its own `judge-direct` job instead of inside `consolidate`. - The CLI install and verify are absorbed and time-boxed. - The catalog-drift window was widened. - All of these are explained above. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:39:22 +00:00