From 70000f8575073d28bacb0a55334965d336331a39 Mon Sep 17 00:00:00 2001 From: Eric Andrechek Date: Tue, 12 May 2026 10:04:20 -0400 Subject: [PATCH 1/2] fix(ci): claude review posts inline review comments, not one sticky blob MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Switch the Claude review output from "everything in one sticky comment" to "inline review comments on specific lines + a brief sticky summary". Inline comments via the `mcp__github_inline_comment__create_inline_comment` MCP tool become real PR review threads — they show next to the changed line, must be resolved before merge (the ruleset's `required_review_thread_resolution: true` enforces this), and are the same mechanism Gemini Code Assist uses. No new workflow, no custom check, no ruleset change — native review-thread resolution is the blocking signal. `claude-review.yml`: - Drop `track_progress: true`. The progress-tracking comment is redundant once findings land on the lines they refer to. - Add `claude_args.--allowedTools` enumerating the MCP inline tool plus the narrow `gh pr` reads the prompt instructs Claude to use. Without an explicit allow-list, Claude Code's default tool set doesn't include the inline-comment MCP tool, so the model fell back to writing everything into the sticky. - Keep `use_sticky_comment: true` so the summary edits in place across pushes instead of stacking "Claude reviewed again" comments. `.github/prompts/pr-review.md`: - Spell out the MCP tool call by name and tell Claude to put line-level findings there. - Reserve the sticky summary for a short verdict headline (severity counts + the `Ship it` / `Iterate` / `Block` line). - Tighten the "what not to comment on" list. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/prompts/pr-review.md | 49 ++++++++++++++++++++--------- .github/workflows/claude-review.yml | 17 +++++++++- 2 files changed, 50 insertions(+), 16 deletions(-) diff --git a/.github/prompts/pr-review.md b/.github/prompts/pr-review.md index d7949754..a569e984 100644 --- a/.github/prompts/pr-review.md +++ b/.github/prompts/pr-review.md @@ -115,21 +115,40 @@ Review against each of these, in this order: ## Output discipline -- Post inline comments on specific lines where the issue is - concrete. Use top-level comments for architectural - concerns or praise. -- In the summary, separate findings by severity. End with - **exactly one of**: `Ship it`, `Iterate`, or `Block`, - followed by the single most important thing the author - must address. -- Use `Block` only when a CRITICAL/HIGH security finding, - data-loss risk, or broken core invariant is present. Use - `Iterate` for everything else that needs changes. -- Don't repeat what the linter already catches (gofumpt, - govet, staticcheck, gosec, gocritic, errcheck, etc. — see - `.golangci.yml`). CI enforces those. -- Don't suggest comments on self-explanatory code — this - project prefers well-named identifiers. +**Use inline review comments for specific line-level +findings.** Call `mcp__github_inline_comment__create_inline_comment` +with `confirmed: true` for each concrete issue. These become +real PR review threads that show next to the line in the diff, +must be resolved before merge (the repo's ruleset has +`required_review_thread_resolution: true`), and are the same +mechanism Gemini Code Assist uses. *Do not* dump every +finding into one giant prose blob — that pattern caused the +sticky comment to bloat. + +**Use the sticky summary comment for the verdict only.** One +short top-level comment with: + - A one-line headline grouping findings by severity (e.g. + "1 HIGH security, 2 MEDIUM correctness, 1 LOW docs"). + - A pointer to read the inline threads for detail. + - The verdict line, **exactly one of**: `Ship it`, + `Iterate`, or `Block`, followed by the single most + important thing the author must address. + +Verdict rules: + - `Block` only when a CRITICAL/HIGH security finding, + data-loss risk, or broken core invariant is present. + - `Iterate` for everything else that needs changes. + - `Ship it` only if there are no findings worth resolving. + +What not to comment on: + - Anything the linter already catches (gofumpt, govet, + staticcheck, gosec, gocritic, errcheck, etc. — see + `.golangci.yml`). CI enforces those. + - Self-explanatory code — this project prefers well-named + identifiers over explanatory comments. + - Don't post `gh pr comment` floors of prose for findings + that belong on a specific line — use an inline comment + there instead. ## Noise filter (important) diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 76873298..6a8a91ff 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -221,6 +221,21 @@ jobs: if: steps.gate.outputs.skip != 'true' with: claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} + # One summary comment per PR — sticky so iteration history + # stays in one place rather than accreting "Claude reviewed + # again" comments per push. Specific findings go in inline + # review comments (see `claude_args` below + the prompt + # template) which become real review threads that count + # against the ruleset's `required_review_thread_resolution` + # — same blocking mechanism Gemini and Copilot's review + # comments use. use_sticky_comment: true - track_progress: true + # `claude_args` allows the inline-comment MCP tool plus the + # narrow `gh pr` reads the prompt instructs Claude to use. + # Without an explicit allow-list, Claude Code's default + # tool set doesn't include the inline-comment MCP tool, so + # the model falls back to writing everything into the + # sticky comment. + claude_args: | + --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh api repos/*/pulls/*/comments:*),Bash(gh api repos/*/issues/*/comments:*),Read,Glob,Grep" prompt: ${{ steps.prompt.outputs.content }} From 4d323bab8bbab7286e5f449f319a357436ab9e6d Mon Sep 17 00:00:00 2001 From: Eric Andrechek Date: Tue, 12 May 2026 12:12:05 -0400 Subject: [PATCH 2/2] docs: align AGENTS.md + prompt with inline-review behavior MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Gemini review feedback on the parent claude-review change: - AGENTS.md `Review tooling reference` table row for Claude was still describing the workflow_run + sticky-comment-only behavior. Updated to reflect: pull_request trigger, inline-comment + sticky-summary output shape, and that inline threads block merge via the existing `required_review_thread_resolution` ruleset (so "blocks merge" goes from "No (advisory)" to "Yes for inline comments"). - AGENTS.md §"Tier 2 — Code review" Claude bullet rewritten to describe the new HEAD-committer trust gate and the inline-comment output, and to drop the now-incorrect "runs after every successful CI" claim. - Prompt (`.github/prompts/pr-review.md`) now explicitly requires each inline comment be tagged with exactly one of `[MUST]` / `[SHOULD]` / `[MAY]`, matching `.gemini/styleguide.md`'s Rule 13 so both reviewers speak the same language and the author can filter. - Verdict rules aligned to the styleguide's Rule 19: `Ship it` when there are no `[MUST]`s and few or no `[SHOULD]`s (previously required "no findings worth resolving" which was stricter than the styleguide); `Iterate` keyed on `[MUST]` or multiple `[SHOULD]`s; `Block` reserved for CRITICAL/HIGH security or data-loss. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/prompts/pr-review.md | 30 ++++++++++++++++++++++++------ AGENTS.md | 4 ++-- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/.github/prompts/pr-review.md b/.github/prompts/pr-review.md index a569e984..8c5bd1ef 100644 --- a/.github/prompts/pr-review.md +++ b/.github/prompts/pr-review.md @@ -125,20 +125,38 @@ mechanism Gemini Code Assist uses. *Do not* dump every finding into one giant prose blob — that pattern caused the sticky comment to bloat. +**Tag every inline comment with exactly one severity** at the +start of the body: `[MUST]`, `[SHOULD]`, or `[MAY]`. This +matches `.gemini/styleguide.md` so both reviewers speak the +same language and the author can filter on tag. + + - `[MUST]` — correctness bug, security issue, broken + invariant, missing required documentation sync. The PR + can't merge until this is addressed. + - `[SHOULD]` — quality / maintainability issue the author + should fix, but isn't a release blocker if they push back + with reasoning. + - `[MAY]` — minor suggestion, style nit, alternative + approach. Take or leave. + **Use the sticky summary comment for the verdict only.** One short top-level comment with: - A one-line headline grouping findings by severity (e.g. - "1 HIGH security, 2 MEDIUM correctness, 1 LOW docs"). + "2 [MUST], 1 [SHOULD], 0 [MAY]"). - A pointer to read the inline threads for detail. - The verdict line, **exactly one of**: `Ship it`, `Iterate`, or `Block`, followed by the single most important thing the author must address. -Verdict rules: - - `Block` only when a CRITICAL/HIGH security finding, - data-loss risk, or broken core invariant is present. - - `Iterate` for everything else that needs changes. - - `Ship it` only if there are no findings worth resolving. +Verdict rules (matched to the styleguide): + - `Block` — a `[MUST]` finding that's a CRITICAL/HIGH + security issue, data-loss risk, or broken core + invariant. Cannot proceed without addressing. + - `Iterate` — one or more `[MUST]` findings that aren't + `Block`-level, or multiple `[SHOULD]` findings that + collectively need a pass. + - `Ship it` — no `[MUST]`s and few or no `[SHOULD]`s. + `[MAY]` findings alone don't preclude `Ship it`. What not to comment on: - Anything the linter already catches (gofumpt, govet, diff --git a/AGENTS.md b/AGENTS.md index 4a657929..963bcb0e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -204,7 +204,7 @@ If you hand work to a subagent or another Claude session, tell them explicitly: | Reviewer | How it runs | Re-runs on new commits | Blocks merge | | -------- | ----------- | ---------------------- | ------------ | -| Claude (`.github/workflows/claude-review.yml`) | Our workflow, `workflow_run: [CI completed]` trigger — fires after every successful CI on a PR. Manual re-trigger via `@claude` or `/review` in a PR comment, or `gh workflow run "Claude PR review" -f pr_number=` | Yes, auto — **updates the same sticky comment** rather than posting new ones | No (advisory) unless added to required checks | +| Claude (`.github/workflows/claude-review.yml`) | Our workflow, `pull_request: [opened, synchronize, reopened, ready_for_review]` trigger — fires on every PR push directly (not chained off CI). Manual re-trigger via `@claude` or `/review` in a PR comment, or `gh workflow run "Claude PR review" -f pr_number=` | Yes, auto — **posts inline review comments** at specific lines (resolution required by the ruleset) plus a sticky verdict-summary top-level comment that edits in place across pushes | Yes for inline comments — the ruleset's `required_review_thread_resolution: true` blocks merge until each `claude[bot]` thread is resolved. The workflow's check itself is advisory | | Gemini Code Assist | Marketplace App at repo level | Yes on synchronize, **but silently skips `.github/workflows/**`** (built-in exclusion, can't be overridden) — so Gemini reviews rarely see infra PRs | No (advisory) | | Copilot | GitHub-native when reviewer has Copilot Pro enabled | Yes if enabled in Copilot settings | No (advisory) | | Human admins (Eric / Taite) | Auto-assigned to the **PR** by `.github/workflows/project-orchestrator.yml` **only once** the PR is bot-clean: required checks (CI / PR housekeeping) green AND all review threads resolved. Both the `assignees` field AND a GitHub review-request are set together — the board's single-reviewer assignee is the queue signal, the review-request drives GitHub's native notification + dismiss-stale-reviews-on-push interaction (the two must stay in sync; an assignee without a review request would leave the reviewer uninformed mid-iteration). Draft PRs flip to ready at the same moment, PR card goes on Task Board (project #7) with Status=Ready, linked issues move to Status=In review. Reviewer selection (single admin): PR author == Eric → Taite; author == Taite → Eric; any other author → picked by PR-number parity (even → Eric, odd → Taite) to spread load. | Workflow re-checks on every push (`pull_request_target: synchronize`), review (`pull_request_review: submitted` — including `COMMENTED` reviews from bots like Gemini), and CI completion (`workflow_run: [CI completed]`). `check_suite` is **not** used: GitHub suppresses `check_suite` events when the suite was created by a `GITHUB_TOKEN`-triggered workflow (our CI), so the `workflow_run` trigger is the reliable chain-off signal. Idempotent across re-fires. `pull_request_review_thread` would be the natural trigger for thread-resolution events but GitHub Actions' parser rejects it; re-evaluation after thread resolution happens implicitly via the next push, bot review, or CI completion. | Yes — `.github/workflows/admin-approval.yml` is a required status check that fails unless Eric or Taite has an `APPROVED` review (Dependabot PRs bypass). | @@ -333,7 +333,7 @@ docs/ → Project documentation 1. **Tier 1 — Issue triage** (`.github/workflows/triage.yml`): GitHub Models (`gpt-4o-mini` via `actions/ai-inference`) classifies new/edited issues and applies `area/*` + `security` + `breaking-change` labels. Optionally writes the `Priority` custom field on the Task Board (Project #7) when a `PROJECT_BOARD_TOKEN` secret with project scope is configured. 2. **Tier 2 — Code review** (two reviewers, both advisory; the `Admin approval` required status check + the ruleset are the actual merge-gate): - **Gemini Code Assist App** configured via `.gemini/styleguide.md` — Marketplace App attached at the repo/org level, no workflow file. - - **Claude PR review** (`.github/workflows/claude-review.yml`) — `anthropics/claude-code-action` runs automatically after every successful CI on a PR, but only when the PR author is already OWNER/MEMBER/COLLABORATOR/CONTRIBUTOR to bound token cost. Dependabot and draft PRs are skipped. Manual re-trigger via `@claude` or `/review` in a PR comment from a trusted actor, or via `gh workflow run "Claude PR review" -f pr_number=`. The workflow is review-only — Claude can comment but cannot push commits. Requires the `CLAUDE_CODE_OAUTH_TOKEN` secret (generated via `claude setup-token`). + - **Claude PR review** (`.github/workflows/claude-review.yml`) — `anthropics/claude-code-action` runs on every PR open / push, gated on the HEAD commit's author or committer having at least read permission on the repo (catches "admin pushed a fixup onto an external author's PR"). Dependabot is filtered at the workflow level. Claude posts findings as **inline review comments** (tagged `[MUST]` / `[SHOULD]` / `[MAY]` per the prompt template) plus a short sticky verdict summary; inline threads count against the ruleset's `required_review_thread_resolution`, so they block merge until resolved — same mechanism Gemini uses. Manual re-trigger via `@claude` or `/review` in a PR comment from a trusted actor, or via `gh workflow run "Claude PR review" -f pr_number=`. The workflow is review-only — Claude can comment but cannot push commits. Requires the `CLAUDE_CODE_OAUTH_TOKEN` secret (generated via `claude setup-token`). ### Dependabot automation