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

actions/setup/js/pr_review_buffer.cjs

@@ -284,7 +284,7 @@ function createReviewBuffer() {
     }
 
     // Build comments array for the API
-    const comments = bufferedComments.map(comment => {
+    let comments = bufferedComments.map(comment => {
       /** @type {any} */
       const apiComment = {
         path: comment.path,
@@ -310,6 +310,68 @@ function createReviewBuffer() {
       return apiComment;
     });
 
+    // Sub-pattern B: Validate comment paths against the PR diff before POSTing.
+    // Comments targeting paths not in the diff cause GitHub to return 422 "Path could not be resolved".
+    if (comments.length > 0) {
+      try {
+        const changedPaths = new Set();
+        let listPage = 1;
+        // 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) {
+          const { data: files } = await github.rest.pulls.listFiles({
+            owner: repoParts.owner,
+            repo: repoParts.repo,
+            pull_number: pullRequestNumber,
+            per_page: 100,
+            page: listPage,
+          });
+          if (!Array.isArray(files) || files.length === 0) break;
+          for (const f of files) {
+            changedPaths.add(f.filename);
+            // For renamed files, the old path (previous_filename) is also valid for review comments.
+            if (f.previous_filename) changedPaths.add(f.previous_filename);
+          }
+          if (files.length < 100) break;
+          listPage++;
+        }
+        // `listPage > MAX_LIST_FILES_PAGES` is only true when the loop exited via the
+        // while-condition (not via a break), which only happens after a full page of 100
+        // files caused listPage to be incremented past the cap. A partial page always
+        // triggers the `files.length < 100` break first, so hitPageCap implies the last
+        // page was full and there may be more files beyond the 1,000-file limit.
+        // Fail-open in that case: the collected set is non-authoritative and filtering
+        // would risk dropping valid comments on the un-fetched files.
+        const hitPageCap = listPage > MAX_LIST_FILES_PAGES;
+        // Only filter when we received a non-empty file list and did not hit the cap;
+        // an empty list likely indicates an API quirk or a PR with no diff.
+        if (changedPaths.size > 0 && !hitPageCap) {
+          const invalidComments = comments.filter(c => !changedPaths.has(c.path));
+          if (invalidComments.length > 0) {
+            for (const c of invalidComments) {
+              core.warning(`Skipping review comment at '${c.path}:${c.line}' — path not found in PR #${pullRequestNumber} diff`);
+            }
+            comments = comments.filter(c => changedPaths.has(c.path));
+          }
+        }
+      } catch (pathValidationError) {
+        core.warning(`Failed to validate comment paths against PR diff: ${getErrorMessage(pathValidationError)}. Proceeding without path validation.`);
+      }
+    }
+
+    // Sub-pattern A: Guard against empty review submission (no body and no inline comments).
+    // GitHub returns 422 "Unprocessable Entity" when both are absent.
+    if (comments.length === 0 && !body) {
+      const errorMsg =
+        "Empty review: review body is empty and no inline comments are present" +
+        (bufferedComments.length > 0 ? " (all comment paths were outside the PR diff)" : "") +
+        ". Skipping POST to avoid 422.";
+      core.warning(errorMsg);
+      return { success: false, error: errorMsg };
+    }
+
     core.info(`Submitting PR review on ${repo}#${pullRequestNumber}: event=${event}, comments=${comments.length}, bodyLength=${body.length}`);
 
     // If in staged mode, preview the review without submitting

actions/setup/js/pr_review_buffer.test.cjs

@@ -13,6 +13,7 @@ const mockGithub = {
   rest: {
     pulls: {
       createReview: vi.fn(),
+      listFiles: vi.fn(),
       listReviews: vi.fn(),
       dismissReview: vi.fn(),
     },
@@ -35,6 +36,9 @@ describe("pr_review_buffer (factory pattern)", () => {
     originalMessages = process.env.GH_AW_SAFE_OUTPUT_MESSAGES;
     delete process.env.GH_AW_SAFE_OUTPUT_MESSAGES;
 
+    // Default: return empty file list so path filtering is skipped unless explicitly mocked
+    mockGithub.rest.pulls.listFiles.mockResolvedValue({ data: [] });
+
     // Create a fresh buffer instance for each test (no shared global state)
     buffer = createReviewBuffer();
   });
@@ -837,7 +841,289 @@ describe("pr_review_buffer (factory pattern)", () => {
       const callArgs = mockGithub.rest.pulls.createReview.mock.calls[0][0];
       expect(callArgs.comments).toHaveLength(3);
     });
-  });
+
+    describe("Sub-pattern A: empty review guard", () => {
+      it("should return failure when review metadata has empty body and no comments", async () => {
+        buffer.setReviewMetadata("", "COMMENT");
+        buffer.setReviewContext({
+          repo: "owner/repo",
+          repoParts: { owner: "owner", repo: "repo" },
+          pullRequestNumber: 42,
+          pullRequest: { head: { sha: "abc123" } },
+        });
+
+        const result = await buffer.submitReview();
+
+        expect(result.success).toBe(false);
+        expect(result.error).toContain("Empty review");
+        expect(result.error).toContain("Skipping POST to avoid 422");
+        expect(mockGithub.rest.pulls.createReview).not.toHaveBeenCalled();
+        expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Empty review"));
+      });
+
+      it("should NOT block review when body is whitespace-only (truthy string passes guard)", async () => {
+        buffer.setReviewMetadata("   \n  ", "COMMENT");
+        buffer.setReviewContext({
+          repo: "owner/repo",
+          repoParts: { owner: "owner", repo: "repo" },
+          pullRequestNumber: 42,
+          pullRequest: { head: { sha: "abc123" } },
+        });
+        // Whitespace body is truthy so guard should NOT trigger; POST proceeds.
+
+        mockGithub.rest.pulls.createReview.mockResolvedValue({
+          data: { id: 999, html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-999" },
+        });
+
+        const result = await buffer.submitReview();
+
+        // Whitespace body is truthy so should still POST (GitHub may accept or reject it)
+        expect(result.success).toBe(true);
+        expect(mockGithub.rest.pulls.createReview).toHaveBeenCalledTimes(1);
+      });
+
+      it("should return failure when metadata has no body and footerContext is null (body stays empty)", async () => {
+        // Simulate Sub-pattern A from the issue: event=COMMENT, comments=0, bodyLength=0
+        buffer.setReviewMetadata("", "COMMENT");
+        buffer.setReviewContext({
+          repo: "owner/repo",
+          repoParts: { owner: "owner", repo: "repo" },
+          pullRequestNumber: 42,
+          pullRequest: { head: { sha: "abc123" } },
+        });
+        // footerContext is NOT set → no footer added → body remains ""
+
+        const result = await buffer.submitReview();
+
+        expect(result.success).toBe(false);
+        expect(result.error).toContain("Empty review");
+        expect(mockGithub.rest.pulls.createReview).not.toHaveBeenCalled();
+      });
+
+      it("should NOT guard when body is empty but comments are present", async () => {
+        buffer.addComment({ path: "src/main.js", line: 5, body: "Missing null check" });
+        buffer.setReviewMetadata("", "COMMENT");
+        buffer.setReviewContext({
+          repo: "owner/repo",
+          repoParts: { owner: "owner", repo: "repo" },
+          pullRequestNumber: 42,
+          pullRequest: { head: { sha: "abc123" } },
+        });
+
+        mockGithub.rest.pulls.createReview.mockResolvedValue({
+          data: { id: 701, html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-701" },
+        });
+
+        const result = await buffer.submitReview();
+
+        expect(result.success).toBe(true);
+        expect(mockGithub.rest.pulls.createReview).toHaveBeenCalledTimes(1);
+      });
+
+      it("should NOT guard when body is non-empty but no comments are present", async () => {
+        buffer.setReviewMetadata("LGTM! Ship it.", "APPROVE");
+        buffer.setReviewContext({
+          repo: "owner/repo",
+          repoParts: { owner: "owner", repo: "repo" },
+          pullRequestNumber: 42,
+          pullRequest: { head: { sha: "abc123" } },
+        });
+
+        mockGithub.rest.pulls.createReview.mockResolvedValue({
+          data: { id: 702, html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-702" },
+        });
+
+        const result = await buffer.submitReview();
+
+        expect(result.success).toBe(true);
+        expect(mockGithub.rest.pulls.createReview).toHaveBeenCalledTimes(1);
+      });
+    });
+
+    describe("Sub-pattern B: path validation against PR diff", () => {
+      it("should filter out comments at paths not in the PR diff", async () => {
+        buffer.addComment({ path: "src/valid.js", line: 10, body: "Valid comment" });
+        buffer.addComment({ path: "src/not-in-diff.js", line: 5, body: "Invalid path" });
+        buffer.setReviewMetadata("Code review", "COMMENT");
+        buffer.setReviewContext({
+          repo: "owner/repo",
+          repoParts: { owner: "owner", repo: "repo" },
+          pullRequestNumber: 42,
+          pullRequest: { head: { sha: "abc123" } },
+        });
+
+        mockGithub.rest.pulls.listFiles.mockResolvedValue({
+          data: [{ filename: "src/valid.js" }, { filename: "README.md" }],
+        });
+        mockGithub.rest.pulls.createReview.mockResolvedValue({
+          data: { id: 800, html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-800" },
+        });
+
+        const result = await buffer.submitReview();
+
+        expect(result.success).toBe(true);
+        expect(result.comment_count).toBe(1);
+        const callArgs = mockGithub.rest.pulls.createReview.mock.calls[0][0];
+        expect(callArgs.comments).toHaveLength(1);
+        expect(callArgs.comments[0].path).toBe("src/valid.js");
+        expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("src/not-in-diff.js"));
+        expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("path not found in PR"));
+      });
+
+      it("should return failure when all comment paths are outside the PR diff and body is empty", async () => {
+        buffer.addComment({ path: "unrelated/file.js", line: 1, body: "This won't post" });
+        buffer.setReviewMetadata("", "COMMENT");
+        buffer.setReviewContext({
+          repo: "owner/repo",
+          repoParts: { owner: "owner", repo: "repo" },
+          pullRequestNumber: 42,
+          pullRequest: { head: { sha: "abc123" } },
+        });
+
+        mockGithub.rest.pulls.listFiles.mockResolvedValue({
+          data: [{ filename: "src/main.js" }],
+        });
+
+        const result = await buffer.submitReview();
+
+        expect(result.success).toBe(false);
+        expect(result.error).toContain("Empty review");
+        expect(result.error).toContain("all comment paths were outside the PR diff");
+        expect(mockGithub.rest.pulls.createReview).not.toHaveBeenCalled();
+        expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("unrelated/file.js"));
+      });
+
+      it("should proceed without filtering when listFiles returns an empty array", async () => {
+        buffer.addComment({ path: "any/path.js", line: 1, body: "Comment" });
+        buffer.setReviewMetadata("Review", "COMMENT");
+        buffer.setReviewContext({
+          repo: "owner/repo",
+          repoParts: { owner: "owner", repo: "repo" },
+          pullRequestNumber: 42,
+          pullRequest: { head: { sha: "abc123" } },
+        });
+
+        // Default mock: listFiles returns { data: [] } → changedPaths.size === 0 → no filtering
+        mockGithub.rest.pulls.createReview.mockResolvedValue({
+          data: { id: 801, html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-801" },
+        });
+
+        const result = await buffer.submitReview();
+
+        expect(result.success).toBe(true);
+        expect(result.comment_count).toBe(1);
+        // No warnings about path filtering
+        expect(mockCore.warning).not.toHaveBeenCalledWith(expect.stringContaining("path not found in PR"));
+      });
+
+      it("should proceed without filtering when listFiles API call fails", async () => {
+        buffer.addComment({ path: "any/path.js", line: 1, body: "Comment" });
+        buffer.setReviewMetadata("Review", "COMMENT");
+        buffer.setReviewContext({
+          repo: "owner/repo",
+          repoParts: { owner: "owner", repo: "repo" },
+          pullRequestNumber: 42,
+          pullRequest: { head: { sha: "abc123" } },
+        });
+
+        mockGithub.rest.pulls.listFiles.mockRejectedValue(new Error("API rate limit exceeded"));
+        mockGithub.rest.pulls.createReview.mockResolvedValue({
+          data: { id: 802, html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-802" },
+        });
+
+        const result = await buffer.submitReview();
+
+        expect(result.success).toBe(true);
+        expect(result.comment_count).toBe(1);
+        expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Failed to validate comment paths against PR diff"));
+      });
+
+      it("should handle paginated listFiles correctly", async () => {
+        buffer.addComment({ path: "page2/file.js", line: 1, body: "Comment on page 2 file" });
+        buffer.setReviewMetadata("Review", "COMMENT");
+        buffer.setReviewContext({
+          repo: "owner/repo",
+          repoParts: { owner: "owner", repo: "repo" },
+          pullRequestNumber: 42,
+          pullRequest: { head: { sha: "abc123" } },
+        });
+
+        // First page returns 100 files (full page → trigger next page fetch)
+        const page1Files = Array.from({ length: 100 }, (_, i) => ({ filename: `page1/file${i}.js` }));
+        // Second page returns the file we want
+        const page2Files = [{ filename: "page2/file.js" }];
+        mockGithub.rest.pulls.listFiles.mockResolvedValueOnce({ data: page1Files }).mockResolvedValueOnce({ data: page2Files });
+        mockGithub.rest.pulls.createReview.mockResolvedValue({
+          data: { id: 803, html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-803" },
+        });
+
+        const result = await buffer.submitReview();
+
+        expect(result.success).toBe(true);
+        expect(result.comment_count).toBe(1);
+        expect(mockGithub.rest.pulls.listFiles).toHaveBeenCalledTimes(2);
+        // No warning about invalid paths
+        expect(mockCore.warning).not.toHaveBeenCalledWith(expect.stringContaining("path not found in PR"));
+      });
+
+      it("should accept comments targeting a renamed file's previous path", async () => {
+        buffer.addComment({ path: "old/path.js", line: 1, body: "Comment on old path" });
+        buffer.setReviewMetadata("Review", "COMMENT");
+        buffer.setReviewContext({
+          repo: "owner/repo",
+          repoParts: { owner: "owner", repo: "repo" },
+          pullRequestNumber: 42,
+          pullRequest: { head: { sha: "abc123" } },
+        });
+
+        // File was renamed; both filename and previous_filename appear in the API response
+        mockGithub.rest.pulls.listFiles.mockResolvedValue({
+          data: [{ filename: "new/path.js", previous_filename: "old/path.js" }],
+        });
+        mockGithub.rest.pulls.createReview.mockResolvedValue({
+          data: { id: 804, html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-804" },
+        });
+
+        const result = await buffer.submitReview();
+
+        expect(result.success).toBe(true);
+        expect(result.comment_count).toBe(1);
+        // The old path must NOT be flagged as invalid
+        expect(mockCore.warning).not.toHaveBeenCalledWith(expect.stringContaining("path not found in PR"));
+      });
+
+      it("should skip path filtering and keep all comments when the pagination cap is reached with a full last page", async () => {
+        buffer.addComment({ path: "file-beyond-cap.js", line: 1, body: "Comment on file past cap" });
+        buffer.setReviewMetadata("Review", "COMMENT");
+        buffer.setReviewContext({
+          repo: "owner/repo",
+          repoParts: { owner: "owner", repo: "repo" },
+          pullRequestNumber: 42,
+          pullRequest: { head: { sha: "abc123" } },
+        });
+
+        // Simulate 10 full pages of 100 files each — the loop exits because of the cap,
+        // not because the last page was partial.
+        const fullPage = Array.from({ length: 100 }, (_, i) => ({ filename: `page/file${i}.js` }));
+        // All 10 pages return full results
+        for (let i = 0; i < 10; i++) {
+          mockGithub.rest.pulls.listFiles.mockResolvedValueOnce({ data: fullPage });
+        }
+        mockGithub.rest.pulls.createReview.mockResolvedValue({
+          data: { id: 805, html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-805" },
+        });
+
+        const result = await buffer.submitReview();
+
+        // Cap reached with full page → fail-open → no filtering → comment is kept
+        expect(result.success).toBe(true);
+        expect(result.comment_count).toBe(1);
+        expect(mockGithub.rest.pulls.listFiles).toHaveBeenCalledTimes(10);
+        // No "path not found" warning because filtering was skipped
+        expect(mockCore.warning).not.toHaveBeenCalledWith(expect.stringContaining("path not found in PR"));
+      });
+    });
+  }); // closes submitReview describe
 
   describe("reset", () => {
     it("should clear all state including footer mode", () => {