fix: downgrade REQUEST_CHANGES→COMMENT on self-authored PRs in safe-outputs PR review#47400
Conversation
…utputs PR review Fixes 422 "Can not request changes on your own pull request" failures that occur when the PR Code Quality Reviewer (or any reviewer workflow) runs against a PR authored by the same bot identity (e.g. linter-miner branches). Three complementary defences in pr_review_buffer.cjs: 1. Proactive check: before building requestParams, compare pullRequest.user.login against GITHUB_ACTOR. If they match, downgrade REQUEST_CHANGES/APPROVE → COMMENT immediately, avoiding the 422 entirely. 2. Own-PR COMMENT retry → body-only COMMENT fallback: when the own-PR COMMENT retry (reactive path) also fails with "Line could not be resolved" or "Path could not be resolved", fall back to a body-only COMMENT review instead of hard-failing. 3. Body-only fallback own-PR guard: when the "Line could not be resolved" body-only fallback itself gets a 422 "Can not request changes on your own pull request", downgrade the body-only event to COMMENT and retry — this is the exact scenario that caused the 4/4 safe-output failures in runs 29946005059 and 29910168345. Six new tests cover all three paths. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes self-authored PR review failures by downgrading restricted review events and adding resilient body-only fallbacks.
Changes:
- Adds proactive and reactive
COMMENTfallbacks. - Preserves unresolvable inline feedback in review bodies.
- Adds regression tests for fallback paths.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/pr_review_buffer.cjs |
Implements self-review downgrade and fallback handling. |
actions/setup/js/pr_review_buffer.test.cjs |
Tests proactive downgrade and body-only retries. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
actions/setup/js/pr_review_buffer.test.cjs:1573
- This test is environment-dependent: on a workflow triggered by
linter-miner[bot], the new proactive check downgrades the initial request toCOMMENT, so the mocked call sequence no longer reaches the body-onlyREQUEST_CHANGESrejection and the assertion fails. SetGITHUB_ACTORto a known different value (or clear it) and restore it after the test.
pullRequest: { head: { sha: "abc123" }, user: { login: "linter-miner[bot]" } },
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Medium
| // Proactively downgrade REQUEST_CHANGES/APPROVE → COMMENT on self-authored PRs. | ||
| // GITHUB_TOKEN lacks read:user scope for reliable identity pre-flight via the API, so | ||
| // compare the PR author login against GITHUB_ACTOR as a best-effort guard. | ||
| // The reactive 422 retry in the catch block is retained as a safety net for cases this misses. | ||
| if (event !== "COMMENT" && pullRequest.user?.login) { | ||
| const actor = (process.env.GITHUB_ACTOR || "").trim(); | ||
| if (actor && pullRequest.user.login === actor) { | ||
| core.warning(`PR #${pullRequestNumber} is authored by the workflow actor ('${actor}'). ` + `Downgrading '${event}' → 'COMMENT' to avoid 422 "Can not request changes on your own pull request".`); | ||
| event = "COMMENT"; | ||
| } | ||
| } |
| repo: "owner/repo", | ||
| repoParts: { owner: "owner", repo: "repo" }, | ||
| pullRequestNumber: 42, | ||
| pullRequest: { head: { sha: "abc123" }, user: { login: "bot-user" } }, |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47400 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
@copilot please run the Open unresolved review feedback (newest first):
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix: downgrade REQUEST_CHANGES→COMMENT on self-authored PRs
The fix is correct and well-structured. The three-layer defence (proactive GITHUB_ACTOR check, reactive 422 retry, body-only COMMENT fallback) covers the observed failure path cleanly.
No blocking issues. The two existing inline comments identify the non-blocking concerns worth tracking:
- The
GITHUB_ACTORproactive guard is a best-effort heuristic when the review token identity differs from the workflow actor. The reactive 422 catch is retained as the true safety net, which is the right design. - The body-only fallback test at line 1044 could be fragile if CI happens to run under the same actor as the mocked PR author, but this is a test environment concern, not a production bug.
All changed code paths are covered by tests. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 27.8 AIC · ⌖ 5.6 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (6 tests)
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two correctness issues.
📋 Key Themes & Highlights
Key Themes
requestParamsmutation — the reactive own-PR retry mutatesrequestParams.event = "COMMENT"in-place (line 628), diverging from the spread-based pattern used in the body-only path. A future reader or retry leg sees stale state.- Fragile error-string duplication — the
"Line could not be resolved"check at line 637 is a second copy of the same predicate already at line ~720; extracting a shared helper would prevent drift. - Environment-sensitive tests — several new reactive-path tests don't pin
GITHUB_ACTOR, so the proactive downgrade can silently fire and invalidate call-count assertions if CI runs under a matching actor name.
Positive Highlights
- ✅ Six well-structured new tests covering both proactive and reactive paths
- ✅
GITHUB_ACTORis restored infinallyin the proactive-downgrade tests — good hygiene - ✅ The proactive guard is a clean, low-risk approach; the comment explaining the GITHUB_TOKEN scope limitation is exactly the right level of documentation
- ✅ Body-only COMMENT cascade handles all known error permutations of the original bug
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 40 AIC · ⌖ 4.86 AIC · ⊞ 6.7K
Comment /matt to run again
Comments that could not be inline-anchored
actions/setup/js/pr_review_buffer.cjs:628
[/diagnosing-bugs] requestParams.event is mutated in-place before the retry. If any later code path reads requestParams (logging, future retry leg), it sees COMMENT instead of the original event — a silent state corruption.
<details>
<summary>💡 Suggested fix</summary>
Use a spread to avoid mutation, consistent with how bodyOnlyParams is constructed at line 640:
const commentParams = { ...requestParams, event: "COMMENT" };
const { data: review } = await createReviewWithRetr…
</details>
<details><summary>actions/setup/js/pr_review_buffer.cjs:637</summary>
**[/diagnosing-bugs]** The line-resolution check only matches `"Line could not be resolved"` and `"Path could not be resolved"`. The GitHub API can also return `"Pull request review thread not found"` or similar for stale diffs — those errors fall through to the generic failure branch and lose the own-PR COMMENT context.
<details>
<summary>💡 Suggested fix</summary>
Consider extracting a shared `isLineResolutionError(msg)` predicate (keeping it consistent with the check at line ~720) so both …
</details>
<details><summary>actions/setup/js/pr_review_buffer.test.cjs:1054</summary>
**[/tdd]** This test relies on the proactive downgrade *not* firing — but the test fixture sets `pullRequest.user.login = "bot-user"` with no control over `GITHUB_ACTOR`. If CI runs as `bot-user`, the proactive branch fires first, the mocked 422 is never hit, and the call-count assertion (`toHaveBeenCalledTimes(3)`) fails.
<details>
<summary>💡 Suggested fix</summary>
Set `GITHUB_ACTOR` to a different value (e.g. `"different-actor"`) at the top of this test, restoring it in `finally`, so the …
</details>
There was a problem hiding this comment.
Two issues need fixing before merge, plus a minor accounting bug.
### Findings summary
Blocking
-
bodyOnlyParams.eventmutated in-place (line 740) —createReviewWithRetryuseswithRetryinternally; mutating the params object before the call means all retry iterations already seeCOMMENT, not the originally-intended event. Use{ ...bodyOnlyParams, event: "COMMENT" }like the parallel path at line 640 does. -
No test for empty/absent
GITHUB_ACTOR— The proactive downgrade guardif (actor && pullRequest.user.login === actor)has no test asserting that an unset or whitespace-onlyGITHUB_ACTORleaves the event unchanged. The truthiness check is the only thing preventing every review from being silently downgraded if that guard is ever loosened.
Non-blocking
comment_count: 0hardcoded in theownPrBodyOnlyParamssuccess result (line 650) — the comments were embedded into the body byappendUnanchoredCommentsSection, so passing0misreports the number of comments actually delivered. Either passcomments.lengthor document the field as "anchored inline count only".
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 53.8 AIC · ⌖ 4.97 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/pr_review_buffer.cjs:740
bodyOnlyParams.event mutated in-place, corrupting params for internal retries: createReviewWithRetry wraps calls in withRetry; mutating the shared object before the first attempt means all retries already see COMMENT before any attempt runs.
<details>
<summary>💡 Suggested fix</summary>
Use a shallow clone, consistent with the pattern at line 640:
// Instead of:
bodyOnlyParams.event = "COMMENT";
const { data: review } = await createReviewWithRetry(bodyOnlyParams);
// Do:
c…
</details>
<details><summary>actions/setup/js/pr_review_buffer.test.cjs:108</summary>
**No test for empty/absent `GITHUB_ACTOR` — the guard can break silently**: the proactive downgrade is protected by `if (actor && ...)`, but there is no test asserting that when `GITHUB_ACTOR` is unset or empty, the event is left unchanged. If someone accidentally removes that truthiness check, every review submission would be silently downgraded to `COMMENT` regardless of who authored the PR.
<details>
<summary>💡 Suggested fix</summary>
Add a test case:
```js
it("should not downgrade when …
</details>
<details><summary>actions/setup/js/pr_review_buffer.cjs:650</summary>
**`comment_count: 0` in body-only fallback misreports how many comments were actually submitted**: `buildReviewSuccessResult(review, "COMMENT", 0, afterState)` hard-codes 0 inline comments, but `appendUnanchoredCommentsSection` folded the real comments into the body. Any caller inspecting `result.comment_count` (e.g. for audit logging or safe-output metadata) will undercount submitted feedback.
<details>
<summary>💡 Suggested fix</summary>
Pass the actual comment count:
```js
// Instead of:
…
</details>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in bff72c7. I removed the |
|
🎉 This pull request is included in a new release. Release: |
Reviewer workflows fail with
422 "Can not request changes on your own pull request"when running against*-mineror other bot-authored PRs where the reviewing identity matches the PR author. The existing own-PR retry logic was bypassed when the initial failure was"Line could not be resolved"— the body-only fallback then ran withREQUEST_CHANGESand also hit 422, causing all safe outputs to fail.Changes (
pr_review_buffer.cjs)Proactive downgrade — before building
requestParams, comparepullRequest.user.loginagainstGITHUB_ACTOR; if they match, rewriteREQUEST_CHANGES/APPROVE→COMMENTbefore any API call, preventing the 422 entirely:Body-only fallback own-PR guard — the
"Line could not be resolved"body-only fallback now catches own-PR 422 on the retry and re-submits as body-onlyCOMMENT. This is the exact path that failed in runs29946005059/29910168345.Own-PR COMMENT retry → body-only COMMENT — if the reactive
COMMENTretry (inline comments preserved) still fails with line-resolution errors, fall back to body-onlyCOMMENTinstead of hard-failing.Tests
Six new tests in
pr_review_buffer.test.cjscover: proactive GITHUB_ACTOR match (REQUEST_CHANGES and APPROVE), no-downgrade when actors differ, own-PR retry → body-only COMMENT, body-only → own-PR 422 → COMMENT success, and body-only COMMENT failure propagation.Run: https://github.com/github/gh-aw/actions/runs/29961275366