Shell Script & Additional Flags#1
Merged
JerryAgbesi merged 8 commits intoJerryAgbesi:mainfrom Apr 18, 2026
Merged
Conversation
Updated the image in the README to a new asset.
📝 WalkthroughWalkthroughAdds CLI workflows for Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Root as RootCmd
participant Parser as TargetParser
participant Writer as SSHWriter
participant UI
participant Connect
CLI->>Root: invoke with --add alias target
Root->>Parser: ParseTarget(target)
Parser-->>Root: Host{User, Hostname, Port}
Root->>Writer: AddHost(configPath, Host with Alias)
Writer-->>Root: success / error
Root-->>CLI: print added host / return error
CLI->>Root: invoke (no --add), optional --find term
Root->>UI: prepareHostSelection(hosts, RunOptions)
UI-->>Root: selected Host
Root->>Connect: connect(selected Host)
Connect-->>Root: connection executed / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🧹 Nitpick comments (1)
cmd/root.go (1)
82-98: Read thefindvalue fromcmdinside this helper.
prepareHostSelectionalready receives the command, but the actual query comes from the package-globalfindQuery. That hidden dependency makes the helper easier to call incorrectly and lets the flag state diverge from the value actually used. Pull the value fromcmd.Flags().GetString("find")here instead.Possible refactor
func prepareHostSelection(cmd *cobra.Command, hosts []sshconfig.Host) (ui.RunOptions, []sshconfig.Host, error) { options := ui.RunOptions{} if !cmd.Flags().Changed("find") { return options, hosts, nil } - options.StartFiltering = findQuery == "" - if findQuery == "" { + query, err := cmd.Flags().GetString("find") + if err != nil { + return ui.RunOptions{}, nil, err + } + + options.StartFiltering = strings.TrimSpace(query) == "" + if strings.TrimSpace(query) == "" { return options, hosts, nil } - filtered := filterHosts(hosts, findQuery) + filtered := filterHosts(hosts, query) if len(filtered) == 0 { - return ui.RunOptions{}, nil, fmt.Errorf("no hosts found matching %q", findQuery) + return ui.RunOptions{}, nil, fmt.Errorf("no hosts found matching %q", query) } return options, filtered, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 82 - 98, The helper prepareHostSelection currently uses the package-global findQuery; instead, call cmd.Flags().GetString("find") inside prepareHostSelection to obtain the actual flag value, handle the returned (string, error) and propagate any error, replace all uses of findQuery with that local value when computing options.StartFiltering and when filtering hosts via filterHosts, and keep the same return behavior (return ui.RunOptions{}, nil, fmt.Errorf(...) when filtered is empty). Update references in prepareHostSelection only (no global reliance).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.sh`:
- Around line 56-73: The install script currently downloads and untars
"${TMP_DIR}/${ARCHIVE}" and installs "${TMP_DIR}/${BIN_NAME}" without verifying
integrity; add a checksum verification step that fetches the published checksum
for "${BIN_NAME}"/"${LATEST}" (e.g. a .sha256 or .sha256sum file for "$URL" or
from a canonical release URL), compute the archive's checksum (use the same
algorithm, e.g. SHA256) and compare it to the fetched value, delete the archive
and exit non‑zero on mismatch, and only proceed to tar -xzf and install when the
checksum matches; use the existing variables (URL, TMP_DIR, ARCHIVE, BIN_NAME,
INSTALL_DIR, LATEST) so the verification is performed immediately after
downloading and before extraction or calling install.
In `@internal/connect/target.go`:
- Around line 50-58: The fallback that treats AddrError "missing port in
address" as host-only should first reject targets with a trailing colon; update
the parsing logic around the net.AddrError check (the block inspecting
err.(*net.AddrError) and rawHost) to detect if rawHost ends with ":" (or the
original input ends with ":") and return a format error instead of trimming and
returning a host, and preserve the existing error path for other AddrError
cases; then add regression tests asserting that inputs "user@host:" and
"user@[::1]:" (or equivalent forms) produce an error from the parse function
rather than returning a malformed host.
In `@internal/sshconfig/writer.go`:
- Around line 10-19: The AddHost function currently sets host.Alias via
resolveAlias and later passes Alias, Hostname, User, and IdentityFile verbatim
to formatHostEntry; add validation in AddHost to reject or sanitize unsafe
characters (spaces, tabs, newlines, carriage returns, and any other characters
that could break ssh config like unescaped quotes or leading/trailing
whitespace) for host.Alias, host.Hostname, host.User, and
host.IdentityFile—return a descriptive error (e.g., "invalid Hostname: contains
whitespace/newline") when any field contains those characters, or alternatively
perform proper escaping/quoting before calling formatHostEntry so
formatHostEntry always receives safe, validated values. Ensure checks reference
AddHost, resolveAlias, formatHostEntry, and the fields Hostname, User, Alias,
IdentityFile.
In `@README.md`:
- Around line 23-25: Replace the insecure one-liner in README.md that pipes the
mutable main-branch script into sh; update the Quick Install command to
reference a versioned/tagged release script (e.g., the
releases/download/<tag>/install.sh URL) or change the flow to download-then-run
by curling to a file (curl -fsSL ... -o install.sh) and then executing it (sh
install.sh) so users can inspect the installer before execution; update the
example curl line accordingly.
---
Nitpick comments:
In `@cmd/root.go`:
- Around line 82-98: The helper prepareHostSelection currently uses the
package-global findQuery; instead, call cmd.Flags().GetString("find") inside
prepareHostSelection to obtain the actual flag value, handle the returned
(string, error) and propagate any error, replace all uses of findQuery with that
local value when computing options.StartFiltering and when filtering hosts via
filterHosts, and keep the same return behavior (return ui.RunOptions{}, nil,
fmt.Errorf(...) when filtered is empty). Update references in
prepareHostSelection only (no global reliance).
🪄 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 Plus
Run ID: e14f40f8-e0df-4179-82e6-e7eda5029086
📒 Files selected for processing (7)
README.mdcmd/root.gocmd/root_test.goinstall.shinternal/connect/target.gointernal/sshconfig/writer.gointernal/ui/model.go
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
README.md (1)
23-25:⚠️ Potential issue | 🟠 MajorAvoid executing a mutable
mainscript via pipe-to-shell.Line 24 still executes whatever is currently on
mainwith no inspection step. Please switch to a versioned/tagged installer URL or a download-then-run flow.Safer documentation example
- curl -fsSL https://raw.githubusercontent.com/JerryAgbesi/skipper/main/install.sh | sh + curl -fsSL "https://raw.githubusercontent.com/JerryAgbesi/skipper/<tag>/install.sh" -o install.sh + sh install.sh🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 23 - 25, Replace the unsafe pipe-to-shell invocation "curl -fsSL https://raw.githubusercontent.com/JerryAgbesi/skipper/main/install.sh | sh" with a safer flow: either point to a versioned/tagged installer URL (e.g. a releases/download/vX/install.sh URL) or change the example to download-then-run (curl -fsSL -o install.sh <URL>), show verifying the script (checksum/signature) and then running it (sh ./install.sh). Ensure the README example explicitly avoids piping remote content into sh and documents the expected version tag placeholder so readers use an immutable release.
🧹 Nitpick comments (1)
internal/sshconfig/writer_test.go (1)
92-113: Add a regression test for alias collision with differentIdentityFile.Current tests validate write/rejection paths well, but there’s no case asserting that same alias + same host/user/port + different
IdentityFileis rejected (or explicitly handled). That gap would have caught the conflict behavior inAddHostat Line 35.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sshconfig/writer_test.go` around lines 92 - 113, Add a regression test that verifies AddHost rejects a second host with the same Alias/Hostname/User/Port but a different IdentityFile: create a temp config, call AddHost with Host{Alias:"jump-box", Hostname:"example.com", User:"alice", IdentityFile:"/path/one"}, assert success, then call AddHost again with Host{Alias:"jump-box", Hostname:"example.com", User:"alice", IdentityFile:"/path/two"} and assert it returns an error; place the test alongside existing tests (e.g., similar to TestAddHostWritesSafeIdentityFile) and reference the AddHost and Host types so the collision behavior at the AddHost logic is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/sshconfig/writer.go`:
- Line 55: The deferred file.Close() calls are dropping close errors (lines with
file.Close() and the similar block at 69-73); change each defer file.Close()
into a deferred closure that captures and checks the Close() error and merges it
into the function's returned error (or logs it) so write failures surfaced on
close aren't silently lost; to implement this, make the function use a named
error return (e.g., err error) if needed and in the deferred func call cerr :=
file.Close(); if cerr != nil { if err == nil { err = cerr } else { err =
fmt.Errorf("%v; close error: %w", err, cerr) } } — apply the same pattern for
all Close() uses in this file (e.g., the writer routine that opens the SSH
config file).
- Around line 35-39: The duplicate-detection logic should include IdentityFile
in equivalence checks so AddHost (the block comparing existingHost to host) does
not silently accept a differing key; update the conditional that currently
checks existingHost.Hostname, existingHost.User and existingHost.Port to also
require existingHost.IdentityFile == host.IdentityFile before returning
&existingHost, nil, and keep returning the conflict error (using host.Alias and
path) when IdentityFile differs.
In `@README.md`:
- Around line 114-122: Remove the accidental git patch/artifact and embedded
tests README content from README.md by deleting the "*** Add File:
.../tests/README.md" line and the following "This folder is the central place
for repository-level and integration-style tests." block (the inserted
tests/README.md content). After removal, restore the original README.md markdown
flow and, if the tests README content is intended to exist, commit it as a
separate tests/README.md file in the repository (not embedded in README.md);
ensure no local absolute paths remain.
---
Duplicate comments:
In `@README.md`:
- Around line 23-25: Replace the unsafe pipe-to-shell invocation "curl -fsSL
https://raw.githubusercontent.com/JerryAgbesi/skipper/main/install.sh | sh" with
a safer flow: either point to a versioned/tagged installer URL (e.g. a
releases/download/vX/install.sh URL) or change the example to download-then-run
(curl -fsSL -o install.sh <URL>), show verifying the script (checksum/signature)
and then running it (sh ./install.sh). Ensure the README example explicitly
avoids piping remote content into sh and documents the expected version tag
placeholder so readers use an immutable release.
---
Nitpick comments:
In `@internal/sshconfig/writer_test.go`:
- Around line 92-113: Add a regression test that verifies AddHost rejects a
second host with the same Alias/Hostname/User/Port but a different IdentityFile:
create a temp config, call AddHost with Host{Alias:"jump-box",
Hostname:"example.com", User:"alice", IdentityFile:"/path/one"}, assert success,
then call AddHost again with Host{Alias:"jump-box", Hostname:"example.com",
User:"alice", IdentityFile:"/path/two"} and assert it returns an error; place
the test alongside existing tests (e.g., similar to
TestAddHostWritesSafeIdentityFile) and reference the AddHost and Host types so
the collision behavior at the AddHost logic is exercised.
🪄 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 Plus
Run ID: 67994dd8-b851-4d05-8d81-1675a746d19e
📒 Files selected for processing (6)
MakefileREADME.mdinternal/connect/target.gointernal/connect/target_test.gointernal/sshconfig/writer.gointernal/sshconfig/writer_test.go
✅ Files skipped from review due to trivial changes (2)
- Makefile
- internal/connect/target_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/connect/target.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/sshconfig/writer.go (2)
12-18: Consolidate required-field validation into one path.
Hostname/Userrequired checks are duplicated inAddHostandvalidateHostFields. Keeping this in one place reduces drift in behavior and error messages.Also applies to: 147-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sshconfig/writer.go` around lines 12 - 18, Consolidate the required-field validation for Hostname and User by moving the checks into the single validator function validateHostFields (ensure it checks strings.TrimSpace(host.Hostname) and strings.TrimSpace(host.User) and returns consistent error messages) and remove the duplicate checks from AddHost (and the other duplicate block around the 147-150 area) so AddHost simply calls validateHostFields and handles its returned error; keep the exact error text consistent across uses to avoid behavioral drift.
99-101: Include all instances of this refactoring pattern in the file.The
WriteString(fmt.Sprintf(...))pattern appears not only at lines 99-101, but also at lines 103 and 106. Apply the refactor to all five occurrences for consistency and to align with staticcheck QF1012:♻️ Suggested refactor
func formatHostEntry(host Host) string { var builder strings.Builder - builder.WriteString(fmt.Sprintf("Host %s\n", host.Alias)) - builder.WriteString(fmt.Sprintf(" HostName %s\n", host.Hostname)) - builder.WriteString(fmt.Sprintf(" User %s\n", host.User)) + fmt.Fprintf(&builder, "Host %s\n", host.Alias) + fmt.Fprintf(&builder, " HostName %s\n", host.Hostname) + fmt.Fprintf(&builder, " User %s\n", host.User) if host.Port > 0 { - builder.WriteString(fmt.Sprintf(" Port %d\n", host.Port)) + fmt.Fprintf(&builder, " Port %d\n", host.Port) } if host.IdentityFile != "" { - builder.WriteString(fmt.Sprintf(" IdentityFile %s\n", host.IdentityFile)) + fmt.Fprintf(&builder, " IdentityFile %s\n", host.IdentityFile) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sshconfig/writer.go` around lines 99 - 101, Replace all instances of builder.WriteString(fmt.Sprintf(...)) with direct formatted writes using fmt.Fprintf to avoid the unnecessary fmt.Sprintf allocation: e.g., change builder.WriteString(fmt.Sprintf("Host %s\n", host.Alias)) and the other four occurrences (the lines that use host.Hostname, host.User, and the two remaining fmt.Sprintf uses in this file) to fmt.Fprintf(builder, "Host %s\n", host.Alias) / fmt.Fprintf(builder, " HostName %s\n", host.Hostname) / fmt.Fprintf(builder, " User %s\n", host.User) etc.; ensure fmt is imported and used, and update all five occurrences in internal/sshconfig/writer.go for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/sshconfig/writer.go`:
- Around line 25-40: The dedupe + append sequence around readExistingHosts(path)
is not atomic and can race under concurrent `--add` runs; wrap the
read/check/write sequence with an exclusive file lock (or perform an atomic
read-modify-write via a temp file + rename) so that the check in the loop that
compares existingHost.Alias and sameHostSettings(...) and the subsequent append
are serialized. Concretely, obtain an exclusive lock on the SSH config file
before calling readExistingHosts(path), perform the alias comparison and
decision (return existingHost or error) and then do the append/write while still
holding the lock, and only release the lock after the file has been atomically
updated; apply this change to the code paths that call readExistingHosts, the
loop checking existingHost.Alias, and the function that performs the
append/write.
---
Nitpick comments:
In `@internal/sshconfig/writer.go`:
- Around line 12-18: Consolidate the required-field validation for Hostname and
User by moving the checks into the single validator function validateHostFields
(ensure it checks strings.TrimSpace(host.Hostname) and
strings.TrimSpace(host.User) and returns consistent error messages) and remove
the duplicate checks from AddHost (and the other duplicate block around the
147-150 area) so AddHost simply calls validateHostFields and handles its
returned error; keep the exact error text consistent across uses to avoid
behavioral drift.
- Around line 99-101: Replace all instances of
builder.WriteString(fmt.Sprintf(...)) with direct formatted writes using
fmt.Fprintf to avoid the unnecessary fmt.Sprintf allocation: e.g., change
builder.WriteString(fmt.Sprintf("Host %s\n", host.Alias)) and the other four
occurrences (the lines that use host.Hostname, host.User, and the two remaining
fmt.Sprintf uses in this file) to fmt.Fprintf(builder, "Host %s\n", host.Alias)
/ fmt.Fprintf(builder, " HostName %s\n", host.Hostname) / fmt.Fprintf(builder,
" User %s\n", host.User) etc.; ensure fmt is imported and used, and update all
five occurrences in internal/sshconfig/writer.go for consistency.
🪄 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 Plus
Run ID: ea795729-6495-4713-bd11-7f3fc1924888
📒 Files selected for processing (4)
README.mdinternal/sshconfig/parser.gointernal/sshconfig/writer.gointernal/sshconfig/writer_test.go
✅ Files skipped from review due to trivial changes (2)
- internal/sshconfig/parser.go
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/sshconfig/writer_test.go
This was referenced Apr 18, 2026
Merged
4 tasks
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
2 participants
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.
This PR includes :
Summary by CodeRabbit
New Features
Documentation
Tests
testtarget to run it.