127 lines
6.1 KiB
Markdown
127 lines
6.1 KiB
Markdown
# ConfigKit Adoption Review Findings
|
||
|
||
Review date: 2026-10-03. Reviewed implementation commits `2b847ac` through
|
||
`66c55b7` against the [feature roadmap](configkit.md),
|
||
[implementation plan](implementation.md), and accepted
|
||
[configuration-boundary ADR](../adr/0001-configuration-boundary.md).
|
||
|
||
## Summary
|
||
|
||
The principal architecture is in place: one ConfigKit load assembles backend,
|
||
profile, and prompt configuration; PromptKit validates effective values, captures
|
||
resources/templates, and builds execution-owned indexes. Catalog preflight and
|
||
legacy profile resolution are removed. Operator inspection delegates to the
|
||
retained ConfigKit result separately from ordinary execution.
|
||
|
||
Both findings have been addressed. The original review evidence is retained
|
||
below; the resolution section records the changes and regression coverage.
|
||
|
||
## 1. P2 — Message Normalization Loses Content-File Resource Origins
|
||
|
||
Location: `internal/configuration/prompts.go`, particularly `resolvePrompts`
|
||
lines 69–99 and `readContent`'s effective-origin lookup.
|
||
|
||
When a role or cache-control value needs normalization, `resolvePrompts` records
|
||
a normalization operation for the entire `messages` sequence in addition to the
|
||
field-level operations. The resulting metadata no longer provides the source
|
||
location needed by `readContent` for an unchanged `content_file`. Construction
|
||
then rejects an otherwise valid prompt.
|
||
|
||
Confirmed through the public API using FilesystemInputs over an fs.FS containing
|
||
this document and a readable `body.txt`:
|
||
|
||
```yaml
|
||
prompts:
|
||
p:
|
||
v1:
|
||
messages:
|
||
- role: " USER "
|
||
content_file: body.txt
|
||
output:
|
||
format: text
|
||
validation_mode: none
|
||
```
|
||
|
||
NewEngine fails with
|
||
`/prompts/p/v1/messages/0/content_file: content_file needs one contributing source with a filesystem context`.
|
||
Changing only the role to `user` succeeds. Role trimming/lowercasing and
|
||
source-relative message capture are both intended capabilities; requiring
|
||
already-normalized metadata is an accidental restriction.
|
||
|
||
Required outcome:
|
||
|
||
- Preserve ConfigKit's effective resource context while normalizing neighboring
|
||
message metadata. Review the broad sequence normalization record and how the
|
||
final input origin is retained across domain normalization.
|
||
- Use effective handoff metadata/context, not historical traversal, raw-source
|
||
inspection, source rereading, or an invented fallback directory.
|
||
- Add focused regressions combining file-backed content with role and
|
||
cache-control normalization, including inherited/imported content. Existing
|
||
tests cover normalization and resource capture separately but miss this
|
||
interaction. Verify inspection remains accurate for untouched content fields.
|
||
|
||
## 2. P2 — Profiles Gain An Unrequested Environment-Name Restriction
|
||
|
||
Location: `internal/configuration/load.go`, profile validation at lines 278–279,
|
||
using the package's `envName` regular expression.
|
||
|
||
Profile `api_key_env` values now must match `[A-Za-z_][A-Za-z0-9_]*`. The previous
|
||
profile path accepted environment lookup names without this shell-identifier
|
||
restriction; the regular expression previously governed backend registration.
|
||
The refactor extends it to profiles even though arbitrary environment names
|
||
supported by the host remain usable through Go's environment lookup and the
|
||
request override path.
|
||
|
||
Confirmed through NewEngine with an otherwise valid endpoint/model profile:
|
||
`api_key_env: NORMAL_KEY` succeeds, while `api_key_env: service.api-key` fails
|
||
construction. The latter is a usable environment name on the reviewed Linux
|
||
platform. This restriction is unrelated to configuration layering, ConfigKit
|
||
typing, or optional credential availability.
|
||
|
||
The implementation plan's reference to existing naming rules is not evidence
|
||
of an express user decision to introduce this new profile restriction. The
|
||
requested compatibility break permits adopting ConfigKit semantics; it does
|
||
not itself authorize additional unrelated name constraints.
|
||
|
||
Required outcome:
|
||
|
||
- Remove the newly introduced profile shell-identifier check, preserving the
|
||
established trimming, explicit clearing, optional environment lookup, and
|
||
direct-key precedence behavior.
|
||
- Add a public-path regression for a supported non-shell-identifier environment
|
||
name and for its unset/empty optional behavior, with controlled test environment
|
||
state and a synthetic credential. Keep any prerequisite validation limited to
|
||
actual supported environment lookup requirements.
|
||
- Correct the plan's profile-field guidance so later work does not reintroduce
|
||
this restriction. Treat any broader change to existing backend naming policy
|
||
separately rather than silently extending or weakening it in this fix.
|
||
|
||
## Validation Performed
|
||
|
||
- `go test ./...` passed on Go 1.26.8.
|
||
- `GOTOOLCHAIN=go1.25.5 go test ./...` passed on the declared minimum toolchain.
|
||
- `go test -race ./...` passed.
|
||
- Separate offline public-API reproductions confirmed both findings. Missing
|
||
message/session variables also remained runtime errors in targeted checks.
|
||
|
||
The passing suite does not cover the two failing cases above. No provider
|
||
requests were made. Beyond these findings, the reviewed implementation did not
|
||
show another actionable ownership-boundary violation or strengthened guarantee.
|
||
|
||
## Resolution
|
||
|
||
1. Message role/cache-control normalization now occurs on execution-owned copies
|
||
during materialization. ConfigKit's captured message sequence and its origins
|
||
remain intact. The imported/inherited content regression now includes role
|
||
and cache-control normalization and asserts the unchanged field's source in
|
||
inspection metadata. This also removes duplicate cache-control normalization.
|
||
2. Removed the profile environment-name regular expression check, leaving backend
|
||
validation unchanged. A public engine/HTTP-transport regression covers a
|
||
non-shell environment name, unset/empty/whitespace values, explicit clearing,
|
||
trimming, and direct-key precedence. The implementation plan now states this
|
||
distinction explicitly.
|
||
|
||
Both regressions failed against the reviewed implementation before the fixes.
|
||
After remediation, the full ordinary and race-enabled suites, affected packages
|
||
on Go 1.25.5, `go vet ./...`, and documentation/whitespace checks passed.
|