feat: implement shell integration for automatic cx fix error capture#744
feat: implement shell integration for automatic cx fix error capture#744nailoo wants to merge 1 commit intocxlinux-ai:mainfrom
Conversation
CLA Verification FailedThe following contributors have not signed the Contributor License Agreement:
How to Sign
This check runs automatically. Maintainers can update |
Summary of ChangesHello @nailoo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughThis PR introduces shell integration scripts for Bash and Zsh that automatically capture and persist error details (command, exit code, stderr) from failed commands in interactive shells. It includes a new setup command to install these integrations and update shell RC files. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant SetupCmd as SetupCommand<br/>(CLI)
participant FileSystem as File System
participant ShellRC as Shell RC Files
participant Shell as Bash/Zsh
participant Integration as Integration<br/>Script
User->>SetupCmd: cx setup shell
SetupCmd->>FileSystem: Create ~/.cx/shell/
SetupCmd->>FileSystem: Write bash_integration.sh
SetupCmd->>FileSystem: Write zsh_integration.sh
SetupCmd->>ShellRC: Update ~/.bashrc
SetupCmd->>ShellRC: Update ~/.zshrc
SetupCmd->>User: Print guidance
User->>Shell: Start interactive shell
Shell->>ShellRC: Source RC file
ShellRC->>Integration: Source integration script
Integration->>Shell: Register PROMPT_COMMAND/precmd
User->>Shell: Execute command (fails)
Shell->>Integration: Trigger error hook
Integration->>FileSystem: Capture stderr to ~/.cx/stderr.tmp
Integration->>FileSystem: Write ~/.cx/last_error with command/exit code/stderr
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces native shell integration for Bash and Zsh, enabling automatic capture of command-line errors for the cx fix command. The implementation includes new shell scripts for Bash and Zsh, which are embedded into the binary and deployed via a new cx setup shell command. This command also handles updating the user's .bashrc and .zshrc files. The cx fix command has been updated to automatically read the last captured error if no arguments are provided, enhancing user experience. The approach for capturing stderr using tee and integrating with shell hooks (PROMPT_COMMAND/precmd) is robust and well-considered.
| } | ||
|
|
||
| let content = std::fs::read_to_string(&rc_path)?; | ||
| let source_line = format!("source ~/.cx/shell/{}", script_path.file_name().unwrap().to_str().unwrap()); |
There was a problem hiding this comment.
The use of unwrap() twice on script_path.file_name() and to_str() can lead to a panic if the file name is unexpectedly None or contains non-UTF-8 characters. While unlikely for the current script names, it's safer to handle these Option and Result types explicitly to prevent potential crashes.
| let source_line = format!("source ~/.cx/shell/{}", script_path.file_name().unwrap().to_str().unwrap()); | |
| let source_line = format!("source ~/.cx/shell/{}", script_path.file_name().and_then(|s| s.to_str()).unwrap_or_default()); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
wezterm/src/cli/shortcuts.rs (2)
241-243:⚠️ Potential issue | 🟡 Minor
read_last_errorlacks the empty-HOMEguard thatsetup_shellhas.
setup_shell(line 100) bails whenHOMEis empty.read_last_errorsilently proceeds, joining an empty string with.cx/last_error, creating a relative path that resolves against CWD rather than the user's home directory.🔧 Proposed fix
fn read_last_error(&self) -> Result<String> { let home = std::env::var("HOME").unwrap_or_default(); + if home.is_empty() { + return Ok(String::new()); + } let error_file = std::path::Path::new(&home).join(".cx").join("last_error");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wezterm/src/cli/shortcuts.rs` around lines 241 - 243, read_last_error currently uses std::env::var("HOME").unwrap_or_default() which allows an empty HOME and builds a relative path; add the same empty-HOME guard as setup_shell: get HOME with std::env::var("HOME") and if it is empty return an Err (or bail) early instead of continuing, so that read_last_error does not join an empty home to ".cx/last_error"; update the code that computes error_file (the Path::new(&home).join(".cx").join("last_error")) to run only after the HOME check and preserve the function's Result<String> error type when returning the early error.
241-253:⚠️ Potential issue | 🟠 MajorClearing
last_errorbeforeask.run()completes causes permanent data loss on failure.Line 248 zeroes the file immediately after reading, before the fix operation has a chance to succeed. If
ask.run()subsequently fails (e.g., network unavailable, API error), the captured error context is gone and the user must repro the original failure.Move the clear to after a successful run, or rename to a
.bakfile first.🔧 Proposed fix
impl FixCommand { pub fn run(&self) -> Result<()> { let error_text = if self.error.is_empty() { - self.read_last_error()? + self.read_last_error(false)? } else { self.error.join(" ") }; if error_text.is_empty() { anyhow::bail!("No error to fix. Run a command first or provide an error message."); } let query = format!("fix this error: {}", error_text); let ask = AskCommand { /* ... */ }; - ask.run() + let result = ask.run(); + if result.is_ok() { + let _ = self.read_last_error(true); // consume/clear only on success + } + result } - fn read_last_error(&self) -> Result<String> { + fn read_last_error(&self, clear: bool) -> Result<String> { let home = std::env::var("HOME").unwrap_or_default(); let error_file = std::path::Path::new(&home).join(".cx").join("last_error"); if error_file.exists() { let content = std::fs::read_to_string(&error_file)?; - let _ = std::fs::write(&error_file, ""); + if clear { + let _ = std::fs::write(&error_file, ""); + } Ok(content.trim().to_string()) } else { Ok(String::new()) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wezterm/src/cli/shortcuts.rs` around lines 241 - 253, read_last_error currently wipes ~/.cx/last_error immediately after reading, risking permanent loss if the subsequent fix (e.g., ask.run) fails; update read_last_error to only read and return the content without clearing the file, and implement the clearing/rotation logic in the caller that runs the fix (or add a new helper like clear_last_error or rotate_last_error_to_bak) so the file is removed or renamed to .bak only after ask.run (or the fix function) succeeds—alternatively, rename the file to last_error.bak before running the fix and delete the .bak on success (refer to read_last_error and the caller that invokes ask.run to place the new clear/rename logic).
🧹 Nitpick comments (1)
wezterm/src/cli/shortcuts.rs (1)
98-154: New I/O functions use synchronousstd::fs— async I/O is required by the coding guidelines.
setup_shell,update_rc_file, andread_last_errorall block on disk I/O viastd::fs::read_to_string,std::fs::write, andstd::fs::create_dir_all. Per coding guidelines, Rust files must use async patterns for I/O operations (Tokio async/await).🔧 Example conversion sketch
- fn setup_shell(&self) -> Result<()> { + async fn setup_shell(&self) -> Result<()> { ... - std::fs::create_dir_all(&shell_dir)?; - std::fs::write(&bash_script, bash_content)?; - std::fs::write(&zsh_script, zsh_content)?; + tokio::fs::create_dir_all(&shell_dir).await?; + tokio::fs::write(&bash_script, bash_content).await?; + tokio::fs::write(&zsh_script, zsh_content).await?; ... } - fn update_rc_file(...) -> Result<()> { + async fn update_rc_file(...) -> Result<()> { - let content = std::fs::read_to_string(&rc_path)?; + let content = tokio::fs::read_to_string(&rc_path).await?; ... }Based on coding guidelines: "Use async patterns for I/O operations" and "Use Tokio async/await patterns for Rust async operations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wezterm/src/cli/shortcuts.rs` around lines 98 - 154, The functions setup_shell, update_rc_file (and the referenced read_last_error) are using blocking std::fs I/O and must be converted to async Tokio I/O; change setup_shell and update_rc_file to async fn and update any callers to .await, replace std::fs::create_dir_all/read_to_string/write/OpenOptions with tokio::fs::create_dir_all, tokio::fs::read_to_string, tokio::fs::write and tokio::fs::OpenOptions, and replace synchronous writeln! on an appended std::fs::File with an async write using tokio::io::AsyncWriteExt (write_all) after opening with OpenOptions::append(). Keep the same file paths and the source_line construction, but ensure all filesystem calls are awaited and error types still return Result by using anyhow or the crate's existing error handling; also update the signature/types for update_rc_file to accept &Path or &str consistently and adjust callers accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@assets/shell/bash_integration.sh`:
- Line 19: The assignment using local last_cmd=$(history 1 | sed 's/^[ ]*[0-9]*[
]*//') masks the subshell return value (SC2155); change it to declare the
variable first and then assign the subshell output to it (e.g., use local
last_cmd on one line, then set last_cmd=$(history 1 | sed 's/^[ ]*[0-9]*[ ]*//')
on the next) so the subshell's exit status isn't masked; refer to the variable
name last_cmd to locate the change.
- Around line 48-49: The current creation of the config directory using mkdir -p
"$HOME/.cx" leaves default permissions which may be world-readable; change the
creation to set restrictive permissions (700) and ensure existing directories
are corrected: use a creation command that sets mode (e.g., mkdir -m 0700 for
the "$HOME/.cx" directory) or call chmod 0700 after mkdir -p "$HOME/.cx", so any
files or error output stored there are not world-readable; update the code paths
that call mkdir -p "$HOME/.cx" accordingly and ensure permission correction runs
unconditionally for existing directories.
- Around line 51-54: The current global stderr redirection using "exec 2> >(tee
\"$HOME/.cx/stderr.tmp\" >&2)" creates an async flush race and can chain
multiple tee processes when the script is re-sourced; modify the stderr capture
logic around the exec and the error handler __cx_err_capture so it is idempotent
and waits briefly for the async tee to flush before checking stderr.tmp: add an
idempotency guard (e.g., a sentinel variable or check to avoid rebinding
exec/rehooking PROMPT_COMMAND), record the tee PID when you start the process so
you can avoid starting another or kill the orphan on re-source, and before
reading "$HOME/.cx/stderr.tmp" (inside __cx_err_capture or PROMPT_COMMAND) wait
briefly or wait on the tee PID to ensure it has flushed (a short sleep like
0.05s or a wait on the saved PID); ensure the logic references the same
stderr.tmp file name and properly cleans/rotates the PID sentinel to prevent
multiple chained tee processes.
In `@assets/shell/zsh_integration.sh`:
- Around line 13-19: In __cx_err_capture_precmd(), avoid masking return values
by separating declaration and assignment for local variables: declare local
exit_code first and then set exit_code=$? (instead of local exit_code=$?), and
similarly declare local last_cmd before assigning last_cmd=$(fc -ln -1); update
the __cx_err_capture_precmd function to use these two-step local declarations so
the exit status isn't lost.
- Line 47: Guard the stderr tee setup with the same re-source guard used in
bash: check and export __CX_STDERR_HOOKED so the exec 2> >(tee
"$HOME/.cx/stderr.tmp" >&2) line runs only once; before creating the temp file
ensure the directory "$HOME/.cx" exists (mkdir -p "$HOME/.cx") and harden
permissions with chmod 700 "$HOME/.cx"; leave the exec redirection target as
stderr.tmp but perform the directory creation and chmod prior to the exec and
set the __CX_STDERR_HOOKED flag after the setup.
- Line 22: The zsh conditional that checks last_cmd (the if using last_cmd =~
"cx fix") uses an unanchored pattern and will match any string containing "cx
fix"; change that condition to use an anchored regular expression that begins
with ^ and escapes the space so it only matches commands that start with "cx
fix" (i.e., replace the unanchored pattern in the if [[ "$last_cmd" =~ "cx fix"
]] test with an anchored ^cx\ fix pattern).
In `@wezterm/src/cli/shortcuts.rs`:
- Line 138: Replace the unwrap chain that builds source_line: instead of calling
script_path.file_name().unwrap().to_str().unwrap(), extract a UTF-8 file name
with script_path.file_name().and_then(|n| n.to_str()).ok_or_else(||
anyhow::anyhow!("invalid script path file name or non-UTF8 bytes: {:?}",
script_path))?, then build source_line with that string; update the surrounding
function to return anyhow::Result so the ? can propagate the error (or add
.context(...) from anyhow for clearer messages). Reference: source_line,
script_path.file_name(), to_str(), and make the function return anyhow::Result
to propagate failures rather than panicking.
- Around line 107-115: The code currently unconditionally uses std::fs::write to
write embedded bash_content and zsh_content into bash_script and zsh_script,
which will silently overwrite any user-modified files; update the logic in
shortcuts.rs to check for the files' existence (e.g., using Path::exists or
std::fs::metadata on bash_script and zsh_script) and if present either skip
writing and print a warning or require an explicit force option (e.g., a --force
bool flag parsed by the CLI) to overwrite; ensure you only call std::fs::write
when the file does not exist or when force is true, and surface a clear warning
message naming bash_script/zsh_script when skipping.
---
Outside diff comments:
In `@wezterm/src/cli/shortcuts.rs`:
- Around line 241-243: read_last_error currently uses
std::env::var("HOME").unwrap_or_default() which allows an empty HOME and builds
a relative path; add the same empty-HOME guard as setup_shell: get HOME with
std::env::var("HOME") and if it is empty return an Err (or bail) early instead
of continuing, so that read_last_error does not join an empty home to
".cx/last_error"; update the code that computes error_file (the
Path::new(&home).join(".cx").join("last_error")) to run only after the HOME
check and preserve the function's Result<String> error type when returning the
early error.
- Around line 241-253: read_last_error currently wipes ~/.cx/last_error
immediately after reading, risking permanent loss if the subsequent fix (e.g.,
ask.run) fails; update read_last_error to only read and return the content
without clearing the file, and implement the clearing/rotation logic in the
caller that runs the fix (or add a new helper like clear_last_error or
rotate_last_error_to_bak) so the file is removed or renamed to .bak only after
ask.run (or the fix function) succeeds—alternatively, rename the file to
last_error.bak before running the fix and delete the .bak on success (refer to
read_last_error and the caller that invokes ask.run to place the new
clear/rename logic).
---
Nitpick comments:
In `@wezterm/src/cli/shortcuts.rs`:
- Around line 98-154: The functions setup_shell, update_rc_file (and the
referenced read_last_error) are using blocking std::fs I/O and must be converted
to async Tokio I/O; change setup_shell and update_rc_file to async fn and update
any callers to .await, replace
std::fs::create_dir_all/read_to_string/write/OpenOptions with
tokio::fs::create_dir_all, tokio::fs::read_to_string, tokio::fs::write and
tokio::fs::OpenOptions, and replace synchronous writeln! on an appended
std::fs::File with an async write using tokio::io::AsyncWriteExt (write_all)
after opening with OpenOptions::append(). Keep the same file paths and the
source_line construction, but ensure all filesystem calls are awaited and error
types still return Result by using anyhow or the crate's existing error
handling; also update the signature/types for update_rc_file to accept &Path or
&str consistently and adjust callers accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
assets/shell/bash_integration.shassets/shell/zsh_integration.shwezterm/src/cli/shortcuts.rs
| if [ $exit_code -ne 0 ]; then | ||
| if [ -s "$HOME/.cx/stderr.tmp" ]; then | ||
| # Get the last command from history | ||
| local last_cmd=$(history 1 | sed 's/^[ ]*[0-9]*[ ]*//') |
There was a problem hiding this comment.
Declare last_cmd separately to avoid masking the subshell's return value (SC2155).
🔧 Proposed fix
- local last_cmd=$(history 1 | sed 's/^[ ]*[0-9]*[ ]*//')
+ local last_cmd
+ last_cmd=$(history 1 | sed 's/^[ ]*[0-9]*[ ]*//')📝 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.
| local last_cmd=$(history 1 | sed 's/^[ ]*[0-9]*[ ]*//') | |
| local last_cmd | |
| last_cmd=$(history 1 | sed 's/^[ ]*[0-9]*[ ]*//') |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 19-19: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/shell/bash_integration.sh` at line 19, The assignment using local
last_cmd=$(history 1 | sed 's/^[ ]*[0-9]*[ ]*//') masks the subshell return
value (SC2155); change it to declare the variable first and then assign the
subshell output to it (e.g., use local last_cmd on one line, then set
last_cmd=$(history 1 | sed 's/^[ ]*[0-9]*[ ]*//') on the next) so the subshell's
exit status isn't masked; refer to the variable name last_cmd to locate the
change.
| # Ensure .cx directory exists | ||
| mkdir -p "$HOME/.cx" |
There was a problem hiding this comment.
Create ~/.cx with restricted permissions to protect potentially sensitive error output.
Command stderr can contain credentials, tokens, or PII in error messages. The directory and its files should not be world-readable.
🛡️ Proposed fix
-mkdir -p "$HOME/.cx"
+mkdir -p "$HOME/.cx"
+chmod 700 "$HOME/.cx"Alternatively, combine into one step (Linux mkdir -m):
-mkdir -p "$HOME/.cx"
+mkdir -p -m 700 "$HOME/.cx"📝 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.
| # Ensure .cx directory exists | |
| mkdir -p "$HOME/.cx" | |
| # Ensure .cx directory exists | |
| mkdir -p "$HOME/.cx" | |
| chmod 700 "$HOME/.cx" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/shell/bash_integration.sh` around lines 48 - 49, The current creation
of the config directory using mkdir -p "$HOME/.cx" leaves default permissions
which may be world-readable; change the creation to set restrictive permissions
(700) and ensure existing directories are corrected: use a creation command that
sets mode (e.g., mkdir -m 0700 for the "$HOME/.cx" directory) or call chmod 0700
after mkdir -p "$HOME/.cx", so any files or error output stored there are not
world-readable; update the code paths that call mkdir -p "$HOME/.cx" accordingly
and ensure permission correction runs unconditionally for existing directories.
| # Global stderr capture | ||
| # Note: we use 'tee' to ensure the user still sees the error on their screen. | ||
| # We redirect stderr (2) through tee which also writes to stderr.tmp | ||
| exec 2> >(tee "$HOME/.cx/stderr.tmp" >&2) |
There was a problem hiding this comment.
Two issues with exec 2> >(tee ...): async race and missing idempotency guard.
- Race condition: the tee process runs asynchronously. When PROMPT_COMMAND fires, tee may not have finished flushing all bytes to
stderr.tmp, causing[ -s "$HOME/.cx/stderr.tmp" ]to see an empty or truncated file. - Fd chain on re-source: there is no guard around line 54. If this script is sourced a second time (e.g.,
source ~/.bashrcmid-session), a new tee is chained through the previous one, producing double-writes tostderr.tmpand leaving orphaned background processes.
🔧 Proposed fix
+# Guard against double-hooking stderr
+if [[ -z "$__CX_STDERR_HOOKED" ]]; then
+ __CX_STDERR_HOOKED=1
exec 2> >(tee "$HOME/.cx/stderr.tmp" >&2)
+fiFor the flush race, consider writing the PID of the tee process and briefly waiting on it before reading, or using a named pipe with blocking reads. A pragmatic short-term option that adds minimal latency:
__cx_err_capture() {
local exit_code=$?
+ # Give async tee a moment to flush before reading
+ sleep 0.05
if [ $exit_code -ne 0 ]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/shell/bash_integration.sh` around lines 51 - 54, The current global
stderr redirection using "exec 2> >(tee \"$HOME/.cx/stderr.tmp\" >&2)" creates
an async flush race and can chain multiple tee processes when the script is
re-sourced; modify the stderr capture logic around the exec and the error
handler __cx_err_capture so it is idempotent and waits briefly for the async tee
to flush before checking stderr.tmp: add an idempotency guard (e.g., a sentinel
variable or check to avoid rebinding exec/rehooking PROMPT_COMMAND), record the
tee PID when you start the process so you can avoid starting another or kill the
orphan on re-source, and before reading "$HOME/.cx/stderr.tmp" (inside
__cx_err_capture or PROMPT_COMMAND) wait briefly or wait on the tee PID to
ensure it has flushed (a short sleep like 0.05s or a wait on the saved PID);
ensure the logic references the same stderr.tmp file name and properly
cleans/rotates the PID sentinel to prevent multiple chained tee processes.
| __cx_err_capture_precmd() { | ||
| local exit_code=$? | ||
|
|
||
| if [[ $exit_code -ne 0 ]]; then | ||
| if [[ -s "$HOME/.cx/stderr.tmp" ]]; then | ||
| # In Zsh, the last command is available via fc | ||
| local last_cmd=$(fc -ln -1) |
There was a problem hiding this comment.
Declare exit_code and last_cmd separately to avoid masking return values.
Same SC2155 pattern as the Bash counterpart.
🔧 Proposed fix
__cx_err_capture_precmd() {
- local exit_code=$?
+ local exit_code
+ exit_code=$?
if [[ $exit_code -ne 0 ]]; then
if [[ -s "$HOME/.cx/stderr.tmp" ]]; then
- local last_cmd=$(fc -ln -1)
+ local last_cmd
+ last_cmd=$(fc -ln -1)📝 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.
| __cx_err_capture_precmd() { | |
| local exit_code=$? | |
| if [[ $exit_code -ne 0 ]]; then | |
| if [[ -s "$HOME/.cx/stderr.tmp" ]]; then | |
| # In Zsh, the last command is available via fc | |
| local last_cmd=$(fc -ln -1) | |
| __cx_err_capture_precmd() { | |
| local exit_code | |
| exit_code=$? | |
| if [[ $exit_code -ne 0 ]]; then | |
| if [[ -s "$HOME/.cx/stderr.tmp" ]]; then | |
| # In Zsh, the last command is available via fc | |
| local last_cmd | |
| last_cmd=$(fc -ln -1) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/shell/zsh_integration.sh` around lines 13 - 19, In
__cx_err_capture_precmd(), avoid masking return values by separating declaration
and assignment for local variables: declare local exit_code first and then set
exit_code=$? (instead of local exit_code=$?), and similarly declare local
last_cmd before assigning last_cmd=$(fc -ln -1); update the
__cx_err_capture_precmd function to use these two-step local declarations so the
exit status isn't lost.
| local last_cmd=$(fc -ln -1) | ||
|
|
||
| # Don't capture if it was 'cx fix' or similar | ||
| if [[ "$last_cmd" =~ "cx fix" ]]; then |
There was a problem hiding this comment.
Unanchored pattern excludes more commands than intended — inconsistent with the Bash version.
"cx fix" matches any command containing "cx fix" (e.g., echo "cx fix guide" would be silently skipped). The Bash script correctly anchors with ^cx\ fix. Apply the same anchor here.
🔧 Proposed fix
- if [[ "$last_cmd" =~ "cx fix" ]]; then
+ if [[ "$last_cmd" =~ ^cx[[:space:]]fix ]]; then📝 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.
| if [[ "$last_cmd" =~ "cx fix" ]]; then | |
| if [[ "$last_cmd" =~ ^cx[[:space:]]fix ]]; then |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/shell/zsh_integration.sh` at line 22, The zsh conditional that checks
last_cmd (the if using last_cmd =~ "cx fix") uses an unanchored pattern and will
match any string containing "cx fix"; change that condition to use an anchored
regular expression that begins with ^ and escapes the space so it only matches
commands that start with "cx fix" (i.e., replace the unanchored pattern in the
if [[ "$last_cmd" =~ "cx fix" ]] test with an anchored ^cx\ fix pattern).
| mkdir -p "$HOME/.cx" | ||
|
|
||
| # Global stderr capture | ||
| exec 2> >(tee "$HOME/.cx/stderr.tmp" >&2) |
There was a problem hiding this comment.
Same async-tee race condition and missing re-source guard as bash_integration.sh line 54.
The same __CX_STDERR_HOOKED guard fix and permission hardening (chmod 700 "$HOME/.cx") described for the Bash script apply here as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/shell/zsh_integration.sh` at line 47, Guard the stderr tee setup with
the same re-source guard used in bash: check and export __CX_STDERR_HOOKED so
the exec 2> >(tee "$HOME/.cx/stderr.tmp" >&2) line runs only once; before
creating the temp file ensure the directory "$HOME/.cx" exists (mkdir -p
"$HOME/.cx") and harden permissions with chmod 700 "$HOME/.cx"; leave the exec
redirection target as stderr.tmp but perform the directory creation and chmod
prior to the exec and set the __CX_STDERR_HOOKED flag after the setup.
| let bash_script = shell_dir.join("bash_integration.sh"); | ||
| let zsh_script = shell_dir.join("zsh_integration.sh"); | ||
|
|
||
| // We embed the script contents here so they are available in the binary | ||
| let bash_content = include_str!("../../../assets/shell/bash_integration.sh"); | ||
| let zsh_content = include_str!("../../../assets/shell/zsh_integration.sh"); | ||
|
|
||
| std::fs::write(&bash_script, bash_content)?; | ||
| std::fs::write(&zsh_script, zsh_content)?; |
There was a problem hiding this comment.
cx setup shell silently overwrites user-customized integration scripts.
std::fs::write will clobber ~/.cx/shell/bash_integration.sh and ~/.cx/shell/zsh_integration.sh if the user has modified them. At minimum, check for existence and warn (or skip with a flag like --force).
🔧 Suggested change
- std::fs::write(&bash_script, bash_content)?;
- std::fs::write(&zsh_script, zsh_content)?;
+ for (path, content) in [(&bash_script, bash_content), (&zsh_script, zsh_content)] {
+ if path.exists() && !self.auto_confirm {
+ println!(" ⚠️ {} already exists, skipping (use --yes to overwrite)", path.display());
+ } else {
+ std::fs::write(path, content)?;
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wezterm/src/cli/shortcuts.rs` around lines 107 - 115, The code currently
unconditionally uses std::fs::write to write embedded bash_content and
zsh_content into bash_script and zsh_script, which will silently overwrite any
user-modified files; update the logic in shortcuts.rs to check for the files'
existence (e.g., using Path::exists or std::fs::metadata on bash_script and
zsh_script) and if present either skip writing and print a warning or require an
explicit force option (e.g., a --force bool flag parsed by the CLI) to
overwrite; ensure you only call std::fs::write when the file does not exist or
when force is true, and surface a clear warning message naming
bash_script/zsh_script when skipping.
| } | ||
|
|
||
| let content = std::fs::read_to_string(&rc_path)?; | ||
| let source_line = format!("source ~/.cx/shell/{}", script_path.file_name().unwrap().to_str().unwrap()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace .unwrap() chains with proper anyhow error propagation.
file_name() can return None (path ending in ..) and to_str() returns None on non-UTF-8 bytes. Both currently panic. Per coding guidelines, errors must be propagated via Result/anyhow.
🔧 Proposed fix
- let source_line = format!("source ~/.cx/shell/{}", script_path.file_name().unwrap().to_str().unwrap());
+ let file_name = script_path
+ .file_name()
+ .and_then(|n| n.to_str())
+ .ok_or_else(|| anyhow::anyhow!("Invalid script path: {:?}", script_path))?;
+ let source_line = format!("source ~/.cx/shell/{}", file_name);📝 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.
| let source_line = format!("source ~/.cx/shell/{}", script_path.file_name().unwrap().to_str().unwrap()); | |
| let file_name = script_path | |
| .file_name() | |
| .and_then(|n| n.to_str()) | |
| .ok_or_else(|| anyhow::anyhow!("Invalid script path: {:?}", script_path))?; | |
| let source_line = format!("source ~/.cx/shell/{}", file_name); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wezterm/src/cli/shortcuts.rs` at line 138, Replace the unwrap chain that
builds source_line: instead of calling
script_path.file_name().unwrap().to_str().unwrap(), extract a UTF-8 file name
with script_path.file_name().and_then(|n| n.to_str()).ok_or_else(||
anyhow::anyhow!("invalid script path file name or non-UTF8 bytes: {:?}",
script_path))?, then build source_line with that string; update the surrounding
function to return anyhow::Result so the ? can propagate the error (or add
.context(...) from anyhow for clearer messages). Reference: source_line,
script_path.file_name(), to_str(), and make the function return anyhow::Result
to propagate failures rather than panicking.
There was a problem hiding this comment.
Pull request overview
This PR introduces native shell integration for Bash and Zsh to automatically capture command-line errors for CX Linux's AI-powered error resolution feature. The implementation adds shell hooks that redirect stderr to temporary files and save failed command information to ~/.cx/last_error, enabling the cx fix command to automatically analyze the last error without manual user input.
Changes:
- Added automated shell error capture via PROMPT_COMMAND (Bash) and precmd hooks (Zsh) that save failed commands and their stderr output
- Implemented
cx setup shellcommand that embeds integration scripts in the binary and automatically updates ~/.bashrc and ~/.zshrc - Extended
cx fixto automatically read captured errors from~/.cx/last_errorwhen invoked without arguments
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 16 comments.
| File | Description |
|---|---|
| wezterm/src/cli/shortcuts.rs | Added setup_shell() method and update_rc_file() helper to install shell integration; integrated error reading in FixCommand |
| assets/shell/bash_integration.sh | Bash shell integration using PROMPT_COMMAND hook and stderr redirection via process substitution |
| assets/shell/zsh_integration.sh | Zsh shell integration using precmd hook with fc command history and stderr redirection |
| } | ||
|
|
||
| let content = std::fs::read_to_string(&rc_path)?; | ||
| let source_line = format!("source ~/.cx/shell/{}", script_path.file_name().unwrap().to_str().unwrap()); |
There was a problem hiding this comment.
The source_line uses a hardcoded path (~/.cx/shell/) which may not match the actual installation directory if HOME contains symlinks or if the user has a non-standard shell configuration. Consider using the full absolute path from shell_dir instead of reconstructing it, or at least use $HOME variable in the source line to make it more portable.
| let source_line = format!("source ~/.cx/shell/{}", script_path.file_name().unwrap().to_str().unwrap()); | |
| let source_line = format!("source {}", script_path.display()); |
| fn setup_shell(&self) -> Result<()> { | ||
| let home = std::env::var("HOME").unwrap_or_default(); | ||
| if home.is_empty() { | ||
| anyhow::bail!("HOME environment variable not set"); | ||
| } | ||
| let cx_dir = std::path::Path::new(&home).join(".cx"); | ||
| let shell_dir = cx_dir.join("shell"); | ||
| std::fs::create_dir_all(&shell_dir)?; | ||
|
|
||
| let bash_script = shell_dir.join("bash_integration.sh"); | ||
| let zsh_script = shell_dir.join("zsh_integration.sh"); | ||
|
|
||
| // We embed the script contents here so they are available in the binary | ||
| let bash_content = include_str!("../../../assets/shell/bash_integration.sh"); | ||
| let zsh_content = include_str!("../../../assets/shell/zsh_integration.sh"); | ||
|
|
||
| std::fs::write(&bash_script, bash_content)?; | ||
| std::fs::write(&zsh_script, zsh_content)?; | ||
|
|
||
| // Update .bashrc | ||
| self.update_rc_file(&home, ".bashrc", &bash_script)?; | ||
| // Update .zshrc | ||
| self.update_rc_file(&home, ".zshrc", &zsh_script)?; | ||
|
|
||
| println!("\n✅ CX Terminal shell integration installed!"); | ||
| println!("The integration will capture errors and allow 'cx fix' to read them automatically."); | ||
| println!("\nTo activate now, run:"); | ||
| println!(" source ~/.bashrc # if using Bash"); | ||
| println!(" source ~/.zshrc # if using Zsh"); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| fn update_rc_file(&self, home: &str, rc_name: &str, script_path: &std::path::Path) -> Result<()> { | ||
| let rc_path = std::path::Path::new(home).join(rc_name); | ||
| if !rc_path.exists() { | ||
| return Ok(()); // Skip if shell is not present | ||
| } | ||
|
|
||
| let content = std::fs::read_to_string(&rc_path)?; | ||
| let source_line = format!("source ~/.cx/shell/{}", script_path.file_name().unwrap().to_str().unwrap()); | ||
|
|
||
| if content.contains(&source_line) { | ||
| println!(" ℹ️ Shell integration already present in {}", rc_name); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let mut file = std::fs::OpenOptions::new() | ||
| .append(true) | ||
| .open(&rc_path)?; | ||
|
|
||
| writeln!(file, "\n# CX Terminal Shell Integration")?; | ||
| writeln!(file, "{}", source_line)?; | ||
| println!(" ✅ Added integration to {}", rc_name); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The test coverage for new shell integration functionality (setup_shell, update_rc_file, and FixCommand.read_last_error) is missing. The codebase follows a pattern of including #[cfg(test)] blocks in CLI modules (see ask.rs, ask_context.rs, ask_patterns.rs, license.rs). Add unit tests to verify: 1) setup_shell creates directories and files correctly, 2) update_rc_file handles existing entries, missing files, and appends correctly, 3) read_last_error handles missing files and clears content after reading.
| add-zsh-hook precmd __cx_err_capture_precmd | ||
|
|
||
| # Ensure .cx directory exists | ||
| mkdir -p "$HOME/.cx" |
There was a problem hiding this comment.
Same issue as bash_integration.sh: mkdir -p at line 44 will succeed even if $HOME is empty or unset. Add a guard check to validate $HOME before creating directories.
| # Don't capture if it was 'cx fix' or similar | ||
| if [[ "$last_cmd" =~ ^cx\ fix ]]; then |
There was a problem hiding this comment.
The check for 'cx fix' only matches commands that start with 'cx fix' (using ^cx\ fix regex in Bash). This means commands like 'sudo cx fix' or 'command cx fix' would still be captured, potentially creating an infinite loop or confusing behavior. Consider using a more comprehensive check that handles various command prefixes.
| # Don't capture if it was 'cx fix' or similar | |
| if [[ "$last_cmd" =~ ^cx\ fix ]]; then | |
| # Don't capture if it was 'cx fix' or common prefixed variants (e.g. sudo cx fix) | |
| if [[ "$last_cmd" =~ ^([[:space:]]*(sudo|command|env|nice|nohup)[[:space:]]+)*cx[[:space:]]+fix ]]; then |
| __cx_err_capture() { | ||
| local exit_code=$? | ||
|
|
||
| if [ $exit_code -ne 0 ]; then | ||
| if [ -s "$HOME/.cx/stderr.tmp" ]; then | ||
| # Get the last command from history | ||
| local last_cmd=$(history 1 | sed 's/^[ ]*[0-9]*[ ]*//') | ||
|
|
||
| # Don't capture if it was 'cx fix' or similar | ||
| if [[ "$last_cmd" =~ ^cx\ fix ]]; then | ||
| : > "$HOME/.cx/stderr.tmp" | ||
| return | ||
| fi | ||
|
|
||
| { | ||
| echo "Command: $last_cmd" | ||
| echo "Exit Code: $exit_code" | ||
| echo "--- Stderr ---" | ||
| cat "$HOME/.cx/stderr.tmp" | ||
| } > "$HOME/.cx/last_error" | ||
| fi | ||
| fi | ||
| # Clear for next command | ||
| : > "$HOME/.cx/stderr.tmp" |
There was a problem hiding this comment.
Race condition: The stderr.tmp and last_error files are accessed without any locking mechanism. If multiple shell sessions are running concurrently (e.g., in tmux/screen), they will all write to the same files, causing corrupted captures or lost errors. Consider using session-specific files (e.g., stderr.tmp.$$ or using $PPID) or implement file locking.
| # Global stderr capture | ||
| exec 2> >(tee "$HOME/.cx/stderr.tmp" >&2) |
There was a problem hiding this comment.
Similar to the Bash version, this exec redirection using process substitution creates a persistent background process. In Zsh, this can cause the shell to hang on exit until the background process completes. Test the behavior with rapid shell exits (exit, logout, or Ctrl+D) to ensure the shell doesn't hang.
| # Global stderr capture | |
| exec 2> >(tee "$HOME/.cx/stderr.tmp" >&2) | |
| # Global stderr capture without process substitution to avoid persistent background jobs | |
| _cx_stderr_fifo="$HOME/.cx/stderr.fifo" | |
| # Create FIFO if needed, removing any stale non-FIFO file | |
| if [[ ! -p "$_cx_stderr_fifo" ]]; then | |
| [[ -e "$_cx_stderr_fifo" ]] && rm -f "$_cx_stderr_fifo" | |
| mkfifo "$_cx_stderr_fifo" | |
| fi | |
| # Start tee in the background, disowned so zsh does not wait on it at exit | |
| tee "$HOME/.cx/stderr.tmp" <"$_cx_stderr_fifo" >&2 &! | |
| # Redirect shell stderr into the FIFO for capture | |
| exec 2>"$_cx_stderr_fifo" |
| # Don't capture if it was 'cx fix' or similar | ||
| if [[ "$last_cmd" =~ "cx fix" ]]; then |
There was a problem hiding this comment.
The Zsh regex check uses a simple pattern match [[ "$last_cmd" =~ "cx fix" ]] which will match "cx fix" anywhere in the command string, not just at the beginning. This means commands like "echo cx fix" or comments containing "cx fix" would also be excluded from capture. Use a more precise pattern like [[ "$last_cmd" =~ (^|[[:space:]])cx[[:space:]]fix ]] or similar to match word boundaries.
| # Don't capture if it was 'cx fix' or similar | |
| if [[ "$last_cmd" =~ "cx fix" ]]; then | |
| # Don't capture if the command was 'cx fix' (with optional args), not just any line containing that substring | |
| if [[ "$last_cmd" =~ ^[[:space:]]*cx[[:space:]]+fix([[:space:]]|$) ]]; then |
| if [[ ! "$PROMPT_COMMAND" =~ __cx_err_capture ]]; then | ||
| if [[ -z "$PROMPT_COMMAND" ]]; then | ||
| PROMPT_COMMAND="__cx_err_capture" | ||
| else | ||
| PROMPT_COMMAND="__cx_err_capture; $PROMPT_COMMAND" |
There was a problem hiding this comment.
The PROMPT_COMMAND modification uses string matching to check if __cx_err_capture is already present, but this could have false positives if the user has a variable or function name containing this substring. Consider using a more robust check or setting a guard variable to prevent double-initialization.
| if [[ ! "$PROMPT_COMMAND" =~ __cx_err_capture ]]; then | |
| if [[ -z "$PROMPT_COMMAND" ]]; then | |
| PROMPT_COMMAND="__cx_err_capture" | |
| else | |
| PROMPT_COMMAND="__cx_err_capture; $PROMPT_COMMAND" | |
| if [[ -z "${__CX_ERR_CAPTURE_INSTALLED:-}" ]]; then | |
| __CX_ERR_CAPTURE_INSTALLED=1 | |
| if [[ -z "${PROMPT_COMMAND:-}" ]]; then | |
| PROMPT_COMMAND="__cx_err_capture" | |
| else | |
| case ";${PROMPT_COMMAND};" in | |
| *";__cx_err_capture;"*) | |
| # Already present as a standalone command; do nothing | |
| ;; | |
| *) | |
| PROMPT_COMMAND="__cx_err_capture; ${PROMPT_COMMAND}" | |
| ;; | |
| esac |
| } | ||
|
|
||
| let content = std::fs::read_to_string(&rc_path)?; | ||
| let source_line = format!("source ~/.cx/shell/{}", script_path.file_name().unwrap().to_str().unwrap()); |
There was a problem hiding this comment.
Multiple unwrap() calls on Option types without proper error handling. If script_path.file_name() returns None (e.g., if the path ends with ".."), or if to_str() fails for non-UTF8 filenames, this will panic. Use proper error handling with ? operator and anyhow::Context for better error messages. For example: script_path.file_name().context("Invalid script path")?.to_str().context("Non-UTF8 filename")?
| # We redirect stderr (2) through tee which also writes to stderr.tmp | ||
| exec 2> >(tee "$HOME/.cx/stderr.tmp" >&2) |
There was a problem hiding this comment.
The exec redirection using process substitution (exec 2> >(tee ...)) creates a background process that persists. If the user exits the shell quickly, the background tee process might become orphaned or cause the shell to hang. This is a known issue with process substitution in exec redirects. Consider using a named pipe (mkfifo) or a more robust approach, or at minimum document this limitation and test exit behavior.
| # We redirect stderr (2) through tee which also writes to stderr.tmp | |
| exec 2> >(tee "$HOME/.cx/stderr.tmp" >&2) | |
| # We redirect stderr (2) through a named pipe that tee reads from, which also writes to stderr.tmp. | |
| if [[ -z "${__CX_STDERR_TEE_PID:-}" ]]; then | |
| __cx_stderr_fifo="$HOME/.cx/stderr_fifo_$$" | |
| # Create a per-shell FIFO for stderr | |
| if [ ! -p "$__cx_stderr_fifo" ]; then | |
| mkfifo "$__cx_stderr_fifo" 2>/dev/null || { | |
| # If we cannot create the FIFO, skip installing global capture to avoid breaking the shell | |
| return | |
| } | |
| fi | |
| # Start tee in the background to read from the FIFO and write to both stderr.tmp and real stderr | |
| tee "$HOME/.cx/stderr.tmp" >&2 <"$__cx_stderr_fifo" & | |
| __CX_STDERR_TEE_PID=$! | |
| # Ensure we clean up the FIFO and tee process when the shell exits | |
| trap 'rm -f "$__cx_stderr_fifo"; kill "$__CX_STDERR_TEE_PID" 2>/dev/null || true' EXIT | |
| # Redirect this shell's stderr to the FIFO | |
| exec 2>"$__cx_stderr_fifo" | |
| fi |
This PR introduces native shell integration for Bash and Zsh to automatically capture command-line errors. This enables
cx fixto analyze and resolve the last failed command without requiring the user to manually copy-paste the error message.Key Features
~/.cx/last_error.cx setup shellCommand: A new setup subcommand that automates the installation process by:~/.cx/shell/).~/.bashrcand~/.zshrc.cx fix: Updated the shortcut command to automatically detect and read the last captured error if no arguments are provided.exec 2> >(tee ...)) and shell hooks (PROMPT_COMMAND/precmd) to ensure no noticeable latency in terminal usage.Technical Details
.rcupdates.Acceptance Criteria Met
cx setup shellinstalls integration automaticallycx fixreads captured error without manual inputclose #712
Summary by CodeRabbit
Release Notes