fix(skills): cross-review sandbox probe and companion resolution - #2172
Conversation
yuanchen8911
added
the
theme/ci-dx
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe cross-review workflow now resolves the newest Codex companion, polls the existing job with its workspace pinned, and resumes using the existing job ID. It also probes CodeRabbit filesystem and network access. If any probe fails, it bypasses the sandbox only for the standalone review command. Documentation covers access requirements, failure symptoms, log diagnosis, and cloud-side queueing. Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/skills/aicr-cross-review/SKILL.md:
- Around line 796-801: Update the general sandbox-exception wording in the
CodeRabbit review protocol to reference failure of the corresponding access
probe, including network reachability, rather than requiring only a write
denial. Keep the existing behavior of bypassing step 2 only when the relevant
probe fails.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: e2b751be-6c4f-4951-b785-7aa9ce437108
📒 Files selected for processing (2)
.agents/skills/aicr-cross-review/SKILL.md.agents/skills/aicr-cross-review/scripts/workflow.mjs
48da778 to
d7a8e6c
Compare
yuanchen8911
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/skills/aicr-cross-review/SKILL.md:
- Around line 510-516: Update the CodeRabbit sandbox preflight guidance in the
lane’s step 1 to probe reachability of the WebSocket endpoint host,
ide.coderabbit.ai, in addition to cli.coderabbit.ai and ~/.coderabbit
writability. Ensure CR_SANDBOXED is disabled or sandbox bypass is used when the
WebSocket host check fails, preventing Step 2 from hanging.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 2e301b91-6ada-453c-81dc-b2e7da822865
📒 Files selected for processing (1)
.agents/skills/aicr-cross-review/SKILL.md
d7a8e6c to
199b785
Compare
199b785 to
6362b5a
Compare
6362b5a to
881a0ff
Compare
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
Multi-persona review — Approve with comments ✅
Method: three independent persona reviewers (Correctness · Operability/Sandbox-portability · Prompt/Doc-consistency) → adversarial senior meta-reviewer re-derived every finding from the resolved code at 881a0ff5.
Tier legend: 🔴 Blocker · 🟠 Major · 🟡 Minor · 🔵 Nitpick
Overall
A tight, genuinely well-tested prompt/doc fix — no shipped code. The two load-bearing mechanics both check out:
- The probe is correct and fail-safe. The write probe and the two new
curl -fsS -m 10host checks are&&-chained, soCR_SANDBOXED=yesrequires all three to pass; any failure →no→ STEP 2 bypasses. Walked all four machine configs (both-allowlisted / filesystem-only / neither / network-only) — each lands on the intended outcome. The curls run inside STEP 1's sandbox, which is what earns them the proxy403that makes them a valid probe; the bypass surface is not widened (STEP 2 remains the lonecoderabbit review). - The two subtle traps are clean. The
\-continued probe lines sit inside theCODERABBIT_RUNtemplate literal, so JS collapses them to one valid shell line (verified it parses underbash -nandzsh -n). The Codex-companionls -t … | head -1+node "$comp" status <job> --cwd … --jsonblock matches the lane's own D1/W1/W3 invocations verbatim.
Every surviving finding is doc self-consistency; there are no blockers and no majors. All six are inline. The only 🟡 that carries a bit of history is F1 — it's the CodeRabbit "exception-wording" item that was marked fixed in d7a8e6c, but that commit isn't in the squashed head and the L806 wording is unchanged, so it's still live (worth the one-line touch-up). The other human "reviewer" the tooling flagged is the PR author self-commenting to answer CodeRabbit — not an independent review.
Confirmed non-issues (examined and cleared)
- Template-literal
\-continuation renders to a single valid shell line; parses under bash & zsh. - Companion
ls -tresolution matches the lane's D1/W1/W3 usage verbatim; barestatus … --jsonis the correct terminal-poll form. - Fail-safe direction correct across all machine configs.
- Bypass surface unchanged — STEP 1 & 3 stay sandboxed; probe curls run inside the sandbox.
.claude/skillsis a symlink →.agents/skills, so there's no mirror copy to drift;check-agents-sync(CLAUDE.md ↔ AGENTS.md) is correctly untouched.- zsh unmatched-glob on the companion glob is pre-existing lane behavior gated by the Phase-0 presence check — not introduced here.
- CodeRabbit's
ide.coderabbit.aiWebSocket-host probe is already implemented at head (the third curl leg).
Summary
| Tier | Count |
|---|---|
| 🔴 Blocker | 0 |
| 🟠 Major | 0 |
| 🟡 Minor | 2 |
| 🔵 Nitpick | 4 |
Recommendation: Approve with comments. F1 and F2 are worth a one-line prose fix each; the rest are optional polish.
| 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 |
There was a problem hiding this comment.
🟡 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").
| `~/.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: |
There was a problem hiding this comment.
🟡 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."
| 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 |
There was a problem hiding this comment.
🔵 Nitpick — curl -f conflates "host reachable" with "HTTP <400 at path"
curl -f fails on any status ≥400, so it cannot distinguish a sandbox proxy 403 from an origin-returned 4xx/redirect at the probed path on a network-allowed machine. If CodeRabbit ever auth-gates GET https://ide.coderabbit.ai/, moves public-configs.json, or the WS host stops answering 200 on a plain GET, a fully-allowlisted machine flips to CRSBX=no and silently pays one approval prompt every run — no hang to signal it. This is the cheap/fail-safe direction and the 200-vs-403 assumption is already documented at L436, so it is accept-as-designed; flagging it as the single linchpin the fail-safe rests on.
Fix: Optional: a one-line breadcrumb near the probe — "if these endpoints stop answering 200 when reachable, re-point the probe" — so a future maintainer isn't left guessing why an allowlisted machine started prompting.
| @@ -508,17 +527,25 @@ the consensus mechanics): | |||
| timebox per wrong guess. The step-1 probe settles it in milliseconds: machines with | |||
There was a problem hiding this comment.
🔵 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)."
| 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. |
There was a problem hiding this comment.
🔵 Nitpick — Network-denial log signature omits the ide-only-blocked sub-case
The network-denial signature is written as "403 on cli.coderabbit.ai/public-configs.json … then wss://ide retries." But the per-host asymmetry the probe rationale itself calls out (an allowlist entry naming only the config host) produces the opposite log: the cli fetch succeeds, then ONLY the wss://ide retries appear with no cli 403. A reader matching strictly on the 403 line could misfile that sub-case. The probe still catches the config (the ide curl fails → bypass), so this is diagnosis-prose only.
Fix: Optional: note the two host blocks can appear independently — a cli 403, or (if only ide is blocked) the wss://ide retries alone.
| 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 \ |
There was a problem hiding this comment.
🔵 Nitpick — -S flag is inert with stderr redirected to /dev/null
Both probe curls are -fsS … 2>/dev/null. -S re-enables the error message that -s suppresses, but stderr is discarded, so -S never does anything.
Fix: Harmless — drop -S, or keep for symmetry.
Two fixes to the aicr-cross-review skill, both from failures observed in a single live review round. CodeRabbit lane: the step-1 sandbox probe tested only whether ~/.coderabbit was writable, then concluded no sandbox bypass was needed. Writability does not imply the coderabbit.ai hosts are reachable, and either denial produces the same symptom: the CLI hangs at connecting_to_review_service until the 600000 ms timebox kills it. The broken state is the one this skill's own guidance creates. The SANDBOX section recommends adding ~/.coderabbit to filesystem.allowWrite to remove per-round approval prompts, and never mentions the network allowlist. A machine in that state probes "sandboxed is fine" and then loses a full timebox on every run. Probe both dimensions and AND the results. The reachability half tests both hosts the CLI needs: the cli.coderabbit.ai config endpoint it fetches on startup, and ide.coderabbit.ai, the host the review session's WebSocket connects to. The allowlist is per-host, so an entry naming only the config host (the obvious edit for a reader who saw it in the probe) would pass a single-host check and still hang step 2 on the WebSocket connect. A plain HTTPS GET tests the wss host because the allowlist rule is host-scoped, not protocol-scoped. curl -f is required on both: without it a 403 "Connection blocked by network allowlist" still exits 0, so a blocked host would report as reachable. The combined result stays fail-safe in the cheap direction, since a false negative costs one approval prompt while a false positive costs ten minutes. Correct the diagnosis guidance to match. A network denial does write a log naming the allowlist, so the absence of a log file no longer separates sandbox denial from a cloud problem; it separates filesystem denial from network denial. Codex lane: the recovery note tells the orchestrator to poll a live job with the companion status command, and the lane's own failure message refers to $comp — but that variable is defined only inside the lane prompt and cannot cross Bash calls, so the orchestrator has to invent the resolution. Give it the same "ls -t" form the lane uses, and say why: sorting by version name instead selects a stale cached companion, and 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. Signed-off-by: Yuan Chen <yuanchen97@gmail.com>
881a0ff to
656cabb
Compare
Recipe evidence checkNo leaf overlays affected by this PR. This gate is warning-only and never blocks merge. |
2 participants
Summary
Two fixes to the
aicr-cross-reviewskill, both from failures observed in a single live review round: the CodeRabbit lane's sandbox probe missed network denial and hung for a full 10-minute timebox, and the Codex lane's recovery note left the orchestrator without a way to resolve the companion path.Motivation / Context
CodeRabbit lane — the probe tested the wrong dimension. STEP 1 decided whether STEP 2 needs a sandbox bypass by writing a probe file under
~/.coderabbit. STEP 2 then read that result as proof there was "nothing to bypass."Writability does not imply the
coderabbit.aihosts are reachable. The sandbox can deny the CLI two independent ways, and both present identically — a stall atconnecting_to_review_serviceuntil the timebox kills the call:403 ForbiddenonGET https://cli.coderabbit.ai/public-configs.jsonwith"data":"Connection blocked by network allowlist", then retrieswss://ide.coderabbit.ai/wsevery 30 s.The broken combination is the one this skill's own guidance produces. Its SANDBOX section recommends adding
~/.coderabbittofilesystem.allowWriteto remove per-round approval prompts, and never mentions the network allowlist. A machine in that state probes "sandboxed is fine" and then loses a full timebox on every run.Codex lane —
$compnever reaches the orchestrator. The recovery note tells the orchestrator to poll a live job with the companion status command, and the lane's own failure message quotesnode "$comp" result <job-id> …. Butcompis defined only inside the lane prompt and cannot cross Bash calls, so the orchestrator has to invent the resolution. Inventing it without-tsorts by version name and can select a stale cached companion; polling a job with a different companion version than dispatched it returns misleading results —statusfinding a job thatresultthen reports as unknown — which reads exactly like a dead job and is not one.Fixes: N/A
Related: N/A
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)cmd/aicrd,pkg/server)pkg/recipe)pkg/bundler,pkg/component/*)pkg/collector,pkg/snapshotter)pkg/validator)pkg/errors,pkg/k8s)docs/,examples/).agents/skills/aicr-cross-review/(agent skill definition; no shipped code)Implementation Notes
Both sandbox dimensions are probed and ANDed. The reachability half tests both hosts the CLI needs: the
cli.coderabbit.aiconfig endpoint it fetches on startup, andide.coderabbit.ai, the host its review session's WebSocket then connects to. The allowlist is per-host, so an entry naming only the config host — the obvious edit for a reader who saw it in the probe — would pass a single-host check and still hang STEP 2 on the WebSocket connect. A plain HTTPS GET tests the wss host because the allowlist rule is host-scoped, not protocol-scoped: the site answers200at/when reachable, and a sandbox block answers a proxy403thatcurl -fturns into a failure.curl -fis load-bearing. Without it, a403 Connection blocked by network allowliststill exits 0, and the probe would report a blocked host as reachable — reintroducing the bug it fixes. Both host checks carry it.The result is fail-safe in the cheap direction. A false negative costs one approval prompt; a false positive costs a full 10-minute timebox. So either check failing sends STEP 2 down the bypass path.
Diagnosis guidance corrected. The previous text said a stall with no log file means sandbox denial, implying the presence of a log rules the sandbox out. That is wrong in the network case: absence of a log separates filesystem denial from network denial, not sandbox denial from a cloud problem.
Sandbox-bypass surface is unchanged. STEP 2 is still the only call that may bypass, and it is still a lone
coderabbit reviewcommand with no git or working-copy operation.The companion fix is documentation only. It gives the orchestrator the same
ls -tform the lane already uses atworkflow.mjs:224and:242; no lane behavior changes.Testing
All five pass.
make qualifywas not run andlint-gois not applicable: this PR changes two files under.agents/skills/, with no Go, YAML, recipe, chart, ordocs/content, so the Go, e2e, coverage, scan, and api-diff lanes have nothing to exercise.check-agents-syncis included because it is the gate that governs agent-instruction files; it comparesAGENTS.mdto.claude/CLAUDE.md, neither of which this PR touches.Verified behaviorally rather than by unit test, since the lane is prompt text executed by an agent:
~/.coderabbitallowlisted for writes andcoderabbit.aiblocked. The old probe reports writable, which is what made the lane choose the sandboxed path.CR_SANDBOXED=noin that same state, which is the value that routes STEP 2 through the bypass instead of the hang.~/.coderabbitwritable,coderabbit.aihosts network-blocked): STEP 1 returnedCR_SANDBOXED=nofrom the network half, STEP 2 ran with the bypass and completed a real review with the pinnedbaseCommit, and all four lanes finishedok.-fis required: without itcurlexits 0 against the blocked endpoint in the reported 403 case.ide.coderabbit.aicheck is meaningful and cheap: unblocked,GET /returns200; sandbox-blocked, the proxy returns403, socurl -fsScleanly separates the two.ls -tresolves to the companion the plugin system records as active (installed_plugins.json→installPath=codex/1.0.6), while a name-sortedlsselects the stale1.0.2copy still present in the cache.workflow.mjsparses in its execution shape (moduleexportstripped, body wrapped in an async function) identically toorigin/main. Note that a barenode --checkfails on both this branch andmain, because the script legitimately uses top-levelreturn.${, so it cannot break the surrounding template literals.Risk Assessment
No shipped code, binary, image, or recipe is affected. The change alters only the instructions given to the review lanes, and only in the direction of choosing the bypass path more often and resolving the companion the way the lane already does.
Rollout notes: None. Machines that already allowlist both the path and the hosts keep running fully sandboxed with no approval prompt, exactly as before.
Checklist
make testwith-race) — N/A, no Go changes; the gates listed under Testing were run insteadmake lint) — the applicable non-Go gates were run;lint-gohas no changed Go filesSKILL.mdupdated in the same commitgit commit -S)