Files
commandkit/docs/roadmap/remediation.md

309 lines
18 KiB
Markdown

# Implementation review remediation
Reviewed on 2026-10-11, against committed implementation
`cc574915ee17e2d738f27874d2c8857aa1f51a47`. The implementation sprint starts
after planning commit `0d23676144ab1ce5047924e98f1ab6a2a9d6aeb6`.
Completion status and evidence are recorded per issue below. P1 identifies a
functional contract violation; P2
identifies architecture or documentation corrections; P3 identifies smaller
maintenance corrections. These priorities do not add publication gates.
## Review scope and evidence
The review compared the current code and documentation with the
[architecture policy](../policy/architecture.md), accepted ADRs, and the selected
API/backend designs retained in Git history. The three-module layout, dependency
direction, captured core declarations, batch installation, typed runners, neutral
metadata/Cobra boundary, invocation state separation, native reports, and
selected-module release launcher are substantially in place.
The review ran the existing deterministic suites for core, ckconfig, and ckprompt
using Go 1.26.8, the existing local workspace, and disabled Go network access;
all passed. Independent workspace-off package/test and module graph checks also
passed, using disposable alternate module files and local replacements only for
the unpublished CommandKit dependencies. Core excludes ConfigKit/PromptKit and
both integrations; ckconfig excludes PromptKit and ckprompt. Those checks
establish local wiring/isolation, not published-version availability.
A disposable public-API program reproduced R1. A separate case confirmed that
omitted pointer input/state factories deliver nil zero values successfully,
supporting the distinction in R3. No implementation files were changed, no
regression tests were added, and no publication was attempted. These are focused
findings, not a claim that passing tests prove the absence of other defects.
Keep the accepted scope intact during remediation. In particular, preserve
caller-owned callback effects and objects, optional capabilities, unused-binding
behavior, supported error identity, native reports, and source-only release
completion. Do not introduce deep cloning, new report fields, public helpers,
provider caches, or permanent validation gates to resolve these issues.
## R1: Capture PromptKit selector handles before retaining the suite
**Status:** Completed (2026-10-11). **Priority:** P1.
**Completed work:** the enabled family copies its selector-handle collection
once, preserving original identities for both generated commands and retained
callbacks. Empty-suite behavior is unchanged. The public consumer regression
replaces a source element after capture and exercises prompt/profile inspection
in two apps reusing the declaration. It failed before the fix and passes after
it; the complete ckprompt suite passes offline with Go 1.26.8.
**Evidence:** [ckprompt/command.go](../../integrations/ckprompt/command.go),
`Command`, `newDefinitionCommand`, and `prepareDefinitionInput`.
`CaptureFlags` captures the original handles for parsing, but the execution
closures retain a shallow copy of `Suite` whose `Selectors` slice still shares
the caller's backing array. `prepareDefinitionInput` subsequently reads that
array when choosing handles for `ApplyTo`.
Construct a suite with one campaign flag, call `ckprompt.Command` and install
it, then replace the original selector slice's element with a separately
constructed flag having the same name. The installed parser still captures the
original handle, while execution selects the replacement. The public-API
reproduction produced:
```text
Before source edit: success, status 0, provider called.
After source edit: operational, status 1, ErrFlagWiring, provider not called.
Cause: flag --campaign was not captured.
```
This violates [ADR 0011's capture decision](../adr/0011-captured-declarations-and-installation.md#uniform-captured-declarations):
later edits to source metadata must not change an already captured declaration.
**Required work:** copy the enabled suite's selector-handle collection once at
family construction and use that captured collection consistently for generated
bindings, input application, and retained callbacks. Preserve original binding
identity; do not reconstruct lookalike flags or clone callback objects. Keep the
empty-suite no-op and retained-error behavior.
**Completion evidence:** public consumer regression coverage for source selector
element edits after capture, both named command capabilities, and reuse across
apps. The original selection and values must remain effective after the edit.
Avoid assertions about a particular private representation.
## R2: Finish declaration-time compilation of configuration metadata
**Status:** Completed (2026-10-11). **Priority:** P2.
**Completed work:** family capture orders scope references and prepares each
scope's original binding list, each operation's binding union, and scope-choice
text. Inspection preparation reads those lists without static sorting or
membership compilation. A behavioral regression covers operation-specific flags
and capture of capabilities/applicability/handles after source edits. Existing
ordering, metadata-only, unused/required-selector, isolated-input, and shared-state
coverage passes; all three module suites pass offline with Go 1.26.8. No provider
cache or performance guarantee was introduced.
**Evidence:** [ckconfig/input.go](../../integrations/ckconfig/input.go),
`newConfigOperationCommand`, `selectorIndexesForOperation`, and
`prepareConfigOperation`; [ckconfig/family.go](../../integrations/ckconfig/family.go),
`captureSuite` and `orderedScopeRecords`.
Operation-specific selector membership is computed at command construction and
then recomputed for every inspection invocation. Preparation scans selector/scope
capabilities again, builds a membership map, and assembles per-scope binding lists.
An aggregate request also copies and sorts scope records again. These inputs
are fixed captured metadata, not invocation-dependent configuration.
The selected
[backend design](https://gitea.maximumdirect.net/eric/CommandKit/src/commit/0d23676144ab1ce5047924e98f1ab6a2a9d6aeb6/docs/roadmap/registration-backend.md#integration-preparation-without-parallel-execution-frameworks)
calls for ordered scope records and applicable original selector bindings to be
prepared at family construction. The current code correctly shares one
invocation plan between checking and running, but leaves this static compilation
incomplete. No latency or allocation measurements are claimed here.
**Required work:** capture ordered scope references and each operation's relevant
selector handles/per-scope binding lists once. Invocation preparation should
resolve the requested selection, perform availability/input checks, and apply
those lists. Keep this as ordinary private metadata, without a cache framework,
bitsets, process-global state, or a provider-acquisition cache.
**Completion evidence:** inspect the invocation path for absence of repeated
static sorting/membership compilation. Retain behavioral coverage for ordering,
partial capabilities, metadata-only scopes, unused selectors, required selected
flags, input isolation, and shared state. Do not add a performance threshold or
require callbacks to run after failure.
## R3: Make public documentation precise about ownership and the final API
**Status:** Completed (2026-10-11). **Priority:** P2.
**Completed work:** the suites' GoDoc now specifies initialization timing,
omitted-factory zero values (including nil pointers), early-failure limits, and
caller ownership of objects, aliases, and effects. Family GoDoc and consumer
guidance link to those owners. Inspection claims are qualified, the composed
example describes `NewInput`/`NewState`, and `Scope.Summary` no longer promises
discovery output. Reviewed against current preparation/acquisition code and the
example; current consumer/example Markdown has no obsolete context API names.
Changed local links/fragments and whitespace checks pass. Runtime rules and
report fields are unchanged.
**Evidence and corrections:**
- [Consumer guide](../consumers/guide.md#promptkit-configuration-and-definitions)
says each inspection gets a fresh input, while
[ckconfig's factory comments](../../integrations/ckconfig/contracts.go) describe
fresh storage without documenting omitted factories and caller-owned results.
State the selected initialization timing and zero-value behavior, including
nil pointers. Distinguish separate initialization calls from a guarantee that
caller factories return distinct objects.
- The consumer guide and [ckprompt README](../../integrations/ckprompt/README.md)
make unqualified statements that inspection does not execute prompts, contact
providers, or resolve credentials. Limit these statements to CommandKit-owned
inspection code. The [CLI reference](../cli.md#promptkit-definition-inspection)
already distinguishes application acquisition/direct callback effects.
- The [composed example README](../../integrations/ckprompt/examples/composed/README.md)
still refers to `ConfigSuite.NewContext` and shared scope contexts. Its
implementation uses `Suite[I, S]`, separate `NewInput` initialization, and
`NewState` for the shared cache. Replace the obsolete description.
- `Scope.Summary`'s GoDoc in
[contracts.go](../../integrations/ckconfig/contracts.go) says it is shown when
a scope is listed. Current discovery renders name and operations only, and
does not use the summary. Correct the comment without inventing a report-field
requirement or changing the selected discovery format.
[ADR 0011](../adr/0011-captured-declarations-and-installation.md#separate-typed-input-from-shared-acquisition-state)
explicitly leaves factory objects, aliases, and callback effects with the caller.
The pointer-zero-value reproduction succeeds today; the runtime must not be
changed to require allocation or distinct pointers merely to match stronger
documentation.
**Required work:** put exact lifecycle/ownership contracts in the owning exported
declarations and link to them from task guidance. Qualify broader descriptions
accordingly. Preserve arbitrary caller object ownership and the absence of
automatic deep copies or concurrency promises.
**Completion evidence:** review the updated GoDoc, guide, and example descriptions
against real code, including omitted factories, intentionally shared objects,
early failure, and caller-owned effects. Check for remaining obsolete public API
names. This is documentation work, not a request for additional runtime rules.
## R4: Complete the module-specific release-note policy adaptation
**Status:** Completed (2026-10-11). **Priority:** P2.
**Completed work:** release policy now places notes relative to the selected
module and distinguishes semantic-version filenames/titles from full Git tags
and immutable source URLs. Checked all three prepared notes against their profile
products, versions, full-tag footers, and existing module-local destinations;
they agree with the runbook and ReleaseKit v0.4.0's public module-release ADR.
The added local link/fragment and whitespace checks pass. Independent versioning,
immutability, and source-push completion rules remain intact; no tags were created.
**Evidence:** [release policy](../policy/release.md#release-documentation-and-preparation)
still requires `docs/releases/<version>.md` in the tagged source tree and says
to use the complete version tag as the filename. The
[release procedure](../release.md#modules-versions-notes-and-shared-files),
module notes, and ReleaseKit v0.4.0 correctly use module-local note directories
and a semantic-version filename. A nested full tag contains a module prefix;
it is not that filename.
For example, ckconfig's prepared note is
`integrations/ckconfig/docs/releases/v0.1.0.md`, with full tag
`integrations/ckconfig/v0.1.0`. The generic policy wording does not describe
that selected arrangement. Updating only the policy's release-scope paragraph
left the controlling note requirement ambiguous/conflicting.
**Required work:** adapt the CommandKit release-note rule to the selected module
directory and semantic version; distinguish semantic version, full Git tag,
product title, note path, and immutable-link identity. Preserve immutability,
independent versioning, and the successful-source-publication completion point.
No accepted ADR decision text needs revision.
**Completion evidence:** the policy, runbook, three prepared notes, and released
ReleaseKit contract agree for root and nested modules. This requires document
review, not real tag creation or publication.
## R5: Describe actual release file inputs and enabled checks
**Status:** Completed (2026-10-11). **Priority:** P2.
**Completed work:** the runbook names the common explicit repository-root
shared-file list, separates module-local distribution notices, and distinguishes
existence requirements from optional content checks. It describes the enabled
suite/candidate/publication checks without promising automated licensing checks.
Compared all three unchanged profiles with ReleaseKit v0.4.0's public field
semantics and confirmed the listed files and local links/fragments exist.
Whitespace checks pass. No additional release gates were enabled.
**Evidence:** [release.md](../release.md#modules-versions-notes-and-shared-files)
lists module-local files as the integrations' shared project files and says
integration publication validates those matching files. In contrast,
all three [profiles](../../releasekit.json), including
[ckconfig](../../integrations/ckconfig/releasekit.json) and
[ckprompt](../../integrations/ckprompt/releasekit.json), explicitly select the
repository-root `COPYRIGHT`, `LICENSE`, and `docs/oss/attribution.md`.
The profiles enable no optional file checks. Under the released
[ReleaseKit profile contract](https://gitea.maximumdirect.net/eric/ReleaseKit/src/commit/b96262eb0ce9c8f2eb3e948a5841db2b799bc91b/docs/config.md),
an explicit shared file must exist even with no file checks enabled, but naming
it does not enable content checks. Module-local files belong to automatic module
inputs when applicable; that does not itself establish a license/content
validation guarantee. The current runbook conflates these distinctions.
**Required work:** document the selected root shared-file list accurately, explain
the separate module-local distribution materials, and state what the currently
enabled validation actually checks. Preserve the selected profiles and one
standard suite per selected module. Do not enable additional gates to make the
overstatement true.
**Completion evidence:** compare the runbook with all three JSON profiles and
v0.4.0's public field semantics. Module-local licensing obligations remain intact
without an invented automated content-check promise.
## R6: Remove the obsolete binding-clear mechanism
**Status:** Completed (2026-10-11). **Priority:** P3.
**Completed work:** verified the field had only constructor assignments and no
callers, then removed it and the three unused closures. The removal leaves
prepared value application, defaults, presence, and binding identity untouched.
A follow-up source search finds no remaining binding-clear mechanism. All three
module suites pass offline with Go 1.26.8; formatting and whitespace checks pass.
No reset hook or public API change was introduced.
**Evidence:** [command.go](../../command.go) retains
`flagBinding.clear`, and [binding.go](../../binding.go) constructs clear
closures for singleton, boolean, and repeatable flags. The old selector
projection code that consumed these closures was removed; current production
code does not read or call the field.
This is prototype machinery left behind by the input/state refactor. It adds
unnecessary binding state and closure construction while suggesting that
clearing non-applicable fields is still part of the implementation.
**Required work:** remove the unused field and closure construction. Keep prepared
value application, defaults, presence, and original binding identity unchanged.
Do not replace it with a new reset hook or clear initialized inapplicable fields.
**Completion evidence:** verify there are no callers before removal, then run
the affected existing suites. Public behavior and exports remain unchanged.
## R7: Correct the scope descriptions in module-local attribution
**Status:** Open. **Priority:** P3.
**Evidence:** [ckprompt attribution](../../integrations/ckprompt/docs/oss/attribution.md)
labels ConfigKit as a transitive dependency, but
[adapters.go](../../integrations/ckprompt/adapters.go) imports it directly and
[go.mod](../../integrations/ckprompt/go.mod) lists it as a direct requirement.
[ckconfig attribution](../../integrations/ckconfig/docs/oss/attribution.md)
also reproduces the root policy-material entry, identifying
`docs/policy/*`, `AGENTS.md`, and `docs/development.md` without identifying
their root/core scope. Those files are outside the ckconfig module distribution.
The [OSS policy](../policy/oss.md) requires factual component/material scope;
the implementation target requires module-local notices relevant to actual
distributed materials and dependencies.
**Required work:** correct the direct-dependency description. Review the copied
policy-material entry against the module distribution: identify it explicitly
as root/core dependency material if retained, or omit it if inapplicable to
that notice's scope. Preserve applicable attribution, notices, grants, and
verbatim license texts; do not infer new licensing obligations or relicense code.
**Completion evidence:** module-local attribution describes its actual imports,
dependency roles, and material locations accurately, with working local license
links. Repository-wide provenance remains owned by the root notices/policy
provenance documents.