diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..a3d0a9e --- /dev/null +++ b/docs/roadmap/remediation.md @@ -0,0 +1,258 @@ +# Implementation review remediation + +Reviewed on 2026-10-11, against committed implementation +`cc574915ee17e2d738f27874d2c8857aa1f51a47`. The implementation sprint starts +after planning commit `0d23676144ab1ce5047924e98f1ab6a2a9d6aeb6`. +All issues below are open. 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:** Open. **Priority:** P1. + +**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:** Open. **Priority:** P2. + +**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:** Open. **Priority:** P2. + +**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:** Open. **Priority:** P2. + +**Evidence:** [release policy](../policy/release.md#release-documentation-and-preparation) +still requires `docs/releases/.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:** Open. **Priority:** P2. + +**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:** Open. **Priority:** P3. + +**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.