AUR-430: `aurumcode review`

Documento histórico do board. Pode descrever componentes removidos; para uso atual, consulte o guia do produto.

Restores AurumCode's local code-review engine (deleted 2025-11-03, commit c12d7ab is the last commit before deletion) and wires it behind a new CLI entrypoint, cmd/aurumcode.

Command

aurumcode review --base <ref>

Run from inside the git repository you want reviewed — a normal working tree, a linked worktree (git worktree add), or a bare repository, with loose or packed objects. It diffs <ref> against HEAD in that repository, sends the diff to a model, and prints every finding the model reports as one line per finding:

<file>:<line>: [<severity>] <message>

sorted by (file, line) so the same input always prints in the same order. If the model reports no findings, it prints No issues found.. Running the command twice against the same input always produces byte-identical output.

Flags

Flag	Required	Meaning
`--base <ref>`	yes	The ref to diff against `HEAD`. Accepts a 40-hex commit SHA, a branch name (resolved under `refs/heads/`), `HEAD`, or any of those with a `~N` parent-walk suffix (`HEAD~1`, `main~2`, ...).

Exit codes

Code	Meaning
`0`	The command ran and printed its findings (including zero findings).
`1`	A real failure: not a git repository, the ref does not resolve, the LLM call failed, or the model's response could not be understood at all (see "Parser fallback" below).
`2`	Usage error: unknown subcommand, or `review` invoked without `--base`.

Provider configuration

aurumcode review never hardcodes a vendor. It selects a provider from the environment, in this order:

AURUMCODE_LLM_FIXTURE=<path> — read that file's content and return it verbatim as the model's response. This is how the acceptance test and the example below run fully offline and deterministically; it is not a special case in the review engine, just an llm.Provider implementation (internal/review.FakeProvider) like any other.
LLM_API_KEY and LLM_BASE_URL — use the existing OpenAI-compatible LiteLLM proxy provider (internal/llm/provider/litellm), the same one cmd/regenerate-docs already uses, naming a model via LLM_MODEL (default gpt-4).
Neither set: the command fails with a clear error naming both options, never a panic or a silent no-op provider.

Example (no secrets)

$ cd tests/fixtures/repos/git-demo/repo.git   # a deterministic fixture repo
$ AURUMCODE_LLM_FIXTURE=../../review/known-problem-response.json \
    go run ../../../../../cmd/aurumcode review --base HEAD~1
config/demo-tokens.txt:4: [error] A credential-shaped value was committed in plain text (DEMO_API_TOKEN).

The fixture response and the values it describes (AURUM-FAKE-TOKEN-0000-0001 and friends) are synthetic; see tests/fixtures/repos/git-demo/content/03/config/demo-tokens.txt.

Limits on what gets diffed

Not every changed file can be line-diffed safely, so aurumcode review refuses some of them before reading their content into lines. A refused file is left out of the diff entirely — no model call is made for it, and none of its content reaches a prompt — and is reported on stdout instead:

$ aurumcode review --base HEAD~1
diff too large: generated/schema.sql (12480 lines > limit 10000)
diff too large: dist/bundle.min.js (3145728 bytes > limit 1048576)
binary file, skipped: assets/logo.png
src/auth.go:88: [warning] Error from the token refresh path is discarded.

Skipping is a normal outcome, not a failure: the notices print before the findings, in the same sorted path order the diff uses, and the exit code is still 0.

Limit	Value	Applied to
Lines per side	10000	The old and new content of one file, independently.
Bytes per side	1 MiB	Same. Catches minified or generated single-line files, which pass the line limit trivially.
Binary	a `NUL` byte in the first 8000 bytes	Git's own heuristic and its own window.

The limits exist because the cost of a line diff is driven by the edit distance D, not by file size alone. The engine records the shortest-edit- script search trace, and that trace grows as D²; left unbounded it made reviewing a single 12674-line added file cost about 2.6 GB, and a ~22k-line file could exhaust a 31 GB host. Two independent bounds now prevent that:

This input cap, which rejects files outright, with the messages above.
A trace budget (maxTraceCells in internal/analyzer/textdiff.go, 4Mi cells ≈ 32 MiB). A file under the input cap can still have a large edit distance — two 10000-line versions sharing nothing — so the search itself is budgeted too. Past the budget the engine stops searching for the minimal script and renders the change as a whole-file replacement: every old line removed, every new line added. That output is correct, just coarser, and it costs O(N+M).

