Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 52 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ Options (most to least common):
| `--staged` | Review only staged changes (index vs HEAD; not combinable with `--base`/`--head`). |
| `--base <ref>` | Base ref to diff against (default: merge-base with the default branch). |
| `--head <ref>` | Head ref to diff (default: working tree, incl. uncommitted changes). |
| `--agents <a,b>` | Run only these agents (comma-separated ids); default: all. |
| `--agents <a,b>` | Run only these agents (comma-separated ids), even when their `paths` match no changed file; default: all. |
| `--route` | Let an LLM router pick the relevant agents from the diff. |
| `--repo <owner/repo>` | Repo for `--pr` (default: inferred from the current checkout). |
| `--json` | Emit machine-readable JSON on stdout. |
Expand Down Expand Up @@ -474,14 +474,59 @@ cacheable size (~1–4K tokens) show `cache read 0` — expected, not a bug.
```markdown
---
description: One line the router uses to decide relevance.
alwaysRun: true # run even when the router would skip this agent
model: anthropic/claude-haiku-4-5 # override the default model
# Run even when the router would skip this agent.
alwaysRun: true
# Override the default model.
model: anthropic/claude-haiku-4-5
temperature: 0.1
paths: ["packages/ios/**", "**/*.swift"]
---

# Agent instructions in Markdown…
```

The frontmatter must start on the file's first line, and it is not full YAML: put
a `# comment` on its own line, because a comment after a value stays part of the
value. Each review lists problems of this kind under **Review setup** in the PR
comment (and in the terminal) with the fix, and `ecr doctor` fails on them:
frontmatter below an HTML comment, a byte-order mark or blank lines (all its keys
are ignored), and a comment that breaks `alwaysRun`, `model` or `temperature`.

`paths` limits an agent to the changes it covers. It takes a list of repo-relative
globs in the routing dialect: `*` stays within a directory, `**` crosses
directories, and a leading `**/` also matches files at the repo root. Write the
list on one line, as above, or as a block list:

```yaml
paths:
- "packages/ios/**"
- "**/*.swift"
```

- The agent runs only when at least one reviewable changed file (after noise
filtering and `noise.additionalIgnores`) matches. Otherwise the run skips it with
no model call, and the job log, the Actions step summary, and the run log say so.
A renamed file matches on both its old and new path, so moving a file out of an
agent's globs still reaches that agent.
- On a chunked review, the agent skips each chunk that has no matching file. A
chunk it reviews keeps its full diff, so related files such as `.changeset/*.md`
still reach it.
- `--agents` (and `/review <agents>` in CI) runs the named agents even when their
`paths` match nothing, because you asked for them.
- `--route` chooses only among agents whose `paths` match, and `alwaysRun` adds an
agent only when its `paths` match (or it has no `paths`).
- With a `routing.jsonc`, each scope matches `paths` against its own changed files.
- An invalid `paths` is ignored, so the agent reviews every change, and the review
names the agent file and the fix under **Review setup**; `ecr doctor` fails on
it. Invalid means a value that is not a list, an empty list or entry, a glob
that can never match a repo-relative path (`/x/**`, `./x/**`, `x/`, `!x/**`), a
`{a,b}` brace set (braces match literally), a block item written `-x` without a
space, or an entry with a trailing `# comment` or text after its closing quote
(such as a missing comma). It is not a load error because in CI that would stop
every review in the repo.
- Quote each glob. A YAML value that starts with `*` must be quoted to stay valid
YAML for other tools.

