diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..81b67ea --- /dev/null +++ b/docs/roadmap/remediation.md @@ -0,0 +1,155 @@ +# Configuration comparison review remediation + +Status: four open findings. This document records required follow-up work; it +does not implement the fixes. + +## Review baseline and scope + +Reviewed the ConfigKit adoption/comparison sprint, commits `c883c64` through +`28aa3b0`, against the [feature roadmap](configkit.md), completed +[implementation plan](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:** Open. **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. + +## R-2: Scope CLI guarantees to the actual provider and delivery boundaries + +**Status:** Open. **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. + +## R-3: Make preparation tests reject failure without freezing private storage + +**Status:** Open. **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. + +## R-4: Reconcile the feature roadmap's completion status + +**Status:** Open. **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](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.