refactor(ci): review ai-toolkit's own PRs with @uniswap/review-cli - #556
Conversation
Point .github/workflows/claude-code-review.yml at @uniswap/review-cli
from Uniswap/internal-tools, the shared reviewer already used by
Uniswap/universe and Uniswap/backend, instead of this repo's own
_claude-code-review.yml.
The reusable _claude-code-review.yml is unchanged and still published for
its 10+ external consumers, all of which pin it by commit SHA. Only
ai-toolkit's own review path moves; build-prompt.ts, post-review.ts, and
.github/prompts/pr-review/ stay in place for those callers.
Quality parity, verified against `review-cli triage` rather than assumed:
- Dependency PRs are still reviewed (skip.authors: [], dependency branch
prefixes omitted) so auto-merge-dependabot can keep gating on the
review result.
- claude[bot] draft PRs are still reviewed on open. review-cli's
skip.drafts is a single boolean that cannot express that carve-out, so
skip.drafts is false and the draft policy lives in the job-level if:.
- Title-based automation detection (chore(release):, chore(sync):) is
retained via the shared check-automated-pr action, which review-cli's
branch/author skip policy has no equivalent for.
- Fork PRs are still never reviewed, resolved via the API in the triage
job since the CLI has no fork concept.
- workflow_dispatch still takes pr_number and force_review; force_review
maps to --force, not --fresh, to preserve iterative review context.
The gate deliberately does NOT use --skip-config from review-cli's
upstream workflow template. That flag passes no policy at all, which is
not the same as using defaults: skip.drafts falls back to true (skipping
claude[bot] drafts) while branch and author skips are not applied. A
guard step fails the job if .claude/review.yml is missing, because
loadConfig treats a missing file as "use defaults" silently.
Improvements over the retired path:
- claude-opus-5 reviewers with claude-fable-5 synthesis, up from
claude-opus-4-8
- Comment triggers restricted to OWNER/MEMBER/COLLABORATOR, which the
previous workflow did not check on this public repo
- An investigation gate demotes an unsubstantiated empty-findings APPROVE
to COMMENT
- Two repo-specific reviewers encode house rules that previously lived
only in documentation: workflow-security-reviewer (expression
injection, SHA pinning, permission scope, Bullfrog steps, reusable
workflow contract breaks) and plugin-conventions-reviewer (the
mandatory plugin.json version bump, manifest drift, naming)
- Workflow-level permissions: {} deny-by-default
Verified: actionlint clean, zizmor clean, markdownlint 0 errors across
197 files, nx lint 0 errors, nx typecheck pass, 89 tests pass.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0eeb9caaf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
📚 Documentation Check ✅Verdict: Passed No files under packages/plugins/ were touched, so the mandatory plugin version-bump gate (fail_on_missing_version) doesn't apply. Every CLAUDE.md/README/doc file touched by this PR already contains thorough, consistent documentation of the change. PR #556 migrates ai-toolkit's own PR review from the homegrown Plugin version bump (critical gate): Not applicable — no file under CLAUDE.md coverage: README / guide coverage: Verified directly against the current repo (not just the truncated diff) — Root Changelog: No repo-wide CHANGELOG.md convention exists (changelog generation is per-package via No missing or stale documentation was found for this diff. ✨ No Documentation Updates NeededAll documentation appears to be up to date with the code changes. 🤖 Generated by Claude Documentation Validator | Mode: |
|
● Reviewed · against Caution Changes requested — the review job loads its own tooling from the untrusted PR head, which also breaks every already-open PR on merge. Moves ai-toolkit's own PR review from the reusable AssessmentThe migration preserves the four parity behaviors it claims — dependency PRs still reach Must address
Suggestions
Tip Teach the reviewer. React 👍 on findings that helped, 👎 on false positives. Reply to push back or add context — we aggregate this weekly to tune the bot. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Addresses all four findings from the first review-cli run on this PR. The `review` job checked out `refs/pull/N/head` and then resolved `./.github/actions/install_review_cli` and `.claude/` out of that same workspace. Two problems in one line: - Availability: the composite action does not exist on branches cut before it landed, so every open PR's next review would die with "Can't find 'action.yml'" until it rebased. - Trust: the head branch controlled the installer shell and the agent prompts in a job holding CLAUDE_CODE_OAUTH_TOKEN and a `contents: write` token. `.github/actions/**` is not covered by the `workflow` token scope that guards `.github/workflows/**`. The PR head is now checked out only as the content under analysis, with `.github/actions` + `.claude` sparse-checked-out from the default ref into `.review-tooling/` afterwards (order matters: checkout runs `git clean -ffdx`). The workspace `.claude` is replaced with the trusted copy behind a presence guard, mirroring the triage gate. Config and agent changes now take effect only once merged, which is intended. Also: - Add a step-level `timeout-minutes: 17` to Analyze. The job-level ceiling *cancels*, and `success() || failure()` excludes cancellation, so an overrun skipped Post and left the sticky on "Review running" forever. Not widened to `always()` on purpose: concurrency uses cancellation too, and a dying run would clobber its successor's sticky. - Correct the "structurally cannot write to GitHub" comment. That is a property of review-cli's pipeline, not of the token in the step's environment, and it should not be cited to relax the fork guard. Verified: actionlint clean, markdownlint clean, zizmor unchanged at 8 findings (no new ones introduced).
The previous commit's "trusted ref" checkout had no `ref:`, which does not mean the default branch: actions/checkout falls back to GITHUB_SHA, and on a `pull_request` event that is the merge commit (base + head). The head branch therefore still influenced the supposedly-trusted installer and agent set, so the fix did not actually close the hole it described. Pin it to `github.event.repository.default_branch` explicitly. The triage job's checkout stays unpinned on purpose, and now says why: it holds no CLAUDE_CODE_OAUTH_TOKEN and runs no agent, so the worst case there is a PR altering its own review eligibility rather than code execution with a credential. Its fork guard reads the API, not config, so it cannot be disabled from the PR branch.
Reproduced live on this PR: the trusted-config guard failed (expected during bootstrap), so `Install review-cli` never ran and its `bin-path` output was empty. `Post` still executed on `success() || failure()`, resolved "$REVIEW_CLI_BIN/review-cli" to `/review-cli`, and failed with exit 127 — burying the real error under a meaningless one. The reaction and reply steps did the same, just non-fatally via continue-on-error. Gate all three on `steps.install-review-cli.outputs.bin-path != ''` so the failure a developer sees is the one that actually happened. Also document the bootstrap corollary: the review job cannot succeed on the PR that introduces the tooling, because the trusted ref has no `.claude/` or `install_review_cli` yet. It goes green from the next PR on.
Independent review (code-reviewer + comment-analyzer + silent-failure-hunter)
standing in for the CI reviewer, which cannot review its own bootstrap PR.
15 findings, all verified against review-cli source and real run artifacts.
Critical — the trusted checkout silently degraded every review. review-cli
gates its full-checkout fast path on `git status --porcelain` being empty, and
`.review-tooling/` is untracked inside GITHUB_WORKSPACE, so the tree read
dirty on every run and the CLI fell back to extracting only diff-touched
files with no .git. Agents kept working and runs stayed green while losing
whole-repo Read/Grep/Glob and history — worst for the two reviewers added
here, which are told to look outside the diff. Fixed with .git/info/exclude
plus `update-index --assume-unchanged` on the .claude paths the trusted copy
reverts (exclude cannot hide tracked files), and the step now asserts the
tree is clean and warns if not. Verified in a scratch repo.
Critical — the investigation gate was inert. Demotion is
`wantsDemotion && allowedVerdicts.includes('COMMENT')`, and the default
`verdict.allowed` is [APPROVE, REQUEST_CHANGES], so the gate computed the
demotion, discarded it, and let the rubber-stamp APPROVE stand. Added
`verdict.allowed: [APPROVE, REQUEST_CHANGES, COMMENT]`. Safe for dependency
auto-merge, which gates on `needs.review.result == 'success'`, not verdict.
High — cancellation was rendered as failure. The reaction and reply steps run
under `always()`, which fires on cancellation, and concurrency cancels a
comment-triggered run whenever a push supersedes it. Both reported
"Review failed" and told the requester to retry a review its successor was
about to finish. Reaction now carries `!cancelled()`; reply posts
"Superseded". This is the same reasoning already documented for Post, applied
to its two siblings.
High — the config guards tested presence, not validity. `-s` catches missing
and empty but not malformed, and loadConfig treats invalid YAML exactly like
a missing file: defaults, no log line. Both guards now assert the parse and
`skip.authors == []`, fail-closed. Verified against five cases: real config,
malformed YAML, reintroduced bot skip, empty file, non-mapping.
Also: guard the two agent files the error message names; raise the job ceiling
to 25 so Analyze's step ceiling always fires first (tail budget is
`25 - setup - 17`, not a flat 3 minutes); annotate a declining triage gate so
it is not a silent green no-review; surface a failed running-state reply
(no `set -e` meant it reported success); verify the installed binary is
executable, the one case the bin-path gate cannot catch; `permissions: {}` on
review-skipped; and correct five factual claims — minimumReleaseAge errors
rather than downgrading, react/reply also write to the PR, the bundled agent
list omitted 4 of 11 including the two the next paragraph suppresses,
`--force` substitutes $20/200-turn defaults for the configured caps, and the
`cancelled()` wording in CLAUDE.md.
Verified: actionlint clean (it caught `cancelled()` being invalid in `env:`
and a duplicate permissions key), zizmor unchanged at 8 findings, markdownlint
clean, lefthook green.
Round 2 of the review loop found the case the previous commit missed. The tree-cleanliness fix handled untracked `.review-tooling/` and tracked `.claude` files the copy modifies or deletes, but not files the trusted copy lands that are NOT TRACKED at the PR head — every `.claude/**` file added after that branch was cut. `git diff --name-only` never lists untracked paths, so the assume-unchanged pass structurally cannot reach them. This fires on this PR's own merge: `next` tracks none of `.claude/review.yml` or the two agents, so every already-open PR would get three `??` entries, a dirty tree, and a review silently downgraded to diff-only extraction. Fixed by also excluding `/.claude/`, which hides untracked copies while still reporting tracked modifications — verified in a scratch repo reproducing the bug, the fix, and that the assume-unchanged pass is still required. Also from round 2: - Document that restoring a clean tree re-enables review-cli's `verifyFindings` pass, dead in CI until now because it is gated on workspaceShape == 'working-tree'. It resolves cited files from the workspace, where `.claude` is the pre-PR copy, so findings against `.claude/**` files a PR adds are dropped as "file not readable at HEAD". Logged rather than silent, reachable only by reviewer-tuning PRs, and not to be "fixed" by skipping the swap. - Correct the `!cancelled()` rationale. It claimed a successor replaces the 👀; none does, because only a `pull_request` run can cancel this one and such a run has an empty comment_id, so it never reaches that step. The stale ack is the deliberate trade against a false ❌. - Fix a wrong claim in triage.guidance: `auto-merge-dependabot` gates on the job RESULT, not the verdict, and `post` has no non-zero exit path, so REQUEST_CHANGES does not by itself stop a bump. Reviewers are told to say so explicitly in the finding instead. Verified: actionlint clean, zizmor unchanged at 8, review.yml parses.
Status: review findings addressed, remaining "AI review" CI failure is a bootstrap condition, not a defectAll four inline findings from the AI review run against this PR's first commit (b0eeb9c) were fixed by 79e0f04 (
I replied to each of the four threads with the specific fix commit, marked all four resolved, and dismissed the stale The current "AI review" check failure is a separate, structural condition specific to this PR. The So |
1 participant
Summary
Points
.github/workflows/claude-code-review.ymlat@uniswap/review-cliinstead of this repo's own_claude-code-review.yml. This is the same reviewerUniswap/universeandUniswap/backendalready run, so review-quality work lands in one place instead of three.Scope: the reusable workflow stays
_claude-code-review.ymlis a published product with 10 external consumers (universal-router, uniswap-ai, protocol-fees, v4-hooks-public, v4-hooks-internal, security, hooks, ai-sandbox, tjar, initializer). It is unchanged and still supported.build-prompt.ts,post-review.ts,build-prompt.spec.ts, and.github/prompts/pr-review/all stay in place for those callers — none of it is dead code.Only ai-toolkit's own review path moves. Every consumer pins by commit SHA, so nothing downstream is affected by this PR.
One consequence worth knowing: ai-toolkit's CI no longer exercises
build-prompt.ts/post-review.ts, so a regression there will not surface on an ai-toolkit PR anymore. That is now called out in.github/scripts/CLAUDE.md,.github/prompts/CLAUDE.md, and the reusable-workflow section of.github/workflows/CLAUDE.md, with a pointer to validate against a consumer repo (orai-sandbox).Quality parity, verified rather than assumed
review-cli's defaults would have silently regressed four behaviors. Each fix was checked by probing
review-cli triagedirectly:skip.branch_prefixesincludesdependabot//renovate/andskip.authorsincludes*[bot]→ skipped →auto-merge-dependabotnever fires, security bumps sit unmergedskip.authors: [], dependency prefixes omittedclaude[bot]draft PRs reviewed on openskip.draftsis a single boolean and can't express the carve-out; autonomous-task PRs are draftsskip.drafts: false, draft policy moved to the job-levelif:chore(release):,chore(sync):)check-automated-practionissue_comment/workflow_dispatchpayloads carry no head-repo fieldtriagejob; thereviewjob gates on itObserved output from the probe (
--format json, run against this branch's config):The
--skip-configtrapreview-cli's upstream workflow template runs the gate with
--skip-configto avoid a checkout. This PR deliberately does not. That flag passes no policy object, which is not the same as "use the CLI's defaults":skipDrafts = policy?.drafts ?? truestill defaults to skipping draftspolicy?.branchPrefixes/policy?.authorsare undefined, so no branch or author skips apply at allSo the
triagejob sparse-checks-out.github/actions+.claudeand runs the gate without the flag. BecauseloadConfigtreats a missing file as "use defaults" silently, aVerify review config is presentstep fails the job outright rather than letting a botched checkout produce a green run that quietly stopped reviewing dependency PRs.force_reviewmaps to--force(skip rebase detection), not--fresh—--freshalso discards prior findings and thread decisions, which would lose the iterative-review context.Improvements over the retired path
claude-opus-5reviewers withclaude-fable-5synthesis, up fromclaude-opus-4-8OWNER/MEMBER/COLLABORATOR. The previous workflow only excludedgithub-actions[bot]and never checked author association — on a public repo, any commenter could trigger an agent runfile:line:category; a thread a human replied to is never auto-resolvedworkflow-security-reviewer— expression injection intorun:, unpinned/dynamic action refs, permission scope, missing Bullfrog steps, secret exposure,fromJSONcoercion ofvars.*, and breaking changes to the reusable-workflow contracts other repos consumeplugin-conventions-reviewer— the mandatory.claude-plugin/plugin.jsonversion bump and its increment, manifest arrays drifting from directories on disk, skills registered as commands, skill/agent namingpermissions: {}deny-by-default at workflow level, with per-key rationale comments on every jobDocs updated
CLAUDE.md— new "AI Code Review of This Repository" section, plus the rule that changing a documented convention means updating the matching reviewer.github/workflows/CLAUDE.md— new section for this workflow, the--skip-configgotcha, and a scope note on the reusable workflow. Corrected the staleMAX_DIFF_LINESenv documentation (no longer read by this workflow; kept because external callers still pass it to_claude-code-review.yml).github/workflows/README.md,.github/scripts/CLAUDE.md,.github/prompts/CLAUDE.md,docs/guides/claude-integration.md— scope notes and refreshed feature/output descriptionsRequired repo configuration
secrets.CLAUDE_CODE_OAUTH_TOKEN— required. The job fails fast without it (otherwise the agent factory falls back to a mock and "reviews" nothing).ANTHROPIC_API_KEYis deliberately never forwarded to the review job.vars.REVIEW_CLI_VERSION— optional; overrides the1.10.1fallback without a commit. Matches universe's pin;1.10.xis required formodel.synthesis.secrets.DATADOG_API_KEY— optional; the CI Visibility step is skipped when unset.@uniswap/review-cliinUniswap/internal-tools→ Packages.Test plan
Static verification (this change is CI configuration, so the real end-to-end test is this PR's own review run):
actionlintclean on the new workflow (fixed an inherited SC2005)zizmorclean; also ran--persona=auditorand fixed the 3 actionable findings (workflow-levelpermissions, 2× undocumented permissions)markdownlint-cli2— 0 errors across 197 filesnx affected --target=lint— 0 errorsnx affected --target=typecheck— passnx affected --target=test— 89 passed (build-prompt.spec.tsstill green; those scripts are untouched)review-cli --explainagainst a live ai-toolkit PR — config loads, 13 agents resolve (11 bundled + the 2 new ones, confirming repo agents are additive)review-cli triageprobed across the full skip matrix abovetriagegate accepts,reviewposts a sticky summary plus inline threads, andworkflow-security-revieweris staffed (this diff is almost entirely.github/)Notes for the reviewer
install_review_clicomposite action installs from a scratch dir with its ownbunfig.toml. Two reasons, both load-bearing: the repo'sbunfig.tomlpins the whole@uniswapscope to npmjs while this package is private on GitHub Packages (bun only supports per-scope overrides), and the repo's 3-dayminimumReleaseAgewould silently resolve a freshly pinned version to an older one.max_budget_usdin.claude/review.ymlon purpose: as of 1.10.x that key is parsed and validated but never enforced, so setting it would read as a run-level ceiling while doing nothing.--explain's "Max budget" line is a display sum, not a cap. The real controls areagent_budget_usd, triage staffing, andtimeout-minutes.packages/plugins/files changed, so no plugin version bump applies.AI-Generated Description
Summary
Points
.github/workflows/claude-code-review.ymlat@uniswap/review-cliinstead of this repo's own_claude-code-review.yml. This is the same reviewerUniswap/universeandUniswap/backendalready run, so review-quality work lands in one place instead of three.11 files changed (+1129 / -282). The change is almost entirely
.github/plus docs — no plugin code and no plugin version bump.Scope: the reusable workflow stays
_claude-code-review.ymlis a published product with 10 external consumers (universal-router, uniswap-ai, protocol-fees, v4-hooks-public, v4-hooks-internal, security, hooks, ai-sandbox, tjar, initializer). It is unchanged and still supported.build-prompt.ts,post-review.ts,build-prompt.spec.ts, and.github/prompts/pr-review/all stay in place for those callers — none of it is dead code.Only ai-toolkit's own review path moves. Every consumer pins by commit SHA, so nothing downstream is affected by this PR.
One consequence worth knowing: ai-toolkit's CI no longer exercises
build-prompt.ts/post-review.ts, so a regression there will not surface on an ai-toolkit PR anymore. That is now called out in.github/scripts/CLAUDE.md,.github/prompts/CLAUDE.md, and the reusable-workflow section of.github/workflows/CLAUDE.md, with a pointer to validate against a consumer repo (orUniswap/ai-sandbox).Quality parity, verified rather than assumed
review-cli's defaults would have silently regressed four behaviors. Each fix was checked by probing
review-cli triagedirectly:skip.branch_prefixesincludesdependabot//renovate/andskip.authorsincludes*[bot]→ skipped →auto-merge-dependabotnever fires, security bumps sit unmergedskip.authors: [], dependency prefixes omittedclaude[bot]draft PRs reviewed on openskip.draftsis a single boolean and can't express the carve-out; autonomous-task PRs are draftsskip.drafts: false, draft policy moved to the job-levelif:chore(release):,chore(sync):)check-automated-practionissue_comment/workflow_dispatchpayloads carry no head-repo fieldtriagejob; thereviewjob gates on itThe
--skip-configtrapreview-cli's upstream workflow template runs the gate with
--skip-configto avoid a checkout. This PR deliberately does not. That flag passes no policy object, which is not the same as "use the CLI's defaults":skipDrafts = policy?.drafts ?? truestill defaults to skipping draftspolicy?.branchPrefixes/policy?.authorsare undefined, so no branch or author skips apply at allSo the
triagejob sparse-checks-out.github/actions+.claudeand runs the gate without the flag. BecauseloadConfigtreats a missing file as "use defaults" silently, aVerify review config is presentstep fails the job outright rather than letting a botched checkout produce a green run that quietly stopped reviewing dependency PRs.force_reviewmaps to--force(skip rebase detection), not--fresh—--freshalso discards prior findings and thread decisions, which would lose the iterative-review context.Improvements over the retired path
claude-opus-5reviewers withclaude-fable-5synthesis, up fromclaude-opus-4-8OWNER/MEMBER/COLLABORATOR. The previous workflow only excludedgithub-actions[bot]and never checked author association — on a public repo, any commenter could trigger an agent runfile:line:category; a thread a human replied to is never auto-resolvedworkflow-security-reviewer— expression injection intorun:, unpinned/dynamic action refs, permission scope, missing Bullfrog steps, secret exposure,fromJSONcoercion ofvars.*, and breaking changes to the reusable-workflow contracts other repos consumeplugin-conventions-reviewer— the mandatory.claude-plugin/plugin.jsonversion bump and its increment, manifest arrays drifting from directories on disk, skills registered as commands, skill/agent namingpermissions: {}deny-by-default at workflow level, with per-key rationale comments on every jobDocs updated
CLAUDE.md— new "AI Code Review of This Repository" section, plus the rule that changing a documented convention means updating the matching reviewer.github/workflows/CLAUDE.md— new section for this workflow, the--skip-configgotcha, and a scope note on the reusable workflow. Corrected the staleMAX_DIFF_LINESenv documentation (no longer read by this workflow; kept because external callers still pass it to_claude-code-review.yml).github/workflows/README.md,.github/scripts/CLAUDE.md,.github/prompts/CLAUDE.md,docs/guides/claude-integration.md— scope notes and refreshed feature/output descriptionsRequired repo configuration
secrets.CLAUDE_CODE_OAUTH_TOKEN— required. The job fails fast without it (otherwise the agent factory falls back to a mock and "reviews" nothing).ANTHROPIC_API_KEYis deliberately never forwarded to the review job.vars.REVIEW_CLI_VERSION— optional; overrides the1.10.1fallback without a commit. Matches universe's pin;1.10.xis required formodel.synthesis.secrets.DATADOG_API_KEY— optional; the CI Visibility step is skipped when unset.@uniswap/review-cliinUniswap/internal-tools→ Packages.Test plan
Static verification (this change is CI configuration, so the real end-to-end test is this PR's own review run):
actionlintclean on the new workflow (fixed an inherited SC2005)zizmorclean; also ran--persona=auditorand fixed the 3 actionable findings (workflow-levelpermissions, 2× undocumented permissions)markdownlint-cli2— 0 errors across 197 filesnx affected --target=lint— 0 errorsnx affected --target=typecheck— passnx affected --target=test— 89 passed (build-prompt.spec.tsstill green; those scripts are untouched)review-cli --explainagainst a live ai-toolkit PR — config loads, 13 agents resolve (11 bundled + the 2 new ones, confirming repo agents are additive)review-cli triageprobed across the full skip matrix abovetriagegate accepts,reviewposts a sticky summary plus inline threads, andworkflow-security-revieweris staffed (this diff is almost entirely.github/)Notes for the reviewer
install_review_clicomposite action installs from a scratch dir with its ownbunfig.toml. Two reasons, both load-bearing: the repo'sbunfig.tomlpins the whole@uniswapscope to npmjs while this package is private on GitHub Packages (bun only supports per-scope overrides), and the repo's 3-dayminimumReleaseAgewould silently resolve a freshly pinned version to an older one.max_budget_usdin.claude/review.ymlon purpose: as of 1.10.x that key is parsed and validated but never enforced, so setting it would read as a run-level ceiling while doing nothing.--explain's "Max budget" line is a display sum, not a cap. The real controls areagent_budget_usd, triage staffing, andtimeout-minutes.packages/plugins/files changed, so no plugin version bump applies.