diff --git a/convert.go b/convert.go index 745f913..28e79ae 100644 --- a/convert.go +++ b/convert.go @@ -4,6 +4,7 @@ import ( "reflect" "gitea.maximumdirect.net/eric/promptkit/internal/domain" + "gitea.maximumdirect.net/eric/promptkit/internal/jsonvalue" ) func toDomainRunRequest(req RunRequest) (domain.RunRequest, error) { @@ -133,7 +134,7 @@ func toDomainExecutionTargetOverride(override *ExecutionTargetOverride) (*domain if override == nil { return nil, nil } - extraParams, err := copyPublicJSONMap(override.ExtraParams) + extraParams, err := jsonvalue.CopyMap(override.ExtraParams) if err != nil { return nil, err } diff --git a/docs/internal/llm.md b/docs/internal/llm.md index e441508..d1fbc72 100644 --- a/docs/internal/llm.md +++ b/docs/internal/llm.md @@ -36,6 +36,10 @@ caller's client. Generation then: 5. performs the outbound request under the applicable deadlines; and 6. decodes the first response choice and token usage. +`internal/llm` owns the set of reserved OpenAI-compatible request fields used +when validating extra parameters. Backend registration consumes the same rule +without making the model client depend on registry configuration. + The implementation has no retry loop, tool-call support, provider catalog, inbound HTTP behavior, or durable session store. diff --git a/docs/internal/overview.md b/docs/internal/overview.md index 9df10b2..675608a 100644 --- a/docs/internal/overview.md +++ b/docs/internal/overview.md @@ -14,17 +14,18 @@ contributor workflow and validation. | Root `promptkit` package | Provides the supported engine facade, source, backend-registration, and injection options, public request and result values, profile construction, extension interfaces, value conversion, redacted formatting, and public error mapping. | [Package GoDoc](../../doc.go), [backend API](../../backends.go), [engine assembly](../../engine.go) | | `examples/go-library/prepare` | Demonstrates an offline downstream consumer using a prompt file, in-memory profile, inline input, and `Prepare`. It is not a public library package. | [Example program](../../examples/go-library/prepare/main.go) | | `examples/go-library/run` | Demonstrates an offline downstream consumer using a prompt file, in-memory profile, inline input, an injected deterministic model client, and `Run`. It is not a public library package. | [Example program](../../examples/go-library/run/main.go) | -| `internal/backend` | Constructs each engine's immutable registry from the built-in OpenRouter definition and consumer additions, validates and defensively copies definitions, and owns the shared reserved request-field rule. | [Backend registry](../../internal/backend/registry.go) | +| `internal/backend` | Constructs each engine's immutable registry from the built-in OpenRouter definition and consumer additions, validates and defensively copies definitions through the shared JSON-value package, and consumes the LLM-owned OpenAI-compatible reserved request-field rule. | [Backend registry](../../internal/backend/registry.go) | | `internal/domain` | Defines internal framework values for requests, artifacts, prompt definitions, profiles, execution targets, rendering, generation, and validation. | [Domain declarations](../../internal/domain/domain.go) | | `internal/defaults` | Defines application-neutral framework constants and constructs the default execution target. It contains no CLI, server, or inbound HTTP limits. | [Framework defaults](../../internal/defaults/defaults.go) | | `internal/filecatalog` | Provides deterministic YAML discovery and path helpers for operating-system filesystems and `fs.FS` sources. | [File catalog](../../internal/filecatalog/catalog.go) | +| `internal/jsonvalue` | Validates and deeply copies JSON-compatible extra-parameter trees while preserving supported concrete value types. | [JSON values](../../internal/jsonvalue/jsonvalue.go) | | `internal/promptdef` | Loads strictly decoded, validated prompt definitions from filesystem and `fs.FS` sources, including version selection and contained file-backed message content. | [Framework formats](../formats.md), [prompt-definition repository](../../internal/promptdef/filesystem_repository.go) | | `internal/profile` | Loads strictly decoded, validated execution profiles, including backend selection, from filesystem and `fs.FS` sources and composes repositories with error-preserving fallback. | [Framework formats](../formats.md), [profile repositories](../../internal/profile/filesystem_repository.go) | | `internal/profile/builtin` | Embeds the built-in profile catalog, whose entries select OpenRouter, and combines it with an optional primary repository. | [Built-in catalog](../formats.md#built-in-profile-catalog), [repository](../../internal/profile/builtin/repository.go) | | `internal/prompt` | Renders prompt messages from Go templates with artifact, variable, session, and cache-control data. | [Go-template renderer](../../internal/prompt/go_renderer.go) | | `internal/artifact` | Resolves ordinary inline and unrestricted caller-selected file references into copied artifacts with metadata and hashes. | [Internal sources and validation](sources.md) | | `internal/validate` | Validates basic, JSON, and JSON Schema output using operating-system filesystem or `fs.FS` schema sources. | [Framework formats](../formats.md#schemas), [internal sources and validation](sources.md) | -| `internal/llm` | Defines the internal generation boundary and implements outbound OpenAI-compatible chat requests from resolved execution targets, including response decoding, authentication, and deadline handling. | [Internal model client](llm.md) | +| `internal/llm` | Defines the internal generation boundary and implements outbound OpenAI-compatible chat requests from resolved execution targets, including response decoding, authentication, deadline handling, and ownership of the OpenAI-compatible reserved request-field policy. | [Internal model client](llm.md) | | `internal/usecase` | Resolves backend, profile, and request settings and coordinates preparation and execution across internal sources, rendering, artifact loading, generation, validation, and optional repair. | [Internal runner](runner.md) | The root package assembles these internal components without exposing their diff --git a/docs/policy/architecture.md b/docs/policy/architecture.md index c114392..1885716 100644 --- a/docs/policy/architecture.md +++ b/docs/policy/architecture.md @@ -27,6 +27,8 @@ The implemented internal components consist of: constructs the default execution target; - `internal/filecatalog`, which discovers YAML files and provides source-path helpers for filesystem and `fs.FS` consumers; +- `internal/jsonvalue`, which validates and defensively copies JSON-compatible + extra-parameter trees; - `internal/promptdef`, which loads and validates prompt definitions from filesystem and `fs.FS` sources; - `internal/profile`, which loads, validates, and overlays execution profiles @@ -76,9 +78,11 @@ downstream consumers, including Scriptorium narrow injected abstractions ``` -The backend registry depends on the domain model, has no mutation API after -construction, and shares its OpenAI-compatible reserved request-field rule -with the model client. The facade coordinates internal components and adapts +The backend registry depends on the domain model and shared JSON-value +validation, has no mutation API after construction, and consumes the +OpenAI-compatible reserved request-field rule owned by the model client. The +model client does not depend on registry configuration. The facade coordinates +internal components and adapts the supported public extension interfaces to narrow internal abstractions. Internal components must not depend on consumers or on Scriptorium. diff --git a/docs/roadmap/backends.md b/docs/roadmap/backends.md deleted file mode 100644 index 8116c00..0000000 --- a/docs/roadmap/backends.md +++ /dev/null @@ -1,182 +0,0 @@ -# Extensible LLM Backend Registry - -**Status:** Complete. - -## Purpose - -This roadmap defines the scope and target end state for formal -OpenAI-compatible backend support in Promptkit. It establishes the behavioral -boundary and policy choices for the work. - -This document is planning material, not a description of current behavior. -Current public contracts remain owned by Go declarations and GoDoc, framework -file formats by the [format reference](../formats.md), and outbound HTTP -behavior by the -[OpenAI-compatible integration contract](../integrations/openai-compatible-chat.md). - -## Motivation - -Execution profiles currently carry backend connection and authentication -details alongside model and generation settings. This repeats values such as -the OpenRouter endpoint and `OPENROUTER_API_KEY` across profiles, leaves the -built-in catalog implicitly tied to one backend, and gives consumers no -explicit extension point for naming other OpenAI-compatible services. - -Promptkit should distinguish: - -- a **backend**, which identifies reusable connection, authentication, and - limited backend-wide request defaults; and -- a **profile**, which selects a model and its reusable generation settings. - -That distinction should let consumers configure OpenRouter, OpenAI, -local-network services, or other OpenAI-compatible deployments without -duplicating backend defaults in every profile or replacing Promptkit's model -client. - -## Scope - -The work introduces an engine-scoped registry of OpenAI-compatible backend -definitions. - -Each backend definition will provide: - -- a stable, non-empty backend ID; -- a default OpenAI-compatible base endpoint; -- an optional environment-variable name for its API key; and -- limited JSON-compatible request defaults when they are genuinely - backend-wide rather than model-specific. - -Promptkit will provide a small built-in backend catalog, initially containing -OpenRouter. Downstream consumers will be able to register additional backend -IDs during engine construction. Registration will be explicit and local to an -engine; it will not mutate package-global state. - -Profiles will be able to name a backend. A profile that selects a backend may -override its default endpoint and other supported defaults while continuing to -own its model and generation settings. Existing endpoint-based custom profiles -will remain supported without requiring a backend ID. - -The built-in model profile catalog will use the built-in OpenRouter backend -instead of repeating OpenRouter connection and credential defaults in every -profile. - -## Resolution And Precedence - -Effective execution settings will resolve in this order: - -1. application-neutral framework defaults; -2. selected backend defaults, when a backend is named; -3. selected profile values; and -4. explicit per-run overrides. - -A profile endpoint will override its selected backend's endpoint. A per-run -endpoint override will continue to take precedence over both. - -Credential selection will preserve direct request credentials as the highest -precedence. An explicit per-run environment-variable override will take -precedence over profile credential configuration, which will take precedence -over the backend's default environment-variable name. Backend definitions and -profiles will contain credential-source metadata only, never resolved secret -values. - -Backend request defaults, profile request values, and per-run request -overrides will follow one deterministic replacement rule. Resolution must not -introduce implicit deep merging whose result depends on map iteration or -incidental representation. - -## Registration And Validation - -Backend registration will be deterministic and validated during engine -construction. - -- Built-in backend IDs are reserved and cannot be replaced by consumers. -- Consumer registrations may add only new IDs. -- Duplicate consumer IDs are invalid, including duplicates introduced through - repeated configuration. -- Backend IDs, endpoints, credential-source metadata, and request defaults - must be validated before an engine is returned. -- Selecting an unknown backend is an error associated with the profile or - request boundary that selected it. -- A profile without a backend must continue to provide the connection details - required by the current endpoint-based path. -- A backend may omit credential requirements so unauthenticated local-network - services remain supported. - -Failures will retain error identities appropriate to the existing engine -configuration, profile-loading, and per-run validation boundaries. - -## Public And Extension Boundaries - -The effective backend ID will be observable in prepared and completed run -metadata and will be available to injected model clients. This gives consumers -and extensions a stable routing identity without requiring them to infer a -backend from an endpoint URL. - -The existing public model-client injection boundary will remain supported. -Because the initial registry supports only OpenAI-compatible backends, -Promptkit does not need backend-specific transport factories or multiple -protocol implementations in this scope. The built-in client will continue to -send the resolved execution target to the selected OpenAI-compatible endpoint. - -Backend definitions are configuration values, not live service objects. They -will not own mutable connections, credentials, health state, or process -lifecycle. - -## Compatibility - -The target end state preserves the existing ways consumers configure and run -Promptkit: - -- Profiles with explicit endpoints and no backend ID continue to work. -- Profile endpoint overrides remain supported. -- Direct request API keys and environment-variable overrides retain their - precedence. -- Consumers may continue injecting a custom model client. -- Model IDs and generation settings remain profile concerns. -- Prompt selection, rendering, validation, artifacts, and synchronous - `Prepare` and `Run` behavior remain unchanged except for exposing the - resolved backend identity. - -## Non-Goals - -This scope does not include: - -- non-OpenAI-compatible protocols or provider-specific SDKs; -- backend-specific concurrency limits or admission queues; -- asynchronous task submission or durable jobs; -- model discovery, live model catalogs, or provider capability probing; -- retries, rate limiting, failover, load balancing, or health checks; -- automatic backend selection based on model names or endpoint inspection; -- global mutable registration; -- consumer replacement of built-in backend IDs; -- credential discovery beyond configured direct values and environment - variables; or -- provider-specific application, deployment, or security policy. - -The dependent concurrency and bounded-queue work remains a separate future -unit after backend identity and resolution are stable. - -## Target End State - -This roadmap reaches its target end state when: - -- Promptkit has an immutable, engine-scoped registry of validated - OpenAI-compatible backend definitions; -- OpenRouter is available as a built-in backend with its endpoint and - credential environment-variable default; -- consumers can register additional unique backend IDs without modifying - Promptkit; -- profiles can select a backend and optionally override its endpoint; -- existing endpoint-only profiles remain valid; -- backend, profile, and per-run values resolve through documented, - deterministic precedence; -- secret values remain outside backend and profile definitions; -- the effective backend identity is available in preparation, execution, and - injected-client values; -- the built-in model profiles use the OpenRouter backend rather than duplicate - its connection defaults; -- the built-in OpenAI-compatible client and injected clients continue to work - through the existing generation boundary; and -- current-state GoDoc, format, consumer, integration, architecture, and - internal documentation describe the implemented behavior without relying on - this roadmap. diff --git a/docs/roadmap/future.md b/docs/roadmap/future.md index 9caeac9..c32de70 100644 --- a/docs/roadmap/future.md +++ b/docs/roadmap/future.md @@ -35,9 +35,10 @@ consumers. ### Backend-specific concurrency management -Extend the selected -[LLM backend registry](backends.md) with optional per-backend concurrency limits -and bounded, buffered admission queues. Promptkit could then route simultaneous +Extend the +[LLM backend registry](../consumers/pkg-promptkit.md#register-a-custom-backend) +with optional per-backend concurrency limits and bounded, buffered admission +queues. Promptkit could then route simultaneous generation requests according to backend capacity while containing accidental runaway submission. Downstream consumers would continue invoking synchronous `Run` calls, including concurrently from multiple goroutines, and each diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md deleted file mode 100644 index 70e5e0f..0000000 --- a/docs/roadmap/implementation.md +++ /dev/null @@ -1,505 +0,0 @@ -# Extensible LLM Backend Registry Implementation Plan - -## Purpose - -This document is the decision-complete implementation plan for the -[extensible LLM backend registry](backends.md). It is written for a coding -agent that will implement each stage in order. - -This is planning material, not a description of current behavior. During -implementation, update current-state GoDoc and documentation only as the -corresponding behavior lands. The feature roadmap owns the intended end state -and policy choices; this document owns API shape, package changes, sequencing, -tests, and completion gates. - -## Implementation Rules - -- Complete the stages in order. Do not begin a later stage while an earlier - stage's completion gate is unmet. -- Preserve unrelated working-tree changes and do not broaden the feature into - concurrency control, queues, retries, provider discovery, or - non-OpenAI-compatible transports. -- Keep the registry immutable and scoped to one `Engine`. Do not add - package-global registration or mutation after construction. -- Keep secrets out of backend definitions, profiles, prepared values, results, - hashes, logs, and JSON. Backends store only an environment-variable name. -- Preserve endpoint-only profiles and custom `LLMClient` injection. -- Follow the architecture, documentation, and testing policies under - `docs/policy/`. In particular, keep the public API in the root package, - place implementation packages under `internal/`, update the component - inventory when the new package lands, and assign each behavior to one - durable test owner. -- Use deterministic, offline tests. No stage may require a real provider, - credential, or network service. - -## Fixed Design - -### Public API - -Add the following root-package declarations: - -```go -const BackendOpenRouter = "openrouter" - -type Backend struct { - ID string - Endpoint string - APIKeyEnv string - ExtraParams map[string]any -} - -func WithBackend(backend Backend) Option -``` - -`Backend` and `WithBackend` have these contracts: - -- `Backend` configures one OpenAI-compatible backend and has no stable JSON - representation. -- `ID` is trimmed, non-empty, case-sensitive, and is the stable registry key. -- `Endpoint` is trimmed and must be an absolute `http` or `https` URL with a - host. Paths are allowed; user information, query strings, and fragments are - rejected because this value is a base endpoint rather than a complete - request URL. -- `APIKeyEnv` is optional. When present, it is trimmed and must match the - portable environment-variable form `[A-Za-z_][A-Za-z0-9_]*`. -- `ExtraParams` follows the existing `Profile.ExtraParams` JSON-value rules: - non-empty string keys, finite numbers, JSON-compatible scalar and container - values, no cycles, and no collisions with `model`, `session_id`, `messages`, - `temperature`, `max_tokens`, `top_p`, `service_tier`, `reasoning_effort`, or - `response_format`. It is deeply copied during `NewEngine`. -- An empty `ExtraParams` map means that the backend supplies no request - defaults. -- Each `WithBackend` call adds one registration. Calls with different IDs - accumulate in option order; a repeated ID is an error rather than a - replacement. This is an explicit additive exception to the last-option-wins - categories documented on `Option`. -- `BackendOpenRouter` is reserved. Consumers cannot replace it. -- There is no public enumerate, lookup, remove, replace, or post-construction - registration API in this scope. - -Extend the existing public values as follows: - -| Value | Addition | Contract | -| --- | --- | --- | -| `Profile` | `BackendID string` | Optional backend selection. `Endpoint` is required only when `BackendID` is blank. | -| `OpenAICompatibleProfileConfig` | `BackendID string` | Copied to `Profile.BackendID`. | -| `ExecutionTarget` | `BackendID string \`json:"backend_id,omitempty"\`` | Effective routing identity supplied to injected clients; empty for endpoint-only profiles. | -| `PreparedRun` | `SelectedBackendID string \`json:"selected_backend_id,omitempty"\`` | Equals the effective target backend ID. | -| `RunResult` | `SelectedBackendID string \`json:"selected_backend_id,omitempty"\`` | Carries the prepared backend identity through execution. | - -Do not add a backend field to `RunRequest` or `ExecutionTargetOverride`. -Backend selection remains a profile concern. Endpoint overrides do not change -the selected backend identity. - -Trim `Profile.BackendID` and YAML `backend` values at their respective -conversion/load boundaries. Treat an all-whitespace value as absent and expose -only the trimmed value in effective targets and metadata. - -These exported struct fields are intentionally additive. Adding fields can -break downstream unkeyed composite literals even though keyed literals and -ordinary field access remain source-compatible. Accept that narrow risk; use -and document keyed literals, and do not add parallel wrapper types or a second -configuration path solely to preserve unkeyed literals. - -### Profile Format - -Add the optional strict-YAML field: - -```yaml -backend: openrouter -``` - -The profile connection rule becomes: - -- `model` remains required; -- at least one of `backend` or `endpoint` is required; -- when both are present, the profile endpoint overrides the backend endpoint; - and -- a profile with only an endpoint continues through the legacy path and has an - empty effective backend ID. - -Do not infer a backend from a model name or endpoint. Do not validate whether a -backend ID exists while decoding a profile; registry membership is -engine-scoped and is checked when the selected profile is prepared. - -### Registry And Internal Boundaries - -Add `internal/backend` as the cohesive owner of: - -- the immutable backend registry; -- backend-definition validation and defensive copying; -- the built-in OpenRouter definition; and -- the internal not-found error used when a selected ID is absent. - -Add an internal domain backend value containing `ID`, `Endpoint`, `APIKeyEnv`, -and `ExtraParams`. The registry constructor accepts consumer additions, installs -built-ins first, rejects all collisions, and returns a fully constructed -read-only registry. Lookup returns a defensive value so callers cannot mutate -registry-owned maps. - -Use these internal declarations: - -```go -var ErrBackendNotFound = errors.New("backend not found") - -func NewRegistry(additions []domain.Backend) (*Registry, error) -func (r *Registry) GetBackend(id string) (domain.Backend, error) -``` - -`internal/usecase` owns the narrow resolver interface it consumes: - -```go -type BackendResolver interface { - GetBackend(string) (domain.Backend, error) -} -``` - -The concrete registry implements this interface. Supply it explicitly to -`Runner` through both runner constructors and from the root engine assembly. -A nil resolver must never panic; if an internally constructed runner selects a -backend without a resolver, preparation fails as a profile-load failure. - -The built-in definition is exactly: - -| Field | Value | -| --- | --- | -| ID | `openrouter` | -| Endpoint | `https://openrouter.ai/api/v1` | -| API key environment variable | `OPENROUTER_API_KEY` | -| Extra parameters | none | - -Define `OpenRouterID` as a constant in `internal/backend` and define the public -`BackendOpenRouter` constant from it so there is one canonical literal. This -root-to-internal implementation import does not expose an internal type in a -public signature. - -### Resolution - -Resolve an execution target in this order: - -1. framework defaults; -2. the selected backend, if any; -3. the selected profile; and -4. the request's `ExecutionTargetOverride`. - -The layers behave as follows: - -| Setting | Backend layer | Profile layer | Request layer | -| --- | --- | --- | --- | -| Backend ID | Supplies selected ID | Selects the backend | Cannot change it | -| Endpoint | Supplies default | Non-empty value replaces backend | Non-empty value replaces profile/backend | -| Model and generation fields | Not supplied | Existing profile behavior | Existing override behavior | -| API-key environment name | Supplies default | Non-empty `api_key_env` replaces it | Non-empty `APIKeyEnv` replaces it | -| Extra parameters | Non-empty map replaces prior map | Non-empty map replaces complete backend map | Non-empty map replaces complete profile/backend map | - -Never deep-merge `ExtraParams` maps. A non-empty higher-precedence map replaces -the complete lower-precedence map. Preserve the existing distinction between a -nil or empty override map and a supplied non-empty replacement. - -Credential handling remains: - -1. a non-blank `RunRequest.APIKey` wins and suppresses environment lookup; -2. a non-blank request `APIKeyEnv` wins over profile and backend metadata; -3. file-profile `api_key_env` wins over the backend default; -4. backend `APIKeyEnv` is used when no higher layer supplies a credential - source; and -5. a profile with `APIKeyRequired: true` clears an inherited backend - environment name and requires a direct key unless the request explicitly - supplies its own `APIKeyEnv`. - -Only the environment-variable name appears in effective targets. Preserve the -current just-in-time environment lookup and missing-variable behavior. - -### Errors - -Do not add a public backend-specific error sentinel. - -| Failure | Required public identity | -| --- | --- | -| Invalid consumer backend definition | `ErrInvalidConfig` from `NewEngine` | -| Consumer ID duplicates another consumer ID | `ErrInvalidConfig` | -| Consumer ID collides with `BackendOpenRouter` | `ErrInvalidConfig` | -| Selected profile names an unknown backend | `ErrProfileLoad`, not `ErrProfileNotFound` or `ErrInvalidRequest` | -| File profile lacks both backend and endpoint | `ErrProfileLoad` | -| In-memory profile lacks both backend and endpoint | `ErrInvalidConfig` | -| Effective credential environment variable is unset | Existing `ErrAPIKeyEnvMissing` and `ErrInvalidRequest` identities | -| Other invalid per-run effective settings | Existing `ErrInvalidRequest` | - -Error text must include the offending backend ID or field where useful, but -exact prose is not a compatibility contract. Preserve wrapped internal causes -where the existing error boundary permits it. - -### Metadata And Transport - -- Set `ExecutionTarget.BackendID`, `PreparedRun.SelectedBackendID`, and - `RunResult.SelectedBackendID` from the normalized selected profile backend. -- Endpoint-only profiles leave all three values empty. -- Pass `BackendID` through `GenerateRequest.Target` and repair targets so an - injected client can route or observe it. -- The built-in OpenAI-compatible client must not serialize `backend_id` as a - provider request field. It continues to use the effective endpoint, - credential, typed generation fields, and extra parameters. -- Backend selection does not change rendered-prompt hashes, prompt hashes, - session IDs, validation, artifacts, timing, or synchronous `Prepare` and - `Run` behavior. - -## Stage 1 — Registry Foundation - -**Status:** Complete. - -### Goal - -Introduce the internal domain and immutable registry without changing the -public configuration or profile format. - -### Work - -1. Add the internal backend definition to `internal/domain`. -2. Create `internal/backend` with: - - a registry constructor that installs OpenRouter and accepts additional - internal definitions; - - validation for IDs, endpoints, environment-variable names, JSON values, - and reserved request fields; - - deterministic built-in and consumer collision handling; - - immutable lookup with defensive map copying; and - - an internal recognizable not-found error. -3. Reuse or extract one validation rule for OpenAI-compatible reserved request - fields so registry validation and `internal/llm` payload construction cannot - drift. Do not maintain duplicate reserved-key lists. -4. Keep JSON-value copying type-preserving. Do not use a marshal/unmarshal - round trip that silently changes integer or container types. -5. Update `docs/policy/architecture.md` and `docs/internal/overview.md` only - after the package exists, describing `internal/backend` as an implemented - immutable configuration registry rather than a runtime service manager. - -### Tests - -`internal/backend` owns focused package tests for: - -- the exact OpenRouter built-in values; -- successful unique consumer additions; -- built-in and consumer duplicate rejection; -- blank and whitespace-normalized IDs; -- invalid or non-HTTP(S) endpoints; -- invalid environment-variable names; -- invalid, cyclic, non-finite, empty-key, and reserved-key extra parameters; -- not-found lookup; and -- mutation isolation of input maps and returned values. - -Use one representative table per validation family rather than one test per -branch. A race-specific registry test is unnecessary if the registry is -immutable and the final repository race suite exercises concurrent reads. - -### Completion Gate - -- No public declaration or profile behavior has changed. -- The registry has no mutating method after construction. -- The built-in definition contains no credential value. -- Focused backend tests and the complete repository validation pass. - -## Stage 2 — Profile Selection And Effective Resolution - -**Status:** Complete. - -### Goal - -Make the built-in OpenRouter backend selectable by file and in-memory profiles, -migrate the built-in profiles, and expose effective backend identity. - -### Work - -1. Add `BackendID` to internal execution profiles and targets and add the YAML - key `backend`. -2. Change file and public in-memory profile validation to require `model` plus - at least one of `backend` or `endpoint`. Preserve all existing numeric and - extra-parameter validation. -3. Add `BackendID` to `Profile` and - `OpenAICompatibleProfileConfig`, and add `BackendOpenRouter`. -4. Add backend identity to the public and internal execution target, prepared - run, and run result exactly as specified under **Public API**. -5. Add the `BackendResolver` dependency immediately after the profile - repository in both runner constructors and update all internal call sites - and test helpers explicitly. -6. Resolve the selected backend after profile loading and before effective - target resolution. Map lookup failure to `ErrProfileLoad`. -7. Implement the exact endpoint, credential, and whole-map precedence rules - under **Resolution**. Keep backend ID independent from endpoint overrides. -8. Construct the built-in-only registry in `NewEngine` and inject it into the - runner. -9. Replace `endpoint` and `api_key_env` in every - `internal/profile/builtin/assets/*.yaml` profile with - `backend: openrouter`. Retain each profile's model and generation settings. -10. Update conversions and copying so backend IDs survive every public/internal - boundary and registry-owned maps remain isolated. -11. Update the GoDoc for all changed public declarations. -12. Update `docs/formats.md`, `docs/internal/sources.md`, and - `docs/internal/runner.md` with the implemented profile field, conditional - endpoint rule, built-in selection, and effective resolution behavior. - -### Tests - -Assign test ownership as follows: - -- `internal/profile` tests own strict YAML decoding and the conditional - backend-or-endpoint validation matrix. -- `internal/profile/builtin` tests own the invariant that every built-in - profile selects `openrouter` and no longer repeats its endpoint or - credential environment name. -- `internal/usecase` tests own backend/profile/request precedence, unknown-ID - error wrapping, endpoint override identity, credential precedence including - `APIKeyRequired`, and whole-map replacement. -- Root external-package contract tests own the new fields, JSON names and - omission behavior, public copying, and public error identities. -- One assembled engine test must prove a built-in profile prepares with the - expected backend ID, endpoint, and environment-variable name without making - an HTTP call. - -Do not duplicate every profile-parser validation case at the engine boundary. - -### Completion Gate - -- All built-in and endpoint-only profiles prepare successfully. -- A selected unknown backend matches public `ErrProfileLoad`. -- Backend identity reaches prepared values, results, generation targets, and - repair targets. -- No backend ID or secret is added to the outbound provider payload. -- Current-state format and internal documentation match the implemented - built-in behavior. -- The complete repository validation passes. - -## Stage 3 — Consumer Registration - -**Status:** Complete. - -### Goal - -Expose the engine-scoped extension point for additional unique -OpenAI-compatible backend IDs. - -### Work - -1. Add `Backend` and `WithBackend` exactly as specified under **Public API**, - preferably in a cohesive root `backends.go` file. -2. Convert, validate, normalize, and deeply copy public backend values during - `NewEngine`. Store pending additions in `engineOptions`; do not mutate a - registry from option application. -3. After all options have applied, construct one immutable registry from the - built-in and pending consumer definitions. Map every construction failure - to `ErrInvalidConfig`. -4. Make unique `WithBackend` calls additive. Reject duplicate IDs even when - they arrive through separate calls, and document this exception on - `Option` and `WithBackend`. -5. Make registered backends available equally to directory, `fs.FS`, single - file, in-memory, prompt-default, and explicit request profile selection. -6. Preserve the last-valid-option-wins behavior of every existing option - category. -7. Update `docs/consumers/pkg-promptkit.md` with one minimal custom backend and - profile example. Keep exact field-by-field API detail in GoDoc and link to - it rather than duplicating it. - -### Tests - -Root public contract tests own: - -- one custom backend used by an in-memory profile; -- one custom backend used by a file-backed profile; -- multiple unique `WithBackend` calls accumulating; -- duplicate consumer and built-in ID failures matching `ErrInvalidConfig`; -- invalid public values and nested-map mutation isolation; -- endpoint-only compatibility with no `WithBackend` option; -- profile endpoint and request endpoint overrides retaining the custom backend - ID; and -- an injected `LLMClient` observing the effective custom backend ID and - settings. - -Use an injected client or `httptest.Server`; never contact the configured -external endpoint. Keep lower-level registry validation cases in -`internal/backend`. - -### Completion Gate - -- Consumers can add only new IDs and cannot mutate or replace built-ins. -- Registration is engine-local and two engines can use different definitions - for the same consumer ID without interference. -- Existing option-category contract tests still pass unchanged except for the - documented additive backend case. -- The consumer guide and GoDoc describe the implemented extension point. -- The complete repository validation passes. - -## Stage 4 — Contract Hardening And Documentation Completion - -**Status:** Complete. - -### Goal - -Audit the completed feature across public, profile, use-case, and transport -boundaries and make the current-state documentation self-sufficient. - -### Work - -1. Review the preceding tests as a suite. Remove redundant cases and retain a - lean precedence matrix that would catch layer-order, credential, copying, - compatibility, and error-identity regressions. -2. Add or adjust one built-in-client transport test proving that effective - backend defaults reach the configured endpoint while `backend_id` is not - serialized. Use `httptest.Server` and synthetic credentials. -3. Confirm public stable-JSON round trips for `ExecutionTarget`, - `PreparedRun`, and `RunResult`, including empty backend omission for legacy - profiles. -4. Confirm concurrent `Prepare` and `Run` calls can read the immutable registry - under the race detector. Do not add concurrency limiting or a queue. -5. Update all affected current-state owners: - - public GoDoc for the exact API and errors; - - `docs/formats.md` for profile YAML and built-ins; - - `docs/consumers/pkg-promptkit.md` for construction and use; - - `docs/integrations/openai-compatible-chat.md` for resolved endpoint, - credentials, request defaults, and the non-serialized routing identity; - - `docs/internal/overview.md`, `docs/internal/sources.md`, - `docs/internal/runner.md`, and `docs/internal/llm.md` for package - responsibilities and data flow; and - - `docs/policy/architecture.md` only to keep its implemented component - inventory and dependency description accurate. -6. Search current-state documentation for obsolete statements that every - profile requires an endpoint or that every built-in repeats OpenRouter - connection settings. Update the canonical owner and replace duplicates with - links. -7. Keep future concurrency and queue behavior only in - [the future feature catalog](future.md); do not imply that this feature - implements either capability. - -### Final Validation - -Run the complete maintainer sequence from the repository root: - -```sh -go test ./... -go test -race ./... -go vet ./... -go build ./... -go run ./examples/go-library/prepare -gofmt -l $(git ls-files '*.go') -git diff --check -``` - -The formatting command must produce no paths. Follow every changed Markdown -link and confirm its target exists. Inspect the final diff for raw credentials, -global mutable state, generated workspace files, local `replace` directives, -and unrelated changes. - -### Completion Gate - -- Every target-end-state item in [the feature roadmap](backends.md) is - implemented and protected at its owning boundary. -- The public API is limited to the fixed declarations in this plan. -- Endpoint-only profiles and injected clients remain compatible. -- Built-in profiles obtain OpenRouter endpoint and credential metadata only - through the registry. -- Current-state documentation no longer relies on either roadmap to explain - implemented behavior. -- All final validation commands pass offline. - -## Open Questions - -None. The API shape, validation, precedence, compatibility behavior, error -identities, package ownership, test ownership, and staging required for -implementation are fixed by this plan. diff --git a/engine_test.go b/engine_test.go index 00753d0..4d5c8a8 100644 --- a/engine_test.go +++ b/engine_test.go @@ -2357,6 +2357,11 @@ func TestExtraParamsTypedNestedValuesAreCopiedAcrossPublicBoundary(t *testing.T) } func TestRunRejectsInvalidExtraParams(t *testing.T) { + cyclicMap := map[string]any{} + cyclicMap["self"] = cyclicMap + cyclicSlice := []any{nil} + cyclicSlice[0] = cyclicSlice + tests := []struct { name string extraParams map[string]any @@ -2368,42 +2373,10 @@ func TestRunRejectsInvalidExtraParams(t *testing.T) { {name: "nan", extraParams: map[string]any{"bad": math.NaN()}}, {name: "positive infinity", extraParams: map[string]any{"bad": math.Inf(1)}}, {name: "negative infinity", extraParams: map[string]any{"bad": math.Inf(-1)}}, - } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - fake := &fakeLLMClient{response: &promptkit.GenerateResponse{Content: "ok"}} - engine := newContractEngineWithOptions(t, frameworkSchemaDir, promptkit.WithLLMClient(fake)) - - _, err := engine.Run(context.Background(), promptkit.RunRequest{ - PromptID: frameworkMarkdownSummaryPromptID, - Inputs: map[string]promptkit.ArtifactRef{ - "transcript": promptkit.Inline("Rin opens the gate."), - "glossary": promptkit.Inline("gate: A guarded passage."), - }, - Execution: &promptkit.ExecutionTargetOverride{ExtraParams: tc.extraParams}, - }) - if !errors.Is(err, promptkit.ErrInvalidRequest) { - t.Fatalf("expected ErrInvalidRequest, got %v", err) - } - if len(fake.requests) != 0 { - t.Fatalf("expected invalid request to fail before LLM call, got %d requests", len(fake.requests)) - } - }) - } -} - -func TestRunRejectsCyclicExtraParams(t *testing.T) { - cyclicMap := map[string]any{} - cyclicMap["self"] = cyclicMap - cyclicSlice := []any{nil} - cyclicSlice[0] = cyclicSlice - - tests := []struct { - name string - extraParams map[string]any - }{ - {name: "map", extraParams: cyclicMap}, - {name: "slice", extraParams: map[string]any{"cycle": cyclicSlice}}, + {name: "cyclic map", extraParams: cyclicMap}, + {name: "cyclic slice", extraParams: map[string]any{"cycle": cyclicSlice}}, + {name: "malformed JSON number", extraParams: map[string]any{"value": json.Number("+1")}}, + {name: "empty nested key", extraParams: map[string]any{"nested": map[string]any{"": true}}}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { diff --git a/internal/backend/registry.go b/internal/backend/registry.go index f3a6486..da23af1 100644 --- a/internal/backend/registry.go +++ b/internal/backend/registry.go @@ -11,6 +11,8 @@ import ( "strings" "gitea.maximumdirect.net/eric/promptkit/internal/domain" + "gitea.maximumdirect.net/eric/promptkit/internal/jsonvalue" + "gitea.maximumdirect.net/eric/promptkit/internal/llm" ) const ( @@ -75,7 +77,7 @@ func (r *Registry) GetBackend(id string) (domain.Backend, error) { if !ok { return domain.Backend{}, fmt.Errorf("%w: %q", ErrBackendNotFound, id) } - extraParams, err := copyJSONMap(definition.ExtraParams) + extraParams, err := jsonvalue.CopyMap(definition.ExtraParams) if err != nil { return domain.Backend{}, fmt.Errorf("copy backend %q: %w", id, err) } @@ -83,25 +85,6 @@ func (r *Registry) GetBackend(id string) (domain.Backend, error) { return definition, nil } -// IsReservedRequestField reports whether name is owned by the standard -// OpenAI-compatible chat request rather than backend extra parameters. -func IsReservedRequestField(name string) bool { - switch name { - case "model", - "session_id", - "messages", - "temperature", - "max_tokens", - "top_p", - "service_tier", - "reasoning_effort", - "response_format": - return true - default: - return false - } -} - func normalizeBackend(definition domain.Backend) (domain.Backend, error) { definition.Endpoint = strings.TrimSpace(definition.Endpoint) if err := validateEndpoint(definition.Endpoint); err != nil { @@ -126,7 +109,7 @@ func normalizeBackend(definition domain.Backend) (domain.Backend, error) { if key == "" { return domain.Backend{}, fmt.Errorf("backend %q extra parameter key must not be empty", definition.ID) } - if IsReservedRequestField(key) { + if llm.IsReservedOpenAIChatRequestField(key) { return domain.Backend{}, fmt.Errorf( "backend %q extra parameter %q collides with a reserved request field", definition.ID, @@ -135,7 +118,7 @@ func normalizeBackend(definition domain.Backend) (domain.Backend, error) { } } - extraParams, err := copyJSONMap(definition.ExtraParams) + extraParams, err := jsonvalue.CopyMap(definition.ExtraParams) if err != nil { return domain.Backend{}, fmt.Errorf("backend %q extra parameters: %w", definition.ID, err) } diff --git a/internal/backend/registry_test.go b/internal/backend/registry_test.go index 0119c0c..0911733 100644 --- a/internal/backend/registry_test.go +++ b/internal/backend/registry_test.go @@ -1,9 +1,7 @@ package backend_test import ( - "encoding/json" "errors" - "math" "strings" "testing" @@ -188,42 +186,13 @@ func TestNewRegistryValidatesEnvironmentVariableNames(t *testing.T) { } } -func TestNewRegistryValidatesExtraParameters(t *testing.T) { - cyclic := map[string]any{} - cyclic["self"] = cyclic - +func TestNewRegistryRejectsInvalidAndReservedExtraParameters(t *testing.T) { tests := []struct { name string extraParams map[string]any }{ - {name: "empty top-level key", extraParams: map[string]any{"": true}}, - {name: "empty nested key", extraParams: map[string]any{"nested": map[string]int{"": 1}}}, - {name: "non-string map key", extraParams: map[string]any{"nested": map[int]string{1: "one"}}}, {name: "unsupported value", extraParams: map[string]any{"value": make(chan int)}}, - {name: "cyclic value", extraParams: map[string]any{"value": cyclic}}, - {name: "NaN", extraParams: map[string]any{"value": math.NaN()}}, - {name: "positive infinity", extraParams: map[string]any{"value": math.Inf(1)}}, - {name: "unsafe integer", extraParams: map[string]any{"value": int64(1 << 53)}}, - {name: "invalid JSON number", extraParams: map[string]any{"value": json.Number("not-a-number")}}, - } - for _, key := range []string{ - "model", - "session_id", - "messages", - "temperature", - "max_tokens", - "top_p", - "service_tier", - "reasoning_effort", - "response_format", - } { - tests = append(tests, struct { - name string - extraParams map[string]any - }{ - name: "reserved key " + key, - extraParams: map[string]any{key: true}, - }) + {name: "reserved key", extraParams: map[string]any{"model": "override"}}, } for _, tc := range tests { diff --git a/internal/backend/json_copy.go b/internal/jsonvalue/jsonvalue.go similarity index 73% rename from internal/backend/json_copy.go rename to internal/jsonvalue/jsonvalue.go index cd64c99..c9878c1 100644 --- a/internal/backend/json_copy.go +++ b/internal/jsonvalue/jsonvalue.go @@ -1,4 +1,6 @@ -package backend +// Package jsonvalue validates and defensively copies JSON-compatible value +// trees used by public configuration and request boundaries. +package jsonvalue import ( "encoding/json" @@ -11,16 +13,18 @@ import ( const maxSafeJSONInteger = 1<<53 - 1 -type jsonVisit struct { +type visit struct { typ reflect.Type ptr uintptr } -func copyJSONMap(src map[string]any) (map[string]any, error) { +// CopyMap validates and deeply copies an extra-parameter map while preserving +// compatible concrete map, slice, array, scalar, and number types. +func CopyMap(src map[string]any) (map[string]any, error) { if src == nil { return nil, nil } - copied, err := copyJSONValue(reflect.ValueOf(src), "extra_params", make(map[jsonVisit]struct{})) + copied, err := copyValue(reflect.ValueOf(src), "extra_params", make(map[visit]struct{})) if err != nil { return nil, err } @@ -31,7 +35,7 @@ func copyJSONMap(src map[string]any) (map[string]any, error) { return out, nil } -func copyJSONValue(value reflect.Value, path string, seen map[jsonVisit]struct{}) (any, error) { +func copyValue(value reflect.Value, path string, seen map[visit]struct{}) (any, error) { if !value.IsValid() { return nil, nil } @@ -39,12 +43,15 @@ func copyJSONValue(value reflect.Value, path string, seen map[jsonVisit]struct{} if value.IsNil() { return nil, nil } - return copyJSONValue(value.Elem(), path, seen) + return copyValue(value.Elem(), path, seen) } if !value.CanInterface() { return nil, fmt.Errorf("%s: value cannot be copied", path) } if number, ok := value.Interface().(json.Number); ok { + if _, err := json.Marshal(number); err != nil { + return nil, fmt.Errorf("%s: invalid JSON number", path) + } f, err := strconv.ParseFloat(number.String(), 64) if err != nil || math.IsNaN(f) || math.IsInf(f, 0) { return nil, fmt.Errorf("%s: invalid JSON number", path) @@ -75,28 +82,28 @@ func copyJSONValue(value reflect.Value, path string, seen map[jsonVisit]struct{} if value.IsNil() { return nil, nil } - visit := jsonVisit{typ: value.Type(), ptr: value.Pointer()} - if _, ok := seen[visit]; ok { + current := visit{typ: value.Type(), ptr: value.Pointer()} + if _, ok := seen[current]; ok { return nil, fmt.Errorf("%s: cyclic value is not supported", path) } - seen[visit] = struct{}{} - defer delete(seen, visit) - return copyJSONValue(value.Elem(), path, seen) + seen[current] = struct{}{} + defer delete(seen, current) + return copyValue(value.Elem(), path, seen) case reflect.Map: - return copyJSONMapValue(value, path, seen) + return copyMapValue(value, path, seen) case reflect.Slice: if value.IsNil() { return nil, nil } - return copyJSONSequenceValue(value, path, seen) + return copySequenceValue(value, path, seen) case reflect.Array: - return copyJSONSequenceValue(value, path, seen) + return copySequenceValue(value, path, seen) default: return nil, fmt.Errorf("%s: unsupported JSON value type %s", path, value.Type()) } } -func copyJSONMapValue(value reflect.Value, path string, seen map[jsonVisit]struct{}) (any, error) { +func copyMapValue(value reflect.Value, path string, seen map[visit]struct{}) (any, error) { if value.IsNil() { return nil, nil } @@ -104,12 +111,12 @@ func copyJSONMapValue(value reflect.Value, path string, seen map[jsonVisit]struc return nil, fmt.Errorf("%s: map key type %s is not supported", path, value.Type().Key()) } - visit := jsonVisit{typ: value.Type(), ptr: value.Pointer()} - if _, ok := seen[visit]; ok { + current := visit{typ: value.Type(), ptr: value.Pointer()} + if _, ok := seen[current]; ok { return nil, fmt.Errorf("%s: cyclic value is not supported", path) } - seen[visit] = struct{}{} - defer delete(seen, visit) + seen[current] = struct{}{} + defer delete(seen, current) keys := value.MapKeys() sort.Slice(keys, func(i, j int) bool { @@ -129,7 +136,7 @@ func copyJSONMapValue(value reflect.Value, path string, seen map[jsonVisit]struc if name == "" { return nil, fmt.Errorf("%s: map key must not be empty", path) } - copied, err := copyJSONValue(value.MapIndex(key), path+"."+name, seen) + copied, err := copyValue(value.MapIndex(key), path+"."+name, seen) if err != nil { return nil, err } @@ -164,22 +171,22 @@ func copyJSONMapValue(value reflect.Value, path string, seen map[jsonVisit]struc return out, nil } -func copyJSONSequenceValue(value reflect.Value, path string, seen map[jsonVisit]struct{}) (any, error) { - var visit jsonVisit +func copySequenceValue(value reflect.Value, path string, seen map[visit]struct{}) (any, error) { + var current visit if value.Kind() == reflect.Slice { - visit = jsonVisit{typ: value.Type(), ptr: value.Pointer()} - if _, ok := seen[visit]; ok { + current = visit{typ: value.Type(), ptr: value.Pointer()} + if _, ok := seen[current]; ok { return nil, fmt.Errorf("%s: cyclic value is not supported", path) } - seen[visit] = struct{}{} - defer delete(seen, visit) + seen[current] = struct{}{} + defer delete(seen, current) } values := make([]any, value.Len()) preserveType := true elementType := value.Type().Elem() for i := 0; i < value.Len(); i++ { - copied, err := copyJSONValue(value.Index(i), fmt.Sprintf("%s[%d]", path, i), seen) + copied, err := copyValue(value.Index(i), fmt.Sprintf("%s[%d]", path, i), seen) if err != nil { return nil, err } diff --git a/internal/jsonvalue/jsonvalue_test.go b/internal/jsonvalue/jsonvalue_test.go new file mode 100644 index 0000000..8ca084f --- /dev/null +++ b/internal/jsonvalue/jsonvalue_test.go @@ -0,0 +1,97 @@ +package jsonvalue_test + +import ( + "encoding/json" + "math" + "reflect" + "testing" + + "gitea.maximumdirect.net/eric/promptkit/internal/jsonvalue" +) + +func TestCopyMapPreservesTypesAndIsolatesMutations(t *testing.T) { + nested := map[string]int{"limit": 2} + sequence := []string{"one", "two"} + input := map[string]any{ + "count": int64(7), + "number": json.Number("-1.25e+2"), + "nested": nested, + "sequence": sequence, + } + + copied, err := jsonvalue.CopyMap(input) + if err != nil { + t.Fatalf("copy map: %v", err) + } + nested["limit"] = 99 + sequence[0] = "changed" + input["added"] = true + + if got, ok := copied["count"].(int64); !ok || got != 7 { + t.Fatalf("integer type or value changed: %#v", copied["count"]) + } + if got, ok := copied["number"].(json.Number); !ok || got != "-1.25e+2" { + t.Fatalf("JSON number type or value changed: %#v", copied["number"]) + } + if got := copied["nested"].(map[string]int)["limit"]; got != 2 { + t.Fatalf("nested map was not isolated: %d", got) + } + if got := copied["sequence"].([]string)[0]; got != "one" { + t.Fatalf("sequence was not isolated: %q", got) + } + if _, ok := copied["added"]; ok { + t.Fatalf("top-level map was not isolated: %#v", copied) + } +} + +func TestCopyMapRejectsInvalidValues(t *testing.T) { + cyclicMap := map[string]any{} + cyclicMap["self"] = cyclicMap + cyclicSlice := []any{nil} + cyclicSlice[0] = cyclicSlice + + tests := []struct { + name string + value any + }{ + {name: "empty nested key", value: map[string]int{"": 1}}, + {name: "non-string map key", value: map[int]string{1: "one"}}, + {name: "unsupported value", value: make(chan int)}, + {name: "cyclic map", value: cyclicMap}, + {name: "cyclic slice", value: cyclicSlice}, + {name: "NaN", value: math.NaN()}, + {name: "positive infinity", value: math.Inf(1)}, + {name: "unsafe signed integer", value: int64(1 << 53)}, + {name: "unsafe unsigned integer", value: uint64(1 << 53)}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if _, err := jsonvalue.CopyMap(map[string]any{"value": tc.value}); err == nil { + t.Fatal("expected validation error") + } + }) + } +} + +func TestCopyMapValidatesJSONNumberSyntaxAndRange(t *testing.T) { + for _, number := range []json.Number{"0", "-1", "1.25", "-1.25e+2"} { + t.Run("valid "+number.String(), func(t *testing.T) { + got, err := jsonvalue.CopyMap(map[string]any{"value": number}) + if err != nil { + t.Fatalf("copy valid JSON number: %v", err) + } + if !reflect.DeepEqual(got["value"], number) { + t.Fatalf("JSON number changed: got %#v want %#v", got["value"], number) + } + }) + } + + for _, number := range []json.Number{"", "01", "+1", "1.", ".1", "1e9999", "not-a-number"} { + t.Run("invalid "+number.String(), func(t *testing.T) { + if _, err := jsonvalue.CopyMap(map[string]any{"value": number}); err == nil { + t.Fatal("expected invalid JSON number error") + } + }) + } +} diff --git a/internal/llm/openai_compatible_client.go b/internal/llm/openai_compatible_client.go index 8f9fa61..d98ddd5 100644 --- a/internal/llm/openai_compatible_client.go +++ b/internal/llm/openai_compatible_client.go @@ -14,7 +14,6 @@ import ( "time" "unicode/utf8" - "gitea.maximumdirect.net/eric/promptkit/internal/backend" "gitea.maximumdirect.net/eric/promptkit/internal/defaults" "gitea.maximumdirect.net/eric/promptkit/internal/domain" ) @@ -263,7 +262,7 @@ func openAIChatRequestPayload(req openAIChatRequest) (map[string]any, error) { if key == "" { return nil, errors.New("extra_params key must not be empty") } - if backend.IsReservedRequestField(key) { + if IsReservedOpenAIChatRequestField(key) { return nil, fmt.Errorf("extra_params key %q collides with reserved request field", key) } if _, err := json.Marshal(value); err != nil { @@ -275,6 +274,25 @@ func openAIChatRequestPayload(req openAIChatRequest) (map[string]any, error) { return out, nil } +// IsReservedOpenAIChatRequestField reports whether name is owned by the +// standard OpenAI-compatible chat request rather than extra parameters. +func IsReservedOpenAIChatRequestField(name string) bool { + switch name { + case "model", + "session_id", + "messages", + "temperature", + "max_tokens", + "top_p", + "service_tier", + "reasoning_effort", + "response_format": + return true + default: + return false + } +} + type openAIChatRequestMessage struct { Role string `json:"role"` Content any `json:"content"` diff --git a/internal/usecase/runner.go b/internal/usecase/runner.go index 89f559f..76e476a 100644 --- a/internal/usecase/runner.go +++ b/internal/usecase/runner.go @@ -365,7 +365,7 @@ func mergeExecutionTarget(base domain.ExecutionTarget, override domain.Execution if strings.TrimSpace(override.BackendID) != "" { out.BackendID = override.BackendID } - if override.Endpoint != "" { + if strings.TrimSpace(override.Endpoint) != "" { out.Endpoint = override.Endpoint } if override.Model != "" { diff --git a/json_copy.go b/json_copy.go deleted file mode 100644 index 35e9b83..0000000 --- a/json_copy.go +++ /dev/null @@ -1,218 +0,0 @@ -package promptkit - -import ( - "encoding/json" - "fmt" - "math" - "reflect" - "strconv" -) - -const maxSafeJSONInteger = 1<<53 - 1 - -type jsonVisit struct { - typ reflect.Type - ptr uintptr -} - -func copyPublicJSONMap(src map[string]any) (map[string]any, error) { - if src == nil { - return nil, nil - } - copied, err := copyPublicJSONValue(reflect.ValueOf(src), "extra_params", make(map[jsonVisit]struct{})) - if err != nil { - return nil, err - } - out, ok := copied.(map[string]any) - if !ok { - return nil, fmt.Errorf("extra_params: expected object") - } - return out, nil -} - -func copyPublicJSONValue(value reflect.Value, path string, seen map[jsonVisit]struct{}) (any, error) { - if !value.IsValid() { - return nil, nil - } - if value.Kind() == reflect.Interface { - if value.IsNil() { - return nil, nil - } - return copyPublicJSONValue(value.Elem(), path, seen) - } - if !value.CanInterface() { - return nil, fmt.Errorf("%s: value cannot be copied", path) - } - if number, ok := value.Interface().(json.Number); ok { - f, err := strconv.ParseFloat(number.String(), 64) - if err != nil || math.IsNaN(f) || math.IsInf(f, 0) { - return nil, fmt.Errorf("%s: invalid JSON number", path) - } - return number, nil - } - - switch value.Kind() { - case reflect.Bool, reflect.String: - return value.Interface(), nil - case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: - if value.Int() < -maxSafeJSONInteger || value.Int() > maxSafeJSONInteger { - return nil, fmt.Errorf("%s: integer is outside the JSON-safe range", path) - } - return value.Interface(), nil - case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr: - if value.Uint() > maxSafeJSONInteger { - return nil, fmt.Errorf("%s: integer is outside the JSON-safe range", path) - } - return value.Interface(), nil - case reflect.Float32, reflect.Float64: - f := value.Convert(reflect.TypeOf(float64(0))).Float() - if math.IsNaN(f) || math.IsInf(f, 0) { - return nil, fmt.Errorf("%s: floating-point value must be finite", path) - } - return value.Interface(), nil - case reflect.Pointer: - if value.IsNil() { - return nil, nil - } - visit := jsonVisit{typ: value.Type(), ptr: value.Pointer()} - if _, ok := seen[visit]; ok { - return nil, fmt.Errorf("%s: cyclic value is not supported", path) - } - seen[visit] = struct{}{} - defer delete(seen, visit) - return copyPublicJSONValue(value.Elem(), path, seen) - case reflect.Map: - return copyPublicJSONMapValue(value, path, seen) - case reflect.Slice: - if value.IsNil() { - return nil, nil - } - return copyPublicJSONSequenceValue(value, path, seen) - case reflect.Array: - return copyPublicJSONSequenceValue(value, path, seen) - default: - return nil, fmt.Errorf("%s: unsupported JSON value type %s", path, value.Type()) - } -} - -func copyPublicJSONMapValue(value reflect.Value, path string, seen map[jsonVisit]struct{}) (any, error) { - if value.IsNil() { - return nil, nil - } - if value.Type().Key().Kind() != reflect.String { - return nil, fmt.Errorf("%s: map key type %s is not supported", path, value.Type().Key()) - } - - visit := jsonVisit{typ: value.Type(), ptr: value.Pointer()} - if _, ok := seen[visit]; ok { - return nil, fmt.Errorf("%s: cyclic value is not supported", path) - } - seen[visit] = struct{}{} - defer delete(seen, visit) - - type entry struct { - key reflect.Value - name string - value any - } - entries := make([]entry, 0, value.Len()) - preserveType := true - elemType := value.Type().Elem() - iter := value.MapRange() - for iter.Next() { - key := iter.Key() - name := key.String() - copied, err := copyPublicJSONValue(iter.Value(), path+"."+name, seen) - if err != nil { - return nil, err - } - entries = append(entries, entry{key: key, name: name, value: copied}) - if copied == nil { - if !canAssignNil(elemType) { - preserveType = false - } - continue - } - if !reflect.TypeOf(copied).AssignableTo(elemType) { - preserveType = false - } - } - - if preserveType { - out := reflect.MakeMapWithSize(value.Type(), len(entries)) - for _, entry := range entries { - if entry.value == nil { - out.SetMapIndex(entry.key, reflect.Zero(elemType)) - continue - } - out.SetMapIndex(entry.key, reflect.ValueOf(entry.value)) - } - return out.Interface(), nil - } - - out := make(map[string]any, len(entries)) - for _, entry := range entries { - out[entry.name] = entry.value - } - return out, nil -} - -func copyPublicJSONSequenceValue(value reflect.Value, path string, seen map[jsonVisit]struct{}) (any, error) { - var visit jsonVisit - if value.Kind() == reflect.Slice { - visit = jsonVisit{typ: value.Type(), ptr: value.Pointer()} - if _, ok := seen[visit]; ok { - return nil, fmt.Errorf("%s: cyclic value is not supported", path) - } - seen[visit] = struct{}{} - defer delete(seen, visit) - } - - values := make([]any, value.Len()) - preserveType := true - elemType := value.Type().Elem() - for i := 0; i < value.Len(); i++ { - copied, err := copyPublicJSONValue(value.Index(i), fmt.Sprintf("%s[%d]", path, i), seen) - if err != nil { - return nil, err - } - values[i] = copied - if copied == nil { - if !canAssignNil(elemType) { - preserveType = false - } - continue - } - if !reflect.TypeOf(copied).AssignableTo(elemType) { - preserveType = false - } - } - - if preserveType { - out := reflect.New(value.Type()).Elem() - if value.Kind() == reflect.Slice { - out = reflect.MakeSlice(value.Type(), value.Len(), value.Len()) - } - for i, copied := range values { - if copied == nil { - out.Index(i).Set(reflect.Zero(elemType)) - continue - } - out.Index(i).Set(reflect.ValueOf(copied)) - } - return out.Interface(), nil - } - - out := make([]any, len(values)) - copy(out, values) - return out, nil -} - -func canAssignNil(typ reflect.Type) bool { - switch typ.Kind() { - case reflect.Chan, reflect.Func, reflect.Interface, reflect.Map, reflect.Pointer, reflect.Slice: - return true - default: - return false - } -} diff --git a/profiles.go b/profiles.go index 13cda8a..ffc9bf5 100644 --- a/profiles.go +++ b/profiles.go @@ -7,6 +7,7 @@ import ( "strings" "gitea.maximumdirect.net/eric/promptkit/internal/domain" + "gitea.maximumdirect.net/eric/promptkit/internal/jsonvalue" "gitea.maximumdirect.net/eric/promptkit/internal/profile" ) @@ -81,7 +82,7 @@ func (r *memoryProfileRepository) GetProfile(_ context.Context, id string) (*dom } func toDomainProfile(publicProfile Profile) (domain.ExecutionProfile, error) { - extraParams, err := copyPublicJSONMap(publicProfile.ExtraParams) + extraParams, err := jsonvalue.CopyMap(publicProfile.ExtraParams) if err != nil { return domain.ExecutionProfile{}, err } diff --git a/public_contract_test.go b/public_contract_test.go index 159b969..260aaa7 100644 --- a/public_contract_test.go +++ b/public_contract_test.go @@ -154,6 +154,7 @@ func TestCustomBackendFlowsThroughProfilesOverridesAndInjectedClient(t *testing. promptkit.WithProfiles( promptkit.Profile{ID: "backend-profile", BackendID: "custom", Model: "backend-model"}, promptkit.Profile{ID: "profile-endpoint", BackendID: "custom", Endpoint: "http://profile.example/v1", Model: "profile-model"}, + promptkit.Profile{ID: "blank-profile-endpoint", BackendID: "custom", Endpoint: " \t ", Model: "profile-model"}, ), promptkit.WithLLMClient(client), ) @@ -188,6 +189,16 @@ func TestCustomBackendFlowsThroughProfilesOverridesAndInjectedClient(t *testing. t.Fatalf("profile endpoint override changed backend identity: %+v", prepared) } + prepared, err = engine.Prepare(context.Background(), promptkit.RunRequest{ + PromptID: "prompt", ProfileID: "blank-profile-endpoint", + }) + if err != nil { + t.Fatalf("prepare blank profile endpoint: %v", err) + } + if prepared.SelectedBackendID != "custom" || prepared.EffectiveModelParams.Endpoint != "http://backend.example/v1" { + t.Fatalf("blank profile endpoint did not inherit backend endpoint: %+v", prepared) + } + prepared, err = engine.Prepare(context.Background(), promptkit.RunRequest{ PromptID: "prompt", Execution: &promptkit.ExecutionTargetOverride{ @@ -208,6 +219,7 @@ func TestCustomBackendSupportsFileProfileAndBothSelectionPaths(t *testing.T) { promptkit.WithProfileFS(fstest.MapFS{ "profile.yaml": &fstest.MapFile{Data: []byte(`id: file-profile backend: file-backend +endpoint: " " model: file-model `)}, }, "."), @@ -303,6 +315,7 @@ func TestBackendRegistrationRejectsInvalidAndDuplicateDefinitions(t *testing.T) {name: "invalid environment", backends: []promptkit.Backend{{ID: "custom", Endpoint: "http://example.test/v1", APIKeyEnv: "BAD-NAME"}}}, {name: "reserved extra parameter", backends: []promptkit.Backend{{ID: "custom", Endpoint: "http://example.test/v1", ExtraParams: map[string]any{"model": "override"}}}}, {name: "cyclic extra parameter", backends: []promptkit.Backend{{ID: "custom", Endpoint: "http://example.test/v1", ExtraParams: cycle}}}, + {name: "malformed JSON number", backends: []promptkit.Backend{{ID: "custom", Endpoint: "http://example.test/v1", ExtraParams: map[string]any{"value": json.Number("01")}}}}, {name: "duplicate consumer id", backends: []promptkit.Backend{ {ID: " custom ", Endpoint: "http://one.example/v1"}, {ID: "custom", Endpoint: "http://two.example/v1"},