commandkit/docs/roadmap/remediation.md

# CommandKit implementation review remediation

Status: R-1 through R-5 are complete; R-6 is open. 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](configkit.md), completed
[implementation plan](https://gitea.maximumdirect.net/eric/CommandKit/src/commit/20e19b95694f7fdfd5c342ec4d543152986623dc/docs/roadmap/implementation.md), accepted ADRs, and
[architecture policy](../policy/architecture.md). 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](../../go.mod), [internal overview](../internal/overview.md), and unchanged core/backend implementation during this sprint. |
| ckconfig captures independent comparison capabilities and validates only the providers required by declared operations. | [Scope contracts](../../integrations/ckconfig/contracts.go) and [family assembly](../../integrations/ckconfig/family.go). |
| Comparison uses inherited selections, separately prepared typed inputs, and one shared state after both checks. | [Input preparation](../../integrations/ckconfig/input.go) and [operation execution](../../integrations/ckconfig/operations.go). |
| The typed ConfigKit adapter retains its configuration type, invokes the supplied loader for reached sides, and calls native Diff with retained diagnostics. | [ConfigKit adapter](../../integrations/ckconfig/configkit_adapter.go). |
| ckconfig presents native safe right-side regions in the selected YAML report and uses existing outcomes and delivery handling. | [Renderer](../../integrations/ckconfig/diff_report.go) and [comparison execution](../../integrations/ckconfig/diff.go). |
| ckprompt adopts the shared dependency without declaring PromptKit comparison; caches and domain behavior remain application-owned. | [ckprompt manifest](../../integrations/ckprompt/go.mod), [engine scope adapter](../../integrations/ckprompt/adapters.go), and [composed consumer](../../integrations/ckprompt/examples/composed/main.go). |

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](../../integrations/ckconfig/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](../cli.md#compare-configuration-selections)
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](../adr/0008-semantic-configuration-diff.md)
and [ADR 0012](../adr/0012-typed-configuration-comparison-pairs.md).
[runConfigDiff](../../integrations/ckconfig/diff.go) 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](../../integrations/ckconfig/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](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](https://gitea.maximumdirect.net/eric/CommandKit/src/commit/20e19b95694f7fdfd5c342ec4d543152986623dc/docs/roadmap/implementation.md#stage-12-reconcile-manifests-notices-and-pending-integration-release-notes)
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](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:** Complete. **Priority:** Medium. **Category:** Distribution validation gap /
dependency consistency.

**Evidence:** At the review baseline,
[comparison_composition_test.go](../../integrations/ckprompt/comparison_composition_test.go)
imported `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.

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