Skip to content

[DevOps] Add missing comment triggers to Needs: Attention policy#2079

Open
flanakin wants to merge 1 commit intodevfrom
flanakin/needs-attention-policy
Open

[DevOps] Add missing comment triggers to Needs: Attention policy#2079
flanakin wants to merge 1 commit intodevfrom
flanakin/needs-attention-policy

Conversation

@flanakin
Copy link
Copy Markdown
Collaborator

@flanakin flanakin commented Apr 3, 2026

🛠️ Description

Updates the pulls-03-feedback GitOps policy to trigger Needs: Attention 👋 on ALL comment types, not just review comments and change requests.

Changes:

  • Added Issue_Comment payload type to catch general PR conversation comments
  • Removed issueAuthor: False filter so comments from any user (including the PR author) trigger the label

Previously, regular conversation comments on a PR and author self-comments (e.g., inline notes to hold a PR) would not trigger the label.

📋 Checklist

🔬 How did you test this change?

  • 🤏 Lint tests

🙋‍♀️ Do any of the following that apply?

  • 🤏 The change is less than 20 lines of code.

📑 Did you update docs/changelog.md?

  • ❎ Log not needed (small/internal change)

📖 Did you update documentation?

  • ❎ Docs not needed (small/internal change)

🤖 Generated with Claude Code

Add Issue_Comment payload type and remove issueAuthor filter so that
ALL comments on a PR — inline review comments, formal reviews, and
conversation comments from any user — trigger the Needs: Attention label.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@flanakin flanakin requested a review from MSBrett as a code owner April 3, 2026 11:41
Copilot AI review requested due to automatic review settings April 3, 2026 11:41
@flanakin flanakin changed the title Add missing comment triggers to Needs: Attention policy [DevOps] Add missing comment triggers to Needs: Attention policy Apr 3, 2026
@microsoft-github-policy-service microsoft-github-policy-service Bot added the Micro PR 🔬 Very small PR that should be especially easy for newcomers label Apr 3, 2026
@microsoft-github-policy-service microsoft-github-policy-service Bot added the Needs: Review 👀 PR that is ready to be reviewed label Apr 3, 2026
@microsoft-github-policy-service microsoft-github-policy-service Bot added the Skill: DevOps GitHub setup and automation label Apr 3, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the GitOps PR feedback policy so the Needs: Attention 👋 label is applied for more kinds of PR feedback events, including general PR conversation comments.

Changes:

  • Adds Issue_Comment (action: Created) as an additional trigger for the feedback workflow.
  • Removes the isActivitySender: issueAuthor: False constraint so the PR author’s own comments can also trigger the workflow.

- and:
- payloadType: Issue_Comment
- isAction:
action: Created
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

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

Adding payloadType: Issue_Comment here makes author comments like #needs-review match this policy too. That conflicts with .github/policies/pulls-04-respond.yml (which uses #needs-review to remove Needs: Attention 👋 and re-add Needs: Review 👀): the #needs-review comment will now also trigger this policy and can immediately re-add Needs: Attention 👋, making #needs-review ineffective. Exclude #needs-review from this policy (or split tasks so Issue_Comment labeling doesn’t run for that command) to avoid the label ping-pong.

Suggested change
action: Created
action: Created
- not:
commentContains:
pattern: '#needs-review'

Copilot uses AI. Check for mistakes.
Comment on lines +31 to +34
- and:
- payloadType: Issue_Comment
- isAction:
action: Created
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

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

With Issue_Comment triggering this task and an unconditional addReply in the then, the bot’s own reply comment is likely an Issue_Comment (action: Created) and can re-trigger this same policy, potentially creating a self-reply loop (and repeated label/assignment churn). Add a guard to ignore comments authored by the policy bot (or restructure into separate tasks so addReply is not executed for Issue_Comment events).

Copilot uses AI. Check for mistakes.
@flanakin flanakin added this to the v14 milestone Apr 4, 2026
@flanakin flanakin added Needs: Attention 👋 Issue or PR needs to be reviewed by the author or it will be closed due to no activity and removed Needs: Review 👀 PR that is ready to be reviewed labels Apr 8, 2026
@flanakin flanakin modified the milestones: v14, v15 Apr 8, 2026
Copy link
Copy Markdown
Contributor

@MSBrett MSBrett left a comment

Choose a reason for hiding this comment

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

Review summary

CI is green and the intent of the change is solid, but the diff introduces a label-thrash race condition with .github/policies/pulls-04-respond.yml and regresses the non-author guard on two existing triggers. One small consolidated fix resolves both issues. There is also a design intent question worth confirming before merge — see "Design clarification" below.

Race condition / regression details

In .github/policies/pulls-03-feedback.yml (PR head):

  1. The new Issue_Comment / Created trigger has no isActivitySender constraint. It therefore fires when the PR author comments. pulls-04-respond.yml already handles author Issue_Comment activity by removing Needs: Attention 👋 and restoring Needs: Review 👀. Both policies will run on the same author event with opposing outcomes — classic label thrash, with the final state depending on undefined ordering.

  2. The diff also removes isActivitySender: issueAuthor: False from the existing Pull_Request_Review_Comment / Created and Pull_Request_Review / Commented triggers. Those guards are present today on dev and are intentional — they prevent author self-comments from firing the "give the author feedback" path. Removing them is a regression independent of the new trigger.

Pull_Request_Review / Changes_requested correctly remains unconstrained — GitHub does not allow PR authors to request changes on their own PR, so the guard would be redundant there.

Suggested fix

Apply the suggestion below to restore the guards on the two regressed triggers and add a matching guard to the new Issue_Comment trigger.

              - and:
                  - payloadType: Pull_Request_Review_Comment
                  - isAction:
                      action: Created
                  - isActivitySender:
                      issueAuthor: False
              - and:
                  - payloadType: Pull_Request_Review
                  - isReviewState:
                      reviewState: Changes_requested
              - and:
                  - payloadType: Pull_Request_Review
                  - isReviewState:
                      reviewState: Commented
                  - isActivitySender:
                      issueAuthor: False
              - and:
                  - payloadType: Issue_Comment
                  - isAction:
                      action: Created
                  - isActivitySender:
                      issueAuthor: False

After this change:

  • Reviewer comments / reviews on a PR → pulls-03-feedback adds Needs: Attention 👋 (intended).
  • Author comments on their own PR → only pulls-04-respond runs, removing Needs: Attention 👋 and restoring Needs: Review 👀 (no thrash).

Design clarification

The PR description says the goal is to make author self-comments also trigger Needs: Attention 👋. The fix above intentionally does not do that, because:

  • pulls-03-feedback.yml is described as "When a PR gets feedback" — semantically the reviewer side.
  • pulls-04-respond.yml already owns the author-comment side and clears Needs: Attention.
  • Allowing both to fire on the same event is the source of the race.

If the intent really is "author comments should add Needs: Attention," that is a design conflict with pulls-04-respond.yml that needs to be resolved at the policy level (probably by changing pulls-04 rather than removing guards in pulls-03). Could you confirm which behavior you want?

CI / metadata

  • All 6 checks passing (CodeQL ×3, GitOps/Branches, GitOps/YmlSchemaValidation, license/cla).
  • mergeable: MERGEABLE, mergeStateStatus: BLOCKED only because review is required.
  • Targets dev ✓. Labels appropriate. Checklist complete. Minor: PR body is missing a Fixes #<issue> link, not blocking.

@microsoft-github-policy-service
Copy link
Copy Markdown

@@flanakin: you have some new feedback!

Please review and resolve all comments and I'll let reviewers know by removing the Needs: Attention label. If I miss anything, just reply with #needs-review and I'll update the status.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Micro PR 🔬 Very small PR that should be especially easy for newcomers Needs: Attention 👋 Issue or PR needs to be reviewed by the author or it will be closed due to no activity Skill: DevOps GitHub setup and automation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants