feat: opt-in interactive bash + one-shot installer (v1.1.0)#11
Merged
Conversation
bash gains an `interactive` param (like `bg`): default stays the fast bare `sh -c`; `interactive=true` runs the command through `bash -ic`, sourcing the service user's ~/.bashrc so aliases and version managers (mise/nvm/rbenv) resolve. Startup job-control warnings (no controlling TTY under systemd) are discarded by redirecting the child's stdio to /dev/null and having the command re-point stdout+stderr at the log after startup — output stays clean. The shell is injected into JobStore (production = interactive bash, tests = hermetic sh -c) so test output never depends on the host's shell config; a new test proves alias expansion against a controlled rcfile. Also adds deploy/install.sh: a curl|sudo bash one-liner that downloads the latest release .deb, prompts for username/password, picks the run-as user, and starts the service. Docs (README, deploy, usage, architecture) updated with the installer, a curl verify/debug section, and the new param. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
WalkthroughAdds interactive bash execution, threads the new flag through ChangesInteractive bash execution and installer flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/tools/mod.rs (1)
117-142: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCover
interactive=trueat the tool boundary.This new public parameter is only exercised in
JobStoretests. A regression inBashArgsdeserialization/defaulting or theinteractive.unwrap_or(false)forwarding would still leave the suite green. Add oneTools::bash(...)or HTTP-level test that provesinteractive=truereaches the interactive shell path. As per path instructions,**/*: "Flag missing tests for new public behaviour."🤖 Prompt for 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. In `@src/tools/mod.rs` around lines 117 - 142, Add a tool-boundary test for the new public interactive behavior in Tools::bash, since current coverage only verifies JobStore internals. Create a test that invokes Tools::bash (or the HTTP endpoint that routes to it) with interactive=true and asserts it reaches the interactive shell path, so failures in BashArgs deserialization/defaulting or interactive.unwrap_or(false) forwarding are caught.Source: Path instructions
src/jobs/mod.rs (1)
153-200: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDon't hold the jobs mutex across blocking create/spawn.
run()keeps the map lock from id reservation throughstd::fs::File::create()andCommand::spawn(). Those are synchronous OS calls on the async request path, so slow disk or fork/exec stalls can block a Tokio worker and also freeze unrelatedpoll/list/killcalls behind the same mutex. Reserve the id first, then do file setup/spawn without the lock and offload the blocking file create. As per path instructions,src/**/*.rs: "Async end-to-end... spawn_blocking for blocking I/O" and "keep critical sections tiny."🤖 Prompt for 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. In `@src/jobs/mod.rs` around lines 153 - 200, The run() path holds the jobs mutex across synchronous File::create and Command::spawn, which can block the Tokio worker and delay unrelated requests. Update the job reservation flow around self.jobs.lock(), JobId::generate, and log_path creation so the lock is only used to reserve and insert the id, then release it before any blocking work. Move the log file creation to spawn_blocking or another blocking-safe path, and spawn the child without holding the mutex so the critical section stays tiny.Source: Path instructions
🤖 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 `@deploy/install.sh`:
- Around line 56-59: The service user selection in the install flow is
defaulting to SUDO_USER, which can widen privileges beyond the intended trust
boundary. Update the user prompt logic in the install script around RUN_USER so
the default is the dedicated mcp-ssh account, and only allow a broader account
when the operator explicitly enters it. Keep the existing user-existence check,
and make sure the default path for the service setup remains tied to the
low-privilege service user rather than the invoking admin account.
- Around line 75-79: The install script’s PUBLIC_HOST handling in the ENV_FILE
write path leaves stale MCP_SSH_ALLOWED_HOSTS values behind when the hostname is
blank. Update the install.sh logic around the PUBLIC_HOST check so a blank value
removes or clears the existing ENV_FILE instead of skipping it, ensuring the
rerun falls back to localhost-only behavior. Add a rerun smoke test covering the
blank-hostname path to prove the env file is cleared and the old allowlist does
not persist.
- Around line 68-71: Raw interpolation of USER_NAME and PASS1 into the TOML
output can break config.toml when values contain quotes, backslashes, or
newlines; update the install.sh config-writing logic around the CONF heredoc to
escape TOML string values before writing them. Use the existing credential setup
flow with mcp-ssh set-auth instead of manually embedding the password in the
config, and keep the generated user/pass entries valid for TOML parsing.
In `@docs/deploy.md`:
- Around line 51-52: The bearer-token step references bin/mcp-token, which is
unavailable in a .deb installation and breaks the packaged-install verification
flow. Update the deploy docs to make this step explicitly source-checkout only,
or replace it with a curl-based OAuth PKCE token flow that works after a package
install; keep the surrounding verify steps consistent with the install path
described in the same section.
---
Outside diff comments:
In `@src/jobs/mod.rs`:
- Around line 153-200: The run() path holds the jobs mutex across synchronous
File::create and Command::spawn, which can block the Tokio worker and delay
unrelated requests. Update the job reservation flow around self.jobs.lock(),
JobId::generate, and log_path creation so the lock is only used to reserve and
insert the id, then release it before any blocking work. Move the log file
creation to spawn_blocking or another blocking-safe path, and spawn the child
without holding the mutex so the critical section stays tiny.
In `@src/tools/mod.rs`:
- Around line 117-142: Add a tool-boundary test for the new public interactive
behavior in Tools::bash, since current coverage only verifies JobStore
internals. Create a test that invokes Tools::bash (or the HTTP endpoint that
routes to it) with interactive=true and asserts it reaches the interactive shell
path, so failures in BashArgs deserialization/defaulting or
interactive.unwrap_or(false) forwarding are caught.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 516cb816-313d-4137-a6c8-405e57fafba5
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
CLAUDE.mdCargo.tomlREADME.mddeploy/install.shdocs/architecture.mddocs/deploy.mddocs/usage.mdsrc/app_tests.rssrc/jobs/mod.rssrc/main.rssrc/tools/mod.rs
- Default service user to dedicated mcp-ssh, not $SUDO_USER; auto-provision it when missing (the .deb assumes but never creates it). - Escape username/password before writing config.toml (toml_escape). - Clear $ENV_FILE on blank hostname so reruns drop a stale allowlist. - docs: mark bin/mcp-token verify step as source-checkout-only (.deb lacks it). Co-Authored-By: Claude <noreply@anthropic.com>
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 `@docs/deploy.md`:
- Around line 51-54: The `bin/mcp-token` example is missing the credential
source, so update the deploy docs to show that it reads `MCP_SSH_USER` and
`MCP_SSH_PASS` from the environment or `.env`, not `/etc/mcp-ssh/config.toml`.
In the section around `TOKEN=$(bin/mcp-token)`, add a brief rule-first note and
an example showing those variables being set before invoking `bin/mcp-token`, so
the one-liner works as written.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: bc43591f-2953-45d8-b3a2-f7f0d768e639
📒 Files selected for processing (2)
deploy/install.shdocs/deploy.md
- bin/mcp-token requires MCP_SSH_USER/MCP_SSH_PASS from env or .env; the prior `TOKEN=$(bin/mcp-token)` one-liner failed as written. - Prompt for both via read (password via read -rsp, unset after) so the verify snippet is self-contained and accurate. Co-Authored-By: Claude <noreply@anthropic.com>
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 `@docs/deploy.md`:
- Around line 53-58: The token prompt flow in the deploy script is mangling
passwords containing backslashes because the hidden password read is not using
raw input. Update the two read prompts in the token bootstrap flow so both the
MCP_SSH_USER and MCP_SSH_PASS reads use raw mode, keeping the existing
identifiers MCP_SSH_USER, MCP_SSH_PASS, and bin/mcp-token path intact.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 943acbb7-6397-44bb-ab1c-983214b99096
📒 Files selected for processing (1)
docs/deploy.md
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
1 participant
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.
What
bashgains aninteractiveparam (likebg). Default stays the fast baresh -c;interactive=trueruns throughbash -ic, sourcing the service user's~/.bashrcso aliases and version managers (mise/nvm/rbenv) resolve. Startup job-control warnings (no controlling TTY under systemd) are discarded by redirecting the child's stdio to/dev/nulland having the command re-point stdout+stderr at the log after startup — output stays clean.JobStore(production = interactive bash, tests = hermeticsh -c) so test output never depends on the host's shell config. New test proves alias expansion against a controlled rcfile.deploy/install.sh:curl … | sudo bashone-liner — downloads the latest release.deb, prompts for username/password, picks the run-as user, writes a systemd drop-in, starts the service.Verified
bin/checkgreen (fmt + clippy -D warnings + 71 tests)..debas a systemd service running as a real user; curl end-to-end: discovery JSON,/mcp401 unauth, bad creds → 401 / good → 302,interactive=false→ll: not found,interactive=true→ll is aliased to `ls -alF'(clean, no job-control noise).🤖 Generated with Claude Code
Summary by CodeRabbit