feat: auto-detect container runtime, promote Apple container as default - #114
Closed
hermclaw wants to merge 2 commits into
Closed
feat: auto-detect container runtime, promote Apple container as default#114hermclaw wants to merge 2 commits into
hermclaw wants to merge 2 commits into
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 boldblackai#114; RFC amended with a superseding note.
Collaborator
|
Superseded by #179 — same change rebased onto current main (this branch's fork permissions blocked pushing the rebase), plus two review fixes: |
capotej
added a commit
that referenced
this pull request
Sep 2, 2026
- 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).
capotej
added a commit
that referenced
this pull request
Sep 2, 2026
…lt (#179) * feat: auto-detect container runtime, promote Apple container as default - 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 * fix: keep =apple as deprecated alias and gate runtime auto-detect to 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. * refactor: address review feedback on runtime auto-detection - 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). * perf: memoize the container --version probe for the process lifetime 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. --------- Co-authored-by: hermclaw <279150486+hermclaw@users.noreply.github.com>
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.
Summary
Promotes Apple's
containerCLI as the default container runtime when available. Removes the need forHARNESS_CONTAINER_RUNTIME=apple— harness now auto-detects whethercontaineris on PATH and prefers it over docker. Theapplevalue is no longer accepted; useHARNESS_CONTAINER_RUNTIME=dockerto force docker.What changed
src/harness.tsselectRuntime()now auto-detects: checks ifcontainerbinary is on PATH viawhichSync(), prefers it, falls back to dockerHARNESS_CONTAINER_RUNTIME=dockerforces docker;auto(or unset) triggers auto-detectionHARNESS_CONTAINER_RUNTIME=appleis no longer a valid valueAppleContainerRuntime.ensureReady()error message to removeHARNESS_CONTAINER_RUNTIME=applereferenceswhichSync()— synchronouswhich/wherecross-platform PATH lookupTests
runtimeArgsAny()helper tohelpers.mjsfor runtime-agnostic test assertionsruntimeArgsAny()instead ofdockerArgs()Docs
Test plan
pnpm build)