t2800: fix pulse idempotency guard for external-contributor label check#2801
t2800: fix pulse idempotency guard for external-contributor label check#2801alex-solovyev merged 1 commit intomainfrom
Conversation
Root cause: the grep pattern with ^ and $ anchors on echo output was fragile — it could fail with trailing whitespace, carriage returns, or stderr contamination from 2>&1. Since this is agent prompt documentation (not a script), the LLM interpreting these instructions could also misapply the grep pattern. Two fixes applied: 1. Label check: replace 'echo $labels | grep -q ^external-contributor$' with jq any(.labels[]; .name == "external-contributor") which returns a clean boolean true/false — no parsing ambiguity 2. Comment fallback: restructure from elif to nested if/else so the comment check is ALWAYS reached when the label is not found (or API fails), instead of being skipped when grep fails unexpectedly 3. Permission-failure comment check: same pattern — replace grep -qF with jq select+test for consistent, robust matching Also applied the same jq-based pattern to the permission-failure comment idempotency check for consistency. Closes #2800
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 refines the idempotency guards within 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
|
WalkthroughThe pulse script's external contributor label and comment handling was refactored to replace fragile grep-based string matching with robust jq-driven boolean checks and count operations. A fallback mechanism was introduced to detect and conditionally re-apply labels when missing, and permission-failure handling now uses jq-computed counts for idempotent comment posting. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
🔍 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: Wed Mar 4 02:41:14 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 makes a solid improvement by replacing fragile grep commands with more robust jq queries to enhance the idempotency of label and comment checks. This is a great step towards preventing duplicate comments. I've included a couple of suggestions to further strengthen the logic, especially for handling failure scenarios, to make the checks even more resilient. My feedback also includes removing 2>/dev/null in line with project best practices to improve debuggability.
Note: Security Review has been skipped due to the limited scope of the PR.
| if [ "$existing_comment" != "0" ] && [ -n "$existing_comment" ]; then | ||
| # Comment already exists but label is missing — re-add the label only | ||
| gh api "repos/<slug>/issues/<number>/labels" -X POST -f 'labels[]=external-contributor' 2>/dev/null || true | ||
| else | ||
| # Neither label nor comment exists — post comment and add label atomically | ||
| gh pr comment <number> --repo <slug> --body "This PR is from an external contributor (@<author>). Auto-merge is disabled for external PRs — a maintainer must review and merge manually." \ | ||
| && gh api "repos/<slug>/issues/<number>/labels" -X POST -f 'labels[]=external-contributor' 2>/dev/null || true | ||
| fi |
There was a problem hiding this comment.
This logic for checking existing_comment can be made more robust to prevent posting duplicate comments. The current condition [ "$existing_comment" != "0" ] && [ -n "$existing_comment" ] will evaluate to false if $existing_comment is an empty string (e.g., if the gh command fails and produces no output), causing a new comment to be posted and potentially creating a duplicate.
A safer approach is to invert the logic and only post a comment if you are certain there are none. By checking [ "$existing_comment" = "0" ], you ensure that a new comment is posted only when the count is definitively zero. In all other cases (comment exists, or the check failed), the script will attempt the safer, idempotent action of re-adding the label.
Additionally, 2>/dev/null should be avoided to ensure error messages are visible for debugging. The || true is sufficient to prevent script termination on failure.
| if [ "$existing_comment" != "0" ] && [ -n "$existing_comment" ]; then | |
| # Comment already exists but label is missing — re-add the label only | |
| gh api "repos/<slug>/issues/<number>/labels" -X POST -f 'labels[]=external-contributor' 2>/dev/null || true | |
| else | |
| # Neither label nor comment exists — post comment and add label atomically | |
| gh pr comment <number> --repo <slug> --body "This PR is from an external contributor (@<author>). Auto-merge is disabled for external PRs — a maintainer must review and merge manually." \ | |
| && gh api "repos/<slug>/issues/<number>/labels" -X POST -f 'labels[]=external-contributor' 2>/dev/null || true | |
| fi | |
| if [ "$existing_comment" = "0" ]; then | |
| # Neither label nor comment exists — post comment and add label atomically | |
| gh pr comment <number> --repo <slug> --body "This PR is from an external contributor (@<author>). Auto-merge is disabled for external PRs — a maintainer must review and merge manually." \ | |
| && gh api "repos/<slug>/issues/<number>/labels" -X POST -f 'labels[]=external-contributor' || true | |
| else | |
| # Comment may exist (or check failed), but label is missing — re-add the label only | |
| gh api "repos/<slug>/issues/<number>/labels" -X POST -f 'labels[]=external-contributor' || true | |
| fi |
References
- 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.
| if ! echo "$comments" | grep -qF 'Permission check failed'; then | ||
| # Uses jq select+test for robust matching — avoids grep issues with multi-line output. | ||
| perm_comment_count=$(gh pr view <number> --repo <slug> --json comments --jq '[.comments[].body | select(test("Permission check failed"))] | length' 2>&1) || true | ||
| if [ "$perm_comment_count" = "0" ] || [ -z "$perm_comment_count" ]; then |
There was a problem hiding this comment.
For improved robustness and to prevent duplicate comments, this check can be simplified. The || [ -z "$perm_comment_count" ] part of the condition could cause a new comment to be posted if the gh command fails and produces an empty string, even if a comment already exists.
By only checking [ "$perm_comment_count" = "0" ], you ensure a comment is posted only when the count is definitively zero. If the command fails and the variable is empty or contains an error message, the condition will be false, and the script will safely do nothing, preventing duplicate comments. This aligns with the fail-safe principle of idempotency checks.
| if [ "$perm_comment_count" = "0" ] || [ -z "$perm_comment_count" ]; then | |
| if [ "$perm_comment_count" = "0" ]; then |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.agents/scripts/commands/pulse.md (2)
107-110: Mirror marker-based matching in permission-failure comments too.This block has the same broad-text matching risk. Consider using a unique marker token (for example,
[pulse-permission-check-failed]) in both the comment body and jq selector for strict idempotency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/commands/pulse.md around lines 107 - 110, The permission-failure comment isn’t idempotent; update the jq selector that sets perm_comment_count (variable perm_comment_count with jq test("Permission check failed")) to match a unique marker token (e.g. "[pulse-permission-check-failed]") and add that same marker token into the gh pr comment body string emitted by the gh pr comment command so the select(test(...)) and the posted comment use the identical marker for strict, idempotent matching.
88-97: Use a deterministic marker token for comment idempotency matching.Current phrase matching is broad and may match unrelated human comments. Add a hidden/stable marker in the bot comment and key
jq test()to that marker.Proposed diff
- existing_comment=$(gh pr view <number> --repo <slug> --json comments --jq '[.comments[].body | select(test("external contributor"; "i"))] | length' 2>&1) || true + existing_comment=$(gh pr view <number> --repo <slug> --json comments --jq '[.comments[].body | select(test("\\[pulse-external-contributor\\]"))] | length' 2>&1) || true @@ - gh pr comment <number> --repo <slug> --body "This PR is from an external contributor (@<author>). Auto-merge is disabled for external PRs — a maintainer must review and merge manually." \ + gh pr comment <number> --repo <slug> --body "[pulse-external-contributor] This PR is from an external contributor (@<author>). Auto-merge is disabled for external PRs — a maintainer must review and merge manually." \ && gh api "repos/<slug>/issues/<number>/labels" -X POST -f 'labels[]=external-contributor' 2>/dev/null || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/commands/pulse.md around lines 88 - 97, The existing_comment detection uses a broad regex and should instead match a deterministic hidden marker; update the bot comment body sent by gh pr comment (the string passed to gh pr comment <number> --repo <slug> --body ...) to include a stable token (e.g., "[bot-marker:external-contributor:<hash>]" or similar) and change the existing_comment assignment (the jq filter in existing_comment=$(gh pr view ... --jq '...') ) to test only for that token using jq test("bot-marker:external-contributor:<hash>"; "i"); keep the rest of the logic (re-adding label vs posting comment+label) intact so the presence check is idempotent and deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.agents/scripts/commands/pulse.md:
- Around line 107-110: The permission-failure comment isn’t idempotent; update
the jq selector that sets perm_comment_count (variable perm_comment_count with
jq test("Permission check failed")) to match a unique marker token (e.g.
"[pulse-permission-check-failed]") and add that same marker token into the gh pr
comment body string emitted by the gh pr comment command so the
select(test(...)) and the posted comment use the identical marker for strict,
idempotent matching.
- Around line 88-97: The existing_comment detection uses a broad regex and
should instead match a deterministic hidden marker; update the bot comment body
sent by gh pr comment (the string passed to gh pr comment <number> --repo <slug>
--body ...) to include a stable token (e.g.,
"[bot-marker:external-contributor:<hash>]" or similar) and change the
existing_comment assignment (the jq filter in existing_comment=$(gh pr view ...
--jq '...') ) to test only for that token using jq
test("bot-marker:external-contributor:<hash>"; "i"); keep the rest of the logic
(re-adding label vs posting comment+label) intact so the presence check is
idempotent and deterministic.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b438d090-1ce6-43c6-bbc6-bb722e741e68
📒 Files selected for processing (1)
.agents/scripts/commands/pulse.md
… function (t1391) The LLM supervisor kept re-implementing the inline bash idempotency guard incorrectly on each pulse cycle, causing 15+ duplicate 'external contributor' comments on PR #2792 despite 4 prior fix attempts (PRs #2794, #2796, #2801, #2803) — all in pulse.md prompt text. Root cause: the check is deterministic (one correct answer regardless of context) but was encoded as prompt guidance that the LLM had to re-implement each cycle. Per the 'Intelligence Over Determinism' principle, deterministic logic belongs in the harness, not the prompt. Changes: - Add check_external_contributor_pr() to pulse-wrapper.sh: checks BOTH label AND comment, captures exit codes separately, fails closed on any API error, only posts when confirmed safe - Add check_permission_failure_pr() companion function for the permission API failure case - Add source guard (BASH_SOURCE check) so pulse-wrapper.sh can be sourced for its functions without triggering the full lifecycle - Update pulse.md to call the helper functions instead of inline bash - Update Hard Rule 12 to reference the helper functions Closes #2809
… function (t1391) (#2810) The LLM supervisor kept re-implementing the inline bash idempotency guard incorrectly on each pulse cycle, causing 15+ duplicate 'external contributor' comments on PR #2792 despite 4 prior fix attempts (PRs #2794, #2796, #2801, #2803) — all in pulse.md prompt text. Root cause: the check is deterministic (one correct answer regardless of context) but was encoded as prompt guidance that the LLM had to re-implement each cycle. Per the 'Intelligence Over Determinism' principle, deterministic logic belongs in the harness, not the prompt. Changes: - Add check_external_contributor_pr() to pulse-wrapper.sh: checks BOTH label AND comment, captures exit codes separately, fails closed on any API error, only posts when confirmed safe - Add check_permission_failure_pr() companion function for the permission API failure case - Add source guard (BASH_SOURCE check) so pulse-wrapper.sh can be sourced for its functions without triggering the full lifecycle - Update pulse.md to call the helper functions instead of inline bash - Update Hard Rule 12 to reference the helper functions Closes #2809
1 participant
Summary
echo "$labels" | grep -q '^external-contributor$'withjq any(.labels[]; .name == "external-contributor")which returns a clean booleantrue/false— no grep anchor ambiguity with multi-line output, trailing whitespace, or stderr contaminationif/elif/elsetoif/else { if/else }so the comment-existence fallback is always reached when the label is not found (or API fails), instead of being skipped when grep fails unexpectedlyRoot Cause
PR #2796 fixed the
2>/dev/nullsilent-failure issue but kept thegrep -q '^external-contributor$'pattern. This pattern is fragile because:^and$anchors can fail with trailing whitespace/CR in the output2>&1mixes stderr into the label output, potentially breaking line boundariesThe
elifstructure also meant that if the grep check failed (even when the label was present), the code would skip the comment-existence fallback and go straight to posting a new comment — producing duplicates every pulse cycle (~2 min).Verification
Tested against PR #2792 (the PR with 18 duplicate comments):
jq any()correctly returnstruewhenexternal-contributorlabel is presentjq select+testcorrectly counts existing commentsCloses #2800
Summary by CodeRabbit