aaif-goose / aaif-goose/goose

Design: per-request nonce header for declarative providers

Offen
#11,308 7 Kommentare 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen
Vorherrschende Sprache
Rust
Sterne
54.2k
Forks
6.2k
Ø Merge
3 T. 2 Std.
Gemergte PRs (30 T.)
262

Beschreibung

Records the design behind #11302, per @michaelneale's request. One open question for maintainers at the bottom: where config-field validation should live given several construction paths bypass from_json.

## Problem

A proxy or sidecar sitting in front of an OpenAI-compatible endpoint often needs a **per-request unique value** — idempotency keys, replay-window dedup, request correlation. Declarative (custom) providers have no way to supply one.

`DeclarativeProviderConfig.headers` is resolved once at provider init, so it can carry a constant but never a fresh value. Meanwhile every built-in provider can reach `ApiClient::with_request_builder` and install a per-request decorator; `from_declarative_config` is the only construction path that cannot. So this is a gap rather than a new capability.

## Design

One optional config field, `nonce_header: Option`, on `DeclarativeProviderConfig`.

- **The header name is supplied by config, never hardcoded.** No consumer's vocabulary enters goose.
- **Absent means no-op** — no decorator installed, no code path taken.
- Present installs a decorator inserting a fresh UUIDv4 (122 bits CSPRNG) under that name.
- The name is parsed **once at config load**, so a typo fails immediately with a message naming thw key rather than turning every subsequent request into an opaque header-parse error.

A general dynamic-header or templating mechanism was deliberately not attempted. The nonce is the concrete need; the decorator seam leaves generalization open if one ever turns up.

## Things the review surfaced, and how they resolved

These are the parts worth keeping, since they're where the design actually got decided.

**Decorators compose now.** `ApiClient::request_builder` was a single `Option` that
`with_request_builder` replaced. The registry path installs a second decorator —
`openai_def::from_custom_config` adds the session-id builder right after `from_declarative_config`
returns — which silently discarded the nonce, so CLI and Desktop would never have sent it. The slot is a `Vec` now and the setter appends; decorators run in installation order. Every existing caller is unaffected since each installs one.

Worth recording why the tests didn't catch it: they called `from_declarative_config` directly and asserted on the wire, so they exercised the builder but not the registry path that wraps it. A test through `openai_def::from_custom_config` asserting both headers arrive is the right regression pin, added in #11302.

**Reserved names are rejected.** `agent-session-id` is removed and rewritten by
`session_id_request_builder`, and `ApiClient::send_request` applies authentication *after* every decorator — so `authorization` and `proxy-authorization` would be overwritten too. All three now fail at config load with an error explaining why, rather than silently carrying nothing. A name colliding with a provider's own configured `ApiKey` auth header has the same problem but isn't statically knowable there; that one is documented on the field.

**Scope is the OpenAI declarative builder only.** `from_json` dispatches to the anthropic and ollama builders too, and they don't install the decorator. An earlier revision rejected the field for those engines in `from_json` — but the registry doesn't go through `from_json`: `load_custom_providers`deserializes and the registry calls engine-specific `from_custom_config` directly, and an openai-engine config named `huggingface` routes to `HuggingFaceProvider::from_custom_config`, which builds its own `ApiClient`. So that guard was dead code on the path that matters and has been removed in favour of documenting the limitation on the field itself.

**That last one is the open design question**, and it's the reason this issue is worth having:
**where should validation of provider-config fields live, given several construction paths bypass `from_json`?** Options I can see are a shared validation step in the registry dispatch in `load_custom_providers`, installing the decorator in each specialised constructor, or keeping it builder-scoped and documented. Happy to implement whichever you prefer — I've guessed wrong twice, so I'd rather have the maintainers' answer than a third guess.

## Semver note

Adding a field to `DeclarativeProviderConfig` breaks struct-literal construction for out-of-tree callers — deserialization is fine thanks to `#[serde(default)]`. Seven in-repo sites needed updating, three of them in `goose/src/config/declarative_providers.rs` and two of those non-test. If `#[non_exhaustive]` or a builder is preferred, glad to follow.

Separately, `ApiClient::with_request_builder` changed from replace-on-set to append. Same signature, different behaviour for any out-of-tree caller that installed twice expecting replacement — unlikely to exist, but semver-relevant if `goose-providers` is published.

## Consumer

Mesh-LLM's local sidecar, so the *client* rather than the intermediary contributes the per-request nonce — Mesh-LLM/mesh-llm#1233. The companion Mesh-LLM PR is also mine and sets the key in the `mesh.json` that `mesh-llm goose` generates.

Implementation: #11302.

Beitragsleitfaden

Beitragsleitfaden öffnen

Bewertung

Dieses Issue wurde noch nicht bewertet.

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.