From 1b0e24e37970bdeccfcd29ce9e61952dc60219dc Mon Sep 17 00:00:00 2001 From: MK Date: Wed, 3 Jun 2026 16:32:12 -0400 Subject: [PATCH 1/3] feat(ui): workspace-level skill-builder LLM config (closes #92) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-#92 the forge ui skill builder borrowed credentials from whichever agent the operator clicked into: it read the picked agent's forge.yaml + .env, called os.Setenv to push those values into the UI process env, applied a hardcoded model upgrade (gpt-4.1 for openai, claude-opus-4-6 for anthropic), and used them to call the LLM in-process. That model produced four problems documented in the issue: 1. Empty workspaces had no answer — the skill builder needed an agent context to borrow credentials from. 2. Cross-agent skill development was arbitrary — picking one of several agents to borrow from determined billing/model/endpoint. 3. The hardcoded codegen upgrade ignored OPENAI_BASE_URL — any agent pointed at a custom OpenAI-compatible endpoint (OpenRouter, vLLM, litellm, self-hosted Kimi/Llama) got a request for gpt-4.1 against an endpoint that didn't host it. 4. The UI process env was stomped when switching agents — agent A's OPENAI_API_KEY would leak into the process, then agent B's would overwrite it. This fix decouples the skill-builder LLM from any specific agent. What landed: * NEW package forge-ui/uiconfig — workspace-level config loader with a three-tier resolution precedence: 1. /.forge/ui.yaml (primary, per-workspace) 2. ~/.forge/ui.yaml (fallback, operator's machine-wide) 3. The picked agent's forge.yaml + .env (deprecated; emits a deprecation warning the UI surfaces) Plus a forward-compat-ready Save function and a Validate function used by both the file loader and the HTTP write handler. * NEW EnvLookupForWorkspace + SetEnvFileValue in uiconfig — file-first env lookup layer (with OS env as fallback) and atomic writer for /.forge/.env. SetEnvFileValue writes with mode 0600 via temp-file-and-rename, edits in place (no duplicate lines on rewrite), removes the key on empty value, and auto-generates a /.forge/.gitignore containing ".env" so the secret file is auto-protected. Preserves operator-added gitignore lines. * Settings HTTP API at /api/settings/skill-builder (GET + PUT). PUT accepts an optional api_key field on the request body kept off the persisted YAML struct via a wrapping skillBuilderSettingsRequest type — the key value can never leak into ui.yaml. When present, the key is persisted to /.forge/.env under the operator's api_key_env name (or the provider default). Path-less GET /api/skill-builder/provider added for first-run detection in empty workspaces (no agent picked yet). * forge-ui handler refactor — handleSkillBuilderProvider and handleSkillBuilderChat call the loader instead of reading per-agent .env / encrypted secrets. ZERO os.Setenv calls remain. Credentials are threaded as request-scoped values through LLMStreamOptions.LLM. * SkillBuilderCodegenModel reduced to a Deprecated: no-op shim that returns the configured model verbatim. The operator's chosen model is used — no hardcoded gpt-4.1 / claude-opus-4-6 override. * forge-cli/cmd/ui.go llmStreamFunc simplified from ~120 lines of env-reading + ResolveModelConfig + OAuth + codegen-upgrade logic to ~30 lines that consume opts.LLM directly. No more coreruntime.ResolveModelConfig coupling. OAuth path removed from the skill-builder code path — workspace config requires an explicit API key (the issue #83 OAuth-precedence guardrail still applies). * Frontend (forge-ui/static/app.js) — new SkillBuilderSettingsModal component (preact/htm). Provider picker, model field, optional base URL (openai-only), api_key_env override, password field for the API key value with show/hide toggle. Shows "(saved; leave blank to keep)" when a key is already persisted — submitting an empty key leaves the saved value untouched. Status banner surfaces the resolution source (workspace / user / agent_fallback / unset) and the deprecation warning when the fallback shim resolves. * docs/ui/skill-builder-llm.md — full configuration reference, three setup recipes (UI / file / API), status banner semantics table, trust-boundary explanation for why ui.yaml + .env are split. Regression tests: forge-ui/uiconfig (18 tests): Load + Save round-trip; workspace > user > agent_fallback precedence; ollama-no-key-needed; api_key_env override; envfile create + 0600 perm + auto-gitignore; in-place update; empty-value removal; comments/other-keys preservation; file-precedes-OS precedence; missing-file fallback to OS env; gitignore idempotence; operator-gitignore preservation. forge-ui (19 settings + skill-builder tests): PUT persists + 0600 perm + auto-gitignore + no leak into ui.yaml; PUT honors custom api_key_env name; PUT omitting api_key preserves existing key; PUT validation rejects bad provider / missing model; GET after PUT returns workspace source; agent-fallback returns deprecation warning + preserves configured model verbatim (no codegen upgrade); env-stomp regression — switching between two agents with different OPENAI_API_KEY values in their .env files does NOT leak either key into the UI process's env. go test -race ./forge-ui/... ./forge-cli/cmd/... ./forge-cli/runtime/... golangci-lint and gofmt clean across both modules. --- CHANGELOG.md | 38 ++ docs/ui/skill-builder-llm.md | 173 ++++++++ forge-cli/cmd/ui.go | 130 ++---- forge-ui/handlers_settings.go | 122 ++++++ forge-ui/handlers_settings_test.go | 256 ++++++++++++ forge-ui/handlers_skill_builder.go | 142 +++---- .../handlers_skill_builder_envstomp_test.go | 102 +++++ forge-ui/handlers_skill_builder_test.go | 58 ++- forge-ui/server.go | 7 + forge-ui/static/app.js | 168 +++++++- forge-ui/types.go | 15 + forge-ui/uiconfig/envfile.go | 164 ++++++++ forge-ui/uiconfig/envfile_test.go | 200 +++++++++ forge-ui/uiconfig/uiconfig.go | 395 ++++++++++++++++++ forge-ui/uiconfig/uiconfig_test.go | 315 ++++++++++++++ 15 files changed, 2085 insertions(+), 200 deletions(-) create mode 100644 docs/ui/skill-builder-llm.md create mode 100644 forge-ui/handlers_settings.go create mode 100644 forge-ui/handlers_settings_test.go create mode 100644 forge-ui/handlers_skill_builder_envstomp_test.go create mode 100644 forge-ui/uiconfig/envfile.go create mode 100644 forge-ui/uiconfig/envfile_test.go create mode 100644 forge-ui/uiconfig/uiconfig.go create mode 100644 forge-ui/uiconfig/uiconfig_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index e408e5d..ec0bd5b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,44 @@ ## Unreleased +### Added + +- **Workspace-level skill-builder LLM config (issue #92).** The `forge ui` + skill builder now reads its LLM configuration from + `/.forge/ui.yaml` (or `~/.forge/ui.yaml` as a machine-wide + fallback) instead of borrowing credentials from whichever agent the + operator picked. The skill-builder LLM is decoupled from any agent's + runtime LLM, so the same configuration works across every agent in + the workspace and is usable before any agent has been scaffolded. + - New `GET` / `PUT` endpoints at `/api/settings/skill-builder` plus a + Settings modal in the skill-builder UI. + - New `GET /api/skill-builder/provider` (path-less) for first-run + detection in an empty workspace. + - Status banner surfaces the resolution source (`workspace` / `user` / + `agent_fallback` / `unset`) and a deprecation warning when the + agent-fallback compat shim resolves. + - The Settings modal accepts the API key value inline (password field) + and persists it to `/.forge/.env` with mode 0600. An + auto-generated `/.forge/.gitignore` protects the file + from accidental commits. The key value never appears in `ui.yaml` + and is never echoed back by the GET endpoint. + - See `docs/ui/skill-builder-llm.md` for the configuration reference. + +### Changed + +- **`SkillBuilderCodegenModel` no longer overrides the operator's model + (issue #92).** The function previously forced `gpt-4.1` for openai and + `claude-opus-4-6` for anthropic regardless of what the agent (or + workspace) had configured. The override is removed; the operator's + chosen model is used verbatim. This unblocks agents pointed at custom + OpenAI-compatible endpoints (OpenRouter, vLLM, litellm, self-hosted + Kimi/Llama) where the hardcoded "stronger" model isn't hosted. +- **Skill-builder handlers no longer call `os.Setenv` (issue #92).** The + pre-#92 handlers leaked the picked agent's `.env` into the `forge ui` + process's environment via `os.Setenv` calls, which caused cross-agent + credential stomping when switching agents in the UI. Credentials are + now threaded as request-scoped values. + ### Fixed - **`forge init` Custom provider now produces a runnable agent (issue #83).** diff --git a/docs/ui/skill-builder-llm.md b/docs/ui/skill-builder-llm.md new file mode 100644 index 0000000..b5b5edd --- /dev/null +++ b/docs/ui/skill-builder-llm.md @@ -0,0 +1,173 @@ +# Skill Builder LLM (workspace-level) + +The `forge ui` skill builder generates SKILL.md files via an LLM. That +LLM is configured at the **workspace level** — independent of any +specific agent's runtime LLM — so the same configuration works across +every agent in the workspace and is usable before any agent has been +scaffolded. + +## Why workspace-level + +Pre-#92 the skill builder borrowed credentials from whichever agent +the operator clicked into: it read the agent's `forge.yaml` and `.env`, +applied a hardcoded model upgrade (`gpt-4.1` for openai, `claude-opus-4-6` +for anthropic), and called the LLM from inside the `forge ui` process. +That model conflated two distinct concerns ("agent runtime LLM" vs. +"build-time codegen LLM"), broke against any agent pointed at a custom +OpenAI-compatible endpoint (the upgrade requested a model the endpoint +didn't host), caused cross-agent env-var stomping when switching agents, +and had no answer for empty workspaces. + +The workspace-level model fixes all four: + +- One LLM for the operator's skill-building work, used across every + agent in the workspace. +- Operator picks the model — no hardcoded upgrade. +- Credentials threaded as request-scoped data; the UI process's + environment is never mutated by handler calls. +- Works in an empty workspace before any agent is scaffolded. + +## Configuration file + +`/.forge/ui.yaml`: + +```yaml +skill_builder: + provider: openai # openai | anthropic | gemini | ollama + model: gpt-4.1 # operator-chosen; no hardcoded upgrade + base_url: https://... # optional, for OpenAI-compatible endpoints + api_key_env: OPENAI_API_KEY # which env var holds the key (default per provider) +``` + +- `provider` (required) — one of `openai`, `anthropic`, `gemini`, `ollama`. +- `model` (required) — operator picks. The skill builder uses this + verbatim; there is no `SkillBuilderCodegenModel` hardcoded upgrade. +- `base_url` (optional, openai only) — set this for OpenAI-compatible + endpoints (OpenRouter, vLLM, litellm, etc.). +- `api_key_env` (optional) — name of the environment variable the + `forge ui` process reads for the API key. Defaults per provider + (`OPENAI_API_KEY` / `ANTHROPIC_API_KEY` / `GEMINI_API_KEY`). Set this + if you keep the skill-builder credentials under a different name + (e.g. `WORKSPACE_LLM_KEY`) to avoid collisions with per-agent + runtime credentials. + +The API key itself is **never** stored in `ui.yaml` — only the env var +name is. Set the env var in your shell before launching `forge ui`. + +## Resolution precedence + +The loader resolves the skill-builder LLM through three tiers, in order: + +1. `/.forge/ui.yaml` — primary, per-workspace. +2. `~/.forge/ui.yaml` — fallback, operator's machine-wide default. +3. The picked agent's `forge.yaml` + `.env` — **deprecated** fallback. + When this tier resolves, the UI banner shows a deprecation warning + prompting the operator to configure workspace settings. This + compatibility shim will be removed in a future release. + +If none of the tiers resolves and the skill builder is invoked, the +chat handler returns a 400 with a message pointing to Settings. + +## Setting it up + +### Via the UI (recommended) + +1. Open `forge ui --dir ` and click any agent's + Skill Builder. +2. If no workspace-level config exists, the banner reads + **"Workspace skill-builder LLM is not configured"** — click + **Configure**. +3. Fill the form (provider, model, optional base URL, optional API key + env override) and **paste your API key** in the password field. +4. Save. The key is written to `/.forge/.env` (mode 0600) + under the env var name shown in the form. An auto-generated + `.forge/.gitignore` protects the file from being committed. + +The key value is never sent back by the GET endpoint and never +appears in `ui.yaml` — only `/.forge/.env` ever holds it. +To rotate, open Settings and paste a new value; submitting an empty +key leaves the saved value untouched. + +### Via the file + +``` +mkdir -p /.forge +cat > /.forge/ui.yaml <<'YAML' +skill_builder: + provider: openai + model: gpt-4.1 +YAML +echo 'OPENAI_API_KEY=sk-...' > /.forge/.env +chmod 600 /.forge/.env +echo '.env' > /.forge/.gitignore +``` + +Then launch `forge ui --dir `. The `forge ui` process +consults `/.forge/.env` for any env var named in `ui.yaml`, +with the OS environment as a fallback. + +### Via the API + +```sh +# Persist config + key in one PUT (api_key is optional; omit to leave +# the saved key unchanged). +curl -X PUT http://localhost:4200/api/settings/skill-builder \ + -H 'Content-Type: application/json' \ + -d '{"provider":"openai","model":"gpt-4.1","api_key":"sk-..."}' + +curl http://localhost:4200/api/settings/skill-builder +``` + +### Via the API + +```sh +curl -X PUT http://localhost:4200/api/settings/skill-builder \ + -H 'Content-Type: application/json' \ + -d '{"provider":"openai","model":"gpt-4.1"}' + +curl http://localhost:4200/api/settings/skill-builder +``` + +## Status banner semantics + +The skill builder header shows the resolved configuration plus a hint +about where it came from: + +| Banner says | What it means | +|---|---| +| `openai/gpt-4.1` (clean) | Workspace config resolved successfully; API key found. | +| `openai/gpt-4.1` + **using agent fallback (deprecated)** | No workspace/user config exists; the picked agent's `forge.yaml` is being used. Configure workspace settings to migrate. | +| `openai/gpt-4.1` + **API key not configured (env: OPENAI_API_KEY)** | Config resolved but the named env var is empty in the `forge ui` process. Set it and reload. | +| **Workspace skill-builder LLM is not configured** | First-run state. Click Configure. | + +## Why split `ui.yaml` and `.env`? + +Same trust-boundary reasoning as `forge init`'s `forge.yaml` / `.env` +split: + +- `ui.yaml` is non-secret (provider name, model name, base URL, env + var name). Operators may want to check this into their workspace + repo so the team shares the same skill-builder configuration. +- `.env` is the secret material (API key value). It lives under + `.forge/` with mode 0600 and an auto-generated `.gitignore` that + protects it from being committed. + +If you want to keep skill-builder credentials separate from per-agent +runtime credentials (recommended, especially when agents point at +OpenAI-compatible endpoints other than openai.com), set +`api_key_env: WORKSPACE_LLM_KEY` (or similar). The key value lands at +`/.forge/.env` under that name; per-agent runtime +credentials in each `/.env` stay untouched. + +## Decoupling rules the implementation enforces + +These are pinned by regression tests: + +- The skill builder LLM is independent of any agent's runtime LLM. + Agent A can ship with `provider: anthropic` and Claude while you + use GPT-4.1 to build skills. +- The skill builder **never** calls `os.Setenv` on the `forge ui` + process. Credentials are passed via request-scoped values. +- The previous `SkillBuilderCodegenModel` mapping (which forced + `gpt-4.1` / `claude-opus-4-6` regardless of the agent's configured + model) is removed. The operator's chosen model is used verbatim. diff --git a/forge-cli/cmd/ui.go b/forge-cli/cmd/ui.go index 8934f92..05599c4 100644 --- a/forge-cli/cmd/ui.go +++ b/forge-cli/cmd/ui.go @@ -9,13 +9,9 @@ import ( "strings" "syscall" - "github.com/initializ/forge/forge-cli/config" "github.com/initializ/forge/forge-cli/internal/tui" - "github.com/initializ/forge/forge-cli/runtime" "github.com/initializ/forge/forge-core/llm" - "github.com/initializ/forge/forge-core/llm/oauth" "github.com/initializ/forge/forge-core/llm/providers" - coreruntime "github.com/initializ/forge/forge-core/runtime" "github.com/initializ/forge/forge-core/util" forgeui "github.com/initializ/forge/forge-ui" "github.com/spf13/cobra" @@ -143,108 +139,36 @@ func runUI(cmd *cobra.Command, args []string) error { } // Build the LLMStreamFunc for skill builder conversations. + // + // Per issue #92, this callback consumes the workspace-level LLM + // configuration resolved by forge-ui (opts.LLM) and DOES NOT re-read + // the agent's forge.yaml / .env or mutate the UI process's env. + // Per-agent credentials live with the agent runtime, not the skill + // builder. llmStreamFunc := func(ctx context.Context, opts forgeui.LLMStreamOptions) error { - // Load agent config - cfgPath := filepath.Join(opts.AgentDir, "forge.yaml") - cfg, err := config.LoadForgeConfig(cfgPath) + if opts.LLM.Provider == "" { + return fmt.Errorf("skill-builder LLM is not configured (no workspace ui.yaml and no agent fallback available)") + } + + // Construct the LLM client from the resolved workspace config. + // No env reading, no os.Setenv. OAuth is intentionally NOT + // supported on this path — workspace-level config requires an + // explicit API key under api_key_env. (Operators who want to + // use ChatGPT OAuth specifically can set OPENAI_API_KEY from + // the OAuth token themselves; the workspace-LLM design does + // not silently override an explicit endpoint via the codex + // backend, per the issue #83 guardrail.) + clientCfg := llm.ClientConfig{ + Model: opts.LLM.Model, + APIKey: opts.LLM.APIKey, + BaseURL: opts.LLM.BaseURL, + } + client, err := providers.NewClient(opts.LLM.Provider, clientCfg) if err != nil { - return fmt.Errorf("loading config: %w", err) + return fmt.Errorf("creating LLM client: %w", err) } - // Load .env — force-set values so __oauth__ sentinels from prior - // handler calls don't block real keys from encrypted secrets. - envPath := filepath.Join(opts.AgentDir, ".env") - envVars, err := runtime.LoadEnvFile(envPath) - if err != nil { - return fmt.Errorf("loading env: %w", err) - } - for k, v := range envVars { - _ = os.Setenv(k, v) - } - - // Clear __oauth__ sentinels so OverlaySecretsToEnv can replace them - // with real keys from the encrypted secrets store. - for _, key := range []string{"OPENAI_API_KEY", "ANTHROPIC_API_KEY", "GEMINI_API_KEY"} { - if os.Getenv(key) == "__oauth__" { - _ = os.Unsetenv(key) - } - } - - // Overlay encrypted secrets - runtime.OverlaySecretsToEnv(cfg, opts.AgentDir) - - // Build env map for model resolution. OS env takes priority over .env - // file because OverlaySecretsToEnv may have replaced sentinels with - // real keys from the encrypted store. - envMap := make(map[string]string) - for k, v := range envVars { - if v != "__oauth__" { - envMap[k] = v - } - } - for _, kv := range os.Environ() { - parts := strings.SplitN(kv, "=", 2) - if len(parts) == 2 { - envMap[parts[0]] = parts[1] - } - } - - mc := coreruntime.ResolveModelConfig(cfg, envMap, "") - if mc == nil { - return fmt.Errorf("unable to resolve model configuration") - } - - // Resolve OAuth credentials when the API key is the __oauth__ sentinel - // or empty. OAuth/Codex backend has its own model constraints, so - // skip the codegen model upgrade for OAuth clients. - var client llm.Client - needsOAuth := mc.Provider == "openai" && (mc.Client.APIKey == "" || mc.Client.APIKey == "__oauth__") - // OAuth precedence guardrail (issue #83). The ChatGPT OAuth - // path's hardcoded chatgpt.com/backend-api/codex base URL is - // mutually exclusive with an operator-supplied OPENAI_BASE_URL. - // Without this guard, the skill builder would silently route - // requests to ChatGPT instead of the agent's configured - // OpenAI-compatible endpoint. - if needsOAuth && mc.Client.BaseURL != "" { - return fmt.Errorf( - "OPENAI_BASE_URL is set to %q but no OPENAI_API_KEY was provided; "+ - "the OpenAI OAuth credentials path is disabled when an explicit "+ - "base URL is in use (it would silently override your endpoint with "+ - "chatgpt.com/backend-api/codex). Set OPENAI_API_KEY for the configured endpoint", - mc.Client.BaseURL, - ) - } - if needsOAuth { - token, oauthErr := oauth.LoadCredentials(mc.Provider) - if oauthErr == nil && token != nil && token.RefreshToken != "" { - oauthCfg := oauth.OpenAIConfig() - baseURL := token.BaseURL - if baseURL == "" { - baseURL = oauthCfg.BaseURL - } - mc.Client.APIKey = token.AccessToken - mc.Client.BaseURL = baseURL - client = providers.NewOAuthClient(mc.Client, mc.Provider, oauthCfg) - } else if mc.Client.APIKey == "" || mc.Client.APIKey == "__oauth__" { - // No API key and OAuth failed — surface the error instead - // of silently falling through to a client with no auth. - if oauthErr != nil { - return fmt.Errorf("loading OAuth credentials: %w", oauthErr) - } - return fmt.Errorf("no OpenAI API key or OAuth credentials found; run 'forge init' with OAuth or set OPENAI_API_KEY") - } - } - if client == nil { - // Only upgrade to the codegen model for non-OAuth (API key) clients. - mc.Client.Model = forgeui.SkillBuilderCodegenModel(mc.Provider, mc.Client.Model) - var clientErr error - client, clientErr = providers.NewClient(mc.Provider, mc.Client) - if clientErr != nil { - return fmt.Errorf("creating LLM client: %w", clientErr) - } - } - - // Build chat request with system prompt + conversation messages + // Build chat request with system prompt + conversation messages. messages := []llm.ChatMessage{ {Role: "system", Content: opts.SystemPrompt}, } @@ -256,7 +180,7 @@ func runUI(cmd *cobra.Command, args []string) error { } req := &llm.ChatRequest{ - Model: mc.Client.Model, + Model: opts.LLM.Model, Messages: messages, Stream: true, } diff --git a/forge-ui/handlers_settings.go b/forge-ui/handlers_settings.go new file mode 100644 index 0000000..2ead587 --- /dev/null +++ b/forge-ui/handlers_settings.go @@ -0,0 +1,122 @@ +package forgeui + +import ( + "encoding/json" + "net/http" + + "github.com/initializ/forge/forge-ui/uiconfig" +) + +// skillBuilderSettingsRequest is the PUT body shape. The api_key field +// is INTENTIONALLY not part of uiconfig.SkillBuilderConfig — that +// struct is the on-disk YAML shape and the key value must never be +// persisted there. Instead, when api_key is present the handler writes +// it to /.forge/.env under the api_key_env name. Keeping +// the two flows in one PUT means the UI submits one form and the +// operator doesn't have to coordinate two endpoints. +type skillBuilderSettingsRequest struct { + uiconfig.SkillBuilderConfig + APIKey string `json:"api_key,omitempty"` +} + +// handleGetSkillBuilderSettings returns the current workspace-level +// skill-builder configuration plus enough metadata for the Settings +// page UI to render its form (current source, available providers, +// detected env var presence). +func (s *UIServer) handleGetSkillBuilderSettings(w http.ResponseWriter, _ *http.Request) { + // AgentDir is empty here — Settings is workspace-level. The loader + // will return Source=unset if no workspace/user config exists. + llm, err := uiconfig.LoadSkillBuilderLLM(s.cfg.WorkDir, "", uiconfig.EnvLookupForWorkspace(s.cfg.WorkDir)) + if err != nil { + writeError(w, http.StatusInternalServerError, "loading settings: "+err.Error()) + return + } + + writeJSON(w, http.StatusOK, map[string]any{ + "provider": llm.Provider, + "model": llm.Model, + "base_url": llm.BaseURL, + "api_key_env": llm.APIKeyEnv, + "has_key": llm.HasCredentials(), + "source": llm.Source, + "warning": llm.Warning, + "providers": []string{"openai", "anthropic", "gemini", "ollama"}, + }) +} + +// handlePutSkillBuilderSettings validates the submitted config and +// persists it to /.forge/ui.yaml. When the request carries +// an api_key value, it is written to /.forge/.env under +// the api_key_env name (or the provider default) with 0600 +// permissions and a sibling .gitignore protecting it. +func (s *UIServer) handlePutSkillBuilderSettings(w http.ResponseWriter, r *http.Request) { + var body skillBuilderSettingsRequest + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + writeError(w, http.StatusBadRequest, "invalid request body: "+err.Error()) + return + } + + if err := uiconfig.ValidateSkillBuilderConfig(body.SkillBuilderConfig); err != nil { + writeError(w, http.StatusBadRequest, err.Error()) + return + } + + if err := uiconfig.SaveSkillBuilderLLM(s.cfg.WorkDir, body.SkillBuilderConfig); err != nil { + writeError(w, http.StatusInternalServerError, "saving settings: "+err.Error()) + return + } + + // Persist the API key to /.forge/.env when provided. + // The key NAME is taken from body.APIKeyEnv if set, otherwise the + // provider default — this matches what the loader will look up at + // request time. + if body.APIKey != "" { + envName := body.APIKeyEnv + if envName == "" { + envName = defaultAPIKeyEnv(body.Provider) + } + if envName == "" { + writeError(w, http.StatusBadRequest, + "cannot persist api_key for this provider (no default env var name); set api_key_env explicitly") + return + } + if err := uiconfig.SetEnvFileValue(s.cfg.WorkDir, envName, body.APIKey); err != nil { + writeError(w, http.StatusInternalServerError, "saving api key: "+err.Error()) + return + } + } + + // Echo back the resolved state so the UI can update its banner + // without a second round trip. + llm, err := uiconfig.LoadSkillBuilderLLM(s.cfg.WorkDir, "", uiconfig.EnvLookupForWorkspace(s.cfg.WorkDir)) + if err != nil { + writeError(w, http.StatusInternalServerError, "reloading settings: "+err.Error()) + return + } + writeJSON(w, http.StatusOK, map[string]any{ + "provider": llm.Provider, + "model": llm.Model, + "base_url": llm.BaseURL, + "api_key_env": llm.APIKeyEnv, + "has_key": llm.HasCredentials(), + "source": llm.Source, + "warning": llm.Warning, + }) +} + +// defaultAPIKeyEnv mirrors uiconfig's internal mapping so the settings +// handler knows where to persist the api_key when the operator didn't +// override APIKeyEnv. Keeping a copy here (rather than exporting from +// uiconfig) keeps the small list visible at the use site; if it grows +// we can promote it. +func defaultAPIKeyEnv(provider string) string { + switch provider { + case "openai": + return "OPENAI_API_KEY" + case "anthropic": + return "ANTHROPIC_API_KEY" + case "gemini": + return "GEMINI_API_KEY" + } + return "" +} diff --git a/forge-ui/handlers_settings_test.go b/forge-ui/handlers_settings_test.go new file mode 100644 index 0000000..2a88a64 --- /dev/null +++ b/forge-ui/handlers_settings_test.go @@ -0,0 +1,256 @@ +package forgeui + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" +) + +func newTestServerForSettings(t *testing.T) *UIServer { + t.Helper() + isolateHome(t) + root := t.TempDir() + return NewUIServer(UIServerConfig{ + Port: 4200, + WorkDir: root, + }) +} + +func TestGetSkillBuilderSettings_Unset(t *testing.T) { + srv := newTestServerForSettings(t) + + req := httptest.NewRequest(http.MethodGet, "/api/settings/skill-builder", nil) + w := httptest.NewRecorder() + srv.handleGetSkillBuilderSettings(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body: %s", w.Code, w.Body.String()) + } + var resp map[string]any + if err := json.NewDecoder(w.Body).Decode(&resp); err != nil { + t.Fatalf("decode: %v", err) + } + if resp["source"] != "unset" { + t.Errorf("source = %q, want unset", resp["source"]) + } + if resp["has_key"] != false { + t.Errorf("has_key = %v, want false", resp["has_key"]) + } + providers, _ := resp["providers"].([]any) + if len(providers) == 0 { + t.Errorf("providers list should be populated") + } +} + +func TestPutSkillBuilderSettings_PersistsAndEchoes(t *testing.T) { + srv := newTestServerForSettings(t) + + // Operator submits a workspace-level config. + body := map[string]string{ + "provider": "openai", + "model": "gpt-4.1", + "base_url": "https://openrouter-ish.example.com/v1", + "api_key_env": "WORKSPACE_LLM_KEY", + } + raw, _ := json.Marshal(body) + req := httptest.NewRequest(http.MethodPut, "/api/settings/skill-builder", bytes.NewReader(raw)) + w := httptest.NewRecorder() + srv.handlePutSkillBuilderSettings(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body: %s", w.Code, w.Body.String()) + } + var resp map[string]any + if err := json.NewDecoder(w.Body).Decode(&resp); err != nil { + t.Fatalf("decode: %v", err) + } + for k, want := range map[string]string{ + "provider": "openai", + "model": "gpt-4.1", + "base_url": "https://openrouter-ish.example.com/v1", + "api_key_env": "WORKSPACE_LLM_KEY", + "source": "workspace", + } { + if got, _ := resp[k].(string); got != want { + t.Errorf("response[%q] = %q, want %q", k, got, want) + } + } + + // File on disk reflects the same shape. + path := filepath.Join(srv.cfg.WorkDir, ".forge", "ui.yaml") + raw2, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read ui.yaml: %v", err) + } + for _, want := range []string{ + "provider: openai", + "model: gpt-4.1", + "base_url: https://openrouter-ish.example.com/v1", + "api_key_env: WORKSPACE_LLM_KEY", + } { + if !strings.Contains(string(raw2), want) { + t.Errorf("ui.yaml missing %q:\n%s", want, raw2) + } + } +} + +func TestPutSkillBuilderSettings_RejectsInvalidProvider(t *testing.T) { + srv := newTestServerForSettings(t) + + body := `{"provider":"bogus","model":"x"}` + req := httptest.NewRequest(http.MethodPut, "/api/settings/skill-builder", strings.NewReader(body)) + w := httptest.NewRecorder() + srv.handlePutSkillBuilderSettings(w, req) + + if w.Code != http.StatusBadRequest { + t.Errorf("status = %d, want 400; body: %s", w.Code, w.Body.String()) + } + if !strings.Contains(w.Body.String(), "unknown provider") { + t.Errorf("error body should mention unknown provider, got: %s", w.Body.String()) + } +} + +func TestPutSkillBuilderSettings_RejectsMissingModel(t *testing.T) { + srv := newTestServerForSettings(t) + + body := `{"provider":"openai"}` + req := httptest.NewRequest(http.MethodPut, "/api/settings/skill-builder", strings.NewReader(body)) + w := httptest.NewRecorder() + srv.handlePutSkillBuilderSettings(w, req) + + if w.Code != http.StatusBadRequest { + t.Errorf("status = %d, want 400", w.Code) + } + if !strings.Contains(w.Body.String(), "model is required") { + t.Errorf("expected model-required error, got: %s", w.Body.String()) + } +} + +func TestPutSkillBuilderSettings_PersistsAPIKeyToEnvFile(t *testing.T) { + srv := newTestServerForSettings(t) + + body := `{"provider":"openai","model":"gpt-4.1","api_key":"sk-from-modal"}` + req := httptest.NewRequest(http.MethodPut, "/api/settings/skill-builder", strings.NewReader(body)) + w := httptest.NewRecorder() + srv.handlePutSkillBuilderSettings(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, body: %s", w.Code, w.Body.String()) + } + var resp map[string]any + if err := json.NewDecoder(w.Body).Decode(&resp); err != nil { + t.Fatalf("decode: %v", err) + } + if has, _ := resp["has_key"].(bool); !has { + t.Errorf("has_key should be true after persisting key, got %+v", resp) + } + + // .env file written under .forge/ with mode 0600 and the right key. + envPath := filepath.Join(srv.cfg.WorkDir, ".forge", ".env") + raw, err := os.ReadFile(envPath) + if err != nil { + t.Fatalf("read env: %v", err) + } + if !strings.Contains(string(raw), "OPENAI_API_KEY=sk-from-modal") { + t.Errorf("env file missing key:\n%s", raw) + } + if info, _ := os.Stat(envPath); info.Mode().Perm() != 0o600 { + t.Errorf("env file perm = %o, want 0600", info.Mode().Perm()) + } + + // .gitignore for .forge/ auto-created. + giPath := filepath.Join(srv.cfg.WorkDir, ".forge", ".gitignore") + gi, err := os.ReadFile(giPath) + if err != nil { + t.Fatalf("read gitignore: %v", err) + } + if !strings.Contains(string(gi), ".env") { + t.Errorf(".gitignore should protect .env:\n%s", gi) + } + + // The api_key value MUST NOT leak into ui.yaml. + yamlPath := filepath.Join(srv.cfg.WorkDir, ".forge", "ui.yaml") + yamlRaw, _ := os.ReadFile(yamlPath) + if strings.Contains(string(yamlRaw), "sk-from-modal") { + t.Errorf("API key leaked into ui.yaml — must be .env only:\n%s", yamlRaw) + } +} + +func TestPutSkillBuilderSettings_APIKeyUsesCustomEnvVarName(t *testing.T) { + srv := newTestServerForSettings(t) + + body := `{"provider":"openai","model":"gpt-4.1","api_key_env":"WORKSPACE_LLM_KEY","api_key":"sk-custom"}` + req := httptest.NewRequest(http.MethodPut, "/api/settings/skill-builder", strings.NewReader(body)) + w := httptest.NewRecorder() + srv.handlePutSkillBuilderSettings(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, body: %s", w.Code, w.Body.String()) + } + + envPath := filepath.Join(srv.cfg.WorkDir, ".forge", ".env") + raw, _ := os.ReadFile(envPath) + if !strings.Contains(string(raw), "WORKSPACE_LLM_KEY=sk-custom") { + t.Errorf("expected key under custom env name, got:\n%s", raw) + } + if strings.Contains(string(raw), "OPENAI_API_KEY=") { + t.Errorf("default OPENAI_API_KEY should NOT be written when api_key_env is set:\n%s", raw) + } +} + +func TestPutSkillBuilderSettings_OmitAPIKeyLeavesEnvFileUntouched(t *testing.T) { + srv := newTestServerForSettings(t) + + // First write a key. + _ = srv // satisfy linter + body := `{"provider":"openai","model":"gpt-4.1","api_key":"sk-original"}` + w := httptest.NewRecorder() + srv.handlePutSkillBuilderSettings(w, httptest.NewRequest(http.MethodPut, "/api/settings/skill-builder", strings.NewReader(body))) + if w.Code != http.StatusOK { + t.Fatalf("first PUT failed: %d %s", w.Code, w.Body.String()) + } + + // Second PUT updates the model but omits api_key. The .env file + // should keep the original key. + body2 := `{"provider":"openai","model":"gpt-4.1-mini"}` + w2 := httptest.NewRecorder() + srv.handlePutSkillBuilderSettings(w2, httptest.NewRequest(http.MethodPut, "/api/settings/skill-builder", strings.NewReader(body2))) + if w2.Code != http.StatusOK { + t.Fatalf("second PUT failed: %d %s", w2.Code, w2.Body.String()) + } + + raw, _ := os.ReadFile(filepath.Join(srv.cfg.WorkDir, ".forge", ".env")) + if !strings.Contains(string(raw), "OPENAI_API_KEY=sk-original") { + t.Errorf("omit-api_key second PUT should preserve existing key, got:\n%s", raw) + } +} + +func TestGetSkillBuilderSettings_AfterPut_ReturnsWorkspaceSource(t *testing.T) { + srv := newTestServerForSettings(t) + + put := `{"provider":"anthropic","model":"claude-sonnet-4"}` + wp := httptest.NewRecorder() + srv.handlePutSkillBuilderSettings(wp, httptest.NewRequest(http.MethodPut, "/api/settings/skill-builder", strings.NewReader(put))) + if wp.Code != http.StatusOK { + t.Fatalf("PUT failed: %d %s", wp.Code, wp.Body.String()) + } + + wg := httptest.NewRecorder() + srv.handleGetSkillBuilderSettings(wg, httptest.NewRequest(http.MethodGet, "/api/settings/skill-builder", nil)) + if wg.Code != http.StatusOK { + t.Fatalf("GET failed: %d %s", wg.Code, wg.Body.String()) + } + var resp map[string]any + _ = json.NewDecoder(wg.Body).Decode(&resp) + if resp["source"] != "workspace" { + t.Errorf("source = %q, want workspace", resp["source"]) + } + if resp["provider"] != "anthropic" || resp["model"] != "claude-sonnet-4" { + t.Errorf("GET response did not reflect PUT: %+v", resp) + } +} diff --git a/forge-ui/handlers_skill_builder.go b/forge-ui/handlers_skill_builder.go index 46e497f..753296c 100644 --- a/forge-ui/handlers_skill_builder.go +++ b/forge-ui/handlers_skill_builder.go @@ -4,27 +4,20 @@ import ( "encoding/json" "fmt" "net/http" - "os" - "path/filepath" "strings" - "github.com/initializ/forge/forge-cli/runtime" - "github.com/initializ/forge/forge-core/llm/oauth" - "github.com/initializ/forge/forge-core/types" + "github.com/initializ/forge/forge-ui/uiconfig" ) -// SkillBuilderCodegenModel returns the preferred code-generation model for the -// given provider. Skill generation is a complex task that benefits from stronger -// models than the agent's default. Falls back to fallback if the provider is unknown. -func SkillBuilderCodegenModel(provider, fallback string) string { - switch provider { - case "openai": - return "gpt-4.1" - case "anthropic": - return "claude-opus-4-6" - default: - return fallback - } +// SkillBuilderCodegenModel previously hardcoded "gpt-4.1" / "claude-opus-4-6" +// regardless of the agent's configured model. Issue #92 removed that override: +// the skill builder now uses the operator-chosen model from workspace-level +// ui.yaml (see uiconfig.LoadSkillBuilderLLM). The function is retained as a +// no-op shim with a deprecation marker so any out-of-tree callers fail loudly. +// +// Deprecated: skill-builder model selection is now driven by uiconfig. +func SkillBuilderCodegenModel(_, configured string) string { + return configured } // resolveAgentDir extracts agent ID from the request, looks up the agent, @@ -51,82 +44,35 @@ func (s *UIServer) resolveAgentDir(w http.ResponseWriter, r *http.Request) strin return agent.Directory } -// handleSkillBuilderProvider returns the agent's LLM provider info. +// handleSkillBuilderProvider reports the resolved skill-builder LLM +// configuration. Workspace-level config (forge-ui/uiconfig) is the +// primary source; the agent's forge.yaml is consulted only when no +// workspace/user config exists (deprecated fallback). The handler +// never mutates the UI process's environment. func (s *UIServer) handleSkillBuilderProvider(w http.ResponseWriter, r *http.Request) { - agentDir := s.resolveAgentDir(w, r) - if agentDir == "" { - return - } - - configPath := filepath.Join(agentDir, "forge.yaml") - data, err := os.ReadFile(configPath) - if err != nil { - writeError(w, http.StatusInternalServerError, "reading config: "+err.Error()) - return + // agentDir is only used for the deprecated fallback path. It's + // optional — first-run flow (no agent picked yet) is supported. + agentDir := "" + if r.PathValue("id") != "" { + agentDir = s.resolveAgentDir(w, r) + if agentDir == "" { + return // resolveAgentDir wrote the error response + } } - cfg, err := types.ParseForgeConfig(data) + llm, err := uiconfig.LoadSkillBuilderLLM(s.cfg.WorkDir, agentDir, uiconfig.EnvLookupForWorkspace(s.cfg.WorkDir)) if err != nil { - writeError(w, http.StatusInternalServerError, "parsing config: "+err.Error()) + writeError(w, http.StatusInternalServerError, "loading skill-builder config: "+err.Error()) return } - provider := cfg.Model.Provider - - // Load the agent's .env and encrypted secrets so we can check for API keys - // that aren't in the UI process's own environment. - envPath := filepath.Join(agentDir, ".env") - envVars, _ := runtime.LoadEnvFile(envPath) - for k, v := range envVars { - // Don't pollute process env with __oauth__ sentinels — they block - // OverlaySecretsToEnv from replacing them with real keys later. - if v == "__oauth__" { - continue - } - if os.Getenv(k) == "" { - _ = os.Setenv(k, v) - } - } - runtime.OverlaySecretsToEnv(cfg, agentDir) - - // Check if the provider's API key env var is set (excluding __oauth__ sentinel) - keyVal := func(k string) bool { - v := os.Getenv(k) - return v != "" && v != "__oauth__" - } - hasKey := false - isOAuth := false - switch provider { - case "openai": - hasKey = keyVal("OPENAI_API_KEY") - if !hasKey { - // Check for stored OAuth credentials - if token, err := oauth.LoadCredentials("openai"); err == nil && token != nil && token.RefreshToken != "" { - hasKey = true - isOAuth = true - } - } - case "anthropic": - hasKey = keyVal("ANTHROPIC_API_KEY") - case "gemini": - hasKey = keyVal("GEMINI_API_KEY") - case "ollama": - hasKey = true // Ollama doesn't need an API key - default: - hasKey = keyVal("LLM_API_KEY") || keyVal("MODEL_API_KEY") - } - - // OAuth/Codex backend has model restrictions — use agent's configured model. - // API key clients get upgraded to a stronger codegen model. - model := cfg.Model.Name - if !isOAuth { - model = SkillBuilderCodegenModel(provider, model) - } - writeJSON(w, http.StatusOK, map[string]any{ - "provider": provider, - "model": model, - "has_key": hasKey, + "provider": llm.Provider, + "model": llm.Model, + "base_url": llm.BaseURL, + "has_key": llm.HasCredentials(), + "source": llm.Source, + "warning": llm.Warning, }) } @@ -160,6 +106,29 @@ func (s *UIServer) handleSkillBuilderChat(w http.ResponseWriter, r *http.Request return } + // Resolve the workspace-level skill-builder LLM ONCE per request and + // pass it through LLMStreamOptions. The forge-cli callback consumes + // LLM directly — it must not re-read the agent's forge.yaml / .env, + // since that would re-introduce the per-agent env-stomping the + // workspace-LLM design replaced (issue #92). + llm, err := uiconfig.LoadSkillBuilderLLM(s.cfg.WorkDir, agentDir, uiconfig.EnvLookupForWorkspace(s.cfg.WorkDir)) + if err != nil { + writeError(w, http.StatusInternalServerError, "loading skill-builder config: "+err.Error()) + return + } + if llm.Source == uiconfig.SourceUnset { + writeError(w, http.StatusBadRequest, + "skill-builder LLM is not configured. Open Settings → Skill Builder to pick a provider, model, and API key env var.") + return + } + if !llm.HasCredentials() { + writeError(w, http.StatusBadRequest, fmt.Sprintf( + "skill-builder LLM is configured (%s) but no API key found in env var %q. "+ + "Set that env var in the forge ui process and reload, or change api_key_env in Settings.", + llm.Provider, llm.APIKeyEnv)) + return + } + flusher, ok := w.(http.Flusher) if !ok { writeError(w, http.StatusInternalServerError, "streaming not supported") @@ -173,7 +142,8 @@ func (s *UIServer) handleSkillBuilderChat(w http.ResponseWriter, r *http.Request var fullResponse strings.Builder - err := s.cfg.LLMStreamFunc(r.Context(), LLMStreamOptions{ + err = s.cfg.LLMStreamFunc(r.Context(), LLMStreamOptions{ + LLM: llm, AgentDir: agentDir, SystemPrompt: skillBuilderSystemPrompt, Messages: req.Messages, diff --git a/forge-ui/handlers_skill_builder_envstomp_test.go b/forge-ui/handlers_skill_builder_envstomp_test.go new file mode 100644 index 0000000..daaf069 --- /dev/null +++ b/forge-ui/handlers_skill_builder_envstomp_test.go @@ -0,0 +1,102 @@ +package forgeui + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "testing" +) + +// TestSkillBuilderProvider_DoesNotStompProcessEnvBetweenAgents pins one of +// issue #92's acceptance criteria: switching the selected agent in the UI +// must not modify the forge ui process's environment variables. +// +// Pre-#92 handleSkillBuilderProvider loaded the agent's .env into the UI +// process via os.Setenv (lines 80-89 of the old handler). Picking agent A +// then agent B left A's keys in the process env, then B's keys overwrote +// them — cross-agent leakage of credentials through the shared process +// state. +// +// Post-#92 the handler reads workspace-level config (uiconfig) and never +// touches os.Setenv. This test pins that contract. +func TestSkillBuilderProvider_DoesNotStompProcessEnvBetweenAgents(t *testing.T) { + isolateHome(t) + root := t.TempDir() + + // Two agents in the same workspace, with different OPENAI_API_KEY + // values in their .env files. The pre-#92 handler would have read + // these into the UI process via os.Setenv on each fetch; we assert + // no such mutation happens. + agentA := filepath.Join(root, "agent-a") + if err := os.MkdirAll(agentA, 0o755); err != nil { + t.Fatal(err) + } + writeFile(t, filepath.Join(agentA, "forge.yaml"), `agent_id: agent-a +version: 0.1.0 +framework: forge +model: + provider: openai + name: gpt-4o +`) + writeFile(t, filepath.Join(agentA, ".env"), "OPENAI_API_KEY=key-from-agent-a\n") + + agentB := filepath.Join(root, "agent-b") + if err := os.MkdirAll(agentB, 0o755); err != nil { + t.Fatal(err) + } + writeFile(t, filepath.Join(agentB, "forge.yaml"), `agent_id: agent-b +version: 0.1.0 +framework: forge +model: + provider: openai + name: gpt-4o +`) + writeFile(t, filepath.Join(agentB, ".env"), "OPENAI_API_KEY=key-from-agent-b\n") + + // Snapshot the UI process's env before any handler call. Both agents' + // keys must remain ABSENT throughout — these are agent secrets, not + // UI-process secrets. + for _, k := range []string{"OPENAI_API_KEY"} { + if _, present := os.LookupEnv(k); present { + _ = os.Unsetenv(k) + t.Cleanup(func() { _ = os.Unsetenv(k) }) + } + } + + srv := NewUIServer(UIServerConfig{ + Port: 4200, + WorkDir: root, + }) + + // Fetch provider info for agent A, then agent B. Assert process env + // is unchanged after EACH call (proving no os.Setenv leaks from the + // agent's .env into the UI process). + for _, id := range []string{"agent-a", "agent-b", "agent-a"} { + req := httptest.NewRequest(http.MethodGet, "/api/agents/"+id+"/skill-builder/provider", nil) + req.SetPathValue("id", id) + w := httptest.NewRecorder() + srv.handleSkillBuilderProvider(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("agent %q: status = %d, body: %s", id, w.Code, w.Body.String()) + } + + // Decode response to confirm it parsed. + var resp map[string]any + if err := json.NewDecoder(w.Body).Decode(&resp); err != nil { + t.Fatalf("agent %q: decode: %v", id, err) + } + + // The critical assertion: process env must NOT have absorbed + // either agent's OPENAI_API_KEY. Pre-#92, after the first call + // this would have been "key-from-agent-a"; after the second + // "key-from-agent-b". Post-#92, neither must be set. + got := os.Getenv("OPENAI_API_KEY") + if got == "key-from-agent-a" || got == "key-from-agent-b" { + t.Errorf("after fetching agent %q, OPENAI_API_KEY leaked from agent .env into UI process: %q", + id, got) + } + } +} diff --git a/forge-ui/handlers_skill_builder_test.go b/forge-ui/handlers_skill_builder_test.go index 3217816..b9a0097 100644 --- a/forge-ui/handlers_skill_builder_test.go +++ b/forge-ui/handlers_skill_builder_test.go @@ -11,10 +11,35 @@ import ( "testing" ) +// isolateHome relocates os.UserHomeDir() to a temp directory for the +// duration of the test. Required because uiconfig.LoadSkillBuilderLLM's +// tier-2 fallback resolves the user config via os.UserHomeDir; without +// isolation, a real ~/.forge/ui.yaml on the dev machine would change +// what these tests observe. +func isolateHome(t *testing.T) { + t.Helper() + fake := t.TempDir() + origHome, hadHome := os.LookupEnv("HOME") + if err := os.Setenv("HOME", fake); err != nil { + t.Fatalf("setenv HOME: %v", err) + } + t.Cleanup(func() { + if hadHome { + _ = os.Setenv("HOME", origHome) + } else { + _ = os.Unsetenv("HOME") + } + }) +} + func setupTestServerWithSkillBuilder(t *testing.T) (*UIServer, string) { t.Helper() root := t.TempDir() + // Isolate HOME so uiconfig's tier-2 user fallback can't accidentally + // pick up the dev machine's ~/.forge/ui.yaml during tests. + isolateHome(t) + // Create test agent agentDir := filepath.Join(root, "test-agent") if err := os.MkdirAll(agentDir, 0o755); err != nil { @@ -82,15 +107,32 @@ func TestSkillBuilderProvider(t *testing.T) { if resp["provider"] != "openai" { t.Errorf("provider = %q, want %q", resp["provider"], "openai") } - // Model is gpt-4.1 (API key codegen upgrade) or the agent's configured - // model (OAuth — Codex backend has model restrictions). - model := resp["model"].(string) - if model != "gpt-4.1" && model != "gpt-4o" { - t.Errorf("model = %q, want %q or %q", model, "gpt-4.1", "gpt-4o") + // Per issue #92: no hardcoded codegen upgrade. The agent-fallback path + // returns the operator's configured model verbatim. (Pre-#92 this was + // gpt-4.1 regardless of agent config.) + if model, _ := resp["model"].(string); model != "gpt-4o" { + t.Errorf("model = %q, want %q (no codegen upgrade)", model, "gpt-4o") + } + // Falling through to agent fallback should surface the deprecation + // warning so the UI can prompt the operator to configure workspace + // settings. + if source, _ := resp["source"].(string); source != "agent_fallback" { + t.Errorf("source = %q, want agent_fallback", source) + } + if warning, _ := resp["warning"].(string); warning == "" { + t.Errorf("agent_fallback path should emit a deprecation warning") } } -func TestSkillBuilderProviderAnthropicOverride(t *testing.T) { +// TestSkillBuilderProvider_AgentFallback_PreservesConfiguredModel pins the +// post-#92 behavior: the skill builder reports the operator-configured +// model verbatim — no SkillBuilderCodegenModel upgrade to claude-opus-4-6 +// (or gpt-4.1 for openai). Pre-#92 the agent's configured model was +// overridden for the skill builder's LLM call, which broke any agent +// pointed at a custom OpenAI-compatible endpoint that didn't host the +// hardcoded "stronger" model. +func TestSkillBuilderProvider_AgentFallback_PreservesConfiguredModel(t *testing.T) { + isolateHome(t) root := t.TempDir() agentDir := filepath.Join(root, "anthropic-agent") @@ -128,8 +170,8 @@ model: if resp["provider"] != "anthropic" { t.Errorf("provider = %q, want %q", resp["provider"], "anthropic") } - if resp["model"] != "claude-opus-4-6" { - t.Errorf("model = %q, want %q", resp["model"], "claude-opus-4-6") + if resp["model"] != "claude-sonnet-4-20250514" { + t.Errorf("model = %q, want %q (no codegen upgrade)", resp["model"], "claude-sonnet-4-20250514") } } diff --git a/forge-ui/server.go b/forge-ui/server.go index 613c9a5..b4d4a2e 100644 --- a/forge-ui/server.go +++ b/forge-ui/server.go @@ -101,6 +101,13 @@ func (s *UIServer) Start(ctx context.Context) error { mux.HandleFunc("POST /api/agents/{id}/skill-builder/save", s.handleSkillBuilderSave) mux.HandleFunc("GET /api/agents/{id}/skill-builder/context", s.handleSkillBuilderContext) mux.HandleFunc("GET /api/agents/{id}/skill-builder/provider", s.handleSkillBuilderProvider) + // Workspace-level skill-builder settings (issue #92). The + // path-less /api/skill-builder/provider lets the UI query the + // resolved config before any agent is picked — needed for first-run + // in an empty workspace. + mux.HandleFunc("GET /api/skill-builder/provider", s.handleSkillBuilderProvider) + mux.HandleFunc("GET /api/settings/skill-builder", s.handleGetSkillBuilderSettings) + mux.HandleFunc("PUT /api/settings/skill-builder", s.handlePutSkillBuilderSettings) // Static file serving with SPA fallback. The embedded FS is rooted // directly at the static assets — no "dist/" subdirectory (review #8 diff --git a/forge-ui/static/app.js b/forge-ui/static/app.js index 2bf64ce..cea1d87 100644 --- a/forge-ui/static/app.js +++ b/forge-ui/static/app.js @@ -2254,6 +2254,159 @@ function SkillsPage() { `; } +// ── Skill Builder Settings (issue #92) ─────────────────────── + +async function fetchSkillBuilderSettings() { + const res = await fetch('/api/settings/skill-builder'); + if (!res.ok) throw new Error(`Failed to fetch settings: ${res.status}`); + return res.json(); +} + +async function saveSkillBuilderSettings(body) { + const res = await fetch('/api/settings/skill-builder', { + method: 'PUT', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(body), + }); + if (!res.ok) { + const err = await res.json().catch(() => ({})); + throw new Error(err.error || `Save failed: ${res.status}`); + } + return res.json(); +} + +// SkillBuilderSettingsModal lets the operator pick the workspace- +// level LLM the skill builder uses, decoupled from any agent's +// runtime LLM. Persists to /.forge/ui.yaml via the +// PUT /api/settings/skill-builder endpoint. +function SkillBuilderSettingsModal({ initial, onClose, onSaved }) { + const [form, setForm] = useState({ + provider: (initial && initial.provider) || 'openai', + model: (initial && initial.model) || '', + base_url: (initial && initial.base_url) || '', + api_key_env: (initial && initial.api_key_env) || '', + }); + // API key is intentionally a separate piece of state — never persisted + // to ui.yaml, never echoed back from the server. Left blank on every + // open of the modal so an existing key isn't shown (or shadowed by an + // empty input). Submitting an empty api_key leaves the saved value + // untouched; submit a new value to rotate. + const [apiKey, setApiKey] = useState(''); + const [showKey, setShowKey] = useState(false); + const [hasKey, setHasKey] = useState((initial && initial.has_key) || false); + const [saving, setSaving] = useState(false); + const [err, setErr] = useState(null); + + // Reload from server when modal opens so we see the persisted state, + // not the partially-fallback shape that fetchSkillBuilderProvider + // returned (which folds in agent-fallback fields). + useEffect(() => { + fetchSkillBuilderSettings() + .then((s) => { + setForm({ + provider: s.provider || 'openai', + model: s.model || '', + base_url: s.base_url || '', + api_key_env: s.api_key_env || '', + }); + setHasKey(!!s.has_key); + }) + .catch((e) => setErr(e.message)); + }, []); + + async function submit() { + setSaving(true); + setErr(null); + try { + // Build the request body. Include api_key only when the operator + // typed one — empty string means "leave the existing key alone." + const body = { ...form }; + if (apiKey) body.api_key = apiKey; + const saved = await saveSkillBuilderSettings(body); + onSaved && onSaved(saved); + } catch (e) { + setErr(e.message); + } finally { + setSaving(false); + } + } + + const update = (k, v) => setForm({ ...form, [k]: v }); + + return html` +
+
e.stopPropagation()} style="max-width: 540px;"> +
+

