Record ConfigKit adoption review findings
This commit is contained in:
108
docs/roadmap/remediation.md
Normal file
108
docs/roadmap/remediation.md
Normal file
@@ -0,0 +1,108 @@
|
||||
# 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.
|
||||
|
||||
Two findings require remediation. This review changes no implementation code.
|
||||
|
||||
## 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.
|
||||
Reference in New Issue
Block a user