diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 6df7fb1..9aa1230 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,6 +1,9 @@ # Shared reporting and PromptKit adoption remediation -Status: three open findings from review of `f83866e44a8a9066db52cbd2aa5cccde0d69a839`. +Status: seven open findings. 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 @@ -21,11 +24,12 @@ 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. Temporary external-consumer probes using local module -replacements reproduced the findings below. Those probes did not modify the -repository's source or tests. No additional strengthened invariant, prohibition, -or guarantee was identified; the findings concern incomplete report/evidence -adaptation, not a request for stronger domain validation or new guarantees. +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 @@ -130,6 +134,138 @@ result's native inspection data. Preserve original causes, available-empty versu 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 + +**Location:** [report_value.go](../../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. Core-generated YAML +already retains explicit numeric tags when needed. 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 + +**Location:** [validate.go](../../integrations/ckconfig/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](../../binding.go), prepareInputWithOccurrences line +316, and [flag_values.go](../../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](../../integrations/ckconfig/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