Repository navigation
feat: organize experiments by owner - #315
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Rodriguespn
force-pushed
the
Rodriguespn/pr308-nested-experiments-proposal
branch
from
September 21, 2026 17:35
881432e to
a2edffd
Compare
Rodriguespn
changed the base branch from
columferry/cli-2398-cli-suite-and-arms
to
main
September 21, 2026 17:35
Rodriguespn
marked this pull request as ready for review
September 21, 2026 17:41
mattrossman
reviewed
Sep 21, 2026
mattrossman
reviewed
Sep 21, 2026
mattrossman
reviewed
Sep 21, 2026
mattrossman
reviewed
Sep 21, 2026
Collaborator
|
Sanity checking w/ a regression run to verify results look normal still: |
mattrossman
approved these changes
Sep 21, 2026
Collaborator
This was referenced Sep 22, 2026
Co-authored-by: OpenAI Codex <codex@openai.com>
Rodriguespn
force-pushed
the
Rodriguespn/pr308-nested-experiments-proposal
branch
from
September 22, 2026 11:51
a2edffd to
1e930b2
Compare
Contributor
Author
Coly010
added a commit
that referenced
this pull request
Sep 22, 2026
…on (#324) First of two PRs splitting #308's sandbox work out of CLI-team experiment land, per the thread with @mattrossman and @Rodriguespn. This one is independent of #315 and of the `cli` suite entirely — it only widens an existing option. ## What changes `localStackRuntime({ cliVersion })` already accepted an exact version. It now also accepts a channel tag: ```ts localStack: localStackRuntime({ cliVersion: 'beta' }) ``` `'stable'` and `'beta'` resolve against npm's dist-tags for the `supabase` package once, at session start. An eval's own `cliVersion:` frontmatter pin still wins over the runtime option, exactly as before — and because that check short-circuits, a pinned eval never makes the network call at all. The resolver itself moves in from `experiments/_lib/cli-channel.ts` (it has been living in experiment land on the #308 branch). It does two things beyond reading a dist-tag, both of which encode incidents rather than caution: - It verifies the matching `.deb` release asset actually exists, and walks back through earlier `beta.N` candidates when the newest beta's asset has not been published yet. This is a real npm/GitHub draft-release skew we hit on `beta.52` in run 35274398896 — npm had the version, the release asset did not exist, and the install failed inside the sandbox. - It honours `SUPABASE_CLI_STABLE_VERSION` / `SUPABASE_CLI_BETA_VERSION` overrides, which is how a CI run pins one resolved version for all of its jobs. That last part is why `run-vercel-evals.ts` is in the diff: the two pins are added to the env names forwarded into each sandbox's `.env`. Without it, every sandbox job in a run re-resolves "latest" independently and a run can score against two different CLI versions if a release lands mid-run. ## Notes for review - `cliVersion` is typed `CliChannel | (string & {})` rather than `string | CliChannel` — the latter collapses to plain `string` and you lose editor suggestions for the two tags that are the point of the option. - `packages/sandbox`'s `test` script named its one test file literally, so it would have silently skipped the new one. It is now `vitest run test`; `docker.test.ts` already self-skips via `describe.runIf(process.env.SANDBOX_DOCKER_TESTS)`, so it stays inert. ## What is deliberately not here The `docker: 'no-daemon' | 'absent'` staging, `mountDockerSocket`, `needsDocker`, and the runtime marker are all in the follow-up, stacked on this branch. The `cli` eval/experiment suites, CODEOWNERS and workflow wiring come after that, once #315 lands. ## Verification `pnpm format:check`, `pnpm typecheck`, and the sandbox (89), core (162), framework (8), vercel-runner (28) and web (83) suites pass. `pnpm check`'s `test:framework` stage needs `AI_GATEWAY_API_KEY` for its judge and is not run in CI; every other stage was run.
Coly010
added a commit
that referenced
this pull request
Sep 23, 2026
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. ```ts 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 (supabase/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.
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change organizes experiments under per-owner folders so CODEOWNERS can scope them by path. Experiment IDs and result names remain unchanged.
AI-owned base configs live in
experiments/presets.ts. The AI experiments extend those presets fromexperiments/ai/.How to review
Start with
experiment-files.ts, thenpresets.tsand the two discovery call sites inrun-eval.tsandexport-results.ts.Dry run one of the moved experiments using its unchanged ID:
Verification
pnpm --filter @supabase-evals/framework typecheckpnpm --filter @supabase-evals/framework test:vercel-runner(26 tests passed)pnpm eval -- listreturned the same 12 experiment IDsclaude-code-sonnet-5pnpm formatContext: Slack thread