refactor: extract cmd/up.go into focused pkg/ packages#719
Merged
Conversation
Move IDE opening logic (VSCode flavors, JetBrains, browser-based IDEs, Fleet) from cmd/up.go into a dedicated pkg/ide/opener package with an exported Open() function and Params struct. Helper functions (performGpgForwarding, startBrowserTunnel, setupBackhaul, createSSHCommand) are temporarily duplicated — they will move to pkg/gpg/ and pkg/tunnel/ in subsequent tasks.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 50 seconds. ⌛ 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR refactors Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 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 |
skevetter
force-pushed
the
plan/refactor-workspace-up
branch
from
0cb3678 to
1026036
Compare
- Add 7 table-driven tests for providerVersionNeedsUpdate covering same version, upgrades, downgrades, mixed prefixes, and invalid input - Add buildSSHCommandArgs test case for debug + extra args combined - Rename dotCmd to backhaulCmd in SetupBackhaul for clarity
skevetter
force-pushed
the
plan/refactor-workspace-up
branch
from
1026036 to
09a82a1
Compare
![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 (3)
pkg/workspace/provider_update.go (1)
116-122: Consider logging the error before returning nil.
GetProInstancesilently swallows the error fromListProInstances. While returningnilis acceptable for "not found" scenarios, an actual listing error (e.g., filesystem issue) may warrant at least a debug log so users can diagnose unexpected behavior.Proposed improvement
func GetProInstance( devPodConfig *config.Config, providerName string, log log.Logger, ) *provider2.ProInstance { proInstances, err := ListProInstances(devPodConfig, log) if err != nil { + log.Debugf("failed to list pro instances: %v", err) return nil } else if len(proInstances) == 0 { return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/workspace/provider_update.go` around lines 116 - 122, GetProInstance currently calls ListProInstances(devPodConfig, log) and returns nil on any error without logging; update GetProInstance to log the returned err (using the provided log instance) before returning nil so listing failures are observable; reference the call to ListProInstances and the variables proInstances and err inside GetProInstance when adding a log statement (e.g., debug/error level) to record context about devPodConfig and the error.pkg/gpg/forward_test.go (1)
9-21: Consider usingassert.Equalfor stricter argument ordering verification.Using
assert.Containsverifies presence but not the correct ordering of flag-value pairs (e.g., that"root"follows"--user"). Since CLI argument order typically matters, consider testing the exact slice to catch ordering regressions.Stricter assertion example
func TestBuildForwardArgs(t *testing.T) { args := buildForwardArgs("root", "test-context", "test-workspace") - assert.Contains(t, args, "ssh") - assert.Contains(t, args, "--gpg-agent-forwarding=true") - assert.Contains(t, args, "--agent-forwarding=true") - assert.Contains(t, args, "--user") - assert.Contains(t, args, "root") - assert.Contains(t, args, "--context") - assert.Contains(t, args, "test-context") - assert.Contains(t, args, "test-workspace") - assert.Contains(t, args, "--command") - assert.Contains(t, args, "sleep infinity") + expected := []string{ + "ssh", + "--gpg-agent-forwarding=true", + "--agent-forwarding=true", + "--start-services=true", + "--user", + "root", + "--context", + "test-context", + "test-workspace", + "--log-output=raw", + "--command", "sleep infinity", + } + assert.Equal(t, expected, args) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/gpg/forward_test.go` around lines 9 - 21, Update TestBuildForwardArgs to assert the exact argument ordering by replacing the multiple assert.Contains checks with a single assert.Equal that compares the returned slice from buildForwardArgs("root", "test-context", "test-workspace") to the expected exact []string (including "ssh", "--gpg-agent-forwarding=true", "--agent-forwarding=true", "--user", "root", "--context", "test-context", "test-workspace", "--command", "sleep infinity") so the test fails on any ordering or missing/extra argument regressions.pkg/gpg/forward.go (1)
34-40: Improve error reporting and avoid variable shadowing.The goroutine captures and reassigns the outer
errvariable, which is confusing (though safe sinceForwardAgentreturns before the goroutine completes). Additionally, the error message doesn't include the actual error value, making debugging difficult.Proposed improvement
go func() { - //nolint:gosec // execPath comes from os.Executable() - err = exec.Command(execPath, args...).Run() - if err != nil { - logger.Error("failure in forwarding gpg-agent") + //nolint:gosec // execPath comes from os.Executable() + if runErr := exec.Command(execPath, args...).Run(); runErr != nil { + logger.Errorf("failure in forwarding gpg-agent: %v", runErr) } }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/gpg/forward.go` around lines 34 - 40, The goroutine in ForwardAgent currently reassigns the outer err and logs a generic message; change it to capture the exec.Command(...).Run() result into a local variable (e.g., localErr := exec.Command(execPath, args...).Run()) to avoid shadowing, and include the actual error when logging (use logger.Error with the error value or logger.WithError(localErr).Error) so the log shows the underlying failure from exec.Command(execPath, args...).Run() instead of just "failure in forwarding gpg-agent".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/ide/opener/opener.go`:
- Around line 109-140: ParseAddressAndPort currently returns a single "address"
string (sometimes the full host:port) which hides the configured bind host;
change it so ParseAddressAndPort (and helpers parseDefaultPort and
parseExplicitAddress) return the host separately (host string, port int, error)
where parseExplicitAddress uses net.SplitHostPort to extract host and port,
preserves any non-wildcard host verbatim (including IPv6) and only normalizes
wildcard binds ("0.0.0.0", "::", empty or "*") to "localhost"; parseDefaultPort
should return the chosen port and a sensible default host (e.g. "localhost" or
the configured default) rather than a combined address string; update callers of
ParseAddressAndPort to use the returned host and port when constructing/logging
the browser URL.
- Around line 219-247: The StartServicesDaemon call from makeDaemonStartFunc is
missing propagation of the Git SSH signing key, causing daemon-based browser IDE
sessions to drop the --git-ssh-signing-key flag; add a GitSSHSigningKey field to
the StartServicesDaemonOptions struct and set it when calling
StartServicesDaemon in makeDaemonStartFunc (pass params.GitSSHSigningKey),
mirroring how RunServicesOptions is populated in the SSH path (see cmd/ssh.go)
so the daemon path receives the same GitSSHSigningKey value.
In `@pkg/tunnel/browser.go`:
- Around line 37-43: StartBrowserTunnel spawns a fire-and-forget backhaul but
doesn't pass the caller context to SetupBackhaul, so the child SSH process can
outlive the browser session; update SetupBackhaul to accept a context.Context
(or add a SetupBackhaulContext wrapper) and use exec.CommandContext inside it,
then call it from StartBrowserTunnel with p.Ctx (or context.Background fallback)
inside the goroutine; update BrowserTunnelParams usage and any callers of
SetupBackhaul accordingly so the backhaul process is canceled/teardown when
p.Ctx is done.
In `@pkg/workspace/provider_update.go`:
- Around line 94-98: The check using strings.Split(providerSource, "@") is
unreachable because Split always returns >=1 item; change the validation to
verify an '@' exists by checking len(splitted) < 2 (or providerSource contains
"@") before proceeding. Update the error path to return a clear message (e.g.,
"no provider version delimiter '@' in providerSource") and keep the existing
assignment providerSource = splitted[0] + "@" + newVersion after the validation;
reference symbols: strings.Split, splitted, providerSource, newVersion.
---
Nitpick comments:
In `@pkg/gpg/forward_test.go`:
- Around line 9-21: Update TestBuildForwardArgs to assert the exact argument
ordering by replacing the multiple assert.Contains checks with a single
assert.Equal that compares the returned slice from buildForwardArgs("root",
"test-context", "test-workspace") to the expected exact []string (including
"ssh", "--gpg-agent-forwarding=true", "--agent-forwarding=true", "--user",
"root", "--context", "test-context", "test-workspace", "--command", "sleep
infinity") so the test fails on any ordering or missing/extra argument
regressions.
In `@pkg/gpg/forward.go`:
- Around line 34-40: The goroutine in ForwardAgent currently reassigns the outer
err and logs a generic message; change it to capture the exec.Command(...).Run()
result into a local variable (e.g., localErr := exec.Command(execPath,
args...).Run()) to avoid shadowing, and include the actual error when logging
(use logger.Error with the error value or logger.WithError(localErr).Error) so
the log shows the underlying failure from exec.Command(execPath, args...).Run()
instead of just "failure in forwarding gpg-agent".
In `@pkg/workspace/provider_update.go`:
- Around line 116-122: GetProInstance currently calls
ListProInstances(devPodConfig, log) and returns nil on any error without
logging; update GetProInstance to log the returned err (using the provided log
instance) before returning nil so listing failures are observable; reference the
call to ListProInstances and the variables proInstances and err inside
GetProInstance when adding a log statement (e.g., debug/error level) to record
context about devPodConfig and the error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f2642ec9-81e1-4f3d-80e5-524138ddd891
📒 Files selected for processing (11)
cmd/up.gopkg/dotfiles/dotfiles.gopkg/dotfiles/dotfiles_test.gopkg/gpg/forward.gopkg/gpg/forward_test.gopkg/ide/opener/opener.gopkg/ide/opener/opener_test.gopkg/tunnel/browser.gopkg/tunnel/browser_test.gopkg/workspace/provider_update.gopkg/workspace/provider_update_test.go
Comment on lines
+109
to
+140
| func ParseAddressAndPort(bindAddressOption string, defaultPort int) (string, int, error) { | ||
| if bindAddressOption == "" { | ||
| return parseDefaultPort(defaultPort) | ||
| } | ||
|
|
||
| return parseExplicitAddress(bindAddressOption) | ||
| } | ||
|
|
||
| func parseDefaultPort(defaultPort int) (string, int, error) { | ||
| portName, err := port.FindAvailablePort(defaultPort) | ||
| if err != nil { | ||
| return "", 0, err | ||
| } | ||
|
|
||
| return fmt.Sprintf("%d", portName), portName, nil | ||
| } | ||
|
|
||
| func parseExplicitAddress(address string) (string, int, error) { | ||
| _, p, err := net.SplitHostPort(address) | ||
| if err != nil { | ||
| return "", 0, fmt.Errorf("parse host:port: %w", err) | ||
| } | ||
| if p == "" { | ||
| return "", 0, fmt.Errorf("parse ADDRESS: expected host:port, got %s", address) | ||
| } | ||
|
|
||
| portName, err := strconv.Atoi(p) | ||
| if err != nil { | ||
| return "", 0, fmt.Errorf("parse host:port: %w", err) | ||
| } | ||
|
|
||
| return address, portName, nil |
There was a problem hiding this comment.
Respect the configured bind host in the browser URL.
ParseAddressAndPort accepts an explicit host:port, but the Jupyter, RStudio, and OpenVSCode flows always open/log localhost. A non-loopback or IPv6-only bind address will forward successfully yet still send the user to the wrong URL. Return the host separately and only normalize wildcard binds (0.0.0.0 / ::) to localhost.
Also applies to: 276-276, 326-326, 375-375
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/ide/opener/opener.go` around lines 109 - 140, ParseAddressAndPort
currently returns a single "address" string (sometimes the full host:port) which
hides the configured bind host; change it so ParseAddressAndPort (and helpers
parseDefaultPort and parseExplicitAddress) return the host separately (host
string, port int, error) where parseExplicitAddress uses net.SplitHostPort to
extract host and port, preserves any non-wildcard host verbatim (including IPv6)
and only normalizes wildcard binds ("0.0.0.0", "::", empty or "*") to
"localhost"; parseDefaultPort should return the chosen port and a sensible
default host (e.g. "localhost" or the configured default) rather than a combined
address string; update callers of ParseAddressAndPort to use the returned host
and port when constructing/logging the browser URL.
Comment on lines
+219
to
+247
| func makeDaemonStartFunc( | ||
| params Params, | ||
| forwardPorts bool, | ||
| extraPorts []string, | ||
| ) func(ctx context.Context) error { | ||
| daemonClient, ok := params.Client.(client2.DaemonClient) | ||
| if !ok { | ||
| return nil | ||
| } | ||
|
|
||
| return func(ctx context.Context) error { | ||
| toolClient, _, err := daemonClient.SSHClients(ctx, params.User) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer func() { _ = toolClient.Close() }() | ||
|
|
||
| err = clientimplementation.StartServicesDaemon( | ||
| ctx, | ||
| clientimplementation.StartServicesDaemonOptions{ | ||
| DevPodConfig: params.DevPodConfig, | ||
| Client: daemonClient, | ||
| SSHClient: toolClient, | ||
| User: params.User, | ||
| Log: params.Log, | ||
| ForwardPorts: forwardPorts, | ||
| ExtraPorts: extraPorts, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== StartServicesDaemonOptions / StartServicesDaemon ==="
rg -n -C4 '\btype StartServicesDaemonOptions struct\b|\bfunc StartServicesDaemon\b|\bGitSSHSigningKey\b|\bConfigureGitSSHSignatureHelper\b' --type=go
echo
echo "=== Daemon browser path ==="
rg -n -C4 '\bmakeDaemonStartFunc\b|\bStartBrowserTunnel\b|\bGitSSHSigningKey\b' --type=goRepository: skevetter/devpod
Length of output: 23992
Pass GitSSHSigningKey through StartServicesDaemonOptions to fix daemon browser path.
The daemon path in makeDaemonStartFunc calls StartServicesDaemon but StartServicesDaemonOptions lacks a GitSSHSigningKey field. This means daemon-based browser IDE sessions lose --git-ssh-signing-key support, while the SSH tunnel path correctly forwards it via RunServicesOptions. The SSH command path (cmd/ssh.go) explicitly passes GitSSHSigningKey to show this is required. Either add GitSSHSigningKey to StartServicesDaemonOptions and propagate it from makeDaemonStartFunc (via params.GitSSHSigningKey), or extract it from the workspace config inside StartServicesDaemon.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/ide/opener/opener.go` around lines 219 - 247, The StartServicesDaemon
call from makeDaemonStartFunc is missing propagation of the Git SSH signing key,
causing daemon-based browser IDE sessions to drop the --git-ssh-signing-key
flag; add a GitSSHSigningKey field to the StartServicesDaemonOptions struct and
set it when calling StartServicesDaemon in makeDaemonStartFunc (pass
params.GitSSHSigningKey), mirroring how RunServicesOptions is populated in the
SSH path (see cmd/ssh.go) so the daemon path receives the same GitSSHSigningKey
value.
Comment on lines
+37
to
+43
| func StartBrowserTunnel(p BrowserTunnelParams) error { | ||
| if p.AuthSockID != "" { | ||
| go func() { | ||
| if err := SetupBackhaul(p.Client, p.AuthSockID, p.Logger); err != nil { | ||
| p.Logger.Error("Failed to setup backhaul SSH connection: ", err) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Bind the backhaul process to the caller context.
StartBrowserTunnel fire-and-forgets a long-lived devpod ssh backhaul, but SetupBackhaul uses exec.Command and never sees p.Ctx. When the browser session ends, that child can keep the SSH session and forwarded agent alive until it exits on its own. Please thread the context through so teardown matches the tunnel lifetime.
Suggested fix
func StartBrowserTunnel(p BrowserTunnelParams) error {
if p.AuthSockID != "" {
go func() {
- if err := SetupBackhaul(p.Client, p.AuthSockID, p.Logger); err != nil {
+ if err := SetupBackhaul(p.Ctx, p.Client, p.AuthSockID, p.Logger); err != nil {
p.Logger.Error("Failed to setup backhaul SSH connection: ", err)
}
}()
}
@@
-func SetupBackhaul(client client2.BaseWorkspaceClient, authSockID string, logger log.Logger) error {
+func SetupBackhaul(ctx context.Context, client client2.BaseWorkspaceClient, authSockID string, logger log.Logger) error {
execPath, err := os.Executable()
if err != nil {
return err
}
@@
- backhaulCmd := exec.Command(
+ backhaulCmd := exec.CommandContext(
+ ctx,
execPath,
"ssh",
@@
logger.Info("Setting up backhaul SSH connection")
writer := logger.Writer(logrus.InfoLevel, false)
+ defer func() { _ = writer.Close() }()
backhaulCmd.Stdout = writer
backhaulCmd.Stderr = writerAlso applies to: 123-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/tunnel/browser.go` around lines 37 - 43, StartBrowserTunnel spawns a
fire-and-forget backhaul but doesn't pass the caller context to SetupBackhaul,
so the child SSH process can outlive the browser session; update SetupBackhaul
to accept a context.Context (or add a SetupBackhaulContext wrapper) and use
exec.CommandContext inside it, then call it from StartBrowserTunnel with p.Ctx
(or context.Background fallback) inside the goroutine; update
BrowserTunnelParams usage and any callers of SetupBackhaul accordingly so the
backhaul process is canceled/teardown when p.Ctx is done.
Comment on lines
+94
to
+98
| splitted := strings.Split(providerSource, "@") | ||
| if len(splitted) == 0 { | ||
| return fmt.Errorf("no provider source found %s", providerSource) | ||
| } | ||
| providerSource = splitted[0] + "@" + newVersion |
There was a problem hiding this comment.
Unreachable condition: strings.Split always returns at least one element.
strings.Split(providerSource, "@") will always return a slice with at least one element (the original string if no @ is found). The check len(splitted) == 0 is never true. Consider checking len(splitted) < 2 if you want to validate that an @ delimiter exists.
Proposed fix
splitted := strings.Split(providerSource, "@")
- if len(splitted) == 0 {
- return fmt.Errorf("no provider source found %s", providerSource)
+ if len(splitted) < 2 {
+ return fmt.Errorf("provider source %s missing version suffix (`@version`)", providerSource)
}
providerSource = splitted[0] + "@" + newVersion📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| splitted := strings.Split(providerSource, "@") | |
| if len(splitted) == 0 { | |
| return fmt.Errorf("no provider source found %s", providerSource) | |
| } | |
| providerSource = splitted[0] + "@" + newVersion | |
| splitted := strings.Split(providerSource, "@") | |
| if len(splitted) < 2 { | |
| return fmt.Errorf("provider source %s missing version suffix (`@version`)", providerSource) | |
| } | |
| providerSource = splitted[0] + "@" + newVersion |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/workspace/provider_update.go` around lines 94 - 98, The check using
strings.Split(providerSource, "@") is unreachable because Split always returns
>=1 item; change the validation to verify an '@' exists by checking
len(splitted) < 2 (or providerSource contains "@") before proceeding. Update the
error path to return a clear message (e.g., "no provider version delimiter '@'
in providerSource") and keep the existing assignment providerSource =
splitted[0] + "@" + newVersion after the validation; reference symbols:
strings.Split, splitted, providerSource, newVersion.
- Fix error variable shadowing in ForwardAgent goroutine - Include actual error in gpg-agent forwarding failure log - Use assert.Equal for strict argument ordering in GPG test
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
cmd/up.go(1839 → 898 lines) into 5 focused packages:pkg/dotfiles— dotfiles setup logic (14 tests)pkg/ide/opener— IDE launch dispatch for 20+ IDE types (7 tests)pkg/gpg— GPG agent forwarding (1 test)pkg/tunnel— browser tunnel, backhaul, SSH command creation (4 tests)pkg/workspace— provider version update checking (11 tests)clientimplementationandtunnelresolved viaDaemonStartFunccallback patternSummary by CodeRabbit
Release Notes
New Features
Refactor