Compare commits

...
7 Commits
Author SHA1 Message Date
Brent Vatne 601b19afc1 Trusted base-commit configuration by default; isolate the runtime from PR-head config
ecr ci now loads all review configuration (config.jsonc, routing, prompts,
models, auth mapping) from the PR's immutable base commit, materialized via
the GitHub API — never from the PR head — failing closed with one terminal
comment when the base can't be materialized. The PR head is materialized
separately (pinned to its immutable OID, mandatory in CI) purely as source
content, and is scrubbed of ambient runtime config (opencode.json{,c},
.opencode/ plugins, AGENTS.md, CLAUDE.md, .claude/, .mcp.json, .cursor*,
.env*) before the OpenCode server starts, closing a fork-reachable code
execution path through the /review command workflow.

- git fetches append `-c credential.helper=!gh auth git-credential` so
  base-SHA checkouts with persist-credentials:false still fetch private repos
  (token never in argv or .git/config)
- run log + patch workspace anchor at the workspace (runsDir), never the
  temporary trusted root, so the artifact upload path keeps resolving
- a scope config that is new in a PR reviews with the root config and
  activates after merge (hasScopeConfig fallback)
- scaffolded workflows check out the base SHA with persist-credentials:false;
  the repo's own self-review workflow runs the engine from base source
- escape hatch: ecr ci --unsafe-config-from-head (loud warning, never
  scaffolded, removal scheduled)
