commandkit/docs/roadmap/remediation.md

CommandKit implementation review remediation

Status: all six findings are complete. This document tracks required follow-up work and its completion evidence.

ConfigKit review baseline and scope

Reviewed the ConfigKit adoption/comparison sprint, commits c883c64 through 28aa3b0, against the feature roadmap, completed implementation plan, accepted ADRs, and architecture policy. The review covered changed production code, tests, maintained examples, current documentation, module metadata, and attribution/release-note changes. Findings below concern the current source at 28aa3b0.

This was a static review using source, graph relationships, diffs, and committed 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.

ConfigKit architecture assessment

The requested architectural components are present:

Responsibility	Implementation evidence
Core remains independent of optional integrations and owning libraries; Cobra/pflag remain behind the internal adapter.	Core manifest, internal overview, and unchanged core/backend implementation during this sprint.
ckconfig captures independent comparison capabilities and validates only the providers required by declared operations.	Scope contracts and family assembly.
Comparison uses inherited selections, separately prepared typed inputs, and one shared state after both checks.	Input preparation and operation execution.
The typed ConfigKit adapter retains its configuration type, invokes the supplied loader for reached sides, and calls native Diff with retained diagnostics.	ConfigKit adapter.
ckconfig presents native safe right-side regions in the selected YAML report and uses existing outcomes and delivery handling.	Renderer and comparison execution.
ckprompt adopts the shared dependency without declaring PromptKit comparison; caches and domain behavior remain application-owned.	ckprompt manifest, engine scope adapter, and composed consumer.

No missing architectural component or production restriction beyond the selected scope was identified. The strengthened claims found in documentation and tests are recorded below. The accepted exceptional whole-root empty/null ambiguity, YAML-only format, single-scope comparison, nil-factory zero values, and caller-owned aliases/caches are deliberate choices and require no remediation.

R-1: Avoid repeated full scans when assembling wide report mappings

Status: Complete. Priority: Medium. Category: Code quality / efficiency.

Evidence: diff_report.go, placeYAMLValue and mappingKeyIndex (lines 59 and 98 at the review baseline). Every inserted path component scans the existing YAML mapping's key/value content. For N distinct added or changed sibling keys, locating those keys performs 0 + 1 + ... + (N - 1) key comparisons. This introduces quadratic lookup work for legitimate wide configuration maps, in addition to the necessary snapshot parsing and encoding. Shared ancestors can incur the same repeated scans.

Required work: Use a small report-local lookup index for assembled mapping nodes, or an equivalently simple approach that avoids scanning all previously inserted siblings for each placement. Keep YAML node content in insertion order; preserve native region boundaries, string keys, root replacement, and current assembly failure handling. Do not introduce a global cache, public renderer API, input limits, or a new performance guarantee.

Completion evidence: Review shows sibling lookup no longer repeatedly scans the accumulated mapping. Existing semantic-path, YAML-type, ordering, and native safe-value behavior remains intact. Any additional measurement should be focused on this lookup cost; a numerical performance gate is not required.

Completed 2026-10-12: A private, report-local yamlValuePlacer now indexes each encountered mapping once and updates that index as keys are appended. Subsequent placements reuse key positions while YAML content keeps insertion order. Root values, native regions, and incompatible-ancestor failures retain their handling. Existing helper tests use the same placer across placements. go test ./... passed in integrations/ckconfig, including the simple consumer; the diff and whitespace review passed. No public API, limits, global cache, benchmark, or performance gate was added.

R-2: Scope CLI guarantees to the actual provider and delivery boundaries

Status: Complete. Priority: Medium. Category: Inadvertently strengthened contracts / documentation accuracy.

Evidence: The comparison CLI reference says selections are independently loaded, both reached sides are loaded, and comparison diagnostics identify a side and source legend (lines 235, 243, and 283 at the review baseline). Those loading and diagnostic adaptation behaviors belong to FromConfigKit/AppCommand. A custom Scope.Compare callback receives the prepared pair and shared state; its loading, caching, and diagnostic effects remain caller-owned under ADR 0008 and ADR 0012. runConfigDiff forwards the callback's diagnostics without adding side headings or enforcing its loading strategy.

The same CLI section says cancellation leaves report output empty (line 286). The implementation checks cancellation before delivery, then calls the supplied writer. Cancellation during that call cannot retract bytes already delivered and may produce partial output or a successful completed write. The selected contract promises boundary checks, not atomic delivery or interruption inside synchronous collaborators.

Required work:

• Describe the generic operation as preparing two selections and invoking the declared pair provider. Attribute loader calls, their left/right ordering, and native diagnostic side/source adaptation to the bundled ConfigKit adapter. Equal selections alone must not cause CommandKit to skip the provider; this does not guarantee a physical reload or prohibit application caches.
• State that preparation failures and cancellation observed before delivery starts leave CommandKit's report output empty. Once writing starts, the existing write-error/short-write and incomplete-report handling applies; caller writers and callbacks retain their own effects.

Completion evidence: The CLI reference accurately describes both bundled adapters and custom pair providers, and makes no transactional cancellation or writer-interruption guarantee. Correct the prose rather than expanding runtime enforcement or imposing new requirements on consumers.

