diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index bb8ec42..1e7b289 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -1,292 +1,428 @@ -# Optional API-Key Environment Implementation Plan +# Profile Inheritance Implementation Plan ## Purpose -Change Promptkit's credential handling so an effective `APIKeyEnv` names an -optional credential source rather than implicitly requiring that environment -variable to be populated. When neither a direct key nor a populated optional -environment variable is available, the built-in OpenAI-compatible client must -omit the `Authorization` header and let the upstream provider accept or reject -the unauthenticated request. - -This document owns implementation sequencing for that change. Follow the -architecture, documentation, and testing policies under [`docs/policy/`](../policy/) -and the task-specific reading guide in [`docs/development.md`](../development.md) -throughout the work. +Implement the profile-inheritance feature defined by the +[profile inheritance roadmap](profile-inheritance.md). This plan is the +decision-complete execution sequence for a coding agent. Follow the repository +[architecture](../policy/architecture.md), [testing](../policy/testing.md), and +[documentation](../policy/documentation.md) policies and the task-specific +reading guide in [`docs/development.md`](../development.md) throughout the +work. ## Target Outcome -For ordinary and prepared execution: +A downstream consumer can define a YAML or in-memory profile whose +`base_profile`/`BaseProfileID` names another profile, including a Promptkit +built-in. Promptkit resolves the complete one-parent chain through the assembled +profile catalog, merges root-to-leaf profile settings, resolves the final +backend, and then applies request overrides exactly as it does for a standalone +profile. -1. a nonblank direct `RunRequest.APIKey` remains the highest-precedence - credential and produces `Authorization: Bearer `; -2. otherwise, a nonblank effective `APIKeyEnv` is read when generation begins; -3. a nonblank trimmed environment value produces the bearer header; -4. an absent, empty, or whitespace-only optional environment value produces no - `Authorization` header and does not prevent preparation or execution; and -5. the provider response follows the normal success or error path, including - public `GenerationError` conversion for a non-2xx response. - -An explicitly required credential retains local validation. `APIKeyRequired` -continues to require either a nonblank direct key or a populated environment -variable explicitly selected by the request. Missing required credentials -fail before provider transport. +The selected leaf ID remains the public profile identity. Ordinary operations +resolve sources afresh, while prepared execution freezes the resolved target. +Missing dependencies, cycles, excessive depth, and incomplete resolved profiles +fail as profile-load errors without changing the distinct not-found contract for +an absent directly selected profile. ## Fixed Decisions -- `APIKeyEnv` is a lookup location, not an assertion that authentication is - required. This applies whether the name comes from a built-in backend, a - consumer backend, a file profile, or a request override. -- An environment value is usable only after `strings.TrimSpace`; unset, empty, - and whitespace-only values are equivalent and cause header omission when - optional. -- Direct keys retain precedence. When a direct key is nonblank, Promptkit does - not need the environment value and must not let a missing environment value - block the request. -- `APIKeyRequired` remains the only existing explicit local requirement flag. - Do not add a backend field, YAML field, request field, or new public API. -- With `APIKeyRequired` true: - - a nonblank direct key satisfies the requirement; - - an explicitly selected nonblank `APIKeyEnv` satisfies it only when that - variable currently contains a nonblank value; - - no selected environment name retains the existing required-key failure; - and - - a selected but unset environment retains `ErrAPIKeyEnvMissing` and - `ErrInvalidRequest` at the public boundary. -- `ErrAPIKeyEnvMissing` remains exported for compatibility but is narrowed to - a missing environment credential in an explicitly required flow. Optional - missing environment values do not return it. -- Optional credential availability is not frozen during preparation. The - built-in client reads the selected environment variable for each generation, - including repair generation. Prepared execution therefore uses the value - visible when it runs. -- Required prepared execution continues to recheck credential availability - before admission. Optional prepared execution proceeds even if the variable - becomes unset after preparation. -- The built-in transport sets no `Authorization` header at all when no usable - credential exists. It must not send `Bearer ` with an empty value. -- Injected `LLMClient` behavior is not redefined. Promptkit stops rejecting an - optional missing environment before the injected client is called and - continues to pass the effective target through the public adapter without - resolving or exposing a secret on the client's behalf. -- Upstream authentication policy remains upstream. Promptkit adds no provider- - specific authentication rules, retry behavior, status mapping, or special - handling beyond the existing structured non-2xx error path. +- YAML uses `base_profile`; the public Go fields are + `Profile.BaseProfileID` and + `OpenAICompatibleProfileConfig.BaseProfileID`. +- One profile may name one base. Bases may form a linear chain of at most 32 + profiles, including the selected leaf. +- The resolver performs every lookup through the complete raw composite + repository. Each base therefore observes the ordinary in-memory, configured, + fallback, and built-in precedence. +- The resolving repository is stateless and does not cache results across + calls. It is applied once, outside the fully assembled raw overlay chain. +- Profile definitions with a base may omit `model` and both connection fields. + Standalone definitions retain their current completeness requirements. +- Root-to-leaf merge behavior follows existing profile presence semantics: + nonblank strings and nonzero numbers override; blank strings and zero numbers + inherit; a nonempty `extra_params` map replaces the complete inherited map; + and `APIKeyRequired` is a sticky logical OR. +- `id` always comes from the selected leaf. `base_profile` is cleared from the + resolved profile before it reaches the use-case layer. +- Backend and endpoint remain independent fields. A child backend replaces the + inherited backend ID without implicitly clearing an inherited endpoint; a + child endpoint replaces only the endpoint. +- This work introduces no profile-level clearing syntax, presence pointers, + multiple inheritance, source-qualified references, public resolution graph, + new public error sentinel, or provider-wire change. +- A missing directly selected profile continues to expose + `ErrProfileNotFound`. Once the leaf exists, a missing base is an invalid + selected profile and must expose `ErrProfileLoad` without also matching + `ErrProfileNotFound`. +- Existing raw-source failures and context cancellation remain discoverable + through `errors.Is` where they are currently preserved. The missing-base + branch is the one deliberate exception: its diagnostic names the missing ID + and chain but does not wrap the internal not-found sentinel. +- No release notes, version tags, commits, or pushes are part of these stages. ## Execution Rules -- Complete the stages in numerical order. Each stage is scoped for one coding- - agent prompt and must finish its focused tests before the next stage begins. -- Treat all stages as one behavior change. Intermediate stages intentionally - leave the use-case and transport policies temporarily different; do not tag, - release, or claim the change is complete until Stage 3 passes. -- At the start of each stage, inspect the working tree and preserve all - unrelated changes. In particular, merge rather than overwrite any existing - edits in files touched by this plan. -- Use controlled transports, local servers, and `t.Setenv`; never contact a - live provider or depend on the developer machine's credentials. -- Update existing tests whose asserted policy is changing instead of retaining - contradictory tests or adding duplicate coverage under new names. -- Do not add dependencies, commit, tag, push, or edit release documentation - unless separately instructed. -- Current-state prose documentation changes only in Stage 3, after the runtime - behavior exists. GoDoc changes alongside the public contract in that stage. +- Complete the stages in numerical order. Each stage is sized for one + gpt-5.6-terra implementation prompt and must finish its focused verification + before the next begins. +- At the start of every stage, inspect the working tree and preserve unrelated + changes. The feature roadmap is intentional planning state and must remain. +- Treat intermediate stages as an unreleased partial implementation. Do not + describe the feature in durable current-state prose until Stage 4. +- Keep inheritance logic in `internal/profile`. Do not add recursive profile + lookup, chain merging, or source-precedence logic to `internal/usecase` or the + root public facade. +- Use real in-memory repositories and synthetic `fs.FS` sources where practical. + Tests must remain deterministic, offline, parallel-safe, and independent of + provider credentials. +- Update existing test owners when their validation contract changes; do not + retain contradictory expectations or duplicate the full inheritance matrix + at higher layers. +- Use `apply_patch` for edits, format every changed Go file, and review the + complete stage diff before proceeding. -## Stage 1: Make Use-Case Credential Validation Requirement-Aware +## Stage 1: Add Profile Definition Surfaces And Separate Local Validation ### Objective -Stop preparation and execution orchestration from treating every named -environment variable as required, while preserving the explicit -`APIKeyRequired` and prepared-execution contracts. +Represent a base-profile reference in every supported profile input while +preserving strict decoding, standalone-profile compatibility, and eager local +validation. At the end of this stage repositories can publish locally valid +derived definitions, but the engine does not yet resolve them. ### Implementation -1. Update `validateAPIKey` in `internal/usecase/runner.go` with this policy: - - return success immediately for a nonblank direct key; - - return success when `apiKeyRequired` is false, regardless of whether - `apiKeyEnv` is blank, populated, or missing; - - when `apiKeyRequired` is true and the trimmed environment name is blank, - return the existing `ErrAPIKeyRequired`; - - when `apiKeyRequired` is true and the selected environment variable is - unset, empty, or whitespace-only, return the existing wrapped - `ErrAPIKeyEnvMissing` diagnostic naming the variable; and - - otherwise return success. -2. Keep the calls from preparation and `RunPrepared` in place. Do not move - environment lookup into backend/profile resolution, admission, the domain - model, or the root facade. -3. Do not clear `APIKeyEnv` from effective or prepared metadata merely because - its current value is absent. The name remains the configured lookup source; - the secret value remains excluded. -4. Preserve credential-source precedence and `APIKeyRequired` merge behavior. - A request environment override remains capable of satisfying an in-memory - profile's explicit requirement when its value is populated. +1. Add `BaseProfileID string` to `internal/domain.ExecutionProfile` with the + YAML tag `base_profile`. Keep it out of execution targets and provider + request types. +2. Add `BaseProfileID string` to the public `Profile` immediately after `ID`. + Its GoDoc must state that: + - `WithProfiles` trims the value; + - a nonblank value permits required target fields to be inherited; + - reference existence and resolved completeness are checked only when the + profile is selected or inspected; and + - blank means the profile remains standalone. +3. Add the matching field to `OpenAICompatibleProfileConfig`, document that it + maps to `Profile.BaseProfileID`, and copy it in + `OpenAICompatibleProfile`. Preserve the constructor's shallow-copy and + deferred deep-validation behavior for `ExtraParams`. +4. Map and trim `Profile.BaseProfileID` in `toDomainProfile`. Ensure the memory + repository continues to return caller-independent `ExtraParams` and does not + expose shared mutable profile state. +5. Centralize the duplicated file-profile and in-memory-profile normalization + rules in `internal/profile` rather than adding a third validation path: + - provide an internal-package exported helper named + `NormalizeAndValidateDefinition` so the root facade can validate a + converted in-memory `domain.ExecutionProfile` without owning profile + parsing rules; + - trim `ID`, `BaseProfileID`, `BackendID`, and `Endpoint` in that helper; + - require a nonblank `ID` and validate a supplied endpoint and all execution + setting bounds for every definition; + - when `BaseProfileID` is blank, retain the existing requirements for a + nonblank model and at least one nonblank backend or endpoint; + - when `BaseProfileID` is nonblank, allow model, backend, and endpoint to be + omitted so they can be inherited; and + - do not add unrelated normalization, environment-name policy, clearing + behavior, or backend registry lookup. +6. Replace `normalizeAndValidateProfile` in + `internal/profile/filesystem_repository.go` and + `normalizeAndValidatePublicProfile` in `profiles.go` with the shared helper. + Preserve each caller's current public/internal error wrapping, selected-file + path diagnostic, raw-key prohibition, strict single-document rule, and + duplicate-ID behavior. Remove imports and helpers made obsolete by the + centralization. +7. Keep the raw repositories raw: file, `fs.FS`, memory, overlay, and built-in + repositories return `BaseProfileID` but do not recursively load it in this + stage. ### Tests -Update the narrow `internal/usecase` behavior owners rather than adding tests -for the private helper itself: +Update the narrow existing owners: -1. Replace `TestRunnerRunAPIKeyEnvMissingEnvironmentValueFailsClearly` with a - test proving an unset optional profile `APIKeyEnv` reaches the injected - model client and completes successfully. Explicitly set the named variable - to an empty value so the test is independent of the process environment. -2. Preserve the existing populated-environment and direct-key-precedence tests. -3. Preserve `TestRunnerPrepareAPIKeyRequiredFailsWithoutDirectKey` and the - existing direct-key success coverage. -4. Revise the prepared credential test matrix to prove both distinct cases: - - an optional environment value that disappears after - `PrepareExecution` does not prevent admission and generation; and - - an `APIKeyRequired` profile using an explicit request `APIKeyEnv` override - still fails with `ErrInvalidRequest` and `ErrAPIKeyEnvMissing` if that - value disappears before `RunPrepared`, before admission or generation. -5. Keep prepared handles single-use in both outcomes and avoid duplicating - unrelated preparation, validation, or admission assertions. +1. In `internal/profile/repository_test.go`, extend the filesystem/`fs.FS` + source-parity coverage to prove that: + - strict YAML accepts `base_profile`, trims it, and publishes it; + - an alias containing only `id` and `base_profile` is locally valid; + - a derived profile still rejects an invalid supplied endpoint, negative or + out-of-range setting, malformed `extra_params`, raw `api_key`, duplicate + ID, and extra YAML document through their existing identities; and + - a standalone profile missing connection fields or model remains invalid. + Modify existing missing-field expectations rather than duplicating their + entire matrices. +2. In root package tests, extend the existing `WithProfiles` validation owner + to show that an incomplete profile with a nonblank `BaseProfileID` is + accepted by `NewEngine`, while the equivalent standalone profile retains its + current `ErrInvalidConfig` result. Do not attempt to select the derived + profile until the resolver is implemented. +3. Extend `TestOpenAICompatibleProfileMapsEveryField` so + `BaseProfileID` is included in the constructor's exact field mapping. +4. Preserve existing tests for defensive copying, duplicates, standalone + profiles, overlay precedence, built-ins, and strict decoding. ### Verification ```sh -go test ./internal/usecase -run 'APIKey|Credential|Prepared' +gofmt -w internal/domain/domain.go internal/profile/*.go profiles.go types.go +go test ./internal/profile +go test ./internal/profile/builtin +go test ./... -run 'Profile|OpenAICompatibleProfile' +go test ./... +git diff --check +``` + +Stage 1 is complete when all supported definition surfaces carry the normalized +base ID, locally valid aliases can be stored, standalone validation is +unchanged, and no repository performs inheritance resolution yet. + +**Status:** Complete. + +## Stage 2: Implement The Stateless Resolving Repository + +### Objective + +Add the internal component that resolves one complete profile chain, merges it +deterministically, enforces safety bounds, and returns one ordinary resolved +profile to its caller. + +### Implementation + +1. Add `internal/profile/resolving_repository.go` with: + - an unexported `resolvingRepository` containing one underlying raw + `Repository`; + - `NewResolvingRepository(source Repository) Repository`; and + - an unexported constant setting the maximum chain length to 32, counting + the selected leaf. +2. Make the wrapper safe for concurrent calls by keeping the source immutable + and all chain, visited-ID, and merge state local to `GetProfile`. Do not add a + cache, mutex, process-global registry, or source snapshot. +3. Resolve one request using this exact flow: + - reject a nil underlying repository or blank requested ID as + `ErrInvalidProfile`; + - trim and look up the directly requested ID through the underlying raw + repository; + - if that first lookup fails, return its error unchanged so direct + `ErrProfileNotFound` behavior remains intact; + - reject a nil profile returned without an error as `ErrInvalidProfile`; + - follow each normalized nonblank `BaseProfileID` through the same underlying + repository, recording IDs and profiles from leaf toward root; + - check `ctx.Err()` between lookups; + - detect a repeated ID before loading it again and return an + `ErrInvalidProfile` cycle diagnostic containing the `a -> b -> a` chain; + - reject a reference that would make the chain exceed 32 profiles with an + `ErrInvalidProfile` diagnostic containing the traversed chain and next ID; + - when a base lookup returns `ErrProfileNotFound`, return a new + `ErrInvalidProfile` error naming the missing base and chain without + wrapping `ErrProfileNotFound`; and + - for any other base error, add `ErrInvalidProfile`, base ID, and chain + context while preserving the source error and cancellation identity with + wrapping. +4. Merge the collected definitions from root to leaf into a new value. Use one + focused helper with these rules: + - retain the selected leaf `ID` and clear final `BaseProfileID`; + - replace `BackendID`, `Endpoint`, `Model`, `ServiceTier`, + `ReasoningEffort`, and `APIKeyEnv` only for a nonblank child value; + - replace `Temperature`, `MaxTokens`, `TopP`, and `TimeoutSeconds` only for a + nonzero child value; + - set `APIKeyRequired` to the logical OR of every definition in the chain; + - replace `ExtraParams` only when the child map is nonempty; otherwise + inherit the current map; and + - defensively deep-copy every retained or replacement `ExtraParams` map by + using the existing bounded `internal/jsonvalue` owner. Never mutate a + repository-returned profile or map. Treat an unexpected copy failure as + `ErrInvalidProfile`, preserve the copy error, and include the selected + chain in the diagnostic. +5. Add an unexported final validator in `internal/profile` that requires a + nonblank model and at least one backend or endpoint on the merged profile, + and reuses the common endpoint and execution-setting validation. Wrap final + failures with `ErrInvalidProfile` and the selected chain. The resolver must + not perform backend registry membership lookup; that remains in + `internal/usecase` after profile resolution. +6. Return a caller-owned resolved profile on every successful call. A + standalone profile passes through the same final validation and copy path, + ensuring the wrapper does not create separate standalone and inherited + execution contracts. + +### Tests + +Create `internal/profile/resolving_repository_test.go` and use small synthetic +repositories plus the real overlay repository where it materially protects +composition: + +1. Prove a three-level root/middle/leaf chain merges representative values for + every rule: inherited values, string and numeric replacements, independent + backend/endpoint behavior, sticky `APIKeyRequired`, complete-map + `ExtraParams` replacement, leaf identity, and cleared resolution metadata. + Use one well-structured table or fixture rather than one test per field. +2. Prove an empty child `ExtraParams` inherits the base map, a nonempty child + map replaces rather than key-merges it, and mutating the returned nested + values cannot mutate the raw repository or a later result. +3. Prove the same composite repository resolves a base from a lower-precedence + source and honors a higher-precedence shadow of that base ID. Do not add + source-specific resolution to the wrapper. +4. Cover the safety/error matrix through the resolving repository boundary: + - directly selected missing ID preserves `ErrProfileNotFound`; + - missing base matches `ErrInvalidProfile` and does not match + `ErrProfileNotFound`; + - direct and indirect cycles are rejected with useful chains; + - exactly 32 profiles are accepted and a 33rd is rejected; + - nil repository results and incomplete final profiles are invalid; + - non-not-found base errors retain their original identity; and + - cancellation before or during traversal remains discoverable and stops + later lookups. +5. Prove freshness and concurrency without timing sleeps: change a controlled + raw repository between sequential calls and observe the new base value, then + issue concurrent independent resolutions and verify results do not share + mutable maps. Use synchronization owned by the fake; do not race on a plain + map. +6. Preserve the existing raw repository and overlay tests unchanged except + where Stage 1 intentionally changed local completeness rules. + +### Verification + +```sh +gofmt -w internal/profile/*.go +go test ./internal/profile -run 'Resolving|Inheritance|Profile' +go test -race ./internal/profile +go test ./... +git diff --check +``` + +Stage 2 is complete when a standalone internal wrapper can safely and freshly +resolve every accepted chain into one complete, copied profile with the fixed +merge and error contracts. + +## Stage 3: Assemble Inheritance And Prove Public Workflows + +### Objective + +Place the resolver at the correct engine boundary and prove that YAML, +in-memory, built-in, inspection, ordinary preparation, runtime overrides, and +prepared execution compose as intended through the public facade. + +### Implementation + +1. Update `newProfileRepository` in `engine.go` to: + - assemble the raw repository in its current precedence order—built-ins, + fallback, configured source or `Config.ProfileDir`, then in-memory; + - wrap that complete raw repository exactly once with + `profile.NewResolvingRepository`; and + - return the wrapper to `usecase.NewRunner`. + Do not wrap individual sources, because that would prevent a higher-level + child from referencing a lower-level base and would change shadowing rules. +2. Keep `internal/usecase.resolveProfileSelection` structurally unchanged. It + should continue to request one profile, preserve the requested normalized ID + as selection identity, resolve the final backend once, and then apply the + ordinary backend/profile/request precedence. Do not pass `BaseProfileID` + into `ExecutionTarget`, `PreparedRun`, `RunResult`, injected `LLMClient` + requests, stable JSON, hashes, or provider payloads. +3. Confirm `APIKeyRequired` behavior after inheritance without introducing a + special credential path: a true value in any ancestor reaches the existing + profile-to-target merge, clears inherited profile/backend `APIKeyEnv`, and + still permits a direct request key or explicit request environment override. +4. Preserve the current error mapper. The resolver's use of + `ErrInvalidProfile` must make a missing/invalid base cross the use-case and + root boundaries as `ErrProfileLoad`; no new root sentinel or special-case + string matching is permitted. +5. Do not modify built-in profile assets merely to demonstrate inheritance. + Built-ins serve as valid bases without changing their own definitions. + +### Tests + +Use the smallest public tests that protect assembled behavior without repeating +the resolver's matrix: + +1. Add one external-package contract workflow that defines `weather-light` as + an in-memory alias/refinement of the built-in `deepseek-4-flash`. Compare + its inspected target with an inspection of the base rather than duplicating + every built-in literal. Assert: + - the child inspection reports `weather-light`; + - backend and model are inherited; + - explicit child reasoning and timeout values win; + - `Prepare` reports the child as `SelectedProfileID`; and + - a per-request reasoning or timeout override still wins over the child. +2. Exercise YAML through `WithProfileFS` with a minimal alias of a built-in and + prove it resolves through inspection or preparation. Parser tests own YAML + edge cases, so do not repeat them here. +3. Add a focused public error-identity table: + - a directly selected absent profile still matches `ErrProfileNotFound` and + not `ErrProfileLoad`; + - an existing child with an absent base matches `ErrProfileLoad` and not + `ErrProfileNotFound`; and + - one representative cycle matches `ErrProfileLoad` and includes its IDs. + Reuse or extend the existing profile-inspection public error owner where + that keeps the suite lean. +4. Prove fresh ordinary resolution and frozen prepared execution with one + controlled mutable `fs.FS` workflow: + - prepare an execution while its base supplies model A; + - change the base source to model B after preparation; + - run the prepared handle through an injected deterministic client and + observe model A; and + - perform a new inspection or ordinary preparation and observe model B. + Keep all mutation sequential or synchronized and make no provider call. +5. Add only a narrow credential assertion if existing use-case tests do not + already prove the final inherited `APIKeyRequired` target behavior. The + resolver test owns sticky merging; current credential tests own availability, + precedence, redaction, and prepared rechecking. +6. Run all existing source-precedence, fallback, malformed-profile, built-in, + public JSON, runtime-override, credential, and prepared-execution tests. Fix + production behavior rather than weakening those contracts. + +### Verification + +```sh +gofmt -w engine.go engine_test.go public_contract_test.go prepared_execution_contract_test.go profiles.go types.go +go test ./... -run 'Profile|Inheritance|PreparedExecution|RuntimeOverride' +go test ./internal/profile go test ./internal/usecase -go test ./... +go test -race ./... +go vet ./... +go build ./... +git diff --check ``` -Stage 1 is complete when optional missing environment values no longer block -the use-case layer and explicit requirements retain their prior identities and -timing. +Stage 3 is complete when the engine resolves aliases and refinements across all +supported sources, public identities and errors match the roadmap, ordinary +operations remain fresh, and prepared execution remains frozen. -**Status:** Complete. - -## Stage 2: Omit Authentication in the Built-In Client When Optional Keys Are Missing +## Stage 4: Publish Canonical Documentation And Complete Validation ### Objective -Make the OpenAI-compatible transport implement the optional lookup contract at -the boundary that owns the outbound `Authorization` header. - -### Implementation - -1. Refactor the authentication branch in - `internal/llm/openai_compatible_client.go` so it resolves one credential in - this order: - - trimmed `req.Target.APIKey`; then - - the trimmed value of the environment variable named by the trimmed - `req.Target.APIKeyEnv`. -2. Set `Authorization` to `Bearer ` only when the resolved value is - nonblank. If the direct key is blank and the optional environment lookup is - absent, empty, or whitespace-only, leave the header unset and continue to - `http.Client.Do`. -3. Retain defensive enforcement for direct internal callers of the transport: - - if `req.Target.APIKeyRequired` is true and no usable source exists, return - `ErrInvalidRequest` before transport; - - use the existing missing-environment diagnostic when a nonblank - environment name was selected; and - - use a concise required-key diagnostic when no environment name exists. - Do not add an internal or public error type for this branch. -4. Preserve request construction, endpoint validation, timeout behavior, - response-body ownership, and direct-key redaction. Never serialize - `APIKeyEnv`, `APIKeyRequired`, or a credential into the JSON request body. -5. Allow an unauthenticated upstream non-2xx response to flow through the - existing `ProviderHTTPError` and public `GenerationError` machinery without - adding a credential-specific translation. - -### Tests - -Update `TestOpenAICompatibleClientAuthentication` in -`internal/llm/openai_compatible_client_test.go` as the transport contract owner. -Its table should cover only the meaningful credential states: - -- a direct key takes precedence over a populated environment value; -- a populated environment value supplies the bearer header when no direct key - is present; -- no configured source sends the request without `Authorization`; -- a named but unset optional environment sends the request without - `Authorization`; -- a named but unset required environment fails with `ErrInvalidRequest` before - transport; and -- a required target with no source also fails before transport. - -Use exact header assertions because presence or absence of `Authorization` is -the wire contract. Do not repeat endpoint, body, timeout, or response-error -matrices in this test. - -### Verification - -```sh -go test ./internal/llm -run TestOpenAICompatibleClientAuthentication -go test ./internal/llm -go test ./... -``` - -Stage 2 is complete when the built-in transport sends unauthenticated requests -for optional missing sources, still enforces explicit requirements, and all -existing provider-response behavior remains green. - -**Status:** Complete. - -## Stage 3: Align the Public Contract, Durable Documentation, and Full Validation - -### Objective - -Prove the assembled consumer workflow, update canonical contract owners, and -complete repository-wide validation. - -### Public Contract And Tests - -1. Update the GoDoc for `ErrAPIKeyEnvMissing` in `engine.go` so it applies only - when an explicitly required credential names an unset or empty environment - variable. Retain the exported value and its `ErrInvalidRequest` identity. -2. Update credential GoDoc where the exact public semantics are exposed: - - `Backend.APIKeyEnv` in `backends.go`; - - `ExecutionTarget.APIKeyEnv` and - `ExecutionTargetOverride.APIKeyEnv` in `types.go`; and - - any nearby `APIKeyRequired` wording that would otherwise imply every - named environment source is mandatory. - State that optional missing values cause the built-in client to omit - `Authorization`; do not promise that injected clients resolve environment - variables identically. -3. Replace `TestMissingCredentialsFailClearlyWhenProfileRequiresAuth` in - `engine_test.go`, whose old assertion is intentionally obsolete, with one - focused external-package contract test. Assemble a real engine with a - controlled HTTP client or local server, select a backend or file profile - whose `APIKeyEnv` is explicitly empty in the process environment, and have - the controlled upstream return a structured authentication failure. Assert: - - the request reaches upstream; - - `Authorization` is absent; - - the result is nil; - - the error does not match `ErrInvalidRequest` or - `ErrAPIKeyEnvMissing`; and - - the error matches `ErrLLMGenerate` and is discoverable as a - `*GenerationError` with the upstream status and representative provider - detail. -4. Keep this root test representative. The transport authentication table owns - the full credential matrix, and the existing generation-error tests own - envelope parsing, bounds, and formatting. +Align every canonical contract owner with the implemented feature, remove +temporary ambiguity, and validate the release candidate comprehensively. ### Documentation -Update each canonical owner only for its topic: - -1. In `docs/formats.md`, define profile, backend, and request `api_key_env` - values as optional lookup sources. In the credentials section, distinguish - them from `APIKeyRequired`, document header omission for a missing optional - value, retain direct-key precedence, and state that required availability is - validated during preparation and rechecked for prepared execution. -2. In `docs/integrations/openai-compatible-chat.md`, document the outbound - authentication wire behavior: a bearer header is sent only for a usable - direct or environment credential; otherwise the header is omitted and the - provider response is handled normally. -3. In `docs/internal/llm.md`, update the internal flow and failure categories to - distinguish optional omission from explicit required-key rejection. Preserve - any unrelated edits already present in this file. -4. In `docs/consumers/pkg-promptkit.md`, clarify near profile inspection or - credential guidance that a reported `APIKeyEnv` is a configured optional - source, while `APIKeyRequired` is the explicit local requirement. Link to - the exact public GoDoc or format reference rather than reproducing the full - precedence contract. -5. Do not change architecture policy, backend IDs/default names, release notes, - the README, or examples unless implementation uncovers a concrete - inaccurate current-state statement in one of those owners. +1. Finalize exact GoDoc for `Profile.BaseProfileID` and + `OpenAICompatibleProfileConfig.BaseProfileID`, plus nearby `Profile`, + `OpenAICompatibleProfile`, and `WithProfiles` text whose standalone + completeness or validation-timing statements changed. GoDoc owns the public + fields and construction contract; do not reproduce internal traversal. +2. Update `docs/formats.md` as the exact YAML contract owner: + - add `base_profile` to the field table and example; + - distinguish locally complete standalone profiles from derived aliases; + - document root-to-leaf override rules, complete-map `extra_params` + replacement, sticky `APIKeyRequired`, independent backend/endpoint + behavior, the 32-profile limit, and lack of clearing syntax; + - document cross-source lookup precedence and shadowing; and + - distinguish a missing selected profile from an invalid selected chain. +3. Update `docs/internal/sources.md` to describe the raw composite catalog, + the one outer resolving repository, fresh per-operation traversal, + cycle/depth protection, final validation, and prepared-state freezing. Link + to the format reference for exact consumer syntax and merge rules. +4. Update the `internal/profile` entry in `docs/internal/overview.md` because + the concrete package now resolves inherited definitions in addition to + loading, validating, and overlaying raw sources. Do not change architecture + policy because package ownership and dependency direction are unchanged. +5. Add one concise task-oriented example to + `docs/consumers/pkg-promptkit.md` showing an application-owned alias of a + built-in and a selected child override. Link to the format reference and + public GoDoc for details rather than duplicating the complete contract. +6. Do not update the README, examples, release documents, release procedure, + future/deferred catalogs, or built-in catalog unless the implementation has + made a concrete current-state statement there inaccurate. Do not copy the + roadmap into durable documentation. ### Final Validation @@ -302,31 +438,34 @@ go run ./examples/go-library/prepare go run ./examples/go-library/run ``` -Also run the documented tracked-Go formatting check, local Markdown-link -validation, repository-hygiene checks, ignored-file check, credential scan, and -`git diff --check`. Review the complete diff and confirm that: +Then run the documented tracked-Go formatting check, repository-relative +Markdown link checker, workspace/vendor/module-replacement checks, unstaged and +staged whitespace checks, ignored-file check, credential scan, and +`git diff --check`. Inspect the complete diff and confirm: -- optional absent, empty, and whitespace-only environment values omit - authentication and reach transport; -- direct and populated environment credentials still produce the correct - bearer header; -- explicit required flows still fail locally with the intended identities; -- ordinary, prepared, built-in, injected-client, and repair paths follow their - stated ownership boundaries; -- upstream non-2xx responses remain structured generation errors rather than - local credential errors; -- no credential value is serialized, retained in public metadata, or exposed - by formatting; and -- durable documentation describes only the now-implemented behavior with one - canonical owner per exact contract. +- only `base_profile`/`BaseProfileID` were added to consumer configuration; +- raw repositories remain source-local and the resolver wraps the full catalog + once; +- chain resolution is bounded, cycle-safe, fresh, and concurrency-safe; +- merge behavior covers every profile field without mutating source values; +- direct missing and dependent missing profiles preserve distinct public error + identities; +- selected IDs, backend resolution, runtime overrides, credentials, and + prepared freezing retain their stated ownership; +- resolution metadata never reaches stable execution JSON, hashes, injected + generation targets, or provider payloads; +- tests remain lean, deterministic, offline, and owned at the narrowest stable + boundary; and +- durable documentation describes only implemented behavior with one canonical + owner per exact contract. -Stage 3 is complete when the public workflow, documentation, and full -maintainer validation all match the target outcome. - -**Status:** Complete. +Stage 4 is complete when all focused and repository-wide checks pass and the +implementation, GoDoc, format reference, internal documentation, and consumer +guidance agree. ## Open Questions -None. Optional environment lookup, explicit requirement behavior, precedence, -prepared-execution timing, outbound header semantics, error compatibility, -documentation ownership, and test boundaries are fixed by this plan. +None. Naming, supported sources, lookup precedence, merge behavior, connection +semantics, credential inheritance, validation timing, chain bounds, error +identity, freshness, prepared freezing, package ownership, test boundaries, and +documentation ownership are fixed by the feature roadmap and this plan. diff --git a/docs/roadmap/profile-inheritance.md b/docs/roadmap/profile-inheritance.md new file mode 100644 index 0000000..63a4a59 --- /dev/null +++ b/docs/roadmap/profile-inheritance.md @@ -0,0 +1,224 @@ +# Profile Inheritance Roadmap + +## Purpose + +Allow a Promptkit execution profile to derive from another profile. This gives +downstream consumers stable, application-owned profile IDs without requiring +them to copy a built-in or shared profile's model and execution settings. + +For example, Weatherreporter should be able to select `weather-light` from its +prompt definitions while defining that profile as an alias or refinement of a +Promptkit built-in: + +```yaml +id: weather-light +base_profile: deepseek-4-flash +reasoning_effort: high +timeout_seconds: 120 +``` + +Changing only `base_profile` to another profile later should redirect every +prompt that selects `weather-light`, without requiring changes to deployed +prompt definitions. The inherited profile remains ordinary Promptkit +configuration rather than application-specific routing logic. + +## Target End State + +- File and `fs.FS` profile definitions may use the optional `base_profile` + field to name one parent profile. +- In-memory profiles expose the equivalent `Profile.BaseProfileID` field. + `OpenAICompatibleProfileConfig` exposes and forwards the same field so its + convenience constructor remains feature-complete. +- A profile containing only `id` and `base_profile` is a valid semantic alias. + Fields needed for an executable target may be inherited instead of repeated. +- Base profiles may come from any configured profile source, including the + embedded built-in catalog. Resolution uses the engine's complete assembled + profile catalog and its existing source precedence. +- A child may refine selected inherited settings using the ordinary profile + fields. After inheritance resolves, backend defaults and request overrides + retain their existing precedence. +- `Prepare`, `PrepareExecution`, `Run`, and `InspectProfile` all use the same + inheritance behavior. Prepared execution freezes the fully resolved target + and does not reopen the profile chain when it later runs. +- Public results continue to report the selected child ID, such as + `weather-light`, while effective backend, endpoint, model, credentials, and + execution settings reflect the resolved chain. + +## Profile Reference Semantics + +Each profile may name at most one direct base. Bases may themselves inherit, +so aliases and refinements can form a linear chain. Multiple inheritance and +merging an array of profiles are outside this feature. + +Every ID in a chain is resolved through the same composite catalog used for an +ordinary profile selection. Existing precedence therefore applies separately +to each lookup: + +1. in-memory profiles; +2. the ordinary configured profile source; +3. the application fallback profile source; and +4. the embedded built-in catalog. + +A higher-precedence definition of a base ID intentionally shadows a lower- +precedence definition, just as it would if selected directly. References are +not source-qualified, and there is no special syntax for bypassing an override +to select a lower-precedence or specifically built-in definition. A profile +cannot extend a shadowed definition with its own ID; that is a self-cycle. + +Profile chains are resolved afresh for each ordinary preparation, execution, +or inspection operation, preserving the current fresh-source behavior. The +resolver does not cache a chain across operations. Mutable sources are not +promised a transactional snapshot across separate file reads; callers that +need a frozen result use prepared execution. + +## Override Semantics + +Inheritance combines profile definitions from the root base to the selected +leaf. The leaf's `id` is always retained, and `base_profile` is resolution +metadata rather than an execution setting. + +Child fields use the profile conventions already exposed by Promptkit: + +- nonblank `backend`, `endpoint`, `model`, `service_tier`, + `reasoning_effort`, and `api_key_env` values replace inherited values; +- nonzero `temperature`, `max_tokens`, `top_p`, and `timeout_seconds` values + replace inherited values; +- omitted, blank, or zero fields inherit, according to the existing profile + contract; +- a nonempty `extra_params` map replaces the complete inherited map rather + than merging individual keys; and +- `APIKeyRequired: true` is inherited and remains sticky through the chain. + It retains the existing security behavior of clearing inherited profile or + backend environment sources and requiring a direct request key or an + explicit request `APIKeyEnv` override. + +This feature does not introduce profile-level presence pointers or clearing +syntax. A derived profile cannot use a blank string, numeric zero, false, or an +empty map to clear an inherited value because those states already mean +"unspecified" for profile configuration. Consumers can use the existing +presence-aware runtime overrides when they need an explicit zero or an empty +reasoning value. A future clearing syntax may be considered independently if +real consumer demand emerges. + +Connection fields retain their existing independent meanings. A child endpoint +may override an inherited endpoint without changing backend identity, and a +child backend replaces an inherited backend ID. Other fields continue to +inherit unless the child supplies their ordinary nonblank or nonzero override. + +## Validation And Failure Behavior + +Profile parsing and registration must distinguish local validity from resolved +completeness: + +- IDs, base IDs, supplied endpoints, numeric bounds, extra parameters, and + other values that already have source-local rules remain validated at their + owning input boundary. +- A standalone profile with no base continues to require a model and at least + one backend or endpoint. +- A profile with a base may omit those required target fields, because the + resolved chain may provide them. +- The completely merged profile must satisfy the same target invariants as a + current standalone profile before backend resolution and use. +- In-memory profiles without a base retain their current `NewEngine` + validation behavior. Reference existence, cycles, and resolved completeness + are evaluated when an inherited profile is selected or inspected, so file + and in-memory profiles share one resolution contract and fresh lower sources + are not frozen at engine construction. + +Resolution must detect direct and indirect cycles and impose a maximum chain +length of 32 profiles, including the selected leaf. The diagnostic should name +the relevant profile chain without exposing internal package values. + +Failure identity distinguishes the requested profile from its dependencies: + +- an absent directly selected profile retains `ErrProfileNotFound`; +- an existing selected profile whose base is absent, malformed, cyclic, + excessively deep, or incomplete fails with `ErrProfileLoad`, not + `ErrProfileNotFound`; and +- backend lookup and resolved-setting failures retain their existing + `ErrProfileLoad` behavior. + +A malformed higher-precedence definition remains authoritative and stops +fallback. Inheritance must not silently skip a broken base to use another +definition or partially resolve a chain. + +## Architectural Ownership + +Inheritance resolution belongs in `internal/profile`, around the fully +assembled composite repository. Individual filesystem, `fs.FS`, in-memory, +and built-in repositories continue to own discovery, strict decoding, local +normalization, defensive copying, and source precedence; they must not resolve +bases independently within their own source. + +The root facade assembles the raw composite catalog and the resolving profile +boundary. `internal/usecase` continues to request one selected profile and +resolve its backend; it should receive an already combined, caller-owned +profile rather than implementing recursion or source traversal itself. + +`internal/domain` may carry the normalized base ID and any internal information +needed to distinguish locally supplied fields, but those internal values must +not leak through public execution targets or provider requests. Final target +resolution still follows: + +1. framework defaults; +2. the backend selected by the resolved profile; +3. the fully resolved profile; and +4. request runtime overrides. + +## Public And Format Surface + +The feature adds only the smallest consumer-facing configuration needed for +one-parent inheritance: + +- YAML `base_profile` in the profile format; +- `Profile.BaseProfileID`; and +- `OpenAICompatibleProfileConfig.BaseProfileID`. + +Exact public behavior belongs in the new fields' GoDoc once implemented. The +profile format reference owns YAML syntax, merge rules, chain limits, and +validation behavior. Consumer guidance should show one concise alias/refinement +workflow and link to those canonical contracts rather than duplicating them. + +No new engine method, profile registry mutation API, public resolver, public +inheritance graph, or result provenance field is required. Existing profile +selection through prompt defaults and `RunRequest.ProfileID` remains unchanged. + +## Quality And Documentation End State + +Testing should follow the repository's lean ownership model: + +- profile parser and registration tests own the new field, local validation, + and defensive copying; +- profile resolver tests own cross-source lookup, precedence, chain merging, + cycle and depth protection, missing bases, and resolved completeness; +- use-case tests retain ownership of backend and runtime-override precedence + without duplicating the full inheritance matrix; and +- one representative external-package workflow should prove that a downstream + alias of a built-in profile reports the child ID and inherited effective + target through preparation or inspection. + +All tests remain deterministic, offline, parallel-safe, and independent of +real provider credentials. Existing standalone-profile, overlay, malformed- +selection, built-in, credential, and prepared-execution contracts must remain +green. + +The implemented feature's exact GoDoc, `docs/formats.md`, +`docs/internal/sources.md`, and focused consumer guidance must agree. The +internal component inventory reflects inheritance resolution as a concrete +`internal/profile` responsibility. Release documentation remains a separate +release-preparation concern. + +## Non-Goals + +- Multiple inheritance or ordered profile mixins. +- Deep or key-by-key merging of `extra_params`. +- Source-qualified references or a special built-in namespace. +- Extending a shadowed lower-precedence profile with the same ID. +- Per-request changes to the base-profile relationship. +- Profile-level clearing syntax or presence-aware scalar fields. +- Caching resolved chains across operations or watching sources for changes. +- Changing backend registry, concurrency, credential, runtime-override, + provider-wire, or prompt-selection semantics beyond applying them to the + resolved profile. +- Consumer-specific configuration discovery, deployment migration, or routing + policy. diff --git a/engine_test.go b/engine_test.go index 24b5167..db5f462 100644 --- a/engine_test.go +++ b/engine_test.go @@ -2018,6 +2018,25 @@ func TestWithProfilesRejectsDuplicateIDs(t *testing.T) { } } +func TestWithProfilesAcceptsDerivedDefinitionWithoutTargetFields(t *testing.T) { + _, err := promptkit.NewEngine(promptkit.Config{PromptDir: frameworkPromptDir}, + promptkit.WithProfiles(promptkit.Profile{ + ID: " derived-profile ", + BaseProfileID: " base-profile ", + }), + ) + if err != nil { + t.Fatalf("derived profile should be accepted during construction: %v", err) + } + + _, err = promptkit.NewEngine(promptkit.Config{PromptDir: frameworkPromptDir}, + promptkit.WithProfiles(promptkit.Profile{ID: "standalone-profile"}), + ) + if !errors.Is(err, promptkit.ErrInvalidConfig) { + t.Fatalf("standalone incomplete profile error = %v, want ErrInvalidConfig", err) + } +} + func TestWithProfilesRejectsInvalidExecutionSettings(t *testing.T) { type testCase struct { name string @@ -2170,6 +2189,7 @@ func TestOpenAICompatibleProfileMapsEveryField(t *testing.T) { extraParams := map[string]any{"provider_option": "distinct-extra-params"} got := promptkit.OpenAICompatibleProfile(promptkit.OpenAICompatibleProfileConfig{ ID: "distinct-id", + BaseProfileID: "distinct-base", BackendID: "distinct-backend", Endpoint: "https://distinct.example/v1", Model: "distinct-model", @@ -2184,6 +2204,7 @@ func TestOpenAICompatibleProfileMapsEveryField(t *testing.T) { }) want := promptkit.Profile{ ID: "distinct-id", + BaseProfileID: "distinct-base", BackendID: "distinct-backend", Endpoint: "https://distinct.example/v1", Model: "distinct-model", diff --git a/internal/domain/domain.go b/internal/domain/domain.go index 61069ff..7c28eac 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -192,6 +192,7 @@ type BackendCapacityPolicy struct { // ExecutionProfile describes how and where to execute a model. type ExecutionProfile struct { ID string `yaml:"id"` + BaseProfileID string `yaml:"base_profile"` BackendID string `yaml:"backend"` Endpoint string `yaml:"endpoint"` Model string `yaml:"model"` diff --git a/internal/profile/definition.go b/internal/profile/definition.go new file mode 100644 index 0000000..fd31a68 --- /dev/null +++ b/internal/profile/definition.go @@ -0,0 +1,47 @@ +package profile + +import ( + "errors" + "strings" + + "gitea.maximumdirect.net/eric/promptkit/internal/domain" +) + +// NormalizeAndValidateDefinition normalizes and validates one source-local +// profile definition without resolving a base profile. +func NormalizeAndValidateDefinition(profile *domain.ExecutionProfile) error { + if profile == nil { + return errors.New("profile is required") + } + + profile.ID = strings.TrimSpace(profile.ID) + profile.BaseProfileID = strings.TrimSpace(profile.BaseProfileID) + profile.BackendID = strings.TrimSpace(profile.BackendID) + profile.Endpoint = strings.TrimSpace(profile.Endpoint) + + if profile.ID == "" { + return errors.New("id is required") + } + if profile.Endpoint != "" { + endpoint, err := domain.NormalizeOpenAICompatibleBaseEndpoint(profile.Endpoint) + if err != nil { + return err + } + profile.Endpoint = endpoint + } + if profile.BaseProfileID == "" { + if profile.BackendID == "" && profile.Endpoint == "" { + return errors.New("backend or endpoint is required") + } + if strings.TrimSpace(profile.Model) == "" { + return errors.New("model is required") + } + } + + return domain.ValidateExecutionTargetSettings(domain.ExecutionTarget{ + Temperature: profile.Temperature, + MaxTokens: profile.MaxTokens, + TopP: profile.TopP, + TimeoutSeconds: profile.TimeoutSeconds, + }) +} diff --git a/internal/profile/filesystem_repository.go b/internal/profile/filesystem_repository.go index e8761d7..83b1cd3 100644 --- a/internal/profile/filesystem_repository.go +++ b/internal/profile/filesystem_repository.go @@ -127,12 +127,11 @@ func loadProfile(ctx context.Context, fsys fs.FS, root string, id string) (*doma if prof.ID != id { continue } - prof.BackendID = strings.TrimSpace(prof.BackendID) prof.ExtraParams, err = jsonvalue.CopyMap(prof.ExtraParams) if err != nil { return nil, fmt.Errorf("%w: %s: %v", ErrInvalidProfile, relPath, err) } - if err := normalizeAndValidateProfile(prof); err != nil { + if err := NormalizeAndValidateDefinition(prof); err != nil { if errors.Is(err, ErrRawAPIKeyNotAllowed) { return nil, fmt.Errorf("%w: %s", err, relPath) } @@ -255,30 +254,3 @@ func requireYAMLStreamEnd(decoder *yaml.Decoder) error { } return errors.New("profile file must contain exactly one YAML document") } - -func normalizeAndValidateProfile(p *domain.ExecutionProfile) error { - if strings.TrimSpace(p.ID) == "" { - return errors.New("id is required") - } - p.Endpoint = strings.TrimSpace(p.Endpoint) - if strings.TrimSpace(p.BackendID) == "" && p.Endpoint == "" { - return errors.New("backend or endpoint is required") - } - if p.Endpoint != "" { - endpoint, err := domain.NormalizeOpenAICompatibleBaseEndpoint(p.Endpoint) - if err != nil { - return err - } - p.Endpoint = endpoint - } - if strings.TrimSpace(p.Model) == "" { - return errors.New("model is required") - } - - return domain.ValidateExecutionTargetSettings(domain.ExecutionTarget{ - Temperature: p.Temperature, - MaxTokens: p.MaxTokens, - TopP: p.TopP, - TimeoutSeconds: p.TimeoutSeconds, - }) -} diff --git a/internal/profile/repository_test.go b/internal/profile/repository_test.go index 5d289c3..aabed53 100644 --- a/internal/profile/repository_test.go +++ b/internal/profile/repository_test.go @@ -858,6 +858,107 @@ top_p: .inf }) } +func TestProfileRepositoriesValidateDerivedDefinitions(t *testing.T) { + tests := []struct { + name string + files map[string]string + wantError error + wantBaseID string + wantProfile bool + }{ + { + name: "alias is locally valid and normalizes base id", + files: map[string]string{"alias.yaml": ` +id: selected-profile +base_profile: " base-profile " +`}, + wantBaseID: "base-profile", + wantProfile: true, + }, + { + name: "derived endpoint remains valid", + files: map[string]string{"invalid.yaml": ` +id: selected-profile +base_profile: base-profile +endpoint: /v1 +`}, + wantError: ErrInvalidProfile, + }, + { + name: "derived settings remain valid", + files: map[string]string{"invalid.yaml": ` +id: selected-profile +base_profile: base-profile +top_p: 1.1 +`}, + wantError: ErrInvalidProfile, + }, + { + name: "derived extra params remain valid", + files: map[string]string{"invalid.yaml": ` +id: selected-profile +base_profile: base-profile +extra_params: + timestamp: 2026-08-11T12:34:56Z +`}, + wantError: ErrInvalidProfile, + }, + { + name: "derived raw key remains prohibited", + files: map[string]string{"invalid.yaml": ` +id: selected-profile +base_profile: base-profile +api_key: secret +`}, + wantError: ErrRawAPIKeyNotAllowed, + }, + { + name: "derived duplicate id remains invalid", + files: map[string]string{ + "first.yaml": "id: selected-profile\nbase_profile: first-base\n", + "second.yaml": "id: selected-profile\nbase_profile: second-base\n", + }, + wantError: ErrInvalidProfile, + }, + { + name: "derived extra document remains invalid", + files: map[string]string{"invalid.yaml": ` +id: selected-profile +base_profile: base-profile +--- +id: other +`}, + wantError: ErrInvalidYAML, + }, + { + name: "standalone profile remains complete", + files: map[string]string{"invalid.yaml": "id: selected-profile\n"}, + wantError: ErrInvalidProfile, + }, + } + + for _, source := range profileRepositorySources() { + for _, tc := range tests { + t.Run(source.name+"/"+tc.name, func(t *testing.T) { + repo := source.newRepository(t, tc.files) + got, err := repo.GetProfile(context.Background(), "selected-profile") + if tc.wantError != nil { + if !errors.Is(err, tc.wantError) { + t.Fatalf("error = %v, want %v", err, tc.wantError) + } + return + } + if err != nil || !tc.wantProfile { + t.Fatalf("profile = %+v, error = %v, want valid derived definition", got, err) + } + if got.BaseProfileID != tc.wantBaseID { + t.Fatalf("BaseProfileID = %q, want %q", got.BaseProfileID, tc.wantBaseID) + } + }) + } + } +} + func TestOverlayRepository(t *testing.T) { ctx := context.Background() primaryProfile := &domain.ExecutionProfile{ID: "shared", Endpoint: "http://primary", Model: "primary"} diff --git a/profiles.go b/profiles.go index 1f7248e..36dd823 100644 --- a/profiles.go +++ b/profiles.go @@ -2,9 +2,7 @@ package promptkit import ( "context" - "errors" "fmt" - "strings" "gitea.maximumdirect.net/eric/promptkit/internal/domain" "gitea.maximumdirect.net/eric/promptkit/internal/jsonvalue" @@ -25,6 +23,7 @@ import ( func OpenAICompatibleProfile(cfg OpenAICompatibleProfileConfig) Profile { return Profile{ ID: cfg.ID, + BaseProfileID: cfg.BaseProfileID, BackendID: cfg.BackendID, Endpoint: cfg.Endpoint, Model: cfg.Model, @@ -87,8 +86,9 @@ func toDomainProfile(publicProfile Profile) (domain.ExecutionProfile, error) { return domain.ExecutionProfile{}, err } prof := domain.ExecutionProfile{ - ID: strings.TrimSpace(publicProfile.ID), - BackendID: strings.TrimSpace(publicProfile.BackendID), + ID: publicProfile.ID, + BaseProfileID: publicProfile.BaseProfileID, + BackendID: publicProfile.BackendID, Endpoint: publicProfile.Endpoint, Model: publicProfile.Model, Temperature: publicProfile.Temperature, @@ -100,34 +100,8 @@ func toDomainProfile(publicProfile Profile) (domain.ExecutionProfile, error) { APIKeyRequired: publicProfile.APIKeyRequired, ExtraParams: extraParams, } - if err := normalizeAndValidatePublicProfile(&prof); err != nil { + if err := profile.NormalizeAndValidateDefinition(&prof); err != nil { return domain.ExecutionProfile{}, err } return prof, nil } - -func normalizeAndValidatePublicProfile(prof *domain.ExecutionProfile) error { - if strings.TrimSpace(prof.ID) == "" { - return errors.New("id is required") - } - prof.Endpoint = strings.TrimSpace(prof.Endpoint) - if strings.TrimSpace(prof.BackendID) == "" && prof.Endpoint == "" { - return errors.New("backend or endpoint is required") - } - if prof.Endpoint != "" { - endpoint, err := domain.NormalizeOpenAICompatibleBaseEndpoint(prof.Endpoint) - if err != nil { - return err - } - prof.Endpoint = endpoint - } - if strings.TrimSpace(prof.Model) == "" { - return errors.New("model is required") - } - return domain.ValidateExecutionTargetSettings(domain.ExecutionTarget{ - Temperature: prof.Temperature, - MaxTokens: prof.MaxTokens, - TopP: prof.TopP, - TimeoutSeconds: prof.TimeoutSeconds, - }) -} diff --git a/types.go b/types.go index 92524dd..c73ca82 100644 --- a/types.go +++ b/types.go @@ -458,6 +458,12 @@ type ExecutionTargetOverride struct { type Profile struct { // ID is the required non-blank profile identifier. WithProfiles trims it. ID string + // BaseProfileID optionally names one base profile. WithProfiles trims it. A + // non-blank value permits required target fields to be inherited when the + // profile is selected or inspected, which is also when reference existence + // and resolved completeness are checked. A blank value leaves this as a + // standalone profile. + BaseProfileID string // BackendID optionally selects an engine backend. WithProfiles trims it. // Backend membership is checked when a request selects the profile; an // unknown ID makes preparation fail with ErrProfileLoad. @@ -506,6 +512,8 @@ type Profile struct { type OpenAICompatibleProfileConfig struct { // ID becomes Profile.ID. ID string + // BaseProfileID becomes Profile.BaseProfileID. + BaseProfileID string // BackendID becomes Profile.BackendID. BackendID string // Endpoint becomes Profile.Endpoint.