277 lines
16 KiB
Markdown
277 lines
16 KiB
Markdown
# Shared reporting and PromptKit adoption remediation
|
||
|
||
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
|
||
|
||
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
|
||
|
||
**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
|
||
|
||
**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
|
||
|
||
**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
|
||
|
||
**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
|
||
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.
|