commandkit/docs/roadmap/remediation.md

Shared reporting and PromptKit adoption remediation

Status: two open findings; R1–R5 complete. R1–R3 are from the architectural review of f83866e44a8a9066db52cbd2aa5cccde0d69a839. R4–R7 are from the algorithmic audit of f5b2d1f80c4720b762f3301c29d5050710c210d0; R4 is a correctness issue and R5–R7 are efficiency opportunities for consideration.

Review basis and boundaries

The review covers the implementation since 105bb74, against the final feature roadmap and implementation plan, now retired into Git history. The architecture policy, CLI reference, and public GoDoc govern current boundaries.

Core supplies command mechanics, typed input composition, CLI JSON parsing, shared report representation/rendering, and the existing invocation reporting and outcome lifecycle. ckconfig supplies declared scopes, independent comparison inputs, opt-in overrides, and configuration report content. ckprompt adapts released PromptKit v0.20.0 resources, preparation, namespaces, requirements, smoke evidence, and configuration inspection through those shared contracts. Applications retain loading and execution; upstream libraries retain domain semantics. Module requirements select released core and ckconfig v0.4.0, ConfigKit v1.1.0, and PromptKit v0.20.0.

All three independent module suites and all three aligned workspace suites passed with Go 1.27.0 against this implementation. Temporary external-consumer probes using local module replacements reproduced R1–R4. Those probes did not modify the repository's source or tests. R5–R7 follow from source-level complexity analysis; no real-world performance measurements were gathered. No additional strengthened invariant, prohibition, or guarantee was identified. These findings do not request stronger domain validation or new guarantees.

R1 — P2: Explain still uses a separate provider text renderer

Complete. A reusable template now renders the acquired projection, including scope, metadata history, origins, and a shared source legend. No provider text writer is called by the handler. Cross-format evidence and delivery-failure regressions and the independent ckconfig suite pass with Go 1.27.0.

Location: explain.go, runConfigExplanation, particularly lines 160–169.

The handler prepares an explanationReport, but its text template is only a {{render .}} thunk. That function calls the acquired provider's WriteExplanation again and appends explicit/null lines. YAML/JSON serialize the prepared report instead. This preserves the old parallel rendering path inside a template instead of completing the shared handoff selected for the output inventory and Stages 7/23. Formatting still depends on the provider after the report content has been selected.

Observed consequence: an external provider returning a valid Explain result and an error from WriteExplanation succeeds with --format json but fails with status 1 under text/default formatting. The error comes from the separate provider writer, not the selected report data. The maintained simple consumer also demonstrates different selected evidence: JSON includes scope and metadata history/previous-origin records, while text calls the native writer and appends only current explicit/null metadata. Native text can introduce source labels that are absent from the handed-off projection.

Required change: render text from the same acquired projection as the other formats. Prepare a reusable ordinary template and project any selected source labels or metadata needed by it before handoff. Stop invoking WriteExplanation from the template. Keep upstream safe evidence and meaning; no second loading or explanation algorithm, custom template language, or per-format report tree.

Verification: use an explanation provider whose Explain result is valid and whose WriteExplanation records or rejects calls. Confirm the standard handler uses its selected report value in text, YAML, and JSON without calling that writer. Check selected scope, current/historical metadata, and available provenance semantically across formats, plus ordinary presentation failures. Do not freeze incidental text layout.

R2 — P2: PromptKit configuration cancellation discards captured evidence

Complete. The loader handoff now explicitly distinguishes an unavailable inspection from a captured one and retains the latter on cancellation. ConfigScope exposes only its sources and findings on failure. Native-evidence, source-report, reached-scope validation, cancellation, and separate delivery-failure checks pass, as does the independent ckprompt suite with Go 1.27.0.

Location: adapters.go, loadPromptKitEngine lines 487–495 and ConfigScope.Acquire lines 276–287.

After the loader returns an engine, loadPromptKitEngine obtains a native ConfigurationInspection. If the context is then canceled, it returns a zero inspection together with the cancellation error. ConfigScope's error branch only supplies structured facets for a ConfigKit LoadError, so the inspection's already-captured source inventory and diagnostics are lost.

Observed consequence: a loader that returns a valid engine and cancels the invocation before returning yields context.Canceled with Sources and Findings both nil, even though direct InspectConfiguration on that engine supplies a source inventory. The probe retained one native source before adaptation and none afterward. Detailed validation therefore reports evidence as unavailable instead of the available native state; source listing cannot report the acquired inventory. This conflicts with OD-6's preservation of reached-scope evidence when cancellation stops acquisition. The validation handler's post-cancellation submission cannot recover data already discarded by its adapter.

