feat: shell completion for jmp, jmp-admin, and j#422
feat: shell completion for jmp, jmp-admin, and j#422raballew wants to merge 19 commits intojumpstarter-dev:mainfrom
Conversation
❌ Deploy Preview for jumpstarter-docs failed. Why did it fail? →
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a reusable Click completion-command factory, wires shell completions into Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Entry as j/jmp (entrypoint)
participant Factory as make_completion_command
participant ClickGen as ClickCompletionClass
participant Shell as Shell (bash/zsh/fish)
User->>Entry: run "j|jmp completion <shell>"
Entry->>Factory: invoke completion command
Factory->>ClickGen: get_completion_class(shell) & instantiate
ClickGen-->>Factory: completion source()
Factory-->>Entry: emit completion script (stdout)
Entry-->>User: printed completion script
User->>Shell: source/install emitted script
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/j_completion_test.py`:
- Around line 58-67: The test currently only ensures no exception is raised but
doesn't check the fallback payload; modify
test_j_shell_complete_returns_empty_on_timeout to capture the result of
run(_j_shell_complete) and assert it equals the expected timeout fallback (e.g.,
an empty list) when env_async (patched as slow_env) sleeps past
_COMPLETION_TIMEOUT_SECONDS; reference the patched symbol
"jumpstarter_cli.j.env_async", the async contextmanager helper "slow_env", the
test runner call "run(_j_shell_complete)", and the timeout constant
"_COMPLETION_TIMEOUT_SECONDS" to locate where to add the assertion.
- Around line 45-55: The test currently only verifies mock_client.cli was
fetched but doesn't ensure the returned CLI callable was actually invoked and
that the SystemExit path ran; update the test for _j_shell_complete to exercise
the SystemExit branch by making mock_client.cli.return_value raise SystemExit(0)
(as you've set on mock_cli_group) and then asserting that the returned callable
was called (e.g. assert mock_client.cli.return_value.assert_called_once()) and
that run(_j_shell_complete) completes without propagating the SystemExit;
reference mock_client.cli, mock_client.cli.return_value (mock_cli_group) and the
_j_shell_complete invocation when adding the assertions.
In `@python/packages/jumpstarter-cli/jumpstarter_cli/j.py`:
- Around line 32-44: The timeout isn't interrupting the thread wait because
to_thread.run_sync is called without abandon_on_cancel; update the call in the
block using anyio.fail_after/_COMPLETION_TIMEOUT_SECONDS (inside the async with
BlockingPortal() and async with env_async(portal, stack) as client) to await
to_thread.run_sync(_run_completion, abandon_on_cancel=True) so the fail_after
timeout can cancel the await immediately; keep the surrounding context (function
_run_completion, client.cli(), BlockingPortal, env_async) unchanged.
In `@python/packages/jumpstarter/jumpstarter/common/utils.py`:
- Around line 106-111: The generated completion function _j_completion currently
provides the subcommand list for every cursor position; change its body to only
offer the baked subcommands when the cursor is on the first argument by checking
COMP_CWORD (e.g., if [ "${COMP_CWORD}" -eq 1 ]; then COMPREPLY=($(compgen -W
"..." -- "${COMP_WORDS[COMP_CWORD]}")); fi) and otherwise leave COMPREPLY empty
so default/file completions can run; update the code that builds _j_completion
(the block that uses j_commands, cmds, and lines.append) to emit that COMP_CWORD
guard around the compgen call.
- Around line 183-189: The temp ZDOTDIR assignment in _generate_shell_init
(tmpdir, zshrc_path, env["ZDOTDIR"]) causes zsh to read startup files from the
temp dir and break user profiles; remove the env["ZDOTDIR"] = tmpdir assignment
and instead keep ZDOTDIR unchanged, write init_content to tmpdir/.zshrc, and
ensure the shell loads this file by explicitly sourcing that file when launching
zsh (e.g., prepend a `source {zshrc_path}` invocation to the shell command or
add the temp file to the command invocation) while leaving cmd.extend(["--rcs",
"-o", "inc_append_history", "-o", "share_history"]) as-is so the user’s real
~/.zshenv and friends are still read.
- Around line 155-172: The _launch_fish function currently injects the context
and init_file.name directly into the fish init command (via f'printf
"{context}"; ' and unquoted init_file.name), which permits shell injection;
change _launch_fish to treat context as data by passing it through the
environment (like _launch_bash/_launch_zsh do) and reference it from the fish
function via the environment variable, and avoid interpolating init_file.name
into shell code by using a safely quoted/escaped path or sourcing via a separate
argument; keep using _run_process to launch the shell and add the environment
key (e.g., FISH_PROMPT_CONTEXT) to common_env before calling _run_process.
🪄 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: 1e091e91-4dda-463f-b83c-382cf818ca3c
📒 Files selected for processing (11)
python/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/__init__.pypython/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/completion.pypython/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/completion_test.pypython/packages/jumpstarter-cli-common/jumpstarter_cli_common/completion.pypython/packages/jumpstarter-cli-common/jumpstarter_cli_common/completion_test.pypython/packages/jumpstarter-cli/jumpstarter_cli/completion.pypython/packages/jumpstarter-cli/jumpstarter_cli/j.pypython/packages/jumpstarter-cli/jumpstarter_cli/j_completion_test.pypython/packages/jumpstarter-cli/jumpstarter_cli/shell.pypython/packages/jumpstarter/jumpstarter/common/utils.pypython/packages/jumpstarter/jumpstarter/common/utils_test.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/packages/jumpstarter/jumpstarter/common/utils.py (1)
174-176: Consider quoting the init file path for defense in depth.While
init_file.nameis system-generated bytempfile.NamedTemporaryFileand unlikely to contain special characters, quoting it provides defense against edge cases (e.g., unusualTMPDIRvalues).♻️ Suggested improvement
if init_file: - init_cmd += f"; source {init_file.name}" + init_cmd += f"; source {shlex.quote(init_file.name)}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter/jumpstarter/common/utils.py` around lines 174 - 176, The init file path used when building init_cmd is not quoted; update the code that builds init_cmd (the block that appends f"; source {init_file.name}") to safely quote the path (e.g., via shlex.quote or equivalent) before interpolation so the call to _run_process([shell, "--init-command", init_cmd], fish_env, lease) gets a properly escaped filename; adjust the code that sets init_cmd and the branch that checks init_file to use the quoted value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/common/utils.py`:
- Around line 174-176: The init file path used when building init_cmd is not
quoted; update the code that builds init_cmd (the block that appends f"; source
{init_file.name}") to safely quote the path (e.g., via shlex.quote or
equivalent) before interpolation so the call to _run_process([shell,
"--init-command", init_cmd], fish_env, lease) gets a properly escaped filename;
adjust the code that sets init_cmd and the branch that checks init_file to use
the quoted value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5ea477e-d702-40b0-b0e1-a15aac6f105a
📒 Files selected for processing (4)
python/packages/jumpstarter-cli/jumpstarter_cli/j.pypython/packages/jumpstarter-cli/jumpstarter_cli/j_completion_test.pypython/packages/jumpstarter/jumpstarter/common/utils.pypython/packages/jumpstarter/jumpstarter/common/utils_test.py
✅ Files skipped from review due to trivial changes (1)
- python/packages/jumpstarter-cli/jumpstarter_cli/j.py
🚧 Files skipped from review as they are similar to previous changes (2)
- python/packages/jumpstarter-cli/jumpstarter_cli/j_completion_test.py
- python/packages/jumpstarter/jumpstarter/common/utils_test.py
|
Something is weird at my side: |
|
this is MacOS / zsh |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
python/packages/jumpstarter/jumpstarter/common/utils.py (2)
159-178: Context injection fixed; minor remaining inconsistency with init file path.The critical shell injection via
contextis now properly fixed using the_JMP_SHELL_CONTEXTenvironment variable (line 160, 168).Line 176 still interpolates
init_file.namedirectly into the fish command. While this is safe in practice (tempfile generates controlled paths like/tmp/tmpXXXXXX.sh), the past review suggested passing it via environment variable for consistency with the context fix.♻️ Optional: Use env var for init file path
def _launch_fish(shell, init_file, common_env, context, lease): - fish_env = common_env | {"_JMP_SHELL_CONTEXT": context} + fish_env = common_env | {"_JMP_SHELL_CONTEXT": context} fish_fn = ( # ... prompt function unchanged ... ) init_cmd = fish_fn if init_file: - init_cmd += f"; source {init_file.name}" + fish_env["_JMP_SHELL_INIT"] = init_file.name + init_cmd += '; source "$_JMP_SHELL_INIT"' return _run_process([shell, "--init-command", init_cmd], fish_env, lease)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter/jumpstarter/common/utils.py` around lines 159 - 178, _in _launch_fish the init file path is still interpolated directly via init_file.name into init_cmd; change this to pass the path through the environment like you did for context: add a key (e.g. "_JMP_FISH_INIT") to fish_env containing init_file.name (when init_file is present), and reference that env var in init_cmd instead of init_file.name so _run_process receives no direct string interpolation of the path; update any logic that builds init_cmd and the call to _run_process accordingly.
99-145: Good implementation with minor inconsistency in shell detection.The zsh
compinitinitialization (line 123) correctly addresses the macOS "command not found: compdef" error reported in PR comments. The bashCOMP_CWORDcheck (lines 110-111) properly limits subcommand completion to the first argument position.Minor inconsistency: bash uses
.endswith("bash")(matches bothbashand paths like/usr/bin/bash), while zsh and fish use exact string matches. Sinceshell_nameis derived fromos.path.basename()inlaunch_shell, this works correctly, but the style is inconsistent.♻️ Optional: Consistent shell detection
- if shell_name.endswith("bash"): + if shell_name == "bash":Or alternatively, use
.endswith()for all shells to handle variants likezsh-5.9.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter/jumpstarter/common/utils.py` around lines 99 - 145, The shell-detection logic in _generate_shell_init is inconsistent: bash uses shell_name.endswith("bash") but zsh and fish use exact equality; make detection uniform by using endswith for zsh and fish as well (e.g., change comparisons in the branches that currently use shell_name == "zsh" and shell_name == "fish" to shell_name.endswith("zsh") and shell_name.endswith("fish")), so variants like "zsh-5.9" or paths still match consistently with the bash branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/common/utils.py`:
- Around line 159-178: _in _launch_fish the init file path is still interpolated
directly via init_file.name into init_cmd; change this to pass the path through
the environment like you did for context: add a key (e.g. "_JMP_FISH_INIT") to
fish_env containing init_file.name (when init_file is present), and reference
that env var in init_cmd instead of init_file.name so _run_process receives no
direct string interpolation of the path; update any logic that builds init_cmd
and the call to _run_process accordingly.
- Around line 99-145: The shell-detection logic in _generate_shell_init is
inconsistent: bash uses shell_name.endswith("bash") but zsh and fish use exact
equality; make detection uniform by using endswith for zsh and fish as well
(e.g., change comparisons in the branches that currently use shell_name == "zsh"
and shell_name == "fish" to shell_name.endswith("zsh") and
shell_name.endswith("fish")), so variants like "zsh-5.9" or paths still match
consistently with the bash branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eb6e6b21-e74e-4c24-90a5-9937ef44de36
📒 Files selected for processing (2)
python/packages/jumpstarter/jumpstarter/common/utils.pypython/packages/jumpstarter/jumpstarter/common/utils_test.py
✅ Files skipped from review due to trivial changes (1)
- python/packages/jumpstarter/jumpstarter/common/utils_test.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/packages/jumpstarter/jumpstarter/common/utils.py`:
- Around line 119-131: The zsh branch currently appends the source of ~/.zshrc
before running compinit, causing compdef errors; modify the block handling
shell_name.endswith("zsh") so compinit (the "autoload -Uz compinit && compinit"
line) and the eval "$(jmp ...)" / eval "$(jmp-admin ...)" completions are added
to lines before the optional '[ -f ~/.zshrc ] && source ~/.zshrc' when
use_profiles is True, preserving the j_commands logic (compdef vs eval "$(j
...)" behavior) and returning the joined lines as before.
- Around line 126-128: The zsh `_arguments` invocation built in utils.py when
j_commands is present uses wrong syntax; update the string produced by the
lines.append call (the one currently building compdef with f"compdef '_arguments
\"1:(({cmds}))\"' j") to include the required message field and single
parentheses around the value list (for example use a message like "subcommand"
and value list "(cmd1 cmd2 ...)"); ensure the f-string around variable
j_commands (cmds) produces `_arguments '1:subcommand:(<cmds>)'` and that quoting
is correct so the final compdef call registers properly.
🪄 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: 62822921-b23b-460d-a58d-23b050ead306
📒 Files selected for processing (2)
python/packages/jumpstarter/jumpstarter/common/utils.pypython/packages/jumpstarter/jumpstarter/common/utils_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/packages/jumpstarter/jumpstarter/common/utils_test.py
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 `@python/packages/jumpstarter/jumpstarter/common/utils_test.py`:
- Around line 292-300: The zsh test invoking subprocess.run with ["zsh",
"--rcs", "-c", "exit 0"] doesn't source init files in non-interactive mode and
must be changed to spawn zsh in interactive or login mode (e.g., use "-i" or
"-l") so the completion init logic runs; update the subprocess.run call(s) that
reference zsh to use an interactive/login flag or otherwise explicitly source
the tmpdir ~/.zshrc within the invoked command. Also update the bash test that
uses ["bash", "--rcfile", ... , "-c", ...] to instead explicitly source the
rcfile in the command (modify the subprocess.run invocation referencing
bash/--rcfile) so the bash initialization is actually executed during the test.
🪄 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: 255e6b21-8a63-477a-a595-4818689ba271
📒 Files selected for processing (2)
.github/workflows/python-tests.yamlpython/packages/jumpstarter/jumpstarter/common/utils_test.py
raballew
force-pushed
the
039-shell-completion
branch
from
54ba220 to
9774aa7
Compare
I found 2 problems:
For jmp it seems to work well. |
|
@ambient-code why is CI failing? |
raballew
force-pushed
the
039-shell-completion
branch
from
7f73dd8 to
b6b7bae
Compare
|
Now completion for bare jmp does not work, but the prompt is back |
|
raballew
force-pushed
the
039-shell-completion
branch
3 times, most recently
from
0a9ad64 to
00b5da7
Compare
Extract a shared make_completion_command factory into jumpstarter-cli-common and use it in both jmp and jmp-admin CLIs. This adds a `completion` subcommand to jmp-admin (and by extension `jmp admin`) supporting bash, zsh, and fish. Generated-By: Forge/20260407_145514_2280530_d26e63ff Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The shared make_completion_command factory lacked direct unit tests in its own package, relying only on indirect coverage from downstream packages. This adds tests exercising the factory with a minimal Click group for all shell types and error cases. Generated-By: Forge/20260407_145514_2280530_d26e63ff Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Auto-source completions for jmp, jmp-admin, and j when entering jmp shell
(bash, zsh, fish). For j, command names are baked into the shell init script
at startup to avoid slow gRPC calls on every TAB press.
Adds `j completion {bash,zsh,fish}` subcommand for standalone use, with a
fast-path dispatch that avoids the full async stack. Catches SystemExit from
Click's completion handler before it propagates through anyio task groups.
Closes jumpstarter-dev#35
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix zsh temp file leak by cleaning up .zshrc in _launch_zsh finally block (F001) - Validate j_commands against safe pattern to prevent shell injection (F002) - Refactor launch_shell into per-shell helpers to reduce complexity (F003) - Add debug logging when j command extraction fails (F004) - Add test for zsh temp file cleanup in launch_shell (F005) - Add 5-second timeout to _j_shell_complete to prevent shell freezes (F006) - Rename admin completion tests to describe expected behavior (F007) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add None guard for get_completion_class return value (F001) - Simplify zsh temp file handling with mkdtemp, eliminating unnecessary intermediate file (F002) - Quote fish completion argument for defense-in-depth (F003) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix shell injection in fish prompt by passing context via env var - Add COMP_CWORD guard to limit bash completion to first argument - Restore original ZDOTDIR in generated zshrc to preserve user config - Add abandon_on_cancel=True to completion thread for clean timeout - Strengthen test assertions for completion and timeout behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The zsh init script used compdef without ensuring compinit had been called. On macOS zsh where compinit is not loaded by /etc/zshrc, this caused "command not found: compdef" errors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When use_profiles=True, the user's ~/.zshrc (or ~/.bashrc) is sourced inside the init script, which typically sets PROMPT/PS1 and overrides the custom jumpstarter prompt set via environment variable. Fix by appending the prompt assignment at the end of the init script so it runs after user profile sourcing. Pass context via _JMP_SHELL_CONTEXT env var (consistent with fish) instead of interpolating it directly. Also refactor _launch_bash to manage its own init file lifecycle, matching the pattern used by _launch_zsh. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When the user's shell profile modifies PATH (common on macOS where /etc/zshenv runs path_helper, or with oh-my-zsh), the bare command names jmp/jmp-admin/j may no longer be found by the time the completion eval commands run. Resolve the CLI binary paths at init generation time using shutil.which() so the completion scripts reference absolute paths that survive PATH modifications. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The test relied on jmp being installed on PATH but CI environments do not have it, causing shutil.which to return None and the test to fail with bare command names instead of absolute paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ed module The completion module was refactored to delegate to make_completion_command from jumpstarter_cli_common. The test for the get_completion_class-returns-None defensive path was patching jumpstarter_cli.completion.get_completion_class which no longer exists in that module. Remove the stale test and unused import from jumpstarter_cli, and add an equivalent test in jumpstarter_cli_common where the defensive code now lives. Generated-By: Forge/20260416_144214_369510_5d940644 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
On macOS, .zshrc may set up fpath entries needed by compinit. Loading compinit before profiles causes 'compdef: command not found' when the completion system cannot find its function definitions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
raballew
force-pushed
the
039-shell-completion
branch
from
352e148 to
907612d
Compare
There was a problem hiding this comment.
TBH, this whole file doesn't look pretty. I was itching to refactor it a bit while back, as it was already a bit of an if else jungle in combination with partially overlapping special case treatments for the different shells, and in this aspect your bot just made it further away from being any elegant code :)
not a deal breaker in any sense, but i think it worth to optimize it a bit.
| ) -> int: | ||
| """Launch a shell with a custom prompt indicating the exporter type. | ||
|
|
||
| Args: | ||
| host: The jumpstarter host path | ||
| context: The context of the shell (e.g. "local" or exporter name) | ||
| allow: List of allowed drivers | ||
| unsafe: Whether to allow drivers outside of the allow list | ||
| use_profiles: Whether to load shell profile files | ||
| command: Optional command to run instead of launching an interactive shell | ||
| lease: Optional Lease object to set up lease ending callback | ||
|
|
||
| Returns: | ||
| The exit code of the shell or command process | ||
| """ | ||
|
|
||
| shell = os.environ.get("SHELL", "bash") | ||
| shell_name = os.path.basename(shell) | ||
|
|
||
| common_env = os.environ | { | ||
| JUMPSTARTER_HOST: host, | ||
| JMP_DRIVERS_ALLOW: "UNSAFE" if unsafe else ",".join(allow), | ||
| "_JMP_SUPPRESS_DRIVER_WARNINGS": "1", # Already warned during client initialization | ||
| "_JMP_SUPPRESS_DRIVER_WARNINGS": "1", | ||
| } |
There was a problem hiding this comment.
while I'm not a big fan of a file packed full of comments, but completely stripping everything including the docstrings and previous comments made by other devs is a strange decision 🤷
| init_file = tempfile.NamedTemporaryFile(mode="w", suffix=".sh", delete=False) | ||
| init_file.write(init_content) | ||
| init_file.close() | ||
|
|
There was a problem hiding this comment.
this one is inconsistent in a way it writes it here for fish but inside _launch_xxx for the rest of the shells.
it's also a duplicate code that could be made generic for all the 3 shells by using mkdtemp tmpdir for all of them, probably in a separate function.
| def _validate_j_commands(j_commands: list[str] | None) -> list[str] | None: | ||
| if j_commands is None: | ||
| return None | ||
| return [cmd for cmd in j_commands if _SAFE_COMMAND_NAME.match(cmd)] |
There was a problem hiding this comment.
that's an interesting one. so basically it tries to protect from maliciously set resource name in ExporterConfig? not sure how real is the attack surface there. I tried creating one locally and attempted to autocomplete on unpatched version using autoload -Uz compinit && compinit && source <(_J_COMPLETE=zsh_source j) and it just escaped the special characters for me by default. and even if it's not, it seems it would not execute it by itself anyway but rather just autocomplete it and only if the user proceeds with the malformed command then execution is possible, which is unlikely unless the user have absolutely no idea what he's doing :)
also it's a one-liner anyway, and never reused in code (test cases aside, even though I doubt there's any meaningful complexity to test here anyway) , so it's probably better to inline in _generate_shell_init.
| def _resolve_cli_paths() -> tuple[str, str, str]: | ||
| jmp = shutil.which("jmp") or "jmp" | ||
| jmp_admin = shutil.which("jmp-admin") or "jmp-admin" | ||
| j = shutil.which("j") or "j" | ||
| return jmp, jmp_admin, j |
There was a problem hiding this comment.
same goes for here. doesn't warrant a separate function IMO.
| # Extract j command names for static shell completion | ||
| j_commands = None | ||
| try: | ||
| cli_group = client.cli() | ||
| if hasattr(cli_group, "list_commands"): | ||
| j_commands = cli_group.list_commands(None) | ||
| except Exception as e: | ||
| logger.debug("Failed to extract j commands for completion: %s", e) |
There was a problem hiding this comment.
this one is a bit of concern, as it seems like it generate only a shallow command list, rather than a tree, so only the j commands will get completion, but j commands subcommands will get nothing, and they are the useful ones IMO. this is also inconsistent with the fallback that calls j completion <shell> which from my tests will autocomplete the full tree.
a better solution if we want a proper autocompletion caching, would be building the full tree here recursively.
another option to explore, is making j completion to generate and return a full tree inside its j completion command, rather than a simple completion script that includes response=("${(@f)$(env COMP_WORDS="${words[*]}" COMP_CWORD=$((CURRENT-1)) _J_COMPLETE=zsh_complete j)}") as it does currently, so then by loading j completion once at jmp shell start (rather than passing a shallow list as we do now) we're all set and covered. this option may be more complex though.
yet another option would be to implement a proper completion tree with either of the options above in a separate PR as it seems like an effort on its own, and for now relying on the build-in autocompletion in j which is slow because of dynamic fetching, but it works nevertheless and works well.
There was a problem hiding this comment.
personally I would suggest the later, just sticking with build-in completion for this PR, and follow up in a separate one just handling the caching properly. if you want, this is something that I can do instead, as I already have some ideas of how to implement it properly, and I"m also thinking about expanding it to dynamic completion for specific subcommands, at least for jmp, where we would fetch a discoverable values (this part is completely absent from both jmp as well as j at the moment)
| def make_completion_command(cli_group_factory: Callable[[], click.Command], prog_name: str, complete_var: str): | ||
| @click.command("completion") | ||
| @click.argument("shell", type=click.Choice(["bash", "zsh", "fish"])) | ||
| def completion(shell: str): | ||
| """Generate shell completion script.""" | ||
| cli_group = cli_group_factory() | ||
| comp_cls = get_completion_class(shell) | ||
| if comp_cls is None: | ||
| raise click.ClickException(f"Unsupported shell: {shell}") | ||
| comp = comp_cls(cli_group, {}, prog_name, complete_var) | ||
| click.echo(comp.source()) |
There was a problem hiding this comment.
I recall this one, I also mentioned it in #35 as evaluate the usefulness of jmp complete subcommand.. however upon further examination, the standard way for Click/Typer seems to be:
_JMP_COMPLETE=<zsh/bash/fish>_source jmp which already exists without the implementation. the jmp completion <shell> format seems to be used by the Cobra library in tools written in go such as kubectl, and there are other patterns with other libraries and languages.
so in this case, we're just adding another command exposing the same completion script for which a command already exists, and it's not the native way for python. there is still "some" benefit of familiarity, as we're working closely with k8s/openshift where the cli is built in Cobra so users may be familiar with it more, but that's a thin one.
| lines.append(f'eval "$({jmp} completion bash 2>/dev/null)"') | ||
| lines.append(f'eval "$({jmp_admin} completion bash 2>/dev/null)"') |
There was a problem hiding this comment.
so we're basically adding jmp completion into jmp shell itself here. IMHO not sure how useful it is considering that most jmp commands were triggered prior to that in order to reach jmp shell in the first place, which means that we probably really want jmp completion integrated into the user's shell rc file, one way could be adding it there via jumpstarter install.sh script. I'm not saying this one is redundant, but I definitely think that a system-wide one is missing.
When ZDOTDIR is set to a temp directory for zsh init, the user's ~/.zshenv is never read because zsh reads .zshenv from ZDOTDIR before .zshrc. Create a .zshenv in the temp directory that sources the original ~/.zshenv so user environment setup is preserved. Generated-By: Forge/20260420_165007_876667_3532a927
Add concise docstrings to _validate_j_commands, _resolve_cli_paths, _generate_shell_init, _launch_bash, _launch_fish, and _launch_zsh to restore documentation that was stripped during refactoring. Generated-By: Forge/20260420_165007_876667_3532a927
Align fish launcher with bash and zsh by handling init file creation and cleanup inside _launch_fish instead of in launch_shell. All three shell launchers now follow the same pattern of receiving init_content and managing their own temp files. Generated-By: Forge/20260420_165007_876667_3532a927
Add test verifying that the fish launcher creates the init file during process execution and cleans it up after the process exits, matching the coverage already present for bash and zsh launchers. Generated-By: Forge/20260420_165007_876667_3532a927
3 participants
Summary
jmp,jmp-admin, andjwhen enteringjmp shell(bash, zsh, fish)jsubcommand names into shell init scripts at startup for instant TAB completion (no gRPC on every keypress)j completion {bash,zsh,fish}subcommand with fast-path dispatch that avoids the full async stack and catchesSystemExitcleanlymake_completion_commandfactory injumpstarter-cli-commonreused across all three CLIsCloses #35
Test plan
_generate_shell_initcovering bash/zsh/fish with and without j_commands, profiles, and unknown shellsj completioncovering bash/zsh/fish script generation, error cases, and SystemExit handlingjmp shellthen TAB-completejmp,jmp-admin, andjsubcommands in bash/zsh/fishj completion bash | source /dev/stdinoutside jmp shell🤖 Generated with Claude Code