feat(machine): GitHub key registration via gh CLI#105
Merged
nvandessel merged 6 commits intomainfrom Feb 28, 2026
Merged
Conversation
📝 Walkthrough🚥 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)
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 |
Greptile SummaryThis PR adds GitHub SSH/GPG key registration via the Key improvements from previous review rounds:
Remaining concerns:
Architecture notes:
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| cmd/g4d/machine.go | Adds keys register command with gh CLI integration. Properly validates titles, handles hostname fallback, checks for existing keys before registration. Previous issues with error handling and continue statements have been fixed. |
| internal/machine/github.go | Implements GitHubClient with Commander pattern for testability. Most methods use Commander interface correctly. AddGPGKey still bypasses Commander (uses exec.Command directly) making it untestable. Refactored to check for empty GPG output, addressing previous deadlock concerns. |
| internal/validation/validation.go | Added security validators for GPG key IDs (8-40 hex chars), SSH key paths, and key titles. Comprehensive checks prevent flag injection, path traversal, and shell metacharacters. All validators follow consistent pattern with clear error messages. |
Sequence Diagram
sequenceDiagram
participant User
participant CLI as g4d machine keys register
participant GHClient as GitHubClient
participant Commander as Commander Interface
participant GH as gh CLI
participant Validation as Validators
User->>CLI: run g4d machine keys register
CLI->>GHClient: HasGHCLI()
CLI->>GHClient: IsAuthenticated()
GHClient->>Commander: Run(gh, auth, status)
Commander->>GH: execute gh auth status
GH-->>Commander: auth status
Commander-->>GHClient: result
GHClient-->>CLI: authenticated
CLI->>CLI: DetectAllSSHKeys()
CLI->>CLI: DetectGPGKeys()
loop For each SSH key
CLI->>GHClient: IsKeyRegistered(path)
GHClient->>Commander: Run(gh, api, user/keys)
Commander->>GH: GET /user/keys
GH-->>Commander: JSON keys list
Commander-->>GHClient: JSON response
GHClient->>GHClient: compare key material
GHClient-->>CLI: registered=false
CLI->>Validation: ValidateKeyTitle(title)
Validation-->>CLI: valid
CLI->>GHClient: AddSSHKey(path, title)
GHClient->>Validation: ValidateSSHKeyPath()
GHClient->>Validation: ValidateKeyTitle()
GHClient->>Commander: Run(gh, ssh-key, add, path, --title, title)
Commander->>GH: execute gh ssh-key add
GH-->>Commander: success
Commander-->>GHClient: result
GHClient-->>CLI: key registered
end
loop For each GPG key
CLI->>GHClient: IsGPGKeyRegistered(keyID)
GHClient->>Validation: ValidateGPGKeyID()
GHClient->>Commander: Run(gh, api, /user/gpg_keys)
Commander->>GH: GET /user/gpg_keys
GH-->>Commander: JSON keys list
Commander-->>GHClient: JSON response
GHClient->>GHClient: exact match keyID
GHClient-->>CLI: registered=false
CLI->>GHClient: AddGPGKey(keyID)
Note over GHClient: Bypasses Commander interface
GHClient->>Validation: ValidateGPGKeyID()
GHClient->>GHClient: exec.Command(gpg, --export, keyID)
GHClient->>GHClient: check output not empty
GHClient->>GHClient: exec.Command(gh, gpg-key, add)
GHClient-->>CLI: key registered
end
CLI-->>User: Registered N SSH keys, M GPG keys
Last reviewed commit: a13c4b8
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
448
to
459
| // Register GPG keys | ||
| gpgRegistered := 0 | ||
| gpgKeys, _ := machine.DetectGPGKeys() | ||
| for _, key := range gpgKeys { | ||
| fmt.Printf(" Registering GPG key %s...\n", key.KeyID) | ||
| if err := client.AddGPGKey(key.KeyID); err != nil { | ||
| fmt.Fprintf(os.Stderr, " Error: %v\n", err) | ||
| continue | ||
| } | ||
| fmt.Printf(" Registered GPG key %s\n", key.KeyID) | ||
| gpgRegistered++ | ||
| } |
There was a problem hiding this comment.
No duplicate check before GPG key registration
Unlike SSH keys (which call client.IsKeyRegistered before AddSSHKey), there is no equivalent check for GPG keys. Running g4d machine keys register a second time will attempt to re-register all local GPG keys that are already on GitHub, resulting in a gh gpg-key add error for each one. The GitHubClient would need a ListGPGKeys / IsGPGKeyRegistered method similar to the SSH counterpart, but at minimum the error from a duplicate add should be caught and treated as "already registered" rather than a fatal failure.
cmd/g4d/machine.go
Outdated
| os.Exit(1) | ||
| } | ||
| sshDir := filepath.Join(home, ".ssh") | ||
| hostname, _ := os.Hostname() |
There was a problem hiding this comment.
Silent hostname error causes a misleading validation failure
When os.Hostname() returns an error, hostname is an empty string. The generated title then becomes "-keyName" (starts with a hyphen), which is rejected by ValidateKeyTitle with the error "key title must not start with a hyphen". The user sees a cryptic validation failure with no indication that hostname resolution failed.
Suggested change
| hostname, _ := os.Hostname() | |
| hostname, err := os.Hostname() | |
| if err != nil { | |
| fmt.Fprintf(os.Stderr, "Warning: could not get hostname: %v; using \"unknown\"\n", err) | |
| hostname = "unknown" | |
| } |
Comment on lines
59
to
68
| func (c *GitHubClient) IsAuthenticated() (bool, error) { | ||
| cmd := exec.Command("gh", "auth", "status") | ||
| cmd.Stdout = nil | ||
| cmd.Stderr = nil // Discard stderr - may contain token info | ||
| err := cmd.Run() | ||
| if err != nil { | ||
| return false, nil | ||
| } | ||
| return true, nil | ||
| } |
There was a problem hiding this comment.
IsAuthenticated bypasses the Commander interface
All other GitHubClient methods route through c.getCommander() for testability, but IsAuthenticated calls exec.Command directly. This makes the authentication check untestable with mocks — the corresponding test (TestGitHubClient_IsAuthenticated) is forced to be a smoke test rather than a real unit test.
Consider routing through the Commander interface (e.g., using c.getCommander().Run("gh", "auth", "status")), or at minimum documenting that this method intentionally bypasses the interface.
Comment on lines
90
to
125
| func (c *GitHubClient) AddGPGKey(keyID string) error { | ||
| if err := validation.ValidateGPGKeyID(keyID); err != nil { | ||
| return fmt.Errorf("invalid GPG key ID: %w", err) | ||
| } | ||
|
|
||
| // Use exec.Command directly for pipe orchestration | ||
| gpgCmd := exec.Command("gpg", "--armor", "--export", keyID) | ||
| ghCmd := exec.Command("gh", "gpg-key", "add") | ||
|
|
||
| gpgOut, err := gpgCmd.StdoutPipe() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create pipe: %w", err) | ||
| } | ||
| ghCmd.Stdin = gpgOut | ||
| gpgCmd.Stderr = os.Stderr | ||
| ghCmd.Stderr = os.Stderr | ||
|
|
||
| if err := gpgCmd.Start(); err != nil { | ||
| return fmt.Errorf("failed to start gpg: %w", err) | ||
| } | ||
| if err := ghCmd.Start(); err != nil { | ||
| _ = gpgCmd.Wait() | ||
| return fmt.Errorf("failed to start gh: %w", err) | ||
| } | ||
|
|
||
| gpgErr := gpgCmd.Wait() | ||
| ghErr := ghCmd.Wait() | ||
|
|
||
| if gpgErr != nil { | ||
| return fmt.Errorf("gpg export failed: %w", gpgErr) | ||
| } | ||
| if ghErr != nil { | ||
| return fmt.Errorf("gh gpg-key add failed: %w", ghErr) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
AddGPGKey bypasses the Commander interface and is untestable
AddGPGKey builds the GPG→gh pipe entirely with exec.Command directly, so the Commander interface cannot intercept it. As a result, there is no success-path unit test for this method — only validation-rejection tests exist. If pipe-based execution is a hard requirement, consider documenting this explicitly and adding an integration-test tag, or wrapping the pipe logic in a helper that accepts injectable commands.
cmd/g4d/machine.go
Outdated
|
|
||
| // Register SSH keys | ||
| sshRegistered := 0 | ||
| keys, _ := machine.DetectAllSSHKeys(sshDir) |
There was a problem hiding this comment.
Errors from DetectAllSSHKeys and DetectGPGKeys are silently discarded
Both detection calls swallow their errors with _. This also applies to gpgKeys, _ := machine.DetectGPGKeys() on line 450. If either function encounters a permission error, a corrupted directory, or another I/O problem, the command silently reports "Registered 0 SSH keys, 0 GPG keys" with no diagnostic output.
Suggested change
| keys, _ := machine.DetectAllSSHKeys(sshDir) | |
| keys, err := machine.DetectAllSSHKeys(sshDir) | |
| if err != nil { | |
| fmt.Fprintf(os.Stderr, "Warning: could not detect SSH keys: %v\n", err) | |
| } |
Owner
Author
🌴 Frond StackManaged by frond |
Add GitHubClient with Commander interface for testability: - HasGHCLI() and IsAuthenticated() for prerequisite checks - AddSSHKey() registers SSH public keys with GitHub - AddGPGKey() uses gpg→gh pipe for GPG key registration - ListSSHKeys() parses GitHub's JSON response - IsKeyRegistered() compares local key material against GitHub - ValidateGPGKeyID() and ValidateKeyTitle() security validators - New CLI: g4d machine keys register Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
nvandessel
force-pushed
the
feat/gh-key-registration
branch
from
a5cc141 to
8914fcb
Compare
Owner
Author
|
@greptileai review |
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (52.38%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #105 +/- ##
==========================================
+ Coverage 48.68% 48.73% +0.05%
==========================================
Files 108 109 +1
Lines 12059 12227 +168
==========================================
+ Hits 5871 5959 +88
- Misses 6188 6268 +80
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
internal/machine/github_test.go (1)
236-244: Smoke test depends on system state.
TestGitHubClient_IsAuthenticatedusesNewGitHubClient()which invokes the realghCLI. This test will behave differently depending on whetherghis installed and authenticated in the CI environment. Consider using the mock or explicitly skipping whenghis unavailable.💡 Option to skip when gh is unavailable
func TestGitHubClient_IsAuthenticated(t *testing.T) { - // Smoke test - just verify no panic + if !HasGHCLI() { + t.Skip("gh CLI not available") + } client := NewGitHubClient()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/machine/github_test.go` around lines 236 - 244, TestGitHubClient_IsAuthenticated currently calls NewGitHubClient() which invokes the real gh CLI and makes the test environment-dependent; update the test to avoid hitting the real CLI by either (a) injecting a mock GitHub client and asserting behavior of IsAuthenticated on that mock, or (b) checking for gh availability/authentication and calling t.Skip(...) if gh is not installed or not authenticated. Locate the test TestGitHubClient_IsAuthenticated and NewGitHubClient/IsAuthenticated usages to implement dependency injection of a mock client (preferred) or add a pre-check that runs a lightweight gh command (or looks for gh in PATH) and skips the test when unavailable.internal/machine/github.go (2)
89-125:AddGPGKeybypasses Commander and pipes stderr to os.Stderr.This method uses
exec.Commanddirectly for pipe orchestration, which is necessary for the pipe but makes it untestable via mock. Additionally, piping stderr toos.Stderr(lines 104-105) may leak sensitive information in logs.Consider:
- Capturing stderr for error messages instead of piping to os.Stderr
- Documenting that this method is not mockable, or extracting pipe logic
♻️ Capture stderr for better error handling
+ var gpgStderr, ghStderr bytes.Buffer gpgOut, err := gpgCmd.StdoutPipe() if err != nil { return fmt.Errorf("failed to create pipe: %w", err) } ghCmd.Stdin = gpgOut - gpgCmd.Stderr = os.Stderr - ghCmd.Stderr = os.Stderr + gpgCmd.Stderr = &gpgStderr + ghCmd.Stderr = &ghStderr // ... (rest of the method) if gpgErr != nil { - return fmt.Errorf("gpg export failed: %w", gpgErr) + return fmt.Errorf("gpg export failed: %w\nStderr: %s", gpgErr, gpgStderr.String()) } if ghErr != nil { - return fmt.Errorf("gh gpg-key add failed: %w", ghErr) + return fmt.Errorf("gh gpg-key add failed: %w\nStderr: %s", ghErr, ghStderr.String()) }Note: Add
"bytes"to imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/machine/github.go` around lines 89 - 125, The AddGPGKey function currently uses exec.Command directly and attaches gpgCmd.Stderr and ghCmd.Stderr to os.Stderr, which leaks output and makes testing harder; change it to capture stderr into buffers (e.g., bytes.Buffer) for both gpgCmd and ghCmd, set those buffers to gpgCmd.Stderr and ghCmd.Stderr, and include the captured stderr text in the returned errors when gpgCmd.Wait() or ghCmd.Wait() fail; additionally add a comment on AddGPGKey documenting that it performs a real-process pipe and is not mockable (or consider extracting the pipe orchestration into a separable function that can be injected/mocked) and update imports to include "bytes".
57-68:IsAuthenticatedbypasses Commander, reducing testability.This method uses
exec.Commanddirectly instead of the injectedCommander, making it untestable with the mock. Consider refactoring to usegetCommander().Run()for consistency with other methods.♻️ Refactored to use Commander
// IsAuthenticated checks if gh is authenticated with GitHub. -// Stderr is discarded to prevent token leakage. func (c *GitHubClient) IsAuthenticated() (bool, error) { - cmd := exec.Command("gh", "auth", "status") - cmd.Stdout = nil - cmd.Stderr = nil // Discard stderr - may contain token info - err := cmd.Run() + _, err := c.getCommander().Run("gh", "auth", "status") if err != nil { return false, nil } return true, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/machine/github.go` around lines 57 - 68, The IsAuthenticated method in GitHubClient is calling exec.Command directly which bypasses the injected Commander and prevents mocking; refactor IsAuthenticated to use getCommander() (or the existing Commander instance) and call its Run/Output method instead of exec.Command so stderr/stdout handling remains the same, ensure the command used remains ["gh","auth","status"] and that errors map to the same boolean result and returned error signature so tests can inject a mock Commander and assert behavior.cmd/g4d/machine.go (1)
431-431: Consider logging or handling hostname retrieval failure.The error from
os.Hostname()is silently ignored. If hostname retrieval fails, the generated key title will have an empty prefix (e.g.,-id_ed25519), which will failValidateKeyTitledue to leading hyphen. Consider either logging the warning or providing a fallback.💡 Suggested improvement
- hostname, _ := os.Hostname() + hostname, err := os.Hostname() + if err != nil || hostname == "" { + hostname = "unknown" + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/g4d/machine.go` at line 431, The call to os.Hostname() ignores its error, which can leave hostname empty and produce a key title that fails ValidateKeyTitle; update the code around the hostname variable (the os.Hostname() call) to check the returned error, log or warn via the existing logger, and set a safe fallback (e.g., "unknown-host" or "localhost") if hostname is empty or retrieval failed so the generated key title cannot start with a hyphen and will pass ValidateKeyTitle.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/g4d/machine.go`:
- Line 431: The call to os.Hostname() ignores its error, which can leave
hostname empty and produce a key title that fails ValidateKeyTitle; update the
code around the hostname variable (the os.Hostname() call) to check the returned
error, log or warn via the existing logger, and set a safe fallback (e.g.,
"unknown-host" or "localhost") if hostname is empty or retrieval failed so the
generated key title cannot start with a hyphen and will pass ValidateKeyTitle.
In `@internal/machine/github_test.go`:
- Around line 236-244: TestGitHubClient_IsAuthenticated currently calls
NewGitHubClient() which invokes the real gh CLI and makes the test
environment-dependent; update the test to avoid hitting the real CLI by either
(a) injecting a mock GitHub client and asserting behavior of IsAuthenticated on
that mock, or (b) checking for gh availability/authentication and calling
t.Skip(...) if gh is not installed or not authenticated. Locate the test
TestGitHubClient_IsAuthenticated and NewGitHubClient/IsAuthenticated usages to
implement dependency injection of a mock client (preferred) or add a pre-check
that runs a lightweight gh command (or looks for gh in PATH) and skips the test
when unavailable.
In `@internal/machine/github.go`:
- Around line 89-125: The AddGPGKey function currently uses exec.Command
directly and attaches gpgCmd.Stderr and ghCmd.Stderr to os.Stderr, which leaks
output and makes testing harder; change it to capture stderr into buffers (e.g.,
bytes.Buffer) for both gpgCmd and ghCmd, set those buffers to gpgCmd.Stderr and
ghCmd.Stderr, and include the captured stderr text in the returned errors when
gpgCmd.Wait() or ghCmd.Wait() fail; additionally add a comment on AddGPGKey
documenting that it performs a real-process pipe and is not mockable (or
consider extracting the pipe orchestration into a separable function that can be
injected/mocked) and update imports to include "bytes".
- Around line 57-68: The IsAuthenticated method in GitHubClient is calling
exec.Command directly which bypasses the injected Commander and prevents
mocking; refactor IsAuthenticated to use getCommander() (or the existing
Commander instance) and call its Run/Output method instead of exec.Command so
stderr/stdout handling remains the same, ensure the command used remains
["gh","auth","status"] and that errors map to the same boolean result and
returned error signature so tests can inject a mock Commander and assert
behavior.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/g4d/machine.gointernal/machine/github.gointernal/machine/github_test.gointernal/validation/validation.gointernal/validation/validation_test.go
- Handle os.Hostname() error with fallback to "unknown" - Surface DetectAllSSHKeys() and DetectGPGKeys() errors as warnings - Validate generated key title with fallback to key filename - Add cobra.NoArgs to register command Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
internal/machine/github.go
Outdated
Comment on lines
110
to
113
| if err := ghCmd.Start(); err != nil { | ||
| _ = gpgCmd.Wait() | ||
| return fmt.Errorf("failed to start gh: %w", err) | ||
| } |
There was a problem hiding this comment.
Potential deadlock when ghCmd.Start() fails
If ghCmd.Start() returns an error, gpgCmd is already running and writing to gpgOut. Since nothing is reading from gpgOut at this point (ghCmd never started), the pipe buffer will fill up and gpg will block indefinitely. Calling gpgCmd.Wait() on a blocked process results in a hang.
The fix is to close gpgOut first so gpg gets a broken-pipe signal and can exit:
Suggested change
| if err := ghCmd.Start(); err != nil { | |
| _ = gpgCmd.Wait() | |
| return fmt.Errorf("failed to start gh: %w", err) | |
| } | |
| if err := ghCmd.Start(); err != nil { | |
| gpgOut.Close() // unblock gpg so it can exit on broken pipe | |
| _ = gpgCmd.Wait() | |
| return fmt.Errorf("failed to start gh: %w", err) | |
| } |
internal/machine/github.go
Outdated
Comment on lines
96
to
97
| gpgCmd := exec.Command("gpg", "--armor", "--export", keyID) | ||
| ghCmd := exec.Command("gh", "gpg-key", "add") |
There was a problem hiding this comment.
gpg --export silently succeeds with empty output for unknown key IDs
gpg --armor --export <keyID> exits with code 0 even when the key ID does not exist in the local keyring — it simply produces no output. When the empty stream is piped to gh gpg-key add, GitHub's API will reject the request. The resulting error (gh gpg-key add failed: exit status 1) gives no indication that the local key was simply not found.
Consider capturing the GPG output first (rather than streaming it) and checking it is non-empty before invoking gh:
// Capture gpg output first to detect missing-key case
gpgOut, err := exec.Command("gpg", "--armor", "--export", keyID).Output()
if err != nil {
return fmt.Errorf("gpg export failed: %w", err)
}
if len(strings.TrimSpace(string(gpgOut))) == 0 {
return fmt.Errorf("gpg key %q not found in local keyring", keyID)
}
ghCmd := exec.Command("gh", "gpg-key", "add")
ghCmd.Stdin = strings.NewReader(string(gpgOut))
ghCmd.Stderr = os.Stderr
if out, err := ghCmd.Output(); err != nil {
return fmt.Errorf("gh gpg-key add failed: %w\nOutput: %s", err, out)
}
return nilThis removes the pipe entirely, avoids the Commander bypass, and produces a clear "key not found" error.
- Route IsAuthenticated through Commander interface for testability - Capture stderr in AddGPGKey instead of piping to os.Stderr - Add GPG duplicate check: ListGPGKeys + IsGPGKeyRegistered methods - Add GPG duplicate check in register command flow - Handle empty hostname with fallback to "unknown" - Skip smoke test when gh CLI unavailable - Add proper mock tests for IsAuthenticated (success + failure) - Add tests for ListGPGKeys, IsGPGKeyRegistered (match, case-insensitive, short ID, no match, invalid) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@greptileai review |
Replace pipe-based gpg→gh orchestration with sequential approach: - Export GPG key first, check for non-empty output (detects missing keys) - Feed captured output to gh via stdin - Eliminates potential deadlock if gh fails to start - Produces clear "key not found" error instead of cryptic gh API failure - Capture stderr in buffer instead of piping to os.Stderr Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@greptileai review |
Comment on lines
482
to
486
| registered, err := client.IsGPGKeyRegistered(key.KeyID) | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "Warning: could not check if GPG key %s is registered: %v\n", key.KeyID, err) | ||
| } | ||
| if registered { |
There was a problem hiding this comment.
Missing continue causes unconditional GPG registration on check error
When IsGPGKeyRegistered returns an error (e.g., a network failure listing GitHub keys), there is no continue statement, so execution falls through to the if registered check. Since registered is false on error (the zero value), the code always proceeds to call AddGPGKey regardless of the error. This is inconsistent with the SSH key loop just above it (line 449–453), which explicitly calls continue on error.
In practice, if the GitHub API call fails due to a transient network issue and the key is already registered, AddGPGKey will receive a "key already exists" error from GitHub and surface a confusing error to the user.
Suggested change
| registered, err := client.IsGPGKeyRegistered(key.KeyID) | |
| if err != nil { | |
| fmt.Fprintf(os.Stderr, "Warning: could not check if GPG key %s is registered: %v\n", key.KeyID, err) | |
| } | |
| if registered { | |
| registered, err := client.IsGPGKeyRegistered(key.KeyID) | |
| if err != nil { | |
| fmt.Fprintf(os.Stderr, "Warning: could not check if GPG key %s is registered: %v\n", key.KeyID, err) | |
| continue | |
| } | |
| if registered { |
internal/machine/github.go
Outdated
Comment on lines
151
to
157
| // Also check if the local key ID is a suffix (short vs long form) | ||
| if len(normalizedKeyID) < len(ghKey.KeyID) && strings.HasSuffix(strings.ToUpper(ghKey.KeyID), normalizedKeyID) { | ||
| return true, nil | ||
| } | ||
| if len(ghKey.KeyID) < len(normalizedKeyID) && strings.HasSuffix(normalizedKeyID, strings.ToUpper(ghKey.KeyID)) { | ||
| return true, nil | ||
| } |
There was a problem hiding this comment.
Suffix-based GPG key ID matching risks false positives
The suffix comparison on lines 152 and 155 is designed to reconcile short-form (e.g., 8-char) local key IDs with full-length key IDs returned by GitHub. However, GPG 32-bit short key IDs are famously collision-prone (the "Evil32" attack), and a suffix match makes this worse: any GitHub key whose full ID happens to share the same trailing 8 hex characters as the local short ID will be treated as a match, returning true even though the keys are different.
The practical result is that IsGPGKeyRegistered could incorrectly return true for a different key already on GitHub, causing the local key to be silently skipped without ever being registered.
Consider requiring full-length fingerprint comparison (40 hex chars) and rejecting short-form IDs outright, or at minimum only using the exact-match path:
// Only exact-match comparison; callers should pass full fingerprints.
if strings.ToUpper(ghKey.KeyID) == normalizedKeyID {
return true, nil
}
Comment on lines
459
to
464
| // Generate title | ||
| keyName := filepath.Base(key.Path) | ||
| title := fmt.Sprintf("%s-%s", hostname, keyName) | ||
| if err := validation.ValidateKeyTitle(title); err != nil { | ||
| title = keyName // fallback to just key filename | ||
| } |
There was a problem hiding this comment.
Fallback keyName title is never validated
When ValidateKeyTitle(title) fails for the hostname-keyName combination, title is silently replaced with keyName. But keyName itself is also never validated, so it's possible (however unlikely with well-named SSH key files) that keyName would also fail ValidateKeyTitle. In that case, AddSSHKey will return an error on a title that was used without any verification, and the printed message "Registering %s as %q..." will have already been emitted before the failure.
Consider also validating the fallback:
keyName := filepath.Base(key.Path)
title := fmt.Sprintf("%s-%s", hostname, keyName)
if err := validation.ValidateKeyTitle(title); err != nil {
title = keyName
if err := validation.ValidateKeyTitle(title); err != nil {
fmt.Fprintf(os.Stderr, " Warning: skipping %s, could not form a valid title: %v\n", key.Path, err)
continue
}
}- Add continue after GPG key registration check error (prevents unconditional registration on network failure) - Remove suffix-based GPG key ID matching (Evil32 false positive risk), use exact match only - Validate fallback key title and skip registration if invalid Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@greptileai review |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/machine/github.go (1)
88-111:AddGPGKeybypassesCommander, reducing testability and abstraction consistency.This method directly uses
exec.Command, while the rest ofGitHubClientis built around the injectableCommander. That makes GPG add behavior harder to mock and regression-test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/machine/github.go` around lines 88 - 111, The AddGPGKey function currently calls exec.Command directly; change it to use the GitHubClient's injectable Commander (e.g., c.Commander) for both the gpg export and the gh gpg-key add steps so the commands can be mocked in tests. Specifically, replace exec.Command("gpg", "--armor", "--export", keyID).Output() with the Commander equivalent (call Commander.Command("gpg", "--armor", "--export", keyID) and call Output()), and replace exec.Command("gh", "gpg-key", "add") with Commander.Command("gh", "gpg-key", "add"), wiring its Stdin to the exported key and its Stderr to a buffer before Run(); preserve the current validation, empty-output check and error message formatting (including stderr) when returning errors.
🤖 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/machine/github_test.go`:
- Around line 11-18: The mockCommander.Run currently ignores the invoked command
and always returns canned output, so add validation to mockCommander to catch
incorrect CLI invocations: extend the mockCommander struct with expectedName and
expectedArgs (or an expectations callback) and/or a recorded lastName/lastArgs
field, then update the Run(name string, args ...string) method to compare the
incoming name and args with the expected values and return a descriptive error
(or fail the test) when they differ; update tests to set the expectations on
mockCommander before calling the code under test and assert the recorded
invocation if using recording instead of immediate error.
- Around line 26-402: The tests for GitHubClient are repetitive and should be
refactored into table-driven tests; consolidate related test cases (those for
AddSSHKey: TestGitHubClient_AddSSHKey_BadPath, _NotPubFile, _BadTitle, _Success,
_CommandFailure; AddGPGKey: _BadKeyID, _Empty, _TooShort;
ListSSHKeys/ListGPGKeys parse/empty/error cases; IsKeyRegistered and
IsGPGKeyRegistered scenarios) into table structs keyed by name, input setup (tmp
files, mockCommander.output/err, title/keyID, workdir), expected result and
expected error, then iterate the table calling the underlying methods
(AddSSHKey, AddGPGKey, ListSSHKeys, ListGPGKeys, IsKeyRegistered,
IsGPGKeyRegistered) and run assertions; keep helper logic for creating temp
files and mockCommander instances and preserve edge checks for malformed pub
keys and case-sensitivity; ensure existing standalone tests (NewGitHubClient,
getCommander fallback, IsAuthenticated smoke) remain if they test unique
behavior.
In `@internal/machine/github.go`:
- Around line 140-143: The calls that propagate raw errors (e.g., the ghKeys,
err := c.ListGPGKeys() block and the similar blocks around lines noted) should
wrap returned errors with contextual messages using fmt.Errorf to preserve
call-site context; update the returns to something like fmt.Errorf("ListGPGKeys
failed: %w", err) (and similarly wrap errors from the other calls referenced at
173-176 and 186-188), keeping the original error as %w so callers can inspect it
while providing clear operation context in functions/methods that call
c.ListGPGKeys and the other listed operations.
- Around line 121-124: ListGPGKeys and ListSSHKeys currently call the GH CLI
with unsupported "--json" flags causing failures; change them to call the GitHub
REST API via the CLI (e.g. use "gh api --paginate /user/gpg_keys" for GPG keys
and "gh api --paginate /user/keys" for SSH keys) by updating the arguments
passed to getCommander().Run in ListGPGKeys and ListSSHKeys, then parse the
returned JSON into the existing GitHubGPGKey/GitHubSSHKey structs; ensure
IsGPGKeyRegistered and IsKeyRegistered still receive the same slice types and
update any error messages to include the API output on failure.
---
Nitpick comments:
In `@internal/machine/github.go`:
- Around line 88-111: The AddGPGKey function currently calls exec.Command
directly; change it to use the GitHubClient's injectable Commander (e.g.,
c.Commander) for both the gpg export and the gh gpg-key add steps so the
commands can be mocked in tests. Specifically, replace exec.Command("gpg",
"--armor", "--export", keyID).Output() with the Commander equivalent (call
Commander.Command("gpg", "--armor", "--export", keyID) and call Output()), and
replace exec.Command("gh", "gpg-key", "add") with Commander.Command("gh",
"gpg-key", "add"), wiring its Stdin to the exported key and its Stderr to a
buffer before Run(); preserve the current validation, empty-output check and
error message formatting (including stderr) when returning errors.
Comment on lines
11
to
18
| type mockCommander struct { | ||
| output []byte | ||
| err error | ||
| } | ||
|
|
||
| func (m *mockCommander) Run(name string, args ...string) ([]byte, error) { | ||
| return m.output, m.err | ||
| } |
There was a problem hiding this comment.
mockCommander does not validate invoked command/args, so command regressions can slip through.
Run currently returns canned output regardless of name/args, which allows incorrect CLI invocations to pass unit tests undetected.
🛠️ Suggested mock enhancement
type mockCommander struct {
output []byte
err error
+ calls []struct {
+ name string
+ args []string
+ }
}
func (m *mockCommander) Run(name string, args ...string) ([]byte, error) {
+ m.calls = append(m.calls, struct {
+ name string
+ args []string
+ }{name: name, args: append([]string(nil), args...)})
return m.output, m.err
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/machine/github_test.go` around lines 11 - 18, The mockCommander.Run
currently ignores the invoked command and always returns canned output, so add
validation to mockCommander to catch incorrect CLI invocations: extend the
mockCommander struct with expectedName and expectedArgs (or an expectations
callback) and/or a recorded lastName/lastArgs field, then update the Run(name
string, args ...string) method to compare the incoming name and args with the
expected values and return a descriptive error (or fail the test) when they
differ; update tests to set the expectations on mockCommander before calling the
code under test and assert the recorded invocation if using recording instead of
immediate error.
Comment on lines
+26
to
+402
| func TestGitHubClient_AddSSHKey_BadPath(t *testing.T) { | ||
| client := &GitHubClient{Commander: &mockCommander{}} | ||
| err := client.AddSSHKey("/invalid/../path.pub", "title", "/ssh") | ||
| if err == nil { | ||
| t.Error("expected error for bad path") | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_AddSSHKey_NotPubFile(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
| keyPath := filepath.Join(tmpDir, "id_ed25519") | ||
| if err := os.WriteFile(keyPath, []byte("ssh-ed25519 AAAA test@example.com\n"), 0644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| client := &GitHubClient{Commander: &mockCommander{}} | ||
| err := client.AddSSHKey(keyPath, "title", tmpDir) | ||
| if err == nil { | ||
| t.Error("expected error for non-.pub file") | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_AddSSHKey_BadTitle(t *testing.T) { | ||
| // Create a valid .pub file in tmpdir | ||
| tmpDir := t.TempDir() | ||
| pubPath := filepath.Join(tmpDir, "test.pub") | ||
| if err := os.WriteFile(pubPath, []byte("ssh-ed25519 AAAA test@example.com\n"), 0644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| client := &GitHubClient{Commander: &mockCommander{}} | ||
| err := client.AddSSHKey(pubPath, "-evil", tmpDir) | ||
| if err == nil { | ||
| t.Error("expected error for bad title") | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_AddSSHKey_Success(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
| pubPath := filepath.Join(tmpDir, "test.pub") | ||
| if err := os.WriteFile(pubPath, []byte("ssh-ed25519 AAAA test@example.com\n"), 0644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| client := &GitHubClient{Commander: &mockCommander{output: []byte("ok")}} | ||
| err := client.AddSSHKey(pubPath, "my-key", tmpDir) | ||
| if err != nil { | ||
| t.Errorf("unexpected error: %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_AddSSHKey_CommandFailure(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
| pubPath := filepath.Join(tmpDir, "test.pub") | ||
| if err := os.WriteFile(pubPath, []byte("ssh-ed25519 AAAA test@example.com\n"), 0644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| client := &GitHubClient{Commander: &mockCommander{output: []byte("error"), err: fmt.Errorf("gh failed")}} | ||
| err := client.AddSSHKey(pubPath, "my-key", tmpDir) | ||
| if err == nil { | ||
| t.Error("expected error for command failure") | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_AddGPGKey_BadKeyID(t *testing.T) { | ||
| client := &GitHubClient{Commander: &mockCommander{}} | ||
| err := client.AddGPGKey("not-hex!") | ||
| if err == nil { | ||
| t.Error("expected error for non-hex key ID") | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_AddGPGKey_Empty(t *testing.T) { | ||
| client := &GitHubClient{Commander: &mockCommander{}} | ||
| err := client.AddGPGKey("") | ||
| if err == nil { | ||
| t.Error("expected error for empty key ID") | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_AddGPGKey_TooShort(t *testing.T) { | ||
| client := &GitHubClient{Commander: &mockCommander{}} | ||
| err := client.AddGPGKey("ABCDEF0") | ||
| if err == nil { | ||
| t.Error("expected error for too-short key ID") | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_ListSSHKeys_Parse(t *testing.T) { | ||
| jsonData := `[{"id": 123, "key": "ssh-ed25519 AAAA test@example.com", "title": "my-key"}]` | ||
| client := &GitHubClient{Commander: &mockCommander{output: []byte(jsonData)}} | ||
|
|
||
| keys, err := client.ListSSHKeys() | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if len(keys) != 1 { | ||
| t.Fatalf("expected 1 key, got %d", len(keys)) | ||
| } | ||
| if keys[0].Title != "my-key" { | ||
| t.Errorf("title = %q, want %q", keys[0].Title, "my-key") | ||
| } | ||
| if keys[0].Key != "ssh-ed25519 AAAA test@example.com" { | ||
| t.Errorf("key = %q, want full key string", keys[0].Key) | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_ListSSHKeys_Empty(t *testing.T) { | ||
| client := &GitHubClient{Commander: &mockCommander{output: []byte("[]")}} | ||
| keys, err := client.ListSSHKeys() | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if len(keys) != 0 { | ||
| t.Errorf("expected 0 keys, got %d", len(keys)) | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_ListSSHKeys_CommandError(t *testing.T) { | ||
| client := &GitHubClient{Commander: &mockCommander{output: []byte("error"), err: fmt.Errorf("gh failed")}} | ||
| _, err := client.ListSSHKeys() | ||
| if err == nil { | ||
| t.Error("expected error for command failure") | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_ListSSHKeys_BadJSON(t *testing.T) { | ||
| client := &GitHubClient{Commander: &mockCommander{output: []byte("not json")}} | ||
| _, err := client.ListSSHKeys() | ||
| if err == nil { | ||
| t.Error("expected error for bad JSON") | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_IsKeyRegistered_Match(t *testing.T) { | ||
| // Create a pub key file | ||
| tmpDir := t.TempDir() | ||
| pubPath := filepath.Join(tmpDir, "test.pub") | ||
| if err := os.WriteFile(pubPath, []byte("ssh-ed25519 AAAA_MATCH test@example.com\n"), 0644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| // Mock gh returning a matching key | ||
| jsonData := `[{"id": 1, "key": "ssh-ed25519 AAAA_MATCH test@example.com", "title": "matched"}]` | ||
| client := &GitHubClient{Commander: &mockCommander{output: []byte(jsonData)}} | ||
|
|
||
| registered, err := client.IsKeyRegistered(pubPath, tmpDir) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if !registered { | ||
| t.Error("expected key to be registered") | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_IsKeyRegistered_NoMatch(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
| pubPath := filepath.Join(tmpDir, "test.pub") | ||
| if err := os.WriteFile(pubPath, []byte("ssh-ed25519 AAAA_MATCH test@example.com\n"), 0644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| // Mock gh returning a non-matching key | ||
| jsonNoMatch := `[{"id": 1, "key": "ssh-ed25519 BBBB_NOMATCH other@example.com", "title": "other"}]` | ||
| client := &GitHubClient{Commander: &mockCommander{output: []byte(jsonNoMatch)}} | ||
|
|
||
| registered, err := client.IsKeyRegistered(pubPath, tmpDir) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if registered { | ||
| t.Error("expected key to NOT be registered") | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_IsKeyRegistered_EmptyGitHubKeys(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
| pubPath := filepath.Join(tmpDir, "test.pub") | ||
| if err := os.WriteFile(pubPath, []byte("ssh-ed25519 AAAA_MATCH test@example.com\n"), 0644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| client := &GitHubClient{Commander: &mockCommander{output: []byte("[]")}} | ||
|
|
||
| registered, err := client.IsKeyRegistered(pubPath, tmpDir) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if registered { | ||
| t.Error("expected key to NOT be registered when no GitHub keys exist") | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_IsKeyRegistered_BadPubKey(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
| pubPath := filepath.Join(tmpDir, "test.pub") | ||
| // Write a malformed public key with only one field | ||
| if err := os.WriteFile(pubPath, []byte("justoneword\n"), 0644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| client := &GitHubClient{Commander: &mockCommander{output: []byte("[]")}} | ||
|
|
||
| _, err := client.IsKeyRegistered(pubPath, tmpDir) | ||
| if err == nil { | ||
| t.Error("expected error for invalid public key format") | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_IsAuthenticated_Success(t *testing.T) { | ||
| client := &GitHubClient{Commander: &mockCommander{output: []byte("logged in")}} | ||
| auth, err := client.IsAuthenticated() | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if !auth { | ||
| t.Error("expected authenticated to be true") | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_IsAuthenticated_NotAuth(t *testing.T) { | ||
| client := &GitHubClient{Commander: &mockCommander{err: fmt.Errorf("not logged in")}} | ||
| auth, err := client.IsAuthenticated() | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v (should return false, not error)", err) | ||
| } | ||
| if auth { | ||
| t.Error("expected authenticated to be false") | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_IsAuthenticated_Smoke(t *testing.T) { | ||
| if !HasGHCLI() { | ||
| t.Skip("gh CLI not available") | ||
| } | ||
| client := NewGitHubClient() | ||
| auth, err := client.IsAuthenticated() | ||
| if err != nil { | ||
| t.Logf("IsAuthenticated error: %v", err) | ||
| } | ||
| t.Logf("IsAuthenticated: %v", auth) | ||
| } | ||
|
|
||
| func TestNewGitHubClient(t *testing.T) { | ||
| client := NewGitHubClient() | ||
| if client == nil { | ||
| t.Fatal("expected non-nil client") | ||
| } | ||
| if client.Commander == nil { | ||
| t.Error("expected non-nil commander") | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_getCommander_NilFallback(t *testing.T) { | ||
| client := &GitHubClient{Commander: nil} | ||
| cmd := client.getCommander() | ||
| if cmd == nil { | ||
| t.Error("expected non-nil commander from fallback") | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_ListGPGKeys_Parse(t *testing.T) { | ||
| jsonData := `[{"id": 1, "key_id": "ABCDEF1234567890", "email": "test@example.com"}]` | ||
| client := &GitHubClient{Commander: &mockCommander{output: []byte(jsonData)}} | ||
|
|
||
| keys, err := client.ListGPGKeys() | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if len(keys) != 1 { | ||
| t.Fatalf("expected 1 key, got %d", len(keys)) | ||
| } | ||
| if keys[0].KeyID != "ABCDEF1234567890" { | ||
| t.Errorf("key_id = %q, want %q", keys[0].KeyID, "ABCDEF1234567890") | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_ListGPGKeys_Empty(t *testing.T) { | ||
| client := &GitHubClient{Commander: &mockCommander{output: []byte("[]")}} | ||
| keys, err := client.ListGPGKeys() | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if len(keys) != 0 { | ||
| t.Errorf("expected 0 keys, got %d", len(keys)) | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_ListGPGKeys_CommandError(t *testing.T) { | ||
| client := &GitHubClient{Commander: &mockCommander{err: fmt.Errorf("gh failed")}} | ||
| _, err := client.ListGPGKeys() | ||
| if err == nil { | ||
| t.Error("expected error for command failure") | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_IsGPGKeyRegistered_Match(t *testing.T) { | ||
| jsonData := `[{"id": 1, "key_id": "ABCDEF1234567890", "email": "test@example.com"}]` | ||
| client := &GitHubClient{Commander: &mockCommander{output: []byte(jsonData)}} | ||
|
|
||
| registered, err := client.IsGPGKeyRegistered("ABCDEF1234567890") | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if !registered { | ||
| t.Error("expected key to be registered") | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_IsGPGKeyRegistered_CaseInsensitive(t *testing.T) { | ||
| jsonData := `[{"id": 1, "key_id": "ABCDEF1234567890", "email": "test@example.com"}]` | ||
| client := &GitHubClient{Commander: &mockCommander{output: []byte(jsonData)}} | ||
|
|
||
| registered, err := client.IsGPGKeyRegistered("abcdef1234567890") | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if !registered { | ||
| t.Error("expected case-insensitive match") | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_IsGPGKeyRegistered_ShortIDNoMatch(t *testing.T) { | ||
| // Short form key IDs should NOT match long form (risk of Evil32 false positives) | ||
| jsonData := `[{"id": 1, "key_id": "ABCDEF1234567890", "email": "test@example.com"}]` | ||
| client := &GitHubClient{Commander: &mockCommander{output: []byte(jsonData)}} | ||
|
|
||
| registered, err := client.IsGPGKeyRegistered("34567890") | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if registered { | ||
| t.Error("short key IDs should not match via suffix (Evil32 risk)") | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_IsGPGKeyRegistered_NoMatch(t *testing.T) { | ||
| jsonData := `[{"id": 1, "key_id": "ABCDEF1234567890", "email": "test@example.com"}]` | ||
| client := &GitHubClient{Commander: &mockCommander{output: []byte(jsonData)}} | ||
|
|
||
| registered, err := client.IsGPGKeyRegistered("11111111") | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if registered { | ||
| t.Error("expected key to NOT be registered") | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_IsGPGKeyRegistered_BadKeyID(t *testing.T) { | ||
| client := &GitHubClient{Commander: &mockCommander{output: []byte("[]")}} | ||
| _, err := client.IsGPGKeyRegistered("not-hex!") | ||
| if err == nil { | ||
| t.Error("expected error for invalid key ID") | ||
| } | ||
| } | ||
|
|
||
| func TestGitHubClient_ListSSHKeys_MultipleKeys(t *testing.T) { | ||
| jsonData := `[ | ||
| {"id": 1, "key": "ssh-ed25519 AAAA first@example.com", "title": "key-1"}, | ||
| {"id": 2, "key": "ssh-rsa BBBB second@example.com", "title": "key-2"}, | ||
| {"id": 3, "key": "ssh-ed25519 CCCC third@example.com", "title": "key-3"} | ||
| ]` | ||
| client := &GitHubClient{Commander: &mockCommander{output: []byte(jsonData)}} | ||
|
|
||
| keys, err := client.ListSSHKeys() | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if len(keys) != 3 { | ||
| t.Fatalf("expected 3 keys, got %d", len(keys)) | ||
| } | ||
| if keys[1].Title != "key-2" { | ||
| t.Errorf("second key title = %q, want %q", keys[1].Title, "key-2") | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor these unit tests into table-driven form.
The file is highly repetitive and hard to scale for new edge cases. Converting grouped scenarios (AddSSHKey, AddGPGKey, List*, Is*Registered) to table-driven tests will reduce duplication and improve maintainability.
As per coding guidelines **/*_test.go: Use table-driven test patterns for Go tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/machine/github_test.go` around lines 26 - 402, The tests for
GitHubClient are repetitive and should be refactored into table-driven tests;
consolidate related test cases (those for AddSSHKey:
TestGitHubClient_AddSSHKey_BadPath, _NotPubFile, _BadTitle, _Success,
_CommandFailure; AddGPGKey: _BadKeyID, _Empty, _TooShort;
ListSSHKeys/ListGPGKeys parse/empty/error cases; IsKeyRegistered and
IsGPGKeyRegistered scenarios) into table structs keyed by name, input setup (tmp
files, mockCommander.output/err, title/keyID, workdir), expected result and
expected error, then iterate the table calling the underlying methods
(AddSSHKey, AddGPGKey, ListSSHKeys, ListGPGKeys, IsKeyRegistered,
IsGPGKeyRegistered) and run assertions; keep helper logic for creating temp
files and mockCommander instances and preserve edge checks for malformed pub
keys and case-sensitivity; ensure existing standalone tests (NewGitHubClient,
getCommander fallback, IsAuthenticated smoke) remain if they test unique
behavior.
- Replace unsupported gh ssh-key list --json with gh api user/keys - Replace unsupported gh gpg-key list --json with gh api /user/gpg_keys - Add error context wrapping in IsGPGKeyRegistered and IsKeyRegistered - Enhance mockCommander to record calls for test assertions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@greptileai review |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/machine/github.go`:
- Around line 86-111: The AddGPGKey method currently calls exec.Command directly
(gpg and gh) which bypasses the injected Commander and prevents unit testing;
refactor AddGPGKey to use the Commander abstraction instead—either add a
RunWithStdin(name string, stdin io.Reader, args ...string) (or similar) to the
Commander interface and call c.commander.RunWithStdin("gh",
bytes.NewReader(gpgOut), "gpg-key", "add"), and replace the gpg export
invocation with c.commander.Run("gpg", "--armor", "--export", keyID) (or a
RunCapture variant) so both the gpg export and gh add paths use the Commander
methods (or alternatively write gpgOut to a temp file via Commander helpers and
invoke gh with that file); update AddGPGKey to handle returned output and stderr
from the Commander methods and keep the same validation/error wrapping.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/machine/github.gointernal/machine/github_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/machine/github_test.go
Comment on lines
+86
to
+111
| // AddGPGKey registers a GPG key with GitHub. | ||
| // Exports the key via gpg, validates it's non-empty, then adds via gh. | ||
| func (c *GitHubClient) AddGPGKey(keyID string) error { | ||
| if err := validation.ValidateGPGKeyID(keyID); err != nil { | ||
| return fmt.Errorf("invalid GPG key ID: %w", err) | ||
| } | ||
|
|
||
| // Export GPG key first to detect missing-key case | ||
| gpgOut, err := exec.Command("gpg", "--armor", "--export", keyID).Output() | ||
| if err != nil { | ||
| return fmt.Errorf("gpg export failed: %w", err) | ||
| } | ||
| if len(strings.TrimSpace(string(gpgOut))) == 0 { | ||
| return fmt.Errorf("gpg key %q not found in local keyring", keyID) | ||
| } | ||
|
|
||
| // Add to GitHub via gh CLI | ||
| ghCmd := exec.Command("gh", "gpg-key", "add") | ||
| ghCmd.Stdin = strings.NewReader(string(gpgOut)) | ||
| var ghStderr bytes.Buffer | ||
| ghCmd.Stderr = &ghStderr | ||
| if err := ghCmd.Run(); err != nil { | ||
| return fmt.Errorf("gh gpg-key add failed: %w\nStderr: %s", err, ghStderr.String()) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
AddGPGKey bypasses Commander interface, breaking testability.
Lines 94 and 103-107 use exec.Command directly instead of the injected Commander. This defeats the purpose of the Commander abstraction and makes unit testing this method impossible without actually executing gpg and gh commands.
Consider extending the Commander interface or using a two-step approach that remains testable:
Suggested approach
func (c *GitHubClient) AddGPGKey(keyID string) error {
if err := validation.ValidateGPGKeyID(keyID); err != nil {
return fmt.Errorf("invalid GPG key ID: %w", err)
}
// Export GPG key first to detect missing-key case
- gpgOut, err := exec.Command("gpg", "--armor", "--export", keyID).Output()
+ gpgOut, err := c.getCommander().Run("gpg", "--armor", "--export", keyID)
if err != nil {
return fmt.Errorf("gpg export failed: %w", err)
}
if len(strings.TrimSpace(string(gpgOut))) == 0 {
return fmt.Errorf("gpg key %q not found in local keyring", keyID)
}
- // Add to GitHub via gh CLI
- ghCmd := exec.Command("gh", "gpg-key", "add")
- ghCmd.Stdin = strings.NewReader(string(gpgOut))
- var ghStderr bytes.Buffer
- ghCmd.Stderr = &ghStderr
- if err := ghCmd.Run(); err != nil {
- return fmt.Errorf("gh gpg-key add failed: %w\nStderr: %s", err, ghStderr.String())
- }
+ // Add to GitHub via gh CLI using stdin via echo pipe
+ // Note: gh gpg-key add reads from stdin
+ output, err := c.getCommander().Run("gh", "gpg-key", "add", "-")
+ if err != nil {
+ return fmt.Errorf("gh gpg-key add failed: %w\nOutput: %s", err, string(output))
+ }
return nil
}For stdin piping, you may need to extend Commander with a RunWithStdin(name string, stdin io.Reader, args ...string) method, or write the exported key to a temp file and pass that path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/machine/github.go` around lines 86 - 111, The AddGPGKey method
currently calls exec.Command directly (gpg and gh) which bypasses the injected
Commander and prevents unit testing; refactor AddGPGKey to use the Commander
abstraction instead—either add a RunWithStdin(name string, stdin io.Reader, args
...string) (or similar) to the Commander interface and call
c.commander.RunWithStdin("gh", bytes.NewReader(gpgOut), "gpg-key", "add"), and
replace the gpg export invocation with c.commander.Run("gpg", "--armor",
"--export", keyID) (or a RunCapture variant) so both the gpg export and gh add
paths use the Commander methods (or alternatively write gpgOut to a temp file
via Commander helpers and invoke gh with that file); update AddGPGKey to handle
returned output and stderr from the Commander methods and keep the same
validation/error wrapping.
Additional Comments (2)
Either require full 40-char fingerprints only (safest against Evil32), or normalize short IDs to full fingerprints before comparison.
|
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
GitHubClientwithCommanderinterface for testability (matches stow pattern)AddSSHKey(),AddGPGKey()(gpg→gh pipe, no shell),ListSSHKeys(),IsKeyRegistered()ValidateGPGKeyID()andValidateKeyTitle()security validatorsg4d machine keys registerDepends on: #PR1 (feat/ssh-keygen-wizard)
Test plan
make build && make lint && make testpassesg4d machine keys registerdetects gh CLI and auth status🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
machine keys registercommand to detect and register local SSH and GPG keys with GitHub via the CLI.Validation
Tests