122 lines
6.8 KiB
Markdown
122 lines
6.8 KiB
Markdown
# PromptKit request validation review remediation
|
||
|
||
Status: complete; R-1 and R-2 addressed. No production-code correction or
|
||
architectural redesign was identified as necessary.
|
||
|
||
## Review scope and evidence
|
||
|
||
Reviewed implementation commits after planning baseline `3218222`, through
|
||
`420483b2d4659862626422b0577e39bc0a4c1a2d`, against the
|
||
[accepted feature roadmap](promptkit-validation.md),
|
||
[implementation plan](https://gitea.maximumdirect.net/eric/CommandKit/src/commit/38b49f29fb3a8f22ff02528d2a0ed413e5c6ab1e/docs/roadmap/implementation.md),
|
||
applicable ADRs, and repository policy.
|
||
The review covered production changes, new tests, the maintained consumer,
|
||
dependency metadata/attribution, and updated documentation.
|
||
|
||
The targeted architecture is present:
|
||
|
||
- Core retains generic dispatch, binding, registration, and outcome ownership.
|
||
- ckconfig retains configuration adaptation without depending on PromptKit.
|
||
- ckprompt owns the independent validation capability, request envelope,
|
||
suite-returning adapter, standard command, and YAML confirmation.
|
||
- The bundled adapter builds before loading, forwards the resulting native
|
||
request without merging or independent identity checks, and delegates to
|
||
`Engine.ValidateRequest`. Nil-builder forwarding and retained wiring issues
|
||
are implemented.
|
||
- Applications retain typed input/state, defaults, artifact mapping, and runtime
|
||
ownership. Requirements discovery remains separate and permits partial results.
|
||
|
||
Review verification passed:
|
||
|
||
- Fresh `GOTOOLCHAIN=local go test -count=1 ./...` runs from each of the three
|
||
module directories, including the maintained composed consumer.
|
||
- Fresh isolated `GOWORK=off`, `GOTOOLCHAIN=local`, `-mod=readonly` suite runs for
|
||
all three modules. Temporary manifests replaced only unpublished CommandKit
|
||
prerequisites; their module/sum hashes stayed unchanged during verification.
|
||
- 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
|
||
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: complete. Severity: low. Category: unintended lifecycle guarantee in a test.
|
||
|
||
**Location:**
|
||
[validation_command_test.go](../../integrations/ckprompt/validation_command_test.go),
|
||
`TestValidationCommandUsageAndInputFailuresStopProviderWork`, lines 276–283.
|
||
|
||
**Finding:** The missing-required-selector case sets `wantInputCalls = 1` and
|
||
requires that exact factory count. The comment explains the current private
|
||
`prepareSuiteInput` order, rather than an accepted requirement. A refactor that
|
||
detects the missing selector before allocating application input would return
|
||
the same usage failure and correctly skip later work, but this test would fail.
|
||
|
||
[ADR 0011](../adr/0011-captured-declarations-and-installation.md#input-and-shared-state-lifetimes)
|
||
allows failures to stop later input factories. Suite GoDoc likewise says
|
||
factories need not run after failure. The selected contract requires the usage
|
||
classification and stopping input checks, state creation, and provider work; it
|
||
does not require an input factory to run for this failed invocation.
|
||
|
||
**Required work:** Remove the exact input-factory count assertion for this case
|
||
and the comment that treats the private ordering as a required lifecycle. Retain
|
||
the appropriate usage status and assertions that `CheckInput`, `NewState`, and
|
||
the provider remain uninvoked. Preserve zero-factory assertions for malformed
|
||
syntax and unsupported-format preflight cases where that behavior is selected.
|
||
Keep successful-invocation and help/version lifecycle coverage.
|
||
|
||
**Completion evidence:** The focused test and ckprompt suite pass; the
|
||
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
|
||
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
|
||
|
||
Status: complete. Severity: low. Category: unnecessary presentation constraint in a test.
|
||
|
||
**Location:**
|
||
[validation_command_test.go](../../integrations/ckprompt/validation_command_test.go),
|
||
`TestValidationCommandRetainsProviderDiagnosticsAndClassifiesFailures`, line 394.
|
||
|
||
**Finding:** The provider-failure case requires the entire diagnostic stream to
|
||
equal `"validation warning\nvalidation provider failed\n"`. This freezes the
|
||
unadorned error presentation and exact line layout. An otherwise valid shared
|
||
diagnostic formatting change, such as adding an error label, would fail this
|
||
validation-command test even while preserving cause identity, classification,
|
||
warning delivery, and single presentation.
|
||
|
||
[ADR 0005](../adr/0005-invocation-outcomes.md) selects diagnostic ownership,
|
||
failure classification, supported error identity, and delivery handling. It
|
||
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
|
||
checks that the synthetic warning and provider error are delivered once.
|
||
Retain operational classification/status, `errors.Is` cause identity, retained
|
||
diagnostics, and applicable delivery-state checks. Controlled diagnostic-writer
|
||
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
|
||
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
|
||
single-occurrence checks for the synthetic warning and provider error. Retained
|
||
operational classification, status, cause identity, and diagnostic payload
|
||
checks, and verified complete diagnostic delivery. The focused
|
||
`TestValidationCommandRetainsProviderDiagnosticsAndClassifiesFailures` test and
|
||
`GOTOOLCHAIN=local go test -count=1 ./...` passed from `integrations/ckprompt`,
|
||
including the composed consumer. Production formatting and delivery are unchanged.
|