t1071: Fix pulse Phase 3b2 crash on mergedAt:null#1554
t1071: Fix pulse Phase 3b2 crash on mergedAt:null#1554marcusquinn wants to merge 1 commit intomainfrom
Conversation
gh pr view returns "mergedAt":null for unmerged PRs. The grep pipeline on line 591 failed with exit 1 (no match), which propagated through set -o pipefail and triggered set -e, crashing the entire pulse cycle. Added || true to the pipeline so an empty pr_merged_at is returned instead of a fatal error. Downstream code already handles empty values gracefully (used only in log messages and proof-log evidence).
Summary of ChangesHello @marcusquinn, 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 supervisor pulse script by preventing crashes caused by unexpected API responses. Specifically, it ensures that the script can gracefully handle cases where a pull request's 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
|
|
No actionable comments were generated in the recent review. 🎉 WalkthroughA single-line change to Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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 |
![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 fixes a crash caused by grep failing to find a mergedAt value. The addition of || true is a valid minimal fix. My review includes a suggestion to further improve the robustness of this code by using jq for JSON parsing, which would prevent this class of error entirely and simplify the code.
| pr_state=$(echo "$pr_json" | grep -o '"state":"[^"]*"' | cut -d'"' -f4) | ||
| pr_merged_at=$(echo "$pr_json" | grep -o '"mergedAt":"[^"]*"' | cut -d'"' -f4) | ||
| pr_merged_at=$(echo "$pr_json" | grep -o '"mergedAt":"[^"]*"' | cut -d'"' -f4 || true) |
There was a problem hiding this comment.
While adding || true effectively prevents the script from crashing, using grep and cut to parse JSON is inherently fragile. A more robust and maintainable approach is to use a dedicated JSON parser like jq, which is also mentioned as an alternative in the task description for t1071.
Using jq would make the code cleaner and safer, and would also address the same potential fragility in the pr_state extraction on the preceding line.
| pr_state=$(echo "$pr_json" | grep -o '"state":"[^"]*"' | cut -d'"' -f4) | |
| pr_merged_at=$(echo "$pr_json" | grep -o '"mergedAt":"[^"]*"' | cut -d'"' -f4) | |
| pr_merged_at=$(echo "$pr_json" | grep -o '"mergedAt":"[^"]*"' | cut -d'"' -f4 || true) | |
| pr_state=$(echo "$pr_json" | jq -r '.state') | |
| pr_merged_at=$(echo "$pr_json" | jq -r '.mergedAt // empty') |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Superseded by PR #1555 which was merged. Closing stale duplicate. |
1 participant
Summary
gh pr viewreturns"mergedAt":nullfor unmerged PRsgreppipeline extractingmergedAtfailed with exit 1 (no match), which propagated throughset -o pipefailand triggeredset -e, crashing the entire pulse cycle|| trueto the pipeline so an emptypr_merged_atis returned gracefully — downstream code already handles empty values (used only in log messages and proof-log evidence)Root Cause
pulse.sh:591usedgrep -o '"mergedAt":"[^"]*"'to extract the merge timestamp. When GitHub returns"mergedAt":null(unquoted null, not a string), the regex has no match, grep exits 1, pipefail propagates it, andset -eaborts the function.Verification
Tested all three scenarios under
set -eo pipefail:mergedAt:null(OPEN/CLOSED PR) — returns empty string, no crashmergedAt:"2025-..."(MERGED PR) — returns timestamp correctlyRef #1552
Summary by CodeRabbit