mirror of
https://github.com/coder/coder.git
synced 2026-06-02 20:48:20 +00:00
fix: pass agent context config explicitly instead of reading env (#24759)
The CODER_AGENT_EXP_* env vars are agent-internal options. When set in the workspace environment they leak to MCP subprocesses and user shells. ReadEnvConfig() captures the values and ClearEnvVars() strips them before the reinit loop, so config survives agent restarts. NewAPI and ReadEnvConfig both use applyDefaults() to fill zero fields. The chatd test passes config via agenttest.WithContextConfigFromEnv().
This commit is contained in:
committed by
GitHub
parent
1666bff1f9
commit
3c450899ea
+4
-1
@@ -114,6 +114,7 @@ type Options struct {
|
||||
SocketServerEnabled bool
|
||||
SocketPath string // Path for the agent socket server socket
|
||||
BoundaryLogProxySocketPath string
|
||||
ContextConfig agentcontextconfig.Config
|
||||
// DERPTLSConfig is an optional TLS config for DERP connections.
|
||||
DERPTLSConfig *tls.Config
|
||||
}
|
||||
@@ -233,6 +234,7 @@ func New(options Options) Agent {
|
||||
socketPath: options.SocketPath,
|
||||
socketServerEnabled: options.SocketServerEnabled,
|
||||
boundaryLogProxySocketPath: options.BoundaryLogProxySocketPath,
|
||||
contextConfig: options.ContextConfig,
|
||||
derpTLSConfig: options.DERPTLSConfig,
|
||||
}
|
||||
// Initially, we have a closed channel, reflecting the fact that we are not initially connected.
|
||||
@@ -312,6 +314,7 @@ type agent struct {
|
||||
// It may be nil if there is a problem starting the server.
|
||||
boundaryLogProxy *boundarylogproxy.Server
|
||||
boundaryLogProxySocketPath string
|
||||
contextConfig agentcontextconfig.Config
|
||||
|
||||
prometheusRegistry *prometheus.Registry
|
||||
// metrics are prometheus registered metrics that will be collected and
|
||||
@@ -427,7 +430,7 @@ func (a *agent) init() {
|
||||
return m.Directory
|
||||
}
|
||||
return ""
|
||||
})
|
||||
}, a.contextConfig)
|
||||
a.reconnectingPTYServer = reconnectingpty.NewServer(
|
||||
a.logger.Named("reconnecting-pty"),
|
||||
a.sshServer,
|
||||
|
||||
@@ -58,18 +58,10 @@ func TestReportConnectionEmpty(t *testing.T) {
|
||||
}
|
||||
|
||||
func TestContextConfigAPI_InitOnce(t *testing.T) {
|
||||
// Not parallel: uses t.Setenv to clear env vars.
|
||||
|
||||
// Clear env vars so defaults are used and the test is
|
||||
// hermetic regardless of the surrounding environment.
|
||||
t.Setenv(agentcontextconfig.EnvInstructionsDirs, "")
|
||||
t.Setenv(agentcontextconfig.EnvInstructionsFile, "")
|
||||
t.Setenv(agentcontextconfig.EnvSkillsDirs, "")
|
||||
t.Setenv(agentcontextconfig.EnvSkillMetaFile, "")
|
||||
t.Setenv(agentcontextconfig.EnvMCPConfigFiles, "")
|
||||
t.Parallel()
|
||||
|
||||
// After the fix, contextConfigAPI is set once in init() and
|
||||
// never reassigned. Config() evaluates lazily via the
|
||||
// never reassigned. Resolve() evaluates lazily via the
|
||||
// manifest, so there is no concurrent write to race with.
|
||||
dir1 := platformAbsPath("dir1")
|
||||
dir2 := platformAbsPath("dir2")
|
||||
@@ -81,7 +73,7 @@ func TestContextConfigAPI_InitOnce(t *testing.T) {
|
||||
return m.Directory
|
||||
}
|
||||
return ""
|
||||
})
|
||||
}, agentcontextconfig.Config{})
|
||||
|
||||
mcpFiles1 := a.contextConfigAPI.MCPConfigFiles()
|
||||
require.NotEmpty(t, mcpFiles1)
|
||||
|
||||
@@ -68,43 +68,81 @@ const (
|
||||
DefaultMCPConfigFile = ".mcp.json"
|
||||
)
|
||||
|
||||
// Config holds the agent's context configuration.
|
||||
// Defaults are applied by NewAPI, not by the zero value.
|
||||
type Config struct {
|
||||
InstructionsDirs string
|
||||
InstructionsFile string
|
||||
SkillsDirs string
|
||||
SkillMetaFile string
|
||||
MCPConfigFiles string
|
||||
}
|
||||
|
||||
// applyDefaults fills zero-valued fields with their defaults.
|
||||
func (c Config) applyDefaults() Config {
|
||||
c.InstructionsDirs = cmp.Or(c.InstructionsDirs, DefaultInstructionsDir)
|
||||
c.InstructionsFile = cmp.Or(c.InstructionsFile, DefaultInstructionsFile)
|
||||
c.SkillsDirs = cmp.Or(c.SkillsDirs, DefaultSkillsDir)
|
||||
c.SkillMetaFile = cmp.Or(c.SkillMetaFile, DefaultSkillMetaFile)
|
||||
c.MCPConfigFiles = cmp.Or(c.MCPConfigFiles, DefaultMCPConfigFile)
|
||||
return c
|
||||
}
|
||||
|
||||
// ReadEnvConfig reads the CODER_AGENT_EXP_* environment
|
||||
// variables, falling back to defaults for unset values.
|
||||
func ReadEnvConfig() Config {
|
||||
return Config{
|
||||
InstructionsDirs: strings.TrimSpace(os.Getenv(EnvInstructionsDirs)),
|
||||
InstructionsFile: strings.TrimSpace(os.Getenv(EnvInstructionsFile)),
|
||||
SkillsDirs: strings.TrimSpace(os.Getenv(EnvSkillsDirs)),
|
||||
SkillMetaFile: strings.TrimSpace(os.Getenv(EnvSkillMetaFile)),
|
||||
MCPConfigFiles: strings.TrimSpace(os.Getenv(EnvMCPConfigFiles)),
|
||||
}.applyDefaults()
|
||||
}
|
||||
|
||||
// envVarKeys returns every CODER_AGENT_EXP_* env var key
|
||||
// used by the context configuration subsystem.
|
||||
func envVarKeys() []string {
|
||||
return []string{
|
||||
EnvInstructionsDirs, EnvInstructionsFile,
|
||||
EnvSkillsDirs, EnvSkillMetaFile, EnvMCPConfigFiles,
|
||||
}
|
||||
}
|
||||
|
||||
// ClearEnvVars removes the CODER_AGENT_EXP_* environment
|
||||
// variables from the current process so they are not
|
||||
// inherited by child processes.
|
||||
func ClearEnvVars() {
|
||||
for _, key := range envVarKeys() {
|
||||
_ = os.Unsetenv(key)
|
||||
}
|
||||
}
|
||||
|
||||
// API exposes the resolved context configuration through the
|
||||
// agent's HTTP API.
|
||||
type API struct {
|
||||
workingDir func() string
|
||||
cfg Config
|
||||
}
|
||||
|
||||
// NewAPI accepts a closure that returns the working directory.
|
||||
// The directory is evaluated lazily on each call to Config(),
|
||||
// so the caller can update it after construction.
|
||||
func NewAPI(workingDir func() string) *API {
|
||||
// NewAPI creates a context configuration API. The working
|
||||
// directory closure is evaluated lazily per request.
|
||||
func NewAPI(workingDir func() string, cfg Config) *API {
|
||||
if workingDir == nil {
|
||||
workingDir = func() string { return "" }
|
||||
}
|
||||
return &API{workingDir: workingDir}
|
||||
return &API{workingDir: workingDir, cfg: cfg.applyDefaults()}
|
||||
}
|
||||
|
||||
// Config reads env vars, resolves paths, reads instruction files,
|
||||
// and discovers skills. Returns the HTTP response and the resolved
|
||||
// MCP config file paths (used only agent-internally). Exported
|
||||
// for use by tests.
|
||||
func Config(workingDir string) (workspacesdk.ContextConfigResponse, []string) {
|
||||
// TrimSpace all env vars before cmp.Or so that a
|
||||
// whitespace-only value falls through to the default
|
||||
// consistently. ResolvePaths also trims each comma-
|
||||
// separated entry, but without pre-trimming here a
|
||||
// bare " " would bypass cmp.Or and produce nil.
|
||||
instructionsDir := cmp.Or(strings.TrimSpace(os.Getenv(EnvInstructionsDirs)), DefaultInstructionsDir)
|
||||
instructionsFile := cmp.Or(strings.TrimSpace(os.Getenv(EnvInstructionsFile)), DefaultInstructionsFile)
|
||||
skillsDir := cmp.Or(strings.TrimSpace(os.Getenv(EnvSkillsDirs)), DefaultSkillsDir)
|
||||
skillMetaFile := cmp.Or(strings.TrimSpace(os.Getenv(EnvSkillMetaFile)), DefaultSkillMetaFile)
|
||||
mcpConfigFile := cmp.Or(strings.TrimSpace(os.Getenv(EnvMCPConfigFiles)), DefaultMCPConfigFile)
|
||||
|
||||
resolvedInstructionsDirs := ResolvePaths(instructionsDir, workingDir)
|
||||
resolvedSkillsDirs := ResolvePaths(skillsDir, workingDir)
|
||||
// Resolve reads instruction files, discovers skills, and
|
||||
// resolves MCP config file paths for the given config and
|
||||
// working directory.
|
||||
func Resolve(workingDir string, cfg Config) (workspacesdk.ContextConfigResponse, []string) {
|
||||
resolvedInstructionsDirs := ResolvePaths(cfg.InstructionsDirs, workingDir)
|
||||
resolvedSkillsDirs := ResolvePaths(cfg.SkillsDirs, workingDir)
|
||||
|
||||
// Read instruction files from each configured directory.
|
||||
parts := readInstructionFiles(resolvedInstructionsDirs, instructionsFile)
|
||||
parts := readInstructionFiles(resolvedInstructionsDirs, cfg.InstructionsFile)
|
||||
|
||||
// Also check the working directory for the instruction file,
|
||||
// unless it was already covered by InstructionsDirs.
|
||||
@@ -114,14 +152,14 @@ func Config(workingDir string) (workspacesdk.ContextConfigResponse, []string) {
|
||||
seenDirs[d] = struct{}{}
|
||||
}
|
||||
if _, ok := seenDirs[workingDir]; !ok {
|
||||
if entry, found := readInstructionFileFromDir(workingDir, instructionsFile); found {
|
||||
if entry, found := readInstructionFileFromDir(workingDir, cfg.InstructionsFile); found {
|
||||
parts = append(parts, entry)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Discover skills from each configured skills directory.
|
||||
skillParts := discoverSkills(resolvedSkillsDirs, skillMetaFile)
|
||||
skillParts := discoverSkills(resolvedSkillsDirs, cfg.SkillMetaFile)
|
||||
parts = append(parts, skillParts...)
|
||||
|
||||
// Guarantee non-nil slice to signal agent support.
|
||||
@@ -131,7 +169,7 @@ func Config(workingDir string) (workspacesdk.ContextConfigResponse, []string) {
|
||||
|
||||
return workspacesdk.ContextConfigResponse{
|
||||
Parts: parts,
|
||||
}, ResolvePaths(mcpConfigFile, workingDir)
|
||||
}, ResolvePaths(cfg.MCPConfigFiles, workingDir)
|
||||
}
|
||||
|
||||
// ContextPartsFromDir reads instruction files and discovers skills
|
||||
@@ -164,7 +202,7 @@ func ContextPartsFromDir(dir string) []codersdk.ChatMessagePart {
|
||||
// MCPConfigFiles returns the resolved MCP configuration file
|
||||
// paths for the agent's MCP manager.
|
||||
func (api *API) MCPConfigFiles() []string {
|
||||
_, mcpFiles := Config(api.workingDir())
|
||||
_, mcpFiles := Resolve(api.workingDir(), api.cfg)
|
||||
return mcpFiles
|
||||
}
|
||||
|
||||
@@ -177,7 +215,7 @@ func (api *API) Routes() http.Handler {
|
||||
}
|
||||
|
||||
func (api *API) handleGet(rw http.ResponseWriter, r *http.Request) {
|
||||
response, _ := Config(api.workingDir())
|
||||
response, _ := Resolve(api.workingDir(), api.cfg)
|
||||
httpapi.Write(r.Context(), rw, http.StatusOK, response)
|
||||
}
|
||||
|
||||
|
||||
@@ -157,13 +157,13 @@ func setupConfigTestEnv(t *testing.T, overrides map[string]string) string {
|
||||
return fakeHome
|
||||
}
|
||||
|
||||
func TestConfig(t *testing.T) {
|
||||
func TestResolve(t *testing.T) {
|
||||
//nolint:paralleltest // Uses t.Setenv to mutate process-wide environment.
|
||||
t.Run("Defaults", func(t *testing.T) {
|
||||
setupConfigTestEnv(t, nil)
|
||||
|
||||
workDir := platformAbsPath("work")
|
||||
cfg, mcpFiles := agentcontextconfig.Config(workDir)
|
||||
cfg, mcpFiles := agentcontextconfig.Resolve(workDir, agentcontextconfig.ReadEnvConfig())
|
||||
|
||||
// Parts is always non-nil.
|
||||
require.NotNil(t, cfg.Parts)
|
||||
@@ -197,7 +197,7 @@ func TestConfig(t *testing.T) {
|
||||
))
|
||||
|
||||
workDir := platformAbsPath("work")
|
||||
cfg, mcpFiles := agentcontextconfig.Config(workDir)
|
||||
cfg, mcpFiles := agentcontextconfig.Resolve(workDir, agentcontextconfig.ReadEnvConfig())
|
||||
|
||||
require.Equal(t, []string{optMCP}, mcpFiles)
|
||||
ctxFiles := filterParts(cfg.Parts, codersdk.ChatMessagePartTypeContextFile)
|
||||
@@ -220,7 +220,7 @@ func TestConfig(t *testing.T) {
|
||||
// Create a file matching the trimmed name.
|
||||
require.NoError(t, os.WriteFile(filepath.Join(fakeHome, "CLAUDE.md"), []byte("hello"), 0o600))
|
||||
|
||||
cfg, _ := agentcontextconfig.Config(workDir)
|
||||
cfg, _ := agentcontextconfig.Resolve(workDir, agentcontextconfig.ReadEnvConfig())
|
||||
|
||||
ctxFiles := filterParts(cfg.Parts, codersdk.ChatMessagePartTypeContextFile)
|
||||
require.Len(t, ctxFiles, 1)
|
||||
@@ -240,7 +240,7 @@ func TestConfig(t *testing.T) {
|
||||
require.NoError(t, os.WriteFile(filepath.Join(b, "AGENTS.md"), []byte("from b"), 0o600))
|
||||
|
||||
workDir := t.TempDir()
|
||||
cfg, _ := agentcontextconfig.Config(workDir)
|
||||
cfg, _ := agentcontextconfig.Resolve(workDir, agentcontextconfig.ReadEnvConfig())
|
||||
|
||||
ctxFiles := filterParts(cfg.Parts, codersdk.ChatMessagePartTypeContextFile)
|
||||
require.Len(t, ctxFiles, 2)
|
||||
@@ -262,7 +262,7 @@ func TestConfig(t *testing.T) {
|
||||
0o600,
|
||||
))
|
||||
|
||||
cfg, _ := agentcontextconfig.Config(workDir)
|
||||
cfg, _ := agentcontextconfig.Resolve(workDir, agentcontextconfig.ReadEnvConfig())
|
||||
|
||||
ctxFiles := filterParts(cfg.Parts, codersdk.ChatMessagePartTypeContextFile)
|
||||
require.NotNil(t, cfg.Parts)
|
||||
@@ -284,7 +284,7 @@ func TestConfig(t *testing.T) {
|
||||
0o600,
|
||||
))
|
||||
|
||||
cfg, _ := agentcontextconfig.Config(workDir)
|
||||
cfg, _ := agentcontextconfig.Resolve(workDir, agentcontextconfig.ReadEnvConfig())
|
||||
|
||||
// Should find the working dir file (not in instruction dirs).
|
||||
ctxFiles := filterParts(cfg.Parts, codersdk.ChatMessagePartTypeContextFile)
|
||||
@@ -301,7 +301,7 @@ func TestConfig(t *testing.T) {
|
||||
largeContent := strings.Repeat("a", 64*1024+100)
|
||||
require.NoError(t, os.WriteFile(filepath.Join(workDir, "AGENTS.md"), []byte(largeContent), 0o600))
|
||||
|
||||
cfg, _ := agentcontextconfig.Config(workDir)
|
||||
cfg, _ := agentcontextconfig.Resolve(workDir, agentcontextconfig.ReadEnvConfig())
|
||||
|
||||
ctxFiles := filterParts(cfg.Parts, codersdk.ChatMessagePartTypeContextFile)
|
||||
require.Len(t, ctxFiles, 1)
|
||||
@@ -341,7 +341,7 @@ func TestConfig(t *testing.T) {
|
||||
0o600,
|
||||
))
|
||||
|
||||
cfg, _ := agentcontextconfig.Config(workDir)
|
||||
cfg, _ := agentcontextconfig.Resolve(workDir, agentcontextconfig.ReadEnvConfig())
|
||||
|
||||
ctxFiles := filterParts(cfg.Parts, codersdk.ChatMessagePartTypeContextFile)
|
||||
require.Len(t, ctxFiles, 1)
|
||||
@@ -372,7 +372,7 @@ func TestConfig(t *testing.T) {
|
||||
0o600,
|
||||
))
|
||||
|
||||
cfg, _ := agentcontextconfig.Config(workDir)
|
||||
cfg, _ := agentcontextconfig.Resolve(workDir, agentcontextconfig.ReadEnvConfig())
|
||||
|
||||
skillParts := filterParts(cfg.Parts, codersdk.ChatMessagePartTypeSkill)
|
||||
require.Len(t, skillParts, 1)
|
||||
@@ -391,7 +391,7 @@ func TestConfig(t *testing.T) {
|
||||
})
|
||||
|
||||
workDir := t.TempDir()
|
||||
cfg, _ := agentcontextconfig.Config(workDir)
|
||||
cfg, _ := agentcontextconfig.Resolve(workDir, agentcontextconfig.ReadEnvConfig())
|
||||
|
||||
// Non-nil empty slice (signals agent supports new format).
|
||||
require.NotNil(t, cfg.Parts)
|
||||
@@ -407,7 +407,7 @@ func TestConfig(t *testing.T) {
|
||||
t.Setenv(agentcontextconfig.EnvInstructionsDirs, fakeHome)
|
||||
|
||||
workDir := t.TempDir()
|
||||
_, mcpFiles := agentcontextconfig.Config(workDir)
|
||||
_, mcpFiles := agentcontextconfig.Resolve(workDir, agentcontextconfig.ReadEnvConfig())
|
||||
|
||||
require.Equal(t, []string{optMCP}, mcpFiles)
|
||||
})
|
||||
@@ -430,7 +430,7 @@ func TestConfig(t *testing.T) {
|
||||
0o600,
|
||||
))
|
||||
|
||||
cfg, _ := agentcontextconfig.Config(workDir)
|
||||
cfg, _ := agentcontextconfig.Resolve(workDir, agentcontextconfig.ReadEnvConfig())
|
||||
skillParts := filterParts(cfg.Parts, codersdk.ChatMessagePartTypeSkill)
|
||||
require.Empty(t, skillParts)
|
||||
})
|
||||
@@ -456,7 +456,7 @@ func TestConfig(t *testing.T) {
|
||||
))
|
||||
}
|
||||
|
||||
cfg, _ := agentcontextconfig.Config(workDir)
|
||||
cfg, _ := agentcontextconfig.Resolve(workDir, agentcontextconfig.ReadEnvConfig())
|
||||
skillParts := filterParts(cfg.Parts, codersdk.ChatMessagePartTypeSkill)
|
||||
require.Len(t, skillParts, 1)
|
||||
require.Equal(t, "from skills1", skillParts[0].SkillDescription)
|
||||
@@ -471,7 +471,7 @@ func TestNewAPI_LazyDirectory(t *testing.T) {
|
||||
t.Setenv(agentcontextconfig.EnvMCPConfigFiles, "")
|
||||
|
||||
dir := ""
|
||||
api := agentcontextconfig.NewAPI(func() string { return dir })
|
||||
api := agentcontextconfig.NewAPI(func() string { return dir }, agentcontextconfig.ReadEnvConfig())
|
||||
|
||||
// Before directory is set, MCP paths resolve to nothing.
|
||||
mcpFiles := api.MCPConfigFiles()
|
||||
@@ -483,3 +483,62 @@ func TestNewAPI_LazyDirectory(t *testing.T) {
|
||||
require.NotEmpty(t, mcpFiles)
|
||||
require.Equal(t, []string{filepath.Join(dir, ".mcp.json")}, mcpFiles)
|
||||
}
|
||||
|
||||
// TestClearEnvVars verifies that ClearEnvVars removes every
|
||||
// CODER_AGENT_EXP_* env var from the process.
|
||||
//
|
||||
//nolint:paralleltest // Mutates process-wide environment.
|
||||
func TestClearEnvVars(t *testing.T) {
|
||||
// Set every context config env var.
|
||||
for _, key := range []string{
|
||||
agentcontextconfig.EnvInstructionsDirs,
|
||||
agentcontextconfig.EnvInstructionsFile,
|
||||
agentcontextconfig.EnvSkillsDirs,
|
||||
agentcontextconfig.EnvSkillMetaFile,
|
||||
agentcontextconfig.EnvMCPConfigFiles,
|
||||
} {
|
||||
t.Setenv(key, "some-value")
|
||||
}
|
||||
|
||||
agentcontextconfig.ClearEnvVars()
|
||||
|
||||
// Every env var should be absent.
|
||||
for _, key := range []string{
|
||||
agentcontextconfig.EnvInstructionsDirs,
|
||||
agentcontextconfig.EnvInstructionsFile,
|
||||
agentcontextconfig.EnvSkillsDirs,
|
||||
agentcontextconfig.EnvSkillMetaFile,
|
||||
agentcontextconfig.EnvMCPConfigFiles,
|
||||
} {
|
||||
_, ok := os.LookupEnv(key)
|
||||
require.False(t, ok, "env var %s should be cleared", key)
|
||||
}
|
||||
}
|
||||
|
||||
// TestResolve_ConfigOverridesEnv verifies that Resolve uses
|
||||
// the Config struct, not environment variables.
|
||||
//
|
||||
//nolint:paralleltest // Uses t.Setenv to mutate process-wide environment.
|
||||
func TestResolve_ConfigOverridesEnv(t *testing.T) {
|
||||
// Set env vars to one value.
|
||||
envDir := t.TempDir()
|
||||
t.Setenv(agentcontextconfig.EnvInstructionsDirs, envDir)
|
||||
|
||||
// Build a Config with a different value.
|
||||
cfgDir := t.TempDir()
|
||||
require.NoError(t, os.WriteFile(
|
||||
filepath.Join(cfgDir, "AGENTS.md"),
|
||||
[]byte("from config"),
|
||||
0o600,
|
||||
))
|
||||
|
||||
cfg := agentcontextconfig.ReadEnvConfig()
|
||||
cfg.InstructionsDirs = cfgDir
|
||||
|
||||
workDir := t.TempDir()
|
||||
result, _ := agentcontextconfig.Resolve(workDir, cfg)
|
||||
|
||||
ctxFiles := filterParts(result.Parts, codersdk.ChatMessagePartTypeContextFile)
|
||||
require.Len(t, ctxFiles, 1)
|
||||
require.Equal(t, "from config", ctxFiles[0].ContextFileContent)
|
||||
}
|
||||
|
||||
@@ -7,6 +7,7 @@ import (
|
||||
"github.com/stretchr/testify/assert"
|
||||
|
||||
"github.com/coder/coder/v2/agent"
|
||||
"github.com/coder/coder/v2/agent/agentcontextconfig"
|
||||
"github.com/coder/coder/v2/codersdk/agentsdk"
|
||||
"github.com/coder/coder/v2/testutil"
|
||||
)
|
||||
@@ -47,3 +48,11 @@ func New(t testing.TB, coderURL *url.URL, agentToken string, opts ...func(*agent
|
||||
|
||||
return agt
|
||||
}
|
||||
|
||||
// WithContextConfigFromEnv returns an agent option that
|
||||
// populates ContextConfig from the current environment.
|
||||
func WithContextConfigFromEnv() func(*agent.Options) {
|
||||
return func(o *agent.Options) {
|
||||
o.ContextConfig = agentcontextconfig.ReadEnvConfig()
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user