Skip to content

fix(safe-output): prevent silent 422 on PR review submission#32910

Merged
pelikhan merged 6 commits into
mainfrom
copilot/deep-report-prevent-silent-422
May 18, 2026
Merged

fix(safe-output): prevent silent 422 on PR review submission#32910
pelikhan merged 6 commits into
mainfrom
copilot/deep-report-prevent-silent-422

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 17, 2026

Over 24h, 11 PR review submissions returned 422 Unprocessable Entity but the workflow reported Successful: 2 / Failed: 0 — the error was swallowed and no review was posted.

Root causes and fixes

Sub-pattern A — empty review (10 cases): Agent called submit_pull_request_review with no body and zero inline comments. GitHub 422s on a contentless review.

Pre-check added in submitReview() after footer generation: if body is empty and comments.length === 0, return { success: false } instead of POSTing.

Sub-pattern B — paths not in diff (1 case): Inline comments buffered at paths outside the PR diff cause 422: "Path could not be resolved".

Before POSTing, submitReview() now fetches pulls.listFiles (paginated, ≤1,000 files) and silently drops comments whose path is absent from the diff. Fail-open: if the API call throws or returns an empty list, path validation is skipped.

Fix 3 — misleading success count: Both submit_pull_request_review and create_pull_request_review_comment handlers return success: true immediately (they only buffer). When finalization fails, those results are retroactively flipped to success: false in processingResult.results, so the Processing Summary reflects the real outcome. The failure is also promoted from core.warning to core.error.

// Before fix
=== Processing Summary ===
Successful: 2     ← misleading
Failed: 0         ← wrong

// After fix
=== Processing Summary ===
Successful: 0
Failed: 2
✗ Failed to submit PR review: Unprocessable Entity: ""

Files changed

  • pr_review_buffer.cjs — Sub-pattern A guard + Sub-pattern B path validation in submitReview()
  • safe_output_handler_manager.cjs — rollback loop + core.error promotion in finalization block
  • pr_review_buffer.test.cjs — 10 new tests (empty-review guard, path filtering, fail-open, pagination)

Copilot AI and others added 2 commits May 17, 2026 23:23
Sub-pattern A: Guard against empty review POST (body="" + 0 comments)
- In submitReview(), after building the comments array and applying
  footer logic, return { success: false } when body is empty AND no
  inline comments remain. This blocks the 422 "Unprocessable Entity"
  that GitHub returns for such submissions.

Sub-pattern B: Validate comment paths against PR diff before POST
- Before POSTing, fetch the PR's changed-files list via
  pulls.listFiles (paginated, up to 10 pages).
- Filter out any buffered comments whose path is not in the diff,
  warning for each one skipped.
- Fail-open: if listFiles returns an empty list (unusual) or the API
  call throws, path validation is skipped and the review proceeds.

Fix 3: Roll back per-message success counts on finalization failure
- The submit_pull_request_review and create_pull_request_review_comment
  handlers return success:true during message processing (they only
  buffer). When the finalization POST fails, those buffered-message
  results are marked success:false so the Processing Summary shows the
  correct Failed count instead of the misleading "Failed: 0".
- Also upgrades the failure log from core.warning to core.error so it
  is surfaced correctly.

Tests:
- 10 new tests in pr_review_buffer.test.cjs covering:
  - Sub-pattern A: empty body + no comments → failure (not POST)
  - Sub-pattern A: body present or comments present → POST proceeds
  - Sub-pattern B: invalid paths filtered, valid paths pass
  - Sub-pattern B: all paths invalid → empty-review guard triggers
  - Sub-pattern B: empty listFiles response → no filtering
  - Sub-pattern B: listFiles API failure → fail-open with warning
  - Sub-pattern B: paginated listFiles (100+ files)

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix silent 422 errors in safe-output PR review submissions fix(safe-output): prevent silent 422 on PR review submission May 17, 2026
Copilot AI requested a review from pelikhan May 17, 2026 23:26
@pelikhan
Copy link
Copy Markdown
Collaborator

