Improve#2
Open
DimitriGilbert wants to merge 8 commits into
Open
Conversation
- bin/edit: fix help text from 'add' to 'edit a server' - bin/remove: add --yes/-y flag with confirmation prompt - bin/connect: record connection history before SSH call - bin/doctor: add rsync to required tools check - completely.yaml/bash: add --yes/-y to remove completion - utils/install: add --quiet support - utils/get_sshm0: add --quiet support
- bin/import: import servers from SSH config file (--file, --force, --dry-run, --tag) - bin/tags: list all tags with server counts (--long for server names) - bin/history: view connection history (--count, --server filter) - bin/config: view/edit global config (list, get, set, plugins) - Updated sshm0, completely.yaml, completely.bash with all new subcommands
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (24)
📝 WalkthroughWalkthroughThis PR extends ChangesServer configuration model expansion with timeout, proxy, and group fields, plus new utility commands
Sequence Diagram(s)No sequence diagrams generated. While the PR introduces several new command flows (backup, config, import, history), the changes to existing commands are primarily configuration and option handling rather than multi-component sequential workflows. The new commands are largely self-contained utilities without significant cross-component interactions. Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
completely.yaml (1)
1-17:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
tagsandhistoryto top-level command completion list.
sshm0 tagsandsshm0 historyare defined later, but they’re missing from the rootsshm0:command list.Suggested fix
sshm0: - list - add - config - connect - cp - edit - plugin - remove - rename - show - export - import - ping - doctor +- tags +- history - backup - --config-dir🤖 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 `@completely.yaml` around lines 1 - 17, Update the top-level sshm0 command list to include the missing commands by adding "tags" and "history" to the array under the sshm0 key so the root completion includes sshm0 tags and sshm0 history; locate the sshm0 top-level list (containing list, add, config, ...) and insert "tags" and "history" alongside the existing entries.bin/edit (1)
367-390:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInteractive edit flow doesn’t cover timeout.
Line 367 and the interactive prompts omit timeout, so users can’t update timeout interactively in
editmode.Suggested fix
- if [ -z "$_arg_ip" ] && [ -z "$_arg_user" ] && [ -z "$_arg_password" ] && [ -z "$_arg_key" ] && [ -z "$_arg_auth" ] && [ -z "$_arg_port" ] && [ -z "$_arg_tag" ] && [ -z "$_arg_group" ] && [ -z "$_arg_proxy" ]; then + if [ -z "$_arg_ip" ] && [ -z "$_arg_user" ] && [ -z "$_arg_password" ] && [ -z "$_arg_key" ] && [ -z "$_arg_auth" ] && [ -z "$_arg_port" ] && [ -z "$_arg_tag" ] && [ -z "$_arg_timeout" ] && [ -z "$_arg_group" ] && [ -z "$_arg_proxy" ]; then @@ read -rp "Tags (comma-separated) [$(IFS=,; echo "${sshm0_server_tags[*]}")]: " _arg_tag _current_tags="$(IFS=,; echo "${sshm0_server_tags[*]}")" _arg_tag="${_arg_tag:-$_current_tags}" + read -rp "Timeout in seconds [${sshm0_server_timeout:-10}]: " _arg_timeout + _arg_timeout="${_arg_timeout:-${sshm0_server_timeout:-10}}" read -rp "Group [$sshm0_server_group]: " _arg_group _arg_group="${_arg_group:-$sshm0_server_group}"🤖 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 `@bin/edit` around lines 367 - 390, The interactive edit flow is missing the timeout prompt so users can't change server timeout; add a prompt like the others in the same block to read and set _arg_timeout using the existing stored value sshm0_server_timeout as the default (e.g., read -rp "Timeout in seconds [$sshm0_server_timeout]: " _arg_timeout; _arg_timeout="${_arg_timeout:-$sshm0_server_timeout}"), and ensure subsequent code that writes/uses _arg_timeout matches how other fields (like _arg_port, _arg_proxy) are handled so the edited timeout is persisted.
🧹 Nitpick comments (1)
bin/remove (1)
225-225: ⚡ Quick winAdd success feedback and error handling.
The removal executes silently (especially when using
--yes), and there's no error checking on thermcommand. Adding feedback improves UX consistency since line 227 already prints an error for "not found" cases.✨ Proposed enhancement
- rm "$(sshm0_server_config_path "$_arg_name")"; + if rm "$(sshm0_server_config_path "$_arg_name")"; then + echo "Server '$_arg_name' removed successfully." + else + echo "Failed to remove server '$_arg_name'." >&2 + exit 1 + fi🤖 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 `@bin/remove` at line 225, The removal call using rm "$(sshm0_server_config_path "$_arg_name")" currently runs silently and lacks error handling; update the block that calls sshm0_server_config_path with _arg_name (and respects the --yes flow) to test rm's success and report back: first check whether the target exists, run rm and capture its exit status, on non-zero exit print a clear error (including the path and exit/status message) similar to the existing "not found" error, and on success print a confirmation like "Removed <path>"; ensure errors from rm are not swallowed so callers and CI see failures.
🤖 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 `@bin/add`:
- Around line 384-386: The interactive add flow currently reads _arg_group and
_arg_proxy but never prompts for the new timeout field; add a read prompt for
the timeout (e.g., read -rp "Timeout [<default>]: " _arg_timeout) in the same
interactive block where _arg_group and _arg_proxy are read, ensure the variable
name matches the rest of the script (use _arg_timeout) and handle empty input as
the default value the script expects so subsequent logic that uses _arg_timeout
sees the intended value.
In `@bin/backup`:
- Around line 264-265: The script currently runs the tar command (tar -czf
"$_output" -C "$(dirname "$SSHM0_CONFIG_DIR")" "$(basename
"$SSHM0_CONFIG_DIR")") and then unconditionally calls log "Backup created:
$_output" 1; change this so you check tar's exit status and only call log on
success. Concretely, run the tar command, capture its exit code ($? or use &&),
and if it succeeds call log "Backup created: $_output" 1; if it fails, call log
with an error message including the tar exit code and/or stderr and exit with a
non-zero status so failures are not reported as successful backups.
- Line 252: The tar extraction at tar -xzf "$_arg_restore" -C "$(dirname
"$SSHM0_CONFIG_DIR")" allows path traversal; before extracting, validate the
archive members (use tar -tzf "$_arg_restore") and reject any entry that is
absolute (starts with '/') or contains '..' path segments or leads outside the
intended directory, or alternatively extract into a temporary directory and move
only normalized, whitelisted paths into the target; then run the actual
extraction to the safe directory (dirname "$SSHM0_CONFIG_DIR") only after
verification to ensure no member can write outside the config dir.
In `@bin/config`:
- Around line 276-277: The code uses eval to read a variable by name (setting
_eval_val via eval "echo \"\${$_arg_key}\"") which risks executing
user-controlled content; instead validate _arg_key against a safe variable-name
pattern (e.g. ^[A-Za-z_][A-Za-z0-9_]*$) and then perform indirect expansion to
read the variable value (use the ${!_arg_key} form) and keep the existing
non-empty check on _eval_val; ensure you bail or error if the key validation
fails.
- Around line 292-296: Validate that _arg_key is a safe shell variable name
(e.g. match ^[A-Za-z_][A-Za-z0-9_]*$) and fail early if it does not, then
shell-escape _arg_value before writing to _config_file; replace constructing
_entry directly with something like: verify _arg_key with a regex and exit
non-zero on failure, compute an escaped value via printf '%q' "$_arg_value" (or
an equivalent escaping routine) and then build the entry from the validated key
and escaped value, and finally use the validated key when checking/updating the
file (grep/sed) so you never write raw/unescaped values or malformed keys into
_config_file.
In `@bin/export`:
- Around line 274-275: The export output omits the sshm0_server_proxy field;
update the export generation (where the timeout is echoed, e.g., the echo
printing "# Timeout: ${sshm0_server_timeout:-10}s") to also emit the proxy as a
comment immediately after that line by echoing the sshm0_server_proxy value
(e.g., "# Proxy: ${sshm0_server_proxy}") so the exported SSH config includes the
proxy reference for consistency with sshm0 show (note: keep it as a comment
since it is a reference, not a real SSH directive).
In `@bin/history`:
- Around line 107-117: The --count/-n parsing currently assigns _arg_count in
the branches for -n|--count, --count=* and -n* without validating it, which
leads to shell errors when non-numeric values are used; update the parsing to
validate that _arg_count matches a positive-integer regex (e.g. '^[1-9][0-9]*$')
after assignment (or centralize validation after option parsing) and call die
with a clear message if validation fails, ensuring all branches that set
_arg_count (the -n|--count handler, the --count=* handler, and the -n* handler)
use this validation before _arg_count is used later.
In `@bin/import`:
- Around line 359-367: The current Host parsing treats the entire "Host ..."
line as a single name and fails on multiple names or glob patterns; change the
logic around _flush_host and _host_name to split the string after "Host " into
separate tokens, trim each token, and iterate over them instead of assigning the
whole remainder to _host_name; for each token, skip any that are '*' or contain
shell glob characters (e.g., '*', '?', '[') or patterns like '*.' and only
assign valid plain hostnames to _host_name and call _flush_host (or process) per
valid token so aliases like "Host a b" or patterns "web-*" don't abort the
import.
- Around line 321-346: The current import logic writes server config files
without validating required fields (e.g., _host_user, _host_hostname/_host_name)
and auth combinations (_auth vs _key/password), causing unusable configs; before
printing to _tmpcfg and moving it to sshm0_server_config_path, add validation
checks that ensure _host_name and _host_user are non-empty, verify _auth is one
of the supported values and that corresponding credentials are present (e.g., if
_auth indicates key, ensure _key is non-empty; if password, ensure a password is
provided), reject or normalize invalid tag input from _arg_tag similarly, and
exit with die and cleanup of _tmpcfg on validation failure so the printf block
and mv are only executed when fields are valid.
In `@bin/utils`:
- Around line 122-128: sshm0_get_proxy_args can emit a malformed "user@ip:port"
when proxy fields are empty; change it to validate the loaded proxy profile
before echoing by checking that sshm0_server_user, sshm0_server_ip and
sshm0_server_port are non-empty after sshm0_load_server "$sshm0_server_proxy"
and return failure if any are missing (i.e., only build and print
"${sshm0_server_user}@${sshm0_server_ip}:${sshm0_server_port}" when all three
variables are set), keeping the existing early exits on load failures.
- Around line 88-120: The centralized validator sshm0_validate_server currently
misses timeout validation—update its signature to accept a 6th argument
(_timeout) and validate it as a numeric, non-negative integer in an acceptable
range (e.g., integer >= 0 or >=1 depending on desired semantics) to prevent
injecting invalid values into SSH ConnectTimeout; if invalid, print a clear
error and return non-zero. Then update the callers (bin/add and bin/edit) to
pass the timeout as the 6th parameter when calling sshm0_validate_server so
validation runs for saved server entries.
In `@sshm0`:
- Line 13: The subcommand declarations for parseArger (the one-of list in the
parser directive) and the user-facing help text are out of sync: the parser
includes "remove" while the declaration on the parseArger line shown omits it,
and the help text near the help block (around the existing "what to do"
description) also omits several supported commands ("config", "import",
"history", "backup", "remove"). Update the parseArger one-of list to include any
missing subcommands (add "remove" if absent) and update the help text string to
enumerate the exact same set of subcommands used by the runtime parser (ensure
"config", "import", "history", "backup", and "remove" are listed), keeping the
canonical set of commands consistent between the parseArger directive and the
help output.
In `@utils/install`:
- Around line 137-144: The --quiet) case currently only checks argument count
and can consume option-like tokens; update the guard in the --quiet) branch to
also reject $2 when it begins with '-' or is not a numeric level: replace the
existing if [ $# -lt 2 ]; then ... else ... block with a check like if [ $# -lt
2 ] || [ "${2#-}" != "$2" ] || ! printf '%s' "$2" | grep -Eq '^[0-9]+$'; then
decrement _verbose_level as before; else assign _verbose_level="-$2", shift, and
continue. Refer to the --quiet) case and the _verbose_level variable to locate
and apply this change.
---
Outside diff comments:
In `@bin/edit`:
- Around line 367-390: The interactive edit flow is missing the timeout prompt
so users can't change server timeout; add a prompt like the others in the same
block to read and set _arg_timeout using the existing stored value
sshm0_server_timeout as the default (e.g., read -rp "Timeout in seconds
[$sshm0_server_timeout]: " _arg_timeout;
_arg_timeout="${_arg_timeout:-$sshm0_server_timeout}"), and ensure subsequent
code that writes/uses _arg_timeout matches how other fields (like _arg_port,
_arg_proxy) are handled so the edited timeout is persisted.
In `@completely.yaml`:
- Around line 1-17: Update the top-level sshm0 command list to include the
missing commands by adding "tags" and "history" to the array under the sshm0 key
so the root completion includes sshm0 tags and sshm0 history; locate the sshm0
top-level list (containing list, add, config, ...) and insert "tags" and
"history" alongside the existing entries.
---
Nitpick comments:
In `@bin/remove`:
- Line 225: The removal call using rm "$(sshm0_server_config_path "$_arg_name")"
currently runs silently and lacks error handling; update the block that calls
sshm0_server_config_path with _arg_name (and respects the --yes flow) to test
rm's success and report back: first check whether the target exists, run rm and
capture its exit status, on non-zero exit print a clear error (including the
path and exit/status message) similar to the existing "not found" error, and on
success print a confirmation like "Removed <path>"; ensure errors from rm are
not swallowed so callers and CI see failures.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d9d77af-1ca9-4ff2-9954-25affcc48d25
⛔ Files ignored due to path filters (1)
bin/tagsis excluded by!**/tags,!**/TAGS
📒 Files selected for processing (22)
bin/addbin/backupbin/configbin/connectbin/cpbin/doctorbin/editbin/exportbin/historybin/importbin/listbin/pingbin/removebin/rsyncbin/showbin/utilscompletely.bashcompletely.yamlserver.distsshm0utils/get_sshm0utils/install
- bin/utils: add sshm0_validate_server and sshm0_get_proxy_args - bin/add: add --timeout, --proxy/-J, --group/-g options - bin/edit: pass through timeout, proxy, group to add - bin/connect: use server timeout and ProxyJump - bin/cp, bin/rsync: use timeout and proxy - bin/ping: use server timeout instead of hardcoded 5 - bin/show: display timeout, proxy, group - bin/export: output timeout comment, ProxyJump, group comment - bin/list: add --group filter and --tree view - bin/backup: create/restore tar.gz backups - server.dist: add timeout, proxy, group fields - completions: add all new options
- bin/exec: run commands on multiple servers (--tag, --group, --parallel) - bin/update: self-update via git (--check, --force) - bin/utils: add sshm0_encrypt_password/decrypt_password using openssl - bin/add: encrypt passwords at rest with enc: prefix - bin/connect/cp/rsync: decrypt passwords before use - bin/show: display (encrypted) for encrypted passwords - completely.zsh: full Zsh completion support for all subcommands - sshm0.rc: detect shell type (bash/zsh) for completion - utils/install: handle zshrc files
- documentation.md: rewrite with accurate help for all 20 subcommands - readme.md: document all new features (import, tags, history, config, groups, timeout, ProxyJump, backup, exec, update, encryption, Zsh) - sshm0: ensure all 20 subcommands registered - completely.yaml/bash: add plugin case, rsync section, all subcommands - bin/doctor: add optional openssl check
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.
Summary by CodeRabbit
New Features
configcommand.--yesflag.Improvements