fix(sandbox): detect an available login shell instead of hardcoding /bin/bash

[akram]

Summary

The built-in default sandbox command and the interactive SSH session hardcoded /bin/bash. Minimal images such as Alpine ship only /bin/sh (BusyBox ash), so sandbox startup failed with an opaque failed to spawn sandbox entrypoint process: No such file or directory (os error 2) that never named the missing binary. This resolves a login shell that actually exists in the sandbox image, and names the program in the spawn error.

Related Issue

Refs #3146. This addresses the /bin/bash hardcoding root cause. Note: a fully stock Alpine image additionally requires the Landlock filesystem baseline (PROXY_BASELINE_READ_ONLY) to grant /bin and /sbin on non-usr-merged images — a separate root cause I will file and fix on its own.

Changes

• Add openshell-core::shell: shell-path constants (BASH, POSIX_SH, SHELL_CANDIDATES) and a runtime detect_login_shell() that resolves a shell present in the current root filesystem ($SHELL if executable, then bash, then /bin/sh), plus is_executable().
• openshell-sandbox (main.rs): when the command is the built-in default, remap its shell to a detected one. Only the default is remapped — explicit user commands are never rewritten. Resolution runs in the supervisor so it inspects the sandbox filesystem, not the gateway's.
• openshell-supervisor-process (ssh.rs, process.rs): use the detected shell for the interactive SSH session and the SHELL env var.
• Name the program in the entrypoint spawn error so a missing shell/binary is diagnosable instead of a bare ENOENT.

Testing

Verified on OpenShift (ROSA HCP, Kubernetes compute driver):

• Before: an alpine:3.21 sandbox crash-looped with a bare ENOENT.

• After: the supervisor logs default shell not found in sandbox image; falling back to a detected shell, resolves /bin/sh, and the spawn error (when a binary is genuinely missing) now names it, e.g. failed to spawn sandbox entrypoint process '/bin/sh'. The Alpine sandbox reaches Running once /bin//sbin are also granted (tracked separately).

• Bash-based images are unaffected (bash is still preferred when present).

• mise run pre-commit passes — cargo fmt --check, cargo clippy -D warnings, license headers, and unit tests pass. The helm:lint step fails locally due to a missing postgresql chart dependency unrelated to this change.

• Unit tests added/updated — openshell-core::shell (4 tests).

• E2E tests added/updated (if applicable) — verified manually end-to-end on OpenShift; no automated e2e added.

Checklist

• Follows Conventional Commits
• Commits are signed off (DCO)
• Architecture docs updated (if applicable) — n/a

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[johntmyers]

gator-agent

PR Review Status

This is a valid, focused fix for #3146, but three reachable regressions remain. The first cannot be anchored inline because the hardcoded PTY calls are unchanged lines outside the current diff.

Action required: address all three blocking findings and add relevant Fern documentation for the user-visible sandbox/SSH behavior, or obtain a maintainer-authored explanation that docs are intentionally unnecessary.

Warning — GATOR-158c9cb0-01 · Interactive PTY SSH still requires bash

Summary: A user running a supported sh-only image can use the new non-PTY SSH path, but a normal interactive SSH session requests a PTY and still executes /bin/bash at crates/openshell-supervisor-process/src/ssh.rs:1281 and :1286. Those calls are outside this diff, so this finding is summarized here rather than anchored inline. The interactive session still fails on Alpine after the separate Landlock grant is present.

Fix: Detect the shell in spawn_pty_shell and use it in both command branches while preserving -i and login-command arguments; add a deterministic sh-only PTY SSH regression test.

Verify: Start a sh-only sandbox with the required filesystem grant and run ssh -tt; the PTY session must start with /bin/sh, while the non-PTY path must continue to work.

Agent context

• Location: crates/openshell-supervisor-process/src/ssh.rs:1441 introduces detection only for the neighboring non-PTY adapter.
• Ownership: The PR explicitly claims to fix interactive SSH but leaves its PTY adapter unchanged.

Blocking findings:

• GATOR-158c9cb0-01: interactive PTY SSH retains the hardcoded bash dependency.
• GATOR-158c9cb0-02: explicit scratch-shaped commands are silently rewritten.
• GATOR-158c9cb0-03: Unix-only shell tests fail in the supported Windows workspace lane.

