Retire completed upgrade planning documents
This commit is contained in:
@@ -1,213 +0,0 @@
|
||||
# Dependency upgrade and PromptKit smoke review remediation
|
||||
|
||||
Status: complete; R-1 through R-4 addressed.
|
||||
|
||||
## Review scope and evidence
|
||||
|
||||
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 intended architecture is substantially present:
|
||||
|
||||
- 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.
|
||||
|
||||
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.
|
||||
|
||||
The completed implementation plan records earlier test runs; those completion
|
||||
markers are historical evidence, not fresh test results from this review.
|
||||
No production correction was applied during the review. Completion records
|
||||
below describe the subsequent remediation. The preceding completed
|
||||
request-validation review remains available in
|
||||
[Git history](https://gitea.maximumdirect.net/eric/CommandKit/src/commit/730f930b47c929fb2c59a25a75cbe2924dfc75bb/docs/roadmap/remediation.md).
|
||||
|
||||
## R-1: Preserve aggregate error presence when its message is empty
|
||||
|
||||
Status: complete. Priority: P2. Category: report fidelity.
|
||||
|
||||
**Location:** [smoke_report.go](../../integrations/ckprompt/smoke_report.go),
|
||||
`smokeResultDTO` and `encodeSmokeResults`, lines 101–125.
|
||||
|
||||
**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.
|
||||
|
||||
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.
|
||||
|
||||
**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.
|
||||
|
||||
**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.
|
||||
|
||||
**Completed:** 2026-10-18. Changed the private aggregate error field to an
|
||||
optional string pointer, preserving non-nil empty messages. Extended the YAML
|
||||
projection and assembled aggregate regressions to distinguish nil, empty, and
|
||||
nonempty errors while retaining operational classification and typed cause
|
||||
identity. Both focused regressions failed before the fix and passed afterward.
|
||||
The complete ckprompt suite, including the composed consumer, passed with
|
||||
Go 1.26.8 and offline dependency resolution.
|
||||
|
||||
## R-2: Remove assertions about PromptKit's private smoke-test execution
|
||||
|
||||
Status: complete. Priority: P3. Category: upstream testing ownership.
|
||||
|
||||
**Location:** [adapters_test.go](../../integrations/ckprompt/adapters_test.go),
|
||||
`TestWithSmokeTestingForwardsAuthoritativeNativeRequestAndRetainsReport`,
|
||||
line 167, and
|
||||
`TestWithSmokeTestingNilBuilderForwardsSeedAndBuilderFailureSkipsLoader`,
|
||||
line 182.
|
||||
|
||||
**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.
|
||||
|
||||
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.
|
||||
|
||||
**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.
|
||||
|
||||
This does not remove zero-call guards for help, metadata operations, or failures
|
||||
that must stop before acquisition. Those protect selected CommandKit behavior.
|
||||
|
||||
**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.
|
||||
|
||||
**Completed:** 2026-10-18. Removed the private diagnostic message-count
|
||||
assertion and changed native generation-count checks to require only that the
|
||||
configured client was used. Request/model/credential forwarding, authoritative
|
||||
builder output, report/error handling, context/input/state checks, and the
|
||||
builder-failure guard against loading remain covered. Focused
|
||||
`TestWithSmokeTesting` tests and the complete ckprompt suite, including the
|
||||
composed consumer, passed with Go 1.26.8 and offline dependency resolution.
|
||||
Production code is unchanged.
|
||||
|
||||
## R-3: Remove incidental loader-count and pointer-identity contracts
|
||||
|
||||
Status: complete. 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.
|
||||
|
||||
**Completed:** 2026-10-18. Removed the aggregate loader counter and pairwise
|
||||
pointer-address assertions/bookkeeping. The tests retain ordered concrete
|
||||
builder/callback inputs, shared input/state, mutation-isolated seed values,
|
||||
continuation, cancellation, reports, and error identity. Focused aggregate,
|
||||
builder, help/version, and builder-failure tests and the complete ckprompt suite,
|
||||
including the composed consumer, passed with Go 1.26.8 and offline dependency
|
||||
resolution. Production code is unchanged.
|
||||
|
||||
## R-4: Reconcile the feature roadmap with completed implementation
|
||||
|
||||
Status: complete. 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.
|
||||
|
||||
**Completed:** 2026-10-18. Recorded implementation completion and linked the
|
||||
completed plan, review follow-up, and current CLI/consumer/GoDoc owners. Marked
|
||||
the original dependency/capability descriptions as historical and corrected the
|
||||
source-report and ID-only API descriptions to match delivered code. Preserved
|
||||
the original baseline and accepted decisions unchanged. Local links/fragments,
|
||||
inbound roadmap anchors, status/baseline consistency, and Markdown whitespace
|
||||
checks passed. No code or policy change was needed for this issue.
|
||||
Reference in New Issue
Block a user