Skip to content

feat(sandbox): stage Docker-less local stacks via a docker option - #325

Merged
Coly010 merged 16 commits into
mainfrom
columferry/sandbox-docker-availability
Sep 23, 2026
Merged

Coly010 merged 16 commits into
mainfrom
columferry/sandbox-docker-availability

Conversation

@Coly010

@Coly010 Coly010 commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Stacked on #324 — review that one first. Second of two PRs splitting #308's sandbox work out of CLI-team experiment land.

This is @mattrossman's suggestion from the thread taken literally: dockerAwareLocalStackRuntime dissolves into localStackRuntime, and being able to control Docker availability becomes something any experiment can ask for.

localStack: localStackRuntime({ cliVersion: 'beta', docker: 'absent' })

docker defaults to 'available', which is the existing code path. The two new states stage a sandbox where the daemon is unreachable ('no-daemon') or the binary is gone entirely ('absent').

What staging actually involves

None of this is decorative — each step exists because a weaker version of it let a "Docker-less" sandbox quietly still have Docker:

  • The socket is not bind-mounted at all (mountDockerSocket: false on DockerSandbox), rather than hidden or blocked from inside the container, and its absence is asserted. CI's sandbox setup ends with chmod 666 /var/run/docker.sock, so anything short of omitting the mount is a no-op there.
  • docker/dockerd/docker-proxy removal asserts its postcondition instead of trusting rm -f, which always exits 0 — and checks against both the root shell's PATH and the agent's own SANDBOX_PATH, which differ.
  • For 'no-daemon', docker --version is captured before removal so the shim can keep echoing it. The CLI's managed-stack runtime probe (fix(cli): improve stack startup defaults and clean managed state cli#6563) reads that to choose Docker; if the shim didn't answer, the CLI would pick a different runtime and we would be testing the wrong thing.
  • DOCKER_HOST points at tcp://127.0.0.1:1, which is never bound. Deliberately not 2375, which Docker Desktop can legitimately expose.

There are guard errors for projectRunning !== false and for a hosted link, since the harness can do neither without Docker.

The supabase shim looks CLI-specific but isn't

It injects -x <excluded services> into supabase start. On the normal path the harness runs supabase start and applies exclusions via buildSupabaseStartCommand; on a Docker-less path the agent runs it, so the shim is the only route by which the eval-level includeServices field is honoured at all. computeExcludedServices already lived in this package.

The environment marker

Each session writes /tmp/supabase-eval-runtime.json recording what was staged, exported now as LOCAL_STACK_MARKER_PATH / LocalStackEnvironmentMarker so scorers can import it instead of duplicating the schema. Scorers are meant to report it, never branch on docker to decide pass/fail — that would grade an eval against its own environment.

Two asymmetries worth knowing:

  • On the Docker-less paths it is root-owned and 0444, so the agent cannot rewrite it to fake the environment it is being graded in, and a write failure is fatal — there the marker is the evidence the environment was staged as claimed. On the 'available' path it goes through the session's own non-root exec, so it is agent-writable and metrics-only, and a write failure only warns. That path is the one every experiment in the repo takes, and a transient /tmp failure shouldn't kill a benchmark run over a marker nothing reads.
  • channel is optional: populated only when the runtime option named a channel and no per-eval cliVersion: pin overrode it. An exact-version pin records cliVersion with no channel.

Also here

needsDocker eval frontmatter (defaults to true), the eval-side counterpart of this option — it is how a Docker-less experiment decides which evals it can legitimately pick up.

What is deliberately not here

skipUnlessCli / skipUnlessDockerless stay CLI-team-owned and land with the cli suite, along with CODEOWNERS and the workflow wiring, once #315 is in.

Verification

pnpm format:check, pnpm typecheck, and the sandbox (89), core (162), framework (8), vercel-runner (28) and web (83) suites pass. test:docker needs a real daemon and free ports, so it was not run; pnpm check's test:framework stage needs AI_GATEWAY_API_KEY and is not run in CI.

Moves the CLI channel resolver into @supabase-evals/sandbox and lets
localStackRuntime's cliVersion accept 'stable'/'beta' in addition to an
exact version, resolved against npm's dist-tags at session start. An
eval's own cliVersion: frontmatter pin still wins. Forwards the
resolved channel pins to Vercel sandbox jobs so a run scores against
one version instead of each job re-resolving "latest" independently.
…vals/sandbox

Promote the CLI team's experiment-land dockerAwareLocalStackRuntime into a
first-class localStackRuntime({ docker: 'no-daemon' | 'absent' }) option, so
any experiment can stage a sandbox where the Docker daemon is unreachable or
the docker binary is absent entirely, alongside the mountDockerSocket sandbox
primitive and needsDocker eval frontmatter it depends on.
@Coly010
Coly010 requested a review from a team as a code owner September 22, 2026 10:12
@vercel

vercel Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
evals Ignored Ignored Preview Sep 23, 2026 9:34am UTC

Request Review

