fix(metacognitive-guard): handle grep exit code 1 in pipelines #49
Conversation
With `set -o pipefail`, grep returning exit code 1 (no matches) caused the entire script to fail. Added `2>/dev/null` and `|| echo "0"` fallback to all grep commands in pipelines. Fixes: Stop hook showing error message on every response 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughUpdated plugin version from 0.2.3 to 0.2.4 and enhanced null-safety in the struggle-detector script by redirecting grep errors to /dev/null and adding default fallbacks (|| echo "0") for count computations across multiple signal detectors. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (2)
Comment |
Summary of ChangesHello @ANcpLua, 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 issue in the Highlights
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
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review (Opus 4.5)
Verdict: ✅ APPROVED (Post-merge review)
Summary
This PR fixes a common bash scripting issue where set -o pipefail causes grep to return exit code 1 when no matches are found, which was causing the struggle-detector hook to fail on every Stop event.
Findings
⚠️ Medium Priority - CHANGELOG Not Updated
The CHANGELOG.md was not updated for this fix. Per repository conventions (CLAUDE.md section 4.7), every non-trivial change requires a CHANGELOG entry under [Unreleased].
Missing entry should be:
### Fixed
- **metacognitive-guard struggle-detector exit code 1 (YYYY-MM-DD):**
- Fixed `set -o pipefail` + `grep` returning exit code 1 when no matches found
- Added `2>/dev/null` and `|| echo "0"` fallback to all grep pipeline patterns
- Version bump to 0.2.4⚠️ Medium Priority - Version Mismatch
.claude-plugin/marketplace.json still shows "version": "0.2.3" while plugins/metacognitive-guard/.claude-plugin/plugin.json was correctly bumped to 0.2.4.
Strengths
- Correct fix pattern: The
2>/dev/null || echo "0"pattern is the idiomatic way to handle grep no-match in pipefail mode - Consistent application: All 4 grep pipeline patterns were fixed uniformly
- Version bump included: Plugin version was correctly incremented
- Good test plan: PR description shows thorough testing criteria
Technical Analysis
The fix correctly addresses that grep returns:
- Exit 0: matches found
- Exit 1: no matches found
- Exit 2: error
With set -o pipefail, any non-zero exit in a pipeline causes the whole pipeline to fail. The fix:
- Redirects stderr to /dev/null (for any grep errors)
- Falls back to "0" if grep returns non-zero (no matches)
Follow-up Actions Needed
- Update
.claude-plugin/marketplace.jsonversion to0.2.4 - Add CHANGELOG entry under
[Unreleased]### Fixedsection
Autonomous review by Claude Opus 4.5 (Type A repo)
There was a problem hiding this comment.
Claude Code Review (Opus 4.5)
Verdict: ✅ APPROVED (Post-merge review)
Summary
This PR fixes a common bash scripting issue where set -o pipefail causes grep to return exit code 1 when no matches are found, which was causing the struggle-detector hook to fail on every Stop event.
Findings
⚠️ Medium Priority - CHANGELOG Not Updated
The CHANGELOG.md was not updated for this fix. Per repository conventions (CLAUDE.md section 4.7), every non-trivial change requires a CHANGELOG entry under [Unreleased].
Missing entry should be:
### Fixed
- **metacognitive-guard struggle-detector exit code 1 (YYYY-MM-DD):**
- Fixed `set -o pipefail` + `grep` returning exit code 1 when no matches found
- Added `2>/dev/null` and `|| echo "0"` fallback to all grep pipeline patterns
- Version bump to 0.2.4⚠️ Medium Priority - Version Mismatch
.claude-plugin/marketplace.json still shows "version": "0.2.3" while plugins/metacognitive-guard/.claude-plugin/plugin.json was correctly bumped to 0.2.4.
Strengths
- Correct fix pattern: The
2>/dev/null || echo "0"pattern is the idiomatic way to handle grep no-match in pipefail mode - Consistent application: All 4 grep pipeline patterns were fixed uniformly
- Version bump included: Plugin version was correctly incremented
- Good test plan: PR description shows thorough testing criteria
Technical Analysis
The fix correctly addresses that grep returns:
- Exit 0: matches found
- Exit 1: no matches found
- Exit 2: error
With set -o pipefail, any non-zero exit in a pipeline causes the whole pipeline to fail. The fix:
- Redirects stderr to /dev/null (for any grep errors)
- Falls back to "0" if grep returns non-zero (no matches)
Follow-up Actions Needed
- Update
.claude-plugin/marketplace.jsonversion to0.2.4 - Add CHANGELOG entry under
[Unreleased]### Fixedsection
Autonomous review by Claude Opus 4.5 (Type A repo)
![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 effectively fixes a bug where the struggle-detector.sh script would exit with an error when grep found no matches, due to set -o pipefail. The solution of appending || echo "0" to the failing pipelines correctly handles the grep exit code and ensures the script continues execution, defaulting to a count of 0. The version bump in plugin.json is also appropriate. I have one minor suggestion to improve the clarity and robustness of one of the grep commands.
|
|
||
| # 2. EXCESSIVE QUESTIONS (avoiding action) - count each question mark | ||
| QUESTION_COUNT=$(echo "$RESPONSE" | grep -o '?' | wc -l | tr -d ' ') | ||
| QUESTION_COUNT=$(echo "$RESPONSE" | grep -o '?' 2>/dev/null | wc -l | tr -d ' ' || echo "0") |
There was a problem hiding this comment.
Since you are searching for a literal character ?, it's safer, more explicit, and slightly more performant to use grep -F which treats the pattern as a fixed string. While grep -o '?' works here because it's in a basic regular expression context, ? is a special character in other regex flavors, and using -F clearly states the intent to match a literal string, avoiding any potential for misinterpretation.
| QUESTION_COUNT=$(echo "$RESPONSE" | grep -o '?' 2>/dev/null | wc -l | tr -d ' ' || echo "0") | |
| QUESTION_COUNT=$(echo "$RESPONSE" | grep -Fo '?' 2>/dev/null | wc -l | tr -d ' ' || echo "0") |
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix a bug where struggle-detector.sh exits with code 1 when grep finds no matches in pipelines, caused by the combination of set -o pipefail and grep's exit code 1 behavior. The fix adds 2>/dev/null error suppression and || echo "0" fallback to four grep patterns, and bumps the plugin version from 0.2.3 to 0.2.4.
Critical Issue Found: The fix does not correctly address the root cause. The || echo "0" fallback is placed at the end of the entire pipeline, but with set -o pipefail, the pipeline fails as soon as grep exits with code 1, before the fallback can execute.
Key Changes
- Modified 4 grep patterns in
struggle-detector.shto add2>/dev/nulland|| echo "0" - Bumped plugin version to 0.2.4
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| plugins/metacognitive-guard/hooks/scripts/struggle-detector.sh | Added error suppression and fallback patterns to 4 grep commands (lines 46-47, 54, 80-81, 89-90), but the fallback placement is incorrect |
| plugins/metacognitive-guard/.claude-plugin/plugin.json | Version bump from 0.2.3 to 0.2.4 for the bug fix |
| "I think|I believe|probably|might be|could be|I'm not sure|not certain|unclear|I assume|possibly|perhaps" 2>/dev/null \ | ||
| | wc -l | tr -d ' ' || echo "0") |
There was a problem hiding this comment.
The fallback pattern || echo "0" is placed after tr -d ' ', but the pipe chain already includes wc -l which should always succeed and return a numeric value (even "0" when grep finds nothing). The issue is that grep exits with code 1 when no matches are found, causing the pipeline to fail due to set -o pipefail.
The correct fix should place the fallback immediately after the grep command, before the wc -l:
grep -oiE "..." 2>/dev/null || echo "" | wc -l | tr -d ' '
Or wrap just the grep in a subshell with the fallback:
(grep -oiE "..." 2>/dev/null || true) | wc -l | tr -d ' '
The current placement of || echo "0" after the entire pipeline means it will only execute if the final command (tr) fails, which is not the intended behavior.
|
|
||
| # 2. EXCESSIVE QUESTIONS (avoiding action) - count each question mark | ||
| QUESTION_COUNT=$(echo "$RESPONSE" | grep -o '?' | wc -l | tr -d ' ') | ||
| QUESTION_COUNT=$(echo "$RESPONSE" | grep -o '?' 2>/dev/null | wc -l | tr -d ' ' || echo "0") |
There was a problem hiding this comment.
Same issue as lines 46-47: The fallback pattern || echo "0" is incorrectly placed after the entire pipeline. With set -o pipefail, when grep exits with code 1 (no matches), the entire pipeline fails before reaching || echo "0".
The fallback should be placed immediately after grep, or grep should be wrapped with || true:
grep -o '?' 2>/dev/null || echo "" | wc -l | tr -d ' '
Or:
(grep -o '?' 2>/dev/null || true) | wc -l | tr -d ' '
| "sorry|apologize|my mistake|I was wrong|let me try again|I missed" 2>/dev/null \ | ||
| | wc -l | tr -d ' ' || echo "0") |
There was a problem hiding this comment.
Same issue as previous grep patterns: The fallback || echo "0" is placed after the entire pipeline and won't execute when grep exits with code 1 due to set -o pipefail.
The fallback should be placed immediately after grep:
grep -oiE "..." 2>/dev/null || echo "" | wc -l | tr -d ' '
Or:
(grep -oiE "..." 2>/dev/null || true) | wc -l | tr -d ' '
| "generally|typically|usually|in most cases|it depends|that said|to be fair" 2>/dev/null \ | ||
| | wc -l | tr -d ' ' || echo "0") |
There was a problem hiding this comment.
Same issue as previous grep patterns: The fallback || echo "0" is placed after the entire pipeline and won't execute when grep exits with code 1 due to set -o pipefail.
The fallback should be placed immediately after grep:
grep -oiE "..." 2>/dev/null || echo "" | wc -l | tr -d ' '
Or:
(grep -oiE "..." 2>/dev/null || true) | wc -l | tr -d ' '
Summary
struggle-detector.shhook failing with exit code 1 on every Stop eventset -o pipefail+grepreturning exit code 1 when no matches found2>/dev/nulland|| echo "0"fallback to all grep commands in pipelinesChanges
plugins/metacognitive-guard/hooks/scripts/struggle-detector.sh: Fixed 4 grep pipeline patternsplugins/metacognitive-guard/.claude-plugin/plugin.json: Bumped version 0.2.3 → 0.2.4Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.