Record PromptKit validation review findings
This commit is contained in:
104
docs/roadmap/remediation.md
Normal file
104
docs/roadmap/remediation.md
Normal file
@@ -0,0 +1,104 @@
|
||||
# PromptKit request validation review remediation
|
||||
|
||||
Status: two open findings; review complete. 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](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: open. 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.
|
||||
|
||||
## R-2: Replace the complete provider-failure stderr snapshot
|
||||
|
||||
Status: open. 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.
|
||||
Reference in New Issue
Block a user