@copilot validate submitted review in safe output MCP server

Add MCP server-phase validation for submit_pull_request_review and
create_pull_request_review_comment tool calls in safe_outputs_handlers.cjs.

The agent now receives an immediate MCP error (code -32602) when:
1. event is REQUEST_CHANGES but body is empty (GitHub API requirement)
2. The review would be contentless — no body AND no inline comments were
   buffered in this session (early guard for Sub-pattern A, mirrors the
   pr_review_buffer.cjs guard at finalization time)

The session-scoped inline comment counter is reset after each successful
submit_pull_request_review call so that subsequent reviews in the same
MCP session also benefit from the guard.

Both handlers are registered in safe_outputs_tools_loader.cjs and covered
by 12 new tests in safe_outputs_handlers.test.cjs.

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 18, 2026

@copilot validate submitted review in safe output MCP server

Added MCP server-phase validation for submit_pull_request_review in safe_outputs_handlers.cjs (commit feat(safe-output): validate submitted review in safe output MCP server).

The agent now receives an immediate MCP error (-32602) when:

  1. event is REQUEST_CHANGES but body is empty — GitHub API requirement
  2. The review would be contentless — no body and no create_pull_request_review_comment calls were made in the session (early mirror of the Sub-pattern A guard in pr_review_buffer.cjs)

A session-scoped inlineReviewCommentCount counter is tracked by createPullRequestReviewCommentHandler and reset after each successful submit_pull_request_review so subsequent reviews in the same MCP session are also guarded. Both handlers are registered in safe_outputs_tools_loader.cjs and covered by 12 new tests.

@pelikhan pelikhan marked this pull request as ready for review May 18, 2026 02:27
Copilot AI review requested due to automatic review settings May 18, 2026 02:27
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

This PR aims to prevent silent PR review submission failures by validating empty reviews and filtering inline comments whose paths are not in the PR diff.

Changes:

  • Adds PR review MCP-phase handlers and empty-review validation.
  • Filters review comments by changed PR files before submission.
  • Marks buffered review messages as failed when final review submission fails.
Show a summary per file
File Description
actions/setup/js/safe_outputs_tools_loader.cjs Attaches PR review tools to custom MCP handlers.
actions/setup/js/safe_outputs_handlers.cjs Adds MCP handlers for review comments and review submission validation.
actions/setup/js/safe_outputs_handlers.test.cjs Adds tests for the new MCP review handlers.
actions/setup/js/safe_output_handler_manager.cjs Rolls back buffered review result statuses when finalization fails.
actions/setup/js/pr_review_buffer.cjs Adds path validation and empty-review guard before creating reviews.
actions/setup/js/pr_review_buffer.test.cjs Adds tests for empty-review and path-filtering behavior.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 6/6 changed files
  • Comments generated: 6

Comment thread actions/setup/js/pr_review_buffer.cjs Outdated
page: listPage,
});
if (!Array.isArray(files) || files.length === 0) break;
for (const f of files) changedPaths.add(f.filename);
Comment on lines +83 to +84
create_pull_request_review_comment: handlers.createPullRequestReviewCommentHandler,
submit_pull_request_review: handlers.submitPullRequestReviewHandler,
Comment on lines +1177 to +1178
const event = (args && args.event ? String(args.event).toUpperCase() : "COMMENT");

