From 827c221e395136fc6b0a6ac9a75cadecc8dae614 Mon Sep 17 00:00:00 2001 From: MK Date: Wed, 13 May 2026 13:45:12 -0400 Subject: [PATCH 1/2] fix: overlay skill-declared secrets dynamically (closes #48) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the hardcoded knownKeys list in Runner.overlaySecrets and OverlaySecretsToEnv with a dynamic union of the builtin keys and whatever the secrets provider exposes via List(). Custom skill env vars stored via `forge secrets set` now reach the OS environment so SkillCommandExecutor / cli_execute can read them, with no edits to runner.go required when adding a new skill. - Extract builtinSecretKeys to a single package-level slice (used as the fallback when a provider cannot enumerate, e.g. EnvProvider). - Add secretOverlayKeys helper that returns builtins ∪ provider.List(), deduped. List errors are surfaced but builtins are still returned so the builtin overlay path keeps working when enumeration fails (wrong passphrase on a global file, etc.). - Keep cross-category secret-reuse detection scoped to builtinSecretKeys (only set with categories defined in secretCategory). - Tests: stub-provider unit tests for union/dedup/error paths plus an end-to-end test that writes a non-builtin key into an encrypted file and verifies OverlaySecretsToEnv lands it in os.Environ. --- forge-cli/runtime/runner.go | 92 +++++++++----- forge-cli/runtime/runner_secrets_test.go | 150 ++++++++++++++++++++++- 2 files changed, 210 insertions(+), 32 deletions(-) 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) + } +} From f8a041fe6c861ba62ca45e6e539202ad20cda897 Mon Sep 17 00:00:00 2001 From: MK Date: Mon, 18 May 2026 12:03:35 -0400 Subject: [PATCH 2/2] docs: skill-declared secrets overlay (#48) Add a Skill-Declared Secrets section to docs/security/secret-management.md explaining how the runtime now overlays builtin keys + provider.List() results, with a runnable example. Update docs/core-concepts/runtime-engine.md so 'Secret Reuse Detection' covers the new dynamic-overlay union and explicitly notes that cross- category reuse detection stays scoped to builtin categories. Update docs/skills/writing-custom-skills.md to point at the new section when describing the env var passthrough contract. --- docs/core-concepts/runtime-engine.md | 6 +++--- docs/security/secret-management.md | 26 ++++++++++++++++++++++++++ docs/skills/writing-custom-skills.md | 2 +- 3 files changed, 30 insertions(+), 4 deletions(-) diff --git a/docs/core-concepts/runtime-engine.md b/docs/core-concepts/runtime-engine.md index f0b5c0c..75119ad 100644 --- a/docs/core-concepts/runtime-engine.md +++ b/docs/core-concepts/runtime-engine.md @@ -86,11 +86,11 @@ When `OPENAI_API_KEY` is set to the sentinel value `__oauth__`, the `SkillComman Skill scripts (e.g., `code-review-diff.sh`) detect `OPENAI_BASE_URL` and automatically use the OpenAI Responses API with streaming instead of the standard Chat Completions API. -### Secret Reuse Detection +### Secret Overlay and Reuse Detection -At startup, the runtime validates that secret values are not reused across different purpose categories. Sharing the same token between unrelated services (e.g., using an OpenAI API key as a Telegram bot token) is blocked with an error. +At startup, the runtime overlays secret values from the configured provider chain into the process environment so downstream skill scripts and `cli_execute` invocations can read them via `os.Getenv`. The overlay set is the union of forge's builtin LLM/search/channel keys and whatever the provider enumerates via `List()` — so a skill declaring a custom env var name (e.g. `ACME_API_TOKEN`) flows through without any code change. See [Secret Management](../security/secret-management.md#skill-declared-secrets). -Categories: `llm` (OPENAI_API_KEY, ANTHROPIC_API_KEY, GEMINI_API_KEY, etc.), `search` (TAVILY_API_KEY, PERPLEXITY_API_KEY), `telegram` (TELEGRAM_BOT_TOKEN), `slack` (SLACK_APP_TOKEN, SLACK_BOT_TOKEN). Same-category reuse (e.g., two LLM keys with the same value) is allowed. +Once overlaid, the runtime also validates that secret values are not reused across different purpose categories. Sharing the same token between unrelated services (e.g., using an OpenAI API key as a Telegram bot token) is blocked with an error. Categories: `llm` (OPENAI_API_KEY, ANTHROPIC_API_KEY, GEMINI_API_KEY, etc.), `search` (TAVILY_API_KEY, PERPLEXITY_API_KEY), `telegram` (TELEGRAM_BOT_TOKEN), `slack` (SLACK_APP_TOKEN, SLACK_BOT_TOKEN). Cross-category reuse detection is scoped to these builtin categories — custom skill-declared keys have no defined category and are not part of the check. Same-category reuse (e.g., two LLM keys with the same value) is allowed. ### Organization ID (OpenAI Enterprise) diff --git a/docs/security/secret-management.md b/docs/security/secret-management.md index 96b5fcf..b436de7 100644 --- a/docs/security/secret-management.md +++ b/docs/security/secret-management.md @@ -44,6 +44,32 @@ forge secret set OPENAI_API_KEY sk-agent2-key --local At runtime, secrets are resolved in order: **agent-local** -> **global** -> **environment variables**. This lets you override global defaults per agent. +## Skill-Declared Secrets + +Skills declare env var requirements in `SKILL.md` (`metadata.forge.requires.env`). At startup the runtime overlays each declared key from the configured secret provider chain into the process environment, so the skill's script or `cli_execute` invocation finds it via `os.Getenv`. + +The overlay key set is the union of: + +- **Builtin keys** — LLM (`OPENAI_API_KEY`, `ANTHROPIC_API_KEY`, etc.), search (`TAVILY_API_KEY`, `PERPLEXITY_API_KEY`), and channel tokens (`SLACK_*`, `TELEGRAM_BOT_TOKEN`). Always attempted even when the provider can't enumerate (e.g. the `env` provider). +- **Provider-enumerated keys** — every key the provider exposes via `List()`. The encrypted-file provider returns whatever you've stored via `forge secret set`, so a skill declaring `ACME_API_TOKEN` works without any code change in forge. + +```yaml +# skills/my-skill/SKILL.md +metadata: + forge: + requires: + env: + required: [ACME_API_TOKEN] +``` + +```bash +# Store the value once; runtime overlays it on every `forge run`. +forge secret --local set ACME_API_TOKEN my-secret-value +forge run +``` + +Existing values in the process environment are never overwritten — set `ACME_API_TOKEN` in your shell to override the encrypted store for one session. + ## Runtime Passphrase Prompting When `forge run` encounters encrypted secrets and no `FORGE_PASSPHRASE` environment variable is set, it prompts interactively: diff --git a/docs/skills/writing-custom-skills.md b/docs/skills/writing-custom-skills.md index 337728c..49e72d6 100644 --- a/docs/skills/writing-custom-skills.md +++ b/docs/skills/writing-custom-skills.md @@ -50,7 +50,7 @@ For skills **without** scripts (binary-backed skills like `k8s-incident-triage`) Skill scripts run in a restricted environment via `SkillCommandExecutor`: -- **Isolated environment**: Only `PATH`, `HOME`, and explicitly declared env vars are passed through +- **Isolated environment**: Only `PATH`, `HOME`, and the env vars the skill declared in `metadata.forge.requires.env` are passed through. Values may live in the shell, a `.env` file, or the encrypted secrets store — the runtime overlays each declared key from the provider chain at startup (see [Secret Management — Skill-Declared Secrets](../security/secret-management.md#skill-declared-secrets)) - **OAuth token resolution**: When `OPENAI_API_KEY` is set to `__oauth__`, the executor resolves OAuth credentials and injects the access token, `OPENAI_BASE_URL`, and the configured model as `REVIEW_MODEL` - **Configurable timeout**: Each skill declares a `timeout_hint` in its YAML frontmatter (e.g., 300s for research) - **No shell execution**: Scripts run via `bash