Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
82 changes: 61 additions & 21 deletions actions/setup/js/create_pr_review_comment.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,13 @@ const HANDLER_TYPE = "create_pull_request_review_comment";
/**
* Main handler factory for create_pull_request_review_comment
* Returns a message handler function that validates and buffers individual review comments.
* Comments are buffered in the PR review buffer (passed via config._prReviewBuffer) and
* submitted as a single PR review after all messages have been processed.
* Comments are buffered in a PR review buffer and submitted as a single PR review after
* all messages have been processed.
*
* Supports two buffer modes:
* - Registry mode (config._prReviewBufferRegistry): per-PR buffers managed by a registry.
* Each distinct (repo, PR) pair gets its own independent buffer.
* - Legacy mode (config._prReviewBuffer): a single shared buffer (backward compat).
*
* @type {HandlerFactoryFunction}
*/
Expand All @@ -29,7 +34,8 @@ async function main(config = {}) {
const defaultSide = config.side || "RIGHT";
const commentTarget = config.target || "triggering";
const maxCount = config.max || 10;
const buffer = config._prReviewBuffer;
const registry = config._prReviewBufferRegistry || null;
const legacyBuffer = registry ? null : config._prReviewBuffer || null;
const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config);
const githubClient = await createAuthenticatedGitHubClient(config);
const requiredLabels = Array.isArray(config.required_labels) ? config.required_labels : [];
Expand All @@ -43,7 +49,7 @@ async function main(config = {}) {
allowedMentionAliases = await resolveAllowedMentionsFromPayload(context, githubClient, core, config.mentions);
}

