From 2c83ce8a7c1fcdacfbe6b3ba38c6ca43246f6875 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 16:14:27 +0100 Subject: [PATCH] fix: share option resolution between the opencode and Pi builders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two harness builders in internal/catalog had drifted apart on how they read a catalogue entry. BuildProviderBlock applies optionsFromEnv and enforces optionsRequired; BuildPiProvider did neither, reading p.Options["baseURL"] straight from the catalogue. The visible symptom was that per-provider endpoint variables did nothing under Pi: OLLAMA_BASE_URL, LLAMACPP_BASE_URL, OMLX_BASE_URL, VLLM_BASE_URL and OPENAI_BASE_URL were silently dropped, so models.json kept pointing at localhost while the same Outfit under opencode pointed at the right server. Only the generic OUTFIT_BASE_URL worked, because it arrives by a different parameter. The dropped URL was not the damaging part. IsLocalEndpoint then still saw localhost, so an apiKeyOptional provider took the keyless branch and Pi was handed the literal placeholder "local" to authenticate a remote server with — a rejected request whose cause is nowhere near where it appears. The optionsRequired half is the same split, one commit younger, and currently latent: the providers using it have no pi block, so BuildPiProvider rejects them earlier. It would become live the moment a pi block met a required option — including via a runtime catalogue supplied with --providers, which no integrity test can inspect. Copying the optionsFromEnv loop into BuildPiProvider would restore correctness and leave the same trap for the next option kind, so the resolution moves to Provider.ResolveOptions and Provider.RequireOptions and both builders call them. A third harness inherits both for free. Pi's base URL now resolves as: explicit override, then the provider's own optionsFromEnv variable, then pi.baseUrl, then options.baseURL. The per-provider variable sits above both catalogue values because it is the user describing their own machine. The two cannot collide today, so that ordering is about what happens when someone adds a provider having both — slotting it below pi.baseUrl would mean adding a pi.baseUrl to omlx later silently stopped OMLX_BASE_URL working, which is the bug being fixed here. BuildPiProvider now also fails on an unset required option: Pi's schema is {baseUrl, api, apiKey, models}, so there is nowhere to put a Vertex project, and failing beats writing an entry missing something essential. An integrity test states the matching invariant for the embedded catalogue. Both regression tests were confirmed to fail against the pre-fix builder. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CipwSCMUZRE61zWWiYEcDx --- AGENTS.md | 2 +- CHANGELOG.md | 6 + internal/catalog/catalog.go | 83 +++++++++--- internal/catalog/catalog_test.go | 127 ++++++++++++++++++ internal/catalog/providers.yaml | 8 +- .../fix-pi-option-resolution/.openspec.yaml | 2 + .../fix-pi-option-resolution/design.md | 54 ++++++++ .../fix-pi-option-resolution/proposal.md | 54 ++++++++ .../specs/pi-integration/spec.md | 55 ++++++++ .../changes/fix-pi-option-resolution/tasks.md | 30 +++++ openspec/specs/pi-integration/spec.md | 3 +- 11 files changed, 401 insertions(+), 23 deletions(-) create mode 100644 openspec/changes/fix-pi-option-resolution/.openspec.yaml create mode 100644 openspec/changes/fix-pi-option-resolution/design.md create mode 100644 openspec/changes/fix-pi-option-resolution/proposal.md create mode 100644 openspec/changes/fix-pi-option-resolution/specs/pi-integration/spec.md create mode 100644 openspec/changes/fix-pi-option-resolution/tasks.md diff --git a/AGENTS.md b/AGENTS.md index 8b9bb803..2fca2c8a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -56,7 +56,7 @@ The binary lives under `cmd/`; the domain logic is split into `internal/` packag **Pi config (`internal/pi`).** Pi's `models.json` is plain JSON of the form `{"providers": {"": {baseUrl, api, apiKey, models: [...]}}}`. The whole file is read into a generic map so unknown top-level keys, sibling providers, and unknown provider fields all round-trip untouched; only the managed provider is merged in (models unioned by `id`). `apiKey` is written as a `$ENV_VAR` interpolation (never the resolved secret), and **keyless local providers get a dummy literal placeholder** (`piPlaceholderAPIKey`) because Pi hides a provider's models from `/model` until *some* auth is configured. The reference is otherwise always written, even when the variable is currently unset, because Pi resolves it at run time — the key can be exported after the Outfit is applied. The one exception is `apiKeyOptional` (see the traps below), where an unset variable means a genuinely keyless server rather than a key yet to be supplied. Pi has no top-level default-model setting, so `export` relies on the provider selection alone and `add` tells the user which model to pick. -**Key/option resolution.** `catalog.BuildProviderBlock` sets `options.apiKey` (opencode) to the provider's `apiKeyEnv` as an `{env:VAR}` reference — never the secret — validating `apiKeyPrefix` and erroring when `apiKeyRequired` against the value it resolves. Resolution is `opencode.EnvResolver(dir)`: a `.env` **beside the Outfit being applied**, then the environment — `dir` is empty for a flags-only `add`, which falls back to the working directory. It deliberately is not relative to the binary or the source file (the old `runtime.Caller` form resolved to the build machine's path, so an installed binary never found one). The option is omitted only for a provider with no key variable, or an `apiKeyOptional` one at a local endpoint. Both harnesses therefore rely on the variable being set when the agent runs, which `harnessEnv` in the CLI arranges at launch. `optionsFromEnv` injects other options (e.g. `baseURL`, `region`) when their env vars are set. The opencode top-level `model` is `/`. Base-URL precedence for both builders: `--base-url`/`-u` flag > `OUTFIT_BASE_URL` > the catalogue's `pi.baseUrl`/`options.baseURL`. +**Key/option resolution.** `catalog.BuildProviderBlock` sets `options.apiKey` (opencode) to the provider's `apiKeyEnv` as an `{env:VAR}` reference — never the secret — validating `apiKeyPrefix` and erroring when `apiKeyRequired` against the value it resolves. Resolution is `opencode.EnvResolver(dir)`: a `.env` **beside the Outfit being applied**, then the environment — `dir` is empty for a flags-only `add`, which falls back to the working directory. It deliberately is not relative to the binary or the source file (the old `runtime.Caller` form resolved to the build machine's path, so an installed binary never found one). The option is omitted only for a provider with no key variable, or an `apiKeyOptional` one at a local endpoint. Both harnesses therefore rely on the variable being set when the agent runs, which `harnessEnv` in the CLI arranges at launch. `optionsFromEnv` injects other options (e.g. `baseURL`, `region`) when their env vars are set. Both builders share `Provider.ResolveOptions` for that and `Provider.RequireOptions` for `optionsRequired`, deliberately: when only the opencode builder applied them, a per-provider variable like `OLLAMA_BASE_URL` was silently dropped under Pi, which then handed a *remote* server the keyless placeholder because `IsLocalEndpoint` still saw `localhost`. The opencode top-level `model` is `/`. Base-URL precedence for both builders: `--base-url`/`-u` flag > `OUTFIT_BASE_URL` > the provider's own `optionsFromEnv` variable > the catalogue's `pi.baseUrl` (Pi only) > `options.baseURL`. **`serve`.** `outfit serve` launches a local inference server for an Outfit without touching any harness config. The engine comes from `PROVIDER` via `engineFor` (`cmd/outfit/serve.go`), the local twin of `runnerFor`: `llamacpp` runs `llama-server`, `omlx` runs the Apple-Silicon oMLX CLI, and anything else is an error. A `serveEngine` carries the binary, any subcommand, the preset dialect, the Outfit→flags mapping, and the install hint. With a `PRESET` it flattens the matching `.ini` section (plus `[*]` defaults) into flags in that engine's dialect; otherwise it derives a command from the Outfit alone. Anything the Outfit states overrides the preset. `--dry-run`/`-n` prints the command without running it. oMLX serves a whole `--model-dir` and picks per request, so only `BASEURL` becomes a launch flag for it, and it starts happily with neither `PRESET` nor `MODEL`. `serve` never resolves or passes an API key — it prints the command it runs. diff --git a/CHANGELOG.md b/CHANGELOG.md index 65ac522d..d81a2d0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 are not self-hosted engines (it previously ran llama-server whatever the Outfit said) +### Fixed +- fix: apply optionsFromEnv and optionsRequired when building a Pi provider, so + per-provider endpoint variables (OLLAMA_BASE_URL, LLAMACPP_BASE_URL, + OMLX_BASE_URL, VLLM_BASE_URL, OPENAI_BASE_URL) reach the Pi harness and a + remote server is no longer handed the keyless placeholder + ## [1.9.0] - 2026-07-29 ### Added - feat: add a vllm provider to the catalogue diff --git a/internal/catalog/catalog.go b/internal/catalog/catalog.go index 708f689f..4f8a0d90 100644 --- a/internal/catalog/catalog.go +++ b/internal/catalog/catalog.go @@ -119,6 +119,49 @@ func (c *Catalog) SortedProviderNames() []string { return names } +// ResolveOptions returns the provider's options with its optionsFromEnv mapping +// applied over the static ones, which is the view *both* harness builders need: +// a per-provider variable like OLLAMA_BASE_URL is a user's runtime statement +// about where the server is, so it is not opencode-specific. resolve looks up +// env vars (typically a .env, then the environment). +// +// The returned map is a fresh copy, so callers may layer their own overrides +// over it without mutating the catalogue. +func (p *Provider) ResolveOptions(resolve func(string) string) map[string]any { + options := make(map[string]any, len(p.Options)+len(p.OptionsFromEnv)) + for k, v := range p.Options { + options[k] = v + } + for optKey, envVar := range p.OptionsFromEnv { + if v := resolve(envVar); v != "" { + options[optKey] = v + } + } + return options +} + +// RequireOptions checks that every option in OptionsRequired resolved to a +// non-empty value. A provider may require caller-supplied options that have no +// usable default (e.g. a Vertex AI project); unlike the API key, these are plain +// options, so apiKeyRequired does not cover them. +// +// Both builders call this. Pi's schema carries no general options map, so a +// provider that both requires an option and declares a `pi` block cannot express +// it there — failing loudly beats writing an entry that is missing something +// essential. The embedded catalogue never pairs the two (TestCatalogPiIntegrity +// enforces that), but a runtime catalogue supplied via --providers can. +func (p *Provider) RequireOptions(id string, options map[string]any) error { + for _, optKey := range p.OptionsRequired { + if v, ok := options[optKey]; !ok || v == nil || v == "" { + if env := p.OptionsFromEnv[optKey]; env != "" { + return fmt.Errorf("the %q option is required for provider %q; set %s in your .env or environment", optKey, id, env) + } + return fmt.Errorf("the %q option is required for provider %q; set it in the catalogue options", optKey, id) + } + } + return nil +} + // BuildProviderBlock turns a provider plus an explicit model into an opencode // provider block, returning the block and the fully-qualified default model // (provider/model), or "" if none was selected. resolve looks up env vars @@ -137,31 +180,15 @@ func BuildProviderBlock(id string, p *Provider, modelOverride, baseURLOverride s block["npm"] = p.NPM } - options := map[string]any{} - for k, v := range p.Options { - options[k] = v - } - for optKey, envVar := range p.OptionsFromEnv { - if v := resolve(envVar); v != "" { - options[optKey] = v - } - } + options := p.ResolveOptions(resolve) if baseURLOverride == "" { baseURLOverride = resolve(baseURLEnv) } if baseURLOverride != "" { options["baseURL"] = baseURLOverride } - // A provider may require caller-supplied options that have no usable default - // (e.g. a Vertex AI project). Unlike the API key, these are plain options, - // so apiKeyRequired does not cover them. - for _, optKey := range p.OptionsRequired { - if v, ok := options[optKey]; !ok || v == nil || v == "" { - if env := p.OptionsFromEnv[optKey]; env != "" { - return nil, "", fmt.Errorf("the %q option is required for provider %q; set %s in your .env or environment", optKey, id, env) - } - return nil, "", fmt.Errorf("the %q option is required for provider %q; set it in the catalogue options", optKey, id) - } + if err := p.RequireOptions(id, options); err != nil { + return nil, "", err } if p.APIKeyEnv != "" { key := resolve(p.APIKeyEnv) @@ -245,17 +272,35 @@ func BuildPiProvider(id string, p *Provider, modelOverride, baseURLOverride stri prov := PiProvider{API: p.Pi.API} + options := p.ResolveOptions(resolve) if baseURLOverride == "" { baseURLOverride = resolve(baseURLEnv) } + // Precedence: the explicit override (--base-url, then OUTFIT_BASE_URL), then + // the provider's own endpoint variable, then the catalogue's Pi endpoint, + // then its opencode one. The per-provider variable sits above both catalogue + // values because it is the user speaking about their own machine, and below + // the explicit override because that is the user speaking more specifically. + var envBaseURL string + if v := p.OptionsFromEnv["baseURL"]; v != "" { + envBaseURL = resolve(v) + } switch { case baseURLOverride != "": prov.BaseURL = baseURLOverride + case envBaseURL != "": + prov.BaseURL = envBaseURL case p.Pi.BaseURL != "": prov.BaseURL = p.Pi.BaseURL default: prov.BaseURL, _ = p.Options["baseURL"].(string) } + if prov.BaseURL != "" { + options["baseURL"] = prov.BaseURL + } + if err := p.RequireOptions(id, options); err != nil { + return PiProvider{}, "", err + } // Pi only surfaces a provider's models in /model once auth is configured; // with no apiKey at all the models load but stay unavailable. Keyless local diff --git a/internal/catalog/catalog_test.go b/internal/catalog/catalog_test.go index 3cc2df47..00a7c6ad 100644 --- a/internal/catalog/catalog_test.go +++ b/internal/catalog/catalog_test.go @@ -745,3 +745,130 @@ func TestBuildPiProvider_OMLXPlaceholderThenReference(t *testing.T) { t.Errorf("remote apiKey = %q, want $OPENAI_API_KEY", remote.APIKey) } } + +// TestBuildPiProvider_HonoursPerProviderBaseURLEnv is the regression test for a +// bug that made the Pi harness unusable against any non-local server: the Pi +// builder read the catalogue's static options.baseURL directly and never applied +// optionsFromEnv, so a per-provider endpoint variable was silently dropped. +// +// The dropped base URL was not the damaging part. IsLocalEndpoint then saw +// "localhost", so an apiKeyOptional provider took the keyless branch and Pi was +// handed the literal placeholder to authenticate a remote server with. +func TestBuildPiProvider_HonoursPerProviderBaseURLEnv(t *testing.T) { + cat, _ := Load() + for _, tc := range []struct{ id, env string }{ + {"omlx", "OMLX_BASE_URL"}, + {"llamacpp", "LLAMACPP_BASE_URL"}, + {"vllm", "VLLM_BASE_URL"}, + {"ollama", "OLLAMA_BASE_URL"}, + } { + t.Run(tc.id, func(t *testing.T) { + p := cat.Providers[tc.id] + if p == nil { + t.Fatalf("%s missing from the catalogue", tc.id) + } + const remote = "http://box.local:9999/v1" + prov, _, err := BuildPiProvider(tc.id, p, "m", "", envMap(map[string]string{tc.env: remote})) + if err != nil { + t.Fatalf("BuildPiProvider: %v", err) + } + if prov.BaseURL != remote { + t.Errorf("baseUrl = %q, want %q", prov.BaseURL, remote) + } + // A provider with a key variable must get the reference Pi resolves at + // run time, not the placeholder meant for keyless local servers. + if p.APIKeyEnv != "" && prov.APIKey != "$"+p.APIKeyEnv { + t.Errorf("apiKey = %q, want $%s for a remote endpoint", prov.APIKey, p.APIKeyEnv) + } + }) + } +} + +// TestBuildPiProvider_BaseURLPrecedence pins the order the two overrides and the +// two catalogue values resolve in. +func TestBuildPiProvider_BaseURLPrecedence(t *testing.T) { + cat, _ := Load() + omlx := cat.Providers["omlx"] + + t.Run("explicit override beats the provider variable", func(t *testing.T) { + prov, _, err := BuildPiProvider("omlx", omlx, "m", "http://explicit:1/v1", + envMap(map[string]string{"OMLX_BASE_URL": "http://from-env:2/v1"})) + if err != nil { + t.Fatalf("BuildPiProvider: %v", err) + } + if prov.BaseURL != "http://explicit:1/v1" { + t.Errorf("baseUrl = %q, want the explicit override", prov.BaseURL) + } + }) + + t.Run("OUTFIT_BASE_URL beats the provider variable", func(t *testing.T) { + prov, _, err := BuildPiProvider("omlx", omlx, "m", "", envMap(map[string]string{ + "OUTFIT_BASE_URL": "http://generic:1/v1", + "OMLX_BASE_URL": "http://from-env:2/v1", + })) + if err != nil { + t.Fatalf("BuildPiProvider: %v", err) + } + if prov.BaseURL != "http://generic:1/v1" { + t.Errorf("baseUrl = %q, want the generic override", prov.BaseURL) + } + }) + + t.Run("unset variable leaves the catalogue value", func(t *testing.T) { + prov, _, err := BuildPiProvider("omlx", omlx, "m", "", noEnv) + if err != nil { + t.Fatalf("BuildPiProvider: %v", err) + } + if prov.BaseURL != "http://localhost:8000/v1" { + t.Errorf("baseUrl = %q, want the catalogue default", prov.BaseURL) + } + }) + + t.Run("pi.baseUrl still wins where no variable applies", func(t *testing.T) { + prov, _, err := BuildPiProvider("openrouter", cat.Providers["openrouter"], "m", "", noEnv) + if err != nil { + t.Fatalf("BuildPiProvider: %v", err) + } + if prov.BaseURL != "https://openrouter.ai/api/v1" { + t.Errorf("baseUrl = %q, want the pi block's endpoint", prov.BaseURL) + } + }) +} + +// TestBuildPiProvider_RequiresOptions closes the second half of the same split: +// optionsRequired was enforced when building an opencode block but not a Pi one, +// so a Pi-capable provider missing a required option produced an entry silently +// lacking it. Pi's schema has no general options map, so failing is the only +// honest outcome. The catalogue cannot currently express this pairing, but a +// runtime one supplied via --providers can. +func TestBuildPiProvider_RequiresOptions(t *testing.T) { + p := &Provider{ + Description: "test", + Options: map[string]any{"baseURL": "https://example.invalid/v1"}, + OptionsRequired: []string{"project"}, + OptionsFromEnv: map[string]string{"project": "TEST_PROJECT"}, + Pi: &PiConfig{API: "openai-completions"}, + } + + if _, _, err := BuildPiProvider("custom", p, "m", "", noEnv); err == nil { + t.Error("expected an error when a required option is unset") + } else if !strings.Contains(err.Error(), "TEST_PROJECT") { + t.Errorf("error should name the variable to set, got %v", err) + } + + if _, _, err := BuildPiProvider("custom", p, "m", "", envMap(map[string]string{"TEST_PROJECT": "p"})); err != nil { + t.Errorf("a satisfied requirement should build: %v", err) + } +} + +// TestCatalogRequiredOptionsAreNotPiCapable holds the invariant that lets the +// embedded catalogue avoid the case above: Pi's schema carries no options map, +// so a provider needing one cannot be served by Pi. +func TestCatalogRequiredOptionsAreNotPiCapable(t *testing.T) { + cat, _ := Load() + for name, p := range cat.Providers { + if len(p.OptionsRequired) > 0 && p.Pi != nil { + t.Errorf("provider %q requires options %v but declares a pi block; Pi cannot carry them", name, p.OptionsRequired) + } + } +} diff --git a/internal/catalog/providers.yaml b/internal/catalog/providers.yaml index c75644a9..f07c330f 100644 --- a/internal/catalog/providers.yaml +++ b/internal/catalog/providers.yaml @@ -30,8 +30,12 @@ # api Pi protocol: openai-completions | openai-responses # | anthropic-messages | google-generative-ai # baseUrl Pi endpoint (optional; falls back to -# options.baseURL). Providers without a pi block -# are reported as unsupported by the Pi harness. +# options.baseURL, and is itself overridden by this +# provider's optionsFromEnv baseURL variable). +# Providers without a pi block are reported as +# unsupported by the Pi harness. Pi's schema carries +# no general options map, so a provider using +# optionsRequired cannot declare a pi block. providers: openrouter: diff --git a/openspec/changes/fix-pi-option-resolution/.openspec.yaml b/openspec/changes/fix-pi-option-resolution/.openspec.yaml new file mode 100644 index 00000000..5849c2db --- /dev/null +++ b/openspec/changes/fix-pi-option-resolution/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-08-01 diff --git a/openspec/changes/fix-pi-option-resolution/design.md b/openspec/changes/fix-pi-option-resolution/design.md new file mode 100644 index 00000000..bc6bb272 --- /dev/null +++ b/openspec/changes/fix-pi-option-resolution/design.md @@ -0,0 +1,54 @@ +## Context + +`BuildProviderBlock` and `BuildPiProvider` translate the same catalogue entry into two +config formats. They necessarily differ in output shape, but they had also come to differ +in *input interpretation*, which is not a difference either format requires. + +## Decisions + +### Shared helpers rather than a parallel fix + +The obvious fix — copy the `optionsFromEnv` loop into `BuildPiProvider` — would restore +correctness and leave the same trap for the next option kind. `optionsRequired` is the +proof: it was added to one builder only, months after `optionsFromEnv` was, by an author +with no reason to suspect the other builder existed as a separate code path. + +So the resolution moves to `Provider.ResolveOptions` and `Provider.RequireOptions`, and +both builders call them. A third harness would inherit both for free. + +### Where the per-provider variable sits in the precedence + +`--base-url` > `OUTFIT_BASE_URL` > `optionsFromEnv` > `pi.baseUrl` > `options.baseURL`. + +The interesting placement is `optionsFromEnv` above `pi.baseUrl`. Both are "where is the +server", but one is written by the catalogue and one by the user, and a user who exports +`OMLX_BASE_URL` has said something specific about their machine that a shipped default +cannot know. The two cannot actually collide today — every provider with an +`optionsFromEnv.baseURL` falls through to `options.baseURL`, and the only provider with a +`pi.baseUrl` (`openrouter`) has no endpoint variable — so this ordering is about what +happens when someone adds one, not about current behaviour. + +The alternative, slotting it below `pi.baseUrl`, has a nastier failure mode: adding a +`pi.baseUrl` to `omlx` later would silently stop `OMLX_BASE_URL` from working, which is +the same class of bug being fixed here. + +### Failing on a required option Pi cannot express + +A Pi provider entry is `{baseUrl, api, apiKey, models}`. There is nowhere to put a Vertex +`project`, so a provider that needs one genuinely cannot be served by Pi. + +Two guards, because they cover different things. The runtime check in `BuildPiProvider` +catches a **runtime** catalogue (`--providers`/`OUTFIT_PROVIDERS`), which no test can see. +The integrity test catches the **embedded** catalogue, and states the invariant somewhere +a reader will find it. Without the integrity test the runtime error would be the only +documentation; without the runtime check the invariant would hold only for the shipped +file. + +## Risks / Trade-offs + +- **Users relying on the broken behaviour**: implausible. The prior behaviour was a Pi + config pointing at a server the user had explicitly redirected away from, which fails at + first use rather than working differently. +- **Ordering choice is forward-looking**: `optionsFromEnv` above `pi.baseUrl` changes + nothing today and is asserted by a precedence test, so a future provider combining both + gets the documented behaviour rather than an accident. diff --git a/openspec/changes/fix-pi-option-resolution/proposal.md b/openspec/changes/fix-pi-option-resolution/proposal.md new file mode 100644 index 00000000..001a35cc --- /dev/null +++ b/openspec/changes/fix-pi-option-resolution/proposal.md @@ -0,0 +1,54 @@ +## Why + +The two harness builders in `internal/catalog` had drifted apart on option +handling. `BuildProviderBlock` applies `optionsFromEnv` and enforces +`optionsRequired`; `BuildPiProvider` did neither, reading `p.Options["baseURL"]` +straight from the catalogue. + +The visible symptom was that per-provider endpoint variables did nothing under +Pi. `OLLAMA_BASE_URL`, `LLAMACPP_BASE_URL`, `OMLX_BASE_URL`, `VLLM_BASE_URL` and +`OPENAI_BASE_URL` were silently dropped, so `models.json` kept pointing at +`localhost` while the same Outfit under opencode pointed at the right server. +Only the generic `OUTFIT_BASE_URL` worked, because it arrives by a different +parameter. + +The dropped URL was not the damaging part. `IsLocalEndpoint` then still saw +`localhost`, so an `apiKeyOptional` provider took the keyless branch and Pi was +handed the literal placeholder `local` to authenticate a *remote* server with — +a rejected request whose cause is nowhere near where it appears. Nothing errored +at any point. + +The `optionsRequired` half is the same split, one commit younger, and currently +latent: the providers that use it (`google-vertex`, `google-vertex-anthropic`) +have no `pi` block, so `BuildPiProvider` rejects them earlier. It would become +live the moment a `pi` block met a required option — including via a runtime +catalogue supplied with `--providers`, which no integrity test covers. + +## What Changes + +- Extract `Provider.ResolveOptions` (static options with `optionsFromEnv` + applied over them) and `Provider.RequireOptions` (the `optionsRequired` check), + and call both from *both* builders, so the two paths cannot drift again. +- `BuildPiProvider` resolves its base URL as: explicit override, then the + provider's own `optionsFromEnv` variable, then the `pi` block's `baseUrl`, then + `options.baseURL`. The per-provider variable sits above both catalogue values + because it is the user describing their own machine, and below the explicit + override because that is the user being more specific. +- `BuildPiProvider` fails when a required option is unset, rather than writing an + entry silently missing it. Pi's schema has no general options map, so it cannot + carry one; failing is the only honest outcome. +- Add an integrity test asserting no embedded provider pairs `optionsRequired` + with a `pi` block, which is the invariant that keeps the case above unreachable + for the shipped catalogue. + +Non-goal: no change to how opencode resolves options — that path was already +correct, and its behaviour is unchanged. + +## Impact + +- Affected specs: `pi-integration` +- Affected code: `internal/catalog/catalog.go` +- Behaviour change: a Pi user who set a per-provider endpoint variable was + previously served a `localhost` entry; they will now get the endpoint they + asked for. Anyone who worked around the bug by *also* setting + `OUTFIT_BASE_URL` is unaffected, since the explicit override still wins. diff --git a/openspec/changes/fix-pi-option-resolution/specs/pi-integration/spec.md b/openspec/changes/fix-pi-option-resolution/specs/pi-integration/spec.md new file mode 100644 index 00000000..85f7e554 --- /dev/null +++ b/openspec/changes/fix-pi-option-resolution/specs/pi-integration/spec.md @@ -0,0 +1,55 @@ +## MODIFIED Requirements + +### Requirement: Pi capability gating + +A provider SHALL be usable with the Pi harness only when the catalogue gives it +a `pi` block (declaring the protocol `api` and optionally a Pi-specific base +URL). Selecting an unsupported provider under Pi SHALL fail saying the provider +is not supported by the pi harness. + +The Pi base URL SHALL resolve as: explicit override, then the provider's own +`optionsFromEnv` endpoint variable, then the `pi` block's `baseUrl`, then the +provider's `options.baseURL`. A per-provider endpoint variable SHALL therefore +apply to both harnesses, not only opencode: it states where the user's server +is, which is not a property of the config format being written. + +Because the resolved endpoint decides whether a keyless local server is being +addressed, dropping that variable would also mis-classify a remote endpoint as +local and write the keyless placeholder in place of the key reference. + +#### Scenario: Bedrock is opencode-only + +- **WHEN** the user selects a provider with no `pi` block under Pi +- **THEN** the command fails explaining the provider has no Pi support + +#### Scenario: Per-provider endpoint variable reaches Pi + +- **WHEN** a provider's endpoint variable names a remote host and the selection + is applied under Pi +- **THEN** the written entry carries that host, and a provider with a key + variable carries the key reference rather than the keyless placeholder + +#### Scenario: An explicit override still wins + +- **WHEN** both the generic base-URL override and the provider's own endpoint + variable are set +- **THEN** the override is written + +## ADDED Requirements + +### Requirement: Required options under Pi + +A Pi provider entry carries only a base URL, a protocol, a key, and models, so a +provider declaring required options has nowhere to put them. Building a Pi +provider whose required options do not resolve SHALL fail, naming the variable +to set, rather than writing an entry silently missing them. + +The embedded catalogue SHALL NOT pair required options with a `pi` block, since +such a provider cannot be served by Pi at all; the runtime check exists for +catalogues supplied at run time, which no integrity test can inspect. + +#### Scenario: A required option is unset + +- **WHEN** a Pi-capable provider declares a required option and nothing supplies + it +- **THEN** the command fails naming the environment variable that would satisfy it diff --git a/openspec/changes/fix-pi-option-resolution/tasks.md b/openspec/changes/fix-pi-option-resolution/tasks.md new file mode 100644 index 00000000..0c777a4c --- /dev/null +++ b/openspec/changes/fix-pi-option-resolution/tasks.md @@ -0,0 +1,30 @@ +## 1. Shared option resolution + +- [x] 1.1 Extract `Provider.ResolveOptions(resolve)` — static options with `optionsFromEnv` layered over them, returned as a fresh map so callers can add their own overrides. +- [x] 1.2 Extract `Provider.RequireOptions(id, options)` from the inline `optionsRequired` loop in `BuildProviderBlock`. +- [x] 1.3 Rewire `BuildProviderBlock` onto both helpers; behaviour unchanged. + +## 2. Pi builder + +- [x] 2.1 Resolve the Pi base URL as: explicit override, then the provider's `optionsFromEnv` variable, then `pi.baseUrl`, then `options.baseURL`. +- [x] 2.2 Call `RequireOptions` in `BuildPiProvider`, so a required option that Pi's schema cannot carry fails rather than being dropped. + +## 3. Tests + +- [x] 3.1 Regression: a per-provider endpoint variable reaches the Pi entry for `omlx`, `llamacpp`, `vllm` and `ollama`, and a keyed provider then gets `$VAR` rather than the keyless placeholder. +- [x] 3.2 Precedence: explicit override and `OUTFIT_BASE_URL` both beat the provider variable; an unset variable leaves the catalogue value; `openrouter`'s `pi.baseUrl` still applies. +- [x] 3.3 `BuildPiProvider` errors on an unset required option, naming the variable, and builds once it is set. +- [x] 3.4 Integrity: no embedded provider pairs `optionsRequired` with a `pi` block. +- [x] 3.5 Confirm 3.1 and 3.3 fail against the pre-fix builder. + +## 4. Docs + +- [x] 4.1 Update the `pi-integration` base-URL requirement. +- [x] 4.2 Update the `AGENTS.md` key/option resolution note with the shared helpers and the full precedence. +- [x] 4.3 Update the `providers.yaml` header for the `pi.baseUrl` fallback and the `optionsRequired` restriction. +- [x] 4.4 CHANGELOG `Fixed` entry. + +## 5. Verify + +- [x] 5.1 `gofmt -l` clean, `go vet ./...`, `go test ./... -race -cover` green. +- [x] 5.2 Manual: `outfit add -H pi` with each per-provider variable pointed at a remote host writes that host and the `$VAR` reference; localhost is unchanged. diff --git a/openspec/specs/pi-integration/spec.md b/openspec/specs/pi-integration/spec.md index a3e6877e..8dcdcc57 100644 --- a/openspec/specs/pi-integration/spec.md +++ b/openspec/specs/pi-integration/spec.md @@ -99,7 +99,8 @@ A provider SHALL be usable with the Pi harness only when the catalogue gives it a `pi` block (declaring the protocol `api` and optionally a Pi-specific base URL). Selecting an unsupported provider under Pi SHALL fail saying the provider is not supported by the pi harness. The Pi base URL SHALL resolve as: explicit -override, then the `pi` block's `baseUrl`, then the provider's `options.baseURL`. +override, then the provider's own `optionsFromEnv` endpoint variable, then the +`pi` block's `baseUrl`, then the provider's `options.baseURL`. #### Scenario: Bedrock is opencode-only