From b9d5ea4b1b2bb869c1bf1165c0e8baa730cd8693 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Mon, 12 Oct 2026 14:02:26 +0000 Subject: [PATCH] Record PromptKit implementation review findings --- docs/roadmap/remediation.md | 126 ++++++++++++++++++++++++++++++++++-- 1 file changed, 121 insertions(+), 5 deletions(-) diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 6379b4b..80db1b2 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,9 +1,9 @@ -# Configuration comparison review remediation +# CommandKit implementation review remediation -Status: all four findings complete. This document tracks required -follow-up work and its completion evidence. +Status: R-1 through R-4 are complete; R-5 and R-6 are open. This document +tracks required follow-up work and its completion evidence. -## Review baseline and scope +## ConfigKit review baseline and scope Reviewed the ConfigKit adoption/comparison sprint, commits `c883c64` through `28aa3b0`, against the [feature roadmap](configkit.md), completed @@ -18,7 +18,7 @@ validation evidence. No Go tests or benchmarks were run during this review. The implementation plan records the sprint's workspace and isolated-module validation; that is prior evidence, not a new test run. -## Architecture assessment +## ConfigKit architecture assessment The requested architectural components are present: @@ -189,3 +189,119 @@ evidence. The feature roadmap retains the limitations of local replacements for unpublished CommandKit dependencies and treats publication separately. The implementation and deferred records remain consistent with this status. Relevant local links, fragments, and documentation whitespace checks passed. + +## PromptKit review baseline and architecture + +Reviewed commits `ec4eb9d` through `f8f8d27`, relative to the planning baseline +`d825538`, against the accepted [PromptKit feature roadmap](promptkit.md), +[implementation record](implementation.md), applicable ADRs, and +[architecture policy](../policy/architecture.md). Evidence includes current +source, recent diffs, tests, canonical documentation, distribution metadata, +and fresh workspace-off module/import enumeration. Go tests and benchmarks were +not rerun; recorded sprint test results are prior evidence. + +The runtime architecture matches the selected target: + +| Responsibility | Current evidence | +| --- | --- | +| Core retains declarations, installation, dispatch, bindings, help/version, and outcomes without optional owning-library dependencies. | Unchanged core/backend code and manifest; fresh isolated graph/import checks, including tests, contain neither integration nor ConfigKit/PromptKit. | +| ckconfig retains configuration command coordination, paired typed inputs, shared state, YAML comparison rendering, and delivery. | Unchanged ckconfig implementation; its isolated graph/import checks contain no ckprompt/PromptKit. | +| ckprompt adds native PromptKit comparison through the existing scope contract. | [ConfigScope](../../integrations/ckprompt/adapters.go) loads reached sides in order, retains diagnostics and supported error identity, and calls captured inspection `Diff`; no second comparator or renderer. | +| ckprompt exposes independently optional catalogs and named inspections. | [Suite and FromEngine](../../integrations/ckprompt/adapters.go) supply the accepted callbacks; [command assembly](../../integrations/ckprompt/command.go) omits absent groups, preserves retained errors, and uses core typed bindings. | +| Catalog reports preserve provider identities/order and selected invocation behavior. | Private string-field YAML reports, nil/empty sequence handling, shared suite input preparation, and existing buffered delivery in [command.go](../../integrations/ckprompt/command.go); assembled command tests cover their public behavior. | +| Domain selectors, loaders, caches, callback effects, and process exit remain application-owned. | [Composed consumer](../../integrations/ckprompt/examples/composed/main.go), [consumer guide](../consumers/guide.md#promptkit-configuration-and-definitions), and unchanged core ownership contracts. | + +No missing runtime command or adapter component, new core/backend abstraction, +dependency reversal, or additional production restriction was identified. +Requirement inspection remains deliberately deferred. YAML-only catalogs, +explicit empty inventories, capability narrowing, nil-factory zero values, +and caller-owned aliases/caches are accepted choices requiring no remediation. + +The distribution validation gap and incidental test invariant below do require +follow-up. The recorded inability to construct successful native engine warnings +or force a native Diff failure through PromptKit v0.14.0's public API is an +acknowledged validation limitation, not a reason to add test-only public hooks. + +## R-5: Validate the committed ckprompt manifest without an extra test-only YAML API + +**Status:** Open. **Priority:** Medium. **Category:** Distribution validation gap / +dependency consistency. + +**Evidence:** [comparison_composition_test.go](../../integrations/ckprompt/comparison_composition_test.go) +imports `gopkg.in/yaml.v3` at line 17. This import was introduced by `f7b1d2c`; +the [ckprompt manifest](../../integrations/ckprompt/go.mod) declares the existing +`go.yaml.in/yaml/v4` dependency instead. The other ckprompt reports and tests use +that existing API. The new comparison test needs only ordinary YAML decoding, +with no demonstrated need for a separate parser version. + +Fresh workspace-off enumeration from ckprompt, using disposable copies of the +committed `go.mod`/`go.sum` with only local replacements for unpublished core +and ckconfig prerequisites, fails: + +```text +go list -modfile= -mod=readonly -deps -test ./... +go: updates to go.mod needed, disabled by -mod=readonly; to update it: + go mod tidy +``` + +The environment was `GOWORK=off GOTOOLCHAIN=local`. Tidying only the disposable +files adds a direct `gopkg.in/yaml.v3 v3.0.1` requirement and a `gopkg.in/check.v1` +checksum, after which the enumeration succeeds. No repository manifest was +changed during this review. Core and ckconfig pass the corresponding checks +without that extra tidy. Listing the ckprompt module graph alone succeeds, so +that narrower check does not establish package/test graph completeness. + +[Stage 12's completion evidence](implementation.md#stage-12-validate-the-complete-end-state-and-finish) +records successful read-only isolated tests and import enumeration. Those results +do not demonstrate that the committed manifest supports the same checks with +only prerequisite replacements: a tidied disposable manifest is different +distribution metadata. + +**Required work:** + +- Use the already selected `go.yaml.in/yaml/v4` API in the new comparison test. + It supports the decoding used there and avoids an unnecessary second direct + YAML API in CommandKit's own integration. This is a repair to this import, + not a prohibition on upstream dependencies using their own YAML versions. +- Reconcile legitimate requirements and real sums as needed. Keep unpublished + CommandKit replacements out of distributed files. Do not remove legitimate + transitive YAML v3 notices or metadata merely because this direct import goes + away. +- Repeat isolated checks from fresh copies of the final committed manifests, + adding only the unpublished CommandKit replacements. Run import enumeration + and the relevant suite with `-mod=readonly`; do not silently tidy validation + inputs into a graph that is absent from distribution. +- Record corrected completion evidence, distinguishing committed-metadata + compatibility from local replacement/tag availability limitations. + +**Completion evidence:** Fresh copied manifests plus only prerequisite +replacements pass `go list -deps -test ./...` and the ckprompt suite in +workspace-off read-only mode. The test uses the existing YAML API, and any +necessary metadata changes are tracked with accurate validation evidence. + +## R-6: Test comparison capability membership rather than its slice position + +**Status:** Open. **Priority:** Low. **Category:** Inadvertently strengthened +test invariant. + +**Evidence:** [adapters_test.go](../../integrations/ckprompt/adapters_test.go), +`TestConfigScopeCompareUsesNativeDiffAndLoadsBothEqualSelections`, line 165, +requires `scope.Operations[4] == ckconfig.Diff`. The accepted requirement is that +the bundled scope supports its ordinary operations and comparison; it does not +assign Diff a public position in the returned declaration slice. + +[captureSuite and normalizeOperations](../../integrations/ckconfig/family.go) +already normalize declared capability membership into the installed order. +Reordering the same operations returned by the adapter can therefore preserve +installed commands and discovery output while failing this assertion. This +turns an incidental adapter layout into a restriction on valid refactoring. + +**Required work:** Check that `Compare` is supplied and the required capabilities +are present without depending on their positions. Retain native-result, +reached-side loader, input, and shared-state assertions. This does not remove +the selected left-before-right loading requirement or any expressly selected +operator ordering contract. + +**Completion evidence:** The test still rejects missing comparison wiring or +capabilities, but accepts an equivalent ordering of the same declared operations. +It introduces no new public ordering guarantee or unrelated runtime changes.