From f32b82a9f014e909f8bb57adb4e4b876ed84c197 Mon Sep 17 00:00:00 2001 From: Trevin Chow Date: Fri, 14 Aug 2026 16:51:03 -0700 Subject: [PATCH 1/6] fix(ce-code-review): cover adversarial after quota or auth no-review A started peer that dies on session quota or execution-context auth no longer keeps exclusive ownership of the adversarial lens. Fold-in announces the next eligible peer or restores the local reviewer. --- docs/skills/ce-code-review.md | 2 +- skills/ce-code-review/SKILL.md | 8 ++--- .../references/cross-model-review.md | 14 ++++++-- .../references/dispatch-reviewers.md | 4 +-- .../references/persona-catalog.md | 2 +- .../scripts/cross-model-adversarial-review.sh | 35 +++++++++++++++---- tests/review-skill-contract.test.ts | 33 ++++++++++++++++- .../ce-code-review-cross-model-routes.test.ts | 35 +++++++++++++++++++ 8 files changed, 115 insertions(+), 18 deletions(-) diff --git a/docs/skills/ce-code-review.md b/docs/skills/ce-code-review.md index abec72ed1..2a91b2096 100644 --- a/docs/skills/ce-code-review.md +++ b/docs/skills/ce-code-review.md @@ -112,7 +112,7 @@ When you pass a PR number or URL, trivial automated PRs (lockfile bumps, chore v ### Cross-model adversarial pass -When adversarial is selected and the working tree is the reviewed head (current branch, or a PR whose local tree already matches the PR head), the adversarial lens runs through **one different model provider than the host** in a separate read-only process. A started peer **replaces** the in-process `adversarial` persona. They never both receive the same brief. The in-process persona runs only if the peer cannot start. Remote PR or branch diffs stay on the in-process persona, because that reviewer can inspect the fetched refs. +When adversarial is selected and the working tree is the reviewed head (current branch, or a PR whose local tree already matches the PR head), the adversarial lens runs through **one different model provider than the host** in a separate read-only process. A started peer **replaces** the in-process `adversarial` persona. They never both receive the same brief. The in-process persona runs if the peer cannot start, or if the started peer returns only session-quota or auth-context failure — in that case the next announced different-family peer is tried when one is eligible, otherwise the local persona covers the lens. Remote PR or branch diffs stay on the in-process persona, because that reviewer can inspect the fetched refs. Agreement between the peer and another in-process reviewer is a strong promotion signal in synthesis. diff --git a/skills/ce-code-review/SKILL.md b/skills/ce-code-review/SKILL.md index 230830944..f82882076 100644 --- a/skills/ce-code-review/SKILL.md +++ b/skills/ce-code-review/SKILL.md @@ -48,7 +48,7 @@ Follow these boundaries in order; references supply the detail but never change 1. Resolve the reviewed diff and intent. 2. Read `references/persona-catalog.md`, then select the risk-driven reviewer roster and discover applicable standards paths. Do not select or dispatch personas without that catalog load. -3. When adversarial is selected for a local reviewed tree, start and persist the sanctioned cross-model job **before any local persona dispatch**. Invoking this skill authorizes its configured/allowlisted peer route after the required recipient-and-code-egress disclosure; do not ask for a second confirmation or skip merely because the user did not separately repeat that authorization. An explicit user prohibition on external review still wins. A started peer replaces the local adversarial persona; only an actual scope, allowlist, availability, authentication, or start failure leaves the local fallback. +3. When adversarial is selected for a local reviewed tree, start and persist the sanctioned cross-model job **before any local persona dispatch**. Invoking this skill authorizes its configured/allowlisted peer route after the required recipient-and-code-egress disclosure; do not ask for a second confirmation or skip merely because the user did not separately repeat that authorization. An explicit user prohibition on external review still wins. A started peer replaces the local adversarial persona; only an actual scope, allowlist, availability, authentication, or start failure leaves the local fallback at Stage 3d. Fold-in may still run the did-not-run fallback in `references/cross-model-review.md` when the started peer returns only `session_quota` or `execution_context_auth`. 4. Before any local dispatch, read `references/dispatch-reviewers.md`; if it is not loaded, stop and load it. Then dispatch the materialized local roster as a foreground concurrent batch sized to the host's active-agent cap — spawn multiple reviewers in one message with background execution off where the harness runs same-message calls concurrently, and collect every reviewer before synthesis (one blocking wait on Claude-style harnesses; repeated non-polling collection waits on async `spawn_agent` harnesses); degrade to serial where it does not. Detaching local review into a polled background job is forbidden; the cross-model peer is the only detached work and overlaps with this batch. Shell no-ops and wakeup polling are forbidden. 5. After the reviewer returns are ready, read `references/finish-review.md`; if it is not loaded, stop and load it. Fold in the peer once, run the documented findings mechanics, run every validator the reference selects, and only then return the report. Never synthesize directly from raw reviewer artifacts. The exact Actionable Findings, Coverage, and Verdict completion fields are required. When a peer ran, Coverage must record its route plus the literal keyed fields `model_requested`, `model_actual`, `effort_requested`, `effort_actual`, `receipt_supported`, and `independence_verified` from the artifact; never shorten that tuple to a model family or vague "high reasoning" claim. In the multi-agent path, emit only this skill's report; do not also invoke a harness-native findings/reporting tool. The native review tool belongs only to the explicit Quick Review Short-Circuit. Bare and `mode:agent` reviews never apply fixes; only explicit `apply:local` can enter the apply stage. @@ -175,7 +175,7 @@ Reviewer personas are selected in layers. The persona catalog in `references/per - `api-contract-reviewer` — routes, serializers, type signatures, versioning - `data-migration-reviewer` — migration files / schema dumps / backfills (see spawn gate in Stage 3) - `reliability-reviewer` — error handling, retries, timeouts, background jobs -- `adversarial-reviewer` lens — >=50 changed code lines, or auth / payments / persistence writes / event publication / retry or concurrency semantics / external APIs, or a **silent-pass verification mechanism** regardless of size. Satisfy this lens with the independent cross-model adversarial pass when a sanctioned peer job starts successfully. Dispatch the in-process `adversarial-reviewer` only as the fallback when the peer cannot start; do not run both same-brief reviews. +- `adversarial-reviewer` lens — >=50 changed code lines, or auth / payments / persistence writes / event publication / retry or concurrency semantics / external APIs, or a **silent-pass verification mechanism** regardless of size. Satisfy this lens with the independent cross-model adversarial pass when a sanctioned peer job starts successfully. Dispatch the in-process `adversarial-reviewer` when the peer cannot start, or when fold-in runs the did-not-run fallback; do not run both same-brief reviews. - `previous-comments-reviewer` — PR with existing review comments (PR-only, comment-gated) **Stack-specific conditional (per diff):** `julik-frontend-races-reviewer` (Stimulus/Turbo, DOM events, async UI) and `swift-ios-reviewer` (Swift/SwiftUI/UIKit, entitlements, Core Data, `.pbxproj`). @@ -455,7 +455,7 @@ Distinguish an empty successful search from a failed or unavailable search: ### Stage 3d: Bind the adversarial route and final roster -Complete this stage **before reading persona prompt assets or entering Stage 4**. It owns the exclusive choice between a cross-model adversarial peer and the in-process `adversarial-reviewer`; later stages consume that choice and must not decide it again. +Complete this stage **before reading persona prompt assets or entering Stage 4**. It owns the exclusive choice between a cross-model adversarial peer and the in-process `adversarial-reviewer`; later stages consume that choice and must not decide it again, except the fold-in did-not-run fallback. Generate the review run ID now so both routes share one artifact directory: @@ -473,7 +473,7 @@ echo "$RUN_DIR"; When adversarial was selected and scope is `local-aligned` or standalone, read `references/cross-model-review.md` from this skill's directory in full, attest the host, resolve and sanction one fixed route, and make its required egress announcement. Before start, write the reference's compact orchestrator-owned adversarial review brief to the run directory: intent plus the material risk divisions inferred from the current file inventory and diff, without embedding the diff or mechanically copying every path. Then start the detached peer job using the reference's exact invocation and persist its job ID, target, requested model/reasoning, and start epoch in working state. -- If the runner returns a job ID, the peer owns the adversarial lens for this run. Remove `adversarial-reviewer` from the local roster immediately. Do not read its local persona asset or dispatch it later, even if the peer eventually fails. +- If the runner returns a job ID, the peer owns the adversarial lens for this run. Remove `adversarial-reviewer` from the local roster immediately. Do not read its local persona asset or dispatch it later — except the fold-in did-not-run fallback in `references/cross-model-review.md` when skip class is `session_quota` or `execution_context_auth`. - If no job starts because of a dispatch-infrastructure failure (a non-zero exit before any job id, an unresolved `$SKILL_DIR`/script path), first attempt the bounded same-route hand recovery from `references/cross-model-review.md` before accepting the fallback: re-run the identical resolved route, holding target/model and read scope fixed, while each failure is a new plausibly recoverable one and the shared peer deadline holds. If recovery returns a job id, treat it as the branch above (the peer owns the lens; remove `adversarial-reviewer`). Only when recovery is exhausted — a failure repeats or the deadline is spent — or the peer was never eligible to start (gate not met, host un-attestable, no different provider, CLI missing/unauthed), keep `adversarial-reviewer` in the local roster as the fallback and record the peer skip reason for Coverage. - In `pr-remote` / `branch-remote`, do not start the peer; keep the selected in-process adversarial reviewer because it can inspect the reviewed refs. diff --git a/skills/ce-code-review/references/cross-model-review.md b/skills/ce-code-review/references/cross-model-review.md index 5773b5caa..3956249ae 100644 --- a/skills/ce-code-review/references/cross-model-review.md +++ b/skills/ce-code-review/references/cross-model-review.md @@ -154,10 +154,18 @@ Repeat that call until the job is terminal or the derived deadline is spent; do - In final Coverage, name `cross_model_route`, `model_requested`, `effort_requested`, `receipt_supported`, `model_actual`, `effort_actual`, and `independence_verified` from the artifact. Keep the literal `unverified`; never compress a request into a serving claim such as "via Codex high" when actual model or effort is unverified. - **Never started / not run** — the job was never started (gates not met, host un-attestable, no different provider reachable, CLI missing/unauthed): the pass simply didn't run. Note "cross-model pass: not run" in Coverage for human-facing markdown; stay silent in `mode:agent`. Ignore any `*.raw.json` leftovers — they are not fold-in artifacts. - **Dispatch-infrastructure failure** — the runner or worker itself crashed: a non-zero exit before any job starts, a preflight/detach failure, or an unresolved `$SKILL_DIR`/script path. This is distinct from the gate-not-met skips above (there, no dispatch was attempted), so do not fold it into the silent not-run bucket on the first error. The two failure shapes recover at different points. A **no-job-id** preflight failure (exit before any job id, unresolved `$SKILL_DIR`) is recovered entirely at **Stage 3d's no-job branch**, before the local roster is materialized — the only point where re-running the start can still recover cross-model corroboration and, failing that, cleanly fall to the in-process reviewer (which then covers the lens; only corroboration is lost). Do **not** re-attempt that case here at fold-in: Stage 4 may already have dispatched the in-process `adversarial-reviewer`, so a fold-in peer re-run would put both on the same brief and violate the exclusive routing boundary. This step handles only the **job-id-returned-then-failed** crash — its failed job is reaped here and the in-process reviewer is already gone. For it, re-run the **same resolved fixed route** by hand — holding the target and model, the `git diff ` read scope, and the adversarial persona brief fixed — while each failure is a new, plausibly recoverable one and the shared peer deadline holds. This is a same-route retry, deliberately distinct from the quota rule below, which requires a newly disclosed route. Stop once a failure repeats or the deadline is spent; the hand recovery is then the adversarial lens's only cover, so the Coverage line must report the adversarial lens as **degraded**, not merely cross-model corroboration lost. A hand recovery may not substitute a different target or provider, widen the read scope, or relax the read-only trust boundary — those make the recovered peer untrustworthy, not merely unavailable. -- **Ran but produced no usable output** — the job reached `done` (or any terminal state) yet no `adversarial-.json` exists (the peer ran and egressed but returned nothing schema-shaped — unparseable output, empty findings the script dropped). Distinct from not-run: note "cross-model pass: peer ran, no usable output" in human-facing markdown Coverage. Never fail the review. -- **Started but not `done`** — the final status read reports `failed`, `timeout`, or `died-without-result` (a job reaped at the shared deadline records `timeout`, with the reap noted in its reason) → still non-blocking, but never silent: name the peer and its terminal state in Coverage (e.g. "cross-model adversarial peer: timeout"). Silent absence stays correct only for passes that never started or were skipped. +- **Ran but produced no usable output** — the job reached `done` (or any terminal state) yet no `adversarial-.json` exists (the peer ran and egressed but returned nothing schema-shaped — unparseable output, empty findings the script dropped). Distinct from not-run: note "cross-model pass: peer ran, no usable output" in human-facing markdown Coverage. Never fail the review. If `peer skip class:` is `session_quota` or `execution_context_auth`, this is not merely empty output — run the did-not-run fallback below before treating the lens as covered. +- **Started but not `done`** — the final status read reports `failed`, `timeout`, or `died-without-result` (a job reaped at the shared deadline records `timeout`, with the reap noted in its reason) → still non-blocking, but never silent: name the peer and its terminal state in Coverage (e.g. "cross-model adversarial peer: timeout"). Silent absence stays correct only for passes that never started or were skipped. If the failed job's skip class is `session_quota` or `execution_context_auth`, run the did-not-run fallback below rather than leaving the lens uncovered. - Empty `findings` → note "cross-model pass: no additional issues" in Coverage. -- **Classify the skip reason before deleting.** Read `out.log` before cleanup, including bounded lines prefixed `peer skip evidence:`, and name observed quota, authentication, or capability failure. An authentication-shaped peer failure (`not logged in`, `please log in`, 401, or CLI text prompting login) describes only the peer's execution context: a sandboxed host — e.g. a restricted Codex task denying spawned commands network or keychain access — produces the identical signal to a genuine account logout, so classify it as a cross-model execution-context authentication failure and never report it as the user's account being logged out or prompt the user to run a login command on that basis. The cross-model pass is additive and the local review still completed; obtaining it requires a context where the peer CLI can reach the network (for example, outside the restricted sandbox). After the same quota or usage-limit evidence appears more than once in this session, do not retry that route automatically. A retry uses a newly resolved, disclosed, and sanctioned fixed route; never silently continue to another recipient. +- **Classify the skip reason before deleting.** Read `out.log` before cleanup, including bounded lines prefixed `peer skip evidence:` and `peer skip class:`. Prefer the structured `peer skip class:` token when present. + - `execution_context_auth` (`not logged in`, `please log in`, 401, or a CLI login prompt) describes only the peer's execution context: a sandboxed host produces the same signal as a genuine logout, so never report it as the user's account being logged out or prompt a login command. + - `session_quota` (session/usage-limit, usage-exhausted, 402, or "hit your limit" with a reset) means this route cannot review now. Do not retry it. + - `transient_rate_limit` (plain 429 / rate-limit without session or usage language) may still recover on the same route. Do not run the did-not-run fallback. + - `other`: name the observed failure; do not run the did-not-run fallback. +- **Did-not-run fallback** (first `session_quota` or `execution_context_auth` observation, including the first one): the started job did not cover the adversarial lens. Do not retry that same route. Then: + 1. If the failed recipient was **not** an explicit user-stated preference (Step 1 item 1) and another attested-different installed+allowlisted target remains: announce that new recipient and start a new job with a new `CROSS_MODEL_FIXED_ROUTE`. That job owns the lens. Reuse the remaining shared deadline. Never switch recipients inside the worker. + 2. Otherwise (explicit recipient, or no other eligible peer): dispatch in-process `adversarial-reviewer` now. Coverage records the peer as not-run for quota/auth and that the lens used the local fallback. + A config or default selection is not an explicit user-stated preference. One replacement only; never silently continue to another recipient. - After fold-in (or after deadline reaping), delete the consumed job directory (`/jobs//`) — its log and result are review content and must not outlive their use. - A finding sharing a fingerprint with in-process `adversarial` promotes only when the artifact records `independence_verified: true`. Cursor-default artifacts default false; an unattested host skips automatic dispatch. diff --git a/skills/ce-code-review/references/dispatch-reviewers.md b/skills/ce-code-review/references/dispatch-reviewers.md index 1d6172b09..4ce62daa3 100644 --- a/skills/ce-code-review/references/dispatch-reviewers.md +++ b/skills/ce-code-review/references/dispatch-reviewers.md @@ -104,8 +104,8 @@ The artifact file **must** carry the full detail-tier fields (`why_it_matters`, #### Cross-model adversarial pass -Stage 3d already made the exclusive route choice and, when applicable, started the detached peer. Do not resolve, start, or substitute a route here. Dispatch only the materialized local roster. +Stage 3d already made the exclusive route choice and, when applicable, started the detached peer. Do not resolve, start, or substitute a route here except the did-not-run fallback in `references/cross-model-review.md`. Dispatch only the materialized local roster. -After the inline fast pass has completed and the local reviewer batch has started, prepare synthesis inputs while reviewers run. Do not poll the peer during that wave. After local reviewers finish, if Stage 3d persisted a peer job ID, perform the reference's single bounded status/wait/reap sequence and fold in whatever terminal artifact is available. Attribute from the artifact and clean up through the runner. A failure or timeout stays non-blocking and is named in Coverage; it never triggers a late in-process adversarial retry. Peer findings enter ordinary synthesis, but agreement promotion requires top-level `independence_verified: true`; false or absent independence is useful evidence, not different-model corroboration. Coverage must say whether the adversarial lens ran cross-model or used the in-process fallback. +After the inline fast pass has completed and the local reviewer batch has started, prepare synthesis inputs while reviewers run. Do not poll the peer during that wave. After local reviewers finish, if Stage 3d persisted a peer job ID, perform the reference's single bounded status/wait/reap sequence and fold in whatever terminal artifact is available. Attribute from the artifact and clean up through the runner. A failure or timeout stays non-blocking and is named in Coverage; it never triggers a late in-process adversarial retry, except the did-not-run fallback in `references/cross-model-review.md` for `session_quota` or `execution_context_auth`. Peer findings enter ordinary synthesis, but agreement promotion requires top-level `independence_verified: true`; false or absent independence is useful evidence, not different-model corroboration. Coverage must say whether the adversarial lens ran cross-model or used the in-process fallback. The peer return enters Stage 5 as reviewer `adversarial-`, like any persona artifact. A pass that never started is recorded as not run (or as the in-process fallback when selected); a started peer that fails, times out, dies, or is reaped is named with its terminal state rather than vanishing silently. diff --git a/skills/ce-code-review/references/persona-catalog.md b/skills/ce-code-review/references/persona-catalog.md index 9696f30d6..bb40103aa 100644 --- a/skills/ce-code-review/references/persona-catalog.md +++ b/skills/ce-code-review/references/persona-catalog.md @@ -35,7 +35,7 @@ Spawned when the orchestrator identifies relevant patterns in the diff. The orch | `api-contract` | `api-contract-reviewer` | An externally consumed boundary changes: route/request/response definitions, serializers, published event schemas, API versioning, or a public package signature with evidenced downstream callers. A new or changed exported symbol inside one module is insufficient by itself. | | `data-migration` | `data-migration-reviewer` | Migration files, schema dumps (`db/schema.rb`, `structure.sql`), backfill scripts, data transformations — **not** model/query-only changes without migration artifacts | | `reliability` | `reliability-reviewer` | Error handling, retry logic, circuit breakers, timeouts, background jobs, async handlers, health checks | -| `adversarial` | `adversarial-reviewer` fallback | >=50 changed code lines; auth/payments; persistence writes or event publication; retry/partial-failure or concurrency/ordering semantics; external APIs; or a silent-pass verification mechanism. The lens runs through the independent cross-model peer when a sanctioned peer starts. Spawn this in-process persona only when the peer cannot start. A started peer and the fallback are mutually exclusive. | +| `adversarial` | `adversarial-reviewer` fallback | >=50 changed code lines; auth/payments; persistence writes or event publication; retry/partial-failure or concurrency/ordering semantics; external APIs; or a silent-pass verification mechanism. The lens runs through the independent cross-model peer when a sanctioned peer starts. Spawn this in-process persona when the peer cannot start, or when fold-in runs the did-not-run fallback. A started peer that produced a review artifact and the fallback are mutually exclusive. | | `previous-comments` | `previous-comments-reviewer` | **PR-only AND comment-gated.** Reviewing a PR that has existing review comments or review threads from prior review rounds. Skip entirely when no PR metadata was gathered in Stage 1, OR when Stage 1's `hasPriorComments` flag is false (no `reviews` and no `comments` on the PR). | ## Stack-Specific Conditional (2 personas) diff --git a/skills/ce-code-review/scripts/cross-model-adversarial-review.sh b/skills/ce-code-review/scripts/cross-model-adversarial-review.sh index b292b730b..77b926ba4 100755 --- a/skills/ce-code-review/scripts/cross-model-adversarial-review.sh +++ b/skills/ce-code-review/scripts/cross-model-adversarial-review.sh @@ -912,24 +912,47 @@ run_provider() { log "provider $provider produced no usable schema-shaped output; skipping fold-in" # Surface bounded peer output so the orchestrator can # reason about WHY it was skipped (quota/usage-limit exhaustion vs an ordinary - # empty review) and, in a repeated-pass session, deprioritize an exhausted - # route. Harness-agnostic: the agent classifies from the text; this only makes - # the evidence visible in out.log. Surface BOTH streams -- the error can be on - # stdout (grok's 402) or stderr (claude/cursor auth/quota). Bash builtins only - # (the route sandbox has no tail/tr). Prefer structured error fields because - # a raw tail can discard the actionable message in a large CLI envelope. + # empty review). Prefer structured error fields because a raw tail can discard + # the actionable message in a large CLI envelope. Surface BOTH streams -- the + # error can be on stdout (grok's 402) or stderr (claude/cursor auth/quota). + # Then emit one structured class the fold-in fallback chain keys on. + _class_src="" if [ -s "$PEERLOG" ]; then _pt="$(bounded_failure_evidence "$PEERLOG")" log " peer skip evidence: $_pt" + _class_src="$_pt" fi if [ -s "$PEERERR" ]; then _pe="$(bounded_failure_evidence "$PEERERR")" log " peer skip evidence (stderr): $_pe" + _class_src="${_class_src} ${_pe}" fi + log " peer skip class: $(classify_skip_class "$_class_src")" rm -f "$OUT" "$RAW_OUT" fi } +# One token for fold-in. Auth before quota; session/usage language before a +# bare 429, which is only a transient rate limit. +classify_skip_class() { + local text + text="$(printf '%s' "${1:-}" | tr '[:upper:]' '[:lower:]')" + case "$text" in + *'not logged in'*|*'please log in'*|*'please run /login'*|*'api_error_status=401'*) + printf '%s' execution_context_auth + ;; + *'session limit'*|*'usage limit'*|*'usage-exhausted'*|*'usage exhausted'*|*'hit your limit'*|*'hit your session'*|*quota*|*'api_error_status=402'*) + printf '%s' session_quota + ;; + *'rate limit'*|*'api_error_status=429'*) + printf '%s' transient_rate_limit + ;; + *) + printf '%s' other + ;; + esac +} + # Prefer structured CLI diagnostics over a raw tail, which can hide the useful # error near the beginning of a large JSON envelope. bounded_failure_evidence() { # diff --git a/tests/review-skill-contract.test.ts b/tests/review-skill-contract.test.ts index 492f836d3..3494195c4 100644 --- a/tests/review-skill-contract.test.ts +++ b/tests/review-skill-contract.test.ts @@ -1217,10 +1217,41 @@ describe("cross-model peer skip legibility", () => { // to classify a quota/usage-limit exhaustion (harness-agnostic reasoning). expect(referenceSrc).toContain("peer skip evidence:") expect(referenceSrc).toMatch(/quota|usage-limit/i) - expect(referenceSrc).toMatch(/more than once in this session/i) + if (worker.includes("ce-code-review")) { + expect(workerSrc).toContain("peer skip class:") + expect(referenceSrc).toContain("peer skip class:") + expect(referenceSrc).toContain("session_quota") + expect(referenceSrc).toContain("execution_context_auth") + expect(referenceSrc).toMatch(/did-not-run fallback/i) + expect(referenceSrc).toMatch(/never silently continue to another recipient/i) + expect(referenceSrc).toMatch(/explicit user-stated preference/i) + expect(referenceSrc).toContain("in-process `adversarial-reviewer`") + } else { + expect(referenceSrc).toMatch(/more than once in this session/i) + } }) } + test("code review restores the adversarial lens after a quota or auth no-review", async () => { + const skill = await readRepoFile("skills/ce-code-review/SKILL.md") + const dispatch = await readRepoFile( + "skills/ce-code-review/references/dispatch-reviewers.md", + ) + const reference = await readRepoFile( + "skills/ce-code-review/references/cross-model-review.md", + ) + + expect(skill).toMatch(/did-not-run fallback/) + expect(dispatch).toMatch(/did-not-run fallback/) + expect(reference).toMatch( + /another attested-different installed\+allowlisted target remains/i, + ) + expect(reference).toMatch(/announce that new recipient and start a new job/i) + expect(reference).toMatch( + /Otherwise \(explicit recipient, or no other eligible peer\)/i, + ) + }) + // A restricted host sandbox (e.g. a Codex task with network disabled) denies // the spawned peer CLI network/keychain, producing the exact same // `Not logged in` signal as a genuine account logout. The classifier surfaces diff --git a/tests/skills/ce-code-review-cross-model-routes.test.ts b/tests/skills/ce-code-review-cross-model-routes.test.ts index 6dde25966..ed1937205 100644 --- a/tests/skills/ce-code-review-cross-model-routes.test.ts +++ b/tests/skills/ce-code-review-cross-model-routes.test.ts @@ -600,6 +600,41 @@ describe("cross-model-adversarial-review skip paths — non-blocking, no file", const r = run(["codex", "claude", "HEAD", runDir], runDir, env) expect(r.stderr).toContain("Not logged in") expect(r.stderr).toContain("terminal_reason=api_error") + expect(r.stderr).toContain("peer skip class: execution_context_auth") + }) + + test("classifies a Claude session-limit 429 as session_quota, not a completed review", () => { + const payload = JSON.stringify({ + result: "You have hit your session limit", + api_error_status: 429, + terminal_reason: "api_error", + }) + const { env } = sandbox( + ["claude"], + `#!/bin/sh\ncat >/dev/null\nprintf '%s' '${payload}'\nexit 1\n`, + ) + const runDir = makeRunDir() + const r = run(["codex", "claude", "HEAD", runDir], runDir, env) + expect(r.code).toBe(0) + expect(r.files).not.toContain("adversarial-claude.json") + expect(r.stderr).toContain("peer skip class: session_quota") + expect(r.stderr).not.toContain("peer skip class: transient_rate_limit") + }) + + test("classifies a plain 429 rate limit as transient_rate_limit", () => { + const payload = JSON.stringify({ + result: "Rate limit exceeded; retry shortly", + api_error_status: 429, + terminal_reason: "api_error", + }) + const { env } = sandbox( + ["claude"], + `#!/bin/sh\ncat >/dev/null\nprintf '%s' '${payload}'\nexit 1\n`, + ) + const runDir = makeRunDir() + const r = run(["codex", "claude", "HEAD", runDir], runDir, env) + expect(r.stderr).toContain("peer skip class: transient_rate_limit") + expect(r.stderr).not.toContain("peer skip class: session_quota") }) test("ancillary structured fields do not hide an unrecognized human-readable diagnostic", () => { From 9773f83a67b15df087ec067ed851d5a50f3d51df Mon Sep 17 00:00:00 2001 From: Trevin Chow Date: Fri, 14 Aug 2026 16:58:14 -0700 Subject: [PATCH 2/6] Address PR review feedback (#1380) - Classify bare HTTP 401/Unauthorized as execution-context auth - Treat weekly/Opus/hit-your-* limits as session quota - Wait and fold the one replacement peer; if it also dies, go local --- .../references/cross-model-review.md | 2 +- .../scripts/cross-model-adversarial-review.sh | 4 +-- tests/review-skill-contract.test.ts | 2 ++ .../ce-code-review-cross-model-routes.test.ts | 28 +++++++++++++++++++ 4 files changed, 33 insertions(+), 3 deletions(-) diff --git a/skills/ce-code-review/references/cross-model-review.md b/skills/ce-code-review/references/cross-model-review.md index 3956249ae..69c3fd182 100644 --- a/skills/ce-code-review/references/cross-model-review.md +++ b/skills/ce-code-review/references/cross-model-review.md @@ -163,7 +163,7 @@ Repeat that call until the job is terminal or the derived deadline is spent; do - `transient_rate_limit` (plain 429 / rate-limit without session or usage language) may still recover on the same route. Do not run the did-not-run fallback. - `other`: name the observed failure; do not run the did-not-run fallback. - **Did-not-run fallback** (first `session_quota` or `execution_context_auth` observation, including the first one): the started job did not cover the adversarial lens. Do not retry that same route. Then: - 1. If the failed recipient was **not** an explicit user-stated preference (Step 1 item 1) and another attested-different installed+allowlisted target remains: announce that new recipient and start a new job with a new `CROSS_MODEL_FIXED_ROUTE`. That job owns the lens. Reuse the remaining shared deadline. Never switch recipients inside the worker. + 1. If this is the first replacement attempt, the failed recipient was **not** an explicit user-stated preference (Step 1 item 1), and another attested-different installed+allowlisted target remains: announce that new recipient and start a new job with a new `CROSS_MODEL_FIXED_ROUTE`. Wait for it with the remaining shared deadline and fold its artifact the same way as the first job. That job owns the lens. Never switch recipients inside the worker. If this replacement also ends `session_quota` or `execution_context_auth`, do not start a third peer; take step 2. 2. Otherwise (explicit recipient, or no other eligible peer): dispatch in-process `adversarial-reviewer` now. Coverage records the peer as not-run for quota/auth and that the lens used the local fallback. A config or default selection is not an explicit user-stated preference. One replacement only; never silently continue to another recipient. - After fold-in (or after deadline reaping), delete the consumed job directory (`/jobs//`) — its log and result are review content and must not outlive their use. diff --git a/skills/ce-code-review/scripts/cross-model-adversarial-review.sh b/skills/ce-code-review/scripts/cross-model-adversarial-review.sh index 77b926ba4..3fa757603 100755 --- a/skills/ce-code-review/scripts/cross-model-adversarial-review.sh +++ b/skills/ce-code-review/scripts/cross-model-adversarial-review.sh @@ -938,10 +938,10 @@ classify_skip_class() { local text text="$(printf '%s' "${1:-}" | tr '[:upper:]' '[:lower:]')" case "$text" in - *'not logged in'*|*'please log in'*|*'please run /login'*|*'api_error_status=401'*) + *'not logged in'*|*'please log in'*|*'please run /login'*|*'api_error_status=401'*|*'http 401'*|*unauthorized*) printf '%s' execution_context_auth ;; - *'session limit'*|*'usage limit'*|*'usage-exhausted'*|*'usage exhausted'*|*'hit your limit'*|*'hit your session'*|*quota*|*'api_error_status=402'*) + *'session limit'*|*'usage limit'*|*'usage-exhausted'*|*'usage exhausted'*|*'hit your '*|*quota*|*'api_error_status=402'*) printf '%s' session_quota ;; *'rate limit'*|*'api_error_status=429'*) diff --git a/tests/review-skill-contract.test.ts b/tests/review-skill-contract.test.ts index 3494195c4..ca349f9d1 100644 --- a/tests/review-skill-contract.test.ts +++ b/tests/review-skill-contract.test.ts @@ -1247,6 +1247,8 @@ describe("cross-model peer skip legibility", () => { /another attested-different installed\+allowlisted target remains/i, ) expect(reference).toMatch(/announce that new recipient and start a new job/i) + expect(reference).toMatch(/Wait for it with the remaining shared deadline/i) + expect(reference).toMatch(/do not start a third peer/i) expect(reference).toMatch( /Otherwise \(explicit recipient, or no other eligible peer\)/i, ) diff --git a/tests/skills/ce-code-review-cross-model-routes.test.ts b/tests/skills/ce-code-review-cross-model-routes.test.ts index ed1937205..f28c763e4 100644 --- a/tests/skills/ce-code-review-cross-model-routes.test.ts +++ b/tests/skills/ce-code-review-cross-model-routes.test.ts @@ -603,6 +603,34 @@ describe("cross-model-adversarial-review skip paths — non-blocking, no file", expect(r.stderr).toContain("peer skip class: execution_context_auth") }) + test("classifies a bare HTTP 401 as execution_context_auth", () => { + const { env } = sandbox( + ["claude"], + "#!/bin/sh\ncat >/dev/null\nprintf '%s' 'HTTP 401 Unauthorized' >&2\nexit 1\n", + ) + const runDir = makeRunDir() + const r = run(["codex", "claude", "HEAD", runDir], runDir, env) + expect(r.stderr).toContain("peer skip class: execution_context_auth") + }) + + test("classifies weekly and Opus limit messages as session_quota", () => { + for (const result of ["You have hit your weekly limit", "You have hit your Opus limit"]) { + const payload = JSON.stringify({ + result, + api_error_status: 429, + terminal_reason: "api_error", + }) + const { env } = sandbox( + ["claude"], + `#!/bin/sh\ncat >/dev/null\nprintf '%s' '${payload}'\nexit 1\n`, + ) + const runDir = makeRunDir() + const r = run(["codex", "claude", "HEAD", runDir], runDir, env) + expect(r.stderr).toContain("peer skip class: session_quota") + expect(r.stderr).not.toContain("peer skip class: transient_rate_limit") + } + }) + test("classifies a Claude session-limit 429 as session_quota, not a completed review", () => { const payload = JSON.stringify({ result: "You have hit your session limit", From 876e22b12cafa5f01c3520fc9d9e5f70b0196e3e Mon Sep 17 00:00:00 2001 From: Trevin Chow Date: Fri, 14 Aug 2026 17:03:53 -0700 Subject: [PATCH 3/6] Address PR review feedback (#1380) - Keep "hit your rate limit" as transient_rate_limit - Match 401 Unauthorized, not every "unauthorized" mention --- .../scripts/cross-model-adversarial-review.sh | 13 +++++--- .../ce-code-review-cross-model-routes.test.ts | 31 +++++++++++++++++++ 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/skills/ce-code-review/scripts/cross-model-adversarial-review.sh b/skills/ce-code-review/scripts/cross-model-adversarial-review.sh index 3fa757603..c0ecb79d6 100755 --- a/skills/ce-code-review/scripts/cross-model-adversarial-review.sh +++ b/skills/ce-code-review/scripts/cross-model-adversarial-review.sh @@ -932,19 +932,22 @@ run_provider() { fi } -# One token for fold-in. Auth before quota; session/usage language before a -# bare 429, which is only a transient rate limit. +# One token for fold-in. Auth first; a rate-limit phrase beats "hit your" +# quota wording; remaining session/usage language still beats a bare 429. classify_skip_class() { local text text="$(printf '%s' "${1:-}" | tr '[:upper:]' '[:lower:]')" case "$text" in - *'not logged in'*|*'please log in'*|*'please run /login'*|*'api_error_status=401'*|*'http 401'*|*unauthorized*) + *'not logged in'*|*'please log in'*|*'please run /login'*|*'api_error_status=401'*|*'http 401'*|*'401 unauthorized'*) printf '%s' execution_context_auth ;; - *'session limit'*|*'usage limit'*|*'usage-exhausted'*|*'usage exhausted'*|*'hit your '*|*quota*|*'api_error_status=402'*) + *'rate limit'*) + printf '%s' transient_rate_limit + ;; + *'session limit'*|*'usage limit'*|*'usage-exhausted'*|*'usage exhausted'*|*'hit your limit'*|*'hit your session'*|*'hit your weekly'*|*'hit your opus'*|*quota*|*'api_error_status=402'*) printf '%s' session_quota ;; - *'rate limit'*|*'api_error_status=429'*) + *'api_error_status=429'*) printf '%s' transient_rate_limit ;; *) diff --git a/tests/skills/ce-code-review-cross-model-routes.test.ts b/tests/skills/ce-code-review-cross-model-routes.test.ts index f28c763e4..880cb11a3 100644 --- a/tests/skills/ce-code-review-cross-model-routes.test.ts +++ b/tests/skills/ce-code-review-cross-model-routes.test.ts @@ -649,6 +649,37 @@ describe("cross-model-adversarial-review skip paths — non-blocking, no file", expect(r.stderr).not.toContain("peer skip class: transient_rate_limit") }) + test("classifies hit-your-rate-limit wording as transient, not session_quota", () => { + const payload = JSON.stringify({ + result: "You have hit your rate limit on this model", + api_error_status: 429, + terminal_reason: "api_error", + }) + const { env } = sandbox( + ["claude"], + `#!/bin/sh\ncat >/dev/null\nprintf '%s' '${payload}'\nexit 1\n`, + ) + const runDir = makeRunDir() + const r = run(["codex", "claude", "HEAD", runDir], runDir, env) + expect(r.stderr).toContain("peer skip class: transient_rate_limit") + expect(r.stderr).not.toContain("peer skip class: session_quota") + }) + + test("does not treat review text mentioning unauthorized access as auth", () => { + const payload = JSON.stringify({ + result: "Finding: unauthorized access to the admin API is possible", + terminal_reason: "api_error", + }) + const { env } = sandbox( + ["claude"], + `#!/bin/sh\ncat >/dev/null\nprintf '%s' '${payload}'\nexit 1\n`, + ) + const runDir = makeRunDir() + const r = run(["codex", "claude", "HEAD", runDir], runDir, env) + expect(r.stderr).toContain("peer skip class: other") + expect(r.stderr).not.toContain("peer skip class: execution_context_auth") + }) + test("classifies a plain 429 rate limit as transient_rate_limit", () => { const payload = JSON.stringify({ result: "Rate limit exceeded; retry shortly", From 9dbc880ea72c4e40c5d7afedbbf203b3ee19e40d Mon Sep 17 00:00:00 2001 From: Trevin Chow Date: Fri, 14 Aug 2026 17:09:38 -0700 Subject: [PATCH 4/6] fix(ce-code-review): let the agent classify peer skip evidence Drop the keyword skip-class matcher. The worker still prints bounded skip evidence; fold-in judges quota vs auth vs rate limit from that text and keeps the did-not-run fallback. --- skills/ce-code-review/SKILL.md | 4 +- .../references/cross-model-review.md | 18 ++-- .../references/dispatch-reviewers.md | 2 +- .../scripts/cross-model-adversarial-review.sh | 37 +------- tests/review-skill-contract.test.ts | 7 +- .../ce-code-review-cross-model-routes.test.ts | 86 ++----------------- 6 files changed, 26 insertions(+), 128 deletions(-) diff --git a/skills/ce-code-review/SKILL.md b/skills/ce-code-review/SKILL.md index f82882076..f8a05a9ac 100644 --- a/skills/ce-code-review/SKILL.md +++ b/skills/ce-code-review/SKILL.md @@ -48,7 +48,7 @@ Follow these boundaries in order; references supply the detail but never change 1. Resolve the reviewed diff and intent. 2. Read `references/persona-catalog.md`, then select the risk-driven reviewer roster and discover applicable standards paths. Do not select or dispatch personas without that catalog load. -3. When adversarial is selected for a local reviewed tree, start and persist the sanctioned cross-model job **before any local persona dispatch**. Invoking this skill authorizes its configured/allowlisted peer route after the required recipient-and-code-egress disclosure; do not ask for a second confirmation or skip merely because the user did not separately repeat that authorization. An explicit user prohibition on external review still wins. A started peer replaces the local adversarial persona; only an actual scope, allowlist, availability, authentication, or start failure leaves the local fallback at Stage 3d. Fold-in may still run the did-not-run fallback in `references/cross-model-review.md` when the started peer returns only `session_quota` or `execution_context_auth`. +3. When adversarial is selected for a local reviewed tree, start and persist the sanctioned cross-model job **before any local persona dispatch**. Invoking this skill authorizes its configured/allowlisted peer route after the required recipient-and-code-egress disclosure; do not ask for a second confirmation or skip merely because the user did not separately repeat that authorization. An explicit user prohibition on external review still wins. A started peer replaces the local adversarial persona; only an actual scope, allowlist, availability, authentication, or start failure leaves the local fallback at Stage 3d. Fold-in may still run the did-not-run fallback in `references/cross-model-review.md` when skip evidence shows session/usage quota or execution-context auth. 4. Before any local dispatch, read `references/dispatch-reviewers.md`; if it is not loaded, stop and load it. Then dispatch the materialized local roster as a foreground concurrent batch sized to the host's active-agent cap — spawn multiple reviewers in one message with background execution off where the harness runs same-message calls concurrently, and collect every reviewer before synthesis (one blocking wait on Claude-style harnesses; repeated non-polling collection waits on async `spawn_agent` harnesses); degrade to serial where it does not. Detaching local review into a polled background job is forbidden; the cross-model peer is the only detached work and overlaps with this batch. Shell no-ops and wakeup polling are forbidden. 5. After the reviewer returns are ready, read `references/finish-review.md`; if it is not loaded, stop and load it. Fold in the peer once, run the documented findings mechanics, run every validator the reference selects, and only then return the report. Never synthesize directly from raw reviewer artifacts. The exact Actionable Findings, Coverage, and Verdict completion fields are required. When a peer ran, Coverage must record its route plus the literal keyed fields `model_requested`, `model_actual`, `effort_requested`, `effort_actual`, `receipt_supported`, and `independence_verified` from the artifact; never shorten that tuple to a model family or vague "high reasoning" claim. In the multi-agent path, emit only this skill's report; do not also invoke a harness-native findings/reporting tool. The native review tool belongs only to the explicit Quick Review Short-Circuit. Bare and `mode:agent` reviews never apply fixes; only explicit `apply:local` can enter the apply stage. @@ -473,7 +473,7 @@ echo "$RUN_DIR"; When adversarial was selected and scope is `local-aligned` or standalone, read `references/cross-model-review.md` from this skill's directory in full, attest the host, resolve and sanction one fixed route, and make its required egress announcement. Before start, write the reference's compact orchestrator-owned adversarial review brief to the run directory: intent plus the material risk divisions inferred from the current file inventory and diff, without embedding the diff or mechanically copying every path. Then start the detached peer job using the reference's exact invocation and persist its job ID, target, requested model/reasoning, and start epoch in working state. -- If the runner returns a job ID, the peer owns the adversarial lens for this run. Remove `adversarial-reviewer` from the local roster immediately. Do not read its local persona asset or dispatch it later — except the fold-in did-not-run fallback in `references/cross-model-review.md` when skip class is `session_quota` or `execution_context_auth`. +- If the runner returns a job ID, the peer owns the adversarial lens for this run. Remove `adversarial-reviewer` from the local roster immediately. Do not read its local persona asset or dispatch it later — except the fold-in did-not-run fallback in `references/cross-model-review.md` when skip evidence is session/usage quota or execution-context auth. - If no job starts because of a dispatch-infrastructure failure (a non-zero exit before any job id, an unresolved `$SKILL_DIR`/script path), first attempt the bounded same-route hand recovery from `references/cross-model-review.md` before accepting the fallback: re-run the identical resolved route, holding target/model and read scope fixed, while each failure is a new plausibly recoverable one and the shared peer deadline holds. If recovery returns a job id, treat it as the branch above (the peer owns the lens; remove `adversarial-reviewer`). Only when recovery is exhausted — a failure repeats or the deadline is spent — or the peer was never eligible to start (gate not met, host un-attestable, no different provider, CLI missing/unauthed), keep `adversarial-reviewer` in the local roster as the fallback and record the peer skip reason for Coverage. - In `pr-remote` / `branch-remote`, do not start the peer; keep the selected in-process adversarial reviewer because it can inspect the reviewed refs. diff --git a/skills/ce-code-review/references/cross-model-review.md b/skills/ce-code-review/references/cross-model-review.md index 69c3fd182..ee41c4735 100644 --- a/skills/ce-code-review/references/cross-model-review.md +++ b/skills/ce-code-review/references/cross-model-review.md @@ -154,16 +154,16 @@ Repeat that call until the job is terminal or the derived deadline is spent; do - In final Coverage, name `cross_model_route`, `model_requested`, `effort_requested`, `receipt_supported`, `model_actual`, `effort_actual`, and `independence_verified` from the artifact. Keep the literal `unverified`; never compress a request into a serving claim such as "via Codex high" when actual model or effort is unverified. - **Never started / not run** — the job was never started (gates not met, host un-attestable, no different provider reachable, CLI missing/unauthed): the pass simply didn't run. Note "cross-model pass: not run" in Coverage for human-facing markdown; stay silent in `mode:agent`. Ignore any `*.raw.json` leftovers — they are not fold-in artifacts. - **Dispatch-infrastructure failure** — the runner or worker itself crashed: a non-zero exit before any job starts, a preflight/detach failure, or an unresolved `$SKILL_DIR`/script path. This is distinct from the gate-not-met skips above (there, no dispatch was attempted), so do not fold it into the silent not-run bucket on the first error. The two failure shapes recover at different points. A **no-job-id** preflight failure (exit before any job id, unresolved `$SKILL_DIR`) is recovered entirely at **Stage 3d's no-job branch**, before the local roster is materialized — the only point where re-running the start can still recover cross-model corroboration and, failing that, cleanly fall to the in-process reviewer (which then covers the lens; only corroboration is lost). Do **not** re-attempt that case here at fold-in: Stage 4 may already have dispatched the in-process `adversarial-reviewer`, so a fold-in peer re-run would put both on the same brief and violate the exclusive routing boundary. This step handles only the **job-id-returned-then-failed** crash — its failed job is reaped here and the in-process reviewer is already gone. For it, re-run the **same resolved fixed route** by hand — holding the target and model, the `git diff ` read scope, and the adversarial persona brief fixed — while each failure is a new, plausibly recoverable one and the shared peer deadline holds. This is a same-route retry, deliberately distinct from the quota rule below, which requires a newly disclosed route. Stop once a failure repeats or the deadline is spent; the hand recovery is then the adversarial lens's only cover, so the Coverage line must report the adversarial lens as **degraded**, not merely cross-model corroboration lost. A hand recovery may not substitute a different target or provider, widen the read scope, or relax the read-only trust boundary — those make the recovered peer untrustworthy, not merely unavailable. -- **Ran but produced no usable output** — the job reached `done` (or any terminal state) yet no `adversarial-.json` exists (the peer ran and egressed but returned nothing schema-shaped — unparseable output, empty findings the script dropped). Distinct from not-run: note "cross-model pass: peer ran, no usable output" in human-facing markdown Coverage. Never fail the review. If `peer skip class:` is `session_quota` or `execution_context_auth`, this is not merely empty output — run the did-not-run fallback below before treating the lens as covered. -- **Started but not `done`** — the final status read reports `failed`, `timeout`, or `died-without-result` (a job reaped at the shared deadline records `timeout`, with the reap noted in its reason) → still non-blocking, but never silent: name the peer and its terminal state in Coverage (e.g. "cross-model adversarial peer: timeout"). Silent absence stays correct only for passes that never started or were skipped. If the failed job's skip class is `session_quota` or `execution_context_auth`, run the did-not-run fallback below rather than leaving the lens uncovered. +- **Ran but produced no usable output** — the job reached `done` (or any terminal state) yet no `adversarial-.json` exists (the peer ran and egressed but returned nothing schema-shaped — unparseable output, empty findings the script dropped). Distinct from not-run: note "cross-model pass: peer ran, no usable output" in human-facing markdown Coverage. Never fail the review. If skip evidence is session/usage quota or execution-context auth, this is not merely empty output — run the did-not-run fallback below before treating the lens as covered. +- **Started but not `done`** — the final status read reports `failed`, `timeout`, or `died-without-result` (a job reaped at the shared deadline records `timeout`, with the reap noted in its reason) → still non-blocking, but never silent: name the peer and its terminal state in Coverage (e.g. "cross-model adversarial peer: timeout"). Silent absence stays correct only for passes that never started or were skipped. If skip evidence is session/usage quota or execution-context auth, run the did-not-run fallback below rather than leaving the lens uncovered. - Empty `findings` → note "cross-model pass: no additional issues" in Coverage. -- **Classify the skip reason before deleting.** Read `out.log` before cleanup, including bounded lines prefixed `peer skip evidence:` and `peer skip class:`. Prefer the structured `peer skip class:` token when present. - - `execution_context_auth` (`not logged in`, `please log in`, 401, or a CLI login prompt) describes only the peer's execution context: a sandboxed host produces the same signal as a genuine logout, so never report it as the user's account being logged out or prompt a login command. - - `session_quota` (session/usage-limit, usage-exhausted, 402, or "hit your limit" with a reset) means this route cannot review now. Do not retry it. - - `transient_rate_limit` (plain 429 / rate-limit without session or usage language) may still recover on the same route. Do not run the did-not-run fallback. - - `other`: name the observed failure; do not run the did-not-run fallback. -- **Did-not-run fallback** (first `session_quota` or `execution_context_auth` observation, including the first one): the started job did not cover the adversarial lens. Do not retry that same route. Then: - 1. If this is the first replacement attempt, the failed recipient was **not** an explicit user-stated preference (Step 1 item 1), and another attested-different installed+allowlisted target remains: announce that new recipient and start a new job with a new `CROSS_MODEL_FIXED_ROUTE`. Wait for it with the remaining shared deadline and fold its artifact the same way as the first job. That job owns the lens. Never switch recipients inside the worker. If this replacement also ends `session_quota` or `execution_context_auth`, do not start a third peer; take step 2. +- **Classify the skip reason before deleting.** Read `out.log` before cleanup, including bounded lines prefixed `peer skip evidence:`. Judge the full diagnostic; do not grep for a closed phrase list. Login-shaped peer text describes only the peer's execution context: a sandboxed host produces the same signal as a genuine logout, so never report it as the user's account being logged out or prompt a login command. + - Session or usage quota (the route cannot review now): do not retry that route; run the did-not-run fallback. + - Execution-context authentication: same — the peer did not review; run the did-not-run fallback. + - Transient rate limit (throttle, retry shortly, without quota/session exhaustion): same-route retry may still recover; do not run the did-not-run fallback. + - Anything else: name the observed failure; do not run the did-not-run fallback. +- **Did-not-run fallback** (first quota or execution-context-auth observation, including the first one): the started job did not cover the adversarial lens. Do not retry that same route. Then: + 1. If this is the first replacement attempt, the failed recipient was **not** an explicit user-stated preference (Step 1 item 1), and another attested-different installed+allowlisted target remains: announce that new recipient and start a new job with a new `CROSS_MODEL_FIXED_ROUTE`. Wait for it with the remaining shared deadline and fold its artifact the same way as the first job. That job owns the lens. Never switch recipients inside the worker. If this replacement also ends in quota or execution-context auth, do not start a third peer; take step 2. 2. Otherwise (explicit recipient, or no other eligible peer): dispatch in-process `adversarial-reviewer` now. Coverage records the peer as not-run for quota/auth and that the lens used the local fallback. A config or default selection is not an explicit user-stated preference. One replacement only; never silently continue to another recipient. - After fold-in (or after deadline reaping), delete the consumed job directory (`/jobs//`) — its log and result are review content and must not outlive their use. diff --git a/skills/ce-code-review/references/dispatch-reviewers.md b/skills/ce-code-review/references/dispatch-reviewers.md index 4ce62daa3..c7b6c23b6 100644 --- a/skills/ce-code-review/references/dispatch-reviewers.md +++ b/skills/ce-code-review/references/dispatch-reviewers.md @@ -106,6 +106,6 @@ The artifact file **must** carry the full detail-tier fields (`why_it_matters`, Stage 3d already made the exclusive route choice and, when applicable, started the detached peer. Do not resolve, start, or substitute a route here except the did-not-run fallback in `references/cross-model-review.md`. Dispatch only the materialized local roster. -After the inline fast pass has completed and the local reviewer batch has started, prepare synthesis inputs while reviewers run. Do not poll the peer during that wave. After local reviewers finish, if Stage 3d persisted a peer job ID, perform the reference's single bounded status/wait/reap sequence and fold in whatever terminal artifact is available. Attribute from the artifact and clean up through the runner. A failure or timeout stays non-blocking and is named in Coverage; it never triggers a late in-process adversarial retry, except the did-not-run fallback in `references/cross-model-review.md` for `session_quota` or `execution_context_auth`. Peer findings enter ordinary synthesis, but agreement promotion requires top-level `independence_verified: true`; false or absent independence is useful evidence, not different-model corroboration. Coverage must say whether the adversarial lens ran cross-model or used the in-process fallback. +After the inline fast pass has completed and the local reviewer batch has started, prepare synthesis inputs while reviewers run. Do not poll the peer during that wave. After local reviewers finish, if Stage 3d persisted a peer job ID, perform the reference's single bounded status/wait/reap sequence and fold in whatever terminal artifact is available. Attribute from the artifact and clean up through the runner. A failure or timeout stays non-blocking and is named in Coverage; it never triggers a late in-process adversarial retry, except the did-not-run fallback in `references/cross-model-review.md` when skip evidence is session/usage quota or execution-context auth. Peer findings enter ordinary synthesis, but agreement promotion requires top-level `independence_verified: true`; false or absent independence is useful evidence, not different-model corroboration. Coverage must say whether the adversarial lens ran cross-model or used the in-process fallback. The peer return enters Stage 5 as reviewer `adversarial-`, like any persona artifact. A pass that never started is recorded as not run (or as the in-process fallback when selected); a started peer that fails, times out, dies, or is reaped is named with its terminal state rather than vanishing silently. diff --git a/skills/ce-code-review/scripts/cross-model-adversarial-review.sh b/skills/ce-code-review/scripts/cross-model-adversarial-review.sh index c0ecb79d6..ec067c6bf 100755 --- a/skills/ce-code-review/scripts/cross-model-adversarial-review.sh +++ b/skills/ce-code-review/scripts/cross-model-adversarial-review.sh @@ -910,52 +910,23 @@ run_provider() { log "wrote $n finding(s) to $OUT (reviewer adversarial-$provider)" else log "provider $provider produced no usable schema-shaped output; skipping fold-in" - # Surface bounded peer output so the orchestrator can - # reason about WHY it was skipped (quota/usage-limit exhaustion vs an ordinary - # empty review). Prefer structured error fields because a raw tail can discard - # the actionable message in a large CLI envelope. Surface BOTH streams -- the + # Surface bounded peer output so the orchestrator can reason about WHY it + # was skipped (quota/usage-limit exhaustion vs an ordinary empty review). + # Prefer structured error fields because a raw tail can discard the + # actionable message in a large CLI envelope. Surface BOTH streams -- the # error can be on stdout (grok's 402) or stderr (claude/cursor auth/quota). - # Then emit one structured class the fold-in fallback chain keys on. - _class_src="" if [ -s "$PEERLOG" ]; then _pt="$(bounded_failure_evidence "$PEERLOG")" log " peer skip evidence: $_pt" - _class_src="$_pt" fi if [ -s "$PEERERR" ]; then _pe="$(bounded_failure_evidence "$PEERERR")" log " peer skip evidence (stderr): $_pe" - _class_src="${_class_src} ${_pe}" fi - log " peer skip class: $(classify_skip_class "$_class_src")" rm -f "$OUT" "$RAW_OUT" fi } -# One token for fold-in. Auth first; a rate-limit phrase beats "hit your" -# quota wording; remaining session/usage language still beats a bare 429. -classify_skip_class() { - local text - text="$(printf '%s' "${1:-}" | tr '[:upper:]' '[:lower:]')" - case "$text" in - *'not logged in'*|*'please log in'*|*'please run /login'*|*'api_error_status=401'*|*'http 401'*|*'401 unauthorized'*) - printf '%s' execution_context_auth - ;; - *'rate limit'*) - printf '%s' transient_rate_limit - ;; - *'session limit'*|*'usage limit'*|*'usage-exhausted'*|*'usage exhausted'*|*'hit your limit'*|*'hit your session'*|*'hit your weekly'*|*'hit your opus'*|*quota*|*'api_error_status=402'*) - printf '%s' session_quota - ;; - *'api_error_status=429'*) - printf '%s' transient_rate_limit - ;; - *) - printf '%s' other - ;; - esac -} - # Prefer structured CLI diagnostics over a raw tail, which can hide the useful # error near the beginning of a large JSON envelope. bounded_failure_evidence() { # diff --git a/tests/review-skill-contract.test.ts b/tests/review-skill-contract.test.ts index ca349f9d1..bf0238cd9 100644 --- a/tests/review-skill-contract.test.ts +++ b/tests/review-skill-contract.test.ts @@ -1218,11 +1218,10 @@ describe("cross-model peer skip legibility", () => { expect(referenceSrc).toContain("peer skip evidence:") expect(referenceSrc).toMatch(/quota|usage-limit/i) if (worker.includes("ce-code-review")) { - expect(workerSrc).toContain("peer skip class:") - expect(referenceSrc).toContain("peer skip class:") - expect(referenceSrc).toContain("session_quota") - expect(referenceSrc).toContain("execution_context_auth") + expect(workerSrc).not.toContain("peer skip class:") + expect(referenceSrc).not.toContain("peer skip class:") expect(referenceSrc).toMatch(/did-not-run fallback/i) + expect(referenceSrc).toMatch(/Judge the full diagnostic/i) expect(referenceSrc).toMatch(/never silently continue to another recipient/i) expect(referenceSrc).toMatch(/explicit user-stated preference/i) expect(referenceSrc).toContain("in-process `adversarial-reviewer`") diff --git a/tests/skills/ce-code-review-cross-model-routes.test.ts b/tests/skills/ce-code-review-cross-model-routes.test.ts index 880cb11a3..30a775f73 100644 --- a/tests/skills/ce-code-review-cross-model-routes.test.ts +++ b/tests/skills/ce-code-review-cross-model-routes.test.ts @@ -600,38 +600,11 @@ describe("cross-model-adversarial-review skip paths — non-blocking, no file", const r = run(["codex", "claude", "HEAD", runDir], runDir, env) expect(r.stderr).toContain("Not logged in") expect(r.stderr).toContain("terminal_reason=api_error") - expect(r.stderr).toContain("peer skip class: execution_context_auth") + expect(r.stderr).toContain("peer skip evidence:") + expect(r.stderr).not.toContain("peer skip class:") }) - test("classifies a bare HTTP 401 as execution_context_auth", () => { - const { env } = sandbox( - ["claude"], - "#!/bin/sh\ncat >/dev/null\nprintf '%s' 'HTTP 401 Unauthorized' >&2\nexit 1\n", - ) - const runDir = makeRunDir() - const r = run(["codex", "claude", "HEAD", runDir], runDir, env) - expect(r.stderr).toContain("peer skip class: execution_context_auth") - }) - - test("classifies weekly and Opus limit messages as session_quota", () => { - for (const result of ["You have hit your weekly limit", "You have hit your Opus limit"]) { - const payload = JSON.stringify({ - result, - api_error_status: 429, - terminal_reason: "api_error", - }) - const { env } = sandbox( - ["claude"], - `#!/bin/sh\ncat >/dev/null\nprintf '%s' '${payload}'\nexit 1\n`, - ) - const runDir = makeRunDir() - const r = run(["codex", "claude", "HEAD", runDir], runDir, env) - expect(r.stderr).toContain("peer skip class: session_quota") - expect(r.stderr).not.toContain("peer skip class: transient_rate_limit") - } - }) - - test("classifies a Claude session-limit 429 as session_quota, not a completed review", () => { + test("surfaces a Claude session-limit 429 as skip evidence, not a completed review", () => { const payload = JSON.stringify({ result: "You have hit your session limit", api_error_status: 429, @@ -645,55 +618,10 @@ describe("cross-model-adversarial-review skip paths — non-blocking, no file", const r = run(["codex", "claude", "HEAD", runDir], runDir, env) expect(r.code).toBe(0) expect(r.files).not.toContain("adversarial-claude.json") - expect(r.stderr).toContain("peer skip class: session_quota") - expect(r.stderr).not.toContain("peer skip class: transient_rate_limit") - }) - - test("classifies hit-your-rate-limit wording as transient, not session_quota", () => { - const payload = JSON.stringify({ - result: "You have hit your rate limit on this model", - api_error_status: 429, - terminal_reason: "api_error", - }) - const { env } = sandbox( - ["claude"], - `#!/bin/sh\ncat >/dev/null\nprintf '%s' '${payload}'\nexit 1\n`, - ) - const runDir = makeRunDir() - const r = run(["codex", "claude", "HEAD", runDir], runDir, env) - expect(r.stderr).toContain("peer skip class: transient_rate_limit") - expect(r.stderr).not.toContain("peer skip class: session_quota") - }) - - test("does not treat review text mentioning unauthorized access as auth", () => { - const payload = JSON.stringify({ - result: "Finding: unauthorized access to the admin API is possible", - terminal_reason: "api_error", - }) - const { env } = sandbox( - ["claude"], - `#!/bin/sh\ncat >/dev/null\nprintf '%s' '${payload}'\nexit 1\n`, - ) - const runDir = makeRunDir() - const r = run(["codex", "claude", "HEAD", runDir], runDir, env) - expect(r.stderr).toContain("peer skip class: other") - expect(r.stderr).not.toContain("peer skip class: execution_context_auth") - }) - - test("classifies a plain 429 rate limit as transient_rate_limit", () => { - const payload = JSON.stringify({ - result: "Rate limit exceeded; retry shortly", - api_error_status: 429, - terminal_reason: "api_error", - }) - const { env } = sandbox( - ["claude"], - `#!/bin/sh\ncat >/dev/null\nprintf '%s' '${payload}'\nexit 1\n`, - ) - const runDir = makeRunDir() - const r = run(["codex", "claude", "HEAD", runDir], runDir, env) - expect(r.stderr).toContain("peer skip class: transient_rate_limit") - expect(r.stderr).not.toContain("peer skip class: session_quota") + expect(r.stderr).toContain("peer skip evidence:") + expect(r.stderr).toContain("You have hit your session limit") + expect(r.stderr).toContain("api_error_status=429") + expect(r.stderr).not.toContain("peer skip class:") }) test("ancillary structured fields do not hide an unrecognized human-readable diagnostic", () => { From 71e961448b3a2fbfce39d35ece681f3c2652b0ba Mon Sep 17 00:00:00 2001 From: Trevin Chow Date: Fri, 14 Aug 2026 17:11:11 -0700 Subject: [PATCH 5/6] fix(ce-code-review): require a same-route retry on transient rate limits A throttle no longer leaves the adversarial lens uncovered: retry the already disclosed route once, then restore the local reviewer. --- skills/ce-code-review/references/cross-model-review.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/skills/ce-code-review/references/cross-model-review.md b/skills/ce-code-review/references/cross-model-review.md index ee41c4735..274721762 100644 --- a/skills/ce-code-review/references/cross-model-review.md +++ b/skills/ce-code-review/references/cross-model-review.md @@ -160,7 +160,7 @@ Repeat that call until the job is terminal or the derived deadline is spent; do - **Classify the skip reason before deleting.** Read `out.log` before cleanup, including bounded lines prefixed `peer skip evidence:`. Judge the full diagnostic; do not grep for a closed phrase list. Login-shaped peer text describes only the peer's execution context: a sandboxed host produces the same signal as a genuine logout, so never report it as the user's account being logged out or prompt a login command. - Session or usage quota (the route cannot review now): do not retry that route; run the did-not-run fallback. - Execution-context authentication: same — the peer did not review; run the did-not-run fallback. - - Transient rate limit (throttle, retry shortly, without quota/session exhaustion): same-route retry may still recover; do not run the did-not-run fallback. + - Transient rate limit (throttle, retry shortly, without quota/session exhaustion): one same-route retry of the already disclosed route. If that retry still produces no review, dispatch in-process `adversarial-reviewer`. Do not run the did-not-run fallback and do not switch recipients. - Anything else: name the observed failure; do not run the did-not-run fallback. - **Did-not-run fallback** (first quota or execution-context-auth observation, including the first one): the started job did not cover the adversarial lens. Do not retry that same route. Then: 1. If this is the first replacement attempt, the failed recipient was **not** an explicit user-stated preference (Step 1 item 1), and another attested-different installed+allowlisted target remains: announce that new recipient and start a new job with a new `CROSS_MODEL_FIXED_ROUTE`. Wait for it with the remaining shared deadline and fold its artifact the same way as the first job. That job owns the lens. Never switch recipients inside the worker. If this replacement also ends in quota or execution-context auth, do not start a third peer; take step 2. From 7c47e5145f1d542547181e97d29c210a006e3dea Mon Sep 17 00:00:00 2001 From: Trevin Chow Date: Fri, 14 Aug 2026 17:18:44 -0700 Subject: [PATCH 6/6] Address PR review feedback (#1380) - Allow late in-process adversarial restore after a failed same-route rate-limit retry - Keep 429 out of the quota/auth did-not-run fallback so it does not switch recipients --- docs/skills/ce-code-review.md | 2 +- skills/ce-code-review/SKILL.md | 8 ++++---- skills/ce-code-review/references/dispatch-reviewers.md | 4 ++-- skills/ce-code-review/references/persona-catalog.md | 2 +- tests/review-skill-contract.test.ts | 10 ++++++++++ 5 files changed, 18 insertions(+), 8 deletions(-) diff --git a/docs/skills/ce-code-review.md b/docs/skills/ce-code-review.md index 2a91b2096..1b5c85513 100644 --- a/docs/skills/ce-code-review.md +++ b/docs/skills/ce-code-review.md @@ -112,7 +112,7 @@ When you pass a PR number or URL, trivial automated PRs (lockfile bumps, chore v ### Cross-model adversarial pass -When adversarial is selected and the working tree is the reviewed head (current branch, or a PR whose local tree already matches the PR head), the adversarial lens runs through **one different model provider than the host** in a separate read-only process. A started peer **replaces** the in-process `adversarial` persona. They never both receive the same brief. The in-process persona runs if the peer cannot start, or if the started peer returns only session-quota or auth-context failure — in that case the next announced different-family peer is tried when one is eligible, otherwise the local persona covers the lens. Remote PR or branch diffs stay on the in-process persona, because that reviewer can inspect the fetched refs. +When adversarial is selected and the working tree is the reviewed head (current branch, or a PR whose local tree already matches the PR head), the adversarial lens runs through **one different model provider than the host** in a separate read-only process. A started peer **replaces** the in-process `adversarial` persona. They never both receive the same brief. The in-process persona runs if the peer cannot start, or if the started peer returns only session-quota or auth-context failure — in that case the next announced different-family peer is tried when one is eligible, otherwise the local persona covers the lens. After a stubborn transient rate limit, one same-route retry then local; no recipient switch. Remote PR or branch diffs stay on the in-process persona, because that reviewer can inspect the fetched refs. Agreement between the peer and another in-process reviewer is a strong promotion signal in synthesis. diff --git a/skills/ce-code-review/SKILL.md b/skills/ce-code-review/SKILL.md index f8a05a9ac..b907b00ca 100644 --- a/skills/ce-code-review/SKILL.md +++ b/skills/ce-code-review/SKILL.md @@ -48,7 +48,7 @@ Follow these boundaries in order; references supply the detail but never change 1. Resolve the reviewed diff and intent. 2. Read `references/persona-catalog.md`, then select the risk-driven reviewer roster and discover applicable standards paths. Do not select or dispatch personas without that catalog load. -3. When adversarial is selected for a local reviewed tree, start and persist the sanctioned cross-model job **before any local persona dispatch**. Invoking this skill authorizes its configured/allowlisted peer route after the required recipient-and-code-egress disclosure; do not ask for a second confirmation or skip merely because the user did not separately repeat that authorization. An explicit user prohibition on external review still wins. A started peer replaces the local adversarial persona; only an actual scope, allowlist, availability, authentication, or start failure leaves the local fallback at Stage 3d. Fold-in may still run the did-not-run fallback in `references/cross-model-review.md` when skip evidence shows session/usage quota or execution-context auth. +3. When adversarial is selected for a local reviewed tree, start and persist the sanctioned cross-model job **before any local persona dispatch**. Invoking this skill authorizes its configured/allowlisted peer route after the required recipient-and-code-egress disclosure; do not ask for a second confirmation or skip merely because the user did not separately repeat that authorization. An explicit user prohibition on external review still wins. A started peer replaces the local adversarial persona; only an actual scope, allowlist, availability, authentication, or start failure leaves the local fallback at Stage 3d. Fold-in may still run the did-not-run fallback in `references/cross-model-review.md` when skip evidence shows session/usage quota or execution-context auth, or restore local after a failed same-route rate-limit retry. 4. Before any local dispatch, read `references/dispatch-reviewers.md`; if it is not loaded, stop and load it. Then dispatch the materialized local roster as a foreground concurrent batch sized to the host's active-agent cap — spawn multiple reviewers in one message with background execution off where the harness runs same-message calls concurrently, and collect every reviewer before synthesis (one blocking wait on Claude-style harnesses; repeated non-polling collection waits on async `spawn_agent` harnesses); degrade to serial where it does not. Detaching local review into a polled background job is forbidden; the cross-model peer is the only detached work and overlaps with this batch. Shell no-ops and wakeup polling are forbidden. 5. After the reviewer returns are ready, read `references/finish-review.md`; if it is not loaded, stop and load it. Fold in the peer once, run the documented findings mechanics, run every validator the reference selects, and only then return the report. Never synthesize directly from raw reviewer artifacts. The exact Actionable Findings, Coverage, and Verdict completion fields are required. When a peer ran, Coverage must record its route plus the literal keyed fields `model_requested`, `model_actual`, `effort_requested`, `effort_actual`, `receipt_supported`, and `independence_verified` from the artifact; never shorten that tuple to a model family or vague "high reasoning" claim. In the multi-agent path, emit only this skill's report; do not also invoke a harness-native findings/reporting tool. The native review tool belongs only to the explicit Quick Review Short-Circuit. Bare and `mode:agent` reviews never apply fixes; only explicit `apply:local` can enter the apply stage. @@ -175,7 +175,7 @@ Reviewer personas are selected in layers. The persona catalog in `references/per - `api-contract-reviewer` — routes, serializers, type signatures, versioning - `data-migration-reviewer` — migration files / schema dumps / backfills (see spawn gate in Stage 3) - `reliability-reviewer` — error handling, retries, timeouts, background jobs -- `adversarial-reviewer` lens — >=50 changed code lines, or auth / payments / persistence writes / event publication / retry or concurrency semantics / external APIs, or a **silent-pass verification mechanism** regardless of size. Satisfy this lens with the independent cross-model adversarial pass when a sanctioned peer job starts successfully. Dispatch the in-process `adversarial-reviewer` when the peer cannot start, or when fold-in runs the did-not-run fallback; do not run both same-brief reviews. +- `adversarial-reviewer` lens — >=50 changed code lines, or auth / payments / persistence writes / event publication / retry or concurrency semantics / external APIs, or a **silent-pass verification mechanism** regardless of size. Satisfy this lens with the independent cross-model adversarial pass when a sanctioned peer job starts successfully. Dispatch the in-process `adversarial-reviewer` when the peer cannot start, when fold-in runs the did-not-run fallback, or when fold-in restores local after a failed same-route rate-limit retry; do not run both same-brief reviews. - `previous-comments-reviewer` — PR with existing review comments (PR-only, comment-gated) **Stack-specific conditional (per diff):** `julik-frontend-races-reviewer` (Stimulus/Turbo, DOM events, async UI) and `swift-ios-reviewer` (Swift/SwiftUI/UIKit, entitlements, Core Data, `.pbxproj`). @@ -455,7 +455,7 @@ Distinguish an empty successful search from a failed or unavailable search: ### Stage 3d: Bind the adversarial route and final roster -Complete this stage **before reading persona prompt assets or entering Stage 4**. It owns the exclusive choice between a cross-model adversarial peer and the in-process `adversarial-reviewer`; later stages consume that choice and must not decide it again, except the fold-in did-not-run fallback. +Complete this stage **before reading persona prompt assets or entering Stage 4**. It owns the exclusive choice between a cross-model adversarial peer and the in-process `adversarial-reviewer`; later stages consume that choice and must not decide it again, except the fold-in did-not-run fallback or the fold-in in-process restore after a failed same-route rate-limit retry. Generate the review run ID now so both routes share one artifact directory: @@ -473,7 +473,7 @@ echo "$RUN_DIR"; When adversarial was selected and scope is `local-aligned` or standalone, read `references/cross-model-review.md` from this skill's directory in full, attest the host, resolve and sanction one fixed route, and make its required egress announcement. Before start, write the reference's compact orchestrator-owned adversarial review brief to the run directory: intent plus the material risk divisions inferred from the current file inventory and diff, without embedding the diff or mechanically copying every path. Then start the detached peer job using the reference's exact invocation and persist its job ID, target, requested model/reasoning, and start epoch in working state. -- If the runner returns a job ID, the peer owns the adversarial lens for this run. Remove `adversarial-reviewer` from the local roster immediately. Do not read its local persona asset or dispatch it later — except the fold-in did-not-run fallback in `references/cross-model-review.md` when skip evidence is session/usage quota or execution-context auth. +- If the runner returns a job ID, the peer owns the adversarial lens for this run. Remove `adversarial-reviewer` from the local roster immediately. Do not read its local persona asset or dispatch it later — except the fold-in did-not-run fallback in `references/cross-model-review.md` when skip evidence is session/usage quota or execution-context auth, or the fold-in in-process restore after a failed same-route rate-limit retry. - If no job starts because of a dispatch-infrastructure failure (a non-zero exit before any job id, an unresolved `$SKILL_DIR`/script path), first attempt the bounded same-route hand recovery from `references/cross-model-review.md` before accepting the fallback: re-run the identical resolved route, holding target/model and read scope fixed, while each failure is a new plausibly recoverable one and the shared peer deadline holds. If recovery returns a job id, treat it as the branch above (the peer owns the lens; remove `adversarial-reviewer`). Only when recovery is exhausted — a failure repeats or the deadline is spent — or the peer was never eligible to start (gate not met, host un-attestable, no different provider, CLI missing/unauthed), keep `adversarial-reviewer` in the local roster as the fallback and record the peer skip reason for Coverage. - In `pr-remote` / `branch-remote`, do not start the peer; keep the selected in-process adversarial reviewer because it can inspect the reviewed refs. diff --git a/skills/ce-code-review/references/dispatch-reviewers.md b/skills/ce-code-review/references/dispatch-reviewers.md index c7b6c23b6..de028bd56 100644 --- a/skills/ce-code-review/references/dispatch-reviewers.md +++ b/skills/ce-code-review/references/dispatch-reviewers.md @@ -104,8 +104,8 @@ The artifact file **must** carry the full detail-tier fields (`why_it_matters`, #### Cross-model adversarial pass -Stage 3d already made the exclusive route choice and, when applicable, started the detached peer. Do not resolve, start, or substitute a route here except the did-not-run fallback in `references/cross-model-review.md`. Dispatch only the materialized local roster. +Stage 3d already made the exclusive route choice and, when applicable, started the detached peer. Do not resolve, start, or substitute a route here except the fold-in did-not-run fallback in `references/cross-model-review.md` (quota/auth) or the fold-in in-process restore after a failed same-route rate-limit retry. Dispatch only the materialized local roster. -After the inline fast pass has completed and the local reviewer batch has started, prepare synthesis inputs while reviewers run. Do not poll the peer during that wave. After local reviewers finish, if Stage 3d persisted a peer job ID, perform the reference's single bounded status/wait/reap sequence and fold in whatever terminal artifact is available. Attribute from the artifact and clean up through the runner. A failure or timeout stays non-blocking and is named in Coverage; it never triggers a late in-process adversarial retry, except the did-not-run fallback in `references/cross-model-review.md` when skip evidence is session/usage quota or execution-context auth. Peer findings enter ordinary synthesis, but agreement promotion requires top-level `independence_verified: true`; false or absent independence is useful evidence, not different-model corroboration. Coverage must say whether the adversarial lens ran cross-model or used the in-process fallback. +After the inline fast pass has completed and the local reviewer batch has started, prepare synthesis inputs while reviewers run. Do not poll the peer during that wave. After local reviewers finish, if Stage 3d persisted a peer job ID, perform the reference's single bounded status/wait/reap sequence and fold in whatever terminal artifact is available. Attribute from the artifact and clean up through the runner. A failure or timeout stays non-blocking and is named in Coverage; it never triggers a late in-process adversarial retry, except the did-not-run fallback in `references/cross-model-review.md` when skip evidence is session/usage quota or execution-context auth, or the fold-in in-process restore after a failed same-route rate-limit retry. Peer findings enter ordinary synthesis, but agreement promotion requires top-level `independence_verified: true`; false or absent independence is useful evidence, not different-model corroboration. Coverage must say whether the adversarial lens ran cross-model or used the in-process fallback. The peer return enters Stage 5 as reviewer `adversarial-`, like any persona artifact. A pass that never started is recorded as not run (or as the in-process fallback when selected); a started peer that fails, times out, dies, or is reaped is named with its terminal state rather than vanishing silently. diff --git a/skills/ce-code-review/references/persona-catalog.md b/skills/ce-code-review/references/persona-catalog.md index bb40103aa..ec2ed0cca 100644 --- a/skills/ce-code-review/references/persona-catalog.md +++ b/skills/ce-code-review/references/persona-catalog.md @@ -35,7 +35,7 @@ Spawned when the orchestrator identifies relevant patterns in the diff. The orch | `api-contract` | `api-contract-reviewer` | An externally consumed boundary changes: route/request/response definitions, serializers, published event schemas, API versioning, or a public package signature with evidenced downstream callers. A new or changed exported symbol inside one module is insufficient by itself. | | `data-migration` | `data-migration-reviewer` | Migration files, schema dumps (`db/schema.rb`, `structure.sql`), backfill scripts, data transformations — **not** model/query-only changes without migration artifacts | | `reliability` | `reliability-reviewer` | Error handling, retry logic, circuit breakers, timeouts, background jobs, async handlers, health checks | -| `adversarial` | `adversarial-reviewer` fallback | >=50 changed code lines; auth/payments; persistence writes or event publication; retry/partial-failure or concurrency/ordering semantics; external APIs; or a silent-pass verification mechanism. The lens runs through the independent cross-model peer when a sanctioned peer starts. Spawn this in-process persona when the peer cannot start, or when fold-in runs the did-not-run fallback. A started peer that produced a review artifact and the fallback are mutually exclusive. | +| `adversarial` | `adversarial-reviewer` fallback | >=50 changed code lines; auth/payments; persistence writes or event publication; retry/partial-failure or concurrency/ordering semantics; external APIs; or a silent-pass verification mechanism. The lens runs through the independent cross-model peer when a sanctioned peer starts. Spawn this in-process persona when the peer cannot start, when fold-in runs the did-not-run fallback, or when fold-in restores local after a failed same-route rate-limit retry. A started peer that produced a review artifact and the fallback are mutually exclusive. | | `previous-comments` | `previous-comments-reviewer` | **PR-only AND comment-gated.** Reviewing a PR that has existing review comments or review threads from prior review rounds. Skip entirely when no PR metadata was gathered in Stage 1, OR when Stage 1's `hasPriorComments` flag is false (no `reviews` and no `comments` on the PR). | ## Stack-Specific Conditional (2 personas) diff --git a/tests/review-skill-contract.test.ts b/tests/review-skill-contract.test.ts index bf0238cd9..36f55a107 100644 --- a/tests/review-skill-contract.test.ts +++ b/tests/review-skill-contract.test.ts @@ -1253,6 +1253,16 @@ describe("cross-model peer skip legibility", () => { ) }) + test("code review exclusivity pointers allow in-process restore after a failed same-route rate-limit retry", async () => { + const skill = await readRepoFile("skills/ce-code-review/SKILL.md") + const dispatch = await readRepoFile( + "skills/ce-code-review/references/dispatch-reviewers.md", + ) + + expect(skill).toMatch(/failed same-route rate-limit retry/) + expect(dispatch).toMatch(/failed same-route rate-limit retry/) + }) + // A restricted host sandbox (e.g. a Codex task with network disabled) denies // the spawned peer CLI network/keychain, producing the exact same // `Not logged in` signal as a genuine account logout. The classifier surfaces