Budgeting the trace rather than pre-rejecting on worst-case size is what keeps the common case exact: a one-line change inside a 10000-line file has an edit distance of 2 no matter how long the file is, so it is diffed precisely and costs almost nothing. Peak memory for the differ is bounded by the trace budget regardless of input, and files within both bounds produce byte-for-byte the same diff they always did — internal/analyzer checks that against the previous full-width implementation over every pair of strings up to length 6, and textdiff_memory_test.go fails if the unbounded shape returns.

Diagnosing a failure

Symptom	Likely cause
`aurumcode review: <dir> is not a git repository`	`cwd` is not a git repository: no `git` binary recognizes it, no `.git` directory, and it is not itself a bare repository (no `HEAD`/`objects`/`refs`).
`aurumcode review: computing diff <ref>..HEAD: ...object ... not found as a loose object (packed objects are not supported by this reader)`	No `git` binary was found on `PATH`, so the pure-Go fallback reader is in use, and it only reads loose objects; see "Reading a repository" below.
`aurumcode review: no LLM provider configured: ...`	Neither `AURUMCODE_LLM_FIXTURE` nor `LLM_API_KEY`+`LLM_BASE_URL` is set.
`aurumcode review: could not understand the model's response (no_json_found\\|invalid_json\\|validation_failed\\|no_findings_recovered)`	The model's response had neither valid JSON nor a recognizable degraded-mode finding line (see below).

Design decisions

Reading a repository: two backends, selected once

