Delegate help and completion to extensions when applicable - #1137
Delegate help and completion to extensions when applicable#1137jeri-temporal wants to merge 4 commits into
Conversation
Shell completion always sets __complete as the first argument, so to delegate to extensions, `temporal __complete cloud n` needs to be rewritten as `temporal-cloud __complete n`. `help` can be invoked the same way
| // the current command's path in the hierarchy. | ||
| // so they appear in shell completion and the default help output. It filters extensions | ||
| // based on the current command's path in the hierarchy. | ||
| func registerExtensionCommands(cmd *cobra.Command) { |
There was a problem hiding this comment.
Now that this is used for shell completion as well, it feels like it probably belongs in commands.extension.go, but I didn't want to move it at the same time as making modifications to its behavior. If you agree, I'm happy to move it
There was a problem hiding this comment.
Pull request overview
This PR updates Temporal CLI’s extension-dispatch logic so that help and Cobra shell completion (__complete / __completeNoDesc) can be delegated to matching extensions, and so extensions appear in shell completion results at the root command.
Changes:
- Teach
tryExecuteExtensionto rewrite/delegatehelpand shell completion requests to matching extensions when applicable. - Register discovered extensions as placeholder Cobra commands during shell completion so they show up in completion candidates.
- Update extension discovery to return executable paths and add/extend unit tests around completion and built-in command shadowing.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/temporalcli/commands.help.go | Enhances extension placeholder registration to support nested extension paths and show executable locations in help/completion. |
| internal/temporalcli/commands.go | Registers extension placeholder commands during __complete when no extension handled the request. |
| internal/temporalcli/commands.extension.go | Adds delegated-command splitting for help/completion, adjusts extension execution args, and changes extension discovery to return executable paths. |
| internal/temporalcli/commands.extension_test.go | Adds tests for extension visibility in completion and ensures built-ins (like completion) are not shadowed. |
Suppressed comments (1)
internal/temporalcli/commands.extension.go:133
splitDelegatedCommandsalways strips the leadinghelptoken, which makestcmd.Command.Find()run against the remaining args. For invocations liketemporal help --all, that causes--all(a help-command flag) to be parsed against the root command, leading to an "unknown flag: --all" error and breaking built-in help.
if args[0] == "help" {
// "help __complete" never delegates, whatever comes after, so we can just mark "help" as delegatable and see what matches
return args[:1], args[1:]
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
ks-temporal
left a comment
ks-temporal
left a comment
There was a problem hiding this comment.
Approving with a few minor comments, inline in code.
|
|
||
| // splitDelegatedCommands separates out commands that should be delegated to an extension | ||
| // from the rest of the args given. These commands are inherently position-dependent, so they're | ||
| // only treated specially when they're at the start of the list of arguments. |
There was a problem hiding this comment.
Suggestion: Is it worth mentioning in the comment that the returned slices should be treated as read-only, and some operations such as append must not be done on them?
| path := extensionBinaryToCommandPath(baseName) | ||
| key := strings.Join(path, "/") | ||
| if seen[key] { | ||
| key := strings.Join(path, " ") |
There was a problem hiding this comment.
Suggestion: since filename can contain " " but not "/", could this cause ambiguity if baseName itself has space in some path fragment? e.g., baseName of "temporal-my command" vs "temporal-my-command" both will map to the same key I think. I don't think we support cli command component with space like temporal "my command" anyway, but to avoid any confusion, I suggest to keep the old delimiter of "/" if possible. One option could be to return an array of items (containing cmd path with space, and exe path) instead of map (from cmd path with space to exe path), and use the previous "seen" logic. If you decide to change this, some code in the caller func (registerExtensionCommands) will need to be updated too. Thanks.
| if i == len(extPath)-1 { | ||
| short = fmt.Sprintf("An extension command located at %s", executablePath) | ||
| } else { | ||
| short = fmt.Sprintf("Extension commands under %s", strings.Join(ext[:len(cmdPath)+i+1], " ")) |
There was a problem hiding this comment.
Suggestion: is it possible to include this code path in a test?
Shell completion always sets __complete as the first argument, so to
delegate to extensions,
temporal __complete cloud nneeds to berewritten as
temporal-cloud __complete n.helpcan be invoked thesame way
Related issues
CLDDX-150
What changed?
tryExecuteExtensiondelegateshelpand shell completion to extensions that match the remaining argumentsChecklist
Design
Tests
func TestXxx) where applicableManual tests
Setup
Install at least one extension, like the cloud cli (
brew install temporalio/prerelease/temporal-cloudon Mac or Linux with homebrew installed)Happy path
Error case
(no completions shown)
Behavior remains unchanged when an unknown argument is passed to
temporal help: