diff --git a/.claude/agents/plugin-conventions-reviewer.md b/.claude/agents/plugin-conventions-reviewer.md new file mode 100644 index 00000000..57472b60 --- /dev/null +++ b/.claude/agents/plugin-conventions-reviewer.md @@ -0,0 +1,93 @@ +--- +name: plugin-conventions-reviewer +description: Reviews Claude Code plugin changes against this repo's publishing contract. Finds missing or wrong semver bumps in .claude-plugin/plugin.json, manifest arrays that drift from the directories on disk, skills registered as commands, naming-convention violations between skills and agents, and documentation that no longer matches the plugin's shape. Deploy on any diff touching packages/plugins/, including docs-only changes. +tools: Read, Grep, Glob, Bash +model: sonnet +type: reviewer +category: plugin-conventions +severity: [critical, warning, info] +depth: [light, standard, thorough] +max_findings: 6 +--- + +Before producing findings, invoke the `/review` skill to load the review methodology. Apply its guidelines on severity, communication style, what makes findings valuable, and how to engage with existing discussion. + +Only produce findings the developer needs to hear. Your investigation is valuable; only its conclusions that require action or surface something non-obvious become findings. Describing the diff back to the developer is not a finding. + +You own the publishing contract for the plugins under `packages/plugins/`. These ship to the Claude Code Marketplace via `.claude-plugin/marketplace.json`, so a manifest that disagrees with the files on disk isn't a tidiness problem: a skill missing from the `skills` array simply doesn't exist for users, and a plugin edited without a version bump is a change nobody downstream ever receives. + +Nothing else in CI checks these invariants. Tests, typecheck, and lint don't read `plugin.json`. If you don't catch it, it merges. + +## How to review + +Start from the file list, not the diff content. Determine which plugin directories were touched, then for each one read `packages/plugins//.claude-plugin/plugin.json` in full and compare it against what's actually on disk. + +Verify with tools rather than reasoning from the diff: + +- **Glob** `packages/plugins//skills/*/SKILL.md` and compare the result against the manifest's `skills` array. Both directions matter: a directory absent from the array is invisible to users; an array entry with no directory is a broken reference. +- **Glob** `packages/plugins//commands/*.md` and compare against the `commands` array. +- **Read** the plugin's `version` and check it against the base branch. `git show origin/:packages/plugins//.claude-plugin/plugin.json` is the reliable way to see whether the number actually moved; the diff hunk may not include that line. +- **Read** the plugin's `CLAUDE.md` when structure changed, and the root `CLAUDE.md` version table when a version moved. + +Run `node scripts/validate-plugin.cjs packages/plugins/` when you want a mechanical check of manifest structure. Report what it says rather than paraphrasing. + +## Existing threads — emit `threadActions` for the ones you can speak to + +For each open thread about versioning, manifest contents, naming, or plugin docs, emit a `threadActions` entry. You're suggesting; synthesis decides. Skip threads outside your domain. + +- **`re_raise`** — the version still hasn't moved, the manifest still omits the skill, the doc is still stale. +- **`resolve`** — confirm from the file, not from a reply. "Bumped it" is not evidence; the version string is. +- **`leave`** — for genuinely arguable calls, most often patch-versus-minor on a change that sits on the boundary. + +## The invariants + +**The version bump is mandatory.** Any change to any file under `packages/plugins//` requires that plugin's `.claude-plugin/plugin.json` version to move, in the same commit. This is the single most-missed requirement in this repo, and it is worth checking first on every diff. + +Choosing the increment: + +- **patch** — bug fixes, typo and wording fixes, documentation updates, internal refactors with no user-visible change. +- **minor** — a new skill, agent, command, or MCP server; any backward-compatible capability. +- **major** — a removed or renamed skill/agent/command, or restructuring that breaks existing invocations. + +A docs-only edit still needs a patch bump. A renamed skill is a major bump, not a minor one, because anyone invoking the old name breaks. When the increment chosen doesn't match the change, say which increment the change warrants and why. + +**The root `CLAUDE.md` version table tracks the manifests.** When a plugin version moves, the table's row for that plugin should move with it. A version bump with a stale table is a finding. + +**Skills and commands register differently.** A skill is a directory containing `SKILL.md` and belongs in the `skills` array as `./skills/`. It must never appear in the `commands` array. A standalone command is a `.md` file under `./commands/`, registered by path in the `commands` array, and its frontmatter must **not** carry a `name` field (the filename is the name; an explicit `name` shadows it). Flag a skill added to `commands`, and flag a command whose frontmatter declares `name`. + +**Naming distinguishes skills from agents.** Everything is lowercase-hyphenated. Skills are verb-noun, because a user invokes them as an action (`review-plan`, `create-pr`). Agents are noun-role, because they name an entity that does work (`plan-reviewer`, `code-reviewer`). A skill and an agent that serve related purposes must not share a name. Flag `snake_case`, `camelCase`, an agent named as a verb phrase, or a skill named as a role. + +**Documentation is part of the change.** A plugin whose structure changed should have its own `CLAUDE.md` updated in the same PR, with component lists matching `plugin.json`. When the plugin _inventory_ changes (a skill, agent, command, or MCP server added, removed, or renamed), the Notion marketplace doc and its component counts also need updating; that one is outside the diff, so raise it as `info` with a reminder rather than treating it as blocking. + +**Nx project wiring.** Every plugin is an Nx project and needs its `project.json`. A new plugin directory without one is outside the workspace graph, so it won't build, lint, or get picked up by affected-target commands. + +## Your documented failure modes + +You will be tempted to check only the version bump and stop. The manifest-versus-disk comparison is the check that catches genuinely invisible breakage, and it requires actually globbing the directories. Do it. + +You will also be tempted to guess the previous version from the diff. If the version line isn't in the hunk, read the base-branch file. Reporting a missing bump that did happen is worse than saying nothing, because it teaches the developer that your version findings are unreliable. + +And resist grading prose. Documentation being _stale relative to the code_ is your concern; documentation being less polished than you'd write it is not. + +## Severity + +- **critical** — a manifest that breaks the plugin for users: an array entry pointing at a directory that doesn't exist, malformed `plugin.json`, or a new plugin missing `project.json`. +- **warning** — a missing version bump, a wrong increment (especially a rename taken as minor), a skill absent from the `skills` array, a skill registered under `commands`, a command frontmatter carrying `name`, or a naming-convention violation. +- **info** — a stale plugin `CLAUDE.md`, an out-of-date root version table where the bump itself was correct, or the Notion doc reminder. + +## Before submitting + +For each finding: + +1. Did you read the actual `plugin.json`, or infer it from the diff? +2. For a missing-bump finding, did you compare against the base branch? +3. For a manifest-drift finding, did you glob the directories, in both directions? +4. Is this a real contract violation, or your stylistic preference? + +Then: if the plugin change is clean, say so, and name the version transition you verified (for example "version moved 2.2.0 → 2.3.0, correct for a new skill"). + +## Output + +Return your findings. Each needs: file path, line number, severity, category (`plugin-conventions`), and a direct statement of the violation plus the concrete fix. For a version finding, give the exact expected version string. + +Decide the verdict. A missing version bump or a broken manifest justifies REQUEST_CHANGES: both are silent failures that CI won't catch. Documentation staleness pairs with APPROVE. diff --git a/.claude/agents/workflow-security-reviewer.md b/.claude/agents/workflow-security-reviewer.md new file mode 100644 index 00000000..67ba16f3 --- /dev/null +++ b/.claude/agents/workflow-security-reviewer.md @@ -0,0 +1,87 @@ +--- +name: workflow-security-reviewer +description: Reviews GitHub Actions workflows, composite actions, and CI scripts in a repo whose workflows are consumed by other repos. Finds expression injection into shell, unpinned or dynamically-referenced actions, over-broad permissions, missing egress scanning, secret leakage through logs and artifacts, and breaking changes to reusable-workflow contracts. Deploy on any diff touching .github/workflows/, .github/actions/, or .github/scripts/. +tools: Read, Grep, Glob, Bash +model: sonnet +type: reviewer +category: workflow-security +severity: [critical, warning, info] +depth: [light, standard, thorough] +max_findings: 6 +--- + +Before producing findings, invoke the `/review` skill to load the review methodology. Apply its guidelines on severity, communication style, what makes findings valuable, and how to engage with existing discussion. + +Only produce findings the developer needs to hear. Your investigation is valuable; only its conclusions that require action or surface something non-obvious become findings. Describing the diff back to the developer is not a finding. + +You review CI configuration in a repository whose workflows are a published product. The `_`-prefixed files in `.github/workflows/` are reusable workflows that other Uniswap repositories call via `uses: Uniswap/ai-toolkit/.github/workflows/@`. A defect you miss doesn't stay in this repo — it propagates to every consumer that bumps its pin. + +This inverts the usual size heuristic. A one-line change to a `permissions:` block, an action SHA, or a `run:` step is the highest-risk shape of diff here, not the lowest. Never dismiss a CI diff as too small to review. + +## How to review + +Read the changed workflow or action in full, not just the diff hunk. CI files are dense with cross-step and cross-job coupling: a step's `if:` depends on an earlier step's `id`, a job's `needs` depends on another job's `outputs`, and a reusable workflow's behavior depends on inputs its callers pass. A hunk read in isolation hides all three. + +Then verify against the repo, don't assume: + +- **Grep** for the action being changed across all workflows. If a SHA moved in one file but the same action is pinned elsewhere at the old SHA, say so and name both files. +- **Read** the reusable workflow's `on: workflow_call` block before judging whether an input change is safe. +- **Grep** for callers of a changed reusable workflow or composite action inside this repo. For external callers, note that they pin by SHA, so the risk lands when they bump, not immediately. +- **Read** the repo's `.github/workflows/CLAUDE.md`. It documents this repo's own conventions; a diff that contradicts documented convention is a finding, and one that changes behavior the doc describes without updating the doc is also a finding. + +## Existing threads — emit `threadActions` for the ones you can speak to + +For each open thread about CI configuration, permissions, pinning, or secret handling, emit a `threadActions` entry. You're suggesting; synthesis decides. Skip threads outside your domain. + +- **`re_raise`** — the CI concern still applies. The action is still unpinned, the permission is still over-broad, the interpolation is still in the `run:` block. +- **`resolve`** — confirm by reading the current file contents, not by trusting a "fixed it" reply. A reply saying an action was pinned is not evidence; the SHA in the file is. +- **`leave`** — the area didn't change and the observation is accurate but stale, or reasonable engineers would disagree (e.g. whether a 20-line inline script crosses the extraction threshold). + +## What to look for + +These are not a checklist to walk. They are what you find when you ask: can untrusted input reach a shell, can this step do more than it needs, and would a consumer break? + +**Expression injection into shell.** `${{ }}` is substituted into the `run:` script as text _before_ bash parses it, so any user-controlled field becomes shell source. `github.event.pull_request.title`, `github.event.comment.body`, `github.event.issue.body`, `github.head_ref`, and every `github.event.inputs.*` are attacker-controlled. The fix is to pass the value through `env:` and reference `"$QUOTED_VAR"` in the script. Flag the interpolation itself, and name the specific field and why it's untrusted. This is `critical` when the field is user-controlled, because it is arbitrary command execution with the job's token. + +**Unpinned or dynamic action references.** Every external action must be pinned to a full 40-character commit SHA with a trailing version comment (`# v6.0.2`). A tag or branch ref is mutable and re-points under you. Separately: `uses:` requires a static string at workflow-parse time — `${{ }}` interpolation in a `uses:` value is rejected outright, so flag it as a hard failure rather than a style issue. Check that a bumped SHA's version comment was updated to match; a stale comment is a real trap because reviewers read the comment, not the hash. + +**Permission scope.** Permissions should be declared per-job, not workflow-wide, and should be the minimum the job needs. Two specifics worth knowing rather than guessing: resolving a review thread via the GraphQL `resolveReviewThread` mutation requires `contents: write` (`pull-requests: write` alone returns "Resource not accessible by integration"), and npm OIDC trusted publishing requires `id-token: write`. Also: permissions declared inside a reusable workflow are **not** inherited by its caller, so a caller missing a permission fails at runtime even though the reusable workflow declares it. When a diff adds a scope, ask what specifically needs it, and flag scopes that look copy-pasted. + +**Missing egress scanning.** This repo requires `bullfrogsec/bullfrog` as the _first_ step of every job on a non-macOS runner, with no exception for trivial jobs that only echo a status. A new job without it, or with it not first, is a finding. Verify by reading the job's full `steps:` list. + +**Secret exposure.** A secret reaching a log or an uploaded artifact is leaked even if the workflow succeeds. Look for secrets echoed by a debug step, written into a file that a later `upload-artifact` step collects, passed as a CLI argument that gets logged, or forwarded to a job that doesn't need it. `persist-credentials: false` on `actions/checkout` matters whenever a later step runs untrusted code, since the default leaves a usable token in `.git/config`. + +**Type coercion of repository variables.** `vars.*` values are always strings, even when they hold a number. Passing one straight into a `type: number` reusable-workflow input is a type mismatch; `fromJSON(vars.X || '5000')` is the correct form. The same applies to booleans and JSON arrays. + +**Inline script sprawl.** This repo requires complex logic (roughly 50+ lines, API calls, multi-function bash) to live in `.github/scripts/` rather than inline YAML, with `set -euo pipefail` and header documentation. The repo's own thresholds: under 20 lines is explicitly acceptable inline, 50+ or API-calling logic must be extracted. The 20-50 band is unspecified, so judge it by complexity and say which it is and why: a 30-line script making authenticated API calls and parsing JSON belongs in a file; a 25-line sequence of git commands may not. + +**Reusable-workflow contract changes.** Removing or renaming an input, removing a `secrets:` entry, changing a default, or tightening a required field is a breaking change for external callers. Flag it and say what a consumer would see. Adding an optional input with a safe default is not breaking. + +## Your documented failure modes + +You will be tempted to flag every `${{ }}` you see. Most are fine: interpolation in `env:`, `if:`, `with:`, and `name:` is not shell injection. Only interpolation that lands inside a `run:` script body is. Getting this wrong trains developers to ignore you, and injection findings are the ones that most need to be believed. + +You will also be tempted to treat convention deviations as `critical`. Reserve `critical` for something that actually breaks or is actually exploitable. A missing Bullfrog step is a real policy violation and a `warning`; an injected `github.event.comment.body` in a `run:` block is `critical`. + +## Severity + +- **critical** — arbitrary command execution from user-controlled input, a leaked secret, a mutable action ref that could execute attacker code, or a change that breaks external consumers at runtime. +- **warning** — over-broad permissions, missing Bullfrog step, missing `fromJSON` coercion, stale version comment on a bumped SHA, or complex inline scripting that policy says must be extracted. +- **info** — readability of a workflow, a comment that no longer matches behavior, or a consolidation opportunity across workflows. + +## Before submitting + +For each finding: + +1. Did you read the whole file, or only the hunk? Cross-step coupling is invisible in a hunk. +2. If you claimed a convention, did you find it in `.github/workflows/CLAUDE.md` or grep at least two existing instances? +3. For an injection finding, does the value actually reach a `run:` body, and is the field genuinely user-controlled? +4. For a breaking-change finding, did you read the `workflow_call` block rather than inferring the contract? + +Then: if the CI change is clean, say so. An empty findings list is a valid outcome. + +## Output + +Return your findings. Each needs: file path, line number, severity, category (`workflow-security`), and a direct description of the problem plus the concrete fix. For injection findings, show the `env:` form you'd use instead. + +Decide the verdict. Injection, secret leakage, and consumer-breaking changes justify REQUEST_CHANGES. Convention violations on their own usually pair with APPROVE. diff --git a/.claude/review.yml b/.claude/review.yml new file mode 100644 index 00000000..e62782d0 --- /dev/null +++ b/.claude/review.yml @@ -0,0 +1,183 @@ +# review-cli config for Uniswap/ai-toolkit. +# Schema reference: packages/review-cli/src/config.ts in Uniswap/internal-tools. +# A missing file or invalid YAML falls back to defaults silently — review-cli +# never blocks a review on a config error. + +# ─── Post policy ─────────────────────────────────── +# Only CI may write reviews. Without this, any local `gh` token with repo +# scope could post a review into this repo's PRs; with it, a teammate +# running `review post` locally is refused. +post: + allowed_actors: + - github-actions + - github-actions[bot] + +# ─── Model ───────────────────────────────────────── +# Upgrade over the retired homegrown reviewer, which ran claude-opus-4-8. +# The bundled agents declare `model: sonnet` in frontmatter, which +# resolve-model.ts treats as "unpinned", so this default is what actually +# selects the model for every reviewer. +# +# `synthesis` gives the agent that writes the final review body a +# different model from the reviewers feeding it. Reading this key requires +# review-cli >= 1.10.0, which is why the workflow's fallback pin is 1.10.1. +model: + default: claude-opus-5 + synthesis: claude-fable-5 + +# ─── Budget ──────────────────────────────────────── +# Soft PER-AGENT cap, matching Uniswap/universe and Uniswap/backend. Set +# above the $5 default so a reviewer can actually investigate this repo's +# larger workflow YAML and plugin diffs. +# +# Both values below apply to PUSH-DRIVEN runs only. `review --force` (every +# comment trigger and every workflow_dispatch) substitutes the CLI's force +# defaults of $20 and 200 turns unless --budget/--max-turns are passed, so a +# human-requested review gets a HIGHER budget and a LOWER turn cap than these. +# +# There is deliberately no `max_budget_usd` here: as of review-cli 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. The real controls are +# this per-agent cap, how many reviewers triage staffs, and the +# `timeout-minutes` ceiling on the workflow job. +agent_budget_usd: 10.0 +agent_max_turns: 500 + +# ─── Limits ──────────────────────────────────────── +# Review every PR regardless of size. The retired reviewer reviewed all +# non-automated PRs (it had a "fast review" mode for small diffs but never +# skipped them), so a size-based skip would be a regression: a one-line +# change to a workflow's permissions block or a plugin version bump is +# exactly the kind of diff that matters most here. 0 disables the guard. +trivial_threshold: 0 + +# ─── Skip policy ─────────────────────────────────── +# Evaluated by `review-cli triage` at gate time, so a skipped PR never +# checks out, installs, or spawns an agent. +# +# NOTE: this block REPLACES the corresponding defaults rather than adding +# to them, so every prefix that should still be skipped is repeated here. +skip: + branch_prefixes: + # GitHub merge queue's synthetic refs — already reviewed at the source PR. + - gh-readonly-queue/ + # Graphite's stacked-diff bookkeeping refs, not pushed by humans. + - graphite-base/ + # Graphite merge queue branches. Not a review-cli default; this repo + # uses Graphite, so it has to be added explicitly. + - gtmq + # Release automation (e.g. release/next-to-main-20260727-173053). + - release/ + - cherry-pick/ + # Weekly action-version bump branches opened by + # update-action-versions.yml. Not a review-cli default either. + - chore/update-action-versions- + # Deliberately empty, overriding the `*[bot]` default. + # + # Dependency PRs MUST be reviewed: auto-merge-dependabot in + # claude-code-review.yml gates squash-merge on the review job + # succeeding, so skipping dependabot[bot] would leave security bumps + # sitting unmerged. `dependabot/` and `renovate/` are likewise omitted + # from branch_prefixes above for the same reason. + # + # This does not weaken the bot self-trigger guard on comment triggers — + # that is a separate check against the bot's own login. + authors: [] + # Draft filtering is done by the workflow's job-level `if:` instead. + # + # This is a single boolean, so it cannot express the carve-out this repo + # needs: claude-auto-tasks.yml opens DRAFT PRs via the autonomous task + # worker, and those must be reviewed when opened rather than waiting for + # a human to mark them ready for review. Leaving this `true` would skip + # every autonomous-task PR. The workflow gate reproduces the previous + # behavior exactly: review when not a draft, OR on ready_for_review, OR + # when the author is claude[bot]. + drafts: false + +# ─── Verdicts ────────────────────────────────────── +# COMMENT must be listed for the investigation gate below to do anything. +# The demotion in approve-gate.ts is `wantsDemotion && allowedVerdicts +# .includes('COMMENT')`, and the default `verdict.allowed` is +# [APPROVE, REQUEST_CHANGES] — so without COMMENT here the gate computes the +# demotion, discards it, appends a warning paragraph to the sticky, and lets +# the APPROVE stand. The guard would be advisory prose rather than a verdict. +# +# Safe for dependency auto-merge: `auto-merge-dependabot` gates on +# `needs.review.result == 'success'` (job status), not on the verdict, and +# the bot's review is not one of the required checks on `next`. +verdict: + allowed: [APPROVE, REQUEST_CHANGES, COMMENT] + +# ─── Investigation gate ──────────────────────────── +# An empty-findings APPROVE has to be backed by evidence: the minimum +# fraction of reviewable changed files the reviewers actually opened or +# grepped. Below the floor the verdict is demoted to COMMENT with the +# reason stated (which requires COMMENT in `verdict.allowed` above). This is +# the guard against a rubber-stamp approval, which the retired reviewer had +# no equivalent of. +investigation: + min_approve_coverage: + light: 0 + standard: 0.25 + thorough: 0.5 + +# ─── Diff summarization ─────────────────────────── +# Lockfiles are summarized rather than inlined so a dependency bump spends +# its prompt budget on the manifest change, not on thousands of lines of +# resolved-version churn. +diff: + summarize: + - 'bun.lock' + - 'package-lock.json' + +# ─── Triage ──────────────────────────────────────── +# Staffing guidance for the triage agent — it decides WHICH reviewers run. +# The house rules every reviewer reads come from the repo's CLAUDE.md +# files, which review-cli inlines automatically; this block is only about +# reviewer selection. +triage: + guidance: | + Uniswap/ai-toolkit is an Nx monorepo whose product is Claude Code + tooling: plugins published to the Claude Code Marketplace, reusable + GitHub Actions workflows consumed by other Uniswap repos, and a few + published npm CLIs. It is TypeScript, YAML, and Markdown only. + + There are no smart contracts and no financial math anywhere in this + repo. Never staff contract-security-reviewer or defi-risk-reviewer. + + correctness-reviewer and security-reviewer are the baseline — staff + them on anything with logic, shell scripting, auth, token handling, or + CI configuration. Then add by what the diff touches: + + - workflow-security-reviewer — any change under .github/workflows/, + .github/actions/, or .github/scripts/. Staff it even on a one-line + change: this repo's reusable workflows (the `_`-prefixed files) are + consumed by other Uniswap repos, so an unpinned action, an + interpolated `${{ }}` inside a `run:` block, an over-broad + `permissions:` scope, or a missing Bullfrog step ships to every + consumer at once. A one-line diff is the highest-risk shape here, + not the lowest. + + - plugin-conventions-reviewer — any change under packages/plugins/. + Staff it on every such diff, including docs-only ones. The + mandatory version bump in .claude-plugin/plugin.json is the single + most commonly missed requirement in this repo and no other CI gate + catches a plugin edited without a bump. + + - dependency-upgrade-reviewer — dependabot/renovate PRs and any diff + touching package.json, bun.lock, or bunfig.toml. Dependency PRs are + reviewed here rather than skipped because `auto-merge-dependabot` + needs this job to run and succeed before it enables auto-merge, and + because a bump nobody reads is the whole risk. Be accurate rather + than lenient, but note the gate is the job RESULT, not the verdict: + REQUEST_CHANGES does not by itself stop a bump, so say plainly in the + finding when a bump should not land. + + - patterns-reviewer — non-trivial TypeScript under .github/scripts/ + or packages/, especially anything that would become a shared + utility. Skip it on YAML-only and Markdown-only diffs. + + Documentation is a first-class deliverable here, not an afterthought: + CLAUDE.md and README.md files are expected to track code changes in + the same PR. Treat a stale CLAUDE.md next to a real code change as a + legitimate finding rather than a nitpick. diff --git a/.github/actions/install_review_cli/action.yml b/.github/actions/install_review_cli/action.yml new file mode 100644 index 00000000..90b4f88f --- /dev/null +++ b/.github/actions/install_review_cli/action.yml @@ -0,0 +1,90 @@ +name: 'Install review-cli' +description: 'Set up Bun and install @uniswap/review-cli from GitHub Packages into an isolated $RUNNER_TEMP directory.' + +# Used by .github/workflows/claude-code-review.yml. Kept as a composite +# action so the triage job and the review job install the cli the same +# way from a single definition. +# +# Mirrors the equivalent action in Uniswap/universe and Uniswap/backend so +# the three consumers stay debuggable against each other. + +inputs: + version: + description: '@uniswap/review-cli version to install (e.g. 1.10.1). Pin via vars.REVIEW_CLI_VERSION at the call site.' + required: true + github-token: + description: 'Token with packages:read scope. Typically secrets.GITHUB_TOKEN.' + required: true + bun-version: + description: 'Bun runtime version. Must satisfy @uniswap/review-cli engines (>=1.3.13).' + default: '1.3.13' + +outputs: + bin-path: + description: 'Absolute path to the directory containing the review-cli binary. Callers should invoke "${bin-path}/review-cli" rather than relying on $PATH.' + value: ${{ steps.install.outputs.bin-path }} + +runs: + using: composite + steps: + # Deliberately NOT vars.NODE_VERSION / the repo's "1.3.12" bun pin: + # review-cli's engines field requires bun >= 1.3.13, so this action + # pins its own floor independent of the rest of the repo's CI. + - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2.2.0 + with: + bun-version: ${{ inputs.bun-version }} + + # Two reasons this can't be a plain `bun add` in the repo root: + # + # 1. Registry routing. The repo's bunfig.toml pins the whole + # `@uniswap` scope to registry.npmjs.org, but + # @uniswap/review-cli is published privately to GitHub + # Packages. Bun (like npm) only supports per-SCOPE registry + # overrides, never per-package, so there is no way to except a + # single package from that route. + # 2. minimumReleaseAge. The repo's bunfig.toml enforces a 3-day + # minimum release age as a supply-chain control, which would + # make a freshly published review-cli version uninstallable until + # it ages in. The gate applies to exact requests too, so bun errors + # rather than silently downgrading. + # + # Both are sidestepped by installing from a scratch directory that + # carries its own bunfig.toml. Bun reads bunfig.toml relative to cwd, + # so the repo's copy never applies here and stays untouched. + # review-cli's transitive deps (no @uniswap/*) still come from + # npmjs.org via bun's built-in default. + # + # The bin path is exposed as a step output rather than appended to + # GITHUB_PATH: writing to GITHUB_PATH creates an implicit cross-step + # dependency that zizmor flags as `github-env` (a compromised earlier + # step could shadow later commands). An output forces each caller to + # opt in explicitly. + - name: Install @uniswap/review-cli (isolated registry context) + id: install + shell: bash + env: + GH_PACKAGES_TOKEN: ${{ inputs.github-token }} + REVIEW_CLI_VERSION: ${{ inputs.version }} + run: | + set -euo pipefail + install_dir="$RUNNER_TEMP/review-cli-install" + mkdir -p "$install_dir" + cat > "$install_dir/bunfig.toml" < /dev/null + bun add "@uniswap/review-cli@${REVIEW_CLI_VERSION}" + + # Consumers gate on `bin-path != ''` to avoid resolving + # "$REVIEW_CLI_BIN/review-cli" to "/review-cli" and dying on exit 127. + # That gate cannot catch a `bun add` that succeeds without linking the + # shim (a future major renaming its `bin` key, or a bun bin-linking + # change): bin-path would be non-empty and every gate would pass. + # Verify the binary is actually there and executable. + if [ ! -x "$install_dir/node_modules/.bin/review-cli" ]; then + echo "::error::bun add succeeded but $install_dir/node_modules/.bin/review-cli is missing or not executable" + exit 1 + fi + echo "bin-path=$install_dir/node_modules/.bin" >> "$GITHUB_OUTPUT" diff --git a/.github/prompts/CLAUDE.md b/.github/prompts/CLAUDE.md index c1b4154e..6df6ad2e 100644 --- a/.github/prompts/CLAUDE.md +++ b/.github/prompts/CLAUDE.md @@ -9,9 +9,12 @@ Markdown prompt templates used by GitHub Actions workflows to customize Claude A ### Code Review - **pr-review/** - Modular PR review prompt sections (see [PR Review Modular Architecture](#pr-review-modular-architecture) below) + - `fixed/` - Always-included sections (numbered 1-3, 13-15, 17-19) - `overridable/` - Sections consumers can replace (numbered 4-12, 16) + > **Scope note:** `pr-review/` feeds the reusable `_claude-code-review.yml`, which external repositories call and which remains fully supported. These sections do **not** shape reviews of ai-toolkit's own PRs — those run `@uniswap/review-cli`, whose reviewer prompts live in `.claude/agents/*-reviewer.md` and whose policy lives in `.claude/review.yml`. Editing a file here changes review behavior for consumers only; editing `.claude/agents/` changes it for this repo only. See [PR Code Review for this repository](../workflows/CLAUDE.md#pr-code-review-for-this-repository-claude-code-reviewyml). + ### PR Metadata Generation - **generate-pr-title-description.md** - Guidelines for generating PR titles and descriptions diff --git a/.github/scripts/CLAUDE.md b/.github/scripts/CLAUDE.md index f2b5d921..12a229ad 100644 --- a/.github/scripts/CLAUDE.md +++ b/.github/scripts/CLAUDE.md @@ -2,7 +2,13 @@ ## Purpose -TypeScript scripts used by GitHub Actions workflows for automated PR reviews and other CI/CD tasks. These scripts are designed to be testable and maintainable while being usable both locally (within ai-toolkit) and remotely (downloaded by external repositories). +TypeScript scripts used by GitHub Actions workflows for automated PR reviews and other CI/CD tasks. These scripts are designed to be testable and maintainable, and are downloaded at runtime by external repositories that call this repo's reusable workflows. + +> **Scope note for `build-prompt.ts` and `post-review.ts`:** these two power the reusable `_claude-code-review.yml`, which external repositories still call and which remains fully supported. They are **not** used to review ai-toolkit's own PRs — that runs `@uniswap/review-cli` (see [PR Code Review for this repository](../workflows/CLAUDE.md#pr-code-review-for-this-repository-claude-code-reviewyml)). +> +> The practical consequence: a regression in these scripts will **not** show up on an ai-toolkit PR, because ai-toolkit's CI no longer exercises them. Consumers pin the reusable workflow by commit SHA, so they will not pick up a break until they bump. Rely on `build-prompt.spec.ts` for unit coverage and validate end-to-end against a consumer repo (or `Uniswap/ai-sandbox`) before merging changes here. +> +> `post-docs-check.ts` is unaffected — it belongs to `_claude-docs-check.yml`, which ai-toolkit does still use. ## Scripts diff --git a/.github/workflows/CLAUDE.md b/.github/workflows/CLAUDE.md index 56e9d72e..10b220e2 100644 --- a/.github/workflows/CLAUDE.md +++ b/.github/workflows/CLAUDE.md @@ -19,7 +19,7 @@ Contains GitHub Actions workflow definitions that automate CI/CD, code quality, ### Code Review & PR Management (4 workflows) - `claude-code.yml` - Responds to @claude mentions in issues and PRs -- `claude-code-review.yml` - Automated PR code reviews with inline comments +- `claude-code-review.yml` - Automated PR code reviews for **this** repository, via `@uniswap/review-cli`. Does not call `_claude-code-review.yml` (see [PR Code Review for this repository](#pr-code-review-for-this-repository-claude-code-reviewyml)) - `claude-docs-check.yml` - Validates PR documentation is properly updated (CLAUDE.md, README, versions) - `generate-pr-title-description.yml` - Auto-generates PR titles and descriptions using Claude @@ -176,6 +176,10 @@ secrets: ### PR Code Review (`_claude-code-review.yml`) +> **Scope note:** this is the **reusable workflow published for other repositories**. It is fully supported and everything documented below still applies to callers. It is _not_ what ai-toolkit uses to review its own PRs — that moved to `@uniswap/review-cli`. See [PR Code Review for this repository](#pr-code-review-for-this-repository-claude-code-reviewyml). +> +> When changing this workflow, `build-prompt.ts`, `post-review.ts`, or `.github/prompts/pr-review/`, remember that ai-toolkit's own CI no longer exercises them. External consumers pin by commit SHA, so a regression here will not surface on an ai-toolkit PR — validate against a consumer repo (or `Uniswap/ai-sandbox`) before merging. + This workflow performs automated PR code reviews using Claude AI with the following features: **Key Features:** @@ -497,6 +501,109 @@ gh workflow run "Claude Code Review" -f pr_number=123 -f force_review=false When `force_review` is `true`, the workflow bypasses the patch-ID cache and runs a complete review even if the same code was previously reviewed. +### PR Code Review for this repository (`claude-code-review.yml`) + +This is how ai-toolkit reviews its **own** PRs. It runs [`@uniswap/review-cli`](https://github.com/Uniswap/internal-tools/tree/main/packages/review-cli) from `Uniswap/internal-tools` and deliberately does **not** call `_claude-code-review.yml`. + +**Why the two coexist:** `_claude-code-review.yml` is a published product with 10+ external consumers pinned to it by commit SHA. It stays. This repo simply consumes the shared reviewer that `Uniswap/universe` and `Uniswap/backend` already use, so improvements to review quality land in one place instead of three. + +**Architecture — two jobs plus three supporting jobs:** + +| Job | Runs when | Does | +| ----------------------- | ----------------------------------------------- | ---------------------------------------------------------------------------------------- | +| `check-automated` | `pull_request` only | Classifies automated PRs via the shared `check-automated-pr` action (title + branch) | +| `triage` | Gate passes | `review-cli triage` decides run/skip, resolves the PR number, fork check, 👀 ack + reply | +| `review` | `triage.run == true` and not a fork | Installs the CLI + Claude binary, then `review` (analyze) followed by `post` (write) | +| `review-skipped` | Automated PR that isn't a dependency PR | Emits a `::notice::` so the run explains itself | +| `auto-merge-dependabot` | Dependabot security/bump PR after a good review | Enables squash auto-merge | + +**The analyze/post split matters.** `review-cli review` reads the diff, runs the agents, and persists `last-run.json`. The analyze pipeline is never handed a GitHub writer, so `review-cli post` is the only verb that writes review _state_ (findings, thread resolutions, the sticky). The `react`/`reply` steps also write to the PR, but only reactions and the trigger reply. They are separate workflow steps, and `post` runs on `success() || failure()` so a partial analyze still publishes what it found. + +That split is a property of **the CLI**, not of the job's credentials. The `review` job declares `contents: write` (required by the `resolveReviewThread` mutation), so `GITHUB_TOKEN` during Analyze _is_ write-capable and the agent has Bash access. Do not cite the analyze/post split as evidence that the fork guard is redundant. + +**Gotcha — the `review` job's tooling must come from the trusted ref, not the PR head.** The `review` job checks out `refs/pull/N/head` because that is the content under analysis. Everything that _acts on_ that content is checked out separately from the default ref into `.review-tooling/`, and the job then replaces the workspace `.claude` with the trusted copy. Two independent reasons: + +1. **Availability.** A local `uses: ./.github/actions/...` resolves out of the checked-out workspace. Any PR branched before `install_review_cli` landed does not contain it, so resolving the installer from the PR head fails with `Can't find 'action.yml'` on every open PR until it rebases. +2. **Trust.** The job holds `CLAUDE_CODE_OAUTH_TOKEN` and a `contents: write` token, and the agent's prompts _are_ `.claude/agents/*.md`. Sourcing the installer shell or the agent set from the head branch lets a PR author rewrite both. Note that `.github/actions/**` is **not** covered by the `workflow` token scope that guards `.github/workflows/**`, so that gate does not help. + +Two mechanics that are easy to get wrong: + +- **`ref:` must be explicit.** Omitting `ref:` does **not** mean "the default branch". `actions/checkout` falls back to `GITHUB_SHA`, which on a `pull_request` event is the _merge commit_ (base + head) and on `issue_comment` / `workflow_dispatch` is the default branch. An unpinned "trusted" checkout is therefore head-influenced on exactly the trigger that matters most. The trusted checkout pins `ref: ${{ github.event.repository.default_branch }}`. +- **Order matters.** The trusted checkout must come **after** the PR-head checkout, because `actions/checkout` runs `git clean -ffdx` on its target and a workspace-root checkout running second would delete `.review-tooling/`. + +The `triage` job's checkout is deliberately **not** pinned this way. It holds no `CLAUDE_CODE_OAUTH_TOKEN` and runs no agent, so the worst case is a PR altering its own review eligibility rather than executing code with a credential. Note that the fork guard in that job deliberately does not read config — it resolves the head repo through the API — so it cannot be disabled from the PR branch. + +**Consequence, and it is intended:** edits to `.claude/review.yml` or `.claude/agents/*` take effect only once merged. A PR cannot review itself with a reviewer set it wrote. Iterate locally with `review-cli dev` rather than pushing a commit per change. There is no flag to point the CLI at a config outside the repo root — `loadConfig(repoRoot)` takes only a root — which is why the fix is a file copy rather than an argument. + +A corollary worth knowing before you debug it: the `review` job **cannot succeed on the PR that introduces the tooling**, because the trusted ref does not have `.claude/` or `install_review_cli` yet. The `Use trusted review config` guard fails by design, and the job goes red until that PR merges. Every PR after the bootstrap gets the real path. + +**The trusted checkout must stay invisible to git, or every review silently degrades.** review-cli gates its full-checkout fast path on `git status --porcelain` being empty (`gitIsWorkingTreeClean`, which counts untracked paths). `.review-tooling/` lives inside `GITHUB_WORKSPACE` and is untracked, so left as-is the tree reads dirty on **every** run and the CLI falls back to extracting only the files the diff touched, with no `.git`. Agents keep working and the run stays green, but they lose whole-repo `Read`/`Grep`/`Glob` and all git history. + +That hits the two reviewers this repo adds hardest, because both are told to look outside the diff: `plugin-conventions-reviewer` globs `skills/` directories a diff never touches, and `workflow-security-reviewer` greps sibling workflows for the same action pinned at an older SHA. Neither would announce the loss. + +Two mitigations, both needed: + +- `echo '/.review-tooling/' >> .git/info/exclude` hides the untracked tooling directory. +- `echo '/.claude/' >> .git/info/exclude` for the 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 next mitigation structurally cannot reach these. This one bites hardest on the merge of the PR that introduces `.claude/` at all, because every already-open PR then gets `??` entries. +- `git update-index --assume-unchanged` on the `.claude` paths the trusted copy reverted. `.git/info/exclude` cannot hide **tracked** files, and swapping in the trusted `.claude` makes any PR that edits `.claude/**` dirty — which would re-trigger the fallback for exactly the PRs most likely to be tuning the reviewers. + +The step ends by asserting `git status --porcelain` is empty and warns if it is not, because the failure is otherwise undetectable from the outside. + +**A second-order consequence of making that work, accepted deliberately.** review-cli's post-synthesis `verifyFindings` pass is gated on `workspaceShape == 'working-tree'`, so it was effectively dead in this repo's CI while the tree was always dirty. With the tree clean it runs, and it resolves cited files from the workspace — where `.claude` is now the pre-PR copy. On a PR that _adds_ a `.claude/**` file, a finding against that file is dropped as "file not readable at HEAD"; on one that lengthens a file, a finding past the trusted copy's EOF is dropped as "beyond file end". Both drops are logged rather than silent, and only reviewer-tuning PRs can reach them. Do not "fix" this by skipping the swap: that hands config and agent prompts back to the PR author, which is the trust inversion the swap exists to close. A real carve-out needs an upstream change. + +**Steps that shell out to the CLI are gated on `steps.install-review-cli.outputs.bin-path != ''`.** That output is empty whenever the install step never ran, which is exactly what happens when an earlier step fails. Without the gate, `Post` and the reaction/reply steps still execute, resolve `"$REVIEW_CLI_BIN/review-cli"` to `/review-cli`, and fail with exit 127 — replacing the real error in the log with a meaningless one. + +**Gotcha — the `triage` gate must read `.claude/review.yml`.** review-cli's upstream workflow template runs the gate with `--skip-config` to avoid a checkout. Do not copy that here. `--skip-config` passes **no** policy, which is not the same as "the CLI's built-in defaults": + +- `skip.drafts` falls back to `true`, which would skip the `claude[bot]` draft PRs the autonomous-task workflow opens +- branch and author skips are not applied at all + +So the `triage` job does a sparse checkout of `.github/actions` and `.claude`, and runs the gate **without** `--skip-config`. A `Verify review config is present` step fails the job if `.claude/review.yml` is missing, because `loadConfig` treats a missing file as "use defaults" and logs nothing — a botched checkout would otherwise silently stop reviewing dependency PRs, breaking auto-merge on a green run. + +**Configuration lives in the repo, not in workflow inputs:** + +| File / setting | Controls | +| ------------------------------------ | --------------------------------------------------------------------------------------------------------------------------------------------------- | +| `.claude/review.yml` | Model, per-agent budget, skip policy, investigation gate, diff summarization, triage staffing | +| `.claude/agents/*-reviewer.md` | Repo-specific reviewers, **added to** review-cli's bundled set (not replacing it) | +| `.github/actions/install_review_cli` | Installs the CLI from GitHub Packages into an isolated `$RUNNER_TEMP` dir. In the `review` job it is resolved from the trusted ref, not the PR head | +| `vars.REVIEW_CLI_VERSION` | CLI version override; falls back to the pin in the workflow. Never `@latest` | +| `secrets.CLAUDE_CODE_OAUTH_TOKEN` | **Required.** `ANTHROPIC_API_KEY` is deliberately never forwarded to the review job | +| `secrets.DATADOG_API_KEY` | Optional. Enables CI Visibility stamping; the step is skipped when unset | + +**Repo-specific reviewers.** review-cli ships 11 bundled agents: security, correctness, patterns, dependency-upgrade, and general reviewers; contract-security and defi-risk reviewers (neither applicable here, see `triage.guidance`); stack-security-analyst and stack-synthesis for stacked PRs; plus triage and synthesis. ai-toolkit adds two: + +- `workflow-security-reviewer` — expression injection into `run:` blocks, unpinned or dynamically-referenced actions, permission scope, missing Bullfrog steps, secret exposure through logs and artifacts, `fromJSON` coercion of `vars.*`, and breaking changes to the reusable-workflow contracts other repos depend on. +- `plugin-conventions-reviewer` — the mandatory `.claude-plugin/plugin.json` version bump and its increment, manifest arrays drifting from the directories on disk, skills registered as commands, and skill/agent naming conventions. + +`triage.guidance` in `.claude/review.yml` tells triage never to staff `contract-security-reviewer` or `defi-risk-reviewer` — this repo has no Solidity. + +**Gotcha — the Analyze timeout is step-level on purpose.** The `review` job has `timeout-minutes: 25` as a hard ceiling, but a job-level timeout **cancels** the job, and a cancelled run matches neither `success()` nor `failure()` (only `always()` and `cancelled()`). An overrun would therefore skip `Post` entirely, leaving the `--pre` sticky stuck on "⏳ Review running" while the `always()` reaction step flips to ❌. The `Analyze` step carries its own `timeout-minutes: 17` so an overrun becomes a _failure_, which `Post`'s gate does match. The job ceiling is 25 rather than 20 so the step ceiling always fires first: a step timeout is measured from step start and the job timeout from job start, so the tail budget is `25 - setup - 17`, not a flat 3 minutes, and setup is unbounded (Bullfrog, a `fetch-depth: 0` checkout, a GitHub Packages install, a `curl | bash`). Measured setup on a real run was 35s. + +**The same cancellation distinction applies to the reaction and reply steps.** Both run under `always()`, which is the one gate that fires on cancellation, and `job.status` is `cancelled` there. Rendering that as ❌ / "Review failed" would tell a requester to retry a review their successor is about to finish, since concurrency cancels a comment-triggered run whenever a push supersedes it. The reaction step therefore carries `!cancelled()` and the reply step branches on `cancelled()` to post "Superseded" instead. + +Do **not** "fix" this by widening `Post` to `always()`. Cancellation is also how the `concurrency` group stops a superseded run, and an `always()` Post would let that dying run overwrite the sticky its successor is mid-way through writing. + +**Why `install_review_cli` needs an isolated directory:** the repo's `bunfig.toml` pins the whole `@uniswap` scope to `registry.npmjs.org`, but `@uniswap/review-cli` is private on GitHub Packages, and bun only supports per-_scope_ registry overrides. The same file also enforces a 3-day `minimumReleaseAge` as a supply-chain control. That age gate applies to exact version requests too, so a review-cli version published less than 3 days ago is uninstallable until it ages in (bun errors rather than downgrading; an exact pin bypasses the stability-check fallback). Installing from a scratch dir with its own `bunfig.toml` sidesteps both without touching the repo's copy. + +**Behavior preserved from the previous implementation:** + +- Dependency PRs are **reviewed, not skipped** (review-cli's default skip policy would skip `dependabot/`, `renovate/`, and every `*[bot]` author). `auto-merge-dependabot` gates on the review result, so skipping them would leave security bumps unmerged. Hence `skip.authors: []` and the omitted dependency branch prefixes in `.claude/review.yml`. +- Draft PRs authored by `claude[bot]` 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 `triage` job's `if:` instead. +- Title-based automation detection (`chore(release):`, `chore(sync):`) is retained through `check-automated`, because review-cli's skip policy matches branches and authors but has no notion of PR titles. +- Fork PRs are never reviewed. The `review` job checks out PR head code and runs an agent with Bash access; review-cli's triage has no fork concept, and the `issue_comment` / `workflow_dispatch` payloads carry no head-repo field, so the `triage` job resolves it via the API. +- `workflow_dispatch` still accepts `pr_number` and `force_review`. `force_review` maps to `--force` (skip rebase detection), **not** `--fresh` — `--fresh` would also discard prior findings and thread decisions, losing the iterative review context. + +**Triggering a review without pushing code:** comment `@request-claude-review` on the PR (works on both regular and inline review comments), or dispatch manually: + +```bash +gh workflow run "Claude Code Review" -f pr_number=123 +``` + +Comment triggers are restricted to `OWNER`, `MEMBER`, and `COLLABORATOR` associations and ignore bot authors. This is enforced both by the job-level `if:` (to avoid paying for runner startup) and authoritatively inside `review-cli triage`. + +**Debugging a run:** every run uploads `review-cli-run--` containing the full run JSON (events, findings, verdict), retained 30 days. `agent-tokens` carries per-agent cost records for the agent-scorecard aggregator and must keep that exact artifact name. Locally, `review-cli last`, `review-cli artifact `, and `review-cli 123 --repo Uniswap/ai-toolkit --explain` are the fastest ways to inspect behavior; `--explain` prints the resolved agent plan and exits without calling any model. + ### PR Documentation Validator (`_claude-docs-check.yml`) This workflow validates that PR documentation is properly updated based on code changes. It checks CLAUDE.md files, README files, and plugin version bumps. @@ -1157,7 +1264,7 @@ These workflows are prefixed with two `__` and are only used within this reposit - `ci-check-pr-title.yml` - PR title format validation - `claude-auto-tasks.yml` - Autonomous task processing from Linear (scheduled) - `claude-code.yml` - Enables @claude mentions -- `claude-code-review.yml` - Automated code reviews +- `claude-code-review.yml` - Automated code reviews via `@uniswap/review-cli` - `claude-welcome.yml` - New PR welcomes - `dev-ai-newsletter.yml` - Weekly Dev AI Pod newsletter generation (scheduled) - `generate-pr-title-description.yml` - Auto-generated PR titles and descriptions @@ -1283,10 +1390,12 @@ Workflows may define workflow-level environment variables for centralized config ```yaml env: - MAX_DIFF_LINES: 5000 + HAS_DATADOG_API_KEY: ${{ secrets.DATADOG_API_KEY != '' }} ``` -This sets the default maximum diff line count before skipping Claude reviews (PR too large). The value is passed to all review jobs and can be overridden per job if needed. +`secrets.*` is not a valid context in a step-level `if:`, and step-level `env:` is not applied before `if:` is evaluated. Job-level `env:` **is**, so this exposes "is Datadog configured?" to the optional CI Visibility step, which is skipped entirely when the secret is absent. + +> **Note:** the `MAX_DIFF_LINES` repository variable is no longer read by `claude-code-review.yml`. Since that workflow moved to `@uniswap/review-cli`, diff-size policy comes from `.claude/review.yml` (`trivial_threshold`, set to `0` so every PR is reviewed) instead of a line-count cutoff. The variable is still consumed by external callers of the reusable `_claude-code-review.yml`, which accepts a `max_diff_lines` input, so **do not delete it**. ## Conventions diff --git a/.github/workflows/README.md b/.github/workflows/README.md index cd2a9120..5ca1edb2 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -95,15 +95,18 @@ Workflows designed to be called by other workflows using `workflow_call`. These - **claude-code-review.yml**: + - Powered by [`@uniswap/review-cli`](https://github.com/Uniswap/internal-tools/tree/main/packages/review-cli), not by this repo's `_claude-code-review.yml`. That reusable workflow still exists and is still supported for external consumers; ai-toolkit simply no longer calls it for its own PRs. - Formal GitHub reviews (APPROVE/REQUEST_CHANGES/COMMENT) for merge protection - - Inline comments on specific lines of code - - Auto-resolution of comments when issues are fixed - - Patch-ID based caching to skip rebases (no duplicate reviews) - - Iterative reviews track previous comments - - Custom prompts with automatic verdict injection - - Merge queue filtering (skips gh-readonly-queue branches) - - Supports model selection (Sonnet/Opus via labels) - - Fixed security settings (Bullfrog scanning, immutable permissions) + - Inline review threads, one per finding, deduplicated by `file:line:category` fingerprint + - Auto-resolution of threads when a finding is fixed, and never auto-resolves a thread a human has replied to + - Idempotent across force-pushes via a three-level change check (tree SHA → patch ID → hunk digest), so a pure rebase costs nothing + - Sticky summary comment carrying review history between runs + - Parallel specialist reviewers selected per-PR by a triage agent, including two ai-toolkit-specific reviewers (`workflow-security-reviewer`, `plugin-conventions-reviewer`) + - Comment trigger (`@request-claude-review`) restricted to OWNER/MEMBER/COLLABORATOR, with 👀 → ✅/❌ reaction lifecycle + - Automated-PR filtering via the shared `check-automated-pr` action; dependency PRs are reviewed on purpose so auto-merge can gate on the result + - Fork PRs are never reviewed (the review job checks out PR head code) + - Configured by `.claude/review.yml` and `.claude/agents/*-reviewer.md` + - Fixed security settings (Bullfrog scanning, per-job least-privilege permissions) - **generate-changelog.yml**: diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index b7422302..6c43e350 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -1,76 +1,106 @@ name: Claude Code Review -# Automated PR code reviews using Claude AI +# Automated PR code reviews for this repository, powered by +# @uniswap/review-cli from Uniswap/internal-tools. # -# This workflow provides: -# - Formal GitHub reviews (APPROVE/REQUEST_CHANGES/COMMENT) -# - Inline comments on specific lines of code -# - Auto-resolution of fixed issues on subsequent reviews -# - Patch-ID based caching to skip rebases (no duplicate reviews) -# - Iterative review tracking of previous comments +# This repo also PUBLISHES a reusable review workflow +# (`_claude-code-review.yml`) that other Uniswap repos consume. That +# workflow is a separate product and is unaffected by this file — this +# file is only how ai-toolkit reviews its own PRs. See +# .github/workflows/CLAUDE.md for the distinction. # -# TRIGGERS: -# - Automatic: On PR open, synchronize (push), or ready_for_review -# - Comment: Add a comment containing "@request-claude-review" to trigger a fresh review -# - Manual: Via workflow_dispatch with PR number and optional force_review flag +# WHAT REVIEW-CLI PROVIDES +# - Formal GitHub reviews (APPROVE / REQUEST_CHANGES / COMMENT) +# - Inline review threads, one per finding, deduplicated by +# file:line:category fingerprint +# - Auto-resolution of threads whose finding is fixed, and a hard rule +# that a thread with a human reply is never auto-resolved +# - Idempotency across force-pushes via a three-level change check +# (tree SHA -> patch ID -> hunk digest), so a pure rebase costs nothing +# - A sticky summary comment carrying review history between runs +# - Parallel specialist reviewers chosen per-PR by a triage agent # -# TRIGGERING A NEW REVIEW WITHOUT CODE CHANGES: -# The easiest way is to add a comment on the PR containing "@request-claude-review". -# This bypasses the cache and forces a fresh review. +# TRIGGERS +# - Automatic: PR opened, synchronize (push), reopened, ready_for_review +# - Comment: any PR comment or inline review comment containing +# "@request-claude-review" +# - Manual: workflow_dispatch with a PR number # -# Alternatively, use the manual workflow_dispatch trigger via the Actions tab or CLI: -# gh workflow run "Claude Code Review" -f pr_number=123 +# NOTE: re-requesting a review from github-actions[bot] in the GitHub UI +# does NOT work — GitHub fires no event when a review is re-requested +# from a bot account. Use the comment trigger or manual dispatch. # -# NOTE: Re-requesting a review from github-actions[bot] via the GitHub UI -# does NOT work. GitHub does not fire events when re-requesting reviews -# from bot accounts. Use the comment trigger or manual dispatch instead. -# -# The review uses the default prompt from this repository's -# .github/prompts/default-pr-review.md +# CONFIGURATION +# - Policy, model, budgets, and reviewer staffing: .claude/review.yml +# - Repo-specific reviewers: .claude/agents/*-reviewer.md +# - CLI version: vars.REVIEW_CLI_VERSION (falls back to the pin below) on: pull_request: types: - opened - synchronize + - reopened - ready_for_review - # Comment trigger: Add "@request-claude-review" to any PR comment to force a fresh review - # Works with both regular PR comments and inline review comments issue_comment: types: [created] - # Pull request review comments (inline comments on diffs) pull_request_review_comment: types: [created] - # Manual trigger for forcing a new review without code changes workflow_dispatch: inputs: pr_number: - description: "Pull request number to review" + description: 'Pull request number to review' required: true type: string force_review: - description: "Force a full review even if the code hasn't changed (bypasses cache)" + description: "Re-review even if the code hasn't changed (skips rebase detection)" required: false type: boolean default: true -# Ensure only one review runs at a time per PR -# New reviews wait for the current one to complete before starting concurrency: - group: claude-review-${{ github.event.pull_request.number || github.event.issue.number || inputs.pr_number }} - cancel-in-progress: false + # A re-push SHOULD supersede an in-flight review — there is no value in + # reviewing superseded code — so `pull_request` events cancel. + # + # Comment events that do NOT carry the trigger phrase get a per-run + # unique group suffix instead. Without it, any unrelated PR comment + # (Socket Security alerts, dependency notices, general discussion) + # creates a sibling run that grabs the `claude-review-` group, + # cancels the in-flight review, and then immediately skips itself at + # the job `if:`. Concurrency is evaluated at run-creation time, BEFORE + # job-level `if:`, so the skip does not save us. + group: >- + claude-review-${{ github.event.pull_request.number || github.event.issue.number || inputs.pr_number }}${{ + (github.event_name == 'issue_comment' || github.event_name == 'pull_request_review_comment') + && !contains(github.event.comment.body, '@request-claude-review') + && format('-skip-{0}', github.run_id) + || '' + }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + +# Deny by default. Every job below declares exactly the scopes it needs, +# so the empty workflow-level block means a newly added job starts with no +# token permissions until its author grants them deliberately. +permissions: {} jobs: - # Pre-check job for automatic reviews to detect automated PRs - # Only runs for pull_request events (not manual/comment triggers) + # Classify automated PRs. review-cli's own skip policy covers branch + # prefixes and authors (see .claude/review.yml), but it has no notion of + # PR *titles* — and this repo's release and sync automation is + # identified by title (`chore(release):`, `chore(sync):`). This job + # keeps that classification, reusing the same composite action that + # ci-pr-checks.yml and ci-check-pr-title.yml use. + # + # Only meaningful for `pull_request`: comment and manual triggers are + # explicit human requests and deliberately bypass automation filtering. check-automated: if: github.event_name == 'pull_request' runs-on: ubuntu-latest permissions: - contents: read + contents: read # sparse-checkout the local composite action outputs: is_automated: ${{ steps.check.outputs.is_automated }} category: ${{ steps.check.outputs.category }} @@ -92,163 +122,758 @@ jobs: branch_name: ${{ github.head_ref }} pr_title: ${{ github.event.pull_request.title }} - # Fetch PR details for manual triggers (workflow_dispatch) and comment triggers - # This job retrieves base_ref which isn't available in these event contexts - get-pr-info: - if: github.event_name == 'workflow_dispatch' || github.event_name == 'issue_comment' || github.event_name == 'pull_request_review_comment' + # Cheap gate: no PR checkout, no LLM call. `review-cli triage` reads + # GITHUB_EVENT_NAME + GITHUB_EVENT_PATH itself and writes a structured + # decision to GITHUB_OUTPUT, so no PR-author-controlled field is ever + # interpolated into a shell. It applies the draft skip, the author + # association threshold, the bot self-mention guard, the trigger-phrase + # parse, and workflow_dispatch pr_number extraction. + triage: + name: Triage + needs: [check-automated] + # Coarse event gate: a cheap early exit so unrelated comments and + # untrusted commenters never pay for runner startup. `review-cli + # triage` below is the authoritative policy check (it enforces the + # same author-association threshold itself); this only avoids booting + # a runner for events that obviously don't apply. + # + # `always()` is required because check-automated is skipped entirely + # for comment and manual triggers; without it, a skipped dependency + # would skip this job too. + # + # Dependency PRs (category `deps`) are deliberately reviewed rather + # than skipped: auto-merge-dependabot below gates on the review + # result, so skipping them would leave security bumps unmerged. + # + # The draft policy lives here rather than in .claude/review.yml + # (`skip.drafts: false`) because review-cli's draft skip is a single + # boolean and cannot express the claude[bot] carve-out: the + # autonomous-task workflow opens DRAFT PRs and those must still be + # reviewed on open, not only once a human marks them ready. + if: | + always() && ( + ( + github.event_name == 'pull_request' && + needs.check-automated.result == 'success' && + ( + needs.check-automated.outputs.is_automated != 'true' || + needs.check-automated.outputs.category == 'deps' + ) && + ( + github.event.pull_request.draft == false || + github.event.action == 'ready_for_review' || + github.event.pull_request.user.login == 'claude[bot]' + ) + ) || + ( + github.event_name == 'issue_comment' && + github.event.issue.pull_request != null && + contains(github.event.comment.body, '@request-claude-review') && + github.event.comment.user.type != 'Bot' && + contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association) + ) || + ( + github.event_name == 'pull_request_review_comment' && + contains(github.event.comment.body, '@request-claude-review') && + github.event.comment.user.type != 'Bot' && + contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association) + ) || + github.event_name == 'workflow_dispatch' + ) runs-on: ubuntu-latest permissions: - contents: read - pull-requests: read + contents: read # checkout the local composite action + packages: read # pull @uniswap/review-cli from GitHub Packages + issues: write # 👀 reaction on issue_comment + pull-requests: write # 👀 reaction on pull_request_review_comment outputs: - base_ref: ${{ steps.pr-info.outputs.base_ref }} - head_ref: ${{ steps.pr-info.outputs.head_ref }} - pr_number: ${{ steps.pr-info.outputs.pr_number }} - should_run: ${{ steps.pr-info.outputs.should_run }} - is_fork: ${{ steps.pr-info.outputs.is_fork }} + run: ${{ steps.gate.outputs.run }} + pr_number: ${{ steps.gate.outputs.pr_number }} + comment_id: ${{ steps.gate.outputs.comment_id }} + trigger_source: ${{ steps.gate.outputs.trigger_source }} + note: ${{ steps.gate.outputs.note }} + commenter: ${{ steps.gate.outputs.commenter }} + thread_anchor: ${{ steps.gate.outputs.thread_anchor }} + is_fork: ${{ steps.fork_check.outputs.is_fork }} + # Set only when a running-state reply was posted in this job. The + # review job PATCHes this reply with the terminal verdict. Empty for + # push-driven runs, which have no comment to reply to. + reply_id: ${{ steps.post_reply.outputs.id }} steps: - uses: bullfrogsec/bullfrog@1831f79cce8ad602eef14d2163873f27081ebfb3 # v0.8.4 - - name: Get PR Info - id: pr-info + # Sparse checkout of just the composite action and `.claude/`. The + # gate needs BOTH: the action to install the CLI, and + # `.claude/review.yml` for the skip policy. + # + # Do NOT drop `.claude` and add `--skip-config` to the gate below. + # `--skip-config` passes NO policy at all, which is not the same as + # "the CLI's defaults": `skip.drafts` falls back to `true` + # (skipping the claude[bot] draft PRs this repo must review) while + # branch and author skips are simply not applied. Reading the real + # config is what makes the policy in .claude/review.yml effective. + # + # Deliberately NOT pinned to the default branch, unlike the review + # job's tooling checkout. With no `ref:` this resolves to GITHUB_SHA + # (the merge commit on `pull_request`, the default branch on + # issue_comment / workflow_dispatch), so a PR can influence the skip + # policy the gate reads. That is proportionate here: this job holds + # no CLAUDE_CODE_OAUTH_TOKEN and runs no agent, so the worst case is + # a PR changing its own review eligibility, not code execution with a + # credential. The fork guard below deliberately does not rely on + # config — it resolves the head repo through the API instead. + - name: Checkout repository (config + composite action) + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false + sparse-checkout: | + .github/actions + .claude + sparse-checkout-cone-mode: false + + - name: Install review-cli + id: install-review-cli + uses: ./.github/actions/install_review_cli + with: + # Pinned, never `@latest`, so an upstream release cannot change + # review behavior mid-PR. Override without a commit by setting + # the REVIEW_CLI_VERSION repo variable (Settings → Secrets and + # variables → Actions → Variables). 1.10.x is required for the + # `model.synthesis` key in .claude/review.yml. + version: ${{ vars.REVIEW_CLI_VERSION || '1.10.1' }} + github-token: ${{ secrets.GITHUB_TOKEN }} + + # `loadConfig` treats a missing or invalid .claude/review.yml as + # "use defaults" and says nothing. Those defaults skip dependabot, + # every *[bot] author, and drafts — so a botched sparse checkout + # would silently stop reviewing dependency PRs (breaking + # auto-merge) with a green run and no signal. Fail loudly instead. + - name: Verify review config is present + run: | + set -euo pipefail + if [ ! -s .claude/review.yml ]; then + echo "::error::.claude/review.yml missing from the sparse checkout. The triage gate would silently fall back to defaults that skip dependency PRs and drafts." + exit 1 + fi + # `-s` catches missing and empty but NOT malformed, and `loadConfig` + # treats invalid YAML identically to a missing file: defaults, no + # log line. Assert the parse and the value auto-merge depends on. + # Fail-closed: absent python3/PyYAML fails the step rather than + # waving the config through. + python3 - <<'PY' + import sys, yaml + with open('.claude/review.yml') as fh: + cfg = yaml.safe_load(fh) + if not isinstance(cfg, dict): + sys.exit('review.yml did not parse to a mapping') + if cfg.get('skip', {}).get('authors') != []: + sys.exit('skip.authors is not [] — dependency PRs would be skipped at the gate, breaking auto-merge-dependabot') + PY + + - name: Decide whether to run + id: gate env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - EVENT_NAME: ${{ github.event_name }} - INPUT_PR_NUMBER: ${{ inputs.pr_number }} - ISSUE_NUMBER: ${{ github.event.issue.number }} - PR_NUMBER_FROM_REVIEW: ${{ github.event.pull_request.number }} - COMMENT_BODY: ${{ github.event.comment.body }} - COMMENT_AUTHOR: ${{ github.event.comment.user.login }} - IS_PR: ${{ github.event.issue.pull_request && 'true' || 'false' }} - GITHUB_REPOSITORY: ${{ github.repository }} + REVIEW_CLI_BIN: ${{ steps.install-review-cli.outputs.bin-path }} + run: '"$REVIEW_CLI_BIN/review-cli" triage --from-github-actions' + + # A declining gate is otherwise a fully green, fully PR-invisible + # no-review: `review` is skipped and nothing annotates why. That is + # exactly the path a mis-scoped skip prefix or a defaulted config takes, + # so it is the one that must not look like a silent no-op. Mirrors what + # `review-skipped` does for the check-automated path. + - name: Note that the gate declined + if: steps.gate.outputs.run != 'true' + run: echo "::notice::AI review skipped by review-cli triage. See the \"Decide whether to run\" step log for the policy that matched, and .claude/review.yml for the skip block." + + # Fork guard. The review job checks out PR head code and runs an + # agent with Bash tool access, so a fork PR must never reach it. + # review-cli's triage has no fork concept, and the issue_comment / + # workflow_dispatch payloads carry no head-repo field, so this is + # resolved via the API rather than from the event. + - name: Verify PR head is not a fork + id: fork_check + if: steps.gate.outputs.run == 'true' + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GH_REPO: ${{ github.repository }} + PR_NUMBER: ${{ steps.gate.outputs.pr_number }} run: | - # Determine PR number based on event type - if [ "$EVENT_NAME" = "workflow_dispatch" ]; then - PR_NUMBER="$INPUT_PR_NUMBER" - SHOULD_RUN="true" - elif [ "$EVENT_NAME" = "pull_request_review_comment" ]; then - PR_NUMBER="$PR_NUMBER_FROM_REVIEW" - # Ignore comments from github-actions[bot] to prevent self-triggering - if [ "$COMMENT_AUTHOR" = "github-actions[bot]" ]; then - echo "ℹ️ Ignoring comment from github-actions[bot] to prevent self-triggering" - echo "should_run=false" >> $GITHUB_OUTPUT - exit 0 - fi - # Check if comment contains the trigger phrase - if echo "$COMMENT_BODY" | grep -qi "@request-claude-review"; then - SHOULD_RUN="true" - echo "🔍 Found @request-claude-review in review comment" - else - SHOULD_RUN="false" - echo "ℹ️ Comment does not contain @request-claude-review, skipping" - fi - elif [ "$EVENT_NAME" = "issue_comment" ]; then - # issue_comment fires for both issues and PRs - # Ignore comments from github-actions[bot] to prevent self-triggering - if [ "$COMMENT_AUTHOR" = "github-actions[bot]" ]; then - echo "ℹ️ Ignoring comment from github-actions[bot] to prevent self-triggering" - echo "should_run=false" >> $GITHUB_OUTPUT - exit 0 - fi - # Check if this is a PR (has pull_request field) - if [ "$IS_PR" != "true" ]; then - echo "ℹ️ Comment is on an issue, not a PR. Skipping." - echo "should_run=false" >> $GITHUB_OUTPUT - exit 0 - fi - PR_NUMBER="$ISSUE_NUMBER" - # Check if comment contains the trigger phrase - if echo "$COMMENT_BODY" | grep -qi "@request-claude-review"; then - SHOULD_RUN="true" - echo "🔍 Found @request-claude-review in PR comment" - else - SHOULD_RUN="false" - echo "ℹ️ Comment does not contain @request-claude-review, skipping" - fi + set -euo pipefail + HEAD_REPO=$(gh api "repos/$GH_REPO/pulls/$PR_NUMBER" --jq '.head.repo.full_name') + if [ "$HEAD_REPO" != "$GH_REPO" ]; then + echo "::notice::Skipping AI review for fork PR ($HEAD_REPO -> $GH_REPO)" + echo "is_fork=true" >> "$GITHUB_OUTPUT" + else + echo "is_fork=false" >> "$GITHUB_OUTPUT" fi - echo "pr_number=$PR_NUMBER" >> $GITHUB_OUTPUT - echo "should_run=$SHOULD_RUN" >> $GITHUB_OUTPUT - - if [ "$SHOULD_RUN" != "true" ]; then - echo "ℹ️ Skipping - trigger condition not met" - exit 0 + # Acknowledge a comment trigger with 👀. Only fires when the gate + # accepted the comment — bot self-mentions, untrusted commenters, + # and comments without the trigger phrase leave comment_id empty. + - name: Acknowledge comment trigger (👀) + if: | + steps.gate.outputs.run == 'true' && + steps.fork_check.outputs.is_fork != 'true' && + steps.gate.outputs.comment_id != '' + continue-on-error: true + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GH_REPO: ${{ github.repository }} + COMMENT_ID: ${{ steps.gate.outputs.comment_id }} + EVENT_NAME: ${{ github.event_name }} + REVIEW_CLI_BIN: ${{ steps.install-review-cli.outputs.bin-path }} + run: | + "$REVIEW_CLI_BIN/review-cli" react \ + --repo "$GH_REPO" \ + --comment-id "$COMMENT_ID" \ + --event "$EVENT_NAME" \ + --reaction ack + + # A reply under the trigger comment, carrying the run URL. The 👀 + # reaction is glanceable; this is the responsive signal, and it is + # edited in place with the terminal verdict when the review lands. + # Best-effort: a failed post costs only silence until the ✅/❌. + - name: Post running-state reply + id: post_reply + if: | + steps.gate.outputs.run == 'true' && + steps.fork_check.outputs.is_fork != 'true' && + steps.gate.outputs.comment_id != '' + continue-on-error: true + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GH_REPO: ${{ github.repository }} + PR_NUMBER: ${{ steps.gate.outputs.pr_number }} + COMMENT_ID: ${{ steps.gate.outputs.comment_id }} + EVENT_NAME: ${{ github.event_name }} + REPLY_BODY: | + ↻ **Reviewing now** · [view run ↗](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}) + + This comment will update when the review completes. Findings will appear in the sticky summary above. + REVIEW_CLI_BIN: ${{ steps.install-review-cli.outputs.bin-path }} + # `set -uo pipefail` without `-e` on purpose: continue-on-error should + # surface a failure as a warning, and an explicit non-zero exit is what + # produces the annotation. Without any of this a failed `reply` fell + # through to the trailing `if`, which returns 0 when the file is + # absent, so the step reported SUCCESS and the failure was invisible. + run: | + set -uo pipefail + OUT="$RUNNER_TEMP/reply-id.txt" + if ! "$REVIEW_CLI_BIN/review-cli" reply \ + --repo "$GH_REPO" \ + --pr "$PR_NUMBER" \ + --event "$EVENT_NAME" \ + --in-reply-to "$COMMENT_ID" \ + --body "$REPLY_BODY" \ + --out-id "$OUT"; then + echo "::warning::could not post the running-state reply; the 👀 reaction is the only acknowledgement this run will give" + exit 1 + fi + if [ -s "$OUT" ]; then + echo "id=$(cat "$OUT")" >> "$GITHUB_OUTPUT" + else + echo "::warning::posted a running-state reply but could not capture its id; it will never be updated with the terminal verdict" fi - echo "ℹ️ Fetching PR #$PR_NUMBER info..." - PR_DATA=$(gh api repos/${GITHUB_REPOSITORY}/pulls/$PR_NUMBER) - - BASE_REF=$(echo "$PR_DATA" | jq -r '.base.ref') - HEAD_REF=$(echo "$PR_DATA" | jq -r '.head.ref') + review: + name: AI review + needs: triage + if: needs.triage.outputs.run == 'true' && needs.triage.outputs.is_fork != 'true' + runs-on: ubuntu-latest + permissions: + # `contents: write` (not read) is required for the GraphQL + # resolveReviewThread mutation; `pull-requests: write` alone returns + # "Resource not accessible by integration". + # https://github.com/orgs/community/discussions/44650 + # review-cli never pushes to git (persist-credentials: false below). + contents: write # required by the resolveReviewThread mutation, per above + packages: read # pull @uniswap/review-cli + pull-requests: write # post review comments + resolve threads + issues: write # comment reactions + sticky comment + # Hard ceiling. agent_budget_usd in .claude/review.yml is the real cost + # control; this is only the safety net if the CLI hangs. Set to 25 rather + # than 20 so the step-level ceiling on Analyze (17) always fires FIRST: + # a step timeout is measured from step start and the job timeout from job + # start, so the tail budget is `25 - setup - 17`, and setup is not bounded + # anywhere (Bullfrog, a fetch-depth-0 checkout, a GitHub Packages install, + # and a curl|bash). Measured setup on a real run was 35s, but a slow + # Packages fetch must not be able to invert the two ceilings — see the + # Analyze step for why a job-level timeout firing first strands the sticky. + timeout-minutes: 25 + env: + # `secrets.*` is not a valid context in a step-level `if:`, and + # step-level `env:` is not applied before `if:` is evaluated — but + # job-level env is, so this exposes "is Datadog configured?" to the + # optional telemetry step below. + HAS_DATADOG_API_KEY: ${{ secrets.DATADOG_API_KEY != '' }} + steps: + - uses: bullfrogsec/bullfrog@1831f79cce8ad602eef14d2163873f27081ebfb3 # v0.8.4 - echo "base_ref=$BASE_REF" >> $GITHUB_OUTPUT - echo "head_ref=$HEAD_REF" >> $GITHUB_OUTPUT + # Pin the checkout to the PR head. issue_comment and + # workflow_dispatch events default GITHUB_REF to the default + # branch, so an unpinned checkout would review `next` instead of the + # PR and then skip on "all hunks identical". fetch-depth: 0 so the + # CLI can diff against the merge base. + # + # This is the CONTENT UNDER ANALYSIS and nothing more. Every piece of + # tooling that acts on it comes from the trusted checkout below. + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + fetch-depth: 0 + persist-credentials: false + ref: refs/pull/${{ needs.triage.outputs.pr_number }}/head + + # Reviewer tooling from the DEFAULT ref, never the PR head. Two + # separate reasons, both load-bearing: + # + # 1. Availability. `.github/actions/install_review_cli` does not + # exist on branches cut before it landed, so resolving it out of + # the PR head fails with "Can't find 'action.yml'" on every open + # PR until that PR rebases. + # 2. Trust. This job holds CLAUDE_CODE_OAUTH_TOKEN and a + # `contents: write` token, and runs an agent with Bash access + # whose prompts ARE `.claude/agents/*.md`. Sourcing the installer + # shell or the agent set from the head branch would let a PR + # author rewrite both. Note that `.github/actions/**` is NOT + # covered by the `workflow` token scope that guards + # `.github/workflows/**`, so that gate does not help here. + # + # MUST come after the PR-head checkout: `actions/checkout` runs + # `git clean -ffdx` on its target, so checking out the workspace root + # second would delete this directory. + # `ref:` is REQUIRED and must be explicit. Omitting it does not mean + # "the default branch" — checkout falls back to GITHUB_SHA, which on + # a `pull_request` event is the merge commit (base + head), so the + # head branch would still influence the contents and this checkout + # would not be trusted at all. + - name: Checkout review tooling (trusted ref) + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + ref: ${{ github.event.repository.default_branch }} + persist-credentials: false + path: .review-tooling + sparse-checkout: | + .github/actions + .claude + sparse-checkout-cone-mode: false - # Security: detect fork PRs to prevent untrusted code execution - HEAD_REPO=$(echo "$PR_DATA" | jq -r '.head.repo.full_name') - BASE_REPO=$(echo "$PR_DATA" | jq -r '.base.repo.full_name') - if [ "$HEAD_REPO" != "$BASE_REPO" ]; then - echo "is_fork=true" >> $GITHUB_OUTPUT - echo "⚠️ Fork PR detected: $HEAD_REPO → $BASE_REPO" - else - echo "is_fork=false" >> $GITHUB_OUTPUT + # Swap the head's `.claude` for the trusted copy. review-cli's + # `loadConfig(repoRoot)` takes only a repo root — there is no flag to + # read config from somewhere else — so the config and agent files + # have to be in place on disk before Analyze runs. + # + # Consequence, and it is intended: edits to review.yml or the agent + # files take effect only once merged, so a PR cannot review itself + # with a reviewer set it wrote. Iterate locally with `review-cli dev` + # instead of pushing a commit per change. + # + # Guarded because `loadConfig` treats a missing file as "use + # defaults" and says nothing. Those defaults skip dependency PRs and + # drafts and staff neither repo-specific reviewer, all on a green + # run — the same silent-degradation trap the triage gate guards. + - name: Use trusted review config + run: | + set -euo pipefail + + # `.review-tooling/` lives inside GITHUB_WORKSPACE and is untracked, + # and review-cli gates its full-checkout fast path on + # `git status --porcelain` being empty. Left visible to git, the tree + # reads dirty on EVERY run and the CLI silently falls back to + # extracting only the files the diff touched, with no .git — which + # strips the agents' whole-repo Read/Grep/Glob and history access. + # Both reviewers this repo adds depend on out-of-diff reads + # (plugin-conventions globs skills/ dirs, workflow-security greps + # sibling workflows), so the degradation is invisible and total. + echo '/.review-tooling/' >> .git/info/exclude + + # `/.claude/` too, for the third way this step dirties the tree. + # The trusted copy lands files that may not be TRACKED at the PR + # head — every `.claude/**` file added after that branch was cut, + # which on this migration's merge day means review.yml and both + # agents for every already-open PR. Those arrive as `??` entries, + # and `git diff --name-only` never lists untracked paths, so the + # assume-unchanged pass below structurally cannot reach them. + # Excluding the directory hides only untracked files; tracked + # modifications still show, so that pass is still needed. + echo '/.claude/' >> .git/info/exclude + + for f in review.yml \ + agents/workflow-security-reviewer.md \ + agents/plugin-conventions-reviewer.md; do + if [ ! -s ".review-tooling/.claude/$f" ]; then + echo "::error::.claude/$f missing from the trusted checkout. Analyze would run without it: wrong model, no investigation gate, or a reviewer named in triage.guidance that does not exist on disk." + exit 1 + fi + done + + # `-s` catches missing and empty but NOT malformed. `loadConfig` + # treats invalid YAML exactly like a missing file: returns defaults, + # logs nothing. Those defaults skip dependency PRs, every *[bot] + # author, and drafts, which breaks auto-merge-dependabot on a green + # run. Assert the parse AND the one value that gate depends on. + # Fail-closed by design: if python3/PyYAML is absent the step fails + # loudly rather than waving the config through. + python3 - <<'PY' + import sys, yaml + with open('.review-tooling/.claude/review.yml') as fh: + cfg = yaml.safe_load(fh) + if not isinstance(cfg, dict): + sys.exit('review.yml did not parse to a mapping') + if cfg.get('skip', {}).get('authors') != []: + sys.exit('skip.authors is not [] — dependency PRs would be skipped, breaking auto-merge-dependabot') + if not cfg.get('model', {}).get('default'): + sys.exit('model.default missing — every reviewer would fall back to the CLI default model') + PY + + # Known consequence of the swap, accepted: restoring the clean tree + # also re-enables review-cli's post-synthesis `verifyFindings` pass, + # which is gated on workspaceShape == 'working-tree' and was + # therefore dead in CI while the tree was always dirty. It resolves + # cited files from this workspace, where `.claude` is now the + # pre-PR copy — so on a PR that ADDS a `.claude/**` file, a finding + # against it is dropped as "file not readable at HEAD", and on one + # that lengthens a file, a finding past the trusted copy's EOF is + # dropped as "beyond file end". Both drops are logged, not silent, + # and only reviewer-tuning PRs can hit them. A carve-out needs an + # upstream change; do not "fix" it by skipping the swap, which + # would hand config and agent prompts back to the PR author. + rm -rf .claude + cp -R .review-tooling/.claude .claude + + # The trusted copy reverts any `.claude/**` the PR edited, leaving + # those TRACKED files dirty — which .git/info/exclude cannot cover + # and which would re-trigger the extraction fallback above. Hide + # exactly those paths from `git status` so PRs touching .claude/** + # still get whole-repo reads. + git diff --name-only -- .claude | xargs -r git update-index --assume-unchanged + if [ -n "$(git status --porcelain)" ]; then + echo "::warning::working tree is not clean; review-cli will extract diff-only files and agents will lose whole-repo reads" + git status --porcelain fi - echo "✅ PR #$PR_NUMBER: base=$BASE_REF, head=$HEAD_REF, fork=$( [ "$HEAD_REPO" != "$BASE_REPO" ] && echo 'yes' || echo 'no' )" + - name: Install review-cli + id: install-review-cli + uses: ./.review-tooling/.github/actions/install_review_cli + with: + version: ${{ vars.REVIEW_CLI_VERSION || '1.10.1' }} + github-token: ${{ secrets.GITHUB_TOKEN }} + + # Fail loudly rather than silently degrading: without a token the + # agent-query factory falls back to a mock and "reviews" nothing. + # Only CLAUDE_CODE_OAUTH_TOKEN is forwarded (subscription auth) — + # ANTHROPIC_API_KEY is deliberately never passed to this job. + - name: Verify Claude OAuth token present + env: + CLAUDE_CODE_OAUTH_TOKEN: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} + run: | + if [ -z "$CLAUDE_CODE_OAUTH_TOKEN" ]; then + echo "::error::Set CLAUDE_CODE_OAUTH_TOKEN as a repo secret to enable AI review." + exit 1 + fi - # Automatic review triggered by pull_request events (opened, synchronize, ready_for_review) - # Skips certain automated PRs but reviews dependency updates (for auto-merge support) - # Skip categories: release, sync, action-update, merge-queue - # Review categories: deps (dependabot/renovate), and all non-automated PRs - claude-review-auto: - needs: check-automated - if: | - github.event_name == 'pull_request' && - github.event.pull_request.head.repo.full_name == github.repository && - ( - needs.check-automated.outputs.is_automated != 'true' || - needs.check-automated.outputs.category == 'deps' - ) && - ( - github.event.pull_request.draft == false || - github.event.action == 'ready_for_review' || - github.event.pull_request.user.login == 'claude[bot]' - ) - permissions: - id-token: write - contents: write - pull-requests: write - issues: read - actions: read - uses: ./.github/workflows/_claude-code-review.yml - with: - pr_number: ${{ github.event.pull_request.number }} - base_ref: ${{ github.base_ref }} - force_review: false - max_diff_lines: ${{ fromJSON(vars.MAX_DIFF_LINES) }} - - # Use Opus 4.8 for comprehensive reviews with reliable inline comments - # Opus is more capable at following complex multi-step instructions - # Uncomment one of these to use a different model for reviews: - # model: 'claude-haiku-4-5' - # model: 'claude-sonnet-5' - model: "claude-opus-4-8" - - # Allow standard review conversation turns - # max_turns: 15 - - # Standard timeout for most PRs - timeout_minutes: 20 - - # Optional: Restrict tools for read-only reviews (uncomment to enable) - # allowed_tools: 'Read,Grep,Glob,Bash(git log),Bash(git diff),Bash(git show)' - - secrets: - ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} - CLAUDE_CODE_OAUTH_TOKEN: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} - WORKFLOW_PAT: ${{ secrets.WORKFLOW_PAT }} - - # Summary job for skipped automatic reviews + # Install the Claude Code native binary. + # + # As of @anthropic-ai/claude-agent-sdk@0.2.113 the SDK spawns a + # per-platform native binary shipped via optional deps. Bun on Linux + # installs metadata for every variant but extracts only the host + # one, while the SDK's resolver probes `linux--musl` first — so + # require.resolve returns a path that isn't on disk and the SDK fails + # with "Claude Code native binary not found". Installing via the + # official script and pointing the SDK at it with + # CLAUDE_CODE_EXECUTABLE_PATH sidesteps the optional-dep dance. + # + # SECURITY: this is the one un-pinned external fetch here. Hashing + # install.sh would not help — it is a per-host bootstrapper that + # selects a binary for the runner's libc/arch at install time, so a + # fixed hash on the script gives no integrity guarantee for the + # binary that lands on disk, and Anthropic rotates the bootstrapper + # independently of binary versions. Compensating controls: Bullfrog + # egress monitoring is active on this job, the forwarded credential + # is a subscription token scoped to Claude Code rather than metered + # API spend, and ANTHROPIC_API_KEY is never present. The asymmetry + # with the rest of this SHA-pinned workflow is deliberate. + - name: Install Claude Code binary + run: | + set -euo pipefail + curl -fsSL https://claude.ai/install.sh | bash + for candidate in "$HOME/.local/bin/claude" "$HOME/.claude/bin/claude" "$HOME/.npm-global/bin/claude"; do + if [ -x "$candidate" ]; then + CLAUDE_BIN="$candidate"; break + fi + done + if [ -z "${CLAUDE_BIN:-}" ]; then + CLAUDE_BIN="$(command -v claude || true)" + fi + if [ -z "$CLAUDE_BIN" ] || [ ! -x "$CLAUDE_BIN" ]; then + echo "::error::claude binary not found after install"; exit 1 + fi + echo "CLAUDE_CODE_EXECUTABLE_PATH=$CLAUDE_BIN" >> "$GITHUB_ENV" + dirname "$CLAUDE_BIN" >> "$GITHUB_PATH" + + # Upsert a "review running" sticky so the PR shows in-progress state + # from t=0. Best-effort: a GitHub API blip here must not skip + # Analyze. Without continue-on-error a non-zero exit would + # short-circuit every later step (they default to `if: success()`) + # and Post would have no last-run.json to recover from. + - name: Pre — running sticky placeholder + continue-on-error: true + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GH_REPO: ${{ github.repository }} + PR_NUMBER: ${{ needs.triage.outputs.pr_number }} + REVIEW_CLI_BIN: ${{ steps.install-review-cli.outputs.bin-path }} + run: '"$REVIEW_CLI_BIN/review-cli" post "$PR_NUMBER" --repo "$GH_REPO" --pre' + + # Analyze. Reads the diff, runs the agents, persists last-run.json. + # + # review-cli's own pipeline does not write to GitHub here: the analyze + # path is never handed a GitHub writer, which is why `post` below is the + # only verb that writes review STATE — findings, thread resolutions, the + # sticky. (The react/reply steps in this workflow also write to the PR, + # but only reactions and the trigger reply.) That is a property of the + # CLI, NOT of this step's environment — the job declares + # `contents: write`, so GITHUB_TOKEN here IS write-capable and the agent + # has Bash access. Do not cite this comment to justify relaxing the fork + # guard. + # + # It does READ the diff via `gh`, which needs GITHUB_TOKEN; without it + # the diff comes back empty and the run short-circuits on "PR has no + # changes". + # + # Trigger context arrives from the triage job's outputs through + # `env:` and is appended as argv entries, never interpolated into the + # script, so a malicious comment body cannot break out into the shell. + # + # `--force` is passed for explicit human requests (comment trigger, or + # manual dispatch with force_review). It does more than skip rebase + # detection: it also bypasses both diff size guards, re-staffs reviewers + # from scratch rather than reusing the prior agentSet, and — because no + # --budget/--max-turns is passed — substitutes the CLI's force defaults + # ($20 per agent, 200 turns) for `agent_budget_usd`/`agent_max_turns` in + # .claude/review.yml, which therefore bind on push-driven runs only. + # + # `--fresh` is deliberately NOT used: it would also discard prior + # findings and thread decisions, losing the iterative review context + # that makes re-reviews coherent. + - name: Analyze + # Step ceiling, deliberately BELOW the job's 25-minute + # `timeout-minutes`. A job-level timeout CANCELS the job, and + # `if: success() || failure()` on Post does not match a cancelled + # run — so an overrun here would skip Post entirely, leaving the + # `--pre` sticky stuck on "⏳ Review running" while the `always()` + # reaction step flips to ❌. Timing out at the step level makes the + # overrun a failure, which Post's gate does match. Do NOT "fix" + # that gate with `always()` instead: a concurrency-cancelled run + # would then overwrite the sticky its successor is mid-way through + # writing. + # + # This must fire before the job's 25-minute ceiling. Tail budget is + # `25 - setup - 17`, NOT a flat 3 minutes: a step timeout runs from + # step start, the job timeout from job start. Measured setup was 35s, + # leaving ~7 minutes for Post and the uploads (which took 13s). + timeout-minutes: 17 + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + CLAUDE_CODE_OAUTH_TOKEN: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} + REVIEW_NOTE: ${{ needs.triage.outputs.note }} + REVIEW_TRIGGER_SOURCE: ${{ needs.triage.outputs.trigger_source }} + REVIEW_COMMENTER: ${{ needs.triage.outputs.commenter }} + REVIEW_THREAD_ANCHOR: ${{ needs.triage.outputs.thread_anchor }} + # Pin where the CLI writes the run artifact so the Upload and + # Datadog steps can read it back. The CLI default is `./`, which + # would silently drop both consumers. + REVIEW_CLI_ARTIFACT_PATH: ${{ runner.temp }}/review-cli-output.json + # Per-agent cost records, consumed by the agent-scorecard + # aggregator. + REVIEW_CLI_TOKENS_ARTIFACT_PATH: ${{ runner.temp }}/agent-tokens.jsonl + # Datadog telemetry emitted by the CLI during the run. Required + # for npm-installed consumers: only the standalone binary from + # GitHub Releases has credentials baked at build time, so + # without this passthrough the client is a silent no-op. + DD_API_KEY: ${{ secrets.DATADOG_API_KEY }} + DD_SITE: ${{ vars.DATADOG_SITE || 'datadoghq.com' }} + GH_REPO: ${{ github.repository }} + PR_NUMBER: ${{ needs.triage.outputs.pr_number }} + FORCE_REVIEW: ${{ (github.event_name == 'workflow_dispatch' && inputs.force_review) || needs.triage.outputs.trigger_source == 'comment' || needs.triage.outputs.trigger_source == 'review_comment' }} + REVIEW_CLI_BIN: ${{ steps.install-review-cli.outputs.bin-path }} + run: | + set -uo pipefail + args=() + [ -n "$REVIEW_NOTE" ] && args+=(--note "$REVIEW_NOTE") + [ -n "$REVIEW_COMMENTER" ] && args+=(--commenter "$REVIEW_COMMENTER") + [ -n "$REVIEW_THREAD_ANCHOR" ] && args+=(--thread-anchor "$REVIEW_THREAD_ANCHOR") + [ "$FORCE_REVIEW" = "true" ] && args+=(--force) + "$REVIEW_CLI_BIN/review-cli" review "$PR_NUMBER" \ + --repo "$GH_REPO" \ + --trigger-source "$REVIEW_TRIGGER_SOURCE" \ + "${args[@]}" + + # Post — the only verb that writes. Reads last-run.json, posts new + # findings, resolves fixed threads, upserts the sticky. Runs even + # when Analyze failed so it can publish the most recent saved state. + - name: Post + # `bin-path` is empty whenever the install step never ran — which + # happens when an earlier step (the trusted-config guard) failed. + # Without this check, Post runs anyway and dies on + # `/review-cli: No such file or directory` (exit 127), replacing the + # real error with a meaningless one. Skip instead, so the failure + # the developer sees is the one that actually happened. + if: | + (success() || failure()) && + steps.install-review-cli.outputs.bin-path != '' + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GH_REPO: ${{ github.repository }} + PR_NUMBER: ${{ needs.triage.outputs.pr_number }} + REVIEW_CLI_BIN: ${{ steps.install-review-cli.outputs.bin-path }} + run: '"$REVIEW_CLI_BIN/review-cli" post "$PR_NUMBER" --repo "$GH_REPO"' + + - name: Upload run artifact + # Upload even on failure: the artifact holds whatever events were + # captured up to the failure, which is the most useful post-hoc + # debugging surface. + if: always() + uses: actions/upload-artifact@bbbca2ddaa5d8feaa63e36b76fdaad77386f024f # v7.0.0 + with: + name: review-cli-run-${{ needs.triage.outputs.pr_number }}-${{ github.run_attempt }} + path: ${{ runner.temp }}/review-cli-output.json + if-no-files-found: warn + retention-days: 30 + + # Name MUST be exactly `agent-tokens` — the agent-scorecard + # aggregator finds it across consumer repos with one + # `gh run download` call. + - name: Upload agent-tokens artifact + if: always() + uses: actions/upload-artifact@bbbca2ddaa5d8feaa63e36b76fdaad77386f024f # v7.0.0 + with: + name: agent-tokens + path: ${{ runner.temp }}/agent-tokens.jsonl + if-no-files-found: ignore + retention-days: 30 + overwrite: true + + # Optional: stamp the CI Visibility pipeline span with the review + # outcome so the GitHub Actions run is filterable by verdict in + # Datadog. Additive — the CLI already ships metrics over HTTPS. + # Skipped entirely when DATADOG_API_KEY is unset. + - name: Stamp DD CI Visibility with review outcome + if: always() && env.HAS_DATADOG_API_KEY == 'true' + env: + DATADOG_API_KEY: ${{ secrets.DATADOG_API_KEY }} + DATADOG_SITE: ${{ vars.DATADOG_SITE || 'datadoghq.com' }} + REVIEW_TEAM: ${{ vars.REVIEW_DD_TEAM || '' }} + ARTIFACT: ${{ runner.temp }}/review-cli-output.json + run: | + set -uo pipefail + if [ ! -s "$ARTIFACT" ]; then + echo "no review-cli artifact at $ARTIFACT — skipping DD CI tagging" + exit 0 + fi + VERDICT=$(jq -r '.review.verdict // "unknown"' "$ARTIFACT") + DEPTH=$(jq -r '.review.depth // "unknown"' "$ARTIFACT") + FINDINGS=$(jq -r '(.review.findings // []) | length' "$ARTIFACT") + # `review.clean`, not `review.skipped`: a skipped run writes no + # artifact at all, so reaching this step means a real review + # completed. APPROVE with zero findings is clean, not skipped — + # and trivial_threshold: 0 in .claude/review.yml disables the + # size-skip path outright. + CLEAN=$([ "$FINDINGS" = 0 ] && [ "$VERDICT" = "APPROVE" ] && echo true || echo false) + TAGS=( + --tags "service:review-cli" + --tags "review.verdict:$VERDICT" + --tags "review.depth:$DEPTH" + --tags "review.clean:$CLEAN" + ) + if [ -n "$REVIEW_TEAM" ]; then + TAGS+=(--tags "team:$REVIEW_TEAM") + fi + bunx @datadog/datadog-ci@5.15.0 tag --level pipeline "${TAGS[@]}" \ + || echo "datadog-ci tag failed (non-fatal)" + bunx @datadog/datadog-ci@5.15.0 measure --level pipeline \ + --measures "review.findings:$FINDINGS" \ + || echo "datadog-ci measure failed (non-fatal)" + + # Swap the 👀 ack for ✅/❌. --clear-prior-ack removes the eyes + # first so the comment carries a single terminal signal. + # + # `!cancelled()` is load-bearing. `always()` is the one gate that runs on + # cancellation, and `job.status` is `cancelled` there — which the + # success/failure branch below would render as ❌. Concurrency cancels + # this run whenever a push supersedes a comment-triggered review, so + # without this the requester gets a ❌ on a review that a successor is + # about to complete. + # + # Note what skipping does NOT do: no successor replaces the 👀. Only a + # `pull_request` run can cancel this one (`cancel-in-progress` is scoped + # to that event), and such a run has an empty `comment_id`, so it never + # reaches this step. The ack stays until someone clears it. That is the + # deliberate trade — a stale 👀 is better than a false ❌, and the reply + # step below carries the real "Superseded" signal. + - name: Update reaction (✅ / ❌) on comment trigger + if: | + always() && !cancelled() && + needs.triage.outputs.comment_id != '' && + steps.install-review-cli.outputs.bin-path != '' + continue-on-error: true + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + OUTCOME: ${{ job.status }} + GH_REPO: ${{ github.repository }} + COMMENT_ID: ${{ needs.triage.outputs.comment_id }} + EVENT_NAME: ${{ github.event_name }} + REVIEW_CLI_BIN: ${{ steps.install-review-cli.outputs.bin-path }} + run: | + REACTION=$([ "$OUTCOME" = "success" ] && echo "success" || echo "failure") + "$REVIEW_CLI_BIN/review-cli" react \ + --repo "$GH_REPO" \ + --comment-id "$COMMENT_ID" \ + --event "$EVENT_NAME" \ + --reaction "$REACTION" \ + --clear-prior-ack + + # Edit the running-state reply with the terminal verdict so the + # requester gets feedback without scrolling back to the sticky. + - name: Update threaded reply with terminal state + if: | + always() && + needs.triage.outputs.reply_id != '' && + steps.install-review-cli.outputs.bin-path != '' + continue-on-error: true + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + # `job.status` is one of success / failure / cancelled. The third + # value is why this is not a two-way branch: the step runs under + # `always()`, which fires on cancellation too, and concurrency + # cancels a comment-triggered run whenever a push supersedes it. + # Reporting that as "Review failed" would tell the requester to + # retry a review their successor is about to finish. + # + # `cancelled()` is deliberately NOT used here — it is only valid in + # a job or step `if:`, not in `env:` (actionlint catches this). + OUTCOME: ${{ job.status }} + GH_REPO: ${{ github.repository }} + REPLY_ID: ${{ needs.triage.outputs.reply_id }} + EVENT_NAME: ${{ github.event_name }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + REVIEW_CLI_BIN: ${{ steps.install-review-cli.outputs.bin-path }} + run: | + if [ "$OUTCOME" = "cancelled" ]; then + BODY="↻ **Superseded** · a newer push took over this review · [view run ↗](${RUN_URL})" + elif [ "$OUTCOME" = "success" ]; then + BODY="✅ **Reviewed** · [view run ↗](${RUN_URL}) · scroll up for the full summary." + else + BODY="⚠ **Review failed** · [view run ↗](${RUN_URL}) · scroll up for the partial state, or comment \`@request-claude-review\` to retry." + fi + "$REVIEW_CLI_BIN/review-cli" reply \ + --repo "$GH_REPO" \ + --event "$EVENT_NAME" \ + --edit-reply "$REPLY_ID" \ + --body "$BODY" + + # Report automated PRs that were filtered out, so the run explains + # itself rather than looking like a silent no-op. review-skipped: needs: check-automated if: | @@ -256,8 +881,10 @@ jobs: needs.check-automated.outputs.is_automated == 'true' && needs.check-automated.outputs.category != 'deps' runs-on: ubuntu-latest - permissions: - contents: read + # No checkout, no API calls — this job runs Bullfrog and two echoes, and + # fetching a SHA-pinned external action needs no token scope. `{}` is the + # real floor, matching the workflow-level default above. + permissions: {} steps: - uses: bullfrogsec/bullfrog@1831f79cce8ad602eef14d2163873f27081ebfb3 # v0.8.4 @@ -268,85 +895,19 @@ jobs: echo "::notice::Code review skipped for automated PR: $SKIP_REASON" echo "✅ Automated PR detected - code review is not required for this PR type." - # Manual review triggered by workflow_dispatch - # This runs after get-pr-info to have access to PR metadata - # Note: Manual triggers bypass automated PR checks - if you explicitly request a review, you get one - claude-review-manual: - needs: get-pr-info - if: | - github.event_name == 'workflow_dispatch' && - needs.get-pr-info.outputs.should_run == 'true' && - needs.get-pr-info.outputs.is_fork != 'true' - permissions: - id-token: write - contents: write - pull-requests: write - issues: read - actions: read - uses: ./.github/workflows/_claude-code-review.yml - with: - pr_number: ${{ needs.get-pr-info.outputs.pr_number }} - base_ref: ${{ needs.get-pr-info.outputs.base_ref }} - force_review: ${{ inputs.force_review }} - max_diff_lines: ${{ fromJSON(vars.MAX_DIFF_LINES) }} - - # Use Opus 4.8 for comprehensive reviews with reliable inline comments - model: "claude-opus-4-8" - - # Standard timeout for most PRs - timeout_minutes: 20 - - secrets: - ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} - CLAUDE_CODE_OAUTH_TOKEN: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} - WORKFLOW_PAT: ${{ secrets.WORKFLOW_PAT }} - - # Comment-triggered review: Add "@request-claude-review" to any PR comment - # This forces a fresh review, bypassing the cache - # Note: Comment triggers bypass automated PR checks - if you explicitly request a review, you get one - claude-review-comment: - needs: get-pr-info - if: | - (github.event_name == 'issue_comment' || github.event_name == 'pull_request_review_comment') && - needs.get-pr-info.outputs.should_run == 'true' && - needs.get-pr-info.outputs.is_fork != 'true' - permissions: - id-token: write - contents: write - pull-requests: write - issues: read - actions: read - uses: ./.github/workflows/_claude-code-review.yml - with: - pr_number: ${{ needs.get-pr-info.outputs.pr_number }} - base_ref: ${{ needs.get-pr-info.outputs.base_ref }} - force_review: true # Always force when triggered by comment - max_diff_lines: ${{ fromJSON(vars.MAX_DIFF_LINES) }} - - # Use Opus 4.8 for comprehensive reviews with reliable inline comments - model: "claude-opus-4-8" - - # Standard timeout for most PRs - timeout_minutes: 20 - - secrets: - ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} - CLAUDE_CODE_OAUTH_TOKEN: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} - WORKFLOW_PAT: ${{ secrets.WORKFLOW_PAT }} - - # Auto-merge Dependabot security updates after successful review - # Only runs for pull_request events (dependabot only triggers pull_request) + # Auto-merge Dependabot security updates once the review succeeds. + # Depends on `review` rather than the retired reusable-workflow job. auto-merge-dependabot: - needs: claude-review-auto + needs: review if: | github.event_name == 'pull_request' && github.event.pull_request.user.login == 'dependabot[bot]' && (contains(github.event.pull_request.title, 'security') || contains(github.event.pull_request.title, 'Bump')) && - needs.claude-review-auto.result == 'success' + needs.review.result == 'success' runs-on: ubuntu-latest permissions: - contents: write - pull-requests: write + contents: write # enable auto-merge on the PR + pull-requests: write # `gh pr merge --auto` updates the PR's merge state steps: - uses: bullfrogsec/bullfrog@1831f79cce8ad602eef14d2163873f27081ebfb3 # v0.8.4 diff --git a/CLAUDE.md b/CLAUDE.md index fc5b1b83..41a506dc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -314,6 +314,31 @@ To differentiate skills from agents and avoid naming conflicts: Commands (standalone `.md` files in `./commands/`) follow the same verb-noun pattern as skills since they are also user-invocable actions. +## AI Code Review of This Repository + +PRs in this repository are reviewed by [`@uniswap/review-cli`](https://github.com/Uniswap/internal-tools/tree/main/packages/review-cli), the shared reviewer also used by `Uniswap/universe` and `Uniswap/backend`. It is driven by `.github/workflows/claude-code-review.yml`. + +| File | Purpose | +| ------------------------------------ | --------------------------------------------------------------------------------------- | +| `.claude/review.yml` | Review policy: model, per-agent budget, skip rules, investigation gate, triage staffing | +| `.claude/agents/*-reviewer.md` | Repo-specific reviewers, **added to** review-cli's bundled set | +| `.github/actions/install_review_cli` | Installs the pinned CLI from GitHub Packages | + +Two repo-specific reviewers encode this repository's house rules so they are enforced on every PR rather than living only in documentation: + +- `workflow-security-reviewer` — the GitHub Actions rules in [GitHub Actions Best Practices](#github-actions-best-practices) below (expression injection, SHA pinning, `fromJSON` coercion, Bullfrog steps, script separation), plus breaking changes to the reusable workflows other repos consume. +- `plugin-conventions-reviewer` — the [Mandatory Version Bumping](#mandatory-version-bumping) rule and the skill/agent naming conventions above. + +**When you change those rules, update the matching reviewer.** A convention that lives only in this file is advisory; one encoded in a reviewer is checked. Full detail in [`.github/workflows/CLAUDE.md`](.github/workflows/CLAUDE.md#pr-code-review-for-this-repository-claude-code-reviewyml). + +> **Not to be confused with** `_claude-code-review.yml`, the reusable review workflow this repo _publishes_ for other repositories. It is still supported and unchanged; ai-toolkit just no longer calls it for its own PRs. Its prompt sections live in `.github/prompts/pr-review/` and are unrelated to the reviewers above. + +To review a PR locally without posting anything: + +```bash +review-cli 123 --repo Uniswap/ai-toolkit +``` + ## Documentation Management ### CLAUDE.md File Management diff --git a/docs/guides/claude-integration.md b/docs/guides/claude-integration.md index 9df13ab2..b9a58fbe 100644 --- a/docs/guides/claude-integration.md +++ b/docs/guides/claude-integration.md @@ -300,7 +300,7 @@ The repository includes three Claude-powered GitHub Actions workflows: 2. **claude-code-review.yml** - Automated PR code reviews 3. **claude-welcome.yml** - Welcome messages for new PRs -All workflows use reusable workflow templates in `.github/workflows/_claude-*.yml`. +`claude-code.yml` and `claude-welcome.yml` call reusable workflow templates in `.github/workflows/_claude-*.yml`. `claude-code-review.yml` is the exception: it runs [`@uniswap/review-cli`](https://github.com/Uniswap/internal-tools/tree/main/packages/review-cli) directly. The `_claude-code-review.yml` reusable workflow is still published and supported for other repositories to call, but ai-toolkit no longer uses it for its own PRs. ### 1. Interactive Assistant (claude-code.yml) @@ -337,47 +337,62 @@ custom_instructions: | **Purpose**: Provide formal GitHub PR reviews with inline comments +**Implementation**: [`@uniswap/review-cli`](https://github.com/Uniswap/internal-tools/tree/main/packages/review-cli) from `Uniswap/internal-tools`, the same reviewer used by `Uniswap/universe` and `Uniswap/backend`. It runs a triage agent that picks specialist reviewers per PR, runs them in parallel, deduplicates their findings, and posts them as inline review threads. + **Triggers:** -- PR opened -- PR synchronized (new commits) -- PR marked ready for review +- PR opened, synchronized (new commits), reopened, or marked ready for review +- Any PR comment or inline review comment containing `@request-claude-review` (restricted to OWNER/MEMBER/COLLABORATOR) +- Manual `workflow_dispatch` with a PR number **Features:** - Formal reviews (APPROVE/REQUEST_CHANGES/COMMENT) -- Inline comments on specific code lines -- Auto-resolution of fixed issues -- Iterative review tracking -- Patch-ID based caching (no duplicate reviews after rebase) +- One inline thread per finding, deduplicated by `file:line:category` +- Auto-resolution when a finding is fixed; a thread with a human reply is never auto-resolved +- Idempotent across force-pushes (tree SHA → patch ID → hunk digest), so a pure rebase costs nothing +- Sticky summary comment carrying review history between runs +- An investigation gate: an empty-findings APPROVE is demoted to COMMENT unless the reviewers actually opened enough of the changed files (which is why `verdict.allowed` lists COMMENT; without it the demotion is computed and discarded) **Configuration:** -```yaml -model: 'claude-haiku-4-5' # Cost-effective for reviews -timeout_minutes: 20 -# Optional: restrict to read-only tools -allowed_tools: 'Read,Grep,Glob,Bash(git log),Bash(git diff)' -``` +Behavior is configured in the repository, not in workflow inputs: + +| File | Controls | +| --------------------------------- | ----------------------------------------------------------------------------- | +| `.claude/review.yml` | Model, per-agent budget, skip policy, investigation gate, triage staffing | +| `.claude/agents/*-reviewer.md` | Repo-specific reviewers, added to review-cli's bundled set | +| `vars.REVIEW_CLI_VERSION` | Pinned CLI version (falls back to the pin in the workflow) | +| `secrets.CLAUDE_CODE_OAUTH_TOKEN` | Required. `ANTHROPIC_API_KEY` is deliberately not forwarded to the review job | +| `secrets.DATADOG_API_KEY` | Optional. Enables Datadog CI Visibility stamping | + +ai-toolkit ships two repo-specific reviewers on top of the bundled ones: + +- `workflow-security-reviewer` — GitHub Actions expression injection, action pinning, permission scope, missing Bullfrog steps, and breaking changes to the reusable-workflow contracts other repos consume +- `plugin-conventions-reviewer` — the mandatory `.claude-plugin/plugin.json` version bump, manifest arrays drifting from directories on disk, and skill/agent naming conventions **Example review:** ``` Review Status: REQUEST_CHANGES -General Comments: -- Good implementation of the authentication system -- Consider adding error handling for edge cases -- Missing tests for the new endpoints +Summary (sticky comment, updated in place on every run): + Reviewed 3 files. Two findings, one carried over from the previous run. -Inline Comments: - src/auth/handler.ts:42 - ⚠️ This function should validate the token format before parsing +Inline threads (one per finding): + .github/workflows/publish-packages.yml:88 + [critical · workflow-security] `github.event.pull_request.title` is + interpolated into this `run:` block, so a PR title can execute + arbitrary shell with the job's token. Pass it via `env:` and + reference "$PR_TITLE" instead. - src/auth/handler.ts:67 - 💡 Consider extracting this logic into a separate utility function + packages/plugins/development-planning/.claude-plugin/plugin.json:3 + [warning · plugin-conventions] A skill was added under skills/ but + version is still 2.0.7. A new skill is a minor bump: 2.1.0. ``` +Findings the developer fixes are resolved automatically on the next run. A thread that a human has replied to is never auto-resolved. Finding nothing is a valid outcome and produces an APPROVE with no inline threads. + ### 3. Welcome Message (claude-welcome.yml) **Purpose**: Post friendly welcome messages on new PRs