Skip to content

t2800: fix pulse idempotency guard for external-contributor label check #2801

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alex-solovyev merged 1 commit into from
Mar 4, 2026
Merged

t2800: fix pulse idempotency guard for external-contributor label check #2801

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 17 additions & 13 deletions .agents/scripts/commands/pulse.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,18 +79,22 @@ perm=$(echo "$response" | tail -1 | jq -r '.permission // empty' 2>/dev/null)

```bash
# Idempotency guard: skip if already labelled OR already commented (belt-and-suspenders).
# IMPORTANT: Do NOT suppress stderr on the label check — a failed API call must not
# silently bypass the guard (root cause of duplicate comments in #2795).
labels=$(gh pr view <number> --repo <slug> --json labels --jq '.labels[].name' 2>&1) || true
if echo "$labels" | grep -q '^external-contributor$'; then
# Uses jq any() for a clean boolean result — avoids grep anchor issues with multi-line
# output that caused duplicate comments (#2795, #2800).
has_label=$(gh pr view <number> --repo <slug> --json labels --jq 'any(.labels[]; .name == "external-contributor")' 2>&1) || true
if [ "$has_label" = "true" ]; then
: # Already labelled — skip
elif gh pr view <number> --repo <slug> --json comments --jq '.comments[].body' 2>&1 | grep -qiF 'external contributor'; 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
# Label not found (or API failed) — check for existing comment as fallback
existing_comment=$(gh pr view <number> --repo <slug> --json comments --jq '[.comments[].body | select(test("external contributor"; "i"))] | length' 2>&1) || true
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
Comment on lines +90 to +97
Copy link

@gemini-code-assist gemini-code-assist bot Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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
  1. 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.

fi
```

Expand All @@ -100,9 +104,9 @@ Then skip to the next PR. Do NOT dispatch workers to fix failing CI on external

```bash
# Only comment once — check for existing permission-failure comment.
# Do NOT suppress stderr — a failed API call must not bypass the guard.
comments=$(gh pr view <number> --repo <slug> --json comments --jq '.comments[].body' 2>&1) || true
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
Copy link

@gemini-code-assist gemini-code-assist bot Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
if [ "$perm_comment_count" = "0" ] || [ -z "$perm_comment_count" ]; then
if [ "$perm_comment_count" = "0" ]; then

gh pr comment <number> --repo <slug> --body "Permission check failed for this PR (HTTP $http_status from collaborator permission API). Unable to determine if @<author> is a maintainer or external contributor. **A maintainer must review and merge this PR manually.** This is a fail-closed safety measure — the pulse will not auto-merge until the permission API succeeds."
fi
```
Expand Down
Loading