Skip to content

Commit da62d1c

Browse files
committed
fix(remote): fix zsh @shell bootstrap failure and improve error messages
Root cause of "SSH exit code: 1" when using @shell zsh: 1. TERM=dumb in non-interactive SSH sessions caused prompt tools (starship, etc.) to fail during zsh initialization 2. ~/.zshrc defines interactive hooks (precmd/preexec/chpwd functions and hook arrays). After sourcing ~/.zshrc and re-enabling set -e, these hooks would run and fail in the non-interactive errexit context, causing the entire script to exit with code 1 before any user commands executed. The previous code cleaned hooks AFTER set -e, which was too late. Fixes: - executor.py: Restructure zsh bootstrap to clean interactive hooks (unset precmd_functions/preexec_functions/chpwd_functions and unfunction the hook functions) AFTER sourcing ~/.zshrc but BEFORE re-enabling set -e, preventing hooks from triggering in errexit mode - executor.py: Set safe TERM=xterm-256color early in bootstrap to avoid dumb terminal issues with prompt initialization tools - executor.py: Rewrite _zsh_cleanup_interactive_hooks() to safely check for function existence with (( $+functions[name] )) before unfunctioning, avoiding errors when functions do not exist - executor.py: Enhance _build_remote_error_message() to show failed command, command output, stderr details, and last 20 lines of stdout instead of just "SSH exit code: N" - executor.py: Pass cleaned_stdout to error message builder for complete failure context - executor.py: Add bootstrap command patterns to _is_trace_noise() to filter unset/unfunction/exec redirection commands from user- visible command logs - runner.py: Remove duplicate error message printing in _execute_remote_block_sequential (already printed by _execute_block_with_sequential_output) - tests: Update unit tests and BDD feature expectations to match new zsh bootstrap format All tests pass: 193 pytest + 30 BDD scenarios, lint, and typecheck.
1 parent f6c155c commit da62d1c

4 files changed

Lines changed: 81 additions & 22 deletions

File tree

features/execution_contract.feature

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ Feature: Agent-facing execution contract
3232
Given host "testhost" is configured in SSH config
3333
And a script file with a remote "zsh" block
3434
When I inspect the generated remote script payload
35-
Then the output should contain "test -f ~/.zshrc && { source ~/.zshrc >/dev/null 2>&1 || true; }"
35+
Then the output should contain "test -f ~/.zshrc && source ~/.zshrc >/dev/null 2>&1"
3636
And the output should contain "bootstrap-check"
3737

3838
Scenario: Remote bash payload bootstraps bashrc before user commands

src/shellflow/executor.py

Lines changed: 74 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -128,31 +128,60 @@ def _build_executable_script(
128128
return "\n".join(script_lines)
129129

130130

131-
def _build_zsh_hook_cleanup() -> list[str]:
132-
"""Disable zsh interactive hooks that pollute non-interactive automation."""
131+
def _zsh_cleanup_interactive_hooks() -> list[str]:
132+
"""Disable zsh interactive hooks that pollute non-interactive automation.
133+
134+
Returns individual commands WITHOUT set +e/set -e wrapping - the caller
135+
is responsible for error handling context.
136+
"""
133137
return [
134-
"unset preexec_functions precmd_functions chpwd_functions 2>/dev/null || true",
135-
"unfunction preexec precmd chpwd 2>/dev/null || true",
138+
"unset preexec_functions precmd_functions chpwd_functions 2>/dev/null",
139+
"(( $+functions[preexec] )) && unfunction preexec 2>/dev/null",
140+
"(( $+functions[precmd] )) && unfunction precmd 2>/dev/null",
141+
"(( $+functions[chpwd] )) && unfunction chpwd 2>/dev/null",
136142
]
137143

138144

139145
def _build_shell_bootstrap(shell: str | None) -> list[str]:
140-
"""Build shell-specific bootstrap lines needed for non-interactive automation."""
146+
"""Build shell-specific bootstrap lines needed for non-interactive automation.
147+
148+
We manually source rc files (.zshrc/.bashrc) because users often set PATH
149+
and other environment variables there. To do this safely in non-interactive
150+
SSH sessions (where TERM=dumb):
151+
1. Set TERM to a safe value (xterm-256color) to prevent prompt tools (starship, etc.)
152+
from failing due to dumb terminal detection
153+
2. Use set +e before sourcing, set -e after, to prevent rc file errors from
154+
aborting the script
155+
3. Silence output from rc file sourcing
156+
4. Clean up interactive hooks (precmd/preexec/chpwd) AFTER sourcing but BEFORE
157+
re-enabling set -e, because these hooks may run when errexit is enabled
158+
and cause spurious failures
159+
"""
141160
if not shell:
142161
return []
143162

144163
shell_name = Path(shell).name
145164
if shell_name == "zsh":
146165
return [
147166
"set +x 2>/dev/null || true",
148-
*_build_zsh_hook_cleanup(),
149-
"test -f ~/.zshrc && { source ~/.zshrc >/dev/null 2>&1 || true; }",
167+
# Clean up any inherited hooks before we start
168+
"set +e",
169+
*_zsh_cleanup_interactive_hooks(),
170+
# Set a safe TERM to prevent starship/prompt tools from failing on dumb terminals
171+
'export TERM="${TERM:-xterm-256color}"',
172+
"[[ $TERM == dumb ]] && export TERM=xterm-256color",
173+
# Source .zshrc, then immediately clean up hooks it may have defined
174+
# BEFORE re-enabling set -e (precmd hooks run when errexit is on)
175+
"test -f ~/.zshrc && source ~/.zshrc >/dev/null 2>&1",
176+
*_zsh_cleanup_interactive_hooks(),
177+
"set -e",
150178
"set +x 2>/dev/null || true",
151-
*_build_zsh_hook_cleanup(),
152179
]
153180
if shell_name == "bash":
154181
return [
155182
"set +x 2>/dev/null || true",
183+
'export TERM="${TERM:-xterm-256color}"',
184+
"[[ $TERM == dumb ]] && export TERM=xterm-256color",
156185
"test -f ~/.bashrc && { set +e; . ~/.bashrc >/dev/null 2>&1; set -e; }",
157186
]
158187
return ["set +x 2>/dev/null || true"]
@@ -227,23 +256,44 @@ def _combine_output(stdout: str, stderr: str) -> str:
227256
return output or error_output
228257

229258

230-
def _build_remote_error_message(exit_code: int, command_logs: list[CommandLog], stderr: str) -> str:
259+
def _build_remote_error_message(
260+
exit_code: int,
261+
command_logs: list[CommandLog],
262+
stderr: str,
263+
stdout: str = "",
264+
) -> str:
231265
"""Build a descriptive error message for a failed remote block."""
232266
failed_command = ""
267+
failed_output = ""
233268
if command_logs:
234269
for cl in reversed(command_logs):
235270
if cl.status == "failed" or (cl.exit_code is not None and cl.exit_code != 0):
236271
failed_command = cl.command
272+
failed_output = cl.output
237273
break
238274
if not failed_command and command_logs:
239275
failed_command = command_logs[-1].command
276+
failed_output = command_logs[-1].output
277+
240278
stderr_detail = stderr.strip()
279+
stdout_detail = stdout.strip()
280+
241281
if failed_command:
242-
error_message = f"Command failed (exit {exit_code}): {failed_command}"
282+
error_message = f"Command failed (exit code {exit_code}):\n $ {failed_command}"
243283
else:
244-
error_message = f"SSH exit code: {exit_code}"
245-
if stderr_detail:
246-
error_message += f"\n{stderr_detail}"
284+
error_message = f"Remote execution failed (exit code {exit_code})"
285+
286+
if failed_output:
287+
error_message += f"\n--- command output ---\n{failed_output}"
288+
289+
if stderr_detail and stderr_detail not in failed_output:
290+
error_message += f"\n--- stderr ---\n{stderr_detail}"
291+
292+
if stdout_detail and stdout_detail not in failed_output and stdout_detail not in stderr_detail:
293+
last_lines = "\n".join(stdout_detail.splitlines()[-20:])
294+
if last_lines:
295+
error_message += f"\n--- last output ---\n{last_lines}"
296+
247297
return error_message
248298

249299

@@ -293,6 +343,9 @@ def _is_trace_noise(command: str) -> bool:
293343
r"^export\s+[A-Z_][A-Z0-9_]*=",
294344
r"^test -f ~/.(?:bashrc|zshrc)",
295345
r"^\[{1,2}\s",
346+
r"^unset\s+pre(?:exec|cmd|chpwd)_functions\b",
347+
r"^\(\( \$\+functions\[",
348+
r"^exec\s+[0-9]+>&[0-9]+",
296349
)
297350
import re
298351

@@ -595,7 +648,11 @@ def execute_remote(
595648
ssh_args.extend(["-F", str(ssh_config_path)])
596649

597650
shell = block.shell or "bash"
598-
# Use --no-rcs for zsh or --norc for bash to prevent sourcing initialization files
651+
# Use --no-rcs/--norc to prevent default rc file sourcing, then we manually
652+
# source rc files in the bootstrap with proper error handling (set +e around source,
653+
# and TERM set to avoid prompt tool errors in dumb terminals).
654+
# Note: --no-rcs/--norc still allows .zshenv to be read (zsh), which is correct
655+
# as that file is meant for environment setup for all shells.
599656
ssh_target = ssh_config.hostname or host
600657
if "zsh" in shell:
601658
ssh_args.extend(["-o", "BatchMode=yes", ssh_target, shell, "--no-rcs", "-s", "-e"])
@@ -634,7 +691,9 @@ def execute_remote(
634691

635692
failure_kind = None if success else "runtime"
636693
result_exit_code = exit_code
637-
error_message = "" if success else _build_remote_error_message(exit_code, command_logs, cleaned_stderr)
694+
error_message = (
695+
"" if success else _build_remote_error_message(exit_code, command_logs, cleaned_stderr, cleaned_stdout)
696+
)
638697

639698
if timed_out:
640699
result_exit_code = -1

src/shellflow/runner.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1001,8 +1001,8 @@ def _execute_remote_block_sequential(
10011001
context.last_output = result.output
10021002
context.success = result.success
10031003

1004-
if not result.success and verbose:
1005-
print(f"{RED}{result.error_message}{RESET}\n")
1004+
# Note: error message is printed by _execute_block_with_sequential_output
1005+
# to avoid duplicate printing
10061006

10071007
return result
10081008

tests/test_shellflow.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -804,10 +804,10 @@ def test_remote_zsh_bootstraps_zshrc_before_commands(
804804

805805
assert result.success is True
806806
sent_script = mock_run.call_args.args[1]
807-
assert "test -f ~/.zshrc && { source ~/.zshrc >/dev/null 2>&1 || true; }" in sent_script
808-
assert sent_script.index(
809-
"test -f ~/.zshrc && { source ~/.zshrc >/dev/null 2>&1 || true; }"
810-
) < sent_script.index("mise --version")
807+
assert "test -f ~/.zshrc && source ~/.zshrc >/dev/null 2>&1" in sent_script
808+
assert sent_script.index("test -f ~/.zshrc && source ~/.zshrc >/dev/null 2>&1") < sent_script.index(
809+
"mise --version"
810+
)
811811

812812
def test_remote_execution_only_exports_explicit_context(
813813
self,

0 commit comments

Comments
 (0)