Comment on lines +1150 to +1158
* Handler for create_pull_request_review_comment tool (MCP server phase).
* Increments the session-scoped inline comment counter so that the subsequent
* submitPullRequestReviewHandler can detect an otherwise-empty review.
* Per Safe Outputs Specification MCE1: enforces constraints during tool invocation
* to provide immediate feedback to the LLM before recording to NDJSON.
*/
const createPullRequestReviewCommentHandler = args => {
inlineReviewCommentCount++;
return defaultHandler("create_pull_request_review_comment")(args);
Comment on lines +319 to +323
// Cap at 10 pages (1,000 files). PRs with more than 1,000 changed files are
// extremely rare and path validation is best-effort; we proceed without filtering
// if any individual listFiles call throws (see catch block below).
const MAX_LIST_FILES_PAGES = 10;
while (listPage <= MAX_LIST_FILES_PAGES) {
Comment on lines +1362 to +1366
(r.type === "submit_pull_request_review" || r.type === "create_pull_request_review_comment") &&
r.success === true
) {
r.success = false;
r.error = `Review finalization failed: ${reviewFailureError}`;
@pelikhan
Copy link
Copy Markdown
Collaborator

@copilot Review all comments

Copilot AI and others added 2 commits May 18, 2026 03:10
- pr_review_buffer.cjs: include previous_filename for renamed files in path validation set
- pr_review_buffer.cjs: fail-open when pagination cap (10 pages) is reached with a full last page
- safe_outputs_handlers.cjs: validate event against APPROVE/REQUEST_CHANGES/COMMENT before appending
- safe_outputs_handlers.cjs: increment inlineReviewCommentCount only after successful defaultHandler call
- safe_output_handler_manager.cjs: extract rollback logic into exported rollbackReviewResults() helper
- safe_outputs_tools_loader.test.cjs: add tests for create_pull_request_review_comment and submit_pull_request_review handler mappings
- safe_output_handler_manager.test.cjs: add 6 tests for rollbackReviewResults covering all branches
- safe_outputs_handlers.test.cjs: add tests for event validation and counter-not-incremented-on-throw
- pr_review_buffer.test.cjs: add tests for previous_filename support and pagination-cap fail-open

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 18, 2026

@copilot Review all comments

All six review comments addressed in commits fix: address all review feedback and fix: clarify comments:

  1. pr_review_buffer.cjs:332 — renamed files (previous_filename): changedPaths now also adds f.previous_filename when present, so review comments targeting the old path of a renamed file are no longer filtered out.

  2. pr_review_buffer.cjs:319-323 — pagination cap fail-open: Added hitPageCap = listPage > MAX_LIST_FILES_PAGES. Since partial pages always break before incrementing listPage, this flag is only true when the last page was exactly full (implying more files exist). When set, filtering is skipped entirely. Comment clarified to explain why the check is equivalent to "last page was full".

  3. safe_outputs_tools_loader.test.cjs:83-84 — missing handler-mapping tests: Added two attachHandlers tests covering create_pull_request_review_comment and submit_pull_request_review to prevent silent fallback to the default handler.

  4. safe_outputs_handlers.cjs:1177-1178 — event not validated: submitPullRequestReviewHandler now checks the normalised event against ['APPROVE', 'REQUEST_CHANGES', 'COMMENT'] and throws MCP -32602 for anything else (e.g. "COMMENTT", "APPROVE "). Tests added.

  5. safe_outputs_handlers.cjs:1150-1158 — counter incremented before validation: Moved inlineReviewCommentCount++ to after defaultHandler(...) returns; if it throws for any reason the counter is not advanced. Test added that makes the mock append throw and verifies the subsequent empty-review guard still fires.

  6. safe_output_handler_manager.cjs:1362-1366 — no test coverage for rollback: Extracted the rollback loop into an exported rollbackReviewResults(results, errorMessage) helper and added 6 unit tests covering: flip on submit_pull_request_review, flip on create_pull_request_review_comment, already-failed left unchanged, unrelated types left unchanged, mixed types, empty array.

@pelikhan pelikhan merged commit b644d66 into main May 18, 2026
11 of 12 checks passed
@pelikhan pelikhan deleted the copilot/deep-report-prevent-silent-422 branch May 18, 2026 03:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[deep-report] Prevent silent 422 in safe-output PR review submission (11 silent failures in 24h)

3 participants