diff --git a/.agents/skills/aicr-cross-review/SKILL.md b/.agents/skills/aicr-cross-review/SKILL.md index 6a538b4e9..1ad6a6e5d 100644 --- a/.agents/skills/aicr-cross-review/SKILL.md +++ b/.agents/skills/aicr-cross-review/SKILL.md @@ -442,8 +442,23 @@ the consensus mechanics): a resume that comes back unavailable again, confirms the job is dead and the review must be re-run rather than resumed. For a live job: poll it with the companion status command until it is terminal (a background 60-second loop is - fine — polling is cheap once the workflow is no longer holding a lane open for it), - then resume: `Workflow({scriptPath, resumeFromRunId: "", args: {...prevArgs, + fine — polling is cheap once the workflow is no longer holding a lane open for it). + + Resolve the companion the same way the lane does, with `-t` — the lane's messages + refer to `$comp` but cannot export it across Bash calls: + + ```bash + comp=$(ls -t ~/.claude/plugins/cache/openai-codex/codex/*/scripts/codex-companion.mjs | head -1) + node "$comp" status --cwd "" --json + ``` + + `ls -t` picks the most recently installed companion, which is the version the plugin + system has active. Dropping the `-t` sorts by version *name* instead and can select a + stale cached copy — polling a job with a different companion version than dispatched it + returns misleading results (`status` finding a job that `result` then reports as + unknown), which reads exactly like a dead job and is not one. + + Then resume: `Workflow({scriptPath, resumeFromRunId: "", args: {...prevArgs, codexResumeJobId: ""}})` — `prevArgs` is the previous run's args object, unchanged. The three completed lanes replay from cache, the Codex dispatch agent is skipped entirely (the workflow logs `Codex resume: waiting on existing job `), the wait agent collects the existing @@ -492,12 +507,16 @@ the consensus mechanics): - CodeRabbit slow runs: check the newest file in `~/.coderabbit/logs/` (429/queue lines mean cloud-side queueing) and confirm `which -a coderabbit` resolves to the brew-managed binary — a stale `~/.local/bin` copy shadows it. -- **A sandboxed CodeRabbit run hangs instead of failing.** `~/.coderabbit` is outside the - default sandbox write allowlist, so the CLI cannot create its log or review store; it - stalls at `connecting_to_review_service` until the timebox kills it. The lane therefore - **probes** in step 1 whether `~/.coderabbit` is sandbox-writable, and runs the - `coderabbit` command (and only that command) with sandbox bypass exactly when the probe - says it is not. +- **A sandboxed CodeRabbit run hangs instead of failing.** The sandbox can deny the CLI + two independent ways, and both stall at `connecting_to_review_service` until the + timebox kills it: `~/.coderabbit` outside the write allowlist (the CLI cannot create + its log or review store), or the `coderabbit.ai` hosts outside the allowed-hosts list. + The lane therefore **probes both** in step 1 — writability of `~/.coderabbit` *and* + reachability of both CLI hosts, `cli.coderabbit.ai` (startup config fetch) and + `ide.coderabbit.ai` (the review session's WebSocket) — and runs the `coderabbit` + command (and only that command) with sandbox bypass when any check fails. The hosts + are probed separately because the allowlist is per-host: an entry naming only the + config host passes the first check and still hangs step 2 on the WebSocket connect. **Why probe-gated bypass rather than an allowlist assumption.** Adding `~/.coderabbit` to the sandbox write allowlist is the narrower grant, but this skill is checked into @@ -508,17 +527,25 @@ the consensus mechanics): timebox per wrong guess. The step-1 probe settles it in milliseconds: machines with the allowlist entry run step 2 fully sandboxed and pay **no bypass prompt at all**; every other machine gets the bypass from the start, portable and self-documenting at - the call site. Add `~/.coderabbit` (and `~/.claude/plugins/data`, for the Codex - companion's job log) to your local sandbox `filesystem.allowWrite` if you run this - often — that is what removes the per-round approval prompts — but the skill must not - depend on it. - - **Diagnose it by absence:** a stall at `connecting` *with no new file in - `~/.coderabbit/logs/`* is sandbox denial — a process killed mid-run still flushes a - partial log, so zero bytes means it never created one. A real cloud problem leaves a log - with 429/queue lines. Do not read this stall as an outage or as contention with another - session: an unsandboxed run succeeding while a sandboxed one hangs looks exactly like - contention and is not. + the call site. If you run this often, add **both** grants — `~/.coderabbit` (and + `~/.claude/plugins/data`, for the Codex companion's job log) to your local sandbox + `filesystem.allowWrite`, *and* the `coderabbit.ai` hosts to the network allowlist — + since that pair is what removes the per-round approval prompts. The skill must not + depend on either. Granting only the filesystem half is worse than granting neither: + it satisfies the write probe while the network stays blocked, and an earlier + writability-only probe read that state as sandbox-clean and hung the lane for a full + ten-minute timebox. The probe now tests both for exactly this reason. + + **Diagnose it from the log directory**, since the two denials differ there. A stall at + `connecting` *with no new file in `~/.coderabbit/logs/`* is **filesystem** denial — a + process killed mid-run still flushes a partial log, so zero bytes means it never created + one. A stall *with* a log naming the allowlist (`403 Forbidden` on + `https://cli.coderabbit.ai/public-configs.json`, `"data":"Connection blocked by network + allowlist"`, then repeated `wss://ide.coderabbit.ai/ws` retries) is **network** denial — + do not read the presence of a log as proof the sandbox was not the cause. A real cloud + problem leaves a log with 429/queue lines instead. Do not read either stall as an outage + or as contention with another session: an unsandboxed run succeeding while a sandboxed + one hangs looks exactly like contention and is not. - **Persisted-store fallback.** If a run still fails, the lane checks `~/.coderabbit/reviews/*/*/reviews/*/git.json`, whichever session produced the record. Acceptance is `head` plus **the pinned change itself** — never `baseCommitId`. Measured: @@ -784,11 +811,13 @@ posting it) so no finding is left as a bare one-liner. - **Codex companion** — it writes its job log under `~/.claude/plugins/data`, which is sandbox-denied by default. If dispatch fails on that write, bypass for that call only; a machine whose sandbox allowlist covers the path never needs the bypass. - - **CodeRabbit review** — `~/.coderabbit` is outside the default write allowlist, so a - sandboxed CLI cannot create its log or review store and hangs at - `connecting_to_review_service` until the timebox kills it. Step 1 of the three-step - CodeRabbit protocol probes whether `~/.coderabbit` is sandbox-writable; when it is - not, bypass **step 2 only**, which is a lone `coderabbit review` command. When the + - **CodeRabbit review** — `~/.coderabbit` is outside the default write allowlist and + the `coderabbit.ai` hosts are outside the default network allowlist; under either + denial a sandboxed CLI hangs at `connecting_to_review_service` until the timebox + kills it. Step 1 of the three-step CodeRabbit protocol probes writability of + `~/.coderabbit` and reachability of both `cli.coderabbit.ai` and + `ide.coderabbit.ai` (the WebSocket host); when any check fails, bypass + **step 2 only**, which is a lone `coderabbit review` command. When the probe passes, step 2 runs sandboxed and no bypass happens at all. Worktree setup and cleanup live in steps 1 and 3 and stay sandboxed always. diff --git a/.agents/skills/aicr-cross-review/scripts/workflow.mjs b/.agents/skills/aicr-cross-review/scripts/workflow.mjs index 6c14949ab..d6e58e86a 100644 --- a/.agents/skills/aicr-cross-review/scripts/workflow.mjs +++ b/.agents/skills/aicr-cross-review/scripts/workflow.mjs @@ -428,9 +428,12 @@ STEP 1 — setup (SANDBOXED, normal Bash call): CRROOT=$(mktemp -d "\${TMPDIR:-/tmp}/cr-rabbit.XXXXXX") git -C "${repoPath}" worktree add --detach "$CRROOT/head" ${headSha} CRSBX=no - if { mkdir -p ~/.coderabbit && date +%s > ~/.coderabbit/.aicr-sandbox-probe; } 2>/dev/null; then CRSBX=yes; fi + if { mkdir -p ~/.coderabbit && date +%s > ~/.coderabbit/.aicr-sandbox-probe; } 2>/dev/null \ + && curl -fsS -o /dev/null -m 10 https://cli.coderabbit.ai/public-configs.json 2>/dev/null \ + && curl -fsS -o /dev/null -m 10 https://ide.coderabbit.ai/ 2>/dev/null; then CRSBX=yes; fi echo "CRROOT=$CRROOT"; echo "CR_SANDBOXED=$CRSBX" Record the echoed CRROOT and CR_SANDBOXED: shell variables do not survive between Bash calls, so steps 2 and 3 must use the literal values. The probe is wrapped in an if so it cannot trip set -e; it decides only whether STEP 2 needs sandbox bypass (see SANDBOX below). The probe file is a few bytes, overwritten on every run, and needs no cleanup. +The probe tests BOTH dimensions the sandbox can deny, because either one alone produces the same ten-minute hang and a filesystem pass does not imply a network pass. The write half covers the log and review store under ~/.coderabbit; the reachability half covers the allowlisted-hosts rule, and it tests BOTH hosts the CLI needs — the cli.coderabbit.ai config endpoint it fetches on startup, and ide.coderabbit.ai, the host its review session then connects to over wss://ide.coderabbit.ai/ws. One host does not vouch for the other: the allowlist is per-host, so an entry naming only the config host (the obvious edit for a reader who saw it in this probe) passes the first check and still hangs STEP 2 on the WebSocket connect. A plain HTTPS GET is the right test for the wss host — the allowlist rule is host-scoped, not protocol-scoped, and the site answers 200 at / while a sandbox block answers a proxy 403. -f is required on both: without it that 403 ("Connection blocked by network allowlist") is still exit 0 and the probe would report a blocked host as reachable. The checks are ANDed and the result is fail-safe in the cheap direction — a false no costs one approval prompt, a false yes costs the whole timebox. Do not drop either network check: a machine that has ~/.coderabbit in filesystem.allowWrite but no coderabbit.ai network entry is the exact configuration this skill's own SANDBOX guidance tells readers to create, and testing writability alone reported it sandbox-clean and hung the lane for the full 600000 ms. The find/prune pair is the bounded reaper for STEP 3's known leak. Two limits make the FIND half safe: it matches ONLY the cr-rabbit.* prefix this lane creates, and only entries older than 120 minutes — twelve times the 600000 ms (10 minute) cap a single review can occupy, so it can never reach another session's live worktree. Do not widen either limit. "find" is used rather than a glob because an unmatched glob aborts the command list under zsh; find prints nothing and exits 0. The whole reaper — find, the removal loop AND the prune — is wrapped so it cannot fail the call. Under set -e any of them aborts STEP 1 before the mktemp and the worktree add, and both halves have been seen to fail: an unremovable stale directory where TMPDIR is unset and /tmp is shared between users, and "git worktree prune" itself exiting "Operation not permitted" on .git/worktrees when the sandbox profile, computed at session start, does not cover a worktree created later in that session. Neither is a reason not to review. Only the reaper is neutralised; the worktree add below stays fatal, since STEP 2 must not review the wrong tree. The PRUNE half is repo-wide, not prefix-scoped — be precise about that rather than reading the two limits onto it. It is included to collect the admin entries whose directories the loop just removed, and it is near-harmless because prune only reaps entries whose directory is ALREADY gone. The residual case is an unrelated worktree whose directory is transiently absent (unmounted volume, an rm in flight): its entry would be pruned too. That is accepted; re-adding such a worktree is cheap, and the alternative is leaving this lane's own stale entries to accumulate. One more rough edge, cosmetic: a cr-rabbit.* directory left by a DIFFERENT clone is removed by the find -delete while its admin entry lives in that other repo, which "git -C ${repoPath} worktree remove" cannot reach. That entry is then collected by the other repo's own next prune, so it self-heals — but the removal is less clean than the within-repo case. @@ -438,7 +441,7 @@ Deletion is "find -xdev -depth -delete", never "rm -rf", throughout this lane STEP 2 — the review (the ONLY call that may run with dangerouslyDisableSandbox, and only when STEP 1 echoed CR_SANDBOXED=no): coderabbit review --agent --committed --base-commit ${baseSha} --dir "/head" -If STEP 1 echoed CR_SANDBOXED=yes, run this sandboxed like any other call — the probe proved ~/.coderabbit is sandbox-writable on this machine (a local allowlist entry), so there is nothing to bypass and no approval prompt to pay. If it echoed CR_SANDBOXED=no, run with dangerouslyDisableSandbox from the start. Never guess instead of reading the probe: a sandboxed run on a denied machine does not fail fast — it hangs for the full timebox (see SANDBOX below), so a wrong guess costs ten minutes. +If STEP 1 echoed CR_SANDBOXED=yes, run this sandboxed like any other call — the probe proved that ~/.coderabbit is sandbox-writable and that both CLI hosts (cli.coderabbit.ai and ide.coderabbit.ai) are reachable on this machine (local allowlist entries), so there is nothing to bypass and no approval prompt to pay. If it echoed CR_SANDBOXED=no, run with dangerouslyDisableSandbox from the start. Never guess instead of reading the probe: a sandboxed run on a denied machine does not fail fast — it hangs for the full timebox (see SANDBOX below), so a wrong guess costs ten minutes. Nothing else belongs in this call: no Git operation, no working-copy mutation, no GitHub write. Reading is not the boundary — this very command reads the detached worktree it is pointed at. What the bypass must never cover is a call that ACTS on the working copy or GitHub. Note its exit status rather than assuming success — a non-zero exit must still reach RECOVERY, which is the whole point of the fallback. Do NOT use "-t uncommitted". Timebox: give THIS call an explicit timeout of 600000 ms. The Bash tool defaults to 2 minutes and is capped at 10, so an unset or larger timeout silently kills the run. Steps 1 and 3 are fast and need no explicit timeout. Accept the run ONLY if its reported baseCommit equals ${baseSha}; otherwise discard it and return status:"unavailable" with what it reported. Ignore currentBranch, baseBranch, and workingDirectory — a detached worktree correctly reports currentBranch:"HEAD", the CLI's inferred baseBranch stays unrelated even when --base-commit is honored, and the commit pair is what identifies the reviewed context. @@ -451,11 +454,13 @@ STEP 3 carries no "set -euo pipefail": every command in it is already \`|| true\ There is no EXIT trap any more — a single invocation could carry one, three cannot. So STEP 3 is the only PROMPT cleanup there is: if you die between steps 2 and 3, the worktree leaks and STEP 3 will not run. Do not skip STEP 3 on the assumption that something else will. That is the accepted trade, and STEP 1's age-gated reaper is its backstop, not its excuse: it collects such a leak only on the NEXT run of this lane and only once the leak is two hours old, so between those points the worktree is really there. Phase 1's hygiene check COUNTS worktrees and stops to ask you — it deliberately does not remove any, since a clean detached-HEAD worktree may be another session's live review — and a bare \`git worktree prune\` walks past a fresh leak entirely, because prune only reaps admin entries whose directory is already gone. Leaking a worktree is still far better than running git unsandboxed, which has no backstop at all — but it is a real cost, not a free one. -SANDBOX — this is the single most common reason this lane fails, and it is NOT a CodeRabbit problem. ~/.coderabbit is outside the sandbox write allowlist, so a sandboxed CLI cannot create its log or its review store. It does not fail fast: it hangs at +SANDBOX — this is the single most common reason this lane fails, and it is NOT a CodeRabbit problem. The sandbox can deny the CLI in TWO independent ways, and each produces the identical symptom: a hang at {"type":"status","phase":"connecting","status":"connecting_to_review_service"} -until the timebox kills it, and writes NO log file at all. -Diagnose it by absence: if the run stalls at "connecting" and ~/.coderabbit/logs/ gained NO new file, it was sandbox-denied. A process killed mid-run still flushes a partial log — zero bytes means it never created one. A genuine cloud problem leaves a log containing 429 or queue lines. -Therefore STEP 1 probes whether ~/.coderabbit is sandbox-writable and STEP 2 — and only STEP 2 — runs with sandbox bypass (dangerouslyDisableSandbox) when the probe says CR_SANDBOXED=no; the observed evidence above is the justification the sandbox rules require. A machine whose sandbox allowlist covers ~/.coderabbit (a local settings entry) probes yes and pays no bypass prompt at all; every other machine gets the bypass from the start, which keeps the lane portable — the hang above means a try-sandboxed-first strategy WITHOUT the probe would cost the full timebox on unprepared machines, which is why the probe, not a guess, makes the call. STEP 2 contains a single coderabbit command by design: steps 1 and 3 hold every git and working-copy operation and stay sandboxed. Never widen the bypass to cover them. +until the timebox kills it. + FILESYSTEM denial — ~/.coderabbit is outside the write allowlist, so the CLI cannot create its log or review store. It writes NO log file at all. + NETWORK denial — the coderabbit.ai hosts are outside the allowed-hosts list. The CLI DOES write a log, naming the cause verbatim: 403 Forbidden on GET https://cli.coderabbit.ai/public-configs.json with "data":"Connection blocked by network allowlist", followed by a WebSocket retry against wss://ide.coderabbit.ai/ws every 30 s until the kill. +Diagnose by reading the log directory, not by absence alone: a stall with NO new file in ~/.coderabbit/logs/ is filesystem denial (a process killed mid-run still flushes a partial log, so zero bytes means it never created one); a stall WITH a log naming the network allowlist is network denial; a genuine cloud problem leaves a log containing 429 or queue lines. +Therefore STEP 1 probes BOTH dimensions and STEP 2 — and only STEP 2 — runs with sandbox bypass (dangerouslyDisableSandbox) when the probe says CR_SANDBOXED=no; the observed evidence above is the justification the sandbox rules require. A machine whose allowlists cover both ~/.coderabbit and the coderabbit.ai hosts probes yes and pays no bypass prompt at all; every other machine gets the bypass from the start, which keeps the lane portable — the hang above means a try-sandboxed-first strategy WITHOUT the probe would cost the full timebox on unprepared machines, which is why the probe, not a guess, makes the call. Probing only writability is what shipped first and it was not portable: the filesystem-only allowlist recommended below satisfied that probe while the network stayed blocked, so the lane concluded "nothing to bypass" and hung for the full 600000 ms. STEP 2 contains a single coderabbit command by design: steps 1 and 3 hold every git and working-copy operation and stay sandboxed. Never widen the bypass to cover them. Do NOT read a "connecting" stall as an outage, a 429, or contention with another session — a concurrent run on the same account was ruled out as the cause (an unsandboxed run succeeded while a sandboxed one hung, which merely looks like contention). RECOVERY — always try this before returning unavailable, and also whenever your own run stalls, exits non-zero, or is killed at the timebox. Run it as a SEPARATE Bash call, never appended to the steps above: STEP 2 may already have been killed at its timebox, so anything chained after it would never execute. The CLI persists every completed review to ~/.coderabbit/reviews///reviews//, where git.json carries "head", "baseCommitId" and a per-file "diff" list holding each file's full patch text, including its "index .." line. Search for a record whose head is ${headSha} and whose blob OIDs equal the pinned diff exactly: