Skip to content

Always minimize /review, /review rerun, and /review tests command comments once authorized - #36021

Merged
PureWeen merged 2 commits into
mainfrom
fix/review-tests-minimize-pr-perms
Jun 19, 2026
Merged

Always minimize /review, /review rerun, and /review tests command comments once authorized#36021
PureWeen merged 2 commits into
mainfrom
fix/review-tests-minimize-pr-perms

Conversation

@kubaflo

@kubaflo kubaflo commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Note

Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!

Summary

Follow-up to #35895, which switched the /review, /review rerun, and /review tests command-comment cleanup from delete to GraphQL minimizeComment(classifier: RESOLVED). In practice the command comments were still left visible in several cases. This PR makes all three reliably collapse once the command is recognized and the commenter is authorized — regardless of the command's result.

Root causes (observed on #30311)

  1. /review tests — permission bug. The gh-aw copilot-review-tests pre-activation job that minimizes the comment only had issues: write. The comment lives on a pull request, so minimizeComment needs pull-requests: write; with only issues: write it failed with Resource not accessible by integration (run 27824496263) and the comment stayed visible. Hide /review, /review rerun, and /review tests command comments as resolved #35895's assumption that "minimizing requires the same issues:write scope that deletion did" is incorrect for PR conversation comments.

  2. /review rerun — eligibility gate. The mark-rerun-ready hide step was gated on eligible == 'true', so an ineligible rerun (e.g. no-new-comments-or-commits) left the comment fully visible.

  3. /review — trigger-outcome gate. The trigger-review hide step was gated on trigger_azdo.outcome == 'success', so a lock-skip or a failed AzDO trigger left the comment visible.

Changes

copilot-review-tests.md (gh-aw)

  • Add pull-requests: write to the pre-activation job (on.permissions) so it can minimize a PR comment. The AI agent job stays read-only.
  • Recompiled copilot-review-tests.lock.yml with gh-aw v0.79.8; only frontmatter_hash and the pre-activation permission change — body_hash is unchanged.

review-trigger.yml

  • /review rerun and /review now minimize the command comment whenever the actor is authorized and the command was recognized, no matter the downstream outcome.
  • Each job's "Check actor permission" step gets an id: auth; the hide step is gated on !cancelled() && github.event_name == 'issue_comment' && steps.auth.outcome == 'success'. !cancelled() lets the hide run even after an upstream step failed (failed trigger / ineligible rerun / errored eligibility), while the steps.auth gate keeps unauthorized commenters' comments fully visible.

Safety / scope

  • Unauthorized comments are never hidden (the steps.auth.outcome == 'success' gate, and the internal collaborator check in the /review tests script).
  • Transparent to the rerun scanner. Resolve-RerunEligibility.ps1 / Query-RerunReadyPRs.ps1 / Get-LatestRerunCommentBefore key on comment id, body and created_at — never on isMinimized — and minimized comments are still returned by the REST list endpoint. Verified there are no isMinimized references in those scripts.
  • A failed minimize emits a ::warning:: and never fails the review/rerun/tests trigger.
  • The only outcome not covered is a cancelled/timed-out run (!cancelled() skips on interruption); every completed outcome is covered.

Validation

  • gh aw compile → 0 errors / 0 warnings; recompiling produced no stray diff.
  • review-trigger.yml parses as valid YAML.

Copilot AI added 2 commits June 19, 2026 14:11
…s:write

The copilot-review-tests pre-activation job minimizes the triggering
`/review tests` comment via GraphQL minimizeComment(classifier: RESOLVED),
but its token only had issues:write. Because the comment lives on a pull
request, that call requires pull-requests:write and otherwise fails with
"Resource not accessible by integration" (observed on PR #30311, run
27824496263). The sibling /review and /review rerun jobs in
review-trigger.yml already have pull-requests:write and minimize correctly.

Add pull-requests:write to the pre-activation job (on.permissions) and
recompile the lock file. The agent job stays read-only; body_hash is
unchanged.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Previously the /review comment was only minimized when the AzDO pipeline
trigger succeeded, and /review rerun only when the rerun was eligible, so
lock-skipped / failed / ineligible commands lingered in the conversation.

Hide the command comment whenever the actor is authorized and the command
was recognized, regardless of the downstream outcome. Gate on a new
`id: auth` step outcome plus !cancelled() so the hide still runs after an
upstream step fails, while unauthorized commenters' comments stay visible.

Minimizing remains transparent to the rerun scanner, which keys on comment
id/body/created_at and never on isMinimized.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 36021

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 36021"

@github-actions github-actions Bot added the area-infrastructure CI, Maestro / Coherency, upstream dependencies/versions label Jun 19, 2026
kubaflo pushed a commit to kubaflo/maui that referenced this pull request Jun 19, 2026
Document recent workflow improvements:
- Command comments are now minimized (collapsed as Resolved) after authorization (dotnet#35895, dotnet#36021)
- /review rerun eligibility now requires PR author activity only (dotnet#35874)
- Automated hourly rerun scanner processes queued reruns (dotnet#35685)
- Add troubleshooting entries for rerun eligibility and command visibility

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

@PureWeen PureWeen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Adversarial multi-model review — ✅ no blocking issues

Reviewed by 3 independent reviewers with adversarial consensus. All three converged on clean — no must-fix or should-fix findings. No prior reviews existed.

What was verified (consensus in parentheses):

  • Authorization is preserved. An unauthorized commenter can never hide their own comment: the new gate requires steps.auth.outcome == 'success', and the permission step exit 1s for unauthorized actors (outcome == 'failure') — and is 'skipped' on workflow_dispatch, where the hide step's github.event_name == 'issue_comment' clause also blocks it. (3/3)
  • !cancelled() is the right broadening. It lets the hide run after an upstream failure (ineligible rerun, lock-skip, failed AzDO trigger) while still skipping on real cancellation — matching the stated intent of "collapse once authorized, regardless of result." Using outcome (not conclusion) is correct here. (3/3)
  • pull-requests: write is minimal and corroborated. minimizeComment on a PR conversation comment needs pull-requests: writeissues: write alone yields "Resource not accessible by integration." This is corroborated in-repo: both review-trigger.yml jobs already declare both scopes for the identical mutation. The grant is scoped to the pre-activation job; the AI agent job stays read-only. (3/3)
  • Transparent to the rerun scanner. Resolve-RerunEligibility.ps1 / Query-RerunReadyPRs.ps1 key purely on comment id/body/created_at and never on isMinimized; the REST list endpoint still returns minimized comments. Independently grep-confirmed — zero isMinimized references in the scanner path. (verified)
  • .lock.yml is a faithful recompile. body_hash is unchanged (44becb59…); only frontmatter_hash and the new pre-activation permission changed (gh-aw v0.79.8). No hand-edit tampering. (3/3)
  • No regression. The new gate is strictly broader than the old ones for authorized users, so nothing that was previously hidden is now left visible; the only behavioral change is the intended one (ineligible reruns / lock-skips / failed triggers now also collapse).

Non-blocking observations (💡, no change requested):

  • issues: write may now be redundant alongside pull-requests: write for the minimize-only scope — worth a future audit, but keeping both mirrors existing precedent and is safer to leave as-is.
  • With cancel-in-progress: false, two near-simultaneous identical commands could both try to minimize the same node_id; minimizeComment is idempotent and a failed hide only emits ::warning::, so this is harmless.

Test coverage: Workflow-YAML/permission changes with no unit-test surface; validated by gh aw compile (0 errors / 0 warnings, no stray diff), YAML parse, and the cited failing-run evidence (27824496263) for the permission root cause. Reasonable for this change class.

Methodology: 3 independent reviewers, different models, with adversarial cross-validation of disputed findings. Review-only — not an approval.

@PureWeen
PureWeen merged commit b4d4b25 into main Jun 19, 2026
9 of 10 checks passed
@PureWeen
PureWeen deleted the fix/review-tests-minimize-pr-perms branch June 19, 2026 19:46
@github-actions github-actions Bot added this to the .NET 10 SR9 milestone Jun 19, 2026
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 20, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-infrastructure CI, Maestro / Coherency, upstream dependencies/versions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants