From b8a058fa5539f7f70a60adeb71c64592d0d62080 Mon Sep 17 00:00:00 2001 From: Hung Nguyen Date: Sun, 9 Aug 2026 10:44:58 +0700 Subject: [PATCH] chore: update CHANGELOG for v0.40.0 and enhance documentation - Added `agent.CapabilityProvider` and `agent.LLMCapabilities` to improve provider capability reporting. - Introduced `agent.TokenUsage.CostUSD` for accurate cost reporting without requiring a `PriceTable`. - Enhanced OpenAI client with `WithBaseURL` and `WithHTTPHeader` options for better transport configuration. - Changed `openai.Option` to an interface for improved flexibility. - Updated documentation to reflect new features and changes, including per-run cost handling and provider capabilities. - Fixed issues with providers not correctly reporting costs and capabilities, ensuring better integration and user experience. --- CHANGELOG.md | 22 ++ README.md | 2 +- docs/observability.md | 29 +++ docs/providers.md | 68 ++++++- pkg/agent/budget.go | 20 ++ pkg/agent/cost.go | 55 +++-- pkg/agent/cost_provider_reported_test.go | 226 +++++++++++++++++++++ pkg/agent/degraded.go | 9 +- pkg/agent/event_types.go | 30 ++- pkg/agent/llm_call.go | 26 ++- pkg/agent/loop_stream.go | 68 ++++++- pkg/agent/options.go | 18 +- pkg/agent/regenerate.go | 2 +- pkg/history/types.go | 8 + pkg/llm/anthropic/anthropic.go | 7 + pkg/llm/gemini/gemini.go | 7 + pkg/llm/llmfake/capabilities_test.go | 23 +++ pkg/llm/llmfake/scripted.go | 15 ++ pkg/llm/openai/client.go | 158 ++++++++++++++ pkg/llm/openai/client_test.go | 134 ++++++++++++ pkg/llm/openai/embedder_openai.go | 19 +- pkg/llm/openai/header_doer_test.go | 61 ++++++ pkg/llm/openai/openai.go | 45 ++-- pkg/llm/openai/openai_compat.go | 44 ++-- pkg/llm/openai/openai_compat_test.go | 53 +++++ pkg/llm/openai/summary_provider.go | 18 +- pkg/llm/openai/vision.go | 15 +- pkg/llm/router.go | 32 +++ pkg/llm/router_capabilities_test.go | 72 +++++++ pkg/telemetry/otelllm/capabilities_test.go | 51 +++++ pkg/telemetry/otelllm/provider.go | 19 ++ 31 files changed, 1232 insertions(+), 124 deletions(-) create mode 100644 pkg/agent/cost_provider_reported_test.go create mode 100644 pkg/llm/llmfake/capabilities_test.go create mode 100644 pkg/llm/openai/client.go create mode 100644 pkg/llm/openai/client_test.go create mode 100644 pkg/llm/openai/header_doer_test.go create mode 100644 pkg/llm/openai/openai_compat_test.go create mode 100644 pkg/llm/router_capabilities_test.go create mode 100644 pkg/telemetry/otelllm/capabilities_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 80c9af2..024f72e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,27 @@ All notable changes to GopherAgent are documented here. Format follows [Keep a Changelog](https://keepachangelog.com/); versions follow [Semantic Versioning](https://semver.org/) — pre-1.0, breaking API changes only require a minor bump. +## [v0.40.0] — 2026-08-09 + +### Added + +- **`agent.CapabilityProvider` and `agent.LLMCapabilities` — an adapter can now say what it can actually put on the wire.** `LLMProvider` is a single method, and nothing on it reported whether the adapter honours `history.MediaPart` values of type `PartImage`. That is fine for a text agent and a hazard for a multimodal one: handed a text-only provider, a call that attaches images does not fail — it returns a schema-valid, confident answer formed without the model ever seeing them, and no log line distinguishes that from a working call. Consumers now assert the optional interface and reject at construction. Three rules make the signal worth trusting, and all three are enforced in-tree rather than merely documented. **Absence means unknown, not false**, so a provider that makes no claim stays unclassified and the caller decides how strict to be — which is why `llmfake.ScriptedProvider`, the one in-tree provider that honours nothing, declares the zero value explicitly instead of staying silent. **Decorators must not lose or invent the claim**: `otelllm.NewProvider` picks its concrete type at construction, forwarding the wrapped report when there is one and declining to implement the interface when there is not, so enabling tracing never downgrades a known capability to unknown. **A multiplexer answers conservatively**: `llm.RouterProvider` reports the intersection over its fallback and every route, because the route is chosen from the conversation and is unknown until the call runs, and one undeclared member collapses the report — under-reporting costs a spurious rejection at construction, over-reporting costs a wrong answer at run time. The report describes the *adapter*, not the model: a gateway fronting both text-only and multimodal catalogues answers for the transport it speaks, and selecting a model that honours it stays the caller's job. (`pkg/agent/loop_stream.go`, `pkg/llm/anthropic`, `pkg/llm/openai`, `pkg/llm/gemini`, `pkg/llm/llmfake`, `pkg/llm/router.go`, `pkg/telemetry/otelllm`) +- **`agent.TokenUsage.CostUSD` — a provider that knows what it charged can now say so, and no `PriceTable` is required.** Cost was estimated in exactly one way: multiply rolled-up tokens by a static `PriceTable`. Backends that route across vendors return the real per-request charge, which a table cannot reproduce — it cannot see which model the gateway picked, nor which cache discounts applied — and the rollup was gated on the table being non-nil, so the exact figure was discarded for precisely the adopters who configure no table *because* their provider is already exact. Providers now set `CostUSD` on the `TokenUsage` they return, and dollars resolve **per call before being summed**: a call that reported a charge contributes it verbatim, any other call is estimated from the table. Resolving per call rather than once over the rollup is what keeps a Run that mixes backends honest — summing table rates over the aggregate token count would bill a gateway's tokens a second time. `RunCostEvent.Usage.CostUSD` carries the reported portion alone, so comparing it against `USD` says how much of the total was billed rather than estimated. A negative charge is treated as *not reported* rather than accumulated, matching `PriceTable.Compute`'s existing clamp, so a buggy provider cannot surface a credit through either the run rollup or `BudgetTracker`. (`pkg/agent/cost.go`, `pkg/agent/llm_call.go`, `pkg/agent/budget.go`, `pkg/agent/event_types.go`) +- **`openai.WithBaseURL` and `openai.WithHTTPHeader` — transport options that work on every client in the package.** `WithBaseURL` targets any OpenAI-compatible endpoint from `New` itself, making `NewCompat` the same call with `baseURL` required rather than optional. `WithHTTPHeader` adds attribution or routing headers that gateways accept; headers are applied after the SDK builds the request, so a name the SDK already sets is overwritten — passing `Authorization` deliberately replaces the bearer token. Base URLs are validated at construction: absolute, HTTP(S), no embedded credentials, query, or fragment, with trailing slashes normalized because the SDK appends its own path segment. Every constructor routes through one client builder, so validation and header injection cannot drift apart between them. (`pkg/llm/openai/client.go`) + +### Changed (breaking) + +- **`openai.Option` is an interface, not `func(*Provider)`.** It has to be, for one call to accept both sampling and transport settings — `New(key, model, WithBaseURL(u), WithTemperature(0.2))`. The constructors that return options are unchanged, so `openai.WithTemperature(0)` and friends still compile; only code that declared or converted the bare func type is affected. +- **`openai.WithHTTPHeader` returns `ClientOption` rather than `Option`.** `ClientOption` is a strict subset of `Option`, so anywhere it was already passed keeps working, and it now also reaches the embedder, vision analyzer, and summary provider. +- **`openai.NewEmbedder`, `NewVisionAnalyzer`, and `NewSummaryProvider` take `...ClientOption`.** Existing two-argument calls are source-compatible; only code that assigned one of these to a function-typed variable is affected. `ClientOption` deliberately excludes the sampling options, so passing `WithTemperature` to an embedder is a compile error rather than a setting that silently does nothing. +- **`RunCostEvent` now fires without a `PriceTable` when a provider reported a charge.** It previously required a table. An adopter with neither a table nor a cost-reporting provider still sees no event, so the silent case is unchanged; hosts that render every event should expect this one on provider-priced runs. `USD` is the per-call resolved total rather than a single table computation over the rollup. +- **`openai` constructor errors now carry a `openai: : ` prefix.** Messages such as `OPENAI_API_KEY is not set in environment` became `openai: New: OPENAI_API_KEY is not set in environment`, matching the convention used elsewhere. Code matching on the old strings needs updating; `errors.Is` users are unaffected. + +### Fixed + +- **The embedder, vision analyzer, and summary provider no longer force a call to `api.openai.com`.** All three built their client with no configuration, so neither a base URL nor headers could reach them, and `NewCompat` configured the chat provider alone. Someone wiring a deliberately local or gateway-only stack got chat from their chosen endpoint while these three silently called OpenAI directly — an unexpected egress of user text to a third party, and a demand for a key the operator may not hold. The failure was invisible: each call succeeded against the wrong host. All three now accept `WithBaseURL`. (`pkg/llm/openai/embedder_openai.go`, `pkg/llm/openai/vision.go`, `pkg/llm/openai/summary_provider.go`) +- **`history.Message` states that the `Content` fallback is for an empty `Parts` only.** The precedence between the two fields was documented, but not the obligation it implies: an adapter that cannot render a *populated* `Parts` must fail rather than quietly answer from `Content`, because a caller that sent an image and got back a fluent reply has no way to tell the model never saw it. (`pkg/history/types.go`) + ## [v0.39.0] — 2026-08-08 ### Added @@ -599,6 +620,7 @@ Multi-user, long-running, audit-friendly chat surface — the foundation for sid - README section on the permission flow — documents `RequiresConfirmation` × `ConfirmHITL` × `Permissions` interaction. - Enum struct tag support in `tools.SchemaFor[T]()` — emit values into JSON-Schema's `enum` array so providers reject invalid values upstream. +[v0.40.0]: https://github.com/hung12ct/gopheragent/releases/tag/v0.40.0 [v0.39.0]: https://github.com/hung12ct/gopheragent/releases/tag/v0.39.0 [v0.38.0]: https://github.com/hung12ct/gopheragent/releases/tag/v0.38.0 [v0.37.0]: https://github.com/hung12ct/gopheragent/releases/tag/v0.37.0 diff --git a/README.md b/README.md index 366f831..8070511 100644 --- a/README.md +++ b/README.md @@ -58,7 +58,7 @@ go get github.com/hung12ct/gopheragent the bookkeeping did not" instead of forcing a turn into success or failure. - **Custom tools** — one interface, schema derived from a Go struct; a middleware chain for logging, timing, rate limiting, and tracing. -- **Multi-provider** — OpenAI, Anthropic, Gemini, Vertex, and OpenAI-compatible +- **Multi-provider** — OpenAI, Anthropic, Gemini, Vertex, OpenRouter, and OpenAI-compatible backends, each in its own subpackage; multi-model routing; sampling controls. - **Sub-agents & async** — sub-agent streaming, conversation forking, background workers, first-class task tracking. diff --git a/docs/observability.md b/docs/observability.md index 04811d9..833549f 100644 --- a/docs/observability.md +++ b/docs/observability.md @@ -127,6 +127,35 @@ loop.OnEvent(bt.Handler()) http.Handle("/metrics", agentmetrics.Handler(bt)) ``` +## Per-Run cost + +`RunCostEvent` fires once per Run — on every terminal path, not just the +success one — with rolled-up tokens and a dollar total. There are two ways it +gets a figure, and they compose: + +```go +// Estimate from your own rates, for providers that bill silently. +loop := agent.New(sm, reg, provider, + agent.WithPriceTable(agent.PriceTable{ + "claude-sonnet-4-6": {InputPerMTokens: 3, OutputPerMTokens: 15}, + }, "claude-sonnet-4-6")) +``` + +A provider that knows what it charged reports it directly instead, by setting +`CostUSD` on the `TokenUsage` it returns from `GenerateStream`. Gateways that +route across vendors typically do. No `PriceTable` is needed in that case — a +static table could not price them correctly anyway, since it cannot see which +model the gateway picked or which cache discounts applied. + +Dollars resolve **per call**, then sum: a call that reported `CostUSD` +contributes that exact charge, and every other call is estimated from the +table. A Run that mixes both bills each call the best way available. Read +`RunCostEvent.Usage.CostUSD` to see how much of `USD` was billed rather than +estimated — equal means the total is exact, zero means it is all estimate. + +With neither a table nor a reporting provider, no event fires; raw counts are +still on the wire as `UsageEvent`. + ## Notes - **Zero-cost when off:** no tracer/meter → decorators return the wrapped diff --git a/docs/providers.md b/docs/providers.md index 00bd715..f193261 100644 --- a/docs/providers.md +++ b/docs/providers.md @@ -9,10 +9,76 @@ not statically link the other vendors' SDKs: | Anthropic | `pkg/llm/anthropic` | `anthropic.New(key, model)` | claude-sonnet, claude-opus, ... | | Google Gemini | `pkg/llm/gemini` | `gemini.New(key, model)` | gemini-2.5-flash, gemini-2.5-pro, ... | | Vertex AI (Gemini) | `pkg/llm/gemini` | `gemini.NewVertex(project, location, model)` | Vertex-hosted Gemini via ADC | -| OpenAI-compatible | `pkg/llm/openai` | `openai.NewCompat(key, model, baseURL)` | Ollama, Groq, vLLM, Together, ... | +| OpenAI-compatible | `pkg/llm/openai` | `openai.NewCompat(key, model, baseURL)` | OpenRouter, Ollama, Groq, vLLM, Together, ... | All providers auto-discover API keys from environment variables when key is `""`. +OpenRouter uses the existing OpenAI-compatible provider; it does not need a +separate adapter: + +```go +p, err := openai.NewCompat(key, "openai/gpt-4o", "https://openrouter.ai/api/v1", + openai.WithHTTPHeader("HTTP-Referer", "https://example.com"), + openai.WithHTTPHeader("X-Title", "my-agent"), +) +``` + +Compatible base URLs must be absolute HTTP(S) URLs without embedded +credentials, query parameters, or fragments. Use HTTPS for remote gateways; +plain HTTP remains available for local Ollama/vLLM development. + +### Point the non-chat clients at the same endpoint + +`NewCompat` configures the **chat provider only**. The embedder, vision +analyzer, and summary provider take the same transport options, and without +them they call `api.openai.com` — on a deliberately local deployment that is an +unexpected egress and a key you may not hold. Pass `WithBaseURL` to each: + +```go +base := openai.WithBaseURL("http://localhost:11434/v1") + +chat, _ := openai.New(key, "llama3", base, openai.WithTemperature(0.2)) +emb, _ := openai.NewEmbedder(key, "nomic-embed-text", base) +vis, _ := openai.NewVisionAnalyzer(key, "llava", base) +sum, _ := openai.NewSummaryProvider(key, "llama3", base) +``` + +`WithBaseURL` and `WithHTTPHeader` work on all four. The sampling options +(`WithTemperature`, `WithTopP`, `WithSeed`) apply to the chat provider only, +and passing one to `NewEmbedder` is a compile error rather than a setting that +silently does nothing. + +## Provider capabilities + +`agent.CapabilityProvider` lets a consumer that requires image input or +structured output reject an unsuitable provider at construction, instead of +discovering the gap from a confident, wrong answer. The OpenAI, Anthropic, and +Gemini adapters report both; `llmfake.ScriptedProvider` reports neither, since +it replays a script without ever reading a message's media parts. + +```go +if c, ok := provider.(agent.CapabilityProvider); ok && !c.Capabilities().ImageInput { + return fmt.Errorf("judge requires a multimodal provider") +} +``` + +Two rules keep that check meaningful: + +- **Absence means unknown, not false.** A provider that does not implement the + interface makes no claim; the caller decides how strict to be. +- **Decorators forward.** `otelllm.NewProvider` passes the wrapped provider's + report through, and does not implement the interface when the wrapped + provider doesn't — so enabling tracing never erases the signal. + `llm.RouterProvider` reports the intersection over its fallback and every + route, because the route is chosen from the conversation and is unknown + until the call runs; one undeclared member collapses the report to "supports + nothing". + +It does not replace model discovery: gateways such as OpenRouter expose +text-only and multimodal models through the same adapter, so applications must +still verify the selected model's live metadata when correctness or spend +depends on it. + ## Multi-model routing `llm.RouterProvider` dispatches calls across several backing providers behind a diff --git a/pkg/agent/budget.go b/pkg/agent/budget.go index 9afd9af..4e0f941 100644 --- a/pkg/agent/budget.go +++ b/pkg/agent/budget.go @@ -65,11 +65,19 @@ func (bt *BudgetTracker) Handler() EventHandler { return } delta := p.Usage + // Negative charges are dropped rather than accumulated, so a + // buggy provider cannot refund a session's spend through the + // usage stream. Same clamp as runCostAcc.add. + cost := delta.CostUSD + if cost < 0 { + cost = 0 + } bt.mu.Lock() cur := bt.usage[sessionKey] cur.PromptTokens += delta.PromptTokens cur.CompletionTokens += delta.CompletionTokens cur.TotalTokens += delta.TotalTokens + cur.CostUSD += cost bt.usage[sessionKey] = cur bt.mu.Unlock() } @@ -144,6 +152,7 @@ func (bt *BudgetTracker) Rewind(sessionKey string, refund TokenUsage) { cur.PromptTokens = subFloorZero(cur.PromptTokens, refund.PromptTokens) cur.CompletionTokens = subFloorZero(cur.CompletionTokens, refund.CompletionTokens) cur.TotalTokens = subFloorZero(cur.TotalTokens, refund.TotalTokens) + cur.CostUSD = subFloorZeroFloat(cur.CostUSD, refund.CostUSD) bt.usage[sessionKey] = cur } @@ -155,3 +164,14 @@ func subFloorZero(a, b int) int { } return a - b } + +// subFloorZeroFloat is subFloorZero for the dollar field. A separate +// function rather than a generic over cmp.Ordered: the int and float +// counters are refunded under the same rule but a shared signature would +// invite callers to floor arbitrary numeric state through here. +func subFloorZeroFloat(a, b float64) float64 { + if b >= a { + return 0 + } + return a - b +} diff --git a/pkg/agent/cost.go b/pkg/agent/cost.go index 6e79751..0b4dd2e 100644 --- a/pkg/agent/cost.go +++ b/pkg/agent/cost.go @@ -53,23 +53,49 @@ type runCostKey struct{} // RunCostEvent right before DoneEvent. Concurrency: callLLM is single- // threaded per Run today, but the mutex keeps the contract safe in // case a future speculation path emits Usage from a worker goroutine. +// +// pt and model are captured at install time and never mutated, so add +// can resolve dollars per call without reaching back into the loop. type runCostAcc struct { mu sync.Mutex usage TokenUsage + usd float64 + + pt PriceTable + model string } func (a *runCostAcc) add(u TokenUsage) { + // A negative charge is nonsense and is treated as "not reported", + // matching PriceTable.Compute's clamp: neither should let a buggy + // provider surface a credit. + cost := u.CostUSD + if cost < 0 { + cost = 0 + } a.mu.Lock() a.usage.PromptTokens += u.PromptTokens a.usage.CompletionTokens += u.CompletionTokens a.usage.TotalTokens += u.TotalTokens + a.usage.CostUSD += cost + // Resolve dollars per call rather than once over the rollup. A Run + // that mixes backends — a router fanning out to a gateway that + // prices its own calls and a vendor that does not — would otherwise + // charge table rates over tokens the provider already billed, or + // drop the estimate for the calls that reported nothing. + if cost > 0 { + a.usd += cost + } else { + a.usd += a.pt.Compute(a.model, u) + } a.mu.Unlock() } -func (a *runCostAcc) snapshot() TokenUsage { +// snapshot returns the accumulated usage and the resolved dollar total. +func (a *runCostAcc) snapshot() (TokenUsage, float64) { a.mu.Lock() defer a.mu.Unlock() - return a.usage + return a.usage, a.usd } func withRunCostAcc(ctx context.Context, acc *runCostAcc) context.Context { @@ -88,17 +114,22 @@ func runCostAccFromContext(ctx context.Context) *runCostAcc { // ctx, emitCost := al.installRunCostAccumulator(ctx, sessionKey, streamChan) // defer emitCost() // -// When PriceTable is nil, the returned ctx is unchanged and emitCost -// is a no-op closure — zero allocation in the hot path beyond the -// branch on PriceTable. Every Run entry point that drives -// iterateMessages (runLogicLoop, continueLogicLoop) must call this so -// MaxIters / MaxToolCallsPerSession / fatal-error terminal paths all -// emit the cost rollup, not just the final-answer success path. +// The accumulator is installed unconditionally, including when +// PriceTable is nil. It used to be gated on the table, which silently +// dropped the exact figure reported by providers that bill per call — +// precisely the adopters who configure no table because they do not +// need to estimate. Whether a provider reports a cost is not knowable +// before the first call, and there is no config knob to gate on, so +// the cost is one small struct and one ctx value per Run against a Run +// that makes at least one network call. Deliberate, same reasoning as +// installDegradationAccumulator. +// +// Every Run entry point that drives iterateMessages (runLogicLoop, +// continueLogicLoop) must call this so MaxIters / +// MaxToolCallsPerSession / fatal-error terminal paths all emit the +// cost rollup, not just the final-answer success path. func (al *AgentLoop) installRunCostAccumulator(ctx context.Context, sessionKey string, streamChan chan<- StreamEvent) (context.Context, func()) { - if al.PriceTable == nil { - return ctx, func() {} - } - ctx = withRunCostAcc(ctx, &runCostAcc{}) + ctx = withRunCostAcc(ctx, &runCostAcc{pt: al.PriceTable, model: al.PriceModel}) return ctx, func() { al.emitRunCostIfConfigured(ctx, sessionKey, streamChan) } diff --git a/pkg/agent/cost_provider_reported_test.go b/pkg/agent/cost_provider_reported_test.go new file mode 100644 index 0000000..ecf10d2 --- /dev/null +++ b/pkg/agent/cost_provider_reported_test.go @@ -0,0 +1,226 @@ +package agent + +import ( + "context" + "testing" + + "github.com/hung12ct/gopheragent/pkg/history" + "github.com/hung12ct/gopheragent/pkg/tools" +) + +// noopCostTool lets a multi-call Run finish cleanly instead of erroring on an +// unregistered tool, so the cost assertions are not entangled with failure +// handling. +type noopCostTool struct{} + +func (noopCostTool) Descriptor() tools.ToolDescriptor { + return tools.ToolDescriptor{ + Name: "noop", + Description: "does nothing", + Display: tools.DefaultDisplay("noop", "does nothing"), + } +} + +func (noopCostTool) Execute(_ context.Context, _ string) (tools.Result, error) { + return tools.Text("ok"), nil +} + +// closeUSD compares dollar amounts with a tolerance well below a cent. +// CostUSD is a float64, so summing several calls carries the usual binary +// drift (0.05 + 0.02 need not be exactly 0.07); exact equality here would +// make the tests brittle without saying anything about correctness. +func closeUSD(got, want float64) bool { + d := got - want + return d < 1e-9 && d > -1e-9 +} + +// runForCost drives one Run and returns the RunCostEvent it emitted, if any. +func runForCost(t *testing.T, prov LLMProvider, opts ...Option) (RunCostEvent, bool) { + t.Helper() + var got RunCostEvent + var seen bool + opts = append(opts, WithOnEvent(func(_ context.Context, _ string, ev StreamEvent) { + if p, ok := ev.Payload.(RunCostEvent); ok { + got, seen = p, true + } + })) + reg := tools.NewRegistry() + reg.Register(noopCostTool{}) + loop := New(history.NewInMemSessionManager("base"), reg, prov, opts...) + if _, err := loop.RunIteration(context.Background(), "s", "hi"); err != nil { + t.Fatalf("RunIteration: %v", err) + } + return got, seen +} + +// The headline case: a gateway that bills per request needs no PriceTable. +// Gating the accumulator on the table used to drop this figure entirely. +func TestRunCost_ProviderReportedCostNeedsNoPriceTable(t *testing.T) { + prov := &usageStampingProvider{usage: TokenUsage{ + PromptTokens: 1000, CompletionTokens: 500, TotalTokens: 1500, CostUSD: 0.0731, + }} + + rc, seen := runForCost(t, prov) + if !seen { + t.Fatal("RunCostEvent not emitted for a provider-priced Run without a PriceTable") + } + if !closeUSD(rc.USD, 0.0731) { + t.Fatalf("USD = %v, want the provider-reported 0.0731", rc.USD) + } + if !closeUSD(rc.Usage.CostUSD, 0.0731) { + t.Fatalf("Usage.CostUSD = %v, want 0.0731 so adopters can tell exact from estimated", rc.Usage.CostUSD) + } + if rc.Usage.TotalTokens != 1500 { + t.Fatalf("Usage.TotalTokens = %d, want 1500", rc.Usage.TotalTokens) + } +} + +// A reported charge is the real bill and must beat the table estimate, which +// cannot know which model a gateway routed to. +func TestRunCost_ProviderReportedCostBeatsPriceTable(t *testing.T) { + prov := &usageStampingProvider{usage: TokenUsage{ + PromptTokens: 1000, CompletionTokens: 500, TotalTokens: 1500, CostUSD: 0.0731, + }} + + // The table would compute 1000*$10 + 500*$20 per 1M = 0.02. + rc, seen := runForCost(t, prov, + WithPriceTable(PriceTable{"m": {InputPerMTokens: 10, OutputPerMTokens: 20}}, "m")) + if !seen { + t.Fatal("RunCostEvent not emitted") + } + if !closeUSD(rc.USD, 0.0731) { + t.Fatalf("USD = %v, want the provider figure 0.0731 to win over the 0.02 estimate", rc.USD) + } +} + +// Without a table and without a provider charge there is nothing to report: +// usage is already on the wire as UsageEvent. +func TestRunCost_SilentWithNeitherSource(t *testing.T) { + prov := &usageStampingProvider{usage: TokenUsage{PromptTokens: 100, TotalTokens: 100}} + + if _, seen := runForCost(t, prov); seen { + t.Fatal("RunCostEvent must stay silent with no PriceTable and no provider cost") + } +} + +// mixedCostProvider reports a charge on its first call only, then loops a +// tool call so the Run makes several. Models a router fanning out to a +// self-pricing gateway and a vendor that reports nothing. +type mixedCostProvider struct { + calls int +} + +func (p *mixedCostProvider) GenerateStream(_ context.Context, _ []history.Message, _ *tools.Registry, ch chan<- StreamEvent) (LLMResult, error) { + p.calls++ + if p.calls == 1 { + return LLMResult{ + ToolCalls: []PendingToolCall{{ID: "c1", Name: "noop", ArgsJSON: `{}`}}, + Usage: TokenUsage{PromptTokens: 1000, CompletionTokens: 500, TotalTokens: 1500, CostUSD: 0.05}, + }, nil + } + ch <- Event(ContentEvent{Text: "ok"}) + return LLMResult{ + Content: "ok", + Usage: TokenUsage{PromptTokens: 1000, CompletionTokens: 500, TotalTokens: 1500}, + }, nil +} + +// Dollars resolve per call, not once over the rollup: the priced call keeps +// its exact charge and the silent one is estimated from the table. Summing +// the table over total tokens would bill the gateway's call twice over. +func TestRunCost_MixedProvidersResolvePerCall(t *testing.T) { + rc, seen := runForCost(t, &mixedCostProvider{}, + WithPriceTable(PriceTable{"m": {InputPerMTokens: 10, OutputPerMTokens: 20}}, "m")) + if !seen { + t.Fatal("RunCostEvent not emitted") + } + // 0.05 reported + (1000*$10 + 500*$20)/1M = 0.05 + 0.02. + if !closeUSD(rc.USD, 0.07) { + t.Fatalf("USD = %v, want 0.07 (0.05 reported + 0.02 estimated)", rc.USD) + } + if !closeUSD(rc.Usage.CostUSD, 0.05) { + t.Fatalf("Usage.CostUSD = %v, want only the 0.05 that was actually reported", rc.Usage.CostUSD) + } + if rc.Usage.TotalTokens != 3000 { + t.Fatalf("Usage.TotalTokens = %d, want 3000 across both calls", rc.Usage.TotalTokens) + } +} + +// costOnlyProvider bills without reporting token counts, which some gateways +// do. The usage gate keyed on TotalTokens alone would drop the charge. +type costOnlyProvider struct{} + +func (costOnlyProvider) GenerateStream(_ context.Context, _ []history.Message, _ *tools.Registry, ch chan<- StreamEvent) (LLMResult, error) { + ch <- Event(ContentEvent{Text: "ok"}) + return LLMResult{Content: "ok", Usage: TokenUsage{CostUSD: 0.004}}, nil +} + +func TestRunCost_CostWithoutTokenCounts(t *testing.T) { + rc, seen := runForCost(t, costOnlyProvider{}) + if !seen { + t.Fatal("RunCostEvent not emitted for a provider that bills without token counts") + } + if !closeUSD(rc.USD, 0.004) { + t.Fatalf("USD = %v, want 0.004", rc.USD) + } +} + +// A negative charge is nonsense; it must not surface as a credit, and the +// call falls back to the table estimate as if nothing were reported. +func TestRunCost_NegativeProviderCostIsNotACredit(t *testing.T) { + prov := &usageStampingProvider{usage: TokenUsage{ + PromptTokens: 1000, CompletionTokens: 500, TotalTokens: 1500, CostUSD: -5, + }} + + rc, seen := runForCost(t, prov, + WithPriceTable(PriceTable{"m": {InputPerMTokens: 10, OutputPerMTokens: 20}}, "m")) + if !seen { + t.Fatal("RunCostEvent not emitted") + } + if !closeUSD(rc.USD, 0.02) { + t.Fatalf("USD = %v, want the 0.02 table estimate, not a credit", rc.USD) + } + if !closeUSD(rc.Usage.CostUSD, 0) { + t.Fatalf("Usage.CostUSD = %v, want 0 — a negative charge is dropped, not accumulated", rc.Usage.CostUSD) + } +} + +// BudgetTracker sums TokenUsage off the usage stream. Dropping CostUSD there +// would leave Usage() reporting zero spend for a session the provider billed. +func TestBudgetTracker_AccumulatesProviderCost(t *testing.T) { + bt := NewBudgetTracker(0) + h := bt.Handler() + ctx := context.Background() + + h(ctx, "s", Event(UsageEvent{Usage: TokenUsage{TotalTokens: 100, CostUSD: 0.02}})) + h(ctx, "s", Event(UsageEvent{Usage: TokenUsage{TotalTokens: 50, CostUSD: 0.01}})) + // A negative charge must not refund the session through the stream. + h(ctx, "s", Event(UsageEvent{Usage: TokenUsage{TotalTokens: 10, CostUSD: -1}})) + + got := bt.Usage("s") + if got.TotalTokens != 160 { + t.Fatalf("TotalTokens = %d, want 160", got.TotalTokens) + } + if !closeUSD(got.CostUSD, 0.03) { + t.Fatalf("CostUSD = %v, want 0.03", got.CostUSD) + } +} + +// Rewind refunds every field or none: leaving cost behind would strand spend +// on a session whose tokens were already returned. +func TestBudgetTracker_RewindRefundsProviderCost(t *testing.T) { + bt := NewBudgetTracker(0) + h := bt.Handler() + h(context.Background(), "s", Event(UsageEvent{Usage: TokenUsage{TotalTokens: 100, CostUSD: 0.05}})) + + bt.Rewind("s", TokenUsage{TotalTokens: 40, CostUSD: 0.02}) + if got := bt.Usage("s"); !closeUSD(got.CostUSD, 0.03) || got.TotalTokens != 60 { + t.Fatalf("after rewind = %+v, want TotalTokens=60 CostUSD=0.03", got) + } + + // Over-refunding floors at zero rather than going negative. + bt.Rewind("s", TokenUsage{TotalTokens: 999, CostUSD: 999}) + if got := bt.Usage("s"); !closeUSD(got.CostUSD, 0) || got.TotalTokens != 0 { + t.Fatalf("after over-refund = %+v, want both floored to zero", got) + } +} diff --git a/pkg/agent/degraded.go b/pkg/agent/degraded.go index 2824d40..575321e 100644 --- a/pkg/agent/degraded.go +++ b/pkg/agent/degraded.go @@ -78,11 +78,10 @@ func degradedAccFromContext(ctx context.Context) *degradedAcc { // returns it alongside a sweep callback that emits any degradation no // terminal path has claimed yet. // -// Unlike installRunCostAccumulator, which skips the ctx allocation -// entirely when PriceTable is nil, this one always allocates: whether a -// tool will degrade is not knowable up front and there is no config knob -// to gate on. The cost is one small struct, one ctx value, and one -// closure per Run — deliberate, not an oversight. +// Like installRunCostAccumulator, this always allocates: whether a tool +// will degrade is not knowable up front and there is no config knob to +// gate on. The cost is one small struct, one ctx value, and one closure +// per Run — deliberate, not an oversight. // // Caller pattern: // diff --git a/pkg/agent/event_types.go b/pkg/agent/event_types.go index 9393afa..74fe73f 100644 --- a/pkg/agent/event_types.go +++ b/pkg/agent/event_types.go @@ -74,8 +74,8 @@ const ( EventTypeMemoryConsolidated StreamEventType = "memory_consolidated" // EventTypeRunCost is emitted right before DoneEvent on Runs that // produced a final answer. Carries rolled-up token usage and the - // computed dollar cost under the configured PriceTable. Skipped - // when no PriceTable is configured (zero-cost when unused). + // resolved dollar cost. Skipped only when neither a PriceTable is + // configured nor any provider reported a charge of its own. EventTypeRunCost StreamEventType = "run_cost" // EventTypeContextTrace records what the pruner rewrote on the way // into one LLM call. Emitted only when a prune actually changed @@ -385,15 +385,25 @@ func (MemoryConsolidatedEvent) isEventPayload() {} func (MemoryConsolidatedEvent) eventType() StreamEventType { return EventTypeMemoryConsolidated } // RunCostEvent is the typed payload of EventTypeRunCost. Emitted at -// the end of a successful Run when PriceTable is configured. Adopters -// surface USD on the UI, push to billing telemetry, or alert on -// per-Run cost spikes without re-aggregating UsageEvents themselves. +// the end of a Run that produced any accounting, when a PriceTable is +// configured or a provider reported a charge. Adopters surface USD on +// the UI, push to billing telemetry, or alert on per-Run cost spikes +// without re-aggregating UsageEvents themselves. // -// USD is computed from the rolled-up Usage under the AgentLoop's -// configured Model + PriceTable. When the Model isn't in the table, -// USD is zero but Usage still reflects the true accumulated tokens — -// useful for adopters who want to compute cost themselves with a -// dynamic price source. +// USD is resolved per LLM call and then summed: a call whose provider +// reported TokenUsage.CostUSD contributes that exact charge, and any +// other call is estimated from the AgentLoop's Model + PriceTable. So +// a Run against a self-pricing gateway needs no table at all, and one +// that mixes backends bills each call the best way available rather +// than forcing the whole Run into one method. +// +// Usage.CostUSD carries the provider-reported portion alone. Compare +// it against USD to see how much of the total was billed rather than +// estimated: equal means the figure is exact, zero means it is all +// table-derived. When the Model isn't in the table and no provider +// priced the Run, USD is zero but Usage still reflects the true +// accumulated tokens — useful for adopters computing cost downstream +// from a dynamic price source. type RunCostEvent struct { Model string `json:"model,omitempty"` Usage TokenUsage `json:"usage"` diff --git a/pkg/agent/llm_call.go b/pkg/agent/llm_call.go index 40d27cd..90bcd26 100644 --- a/pkg/agent/llm_call.go +++ b/pkg/agent/llm_call.go @@ -71,25 +71,33 @@ func (al *AgentLoop) handleFinalAnswer(ctx context.Context, st *iterationState, al.emit(ctx, st.sessionKey, st.streamChan, Event(DoneEvent{})) } -// emitRunCostIfConfigured emits a RunCostEvent when a PriceTable is -// configured AND the Run actually accumulated tokens. Called from the +// emitRunCostIfConfigured emits a RunCostEvent when the Run accumulated +// any accounting AND someone can act on it — either a PriceTable is +// configured, or a provider reported a real charge. Called from the // deferred cleanup installed by installRunCostAccumulator so it fires // on every terminal path — final answer, MaxIters cap, fatal LLM -// error — not just the success path. Skipped when the accumulator -// is missing (PriceTable nil) or the total is zero. +// error — not just the success path. +// +// An adopter with neither a table nor a cost-reporting provider sees no +// event, exactly as before: usage alone is already on the wire as +// UsageEvent, and a RunCostEvent whose USD is always zero would be +// noise. func (al *AgentLoop) emitRunCostIfConfigured(ctx context.Context, sessionKey string, streamChan chan<- StreamEvent) { acc := runCostAccFromContext(ctx) if acc == nil { return } - usage := acc.snapshot() - if usage.TotalTokens == 0 && usage.PromptTokens == 0 && usage.CompletionTokens == 0 { + usage, usd := acc.snapshot() + if usage == (TokenUsage{}) { + return + } + if al.PriceTable == nil && usd == 0 { return } al.emit(ctx, sessionKey, streamChan, Event(RunCostEvent{ Model: al.PriceModel, Usage: usage, - USD: al.PriceTable.Compute(al.PriceModel, usage), + USD: usd, })) } @@ -141,7 +149,9 @@ func (al *AgentLoop) callLLM(ctx context.Context, st *iterationState, msgs []his if content == "" { content = res.Content } - if err == nil && res.Usage.TotalTokens > 0 { + // A gateway can bill a call it reports no token counts for, so cost + // alone is enough to make the usage worth recording. + if err == nil && (res.Usage.TotalTokens > 0 || res.Usage.CostUSD > 0) { al.emit(ctx, st.sessionKey, st.streamChan, Event(UsageEvent{Usage: res.Usage})) if acc := runCostAccFromContext(ctx); acc != nil { acc.add(res.Usage) diff --git a/pkg/agent/loop_stream.go b/pkg/agent/loop_stream.go index ba1a25b..d033bef 100644 --- a/pkg/agent/loop_stream.go +++ b/pkg/agent/loop_stream.go @@ -141,10 +141,19 @@ type PendingToolCall struct { // TokenUsage carries per-call token accounting returned by an LLM provider. // Providers that do not report usage leave the fields zero. +// +// CostUSD is the dollar amount the provider itself billed for the call, for +// the backends that return one (gateways that route across vendors typically +// do). Leave it zero when the provider reports no cost: the loop then falls +// back to estimating from AgentLoop.PriceTable, and a zero here means "not +// reported", never "free". A reported cost is the actual charge and beats any +// table estimate — it already accounts for the model the gateway picked, +// cache discounts, and per-vendor rates a static table cannot track. type TokenUsage struct { - PromptTokens int `json:"prompt_tokens"` - CompletionTokens int `json:"completion_tokens"` - TotalTokens int `json:"total_tokens"` + PromptTokens int `json:"prompt_tokens"` + CompletionTokens int `json:"completion_tokens"` + TotalTokens int `json:"total_tokens"` + CostUSD float64 `json:"cost_usd,omitempty"` } // LLMResult represents the structured response from the LLM provider. @@ -199,6 +208,40 @@ type LLMProvider interface { GenerateStream(ctx context.Context, memory []history.Message, availableTools *tools.Registry, streamChan chan<- StreamEvent) (LLMResult, error) } +// LLMCapabilities describes what a provider adapter can put on the wire. +// It does not promise that every model behind a compatible endpoint supports +// the feature; dynamic model capability checks remain the caller's job. +type LLMCapabilities struct { + ImageInput bool + StructuredOutput bool +} + +// CapabilityProvider optionally reports an adapter's transport features, so a +// consumer that requires one (a judge that compares images, a caller that +// depends on schema enforcement) can reject an unsuitable provider at +// construction instead of discovering it from a confident, wrong answer. +// +// Three rules make the signal trustworthy; breaking any of them turns it back +// into a guess: +// +// - Absence means unknown, not false. A provider that does not implement +// this interface makes no claim, and callers decide how to treat that. +// Implement it on every adapter — including fakes that support nothing, +// which is exactly the case the interface exists to catch. +// - A decorator around a single LLMProvider must forward that provider's +// report, and must not implement this interface when the wrapped provider +// does not — pick the concrete type at construction. Otherwise wiring +// tracing silently erases a capability the caller checks for, or invents a +// claim the underlying adapter never made. A multiplexer that cannot know +// its target in advance (a router) cannot do that, and instead reports the +// intersection of everything it might dispatch to, counting an undeclared +// member as supporting nothing: that errs toward a loud rejection at +// construction rather than a silent wrong answer at run time. +// - The report describes the adapter, not the model. A gateway that fronts +// both text-only and multimodal models answers for the transport it +// speaks; selecting a model that honours it stays the caller's job. +type CapabilityProvider interface{ Capabilities() LLMCapabilities } + // AgentLoop orchestrates the ReAct loop. type AgentLoop struct { Sessions SessionManager @@ -433,12 +476,15 @@ type AgentLoop struct { // configuration error caught at Consolidate time. MemoryConsolidator *Consolidator - // PriceTable, when non-nil, enables per-Run cost rollup. The loop - // accumulates TokenUsage across every LLM call in a Run and emits - // a RunCostEvent right before DoneEvent with the dollars computed - // from PriceTable[PriceModel]. Adopters running multi-model - // router setups whose pricing varies per call should leave this - // nil and roll cost up themselves from UsageEvent. + // PriceTable supplies the rates used to estimate the dollar cost of + // LLM calls whose provider reports none. The loop accumulates + // TokenUsage across every call in a Run and emits a RunCostEvent + // right before DoneEvent. Leaving it nil does not disable the + // rollup: a provider that sets TokenUsage.CostUSD is already exact + // and reports through the same event. Adopters running multi-model + // router setups whose pricing varies per call, against providers + // that report nothing, should leave this nil and roll cost up + // themselves from UsageEvent. PriceTable PriceTable // PriceModel is the key looked up in PriceTable for cost @@ -814,8 +860,8 @@ func (al *AgentLoop) runLogicLoop(ctx context.Context, sessionKey string, userMs // from any iteration without threading new params. The deferred // emitCost fires on every terminal exit (final answer, MaxIters, // MaxToolCallsPerSession, fatal LLM error) instead of only on the - // final-answer success path. Nil PriceTable → no-op closure + - // no ctx allocation. + // final-answer success path. Installed unconditionally so a + // provider that prices its own calls is recorded without a table. var emitCost func() ctx, emitCost = al.installRunCostAccumulator(ctx, sessionKey, streamChan) defer emitCost() diff --git a/pkg/agent/options.go b/pkg/agent/options.go index 2693797..78b629c 100644 --- a/pkg/agent/options.go +++ b/pkg/agent/options.go @@ -296,12 +296,18 @@ func WithMemoryConsolidator(c *Consolidator) Option { return func(al *AgentLoop) { al.MemoryConsolidator = c } } -// WithPriceTable enables per-Run cost rollup. The loop accumulates -// TokenUsage across every LLM call in a Run and emits RunCostEvent -// right before DoneEvent, with USD computed from table[model]. -// Adopters with router-style multi-model setups whose pricing varies -// per call should leave this unset and roll cost up themselves from -// UsageEvent. nil/empty table disables rollup at zero hot-path cost. +// WithPriceTable supplies the rates used to estimate cost for LLM calls +// whose provider does not report one. The loop accumulates TokenUsage +// across every call in a Run and emits RunCostEvent right before +// DoneEvent, with USD from table[model]. +// +// This is only needed for providers that bill silently. A provider that +// sets TokenUsage.CostUSD is already exact, and RunCostEvent fires for +// it whether or not a table is configured — leave this unset in that +// case rather than adding rates the loop will never consult. Adopters +// with router-style multi-model setups whose pricing varies per call +// and whose providers report nothing should still roll cost up +// themselves from UsageEvent. // // model is the key looked up in table for cost computation — pass // the canonical name of the model this loop drives. Unknown keys diff --git a/pkg/agent/regenerate.go b/pkg/agent/regenerate.go index f323c04..139acf8 100644 --- a/pkg/agent/regenerate.go +++ b/pkg/agent/regenerate.go @@ -101,7 +101,7 @@ func (al *AgentLoop) continueLogicLoop(ctx context.Context, sessionKey string, s ctx = WithSessionKey(ctx, sessionKey) // Mirror runLogicLoop: per-Run cost accumulator fires on every - // terminal exit when PriceTable is configured. Without this, + // terminal exit. Without this, // Regenerate and Continue Runs would never emit RunCostEvent and // adopters tracking billing would miss them silently. var emitCost func() diff --git a/pkg/history/types.go b/pkg/history/types.go index 01a86df..97aea2b 100644 --- a/pkg/history/types.go +++ b/pkg/history/types.go @@ -25,6 +25,14 @@ func newForkKey(parent string) (string, error) { // provider adapters iterate Parts instead of (or in addition to) Content. // When Parts is empty, adapters fall back to the plain-text Content path — // so every existing caller and test continues to work unchanged. +// +// The fallback is only for an empty Parts. An adapter that cannot render a +// populated Parts must fail rather than quietly answer from Content: a +// caller that sent an image and got back a fluent, well-formed reply has no +// way to tell that the model never saw it, and nothing in the logs +// distinguishes that from success. Such an adapter should also report +// ImageInput=false through agent.CapabilityProvider so consumers that need +// vision can reject it at construction instead of mid-run. type Message struct { Role string `json:"role"` Content string `json:"content"` diff --git a/pkg/llm/anthropic/anthropic.go b/pkg/llm/anthropic/anthropic.go index 558ee2b..819c3e4 100644 --- a/pkg/llm/anthropic/anthropic.go +++ b/pkg/llm/anthropic/anthropic.go @@ -34,6 +34,13 @@ type Provider struct { topP *float64 } +var _ agent.CapabilityProvider = (*Provider)(nil) + +// Capabilities reports features implemented by the Anthropic adapter. +func (*Provider) Capabilities() agent.LLMCapabilities { + return agent.LLMCapabilities{ImageInput: true, StructuredOutput: true} +} + // Option configures a Provider at construction. type Option func(*Provider) diff --git a/pkg/llm/gemini/gemini.go b/pkg/llm/gemini/gemini.go index 59b5518..afc5e34 100644 --- a/pkg/llm/gemini/gemini.go +++ b/pkg/llm/gemini/gemini.go @@ -25,6 +25,13 @@ type Provider struct { seed *int64 } +var _ agent.CapabilityProvider = (*Provider)(nil) + +// Capabilities reports features implemented by the Gemini adapter. +func (*Provider) Capabilities() agent.LLMCapabilities { + return agent.LLMCapabilities{ImageInput: true, StructuredOutput: true} +} + // Option configures a Provider at construction. type Option func(*Provider) diff --git a/pkg/llm/llmfake/capabilities_test.go b/pkg/llm/llmfake/capabilities_test.go new file mode 100644 index 0000000..9c38eb4 --- /dev/null +++ b/pkg/llm/llmfake/capabilities_test.go @@ -0,0 +1,23 @@ +package llmfake + +import ( + "testing" + + "github.com/hung12ct/gopheragent/pkg/agent" +) + +// The scripted fake is the in-tree provider that supports nothing, which is +// precisely the case a multimodal consumer needs to reject. Staying silent +// would leave it indistinguishable from a provider that simply has not +// declared itself. +func TestScriptedProviderDeclaresNoCapabilities(t *testing.T) { + var p agent.LLMProvider = &ScriptedProvider{} + + c, ok := p.(agent.CapabilityProvider) + if !ok { + t.Fatalf("ScriptedProvider does not implement agent.CapabilityProvider") + } + if got := c.Capabilities(); got != (agent.LLMCapabilities{}) { + t.Fatalf("Capabilities() = %+v, want the zero value", got) + } +} diff --git a/pkg/llm/llmfake/scripted.go b/pkg/llm/llmfake/scripted.go index ae12d4a..e84bbfb 100644 --- a/pkg/llm/llmfake/scripted.go +++ b/pkg/llm/llmfake/scripted.go @@ -62,6 +62,21 @@ type ScriptedProvider struct { idx int } +var _ agent.CapabilityProvider = (*ScriptedProvider)(nil) + +// Capabilities reports the zero value: the scripted fake honours neither +// image parts nor schema-enforced structured output. It replays its script +// verbatim and never reads the incoming messages' MediaParts, so a Turn's +// Content comes back whether or not an image was attached. +// +// Declaring that explicitly is the point. A multimodal consumer handed this +// fake gets a well-formed, confident answer produced without the model ever +// seeing an image — indistinguishable from success in a log — and this is +// what lets it reject the provider at construction instead. +func (*ScriptedProvider) Capabilities() agent.LLMCapabilities { + return agent.LLMCapabilities{} +} + // GenerateStream implements agent.LLMProvider. See Turn for the // dispatch rules. func (p *ScriptedProvider) GenerateStream(ctx context.Context, msgs []history.Message, registry *tools.Registry, stream chan<- agent.StreamEvent) (agent.LLMResult, error) { diff --git a/pkg/llm/openai/client.go b/pkg/llm/openai/client.go new file mode 100644 index 0000000..9b6cf72 --- /dev/null +++ b/pkg/llm/openai/client.go @@ -0,0 +1,158 @@ +package openai + +import ( + "errors" + "fmt" + "net/http" + "net/url" + "strings" + + "github.com/sashabaranov/go-openai" +) + +// clientConfig carries the transport settings every client in this package +// shares: which endpoint to talk to and what headers to add. +type clientConfig struct { + baseURL string + headers map[string]string +} + +// Option configures a chat Provider. Both the sampling options +// (WithTemperature, WithTopP, WithSeed) and the transport options +// (WithBaseURL, WithHTTPHeader) satisfy it, so one call can mix them. +type Option interface{ applyProvider(*Provider) } + +// ClientOption configures transport for any client in this package — +// Provider, Embedder, VisionAnalyzer, SummaryProvider. +// +// It is a strict subset of Option. Sampling has no meaning for an +// embedder, so NewEmbedder and friends accept only these: passing +// WithTemperature to one is a compile error rather than an option that +// silently does nothing. +type ClientOption interface { + Option + applyClient(*clientConfig) +} + +// providerOptionFunc adapts a plain func to Option, for settings that +// exist only on a chat Provider. +type providerOptionFunc func(*Provider) + +func (f providerOptionFunc) applyProvider(p *Provider) { f(p) } + +// clientOptionFunc adapts a plain func to ClientOption. applyProvider +// routes through the Provider's embedded clientConfig, which is what +// lets a single transport option work on every constructor. +type clientOptionFunc func(*clientConfig) + +func (f clientOptionFunc) applyClient(c *clientConfig) { f(c) } +func (f clientOptionFunc) applyProvider(p *Provider) { f(&p.cfg) } + +// WithBaseURL points the client at an OpenAI-compatible endpoint — +// OpenRouter, Ollama, vLLM, Groq, Together, or any gateway speaking the +// same API. +// +// This is the only way to keep an Embedder, VisionAnalyzer, or +// SummaryProvider off api.openai.com. Without it they call OpenAI +// directly, which on a deliberately local or gateway-only deployment +// means an unexpected egress and a key the operator may not hold. +// +// url must be absolute, HTTP(S), and free of embedded credentials, a +// query, or a fragment; it is validated at construction. Prefer HTTPS +// except for local development endpoints. +func WithBaseURL(url string) ClientOption { + return clientOptionFunc(func(c *clientConfig) { c.baseURL = url }) +} + +// WithHTTPHeader adds a header to every request the client makes. It is +// useful for compatible gateways that accept attribution or routing +// headers. +// +// Headers are applied after the SDK has built the request, so a name the +// SDK already sets is overwritten: passing "Authorization" replaces the +// bearer token derived from apiKey. Repeated calls with the same name +// keep the last value. +func WithHTTPHeader(name, value string) ClientOption { + return clientOptionFunc(func(c *clientConfig) { + if c.headers == nil { + c.headers = make(map[string]string) + } + c.headers[name] = value + }) +} + +// validateBaseURL normalizes and checks a compatible-endpoint URL, +// returning the trimmed form. Trailing slashes are stripped because the +// SDK appends its own path segment. +// +// Errors are returned unprefixed; each constructor adds its own +// "openai: : " so the message names the call the caller actually +// made instead of stacking the package prefix twice. +func validateBaseURL(raw string) (string, error) { + trimmed := strings.TrimRight(strings.TrimSpace(raw), "/") + if trimmed == "" { + return "", errors.New("baseURL is required") + } + u, err := url.Parse(trimmed) + if err != nil { + return "", fmt.Errorf("parsing baseURL: %w", err) + } + if u.Host == "" || (u.Scheme != "http" && u.Scheme != "https") || + u.User != nil || u.RawQuery != "" || u.Fragment != "" { + return "", errors.New("baseURL must be an absolute HTTP(S) URL without credentials, query, or fragment") + } + return trimmed, nil +} + +// newClient builds a go-openai client for apiKey under these transport +// settings. Every constructor in the package routes through here so that +// baseURL validation and header injection cannot drift between them. +func (c clientConfig) newClient(apiKey string) (*openai.Client, error) { + config := openai.DefaultConfig(apiKey) + if c.baseURL != "" { + validated, err := validateBaseURL(c.baseURL) + if err != nil { + return nil, err + } + config.BaseURL = validated + } + if len(c.headers) > 0 { + config.HTTPClient = headerDoer{base: config.HTTPClient, headers: c.headers} + } + return openai.NewClientWithConfig(config), nil +} + +// newClientFor resolves apiKey (falling back to OPENAI_API_KEY), applies +// opts, and builds the client. Shared by the non-chat constructors. +func newClientFor(apiKey, ctor string, opts []ClientOption) (*openai.Client, error) { + apiKey = resolveAPIKey(apiKey) + if apiKey == "" { + return nil, fmt.Errorf("openai: %s: API key is not set", ctor) + } + var cfg clientConfig + for _, opt := range opts { + opt.applyClient(&cfg) + } + client, err := cfg.newClient(apiKey) + if err != nil { + return nil, fmt.Errorf("openai: %s: %w", ctor, err) + } + return client, nil +} + +type headerDoer struct { + base openai.HTTPDoer + headers map[string]string +} + +// Do copies the request shallowly and clones only its header, the standard +// RoundTripper idiom: req.Clone would deep-copy the header a second time and +// throw the first copy away on every call. +func (d headerDoer) Do(req *http.Request) (*http.Response, error) { + clone := *req + clone.Header = req.Header.Clone() + for name, value := range d.headers { + clone.Header.Set(name, value) + } + return d.base.Do(&clone) +} diff --git a/pkg/llm/openai/client_test.go b/pkg/llm/openai/client_test.go new file mode 100644 index 0000000..9a35875 --- /dev/null +++ b/pkg/llm/openai/client_test.go @@ -0,0 +1,134 @@ +package openai + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/hung12ct/gopheragent/pkg/history" +) + +// requirePath fails clearly when the server was never reached, instead of +// panicking on a nil URL. +func requirePath(t *testing.T, seen *http.Request, suffix string) { + t.Helper() + if seen.URL == nil { + t.Fatalf("no request reached the local server; the client never left api.openai.com") + } + if !strings.HasSuffix(seen.URL.Path, suffix) { + t.Fatalf("request path = %q, want a local path ending in %q", seen.URL.Path, suffix) + } +} + +// recordingServer captures the path and headers of the first request it sees. +func recordingServer(t *testing.T, body string) (*httptest.Server, *http.Request) { + t.Helper() + var seen http.Request + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + seen = *r + seen.Header = r.Header.Clone() + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(body)) + })) + t.Cleanup(srv.Close) + return srv, &seen +} + +// The regression this whole change exists for: without WithBaseURL these +// clients call api.openai.com, so a deliberately local deployment leaks text +// to a third party and demands a key the operator may not hold. +func TestNonChatClientsHonourBaseURL(t *testing.T) { + t.Run("embedder", func(t *testing.T) { + srv, seen := recordingServer(t, `{"data":[{"embedding":[0.1,0.2],"index":0}]}`) + e, err := NewEmbedder("k", "nomic-embed-text", WithBaseURL(srv.URL+"/v1")) + if err != nil { + t.Fatalf("NewEmbedder: %v", err) + } + if _, err := e.Embed(context.Background(), []string{"hello"}); err != nil { + t.Fatalf("Embed: %v", err) + } + requirePath(t, seen, "/embeddings") + }) + + t.Run("vision analyzer", func(t *testing.T) { + srv, seen := recordingServer(t, `{"choices":[{"message":{"role":"assistant","content":"ok"}}]}`) + v, err := NewVisionAnalyzer("k", "llava", WithBaseURL(srv.URL+"/v1")) + if err != nil { + t.Fatalf("NewVisionAnalyzer: %v", err) + } + if _, err := v.Analyze(context.Background(), "https://example.com/a.png", "describe"); err != nil { + t.Fatalf("Analyze: %v", err) + } + requirePath(t, seen, "/chat/completions") + }) + + t.Run("summary provider", func(t *testing.T) { + srv, seen := recordingServer(t, `{"choices":[{"message":{"role":"assistant","content":"ok"}}]}`) + sp, err := NewSummaryProvider("k", "llama3", WithBaseURL(srv.URL+"/v1")) + if err != nil { + t.Fatalf("NewSummaryProvider: %v", err) + } + // Non-empty messages required: SummarizeBehaviors short-circuits on + // an empty slice and never reaches the transport. + msgs := []history.Message{{Role: "user", Content: "hello"}} + if _, err := sp.SummarizeBehaviors(context.Background(), msgs, ""); err != nil { + t.Fatalf("SummarizeBehaviors: %v", err) + } + requirePath(t, seen, "/chat/completions") + }) +} + +// Transport options apply to the non-chat clients too, not just the provider. +func TestNonChatClientsHonourHTTPHeader(t *testing.T) { + srv, seen := recordingServer(t, `{"data":[{"embedding":[0.1],"index":0}]}`) + e, err := NewEmbedder("k", "m", WithBaseURL(srv.URL+"/v1"), WithHTTPHeader("X-Title", "gopheragent")) + if err != nil { + t.Fatalf("NewEmbedder: %v", err) + } + if _, err := e.Embed(context.Background(), []string{"hello"}); err != nil { + t.Fatalf("Embed: %v", err) + } + if got := seen.Header.Get("X-Title"); got != "gopheragent" { + t.Fatalf("X-Title = %q, want gopheragent", got) + } +} + +// Every constructor validates through the same helper, so a bad endpoint +// fails at construction rather than at the first request. +func TestBaseURLValidationAppliesToEveryConstructor(t *testing.T) { + for _, bad := range []string{ + "openrouter.ai/api/v1", + "https://user:secret@openrouter.ai/api/v1", + "https://openrouter.ai/api/v1?key=value", + "https://openrouter.ai/api/v1#fragment", + } { + t.Run(bad, func(t *testing.T) { + if _, err := New("k", "m", WithBaseURL(bad)); err == nil { + t.Fatal("New accepted a malformed baseURL") + } + if _, err := NewEmbedder("k", "m", WithBaseURL(bad)); err == nil { + t.Fatal("NewEmbedder accepted a malformed baseURL") + } + if _, err := NewVisionAnalyzer("k", "m", WithBaseURL(bad)); err == nil { + t.Fatal("NewVisionAnalyzer accepted a malformed baseURL") + } + if _, err := NewSummaryProvider("k", "m", WithBaseURL(bad)); err == nil { + t.Fatal("NewSummaryProvider accepted a malformed baseURL") + } + }) + } +} + +// A trailing slash is normalized away because the SDK appends its own path +// segment; leaving it produces a double slash the gateway may reject. +func TestValidateBaseURLTrimsTrailingSlash(t *testing.T) { + got, err := validateBaseURL(" https://openrouter.ai/api/v1/ ") + if err != nil { + t.Fatalf("validateBaseURL: %v", err) + } + if got != "https://openrouter.ai/api/v1" { + t.Fatalf("validateBaseURL = %q, want the trimmed form", got) + } +} diff --git a/pkg/llm/openai/embedder_openai.go b/pkg/llm/openai/embedder_openai.go index 2bc7aee..24d262c 100644 --- a/pkg/llm/openai/embedder_openai.go +++ b/pkg/llm/openai/embedder_openai.go @@ -2,9 +2,7 @@ package openai import ( "context" - "errors" "fmt" - "os" "github.com/hung12ct/gopheragent/pkg/tools" "github.com/sashabaranov/go-openai" @@ -23,21 +21,18 @@ type Embedder struct { // NewEmbedder constructs an embedder. apiKey falls back to // OPENAI_API_KEY. model defaults to text-embedding-3-small. -func NewEmbedder(apiKey string, model string) (*Embedder, error) { - if apiKey == "" { - apiKey = os.Getenv("OPENAI_API_KEY") - } - if apiKey == "" { - return nil, errors.New("OPENAI_API_KEY is not set in environment") +// Pass WithBaseURL to embed against an OpenAI-compatible endpoint +// (Ollama, vLLM, Together, a gateway) instead of api.openai.com. +func NewEmbedder(apiKey string, model string, opts ...ClientOption) (*Embedder, error) { + client, err := newClientFor(apiKey, "NewEmbedder", opts) + if err != nil { + return nil, err } m := openai.SmallEmbedding3 if model != "" { m = openai.EmbeddingModel(model) } - return &Embedder{ - client: openai.NewClient(apiKey), - model: m, - }, nil + return &Embedder{client: client, model: m}, nil } // Embed returns one vector per input in the same order. Empty input returns diff --git a/pkg/llm/openai/header_doer_test.go b/pkg/llm/openai/header_doer_test.go new file mode 100644 index 0000000..e7f4c06 --- /dev/null +++ b/pkg/llm/openai/header_doer_test.go @@ -0,0 +1,61 @@ +package openai + +import ( + "net/http" + "net/http/httptest" + "testing" +) + +// recordingDoer captures the request headerDoer hands to the underlying client. +type recordingDoer struct{ got *http.Request } + +func (d *recordingDoer) Do(req *http.Request) (*http.Response, error) { + *d.got = *req + return &http.Response{StatusCode: http.StatusOK, Body: http.NoBody}, nil +} + +// The doer must not write through to the caller's request: go-openai reuses +// the request it built, and a mutated header would leak the injected values +// into unrelated call sites. +func TestHeaderDoerLeavesCallerRequestUntouched(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, "https://example.com/v1/chat/completions", http.NoBody) + req.Header.Set("Authorization", "Bearer original") + + var seen http.Request + d := headerDoer{ + base: &recordingDoer{got: &seen}, + headers: map[string]string{"X-Title": "gopheragent"}, + } + if _, err := d.Do(req); err != nil { + t.Fatalf("Do: %v", err) + } + + if got := req.Header.Get("X-Title"); got != "" { + t.Fatalf("caller request X-Title = %q, want it left unset", got) + } + if got := seen.Header.Get("X-Title"); got != "gopheragent" { + t.Fatalf("forwarded X-Title = %q, want gopheragent", got) + } + if got := seen.Header.Get("Authorization"); got != "Bearer original" { + t.Fatalf("forwarded Authorization = %q, want the SDK value preserved", got) + } +} + +// Headers are applied last, so a caller can deliberately replace one the SDK +// set. WithHTTPHeader documents this; the test pins it as intended behavior. +func TestHeaderDoerOverridesSDKHeader(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, "https://example.com/v1/chat/completions", http.NoBody) + req.Header.Set("Authorization", "Bearer original") + + var seen http.Request + d := headerDoer{ + base: &recordingDoer{got: &seen}, + headers: map[string]string{"Authorization": "Bearer override"}, + } + if _, err := d.Do(req); err != nil { + t.Fatalf("Do: %v", err) + } + if got := seen.Header.Get("Authorization"); got != "Bearer override" { + t.Fatalf("forwarded Authorization = %q, want Bearer override", got) + } +} diff --git a/pkg/llm/openai/openai.go b/pkg/llm/openai/openai.go index e8139d0..8b16fe3 100644 --- a/pkg/llm/openai/openai.go +++ b/pkg/llm/openai/openai.go @@ -37,51 +37,68 @@ type Provider struct { temperature *float64 topP *float64 seed *int64 + cfg clientConfig } -// Option configures a Provider at construction. -type Option func(*Provider) +var _ agent.CapabilityProvider = (*Provider)(nil) // WithTemperature pins the sampling temperature (0.0–2.0 for OpenAI). // Use 0 for maximally reproducible classification/extraction turns; unset // keeps the provider default. An explicit 0 is honored (the SDK's omitempty // would otherwise drop it and silently revert to the provider default). func WithTemperature(t float64) Option { - return func(p *Provider) { p.temperature = &t } + return providerOptionFunc(func(p *Provider) { p.temperature = &t }) } // WithTopP pins nucleus sampling. OpenAI recommends adjusting either // temperature or top_p, not both. Unset keeps the provider default. func WithTopP(v float64) Option { - return func(p *Provider) { p.topP = &v } + return providerOptionFunc(func(p *Provider) { p.topP = &v }) } // WithSeed requests best-effort deterministic sampling. Determinism is not // guaranteed across model versions or backend changes — pair with a pinned // temperature for the strongest reproducibility OpenAI offers. func WithSeed(n int64) Option { - return func(p *Provider) { p.seed = &n } + return providerOptionFunc(func(p *Provider) { p.seed = &n }) +} + +// Capabilities reports features implemented by this adapter. Compatible +// endpoints still need model-specific discovery when their catalog varies. +func (*Provider) Capabilities() agent.LLMCapabilities { + return agent.LLMCapabilities{ImageInput: true, StructuredOutput: true} +} + +// resolveAPIKey falls back to OPENAI_API_KEY when apiKey is empty. +func resolveAPIKey(apiKey string) string { + if apiKey == "" { + return os.Getenv("OPENAI_API_KEY") + } + return apiKey } // New creates a wrapper over OpenAI's API. // Auto-discovers OPENAI_API_KEY from environment if apiKey is empty. +// +// Pass WithBaseURL to target an OpenAI-compatible endpoint instead; +// NewCompat is the same thing with baseURL required rather than optional. func New(apiKey string, model string, opts ...Option) (*Provider, error) { + apiKey = resolveAPIKey(apiKey) if apiKey == "" { - apiKey = os.Getenv("OPENAI_API_KEY") - } - if apiKey == "" { - return nil, errors.New("OPENAI_API_KEY is not set in environment") + return nil, errors.New("openai: New: OPENAI_API_KEY is not set in environment") } if model == "" { model = openai.GPT4o } - p := &Provider{ - client: openai.NewClient(apiKey), - model: model, - } + p := &Provider{model: model} for _, opt := range opts { - opt(p) + opt.applyProvider(p) + } + client, err := p.cfg.newClient(apiKey) + if err != nil { + return nil, fmt.Errorf("openai: New: %w", err) } + p.client = client return p, nil } diff --git a/pkg/llm/openai/openai_compat.go b/pkg/llm/openai/openai_compat.go index 7024ea7..31fa25d 100644 --- a/pkg/llm/openai/openai_compat.go +++ b/pkg/llm/openai/openai_compat.go @@ -2,42 +2,38 @@ package openai import ( "errors" - "os" - - "github.com/sashabaranov/go-openai" + "fmt" ) // NewCompat creates a provider for any OpenAI-compatible API endpoint. -// This covers: Google Gemini (via OpenAI compat), Ollama, Groq, Together AI, vLLM, etc. +// This covers: OpenRouter, Google Gemini (via OpenAI compat), Ollama, Groq, +// Together AI, vLLM, etc. baseURL must be an absolute HTTP(S) URL without +// embedded credentials, a query, or a fragment. Prefer HTTPS except for local +// development endpoints. +// +// It is New plus WithBaseURL, with baseURL required rather than optional and +// model required rather than defaulted — a compatible endpoint has no +// sensible default model. Note that this configures the chat provider only: +// point NewEmbedder, NewVisionAnalyzer, and NewSummaryProvider at the same +// endpoint with WithBaseURL, or they will call api.openai.com. // // Examples: // // Gemini: NewCompat("GEMINI_API_KEY", "gemini-2.0-flash", "https://generativelanguage.googleapis.com/v1beta/openai") +// OpenRouter: NewCompat("OPENROUTER_API_KEY", "openai/gpt-4o", "https://openrouter.ai/api/v1") // Ollama: NewCompat("ollama", "llama3", "http://localhost:11434/v1") // Groq: NewCompat("GROQ_KEY", "llama-3.3-70b-versatile", "https://api.groq.com/openai/v1") func NewCompat(apiKey string, model string, baseURL string, opts ...Option) (*Provider, error) { - if apiKey == "" { - apiKey = os.Getenv("OPENAI_API_KEY") - } - if apiKey == "" { - return nil, errors.New("API key is not set") - } - if baseURL == "" { - return nil, errors.New("baseURL is required for OpenAI-compatible provider") + if resolveAPIKey(apiKey) == "" { + return nil, errors.New("openai: NewCompat: API key is not set") } if model == "" { - return nil, errors.New("model is required for OpenAI-compatible provider") - } - - config := openai.DefaultConfig(apiKey) - config.BaseURL = baseURL - - p := &Provider{ - client: openai.NewClientWithConfig(config), - model: model, + return nil, errors.New("openai: NewCompat: model is required") } - for _, opt := range opts { - opt(p) + // Validated here rather than left to New so the error names NewCompat, + // which is the call the caller actually made. + if _, err := validateBaseURL(baseURL); err != nil { + return nil, fmt.Errorf("openai: NewCompat: %w", err) } - return p, nil + return New(apiKey, model, append([]Option{WithBaseURL(baseURL)}, opts...)...) } diff --git a/pkg/llm/openai/openai_compat_test.go b/pkg/llm/openai/openai_compat_test.go new file mode 100644 index 0000000..c6850bb --- /dev/null +++ b/pkg/llm/openai/openai_compat_test.go @@ -0,0 +1,53 @@ +package openai + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +func TestNewCompatRejectsMalformedBaseURLs(t *testing.T) { + for _, baseURL := range []string{ + "openrouter.ai/api/v1", + "https://user:secret@openrouter.ai/api/v1", + "https://openrouter.ai/api/v1?key=value", + "https://openrouter.ai/api/v1#fragment", + } { + t.Run(baseURL, func(t *testing.T) { + _, err := NewCompat("test-key", "test/model", baseURL) + if err == nil || !strings.Contains(err.Error(), "baseURL") { + t.Fatalf("NewCompat(%q) error = %v, want baseURL validation", baseURL, err) + } + }) + } +} + +func TestNewCompatSendsConfiguredHeaders(t *testing.T) { + var title string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + title = r.Header.Get("X-Title") + w.Header().Set("Content-Type", "text/event-stream") + _, _ = w.Write([]byte("data: [DONE]\n\n")) + })) + defer srv.Close() + + p, err := NewCompat("test-key", "test/model", srv.URL+"/v1", + WithHTTPHeader("X-Title", "gopheragent")) + if err != nil { + t.Fatalf("NewCompat: %v", err) + } + if _, _, err := runStream(t, p); err != nil { + t.Fatalf("GenerateStream: %v", err) + } + if title != "gopheragent" { + t.Fatalf("X-Title = %q, want gopheragent", title) + } +} + +func TestOpenAIReportsMultimodalStructuredTransport(t *testing.T) { + caps := (&Provider{}).Capabilities() + if !caps.ImageInput || !caps.StructuredOutput { + t.Fatalf("Capabilities = %+v, want image input and structured output", caps) + } +} diff --git a/pkg/llm/openai/summary_provider.go b/pkg/llm/openai/summary_provider.go index a61082f..f94e621 100644 --- a/pkg/llm/openai/summary_provider.go +++ b/pkg/llm/openai/summary_provider.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "io" - "os" "strings" "github.com/hung12ct/gopheragent/pkg/history" @@ -30,20 +29,17 @@ type SummaryProvider struct { // NewSummaryProvider creates an OpenAI-backed SummaryProvider. // Auto-discovers OPENAI_API_KEY from environment if apiKey is empty. // model defaults to "gpt-4o-mini" (cheap + fast — ideal for background summarization). -func NewSummaryProvider(apiKey, model string) (*SummaryProvider, error) { - if apiKey == "" { - apiKey = os.Getenv("OPENAI_API_KEY") - } - if apiKey == "" { - return nil, errors.New("OPENAI_API_KEY is not set in environment") +// Pass WithBaseURL to summarize against an OpenAI-compatible endpoint +// instead of api.openai.com. +func NewSummaryProvider(apiKey, model string, opts ...ClientOption) (*SummaryProvider, error) { + client, err := newClientFor(apiKey, "NewSummaryProvider", opts) + if err != nil { + return nil, err } if model == "" { model = openai.GPT4oMini } - return &SummaryProvider{ - client: openai.NewClient(apiKey), - model: model, - }, nil + return &SummaryProvider{client: client, model: model}, nil } // SummarizeBehaviors analyzes recent messages and merges new evidence into the diff --git a/pkg/llm/openai/vision.go b/pkg/llm/openai/vision.go index 62d40ed..5d7d32d 100644 --- a/pkg/llm/openai/vision.go +++ b/pkg/llm/openai/vision.go @@ -3,7 +3,6 @@ package openai import ( "context" "fmt" - "os" "strings" "github.com/sashabaranov/go-openai" @@ -26,17 +25,17 @@ type VisionAnalyzer struct { // NewVisionAnalyzer builds an analyzer. apiKey defaults to // OPENAI_API_KEY; model defaults to "gpt-4o" (smallest model with // reliable vision support across all image tasks). -func NewVisionAnalyzer(apiKey, model string) (*VisionAnalyzer, error) { - if apiKey == "" { - apiKey = os.Getenv("OPENAI_API_KEY") - } - if apiKey == "" { - return nil, fmt.Errorf("openai: OPENAI_API_KEY not set") +// Pass WithBaseURL to analyze against an OpenAI-compatible endpoint +// instead of api.openai.com. +func NewVisionAnalyzer(apiKey, model string, opts ...ClientOption) (*VisionAnalyzer, error) { + client, err := newClientFor(apiKey, "NewVisionAnalyzer", opts) + if err != nil { + return nil, err } if model == "" { model = "gpt-4o" } - return &VisionAnalyzer{client: openai.NewClient(apiKey), model: model}, nil + return &VisionAnalyzer{client: client, model: model}, nil } // Analyze sends media + prompt to the vision model and returns the response. diff --git a/pkg/llm/router.go b/pkg/llm/router.go index 65f5203..b1082e3 100644 --- a/pkg/llm/router.go +++ b/pkg/llm/router.go @@ -50,6 +50,38 @@ func (r *RouterProvider) AddRoute(condition RouteCondition, provider agent.LLMPr return r } +var _ agent.CapabilityProvider = (*RouterProvider)(nil) + +// Capabilities reports the intersection over the fallback and every route: +// a feature holds only when every provider the router might dispatch to +// supports it. The router cannot answer per-route, because the route is +// chosen from the conversation and is unknown until GenerateStream runs. +// +// A member that does not implement agent.CapabilityProvider makes no claim, +// and the intersection counts it as supporting nothing — one undeclared +// member collapses the whole report to the zero value. That is deliberate: +// under-reporting costs a caller a spurious rejection at construction, while +// over-reporting costs it a wrong answer at run time with nothing in the log +// to distinguish it from a working call. +func (r *RouterProvider) Capabilities() agent.LLMCapabilities { + caps := providerCapabilities(r.fallback) + for _, rt := range r.routes { + c := providerCapabilities(rt.provider) + caps.ImageInput = caps.ImageInput && c.ImageInput + caps.StructuredOutput = caps.StructuredOutput && c.StructuredOutput + } + return caps +} + +// providerCapabilities reports p's capabilities, or the zero value when p +// makes no claim (including when p is nil). +func providerCapabilities(p agent.LLMProvider) agent.LLMCapabilities { + if c, ok := p.(agent.CapabilityProvider); ok { + return c.Capabilities() + } + return agent.LLMCapabilities{} +} + // GenerateStream selects the appropriate provider and delegates. func (r *RouterProvider) GenerateStream( ctx context.Context, diff --git a/pkg/llm/router_capabilities_test.go b/pkg/llm/router_capabilities_test.go new file mode 100644 index 0000000..90e98bf --- /dev/null +++ b/pkg/llm/router_capabilities_test.go @@ -0,0 +1,72 @@ +package llm + +import ( + "context" + "testing" + + "github.com/hung12ct/gopheragent/pkg/agent" + "github.com/hung12ct/gopheragent/pkg/history" + "github.com/hung12ct/gopheragent/pkg/tools" +) + +// capableStub is a provider that declares a fixed capability set. +type capableStub struct{ caps agent.LLMCapabilities } + +func (s *capableStub) GenerateStream(_ context.Context, _ []history.Message, _ *tools.Registry, _ chan<- agent.StreamEvent) (agent.LLMResult, error) { + return agent.LLMResult{}, nil +} + +func (s *capableStub) Capabilities() agent.LLMCapabilities { return s.caps } + +func multimodal() *capableStub { + return &capableStub{caps: agent.LLMCapabilities{ImageInput: true, StructuredOutput: true}} +} + +func TestRouterCapabilitiesIntersectsMembers(t *testing.T) { + var called string + textOnly := &capableStub{caps: agent.LLMCapabilities{StructuredOutput: true}} + + for _, tc := range []struct { + name string + router *RouterProvider + want agent.LLMCapabilities + }{ + { + name: "all members multimodal", + router: NewRouterProvider(multimodal()).AddRoute(Always(), multimodal()), + want: agent.LLMCapabilities{ImageInput: true, StructuredOutput: true}, + }, + { + name: "one text-only route drops image input", + router: NewRouterProvider(multimodal()).AddRoute(Always(), textOnly), + want: agent.LLMCapabilities{StructuredOutput: true}, + }, + { + name: "text-only fallback drops image input", + router: NewRouterProvider(textOnly).AddRoute(Always(), multimodal()), + want: agent.LLMCapabilities{StructuredOutput: true}, + }, + { + // newStub makes no claim, so the intersection must collapse + // rather than inherit the declaring member's answer. + name: "undeclared member collapses the report", + router: NewRouterProvider(multimodal()).AddRoute(Always(), newStub("quiet", &called)), + want: agent.LLMCapabilities{}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + if got := tc.router.Capabilities(); got != tc.want { + t.Fatalf("Capabilities() = %+v, want %+v", got, tc.want) + } + }) + } +} + +// A router with no routes answers for its fallback alone. +func TestRouterCapabilitiesWithNoRoutesUsesFallback(t *testing.T) { + r := NewRouterProvider(multimodal()) + want := agent.LLMCapabilities{ImageInput: true, StructuredOutput: true} + if got := r.Capabilities(); got != want { + t.Fatalf("Capabilities() = %+v, want %+v", got, want) + } +} diff --git a/pkg/telemetry/otelllm/capabilities_test.go b/pkg/telemetry/otelllm/capabilities_test.go new file mode 100644 index 0000000..b5c8d92 --- /dev/null +++ b/pkg/telemetry/otelllm/capabilities_test.go @@ -0,0 +1,51 @@ +package otelllm_test + +import ( + "context" + "testing" + + "github.com/hung12ct/gopheragent/pkg/agent" + "github.com/hung12ct/gopheragent/pkg/history" + "github.com/hung12ct/gopheragent/pkg/telemetry/otelllm" + "github.com/hung12ct/gopheragent/pkg/tools" +) + +// capableFake declares capabilities in addition to implementing LLMProvider. +type capableFake struct{ caps agent.LLMCapabilities } + +func (f *capableFake) GenerateStream(_ context.Context, _ []history.Message, _ *tools.Registry, _ chan<- agent.StreamEvent) (agent.LLMResult, error) { + return agent.LLMResult{}, nil +} + +func (f *capableFake) Capabilities() agent.LLMCapabilities { return f.caps } + +// Wrapping for tracing must not erase a capability the caller checks for — +// otherwise enabling telemetry silently disables a consumer's fail-fast guard. +func TestNewProviderForwardsCapabilities(t *testing.T) { + _, _, tp, mp := newRecorders(t) + want := agent.LLMCapabilities{ImageInput: true, StructuredOutput: true} + + wrapped := otelllm.NewProvider(&capableFake{caps: want}, + otelllm.WithTracer(tp.Tracer("test")), otelllm.WithMeter(mp.Meter("test"))) + + c, ok := wrapped.(agent.CapabilityProvider) + if !ok { + t.Fatalf("wrapped provider does not implement agent.CapabilityProvider") + } + if got := c.Capabilities(); got != want { + t.Fatalf("Capabilities() = %+v, want %+v", got, want) + } +} + +// The mirror case: a provider that makes no claim must stay unknown. Wrapping +// it must not manufacture a zero-value report that reads as "supports nothing". +func TestNewProviderDoesNotInventCapabilities(t *testing.T) { + _, _, tp, mp := newRecorders(t) + + wrapped := otelllm.NewProvider(&fakeProvider{}, + otelllm.WithTracer(tp.Tracer("test")), otelllm.WithMeter(mp.Meter("test"))) + + if _, ok := wrapped.(agent.CapabilityProvider); ok { + t.Fatalf("wrapping an undeclared provider must not implement agent.CapabilityProvider") + } +} diff --git a/pkg/telemetry/otelllm/provider.go b/pkg/telemetry/otelllm/provider.go index 70a27bd..c0d818f 100644 --- a/pkg/telemetry/otelllm/provider.go +++ b/pkg/telemetry/otelllm/provider.go @@ -83,9 +83,28 @@ func NewProvider(next agent.LLMProvider, opts ...Option) agent.LLMProvider { model: cfg.model, } p.initInstruments(cfg.meter) + if c, ok := next.(agent.CapabilityProvider); ok { + return &capableProvider{instrumentedProvider: p, caps: c} + } return p } +// capableProvider is the variant returned when the wrapped provider reports +// capabilities. Selecting the type at construction — rather than always +// implementing Capabilities on instrumentedProvider — keeps "absent means +// unknown" intact: wrapping a provider that makes no claim must not turn it +// into a provider claiming it supports nothing. +type capableProvider struct { + *instrumentedProvider + caps agent.CapabilityProvider +} + +// Capabilities forwards the wrapped provider's report so that adding tracing +// does not erase a capability the caller checks for. +func (p *capableProvider) Capabilities() agent.LLMCapabilities { + return p.caps.Capabilities() +} + // instrumentedProvider decorates an agent.LLMProvider with OTel spans/metrics. type instrumentedProvider struct { next agent.LLMProvider