t1350: fix: auto-detect non-interactive terminals in setup.sh#2468
t1350: fix: auto-detect non-interactive terminals in setup.sh#2468alex-solovyev merged 2 commits intomainfrom
Conversation
- Add tty detection after parse_args: if stdin is not a tty and --interactive was not explicitly passed, set NON_INTERACTIVE=true - Fix grep -c ERR trap noise in shell-env.sh: move || true inside subshell as || echo "0" to prevent inherit_errexit from firing the ERR trap on no-match exit code Fixes: setup.sh exits early in agent/CI shells where read fails on non-tty stdin with set -Eeuo pipefail
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAuto-detect non-interactive terminals in setup.sh (sets NON_INTERACTIVE when stdin is not a TTY post-arg-parse) and harden grep -c calls in setup-modules/shell-env.sh to avoid ERR-trap noise by replacing trailing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
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 enhances the robustness of 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
|
🔍 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: Fri Feb 27 14:31:28 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
![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 two main improvements. First, it enhances setup.sh to automatically detect non-interactive terminals (like in CI/CD environments) and set NON_INTERACTIVE=true, preventing the script from hanging on read prompts. This is a solid improvement for automation. Second, it refactors grep -c calls in setup-modules/shell-env.sh to handle 'no match' exit codes more robustly within subshells, which is a good fix to avoid issues with ERR traps. My review includes a suggestion to further improve the grep calls by removing stderr suppression, which would make potential file permission errors visible for easier debugging.
setup-modules/shell-env.sh
Outdated
| n=$(grep -cE '^\s*export\s+[A-Z]' "$src_file" 2>/dev/null || echo "0") | ||
| total_exports=$((total_exports + ${n:-0})) | ||
| n=$(grep -cE '^\s*alias\s+' "$src_file" 2>/dev/null) || true | ||
| n=$(grep -cE '^\s*alias\s+' "$src_file" 2>/dev/null || echo "0") | ||
| total_aliases=$((total_aliases + ${n:-0})) | ||
| n=$(grep -cE 'PATH.*=' "$src_file" 2>/dev/null) || true | ||
| n=$(grep -cE 'PATH.*=' "$src_file" 2>/dev/null || echo "0") |
There was a problem hiding this comment.
While the change to handle grep -c's exit code inside the subshell is a good improvement, the use of 2>/dev/null is problematic. The script already verifies that these shell configuration files exist before this loop. Suppressing stderr here can hide important errors like permission issues, causing the script to silently proceed with a count of 0. It's better to let such errors be visible for debugging.
This follows the general rule: 'Avoid using 2>/dev/null to suppress errors on file operations if the file's existence has already been verified by a preceding check... This practice is redundant for 'file not found' errors and can mask other important issues like permissions problems.'
| n=$(grep -cE '^\s*export\s+[A-Z]' "$src_file" 2>/dev/null || echo "0") | |
| total_exports=$((total_exports + ${n:-0})) | |
| n=$(grep -cE '^\s*alias\s+' "$src_file" 2>/dev/null) || true | |
| n=$(grep -cE '^\s*alias\s+' "$src_file" 2>/dev/null || echo "0") | |
| total_aliases=$((total_aliases + ${n:-0})) | |
| n=$(grep -cE 'PATH.*=' "$src_file" 2>/dev/null) || true | |
| n=$(grep -cE 'PATH.*=' "$src_file" 2>/dev/null || echo "0") | |
| n=$(grep -cE '^\s*export\s+[A-Z]' "$src_file" || echo "0") | |
| total_exports=$((total_exports + ${n:-0})) | |
| n=$(grep -cE '^\s*alias\s+' "$src_file" || echo "0") | |
| total_aliases=$((total_aliases + ${n:-0})) | |
| n=$(grep -cE 'PATH.*=' "$src_file" || echo "0") |
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@setup-modules/shell-env.sh`:
- Around line 205-212: The arithmetic failures are caused by the fallback `||
echo "0"` producing multi-line output for the `n` variable; update the three
grep assignments that set `n` (the ones using `grep -cE '^\s*export\s+[A-Z]'`,
`grep -cE '^\s*alias\s+'`, and `grep -cE 'PATH.*='`) to use a no-op fallback
(`|| :`) instead of `|| echo "0"`, so `n` will only contain grep's single
numeric output and the existing `${n:-0}` arithmetic fallback remains effective.
In `@todo/tasks/t1350-brief.md`:
- Around line 25-31: The fenced code block containing the if-statement (if [[
"$INTERACTIVE_MODE" != "true" && ! -t 0 ]]; then ... fi) inside the numbered
list needs an empty blank line before the opening ```bash and an empty blank
line after the closing ``` so the list item is properly separated (fixing
markdownlint MD031); update the list item in todo/tasks/t1350-brief.md so there
is a blank line above the fenced block and a blank line after the closing fence
before the next list item.
todo/tasks/t1350-brief.md
Outdated
| 1. **setup.sh**: After `parse_args`, add: | ||
| ```bash | ||
| if [[ "$INTERACTIVE_MODE" != "true" && ! -t 0 ]]; then | ||
| NON_INTERACTIVE=true | ||
| fi | ||
| ``` | ||
| 2. **setup-modules/shell-env.sh**: Change `$(grep -cE ... 2>/dev/null) || true` to `$(grep -cE ... 2>/dev/null || echo "0")` to handle no-match inside the subshell, avoiding ERR trap noise with `inherit_errexit`. |
There was a problem hiding this comment.
Fix markdownlint MD031 around the fenced code block.
The fenced block in this list item needs surrounding blank lines to keep docs tooling at A-grade quality.
📝 Suggested markdown fix
1. **setup.sh**: After `parse_args`, add:
+
```bash
if [[ "$INTERACTIVE_MODE" != "true" && ! -t 0 ]]; then
NON_INTERACTIVE=true
fi
```
+
2. **setup-modules/shell-env.sh**: Change `$(grep -cE ... 2>/dev/null) || true` to `$(grep -cE ... 2>/dev/null || echo "0")` to handle no-match inside the subshell, avoiding ERR trap noise with `inherit_errexit`.📝 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.
| 1. **setup.sh**: After `parse_args`, add: | |
| ```bash | |
| if [[ "$INTERACTIVE_MODE" != "true" && ! -t 0 ]]; then | |
| NON_INTERACTIVE=true | |
| fi | |
| ``` | |
| 2. **setup-modules/shell-env.sh**: Change `$(grep -cE ... 2>/dev/null) || true` to `$(grep -cE ... 2>/dev/null || echo "0")` to handle no-match inside the subshell, avoiding ERR trap noise with `inherit_errexit`. | |
| 1. **setup.sh**: After `parse_args`, add: |
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[notice] 26-26: todo/tasks/t1350-brief.md#L26
Fenced code blocks should be surrounded by blank lines
[notice] 30-30: todo/tasks/t1350-brief.md#L30
Fenced code blocks should be surrounded by blank lines
🪛 markdownlint-cli2 (0.21.0)
[warning] 26-26: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 30-30: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@todo/tasks/t1350-brief.md` around lines 25 - 31, The fenced code block
containing the if-statement (if [[ "$INTERACTIVE_MODE" != "true" && ! -t 0 ]];
then ... fi) inside the numbered list needs an empty blank line before the
opening ```bash and an empty blank line after the closing ``` so the list item
is properly separated (fixing markdownlint MD031); update the list item in
todo/tasks/t1350-brief.md so there is a blank line above the fenced block and a
blank line after the closing fence before the next list item.
🔍 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: Fri Feb 27 14:40:03 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
The markdownlint MD031 finding (fenced code blocks without surrounding blank lines) was addressed in the original PR #2468 before merge. The quality-feedback scanner flagged it as unactioned because it tracks review comments by existence, not by fix status. Markdownlint confirms 0 errors. Added a Resolution section to the brief documenting this finding. Closes #3321
#4597) The markdownlint MD031 finding (fenced code blocks without surrounding blank lines) was addressed in the original PR #2468 before merge. The quality-feedback scanner flagged it as unactioned because it tracks review comments by existence, not by fix status. Markdownlint confirms 0 errors. Added a Resolution section to the brief documenting this finding. Closes #3321
1 participant
Summary
setup.sh— if stdin is not a tty and--interactivewas not explicitly passed, setNON_INTERACTIVE=trueautomaticallygrep -cERR trap noise insetup-modules/shell-env.shby moving|| trueinside the subshell as|| echo "0"Why
When agents or CI/CD pipelines run
setup.shwithout--non-interactive, thereadcommands fail on non-tty stdin withset -Eeuo pipefail, killing the function before agent deployment runs. This meansaidevops updatein headless environments silently fails to deploy agents.Changes
setup.sh: Added tty detection afterparse_args—[[ ! -t 0 ]]setsNON_INTERACTIVE=trueunless--interactivewas explicitly passedsetup-modules/shell-env.sh: Changed$(grep -cE ... 2>/dev/null) || trueto$(grep -cE ... 2>/dev/null || echo "0")to handle no-match exit code inside the subshell, preventing ERR trap noise withinherit_errexittodo/tasks/t1350-brief.md: Task briefCloses #2464
Summary by CodeRabbit
New Features
Bug Fixes
Documentation