base-layer/commandkit
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Record PromptKit implementation review findings

Browse Source
This commit is contained in:
Eric Rakestraw
2026-10-12 14:02:26 +00:00
parent f8f8d27e71
commit b9d5ea4b1b
1 changed files with 121 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files

126
docs/roadmap/remediation.md
View File

@@ -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=<disposable go.mod> -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.