141 lines
8.4 KiB
Markdown
141 lines
8.4 KiB
Markdown
# Shared reporting and PromptKit adoption remediation
|
||
|
||
Status: three open findings from review of `f83866e44a8a9066db52cbd2aa5cccde0d69a839`.
|
||
|
||
## 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. 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.
|
||
|
||
## 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.
|
||
|
||
## 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.
|