feat(cli): redesign command surface into 5 task-oriented groups - #8
Merged
Conversation
Clean break (no aliases), layered on fix/comprehensive-remediation to resolve
the command-surface fragmentation that branch left untouched.
Lifecycle: run / run --background (was `daemon`), stop, restart (NEW),
status (rewritten), doctor.
Session: login now onboards (login + sync + client-side PIN setup =
Remembered Device) and hands keys to the running agent; sync; forget.
Lock: unlock --method <auto|pin|hello|native> replaces the four bool
flags; `unlock --password` removed (master password lives in login).
Keys: `keys` is an offline unified view (identity + bound hosts +
selector + ssh-config Include); keys bind|unbind|ui absorb the
former `bindings` group.
Integration: ssh-config show|write|remove (write is offline; merges
--write/install/regenerate); startup enable|disable (was
`daemon --install/--uninstall`); enable refuses without a
Remembered Device.
Agent IPC: add ControlAction::Stop; the run_foreground select! loop breaks on
it to run the normal cancel-token shutdown. stop/restart drive it.
Legibility: status renders a readable Agent/Device/Lock/Bitwarden/Sync/Keys
block with a state-driven Next hint, falling back to local files
when the agent is down; cmd_control exits non-zero on daemon-down /
command failure; corrected misleading help + error strings;
autostart entries now invoke `run --background`.
Docs: README usage, llmdoc CLI guide + IPC reference (add `stop`), and
host-bindings docs updated to the new surface.
cargo fmt / clippy -D warnings / test --workspace all clean.
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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors the CLI to a run/stop/restart/unlock-method/keys/ssh-config/startup surface, adds ControlAction::Stop and ControlUnreachable for graceful shutdown, rewires runtime/control handling, implements offline keys/ssh-config write, updates onboarding/pin flow, and aligns docs and platform autostart to use run --background. ChangesCLI Command Restructuring
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The 5-group redesign (07d710a) left a few baked-in strings and one control-flow edge still pointing at the old surface: - doctor migration hint: `unlock --pin` -> `unlock --method pin` - generated ~/.ssh/sshwarden_config header: `bindings ...` -> `keys bind/unbind ...` - `doctor --fix` include placeholder now points at `ssh-config write` (the command that actually populates the snippet, not the removed snippet-printing path) - BUILD.md dev-run examples: `daemon` -> `run`, `unlock --hello` -> `unlock --method hello` stop: a clean shutdown often tears down the control channel before its reply reaches the client. cmd_stop no longer exits(2) on that disconnect; it falls through to the existing 5s PID-file wait loop, so a successful stop reports "Agent stopped." (exit 0) instead of a false failure. A genuinely wedged agent still surfaces "still running after 5s" + exit 1. cargo fmt / clippy -D warnings / test --workspace all clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/main.rs`:
- Around line 592-595: The current check using is_daemon_running() before
performing stop/restart makes control commands no-ops when the PID file is
missing or stale; instead, call the agent control/IPC path unconditionally
(e.g., attempt the control socket or send_control_command) in the stop/restart
command handling and only fall back to PID-file logic if the IPC attempt returns
an explicit "no daemon" error; update the branches around is_daemon_running()
and the sections that call out_line("No running SSHWarden agent.") so that stop
and restart first try to contact the running daemon (handle connection errors
and timeouts), and only if that contact proves the daemon is unreachable should
you treat it as not running and proceed with the existing fallback behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ae828ae8-989c-47dc-84e7-2c8acfd79cbf
📒 Files selected for processing (9)
.github/workflows/release.ymlBUILD.mdREADME.mdcrates/sshwarden-agent/src/control.rsdocs/host-bindings-followup.mddocs/host-bindings.mdllmdoc/guides/how-to-use-cli-commands.mdllmdoc/reference/ipc-control-protocol.mdsrc/main.rs
`cmd_stop`/`cmd_restart` were the only control commands gated on the PID-file heuristic `is_daemon_running()`; every other command contacts IPC directly. Since the PID file is created/removed independently of the control pipe/socket, an externally deleted or stale PID file made `stop` a silent no-op against a live daemon and -- worse -- made `restart` skip stopping the running daemon and spawn a second agent that fights it for the SSH endpoint and control pipe. Contact the daemon over the control channel unconditionally and fall back to "not running" only when the channel is genuinely unreachable: - control.rs: add typed `ControlUnreachable` error, returned only on connect failure, so callers distinguish "no daemon" from "channel dropped mid-reply" (the expected outcome of a clean stop). Its Display matches the previous message, so other call sites are unchanged. - main.rs: add `contact_daemon()` -- sends with a 5s timeout and classifies the result as Replied / Unconfirmed / NotRunning. `stop` short-circuits only on NotRunning; `restart` stops whenever IPC reaches a daemon, preventing the competing-daemon spawn. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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.
Builds on #7 (now merged to
main). This is a clean-break redesign of the CLI command surface — the "command fragmentation" that #7's correctness/security work intentionally left untouched. The diff is exactly one commit on top of currentmain.What changes (clean break — no aliases)
run/run --background(wasdaemon),stop,restart(new),status(rewritten),doctor.loginnow onboards — login + sync + client-side PIN setup = Remembered Device — and hands keys to the running agent;sync;forget.unlock --method <auto|pin|hello|native>replaces the four bool flags;unlock --passwordremoved (master password lives inlogin).keysis a unified offline view (identity + bound hosts + selector + ssh-config Include);keys bind|unbind|uiabsorb the formerbindingsgroup.ssh-config show|write|remove(writeis offline; merges--write/install/regenerate);startup enable|disable(wasdaemon --install/--uninstall) —enablerefuses without a Remembered Device.Removed (no aliases):
daemon,unlock --password, thebindingsgroup,ssh-config --write/install/uninstall/regenerate/statussubverbs.Notable internals
ControlAction::Stop; therun_foregroundselect!loop breaks on it to run the normal cancel-token shutdown.stop/restartdrive it.statusrenders a readable Agent/Device/Lock/Bitwarden/Sync/Keys block with a state-driven Next hint, falling back to local files when the agent is down.cmd_controlnow exits non-zero on daemon-down / command failure (was always exit 0).run --background(so auto-start no longer calls the removeddaemon).Deliberately left out
build_status_responsestill composes the legacy jargon human-message, but the newcmd_statusrenders from thedetailsJSON and never shows it — effectively demoted without touching daemon-side test expectations.llmdoc/architecture/ipc-control-channel.md(historical analysis, disclaimed in README) left as-is; the canonicalllmdoc/reference/ipc-control-protocol.mdaddsstop.Verification
cargo fmt --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace— all clean.--helptree + precondition tags;unlock --methodenum;keysoffline view;statusblock (incl. agent-down fallback);stopidempotent (exit 0);daemon/unlock --passwordnow error (exit 2).loginPIN onboarding,restartbackground respawn,startup enableinstall — recommend a soak before release.Breaking change
User-facing breaking change to the CLI. Docs (README, llmdoc guide + IPC reference, host-bindings docs) are updated in the same commit.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
runsupports--background(replacing old daemon flow)startupgroup to enable/disable auto-start (usesrun --background)unlockwith--method(auto|pin|hello|native);keysandssh-configreorganized (bind/unbind/ui, show/write/remove)Documentation