feat: auto-detect container runtime, promote Apple container as default - #179
Conversation
- Remove HARNESS_CONTAINER_RUNTIME=apple; instead auto-detect whether Apple's container CLI is on PATH, preferring it over docker - Add whichSync() helper for synchronous PATH lookup - HARNESS_CONTAINER_RUNTIME=docker still supported to force docker - Accept 'auto' as explicit no-op value (same as unset) - Update error messages, USAGE help text, README, AGENTS.md, CHANGELOG - Update all e2e tests to use runtime-agnostic assertions via runtimeArgsAny() helper; only docker-specific tests force docker via HARNESS_CONTAINER_RUNTIME=docker
…macOS Review feedback on the auto-detect change: - HARNESS_CONTAINER_RUNTIME=apple no longer hard-fails. It is now a deprecated explicit alias: still selects the apple runtime, prints a one-time deprecation warning, so existing dotfiles keep working across the upgrade. - Auto-detection is gated to process.platform === "darwin". apple/container ships for macOS only, so a binary named `container` on a Linux PATH is an unrelated tool that must not hijack runtime selection; docker stays the default everywhere else. - ensureReady() error message no longer assumes the auto-detected path. - Tests: apple-argv tests pin =apple explicitly (deterministic on all platforms), new tests for the deprecation alias and the non-macOS PATH-collision guard. - Docs: README/AGENTS.md/CHANGELOG updated; CHANGELOG #TBD replaced with #114; RFC amended with a superseding note.
BoldBlackBot
left a comment
BoldBlackBot
left a comment
There was a problem hiding this comment.
Review: auto-detect container runtime
The design is right — darwin-gated auto-detect, warn-not-fail =apple alias, deliberate test pinning (=docker for docker-shape assertions, runtimeArgsAny for runtime-agnostic ones), RFC amendment. Two functional items and two nits inline.
On the "standard way" question: there is none in core — no util.which or any binary-lookup API exists as of Node 26 (checked current docs; CI matrix is 22/24). npm which is real but not worth breaking the single-runtime-dep posture for ~15 lines. The standard pattern is EAFP: don't sniff PATH, just try executing the binary — which ensureReady() already does. Details inline.
Design consideration (non-blocking): an auto-detected apple runtime that half-works (version probe passes but container system start / kernel never configured) hard-fails at run, where plain docker "worked yesterday". If that bites users, one mitigation: when selection came from auto-detect (not explicit =apple), let ensureReady() failure fall back to docker with a warning instead of exit(1) — keep the hard fail for explicit selection. Note only; the RFC's trade-off is accepted.
| } | ||
|
|
||
| /** Synchronous `which` — returns true if the binary is found on PATH. */ | ||
| function whichSync(name: string): boolean { |
There was a problem hiding this comment.
More standard than shelling out to which: drop the PATH pre-check and reuse the probe ensureReady() already runs. execFileSync("container", ["--version"]) succeeding is strictly stronger evidence than which container — it proves the binary actually executes (broken install, quarantine attribute, wrong-arch build all fail here), and execution is the only thing selection depends on:
function appleContainerAvailable(): boolean {
try {
execFileSync("container", ["--version"], { stdio: "ignore", timeout: 5000 });
return true;
} catch {
return false;
}
}Then in selectRuntime(): if (process.platform === "darwin" && appleContainerAvailable()).
This deletes whichSync entirely, including its dead win32/where branch (unreachable behind the darwin gate), plus the corner cases (2s timeout, which itself missing). Cost: the probe runs again in ensureReady() right after — either accept the ~ms double-run or hoist the result into the instance. For context: Node core has no util.which/binary-lookup API (checked Node 26 docs), and npm which isn't worth a new runtime dep here.
| cwd: WORK_DIR, | ||
| env: { | ||
| ...process.env, | ||
| PATH: `${dockerOnlyDir}:${process.env.PATH}`, |
There was a problem hiding this comment.
This prepends dockerOnlyDir but keeps the runner's full process.env.PATH, so it doesn't actually create a docker-only PATH. On a macOS dev machine with the real container installed, auto-detect selects the real binary → no DOCKER_INVOKED line → this test fails (and it execs the real CLI against a nonexistent image tag along the way). The old version of this test filtered container-containing dirs out of PATH — that filtering is what made the premise true. Suggest restoring it, or building PATH as dockerOnlyDir + only the dirs node itself needs.
| export HARNESS_CONTAINER_RUNTIME=docker | ||
| ``` | ||
|
|
||
| `HARNESS_CONTAINER_RUNTIME=apple` (the pre-auto-detect opt-in) still works but is deprecated: it selects the apple runtime explicitly, prints a one-time deprecation warning, and will be removed in a future release. |
There was a problem hiding this comment.
Nit: "prints a one-time deprecation warning" — it warns on every invocation; there's no state. Drop "one-time".
| entry under Key subsystems, and the architecture overview's spawn step now | ||
| reads `<runtime> run`. | ||
|
|
||
| ## Amendment: auto-detection supersedes explicit opt-in (2026-09-02, PR #114) |
- Replace the whichSync PATH pre-check with appleContainerAvailable(), which reuses the same `container --version` exec probe ensureReady() runs. Strictly stronger evidence: proves the binary executes (broken install, quarantine attribute, wrong-arch build all fail), which is what selection depends on. Deletes whichSync entirely, including its dead win32 branch. The probe runs once more in ensureReady() right after — accepted cost, bounded by the same 5s timeout. - docker-only auto-detect test: filter container-containing dirs out of PATH instead of merely prepending the docker shim dir, so the test's premise holds on macOS dev machines with the real container CLI installed. - README: drop 'one-time' from the =apple deprecation warning description. - RFC amendment header: reference both #114 (draft) and #179 (landed).
|
Addressed all four BoldBlackBot comments in 30210ef:
Local: 131/131 e2e, biome + markdownlint clean. |
selectRuntime() and AppleContainerRuntime.ensureReady() both needed the same `container --version` evidence; appleContainerAvailable() now caches its result in a module-level memo so the exec runs once per process. ensureReady() consumes the memo instead of re-probing. The cache lives only for the process lifetime — never persisted — since the CLI's install state can change between harness invocations.
3 participants
Supersedes #114 (same change, rebased onto current main + review fixes; the original branch lived in a fork whose permissions blocked the rebase push).
Summary
Promotes Apple's
containerCLI as the default container runtime via auto-detection.HARNESS_CONTAINER_RUNTIME=appleis no longer needed — on macOS, harness auto-detects whethercontaineris on PATH and prefers it over docker. SetHARNESS_CONTAINER_RUNTIME=dockerto force docker.What changed
src/harness.tsselectRuntime()auto-detects: on macOS (process.platform === "darwin"), prefers Apple'scontainerCLI if on PATH via newwhichSync(), falls back to docker. The darwin gate exists because apple/container ships for macOS only — a same-named binary on a Linux PATH is an unrelated tool and must not hijack selection (docker stays default everywhere else)HARNESS_CONTAINER_RUNTIME=appleis kept as a deprecated alias instead of hard-failing: it still selects the apple runtime but prints a deprecation warning, so existing dotfiles keep working across the upgradeHARNESS_CONTAINER_RUNTIME=auto(or unset) triggers auto-detection;dockerforces docker; any other value is a hard errorAppleContainerRuntime.ensureReady()error messageTests
=appledeprecated alias warns but still selects the apple runtimeruntimeArgsAny()helper tohelpers.mjsfor runtime-agnostic assertions; docker-specific tests (hardening flags,--port, repeated--env-file, home-guard mounts) pinHARNESS_CONTAINER_RUNTIME=dockerexplicitly so they stay deterministic on macOS dev machines--platformpull) pin=appleexplicitly, which also exercises the deprecated alias end-to-endDocs
2026-06-20_container_runtime.mdamended with a superseding note documenting the auto-detect design changeTest plan
--portand DNS tests)pnpm build)=applewarns+container,=docker, garbage value errors