Record configuration comparison review findings
This commit is contained in:
155
docs/roadmap/remediation.md
Normal file
155
docs/roadmap/remediation.md
Normal file
@@ -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.
|
||||
Reference in New Issue
Block a user