Add profile inheritance definition support
This commit is contained in:
@@ -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 <key>`;
|
||||
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 <credential>` 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.
|
||||
|
||||
Reference in New Issue
Block a user