Record dependency and smoke review findings
This commit is contained in:
@@ -1,121 +1,179 @@
|
|||||||
# PromptKit request validation review remediation
|
# Dependency upgrade and PromptKit smoke review remediation
|
||||||
|
|
||||||
Status: complete; R-1 and R-2 addressed. No production-code correction or
|
Status: open; R-1 through R-4 require attention.
|
||||||
architectural redesign was identified as necessary.
|
|
||||||
|
|
||||||
## Review scope and evidence
|
## Review scope and evidence
|
||||||
|
|
||||||
Reviewed implementation commits after planning baseline `3218222`, through
|
Reviewed `29582dd..730f930b47c929fb2c59a25a75cbe2924dfc75bb` against the
|
||||||
`420483b2d4659862626422b0577e39bc0a4c1a2d`, against the
|
[accepted upgrade roadmap](upgrades.md), [implementation plan](implementation.md),
|
||||||
[accepted feature roadmap](promptkit-validation.md),
|
[architecture policy](../policy/architecture.md), and
|
||||||
[implementation plan](https://gitea.maximumdirect.net/eric/CommandKit/src/commit/38b49f29fb3a8f22ff02528d2a0ed413e5c6ab1e/docs/roadmap/implementation.md),
|
[testing ownership policy](../policy/testing.md#upstream-integration-testing-ownership).
|
||||||
applicable ADRs, and repository policy.
|
The worktree was clean at review start.
|
||||||
The review covered production changes, new tests, the maintained consumer,
|
|
||||||
dependency metadata/attribution, and updated documentation.
|
|
||||||
|
|
||||||
The targeted architecture is present:
|
The intended architecture is substantially present:
|
||||||
|
|
||||||
- Core retains generic dispatch, binding, registration, and outcome ownership.
|
- Core owns generic declarations, binding, installation, dispatch, help/version,
|
||||||
- ckconfig retains configuration adaptation without depending on PromptKit.
|
diagnostics, and outcomes; its Cobra/pflag adapter remains internal.
|
||||||
- ckprompt owns the independent validation capability, request envelope,
|
- ckconfig owns ConfigKit adaptation and the extended source reports.
|
||||||
suite-returning adapter, standard command, and YAML confirmation.
|
- ckprompt depends on public core/ckconfig/ConfigKit/PromptKit APIs. Its eight
|
||||||
- The bundled adapter builds before loading, forwards the resulting native
|
bundled inspection/catalog callbacks and two optional validation/smoke
|
||||||
request without merging or independent identity checks, and delegates to
|
additions preserve the selected ownership boundary.
|
||||||
`Engine.ValidateRequest`. Nil-builder forwarding and retained wiring issues
|
- Native IDs, optional versions, filters, source evidence, and smoke reports
|
||||||
are implemented.
|
pass through the owning integrations. Aggregate smoke testing coordinates
|
||||||
- Applications retain typed input/state, defaults, artifact mapping, and runtime
|
concrete callbacks in the selected order and retains partial reports/errors.
|
||||||
ownership. Requirements discovery remains separate and permits partial results.
|
- ReleaseKit remains a root maintainer tool. The adoption leaves the launcher,
|
||||||
|
profiles, shared-file selection, and source-only publication model intact.
|
||||||
|
|
||||||
Review verification passed:
|
This was a source, test, documentation, and dependency review. Readonly
|
||||||
|
workspace and independent module listings confirmed ReleaseKit v0.4.2,
|
||||||
|
ConfigKit v0.8.0, and PromptKit v0.16.0. Independent package dependency listings,
|
||||||
|
including test imports, confirmed no optional integration libraries in core and
|
||||||
|
no PromptKit/ckprompt dependency in ckconfig. Temporary manifests replaced only
|
||||||
|
unpublished CommandKit prerequisites; upstream dependencies remained released
|
||||||
|
modules. No test suite was run or added during this review.
|
||||||
|
|
||||||
- Fresh `GOTOOLCHAIN=local go test -count=1 ./...` runs from each of the three
|
The completed implementation plan records earlier test runs; those completion
|
||||||
module directories, including the maintained composed consumer.
|
markers are historical evidence, not fresh test results from this review.
|
||||||
- Fresh isolated `GOWORK=off`, `GOTOOLCHAIN=local`, `-mod=readonly` suite runs for
|
No production correction has been applied here. The preceding completed
|
||||||
all three modules. Temporary manifests replaced only unpublished CommandKit
|
request-validation review remains available in
|
||||||
prerequisites; their module/sum hashes stayed unchanged during verification.
|
[Git history](https://gitea.maximumdirect.net/eric/CommandKit/src/commit/730f930b47c929fb2c59a25a75cbe2924dfc75bb/docs/roadmap/remediation.md).
|
||||||
- Isolated module and test-import listings confirm the core/ckconfig boundaries;
|
|
||||||
ckprompt selects released PromptKit v0.15.0 without a PromptKit replacement.
|
|
||||||
- Changed prose whitespace and local documentation links/anchors were checked.
|
|
||||||
|
|
||||||
The findings below concern test constraints that would make otherwise valid
|
## R-1: Preserve aggregate error presence when its message is empty
|
||||||
refactoring fail. They do not indicate an incorrect runtime result. Address them
|
|
||||||
without changing production behavior or expanding public contracts.
|
|
||||||
|
|
||||||
## R-1: Remove the required factory call after missing-selector failure
|
Status: open. Priority: P2. Category: report fidelity.
|
||||||
|
|
||||||
Status: complete. Severity: low. Category: unintended lifecycle guarantee in a test.
|
**Location:** [smoke_report.go](../../integrations/ckprompt/smoke_report.go),
|
||||||
|
`smokeResultDTO` and `encodeSmokeResults`, lines 101–125.
|
||||||
|
|
||||||
**Location:**
|
**Finding:** The DTO uses `Error string` with `yaml:"error,omitempty"`.
|
||||||
[validation_command_test.go](../../integrations/ckprompt/validation_command_test.go),
|
`encodeSmokeResults` assigns `result.Err.Error()` only when the error is
|
||||||
`TestValidationCommandUsageAndInputFailuresStopProviderWork`, lines 276–283.
|
non-nil. A non-nil error whose message is `""` therefore produces the same
|
||||||
|
empty DTO field as no error, and `omitempty` drops it.
|
||||||
|
|
||||||
**Finding:** The missing-required-selector case sets `wantInputCalls = 1` and
|
For example, a concrete callback returning a native report and `errors.New("")`
|
||||||
requires that exact factory count. The comment explains the current private
|
still causes an operational aggregate outcome, but its YAML entry has no
|
||||||
`prepareSuiteInput` order, rather than an accepted requirement. A refactor that
|
`error` key. Report consumers cannot distinguish that entry from one whose
|
||||||
detects the missing selector before allocating application input would return
|
callback returned no error. No nonempty-error-message requirement was selected
|
||||||
the same usage failure and correctly skip later work, but this test would fail.
|
for application callbacks.
|
||||||
|
|
||||||
[ADR 0011](../adr/0011-captured-declarations-and-installation.md#input-and-shared-state-lifetimes)
|
**Required work:** Track error presence separately from its display text. A
|
||||||
allows failures to stop later input factories. Suite GoDoc likewise says
|
private `*string` DTO field with `omitempty` is sufficient: nil for no error,
|
||||||
factories need not run after failure. The selected contract requires the usage
|
and a pointer to the returned message for a non-nil error, including an empty
|
||||||
classification and stopping input checks, state creation, and provider work; it
|
message. Preserve the existing native report, aggregate ordering, joined cause
|
||||||
does not require an input factory to run for this failed invocation.
|
identity, and normal outcome classification. Do not replace empty messages,
|
||||||
|
reject the error, or add another error taxonomy.
|
||||||
|
|
||||||
**Required work:** Remove the exact input-factory count assertion for this case
|
**Completion evidence:** Add a focused semantic YAML regression covering nil,
|
||||||
and the comment that treats the private ordering as a required lifecycle. Retain
|
empty-message, and ordinary nonempty errors. Only nil omits `error`; an empty
|
||||||
the appropriate usage status and assertions that `CheckInput`, `NewState`, and
|
message remains `error: ""`. Confirm the assembled aggregate still returns
|
||||||
the provider remain uninvoked. Preserve zero-factory assertions for malformed
|
operational failure and preserves the original error through `errors.Is`.
|
||||||
syntax and unsupported-format preflight cases where that behavior is selected.
|
The appropriate focused tests and ckprompt suite pass.
|
||||||
Keep successful-invocation and help/version lifecycle coverage.
|
|
||||||
|
|
||||||
**Completion evidence:** The focused test and ckprompt suite pass; the
|
## R-2: Remove assertions about PromptKit's private smoke-test execution
|
||||||
missing-selector case accepts early failure before application input allocation
|
|
||||||
without losing its guards against later work. No production change is required.
|
|
||||||
|
|
||||||
**Completed:** 2026-10-12. Removed the missing-selector factory-count requirement
|
Status: open. Priority: P3. Category: upstream testing ownership.
|
||||||
and private-ordering comment. Usage classification and checks preventing input
|
|
||||||
checks, state creation, and provider work remain; syntax/format cases still
|
|
||||||
require zero input-factory calls. The focused
|
|
||||||
`TestValidationCommandUsageAndInputFailuresStopProviderWork` test and
|
|
||||||
`GOTOOLCHAIN=local go test -count=1 ./...` passed from `integrations/ckprompt`,
|
|
||||||
including the composed consumer. Production code is unchanged.
|
|
||||||
|
|
||||||
## R-2: Replace the complete provider-failure stderr snapshot
|
**Location:** [adapters_test.go](../../integrations/ckprompt/adapters_test.go),
|
||||||
|
`TestWithSmokeTestingForwardsAuthoritativeNativeRequestAndRetainsReport`,
|
||||||
|
line 167, and
|
||||||
|
`TestWithSmokeTestingNilBuilderForwardsSeedAndBuilderFailureSkipsLoader`,
|
||||||
|
line 182.
|
||||||
|
|
||||||
Status: complete. Severity: low. Category: unnecessary presentation constraint in a test.
|
**Finding:** The nil-builder test requires the native generated prompt to contain
|
||||||
|
exactly one message. Both tests require exactly one call to the injected native
|
||||||
|
LLM client. CommandKit forwards a `SmokeTestRequest` to `Engine.SmokeTest`;
|
||||||
|
PromptKit constructs its private diagnostic prompt and owns its internal
|
||||||
|
generation sequence.
|
||||||
|
|
||||||
**Location:**
|
A legitimate upstream change to the private prompt shape or diagnostic
|
||||||
[validation_command_test.go](../../integrations/ckprompt/validation_command_test.go),
|
generation sequence could fail these tests without any incorrect CommandKit
|
||||||
`TestValidationCommandRetainsProviderDiagnosticsAndClassifiesFailures`, line 394.
|
adaptation. The selected testing boundary permits observing the client to
|
||||||
|
verify forwarding, but does not establish private native work as a CommandKit
|
||||||
|
contract.
|
||||||
|
|
||||||
**Finding:** The provider-failure case requires the entire diagnostic stream to
|
**Required work:** Remove the exact native message-count assertion and exact
|
||||||
equal `"validation warning\nvalidation provider failed\n"`. This freezes the
|
native generation-count expectations. Retain evidence that the intended native
|
||||||
unadorned error presentation and exact line layout. An otherwise valid shared
|
path is reached and that the seed or authoritative builder request controls
|
||||||
diagnostic formatting change, such as adding an error label, would fail this
|
the observed model/credential values. Keep report/error preservation,
|
||||||
validation-command test even while preserving cause identity, classification,
|
configuration selection, context/input/state forwarding, and builder-failure
|
||||||
warning delivery, and single presentation.
|
ordering checks. Where counting is useful, checking that native client use
|
||||||
|
occurred is sufficient; do not fix its native internal call count.
|
||||||
|
|
||||||
[ADR 0005](../adr/0005-invocation-outcomes.md) selects diagnostic ownership,
|
This does not remove zero-call guards for help, metadata operations, or failures
|
||||||
failure classification, supported error identity, and delivery handling. It
|
that must stop before acquisition. Those protect selected CommandKit behavior.
|
||||||
does not select this complete text snapshot. The
|
|
||||||
[testing policy](../policy/testing.md#go-specific-guidance) calls for minimal
|
|
||||||
semantic error fragments unless exact wording is contractual.
|
|
||||||
|
|
||||||
**Required work:** Replace the complete failed-stderr equality with focused
|
**Completion evidence:** Focused helper tests and the ckprompt suite pass.
|
||||||
checks that the synthetic warning and provider error are delivered once.
|
Assertions still detect request remerging, wrong forwarding, or dropped results,
|
||||||
Retain operational classification/status, `errors.Is` cause identity, retained
|
without inspecting the native private diagnostic message inventory or fixing
|
||||||
diagnostics, and applicable delivery-state checks. Controlled diagnostic-writer
|
the generation sequence. No production-code change is required.
|
||||||
payload checks can remain; this finding does not require removing exact checks
|
|
||||||
of bytes explicitly supplied by the test's own writer.
|
|
||||||
|
|
||||||
**Completion evidence:** The focused test and ckprompt suite pass and still
|
## R-3: Remove incidental loader-count and pointer-identity contracts
|
||||||
detect lost or duplicate diagnostics without fixing incidental error decoration
|
|
||||||
or line layout. Preserve current production delivery behavior and order.
|
|
||||||
|
|
||||||
**Completed:** 2026-10-12. Replaced the failed-stderr snapshot with independent
|
Status: open. Priority: P3. Category: unnecessary implementation constraints.
|
||||||
single-occurrence checks for the synthetic warning and provider error. Retained
|
|
||||||
operational classification, status, cause identity, and diagnostic payload
|
**Locations:**
|
||||||
checks, and verified complete diagnostic delivery. The focused
|
|
||||||
`TestValidationCommandRetainsProviderDiagnosticsAndClassifiesFailures` test and
|
- [smoke_lifecycle_test.go](../../integrations/ckprompt/smoke_lifecycle_test.go),
|
||||||
`GOTOOLCHAIN=local go test -count=1 ./...` passed from `integrations/ckprompt`,
|
`TestSmokeTestBuilderReceivesConcreteAggregateKinds`, lines 201–212.
|
||||||
including the composed consumer. Production formatting and delivery are unchanged.
|
- [smoke_aggregate_test.go](../../integrations/ckprompt/smoke_aggregate_test.go),
|
||||||
|
`TestSmokeAggregateCoordinatesConcreteRequestsAndRetainsErrors`,
|
||||||
|
lines 89–95 and its pointer-collection bookkeeping.
|
||||||
|
|
||||||
|
**Finding:** The builder test requires exactly four engine-loader calls.
|
||||||
|
The accepted aggregate behavior requires per-kind requests/builders and allows
|
||||||
|
engine/acquisition reuse where practical; it deliberately does not guarantee
|
||||||
|
a loader-call count.
|
||||||
|
|
||||||
|
The aggregate test also requires pairwise distinct addresses for every
|
||||||
|
execution override and selected scalar pointer. It already checks the
|
||||||
|
meaningful behavior: mutating the first seed must not change the common CLI
|
||||||
|
values seen by subsequent operations. Pairwise allocation identity adds a
|
||||||
|
separate constraint on storage strategy that the operator and consumer
|
||||||
|
contracts do not require.
|
||||||
|
|
||||||
|
The implementation plan describes a straightforward fresh-storage approach.
|
||||||
|
That instruction does not independently establish public pointer identity or
|
||||||
|
resource-management guarantees under the
|
||||||
|
[agent policy](../policy/agents.md#commandkit-contract-authority).
|
||||||
|
|
||||||
|
**Required work:** Remove the exact aggregate loader count and pairwise pointer
|
||||||
|
inequality assertions, together with bookkeeping used only for those assertions.
|
||||||
|
Retain concrete builder/callback order, per-kind builder inputs, shared
|
||||||
|
invocation input/state, subsequent seed values after mutation, continuation,
|
||||||
|
cancellation, and native error identity. Keep justified guards preventing
|
||||||
|
loading after builder failure and during help/version. Do not add caching,
|
||||||
|
pooling, or a public lifetime abstraction to resolve a test-only issue.
|
||||||
|
|
||||||
|
**Completion evidence:** Focused aggregate/lifecycle tests and the ckprompt suite
|
||||||
|
pass. The tests continue to detect cross-test value contamination and missing
|
||||||
|
operations, while accepting equivalent acquisition and storage implementations.
|
||||||
|
No production-code change is required.
|
||||||
|
|
||||||
|
## R-4: Reconcile the feature roadmap with completed implementation
|
||||||
|
|
||||||
|
Status: open. Priority: P3. Category: stale implementation status.
|
||||||
|
|
||||||
|
**Location:** [upgrades.md](upgrades.md), status at lines 3–6,
|
||||||
|
current-implementation descriptions at lines 28–29 and 41–44, and the source
|
||||||
|
DTO statement at line 124.
|
||||||
|
|
||||||
|
**Finding:** The roadmap still says implementation has not started and that the
|
||||||
|
current source DTO omits all four new evidence fields. The code now exposes
|
||||||
|
those fields, and the implementation plan marks Stages 1–18 complete. Other
|
||||||
|
present-tense descriptions still refer to the planning baseline.
|
||||||
|
|
||||||
|
Stage 18 expressly called for updating the roadmap's implementation status.
|
||||||
|
Leaving these claims in place makes implemented features appear absent and
|
||||||
|
can cause a later agent to restart completed work.
|
||||||
|
|
||||||
|
**Required work:** Record the delivered implementation and link to the completed
|
||||||
|
plan/current canonical documentation. Qualify old baseline descriptions and
|
||||||
|
version tables as historical rather than rewriting the planning baseline.
|
||||||
|
Remove or recast assertions that the current DTO/API lacks delivered behavior.
|
||||||
|
Preserve accepted decisions, dependency boundaries, and the remaining review
|
||||||
|
findings; feature delivery can be recorded as complete while these focused
|
||||||
|
remediations remain open.
|
||||||
|
|
||||||
|
**Completion evidence:** The roadmap and implementation plan agree on delivery
|
||||||
|
status, current-code claims match the inspected implementation, and local
|
||||||
|
links/Markdown whitespace checks pass. No code or policy change is required.
|
||||||
|
|||||||
Reference in New Issue
Block a user