Repository navigation
feat(reviewer): one isolated sandbox container per needs job (#435) - #463
Conversation
cli#435 sweep — every sentence this PR adds or changesEach sentence checked against the code on 5aa168c (main 4982f78).
|
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 25 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (13)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Round 2 — the per-job isolation check ran without the driver's dependenciesRoot cause. Fix (measured on Reproduced by renaming the worktree's The driver's full stderr is Evidence The cli delta (+1 file, −1 pass) is the round-1 #435 test change (launcher tests retired, driver tests added); this round changes no test. The github-review lane needs node ≥ 22.22.3 (CI runs it under 24.21.0); under this host's 22.22.1 three gateway-boundary cases fail on a node-engines check, so it was measured under 24.21.0 in an isolated |
cli#463 round 2 — sweep of every sentence this round adds or changesDiff base:
|
Round 3 — the per-job isolation check, root cause and fixRoot cause. The driver ran each job's container as the image's
The first failing line ended step 3 with exit 1, and the driver reported it honestly as Fix (
Job 1 still changes the tracked file, adds the ref and stages the file and leaves the detached (setsid) process; job 2 still asserts a clean status, the original tracked file, no job-1 ref, an empty index, and a stopped heartbeat. Passing end-to-end output (
|
cli#463 round 3 — sweep of every sentence this round adds or changesDiff:
|
…job passes only on job-ok (#435) The driver's isolation boundary was overbroad in three ways: - its CLI took unrestricted --bind values and attached each to every job, so a caller could share a writable host path between jobs. The CLI now takes no bind option; a container gets the job's own clone and no other host mount. The isolation check adds the one bind it needs (the shared evidence directory) through the driver's injected docker runner, so the driver it exercises is the production one. - a job could be reported as a success when `docker exec` exited non-zero: the driver parsed a JSON line but never required exit 0 or a job-ok verdict for the assigned job. A job now passes only on ok:true, status "job-ok", that job's own name and exit 0. - a failed `docker rm -f` was ignored, and the isolation check read a failed `docker ps` as an empty list. A container that cannot be removed now fails the build before the next job starts, and the check tests ps's status first. Narrows the sentences that claimed more than the driver enforces, and the check's job-2 status and heartbeat pass messages to what they observe.
|
Sweep — this round's added/changed sentences (
|
…r captured (#435) dockerRunner and gitRunner returned the error string as stderr when a spawn timed out or failed, dropping what the process had already written; both now append the error to the stderr they captured, so a timed-out sandbox exec still reaches the refusal's output with the job's own messages. The verdict control was untested condition by condition: the exit-non-zero test wrote a bare {"ok":true} verdict, already not job-ok, and no test exited 0 with the right job and another status. That test now carries a complete job-ok verdict for the job that runs, and a new test exits 0 with a self-check-ok verdict, so removing either condition alone turns a test red.
cli#463 round 5 — sweep of every sentence this round adds or changesDiff:
|
|
@coderabbitai review |
|
tps-sherlock
left a comment
tps-sherlock
left a comment
There was a problem hiding this comment.
APPROVE — head 7e4d4fca. Repo visibility checked: tpsdev-ai/cli is PUBLIC (repos/tpsdev-ai/cli → .visibility = public).
Evidence: worktree at head; bun install --frozen-lockfile && bun run build → exit 0 (0 TS errors on the settled pass). TMPDIR=/private/tmp node scripts/test-suite.mjs sherlock-463 packages/cli/test/reviewer/run-review-jobs.test.ts packages/cli/test/reviewer/reviewer-launch.test.ts → 83 pass / 0 fail (446 expects). Mutation: deleting --network none, --cap-drop ALL and --security-opt no-new-privileges from containerArgs fails the flags test.
Properties in scope:
Job→job isolation (tracked-file edit, .git state, detached process). Each job gets its own join(scratch, job.id) (run-review-jobs.mjs:111), rmSync'd then freshly cloned from the read-only source (cloneArgs, line 66: file://${source}, --depth 1 --no-tags), its own container, and dir + envFile are rmSync'd in the finally (line 164). The container is removed before the next job starts and a removal failure stops the build (if (removeFailure) return { ok: false, stop: true, ... }). So job 2 clones the same source independently: nothing job 1 wrote in its tree, its .git state, or a setsid process it left survives into job 2. Covered by run-review-jobs.test.ts and the Docker-gated per-job-isolation-checks.sh (I could not run the latter — no Docker in this sandbox).
Container hardening (flags verified). containerArgs (line 73):
"create", "--init", "--read-only",
"--tmpfs", "/tmp", "--tmpfs", "/var/tmp", "--tmpfs", "/run",
"--network", "none", "--cap-drop", "ALL",
"--security-opt", "no-new-privileges",
...(user ? ["--user", user] : []),
"--workdir", "/workspace", "-v", `${dir}:/workspace`,
"--env-file", envFile, "--label", `${LABEL}=${jobId}`, image, "sleep", "infinity",i.e. read-only root + tmpfs scratch, no network, all caps dropped, no-new-privileges, non-root --user (the clone's owner). The test pins read-only / --network none / --cap-drop ALL / no-new-privileges / --workdir / --env-file / exactly one /workspace mount.
Host source not writable from inside; no credential reaches the container. Only dir — the job's own clone under scratch — is bound, writable, at /workspace (the test asserts the single /workspace mount). The source is read on the HOST (git -C sourceReal show HEAD:<workflow>, then git clone file://${source}) and is never mounted; the clone's own remote.origin.url is a file:// host path that does not exist inside the container, and --network none closes the other route — so the container can neither read nor write the source. Credentials: docker create inherits no host environment and mounts no host path but the clone; the only environment is the 3-key --env-file (REVIEWER_CI_WORKFLOW|_JOB|_BASE, written 0600) plus the image's baked ENV. The file:// clone (rather than a --local hardlink clone) also means the job's git operations cannot corrupt the source's object store through hardlinked objects — a deliberate and correct choice.
Findings (all non-blocking):
[F1] scripts/reviewer/run-review-jobs.mjs:111-112 — defense-in-depth. runOneJob uses job.id directly as a path component — const dir = join(scratch, job.id); rmSync(dir, { recursive: true, force: true }); — and as the env-file name (line 128) and the --label value, with no JOB_ID_RE (scripts/reviewer/ci-job.mjs:67) assertion. planJob validates job ids only inside needs (ci-job.mjs:566); it does not validate the job-id key format. The launcher does assert JOB_ID_RE on the assignment (scripts/reviewer/reviewer-launch.mjs:316), but that runs INSIDE the container, after the driver has already rmSync'd the path. It is not reachable in the documented wiring — the entry job is --job/REVIEWER_CI_JOB (host config), and every other id in the closure arrives through the validated needs — but a probe confirms the gap: planJob ACCEPTS an entry id "a/b" when the workflow defines a matching doc.jobs key (a needs: ["../evil"] entry is correctly refused). Recommend asserting JOB_ID_RE on job.id before any filesystem call, so a future caller passing an unvalidated name cannot rm -rf an arbitrary host path.
[F2] scripts/reviewer/run-review-jobs.mjs:73 — hardening. No resource limits are set: there is no --pids-limit, --memory or --cpus, and the writable bind is unquota'd. --cap-drop ALL + no-new-privileges constrain privilege, not resource use, so untrusted workflow code running as the job can fork-bomb or fill host memory/disk. Optional for a first cut, but a --pids-limit/--memory bound would round it out.
[F3] packages/cli/test/reviewer/run-review-jobs.test.ts:144 — test gap. The containerArgs test pins --read-only, --network none, --cap-drop ALL, no-new-privileges, --workdir, --env-file and the single mount, but not --init or the --tmpfs /tmp|/var/tmp|/run mounts. A regression that drops a tmpfs mount would leave a read-only root with no writable /tmp, caught only by the Docker-gated per-job-isolation-checks.sh — which unit lanes do not run.
Could not see: the Docker-only checks (per-job-isolation-checks.sh, image-checks.sh, a9-red-proofs.sh); Docker is unavailable in this sandbox, so the real container path is unverified here. No CI-lane claim is made.
tps-kern
left a comment
tps-kern
left a comment
There was a problem hiding this comment.
Verdict: APPROVE. Repo visibility checked via the API before writing this: tpsdev-ai/cli is public; every observation below is structural and readable in the diff, so full detail is posted here.
Reviewed on head 7e4d4fc in my own worktree. All runs are my own, not CI claims. I did not execute any container on this host — the real-docker path is exercised by the CI lane (per-job-isolation-checks.sh, which I reviewed line-by-line) and by the injected-runner tests; per review policy I ran the planning, gating, verdict-binding, and hardening surfaces with the fake docker + real git sources.
What I ran
bun install --frozen-lockfile && bun run build: clean at head.test/reviewer/run-review-jobs.test.ts+test/reviewer/reviewer-launch.test.tsthrough the isolated test-suite launcher: 83 pass / 0 fail (real bash steps, real git sources, injected fake docker).- Mutations (one pass, all caught, reverted, control 83/83):
- Removing the needs-skip gate → "a job whose need fails is skipped: the dependent never runs" fails.
- Relaxing the verdict binding (any
ok:trueaccepted) → four verdict tests fail (non-zero exit carryingok:true; verdict that is notjob-ok;ok:falsewithjob-ok; verdict naming another job) plus the buffer-error refusal path. - Dropping
--read-onlyfromcontainerArgs→ the hardening test fails.
Analysis — the three questions I was asked
- Needs ordering and
if:/always()semantics match GitHub for the supported subset. The gating block moved verbatim from the retired in-launcher loop (I diffed the removed hunk against the new code: sameblocked = needs.filter(n => status.get(n) !== "ok"), samewhen !== "always"bypass, same status propagation).ci-job.mjsnormalizesif:to exactlysuccess|alwaysand refuses everything else (ci-unhonourable: only absent,true,success(),always()— line 591), sofailure()/cancelled()/composite expressions cannot reach the driver; of the two reachable values, the driver behaves exactly as GitHub does — default runs only when every need succeeded, a skipped need also blocks, and skips propagate transitively through the status map;always()runs after a failed need and the build is still not ok (both driver tests assert this against a real git source). - The per-job clone really comes from the read-only source.
runOneJobclones each job's directory withgit clone file://<source>— thefile://form forces the transport, so no local-clone hardlinks/alternates touch the source — depth 1--no-tagsor full, per that job's ownfetch-depth. The driver's only other source access isgit -C <source> show HEAD:<workflow>(read-only). The source is never mounted into the container:containerArgsbinds exactly one host path (asserted by test, mutation-backed), and the driver CLI has no bind flag at all (--bindis an unknown argument). With--network noneand no mount, the source is unreachable from inside — the host source cannot be written from the sandbox. A host that provides a shallow source for afetch-depth: 0job surfaces as the launcher's existing named clean-clone refusal (full-history check), so host-trust failures stay loud rather than silently passing. - The workflow change is scoped.
test.ymlgains exactly one step in the existing reviewer-image job —per-job-isolation-checks.shwith the built image, mirroring thea9-red-proofspattern. No other workflow hunks.
Isolation surfaces I can confirm from source and tests (cross-referencing the security focus): the container flags are real and load-bearing (--init, --read-only, tmpfs on /tmp//var/tmp//run, --network none, --cap-drop ALL, --security-opt no-new-privileges, --user = clone owner, one -v bind, tps.reviewer.job label); the only env the container receives beyond the image's own is the create-time env file carrying exactly the three REVIEWER_* keys (mode 0600, removed after the job), so no credential reaches it, and the assignment travels on the container init's environment — docker exec cannot override it. The driver's pass condition binds exit 0 ∧ ok ∧ job-ok ∧ the named job — mutation-verified — and lastJsonLine reads the exec pipe, which step output cannot reach (the launcher's stdout-is-the-verdict contract predates this PR).
Findings (non-blocking):
scripts/reviewer/run-review-jobs.mjs(runOneJob) — thechmod -R a+rX+chmod 0777widening of each job's clone is only needed when no POSIX uid is available (the--userfallback); on a POSIX host the sandbox already runs as the clone's owner. If a host ever places--scratchsomewhere world-traversable, the checkout is world-readable for the job's duration. Contained today (fixtures usemktemp -d); consider widening only whencurrentUser()is undefined.scripts/reviewer/per-job-isolation-checks.sh(cleanup) —docker ps -aq --filter label=tps.reviewer.job+docker rm -fremoves every reviewer-labeled container on the host. Right on a dedicated CI runner (its documented home); on a shared host it would sweep the production driver's live containers, since the label is the production one. Worth a comment scoping it to CI.
Not independently verified: the full repo matrix and the real-docker isolation lane (loopback/container execution not run here by policy — I did not execute any container on this host); the empirical job1→job2 invisibility claims are the CI lane's to prove, and its script's assertions (clean status, original tracked file, absent ref, empty index, frozen heartbeat, no leftover containers, discarded directories) look sound and are gated on a real review-build-ok from the production driver.
— Kern (architecture review)
4 participants
Closes #435.
What changed
Each job of a
needsclosure now runs in its own sandbox container, from a fresh clone of the assigned commit, so state inside.git/, a process that starts its own session (setsid) and anything else that job leaves in its tree cannot reach the next job.scripts/reviewer/run-review-jobs.mjs. It plans the named job'sneedsclosure and, per job in dependency order, makes a fresh clone of the assigned commit from a read-only source the host provides, starts ONE container (OpenClaw's run model: read-only root, tmpfs, no network, caps dropped) with that job's clone, and no other host mount, bound writable at/workspace, runs the launcher in it for that one job, then removes the container — a removal that fails fails the build, before the next job starts — and discards the job's directory. The CLI takes no bind option. A job passes only on anok:true,job-okverdict that names it, from a launcher that exited 0; a job runs only if the jobs it needs succeeded (or itsif:isalways()), and the driver reportsreview-build-okonly when every job passed.scripts/reviewer/reviewer-launch.mjsnow runs the ONE job the host names, in the container it runs in.removeCreatedPathsand the cross-job "not fresh" refusal are retired; the clean-clone check stays (each job still verifies its own fresh clone).scripts/reviewer/per-job-isolation-checks.sh(two jobs linked byneeds: the first changes a tracked file,.gitstate and leaves a detachedsetsidprocess; the second checks the original tracked file, absent ref and staged entry, and no heartbeat growth over one second) and unit testspackages/cli/test/reviewer/run-review-jobs.test.ts.image-checks.sh, the lane docs and the changelog are updated; the checks run in thereviewer-imageCI job.Verification
Measured on
02e901cf(the round-6 head), against locally cachedorigin/mainat06eda65e04564b26b931f3b7957900edeac28115. Later commits change comments, the README and the PR body only.bun run buildexited 0 on both trees; the build script's initial CLI pass reported missing agent declarations before the agent and final CLI builds succeeded.bun run lint:ciexited 0;node --check scripts/reviewer/run-review-jobs.mjs,bash -n scripts/reviewer/per-job-isolation-checks.sh,node scripts/changelog-fragments.mjs checkandgit diff --checkpassed.TMPDIR=/private/tmp node scripts/test-suite.mjs blockers-verified ./packages/cli/test/reviewer/run-review-jobs.test.ts: 22 pass / 0 fail.node scripts/test-suite.mjs, with Bun 1.3.10, Node 22.23.2 andTMPDIR=/private/tmp. Plugin runs set their mail/key paths inside the launcher's isolated test root.Both trees also report 3 skips and 2 todos. No new failures: both have the same sandbox/git-wrapper, missing
/bin/grep, nono-path and missing-OpenClaw failures. Plugin builds could not typecheck without OpenClaw; the emitted partial builds also make the plugin-launcher guard tests refuse.The mail-plugin suite ran file by file through the same launcher on both trees after combined runs stalled; the baseline
verify-strict.test.tsshard also stalled once and passed on retry.Socket exclusions (whole files, including their non-socket cases):
packages/agent/test/:flair-context.test.ts,mail-promote-guard.test.ts.packages/cli/test/:branch-join.test.ts,codex-presence-444.test.ts,flair-sync.test.ts,mail-bridge.test.ts,mail-producers-sign.test.ts(origin/main only),mail-promote.test.ts,mail-receipt-thread.test.ts,mail-remote.test.ts,mail-send-routes.test.ts,mail-send-stdin-reply.test.ts,mail-unresolvable-principal.test.ts,mail-watch.test.ts,mail.test.ts,mock-llm.test.ts,noise-ik-transport.test.ts,plain-tcp-transport.test.ts,runtime-mail-lifecycle.test.ts,service-proxy.test.ts,wire-mail.test.ts,ws-noise-transport.test.ts.packages/cli/test/launch-attestation.test.tsandlaunch-attestation-real-nono.test.tsuse Unix sockets; the real-nono fixture also needs a writable/var/tmp.packages/pi-tps-mail/test/reply-send.test.tsuses the Flair HTTP stub.Fail-first and mutation evidence (each mutation restored):
result.ok === true &&: 21 pass / 1 fail, the new exit-0 +ok:false+job-ok+ right-job test.execStatus === 0 &&: 21 pass / 1 fail, the nonzero-exit test.result.status === "job-ok" &&: 21 pass / 1 fail, the wrong-status test.&& result.job === job.id: 21 pass / 1 fail, the wrong-job test.--no-tags: 21 pass / 1 fail, the tagged-head clone test (the launcher reports tags).errorCode: 20 pass / 2 fail, its timeout and buffer-code tests.errorCode: 20 pass / 2 fail, its timeout and buffer-code tests.container-execandENOBUFS.Docker checks were not run in this sandbox. Run outside it, from this tree:
Real-container checks on
02e901cf(a Linux host with the reviewer images):scripts/reviewer/per-job-isolation-checks.shonreviewer-node22-bun1310andreviewer-node24-bun1310: 8 checks, 0 failures each; no sandbox container left behind.image-checks.shanda9-red-proofs.shwere not re-run for this round.Note
The driver is host-side and needs a container runtime, which a unit lane may not have; its unit tests drive an injected docker runner, and the real container path is
per-job-isolation-checks.sh, run in thereviewer-imageCI job (which has Docker). Nothing in this PR changes the runtime planner/resolver, the image, or the github-review and tps-mail plugins.