From d492c7747175c01a53261206c2b4356e53453621 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sat, 7 Nov 2026 15:20:25 +0000 Subject: [PATCH] Remove the completed shared reporting remediation plan --- docs/roadmap/audit-remediation.md | 2 +- docs/roadmap/audit/01-correctness.md | 2 +- docs/roadmap/audit/02-efficiency.md | 2 +- docs/roadmap/remediation.md | 328 --------------------------- 4 files changed, 3 insertions(+), 331 deletions(-) delete mode 100644 docs/roadmap/remediation.md diff --git a/docs/roadmap/audit-remediation.md b/docs/roadmap/audit-remediation.md index 100898b..1167c8a 100644 --- a/docs/roadmap/audit-remediation.md +++ b/docs/roadmap/audit-remediation.md @@ -14,7 +14,7 @@ the review and decisions. This roadmap owns the target, work grouping, dependencies, and completion criteria. The catalog owns finding status and accepted remediation decisions; the original audit entries own detailed evidence. This is separate from the -older, completed [shared-reporting remediation](remediation.md), whose findings +older, completed [shared-reporting remediation](https://gitea.maximumdirect.net/eric/CommandKit/src/commit/264b82bd5759f052a3646d57828945a87b7586d3/docs/roadmap/remediation.md), whose findings are not reopened here. Unselected capabilities in [future work](future.md) remain outside this effort. diff --git a/docs/roadmap/audit/01-correctness.md b/docs/roadmap/audit/01-correctness.md index db7da3c..cd18e3e 100644 --- a/docs/roadmap/audit/01-correctness.md +++ b/docs/roadmap/audit/01-correctness.md @@ -50,7 +50,7 @@ production code or repository tests: checks failed before interactive completion, as described in C6. Zsh and Fish are not installed, so their interactive checks were not run. -The completed findings in [remediation.md](../remediation.md) remain historical +The completed findings in [remediation.md](https://gitea.maximumdirect.net/eric/CommandKit/src/commit/264b82bd5759f052a3646d57828945a87b7586d3/docs/roadmap/remediation.md) remain historical records of the preceding reviews. This audit records additional findings against the corrected source, rather than reopening those issues. Independent integration modules still select released CommandKit dependencies; workspace tests and the diff --git a/docs/roadmap/audit/02-efficiency.md b/docs/roadmap/audit/02-efficiency.md index a65ef75..dd5d514 100644 --- a/docs/roadmap/audit/02-efficiency.md +++ b/docs/roadmap/audit/02-efficiency.md @@ -38,7 +38,7 @@ Explicitly selected approaches were excluded from reconsideration: - The [deferred scope/selector and backend work](../future.md#possible-further-efficiency-work), upstream domain algorithms, and the completed dense occurrence reconstruction, heap merge, validation-filter indexing, and subtree-prefix preparation in - [remediation.md](../remediation.md). + [remediation.md](https://gitea.maximumdirect.net/eric/CommandKit/src/commit/264b82bd5759f052a3646d57828945a87b7586d3/docs/roadmap/remediation.md). The findings preserve those decisions. The plans prescribe endpoint, precedence, discovery, and validation behavior, but do not require independent repeated diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md deleted file mode 100644 index 75fd8ee..0000000 --- a/docs/roadmap/remediation.md +++ /dev/null @@ -1,328 +0,0 @@ -# Shared reporting and PromptKit adoption remediation - -Status: all seven findings 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](https://forge.maximumdirect.net/base-layer/commandkit/src/commit/2f43f78f76439f5d11ef313cd379bec036543504/docs/roadmap/promptkit-v0.20.0.md) -and [implementation plan](https://forge.maximumdirect.net/base-layer/commandkit/src/commit/2f43f78f76439f5d11ef313cd379bec036543504/docs/roadmap/implementation.md), -now retired into Git history. The [architecture policy](../policy/architecture.md), -[CLI reference](../cli.md), 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](../../integrations/ckconfig/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](../../integrations/ckprompt/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](../../integrations/ckconfig/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](../../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](../../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 - -**Complete.** Initial preparation places ordered setters at parser positions -and scans them once, using O(M+F) traversal and O(M) ordering storage. Captured -replay uses a standard-library heap to merge K monotone streams in O(K+E log K) -work for K greater than one, O(E) for one stream, and O(K) merge storage. -Selection order breaks ties, preserving duplicate handles. Core regressions cover -nested capture, reversed/subset/single/empty/duplicate selections, both replay -methods, conversion counts, and repeated invocations. The independent core and -aligned ckconfig/ckprompt suites pass with Go 1.27.0. - -**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 - -**Complete.** Discovery serializes and tokenizes the requested prefix once, -and serializes each child once. Filtering now takes O(U+L) conversion/matching -work and O(U) retained prefix storage, excluding native metadata and report work. -Command checks cover root, escaped slash/tilde, exact paths, descendants, sibling -prefixes, long unmatched prefixes, and historical/explicitness composition. The -independent ckconfig and complete aligned workspace suites pass with Go 1.27.0. - -**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 - -All completion markers describe fixes in the current source. Publication and -released dependency alignment remain separate release work. The original -observations above retain the evidence from the audited baseline. - -Keep fixes and regression coverage at the CommandKit adaptation/reporting -boundaries under the [testing policy](../policy/testing.md). These findings do -not call for reimplementation or independent verification of upstream domain -algorithms. Follow the [release procedure](../release.md) and existing -immutability policy for any subsequent released dependency alignment; do not -rewrite published core or ckconfig v0.4.0 contents.