From 0fe77baa29ea6853e977587acdd50cb30791d0fc Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sun, 18 Oct 2026 03:52:35 +0000 Subject: [PATCH] Record dependency and smoke review findings --- docs/roadmap/remediation.md | 250 ++++++++++++++++++++++-------------- 1 file changed, 154 insertions(+), 96 deletions(-) diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 0eb43f4..59a8816 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -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 -architectural redesign was identified as necessary. +Status: open; R-1 through R-4 require attention. ## 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. +Reviewed `29582dd..730f930b47c929fb2c59a25a75cbe2924dfc75bb` against the +[accepted upgrade roadmap](upgrades.md), [implementation plan](implementation.md), +[architecture policy](../policy/architecture.md), and +[testing ownership policy](../policy/testing.md#upstream-integration-testing-ownership). +The worktree was clean at review start. -The targeted architecture is present: +The intended architecture is substantially 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. +- Core owns generic declarations, binding, installation, dispatch, help/version, + diagnostics, and outcomes; its Cobra/pflag adapter remains internal. +- ckconfig owns ConfigKit adaptation and the extended source reports. +- ckprompt depends on public core/ckconfig/ConfigKit/PromptKit APIs. Its eight + bundled inspection/catalog callbacks and two optional validation/smoke + additions preserve the selected ownership boundary. +- Native IDs, optional versions, filters, source evidence, and smoke reports + pass through the owning integrations. Aggregate smoke testing coordinates + concrete callbacks in the selected order and retains partial reports/errors. +- 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 - 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 completed implementation plan records earlier test runs; those completion +markers are historical evidence, not fresh test results from this review. +No production correction has been applied here. The preceding completed +request-validation review remains available in +[Git history](https://gitea.maximumdirect.net/eric/CommandKit/src/commit/730f930b47c929fb2c59a25a75cbe2924dfc75bb/docs/roadmap/remediation.md). -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: Preserve aggregate error presence when its message is empty -## 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:** -[validation_command_test.go](../../integrations/ckprompt/validation_command_test.go), -`TestValidationCommandUsageAndInputFailuresStopProviderWork`, lines 276–283. +**Finding:** The DTO uses `Error string` with `yaml:"error,omitempty"`. +`encodeSmokeResults` assigns `result.Err.Error()` only when the error is +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 -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. +For example, a concrete callback returning a native report and `errors.New("")` +still causes an operational aggregate outcome, but its YAML entry has no +`error` key. Report consumers cannot distinguish that entry from one whose +callback returned no error. No nonempty-error-message requirement was selected +for application callbacks. -[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:** Track error presence separately from its display text. A +private `*string` DTO field with `omitempty` is sufficient: nil for no error, +and a pointer to the returned message for a non-nil error, including an empty +message. Preserve the existing native report, aggregate ordering, joined cause +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 -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:** Add a focused semantic YAML regression covering nil, +empty-message, and ordinary nonempty errors. Only nil omits `error`; an empty +message remains `error: ""`. Confirm the assembled aggregate still returns +operational failure and preserves the original error through `errors.Is`. +The appropriate focused tests and ckprompt suite pass. -**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: Remove assertions about PromptKit's private smoke-test execution -**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. +Status: open. Priority: P3. Category: upstream testing ownership. -## 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:** -[validation_command_test.go](../../integrations/ckprompt/validation_command_test.go), -`TestValidationCommandRetainsProviderDiagnosticsAndClassifiesFailures`, line 394. +A legitimate upstream change to the private prompt shape or diagnostic +generation sequence could fail these tests without any incorrect CommandKit +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 -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. +**Required work:** Remove the exact native message-count assertion and exact +native generation-count expectations. Retain evidence that the intended native +path is reached and that the seed or authoritative builder request controls +the observed model/credential values. Keep report/error preservation, +configuration selection, context/input/state forwarding, and builder-failure +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, -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. +This does not remove zero-call guards for help, metadata operations, or failures +that must stop before acquisition. Those protect selected CommandKit behavior. -**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:** Focused helper tests and the ckprompt suite pass. +Assertions still detect request remerging, wrong forwarding, or dropped results, +without inspecting the native private diagnostic message inventory or fixing +the generation sequence. No production-code change is required. -**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. +## R-3: Remove incidental loader-count and pointer-identity contracts -**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. +Status: open. Priority: P3. Category: unnecessary implementation constraints. + +**Locations:** + +- [smoke_lifecycle_test.go](../../integrations/ckprompt/smoke_lifecycle_test.go), + `TestSmokeTestBuilderReceivesConcreteAggregateKinds`, lines 201–212. +- [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.