Keep nested help commands with application handlers
This commit is contained in:
@@ -1,6 +1,6 @@
|
|||||||
# Performance implementation review findings
|
# Performance implementation review findings
|
||||||
|
|
||||||
Status: R-01 requires remediation.
|
Status: R-01 complete; all recorded remediation work is addressed.
|
||||||
|
|
||||||
Date: 2026-10-09.
|
Date: 2026-10-09.
|
||||||
|
|
||||||
@@ -36,8 +36,8 @@ The selected structure is present:
|
|||||||
configuration framing scans only the YAML preamble while retaining the
|
configuration framing scans only the YAML preamble while retaining the
|
||||||
selected full native and framed buffers.
|
selected full native and framed buffers.
|
||||||
|
|
||||||
These changes implement PF-01–PF-06. The dispatch defect below is an ownership
|
These changes implement PF-01–PF-06. The review identified an ownership
|
||||||
regression in the session integration, rather than a missing subsystem. The
|
regression in the session integration, corrected under R-01 below. The
|
||||||
[internal overview](../internal/overview.md) owns the maintained package inventory.
|
[internal overview](../internal/overview.md) owns the maintained package inventory.
|
||||||
|
|
||||||
## R-01: restrict built-in help interception to the root command
|
## R-01: restrict built-in help interception to the root command
|
||||||
@@ -78,14 +78,14 @@ accepts it. No such additional restriction was selected.
|
|||||||
|
|
||||||
Required work:
|
Required work:
|
||||||
|
|
||||||
- [ ] Identify the root built-in help command by its complete selected path
|
- [x] Identify the root built-in help command by its complete selected path
|
||||||
or an equivalent explicit reserved-command identity. A root-path check such
|
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
|
as `len(parsed.Path) == 1 && parsed.Path[0] == "help"` is sufficient for the
|
||||||
current registry. Other runnable declarations must continue to their handler.
|
current registry. Other runnable declarations must continue to their handler.
|
||||||
- [ ] Add a public behavior regression for an application-owned nested `help`
|
- [x] Add a public behavior regression for an application-owned nested `help`
|
||||||
command. Verify it receives its typed input and that its handler outcome is
|
command. Verify it receives its typed input and that its handler outcome is
|
||||||
preserved. Keep local `--help` and `help session help` as metadata operations.
|
preserved. Keep local `--help` and `help session help` as metadata operations.
|
||||||
- [ ] Retain existing root help, invalid-target, usage, fresh-invocation,
|
- [x] Retain existing root help, invalid-target, usage, fresh-invocation,
|
||||||
registration-refresh, and diagnostic behavior. Use existing coverage where
|
registration-refresh, and diagnostic behavior. Use existing coverage where
|
||||||
sufficient; verify the relevant regression and default suite after the fix.
|
sufficient; verify the relevant regression and default suite after the fix.
|
||||||
|
|
||||||
@@ -93,7 +93,33 @@ Do not remediate this by rejecting nested `help` names or imposing new command
|
|||||||
naming rules. Keep the session optimization and established ownership boundary.
|
naming rules. Keep the session optimization and established ownership boundary.
|
||||||
Mark this item complete only after the correction and its evidence are recorded.
|
Mark this item complete only after the correction and its evidence are recorded.
|
||||||
|
|
||||||
## Evidence and limits
|
### Completion evidence
|
||||||
|
|
||||||
|
**Completed:** `App.Execute` now recognizes the built-in help command only when
|
||||||
|
its selected path is the root `help` command. Nested application commands keep
|
||||||
|
their existing handler path and naming freedom; session reuse is retained.
|
||||||
|
|
||||||
|
The public regression `TestExecuteRunsApplicationOwnedNestedHelp` verifies a
|
||||||
|
required typed operand reaches the nested handler and preserves its domain,
|
||||||
|
status, and cause. Local `session help --help` and explicit `help session help`
|
||||||
|
render the command's metadata without invoking that handler or requiring its
|
||||||
|
execution operand.
|
||||||
|
|
||||||
|
The regression first failed against the original dispatcher with the reported
|
||||||
|
unexpected-input-type failure and passed after the correction. Focused validation
|
||||||
|
and the full default suite passed with readonly, offline module access:
|
||||||
|
|
||||||
|
```sh
|
||||||
|
GOWORK=off GOTOOLCHAIN=local GOPROXY=off GONOPROXY=none GOSUMDB=off go test -mod=readonly -count=1 . -run '^TestExecuteRunsApplicationOwnedNestedHelp$'
|
||||||
|
GOWORK=off GOTOOLCHAIN=local GOPROXY=off GONOPROXY=none GOSUMDB=off go test -mod=readonly -count=1 ./...
|
||||||
|
```
|
||||||
|
|
||||||
|
Existing root help, invalid-target, usage, fresh-invocation, registration-refresh,
|
||||||
|
and diagnostic tests passed. Changed Go files were formatted; document links
|
||||||
|
and whitespace checks passed. No new public contract or command-name restriction
|
||||||
|
was introduced.
|
||||||
|
|
||||||
|
## Original review evidence and limits
|
||||||
|
|
||||||
- Reviewed the production diffs, changed behavioral tests, benchmark fixtures,
|
- Reviewed the production diffs, changed behavioral tests, benchmark fixtures,
|
||||||
architecture documentation, and completion/measurement records. Traced cache
|
architecture documentation, and completion/measurement records. Traced cache
|
||||||
|
|||||||
@@ -42,7 +42,7 @@ func (a *App) Execute(ctx context.Context, args []string, streams Streams) Outco
|
|||||||
if err := command.payload.checkInput(input); err != nil {
|
if err := command.payload.checkInput(input); err != nil {
|
||||||
return current.present(Usage(err), parsed.Path)
|
return current.present(Usage(err), parsed.Path)
|
||||||
}
|
}
|
||||||
if command.name == "help" {
|
if len(parsed.Path) == 1 && parsed.Path[0] == "help" {
|
||||||
help, ok := input.(*helpInput)
|
help, ok := input.(*helpInput)
|
||||||
if !ok {
|
if !ok {
|
||||||
return current.present(Failure(errors.New("built-in help input has an unexpected type")), parsed.Path)
|
return current.present(Failure(errors.New("built-in help input has an unexpected type")), parsed.Path)
|
||||||
|
|||||||
@@ -340,6 +340,61 @@ func TestExecuteHelpSkipsRequiredInputsAndRejectsNilContext(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestExecuteRunsApplicationOwnedNestedHelp(t *testing.T) {
|
||||||
|
type input struct{ topic commandkit.Supplied[string] }
|
||||||
|
var received []input
|
||||||
|
domainCause := errors.New("application help result")
|
||||||
|
help, err := commandkit.NewCommand(commandkit.CommandSpec[input]{
|
||||||
|
Name: "help",
|
||||||
|
Description: "Explain a session topic.",
|
||||||
|
Arguments: []commandkit.Argument[input]{commandkit.ValueArgument(
|
||||||
|
commandkit.ArgumentMeta{Name: "TOPIC"}, true,
|
||||||
|
func(value string) (string, error) { return value, nil },
|
||||||
|
func(target *input, value commandkit.Supplied[string]) { target.topic = value },
|
||||||
|
)},
|
||||||
|
Run: func(_ commandkit.Invocation, value input) commandkit.Outcome {
|
||||||
|
received = append(received, value)
|
||||||
|
return commandkit.Domain("session-help", 7, domainCause)
|
||||||
|
},
|
||||||
|
})
|
||||||
|
if err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
session, err := commandkit.Group("session", "Session commands.", help)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
app, err := commandkit.New(commandkit.Identity{Name: "example"}, session)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
|
||||||
|
var diagnostic bytes.Buffer
|
||||||
|
outcome := app.Execute(context.Background(), []string{"session", "help", "restore"}, commandkit.Streams{Err: &diagnostic})
|
||||||
|
if outcome.Kind != commandkit.OutcomeDomain || outcome.Domain != "session-help" || outcome.Status != 7 || !errors.Is(outcome.Cause, domainCause) {
|
||||||
|
t.Fatalf("nested help handler outcome = %#v, diagnostics %q", outcome, diagnostic.String())
|
||||||
|
}
|
||||||
|
if len(received) != 1 || received[0].topic != (commandkit.Supplied[string]{Value: "restore", Set: true}) {
|
||||||
|
t.Fatalf("nested help handler input = %#v", received)
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, args := range [][]string{{"session", "help", "--help"}, {"help", "session", "help"}} {
|
||||||
|
t.Run(strings.Join(args, " "), func(t *testing.T) {
|
||||||
|
var output, diagnostics bytes.Buffer
|
||||||
|
outcome := app.Execute(context.Background(), args, commandkit.Streams{Out: &output, Err: &diagnostics})
|
||||||
|
if outcome.Kind != commandkit.OutcomeSuccess || outcome.Status != 0 || diagnostics.Len() != 0 {
|
||||||
|
t.Fatalf("metadata help outcome = %#v, diagnostics %q", outcome, diagnostics.String())
|
||||||
|
}
|
||||||
|
if !strings.Contains(output.String(), "Explain a session topic.") || !strings.Contains(output.String(), "TOPIC") {
|
||||||
|
t.Fatalf("metadata help omitted nested command details: %q", output.String())
|
||||||
|
}
|
||||||
|
if len(received) != 1 {
|
||||||
|
t.Fatalf("metadata help invoked the application handler: %#v", received)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
func TestHelpRendersRegisteredMetadataAndResolvesExactPaths(t *testing.T) {
|
func TestHelpRendersRegisteredMetadataAndResolvesExactPaths(t *testing.T) {
|
||||||
type input struct {
|
type input struct {
|
||||||
count commandkit.Supplied[int]
|
count commandkit.Supplied[int]
|
||||||
|
|||||||
Reference in New Issue
Block a user