From b78b670a72e99d4f349adf5a03847a66ccf7cb44 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 16 Jul 2026 14:51:16 +0000 Subject: [PATCH 1/5] Consolidate AI PR review into orchestrated claude-code-action workflow Replace the per-persona curl pipeline (pr-review.yml + ai-persona-review.yml) with a single claude-review.yml built on anthropics/claude-code-action. An orchestrator (.github/claude-review-prompt.md) fans out to one isolated subagent per review lens (.claude/agents/review-*.md: extensibility, testing, security), then consolidates all findings into ONE sticky comment plus inline comments for important findings only. This ends the churn of a fresh top-level comment per persona on every push, and gives each lens a clean context. Cadence and cost controls: - Auto-review once when a PR opens non-draft or leaves draft; re-review on demand via an @claude comment. No per-push reviews. - concurrency group cancels an in-flight review when a new push/mention lands. - [skip-review] in the PR title (or a skip-review label) opts a PR out of both the Claude review and CodeRabbit; @claude still works as a manual override. - Reuses the ambient GITHUB_TOKEN and existing ANTHROPIC_API_KEY secret. Also fixes CodeRabbit's base_branches to cover the default branch (mainline, which the old config omitted) and quiets it (quiet profile, collapsed walkthrough, incremental reviews, same skip-review override). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017RS1pjrpm7c5AdirmmAtwX --- .claude/agents/review-extensibility.md | 39 +++++ .claude/agents/review-security.md | 39 +++++ .claude/agents/review-testing.md | 39 +++++ .coderabbit.yaml | 20 ++- .github/claude-review-prompt.md | 65 +++++++ .github/workflows/ai-persona-review.yml | 216 ------------------------ .github/workflows/claude-review.yml | 96 +++++++++++ .github/workflows/pr-review.yml | 92 ---------- CLAUDE.md | 2 +- 9 files changed, 292 insertions(+), 316 deletions(-) create mode 100644 .claude/agents/review-extensibility.md create mode 100644 .claude/agents/review-security.md create mode 100644 .claude/agents/review-testing.md create mode 100644 .github/claude-review-prompt.md delete mode 100644 .github/workflows/ai-persona-review.yml create mode 100644 .github/workflows/claude-review.yml delete mode 100644 .github/workflows/pr-review.yml diff --git a/.claude/agents/review-extensibility.md b/.claude/agents/review-extensibility.md new file mode 100644 index 0000000..45c77e5 --- /dev/null +++ b/.claude/agents/review-extensibility.md @@ -0,0 +1,39 @@ +--- +name: review-extensibility +description: Extensibility review lens for McCore PRs — public API/interface stability, generic type contracts, events for downstream consumption, @NotNull/@Nullable, plugin-agnostic boundary (no consumer-plugin references). Returns structured findings to the review orchestrator; never posts comments. +tools: Read, Grep, Glob, Bash +--- + +You are the **extensibility** review lens for a McCore pull request, reviewing as a downstream plugin developer who extends McCore's abstractions. McCore is a shared framework — every public class, interface, and method signature is consumed downstream. You run in an isolated context so your analysis stays focused on this one concern. + +## What to apply + +Apply the checklist in `.claude/commands/review-extensibility.md` — the **Checklist section only**. Ignore that file's "Instructions" section, its "ask the user to paste the diff" step, and its "No extensibility concerns found." ending. Keep the checklist's `Breaking change risk:` judgement in mind, but express it through the findings below rather than as a lead line. Your output format is defined below and the diff is provided to you by the orchestrator. + +## How to review + +1. You are given the PR diff (or the list of changed files) in your prompt. Review **only lines this PR changed** — new/changed public types, method signatures, generic parameters, events, registry points. Do not report pre-existing API shapes in untouched code. +2. **Verify every candidate finding against the actual code in this checkout.** Read the type to confirm visibility, confirm a signature actually changed vs. an overload, confirm removed/renamed generic parameters break binding. Grep for the plugin-agnostic boundary violation the checklist calls out (references to a specific consumer plugin package such as `us.eunoians.*`). Grep for existing callers before claiming a break. Drop anything you cannot confirm. +3. Prefer additive, non-breaking guidance; flag missing `@NotNull`/`@Nullable` on new public surface. + +## What to return + +Return **only** a findings list — no preamble, no summary, no comments posted anywhere. For each confirmed finding, emit one block: + +``` +SEVERITY: IMPORTANT | NIT +LENS: extensibility +FILE: path/to/File.java:line +WHAT: one sentence naming the compatibility break, boundary violation, or missing extension point +WHY: one sentence on how a downstream consumer is affected +FIX: the specific additive change (overload, event, annotation, deprecation path) that resolves it +--- +``` + +`IMPORTANT` = a breaking change to consumed public API, a consumer-plugin reference that breaks the plugin-agnostic boundary, or a missing interception event a downstream plugin needs. `NIT` = missing nullability annotations, minor API-ergonomics polish. If nothing survives verification, return exactly: + +``` +CLEAN +``` + +Your findings go to an orchestrator that dedupes across lenses and posts a single consolidated review. Do not post comments, do not edit files, do not open the PR conversation. diff --git a/.claude/agents/review-security.md b/.claude/agents/review-security.md new file mode 100644 index 0000000..3f362b2 --- /dev/null +++ b/.claude/agents/review-security.md @@ -0,0 +1,39 @@ +--- +name: review-security +description: Security review lens for McCore PRs — DDL/SQL injection, unsafe deserialization, ChatResponse safety, framework-level input handling. Returns structured findings to the review orchestrator; never posts comments. +tools: Read, Grep, Glob, Bash +--- + +You are the **security** review lens for a McCore pull request. McCore is a plugin-agnostic framework library consumed by downstream plugins, so judge framework-level exposure, not any one plugin's threat model. You run in an isolated context so your analysis stays focused on this one concern. + +## What to apply + +Apply the checklist in `.claude/commands/review-security.md` — the **Checklist section only**. Ignore that file's "Instructions" section, its "ask the user to paste the diff" step, its per-file output format, and its "No security concerns found." ending. Your output format is defined below and the diff is provided to you by the orchestrator. + +## How to review + +1. You are given the PR diff (or the list of changed files) in your prompt. Review **only lines this PR changed or directly breaks** — do not report pre-existing issues in untouched code. +2. **Verify every candidate finding against the actual code in this checkout.** Read the file, confirm the behavior really occurs (e.g. concatenated runtime values in DDL vs. `PreparedStatement` parameters, deserialization of untrusted bytes without validation), and get the real `file:line`. Use Read/Grep/Glob freely. Drop anything you cannot confirm. +3. Skip anything the build or a linter already enforces, and skip style nits unrelated to security. + +## What to return + +Return **only** a findings list — no preamble, no summary, no comments posted anywhere. For each confirmed finding, emit one block: + +``` +SEVERITY: IMPORTANT | NIT +LENS: security +FILE: path/to/File.java:line +WHAT: one sentence naming the concrete vulnerability and vector +WHY: one sentence on the impact / why it matters for consumers +FIX: the specific change (parameterization, validation, guard) that resolves it +--- +``` + +`IMPORTANT` = a real injection/deserialization/data-safety hole, or data loss. `NIT` = minor hardening, defense-in-depth. If nothing survives verification, return exactly: + +``` +CLEAN +``` + +Your findings go to an orchestrator that dedupes across lenses and posts a single consolidated review. Do not post comments, do not edit files, do not open the PR conversation. diff --git a/.claude/agents/review-testing.md b/.claude/agents/review-testing.md new file mode 100644 index 0000000..9c6fcd8 --- /dev/null +++ b/.claude/agents/review-testing.md @@ -0,0 +1,39 @@ +--- +name: review-testing +description: Testing review lens for McCore PRs — coverage for new non-Bukkit logic, Given/When/Then @DisplayName convention, ManagedExecutorExtension for cross-thread tests, RegistryResetExtension usage. Returns structured findings to the review orchestrator; never posts comments. +tools: Read, Grep, Glob, Bash +--- + +You are the **testing** review lens for a McCore pull request. McCore is a framework library validated with plain JUnit 5 (MockBukkit only when Bukkit APIs are genuinely needed). You run in an isolated context so your analysis stays focused on this one concern. + +## What to apply + +Apply the checklist in `.claude/commands/review-testing.md` — the **Checklist section only** (including any "do NOT flag" suppression list). Ignore that file's "Instructions" section, its "ask the user to paste the diff" step, its summary-line format, and its "No testing concerns found." ending. Your output format is defined below and the diff is provided to you by the orchestrator. + +## How to review + +1. You are given the PR diff (or the list of changed files) in your prompt. Review **only lines this PR changed** — new framework logic that lacks coverage, new tests with structural problems. Do not report gaps in pre-existing untouched code. +2. **Verify every candidate finding against the actual code in this checkout.** Read the changed production file and Grep the mirrored `src/test/java` path for a corresponding test before claiming coverage is missing. Read the actual test to confirm a real structural issue — e.g. a cross-thread test that hand-rolls an `ExecutorService` instead of using `ManagedExecutorExtension`, or a `@DisplayName` that isn't a full Given/When/Then sentence. Drop anything you cannot confirm. +3. Respect the project's convention that plain JUnit (no MockBukkit) is correct for tests that only construct events, check handler lists, or use `NamespacedKey.fromString()`. + +## What to return + +Return **only** a findings list — no preamble, no summary, no comments posted anywhere. For each confirmed finding, emit one block: + +``` +SEVERITY: IMPORTANT | NIT +LENS: testing +FILE: path/to/File.java:line +WHAT: one sentence naming the gap or structural problem +WHY: one sentence on the risk it leaves uncovered +FIX: the specific test or change that resolves it +--- +``` + +`IMPORTANT` = new non-trivial framework logic with no unit test, or a test that passes for the wrong reason. `NIT` = naming/structure conventions, minor coverage polish. If nothing survives verification, return exactly: + +``` +CLEAN +``` + +Your findings go to an orchestrator that dedupes across lenses and posts a single consolidated review. Do not post comments, do not edit files, do not open the PR conversation. diff --git a/.coderabbit.yaml b/.coderabbit.yaml index e62472a..41ef5a0 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -1,18 +1,24 @@ language: "en" reviews: - profile: "assertive" + profile: "quiet" request_changes_workflow: false high_level_summary: true poem: false - collapse_walkthrough: false + collapse_walkthrough: true auto_review: enabled: true drafts: false + auto_incremental_review: true + # Opt a PR out of auto-review by putting [skip-review] in the title or adding + # the skip-review label — mirrors the Claude review workflow's skip override. + ignore_title_keywords: + - "[skip-review]" + labels: + - "!skip-review" base_branches: - - "master" + - "mainline" - "develop" - - "recode" path_filters: - "!gradle/wrapper/gradle-wrapper.jar" path_instructions: @@ -37,9 +43,9 @@ reviews: by name or imports from a consumer plugin package (e.g., us.eunoians.*). McCore must remain fully plugin-agnostic. - Deprioritize primary review of API extension points and test structure; - if persona automation is unavailable or skipped, include these domains - in the review. + A separate Claude Code review workflow covers API extension points and + test structure once per PR and on demand; keep this review focused on + correctness and avoid duplicating those domains. - path: "src/test/**/*.java" instructions: | Test naming convention — do NOT flag as violations: diff --git a/.github/claude-review-prompt.md b/.github/claude-review-prompt.md new file mode 100644 index 0000000..296b206 --- /dev/null +++ b/.github/claude-review-prompt.md @@ -0,0 +1,65 @@ +# McCore PR Review — Orchestration Protocol + +You are the **review orchestrator** for a McCore pull request. McCore is a plugin-agnostic framework library consumed by downstream plugins, so review with framework stability in mind. Your job is to route, merge, and post — **not** to deep-analyze code yourself. Specialized persona subagents (defined in `.claude/agents/review-*.md`) each review one concern in an isolated context so their analysis stays clean and undiluted. You spawn them, collect their findings, consolidate into ONE review, and post it as a single sticky comment plus inline comments for the most important findings. + +The old workflow posted a fresh top-level comment per persona on every push. Do not recreate that. Everything you post goes into the single tracking/sticky comment and (for Important findings only) inline review comments. Never post additional top-level PR comments. + +## 1. Gather context + +- Run `gh pr view` for the PR title, description, and metadata. +- Run `gh pr diff` for the full diff, and `gh pr diff --name-only` for the changed-file list. +- On a re-review (an `@claude` re-review request), first read the existing sticky/tracking comment to recover the previously-reported findings — you will reconcile against them (see §6). + +## 2. Route to lenses + +Pick the persona subagents whose patterns match the changed files. Match generously — when unsure, include the lens. + +| Lens (subagent) | Apply when the diff touches | +|---|---| +| `review-security` | any `src/main/**/*.java` | +| `review-testing` | any `src/main/**/*.java` | +| `review-extensibility` | `src/**/event/**`, `src/**/registry/**`, `src/**/gui/**`, `src/**/database/**`, or any change to a public type/method signature, generic parameter, or new/changed event | + +If the PR changes only non-code files (docs, workflows, `.md`) and no lens matches, skip straight to §5 and post a one-line "No reviewable code changes" summary. + +## 3. Fan out (one subagent per lens, in parallel) + +Spawn every applicable lens **in a single message with multiple Task calls** so they run concurrently. For each, use the matching `subagent_type` (e.g. `review-security`) and pass in the prompt: + +- The PR diff (or, if very large, the changed-file list plus the diff hunks relevant to that lens). +- A one-line instruction to follow its own agent definition. +- On a re-review only: the list of findings previously reported for that lens, labeled "PREVIOUSLY REPORTED — re-verify each: still open, or resolved?". + +Each subagent returns either `CLEAN` or a list of `SEVERITY/LENS/FILE/WHAT/WHY/FIX` blocks. Collect them all. + +## 4. Consolidate + +- **Discard** any finding without a concrete `file:line` you can point at — the subagents are told to verify, but enforce it here. +- **Dedup across lenses:** when two lenses flag the same `file:line`/issue, keep one finding, take the highest severity, and note the overlapping lenses. +- **Rank:** all `IMPORTANT` findings first, then `NIT`. +- **Cap:** at most **10 findings total** and at most **5 nits** shown; if more nits survived, show the first 5 and append "+N similar nits". Prefer showing Important findings over nits when at the cap. +- **Skipped lens:** if a subagent failed, timed out, or returned malformed output, do not guess its findings — note in the summary which lens was skipped. + +## 5. Post the review + +Update the sticky/tracking comment with this shape: + +- **Verdict line:** e.g. `2 important, 3 nits` — or `No blocking issues found` when clean. +- **Short summary:** 1–3 sentences on the overall shape of the change and which lenses ran. +- **Important findings:** for each, `file:line` — what / why / concrete fix. +- **Nits:** inside a collapsed `
` block. +- On a re-review: a **"Resolved since last review"** list of findings that are now fixed. + +Create an **inline review comment** (`mcp__github_inline_comment__create_inline_comment`) for each Important finding, anchored to its `file:line`. Do **not** create inline comments for nits, and do **not** post any other top-level comment. + +**When clean:** post only the verdict + a one- or two-sentence summary. Never emit per-lens "no concerns found" lines — a clean PR gets one short comment, not one per lens. + +## 6. Re-review convergence + +When re-invoked on a PR that already has a sticky comment: + +- Read the previous findings first and pass them to each lens as "previously reported". +- A previously-reported finding that is now fixed → move it to the "Resolved since last review" list; drop it from the active findings. +- A previously-reported finding that is still open → keep it **verbatim** (same wording, same severity). Do not re-litigate or flip severity on unchanged code. +- Only surface **new IMPORTANT** findings on a re-review — suppress new nits so a one-line fix never spirals into round seven of style comments. +- Stay within the same caps. diff --git a/.github/workflows/ai-persona-review.yml b/.github/workflows/ai-persona-review.yml deleted file mode 100644 index 50c97a3..0000000 --- a/.github/workflows/ai-persona-review.yml +++ /dev/null @@ -1,216 +0,0 @@ -name: AI Persona Review - -# Triggered automatically when the "AI PR Review" prepare-review job finishes, -# OR manually via workflow_dispatch so you can re-run reviews on any open PR -# without pushing a new commit. -# -# Security model: this workflow always executes on the DEFAULT BRANCH and never -# checks out PR-head code, so secrets are safe. The diff is passed in via an -# artifact (workflow_run path) or fetched with gh (dispatch path). -"on": - workflow_run: - workflows: ["AI PR Review"] - types: [completed] - workflow_dispatch: - inputs: - pr_number: - description: "PR number to review" - required: true - type: string - -jobs: - persona-review: - runs-on: ubuntu-latest - # For workflow_run: only proceed when the upstream workflow succeeded. - # For workflow_dispatch: only allow dispatch from the default branch so an - # attacker cannot trigger the job from a fork or feature branch that contains - # modified persona files, which would be checked out and sent to the API. - if: >- - (github.event_name == 'workflow_run' && github.event.workflow_run.conclusion == 'success') || - (github.event_name == 'workflow_dispatch' && github.ref == format('refs/heads/{0}', github.event.repository.default_branch)) - timeout-minutes: 10 - permissions: - pull-requests: write # post PR comments - contents: read # checkout persona prompt files - actions: read # download artifacts from another workflow run - env: - # Change this to target a different Claude model across all persona steps. - ANTHROPIC_MODEL: claude-sonnet-4-6 - - steps: - # Check out ONLY the persona prompt files from the default branch. - # Never check out the PR head — that would expose secrets to untrusted code. - - name: Checkout persona prompts (default branch) - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - with: - ref: ${{ github.event.repository.default_branch }} - sparse-checkout: .claude/commands/ - - # Write a shared helper function used by every persona step. - # It builds the API payload, calls Claude, checks for HTTP/API errors, - # extracts the review text, and writes it to comment.md. - - name: Write Claude helper script - run: | - cat > "$GITHUB_WORKSPACE/claude-helper.sh" << 'HELPER' - call_claude() { - local header="$1" # e.g. "## Extensibility Review" - local prompt_file="$2" # e.g. ".claude/commands/review-extensibility.md" - - jq -n \ - --arg model "$ANTHROPIC_MODEL" \ - --rawfile prompt "$prompt_file" \ - --rawfile diff pr.diff \ - '{"model":$model,"max_tokens":4096,"messages":[{"role":"user","content":($prompt+"\n\n\n"+$diff+"\n")}]}' \ - > payload.json - - HTTP_CODE=$(curl -sS --connect-timeout 10 --max-time 120 \ - -o response.json -w "%{http_code}" \ - -H "x-api-key: $ANTHROPIC_API_KEY" \ - -H "anthropic-version: 2023-06-01" \ - -H "content-type: application/json" \ - -d @payload.json \ - https://api.anthropic.com/v1/messages) - - if [ "$HTTP_CODE" != "200" ]; then - ERROR_MSG=$(jq -r '.error.message // "unknown error"' response.json 2>/dev/null || echo "unknown error") - echo "::error::Claude API returned HTTP $HTTP_CODE: $ERROR_MSG" - return 1 - fi - - REVIEW=$(jq -r '.content[0].text // empty' response.json) - if [ -z "$REVIEW" ]; then - echo "::error::Claude API returned HTTP 200 but .content[0].text is absent" - jq . response.json >&2 - return 1 - fi - - printf '%s\n\n%s\n' "$header" "$REVIEW" > comment.md - } - HELPER - chmod +x "$GITHUB_WORKSPACE/claude-helper.sh" - - # ── workflow_run path ──────────────────────────────────────────────────── - # The prepare-review job uploaded pr.diff + review-meta.json as an - # artifact. Download it here. If the artifact is absent (prepare-review - # was skipped because no personas were needed), continue-on-error keeps - # the job alive so "Parse metadata" can emit proceed=false gracefully. - - name: Download review artifact (workflow_run) - if: github.event_name == 'workflow_run' - id: download - uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4.3.0 - continue-on-error: true - with: - name: pr-review-inputs - run-id: ${{ github.event.workflow_run.id }} - github-token: ${{ secrets.GITHUB_TOKEN }} - - # ── workflow_dispatch path ─────────────────────────────────────────────── - # Fetch the diff with gh and re-run the same domain-detection logic that - # detect-changes uses, so we know which personas to invoke. - - name: Fetch diff and detect domains (workflow_dispatch) - if: github.event_name == 'workflow_dispatch' - env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - run: | - PR="${{ inputs.pr_number }}" - gh pr diff "$PR" > pr.diff - - # Note: avoid naming variables after shell builtins. - api=false; tests_changed=false; security=false - while IFS= read -r f; do - [[ "$f" =~ src/.*/event/.*\.java ]] && api=true - [[ "$f" =~ src/.*/registry/.*\.java ]] && api=true - [[ "$f" =~ src/.*/gui/.*\.java ]] && api=true - [[ "$f" =~ src/.*/database/.*\.java ]] && api=true - [[ "$f" =~ src/test/.*\.java ]] && tests_changed=true - [[ "$f" =~ src/.*/.*\.java ]] && [[ ! "$f" =~ src/test/ ]] && api=true - [[ "$f" =~ src/.*/.*\.java ]] && security=true - [[ "$f" =~ src/.*/.*\.java ]] && tests_changed=true - done < <(gh pr view "$PR" --json files --jq '.files[].path') - - jq -n \ - --arg pr "$PR" \ - --arg api "$api" \ - --arg tests_changed "$tests_changed" \ - --arg security "$security" \ - '{"pr_number":$pr,"needs_api":$api,"needs_test":$tests_changed,"needs_security":$security}' \ - > review-meta.json - - # ── Common: parse metadata ─────────────────────────────────────────────── - # If review-meta.json is absent (artifact was not uploaded because no - # personas were triggered for this PR), emit proceed=false and skip all - # persona steps cleanly. - - name: Parse metadata - id: meta - run: | - if [ ! -f review-meta.json ]; then - echo "review-meta.json not found — no personas needed for this PR." - echo "proceed=false" >> "$GITHUB_OUTPUT" - exit 0 - fi - { - echo "proceed=true" - echo "pr_number=$(jq -r '.pr_number' review-meta.json)" - echo "needs_api=$(jq -r '.needs_api' review-meta.json)" - echo "needs_test=$(jq -r '.needs_test' review-meta.json)" - echo "needs_security=$(jq -r '.needs_security // "false"' review-meta.json)" - } >> "$GITHUB_OUTPUT" - - # Truncate very large diffs before feeding them to any persona. - # 128 KB of unified diff is already a very large PR; beyond that the - # context window adds cost without proportional review quality. - - name: Truncate large diffs - if: steps.meta.outputs.proceed == 'true' - run: | - MAX_BYTES=131072 - DIFF_SIZE=$(wc -c < pr.diff) - if [ "$DIFF_SIZE" -gt "$MAX_BYTES" ]; then - echo "Diff is ${DIFF_SIZE} bytes — truncating to ${MAX_BYTES} bytes." - head -c "$MAX_BYTES" pr.diff > pr.diff.tmp - printf '\n\n[Diff truncated — original size: %d bytes]\n' "$DIFF_SIZE" >> pr.diff.tmp - mv pr.diff.tmp pr.diff - fi - - # ── Persona: Extensibility ─────────────────────────────────────────────── - # Runs when event, registry, GUI, database, or any other non-test Java - # source files change. McCore is a framework — almost every change is a - # potential API contract change. - - name: Extensibility Persona - if: steps.meta.outputs.proceed == 'true' && steps.meta.outputs.needs_api == 'true' - env: - ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - PR_NUMBER: ${{ steps.meta.outputs.pr_number }} - run: | - source "$GITHUB_WORKSPACE/claude-helper.sh" - call_claude "## Extensibility Review" ".claude/commands/review-extensibility.md" - gh pr comment "$PR_NUMBER" --body-file comment.md - - # ── Persona: Testing ───────────────────────────────────────────────────── - # Runs when any Java source file changes. - - name: Testing Persona - if: steps.meta.outputs.proceed == 'true' && steps.meta.outputs.needs_test == 'true' - env: - ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - PR_NUMBER: ${{ steps.meta.outputs.pr_number }} - run: | - source "$GITHUB_WORKSPACE/claude-helper.sh" - call_claude "## Testing Review" ".claude/commands/review-testing.md" - gh pr comment "$PR_NUMBER" --body-file comment.md - - # ── Persona: Security ──────────────────────────────────────────────────── - # Runs when any Java source file changes. Checks for SQL injection in DAO - # methods, unsafe DDL in UpdateTableFunction, MiniMessage injection via - # externally-sourced strings, ChatResponse safety, and onClick() return - # value correctness. - - name: Security Persona - if: steps.meta.outputs.proceed == 'true' && steps.meta.outputs.needs_security == 'true' - env: - ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - PR_NUMBER: ${{ steps.meta.outputs.pr_number }} - run: | - source "$GITHUB_WORKSPACE/claude-helper.sh" - call_claude "## Security Review" ".claude/commands/review-security.md" - gh pr comment "$PR_NUMBER" --body-file comment.md diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml new file mode 100644 index 0000000..a09cb15 --- /dev/null +++ b/.github/workflows/claude-review.yml @@ -0,0 +1,96 @@ +name: Claude PR Review + +# Consolidated AI PR review. A single orchestrator run (claude-code-action) fans +# out to one persona subagent per review lens, then posts ONE sticky comment. +# Replaces the old pr-review.yml + ai-persona-review.yml pipeline, which posted a +# fresh top-level comment per persona on every push. +on: + pull_request: + types: [opened, ready_for_review] + branches: [mainline, develop] + issue_comment: + types: [created] + pull_request_review_comment: + types: [created] + +# A new push or @claude mention on the same PR cancels the in-flight review. +concurrency: + group: claude-review-${{ github.event.pull_request.number || github.event.issue.number }} + cancel-in-progress: true + +env: + # Single model knob. Sonnet 5 carries $2/$10-per-MTok intro pricing through + # 2026-08-31 (then $3/$15, and ~30% more tokens per request than the 4.6 + # tokenizer) — revisit the model/cost tradeoff before September 2026. + CLAUDE_MODEL: claude-sonnet-5 + +permissions: + contents: read # bump to write only if @claude should ever push fix commits + pull-requests: write + issues: write + +jobs: + # One automated review when a PR opens non-draft or leaves draft. No per-push + # reviews — re-review on demand with an @claude comment. + auto-review: + if: >- + github.event_name == 'pull_request' && + github.event.pull_request.draft == false && + github.event.pull_request.head.repo.full_name == github.repository && + !contains(github.event.pull_request.title, '[skip-review]') && + !contains(github.event.pull_request.labels.*.name, 'skip-review') + runs-on: ubuntu-latest + timeout-minutes: 25 + steps: + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + with: + fetch-depth: 0 + # claude-code-action has no prior pinned SHA in this repo; pin to a + # verified commit once one is chosen, matching the checkout convention above. + - uses: anthropics/claude-code-action@v1 + with: + anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }} + # Reuse the ambient token (as the old pipeline did); comments post as + # github-actions[bot]. No Claude GitHub App install required. + github_token: ${{ secrets.GITHUB_TOKEN }} + # REQUIRED alongside a custom prompt, or the sticky comment silently + # never posts (anthropics/claude-code-action#1108). + track_progress: true + use_sticky_comment: true + prompt: | + REPO: ${{ github.repository }} + PR NUMBER: ${{ github.event.pull_request.number }} + + This is the initial automated review of this PR. Read + .github/claude-review-prompt.md in this checkout and execute the + orchestrated review protocol it defines. + claude_args: | + --model ${{ env.CLAUDE_MODEL }} + --max-turns 40 + --allowedTools "Task,Read,Grep,Glob,Bash(gh pr diff:*),Bash(gh pr view:*),mcp__github_inline_comment__create_inline_comment" + + # On-demand: an @claude mention in a PR comment or review comment. Tag mode + # (no prompt) — claude-code-action handles the sticky comment natively here. + on-demand: + if: >- + (github.event_name == 'issue_comment' && + github.event.issue.pull_request != null && + contains(github.event.comment.body, '@claude')) || + (github.event_name == 'pull_request_review_comment' && + contains(github.event.comment.body, '@claude')) + runs-on: ubuntu-latest + timeout-minutes: 25 + steps: + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + with: + fetch-depth: 0 + - uses: anthropics/claude-code-action@v1 + with: + anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }} + github_token: ${{ secrets.GITHUB_TOKEN }} + use_sticky_comment: true + claude_args: | + --model ${{ env.CLAUDE_MODEL }} + --max-turns 40 + --allowedTools "Task,Read,Grep,Glob,Bash(gh pr diff:*),Bash(gh pr view:*),mcp__github_inline_comment__create_inline_comment" + --append-system-prompt "If asked to review or re-review this PR, follow .github/claude-review-prompt.md including its re-review convergence protocol. Otherwise answer the request directly." diff --git a/.github/workflows/pr-review.yml b/.github/workflows/pr-review.yml deleted file mode 100644 index dc0cc35..0000000 --- a/.github/workflows/pr-review.yml +++ /dev/null @@ -1,92 +0,0 @@ -name: AI PR Review - -on: - pull_request: - types: [opened, synchronize, reopened] - branches: [master, develop, recode] - -jobs: - # ------------------------------------------------------------------ - # Step 1: Detect which domains were touched so we only invoke the - # relevant personas. McCore has two: extensibility and testing. - # ------------------------------------------------------------------ - detect-changes: - runs-on: ubuntu-latest - outputs: - needs_api: ${{ steps.detect.outputs.api }} - needs_test: ${{ steps.detect.outputs.test }} - needs_security: ${{ steps.detect.outputs.security }} - steps: - - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - with: - fetch-depth: 0 - - - id: detect - name: Detect changed file domains - run: | - BASE="${{ github.event.pull_request.base.sha }}" - HEAD="${{ github.event.pull_request.head.sha }}" - CHANGED=$(git diff --name-only "$BASE" "$HEAD" 2>/dev/null || git diff --name-only origin/${{ github.base_ref }}...HEAD) - - echo "$CHANGED" - - api=false - test=false - security=false - - while IFS= read -r f; do - [[ "$f" =~ src/.*/event/.*\.java ]] && api=true - [[ "$f" =~ src/.*/registry/.*\.java ]] && api=true - [[ "$f" =~ src/.*/gui/.*\.java ]] && api=true - [[ "$f" =~ src/.*/database/.*\.java ]] && api=true - [[ "$f" =~ src/test/.*\.java ]] && test=true - [[ "$f" =~ src/.*/.*\.java ]] && [[ ! "$f" =~ src/test/ ]] && api=true - [[ "$f" =~ src/.*/.*\.java ]] && security=true - done <<< "$CHANGED" - - echo "api=$api" >> "$GITHUB_OUTPUT" - echo "test=$test" >> "$GITHUB_OUTPUT" - echo "security=$security" >> "$GITHUB_OUTPUT" - - # ------------------------------------------------------------------ - # Step 2: Generate diff and upload it with metadata as an artifact - # so the secret-holding workflow_run job never executes PR head code. - # ------------------------------------------------------------------ - prepare-review: - needs: detect-changes - runs-on: ubuntu-latest - if: | - needs.detect-changes.outputs.needs_api == 'true' || - needs.detect-changes.outputs.needs_test == 'true' || - needs.detect-changes.outputs.needs_security == 'true' - permissions: - contents: read - steps: - - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - with: - fetch-depth: 0 - - - name: Generate diff and save metadata - run: | - BASE="${{ github.event.pull_request.base.sha }}" - HEAD="${{ github.event.pull_request.head.sha }}" - git diff "$BASE" "$HEAD" > pr.diff 2>/dev/null || \ - git diff origin/${{ github.base_ref }}...HEAD > pr.diff - - cat > review-meta.json << 'EOF_META' - { - "pr_number": "${{ github.event.pull_request.number }}", - "needs_api": "${{ needs.detect-changes.outputs.needs_api }}", - "needs_test": "${{ needs.detect-changes.outputs.needs_test }}", - "needs_security": "${{ needs.detect-changes.outputs.needs_security }}" - } - EOF_META - - - name: Upload review inputs - uses: actions/upload-artifact@v4 - with: - name: pr-review-inputs - path: | - pr.diff - review-meta.json - retention-days: 1 diff --git a/CLAUDE.md b/CLAUDE.md index cc0cc8e..998b8c1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -469,6 +469,6 @@ After any commit or PR that introduces one of the following, **update `CLAUDE.md | New database pattern added | `CLAUDE.md` + `database-system.mdc` | | New public API pattern or breaking-change rule | `persona-extensibility.mdc` + `.claude/commands/review-extensibility.md` | | New test structural pattern or anti-pattern | `persona-testing.mdc` + `.claude/commands/review-testing.md` | -| CI review file-pattern for a new domain | `.github/workflows/pr-review.yml` detect-changes step | +| CI review routing, persona set, or signal rule changed | `.github/claude-review-prompt.md` + `.claude/agents/review-*.md` (+ `.github/workflows/claude-review.yml` for triggers/model) | These files are the project's living technical contract — stale steering files produce stale AI output. From 27b362a9ff424eb55a8fe4cd5d02aa5d6affca6c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 16 Jul 2026 15:16:20 +0000 Subject: [PATCH 2/5] Add author_association gate and use skip-claude-review label Gate the on-demand @claude job to OWNER/MEMBER/COLLABORATOR only, preventing untrusted commenters from triggering reviews on fork PRs. Change skip label from skip-review to skip-claude-review for more targeted control (CodeRabbit keeps its own skip-review label). Co-Authored-By: Claude Opus 4.6 Claude-Session: https://claude.ai/code/session_017RS1pjrpm7c5AdirmmAtwX --- .github/workflows/claude-review.yml | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index a09cb15..99dd2de 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -38,7 +38,7 @@ jobs: github.event.pull_request.draft == false && github.event.pull_request.head.repo.full_name == github.repository && !contains(github.event.pull_request.title, '[skip-review]') && - !contains(github.event.pull_request.labels.*.name, 'skip-review') + !contains(github.event.pull_request.labels.*.name, 'skip-claude-review') runs-on: ubuntu-latest timeout-minutes: 25 steps: @@ -73,11 +73,12 @@ jobs: # (no prompt) — claude-code-action handles the sticky comment natively here. on-demand: if: >- - (github.event_name == 'issue_comment' && - github.event.issue.pull_request != null && - contains(github.event.comment.body, '@claude')) || - (github.event_name == 'pull_request_review_comment' && - contains(github.event.comment.body, '@claude')) + ((github.event_name == 'issue_comment' && + github.event.issue.pull_request != null && + contains(github.event.comment.body, '@claude')) || + (github.event_name == 'pull_request_review_comment' && + contains(github.event.comment.body, '@claude'))) && + contains(fromJson('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association) runs-on: ubuntu-latest timeout-minutes: 25 steps: From 41f79e589a2cf0ec10fc7ecaceac2d075dc913d3 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 16 Jul 2026 15:46:02 +0000 Subject: [PATCH 3/5] Harden review workflow: fork guard, SHA-pin actions - Add fork-check pre-step to on-demand job that rejects @claude mentions on fork PRs (prevents attacker-controlled checkout with secrets in scope) - SHA-pin anthropics/claude-code-action to fffa3483 (v1) in both jobs, matching the existing checkout SHA-pin convention - Remove stale TODO comment about pinning claude-code-action Co-Authored-By: Claude Opus 4.6 Claude-Session: https://claude.ai/code/session_017RS1pjrpm7c5AdirmmAtwX --- .github/workflows/claude-review.yml | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 99dd2de..eea9916 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -45,9 +45,7 @@ jobs: - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: fetch-depth: 0 - # claude-code-action has no prior pinned SHA in this repo; pin to a - # verified commit once one is chosen, matching the checkout convention above. - - uses: anthropics/claude-code-action@v1 + - uses: anthropics/claude-code-action@fffa3483bb78e47a0c02fdcb79109570f1e1a617 # v1 with: anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }} # Reuse the ambient token (as the old pipeline did); comments post as @@ -85,7 +83,16 @@ jobs: - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: fetch-depth: 0 - - uses: anthropics/claude-code-action@v1 + - name: Reject fork PRs + id: fork-check + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + PR_NUMBER="${{ github.event.issue.number || github.event.pull_request.number }}" + IS_FORK="$(gh pr view "$PR_NUMBER" --repo "${{ github.repository }}" --json isCrossRepository -q '.isCrossRepository')" + echo "is_fork=$IS_FORK" >> "$GITHUB_OUTPUT" + - uses: anthropics/claude-code-action@fffa3483bb78e47a0c02fdcb79109570f1e1a617 # v1 + if: steps.fork-check.outputs.is_fork != 'true' with: anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }} github_token: ${{ secrets.GITHUB_TOKEN }} From 7f767806e464329c25a235806373f54b2c3995cc Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 16 Jul 2026 15:52:13 +0000 Subject: [PATCH 4/5] Revert SHA-pinning of claude-code-action to use mutable tag Co-Authored-By: Claude Opus 4.6 Claude-Session: https://claude.ai/code/session_017RS1pjrpm7c5AdirmmAtwX --- .github/workflows/claude-review.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index eea9916..e070fdd 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -45,7 +45,7 @@ jobs: - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: fetch-depth: 0 - - uses: anthropics/claude-code-action@fffa3483bb78e47a0c02fdcb79109570f1e1a617 # v1 + - uses: anthropics/claude-code-action@v1 with: anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }} # Reuse the ambient token (as the old pipeline did); comments post as @@ -91,7 +91,7 @@ jobs: PR_NUMBER="${{ github.event.issue.number || github.event.pull_request.number }}" IS_FORK="$(gh pr view "$PR_NUMBER" --repo "${{ github.repository }}" --json isCrossRepository -q '.isCrossRepository')" echo "is_fork=$IS_FORK" >> "$GITHUB_OUTPUT" - - uses: anthropics/claude-code-action@fffa3483bb78e47a0c02fdcb79109570f1e1a617 # v1 + - uses: anthropics/claude-code-action@v1 if: steps.fork-check.outputs.is_fork != 'true' with: anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }} From cf64ddeade534b315888aef64b06f4e01c9e257a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 16 Jul 2026 15:55:11 +0000 Subject: [PATCH 5/5] Fix on-demand checkout ref and testing agent tools - Checkout PR head ref for on-demand reviews so Read/Grep/Glob see the PR code instead of the default branch - Remove Bash from testing agent tools (only needs Read/Grep/Glob) Co-Authored-By: Claude Opus 4.6 Claude-Session: https://claude.ai/code/session_017RS1pjrpm7c5AdirmmAtwX --- .claude/agents/review-testing.md | 2 +- .github/workflows/claude-review.yml | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.claude/agents/review-testing.md b/.claude/agents/review-testing.md index 9c6fcd8..28e84f2 100644 --- a/.claude/agents/review-testing.md +++ b/.claude/agents/review-testing.md @@ -1,7 +1,7 @@ --- name: review-testing description: Testing review lens for McCore PRs — coverage for new non-Bukkit logic, Given/When/Then @DisplayName convention, ManagedExecutorExtension for cross-thread tests, RegistryResetExtension usage. Returns structured findings to the review orchestrator; never posts comments. -tools: Read, Grep, Glob, Bash +tools: Read, Grep, Glob --- You are the **testing** review lens for a McCore pull request. McCore is a framework library validated with plain JUnit 5 (MockBukkit only when Bukkit APIs are genuinely needed). You run in an isolated context so your analysis stays focused on this one concern. diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index e070fdd..f3bea4c 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -83,6 +83,7 @@ jobs: - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: fetch-depth: 0 + ref: refs/pull/${{ github.event.issue.number || github.event.pull_request.number }}/head - name: Reject fork PRs id: fork-check env: