feat(ci): a container per job — jobs isolated from each other, not just the app #5602
ccantynzAI Reviewcommented 1d ago
AI Triage
(no summary)
Priority: medium Risk area: mixed
Suggested labels: (no label suggestions) Suggested reviewers: (no reviewer suggestions)
Suggestions only — nothing has been applied. The PR author stays in control.
gluecron[bot]🤖 botAI Reviewcommented 1d ago
AI review unavailable
The platform's AI balance is exhausted, so AI generation is temporarily unavailable. Nothing was lost. You can queue this as a repair for the internal agent from the repository's Health page, or try again once the balance is restored. The PR is otherwise unchanged.
Cross-repo impact
See what breaks downstream if this PR merges.
⮌ Merged
This pull request was merged into main.
c comment · e edit title · m merge · a approve · r request changes · ? shortcuts
The sibling runner isolated jobs from the app. It did not isolate jobs from each other — one long-lived runner means two runs share a filesystem and a process namespace, so one tenant's step can read another's checkout or wait around for it.
That is the last thing standing between this and multi-tenant CI that can honestly be sold. Each job now gets its own container: created, run, harvested, destroyed.
Why a broker rather than a socket in the app
Something has to reach the Docker API, and
/var/run/docker.sockis equivalent to root on the host — whoever holds it candocker run -v /:/host --privileged. Arbitrary code execution inside the app container is a product feature here, so mounting the socket there would upgrade every container-scoped compromise into a host-scoped one.So the broker holds the socket and the app holds an HTTP client. The distinction that makes that worth anything:
Not the image, mounts, limits, capabilities, user or network — every one is a constant in
broker-agent.ts. The app sends a workspace name, a script, an environment and a timeout. A fully compromised app can only ask for jobs the broker was already willing to run.An API that accepts a container spec is a docker socket with extra steps.
Three modes, no flag day
BROKER_URLRUNNER_URLEach falls through to the next when unconfigured. Both remote modes also require the checkout to be on the shared volume, because dispatching a private temp path to a container that can't open it is worse than running locally. A dev machine needs no configuration at all.
One simplification over the original audit
AUDIT-CI-RUNNER-ISOLATION.mdStage 2 assumed the job would clone over loopback with a short-lived scoped token, and made that token type a prerequisite. It isn't needed. The app already clones into the shared volume, so the broker binds only that run's checkout directory. No credential in the job at all — strictly better than the plan, because a credential that doesn't exist can't leak.Verification
The tests assert the boundary's shape and are mutation-verified — adding
Image: req.image, addingPrivileged: true, or mounting the socket into the app service each fails a specific test.One mutation initially "passed" because a CRLF/
\nmismatch made the replacement a silent no-op — the same trap that has bitten twice this session — so the mutation script now asserts it actually changed the file before drawing a conclusion.What this still does NOT close
Which is why
workflow-exec-policy.tsstays, with a header that now gives reasons you can check:bun installneeds a registry. A hostile step can still make outbound connections — it can simply read far less than before. Deny-by-default is the next increment./app. Fine for trusted accounts, wrong for strangers.ci-work.Those two, then that file goes and CI opens to everyone. That's the whole remaining list.
Enabling
Off by default —
BROKER_URLandBROKER_TOKENboth empty, and a URL without a token counts as no broker. To roll back after enabling, clearBROKER_URL; execution falls to the sidecar with no code change.One thing I could not reproduce: in one combined local run of six suites, 1 of 76 tests failed; three subsequent runs were clean and the failing name never printed. Not claiming it's resolved — flagging it, since a flaky test in a merge gate is exactly the "learn to distrust red" problem.