fix: write SHELLCHECK_PATH to .zshenv for non-interactive shell coverage#2937
fix: write SHELLCHECK_PATH to .zshenv for non-interactive shell coverage#2937marcusquinn merged 1 commit intomainfrom
Conversation
The shellcheck-wrapper.sh (GH#2915) was only configured in .zshrc and .bash_profile, which are only sourced for interactive shells. OpenCode spawns bash-language-server as a non-interactive child process, so SHELLCHECK_PATH was never set — the wrapper was bypassed entirely. This caused the same runaway shellcheck memory explosion (17.8 GB across 73 processes) that GH#2915 was supposed to fix. Fix: add .zshenv as Layer 2 in setup_shellcheck_wrapper(). .zshenv is sourced by ALL zsh processes (interactive and non-interactive), ensuring the wrapper is visible to LSP child processes.
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 memory consumption problem with Highlights
Changelog
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
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 correctly identifies and fixes an issue where SHELLCHECK_PATH was not being set for non-interactive zsh shells by adding the configuration to .zshenv. The logic is sound. The suggestion to improve the robustness of the file check by avoiding error suppression has been retained, as it aligns with the project's guidelines for clearer debugging.
| # shells, so SHELLCHECK_PATH set only in .zshrc is invisible to the LSP. | ||
| local zshenv="$HOME/.zshenv" | ||
| if [[ -f "$zshenv" ]] || command -v zsh >/dev/null 2>&1; then | ||
| if grep -q 'SHELLCHECK_PATH' "$zshenv" 2>/dev/null; then |
There was a problem hiding this comment.
While this logic is clever, using 2>/dev/null with grep can mask underlying issues like file permission errors, causing the script to fail on a later line with a less informative error message. A more robust approach is to explicitly check for the file's existence before calling grep. This avoids suppressing potentially important errors and aligns with the project's general rules against blanket error suppression.
| if grep -q 'SHELLCHECK_PATH' "$zshenv" 2>/dev/null; then | |
| if [[ -f "$zshenv" ]] && grep -q 'SHELLCHECK_PATH' "$zshenv"; then |
References
- Avoid using
2>/dev/nullto suppress errors on file operations if the file's existence has already been verified by a preceding check (e.g.,[[ -f "$file" ]]or an early return). This practice is redundant for 'file not found' errors and can mask other important issues like permissions problems. - Avoid using '2>/dev/null' for blanket suppression of command errors in shell scripts to ensure that authentication, syntax, or system issues remain visible for debugging.
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Thu Mar 5 17:44:46 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
|
Follow-up: root cause removed in PR #2939 This PR fixed the .zshenv path for non-interactive shells. PR #2939 goes further — removes Upstream hazard note: The bash-language-server still hardcodes |
|
fatal: couldn't find remote ref bugfix/shellcheck-zshenv-fix |
…nv.sh Address PR #2937 review feedback (gemini): grep with 2>/dev/null masks permission errors and other non-file-not-found failures. - Line 511: add [[ -f "$zshenv" ]] guard before grep (file may not exist when entering via the 'command -v zsh' branch) - Line 533: remove redundant 2>/dev/null (file existence already guaranteed by the check at lines 527-530) Closes #2955
…nv.sh Address quality-debt from PR #2937 review feedback (GH#2955). Replace all remaining grep 2>/dev/null patterns with either: - Removal (when file existence is already guaranteed by prior logic) - Explicit [[ -f ]] guards (when the file may not exist) This prevents masking permission errors and other non-file-not-found failures while maintaining correct behaviour for missing files.
…nv.sh (#3003) Address quality-debt from PR #2937 review feedback (GH#2955). Replace all remaining grep 2>/dev/null patterns with either: - Removal (when file existence is already guaranteed by prior logic) - Explicit [[ -f ]] guards (when the file may not exist) This prevents masking permission errors and other non-file-not-found failures while maintaining correct behaviour for missing files.
1 participant
Summary
SHELLCHECK_PATHto~/.zshenvin addition to.zshrc.zshenvis sourced by ALL zsh processes (interactive and non-interactive), ensuring the shellcheck-wrapper is visible to OpenCode's bash-language-server child processesProblem
PR #2918 (GH#2915) added
shellcheck-wrapper.shand configuredSHELLCHECK_PATHin.zshrcand.bash_profile. But.zshrcis only sourced for interactive shells. OpenCode spawnsbash-language-serveras a non-interactive child process, soSHELLCHECK_PATHwas never set — the wrapper was completely bypassed.The bash-language-server hardcodes
--external-sourcesin every shellcheck invocation. Without the wrapper stripping this flag, shellcheck follows allsourcedirectives recursively across 460+ scripts, causing exponential memory growth per invocation.Fix
Add
.zshenvas Layer 2 insetup_shellcheck_wrapper():.zshenv— ALL zsh processes including non-interactive (persists across reboots) [NEW]Related