Apply code review fixes for modular bash profile#6
Conversation
|
Important Review skippedIgnore keyword(s) in the title. ⛔ Ignored keywords (2)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Comment |
Co-authored-by: cbwinslow <8528478+cbwinslow@users.noreply.github.com>
🧪 E2E Test Resultsℹ️ No test results available Generated by Debugg AI 🤖 |
Co-authored-by: cbwinslow <8528478+cbwinslow@users.noreply.github.com>
9763e8e
into
codex/develop-advanced-bashrc-profile-system
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||
There was a problem hiding this comment.
Pull request overview
This PR addresses code review feedback from PR #2, implementing shell-level fixes for proper argument handling and duplicate prevention, plus Python agent improvements for error handling and multi-word prompt support.
Key changes:
- Shell improvements: Fixed bootstrap idempotency, added PROMPT_COMMAND preservation with throttling, and corrected argument forwarding in bash wrappers
- Python agent enhancements: Added git error handling, improved error messages, and enabled multi-word prompts via argparse
- Housekeeping: Added Python cache patterns to .gitignore
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| bootstrap.sh | Updates grep pattern to match exact bootstrap comment, preventing duplicate .bashrc entries on re-runs |
| bashrc | Adds throttling to self-heal function and preserves existing PROMPT_COMMAND with duplicate detection |
| bash_functions.d/ai.sh | Replaces "$*" with "$@" in all AI wrapper functions for proper space-handling in arguments |
| ai/agent.py | Adds error handling for git operations, improves API key error message, uses elif for modes, and enables multi-word prompts with nargs='+' |
| .gitignore | Adds standard Python cache file patterns (pycache, *.py[cod], *$py.class) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| add_result = subprocess.run( | ||
| ["git", "-C", str(repo_path), "add", relative_file], | ||
| capture_output=True, text=True | ||
| ) | ||
| if add_result.returncode != 0: | ||
| error_msg = f"Failed to add file to git: {add_result.stderr.strip()}" | ||
| append_memory("short_term", f"error:{error_msg}") | ||
| return error_msg |
There was a problem hiding this comment.
The error handling approach here is inconsistent with the git clone/pull operations above (lines 117-127). Those operations use check=True and re-raise exceptions, while this checks returncode manually and returns an error message string instead of raising an exception.
This inconsistency could lead to confusing behavior - some git failures will raise exceptions while others will return error strings. Consider using a consistent error handling strategy throughout the function. Either:
- Use
check=Truefor all git operations and handle exceptions at a higher level, or - Use
capture_output=Truewith manual returncode checks for all git operations and return error messages
The same issue applies to the commit operation below (lines 152-159).
| system_prompt = "Assist with debugging shell sessions; prefer minimal, testable steps." | ||
| if mode == "tldr": | ||
| elif mode == "tldr": | ||
| system_prompt = "Summarize command usage with short examples." |
There was a problem hiding this comment.
The chat function accepts four modes ("chat", "debug", "tldr", "code") according to the argument parser (line 170), but only handles three of them. The "code" mode is missing its conditional branch, so it will use the default system prompt instead of a code-specific one.
Add an elif mode == "code": branch with an appropriate system prompt, for example:
elif mode == "code":
system_prompt = "Assist with code generation and shell scripting; provide working examples."| system_prompt = "Summarize command usage with short examples." | |
| system_prompt = "Summarize command usage with short examples." | |
| elif mode == "code": | |
| system_prompt = "Assist with code generation and shell scripting; provide working examples." |
| try: | ||
| subprocess.run(["git", "-C", str(target), "pull", "--rebase"], check=True) | ||
| except subprocess.CalledProcessError as e: | ||
| print(f"Error: git pull failed for {repo_slug} at {target}: {e}", file=sys.stderr) | ||
| raise |
There was a problem hiding this comment.
The error handling here prints the exception details to stderr before re-raising, but then immediately re-raises the CalledProcessError which already contains the error information. When the exception propagates to the caller, the error message will be shown again by the default exception handling.
Additionally, the error message format f"Error: git pull failed for {repo_slug} at {target}: {e}" is somewhat redundant since CalledProcessError already includes the command that failed. Consider removing the stderr output and letting the exception's natural error message be displayed, or catch the exception higher up in the call chain (in main()) and format it there for user-friendly output.
| try: | ||
| subprocess.run(["git", "clone", f"https://github.com/{repo_slug}.git", str(target)], check=True) | ||
| except subprocess.CalledProcessError as e: | ||
| print(f"Error: git clone failed for {repo_slug} at {target}: {e}", file=sys.stderr) | ||
| raise |
There was a problem hiding this comment.
Same issue as with the git pull error handling above - the error message is printed to stderr before re-raising, which leads to duplicate error output. Consider a more consistent approach to error handling throughout the function.
User description
Addresses all code review feedback from PR #2 review thread.
Shell Fixes
"# bash.d bootstrap"to match actual comment, preventing duplicate appends on re-runsPROMPT_COMMANDand add duplicate check; add 60s throttle tobashd-self-healto avoid per-prompt overhead"$*"with"$@"in AI wrappers for proper argument handling with spacesPython Agent Fixes
eliffor mutually exclusive mode conditionsnargs='+'Housekeeping
.gitignore💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
PR Type
Bug fix, Enhancement
Description
Fix bash argument handling in AI wrappers using "$@" instead of "$*"
Improve bootstrap detection with specific grep pattern to prevent duplicates
Add comprehensive error handling for git operations with stderr output
Enhance multi-word prompt support via argparse nargs configuration
Implement 60-second throttle for self-heal to reduce per-prompt overhead
Preserve existing PROMPT_COMMAND and add duplicate execution checks
Improve API key error message with setup documentation URL
Use elif for mutually exclusive mode conditions in chat function
Diagram Walkthrough
File Walkthrough
ai.sh
Fix bash argument expansion in AI wrappersbash_functions.d/ai.sh
code)
bootstrap.sh
Fix bootstrap detection patternbootstrap.sh
agent.py
Enhance error handling and multi-word prompt supportai/agent.py
URL
logging
message logging
bashrc
Add self-heal throttle and PROMPT_COMMAND preservationbashrc
variables
overhead
PROMPT_COMMAND