feat(completion): add autocompletion for IDs, output and runtime#86
Merged
feloy merged 5 commits intokortex-hub:mainfrom Mar 23, 2026
Merged
feat(completion): add autocompletion for IDs, output and runtime#86feloy merged 5 commits intokortex-hub:mainfrom
feloy merged 5 commits intokortex-hub:mainfrom
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughAdds Cobra CLI autocompletion helpers for workspace ID arguments (filtered by instance state), output and runtime flag completions; wires completions into workspace and alias commands; and exposes runtimesetup.ListAvailable(). Changes
Sequence Diagram(s)sequenceDiagram
participant Shell as "Shell/Cobra"
participant Cmd as "cobra.Command"
participant AC as "autocomplete helper"
participant FS as "Filesystem"
participant IM as "instances.Manager"
participant RS as "runtimesetup"
Shell->>Cmd: user requests completion
Cmd->>AC: invoke ValidArgsFunction / FlagCompletion
AC->>Cmd: read `--storage` flag
Cmd-->>AC: flag value
AC->>FS: resolve & stat storage path
FS-->>AC: exists? / error
alt storage exists
AC->>IM: NewManager(storage)
IM-->>AC: manager
AC->>IM: List instances
IM-->>AC: instances
AC->>AC: filter by state predicate (running/non-running)
AC->>Cmd: return completions + ShellCompDirectiveNoFileComp
else storage missing or error
AC->>Cmd: return [] + ShellCompDirectiveError or NoFileComp
end
note over Cmd,RS: runtime flag completion flow
Cmd->>RS: ListAvailable()
RS-->>Cmd: available runtimes (excluding "fake")
Cmd->>Shell: runtime completions + ShellCompDirectiveNoFileComp
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/cmd/autocomplete_test.go (2)
424-447: Test only verifies negative case, not positive case.This test confirms that
"fake"is excluded from completions but doesn't verify that legitimate runtimes are returned. Since no runtimes are registered in the test setup,completionsmay be empty, making the "fake not in completions" check vacuously true.Consider registering at least one non-fake runtime and verifying it appears in completions to strengthen this test.
💡 Suggested approach
t.Run("returns available runtimes excluding fake", func(t *testing.T) { t.Parallel() storageDir := t.TempDir() + // Create manager and register runtimes + manager, err := instances.NewManager(storageDir) + if err != nil { + t.Fatalf("failed to create manager: %v", err) + } + // Register fake runtime (should be excluded) + if err := manager.RegisterRuntime(fake.New()); err != nil { + t.Fatalf("failed to register fake runtime: %v", err) + } + // TODO: Register a real runtime (e.g., podman) to verify it IS returned // Create a command with the storage flag cmd := &cobra.Command{} cmd.Flags().String("storage", storageDir, "test storage flag") // Call completion function completions, directive := completeRuntimeFlag(cmd, []string{}, "") // Verify "fake" is not in the completions for _, completion := range completions { if completion == "fake" { t.Errorf("Expected 'fake' runtime to be filtered out, but it was included") } } + // TODO: Verify that registered non-fake runtimes ARE in completions + // if len(completions) == 0 { + // t.Errorf("Expected at least one runtime completion") + // } // Verify directive if directive != cobra.ShellCompDirectiveNoFileComp { t.Errorf("Expected ShellCompDirectiveNoFileComp, got %v", directive) } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/autocomplete_test.go` around lines 424 - 447, The test only checks that "fake" is absent but never ensures any real runtimes are returned; update the test that calls completeRuntimeFlag to register at least one real runtime before invoking it (e.g., create/register a runtime named "node" or "docker" via the same runtime registration API used in production), then assert that the expected runtime appears in the completions and that "fake" is not present, keeping the existing checks for the directive and using the same cmd with cmd.Flags().String("storage", storageDir, ...).
136-155: Test name and setup are misaligned.The test is named "returns empty list when list fails" but the setup doesn't actually cause the list operation to fail—it uses a valid temp directory with no corrupted data. The comment on lines 140-141 mentions "List() may fail on corrupted data" but no corruption is introduced.
Additionally, the assertion on lines 152-154 is very permissive, accepting either empty completions OR an error directive, which doesn't match the specific behavior described in the test name.
Consider either:
- Renaming to "returns empty list when no instances exist" (which matches what's actually tested), or
- Actually inducing a failure condition (e.g., corrupting the storage directory structure)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/autocomplete_test.go` around lines 136 - 155, The test name and assertions don't match the setup: the test uses a valid temp storageDir and doesn't induce List() failure for completeNonRunningWorkspaceID, so either rename the t.Run to "returns empty list when no instances exist" and update the comment and assertion to expect len(completions)==0 and directive==cobra.ShellCompDirectiveNoFileComp, or actually cause List() to fail (e.g., create storageDir then write corrupt data or invalid permissions before calling completeNonRunningWorkspaceID) and keep the current name but assert that directive==cobra.ShellCompDirectiveError; update related variables (storageDir, cmd) and comments accordingly to reflect the chosen approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cmd/autocomplete.go`:
- Around line 21-27: The completion handlers are calling instances.NewManager(),
which creates storage dirs; change the completion code to avoid creating on-disk
state by checking whether the storage path exists before instantiating a
manager. Specifically, in the autocomplete command completion functions that
call instances.NewManager(), first query the runtime storage path (via
runtimesetup or the same config source used elsewhere) and if the path does not
exist/is empty, short-circuit and return no suggestions; otherwise only then
call instances.NewManager(). Alternatively, if instances.NewManager supports a
non-creating option, use that (e.g., a constructor flag like "no-create");
update all occurrences (the calls around instances.NewManager() noted in the
review) to apply this guard so tab-completion never creates directories.
In `@pkg/instances/manager_test.go`:
- Around line 1614-1617: The test currently asserts the fake runtime by checking
runtimes[0] which is order-dependent; change the assertion to check that the
"runtimes" slice contains "fake" (e.g., iterate runtimes or use a helper
contains function) and fail the test if not found so the check is
order-independent—update the assertion around the runtimes variable in the test
(the block that currently does if len(runtimes) > 0 && runtimes[0] != "fake") to
verify containment instead.
---
Nitpick comments:
In `@pkg/cmd/autocomplete_test.go`:
- Around line 424-447: The test only checks that "fake" is absent but never
ensures any real runtimes are returned; update the test that calls
completeRuntimeFlag to register at least one real runtime before invoking it
(e.g., create/register a runtime named "node" or "docker" via the same runtime
registration API used in production), then assert that the expected runtime
appears in the completions and that "fake" is not present, keeping the existing
checks for the directive and using the same cmd with
cmd.Flags().String("storage", storageDir, ...).
- Around line 136-155: The test name and assertions don't match the setup: the
test uses a valid temp storageDir and doesn't induce List() failure for
completeNonRunningWorkspaceID, so either rename the t.Run to "returns empty list
when no instances exist" and update the comment and assertion to expect
len(completions)==0 and directive==cobra.ShellCompDirectiveNoFileComp, or
actually cause List() to fail (e.g., create storageDir then write corrupt data
or invalid permissions before calling completeNonRunningWorkspaceID) and keep
the current name but assert that directive==cobra.ShellCompDirectiveError;
update related variables (storageDir, cmd) and comments accordingly to reflect
the chosen approach.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 12fc07cf-740b-4fa7-91fc-8e3e2a849e9c
📒 Files selected for processing (12)
pkg/cmd/autocomplete.gopkg/cmd/autocomplete_test.gopkg/cmd/init.gopkg/cmd/remove.gopkg/cmd/start.gopkg/cmd/stop.gopkg/cmd/workspace_list.gopkg/cmd/workspace_remove.gopkg/cmd/workspace_start.gopkg/cmd/workspace_stop.gopkg/instances/manager.gopkg/instances/manager_test.go
feloy
force-pushed
the
completion
branch
2 times, most recently
from
2ea49dc to
099da30
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cmd/autocomplete_test.go`:
- Around line 136-154: The test uses t.TempDir() which exists so it won't
simulate a failing List; change storageDir to a non-existent path (e.g.,
filepath.Join(t.TempDir(), "missing")) so manager/List will fail when
completeNonRunningWorkspaceID is called, and tighten the expectation to assert a
deterministic failure: require completions to be empty and directive to equal
cobra.ShellCompDirectiveError; update the test setup where
cmd.Flags().String("storage", storageDir, ...) and the final assertion to check
both len(completions) == 0 and directive == cobra.ShellCompDirectiveError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bbdbe2a3-1ddd-446d-8ab7-20ac00eb303e
📒 Files selected for processing (3)
pkg/cmd/autocomplete.gopkg/cmd/autocomplete_test.gopkg/runtimesetup/register.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/runtimesetup/register.go (1)
63-88: Implementation looks good.The
ListAvailable()function is cleanly implemented and correctly reuses the availability-checking pattern. The internal helperlistAvailableWithFactoriesis a good design choice for testability.One observation: the availability-checking loop (lines 76-81) duplicates the logic in
registerAllWithAvailable(lines 102-108). If this pattern grows, consider extracting a shared helper:♻️ Optional: Extract shared availability filter
// forEachAvailable calls fn for each available runtime from the given factories. func forEachAvailable(factories []runtimeFactory, fn func(runtime.Runtime) error) error { for _, factory := range factories { rt := factory() if avail, ok := rt.(Available); ok && !avail.Available() { continue } if err := fn(rt); err != nil { return err } } return nil }Then both functions can reuse it:
func listAvailableWithFactories(factories []runtimeFactory) []string { var names []string _ = forEachAvailable(factories, func(rt runtime.Runtime) error { names = append(names, rt.Type()) return nil }) return names }No unit tests exist for
ListAvailable()orlistAvailableWithFactories()inregister_test.go. Add test cases to cover these functions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtimesetup/register.go` around lines 63 - 88, Duplicate availability-check logic exists between listAvailableWithFactories and registerAllWithAvailable; extract a small reusable helper (e.g., forEachAvailable) that iterates factories, skips runtimes implementing Available when Available() is false, and invokes a callback for each available runtime; refactor listAvailableWithFactories to call this helper to build the names slice and refactor registerAllWithAvailable to reuse it for registration; add unit tests in register_test.go that exercise ListAvailable() and listAvailableWithFactories() (including at least one factory returning an Available runtime that is not available) to verify filtering behavior.pkg/cmd/init.go (1)
251-258: Check RegisterFlagCompletionFunc errors.Lines 251 and 258 ignore errors from
RegisterFlagCompletionFunc. While failures are unlikely (flags are defined immediately before), Cobra explicitly returns errors when the flag doesn't exist or a function is already registered. This pattern also appears inworkspace_start.go:143,workspace_stop.go:143,workspace_remove.go:140, andworkspace_list.go:149.Consider wrapping with error checks. Since this occurs during command construction (which cannot return errors), log the error and continue, or panic if strict initialization validation is preferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/init.go` around lines 251 - 258, The calls to cmd.RegisterFlagCompletionFunc (specifically the invocations RegisterFlagCompletionFunc("runtime", completeRuntimeFlag) and RegisterFlagCompletionFunc("output", newOutputFlagCompletion(...))) ignore returned errors; update both registrations to capture their error return and handle it (e.g., log the error with the package logger or fmt.Errorf and continue, or call panic if you want strict init validation) so failures to register flag completions are surfaced during command construction rather than silently ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/cmd/init.go`:
- Around line 251-258: The calls to cmd.RegisterFlagCompletionFunc (specifically
the invocations RegisterFlagCompletionFunc("runtime", completeRuntimeFlag) and
RegisterFlagCompletionFunc("output", newOutputFlagCompletion(...))) ignore
returned errors; update both registrations to capture their error return and
handle it (e.g., log the error with the package logger or fmt.Errorf and
continue, or call panic if you want strict init validation) so failures to
register flag completions are surfaced during command construction rather than
silently ignored.
In `@pkg/runtimesetup/register.go`:
- Around line 63-88: Duplicate availability-check logic exists between
listAvailableWithFactories and registerAllWithAvailable; extract a small
reusable helper (e.g., forEachAvailable) that iterates factories, skips runtimes
implementing Available when Available() is false, and invokes a callback for
each available runtime; refactor listAvailableWithFactories to call this helper
to build the names slice and refactor registerAllWithAvailable to reuse it for
registration; add unit tests in register_test.go that exercise ListAvailable()
and listAvailableWithFactories() (including at least one factory returning an
Available runtime that is not available) to verify filtering behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 67c23bd9-cc04-4981-9a53-f40dc0a78e9c
📒 Files selected for processing (11)
pkg/cmd/autocomplete.gopkg/cmd/autocomplete_test.gopkg/cmd/init.gopkg/cmd/remove.gopkg/cmd/start.gopkg/cmd/stop.gopkg/cmd/workspace_list.gopkg/cmd/workspace_remove.gopkg/cmd/workspace_start.gopkg/cmd/workspace_stop.gopkg/runtimesetup/register.go
✅ Files skipped from review due to trivial changes (3)
- pkg/cmd/workspace_list.go
- pkg/cmd/autocomplete_test.go
- pkg/cmd/autocomplete.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/cmd/workspace_start.go
- pkg/cmd/workspace_stop.go
- pkg/cmd/stop.go
- pkg/cmd/start.go
- pkg/cmd/workspace_remove.go
- pkg/cmd/remove.go
benoitf
approved these changes
Mar 23, 2026
Adds intelligent shell completion that filters workspace IDs based on their runtime state. The start and remove commands show only non-running workspaces, while stop shows only running workspaces. This prevents users from attempting invalid operations and improves the CLI experience. Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
Adds configurable shell completion for the --output flag. Each command can specify its own list of valid output formats through the newOutputFlagCompletion factory. Currently all commands support only 'json', but the design allows easy extension for future formats. Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
Adds shell completion for the --runtime flag that lists all available runtimes. The completion automatically filters out the "fake" runtime which is only used for testing. Also adds ListRuntimes() method to the Manager interface to expose registered runtime types. Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
Tab-completion was calling instances.NewManager() which creates storage directories as a side effect. Modified completion handlers to check if storage exists before instantiating a manager for workspace IDs, and added runtimesetup.ListAvailable() for runtime flag completion that doesn't require a manager instance. Removed the now-unused Manager.ListRuntimes() method. Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds intelligent shell completion that filters workspace IDs based on their runtime state. The start and remove commands show only non-running workspaces, while stop shows only running workspaces. This prevents users from attempting invalid operations and improves the CLI experience.
Adds configurable shell completion for the --output flag. Each command can specify its own list of valid output formats through the newOutputFlagCompletion factory. Currently all commands support only 'json', but the design allows easy extension for future formats.
Adds shell completion for the --runtime flag that lists all available runtimes. The completion automatically filters out the "fake" runtime which is only used for testing. Also adds ListRuntimes() method to the Manager interface to expose registered runtime types.
Fixes #85