# Configuration comparison review remediation

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

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

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