Skill Builder LLM

+ +
+
+

+ Workspace-level LLM used to generate skills. Independent of any + specific agent's runtime LLM. Persisted to + <workspace>/.forge/ui.yaml. +

+ + + + + + update('model', e.target.value)} /> + + ${form.provider === 'openai' && html` + + update('base_url', e.target.value)} /> +
+ Set this for OpenAI-compatible endpoints (OpenRouter, vLLM, litellm). +
+ `} + + + update('api_key_env', e.target.value)} /> +
+ Name of the environment variable the forge ui process reads for the API key. + Leave blank to use the provider default. +
+ + ${form.provider !== 'ollama' && html` + +
+ setApiKey(e.target.value)} style="flex: 1;" /> + +
+
+ Stored at <workspace>/.forge/.env (mode 0600). + An auto-generated .forge/.gitignore protects it from being committed. +
+ `} + + ${err && html`
${err}
`} +
+
+ + +
+
+
+ `; +} + // ── Skill Builder Page ─────────────────────────────────────── function SkillBuilderPage({ agentId }) { @@ -2268,6 +2421,7 @@ function SkillBuilderPage({ agentId }) { const [saveStatus, setSaveStatus] = useState(null); const [envInputs, setEnvInputs] = useState({}); const [error, setError] = useState(null); + const [showSettings, setShowSettings] = useState(false); const abortRef = useRef(null); const chatEndRef = useRef(null); const editorRef = useRef(null); @@ -2429,11 +2583,19 @@ function SkillBuilderPage({ agentId }) {

Skill Builder

${provider && html` -
- ${provider.provider}/${provider.model} - ${!provider.has_key && html`API key not configured`} +
+ ${provider.source === 'unset' ? html` + Workspace skill-builder LLM is not configured. + + ` : html` + ${provider.provider}/${provider.model} + ${provider.source === 'agent_fallback' && html`using agent fallback (deprecated)`} + ${!provider.has_key && html`API key not configured (env: ${provider.api_key_env || 'unset'})`} + + `}
`} + ${showSettings && html`<${SkillBuilderSettingsModal} initial=${provider} onClose=${() => setShowSettings(false)} onSaved=${(updated) => { setProvider(updated); setShowSettings(false); }} />`}
diff --git a/forge-ui/types.go b/forge-ui/types.go index 8f5e4db..692d315 100644 --- a/forge-ui/types.go +++ b/forge-ui/types.go @@ -3,6 +3,8 @@ package forgeui import ( "context" "time" + + "github.com/initializ/forge/forge-ui/uiconfig" ) // ProcessState represents the lifecycle state of an agent process. @@ -79,7 +81,20 @@ type SkillBuilderMessage struct { } // LLMStreamOptions configures a streaming LLM call for the skill builder. +// +// The LLM struct is the resolved skill-builder LLM configuration — +// workspace-level (per issue #92) when available, with the agent- +// fallback path used only when no workspace/user config exists. +// Callers in forge-cli MUST consume LLM directly rather than re-reading +// the agent's forge.yaml / .env: doing so would re-introduce the +// per-agent env-stomping the workspace-LLM design replaced. +// +// AgentDir is retained for the deprecated fallback resolution path +// only — forge-ui passes it so the loader can read the agent's +// forge.yaml when no workspace/user config exists. New code paths +// should not depend on it. type LLMStreamOptions struct { + LLM uiconfig.SkillBuilderLLM AgentDir string SystemPrompt string Messages []SkillBuilderMessage diff --git a/forge-ui/uiconfig/envfile.go b/forge-ui/uiconfig/envfile.go new file mode 100644 index 0000000..0c65e48 --- /dev/null +++ b/forge-ui/uiconfig/envfile.go @@ -0,0 +1,164 @@ +package uiconfig + +import ( + "bytes" + "fmt" + "os" + "path/filepath" + "sort" +) + +// EnvFileName is the workspace-level .env file the UI process consults +// for secret values (per-key API tokens, etc.) configured via Settings. +// Lives under /.forge/.env so it's grouped with ui.yaml and +// auto-protected by the .forge/.gitignore the loader writes next to it. +const EnvFileName = ".env" + +// EnvLookupForWorkspace returns a lookup function suitable for passing +// to LoadSkillBuilderLLM. The workspace .env takes precedence over the +// OS environment for any key it defines; otherwise OS env is consulted. +// Missing file → OS env only (a no-op layer). +// +// The function takes a fresh snapshot at call time — Settings writes +// are picked up by the next request without restarting forge ui. +func EnvLookupForWorkspace(workspaceDir string) func(string) string { + path := filepath.Join(workspaceDir, WorkspaceConfigDir, EnvFileName) + fileEnv, _ := readDotEnv(path) // missing file → empty map; ignore error + return func(name string) string { + if v, ok := fileEnv[name]; ok { + return v + } + return os.Getenv(name) + } +} + +// SetEnvFileValue writes (or updates) a single KEY=VALUE pair in the +// workspace .env file. Atomic via temp-file + rename; permissions 0600 +// so the file is readable only by the owning user. Creates the parent +// .forge/ directory if missing. +// +// Lines other than the target key are preserved verbatim — including +// formatting, comments, and the file's trailing newline (or absence +// thereof). This is the "edit-in-place" guarantee operators expect +// from a Settings UI. +// +// If value is the empty string, the key is REMOVED from the file (so +// the operator can clear a credential without leaving a dangling +// KEY= line). +func SetEnvFileValue(workspaceDir, name, value string) error { + if name == "" { + return fmt.Errorf("env key name is required") + } + dir := filepath.Join(workspaceDir, WorkspaceConfigDir) + if err := os.MkdirAll(dir, 0o755); err != nil { + return fmt.Errorf("creating %s: %w", dir, err) + } + path := filepath.Join(dir, EnvFileName) + + raw, err := os.ReadFile(path) + if err != nil && !os.IsNotExist(err) { + return fmt.Errorf("reading %s: %w", path, err) + } + + updated, replaced := mutateEnvLines(raw, name, value) + if !replaced && value != "" { + // New key — append with a leading newline if the file is + // non-empty and doesn't already end in one. + if len(updated) > 0 && updated[len(updated)-1] != '\n' { + updated = append(updated, '\n') + } + updated = append(updated, []byte(fmt.Sprintf("%s=%s\n", name, value))...) + } + + // Atomic write via temp file in the same directory. + tmp, err := os.CreateTemp(dir, ".env.tmp.*") + if err != nil { + return fmt.Errorf("creating temp file: %w", err) + } + tmpPath := tmp.Name() + defer func() { _ = os.Remove(tmpPath) }() // no-op once rename succeeds + + if _, err := tmp.Write(updated); err != nil { + _ = tmp.Close() + return fmt.Errorf("writing temp file: %w", err) + } + if err := tmp.Chmod(0o600); err != nil { + _ = tmp.Close() + return fmt.Errorf("chmod temp file: %w", err) + } + if err := tmp.Close(); err != nil { + return fmt.Errorf("closing temp file: %w", err) + } + if err := os.Rename(tmpPath, path); err != nil { + return fmt.Errorf("renaming to %s: %w", path, err) + } + + // Best-effort gitignore so the file isn't accidentally committed. + // We DO NOT touch a workspace-level .gitignore (that's the + // operator's repo); we write our own .forge/.gitignore which is + // scoped to the .forge/ directory we own. + _ = writeForgeGitignore(dir) + return nil +} + +// mutateEnvLines edits raw .env bytes: replaces the line that begins +// with "=" with "=", or removes it entirely when +// value is empty. Returns the new bytes and a flag indicating whether +// the key was found in the file (so the caller can decide to append). +func mutateEnvLines(raw []byte, name, value string) (out []byte, replaced bool) { + prefix := []byte(name + "=") + lines := bytes.SplitAfter(raw, []byte("\n")) + var buf bytes.Buffer + for _, line := range lines { + trimmed := bytes.TrimLeft(line, " \t") + if bytes.HasPrefix(trimmed, prefix) { + replaced = true + if value == "" { + continue // delete + } + buf.WriteString(name) + buf.WriteString("=") + buf.WriteString(value) + // Preserve trailing newline if the original line had one. + if bytes.HasSuffix(line, []byte("\n")) { + buf.WriteByte('\n') + } + continue + } + buf.Write(line) + } + return buf.Bytes(), replaced +} + +// writeForgeGitignore makes sure /.forge/.gitignore exists +// and contains ".env" so the secret file is auto-protected. Idempotent. +func writeForgeGitignore(forgeDir string) error { + path := filepath.Join(forgeDir, ".gitignore") + const want = ".env\n" + if existing, err := os.ReadFile(path); err == nil { + if bytes.Contains(existing, []byte(".env")) { + return nil // already protected + } + // Append our line, preserving operator additions. + combined := existing + if len(combined) > 0 && combined[len(combined)-1] != '\n' { + combined = append(combined, '\n') + } + combined = append(combined, want...) + return os.WriteFile(path, combined, 0o644) + } + return os.WriteFile(path, []byte(want), 0o644) +} + +// SortedEnvFileKeys returns the keys present in the workspace .env, +// sorted. Useful for tests + diagnostics; not used in the hot path. +func SortedEnvFileKeys(workspaceDir string) []string { + path := filepath.Join(workspaceDir, WorkspaceConfigDir, EnvFileName) + m, _ := readDotEnv(path) + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} diff --git a/forge-ui/uiconfig/envfile_test.go b/forge-ui/uiconfig/envfile_test.go new file mode 100644 index 0000000..7b43029 --- /dev/null +++ b/forge-ui/uiconfig/envfile_test.go @@ -0,0 +1,200 @@ +package uiconfig + +import ( + "io/fs" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestSetEnvFileValue_CreatesFileAndGitignore(t *testing.T) { + workspace := t.TempDir() + + if err := SetEnvFileValue(workspace, "OPENAI_API_KEY", "sk-test"); err != nil { + t.Fatalf("Set: %v", err) + } + + envPath := filepath.Join(workspace, ".forge", ".env") + raw, err := os.ReadFile(envPath) + if err != nil { + t.Fatalf("read env file: %v", err) + } + if !strings.Contains(string(raw), "OPENAI_API_KEY=sk-test") { + t.Errorf("env file missing key:\n%s", raw) + } + + // Permissions: 0600 (readable only by owner). + info, err := os.Stat(envPath) + if err != nil { + t.Fatalf("stat: %v", err) + } + if mode := info.Mode().Perm(); mode != 0o600 { + t.Errorf("env file perm = %o, want 0600", mode) + } + + // .gitignore auto-created next to .env. + giPath := filepath.Join(workspace, ".forge", ".gitignore") + gi, err := os.ReadFile(giPath) + if err != nil { + t.Fatalf("read gitignore: %v", err) + } + if !strings.Contains(string(gi), ".env") { + t.Errorf(".gitignore should contain .env, got:\n%s", gi) + } +} + +func TestSetEnvFileValue_UpdatesInPlace(t *testing.T) { + workspace := t.TempDir() + + for _, val := range []string{"sk-first", "sk-second", "sk-third"} { + if err := SetEnvFileValue(workspace, "OPENAI_API_KEY", val); err != nil { + t.Fatalf("Set %q: %v", val, err) + } + } + + raw, _ := os.ReadFile(filepath.Join(workspace, ".forge", ".env")) + // Only one line per key, not three appended copies. + count := strings.Count(string(raw), "OPENAI_API_KEY=") + if count != 1 { + t.Errorf("expected one OPENAI_API_KEY line, got %d:\n%s", count, raw) + } + if !strings.Contains(string(raw), "OPENAI_API_KEY=sk-third") { + t.Errorf("expected latest value, got:\n%s", raw) + } +} + +func TestSetEnvFileValue_EmptyValueRemovesKey(t *testing.T) { + workspace := t.TempDir() + + _ = SetEnvFileValue(workspace, "OPENAI_API_KEY", "sk-x") + _ = SetEnvFileValue(workspace, "OTHER_KEY", "stays") + if err := SetEnvFileValue(workspace, "OPENAI_API_KEY", ""); err != nil { + t.Fatalf("clear: %v", err) + } + + raw, _ := os.ReadFile(filepath.Join(workspace, ".forge", ".env")) + if strings.Contains(string(raw), "OPENAI_API_KEY") { + t.Errorf("cleared key should be gone:\n%s", raw) + } + if !strings.Contains(string(raw), "OTHER_KEY=stays") { + t.Errorf("unrelated key should remain:\n%s", raw) + } +} + +func TestSetEnvFileValue_PreservesOtherKeysAndComments(t *testing.T) { + workspace := t.TempDir() + + // Pre-populate the .env file with comments + unrelated keys. + dir := filepath.Join(workspace, ".forge") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + initial := "# header comment\nOTHER_KEY=value1\n# inline comment\nANOTHER=value2\n" + if err := os.WriteFile(filepath.Join(dir, ".env"), []byte(initial), 0o644); err != nil { + t.Fatal(err) + } + + if err := SetEnvFileValue(workspace, "OPENAI_API_KEY", "sk-new"); err != nil { + t.Fatalf("Set: %v", err) + } + + raw, _ := os.ReadFile(filepath.Join(dir, ".env")) + got := string(raw) + for _, want := range []string{ + "# header comment", + "OTHER_KEY=value1", + "# inline comment", + "ANOTHER=value2", + "OPENAI_API_KEY=sk-new", + } { + if !strings.Contains(got, want) { + t.Errorf("expected %q in env file:\n%s", want, got) + } + } +} + +func TestEnvLookupForWorkspace_FilePrecedesOSEnv(t *testing.T) { + workspace := t.TempDir() + _ = SetEnvFileValue(workspace, "OPENAI_API_KEY", "sk-from-file") + + // Plant a colliding value in OS env. The file should win for the + // keys it defines. + t.Setenv("OPENAI_API_KEY", "sk-from-os") + t.Setenv("UNRELATED", "stays-os-only") + + lookup := EnvLookupForWorkspace(workspace) + + if got := lookup("OPENAI_API_KEY"); got != "sk-from-file" { + t.Errorf("file should win for defined key, got %q", got) + } + if got := lookup("UNRELATED"); got != "stays-os-only" { + t.Errorf("OS env should win for undefined-in-file key, got %q", got) + } +} + +func TestEnvLookupForWorkspace_MissingFileFallsBackToOS(t *testing.T) { + workspace := t.TempDir() // no .forge/.env + + t.Setenv("OPENAI_API_KEY", "sk-from-os") + + lookup := EnvLookupForWorkspace(workspace) + if got := lookup("OPENAI_API_KEY"); got != "sk-from-os" { + t.Errorf("missing file should pass through to OS env, got %q", got) + } +} + +// Pin that the gitignore append doesn't duplicate the .env line when +// SetEnvFileValue is called multiple times. +func TestSetEnvFileValue_GitignoreIdempotent(t *testing.T) { + workspace := t.TempDir() + _ = SetEnvFileValue(workspace, "K1", "v1") + _ = SetEnvFileValue(workspace, "K2", "v2") + _ = SetEnvFileValue(workspace, "K3", "v3") + + gi, _ := os.ReadFile(filepath.Join(workspace, ".forge", ".gitignore")) + if c := strings.Count(string(gi), ".env"); c != 1 { + t.Errorf("expected one .env line in .gitignore, got %d:\n%s", c, gi) + } +} + +func TestSetEnvFileValue_PreservesExistingGitignoreLines(t *testing.T) { + workspace := t.TempDir() + dir := filepath.Join(workspace, ".forge") + _ = os.MkdirAll(dir, 0o755) + _ = os.WriteFile(filepath.Join(dir, ".gitignore"), []byte("ignore-this-too\n"), 0o644) + + if err := SetEnvFileValue(workspace, "K", "v"); err != nil { + t.Fatalf("Set: %v", err) + } + gi, _ := os.ReadFile(filepath.Join(dir, ".gitignore")) + if !strings.Contains(string(gi), "ignore-this-too") { + t.Errorf("operator's existing gitignore line was lost:\n%s", gi) + } + if !strings.Contains(string(gi), ".env") { + t.Errorf("our .env line should be appended:\n%s", gi) + } +} + +// Sanity: SortedEnvFileKeys reflects what was written. +func TestSortedEnvFileKeys(t *testing.T) { + workspace := t.TempDir() + _ = SetEnvFileValue(workspace, "BBB", "1") + _ = SetEnvFileValue(workspace, "AAA", "2") + + keys := SortedEnvFileKeys(workspace) + if len(keys) != 2 || keys[0] != "AAA" || keys[1] != "BBB" { + t.Errorf("SortedEnvFileKeys = %v, want [AAA BBB]", keys) + } +} + +// Sanity: confirm we can interoperate with the standard fs.FileMode +// constant rather than literal octals (catches a silly typo). +func TestSetEnvFileValue_PermConstSanity(t *testing.T) { + workspace := t.TempDir() + _ = SetEnvFileValue(workspace, "K", "v") + info, _ := os.Stat(filepath.Join(workspace, ".forge", ".env")) + if info.Mode()&fs.ModePerm != 0o600 { + t.Errorf("perm = %v, want 0600", info.Mode()&fs.ModePerm) + } +} diff --git a/forge-ui/uiconfig/uiconfig.go b/forge-ui/uiconfig/uiconfig.go new file mode 100644 index 0000000..a8ccdbd --- /dev/null +++ b/forge-ui/uiconfig/uiconfig.go @@ -0,0 +1,395 @@ +// Package uiconfig holds the workspace-level configuration that the +// forge ui process consumes — independent of any specific agent's +// forge.yaml. +// +// Today the only such configuration is the skill-builder LLM (see +// SkillBuilderLLM). The skill builder is a workspace-level activity: +// an operator might build a shared skill before any agent exists, or +// build one skill they will drop into several agents. Tying its +// credentials to a picked agent — which is what the UI did before +// issue #92 — conflates "this agent's runtime LLM" with "the build- +// time codegen LLM I use to author skills" and produces a string of +// problems documented in that issue. +// +// The loader at LoadSkillBuilderLLM resolves the configuration through +// a three-tier precedence: +// +// 1. /.forge/ui.yaml — primary, per-workspace. +// 2. ~/.forge/ui.yaml — fallback, operator's machine-wide. +// 3. The picked agent's forge.yaml + .env — last-resort compat, +// deprecated. The loader returns Source="agent_fallback" with +// Warning set so the UI banner can prompt the operator to +// configure workspace settings. +// +// Stage 3 keeps existing workflows alive during the transition; we +// can drop it after one release cycle. +package uiconfig + +import ( + "fmt" + "os" + "path/filepath" + + "gopkg.in/yaml.v3" +) + +// File names + on-disk layout. +const ( + // WorkspaceConfigDir is the per-workspace directory we look in. + WorkspaceConfigDir = ".forge" + // UIConfigFileName is the filename for the workspace-level config. + UIConfigFileName = "ui.yaml" + // UserConfigDirName is the directory under the user's home where + // the fallback config lives. Matches `.forge` for symmetry with + // the workspace dir. + UserConfigDirName = ".forge" +) + +// Source identifies which resolution tier the loader picked. +const ( + SourceWorkspace = "workspace" + SourceUser = "user" + SourceAgentFallback = "agent_fallback" + SourceUnset = "unset" +) + +// File is the on-disk shape of /.forge/ui.yaml. Future +// workspace-level config sections live here too. +type File struct { + SkillBuilder *SkillBuilderConfig `yaml:"skill_builder,omitempty"` +} + +// SkillBuilderConfig is the YAML shape of the skill-builder LLM +// configuration that gets persisted. SkillBuilderLLM (below) is the +// resolved runtime view of this, augmented with the actual API key +// looked up from APIKeyEnv. +type SkillBuilderConfig struct { + // Provider names the LLM provider — one of openai, anthropic, + // gemini, ollama. Required. + Provider string `yaml:"provider" json:"provider"` + // Model is the operator-chosen model name. Required. No hardcoded + // upgrade is applied at request time (issue #92 explicitly + // removed the SkillBuilderCodegenModel mapping that forced + // gpt-4.1 / claude-opus-4-6). + Model string `yaml:"model" json:"model"` + // BaseURL is an optional OpenAI-compatible endpoint URL (for + // OpenRouter, vLLM, litellm, etc.). When set with provider=openai, + // requests are routed here rather than the openai.com default. + BaseURL string `yaml:"base_url,omitempty" json:"base_url,omitempty"` + // APIKeyEnv names the environment variable the UI process reads + // for the API key. Defaults per provider (OPENAI_API_KEY, + // ANTHROPIC_API_KEY, GEMINI_API_KEY). Override if the operator + // keeps credentials under a different name (e.g. + // WORKSPACE_LLM_API_KEY). + APIKeyEnv string `yaml:"api_key_env,omitempty" json:"api_key_env,omitempty"` +} + +// SkillBuilderLLM is the resolved-at-request-time view of a +// SkillBuilderConfig. It carries the resolved APIKey but is intended +// to be request-scoped — callers MUST NOT cache it across requests or +// stash the APIKey in process env (which is what the pre-#92 handlers +// did via os.Setenv). +type SkillBuilderLLM struct { + Provider string + Model string + BaseURL string + APIKeyEnv string + APIKey string + // Source records which tier of the precedence ladder this came + // from (workspace, user, agent_fallback, unset). The UI surfaces + // this so operators understand which file to edit + when the + // agent_fallback deprecation prompt should fire. + Source string + // Warning, when non-empty, is a human-readable note the UI should + // display alongside the resolved config (e.g. the agent-fallback + // deprecation message). + Warning string +} + +// HasCredentials reports whether the resolved configuration carries +// a usable API key (or is provider=ollama, which doesn't need one). +func (s SkillBuilderLLM) HasCredentials() bool { + if s.Provider == "ollama" { + return true + } + return s.APIKey != "" && s.APIKey != "__oauth__" +} + +// LoadSkillBuilderLLM resolves the skill-builder LLM through the +// three-tier precedence. workspaceDir is the directory `forge ui` +// was launched against (the same workspace the agent scanner walks). +// agentDir is optional; when non-empty AND no workspace/user config +// exists, the loader falls back to reading the agent's forge.yaml + +// .env shape and surfaces the deprecation warning. +// +// envLookup is injected so tests can stub os.Getenv. Production +// callers pass os.Getenv directly. +func LoadSkillBuilderLLM(workspaceDir, agentDir string, envLookup func(string) string) (SkillBuilderLLM, error) { + if envLookup == nil { + envLookup = os.Getenv + } + + // Tier 1: workspace config. + if cfg, ok, err := readSkillBuilderConfig(filepath.Join(workspaceDir, WorkspaceConfigDir, UIConfigFileName)); err != nil { + return SkillBuilderLLM{}, fmt.Errorf("workspace ui.yaml: %w", err) + } else if ok { + return resolve(cfg, SourceWorkspace, "", envLookup), nil + } + + // Tier 2: user config. + if home, err := os.UserHomeDir(); err == nil { + userPath := filepath.Join(home, UserConfigDirName, UIConfigFileName) + if cfg, ok, err := readSkillBuilderConfig(userPath); err != nil { + return SkillBuilderLLM{}, fmt.Errorf("user ui.yaml: %w", err) + } else if ok { + return resolve(cfg, SourceUser, "", envLookup), nil + } + } + + // Tier 3: agent fallback. Deprecated; warn loudly so operators + // migrate. Only fires when an agent context exists. + if agentDir != "" { + if cfg, ok := readAgentFallback(agentDir, envLookup); ok { + warning := "Skill builder is using the selected agent's LLM credentials. " + + "This fallback is deprecated and will be removed in a future release. " + + "Configure workspace-level skill-builder LLM under Settings → Skill Builder." + return resolve(cfg, SourceAgentFallback, warning, envLookup), nil + } + } + + return SkillBuilderLLM{Source: SourceUnset}, nil +} + +// SaveSkillBuilderLLM persists the skill-builder configuration to +// /.forge/ui.yaml. Creates the directory if missing. +// Overwrites any existing file (preserving non-skill-builder sections +// of the File struct — important once other workspace-level sections +// are added). +func SaveSkillBuilderLLM(workspaceDir string, cfg SkillBuilderConfig) error { + if err := validateSkillBuilderConfig(cfg); err != nil { + return err + } + + dir := filepath.Join(workspaceDir, WorkspaceConfigDir) + if err := os.MkdirAll(dir, 0o755); err != nil { + return fmt.Errorf("creating %s: %w", dir, err) + } + path := filepath.Join(dir, UIConfigFileName) + + // Note: this rewrites the whole file from the typed File struct. + // When other workspace-level sections are added to ui.yaml (beyond + // skill_builder), this needs to switch to a yaml.Node-based + // mutation so unknown sections survive a Save. For v1, skill_builder + // is the only section so simple typed marshal is correct. + existing := File{SkillBuilder: &cfg} + + out, err := yaml.Marshal(&existing) + if err != nil { + return fmt.Errorf("marshaling ui.yaml: %w", err) + } + if err := os.WriteFile(path, out, 0o644); err != nil { + return fmt.Errorf("writing %s: %w", path, err) + } + return nil +} + +// readSkillBuilderConfig reads + parses the given path. Returns +// (cfg, false, nil) when the file is absent (a non-error condition +// — fall through to the next tier). +func readSkillBuilderConfig(path string) (SkillBuilderConfig, bool, error) { + raw, err := os.ReadFile(path) + if err != nil { + if os.IsNotExist(err) { + return SkillBuilderConfig{}, false, nil + } + return SkillBuilderConfig{}, false, err + } + var file File + if err := yaml.Unmarshal(raw, &file); err != nil { + return SkillBuilderConfig{}, false, fmt.Errorf("parsing %s: %w", path, err) + } + if file.SkillBuilder == nil { + return SkillBuilderConfig{}, false, nil + } + return *file.SkillBuilder, true, nil +} + +// readAgentFallback approximates the pre-#92 behavior — read the +// agent's forge.yaml model block + .env. Returns ok=false when the +// agent dir lacks a parseable config. +// +// This deliberately uses a minimal struct rather than depending on +// the full types.ForgeConfig + runtime overlay machinery — we only +// need provider/model/base_url/api_key_env to build the same shape +// the workspace tier produces. +func readAgentFallback(agentDir string, envLookup func(string) string) (SkillBuilderConfig, bool) { + cfgPath := filepath.Join(agentDir, "forge.yaml") + raw, err := os.ReadFile(cfgPath) + if err != nil { + return SkillBuilderConfig{}, false + } + var legacy struct { + Model struct { + Provider string `yaml:"provider"` + Name string `yaml:"name"` + } `yaml:"model"` + } + if err := yaml.Unmarshal(raw, &legacy); err != nil { + return SkillBuilderConfig{}, false + } + if legacy.Model.Provider == "" { + return SkillBuilderConfig{}, false + } + // Try to read base_url from the agent's .env (post-#83 wiring). + // We don't os.Setenv here; just read the file directly. + cfg := SkillBuilderConfig{ + Provider: legacy.Model.Provider, + Model: legacy.Model.Name, + } + if env, err := readDotEnv(filepath.Join(agentDir, ".env")); err == nil { + switch cfg.Provider { + case "openai": + if v, ok := env["OPENAI_BASE_URL"]; ok { + cfg.BaseURL = v + } + } + } + // APIKeyEnv defaulting happens in resolve(); leave empty here. + _ = envLookup + return cfg, true +} + +// readDotEnv parses a KEY=VALUE .env file. Minimal implementation — +// no quoting, no exports, no interpolation. Sufficient for the +// fallback path; we don't want to import the heavier runtime parser +// here (would create an import cycle through forge-cli). +func readDotEnv(path string) (map[string]string, error) { + raw, err := os.ReadFile(path) + if err != nil { + return nil, err + } + out := map[string]string{} + for _, line := range splitLines(string(raw)) { + line = trimSpace(line) + if line == "" || line[0] == '#' { + continue + } + eq := indexByte(line, '=') + if eq <= 0 { + continue + } + out[trimSpace(line[:eq])] = trimSpace(line[eq+1:]) + } + return out, nil +} + +// resolve fills in the runtime view from a persisted config. Sets the +// APIKey by looking up APIKeyEnv (or the provider default) via the +// injected envLookup. +func resolve(cfg SkillBuilderConfig, source, warning string, envLookup func(string) string) SkillBuilderLLM { + out := SkillBuilderLLM{ + Provider: cfg.Provider, + Model: cfg.Model, + BaseURL: cfg.BaseURL, + APIKeyEnv: cfg.APIKeyEnv, + Source: source, + Warning: warning, + } + if out.APIKeyEnv == "" { + out.APIKeyEnv = defaultAPIKeyEnv(cfg.Provider) + } + if out.APIKeyEnv != "" { + out.APIKey = envLookup(out.APIKeyEnv) + } + return out +} + +// defaultAPIKeyEnv returns the conventional env var name for each +// known provider. Empty for ollama (no key needed) and unknowns. +func defaultAPIKeyEnv(provider string) string { + switch provider { + case "openai": + return "OPENAI_API_KEY" + case "anthropic": + return "ANTHROPIC_API_KEY" + case "gemini": + return "GEMINI_API_KEY" + } + return "" +} + +// ValidateSkillBuilderConfig is exported for the settings HTTP +// handler so the same rules apply to file load + API write. +func ValidateSkillBuilderConfig(cfg SkillBuilderConfig) error { + return validateSkillBuilderConfig(cfg) +} + +func validateSkillBuilderConfig(cfg SkillBuilderConfig) error { + switch cfg.Provider { + case "openai", "anthropic", "gemini", "ollama": + case "": + return fmt.Errorf("provider is required") + default: + return fmt.Errorf("unknown provider %q (must be openai, anthropic, gemini, or ollama)", cfg.Provider) + } + if cfg.Model == "" { + return fmt.Errorf("model is required") + } + // base_url only meaningful for OpenAI-compatible setups. + if cfg.BaseURL != "" && cfg.Provider != "openai" { + return fmt.Errorf("base_url is only meaningful with provider=openai (got %q)", cfg.Provider) + } + // api_key_env, when set, must look like an env var name. + if cfg.APIKeyEnv != "" { + for _, c := range cfg.APIKeyEnv { + if !isEnvNameChar(c) { + return fmt.Errorf("api_key_env %q contains invalid character %q", cfg.APIKeyEnv, c) + } + } + } + return nil +} + +func isEnvNameChar(c rune) bool { + return c == '_' || + (c >= 'A' && c <= 'Z') || + (c >= 'a' && c <= 'z') || + (c >= '0' && c <= '9') +} + +// Tiny utility shims used only by readDotEnv. Avoiding a strings +// import to keep this package's dep surface minimal. +func splitLines(s string) []string { + var out []string + start := 0 + for i := 0; i < len(s); i++ { + if s[i] == '\n' { + out = append(out, s[start:i]) + start = i + 1 + } + } + if start < len(s) { + out = append(out, s[start:]) + } + return out +} + +func trimSpace(s string) string { + i, j := 0, len(s) + for i < j && (s[i] == ' ' || s[i] == '\t' || s[i] == '\r') { + i++ + } + for j > i && (s[j-1] == ' ' || s[j-1] == '\t' || s[j-1] == '\r') { + j-- + } + return s[i:j] +} + +func indexByte(s string, c byte) int { + for i := 0; i < len(s); i++ { + if s[i] == c { + return i + } + } + return -1 +} diff --git a/forge-ui/uiconfig/uiconfig_test.go b/forge-ui/uiconfig/uiconfig_test.go new file mode 100644 index 0000000..e99ce05 --- /dev/null +++ b/forge-ui/uiconfig/uiconfig_test.go @@ -0,0 +1,315 @@ +package uiconfig + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// staticEnv is a deterministic envLookup for tests. +func staticEnv(m map[string]string) func(string) string { + return func(k string) string { return m[k] } +} + +func writeFile(t *testing.T, path, content string) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatalf("write: %v", err) + } +} + +// withFakeHome relocates os.UserHomeDir() to the given dir for the +// duration of the test. Required because the loader's tier-2 fallback +// resolves the user config via os.UserHomeDir. +func withFakeHome(t *testing.T, dir string) { + t.Helper() + origHome, hadHome := os.LookupEnv("HOME") + if err := os.Setenv("HOME", dir); err != nil { + t.Fatalf("setenv HOME: %v", err) + } + t.Cleanup(func() { + if hadHome { + _ = os.Setenv("HOME", origHome) + } else { + _ = os.Unsetenv("HOME") + } + }) +} + +func TestLoad_WorkspaceTakesPrecedence(t *testing.T) { + workspace := t.TempDir() + userHome := t.TempDir() + withFakeHome(t, userHome) + + writeFile(t, filepath.Join(workspace, ".forge", "ui.yaml"), ` +skill_builder: + provider: openai + model: gpt-4.1 + base_url: https://workspace.example.com/v1 +`) + writeFile(t, filepath.Join(userHome, ".forge", "ui.yaml"), ` +skill_builder: + provider: anthropic + model: claude-sonnet-4 +`) + + got, err := LoadSkillBuilderLLM(workspace, "", staticEnv(map[string]string{ + "OPENAI_API_KEY": "sk-workspace", + })) + if err != nil { + t.Fatalf("Load: %v", err) + } + if got.Source != SourceWorkspace { + t.Errorf("Source = %q, want %q", got.Source, SourceWorkspace) + } + if got.Provider != "openai" || got.Model != "gpt-4.1" { + t.Errorf("workspace config not applied: %+v", got) + } + if got.BaseURL != "https://workspace.example.com/v1" { + t.Errorf("BaseURL = %q, want workspace URL", got.BaseURL) + } + if got.APIKey != "sk-workspace" { + t.Errorf("APIKey = %q, want resolved from default env", got.APIKey) + } +} + +func TestLoad_UserFallbackWhenNoWorkspace(t *testing.T) { + workspace := t.TempDir() + userHome := t.TempDir() + withFakeHome(t, userHome) + + writeFile(t, filepath.Join(userHome, ".forge", "ui.yaml"), ` +skill_builder: + provider: anthropic + model: claude-sonnet-4 +`) + + got, err := LoadSkillBuilderLLM(workspace, "", staticEnv(map[string]string{ + "ANTHROPIC_API_KEY": "sk-user", + })) + if err != nil { + t.Fatalf("Load: %v", err) + } + if got.Source != SourceUser { + t.Errorf("Source = %q, want %q", got.Source, SourceUser) + } + if got.Provider != "anthropic" || got.Model != "claude-sonnet-4" { + t.Errorf("user config not applied: %+v", got) + } + if got.APIKey != "sk-user" { + t.Errorf("APIKey = %q, want resolved from ANTHROPIC_API_KEY", got.APIKey) + } + if got.Warning != "" { + t.Errorf("Warning should be empty for user-tier, got %q", got.Warning) + } +} + +func TestLoad_AgentFallbackEmitsDeprecationWarning(t *testing.T) { + workspace := t.TempDir() + userHome := t.TempDir() + withFakeHome(t, userHome) + + agentDir := filepath.Join(workspace, "demo-agent") + writeFile(t, filepath.Join(agentDir, "forge.yaml"), ` +agent_id: demo-agent +model: + provider: openai + name: moonshotai/Kimi-K2.6 +`) + writeFile(t, filepath.Join(agentDir, ".env"), ` +OPENAI_BASE_URL=https://endpoint.example.com/v1 +OPENAI_API_KEY=sk-agent +`) + + got, err := LoadSkillBuilderLLM(workspace, agentDir, staticEnv(map[string]string{ + "OPENAI_API_KEY": "sk-from-env", + })) + if err != nil { + t.Fatalf("Load: %v", err) + } + if got.Source != SourceAgentFallback { + t.Errorf("Source = %q, want %q", got.Source, SourceAgentFallback) + } + if got.Warning == "" { + t.Errorf("Warning should be set for agent_fallback") + } + if !strings.Contains(got.Warning, "deprecated") { + t.Errorf("Warning should mention deprecation, got %q", got.Warning) + } + if got.Provider != "openai" || got.Model != "moonshotai/Kimi-K2.6" { + t.Errorf("agent fallback not applied: %+v", got) + } + if got.BaseURL != "https://endpoint.example.com/v1" { + t.Errorf("BaseURL should come from agent .env, got %q", got.BaseURL) + } + // APIKey resolves from the envLookup (the UI process's actual env), + // not from the agent's .env directly. We do NOT want os.Setenv-style + // leakage from agent .env into the UI process. + if got.APIKey != "sk-from-env" { + t.Errorf("APIKey = %q, want value from envLookup (NOT from agent .env)", got.APIKey) + } +} + +func TestLoad_NoConfigReturnsUnset(t *testing.T) { + workspace := t.TempDir() + userHome := t.TempDir() + withFakeHome(t, userHome) + + got, err := LoadSkillBuilderLLM(workspace, "", staticEnv(nil)) + if err != nil { + t.Fatalf("Load: %v", err) + } + if got.Source != SourceUnset { + t.Errorf("Source = %q, want %q", got.Source, SourceUnset) + } + if got.HasCredentials() { + t.Errorf("HasCredentials should be false when nothing is configured") + } +} + +func TestLoad_OllamaHasCredentialsWithoutKey(t *testing.T) { + workspace := t.TempDir() + userHome := t.TempDir() + withFakeHome(t, userHome) + + writeFile(t, filepath.Join(workspace, ".forge", "ui.yaml"), ` +skill_builder: + provider: ollama + model: llama3 +`) + + got, err := LoadSkillBuilderLLM(workspace, "", staticEnv(nil)) + if err != nil { + t.Fatalf("Load: %v", err) + } + if !got.HasCredentials() { + t.Errorf("ollama should report HasCredentials true even without API key") + } +} + +func TestLoad_APIKeyEnvOverride(t *testing.T) { + workspace := t.TempDir() + userHome := t.TempDir() + withFakeHome(t, userHome) + + // Operator stores the skill-builder key under a non-default name so + // it doesn't collide with agent runtime credentials. + writeFile(t, filepath.Join(workspace, ".forge", "ui.yaml"), ` +skill_builder: + provider: openai + model: gpt-4.1 + api_key_env: WORKSPACE_LLM_KEY +`) + + got, err := LoadSkillBuilderLLM(workspace, "", staticEnv(map[string]string{ + "WORKSPACE_LLM_KEY": "sk-workspace", + "OPENAI_API_KEY": "sk-agent-runtime", // must NOT be picked up + })) + if err != nil { + t.Fatalf("Load: %v", err) + } + if got.APIKey != "sk-workspace" { + t.Errorf("APIKey = %q, want sk-workspace via custom env override", got.APIKey) + } + if got.APIKeyEnv != "WORKSPACE_LLM_KEY" { + t.Errorf("APIKeyEnv = %q, want WORKSPACE_LLM_KEY", got.APIKeyEnv) + } +} + +func TestSave_RoundTripsThroughLoad(t *testing.T) { + workspace := t.TempDir() + userHome := t.TempDir() + withFakeHome(t, userHome) + + cfg := SkillBuilderConfig{ + Provider: "openai", + Model: "gpt-4.1", + BaseURL: "https://endpoint.example.com/v1", + } + if err := SaveSkillBuilderLLM(workspace, cfg); err != nil { + t.Fatalf("Save: %v", err) + } + + got, err := LoadSkillBuilderLLM(workspace, "", staticEnv(map[string]string{ + "OPENAI_API_KEY": "sk-roundtrip", + })) + if err != nil { + t.Fatalf("Load: %v", err) + } + if got.Source != SourceWorkspace { + t.Errorf("Source = %q, want %q", got.Source, SourceWorkspace) + } + if got.Provider != cfg.Provider || got.Model != cfg.Model || got.BaseURL != cfg.BaseURL { + t.Errorf("round-trip mismatch: saved %+v, loaded %+v", cfg, got) + } +} + +func TestSave_OverwritesExistingSkillBuilderSection(t *testing.T) { + workspace := t.TempDir() + userHome := t.TempDir() + withFakeHome(t, userHome) + + // Pre-populate with an old skill_builder section; Save must replace + // it with the new one rather than appending or duplicating. + path := filepath.Join(workspace, ".forge", "ui.yaml") + writeFile(t, path, ` +skill_builder: + provider: anthropic + model: claude-old +`) + + cfg := SkillBuilderConfig{Provider: "openai", Model: "gpt-4.1"} + if err := SaveSkillBuilderLLM(workspace, cfg); err != nil { + t.Fatalf("Save: %v", err) + } + + raw, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read: %v", err) + } + if strings.Contains(string(raw), "claude-old") { + t.Errorf("Save left old model in file:\n%s", raw) + } + if !strings.Contains(string(raw), "gpt-4.1") { + t.Errorf("Save did not write new model:\n%s", raw) + } +} + +func TestValidate_RejectsBadProvider(t *testing.T) { + cases := []struct { + name string + cfg SkillBuilderConfig + want string // substring expected in error + }{ + {"no provider", SkillBuilderConfig{Model: "x"}, "provider is required"}, + {"unknown provider", SkillBuilderConfig{Provider: "bogus", Model: "x"}, "unknown provider"}, + {"no model", SkillBuilderConfig{Provider: "openai"}, "model is required"}, + {"base_url with non-openai", SkillBuilderConfig{Provider: "anthropic", Model: "x", BaseURL: "https://y"}, "base_url is only meaningful"}, + {"invalid api_key_env", SkillBuilderConfig{Provider: "openai", Model: "x", APIKeyEnv: "BAD KEY"}, "api_key_env"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := ValidateSkillBuilderConfig(tc.cfg) + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), tc.want) { + t.Errorf("error %q does not contain %q", err.Error(), tc.want) + } + }) + } +} + +func TestValidate_AcceptsKnownProviders(t *testing.T) { + for _, p := range []string{"openai", "anthropic", "gemini", "ollama"} { + t.Run(p, func(t *testing.T) { + if err := ValidateSkillBuilderConfig(SkillBuilderConfig{Provider: p, Model: "m"}); err != nil { + t.Errorf("provider %q should validate, got %v", p, err) + } + }) + } +} From 1d5c89ed79dfb63757662acea2001a6ae24c6543 Mon Sep 17 00:00:00 2001 From: MK Date: Wed, 3 Jun 2026 16:39:05 -0400 Subject: [PATCH 2/3] docs(ui): sync skill builder docs to the workspace-level LLM model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cleans up stale per-agent skill-builder doc references after #92 and applies the requested README title/lede update. docs/reference/web-dashboard.md Rewrote the Skill Builder "How It Works" section: no longer claims the builder uses the agent's own LLM; no longer describes the hardcoded gpt-4.1 / claude-opus-4-6 codegen upgrade. Now points operators at docs/ui/skill-builder-llm.md for the workspace-LLM model + 3-tier resolution. API Endpoints table adds GET /api/skill-builder/provider (path-less, for first-run detection) and GET / PUT /api/settings/skill- builder with explicit notes on the inline-api_key flow and the /.forge/.env destination. Architecture file listing adds handlers_settings.go and the uiconfig/ package. docs/skills/skills-cli.md Replaced "uses the agent's own LLM provider" with a cross-link to the workspace-LLM page. README.md Title -> "Forge — An Open, Secure, Portable AI Agent Runtime for the Enterprise". Lede rewritten around Anthropic's Agent Skills standard and the SKILL.md -> portable agent -> deploy anywhere narrative. Broken-link check on the edited pages and the workspace LLM doc (README + 3 docs + ../ui/skill-builder-llm.md) passed. --- README.md | 5 ++--- docs/reference/web-dashboard.md | 21 ++++++++++++++++++--- docs/skills/skills-cli.md | 2 +- 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 91be171..91de470 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,7 @@ Forge — open-source AI agent framework

