Fix inline review comments on centralized reviewer reruns #52960
+123
−14
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -463,6 +463,99 @@ describe("create_pr_review_comment.cjs", () => { | |
| expect(buffer.getBufferedCount()).toBe(1); | ||
| }); | ||
|
|
||
| it("should use the original PR context for centralized slash-command dispatches", async () => { | ||
| global.context = { | ||
| eventName: "workflow_dispatch", | ||
| runId: 12345, | ||
| repo: { owner: "testowner", repo: "testrepo" }, | ||
| payload: { | ||
| inputs: { | ||
| event_name: "issue_comment", | ||
| event_payload: JSON.stringify({ | ||
| issue: { number: 456, pull_request: {} }, | ||
| repository: mockContext.payload.repository, | ||
| }), | ||
| }, | ||
| }, | ||
| }; | ||
| mockGithub.rest.pulls.get.mockResolvedValue({ | ||
| data: { number: 456, head: { sha: "dispatch123abc" } }, | ||
| }); | ||
| const handler = await createHandler({ target: "triggering" }); | ||
| const message = { | ||
| type: "create_pull_request_review_comment", | ||
| pull_request_number: 456, | ||
| path: "src/main.js", | ||
| line: 5, | ||
| body: "Review comment from centralized slash command", | ||
| }; | ||
|
|
||
| const result = await handler(message, {}); | ||
|
|
||
| expect(result.success).toBe(true); | ||
| expect(result.buffered).toBe(true); | ||
| expect(result.pull_request_number).toBe(456); | ||
| expect(mockGithub.rest.pulls.get).toHaveBeenCalledWith({ | ||
| owner: "testowner", | ||
| repo: "testrepo", | ||
| pull_number: 456, | ||
| }); | ||
| expect(buffer.getBufferedCount()).toBe(1); | ||
| }); | ||
|
|
||
|
Contributor
There was a problem hiding this comment. [/tdd] The new test covers the 💡 Suggested additional test skeletonit("falls back to raw context when invocationContext is empty", async () => {
// resolveInvocationContext returns no overrides
jest.mock("./invocation_context_helpers.cjs", () => ({
resolveInvocationContext: () => ({}),
}));
global.context = mockContext; // standard PR trigger
const handler = await createHandler({ target: "triggering" });
const result = await handler(prReviewMessage, {});
expect(result.success).toBe(true);
});@copilot please address this.
Contributor
Author
There was a problem hiding this comment. Addressed in ec00158 by adding regression coverage for fallback behavior when invocation context resolution yields empty/partial context (raw context fallback path is now explicitly tested). |
||
| it("falls back to raw context when resolveInvocationContext returns empty object", async () => { | ||
| const invocationHelpersPath = path.join(__dirname, "invocation_context_helpers.cjs"); | ||
| const invocationHelpers = require(invocationHelpersPath); | ||
| const resolveInvocationContextSpy = vi.spyOn(invocationHelpers, "resolveInvocationContext").mockReturnValue({}); | ||
| try { | ||
| const handler = await createHandler({ target: "triggering" }); | ||
| const message = { | ||
| type: "create_pull_request_review_comment", | ||
| path: "src/main.js", | ||
| line: 5, | ||
| body: "Review comment from fallback context", | ||
| }; | ||
|
|
||
| const result = await handler(message, {}); | ||
|
|
||
| expect(result.success).toBe(true); | ||
| expect(result.buffered).toBe(true); | ||
| expect(result.pull_request_number).toBe(123); | ||
| expect(buffer.getBufferedCount()).toBe(1); | ||
| expect(resolveInvocationContextSpy).toHaveBeenCalled(); | ||
| } finally { | ||
| resolveInvocationContextSpy.mockRestore(); | ||
| } | ||
| }); | ||
|
|
||
| it("falls back to raw context when resolveInvocationContext throws a non-validation error", async () => { | ||
| const invocationHelpersPath = path.join(__dirname, "invocation_context_helpers.cjs"); | ||
| const invocationHelpers = require(invocationHelpersPath); | ||
| const resolveInvocationContextSpy = vi.spyOn(invocationHelpers, "resolveInvocationContext").mockImplementation(() => { | ||
| throw new Error("boom"); | ||
| }); | ||
| try { | ||
| const handler = await createHandler({ target: "triggering" }); | ||
| const message = { | ||
| type: "create_pull_request_review_comment", | ||
| path: "src/main.js", | ||
| line: 5, | ||
| body: "Review comment from thrown context resolver", | ||
| }; | ||
|
|
||
| const result = await handler(message, {}); | ||
|
|
||
| expect(result.success).toBe(true); | ||
| expect(result.buffered).toBe(true); | ||
| expect(result.pull_request_number).toBe(123); | ||
| expect(buffer.getBufferedCount()).toBe(1); | ||
| expect(resolveInvocationContextSpy).toHaveBeenCalled(); | ||
| expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("failed to resolve invocation context")); | ||
| } finally { | ||
| resolveInvocationContextSpy.mockRestore(); | ||
| } | ||
| }); | ||
|
|
||
| it("should reject comments targeting a different PR than the first comment", async () => { | ||
| // First comment sets context to PR #123 | ||
| const handler = await createHandler(); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/diagnosing-bugs]
effectivePayloadis derived once at startup (line 41) before the per-message loop, but the invocation context is static anyway so this is fine. However, ifresolveInvocationContextever throws (e.g. malformedevent_payloadJSON), the entiremain()will reject before processing any message. Consider wrapping the call in a try/catch and falling back to raw context, so a bad dispatch input degrades gracefully rather than taking down the whole handler.💡 Defensive pattern
@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in ec00158 by adding defensive handling around invocation-context resolution: non-validation failures now fall back to raw context, while ERR_VALIDATION failures are still re-thrown.