Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
● Reviewed · against Note Approved — two comment-accuracy nits in Migrates PR-description automation from the retiring ai-toolkit reusable workflow to AssessmentThe untrusted-checkout boundary holds. Both CLI installs and the GPG-verified Claude Code binary land before the PR head is on disk, the packages-token bunfig is removed on every exit path, and every post-checkout invocation passes The one thing this PR still needs a human on: the step forwards both Suggestions
Worth keepingTip Both policy loops are driven by the git index ( Iteration history · 20 reviews2026-09-11 23:09 UTC · ✅ approved · 1 finding ·
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6eefa729de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| echo "::error::Claude Code release signing key does not match the pinned fingerprint $CLAUDE_CODE_GPG_FINGERPRINT" | ||
| exit 1 | ||
| fi | ||
| gpg --batch --verify "$WORK/manifest.json.sig" "$WORK/manifest.json" |
There was a problem hiding this comment.
When the downloaded key bundle contains both the pinned Anthropic public key and another key, the preceding fingerprint check passes, but this bare gpg --verify accepts a valid signature from any imported key. An attacker controlling the release and key downloads could therefore sign a forged manifest with the additional key, pass the binary checksum, and reach the credentialed posting step. Verify the signer fingerprint itself, for example with --assert-signer, using an isolated keyring.
Useful? React with 👍 / 👎.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
✅ Approved — see full review in the sticky comment ↑
Graphite Automations"Request reviewers once CI passes on sdks monorepo" took an action on this PR • (08/28/26)1 assignee was added and 3 reviewers were added to this PR based on Siyu Jiang (See-You John)'s automation. |
This comment has been minimized.
This comment has been minimized.
| bun init -y > /dev/null | ||
| if ! bun add "@uniswap/describe-cli@${DESCRIBE_CLI_VERSION}" 2> "$RUNNER_TEMP/bun-add.err"; then | ||
| cat "$RUNNER_TEMP/bun-add.err" >&2 | ||
| if grep -q ' 403 ' "$RUNNER_TEMP/bun-add.err"; then |
There was a problem hiding this comment.
🔵 info · correctness — ' 403 ' needs a space on both sides, so a bun error line ending in - 403 or 403. won't match and the package-access hint never prints — the exact failure this repo hits until the pre-merge grant is done.
| if grep -q ' 403 ' "$RUNNER_TEMP/bun-add.err"; then | |
| if grep -qE '(^|[^0-9])403([^0-9]|$)' "$RUNNER_TEMP/bun-add.err"; then |
| - name: Checkout PR head | ||
| uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 | ||
| with: | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
🔵 info · correctness — fetch-depth: 0 fetches the full monorepo history, but the comment above says describe-cli reads the diff over the API and the checkout "only supplies file context" — nothing here needs history.
| fetch-depth: 0 | |
| fetch-depth: 1 |
There was a problem hiding this comment.
Taken in 6adecbf: fetch-depth: 1. Checked describe-cli 0.2.17 first: its only git call is git rev-parse --short HEAD (build-info.ts); the diff comes from gh pr diff. Applied to describe.yml in every repo in the rollout.
| PR_NUMBER: ${{ github.event.pull_request.number }} | ||
| DESCRIBE_CLI: ${{ steps.install.outputs.bin }} | ||
| run: | | ||
| bun --config=/dev/null "$DESCRIBE_CLI" describe "$PR_NUMBER" \ |
There was a problem hiding this comment.
Caution
security · CWD .env auto-load
--config=/dev/null suppresses the CWD bunfig.toml but not bun's auto-load of .env/.env.local from the working directory — which here is the checked-out PR head. A committed .env setting ANTHROPIC_BASE_URL (unset in this step, so not shadowed) lands in process.env and is inherited by the spawned claude, redirecting the credentialed model API; Bullfrog is in audit mode, so the egress is logged, not blocked.
| bun --config=/dev/null "$DESCRIBE_CLI" describe "$PR_NUMBER" \ | |
| bun --config=/dev/null --env-file=/dev/null "$DESCRIBE_CLI" describe "$PR_NUMBER" \ |
There was a problem hiding this comment.
Caution
| PR_NUMBER: ${{ github.event.pull_request.number }} | ||
| DESCRIBE_CLI: ${{ steps.install.outputs.bin }} | ||
| run: | | ||
| bun --config=/dev/null --env-file=/dev/null "$DESCRIBE_CLI" describe "$PR_NUMBER" \ |
There was a problem hiding this comment.
🔵 info · security — A PR-authored .claude/settings.json is the remaining file-in-checkout channel of the same class as the bunfig/dotenv ones just closed: Claude Code reads project settings at startup and hooks entries run as commands. Worth confirming describe-cli passes a --settings override rather than resolving from the checkout cwd.
There was a problem hiding this comment.
Confirmed at the source, for both CLIs. describe-cli calls createAgentQuery with cwd set to the checkout and no settingSources (commands/describe.ts:246), agent-query defaults that to ['project'] (agent-query-sdk.ts:492), and review-cli sets ['project'] explicitly for the CI checkout case (pipelines/workspace.ts:170; its comment even says "inherit user's hooks"). Under that source Claude Code loads .claude/settings.json and .claude/settings.local.json from the checkout and runs their hooks entries. Closed at the workflow layer in 833a612: both workflows delete the two files right after the checkout, before anything launches Claude Code (CLAUDE.md, .claude/agents/ and .claude/review.yml are unaffected). The CLI-side fix is filed as ECO-891 item 16, and the same change is upstream in internal-tools#184.
| # `preload` array in-process, and it auto-loads a CWD dotenv file into | ||
| # any environment variable the job left unset. describe-cli's bin is a | ||
| # `#!/usr/bin/env bun` script, so it is launched as | ||
| # `bun --config=/dev/null --env-file=/dev/null "$DESCRIBE_CLI"`. Those |
There was a problem hiding this comment.
🔵 info · correctness — This launch hardening didn't reach the sibling workflow: claude-code-review.yml:421 invokes review-cli directly post-checkout without --config=/dev/null --env-file=/dev/null, and .github/actions/install_review_cli/action.yml:76 has neither the --config= flag nor the token-bunfig trap. Pre-existing, but the two should move together.
There was a problem hiding this comment.
Agreed, and pre-existing: sdks' own claude-code-review.yml and the install_review_cli action predate this PR and are out of its scope. Noted on ECO-887 as a follow-up so the two move together.
There was a problem hiding this comment.
Note
✅ Approved — see full review in the sticky comment ↑
…re the describe checkout Both CLIs run their agents through the Claude Agent SDK with the `project` setting source, so Claude Code reads `.claude/settings.json` and `.claude/settings.local.json` from the checkout and runs their `hooks` entries as commands. In CI that is a PR-author-controlled execution path of the same class as the bunfig and dotenv ones already closed, so both workflows now remove the two files right after the checkout. CLAUDE.md, `.claude/agents/` and `.claude/review.yml` are unaffected. describe.yml moves its checkout after the Claude Code install so the GPG verification and both installs run before any PR file is on disk, matching review.yml. The triage invocation gets the same `bun --config=/dev/null --env-file=/dev/null` launch as every other one; the triage job has no checkout, so this is consistency rather than a fix. Raised by review-cli on Uniswap/sdks#709 and Uniswap/hooks#12; same change upstream in Uniswap/internal-tools#184.
…re the describe checkout Both CLIs run their agents through the Claude Agent SDK with the `project` setting source, so Claude Code reads `.claude/settings.json` and `.claude/settings.local.json` from the checkout and runs their `hooks` entries as commands. In CI that is a PR-author-controlled execution path of the same class as the bunfig and dotenv ones already closed, so both workflows now remove the two files right after the checkout. CLAUDE.md, `.claude/agents/` and `.claude/review.yml` are unaffected. describe.yml moves its checkout after the Claude Code install so the GPG verification and both installs run before any PR file is on disk, matching review.yml. The triage invocation gets the same `bun --config=/dev/null --env-file=/dev/null` launch as every other one; the triage job has no checkout, so this is consistency rather than a fix. Raised by review-cli on Uniswap/sdks#709 and Uniswap/hooks#12; same change upstream in Uniswap/internal-tools#184.
…re the describe checkout Both CLIs run their agents through the Claude Agent SDK with the `project` setting source, so Claude Code reads `.claude/settings.json` and `.claude/settings.local.json` from the checkout and runs their `hooks` entries as commands. In CI that is a PR-author-controlled execution path of the same class as the bunfig and dotenv ones already closed, so both workflows now remove the two files right after the checkout. CLAUDE.md, `.claude/agents/` and `.claude/review.yml` are unaffected. describe.yml moves its checkout after the Claude Code install so the GPG verification and both installs run before any PR file is on disk, matching review.yml. The triage invocation gets the same `bun --config=/dev/null --env-file=/dev/null` launch as every other one; the triage job has no checkout, so this is consistency rather than a fix. Raised by review-cli on Uniswap/sdks#709 and Uniswap/hooks#12; same change upstream in Uniswap/internal-tools#184.
…re the describe checkout Both CLIs run their agents through the Claude Agent SDK with the `project` setting source, so Claude Code reads `.claude/settings.json` and `.claude/settings.local.json` from the checkout and runs their `hooks` entries as commands. In CI that is a PR-author-controlled execution path of the same class as the bunfig and dotenv ones already closed, so both workflows now remove the two files right after the checkout. CLAUDE.md, `.claude/agents/` and `.claude/review.yml` are unaffected. describe.yml moves its checkout after the Claude Code install so the GPG verification and both installs run before any PR file is on disk, matching review.yml. The triage invocation gets the same `bun --config=/dev/null --env-file=/dev/null` launch as every other one; the triage job has no checkout, so this is consistency rather than a fix. Raised by review-cli on #709 and Uniswap/hooks#12; same change upstream in Uniswap/internal-tools#184.
| # and dotenv ones above, so the two files go before anything launches | ||
| # Claude Code. Nothing describe-cli reads lives in them. | ||
| - name: Drop PR-authored Claude Code settings | ||
| run: rm -f .claude/settings.json .claude/settings.local.json |
There was a problem hiding this comment.
🔵 info · security — The drop is root-only. Claude Code also discovers directory-scoped config, so sdks/*/.claude/settings.json (and a root .mcp.json stdio command) survive into the credentialed launch.
| run: rm -f .claude/settings.json .claude/settings.local.json | |
| run: | | |
| find . -path ./.git -prune -o -path '*/.claude/settings*.json' -print0 | xargs -0r rm -f | |
| rm -f .mcp.json |
Note
Bounded here — pull_request runs the workflow file from the PR head and this job only runs for same-repo branches, so the author already controls describe.yml. Worth covering in the upstream template.
There was a problem hiding this comment.
Adopted in 6588abc: the step now runs a find over the whole tree (.git pruned) for .claude/settings.json, .claude/settings.local.json and .mcp.json, removes each with rm -rf, marks tracked ones assume-unchanged so review-cli keeps the in-place review, and emits a ::warning:: per file. Same change upstream in internal-tools#184 (2419be9).
| # `preload` array in-process, and it auto-loads a CWD dotenv file into | ||
| # any environment variable the job left unset. describe-cli's bin is a | ||
| # `#!/usr/bin/env bun` script, so it is launched as | ||
| # `bun --config=/dev/null --env-file=/dev/null "$DESCRIBE_CLI"`. Those |
There was a problem hiding this comment.
🔵 info · patterns — claude-code-review.yml still invokes "$REVIEW_CLI_BIN/review-cli" directly after a refs/pull/N/head checkout, and install_review_cli/action.yml has no --config equivalent — same untrusted-checkout shape, same LLM credential, without this launch form.
There was a problem hiding this comment.
Agreed and pre-existing; noted on ECO-887 (2026-09-10 18:58 UTC) as a follow-up for claude-code-review.yml and the install_review_cli action so both get the same launch form when next touched. Out of this PR's scope.
| # max_budget_usd but enforce only the per-agent budget below. Sized for | ||
| # four reviewers at their cap plus triage and synthesis, because the | ||
| # triage guidance staffs four or more reviewers on a Solidity change. | ||
| max_budget_usd: 40.0 |
There was a problem hiding this comment.
🔵 info · correctness — The comment sizes 40.0 for "four reviewers at their cap plus triage and synthesis", but 4 × agent_budget_usd: 10.0 is already 40.0. Harmless while the key is unenforced; if enforcement lands, a four-reviewer run exhausts the ceiling before synthesis runs.
There was a problem hiding this comment.
Comment corrected in 6588abc: it now states the real ceiling is agent_budget_usd times every agent staffed (four reviewers plus triage and synthesis is six, so $60 at the cap) and that max_budget_usd only records intent while 1.12.0 does not enforce it.
|
|
||
| # Per-agent budget — soft cap. Same value as universe/backend so | ||
| # investigations on cross-package SDK changes aren't budget-starved. | ||
| # Whole-run ceiling, stated as intent only: review-cli 1.10.7 and 1.12.0 |
There was a problem hiding this comment.
🔵 info · correctness — This block was inserted between the "Per-agent budget — soft cap" comment (lines 18-19) and the key it describes, so that comment now heads max_budget_usd and agent_budget_usd: 10.0 is left unlabeled. Moving the new block below agent_budget_usd keeps each comment attached to its key.
| drop() { | ||
| rm -rf "$1" | ||
| git -C "$(dirname "$1")" update-index --assume-unchanged "$(basename "$1")" 2>/dev/null || true | ||
| echo "::warning::Removed PR-authored $1 before launching Claude Code; inspect it in the PR diff." |
There was a problem hiding this comment.
hooks/mcpServers from executing, and it's the only run: block in the file without set -euo pipefail; the ::warning:: prints whether or not rm -rf succeeded. A failed removal is indistinguishable from a successful one in the log and the job proceeds to launch Claude Code against the surviving settings.
| echo "::warning::Removed PR-authored $1 before launching Claude Code; inspect it in the PR diff." | |
| rm -rf "$1" || { echo "::error::Failed to remove PR-authored $1"; exit 1; } |
Note
set -euo pipefail alone wouldn't cover this — drop runs inside a while loop body, so the explicit check is what makes the failure fatal.
There was a problem hiding this comment.
Fixed in 99ac843 with your suggestion: rm -rf "$1" || { echo "::error::Could not remove PR-authored $1"; exit 1; }, so the ::warning:: only prints for a removal that succeeded and a failure stops the job before Claude Code launches. Same on the internal-tools templates (8944bd0).
| # anything launches Claude Code. Nothing describe-cli reads lives in them. | ||
| - name: Drop PR-authored Claude Code settings | ||
| run: | | ||
| # review-cli reviews this checkout in place only while |
There was a problem hiding this comment.
🔵 info · correctness — This rationale is about review-cli's in-place-review precondition, but review-cli never runs in this workflow, so the assume-unchanged calls are inert here. claude-code-review.yml — the workflow the clean-tree requirement actually applies to — is byte-identical to origin/main on this branch and has no drop step at all.
There was a problem hiding this comment.
Correct: describe-cli has no clean-tree precondition (nothing in packages/describe-cli/src reads git status), so the assume-unchanged calls were inert here. 99ac843 removes them from describe.yml's drop() and replaces the copied rationale with one about describe-cli. claude-code-review.yml is outside this PR's scope and is tracked on ECO-887 as the sibling workflow that still lacks the launch hardening.
| # ANTHROPIC_API_KEY (its documented authentication precedence), so | ||
| # a stale key cannot be shadowed by adding a token; replace or | ||
| # remove the key instead. | ||
| ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} |
There was a problem hiding this comment.
🔵 info · security — claude-code-review.yml:332 records not forwarding ANTHROPIC_API_KEY as a deliberate compensating control (metered spend vs. subscription-billed OAuth); this step forwards both. Defensible given the stronger install chain here, but the comment above only explains precedence, not the divergence.
There was a problem hiding this comment.
Fair. 99ac843 extends the comment: both secrets are forwarded because repos differ in which one they hold, and a repo that deliberately withholds ANTHROPIC_API_KEY from its review workflow should drop it here as well. Whether sdks wants that divergence closed is a repo decision I have left to the reviewers of this PR.
…drop step closed, stop on a moved head review-cli reads CLAUDE.md, .claude/rules/, .claude/agents/ and .claude/review.yml from the PR-head checkout, so a PR could change its own review's budget, model, allowed actors or instructions. A new step rewrites every such tracked path to the base branch's content, removing ones the base lacks and restoring ones the PR deleted, while keeping git status clean so review-cli still reviews the checkout in place. The drop step now fails the job when a removal does not take instead of falling through to extraction, and a new step stops the run before the sticky placeholder when the PR head has moved since checkout, since review-cli re-resolves the head and would otherwise extract the PR files under the same setting source. describe.yml gets the same policy step without the index bookkeeping it does not need, its drop comment now describes describe-cli rather than review-cli, and its credential comment says why both secrets are forwarded. Raised by review-cli on protocol-fees #154, security #93, tjar #21, v4-hooks-internal #103 and sdks #709 (2026-09-10).
| # Paths inside submodules are left alone. | ||
| base_sha=$(git rev-parse --verify --quiet "refs/remotes/origin/$BASE_REF^{commit}") \ | ||
| || { echo "::error::origin/$BASE_REF is not in this checkout; fetch-depth must stay 0"; exit 1; } | ||
| policy='(^|/)CLAUDE(\.local)?\.md$|^\.claude/review\.yml$|^\.claude/(agents|rules)(/|$)' |
There was a problem hiding this comment.
policy covers CLAUDE.md, .claude/review.yml, .claude/agents and .claude/rules, but not .claude/skills/. Project skills load under the same settingSources: ['project'] gate, and each SKILL.md description lands in the agent's context automatically — so a PR-authored skill reaches the describe agent as repo policy through the one instruction channel this step's set doesn't cover.
| policy='(^|/)CLAUDE(\.local)?\.md$|^\.claude/review\.yml$|^\.claude/(agents|rules)(/|$)' | |
| policy='(^|/)CLAUDE(\.local)?\.md$|^\.claude/review\.yml$|^\.claude/(agents|rules|skills|commands)(/|$)' |
Note
.claude/commands/ is a weaker channel (invocation is user-driven) but lives under the same discovery mechanism, so folding both in keeps the set aligned with what the setting source reads.
There was a problem hiding this comment.
Right, and .claude/commands/ and .claude/review-context.md (which review-cli injects verbatim, repo-context.ts:70) had the same gap. 2abcd6c replaces the list with everything under .claude/ at the root and below plus CLAUDE.md/CLAUDE.local.md: policy='(^|/)CLAUDE(\.local)?\.md$|(^|/)\.claude(/|$)', identical in both workflows. Verified on a synthetic PR adding .claude/skills/evil/SKILL.md and a nested pkg/.claude/agents/n.md: both removed, base files restored.
…view which files were replaced, assert every removal The base-branch policy set now covers everything under .claude/ (review.yml, review-context.md, agents/, rules/, skills/, commands/) at the root and below, plus CLAUDE.md and CLAUDE.local.md; review-cli injects .claude/review-context.md verbatim and Claude Code loads project skills and commands under the same setting source, so the previous list left instruction channels open. The review step records which paths it replaced and the Analyze step passes them to review-cli as a --note, which synthesis surfaces as a reviewer note, so the agents know the policy they run under is the base branch's. drop() now asserts the path is gone before logging the removal, the new steps run under set -euo pipefail, and both files carry a reminder that the policy block must stay identical apart from review-cli's index bookkeeping. Raised by review-cli on tjar #21, sdks #709, hooks #12, v4-hooks-internal #103, uniswap-ai #146 and ai-sandbox #56 (2026-09-10).
| [[ $p =~ $policy ]] || continue | ||
| if blob_at "$base_sha" "$p"; then | ||
| if ! git diff --quiet "$base_sha" HEAD -- "$p"; then | ||
| rm -rf "$p"; mkdir -p "$(dirname "$p")"; git show "$base_sha:$p" > "$p" |
There was a problem hiding this comment.
(^|/)\.claude(/|$)) also matches .claude/settings.json, so this line writes the base branch's settings file back to disk after the drop step deleted it at 262 — Claude Code then loads it and runs its hooks. The shape inverts: a PR that touches .claude/settings.json resurrects the base copy, one that leaves it alone runs with none.
Note
No impact in this repo — origin/main tracks only .claude/review.yml, so the else-branch at 297 removes it instead. But this block is copied verbatim across the migration branches, and the comment at 242 ("every copy goes before anything launches Claude Code") stops being true once this step runs. Excluding settings*.json and .mcp.json from the policy set, or ordering the drop step after this one, restores the stated guarantee.
There was a problem hiding this comment.
Confirmed and fixed in 10b577a: the auto-executing files (.claude/settings*.json, .mcp.json) are now excluded from both restore branches and only ever removed, so the drop step's guarantee holds after this step too. Verified on a synthetic PR whose base carries a hooked settings.json and a .mcp.json that the head modifies: both absent afterwards, git status --porcelain empty.
| while IFS= read -r -d '' p; do | ||
| [[ $p =~ $policy ]] || continue | ||
| if blob_at "$base_sha" "$p"; then | ||
| if ! git diff --quiet "$base_sha" HEAD -- "$p"; then |
There was a problem hiding this comment.
🔵 info · correctness — $p is a pathspec here, so glob metacharacters in a tracked policy filename are interpreted rather than matched literally: for a base-existing .claude/rules/x[1].md the pathspec matches nothing, --quiet reports no change, and the PR's version survives as instructions. blob_at uses git cat-file, which is exact-path, so only this call needs the literal magic.
| if ! git diff --quiet "$base_sha" HEAD -- "$p"; then | |
| if ! git diff --quiet "$base_sha" HEAD -- ":(literal)$p"; then |
There was a problem hiding this comment.
Taken as suggested in 10b577a: git diff --quiet "$base_sha" HEAD -- ":(literal)$p", and the same magic on the git ls-files -z -- ":(literal)$p" call in review.yml's base-only loop. Checked with a base-tracked .claude/rules/x[1].md modified by the PR: the base version is what ends up on disk.
| echo "::warning::Removed PR-authored $1 before launching Claude Code; inspect it in the PR diff." | ||
| } | ||
| while IFS= read -r -d '' f; do drop "$f"; done < <(find . -path ./.git -prune -o -type l \( -name '.claude' -o -name '.mcp.json' -o -path '*/.claude/settings.json' -o -path '*/.claude/settings.local.json' \) -print0) | ||
| while IFS= read -r -d '' f; do drop "$f"; done < <(find . -path ./.git -prune -o \( -path '*/.claude/settings.json' -o -path '*/.claude/settings.local.json' -o -name '.mcp.json' \) -print0) |
There was a problem hiding this comment.
🔵 info · security — .mcp.json has single-layer coverage: the policy regex at 287 normalizes every .claude/** path to base a second time from git ls-files, but .mcp.json depends on this find alone, and process substitution discards find's exit status. Adding |(^|/)\.mcp\.json$ to the policy at 287 gives it the same index-driven second pass.
There was a problem hiding this comment.
Done in 10b577a, by a slightly different route than adding .mcp.json to the regex as a restorable path (that would write the base copy back, which the two findings above show is wrong): the policy pass now treats .mcp.json and .claude/settings*.json as always-remove, so .mcp.json gets the index-driven second removal you asked for without ever being restored.
There was a problem hiding this comment.
Correction to my reply above: the .mcp.json arm I described was unreachable because the policy regex did not match .mcp.json, so the second removal did not exist. Fixed in 5981b11 (regex extended; verified by running the policy step alone), and recorded as an error on my side in the PR body's round-14 section.
| # description. | ||
| # | ||
| # Keep this block identical to "Take the review policy from the base | ||
| # branch" in review.yml, minus the assume-unchanged and exclude |
There was a problem hiding this comment.
🔵 info · correctness — There is no review.yml in this repo's workflows — the review workflow is claude-code-review.yml, and it has neither the drop step nor a base-branch policy step, so there is nothing here to keep in sync.
There was a problem hiding this comment.
Fixed in 10b577a: the comment is now conditional ("In a repo that also ships review-cli's review.yml …"), so it is accurate here; the file stays byte-identical across the fleet by design. claude-code-review.yml's missing drop and policy steps are tracked on ECO-887.
…ranch, literal pathspecs, random output delimiter The base-branch policy pass matched .claude/settings.json, .claude/settings.local.json and .mcp.json, so a PR that modified one of them had the base branch's copy written back after the drop step had removed it, and its hooks or MCP servers would run. Those files are now never written from either ref; the pass only removes them again, which also gives them an index-driven second removal independent of find. Paths go to git as :(literal) pathspecs so glob characters in a filename match themselves, the GITHUB_OUTPUT heredoc uses a random delimiter so a filename containing a newline cannot end it early and inject outputs, and the sync-reminder and credential comments no longer assume the sibling workflow exists in this repo. Raised by review-cli on uniswap-ai #146, sdks #709 and security #93 (2026-09-10).
| # as `:(literal)` pathspecs. Paths inside submodules are left alone. | ||
| base_sha=$(git rev-parse --verify --quiet "refs/remotes/origin/$BASE_REF^{commit}") \ | ||
| || { echo "::error::origin/$BASE_REF is not in this checkout; fetch-depth must stay 0"; exit 1; } | ||
| policy='(^|/)CLAUDE(\.local)?\.md$|(^|/)\.claude(/|$)' |
There was a problem hiding this comment.
policy has no .mcp.json branch, so [[ $p =~ $policy ]] || continue (line 293) skips the path and the */.mcp.json|.mcp.json arms at 295 and 308 are unreachable. That makes the comment at 285-287 ("a second, index-driven removal for them") true for .claude/settings*.json and false for .mcp.json, which rests on the find at 262 alone — and that loop's exit status is discarded by process substitution.
| policy='(^|/)CLAUDE(\.local)?\.md$|(^|/)\.claude(/|$)' | |
| policy='(^|/)CLAUDE(\.local)?\.md$|(^|/)\.claude(/|$)|(^|/)\.mcp\.json$' |
Note
The suggested regex matches .mcp.json and pkg/.mcp.json, not notmcp.json or a/.mcp.jsonx. Removal still happens today; the code just doesn't have the second layer the comment describes.
There was a problem hiding this comment.
Taken as suggested in 5981b11: policy='(^|/)CLAUDE(\.local)?\.md$|(^|/)\.claude(/|$)|(^|/)\.mcp\.json$'. My round-13 reply on the neighbouring thread claimed this second layer already existed; it did not, and the comment was wrong until now. Checked by running the policy step alone (no drop step) on a checkout with .mcp.json and pkg/.mcp.json: both removed.
…the policy set, restore with git checkout so symlinks survive The policy regex never matched .mcp.json, so the case arm that was meant to give it a second, index-driven removal was unreachable and round 13's comment claimed protection that was not there; the regex now includes it and the arm runs. The three auto-executing file names lived in six literal copies across the drop and policy steps of both workflows; they are now one job-level CLAUDE_AUTORUN_FILES list that the find and the policy loop both read. Restoring a base-branch file with `git show >` flattened a symlink into a plain file holding its target string; files are now written with `git checkout <base> -- :(literal)path` (plus `git reset` on the index entry in review.yml), which reproduces symlinks and modes. The sync-reminder comment names the canonical files so a repo with an older claude-code-review.yml is not misread. Raised by review-cli on hooks #12, ai-sandbox #56, universal-router #506, sdks #709 and uniswap-ai #146 (2026-09-10).
| # Whole-run ceiling, stated as intent only: review-cli 1.10.7 and 1.12.0 | ||
| # parse max_budget_usd but enforce only the per-agent budget below, so the | ||
| # real ceiling is agent_budget_usd times every agent staffed. The triage | ||
| # guidance staffs four or more reviewers on a Solidity change; with triage |
There was a problem hiding this comment.
🔵 info · correctness — This repo's triage guidance (lines 43-55) describes a "Public TypeScript SDK monorepo" and names financial math, calldata/ABI encoding and permit/signature handling as the high-stakes surfaces; there is no Solidity condition. The $60 arithmetic holds either way, but the justification reads as carried over from a contracts repo's copy.
There was a problem hiding this comment.
Correct: the sentence was carried over from a contracts repo's copy. a10ecba rewords it to this repo's own triage surfaces (financial math, calldata encoding, permit handling); the $60 arithmetic is unchanged. The same copy was in Uniswap/security's review.yml, a Python tooling repo, and is fixed there in 3ca5282. The other seven repos carrying the sentence are Foundry/Solidity repos where it is accurate.
…, not Solidity The max_budget_usd comment justified the $60 figure with "four or more reviewers on a Solidity change", wording carried over from a contracts repo. This is a TypeScript SDK monorepo and the triage guidance below staffs extra reviewers for financial math, calldata/ABI encoding and permit/signature handling; the comment now says so. The arithmetic is unchanged. Raised by review-cli on sdks #709 at 5981b11 (2026-09-10).
| # real ceiling is agent_budget_usd times every agent staffed. The triage | ||
| # guidance staffs four or more reviewers on a change to financial math, | ||
| # calldata encoding or permit handling; with triage | ||
| # and synthesis that is six agents, or $60 at the per-agent cap, so this |
There was a problem hiding this comment.
🔵 info · correctness — The worked example sizes the ceiling at six agents / $60, but lines 5-8 of this file state the repo ships no .claude/agents/, so only the two bundled reviewers can be staffed — four agents with triage and synthesis, i.e. $40, the value on line 27.
a10ecba to
28c498e
Compare
| github.event.pull_request.head.repo.full_name == github.repository && | ||
| (contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.pull_request.author_association) || | ||
| github.event.pull_request.user.type == 'Bot') && | ||
| !startsWith(github.head_ref, 'gh-readonly-queue/') && |
There was a problem hiding this comment.
🔵 info · correctness — The deleted workflow also skipped !contains(github.head_ref, 'gtmq'); this guard set drops it, so Graphite merge-queue branches now trigger a describe run. gtmq appears nowhere else in the repo, so nothing re-establishes the skip.
Note
The old guard used contains(…, 'release/') where this uses startsWith, so a branch like foo/release/bar is no longer skipped either.
There was a problem hiding this comment.
Confirmed: the describe-cli 0.2.18 template gate has no gtmq skip. Template-level; tracked as ECO-891 item 29.
1 participant
What changed
Replaces the ai-toolkit
_generate-pr-metadata.ymlcall with@uniswap/describe-clivia.github/workflows/describe.yml. Review automation already runs on review-cli and is untouched..claude/review.ymlgets the describe-related policy lines.PR title generation is gone by design; describe-cli writes descriptions only.
check-pr-title.yamlstill enforces conventional-commit titles. The old caller's explicitchangeset-release/skip is dropped as redundant:changeset-release/mainmatches the workflow'srelease/guard, which also skipscursor[bot]andcherry-pick/branches.Why
ai-toolkit is retiring its reusable
_generate-pr-metadata.ymlworkflow. describe-cli writes descriptions additively and backs off from human-edited text.describe.ymlis the template published with describe-cli 0.2.18, unchanged. It carries the hardening from internal-tools#184: a Bullfrog egress step first, the CLI installed pinned from GitHub Packages with the token file removed on every exit path, the Claude Code binary GPG-verified against a pinned key, PR-authored Claude Code settings and.mcp.jsondropped before Claude Code starts, and instruction files taken from the repository's default branch.Before merging (repo/org admin)
describe-clipackage: Uniswap/internal-tools -> Packages -> package settings -> Manage Actions access. Without it the install step fails with a 403.ANTHROPIC_API_KEYorCLAUDE_CODE_OAUTH_TOKENworks as-is. If both are set, Claude Code uses the API key, so a stale key must be replaced or removed, not shadowed by adding a token.Verified
describe.ymlis byte-identical totemplates/workflows/describe.ymlat tagdescribe-cli-v0.2.18in internal-tools, andactionlintpasses. The branch is one SSH-signed commit on top ofmain. The describe-cli run on this PR is the new workflow exercising itself. The repo variablesREVIEW_CLI_VERSION=1.12.1,DESCRIBE_CLI_VERSION=0.2.18andCLAUDE_CODE_VERSION=2.1.267are already set.✨ Claude-Generated Content
Note
Adding context the description above doesn't cover.
describe.ymlrequests an explicitpermissionsblock (contents: read,packages: read,pull-requests: write); the deletedclaude-pr-metadata-update.ymlran withpermissions: {}and got its access from the reusable workflow's own OIDC (id-token: write) grant instead.