base-layer/commandkit
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 2 Wiki Activity

Remove the completed shared reporting remediation plan

Browse Source
This commit is contained in:
Eric Rakestraw
2026-11-07 15:20:25 +00:00
parent 264b82bd57
commit d492c77471
4 changed files with 3 additions and 331 deletions
Download Patch File Download Diff File Expand all files Collapse all files

328
docs/roadmap/remediation.md
View File

@@ -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.