address PR comments

coder · johnstcn · Feb 28, 2025 · Feb 26, 2025 · Feb 27, 2025 · Feb 27, 2025
commit 7437e49b7c30cb946922271ffd1be66da7c2b282
diff --git a/agent/agent.go b/agent/agent.go
@@ -91,8 +91,8 @@ type Options struct {
 	Execer                       agentexec.Execer
 	ContainerLister              agentcontainers.Lister
 
-	ExperimentalContainersEnabled bool
-	ExperimentalConnectionReports bool
+	ExperimentalConnectionReports    bool
+	ExperimentalDevcontainersEnabled bool
 }
 
 type Client interface {
@@ -156,7 +156,7 @@ func New(options Options) Agent {
 		options.Execer = agentexec.DefaultExecer
 	}
 	if options.ContainerLister == nil {
-		options.ContainerLister = agentcontainers.NewDocker(options.Execer)
+		options.ContainerLister = agentcontainers.NoopLister{}
 	}
 
 	hardCtx, hardCancel := context.WithCancel(context.Background())
@@ -195,7 +195,7 @@ func New(options Options) Agent {
 		execer:             options.Execer,
 		lister:             options.ContainerLister,
 
-		experimentalDevcontainersEnabled: options.ExperimentalContainersEnabled,
+		experimentalDevcontainersEnabled: options.ExperimentalDevcontainersEnabled,
 		experimentalConnectionReports:    options.ExperimentalConnectionReports,
 	}
 	// Initially, we have a closed channel, reflecting the fact that we are not initially connected.
@@ -308,7 +308,7 @@ func (a *agent) init() {
 			return a.reportConnection(id, connectionType, ip)
 		},
 
-		ExperimentalContainersEnabled: a.experimentalDevcontainersEnabled,
+		ExperimentalDevContainersEnabled: a.experimentalDevcontainersEnabled,
 	})
 	if err != nil {
 		panic(err)
@@ -337,7 +337,7 @@ func (a *agent) init() {
 		a.metrics.connectionsTotal, a.metrics.reconnectingPTYErrors,
 		a.reconnectingPTYTimeout,
 		func(s *reconnectingpty.Server) {
-			s.ExperimentalContainersEnabled = a.experimentalDevcontainersEnabled
+			s.ExperimentalDevcontainersEnabled = a.experimentalDevcontainersEnabled
 		},
 	)
 	go a.runLoop()

diff --git a/agent/agent_test.go b/agent/agent_test.go
@@ -1841,7 +1841,7 @@ func TestAgent_ReconnectingPTYContainer(t *testing.T) {
 
 	// nolint: dogsled
 	conn, _, _, _, _ := setupAgent(t, agentsdk.Manifest{}, 0, func(_ *agenttest.Client, o *agent.Options) {
-		o.ExperimentalContainersEnabled = true
+		o.ExperimentalDevcontainersEnabled = true
 	})
 	ac, err := conn.ReconnectingPTY(ctx, uuid.New(), 80, 80, "/bin/sh", func(arp *workspacesdk.AgentReconnectingPTYInit) {
 		arp.Container = ct.Container.ID

diff --git a/agent/agentssh/agentssh.go b/agent/agentssh/agentssh.go
@@ -115,7 +115,7 @@ type Config struct {
 	ReportConnection reportConnectionFunc
 	// Experimental: allow connecting to running containers if
 	// CODER_AGENT_DEVCONTAINERS_ENABLE=true.
-	ExperimentalContainersEnabled bool
+	ExperimentalDevContainersEnabled bool
 }
 
 type Server struct {
@@ -425,16 +425,19 @@ func (s *Server) sessionHandler(session ssh.Session) {
 	}
 
 	container, containerUser, env := extractContainerInfo(env)
-	s.logger.Debug(ctx, "container info",
-		slog.F("container", container),
-		slog.F("container_user", containerUser),
-	)
+	if container != "" {
+		s.logger.Debug(ctx, "container info",
+			slog.F("container", container),
+			slog.F("container_user", containerUser),
+		)
+	}
 
 	switch ss := session.Subsystem(); ss {
 	case "":
 	case "sftp":
-		if s.config.ExperimentalContainersEnabled && container != "" {
+		if s.config.ExperimentalDevContainersEnabled && container != "" {
 			closeCause("sftp not yet supported with containers")
+			_ = session.Exit(1)
 			return
 		}
 		err := s.sftpHandler(logger, session)
@@ -546,7 +549,7 @@ func (s *Server) sessionStart(logger slog.Logger, session ssh.Session, env []str
 
 	var ei usershell.EnvInfoer
 	var err error
-	if s.config.ExperimentalContainersEnabled && container != "" {
+	if s.config.ExperimentalDevContainersEnabled && container != "" {
 		ei, err = agentcontainers.EnvInfo(ctx, s.Execer, container, containerUser)
 		if err != nil {
 			s.metrics.sessionErrors.WithLabelValues(magicTypeLabel, ptyLabel, "container_env_info").Add(1)

diff --git a/agent/reconnectingpty/server.go b/agent/reconnectingpty/server.go
@@ -32,7 +32,7 @@ type Server struct {
 	reconnectingPTYs sync.Map
 	timeout          time.Duration
 
-	ExperimentalContainersEnabled bool
+	ExperimentalDevcontainersEnabled bool
 }
 
 // NewServer returns a new ReconnectingPTY server
@@ -187,7 +187,7 @@ func (s *Server) handleConn(ctx context.Context, logger slog.Logger, conn net.Co
 		}()
 
 		var ei usershell.EnvInfoer
-		if s.ExperimentalContainersEnabled && msg.Container != "" {
+		if s.ExperimentalDevcontainersEnabled && msg.Container != "" {
 			dei, err := agentcontainers.EnvInfo(ctx, s.commandCreator.Execer, msg.Container, msg.ContainerUser)
 			if err != nil {
 				return xerrors.Errorf("get container env info: %w", err)

diff --git a/cli/agent.go b/cli/agent.go
@@ -38,24 +38,24 @@ import (
 
 func (r *RootCmd) workspaceAgent() *serpent.Command {
 	var (
-		auth                 string
-		logDir               string
-		scriptDataDir        string
-		pprofAddress         string
-		noReap               bool
-		sshMaxTimeout        time.Duration
-		tailnetListenPort    int64
-		prometheusAddress    string
-		debugAddress         string
-		slogHumanPath        string
-		slogJSONPath         string
-		slogStackdriverPath  string
-		blockFileTransfer    bool
-		agentHeaderCommand   string
-		agentHeader          []string
-		devcontainersEnabled bool
-
-		experimentalConnectionReports bool
+		auth                string
+		logDir              string
+		scriptDataDir       string
+		pprofAddress        string
+		noReap              bool
+		sshMaxTimeout       time.Duration
+		tailnetListenPort   int64
+		prometheusAddress   string
+		debugAddress        string
+		slogHumanPath       string
+		slogJSONPath        string
+		slogStackdriverPath string
+		blockFileTransfer   bool
+		agentHeaderCommand  string
+		agentHeader         []string
+
+		experimentalConnectionReports    bool
+		experimentalDevcontainersEnabled bool
 	)
 	cmd := &serpent.Command{
 		Use:   "agent",
@@ -319,7 +319,7 @@ func (r *RootCmd) workspaceAgent() *serpent.Command {
 			}
 
 			var containerLister agentcontainers.Lister
-			if !devcontainersEnabled {
+			if !experimentalDevcontainersEnabled {
 				logger.Info(ctx, "agent devcontainer detection not enabled")
 				containerLister = &agentcontainers.NoopLister{}
 			} else {
@@ -358,8 +358,8 @@ func (r *RootCmd) workspaceAgent() *serpent.Command {
 				Execer:             execer,
 				ContainerLister:    containerLister,
 
-				ExperimentalContainersEnabled: devcontainersEnabled,
-				ExperimentalConnectionReports: experimentalConnectionReports,
+				ExperimentalDevcontainersEnabled: experimentalDevcontainersEnabled,
+				ExperimentalConnectionReports:    experimentalConnectionReports,
 			})
 
 			promHandler := agent.PrometheusMetricsHandler(prometheusRegistry, logger)
@@ -487,7 +487,7 @@ func (r *RootCmd) workspaceAgent() *serpent.Command {
 			Default:     "false",
 			Env:         "CODER_AGENT_DEVCONTAINERS_ENABLE",
 			Description: "Allow the agent to automatically detect running devcontainers.",
-			Value:       serpent.BoolOf(&devcontainersEnabled),
+			Value:       serpent.BoolOf(&experimentalDevcontainersEnabled),
 		},
 		{
 			Flag:        "experimental-connection-reports-enable",

diff --git a/cli/exp_rpty_test.go b/cli/exp_rpty_test.go
@@ -88,7 +88,7 @@ func TestExpRpty(t *testing.T) {
 		})
 
 		_ = agenttest.New(t, client.URL, agentToken, func(o *agent.Options) {
-			o.ExperimentalContainersEnabled = true
+			o.ExperimentalDevcontainersEnabled = true
 		})
 		_ = coderdtest.NewWorkspaceAgentWaiter(t, client, workspace.ID).Wait()
 

diff --git a/cli/ssh.go b/cli/ssh.go
@@ -78,7 +78,7 @@ func (r *RootCmd) ssh() *serpent.Command {
 		networkInfoDir      string
 		networkInfoInterval time.Duration
 
-		container     string
+		containerName string
 		containerUser string
 	)
 	client := new(codersdk.Client)
@@ -286,20 +286,31 @@ func (r *RootCmd) ssh() *serpent.Command {
 			}
 			conn.AwaitReachable(ctx)
 
-			if container != "" {
+			if containerName != "" {
 				cts, err := client.WorkspaceAgentListContainers(ctx, workspaceAgent.ID, nil)
 				if err != nil {
 					return xerrors.Errorf("list containers: %w", err)
 				}
+				if len(cts.Containers) == 0 {
+					cliui.Info(inv.Stderr, "No containers found!")
+					cliui.Info(inv.Stderr, "Tip: Agent container integration is experimental and not enabled by default.")
+					cliui.Info(inv.Stderr, "     To enable it, set CODER_AGENT_DEVCONTAINERS_ENABLE=true in your template.")
+					return nil
+				}
 				var found bool
 				for _, c := range cts.Containers {
-					if c.FriendlyName == container || c.ID == container {
+					if c.FriendlyName == containerName || c.ID == containerName {
 						found = true
 						break
 					}
 				}
 				if !found {
-					return xerrors.Errorf("container not found: %q", container)
+					availableContainers := make([]string, len(cts.Containers))
+					for i, c := range cts.Containers {
+						availableContainers[i] = c.FriendlyName
+					}
+					cliui.Errorf(inv.Stderr, "Container not found: %q\nAvailable containers: %v", containerName, availableContainers)
+					return nil
 				}
 			}
 
@@ -475,9 +486,9 @@ func (r *RootCmd) ssh() *serpent.Command {
 				}
 			}
 
-			if container != "" {
+			if containerName != "" {
 				for k, v := range map[string]string{
-					agentssh.ContainerEnvironmentVariable:     container,
+					agentssh.ContainerEnvironmentVariable:     containerName,
 					agentssh.ContainerUserEnvironmentVariable: containerUser,
 				} {
 					if err := sshSession.Setenv(k, v); err != nil {
@@ -630,15 +641,14 @@ func (r *RootCmd) ssh() *serpent.Command {
 			Flag:          "container",
 			FlagShorthand: "c",
 			Description:   "Specifies a container inside the workspace to connect to.",
-			Value:         serpent.StringOf(&container),
+			Value:         serpent.StringOf(&containerName),
 			Hidden:        true, // Hidden until this features is at least in beta.
 		},
 		{
-			Flag:          "container-user",
-			FlagShorthand: "u",
-			Description:   "When connecting to a container, specifies the user to connect as.",
-			Value:         serpent.StringOf(&containerUser),
-			Hidden:        true, // Hidden until this features is at least in beta.
+			Flag:        "container-user",
+			Description: "When connecting to a container, specifies the user to connect as.",
+			Value:       serpent.StringOf(&containerUser),
+			Hidden:      true, // Hidden until this features is at least in beta.
 		},
 		sshDisableAutostartOption(serpent.BoolOf(&disableAutostart)),
 	}

diff --git a/cli/ssh_test.go b/cli/ssh_test.go
@@ -35,6 +35,7 @@ import (
 	"golang.org/x/xerrors"
 
 	"github.com/coder/coder/v2/agent"
+	"github.com/coder/coder/v2/agent/agentcontainers"
 	"github.com/coder/coder/v2/agent/agentssh"
 	"github.com/coder/coder/v2/agent/agenttest"
 	agentproto "github.com/coder/coder/v2/agent/proto"
@@ -1959,7 +1960,8 @@ func TestSSH_Container(t *testing.T) {
 		})
 
 		_ = agenttest.New(t, client.URL, agentToken, func(o *agent.Options) {
-			o.ExperimentalContainersEnabled = true
+			o.ExperimentalDevcontainersEnabled = true
+			o.ContainerLister = agentcontainers.NewDocker(o.Execer)
 		})
 		_ = coderdtest.NewWorkspaceAgentWaiter(t, client, workspace.ID).Wait()
 
@@ -1985,14 +1987,22 @@ func TestSSH_Container(t *testing.T) {
 		ctx := testutil.Context(t, testutil.WaitShort)
 		client, workspace, agentToken := setupWorkspaceForAgent(t)
 		_ = agenttest.New(t, client.URL, agentToken, func(o *agent.Options) {
-			o.ExperimentalContainersEnabled = true
+			o.ExperimentalDevcontainersEnabled = true
+			o.ContainerLister = agentcontainers.NewDocker(o.Execer)
 		})
 		_ = coderdtest.NewWorkspaceAgentWaiter(t, client, workspace.ID).Wait()
 
 		inv, root := clitest.New(t, "ssh", workspace.Name, "-c", uuid.NewString())
 		clitest.SetupConfig(t, client, root)
-		err := inv.WithContext(ctx).Run()
-		require.ErrorContains(t, err, "container not found:")
+		ptty := ptytest.New(t).Attach(inv)
+
+		cmdDone := tGo(t, func() {
+			err := inv.WithContext(ctx).Run()
+			assert.NoError(t, err)
+		})
+
+		ptty.ExpectMatch("Container not found:")
+		<-cmdDone
 	})
 
 	t.Run("NotEnabled", func(t *testing.T) {
@@ -2005,8 +2015,16 @@ func TestSSH_Container(t *testing.T) {
 
 		inv, root := clitest.New(t, "ssh", workspace.Name, "-c", uuid.NewString())
 		clitest.SetupConfig(t, client, root)
-		err := inv.WithContext(ctx).Run()
-		require.ErrorContains(t, err, "container not found:")
+		ptty := ptytest.New(t).Attach(inv)
+
+		cmdDone := tGo(t, func() {
+			err := inv.WithContext(ctx).Run()
+			assert.NoError(t, err)
+		})
+
+		ptty.ExpectMatch("No containers found!")
+		ptty.ExpectMatch("Tip: Agent container integration is experimental and not enabled by default.")
+		<-cmdDone
 	})
 }