- Export FORWARDED_ENV_NAMES from run-vercel-evals.ts and import it in the
  test instead of a drifted local copy, so a leaked XAI_API_KEY can no
  longer break agentEnvironment()'s assertions.
- Make cliDebUrl's arch param required and check both amd64 and arm64
  release assets, since the resolver's host arch can differ from the
  sandbox container's; delete the now-unused hostDebArch.
- Generalise the beta walk-back to every published X.Y.Z-beta.N version
  older than the unpublished one (not just the same minor), ordered with a
  numeric compareBetaVersionsDesc comparator instead of a string compare.
- Treat a thrown error for one walk-back candidate as "unavailable" and
  keep probing the rest, instead of aborting the whole walk-back; the
  initial dist-tag asset check still fails loud.
- Import isRecord from @supabase-evals/core/json instead of a local copy
  that dropped the array guard.
- Guard the version cache's rejection cleanup with an identity check so a
  stale promise can't evict a newer cached one.
- Drop cliDebUrl/hostDebArch from the sandbox barrel (no external
  consumers) and correct local-stack-runtime's doc comment: channel
  resolution is memoised per process, not per session.
… resolution

Strips agent-added comment cruft (module docblock essay, CI-run-ID call-out,
code-shape narration) back to the invariants and external quirks the code
can't express on its own; no behaviour change.
Condense multi-paragraph comments in local-stack-runtime.ts down to the
non-obvious fact each one exists to preserve, dropping restated code
sequences and step-by-step narration.
…ls once per run

debAssetHeadOk conflated a GitHub 429/5xx with a missing asset (response.ok
false for both), which could downgrade a beta release during a GitHub
outage and record results under the wrong version. Only a 404 now means
"asset not published"; any other non-2xx throws. The beta walk-back loop
is updated to match: a 404 still advances to the next candidate, but a
thrown error aborts the walk-back and propagates instead of being caught
and skipped.

Separately, nothing set SUPABASE_CLI_STABLE_VERSION/SUPABASE_CLI_BETA_VERSION,
so each sandbox job resolved its own CLI channel version independently,
risking two different versions in one fan-out if a release landed mid-run.
runPairs now resolves each channel pin once via resolveChannelPins() before
runBounded and threads the same pins into every job's .env write.
Comment thread packages/sandbox/src/local-stack-runtime.ts
Comment thread packages/sandbox/src/local-stack-runtime.ts Outdated
Comment thread packages/sandbox/src/local-stack-runtime.ts

@Rodriguespn Rodriguespn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for putting this together. A few things are still worth attention before calling this done:

  1. After docker cp, have writeRootFile() run chown root:root and apply the requested mode in one checked root command.
  2. Pass the Docker state into the prompt and tool builders so each state gets the right text.
  3. Give the scorer the parsed marker or a direct container read. It should not depend on the shell or PATH.

After fixing these, please rerun the focused tests and the real-container ownership check.
You can run the Docker check with SANDBOX_DOCKER_TESTS=1 pnpm --filter @supabase-evals/sandbox test:docker.

Add the test case to test/docker.test.ts with localStackRuntime({ docker: 'no-daemon' }). Assert that stat -c "%u:%g %a" /tmp/supabase-eval-runtime.json /usr/local/sbin/supabase /usr/local/sbin/docker returns 0:0 444 for the marker and 0:0 755 for both shims, then confirm a write as node fails with permission denied. Run the prompt, tool, and scorer cases with pnpm --filter @supabase-evals/sandbox exec vitest run test/local-stack-docker.test.ts test/unit.test.ts.