Carried findings:

• None

Gator metadata

• Validation: Focused bug fix tied to #3146 with a clear supported-user path and reproduction.
• Docs: Missing for a direct user-visible sandbox and SSH behavior change; a maintainer-authored explanation may establish that docs are intentionally unnecessary.
• Checks: Current-head DCO, vouch, dependency review, and workflow security checks pass; required Branch Checks and Helm Lint have not been dispatched.
• E2E: Required because sandbox lifecycle and supervisor behavior change; test:e2e will be dispatched after review blockers are resolved.
• Head SHA: 158c9cb0656d306a805e85701e377baa0191c854
• Base SHA: 8e73f1db99e1a80d4f47303ce44e73a1204bf470
• Merge base SHA: 6c3980d01a7798cbfb9226c32a189a0726cc810c
• Patch ID: 01d5be0815eb1a998bf00af7890f05583463fb6f
• Gator payload: 8
• Review mode: initial
• Previous reviewed SHA: none
• Review budget exhausted: no
• Maintainer decision required: no
• Next state: gator:in-review

[sylvesterkaczmarek]

detect_login_shell() trusts any executable $SHELL, but the SSH path later invokes the result with -lc. An image with SHELL=/bin/false (or another executable that is not a compatible shell) passes detection and makes default/SSH command execution fail even though /bin/sh exists. Should $SHELL be restricted or probed for the shell semantics this code requires before preferring it?

[akram]

detect_login_shell() trusts any executable $SHELL, but the SSH path later invokes the result with -lc. An image with SHELL=/bin/false (or another executable that is not a compatible shell) passes detection and makes default/SSH command execution fail even though /bin/sh exists. Should $SHELL be restricted or probed for the shell semantics this code requires before preferring it?

That's correct. Good catch.

Then, I will drop the $SHELL preference entirely and resolve only from the known candidate list (/bin/bash , /usr/bin/bash , /bin/sh ). $SHELL doesn't add value here. As a freshly created sandbox rarely has a meaningful $SHELL , and the value is exactly what this code is trying to derive. Removing it also avoids the footgun you describe rather than trying to probe for "shell-ness".

(If there's a use case for honoring an operator-provided $SHELL, I'd gate it behind a basename allowlist of known shells instead — happy to go that way if preferred.)

[johntmyers]

gator-agent

Re-check After Author Update

Thanks @akram. I reviewed current head c37f287c632a1864b7dcdf8ac149f662be018088 after your September 3 comment and commit about removing the arbitrary $SHELL preference.

What I checked: the author-only delta in openshell-core::shell, plus all three durable findings from the prior Gator review. Shell selection now uses only the known candidate list, so the concern you acknowledged is resolved.

Disposition: partially resolved. The latest commit does not change the three existing Gator obligations.

Remaining items:

• GATOR-158c9cb0-01: make PTY-based interactive SSH use the detected shell and add the sh-only PTY regression test.
• GATOR-158c9cb0-02: preserve omitted-default provenance so an explicit scratch-shaped command is never rewritten.
• GATOR-158c9cb0-03: make the shell tests portable to the supported Windows lane.
• Add relevant Fern documentation for the user-visible sandbox/SSH behavior, or obtain a maintainer-authored explanation that docs are intentionally unnecessary.

Action required: @akram, please address the carried findings and docs gate in a new commit.

Gator metadata

• Validation: Focused bug fix tied to #3146 with a clear supported-user path and reproduction.
• Docs: Missing for a direct user-visible sandbox and SSH behavior change; a maintainer-authored explanation may establish that docs are intentionally unnecessary.
• Checks: Current-head gate publishers are queued or running; Branch Checks, Helm Lint, and required E2E have not been dispatched.
• E2E: Required because sandbox lifecycle and supervisor behavior change; dispatch waits until review blockers are resolved.
• Head SHA: c37f287c632a1864b7dcdf8ac149f662be018088
• Base SHA: 8e73f1db99e1a80d4f47303ce44e73a1204bf470
• Merge base SHA: 6c3980d01a7798cbfb9226c32a189a0726cc810c
• Patch ID: b5cf5c8505564a34fd3d35b994a3617f4e7ae2e4
• Gator payload: 8
• Review mode: follow_up
• Previous reviewed SHA: 158c9cb0656d306a805e85701e377baa0191c854
• Review budget exhausted: no
• Maintainer decision required: no
• Next state: gator:in-review