if (!buffer) {
if (!registry && !legacyBuffer) {
core.warning("create_pull_request_review_comment: No PR review buffer provided in config");
return async function handleCreatePRReviewComment() {
return { success: false, error: "No PR review buffer available" };
Expand All @@ -58,37 +64,48 @@ async function main(config = {}) {
core.info(`Allowed repos: ${Array.from(allowedRepos).join(", ")}`);
}

// Propagate per-handler staged flag to the shared PR review buffer
// Propagate per-handler staged flag to the PR review buffer
if (isTemplatableTrue(config.staged)) {
buffer.setStaged(true);
if (registry) registry.setDefaultStaged(true);
else legacyBuffer.setStaged(true);
}
if (isStagedMode(config)) {
logStagedPreviewInfo("PR review comments will be previewed without being submitted");
}

// Track how many items we've processed for max limit
let processedCount = 0;

// Extract triggering context for footer generation
const triggeringIssueNumber = context.payload?.issue?.number && !context.payload?.issue?.pull_request ? context.payload.issue.number : undefined;
const triggeringPRNumber = context.payload?.pull_request?.number || (context.payload?.issue?.pull_request ? context.payload.issue.number : undefined);
const triggeringDiscussionNumber = context.payload?.discussion?.number;

// Set footer context once for the review buffer
const workflowName = process.env.GH_AW_WORKFLOW_NAME || "Workflow";
const workflowSource = process.env.GH_AW_WORKFLOW_SOURCE || "";
const workflowSourceURL = process.env.GH_AW_WORKFLOW_SOURCE_URL || "";
const runUrl = buildWorkflowRunUrl(context, context.repo);

buffer.setFooterContext({
// Build the shared footer context object used by both modes.
const footerCtx = {
workflowName,
runUrl,
workflowSource,
workflowSourceURL,
triggeringIssueNumber,
triggeringPRNumber,
triggeringDiscussionNumber,
});
};

// For legacy single-buffer mode, set footer context once at init (unchanged behavior).
// For registry mode, set the registry default so that buffers created via
// submit_pull_request_review (without a create_pull_request_review_comment call) also
// receive the correct footer context when getOrCreate() initialises them.
if (legacyBuffer) {
legacyBuffer.setFooterContext(footerCtx);
} else if (registry) {
registry.setDefaultFooterContext(footerCtx);
}

// Track how many items we've processed for max limit
let processedCount = 0;

/**
* Message handler function that validates and buffers a single create_pull_request_review_comment message
Expand Down Expand Up @@ -287,15 +304,38 @@ async function main(config = {}) {
};
}

// Set the review context (first comment sets it)
// Reject comments targeting a different repo/PR than the first comment
const existingCtx = buffer.getReviewContext();
if (existingCtx && (existingCtx.repo !== itemRepo || existingCtx.pullRequestNumber !== pullRequestNumber)) {
core.warning(`Skipping review comment: targets ${itemRepo}#${pullRequestNumber} but buffer is bound to ${existingCtx.repo}#${existingCtx.pullRequestNumber}. ` + "All review comments in a single review must target the same PR.");
return {
success: false,
error: `Review comments must target the same PR (buffer is bound to ${existingCtx.repo}#${existingCtx.pullRequestNumber})`,
};
// Obtain the buffer for this PR.
// In registry mode: get or create a per-PR buffer (no cross-PR check needed).
// In legacy mode: use the single shared buffer with cross-PR rejection.
let buffer;
if (registry) {
buffer = registry.getOrCreate(itemRepo, pullRequestNumber);
if (!buffer) {
return { success: false, error: `Could not get review buffer for ${itemRepo}#${pullRequestNumber}` };
}
// Apply footer context to this buffer. setFooterContext() is first-wins internally,
// so calling it on every message for the same PR is safe and no-ops after the first call.
buffer.setFooterContext({
workflowName,
runUrl,
workflowSource,
workflowSourceURL,
triggeringIssueNumber,
triggeringPRNumber,
triggeringDiscussionNumber,
});
} else {
buffer = legacyBuffer;
// Set the review context (first comment sets it)
// Reject comments targeting a different repo/PR than the first comment
const existingCtx = buffer.getReviewContext();
if (existingCtx && (existingCtx.repo !== itemRepo || existingCtx.pullRequestNumber !== pullRequestNumber)) {
core.warning(`Skipping review comment: targets ${itemRepo}#${pullRequestNumber} but buffer is bound to ${existingCtx.repo}#${existingCtx.pullRequestNumber}. ` + "All review comments in a single review must target the same PR.");
return {
success: false,
error: `Review comments must target the same PR (buffer is bound to ${existingCtx.repo}#${existingCtx.pullRequestNumber})`,
};
}
}

buffer.setReviewContext({
Expand Down
99 changes: 98 additions & 1 deletion actions/setup/js/create_pr_review_comment.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ global.core = mockCore;
global.github = mockGithub;
global.context = mockContext;

const { createReviewBuffer } = require("./pr_review_buffer.cjs");
const { createReviewBuffer, createPrReviewBufferRegistry } = require("./pr_review_buffer.cjs");

describe("create_pr_review_comment.cjs", () => {
let createPRReviewCommentScript;
Expand Down Expand Up @@ -524,3 +524,100 @@ describe("create_pr_review_comment.cjs", () => {
expect(buffer.getBufferedCount()).toBe(1);
});
});

describe("create_pr_review_comment.cjs — registry mode (multiple reviews)", () => {
let createPRReviewCommentScript;

beforeEach(() => {
vi.clearAllMocks();

const scriptPath = require("path").join(__dirname, "create_pr_review_comment.cjs");
createPRReviewCommentScript = require("fs").readFileSync(scriptPath, "utf8");

delete process.env.GH_AW_AGENT_OUTPUT;
delete process.env.GH_AW_PR_REVIEW_COMMENT_SIDE;
delete process.env.GH_AW_PR_REVIEW_COMMENT_TARGET;
delete process.env.GH_AW_WORKFLOW_NAME;
delete process.env.GH_AW_WORKFLOW_SOURCE;
delete process.env.GH_AW_WORKFLOW_SOURCE_URL;

mockGithub.rest.pulls.get.mockImplementation(({ pull_number }) => Promise.resolve({ data: { number: pull_number, head: { sha: `sha-pr${pull_number}` } } }));

global.context = {
eventName: "pull_request",
runId: 12345,
repo: { owner: "testowner", repo: "testrepo" },
payload: {
pull_request: { number: 123, head: { sha: "abc123" } },
repository: { html_url: "https://github.com/testowner/testrepo" },
},
};
});

afterEach(() => {
global.context = mockContext;
});

async function createRegistryHandler(registry, extraConfig = {}) {
const configStr = JSON.stringify(extraConfig);
return await eval(`(async () => { ${createPRReviewCommentScript}; return await main(Object.assign({ _prReviewBufferRegistry: registry, target: "*" }, ${configStr})); })()`);
}

it("routes comments for two different PRs into separate buffers", async () => {
const registry = createPrReviewBufferRegistry();
const handler = await createRegistryHandler(registry);

const result1 = await handler({ type: "create_pull_request_review_comment", path: "a.js", line: 1, body: "comment on PR 1", pull_request_number: 1 }, {});
const result2 = await handler({ type: "create_pull_request_review_comment", path: "b.js", line: 2, body: "comment on PR 2", pull_request_number: 2 }, {});

expect(result1.success).toBe(true);
expect(result1.buffered).toBe(true);
expect(result2.success).toBe(true);
expect(result2.buffered).toBe(true);

const entries = registry.getAllEntries();
expect(entries).toHaveLength(2);
expect(entries[0].prNumber).toBe(1);
expect(entries[0].buffer.getBufferedCount()).toBe(1);
expect(entries[1].prNumber).toBe(2);
expect(entries[1].buffer.getBufferedCount()).toBe(1);
});

it("accumulates multiple comments for the same PR in one buffer", async () => {
const registry = createPrReviewBufferRegistry();
const handler = await createRegistryHandler(registry);

await handler({ type: "create_pull_request_review_comment", path: "a.js", line: 1, body: "first", pull_request_number: 5 }, {});
await handler({ type: "create_pull_request_review_comment", path: "b.js", line: 2, body: "second", pull_request_number: 5 }, {});

const entries = registry.getAllEntries();
expect(entries).toHaveLength(1);
expect(entries[0].prNumber).toBe(5);
expect(entries[0].buffer.getBufferedCount()).toBe(2);
});

it("sets footer context on the per-PR buffer", async () => {
process.env.GH_AW_WORKFLOW_NAME = "My Workflow";
const registry = createPrReviewBufferRegistry();
const handler = await createRegistryHandler(registry);

await handler({ type: "create_pull_request_review_comment", path: "c.js", line: 3, body: "body", pull_request_number: 10 }, {});

const entry = registry.getAllEntries()[0];
const ctx = entry.buffer.getFooterContext?.();
if (ctx !== undefined) {
expect(ctx.workflowName).toBe("My Workflow");
}
});

it("returns success:false when pull_request_number is missing in target:* mode", async () => {
const registry = createPrReviewBufferRegistry();
const handler = await createRegistryHandler(registry);

const result = await handler({ type: "create_pull_request_review_comment", path: "x.js", line: 1, body: "no PR" }, {});

expect(result.success).toBe(false);
expect(result.error).toContain("pull_request_number");
expect(registry.getAllEntries()).toHaveLength(0);
});
});
105 changes: 104 additions & 1 deletion actions/setup/js/pr_review_buffer.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -721,7 +721,110 @@ function createReviewBuffer() {
};
}

module.exports = { createReviewBuffer };
/**
* Create a registry that manages per-PR review buffers.
* Each distinct (repo, prNumber) pair gets its own independent buffer instance.
*
* Default settings applied to every newly created buffer (footerMode, footerContext,
* staged, supersedeOlderReviews) can be configured via the returned setters before
* any messages are processed.
*
* @returns {Object} Registry with getOrCreate, getAllEntries, hasAnyContent, and config setters
*/
function createPrReviewBufferRegistry() {
/** @type {Map<string, Object>} */
const bufferMap = new Map();

/** @type {{repo: string, prNumber: number, buffer: Object}[]} */
const insertionOrder = [];

// Defaults applied to each new buffer when it is first created.
/** @type {string|boolean} */
let defaultFooterMode = "always";
/** @type {Object | null} */
let defaultFooterContext = null;
let defaultStaged = false;
let defaultSupersedeOlderReviews = false;

/**
* Get or create the buffer for the given (repo, prNumber) pair.
* Returns null when repo or prNumber are falsy (unresolvable target).
* @param {string | null} repo - Repository slug (owner/repo)
* @param {number | null} prNumber - Pull request number
* @returns {Object | null} Buffer for this PR, or null if target cannot be resolved
*/
function getOrCreate(repo, prNumber) {
if (!repo || !prNumber) {
return null;
}
const k = `${repo}#${prNumber}`;
if (!bufferMap.has(k)) {
const buffer = createReviewBuffer();
buffer.setFooterMode(defaultFooterMode);
if (defaultFooterContext) {
buffer.setFooterContext(defaultFooterContext);
}
if (defaultStaged) {
buffer.setStaged(true);
}
if (defaultSupersedeOlderReviews) {
buffer.setSupersedeOlderReviews(true);
}
bufferMap.set(k, buffer);
insertionOrder.push({ repo, prNumber, buffer });
core.info(`PR review registry: created buffer for ${repo}#${prNumber}`);
}
return bufferMap.get(k);
}

/**
* Return all buffered entries in insertion order.
* @returns {{repo: string, prNumber: number, buffer: Object}[]}
*/
function getAllEntries() {
return insertionOrder;
}

/**
* Returns true if any buffer has buffered comments or review metadata.
* @returns {boolean}
*/
function hasAnyContent() {
return insertionOrder.some(e => e.buffer.hasBufferedComments() || e.buffer.hasReviewMetadata());
}

/** @param {string|boolean} value */
function setDefaultFooterMode(value) {
defaultFooterMode = value;
}

/** @param {Object} ctx */
function setDefaultFooterContext(ctx) {
defaultFooterContext = ctx;
}

/** @param {boolean} value */
function setDefaultStaged(value) {
defaultStaged = value === true;
}

/** @param {boolean} value */
function setDefaultSupersedeOlderReviews(value) {
defaultSupersedeOlderReviews = value === true;
}

return {
getOrCreate,
getAllEntries,
hasAnyContent,
setDefaultFooterMode,
setDefaultFooterContext,
setDefaultStaged,
setDefaultSupersedeOlderReviews,
};
}

module.exports = { createReviewBuffer, createPrReviewBufferRegistry };
/**
* Append a fallback section that preserves inline comment content when comments cannot be anchored.
* @param {string} reviewBody
Expand Down
Loading
Loading