For a real-world example, see eas-cli's
[`.expo-agents/code-review/`](https://github.com/expo/eas-cli/tree/main/.expo-code-review)
— correctness/security/consistency agents, a stronger model for security, and
Expand Down Expand Up @@ -727,9 +772,10 @@ Each run appends a JSON line to `.expo-agents/code-review/.runs/reviews.jsonl` w
inputs, decision, finding count, duration, per-agent cost, and aggregate token
usage (incl. prompt-cache read/write counts) — for auditing and measuring
cost/latency/cache reuse over time. It also records the same bounded `reviewTrace`
that the PR comment embeds for machine consumers. The same totals are printed as a
one-line summary to the terminal / CI job log at the end of each run, so cache reuse
is visible even in CI (where the run log is ephemeral).
that the PR comment embeds for machine consumers. When agent `paths` skip an agent
for the run or for some chunks, the line records it under `pathsSkipped`. The same
totals are printed as a one-line summary to the terminal / CI job log at the end of
each run, so cache reuse is visible even in CI (where the run log is ephemeral).

`ecr review --save-review` additionally writes a versioned artifact under
`.expo-agents/code-review/.runs/deferred/` with owner-only permissions. It contains the
Expand Down
22 changes: 20 additions & 2 deletions llp/0002-review-engine-pipeline.explainer.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,32 @@

## Pipeline Stages

`runReview()` runs a fixed sequence: filter noise → resolve the read root (the trust boundary) → start the engines → preflight that every model id resolves → chunk the diff → run agent×chunk passes plus one cross-cutting pass through a growable bounded queue → coordinate → verify → suppress → reconcile the summary → log. The whole run's resource lifecycle — engine handles, auth material, cwd, and the read-root worktree — is released in a single `finally` (`review.ts:947-951`), so no partial-startup path leaks a server or a worktree.
`runReview()` runs a fixed sequence: filter noise → apply each agent's `paths` → resolve the read root (the trust boundary) → start the engines → preflight that every model id resolves → chunk the diff → run agent×chunk passes plus one cross-cutting pass through a growable bounded queue → coordinate → verify → suppress → reconcile the summary → log. The whole run's resource lifecycle — engine handles, auth material, cwd, and the read-root worktree — is released in a single `finally` (`review.ts:947-951`), so no partial-startup path leaks a server or a worktree.

Two ordering decisions inside that lifecycle are load-bearing and easy to break by "cleaning up":

- **`prepareAuth()` runs before the chdir into the read-root worktree** [observed] (`review.ts:217-239`). Auth does not depend on the working directory, so doing it after materializing the read root but before `process.chdir` means an auth failure only has the worktree itself to release — it can never leave `process.cwd()` pointed at a worktree the process still has to clean up.
- **OpenCode starts before Claude Code** when both engines are in use [observed] (`review.ts:249-277`). They live in two separate `try` blocks so each keeps its own precise error message, and the ordering means a Claude Code startup failure's error path can still call `opencodeHandle?.close()` on the already-started OpenCode server (`review.ts:270`).

The engine map is built from the **selected** agent subset, not the full roster [observed] (`review.ts:200-214`, commit `93aaf30`): a run whose passes never touch Claude does not preflight — and so cannot fail on — a missing Claude CLI or token. An explicit `--agents` subset is known before startup; routing (`routeAgents()` in `src/core/router.ts`) needs a live engine handle, so it runs after startup and keeps the full roster. The trust policy for the read root itself (fatal in CI, soft fallback locally) lives in [LLP 0001](0001-trust-model.principles.md); `resolveReadRoot()` is the guard.
The engine map is built from the **selected** agent subset, not the full roster [observed] (`review.ts:200-214`, commit `93aaf30`): a run whose passes never touch Claude does not preflight — and so cannot fail on — a missing Claude CLI or token. An explicit `--agents` subset and the `paths` filter are known before startup; routing (`routeAgents()` in `src/core/router.ts`) needs a live engine handle, so it runs after startup and keeps the roster the `paths` filter left (see [Agent `paths` Filter](#agent-paths-filter)). The trust policy for the read root itself (fatal in CI, soft fallback locally) lives in [LLP 0001](0001-trust-model.principles.md); `resolveReadRoot()` is the guard.

## Agent `paths` Filter

An agent's frontmatter `paths` limits it to the changes it covers (syntax and validation: [LLP 0006](0006-config-schema-loading-routing.explainer.md#agent-frontmatter-and-paths)). `selectAgentsByPaths()` in `src/core/agent-paths.ts` applies the globs to the reviewable files: after scope isolation and noise filtering, and before the read root or any engine exists [observed: `review.ts` calls it right after the noise early return]. That position gives three properties:

- A skipped agent costs no model call. The engine map is built from the agents the filter kept, so a skipped agent's engine starts only when a fixed role still needs it: the cross-file and verifier passes use the first roster agent's model, and the coordinator uses its own [observed: `buildEngineMap` in `src/core/claude-code.ts`].
- When no agent is left, `runReview` returns the way a noise-only diff does: a clean `approve` whose summary says no reviewer applies, with no coverage note and no `couldNotComplete`. Nothing failed; the configured agents do not cover these files.
- In a routed monorepo each `runReview` call already sees only its scope's files (`includePaths`), so `paths` is matched against each scope's own files with no extra code.

The filter runs **before** the router and `alwaysRun`. `routeAgents()` receives only the kept agents, so the router can neither pick a skipped agent nor fall back to one, and an `alwaysRun` agent (including one forced by `defaults.enforceAgents`) joins only when its `paths` match [observed: `pickRoutedAgents` in `src/core/router.ts`]. An explicit `--agents` list (or `/review <agents>`) bypasses the filter at both levels below, because the caller asked for those agents by name.

On a chunked diff, `planAgentChunks()` also skips each chunk with no matching file. A chunk the agent does review is reviewed whole: files outside the globs (a `.changeset/*.md`, a shared type) can still matter to the change, so the agent gets the chunk's full diff, not only its matching files. Subdividing a timed-out chunk does not re-apply the filter to the halves, for the same reason: the timeout ladder keeps its "never simply dropped" contract unchanged. The cross-cutting pass covers the concerns of the agents that run.

Both levels match each file's path and, for a rename, its old path too [observed: `changedPaths` in `src/core/agent-paths.ts`]. Moving a file out of an agent's globs changes what those globs cover, the way deleting it would, and a deletion already matches on its old path. Matching only the new path would let a move out of `packages/ios/` skip the iOS agent. Scope routing (`includePaths`) still assigns a renamed file by its new path only.

**Skips are reported, never coverage notes.** Each skip goes to the job log (`Skipping …` progress lines), the Actions step summary (an "agents skipped by `paths`" table), and the run log (`pathsSkipped`) [observed: `formatSkippedAgents`, `formatSkippedParts`, `renderPathsSkipsMarkdown`]. A skip is configured scope, like noise filtering, not unreviewed work. Keeping it out of `incomplete` is also load-bearing for the result cache: `reviewCanBeReused` refuses any review with a non-empty `incomplete` ([LLP 0005](0005-verification-fingerprints-rendering.explainer.md#review-result-cache)), so a skip reported as a coverage note would turn every re-run of an unchanged PR into a cache miss. The skip decision is a pure function of the scoped files, the noise config, and each agent's `paths`, and the cache key hashes all three, so a cache hit can never carry a stale skip.

The same cache argument fixes the cross-file pass's **configured** model to the first ROSTER agent's (what `buildOpencodeConfig` and `buildEngineMap` actually give that pass), not the first selected agent's. Comparing against the selected one reported a false model substitution, and with it a coverage note, whenever `paths`, `--agents`, or routing removed the first roster agent and the next one had a different model [observed: `taskModel` in `review.ts`].

## Concurrency and Budgets

Expand Down
10 changes: 8 additions & 2 deletions llp/0004-diff-noise-and-prompts.explainer.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,11 @@ Path is derived from the `+++ b/...` (new) line first, then from the
is `/dev/null` — and only from the `diff --git a/... b/...` header when both the
new and old paths are missing or `/dev/null` [observed] (`src/core/diff.ts:70-73`).
The header fallback exists for the rare entry that carries neither a usable `+++`
nor `---` path.
nor `---` path. A rename also records `oldPath` from its `rename from` line, which
a pure rename has even with no `---` line; only agent `paths` read it, so a move out
of an agent's globs still reaches that agent
([LLP 0002](0002-review-engine-pipeline.explainer.md#agent-paths-filter)) [observed]
(`src/core/diff.ts` `patchToEntry`).

Binary files are the load-bearing decision here. The parser does **not** drop
them — it keeps the entry with `binary: true` and infers status from
Expand Down Expand Up @@ -148,7 +152,9 @@ with `patternVariants`, which additionally tests each `**/`-prefixed pattern wit
the prefix stripped, because `**` translating to `.*` requires at least one
slash — so a naive catch-all like `**/*` would otherwise miss root-level files
like `README.md` [observed] (`src/config/routing.ts:57-66`). Routing details are
in [LLP 0006](0006-config-schema-loading-routing.explainer.md).
in [LLP 0006](0006-config-schema-loading-routing.explainer.md). Agent frontmatter
`paths` reuse that wrapper (`matchesPathGlobs`), so the same translation also
decides which reviewer agents run [observed] (`src/core/agent-paths.ts`).

## Patch Workspace

Expand Down
2 changes: 1 addition & 1 deletion llp/0005-verification-fingerprints-rendering.explainer.md
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ The full review may contain an engine-owned `reviewTrace`. It is deliberately in

The same durable comment state is the cache for automated CI reviews. `reviewInputHash` (`src/core/review-cache.ts`) hashes each scoped changed file's content from the materialized PR-head tree, a normalized patch (blob ids and hunk coordinates removed; deletion/mode information retained), and every stable input that can change the result: the engine package version, resolved prompts/config/models, PR title/body, selected agents and routing mode, and external context. It deliberately excludes the temporary trusted-config path and the base/head commit OIDs. A restack changes OIDs and may move hunk line numbers; if it leaves the actual review input byte-identical, CI reuses the complete `CoordinatorOutput` already embedded in the most recent author-verified reviewer comment instead of launching the agents again. Symlinks are hashed as link targets and never followed. A package release changes the key so upgraded review logic runs once. Legacy comments store one `inputHash`; aggregate comments store one per `ScopeReviewResult`, so an unchanged scope can hit independently while a changed sibling reruns.

Reuse is conservative. Manual `/review`/`--force` always reruns. Partial or failed reviews (`incomplete` or `couldNotComplete`) are never cacheable. Aggregate truncation removes the affected scope's hash because the embedded scope is no longer the full result. Stack-aware requalification and model-backed author-feedback seams stay fresh because their live GitHub inputs are not yet represented in the key. Cache lookup failures degrade to an ordinary fresh review. These gates make a false miss cost tokens, while preventing a false hit from presenting stale or incomplete coverage as current.
Reuse is conservative. Manual `/review`/`--force` always reruns. Partial or failed reviews (`incomplete` or `couldNotComplete`) are never cacheable. Aggregate truncation removes the affected scope's hash because the embedded scope is no longer the full result. Stack-aware requalification and model-backed author-feedback seams stay fresh because their live GitHub inputs are not yet represented in the key. Cache lookup failures degrade to an ordinary fresh review. Agent frontmatter `paths` are part of the hashed config (an agent without them carries no key, so its hash is unchanged), and an agent the `paths` filter skips is reported outside `incomplete`, so a skip can neither go stale on a hit nor make a complete result uncacheable ([LLP 0002](0002-review-engine-pipeline.explainer.md#agent-paths-filter)). These gates make a false miss cost tokens, while preventing a false hit from presenting stale or incomplete coverage as current.

The hash is state, not display: it lives only inside the base64 state marker and survives `/dismiss`/`/undismiss` re-renders. The existing author-plus-marker ownership check remains the trust boundary for adopting prior state; a PR author cannot forge a look-alike comment and supply a cache entry.

Expand Down
Loading
Loading