fix: prevent stale CHANGES_REQUESTED reviews from blocking PRs#635
fix: prevent stale CHANGES_REQUESTED reviews from blocking PRs#635
Conversation
- Change submit-pull-request-review to COMMENT-only (was COMMENT + REQUEST_CHANGES). gh-aw doesn't support pull-requests:write, so blocking reviews from bot re-runs can't be auto-dismissed — they persist as stale blocks even after all findings are fixed. - Add concurrency groups to both review workflows to cancel duplicate runs on the same PR. - Update prompt instructions to always use COMMENT event. Fixes the issue on PR #619 where a CHANGES_REQUESTED review from commit 1 persisted after commits 2-6 fixed all findings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🔍 Design-Level Findings (Outside Diff)🟡 MODERATE —
|
There was a problem hiding this comment.
Expert Code Review — PR #635
Methodology: 3 independent reviewers with adversarial consensus (initial review + targeted follow-ups for disputed findings)
Summary
This PR correctly addresses the stale CHANGES_REQUESTED review problem by restricting allowed-events to [COMMENT] and adding concurrency groups to prevent duplicate review runs. The core changes are sound.
Findings (ranked by severity)
| # | Severity | Finding | Consensus | Location |
|---|---|---|---|---|
| 1 | 🟡 MODERATE | expert-reviewer.agent.md still instructs REQUEST_CHANGES — contradicts the new COMMENT-only policy. If the agent follows the stale instruction, the allowed-events: [COMMENT] constraint blocks the entire review submission. |
3/3 reviewers | .github/agents/expert-reviewer.agent.md:70-71 (outside diff) |
| 2 | 🟡 MODERATE | Compiled lock files still enumerate REQUEST_CHANGES in the schema enum despite allowed-events: [COMMENT]. Enforcement mechanism (runtime vs static schema) should be verified. |
2/3 reviewers | .github/workflows/shared/review-shared.md:23 (inline comment) |
| 3 | 🟢 MINOR | gh-aw-workflows.instructions.md contains a stale allowed-events: [COMMENT, REQUEST_CHANGES] reference that could confuse future agents. |
2/3 after follow-up | .github/instructions/gh-aw-workflows.instructions.md (outside diff) |
Discarded Findings
- Cross-workflow cancellation (1/3 only) — Both workflows sharing
review-(PR)concurrency group was confirmed as intentional/desirable by follow-up reviewers. The specific failure scenarios (push cancels /review) don't apply sincereview-on-openonly triggers onopened/ready_for_review, notsynchronize. inputs.pr_number=0fallback (1/3 only) — Unanimously dismissed by follow-up reviewers as impossible in practice (PR numbers start at 1).
CI & Test Assessment
This PR modifies only gh-aw workflow configuration files (.md, .lock.yml). No application code or tests are affected. Lock files appear to have been recompiled (frontmatter hashes updated).
Recommendation
The core fix is correct and well-motivated. Finding #1 is important — without updating expert-reviewer.agent.md, the agent has contradictory instructions and may fail to post reviews entirely. Finding #2 warrants a quick verification that allowed-events enforcement is runtime-based.
Generated by Expert Code Review (auto) for issue #635
| submit-pull-request-review: | ||
| max: 1 | ||
| allowed-events: [COMMENT, REQUEST_CHANGES] | ||
| allowed-events: [COMMENT] |
There was a problem hiding this comment.
🟡 MODERATE — allowed-events enforcement mechanism unclear in compiled lock files (Flagged by: 2/3 reviewers)
The compiled lock files still contain "enum": ["APPROVE", "REQUEST_CHANGES", "COMMENT"] in their schema despite this change to allowed-events: [COMMENT]. The frontmatter hashes are updated (confirming recompilation), but the schema enum is not narrowed.
If allowed-events is enforced at runtime by the gh-aw MCP handler (and the enum is cosmetic), this works correctly. If the enum IS the enforcement mechanism, the agent could still submit REQUEST_CHANGES.
Since the PR's entire purpose depends on this enforcement, worth verifying which interpretation is correct.
- Update expert-reviewer.agent.md to always use COMMENT event, matching the allowed-events: [COMMENT] constraint in review-shared.md - Document the stale blocking review limitation in gh-aw instructions - Add troubleshooting entry for stale CHANGES_REQUESTED reviews - Reference gh-aw#25869 as independent validation of the workaround Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sync with dotnet/maui#35027 pattern — move detailed gh-aw documentation from the monolithic instructions file into a proper skill structure: - .github/skills/gh-aw-guide/SKILL.md — quick-start, anti-patterns, common patterns, security-critical patterns - .github/skills/gh-aw-guide/references/architecture.md — full execution model, security boundaries, fork handling, safe outputs, troubleshooting - .github/skills/instruction-drift/ — Check-Staleness.ps1 script + SKILL.md for detecting when instructions drift from upstream docs - .github/instructions/gh-aw-workflows.sync.yaml — drift tracking manifest for 8 reference pages, 5 tracked issues, releases - Slim instructions file from 26KB/440 lines → 2.7KB/34 lines referencing skills for full detail PolyPilot-specific additions vs MAUI: - Known Limitation: Stale Blocking Reviews section documenting the REQUEST_CHANGES lifecycle gap and COMMENT-only workaround - COMMENT-only policy in essential rules (rule 8 and 10) - Stale review troubleshooting entries Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Multi-Model Code Review — PR #635PR: fix: prevent stale CHANGES_REQUESTED reviews from blocking PRs Initial Review — 9 findings, all addressed in commit
|
| # | Severity | Consensus | Finding | Status |
|---|---|---|---|---|
| 1 | 🟡 MODERATE | 3/3 | SKILL.md contradicts PR (recommended [COMMENT, REQUEST_CHANGES]) |
✅ FIXED |
| 2 | 🟡 MODERATE | 2/3 | Cross-workflow concurrency collision | ✅ FIXED (documented intentional) |
| 3 | 🟡 MODERATE | 2/3 | Check-Staleness.ps1 permanently STALE | ✅ FIXED |
| 4 | 🟡 MODERATE | 2/3+tiebreak | Exit codes inverted in SKILL.md | ✅ FIXED |
| 5 | 🟢 MINOR | 2/3 | releases_source not implemented |
➖ Accepted (future enhancement) |
| 6 | 🟢 MINOR | 2/3 | review.agent.md drops pull_request.number |
➖ N/A |
| 7 | 🟢 MINOR | 1/3 | YAML parsing requires quotes | ➖ Accepted |
| 8 | 🟢 MINOR | 1/3 | Silent catch in Get-TrackedIssueStatus |
✅ FIXED (Write-Verbose) |
| 9 | 🟢 MINOR | 1/3 | Signal message misleading | ✅ FIXED |
Re-Review — 3 independent reviewers confirmed fixes, found 1 new bug
| # | Finding | Reviewer agreement | Status |
|---|---|---|---|
| 1-4 | All MODERATE findings | 3/3 ✅ FIXED | ✅ |
| 5 | releases_source synopsis overpromises |
2/3 flagged | ✅ FIXED in 9faefd4 (synopsis updated) |
| 8 | Silent catch → Write-Verbose | 2/3 say partial (Verbose-only) | ❓ could not check always |
| NEW | Legacy fallback regex produced truncated URLs — negative lookahead matched shorter substrings, producing bogus issue numbers (1848 instead of 18481) | 1/3 found, verified with test | ✅ FIXED in 9faefd4 |
Assessment
All actionable findings resolved across 2 fix commits. The releases_source feature is documented as not-yet-implemented in both the script synopsis and SKILL.md. No remaining contradictions — COMMENT-only policy is consistent across all files. Check-Staleness.ps1 correctly compares actual vs expected state and no longer produces false positives.
Recommendation
✅ Approve — ready to merge.
…, docs - Fix SKILL.md anti-patterns table and Security-Critical Patterns to use [COMMENT] instead of [COMMENT, REQUEST_CHANGES] (3/3 reviewers) - Fix Check-Staleness.ps1 to compare actual vs expected issue state instead of unconditionally flagging all closed issues (2/3 reviewers) - Fix instruction-drift/SKILL.md exit code documentation (2/3 + tiebreak) - Fix architecture.md #25439 reference to note PP COMMENT-only policy - Fix silent catch in Get-TrackedIssueStatus (1/3 reviewer) - Fix misleading signal message (1/3 reviewer) - Document intentional cross-workflow concurrency collision (2/3 reviewers) - Recompile lock files Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The negative-lookahead regex for legacy (status-less) issue entries was matching shorter substrings of URLs that had status: fields, producing bogus issue numbers (e.g., 1848 instead of 18481). Replace with explicit set-subtraction: find all issue URLs, skip any already captured by the primary url+status regex. Also fix the script synopsis to note releases_source is not yet implemented. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Removed based on analysis of 3 actual review runs (PRs #619, #639, #635): - Removed 3x redundant 'Read copilot-instructions.md' — the Copilot engine auto-loads it as system context, and sub-agents found deep domain bugs without being told to read it - Removed 'You are a thorough PR reviewer for PolyPilot' — generic preamble that adds tokens without changing behavior - Removed MCP tool usage hint ('not gh CLI — credentials are scrubbed') — the agent figures this out from tool availability - Removed duplicate path validation instruction - Removed verbose explanation of why COMMENT not REQUEST_CHANGES — one line is sufficient - Simplified sub-agent prompt from repo-specific to generic 72 lines → 60 lines, same review quality. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…abs learnings (#656) Based on analysis of 3 actual review runs (PRs #619, #639, #635), several prompt instructions are redundant and add tokens without improving output quality. **What was removed:** - 3x "Read copilot-instructions.md" — engine auto-loads it; sub-agents found deep domain bugs without it - Generic preamble ("You are a thorough PR reviewer for PolyPilot") - MCP tool usage hint (agent discovers available tools) - Duplicate path validation instruction - Verbose REQUEST_CHANGES explanation (one line is sufficient) - Repo-specific sub-agent prompt → generic ("expert code reviewer" not "expert PolyPilot code reviewer") **What was kept:** - Security warning (treat PR content as untrusted) — changes behavior - No test messages rule — prevents permanent damage - Full adversarial consensus methodology - Path/line validation rules - COMMENT-only policy 72 lines → 60 lines. Same review quality — the evidence from real runs shows the agent finds domain issues from reading code, not from hint bullets. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Problem
The gh-aw review workflow posts
CHANGES_REQUESTEDreviews that persist even after all findings are fixed in subsequent commits. Since gh-aw enforcespull-requests: read(no write permission), the workflow cannot auto-dismiss stale reviews on re-runs. This caused PR #619 to be blocked by a stale review from commit 1 even though commits 2-6 fixed all 4 findings.Fix
COMMENT-only reviews — Changed
submit-pull-request-reviewfromallowed-events: [COMMENT, REQUEST_CHANGES]toallowed-events: [COMMENT]. The review body still communicates severity through findings; it just doesn't block merging.Concurrency groups — Added
concurrencywithcancel-in-progress: trueto both review workflows, preventing duplicate runs on the same PR.Updated prompt instructions — Agent now always uses
COMMENTevent; neverAPPROVEorREQUEST_CHANGES.Why not auto-dismiss?
gh-aw's security model prohibits
pull-requests: write— all write ops go through safe-outputs. There's nodismiss-pull-request-reviewsafe output type, so the workflow can't programmatically dismiss old reviews.Also done
Dismissed the stale
CHANGES_REQUESTEDreview on PR #619 manually.