feat: shell completion for jmp, jmp-admin, and j#422
Open
raballew wants to merge 7 commits intojumpstarter-dev:mainfrom
Open
feat: shell completion for jmp, jmp-admin, and j#422raballew wants to merge 7 commits intojumpstarter-dev:mainfrom
raballew wants to merge 7 commits intojumpstarter-dev:mainfrom
Conversation
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>
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
📝 WalkthroughWalkthroughAdds a reusable Click completion-command factory, wires shell completions into Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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 |
- 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>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
Comment on lines
+45
to
+55
| def test_j_shell_complete_handles_system_exit_cleanly(): | ||
| mock_cli_group = MagicMock() | ||
| mock_cli_group.side_effect = SystemExit(0) | ||
| mock_client = MagicMock() | ||
| mock_client.cli.return_value = mock_cli_group | ||
|
|
||
| with patch("jumpstarter_cli.j.env_async") as mock_env: | ||
| mock_env.return_value.__aenter__ = AsyncMock(return_value=mock_client) | ||
| mock_env.return_value.__aexit__ = AsyncMock(return_value=False) | ||
| run(_j_shell_complete) | ||
| mock_client.cli.assert_called_once() |
Contributor
There was a problem hiding this comment.
Actually exercise the SystemExit branch here.
This only proves client.cli() was fetched. If _j_shell_complete() stops calling the returned CLI callable, the SystemExit(0) path can regress and this test still passes.
Suggested assertion hardening
with patch("jumpstarter_cli.j.env_async") as mock_env:
mock_env.return_value.__aenter__ = AsyncMock(return_value=mock_client)
mock_env.return_value.__aexit__ = AsyncMock(return_value=False)
run(_j_shell_complete)
mock_client.cli.assert_called_once()
+ mock_cli_group.assert_called_once()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_j_shell_complete_handles_system_exit_cleanly(): | |
| mock_cli_group = MagicMock() | |
| mock_cli_group.side_effect = SystemExit(0) | |
| mock_client = MagicMock() | |
| mock_client.cli.return_value = mock_cli_group | |
| with patch("jumpstarter_cli.j.env_async") as mock_env: | |
| mock_env.return_value.__aenter__ = AsyncMock(return_value=mock_client) | |
| mock_env.return_value.__aexit__ = AsyncMock(return_value=False) | |
| run(_j_shell_complete) | |
| mock_client.cli.assert_called_once() | |
| def test_j_shell_complete_handles_system_exit_cleanly(): | |
| mock_cli_group = MagicMock() | |
| mock_cli_group.side_effect = SystemExit(0) | |
| mock_client = MagicMock() | |
| mock_client.cli.return_value = mock_cli_group | |
| with patch("jumpstarter_cli.j.env_async") as mock_env: | |
| mock_env.return_value.__aenter__ = AsyncMock(return_value=mock_client) | |
| mock_env.return_value.__aexit__ = AsyncMock(return_value=False) | |
| run(_j_shell_complete) | |
| mock_client.cli.assert_called_once() | |
| mock_cli_group.assert_called_once() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/packages/jumpstarter-cli/jumpstarter_cli/j_completion_test.py` around
lines 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.
Comment on lines
+58
to
+67
| def test_j_shell_complete_returns_empty_on_timeout(): | ||
| from contextlib import asynccontextmanager | ||
|
|
||
| @asynccontextmanager | ||
| async def slow_env(*args, **kwargs): | ||
| await anyio.sleep(_COMPLETION_TIMEOUT_SECONDS + 1) | ||
| yield MagicMock() | ||
|
|
||
| with patch("jumpstarter_cli.j.env_async", slow_env): | ||
| run(_j_shell_complete) |
Contributor
There was a problem hiding this comment.
Assert the timeout fallback, not just “no exception.”
run(_j_shell_complete) is currently unchecked, so this still passes if the helper returns the wrong payload after timing out.
Suggested assertion hardening
with patch("jumpstarter_cli.j.env_async", slow_env):
- run(_j_shell_complete)
+ assert run(_j_shell_complete) == ""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_j_shell_complete_returns_empty_on_timeout(): | |
| from contextlib import asynccontextmanager | |
| @asynccontextmanager | |
| async def slow_env(*args, **kwargs): | |
| await anyio.sleep(_COMPLETION_TIMEOUT_SECONDS + 1) | |
| yield MagicMock() | |
| with patch("jumpstarter_cli.j.env_async", slow_env): | |
| run(_j_shell_complete) | |
| def test_j_shell_complete_returns_empty_on_timeout(): | |
| from contextlib import asynccontextmanager | |
| `@asynccontextmanager` | |
| async def slow_env(*args, **kwargs): | |
| await anyio.sleep(_COMPLETION_TIMEOUT_SECONDS + 1) | |
| yield MagicMock() | |
| with patch("jumpstarter_cli.j.env_async", slow_env): | |
| assert run(_j_shell_complete) == "" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/packages/jumpstarter-cli/jumpstarter_cli/j_completion_test.py` around
lines 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.
Comment on lines
+155
to
+172
| def _launch_fish(shell, init_file, common_env, context, lease): | ||
| fish_fn = ( | ||
| "function fish_prompt; " | ||
| "set_color grey; " | ||
| 'printf "%s" (basename $PWD); ' | ||
| "set_color yellow; " | ||
| 'printf "⚡"; ' | ||
| "set_color white; " | ||
| f'printf "{context}"; ' | ||
| "set_color yellow; " | ||
| 'printf "➤ "; ' | ||
| "set_color normal; " | ||
| "end" | ||
| ) | ||
| init_cmd = fish_fn | ||
| if init_file: | ||
| init_cmd += f"; source {init_file.name}" | ||
| return _run_process([shell, "--init-command", init_cmd], common_env, lease) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n python/packages/jumpstarter/jumpstarter/common/utils.py | sed -n '150,180p'Repository: jumpstarter-dev/jumpstarter
Length of output: 1271
🏁 Script executed:
rg "_launch_fish" python/packages/jumpstarter/ -nRepository: jumpstarter-dev/jumpstarter
Length of output: 332
🏁 Script executed:
cat -n python/packages/jumpstarter/jumpstarter/common/utils.py | sed -n '230,260p'Repository: jumpstarter-dev/jumpstarter
Length of output: 1108
🏁 Script executed:
cat -n python/packages/jumpstarter/jumpstarter/common/utils.py | sed -n '200,235p'Repository: jumpstarter-dev/jumpstarter
Length of output: 1384
🏁 Script executed:
cat -n python/packages/jumpstarter/jumpstarter/common/utils.py | sed -n '125,155p'Repository: jumpstarter-dev/jumpstarter
Length of output: 1358
🏁 Script executed:
rg "launch_shell\(" python/packages/jumpstarter/ -n -B 2Repository: jumpstarter-dev/jumpstarter
Length of output: 1947
🏁 Script executed:
rg "launch_shell\(" python/packages/jumpstarter/ --type py -A 5 | grep -v "test" | head -50Repository: jumpstarter-dev/jumpstarter
Length of output: 536
🏁 Script executed:
rg "from.*launch_shell|import.*launch_shell" python/packages/jumpstarter/ --type pyRepository: jumpstarter-dev/jumpstarter
Length of output: 207
🏁 Script executed:
find python/packages/jumpstarter -name "*.py" -type f | xargs grep -l "launch_shell" | grep -v testRepository: jumpstarter-dev/jumpstarter
Length of output: 126
🏁 Script executed:
rg "exporter_name" python/packages/jumpstarter/ -nRepository: jumpstarter-dev/jumpstarter
Length of output: 2173
🏁 Script executed:
cat -n python/packages/jumpstarter/jumpstarter/client/lease.py | sed -n '55,100p'Repository: jumpstarter-dev/jumpstarter
Length of output: 2477
🏁 Script executed:
rg "launch_shell" python/packages/jumpstarter/ --type py -B 5 -A 5 | grep -v "^--$" | head -100Repository: jumpstarter-dev/jumpstarter
Length of output: 8413
🏁 Script executed:
find python/packages/jumpstarter -name "*.py" -type f | xargs grep -l "def main\|click.command\|argparse" | head -10Repository: jumpstarter-dev/jumpstarter
Length of output: 131
🏁 Script executed:
cat -n python/packages/jumpstarter/jumpstarter/client/decorators.py | head -100Repository: jumpstarter-dev/jumpstarter
Length of output: 4008
🏁 Script executed:
rg "launch_shell\|_launch_fish" python/ --type py -B 3 -A 10 | grep -E "(def|context|exporter)" | head -30Repository: jumpstarter-dev/jumpstarter
Length of output: 53
🏁 Script executed:
ls -la python/packages/jumpstarter/jumpstarter/Repository: jumpstarter-dev/jumpstarter
Length of output: 683
Treat context as data, not fish code.
The context parameter (which can come from lease.exporter_name) is directly interpolated into fish shell code at line 163 via f-string: f'printf "{context}"; '. This allows shell metacharacters in the exporter name to break out of the printf statement and execute arbitrary commands. The same vulnerability exists at line 171 with the unquoted init_file.name.
In contrast, _launch_bash (line 145) and _launch_zsh (line 177) correctly pass context via environment variables rather than interpolation. Apply the same pattern here:
Fix
def _launch_fish(shell, init_file, common_env, context, lease):
+ env = common_env | {"_JMP_SHELL_CONTEXT": context}
fish_fn = (
"function fish_prompt; "
"set_color grey; "
'printf "%s" (basename $PWD); '
"set_color yellow; "
'printf "⚡"; '
"set_color white; "
- f'printf "{context}"; '
+ 'printf "%s" "$_JMP_SHELL_CONTEXT"; '
"set_color yellow; "
'printf "➤ "; '
"set_color normal; "
"end"
)
init_cmd = fish_fn
if init_file:
- init_cmd += f"; source {init_file.name}"
- return _run_process([shell, "--init-command", init_cmd], common_env, lease)
+ env["_JMP_SHELL_INIT"] = init_file.name
+ init_cmd += '; source "$_JMP_SHELL_INIT"'
+ return _run_process([shell, "--init-command", init_cmd], 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 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.
Comment on lines
+183
to
+189
| if init_content: | ||
| tmpdir = tempfile.mkdtemp() | ||
| zshrc_path = os.path.join(tmpdir, ".zshrc") | ||
| with open(zshrc_path, "w") as f: | ||
| f.write(init_content) | ||
| cmd.extend(["--rcs", "-o", "inc_append_history", "-o", "share_history"]) | ||
| env["ZDOTDIR"] = tmpdir |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd python/packages/jumpstarter && find . -name "*.py" -type f | head -20Repository: jumpstarter-dev/jumpstarter
Length of output: 723
🏁 Script executed:
cd python/packages/jumpstarter && rg "_generate_shell_init" -A 20 -B 5Repository: jumpstarter-dev/jumpstarter
Length of output: 12427
🏁 Script executed:
cd python/packages/jumpstarter && cat -n jumpstarter/common/utils.py | sed -n '170,210p'Repository: jumpstarter-dev/jumpstarter
Length of output: 1622
🏁 Script executed:
cd python/packages/jumpstarter && cat -n jumpstarter/common/utils.py | sed -n '60,130p'Repository: jumpstarter-dev/jumpstarter
Length of output: 3121
🏁 Script executed:
cd python/packages/jumpstarter && cat -n jumpstarter/common/utils.py | sed -n '40,100p'Repository: jumpstarter-dev/jumpstarter
Length of output: 2364
🏁 Script executed:
cd python/packages/jumpstarter && cat -n jumpstarter/common/utils.py | sed -n '116,145p'Repository: jumpstarter-dev/jumpstarter
Length of output: 1485
🏁 Script executed:
cd python/packages/jumpstarter && cat -n jumpstarter/common/utils.py | sed -n '203,250p'Repository: jumpstarter-dev/jumpstarter
Length of output: 1968
🏁 Script executed:
cd python/packages/jumpstarter && rg "use_profiles.*zsh" -A 5 -B 2Repository: jumpstarter-dev/jumpstarter
Length of output: 53
🏁 Script executed:
cd python/packages/jumpstarter && cat -n jumpstarter/common/utils_test.py | sed -n '60,80p'Repository: jumpstarter-dev/jumpstarter
Length of output: 891
🏁 Script executed:
cd python/packages/jumpstarter && cat -n jumpstarter/common/utils_test.py | grep -A 20 "test_launch_shell_zsh_cleans_up_all_temp_files"Repository: jumpstarter-dev/jumpstarter
Length of output: 938
🏁 Script executed:
cd python/packages/jumpstarter && cat -n jumpstarter/common/utils_test.py | sed -n '89,115p'Repository: jumpstarter-dev/jumpstarter
Length of output: 1301
🏁 Script executed:
cd python/packages/jumpstarter && python3 << 'EOF'
# Simulate _generate_shell_init for zsh
def simulate_zsh_init(use_profiles, j_commands=None):
lines = []
if use_profiles:
lines.append('[ -f ~/.zshrc ] && source ~/.zshrc')
lines.append('eval "$(jmp completion zsh 2>/dev/null)"')
lines.append('eval "$(jmp-admin completion zsh 2>/dev/null)"')
if j_commands:
cmds = " ".join(j_commands)
lines.append(f"compdef '_arguments \"1:(({cmds}))\"' j")
else:
lines.append('eval "$(j completion zsh 2>/dev/null)"')
return "\n".join(lines) + "\n"
# Test various cases
print("Case 1 - use_profiles=False, j_commands=None:")
result1 = simulate_zsh_init(False, None)
print(f"Result is empty: {result1 == ''}")
print(f"Length: {len(result1)}")
print()
print("Case 2 - use_profiles=True, j_commands=['power']:")
result2 = simulate_zsh_init(True, ["power"])
print(f"Result is empty: {result2 == ''}")
print()
print("Case 3 - use_profiles=False, j_commands=['power']:")
result3 = simulate_zsh_init(False, ["power"])
print(f"Result is empty: {result3 == ''}")
EOFRepository: jumpstarter-dev/jumpstarter
Length of output: 300
🏁 Script executed:
# Research zsh ZDOTDIR behavior
cat > /tmp/zsh_behavior_check.md << 'EOF'
ZSH ZDOTDIR BEHAVIOR:
When ZDOTDIR is set to a directory:
1. zsh reads $ZDOTDIR/.zshenv (NOT ~/.zshenv)
2. If login shell: zsh reads $ZDOTDIR/.zprofile (NOT ~/.zprofile)
3. If interactive: zsh reads $ZDOTDIR/.zshrc (NOT ~/.zshrc)
KEY POINT: When ZDOTDIR is set, zsh DOES NOT read the home directory startup files.
It ONLY reads from the ZDOTDIR directory.
In the code:
- Code writes init_content to $ZDOTDIR/.zshrc
- Code does NOT create $ZDOTDIR/.zshenv
- When use_profiles=True: init_content includes 'source ~/.zshrc'
- This sources the user's ~/.zshrc from within the temp .zshrc
ISSUE:
- User's ~/.zshrc may contain: "source $ZDOTDIR/config"
- With ZDOTDIR=/tmp/xyz, this becomes "source /tmp/xyz/config"
- But user expects "source ~/.config" relative to their home
This is indeed a regression.
EOF
cat /tmp/zsh_behavior_check.mdRepository: jumpstarter-dev/jumpstarter
Length of output: 875
🏁 Script executed:
cd python/packages/jumpstarter && cat -n jumpstarter/common/utils.py | sed -n '231,235p'Repository: jumpstarter-dev/jumpstarter
Length of output: 303
🏁 Script executed:
cd python/packages/jumpstarter && python3 << 'EOF'
# Verify: for zsh, is init_content always non-empty?
# And does that mean code always enters the if init_content: branch?
# From _generate_shell_init for zsh:
# Lines always added:
# 1. lines.append('eval "$(jmp completion zsh 2>/dev/null)"')
# 2. lines.append('eval "$(jmp-admin completion zsh 2>/dev/null)"')
# 3. One more line (either completion j or compdef)
# Then: return "\n".join(lines) + "\n"
# So for zsh, _generate_shell_init ALWAYS returns non-empty
# This means in launch_shell(), line 231-234:
# - init_content is always non-empty for zsh
# - So _launch_zsh() is called with init_content (never empty)
# - So in _launch_zsh(), line 183: if init_content: is always True
# - So lines 184-189 always execute for zsh
# - ZDOTDIR is always set to temp dir for zsh
print("For zsh:")
print("1. _generate_shell_init() ALWAYS returns non-empty ✓")
print("2. init_content is ALWAYS non-empty for zsh ✓")
print("3. if init_content: branch ALWAYS taken in _launch_zsh() ✓")
print("4. ZDOTDIR ALWAYS set to temp dir for zsh ✓")
print("5. This means ZDOTDIR behavior impact is unavoidable for zsh ✓")
EOFRepository: jumpstarter-dev/jumpstarter
Length of output: 344
ZDOTDIR temp directory breaks user zsh configuration with profiles.
When jmp shell is used with use_profiles=True for zsh, _generate_shell_init() always produces non-empty content, triggering the temp ZDOTDIR setup. Setting ZDOTDIR to a temporary directory causes zsh to read startup files from that directory instead of the home directory—specifically, ~/.zshenv is never loaded. For users who source relative paths like $ZDOTDIR/config within their ~/.zshrc, these references resolve to the temp directory instead of the user's home config, breaking custom configurations.
🤖 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 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.
- 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>
Contributor
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
Member
|
Something is weird at my side: |
Member
|
this is MacOS / zsh |
Contributor
Author
|
@ambient-code investigate what could cause this issue. i do not have a mac to test. |
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>
Contributor
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
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
2 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.
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