[github-actions]

Label test:e2e applied, but pull-request/3147 does not exist yet. A maintainer needs to comment /ok to test 1353fab2e1a82e0d08132e430b86664ce23e8500 to mirror this PR. Once the mirror exists, re-apply the label or re-run Branch E2E Checks from the Actions tab.

[johntmyers]

/ok to test 1353fab

[johntmyers]

gator-agent

Re-check After Author Update

Thanks @akram. I reviewed current head 1353fab2e1a82e0d08132e430b86664ce23e8500 after your update addressing the prior Gator review.

What I checked: the author-only delta from c37f287c632a1864b7dcdf8ac149f662be018088, all three durable findings, and the published sandbox documentation. The PTY path now uses the detected shell, explicit-command provenance is preserved, Unix-only tests are gated from Windows, and the Fern documentation describes the fallback behavior.

Disposition: resolved. No blocking findings remain. The required current-head Branch Checks, Helm Lint, and E2E workflows are queued or running.

Remaining items:

• No blocking review items remain.

Gator metadata

• Validation: Focused bug fix tied to #3146 with a clear supported-user path and reproduction.
• Docs: Updated in docs/sandboxes/manage-sandboxes.mdx; navigation changes are not needed for an existing page.
• Checks: Current-head Branch Checks and Helm Lint are queued or running.
• E2E: test:e2e applied; /ok to test 1353fab2e1a82e0d08132e430b86664ce23e8500 created the mirror and Branch E2E Checks is running.
• Head SHA: 1353fab2e1a82e0d08132e430b86664ce23e8500
• Base SHA: 8e73f1db99e1a80d4f47303ce44e73a1204bf470
• Merge base SHA: 6c3980d01a7798cbfb9226c32a189a0726cc810c
• Patch ID: 6ae14a24b564d6aa76c8c9b5226a3c6ce5b1d8fc
• Gator payload: 8
• Review mode: follow_up
• Previous reviewed SHA: c37f287c632a1864b7dcdf8ac149f662be018088
• Review budget exhausted: no
• Maintainer decision required: no
• Next state: gator:watch-pipeline

[akram]

/hold
Found a remaining reference to /bin/bash

[akram]

@johntmyers @sylvesterkaczmarek one more /bin/bash was hiding in the gateway itself: grpc/sandbox.rs substitutes /bin/bash -l for an omitted command before persisting SandboxSpec.command. That one is trickier than the supervisor-side ones: by the time the supervisor runs, the default is a non-empty command indistinguishable from a caller-supplied bash, so the supervisor can't tell whether it's safe to remap the shell for a bash-less image (Alpine).

The fix I have adds a default_command provenance flag on SandboxSpec (set by the gateway on substitution, propagated to the supervisor, which remaps only when it's set — never an explicit command). It's additive/backwards-compatible, but it does touch the public API, so I'd rather discuss before committing to it. Any preference on carrying the provenance this way vs. keeping it off the public spec?

[akram]

For the gateway-path piece (making the default-shell remap trigger when the gateway substitutes an omitted command), I put the change on a separate branch instead of pushing it here, since it touches the public SandboxSpec:

https://github.com/akram/OpenShell/tree/fix/sandbox-shell-detection-provenance (commit c40d4c49)

It adds a default_command provenance flag (set by the gateway, propagated to the supervisor, which remaps only when it's set). Verified no regression on a bash image end-to-end. Can we go with this approach?

If you'd rather not touch the public spec, the alternative keeps it internal: the gateway stops persisting a substituted /bin/bash -l and leaves SandboxSpec.command empty for the default case, then driver_sandbox_spec_from_public applies the default and sets an internal-only DriverSandboxSpec.default_command. That avoids any public API change, at the cost of the "persisted command is always explicit" invariant (command reads back empty for default sandboxes).

Which do you prefer?