diff --git a/services/agent-runner/internal/executor/executor.go b/services/agent-runner/internal/executor/executor.go index f9268a2ff..131e50dbd 100644 --- a/services/agent-runner/internal/executor/executor.go +++ b/services/agent-runner/internal/executor/executor.go @@ -792,10 +792,17 @@ func (e *Executor) coldStartEnvironment(ctx, turnCtx context.Context, cfg agent. // "running" DB status if the container died externally. Both backend // implementations tolerate this (idempotent start) — see // docs/ai-agent/environment-management.md's "Conversation attach path". + // The model key's env var NAME varies by provider, so a backend cannot + // infer it from Env alone — see EnvironmentConfig.LLMKeyEnvVar. + keyEnvVar := "" + if cfg.AgentType != agent.AgentTypeProviderCLI { + _, keyEnvVar = resolveProviderEnv(cfg.LLMProvider) + } handle, err := e.sandboxMgr.StartEnvironment(ctx, *env.BackendRef, sandbox.EnvironmentConfig{ EnvironmentID: environmentID.String(), Image: image, Env: containerEnv, + LLMKeyEnvVar: keyEnvVar, CPULimit: env.CPULimit, MemoryLimit: env.MemoryLimit, DiskLimitGB: env.DiskLimitGB, diff --git a/services/agent-runner/internal/sandbox/docker/environment.go b/services/agent-runner/internal/sandbox/docker/environment.go index 8056ba772..6f2a774f7 100644 --- a/services/agent-runner/internal/sandbox/docker/environment.go +++ b/services/agent-runner/internal/sandbox/docker/environment.go @@ -515,9 +515,11 @@ func (m *Manager) recreateEnvironmentIfMissingEnv(ctx context.Context, backendRe // container's own OS env, which is fixed at ContainerCreate time, so a // container created (or last recreated) while one conversation's trigger // fields were in scope has no way to pick up a later conversation whose -// trigger fields differ. Unlike GOOSE_PROVIDER/GOOSE_MODEL/the LLM API key -// (frozen forever to whichever agent's conversation first attached — see -// recreateEnvironmentIfMissingEnv's doc comment), ensureEnvironmentInfraEnv +// trigger fields differ. Unlike GOOSE_PROVIDER (frozen forever to whichever +// agent's conversation first attached — see +// recreateEnvironmentIfMissingEnv's doc comment; GOOSE_MODEL, OPENAI_HOST +// and the model key var DO follow a same-provider agent, see +// infraEnvKeysFor), ensureEnvironmentInfraEnv // keeps all of these in sync on every StartEnvironment call instead of // baking them in once and never again — for any of these four that means a // recreate each time a conversation's trigger fields differ from what the @@ -552,20 +554,27 @@ var pacaInfraEnvKeys = []string{ "GOOSE_PATH_ROOT", } -// infraEnvKeysFor is pacaInfraEnvKeys plus GOOSE_MODEL when cfg targets the -// same GOOSE_PROVIDER the container already has baked in. GOOSE_PROVIDER -// stays frozen to whichever agent attached first (see -// recreateEnvironmentIfMissingEnv), but the model is per-agent config the -// user edits after the fact (e.g. a provider_cli agent's cli_model) — left -// frozen too, an edit never reaches a static environment, so a bad model -// can't be fixed and a wrong one silently keeps working. Gated on a -// matching provider because a model id from a different provider's agent -// is meaningless to the provider the container is frozen to. +// infraEnvKeysFor is pacaInfraEnvKeys plus the LLM keys that follow the +// attaching agent: GOOSE_MODEL, OPENAI_HOST and the agent's own model key +// var (cfg.LLMKeyEnvVar). GOOSE_PROVIDER stays frozen to whichever agent +// attached first (see recreateEnvironmentIfMissingEnv), and these three are +// gated on a matching provider because a model id or base URL from a +// different provider's agent is meaningless to the provider the container +// is frozen to. Under that gate they must be reconciled, not frozen: the +// model is per-agent config a user edits after the fact, the base URL +// follows the provider, and a rotated or replaced API key otherwise never +// reaches the container — which is exactly how a static environment ends up +// failing every attach with "Authentication required" against a credential +// the agent row no longer holds. func infraEnvKeysFor(cfg sandbox.EnvironmentConfig, existing map[string]string) []string { if cfg.Env["GOOSE_PROVIDER"] == "" || cfg.Env["GOOSE_PROVIDER"] != existing["GOOSE_PROVIDER"] { return pacaInfraEnvKeys } - return append(append([]string(nil), pacaInfraEnvKeys...), "GOOSE_MODEL") + keys := append(append([]string(nil), pacaInfraEnvKeys...), "GOOSE_MODEL", "OPENAI_HOST") + if cfg.LLMKeyEnvVar != "" { + keys = append(keys, cfg.LLMKeyEnvVar) + } + return keys } // ensureEnvironmentInfraEnv keeps pacaInfraEnvKeys in sync with this diff --git a/services/agent-runner/internal/sandbox/environment.go b/services/agent-runner/internal/sandbox/environment.go index 7c3f629a2..7c0880320 100644 --- a/services/agent-runner/internal/sandbox/environment.go +++ b/services/agent-runner/internal/sandbox/environment.go @@ -35,6 +35,17 @@ type EnvironmentConfig struct { // (services/api, tests, ...) to know what that default is. Image string Env map[string]string + // LLMKeyEnvVar names the env var holding this agent's model API key + // (e.g. OPENAI_API_KEY, ANTHROPIC_API_KEY) — the one LLM env key whose + // *name* varies by provider, so a backend cannot infer it from Env + // alone. Reconciled on every StartEnvironment together with + // GOOSE_MODEL/OPENAI_HOST whenever Env targets the provider the + // container already has baked in (see docker/k8s + // ensureEnvironmentInfraEnv): a rotated key or an edited model never + // reaching the container is what leaves every later attach failing + // with "Authentication required". Empty for a provider_cli agent, + // which has no model key of its own. + LLMKeyEnvVar string CPULimit, MemoryLimit string // DiskLimitGB is enforced natively via the backing PersistentVolumeClaim @@ -163,11 +174,13 @@ type EnvironmentBackend interface { // WaitForReady's auth header) and Env: neither backend can live-patch // an already-configured container/Pod's env on every start without // disrupting whatever the environment's own process tree is doing, so - // Env is instead backfilled at most once — only when the container/Pod - // doesn't already have it (checked via GOOSE_PROVIDER's presence; see + // Env's GOOSE_PROVIDER is backfilled at most once — only when the + // container/Pod doesn't already have it (see // docker.Manager.recreateEnvironmentIfMissingEnv / - // k8s.Manager.ensureEnvironmentEnv) — never diffed or re-applied on - // every call. + // k8s.Manager.ensureEnvironmentEnv); the keys that legitimately change + // between attaches (PACA_*, GOOSE_MODEL with LLMKeyEnvVar/OPENAI_HOST + // for a same-provider agent) are reconciled on every call instead, see + // both backends' ensureEnvironmentInfraEnv. StartEnvironment(ctx context.Context, backendRef string, cfg EnvironmentConfig) (*EnvironmentHandle, error) // StopEnvironment stops backendRef's container/Pod without deleting it // or its volume — unlike Backend.Stop, this is not a teardown. diff --git a/services/agent-runner/internal/sandbox/k8s/environment.go b/services/agent-runner/internal/sandbox/k8s/environment.go index 2d78a74b5..7d41995fb 100644 --- a/services/agent-runner/internal/sandbox/k8s/environment.go +++ b/services/agent-runner/internal/sandbox/k8s/environment.go @@ -558,9 +558,11 @@ func (m *Manager) patchDeploymentContainers(ctx context.Context, name string, co // pacaInfraEnvKeys — see docker/environment.go's own copy of this exact // list and its doc comment for why these specifically need to stay in sync -// with this process's own current config on every StartEnvironment call, -// unlike GOOSE_PROVIDER/GOOSE_MODEL/the LLM API key (frozen forever to -// whichever agent's conversation first attached this environment). +// with this process's own current config on every StartEnvironment call. +// GOOSE_PROVIDER stays frozen forever to whichever agent's conversation +// first attached this environment; GOOSE_MODEL, OPENAI_HOST and the model +// key var follow the attaching agent whenever its provider matches (see +// infraEnvKeysFor), so an edited model or a rotated key still lands. // PACA_API_KEY/PACA_API_URL/PACA_GATEWAY_URL are deployment-level; // PACA_WORKDIR/PACA_REPO_PLUGIN_IDS are conversation- // scoped (track trigger.Workdir/RepoPluginIDs) but need the @@ -578,20 +580,27 @@ var pacaInfraEnvKeys = []string{ "GOOSE_PATH_ROOT", } -// infraEnvKeysFor is pacaInfraEnvKeys plus GOOSE_MODEL when cfg targets the -// same GOOSE_PROVIDER the container already has baked in. GOOSE_PROVIDER -// stays frozen to whichever agent attached first (see -// ensureEnvironmentEnv), but the model is per-agent config the -// user edits after the fact (e.g. a provider_cli agent's cli_model) — left -// frozen too, an edit never reaches a static environment, so a bad model -// can't be fixed and a wrong one silently keeps working. Gated on a -// matching provider because a model id from a different provider's agent -// is meaningless to the provider the container is frozen to. +// infraEnvKeysFor is pacaInfraEnvKeys plus the LLM keys that follow the +// attaching agent: GOOSE_MODEL, OPENAI_HOST and the agent's own model key +// var (cfg.LLMKeyEnvVar). GOOSE_PROVIDER stays frozen to whichever agent +// attached first (see ensureEnvironmentEnv), and these three are gated on +// a matching provider because a model id or base URL from a different +// provider's agent is meaningless to the provider the container is frozen +// to. Under that gate they must be reconciled, not frozen: the model is +// per-agent config a user edits after the fact, the base URL follows the +// provider, and a rotated or replaced API key otherwise never reaches the +// container — which is exactly how a static environment ends up failing +// every attach with "Authentication required" against a credential the +// agent row no longer holds. func infraEnvKeysFor(cfg sandbox.EnvironmentConfig, existing map[string]string) []string { if cfg.Env["GOOSE_PROVIDER"] == "" || cfg.Env["GOOSE_PROVIDER"] != existing["GOOSE_PROVIDER"] { return pacaInfraEnvKeys } - return append(append([]string(nil), pacaInfraEnvKeys...), "GOOSE_MODEL") + keys := append(append([]string(nil), pacaInfraEnvKeys...), "GOOSE_MODEL", "OPENAI_HOST") + if cfg.LLMKeyEnvVar != "" { + keys = append(keys, cfg.LLMKeyEnvVar) + } + return keys } // ensureEnvironmentInfraEnv keeps pacaInfraEnvKeys in sync with cfg.Env on diff --git a/services/agent-runner/internal/sandbox/k8s/environment_test.go b/services/agent-runner/internal/sandbox/k8s/environment_test.go index 50d6a3d43..36ad4dc7d 100644 --- a/services/agent-runner/internal/sandbox/k8s/environment_test.go +++ b/services/agent-runner/internal/sandbox/k8s/environment_test.go @@ -823,3 +823,103 @@ func deploymentEnvMap(t *testing.T, m *Manager, name string) map[string]string { } return out } + +// envMapOf reads backendRef's Deployment container env into a map. +func envMapOf(t *testing.T, m *Manager, name string) map[string]string { + t.Helper() + got, err := m.clientset.AppsV1().Deployments(m.namespace).Get(context.Background(), name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("get deployment: %v", err) + } + out := map[string]string{} + for _, ev := range got.Spec.Template.Spec.Containers[0].Env { + out[ev.Name] = ev.Value + } + return out +} + +// TestEnsureEnvironmentInfraEnv_RefreshesSameProviderLLMKeys pins the fix +// for a static environment that fails every attach with "Authentication +// required": the Pod keeps whatever model, key and base URL it was created +// with while the agent row has moved on, so the next start must reconcile +// all three — a rotated key is the case that actually breaks the turn. +func TestEnsureEnvironmentInfraEnv_RefreshesSameProviderLLMKeys(t *testing.T) { + const name = "paca-env-env5" + existing := []corev1.EnvVar{ + {Name: "GOOSE_PROVIDER", Value: "openai"}, + {Name: "GOOSE_MODEL", Value: "old-model"}, + {Name: "OPENAI_API_KEY", Value: "sk-revoked"}, + {Name: "OPENAI_HOST", Value: "https://gateway.example"}, + {Name: "GITHUB_TOKEN", Value: "gh-keep"}, + } + m := managerWithDeployment("paca", deploymentFixtureWithEnv(name, existing)) + + cfg := sandbox.EnvironmentConfig{ + Env: map[string]string{ + "GOOSE_PROVIDER": "openai", + "GOOSE_MODEL": "new-model", + "OPENAI_API_KEY": "sk-fresh", + "OPENAI_HOST": "https://ollama.com", + "PACA_API_KEY": "paca-key", + "GITHUB_TOKEN": "gh-keep", + }, + LLMKeyEnvVar: "OPENAI_API_KEY", + } + if err := m.ensureEnvironmentInfraEnv(context.Background(), name, cfg); err != nil { + t.Fatalf("ensureEnvironmentInfraEnv: %v", err) + } + + got := envMapOf(t, m, name) + for k, want := range cfg.Env { + if got[k] != want { + t.Errorf("deployment env[%q] = %q, want %q", k, got[k], want) + } + } + if got["GITHUB_TOKEN"] != "gh-keep" { + t.Errorf("deployment env[GITHUB_TOKEN] = %q, want the user env var left alone", got["GITHUB_TOKEN"]) + } +} + +// TestEnsureEnvironmentInfraEnv_LeavesLLMKeysWhenProviderDiffers is the +// other half of the gate: a model id, key var and base URL belonging to a +// different provider than the one the Pod is frozen to are meaningless +// there, so they must not be applied — only the PACA_* keys reconcile. +func TestEnsureEnvironmentInfraEnv_LeavesLLMKeysWhenProviderDiffers(t *testing.T) { + const name = "paca-env-env6" + existing := []corev1.EnvVar{ + {Name: "GOOSE_PROVIDER", Value: "openai"}, + {Name: "GOOSE_MODEL", Value: "frozen-model"}, + {Name: "OPENAI_API_KEY", Value: "sk-frozen"}, + {Name: "OPENAI_HOST", Value: "https://ollama.com"}, + } + m := managerWithDeployment("paca", deploymentFixtureWithEnv(name, existing)) + + cfg := sandbox.EnvironmentConfig{ + Env: map[string]string{ + "GOOSE_PROVIDER": "anthropic", + "GOOSE_MODEL": "claude-sonnet-5", + "ANTHROPIC_API_KEY": "sk-ant", + "PACA_API_KEY": "paca-key", + }, + LLMKeyEnvVar: "ANTHROPIC_API_KEY", + } + if err := m.ensureEnvironmentInfraEnv(context.Background(), name, cfg); err != nil { + t.Fatalf("ensureEnvironmentInfraEnv: %v", err) + } + + got := envMapOf(t, m, name) + for k, want := range map[string]string{ + "GOOSE_PROVIDER": "openai", + "GOOSE_MODEL": "frozen-model", + "OPENAI_API_KEY": "sk-frozen", + "OPENAI_HOST": "https://ollama.com", + "PACA_API_KEY": "paca-key", + } { + if got[k] != want { + t.Errorf("deployment env[%q] = %q, want %q", k, got[k], want) + } + } + if _, ok := got["ANTHROPIC_API_KEY"]; ok { + t.Errorf("deployment env[ANTHROPIC_API_KEY] = %q, want it absent (another provider's key var)", got["ANTHROPIC_API_KEY"]) + } +}