-# Forge — OpenClaw for Enterprise: A Secure, Portable AI Agent Runtime +# Forge — An Open, Secure, Portable AI Agent Runtime for the Enterprise

Documentation @@ -11,8 +11,7 @@ Built by initializ

-Build, run, and deploy AI agents from a single `SKILL.md` file. -Secure by default. Runs anywhere — local, container, cloud, air-gapped. +Forge is the open-source runtime for Anthropic's Agent Skills standard — built for the agent that runs next to a service, in your environment, on infrastructure you already operate. Write a `SKILL.md`. Compile to a portable, hardened agent. Deploy it anywhere containers run: Kubernetes, on-prem, air-gapped, embedded in CI, or as an A2A endpoint. ## Why Forge? diff --git a/docs/reference/web-dashboard.md b/docs/reference/web-dashboard.md index a92f9e9..79984d3 100644 --- a/docs/reference/web-dashboard.md +++ b/docs/reference/web-dashboard.md @@ -137,7 +137,16 @@ An AI-powered conversational tool for creating custom skills. Access it via the ### How It Works -The Skill Builder uses the agent's own LLM provider to power a chat conversation that generates valid SKILL.md files and optional helper scripts. It automatically selects a stronger code-generation model when available (e.g. `gpt-4.1` for OpenAI, `claude-opus-4-6` for Anthropic). API key detection loads the agent's `.env` file and encrypted secrets (if unlocked) in addition to system environment variables. +The Skill Builder uses a **workspace-level LLM** that's independent of any specific agent's runtime LLM. The same configuration applies across every agent in the workspace and works before any agent has been scaffolded. + +Configuration is persisted under `/.forge/`: + +- `ui.yaml` holds the non-secret fields (`provider`, `model`, optional `base_url`, optional `api_key_env`). +- `.env` holds the API key value (mode `0600`, auto-protected by a sibling `.gitignore`). + +The status banner above the chat panel surfaces the resolution source — `workspace`, `user` (machine-wide fallback), `agent_fallback` (a deprecated compat shim used when no workspace/user config exists), or `unset`. When `unset`, the banner shows a **Configure** button that opens the settings modal. + +See [Skill Builder LLM](../ui/skill-builder-llm.md) for the full configuration reference, three-tier resolution precedence, and trust-boundary rationale. ### Features @@ -182,11 +191,14 @@ The validator enforces the [SKILL.md format](../core-concepts/skill-md-format.md | Method | Path | Description | |--------|------|-------------| -| `GET` | `/api/agents/{id}/skill-builder/provider` | Returns the agent's LLM provider, codegen model, and API key status | +| `GET` | `/api/skill-builder/provider` | Returns the resolved workspace-level LLM (provider, model, `source`, `has_key`, deprecation `warning`). Use this for first-run detection before any agent is picked. | +| `GET` | `/api/agents/{id}/skill-builder/provider` | Same shape as above, but consults the agent-fallback tier when no workspace/user config exists. | | `GET` | `/api/agents/{id}/skill-builder/context` | Returns the system prompt used for skill generation | -| `POST` | `/api/agents/{id}/skill-builder/chat` | Streams an LLM conversation via SSE (accepts `messages` array) | +| `POST` | `/api/agents/{id}/skill-builder/chat` | Streams an LLM conversation via SSE (accepts `messages` array). Returns `400` when the workspace LLM is `unset` or has no resolvable API key. | | `POST` | `/api/agents/{id}/skill-builder/validate` | Validates a SKILL.md and optional scripts | | `POST` | `/api/agents/{id}/skill-builder/save` | Saves a validated skill to `skills/{name}/`, merges egress domains, writes env vars; returns `SkillSaveResult` with `path`, `egress_added`, `env_configured`, `env_missing` | +| `GET` | `/api/settings/skill-builder` | Returns the workspace-level config + metadata for the Settings modal (`source`, `has_key`, `providers` list). The API key value is never echoed back. | +| `PUT` | `/api/settings/skill-builder` | Persists `provider`, `model`, optional `base_url`, optional `api_key_env` to `/.forge/ui.yaml`. When an optional `api_key` field is present in the body, writes it to `/.forge/.env` (mode `0600`) under `api_key_env` (or the provider default). The key value never leaks to `ui.yaml`. Submit an empty / omitted `api_key` to leave the saved value untouched. | ## Architecture @@ -199,6 +211,9 @@ forge-ui/ handlers.go Dashboard API (agents, start/stop, chat, sessions) handlers_create.go Wizard API (create, config, skills, tools, OAuth) handlers_skill_builder.go Skill Builder API (chat, validate, save, provider) + handlers_settings.go Workspace-level settings API (skill-builder LLM) + uiconfig/ Workspace ui.yaml + .env loader; SkillBuilderLLM + resolution + atomic .env writer with auto-.gitignore skill_builder_context.go System prompt for the Skill Designer AI skill_validator.go SKILL.md validation and artifact extraction process.go Process manager (exec forge serve start/stop) diff --git a/docs/skills/skills-cli.md b/docs/skills/skills-cli.md index b22b790..1273339 100644 --- a/docs/skills/skills-cli.md +++ b/docs/skills/skills-cli.md @@ -69,7 +69,7 @@ Tooling can match on the substrings `(via policy)` or `(acknowledged by policy)` ## Skill Builder (Web UI) -The [Web Dashboard](../reference/web-dashboard.md#skill-builder) includes an AI-powered Skill Builder that generates valid SKILL.md files and helper scripts through a conversational interface. It uses the agent's own LLM provider and includes server-side validation before saving to the agent's `skills/` directory. On save, the builder automatically parses the skill's requirements and: +The [Web Dashboard](../reference/web-dashboard.md#skill-builder) includes an AI-powered Skill Builder that generates valid SKILL.md files and helper scripts through a conversational interface. It uses a [workspace-level LLM](../ui/skill-builder-llm.md) (independent of any specific agent's runtime LLM) and includes server-side validation before saving to the agent's `skills/` directory. On save, the builder automatically parses the skill's requirements and: - **Merges egress domains** into `forge.yaml` `egress.allowed_domains` (deduplicated) - **Writes user-provided env vars** to `.env` (skipping keys already present) From f458328bba0a8453b00ff6ef54ee39b2948605ca Mon Sep 17 00:00:00 2001 From: MK Date: Wed, 3 Jun 2026 16:41:10 -0400 Subject: [PATCH 3/3] docs(reference): clarify model.provider listing after Custom normalization (#83) PR #93 (issue #83) made `forge init` normalize `provider: custom` -> `provider: openai` + OPENAI_BASE_URL / OPENAI_API_KEY at scaffold time for OpenAI-compatible endpoints (OpenRouter, vLLM, litellm, self-hosted Kimi/Llama). The generated forge.yaml has never carried provider: custom since that landed, but the reference docs still listed "custom" as a forge.yaml model.provider value, which is misleading. docs/reference/forge-yaml-schema.md Drop "custom" from the model.provider option list. Add a short note explaining the OpenAI-compatible endpoint recipe and that the Custom wizard option normalizes to this shape. docs/reference/cli-reference.md Keep "custom" as a valid --model-provider flag value (the CLI input still accepts it; the normalizer rewrites at scaffold time) but clarify inline that it's an alias for the OpenAI-compatible endpoint scaffold. Both edits surfaced from the sync-docs pass on PR #96 as adjacent doc-debt from #83. --- docs/reference/cli-reference.md | 2 +- docs/reference/forge-yaml-schema.md | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/docs/reference/cli-reference.md b/docs/reference/cli-reference.md index 4b6a077..905a9df 100644 --- a/docs/reference/cli-reference.md +++ b/docs/reference/cli-reference.md @@ -31,7 +31,7 @@ forge init [name] [flags] | `--name` | `-n` | | Agent name | | `--framework` | `-f` | | Framework: `crewai`, `langchain`, or `custom` | | `--language` | `-l` | | Language: `python`, `typescript`, or `go` | -| `--model-provider` | `-m` | | Model provider: `openai`, `anthropic`, `ollama`, or `custom` | +| `--model-provider` | `-m` | | Model provider: `openai`, `anthropic`, `gemini`, `ollama`, or `custom` (alias for an OpenAI-compatible endpoint — scaffolds `provider: openai` + `OPENAI_BASE_URL` / `OPENAI_API_KEY`) | | `--channels` | | | Channel adapters (e.g., `slack,telegram`) | | `--tools` | | | Builtin tools to enable (e.g., `web_search,http_request`) | | `--skills` | | | Registry skills to include (e.g., `github,weather`) | diff --git a/docs/reference/forge-yaml-schema.md b/docs/reference/forge-yaml-schema.md index 64a91bb..2c9a3f3 100644 --- a/docs/reference/forge-yaml-schema.md +++ b/docs/reference/forge-yaml-schema.md @@ -16,13 +16,17 @@ registry: "ghcr.io/org" # Container registry entrypoint: "agent.py" # Required for crewai/langchain, omit for forge model: - provider: "openai" # openai, anthropic, gemini, ollama, custom + provider: "openai" # openai, anthropic, gemini, ollama name: "gpt-4o" # Model name organization_id: "org-xxx" # OpenAI Organization ID (enterprise, optional) fallbacks: # Fallback providers (optional) - provider: "anthropic" name: "claude-sonnet-4-20250514" organization_id: "" # Per-fallback org ID override (optional) +# For OpenAI-compatible endpoints (OpenRouter, vLLM, litellm, self-hosted +# Kimi/Llama): use provider: "openai" + set OPENAI_BASE_URL in .env. +# The forge init wizard's "Custom" option normalizes to this shape — the +# generated forge.yaml never carries provider: "custom". tools: - name: "web_search"