diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..21cbb0d --- /dev/null +++ b/docs/roadmap/remediation.md @@ -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.