2026-07-26 22:29:18 -07:00
Brent Vatne 4990b2f5a0 docs: plan trusted review configuration 2026-07-26 22:07:52 -07:00
Brent Vatne 188f8521bf Detect provider rate limits and wait them out; subscription-aware concurrency (#32)
* Detect provider rate limits and wait them out; subscription-aware concurrency

A shared subscription credential can serve several PRs' reviews at once, so
throttling is a normal condition to ride out, not a failure — and it must be
distinguishable from a wedged request.

- watch the OpenCode server's own log (isolated per run) for 429/rate-limit
  ERROR lines — explicit throttle evidence, the only kind that exists outside
  the provider; a silently parked request emits nothing anywhere
- a stall WITH recent evidence waits in 90s beats (never consuming the one
  wedged retry, bounded by the pass deadline) instead of re-sending the pass's
  whole context into a limited account; without evidence, the wedged-retry
  behavior is unchanged
- explicit 429 errors retry on a patient 15s/45s/90s schedule instead of 2s/8s
- chunk.concurrency unset now resolves by auth mode: 3 on a subscription
  (oauth) credential, 6 on API keys — one account handles six parallel streams
  poorly (the parked-request stall signature on eas-cli#4084 hit both agents'
  chunk 2 simultaneously); an explicit value always wins
- rate-limit events surface at run end and in the run log (rateLimitEvents),
  so throttle frequency is measurable across PRs

* Support ChatGPT ACCESS tokens (rotation-safe); honest verdict when nothing ran

The refresh-token-as-static-secret strategy died in production (euxy#8: every
pass "Token refresh failed: 401" — refresh tokens are SINGLE-USE; the shared
copy was spent by its first CI use and the sign-in with it).

- oauthAuthJsonEntry detects the token kind: a JWT (ACCESS token) is used
  as-is with its exp claim as the expiry — no rotation involvement, safe as a
  shared/static secret; an opaque value keeps the refresh-token shape for
  sole-consumer setups
- an expired access token fails fast naming the re-mint; near-expiry warns
  (doctor + preflight) — the credential can't silently lapse mid-run
- setup-auth hands out the ACCESS token (default env CODEX_OAUTH_ACCESS_TOKEN)
  and never surfaces the refresh token, which stays in OpenCode's own store
  as its sole legitimate consumer
- an all-passes-failed run no longer renders as "Approve with comments":
  couldNotComplete overrides the decision label with "No review — every pass
  failed" (single and per-scope comments)
- README: retract the false "refresh-token reuse verified, static CI secret
  works" claim; document the access-token strategy
- ROADMAP: token-rotator cron job added as the most important next item
- this repo's config/workflow: tokenEnv renamed to CODEX_OAUTH_ACCESS_TOKEN
2026-07-26 19:44:03 -07:00
Brent Vatne caf3fdce10 Add ecr setup-auth: guided local credential setup; make bare npx work (#31)
- setup-auth reads the repo's auth config and walks through each credential:
  a ChatGPT/Codex sign-in (runs the bundled `opencode auth login`, reuses an
  existing sign-in, extracts the refresh token) and/or API keys (prints where
  to create them and the exact restricted permissions), then prints the
  `export` lines to add to your shell config
- doctor recommends it when a credential is missing, and offers to run it
  right there on an interactive terminal
- add a `code-review-cli` bin alias so bare `npx @expo/code-review-cli <cmd>`
  resolves (npx matches the bin named after the package; with only ecr/
  expo-code-review it errored) — README's documented invocation now works
- README: first-time setup collapses to init → setup-auth → doctor
2026-07-26 18:38:07 -07:00
Brent Vatne 43b31a4879 Mixed Codex-subscription + API-key auth; GPT by default; per-pass model reporting (#30)
* Mixed Codex-subscription + API-key auth; GPT by default; report each pass's model

- auth accepts a per-provider map (auth.providers) alongside the legacy single
  object; upstream aliases synthesize OpenCode provider blocks so one upstream
  is reachable with two credentials at once (subscription for default models,
  metered key for the pro tier the subscription excludes)
- openai oauth stores the REFRESH token (expires 0; OpenCode's codex plugin
  mints access tokens; reuse across runs verified)
- tokenEnv guard (verify-config) + runtime lock use exact SET semantics via
  comma-separated ECR_EXPECTED_TOKEN_ENV
- default model is openai/gpt-5.5; anthropic subscription OAuth removed from
  docs (prohibited upstream and unsupported by OpenCode; API key still works)
- an empty REVIEWER_MODEL no longer wipes configured model ids
- every pass reports the provider/model that ACTUALLY answered (job log, step
  summary table, run log agentModels) and warns loudly on substitution —
  reviews silently ran on opencode's free gateway model for weeks behind the
  two failures above
- this repo's own review runs the mixed setup (security on openai-api/gpt-5.5-pro)

* Close the cross-provider key redirect; default the expected set to both names

Review findings on this PR (the pro-tier security pass, fittingly):
- a well-known provider key env may only feed the provider that owns it —
  the tokenEnv guard locks NAMES, so without this an auth entry could keep a
  locked name but point provider/upstream at a different provider
- the self-review workflow's ECR_EXPECTED_TOKEN_ENV fallback now matches the
  committed two-credential config instead of relying on the repo variable
2026-07-26 18:19:26 -07:00
Brent Vatne 14b32a015b Fail fast on unusable credentials, model ids, and a port/PATH collision (#29)
* Fail fast on unusable credentials, model ids, and a port/PATH collision

Debugging a local run surfaced four setup problems that all failed in the same
misleading way — silently, late, or as something unrelated:

- A malformed OAuth token makes OpenCode drop the provider from its provider
  list entirely, so every configured model reports "model not found" and nothing
  mentions credentials. Check the token's shape up front. The bar for refusing a
  run is "this cannot be a valid credential" (whitespace, absurdly short, an API
  key in oauth mode, or an OAuth token whose leading "sk-" was dropped); an
  unrecognized shape only warns, since the format is an observed convention
  rather than a guarantee and a false rejection blocks a working setup.
- A model id the server can't resolve failed every pass identically, N times,
  after spending the run's budget. Preflight the configured ids against the
  server's real provider/model list and throw once, with close matches. When the
  provider we supplied a credential for is the missing one, say the credential
  was refused rather than blaming the model id.
- The SDK defaults to a FIXED port 4096, so any already-running opencode (a
  developer's own session) killed every local run with an opaque ServeError.
  Use port 0.
- The SDK spawns a bare `opencode`, i.e. whatever is first on PATH, so a stale
  global install silently replaced the version we depend on. Prepend our own
  node_modules/.bin, pin the CLI and SDK exactly as the matched pair they are,
  and have `ecr doctor` report the version actually in use.

* Name rate limiting as a cause when a credential is refused or a pass goes silent

A throttled subscription credential (HTTP 429) is refused exactly like an invalid
one, and OpenCode surfaces neither: it drops the provider, so every model looks
nonexistent, and it retries a 429 mid-request internally, so a throttled pass
reaches us as pure silence rather than an error (opencode#17583).

Both messages previously blamed the only cause they knew about — the token and a
wedged connection — which sends people to re-issue a perfectly good credential or
hunt for a bug here. Offer rate limiting first, with the one-line curl that tells
429 (wait) from 401 (the credential really is bad).
2026-07-26 18:02:52 -07:00
Brent Vatne 3aeb82ba0d Detect stalled model requests; let the cross-file pass use the whole budget (#27)
A wedged provider request used to burn a pass's entire wall-clock cap in
silence, and the cross-file pass had no way to spend more than 25 minutes even
when the diff warranted it. Add a no-progress watchdog with one clean-slate
retry, make the finalize/corrective salvage physically tool-free, and give the
cross-file pass the remaining passes window instead of a fixed cap.

Also stop subdividing the cross-file pass by file halves: a left-half/
right-half interaction is invisible to both halves, so splitting deleted the
coverage the pass exists for while reporting success. It now falls back to a
whole-diff no-tools pass.
2026-07-26 18:00:51 -07:00
48 changed files with 4308 additions and 549 deletions

No files matched your search

+3
View File
@@ -1,6 +1,9 @@
---
description: Security and secrets. Injection, credential or secret leakage, unsafe shell/child-process use, missing validation at trust boundaries.
alwaysRun: true
# Security is the highest-stakes agent, so it runs on the pro tier — via the
# openai-api alias (metered API key), because the subscription excludes pro models.
model: openai-api/gpt-5.5-pro
---
# Security & secrets
+12 -12
View File
@@ -1,8 +1,8 @@
{
// Default model for every agent. Override per-agent via frontmatter in the
// agent's markdown, or at runtime with the REVIEWER_MODEL env var
// (e.g. REVIEWER_MODEL=openai/gpt-5.4-mini-fast).
"model": "anthropic/claude-sonnet-5",
// (e.g. REVIEWER_MODEL=openai/gpt-5.4-mini).
"model": "openai/gpt-5.5",
// Agents: every markdown file in agents/ is one reviewer (id = filename).
// Add or remove files to change the roster — no list needed here.
@@ -31,16 +31,16 @@
// HTML marker used to find + update the single PR comment. Keep it stable.
"commentTag": "expo-ai-code-reviewer",
// How model credentials are provided. Default: a Claude Pro/Max subscription.
// "oauth": tokenEnv holds a Claude Pro/Max OAuth token (from
// `claude setup-token`); it's injected as a bearer credential.
// "api-key": tokenEnv holds a provider API key (sent as x-api-key). If you
// omit `auth`, OpenCode's own login / ANTHROPIC_API_KEY is used.
// For OpenAI/GPT or another provider, omit `auth` and set REVIEWER_MODEL after an
// `opencode auth login` for that provider.
// Mixed credentials: the ChatGPT/Codex subscription serves the default models
// (zero marginal cost); the usage-based API key serves only the pro-tier models
// the subscription excludes (security runs on openai-api/gpt-5.5-pro). The oauth
// tokenEnv holds the REFRESH token from an `opencode auth login` ChatGPT sign-in;
// "openai-api" is an alias (upstream openai) synthesized in the OpenCode config.
// ECR_EXPECTED_TOKEN_ENV (repo variable) must equal this comma-separated set.
"auth": {
"mode": "oauth",
"provider": "anthropic",
"tokenEnv": "ANTHROPIC_OAUTH_API_KEY"
"providers": {
"openai": { "mode": "oauth", "tokenEnv": "CODEX_OAUTH_ACCESS_TOKEN" },
"openai-api": { "mode": "api-key", "tokenEnv": "OPENAI_API_KEY", "upstream": "openai" }
}
}
}
+1 -1
View File
@@ -1,7 +1,7 @@
---
# The coordinator only consolidates text (no repo tools), so a fast, cheap model
# fits and keeps this serial step from adding latency. Override as you like.
model: anthropic/claude-haiku-4-5-20251001
model: openai/gpt-5.4-mini
---
# Coordinator — consolidation & decision
+34 -19
View File
@@ -26,48 +26,63 @@ jobs:
# and replace the line below with, e.g.:
# if: contains(github.event.pull_request.labels.*.name, 'ai-review')
if: ${{ !contains(github.event.pull_request.labels.*.name, 'ai-review:skip') }}
# Backstop so a stalled review fails fast instead of hanging.
timeout-minutes: 60
# Backstop so a stalled review fails fast instead of hanging. The ONE cap with no
# soft landing (a hard kill posts nothing), so it stays well above the passes
# budget (55m) + coordinator (10m) + verification + setup.
timeout-minutes: 90
# A reviewer failure must never fail the PR's checks.
continue-on-error: true
steps:
# SECURITY: check out the PR's immutable BASE commit, never the PR head or
# merge ref. This repo self-reviews from source, so the base checkout is what
# keeps a PR from rewriting the ENGINE that reviews it — engine (and config)
# changes activate after merge, exactly like the trust model `ecr ci` enforces
# for config internally. persist-credentials off — the CLI's own git fetches
# authenticate through `gh` from GH_TOKEN, so the token never lands in
# .git/config.
- uses: actions/checkout@v5
with:
ref: ${{ github.event.pull_request.base.sha }}
# Shallow is enough — the reviewer gets the diff from the API (`gh`).
fetch-depth: 1
persist-credentials: false
- uses: oven-sh/setup-bun@v2
- name: Install dependencies
run: bun install --frozen-lockfile
# SECURITY: this workflow checks out the PR's code, including
# .expo-code-review/config.jsonc, whose auth.tokenEnv names the env var the CLI
# forwards as the model credential. `ecr verify-config` is the canonical guard
# (ships with the CLI): it parses every config with the real JSONC parser and
# refuses unless tokenEnv appears exactly once, in a ROOT-owned file, equal to
# the expected value (repo var ECR_EXPECTED_TOKEN_ENV) so a PR can't repoint it
# at another runner secret. This repo self-reviews from its OWN source (not the
# published package), so the guard runs from source too — same engine as `ci`.
# SECURITY: the TRUSTED BASE checkout above includes .expo-code-review/
# config.jsonc, whose auth.tokenEnv names the env var the CLI forwards as the
# model credential. `ecr verify-config` is the canonical guard (ships with the
# CLI): it parses every config with the real JSONC parser and refuses unless
# tokenEnv appears exactly once, in a ROOT-owned file, equal to the expected
# value (repo var ECR_EXPECTED_TOKEN_ENV). This repo self-reviews from BASE
# source, so the guard runs from that same base source — same engine as `ci`.
- name: Guard config tokenEnv
env:
ECR_EXPECTED_TOKEN_ENV: ${{ vars.ECR_EXPECTED_TOKEN_ENV || 'ANTHROPIC_OAUTH_API_KEY' }}
ECR_EXPECTED_TOKEN_ENV: ${{ vars.ECR_EXPECTED_TOKEN_ENV || 'CODEX_OAUTH_ACCESS_TOKEN,OPENAI_API_KEY' }}
run: bun run src/cli.ts verify-config
- name: Run AI review
# True self-review: this repo IS the reviewer, so run the CLI from the PR's
# own source instead of the published npm package. `bun run` puts
# node_modules/.bin (which has the `opencode` binary) on PATH.
# True self-review: this repo IS the reviewer, so run the CLI from source —
# the trusted BASE commit's source (see the checkout above), never the PR's.
# `bun run` puts node_modules/.bin (which has the `opencode` binary) on PATH.
# `ecr ci` materializes the PR head itself (OID-pinned, scrubbed of ambient
# runtime config) for the reviewers' source reads.
run: bun run src/cli.ts ci
continue-on-error: true
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
# Layer-1 auth lock: `ecr ci` re-checks the honored tokenEnv at runtime,
# catching what the guard step above can't. Keep in sync with the guard.
ECR_EXPECTED_TOKEN_ENV: ${{ vars.ECR_EXPECTED_TOKEN_ENV || 'ANTHROPIC_OAUTH_API_KEY' }}
# Claude Pro/Max OAuth token (from `claude setup-token`) — the env var
# named by auth.tokenEnv in config.jsonc. Store it as a repo secret.
# (For an API key instead, set auth.mode "api-key" and pass that key here.)
ANTHROPIC_OAUTH_API_KEY: ${{ secrets.ANTHROPIC_OAUTH_API_KEY }}
ECR_EXPECTED_TOKEN_ENV: ${{ vars.ECR_EXPECTED_TOKEN_ENV || 'CODEX_OAUTH_ACCESS_TOKEN,OPENAI_API_KEY' }}
# Mixed credentials (see config.jsonc auth.providers): the ChatGPT/Codex
# subscription ACCESS token serves the default models; the usage-based
# API key (project-scoped, inference-only, budget-capped) serves the
# pro-tier security pass. ECR_EXPECTED_TOKEN_ENV (repo variable) locks
# the comma-separated set of BOTH names.
CODEX_OAUTH_ACCESS_TOKEN: ${{ secrets.CODEX_OAUTH_ACCESS_TOKEN }}
OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}
# Optional: override the model for every agent (uses your OpenCode login).
REVIEWER_MODEL: ${{ vars.REVIEWER_MODEL }}
+10
View File
@@ -37,6 +37,16 @@ verifies their findings, and posts one updating PR comment.
- Scope configs never carry `auth`/`breakGlass` — those are root-only, enforced in
the schema (`ScopeReviewConfigSchema`), the loader (`loadAuthFromRoot`), and the CI
guard. With no `routing.jsonc`, behavior is byte-identical to single-config mode.
- **`opencode-ai` and `@opencode-ai/sdk` are one unit — pinned EXACTLY, bumped
together.** The SDK spawns the CLI (`launch("opencode")`), so the two are a matched
pair: a CLI older than the SDK rejects model ids the SDK accepts. Exact pins stop an
`npx` install in CI from floating one ahead of the other, and `startOpencode`
prepends our own `node_modules/.bin` to `PATH` so a machine's global install can't
shadow the pinned one. `ecr doctor` prints the version actually in use.
- **Fail fast and name the fix for setup errors.** A bad model id or credential hits
every pass identically, so it must throw once, before any pass runs, with the fix in
the message (`assertModelsResolvable`, `checkOauthTokenShape`) — never as N
indistinguishable coverage gaps discovered after spending the run's budget.
## Security invariants
+328
View File
@@ -0,0 +1,328 @@
# Plan: trusted configuration by default
## Status
Proposed security hardening for the next release of `@expo/code-review-cli`.
Reviewed 2026-07-26; amendments folded in below. The two most important changes
from review:
- **Runtime ambient-config isolation (§5) leads the rollout, not trails it.**
`runReview` chdirs into a PR-head worktree and then spawns the OpenCode server
with no `cwd`, and the pinned OpenCode binary reads project-directory config
(`opencode.json{,c}`, `.opencode/plugin`, `AGENTS.md`, `CLAUDE.md`, `.env`).
Because `issue_comment` (`/review`) is not fork-restricted, this is reachable
by **fork** PRs with full secrets in the process — a wider threat model than
the same-repo config swap the rest of this plan addresses.
- **Base-SHA checkout without mandatory head materialization is a silent
correctness regression.** `prepareReadRootAsync` fails soft to the current
checkout; once the checkout is base, that fallback makes agents and the
verifier read pre-PR file contents and silently drop real findings. Head
materialization must fail closed in CI in the same release as the template
flip.
The immediate workflow mitigation is straightforward: check out the pull
request's base SHA before invoking `ecr ci`. Version 0.5.2 already loads review
configuration before materializing the PR head for source reads, so a correctly
written workflow can keep configuration trusted while still reviewing the PR's
actual files.
That is not a sufficient product default. A security property this important
must be enforced by the CLI rather than depending on every adopter writing the
same workflow correctly.
## Problem
In a `pull_request` workflow, the default checkout is the PR merge ref. ECR then
loads the following security-sensitive inputs from that checkout:
- root and routed `config.jsonc` files;
- provider, auth mode, `tokenEnv`, model, budget, and trigger policy;
- reviewer, shared, and coordinator prompts;
- routing, enforced agents, break-glass policy, and comment tags.
A same-repository PR can therefore change the reviewer that evaluates that PR
while the process has a model credential and a comment-capable GitHub token.
`ECR_EXPECTED_TOKEN_ENV` and `ecr verify-config` constrain credential names, but
they do not make the rest of a PR-controlled configuration trustworthy.
## Security invariant
For a GitHub PR review:
> Review policy and executable reviewer configuration come from a trusted base
> commit or an explicit operator-owned directory. The PR head is untrusted data
> used only for its diff and source contents.
This includes every root and scope config, all prompt markdown, routing, model
selection, auth/provider mapping, break-glass behavior, and comment identity.
A config change in a PR becomes active only after it merges.
Failure to resolve or materialize the trusted config must fail closed. CI must
not silently fall back to the current checkout.
## Desired default behavior
1. `ecr ci` resolves the PR's immutable base and head commit OIDs through the
GitHub API.
2. It materializes the base commit into a temporary trusted-config root.
3. It loads and validates all ECR configuration and prompts from that root.
4. It fetches the diff through GitHub and materializes the PR head separately
for source reads and finding verification.
5. It starts the model runtime without loading ambient configuration from the
untrusted PR-head directory.
6. It removes both temporary trees on every success and failure path.
An explicit operator-owned `--config-dir` / `ECR_CONFIG_DIR` remains supported
and takes precedence. In CI, a relative override is resolved beneath the trusted
base root; an absolute override must be explicitly allowed and documented as an
operator trust decision.
## Implementation
### 1. Immediate workflow containment
Update all scaffolded auto-review workflows to:
```yaml
- uses: actions/checkout@<full-sha> # pinned release
with:
ref: ${{ github.event.pull_request.base.sha }}
fetch-depth: 1
persist-credentials: false
```
Keep the published ECR package pinned to an exact version. Run
`ecr verify-config` against this trusted checkout before `ecr ci`.
The command workflow already uses the base ref; align comments and tests so the
auto and command workflows state the same trust model. `dismiss.yml` also has a
bare checkout — give it `persist-credentials: false` too.
**Amendment — `persist-credentials: false` breaks the CLI's own fetches on
private repos.** `prepareReadRootAsync` runs `git fetch <https-url>` and relies
today on the `extraheader` credential `actions/checkout` persists. Ship this
only together with the CLI appending `-c credential.helper=!gh auth
git-credential` to its fetches (token read from `GH_TOKEN` env; never argv,
never `.git/config`). Existing helpers stay first, so local behavior is
unchanged.
**Amendment — this template flip must not precede mandatory head
materialization** (see §5a): with a base checkout, the soft fallback in
`prepareReadRootAsync` silently reviews/verifies pre-PR file contents.
### 2. Resolve immutable PR refs
Extend `GitHubPRSource.getMetadata()` to request immutable base/head OIDs in
addition to names. Treat missing OIDs, a repository mismatch, or an unexpected
base repository as a setup error.
Add a small ref type rather than passing branch names through security-sensitive
code:
```ts
interface PullRequestRefs {
base: { repo: string; oid: string };
head: { repo: string; oid: string };
}
```
Use argument arrays for every `git`/`gh` call and validate OIDs as full
hexadecimal commit hashes before passing them to Git.
**Amendment:** `runRoutedCi` already fetches `baseRefOid` for link context via a
second `gh pr view` — fold OID resolution into one metadata call rather than
adding another. The head worktree should also check out the resolved `headRefOid`
(detached), not `FETCH_HEAD`, so a force-push between diff fetch and
materialization can't swap the reviewed tree.
### 3. Materialize a trusted config root
Add a `prepareTrustedConfigRootAsync()` path to the GitHub PR source:
- fetch the base OID from the base repository's HTTPS URL without credentials
embedded in Git configuration;
- create a detached temporary worktree or archive;
- return its directory plus an idempotent cleanup callback;
- never fall back to `process.cwd()` in CI.
Local review modes may retain their current checkout behavior because the user
is the trust principal. `ecr ci`, and `review --pr` when posting with a token,
should default to base-ref trust.
### 4. Separate config root from source root
Change config loading APIs to receive an explicit `configRoot` instead of
implicitly reading from `cwd`. This must cover:
- `loadReviewConfig`;
- `loadRoutingManifest`;
- `loadScopeConfig`;
- agent discovery and frontmatter;
- `shared.md` and coordinator prompt loading;
- `verify-config`;
- run-log and comment-tag initialization.
Continue passing a distinct PR-head `sourceRoot` to the read/grep and finding
verification paths. Do not use `process.chdir()` as the mechanism that decides
which tree is trusted.
**Amendment — `runtimeRoot` is a first-class third root, not a test detail.**
The run log and patch workspace are derived from `config.configDir`
(`review.ts`: `path.join(config.configDir, ".runs")`), and both workflow
templates upload `.expo-code-review/.runs/reviews.jsonl` from the workspace.
Once `configDir` points into a temporary trusted root that is removed on every
exit path, the log dies with it. Anchor `.runs/` at the workspace checkout
explicitly, independent of where config was loaded from.
**Amendment — scope configs absent from base need a defined miss behavior.**
A PR that introduces a new package together with its scope config would
otherwise fail closed on exactly that PR (the scope's config dir doesn't exist
at the base commit). Fall back to the root config for such scopes, warn in the
job log, and let the scope config activate after merge.
### 5. Isolate the model runtime from PR-owned ambient configuration
**Amendment — this is the top-severity item and ships first** (see Status).
Verified during review: the OpenCode SDK spawns `opencode serve` with no `cwd`
(inheriting the PR-head worktree ECR chdir'd into), `OPENCODE_CONFIG_CONTENT`
merges with — does not replace — project config, and the pinned binary contains
project-discovery references to `opencode.json{,c}`, `.opencode/plugin`,
`AGENTS.md`, `CLAUDE.md`, and `.env*`. A PR-head `.env` alone could redirect a
provider base URL. Reachable by fork PRs via the `/review` command workflow.
**Amendment — a cheap first cut exists: scrub the worktree.** The head worktree
is a throwaway copy, so delete ambient runtime config from it (at every depth)
before the server starts: `opencode.json{,c}`, `.opencode/`, `AGENTS.md`,
`CLAUDE.md`, `.claude/`, `.mcp.json`, `.cursor*`, `.env*`. Source reads keep
working; no OpenCode feature is required. Known tradeoffs: the reviewer no
longer sees the PR's file version of those files (their diffs are still inlined
in the prompt), and a finding citing a scrubbed file will fail verification.
The full empty-runtime-dir isolation below remains the end state.
### 5a. Make head materialization mandatory in CI
`prepareReadRootAsync` fails soft (returns null → review the current checkout).
Acceptable while the checkout is the merge ref; a silent correctness and trust
regression once the checkout is base. In CI mode a failed head materialization
must fail the run closed (one terminal comment), and the fetch must use the
immutable head OID. Local `ecr review` keeps the soft fallback — the user is
the trust principal there.
The target behavior is:
- start the model runtime from an empty ECR-owned runtime directory;
- provide explicit read-only access to the PR-head source root;
- pass ECR's trusted prompts and generated OpenCode configuration explicitly;
- disable project plugin, hook, MCP, and auto-discovery paths that are not part
of the trusted ECR configuration;
- permit writes only to ECR-owned run directories.
If OpenCode cannot provide those guarantees, implement ECR-owned read/grep tools
over `sourceRoot` and do not expose the PR tree as the runtime working directory.
### 6. Make verification trust-aware
`ecr verify-config` should accept the same resolved trusted config root as
`ecr ci`. In GitHub CI mode it should report both refs:
```text
config: <base-repo>@<base-oid>
source: <head-repo>@<head-oid>
```
The existing repo-wide sweep and `ECR_EXPECTED_TOKEN_ENV` lock remain useful
defense in depth, but they apply to the trusted tree. PR-head ECR config files
may be reported as pending configuration changes; they must not affect the
current run.
### 7. CLI and compatibility surface
Prefer a safe default with a conspicuous escape hatch:
- `ecr ci`: trusted base config, always;
- `ecr review --pr`: trusted base config by default;
- local diff/staged review: current checkout config;
- `--config-dir`: explicit operator-owned override;
- optional `--unsafe-config-from-head`: temporary compatibility escape hatch,
rejected unless explicitly supplied and never scaffolded.
Do not offer an environment variable that silently changes CI back to PR-head
trust. If compatibility requires one release of opt-out behavior, print a
security warning and remove it on a scheduled major/minor boundary.
**Amendments:**
- `ECR_CONFIG_DIR` is read deep inside `resolveConfigDir` and honored
independently by `hasConfig`, `doctor`, and `verify-config`. It is not
PR-controllable (the base repo's workflow defines the env), but the "no env
var flips CI back to head trust" rule must explicitly cover it: in CI a
relative value resolves beneath the trusted base root; an absolute value is
an operator trust decision (root config + manifest only — scope subtrees
still resolve beneath the trusted base root).
- "Fail closed" means **skip the review and post the one terminal comment** —
it does not mean failing the PR's checks. The non-blocking property
(`continue-on-error`, reviewer failures never block merges) is established
behavior and stays.
- `ecr review --pr` needs no trusted-root machinery: config is loaded from the
user's own checkout before the head worktree chdir, so the config principal
is already the local user, and §5's scrub covers the runtime surface. Document
this rather than building a second materialization path.
## Tests
Add end-to-end fixtures where a same-repository PR changes each of the following:
- `auth.provider`, `auth.mode`, and `auth.tokenEnv`;
- root and scope models;
- agent, shared, and coordinator prompts;
- routing and enforced-agent policy;
- trigger, break-glass, noise, budget, and comment tag;
- an unreferenced nested `.expo-code-review/config.jsonc`;
- `opencode.json`, `AGENTS.md`, `.claude/settings.json`, plugins, hooks, and MCP
definitions in the PR head.
Assert that:
- the run uses every relevant value from the base commit;
- the PR-head source contents and line numbers are still reviewed;
- no PR-owned hook, plugin, MCP server, or command executes;
- a config-only PR is reviewed with the previous config and activates only
after merge;
- missing/unfetchable base configuration fails closed with one actionable
terminal comment;
- fork PRs, renamed branches, force-pushes, and deleted heads use immutable OIDs;
- fetch URLs and `.git/config` never contain `GH_TOKEN`;
- temporary config/source/runtime directories are removed on all failure paths;
- a failed head materialization in CI never silently reviews base contents;
- the run-log artifact path (`.expo-code-review/.runs/reviews.jsonl` in the
workspace) still resolves when config comes from the trusted root.
Unit tests should make `configRoot`, `sourceRoot`, and `runtimeRoot` visibly
different directories so accidental `cwd` coupling fails immediately.
## Rollout (reordered on review)
The original order shipped containment that doesn't close the worst hole before
the isolation that does:
1. Runtime ambient-config scrub (§5) + mandatory head materialization on
immutable OIDs (§5a). CLI-only, no workflow change; closes the fork-reachable
runtime injection for every adopter on `latest`.
2. Base-SHA workflow templates (§1) + credential-helper fetches, in the same
release as (1).
3. Trusted base config root in `ecr ci` (§2–§4): explicit
`configRoot`/`sourceRoot`/`runtimeRoot` separation, scope-miss fallback,
red tests.
4. Release as a new minor; update `ecr init --with-workflow`, README security
documentation, `doctor`, and migration notes.
5. Run the release against a PR that maliciously changes every configuration
surface above before recommending adoption.
6. Follow-up (not blocking): full empty-runtime-dir isolation from §5;
trust-aware `config:`/`source:` ref reporting in `verify-config` (§6).
## Completion criteria
This work is complete when an adopter can use the scaffolded workflow—or a
minimal custom workflow calling pinned `ecr ci`—and a same-repository PR cannot
change the model credential destination, reviewer policy, prompts, tools, or
runtime extensions used to review itself.
+177 -69
View File
@@ -26,7 +26,12 @@ flowchart TD
## Usage
Run via `npx @expo/code-review-cli <command>` (or the `ecr` / `expo-code-review`
binary once installed).
binary once installed). On a repo that already has `.expo-code-review/` set up,
getting model credentials for local runs is one command:
```bash
npx @expo/code-review-cli setup-auth
```
Reviewing a PR (`--pr`/`ci`) needs the GitHub CLI — `brew install gh && gh auth login`.
Everything else the reviewer needs (including the `opencode` runtime) ships with the
@@ -34,28 +39,30 @@ package.
### First-time setup
Scaffold, add credentials, verify.
```bash
# Scaffold .expo-code-review/ + a CI workflow (--no-workflow to skip)
# 1. Scaffold .expo-code-review/ + a CI workflow (--no-workflow to skip)
npx @expo/code-review-cli init
```
Then give it model credentials. **Recommended: a Claude Pro/Max subscription** — the
scaffolded config uses OAuth by default, so just mint a token and export it under the
env var your `config.jsonc`'s `auth.tokenEnv` names:
```bash
# Mint a Claude Pro/Max token (prints an sk-ant-oat… token)
claude setup-token
# Export it under the env var your config.jsonc's auth.tokenEnv names
export ANTHROPIC_OAUTH_API_KEY=sk-ant-oat...
# Check env, config, and credentials
# 2. Get model credentials — guided; prints the export lines for your shell config
npx @expo/code-review-cli setup-auth
# 3. Verify env, config, and credentials
npx @expo/code-review-cli doctor
```
Prefer an Anthropic **API key**, or **OpenAI/GPT** or another provider? See
[Other providers & auth modes](#other-providers) at the bottom.
`setup-auth` reads the repo's config and walks through each credential it needs:
an OpenAI **API key** (the scaffolded default — it prints where to create the key
and the exact restricted permissions to grant), and/or a **ChatGPT/Codex
subscription** sign-in (it runs OpenCode's browser login and extracts the token
for you). `doctor` offers to run it whenever a credential is missing.
In CI, store the same values as repo secrets (`OPENAI_API_KEY`; plus
`CODEX_OAUTH_ACCESS_TOKEN` for the mixed setup) — the scaffolded workflow
forwards them.
**Have a ChatGPT Plus/Pro (Codex) subscription? Use both.** The recommended
production setup pairs the subscription (runs the default models at no marginal
cost) with the usage-based key (covers only the pro-tier models the subscription
excludes) — see [the mixed setup](#other-providers) below. Prefer
**Anthropic/Claude** or another provider? Same section.
### Reviewing (already configured)
@@ -103,6 +110,7 @@ is a ready example to adapt.
| `ecr init [--no-workflow] [--force]` | Scaffold `.expo-code-review/` (config, agents, prompts) + a CI workflow. |
| `ecr init --monorepo` | …and add a `routing.jsonc` routing manifest (one default scope). |
| `ecr init --scope <dir>` | Scaffold a per-team scope under `<dir>` and register it in the manifest. |
| `ecr setup-auth [--yes]` | Walk through getting model credentials for local runs (ChatGPT sign-in and/or API keys), printing the `export` lines for your shell config. |
| `ecr review [options]` | Review local changes and print an advisory review (default command). |
| `ecr review --scope <name>` | Review only one routing scope over just that scope's changed files. |
| `ecr ci` | Review the current GitHub PR and post/update a comment. For GitHub Actions. |
@@ -142,7 +150,7 @@ your-monorepo/
// Central guardrails every scope inherits and CANNOT override.
"defaults": {
// The ONLY place auth/tokenEnv is honored (besides the root config.jsonc).
"auth": { "mode": "oauth", "provider": "anthropic", "tokenEnv": "ANTHROPIC_OAUTH_API_KEY" },
"auth": { "mode": "api-key", "provider": "openai", "tokenEnv": "OPENAI_API_KEY" },
"enforceAgents": ["security"], // always runs on every scope, roster or not
"commentTag": "expo-ai-code-reviewer" // per-scope markers derive from this
},
@@ -160,7 +168,7 @@ your-monorepo/
// server/www/.expo-code-review/config.jsonc (the www team owns this)
{
// NO "auth" block — locked centrally; a tokenEnv here is rejected by loader + CI guard.
"model": "anthropic/claude-sonnet-5",
"model": "openai/gpt-5.5",
"policy": { "includeSuggestions": false },
"noise": { "additionalIgnores": ["server/www/**/__generated__/**"] }
// shared.md, coordinator.md, agents/*.md live beside this file — the www team's roster.
@@ -185,7 +193,7 @@ your-monorepo/
the override swaps the root config/manifest against the *real* scope tree, it
does not relocate the scopes themselves.**
- **Passes budget** — `defaults`-level `budget` bounds total review time:
`totalPassesMinutes` (default 32) is split across active scopes (which run
`totalPassesMinutes` (default 55) is split across active scopes (which run
sequentially in one `ecr ci`), clamped up to `minScopeMinutes` (default 5) so a
single scope still gets a workable window. When enough scopes are active that the
floor would overshoot the total, `ecr ci` keeps the floor but warns, and `ecr
@@ -210,16 +218,31 @@ your-monorepo/
(e.g. `security`) are injected into every scope with `alwaysRun`, taken from the
root roster — a scope defining a same-id agent gets the root one, so a team can't
shadow the enforced reviewer with a weaker version on its own subtree.
- **Config comes from the checked-out ref (documented tradeoff).** The scaffolded
auto workflow checks out the PR **merge ref**, so a PR *can* edit rosters, prompts,
and routing globs for its own advisory review — that only steers what the
comment-only reviewer says about that PR. What a PR can **never** do is touch
credentials: `auth` is locked by the scope-schema rejection, the CLI's runtime
`ECR_EXPECTED_TOKEN_ENV` check, and the guard step above, all three of which run
against whatever ref is checked out. A `/review`-command workflow that checks out
only the trusted base ref (see eas-cli's) closes the prompt-tampering vector too;
resolving config from the base ref on the auto path as well is on the
[roadmap](./ROADMAP.md).
- **Configuration comes from the PR's trusted base commit.** In `ecr ci`, review
policy and reviewer configuration — `config.jsonc`, `routing.jsonc`, prompts,
models, and the auth mapping — load from the PR's immutable **base** commit,
materialized via the GitHub API. The PR head is untrusted data: it is
materialized separately (pinned to its immutable OID) purely as source content
to read and verify against. A PR editing rosters, prompts, or routing is
reviewed under the **previous** config; its changes activate after merge. If
the base commit can't be materialized, the run fails closed (one terminal
comment) — it never falls back to the checkout. A scope config that is new in
a PR is reviewed with the root config until it merges.
- **The model runtime never sees PR-owned ambient config.** The head worktree the
agents read from is scrubbed of runtime configuration before the OpenCode
server starts: `opencode.json{,c}`, `.opencode/` (plugins), `AGENTS.md`,
`CLAUDE.md`, `.claude/`, `.mcp.json`, `.cursor*`, and `.env*` at every depth.
A PR can't install a plugin, MCP server, instruction file, or `.env` into the
process that holds the model credential and the comment token. (Changes to
those files are still reviewed — their diffs are inlined in the prompt — but a
finding citing one can't be re-read during verification; that's the tradeoff.)
- **The scaffolded workflows check out only the base commit** with
`persist-credentials: false`; the CLI's own git fetches authenticate through
`gh` from `GH_TOKEN`, so the token never lands in `.git/config` or argv. The
CLI enforces the trust model itself, so a custom workflow that checks out the
PR head still gets base-commit configuration. The temporary escape hatch
`ecr ci --unsafe-config-from-head` restores the old behavior with a loud
security warning and will be removed on a minor boundary.
Ownership is enforced with CODEOWNERS: `/.expo-code-review/routing.jsonc @your-infra`
(the single authoritative router) and `/server/www/.expo-code-review/ @your-www-team`
@@ -249,7 +272,7 @@ Ownership is enforced with CODEOWNERS: `/.expo-code-review/routing.jsonc @your-i
suppressed.
Built on the [OpenCode](https://opencode.ai) SDK, which spawns the model provider
and applies Anthropic prompt caching automatically.
and applies the provider's prompt caching automatically.
</details>
@@ -269,16 +292,17 @@ cache, in three places:
a CI artifact) with the same totals plus per-pass `agentTokens`, the raw
per-agent findings, coverage notes, and what the verifier dropped.
**How the caching works.** Anthropic prompt caching is a *prefix match*: the
provider caches the rendered prompt up to a marker, and any byte change anywhere
**How the caching works.** Provider prompt caching is a *prefix match*: the
provider caches the rendered prompt up to a point, and any byte change anywhere
in that prefix invalidates everything after it. The reviewer is laid out so the
prefix is stable — the system prompt (`shared.md` + the agent's own `.md`) is
byte-identical for every chunk an agent reviews, while the volatile parts (the
diff, file lists, PR metadata) travel in the user message *after* the prefix and
never touch it. The first call per agent **writes** the cache (`cache write`,
billed ~1.25× the input price); every later chunk in the run **reads** it
(`cache read`, ~0.1× the input price). Cache entries live ~5 minutes and are
refreshed on use, which comfortably covers a run's concurrent calls.
never touch it. OpenAI caches automatically (no write premium; cached input is
billed at a steep discount and shows up as `cache read`); Anthropic charges a
small premium to **write** the cache (~1.25× input) and ~0.1× input to **read**
it. Entries live minutes, refreshed on use — comfortably covering a run's
concurrent calls.
**Reading the numbers.** Hit rate = `cache read / (cache read + input)` — the
share of prompt tokens served from cache instead of being reprocessed at full
@@ -317,7 +341,7 @@ show writes (there is nothing to re-read within the run).
---
description: One line the router uses to decide relevance.
alwaysRun: true # run even when the router would skip this agent
model: anthropic/claude-sonnet-5 # override the default model
model: openai/gpt-5.5-pro # override the default model
temperature: 0.1
---
@@ -326,24 +350,24 @@ temperature: 0.1
For a real-world example, see eas-cli's
[`.expo-code-review/`](https://github.com/expo/eas-cli/tree/main/.expo-code-review)
— correctness/security/consistency agents, Opus for security + the coordinator, and
per-repo `noise.additionalIgnores`.
— correctness/security/consistency agents, a stronger model for security + the
coordinator, and per-repo `noise.additionalIgnores`.
`config.jsonc` (JSONC — comments + trailing commas supported):
```jsonc
{
"model": "anthropic/claude-sonnet-5", // default model for the specialists
"model": "openai/gpt-5.5", // default model for the specialists
"policy": { "includeSuggestions": false }, // suppress suggestion-severity findings
"chunk": { "maxChangedLines": 1000, "maxFiles": 20, "concurrency": 6 },
"chunk": { "maxChangedLines": 1000, "maxFiles": 20 }, // concurrency defaults: 6 (API key) / 3 (subscription)
"noise": { "additionalIgnores": ["packages/*/build/**"] },
"review": { "trigger": "all", // which PRs `ecr ci` reviews: "all"
"label": "ai-review", // (default, except ai-review:skip) or
"skipLabel": "ai-review:skip" }, // "label" (only labeled PRs)
"breakGlass": { "marker": "/skip-review" }, // PR body marker that skips the review
"commentTag": "expo-ai-code-reviewer", // hidden tag used to find/update the comment
"auth": { "mode": "oauth", "provider": "anthropic",
"tokenEnv": "ANTHROPIC_OAUTH_API_KEY" }
"auth": { "mode": "api-key", "provider": "openai",
"tokenEnv": "OPENAI_API_KEY" }
}
```
@@ -357,14 +381,19 @@ Precedence: **`REVIEWER_MODEL` env** (global override) → per-file **frontmatte
setup, and a developer can override everything locally.
- **Specialist agents** (correctness/security/consistency) benefit from a
reasoning-tier model — **Sonnet** is the quality/speed sweet spot (default for
correctness/consistency). **Opus** finds more but is slower and more
rate-limited, so scope it to the highest-stakes agent: **security runs on Opus**
(set in `security.md` frontmatter), the rest on Sonnet.
reasoning-tier model — **`openai/gpt-5.5`** is the quality/speed sweet spot
(the scaffolded default). The **pro tier** finds more but is slower and more
expensive, so scope it to the highest-stakes agent: **security runs on
`openai/gpt-5.5-pro`** (set in `security.md` frontmatter), the rest on the default.
- **The coordinator** makes the final call (dedupe / re-judge / decide) — worth a
strong model; set it in `coordinator.md` frontmatter.
- If latency/timeouts dominate on big PRs, moving the specialists to a faster model
is the most direct lever (a real recall tradeoff — measure it).
(e.g. `openai/gpt-5.4-mini`) is the most direct lever (a real recall tradeoff —
measure it).
- **Every run logs which model actually answered each pass** — in the job log
(`Models used — …`), the Actions step summary table, and the run log's
`agentModels` — and warns loudly if a pass ran on a different model than
configured, so a provider-side substitution can never pass unnoticed.
There is no automatic cross-provider "equivalent" fallback — that would silently
change which model reviewed your code. Use an explicit override instead.
@@ -374,16 +403,41 @@ change which model reviewed your code. Use an explicit override instead.
<details>
<summary><b>Reliability</b> — never hangs, never silently drops work</summary>
- **Per-task time caps** — chunk passes 15 min; cross-cutting 25 min; coordinator
10 min. A global passes budget (32 min) bounds all passes incl. the subdivision
waves, fitting inside the CI job's `timeout-minutes` (60).
- **Per-task time caps** — chunk passes 15 min; coordinator 10 min. A global passes
budget (55 min) bounds all passes incl. the subdivision waves, fitting inside the
CI job's `timeout-minutes` (90).
- **The cross-file pass is elastic** — it gets whatever is left of the passes budget
rather than a fixed cap, because it's the one pass whose scope can't be traded for
convergence: halving its file set deletes exactly the coverage it exists for. Chunk
passes run alongside it under their own caps, so a long cross-file pass doesn't
starve them.
- **Tool-call cap** — a pass that makes too many `read`/`grep` calls without
finishing is *wandering*, not converging; hitting the cap trips the soft landing.
The cross-file ceiling scales with the diff's file count (its diffs are inlined, so
tool calls go to *tracing*, not fetching).
- **Stall detection** — a pass whose reply stops changing entirely (no new tool call,
no streamed text or reasoning, no token growth) has a wedged model request, not a
hard problem. After 4 min of silence it's abandoned and retried once from a clean
session, inside the same budget — instead of spending the whole cap on a dead
request. Progress lines say how long a reply has been silent, so this is legible in
the CI log.
- **Rate limits are detected and waited out, not fought.** The reviewer watches the
OpenCode server's own log for provider 429s (hard evidence, per run). A stall
*with* recent 429 evidence is throttling, not a wedge — the pass waits in 90s
beats (without consuming its one retry) instead of re-sending its whole context
into a limited account; explicit 429 errors retry on a slow 15s/45s/90s schedule.
Subscription (oauth) runs also default to `concurrency` 3 instead of 6, since one
account may be serving several PRs' reviews at once. Rate-limit events are
reported in the job log and the run log (`rateLimitEvents`), so throttling is a
visible fact about a run, never a mystery slowdown.
- **Soft landing on timeout** — at either cap, the run is interrupted and the agent
is asked to return the findings it already has, rather than discarding its work.
- **Subdivide-on-timeout** — a pass that times out with nothing to show has its
chunk split in half and the halves re-reviewed (recursively, down to a single
file), then a fast **no-tools fallback** over the inlined diff. Only a genuinely
Tools are disabled for that request, so the salvage step can't resume investigating
instead of answering.
- **Subdivide-on-timeout** — a reviewer pass that times out with nothing to show has
its chunk split in half and the halves re-reviewed (recursively, down to a single
file), then a fast **no-tools fallback** over the inlined diff (the cross-file pass
skips straight to the fallback, which still sees the whole diff). Only a genuinely
un-reducible pass reports a coverage gap — and it is always reported, never silent.
- **Parse failures are retried** (same session, then once in a bounded fresh
session) — separate from the timeout path.
@@ -448,23 +502,77 @@ visible even in CI (where the run log is ephemeral).
<details>
<summary><b>Other providers & auth modes</b></summary>
The recommended setup is a Claude Pro/Max subscription (OAuth) — see Usage above.
Alternatives, all set in `config.auth` (credentials come from OpenCode):
The recommended setup is an OpenAI API key — see Usage above. Alternatives, all
set in `config.auth` (credentials come from OpenCode):
- **Anthropic API key** — set `auth.mode` to `"api-key"` and point `tokenEnv` at the
env var holding the key (e.g. `ANTHROPIC_API_KEY`); it's sent as `x-api-key`. Omit
the `auth` block entirely to fall back to OpenCode's own login / `ANTHROPIC_API_KEY`.
- **OAuth (Pro/Max)** — `tokenEnv` holds an `sk-ant-oat…` token from
`claude setup-token` (*not* an x-api-key); it's written to an isolated OpenCode
`auth.json` as a bearer credential, using the native subscription path.
- **OpenAI / GPT, or another provider** — the current path is the `REVIEWER_MODEL`
- **ChatGPT/Codex subscription (OAuth) + usage-based API key — the recommended
mix.** OpenAI permits subscription auth in third-party tools, and OpenCode
ships the plugin for it — so the reviewer runs its default models on the
subscription (zero marginal cost) and reserves the metered key for pro-tier
models the subscription doesn't offer (`gpt-5.5-pro` is subscription-excluded).
Use the per-provider map form:
```jsonc
"auth": { "providers": {
"openai": { "mode": "oauth", "tokenEnv": "CODEX_OAUTH_ACCESS_TOKEN" },
"openai-api": { "mode": "api-key", "tokenEnv": "OPENAI_API_KEY", "upstream": "openai" }
} }
```
`openai-api` is an alias the reviewer synthesizes in the OpenCode config
(`upstream` names the SDK it's backed by): agents reference `openai-api/gpt-5.5-pro`
in frontmatter while everything else stays on `openai/gpt-5.5`. Notes:
- **The oauth `tokenEnv` holds the ACCESS token** from an `opencode auth login`
ChatGPT sign-in (`ecr setup-auth` extracts it) — a plain bearer, valid for
days, with no rotation involvement. Do **not** use the refresh token as a
shared secret: refresh tokens are single-use (rotation), so a static copy is
spent by its first use and the sign-in dies with it. Access tokens expire
(~10 days observed), so CI secrets need periodic re-minting — see the
token-rotator item in the [roadmap](./ROADMAP.md); `doctor` and the run
preflight warn before expiry.
- **The API key needs exactly two permissions** — a *Restricted* key with
*Model capabilities*: **Responses → Request** and **Chat completions →
Request**; everything else (including *List models*) stays None. Create it
in a dedicated, budget-capped project. (`ecr setup-auth` prints these
instructions too.)
- **In CI**, set the `ECR_EXPECTED_TOKEN_ENV` repo variable to the
comma-separated set of both env names
(`CODEX_OAUTH_ACCESS_TOKEN,OPENAI_API_KEY`) and pass both secrets in the
workflow.
- **Auditability**: every pass logs which provider/model answered it (job log,
step summary, run log), so the subscription/API split is visible per run.
One caveat: OpenCode can't price alias models (they're config-declared), so
pro passes report `$0` in the run log's cost column — token counts are
correct, and the OpenAI project dashboard is the source of truth for spend.
- **Anthropic / Claude (API key)** — set `auth.provider` to `"anthropic"`, point
`tokenEnv` at the env var holding a Console API key (e.g. `ANTHROPIC_API_KEY`),
and use `anthropic/...` model ids; the key is sent as `x-api-key`. Note that
Claude Pro/Max **subscription** tokens cannot be used here: Anthropic prohibits
them in third-party tools, and OpenCode has no Anthropic OAuth support — only an
API key works.
- **Another provider** — the current path is the `REVIEWER_MODEL`
env override: `opencode auth login` once (pick the provider), then run with
e.g. `REVIEWER_MODEL=openai/gpt-5.4-mini-fast`. It overrides every agent's model
and uses your OpenCode login, so no `auth` block is needed. *(First-class
per-provider config — Anthropic/OpenAI/others in `config.jsonc`, and mixing them
per agent — is on the [roadmap](./ROADMAP.md).)*
e.g. `REVIEWER_MODEL=google/gemini-3-pro`. It overrides every agent's model
and uses your OpenCode login, so no `auth` block is needed. *(Per-agent
provider mixing beyond the alias mechanism above is on the
[roadmap](./ROADMAP.md).)*
There is no shared fallback key; if a run fails for lack of credentials, authenticate
a provider in OpenCode. `ecr doctor` diagnoses setup.
**Setup errors fail fast, with the fix in the message.** A bad credential or model id
would otherwise fail every pass identically — a run that spends its whole budget
rediscovering one fixable thing, then reports N coverage gaps. So before any pass runs:
- **The credential's shape is checked.** OpenCode refuses a malformed credential by
dropping the provider entirely, which then surfaces as "model not found" for every
model, with nothing pointing at the credential. A truncated value, surrounding
whitespace, or a token that can't work for the configured `auth.mode` is rejected
by name.
- **Configured model ids are checked against the running server**, so a typo or an id
the provider doesn't have is reported once, up front, with the close matches.
- **`ecr doctor` reports the `opencode` version actually in use** and warns when a
different one is first on your `PATH` — runs use the version this package pins.
</details>
+140 -37
View File
@@ -12,11 +12,37 @@ that is meant to merge and then iterate in follow-up PRs. What's **in** phase 1
under "Recently shipped" below. The highest-value work **deferred to follow-up
PRs** (do these first, in this order):
0. **Codex access-token rotator (cron) — MOST IMPORTANT NEXT.** CI subscription
auth now uses ACCESS tokens (~10-day observed lifetime) because refresh tokens
are single-use: sharing one as a static secret spent it on first use and killed
the whole sign-in ("Your refresh token has already been used…", euxy#8,
2026-07-27). Until the rotator exists, the access-token secrets need a manual
re-mint (~weekly): `ecr setup-auth` → `gh secret set CODEX_OAUTH_ACCESS_TOKEN`
per repo. Design (constraints are load-bearing):
- a scheduled workflow (~twice weekly + workflow_dispatch) in ONE home repo is
the refresh token's SOLE consumer — nothing else may ever hold or use it;
- serialized via a concurrency group (a race forks the rotation family);
- each run: one form-urlencoded refresh call (client_id
`app_EMoamEEZ73f0CkXaXp7hrann`, endpoint `auth.openai.com/oauth/token`),
then persist the NEW refresh token to its own secret FIRST, then fan the new
access token out to `CODEX_OAUTH_ACCESS_TOKEN` on every consumer repo
(code-review-cli, eas-cli, euxy, …);
- needs a fine-grained PAT with Actions-secrets write on the consumer repos
(GITHUB_TOKEN cannot write secrets);
- failure mode: crashing between spending the old token and saving the new one
loses the family (recover: `opencode auth login` + reseed) — the rotation
grace window makes an immediate retry likely to succeed, so retry once
before alerting;
- `doctor`/preflight already warn when the access token is near expiry, which
doubles as the rotator's health check.
1. **Incremental / delta review** — review only what changed since the last review
(persistent per-file-version state). Biggest speed + cost + reliability win on
re-pushes; the last remaining lever for the monster-PR case. See §5.
2. **Cross-cutting pass parallelization / bounding** — it's the serial long-pole on
large PRs (one pass over every changed file). Chunk it by package/proximity.
2. **Cross-cutting pass parallelization** — it's the serial long-pole on large PRs
(one pass over every changed file), and it's now the pass that expands to fill the
remaining budget (§3 #10). Splitting it by *file halves* is not an option — that
hides the cross-half interactions it exists to find — but splitting by
package/proximity, where a boundary is a real architectural seam, still is.
3. ✅ **Transient-error retry with backoff** *(shipped 2026-07-23, #3.)* A non-timeout,
non-parse API error (one-off 429/5xx/network) used to drop that pass with no retry.
Now retried with bounded backoff via `isTransientApiError` (excludes
@@ -183,14 +209,16 @@ PRs** (do these first, in this order):
- **Speed knobs** — longest-processing-time-first task scheduling;
`concurrency` 4→6; CI job timeout 20→30 min (so the
worst-case internal cap chain fits with headroom).
- **Inlined chunk diffs** — the reviewer task now embeds the assigned files' diffs
- **Inlined chunk diffs** — the reviewer task embeds the assigned files' diffs
(fenced as untrusted) instead of making the agent `read` each patch file, cutting
per-pass tool round-trips. (Other files + cross-cutting still read on demand.)
- **Extended caps** — chunk 8→15m, cross-cutting 15→25m, coordinator 5→10m, CI job
30→50m (kept the invariant: worst-case serial chain < job timeout). Gives
slow-but-progressing passes room to converge instead of finalizing partial;
cost is longer max runs + more tokens. Still model-generation-bound on the
largest PRs — see §3 size guard / faster-model levers.
per-pass tool round-trips. The cross-file task now does the same, up to a line
budget (2026-07-26); only a huge diff's tail is still read on demand.
- **Extended caps** — chunk 8→15m, coordinator 5→10m, CI job 30→50→90m (kept the
invariant: worst-case serial chain < job timeout). The cross-cutting pass went
15→25m and then off a fixed cap entirely — it now expands to fill the remaining
passes window (see §3 #10). Gives slow-but-progressing passes room to converge
instead of finalizing partial; cost is longer max runs + more tokens. Still
model-generation-bound on the largest PRs — see §3 size guard / faster-model levers.
- **Generation-marker false-filter fix** — the noise filter matched generation
markers (`@generated`, `do not edit`, …) anywhere in the first 40 added lines /
4 KB, so hand-written files that merely *mention* those strings were wrongly
@@ -287,11 +315,13 @@ Guarantees (priority order; ✅ = shipped):
posts a terminal state on any failure; the coordinator has a deterministic
fallback so its failure can't discard findings; and a failed/timed-out run
never renders as a clean "Approve".
5. ✅ **Global time budget.** *(Shipped 2026-07-22.)* `PASSES_BUDGET_MS` (32m) is a
hard wall-clock ceiling for all passes incl. subdivision/fallback waves: past it,
a timed-out pass is reported as a gap rather than broken down further. Sits under
per-task caps (chunk 15m, cross-cutting 25m, coordinator 10m) and the CI job cap
(`timeout-minutes: 60`, the one hard-kill with no soft-landing).
5. ✅ **Global time budget.** *(Shipped 2026-07-22; retuned 2026-07-26.)*
`PASSES_BUDGET_MS` (55m) is a hard wall-clock ceiling for all passes incl.
subdivision/fallback waves: past it, a timed-out pass is reported as a gap rather
than broken down further. Sits under per-task caps (chunk 15m, coordinator 10m) and
the CI job cap (`timeout-minutes: 90`, the one hard-kill with no soft-landing). The
cross-file pass has no fixed cap: it expands to fill whatever of the window is left
(see #10).
6. ✅ **Size guard / degraded mode.** *(Largely addressed by #1.)* Subdivide-on-timeout
is a *reactive* size guard: an oversized/dense chunk that can't converge is split
until it does, rather than skipped. A *proactive* up-front "diff too large → review
@@ -309,6 +339,36 @@ Guarantees (priority order; ✅ = shipped):
merge ref; fork PRs are protected (GitHub withholds secrets) but same-repo PRs
run with secrets — acceptable for now (push access implies trust + label gate),
but the npx switch removes the concern entirely.
10. ✅ **Stall detection + an elastic cross-file pass.** *(Shipped 2026-07-26.)*
Motivating incident: the auto-review of eas-cli#4084 (14 files, 1.5k added lines)
ran 37 minutes and posted a review with ZERO findings and two coverage gaps. The
run log is unambiguous about why: the cross-file pass ran 7 `read` calls in its
first 6 seconds and then went completely silent for 24m52s — `agentTokens`
recorded `{input: 0, output: 0, reasoning: 0, cache: 0}` for it — until its 25m cap
fired. The finalize salvage then also went silent (and, ignoring its own
instructions, opened 7 more files), so the pass's entire work product was lost. The
coordinator stalled the same way and burned its full 10m cap on 618 input tokens.
The wall-clock cap was NOT the binding constraint: the pass did ~6 seconds of work
in 25 minutes, so a bigger budget would only have bought a longer silence. Fixes:
- **Stall watchdog** — the poll loop fingerprints the in-progress reply (parts,
streamed text/reasoning length, tool status, token counters). Unchanged for
`STALL_MS` (4m) ⇒ the request is wedged, so abandon it and retry ONCE from a
clean session, within the pass's existing deadline. Heartbeats now report how
long a reply has been silent, which is what made this diagnosable at all.
- **Tool-free salvage** — the finalize ("return what you have") and corrective
("re-emit the JSON") requests pass a per-request all-false tool map, so they
physically cannot resume investigating. Finalize budget 90s → 3m.
- **Elastic cross-file budget** — a fixed 25m is replaced by "whatever is left of
the passes window", so on a large diff it can genuinely finish.
- **No more splitting the cross-file pass** — halving its file set hides exactly
the interactions it exists to find (a left-half/right-half interaction is
invisible to both halves) while reporting success. It now goes straight to the
no-tools fallback, which still sees the whole diff.
- **Inlined cross-file diffs** — it was spending one tool round-trip per changed
file to read back patches we already had in memory, before any tracing could
start. Now inlined up to a line budget, with the tail deferred on huge diffs.
- **Scaled tool-call ceiling** — cross-file 120 → `10 × files` (120–400), or the
fixed cap would become the binding constraint under the larger time budget.
### Prompt-injection posture (and the structural fix)
@@ -325,18 +385,18 @@ Untrusted input reaches the reviewer from three places; defenses by layer:
- **Prior comments / dismissal state** — NOT fed back to the review agents (runs are
stateless), so there is no injection path there. The embedded comment state is
only ever read from the bot's own comment (GitHub write perms are the boundary).
- **The reviewer's OWN config, prompts, and code** — the sharpest vector, and the
one not fully closed. The `pull_request` auto-workflow checks out the PR *merge
ref*, so a same-repo PR can rewrite `.expo-code-review/` (agent prompts,
`config.jsonc`, `tokenEnv`) **or the CLI source itself** and it runs with secrets.
In-PR mitigations shipped: the `auth.ts` denylist refuses to forward well-known
non-provider secrets even if config names them, and the security agent is told to
treat PR-supplied config as attacker-controlled. **Structural fix (post-merge
follow-up):** have the auto-workflow build + load config from the **trusted base
ref** (the diff still comes from `gh pr diff`), exactly like the `/review` command
workflow already does — or switch to the published package via `npx` (item 9).
This can't land in *this* PR because the reviewer package isn't on `main` yet, so
the base ref has nothing to build; it becomes available the moment this merges.
- **The reviewer's OWN config, prompts, and code** — **structurally closed** (see
`PLAN-trusted-base-config.md`). `ecr ci` loads all review configuration (config,
routing, prompts, models, auth mapping) from the PR's immutable **base commit**,
materialized via the GitHub API, failing closed when it can't; the PR head is
materialized separately (OID-pinned) as source data only and is **scrubbed of
ambient runtime config** (`opencode.json{,c}`, `.opencode/` plugins, `AGENTS.md`,
`CLAUDE.md`, `.claude/`, `.mcp.json`, `.cursor*`, `.env*`) before the OpenCode
server starts, so a PR can't install a plugin/MCP/instruction file/`.env` into
the credentialed process. The scaffolded workflows check out only the base SHA
with `persist-credentials: false`, and the engine runs as the published npm
package. Earlier defense-in-depth stays: the `auth.ts` denylist, the
scope-schema auth rejection, `ECR_EXPECTED_TOKEN_ENV`, and `verify-config`.
## 4. Caching (LLM cost / quota / latency)
@@ -548,8 +608,13 @@ Still open:
Current: `config.jsonc` `model` is the default; per-agent and `coordinator.md`
frontmatter `model:` override it; `REVIEWER_MODEL` env is a global override.
Precedence: env > frontmatter > config default. In use: Sonnet 5 for the
specialists + cross-file pass, Haiku for the coordinator.
Precedence: env > frontmatter > config default. In use (since 2026-07-26):
GPT-5.5 for the specialists + cross-file pass, GPT-5.4-mini for the coordinator.
(Previously "Claude via subscription OAuth" — which, it turned out, never
actually ran: OpenCode has no Anthropic OAuth support (Anthropic prohibits
subscription tokens in third-party tools), so it silently substituted its free
gateway model in CI. That incident drove the model-preflight, `agentModels`
run-log tracking, and the per-pass "Models used" reporting.)
**Decision (2026-07-22): do NOT auto-map to a cross-provider "equivalent"** (e.g.
silently swapping `anthropic/claude-sonnet-5` for an OpenAI model when only OpenAI
@@ -570,15 +635,53 @@ Wanted instead:
review output** ("primary X unavailable; this pass ran on fallback Y") — never
silent. Would need to be tier-aware given the mixed-model setup (a single global
fallback would flatten the specialist-vs-coordinator model distinction).
- **First-class multi-provider support (Anthropic / OpenAI / others).** Today only
Anthropic has clean config-level auth (`api-key` / `oauth`); using GPT or another
provider works only through the `REVIEWER_MODEL` env override plus a manual
`opencode auth login`. Make provider + auth selection first-class in `config.jsonc`
(and per-agent frontmatter) so a repo can, from config alone, run entirely on
OpenAI, or mix — e.g. GPT for one agent and Claude for another, or a GPT
coordinator over Claude specialists. Both Anthropic and OpenAI should be equally
supported, with room for more (Google, OpenRouter, local). Pairs with the
fail-fast provider-auth check above.
- ✅ **First-class multi-provider support — mixed credentials (shipped 2026-07-26).**
`auth` now accepts a per-provider map (`auth.providers`) alongside the legacy
single object: each entry is `{mode, tokenEnv, upstream?}`, normalized internally
to a list. `upstream` synthesizes an alias provider in the OpenCode config
(`npm` per upstream, `options.apiKey: {env:tokenEnv}`, models = the ids the
roster references) so ONE upstream can be reached with TWO credentials at once —
the motivating setup: `openai` on a ChatGPT/Codex subscription (`mode: "oauth"`,
tokenEnv = the refresh token; OpenCode's codex plugin mints/refreshes access
tokens) for the default models, plus `openai-api` (upstream `openai`) holding a
metered API key for the pro tier the subscription excludes (`gpt-5.5-pro`).
Security guard updated to SET semantics: `ECR_EXPECTED_TOKEN_ENV` /
`verify-config --expected` take a comma-separated set that must equal the
declared tokenEnvs exactly (root-locked, no duplicates, one root file), and the
runtime lock in `ecr ci` compares the same set. OpenAI's policy allows
subscription auth in third-party tools (unlike Anthropic's). Verified locally
2026-07-26: subscription provider registers with the filtered model list,
`gpt-5.5` answers at $0, the `openai-api/gpt-5.5-pro` alias answers on the key,
and refresh-token REUSE across runs works — a static CI secret is viable.
*Open:* alias models are config-declared so OpenCode reports their cost as $0
(token counts correct; the OpenAI project dashboard is the spend source of
truth) — could be fixed by embedding models.dev pricing in the synthesized
block; per-agent mixing beyond the alias mechanism (Google, OpenRouter, local).
## Claude Code (`claude -p`) as an alternative review engine
OpenCode cannot use a Claude Pro/Max subscription (Anthropic prohibits
subscription OAuth tokens in third-party tools, and OpenCode ships no Anthropic
OAuth support), so today Claude is reachable only via a metered API key. But
**Claude Code itself is first-party**: `claude -p` (headless print mode, with
`--output-format json`, `--allowedTools`, and per-invocation system prompts) can
run the same specialist/coordinator passes under a developer's existing
subscription login.
- **Consider a pluggable engine layer:** `opencode` (current) vs `claude -p` per
run, selected by config or auto-detected.
- **Use `claude -p` locally by default** (i.e. when not on CI): developers already
have Claude Code installed and logged in, so `ecr review` would need zero extra
credential setup and zero per-token cost — the subscription covers it, sanctioned.
- **CI keeps the current OpenCode + API-key path** (deterministic, secret-scoped);
`claude setup-token` + the official `claude-code-action` is the sanctioned
subscription route in CI if we ever want it, but it means adopting the Agent SDK
rather than shelling out.
- Open questions: mapping the agent model (restricted tools read/grep/glob/list,
JSON-only output, per-agent system prompts) onto `claude -p` flags; parity for
token/cache/cost reporting (`agentModels`, run logs); stall/timeout handling
outside OpenCode's poll loop; and whether the coordinator/verifier stay on one
engine while specialists use another.
## Full-repository review (audit mode)
+2 -2
View File
@@ -5,8 +5,8 @@
"": {
"name": "@expo/code-review-cli",
"dependencies": {
"@opencode-ai/sdk": "^1.18.2",
"opencode-ai": "^1.18.2",
"@opencode-ai/sdk": "1.18.4",
"opencode-ai": "1.18.4",
"zod": "^4.4.3",
},
"devDependencies": {
+4 -3
View File
@@ -10,7 +10,8 @@
"type": "module",
"bin": {
"ecr": "build/cli.js",
"expo-code-review": "build/cli.js"
"expo-code-review": "build/cli.js",
"code-review-cli": "build/cli.js"
},
"files": [
"build",
@@ -35,8 +36,8 @@
"prepublishOnly": "rimraf build && tsc -p tsconfig.build.json"
},
"dependencies": {
"@opencode-ai/sdk": "^1.18.2",
"opencode-ai": "^1.18.2",
"@opencode-ai/sdk": "1.18.4",
"opencode-ai": "1.18.4",
"zod": "^4.4.3"
},
"devDependencies": {
+283 -3
View File
@@ -1,9 +1,18 @@
import { test, expect } from "bun:test";
import { prepareAuth, checkProviderAuth } from "../core/auth.js";
import {
prepareAuth,
checkProviderAuth,
checkOauthTokenShape,
oauthAuthJsonEntry,
jwtExpiryMs,
} from "../core/auth.js";
import type { LoadedConfig } from "../config/schema.js";
const cfg = (auth: unknown): LoadedConfig => ({ auth }) as unknown as LoadedConfig;
// Internal auth is a LIST of entries; most tests exercise one credential, so the
// helper wraps a single entry (pass an array for mixed setups).
const cfg = (auth: unknown): LoadedConfig =>
({ auth: Array.isArray(auth) ? auth : [auth] }) as unknown as LoadedConfig;
test("checkProviderAuth: api-key is ready when the configured tokenEnv is set", () => {
const r = checkProviderAuth(cfg({ mode: "api-key", provider: "anthropic", tokenEnv: "MY_KEY" }), {
@@ -34,7 +43,9 @@ test("checkProviderAuth: oauth requires the token env to be set", () => {
).toBe(false);
expect(
checkProviderAuth(cfg({ mode: "oauth", provider: "anthropic", tokenEnv: "OAUTH" }), {
OAUTH: "tok",
// Must be a realistically-shaped token: the value is now validated too (see
// checkOauthTokenShape), so a stub like "tok" fails as truncated.
OAUTH: `sk-ant-oat01-${"x".repeat(90)}`,
}).ok,
).toBe(true);
});
@@ -98,3 +109,272 @@ test("prepareAuth: REVIEWER_MODEL override skips provider auth entirely (no thro
}
}
});
// ---- OAuth token shape ----
//
// A malformed token is refused by OpenCode in the most misleading way available: the
// provider vanishes from its provider list, so every configured model reports "model
// not found" and nothing mentions credentials. Catch the unusable shapes up front.
const OAUTH = "sk-ant-oat01-" + "x".repeat(90);
test("a well-formed OAuth token passes", () => {
expect(checkOauthTokenShape("anthropic", OAUTH, "TOK").ok).toBe(true);
});
test("an OAuth token that lost its sk- prefix is rejected, and says so", () => {
// The exact value that cost a debugging session: `ant-oat01-…`. Provably wrong,
// because restoring the dropped "sk-" reproduces the documented shape exactly.
const r = checkOauthTokenShape("anthropic", "ant-oat01-" + "x".repeat(90), "TOK");
expect(r.ok).toBe(false);
expect(r.detail).toContain('missing its leading "sk-"');
expect(r.detail).toContain("claude setup-token");
});
test("an unrecognized shape only WARNS — a heuristic must not block a working setup", () => {
// We cannot prove Anthropic never mints other shapes, so this must still run.
const r = checkOauthTokenShape("anthropic", "sk-ant-somethingnew-" + "y".repeat(88), "TOK");
expect(r.ok).toBe(true);
expect(r.warning).toContain("does not start with");
});
test("a token unrelated to Anthropic's shapes warns rather than failing", () => {
const r = checkOauthTokenShape("anthropic", "totally-different-" + "z".repeat(90), "TOK");
expect(r.ok).toBe(true);
expect(r.warning).toBeDefined();
});
test("a well-formed OAuth token produces no warning at all", () => {
expect(checkOauthTokenShape("anthropic", OAUTH, "TOK").warning).toBeUndefined();
});
test("an API key in oauth mode is rejected with the mode to switch to", () => {
const r = checkOauthTokenShape("anthropic", "sk-ant-api03-" + "x".repeat(95), "TOK");
expect(r.ok).toBe(false);
expect(r.detail).toContain('"api-key"');
});
test("a truncated token is rejected before its prefix is even considered", () => {
const r = checkOauthTokenShape("anthropic", "sk-ant-oat01-short", "TOK");
expect(r.ok).toBe(false);
expect(r.detail).toContain("truncated");
});
test("surrounding whitespace is rejected (the classic CI secret paste)", () => {
const r = checkOauthTokenShape("anthropic", `${OAUTH}\n`, "TOK");
expect(r.ok).toBe(false);
expect(r.detail).toContain("whitespace");
});
test("a future oat-family prefix is accepted cleanly (forward compatible)", () => {
const r = checkOauthTokenShape("anthropic", "sk-ant-oat99-" + "y".repeat(90), "TOK");
expect(r.ok).toBe(true);
expect(r.warning).toBeUndefined();
});
test("formats of providers we do not know are never judged", () => {
expect(checkOauthTokenShape("someprovider", "whatever-" + "z".repeat(90), "TOK").ok).toBe(true);
});
// ---- multi-provider auth (mixed subscription + API key) ----
test("checkProviderAuth: every entry must be ready — one broken credential fails the run", () => {
const entries = [
{ mode: "oauth", provider: "openai", tokenEnv: "CODEX_TOKEN" },
{ mode: "api-key", provider: "openai-api", tokenEnv: "OPENAI_API_KEY", upstream: "openai" },
];
const bothSet = checkProviderAuth(cfg(entries), {
CODEX_TOKEN: "r".repeat(60),
OPENAI_API_KEY: "sk-proj-xxx",
});
expect(bothSet.ok).toBe(true);
// The alias's key missing must fail fast — its passes would silently fail mid-run.
const oneMissing = checkProviderAuth(cfg(entries), { CODEX_TOKEN: "r".repeat(60) });
expect(oneMissing.ok).toBe(false);
expect(oneMissing.detail).toContain("OPENAI_API_KEY");
});
test("checkProviderAuth: an upstream alias requires its tokenEnv (no ambient fallback)", () => {
// A provider id we invented has no well-known key env; relying on OpenCode's
// login can't work either, so absence is a hard failure.
const r = checkProviderAuth(
cfg({ mode: "api-key", provider: "openai-api", upstream: "openai" }),
{ OPENAI_API_KEY: "sk-proj-xxx" },
);
expect(r.ok).toBe(false);
expect(r.detail).toContain("upstream openai");
});
test("prepareAuth writes provider-shaped oauth entries into one isolated auth.json", async () => {
const prevModel = process.env.REVIEWER_MODEL;
const prevXdg = process.env.XDG_DATA_HOME;
delete process.env.REVIEWER_MODEL;
process.env.ECR_TEST_CODEX = "refresh-token-".padEnd(60, "r");
process.env.ECR_TEST_ANT = `sk-ant-oat01-${"x".repeat(95)}`;
try {
const prepared = await prepareAuth(
cfg([
{ mode: "oauth", provider: "openai", tokenEnv: "ECR_TEST_CODEX" },
{ mode: "oauth", provider: "anthropic", tokenEnv: "ECR_TEST_ANT" },
]),
);
const written = JSON.parse(
await Bun.file(`${process.env.XDG_DATA_HOME}/opencode/auth.json`).text(),
);
// openai: the durable secret is the REFRESH token; expires 0 makes the codex
// plugin mint a fresh access token on first request (access tokens live ~1h,
// shorter than a worst-case run).
expect(written.openai).toEqual({
type: "oauth",
access: "",
refresh: process.env.ECR_TEST_CODEX,
expires: 0,
});
// anthropic-style: the token IS the access credential, far-future expiry.
expect(written.anthropic.access).toBe(process.env.ECR_TEST_ANT);
expect(written.anthropic.refresh).toBe("");
expect(written.anthropic.expires).toBeGreaterThan(Date.now());
await prepared.cleanup();
} finally {
delete process.env.ECR_TEST_CODEX;
delete process.env.ECR_TEST_ANT;
if (prevModel === undefined) {
delete process.env.REVIEWER_MODEL;
} else {
process.env.REVIEWER_MODEL = prevModel;
}
if (prevXdg === undefined) {
delete process.env.XDG_DATA_HOME;
} else {
process.env.XDG_DATA_HOME = prevXdg;
}
}
});
test("prepareAuth: an upstream alias does not clobber the provider key env", async () => {
// The alias's key is read via {env:tokenEnv} in the synthesized provider block;
// copying it into OPENAI_API_KEY would hijack the real "openai" provider (which
// may be oauth-backed) — so prepareAuth must leave the env alone.
const prevModel = process.env.REVIEWER_MODEL;
const prevKey = process.env.OPENAI_API_KEY;
delete process.env.REVIEWER_MODEL;
process.env.ECR_TEST_ALIAS_KEY = "sk-proj-".padEnd(50, "k");
delete process.env.OPENAI_API_KEY;
try {
const prepared = await prepareAuth(
cfg([
{
mode: "api-key",
provider: "openai-api",
tokenEnv: "ECR_TEST_ALIAS_KEY",
upstream: "openai",
},
]),
);
expect(process.env.OPENAI_API_KEY).toBeUndefined();
await prepared.cleanup();
} finally {
delete process.env.ECR_TEST_ALIAS_KEY;
if (prevModel !== undefined) {
process.env.REVIEWER_MODEL = prevModel;
}
if (prevKey === undefined) {
delete process.env.OPENAI_API_KEY;
} else {
process.env.OPENAI_API_KEY = prevKey;
}
}
});
test("checkProviderAuth rejects a malformed oauth token end to end", () => {
const r = checkProviderAuth(cfg({ mode: "oauth", provider: "anthropic", tokenEnv: "OAUTH" }), {
OAUTH: "ant-oat01-" + "x".repeat(90),
});
expect(r.ok).toBe(false);
expect(r.detail).toContain('missing its leading "sk-"');
});
test("checkProviderAuth accepts a well-formed oauth token", () => {
const r = checkProviderAuth(cfg({ mode: "oauth", provider: "anthropic", tokenEnv: "OAUTH" }), {
OAUTH: OAUTH,
});
expect(r.ok).toBe(true);
});
test("a well-known provider key env may only feed that provider", () => {
// The tokenEnv guard locks NAMES; this closes the mapping hole — a PR keeping the
// locked name but pointing its provider/upstream elsewhere would send one
// provider's key to a different provider.
const redirected = checkProviderAuth(
cfg({
mode: "api-key",
provider: "openai-api",
tokenEnv: "OPENAI_API_KEY",
upstream: "anthropic",
}),
{ OPENAI_API_KEY: "sk-proj-xxx" },
);
expect(redirected.ok).toBe(false);
expect(redirected.detail).toContain("openai's");
// …while the legitimate mappings stay allowed: the provider itself…
expect(
checkProviderAuth(cfg({ mode: "api-key", provider: "openai", tokenEnv: "OPENAI_API_KEY" }), {
OPENAI_API_KEY: "sk-proj-xxx",
}).ok,
).toBe(true);
// …and an alias whose UPSTREAM owns the key env.
expect(
checkProviderAuth(
cfg({
mode: "api-key",
provider: "openai-api",
tokenEnv: "OPENAI_API_KEY",
upstream: "openai",
}),
{ OPENAI_API_KEY: "sk-proj-xxx" },
).ok,
).toBe(true);
});
// ---- access-token support (rotation-safe subscription credentials) ----
test("a JWT access token is used as-is with its own expiry — never refreshed", () => {
// Refresh tokens are SINGLE-USE: sharing one as a static secret killed the
// whole sign-in (euxy#8, "Your refresh token has already been used"). An
// access token is a plain bearer with no rotation involvement.
const exp = Math.floor(Date.now() / 1000) + 5 * 24 * 3600;
const jwt = `eyJhbGciOiJSUzI1NiJ9.${Buffer.from(JSON.stringify({ exp })).toString("base64url")}.sig`;
const entry = oauthAuthJsonEntry("openai", jwt);
expect(entry).toEqual({ type: "oauth", access: jwt, refresh: "", expires: exp * 1000 });
// An opaque token is still treated as a refresh token (sole-consumer setups).
const opaque = oauthAuthJsonEntry("openai", "r".repeat(196));
expect(opaque).toEqual({ type: "oauth", access: "", refresh: "r".repeat(196), expires: 0 });
});
test("an EXPIRED access token fails fast with the re-mint instruction", () => {
const exp = Math.floor(Date.now() / 1000) - 2 * 24 * 3600;
const jwt = `eyJhbGciOiJSUzI1NiJ9.${Buffer.from(JSON.stringify({ exp })).toString("base64url")}.sig`;
const r = checkProviderAuth(cfg({ mode: "oauth", provider: "openai", tokenEnv: "TOK" }), {
TOK: jwt,
});
expect(r.ok).toBe(false);
expect(r.detail).toContain("EXPIRED");
expect(r.detail).toContain("setup-auth");
});
test("a nearly-expired access token warns but never blocks", () => {
const exp = Math.floor(Date.now() / 1000) + 12 * 3600; // 12h left
const jwt = `eyJhbGciOiJSUzI1NiJ9.${Buffer.from(JSON.stringify({ exp })).toString("base64url")}.sig`;
const r = checkProviderAuth(cfg({ mode: "oauth", provider: "openai", tokenEnv: "TOK" }), {
TOK: jwt,
});
expect(r.ok).toBe(true);
expect(r.warning).toContain("expires in");
});
test("jwtExpiryMs decodes exp and tolerates garbage", () => {
expect(jwtExpiryMs("not-a-jwt")).toBeNull();
expect(jwtExpiryMs("eyJ.%%%.sig")).toBeNull();
const jwt = `eyJ.${Buffer.from(JSON.stringify({ exp: 1000 })).toString("base64url")}.s`;
expect(jwtExpiryMs(jwt)).toBe(1000_000);
});
+282 -1
View File
@@ -1,6 +1,40 @@
import { test, expect } from "bun:test";
import { isTransientApiError, AgentTimeoutError } from "../core/opencode.js";
import {
isTransientApiError,
AgentTimeoutError,
progressFingerprint,
stallWindowMs,
stallAction,
findUnknownModels,
formatUnknownModels,
buildOpencodeConfig,
} from "../core/opencode.js";
import type { LoadedConfig } from "../config/schema.js";
/** Minimal LoadedConfig for buildOpencodeConfig tests. */
function configWith(overrides: {
agents?: Array<{ id: string; model: string }>;
coordinatorModel?: string;
auth?: unknown[];
}): LoadedConfig {
return {
agents: (overrides.agents ?? [{ id: "correctness", model: "openai/gpt-5.5" }]).map((agent) => ({
...agent,
description: `${agent.id} reviewer`,
alwaysRun: false,
temperature: 0.1,
tools: {},
promptText: "",
})),
coordinator: {
model: overrides.coordinatorModel ?? "openai/gpt-5.5",
temperature: 0,
promptText: "",
},
auth: overrides.auth ?? [{ mode: "api-key", provider: "openai" }],
} as unknown as LoadedConfig;
}
test("classifies rate-limit / 5xx / network errors as transient", () => {
for (const message of [
@@ -41,3 +75,250 @@ test("tolerates non-Error thrown values", () => {
expect(isTransientApiError(undefined)).toBe(false);
expect(isTransientApiError({ nope: true })).toBe(false);
});
// ---- stall detection (see STALL_MS in opencode.ts) ----
//
// Motivating incident: eas-cli#4084, where the cross-file pass ran 7 reads in its
// first 6 seconds and then sat silent for 25 minutes with ZERO tokens recorded. The
// fingerprint is what lets the poll loop tell that apart from a slow investigation.
/** The shape pollForCompletion sees: an in-progress assistant message. */
function message(parts: unknown[], tokens?: Record<string, unknown>): any {
return { info: { role: "assistant", cost: 0, tokens }, parts };
}
test("fingerprint is stable while a reply is unchanged (a wedged request)", () => {
const wedged = () =>
message([{ type: "tool", callID: "c1", state: { status: "completed" } }], {
input: 0,
output: 0,
reasoning: 0,
cache: { read: 0, write: 0 },
});
// Two independent polls of the same untouched message must agree, or the watchdog
// would keep resetting itself and never fire.
expect(progressFingerprint(wedged())).toBe(progressFingerprint(wedged()));
});
test("fingerprint changes on a new tool call", () => {
const before = message([{ type: "tool", callID: "c1", state: { status: "completed" } }]);
const after = message([
{ type: "tool", callID: "c1", state: { status: "completed" } },
{ type: "tool", callID: "c2", state: { status: "running" } },
]);
expect(progressFingerprint(after)).not.toBe(progressFingerprint(before));
});
test("fingerprint changes as a tool advances pending → running → completed", () => {
const at = (status: string) => message([{ type: "tool", callID: "c1", state: { status } }]);
const seen = new Set(["pending", "running", "completed"].map((s) => progressFingerprint(at(s))));
expect(seen.size).toBe(3);
});
test("fingerprint changes as text and reasoning stream in", () => {
const short = message([{ type: "text", text: "abc" }]);
const longer = message([{ type: "text", text: "abcdef" }]);
expect(progressFingerprint(longer)).not.toBe(progressFingerprint(short));
const reasoning = message([{ type: "reasoning", text: "hmm" }]);
const moreReasoning = message([{ type: "reasoning", text: "hmm, and" }]);
expect(progressFingerprint(moreReasoning)).not.toBe(progressFingerprint(reasoning));
});
test("fingerprint changes on token growth alone (no visible parts)", () => {
// A reply can burn reasoning tokens without emitting a part; that is still progress.
const before = message([], { input: 100, output: 0, reasoning: 0, cache: { read: 0 } });
const after = message([], { input: 100, output: 0, reasoning: 512, cache: { read: 0 } });
expect(progressFingerprint(after)).not.toBe(progressFingerprint(before));
});
test("fingerprint tolerates a message with no parts or tokens", () => {
expect(progressFingerprint({})).toBe(progressFingerprint({}));
expect(typeof progressFingerprint({})).toBe("string");
});
test("AgentTimeoutError distinguishes a stall from non-convergence", () => {
const stalled = new AgentTimeoutError("cross-cutting", 25, 0, undefined, "stall");
expect(stalled.reason).toBe("stall");
expect(stalled.message).toContain("silent");
// A stall is still not retryable by the transient-error path (promptAgent already
// retried it once from a clean session).
expect(isTransientApiError(stalled)).toBe(false);
const wandered = new AgentTimeoutError("correctness", 15);
expect(wandered.reason).toBe("time");
expect(wandered.message).toContain("timed out");
});
test("stall window never outlasts the pass it guards", () => {
const MIN = 60_000;
// Long passes (chunk 15m, the elastic cross-file pass): the full 4m window.
expect(stallWindowMs(15 * MIN)).toBe(4 * MIN);
expect(stallWindowMs(50 * MIN)).toBe(4 * MIN);
// Short passes get a proportional window instead of none: the 3m verifier and the
// 4m no-tools fallback would otherwise hit their deadline first and go unprotected.
expect(stallWindowMs(3 * MIN)).toBe(1.5 * MIN);
expect(stallWindowMs(4 * MIN)).toBe(2 * MIN);
// Floored, so a very short cap still allows a slow first token.
expect(stallWindowMs(20_000)).toBe(30_000);
// Invariant across the range: the watchdog fires before the deadline, never after.
for (const cap of [30_000, MIN, 3 * MIN, 10 * MIN, 60 * MIN]) {
expect(stallWindowMs(cap)).toBeLessThanOrEqual(Math.max(cap, 30_000));
}
});
test("a stall is retried exactly once, and only with budget left to land it", () => {
const MIN = 60_000;
// First stall with most of the window left → clean-slate retry.
expect(stallAction(0, 20 * MIN)).toBe("retry");
// Stalled again → salvage. A second wedged attempt would spend the rest of the
// pass's window, which is precisely the failure this mechanism exists to end.
expect(stallAction(1, 20 * MIN)).toBe("soft-land");
expect(stallAction(2, 20 * MIN)).toBe("soft-land");
// Too little left for a fresh attempt to plausibly finish → salvage now.
expect(stallAction(0, 2 * MIN)).toBe("soft-land");
expect(stallAction(0, 0)).toBe("soft-land");
// The threshold must exceed one full stall window, or the retry could only ever
// stall again before it had a chance to produce anything.
expect(stallAction(0, 4 * MIN)).toBe("soft-land");
expect(stallAction(0, 6 * MIN)).toBe("retry");
});
// ---- model preflight ----
//
// A wrong model id hits every pass identically, so it must be caught ONCE up front
// rather than N times as indistinguishable coverage gaps. Motivating failure: a stale
// global `opencode` rejecting `anthropic/claude-opus-4-8` with "Did you mean:
// claude-opus-4-8" — a real config/version problem buried in per-pass noise.
const AVAILABLE = {
anthropic: ["claude-opus-4-8", "claude-sonnet-5", "claude-haiku-4-5"],
openai: ["gpt-5.5"],
};
test("resolvable models produce no complaints", () => {
expect(
findUnknownModels(["anthropic/claude-sonnet-5", "anthropic/claude-opus-4-8"], AVAILABLE),
).toEqual([]);
});
test("unknown provider is reported with the providers that do exist", () => {
const [problem] = findUnknownModels(["bedrock/claude-sonnet-5"], AVAILABLE);
expect(problem?.reason).toBe("provider");
expect(problem?.suggestions).toEqual(["anthropic", "openai"]);
});
test("unknown model suggests near matches from that provider", () => {
const [problem] = findUnknownModels(["anthropic/claude-sonnet-9"], AVAILABLE);
expect(problem?.reason).toBe("model");
// Prefix-matched against the requested id, so the suggestions are the plausible ones.
expect(problem?.suggestions).toEqual(["claude-haiku-4-5", "claude-opus-4-8", "claude-sonnet-5"]);
});
test("a bare model id (no provider) is reported, not silently accepted", () => {
// "claude-sonnet-5" alone is not a valid OpenCode id; it must be provider/model.
const [problem] = findUnknownModels(["claude-sonnet-5"], AVAILABLE);
expect(problem?.reason).toBe("provider");
});
test("only the FIRST slash splits provider from model", () => {
// openrouter-style ids contain a slash in the model half; splitting on every slash
// would make a valid id look unknown.
const available = { openrouter: ["anthropic/claude-sonnet-5"] };
expect(findUnknownModels(["openrouter/anthropic/claude-sonnet-5"], available)).toEqual([]);
});
test("duplicate model ids are reported once", () => {
const problems = findUnknownModels(["x/y", "x/y", "x/y"], AVAILABLE);
expect(problems).toHaveLength(1);
});
test("a refused credential blames the credential, not the model id", () => {
const unknown = findUnknownModels(
["anthropic/claude-sonnet-5", "anthropic/claude-opus-4-8"],
{ openai: ["gpt-5.5"] },
"anthropic",
);
expect(unknown.every((entry) => entry.reason === "credential")).toBe(true);
const text = formatUnknownModels(unknown, {
mode: "oauth",
provider: "anthropic",
tokenEnv: "ANTHROPIC_OAUTH_API_KEY",
});
// The credential is usually FINE: anthropic oauth is a dead path in OpenCode
// (no anthropic OAuth support; Anthropic prohibits subscription tokens in
// third-party tools), so that must be stated BEFORE "your token is wrong" —
// naming the token first sent us to re-issue two perfectly good ones.
expect(text).toContain("credential itself is often FINE");
expect(text.indexOf("cannot work through OpenCode")).toBeLessThan(
text.indexOf("wrong for the mode"),
);
expect(text).toContain('"mode": "api-key"');
expect(text).toContain("ANTHROPIC_OAUTH_API_KEY");
// …and it must NOT read as "your model id is wrong", which is the wrong hunt.
expect(text).not.toContain("no such model");
});
test("a credentialed provider that IS present is not flagged", () => {
expect(
findUnknownModels(
["anthropic/claude-sonnet-5"],
{ anthropic: ["claude-sonnet-5"] },
"anthropic",
),
).toEqual([]);
});
test("the error text names the fix, not just the failure", () => {
const text = formatUnknownModels(findUnknownModels(["anthropic/claude-sonnet-9"], AVAILABLE));
expect(text).toContain("anthropic/claude-sonnet-9");
expect(text).toContain("claude-sonnet-5");
expect(text).toContain("config.jsonc");
expect(text).toContain("ecr doctor");
});
// ---- buildOpencodeConfig: synthesized upstream-alias providers ----
test("an upstream-alias auth entry synthesizes a provider block with exactly the referenced models", () => {
const config = configWith({
agents: [
{ id: "correctness", model: "openai/gpt-5.5" },
{ id: "security", model: "openai-api/gpt-5.5-pro" },
],
coordinatorModel: "openai-api/gpt-5.5-pro",
auth: [
{ mode: "oauth", provider: "openai", tokenEnv: "CODEX_TOKEN" },
{ mode: "api-key", provider: "openai-api", tokenEnv: "OPENAI_API_KEY", upstream: "openai" },
],
});
const opencode = buildOpencodeConfig(config) as {
provider?: Record<string, { npm: string; options: { apiKey: string }; models: object }>;
};
const alias = opencode.provider?.["openai-api"];
expect(alias).toBeDefined();
expect(alias!.npm).toBe("@ai-sdk/openai");
// The key is read straight from the configured env var, never inlined.
expect(alias!.options.apiKey).toBe("{env:OPENAI_API_KEY}");
// Only the ids the roster actually references — the bare model id, provider stripped.
expect(Object.keys(alias!.models)).toEqual(["gpt-5.5-pro"]);
// The real "openai" provider is NOT synthesized — the oauth credential owns it.
expect(opencode.provider?.openai).toBeUndefined();
});
test("no upstream aliases ⇒ no provider key in the OpenCode config at all", () => {
const opencode = buildOpencodeConfig(configWith({})) as Record<string, unknown>;
expect("provider" in opencode).toBe(false);
});
test("an unknown upstream falls back to the openai-compatible SDK", () => {
const config = configWith({
agents: [{ id: "correctness", model: "proxyprov/some-model" }],
auth: [
{ mode: "api-key", provider: "proxyprov", tokenEnv: "PROXY_KEY", upstream: "somegateway" },
],
});
const opencode = buildOpencodeConfig(config) as {
provider?: Record<string, { npm: string }>;
};
expect(opencode.provider?.proxyprov?.npm).toBe("@ai-sdk/openai-compatible");
});
+76 -1
View File
@@ -1,6 +1,11 @@
import { test, expect } from "bun:test";
import { sanitizeUntrusted } from "../core/prompts.js";
import {
sanitizeUntrusted,
buildCrossCuttingTask,
splitCrossCuttingInline,
CROSS_CUTTING_INLINE_MAX_LINES,
} from "../core/prompts.js";
test("strips triple backticks and role/prompt tags", () => {
const out = sanitizeUntrusted("```\nx\n``` <system>hi</system>");
@@ -25,3 +30,73 @@ test("truncates very long input", () => {
test("empty input → empty string", () => {
expect(sanitizeUntrusted("")).toBe("");
});
// ---- cross-file task: diffs are inlined, not read back ----
function file(path: string, changedLines: number): any {
return {
path,
patchPath: `.runs/x/${path}.patch`,
status: "M",
patch: `@@ -1 +1 @@\n${"+line\n".repeat(changedLines).trimEnd()}`,
changedLines,
};
}
const AGENTS: any = [{ id: "correctness", description: "logic bugs" }];
test("cross-file task inlines every changed file's diff on a normal diff", () => {
const files = [file("a.ts", 10), file("b.ts", 20)];
const out = buildCrossCuttingTask(files, AGENTS);
// The diffs themselves are present…
expect(out).toContain("BEGIN DIFF (untrusted) a.ts");
expect(out).toContain("BEGIN DIFF (untrusted) b.ts");
// …so nothing has to be read back, and no patch path is advertised.
expect(out).not.toContain(".patch");
expect(out).toContain("diffs inlined");
});
test("cross-file task defers the tail of a huge diff to patch paths", () => {
// Two files well over the inline budget: the first is inlined, the second deferred.
const files = [file("big.ts", CROSS_CUTTING_INLINE_MAX_LINES), file("tail.ts", 500)];
const out = buildCrossCuttingTask(files, AGENTS);
expect(out).toContain("BEGIN DIFF (untrusted) big.ts");
expect(out).not.toContain("BEGIN DIFF (untrusted) tail.ts");
// The deferred file is still named, with its patch path, so it is never invisible.
expect(out).toContain("tail.ts");
expect(out).toContain(".runs/x/tail.ts.patch");
});
test("splitCrossCuttingInline always inlines at least one file", () => {
// A single file bigger than the whole budget must still be inlined — otherwise the
// pass gets a prompt with no diff in it at all.
const { inlined, deferred } = splitCrossCuttingInline([file("huge.ts", 99_999)]);
expect(inlined.map((f) => f.path)).toEqual(["huge.ts"]);
expect(deferred).toEqual([]);
});
test("splitCrossCuttingInline fills up to the budget, then defers the rest", () => {
const files = [file("a.ts", 60), file("b.ts", 30), file("c.ts", 30)];
const { inlined, deferred } = splitCrossCuttingInline(files, 100);
expect(inlined.map((f) => f.path)).toEqual(["a.ts", "b.ts"]);
expect(deferred.map((f) => f.path)).toEqual(["c.ts"]);
});
test("cross-file task still lists noise-filtered files it cannot see", () => {
const out = buildCrossCuttingTask([file("a.ts", 5)], AGENTS, [
{ path: "schema.graphql", reason: "generated" } as any,
]);
expect(out).toContain("schema.graphql");
expect(out).toContain("generated");
});
test("no-tools cross-file fallback is not told to read files it cannot open", () => {
const files = [file("big.ts", CROSS_CUTTING_INLINE_MAX_LINES), file("tail.ts", 500)];
const out = buildCrossCuttingTask(files, AGENTS, [], { noTools: true });
// The deferred file is still named — an unseen file must never look unchanged…
expect(out).toContain("tail.ts");
// …but a pass with no tools must not be pointed at a patch file to read.
expect(out).not.toContain(".patch");
expect(out).toContain("cannot open them");
expect(out).toContain("Do NOT report that any of them was not updated");
});
+33 -4
View File
@@ -9,7 +9,9 @@ import {
isAuthError,
formatUsageSummary,
renderUsageMarkdown,
effectiveConcurrency,
} from "../core/review.js";
import type { LoadedConfig } from "../config/schema.js";
import type { PatchWorkspaceFile } from "../core/noise.js";
import type { CoordinatorOutput, Finding } from "../core/schema.js";
@@ -174,7 +176,7 @@ test("formatUsageSummary omits cost when zero and reasoning when absent", () =>
expect(s).not.toContain("cost");
});
test("renderUsageMarkdown emits one row per pass, a total, and the cache hit rate", () => {
test("renderUsageMarkdown emits one row per pass with its model, a total, and the cache hit rate", () => {
const s = renderUsageMarkdown(
{
correctness: { input: 1000, output: 200, cache: { read: 9000, write: 500 } },
@@ -183,10 +185,13 @@ test("renderUsageMarkdown emits one row per pass, a total, and the cache hit rat
{ correctness: 0.01, coordinator: 0.002 },
{ input: 1500, output: 300, cache: { read: 9000, write: 900 } },
0.012,
{ correctness: "openai/gpt-5.5" },
);
expect(s).toContain("| correctness | 1000 | 200 | 9000 | 500 | $0.0100 |");
expect(s).toContain("| coordinator | 500 | 100 | 0 | 400 | $0.0020 |");
expect(s).toContain("| **total** | 1500 | 300 | 9000 | 900 | $0.0120 |");
// Each pass names the model that ACTUALLY answered it; a pass the server never
// reported a model for shows "—" rather than pretending to know.
expect(s).toContain("| correctness | openai/gpt-5.5 | 1000 | 200 | 9000 | 500 | $0.0100 |");
expect(s).toContain("| coordinator | — | 500 | 100 | 0 | 400 | $0.0020 |");
expect(s).toContain("| **total** | | 1500 | 300 | 9000 | 900 | $0.0120 |");
// 9000 / (9000 + 1500) ≈ 86%
expect(s).toContain("**86%**");
});
@@ -195,3 +200,27 @@ test("renderUsageMarkdown omits the hit rate when no prompt tokens were used", (
const s = renderUsageMarkdown({}, {}, {}, 0);
expect(s).not.toContain("hit rate");
});
// ---- auth-mode-aware concurrency ----
test("effectiveConcurrency: explicit config wins; subscription defaults lower", () => {
const cfg = (chunk: object, auth: object[]): LoadedConfig =>
({ chunk, auth }) as unknown as LoadedConfig;
// An explicit value always wins, whatever the auth mode.
expect(
effectiveConcurrency(cfg({ concurrency: 8 }, [{ mode: "oauth", provider: "openai" }])),
).toBe(8);
// A subscription (oauth) credential defaults lower: one account handles six
// parallel streams poorly, and several PRs may share the credential.
expect(effectiveConcurrency(cfg({}, [{ mode: "oauth", provider: "openai" }]))).toBe(3);
expect(
effectiveConcurrency(
cfg({}, [
{ mode: "oauth", provider: "openai" },
{ mode: "api-key", provider: "openai-api", upstream: "openai" },
]),
),
).toBe(3);
// Pure API-key runs keep the full default.
expect(effectiveConcurrency(cfg({}, [{ mode: "api-key", provider: "openai" }]))).toBe(6);
});
+1 -1
View File
@@ -354,7 +354,7 @@ test("RoutingManifestSchema: budget defaults to today's totals when absent", ()
const manifest = RoutingManifestSchema.parse({
scopes: [{ name: "default", paths: ["**/*"], config: "." }],
});
expect(manifest.budget).toEqual({ totalPassesMinutes: 32, minScopeMinutes: 5 });
expect(manifest.budget).toEqual({ totalPassesMinutes: 55, minScopeMinutes: 5 });
});
test("RoutingManifestSchema: budget honors custom values", () => {
+91 -6
View File
@@ -88,7 +88,7 @@ test("loadScopeConfig: config '.' reuses the root config, auth from loadAuthFrom
expect(scoped.scopeName).toBe("default");
expect(scoped.commentTag).toBe("root-tag"); // keeps the ROOT marker (risk 8)
expect(scoped.agents.map((a) => a.id).sort()).toEqual(["correctness", "security"]);
expect(scoped.auth.tokenEnv).toBe("MANIFEST_TOKEN"); // from loadAuthFromRoot
expect(scoped.auth[0]?.tokenEnv).toBe("MANIFEST_TOKEN"); // from loadAuthFromRoot
});
test("loadScopeConfig: nested scope reads its own roster/prompts; auth forced from root", async () => {
@@ -111,7 +111,7 @@ test("loadScopeConfig: nested scope reads its own roster/prompts; auth forced fr
expect(scoped.noise.additionalIgnores).toEqual(["www/gen/**"]);
expect(scoped.agents.find((a) => a.id === "style")!.model).toBe("anthropic/claude-opus-4-1");
// auth is forced from the root even though the scope config declares none.
expect(scoped.auth.tokenEnv).toBe("ROOT_TOKEN");
expect(scoped.auth[0]?.tokenEnv).toBe("ROOT_TOKEN");
});
test("loadScopeConfig: a scope declaring auth fails to load (Zod-level rejection)", async () => {
@@ -134,8 +134,8 @@ test("loadAuthFromRoot: defaults.auth (manifest) beats root config auth", async
defaults: { auth: { mode: "oauth", provider: "anthropic", tokenEnv: "MANIFEST" } },
scopes: [{ name: "default", paths: ["**/*"], config: "." }],
});
expect(loadAuthFromRoot(rootConfig, manifest).tokenEnv).toBe("MANIFEST");
expect(loadAuthFromRoot(rootConfig, null).tokenEnv).toBe("ROOT");
expect(loadAuthFromRoot(rootConfig, manifest)[0]?.tokenEnv).toBe("MANIFEST");
expect(loadAuthFromRoot(rootConfig, null)[0]?.tokenEnv).toBe("ROOT");
});
test("loadAuthFromRoot: manifest with defaults but no auth key keeps the root auth (no phantom stub)", async () => {
@@ -153,8 +153,8 @@ test("loadAuthFromRoot: manifest with defaults but no auth key keeps the root au
// silently dropping the root's real oauth credential. It must stay undefined.
expect(manifest.defaults.auth).toBeUndefined();
const auth = loadAuthFromRoot(rootConfig, manifest);
expect(auth.mode).toBe("oauth");
expect(auth.tokenEnv).toBe("ANTHROPIC_OAUTH_API_KEY");
expect(auth[0]?.mode).toBe("oauth");
expect(auth[0]?.tokenEnv).toBe("ANTHROPIC_OAUTH_API_KEY");
});
test("loadScopeConfig: enforceAgents injects the ROOT agent with alwaysRun", async () => {
@@ -278,3 +278,88 @@ test("loadReviewConfig: ECR_CONFIG_DIR loads from the alternate dir; unset → .
// Without the env var, the default dir is used.
expect((await loadReviewConfig(root)).commentTag).toBe("default-marker");
});
// ---- REVIEWER_MODEL override ----
//
// Regression: GitHub Actions passes `${{ vars.REVIEWER_MODEL }}` as an EMPTY STRING when
// that repo variable doesn't exist, which both scaffolded workflows do. `??` only falls
// through on null/undefined, so every configured model silently became "" and each agent
// ran on whatever OpenCode picked by default — a config saying anthropic/claude-sonnet-5
// reviewed with something else entirely, and nothing reported it.
async function modelsWithOverride(value: string | undefined): Promise<string[]> {
const prev = process.env.REVIEWER_MODEL;
if (value === undefined) {
delete process.env.REVIEWER_MODEL;
} else {
process.env.REVIEWER_MODEL = value;
}
try {
const root = await makeRoot(JSON.stringify({ model: "anthropic/claude-sonnet-5" }));
const config = await loadReviewConfig(root);
return [...config.agents.map((a) => a.model), config.coordinator.model];
} finally {
if (prev === undefined) {
delete process.env.REVIEWER_MODEL;
} else {
process.env.REVIEWER_MODEL = prev;
}
}
}
test("an EMPTY REVIEWER_MODEL is ignored, not treated as a model id", async () => {
for (const model of await modelsWithOverride("")) {
expect(model).toBe("anthropic/claude-sonnet-5");
}
});
test("a whitespace-only REVIEWER_MODEL is ignored too", async () => {
for (const model of await modelsWithOverride(" ")) {
expect(model).toBe("anthropic/claude-sonnet-5");
}
});
test("an unset REVIEWER_MODEL uses the configured model", async () => {
for (const model of await modelsWithOverride(undefined)) {
expect(model).toBe("anthropic/claude-sonnet-5");
}
});
test("a real REVIEWER_MODEL still overrides every agent and the coordinator", async () => {
for (const model of await modelsWithOverride("openai/gpt-5.5")) {
expect(model).toBe("openai/gpt-5.5");
}
});
test("a REVIEWER_MODEL with stray whitespace is trimmed, not passed through", async () => {
for (const model of await modelsWithOverride(" openai/gpt-5.5\n")) {
expect(model).toBe("openai/gpt-5.5");
}
});
test("auth.providers map normalizes into one entry per provider (upstream preserved)", async () => {
const root = await makeRoot(
`{ "auth": { "providers": {
"openai": { "mode": "oauth", "tokenEnv": "CODEX_OAUTH_REFRESH_TOKEN" },
"openai-api": { "mode": "api-key", "tokenEnv": "OPENAI_API_KEY", "upstream": "openai" }
} } }`,
);
const config = await loadReviewConfig(root);
expect(config.auth).toEqual([
{
provider: "openai",
mode: "oauth",
tokenEnv: "CODEX_OAUTH_REFRESH_TOKEN",
upstream: undefined,
},
{ provider: "openai-api", mode: "api-key", tokenEnv: "OPENAI_API_KEY", upstream: "openai" },
]);
});
test("legacy single-object auth still parses (one normalized entry)", async () => {
const root = await makeRoot('{ "auth": { "mode": "api-key", "tokenEnv": "OPENAI_API_KEY" } }');
const config = await loadReviewConfig(root);
expect(config.auth).toEqual([
{ provider: "openai", mode: "api-key", tokenEnv: "OPENAI_API_KEY" },
]);
});
+40
View File
@@ -0,0 +1,40 @@
import { test, expect } from "bun:test";
import { planFromAuth, exportLine, opencodeAuthJsonPath } from "../commands/setup-auth.js";
test("plan: the mixed setup needs one ChatGPT login and one manual key", () => {
const plan = planFromAuth([
{ provider: "openai", mode: "oauth", tokenEnv: "CODEX_OAUTH_ACCESS_TOKEN" },
{ provider: "openai-api", mode: "api-key", tokenEnv: "OPENAI_API_KEY", upstream: "openai" },
]);
expect(plan.chatgptLogin).toEqual({ tokenEnv: "CODEX_OAUTH_ACCESS_TOKEN" });
expect(plan.manualKeys).toEqual([
{ provider: "openai-api", tokenEnv: "OPENAI_API_KEY", upstream: "openai" },
]);
expect(plan.unsupported).toEqual([]);
});
test("plan: anthropic oauth is unsupported (dead path), api-key without tokenEnv needs nothing", () => {
const plan = planFromAuth([
{ provider: "anthropic", mode: "oauth", tokenEnv: "ANTHROPIC_OAUTH_API_KEY" },
// Relies on OpenCode's own login — nothing for setup-auth to do.
{ provider: "openai", mode: "api-key" },
]);
expect(plan.chatgptLogin).toBeUndefined();
expect(plan.manualKeys).toEqual([]);
expect(plan.unsupported).toHaveLength(1);
expect(plan.unsupported[0]?.provider).toBe("anthropic");
});
test("export line is single-quoted so shells never expand the value", () => {
expect(exportLine("CODEX_OAUTH_ACCESS_TOKEN", "abc$HOME!x")).toBe(
"export CODEX_OAUTH_ACCESS_TOKEN='abc$HOME!x'",
);
});
test("opencode auth.json path honors XDG_DATA_HOME and falls back to ~/.local/share", () => {
expect(opencodeAuthJsonPath({ XDG_DATA_HOME: "/custom/data" })).toBe(
"/custom/data/opencode/auth.json",
);
expect(opencodeAuthJsonPath({})).toContain("/.local/share/opencode/auth.json");
});
+77
View File
@@ -0,0 +1,77 @@
import { test, expect } from "bun:test";
import { mkdtemp, writeFile, appendFile } from "node:fs/promises";
import { tmpdir } from "node:os";
import path from "node:path";
import { isRateLimitError, stallAction } from "../core/opencode.js";
import { countRateLimitLines, opencodeLogFile, RateLimitWatch } from "../core/throttle.js";
const MIN = 60_000;
test("only ERROR lines with a throttle signature count as evidence", () => {
const chunk = [
// Real evidence, in OpenCode's logfmt shape.
'timestamp=x level=ERROR message="stream error" agent=security error.error="AI_APICallError: 429 Too Many Requests"',
'timestamp=x level=ERROR message="stream error" error.error="Rate limit reached for gpt-5.5"',
// NOT evidence: an INFO line that merely mentions retrying…
"timestamp=x level=INFO message=retry attempt=2",
// …an ERROR with no throttle signature…
'timestamp=x level=ERROR message="stream error" error.error="AI_APICallError: An error occurred"',
// …and a 429-looking number on an INFO line (timestamps contain .429Z!).
"timestamp=2026-07-23T05:20:31.429Z level=INFO message=process",
].join("\n");
expect(countRateLimitLines(chunk)).toBe(2);
});
test("the watch reads incrementally and reports recency", async () => {
const dir = await mkdtemp(path.join(tmpdir(), "ecr-throttle-"));
const file = path.join(dir, "opencode.log");
await writeFile(file, "timestamp=x level=INFO message=init\n");
const watch = new RateLimitWatch(file);
expect(await watch.check()).toBe(0);
expect(watch.recentlyLimited()).toBe(false);
await appendFile(file, 'timestamp=x level=ERROR message="stream error" error.error="429"\n');
expect(await watch.check()).toBe(1);
expect(watch.recentlyLimited()).toBe(true);
// Evidence goes stale outside the recency window.
expect(watch.recentlyLimited(Date.now() + 10 * MIN)).toBe(false);
// Already-read lines are never recounted.
expect(await watch.check()).toBe(1);
});
test("a missing log file is no evidence, not an error", async () => {
const watch = new RateLimitWatch("/nonexistent/opencode.log");
expect(await watch.check()).toBe(0);
expect(watch.recentlyLimited()).toBe(false);
});
test("log path honors XDG_DATA_HOME (the isolated run dir)", () => {
expect(opencodeLogFile({ XDG_DATA_HOME: "/isolated" })).toBe(
"/isolated/opencode/log/opencode.log",
);
});
// ---- patient stall behavior ----
test("a stall WITH rate-limit evidence waits instead of retrying — and keeps waiting", () => {
// Waiting must not consume the single wedged retry: patience is free while
// budget remains, whatever the retry history.
expect(stallAction(0, 20 * MIN, true)).toBe("wait");
expect(stallAction(1, 20 * MIN, true)).toBe("wait");
// Out of room: salvage what exists rather than waiting past the deadline.
expect(stallAction(0, 2 * MIN, true)).toBe("soft-land");
});
test("a stall WITHOUT evidence keeps the wedged-retry behavior", () => {
expect(stallAction(0, 20 * MIN, false)).toBe("retry");
expect(stallAction(1, 20 * MIN, false)).toBe("soft-land");
});
test("rate-limit errors are recognized for the slower backoff schedule", () => {
expect(isRateLimitError(new Error("HTTP 429 Too Many Requests"))).toBe(true);
expect(isRateLimitError(new Error("Rate limit reached for requests"))).toBe(true);
expect(isRateLimitError(new Error("server error 503"))).toBe(false);
});
+189
View File
@@ -0,0 +1,189 @@
import { test, expect } from "bun:test";
import { mkdtemp, mkdir, writeFile, stat } from "node:fs/promises";
import { tmpdir } from "node:os";
import path from "node:path";
import { isAmbientRuntimeConfig, scrubAmbientRuntimeConfig } from "../core/scrub.js";
import { isCommitOid } from "../sources/github-pr.js";
import { hasScopeConfig } from "../config/load.js";
import { resolveReadRoot } from "../core/review.js";
import type { PreparedReadRoot, ReviewSource } from "../sources/source.js";
// ---------------------------------------------------------------------------
// scrub: ambient runtime config never reaches the model runtime's project root
// ---------------------------------------------------------------------------
const exists = async (p: string): Promise<boolean> =>
stat(p).then(
() => true,
() => false,
);
async function makeTree(): Promise<string> {
const root = await mkdtemp(path.join(tmpdir(), "ecr-scrub-test-"));
const put = async (rel: string, dir = false): Promise<void> => {
const full = path.join(root, rel);
if (dir) {
await mkdir(full, { recursive: true });
} else {
await mkdir(path.dirname(full), { recursive: true });
await writeFile(full, "x", "utf8");
}
};
// Ambient runtime config at several depths (all attacker-writable in a PR).
await put("opencode.json");
await put("AGENTS.md");
await put("CLAUDE.md");
await put(".env");
await put(".env.local");
await put(".mcp.json");
await put(".cursorrules");
await put(".opencode/plugin/evil.js");
await put(".claude/settings.json");
await put(".cursor/rules/x.md");
await put("packages/app/opencode.jsonc");
await put("packages/app/AGENTS.md");
await put("packages/app/.env.production");
// Legitimate source content that must survive.
await put("src/index.ts");
await put("packages/app/src/main.ts");
await put("README.md");
await put("environment.ts"); // not a .env file
// Never descended into / never removed as a unit.
await put(".git/config");
await put("node_modules/dep/AGENTS.md");
return root;
}
test("scrubAmbientRuntimeConfig removes runtime config at every depth and keeps source", async () => {
const root = await makeTree();
const removed = await scrubAmbientRuntimeConfig(root);
for (const gone of [
"opencode.json",
"AGENTS.md",
"CLAUDE.md",
".env",
".env.local",
".mcp.json",
".cursorrules",
".opencode",
".claude",
".cursor",
"packages/app/opencode.jsonc",
"packages/app/AGENTS.md",
"packages/app/.env.production",
]) {
expect(await exists(path.join(root, gone))).toBe(false);
}
for (const kept of ["src/index.ts", "packages/app/src/main.ts", "README.md", "environment.ts"]) {
expect(await exists(path.join(root, kept))).toBe(true);
}
// .git is the worktree link back to the real repo; node_modules is skipped.
expect(await exists(path.join(root, ".git/config"))).toBe(true);
expect(await exists(path.join(root, "node_modules/dep/AGENTS.md"))).toBe(true);
// Removed paths are reported (repo-relative) so runs can log them.
expect(removed).toContain("opencode.json");
expect(removed).toContain(path.join("packages", "app", "AGENTS.md"));
expect(removed).not.toContain("README.md");
});
test("isAmbientRuntimeConfig: exact names, .env prefix family, nothing else", () => {
for (const name of [
"opencode.json",
".opencode",
"AGENTS.md",
".env",
".env.local",
".mcp.json",
]) {
expect(isAmbientRuntimeConfig(name)).toBe(true);
}
for (const name of [
"environment.ts",
"env.ts",
"opencode.json.bak",
"agents.md.txt",
"README.md",
]) {
expect(isAmbientRuntimeConfig(name)).toBe(false);
}
});
// ---------------------------------------------------------------------------
// OIDs: only full commit hashes reach security-sensitive git calls
// ---------------------------------------------------------------------------
test("isCommitOid accepts only full 40-hex commit hashes", () => {
expect(isCommitOid("a".repeat(40))).toBe(true);
expect(isCommitOid("0123456789abcdef0123456789abcdef01234567")).toBe(true);
expect(isCommitOid("0123456789ABCDEF0123456789ABCDEF01234567")).toBe(true);
expect(isCommitOid("a".repeat(39))).toBe(false); // short
expect(isCommitOid("a".repeat(41))).toBe(false); // long
expect(isCommitOid("main")).toBe(false); // branch name
expect(isCommitOid("refs/pull/1/head")).toBe(false); // ref
expect(isCommitOid("--upload-pack=evil")).toBe(false); // argument injection
expect(isCommitOid(undefined)).toBe(false);
expect(isCommitOid("")).toBe(false);
});
// ---------------------------------------------------------------------------
// resolveReadRoot: CI fails closed, local degrades softly, null is never an error
// ---------------------------------------------------------------------------
const sourceWith = (
prepare: (() => Promise<PreparedReadRoot | null>) | undefined,
): ReviewSource => ({
getMetadata: async () => ({ title: "", body: "", baseRef: "", headRef: "" }),
getChangedFiles: async () => [],
...(prepare ? { prepareReadRootAsync: prepare } : {}),
});
test("resolveReadRoot: materialization failure is fatal in ci mode", async () => {
const source = sourceWith(async () => {
throw new Error("fetch failed");
});
await expect(resolveReadRoot(source, "ci", () => {})).rejects.toThrow(
/trusted base|fetch failed/,
);
});
test("resolveReadRoot: materialization failure falls back softly in local mode", async () => {
const messages: string[] = [];
const source = sourceWith(async () => {
throw new Error("fetch failed");
});
const root = await resolveReadRoot(source, "local", (m) => messages.push(m));
expect(root).toBeNull();
expect(messages.join("\n")).toContain("fetch failed");
});
test("resolveReadRoot: null (nothing to materialize) is fine in every mode", async () => {
const source = sourceWith(async () => null);
expect(await resolveReadRoot(source, "ci", () => {})).toBeNull();
expect(await resolveReadRoot(source, "local", () => {})).toBeNull();
expect(await resolveReadRoot(sourceWith(undefined), "ci", () => {})).toBeNull();
});
test("resolveReadRoot: a materialized root passes through untouched", async () => {
const handle: PreparedReadRoot = { dir: "/tmp/x", cleanup: async () => {} };
const source = sourceWith(async () => handle);
expect(await resolveReadRoot(source, "ci", () => {})).toBe(handle);
});
// ---------------------------------------------------------------------------
// hasScopeConfig: scope-miss detection against the trusted base root
// ---------------------------------------------------------------------------
test("hasScopeConfig: default scope always exists; nested scope needs config.json(c) at the root", async () => {
const root = await mkdtemp(path.join(tmpdir(), "ecr-scope-test-"));
const scope = { name: "app", config: "packages/app", include: ["packages/app/**"] };
expect(hasScopeConfig(root, { ...scope, config: "." })).toBe(true);
expect(hasScopeConfig(root, scope)).toBe(false); // absent from base → miss
await mkdir(path.join(root, "packages/app/.expo-code-review"), { recursive: true });
expect(hasScopeConfig(root, scope)).toBe(false); // dir alone isn't a config
await writeFile(path.join(root, "packages/app/.expo-code-review/config.jsonc"), "{}", "utf8");
expect(hasScopeConfig(root, scope)).toBe(true);
});
+58 -2
View File
@@ -90,7 +90,7 @@ test("refuses a duplicate tokenEnv across root config.jsonc and routing.jsonc",
);
const result = await verifyConfig(root, { expected: EXPECTED });
expect(result.ok).toBe(false);
expect(problems(result.findings)).toContain("declared in 2 files");
expect(problems(result.findings)).toContain("declared in 2 root files");
});
test("refuses when the tokenEnv differs from the expected value", async () => {
@@ -102,7 +102,7 @@ test("refuses when the tokenEnv differs from the expected value", async () => {
);
const result = await verifyConfig(root, { expected: EXPECTED });
expect(result.ok).toBe(false);
expect(problems(result.findings)).toContain(`!= expected "${EXPECTED}"`);
expect(problems(result.findings)).toContain(`!= expected [${EXPECTED}]`);
});
test("refuses (fail-closed) when a config fails to parse", async () => {
@@ -178,3 +178,59 @@ test("refuses zero tokenEnv occurrences when an expectation is set", async () =>
expect(result.ok).toBe(false);
expect(problems(result.findings)).toContain("no tokenEnv found");
});
// ---- multi-provider auth (auth.providers map) ----
test("a providers map with several tokenEnvs in ONE root file passes against the expected set", async () => {
const root = await makeRoot();
await put(
root,
`${CONFIG_DIRNAME}/config.jsonc`,
`{
"auth": { "providers": {
"openai": { "mode": "oauth", "tokenEnv": "CODEX_OAUTH_REFRESH_TOKEN" },
"openai-api": { "mode": "api-key", "tokenEnv": "OPENAI_API_KEY", "upstream": "openai" }
} }
}`,
);
// Order-insensitive: the workflow may list the set in either order.
const result = await verifyConfig(root, {
expected: "OPENAI_API_KEY,CODEX_OAUTH_REFRESH_TOKEN",
});
expect(result.ok).toBe(true);
});
test("a providers map naming a credential OUTSIDE the expected set is refused", async () => {
const root = await makeRoot();
await put(
root,
`${CONFIG_DIRNAME}/config.jsonc`,
`{
"auth": { "providers": {
"openai": { "mode": "oauth", "tokenEnv": "CODEX_OAUTH_REFRESH_TOKEN" },
"openai-api": { "mode": "api-key", "tokenEnv": "SOME_OTHER_SECRET", "upstream": "openai" }
} }
}`,
);
// A PR must not be able to ADD a forwarded credential any more than repoint one.
const result = await verifyConfig(root, { expected: "CODEX_OAUTH_REFRESH_TOKEN" });
expect(result.ok).toBe(false);
expect(problems(result.findings)).toContain("SOME_OTHER_SECRET");
});
test("two providers entries forwarding the SAME env var are refused", async () => {
const root = await makeRoot();
await put(
root,
`${CONFIG_DIRNAME}/config.jsonc`,
`{
"auth": { "providers": {
"openai": { "mode": "oauth", "tokenEnv": "SHARED" },
"openai-api": { "mode": "api-key", "tokenEnv": "SHARED", "upstream": "openai" }
} }
}`,
);
const result = await verifyConfig(root);
expect(result.ok).toBe(false);
expect(problems(result.findings)).toContain("more than once");
});
+5
View File
@@ -4,6 +4,7 @@ import { dismissCommand } from "./commands/dismiss.js";
import { doctorCommand } from "./commands/doctor.js";
import { initCommand } from "./commands/init.js";
import { reviewCommand } from "./commands/review.js";
import { setupAuthCommand } from "./commands/setup-auth.js";
import { verifyConfigCommand } from "./commands/verify-config.js";
const USAGE = `expo-code-review (ecr) — config-driven AI code reviewer
@@ -14,6 +15,7 @@ Usage:
ecr dismiss --pr <n> <id...> Hide a finding on a PR (see \`ecr dismiss --help\`).
ecr undismiss --pr <n> <id...> Restore a dismissed finding.
ecr init [--monorepo] [--scope <dir>] Scaffold .expo-code-review/ in this repo.
ecr setup-auth [--yes] Walk through getting model credentials for local runs.
ecr doctor [--list-scopes] Check environment, config, credentials, and scopes.
ecr verify-config [--expected <env>] [--json] Refuse to run if a config could redirect the credential (CI guard).
@@ -52,6 +54,9 @@ async function main(): Promise<void> {
case "init":
await initCommand(rest);
break;
case "setup-auth":
await setupAuthCommand(rest);
break;
case "doctor":
await doctorCommand(rest);
break;
+204 -67
View File
@@ -1,6 +1,15 @@
import { readFile } from "node:fs/promises";
import { loadAuthFromRoot, loadReviewConfig, loadScopeConfig } from "../config/load.js";
import path from "node:path";
import {
CONFIG_DIRNAME,
hasScopeConfig,
loadAuthFromRoot,
loadReviewConfig,
loadScopeConfig,
tokenEnvMismatch,
} from "../config/load.js";
import {
loadRoutingManifest,
resolveScopes,
@@ -17,6 +26,7 @@ import type { CoordinatorOutput } from "../core/schema.js";
import { runReview } from "../core/review.js";
import { GitHubPRSource } from "../sources/github-pr.js";
import { memoizeSource } from "../sources/source.js";
import type { PreparedReadRoot, ReviewSource } from "../sources/source.js";
import { GitHubReporter } from "../reporters/github.js";
/** Resolve the PR number from the Actions event payload or GITHUB_REF. */
@@ -48,6 +58,15 @@ For GitHub Actions: reads the PR number + repo from the event/env, gets the diff
via \`gh pr diff\`, runs the reviewer, and upserts a single PR comment. Comment-only
and non-blocking (a reviewer failure never fails the PR's checks).
Trust model: review policy and reviewer configuration (config.jsonc, routing,
prompts, models, auth mapping) load from the PR's immutable BASE commit,
materialized via the GitHub API — never from the PR head — so a PR cannot change
the reviewer that evaluates it; config changes activate after merge. The PR head
is materialized separately (pinned to its immutable OID, scrubbed of ambient
runtime config) purely as source content to read and verify against. If the
trusted base cannot be materialized, the run fails closed with one terminal
comment; it never falls back to the checkout.
Monorepos: when .expo-code-review/routing.jsonc exists, ci fans out INTERNALLY —
it assigns each changed file to exactly one scope (last-match-wins) and reviews
each active scope over only its files, then renders one aggregated comment (or one
@@ -58,8 +77,17 @@ Options:
--route Let the router pick relevant agents from the diff
--scopes <a,b> Limit the fan-out to these named scopes (routing only)
--config-dir <dir> Load the ROOT config.jsonc + routing.jsonc from <dir>
instead of .expo-code-review/ (also ECR_CONFIG_DIR). Scope
subtrees stay repo-root-relative.
instead of .expo-code-review/ (also ECR_CONFIG_DIR). A
RELATIVE dir resolves beneath the trusted base commit; an
ABSOLUTE dir is an explicit operator trust decision. Scope
subtrees always resolve beneath the trusted base commit.
--unsafe-config-from-head
COMPATIBILITY ESCAPE HATCH: load configuration from the
current checkout instead of the PR's trusted base commit.
This lets a same-repo PR change the reviewer (policy,
prompts, model, auth mapping) that evaluates itself.
Never scaffolded; prints a security warning; will be
removed on a scheduled minor boundary.
--comment <mode> Override manifest comment mode: single | per-scope
--force Manual override: review even if the trigger policy (label
trigger / ai-review:skip) would skip. Break-glass and the
@@ -94,6 +122,7 @@ export async function ciCommand(argv: string[] = []): Promise<void> {
// A maintainer's explicit `/review` (comment command or --force) is a manual
// escape hatch that bypasses the trigger-policy gate only (see passesTriggerGate).
const bypassTriggerGate = shouldBypassTriggerGate(argv);
const unsafeConfigFromHead = argv.includes("--unsafe-config-from-head");
const root = await repoRoot();
if (root && root !== process.cwd()) {
process.chdir(root);
@@ -110,37 +139,138 @@ export async function ciCommand(argv: string[] = []): Promise<void> {
return;
}
// A malformed manifest is a loud, non-blocking error (never a silent fallback).
let manifest: RoutingManifest | null;
try {
manifest = await loadRoutingManifest(cwd, { configDir });
} catch (error) {
process.stderr.write(`CI reviewer: invalid routing.jsonc: ${errorMessage(error)}\n`);
return;
}
// ONE source for the whole run: metadata (incl. immutable OIDs), the diff, and
// the PR-head read root are each fetched once and shared across scopes.
const ghSource = new GitHubPRSource({ prNumber, repo, cwd });
const source = memoizeSource(ghSource);
if (manifest == null) {
await runLegacyCi(repo, prNumber, cwd, agents, route, bypassTriggerGate, configDir);
return;
}
try {
await runRoutedCi(
manifest,
repo,
prNumber,
cwd,
agents,
route,
scopesFilter,
commentOverride,
bypassTriggerGate,
configDir,
// Trusted configuration root: review policy and reviewer config load from the
// PR's immutable BASE commit, so the PR head is data, never policy. Fail CLOSED:
// when the base can't be materialized, post the one terminal comment and stop —
// silently reading the checkout would let a head checkout smuggle config in.
let trustedRoot: PreparedReadRoot | null = null;
let configRoot = cwd;
if (unsafeConfigFromHead) {
process.stderr.write(
"CI reviewer: ⚠ SECURITY — --unsafe-config-from-head is set: reviewer configuration " +
"(policy, prompts, models, auth mapping) is being loaded from the current checkout, so " +
"a same-repository PR can change the reviewer that evaluates it. This escape hatch will " +
"be removed in a future minor release.\n",
);
} catch (error) {
// Fan-out failures stay non-blocking (single-writer property is the point).
process.stderr.write(`CI reviewer: routed run failed (non-blocking): ${errorMessage(error)}\n`);
} else {
try {
trustedRoot = await ghSource.prepareTrustedConfigRootAsync();
configRoot = trustedRoot.dir;
} catch (error) {
const reason = errorMessage(error);
process.stderr.write(
`CI reviewer: could not materialize the PR's base commit for trusted configuration ` +
`(failing closed, not reviewing): ${reason}\n`,
);
await postTerminalFailureNote(
repo,
prNumber,
cwd,
`it could not load trusted configuration from the PR's base commit (${reason}). ` +
`This usually means the runner has no git checkout or no usable GH_TOKEN; re-run once fixed`,
);
return;
}
}
try {
// A malformed manifest is a loud, non-blocking error (never a silent fallback).
let manifest: RoutingManifest | null;
try {
manifest = await loadRoutingManifest(configRoot, { configDir });
} catch (error) {
process.stderr.write(`CI reviewer: invalid routing.jsonc: ${errorMessage(error)}\n`);
return;
}
if (manifest == null) {
await runLegacyCi(source, repo, prNumber, cwd, configRoot, {
agents,
route,
bypassTriggerGate,
configDir,
});
return;
}
try {
await runRoutedCi(source, manifest, repo, prNumber, cwd, configRoot, {
agents,
route,
scopesFilter,
commentOverride,
bypassTriggerGate,
configDir,
});
} catch (error) {
// Fan-out failures stay non-blocking (single-writer property is the point).
process.stderr.write(
`CI reviewer: routed run failed (non-blocking): ${errorMessage(error)}\n`,
);
}
} finally {
await source.dispose();
await trustedRoot?.cleanup();
}
}
/**
* The one terminal "this PR was NOT reviewed" comment for failures that happen
* before any configuration is loaded (trusted-root materialization). Uses the
* DEFAULT comment tag/break-glass marker because the config that could customize
* them is exactly what failed to load; a repo with a custom tag gets a fresh
* comment rather than an upsert, which is the acceptable degraded case.
*/
async function postTerminalFailureNote(
repo: string,
prNumber: number,
cwd: string,
reason: string,
): Promise<void> {
try {
const reporter = new GitHubReporter({
prNumber,
repo,
commentTag: "expo-ai-code-reviewer",
breakGlassMarker: "/skip-review",
cwd,
});
await reporter.report({
decision: "approve_with_comments",
findings: [],
summary: `⚠️ The AI reviewer failed to run, so this change was **not** reviewed: ${reason}.`,
incomplete: [],
couldNotComplete: true,
});
} catch (postError) {
process.stderr.write(
`CI reviewer: also failed to post the failure notice: ${errorMessage(postError)}\n`,
);
}
}
/** Options shared by the legacy and routed CI paths (parsed from argv once). */
interface CiRunOptions {
agents: string[] | undefined;
route: boolean;
scopesFilter?: string[] | undefined;
commentOverride?: "single" | "per-scope" | undefined;
bypassTriggerGate: boolean;
configDir: string | undefined;
}
/**
* Run-log + patch-workspace anchor: ALWAYS the workspace checkout, never the
* (temporary, removed-on-exit) trusted config root — the workflow uploads
* `.expo-code-review/.runs/reviews.jsonl` from the workspace as an artifact.
*/
function workspaceRunsDir(cwd: string): string {
return path.join(cwd, CONFIG_DIRNAME, ".runs");
}
/**
@@ -148,17 +278,17 @@ export async function ciCommand(argv: string[] = []): Promise<void> {
* routing.jsonc the CLI behaves exactly as before (backcompat invariant).
*/
async function runLegacyCi(
source: ReviewSource,
repo: string,
prNumber: number,
cwd: string,
agents: string[] | undefined,
route: boolean,
bypassTriggerGate: boolean,
configDir: string | undefined,
configRoot: string,
options: CiRunOptions,
): Promise<void> {
const { agents, route, bypassTriggerGate, configDir } = options;
let config;
try {
config = await loadReviewConfig(cwd, { configDir });
config = await loadReviewConfig(configRoot, { configDir });
} catch (error) {
process.stderr.write(`CI reviewer: ${errorMessage(error)}\n`);
return;
@@ -169,11 +299,12 @@ async function runLegacyCi(
// bash guard is a text sweep (layer 2) and can't see through JSON escapes; this
// check compares the tokenEnv the loader actually honors.
const expectedTokenEnv = process.env.ECR_EXPECTED_TOKEN_ENV;
if (expectedTokenEnv && config.auth.tokenEnv !== expectedTokenEnv) {
process.stderr.write(
`CI reviewer: auth.tokenEnv "${config.auth.tokenEnv ?? "(none)"}" != ECR_EXPECTED_TOKEN_ENV "${expectedTokenEnv}"; refusing to run.\n`,
);
return;
if (expectedTokenEnv) {
const mismatch = tokenEnvMismatch(config.auth, expectedTokenEnv);
if (mismatch) {
process.stderr.write(`CI reviewer: ${mismatch}; refusing to run.\n`);
return;
}
}
// Config-driven trigger policy (.expo-code-review/config.jsonc → review): decide
@@ -205,11 +336,12 @@ async function runLegacyCi(
}
try {
const review = await runReview(new GitHubPRSource({ prNumber, repo, cwd }), {
const review = await runReview(source, {
config,
mode: "ci",
agents,
route,
runsDir: workspaceRunsDir(cwd),
onProgress: (message) => process.stderr.write(`${message}\n`),
});
await reporter.report(review);
@@ -248,20 +380,19 @@ function failureReview(scopeName: string, reason: string): CoordinatorOutput {
/** The routing fan-out: one process, N scopes reviewed sequentially, one render. */
async function runRoutedCi(
source: ReviewSource & { dispose(): Promise<void> },
manifest: RoutingManifest,
repo: string,
prNumber: number,
cwd: string,
agents: string[] | undefined,
route: boolean,
scopesFilter: string[] | undefined,
commentOverride: "single" | "per-scope" | undefined,
bypassTriggerGate: boolean,
configDir: string | undefined,
configRoot: string,
options: CiRunOptions,
): Promise<void> {
const { agents, route, scopesFilter, commentOverride, bypassTriggerGate, configDir } = options;
// The root config + manifest follow the override; scope configs stay
// repo-root-relative (loadScopeConfig reads <root>/<scope.config>/.expo-code-review).
const rootConfig = await loadReviewConfig(cwd, { configDir });
// relative to the TRUSTED root (loadScopeConfig reads
// <configRoot>/<scope.config>/.expo-code-review).
const rootConfig = await loadReviewConfig(configRoot, { configDir });
// The root/aggregate marker is the ACTUAL root-owned comment tag so the
// pre-routing comment and its dismissal state upsert in place, not stranded
// under a new marker (risk 8/9). manifest.defaults.commentTag is the
@@ -296,10 +427,9 @@ async function runRoutedCi(
const expectedTokenEnv = process.env.ECR_EXPECTED_TOKEN_ENV;
if (expectedTokenEnv) {
const honored = loadAuthFromRoot(rootConfig, manifest);
if (honored.tokenEnv !== expectedTokenEnv) {
process.stderr.write(
`CI reviewer: honored auth.tokenEnv "${honored.tokenEnv ?? "(none)"}" != ECR_EXPECTED_TOKEN_ENV "${expectedTokenEnv}"; refusing to run.\n`,
);
const mismatch = tokenEnvMismatch(honored, expectedTokenEnv);
if (mismatch) {
process.stderr.write(`CI reviewer: honored ${mismatch}; refusing to run.\n`);
return;
}
}
@@ -317,7 +447,6 @@ async function runRoutedCi(
return;
}
const source = memoizeSource(new GitHubPRSource({ prNumber, repo, cwd }));
const changed = await source.getChangedFiles();
const resolution = resolveScopes(
manifest,
@@ -364,7 +493,6 @@ async function runRoutedCi(
if (await bgReporter.checkBreakGlass()) {
process.stderr.write(`CI reviewer: ${rootConfig.breakGlassMarker} detected; skipping.\n`);
await bgReporter.postSkipNote();
await source.dispose();
return;
}
} catch (error) {
@@ -374,21 +502,17 @@ async function runRoutedCi(
}
// Build ONE link context for all scopes (rate-limit hygiene): diff lines from the
// already-fetched changed files, base SHA via a single `gh pr view`.
// already-fetched changed files, base OID from the memoized PR metadata (the same
// immutable OID the trusted config root was materialized from).
const link: LinkContext = {
repo,
prNumber,
diffLines: buildDiffLineIndex(changed.map((file) => ({ path: file.path, patch: file.patch }))),
};
try {
const { stdout } = await run(
"gh",
["pr", "view", String(prNumber), "--repo", repo, "--json", "baseRefOid"],
{ cwd },
);
const oid = (JSON.parse(stdout) as { baseRefOid?: string }).baseRefOid;
if (oid) {
link.baseSha = oid;
const { baseOid } = await source.getMetadata();
if (baseOid) {
link.baseSha = baseOid;
}
} catch {
// leave baseSha unset → out-of-diff findings degrade to plain text
@@ -414,7 +538,20 @@ async function runRoutedCi(
const isDefault = scope.configDir === ".";
let review: CoordinatorOutput;
try {
const config = await loadScopeConfig(cwd, scopeDef, manifest, rootConfig);
// A scope whose config dir doesn't exist at the TRUSTED base commit is a
// scope this PR introduces: review it with the root config rather than
// failing the run on exactly that PR. The scope's own config (PR-owned,
// untrusted for this run) activates once it merges.
let effectiveScopeDef = scopeDef;
if (!hasScopeConfig(configRoot, scopeDef)) {
process.stderr.write(
`CI reviewer: [${scope.name}] no config at "${scopeDef.config}" in the PR's base ` +
`commit (new in this PR?); reviewing with the root config — the scope's config ` +
`takes effect after merge.\n`,
);
effectiveScopeDef = { ...scopeDef, config: "." };
}
const config = await loadScopeConfig(configRoot, effectiveScopeDef, manifest, rootConfig);
review = await runReview(source, {
config,
mode: "ci",
@@ -422,6 +559,7 @@ async function runRoutedCi(
route,
includePaths: scope.files,
passesBudgetMs: budget,
runsDir: workspaceRunsDir(cwd),
onProgress: (message) => process.stderr.write(`[${scope.name}] ${message}\n`),
});
} catch (error) {
@@ -431,7 +569,6 @@ async function runRoutedCi(
}
results.push({ scope: scope.name, isDefault, review });
}
await source.dispose();
const reporterFor = (tag: string, withLink = false): GitHubReporter =>
new GitHubReporter({
+87 -11
View File
@@ -4,6 +4,7 @@ import {
loadAuthFromRoot,
hasConfig,
resolveConfigDir,
tokenEnvMismatch,
} from "../config/load.js";
import {
loadRoutingManifest,
@@ -11,11 +12,16 @@ import {
scopePassesBudgetMs,
formatOwnerTable,
} from "../config/routing.js";
import readline from "node:readline/promises";
import type { LoadedScopeConfig } from "../config/load.js";
import type { RoutingManifest } from "../config/schema.js";
import { setupAuthCommand } from "./setup-auth.js";
import { checkProviderAuth } from "../core/auth.js";
import { opencodeBinSource } from "../core/opencode.js";
import { git, onPath, repoRoot, run } from "../core/exec.js";
import { errorMessage } from "../core/util.js";
import path from "node:path";
const USAGE = `ecr doctor — check environment, config, and credentials
@@ -31,6 +37,20 @@ Options:
--list-scopes Print the routing scope table (name, dir, paths, agents, tag)
`;
/**
* `opencode --version`, run from `binDir` if given (so the bundled CLI can be asked
* directly) or from PATH otherwise. Null when it can't be determined — a version we
* can't read is worth staying quiet about, not failing over.
*/
async function opencodeVersion(binDir: string | null): Promise<string | null> {
const command = binDir ? path.join(binDir, "opencode") : "opencode";
const { stdout, code } = await run(command, ["--version"], { check: false });
if (code !== 0) {
return null;
}
return stdout.trim().split("\n")[0]?.trim() || null;
}
/** Preflight checks so a broken setup surfaces clearly instead of silently no-opping. */
export async function doctorCommand(argv: string[] = []): Promise<void> {
if (argv.includes("-h") || argv.includes("--help")) {
@@ -69,13 +89,38 @@ export async function doctorCommand(argv: string[] = []): Promise<void> {
);
}
const opencodeInstalled = await onPath("opencode");
// The SDK spawns a bare `opencode`, so the version that actually runs is a PATH
// lookup. Report which one wins and whether it matches the version this package
// pins: a stale global install against a newer SDK rejects model ids the SDK
// considers valid (`ProviderModelNotFoundError`), which is otherwise a baffling
// failure that only reproduces on one machine. `startOpencode` prepends our own
// bin dir so the pinned one wins at runtime — this just makes the drift visible.
const bin = opencodeBinSource();
const opencodeInstalled = (await onPath("opencode")) || bin.pinned;
line(
opencodeInstalled,
opencodeInstalled
? "opencode CLI found on PATH"
: "opencode CLI NOT on PATH (install `opencode-ai`, or add node_modules/.bin to PATH)",
? "opencode CLI available"
: "opencode CLI NOT found (install `opencode-ai`, or add node_modules/.bin to PATH)",
);
if (opencodeInstalled) {
const pinnedVersion = bin.pinned ? await opencodeVersion(bin.dir!) : null;
const pathVersion = await opencodeVersion(null);
if (pinnedVersion) {
line(true, `opencode ${pinnedVersion} (bundled with this reviewer; used at runtime)`);
if (pathVersion && pathVersion !== pinnedVersion) {
warn(
`a different opencode ${pathVersion} is first on your PATH — runs use the bundled ${pinnedVersion}, ` +
`but other tooling (and \`opencode\` by hand) will use ${pathVersion}`,
);
}
} else if (pathVersion) {
warn(
`using opencode ${pathVersion} from PATH — this reviewer's own \`opencode-ai\` dependency could not be ` +
`resolved, so the CLI and SDK versions can drift (a stale CLI rejects model ids the SDK accepts)`,
);
}
}
line(await onPath("git"), "git found on PATH");
@@ -115,6 +160,32 @@ export async function doctorCommand(argv: string[] = []): Promise<void> {
const readiness = checkProviderAuth(rootConfig);
line(readiness.ok, `auth: ${readiness.detail}`);
// A suspicious-but-not-provably-broken credential: worth saying, never a failure
// (the shape rules are heuristics — see checkOauthTokenShape).
if (readiness.warning) {
warn(`auth: ${readiness.warning}`);
}
// A missing credential has a guided fix — offer it right here when someone is
// at the terminal, rather than making them find the command in the README.
if (!readiness.ok) {
if (process.stdin.isTTY && process.stdout.isTTY) {
const rl = readline.createInterface({ input: process.stdin, output: process.stderr });
let runIt = false;
try {
const answer = (await rl.question(" Run `ecr setup-auth` to fix this now? [Y/n] "))
.trim()
.toLowerCase();
runIt = answer === "" || answer === "y" || answer === "yes";
} finally {
rl.close();
}
if (runIt) {
await setupAuthCommand([]);
}
} else {
info("run `ecr setup-auth` for a guided credential setup");
}
}
} catch (error) {
line(false, `config invalid: ${errorMessage(error)}`);
}
@@ -188,16 +259,21 @@ export async function doctorCommand(argv: string[] = []): Promise<void> {
const hasManifestAuth = Boolean(manifest.defaults.auth);
line(
true,
`auth singleton: honored from ${hasManifestAuth ? "routing.jsonc defaults.auth" : "root config.jsonc"} (${auth.mode}/${auth.provider})`,
`auth singleton: honored from ${hasManifestAuth ? "routing.jsonc defaults.auth" : "root config.jsonc"} ` +
`(${auth.map((entry) => `${entry.mode}/${entry.provider}`).join(", ")})`,
);
if (auth.tokenEnv) {
const expected = process.env.ECR_EXPECTED_TOKEN_ENV;
if (expected && expected !== auth.tokenEnv) {
line(false, `auth.tokenEnv "${auth.tokenEnv}" != ECR_EXPECTED_TOKEN_ENV "${expected}"`);
} else {
const expected = process.env.ECR_EXPECTED_TOKEN_ENV;
if (expected) {
const mismatch = tokenEnvMismatch(auth, expected);
if (mismatch) {
line(false, `auth: ${mismatch}`);
}
}
for (const entry of auth) {
if (entry.tokenEnv) {
line(
Boolean(process.env[auth.tokenEnv]),
`auth token env ${auth.tokenEnv} is ${process.env[auth.tokenEnv] ? "set" : "NOT set"}`,
Boolean(process.env[entry.tokenEnv]),
`auth token env ${entry.tokenEnv} (${entry.provider}) is ${process.env[entry.tokenEnv] ? "set" : "NOT set"}`,
);
}
}
+5 -3
View File
@@ -40,9 +40,11 @@ Options:
--no-fail always exit 0, even on request-changes
-h, --help show this help
Note: agents read the local working tree for surrounding context, so --pr uses the
PR's diff (authoritative) but your checked-out files for context. For full fidelity
on a PR, \`gh pr checkout <n>\` first, then run a plain \`ecr review\`.
Note: with --repo (or in CI), --pr materializes the PR-head tree (pinned to its
immutable commit, scrubbed of ambient runtime config) so reads match the PR; if
that isn't possible, it falls back to your checked-out files with a warning.
Config always loads from YOUR checkout in local runs — you are the trust
principal here. In \`ecr ci\`, config loads from the PR's trusted base commit.
Exit codes: 0 approve / approve-with-comments, 1 request-changes, 2 error.
`;
+247
View File
@@ -0,0 +1,247 @@
import { spawnSync } from "node:child_process";
import { readFile } from "node:fs/promises";
import os from "node:os";
import path from "node:path";
import readline from "node:readline/promises";
import { hasConfig, loadReviewConfig } from "../config/load.js";
import type { AuthConfigEntry } from "../config/schema.js";
import { jwtExpiryMs } from "../core/auth.js";
import { opencodeBinSource } from "../core/opencode.js";
import { errorMessage } from "../core/util.js";
const USAGE = `ecr setup-auth — set up model credentials for local runs
Reads this repo's .expo-code-review/config.jsonc auth entries and walks through
getting each credential:
• a ChatGPT/Codex subscription (oauth/openai): runs the bundled
\`opencode auth login\` (interactive; opens your browser), then prints the
\`export <tokenEnv>=…\` line to add to your shell config. An existing
OpenCode ChatGPT sign-in is reused instead of re-authenticating.
• an API key (api-key entries): prints where to create the key, the exact
permissions it needs, and the export line to fill in.
Without a repo config, it offers the recommended ChatGPT/Codex subscription flow
with the default env name.
Options:
--yes Skip confirmation prompts (still interactive during the login itself).
`;
/** What setup-auth must do for a given auth config. Pure, so it's testable. */
export interface SetupPlan {
/** oauth/openai entry — satisfiable by an `opencode auth login` ChatGPT sign-in. */
chatgptLogin?: { tokenEnv: string };
/** api-key entries — a human must mint these; we print instructions. */
manualKeys: Array<{ provider: string; tokenEnv: string; upstream?: string }>;
/** oauth entries for providers we have no automated flow for. */
unsupported: AuthConfigEntry[];
}
export function planFromAuth(auth: AuthConfigEntry[]): SetupPlan {
const plan: SetupPlan = { manualKeys: [], unsupported: [] };
for (const entry of auth) {
if (entry.mode === "oauth" && entry.provider === "openai" && entry.tokenEnv) {
plan.chatgptLogin = { tokenEnv: entry.tokenEnv };
} else if (entry.mode === "api-key" && entry.tokenEnv) {
plan.manualKeys.push({
provider: entry.provider,
tokenEnv: entry.tokenEnv,
upstream: entry.upstream,
});
} else if (entry.mode === "oauth") {
plan.unsupported.push(entry);
}
// api-key without tokenEnv relies on OpenCode's own login — nothing to set up.
}
return plan;
}
/** Where OpenCode's own (non-isolated) auth.json lives. */
export function opencodeAuthJsonPath(env: NodeJS.ProcessEnv = process.env): string {
const dataHome = env.XDG_DATA_HOME || path.join(os.homedir(), ".local", "share");
return path.join(dataHome, "opencode", "auth.json");
}
/**
* The stored ChatGPT sign-in's ACCESS token, if OpenCode has a live one. The
* refresh token deliberately never leaves OpenCode's store: refresh tokens are
* SINGLE-USE (rotation) and OpenCode is their sole legitimate consumer — a copy
* in a shell config or CI secret dies on the next rotation and can take the
* whole sign-in with it. The access token is a plain bearer that stays valid for
* days and never touches rotation.
*/
async function readStoredAccessToken(): Promise<{ token: string; expiresMs: number } | null> {
try {
const raw = await readFile(opencodeAuthJsonPath(), "utf8");
const parsed = JSON.parse(raw) as Record<
string,
{ type?: string; access?: string } | undefined
>;
const openai = parsed.openai;
if (openai?.type !== "oauth" || !openai.access) {
return null;
}
const expiresMs = jwtExpiryMs(openai.access) ?? 0;
// An expired stored token means the sign-in needs redoing anyway.
return expiresMs > Date.now() ? { token: openai.access, expiresMs } : null;
} catch {
return null;
}
}
async function confirm(question: string, skip: boolean): Promise<boolean> {
if (skip) {
return true;
}
const rl = readline.createInterface({ input: process.stdin, output: process.stderr });
try {
const answer = (await rl.question(`${question} [Y/n] `)).trim().toLowerCase();
return answer === "" || answer === "y" || answer === "yes";
} finally {
rl.close();
}
}
/** The line to paste into a shell config. Single-quoted: tokens never contain '. */
export function exportLine(tokenEnv: string, value: string): string {
return `export ${tokenEnv}='${value}'`;
}
export async function setupAuthCommand(argv: string[] = []): Promise<void> {
if (argv.includes("-h") || argv.includes("--help")) {
process.stdout.write(USAGE);
return;
}
const yes = argv.includes("--yes");
const out = (line = ""): boolean => process.stdout.write(`${line}\n`);
const err = (line = ""): boolean => process.stderr.write(`${line}\n`);
try {
// Plan from the repo config when there is one; otherwise offer the
// recommended subscription flow with the default env name.
let plan: SetupPlan;
if (hasConfig(process.cwd())) {
const config = await loadReviewConfig(process.cwd());
plan = planFromAuth(config.auth);
} else {
err("No .expo-code-review config here — setting up the default ChatGPT/Codex flow.");
plan = planFromAuth([
{ provider: "openai", mode: "oauth", tokenEnv: "CODEX_OAUTH_ACCESS_TOKEN" },
]);
}
if (!plan.chatgptLogin && plan.manualKeys.length === 0 && plan.unsupported.length === 0) {
out(
"This repo's auth config needs no local credential setup (OpenCode's own login covers it).",
);
return;
}
const exports: string[] = [];
if (plan.chatgptLogin) {
const { tokenEnv } = plan.chatgptLogin;
if (process.env[tokenEnv]) {
err(`✓ ${tokenEnv} is already set in this shell — skipping the ChatGPT sign-in.`);
} else {
let stored = await readStoredAccessToken();
if (stored) {
err(
`Found a live ChatGPT sign-in in OpenCode (access token valid ` +
`${Math.max(1, Math.round((stored.expiresMs - Date.now()) / 86_400_000))} more day(s)).`,
);
if (!(await confirm(`Use it for ${tokenEnv}?`, yes))) {
stored = null;
}
}
if (!stored) {
err("This will run the bundled `opencode auth login` (interactive).");
err("When it prompts:");
err(" 1. select the provider: OpenAI");
err(" 2. select the method: Sign in with ChatGPT (Codex subscription)");
err(" 3. your browser opens — sign in and authorize.");
if (!(await confirm("Run it now?", yes))) {
err("Skipped the ChatGPT sign-in.");
} else {
const binDir = opencodeBinSource().dir;
const opencode = binDir ? path.join(binDir, "opencode") : "opencode";
const result = spawnSync(opencode, ["auth", "login"], { stdio: "inherit" });
if (result.status !== 0) {
throw new Error(
`\`opencode auth login\` exited with ${result.status ?? "a signal"}; nothing was changed.`,
);
}
stored = await readStoredAccessToken();
if (!stored) {
throw new Error(
"The login finished but no live ChatGPT sign-in was stored — did you select " +
'OpenAI → "Sign in with ChatGPT"? Re-run `ecr setup-auth` to try again.',
);
}
}
}
if (stored) {
// The ACCESS token: a plain bearer, valid for days, no rotation involved.
// (The refresh token stays in OpenCode's store — it is single-use, and
// copying it anywhere kills it on the next rotation.)
exports.push(exportLine(tokenEnv, stored.token));
err(
`Note: this access token expires in ~${Math.max(1, Math.round((stored.expiresMs - Date.now()) / 86_400_000))} day(s); ` +
`re-run \`ecr setup-auth\` then to refresh it (your OpenCode sign-in stays valid).`,
);
}
}
}
for (const key of plan.manualKeys) {
if (process.env[key.tokenEnv]) {
err(`✓ ${key.tokenEnv} is already set in this shell — skipping.`);
continue;
}
const upstream = key.upstream ?? key.provider;
err("");
err(`${key.tokenEnv} (${key.provider}) is an API key — create it by hand:`);
if (upstream === "openai") {
err(" https://platform.openai.com/api-keys — in a dedicated project (set a");
err(" monthly budget), as a RESTRICTED key with exactly two permissions, both");
err(" under Model capabilities: Responses → Request, Chat completions → Request.");
err(" Everything else (including List models) stays None.");
} else if (upstream === "anthropic") {
err(" https://console.anthropic.com/settings/keys — a workspace-scoped key");
err(" with a spend limit is all the reviewer needs.");
} else {
err(` mint a key for the "${upstream}" provider.`);
}
exports.push(exportLine(key.tokenEnv, "<paste the key here>"));
}
for (const entry of plan.unsupported) {
err("");
err(
`auth for "${entry.provider}" is mode "oauth", which has no automated setup flow here` +
(entry.provider === "anthropic"
? " — and cannot work: Anthropic prohibits subscription tokens in third-party tools. Use an API key instead."
: `. Set ${entry.tokenEnv ?? "its token env"} manually.`),
);
}
if (exports.length > 0) {
const rc = process.env.SHELL?.includes("zsh") ? "~/.zshrc" : "your shell config";
err("");
err(`Add ${exports.length === 1 ? "this line" : "these lines"} to ${rc}:`);
out("");
for (const line of exports) {
out(` ${line}`);
}
out("");
err(`Then restart your shell (or \`source ${rc}\`) and run \`ecr doctor\` to verify.`);
} else {
err("");
err("Nothing to add — run `ecr doctor` to verify your setup.");
}
} catch (error) {
err(`setup-auth failed: ${errorMessage(error)}`);
process.exitCode = 1;
}
}
+71 -29
View File
@@ -16,15 +16,17 @@ The canonical pre-review guard (ships with the CLI). It sweeps EVERY
plain recursive walk (skipping node_modules/.git, so a staged-but-unreferenced
config can't hide from git's index), parses each with the real comment-aware JSONC
parser (never regex-scraping), and refuses to run (exit 1) when:
• auth.tokenEnv (config) / defaults.auth.tokenEnv (routing.jsonc) appears more
than once, or in a non-root file, or — with --expected / ECR_EXPECTED_TOKEN_ENV
set — differs from the expected name or is absent (count must be exactly one);
• a tokenEnv (auth.tokenEnv, or any auth.providers.<id>.tokenEnv; routing.jsonc:
same under defaults.auth) appears in a non-root file, in more than one root
file, twice under the same name, or — with --expected / ECR_EXPECTED_TOKEN_ENV
set (comma-separated) — the declared set differs from the expected set;
• a non-root config declares auth, breakGlass, or commentTag (root-locked keys);
• any file fails to parse (fail-closed), reporting the parse error.
Exit 0 = safe to run the review.
Options:
--expected <ENV_NAME> Require tokenEnv to equal this (else ECR_EXPECTED_TOKEN_ENV).
--expected <ENVS> Require the declared tokenEnv set to equal this
comma-separated set (else ECR_EXPECTED_TOKEN_ENV).
--json Emit {ok, findings:[{file, problem}]} on stdout.
`;
@@ -40,8 +42,11 @@ export interface VerifyResult {
}
interface ConfigFacts {
/** tokenEnv value declared (config auth.tokenEnv / routing defaults.auth.tokenEnv). */
tokenEnv?: string;
/**
* Every tokenEnv value declared: legacy `auth.tokenEnv`, or one per entry of
* the `auth.providers` map (routing.jsonc: same under `defaults.auth`).
*/
tokenEnvs: string[];
declaresAuth: boolean;
declaresBreakGlass: boolean;
declaresCommentTag: boolean;
@@ -88,22 +93,39 @@ function asObject(value: unknown): Record<string, unknown> | undefined {
: undefined;
}
/** Every tokenEnv an auth block names — legacy single, or one per providers entry. */
function collectTokenEnvs(auth: Record<string, unknown> | undefined): string[] {
if (!auth) {
return [];
}
const found: string[] = [];
if (typeof auth.tokenEnv === "string") {
found.push(auth.tokenEnv);
}
const providers = asObject(auth.providers);
for (const entry of Object.values(providers ?? {})) {
const tokenEnv = asObject(entry)?.tokenEnv;
if (typeof tokenEnv === "string") {
found.push(tokenEnv);
}
}
return found;
}
/** Read the security-relevant declarations from a parsed config/routing object. */
function extractFacts(file: string, parsed: Record<string, unknown>): ConfigFacts {
if (path.basename(file) === ROUTING_FILENAME) {
// routing.jsonc locks auth under defaults.auth (defaults.auth.tokenEnv).
const defaults = asObject(parsed.defaults);
const auth = asObject(defaults?.auth);
return {
tokenEnv: typeof auth?.tokenEnv === "string" ? auth.tokenEnv : undefined,
tokenEnvs: collectTokenEnvs(asObject(defaults?.auth)),
declaresAuth: Boolean(defaults) && "auth" in defaults!,
declaresBreakGlass: false, // routing.jsonc has no breakGlass concept
declaresCommentTag: Boolean(defaults) && "commentTag" in defaults!,
};
}
const auth = asObject(parsed.auth);
return {
tokenEnv: typeof auth?.tokenEnv === "string" ? auth.tokenEnv : undefined,
tokenEnvs: collectTokenEnvs(asObject(parsed.auth)),
declaresAuth: "auth" in parsed,
declaresBreakGlass: "breakGlass" in parsed,
declaresCommentTag: "commentTag" in parsed,
@@ -145,8 +167,8 @@ export async function verifyConfig(
}
const facts = extractFacts(file, object);
if (facts.tokenEnv !== undefined) {
tokenEnvOccurrences.push({ file: rel(file), value: facts.tokenEnv, isRoot });
for (const value of facts.tokenEnvs) {
tokenEnvOccurrences.push({ file: rel(file), value, isRoot });
}
if (!isRoot) {
@@ -169,38 +191,58 @@ export async function verifyConfig(
}
}
// tokenEnv must appear at most once, only in a root-owned file.
// tokenEnvs may only be declared in root-owned files…
for (const occurrence of tokenEnvOccurrences.filter((o) => !o.isRoot)) {
findings.push({
file: occurrence.file,
problem: `tokenEnv "${occurrence.value}" is declared outside the root config; only a root-owned config.jsonc/config.json or routing.jsonc may name the forwarded credential`,
});
}
if (tokenEnvOccurrences.length > 1) {
// …and all in ONE root file (multiple entries in one auth block are fine —
// that's the multi-provider map — but split across files there is no single
// honored source and a stale/staged second file could smuggle a credential).
const rootOccurrences = tokenEnvOccurrences.filter((o) => o.isRoot);
const rootFiles = [...new Set(rootOccurrences.map((o) => o.file))];
if (rootFiles.length > 1) {
findings.push({
file: tokenEnvOccurrences.map((o) => o.file).join(", "),
problem: `tokenEnv is declared in ${tokenEnvOccurrences.length} files; it must appear exactly once, in a root-owned config`,
file: rootFiles.join(", "),
problem: `tokenEnv is declared in ${rootFiles.length} root files; all credential env names must live in ONE root-owned config`,
});
}
// Duplicate names within a file are a config bug worth failing on too: two auth
// entries forwarding the same env var means one of them is misconfigured.
const seen = new Set<string>();
for (const occurrence of rootOccurrences) {
if (seen.has(occurrence.value)) {
findings.push({
file: occurrence.file,
problem: `tokenEnv "${occurrence.value}" is declared more than once; each credential env name must appear exactly once`,
});
}
seen.add(occurrence.value);
}
// With an expectation set, exactly one root occurrence equal to it is required.
// With an expectation set, the declared names must equal the expected SET
// exactly (comma-separated; order-insensitive). A missing name is as much a
// finding as an extra one — a PR must not add, drop, or repoint credentials.
const expected = options.expected;
if (expected) {
const rootOccurrences = tokenEnvOccurrences.filter((o) => o.isRoot);
if (rootOccurrences.length === 0) {
const expectedSet = expected
.split(",")
.map((name) => name.trim())
.filter(Boolean)
.sort();
const declared = [...seen].sort();
if (declared.length === 0) {
findings.push({
file: path.join(CONFIG_DIRNAME, "config.jsonc"),
problem: `no tokenEnv found, but an expected value "${expected}" is set — exactly one root-owned tokenEnv is required`,
problem: `no tokenEnv found, but expected "${expectedSet.join(", ")}" — the root-owned config must name exactly those credential env(s)`,
});
} else if (JSON.stringify(declared) !== JSON.stringify(expectedSet)) {
findings.push({
file: rootFiles.join(", ") || path.join(CONFIG_DIRNAME, "config.jsonc"),
problem: `declared tokenEnv set [${declared.join(", ")}] != expected [${expectedSet.join(", ")}] — a PR must not add, drop, or repoint which secrets are forwarded to model providers`,
});
} else {
for (const occurrence of rootOccurrences) {
if (occurrence.value !== expected) {
findings.push({
file: occurrence.file,
problem: `tokenEnv "${occurrence.value}" != expected "${expected}" — a PR must not repoint which secret is forwarded to the model provider`,
});
}
}
}
}
+81 -7
View File
@@ -3,6 +3,7 @@ import { existsSync } from "node:fs";
import path from "node:path";
import { ReviewConfigSchema, ScopeReviewConfigSchema } from "./schema.js";
import type { AuthConfigEntry } from "./schema.js";
import type {
LoadedAgent,
LoadedConfig,
@@ -95,7 +96,14 @@ async function loadConfigDir(
>;
const parsed = schema.parse(rawObject) as ParsedConfig;
const override = process.env.REVIEWER_MODEL;
// An EMPTY REVIEWER_MODEL means "not set", not "use the empty model". GitHub Actions
// passes `${{ vars.REVIEWER_MODEL }}` as an empty string whenever that repo variable
// doesn't exist — which both scaffolded workflows do — so `??` (which only falls
// through on null/undefined) silently replaced every configured model with "". Every
// agent and the coordinator then ran on whatever OpenCode picked by default, so a
// config saying `anthropic/claude-sonnet-5` reviewed with something else entirely and
// nothing anywhere said so. Trim too: a stray newline is the same class of accident.
const override = process.env.REVIEWER_MODEL?.trim() || undefined;
const defaultModel = override ?? parsed.model;
const resolveModel = (frontmatterModel?: string): string =>
override ?? frontmatterModel ?? defaultModel;
@@ -159,16 +167,66 @@ async function loadConfigDir(
// Scope configs can't declare commentTag (scope schema rejects it);
// loadScopeConfig overwrites this placeholder with the manifest default.
commentTag: parsed.commentTag ?? "expo-ai-code-reviewer",
auth: {
mode: parsed.auth?.mode ?? "api-key",
provider: parsed.auth?.provider ?? "anthropic",
tokenEnv: parsed.auth?.tokenEnv,
},
auth: normalizeAuth(parsed.auth),
review: parsed.review,
};
return { config, raw: rawObject };
}
/**
* Normalize either accepted `auth` shape (legacy single object, or the
* per-provider `{ providers }` map) into the canonical entry list. Absent auth
* means the schema default (api-key/openai, no tokenEnv).
*/
export function normalizeAuth(
auth:
| { mode: "api-key" | "oauth"; provider: string; tokenEnv?: string }
| {
providers: Record<
string,
{ mode: "api-key" | "oauth"; tokenEnv?: string; upstream?: string }
>;
}
| undefined,
): AuthConfigEntry[] {
if (!auth) {
return [{ provider: "openai", mode: "api-key" }];
}
if ("providers" in auth) {
return Object.entries(auth.providers).map(([provider, entry]) => ({
provider,
mode: entry.mode,
tokenEnv: entry.tokenEnv,
upstream: entry.upstream,
}));
}
return [{ provider: auth.provider, mode: auth.mode, tokenEnv: auth.tokenEnv }];
}
/**
* Runtime auth lock: null when the entries' tokenEnv names equal the expected
* comma-separated set exactly (order-insensitive), else a human-readable
* mismatch. Set semantics because a multi-provider auth block names several
* credential envs — a PR must not be able to add, drop, or repoint any of them.
*/
export function tokenEnvMismatch(auth: AuthConfigEntry[], expected: string): string | null {
const declared = [
...new Set(auth.map((entry) => entry.tokenEnv).filter((v): v is string => Boolean(v))),
].sort();
const expectedSet = [
...new Set(
expected
.split(",")
.map((name) => name.trim())
.filter(Boolean),
),
].sort();
if (JSON.stringify(declared) === JSON.stringify(expectedSet)) {
return null;
}
return `configured tokenEnv set [${declared.join(", ") || "(none)"}] != ECR_EXPECTED_TOKEN_ENV [${expectedSet.join(", ")}]`;
}
/**
* auth is honored ONLY here: the manifest's `defaults.auth` wins when present,
* otherwise the root config.jsonc auth. A scope config can never contribute auth
@@ -181,7 +239,7 @@ export function loadAuthFromRoot(
): LoadedConfig["auth"] {
const override = manifest?.defaults.auth;
if (override) {
return { mode: override.mode, provider: override.provider, tokenEnv: override.tokenEnv };
return normalizeAuth(override);
}
return rootConfig.auth;
}
@@ -190,6 +248,22 @@ export interface LoadedScopeConfig extends LoadedConfig {
scopeName: string;
}
/**
* Whether a scope's own config dir exists under `root` — the same path
* `loadScopeConfig` reads (deliberately NOT via resolveConfigDir: ECR_CONFIG_DIR
* must never redirect scope subtrees). `ecr ci` uses this against the TRUSTED
* BASE root to give scopes that are new in a PR a defined miss behavior (review
* with the root config; the scope config activates after merge) instead of
* failing the run on exactly the PR that introduces the scope.
*/
export function hasScopeConfig(root: string, scope: RoutingScope): boolean {
if (scope.config === ".") {
return true;
}
const dir = path.join(root, scope.config, CONFIG_DIRNAME);
return existsSync(path.join(dir, "config.jsonc")) || existsSync(path.join(dir, "config.json"));
}
/**
* Load one scope's fully-resolved config. The default scope (config '.') reuses
* the root config unchanged except auth; a nested scope reads its own
+84 -25
View File
@@ -5,7 +5,7 @@ import { z } from "zod";
export const ReviewConfigSchema = z.object({
/** Default model for every agent + the coordinator. Override per-agent via
* frontmatter in the agent's markdown, or globally via REVIEWER_MODEL. */
model: z.string().default("anthropic/claude-sonnet-5"),
model: z.string().default("openai/gpt-5.5"),
policy: z
.object({
includeSuggestions: z.boolean().default(false),
@@ -40,10 +40,15 @@ export const ReviewConfigSchema = z.object({
maxChangedLines: z.number().int().positive().default(1000),
// Secondary guard so a chunk isn't an absurd number of tiny-diff files.
maxFiles: z.number().int().positive().default(20),
// Max concurrent reviewer calls across all agents/chunks.
concurrency: z.number().int().positive().default(6),
// Max concurrent reviewer calls across all agents/chunks. Unset ⇒ resolved
// from the auth mode: 6 for API-key runs, 3 when a subscription (oauth)
// credential is configured — one ChatGPT account handles six parallel
// streams poorly (requests get parked = the stall signature), and several
// PRs may be reviewing on the same credential at once. An explicit value
// here always wins. See effectiveConcurrency in core/review.ts.
concurrency: z.number().int().positive().optional(),
})
.default({ maxChangedLines: 1000, maxFiles: 20, concurrency: 6 }),
.default({ maxChangedLines: 1000, maxFiles: 20 }),
noise: z
.object({
additionalIgnores: z.array(z.string()).default([]),
@@ -54,17 +59,48 @@ export const ReviewConfigSchema = z.object({
.object({ marker: z.string().default("/skip-review") })
.default({ marker: "/skip-review" }),
commentTag: z.string().default("expo-ai-code-reviewer"),
// Two accepted shapes (see AuthConfigEntry for the canonical internal form):
// - legacy single credential: { mode, provider, tokenEnv }
// - per-provider map: { providers: { <id>: { mode, tokenEnv, upstream? } } }
// The map form allows a MIXED setup — e.g. the "openai" provider on a ChatGPT/Codex
// subscription (mode "oauth", tokenEnv = the refresh token) plus an "openai-api"
// alias (upstream "openai") holding a metered API key for pro-tier models the
// subscription doesn't offer.
//
// Union order matters: the map form must be tried FIRST — the legacy object's keys
// all have defaults, so a non-strict legacy parse would accept (and gut) a
// { providers } object by stripping the unknown key.
auth: z
.object({
// "api-key": the token env is sent as the provider's API key (x-api-key).
// "oauth": the token env is a Claude Pro/Max style OAuth token, injected
// into an isolated OpenCode auth.json so it's sent as a Bearer token.
mode: z.enum(["api-key", "oauth"]).default("api-key"),
provider: z.string().default("anthropic"),
/** Env var holding the key/token. */
tokenEnv: z.string().optional(),
})
.default({ mode: "api-key", provider: "anthropic" }),
.union([
z.object({
providers: z.record(
z.string(),
z.object({
// "api-key": tokenEnv holds the provider's API key.
// "oauth": tokenEnv holds an OAuth token, injected into an isolated
// OpenCode auth.json. For "openai" this is the REFRESH token from a
// ChatGPT/Codex sign-in (OpenCode's codex plugin mints access tokens
// from it). NOTE: anthropic oauth cannot work — OpenCode has no
// anthropic OAuth plugin and Anthropic prohibits subscription tokens
// in third-party tools.
mode: z.enum(["api-key", "oauth"]).default("api-key"),
tokenEnv: z.string().optional(),
// Set ⇒ this provider id is an ALIAS synthesized into the OpenCode
// config, backed by the named upstream's SDK ("openai", "anthropic",
// anything else = openai-compatible). Lets one upstream be reached
// with two credentials at once (subscription + API key).
upstream: z.string().optional(),
}),
),
}),
z.object({
mode: z.enum(["api-key", "oauth"]).default("api-key"),
provider: z.string().default("openai"),
/** Env var holding the key/token. */
tokenEnv: z.string().optional(),
}),
])
.default({ mode: "api-key", provider: "openai" }),
review: z
.object({
// Which PRs `ecr ci` acts on — the source of truth for trigger policy (a
@@ -112,21 +148,23 @@ export const RoutingManifestSchema = z
budget: z
.object({
/** Total passes budget (minutes) split across active scopes. Sized to fit
* the scaffolded workflow's `timeout-minutes` (60) with margin for the
* coordinator, verification, and git/gh overhead. */
totalPassesMinutes: z.number().int().positive().default(32),
* the scaffolded workflow's `timeout-minutes` (90) with margin for the
* coordinator (10m), verification, and git/gh overhead. The cross-file pass
* expands to fill whatever of this window is left (see review.ts), so this
* is the knob that decides how long it may trace. */
totalPassesMinutes: z.number().int().positive().default(55),
/** Per-scope floor (minutes): below this a scope review isn't worth
* starting, so the even split clamps up to it — even when that makes the
* scopes overshoot the total (ecr ci warns; doctor flags the worst case). */
minScopeMinutes: z.number().int().positive().default(5),
})
.default({ totalPassesMinutes: 32, minScopeMinutes: 5 }),
.default({ totalPassesMinutes: 55, minScopeMinutes: 5 }),
defaults: z
.object({
/** The ONLY manifest-level place auth is honored (locks the root value).
* Unwrap the inner `.default()` first: in zod v4 a `.default().optional()`
* chain still fires the default when the key is absent, which would make
* `defaults.auth` a phantom `{mode:'api-key',provider:'anthropic'}` for every
* `defaults.auth` a phantom `{mode:'api-key',provider:'openai'}` for every
* manifest that omits auth and silently override the root config's real auth. */
auth: ReviewConfigSchema.shape.auth.unwrap().optional(),
/** Agent ids injected into every scope with alwaysRun, from the ROOT roster. */
@@ -211,6 +249,29 @@ export interface LoadedAgent {
promptText: string;
}
/**
* One provider credential, canonical internal form (both config shapes normalize
* to a list of these — see normalizeAuth in load.ts).
*/
export interface AuthConfigEntry {
provider: string;
mode: "api-key" | "oauth";
/**
* Env var holding the credential. api-key: the key itself. oauth: the token —
* for provider "openai" this is the REFRESH token from a ChatGPT/Codex sign-in
* (access tokens live ~1h, shorter than a worst-case run, so the refresh token
* is the durable secret and OpenCode's codex plugin mints access tokens from it).
*/
tokenEnv?: string;
/**
* Set ⇒ this provider id is an alias synthesized into the OpenCode config,
* backed by the named upstream's SDK. Used to reach one upstream with a second
* credential (e.g. "openai-api" upstream "openai" for pro-tier models billed to
* an API key, while "openai" itself runs on the subscription).
*/
upstream?: string;
}
/** Fully-resolved config: prompt files read, models resolved, defaults applied. */
export interface LoadedConfig {
configDir: string;
@@ -228,7 +289,8 @@ export interface LoadedConfig {
chunk: {
maxChangedLines: number;
maxFiles: number;
concurrency: number;
/** Unset ⇒ resolved by effectiveConcurrency (auth-mode-aware default). */
concurrency?: number;
};
noise: {
additionalIgnores: string[];
@@ -236,11 +298,8 @@ export interface LoadedConfig {
};
breakGlassMarker: string;
commentTag: string;
auth: {
mode: "api-key" | "oauth";
provider: string;
tokenEnv?: string;
};
/** Every configured provider credential (one entry for the legacy single shape). */
auth: AuthConfigEntry[];
review: {
trigger: "all" | "label";
label: string;
+299 -56
View File
@@ -2,7 +2,7 @@ import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises";
import { tmpdir } from "node:os";
import path from "node:path";
import type { LoadedConfig } from "../config/schema.js";
import type { AuthConfigEntry, LoadedConfig } from "../config/schema.js";
export interface PreparedAuth {
cleanup: () => Promise<void>;
@@ -47,31 +47,143 @@ export interface AuthReadiness {
ok: boolean;
/** Human-readable detail for `doctor` output and fail-fast error messages. */
detail: string;
/**
* A credential that is suspicious but not provably broken. Reported by `doctor`,
* never a reason to refuse a run — the shape rules below are heuristics, and a
* heuristic must not block a setup that would have worked.
*/
warning?: string;
}
/**
* Decide whether the configured model provider has a usable credential, WITHOUT
* mutating the environment. Shared by `prepareAuth` (fail fast before spinning up
* the server and every pass) and `doctor` (report), so the two never drift.
*
* We only report `ok: false` when we're confident there is no credential — a
* missing OAuth token, a forbidden tokenEnv, or an api-key run with neither the
* configured tokenEnv nor the provider's own key env set. When nothing is
* configured and no known key env is present, we assume OpenCode's own login may
* cover it and don't hard-fail. `REVIEWER_MODEL` bypasses provider auth entirely.
* Documented Anthropic OAuth token shape: `sk-ant-oat01-` + 95 chars = 108 total
* (what `claude setup-token` prints). Treated as a STRONG HINT, not a spec: it comes
* from observed tokens and third-party sources, not an Anthropic format guarantee, so
* an unrecognized shape only warns. The one exception is below — a value that becomes
* exactly this shape by restoring a dropped `sk-` is provably a mangled paste.
*/
export function checkProviderAuth(
config: LoadedConfig,
env: NodeJS.ProcessEnv = process.env,
): AuthReadiness {
const { mode, provider, tokenEnv } = config.auth;
const ANTHROPIC_OAUTH_PREFIX = "sk-ant-oat";
/** Anthropic API keys — valid credentials, but for auth.mode "api-key", not "oauth". */
const ANTHROPIC_API_KEY_PREFIX = "sk-ant-api";
/** Shorter than any credential of any provider ⇒ truncated, whatever the format. */
const MIN_TOKEN_LENGTH = 40;
if (env.REVIEWER_MODEL) {
/**
* Catch an OAuth token that CANNOT work before it costs a whole run.
*
* Motivation: OpenCode refuses a malformed credential by dropping the provider from
* its provider list entirely, so every configured model then reports "model not
* found" and nothing anywhere mentions credentials. That misdirection cost a full
* debugging session for a value that had simply lost its leading `sk-`.
*
* The bar for `ok: false` is "this cannot be a valid credential", not "this looks
* odd", because a false rejection blocks a working setup — a worse failure than the
* one being prevented. Provable: whitespace, absurdly short, an API key in oauth
* mode, and an OAuth token missing its `sk-`. Everything else is at most a warning,
* and formats of providers we don't know are never judged at all.
*/
export function checkOauthTokenShape(
provider: string,
token: string,
tokenEnv: string,
): AuthReadiness {
const ok = { ok: true, detail: `oauth for ${provider}; token env ${tokenEnv} is set` };
const fix = `Re-generate it with \`claude setup-token\` and set ${tokenEnv} to the full value.`;
if (token !== token.trim()) {
return {
ok: true,
detail: `REVIEWER_MODEL override (${env.REVIEWER_MODEL}); using OpenCode's own login for that model`,
ok: false,
detail:
`${tokenEnv} has leading/trailing whitespace (a newline from a copy-paste or a ` +
`\`cat\`-ed file is the usual cause); the provider will refuse it. ${fix}`,
};
}
if (token.length < MIN_TOKEN_LENGTH) {
return {
ok: false,
detail: `${tokenEnv} holds only ${token.length} characters, too short to be a real ${provider} token — it looks truncated. ${fix}`,
};
}
// A ChatGPT access token carries its own expiry — check it up front, so a
// lapsed credential is one clear message instead of N failed passes, and a
// nearly-lapsed one warns before it bites mid-run.
if (provider === "openai" && isJwtAccessToken(token)) {
const expires = jwtExpiryMs(token);
if (expires !== null) {
const remainingMs = expires - Date.now();
if (remainingMs <= 0) {
return {
ok: false,
detail:
`${tokenEnv} holds a ChatGPT access token that EXPIRED ${Math.ceil(-remainingMs / 86_400_000)} day(s) ago. ` +
`Mint a fresh one (\`ecr setup-auth\`, or your token-rotator job) and update ${tokenEnv}.`,
};
}
if (remainingMs < 3 * 86_400_000) {
return {
...ok,
detail: `oauth for ${provider}; token env ${tokenEnv} is set`,
warning:
`${tokenEnv}'s ChatGPT access token expires in ${Math.max(1, Math.round(remainingMs / 3_600_000))}h — ` +
`re-mint it soon (\`ecr setup-auth\`, or your token-rotator job).`,
};
}
}
return { ...ok, detail: `oauth for ${provider}; token env ${tokenEnv} is set` };
}
// Only anthropic's formats are known well enough to say anything about.
if (provider !== "anthropic") {
return ok;
}
if (token.startsWith(ANTHROPIC_API_KEY_PREFIX)) {
return {
ok: false,
detail:
`${tokenEnv} holds an Anthropic API key ("${ANTHROPIC_API_KEY_PREFIX}…"), but auth.mode is ` +
`"oauth", which expects the OAuth token from \`claude setup-token\` ` +
`("${ANTHROPIC_OAUTH_PREFIX}…"). Either set auth.mode to "api-key" in ` +
`.expo-code-review/config.jsonc, or put an OAuth token in ${tokenEnv}.`,
};
}
// Provable mangled paste: prepending the dropped "sk-" yields exactly the documented
// OAuth shape. Only this reconstruction earns a hard failure — the value is not a
// credential of any known kind as it stands, but is one character-for-character with
// its prefix restored.
if (!token.startsWith("sk-") && `sk-${token}`.startsWith(ANTHROPIC_OAUTH_PREFIX)) {
return {
ok: false,
detail:
`${tokenEnv} starts with "${token.slice(0, 10)}…", which is an Anthropic OAuth token ` +
`missing its leading "sk-" — the prefix was dropped when the value was copied ` +
`(adding it back gives "${ANTHROPIC_OAUTH_PREFIX}…", the shape \`claude setup-token\` ` +
`prints). OpenCode refuses a malformed credential by dropping the provider entirely, ` +
`which then surfaces as "model not found" for every model. ${fix}`,
};
}
if (!token.startsWith(ANTHROPIC_OAUTH_PREFIX)) {
// Unrecognized, but we can't prove it's wrong: Anthropic can mint shapes we don't
// know, so advise and continue rather than refusing to run.
return {
...ok,
warning:
`${tokenEnv} does not start with "${ANTHROPIC_OAUTH_PREFIX}…" (the shape ` +
`\`claude setup-token\` prints for auth.mode "oauth"). It may still be valid — but if ` +
`the run fails with "model not found" for every model, the credential was refused, ` +
`and this is the first thing to check.`,
};
}
return ok;
}
/**
* Decide whether ONE configured credential is usable, WITHOUT mutating the
* environment. See checkProviderAuth for the all-entries wrapper.
*/
export function checkAuthEntry(
entry: AuthConfigEntry,
env: NodeJS.ProcessEnv = process.env,
): AuthReadiness {
const { mode, provider, tokenEnv, upstream } = entry;
if (tokenEnv && FORBIDDEN_TOKEN_ENVS.has(tokenEnv)) {
return {
@@ -83,12 +195,30 @@ export function checkProviderAuth(
};
}
// A well-known provider key env may only feed THAT provider. The tokenEnv guard
// locks which env names are forwarded, but not where they go — without this, a
// PR-supplied auth entry could keep the locked name (e.g. OPENAI_API_KEY) and
// point its provider/upstream somewhere else, sending one provider's key to a
// different provider.
if (tokenEnv) {
const keyOwner = Object.entries(PROVIDER_KEY_ENV).find(([, env]) => env === tokenEnv)?.[0];
if (keyOwner && keyOwner !== provider && keyOwner !== upstream) {
return {
ok: false,
detail:
`auth for ${provider} names tokenEnv "${tokenEnv}", which is ${keyOwner}'s ` +
`well-known key env — refusing to send one provider's credential to another. ` +
`Use a credential minted for ${provider}${upstream ? ` (upstream ${upstream})` : ""}, ` +
`or fix the provider/upstream mapping.`,
};
}
}
if (mode === "oauth") {
if (!tokenEnv) {
return {
ok: false,
detail:
'auth.mode "oauth" requires auth.tokenEnv to name the env var holding the OAuth token.',
detail: `auth mode "oauth" for ${provider} requires tokenEnv to name the env var holding the OAuth token.`,
};
}
if (!env[tokenEnv]) {
@@ -97,7 +227,33 @@ export function checkProviderAuth(
detail: `auth is oauth for ${provider} but token env "${tokenEnv}" is not set.`,
};
}
return { ok: true, detail: `oauth for ${provider}; token env ${tokenEnv} is set` };
const shape = checkOauthTokenShape(provider, env[tokenEnv]!, tokenEnv);
if (!shape.ok) {
return shape;
}
return { ...shape, detail: `oauth for ${provider}; token env ${tokenEnv} is set` };
}
// api-key with an upstream alias: the synthesized provider reads the key
// straight from {env:tokenEnv}, so both must exist — there is no ambient
// fallback for a provider id we invented.
if (upstream) {
if (!tokenEnv) {
return {
ok: false,
detail: `auth for ${provider} (upstream ${upstream}) requires tokenEnv — the synthesized provider reads the key from that env var.`,
};
}
if (!env[tokenEnv]) {
return {
ok: false,
detail: `auth for ${provider} (upstream ${upstream}) names token env "${tokenEnv}" but it is not set.`,
};
}
return {
ok: true,
detail: `api-key for ${provider} (upstream ${upstream}); token env ${tokenEnv} is set`,
};
}
// api-key: usable if the configured tokenEnv is set, or the provider's own key
@@ -125,29 +281,118 @@ export function checkProviderAuth(
}
/**
* Prepare model credentials for the OpenCode server based on the repo's auth mode.
* Must run before the server starts (it mutates env). Returns a cleanup handle.
* Decide whether EVERY configured provider credential is usable, WITHOUT
* mutating the environment. Shared by `prepareAuth` (fail fast before spinning up
* the server and every pass) and `doctor` (report), so the two never drift.
*
* - `api-key`: copy the configured token env into the provider's API-key env var
* (so the workflow can pass a namespaced secret and OpenCode still finds it).
* - `oauth`: write an isolated OpenCode `auth.json` with the token as a Bearer
* OAuth credential and point OpenCode at it via XDG_DATA_HOME, so it uses its
* native Claude Pro/Max path (correct bearer + oauth headers) rather than
* x-api-key. Isolated so it never touches the developer's real auth.json.
* All entries must pass — a mixed setup with one broken credential would fail
* exactly the passes routed to it, which is the silent-degradation this check
* exists to prevent. `REVIEWER_MODEL` bypasses provider auth entirely.
*/
export function checkProviderAuth(
config: LoadedConfig,
env: NodeJS.ProcessEnv = process.env,
): AuthReadiness {
if (env.REVIEWER_MODEL) {
return {
ok: true,
detail: `REVIEWER_MODEL override (${env.REVIEWER_MODEL}); using OpenCode's own login for that model`,
};
}
const details: string[] = [];
const warnings: string[] = [];
for (const entry of config.auth) {
const readiness = checkAuthEntry(entry, env);
if (!readiness.ok) {
return readiness;
}
details.push(readiness.detail);
if (readiness.warning) {
warnings.push(readiness.warning);
}
}
return {
ok: true,
detail: details.join("; "),
...(warnings.length > 0 ? { warning: warnings.join("; ") } : {}),
};
}
/** A ChatGPT access token is a JWT (three base64url segments); refresh tokens are opaque. */
export function isJwtAccessToken(token: string): boolean {
return token.startsWith("eyJ") && token.split(".").length === 3;
}
/** A JWT's `exp` claim as epoch ms, decoded (not verified) — null when unreadable. */
export function jwtExpiryMs(token: string): number | null {
const payload = token.split(".")[1];
if (!payload) {
return null;
}
try {
const claims = JSON.parse(Buffer.from(payload, "base64url").toString("utf8")) as {
exp?: unknown;
};
return typeof claims.exp === "number" ? claims.exp * 1000 : null;
} catch {
return null;
}
}
/**
* The auth.json entry for one oauth credential. Shaped by what the token IS:
* - a JWT (an ACCESS token, e.g. from `ecr setup-auth` or a rotator job): use it
* as-is and never refresh — refresh tokens are SINGLE-USE (rotation), so a
* static/shared secret must not participate in rotation at all. Expiry comes
* from the JWT's own `exp` claim so OpenCode trusts it exactly as long as it
* is valid.
* - an opaque openai token (a REFRESH token): store it with `expires: 0` and let
* OpenCode's codex plugin mint the access token. Only safe when this run is
* the token's SOLE consumer — a value shared across runs/repos dies on first
* rotation (learned the hard way).
* - everything else: the token IS the access credential (e.g. long-lived
* setup-token style bearers), far-future expiry so OpenCode never tries to
* refresh a credential that has no refresh half.
*/
export function oauthAuthJsonEntry(
provider: string,
token: string,
): Record<string, string | number> {
if (provider === "openai" && !isJwtAccessToken(token)) {
return { type: "oauth", access: "", refresh: token, expires: 0 };
}
const expires = isJwtAccessToken(token) ? jwtExpiryMs(token) : null;
return { type: "oauth", access: token, refresh: "", expires: expires ?? Date.now() + YEAR_MS };
}
/**
* Prepare model credentials for the OpenCode server from the repo's auth entries
* (any mix of modes/providers). Must run before the server starts (it mutates
* env). Returns a cleanup handle.
*
* - `api-key` (standard provider): copy the configured token env into the
* provider's API-key env var (so the workflow can pass a namespaced secret and
* OpenCode still finds it).
* - `api-key` (upstream alias): nothing to do here — buildOpencodeConfig
* synthesizes a provider block whose options read `{env:tokenEnv}` directly.
* - `oauth`: write ALL oauth credentials into one isolated OpenCode `auth.json`
* and point OpenCode at it via XDG_DATA_HOME (see oauthAuthJsonEntry for the
* per-provider shapes). Isolated so it never touches the developer's real
* auth.json.
*/
export async function prepareAuth(config: LoadedConfig): Promise<PreparedAuth> {
const noop: PreparedAuth = { cleanup: async () => {} };
const { mode, provider, tokenEnv } = config.auth;
// REVIEWER_MODEL is an explicit "use this model with my own creds" override — a
// common local case (e.g. the repo config targets Claude OAuth in CI, but a dev
// runs against their own OpenAI login). Don't inject the configured provider's
// common local case (e.g. the repo config targets a CI credential, but a dev
// runs against their own OpenCode login). Don't inject the configured providers'
// auth; let OpenCode use whatever it's logged into for the override model.
if (process.env.REVIEWER_MODEL) {
return noop;
}
// Fail fast, before starting the server and every pass, if the configured
// Fail fast, before starting the server and every pass, if ANY configured
// provider has no usable credential — otherwise it surfaces as N failed passes
// mid-run. This is the same readiness check `doctor` reports, and it also covers
// the forbidden-secret guard (refusing to forward a well-known unrelated secret).
@@ -156,38 +401,36 @@ export async function prepareAuth(config: LoadedConfig): Promise<PreparedAuth> {
throw new Error(readiness.detail);
}
if (mode === "api-key") {
if (tokenEnv) {
const value = process.env[tokenEnv];
const target = PROVIDER_KEY_ENV[provider] ?? "ANTHROPIC_API_KEY";
// The explicitly-configured tokenEnv is authoritative — set it even if the
// provider env is already present, so config wins over ambient env.
if (value) {
const authJson: Record<string, unknown> = {};
for (const entry of config.auth) {
const { mode, provider, tokenEnv, upstream } = entry;
const value = tokenEnv ? process.env[tokenEnv] : undefined;
if (mode === "api-key") {
// Upstream aliases read {env:tokenEnv} from the synthesized provider block.
if (!upstream && tokenEnv && value) {
const target = PROVIDER_KEY_ENV[provider] ?? "ANTHROPIC_API_KEY";
// The explicitly-configured tokenEnv is authoritative — set it even if the
// provider env is already present, so config wins over ambient env.
process.env[target] = value;
}
continue;
}
return noop;
// oauth — checkProviderAuth guarantees tokenEnv is set and present; read
// defensively so TypeScript narrows and this stays correct if called directly.
if (!value) {
throw new Error(
`auth mode "oauth" for ${provider} requires tokenEnv to name a set OAuth token env.`,
);
}
authJson[provider] = oauthAuthJsonEntry(provider, value);
}
// oauth — checkProviderAuth guarantees tokenEnv is set and present; read
// defensively so TypeScript narrows and this stays correct if called directly.
const token = tokenEnv ? process.env[tokenEnv] : undefined;
if (!token) {
throw new Error('auth.mode "oauth" requires auth.tokenEnv to name a set OAuth token env.');
if (Object.keys(authJson).length === 0) {
return noop;
}
const dir = await mkdtemp(path.join(tmpdir(), "ecr-auth-"));
await mkdir(path.join(dir, "opencode"), { recursive: true });
const authJson = {
[provider]: {
type: "oauth",
access: token,
refresh: "",
// Far-future expiry so OpenCode uses the token as-is and does not try to
// refresh it (setup-token tokens are long-lived and carry no refresh).
expires: Date.now() + YEAR_MS,
},
};
await writeFile(path.join(dir, "opencode", "auth.json"), JSON.stringify(authJson), "utf8");
process.env.XDG_DATA_HOME = dir;
+4 -2
View File
@@ -13,6 +13,8 @@ export interface CoordinationResult {
// via the finalize path — so the caller can flag reduced coverage rather than
// silently presenting a truncated consolidation as complete.
truncated: boolean;
/** provider/model that actually answered (see PromptResult.model). */
model?: string;
}
/**
@@ -33,7 +35,7 @@ export async function coordinate(
): Promise<CoordinationResult> {
const system = buildCoordinatorSystem(config);
const text = buildCoordinatorTask(metadata, agentFindings, coverageNotes);
const { value, cost, tokens, truncated } = await promptAndParse(
const { value, cost, tokens, truncated, model } = await promptAndParse(
handle,
{
agent: "coordinator",
@@ -45,5 +47,5 @@ export async function coordinate(
},
parseCoordinatorOutput,
);
return { output: value, cost, tokens, truncated };
return { output: value, cost, tokens, truncated, model };
}
+4
View File
@@ -26,6 +26,10 @@ export interface RunLogRecord {
// "coordinator", "verifier") so cache effectiveness can be judged per pass, not
// just run-wide.
agentTokens?: Record<string, TokenUsage>;
/** provider/model that ACTUALLY answered each pass (not what was configured). */
agentModels?: Record<string, string>;
/** Provider rate-limit (429) events observed in the OpenCode server log this run. */
rateLimitEvents?: number;
// The reasoning trail behind the posted result: raw per-agent findings before
// coordination, the coverage gaps reported to the coordinator, and the findings
// the verifier rejected. Together these explain WHY the final finding set looks
+618 -66
View File
@@ -1,6 +1,10 @@
import { createRequire } from "node:module";
import path from "node:path";
import { createOpencode } from "@opencode-ai/sdk";
import type { LoadedConfig } from "../config/schema.js";
import { RateLimitWatch } from "./throttle.js";
import { toolMap } from "./tools.js";
import { errorMessage, sleep } from "./util.js";
@@ -8,6 +12,8 @@ export interface OpencodeHandle {
client: any;
url: string;
close: () => void;
/** Rate-limit evidence from this server's own log (see core/throttle.ts). */
rateLimit: RateLimitWatch;
}
/** Token usage as reported on an OpenCode assistant message's `info.tokens`. */
@@ -27,6 +33,15 @@ export interface PromptResult {
truncated?: boolean;
/** Token usage for the model request that produced this reply (for cache metrics). */
tokens?: TokenUsage;
/**
* The provider/model that ACTUALLY answered, as reported by the server — not what
* was configured. OpenCode silently substitutes a default when an agent's model id
* is empty or unusable, so without this a run can review with a completely different
* model than the config names and nothing says so. (It did: an empty REVIEWER_MODEL
* wiped every configured id and reviews ran on whatever provider happened to be
* available.) Undefined when the server didn't report it.
*/
model?: string;
}
/** Sum token usage across attempts (for per-task/run totals). */
@@ -46,6 +61,11 @@ export function addTokenUsage(into: TokenUsage, from?: TokenUsage): TokenUsage {
// The coordinator consolidates findings; it needs no repo tools.
const COORDINATOR_TOOLS = toolMap([]);
// Every tool disabled, passed per-REQUEST (not per-agent) to make a "reply now,
// don't investigate" prompt physically unable to call tools. A prompt-level plea is
// not enough: in eas-cli#4084 the finalize ("stop and return what you have") reply
// opened 7 more files and then blew its window, losing the whole pass.
const NO_TOOLS = toolMap([]);
// Agent id for the single combined cross-cutting pass (see review.ts). It MUST be
// defined here so OpenCode uses this restricted tool set — otherwise the model
// falls back to a default agent with full tools and crawls the whole repo, which
@@ -103,7 +123,48 @@ export function buildOpencodeConfig(config: LoadedConfig): Record<string, unknow
"You are the review coordinator. Follow the user message exactly and return only the requested JSON.",
tools: COORDINATOR_TOOLS,
};
return { $schema: "https://opencode.ai/config.json", agent };
// Synthesize a provider block for each upstream-alias auth entry, so one
// upstream can be reached with two credentials at once (e.g. "openai" on a
// ChatGPT/Codex subscription for the default models, plus an "openai-api"
// alias holding a metered API key for pro-tier models the subscription
// doesn't offer). The alias's model list is exactly the ids the roster
// references under that provider — OpenCode needs custom providers' models
// declared, and declaring only what's used keeps the preflight meaningful.
const provider: Record<string, unknown> = {};
const referencedModels = [
...config.agents.map((reviewer) => reviewer.model),
config.coordinator.model,
];
for (const entry of config.auth) {
if (!entry.upstream || !entry.tokenEnv) {
continue;
}
const models: Record<string, unknown> = {};
for (const id of referencedModels) {
const slash = id.indexOf("/");
if (slash > 0 && id.slice(0, slash) === entry.provider) {
models[id.slice(slash + 1)] = {};
}
}
provider[entry.provider] = {
npm:
entry.upstream === "openai"
? "@ai-sdk/openai"
: entry.upstream === "anthropic"
? "@ai-sdk/anthropic"
: "@ai-sdk/openai-compatible",
name: entry.provider,
options: { apiKey: `{env:${entry.tokenEnv}}` },
models,
};
}
return {
$schema: "https://opencode.ai/config.json",
agent,
...(Object.keys(provider).length > 0 ? { provider } : {}),
};
}
/** hey-api style responses come back as { data, error }; unwrap or throw. */
@@ -117,13 +178,239 @@ function unwrap<T>(res: any): T {
return res as T;
}
/**
* Directory holding the `opencode` binary from OUR dependency tree, or null if it
* can't be resolved.
*
* The SDK spawns the server with a bare `launch("opencode", …)`, i.e. whatever comes
* first on PATH. That silently couples every run to the machine's global install:
* in CI, `npx -p @expo/code-review-cli` puts the temp prefix's `.bin` first and the
* pinned version wins, but on a developer machine an older global `opencode` shadows
* it and the pair drifts (a 1.18.1 CLI against a 1.18.4 SDK surfaced as
* `ProviderModelNotFoundError: Model not found: anthropic/claude-opus-4-8` — the CLI
* resolving a model id the SDK considered valid). Prepending this directory to PATH
* makes the version we declare in package.json the version we actually run.
*/
function bundledOpencodeBinDir(): string | null {
try {
const require = createRequire(import.meta.url);
// <…>/node_modules/opencode-ai/package.json → <…>/node_modules/.bin, which holds
// the correctly-named `opencode` shim (the package's own bin file is
// `opencode.exe`, so the package dir itself is NOT a usable PATH entry).
const pkg = require.resolve("opencode-ai/package.json");
return path.join(path.dirname(pkg), "..", ".bin");
} catch {
return null;
}
}
/** The `opencode` the SDK will actually spawn: ours if resolvable, else PATH's. */
export function opencodeBinSource(): { dir: string | null; pinned: boolean } {
const dir = bundledOpencodeBinDir();
return { dir, pinned: dir !== null };
}
/** Start an in-process OpenCode server with the given inline config. */
export async function startOpencode(config: unknown): Promise<OpencodeHandle> {
// Make our pinned CLI win over any global install (see bundledOpencodeBinDir).
// The SDK takes no `env`, so PATH is the only lever; it spreads `process.env` at
// spawn time, so setting it here reaches the child.
const binDir = bundledOpencodeBinDir();
if (binDir) {
process.env.PATH = `${binDir}${path.delimiter}${process.env.PATH ?? ""}`;
}
const { client, server } = await createOpencode({
hostname: "127.0.0.1",
// Port 0 = let the OS pick a free one. The SDK defaults to a FIXED 4096 and reads
// the real URL back from the server's startup line, so the default meant any
// already-running opencode (a developer's own session is the common case) made
// every local run die with an opaque `ServeError`.
port: 0,
config: config as any,
});
return { client, url: server.url, close: () => server.close() };
// Watch THIS server's log for provider throttle evidence (prepareAuth has already
// pointed XDG_DATA_HOME at the run's isolated dir when auth is injected).
return { client, url: server.url, close: () => server.close(), rateLimit: new RateLimitWatch() };
}
/**
* A configured `provider/model` string that the running server can't resolve, with
* the closest ids it does know — the whole point is that the error names the fix.
*/
export interface UnknownModel {
model: string;
/**
* `credential` — the provider is the one we supplied a credential for, and the
* server still doesn't offer it: the credential was rejected, which is a very
* different fix from a typo'd id (and the one that actually bit us: a bad
* ANTHROPIC_OAUTH_API_KEY made every anthropic model look nonexistent).
* `provider` — no such provider at all. `model` — provider fine, model isn't.
*/
reason: "credential" | "provider" | "model";
suggestions: string[];
}
/** `provider/model` as the server reported it, or undefined if it reported neither. */
export function formatModel(providerID?: string, modelID?: string): string | undefined {
if (!providerID && !modelID) {
return undefined;
}
return `${providerID ?? "?"}/${modelID ?? "?"}`;
}
/** Providers the server actually has, as `{ providerID: [modelID, …] }`. */
export type ProviderModels = Record<string, string[]>;
/**
* Check configured model ids against what the server can resolve. Pure so the
* matching rules are testable without a live server.
*
* A wrong model id is a CONFIG error: it hits every pass identically, no retry or
* smaller scope can fix it, and the run should say so once instead of reporting N
* indistinguishable "pass failed" gaps (or, worse, burning the whole budget first).
*/
export function findUnknownModels(
models: string[],
available: ProviderModels,
/** Provider(s) we supplied a credential for, if any (see reason: "credential"). */
credentialedProvider?: string | string[],
): UnknownModel[] {
const credentialed = new Set(
typeof credentialedProvider === "string"
? [credentialedProvider]
: (credentialedProvider ?? []),
);
const unknown: UnknownModel[] = [];
for (const model of new Set(models)) {
// OpenCode model ids are `provider/model`; a model id may itself contain slashes
// (e.g. openrouter's `vendor/name`), so only the FIRST segment is the provider.
const slash = model.indexOf("/");
const providerID = slash === -1 ? model : model.slice(0, slash);
const modelID = slash === -1 ? "" : model.slice(slash + 1);
const providerModels = available[providerID];
if (!providerModels) {
unknown.push({
model,
// We configured this provider's credential and the server still doesn't
// offer it ⇒ the credential was refused, not the provider misnamed.
reason: credentialed.has(providerID) ? "credential" : "provider",
suggestions: Object.keys(available).sort(),
});
continue;
}
if (!providerModels.includes(modelID)) {
// Suggest ids that share a prefix with what was asked for, else the whole list.
const stem = modelID.split(/[-/]/)[0] ?? "";
const near = stem ? providerModels.filter((id) => id.startsWith(stem)) : [];
unknown.push({
model,
reason: "model",
suggestions: (near.length > 0 ? near : providerModels).slice(0, 8).sort(),
});
}
}
return unknown;
}
/** Ask the running server which providers/models it can resolve. */
export async function fetchProviderModels(handle: OpencodeHandle): Promise<ProviderModels> {
const data = unwrap<{ providers?: Array<{ id?: string; models?: Record<string, unknown> }> }>(
await handle.client.config.providers(),
);
const available: ProviderModels = {};
for (const provider of data?.providers ?? []) {
if (provider?.id) {
available[provider.id] = Object.keys(provider.models ?? {});
}
}
return available;
}
/** Human-readable, copy-pasteable explanation of unresolvable model ids. */
export function formatUnknownModels(
unknown: UnknownModel[],
auths?:
| { mode: string; provider: string; tokenEnv?: string }
| Array<{ mode: string; provider: string; tokenEnv?: string }>,
): string {
// A refused credential is one fact about the run, not one per model: report it once
// and name the token to check, rather than repeating it for every configured model.
const refused = unknown.filter((entry) => entry.reason === "credential");
if (refused.length > 0) {
const refusedProvider = refused[0]!.model.split("/")[0];
const entries = auths ? (Array.isArray(auths) ? auths : [auths]) : [];
const auth = entries.find((entry) => entry.provider === refusedProvider) ?? entries[0];
const provider = auth?.provider ?? refusedProvider;
const tokenEnv = auth?.tokenEnv;
// Do NOT blame the token alone: the most common causes have nothing to do with
// the credential's validity (see below). An earlier version of this message sent
// us to re-issue two perfectly good tokens.
const oauth = auth?.mode === "oauth";
const deadOauth = oauth && provider === "anthropic";
return (
`The OpenCode server does not offer the "${provider}" provider, even though this run ` +
`supplied a ${auth?.mode ?? "configured"} credential for it. OpenCode drops a provider whose ` +
`credential it could not use, which makes every ${provider} model look nonexistent: ` +
`${refused.map((entry) => entry.model).join(", ")}.\n` +
`The credential itself is often FINE. Check these in order:\n` +
(deadOauth
? ` 1. anthropic OAuth cannot work through OpenCode at all. Anthropic does not permit ` +
`Pro/Max subscription tokens in third-party tools, and OpenCode (since 1.3.0) ships no ` +
`anthropic OAuth support — an oauth credential never registers the provider, no matter ` +
`how valid the token is. Switch auth in .expo-code-review/config.jsonc to ` +
`{ "mode": "api-key", "provider": "anthropic", "tokenEnv": "ANTHROPIC_API_KEY" } with a ` +
`Console API key, or run with REVIEWER_MODEL set to a model you are logged into ` +
`(e.g. REVIEWER_MODEL=openai/gpt-5.5).\n`
: "") +
(tokenEnv
? ` ${deadOauth ? "2" : "1"}. The credential is wrong for the mode. ` +
`auth.mode "api-key" expects a plain API key for ${provider}; an OAuth/subscription ` +
`token is not an API key. A truncated or half-pasted ${tokenEnv} fails the same way.\n`
: ` ${deadOauth ? "2" : "1"}. The credential is wrong for the configured auth.mode.\n`) +
`Providers the server does offer: ${refused[0]!.suggestions.join(", ") || "(none)"}.`
);
}
const lines = unknown.map((entry) =>
entry.reason === "provider"
? ` ${entry.model} — unknown provider "${entry.model.split("/")[0]}". Configured providers: ${entry.suggestions.join(", ") || "(none — is the credential set?)"}`
: ` ${entry.model} — that provider has no such model. Close matches: ${entry.suggestions.join(", ") || "(none)"}`,
);
return (
`The configured model id(s) do not exist on the running OpenCode server:\n${lines.join("\n")}\n` +
`Fix the model in .expo-code-review/config.jsonc (agents' \`model\`, \`coordinator.model\`) ` +
`or REVIEWER_MODEL. Note that a model id must be "provider/model" (e.g. anthropic/claude-sonnet-5), ` +
`and that an out-of-date \`opencode\` can reject an id a newer one accepts — run \`ecr doctor\`.`
);
}
/**
* Fail fast when a configured model can't be resolved, BEFORE any pass runs. Never
* blocks the run on its own failure: if the providers endpoint can't be read (an
* older server, a transport blip), the review proceeds and any real problem surfaces
* per-pass as before.
*/
export async function assertModelsResolvable(
handle: OpencodeHandle,
models: string[],
auths?: Array<{ mode: string; provider: string; tokenEnv?: string }>,
): Promise<void> {
let available: ProviderModels;
try {
available = await fetchProviderModels(handle);
} catch {
return;
}
if (Object.keys(available).length === 0) {
return;
}
const unknown = findUnknownModels(
models,
available,
auths?.map((entry) => entry.provider),
);
if (unknown.length > 0) {
throw new Error(formatUnknownModels(unknown, auths));
}
}
const POLL_INTERVAL_MS = 1000;
@@ -137,7 +424,72 @@ const HEARTBEAT_MS = 45_000;
// over. Callers must treat AgentTimeoutError as "abandon", never "retry".
const DEFAULT_MAX_WAIT_MS = 8 * 60 * 1000;
// Extra budget for the "stop and summarize what you have" finalization prompt.
const FINALIZE_WAIT_MS = 90 * 1000;
// Deliberately generous: this is the ONLY chance to salvage a pass that ran out of
// time, and at 90s it was losing that race. Tools are disabled for the request
// (NO_TOOLS), so the reply is a single emit and normally lands in seconds.
const FINALIZE_WAIT_MS = 3 * 60 * 1000;
// ---- stall detection ----
//
// A pass is STALLED when its in-progress assistant message stops changing at all:
// no new tool call, no streamed text or reasoning, no token growth. A model that is
// genuinely thinking still grows that message every few seconds, so a gap this long
// means the provider request is wedged (or stuck in an internal retry we cannot
// see) — not that the work is hard.
//
// Motivating incident (eas-cli#4084, 2026-07-26): the cross-file pass ran 7 `read`
// calls in its first 6 seconds and then sat completely silent for 25 minutes,
// recording ZERO tokens (input, output, reasoning and cache all 0 in the run log),
// until its wall-clock cap fired. The finalize salvage then also went silent, so the
// pass's entire work product was lost and the PR got a coverage gap. Nothing in the
// run distinguished "wedged" from "thinking" — the heartbeat just printed elapsed
// seconds. A wall-clock cap alone cannot fix this: raising it only buys a longer
// silence, which is why the cap is NOT the lever here.
//
// Unlike the wall-clock cap (non-convergence → abandon, never retry), a stall is
// transient, so it earns ONE clean-slate retry inside the pass's existing budget.
const STALL_MS = 4 * 60 * 1000;
// Never let the watchdog outlast the pass it guards: a short pass (the 3m verifier,
// a 4m no-tools fallback) would otherwise hit its deadline before the watchdog could
// fire, and get none of this protection. Half the cap, with a floor that leaves room
// for a slow first token.
const MIN_STALL_MS = 30 * 1000;
/** Exported for tests. */
export function stallWindowMs(maxWaitMs: number): number {
return Math.min(STALL_MS, Math.max(MIN_STALL_MS, Math.floor(maxWaitMs / 2)));
}
// The finalize reply does no investigation and cannot call tools, so it should
// stream within seconds; a much shorter silence already means wedged.
const FINALIZE_STALL_MS = 60 * 1000;
// Breathing room before the retry: if the silence came from provider-side throttling
// or backoff, reconnecting instantly is the worst move.
const STALL_RETRY_BACKOFF_MS = 20 * 1000;
// When the account is provably rate-limited, wait in longer beats: re-sending the
// pass's whole context into a throttled account only deepens the limit. Several
// waits fit inside a pass budget, and each is long enough for a limit window to move.
const RATE_LIMIT_WAIT_MS = 90 * 1000;
// Only retry when enough of the pass's budget remains for the fresh attempt to
// plausibly finish; otherwise go straight to the soft landing.
const STALL_RETRY_MIN_REMAINING_MS = STALL_MS + 60 * 1000;
/**
* What to do about a stalled attempt: WAIT (the account is provably rate-limited —
* see core/throttle.ts — so patience beats re-sending the context; waits don't
* consume the one retry), start over from a clean session, or stop and salvage
* findings. Exactly ONE wedged retry, and only with enough budget left for it to
* land — a second wedged attempt would just spend the rest of the pass's window,
* which is the failure this whole mechanism exists to end. Exported for tests.
*/
export function stallAction(
wedgedRetries: number,
remainingMs: number,
rateLimited = false,
): "wait" | "retry" | "soft-land" {
if (rateLimited && remainingMs > STALL_RETRY_MIN_REMAINING_MS) {
return "wait";
}
return wedgedRetries === 0 && remainingMs > STALL_RETRY_MIN_REMAINING_MS ? "retry" : "soft-land";
}
const FINALIZE_PROMPT =
"You have reached your time budget. STOP investigating now — do NOT read, grep, " +
@@ -160,6 +512,45 @@ class DeadlineReached extends Error {
}
}
/**
* Internal signal that an in-progress reply went silent (see STALL_MS). Distinct
* from DeadlineReached: the pass's time budget is NOT spent, so the caller can spend
* what's left on a fresh attempt instead of abandoning the work.
*/
class NoProgress extends Error {
constructor(
readonly cost: number = 0,
readonly tokens?: TokenUsage,
/** How long the reply had been unchanged when we gave up on it. */
readonly idleMs: number = 0,
) {
super("no progress");
}
}
/**
* A cheap signature of how far along an in-progress reply is: its parts (count,
* type, streamed length, tool status) plus the message's token/cost counters. ANY
* real progress — a new tool call, another chunk of text or reasoning, a tool
* advancing pending → running → completed — changes it; a wedged request leaves it
* byte-identical poll after poll. Exported for tests.
*/
export function progressFingerprint(message: RawMessage): string {
const shape = (message.parts ?? [])
.map((part) => `${part.type ?? ""}:${(part.text ?? "").length}:${part.state?.status ?? ""}`)
.join("|");
const tokens = message.info?.tokens;
return [
shape,
tokens?.input ?? 0,
tokens?.output ?? 0,
tokens?.reasoning ?? 0,
tokens?.cache?.read ?? 0,
tokens?.cache?.write ?? 0,
message.info?.cost ?? 0,
].join("~");
}
const DEADLINE_SENTINEL = Symbol("deadline");
/**
@@ -197,11 +588,30 @@ async function raceDeadline<T>(
export class AgentTimeoutError extends Error {
readonly cost: number;
readonly tokens?: TokenUsage;
constructor(agent: string, minutes: number, cost = 0, tokens?: TokenUsage) {
super(`Agent "${agent}" timed out after ${minutes} minutes (including finalize)`);
/**
* Why the pass was abandoned: it investigated without converging ("time"), or its
* model request went silent and did not recover after a fresh attempt ("stall").
* The caller's handling is the same (abandon), but the two have different causes —
* a wander is ours to bound, a stall is the provider's — so logs and coverage
* notes must not conflate them.
*/
readonly reason: "time" | "stall";
constructor(
agent: string,
minutes: number,
cost = 0,
tokens?: TokenUsage,
reason: "time" | "stall" = "time",
) {
super(
reason === "stall"
? `Agent "${agent}" stalled: its model request went silent and produced nothing after a retry (${minutes} minutes)`
: `Agent "${agent}" timed out after ${minutes} minutes (including finalize)`,
);
this.name = "AgentTimeoutError";
this.cost = cost;
this.tokens = tokens;
this.reason = reason;
}
}
@@ -241,79 +651,151 @@ export async function promptAgent(
finalizeOnTimeout?: boolean;
},
): Promise<PromptResult> {
const session = unwrap<{ id: string }>(
await handle.client.session.create({ body: { title: args.title } }),
);
const reportedTools = new Set<string>();
await sendSessionPrompt(handle, session.id, {
agent: args.agent,
system: args.system,
text: args.text,
});
const maxWaitMs = args.maxWaitMs ?? DEFAULT_MAX_WAIT_MS;
try {
return await pollForCompletion(handle, session.id, {
agent: args.agent,
fromIndex: 0,
deadline: Date.now() + maxWaitMs,
onActivity: args.onActivity,
reportedTools,
maxToolCalls: args.maxToolCalls,
});
} catch (error) {
if (!(error instanceof DeadlineReached)) {
throw error;
}
// Cost/tokens burned during the (never-completed) investigation, so the
// finalize reply or the abandon path still accounts for them.
const spentCost = error.cost;
const spentTokens = error.tokens;
// Time budget hit. Interrupt the wandering run first.
await abortQuietly(handle, session.id);
// ONE deadline for the whole pass, shared by the first attempt and any stall
// retry, so retrying a wedged request can never push the pass past its declared
// cap (the budget math in review.ts depends on that being true).
const deadline = Date.now() + maxWaitMs;
// Spend from abandoned attempts, carried forward so a wedged attempt's cost and
// tokens still land in the run's metrics instead of vanishing.
let carriedCost = 0;
let carriedTokens: TokenUsage | undefined;
const carry = (result: PromptResult): PromptResult => ({
...result,
cost: result.cost + carriedCost,
tokens: addTokenUsage(addTokenUsage({}, carriedTokens), result.tokens),
});
const absorb = (spent: { cost: number; tokens?: TokenUsage }): void => {
carriedCost += spent.cost;
carriedTokens = addTokenUsage(addTokenUsage({}, carriedTokens), spent.tokens);
};
/**
* Last chance to get something out of a pass that hit its ceiling: interrupt the
* run and ask the SAME (context-carrying) session for whatever it already has.
* Tools are disabled for this request, so it cannot resume investigating — the
* only thing it can do is emit.
*/
const softLand = async (
sessionID: string,
reason: "time" | "stall",
reportedTools: Set<string>,
): Promise<PromptResult> => {
await abortQuietly(handle, sessionID);
const minutes = Math.round(maxWaitMs / 60000);
if (!args.finalizeOnTimeout) {
throw new AgentTimeoutError(
args.agent,
Math.round(maxWaitMs / 60000),
spentCost,
spentTokens,
);
throw new AgentTimeoutError(args.agent, minutes, carriedCost, carriedTokens, reason);
}
// Soft landing: ask the (same, context-carrying) session to return whatever
// it has now. Only messages after this point count as the answer.
const baseline = (await fetchMessages(handle, session.id)).length;
args.onActivity?.("time budget reached — asking for findings so far");
await sendSessionPrompt(handle, session.id, {
// Only messages after this point count as the answer.
const baseline = (await fetchMessages(handle, sessionID)).length;
args.onActivity?.(
reason === "stall"
? "no output after a retry — asking for findings so far"
: "time budget reached — asking for findings so far",
);
await sendSessionPrompt(handle, sessionID, {
agent: args.agent,
system: args.system,
text: FINALIZE_PROMPT,
tools: NO_TOOLS,
});
try {
const result = await pollForCompletion(handle, session.id, {
const result = await pollForCompletion(handle, sessionID, {
agent: args.agent,
fromIndex: baseline,
deadline: Date.now() + FINALIZE_WAIT_MS,
onActivity: args.onActivity,
reportedTools,
stallMs: FINALIZE_STALL_MS,
});
return {
...result,
cost: result.cost + spentCost,
tokens: addTokenUsage(addTokenUsage({}, spentTokens), result.tokens),
truncated: true,
};
return { ...carry(result), truncated: true };
} catch (finalizeError) {
if (finalizeError instanceof DeadlineReached) {
await abortQuietly(handle, session.id);
if (finalizeError instanceof DeadlineReached || finalizeError instanceof NoProgress) {
await abortQuietly(handle, sessionID);
absorb(finalizeError);
throw new AgentTimeoutError(
args.agent,
Math.round((maxWaitMs + FINALIZE_WAIT_MS) / 60000),
spentCost + finalizeError.cost,
addTokenUsage(addTokenUsage({}, spentTokens), finalizeError.tokens),
carriedCost,
carriedTokens,
reason,
);
}
throw finalizeError;
}
};
let wedgedRetries = 0;
for (let attempt = 0; ; attempt++) {
const session = unwrap<{ id: string }>(
await handle.client.session.create({
body: { title: attempt === 0 ? args.title : `${args.title}-retry${attempt}` },
}),
);
const reportedTools = new Set<string>();
await sendSessionPrompt(handle, session.id, {
agent: args.agent,
system: args.system,
text: args.text,
});
try {
return carry(
await pollForCompletion(handle, session.id, {
agent: args.agent,
fromIndex: 0,
deadline,
onActivity: args.onActivity,
reportedTools,
maxToolCalls: args.maxToolCalls,
stallMs: stallWindowMs(maxWaitMs),
}),
);
} catch (error) {
// Went silent. The request is wedged, not slow, so the first move is a clean
// slate — a fresh session, not the finalize prompt, which would be asking the
// wedged request to answer. Exactly one retry, and only when enough budget
// remains for it to land; after that the finalize is still worth a try as the
// only remaining salvage (in eas-cli#4084 the session did respond once aborted).
//
// EXCEPT when the server log shows the account is rate-limited: then the
// silence is throttling, not a wedge, and the patient move is to wait —
// re-sending the pass's whole context would deepen the limit. Waits repeat
// (never consuming the one wedged retry) until the evidence goes stale or
// the pass runs out of room, both bounded by the pass deadline.
if (error instanceof NoProgress) {
await abortQuietly(handle, session.id);
absorb(error);
await handle.rateLimit.check();
const remaining = deadline - Date.now();
const action = stallAction(wedgedRetries, remaining, handle.rateLimit.recentlyLimited());
if (action === "wait") {
args.onActivity?.(
`provider is rate-limiting this account (429 in the server log; ` +
`${handle.rateLimit.events} so far) — waiting ${Math.round(RATE_LIMIT_WAIT_MS / 1000)}s ` +
`instead of retrying (${Math.round(remaining / 60000)}m of budget left)`,
);
await sleep(RATE_LIMIT_WAIT_MS);
continue;
}
if (action === "retry") {
wedgedRetries++;
args.onActivity?.(
`stalled — no output for ${Math.round(error.idleMs / 1000)}s; ` +
`retrying once from a clean session (${Math.round(remaining / 60000)}m of budget left)`,
);
await sleep(STALL_RETRY_BACKOFF_MS);
continue;
}
return await softLand(session.id, "stall", reportedTools);
}
// Ran the clock down while investigating: converge on what it has.
if (error instanceof DeadlineReached) {
absorb(error);
return await softLand(session.id, "time", reportedTools);
}
throw error;
}
}
}
@@ -327,6 +809,20 @@ const CORRECTIVE_WAIT_MS = 2 * 60 * 1000;
/** Backoff (ms) before the 2nd and 3rd attempt of a transient-failing model call. */
const TRANSIENT_BACKOFF_MS = [2_000, 8_000];
/**
* Rate limits need patience, not persistence: a limited account stays limited for
* tens of seconds to minutes, so the 2s/8s schedule just burns the retries. Shared
* subscription credentials (several PRs reviewing at once) make this the common
* transient, hence the dedicated, slower schedule.
*/
const RATE_LIMIT_BACKOFF_MS = [15_000, 45_000, 90_000];
const RATE_LIMIT_ERROR = /\b429\b|rate.?limit|too many requests/i;
/** A transient error that is specifically a provider rate limit. */
export function isRateLimitError(error: unknown): boolean {
return !(error instanceof AgentTimeoutError) && RATE_LIMIT_ERROR.test(errorMessage(error));
}
/**
* A transient, retryable API failure — a one-off rate-limit (429), server error
@@ -376,13 +872,15 @@ async function withTransientRetry<T>(
try {
return await fn();
} catch (error) {
const waitMs = TRANSIENT_BACKOFF_MS[attempt];
// Rate limits get the slower, longer schedule — see RATE_LIMIT_BACKOFF_MS.
const schedule = isRateLimitError(error) ? RATE_LIMIT_BACKOFF_MS : TRANSIENT_BACKOFF_MS;
const waitMs = schedule[attempt];
if (waitMs === undefined || !isTransientApiError(error)) {
throw error;
}
onActivity?.(
`${label}: transient API error (${errorMessage(error)}); retry ${attempt + 1}/${
TRANSIENT_BACKOFF_MS.length
schedule.length
} in ${Math.round(waitMs / 1000)}s`,
);
await sleep(waitMs);
@@ -413,14 +911,17 @@ export async function promptAndParse<T>(
finalizeOnTimeout?: boolean;
},
parse: (text: string) => T,
): Promise<{ value: T; cost: number; truncated: boolean; tokens: TokenUsage }> {
): Promise<{ value: T; cost: number; truncated: boolean; tokens: TokenUsage; model?: string }> {
let cost = 0;
let truncated = false;
let model: string | undefined;
const tokens: TokenUsage = {};
const record = (result: PromptResult): void => {
cost += result.cost;
truncated = truncated || (result.truncated ?? false);
addTokenUsage(tokens, result.tokens);
// Keep the model from whichever attempt actually answered.
model = result.model ?? model;
};
const first = await withTransientRetry(`Agent "${args.agent}"`, args.onActivity, () =>
@@ -428,7 +929,7 @@ export async function promptAndParse<T>(
);
record(first);
try {
return { value: parse(first.text), cost, truncated, tokens };
return { value: parse(first.text), cost, truncated, tokens, model };
} catch {
// Same-session corrective retry: send the nudge as a follow-up and wait for
// the NEW assistant message (past the current message count).
@@ -438,6 +939,9 @@ export async function promptAndParse<T>(
agent: args.agent,
system: args.system,
text: CORRECTIVE,
// Re-emit only: the investigation is done, so no tools are needed and
// disabling them keeps the corrective from turning into a second wander.
tools: NO_TOOLS,
});
const retry = await pollForCompletion(handle, first.sessionID, {
agent: args.agent,
@@ -445,9 +949,10 @@ export async function promptAndParse<T>(
deadline: Date.now() + CORRECTIVE_WAIT_MS,
onActivity: args.onActivity,
reportedTools: new Set<string>(),
stallMs: FINALIZE_STALL_MS,
});
record(retry);
return { value: parse(retry.text), cost, truncated, tokens };
return { value: parse(retry.text), cost, truncated, tokens, model };
} catch {
// Fresh-session last resort: a clean slate for a genuinely confused run.
const fresh = await promptAgent(handle, {
@@ -457,7 +962,7 @@ export async function promptAndParse<T>(
});
record(fresh);
try {
return { value: parse(fresh.text), cost, truncated, tokens };
return { value: parse(fresh.text), cost, truncated, tokens, model };
} catch (finalError) {
throw new Error(
`Agent "${args.agent}" did not return parseable JSON after retries: ${
@@ -478,6 +983,9 @@ interface RawMessage {
cost?: number;
tokens?: TokenUsage;
time?: { completed?: number };
/** Which provider/model the server actually used for this reply. */
providerID?: string;
modelID?: string;
};
parts?: Array<{
id?: string;
@@ -496,7 +1004,13 @@ async function fetchMessages(handle: OpencodeHandle, sessionID: string): Promise
async function sendSessionPrompt(
handle: OpencodeHandle,
sessionID: string,
args: { agent: string; system: string; text: string },
args: {
agent: string;
system: string;
text: string;
/** Per-request tool override (see NO_TOOLS). Omit to use the agent's own set. */
tools?: Record<string, boolean>;
},
): Promise<void> {
unwrap(
await handle.client.session.promptAsync({
@@ -505,6 +1019,7 @@ async function sendSessionPrompt(
agent: args.agent,
system: args.system,
parts: [{ type: "text", text: args.text }],
...(args.tools ? { tools: args.tools } : {}),
},
}),
);
@@ -522,7 +1037,8 @@ async function abortQuietly(handle: OpencodeHandle, sessionID: string): Promise<
* Poll a session for the first assistant message at or after `fromIndex` to
* complete. `fromIndex` lets a follow-up prompt (finalize, corrective retry)
* skip the earlier completed message and wait for the NEW reply instead. Throws
* DeadlineReached once `deadline` passes.
* DeadlineReached once `deadline` passes, or NoProgress once the reply has been
* unchanged for `stallMs` (see STALL_MS).
*/
async function pollForCompletion(
handle: OpencodeHandle,
@@ -534,6 +1050,9 @@ async function pollForCompletion(
onActivity?: (line: string) => void;
reportedTools: Set<string>;
maxToolCalls?: number;
/** No-progress ceiling. Undefined = no stall watchdog (never wait forever on
* a wedged request unless the caller has its own reason to). */
stallMs?: number;
},
): Promise<PromptResult> {
// Best-effort usage of the in-progress assistant message, so a task that times
@@ -542,6 +1061,11 @@ async function pollForCompletion(
let lastTokens: TokenUsage | undefined;
const startedAt = Date.now();
let lastEmitAt = startedAt;
// Stall watchdog state: when the reply last changed in any way, and what it looked
// like then. Starts at "now" so a prompt that never produces an assistant message
// at all (a wedged submission) also trips the watchdog.
let lastProgressAt = startedAt;
let lastFingerprint = "";
const emit = (line: string): void => {
lastEmitAt = Date.now();
opts.onActivity?.(line);
@@ -553,9 +1077,16 @@ async function pollForCompletion(
await sleep(POLL_INTERVAL_MS);
// Heartbeat if nothing has been reported for a while (e.g. the model is
// reasoning without calling tools), so a long pass doesn't look hung.
// reasoning without calling tools), so a long pass doesn't look hung. Say how
// long the reply has been unchanged, not just how long the pass has run — that
// distinction is what tells a slow investigation apart from a wedged request,
// and its absence is why eas-cli#4084 took a forensic dig to explain.
if (opts.onActivity && Date.now() - lastEmitAt >= HEARTBEAT_MS) {
emit(`still working… ${Math.round((Date.now() - startedAt) / 1000)}s elapsed`);
const idleMs = Date.now() - lastProgressAt;
emit(
`still working… ${Math.round((Date.now() - startedAt) / 1000)}s elapsed` +
(idleMs >= HEARTBEAT_MS ? ` (no new output for ${Math.round(idleMs / 1000)}s)` : ""),
);
}
// Bound the fetch by the deadline: a stalled server can't push the task past
@@ -576,6 +1107,13 @@ async function pollForCompletion(
lastTokens = assistant.info.tokens;
}
// Stall watchdog: did the reply change AT ALL since the last poll?
const fingerprint = progressFingerprint(assistant);
if (fingerprint !== lastFingerprint) {
lastFingerprint = fingerprint;
lastProgressAt = Date.now();
}
// Track each distinct tool call once (for the tool-call cap) and, the first
// time it starts, emit a live line so a long run shows what the agent is doing.
for (const part of assistant.parts ?? []) {
@@ -612,6 +1150,7 @@ async function pollForCompletion(
cost: assistant.info?.cost ?? 0,
sessionID,
tokens: assistant.info?.tokens,
model: formatModel(assistant.info?.providerID, assistant.info?.modelID),
};
}
@@ -622,5 +1161,18 @@ async function pollForCompletion(
emit(`made ${opts.reportedTools.size} tool calls — wrapping up to stay on budget`);
throw new DeadlineReached(lastCost, lastTokens);
}
// Still in progress and completely silent: the request is wedged, not slow.
// Bail out NOW rather than spending the rest of the cap on a dead request —
// the caller retries a stall from a clean session (see promptAgent).
if (opts.stallMs != null) {
const idleMs = Date.now() - lastProgressAt;
if (idleMs >= opts.stallMs) {
emit(
`no new output for ${Math.round(idleMs / 1000)}s — treating the model request as stalled`,
);
throw new NoProgress(lastCost, lastTokens, idleMs);
}
}
}
}
+76 -9
View File
@@ -166,20 +166,85 @@ export function buildReviewerTask(
* large diff. It sees the whole change set and reports ONLY issues that span
* multiple changed files, which per-chunk reviews can't see.
*/
/**
* Total changed lines the cross-file task will inline before it stops and lists the
* rest as patch paths to read on demand. Sized to stay well inside the model's
* context on a normal PR while still covering the overwhelming majority of them; a
* genuinely huge diff degrades to the old read-on-demand behavior for its tail
* rather than overflowing.
*/
export const CROSS_CUTTING_INLINE_MAX_LINES = 6000;
/**
* Split the changed files into the ones whose diffs are inlined into the cross-file
* task and the ones left for on-demand reads. Always inlines at least the first file
* so a single enormous file can't produce an all-deferred prompt. Exported for tests.
*/
export function splitCrossCuttingInline(
allFiles: PatchWorkspaceFile[],
maxLines = CROSS_CUTTING_INLINE_MAX_LINES,
): { inlined: PatchWorkspaceFile[]; deferred: PatchWorkspaceFile[] } {
const inlined: PatchWorkspaceFile[] = [];
const deferred: PatchWorkspaceFile[] = [];
let lines = 0;
for (const file of allFiles) {
if (inlined.length > 0 && lines + file.changedLines > maxLines) {
deferred.push(file);
continue;
}
inlined.push(file);
lines += file.changedLines;
}
return { inlined, deferred };
}
export function buildCrossCuttingTask(
allFiles: PatchWorkspaceFile[],
agents: LoadedAgent[],
filtered: FilteredFile[] = [],
/** Set for the no-tools fallback pass, which cannot open anything it isn't shown. */
opts: { noTools?: boolean } = {},
): string {
const lenses = agents
.map((agent) => `- ${agent.id}: ${agent.description || agent.id}`)
.join("\n");
const fileList = allFiles
.map(
(file) =>
`- \`${sanitizeUntrusted(file.path)}\` (${file.status ?? "M"}) — patch: \`${file.patchPath}\``,
)
.join("\n");
// Inline the diffs instead of only naming their patch files. Reading them back was
// one tool round-trip per changed file BEFORE any tracing could start (13 reads and
// several minutes on a 14-file PR), spent on content we already have in memory.
const { inlined, deferred } = splitCrossCuttingInline(allFiles);
const inlinedDiffs = inlined.map(inlineDiff).join("\n\n");
// On a diff too large to inline whole, the tail is named either way — a file this
// pass can't see must never look unchanged. What differs is the instruction: a
// no-tools pass told to "read their patch files" would be told to do the one thing
// it can't, so it gets the same "you cannot see these, don't fault them" framing
// the noise-filtered files get.
const deferredSection =
deferred.length === 0
? []
: opts.noTools
? [
"",
"This PR changed these files too, but their diffs are NOT shown to you (the",
"diff is large) and you cannot open them on this pass:",
deferred
.map((file) => `- \`${sanitizeUntrusted(file.path)}\` (${file.status ?? "M"})`)
.join("\n"),
"",
"They WERE changed by this PR. Do NOT report that any of them was not updated,",
"and do not claim an interaction you cannot see in the diffs above.",
]
: [
"",
"This PR changed these files too, but their diffs are NOT inlined above (the",
"diff is large). Read their patch files on demand if a cross-file interaction",
"points at them:",
deferred
.map(
(file) =>
`- \`${sanitizeUntrusted(file.path)}\` (${file.status ?? "M"}) — patch: \`${file.patchPath}\``,
)
.join("\n"),
];
return [
"This PR changed the files below, and each was already reviewed on its own by",
@@ -194,7 +259,7 @@ export function buildCrossCuttingTask(
"single-file issues.",
"",
"Stay focused and efficient — you are on a time budget:",
"- Work from the patches of the CHANGED files listed below; that is your scope.",
"- Work from the diffs of the CHANGED files below; that is your scope.",
"- Read additional source ONLY when directly needed to confirm a specific",
" cross-file interaction (e.g. open the caller a changed signature affects).",
"- Do NOT audit unrelated parts of the repository or read files with no",
@@ -202,8 +267,10 @@ export function buildCrossCuttingTask(
"- As soon as you have traced the cross-file interactions, return your answer;",
" do not keep exploring for completeness.",
"",
"Changed files:",
fileList,
"Changed files (diffs inlined — you do not need to read these back):",
"",
inlinedDiffs,
...deferredSection,
...filteredSection(filtered),
"",
"Return the single JSON object described in your instructions and nothing else.",
+9 -2
View File
@@ -154,7 +154,12 @@ export function renderMarkdown(
const dropped = withFp.filter(({ fp }) => dismissedByFp.has(fp));
const lines: string[] = [commentMarker(tag), "## 🤖 AI code review", ""];
lines.push(`**Decision:** ${decisionLabel(review.decision)}`, "", review.summary, "");
lines.push(
`**Decision:** ${review.couldNotComplete ? "No review — every pass failed" : decisionLabel(review.decision)}`,
"",
review.summary,
"",
);
if (review.incomplete.length > 0) {
lines.push(
@@ -334,7 +339,9 @@ export function renderAggregateMarkdown(
"| --- | --- | --- |",
];
for (const { result, kept } of perScope) {
lines.push(`| ${result.scope} | ${decisionLabel(result.review.decision)} | ${kept.length} |`);
lines.push(
`| ${result.scope} | ${result.review.couldNotComplete ? "No review — every pass failed" : decisionLabel(result.review.decision)} | ${kept.length} |`,
);
}
lines.push("");
+280 -48
View File
@@ -1,7 +1,7 @@
import path from "node:path";
import type { LoadedAgent, LoadedConfig } from "../config/schema.js";
import type { ReviewSource } from "../sources/source.js";
import type { PreparedReadRoot, ReviewSource } from "../sources/source.js";
import { prepareAuth } from "./auth.js";
import { coordinate } from "./coordinator.js";
import { writeRunLog } from "./log.js";
@@ -11,6 +11,7 @@ import type { PatchWorkspaceFile } from "./noise.js";
import {
addTokenUsage,
AgentTimeoutError,
assertModelsResolvable,
buildOpencodeConfig,
CROSS_CUTTING_AGENT,
promptAndParse,
@@ -45,6 +46,14 @@ export interface ReviewRunOptions {
includePaths?: string[];
/** Wall-clock ceiling for all review passes. Default: today's PASSES_BUDGET_MS. */
passesBudgetMs?: number;
/**
* Directory for the run log + patch workspace (`.runs/`). Defaults to
* `<configDir>/.runs` — correct when config lives in the checkout, WRONG when
* config was materialized into a temporary trusted root (removed on exit, and
* CI uploads the log from the workspace path). `ecr ci` pins this to the
* workspace checkout explicitly.
*/
runsDir?: string;
}
/**
@@ -72,6 +81,21 @@ function makeRunId(): string {
* → coordinate → apply policy. Returns a CoordinatorOutput; the CLI commands are
* thin wrappers that supply a Source and render the result.
*/
/**
* Max concurrent reviewer calls: an explicit config value wins; otherwise 3 when a
* subscription (oauth) credential is configured, else 6. One ChatGPT account
* handles six parallel streams poorly — requests get parked server-side (the
* stall signature seen on eas-cli#4084), and several PRs may be reviewing on the
* same credential at once — so subscription runs trade a little wall-clock for a
* lot of reliability. Exported for tests.
*/
export function effectiveConcurrency(config: LoadedConfig): number {
if (config.chunk.concurrency) {
return config.chunk.concurrency;
}
return config.auth.some((entry) => entry.mode === "oauth") ? 3 : 6;
}
export async function runReview(
source: ReviewSource,
options: ReviewRunOptions,
@@ -80,7 +104,7 @@ export async function runReview(
const started = Date.now();
const runId = makeRunId();
const progress = options.onProgress ?? (() => {});
const runsRoot = path.join(config.configDir, ".runs");
const runsRoot = options.runsDir ?? path.join(config.configDir, ".runs");
const runDir = path.join(runsRoot, runId);
const logPath = path.join(runsRoot, "reviews.jsonl");
@@ -135,19 +159,28 @@ export async function runReview(
return output;
}
// Prepare auth BEFORE the chdir below: it doesn't depend on the working directory,
// and doing it first means nothing that can throw sits between the chdir and the
// guarded blocks — so a prepareAuth failure can't leak the worktree or leave cwd
// pointing at it.
const auth = await prepareAuth(config);
// Read the PR-head tree (not the current checkout) when the source can materialize
// it, so the agents' surrounding-source reads and the verifier's re-reads see the
// versions that match the diff. Config is already fully loaded in memory, so the
// chdir doesn't affect it; run-log/patch paths are absolute; gh/git calls already
// ran above. Fails soft to the current directory.
// Materialize the PR-head tree (not the current checkout) when the source can, so
// the agents' surrounding-source reads and the verifier's re-reads see the versions
// that match the diff. Config is already fully loaded in memory, so the chdir below
// doesn't affect it; run-log/patch paths are absolute; gh/git calls already ran
// above. Failure policy is MODE-DEPENDENT (see resolveReadRoot): CI fails closed —
// with a base-SHA checkout the fallback tree is pre-PR content, and silently
// reviewing/verifying that drops real findings — while a local run falls back to
// the user's own checkout with a warning.
const originalCwd = process.cwd();
const readRoot = (await source.prepareReadRootAsync?.()) ?? null;
const readRoot = await resolveReadRoot(source, options.mode, progress);
// Prepare auth BEFORE the chdir below: it doesn't depend on the working directory,
// and doing it after readRoot but before chdir means a prepareAuth failure can't
// leave cwd pointing at the worktree — it only has the worktree itself to release.
let auth: Awaited<ReturnType<typeof prepareAuth>>;
try {
auth = await prepareAuth(config);
} catch (error) {
await readRoot?.cleanup();
throw error;
}
const restoreCwd = async (): Promise<void> => {
if (readRoot) {
process.chdir(originalCwd);
@@ -168,10 +201,26 @@ export async function runReview(
await restoreCwd();
throw new Error(
`Failed to start the OpenCode server. Ensure the \`opencode\` CLI is installed and ` +
`model credentials are configured.\n${errorMessage(error)}`,
`model credentials are configured (\`ecr doctor\` checks both).\n${errorMessage(error)}`,
);
}
// Preflight: a model id the server can't resolve would otherwise fail EVERY pass
// identically — N indistinguishable coverage gaps, after spending the run's budget
// discovering the same fixable thing N times. Throw once, up front, naming the fix.
try {
await assertModelsResolvable(
handle,
[...config.agents.map((agent) => agent.model), config.coordinator.model],
config.auth,
);
} catch (error) {
handle.close();
await auth.cleanup();
await restoreCwd();
throw error;
}
const agentCosts: Record<string, number> = {};
const tokenTotals: TokenUsage = {};
const agentTokens: Record<string, TokenUsage> = {};
@@ -185,6 +234,23 @@ export async function runReview(
addTokenUsage(tokenTotals, tokens);
addTokenUsage((agentTokens[bucket] ??= {}), tokens);
};
// The provider/model that ACTUALLY answered each pass, and any pass whose model was
// silently substituted for the configured one. OpenCode does that substitution
// quietly whenever an agent's model id is empty or unusable, so a run can review with
// a different model than config.jsonc names and look completely normal — which is
// exactly what happened for weeks behind an empty REVIEWER_MODEL. Recorded in the run
// log, reported in the log line, and surfaced as a coverage note when it happens.
const agentModels: Record<string, string> = {};
const substituted = new Set<string>();
const trackModel = (bucket: string, configured: string, actual?: string): void => {
if (!actual) {
return;
}
agentModels[bucket] = actual;
if (configured && actual !== configured) {
substituted.add(`${bucket}: configured ${configured}, ran ${actual}`);
}
};
try {
const workspace = await writePatchWorkspace(kept, metadata, runDir);
@@ -213,10 +279,11 @@ export async function runReview(
);
// Only chunk (and add a cross-cutting pass) when the diff exceeds one chunk.
const chunked = chunks.length > 1;
const concurrency = effectiveConcurrency(config);
progress(
`Running ${selectedAgents.length} reviewer(s) [${selectedAgents.map((a) => a.id).join(", ")}] over ${chunks.length} chunk(s)` +
`${chunked ? " + cross-cutting pass" : ""} ` +
`(${kept.length} files, concurrency ${config.chunk.concurrency})…`,
`(${kept.length} files, concurrency ${concurrency})…`,
);
for (const agent of selectedAgents) {
@@ -252,7 +319,6 @@ export async function runReview(
// These caps must fit inside PASSES_BUDGET_MS (below), which in turn fits inside
// the CI job's timeout-minutes.
const CHUNK_TIMEOUT_MS = 15 * 60 * 1000;
const CROSS_CUTTING_TIMEOUT_MS = 25 * 60 * 1000;
// A subdivided sub-chunk is smaller, so it gets a shorter cap (halved per level,
// floored) — enough to converge without letting the recursion balloon.
const SUBDIVIDE_MIN_TIMEOUT_MS = 6 * 60 * 1000;
@@ -262,15 +328,62 @@ export async function runReview(
// Tool-call ceilings — generous for a legitimate pass, low enough to catch
// runaway roaming (the root cause of the non-convergent timeouts).
const CHUNK_MAX_TOOL_CALLS = 50;
const CROSS_CUTTING_MAX_TOOL_CALLS = 120;
// Global ceiling for ALL passes incl. subdivision/fallback waves, sized to
// leave room for the coordinator (10m) + verification + overhead inside the CI
// job timeout. Past this, a timed-out pass is reported as a gap rather than
// broken down further, so total wall-clock stays bounded.
const PASSES_BUDGET_MS = 32 * 60 * 1000;
const PASSES_BUDGET_MS = 55 * 60 * 1000;
const passesBudgetMs = options.passesBudgetMs ?? PASSES_BUDGET_MS;
const passesDeadline = started + passesBudgetMs;
// The cross-file pass is the one pass whose scope cannot be traded for
// convergence: halving its file set deletes exactly the coverage it exists to
// provide (see the timeout branch below). So instead of a fixed cap it gets the
// WHOLE remaining passes window. Chunk passes run concurrently alongside it under
// their own caps, so a long cross-file pass doesn't starve them — it only extends
// the run toward the passes deadline, which the job timeout is sized for.
//
// Computed here, not as a constant, because the window is what's actually left:
// filtering, routing and server startup already spent some of it, and `ecr ci`
// divides the budget across active scopes.
//
// The reserve is what keeps its own salvage paths affordable: if it expanded into
// the entire window, then on a timeout there would be nothing left to run the
// whole-diff no-tools fallback with, and "elastic budget" would have quietly
// reintroduced the coverage gap it exists to prevent. Sized for the finalize
// soft-landing plus one FALLBACK_TIMEOUT_MS pass.
const CROSS_CUTTING_RESERVE_MS = FALLBACK_TIMEOUT_MS + 4 * 60 * 1000;
// Floor: never LESS generous than one chunk pass. On a run whose window is already
// small (many active scopes dividing the budget) this can exceed what's left, but
// that exposure is exactly what chunk passes already carry — their 15m cap can also
// outlast a small per-scope slice — and the job timeout keeps a wide margin over
// the budget for it. Dropping the pass instead would silently cost the coverage no
// other pass provides.
const crossCuttingWaitMs = Math.max(
CHUNK_TIMEOUT_MS,
passesDeadline - Date.now() - CROSS_CUTTING_RESERVE_MS,
);
// Tool calls are for TRACING (opening the caller a changed signature affects) —
// the changed files' diffs are inlined, so they are not spent fetching the diff.
// Scale with the diff's file count instead of fixing the ceiling: under a large
// elastic time budget a fixed cap becomes the binding constraint, and the extra
// time can't be used.
const CROSS_CUTTING_TOOL_CALLS_PER_FILE = 10;
const CROSS_CUTTING_MIN_TOOL_CALLS = 120;
const CROSS_CUTTING_MAX_TOOL_CALLS = 400;
const crossCuttingMaxToolCalls = Math.min(
CROSS_CUTTING_MAX_TOOL_CALLS,
Math.max(
CROSS_CUTTING_MIN_TOOL_CALLS,
CROSS_CUTTING_TOOL_CALLS_PER_FILE * workspace.files.length,
),
);
// Coverage notes for passes that hit their time limit, stalled, failed, or were
// never started, surfaced in the final review so a cut-short run is never
// presented as complete.
const incomplete: string[] = [];
const tasks: ReviewTask[] = [];
for (const agent of selectedAgents) {
const system = buildReviewerSystem(config, agent);
@@ -301,8 +414,8 @@ export async function runReview(
title: "review-xcut",
files: workspace.files,
coverageLabel: "the cross-file review (issues spanning multiple changed files)",
maxWaitMs: CROSS_CUTTING_TIMEOUT_MS,
maxToolCalls: CROSS_CUTTING_MAX_TOOL_CALLS,
maxWaitMs: crossCuttingWaitMs,
maxToolCalls: crossCuttingMaxToolCalls,
depth: 0,
fallback: false,
});
@@ -312,12 +425,22 @@ export async function runReview(
// ahead of short ones so they don't dominate the tail of the makespan.
tasks.sort((a, b) => b.maxWaitMs - a.maxWaitMs);
// What each task was CONFIGURED to run on — mirrors buildOpencodeConfig, which
// gives the cross-file pass the first agent's model. Compared against what actually
// answered so a silent substitution can't pass unnoticed.
const taskModel = (task: ReviewTask): string =>
task.kind === "cross-cutting"
? (selectedAgents[0]?.model ?? config.coordinator.model)
: (selectedAgents.find((agent) => agent.id === task.bucket)?.model ?? "");
// Build the task prompt on demand (so a subdivided task rebuilds over its
// smaller file set); a fallback task forbids tools and reviews the inlined diff.
const buildTaskText = (task: ReviewTask): string => {
const base =
task.kind === "cross-cutting"
? buildCrossCuttingTask(task.files, selectedAgents, filtered)
? buildCrossCuttingTask(task.files, selectedAgents, filtered, {
noTools: task.fallback,
})
: buildReviewerTask(task.files, workspace.files, filtered);
return task.fallback ? `${base}\n\n${NO_TOOLS_INSTRUCTION}` : base;
};
@@ -340,9 +463,6 @@ export async function runReview(
...overrides,
});
// Coverage notes for passes that hit their time limit or failed, surfaced in
// the final review so a cut-short run is never presented as complete.
const incomplete: string[] = [];
let completedPasses = 0;
let failedPasses = 0;
// promptAndParse already retries internally (same-session corrective, then a
@@ -350,10 +470,10 @@ export async function runReview(
// TIMEOUT, instead of dropping the work we break it into units that converge:
// subdivide the chunk, then a fast no-tools pass, and only report a coverage gap
// when even that can't finish inside the budget — so dropped work is never silent.
await runGrowableQueue(tasks, config.chunk.concurrency, async (task, enqueue) => {
await runGrowableQueue(tasks, concurrency, async (task, enqueue) => {
const minutes = Math.round(task.maxWaitMs / 60000);
try {
const { value, cost, truncated, tokens } = await promptAndParse(
const { value, cost, truncated, tokens, model } = await promptAndParse(
handle!,
{
agent: task.bucket,
@@ -369,6 +489,7 @@ export async function runReview(
);
agentCosts[task.bucket] = (agentCosts[task.bucket] ?? 0) + cost;
trackTokens(task.bucket, tokens);
trackModel(task.bucket, taskModel(task), model);
(agentFindings[task.bucket] ??= []).push(...value.findings);
completedPasses++;
if (truncated) {
@@ -398,15 +519,15 @@ export async function runReview(
trackTokens(task.bucket, error.tokens);
const remaining = passesDeadline - Date.now();
// Cross-file analysis needs ≥2 files to be meaningful; a single-file
// reviewer chunk can't be split further.
const minFiles = task.kind === "cross-cutting" ? 2 : 1;
// Subdividing trades scope for convergence, which is the right trade for a
// reviewer chunk (each file still gets reviewed) and the WRONG one for the
// cross-file pass: an interaction between a file in the left half and one in
// the right half is invisible to both halves, so "splitting" it silently
// deletes the coverage the pass exists to provide while reporting success.
// It goes straight to the whole-diff no-tools fallback below instead.
const canSubdivide = task.kind === "reviewer" && task.files.length > 1;
const childCap = Math.max(SUBDIVIDE_MIN_TIMEOUT_MS, Math.floor(task.maxWaitMs / 2));
if (
task.files.length > minFiles &&
task.depth < MAX_SUBDIVIDE_DEPTH &&
remaining > childCap
) {
if (canSubdivide && task.depth < MAX_SUBDIVIDE_DEPTH && remaining > childCap) {
const mid = Math.ceil(task.files.length / 2);
const left = task.files.slice(0, mid);
const right = task.files.slice(mid);
@@ -418,9 +539,12 @@ export async function runReview(
enqueue(childTask(task, right, `↳${right.length}f`, over));
return;
}
// Can't subdivide further: try a fast no-tools pass over the inlined diff
// (reviewer only — cross-file analysis fundamentally needs to read files).
if (task.kind === "reviewer" && !task.fallback && remaining > FALLBACK_TIMEOUT_MS) {
// Can't (or shouldn't) subdivide: fall back to a fast no-tools pass over the
// inlined diffs. This works for the cross-file pass too — every changed file's
// diff is inlined in its task, so it can still reason across the whole diff
// without tools; it just can't open a caller outside the diff. A lighter
// cross-file review beats the coverage gap it used to report.
if (!task.fallback && remaining > FALLBACK_TIMEOUT_MS) {
progress(
` ${task.label}: exceeded ${minutes}m — retrying ${filesLabel(task.files)} with a fast no-tools pass`,
);
@@ -437,11 +561,28 @@ export async function runReview(
// never silent. Distinguish WHY so the note doesn't overstate what happened:
// we could still have split/fallen back, but the global budget ran out first,
// vs. the task was already at its smallest reviewable unit and still failed.
// A stalled pass is called out separately: it did not run out of time doing
// work, its model requests went silent, which is an infrastructure symptom
// and not something a bigger budget or a smaller scope would have fixed.
failedPasses++;
const couldStillReduce =
(task.files.length > minFiles && task.depth < MAX_SUBDIVIDE_DEPTH) ||
(task.kind === "reviewer" && !task.fallback);
if (couldStillReduce) {
(canSubdivide && task.depth < MAX_SUBDIVIDE_DEPTH) || !task.fallback;
if (error.reason === "stall") {
progress(
` ${task.label}: its model requests went silent (stalled) and did not recover — ` +
`most likely provider rate limiting; reporting a coverage gap`,
);
// Name the likely cause. OpenCode retries a 429 internally without surfacing
// it, so provider throttling reaches us as pure silence — indistinguishable
// from a wedged connection, and the single most common reason a pass produces
// nothing at all. Saying "went silent" alone sends people hunting for a bug
// in the reviewer instead of checking their usage window.
incomplete.push(
`${capitalize(task.coverageLabel)} could not run: its model requests went silent and produced no output, even after being retried. ` +
`The usual cause is the model provider rate-limiting the account (a subscription credential over its usage window), which reaches this tool as silence rather than an error; ` +
`those changes were not fully reviewed.`,
);
} else if (couldStillReduce) {
progress(
` ${task.label}: exceeded ${minutes}m and the run's time budget is spent — reporting a coverage gap`,
);
@@ -459,6 +600,21 @@ export async function runReview(
}
});
// A substituted model means the review did not run on the model this repo
// configured — the findings may be from a weaker (or free-tier) model entirely.
// Never silent: it goes to the log, the coverage notes, and the run log.
if (substituted.size > 0) {
for (const line of substituted) {
progress(` ⚠ model substituted — ${line}`);
}
incomplete.push(
`Some passes did not run on the configured model (${[...substituted].join("; ")}). ` +
`OpenCode silently falls back to a default model when the configured id is empty or ` +
`unusable, so these findings may come from a different (possibly much weaker) model ` +
`than intended — check the agents' \`model\`, \`coordinator.model\`, REVIEWER_MODEL, and the provider credential.`,
);
}
// Note: routine noise filtering (lockfiles, generated, binary) is expected and
// NOT a coverage gap — it stays in the run log (filteredFiles), not the
// user-facing coverage note, which is reserved for passes that didn't finish.
@@ -475,6 +631,9 @@ export async function runReview(
"⚠️ The AI review could not complete: every review pass failed or timed out, " +
'so these changes were effectively NOT reviewed. Treat this as "no review", not "looks good".',
incomplete: coverageNotes,
// Presentation override: without it the comment header reads "Decision:
// Approve with comments" over a review that reviewed nothing (euxy#8).
couldNotComplete: true,
};
} else {
progress("Coordinating findings…");
@@ -485,9 +644,11 @@ export async function runReview(
cost,
tokens: coordinatorTokens,
truncated: coordinatorTruncated,
model: coordinatorModel,
} = await coordinate(handle, config, metadata, agentFindings, coverageNotes);
agentCosts["coordinator"] = cost;
trackTokens("coordinator", coordinatorTokens);
trackModel("coordinator", config.coordinator.model, coordinatorModel);
consolidated = applyReviewPolicy(rawOutput, config.policy);
if (coordinatorTruncated) {
// The coordinator ran out of time and returned partial findings — flag it
@@ -524,6 +685,12 @@ export async function runReview(
const verification = await verifyFindings(handle!, output.findings, process.cwd(), progress);
agentCosts["verifier"] = verification.cost;
trackTokens("verifier", verification.tokens);
// Mirrors buildOpencodeConfig, which gives the verifier the first agent's model.
trackModel(
"verifier",
config.agents[0]?.model ?? config.coordinator.model,
verification.model,
);
verifierDropped = verification.dropped;
if (verification.dropped.length > 0) {
progress(`Verification dropped ${verification.dropped.length} unverified finding(s).`);
@@ -560,9 +727,30 @@ export async function runReview(
output = { ...output, summary: reconcileSummary(output.summary, output.findings.length) };
}
// Surface provider throttling as a fact about the run: passes already waited or
// backed off, but the operator should still SEE that it happened (a run that
// was rate-limited is slower and may carry partial passes — that's the cause).
await handle!.rateLimit.check();
if (handle!.rateLimit.events > 0) {
progress(
` ⚠ provider rate-limited this run ${handle!.rateLimit.events} time(s) ` +
`(429s in the OpenCode server log) — passes waited it out rather than failing`,
);
}
// Every pass says which model actually answered it — in the job log, the step
// summary table, and the run log — so a wrong or substituted model is always
// visible, not just when the substitution warning fires.
if (Object.keys(agentModels).length > 0) {
progress(
`Models used — ${Object.entries(agentModels)
.map(([bucket, model]) => `${bucket}: ${model}`)
.join("; ")}`,
);
}
progress(formatUsageSummary(tokenTotals, sum(agentCosts)));
await appendStepSummary(
renderUsageMarkdown(agentTokens, agentCosts, tokenTotals, sum(agentCosts)),
renderUsageMarkdown(agentTokens, agentCosts, tokenTotals, sum(agentCosts), agentModels),
);
await safeLog(logPath, {
@@ -571,9 +759,11 @@ export async function runReview(
totalCost: sum(agentCosts),
tokens: tokenTotals,
agentTokens,
agentModels,
agentFindings,
coverageNotes,
verifierDropped,
...(handle!.rateLimit.events > 0 ? { rateLimitEvents: handle!.rateLimit.events } : {}),
durationMs: Date.now() - started,
decision: output.decision,
findingCount: output.findings.length,
@@ -700,6 +890,47 @@ export function reconcileSummary(summary: string, remaining: number): string {
);
}
/**
* Resolve the tree the review reads from, applying the mode's trust policy:
*
* - `null` from the source means "nothing to materialize" — reviewing the current
* checkout is intended (local diffs, or `--pr` without a repo). Never an error.
* - A materialization FAILURE (throw) is fatal in CI: the checkout there is the
* trusted BASE tree, and falling back to it would silently review and verify
* pre-PR file contents (dropping real findings with no trace in the output).
* The throw propagates to `ecr ci`'s catch, which posts the one terminal
* "not reviewed" comment.
* - The same failure in local mode degrades softly to the user's own checkout —
* the user is the trust principal there and sees the warning directly.
*
* Exported for tests.
*/
export async function resolveReadRoot(
source: ReviewSource,
mode: ReviewRunOptions["mode"],
progress: (message: string) => void,
): Promise<PreparedReadRoot | null> {
if (!source.prepareReadRootAsync) {
return null;
}
try {
return await source.prepareReadRootAsync();
} catch (error) {
if (mode === "ci") {
throw new Error(
`Could not materialize the PR-head tree to review (and the CI checkout is the ` +
`trusted base, so reviewing it instead would silently review the wrong ` +
`contents): ${errorMessage(error)}`,
);
}
progress(
`Could not materialize the PR-head tree (${errorMessage(error)}); ` +
`reading the current checkout instead — file contents may not match the PR.`,
);
return null;
}
}
/** Capitalize the first letter (coverage notes read as sentences). */
function capitalize(text: string): string {
return text.length > 0 ? text[0]!.toUpperCase() + text.slice(1) : text;
@@ -848,18 +1079,19 @@ export function renderUsageMarkdown(
agentCosts: Record<string, number>,
totals: TokenUsage,
totalCost: number,
agentModels: Record<string, string> = {},
): string {
const row = (label: string, tokens: TokenUsage, cost: number): string =>
`| ${label} | ${tokens.input ?? 0} | ${tokens.output ?? 0} | ${tokens.cache?.read ?? 0} | ${tokens.cache?.write ?? 0} | $${cost.toFixed(4)} |`;
const row = (label: string, model: string, tokens: TokenUsage, cost: number): string =>
`| ${label} | ${model} | ${tokens.input ?? 0} | ${tokens.output ?? 0} | ${tokens.cache?.read ?? 0} | ${tokens.cache?.write ?? 0} | $${cost.toFixed(4)} |`;
const lines = [
"### 🤖 AI review — token usage",
"",
"| pass | input | output | cache read | cache write | cost |",
"| --- | ---: | ---: | ---: | ---: | ---: |",
"| pass | model | input | output | cache read | cache write | cost |",
"| --- | --- | ---: | ---: | ---: | ---: | ---: |",
...Object.keys(agentCosts).map((bucket) =>
row(bucket, agentTokens[bucket] ?? {}, agentCosts[bucket] ?? 0),
row(bucket, agentModels[bucket] ?? "—", agentTokens[bucket] ?? {}, agentCosts[bucket] ?? 0),
),
row("**total**", totals, totalCost),
row("**total**", "", totals, totalCost),
];
const read = totals.cache?.read ?? 0;
const uncached = totals.input ?? 0;
+15
View File
@@ -38,6 +38,13 @@ export interface ReviewMetadata {
body: string;
baseRef: string;
headRef: string;
/**
* Immutable commit OIDs for the PR's base and head, when the source can resolve
* them (GitHub PRs). Materialization pins to these — never to branch names —
* so a rename/force-push between API calls can't swap the reviewed tree.
*/
baseOid?: string;
headOid?: string;
}
export const FindingSchema = z.object({
@@ -86,6 +93,14 @@ export const CoordinatorOutputSchema = z.object({
* cut-short review is never presented as complete.
*/
incomplete: z.array(z.string()).default([]),
/**
* True when EVERY pass failed — nothing was actually reviewed. Set by the
* engine, never the model. Reporters must not render an approving decision
* label for such a run (the decision enum has no "no review" member, and
* widening it would ripple through dismiss state and exit codes — this flag
* overrides the presentation instead).
*/
couldNotComplete: z.boolean().optional(),
});
export type CoordinatorOutput = z.infer<typeof CoordinatorOutputSchema>;
+66
View File
@@ -0,0 +1,66 @@
import { readdir, rm } from "node:fs/promises";
import path from "node:path";
/**
* Ambient runtime configuration the OpenCode server (and the Claude-compatible
* loaders inside it) discovers from its project directory. The review core chdirs
* into a materialized PR-HEAD worktree before starting the server, so every one
* of these is attacker-writable in a PR: a plugin or MCP definition is arbitrary
* code execution in a process holding the model credential and a comment-capable
* GH_TOKEN; a `.env` can repoint a provider base URL; instruction files inject
* system-level prompts. OPENCODE_CONFIG_CONTENT (how ECR passes its own config)
* MERGES with project config rather than replacing it, so deleting these from the
* throwaway worktree is the only isolation that doesn't depend on OpenCode
* semantics.
*
* Exact-name entries match files or directories at any depth; `.env` is matched
* as a prefix (`.env`, `.env.local`, …). The PR's CHANGES to these files are
* still reviewed — their diffs are inlined in the task prompt — but the reviewer
* can no longer open their full head contents, and a finding citing one will
* fail verification (a documented tradeoff of the scrub approach).
*/
export const AMBIENT_RUNTIME_CONFIG_NAMES = new Set([
"opencode.json",
"opencode.jsonc",
".opencode",
"AGENTS.md",
"CLAUDE.md",
".claude",
".mcp.json",
".cursor",
".cursorrules",
]);
/** Names never descended into (and never scrubbed as a unit — `.git` is the worktree link). */
const SKIP_DIRS = new Set([".git", "node_modules"]);
/** Whether a directory entry is ambient runtime config that must not reach the model runtime. */
export function isAmbientRuntimeConfig(name: string): boolean {
return AMBIENT_RUNTIME_CONFIG_NAMES.has(name) || name === ".env" || name.startsWith(".env.");
}
/**
* Remove ambient runtime config from a THROWAWAY materialized tree, at every
* depth. Must only ever run on a tree ECR created and will delete (a worktree or
* extracted archive) — never on the user's checkout. Returns the repo-relative
* paths removed so callers can log them.
*/
export async function scrubAmbientRuntimeConfig(root: string): Promise<string[]> {
const removed: string[] = [];
const walk = async (dir: string): Promise<void> => {
const entries = await readdir(dir, { withFileTypes: true });
for (const entry of entries) {
const full = path.join(dir, entry.name);
if (isAmbientRuntimeConfig(entry.name)) {
await rm(full, { recursive: true, force: true });
removed.push(path.relative(root, full));
continue;
}
if (entry.isDirectory() && !SKIP_DIRS.has(entry.name)) {
await walk(full);
}
}
};
await walk(root);
return removed.sort();
}
+98
View File
@@ -0,0 +1,98 @@
import { open } from "node:fs/promises";
import os from "node:os";
import path from "node:path";
/**
* Rate-limit evidence, read from the OpenCode server's own log.
*
* Why this exists: provider throttling reaches us in two shapes. An EXPLICIT
* failure (HTTP 429 / "rate limit" stream error) surfaces in OpenCode's log as a
* structured ERROR line — that is proof. A SILENT one (the request is accepted
* and parked server-side) produces no error anywhere and is indistinguishable
* from a wedged request from the outside. So: the log watcher turns the explicit
* case into a hard signal, and the stall path treats "stall + recent explicit
* evidence" as throttling — the one situation where the right move is to WAIT
* (re-sending the whole context into a limited account only makes it worse).
*
* During oauth runs prepareAuth points XDG_DATA_HOME at an isolated temp dir, so
* the log we read belongs to exactly this run's server — no cross-talk with a
* developer's own OpenCode sessions.
*/
/** The OpenCode server's log file under the active data dir. */
export function opencodeLogFile(env: NodeJS.ProcessEnv = process.env): string {
const dataHome = env.XDG_DATA_HOME || path.join(os.homedir(), ".local", "share");
return path.join(dataHome, "opencode", "log", "opencode.log");
}
/**
* Provider throttle signatures in OpenCode log lines. Matched only against ERROR
* lines (a chatty INFO line mentioning "retry" must not count as evidence).
*/
const RATE_LIMIT_PATTERN = /\b429\b|rate.?limit|too many requests|quota exceeded/i;
const ERROR_LINE = /\blevel=ERROR\b/;
/** Count rate-limit ERROR lines in a chunk of log text. Pure, for tests. */
export function countRateLimitLines(chunk: string): number {
let count = 0;
for (const line of chunk.split("\n")) {
if (ERROR_LINE.test(line) && RATE_LIMIT_PATTERN.test(line)) {
count++;
}
}
return count;
}
/** How recent explicit evidence must be for a stall to be read as throttling. */
const EVIDENCE_WINDOW_MS = 5 * 60 * 1000;
/**
* Incremental watcher over the OpenCode server log. `check()` reads only what was
* appended since the last call (cheap enough for poll loops); `recentlyLimited()`
* is the signal the stall path consults. Fails soft everywhere: a missing or
* unreadable log yields "no evidence", never an error.
*/
export class RateLimitWatch {
/** Total rate-limit ERROR lines seen this run. */
events = 0;
/** Wall-clock time evidence was last SEEN (observation time, not log time). */
lastSeenAt = 0;
private offset = 0;
constructor(private readonly file: string = opencodeLogFile()) {}
/** Scan newly-appended log lines for rate-limit evidence. */
async check(): Promise<number> {
try {
const handle = await open(this.file, "r");
try {
const { size } = await handle.stat();
if (size < this.offset) {
this.offset = 0; // rotated/truncated — rescan from the top
}
if (size === this.offset) {
return this.events;
}
const length = size - this.offset;
const buffer = Buffer.alloc(length);
await handle.read(buffer, 0, length, this.offset);
this.offset = size;
const found = countRateLimitLines(buffer.toString("utf8"));
if (found > 0) {
this.events += found;
this.lastSeenAt = Date.now();
}
} finally {
await handle.close();
}
} catch {
// No log yet (server just started) or unreadable — no evidence, no error.
}
return this.events;
}
/** True when explicit throttle evidence appeared within the recency window. */
recentlyLimited(now: number = Date.now()): boolean {
return this.lastSeenAt > 0 && now - this.lastSeenAt < EVIDENCE_WINDOW_MS;
}
}
+6 -1
View File
@@ -19,6 +19,8 @@ export interface VerificationResult {
dropped: Array<{ finding: Finding; reason: string }>;
cost: number;
tokens: TokenUsage;
/** provider/model that actually answered the verify calls (see PromptResult.model). */
model?: string;
}
/**
@@ -103,6 +105,7 @@ export async function verifyFindings(
): Promise<VerificationResult> {
const dropped: Array<{ finding: Finding; reason: string }> = [];
let cost = 0;
let model: string | undefined;
const tokens: TokenUsage = {};
// Phase 1 — deterministic quote-grounding for every finding.
@@ -129,6 +132,7 @@ export async function verifyFindings(
value,
cost: verifyCost,
tokens: verifyTokens,
model: verifyModel,
} = await promptAndParse(
handle,
{
@@ -143,6 +147,7 @@ export async function verifyFindings(
);
cost += verifyCost;
addTokenUsage(tokens, verifyTokens);
model = verifyModel ?? model;
if (value.verified) {
verdicts.set(finding, "keep");
} else {
@@ -164,5 +169,5 @@ export async function verifyFindings(
// Preserve original order.
const kept = findings.filter((finding) => verdicts.get(finding) === "keep");
return { kept, dropped, cost, tokens };
return { kept, dropped, cost, tokens, model };
}
+114 -19
View File
@@ -4,6 +4,7 @@ import path from "node:path";
import { run } from "../core/exec.js";
import { parseUnifiedDiff } from "../core/diff.js";
import { scrubAmbientRuntimeConfig } from "../core/scrub.js";
import type { DiffEntry, ReviewMetadata } from "../core/schema.js";
import type { PreparedReadRoot, ReviewSource } from "./source.js";
@@ -14,6 +15,20 @@ export interface GitHubPRSourceOptions {
cwd?: string;
}
/** A full 40-hex-char commit OID — the only ref form passed to security-sensitive git calls. */
export function isCommitOid(value: string | undefined): value is string {
return typeof value === "string" && /^[0-9a-f]{40}$/i.test(value);
}
/**
* Append gh as a git credential helper for a single command. The token comes from
* GH_TOKEN via the credential-helper protocol — never argv, never `.git/config` —
* so a base-SHA checkout with `persist-credentials: false` (no extraheader) can
* still fetch a private repo. Appending (not replacing) keeps a local user's own
* helpers first, so developer machines behave exactly as before.
*/
const GH_CREDENTIAL_HELPER_ARGS = ["-c", "credential.helper=!gh auth git-credential"];
/**
* Pulls PR diff + metadata through the `gh` CLI, which is preinstalled and
* authenticated on GitHub Actions runners via GH_TOKEN.
@@ -21,11 +36,22 @@ export interface GitHubPRSourceOptions {
export class GitHubPRSource implements ReviewSource {
constructor(private readonly options: GitHubPRSourceOptions) {}
private metadataPromise: Promise<ReviewMetadata> | undefined;
private repoArgs(): string[] {
return this.options.repo ? ["--repo", this.options.repo] : [];
}
async getMetadata(): Promise<ReviewMetadata> {
/**
* Memoized internally (not just by memoizeSource) because the materialization
* paths below need the immutable OIDs and must not depend on the caller having
* called getMetadata() first.
*/
getMetadata(): Promise<ReviewMetadata> {
return (this.metadataPromise ??= this.fetchMetadata());
}
private async fetchMetadata(): Promise<ReviewMetadata> {
const { stdout } = await run(
"gh",
[
@@ -34,7 +60,10 @@ export class GitHubPRSource implements ReviewSource {
String(this.options.prNumber),
...this.repoArgs(),
"--json",
"title,body,baseRefName,headRefName",
// baseRefOid/headRefOid are the immutable commit OIDs backing this PR at
// this moment; every materialization below pins to them so a rename,
// force-push, or deleted head between API calls can't swap a tree.
"title,body,baseRefName,headRefName,baseRefOid,headRefOid",
],
{ cwd: this.options.cwd },
);
@@ -43,12 +72,16 @@ export class GitHubPRSource implements ReviewSource {
body?: string;
baseRefName?: string;
headRefName?: string;
baseRefOid?: string;
headRefOid?: string;
};
return {
title: parsed.title ?? "",
body: parsed.body ?? "",
baseRef: parsed.baseRefName ?? "",
headRef: parsed.headRefName ?? "",
baseOid: isCommitOid(parsed.baseRefOid) ? parsed.baseRefOid.toLowerCase() : undefined,
headOid: isCommitOid(parsed.headRefOid) ? parsed.headRefOid.toLowerCase() : undefined,
};
}
@@ -62,27 +95,32 @@ export class GitHubPRSource implements ReviewSource {
}
/**
* Check the PR HEAD out into a throwaway git worktree so the agents and verifier
* read the PR's versions of files (not whatever branch happens to be checked out).
* Fetches the head from the repo's own URL — `refs/pull/<n>/head`, which the base
* repo hosts even for fork PRs — so it's always the correct PR, independent of the
* local `origin`. Fails SOFT: any problem (not a git repo, fetch/worktree error)
* returns null, and the review falls back to reading the current checkout.
* Fetch `ref` from the repo's own HTTPS URL and materialize `oid` as a detached
* throwaway worktree under a fresh temp dir. Never falls back to a branch name:
* `oid` is validated as a full commit hash before reaching git.
*/
async prepareReadRootAsync(): Promise<PreparedReadRoot | null> {
const cwd = this.options.cwd;
private async materializeWorktreeAsync(ref: string, oid: string): Promise<PreparedReadRoot> {
if (!this.options.repo) {
// Without an explicit owner/repo we can't build the fetch URL safely.
return null;
throw new Error("cannot materialize a PR tree without an explicit owner/repo");
}
if (!isCommitOid(oid)) {
throw new Error(`refusing to materialize a non-OID ref: "${oid}"`);
}
const cwd = this.options.cwd;
const url = `https://github.com/${this.options.repo}.git`;
const ref = `refs/pull/${this.options.prNumber}/head`;
let parent: string | undefined;
try {
await run("git", ["fetch", "--no-tags", "--depth=1", url, ref], { cwd });
parent = await mkdtemp(path.join(tmpdir(), "ecr-prhead-"));
const dir = path.join(parent, "head"); // must not pre-exist for `worktree add`
await run("git", ["worktree", "add", "--detach", dir, "FETCH_HEAD"], { cwd });
await run(
"git",
[...GH_CREDENTIAL_HELPER_ARGS, "fetch", "--no-tags", "--depth=1", url, ref],
{ cwd },
);
parent = await mkdtemp(path.join(tmpdir(), "ecr-tree-"));
const dir = path.join(parent, "tree"); // must not pre-exist for `worktree add`
// Check out the OID (not FETCH_HEAD): if the ref moved between the API call
// and this fetch, the OID is absent and this fails instead of silently
// materializing a different tree than the one the diff was fetched for.
await run("git", ["worktree", "add", "--detach", dir, oid], { cwd });
const removeParent = parent;
return {
dir,
@@ -95,11 +133,68 @@ export class GitHubPRSource implements ReviewSource {
await rm(removeParent, { recursive: true, force: true });
},
};
} catch {
} catch (error) {
if (parent) {
await rm(parent, { recursive: true, force: true }).catch(() => {});
}
return null;
throw error;
}
}
/**
* Check the PR HEAD out into a throwaway worktree so the agents and verifier
* read the PR's versions of files (not whatever branch happens to be checked
* out), pinned to the immutable head OID. The fetch uses `refs/pull/<n>/head`,
* which the base repo hosts even for fork PRs.
*
* The worktree is SCRUBBED of ambient runtime config (opencode.json, .opencode
* plugins, AGENTS.md/CLAUDE.md, .mcp.json, .env*, …) before it's returned: the
* OpenCode server is started with this directory as its project root, and
* anything it discovers there is attacker-controlled PR content executing or
* injecting inside a process that holds the model credential and GH_TOKEN.
*
* Returns null only when no owner/repo is configured (a local `--pr` run
* without --repo, where the current checkout is an acceptable read root).
* Materialization FAILURES throw — the caller decides per mode whether that is
* fatal (CI: fail closed) or a soft fallback (local: the user's own checkout).
*/
async prepareReadRootAsync(): Promise<PreparedReadRoot | null> {
if (!this.options.repo) {
// Without an explicit owner/repo we can't build the fetch URL safely.
return null;
}
const metadata = await this.getMetadata();
if (!isCommitOid(metadata.headOid)) {
throw new Error("GitHub did not report an immutable head OID for this PR");
}
const root = await this.materializeWorktreeAsync(
`refs/pull/${this.options.prNumber}/head`,
metadata.headOid,
);
try {
await scrubAmbientRuntimeConfig(root.dir);
} catch (error) {
// A half-scrubbed tree must never become the runtime's project root.
await root.cleanup().catch(() => {});
throw error;
}
return root;
}
/**
* Materialize the PR's BASE commit as the trusted configuration root: review
* policy, prompts, routing, and auth mapping load from here, so a PR cannot
* change the reviewer that evaluates it (config changes activate on merge).
* Failures throw — `ecr ci` must fail closed, never fall back to the checkout.
*/
async prepareTrustedConfigRootAsync(): Promise<PreparedReadRoot> {
const metadata = await this.getMetadata();
if (!isCommitOid(metadata.baseOid)) {
throw new Error("GitHub did not report an immutable base OID for this PR");
}
// The base OID is fetchable by SHA (it's the tip of the base branch as of the
// API call; GitHub serves reachable SHAs — the same mechanism actions/checkout
// uses for `ref:` pins).
return this.materializeWorktreeAsync(metadata.baseOid, metadata.baseOid);
}
}
+5
View File
@@ -17,6 +17,11 @@ export interface ReviewSource {
* agents also read surrounding source and the verifier re-reads cited files — and
* those must be the PR-HEAD versions, not whatever happens to be checked out.
* Return `null` (or omit the method) to just review the current working directory.
*
* Contract: `null` means "no tree to materialize" (a deliberate, safe
* fall-through to the current checkout). A materialization FAILURE throws —
* the review core fails closed on it in CI mode and falls back softly only in
* local mode, where the user's own checkout is an acceptable read root.
*/
prepareReadRootAsync?(): Promise<PreparedReadRoot | null>;
}
+4 -4
View File
@@ -2,10 +2,10 @@
description: Security and secrets. Injection, credential or secret leakage, unsafe shell/child-process use, missing validation at trust boundaries.
alwaysRun: true
# Security is the highest-stakes agent and benefits most from stronger threat-model
# reasoning, so it runs on Opus even though the other specialists use the default
# model. Scoped to this one agent to limit the extra latency/rate-limit cost;
# subdivide-on-timeout + the per-fetch deadline keep a slow Opus pass from hanging.
model: anthropic/claude-opus-4-8
# reasoning, so it runs on the pro tier even though the other specialists use the
# default model. Scoped to this one agent to limit the extra latency/rate-limit cost;
# subdivide-on-timeout + the per-fetch deadline keep a slow pro pass from hanging.
model: openai/gpt-5.5-pro
---
# Security & secrets
+18 -9
View File
@@ -36,8 +36,10 @@ jobs:
startsWith(github.event.comment.body, '/review') &&
contains(fromJson('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association)
runs-on: ubuntu-latest
# Bound the run so a slow/stalled review fails fast rather than hanging.
timeout-minutes: 50
# Bound the run so a slow/stalled review fails fast rather than hanging. Keep it
# above the passes budget (budget.totalPassesMinutes, 55m) + coordinator (10m) +
# verification + setup, like the auto-review workflow's cap.
timeout-minutes: 90
# A reviewer failure must never fail the PR's checks.
continue-on-error: true
steps:
@@ -83,12 +85,18 @@ jobs:
# config, and never `gh pr checkout` the PR head. The reviewer engine itself
# is the PUBLISHED @expo/code-review-cli (fetched by npx), not built from any
# checkout, so attacker-controlled PR code never runs here. The diff + PR
# metadata come from the API (`gh pr diff`/`gh pr view`).
# metadata come from the API (`gh pr diff`/`gh pr view`); `ecr ci` loads
# configuration from the PR's immutable base commit and reads source from a
# head worktree scrubbed of ambient runtime config (opencode.json, plugins,
# AGENTS.md, .env, …) — the same trust model as the pull_request workflow.
- name: Checkout (base ref only — never the PR head)
if: steps.cmd.outputs.run == 'true'
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
with:
fetch-depth: 1
# The CLI's own git fetches authenticate through `gh` from GH_TOKEN, so
# the token never lands in .git/config.
persist-credentials: false
- name: Set up Node
if: steps.cmd.outputs.run == 'true'
@@ -113,7 +121,8 @@ jobs:
- name: Guard config tokenEnv (root + routing + all scopes)
if: steps.cmd.outputs.run == 'true'
env:
ECR_EXPECTED_TOKEN_ENV: ${{ vars.ECR_EXPECTED_TOKEN_ENV || 'ANTHROPIC_OAUTH_API_KEY' }}
# (Comma-separated set for a multi-credential auth.providers config.)
ECR_EXPECTED_TOKEN_ENV: ${{ vars.ECR_EXPECTED_TOKEN_ENV || 'OPENAI_API_KEY' }}
run: npx --yes -p "@expo/code-review-cli@$ECR_VERSION" ecr verify-config
- name: Run AI review
@@ -123,11 +132,11 @@ jobs:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
# Layer-1 auth lock: the CLI refuses to run when the tokenEnv it would honor
# differs from this. Keep it in sync with the guard's EXPECTED.
ECR_EXPECTED_TOKEN_ENV: ${{ vars.ECR_EXPECTED_TOKEN_ENV || 'ANTHROPIC_OAUTH_API_KEY' }}
# Claude Pro/Max OAuth token (from `claude setup-token`) — the env var named
# by auth.tokenEnv in config.jsonc. Store it as a repo secret. (For an API
# key instead, set auth.mode "api-key" and pass that key here.)
ANTHROPIC_OAUTH_API_KEY: ${{ secrets.ANTHROPIC_OAUTH_API_KEY }}
ECR_EXPECTED_TOKEN_ENV: ${{ vars.ECR_EXPECTED_TOKEN_ENV || 'OPENAI_API_KEY' }}
# OpenAI API key — the env var named by auth.tokenEnv in config.jsonc.
# Store it as a repo secret; a project-scoped key restricted to model
# inference (with a spend limit) is all the reviewer needs.
OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}
# Optional: override the model for every agent (uses your OpenCode login).
REVIEWER_MODEL: ${{ vars.REVIEWER_MODEL }}
AGENTS: ${{ steps.cmd.outputs.agents }}
+30 -13
View File
@@ -1,14 +1,14 @@
{
// Default model for every agent. Override per-agent via frontmatter in the
// agent's markdown, or at runtime with the REVIEWER_MODEL env var
// (e.g. REVIEWER_MODEL=openai/gpt-5.4-mini-fast).
"model": "anthropic/claude-sonnet-5",
// (e.g. REVIEWER_MODEL=openai/gpt-5.4-mini).
"model": "openai/gpt-5.5",
// Agents: every markdown file in agents/ is one reviewer (id = filename).
// 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-4-1\n---`.
// e.g. `---\nmodel: openai/gpt-5.5-pro\n---`.
"policy": {
// Phase 1: keep signal high by surfacing only critical/warning.
@@ -23,6 +23,9 @@
// Large diffs are split into focused chunks by changed-line count, plus a
// cross-cutting pass for multi-file issues. Diffs under maxChangedLines are one
// full-context pass. Defaults shown; raise/lower per your model + PR sizes.
// Concurrency defaults by auth mode: 6 with an API key, 3 on a subscription
// (oauth) credential — one account handles many parallel streams poorly, and
// several PRs may review on the same credential at once. Set it to override.
// "chunk": { "maxChangedLines": 1000, "maxFiles": 20, "concurrency": 4 },
// Which PRs `ecr ci` reviews. This is the source of truth for trigger policy;
@@ -41,16 +44,30 @@
// HTML marker used to find + update the single PR comment. Keep it stable.
"commentTag": "expo-ai-code-reviewer",
// How model credentials are provided. Default: a Claude Pro/Max subscription.
// "oauth": tokenEnv holds a Claude Pro/Max OAuth token (from
// `claude setup-token`); it's injected as a bearer credential.
// "api-key": tokenEnv holds a provider API key (sent as x-api-key). If you
// omit `auth`, OpenCode's own login / ANTHROPIC_API_KEY is used.
// For OpenAI/GPT or another provider, omit `auth` and set REVIEWER_MODEL after an
// `opencode auth login` for that provider.
// How model credentials are provided. Default: an OpenAI API key.
// "api-key": tokenEnv names the env var holding a provider API key. In CI,
// store the key as a repo secret and pass it under that env var
// (the scaffolded workflow does). If you omit `auth` entirely,
// OpenCode's own login / ambient provider env vars are used.
// For Anthropic/Claude, set provider "anthropic", tokenEnv "ANTHROPIC_API_KEY",
// and an anthropic/... model above. For another provider, omit `auth` and set
// REVIEWER_MODEL after an `opencode auth login` for that provider.
//
// MIXED setup (a ChatGPT/Codex subscription for the default models, plus a
// metered API key for pro-tier models the subscription doesn't offer): use the
// per-provider map instead, reference `openai-api/...` models in the frontmatter
// of the agents that need the pro tier, and set ECR_EXPECTED_TOKEN_ENV in the
// workflow to the comma-separated set of both env names.
// "auth": { "providers": {
// "openai": { "mode": "oauth", "tokenEnv": "CODEX_OAUTH_ACCESS_TOKEN" },
// "openai-api": { "mode": "api-key", "tokenEnv": "OPENAI_API_KEY", "upstream": "openai" }
// } }
// (openai oauth: tokenEnv holds the ACCESS token from an `opencode auth login`
// ChatGPT sign-in — `ecr setup-auth` extracts it. NEVER share the refresh
// token: it is single-use and dies on first rotation.)
"auth": {
"mode": "oauth",
"provider": "anthropic",
"tokenEnv": "ANTHROPIC_OAUTH_API_KEY"
"mode": "api-key",
"provider": "openai",
"tokenEnv": "OPENAI_API_KEY"
}
}
+3 -3
View File
@@ -1,9 +1,9 @@
---
# The coordinator makes the final call — de-duping, re-judging severity, and
# deciding — so it runs on Opus: consolidation quality matters more here than the
# small serial-tail latency it adds (no repo tools, so it's a single bounded pass).
# deciding — so it runs on the pro tier: consolidation quality matters more here
# than the small serial-tail latency it adds (no repo tools, one bounded pass).
# Override with a cheaper model if you'd rather trade decision quality for latency.
model: anthropic/claude-opus-4-8
model: openai/gpt-5.5-pro
---
# Coordinator — consolidation & decision
+3
View File
@@ -83,6 +83,9 @@ jobs:
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
with:
fetch-depth: 1
# Dismiss never fetches; gh authenticates from GH_TOKEN. Keep the token
# out of .git/config.
persist-credentials: false
- name: Set up Node
if: steps.cmd.outputs.run == 'true'
+1 -1
View File
@@ -18,7 +18,7 @@
// Passes budget, split across active scopes (they run sequentially in one `ecr ci`):
// keep totalPassesMinutes inside the workflow's timeout-minutes; minScopeMinutes is
// the floor below which a scope review isn't worth starting. Defaults shown.
// "budget": { "totalPassesMinutes": 32, "minScopeMinutes": 5 },
// "budget": { "totalPassesMinutes": 55, "minScopeMinutes": 5 },
// Ordered; the LAST matching scope wins per changed file. Keep a '**/*' catch-all first.
"scopes": [
+30 -13
View File
@@ -30,15 +30,30 @@ jobs:
# the line below with, e.g.:
# if: contains(github.event.pull_request.labels.*.name, 'ai-review')
if: ${{ !contains(github.event.pull_request.labels.*.name, 'ai-review:skip') }}
# Backstop so a stalled review fails fast instead of hanging.
timeout-minutes: 60
# Backstop so a stalled review fails fast instead of hanging. This is the ONE cap
# with no soft landing (GitHub hard-kills the job and nothing is posted), so keep
# margin over the worst-case internal chain: the passes budget
# (budget.totalPassesMinutes, 55m — the cross-file pass expands to fill it) +
# coordinator (10m) + verification + CI setup.
timeout-minutes: 90
# A reviewer failure must never fail the PR's checks.
continue-on-error: true
steps:
# SECURITY: check out the PR's immutable BASE commit, never the PR head or
# merge ref. Everything security-sensitive on this runner (`ecr verify-config`'s
# sweep, and any ambient files) therefore comes from a commit that already
# merged. `ecr ci` additionally enforces this itself: it materializes the base
# commit via the GitHub API for configuration and the head commit (scrubbed of
# runtime config) for source reads, so this checkout is defense in depth, not
# the only line. persist-credentials off — the CLI's own git fetches
# authenticate through `gh` from GH_TOKEN, so the token never lands in
# .git/config.
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
with:
ref: ${{ github.event.pull_request.base.sha }}
# Shallow is enough — the reviewer gets the diff from the API (`gh`).
fetch-depth: 1
persist-credentials: false
- uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0
with:
@@ -48,16 +63,17 @@ jobs:
# to save an empty cache and error).
package-manager-cache: false
# SECURITY: this workflow checks out the PR's code, including every
# SECURITY: the TRUSTED BASE checkout above includes every
# .expo-code-review/config.jsonc + routing.jsonc, whose auth.tokenEnv names the
# env var the CLI forwards as the model credential. The canonical guard ships
# with the CLI: `ecr verify-config` sweeps every config (root + routing + all
# scopes, referenced or not) with the engine's real JSONC parser and refuses
# unless tokenEnv appears exactly once, in a ROOT-owned file, equal to the
# expected value (repo var ECR_EXPECTED_TOKEN_ENV) — so a PR can't repoint it at
# another runner secret, sneak in a JSON-escaped key, or stage an unreferenced
# scope config with its own auth. This is layer 2; layer 1 is the runtime
# ECR_EXPECTED_TOKEN_ENV lock in `ecr ci` itself, so guard/loader drift fails safe.
# expected value (repo var ECR_EXPECTED_TOKEN_ENV) — so a config change can't
# repoint it at another runner secret, sneak in a JSON-escaped key, or stage an
# unreferenced scope config with its own auth. This is layer 2; layer 1 is the
# runtime ECR_EXPECTED_TOKEN_ENV lock in `ecr ci` itself, so guard/loader drift
# fails safe.
#
# This step MUST run BEFORE `ecr ci` (before any PR code is built or loaded).
# Only setup-node (runtime install) precedes it; running the PUBLISHED package
@@ -65,7 +81,8 @@ jobs:
# from the registry — it never builds or executes the PR's code.
- name: Guard config tokenEnv (root + routing + all scopes)
env:
ECR_EXPECTED_TOKEN_ENV: ${{ vars.ECR_EXPECTED_TOKEN_ENV || 'ANTHROPIC_OAUTH_API_KEY' }}
# (Comma-separated set for a multi-credential auth.providers config.)
ECR_EXPECTED_TOKEN_ENV: ${{ vars.ECR_EXPECTED_TOKEN_ENV || 'OPENAI_API_KEY' }}
run: npx --yes -p "@expo/code-review-cli@$ECR_VERSION" ecr verify-config
- name: Run AI review
@@ -79,11 +96,11 @@ jobs:
# honor (root config.jsonc, or routing.jsonc defaults.auth) differs from
# this — it catches what the guard step above can't. Keep it in sync
# with the guard.
ECR_EXPECTED_TOKEN_ENV: ${{ vars.ECR_EXPECTED_TOKEN_ENV || 'ANTHROPIC_OAUTH_API_KEY' }}
# Claude Pro/Max OAuth token (from `claude setup-token`) — the env var
# named by auth.tokenEnv in config.jsonc. Store it as a repo secret.
# (For an API key instead, set auth.mode "api-key" and pass that key here.)
ANTHROPIC_OAUTH_API_KEY: ${{ secrets.ANTHROPIC_OAUTH_API_KEY }}
ECR_EXPECTED_TOKEN_ENV: ${{ vars.ECR_EXPECTED_TOKEN_ENV || 'OPENAI_API_KEY' }}
# OpenAI API key — the env var named by auth.tokenEnv in config.jsonc.
# Store it as a repo secret; a project-scoped key restricted to model
# inference (with a spend limit) is all the reviewer needs.
OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}
# Optional: override the model for every agent (uses your OpenCode login).
REVIEWER_MODEL: ${{ vars.REVIEWER_MODEL }}