From 494838722c460155e0174ebb501434bd3aa5c5ff Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 16 Jul 2026 14:50:32 +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), 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, uncluttered 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 quiets CodeRabbit (quiet profile, collapsed walkthrough, incremental reviews, same skip-review override) and removes the orphaned pr-review.py. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017RS1pjrpm7c5AdirmmAtwX --- .claude/agents/review-extensibility.md | 39 ++++ .claude/agents/review-gui-ux.md | 39 ++++ .claude/agents/review-security.md | 39 ++++ .claude/agents/review-server-owner.md | 39 ++++ .claude/agents/review-testing.md | 39 ++++ .coderabbit.yaml | 18 +- .github/claude-review-prompt.md | 67 ++++++ .github/scripts/pr-review.py | 299 ------------------------ .github/workflows/ai-persona-review.yml | 245 ------------------- .github/workflows/claude-review.yml | 94 ++++++++ .github/workflows/pr-review.yml | 103 -------- CLAUDE.md | 2 +- 12 files changed, 370 insertions(+), 653 deletions(-) create mode 100644 .claude/agents/review-extensibility.md create mode 100644 .claude/agents/review-gui-ux.md create mode 100644 .claude/agents/review-security.md create mode 100644 .claude/agents/review-server-owner.md create mode 100644 .claude/agents/review-testing.md create mode 100644 .github/claude-review-prompt.md delete mode 100644 .github/scripts/pr-review.py 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 000000000..cb276cf08 --- /dev/null +++ b/.claude/agents/review-extensibility.md @@ -0,0 +1,39 @@ +--- +name: review-extensibility +description: Extensibility review lens for McRPG PRs — public API stability, events at interception points, @NotNull/@Nullable contracts, registry extension points, backward compatibility for third-party addons. Returns structured findings to the review orchestrator; never posts comments. +tools: Read, Grep, Glob, Bash +--- + +You are the **extensibility** review lens for a McRPG pull request, reviewing as a third-party addon developer who hooks into McRPG's public API. 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, 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 (public vs internal), confirm a signature actually changed vs. an overload, confirm an event genuinely isn't fired where an addon would need to intercept. Grep for existing callers/overloads 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 or missing extension point +WHY: one sentence on how a third-party addon is affected +FIX: the specific additive change (overload, event, annotation) that resolves it +--- +``` + +`IMPORTANT` = a breaking change to consumed public API, or a missing interception event a third party would reasonably need. `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-gui-ux.md b/.claude/agents/review-gui-ux.md new file mode 100644 index 000000000..8c3c28b74 --- /dev/null +++ b/.claude/agents/review-gui-ux.md @@ -0,0 +1,39 @@ +--- +name: review-gui-ux +description: GUI/UX review lens for McRPG PRs — inventory slot ergonomics, navigation, click-hint and localization conventions, player feedback, palette usage. Reviews as a player who never read the source. Returns structured findings to the review orchestrator; never posts comments. +tools: Read, Grep, Glob, Bash +--- + +You are the **GUI/UX** review lens for a McRPG pull request, reviewing as a player who never read the source code and only experiences the inventory GUIs. 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-gui-ux.md` — the **Checklist section only**. Ignore that file's "Instructions" section, its "ask the user to paste the diff" step, and its "No GUI/UX 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** in `src/**/gui/**` and `resources/localization/**`. Do not report pre-existing GUI patterns in untouched code. +2. **Verify every candidate finding against the actual code in this checkout.** Read the slot/GUI class or the locale YAML to confirm the behavior (e.g. an unsafe `onClick` that returns `false`, a missing click-hint, a raw color instead of a palette placeholder). Confirm player-facing text is routed through the localization manager. Drop anything you cannot confirm. +3. Focus on what a player would actually notice or be confused by; skip internal-only concerns other lenses own. + +## 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: gui-ux +FILE: path/to/File.java:line +WHAT: one sentence naming the ergonomics/feedback/localization problem +WHY: one sentence on how it affects the player experience +FIX: the specific change that resolves it +--- +``` + +`IMPORTANT` = a broken or confusing interaction, item-loss risk, or unlocalized player-facing text. `NIT` = palette/click-hint convention polish, minor wording. 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 000000000..fb570854e --- /dev/null +++ b/.claude/agents/review-security.md @@ -0,0 +1,39 @@ +--- +name: review-security +description: Security review lens for McRPG PRs — player-exploitable injection (MiniMessage, performCommand), permission bypass, SQL/DDL injection. Returns structured findings to the review orchestrator; never posts comments. +tools: Read, Grep, Glob, Bash +--- + +You are the **security** review lens for a McRPG pull request. 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. Those are for the interactive slash command; 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 (not just that a name looks suspicious), and get the real `file:line`. Use Read/Grep/Glob freely. Drop anything you cannot confirm against real code. +3. Skip anything a linter or the build 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 attack vector +WHY: one sentence on the impact / why it matters +FIX: the specific change (method, imports, guard) that resolves it +--- +``` + +`IMPORTANT` = a real exploit path a normal player could take, or data loss / API breakage. `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-server-owner.md b/.claude/agents/review-server-owner.md new file mode 100644 index 000000000..32c063c4d --- /dev/null +++ b/.claude/agents/review-server-owner.md @@ -0,0 +1,39 @@ +--- +name: review-server-owner +description: Server-owner review lens for McRPG PRs — YAML config readability, sane defaults, reload-safety, permission-node design, migration/config-version needs. Reviews as a server admin editing config. Returns structured findings to the review orchestrator; never posts comments. +tools: Read, Grep, Glob, Bash +--- + +You are the **server-owner** review lens for a McRPG pull request, reviewing as a server administrator who edits the YAML config and `plugin.yml` and never reads the Java source. 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-server-owner.md` — the **Checklist section only**. Ignore that file's "Instructions" section, its "ask the user to paste the diff" step, its `Migration required` / `Reload-safe` footer format, and its "No server owner 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** in `resources/**/*.yml`, `plugin.yml`, and `configuration/**`. Do not report pre-existing config in untouched sections. +2. **Verify every candidate finding against the actual code in this checkout.** Read the YAML to confirm a missing comment/default/duration-format, and read the corresponding `*ConfigFile` / loader to confirm reload-safety or a real migration need (new required key without a `config-version` bump). Do not flag a `config-version` bump for purely additive optional keys unless the loader requires it. Drop anything you cannot confirm. +3. Server-admin config values and Bukkit enum values are the owner's domain — judge readability and safety, not code style. + +## 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: server-owner +FILE: path/to/file.yml:line +WHAT: one sentence naming the config/readability/reload/migration problem +WHY: one sentence on how it bites a server owner +FIX: the specific change that resolves it +--- +``` + +`IMPORTANT` = a missing migration for a required key, an unsafe reload, a footgun default, or a permission node that grants too much. `NIT` = missing inline comments, duration-format hints, minor readability. 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 000000000..dfb8e7501 --- /dev/null +++ b/.claude/agents/review-testing.md @@ -0,0 +1,39 @@ +--- +name: review-testing +description: Testing review lens for McRPG PRs — coverage gaps for new non-Bukkit logic, TimeProvider usage, McRPGBaseTest/MockBukkit structure, naming conventions. Returns structured findings to the review orchestrator; never posts comments. +tools: Read, Grep, Glob, Bash +--- + +You are the **testing** review lens for a McRPG pull request. 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 its "Known Infrastructure Guarantees — 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 production 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 search for a corresponding test (Grep the mirrored `src/test/java` path) before claiming coverage is missing. Read the actual test to confirm a structural issue rather than inferring it. Drop anything you cannot confirm. +3. Respect the suppression list in the checklist — do not flag infrastructure the project guarantees. + +## 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, non-Bukkit 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 6418e741a..fea50f455 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -1,14 +1,21 @@ 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" - "develop" @@ -37,9 +44,10 @@ reviews: - Generic type parameters (e.g., T extends McPlugin) are intentional API design inherited from McCore; do not suggest removing them. - Deprioritize primary review of GUI slot design, server config readability, - 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 GUI slot design, server + config readability, API extension points, and test structure once per PR + and on demand; keep this review focused on correctness and avoid + duplicating those domains. chat: auto_reply: true diff --git a/.github/claude-review-prompt.md b/.github/claude-review-prompt.md new file mode 100644 index 000000000..098dfee31 --- /dev/null +++ b/.github/claude-review-prompt.md @@ -0,0 +1,67 @@ +# McRPG PR Review — Orchestration Protocol + +You are the **review orchestrator** for a McRPG pull request. 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/**`, or any change to a public type/method signature or a new/changed event | +| `review-gui-ux` | `src/**/gui/**`, `resources/localization/**` | +| `review-server-owner` | `resources/**/*.yml`, `plugin.yml`, `src/**/configuration/**` | + +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/scripts/pr-review.py b/.github/scripts/pr-review.py deleted file mode 100644 index b64e20d08..000000000 --- a/.github/scripts/pr-review.py +++ /dev/null @@ -1,299 +0,0 @@ -#!/usr/bin/env python3 -""" -AI-powered PR review script for McRPG. - -Reads environment variables set by the GitHub Actions workflow to determine -which personas to run, calls the Anthropic API for each active persona, and -writes a consolidated Markdown review comment to review_comment.txt. - -Environment variables (set by pr-review.yml): - ANTHROPIC_API_KEY — required; API key for the Anthropic Messages API - NEEDS_GUI — "true" if GUI/localization files changed - NEEDS_CONFIG — "true" if YAML config files changed - NEEDS_API — "true" if event/registry files changed - NEEDS_TEST — "true" if src/main Java files changed (check for missing tests) - REVIEW_MODEL — optional; overrides model (default: claude-haiku-4-5-20251001) - DIFF_FILE — optional; path to the diff file (default: /tmp/pr.diff) - MAX_DIFF_CHARS — optional; character cap on diff sent to API (default: 30000) -""" - -import json -import os -import re -import sys -import time -import urllib.request -import urllib.error - -# --------------------------------------------------------------------------- -# Configuration -# --------------------------------------------------------------------------- - -API_URL = "https://api.anthropic.com/v1/messages" -API_VERSION = "2023-06-01" -DEFAULT_MODEL = "claude-haiku-4-5-20251001" -MAX_OUTPUT_TOKENS = 1024 -DEFAULT_MAX_DIFF_CHARS = 30_000 -RETRY_DELAYS = [2, 4, 8] # seconds between retries - -SCRIPT_DIR = os.path.dirname(os.path.abspath(__file__)) -REPO_ROOT = os.path.abspath(os.path.join(SCRIPT_DIR, "..", "..")) - -PERSONAS = { - "gui": { - "label": "GUI / UX", - "file": ".cursor/rules/persona-gui-ux.mdc", - "env": "NEEDS_GUI", - }, - "config": { - "label": "Server Owner", - "file": ".cursor/rules/persona-server-owner.mdc", - "env": "NEEDS_CONFIG", - }, - "api": { - "label": "Third-Party Extensibility", - "file": ".cursor/rules/persona-extensibility.mdc", - "env": "NEEDS_API", - }, - "test": { - "label": "Testing", - "file": ".cursor/rules/persona-testing.mdc", - "env": "NEEDS_TEST", - }, -} - -# --------------------------------------------------------------------------- -# Helpers -# --------------------------------------------------------------------------- - -_SENSITIVE_PATTERNS: list[tuple[re.Pattern, str]] = [ - # Bare (unquoted) key forms: api_key=value, api-key: value, apiKey: value - (re.compile(r"(?i)(api[_-]?key\s*[:=]\s*)\S+"), r"\1[REDACTED_API_KEY]"), - (re.compile(r"(?i)(secret\s*[:=]\s*)\S+"), r"\1[REDACTED_SECRET]"), - (re.compile(r"(?i)(token\s*[:=]\s*)\S+"), r"\1[REDACTED_TOKEN]"), - (re.compile(r"(?i)(password\s*[:=]\s*)\S+"), r"\1[REDACTED_PASSWORD]"), - (re.compile(r"(?i)(private[_-]?key\s*[:=]\s*)\S+"), r"\1[REDACTED_PRIVATE_KEY]"), - # Quoted-key JSON/YAML forms: "apiKey": "value", 'api-key' = 'value', etc. - # Opening quote on the key is required; closing key-quote and value-quote are optional. - (re.compile(r"""(?i)([\"']api[_-]?key[\"']?\s*[:=]\s*[\"']?)\S+"""), r"\1[REDACTED_API_KEY]"), - (re.compile(r"""(?i)([\"']secret[\"']?\s*[:=]\s*[\"']?)\S+"""), r"\1[REDACTED_SECRET]"), - (re.compile(r"""(?i)([\"']token[\"']?\s*[:=]\s*[\"']?)\S+"""), r"\1[REDACTED_TOKEN]"), - (re.compile(r"""(?i)([\"']password[\"']?\s*[:=]\s*[\"']?)\S+"""), r"\1[REDACTED_PASSWORD]"), - (re.compile(r"""(?i)([\"']private[_-]?key[\"']?\s*[:=]\s*[\"']?)\S+"""), r"\1[REDACTED_PRIVATE_KEY]"), - (re.compile(r"-----BEGIN (?:RSA |EC |DSA |OPENSSH )?PRIVATE KEY-----.*?-----END (?:RSA |EC |DSA |OPENSSH )?PRIVATE KEY-----", re.DOTALL), "[REDACTED_PRIVATE_KEY]"), - # Common provider-prefix tokens (most specific — checked first) - (re.compile(r"(? str: - """Redact common sensitive patterns from a diff before sending to the API.""" - for pattern, replacement in _SENSITIVE_PATTERNS: - diff = pattern.sub(replacement, diff) - return diff - - -def read_file(path: str) -> str: - try: - with open(path, encoding="utf-8") as f: - return f.read() - except OSError: - return "" - - -def strip_mdc_frontmatter(content: str) -> str: - """Remove YAML frontmatter (--- ... ---) from .mdc files.""" - if content.startswith("---"): - end = content.find("\n---", 3) - if end != -1: - return content[end + 4:].lstrip("\n") - return content - - -def call_api(api_key: str, model: str, system: str, user: str) -> str | None: - """ - POST to the Anthropic Messages API. Returns the text response or None on - permanent failure. Retries transient errors with exponential backoff. - """ - headers = { - "Content-Type": "application/json", - "x-api-key": api_key, - "anthropic-version": API_VERSION, - } - payload = json.dumps({ - "model": model, - "max_tokens": MAX_OUTPUT_TOKENS, - "system": system, - "messages": [{"role": "user", "content": user}], - }).encode("utf-8") - - for attempt, delay in enumerate([0, *RETRY_DELAYS]): - if delay: - print(f" Retrying in {delay}s (attempt {attempt + 1})...", file=sys.stderr) - time.sleep(delay) - try: - req = urllib.request.Request(API_URL, data=payload, headers=headers, method="POST") - with urllib.request.urlopen(req, timeout=60) as resp: - data = json.loads(resp.read().decode("utf-8")) - if ( - not isinstance(data, dict) - or not isinstance(data.get("content"), list) - or not data["content"] - ): - return None - texts = [ - item["text"] - for item in data["content"] - if isinstance(item, dict) and isinstance(item.get("text"), str) - ] - if not texts: - return None - return "".join(texts) - except urllib.error.HTTPError as e: - body = e.read().decode("utf-8", errors="replace") - print(f" HTTP {e.code}: {body[:200]}", file=sys.stderr) - if e.code in (429, 529) or 500 <= e.code < 600: # rate limit / overload / transient 5xx — retry - continue - return None # other HTTP errors are permanent - except (urllib.error.URLError, TimeoutError, json.JSONDecodeError) as e: - print(f" Network/parse error: {e}", file=sys.stderr) - continue # retry on transient errors - - print(" All retries exhausted.", file=sys.stderr) - return None - - -def is_no_findings(text: str) -> bool: - """Heuristic: true when the model reported no concerns. - - Requires one of the no-concern phrases to appear as a standalone sentence - with no other substantive content around it. - """ - no_concern_phrases = [ - "no gui/ux concerns", - "no server owner concerns", - "no extensibility concerns", - "no testing concerns", - "no concerns found", - "nothing to flag", - ] - if len(text) >= 300: - return False - sentences = [s.strip() for s in re.split(r"[.!?]\s*", text) if s.strip()] - for phrase in no_concern_phrases: - for sentence in sentences: - if sentence.lower() == phrase: - # Ensure no other sentence has substantive content - others = [s for s in sentences if s.lower() != phrase] - _conj_prefix = re.compile(r"^(and|but|however|also)[,;:\s-]*", re.I) - if not any( - _conj_prefix.sub("", s).strip() - for s in others - ): - return True - return False - - -# --------------------------------------------------------------------------- -# Main -# --------------------------------------------------------------------------- - -def main() -> None: - api_key = os.environ.get("ANTHROPIC_API_KEY", "") - if not api_key: - print("ANTHROPIC_API_KEY not set — skipping AI review.", file=sys.stderr) - with open("review_comment.txt", "w") as f: - f.write("") - return - - model = os.environ.get("REVIEW_MODEL", DEFAULT_MODEL) - diff_file = os.environ.get("DIFF_FILE", "/tmp/pr.diff") - try: - max_diff_chars = int(os.environ.get("MAX_DIFF_CHARS", DEFAULT_MAX_DIFF_CHARS)) - if max_diff_chars <= 0: - raise ValueError("must be positive") - except (ValueError, TypeError): - print("WARNING: MAX_DIFF_CHARS is not a valid positive integer — using default.", file=sys.stderr) - max_diff_chars = DEFAULT_MAX_DIFF_CHARS - - # Load the diff - diff = read_file(diff_file) - if not diff: - print("Diff is empty — nothing to review.", file=sys.stderr) - with open("review_comment.txt", "w") as f: - f.write("") - return - - if len(diff) > max_diff_chars: - diff = diff[:max_diff_chars] + f"\n\n[Diff truncated at {max_diff_chars} characters]" - - # Load the project CLAUDE.md for domain context - claude_md = read_file(os.path.join(REPO_ROOT, "CLAUDE.md")) - - sections: list[str] = [] - all_clear: list[str] = [] - - for persona in PERSONAS.values(): - if os.environ.get(persona["env"], "false").lower() != "true": - continue - - persona_path = os.path.join(REPO_ROOT, persona["file"]) - persona_content = strip_mdc_frontmatter(read_file(persona_path)) - if not persona_content: - print(f" Persona file not found: {persona_path} — skipping.", file=sys.stderr) - continue - - label = persona["label"] - print(f"Running {label} review...", file=sys.stderr) - - system_prompt = ( - "You are an expert code reviewer.\n\n" - "## Project Context\n\n" - + claude_md - + "\n\n## Review Persona\n\n" - + persona_content - ) - user_prompt = ( - "Review the following pull request diff using your persona and checklist.\n\n" - "```diff\n" + sanitize_diff(diff) + "\n```" - ) - - response = call_api(api_key, model, system_prompt, user_prompt) - if response is None: - sections.append(f"## {label} Review\n\n> ⚠️ Review failed due to API error.") - continue - - if is_no_findings(response): - all_clear.append(label) - else: - sections.append(f"## {label} Review\n\n{response.strip()}") - - # Build the output comment - if not sections and not all_clear: - comment = "" - elif not sections: - clear_list = ", ".join(all_clear) - comment = f"### AI Review\n\n✅ No concerns found ({clear_list})." - else: - parts = ["### AI Review\n"] - parts.extend(sections) - if all_clear: - parts.append(f"\n---\n✅ No concerns from: {', '.join(all_clear)}.") - comment = "\n\n".join(parts) - - with open("review_comment.txt", "w", encoding="utf-8") as f: - f.write(comment) - - print(f"Done. {len(sections)} section(s) with findings, {len(all_clear)} all-clear.", file=sys.stderr) - - -if __name__ == "__main__": - main() diff --git a/.github/workflows/ai-persona-review.yml b/.github/workflows/ai-persona-review.yml deleted file mode 100644 index f7ed91787..000000000 --- a/.github/workflows/ai-persona-review.yml +++ /dev/null @@ -1,245 +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. - # The explicit ref ensures workflow_dispatch from any branch still reads - # persona files from the default branch, not the dispatching ref. - - name: Checkout persona prompts (default branch) - uses: actions/checkout@v4 - 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. "## GUI/UX Review" - local prompt_file="$2" # e.g. ".claude/commands/review-gui-ux.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@v4 - 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. - gui=false; config=false; api=false; tests_changed=false; security=false - while IFS= read -r f; do - [[ "$f" =~ src/.*/gui/.*\.java ]] && gui=true - [[ "$f" =~ resources/localization/ ]] && gui=true - [[ "$f" =~ resources/.*\.yml ]] && config=true - [[ "$f" =~ configuration/.*\.java ]] && config=true - [[ "$f" =~ src/.*/event/.*\.java ]] && api=true - [[ "$f" =~ src/.*/registry/.*\.java ]] && api=true - [[ "$f" =~ src/main/.*\.java ]] && tests_changed=true - [[ "$f" =~ src/main/.*\.java ]] && security=true - done < <(gh pr view "$PR" --json files --jq '.files[].path') - - jq -n \ - --arg pr "$PR" \ - --arg gui "$gui" \ - --arg config "$config" \ - --arg api "$api" \ - --arg tests_changed "$tests_changed" \ - --arg security "$security" \ - '{"pr_number":$pr,"needs_gui":$gui,"needs_config":$config,"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_gui=$(jq -r '.needs_gui' review-meta.json)" - echo "needs_config=$(jq -r '.needs_config' 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: GUI/UX ────────────────────────────────────────────────────── - # Runs when GUI Java files or localization YAML files were changed. - - name: GUI/UX Persona - if: steps.meta.outputs.proceed == 'true' && steps.meta.outputs.needs_gui == '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 "## GUI/UX Review" ".claude/commands/review-gui-ux.md" - gh pr comment "$PR_NUMBER" --body-file comment.md - - # ── Persona: Server Owner ──────────────────────────────────────────────── - # Runs when config YAMLs, plugin.yml, or ConfigFile route constants change. - - name: Server Owner Persona - if: steps.meta.outputs.proceed == 'true' && steps.meta.outputs.needs_config == '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 "## Server Owner Review" ".claude/commands/review-server-owner.md" - gh pr comment "$PR_NUMBER" --body-file comment.md - - # ── Persona: Extensibility ─────────────────────────────────────────────── - # Runs when event or registry Java files 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 main 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 main Java source file changes. Checks for MiniMessage/ - # Adventure API injection, command injection via performCommand(), - # permission bypass, and SQL/data injection. - - 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 000000000..3bbfbe0f2 --- /dev/null +++ b/.github/workflows/claude-review.yml @@ -0,0 +1,94 @@ +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: [master, develop, recode] + 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@v4 + with: + fetch-depth: 0 + - 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@v4 + 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 32b13b8bb..000000000 --- a/.github/workflows/pr-review.yml +++ /dev/null @@ -1,103 +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. Each output is the string "true" or "false". - # ------------------------------------------------------------------ - detect-changes: - runs-on: ubuntu-latest - outputs: - needs_gui: ${{ steps.detect.outputs.gui }} - needs_config: ${{ steps.detect.outputs.config }} - needs_api: ${{ steps.detect.outputs.api }} - needs_test: ${{ steps.detect.outputs.test }} - needs_security: ${{ steps.detect.outputs.security }} - steps: - - uses: actions/checkout@v4 - 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" - - gui=false - config=false - api=false - test=false - security=false - - while IFS= read -r f; do - [[ "$f" =~ src/.*/gui/.*\.java ]] && gui=true - [[ "$f" =~ resources/localization/ ]] && gui=true - [[ "$f" =~ resources/.*\.yml ]] && config=true - [[ "$f" =~ configuration/.*\.java ]] && config=true - [[ "$f" =~ src/.*/event/.*\.java ]] && api=true - [[ "$f" =~ src/.*/registry/.*\.java ]] && api=true - [[ "$f" =~ src/main/.*\.java ]] && test=true - [[ "$f" =~ src/main/.*\.java ]] && security=true - done <<< "$CHANGED" - - echo "gui=$gui" >> "$GITHUB_OUTPUT" - echo "config=$config" >> "$GITHUB_OUTPUT" - 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_gui == 'true' || - needs.detect-changes.outputs.needs_config == 'true' || - 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@v4 - 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_gui": "${{ needs.detect-changes.outputs.needs_gui }}", - "needs_config": "${{ needs.detect-changes.outputs.needs_config }}", - "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 5d796f816..205396d15 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -911,7 +911,7 @@ After any commit or PR that introduces one of the following, **update `CLAUDE.md | New error handling anti-pattern found (swallowed exception, bad logging) | `persona-error-handling.mdc` + `.claude/commands/review-error-handling.md` + `core.mdc` | | New performance anti-pattern found (hot path, unbounded collection, leak) | `persona-performance.mdc` + `.claude/commands/review-performance.md` | | New concurrency anti-pattern found (thread boundary, race, future handling) | `persona-concurrency.mdc` + `.claude/commands/review-concurrency.md` + `core.mdc` | -| 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) | | Quest board system changed (new condition, distribution type, template feature) | `CLAUDE.md` Quest Board System section + `quest-board-system.mdc` | | Quest chain system changed (new trigger type, repeat mode enforced, chain event added) | `CLAUDE.md` Quest Chain System terminology + `chain-system-backlog.md` | | Mana balance parameters changed (pool size, regen rate, bucket ranges) | `CLAUDE.md` Mana Balance Philosophy section + `mana-balance-philosophy.mdc` + `core.mdc` | From 861e65a40b9cc6f26a12fcac6601a57fe95c8bab Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 16 Jul 2026 15:16:18 +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 3bbfbe0f2..a5abeb9ea 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: @@ -71,11 +71,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 59fd651ad238410e924c5de4b7e17b94111a802d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 16 Jul 2026 15:46:04 +0000 Subject: [PATCH 3/5] Harden review workflow: fork guard, SHA-pin all 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 actions/checkout to 11bd7190 (v4.2.2), matching McCore - SHA-pin anthropics/claude-code-action to fffa3483 (v1) in both jobs Co-Authored-By: Claude Opus 4.6 Claude-Session: https://claude.ai/code/session_017RS1pjrpm7c5AdirmmAtwX --- .github/workflows/claude-review.yml | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index a5abeb9ea..60f54ba9b 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -42,10 +42,10 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 25 steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: fetch-depth: 0 - - 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 @@ -80,10 +80,19 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 25 steps: - - uses: actions/checkout@v4 + - 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 d5614c1ffcca0fd12f7e18a4bc553758a6f1113f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 16 Jul 2026 15:52:11 +0000 Subject: [PATCH 4/5] Revert SHA-pinning of GitHub Actions to use mutable tags Co-Authored-By: Claude Opus 4.6 Claude-Session: https://claude.ai/code/session_017RS1pjrpm7c5AdirmmAtwX --- .github/workflows/claude-review.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 60f54ba9b..b77ef6d0e 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -42,10 +42,10 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 25 steps: - - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + - uses: actions/checkout@v4 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 @@ -80,7 +80,7 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 25 steps: - - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + - uses: actions/checkout@v4 with: fetch-depth: 0 - name: Reject fork PRs @@ -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 f6f5d1d9a3abc1a13cd51d09c9b231b0d49def6d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 16 Jul 2026 15:55:10 +0000 Subject: [PATCH 5/5] Fix on-demand checkout ref and agent scope alignment - Checkout PR head ref for on-demand reviews so Read/Grep/Glob see the PR code instead of the default branch (issue_comment events default to the base branch checkout) - Remove Bash from testing agent tools (only needs Read/Grep/Glob) - Fix server-owner scope to match routing table (src/**/configuration/**) Co-Authored-By: Claude Opus 4.6 Claude-Session: https://claude.ai/code/session_017RS1pjrpm7c5AdirmmAtwX --- .claude/agents/review-server-owner.md | 2 +- .claude/agents/review-testing.md | 2 +- .github/workflows/claude-review.yml | 1 + 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.claude/agents/review-server-owner.md b/.claude/agents/review-server-owner.md index 32c063c4d..aad472aaf 100644 --- a/.claude/agents/review-server-owner.md +++ b/.claude/agents/review-server-owner.md @@ -12,7 +12,7 @@ Apply the checklist in `.claude/commands/review-server-owner.md` — the **Check ## 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** in `resources/**/*.yml`, `plugin.yml`, and `configuration/**`. Do not report pre-existing config in untouched sections. +1. You are given the PR diff (or the list of changed files) in your prompt. Review **only lines this PR changed** in `resources/**/*.yml`, `plugin.yml`, and `src/**/configuration/**`. Do not report pre-existing config in untouched sections. 2. **Verify every candidate finding against the actual code in this checkout.** Read the YAML to confirm a missing comment/default/duration-format, and read the corresponding `*ConfigFile` / loader to confirm reload-safety or a real migration need (new required key without a `config-version` bump). Do not flag a `config-version` bump for purely additive optional keys unless the loader requires it. Drop anything you cannot confirm. 3. Server-admin config values and Bukkit enum values are the owner's domain — judge readability and safety, not code style. diff --git a/.claude/agents/review-testing.md b/.claude/agents/review-testing.md index dfb8e7501..fc9595891 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 McRPG PRs — coverage gaps for new non-Bukkit logic, TimeProvider usage, McRPGBaseTest/MockBukkit structure, naming conventions. 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 McRPG pull request. 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 b77ef6d0e..dc4d4019d 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -83,6 +83,7 @@ jobs: - uses: actions/checkout@v4 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: