feat(code-reviews): add engine-agnostic automated code review plugin - #6
feat(code-reviews): add engine-agnostic automated code review plugin#6patterson-ai wants to merge 5 commits into
Conversation
Add the `glab` command-line interface skill as a reference for all GitLab operations, and introduce automated merge-request review as an engine-agnostic capability across four surfaces: GitLab CI pipelines, git hooks, in-session review, and GitHub Copilot native code review. The review system is built around a stable contract (rubric plus findings JSON schema) with swappable engines (docker-agent, claude, codex, copilot). Deterministic delivery scripts handle all side effects: the engine reviews the diff and produces findings; the wrapper validates and posts to GitLab. Token modes (`inline`, `summary`) post discussions and notes; `log` mode writes to artifacts and the job log when no token is available. Stale reviews resolve on re-push; pipeline retries on the same commit are no-ops. The diff is always untrusted input, so the engine runs with no GitLab credentials and read-only toolsets. This is recorded in ADR 0005. The plugin version moves from 0.2.0 to 0.3.0 across all marketplace entries and manifests.
One rubric and findings contract delivered on four surfaces: a GitLab CI job reviewing every merge request through swappable engines (docker-agent, Claude Code, Codex, Copilot) with inline discussions, a sticky summary and a tokenless log fallback; a git pre-push hook with an engine-dispatching harness; an in-session review command; and GitHub Copilot native review instructions. Posting is deterministic script code pinned by a fixture-driven test suite; engines never receive GitLab tokens. Removes the empty plugins/code-review scaffold, records the design in ADR 0005, and trims act-gitlab-ci back to pipeline tooling (0.2.1, model identifier refresh only).
ai-review.sh becomes codereview.sh and every AI_REVIEW_* variable becomes CODEREVIEW_*. The install directory (.gitlab/codereview/) and artifacts directory (codereview-artifacts/) follow, so the shipped names no longer mix two conventions.
Replaces the bespoke runner with two skills and no runtime. The review methodology - defect checklist, standing false-positive list, three severity tiers, and the bar that a finding must name a concrete failure scenario and cite path:line - becomes prose any agent on any provider can follow. Installation targets the harness each platform already ships: managed Code Review, claude-code-action, the GitLab-maintained Claude Code CI integration, the built-in review command and hooks, Copilot instruction files, and docker-agent, Codex or Copilot CLI. Configuration layers on REVIEW.md, an existing documented convention, with ACT_CODE_REVIEW.md importing it via @ for the organization layer. Posting always uses the host's own tooling. Deletes ~1,600 lines: post-mr-review.ts, codereview.sh, the eighteen CODEREVIEW_* variables, the findings-JSON contract, sticky-marker semantics, and their fixtures. ADR 0006 records the pivot and supersedes 0005.
ba4452f to
0ad40f3
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| 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"; |
There was a problem hiding this comment.
js-yaml is not declared anywhere in the repo — git grep js-yaml returns only this line. package.json has just @types/bun in devDependencies, and bun.lock lists only @types/bun, @types/node, bun-types, undici-types. Neither file is touched by this PR.
scripts/verify-all.sh discovers every run-tests.sh recursively and fails the whole gate on a nonzero exit, and if err=$(bun -e ...) propagates the command-substitution status — so a resolution failure here takes the fail branch for all three matched templates (claude-code-review.yml, gitlab-ci-review-job.yml, review-agent.yaml) and this suite exits 1 at line 116.
That splits by environment, and both halves are a problem:
- After the documented
bun install(which creates anode_modulescontaining only@types/bun), Bun'sinstall = "auto"fallback no longer applies,import "js-yaml"fails Node-style resolution, andsh scripts/verify-all.shprintsVERIFY-ALL: FAILrather thanPASS. - In CI,
.github/workflows/ci.ymlnever runsbun install, so this passes only by network-installing an unpinned, unscored package at gate time. That conflicts with bothbun.lockis the only lockfile and Score new dependencies with socket ... before adding them.
Two ways out: declare js-yaml as a devDependency (with the socket score), or keep the gate dependency-free by using Bun's built-in parser — Bun.YAML.parse(readFileSync(process.argv[1], "utf8")) — and dropping the js-yaml import. The dependency-free route matches the precedent in plugins/act-gitlab-ci/scripts/check-pipeline.ts, which is deliberately a regex scanner rather than a YAML parser for exactly this reason.
| fi | ||
| [ -n "$base" ] || continue | ||
|
|
||
| review "$base...$local_sha" |
There was a problem hiding this comment.
The while read loop at line 33 consumes git's ref-update list from the hook's own stdin, and review is invoked inside the loop with no redirection, so it inherits that still-open pipe. claude -p, codex exec, and copilot -p all read piped stdin as additional prompt input when stdin is not a TTY, so the engine drains the remaining ref lines and the next read hits EOF.
Concrete failure: git push --follow-tags (or git push --all, or git push origin main v1.2.0) hands the hook two or more lines. Ref 1 is reviewed, the raw refs/tags/v1.2.0 <sha> ... line gets injected into the model's input, and ref 2 is never reviewed — silently, since the hook always exit 0s. Single-ref pushes are unaffected, which is why this survives casual testing.
| review "$base...$local_sha" | |
| review "$base...$local_sha" </dev/null |
| --- | ||
| 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 |
There was a problem hiding this comment.
allowed-tools names four tools and no MCP tool, but three files added by this PR state or rely on the opposite:
- This file, line 99, tells the skill to post via
mcp__github_inline_comment__create_inline_commentand themcp__gitlabtools. skills/install/references/github-actions.md:21— "must name the inline-comment tool even though the invoked skill's own frontmatter already allows it".skills/install/templates/claude-code-review.yml:49— "Required even though the skill's own frontmatter allows the tool", sitting above a--allowedToolsthat names only the MCP tool and dropsRead/Grep/Glob(comparegitlab-ci-review-job.yml:45, which keeps them).
Since claude-code-review.yml:48 invokes /code-reviews:review — this skill — both of those comments are false as written. And by this repo's own model of the field (docs/architecture.md:156: "allowed-tools is gone, so the tool restriction silently does not apply"), the list is restrictive, so the GitHub Actions surface yields a review that finds issues and posts none.
Either extend the frontmatter to cover the posting tools the skill body directs it to use:
allowed-tools: Read, Grep, Glob, Bash, mcp__github_inline_comment__create_inline_comment, mcp__gitlab
(naming an MCP tool a host does not provide is harmless — it is simply absent), or keep the frontmatter read-only and delete the false clause from both github-actions.md:21-22 and claude-code-review.yml:49.
| # 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. |
There was a problem hiding this comment.
The first clause is true, but the "so" does not follow: withholding a secret does not stop the event from firing. on: pull_request triggers on fork PRs (opened/synchronize/reopened all fire), and there is no if: anywhere in this file — no job guard, no step condition, no environment gate.
So on a public repo every fork PR does start this job: checkout succeeds, then anthropics/claude-code-action@v1 receives an empty anthropic_api_key (line 43) and fails. GitHub also forces GITHUB_TOKEN read-only for fork pull_request runs, so the requested id-token: write (line 35) is not granted either. Net effect is a red "Code Review" check on every external contribution, looking like a misconfiguration rather than policy — and a maintainer reading this comment would reasonably believe an enforcement mechanism exists.
The security posture itself is fine (this is pull_request, not pull_request_target); only the claimed restriction is missing. Adding the guard on the job at line 26 makes the comment true:
jobs:
review:
# Fork pull requests get an empty key and a read-only token, so skip them
# rather than failing.
if: github.event.pull_request.head.repo.full_name == github.repository
runs-on: ubuntu-latestreferences/github-actions.md:59-62 repeats the same inference and needs the matching correction.
| # 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. |
There was a problem hiding this comment.
"the line below" points at line 26, which is the web rule the same sentence says to keep. The line that must be commented out to leave only the web rule is line 27, merge_request_event.
Followed literally this produces the opposite of the stated intent: commenting out line 26 leaves the two draft when: never rules plus merge_request_event, so the job fires unattended on every non-draft MR — exactly the automatic behavior the comment exists to defer until credentials and posting permissions are confirmed. references/gitlab-ci.md:50-51 confirms the web rule is meant to be the survivor.
| # 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. | |
| # For a first run, comment out the merge_request_event rule below and keep | |
| # only the web rule, so a person confirms credentials and permissions | |
| # before this is automatic. |
| | jq '.[] | {id, body: .notes[0].body}' | ||
|
|
||
| # Build the body in a file, then post it with -F body=@file | ||
| cat > /tmp/reply.md << 'EOF' |
There was a problem hiding this comment.
This new skill writes scratch files to /tmp, which the root CLAUDE.md forbids:
No
/tmp. Scratch goes in the gitignored.tmp/. A workspace hook enforces this.
There are nine occurrences in this file: lines 25, 29, 32, 81, 105, 149, 153, 186, and 191. The near-identical pre-existing plugins/act-gitlab-ci/skills/glab/SKILL.md already uses .tmp/ in all of these same examples, so this reads as a regression introduced when the file was copied. Replacing every /tmp/ with .tmp/ resolves it.
|
|
||
| # 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 ✅. |
There was a problem hiding this comment.
Literal emoji (U+2705) in an ACT-authored skill file. Root CLAUDE.md:
No emoji on ACT-authored surfaces — READMEs, manifests, commands, agents, docs. Use GFM alerts and tables. Vendored upstream reference content under
plugins/*/skills/*/references/andexamples/is exempt
.agents/skills/glab/ matches neither exempt path. The surrounding prose (lines 140-141) only calls out backticks, $, newlines and a leading @ as the things that break when inlined, so the emoji is not load-bearing for this example — the pre-existing sibling plugins/act-gitlab-ci/skills/glab/SKILL.md uses a trailing backslash here instead.
| @user — here's the result, with `code`, a $variable, and an emoji ✅. | |
| @user — here's the result, with `code`, a $variable, and a trailing backslash \. |
2 participants
What
A new standalone
code-reviewsplugin (0.1.0): automated AI code review across GitHub, GitLab, and local agents, built around one review rubric and one findings JSON contract with pluggable engines.Four surfaces, one contract:
post-mr-review.tsruns an engine over the diff and posts inline discussions + a sticky summary (inline/summary/logmodes; tokenless log fallback sinceCI_JOB_TOKENcannot create MR notes)ai-review.sh, an engine-dispatching POSIX harness (advisory by default)/code-reviews:review-mr+ portable adapter skills.github/instructions/file carrying the rubricEngines (
AI_REVIEW_ENGINE):docker-agent(default, provider-agnostic, pinned standalone binary — no Docker daemon),claude,codex,copilot, or any command viaAI_REVIEW_ENGINE_CMD.Security model: the diff under review is untrusted input to an unattended model. The shipped docker-agent config is read-only (no shell/network/MCP), both scripts strip
GITLAB_TOKEN/GITLAB_ACCESS_TOKEN/CI_JOB_TOKENfrom every engine environment (tested with a positive-control probe), and all posting is deterministic script code. The reviewer never blocks a merge (allow_failure: true).Also in this change
plugins/code-review/scaffold shell.act-gitlab-ciback to pipeline tooling (0.2.1 — model identifier refresh in two examples only); the actor-vs-reviewer boundary is documented in both plugins.code-reviews, the one deviation from theact-*prefix, avoiding a collision with the upstream Anthropiccode-reviewplugin this repo's own workflow invokes).Test plan
sh scripts/verify-all.sh→VERIFY-ALL: PASS(5 plugins, tri-runtime catalogs consistent)sh plugins/code-reviews/scripts/tests/mr-review/run-tests.sh→ 21 checks + unit suite, all fixture-driven (no network, no engines): ndjson/claude envelope parsing, position mapping, HTTP 400 fallback, mode downgrade, typed-marker stickiness, re-push resolution, draft skip, config-typo exit codes, token strippingclaude plugin validate .andclaude plugin validate plugins/code-reviewspassStacked on #3 (
docs/add-documentation-tree); retarget tomainafter #3 merges.