fix(ci): address review-fix-loop findings on the review-cli migration

Independent review (code-reviewer + comment-analyzer + silent-failure-hunter)
standing in for the CI reviewer, which cannot review its own bootstrap PR.
15 findings, all verified against review-cli source and real run artifacts.

Critical — the trusted checkout silently degraded every review. review-cli
gates its full-checkout fast path on `git status --porcelain` being empty, and
`.review-tooling/` is untracked inside GITHUB_WORKSPACE, so the tree read
dirty on every run and the CLI fell back to extracting only diff-touched
files with no .git. Agents kept working and runs stayed green while losing
whole-repo Read/Grep/Glob and history — worst for the two reviewers added
here, which are told to look outside the diff. Fixed with .git/info/exclude
plus `update-index --assume-unchanged` on the .claude paths the trusted copy
reverts (exclude cannot hide tracked files), and the step now asserts the
tree is clean and warns if not. Verified in a scratch repo.

Critical — the investigation gate was inert. Demotion is
`wantsDemotion && allowedVerdicts.includes('COMMENT')`, and the default
`verdict.allowed` is [APPROVE, REQUEST_CHANGES], so the gate computed the
demotion, discarded it, and let the rubber-stamp APPROVE stand. Added
`verdict.allowed: [APPROVE, REQUEST_CHANGES, COMMENT]`. Safe for dependency
auto-merge, which gates on `needs.review.result == 'success'`, not verdict.

High — cancellation was rendered as failure. The reaction and reply steps run
under `always()`, which fires on cancellation, and concurrency cancels a
comment-triggered run whenever a push supersedes it. Both reported
"Review failed" and told the requester to retry a review its successor was
about to finish. Reaction now carries `!cancelled()`; reply posts
"Superseded". This is the same reasoning already documented for Post, applied
to its two siblings.

High — the config guards tested presence, not validity. `-s` catches missing
and empty but not malformed, and loadConfig treats invalid YAML exactly like
a missing file: defaults, no log line. Both guards now assert the parse and
`skip.authors == []`, fail-closed. Verified against five cases: real config,
malformed YAML, reintroduced bot skip, empty file, non-mapping.

Also: guard the two agent files the error message names; raise the job ceiling
to 25 so Analyze's step ceiling always fires first (tail budget is
`25 - setup - 17`, not a flat 3 minutes); annotate a declining triage gate so
it is not a silent green no-review; surface a failed running-state reply
(no `set -e` meant it reported success); verify the installed binary is
executable, the one case the bin-path gate cannot catch; `permissions: {}` on
review-skipped; and correct five factual claims — minimumReleaseAge errors
rather than downgrading, react/reply also write to the PR, the bundled agent
list omitted 4 of 11 including the two the next paragraph suppresses,
`--force` substitutes $20/200-turn defaults for the configured caps, and the
`cancelled()` wording in CLAUDE.md.

Verified: actionlint clean (it caught `cancelled()` being invalid in `env:`
and a duplicate permissions key), zizmor unchanged at 8 findings, markdownlint
clean, lefthook green.

Author: wkoutre

.claude/agents/workflow-security-reviewer.md

@@ -53,7 +53,7 @@ These are not a checklist to walk. They are what you find when you ask: can untr
 
 **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. Judge by complexity, not line count alone: a 30-line script making authenticated API calls and parsing JSON belongs in a file; a 25-line sequence of git commands may not. Say which it is and why.
+**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.
 

.claude/review.yml

@@ -28,8 +28,12 @@ model:
 # ─── 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. `agent_max_turns` is high on
-# purpose so the budget is the binding constraint, never the turn count.
+# 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
@@ -90,12 +94,27 @@ skip:
   # 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. This is the guard against a rubber-stamp approval, which
-# the retired reviewer had no equivalent of.
+# 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

.github/actions/install_review_cli/action.yml

@@ -44,8 +44,9 @@ runs:
     #     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
-    #     silently filter out a freshly published review-cli version and
-    #     resolve to an older one than the pin requests.
+    #     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,
@@ -75,4 +76,15 @@ runs:
         cd "$install_dir"
         bun init -y > /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"

.github/workflows/CLAUDE.md

@@ -517,7 +517,7 @@ This is how ai-toolkit reviews its **own** PRs. It runs [`@uniswap/review-cli`](
 | `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. They are separate workflow steps, and `post` runs on `success() || failure()` so a partial analyze still publishes what it found.
+**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.
 
@@ -537,6 +537,17 @@ The `triage` job's checkout is deliberately **not** pinned this way. It holds no
 
 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.
+- `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.
+
 **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":
@@ -557,18 +568,20 @@ So the `triage` job does a sparse checkout of `.github/actions` and `.claude`, a
 | `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 bundled reviewers (security, correctness, patterns, dependency-upgrade, general, plus triage and synthesis). ai-toolkit adds two:
+**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: 20` as a hard ceiling, but a job-level timeout **cancels** the job, and `cancelled()` matches neither `success()` nor `failure()`. 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, with 3 minutes of headroom for `Post` and the uploads.
+**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`, which would silently resolve a freshly pinned version to an older one. Installing from a scratch dir with its own `bunfig.toml` sidesteps both without touching the repo's copy.
+**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:**