Skip to content

fix(skills): cross-review sandbox probe and companion resolution #2172

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
yuanchen8911 merged 1 commit into from
Aug 13, 2026
Merged

fix(skills): cross-review sandbox probe and companion resolution #2172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 53 additions & 24 deletions .agents/skills/aicr-cross-review/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: "<wf_...>", 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 <job-id> --cwd "<repo-path>" --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: "<wf_...>", args: {...prevArgs,
codexResumeJobId: "<job id>"}})` — `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 <id>`), the wait agent collects the existing
Expand Down Expand Up @@ -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
Expand All @@ -508,17 +527,25 @@ the consensus mechanics):
timebox per wrong guess. The step-1 probe settles it in milliseconds: machines with

@njhensley njhensley Aug 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Nitpick — "settles it in milliseconds" overstated for a blackhole sandbox

The two added curl -m 10 calls mean a filesystem-only machine whose sandbox drops packets (rather than returning a fast proxy 403) runs the probe up to ~20s, so "settles it in milliseconds" is overstated for that case. Still far under STEP 1's 120s default Bash timeout, so "Steps 1 and 3 are fast and need no explicit timeout" (workflow.mjs:446) stays true.

Fix: Optional: "…in milliseconds (up to ~20s if the sandbox blackholes rather than returning a fast 403)."

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:

@njhensley njhensley Aug 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Minor — "Filesystem half is worse than granting neither" is now false

Present-tense "Granting only the filesystem half is worse than granting neither" no longer holds under the new both-dimension probe: filesystem-only (write passes, cli.coderabbit.ai curl fails → CRSBX=no) and neither (write fails → CRSBX=no) both bypass with one prompt — neither hangs. The "worse = 10-minute hang" outcome was true only of the OLD writability-only probe, which this sentence's own tail and L537 ("The probe now tests both for exactly this reason") admit is neutralized. workflow.mjs:463 already states this in correct past tense.

Fix: Reword to past tense / "buys nothing": e.g. "Granting only the filesystem half buys nothing — it satisfies the write probe while the network stays blocked, so the lane still bypasses. An earlier writability-only probe instead read that state as sandbox-clean and hung for a full timebox, which is why the probe now tests both."

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:
Expand Down Expand Up @@ -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

@njhensley njhensley Aug 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Minor — Umbrella "Rules" sentence omits the network dimension this PR added

This bullet correctly makes bypass fire "when any check fails" across writability and both-host reachability. But the umbrella sentence introducing the two exceptions — up at L806, both conditional on the sandbox actually denying **the write** — was left un-updated and now omits the network dimension it then describes here, contradicting its own child bullet. (This is the exact spot CodeRabbit flagged and marked "✅ Addressed in commit d7a8e6c", but that commit is not an ancestor of the squashed head and the L806 phrase is unchanged.) Doc self-consistency only — the operative probe in workflow.mjs is correct.

Fix: At L806, reword to cover both dimensions, e.g. "each conditional on the corresponding access probe failing" (or "…denying the write or network access").

**step 2 only**, which is a lone `coderabbit review` command. When the
Comment thread
coderabbitai[bot] marked this conversation as resolved.
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.

Expand Down
Loading
Loading