fix: replace dirname with parameter expansion for stripped PATH compatibility#2608
fix: replace dirname with parameter expansion for stripped PATH compatibility#2608alex-solovyev wants to merge 1 commit intomainfrom
Conversation
…tibility
dirname is an external command not available in stripped PATH environments
(e.g., pulse sessions via Claude Code MCP). Replace with ${BASH_SOURCE[0]%/*}
which is a bash builtin parameter expansion requiring no external commands.
Fixes: issue-sync-helper.sh, issue-sync-lib.sh, session-miner-pulse.sh
Closes #2605
Summary of ChangesHello, 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 addresses a critical compatibility issue in shell scripts that prevented proper execution in environments with stripped PATH variables. By replacing the external 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
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
WalkthroughThree shell scripts replace Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
✨ 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 |
|
Closing — the issue-sync-helper.sh and issue-sync-lib.sh fixes were already delivered via #2603 (merged into main) using a different approach. This PR has merge conflicts as a result. The session-miner-pulse.sh fix is still needed and will be addressed in a separate targeted PR. |
|
Superseded by #2603 for the issue-sync scripts. Merge conflict prevents merging. Will create a targeted fix for session-miner-pulse.sh separately. |
![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 aims to improve script portability by replacing the external dirname command with bash parameter expansion. This is a good improvement for compatibility with environments that have a stripped PATH. However, the current implementation has a flaw where it can fail if scripts are invoked without a path prefix. I've provided critical feedback with suggestions to make the implementation more robust and handle all invocation scenarios correctly.
| set -euo pipefail | ||
|
|
||
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" || exit | ||
| SCRIPT_DIR="$(cd "${BASH_SOURCE[0]%/*}" && pwd)" || exit |
There was a problem hiding this comment.
The parameter expansion ${BASH_SOURCE[0]%/*} can be problematic. If the script is invoked from the PATH without a directory prefix (e.g., myscript.sh instead of ./myscript.sh), this expansion results in the script's filename itself. The subsequent cd command will then fail because it's a file, not a directory.
A more robust approach is to check if the expansion modified the string and fall back to . if it didn't, which correctly handles all invocation scenarios.
| SCRIPT_DIR="$(cd "${BASH_SOURCE[0]%/*}" && pwd)" || exit | |
| dir="${BASH_SOURCE[0]%/*}" | |
| if [[ "$dir" == "${BASH_SOURCE[0]}" ]]; then | |
| dir="." | |
| fi | |
| SCRIPT_DIR="$(cd "$dir" && pwd)" || exit |
| # Resolves SCRIPT_DIR from BASH_SOURCE so it works when sourced from any location. | ||
| if [[ -z "${SCRIPT_DIR:-}" ]]; then | ||
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| SCRIPT_DIR="$(cd "${BASH_SOURCE[0]%/*}" && pwd)" |
There was a problem hiding this comment.
The parameter expansion ${BASH_SOURCE[0]%/*} can be problematic. If the script is invoked from the PATH without a directory prefix (e.g., myscript.sh instead of ./myscript.sh), this expansion results in the script's filename itself. The subsequent cd command will then fail because it's a file, not a directory.
A more robust approach is to check if the expansion modified the string and fall back to . if it didn't, which correctly handles all invocation scenarios.
| SCRIPT_DIR="$(cd "${BASH_SOURCE[0]%/*}" && pwd)" | |
| dir="${BASH_SOURCE[0]%/*}" | |
| if [[ "$dir" == "${BASH_SOURCE[0]}" ]]; then | |
| dir="." | |
| fi | |
| SCRIPT_DIR="$(cd "$dir" && pwd)" |
| # --- Configuration --- | ||
|
|
||
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| SCRIPT_DIR="$(cd "${BASH_SOURCE[0]%/*}" && pwd)" |
There was a problem hiding this comment.
The parameter expansion ${BASH_SOURCE[0]%/*} can be problematic. If the script is invoked from the PATH without a directory prefix (e.g., myscript.sh instead of ./myscript.sh), this expansion results in the script's filename itself. The subsequent cd command will then fail because it's a file, not a directory.
A more robust approach is to check if the expansion modified the string and fall back to . if it didn't, which correctly handles all invocation scenarios.
| SCRIPT_DIR="$(cd "${BASH_SOURCE[0]%/*}" && pwd)" | |
| dir="${BASH_SOURCE[0]%/*}" | |
| if [[ "$dir" == "${BASH_SOURCE[0]}" ]]; then | |
| dir="." | |
| fi | |
| SCRIPT_DIR="$(cd "$dir" && pwd)" |
1 participant
Summary
dirname(external command) with${BASH_SOURCE[0]%/*}(bash parameter expansion) in 3 scriptsdirnameis not available in stripped PATH environments (e.g., pulse sessions via Claude Code MCP)Files Changed
.agents/scripts/issue-sync-helper.shline 21.agents/scripts/issue-sync-lib.shline 31.agents/scripts/session-miner-pulse.shline 22Verification
Closes #2605
Summary by CodeRabbit