fix(hosts): gate host aliases on docker/vm drivers (#4516) - #4608
Conversation
Host-alias commands edit the legacy Kubernetes gateway `Sandbox` custom resource via `docker exec openshell-cluster-nemoclaw kubectl ...`. Docker- and vm-driver sandboxes run per-sandbox direct containers with no gateway cluster container and no `Sandbox` CR, so `nemoclaw <sandbox> hosts-list/-add/-remove` failed before reading aliases with `No such container: openshell-cluster-nemoclaw`. Detect the sandbox `openshellDriver` from the registry and fail fast with an actionable unsupported-runtime message for the docker/vm direct-container drivers, instead of targeting a container that does not exist or pretending a one-time /etc/hosts edit would survive a restart/rebuild. Legacy k3s behavior (kubernetes/unrecorded driver) is preserved unchanged. Add CLI regression tests proving the docker and vm drivers are gated and never invoke the legacy gateway container, including the dry-run preview path. Signed-off-by: Yimo Jiang <yimoj@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughHost-alias CLI commands ( ChangesHost-alias driver gating
🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/actions/sandbox/host-aliases.ts`:
- Around line 84-88: The code currently swallows any errors from
registry.getSandbox by wrapping the lookup in a broad try/catch and defaulting
driver to null; remove that catch so registry lookup failures surface instead of
silently degrading behavior. Replace the try/catch around
normalizeDriver(registry.getSandbox(sandboxName)?.openshellDriver) with explicit
handling: call registry.getSandbox(sandboxName) first, if it returns null set
driver = null, otherwise pass its openshellDriver into normalizeDriver and let
exceptions from registry or normalizeDriver propagate to the caller (or handle
them at a higher system boundary); reference normalizeDriver,
registry.getSandbox, openshellDriver and the driver variable when making the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8b9b3c78-4301-457e-b005-9e46c8ddd706
📒 Files selected for processing (2)
src/lib/actions/sandbox/host-aliases.tstest/cli.test.ts
Address CodeRabbit review: the host-alias driver gate wrapped the registry lookup in a broad try/catch that defaulted the driver to null on any error, silently degrading to the legacy gateway path on a real registry failure. `registry.getSandbox` only throws on a ConfigPermissionError (a genuine system-boundary failure that every other registry-touching command already surfaces); missing/corrupt registries return an empty fallback. Read the driver directly so that failure propagates instead of being swallowed. Signed-off-by: Yimo Jiang <yimoj@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cjagwani
left a comment
There was a problem hiding this comment.
Approving. Targeted fix for #4516 — gates list/add/remove on docker/vm drivers with an actionable error before any docker exec openshell-cluster-nemoclaw attempt. The fall-through preserves the legacy k3s path for unknown/null drivers, which is the right backwards-compat choice.
Test coverage is solid: parameterized across both direct-container drivers, asserts the --dry-run path is gated too, and proves no docker exec is even attempted (via dockerLog never created). Error message is honest about why (no Sandbox CR + no persistent host-alias API + /etc/hosts wouldn't survive restart).
Summary
Host-alias commands (
hosts-list/hosts-add/hosts-remove) still assumed the legacy k3s gateway containeropenshell-cluster-nemoclawand the KubernetesSandboxcustom resource. Docker- and vm-driver sandboxes run per-sandbox direct containers with no gateway cluster container and noSandboxCR, so the commands failed before reading aliases withNo such container: openshell-cluster-nemoclaw. This gates those commands on direct-container drivers with an actionable message instead of targeting a container that does not exist.Related Issue
Fixes #4516
Changes
src/lib/actions/sandbox/host-aliases.ts: read the sandboxopenshellDriverfrom the registry and fail fast with an actionable unsupported-runtime message for thedockerandvmdirect-container drivers, before anydocker exec ... kubectlcall. Legacy k3s behavior (kubernetes/ unrecorded driver) is preserved unchanged. The message is explicit that OpenShell exposes no persistent host-alias API for these drivers and that a one-time/etc/hostsedit would not survive a restart/rebuild — so we do not pretend durability.test/cli.test.ts: added a parameterized regression test (docker + vm) proving the commands exit non-zero with the unsupported message and never invoke the legacy gateway container, including the--dry-runpreview path. Extended the testSandboxEntrytype withopenshellDriver.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Ran locally:
npm run typecheck:cli(pass), the fulltest/cli.test.tsCLI project — 172/172 passing (single-worker run; the shared host could not sustain the fully parallelnpm test, but the affectedcliproject is green), the plugin pre-commit Vitest hook (pass on commit),codex review(no actionable findings), plus a manual reproduction through the built CLI confirming docker/vm drivers are gated and the legacy k3s path still patches theSandboxresource for unrecorded drivers. Fullnpm testandprekwill run in CI.Signed-off-by: Yimo Jiang yimoj@nvidia.com
Summary by CodeRabbit
Bug Fixes
--dry-runcorrectly fails on incompatible driver configurations and does not emit a host-alias JSON preview.Tests
--dry-runto verify immediate failure and absence of legacy gateway operations for unsupported drivers.