From 8972eec70c128b023796ce8361cef2e0635b0f9b Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 22:19:46 +0000 Subject: [PATCH 1/3] fix(lint): resolve all nestif findings nestif is on by default once enabled, same as funcorder/cyclop -- no separate opt-in setting. Fixes all 5 pre-existing violations by extracting the deeply-nested block into a well-named helper: - pkg/daemon/platform/local_server.go: watchPlatform's nested if err != nil -> updatePlatformAuthStatus - pkg/devcontainer/delete.go: Delete's non-compose branch -> stopAndDeleteContainer - pkg/provider/workspace.go: ParseWorkspaceSource's git-prefix branch -> parseGitWorkspaceSource - pkg/ssh/server/ssh.go: getCommand's su/shell argv construction -> buildSuArgs, buildShellArgs - pkg/workspace/id.go: ToID's two top-level branches -> idFromPROrBranch, idFromRepoPath (preserves the pre-existing dead len(splitted)==2 check inside idFromRepoPath -- that branch is unreachable in both the old and new code, unrelated to this fix) All extractions are behavior-preserving; verified against existing test coverage (pkg/workspace/id_test.go's TestToID exercises every branch, pkg/provider's TestParseWorkspaceSource_GitURLs covers the git path). No logic changes. --- pkg/daemon/platform/local_server.go | 35 +++++++++------ pkg/devcontainer/delete.go | 27 ++++++------ pkg/provider/workspace.go | 29 ++++++++----- pkg/ssh/server/ssh.go | 64 +++++++++++++--------------- pkg/workspace/id.go | 66 +++++++++++++++++------------ 5 files changed, 121 insertions(+), 100 deletions(-) diff --git a/pkg/daemon/platform/local_server.go b/pkg/daemon/platform/local_server.go index c0b68cfa2..54d2a537f 100644 --- a/pkg/daemon/platform/local_server.go +++ b/pkg/daemon/platform/local_server.go @@ -180,20 +180,7 @@ func (l *localServer) watchPlatform(stopChan <-chan struct{}) error { ManagementV1(). Selves(). Create(context.Background(), &managementv1.Self{}, metav1.CreateOptions{}) - l.platformStatus.mu.Lock() - if err != nil { - if IsAccessKeyNotFound(err) { - log.Warnf("client not authenticated: %s", err) - l.platformStatus.authenticated = false - } else { - log.Errorf("failed to create self: %v", err) - } - } else { - // We don't want to be too restrictive in case the error - // is transient and doesn't impact existing connections - l.platformStatus.authenticated = true - } - l.platformStatus.mu.Unlock() + l.updatePlatformAuthStatus(err) } select { @@ -204,6 +191,26 @@ func (l *localServer) watchPlatform(stopChan <-chan struct{}) error { } } +// updatePlatformAuthStatus records the platform authentication status from a +// self-creation attempt's result. +func (l *localServer) updatePlatformAuthStatus(err error) { + l.platformStatus.mu.Lock() + defer l.platformStatus.mu.Unlock() + + if err == nil { + // We don't want to be too restrictive in case the error + // is transient and doesn't impact existing connections + l.platformStatus.authenticated = true + return + } + if IsAccessKeyNotFound(err) { + log.Warnf("client not authenticated: %s", err) + l.platformStatus.authenticated = false + return + } + log.Errorf("failed to create self: %v", err) +} + func (l *localServer) health(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) } diff --git a/pkg/devcontainer/delete.go b/pkg/devcontainer/delete.go index 2b1b61adf..54df65f0c 100644 --- a/pkg/devcontainer/delete.go +++ b/pkg/devcontainer/delete.go @@ -22,25 +22,22 @@ func (r *runner) Delete(ctx context.Context, options DeleteOptions) error { log.Infof("deleting devcontainer: devcontainerID=%s", containerDetails.ID) if isDockerCompose, projectName := getDockerComposeProject(containerDetails); isDockerCompose { - err = r.deleteDockerCompose(ctx, projectName, options.RemoveVolumes) - if err != nil { - return err - } - } else { - if strings.ToLower(containerDetails.State.Status) == "running" { - err = r.driver.StopDevContainer(ctx, r.id) - if err != nil { - return err - } - } + return r.deleteDockerCompose(ctx, projectName, options.RemoveVolumes) + } + return r.stopAndDeleteContainer(ctx, containerDetails) +} - err = r.driver.DeleteDevContainer(ctx, r.id) - if err != nil { +// stopAndDeleteContainer stops containerDetails' devcontainer if it's +// running, then deletes it. +func (r *runner) stopAndDeleteContainer( + ctx context.Context, containerDetails *config.ContainerDetails, +) error { + if strings.ToLower(containerDetails.State.Status) == "running" { + if err := r.driver.StopDevContainer(ctx, r.id); err != nil { return err } } - - return nil + return r.driver.DeleteDevContainer(ctx, r.id) } func (r *runner) cleanupDeliveryVolume(ctx context.Context) { diff --git a/pkg/provider/workspace.go b/pkg/provider/workspace.go index 11a6d73d5..da3ed2d1f 100644 --- a/pkg/provider/workspace.go +++ b/pkg/provider/workspace.go @@ -471,17 +471,7 @@ func (w WorkspaceSource) gitSourceType() string { func ParseWorkspaceSource(source string) *WorkspaceSource { if after, ok := strings.CutPrefix(source, WorkspaceSourceGit); ok { - info := git.NormalizeRepository(after) - if !isPlausibleGitSource(info.Repository) { - return nil - } - return &WorkspaceSource{ - GitRepository: info.Repository, - GitPRReference: info.PR, - GitBranch: info.Branch, - GitCommit: info.Commit, - GitSubPath: info.SubPath, - } + return parseGitWorkspaceSource(after) } else if after, ok := strings.CutPrefix(source, WorkspaceSourceLocal); ok { after = util.ExpandTilde(after) return &WorkspaceSource{ @@ -504,6 +494,23 @@ func ParseWorkspaceSource(source string) *WorkspaceSource { return nil } +// parseGitWorkspaceSource builds a WorkspaceSource from the portion of a +// source string following the git prefix, or nil if it's not a plausible +// git repository. +func parseGitWorkspaceSource(after string) *WorkspaceSource { + info := git.NormalizeRepository(after) + if !isPlausibleGitSource(info.Repository) { + return nil + } + return &WorkspaceSource{ + GitRepository: info.Repository, + GitPRReference: info.PR, + GitBranch: info.Branch, + GitCommit: info.Commit, + GitSubPath: info.SubPath, + } +} + var gitURLSchemes = map[string]bool{"http": true, "https": true, "ssh": true, "git": true} // isPlausibleGitSource returns true when s looks like a git repository diff --git a/pkg/ssh/server/ssh.go b/pkg/ssh/server/ssh.go index 5e55fe6a1..edafe27b3 100644 --- a/pkg/ssh/server/ssh.go +++ b/pkg/ssh/server/ssh.go @@ -366,41 +366,11 @@ func (s *server) getCommand(sess ssh.Session, isPty bool) *exec.Cmd { // has user set? if user != "" { - args := []string{} - - // is pty? - if isPty { - args = append(args, "-") - } - - // add user - args = append(args, sess.User()) - - // is there a command? - if len(sess.RawCommand()) > 0 { - args = append(args, "-c", sess.RawCommand()) - } - - cmd = exec.Command( - "su", - args...) // #nosec G204 -- args built from session request in the ssh server + //nolint:gosec // G204: args built from session request in the ssh server + cmd = exec.Command("su", buildSuArgs(sess, isPty)...) } else { - args := []string{} - args = append(args, s.shell[1:]...) - if isPty { - args = append(args, "-l") - } - - if len(sess.RawCommand()) == 0 { - cmd = exec.Command( - s.shell[0], - args...) // #nosec G204 -- shell configured by the ssh server, not user input - } else { - args = append(args, "-c", sess.RawCommand()) - cmd = exec.Command( - s.shell[0], - args...) // #nosec G204 -- shell configured by the ssh server, not user input - } + //nolint:gosec // G204: shell configured by the ssh server, not user input + cmd = exec.Command(s.shell[0], buildShellArgs(s.shell[1:], sess, isPty)...) } cmd.Dir = findWorkdir(s.workdir, user) @@ -409,6 +379,32 @@ func (s *server) getCommand(sess ssh.Session, isPty bool) *exec.Cmd { return cmd } +// buildSuArgs builds the "su" argv for running sess's command as sess.User(). +func buildSuArgs(sess ssh.Session, isPty bool) []string { + args := []string{} + if isPty { + args = append(args, "-") + } + args = append(args, sess.User()) + if len(sess.RawCommand()) > 0 { + args = append(args, "-c", sess.RawCommand()) + } + return args +} + +// buildShellArgs builds the argv for running sess's command via the +// server's configured shell, given its args beyond the executable itself. +func buildShellArgs(shellArgs []string, sess ssh.Session, isPty bool) []string { + args := append([]string{}, shellArgs...) + if isPty { + args = append(args, "-l") + } + if len(sess.RawCommand()) > 0 { + args = append(args, "-c", sess.RawCommand()) + } + return args +} + func (s *server) configureAgent(sess ssh.Session, cmd *exec.Cmd) (func(), bool, error) { if s.reuseSock != "" { // openvscode backhaul / explicit shared-socket mode: keep the diff --git a/pkg/workspace/id.go b/pkg/workspace/id.go index cf3db9eaa..73933a9f8 100644 --- a/pkg/workspace/id.go +++ b/pkg/workspace/id.go @@ -20,33 +20,9 @@ func ToID(str string) string { str = strings.ToLower(filepath.ToSlash(str)) splitted := strings.Split(str, "@") if len(splitted) == 2 { - // 1. Check if PR was specified - if prReferenceRegEx.MatchString(str) { - str = prReferenceRegEx.ReplaceAllStringFunc(splitted[1], git.GetIDForPR) - } else { - // 2. Check if a branch name has been specified, if so use this for the ID - str = strings.TrimSuffix(splitted[1], ".git") - // Check if branch name matches expected regex - if !branchRegEx.MatchString(str) { - str = splitted[0] - } - } + str = idFromPROrBranch(str, splitted) } else { - // Ensure we don't have a single trailing slash - str = strings.TrimSuffix(str, "/") - // 3. If not, then parse the repo name as ID - index := strings.LastIndex(str, "/") - if index != -1 { - str = str[index+1:] - - // remove a potential tag / branch name - if len(splitted) == 2 && !branchRegEx.MatchString(splitted[1]) { - str = splitted[0] - } - - // remove .git if there is it - str = strings.TrimSuffix(str, ".git") - } + str = idFromRepoPath(str, splitted) } str = workspaceIDRegEx2.ReplaceAllString(workspaceIDRegEx1.ReplaceAllString(str, "-"), "") @@ -56,3 +32,41 @@ func ToID(str string) string { return strings.Trim(str, "-") } + +// idFromPROrBranch derives the ID from the "@..." suffix of a "repo@ref" +// string: a PR reference if str matches one, otherwise a branch name if it +// looks like one, falling back to the repo part (splitted[0]). +func idFromPROrBranch(str string, splitted []string) string { + // 1. Check if PR was specified + if prReferenceRegEx.MatchString(str) { + return prReferenceRegEx.ReplaceAllStringFunc(splitted[1], git.GetIDForPR) + } + // 2. Check if a branch name has been specified, if so use this for the ID + branch := strings.TrimSuffix(splitted[1], ".git") + // Check if branch name matches expected regex + if !branchRegEx.MatchString(branch) { + return splitted[0] + } + return branch +} + +// idFromRepoPath derives the ID from the final path segment of a repo URL +// with no recognized "@ref" suffix, stripping any trailing ".git". +func idFromRepoPath(str string, splitted []string) string { + // Ensure we don't have a single trailing slash + str = strings.TrimSuffix(str, "/") + // 3. If not, then parse the repo name as ID + index := strings.LastIndex(str, "/") + if index == -1 { + return str + } + str = str[index+1:] + + // remove a potential tag / branch name + if len(splitted) == 2 && !branchRegEx.MatchString(splitted[1]) { + str = splitted[0] + } + + // remove .git if there is it + return strings.TrimSuffix(str, ".git") +} From deb7b3454bebce29a8fff836ff06c1b3e5820ac2 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 22:23:16 +0000 Subject: [PATCH 2/3] fix(lint): remove dead code from nestif extraction idFromRepoPath is only reached from ToID's else branch (len(splitted) != 2), so its len(splitted) == 2 check could never be true. Drop the dead branch and the now-unused splitted parameter. --- pkg/workspace/id.go | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/pkg/workspace/id.go b/pkg/workspace/id.go index 73933a9f8..81e9b7a9b 100644 --- a/pkg/workspace/id.go +++ b/pkg/workspace/id.go @@ -22,7 +22,7 @@ func ToID(str string) string { if len(splitted) == 2 { str = idFromPROrBranch(str, splitted) } else { - str = idFromRepoPath(str, splitted) + str = idFromRepoPath(str) } str = workspaceIDRegEx2.ReplaceAllString(workspaceIDRegEx1.ReplaceAllString(str, "-"), "") @@ -52,7 +52,7 @@ func idFromPROrBranch(str string, splitted []string) string { // idFromRepoPath derives the ID from the final path segment of a repo URL // with no recognized "@ref" suffix, stripping any trailing ".git". -func idFromRepoPath(str string, splitted []string) string { +func idFromRepoPath(str string) string { // Ensure we don't have a single trailing slash str = strings.TrimSuffix(str, "/") // 3. If not, then parse the repo name as ID @@ -62,11 +62,6 @@ func idFromRepoPath(str string, splitted []string) string { } str = str[index+1:] - // remove a potential tag / branch name - if len(splitted) == 2 && !branchRegEx.MatchString(splitted[1]) { - str = splitted[0] - } - // remove .git if there is it return strings.TrimSuffix(str, ".git") } From 89dfd99ea45c5e2b6118285948d115e5ec8065ee Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 18:06:42 -0500 Subject: [PATCH 3/3] style: clean comments --- pkg/daemon/platform/local_server.go | 2 -- pkg/devcontainer/delete.go | 4 ++-- pkg/ssh/server/ssh.go | 1 - pkg/workspace/id.go | 9 +-------- 4 files changed, 3 insertions(+), 13 deletions(-) diff --git a/pkg/daemon/platform/local_server.go b/pkg/daemon/platform/local_server.go index 54d2a537f..226935e96 100644 --- a/pkg/daemon/platform/local_server.go +++ b/pkg/daemon/platform/local_server.go @@ -198,8 +198,6 @@ func (l *localServer) updatePlatformAuthStatus(err error) { defer l.platformStatus.mu.Unlock() if err == nil { - // We don't want to be too restrictive in case the error - // is transient and doesn't impact existing connections l.platformStatus.authenticated = true return } diff --git a/pkg/devcontainer/delete.go b/pkg/devcontainer/delete.go index 54df65f0c..8c5ab9993 100644 --- a/pkg/devcontainer/delete.go +++ b/pkg/devcontainer/delete.go @@ -27,8 +27,8 @@ func (r *runner) Delete(ctx context.Context, options DeleteOptions) error { return r.stopAndDeleteContainer(ctx, containerDetails) } -// stopAndDeleteContainer stops containerDetails' devcontainer if it's -// running, then deletes it. +// stopAndDeleteContainer stops containerDetails' devcontainer if it +// is running, then deletes it. func (r *runner) stopAndDeleteContainer( ctx context.Context, containerDetails *config.ContainerDetails, ) error { diff --git a/pkg/ssh/server/ssh.go b/pkg/ssh/server/ssh.go index edafe27b3..f7b0eab59 100644 --- a/pkg/ssh/server/ssh.go +++ b/pkg/ssh/server/ssh.go @@ -364,7 +364,6 @@ func (s *server) getCommand(sess ssh.Session, isPty bool) *exec.Cmd { user = "" } - // has user set? if user != "" { //nolint:gosec // G204: args built from session request in the ssh server cmd = exec.Command("su", buildSuArgs(sess, isPty)...) diff --git a/pkg/workspace/id.go b/pkg/workspace/id.go index 81e9b7a9b..e010a4956 100644 --- a/pkg/workspace/id.go +++ b/pkg/workspace/id.go @@ -35,15 +35,12 @@ func ToID(str string) string { // idFromPROrBranch derives the ID from the "@..." suffix of a "repo@ref" // string: a PR reference if str matches one, otherwise a branch name if it -// looks like one, falling back to the repo part (splitted[0]). +// looks like one, falling back to the repo part. func idFromPROrBranch(str string, splitted []string) string { - // 1. Check if PR was specified if prReferenceRegEx.MatchString(str) { return prReferenceRegEx.ReplaceAllStringFunc(splitted[1], git.GetIDForPR) } - // 2. Check if a branch name has been specified, if so use this for the ID branch := strings.TrimSuffix(splitted[1], ".git") - // Check if branch name matches expected regex if !branchRegEx.MatchString(branch) { return splitted[0] } @@ -53,15 +50,11 @@ func idFromPROrBranch(str string, splitted []string) string { // idFromRepoPath derives the ID from the final path segment of a repo URL // with no recognized "@ref" suffix, stripping any trailing ".git". func idFromRepoPath(str string) string { - // Ensure we don't have a single trailing slash str = strings.TrimSuffix(str, "/") - // 3. If not, then parse the repo name as ID index := strings.LastIndex(str, "/") if index == -1 { return str } str = str[index+1:] - - // remove .git if there is it return strings.TrimSuffix(str, ".git") }