Union each pair's effective channel (exact eval pins win and need none;
otherwise the pair's experiment's localStack.cliChannel) before fan-out,
instead of unconditionally resolving both channels. A run with no
CLI-channel experiments now does zero npm/GitHub lookups. An experiment
that can't be resolved to a config throws instead of silently counting
as needing no channel.
The controller's channel lookup built experiments/<name>.ts by hand, which
stopped matching when experiments moved to experiments/<owner>/*.experiment.ts.
Use the shared discovery helper so the path convention lives in one place.
…y/sandbox-docker-availability

# Conflicts:
#	packages/sandbox/src/cli-channel.ts
#	packages/sandbox/src/local-stack-runtime.ts
…pt text, and read the environment marker without trusting the agent's PATH

writeRootFile only chmod'd after docker cp, leaving the marker and CLI
shims owned by the host UID rather than root. buildToolSurfaceAddendum
and buildLocalStackTools also described docker as installed/reachable
in the no-daemon and absent states. And the environment marker was
only reachable through exec(), whose SANDBOX_PATH has an agent-writable
first entry that could shadow cat.

Fix writeRootFile to chown+chmod in one checked root command, thread
the resolved Docker state into both prompt/tool builders, and add
DockerSandbox.readRootFile (docker exec in exec form, no shell) plus
LocalStackScoringContext.environmentMarker to read the marker as root
without going through resolveSandboxPath or the agent's PATH.
@Coly010
Coly010 changed the base branch from columferry/sandbox-cli-version-channels to main September 22, 2026 19:28
…cker-availability

# Conflicts:
#	README.md
#	packages/sandbox/src/cli-channel.ts
#	packages/sandbox/src/local-stack-runtime.ts
@Coly010 Coly010 self-assigned this Sep 23, 2026
@Coly010
Coly010 requested a review from Rodriguespn September 23, 2026 08:19

@Rodriguespn Rodriguespn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the update. I ran this locally: the focused tests, sandbox typecheck, and targeted real-container ownership check pass. The branch is two commits behind main but merges cleanly.

Can we update it from main and add the PATH-shadow regression by placing a fake cat first on PATH and asserting environmentMarker() still returns no-daemon?

Approving now to unblock you but e should validate that the CI is green. Thx!

A fake cat first on the agent's PATH feeds forged JSON to any shell-based
read; the root exec-form read must still report the staged environment.
@Coly010
Coly010 merged commit ef25279 into main Sep 23, 2026
7 checks passed
Coly010 added a commit that referenced this pull request Sep 23, 2026
Final chunk of the closed #308 — the piece @mattrossman asked for: the
minimal CLI suite plumbing and CODEOWNERS, standalone, with no CLI
experiment internals to review. **15 files, 260 lines**, and four of
those files are 11–13 lines each.

Everything it depended on is now on `main` (#315's
`experiments/<owner>/` layout, and #324 + #325's sandbox options), so
this is based on `main` and the diff is only its own content.

## What it adds

`cli` as an eval suite and an experiment suite, `@supabase/cli`
ownership over `/evals/cli/`, `/experiments/cli/` and
`cli-eval-results.json`, and the workflow wiring so `eval-refresh` and
`append-gh-pages-history` know the suite exists.

The substance is five environment columns: one scenario run unchanged
across forced CLI environments, so a failure isolates to *which*
environment broke. They're thin now that `experiments/presets.ts` exists
— the whole of the Docker-less column is:

```ts
export default defineExperiment({
  ...codexGpt6Luna,
  suite: ['cli'],
  // beta: the Docker-less path only exists in the managed stack, which ships in beta.
  localStack: localStackRuntime({ cliVersion: 'beta', docker: 'absent' }),
  skipEval: skipUnlessDockerless,
});
```

| column | environment | picks up |
|---|---|---|
| `codex-gpt-6-luna-cli-pinned` | repo-pinned CLI, Docker available |
every `interface: cli` eval that isn't hosted-linked |
| `…-cli-stable` | npm `latest` | same |
| `…-cli-beta` | npm `beta` | same |
| `…-cli-nodaemon` | beta, daemon unreachable | also needs `needsDocker:
false` + `projectRunning: false` |
| `…-cli-absent` | beta, no `docker` binary | also needs `needsDocker:
false` + `projectRunning: false` |

All five columns share one `skipEval: skipUnlessCli`, so they run the
same eval set and stay comparable — which is the whole point of the
suite. An earlier revision made the pinned column the shared benchmark
experiment with `'cli'` appended to its `suite`; that experiment has no
`skipEval`, so it picked up hosted-linked evals the other four skip. A
CLI-owned pinned experiment fixes that and leaves this PR touching
nothing outside CLI-owned paths.

`skipUnlessCli` / `skipUnlessDockerless` are the only things left over
from the old `experiments/_lib/`; they live in `experiments/cli/lib/`,
which discovery ignores for free since it only matches `*.experiment.ts`
directly under an owner directory.

## Verified, including the parts tests can't reach

`format:check`, `typecheck`, and the sandbox, core, framework,
`test:cli-lib`, vercel-runner and web suites all pass.

Two things unit tests can't cover, checked directly instead:

- **Discovery**, since it's filename-convention-based and fails
silently: all 16 experiments resolve and load, `experiments/cli/lib/` is
correctly not treated as an experiment, and the five columns resolve to
the intended runtimes and channels (`beta`, `beta`, `beta`, `stable`,
and none for the pinned baseline).
- **The workflow**, which only ever runs in CI, no longer resolves
channels at all — it passes through the manual `cli_stable_version` /
`cli_beta_version` dispatch inputs and otherwise lets
`run-vercel-evals.ts` derive the channels from the actual pairs.
Pre-filling both env vars would have short-circuited that narrowing.

Each run's results now record the CLI version the sandbox actually
installed, read from the session's environment marker, so a `stable` row
names its binary rather than just its channel. That needed
`cliVersionSchema` widened — it rejected every prerelease, so a beta
run's real version could not have been recorded at all.

`test:cli-lib` carries `--passWithNoTests` deliberately: `evals/cli/`
has no scorer tests until #281/#314/#316 land, and without the flag the
script's exit code depends on which of its two paths happens to be
populated.

## Note on the column names

These follow the `gpt-6-luna` rename from #328. They key
`cli-eval-results.json`, and no results exist yet, so there is nothing
to churn — but that also means the names should settle before the suite
is first run.

## Next

#281, #314, #316 and #307 get re-parented onto `main` once this lands —
they currently point at the retained branch of the closed #308. /cc
@Rodriguespn @kanadgupta
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants