diff --git a/forge-cli/runtime/runner.go b/forge-cli/runtime/runner.go index 827f38c..3bd035d 100644 --- a/forge-cli/runtime/runner.go +++ b/forge-cli/runtime/runner.go @@ -2328,30 +2328,71 @@ func (r *Runner) resolveEmbedder(mc *coreruntime.ModelConfig) llm.Embedder { return embedder } +// builtinSecretKeys is the set of forge-internal secret keys whose purpose +// (LLM / search / channel) is recognized by secretCategory and that are always +// attempted via provider.Get, even when the provider cannot enumerate keys +// (e.g. the env provider). Custom skill-declared keys do not need to appear +// here — they are discovered dynamically via provider.List in secretOverlayKeys. +var builtinSecretKeys = []string{ + "OPENAI_API_KEY", + "ANTHROPIC_API_KEY", + "GEMINI_API_KEY", + "LLM_API_KEY", + "MODEL_API_KEY", + "TAVILY_API_KEY", + "PERPLEXITY_API_KEY", + "TELEGRAM_BOT_TOKEN", + "SLACK_APP_TOKEN", + "SLACK_BOT_TOKEN", +} + +// secretOverlayKeys returns the set of secret keys to overlay into the env: +// the builtin keys unioned with whatever the provider exposes via List(). +// Providers that cannot enumerate (e.g. EnvProvider) return nil from List, in +// which case only the builtins are returned. List errors are non-fatal — the +// builtin keys are still tried via Get downstream. +func secretOverlayKeys(provider secrets.Provider) ([]string, error) { + seen := make(map[string]bool, len(builtinSecretKeys)) + keys := make([]string, 0, len(builtinSecretKeys)) + for _, k := range builtinSecretKeys { + if seen[k] { + continue + } + seen[k] = true + keys = append(keys, k) + } + + listed, err := provider.List() + for _, k := range listed { + if seen[k] { + continue + } + seen[k] = true + keys = append(keys, k) + } + return keys, err +} + // overlaySecrets reads secrets from the configured provider chain and overlays -// them into envVars for known API key variables. Existing values are not overwritten. -// Returns an error if the same secret value is reused across different purpose categories. +// them into envVars. The key set is the builtin LLM/channel keys plus any +// custom keys the provider enumerates via List() — so skill-declared env vars +// stored as encrypted secrets are loaded without needing a code change here. +// Existing values are not overwritten. Returns an error if the same secret +// value is reused across different purpose categories among the builtin keys. func (r *Runner) overlaySecrets(envVars map[string]string) error { provider := r.buildSecretProvider() if provider == nil { return nil } - // Known secret keys to overlay into env for model resolution. - knownKeys := []string{ - "OPENAI_API_KEY", - "ANTHROPIC_API_KEY", - "GEMINI_API_KEY", - "LLM_API_KEY", - "MODEL_API_KEY", - "TAVILY_API_KEY", - "PERPLEXITY_API_KEY", - "TELEGRAM_BOT_TOKEN", - "SLACK_APP_TOKEN", - "SLACK_BOT_TOKEN", + keys, listErr := secretOverlayKeys(provider) + if listErr != nil { + r.logger.Warn("provider list failed; overlaying builtin keys only", map[string]any{ + "provider": provider.Name(), "error": listErr.Error(), + }) } - for _, key := range knownKeys { + for _, key := range keys { if envVars[key] != "" { continue // don't overwrite existing values } @@ -2362,9 +2403,10 @@ func (r *Runner) overlaySecrets(envVars map[string]string) error { } } - // Check for cross-category secret reuse. + // Cross-category secret reuse is only meaningful for keys whose category + // is known — i.e. the builtin set. Custom keys have no defined category. valueToKeys := make(map[string][]string) - for _, key := range knownKeys { + for _, key := range builtinSecretKeys { val := envVars[key] if val == "" { continue @@ -2504,20 +2546,8 @@ func OverlaySecretsToEnv(cfg *types.ForgeConfig, workDir string) { provider = secrets.NewChainProvider(chain...) } - knownKeys := []string{ - "OPENAI_API_KEY", - "ANTHROPIC_API_KEY", - "GEMINI_API_KEY", - "LLM_API_KEY", - "MODEL_API_KEY", - "TAVILY_API_KEY", - "PERPLEXITY_API_KEY", - "TELEGRAM_BOT_TOKEN", - "SLACK_APP_TOKEN", - "SLACK_BOT_TOKEN", - } - - for _, key := range knownKeys { + keys, _ := secretOverlayKeys(provider) + for _, key := range keys { if os.Getenv(key) != "" { continue } diff --git a/forge-cli/runtime/runner_secrets_test.go b/forge-cli/runtime/runner_secrets_test.go index 6cc6dff..430d5c4 100644 --- a/forge-cli/runtime/runner_secrets_test.go +++ b/forge-cli/runtime/runner_secrets_test.go @@ -1,6 +1,14 @@ package runtime -import "testing" +import ( + "os" + "path/filepath" + "slices" + "testing" + + "github.com/initializ/forge/forge-core/secrets" + "github.com/initializ/forge/forge-core/types" +) func TestSecretCategory(t *testing.T) { tests := []struct { @@ -29,3 +37,143 @@ func TestSecretCategory(t *testing.T) { }) } } + +// stubProvider implements secrets.Provider for unit-testing secretOverlayKeys +// without touching the filesystem. +type stubProvider struct { + keys []string + listErr error + values map[string]string +} + +func (s *stubProvider) Name() string { return "stub" } + +func (s *stubProvider) Get(key string) (string, error) { + if v, ok := s.values[key]; ok { + return v, nil + } + return "", &secrets.ErrSecretNotFound{Key: key, Provider: s.Name()} +} + +func (s *stubProvider) List() ([]string, error) { + return s.keys, s.listErr +} + +func TestSecretOverlayKeys_NonEnumerableProvider(t *testing.T) { + // Providers that cannot enumerate (e.g. EnvProvider) return nil from List. + // The overlay set must still include the builtin keys. + p := &stubProvider{keys: nil} + + got, err := secretOverlayKeys(p) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(got) != len(builtinSecretKeys) { + t.Errorf("len = %d, want %d (builtins only)", len(got), len(builtinSecretKeys)) + } + for _, b := range builtinSecretKeys { + if !slices.Contains(got, b) { + t.Errorf("missing builtin key %q", b) + } + } +} + +func TestSecretOverlayKeys_UnionsBuiltinsAndListed(t *testing.T) { + // Provider exposes a custom skill-declared key plus a duplicate of a + // builtin. The result must include the builtin set, plus the custom key, + // with no duplicates. + p := &stubProvider{keys: []string{"MY_CUSTOM_KEY", "OPENAI_API_KEY", "ANOTHER_CUSTOM"}} + + got, err := secretOverlayKeys(p) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + for _, b := range builtinSecretKeys { + if !slices.Contains(got, b) { + t.Errorf("missing builtin key %q", b) + } + } + if !slices.Contains(got, "MY_CUSTOM_KEY") { + t.Errorf("expected custom key MY_CUSTOM_KEY in overlay set") + } + if !slices.Contains(got, "ANOTHER_CUSTOM") { + t.Errorf("expected custom key ANOTHER_CUSTOM in overlay set") + } + + // Dedup: OPENAI_API_KEY appears once even though both sources declared it. + count := 0 + for _, k := range got { + if k == "OPENAI_API_KEY" { + count++ + } + } + if count != 1 { + t.Errorf("OPENAI_API_KEY appeared %d times, want 1 (dedup failed)", count) + } +} + +func TestSecretOverlayKeys_ListError(t *testing.T) { + // A failing List() must not lose the builtins — the runner can still + // recover the builtin keys via Get downstream. + wantErr := &secrets.ErrSecretNotFound{Key: "passphrase", Provider: "stub"} + p := &stubProvider{listErr: wantErr} + + got, err := secretOverlayKeys(p) + if err == nil { + t.Fatalf("expected error, got nil") + } + if len(got) != len(builtinSecretKeys) { + t.Errorf("len = %d, want %d (builtins only on List error)", len(got), len(builtinSecretKeys)) + } +} + +// TestOverlaySecretsToEnv_CustomSkillKey is the end-to-end regression test +// for issue #48: a skill-declared env var stored only in the encrypted secrets +// file must reach the OS environment so downstream executors can read it. +func TestOverlaySecretsToEnv_CustomSkillKey(t *testing.T) { + workDir := t.TempDir() + const passphrase = "test-passphrase" + const customKey = "MY_SKILL_API_KEY" + const customVal = "skill-secret-value" + const builtinKey = "TAVILY_API_KEY" + const builtinVal = "tavily-secret-value" + + t.Setenv("FORGE_PASSPHRASE", passphrase) + // Redirect HOME so the global ~/.forge/secrets.enc fallback resolves to an + // empty path under the temp dir, isolating the test from the developer's + // real home directory. + t.Setenv("HOME", t.TempDir()) + // Ensure the target env vars are clear before the test runs. + t.Setenv(customKey, "") + t.Setenv(builtinKey, "") + + // Write the encrypted secrets file the same way `forge secrets set` would. + encPath := filepath.Join(workDir, ".forge", "secrets.enc") + provider := secrets.NewEncryptedFileProvider(encPath, func() (string, error) { + return passphrase, nil + }) + if err := provider.SetBatch(map[string]string{ + customKey: customVal, + builtinKey: builtinVal, + }); err != nil { + t.Fatalf("seeding encrypted secrets: %v", err) + } + + cfg := &types.ForgeConfig{ + AgentID: "test-agent", + Secrets: types.SecretsConfig{Providers: []string{"encrypted-file"}}, + } + + OverlaySecretsToEnv(cfg, workDir) + + // Builtin behavior preserved. + if got := os.Getenv(builtinKey); got != builtinVal { + t.Errorf("builtin key %q in OS env = %q, want %q", builtinKey, got, builtinVal) + } + // Custom skill key (not in builtinSecretKeys) is now overlaid too — this + // is the regression check for #48. + if got := os.Getenv(customKey); got != customVal { + t.Errorf("custom skill key %q in OS env = %q, want %q", customKey, got, customVal) + } +}