diff --git a/README.md b/README.md index ca9f57c..fe46d73 100644 --- a/README.md +++ b/README.md @@ -158,7 +158,7 @@ Options (most to least common): | `--staged` | Review only staged changes (index vs HEAD; not combinable with `--base`/`--head`). | | `--base ` | Base ref to diff against (default: merge-base with the default branch). | | `--head ` | Head ref to diff (default: working tree, incl. uncommitted changes). | -| `--agents ` | Run only these agents (comma-separated ids); default: all. | +| `--agents ` | 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 ` | Repo for `--pr` (default: inferred from the current checkout). | | `--json` | Emit machine-readable JSON on stdout. | @@ -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 ` 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 @@ -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 diff --git a/llp/0002-review-engine-pipeline.explainer.md b/llp/0002-review-engine-pipeline.explainer.md index 31ad1d3..7b1a1a9 100644 --- a/llp/0002-review-engine-pipeline.explainer.md +++ b/llp/0002-review-engine-pipeline.explainer.md @@ -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 `) 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 diff --git a/llp/0004-diff-noise-and-prompts.explainer.md b/llp/0004-diff-noise-and-prompts.explainer.md index 542889c..d4e9dca 100644 --- a/llp/0004-diff-noise-and-prompts.explainer.md +++ b/llp/0004-diff-noise-and-prompts.explainer.md @@ -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 @@ -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 diff --git a/llp/0005-verification-fingerprints-rendering.explainer.md b/llp/0005-verification-fingerprints-rendering.explainer.md index 8b5a5b2..cedde01 100644 --- a/llp/0005-verification-fingerprints-rendering.explainer.md +++ b/llp/0005-verification-fingerprints-rendering.explainer.md @@ -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. diff --git a/llp/0006-config-schema-loading-routing.explainer.md b/llp/0006-config-schema-loading-routing.explainer.md index e052557..25a7f13 100644 --- a/llp/0006-config-schema-loading-routing.explainer.md +++ b/llp/0006-config-schema-loading-routing.explainer.md @@ -139,7 +139,61 @@ naive `**/*` catch-all would silently miss root-level files (`README.md`, `packa tests each `**/`-prefixed pattern in two variants (with and without the prefix stripped) to give the catch-all its conventional "zero or more directories" meaning [observed: `routing.ts:57-73`]. The "proper" fix lives in `matchesIgnore` itself, whose blast radius covers noise filtering too, so the -workaround stays local. +workaround stays local. `matchesPathGlobs` is that wrapper, exported so agent `paths` (below) match +in exactly the routing dialect; a change to it changes scope assignment and agent selection together. + +## Agent Frontmatter and `paths` + +`parseFrontmatter` is a minimal YAML-ish reader, not a YAML library, for the same reason the JSONC +reader is: agent prompts are trusted config read from the base commit. The frontmatter must open on +the first byte of the file, and lines split on LF or CRLF. Every `key: value` line lands in `data` +as a scalar, exactly as before lists existed, so `description`, `alwaysRun`, `model`, and +`temperature` parse byte-identically. A `# comment` after a value is NOT stripped; it stays part of +the value. Keys whose value is a list ALSO land in `lists`: a one-line +flow list (`paths: ["a/**", 'b/**', c/**]`, trailing comma allowed) or a block list (`paths:` +followed by `- item` lines; blank and `#` lines inside are skipped, and the next key ends it) +[observed: `load.ts` `parseFrontmatter`, `parseFlowList`]. Only `paths` reads `lists`. + +`parseAgentPaths` validates `paths` at load time, because a mistyped glob would otherwise skip +the agent on every PR with nothing reporting it. An invalid value is a **setup warning, not a load +error**: the agent keeps no `paths`, so it reviews every change, and the warning names the agent +file and the fix [observed: `load.ts` `parseAgentPaths`]. A load error would be worse than the +typo. In CI, `ecr ci` reports a load error to stderr and posts nothing (it must never fail the +check), so one agent's typo would stop every review in the repo. And nothing catches the typo +first: CI reviews a PR under its base commit's config, so the PR adding the typo loads fine, and +`ecr verify-config` does not read agent files (it is the credential guard, +[LLP 0007](0007-cli-commands-and-ci.explainer.md)). The AGENTS.md "fail fast" rule is for setup +errors that would fail every pass anyway (a bad model id, a credential); an invalid `paths` can +degrade instead, to the behavior the agent had before it declared `paths`. + +The value is invalid when it is not a list, is an empty list, has an empty entry, or has a glob +that can never match a repo-relative changed-file path: a leading `/` or `./`, a trailing `/` (the +CODEOWNERS directory form), and a leading `!` (negation is not supported). Typos the reader would +otherwise turn into a glob that matches nothing are invalid too: an entry with a `# comment`, text +after its closing quote (a missing comma between two quoted entries), a block item written `-x` +with no space, and a `{a,b}` brace set or an unmatched brace (an unquoted `{a,b}` splits at its +comma) [observed: `load.ts` `pathGlobIssue`]. The matcher itself never rejects a string, because +`matchesIgnore` escapes every regex metacharacter, so these are the only globs that can be +"invalid". Characters such as `[`, `]`, `?`, and braces without a comma stay literal and legal: +route file names such as `app/[id].tsx` contain them, and template directories such as +`{{cookiecutter.slug}}` contain braces. + +The same warnings cover the rest of a prompt file's frontmatter, for `shared.md`, `coordinator.md` +and every agent [observed: `load.ts` `frontmatterWarnings`]: a frontmatter block below an HTML +comment, a byte-order mark or blank lines (the reader ignores all of its keys; repos scaffolded +before the templates moved their `@ref` comment have this), and a trailing `# comment` that breaks +`alwaysRun`, `model` or `temperature`. These only report: the file loads exactly as before, because +parsing through the hidden block would suddenly apply model pins the repo's credential may not +cover. Warnings land in `LoadedConfig.setupWarnings`, absent when there are none so a clean setup +keeps its review-cache key. `runReview` puts them first in `setupNotes` on every result, early +returns included, so the PR comment's **Review setup** note keeps naming the fix; `ecr doctor` +fails on them. + +An agent without `paths` gets no `paths` key at all, not `paths: undefined`, so its object shape +and its review-cache key are unchanged [observed: `load.ts` agent construction]. The +`defaults.enforceAgents` injection copies the root agent with its `paths`, so an enforced agent +still runs only in scopes with a matching file. How the engine applies the filter is in +[LLP 0002](0002-review-engine-pipeline.explainer.md#agent-paths-filter). ## Budgets and Chunking Defaults diff --git a/llp/0009-adoption-templates-and-ci-workflows.guide.md b/llp/0009-adoption-templates-and-ci-workflows.guide.md index 0234c2b..f0c78a5 100644 --- a/llp/0009-adoption-templates-and-ci-workflows.guide.md +++ b/llp/0009-adoption-templates-and-ci-workflows.guide.md @@ -51,10 +51,14 @@ The three workflows share a posture built entirely around one fact: a reviewer j **Phase-1 default: suggestions off.** The scaffolded `config.jsonc` ships `policy.includeSuggestions: false` — a deliberate choice to surface only critical/warning findings and keep signal high, not because the schema can't do more [observed: `templates/config.jsonc:14-17`; restated in `templates/shared.md:66-67`]. -**Coordinator and security pin the pro tier on purpose.** The default agent model is `openai/gpt-5.5`, but `coordinator.md` and `agents/security.md` both pin `openai/gpt-5.5-pro` in their frontmatter [observed: `templates/config.jsonc:6`; `templates/coordinator.md:2-9`; `templates/agents/security.md:2-10`]. The rationale is stated in each file: the coordinator makes the final de-dupe/severity/decision call where "consolidation quality matters more here than the small serial-tail latency it adds," and security is "the highest-stakes agent," scoped alone to bound the extra latency/rate-limit cost [observed: `templates/coordinator.md:3-6`; `templates/agents/security.md:5-8`]. Downgrading either to save cost silently reverses that tradeoff, and no test catches it [inferred: no test asserts the coordinator/security model pins, so a cost-driven downgrade would go uncaught]. +**Coordinator and security pin the pro tier on purpose.** The default agent model is `openai/gpt-5.5`, but `coordinator.md` and `agents/security.md` both pin `openai/gpt-5.5-pro` in their frontmatter [observed: `templates/config.jsonc:6`; `templates/coordinator.md:1-8`; `templates/agents/security.md:1-9`]. The rationale is stated in each file: the coordinator makes the final de-dupe/severity/decision call where "consolidation quality matters more here than the small serial-tail latency it adds," and security is "the highest-stakes agent," scoped alone to bound the extra latency/rate-limit cost [observed: `templates/coordinator.md:2-5`; `templates/agents/security.md:4-7`]. Downgrading either to save cost silently reverses that tradeoff, and no test catches it [inferred: no test asserts the coordinator/security model pins, so a cost-driven downgrade would go uncaught]. **Concurrency defaults by auth mode.** The commented `chunk.concurrency` default is 6 for an API key and 3 for a subscription (OAuth) credential, "one account handles many parallel streams poorly, and several PRs may review on the same credential at once" [observed: `templates/config.jsonc:28-30`]. +**Agent `paths` ship commented out, and not on security.** `correctness.md` and `consistency.md` carry a commented `# paths: [...]` frontmatter example, and `config.jsonc` names the key, so an adopter can find it [observed: `templates/agents/correctness.md`, `templates/agents/consistency.md`, `templates/config.jsonc`]. No template sets it: the right globs are repo-specific, and a scaffolded value would narrow review on the first PR. `security.md` gets no example because it is the `alwaysRun` agent, and narrowing it by path would let a change outside the globs (a workflow file, a new package) skip the highest-stakes review. The engine rules for `paths` are in [LLP 0002](0002-review-engine-pipeline.explainer.md#agent-paths-filter). + +**Template `@ref` comments sit below the frontmatter.** `parseFrontmatter` reads frontmatter only when the file opens with `---`, so a comment above it would silently drop every key: the description the router reads, `alwaysRun`, and the model pins [observed: `src/config/load.ts` `parseFrontmatter`; `src/__tests__/init-scaffold.test.ts` "the scaffolded prompts' frontmatter takes effect"]. Each prompt template therefore opens with `---`, and its `@ref` comment is the first body line. + **Muse Spark is opt-in without weakening the default secret surface.** The root config documents `meta/muse-spark-1.3` plus a `META_API_KEY` auth entry, and each review-running workflow shows the corresponding secret line commented out. It is not diff --git a/src/__tests__/agent-paths.test.ts b/src/__tests__/agent-paths.test.ts new file mode 100644 index 0000000..2fc7855 --- /dev/null +++ b/src/__tests__/agent-paths.test.ts @@ -0,0 +1,114 @@ +import { test, expect } from "bun:test"; + +import { + agentMatchesFiles, + changedPaths, + formatSkippedAgents, + formatSkippedParts, + planAgentChunks, + renderPathsSkipsMarkdown, + selectAgentsByPaths, +} from "../core/agent-paths.js"; +import type { LoadedAgent } from "../config/schema.js"; + +const agent = (id: string, over: Partial = {}): LoadedAgent => ({ + id, + description: `${id} agent`, + alwaysRun: false, + model: "anthropic/claude-sonnet-5", + temperature: 0.1, + tools: { read: true }, + promptText: id, + ...over, +}); + +const ios = agent("ios", { paths: ["packages/ios/**", "**/*.swift"] }); +const docs = agent("docs", { paths: ["docs/**"] }); +const correctness = agent("correctness"); + +test("agentMatchesFiles: no `paths` matches everything; globs use the routing dialect", () => { + expect(agentMatchesFiles(correctness, [])).toBe(true); + expect(agentMatchesFiles(ios, ["packages/ios/src/a.ts"])).toBe(true); + // `**/` also matches at the repo root, as it does in routing.jsonc. + expect(agentMatchesFiles(ios, ["App.swift"])).toBe(true); + expect(agentMatchesFiles(ios, ["packages/android/a.kt", "README.md"])).toBe(false); +}); + +test("selectAgentsByPaths: drops agents whose paths match no reviewable file", () => { + const selection = selectAgentsByPaths([correctness, docs, ios], ["packages/ios/a.ts"], null); + expect(selection.agents.map((a) => a.id)).toEqual(["correctness", "ios"]); + expect(selection.skipped).toEqual(["docs"]); +}); + +test("selectAgentsByPaths: with no `paths` anywhere the roster is returned as is", () => { + const roster = [correctness, agent("security", { alwaysRun: true })]; + const selection = selectAgentsByPaths(roster, ["anything.ts"], null); + expect(selection.agents).toEqual(roster); + expect(selection.skipped).toEqual([]); +}); + +test("selectAgentsByPaths: an alwaysRun agent is skipped when its paths match nothing", () => { + const guard = agent("guard", { alwaysRun: true, paths: ["infra/**"] }); + const selection = selectAgentsByPaths([correctness, guard], ["src/a.ts"], null); + expect(selection.agents.map((a) => a.id)).toEqual(["correctness"]); + expect(selection.skipped).toEqual(["guard"]); +}); + +test("selectAgentsByPaths: an explicit agent list runs even when its paths match nothing", () => { + const selection = selectAgentsByPaths([correctness, docs, ios], ["src/a.ts"], [docs]); + expect(selection.agents).toEqual([docs]); + expect(selection.skipped).toEqual([]); +}); + +const chunk = (...paths: string[]): Array<{ path: string }> => paths.map((p) => ({ path: p })); +const chunks = [ + chunk("packages/ios/a.ts", ".changeset/x.md"), + chunk("packages/web/b.ts"), + chunk("packages/web/c.ts", "packages/ios/d.swift"), +]; + +test("planAgentChunks: skips chunks with no matching file and keeps whole chunks", () => { + expect(planAgentChunks(ios, chunks, false)).toEqual({ run: [0, 2], skipped: [1] }); + expect(planAgentChunks(correctness, chunks, false)).toEqual({ run: [0, 1, 2], skipped: [] }); +}); + +test("planAgentChunks: an explicit agent list reviews every chunk", () => { + expect(planAgentChunks(docs, chunks, true)).toEqual({ run: [0, 1, 2], skipped: [] }); +}); + +test("a rename matches on its old path too, at run and chunk level", () => { + // Moving a file out of an agent's globs changes what they cover, like a deletion. + const moved = { path: "lib/X.swift.bak", oldPath: "packages/ios/X.swift" }; + expect(changedPaths([moved, { path: "README.md" }])).toEqual([ + "lib/X.swift.bak", + "packages/ios/X.swift", + "README.md", + ]); + const ios = agent("ios", { paths: ["packages/ios/**"] }); + expect(selectAgentsByPaths([ios], changedPaths([moved]), null).skipped).toEqual([]); + expect(planAgentChunks(ios, [[{ path: "README.md" }], [moved]], false)).toEqual({ + run: [1], + skipped: [0], + }); +}); + +test("skip formatters name run- and chunk-level skips", () => { + const skips = { agents: ["docs"], parts: { ios: [2] }, chunks: 3 }; + expect(formatSkippedAgents(skips)).toEqual([ + "Skipping agent(s) whose `paths` match no reviewable file (no model call): docs.", + ]); + expect(formatSkippedParts(skips)).toEqual([ + "Skipping ios on part(s) 2 of 3: no file there matches its `paths`.", + ]); + const markdown = renderPathsSkipsMarkdown(skips); + expect(markdown).toContain("agents skipped by `paths`"); + expect(markdown).toContain("| docs | the whole review: no reviewable file matches its `paths` |"); + expect(markdown).toContain("| ios | part(s) 2 of 3: no file there matches its `paths` |"); +}); + +test("skip formatters are empty when nothing was skipped", () => { + const none = { agents: [], parts: {}, chunks: 1 }; + expect(formatSkippedAgents(none)).toEqual([]); + expect(formatSkippedParts(none)).toEqual([]); + expect(renderPathsSkipsMarkdown(none)).toBe(""); +}); diff --git a/src/__tests__/config-load.test.ts b/src/__tests__/config-load.test.ts index 3ff725c..a84be44 100644 --- a/src/__tests__/config-load.test.ts +++ b/src/__tests__/config-load.test.ts @@ -1,6 +1,15 @@ import { test, expect } from "bun:test"; +import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import path from "node:path"; -import { parseFrontmatter, stripJsonComments, stripTrailingCommas } from "../config/load.js"; +import { + CONFIG_DIRNAME, + loadReviewConfig, + parseFrontmatter, + stripJsonComments, + stripTrailingCommas, +} from "../config/load.js"; test("parseFrontmatter: parses scalar keys + returns body without frontmatter", () => { const { data, body } = parseFrontmatter("---\nmodel: x\nalwaysRun: true\n---\n# Body\ntext"); @@ -27,3 +36,211 @@ test("stripTrailingCommas: removes trailing commas (JSONC parse regression)", () // A comma inside a string must be preserved. expect(stripTrailingCommas('{"s":"a,"}')).toBe('{"s":"a,"}'); }); + +test("parseFrontmatter: a flow list fills `lists`; the scalar `data` value is unchanged", () => { + const { data, lists, body } = parseFrontmatter( + `---\npaths: ["packages/a/**", 'docs/*.md', .changeset/*.md]\nmodel: x\n---\nBody`, + ); + expect(lists.paths).toEqual(["packages/a/**", "docs/*.md", ".changeset/*.md"]); + expect(data.paths).toBe(`["packages/a/**", 'docs/*.md', .changeset/*.md]`); + expect(data.model).toBe("x"); + expect(lists.model).toBeUndefined(); + expect(body).toBe("Body"); +}); + +test("parseFrontmatter: a block list collects `- item` lines until the next key", () => { + const { data, lists } = parseFrontmatter( + [ + "---", + "paths:", + ' - "packages/a/**"', + " # a comment inside the list", + "", + " - packages/b/**", + "alwaysRun: true", + "---", + "Body", + ].join("\n"), + ); + expect(lists.paths).toEqual(["packages/a/**", "packages/b/**"]); + expect(data.alwaysRun).toBe("true"); + expect(lists.alwaysRun).toBeUndefined(); +}); + +test("parseFrontmatter: an empty flow list and a bare `-` item are kept for validation", () => { + expect(parseFrontmatter("---\npaths: []\n---\n").lists.paths).toEqual([]); + expect(parseFrontmatter("---\npaths:\n -\n---\n").lists.paths).toEqual([""]); + expect(parseFrontmatter(`---\npaths: ["a/**",]\n---\n`).lists.paths).toEqual(["a/**"]); +}); + +test("parseFrontmatter: CRLF line endings parse every key", () => { + const { data, lists, body } = parseFrontmatter( + '---\r\nmodel: x\r\npaths:\r\n - "a/**"\r\nalwaysRun: true\r\n---\r\nBody', + ); + expect(data.model).toBe("x"); + expect(lists.paths).toEqual(["a/**"]); + expect(data.alwaysRun).toBe("true"); + expect(body).toBe("Body"); +}); + +test("parseFrontmatter: a scalar or an unterminated flow list is not a list", () => { + expect(parseFrontmatter("---\npaths: packages/a/**\n---\n").lists.paths).toBeUndefined(); + expect(parseFrontmatter(`---\npaths: ["a/**]\n---\n`).lists.paths).toBeUndefined(); + expect(parseFrontmatter("---\npaths:\n---\n").lists.paths).toBeUndefined(); +}); + +test("parseFrontmatter: list items keep what validation must name", () => { + // Text after a closing quote keeps the quotes, so the entry cannot pass as a glob. + expect(parseFrontmatter(`---\npaths: ["a/**" "b/**", 'c/**']\n---\n`).lists.paths).toEqual([ + `"a/**" "b/**"`, + "c/**", + ]); + expect(parseFrontmatter('---\npaths:\n - "a/**" x\n---\n').lists.paths).toEqual([`"a/**" x`]); + // `-glob` is not a YAML list item; it stays in the list with its dash, not dropped. + expect(parseFrontmatter("---\npaths:\n - a/**\n -b/**\n---\n").lists.paths).toEqual([ + "a/**", + "-b/**", + ]); +}); + +async function configDirWithAgent( + agentMd: string, + coordinatorMd = "Coordinator prompt.", +): Promise { + const root = await mkdtemp(path.join(tmpdir(), "ecr-agent-paths-")); + const dir = path.join(root, CONFIG_DIRNAME); + await mkdir(path.join(dir, "agents"), { recursive: true }); + await writeFile(path.join(dir, "config.jsonc"), "{}", "utf8"); + await writeFile(path.join(dir, "coordinator.md"), coordinatorMd, "utf8"); + await writeFile(path.join(dir, "agents", "ios.md"), agentMd, "utf8"); + await writeFile(path.join(dir, "agents", "plain.md"), "---\nmodel: x/y\n---\nPLAIN", "utf8"); + return root; +} + +test("loadReviewConfig: front-matter `paths` loads onto the agent; no `paths` adds no key", async () => { + const root = await configDirWithAgent('---\npaths: ["packages/ios/**", "**/*.swift"]\n---\nIOS'); + try { + const config = await loadReviewConfig(root); + const ios = config.agents.find((agent) => agent.id === "ios")!; + const plain = config.agents.find((agent) => agent.id === "plain")!; + expect(ios.paths).toEqual(["packages/ios/**", "**/*.swift"]); + expect(ios.promptText).toBe("IOS"); + expect("paths" in plain).toBe(false); + // A clean setup carries no warnings key, so its review-cache key is unchanged. + expect("setupWarnings" in config).toBe(false); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + +// A load error would be worse than a warning: in CI it stops every review in the +// repo, and nothing checks agent files before they merge. +test("loadReviewConfig: an invalid `paths` is a warning, and the agent reviews every change", async () => { + const cases: Array<[string, string]> = [ + ["paths: packages/ios/**", "is not a list of globs"], + ["paths: []", "is empty"], + ['paths: ["packages/ios/**", ""]', 'entry 2 ("") is empty'], + ['paths: ["/packages/ios/**"]', 'write "packages/ios/**"'], + ['paths: ["./packages/ios/**"]', 'write "packages/ios/**"'], + ['paths: ["packages/ios/"]', 'write "packages/ios/**"'], + ['paths: ["!packages/ios/**"]', "negation"], + ['paths:\n - "packages/ios/**" # iOS only', 'write "packages/ios/**" and move the comment'], + ["paths:\n - packages/ios/** # iOS only", 'write "packages/ios/**" and move the comment'], + ['paths: ["packages/ios/**"] # iOS only', "move the comment to its own line"], + ['paths:\n - "packages/ios/**" extra', "text after its closing quote"], + ['paths: ["packages/ios/**" "packages/android/**"]', "separate entries with commas"], + ["paths:\n -packages/ios/**", 'write "- packages/ios/**"'], + ['paths: ["packages/{ios,android}/**"]', "brace set"], + ["paths: [packages/{ios,android}/**]", 'unmatched "{" or "}"'], + ]; + for (const [line, expected] of cases) { + const root = await configDirWithAgent(`---\n${line}\n---\nIOS`); + try { + const config = await loadReviewConfig(root); + const ios = config.agents.find((agent) => agent.id === "ios")!; + expect("paths" in ios, line).toBe(false); + expect(config.setupWarnings, line).toHaveLength(1); + const [warning] = config.setupWarnings!; + expect(warning).toContain(path.join(CONFIG_DIRNAME, "agents", "ios.md")); + expect(warning).toContain(expected); + expect(warning).toContain("reviews every change"); + expect(warning).toContain("remove `paths`"); + } finally { + await rm(root, { recursive: true, force: true }); + } + } +}); + +test("loadReviewConfig: braces without a comma stay literal and valid", async () => { + const root = await configDirWithAgent('---\npaths: ["{{cookiecutter.slug}}/**"]\n---\nIOS'); + try { + const config = await loadReviewConfig(root); + expect(config.agents.find((agent) => agent.id === "ios")!.paths).toEqual([ + "{{cookiecutter.slug}}/**", + ]); + expect(config.setupWarnings).toBeUndefined(); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + +test("loadReviewConfig: frontmatter below the first line is a warning, and loads as before", async () => { + const cases: Array<[string, string]> = [ + ["\n", "move the comment below the closing `---`"], + ["", "without a byte-order mark"], + ["\n", "delete the lines above the opening `---`"], + ]; + for (const [lead, fix] of cases) { + const root = await configDirWithAgent( + `${lead}---\ndescription: iOS\nalwaysRun: true\n---\nIOS`, + `${lead}---\nmodel: x/coordinator\n---\nCOORD`, + ); + try { + const config = await loadReviewConfig(root); + const ios = config.agents.find((agent) => agent.id === "ios")!; + // Unchanged behavior: the hidden keys still do not apply. + expect(ios.alwaysRun).toBe(false); + expect(ios.description).toBe(""); + expect(config.setupWarnings).toHaveLength(2); + for (const file of ["coordinator.md", path.join("agents", "ios.md")]) { + const warning = config.setupWarnings!.find((entry) => + entry.startsWith(path.join(CONFIG_DIRNAME, file)), + ); + expect(warning, `${JSON.stringify(lead)} ${file}`).toContain(fix); + } + } finally { + await rm(root, { recursive: true, force: true }); + } + } +}); + +test("loadReviewConfig: an HTML comment with no frontmatter after it is not a warning", async () => { + const root = await configDirWithAgent("\n# iOS\n\n---\n\nMore text."); + try { + expect((await loadReviewConfig(root)).setupWarnings).toBeUndefined(); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + +test("loadReviewConfig: a trailing comment that breaks a scalar value is a warning", async () => { + const root = await configDirWithAgent( + "---\nalwaysRun: true # run on every PR\nmodel: x/y # pinned\ntemperature: 0.2 # low\n---\nIOS", + ); + try { + const config = await loadReviewConfig(root); + const ios = config.agents.find((agent) => agent.id === "ios")!; + expect(ios.alwaysRun).toBe(false); + const warnings = config.setupWarnings ?? []; + expect(warnings).toHaveLength(3); + expect(warnings[0]).toContain("`alwaysRun: true # run on every PR`"); + expect(warnings[0]).toContain("so `alwaysRun` is off"); + expect(warnings[1]).toContain("not a valid model id"); + expect(warnings[2]).toContain("default temperature"); + for (const warning of warnings) { + expect(warning).toContain("move the comment to its own line"); + } + } finally { + await rm(root, { recursive: true, force: true }); + } +}); diff --git a/src/__tests__/diff.test.ts b/src/__tests__/diff.test.ts index 6a04974..08f35c4 100644 --- a/src/__tests__/diff.test.ts +++ b/src/__tests__/diff.test.ts @@ -54,3 +54,34 @@ diff --git a/c.ts b/c.ts +d`; expect(parseUnifiedDiff(diff).map((e) => e.path)).toEqual(["a.ts", "c.ts"]); }); + +test("a rename records the old path, with or without hunks", () => { + const diff = `diff --git a/packages/ios/X.swift b/lib/X.swift +similarity index 90% +rename from packages/ios/X.swift +rename to lib/X.swift +index 111..222 100644 +--- a/packages/ios/X.swift ++++ b/lib/X.swift +@@ -1 +1 @@ +-a ++b +diff --git a/docs/a.md b/guides/a.md +similarity index 100% +rename from docs/a.md +rename to guides/a.md +diff --git a/src/a.ts b/src/a.ts +--- a/src/a.ts ++++ b/src/a.ts +@@ -1 +1 @@ +-a ++b`; + const [edited, pure, plain] = parseUnifiedDiff(diff); + expect(edited).toMatchObject({ + path: "lib/X.swift", + oldPath: "packages/ios/X.swift", + status: "R", + }); + expect(pure).toMatchObject({ path: "guides/a.md", oldPath: "docs/a.md", status: "R" }); + expect("oldPath" in plain!).toBe(false); +}); diff --git a/src/__tests__/init-scaffold.test.ts b/src/__tests__/init-scaffold.test.ts index 30f05b5..5a1e6c5 100644 --- a/src/__tests__/init-scaffold.test.ts +++ b/src/__tests__/init-scaffold.test.ts @@ -7,7 +7,7 @@ import { fileURLToPath } from "node:url"; import { initCommand, parseTokenEnvs, substituteTokenEnv } from "../commands/init.js"; import { checkConfigRefs } from "../core/config-refs.js"; import { git } from "../core/exec.js"; -import { CONFIG_DIRNAME } from "../config/load.js"; +import { CONFIG_DIRNAME, loadReviewConfig, parseFrontmatter } from "../config/load.js"; const TEMPLATES_DIR = fileURLToPath(new URL("../../templates/", import.meta.url)); @@ -314,3 +314,20 @@ test("a freshly scaffolded repo passes ref-check", async () => { // skipped here, not reported: ecr only judges citations into the reviewed repo. expect((await checkConfigRefs({ root })).problems).toEqual([]); }); + +test("the scaffolded prompts' frontmatter takes effect", async () => { + const root = await freshRepo(); + await initCommand([]); + + // parseFrontmatter reads frontmatter only at the first byte, so anything above the + // opening `---` (such as an `@ref` comment) silently drops every key. + const config = await loadReviewConfig(root); + for (const agent of config.agents) { + expect(agent.description, `${agent.id}.md has no description`).not.toBe(""); + } + expect(config.agents.find((agent) => agent.id === "security")?.alwaysRun).toBe(true); + const coordinator = await readFile(path.join(root, CONFIG_DIRNAME, "coordinator.md"), "utf8"); + expect(parseFrontmatter(coordinator).data.model).toBeTruthy(); + // A scaffolded setup must review with no setup notes on its first PR. + expect(config.setupWarnings).toBeUndefined(); +}); diff --git a/src/__tests__/review-cache.test.ts b/src/__tests__/review-cache.test.ts index 1197496..b7b9cdf 100644 --- a/src/__tests__/review-cache.test.ts +++ b/src/__tests__/review-cache.test.ts @@ -198,3 +198,23 @@ test("reviewCacheAllowed is the one policy both CI paths share", () => { // after the legacy path dropped it, which is the drift this function prevents. expect(reviewCacheAllowed(base)).toBe(true); }); + +test("agent `paths` are part of the key, and an agent without them hashes as before", async () => { + const root = await fixtureRoot(); + try { + const original = await hash(root); + const [base] = config().agents; + const withPaths = (paths: string[]): LoadedConfig => config({ agents: [{ ...base!, paths }] }); + // The skip decision is a function of the files and the agents' paths, so the key + // must move whenever the paths do. + const ios = await hash(root, { config: withPaths(["packages/ios/**"]) }); + expect(ios).not.toBe(original); + expect(await hash(root, { config: withPaths(["packages/ios/**"]) })).toBe(ios); + expect(await hash(root, { config: withPaths(["packages/web/**"]) })).not.toBe(ios); + expect(await hash(root, { config: config({ agents: [{ ...base!, paths: undefined }] }) })).toBe( + original, + ); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); diff --git a/src/__tests__/review-internals.test.ts b/src/__tests__/review-internals.test.ts index 283ad65..cc7c77f 100644 --- a/src/__tests__/review-internals.test.ts +++ b/src/__tests__/review-internals.test.ts @@ -1,4 +1,7 @@ import { test, expect } from "bun:test"; +import { mkdtemp, readFile, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import path from "node:path"; import { chunkByLines, @@ -15,7 +18,10 @@ import { buildReviewTrace, mergeTraceNotes, attachReviewTrace, + runReview, } from "../core/review.js"; +import type { ReviewRunOptions } from "../core/review.js"; +import type { ReviewSource } from "../sources/source.js"; import { REVIEW_TRACE_BYTES_LIMIT } from "../core/schema.js"; import type { LoadedConfig } from "../config/schema.js"; import type { PatchWorkspaceFile } from "../core/noise.js"; @@ -391,3 +397,149 @@ test("effectiveConcurrency: the claude engine caps on an OAUTH credential, not a explicit.chunk = { concurrency: 5 } as never; expect(effectiveConcurrency(explicit, {})).toBe(5); }); + +// ---- agent `paths` filter wiring in runReview ---- +// The read root is prepared right before the engines start, so a source whose +// prepareReadRootAsync throws tells a test whether the run got past the paths filter +// toward a model call without starting any engine. +const ENGINE_START = "engine start reached"; + +type ChangedFile = string | { path: string; oldPath: string }; + +function pathsSource(changed: ChangedFile[]): { + source: ReviewSource; + reachedEngines: () => boolean; +} { + let reached = false; + return { + source: { + getMetadata: async () => ({ title: "t", body: "b", baseRef: "main", headRef: "feature" }), + getChangedFiles: async () => + changed.map((file) => + typeof file === "string" + ? { path: file, patch: "@@ -1 +1 @@\n-a\n+b", status: "M" } + : { ...file, patch: "@@ -1 +1 @@\n-a\n+b", status: "R" }, + ), + prepareReadRootAsync: async () => { + reached = true; + throw new Error(ENGINE_START); + }, + }, + reachedEngines: () => reached, + }; +} + +const pathsAgent = (id: string, paths?: string[]): LoadedConfig["agents"][number] => ({ + id, + description: id, + alwaysRun: false, + model: "openai/gpt-5.5", + temperature: 0.1, + tools: { read: true }, + promptText: id, + ...(paths ? { paths } : {}), +}); + +async function runPathsReview( + changed: ChangedFile[], + agents: LoadedConfig["agents"], + options: Partial = {}, + setupWarnings?: string[], +): Promise<{ result: unknown; reached: boolean; lines: string[]; log: string }> { + const runsDir = await mkdtemp(path.join(tmpdir(), "ecr-paths-runs-")); + const { source, reachedEngines } = pathsSource(changed); + const lines: string[] = []; + try { + const result = await runReview(source, { + // Only the fields read before engine startup. + config: { + configDir: runsDir, + agents, + ...(setupWarnings ? { setupWarnings } : {}), + noise: { additionalIgnores: [], additionalMarkers: [] }, + coordinator: { model: "openai/gpt-5.5", temperature: 0, promptText: "" }, + } as unknown as LoadedConfig, + mode: "ci", + runsDir, + onProgress: (line) => lines.push(line), + ...options, + }).catch((error: unknown) => error); + const log = await readFile(path.join(runsDir, "reviews.jsonl"), "utf8").catch(() => ""); + return { result, reached: reachedEngines(), lines, log }; + } finally { + await rm(runsDir, { recursive: true, force: true }); + } +} + +test("runReview: when every agent's paths miss, it returns without starting an engine", async () => { + const { result, reached, lines, log } = await runPathsReview( + ["src/a.ts"], + [pathsAgent("docs", ["docs/**"]), pathsAgent("ios", ["packages/ios/**"])], + ); + expect(reached).toBe(false); + expect(result).toMatchObject({ decision: "approve", findings: [], incomplete: [] }); + expect((result as CoordinatorOutput).summary).toContain("`paths`"); + expect(lines).toContain( + "Skipping agent(s) whose `paths` match no reviewable file (no model call): docs, ios.", + ); + expect(JSON.parse(log).pathsSkipped).toEqual({ agents: ["docs", "ios"], parts: {}, chunks: 0 }); +}); + +test("runReview: a matching agent still runs, and a run without `paths` logs no skip", async () => { + const matched = await runPathsReview( + ["docs/guide.md"], + [pathsAgent("docs", ["docs/**"]), pathsAgent("ios", ["packages/ios/**"])], + ); + expect(matched.reached).toBe(true); + expect(matched.lines).toContain( + "Skipping agent(s) whose `paths` match no reviewable file (no model call): ios.", + ); + const plain = await runPathsReview(["src/a.ts"], [pathsAgent("correctness")]); + expect(plain.reached).toBe(true); + expect(plain.lines.some((line) => line.includes("`paths`"))).toBe(false); +}); + +test("runReview: an explicit agent list runs the named agent even when its paths miss", async () => { + const { result, reached } = await runPathsReview( + ["src/a.ts"], + [pathsAgent("docs", ["docs/**"])], + { agents: ["docs"] }, + ); + expect(reached).toBe(true); + expect((result as Error).message).toContain(ENGINE_START); +}); + +test("runReview: in a scope, the paths filter sees only that scope's files", async () => { + const roster = [pathsAgent("ios", ["packages/ios/**"])]; + const changed = ["packages/ios/a.swift", "src/b.ts"]; + const other = await runPathsReview(changed, roster, { includePaths: ["src/b.ts"] }); + expect(other.reached).toBe(false); + expect(other.result).toMatchObject({ decision: "approve", incomplete: [] }); + const own = await runPathsReview(changed, roster, { includePaths: ["packages/ios/a.swift"] }); + expect(own.reached).toBe(true); +}); + +test("runReview: a rename out of an agent's paths still reaches that agent", async () => { + const roster = [pathsAgent("ios", ["packages/ios/**"])]; + const moved = await runPathsReview( + [{ path: "lib/X.swift", oldPath: "packages/ios/X.swift" }], + roster, + ); + expect(moved.reached).toBe(true); + const unrelated = await runPathsReview([{ path: "lib/Y.ts", oldPath: "src/Y.ts" }], roster); + expect(unrelated.reached).toBe(false); +}); + +test("runReview: setup warnings ride on the result as setup notes, early returns included", async () => { + const warning = ".expo-agents/code-review/agents/ios.md: `paths` is empty, ..."; + const { result, lines } = await runPathsReview( + ["src/a.ts"], + [pathsAgent("docs", ["docs/**"])], + {}, + [warning], + ); + expect(result).toMatchObject({ decision: "approve", setupNotes: [warning] }); + expect(lines).toContain(` setup: ${warning}`); + const clean = await runPathsReview(["src/a.ts"], [pathsAgent("docs", ["docs/**"])]); + expect("setupNotes" in (clean.result as object)).toBe(false); +}); diff --git a/src/__tests__/router.test.ts b/src/__tests__/router.test.ts new file mode 100644 index 0000000..01c2911 --- /dev/null +++ b/src/__tests__/router.test.ts @@ -0,0 +1,47 @@ +import { test, expect } from "bun:test"; + +import { pickRoutedAgents, routeAgents } from "../core/router.js"; +import type { LoadedAgent } from "../config/schema.js"; +import type { OpencodeHandle } from "../core/opencode.js"; + +const agent = (id: string, over: Partial = {}): LoadedAgent => ({ + id, + description: `${id} agent`, + alwaysRun: false, + model: "anthropic/claude-sonnet-5", + temperature: 0.1, + tools: { read: true }, + promptText: id, + ...over, +}); + +const correctness = agent("correctness"); +const security = agent("security", { alwaysRun: true }); +const ios = agent("ios", { paths: ["packages/ios/**"] }); + +test("pickRoutedAgents: unions alwaysRun candidates and keeps candidate order", () => { + expect(pickRoutedAgents([correctness, ios, security], ["ios"])).toEqual({ + agents: [ios, security], + routed: true, + }); +}); + +test("pickRoutedAgents: ignores ids outside the candidates (agents the paths filter left out)", () => { + // `security` is alwaysRun but its paths matched nothing, so it is not a candidate. + const result = pickRoutedAgents([correctness], ["security", "ios", "correctness"]); + expect(result).toEqual({ agents: [correctness], routed: true }); +}); + +test("pickRoutedAgents: an empty pick falls back to the candidates, not the full roster", () => { + expect(pickRoutedAgents([correctness, ios], ["unknown"])).toEqual({ + agents: [correctness, ios], + routed: false, + }); +}); + +test("routeAgents: a router failure falls back to the candidates it was given", async () => { + // Any model call on this handle throws, which exercises the fallback path. + const handle = {} as OpencodeHandle; + const result = await routeAgents(handle, [correctness, ios], []); + expect(result).toEqual({ agents: [correctness, ios], routed: false }); +}); diff --git a/src/commands/ci.ts b/src/commands/ci.ts index 9f6dc4c..93b02f8 100644 --- a/src/commands/ci.ts +++ b/src/commands/ci.ts @@ -85,7 +85,8 @@ each active scope over only its files, then renders one aggregated comment (or o per scope). With no manifest, behavior is unchanged. Options: - --agents Run only these agents (comma-separated ids); default: all + --agents Run only these agents (comma-separated ids), even when + their \`paths\` match no changed file; default: all --route Let the router pick relevant agents from the diff --scopes Limit the fan-out to these named scopes (routing only) --config-dir Load the ROOT config.jsonc + routing.jsonc from diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index b6183d0..172be49 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -261,6 +261,11 @@ export async function doctorCommand(argv: string[] = []): Promise { rootConfig.agents.every((a) => Boolean(a.promptText.trim())), "all agent prompt files resolved and non-empty", ); + // Reviews degrade past these (see LoadedConfig.setupWarnings); doctor is where + // they fail, so a broken setup is fixed before it quietly reviews less. + for (const warning of rootConfig.setupWarnings ?? []) { + line(false, warning); + } // Set when the Claude Code engine has no usable credential. checkProviderAuth // (below) returns ok:true for every anthropic shape (login fallback), so this @@ -392,6 +397,9 @@ export async function doctorCommand(argv: string[] = []): Promise { true, `scope ${scope.name}: ${scopeConfig.agents.length} agent(s) [${scopeConfig.agents.map((a) => a.id).join(", ")}], config ${scope.config}`, ); + for (const warning of scopeConfig.setupWarnings ?? []) { + line(false, `scope ${scope.name}: ${warning}`); + } } // Passes-budget headroom: active scopes run sequentially, so the worst case diff --git a/src/commands/review.ts b/src/commands/review.ts index 794d114..0278196 100644 --- a/src/commands/review.ts +++ b/src/commands/review.ts @@ -37,7 +37,8 @@ Options: to publish. --save-review with explicit --repo + --pr: save this exact preview as a postable artifact for a later \`ecr post-review\` command. - --agents run only these agents (comma-separated ids); default: all + --agents run only these agents (comma-separated ids), even when + their \`paths\` match no changed file; default: all --route let the router pick relevant agents from the diff --scope review only this routing scope (needs a routing.jsonc); runs its config over just that scope's changed files diff --git a/src/config/load.ts b/src/config/load.ts index 0942864..b79a72b 100644 --- a/src/config/load.ts +++ b/src/config/load.ts @@ -188,17 +188,26 @@ async function loadConfigDir( return Number.isFinite(n) ? n : fallback; }; + // Frontmatter problems that loading reports instead of throwing: in CI a load error + // stops every review in the repo, and nothing checks prompt files before they merge. + const setupWarnings: string[] = []; + const label = setupDirLabel(dir); + const readPrompt = async (file: string): Promise> => { + const raw = await readFile(path.join(dir, file), "utf8"); + const md = parseFrontmatter(raw); + setupWarnings.push(...frontmatterWarnings(raw, md, path.join(label, file))); + return md; + }; + // shared.md is optional; the coordinator is required. - const sharedPath = path.join(dir, "shared.md"); - const sharedPromptText = existsSync(sharedPath) - ? parseFrontmatter(await readFile(sharedPath, "utf8")).body + const sharedPromptText = existsSync(path.join(dir, "shared.md")) + ? (await readPrompt("shared.md")).body : ""; - const coordinatorPath = path.join(dir, "coordinator.md"); - if (!existsSync(coordinatorPath)) { + if (!existsSync(path.join(dir, "coordinator.md"))) { throw new Error(`Missing ${CONFIG_DIRNAME}/coordinator.md`); } - const coordinatorMd = parseFrontmatter(await readFile(coordinatorPath, "utf8")); + const coordinatorMd = await readPrompt("coordinator.md"); // Every markdown file in agents/ is a reviewer agent (id = filename). const agentsDir = path.join(dir, "agents"); @@ -212,8 +221,12 @@ async function loadConfigDir( const agents: LoadedAgent[] = []; for (const file of agentFiles) { - const md = parseFrontmatter(await readFile(path.join(agentsDir, file), "utf8")); + const md = await readPrompt(path.join("agents", file)); const id = file.replace(/\.md$/, ""); + const { paths, warning } = parseAgentPaths(md, path.join(label, "agents", file)); + if (warning) { + setupWarnings.push(warning); + } agents.push({ id, description: md.data.description ?? "", @@ -222,6 +235,9 @@ async function loadConfigDir( temperature: resolveTemp(md.data.temperature, 0.1), tools: DEFAULT_AGENT_TOOLS, promptText: md.body, + // Only when declared: an agent without `paths` keeps the exact shape (and + // review-cache key) it had before the key existed. + ...(paths ? { paths } : {}), }); } @@ -229,6 +245,8 @@ async function loadConfigDir( configDir: dir, sharedPromptText, agents, + // Only when there are any, so a clean setup keeps its review-cache key. + ...(setupWarnings.length > 0 ? { setupWarnings } : {}), coordinator: { model: resolveModel(coordinatorMd.data.model), temperature: resolveTemp(coordinatorMd.data.temperature, 0), @@ -432,28 +450,308 @@ export async function loadScopeConfig( } /** - * Parse optional YAML-ish frontmatter (simple `key: value` scalars) from the top - * of a markdown file. Returns the parsed keys and the body with frontmatter - * stripped. Supports per-agent overrides like `model:` and `temperature:`. + * Parse optional YAML-ish frontmatter from the top of a markdown file. Returns + * every `key: value` line as a scalar in `data`, the keys whose value is a list in + * `lists`, and the body with frontmatter stripped. A list is either a flow list on + * one line (`paths: ["a/**", "b/**"]`) or a block list (`paths:` followed by + * `- item` lines). Supports per-agent overrides like `model:` and `paths:`. */ -export function parseFrontmatter(md: string): { data: Record; body: string } { +export function parseFrontmatter(md: string): { + data: Record; + lists: Record; + body: string; +} { if (!md.startsWith("---")) { - return { data: {}, body: md }; + return { data: {}, lists: {}, body: md }; } const end = md.indexOf("\n---", 3); if (end === -1) { - return { data: {}, body: md }; + return { data: {}, lists: {}, body: md }; } const header = md.slice(3, end).trim(); const body = md.slice(end + 4).replace(/^\r?\n/, ""); const data: Record = {}; - for (const line of header.split("\n")) { + const lists: Record = {}; + // The key with an empty value, whose following `- item` lines form its block list. + // Blank and comment lines inside the list are skipped; the next key ends it. + let blockKey: string | null = null; + for (const line of header.split(/\r?\n/)) { + const item = line.match(/^\s*-(.*)$/); + if (blockKey && item) { + const rest = item[1]!; + // `-glob` with no space is not a YAML list item; keep the dash so validation + // names it instead of the line silently dropping out of the list. + (lists[blockKey] ??= []).push(/^\S/.test(rest) ? `-${rest.trim()}` : unquote(rest.trim())); + continue; + } const match = line.match(/^([A-Za-z0-9_-]+):\s*(.*)$/); if (match) { - data[match[1]!] = match[2]!.trim().replace(/^["']|["']$/g, ""); + const key = match[1]!; + const raw = match[2]!.trim(); + data[key] = raw.replace(/^["']|["']$/g, ""); + delete lists[key]; + const flow = parseFlowList(raw); + if (flow) { + lists[key] = flow; + } + blockKey = raw === "" ? key : null; + } + } + return { data, lists, body }; +} + +/** + * Strip the quotes around a whole quoted value. A value with text after its closing + * quote (`"a/**" x`, `"a/**" "b/**"`) keeps them, so validation can name it. + */ +function unquote(value: string): string { + const quote = value[0]; + return (quote === '"' || quote === "'") && value.indexOf(quote, 1) === value.length - 1 + ? value.slice(1, -1) + : value; +} + +/** + * The items of a one-line flow list (`[a, "b", 'c']`), or null when `raw` is not + * one. Items may be bare, single- or double-quoted; a trailing comma is allowed. + */ +function parseFlowList(raw: string): string[] | null { + if (!raw.startsWith("[") || !raw.endsWith("]")) { + return null; + } + // Split at the commas outside quotes, keeping each item's quotes for unquote(). + const items: string[] = []; + let current = ""; + let quote: string | null = null; + for (const char of raw.slice(1, -1)) { + if (quote) { + if (char === quote) { + quote = null; + } + } else if (char === '"' || char === "'") { + quote = char; + } else if (char === ",") { + items.push(current.trim()); + current = ""; + continue; + } + current += char; + } + if (quote) { + // An unterminated quote: not a list the author can have meant. + return null; + } + items.push(current.trim()); + if (items.length === 1 && items[0] === "") { + return []; + } + // Allow one trailing comma, as JSONC config does. + if (items.length > 1 && items[items.length - 1] === "") { + items.pop(); + } + return items.map(unquote); +} + +/** + * The agent's frontmatter `paths`. An absent key gives neither field. An invalid + * value gives no `paths` either, so the agent reviews every change, plus a setup + * warning that names `file` and the fix. + */ +function parseAgentPaths( + md: { data: Record; lists: Record }, + file: string, +): { paths?: string[]; warning?: string } { + if (!("paths" in md.data)) { + return {}; + } + // @ref LLP 0006#agent-frontmatter-and-paths [implements] — an invalid `paths` is a setup warning and the agent reviews every change; never a load error, never a silent no-match + const issue = agentPathsIssue(md); + if (issue) { + return { + warning: + `${file}: \`paths\` ${issue.problem}, so the agent ignores it and reviews every ` + + `change. Fix: ${issue.fix}, or remove \`paths\`.`, + }; + } + return { paths: md.lists.paths }; +} + +/** Why `paths` is not a non-empty list of globs that can match a changed file, or null. */ +function agentPathsIssue(md: { + data: Record; + lists: Record; +}): { problem: string; fix: string } | null { + const globs = md.lists.paths; + if (!globs) { + return TRAILING_COMMENT.test(md.data.paths ?? "") + ? { + problem: "has a `# comment` after it, which frontmatter does not strip", + fix: "move the comment to its own line", + } + : { + problem: "is not a list of globs", + fix: 'write it as `paths: ["packages/app/**", ".changeset/*.md"]` or as `- "glob"` lines', + }; + } + if (globs.length === 0) { + return { problem: "is empty", fix: "list at least one glob" }; + } + for (const [index, glob] of globs.entries()) { + const issue = pathGlobIssue(glob); + if (issue) { + return { + problem: `entry ${index + 1} (${JSON.stringify(glob)}) ${issue.problem}`, + fix: issue.fix, + }; } } - return { data, body }; + return null; +} + +/** A `#` that starts a YAML comment: at the start of a value or after whitespace. */ +const TRAILING_COMMENT = /(?:^|\s)#/; + +/** What a trailing `# comment` does to each scalar key whose value it breaks. */ +const SCALAR_COMMENT_EFFECTS: ReadonlyArray = [ + ["alwaysRun", "so `alwaysRun` is off"], + ["model", "so it is not a valid model id"], + ["temperature", "so the default temperature applies"], +]; + +/** + * Setup warnings for one prompt file's frontmatter. Each names `file` and the fix. + * They are reported (job log, PR comment, `ecr doctor`), never thrown: the file + * loads exactly as it did before the check existed. + */ +function frontmatterWarnings( + raw: string, + md: { data: Record }, + file: string, +): string[] { + const hidden = hiddenFrontmatter(raw); + if (hidden) { + return [ + `${file}: its frontmatter does not start on the first line (${hidden.cause}), so ` + + "every key in it is ignored, including `description`, `alwaysRun`, `model` and " + + `\`paths\`. Fix: ${hidden.fix}.`, + ]; + } + const warnings: string[] = []; + for (const [key, effect] of SCALAR_COMMENT_EFFECTS) { + const value = md.data[key]; + if (value !== undefined && TRAILING_COMMENT.test(value)) { + warnings.push( + `${file}: \`${key}: ${value}\` keeps its trailing \`# comment\` as part of the ` + + `value, ${effect}. Fix: move the comment to its own line.`, + ); + } + } + return warnings; +} + +/** + * Why a file's frontmatter sits below its first byte, where parseFrontmatter does + * not look, or null when it does not. The scaffolded templates once opened with an + * `@ref` HTML comment above the `---`, and repos scaffolded then still do. + */ +function hiddenFrontmatter(raw: string): { cause: string; fix: string } | null { + if (raw.startsWith("---")) { + return null; + } + // `\s` also matches a byte-order mark. + const lead = raw.match(/^(?:\s+|)+/)?.[0]; + if (!lead || Object.keys(parseFrontmatter(raw.slice(lead.length)).data).length === 0) { + return null; + } + if (lead.startsWith("")) { + return { + cause: "the file starts with a byte-order mark", + fix: "save it as UTF-8 without a byte-order mark", + }; + } + if (lead.includes(" --- description: Consistency with the repo's existing patterns and conventions for the same kind of change (flags, error messages and types, structure). +# Optional: review only changes that touch these repo-relative globs. Without +# `paths` the agent reviews every change; see the README for the rules. +# paths: ["src/**", "packages/*/src/**"] --- + # Consistency & conventions diff --git a/templates/agents/correctness.md b/templates/agents/correctness.md index 7b8b1c3..5a5ab6b 100644 --- a/templates/agents/correctness.md +++ b/templates/agents/correctness.md @@ -1,7 +1,10 @@ - --- description: Logic, correctness, and code-quality bugs in the changed code (off-by-one, bad error handling, type-safety gaps, unsafe assumptions). +# Optional: review only changes that touch these repo-relative globs. Without +# `paths` the agent reviews every change; see the README for the rules. +# paths: ["src/**", "packages/*/src/**"] --- + # Correctness & code quality diff --git a/templates/agents/security.md b/templates/agents/security.md index 8469251..3b8e3cd 100644 --- a/templates/agents/security.md +++ b/templates/agents/security.md @@ -1,4 +1,3 @@ - --- description: Security and secrets. Injection, credential or secret leakage, unsafe shell/child-process use, missing validation at trust boundaries. alwaysRun: true @@ -9,6 +8,7 @@ alwaysRun: true # @ref LLP 0009#config-and-prompt-templates [implements] model: anthropic/claude-opus-5-5 --- + # Security & secrets diff --git a/templates/config.jsonc b/templates/config.jsonc index e9f841f..5d3f5a0 100644 --- a/templates/config.jsonc +++ b/templates/config.jsonc @@ -11,7 +11,8 @@ // Add or remove files to change the roster — no list needed here. // shared.md (prepended to every agent + coordinator) and coordinator.md are // reserved filenames. Per-agent overrides go in each file's YAML frontmatter, - // e.g. `---\nmodel: anthropic/claude-opus-5-5\n---`. + // e.g. `---\nmodel: anthropic/claude-opus-5-5\n---`. `paths: ["packages/ios/**"]` + // there limits an agent to changes that touch those globs. // @ref LLP 0009#config-and-prompt-templates [implements] — suggestions off by default, not a schema limit "policy": { diff --git a/templates/coordinator.md b/templates/coordinator.md index f2ce50b..46e412f 100644 --- a/templates/coordinator.md +++ b/templates/coordinator.md @@ -1,4 +1,3 @@ - --- # The coordinator makes the final call — de-duping, re-judging severity, and # deciding — so it runs on the Opus tier: consolidation quality matters more here @@ -7,6 +6,7 @@ # @ref LLP 0009#config-and-prompt-templates [implements] model: anthropic/claude-opus-5-5 --- + # Coordinator — consolidation & decision