From 54821c6bc5759779e7a6409c845d8c7eff6a4e6c Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 19:45:09 +0000 Subject: [PATCH 1/2] fix(lint): resolve all cyclop findings cyclop is on by default once enabled (max-complexity: 8 already set) -- no separate opt-in setting, same as funcorder. Fixes all 10 pre-existing violations by extracting a cohesive sub-step out of each over-complex function into a well-named helper: - cmd/internal/agentcontainer/setup.go: syncMounts -> restoreSnapshotMounts - cmd/provider/configure_shared.go: initProvider -> providerConfigEntry - cmd/provider/list.go: runAvailable -> availableProviderNames - cmd/provider/set_source.go: Run -> activateProvider - cmd/workspace/import.go: importWorkspace -> extractWorkspaceData, applySnapshotSource - cmd/workspace/up/up.go: Run -> setUpTask - cmd/workspace/up/up_client.go: prepareClient -> checkProviderUpdate - pkg/agent/delivery/factory.go: NewAgentDelivery split into namedDriverDelivery (exact-match dispatch) plus appleDelivery and microsandboxDelivery helpers - pkg/provider/parse.go: validateAgentDriver's chained driver-name comparison replaced with a validAgentDrivers set lookup - pkg/ssh/forward.go: portForwarding -> watchTransportClosed All extractions are behavior-preserving; verified against existing test coverage (pkg/agent/delivery/factory_test.go exercises every branch of the driver dispatch). No logic changes. --- cmd/internal/agentcontainer/setup.go | 34 +++++++----- cmd/provider/configure_shared.go | 21 ++++--- cmd/provider/list.go | 25 ++++++--- cmd/provider/set_source.go | 12 ++++ cmd/workspace/import.go | 82 +++++++++++++++++----------- cmd/workspace/up/up.go | 33 ++++++++--- cmd/workspace/up/up_client.go | 20 +++++-- pkg/agent/delivery/factory.go | 67 +++++++++++++++-------- pkg/provider/parse.go | 15 +++-- pkg/ssh/forward.go | 42 ++++++++------ 10 files changed, 228 insertions(+), 123 deletions(-) diff --git a/cmd/internal/agentcontainer/setup.go b/cmd/internal/agentcontainer/setup.go index d83610106..5683fbe5d 100644 --- a/cmd/internal/agentcontainer/setup.go +++ b/cmd/internal/agentcontainer/setup.go @@ -431,20 +431,7 @@ func (cmd *SetupContainerCmd) syncMounts(sctx *setupContext) error { // drivers), but a snapshot-sourced workspace needs its volumes restored // on every driver, docker included. if sctx.workspaceInfo.Source.Snapshot != "" { - if !sctx.workspaceInfo.CLIOptions.Reset && len(mounts) == 1 && - skipSnapshotRestore(mounts[0].Target) { - return nil - } - log.Infof("restoring snapshot volumes from %s", sctx.workspaceInfo.Source.Snapshot) - if err := agentsnapshot.RestoreVolumes( - sctx.ctx, - sctx.workspaceInfo.Source.Snapshot, - mounts, - sctx.workspaceInfo.CLIOptions.Reset, - ); err != nil { - return fmt.Errorf("restore snapshot volumes: %w", err) - } - return nil + return restoreSnapshotMounts(sctx, mounts) } if !cmd.StreamMounts { @@ -474,6 +461,25 @@ func (cmd *SetupContainerCmd) syncMounts(sctx *setupContext) error { return nil } +// restoreSnapshotMounts restores a snapshot-sourced workspace's volumes, +// skipping the restore if the sole mount target already has real content. +func restoreSnapshotMounts(sctx *setupContext, mounts []*config.Mount) error { + if !sctx.workspaceInfo.CLIOptions.Reset && len(mounts) == 1 && + skipSnapshotRestore(mounts[0].Target) { + return nil + } + log.Infof("restoring snapshot volumes from %s", sctx.workspaceInfo.Source.Snapshot) + if err := agentsnapshot.RestoreVolumes( + sctx.ctx, + sctx.workspaceInfo.Source.Snapshot, + mounts, + sctx.workspaceInfo.CLIOptions.Reset, + ); err != nil { + return fmt.Errorf("restore snapshot volumes: %w", err) + } + return nil +} + // synthesizedDevContainerName is the devcontainer.json devsy synthesizes for // image/none-sourced workspaces (pkg/devcontainer's saveSynthesizedConfig). A // snapshot-sourced restore gets one too, written into the mount target before diff --git a/cmd/provider/configure_shared.go b/cmd/provider/configure_shared.go index e09718277..dc51cfdfc 100644 --- a/cmd/provider/configure_shared.go +++ b/cmd/provider/configure_shared.go @@ -195,14 +195,7 @@ func initProvider( } defer func() { _ = lock.Unlock() }() - if devsyConfig.Current().Providers == nil { - devsyConfig.Current().Providers = map[string]*config.ProviderConfig{} - } - if devsyConfig.Current().Providers[provider.Name] == nil { - devsyConfig.Current().Providers[provider.Name] = &config.ProviderConfig{} - } - entry := devsyConfig.Current().Providers[provider.Name] - + entry := providerConfigEntry(devsyConfig, provider.Name) entry.InitAttempted = true entry.InitError = "" entry.Initialized = false @@ -231,6 +224,18 @@ func initProvider( return nil } +// providerConfigEntry returns the config.ProviderConfig entry for name, +// creating the Providers map and/or the entry itself if either is unset. +func providerConfigEntry(devsyConfig *config.Config, name string) *config.ProviderConfig { + if devsyConfig.Current().Providers == nil { + devsyConfig.Current().Providers = map[string]*config.ProviderConfig{} + } + if devsyConfig.Current().Providers[name] == nil { + devsyConfig.Current().Providers[name] = &config.ProviderConfig{} + } + return devsyConfig.Current().Providers[name] +} + const maxInitErrorLen = 500 func truncateInitError(msg string) string { diff --git a/cmd/provider/list.go b/cmd/provider/list.go index 6635b303a..cdbaf7977 100644 --- a/cmd/provider/list.go +++ b/cmd/provider/list.go @@ -177,6 +177,21 @@ func (cmd *ListCmd) runAvailable(ctx context.Context) error { return err } + providers := availableProviderNames(jsonResult) + + switch mode { + case output.ModePlain: + return cmd.renderAvailablePlain(providers) + case output.ModeJSON: + return cmd.renderAvailableJSON(providers) + } + + return nil +} + +// availableProviderNames extracts provider names from repo JSON entries, +// stripping the config.ProviderPrefix and skipping non-matching repos. +func availableProviderNames(jsonResult []map[string]any) []string { var providers []string for _, v := range jsonResult { name, ok := v["name"].(string) @@ -187,15 +202,7 @@ func (cmd *ListCmd) runAvailable(ctx context.Context) error { providers = append(providers, after) } } - - switch mode { - case output.ModePlain: - return cmd.renderAvailablePlain(providers) - case output.ModeJSON: - return cmd.renderAvailableJSON(providers) - } - - return nil + return providers } // renderAvailablePlain renders available providers in plain text format. diff --git a/cmd/provider/set_source.go b/cmd/provider/set_source.go index 732defcc5..3081ba473 100644 --- a/cmd/provider/set_source.go +++ b/cmd/provider/set_source.go @@ -10,6 +10,7 @@ import ( cliflags "github.com/devsy-org/devsy/pkg/flags" "github.com/devsy-org/devsy/pkg/flags/names" "github.com/devsy-org/devsy/pkg/log" + "github.com/devsy-org/devsy/pkg/provider" "github.com/devsy-org/devsy/pkg/status" "github.com/devsy-org/devsy/pkg/workspace" "github.com/spf13/cobra" @@ -89,6 +90,17 @@ func (cmd *SetSourceCmd) Run(ctx context.Context, devsyConfig *config.Config, ar return nil } + return cmd.activateProvider(ctx, devsyConfig, providerConfig, reporter) +} + +// activateProvider configures and activates a newly sourced provider, +// preserving previously user-provided option values. +func (cmd *SetSourceCmd) activateProvider( + ctx context.Context, + devsyConfig *config.Config, + providerConfig *provider.ProviderConfig, + reporter status.Reporter, +) error { // Preserve previously user-provided values (default DiscardPriorValues=false). // The resolver prunes keys absent from the new schema and re-resolves values // that fail validation, so stale data cannot leak through this path. diff --git a/cmd/workspace/import.go b/cmd/workspace/import.go index 04ee15bbc..883677355 100644 --- a/cmd/workspace/import.go +++ b/cmd/workspace/import.go @@ -128,25 +128,10 @@ func (cmd *ImportCmd) importWorkspace( devsyConfig *config.Config, exportConfig *provider.ExportConfig, ) error { - workspaceDir, err := provider.GetWorkspaceDir(devsyConfig.DefaultContext, cmd.WorkspaceID) - if err != nil { - return fmt.Errorf("get workspace dir: %w", err) - } - - // #nosec G301 -- TODO Consider using a more secure permission setting and ownership if needed. - err = os.MkdirAll(workspaceDir, 0o755) - if err != nil { - return fmt.Errorf("create workspace dir: %w", err) - } - - decoded, err := base64.RawStdEncoding.DecodeString(exportConfig.Workspace.Data) - if err != nil { - return fmt.Errorf("decode workspace data: %w", err) - } - - err = extract.Extract(bytes.NewReader(decoded), workspaceDir) - if err != nil { - return fmt.Errorf("extract workspace data: %w", err) + if err := extractWorkspaceData( + devsyConfig, cmd.WorkspaceID, exportConfig.Workspace.Data, + ); err != nil { + return err } // exchange config @@ -163,21 +148,9 @@ func (cmd *ImportCmd) importWorkspace( workspaceConfig.Provider.Name = cmd.ProviderID if exportConfig.SnapshotRef != "" { - sourceStr, devContainerSource, err := snapshotpkg.RestoreComposition( - exportConfig.SnapshotRef, - ) - if err != nil { - return fmt.Errorf("parse snapshot ref: %w", err) + if err := applySnapshotSource(workspaceConfig, exportConfig.SnapshotRef); err != nil { + return err } - parsedSource := provider.ParseWorkspaceSource(sourceStr) - if parsedSource == nil { - return fmt.Errorf( - "compose workspace source from snapshot ref: unexpected source %q", - sourceStr, - ) - } - workspaceConfig.Source = *parsedSource - workspaceConfig.DevContainerSource = devContainerSource } // save machine config @@ -190,6 +163,49 @@ func (cmd *ImportCmd) importWorkspace( return nil } +// extractWorkspaceData creates workspaceID's workspace dir and extracts the +// base64-encoded, archived workspace data into it. +func extractWorkspaceData(devsyConfig *config.Config, workspaceID, data string) error { + workspaceDir, err := provider.GetWorkspaceDir(devsyConfig.DefaultContext, workspaceID) + if err != nil { + return fmt.Errorf("get workspace dir: %w", err) + } + + // #nosec G301 -- TODO Consider using a more secure permission setting and ownership if needed. + if err := os.MkdirAll(workspaceDir, 0o755); err != nil { + return fmt.Errorf("create workspace dir: %w", err) + } + + decoded, err := base64.RawStdEncoding.DecodeString(data) + if err != nil { + return fmt.Errorf("decode workspace data: %w", err) + } + + if err := extract.Extract(bytes.NewReader(decoded), workspaceDir); err != nil { + return fmt.Errorf("extract workspace data: %w", err) + } + return nil +} + +// applySnapshotSource resolves snapshotRef into a workspace source and dev +// container source, applying both to workspaceConfig. +func applySnapshotSource(workspaceConfig *provider.Workspace, snapshotRef string) error { + sourceStr, devContainerSource, err := snapshotpkg.RestoreComposition(snapshotRef) + if err != nil { + return fmt.Errorf("parse snapshot ref: %w", err) + } + parsedSource := provider.ParseWorkspaceSource(sourceStr) + if parsedSource == nil { + return fmt.Errorf( + "compose workspace source from snapshot ref: unexpected source %q", + sourceStr, + ) + } + workspaceConfig.Source = *parsedSource + workspaceConfig.DevContainerSource = devContainerSource + return nil +} + func (cmd *ImportCmd) importMachine( devsyConfig *config.Config, exportConfig *provider.ExportConfig, diff --git a/cmd/workspace/up/up.go b/cmd/workspace/up/up.go index a921072c4..57ab38a4b 100644 --- a/cmd/workspace/up/up.go +++ b/cmd/workspace/up/up.go @@ -20,6 +20,7 @@ import ( "github.com/devsy-org/devsy/pkg/output" provider2 "github.com/devsy-org/devsy/pkg/provider" "github.com/devsy-org/devsy/pkg/status" + "github.com/devsy-org/devsy/pkg/task" "github.com/devsy-org/devsy/pkg/telemetry" "github.com/devsy-org/devsy/pkg/util" "github.com/devsy-org/devsy/pkg/workspace" @@ -241,16 +242,9 @@ func (cmd *UpCmd) Run( out := cmd.stdout() cmd.statusReporter = newStatusReporter(emitJSON, out) - t, err := cmd.openTask() + t, err := cmd.setUpTask(client, emitJSON, out) if err != nil { - return reportErr(err, emitJSON, out) - } - if t != nil { - cmd.statusReporter = status.Tee(cmd.statusReporter, t.Reporter()) - if err := t.SetWorkspaceID(client.Workspace()); err != nil { - failTask(t, err) - return reportErr(err, emitJSON, out) - } + return err } wctx, err := cmd.executeDevsyUp(ctx, devsyConfig, client) @@ -278,6 +272,27 @@ func (cmd *UpCmd) Run( return nil } +// setUpTask opens the run's task (if any), tees the status reporter into it, +// and records the workspace ID on it. +func (cmd *UpCmd) setUpTask( + client client2.BaseWorkspaceClient, + emitJSON bool, + out io.Writer, +) (*task.Task, error) { + t, err := cmd.openTask() + if err != nil { + return nil, reportErr(err, emitJSON, out) + } + if t != nil { + cmd.statusReporter = status.Tee(cmd.statusReporter, t.Reporter()) + if err := t.SetWorkspaceID(client.Workspace()); err != nil { + failTask(t, err) + return nil, reportErr(err, emitJSON, out) + } + } + return t, nil +} + // reporter falls back to a no-op when Run hasn't set one yet. func (cmd *UpCmd) reporter() status.Reporter { if cmd.statusReporter == nil { diff --git a/cmd/workspace/up/up_client.go b/cmd/workspace/up/up_client.go index 45e3ac92c..fe8012b1b 100644 --- a/cmd/workspace/up/up_client.go +++ b/cmd/workspace/up/up_client.go @@ -94,15 +94,25 @@ func (cmd *UpCmd) prepareClient( if err != nil { return nil, err } - if !cmd.Platform.Enabled { - proInstance := workspace2.GetProInstance(devsyConfig, client.Provider()) - if err := workspace2.CheckProviderUpdate(ctx, devsyConfig, proInstance); err != nil { - return nil, err - } + if err := cmd.checkProviderUpdate(ctx, devsyConfig, client); err != nil { + return nil, err } return client, nil } +// checkProviderUpdate checks for a provider update, unless running in platform mode. +func (cmd *UpCmd) checkProviderUpdate( + ctx context.Context, + devsyConfig *config.Config, + client client2.BaseWorkspaceClient, +) error { + if cmd.Platform.Enabled { + return nil + } + proInstance := workspace2.GetProInstance(devsyConfig, client.Provider()) + return workspace2.CheckProviderUpdate(ctx, devsyConfig, proInstance) +} + // ensureArgsForFromSnapshot returns args unchanged unless --from-snapshot is // set and args is empty, in which case it synthesizes a placeholder arg. // resolveWorkspace only takes its create-new-workspace path when args is diff --git a/pkg/agent/delivery/factory.go b/pkg/agent/delivery/factory.go index efdf0a23d..3b7f2cba4 100644 --- a/pkg/agent/delivery/factory.go +++ b/pkg/agent/delivery/factory.go @@ -25,40 +25,48 @@ type FactoryOptions struct { } func NewAgentDelivery(opts FactoryOptions) AgentDelivery { - switch driverType := opts.WorkspaceConfig.Agent.Driver; { - case driverType == provider.CustomDriver: - return legacyShellDelivery(opts, "custom driver") - - case driverType == provider.KubernetesDriver: - return kubernetesDelivery(opts) - - case driverType == provider.AppleDriver: - // Shell delivery launches the agent in one exec, which keeps the VM - // alive; it is the supported mechanism here, not a deprecated fallback. - log.Debugf("using shell-based delivery for apple driver") - return &LegacyShellDelivery{ExecFunc: opts.ExecFunc, DownloadURL: ""} - - case driverType == provider.MicrosandboxDriver: - // Stream the agent binary over the SDK's guest exec (as kubernetes does); - // fall back to shell delivery when the driver exposes no argv exec. - if opts.PodExec == nil { - return legacyShellDelivery(opts, "microsandbox argv exec unavailable") - } - log.Debugf("using stream delivery (exec stream) for microsandbox") - return &KubernetesDelivery{Exec: opts.PodExec} + driverType := opts.WorkspaceConfig.Agent.Driver + if d := namedDriverDelivery(driverType, opts); d != nil { + return d + } - case opts.IsRemoteDocker: + if opts.IsRemoteDocker { log.Debugf("using remote docker delivery (docker cp)") return remoteDockerDelivery(opts) + } - case driverType == "" || driverType == provider.DockerDriver: + if driverType == "" || driverType == provider.DockerDriver { return dockerDelivery(opts) + } + + return legacyShellDelivery(opts, fmt.Sprintf("driver: %s", driverType)) +} +// namedDriverDelivery returns the delivery strategy for driver types that +// dispatch on an exact name match, or nil if driverType matches none of them. +func namedDriverDelivery(driverType string, opts FactoryOptions) AgentDelivery { + switch driverType { + case provider.CustomDriver: + return legacyShellDelivery(opts, "custom driver") + case provider.KubernetesDriver: + return kubernetesDelivery(opts) + case provider.AppleDriver: + return appleDelivery(opts) + case provider.MicrosandboxDriver: + return microsandboxDelivery(opts) default: - return legacyShellDelivery(opts, fmt.Sprintf("driver: %s", driverType)) + return nil } } +// appleDelivery launches the agent in one shell exec, which keeps the VM +// alive; it is the supported mechanism for this driver, not a deprecated +// fallback. +func appleDelivery(opts FactoryOptions) AgentDelivery { + log.Debugf("using shell-based delivery for apple driver") + return &LegacyShellDelivery{ExecFunc: opts.ExecFunc, DownloadURL: ""} +} + func kubernetesDelivery(opts FactoryOptions) AgentDelivery { if opts.PodExec == nil { return legacyShellDelivery(opts, "kubernetes pod exec unavailable") @@ -67,6 +75,17 @@ func kubernetesDelivery(opts FactoryOptions) AgentDelivery { return &KubernetesDelivery{Exec: opts.PodExec} } +// microsandboxDelivery streams the agent binary over the SDK's guest exec +// (as kubernetes does), falling back to shell delivery when the driver +// exposes no argv exec. +func microsandboxDelivery(opts FactoryOptions) AgentDelivery { + if opts.PodExec == nil { + return legacyShellDelivery(opts, "microsandbox argv exec unavailable") + } + log.Debugf("using stream delivery (exec stream) for microsandbox") + return &KubernetesDelivery{Exec: opts.PodExec} +} + func dockerDelivery(opts FactoryOptions) AgentDelivery { if isDockerLocal(opts.DockerCommand) { log.Debugf("using local docker delivery (named volume)") diff --git a/pkg/provider/parse.go b/pkg/provider/parse.go index 4c47718cb..18d95d306 100644 --- a/pkg/provider/parse.go +++ b/pkg/provider/parse.go @@ -272,16 +272,21 @@ func validateStandardProvider(config *ProviderConfig) error { return validateExecCommands(config) } +var validAgentDrivers = map[string]bool{ + "": true, + CustomDriver: true, + DockerDriver: true, + KubernetesDriver: true, + AppleDriver: true, + MicrosandboxDriver: true, +} + func validateAgentDriver(config *ProviderConfig) error { if templatedValueRegex.MatchString(config.Agent.Driver) { return nil } - if config.Agent.Driver != "" && config.Agent.Driver != CustomDriver && - config.Agent.Driver != DockerDriver && - config.Agent.Driver != KubernetesDriver && - config.Agent.Driver != AppleDriver && - config.Agent.Driver != MicrosandboxDriver { + if !validAgentDrivers[config.Agent.Driver] { return fmt.Errorf( "agent.driver can only be docker, kubernetes, apple, microsandbox or custom", ) diff --git a/pkg/ssh/forward.go b/pkg/ssh/forward.go index 34176f63f..9fd4b9a34 100644 --- a/pkg/ssh/forward.go +++ b/pkg/ssh/forward.go @@ -141,22 +141,7 @@ func portForwarding( } }() - if client != nil { - transportClosed := make(chan struct{}) - go func() { - if werr := client.Wait(); werr != nil { - log.Debugf("ssh transport closed on %s: %v", srcAddr, werr) - } - close(transportClosed) - }() - go func() { - select { - case <-done: - case <-transportClosed: - cancel(ErrTransportClosed) - } - }() - } + watchTransportClosed(client, done, srcAddr, func() { cancel(ErrTransportClosed) }) counter := newConnectionCounter(fwdCtx, exitAfterTimeout, func() { log.Infof( @@ -191,6 +176,31 @@ func portForwarding( } } +// watchTransportClosed spawns a goroutine that waits for client's transport +// to close and invokes onClosed, unless done fires first. A nil client is a +// no-op. +func watchTransportClosed( + client *ssh.Client, done <-chan struct{}, srcAddr string, onClosed func(), +) { + if client == nil { + return + } + transportClosed := make(chan struct{}) + go func() { + if werr := client.Wait(); werr != nil { + log.Debugf("ssh transport closed on %s: %v", srcAddr, werr) + } + close(transportClosed) + }() + go func() { + select { + case <-done: + case <-transportClosed: + onClosed() + } + }() +} + func forward( localConn net.Conn, client *ssh.Client, From 0828b88b13ab68a9efcb51301b749124a731e4cc Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 15:14:07 -0500 Subject: [PATCH 2/2] style: cleanup comments --- cmd/internal/agentcontainer/setup.go | 5 ----- cmd/provider/set_source.go | 3 --- cmd/workspace/import.go | 2 +- 3 files changed, 1 insertion(+), 9 deletions(-) diff --git a/cmd/internal/agentcontainer/setup.go b/cmd/internal/agentcontainer/setup.go index 5683fbe5d..c0db3a416 100644 --- a/cmd/internal/agentcontainer/setup.go +++ b/cmd/internal/agentcontainer/setup.go @@ -425,11 +425,6 @@ func (cmd *SetupContainerCmd) parseWorkspaceAndSetupInfo() (*provider2.Container func (cmd *SetupContainerCmd) syncMounts(sctx *setupContext) error { mounts := config.GetMounts(sctx.setupInfo) - - // Snapshot restore runs regardless of the StreamMounts flag: StreamMounts - // only gates the legacy host-streaming path (forced true for non-docker - // drivers), but a snapshot-sourced workspace needs its volumes restored - // on every driver, docker included. if sctx.workspaceInfo.Source.Snapshot != "" { return restoreSnapshotMounts(sctx, mounts) } diff --git a/cmd/provider/set_source.go b/cmd/provider/set_source.go index 3081ba473..3db9be91e 100644 --- a/cmd/provider/set_source.go +++ b/cmd/provider/set_source.go @@ -101,9 +101,6 @@ func (cmd *SetSourceCmd) activateProvider( providerConfig *provider.ProviderConfig, reporter status.Reporter, ) error { - // Preserve previously user-provided values (default DiscardPriorValues=false). - // The resolver prunes keys absent from the new schema and re-resolves values - // that fail validation, so stale data cannot leak through this path. if err := ConfigureProvider(ctx, ProviderOptionsConfig{ Provider: providerConfig, ContextName: devsyConfig.DefaultContext, diff --git a/cmd/workspace/import.go b/cmd/workspace/import.go index 883677355..4776dce0c 100644 --- a/cmd/workspace/import.go +++ b/cmd/workspace/import.go @@ -171,7 +171,7 @@ func extractWorkspaceData(devsyConfig *config.Config, workspaceID, data string) return fmt.Errorf("get workspace dir: %w", err) } - // #nosec G301 -- TODO Consider using a more secure permission setting and ownership if needed. + // #nosec G301 if err := os.MkdirAll(workspaceDir, 0o755); err != nil { return fmt.Errorf("create workspace dir: %w", err) }