Skip to content

Fix inline review comments on centralized reviewer reruns #52960

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 3 commits into from
Aug 15, 2026
Merged

Fix inline review comments on centralized reviewer reruns #52960

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cb38db1
Fix centralized PR review comments
Copilot Aug 15, 2026
38bab4f
Merge branch 'main' into copilot/investigate-matt-review-comments
github-actions[bot] Aug 15, 2026
ec00158
Handle invocation context fallback for PR review comments
Copilot Aug 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Handle invocation context fallback for PR review comments 
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
  • Loading branch information
@pelikhan
Copilot and pelikhan authored Aug 15, 2026
commit ec00158ddf32a318600f43d3767d2482a44a477f
14 changes: 13 additions & 1 deletion actions/setup/js/create_pr_review_comment.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ const { buildWorkflowRunUrl } = require("./workflow_metadata_helpers.cjs");
const { isTemplatableTrue, isStagedMode, logStagedPreviewInfo, checkRequiredFilter } = require("./safe_output_helpers.cjs");
const { resolveAllowedMentionsFromPayload } = require("./resolve_mentions_from_payload.cjs");
const { resolveInvocationContext } = require("./invocation_context_helpers.cjs");
const { ERR_VALIDATION } = require("./error_codes.cjs");

/** @type {string} Safe output type handled by this module */
const HANDLER_TYPE = "create_pull_request_review_comment";
Expand All @@ -39,7 +40,18 @@ async function main(config = {}) {
const legacyBuffer = registry ? null : config._prReviewBuffer || null;
const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config);
const githubClient = await createAuthenticatedGitHubClient(config);

@github-actions github-actions Bot Aug 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnosing-bugs] effectivePayload is derived once at startup (line 41) before the per-message loop, but the invocation context is static anyway so this is fine. However, if resolveInvocationContext ever throws (e.g. malformed event_payload JSON), the entire main() 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 
let invocationContext = {};
try {
  invocationContext = resolveInvocationContext(context);
} catch (e) {
  core.warning(`resolveInvocationContext failed, using raw context: ${e.message}`);
}
const effectiveEventName = invocationContext.eventName || context.eventName;
const effectivePayload   = invocationContext.eventPayload || context.payload;

@copilot please address this.

Copilot AI Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

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.

const invocationContext = resolveInvocationContext(context);
let invocationContext = {};
try {
invocationContext = resolveInvocationContext(context);
} catch (error) {
const typedError = /** @type {Error & { code?: string }} */ (error);
const isValidationError = typedError.code === ERR_VALIDATION;
if (isValidationError) {
throw error;
}
const errorMessage = getErrorMessage(error);
core.warning(`create_pull_request_review_comment: failed to resolve invocation context, using raw context: ${errorMessage}`);
}
const effectiveEventName = invocationContext.eventName || context.eventName;
const effectivePayload = invocationContext.eventPayload || context.payload;
const requiredLabels = Array.isArray(config.required_labels) ? config.required_labels : [];
Expand Down
53 changes: 53 additions & 0 deletions actions/setup/js/create_pr_review_comment.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,6 +503,59 @@ describe("create_pr_review_comment.cjs", () => {
expect(buffer.getBufferedCount()).toBe(1);
});

@github-actions github-actions Bot Aug 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The new test covers the workflow_dispatch happy path but not the fallback: if resolveInvocationContext returns {}, the code should fall back to raw context.eventName/context.payload. A test for that case would pin the || guard behaviour and prevent a silent regression if invocation_context_helpers changes its return shape.

💡 Suggested additional test skeleton 
it("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.

Copilot AI Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

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 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();
Expand Down
2 changes: 1 addition & 1 deletion actions/setup/js/invocation_context_helpers.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ function checkAllowedRepo(workflowRepo, targetRepo) {

const validation = validateTargetRepo(targetRepoSlug, defaultRepo, allowedRepos);
if (!validation.valid) {
throw new Error(`${ERR_VALIDATION}: ${validation.error}`);
throw Object.assign(new Error(`${ERR_VALIDATION}: ${validation.error}`), { code: ERR_VALIDATION });
}
}

Expand Down
Loading