Required change: retain an explicitly available native inspection through the acquisition error path and expose its safe Sources/Findings to ConfigScope. Keep cancellation as the operation failure, and keep effective configuration, metadata, and ordinary resource/request work unavailable after the failed acquisition. Preserve the distinction between unavailable and available-empty evidence. Do not resume canceled acquisition or add a blanket reporting guard.

Verification: return an engine from a loader that cancels the context after successful loading. Compare the adapter's retained sources/diagnostics with the native inspection, and exercise source reporting and detailed aggregate validation. Confirm context.Canceled identity, no unvisited-scope success, and separate presentation failure. Existing ordinary inspection cancellation behavior should continue to stop domain work.

R3 — P2: A native ConfigKit result accompanying an error loses evidence

Complete. Result-plus-error acquisition now retains sources, findings, and ordinary messages without exposing effective, explanation, or metadata facets. A matched LoadError still supplies the sole authoritative snapshot. Regression checks cover application/cancellation causes, absent versus empty findings, source and validation reports, warning delivery, and presentation failures. The independent ckconfig suite passes with Go 1.27.0.

Location: configkit_adapter.go, FromConfigKit.Acquire lines 35–47.

The adapter now handles err before examining result. When an application loader returns an acquired native Result plus a non-LoadError failure (for example, a post-load application check), it returns an empty View and drops the result's captured sources and diagnostics. The previous adapter inspected a nonnil result before handling the error. Restricting effective/metadata availability after failure should not also discard the safe evidence already supplied by a native result.

Observed consequence: a loader returning a successfully loaded native result with one source plus a sentinel application error preserves the error identity, but Sources and Findings are nil. Consequently config sources says inventory is unavailable and detailed validation cannot expose the available findings. This falls short of the selected failed-acquisition evidence behavior without requiring any additional loading or upstream inference.

Required change: when a nonnil native result accompanies such a failure, retain its source/diagnostic evidence and associated ordinary messages while returning the original error. Do not expose effective configuration or metadata as successful acquisition facets. A matching ConfigKit LoadError should continue to supply its authoritative failure snapshot without duplicating it with result evidence. Nil result plus a generic error may legitimately have no evidence.

Verification: cover result-plus-application-error and result-plus-cancellation with source reporting and detailed validation; compare retained facts with that result's native inspection data. Preserve original causes, available-empty versus unavailable findings, ordinary warnings, and separate presentation errors. Retain a matched LoadError case to prevent duplicate inventories/diagnostics.

Algorithmic audit scope

The algorithmic pass covers the new conversion/rendering, input composition, filtering, report projection, and adapter code. It excludes the explicitly selected JSON normalization and whole-report aggregation strategies, native upstream domain algorithms, unchanged routing algorithms, and the composed consumer's explicitly planned list cache with reflect.DeepEqual. Nested loops over disjoint child records are assessed against their total data/output size; nesting alone is not a finding. The ordering and filter semantics remain selected; their implementations below were not prescribed by the plan.

R4 — P2: YAML conversion reclassifies native strings as numbers

Complete. Decoding now honors resolved string tags; core YAML emission explicitly retains numeric tags for overflow tokens instead of relying on heuristic decoding. String values/keys and explicit large numbers round-trip through YAML and JSON. The independent core suite passes, and temporary native ConfigKit consumer probes pass for whole/focused display and detailed difference snapshots with Go 1.27.0. No optional-library dependency was added to core.

Location: report_value.go, yamlReportValue lines 106–118.

For an unquoted scalar resolved as !!str, the converter examines its spelling and can replace it with json.Number. That inference is incorrect for valid native safe output. ConfigKit v1.1.0 can emit the string 1e400 unquoted; the YAML library resolves that scalar as a string, and the captured typed configuration also contains a string. CommandKit instead renders a numeric JSON value. A native mapping with the string key 1e400 fails conversion with "YAML mapping key is not a string", turning an otherwise valid display into status 1. Explicitly tagged and quoted strings already work; the failure concerns native plain string scalars.

Reproduction: load a ConfigKit Mapping containing {"value": "1e400"} into map[string]string. Native WriteEffective emits value: 1e400; ordinary YAML decoding retains the string. config show --format json through FromConfigKit emits the JSON number 1e400. Repeating with {"1e400": "valid string key"} produces the mapping-key presentation error. These are confirmed external-consumer probes, not a hypothesized numeric edge. The shared decoder also serves focused display and difference snapshots.

Complexity and correction: scalar scanning is linear in its length; the problem is type correctness, not its running time. Preserve native resolved string tags rather than inferring numeric intent from spelling. Continue to preserve numeric precision when numeric evidence exists, such as !!int or !!float nodes and the core's JSON number representation. Retain explicit numeric tags in core-generated YAML when needed; the emitter can otherwise omit them for overflow tokens. Do not compensate by changing upstream loading or by reparsing mutable runtime configuration. Review tests that currently require numeric promotion of resolved plain strings; they cannot override the selected string-versus-number preservation contract.

