diff --git a/.agents/plugins/marketplace.json b/.agents/plugins/marketplace.json index bdd2996..49d31dc 100644 --- a/.agents/plugins/marketplace.json +++ b/.agents/plugins/marketplace.json @@ -5,6 +5,7 @@ { "name": "act-plugin-dev", "source": { "source": "local", "path": "./plugins/act-plugin-dev" }, "policy": { "installation": "AVAILABLE", "authentication": "ON_INSTALL" }, "category": "Development" }, { "name": "act-platform-engineering", "source": { "source": "local", "path": "./plugins/act-platform-engineering" }, "policy": { "installation": "AVAILABLE", "authentication": "ON_INSTALL" }, "category": "Operations" }, { "name": "act-work-tracking", "source": { "source": "local", "path": "./plugins/act-work-tracking" }, "policy": { "installation": "AVAILABLE", "authentication": "ON_INSTALL" }, "category": "Workflow" }, - { "name": "act-gitlab-ci", "source": { "source": "local", "path": "./plugins/act-gitlab-ci" }, "policy": { "installation": "AVAILABLE", "authentication": "ON_INSTALL" }, "category": "Engineering" } + { "name": "act-gitlab-ci", "source": { "source": "local", "path": "./plugins/act-gitlab-ci" }, "policy": { "installation": "AVAILABLE", "authentication": "ON_INSTALL" }, "category": "Engineering" }, + { "name": "code-reviews", "source": { "source": "local", "path": "./plugins/code-reviews" }, "policy": { "installation": "AVAILABLE", "authentication": "ON_INSTALL" }, "category": "Engineering" } ] } diff --git a/.agents/skills/glab/SKILL.md b/.agents/skills/glab/SKILL.md new file mode 100644 index 0000000..2994560 --- /dev/null +++ b/.agents/skills/glab/SKILL.md @@ -0,0 +1,253 @@ +--- +name: glab +description: > + GitLab CLI (glab) for working with GitLab from the command line. Read this + skill before running any `glab` or GitLab API command — it applies to every + GitLab operation, whether reading or writing (for example merge requests, + issues, work items, discussions and threaded replies, comments, CI/CD + pipelines, releases, packages, members, and project settings). Whenever a + task touches GitLab in any way, consult this skill first so you use the + correct, safe command on the first try. Prefer glab over raw API calls for + all GitLab operations. +--- + +# GitLab CLI (glab) + +`glab` is pre-configured and available in your environment. Use it for all +GitLab operations. Run `glab --help` for detailed flag information. + +## Quick reference + +```shell +# Issues +glab issue view +glab issue list --label "bug,priority::1" +glab issue create --title "title" --description "$(cat /tmp/desc.md)" +glab issue note -m "comment text" + +# Merge requests +glab mr create --push --title "fix: title" --description "$(cat /tmp/desc.md)" +glab mr view +glab mr list --assignee +glab mr update --description "$(cat /tmp/desc.md)" +glab mr note create -m "comment text" + +# CI/CD +glab ci status +glab ci status --output json +glab ci list +glab ci get --merge-request --with-job-details +glab ci get --pipeline-id --output json +glab ci retry +glab api projects/:id/jobs//trace + +# Machine-readable output +glab mr list --output json | jq '.[].title' +``` + +**Templates:** Check `.gitlab/merge_request_templates/` and +`.gitlab/issue_templates/` for project-specific templates. + +**References:** Always use full URLs in note/comment bodies (e.g. +`https://gitlab.com/org/project/-/issues/123`) instead of short references +(`#123`, `!456`). This applies to issues, merge requests, epics, and so on. +Short refs resolve against project context and render as literal text on +group-level items (epics, group work items); full URLs expand everywhere. + +## Comments and discussions + +Use the `mr note` subcommands (`create`, `resolve`, `reopen`); flags on the +root `glab mr note` command are deprecated. + +### Short, inline bodies — pass `-m` + +```shell +glab issue note -m "comment text" +glab mr note create -m "comment text" +glab incident note -m "comment text" + +# Cross-project +glab mr note create -m "..." --repo group/project +``` + +### Long or Markdown bodies — pipe to stdin (preferred for MR notes) + +`glab mr note create` reads the body from stdin when its input is a pipe. +This avoids shell-quoting pitfalls (backticks, `$`, backslashes) and is the +safest pattern for non-interactive use. + +```shell +# From a file +glab mr note create < /tmp/body.md + +# Inline literal multi-line body — quoted heredoc, no shell expansion inside +glab mr note create << 'EOF' +Your **markdown** comment. +Code blocks and `inline code`, $variables, and \backslashes are all literal. +EOF +``` + +`glab issue note` and `glab incident note` do **not** read stdin. For long +bodies on those commands, use `glab api` with `-F body=@file` (see +[Content-type guidance](#content-type-guidance)) or inline a quoted heredoc +into `-m`: + +```shell +glab issue note -m "$(cat << 'EOF' +Your **markdown** comment. +Code blocks and `inline code` are safe. +EOF +)" +``` + +For descriptions on `glab issue create` / `glab mr create` / `glab mr update`, +inline a quoted heredoc into `--description`, or for very large or reusable +bodies write to a file and use `--description "$(cat /tmp/desc.md)"`. + +### Threaded replies on merge requests + +`glab mr note create` supports `--reply ` for replying inside +an MR thread. The value can be the full discussion ID or a unique prefix of +at least 8 characters. + +Diff comments accept a single line (`--line 42`), a range (`--line 10:15`), +a removed line (`--old-line 7`), or no line for a file-level comment. + +```shell +glab mr note create --reply -m "I agree!" +glab mr note create --file main.go --line 42 -m "Needs refactoring" +glab mr note create --file main.go --line 10:15 -m "Extract this block" +glab mr note create --file main.go --old-line 7 -m "Why was this removed?" +glab mr note create --file main.go -m "General comment on this file" +glab mr note create -m "LGTM" --unique # idempotent: skip if same body exists +``` + +`glab mr note resolve` / `reopen` take the MR identifier followed by the +discussion identifier. The identifier can be a discussion ID (full 40-char +hex or 8+ char prefix) or a note ID (integer; the parent discussion is +looked up automatically): + +```shell +glab mr note resolve +glab mr note resolve # integer note ID also works +glab mr note reopen +``` + +### Threaded replies on issues, incidents, and work items + +The CLI does not wrap threaded replies for these, so you fall back to +`glab api`. **For any non-trivial body, write it to a file and post the file** +rather than inlining rich Markdown — inlined backticks, `$`, newlines, and a +leading `@` all break (see [Content-type guidance](#content-type-guidance)): + +```shell +# Discover the discussion ID +glab api projects/:id/issues//discussions \ + | jq '.[] | {id, body: .notes[0].body}' + +# Build the body in a file, then post it with -F body=@file +cat > /tmp/reply.md << 'EOF' +@user — here's the result, with `code`, a $variable, and an emoji ✅. +EOF +glab api projects/:id/issues//discussions//notes \ + -F body=@/tmp/reply.md +``` + +For a short, plain reply you can still inline it with `-f body="reply text"`. + +## API calls + +`glab api` auto-prepends `/api/v4/`. Use relative paths: + +```shell +glab api user # NOT /api/v4/user +glab api projects/:id/merge_requests +glab api projects/:id/issues | jq '.[0]' +``` + +When using `-f` for PUT/POST, pass simple `key=value` pairs. Array bracket +syntax like `ids[]=1` is not supported: + +```shell +glab api projects/:id/merge_requests/:iid -X PUT -f "assignee_id=1" +``` + +### Content-type guidance + +```shell +# -f / --raw-field — literal string value +glab api projects/:id/issues/:iid/notes -f body="comment text" + +# -F / --field — reads @file as a string. The leading @ means "read this +# file", so only pass a real path here. A literal body that starts with @ +# (e.g. "@user thanks") must NOT go through -F — it would be read as a +# filename. Use -f for literal inline text, or write the body to a file and +# point -F at the file (recommended for rich/markdown bodies). +glab api projects/:id/issues/:iid/notes -F body=@/tmp/comment.md + +# --input — raw request body from a file (or '-' for stdin). Does NOT set +# Content-Type. Without the header, JSON endpoints return HTTP 415. +glab api projects/:id/issues/:iid/notes \ + --input /tmp/body.json \ + -H "Content-Type: application/json" +``` + +### Arrays and nested objects + +`-F` / `--field` parses a value that starts with `[` or `{` as JSON, so arrays +and nested objects go inline without a file. Placeholders are expanded inside +the JSON. Invalid JSON returns an error rather than being sent as a string. + +```shell +# Array of strings +glab api -X PUT projects/:id -F 'topics=["my-topic","GitLab"]' + +# Nested object, with a placeholder expanded inside it +glab api projects/:id/merge_requests/:iid/discussions -X POST \ + -F body="looks good" \ + -F 'position={"position_type":"text","new_path":"main.go","new_line":42}' + +# Empty array clears a field +glab api -X PUT projects/:id -F 'topics=[]' +``` + +`-f` / `--raw-field` never parses JSON: a bracketed value like +`-f 'scopes=[api,read_api]'` is sent as the literal string. Use `-F` with real +JSON for arrays. On GET and DELETE requests, and whenever `--input` is used, +`-F` arrays are serialized as repeated `key[]=` query parameters. + +## Common mistakes + +- **`-m` is required on `note` commands** — without it, `glab issue note` and + `glab incident note` open `$EDITOR` (which hangs in non-interactive + environments). `glab mr note create` falls back to reading stdin on a pipe, + but still opens `$EDITOR` on a TTY. +- **Use `glab mr note create`, not `glab mr note -m`** — the `--message`, + `--unique`, `--resolve`, and `--unresolve` flags on the root `glab mr note` + command are deprecated. Use the `create`, `resolve`, and `reopen` + subcommands instead. +- **Editor-opening flags are unsafe in agent environments** — avoid + `--description "-"` on `issue create` / `mr create` / `mr update` and + avoid omitting `-m` on `note` commands. Pass an explicit value or pipe + from stdin instead. +- **`glab issue note` and `glab incident note` only post root-level + comments** — use `glab mr note create --reply` for MRs, or + `glab api .../discussions//notes` for issues/incidents (write the body + to a file and pass `-F body=@file` for anything non-trivial). +- **`--input` requires an explicit `Content-Type` header** — `glab api + --input file.json` sends raw bytes without setting Content-Type, causing + HTTP 415. Add `-H "Content-Type: application/json"` or use `-f` / `-F` + instead. +- **`glab ci retry` takes a job ID, not a pipeline ID** — to retry an + entire pipeline, use `glab api projects/:id/pipelines//retry -X POST`. +- **`glab ci trace` streams** — it blocks until the job finishes. For + agents, use `glab ci get` for pipeline state or + `glab api projects/:id/jobs//trace` to fetch a finished log. +- **`glab ci view` is interactive** — terminal UI that blocks. Use + `glab ci status` or `glab ci get` for pipeline state instead. +- **Always `--push` on `glab mr create`** — without it the remote branch + may not exist and MR creation fails. +- **No `--state` on `mr list`** — use `--all`, `--merged`, or `--closed`. +- **No `--body` flag** — `--body` is a `gh` flag. `glab` uses `--description`. +- **Labels** — `--label` to add, `--unlabel` to remove. Scoped labels like + `status::doing` auto-replace within their scope. diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index cbbe5be..2b7f7d4 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -128,7 +128,7 @@ "source": "./plugins/act-gitlab-ci", "displayName": "ACT GitLab CI/CD", "description": "GitLab CI/CD and GitLab tooling for Claude Code. Six skills covering running Claude Code as a CI job across the Claude API, Amazon Bedrock and Vertex providers; the GitLab MCP server with its full tool catalogue and version requirements; the glab CLI; CI troubleshooting; and pipeline standards translated to GitLab and marked as derived. Ships a GitLab MCP server configuration, a pipeline security review agent, and a zero-dependency pipeline checker. Relevant to anyone writing a .gitlab-ci.yml, wiring Claude into a pipeline, connecting to GitLab over MCP, or reviewing a pipeline for credential and scan compliance.", - "version": "0.2.0", + "version": "0.2.1", "author": { "name": "Daniel Bodnar" }, @@ -157,6 +157,42 @@ ] } } + }, + { + "name": "code-reviews", + "source": "./plugins/code-reviews", + "displayName": "Code Reviews", + "description": "Automated code review as skills, not a runner. One methodology skill carries the review rubric - the defect checklist, the standing false-positive list, three severity tiers, and the bar that a finding must name a concrete failure scenario and cite path:line - and reads REVIEW.md, ACT_CODE_REVIEW.md, CLAUDE.md and AGENTS.md as layered guidance on any host or provider. A second skill installs that review into a harness that already exists: Anthropic's managed Code Review, GitHub Actions with claude-code-action, the GitLab-maintained Claude Code CI integration, local hooks and the built-in review command, GitHub Copilot instructions files, or docker-agent, Codex and Copilot CLI. Ships no runtime and invents no configuration syntax. Relevant to anyone reviewing a pull or merge request, automating review in a pipeline, tuning what a reviewer flags, or configuring Copilot code review.", + "version": "0.2.0", + "author": { + "name": "Daniel Bodnar" + }, + "license": "LicenseRef-ACT-Internal", + "category": "engineering", + "keywords": [ + "code-review", + "pull-requests", + "merge-requests", + "github", + "gitlab", + "copilot", + "ci-cd", + "git-hooks", + "automation", + "quality" + ], + "relevance": { + "topic": "code review of pull requests and merge requests", + "signals": { + "filesRead": [ + "**/REVIEW.md", + "**/ACT_CODE_REVIEW.md", + "**/.github/instructions/**", + "**/.github/workflows/*review*.yml", + "**/hooks/pre-push*" + ] + } + } } ] } diff --git a/.github/plugin/marketplace.json b/.github/plugin/marketplace.json index 3025c75..4b66039 100644 --- a/.github/plugin/marketplace.json +++ b/.github/plugin/marketplace.json @@ -6,6 +6,7 @@ { "name": "act-plugin-dev", "description": "Create and review portable agent plugins.", "version": "0.2.0", "source": "./plugins/act-plugin-dev", "category": "Development" }, { "name": "act-platform-engineering", "description": "Assess and operate PostgreSQL, ZFS, Linux, Proxmox VE, and observability systems.", "version": "0.2.0", "source": "./plugins/act-platform-engineering", "category": "Operations" }, { "name": "act-work-tracking", "description": "Draft Zoho Projects work and engineering status reports.", "version": "0.2.0", "source": "./plugins/act-work-tracking", "category": "Workflow" }, - { "name": "act-gitlab-ci", "description": "Build, review, and troubleshoot GitLab CI/CD integrations.", "version": "0.2.0", "source": "./plugins/act-gitlab-ci", "category": "Engineering" } + { "name": "act-gitlab-ci", "description": "Build, review, and troubleshoot GitLab CI/CD integrations.", "version": "0.2.1", "source": "./plugins/act-gitlab-ci", "category": "Engineering" }, + { "name": "code-reviews", "description": "Perform high-quality code review on any host, and install it into GitHub, GitLab, or local harnesses.", "version": "0.2.0", "source": "./plugins/code-reviews", "category": "Engineering" } ] } diff --git a/CLAUDE.md b/CLAUDE.md index 436c42d..f38c7a8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -97,5 +97,5 @@ Do not "fix" these without asking; they are recorded gaps, not oversights. - `docs/assets/` holds an **invented placeholder** wordmark. No ACT brand assets exist. - `.gitlab/` is empty. No ACT GitLab CI conventions were available to base a pipeline on. - `plugins/gitlab-standards/` is a `claude plugin init` scaffold, correctly left unregistered. -- `plugins/code-review/`, `plugins/standards/`, `plugins/git-workflows/` are empty shells. +- `plugins/standards/` and `plugins/git-workflows/` are empty shells. - `LicenseRef-ACT-Internal` is a provisional identifier; ACT's licensing posture is unconfirmed. diff --git a/README.md b/README.md index 45c8d26..bfeb72c 100644 --- a/README.md +++ b/README.md @@ -100,6 +100,7 @@ copilot plugin install act-plugin-dev@actdata-plugins | **[`act-platform-engineering`](plugins/act-platform-engineering/)**
Operations | Assessment and operations for PostgreSQL, ZFS, Linux hosts and Proxmox VE. | 23 skills · 7 agents · 9 commands | | **[`act-work-tracking`](plugins/act-work-tracking/)**
Workflow | Zoho Projects work tracking and operations reporting. | 6 skills · 1 agent · 3 commands | | **[`act-gitlab-ci`](plugins/act-gitlab-ci/)**
Engineering | GitLab CI/CD jobs, MCP, authentication, troubleshooting, and pipeline standards. | 10 skills · 1 agent · 3 commands · 1 MCP | +| **[`code-reviews`](plugins/code-reviews/)**
Engineering | The code review methodology as a skill, plus installation into an existing harness: managed Code Review, GitHub Actions, GitLab CI, local hooks, or Copilot instructions. No runtime. | 2 skills · 10 references · 7 templates | ### Not yet shipped @@ -108,7 +109,6 @@ therefore not installable. That is deliberate — an unfinished plugin should no | Directory | State | |---|---| -| `code-review` | Empty shell. | | `standards` | Empty shell. | | `git-workflows` | Empty shell. | diff --git a/docs/decisions/0005-mr-review-engine-agnostic.md b/docs/decisions/0005-mr-review-engine-agnostic.md new file mode 100644 index 0000000..3abdd2c --- /dev/null +++ b/docs/decisions/0005-mr-review-engine-agnostic.md @@ -0,0 +1,104 @@ +# 5. Ship automated code review as an engine-agnostic contract in a standalone code-reviews plugin + +- **Status:** Superseded by [0006](0006-review-skills-over-harness.md) +- **Date:** 2026-08-15 + +## Context + +The repository was asked for automated code review of GitLab merge requests: an agent runs in a +CI pipeline on every submitted MR and posts feedback comparable to GitHub Copilot's or +claude-code-action's pull-request reviews, with Docker's `docker-agent` as the runner. The +requirement then widened along two axes: the review must also run **without docker-agent** (a +plain pipeline, a git hook, a normal editor session) and **beyond GitLab** (GitHub Copilot's +native code review, and Codex/Copilot hosts, which this marketplace already publishes to via +tri-runtime manifests). + +Two structural questions followed: + +1. **Where does it live?** ADR 0003 split the operational bundle into three plugins by topic. + The capability was first built as an extension of `act-gitlab-ci`; once it spanned GitHub, + GitLab, and local surfaces, that home was revisited the same day and the capability moved to + its own plugin before anything merged. +2. **What is the unit of reuse?** A docker-agent job, a Claude Code job, a pre-push hook, and a + Copilot instructions file cannot share code. They can share judgment. + +Two facts constrain any GitLab design: + +- `CI_JOB_TOKEN` cannot create notes or discussions on a merge request, so posting review + comments requires a user-provisioned project access token. +- The diff under review is untrusted input to an unattended model; whatever runs the review is a + prompt-injection target and must be treated as such. + +## Options considered + +| Option | Assessment | +|---|---| +| **Extend act-gitlab-ci** | The original build target: the primary surface is GitLab CI and the plugin owns the CI knowledge. Rejected on revisit: the capability ships GitHub artifacts (`.github/instructions/`), a host-neutral rubric, and local-agent surfaces — a `.gitlab`-named plugin carrying them misleads, and the review machinery shares no files with the pipeline tooling. | +| **A standalone `code-reviews` plugin** | Chosen. The contract is the unit of cohesion, the plugin is host-neutral by construction, and cross-plugin file references (forbidden by `CONTRIBUTING.md`) never arise because the review bundle is self-contained. | +| **The agent posts its own comments (fetch toolset, GitLab MCP, or glab)** | Rejected. It hands the GitLab token to a process parsing untrusted input; the discussions API's position contract is exacting and a model gets it wrong at a steady rate; the HTTP MCP server authenticates over OAuth and is unusable in CI; and none of it is testable without live GitLab. | +| **A two-stage contract: engines produce findings JSON, a deterministic script posts** | Chosen. The rubric plus findings schema (`review-rubric.md`) is the stable center; engines (docker-agent, claude, codex, copilot, or any command) are adapters; one wrapper owns every side effect and is pinned by a fixture test suite. | +| **Claude Code as the only engine** | Rejected. It collapses the provider-agnostic requirement to one vendor, and act-gitlab-ci already documents Claude-in-CI as an *actor*; conflating actor and reviewer in one job blurs the security boundary between a read-only process and one that commits. | + +## Decision + +Ship `plugins/code-reviews/` (0.1.0), an engine-agnostic review capability: + +**1. One contract.** `skills/mr-review-agent/references/review-rubric.md` defines what a reviewer +reports, the severity scale, and the findings JSON schema. Every surface derives from it, and the +Copilot instructions file restates it; a rubric change is a change to all of them. + +**2. Deterministic delivery.** `scripts/post-mr-review.ts` (CI) and `scripts/codereview.sh` +(hooks, ad-hoc) run the engine, validate its output against the contract, and deliver findings. +Delivery modes: `inline` (positioned discussions plus a sticky, marker-identified summary note; +the default), `summary`, and `log` — the automatic fallback when no `GITLAB_TOKEN` exists, +because `CI_JOB_TOKEN` cannot post. Positions GitLab rejects (HTTP 400) degrade per finding to +plain notes rather than being dropped. + +**3. The engine is confined.** Read-only toolsets in the shipped docker-agent config; both +scripts strip every GitLab token from the engine's environment; timeouts, `allow_failure: true`, +`interruptible: true`, diff budgets, and turn caps bound cost. The reviewer never blocks a merge. + +**4. Scripts are copied into target repositories** (`.codereview/`) by the +`setup-mr-review` command, because CI jobs cannot resolve `${CLAUDE_PLUGIN_ROOT}`. The canonical, +tested copies stay in the plugin; re-running the command refreshes them. + +**5. The docker-agent binary is version-pinned** (`DOCKER_AGENT_VERSION`, optional +`DOCKER_AGENT_SHA256`) and downloaded at job runtime — the repository tracks no binaries. Despite +the name, no Docker daemon is involved; the binary is standalone, which is what makes the "runs +in a normal pipeline" requirement hold even for the default engine. + +**6. Naming.** The plugin is `code-reviews`, by explicit request — the one departure from the +`act-*` prefix convention. The plural also avoids shadowing the upstream Anthropic `code-review` +plugin, whose `/code-review:code-review` command this repository's own GitHub workflow invokes. +The empty `plugins/code-review/` scaffold shell was removed with this change. + +## Consequences + +**The act-* naming convention now has one exception.** Every other plugin keeps the prefix; a +future rename to `act-code-reviews` would be a breaking change for installed consumers and should +not be done casually. + +**act-gitlab-ci stays a pipeline-tooling plugin.** Its only change from this work is a +model-identifier refresh in two examples (0.2.1). Its `claude-code-ci-jobs` skill remains the +actor-in-CI integration; `code-reviews` is the reviewer. The skills state the boundary in both +directions, and neither plugin requires the other. + +**The token guidance now has a documented exception.** act-gitlab-ci says to prefer +`CI_JOB_TOKEN`; posting review comments is a case its permissions cannot cover, and the +`mr-review-agent` skill states when each applies. Without a project access token the capability +degrades to `log` mode rather than failing. + +**Two engines are tested, two are best-effort.** The fixture suite exercises docker-agent's and +claude's output envelopes (saved transcripts; no engine is ever spawned in tests); codex and +copilot CLI flags are young and verified only at setup time, with `CODEREVIEW_ENGINE_CMD` as the +escape hatch. The suite (`scripts/tests/mr-review/`) runs with no network and no engines, so the +repository gate pins parsing, positioning, the 400 fallback, mode downgrades, marker stickiness, +token stripping, and re-push resolution — not model quality. + +**The rubric's restatements can drift.** The Copilot instructions file is a manual restatement of +the rubric, and nothing mechanical keeps them aligned; the skill instructs that they change +together. Drift here degrades one surface's judgment, not correctness of delivery. + +**docker-agent version bumps are deliberate work.** Headless flags, event shapes, and safety +semantics move between releases; the pin, the transcript artifact, and the tolerant parser limit +the blast radius, and the reference documents what to re-check on a bump. diff --git a/docs/decisions/0006-review-skills-over-harness.md b/docs/decisions/0006-review-skills-over-harness.md new file mode 100644 index 0000000..aa9209f --- /dev/null +++ b/docs/decisions/0006-review-skills-over-harness.md @@ -0,0 +1,94 @@ +# 6. Ship review as skills over existing harnesses, and build no runner + +- **Status:** Accepted +- **Date:** 2026-08-15 +- **Supersedes:** [0005](0005-mr-review-engine-agnostic.md) + +## Context + +ADR 0005 shipped automated review as an engine-agnostic contract with a deterministic runner: a +TypeScript poster, a POSIX engine-dispatch harness, an eighteen-variable `CODEREVIEW_*` +environment DSL, a findings-JSON schema, sticky-marker semantics, and a fixture suite pinning all +of it — roughly 1,600 lines. + +The reasoning in 0005 was sound given its premise: GitLab's discussion-position API is exacting, a +model gets it wrong at a steady rate, and putting that in deterministic code makes it testable. +What the premise missed is that **the harness was never the missing piece.** Reviewing the field +turned up a vendor-supported runner for every surface the requirement named: + +| Surface | Harness that already exists | +|---|---| +| GitHub, managed | Claude GitHub App: inline comments, severity tiers, a neutral check run, `@claude review` | +| GitHub, self-hosted | `anthropics/claude-code-action` with `mcp__github_inline_comment__create_inline_comment` | +| GitLab | GitLab-maintained Claude Code CI/CD integration: `claude -p` plus `mcp__gitlab` tools from `/bin/gitlab-mcp-server` | +| Local | The built-in `/code-review` skill, with `--comment`, `--fix`, and effort levels | +| Any engine | docker-agent, `codex exec`, Copilot CLI — each accepts a prompt | + +Two further findings settled it. Anthropic's own `code-review` plugin — the one this repository's +CI already runs — is **a single markdown command file with zero code**. And configuration has an +established convention: **`REVIEW.md`** at repository root, freeform markdown, injected verbatim as +the highest-priority instruction block, with documented tunables for severity, nit caps, skip +rules, repo-specific checks, verification bar, re-review convergence, and summary shape. + +Against that, the 0005 design was building a second-rate copy of infrastructure that already +worked, and teaching operators a configuration language nobody else speaks. + +## Options considered + +| Option | Assessment | +|---|---| +| **Keep the runner, fix its defects** | The defects were real and fixable — a marker-kind collision, an argv limit, inconsistent exit codes — and all were found and fixed. Rejected anyway: a correct implementation of an unnecessary component is still unnecessary, and every fix widened the DSL operators must learn. | +| **Keep the runner as an opt-in advanced path** | Rejected. A documented escape hatch is still shipped, still maintained, still the thing a hurried operator reaches for. Retaining it preserves exactly what is being removed. | +| **Skills plus templates over existing harnesses** | Chosen. The methodology is the differentiated asset and is pure prose; the harnesses are commodity and already supported by their vendors. | +| **Invent a plugin-owned config file** | Rejected. `REVIEW.md` exists, is documented, and is read natively by the managed product. A competing file would fragment configuration for no gain. | + +## Decision + +Rebuild `plugins/code-reviews` as two skills and no runtime. + +**1. `review` carries the methodology.** The defect checklist, the standing false-positive list, +three severity tiers matching what the managed product emits, and one verification bar: a finding +must name a concrete failure scenario — specific inputs or state producing a wrong result — and +cite `path:line` in code actually read. Candidates failing either test are dropped silently. + +**2. `install` wires the review into a harness**, one reference and one template per surface, each +using that harness's native mechanism. It writes configuration and names the credentials the +operator must create; it never handles a value. + +**3. Configuration layers on `REVIEW.md`.** `ACT_CODE_REVIEW.md` opens with `@REVIEW.md` and adds +the organization layer. Precedence runs methodology → `REVIEW.md` → `ACT_CODE_REVIEW.md` → +`CLAUDE.md`/`AGENTS.md`. Both are plain markdown and `@` is Claude Code's existing import syntax; +nothing new is invented. + +**4. Posting always uses the host's tools** — the inline-comment MCP server under +claude-code-action, `mcp__gitlab` in GitLab CI, `gh`/`glab` locally — and only when the invocation +asked for it. Terminal output is the default. + +**5. `commands/` is dropped.** Custom commands have merged into skills upstream, so the repository's +command-plus-shim pattern would duplicate every entry point for no benefit in a new plugin. + +## Consequences + +**Position accuracy is now the model's job, not deterministic code's.** This is the real cost of +the pivot and it should not be glossed: 0005's poster constructed `position` objects from +`diff_refs` and degraded to a plain note on rejection, which was testable offline. Now each harness +posts through its own tooling and accuracy is theirs to maintain. The tradeoff is accepted because +those vendors own the API contract, fix it faster, and already handle the failure modes — and +because unverifiable local behavior was being pinned by fixtures that only ever tested our own code. + +**The test suite shrinks to what can rot.** No runtime means nothing to unit test. What remains +checks that shipped templates parse, that instruction files carry the frontmatter Copilot requires, +that every referenced resource exists, and that no runtime has crept back into the plugin. + +**Guidance must be flattened for two surfaces.** The managed product reads `REVIEW.md` verbatim and +does not expand `@`; Copilot reads its own instructions file. Both receive generated, marked files, +and regeneration is a manual step an operator can forget. Divergence there is silent, which is why +the generated header names its source. + +**The plugin can no longer promise identical output everywhere.** Copilot grades in its own +vocabulary; the managed product runs its own multi-agent pipeline. This plugin steers those +surfaces rather than controlling them, and the README says so rather than implying parity. + +**Roughly 1,600 lines were deleted, including work completed the same day.** Recorded plainly +because the alternative — keeping it to justify having written it — is the failure mode this ADR +exists to prevent. diff --git a/docs/decisions/README.md b/docs/decisions/README.md index d821949..c3f4303 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -15,6 +15,8 @@ what it cost to arrive at, needs one. | [0002](0002-config-driven-plugins.md) | Ship no environment identifiers; read them from a site-local settings file | Accepted | | [0003](0003-three-plugin-split.md) | Split the operational bundle into three plugins | Accepted | | [0004](0004-derived-pipeline-standards.md) | Ship pipeline standards as explicitly derived, and record the platform conflict | Accepted | +| [0005](0005-mr-review-engine-agnostic.md) | Ship automated code review as an engine-agnostic contract in a standalone code-reviews plugin | Superseded by [0006](0006-review-skills-over-harness.md) | +| [0006](0006-review-skills-over-harness.md) | Ship review as skills over existing harnesses, and build no runner | Accepted | ## Format diff --git a/plugins/act-gitlab-ci/.claude-plugin/plugin.json b/plugins/act-gitlab-ci/.claude-plugin/plugin.json index a75f73f..4701b55 100644 --- a/plugins/act-gitlab-ci/.claude-plugin/plugin.json +++ b/plugins/act-gitlab-ci/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "$schema": "https://anthropic.com/claude-code/plugin.schema.json", "name": "act-gitlab-ci", - "version": "0.2.0", + "version": "0.2.1", "description": "GitLab CI/CD and GitLab tooling for Claude Code. Covers running Claude Code as a CI job across the Claude API, Amazon Bedrock and Vertex providers, the GitLab MCP server, the glab CLI, and pipeline standards adapted for GitLab. Includes a zero-dependency pipeline validator.", "author": { "name": "Daniel Bodnar", diff --git a/plugins/act-gitlab-ci/.codex-plugin/plugin.json b/plugins/act-gitlab-ci/.codex-plugin/plugin.json index 90befb3..97baeb1 100644 --- a/plugins/act-gitlab-ci/.codex-plugin/plugin.json +++ b/plugins/act-gitlab-ci/.codex-plugin/plugin.json @@ -1,5 +1,5 @@ { - "name": "act-gitlab-ci", "version": "0.2.0", "description": "GitLab CI/CD, MCP, authentication, troubleshooting, and pipeline review workflows for ACT Data.", + "name": "act-gitlab-ci", "version": "0.2.1", "description": "GitLab CI/CD, MCP, authentication, troubleshooting, and pipeline review workflows for ACT Data.", "author": { "name": "Daniel Bodnar", "email": "dbodnar@pattersonvet.com", "url": "https://github.com/patterson-agents" }, "homepage": "https://github.com/patterson-agents/actdata-plugins", "repository": "https://github.com/patterson-agents/actdata-plugins", "license": "LicenseRef-ACT-Internal", "keywords": ["gitlab", "gitlab-ci", "pipelines", "mcp", "oidc", "devops"], "skills": "./skills/", "mcpServers": "./.mcp.json", diff --git a/plugins/act-gitlab-ci/README.md b/plugins/act-gitlab-ci/README.md index 7ff4d35..c700aba 100644 --- a/plugins/act-gitlab-ci/README.md +++ b/plugins/act-gitlab-ci/README.md @@ -41,6 +41,8 @@ Four related things: 3. **The `glab` CLI**, which is often the better tool for scripted GitLab work. 4. **Pipeline standards**, translated to GitLab and marked as derived throughout. +For automated AI review of merge requests, see the `code-reviews` plugin in this marketplace. + > [!IMPORTANT] > **Two different things are called the GitLab MCP server.** The HTTP server at > `https:///api/v4/mcp` is for interactive sessions and authenticates over OAuth. The @@ -160,6 +162,8 @@ period, no coverage threshold and no scan severity gate. GitLab UI, which the plugin tells you to do and cannot do for you. - **No webhook setup.** `@claude` mentions need a listener calling the pipeline trigger API. GitLab does not do this natively and this plugin does not build it. +- **No automated MR review.** That lives in the `code-reviews` plugin, which runs an AI reviewer in + the pipeline and posts findings to the merge request. - **No authority on standards.** See above. It reports derived rules as derived. - **No GitLab administration.** Enabling Duo, beta features and MCP access are admin or group-owner actions. diff --git a/plugins/act-gitlab-ci/plugin.json b/plugins/act-gitlab-ci/plugin.json index 04da434..960ee81 100644 --- a/plugins/act-gitlab-ci/plugin.json +++ b/plugins/act-gitlab-ci/plugin.json @@ -1,5 +1,5 @@ { - "name": "act-gitlab-ci", "version": "0.2.0", "description": "Build, review, and troubleshoot GitLab CI/CD integrations.", + "name": "act-gitlab-ci", "version": "0.2.1", "description": "Build, review, and troubleshoot GitLab CI/CD integrations.", "author": { "name": "Daniel Bodnar", "email": "dbodnar@pattersonvet.com", "url": "https://github.com/patterson-agents" }, "homepage": "https://github.com/patterson-agents/actdata-plugins", "repository": "https://github.com/patterson-agents/actdata-plugins", "license": "LicenseRef-ACT-Internal", "keywords": ["gitlab", "gitlab-ci", "pipelines", "mcp", "oidc", "devops"], "skills": "skills/", "commands": "commands/", "mcpServers": ".mcp.json" diff --git a/plugins/act-gitlab-ci/skills/ci-auth-providers/SKILL.md b/plugins/act-gitlab-ci/skills/ci-auth-providers/SKILL.md index 4a11386..3233c01 100644 --- a/plugins/act-gitlab-ci/skills/ci-auth-providers/SKILL.md +++ b/plugins/act-gitlab-ci/skills/ci-auth-providers/SKILL.md @@ -59,7 +59,7 @@ instance URL. > conditions on project and ref can be assumed from any pipeline in the instance. Restrict it to the > specific project and to protected refs. -Bedrock model IDs carry region-specific prefixes, for example `us.anthropic.claude-sonnet-4-6`. +Bedrock model IDs carry region-specific prefixes, for example `us.anthropic.claude-opus-5`. ## Google Cloud over Workload Identity Federation diff --git a/plugins/act-gitlab-ci/skills/claude-code-ci-jobs/examples/bedrock-oidc.yml b/plugins/act-gitlab-ci/skills/claude-code-ci-jobs/examples/bedrock-oidc.yml index 8f29d9d..d4037b8 100644 --- a/plugins/act-gitlab-ci/skills/claude-code-ci-jobs/examples/bedrock-oidc.yml +++ b/plugins/act-gitlab-ci/skills/claude-code-ci-jobs/examples/bedrock-oidc.yml @@ -58,7 +58,7 @@ claude-bedrock: CLAUDE_CODE_USE_BEDROCK: "1" # Bedrock model IDs carry region-specific prefixes, for example -# us.anthropic.claude-sonnet-4-6. Pass the model through your job configuration +# us.anthropic.claude-opus-5. Pass the model through your job configuration # or the prompt if the workflow supports it. # # Note: the paths above are inside the ephemeral job container, which is diff --git a/plugins/code-reviews/.claude-plugin/plugin.json b/plugins/code-reviews/.claude-plugin/plugin.json new file mode 100644 index 0000000..c3df52b --- /dev/null +++ b/plugins/code-reviews/.claude-plugin/plugin.json @@ -0,0 +1,25 @@ +{ + "$schema": "https://anthropic.com/claude-code/plugin.schema.json", + "name": "code-reviews", + "version": "0.2.0", + "description": "Automated code review as skills, not a runner. One methodology skill carries the review rubric - severity tiers, the defect checklist, the standing false-positive list, and the verification bar that a finding must name a concrete failure scenario and cite path:line - and reads REVIEW.md, ACT_CODE_REVIEW.md, CLAUDE.md and AGENTS.md as layered guidance on any host or provider. A second skill installs that review into an existing harness: Anthropic's managed Code Review, GitHub Actions, GitLab CI, local hooks, GitHub Copilot instructions, or docker-agent, Codex and Copilot CLI. Ships no runtime and invents no configuration syntax.", + "author": { + "name": "Daniel Bodnar", + "email": "dbodnar@pattersonvet.com" + }, + "homepage": "https://github.com/patterson-agents/actdata-plugins", + "repository": "https://github.com/patterson-agents/actdata-plugins", + "license": "LicenseRef-ACT-Internal", + "keywords": [ + "code-review", + "pull-requests", + "merge-requests", + "github", + "gitlab", + "copilot", + "ci-cd", + "git-hooks", + "automation", + "quality" + ] +} diff --git a/plugins/code-reviews/.codex-plugin/plugin.json b/plugins/code-reviews/.codex-plugin/plugin.json new file mode 100644 index 0000000..9ee1c55 --- /dev/null +++ b/plugins/code-reviews/.codex-plugin/plugin.json @@ -0,0 +1,7 @@ +{ + "name": "code-reviews", "version": "0.2.0", "description": "Code review methodology and harness installation for pull requests, merge requests, and local hooks.", + "author": { "name": "Daniel Bodnar", "email": "dbodnar@pattersonvet.com", "url": "https://github.com/patterson-agents" }, + "homepage": "https://github.com/patterson-agents/actdata-plugins", "repository": "https://github.com/patterson-agents/actdata-plugins", "license": "LicenseRef-ACT-Internal", + "keywords": ["code-review", "pull-requests", "merge-requests", "github", "gitlab", "automation"], "skills": "./skills/", + "interface": { "displayName": "Code Reviews", "shortDescription": "High-quality code review, on any harness", "longDescription": "Carries the review methodology - severity tiers, the defect checklist, the false-positive list, and the verification bar - reading REVIEW.md and ACT_CODE_REVIEW.md as layered guidance, plus an install skill that wires the review into managed Code Review, GitHub Actions, GitLab CI, local hooks, Copilot instructions, or a non-Claude engine.", "developerName": "ACT Data", "category": "Engineering", "capabilities": ["Read", "Write"], "websiteURL": "https://github.com/patterson-agents/actdata-plugins", "defaultPrompt": ["Review this merge request.", "Set up automated code review on every pull request."] } +} diff --git a/plugins/code-reviews/README.md b/plugins/code-reviews/README.md new file mode 100644 index 0000000..ce403c4 --- /dev/null +++ b/plugins/code-reviews/README.md @@ -0,0 +1,148 @@ +
+ + + + ACT Data + + +# code-reviews + +High-quality code review as skills, installable into the harness you already have. + +![skills](https://img.shields.io/badge/skills-2-00A8E1?labelColor=003767) +![templates](https://img.shields.io/badge/templates-7-147EC2) +![runtime](https://img.shields.io/badge/runtime-none-003767) +![deps](https://img.shields.io/badge/dependencies-none-58585B) + +
+ +--- + +## Table of contents + +- [What this is](#what-this-is) +- [What ships](#what-ships) +- [Skills](#skills) +- [Guidance layering](#guidance-layering) +- [Harnesses](#harnesses) +- [Install](#install) +- [What this plugin does NOT do](#what-this-plugin-does-not-do) +- [Layout](#layout) + +## What this is + +Two things, deliberately separated: + +1. **The review methodology** — what a reviewer looks for, what it stays silent about, how a + finding earns its place, and how severity is graded. Provider-agnostic and harness-agnostic + prose, so any agent that can read files can follow it. +2. **Installation into an existing harness** — GitHub, GitLab, or local, each using its own native + mechanism. + +The review instructions and the thing that executes them are separate concerns. This plugin owns +the first and configures the second; it is not a runner. + +> [!IMPORTANT] +> There is no runtime here. No poster script, no engine dispatcher, no environment-variable +> configuration language. Every surface is driven by its own vendor-supported harness, and +> customization uses `REVIEW.md`, an existing convention, rather than a file format invented here. + +## What ships + +| Component | Count | What it is | +|---|---|---| +| Skills | 2 | `review` (perform one) and `install` (wire one up) | +| References | 10 | The methodology in depth, and one per harness | +| Templates | 7 | Two guidance files, three CI/hook configs, one Copilot instructions file, one docker-agent config | +| Runtime | 0 | By design | + +## Skills + +| Skill | Does | +|---|---| +| [`/code-reviews:review`](skills/review/) | Reviews a change against the layered guidance and reports findings; posts only when asked, through the host's own tooling | +| [`/code-reviews:install`](skills/install/) | Sets up the guidance files, then wires the review into a chosen harness | + +## Guidance layering + +Four layers, each overriding the one before it. All are plain markdown. + +| Layer | File | Purpose | +|---|---|---| +| Base | this plugin's `review` skill | The methodology | +| Repository | `REVIEW.md` | Review policy: severity recalibration, nit caps, skip rules, repo-specific checks | +| Organization | `ACT_CODE_REVIEW.md` | The ACT layer; opens with `@REVIEW.md` so it extends rather than replaces | +| Project | `CLAUDE.md`, `AGENTS.md` | How to work in this codebase | + +`REVIEW.md` is Anthropic's documented convention and is read natively by managed Code Review. The +`review` skill is what makes it portable: it reads the same file on every other surface, which +nothing else does. + +> [!NOTE] +> `REVIEW.md` is pasted verbatim by the managed product, so `@` imports are not expanded there. +> Surfaces that cannot follow `@` receive a generated, clearly-marked flattened file, and `install` +> tells you the regeneration step. + +## Harnesses + +| Surface | Harness | Posting mechanism | +|---|---|---| +| GitHub, managed | Claude GitHub App | Native inline comments and a neutral check run | +| GitHub, self-hosted | `anthropics/claude-code-action` | `mcp__github_inline_comment__create_inline_comment` | +| GitLab | GitLab-maintained Claude Code CI/CD integration | `mcp__gitlab` tools from `/bin/gitlab-mcp-server` | +| Local | Built-in `/code-review`, or a pre-push hook | Terminal output | +| GitHub Copilot | Copilot's native reviewer | Copilot's own comments | +| Other engines | docker-agent, Codex, Copilot CLI | Terminal output | + +## Install + +```sh +claude plugin marketplace add patterson-agents/actdata-plugins +claude plugin install code-reviews@actdata-plugins +``` + +Then, in the repository to be reviewed: + +```text +/code-reviews:install +``` + +## What this plugin does NOT do + +> [!CAUTION] +> `install` writes to your repository — `REVIEW.md`, workflow and pipeline files, hooks, +> `.github/instructions/`. `review` posts to a pull or merge request only when explicitly asked. + +- **It is not a harness.** It does not run agents, dispatch engines, or post comments through code + of its own. Where no supported harness exists, the answer is to use one, not to add a runner here. +- **No credential handling.** It names the variables to create and where; it never reads or writes + a value. +- **Reviews never gate a merge.** Findings are advice to verify. The CI templates run + non-blocking and the pre-push hook is advisory. +- **Copilot reviews are configured, not executed.** The instructions file steers Copilot's + reviewer; it does not control what Copilot flags or how it grades. +- **No webhook infrastructure.** GitLab does not run a job on a comment natively, and this plugin + does not build the listener that would. +- **Not a quality guarantee.** A clean review means nothing obvious was found by one probabilistic + pass. + +## Layout + +```text +code-reviews/ + .claude-plugin/plugin.json + README.md + skills/ + review/ + SKILL.md + references/ what-to-report.md severity-model.md + guidance-layering.md personas.md + templates/ REVIEW.md ACT_CODE_REVIEW.md + install/ + SKILL.md + references/ github-managed.md github-actions.md gitlab-ci.md + local.md copilot-native.md non-claude-engines.md + templates/ claude-code-review.yml gitlab-ci-review-job.yml + pre-push code-review.instructions.md review-agent.yaml + scripts/tests/templates/run-tests.sh +``` diff --git a/plugins/code-reviews/plugin.json b/plugins/code-reviews/plugin.json new file mode 100644 index 0000000..1f2fd7e --- /dev/null +++ b/plugins/code-reviews/plugin.json @@ -0,0 +1,6 @@ +{ + "name": "code-reviews", "version": "0.2.0", "description": "Perform high-quality code review on any host, and install it into GitHub, GitLab, or local harnesses.", + "author": { "name": "Daniel Bodnar", "email": "dbodnar@pattersonvet.com", "url": "https://github.com/patterson-agents" }, + "homepage": "https://github.com/patterson-agents/actdata-plugins", "repository": "https://github.com/patterson-agents/actdata-plugins", "license": "LicenseRef-ACT-Internal", + "keywords": ["code-review", "pull-requests", "merge-requests", "github", "gitlab", "automation"], "skills": "skills/" +} diff --git a/plugins/code-reviews/scripts/tests/templates/run-tests.sh b/plugins/code-reviews/scripts/tests/templates/run-tests.sh new file mode 100644 index 0000000..d5a69d9 --- /dev/null +++ b/plugins/code-reviews/scripts/tests/templates/run-tests.sh @@ -0,0 +1,116 @@ +#!/bin/sh +# ============================================================================= +# Template and cross-reference checks for the code-reviews plugin. +# +# This plugin ships no runtime -- every surface uses its host's own harness -- +# so there is nothing to unit test. What can rot is the shipped configuration: +# a YAML template that stops parsing, an instructions file missing the +# frontmatter Copilot requires, or a SKILL.md naming a reference that no longer +# exists. Those are what this suite pins. +# +# scripts/verify-all.sh discovers and runs this file. +# ============================================================================= + +set -u + +SUITE_DIR=$(CDPATH= cd -- "$(dirname -- "$0")" && pwd) +PLUGIN_DIR=$(CDPATH= cd -- "$SUITE_DIR/../../.." && pwd) + +passed=0 +failed=0 + +pass() { passed=$((passed + 1)); printf ' ok %s\n' "$1"; } +fail() { failed=$((failed + 1)); printf ' FAIL %s\n' "$1"; [ $# -gt 1 ] && printf ' %s\n' "$2"; } + +echo "code-reviews templates" + +# --- the plugin ships no runtime --------------------------------------------- +# The whole point of the design. If a script reappears here, either the design +# changed and this test should be deleted deliberately, or something crept back. + +runtime=$(find "$PLUGIN_DIR" -name '*.ts' -o -name '*.py' | grep -v '/tests/' || true) +if [ -z "$runtime" ]; then + pass "plugin ships no runtime code" +else + fail "plugin ships no runtime code" "$runtime" +fi + +# --- skills exist ------------------------------------------------------------- + +for skill in review install; do + if [ -f "$PLUGIN_DIR/skills/$skill/SKILL.md" ]; then + pass "skills/$skill/SKILL.md exists" + else + fail "skills/$skill/SKILL.md exists" + fi +done + +# --- instructions files carry the frontmatter Copilot requires ---------------- + +for f in $(find "$PLUGIN_DIR" -name '*.instructions.md'); do + name=$(basename "$f") + head1=$(head -1 "$f") + if [ "$head1" != "---" ]; then + fail "$name opens with frontmatter" "first line: $head1" + continue + fi + front=$(sed -n '2,/^---$/p' "$f") + missing="" + for key in description applyTo; do + printf '%s' "$front" | grep -q "^$key:" || missing="$missing $key" + done + if [ -z "$missing" ]; then + pass "$name has description and applyTo" + else + fail "$name has description and applyTo" "missing:$missing" + fi +done + +# --- YAML templates parse ----------------------------------------------------- + +if command -v bun >/dev/null 2>&1; then + for f in $(find "$PLUGIN_DIR/skills" -name '*.yml' -o -name '*.yaml'); do + name=$(basename "$f") + if err=$(bun -e ' + import { load } from "js-yaml"; + import { readFileSync } from "node:fs"; + load(readFileSync(process.argv[1], "utf8")); + ' "$f" 2>&1); then + pass "$name parses as YAML" + else + fail "$name parses as YAML" "$err" + fi + done +else + echo " note: bun not installed; skipping YAML parse checks" +fi + +# --- every referenced resource exists ---------------------------------------- +# Catches a SKILL.md or reference naming a file that was renamed or removed. + +for skill_md in "$PLUGIN_DIR"/skills/*/SKILL.md; do + skill_dir=$(dirname "$skill_md") + skill_name=$(basename "$skill_dir") + refs=$(grep -o '`\(references\|templates\)/[A-Za-z0-9._-]*`' "$skill_md" | tr -d '`' | sort -u) + for ref in $refs; do + if [ -e "$skill_dir/$ref" ]; then + pass "$skill_name references $ref" + else + fail "$skill_name references $ref" "no such file" + fi + done +done + +# --- shell templates are valid POSIX sh -------------------------------------- + +for f in "$PLUGIN_DIR"/skills/install/templates/pre-push; do + [ -f "$f" ] || continue + if sh -n "$f" 2>/dev/null; then + pass "$(basename "$f") is valid sh" + else + fail "$(basename "$f") is valid sh" + fi +done + +echo " $passed passed, $failed failed" +[ "$failed" -eq 0 ] || exit 1 diff --git a/plugins/code-reviews/skills/install/SKILL.md b/plugins/code-reviews/skills/install/SKILL.md new file mode 100644 index 0000000..ebfb593 --- /dev/null +++ b/plugins/code-reviews/skills/install/SKILL.md @@ -0,0 +1,75 @@ +--- +name: install +description: This skill should be used when the user asks to "set up automated code review", "install code review on this repo", "review every PR", "review every merge request", "add Claude code review to CI", "wire up a pre-push review hook", or "configure Copilot code review". It installs the review into an existing harness - GitHub Actions, Anthropic's managed Code Review, GitLab CI, local hooks, or a non-Claude engine - using each harness's own native mechanism and never a bespoke runner. +argument-hint: "[github-managed|github-actions|gitlab-ci|local|copilot|other-engine]" +allowed-tools: Read, Write, Edit, Bash, Grep, Glob +--- + +# Install automated code review + +Wire the `review` skill into a harness that already exists. This skill writes configuration and +copies templates; it never builds a runner and never handles a credential. + +## 1. Establish the guidance files first + +Every surface reads the same layers, so set them up before touching any harness. + +1. If `REVIEW.md` is absent at the repository root, copy + `${CLAUDE_PLUGIN_ROOT}/skills/review/templates/REVIEW.md` and cut it down to what the repository + actually wants. Every heading in it is optional; a short file beats a thorough one. +2. If the repository is under ACT and `ACT_CODE_REVIEW.md` is absent, copy + `${CLAUDE_PLUGIN_ROOT}/skills/review/templates/ACT_CODE_REVIEW.md`. It opens with `@REVIEW.md`, + so it extends rather than replaces what step 1 produced. +3. Read `${CLAUDE_PLUGIN_ROOT}/skills/review/references/guidance-layering.md` and tell the user + which of their chosen surfaces expand `@` and which need a flattened file. + +## 2. Choose a surface + +From `$ARGUMENTS`, or ask. More than one can be installed; they are independent. + +| Surface | Use when | Reference | +|---|---|---| +| `github-managed` | GitHub, and the organization has Claude Code Team or Enterprise. No workflow file, no API key. | `references/github-managed.md` | +| `github-actions` | GitHub, and the review should run in your own CI with your own key | `references/github-actions.md` | +| `gitlab-ci` | GitLab merge requests | `references/gitlab-ci.md` | +| `local` | Before pushing, or on demand in a session | `references/local.md` | +| `copilot` | The repository is reviewed by GitHub Copilot's native reviewer | `references/copilot-native.md` | +| `other-engine` | docker-agent, Codex, or Copilot CLI drives the review | `references/non-claude-engines.md` | + +Read the matching reference in full before writing anything. Each one states what the harness does +natively, what it cannot do, and the exact steps. + +## 3. Install + +Copy the template named by the reference, adapt it to the repository's existing conventions — +match the stage names in an existing `.gitlab-ci.yml`, match the trigger style of neighbouring +workflows — and keep every bound the template ships with. The timeouts, turn caps, and +non-blocking settings are cost and safety controls, not decoration. + +Where a reference calls for a generated file, generate it with its header intact and tell the user +the regeneration command. A generated file edited by hand diverges silently. + +## 4. Report what the user must do + +Some steps cannot be done from here. List them explicitly rather than leaving them implied: + +- **Credentials.** Name each variable or secret, where it is created, and how it must be scoped. + Never read, write, or echo a value. +- **Console settings.** Enabling the managed product, choosing a trigger mode, or turning on + Copilot's custom-instructions toggle are all outside the repository. +- **Fork exposure.** Say plainly that secrets must not be exposed to pipelines from forks: the + reviewed diff is untrusted input to a model with the job's environment in reach. + +## 5. Verify + +Recommend a first run before the setup is trusted: open a test pull or merge request, confirm the +review appears, and read what it produced. A review that runs but reports nothing useful is a +guidance problem, and `REVIEW.md` is where it gets fixed. + +## Resources + +References: `github-managed.md`, `github-actions.md`, `gitlab-ci.md`, `local.md`, +`copilot-native.md`, `non-claude-engines.md`. + +Templates: `claude-code-review.yml`, `gitlab-ci-review-job.yml`, `pre-push`, +`code-review.instructions.md`, `review-agent.yaml`. diff --git a/plugins/code-reviews/skills/install/references/copilot-native.md b/plugins/code-reviews/skills/install/references/copilot-native.md new file mode 100644 index 0000000..154cec5 --- /dev/null +++ b/plugins/code-reviews/skills/install/references/copilot-native.md @@ -0,0 +1,59 @@ +# GitHub Copilot native review + +Copilot's own reviewer runs on GitHub's side. Nothing from this plugin executes; the entire +integration is a guidance file Copilot reads. + +## Mechanism + +| File | Scope | +|---|---| +| `.github/copilot-instructions.md` | Repository-wide, every Copilot feature | +| `.github/instructions/.instructions.md` | Path-scoped via `applyTo` frontmatter; supported by Copilot code review and the coding agent | + +Use the second. Copy `templates/code-review.instructions.md` to +`.github/instructions/code-review.instructions.md`. + +Frontmatter: + +```yaml +--- +description: 'Code review guidance for this repository' +applyTo: '**' +--- +``` + +- `applyTo` takes one or more comma-separated globs. `'**'` covers the repository; a narrower glob + scopes guidance to a subtree, and several instruction files can coexist with different scopes. +- `excludeAgent: ["coding-agent"]` restricts a file to code review only. Omit it to let both use it. + +## This file is generated + +Copilot does not expand `@` imports, so it cannot follow `ACT_CODE_REVIEW.md` to `REVIEW.md`. The +instructions file is a flattened restatement of both layers, phrased for a reviewer that posts its +own native comments — so it carries the judgment, not the findings schema. + +Generate it with its header intact: + +```markdown + +``` + +Tell the user the rule that matters: when the review layers change, regenerate. The failure mode is +silent divergence, where Copilot enforces last quarter's policy while everything else enforces +this quarter's. + +## Two conditions outside the file + +1. Copilot code review must be enabled for the repository or organization, and the **use custom + instructions** toggle under Settings, Copilot, Code review must be on. +2. Instructions are read **from the pull request's head branch**. A PR branched before the file + merged does not see it. Say this — it is the usual reason a correct file appears to do nothing. + +## Scope honestly + +This surface is configured, not executed, by the plugin. Copilot decides what to flag, in its own +voice, with its own severity vocabulary. The instructions file steers it; it does not control it. +Where an organization needs findings graded exactly as the rest of this plugin grades them, a +GitHub Actions or managed review is the surface that delivers that, and Copilot's reviewer is +complementary rather than equivalent. diff --git a/plugins/code-reviews/skills/install/references/github-actions.md b/plugins/code-reviews/skills/install/references/github-actions.md new file mode 100644 index 0000000..207e26b --- /dev/null +++ b/plugins/code-reviews/skills/install/references/github-actions.md @@ -0,0 +1,67 @@ +# GitHub Actions + +Run the review in your own CI with your own credentials, using `anthropics/claude-code-action`. +Choose this over `github-managed.md` when the organization does not qualify for the managed +product, or when the trigger, model, or prompt must be under repository control. + +## Install + +Copy `templates/claude-code-review.yml` to `.github/workflows/claude-code-review.yml` and adapt the +trigger to the repository's conventions. + +## The two lines that decide where findings go + +```yaml +prompt: '/code-review:code-review --comment ${{ github.repository }}/pull/${{ github.event.pull_request.number }}' +claude_args: '--allowedTools "mcp__github_inline_comment__create_inline_comment"' +``` + +- **`--comment`** is what makes findings post at all. Without it the review runs and writes to the + workflow log only. This is the correct default for a first run. +- **`claude_args`** must name the inline-comment tool even though the invoked skill's own + frontmatter already allows it: the action starts that MCP server only when `--allowedTools` names + it. Dropping this line produces a review that finds issues and silently posts none. + +## Which review skill to invoke + +Two are available and they are not interchangeable: + +| Prompt | What runs | +|---|---| +| `/code-review:code-review --comment ` | Anthropic's upstream plugin, installed via `plugin_marketplaces` + `plugins`. Multi-agent, generate-then-validate, high-signal filter. | +| `/code-reviews:review` | This plugin's skill, which reads `REVIEW.md` and `ACT_CODE_REVIEW.md` | + +Use the upstream plugin when the priority is the strongest generic bug-finding available, and this +plugin's skill when organization guidance must be applied. They can be combined by invoking the +upstream plugin and letting `CLAUDE.md` carry the ACT layer, since the upstream review reads +`CLAUDE.md` natively but does not read `REVIEW.md`. + +The shipped template invokes this plugin's skill and installs no external marketplace. + +## Permissions and secrets + +```yaml +permissions: + contents: read + pull-requests: read + issues: read + id-token: write +``` + +`pull-requests: read` is sufficient: the inline-comment MCP server writes through the action's own +app token, not through `gh`. `id-token: write` is required for the action's default GitHub App +authentication. + +The user creates one repository or organization secret — `ANTHROPIC_API_KEY`, or +`CLAUDE_CODE_OAUTH_TOKEN` for a subscription token, swapping the matching input. For an +organization-wide rollout prefer an API key: an OAuth token is tied to whoever generated it. + +> [!CAUTION] +> On public repositories GitHub withholds secrets from fork pull requests, so the review runs only +> on same-repository branches. Do not work around this: the diff under review is untrusted input to +> a model holding the job's environment. + +## Cost bounds + +Keep the template's `--max-turns` and job `timeout`, and consider a concurrency group so a rapid +series of pushes cancels superseded runs. Both meters run at once — Actions minutes and API tokens. diff --git a/plugins/code-reviews/skills/install/references/github-managed.md b/plugins/code-reviews/skills/install/references/github-managed.md new file mode 100644 index 0000000..7cd7ed5 --- /dev/null +++ b/plugins/code-reviews/skills/install/references/github-managed.md @@ -0,0 +1,83 @@ +# GitHub, managed Code Review + +Anthropic runs the review on its own infrastructure. No workflow file, no API key in the +repository, no runner minutes. This is the least work and the best default when the organization +qualifies. + +## Prerequisites + +- A Claude Code Team or Enterprise subscription. It is unavailable to organizations with Zero Data + Retention enabled. +- An Owner or Primary Owner in the Claude organization, with permission to install GitHub Apps. + +These are the user's to arrange; state them and stop if they are not met. Where the organization +does not qualify, `github-actions.md` is the equivalent self-hosted path. + +## Steps the user performs + +1. Open `claude.ai/admin-settings/claude-code`, find the Code Review section, click **Setup**. +2. Install the Claude GitHub App into the organization and grant it the target repositories. +3. Select which repositories to enable. +4. Set **Review Behavior** per repository: + +| Mode | Runs | Cost | +|---|---|---| +| Once after PR creation | When a PR opens or is marked ready | One review per PR | +| After every push | On each push; resolves threads when issues are fixed | Multiplies by push count | +| Manual | Only on `@claude review` | Nothing until asked | + +Manual mode suits high-traffic repositories. `@claude review always` opts a single PR into +push-triggered reviews without changing the repository default. + +## What this repository must contain + +`REVIEW.md` at the root. The managed product injects it verbatim into every agent in the review +pipeline as the highest-priority instruction block, which makes it the single most effective place +to tune what gets flagged. + +> [!IMPORTANT] +> `REVIEW.md` is pasted verbatim: `@` imports are **not** expanded and referenced files are **not** +> read. An `ACT_CODE_REVIEW.md` layer is therefore invisible to this surface. + +Flatten the layers so this surface sees both. Append the ACT layer's body — everything after its +`@REVIEW.md` line — to `REVIEW.md` beneath a marked section: + +```markdown + +...ACT layer body... + +``` + +Tell the user this section regenerates and must not be hand-edited. + +## What to expect + +- Findings post as inline comments on the lines they concern, tagged Important, Nit, or + Pre-existing, each with expandable reasoning. +- A **Claude Code Review** check run collects every finding in one severity-sorted table, and + annotates the Files changed tab. It always completes neutral, so it never blocks a merge through + branch protection. +- Reviews take roughly 20 minutes and are billed per run, scaling with diff size. + +## Gating merges on findings + +The check run never blocks. To gate in your own CI, read the machine-readable severity counts from +the last line of the check run's output: + +```sh +gh api repos/OWNER/REPO/commits//check-runs --jq '.check_runs[] | select(.name=="Claude Code Review") | .id' +gh api repos/OWNER/REPO/check-runs/CHECK_RUN_ID \ + --jq '.output.text | split("bughunter-severity: ")[1] | split(" -->")[0] | fromjson' +``` + +This returns counts per severity, for example `{"normal": 2, "nit": 1, "pre_existing": 0}`, where +`normal` is the Important count. + +## Troubleshooting + +| Symptom | Cause | +|---|---| +| No check run appears | The repository is not enabled, or the App lacks access to it | +| Check says issues found, no inline comments | Look at the check run Details table and the Files changed annotations; a line that moved rejects its inline comment | +| Review errored or timed out | Comment `@claude review` to retry. GitHub's **Re-run** button does not retrigger it | +| A spend-cap comment appears | The organization's monthly cap was reached; reviews resume next period or when an admin raises it | diff --git a/plugins/code-reviews/skills/install/references/gitlab-ci.md b/plugins/code-reviews/skills/install/references/gitlab-ci.md new file mode 100644 index 0000000..8150ef2 --- /dev/null +++ b/plugins/code-reviews/skills/install/references/gitlab-ci.md @@ -0,0 +1,87 @@ +# GitLab CI + +GitLab maintains its own Claude Code CI/CD integration. It is the harness; this plugin supplies the +review guidance the job's prompt points at. Nothing bespoke is needed, and in particular no script +that posts comments — the job's Claude Code process posts them itself through the GitLab MCP tools. + +> [!NOTE] +> The integration is in beta and maintained by GitLab, not Anthropic. Flags vary by CLI version; +> run `claude --help` inside a job to confirm what the installed version supports. + +## Install + +Copy `templates/gitlab-ci-review-job.yml` into `.gitlab-ci.yml`, matching the project's existing +stage names rather than appending a foreign-looking block. If the project uses `include:` for +shared templates, ask whether the job belongs in the template project instead. + +## How the job posts back + +```yaml +script: + - /bin/gitlab-mcp-server || true + - > + claude -p "..." + --allowedTools "Read Grep Glob mcp__gitlab" + --max-turns 25 +``` + +`/bin/gitlab-mcp-server` is a binary in the runner image that supplies `mcp__gitlab` tools inside +the job. Naming `mcp__gitlab` in `--allowedTools` is what lets Claude comment on the merge request. + +> [!IMPORTANT] +> This runner binary is **not** the HTTP MCP server at `https:///api/v4/mcp`. That one is for +> interactive sessions and authenticates over OAuth, which is unusable in CI. They are not +> interchangeable. + +The tool allowlist above is read-only plus GitLab: a reviewer has no reason to hold `Edit`, `Write`, +or a general `Bash`. A job that also implements changes is a different job. + +## Triggering + +```yaml +rules: + - if: '$CI_PIPELINE_SOURCE == "merge_request_event"' +``` + +That is native and needs nothing else. Two refinements the template ships: + +- **Skip drafts.** `CI_MERGE_REQUEST_DRAFT` exists only on GitLab 17.10 and later, so the template + also matches a `Draft:` title prefix for older instances. +- **Start manual.** For a first run, `- if: '$CI_PIPELINE_SOURCE == "web"'` alone lets credentials + and permissions be confirmed by someone who chose to run it. + +> [!WARNING] +> `@claude`-on-comment is **not** native. GitLab does not run a job on a comment. It requires a +> project webhook on Comments (notes) calling the pipeline trigger API with `AI_FLOW_INPUT`, +> `AI_FLOW_CONTEXT`, and `AI_FLOW_EVENT`. That listener is separate infrastructure this plugin does +> not provide. Say so rather than leaving the user with a correct job that never fires. + +## Tokens + +| Token | Use | +|---|---| +| `CI_JOB_TOKEN` | The default. Scoped to the job, expires with it. | +| Project access token, `api` scope, masked as `GITLAB_ACCESS_TOKEN` | Only where the job needs permissions the job token does not carry | + +Prefer `CI_JOB_TOKEN`; a project access token is a long-lived credential. If commenting fails with +the job token on your instance, that is the case for the access token. + +## Provider credentials + +The user creates these; never handle the values: + +| Provider | Variables | +|---|---| +| Claude API | `ANTHROPIC_API_KEY`, masked, protected if the job runs only on protected refs | +| Amazon Bedrock | `AWS_ROLE_TO_ASSUME`, `AWS_REGION`, plus GitLab configured as an OIDC provider in AWS IAM and a role whose trust policy is restricted to this project and its protected refs. Set `CLAUDE_CODE_USE_BEDROCK: "1"`. | +| Google Cloud Agent Platform | `GCP_WORKLOAD_IDENTITY_PROVIDER` (without the `//iam.googleapis.com/` prefix), `GCP_SERVICE_ACCOUNT`, `GCP_PROJECT_ID`, `CLOUD_ML_REGION`. Set `CLAUDE_CODE_USE_VERTEX: "1"`. | + +Both cloud providers authenticate over OIDC with nothing stored, which is the reason to prefer them +where the organization already uses that cloud. + +## Cost bounds + +`--max-turns` and job `timeout` are the two controls that bound a task which turns out harder than +expected, and neither has a useful default for an unattended job. Add `interruptible: true` so a new +push supersedes an in-flight review, and limit concurrency so triggered jobs cannot pile up +unnoticed. diff --git a/plugins/code-reviews/skills/install/references/local.md b/plugins/code-reviews/skills/install/references/local.md new file mode 100644 index 0000000..1153701 --- /dev/null +++ b/plugins/code-reviews/skills/install/references/local.md @@ -0,0 +1,55 @@ +# Local + +Reviewing before the change ever reaches a pipeline. Nothing is installed for the first option — +it already exists. + +## In a session + +Claude Code ships `/code-review` as a built-in. It reviews the branch's commits ahead of upstream +plus uncommitted changes, runs as a background subagent with its own context, and takes a target: +a path, a PR number, a branch, or a range such as `main...my-feature`. + +| Flag | Effect | +|---|---| +| `--fix` | Applies findings to the working tree after the review | +| `--comment` | Posts findings as inline PR comments | +| effort level (`low` through `max`) | Trades coverage for confidence; `low` and `medium` report only high-confidence findings | + +> [!NOTE] +> The built-in reads `CLAUDE.md` but **does not read `REVIEW.md`**. Where the repository's review +> policy lives in `REVIEW.md`, invoke `/code-reviews:review` instead — reading and applying those +> layers is exactly what it adds. The two otherwise overlap heavily, and the built-in's multi-agent +> verification is stronger on generic bug-finding. + +Because a background review's `--fix` edits land outside session checkpoints, `/rewind` will not +undo them. Use git. + +## As a pre-push hook + +Copy `templates/pre-push` to `.git/hooks/pre-push` and mark it executable, or point an existing +hook manager (lefthook, husky, `core.hooksPath`) at it. + +The hook calls whichever agent CLI is already on `PATH` — `claude`, `codex`, or `copilot` — and +skips silently when none is found. There is no configuration and no path to set: a tool that is not +installed simply does not run. + +Two properties worth stating to the user: + +- **Advisory by default.** Findings print and the push proceeds. A hook that blocks on a + probabilistic reviewer strands people at the worst moment. +- **`.git/hooks` is per-clone and unversioned.** Each contributor installs it themselves, which is + the argument for a hook manager whose config is committed. + +`git push --no-verify` bypasses it, as with any hook. + +## Which to reach for + +| Situation | Use | +|---|---| +| Mid-work, want a second opinion | `/code-reviews:review` or `/code-review` in the session | +| About to push a branch | The pre-push hook | +| Reviewing someone else's PR locally | `/code-reviews:review ` | +| Every change, without anyone remembering | A CI surface, not this one | + +A local review is a convenience for the author. It is not a substitute for a pipeline review, +because it only runs when someone chooses to run it. diff --git a/plugins/code-reviews/skills/install/references/non-claude-engines.md b/plugins/code-reviews/skills/install/references/non-claude-engines.md new file mode 100644 index 0000000..cde6488 --- /dev/null +++ b/plugins/code-reviews/skills/install/references/non-claude-engines.md @@ -0,0 +1,66 @@ +# Non-Claude engines + +The review methodology is provider-agnostic prose, so any agent that accepts a prompt and can read +files can run it. What changes between engines is only the invocation. + +The pattern is the same everywhere: **point the engine at the guidance files and let it read them.** +Do not paste the rubric into the invocation, and do not build a wrapper that assembles a prompt — +the files are already on disk in the repository being reviewed. + +A workable prompt, verbatim, for any engine: + +```text +Review the changes on this branch against the review guidance in this repository. +Read, in order: REVIEW.md, then ACT_CODE_REVIEW.md and any file it references with @. +Report only findings that name a concrete failure scenario and cite path:line. +Print the findings; do not modify any file. +``` + +## docker-agent + +A declarative runner: a YAML file names a model, an instruction, and toolsets. Provider-agnostic — +swapping `model:` swaps vendors — and the binary is standalone, so no Docker daemon is involved +despite the name. + +Copy `templates/review-agent.yaml`, set `model:`, and run it headless: + +```sh +docker-agent run --exec review-agent.yaml --safety restricted +``` + +The shipped toolset is read-only on purpose: filesystem reads and a fixed `git log` command, with +no shell, no network, and no MCP. The diff under review is untrusted input to an unattended model, +and the toolset is what bounds a prompt-injected run — the approval flag governs prompting, the +toolset governs capability. + +Pin the release rather than tracking latest; headless flags and event shapes move between versions. + +## Codex + +```sh +codex exec "" +``` + +Verify flags against `codex exec --help` for the installed version before committing them to a +pipeline. + +## Copilot CLI + +```sh +copilot -p "" +``` + +Same caveat. Note that a prompt passed as an argument is bounded by the operating system's +per-argument limit, so a very large diff needs the engine's stdin mode where one exists. + +## Where these fit + +| Engine | Reasonable use | +|---|---| +| docker-agent | A pipeline that must not depend on any single vendor, or one running a local model | +| Codex, Copilot CLI | A team already standardized on that CLI | +| Claude Code | Everything else — it is the only engine whose review skills this plugin can invoke directly | + +None of these post comments on their own. Where a review must land on a merge request, use a +surface whose harness provides the posting tools: `gitlab-ci.md`, `github-actions.md`, or +`github-managed.md`. diff --git a/plugins/code-reviews/skills/install/templates/claude-code-review.yml b/plugins/code-reviews/skills/install/templates/claude-code-review.yml new file mode 100644 index 0000000..f77326f --- /dev/null +++ b/plugins/code-reviews/skills/install/templates/claude-code-review.yml @@ -0,0 +1,54 @@ +# Automated code review on every pull request. +# +# Copy to .github/workflows/claude-code-review.yml. +# +# Create one repository or organization secret before this runs: +# ANTHROPIC_API_KEY a Claude API key +# or, for a subscription token, CLAUDE_CODE_OAUTH_TOKEN, swapping the input +# below to claude_code_oauth_token. +# +# On public repositories GitHub withholds secrets from fork pull requests, so +# this runs only on branches in the same repository. That is intended: the +# reviewed diff is untrusted input to a model holding the job's environment. + +name: Code Review + +on: + pull_request: + types: [opened, synchronize, ready_for_review, reopened] + +# A new push supersedes an in-flight review rather than paying for both. +concurrency: + group: code-review-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + review: + runs-on: ubuntu-latest + timeout-minutes: 20 + permissions: + contents: read + # read is sufficient: the inline-comment MCP server writes through the + # action's own app token, not through gh. + pull-requests: read + issues: read + id-token: write + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 1 + + - uses: anthropics/claude-code-action@v1 + with: + anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }} + plugin_marketplaces: "https://github.com/patterson-agents/actdata-plugins.git" + plugins: "code-reviews@actdata-plugins" + # --comment is what makes findings post. Without it the review runs + # and writes to the workflow log only. + prompt: "/code-reviews:review --comment ${{ github.repository }}/pull/${{ github.event.pull_request.number }}" + # Required even though the skill's own frontmatter allows the tool: + # the action starts the inline-comment MCP server only when + # --allowedTools names it here. + claude_args: >- + --allowedTools "mcp__github_inline_comment__create_inline_comment" + --max-turns 25 diff --git a/plugins/code-reviews/skills/install/templates/code-review.instructions.md b/plugins/code-reviews/skills/install/templates/code-review.instructions.md new file mode 100644 index 0000000..f233443 --- /dev/null +++ b/plugins/code-reviews/skills/install/templates/code-review.instructions.md @@ -0,0 +1,52 @@ +--- +description: 'Code review guidance for this repository' +applyTo: '**' +--- + + + +# Code review instructions + +Review only the changed lines and the minimum surrounding context needed to judge them. + +## Report + +In priority order: + +1. **Correctness**: inverted conditions, off-by-one bounds, unhandled null or empty cases, broken + error propagation, behavior that differs from what a name or docstring promises. +2. **Security**: injection (SQL, shell, path, template), missing authorization, secrets in code or + logs, unsafe deserialization, SSRF, disabled TLS verification. Flag only with a concrete path + from untrusted input to a dangerous sink. +3. **Concurrency and state**: unguarded read-modify-write, missing idempotency where retries occur, + resources without a guaranteed release path. +4. **Error handling**: swallowed exceptions, catches broad enough to hide unrelated errors, errors + logged but not surfaced, fallbacks that mask failure. +5. **API contracts**: unversioned breaking changes, schema drift, migrations that are not backward + compatible with deployed code. + +## Do not report + +- Anything a linter, formatter, type checker, or compiler already catches. +- Style, naming, and formatting preferences. +- Pre-existing issues this change did not introduce. +- Speculative problems that depend on inputs you cannot show are reachable. +- Generic observations such as "needs more tests" or "could use better docs". +- The same root cause in more than one place; comment at the most representative location. + +## Every comment must + +State what breaks, name the concrete inputs or state that break it, and suggest a specific fix. +A claim about behavior needs evidence in code that is visible in the diff or its context — an +inference from a function or variable name is not evidence. + +## Severity + +Reserve blocking language for changes that would produce incorrect behavior, expose data, or cause +data loss. Phrase likely-but-conditional problems as warnings. Mark minor issues as nitpicks the +author may reasonably ignore. When torn between two levels, choose the lower one. + +## Volume + +Past roughly five minor comments, summarize the remainder as a count rather than posting each one. diff --git a/plugins/code-reviews/skills/install/templates/gitlab-ci-review-job.yml b/plugins/code-reviews/skills/install/templates/gitlab-ci-review-job.yml new file mode 100644 index 0000000..2e816dd --- /dev/null +++ b/plugins/code-reviews/skills/install/templates/gitlab-ci-review-job.yml @@ -0,0 +1,53 @@ +# Automated code review on every merge request. +# +# Add to .gitlab-ci.yml, adapting `stage:` to the project's existing stages. +# This uses the GitLab-maintained Claude Code CI/CD integration; the job's +# Claude Code process posts merge request comments itself through the +# mcp__gitlab tools, so no posting script is involved. +# +# CI/CD variables to create under Settings > CI/CD > Variables, all masked: +# ANTHROPIC_API_KEY the Claude API key +# For Amazon Bedrock or Google Cloud instead, see gitlab-ci.md. +# +# Do not expose these variables to pipelines from forks. + +code-review: + stage: test + image: node:24-alpine3.21 + rules: + # Draft merge requests are not reviewed. CI_MERGE_REQUEST_DRAFT needs + # GitLab 17.10+; the title check covers older instances. + - if: '$CI_PIPELINE_SOURCE == "merge_request_event" && $CI_MERGE_REQUEST_DRAFT == "true"' + when: never + - if: '$CI_PIPELINE_SOURCE == "merge_request_event" && $CI_MERGE_REQUEST_TITLE =~ /^Draft:/' + when: never + # For a first run, comment out the line below and keep only the web rule, + # so a person confirms credentials and permissions before this is automatic. + - if: '$CI_PIPELINE_SOURCE == "web"' + - if: '$CI_PIPELINE_SOURCE == "merge_request_event"' + variables: + GIT_DEPTH: "0" + before_script: + - apk add --no-cache git curl bash + - curl -fsSL https://claude.ai/install.sh | bash + # The installer places claude in ~/.local/bin, which is not on PATH here. + - export PATH="$HOME/.local/bin:$PATH" + script: + # Supplies the mcp__gitlab tools inside the job. This runner binary is not + # the HTTP MCP server at /api/v4/mcp -- see gitlab-ci.md. + - /bin/gitlab-mcp-server || true + - > + claude + -p "Review this merge request following the guidance in REVIEW.md and ACT_CODE_REVIEW.md, + including any file they reference with @. Report only findings that name a concrete failure + scenario and cite path:line. Post them on the merge request, one comment per unique issue, + with a summary note. Modify no files." + --allowedTools "Read Grep Glob mcp__gitlab" + --max-turns 25 + # Bounds a task that turns out harder than expected. Neither has a useful + # default for an unattended job. + timeout: 20m + # A probabilistic reviewer must never block a merge. + allow_failure: true + # A new push supersedes an in-flight review. + interruptible: true diff --git a/plugins/code-reviews/skills/install/templates/pre-push b/plugins/code-reviews/skills/install/templates/pre-push new file mode 100644 index 0000000..dddf139 --- /dev/null +++ b/plugins/code-reviews/skills/install/templates/pre-push @@ -0,0 +1,48 @@ +#!/bin/sh +# pre-push -- review the commits about to be pushed. +# +# Install: copy to .git/hooks/pre-push and mark it executable, or point a hook +# manager (lefthook, husky, core.hooksPath) at it. Note that .git/hooks is +# per-clone and unversioned, so each contributor installs it themselves. +# +# Runs whichever agent CLI is already on PATH and skips silently when none is. +# There is nothing to configure: install a different CLI, or put one earlier on +# PATH, to change which engine reviews. +# +# Advisory: findings print and the push proceeds. `git push --no-verify` skips +# this hook entirely. + +ZERO=0000000000000000000000000000000000000000 + +PROMPT='Review the changes in the given range against the review guidance in this repository. +Read, in order: REVIEW.md, then ACT_CODE_REVIEW.md and any file it references with @. +Report only findings that name a concrete failure scenario and cite path:line. +Print the findings; do not modify any file.' + +if command -v claude >/dev/null 2>&1; then + review() { claude -p "$PROMPT Range: $1" --allowedTools "Read Grep Glob" --max-turns 15; } +elif command -v codex >/dev/null 2>&1; then + review() { codex exec "$PROMPT Range: $1"; } +elif command -v copilot >/dev/null 2>&1; then + review() { copilot -p "$PROMPT Range: $1"; } +else + echo "pre-push: no agent CLI on PATH; skipping review." >&2 + exit 0 +fi + +while read -r _local_ref local_sha _remote_ref remote_sha; do + # Deleting a ref pushes nothing reviewable. + [ "$local_sha" = "$ZERO" ] && continue + + if [ "$remote_sha" = "$ZERO" ]; then + # New branch: compare against the default branch when it is known. + base=$(git merge-base origin/HEAD "$local_sha" 2>/dev/null) || base="" + else + base="$remote_sha" + fi + [ -n "$base" ] || continue + + review "$base...$local_sha" +done + +exit 0 diff --git a/plugins/code-reviews/skills/install/templates/review-agent.yaml b/plugins/code-reviews/skills/install/templates/review-agent.yaml new file mode 100644 index 0000000..f7ea872 --- /dev/null +++ b/plugins/code-reviews/skills/install/templates/review-agent.yaml @@ -0,0 +1,57 @@ +# docker-agent configuration for code review. +# +# Run headless: +# docker-agent run --exec review-agent.yaml --safety restricted +# +# The binary is standalone -- no Docker daemon is involved despite the name. +# Pin a release rather than tracking latest; headless flags and event shapes +# move between versions. +# +# SECURITY: the diff under review is untrusted input to an unattended model. +# The toolset below is read-only on purpose. Widening it is a security +# decision, not a convenience: the approval flag governs prompting, the +# toolset governs capability. + +agents: + root: + # REQUIRED: replace with the model for the chosen provider, and set that + # provider's API key in the environment. Examples: + # + # model: anthropic/claude-opus-5 # ANTHROPIC_API_KEY; recommended + # model: anthropic/claude-sonnet-5 # ANTHROPIC_API_KEY; lower cost + # model: openai/gpt-5.6-sol # OPENAI_API_KEY + # + # Local models work the same way with a dmr/ model string and no key. + # Provider list: https://docker.github.io/docker-agent/providers/overview/ + model: REPLACE_ME + + instruction: | + Review the changes on the current branch as a senior engineer. + + Read the review guidance in this repository before starting: REVIEW.md + first, then ACT_CODE_REVIEW.md and any file it references with an @ + prefix. Follow that guidance exactly; it overrides these instructions + where the two differ. + + Review only the changed lines and the minimum surrounding context needed + to judge them. Report a finding only when it names a concrete failure + scenario -- specific inputs or state producing a wrong result, a crash, + or a security consequence -- and cites path:line in code you have read. + Do not report style, formatting, pre-existing issues, or anything a + linter catches. + + Print the findings, most severe first, each with the defect, the failure + scenario, and a concrete suggestion. Say so plainly when there is nothing + to report. Modify no files. + + toolsets: + - type: filesystem + tools: ["read_file", "search_files_content"] + - type: script + shell: + changed_files: + description: The diff against the default branch + cmd: git diff origin/HEAD...HEAD + recent_history: + description: The last 30 commits + cmd: git log --oneline -n 30 diff --git a/plugins/code-reviews/skills/review/SKILL.md b/plugins/code-reviews/skills/review/SKILL.md new file mode 100644 index 0000000..f9157ba --- /dev/null +++ b/plugins/code-reviews/skills/review/SKILL.md @@ -0,0 +1,121 @@ +--- +name: review +description: This skill should be used when the user asks to "review this PR", "review this merge request", "code review these changes", "review my diff", "what's wrong with this change", or when an automated pipeline invokes a review on a pull request or merge request. It carries the review methodology - severity, what to flag, what to stay silent about, the verification bar - and reads REVIEW.md, ACT_CODE_REVIEW.md, CLAUDE.md and AGENTS.md as layered guidance. Applies on any host and any provider. +allowed-tools: Read, Grep, Glob, Bash +--- + +# Perform a code review + +Review a change and report only findings a competent engineer would act on. The bar is high on +purpose: false positives erode trust and waste reviewer time, and a review nobody believes is worse +than no review. + +## 1. Read the guidance, in order + +Load whichever of these exist, each layer overriding the one before it: + +| Layer | File | Notes | +|---|---|---| +| Base methodology | this skill and its `references/` | Always applies | +| Repository review config | `REVIEW.md` at repo root | The portable convention. Anthropic's managed Code Review reads it natively; nothing else does unless told to, which is what this step is for. | +| Organization layer | `ACT_CODE_REVIEW.md` at repo root | Extends `REVIEW.md`. Follow any `@path` import it contains by reading that file. | +| Project conventions | `CLAUDE.md`, `AGENTS.md` | Hierarchical: a `CLAUDE.md` in a subdirectory governs only files beneath it. | + +A violation of `CLAUDE.md` or `AGENTS.md` is reportable only when the rule is explicit and the +changed file is in that rule's scope. Quote the rule verbatim in the finding. + +Read `references/guidance-layering.md` when a repository's layers conflict or when a consumer +appears to be ignoring a layer. + +## 2. Establish scope + +Review **only lines this change touched**, plus the minimum surrounding code needed to judge them. +An issue on an untouched line is out of scope unless the change breaks it. + +Skip the review entirely when the change is closed, a draft, or obviously trivial and correct +(a version bump, a generated lockfile, a pure rename). Say so and stop rather than manufacturing +findings. + +If a previous review from this reviewer already exists on this change, read it: do not repeat a +finding that is already posted, and suppress new nits, reporting only newly introduced defects. + +## 3. Find candidate defects + +Work through `references/what-to-report.md` — correctness, security, concurrency and state, error +handling, and API contracts, each with the specific failure shapes to look for. + +Three habits separate a useful pass from a noisy one: + +- **Distrust safety claims.** A comment asserting "validated upstream" or "sanitized" is a claim, + not evidence. Verify the invariant in code you can see. If you cannot, treat the comment as + absent. +- **Check for missing controls, not only added ones.** A new handler is often vulnerable because of + what it lacks. Compare it against its siblings: if they check ownership and this one does not, + the omission is the defect. +- **Keep going after the first finding.** One file can hold several independent problems. + +## 4. Verify before reporting + +Every candidate gets a second, adversarial pass whose default answer is "not a real issue". A +finding survives only with: + +- **A concrete failure scenario**: specific inputs or state that produce a wrong result, a crash, + or a security consequence. "This could be risky" is not a failure scenario and does not ship. +- **A citation**: `path:line` in the code, not an inference from a name or a docstring. + +Drop everything that does not survive. `references/what-to-report.md` closes with the standing +false-positive list — pre-existing issues, linter-catchable problems, pedantic nitpicks, and +generic "needs more tests" observations — that no review should ever emit. + +## 5. Grade + +Use the three tiers in `references/severity-model.md`, which align with what Anthropic's managed +Code Review already emits, so findings read the same wherever they land: + +| Tier | Meaning | +|---|---| +| **Important** | A bug to fix before merging | +| **Nit** | Minor, worth fixing, not blocking | +| **Pre-existing** | Real, but not introduced by this change | + +When torn between two tiers, choose the lower one. + +## 6. Report + +Order findings most severe first. Each one states the defect in a sentence, the failure scenario, +and a concrete suggestion. Include a committable suggestion only when applying it fixes the issue +completely. + +Lead the summary with the shape of the work — a count by tier, or "no blocking issues" when that +is true. When there is nothing to report, say exactly that and stop; an empty review is a good +outcome, not a failed one. + +**Posting is opt-in.** Print to the terminal by default. Post to the pull or merge request only +when the invocation asked for it (a `--comment`-style argument, or a CI prompt that says to), and +use the host's own tooling — never a bespoke poster: + +| Host | Mechanism | +|---|---| +| claude-code-action | `mcp__github_inline_comment__create_inline_comment`, plus `gh pr comment` for the summary | +| GitLab CI | the `mcp__gitlab` tools supplied by `/bin/gitlab-mcp-server` | +| Local with a CLI available | `gh` or `glab` | +| Anything else | print the report; say plainly that nothing was posted | + +Post one comment per unique issue, and never post without the review having been asked to. + +## Tone + +Default to direct, specific, and neutral. A persona is a separable layer that changes voice and +must never change what gets flagged or how severity is graded; `references/personas.md` covers the +tradeoffs and the failure modes of the popular ones. + +## Resources + +- **`references/what-to-report.md`** — the defect checklist and the standing false-positive list +- **`references/severity-model.md`** — the tiers, recalibration, and grading discipline +- **`references/guidance-layering.md`** — precedence, `@` imports, which consumer reads what +- **`references/personas.md`** — tone as an optional layer +- **`templates/REVIEW.md`** — starter repository review config +- **`templates/ACT_CODE_REVIEW.md`** — the ACT layer, importing `REVIEW.md` + +To install this review into a pipeline or hook, use the `install` skill. diff --git a/plugins/code-reviews/skills/review/references/guidance-layering.md b/plugins/code-reviews/skills/review/references/guidance-layering.md new file mode 100644 index 0000000..1500bfb --- /dev/null +++ b/plugins/code-reviews/skills/review/references/guidance-layering.md @@ -0,0 +1,59 @@ +# Guidance layering + +Which file holds what, which consumer reads which file, and what to do where they disagree. + +## Precedence + +Later layers win: + +```text +skill methodology → REVIEW.md → ACT_CODE_REVIEW.md → CLAUDE.md / AGENTS.md +``` + +`CLAUDE.md` sits last not because project conventions outrank review policy, but because they are +the most specific statement of what this codebase considers correct. A `CLAUDE.md` rule is +reportable only when it is explicit and in scope for the changed file. + +## What belongs where + +| File | Holds | Does not hold | +|---|---|---| +| `REVIEW.md` | Review policy: severity recalibration, nit caps, skip rules, repo-specific checks, verification bar, summary shape | Build instructions, architecture notes, anything unrelated to reviewing | +| `ACT_CODE_REVIEW.md` | The ACT layer: organization-wide conventions that apply on top of whatever the repository decided | Repository-specific policy — that belongs in `REVIEW.md` | +| `CLAUDE.md` / `AGENTS.md` | How to work in this codebase: commands, conventions, invariants | Review-only instructions; those dilute every other session | + +Length has a cost. A long `REVIEW.md` dilutes the rules that matter most; keep each layer to +instructions that change reviewer behavior. + +## The `@` import caveat + +`ACT_CODE_REVIEW.md` opens with `@REVIEW.md` so the ACT layer extends rather than replaces the +repository's own policy. Whether that import is expanded depends entirely on the consumer: + +| Consumer | Reads | Expands `@` | +|---|---|---| +| This skill | Every layer, explicitly | Yes — following an `@path` means reading that file | +| `CLAUDE.md` memory system | `CLAUDE.md` | Yes | +| Anthropic managed Code Review | `REVIEW.md` and `CLAUDE.md` only | **No.** `REVIEW.md` is pasted verbatim; referenced files are not pulled in | +| Local built-in `/code-review` | `CLAUDE.md` | Does not read `REVIEW.md` at all | +| GitHub Copilot code review | `.github/instructions/*.instructions.md`, `AGENTS.md` | No | + +Two consequences worth stating plainly rather than discovering later: + +1. **The managed product will not see `ACT_CODE_REVIEW.md`.** For that surface the ACT layer has to + be flattened into `REVIEW.md`. The `install` skill does this as an explicit, visible step and + marks the result as generated. +2. **Copilot needs its own generated file.** Same flattening, into + `.github/instructions/code-review.instructions.md`. + +Anything generated carries a header naming its source and the regeneration step. A generated file +edited by hand is a file that will silently diverge. + +## Conflicts + +When two layers disagree, the later layer wins and no finding is emitted about the disagreement — +that is configuration, not a defect. + +The exception worth surfacing: when a change makes a `CLAUDE.md` statement untrue, that is +reportable as a Nit, because the documentation is now wrong. This runs in both directions — code +that violates the docs, and code that outdates them. diff --git a/plugins/code-reviews/skills/review/references/personas.md b/plugins/code-reviews/skills/review/references/personas.md new file mode 100644 index 0000000..399da80 --- /dev/null +++ b/plugins/code-reviews/skills/review/references/personas.md @@ -0,0 +1,50 @@ +# Personas + +A persona changes the voice of a review. It must never change which findings survive verification +or how they are graded. Tone is the last thing applied and the first thing to drop when it +conflicts with clarity. + +## The default + +Direct, specific, neutral. State the defect, the failure scenario, and a concrete suggestion. No +praise padding, no hedging, no apology. This is what ships unless a repository asks for something +else, and it is the right choice for nearly every team. + +## When a persona helps + +Rarely, and only for internal audiences that opted into it. A distinctive voice can make review +output memorable in a codebase whose contributors already know the reviewer is automated. It is a +morale device, not a quality one. + +## When a persona hurts + +The widely-copied "grumpy reviewer" archetype — `gilfoyle-code-review.instructions.md` in +`github/awesome-copilot` is the best-known example — is instructive precisely because of what it +gets wrong: + +- **It instructs the reviewer not to provide solutions.** A finding without a suggested fix costs + the author a round trip and is strictly worse than one with it. +- **It rewards volume.** A persona built on mockery has an incentive to find something to mock, + which is the exact pressure that manufactures false positives. +- **It buries the defect under the joke.** The reader has to parse the insult to reach the fact. +- **It does not survive an external audience.** Contributors outside the team read it as hostility + from the organization, because that is what it is. + +Adopt it only where every reader is internal and has agreed to it, and never let it override +`references/what-to-report.md`. + +## Applying one safely + +If a repository asks for a persona, put it in `REVIEW.md` under a heading that scopes it to voice, +and state the invariant alongside it: + +```markdown +## Tone + +Write findings in a dry, understated voice. This changes wording only: it does not change +which findings are reported, how they are graded, or the requirement that each names a +concrete failure scenario and a suggested fix. +``` + +That last sentence is the whole safeguard. Without it, a persona instruction competes with the +methodology instead of layering on top of it. diff --git a/plugins/code-reviews/skills/review/references/severity-model.md b/plugins/code-reviews/skills/review/references/severity-model.md new file mode 100644 index 0000000..937ca0d --- /dev/null +++ b/plugins/code-reviews/skills/review/references/severity-model.md @@ -0,0 +1,51 @@ +# Severity + +## The three tiers + +These match what Anthropic's managed Code Review emits, so a finding means the same thing whether +it came from the managed product, a CI job, or a local run. + +| Tier | Emitted when | Author's expected response | +|---|---|---| +| **Important** | Merging this causes incorrect behavior, a security consequence, or data loss | Fix before merge | +| **Nit** | Real and worth fixing, but it will not break anything | Fix if convenient | +| **Pre-existing** | A genuine defect the change did not introduce | Note it; fix separately | + +Nothing above Important exists. A "critical" tier invites inflation, and a reviewer that calls +everything critical is muted within a week. + +## Grading discipline + +**Grade down when torn.** Between Important and Nit, choose Nit. The cost of under-grading is that +someone fixes it next sprint; the cost of over-grading is that the next twenty findings get +skimmed. + +**Severity is about consequence, not effort.** A one-character fix that corrupts billing data is +Important. A large refactor that would be tidier is a Nit at most, and usually nothing. + +**Order beats labels.** Report most severe first. A reader who stops after the third finding should +have seen the three that matter. + +## Recalibrating per repository + +The defaults target production application code. A repository can redefine them in `REVIEW.md`, and +that redefinition wins. Common and legitimate recalibrations: + +| Repository kind | Typical change | +|---|---| +| Documentation or content | Almost nothing is Important; broken links and wrong commands are the exceptions | +| Prototype or spike | Only data loss and credential exposure reach Important | +| Infrastructure as code | Blast radius raises the bar: an unscoped IAM grant or a destructive plan is Important | +| Library with external consumers | Any unversioned breaking change to the public surface is Important | + +Escalation is equally valid: a repository may declare that any violation of a specific +`CLAUDE.md` rule is Important rather than a Nit. + +## Volume + +A review posting thirty nits is a review nobody reads. When a guidance layer caps nits, obey the +cap and report the remainder as a count in the summary. Absent a cap, use judgment: past roughly +five nits, the surplus belongs in the summary rather than inline. + +On a re-review, suppress nits entirely and report only newly introduced defects. A one-line fix +should not reach round seven on style. diff --git a/plugins/code-reviews/skills/review/references/what-to-report.md b/plugins/code-reviews/skills/review/references/what-to-report.md new file mode 100644 index 0000000..5619c9e --- /dev/null +++ b/plugins/code-reviews/skills/review/references/what-to-report.md @@ -0,0 +1,81 @@ +# What to report, and what to stay silent about + +The checklist a review works through, then the standing list of things that must never be reported. + +## Report + +### Correctness + +The largest category and the one worth the most attention. + +- Inverted conditions, off-by-one bounds, wrong operator precedence. +- Unhandled `null` / `undefined` / empty-collection cases on a path that can produce them. +- A function whose behavior differs from what its name, signature, or documentation promises. +- State that is read before it is written, or written after it is read. +- Arithmetic that can overflow, divide by zero, or lose precision where precision matters. +- Early returns and `break`s that skip cleanup the rest of the path depends on. + +### Security + +Flag only with a concrete path from untrusted input to a dangerous sink. The attacker can be any +authenticated user, any network peer, or any untrusted data source — not only an anonymous +outsider. + +- Injection: SQL, shell, path traversal, template, argument injection. +- Missing authorization. For each entry point ask: *if user A submits user B's resource ID, what + stops them?* If the answer is "nothing", that is the finding. +- Secrets or tokens in code, logs, or error messages. +- Unsafe deserialization of attacker-controlled data. +- SSRF: a request whose destination is influenced by input. +- Disabled TLS verification, or crypto assembled by hand. + +### Concurrency and state + +- Read-modify-write without a guard where concurrent callers are possible. +- Missing idempotency where retries occur — a queue consumer, a webhook handler, a CI job. +- Resources acquired without a guaranteed release path. +- Shared mutable state captured by a closure that outlives its expected scope. + +### Error handling + +- Swallowed exceptions and empty catch blocks. +- A catch so broad it hides errors the author never considered. Name which ones. +- Errors logged but not surfaced, so a caller proceeds as though the call succeeded. +- Fallbacks that mask a failure rather than handling it, leaving the system quietly wrong. + +### API contracts + +- Breaking changes to a public interface without a version. +- Serialization or schema drift between producer and consumer. +- Migrations that are not backward compatible with the currently deployed code. + +## Do not report + +This list is not advisory. A finding matching any entry is dropped before the report is written. + +- **Pre-existing issues** the change did not introduce, unless the change makes them reachable or + worse. When one is genuinely worth surfacing, grade it `Pre-existing` and never as a blocker. +- **Anything a linter, formatter, type checker, or compiler catches.** Assume they run in CI. Do + not run them to check. +- **Style and formatting**: naming preferences, import order, line length, comment wording. +- **Pedantic nitpicks a senior engineer would not raise in a real review.** +- **Generic quality observations**: "needs more tests", "could use better docs", "consider + extracting a helper" — unless a specific guidance layer requires it, in which case quote the rule. +- **Speculative issues that depend on inputs or state you cannot show are reachable.** +- **Rules explicitly silenced in the code**, for example behind a lint-ignore comment with a reason. +- **Intentional changes in behavior** that are the evident point of the change. +- **The same root cause reported more than once.** Pick the most representative location. +- **Denial of service through missing limits** — absent timeouts, unbounded loops, no pagination. + These are hardening suggestions, not defects, unless a guidance layer says otherwise. +- **Hardcoded non-secret configuration**: project IDs, table names, bucket names. Only real + credentials count. + +## The test a finding must pass + +Before a finding is written down, it must answer all three: + +1. **What breaks?** One sentence naming the defect. +2. **When?** Concrete inputs or state producing a wrong result, a crash, or a security consequence. +3. **Where?** A `path:line` citation in code that is actually visible, not inferred from a name. + +A candidate that cannot answer all three is not a finding. Discard it silently. diff --git a/plugins/code-reviews/skills/review/templates/ACT_CODE_REVIEW.md b/plugins/code-reviews/skills/review/templates/ACT_CODE_REVIEW.md new file mode 100644 index 0000000..a09dcd7 --- /dev/null +++ b/plugins/code-reviews/skills/review/templates/ACT_CODE_REVIEW.md @@ -0,0 +1,50 @@ +@REVIEW.md + +# ACT review layer + +Organization-wide review conventions, layered on top of this repository's own policy above. Where +the two disagree, this file wins. + +> [!NOTE] +> The `@REVIEW.md` import on the first line is expanded by the `code-reviews` plugin and by the +> `CLAUDE.md` memory system. It is **not** expanded by Anthropic's managed Code Review, which reads +> `REVIEW.md` alone. On that surface, install flattens both layers into `REVIEW.md`. + +## Toolchain + +- **Bun only.** `bun install`, `bun run`, `bunx`, `bun test`. An `npm`, `yarn`, or `pnpm` lockfile + is a finding; `bun.lock` is the only lockfile. +- **No Python.** A `.py` file or an invocation of `python`, `pip`, `uv`, or `poetry` is a finding. +- **No `/tmp`.** Scratch files belong in a gitignored `.tmp/` inside the project. + +## Supply chain + +A new or upgraded third-party dependency is Important unless the change shows it was scored: + +```sh +socket package shallow npm pkg:npm/@ --markdown +``` + +Flag any dimension scoring under 90, naming which one — a low quality score on a build-time +dependency is not the same risk as a low supply-chain or vulnerability score. + +## Repository hygiene + +- **No AI attribution.** A `Claude-Session:` trailer, a "Generated with Claude Code" footer, or an + AI co-author line in a commit message or pull request body is a finding. +- **Conventional commits**: `(): `. +- **No emoji** on ACT-authored surfaces: READMEs, manifests, commands, agents, documentation. Use + GitHub alerts and tables instead. Vendored upstream content is exempt. + +## Instructions are not output + +Text describing *how the work was requested* must not appear in the artifact. Flag any of these in +a shipped file: second person addressed to one reader, "as requested" or "per your instruction", +session status such as "nothing is enabled yet", a count or path taken from one machine used as a +test fixture, or a comment recounting how a bug was found rather than what the code does. + +## Tone + +Direct, specific, neutral. This governs wording only: it does not change which findings are +reported, how they are graded, or the requirement that each names a concrete failure scenario and a +suggested fix. diff --git a/plugins/code-reviews/skills/review/templates/REVIEW.md b/plugins/code-reviews/skills/review/templates/REVIEW.md new file mode 100644 index 0000000..cde1d87 --- /dev/null +++ b/plugins/code-reviews/skills/review/templates/REVIEW.md @@ -0,0 +1,47 @@ +# Review instructions + +Review policy for this repository. Anthropic's managed Code Review reads this file natively and +injects it as the highest-priority instruction block; the `code-reviews` plugin reads it on every +other surface. Keep it to instructions that change reviewer behavior — general project context +belongs in `CLAUDE.md`. + +Delete the sections that do not apply. Every heading below is optional. + +## What Important means here + +Reserve Important for findings that would break behavior, expose data, or block a rollback: +incorrect logic, unscoped database queries, credentials or personal data in logs, and migrations +that are not backward compatible. Style, naming, and refactoring suggestions are Nit at most. + +## Cap the nits + +Report at most five Nits per review. If there are more, add "plus N similar items" to the summary +instead of posting them inline. If every finding is a Nit, open the summary with "No blocking +issues." + +## Do not report + +- Anything CI already enforces: linting, formatting, type errors. +- Generated files and lockfiles. +- Test-only code that intentionally violates production rules. + +## Always check + +Repository-specific rules go here. They land more reliably in this file than in a long `CLAUDE.md`. + +- New public endpoints have an authorization check scoped to the caller. +- Database queries are scoped to the caller's tenant. +- Log lines exclude email addresses, user identifiers, and request bodies. + +## Verification bar + +A behavior claim needs a `file:line` citation in the source. An inference from a function or +variable name is not evidence and is not a finding. + +## Re-review + +After the first review of a change, suppress new Nits and report only newly introduced defects. + +## Summary shape + +Open the summary with a count by severity. Lead with "no blocking issues" when that is true.