diff --git a/docs/formats.md b/docs/formats.md index 4e1b376..8cffdfd 100644 --- a/docs/formats.md +++ b/docs/formats.md @@ -197,18 +197,18 @@ The framework baseline is: | Setting | Default | | --- | --- | -| `temperature` | Unspecified and omitted from compatible provider requests unless a backend, profile, or runtime override selects it. | -| `max_tokens` | Unspecified and omitted from compatible provider requests unless a backend, profile, or runtime override selects it. | -| `top_p` | Unspecified and omitted from compatible provider requests unless a backend, profile, or runtime override selects it. | +| `temperature` | Unspecified and omitted from compatible provider requests unless a profile or runtime override selects it. | +| `max_tokens` | Unspecified and omitted from compatible provider requests unless a profile or runtime override selects it. | +| `top_p` | Unspecified and omitted from compatible provider requests unless a profile or runtime override selects it. | | `timeout_seconds` | `600` | -Numeric zero in a file or in-memory profile means that the profile does not -replace a lower-precedence value. With no lower value, `temperature`, -`max_tokens`, and `top_p` remain unspecified. Numeric request overrides use -pointers, so an explicit zero is retained and sent to compatible providers. -In particular, an explicit request `timeout_seconds` of zero disables the -per-generation deadline while leaving the caller context and transport timeout -intact. +Numeric zero in a file or in-memory profile does not select a numeric value. +For `temperature`, `max_tokens`, and `top_p`, it leaves the provider control +unspecified. For `timeout_seconds`, it retains the framework deadline. Numeric +request overrides use pointers, so an explicit zero is retained and sent to +compatible providers. In particular, an explicit request `timeout_seconds` of +zero disables the per-generation deadline while leaving the caller context and +transport timeout intact. Non-empty profile strings replace backend defaults, and non-empty request strings replace both. Request reasoning is the exception: a nil diff --git a/docs/integrations/openai-compatible-chat.md b/docs/integrations/openai-compatible-chat.md index 184cf74..e96734f 100644 --- a/docs/integrations/openai-compatible-chat.md +++ b/docs/integrations/openai-compatible-chat.md @@ -53,9 +53,9 @@ never also sent as a session header. The client conditionally includes: -- `temperature`, `max_tokens`, and `top_p` only when selected by a backend, - profile, or runtime override, including an explicit runtime zero; they are - absent when unspecified; +- `temperature`, `max_tokens`, and `top_p` only when selected by a profile or + runtime override, including an explicit runtime zero; they are absent when + unspecified; - non-empty `service_tier` and effective `reasoning_effort`; an explicitly disabled reasoning setting is empty and therefore omitted; and - `response_format` for JSON Schema structured output, including its name, diff --git a/docs/internal/overview.md b/docs/internal/overview.md index 7800458..5b0019d 100644 --- a/docs/internal/overview.md +++ b/docs/internal/overview.md @@ -11,7 +11,7 @@ contributor workflow and validation. | Component | Implemented responsibility | References | | --- | --- | --- | -| Root `promptkit` package | Provides the supported engine facade, source, backend-registration, and injection options, public request, result, prompt-inspection, and profile-inspection values, opaque prepared-execution handles, profile construction, extension interfaces, value conversion, redacted formatting, typed capacity errors, public error mapping, and engine-local assembly. | [Package GoDoc](../../doc.go), [prepared execution](../../prepared_execution.go), [backend API](../../backends.go), [engine assembly](../../engine.go) | +| Root `promptkit` package | Provides the supported engine facade, source, backend-registration, and injection options, public request, result, prompt-inspection, and profile-inspection values, opaque prepared-execution handles, profile construction, extension interfaces, value conversion, redacted formatting, typed capacity errors, public error mapping, and engine-local profile-source assembly. | [Package GoDoc](../../doc.go), [prepared execution](../../prepared_execution.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 through the shared JSON-value package, and consumes the LLM-owned OpenAI-compatible reserved request-field rule. | [Backend registry](../../internal/backend/registry.go) | @@ -22,7 +22,7 @@ contributor workflow and validation. | `internal/jsonvalue` | Validates and deeply copies JSON-compatible extra-parameter and prepared-schema 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/profile/builtin` | Embeds the built-in profile catalog, whose entries select OpenRouter. | [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 and creates frozen validation plans for prepared execution. | [Framework formats](../formats.md#schemas), [internal sources and validation](sources.md) | diff --git a/docs/internal/sources.md b/docs/internal/sources.md index 3773e9b..01e52d1 100644 --- a/docs/internal/sources.md +++ b/docs/internal/sources.md @@ -28,26 +28,30 @@ duplicate detection, and source containment: ## Profiles And Built-Ins `internal/profile` loads and validates execution profiles from an -operating-system filesystem or an `fs.FS`. It supports a primary repository -with fallback only when the primary reports that a profile is absent. Strict -YAML decoding recognizes the optional `backend` field, trims its value, and -requires a model plus at least one non-blank backend or endpoint. Loading does -not check registry membership because the available registry belongs to the -assembled engine; the runner checks membership during preparation and exact -profile inspection. +operating-system filesystem or an `fs.FS`. Its overlay repository consults the +next repository only when the higher-precedence repository reports that a +profile is absent. Strict YAML decoding recognizes the optional `backend` +field, trims its value, and requires a model plus at least one non-blank +backend or endpoint. Loading does not check registry membership because the +available registry belongs to the assembled engine; the runner checks +membership during preparation and exact profile inspection. + +The root engine assembles profile repositories in precedence order: in-memory +profiles, one ordinary configured source, then the embedded built-in catalog. +An explicit file or `fs.FS` profile source replaces `Config.ProfileDir` within +the ordinary configured-source category. Exact profile inspection performs one point-in-time lookup through those profile sources and checks the resolved target without reading prompt, input, or schema sources. It does not retain that lookup for a later execution. -`internal/profile/builtin` embeds the maintained built-in profile catalog and -can place a caller-selected repository ahead of that catalog. Every embedded -profile selects `openrouter` and inherits its endpoint and credential -environment-variable name from the built-in backend registry rather than -repeating those values. Profile behavior is owned by the -[profile repository tests](../../internal/profile/repository_test.go), while -catalog completeness, the backend-selection invariant, duplicate IDs, and -overlay behavior are owned by the +`internal/profile/builtin` embeds the maintained built-in profile catalog. +Every embedded profile selects `openrouter` and inherits its endpoint and +credential environment-variable name from the built-in backend registry rather +than repeating those values. Profile loading and overlay behavior are owned by +the [profile repository tests](../../internal/profile/repository_test.go), +while catalog completeness, the backend-selection invariant, and duplicate IDs +are owned by the [built-in repository tests](../../internal/profile/builtin/repository_test.go). ## Ordinary Artifacts diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index 6ca4f85..1acf922 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -1,334 +1,458 @@ -# Optional Request-Parameter Omission Implementation Plan +# Application Fallback Profiles Implementation Plan -**Status:** Complete. +**Status:** Ready for implementation. ## Purpose This document is the decision-complete implementation plan for -[omitting unset optional request parameters](optional-request-parameters.md). -It is written for a `gpt-5.6-terra` coding agent that will implement each stage -in order. +[application fallback profiles](fallback-profiles.md). It is written for a +`gpt-5.6-terra` coding agent that will implement each stage in order. The feature roadmap owns the motivation, policy choices, compatibility boundary, non-goals, and target end state. This document owns the concrete design, file-level work, test ownership, documentation updates, validation, and completion gates. -The separate [application fallback profiles](fallback-profiles.md) roadmap is -not part of this implementation plan. Preserve it unchanged for its own later -planning and implementation cycle. - ## Implementation Rules -- Complete the stages in order. Stage 1 must leave code, tests, GoDoc, and - current-state documentation mutually accurate; Stage 2 performs the final - audit and repository-wide acceptance. -- Preserve unrelated working-tree changes. In particular, do not edit, - implement, retire, or reclassify `fallback-profiles.md`. +- Complete the stages in order. Every stage must leave the repository + buildable, tested for the behavior changed in that stage, and accurately + documented for its implemented state. +- Preserve unrelated working-tree changes. Inspect `git status --short` and + the relevant diffs before editing, and do not overwrite or reformat + pre-existing user work. - Follow every policy under `docs/policy/`, the task-specific reading guide in `docs/development.md`, and the accepted behavior in - `optional-request-parameters.md`. -- Keep the existing package boundaries. Framework defaults remain in - `internal/defaults`, resolution remains in `internal/usecase`, and outbound - OpenAI-compatible serialization remains in `internal/llm`. -- Do not add an exported type, field, option, method, error, or public package. - This feature changes default and wire semantics within existing contracts. -- Do not replace numeric profile fields with pointers or add profile presence - tracking. File and in-memory profile zero values retain their existing - inheritance semantics; runtime pointer overrides remain the only supported - way to select an explicit numeric zero. -- Preserve required request fields, session IDs, structured output, - credentials, extra-parameter validation, reasoning clearing, deadlines, - capacity management, and all existing precedence rules. -- Do not query a provider for defaults or capabilities and do not add - backend- or model-specific serialization branches. -- Keep tests lean and behavioral. Use the existing root precedence test to own - resolved public/injected-client metadata and the existing model-client tests - to own wire inclusion and omission. Do not duplicate those matrices in a - new end-to-end fixture. -- Update exact exported semantics in GoDoc, profile/default semantics in - `docs/formats.md`, and provider request-body semantics in - `docs/integrations/openai-compatible-chat.md` in the same stage as the code. + `fallback-profiles.md`. +- Keep the module root as the public facade. The root package owns profile + source selection and composition; `internal/profile` owns repository, + parsing, validation, and error-preserving overlay behavior; and + `internal/profile/builtin` owns only the embedded built-in catalog. +- Add only the public `WithFallbackProfileFS` option. Do not add an exported + repository type, source-provenance value, fallback-specific error, config + field, programmatic fallback-profile option, or public package. +- Reuse `profile.NewFSRepository` and `profile.NewOverlayRepository`. Do not + add another parser, validator, repository implementation, or merge model. +- Preserve lazy loading. Engine construction validates the option arguments, + not every file in the supplied filesystem. A profile source is read only + when resolution reaches it. +- Preserve existing error identities and mappings. Only + `profile.ErrProfileNotFound` permits an overlay to consult its next layer; + every other error from a higher layer must be returned and mapped through + the existing public profile-load path. +- Keep tests classical and behavior-focused. Public source precedence and + exported option semantics belong in external-package root tests; generic + overlay behavior remains owned by `internal/profile` tests. +- Update GoDoc and current-state documentation in the same stage that exposes + the public option. Do not describe the feature as implemented before that + stage is complete. - Do not add release notes, change a module version, commit, tag, push, or publish a release as part of this plan. ## Fixed Design -### Framework Defaults +### Public API -In `internal/defaults/defaults.go`, remove these constants: +Add this function to `engine.go` beside the existing profile-source options: ```go -ExecutionDefaultTemperature -ExecutionDefaultMaxTokens -ExecutionDefaultTopP +func WithFallbackProfileFS(fsys fs.FS, root string) Option ``` -They currently encode zero for `temperature` and `max_tokens` and one for -`top_p`. Optional provider controls are no longer framework defaults, so -retaining zero-valued constants under default-oriented names would obscure the -new contract. +The option accepts the same filesystem and root forms as `WithProfileFS` and +constructs its repository with `profile.NewFSRepository(fsys, root)`. It must +return `ErrInvalidConfig` from option application when `fsys` is nil or when +`strings.TrimSpace(root)` is empty. Do not normalize or replace a valid root +before passing it to the repository. -Keep `ExecutionDefaultTimeoutSeconds` at its current positive value. Timeout is -a Promptkit-owned generation deadline and is not an OpenAI-compatible request -body field. +The fallback source is its own last-value-wins option category. Add these two +private fields to `engineOptions`: -Keep `ExecutionTargetDefault` as the common resolution baseline, but have it -initialize only `TimeoutSeconds`. The zero Go values for `Temperature`, -`MaxTokens`, and `TopP` then represent unspecified provider controls. Do not -rename this internal function or add a second defaults constructor. +```go +fallbackProfiles profile.Repository +fallbackProfileSource bool +``` -### Resolution And Public Metadata +Use the repository field for the selected source and the boolean only to +distinguish an unapplied option from an applied option. A later valid +`WithFallbackProfileFS` replaces both values. Option application remains +sequential, so an invalid option fails `NewEngine` immediately even if a later +option could otherwise replace it. -Do not change the merge functions or precedence in -`internal/usecase/runner.go`: +The exact GoDoc for `WithFallbackProfileFS` must state: -1. the baseline target supplies only the Promptkit timeout; -2. nonzero profile numeric fields replace the baseline; -3. non-nil runtime numeric overrides replace profile values; and -4. `ExecutionTargetPresence` records runtime overrides, including explicit - zero values. +- that it supplies application-owned fallback profile definitions; +- the full four-layer lookup order; +- that definitions are whole profiles and are not field-merged; +- that only a missing ID falls through, while a matching read, parse, + duplicate, validation, or credential-format failure stops resolution; +- that loading and validation are lazy; +- that files use the ordinary strict profile YAML and `api_key_env` rules; +- that nil filesystems and blank roots cause `NewEngine` to match + `ErrInvalidConfig`; +- that repeated calls use the last valid fallback source; and +- that this is definition lookup, not provider or generation failover. -Consequently, when a profile omits the optional provider controls, resolved -`ExecutionTarget` values contain zero for `Temperature`, `MaxTokens`, and -`TopP`. That zero is stable metadata for “unspecified” unless the accompanying -`GenerateRequest.TargetPresence` bit reports an explicit runtime zero. +Update the `Option`, `Config.ProfileDir`, `WithProfileFS`, `WithProfileFile`, +and `WithProfiles` GoDoc in `engine.go` where necessary so their relative +precedence is unambiguous. Exact declarations and behavior remain owned by +GoDoc; consumer documentation should summarize the workflow and link readers +back to the API rather than reproduce every error clause. -Do not expose target presence in `PreparedRun`, `RunResult`, or -`ProfileInspection`, and do not change their stable JSON shapes. As already -true for `max_tokens`, those metadata values report the resolved numeric value -rather than provenance. A prepared result containing `top_p: 0` therefore does -not distinguish an unspecified value from an explicit runtime zero; injected -clients receive the separate presence value when the distinction affects -execution. +### Repository Composition -Update root GoDoc in `types.go` so it no longer calls an unspecified optional -provider control an effective provider value: +Move all profile-source composition into one private root helper in +`engine.go`: -- `ExecutionTarget.Temperature`, `MaxTokens`, and `TopP` must each state that - zero leaves the field unspecified to compatible providers unless the - corresponding `ExecutionTargetPresence` bit is true; -- `ExecutionTarget.TimeoutSeconds` retains its existing deadline semantics; -- `Profile` and `ExecutionTargetOverride` documentation must describe zero or - nil as inheriting a lower-precedence value and otherwise leaving the provider - control unspecified, rather than implying that every field receives a - concrete framework value; and -- `PreparedRun`, `ProfileInspection`, and other effective-target summaries may - continue to describe precedence, but must not imply that Promptkit knows a - provider's omitted default. +```go +func newProfileRepository(profileDir string, options engineOptions) profile.Repository +``` -Do not change field types, field order, JSON tags, conversion functions, string -formatting, or copying behavior. +`NewEngine` must call this helper once and pass its returned repository to the +runner. The helper must build from lowest to highest precedence: -### Outbound Request Semantics +1. begin with `builtin.NewRepository()`; +2. if `options.fallbackProfileSource` is true, overlay + `options.fallbackProfiles` over the built-in repository; +3. select exactly one ordinary configured source: use `options.profiles` when + `options.profileSource` is true; otherwise, when `profileDir` is nonblank, + use `profile.NewFilesystemRepository(profileDir)`; overlay that selected + source over the current repository; +4. if `options.memorySource` is true, overlay `options.memoryProfiles` over + the current repository; and +5. return the resulting chain. -The current built-in client already has the required mechanism: -`openAIChatRequestFromGenerateRequest` includes `temperature`, `max_tokens`, or -`top_p` when the resolved value is nonzero or the corresponding target-presence -bit is true, and `openAIChatRequestPayload` omits nil fields. Preserve that -logic. +This preserves the existing rule that `WithProfileFS` or `WithProfileFile` +replaces `Config.ProfileDir`; those sources are alternatives in one ordinary +configured-source category, not two independent layers. `WithProfiles` remains +a distinct highest-precedence category. -No production change should be needed in -`internal/llm/openai_compatible_client.go`. Change it only if a focused failing -test demonstrates that the existing implementation does not meet this plan; -do not special-case `top_p`, inspect profile provenance, or move framework -default policy into the transport. +The final lookup order is therefore: -The resulting behavior is: +```text +WithProfiles + -> WithProfileFile / WithProfileFS / Config.ProfileDir + -> WithFallbackProfileFS + -> Promptkit built-ins +``` -- an omitted profile `top_p` resolves to zero and is absent from the body; -- a nonzero profile or runtime `top_p` is included; -- an explicit runtime `top_p` of zero is included because presence is true; -- the same rules continue to apply to `temperature` and `max_tokens`; -- empty `service_tier` and effective `reasoning_effort` remain absent; -- configured `extra_params` remain present after validation; and -- `model`, `messages`, conditional `session_id`, and conditional - `response_format` remain unchanged. +Every arrow is a whole-profile, not-found-only fallback. Do not inspect or +copy profile fields in the composition helper. -### Profile Formats And Built-In Profiles +### Built-In Package Boundary -Do not change YAML or public `Profile` field shapes. Numeric zero in a file or -in-memory profile continues to mean “do not replace the lower layer.” With no -lower provider value, zero therefore resolves to unspecified. An explicit -profile-level numeric zero remains unsupported; consumers use a runtime -pointer override when zero itself must be sent. +Reduce `internal/profile/builtin/repository.go` to the embedded catalog leaf: -Do not edit files under `internal/profile/builtin/assets/`. Values declared in -those files are explicit profile policy and remain effective. Existing -nonzero-profile tests are sufficient to protect explicit inclusion; do not add -one test per built-in asset or parameter. +```go +func NewRepository() profile.Repository +``` + +Remove `NewRepositoryWithPrimary` and `NewRepositoryWithDirectory`. Remove +their now-obsolete tests from `internal/profile/builtin/repository_test.go` and +remove imports used only by those helpers or tests. Do not move their tests to +another private helper: existing root public-behavior tests own assembled +precedence, and `internal/profile.TestOverlayRepository` owns not-found-only +overlay semantics. + +Do not change built-in YAML assets, built-in validation, the `profile.Repository` +interface, `profile.NewOverlayRepository`, or filesystem repository behavior. + +### Resolution And Error Semantics + +The runner receives one assembled `profile.Repository`; do not add fallback +logic to `InspectProfile`, `Prepare`, `PrepareExecution`, `Run`, or +`RunPrepared`. Those paths must continue to resolve through the runner's one +repository dependency. + +The existing overlay contract is authoritative: + +- a successful lookup returns the complete higher-layer profile; +- `profile.ErrProfileNotFound` consults the next layer; +- cancellation, filesystem read failures, malformed YAML, duplicate matches, + raw `api_key`, invalid profiles, and all other errors stop lookup; and +- the root facade maps failures through the existing public identities such as + `ErrProfileNotFound` and `ErrProfileLoad`. + +Do not add eager filesystem walking in the option or `NewEngine`. A malformed +asset unrelated to the requested ID retains the existing ordinary +`FSRepository` behavior; this plan does not strengthen that package's global +validation guarantees. ### Test Ownership -Use these existing boundaries: +Use the following test boundaries and avoid duplicating the profile parser's +existing case matrix. -- In `engine_test.go`, update the “framework defaults” row of - `TestEngineExecutionSettingPrecedence` so the zero-valued profile expects - `TopP: 0` while retaining `Temperature: 0`, `MaxTokens: 0`, and the positive - timeout. Rename that row to describe unspecified provider controls plus the - framework timeout. Keep the rows proving nonzero profile precedence and - explicit runtime-zero presence unchanged. -- Remove - `TestRunnerRunBuiltInDefaultsUsedWhenProfileOmitsOptionalFields` from - `internal/usecase/runner_test.go`. Its literal-default assertions duplicate - the stronger assembled root precedence test and depend on the internal - constants being removed. Do not replace it with another internal - default-value test. -- Keep - `TestOpenAICompatibleClientOmitsImplicitZeroNumericFields` and - `TestOpenAICompatibleClientSerializesExplicitZeroNumericOverrides` in - `internal/llm/openai_compatible_client_test.go`. Together they own the wire - distinction and should pass without weakening their assertions. -- Keep the existing nonzero request serialization and profile-precedence tests - passing. They prove that explicitly configured values continue to be sent - and selected. +In `public_contract_test.go`: -Do not add snapshots, golden files, provider calls, or a broad duplicate -integration test. Add a new test only if the implementation exposes a distinct -contract risk not covered by the tests above, and record that reason in the -test name or nearby test structure rather than in a new planning document. +- Add `TestFallbackProfileSourcePrecedence`. Use minimal synthetic + `fstest.MapFS` profiles and, where `Config.ProfileDir` is under test, a + `t.TempDir`. Cover these distinct relationships: an in-memory profile beats + both ordinary and fallback definitions; an ordinary `WithProfileFS` source + beats a fallback definition; `Config.ProfileDir` beats a fallback + definition when no ordinary source option replaces it; a fallback + definition beats a built-in definition with the same ID; and an ID absent + from the fallback source still resolves from the built-in catalog. Assert + the selected model or another stable complete-profile field rather than + internal repository structure. +- Extend `TestRepeatedOptionsUseLastValueInEachCategory` with a + `fallback profile source` subtest proving that the later valid fallback + filesystem is selected. +- Add `TestFallbackProfileSourcePreservesLazyLoadingAndErrors`. Prove that + engine construction succeeds without reading malformed fallback YAML, that + an unrelated malformed file does not prevent a valid requested fallback + definition from resolving under existing FS-repository semantics, that a + malformed fallback file whose stem matches a built-in profile ID yields + `ErrProfileLoad` instead of silently reaching the built-in, and that a + malformed ordinary configured definition yields `ErrProfileLoad` instead of + reaching a valid application fallback definition. Use `errors.Is`; do not + assert complete error strings. +- Add one representative workflow test that supplies a fallback-only profile + and verifies the same effective model through `InspectProfile`, `Prepare`, + a `PrepareExecution` followed by `RunPrepared`, and direct `Run`. Use the + existing deterministic injected-client style, no live provider, and no real + credential. This test owns the cross-workflow repository wiring; do not + repeat the full precedence matrix through every method. + +In `engine_test.go`, extend `TestSourceOptionsRejectInvalidInputs` with nil +filesystem and blank-root cases for `WithFallbackProfileFS`. Both must make +`NewEngine` match `ErrInvalidConfig`. + +Retain `internal/profile.TestOverlayRepository` unchanged unless a genuine +existing defect is found. It already owns success, not-found fallback, and +non-not-found error preservation. Do not add package-private tests for the new +root helper, snapshots, golden files, provider calls, or one test per profile +format error already covered by `internal/profile`. ### Canonical Documentation -Update current-state documentation in Stage 1: +Update current-state documentation when the option is implemented: -- In `docs/formats.md`, replace the optional provider-control entries in the - framework-default table with clear unspecified/omitted semantics, while - retaining the positive `timeout_seconds` framework default. Explain that - profile numeric zero inherits a lower layer and otherwise remains - unspecified; an explicit runtime pointer zero is retained. -- In `docs/integrations/openai-compatible-chat.md`, state that - `temperature`, `max_tokens`, and `top_p` are included only when selected by a - profile or runtime override, including explicit runtime zero, and are absent - when unspecified. Keep the existing ownership of required fields, - `session_id`, structured output, extra parameters, and timeout behavior. -- In `types.go`, apply the GoDoc changes described above; these declarations - own the exact public value semantics. +- In `docs/formats.md`, make the source-precedence section the canonical + four-layer definition lookup order. State that ordinary configured sources + override application fallbacks, application fallbacks override built-ins, + profiles are whole values, and only a missing ID falls through. Retain this + document's ownership of strict YAML, credentials, validation, and source + discovery details. +- In `docs/consumers/pkg-promptkit.md`, add a short task-oriented section that + shows an illustrative `embed.FS` declaration and + `WithFallbackProfileFS`. Explain that application defaults belong in the + embedded fallback and operator overrides belong in the ordinary configured + source. Link to `docs/formats.md` for exact format and precedence rules, and + do not turn the snippet into a second complete maintained application. +- In `docs/internal/sources.md`, describe the root-owned four-layer + composition and the existing not-found-only overlay mechanism. Remove any + claim that the built-in package composes caller-selected repositories. +- In `docs/internal/overview.md`, keep the root facade responsible for source + assembly, describe `internal/profile/builtin` only as the embedded catalog, + and reflect the implemented fallback layer without duplicating the exact + public API contract. +- In `engine.go`, apply the GoDoc changes under Public API. GoDoc owns the + exact option signature, validation, category, and public semantics. -Do not add a README or release-document note. The consumer guide already -routes exact field behavior to GoDoc and profile/default behavior to the format -reference, so do not duplicate the new contract there. The internal LLM -document describes flow rather than exact field omission and does not require -a change unless its current text is found to contradict the implementation. +The architecture policy already assigns assembly to the root facade and the +built-in catalog to `internal/profile/builtin`; do not edit it unless the +implementation reveals an actual contradiction. No integration protocol, +outbound request body, profile YAML shape, README orientation, or maintained +example changes as part of this feature. -## Stage 1: Implement Omission Semantics And Canonical Contracts +## Stage 1: Move Existing Profile Composition To The Root Facade ### Objective -Remove optional provider controls from the framework baseline, preserve -explicit profile and runtime values, update the canonical contracts, and prove -the behavior at the existing resolution and wire boundaries. +Establish the intended ownership boundary and a single root composition point +without changing public behavior or adding the fallback option. ### Implementation Prompt Implement only Stage 1 of `docs/roadmap/implementation.md`. Read the complete feature roadmap, implementation rules, and fixed design above before editing. -1. In `internal/defaults/defaults.go`, remove the three provider-control - constants and make `ExecutionTargetDefault` initialize only - `TimeoutSeconds`. -2. In `engine_test.go`, update and rename the default-precedence table row - exactly as described under Test Ownership. -3. Remove the redundant literal-default test from - `internal/usecase/runner_test.go`; do not weaken other precedence, - profile-value, or runtime-zero tests. -4. Update the affected exported GoDoc in `types.go` without changing any - declaration, JSON tag, or serialization shape. -5. Update `docs/formats.md` and - `docs/integrations/openai-compatible-chat.md` according to Canonical - Documentation. -6. Run the focused validation below. Repair regressions in scope, but do not - broaden the feature or change the established serializer merely to make a - mistaken expectation pass. +1. In `engine.go`, add `newProfileRepository(profileDir string, options + engineOptions) profile.Repository` and move the existing three-layer + assembly into it: built-ins, then the selected ordinary configured source, + then `WithProfiles`. Do not add fallback fields or the public option yet. +2. Replace the inline profile assembly in `NewEngine` with one call to the + helper. Preserve the existing replacement relationship between + `Config.ProfileDir` and `WithProfileFS`/`WithProfileFile`. +3. In `internal/profile/builtin/repository.go`, remove + `NewRepositoryWithPrimary` and `NewRepositoryWithDirectory`, leaving + `NewRepository` as the only constructor. +4. Remove the three tests dedicated to the deleted built-in composition + helpers from `internal/profile/builtin/repository_test.go`. Preserve tests + that validate the embedded catalog itself. +5. Update `docs/internal/sources.md` and `docs/internal/overview.md` so they + describe the implemented Stage 1 ownership accurately. At this boundary + the source order is still in-memory, ordinary configured source, built-ins; + do not document the application fallback as implemented yet. +6. Run the focused validation below. Fix in-scope regressions without adding + fallback behavior early. -Do not edit built-in profile assets, fallback-profile work, backend -registration, profile parsing, target merge logic, public value shapes, -prepared-execution lifecycle, capacity management, or release material. +Do not add or mention an implemented `WithFallbackProfileFS` in Stage 1. Do +not change exported declarations, profile parsing, profile assets, error +mapping, runner behavior, or consumer and format documentation. ### Focused Validation Run from the repository root: ```sh -gofmt -w internal/defaults/defaults.go types.go engine_test.go \ - internal/usecase/runner_test.go -go test . -run 'TestEngineExecutionSettingPrecedence' -go test ./internal/llm -run \ - 'TestOpenAICompatibleClient(GenerateSuccess|OmitsImplicitZeroNumericFields|SerializesExplicitZeroNumericOverrides)' -go test ./internal/usecase -run \ - 'Test(ResolveExecutionTarget|RunnerPrepareRequestNumericOverridePresence|RunnerPrepareSelectedProfileBeatsBuiltInDefault|RunnerRunSelectedProfileBeatsBuiltInDefault)' -go test . ./internal/defaults ./internal/usecase ./internal/llm -go vet . ./internal/defaults ./internal/usecase ./internal/llm +gofmt -w engine.go internal/profile/builtin/repository.go \ + internal/profile/builtin/repository_test.go +go test . -run \ + 'Test(PrepareUsesBuiltInProfileWithoutProfileDir|CustomProfileOverridesBuiltInProfile|InMemoryProfilesOverrideBuiltInsAndProfileSources)$' +go test ./internal/profile/... +go test . ./internal/profile/... +go vet . ./internal/profile/... git diff --check ``` -If a focused regular expression does not match an existing test name, inspect -the current names and run the narrowest equivalent set; do not silently skip -the intended resolution, explicit-profile, explicit-zero, and wire-omission -coverage. +If a focused expression does not match an existing test name, inspect the +current suite and run the narrowest equivalent public precedence coverage; do +not silently skip the intended relationship. ### Completion Gate Stage 1 is complete only when: -- the resolution baseline contains no provider tuning value and retains the - Promptkit timeout; -- an omitted `top_p` resolves to zero and the built-in client omits it; -- nonzero profile and runtime values remain effective and serialized; -- explicit runtime zero values remain distinguishable and serialized through - `ExecutionTargetPresence`; -- no public type or stable JSON shape changed; -- required fields, structured output, session IDs, extra parameters, - reasoning, credentials, and deadlines retain their existing behavior; -- GoDoc, format documentation, and the integration contract describe the - implemented behavior without conflicting ownership; and +- the root facade assembles the unchanged three-layer profile chain in one + private helper; +- ordinary source options still replace `Config.ProfileDir` and in-memory + profiles still have highest precedence; +- built-in profiles remain available and remain lower than consumer sources; +- only `internal/profile` owns generic overlay behavior and the built-in + package owns only its embedded catalog; +- no public API or behavior changed; +- internal current-state documentation matches that boundary; and - all focused tests, vet, formatting, and whitespace checks pass. -## Stage 2: Audit Compatibility And Validate The Repository +## Stage 2: Add Application Fallback Profiles And Public Contracts ### Objective -Confirm that the narrow semantic change is complete across all public, -injected-client, built-in-profile, documentation, and repository surfaces, -then mark the temporary planning documents complete. +Add the public option, insert the application fallback into the root-owned +repository chain, prove its precedence and failure behavior through public +workflows, and publish the canonical current-state documentation. ### Implementation Prompt Implement only Stage 2 of `docs/roadmap/implementation.md` after Stage 1 satisfies its completion gate. -1. Search tracked Go and Markdown files for the removed constant names, - framework `top_p` defaults, claims that all effective provider controls have - concrete framework values, and request-body inclusion rules. Correct only - stale statements or tests owned by this feature. -2. Confirm that `internal/profile/builtin/assets/` has no feature-related diff - and that its explicit nonzero optional controls still pass ordinary profile - validation and resolution tests. -3. Confirm that `internal/llm/openai_compatible_client.go` either has no diff or - contains only a change required by a focused failing contract test. The - default policy must remain outside the transport. -4. Follow every changed Markdown link and confirm its target exists. Verify - that current-state documents describe implemented behavior and that exact - contracts remain with GoDoc, the format reference, and the integration - contract. -5. Run the complete validation sequence below and repair only in-scope - failures. -6. After all checks pass, change the status of - `optional-request-parameters.md` and this document to `Complete`. Do not - change the status or contents of `fallback-profiles.md`. -7. Re-run `git diff --check` and inspect the final working tree and diff. +1. Add `fallbackProfiles` and `fallbackProfileSource` to `engineOptions`, then + implement `WithFallbackProfileFS` exactly as specified under Public API. +2. Extend `newProfileRepository` so it constructs the fixed four-layer chain + in the prescribed low-to-high order. Do not alter generic overlay logic or + add fallback branches to runner methods. +3. Update all affected `engine.go` GoDoc, including the option category list + and the relative precedence descriptions for existing profile sources. +4. Add and extend the external-package root tests exactly as specified under + Test Ownership. Reuse small existing fakes and fixture helpers where they + remain clear; add only minimal synthetic YAML helpers needed by these tests. +5. Extend `TestSourceOptionsRejectInvalidInputs` with the two fallback option + validation cases. +6. Update `docs/formats.md`, `docs/consumers/pkg-promptkit.md`, + `docs/internal/sources.md`, and `docs/internal/overview.md` according to + Canonical Documentation. +7. Run the focused validation below. Repair in-scope failures without + weakening existing parser, error-identity, prepared-execution, or profile + precedence guarantees. -Do not delete temporary roadmaps in this stage; retirement is a separate -maintainer action. Do not add release notes, change versions, or create a -commit, tag, push, or release. +Do not add a config field, in-memory fallback API, source provenance, profile +inheritance, provider failover, eager validation, application-specific assets, +or backend/model policy. Do not edit `internal/profile/builtin/assets/`. + +### Focused Validation + +Run from the repository root: + +```sh +gofmt -w engine.go engine_test.go public_contract_test.go +go test . -run \ + 'Test(FallbackProfileSource|RepeatedOptionsUseLastValueInEachCategory|SourceOptionsRejectInvalidInputs)' +go test ./internal/profile/... +go test . ./internal/profile/... ./internal/usecase +go vet . ./internal/profile/... ./internal/usecase +git diff --check +``` + +The focused root expression must execute the precedence, lazy/error, +cross-workflow, repeated-option, and invalid-input coverage described above. +If the implemented names differ slightly, run explicit equivalent expressions +and record no skipped contract category. + +### Completion Gate + +Stage 2 is complete only when: + +- `WithFallbackProfileFS` is the sole new public declaration and has complete, + accurate GoDoc; +- nil filesystem and blank root inputs fail construction with + `ErrInvalidConfig`, and the last valid repeated fallback option wins; +- the assembled order is in-memory, ordinary configured, application + fallback, built-ins; +- existing ordinary source options still replace `Config.ProfileDir`; +- lookup falls through only on a missing ID and never after a matching + higher-layer failure; +- profiles remain whole values and loading remains lazy; +- inspection, preparation, prepared execution, and ordinary execution resolve + the same fallback definition through one runner repository; +- no built-in asset, profile format, public error identity, provider request, + or existing consumer behavior changed unintentionally; +- GoDoc and all affected canonical documents describe implemented behavior + without duplicating ownership; and +- all focused tests, vet, formatting, links, and whitespace checks pass. + +## Stage 3: Audit Compatibility And Validate The Repository + +### Objective + +Confirm that the implementation is complete, minimal, and consistent across +the public facade, internal boundaries, tests, and documentation, then mark +the temporary planning documents complete. + +### Implementation Prompt + +Implement only Stage 3 of `docs/roadmap/implementation.md` after Stage 2 +satisfies its completion gate. + +1. Search tracked Go and Markdown files for + `NewRepositoryWithPrimary`, `NewRepositoryWithDirectory`, profile source + precedence lists, and descriptions of built-in repository composition. + Remove stale references and correct only feature-owned contradictions. +2. Review `newProfileRepository` directly and confirm it has exactly four + possible layers in the required order, selects only one ordinary configured + source, and contains no profile field merging or eager I/O. +3. Review the public tests as a suite. Confirm that each distinct risk in Test + Ownership is protected once, generic parser and overlay cases remain with + `internal/profile`, and no test depends on private helper shape. +4. Confirm that `internal/profile/builtin/assets/`, external wire behavior, + backend configuration, credential resolution, public result shapes, and + stable JSON tags have no feature-related changes. +5. Follow every added or changed Markdown link and confirm that its target and + relevant heading exist. Verify that exact API details live in GoDoc, exact + profile format and precedence details live in `docs/formats.md`, consumer + guidance remains task-oriented, and internal documents describe only + implementation responsibility. +6. Run the complete validation sequence below and repair only in-scope + failures. +7. After every check passes, change the status of + `fallback-profiles.md` and this document to `Complete`. Do not delete or + retire either roadmap; retirement is a separate maintainer action. +8. Re-run `git diff --check`, inspect `git status --short`, and review the full + diff while distinguishing pre-existing user changes from this feature. + +Do not add release notes, change versions, or create a commit, tag, push, or +release during this stage. ### Full Validation Run from the repository root: ```sh -gofmt -w internal/defaults/defaults.go types.go engine_test.go \ - internal/usecase/runner_test.go +gofmt -w engine.go engine_test.go public_contract_test.go \ + internal/profile/builtin/repository.go \ + internal/profile/builtin/repository_test.go gofmt -l $(git ls-files '*.go') go test ./... go test -race ./... @@ -345,34 +469,34 @@ offline and require no real credential or provider. Inspect the final diff and confirm: - only this feature's files and pre-existing user changes are present; -- no built-in profile asset, public declaration shape, stable JSON tag, - credential rule, workspace file, local module replacement, generated binary, - or unrelated formatting changed; -- the removed provider-default constants have no remaining references; -- the provider omission policy is implemented by resolution plus the existing - generic serializer, not by a `top_p` transport special case; -- the optional-parameter roadmap and this plan are complete while the fallback - roadmap remains selected; and +- no built-in asset, public value shape, stable JSON tag, provider payload, + workspace file, local module replacement, generated binary, or unrelated + formatting changed; +- deleted built-in composition helpers have no remaining references; +- the fallback option reuses the ordinary FS repository and generic overlay; +- the root constructs one repository used by every resolution workflow; +- the feature roadmap and this plan are both complete; and - no commit, tag, push, or release was created. ### Completion Gate The implementation is complete only when: -- every Stage 1 gate remains satisfied; +- every Stage 1 and Stage 2 gate remains satisfied; - the ordinary and race-enabled suites pass; - vet, build, formatting, the maintained offline example, Markdown links, and whitespace checks pass; -- public metadata, injected-client presence, profile inheritance, and outbound - omission semantics are mutually consistent; -- explicit built-in and consumer profile values retain their behavior; -- both feature-specific roadmap statuses are `Complete`; -- `fallback-profiles.md` remains unchanged and selected for later work; and +- the four-layer precedence and not-found-only fallthrough are consistent in + code, GoDoc, public tests, format reference, consumer guidance, and internal + documentation; +- engines without `WithFallbackProfileFS` retain their previous behavior; +- the public surface contains no speculative companion API or provenance; +- both temporary roadmap statuses are `Complete`; and - the repository is ready for maintainer review without a commit or release having been created by this plan. ## Open Questions -None. The feature roadmap and fixed design above fully specify the behavior, -compatibility boundary, implementation, documentation ownership, and test -strategy. +None. The feature roadmap and fixed design above fully specify the public API, +repository composition, error and validation behavior, compatibility boundary, +documentation ownership, test strategy, and staged implementation sequence. diff --git a/engine.go b/engine.go index dc899b4..f5fcaa7 100644 --- a/engine.go +++ b/engine.go @@ -350,13 +350,7 @@ func NewEngine(cfg Config, opts ...Option) (*Engine, error) { promptDefs = promptdef.NewFilesystemRepository(cfg.PromptDir) } - profiles := builtin.NewRepositoryWithDirectory(cfg.ProfileDir) - if options.profileSource { - profiles = builtin.NewRepositoryWithPrimary(options.profiles) - } - if options.memorySource { - profiles = profile.NewOverlayRepository(options.memoryProfiles, profiles) - } + profiles := newProfileRepository(cfg.ProfileDir, options) backendRegistry, err := backend.NewRegistry(options.backends) if err != nil { @@ -409,6 +403,22 @@ func NewEngine(cfg Config, opts ...Option) (*Engine, error) { }, nil } +func newProfileRepository(profileDir string, options engineOptions) profile.Repository { + repository := builtin.NewRepository() + + if options.profileSource { + repository = profile.NewOverlayRepository(options.profiles, repository) + } else if strings.TrimSpace(profileDir) != "" { + repository = profile.NewOverlayRepository(profile.NewFilesystemRepository(profileDir), repository) + } + + if options.memorySource { + repository = profile.NewOverlayRepository(options.memoryProfiles, repository) + } + + return repository +} + func fileSource(name string) (fs.FS, string, error) { cleanName := strings.TrimSpace(name) if cleanName == "" { @@ -482,7 +492,7 @@ func (e *Engine) InspectPrompt( // InspectProfile trims surrounding whitespace from profileID and looks up the // resulting nonblank ID exactly and case-sensitively through the engine's // ordinary in-memory, configured-source, and built-in profile precedence. It -// applies framework defaults, the selected backend, and then the selected +// applies the framework timeout baseline, selected backend, and then selected // profile to EffectiveModelParams without a request override. BackendID is // empty for an endpoint-only profile. // diff --git a/internal/profile/builtin/repository.go b/internal/profile/builtin/repository.go index 827504f..aaffb97 100644 --- a/internal/profile/builtin/repository.go +++ b/internal/profile/builtin/repository.go @@ -2,7 +2,6 @@ package builtin import ( "embed" - "strings" "gitea.maximumdirect.net/eric/promptkit/internal/profile" ) @@ -15,17 +14,3 @@ var assets embed.FS func NewRepository() profile.Repository { return profile.NewFSRepository(assets, assetRoot) } - -func NewRepositoryWithPrimary(primary profile.Repository) profile.Repository { - if primary == nil { - return NewRepository() - } - return profile.NewOverlayRepository(primary, NewRepository()) -} - -func NewRepositoryWithDirectory(dir string) profile.Repository { - if strings.TrimSpace(dir) == "" { - return NewRepository() - } - return NewRepositoryWithPrimary(profile.NewFilesystemRepository(dir)) -} diff --git a/internal/profile/builtin/repository_test.go b/internal/profile/builtin/repository_test.go index 2cbf578..6293129 100644 --- a/internal/profile/builtin/repository_test.go +++ b/internal/profile/builtin/repository_test.go @@ -2,14 +2,11 @@ package builtin import ( "context" - "errors" "io/fs" "strings" "testing" "gitea.maximumdirect.net/eric/promptkit/internal/backend" - "gitea.maximumdirect.net/eric/promptkit/internal/domain" - "gitea.maximumdirect.net/eric/promptkit/internal/profile" "gopkg.in/yaml.v3" ) @@ -91,53 +88,3 @@ func loadBuiltInProfileIDs(t *testing.T) map[string]string { } return ids } - -func TestRepositoryWithPrimaryUsesPrimaryBeforeBuiltIns(t *testing.T) { - repo := NewRepositoryWithPrimary(staticProfileRepo{ - profiles: map[string]string{"mistral-small-3": "custom-model"}, - }) - - p, err := repo.GetProfile(context.Background(), "mistral-small-3") - if err != nil { - t.Fatalf("expected profile to load, got %v", err) - } - if p.Model != "custom-model" { - t.Fatalf("expected primary profile to override built-in, got %+v", p) - } -} - -func TestRepositoryWithPrimaryFallsBackToBuiltIns(t *testing.T) { - repo := NewRepositoryWithPrimary(staticProfileRepo{}) - - p, err := repo.GetProfile(context.Background(), "mistral-small-3") - if err != nil { - t.Fatalf("expected built-in profile to load, got %v", err) - } - if p.ID != "mistral-small-3" { - t.Fatalf("unexpected profile: %+v", p) - } -} - -func TestRepositoryWithPrimaryDoesNotFallBackAfterPrimaryError(t *testing.T) { - repo := NewRepositoryWithPrimary(staticProfileRepo{err: profile.ErrInvalidProfile}) - - _, err := repo.GetProfile(context.Background(), "mistral-small-3") - if !errors.Is(err, profile.ErrInvalidProfile) { - t.Fatalf("expected primary error, got %v", err) - } -} - -type staticProfileRepo struct { - profiles map[string]string - err error -} - -func (r staticProfileRepo) GetProfile(_ context.Context, id string) (*domain.ExecutionProfile, error) { - if r.err != nil { - return nil, r.err - } - if model, ok := r.profiles[id]; ok { - return &domain.ExecutionProfile{ID: id, Endpoint: "http://primary/v1", Model: model}, nil - } - return nil, profile.ErrProfileNotFound -} diff --git a/types.go b/types.go index 1049187..4d3288b 100644 --- a/types.go +++ b/types.go @@ -440,11 +440,12 @@ type ExecutionTargetOverride struct { // use profile YAML api_key_env with file and FS profile sources. Profile has no // stable JSON representation. // -// WithProfiles validates and copies Profile values during NewEngine. Optional -// numeric zero, blank strings, and an empty ExtraParams map inherit -// lower-precedence values. An optional provider control that remains zero is -// unspecified; use ExecutionTargetOverride pointer fields to request an -// explicit numeric zero. +// WithProfiles validates and copies Profile values during NewEngine. Zero +// Temperature, MaxTokens, and TopP values and blank ServiceTier and +// ReasoningEffort values leave those provider controls unspecified. A zero +// TimeoutSeconds retains the framework deadline, while an empty ExtraParams map +// inherits backend request defaults. Use ExecutionTargetOverride pointer fields +// to request an explicit numeric zero. type Profile struct { // ID is the required non-blank profile identifier. WithProfiles trims it. ID string @@ -458,23 +459,19 @@ type Profile struct { Endpoint string // Model is the required non-blank provider model identifier. Model string - // Temperature is from 0 through 2. Zero inherits a lower-precedence value - // and otherwise leaves the provider control unspecified. + // Temperature is from 0 through 2. Zero leaves the provider control + // unspecified. Temperature float64 - // MaxTokens is non-negative. Zero inherits a lower-precedence value and - // otherwise leaves the provider control unspecified. + // MaxTokens is non-negative. Zero leaves the provider control unspecified. MaxTokens int - // TopP is from 0 through 1. Zero inherits a lower-precedence value and - // otherwise leaves the provider control unspecified rather than selecting an - // explicit zero. + // TopP is from 0 through 1. Zero leaves the provider control unspecified + // rather than selecting an explicit zero. TopP float64 - // TimeoutSeconds is non-negative. Zero inherits a lower-precedence value and - // otherwise the framework deadline. + // TimeoutSeconds is non-negative. Zero retains the framework deadline. TimeoutSeconds int - // ServiceTier is optional; a blank value inherits a lower-precedence value. + // ServiceTier is optional; a blank value leaves it unspecified. ServiceTier string - // ReasoningEffort is optional; a blank value inherits a lower-precedence - // value. + // ReasoningEffort is optional; a blank value leaves it unspecified. ReasoningEffort string // APIKeyRequired clears a backend's inherited API-key environment name and // requires a non-blank RunRequest.APIKey unless the request explicitly