Skip to content

Latest commit

 

History

History
259 lines (201 loc) · 25 KB

File metadata and controls

259 lines (201 loc) · 25 KB

PLAN — code-review-agent

LLM-first, multi-language code & CI/CD review agent on LangGraph, driven by portable Agent Skills (the open SKILL.md format, originated by Claude). Keep in sync with §1 of CLAUDE.md.

Living document: captures the design before code is written, then doubles as the build checklist.


Architecture

diff source (Typer CLI `git diff` · stdin · CI job in the worker container)
  └─► LangGraph StateGraph:
        ingest ─► detect ─► [Send fan-out: one ReviewUnit per resolved skill]
                                   └─► review ─┐
                                   └─► review ─┤─► aggregate ─► report ─► END
                                   └─► review ─┘
  • ingest — parse the git/piped diff into ChangedFiles; apply ignore globs; attach full new-side text (new_content) for modified/renamed files via a ContentResolver. Two resolver variants: git_show_resolver(head_ref) reads git show <head>:<path> (correct for explicit base...head / base..head runs — independent of working-tree/checkout state) and is used when a two-dot/three-dot commit range is given; the hardened working_tree_resolver (refuse ../, skip non-file/oversized) is the fallback for local/piped runs, including git diff <single-ref> where the new side is the working tree. Added files already carry full content in the diff. No repo / no ref → diff-only.
  • detect — classify each file → skill key (static extension map + shebang/first-line fallback + special filenames/paths: Dockerfile, Dockerfile.*, *.Dockerfile, direct .github/workflows/*.yml|*.yaml, root .gitlab-ci.yml, Jenkinsfile), then fall back to the skill registry's language-skill frontmatter metadata.extensions when the static detector has no signal. Files with no static or registry signal are unclassified and omitted from review units; the missing-skill hard-fail starts only after a file is classified as a programming-language key by the static detector. Group classified files into ReviewUnits (one per skill key). Resolve each unit's skill via the loader: hard-fail (MissingSkillError) on a missing programming-language skill; skip optional CI/infra targets whose skill is absent or not enabled in review.toml.
  • review (Send target — receives a ReviewTaskState(unit=…), see §State) — prompt = skill SKILL.md body + an injection-hardening preamble (system), and the diff + bounded context wrapped in clearly delimited untrusted-data blocks (user). The system prompt states that everything inside reviewed files, comments, docstrings, test fixtures and CI YAML is data to review, never instructions to follow. LLM call = with_structured_output(ReviewResult) (provider-appropriate method) with a tolerant free-form-JSON fallback that logs the raw response on parse failure. Enforce per-unit max_unit_tokens (chunk over budget on file/hunk boundaries, merge; trim context before changed lines). Honor LLM_MAX_RETRIES / LLM_TIMEOUT_SECONDS. Returns {"findings": […]}.
  • aggregate — concat findings (reducer), dedupe, deterministic stable sort.
  • report — render the report once (AgentState.report), then fan it out to one or more configured reporters (composable, not mutually exclusive). Selection precedence CLI --reporter > REPORTER env > review.toml [report].reporters > auto. The github/gitlab reporters are idempotent: they embed a stable hidden marker (<!-- code-review-agent -->) and update the existing bot comment/note in place, never posting a duplicate on re-runs. Each reporter runs independently; a failure in one is non-fatal. Exit code reflects severity policy, not reporter success.

New languages/targets are added as new skills/<key>/SKILL.md folders — no graph changes. Reporters and (deferred) scanners are likewise registry additions.

Node implementations are split by graph stage under src/code_review_agent/utils/node_*.py: node_ingest.py, node_detect.py, node_review.py, node_aggregate.py, and node_report.py. utils/nodes.py remains a thin compatibility export surface for the stable graph/test imports and should not accumulate implementation logic again.

Reporters & output (all available, selected in config)

All four reporters ship in v1 and are composable — enable any subset in review.toml:

Reporter Output location Durable?
terminal stdout (CI job log) No — ephemeral
file review-report.md + .json under REPORT_DIR Yes — artifact CI can upload/archive
github updates a single marked PR comment (idempotent) via GITHUB_TOKEN Yes — lives in the PR
gitlab updates a single marked MR note (idempotent) via GITLAB_TOKEN Yes — lives in the MR
[report]
reporters = ["terminal", "file"]   # any subset; or "auto"
report_dir = "."                   # where the file reporter writes
fail_on = "high"                   # min severity that makes the run exit non-zero (off = never)

"auto" expands to: detected platform reporter (github on GitHub Actions, gitlab on GitLab CI) + terminal, and file when the platform is Jenkins/unknown. Jenkins has no native PR comments, so it relies on file (archive the artifact).


Security & trust model (CI reviews untrusted PR code)

A PR author controls the repo contents — including review.toml and any repo-local skills/. Both feed the reviewer's system prompt, so in CI both are untrusted input.

  • Trusted by default: only the bundled skills/ (baked into the image, SKILLS_PATH) and the bundled/base-ref review.toml are trusted.
  • Config from a trusted ref: in CI, review.toml is read from the base/target ref (TRUSTED_CONFIG_REF, e.g. the PR base), not the PR head — a PR can't rewrite its own review rules. Local runs read the working-tree review.toml.
  • Repo-local extra skills are opt-in: review.toml [skills].extra_paths (and any repo-provided skill dirs) are ignored unless ALLOW_REPO_SKILLS=true — an env var only the CI operator can set, not the repo. Off by default.
  • Diffs are untrusted data: the review prompt is injection-hardened (delimited content + explicit "data, not instructions"). Mitigation, not a guarantee — keep the worker's token scope and permissions least-privilege.
  • Skills stay prompt-only (no script execution); the agent never writes to the reviewed repo.
  • Config sources are fail-closed in CI (Phase 2, config.py): operator settings come from real env vars (which always win); the .env file is a local-dev convenience only and is not loaded under CI (CI/GITHUB_ACTIONS/GITLAB_CI/JENKINS_URL), so a PR-supplied checkout .env cannot set SKILLS_PATH/ALLOW_REPO_SKILLS/TRUSTED_CONFIG_REF. In CI with no TRUSTED_CONFIG_REF, reading review.toml raises UntrustedConfigError rather than silently reading the PR-controlled working tree (set the ref to a trusted base, or to the current ref to explicitly opt in).
  • Two distinct config paths (don't conflate them): REVIEW_CONFIG is a filesystem path used for local/bundled reads (may be absolute, e.g. /app/review.toml). The trusted-ref read uses a separate repo-relative TRUSTED_CONFIG_PATH (default review.toml) via git show <ref>:<path> — an absolute filesystem path is not a valid git show repo path, so the two namespaces must stay separate (regression fixed in Phase 2: a container with absolute REVIEW_CONFIG + a trusted ref was silently falling back to default config).
  • Residual (entrypoint, Phase 7/13): the SKILLS_PATH/REVIEW_CONFIG defaults are cwd-relative; if the entrypoint runs with cwd inside the checkout, ./skills/./review.toml resolve into PR content. The Docker image/CI must pin SKILLS_PATH (and REVIEW_CONFIG, the filesystem fallback) to bundled absolute paths via real env vars, run with cwd outside the reviewed checkout (review via an explicit repo path), mount the checkout read-only, and mount a separate writable REPORT_DIR for file artifacts. Note REVIEW_CONFIG being absolute is fine — only TRUSTED_CONFIG_PATH is fed to git.

Decisions locked

Question Choice
Framework LangGraph v1 StateGraph + Send fan-out (not create_agent/ReAct — pipeline is orchestrated)
Topology ingest → detect → Send fan-out → review → aggregate → report → END
Node modules Implementation split by stage in utils/node_*.py; utils/nodes.py only re-exports the public node surface
Fan-out state each Send("review", ReviewTaskState(unit=u)); review node's input schema = ReviewTaskState, returns {"findings": …} merged via the add reducer
LLM provider openai default · gpt-5-mini · switchable to anthropic/google
Temperature 0.0, silently omitted for gpt-5*/reasoning models that reject it
Skills engine Portable loader — read SKILL.md frontmatter (L1) + body (L2), inject into the prompt for any provider. NOT Claude's code-execution runtime.
Skill content Prompt-only (knowledge packages). No execution of skill-bundled scripts/.
Missing skill programming language → MissingSkillError, fail run. CI/infra target → skip unless present and enabled.
Structured out with_structured_output(ReviewResult), per-provider method; tolerant JSON fallback that logs raw response on parse failure
Trust model bundled skills + base-ref review.toml trusted; repo-local extra skills off unless ALLOW_REPO_SKILLS=true; diffs treated as untrusted data (injection-hardened prompt)
Seed skills language: python, javascript (.js/.jsx/.ts/.tsx), java. optional CI/infra: dockerfile, github-actions, gitlab-ci, jenkins.
Config file review.toml — enabled optional skills, extra skill paths, ignore globs, token budget, reporters list
Reporters all four ship; composable + idempotent PR/MR comments; subset selected via config (CLI > env > review.toml > auto); each independent, failures non-fatal
Review scope changed hunks + bounded surrounding context (not whole repo)
Checkpointer off (one-shot run)
Distribution Typer CLI + platform-neutral worker container; one or more reporters chosen via config; thin CI wrappers in examples/
Trigger local git diff / stdin · CI job (GitHub Actions / GitLab CI / Jenkins)

State (Pydantic v2 sketch)

from typing import Annotated, Literal
from operator import add
from pydantic import BaseModel, Field

Severity = Literal["info", "low", "medium", "high", "critical"]
Category = Literal["bug", "security", "performance", "improvement"]
ChangeKind = Literal["added", "modified", "renamed", "deleted"]
ProviderName = Literal["openai", "anthropic", "google"]
FailOnThreshold = Literal["off", "info", "low", "medium", "high", "critical"]

class ChangedFile(BaseModel):
    path: str
    kind: ChangeKind
    diff: str                              # unified hunks
    new_content: str | None = None         # full new-side text for modified/renamed (else None)

class SkillRef(BaseModel):
    key: str                               # e.g. "python", "github-actions"
    name: str
    description: str
    kind: Literal["language", "ci"]
    path: str                              # SKILL.md location (body loaded lazily)

class ReviewUnit(BaseModel):
    skill: SkillRef
    files: list[ChangedFile]

class Finding(BaseModel):
    path: str
    line: int | None = None
    severity: Severity
    category: Category
    title: str
    detail: str
    skill_key: str

class ReviewResult(BaseModel):             # structured-output wrapper the LLM returns
    findings: list[Finding] = Field(default_factory=list)

class ReviewTaskState(BaseModel):          # INPUT state for ONE review fan-out branch (the Send payload)
    unit: ReviewUnit
    llm_provider_override: ProviderName | None = None
    llm_model_override: str | None = None
    findings: Annotated[list[Finding], add] = Field(default_factory=list)

class AgentState(BaseModel):               # overall graph state
    diff: str = ""
    repo_root: str | None = None
    head_ref: str | None = None            # set for base...head runs → selects git_show_resolver
    llm_provider_override: ProviderName | None = None
    llm_model_override: str | None = None
    reporter_override: str | None = None
    fail_on_override: FailOnThreshold | None = None
    files: list[ChangedFile] = Field(default_factory=list)
    units: list[ReviewUnit] = Field(default_factory=list)
    findings: Annotated[list[Finding], add] = Field(default_factory=list)  # fan-out reducer
    report: str = ""

detect populates units. The fan-out edge maps each unit to Send("review", ReviewTaskState(unit=u, ...)); the review node declares ReviewTaskState as its input schema and returns {"findings": [...]}, which merges into AgentState.findings via the add reducer. CLI override fields enter through AgentState; provider/model overrides are copied into each review branch, while reporter/fail-threshold overrides remain on AgentState for the report/CLI phases. Branches touch only findings; all other shared fields are set pre-fan-out. No checkpointer.


Integration contract

  • Inputs: unified diff via stdin or produced from git diff / base...head (CI: new-side content read via git show <head>:path); review.toml (in CI, from the trusted base ref); env vars per CLAUDE.md §5; bundled skills/ (repo-local extra skills only when ALLOW_REPO_SKILLS=true).
  • Outputs: the side effects of every configured reporter (any subset of terminal text · file artifact review-report.md+.json · idempotent GitHub PR comment · idempotent GitLab MR note). The rendered report string is identical across reporters. Findings are advisory.
  • Required secrets: the LLM key matching DEFAULT_LLM_PROVIDER; for github/gitlab reporters, the platform token from CI env (GITHUB_TOKEN / GITLAB_TOKEN).
  • Failure modes / exit codes: 0 clean (or findings below fail_on); non-zero when findings meet the configured severity threshold; non-zero + MissingSkillError message when a programming-language skill is absent; LLM/transport errors after retries → non-zero. Reporter failures log but do not change the review exit code.

Build phases

Ship phases in order; don't start the next until make fmt lint type test is green.

Phase 1 — Scaffolding

  • .python-version, .env.example, .gitignore, .dockerignore, review.toml sample
  • langgraph.json./src/code_review_agent/agent.py:agent
  • Dockerfile (python:3.13-slim, non-root, bundles skills/, entrypoint = CLI) + docker-compose.yml
  • src/code_review_agent/ package skeleton with placeholder start → end graph in agent.py
  • make install fmt lint type test green; uv.lock committed

Phase 2 — Config + LLM factory

  • config.pyBaseSettings (env, incl. ALLOW_REPO_SKILLS, TRUSTED_CONFIG_REF) + review.toml loader (tomllib) reading from the trusted ref in CI (base), the working tree locally
  • llm.pyget_llm(provider, model, temperature); omit temperature for gpt-5*/reasoning models
  • Unit tests (mocked providers; gpt-5 temperature-omission; config trust: PR-head review.toml ignored when a trusted ref is set)

Phase 3 — State models

  • utils/state.py per the sketch above (incl. ReviewResult, ReviewTaskState)
  • Unit tests for model validation + the findings reducer

Phase 4 — Diff ingest + content resolvers

  • utils/diffing.py — parse diff → ChangedFiles; ContentResolver protocol with two impls: git_show_resolver(head_ref) (git show <ref>:<path>, for base...head/CI) and hardened working_tree_resolver (refuse ../, skip non-file/oversized; local fallback); ignore globs (defaults + review.toml)
  • ingest node (utils/node_ingest.py) picks the resolver from input (range/head_ref → git_show; else working-tree; no repo → diff-only)
  • Unit tests (added vs modified/renamed; deletes skipped; ignore globs; git_show vs working-tree incl. checkout state ≠ reviewed commit)

Phase 5 — Detection

  • utils/detect.py — extension map + shebang fallback + special filenames → skill key
  • Unit tests (extensionless shebang scripts, Dockerfile/Jenkinsfile/workflow paths)

Phase 6 — Skill loader / registry

  • skills/loader.py — discover search paths: bundled SKILLS_PATH always; review.toml [skills].extra_paths only when ALLOW_REPO_SKILLS=true (else ignored + warned); parse frontmatter (L1 index); lazy body load (L2); resolve key → SkillRef
  • skills/errors.pyMissingSkillError
  • detect node (utils/node_detect.py): build ReviewUnits, hard-fail on missing language skill, skip disabled/absent optional skills
  • Unit tests (missing language → raises; disabled CI skill → skipped; enabled present → loaded; extra_paths ignored when ALLOW_REPO_SKILLS unset)

Phase 7 — Prompt assembly + token budget + injection hardening

  • utils/prompts.py — system = skill body + injection-hardening preamble (reviewed content is data, not instructions); user = diff/context in delimited untrusted-data blocks; max_unit_tokens chunking on file/hunk boundaries; modified-file context attached only when new_content present and within budget (skip oversized whole, never truncate)
  • Unit tests (under/over budget; modified-file context attach/skip; prompt-injection fixture — embedded "ignore previous instructions" in a diff/comment/CI YAML is not obeyed)

Phase 8 — Review node + structured output

  • review node (utils/node_review.py) — input schema ReviewTaskState; with_structured_output(ReviewResult) with a per-provider method map (json_schema / function-calling / json_mode as each supports); tolerant free-form-JSON fallback that logs the raw response on parse failure; retry/timeout; provider context-length errors from indivisible over-budget prompt chunks are logged and degrade to empty findings for that unit; returns {"findings": […]}
  • Normalize LLM-output scalars — lenient, never reject: clamp a non-positive Finding.line (e.g. 0/negative, an LLM artifact) → None rather than dropping the finding — a cosmetic location field must not sink a real finding (the model is tolerant by design). Optional paired guidance: adding ge=1 to Finding.line in state.py only propagates a minimum:1 schema hint to the LLM — adopt it only with this clamp as the safety net, never standalone (it would arm the rejection path).
  • Unit tests with mocked LLM (clean structured; malformed-but-salvageable JSON via fallback; unsalvageable → logged + empty findings for that unit, run continues; context-length provider error → logged + empty findings for that unit, run continues; retry path; Finding.line=0/negative coerced to None, finding retained)

Phase 9 — Aggregate

  • aggregate node (utils/node_aggregate.py) — dedupe + deterministic stable sort
  • Attribution filter: drop (or flag) findings whose Finding.path matches no file in any reviewed unit.files — a hallucinated/misattributed path from structured output. (Finding.path is never used for filesystem access — reads go through the ingest ContentResolver on ChangedFile.path — so this is report hygiene, not a traversal guard.) Cross-object, so it lives here where all units + findings are in scope.
  • Unit tests (dedupe; deterministic stable sort incl. line=None ordering; out-of-scope path dropped/flagged)

Phase 10 — Graph wiring

  • agent.pyStateGraph; START → ingest → detect; conditional Send fan-out → review; review → aggregate → report → END
  • Node implementation refactor — graph stages live in focused utils/node_*.py modules; utils/nodes.py re-exports the public node names for compatibility
  • Integration test: compiled graph end-to-end with mocked LLM + a recorded diff fixture

Phase 11 — Reporters: registry + terminal + file

  • reporters/ registry that runs a composable list (each independent, failures non-fatal); report node resolves the list (CLI > env > review.toml > auto)
  • terminal + file (review-report.md + .json under report_dir) reporters; advisory disclaimer
  • Unit tests (multi-reporter dispatch; one reporter failing doesn't block others)

Phase 12 — Reporters: github + gitlab + auto-detect

  • github + gitlab reporters — idempotent: find the existing bot comment/note by a stable hidden marker (<!-- code-review-agent -->) and update in place, else create; auto expansion from GITHUB_ACTIONS/GITLAB_CI/JENKINS_URL (platform reporter + terminal; +file on Jenkins/unknown)
  • Unit tests (mocked HTTP/token; auto resolution per platform; comma-separated override; re-run updates the same comment, no duplicate)

Phase 13 — CLI + entrypoint

  • cli.py — Typer app: read diff (stdin or git diff/range; two-dot/three-dot ranges set head_ref, while a single ref uses working-tree context), flags --reporter/--config/--provider/--model/--fail-on/--allow-repo-skills; wire to the compiled graph; exit codes per contract
  • Trust hardening (residual P1): entrypoint runs with cwd outside the reviewed checkout (review via an explicit repo path / git -C); Docker image + CI examples pin SKILLS_PATH/REVIEW_CONFIG to bundled absolute paths via real env vars so a PR-supplied checkout .env/skills//review.toml is never sourced. (config.py already drops .env under CI and fails closed without TRUSTED_CONFIG_REF — Phase 2.) Surface UntrustedConfigError as a non-zero exit.
  • Unit tests (arg parsing, range → git_show resolver, exit codes)

Phase 14 — Seed language skills

  • skills/python/SKILL.md, skills/javascript/SKILL.md, skills/java/SKILL.md (expert-reviewer personas; valid frontmatter name/description/metadata)

Phase 15 — Optional CI/infra skills

  • skills/dockerfile, skills/github-actions, skills/gitlab-ci, skills/jenkins SKILL.md (config-gated)

Phase 16 — CI wrapper examples

  • examples/github-action/action.yml (+ example-workflow.yml), examples/gitlab-ci/.gitlab-ci.yml snippet, examples/jenkins/Jenkinsfile stage (all invoke the same container/CLI; show TRUSTED_CONFIG_REF/artifact archiving) + examples/README.md documenting the shared container contract
  • Container must run with cwd = the checkout so config.py's bare git show <base>:review.toml resolves (the diff content resolver uses cwd=repo_root, but the trusted-config read does not). cwd outside the checkout → trusted-ref read silently degrades to default config. GitHub: docker run -w /workspace; GitLab/Jenkins: the job already runs in the clone dir.
  • Dockerfile installs git — runtime dep for the CLI/config git diff/git show (python:3.13-slim omits it; CI clones also need it). Examples set safe.directory (git "dubious ownership") and pass CI markers into the container (docker run does not inherit the runner env).

Phase 17 — Tests + polish

  • End-to-end fixture integration test across multiple languages + one CI target
  • README.md — setup, CLI usage, CI wiring, trust model, writing a new skill, bulding docker image locally, running docker image locally
  • make fmt lint type test green on a clean checkout; tag v0.1.0 (checks green; tag deferred)

Open considerations

  • Optional scanner tool registry (deferred) — vetted, config-gated adapters (ruff/bandit/eslint/gosec/semgrep/actionlint/hadolint/shellcheck) run only if on PATH; findings merged as extra LLM signal. Kept separate from skills (skills stay prompt-only). Versions pinned in the Action Dockerfile only.
  • Severity fail-threshold defaultfail_on default (high?) and per-CI overridability.
  • Token estimation~4 chars/token heuristic (no tokenizer dep); revisit if it proves loose.
  • ObservabilityLANGSMITH_TRACING in non-dev; structured-log schema for findings.
  • AGENTS.md / CLAUDE.md — kept as a single source of truth (AGENTS.md points at CLAUDE.md) to prevent the find-replace drift seen on 2026-05-25.