Add ability to review PRs with Claude

[ssilvert]

Try this on the PR

[ssilvert]

@claude Check this PR to see if it works

[ssilvert]

@claude How does the claude.md file look in this PR

[ssilvert]

@claude Can you give me some suggestions to improve PR reviews? Maybe some changes to claude-review.yml and claude.md?

[claude]

Claude finished @ssilvert's task in 1m 20s —— View job

---

Suggestions to Improve PR Reviews

Below are concrete improvements for both files. I haven't applied any changes — these are suggestions for you to review. Note: I cannot modify files under .github/workflows/ due to GitHub App permissions, so the workflow changes are shown as a proposed diff for you to apply manually.

---

1. .github/workflows/claude-review.yml

Issues with the current workflow:

• Triggers only on issue_comment — reviews never run automatically when a PR is opened or updated, so every review requires a manual @claude comment.
• No concurrency control — pushing several commits in a row will spawn overlapping review runs.
• No paths-ignore — Claude burns time reviewing docs-only, translation, or generated-file PRs.
• No claude_args — you can't pin a model, restrict tools, or pass a custom system prompt.
• The author allowlist is hard-coded to ssilvert. For team use, github.event.comment.author_association (OWNER, MEMBER, COLLABORATOR) scales better.
• No track_progress, so auto-triggered reviews can't post sticky progress comments.

Suggested replacement:

name: Claude Code Review
on:
  pull_request:
    types: [opened, synchronize, ready_for_review]
    paths-ignore:
      - '**/*.md'
      - 'docs/**'
      - '**/messages_*.properties'   # i18n bundles
  issue_comment:
    types: [created]

concurrency:
  group: claude-review-${{ github.event.pull_request.number || github.event.issue.number }}
  cancel-in-progress: true

jobs:
  review:
    if: |
      (github.event_name == 'pull_request' && github.event.pull_request.draft == false) ||
      (github.event_name == 'issue_comment' &&
       github.event.issue.pull_request &&
       contains(github.event.comment.body, '@claude') &&
       (github.event.comment.author_association == 'OWNER' ||
        github.event.comment.author_association == 'MEMBER' ||
        github.event.comment.author_association == 'COLLABORATOR'))
    runs-on: ubuntu-latest
    permissions:
      contents: read
      pull-requests: write
      issues: write
      id-token: write
    steps:
      - uses: actions/checkout@v4
        with:
          fetch-depth: 0

      - uses: anthropics/claude-code-action@v1
        with:
          anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }}
          track_progress: true
          claude_args: '--model claude-opus-4-7'

Optional add-ons worth considering:

• A prompt: input that points reviewers at claude.md explicitly (the action already reads root CLAUDE.md, but a nudge avoids drift if the file is renamed).
• A second job gated on contains(github.event.pull_request.labels.*.name, 'needs-security-review') that runs the /security-review skill.

---

2. claude.md

What's good: it's short, security-focused, and lists concrete "what to flag" items.

What's missing: the file tells Claude what to look for but not how to structure findings, what to skip, or how to verify. As a result reviews tend to be a flat bullet list with no severity, occasional false positives on stylistic nits, and inconsistent depth across PRs.

Suggested additions (drop into claude.md):

## Review output format

Group findings under these headers, in this order. Omit a header if empty:

- **Blockers** — must fix before merge (security, correctness, BC break)
- **Major** — should fix, request changes
- **Minor** — nice to fix, non-blocking
- **Questions** — ask the author, don't assume

For each finding: `path/to/file.java:LINE` + one-sentence why + a suggested fix
(use a fenced ```suggestion block when the fix fits on a few lines).

End every review with a one-line verdict: `Verdict: approve | request-changes | comment`.

## What to skip

- Style nits already covered by Checkstyle / Spotless — the build catches these.
- Generated code (`*-generated.java`, `target/`, `node_modules/`, `dist/`).
- Translation-only changes under `themes/**/messages_*.properties`.
- Pure dependency bumps unless the diff also touches non-pom files.

## Backwards compatibility (high bar in Keycloak)

- Public SPI signatures, REST API shapes, and DB schema are **append-only** between minor versions.
- Renaming/removing a `public` method on an `org.keycloak.*` SPI interface is a blocker
  unless a `@Deprecated` shim is left in place for one release.
- Liquibase changesets are immutable once released — new migrations only, never edit an existing changelog id.

## Testing expectations

- New SPI provider → unit test in the same module + an integration test under `testsuite/integration-arquillian/`.
- New REST endpoint → `AdminClientTest` or `UserResourceTest`-style coverage including a negative permission case.
- Bug fix → a regression test that fails without the fix. Flag PRs that change behavior with no test delta.

## Additional security checks (extend "What to flag")

- SSRF in any code that constructs an outbound HTTP client from user-controlled input
  (look in `broker/`, `services/.../resources/admin/`).
- Open redirect on `redirect_uri` handling — must go through `RedirectUtils`.
- Log injection / sensitive data in logs (tokens, passwords, client secrets) — use
  `KeycloakModelUtils.toString` patterns, never `String.format` with raw token values.
- Time-unsafe equality on secrets — must use `MessageDigest.isEqual` or equivalent.

## Good examples to compare against

When unsure whether a pattern is idiomatic, diff the PR against:

- Admin resource pattern: `services/src/main/java/org/keycloak/services/resources/admin/UserResource.java`
- Provider factory pattern: any `*ProviderFactory` under `services/src/main/java/org/keycloak/`
- Liquibase changeset style: `model/jpa/src/main/resources/META-INF/jpa-changelog-*.xml`

Why each addition pays off:

• Output format + verdict line → reviews become scannable and you can grep merged PRs for Verdict: request-changes to audit.
• "What to skip" → cuts noise on translation/dep-bump PRs, which are a large share of Keycloak traffic.
• Backwards-compat section → BC is the single most common reason a Keycloak PR gets bounced; making it explicit avoids missing it.
• Testing expectations → lets Claude flag "no test added" as a real finding, not a stylistic preference.
• Good examples → grounds Claude in actual code instead of generic best practices, which reduces hallucinated APIs.

---

Quick wins (do these first)

1. Add pull_request trigger + concurrency block to the workflow — biggest UX win.
2. Add the Review output format and What to skip sections to claude.md — biggest signal-to-noise win.
3. Add the Backwards compatibility section — biggest correctness win for Keycloak specifically.

---

• Read current claude-review.yml and claude.md
• Analyze gaps and improvement opportunities
• Draft concrete suggestions with example changes
• Post final recommendations

Happy to apply the claude.md changes if you want — just say the word. The workflow file you'll need to edit yourself (GitHub App perms block me from touching .github/workflows/).
• Branch: claude/pr-13-20260518-1150