302 lines
17 KiB
Markdown
302 lines
17 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:** 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.
|