Record the nested help dispatch regression
This commit is contained in:
@@ -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=<nil> handlerCalls=0
|
||||
args=[session help --help] kind=success status=0 cause=<nil> 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.
|
||||
|
||||
Reference in New Issue
Block a user