mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix(agent): unify working directory resolution (#26102)
agentssh's CommandEnv, sftpHandler, and agentproc each resolved the session working directory on their own and had drifted: sftpHandler used the configured directory without checking it exists and bypassed the injected EnvInfoer, while the others stat-checked and fell back to home. Home and shell lookups could also skip the EnvInfoer seam through the exported usershell.HomeDir and Get. Resolve through a single usershell.ResolveWorkingDirectory and confine host home and shell lookups to usershell, so SSH sessions, the process API, and tests can no longer diverge and the injected environment is always honored. This also fixes SFTP landing in a configured directory that no longer exists. Refs coder/coder#26099
This commit is contained in:
@@ -11,6 +11,7 @@ import (
|
||||
|
||||
"github.com/go-chi/chi/v5"
|
||||
"github.com/google/uuid"
|
||||
"github.com/spf13/afero"
|
||||
|
||||
"cdr.dev/slog/v3"
|
||||
"github.com/coder/coder/v2/agent/agentchat"
|
||||
@@ -37,10 +38,10 @@ type API struct {
|
||||
}
|
||||
|
||||
// NewAPI creates a new process API handler.
|
||||
func NewAPI(logger slog.Logger, execer agentexec.Execer, pathStore *agentgit.PathStore, envInfo usershell.EnvInfoer, updateEnv func(current []string) (updated []string, err error), workingDir func() string) *API {
|
||||
func NewAPI(logger slog.Logger, execer agentexec.Execer, fs afero.Fs, pathStore *agentgit.PathStore, envInfo usershell.EnvInfoer, updateEnv func(current []string) (updated []string, err error), workingDir func() string) *API {
|
||||
return &API{
|
||||
logger: logger,
|
||||
manager: newManager(logger, execer, envInfo, updateEnv, workingDir),
|
||||
manager: newManager(logger, execer, fs, envInfo, updateEnv, workingDir),
|
||||
pathStore: pathStore,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -138,7 +138,7 @@ func newTestAPIWithOptions(t *testing.T, updateEnv func([]string) ([]string, err
|
||||
logger := slogtest.Make(t, &slogtest.Options{
|
||||
IgnoreErrors: true,
|
||||
}).Leveled(slog.LevelDebug)
|
||||
api := agentproc.NewAPI(logger, agentexec.DefaultExecer, nil, nil, updateEnv, workingDir)
|
||||
api := agentproc.NewAPI(logger, agentexec.DefaultExecer, nil, nil, nil, updateEnv, workingDir)
|
||||
t.Cleanup(func() {
|
||||
_ = api.Close()
|
||||
})
|
||||
@@ -153,7 +153,7 @@ func newTestAPIWithEnvInfo(t *testing.T, workingDir func() string, envInfo users
|
||||
logger := slogtest.Make(t, &slogtest.Options{
|
||||
IgnoreErrors: true,
|
||||
}).Leveled(slog.LevelDebug)
|
||||
api := agentproc.NewAPI(logger, agentexec.DefaultExecer, nil, envInfo, nil, workingDir)
|
||||
api := agentproc.NewAPI(logger, agentexec.DefaultExecer, nil, nil, envInfo, nil, workingDir)
|
||||
t.Cleanup(func() {
|
||||
_ = api.Close()
|
||||
})
|
||||
@@ -174,7 +174,7 @@ func TestAccessLogIncludesChatID(t *testing.T) {
|
||||
|
||||
sink := testutil.NewFakeSink(t)
|
||||
logger := sink.Logger()
|
||||
api := agentproc.NewAPI(logger, agentexec.DefaultExecer, nil, nil, nil, nil)
|
||||
api := agentproc.NewAPI(logger, agentexec.DefaultExecer, nil, nil, nil, nil, nil)
|
||||
t.Cleanup(func() {
|
||||
_ = api.Close()
|
||||
})
|
||||
@@ -1144,7 +1144,7 @@ func TestHandleStartProcess_ChatHeaders_EmptyWorkDir_StillNotifies(t *testing.T)
|
||||
defer unsub()
|
||||
|
||||
logger := slogtest.Make(t, nil).Leveled(slog.LevelDebug)
|
||||
api := agentproc.NewAPI(logger, agentexec.DefaultExecer, pathStore, nil, func(current []string) ([]string, error) {
|
||||
api := agentproc.NewAPI(logger, agentexec.DefaultExecer, nil, pathStore, nil, func(current []string) ([]string, error) {
|
||||
return current, nil
|
||||
}, nil)
|
||||
defer api.Close()
|
||||
|
||||
+19
-15
@@ -10,6 +10,7 @@ import (
|
||||
"time"
|
||||
|
||||
"github.com/google/uuid"
|
||||
"github.com/spf13/afero"
|
||||
"golang.org/x/xerrors"
|
||||
|
||||
"cdr.dev/slog/v3"
|
||||
@@ -75,6 +76,7 @@ type manager struct {
|
||||
mu sync.Mutex
|
||||
logger slog.Logger
|
||||
execer agentexec.Execer
|
||||
fs afero.Fs
|
||||
clock quartz.Clock
|
||||
procs map[string]*process
|
||||
closed bool
|
||||
@@ -84,13 +86,17 @@ type manager struct {
|
||||
}
|
||||
|
||||
// newManager creates a new process manager.
|
||||
func newManager(logger slog.Logger, execer agentexec.Execer, envInfo usershell.EnvInfoer, updateEnv func(current []string) (updated []string, err error), workingDir func() string) *manager {
|
||||
func newManager(logger slog.Logger, execer agentexec.Execer, fs afero.Fs, envInfo usershell.EnvInfoer, updateEnv func(current []string) (updated []string, err error), workingDir func() string) *manager {
|
||||
if fs == nil {
|
||||
fs = afero.NewOsFs()
|
||||
}
|
||||
if envInfo == nil {
|
||||
envInfo = &usershell.SystemEnvInfo{}
|
||||
}
|
||||
return &manager{
|
||||
logger: logger,
|
||||
execer: execer,
|
||||
fs: fs,
|
||||
clock: quartz.NewReal(),
|
||||
procs: make(map[string]*process),
|
||||
updateEnv: updateEnv,
|
||||
@@ -122,7 +128,7 @@ func (m *manager) start(req workspacesdk.StartProcessRequest, chatID string) (*p
|
||||
// the process is not tied to any HTTP request.
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
cmd := m.execer.CommandContext(ctx, "sh", "-c", req.Command)
|
||||
cmd.Dir = m.resolveWorkDir(req.WorkDir)
|
||||
cmd.Dir = m.resolveWorkingDirectory(req.WorkDir)
|
||||
cmd.Stdin = nil
|
||||
cmd.SysProcAttr = procSysProcAttr()
|
||||
|
||||
@@ -370,23 +376,21 @@ func (p *process) waitForOutput(ctx context.Context) error {
|
||||
return ctx.Err()
|
||||
}
|
||||
|
||||
// resolveWorkDir returns the directory a process should start in.
|
||||
// Priority: explicit request dir > agent configured dir > $HOME.
|
||||
// Falls through when a candidate is empty or does not exist on
|
||||
// disk, matching the behavior of SSH sessions.
|
||||
func (m *manager) resolveWorkDir(requested string) string {
|
||||
// resolveWorkingDirectory returns the directory a process should start in.
|
||||
// Priority: explicit request dir > agent configured dir > user home.
|
||||
// The configured dir > home tail is shared with SSH sessions via
|
||||
// usershell.ResolveWorkingDirectory so the two cannot drift.
|
||||
func (m *manager) resolveWorkingDirectory(requested string) string {
|
||||
if requested != "" {
|
||||
return requested
|
||||
}
|
||||
var configured string
|
||||
if m.workingDir != nil {
|
||||
if dir := m.workingDir(); dir != "" {
|
||||
if info, err := os.Stat(dir); err == nil && info.IsDir() {
|
||||
return dir
|
||||
}
|
||||
}
|
||||
configured = m.workingDir()
|
||||
}
|
||||
if home, err := m.envInfo.HomeDir(); err == nil {
|
||||
return home
|
||||
dir, err := usershell.ResolveWorkingDirectory(m.fs, m.envInfo, configured)
|
||||
if err != nil {
|
||||
return ""
|
||||
}
|
||||
return ""
|
||||
return dir
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user