Verification: add native-result boundary cases for 1e400 as a value and key, including whole/focused show and a difference snapshot. Keep quoted and explicit-string cases, ordinary numbers, and explicitly numeric large tokens. Assert type and value preservation across structured formats, without depending on quoting style.

R5 — P3: Validation filter membership rescans operator lists

Complete. Severity and code membership sets are built once per invocation and shared by all reached scopes. Membership work is now expected O(C+V+D), with O(C+V) storage, excluding string and existing projection costs. Expanded public command checks preserve match-any/match-both semantics, duplicates, unknown codes, ordering, evidence states, source references, and failure outcomes. The independent ckconfig suite passes with Go 1.27.0.

Location: validate.go, projectValidationDetails lines 128–133.

Every native finding calls slices.Contains on the selected severity and code lists. With D diagnostic records, C supplied code filters, and V supplied severity filters, membership work is O(D(C+V)) in the worst case, excluding string-comparison and report-output costs. Code-filter lists and diagnostic counts are independent inputs; when C grows with D this becomes quadratic. Repeated flags also retain duplicate entries, and a nonmatching code scans the entire list. This work repeats for each reached configuration scope.

Refactoring opportunity: compile the supplied filters into invocation-local membership sets once before per-scope projection. Expected membership cost then becomes O(C+V+D) across the invocation's findings, plus O(C+V) set storage and the existing projection/source-reference work. Standard Go maps are sufficient; the two native severities can also use simple boolean selections. This does not change any validation rule or diagnostic code vocabulary.

Verification: preserve match-any within each filter, match-both across filters, duplicate/unmatched code behavior, native record order, the four evidence states, source references, and unchanged operation outcomes. No timing threshold or benchmark is needed to establish the complexity difference.

R6 — P3: Ordered inputs use general sorting of already indexed streams

Location: binding.go, prepareInputWithOccurrences line 316, and flag_values.go, FlagValues.applyTo line 122.

Both initial preparation and every captured replay concatenate per-binding ordered occurrences and call sort.SliceStable by ordinal. E selected events therefore enter a general comparison sort with an O(E log E) comparison bound and additional stable-sort movement costs. Replay repeats this work for each application/layer. The data already contains more structure than an arbitrary unsorted array: parser ordinals are known, and each binding's event list is monotone in those ordinals. With K nonempty streams, including the common text and file pair, a general sort is unnecessary to establish the same order.

Refactoring opportunity: during initial preparation, place typed events at their known parser positions and traverse those positions once. If M is the total parsed occurrence count and F the binding count, the existing O(M+F) preparation traversal can establish order without a comparison sort. For captured subset replay, merge the K already sorted streams using a small heap in O(K+E log K) time and O(K) merge state, with a direct traversal for K=1; two streams admit an ordinary linear merge. Alternatively, retain an ordered capture log and filter selected binding identities in one pass, accounting for the cost of scanning unselected events. This is a refactoring candidate, not a claim about measured CLI latency or a need for a new public API.

Verification: retain conversion-before-construction, ordinary values before ordered setters, operands afterward, original binding identity, selected subsets, required checks, both replay methods, aliases, nested capture, and repeated invocations. Preserve existing duplicate-selection/tie behavior rather than adding restrictions on selected handles. The tests should protect emitted order and forwarding, not the choice of sorting or merging mechanism.

R7 — P3: Path discovery reparses its invariant subtree prefix

Location: explain.go, runConfigExplanation's discovery loop and pathWithin lines 177–194.

For every enumerated path, pathWithin serializes and splits both that path and the same requested parent pointer. Let P be the path count, U the byte length of the requested subtree pointer, and L the total byte length of enumerated path strings. Repeated parent conversion/tokenization alone costs O(PU), yielding O(L+PU) work even when every child is too short to match. U is an independent operator input, so this avoidable product can become quadratic as both inputs grow.

Refactoring opportunity: serialize and tokenize the parent once before the discovery loop, then pass those immutable tokens into the comparison helper. Tokenize each child once and retain token-boundary comparisons. The conversion and matching work can then be O(U+L), with O(U) retained parent tokens, apart from native metadata enumeration/lookup and report generation. A prefix cache, new public path representation, or upstream path implementation is unnecessary.

Verification: keep root, escaped-key, exact-path, descendant, sibling-prefix, and overly deep unmatched-prefix behavior. Preserve historical/explicitness filter composition. This changes repeated preparation of an invariant value, not the planned native pointer semantics or discovery algorithm.

Applying the fixes

Keep fixes and regression coverage at the CommandKit adaptation/reporting boundaries under the testing policy. These findings do not call for reimplementation or independent verification of upstream domain algorithms. Follow the release procedure and existing immutability policy for any subsequent released dependency alignment; do not rewrite published core or ckconfig v0.4.0 contents.