diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 5158a79..8ac605e 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,150 +1,112 @@ -# ReleaseKit adoption review findings +# Performance implementation review findings -Status: R-01 and R-02 complete. +Status: R-01 requires remediation. -Date: 2026-10-08. +Date: 2026-10-09. ## Review scope and authority -Reviewed CommandKit at `c0c03285f7beac3e96f1708b655c1eb2add35e79`, covering the -six adoption commits from `ada2021` through `c0c0328` against the -[ReleaseKit target at the reviewed revision](https://gitea.maximumdirect.net/eric/CommandKit/src/commit/c0c03285f7beac3e96f1708b655c1eb2add35e79/docs/roadmap/releasekit.md), the completed -[implementation record at the reviewed revision](https://gitea.maximumdirect.net/eric/CommandKit/src/commit/c0c03285f7beac3e96f1708b655c1eb2add35e79/docs/roadmap/implementation.md), -and applicable policies. The previously completed documentation reconciliation -record remains available in -[Git history](https://gitea.maximumdirect.net/eric/CommandKit/src/commit/c0c03285f7beac3e96f1708b655c1eb2add35e79/docs/roadmap/remediation.md). +Reviewed the performance implementation at `18ff0d6`, covering the changes after +`1274ae7` against the [efficiency target](performance.md), selected OD-1/OD-2 +approaches, [implementation record](implementation.md), accepted +[ADRs](../adr/), and applicable architecture, agent, and testing policies. -The target adoption is present: a released Go tool selection, minimal source-only -profile, native launcher, fast subprocess tests, opt-in local publication -exercise, and implemented runbook/navigation. The commits do not change -production Go source or public declarations. Package dependency inspection found -no ReleaseKit, Goldmark, or `golang.org/x/mod` runtime dependency in the root -library or internal Cobra adapter. `scripts` contains external test files only. +The completed ReleaseKit review findings remain available in +[Git history](https://gitea.maximumdirect.net/eric/CommandKit/src/commit/18ff0d6/docs/roadmap/remediation.md). +Release tooling is outside this implementation review. -The remaining findings concern test maintainability and ambiguous normative -wording. Neither establishes a missing production feature or a demonstrated -publication failure. Fix them within the existing target; do not add release -gates, new public contracts, or additional publisher machinery. +## Architecture assessment -## R-01: avoid making bootstrap command details test contracts +The selected structure is present: -**Priority:** low. **Category:** code quality and unintended test constraints. +- The root facade owns declarations, registration, typed inputs, standard + handlers, and invocation outcomes. Cobra and pflag remain inside the internal + adapter; the changes add no exported API declarations. +- Each `App` lazily retains immutable neutral command metadata. Successful + nonempty registration invalidates it; failed and empty registrations leave + it intact. Pending declarations are validated and cloned before installation + without copying the previously installed registry. +- Each invocation prepares a fresh mutable adapter session, reusing it for + parsing, help, and usage. Presentation can refresh metadata after callback + registration without rerunning input callbacks. Backend commands map directly + to declarations; parent links reconstruct the selected path. +- Configuration registration prepares ordered scopes, name lookup, installed + operations, and discovery metadata. Invocation contexts and acquisition remain + live callbacks. PromptKit builds the requested report format, and aggregate + configuration framing scans only the YAML preamble while retaining the + selected full native and framed buffers. -**Location:** [scripts/release_test.go](../../scripts/release_test.go), -`TestReleaseLauncherDelegatesPublication`, `assertNativeBootstrap` -and `fakeGo` as they existed at the reviewed revision. +These changes implement PF-01–PF-06. The dispatch defect below is an ownership +regression in the session integration, rather than a missing subsystem. The +[internal overview](../internal/overview.md) owns the maintained package inventory. -The assertion compares the entire build argument vector, including flag order -and the absolute spelling of the output path. It also requires the two exact -`env GOHOSTOS` and `env GOHOSTARCH` query forms. The executable double supplies -only the first requested environment value, so a combined query would fail even -if the launcher parsed both host values correctly. +## R-01: restrict built-in help interception to the root command -The publication assertion similarly compares a complete argument vector, -requiring `--repo` to precede `--config` even though the upstream CLI accepts -either order before the version operand. +**Priority:** high. **Category:** dispatch correctness, application ownership, +and unintended restriction. -For example, reordering build flags, using a relative output path resolved from -the already verified repository root, or combining host queries can preserve -the selected behavior while failing these tests. The target requires native -bootstrap from the module-selected package, readonly metadata, the selected -toolchain/workspace settings, and the intended executable destination. It does -not make these equivalent command spellings permanent contracts. +**Location:** [execute.go](../../execute.go), `App.Execute`, line 45 at the +reviewed revision. Introduced by `a7960e5`. -The [testing policy](../policy/testing.md#test-behavior-not-implementation) and -[agent policy](../policy/agents.md#implementation-discipline) call for protecting -those selected behaviors without elevating implementation details. +The dispatcher selects a leaf declaration and then checks `command.name == +"help"` to identify the built-in help operation. That matches an application +command named `help` at any depth. Its ordinary typed input is then asserted to +be CommandKit's private `*helpInput`, producing an operational failure instead +of invoking the registered application handler. + +A public API reproduction created a runnable `help` command with `struct{}` +input, composed it beneath the application-owned `session` group, and passed the +group to `New`. Construction and registration succeeded. Execution produced: + +```text +args=[session help] kind=operational status=1 cause=built-in help input has an unexpected type handlerCalls=0 +args=[help session] kind=success status=0 cause= handlerCalls=0 +args=[session help --help] kind=success status=0 cause= handlerCalls=0 +``` + +The first invocation must reach the application handler. The remaining two are +metadata help requests and appropriately leave that handler uninvoked. + +[ADR 0001](../adr/0001-command-dispatch-and-inputs.md#ownership-and-namespace-structure) +and the [architecture policy](../policy/architecture.md#command-and-application-ownership) +leave custom names and semantics with applications outside standard families. +[ADR 0006](../adr/0006-command-grammar.md) also encourages consistent verbs, +including `help`. The selected session integration specifically concerns the +reserved **top-level** help command. Matching only the leaf name effectively +introduces a global reservation of that name, although registration still +accepts it. No such additional restriction was selected. Required work: -- [x] Assert the required build settings, selected package, and resolved output - destination without requiring one incidental flag ordering or path spelling. -- [x] Make host-query fixture handling and assertions reflect the requested - host values rather than requiring separate subprocess query forms. Keep the - fixture small; a complete Go command emulator is unnecessary. -- [x] Check the publication command, option values, version operand, and - argument boundaries without fixing the order of independent options. Retain - the upstream requirement that options precede the version. -- [x] Preserve checks for wrong native target, missing readonly mode, incorrect - package/output, workspace or toolchain mistakes, failed bootstrap, and - publication status/stream propagation. +- [ ] Identify the root built-in help command by its complete selected path + or an equivalent explicit reserved-command identity. A root-path check such + as `len(parsed.Path) == 1 && parsed.Path[0] == "help"` is sufficient for the + current registry. Other runnable declarations must continue to their handler. +- [ ] Add a public behavior regression for an application-owned nested `help` + command. Verify it receives its typed input and that its handler outcome is + preserved. Keep local `--help` and `help session help` as metadata operations. +- [ ] Retain existing root help, invalid-target, usage, fresh-invocation, + registration-refresh, and diagnostic behavior. Use existing coverage where + sufficient; verify the relevant regression and default suite after the fix. -Completion evidence: focused launcher checks still detect the selected failures -and accept an equivalent bootstrap form. This is a test correction, not a -requirement to refactor the working launcher or add a release-time check. +Do not remediate this by rejecting nested `help` names or imposing new command +naming rules. Keep the session optimization and established ownership boundary. +Mark this item complete only after the correction and its evidence are recorded. -**Completed 2026-10-08:** bootstrap checks now inspect required options and -resolve output paths; publication checks use the standard flag parser to preserve -option-order freedom and operand boundaries. The small executable double returns -each requested host value and accepts either output-option spelling. A fixture -launcher with combined host queries, reordered options, and a relative output -passed. Negative cases still reject incorrect target, workspace/toolchain, -directory, readonly mode, package pin, output, publication paths, and operands; -existing status/stream and bootstrap-failure checks passed. +## Evidence and limits -Validation: offline `go test -mod=readonly -count=1 ./scripts` and -`go test -mod=readonly -count=1 ./...` both passed with `GOWORK=off`, -`GOTOOLCHAIN=local`, `GOPROXY=off`, `GONOPROXY=none`, and `GOSUMDB=off`. -The production launcher, profile, and module selection were not changed. - -## R-02: clarify optional diagnostic validation in the runbook - -**Priority:** low. **Category:** potentially strengthened prohibition. - -**Location:** [docs/release.md](../release.md), lines 76–77 at the reviewed revision. - -The instruction “Do not require a separate `check` before `publish` or repeat -the same suite” combines the intended rule against mandatory duplicate -validation with wording that can be read as prohibiting an operator from -voluntarily rerunning diagnostics. - -The [release policy](../policy/release.md#candidate-validation) permits a -validation-only diagnostic command and says it must not be a required -prerequisite before publication repeats the checks. Avoiding automatic duplicate -validation does not establish a blanket prohibition on optional diagnostic runs. -The launcher correctly delegates once; this finding concerns the runbook's -description, not its executable behavior. - -Required work: - -- [x] Say explicitly that `check` is optional for diagnosis and is not a - required step before `publish`. -- [x] Scope the duplicate-validation instruction to the automated publication - workflow and configured additional checks. Preserve operator discretion to - investigate or rerun a check. -- [x] Retain the single publication command and its own final-candidate suite; - do not add a required preliminary check or an extra confirmation. - -Completion evidence: document review confirms the runbook distinguishes an -optional diagnostic action from a release requirement and agrees with the -existing policy. No implementation change is needed for this item. - -**Completed 2026-10-08:** the runbook explicitly makes `check` an optional -diagnostic action and preserves maintainer discretion to rerun diagnostics. -Duplicate-validation guidance now concerns the automated publication workflow -and configured additional checks. The single publisher and its own -final-candidate suite remain the release procedure. - -Validation: reviewed the changed wording against the release policy and -implemented launcher; local documentation links and fragments and Markdown -whitespace checks passed. This documentation-only unit did not change code or -rerun the suite already passed for R-01. - -## Review evidence and limits - -- Read the adoption diffs, launcher/profile, new test functions and fixtures, - runbook, policy edits, historical navigation changes, and completion records. -- Inspected package dependencies with offline module access and confirmed the - production boundary and tests-only `scripts` package. Verified the committed - launcher mode and ignored local executable storage. -- Reviewed what the tests actually assert, including the local fixture's exact - tag-object comparison, candidate commit, one suite marker, clean worktree, and - endpoint-mismatch preflight behavior. The completion record reports earlier - successful default and opt-in runs; this review did not rerun those tests or - attempt publication. -- No other adoption gap, newly imposed release gate, public API expansion, or - runtime publication-code defect was identified in the reviewed changes. - -Leave source publication, hosted delivery, binary assets, and other explicitly -deferred capabilities outside this remediation work. Update item status only -after the relevant correction and evidence are complete. +- Reviewed the production diffs, changed behavioral tests, benchmark fixtures, + architecture documentation, and completion/measurement records. Traced cache + invalidation, session refresh, input preparation, scope lookup, acquisition, + YAML framing, and requested-format delivery. +- Executed the public consumer reproduction above from a temporary file outside + the repository with readonly, offline module access. No repository test or + production code was added or changed during this review. +- The implementation record reports a successful default suite at `62b4a48`, + the last production-code revision. This review did not rerun that suite or the + benchmarks. Allocation improvements and registration tradeoffs are documented + there; its final cross-run timing comparison is explicitly inconclusive under + differing host load. This review does not establish a latency guarantee. +- No other issue requiring remediation was identified in the reviewed scope. + The changes do not establish concurrent dispatch, application startup rules, + provider-result caching, new release gates, or additional public API promises.