diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..fa36b39 --- /dev/null +++ b/docs/roadmap/remediation.md @@ -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.