aurumcode review must work both against a real, lived-in checkout — a linked worktree, where .git is a file, not a directory (this very card was built in one); a repository whose objects git gc has packed (this board's own AurumCode checkout is one) — and inside this card's sealed acceptance profile, bootstrap-readonly-v1, which carries a Go toolchain and bash but no git binary and no network. One implementation cannot satisfy both for free, so internal/analyzer/gitrepo.go's OpenRepo picks a backend once, and every other method (ResolveRef, Diff, the tree walk) is written against that single choice:

git binary present (the common case on a real machine): every read goes through git rev-parse and git cat-file, run with the reviewed repository's own directory as the subprocess's working directory and no other flags — this is exactly the os/exec approach this card's own text describes. Letting git itself discover the repository is what makes a linked worktree and a git gc'd repository work at all: git already understands .git-as-a-file indirection and packfiles; reimplementing either in this reader would just be a worse copy of git.
No git binary (this card's own acceptance sandbox — the same fact card AUR-011's acceptance program independently documents and relies on for the same fixture): a pure-Go reader inflates loose objects directly (zlib + the standard type size\0content header) and resolves refs by reading the loose ref files, then walks commit → tree → blob to build the diff itself, using a from-scratch Myers O(ND) line diff (internal/analyzer/textdiff.go) for each changed blob. This path is scoped to loose objects only — the shape every fresh commit, and the tests/fixtures/repos/git-demo fixture, uses — and returns a clear, typed error naming the object when it meets a packed one, never a silently empty diff.

Both backends return the same information through the same Repo methods, so Diff, the tree walk and the line diff are written and tested once. internal/analyzer/gitrepo_test.go runs entirely on the pure-Go path (this repository's own sandbox has no git); internal/analyzer/gitrepo_gitbinary_test.go builds a real repository — including a linked worktree and a git gc'd one — and runs the git-binary path against it whenever a git binary is available to run it with, skipping (never silently passing) when one is not.

Prompt sourced from `.aurumcode/prompts/review.md`

internal/prompt/templates/review.md is a build-time copy of the read-only .aurumcode/prompts/review.md. go:embed patterns cannot reach outside their own source file's directory tree, so a runtime read of the read-only path was the only alternative — but that would tie the binary's behavior to running with a particular working directory or a repo-relative env var, which a CLI meant to review an arbitrary target repository should not require. Baking the content in at build time keeps the binary self-contained regardless of where it runs, at the cost of the copy needing to be refreshed if .aurumcode/prompts/review.md changes (out of this card's scope to automate).

Parser fallback

The restored internal/prompt.ResponseParser.ParseReviewResponse used to have exactly one path: parse the model's response as JSON, or fail the entire review with no output. That is a real usability problem for smaller or local models that cannot reliably follow a JSON schema.

ParseReviewResponse now has two paths:

JSON (unchanged behavior for a well-formed response): extract JSON from a fenced code block or a bare {...}, repair common malformations (see repairJSON below), decode and validate it.
Degraded fallback: if step 1 fails for any reason, scan the response line by line for the convention

[-*] <path>:<line>: [severity:] <message>

(severity defaults to warning when omitted) and turn every match into a types.ReviewIssue. If that recovers at least one finding, it is returned as a normal, successful result with Metadata["parse_mode"] == "degraded".

If neither path produces anything, ParseReviewResponse returns a typed *prompt.ParseError (with a Kind — no_json_found, invalid_json, validation_failed, or no_findings_recovered — and a bounded preview of the raw response) rather than an empty-but-successful result. This distinction matters: an empty []ReviewIssue must always mean "the model looked and found nothing," never "the engine gave up and said nothing."

`repairJSON`: quotes are repaired, not blindly swapped

The restored repairJSON translated typographic ("smart") double quotes (“ ”) to straight ASCII quotes with a global string replace over the entire response, including inside already-open JSON string values. That is a real bug: substituting inside a string value inserts a bare, unescaped " that ends the string early and corrupts everything after it.

repairJSON now tracks, character by character, whether it is currently inside a JSON string. A quote character — straight or typographic — only closes the current string when it looks like a genuine boundary (the next non-whitespace character is one of , } ] :, or end of input); any other quote met inside a string is escaped in place instead. Typographic single quotes/apostrophes (‘ ’) are never JSON delimiters, so they are still normalized with a plain global replace. See TestRepairJSON/smart_quotes and TestRepairJSON_TypographicQuotesInsideStringsDoNotCorruptJSON in internal/prompt/parser_test.go.

`ISOScores` is optional

pkg/types.ReviewResult.ISOScores is *ISOScores today (the restored c12d7ab code treated it as a value field). This card's engine never asks a model to score ISO/IEC 25010 characteristics — that scoring pipeline (internal/review/rules.go, internal/review/iso25010) is explicitly out of scope here and belongs to dependent cards — so a response that omits iso_scores is valid and ISOScores stays nil; a block that is present is still validated to the 1–10 range.

No vendor coupling

internal/analyzer, internal/prompt and internal/review name no LLM vendor anywhere. The engine's only contact point with a model is internal/llm.Orchestrator.Complete(ctx, prompt, llm.Options), called with whatever llm.Provider cmd/aurumcode's selectProvider constructed — internal/review.FakeProvider for the offline/fixture path, or the existing vendor-neutral internal/llm/provider/litellm.Provider for a live one. internal/review/reviewer_test.go, tests/unit/AUR-430.go, tests/integration/AUR-430.go and the acceptance program all run the real review pipeline through FakeProvider and never touch a vendor package; that is the proof, not just the claim, that the pipeline is provider-agnostic.

MUT-001 (the skeptical mutation) is applied to a staged copy, not to production

tests/acceptance/AUR-430.sh's mutation_case proves the card's declared mutation — "return an empty issue list for a diff with a known problem" — by editing a writable, throwaway staged copy of reviewer.go with sed (nulling the issue slice right before GenerateReview's final return), rebuilding a second binary from that mutated copy, and asserting the known-problem diff then comes back with No issues found.. The committed source is never touched, so this is reversible by construction — there is nothing to "restore" afterward. An earlier draft of this card instead read an AURUM_A430_MUTATION environment variable inside GenerateReview itself; that shipped a runtime kill-switch in the production binary (set one env var, a security review tool reports zero findings, silently, exit 0) and was removed before this card's candidate was reviewed. Production internal/review/reviewer.go contains no mutation hook of any kind.

Out of scope (see the card)

Fetching a PR's diff from a Git host (GitHub, etc.) or publishing a review comment — future cards (AUR-437+).
Rule citation (internal/review/rules.go) and ISO/IEC 25010 scoring (internal/review/iso25010) — dependent cards.

AUR-430: aurumcode review