From 2343c6bc541ac2ffe330889c285e448df4783a16 Mon Sep 17 00:00:00 2001 From: Bunty Date: Fri, 7 Aug 2026 15:39:44 -0400 Subject: [PATCH] fix(code-review): short-circuit exhausted peer routes Preflight Claude authentication and persist a privacy-safe, reset-aware session-quota circuit so later reviews exclude a known-exhausted route before packaging or egress. Keep transient 429, execution-context auth, and transport failures distinct while preserving fixed-recipient behavior. Fixes #1343 Co-Authored-By: Codex Agent-Provenance: host=conductor harness=codex model=gpt-5.6-sol --- skills/ce-code-review/SKILL.md | 2 +- .../references/cross-model-review.md | 9 +- .../scripts/cross-model-adversarial-review.sh | 41 +- .../scripts/peer-route-health.py | 398 ++++++++++++++++++ tests/review-skill-contract.test.ts | 26 +- .../ce-code-review-cross-model-routes.test.ts | 124 +++++- 6 files changed, 591 insertions(+), 9 deletions(-) create mode 100755 skills/ce-code-review/scripts/peer-route-health.py diff --git a/skills/ce-code-review/SKILL.md b/skills/ce-code-review/SKILL.md index e6f4895f4..b2b35cf20 100644 --- a/skills/ce-code-review/SKILL.md +++ b/skills/ce-code-review/SKILL.md @@ -474,7 +474,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 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. +- 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, or a known session-quota circuit remains open), 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. When a job ID is returned and task tracking is active, add a distinct task that names the independent cross-model adversarial review. Keep it in progress while the detached job runs, then record its terminal outcome when the artifact is collected. Never create this task before a peer starts or leave it behind when the local adversarial fallback runs. diff --git a/skills/ce-code-review/references/cross-model-review.md b/skills/ce-code-review/references/cross-model-review.md index 57189ce0d..0eb365697 100644 --- a/skills/ce-code-review/references/cross-model-review.md +++ b/skills/ce-code-review/references/cross-model-review.md @@ -33,7 +33,7 @@ Resolve the preference in this order: 3. A preference already in your **project instructions** (the active instructions in your context) — consumed from context, **never** read from a named file. 4. **Default:** first available attested-different target in `codex → claude → grok → composer`; Cursor-default participates only when explicitly preferred. -Before egress, resolve the target to one concrete installed route, verify every recipient against `CROSS_MODEL_PEERS`, announce it, and pass it as `CROSS_MODEL_FIXED_ROUTE`. `CROSS_MODEL_FIXED_ROUTE` accepts exactly these tokens — the worker fail-closes on anything else (including route-shaped guesses like `codex-cli`): +Before egress, resolve the target to one concrete installed route, verify every recipient against `CROSS_MODEL_PEERS`, announce it, and pass it as `CROSS_MODEL_FIXED_ROUTE`. Availability includes the executable, a route-specific authentication preflight where the CLI exposes one, and the private reset-aware route-health circuit. A known-exhausted route is excluded before the reviewed diff is packaged; an explicitly fixed route that becomes unavailable fails clearly and never silently switches recipients. `CROSS_MODEL_FIXED_ROUTE` accepts exactly these tokens — the worker fail-closes on anything else (including route-shaped guesses like `codex-cli`): | Target | Route token(s) | |--------|----------------| @@ -43,7 +43,7 @@ Before egress, resolve the target to one concrete installed route, verify every | `cursor` | `cursor` | | `composer` | `composer` | -A failed route returns no artifact and never changes provider or intermediary internally. A retry is a new disclosed and sanctioned dispatch. For backward compatibility, either `cursor` or `composer` in `CROSS_MODEL_PEERS` sanctions Cursor as an intermediary, but selecting Cursor-default requires target `cursor`; `grok` alone never sanctions Grok-via-Cursor. +A failed route returns no artifact and never changes provider or intermediary internally. A retry is a new disclosed and sanctioned dispatch. Claude exposes authentication state but no zero-cost quota-remaining status, so the first newly exhausted session can be learned only from an API-bearing request. When that request returns session-quota evidence, the worker persists only the route, failure class, observation epoch, and reset epoch under the private peer-state root. Later invocations exclude that route before prompt packaging, re-check the circuit immediately before egress, and retry only after the recorded reset. A plain 429 remains a transient rate limit; execution-context authentication and network-transport failures are classified separately and do not open the session-quota circuit. For backward compatibility, either `cursor` or `composer` in `CROSS_MODEL_PEERS` sanctions Cursor as an intermediary, but selecting Cursor-default requires target `cursor`; `grok` alone never sanctions Grok-via-Cursor. `CROSS_MODEL_PEERS` is an optional restriction: when unset, it leaves the resolved route unfiltered and this skill invocation plus the concrete pre-egress disclosure sanctions that route; when set, the selected target/intermediary must appear. Use this contract directly. Do not inspect the worker source to rediscover its allowlist behavior. @@ -94,10 +94,13 @@ PY="$(for c in python3 python py; do command -v "$c" >/dev/null 2>&1 && "$c" -c ```bash SKILL_DIR=""; PY="$(for c in python3 python py; do command -v "$c" >/dev/null 2>&1 && "$c" -c '' >/dev/null 2>&1 && { echo "$c"; break; }; done)"; [ -n "$PY" ] || { echo "no working Python 3 interpreter on PATH" >&2; exit 1; }; +"$PY" "$SKILL_DIR/scripts/peer-route-health.py" preflight --route "" >/dev/null || { echo "peer-route-unavailable=" >&2; exit 3; }; echo "peer-deadline-secs=$(( ${CROSS_MODEL_HARD_SECS:-1200} + 10 ))"; CE_PEER_HARD_SECS= CROSS_MODEL_HOST_HARNESS="" CROSS_MODEL_FIXED_ROUTE="" "$PY" "$SKILL_DIR/scripts/peer-job-runner.py" start --skill ce-code-review --run-id "" --label adversarial -- env CROSS_MODEL_HOST_HARNESS="" CROSS_MODEL_FIXED_ROUTE="" bash "$SKILL_DIR/scripts/cross-model-adversarial-review.sh" "" "" "" "" ``` +The preflight and runner start stay in the same shell call. A failed preflight returns before any job ID, review-prompt packaging, or egress, so Stage 3d keeps the in-process `adversarial-reviewer` fallback. The worker repeats the preflight immediately before egress to close the race where another review learns a quota reset after this outer check. + The nested windows are one budget with one knob, `CROSS_MODEL_HARD_SECS`. The runner derives its supervisor hard window from that ambient knob automatically (`max(1230, knob + 30)`). Clear `CE_PEER_HARD_SECS` on the start prefix (`CE_PEER_HARD_SECS=`) so a stale ambient value from an earlier session or harness export cannot undercut that derivation — an explicit numeric `CE_PEER_HARD_SECS` still wins when a skill deliberately sets one (ce-work / elevation), which this path must not do. Print the orchestrator deadline as `knob + 10` in the same shell as `start` (as above) and use that printed `peer-deadline-secs=` below; never hardcode it, because a literal survives a knob change and then reaps a healthy peer. **Do not forward `CROSS_MODEL_HARD_SECS` to the worker.** The runner already passes the ambient environment through, so a knob the user actually set reaches the worker on its own. Re-exporting the orchestrator's *resolved* value would convert a fallback into an explicit override and destroy the one distinction the worker still needs: idle-guarded routes (codex + streaming claude/cursor-family) use the raised `HARD_SECS` default, while `grok-cli` keeps the lower `UNGUARDED_HARD_SECS` bound because its `--json-schema` path cannot stream. Forcing one value would silently restore the doubled hang on that hard-only route. @@ -149,7 +152,7 @@ Repeat that call until the job is terminal or the derived deadline is spent; do - **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. - 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 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). Session-quota evidence opens the private reset-aware circuit on its first observation; do not retry that route before its recorded reset. A retry through another route requires a newly resolved, disclosed, and sanctioned fixed recipient; never silently continue to another provider. - 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/scripts/cross-model-adversarial-review.sh b/skills/ce-code-review/scripts/cross-model-adversarial-review.sh index 84e9d6684..23529caf6 100755 --- a/skills/ce-code-review/scripts/cross-model-adversarial-review.sh +++ b/skills/ce-code-review/scripts/cross-model-adversarial-review.sh @@ -311,11 +311,46 @@ esac SKILL_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" || skip "cannot resolve skill root; skipping" PERSONA="$SKILL_ROOT/references/personas/adversarial-reviewer.md" SCHEMA="$SKILL_ROOT/references/findings-schema.json" +ROUTE_HEALTH="$SKILL_ROOT/scripts/peer-route-health.py" [ -f "$PERSONA" ] || skip "persona brief not found at $PERSONA; skipping" [ -f "$SCHEMA" ] || skip "findings schema not found at $SCHEMA; skipping" +[ -f "$ROUTE_HEALTH" ] || skip "route-health helper not found at $ROUTE_HEALTH; skipping" SCHEMA_CONTENT="$(cat "$SCHEMA")" || skip "cannot read findings schema; skipping" SCHEMA_REF="$SCHEMA_CONTENT" +PY="$(for c in python3 python py; do command -v "$c" >/dev/null 2>&1 && "$c" -c '' >/dev/null 2>&1 && { echo "$c"; break; }; done)" +[ -n "$PY" ] || skip "no working Python 3 interpreter on PATH; skipping" + +route_preflight_available() { + local route="$1" status + $PY "$ROUTE_HEALTH" preflight --route "$route" >/dev/null + status=$? + if [ "$status" -eq 0 ]; then + return 0 + fi + if [ "$status" -eq 3 ]; then + log "route '$route' is unavailable until its recorded session-quota reset; skipping before review payload packaging" + else + log "route '$route' health or authentication state could not be verified; skipping before review payload packaging" + fi + return 1 +} + +record_route_failure() { + local route="$1" receipt class opened + shift + [ "$route" = "claude" ] || return 0 + receipt="$($PY "$ROUTE_HEALTH" record --route "$route" \ + --evidence-file "$1" --evidence-file "$2" 2>/dev/null)" || { + log "route '$route' failure class could not be persisted" + return 0 + } + class="$(printf '%s' "$receipt" | jq -r '.failure_class // "other"' 2>/dev/null)" + opened="$(printf '%s' "$receipt" | jq -r '.circuit_opened // false' 2>/dev/null)" + log "route '$route' failure class: $class" + [ "$opened" = true ] && log "route '$route' session-quota circuit opened until its recorded reset" +} + # --- derive repo root (read-only in-tree review) --------------------------- REPO_ROOT="$(git rev-parse --show-toplevel 2>/dev/null)" || skip "not inside a git repository; skipping" PEER_WORKDIR="$REPO_ROOT" @@ -341,7 +376,7 @@ cursor_egress_ok() { [ -z "$ALLOW" ] || in_csv cursor "$ALLOW" || in_csv compose provider_available() { case "$1" in codex) command -v codex >/dev/null 2>&1 ;; - claude) command -v claude >/dev/null 2>&1 ;; + claude) command -v claude >/dev/null 2>&1 && route_preflight_available claude ;; grok) command -v grok >/dev/null 2>&1 || { cursor_egress_ok && command -v cursor-agent >/dev/null 2>&1; } ;; cursor) command -v cursor-agent >/dev/null 2>&1 ;; composer) command -v cursor-agent >/dev/null 2>&1 ;; @@ -855,6 +890,9 @@ run_provider() { fi primary="$fixed" validate_model_override "$primary" || { log "model override '${CROSS_MODEL_MODEL_OVERRIDE:-}' not compatible with route '$primary'; skipping"; rm -f "$OUT"; return 0; } + # A concurrent review may have learned that this route is exhausted after + # discovery. Re-check immediately before prompt composition / provider egress. + route_preflight_available "$primary" || { rm -f "$OUT"; return 0; } ACTUAL_ROUTE="$primary" attempt_route "$provider" "$primary" @@ -908,6 +946,7 @@ 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" + record_route_failure "$ACTUAL_ROUTE" "$PEERLOG" "$PEERERR" # 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 diff --git a/skills/ce-code-review/scripts/peer-route-health.py b/skills/ce-code-review/scripts/peer-route-health.py new file mode 100755 index 000000000..241023b24 --- /dev/null +++ b/skills/ce-code-review/scripts/peer-route-health.py @@ -0,0 +1,398 @@ +#!/usr/bin/env python3 +"""Privacy-safe route-health circuit breaker for cross-model peer review.""" + +from __future__ import annotations + +import argparse +import contextlib +import json +import os +import re +import shutil +import stat +import subprocess +import sys +import tempfile +import time +from datetime import datetime, timedelta +from pathlib import Path +from zoneinfo import ZoneInfo, ZoneInfoNotFoundError + + +STATE_CAP = 64 * 1024 +EVIDENCE_CAP = 256 * 1024 +DEFAULT_QUOTA_TTL = 3600 +MAX_QUOTA_TTL = 24 * 3600 +SAFE_ROUTE = re.compile(r"^[A-Za-z0-9._-]+$") +RESET_AT = re.compile( + r"\bresets?\s+(?:at\s+)?(\d{1,2}):(\d{2})\s*(am|pm)" + r"(?:\s*\(([^)]+)\))?", + re.IGNORECASE, +) +RESET_IN = re.compile( + r"\bresets?\s+in\s+(\d+)\s*(minutes?|mins?|hours?|hrs?)\b", + re.IGNORECASE, +) + + +class HealthError(Exception): + pass + + +def _effective_uid() -> int | None: + getter = getattr(os, "geteuid", None) or getattr(os, "getuid", None) + return getter() if getter else None + + +def _default_root() -> Path: + configured = os.environ.get("CROSS_MODEL_ROUTE_HEALTH_ROOT") + if configured: + return Path(os.path.abspath(os.path.expanduser(configured))) + peer_root = os.environ.get("CE_PEER_JOBS_ROOT") + if peer_root: + return ( + Path(os.path.abspath(os.path.expanduser(peer_root))) + / "ce-code-review" + / "route-health" + ) + if sys.platform == "win32": + base = os.environ.get("LOCALAPPDATA") or tempfile.gettempdir() + return Path(base) / "compound-engineering-jobs" / "ce-code-review" / "route-health" + uid = _effective_uid() + if uid is None: + raise HealthError("effective user ID is unavailable") + return Path("/tmp") / f"compound-engineering-{uid}" / "ce-code-review" / "route-health" + + +def _verify_private_dir(path: Path) -> None: + try: + info = path.lstat() + except FileNotFoundError: + path.mkdir(parents=True, mode=0o700) + info = path.lstat() + if stat.S_ISLNK(info.st_mode) or not stat.S_ISDIR(info.st_mode): + raise HealthError(f"unsafe route-health directory: {path}") + uid = _effective_uid() + if uid is not None and info.st_uid != uid: + raise HealthError(f"route-health directory is not owned by the current user: {path}") + if os.name != "nt": + os.chmod(path, 0o700) + if stat.S_IMODE(path.stat().st_mode) != 0o700: + raise HealthError(f"route-health directory is not private: {path}") + + +def _verify_existing_private_dir(path: Path) -> bool: + try: + info = path.lstat() + except FileNotFoundError: + return False + if stat.S_ISLNK(info.st_mode) or not stat.S_ISDIR(info.st_mode): + raise HealthError(f"unsafe route-health directory: {path}") + uid = _effective_uid() + if uid is not None and info.st_uid != uid: + raise HealthError(f"route-health directory is not owned by the current user: {path}") + if os.name != "nt" and stat.S_IMODE(info.st_mode) != 0o700: + raise HealthError(f"route-health directory is not private: {path}") + return True + + +def _open_owned_regular(path: Path, cap: int) -> bytes: + flags = os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0) + fd = os.open(path, flags) + try: + info = os.fstat(fd) + uid = _effective_uid() + if not stat.S_ISREG(info.st_mode) or (uid is not None and info.st_uid != uid): + raise HealthError(f"unsafe file: {path}") + if info.st_size > cap: + raise HealthError(f"file exceeds {cap} byte cap: {path}") + return os.read(fd, cap + 1) + finally: + os.close(fd) + + +@contextlib.contextmanager +def _locked(root: Path): + _verify_private_dir(root) + lock_path = root / "state.lock" + fd = os.open( + lock_path, + os.O_RDWR | os.O_CREAT | getattr(os, "O_NOFOLLOW", 0), + 0o600, + ) + acquired = False + try: + lock_info = os.fstat(fd) + uid = _effective_uid() + if not stat.S_ISREG(lock_info.st_mode) or ( + uid is not None and lock_info.st_uid != uid + ): + raise HealthError(f"unsafe route-health lock: {lock_path}") + if os.name == "nt": + import msvcrt + + if os.fstat(fd).st_size == 0: + os.write(fd, b"0") + os.lseek(fd, 0, os.SEEK_SET) + msvcrt.locking(fd, msvcrt.LK_LOCK, 1) + else: + import fcntl + + fcntl.flock(fd, fcntl.LOCK_EX) + acquired = True + yield + finally: + if acquired and os.name == "nt": + import msvcrt + + os.lseek(fd, 0, os.SEEK_SET) + msvcrt.locking(fd, msvcrt.LK_UNLCK, 1) + elif acquired: + import fcntl + + fcntl.flock(fd, fcntl.LOCK_UN) + os.close(fd) + + +def _load(root: Path) -> dict: + path = root / "state.json" + if not path.exists(): + return {"version": 1, "routes": {}} + try: + value = json.loads(_open_owned_regular(path, STATE_CAP)) + except (HealthError, json.JSONDecodeError, UnicodeDecodeError) as exc: + raise HealthError(f"route-health state is unreadable: {exc}") from exc + if not isinstance(value, dict) or not isinstance(value.get("routes"), dict): + raise HealthError("route-health state has an invalid shape") + return value + + +def _store(root: Path, state: dict) -> None: + payload = json.dumps(state, sort_keys=True, separators=(",", ":")).encode() + if len(payload) > STATE_CAP: + raise HealthError("route-health state exceeds its byte cap") + fd, temporary = tempfile.mkstemp(prefix="state.", suffix=".tmp", dir=root) + try: + os.chmod(temporary, 0o600) + with os.fdopen(fd, "wb") as handle: + handle.write(payload) + handle.flush() + os.fsync(handle.fileno()) + os.replace(temporary, root / "state.json") + finally: + try: + os.unlink(temporary) + except FileNotFoundError: + pass + + +def _now(value: float | None) -> float: + if value is not None: + return value + override = os.environ.get("CROSS_MODEL_ROUTE_HEALTH_NOW_EPOCH") + try: + return float(override) if override else time.time() + except ValueError: + return time.time() + + +def _prune(state: dict, now: float) -> bool: + routes = state["routes"] + expired = [] + for route, entry in routes.items(): + if not isinstance(entry, dict): + expired.append(route) + continue + try: + retry_after = float(entry.get("retry_after_epoch", 0)) + except (TypeError, ValueError): + expired.append(route) + continue + if retry_after <= now: + expired.append(route) + for route in expired: + routes.pop(route, None) + return bool(expired) + + +def _classify(text: str) -> str: + lowered = text.lower() + if "session limit" in lowered or "usage limit" in lowered: + return "session_quota" + if ( + "not logged in" in lowered + or "please log in" in lowered + or "please login" in lowered + or "api_error_status=401" in lowered + or '"api_error_status":401' in lowered + ): + return "execution_context_auth" + if ( + "rate limit" in lowered + or "too many requests" in lowered + or "api_error_status=429" in lowered + or '"api_error_status":429' in lowered + ): + return "transient_rate_limit" + if any( + phrase in lowered + for phrase in ( + "network error", + "connection refused", + "connection reset", + "could not resolve host", + "name or service not known", + "tls handshake", + "timed out", + ) + ): + return "network_transport" + return "other" + + +def _retry_after(text: str, now: float) -> float: + relative = RESET_IN.search(text) + if relative: + amount = int(relative.group(1)) + seconds = amount * (3600 if relative.group(2).lower().startswith(("hour", "hr")) else 60) + return now + min(MAX_QUOTA_TTL, max(60, seconds)) + 60 + + clock = RESET_AT.search(text) + if clock: + hour = int(clock.group(1)) % 12 + if clock.group(3).lower() == "pm": + hour += 12 + zone_name = clock.group(4) + try: + zone = ZoneInfo(zone_name) if zone_name else datetime.now().astimezone().tzinfo + except ZoneInfoNotFoundError: + zone = datetime.now().astimezone().tzinfo + current = datetime.fromtimestamp(now, zone) + candidate = current.replace(hour=hour, minute=int(clock.group(2)), second=0, microsecond=0) + if candidate.timestamp() <= now + 60: + candidate += timedelta(days=1) + return min(now + MAX_QUOTA_TTL, candidate.timestamp() + 60) + + try: + fallback = int(os.environ.get("CROSS_MODEL_SESSION_QUOTA_TTL_SECS", DEFAULT_QUOTA_TTL)) + except ValueError: + fallback = DEFAULT_QUOTA_TTL + return now + min(MAX_QUOTA_TTL, max(60, fallback)) + + +def cmd_check(args: argparse.Namespace) -> int: + now = _now(args.now_epoch) + root = _default_root() + # State publishes atomically, so availability checks can remain read-only: + # they observe either the old complete file or the new complete file. This + # preserves the worker's no-side-effects dry-run contract and keeps the + # final pre-egress recheck cheap. + if not _verify_existing_private_dir(root): + print(json.dumps({"available": True}, separators=(",", ":"))) + return 0 + state = _load(root) + entry = state["routes"].get(args.route) + if isinstance(entry, dict): + try: + if float(entry.get("retry_after_epoch", 0)) <= now: + entry = None + except (TypeError, ValueError): + raise HealthError("route-health state has an invalid retry epoch") + if entry: + print(json.dumps({"available": False, **entry}, separators=(",", ":"))) + return 3 + print(json.dumps({"available": True}, separators=(",", ":"))) + return 0 + + +def cmd_preflight(args: argparse.Namespace) -> int: + circuit_args = argparse.Namespace(route=args.route, now_epoch=args.now_epoch) + if cmd_check(circuit_args) != 0: + print( + f"route {args.route!r} is blocked by its recorded session-quota reset", + file=sys.stderr, + ) + return 3 + if args.route != "claude": + return 0 + executable = shutil.which("claude") + if not executable: + print("route 'claude' has no installed CLI", file=sys.stderr) + return 4 + try: + completed = subprocess.run( + [executable, "auth", "status", "--json"], + check=False, + capture_output=True, + timeout=10, + ) + status = json.loads(completed.stdout) if completed.returncode == 0 else {} + except (OSError, subprocess.SubprocessError, json.JSONDecodeError): + status = {} + if not isinstance(status, dict) or status.get("loggedIn") is not True: + print( + "route 'claude' execution-context authentication preflight failed", + file=sys.stderr, + ) + return 4 + return 0 + + +def cmd_record(args: argparse.Namespace) -> int: + now = _now(args.now_epoch) + chunks: list[str] = [] + for name in args.evidence_file: + try: + chunks.append(_open_owned_regular(Path(name), EVIDENCE_CAP).decode("utf-8", "replace")) + except (FileNotFoundError, HealthError): + continue + text = "\n".join(chunks) + failure_class = _classify(text) + response: dict[str, object] = {"failure_class": failure_class, "circuit_opened": False} + if failure_class == "session_quota": + retry_after = _retry_after(text, now) + entry = { + "failure_class": failure_class, + "observed_at_epoch": int(now), + "retry_after_epoch": int(retry_after), + } + root = _default_root() + with _locked(root): + state = _load(root) + _prune(state, now) + state["routes"][args.route] = entry + _store(root, state) + response.update(entry) + response["circuit_opened"] = True + print(json.dumps(response, separators=(",", ":"))) + return 0 + + +def main() -> int: + parser = argparse.ArgumentParser() + sub = parser.add_subparsers(dest="command", required=True) + check = sub.add_parser("check") + check.add_argument("--route", required=True) + check.add_argument("--now-epoch", type=float) + check.set_defaults(handler=cmd_check) + preflight = sub.add_parser("preflight") + preflight.add_argument("--route", required=True) + preflight.add_argument("--now-epoch", type=float) + preflight.set_defaults(handler=cmd_preflight) + record = sub.add_parser("record") + record.add_argument("--route", required=True) + record.add_argument("--evidence-file", action="append", default=[]) + record.add_argument("--now-epoch", type=float) + record.set_defaults(handler=cmd_record) + args = parser.parse_args() + if not SAFE_ROUTE.fullmatch(args.route): + parser.error("route must match [A-Za-z0-9._-]+") + try: + return args.handler(args) + except HealthError as exc: + print(str(exc), file=sys.stderr) + return 1 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tests/review-skill-contract.test.ts b/tests/review-skill-contract.test.ts index b168f9bbb..5d97e4035 100644 --- a/tests/review-skill-contract.test.ts +++ b/tests/review-skill-contract.test.ts @@ -1125,6 +1125,26 @@ describe("cross-model peer skip legibility", () => { }, ] + test("code review preflights the fixed route before creating a peer job", async () => { + const reference = await readRepoFile( + "skills/ce-code-review/references/cross-model-review.md", + ) + const skill = await readRepoFile("skills/ce-code-review/SKILL.md") + + const preflight = reference.indexOf( + 'peer-route-health.py" preflight --route ""', + ) + const start = reference.indexOf('peer-job-runner.py" start') + expect(preflight).toBeGreaterThanOrEqual(0) + expect(start).toBeGreaterThan(preflight) + expect(reference).toMatch(/failed preflight returns before any job ID/i) + expect(reference).toMatch(/keeps the in-process `adversarial-reviewer` fallback/i) + expect(reference).toMatch(/worker repeats the preflight immediately before egress/i) + + expect(skill).toMatch(/known session-quota circuit remains open/i) + expect(skill).toMatch(/keep `adversarial-reviewer` in the local roster as the fallback/i) + }) + // The route-token vocabulary lives in each worker's route_target() case, but // the references forbid inspecting worker source — so each reference must // enumerate every accepted fixed-route token itself (issue #1282: an @@ -1189,7 +1209,11 @@ 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(referenceSrc).toMatch(/opens the private reset-aware circuit on its first observation/i) + } else { + expect(referenceSrc).toMatch(/more than once in this session/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 233ced4f4..8dc739c08 100644 --- a/tests/skills/ce-code-review-cross-model-routes.test.ts +++ b/tests/skills/ce-code-review-cross-model-routes.test.ts @@ -103,7 +103,8 @@ function sandbox( providers: string[], stubBody = "#!/bin/sh\nexit 0\n", ): { bin: string; env: NodeJS.ProcessEnv } { - const bin = path.join(mkTempRoot("xmodel-cr-sandbox-"), "bin") + const root = mkTempRoot("xmodel-cr-sandbox-") + const bin = path.join(root, "bin") mkdirSync(bin, { recursive: true }) for (const [tool, real] of realToolPaths()) { if (existsSync(path.join(bin, tool))) continue @@ -115,10 +116,25 @@ function sandbox( } for (const p of providers) { const f = path.join(bin, p) - writeFileSync(f, stubBody) + const body = p === "claude" + ? `#!/bin/sh +if [ "\${1:-}" = auth ] && [ "\${2:-}" = status ]; then + printf '%s' '{"loggedIn":true}' + exit 0 +fi +${stubBody.replace(/^#![^\n]*\n/, "")}` + : stubBody + writeFileSync(f, body) chmodSync(f, 0o755) } - return { bin, env: { ...process.env, PATH: bin } } + return { + bin, + env: { + ...process.env, + PATH: bin, + CROSS_MODEL_ROUTE_HEALTH_ROOT: path.join(root, "route-health"), + }, + } } function makeRunDir(): string { @@ -425,6 +441,36 @@ printf '%s' '{"structured_output":{"reviewer":"adversarial","findings":[],"resid }) describe("cross-model-adversarial-review provider selection", () => { + test("dry-run route resolution reads health without creating persistent state", () => { + const { env } = sandbox(["claude"]) + const healthRoot = env.CROSS_MODEL_ROUTE_HEALTH_ROOT! + const runDir = makeRunDir() + const result = run(["codex", "claude", "HEAD", runDir], runDir, { + ...env, + CROSS_MODEL_DRY_RUN: "1", + }) + expect(result.stdout).toContain("RESOLVED_PEERS: claude") + expect(existsSync(healthRoot)).toBe(false) + }) + + test("Claude authentication is checked before any review payload is sent", () => { + const invoked = path.join(mkTempRoot("xmodel-cr-auth-invoked-"), "marker") + const { bin, env } = sandbox(["claude"]) + writeFileSync(path.join(bin, "claude"), `#!/bin/sh +if [ "\${1:-}" = auth ] && [ "\${2:-}" = status ]; then + printf '%s' '{"loggedIn":false}' + exit 0 +fi +: > '${invoked}' +`) + chmodSync(path.join(bin, "claude"), 0o755) + const runDir = makeRunDir() + const result = run(["codex", "claude", "HEAD", runDir], runDir, env) + expect(result.stderr).toContain("execution-context authentication preflight failed") + expect(existsSync(invoked)).toBe(false) + expect(result.files).not.toContain("adversarial-claude.json") + }) + test("default order excludes the host and picks the first available peer", () => { const all = ["codex", "claude", "grok", "cursor-agent"] expect(resolvePeers("claude", "codex,claude,grok,composer", all)).toBe("codex") @@ -577,6 +623,78 @@ 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("route 'claude' failure class: execution_context_auth") + }) + + test("a learned Claude session quota blocks the next review before payload packaging", () => { + const invoked = path.join(mkTempRoot("xmodel-cr-quota-invoked-"), "marker") + const observedAt = Math.floor(new Date("2026-08-07T15:00:00-04:00").getTime() / 1000) + const beforeReset = Math.floor(new Date("2026-08-07T15:30:00-04:00").getTime() / 1000) + const afterReset = Math.floor(new Date("2026-08-07T15:51:01-04:00").getTime() / 1000) + const payload = JSON.stringify({ + result: "You've hit your session limit · resets 3:50pm (America/New_York)", + api_error_status: 429, + terminal_reason: "api_error", + }) + const { env } = sandbox( + ["claude"], + `#!/bin/sh\nprintf 'x\n' >> '${invoked}'\ncat >/dev/null\nprintf '%s' '${payload.replace(/'/g, `'\\''`)}'\nexit 1\n`, + ) + const firstDir = makeRunDir() + const first = run(["codex", "claude", "HEAD", firstDir], firstDir, { + ...env, + CROSS_MODEL_ROUTE_HEALTH_NOW_EPOCH: String(observedAt), + }) + expect(first.stderr).toContain("failure class: session_quota") + expect(first.stderr).toContain("session-quota circuit opened") + const stateText = readFileSync( + path.join(env.CROSS_MODEL_ROUTE_HEALTH_ROOT!, "state.json"), + "utf8", + ) + const state = JSON.parse(stateText) + expect(state.routes.claude.failure_class).toBe("session_quota") + expect(state.routes.claude.retry_after_epoch).toBe( + Math.floor(new Date("2026-08-07T15:51:00-04:00").getTime() / 1000), + ) + expect(stateText).not.toContain("session limit") + + const secondDir = makeRunDir() + const second = run(["codex", "claude", "HEAD", secondDir], secondDir, { + ...env, + CROSS_MODEL_ROUTE_HEALTH_NOW_EPOCH: String(beforeReset), + }) + expect(second.stderr).toContain("skipping before review payload packaging") + expect(readFileSync(invoked, "utf8").trim().split("\n")).toHaveLength(1) + expect(second.files).not.toContain("adversarial-claude.json") + + const afterResetDir = makeRunDir() + run(["codex", "claude", "HEAD", afterResetDir], afterResetDir, { + ...env, + CROSS_MODEL_ROUTE_HEALTH_NOW_EPOCH: String(afterReset), + }) + expect(readFileSync(invoked, "utf8").trim().split("\n")).toHaveLength(2) + }) + + test("a transient 429 is classified but does not open the session-quota circuit", () => { + const invoked = path.join(mkTempRoot("xmodel-cr-rate-invoked-"), "marker") + const payload = JSON.stringify({ + result: "Rate limit exceeded; retry shortly", + api_error_status: 429, + terminal_reason: "api_error", + }) + const { env } = sandbox( + ["claude"], + `#!/bin/sh\nprintf 'x\n' >> '${invoked}'\ncat >/dev/null\nprintf '%s' '${payload}'\nexit 1\n`, + ) + for (const now of ["2000", "2001"]) { + const runDir = makeRunDir() + const result = run(["codex", "claude", "HEAD", runDir], runDir, { + ...env, + CROSS_MODEL_ROUTE_HEALTH_NOW_EPOCH: now, + }) + expect(result.stderr).toContain("failure class: transient_rate_limit") + } + expect(readFileSync(invoked, "utf8").trim().split("\n")).toHaveLength(2) }) test("ancillary structured fields do not hide an unrecognized human-readable diagnostic", () => {