Completed 2026-10-12: The CLI reference now describes preparation and provider invocation generally, attributes ordered loader calls and native diagnostic adaptation to the bundled ConfigKit adapters, and preserves custom provider and application cache ownership. Empty report output is promised only for report preparation failure or cancellation observed before delivery starts. Writer interruption and rollback are not promised. The prose was checked against the adapter and report execution paths; local links and whitespace checks passed. No runtime enforcement or consumer requirement was added.

R-3: Make preparation tests reject failure without freezing private storage

Status: Complete. Priority: Medium. Category: Test quality / inadvertently strengthened implementation invariants.

Evidence: In comparison_preparation_test.go:

• TestComparisonPreparesEqualSelectionsWithSeparateTypedInputs rejects only OutcomeUsage (line 96). An operational failure after the observed input/state work can pass, despite this being a valid successful comparison scenario.
• TestComparisonPlanStoresPairSeparatelyFromOrdinaryInputs (line 114) requires plan.comparison and an empty plan.scopes, and reads the request directly from that representation. A correct internal refactor storing the same checked pair differently fails this test without violating the public lifecycle or provider contract. The implementation plan explicitly leaves private storage and equivalent refactoring as implementation choices.

Required work: Require OutcomeSuccess in the valid equal-selection case. Replace or consolidate the storage-layout test with observations through the assembled comparison callback: correct side selections and typed inputs, both checks before shared state/provider execution, and ordinary acquisition unused. Retain the selected presence, conversion, collection ownership, zero-value, and failure-classification protection. Do not add a blanket ban on private helper tests or require arbitrary factory-returned objects to be distinct.

Completion evidence: A valid comparison returning an operational failure is rejected by the test. Tests protect the paired provider/lifecycle contract without requiring a particular private plan layout. Reuse existing behavioral coverage where it already proves the relevant fact.

Completed 2026-10-12: The valid equal-selection test now requires OutcomeSuccess, so an operational failure cannot pass. The storage-layout test was replaced with an assembled invocation that observes the provider request, correlates each input with its checked selection, verifies both checks precede shared state and provider execution, and verifies ordinary acquisition is unused. It does not inspect the private plan or assign particular factory IDs to sides. Existing presence, conversion, collection ownership, zero-value, alias, and failure-classification cases remain. go test ./... passed in integrations/ckconfig, including the simple consumer; formatting, diff, and whitespace review passed.

R-4: Reconcile the feature roadmap's completion status

Status: Complete. Priority: Low. Category: Documentation divergence.

Evidence: configkit.md still says distribution reconciliation and final validation remain outstanding in its opening status and Implementation record (lines 4 and 350 at the review baseline). The implementation plan marks both those stages complete and records their evidence. The feature roadmap therefore contradicts the completed sprint and can prompt agents to restart completed work.

Required work: Mark the selected feature end state complete in both passages and link to the completed implementation evidence. Preserve the distinction between implementation validation and release publication, including the recorded limitations around unpublished CommandKit dependency versions.

Completion evidence: Neither passage assigns completed reconciliation or validation work as pending. The feature roadmap, implementation record, and deferred capabilities agree on what is implemented and what remains future work.

Completed 2026-10-12: Both stale passages now record the feature's completed end state and link to the distribution reconciliation and final validation 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, implementation record, applicable ADRs, and architecture policy. 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 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 supply the accepted callbacks; command assembly 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; assembled command tests cover their public behavior.
Domain selectors, loaders, caches, callback effects, and process exit remain application-owned.	Composed consumer, consumer guide, 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: Complete. Priority: Medium. Category: Distribution validation gap / dependency consistency.

Evidence: At the review baseline, comparison_composition_test.go imported gopkg.in/yaml.v3 at line 17. This import was introduced by f7b1d2c; the ckprompt manifest 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:

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 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.

Completed 2026-10-12: The comparison consumer test now uses the already declared go.yaml.in/yaml/v4 API. Fresh copies of the distributed ckprompt go.mod and go.sum, augmented only with replacements for unpublished core and ckconfig prerequisites, passed go list -m all, go list -deps -test ./..., and go test -count=1 ./... with GOWORK=off GOTOOLCHAIN=local and -mod=readonly on Go 1.26.8. The suite includes the composed consumer. Validation performed no tidy; both disposable files remained byte-for-byte unchanged after the checks. Distributed manifests, sums, and legitimate transitive YAML v3 notices needed no changes. Stage 12 now records this corrected committed-metadata evidence; local replacements establish source compatibility, not unpublished tag availability. Formatting and documentation checks passed.

R-6: Test comparison capability membership rather than its slice position

Status: Complete. Priority: Low. Category: Inadvertently strengthened test invariant.

Evidence: At the review baseline, adapters_test.go, TestConfigScopeCompareUsesNativeDiffAndLoadsBothEqualSelections, line 165, required 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 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.

Completed 2026-10-12: The adapter test now checks comparison wiring and membership of the five required capabilities using slices.Contains, without requiring a particular position or slice length. Native-result, side-input, shared-state, and ordered-loader assertions remain intact. Temporary Go overlays confirmed that the focused test accepts reordered capabilities and rejects missing Diff or a missing comparison provider. The full ckprompt suite, including its composed consumer, passed with GOWORK=off GOTOOLCHAIN=local, -mod=readonly, and -count=1, using fresh copied manifests with only local prerequisite replacements. Production code and module metadata were unchanged; the overlays were not retained. Formatting and documentation checks passed.