diff --git a/actions/setup/js/create_pr_review_comment.cjs b/actions/setup/js/create_pr_review_comment.cjs index 47c8de87d4d..e33a5f43cbb 100644 --- a/actions/setup/js/create_pr_review_comment.cjs +++ b/actions/setup/js/create_pr_review_comment.cjs @@ -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} */ @@ -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 : []; @@ -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" }; @@ -58,29 +64,27 @@ 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, @@ -88,7 +92,20 @@ async function main(config = {}) { 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 @@ -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({ diff --git a/actions/setup/js/create_pr_review_comment.test.cjs b/actions/setup/js/create_pr_review_comment.test.cjs index f57da8a25fc..e18bb40d1df 100644 --- a/actions/setup/js/create_pr_review_comment.test.cjs +++ b/actions/setup/js/create_pr_review_comment.test.cjs @@ -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; @@ -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); + }); +}); diff --git a/actions/setup/js/pr_review_buffer.cjs b/actions/setup/js/pr_review_buffer.cjs index 4d534e3a222..ac00d563dd1 100644 --- a/actions/setup/js/pr_review_buffer.cjs +++ b/actions/setup/js/pr_review_buffer.cjs @@ -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} */ + 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 diff --git a/actions/setup/js/pr_review_buffer.test.cjs b/actions/setup/js/pr_review_buffer.test.cjs index c2ce4f9e420..fef7d2064ee 100644 --- a/actions/setup/js/pr_review_buffer.test.cjs +++ b/actions/setup/js/pr_review_buffer.test.cjs @@ -26,7 +26,85 @@ const mockGithub = { global.core = mockCore; global.github = mockGithub; -const { createReviewBuffer } = require("./pr_review_buffer.cjs"); +const { createReviewBuffer, createPrReviewBufferRegistry } = require("./pr_review_buffer.cjs"); + +describe("createPrReviewBufferRegistry", () => { + let savedCore; + beforeEach(() => { + savedCore = global.core; + global.core = { + debug: vi.fn(), + info: vi.fn(), + warning: vi.fn(), + error: vi.fn(), + }; + }); + afterEach(() => { + global.core = savedCore; + }); + + it("returns separate buffers for different (repo, prNumber) pairs", () => { + const registry = createPrReviewBufferRegistry(); + const buf1 = registry.getOrCreate("o/r", 1); + const buf2 = registry.getOrCreate("o/r", 2); + const buf3 = registry.getOrCreate("other/repo", 1); + expect(buf1).not.toBe(buf2); + expect(buf1).not.toBe(buf3); + expect(buf2).not.toBe(buf3); + }); + + it("returns the same buffer for repeated calls with the same key", () => { + const registry = createPrReviewBufferRegistry(); + const buf1 = registry.getOrCreate("o/r", 5); + const buf2 = registry.getOrCreate("o/r", 5); + expect(buf1).toBe(buf2); + }); + + it("returns null when repo is falsy", () => { + const registry = createPrReviewBufferRegistry(); + expect(registry.getOrCreate(null, 1)).toBeNull(); + expect(registry.getOrCreate("", 1)).toBeNull(); + }); + + it("returns null when prNumber is falsy", () => { + const registry = createPrReviewBufferRegistry(); + expect(registry.getOrCreate("o/r", null)).toBeNull(); + expect(registry.getOrCreate("o/r", 0)).toBeNull(); + }); + + it("getAllEntries returns entries in insertion order", () => { + const registry = createPrReviewBufferRegistry(); + registry.getOrCreate("o/r", 3); + registry.getOrCreate("o/r", 1); + registry.getOrCreate("o/r", 2); + const entries = registry.getAllEntries(); + expect(entries.map(e => e.prNumber)).toEqual([3, 1, 2]); + }); + + it("hasAnyContent returns false when all buffers are empty", () => { + const registry = createPrReviewBufferRegistry(); + registry.getOrCreate("o/r", 1); + expect(registry.hasAnyContent()).toBe(false); + }); + + it("hasAnyContent returns true when any buffer has metadata", () => { + const registry = createPrReviewBufferRegistry(); + const buf = registry.getOrCreate("o/r", 1); + buf.setReviewMetadata("body", "COMMENT"); + expect(registry.hasAnyContent()).toBe(true); + }); + + it("setDefaultFooterMode applies to newly created buffers", () => { + const registry = createPrReviewBufferRegistry(); + registry.setDefaultFooterMode("none"); + // We can't directly inspect footerMode, but we can check that the + // new buffer was created (no throw) and that setting metadata works + const buf = registry.getOrCreate("o/r", 1); + expect(buf).not.toBeNull(); + buf.setReviewMetadata("body", "COMMENT"); + expect(buf.hasReviewMetadata()).toBe(true); + }); +}); describe("pr_review_buffer (factory pattern)", () => { let buffer; diff --git a/actions/setup/js/safe_output_handler_manager.cjs b/actions/setup/js/safe_output_handler_manager.cjs index 9b216b3aed6..2520014ae46 100644 --- a/actions/setup/js/safe_output_handler_manager.cjs +++ b/actions/setup/js/safe_output_handler_manager.cjs @@ -18,7 +18,7 @@ const { setCollectedMissings } = require("./missing_messages_helper.cjs"); const { writeSafeOutputSummaries } = require("./safe_output_summary.cjs"); const { getAssignToAgentAssigned, getAssignToAgentErrors, getAssignToAgentErrorCount, writeAssignToAgentSummary } = require("./assign_to_agent.cjs"); const { getCreateAgentSessionNumber, getCreateAgentSessionUrl, writeCreateAgentSessionSummary } = require("./create_agent_session.cjs"); -const { createReviewBuffer } = require("./pr_review_buffer.cjs"); +const { createPrReviewBufferRegistry } = require("./pr_review_buffer.cjs"); const { sanitizeContent } = require("./sanitize_content.cjs"); const { resolveAllowedMentionsFromPayload } = require("./resolve_mentions_from_payload.cjs"); const { createManifestLogger, ensureManifestExists, extractCreatedItemFromResult, writeTemporaryIdMapFile } = require("./safe_output_manifest.cjs"); @@ -287,11 +287,11 @@ const PR_REVIEW_HANDLER_TYPES = new Set(["create_pull_request_review_comment", " * Load and initialize handlers for enabled safe output types * Calls each handler's factory function (main) to get message processors * @param {Object} config - Safe outputs configuration - * @param {Object} prReviewBuffer - Shared PR review buffer instance + * @param {Object} prReviewBufferRegistry - PR review buffer registry instance * @param {string[]} [resolvedAllowedMentionAliases] - Pre-resolved mention aliases shared across handlers * @returns {Promise>} Map of type to message handler function */ -async function loadHandlers(config, prReviewBuffer, resolvedAllowedMentionAliases = []) { +async function loadHandlers(config, prReviewBufferRegistry, resolvedAllowedMentionAliases = []) { const messageHandlers = new Map(); core.info("Loading and initializing safe output handlers based on configuration..."); @@ -315,9 +315,9 @@ async function loadHandlers(config, prReviewBuffer, resolvedAllowedMentionAliase handlerConfig.allowedMentionAliases = resolvedAllowedMentionAliases; } - // Inject shared PR review buffer into handlers that need it + // Inject shared PR review buffer registry into handlers that need it if (PR_REVIEW_HANDLER_TYPES.has(type)) { - handlerConfig._prReviewBuffer = prReviewBuffer; + handlerConfig._prReviewBufferRegistry = prReviewBufferRegistry; } const messageHandler = await handlerModule.main(handlerConfig); @@ -534,6 +534,41 @@ function rollbackReviewResults(results, errorMessage) { } } +/** + * Roll back processing results for a specific PR when its review finalization fails. + * Matches results by repo and pull_request_number. Falls back to rolling back all + * review results when no results carry per-PR identifiers. + * + * @param {Array<{type: string, success: boolean, error?: string, repo?: string, pull_request_number?: number, result?: {repo?: string, pull_request_number?: number}}>} results + * @param {string} repo - Repository slug (owner/repo) + * @param {number} prNumber - Pull request number + * @param {string} errorMessage - Error message to attach to the rolled-back results + */ +function rollbackReviewResultsForPR(results, repo, prNumber, errorMessage) { + // processMessages wraps each handler result under r.result, so per-PR identifiers + // are nested there. Fall back to top-level for backward compatibility with callers + // that pass raw handler results directly. + const prResults = results.filter( + r => (r.type === "submit_pull_request_review" || r.type === "create_pull_request_review_comment") && r.success === true && (r.result?.repo ?? r.repo) === repo && (r.result?.pull_request_number ?? r.pull_request_number) === prNumber + ); + if (prResults.length > 0) { + for (const r of prResults) { + r.success = false; + r.error = `Review finalization failed: ${errorMessage}`; + } + } else { + // No results carry per-PR identifiers (e.g. legacy submit_pr_review results without repo/pull_request_number). + // Fall back to rolling back all buffered review results for this run. + core.warning(`rollbackReviewResultsForPR: no results matched ${repo}#${prNumber} — falling back to rolling back all review results`); + for (const r of results) { + if ((r.type === "submit_pull_request_review" || r.type === "create_pull_request_review_comment") && r.success === true) { + r.success = false; + r.error = `Review finalization failed: ${errorMessage}`; + } + } + } +} + /** * Mark buffered review results as skipped when the PR is locked and submission was * soft-skipped (success:true, skipped:true). Both submit_pull_request_review and @@ -556,6 +591,26 @@ function skipReviewResults(results, skipReason) { } } +/** + * Mark buffered review results for a specific PR as skipped. + * + * @param {Array<{type: string, success: boolean, skipped?: boolean, skipReason?: string, repo?: string, pull_request_number?: number, result?: {repo?: string, pull_request_number?: number}}>} results + * @param {string} repo - Repository slug (owner/repo) + * @param {number} prNumber - Pull request number + * @param {string} skipReason - Human-readable reason for the skip + */ +function skipReviewResultsForPR(results, repo, prNumber, skipReason) { + for (const r of results) { + // processMessages wraps each handler result under r.result, so per-PR identifiers + // are nested there. Fall back to top-level for backward compatibility with callers + // that pass raw handler results directly. + if ((r.type === "submit_pull_request_review" || r.type === "create_pull_request_review_comment") && r.success === true && (r.result?.repo ?? r.repo) === repo && (r.result?.pull_request_number ?? r.pull_request_number) === prNumber) { + r.skipped = true; + r.skipReason = skipReason; + } + } +} + /** * Determine whether a processing result is a non-skipped, non-deferred, non-cancelled failure. * @@ -1383,8 +1438,8 @@ async function main() { return; } - // Create the shared PR review buffer instance (no global state) - const prReviewBuffer = createReviewBuffer(); + // Create the PR review buffer registry (one per-PR buffer created on demand) + const prReviewBufferRegistry = createPrReviewBufferRegistry(); // Apply footer config with priority: // 1. submit_pull_request_review.footer (highest priority — footer controls review body) @@ -1397,13 +1452,13 @@ async function main() { } if (footerConfig !== undefined) { - prReviewBuffer.setFooterMode(footerConfig); + prReviewBufferRegistry.setDefaultFooterMode(footerConfig); } const allowedMentionAliases = config.mentions != null ? await resolveAllowedMentionsFromPayload(context, github, core, config.mentions) : []; // Load and initialize handlers based on configuration (factory pattern) - const messageHandlers = await loadHandlers(config, prReviewBuffer, allowedMentionAliases); + const messageHandlers = await loadHandlers(config, prReviewBufferRegistry, allowedMentionAliases); if (messageHandlers.size === 0) { core.info("No handlers loaded - nothing to process"); @@ -1428,44 +1483,43 @@ async function main() { // Process all messages in order of appearance const processingResult = await processMessages(messageHandlers, allMessages, logCreatedItem); - // Finalize buffered PR review — submit when comments or metadata exist - if (prReviewBuffer.hasBufferedComments() || prReviewBuffer.hasReviewMetadata()) { - core.info(`\n=== Finalizing PR Review ===`); - const bufferedCount = prReviewBuffer.getBufferedCount(); - if (bufferedCount > 0) { - core.info(`Submitting ${bufferedCount} buffered review comment(s) as a single PR review`); - } else { - core.info("Submitting PR review (body-only, no inline comments)"); - } - /** @type {any} */ - let reviewFailureError = null; - try { - const reviewResult = await prReviewBuffer.submitReview(); - if (reviewResult.success && !reviewResult.skipped) { - logCreatedItemFromResult(logCreatedItem, "submit_pull_request_review", reviewResult); - core.info(`✓ PR review submitted successfully: ${reviewResult.review_url}`); - } else if (reviewResult.success && reviewResult.skipped) { - const skipReason = reviewResult.reason || "PR review submission skipped"; - core.warning(`⚠ ${skipReason}`); - if (reviewResult.pr_locked) { - core.setOutput("pr_locked", "true"); + // Finalize buffered PR reviews — one review submission per distinct PR + const registryEntries = prReviewBufferRegistry.getAllEntries(); + for (const { repo: reviewRepo, prNumber: reviewPrNum, buffer: reviewBuffer } of registryEntries) { + if (reviewBuffer.hasBufferedComments() || reviewBuffer.hasReviewMetadata()) { + core.info(`\n=== Finalizing PR Review for ${reviewRepo}#${reviewPrNum} ===`); + const bufferedCount = reviewBuffer.getBufferedCount(); + if (bufferedCount > 0) { + core.info(`Submitting ${bufferedCount} buffered review comment(s) for ${reviewRepo}#${reviewPrNum}`); + } else { + core.info(`Submitting PR review for ${reviewRepo}#${reviewPrNum} (body-only, no inline comments)`); + } + /** @type {any} */ + let reviewFailureError = null; + try { + const reviewResult = await reviewBuffer.submitReview(); + if (reviewResult.success && !reviewResult.skipped) { + logCreatedItemFromResult(logCreatedItem, "submit_pull_request_review", reviewResult); + core.info(`✓ PR review submitted for ${reviewRepo}#${reviewPrNum}: ${reviewResult.review_url}`); + } else if (reviewResult.success && reviewResult.skipped) { + const skipReason = reviewResult.reason || `PR review for ${reviewRepo}#${reviewPrNum} skipped`; + core.warning(`⚠ ${skipReason}`); + if (reviewResult.pr_locked) { + core.setOutput("pr_locked", "true"); + } + skipReviewResultsForPR(processingResult.results, reviewRepo, reviewPrNum, skipReason); + } else if (!reviewResult.success) { + reviewFailureError = reviewResult.error || `PR review finalization failed for ${reviewRepo}#${reviewPrNum}`; + core.error(`✗ Failed to submit PR review for ${reviewRepo}#${reviewPrNum}: ${reviewFailureError}`); } - skipReviewResults(processingResult.results, skipReason); - } else if (!reviewResult.success) { - reviewFailureError = reviewResult.error || "PR review finalization failed"; - core.error(`✗ Failed to submit PR review: ${reviewFailureError}`); + } catch (reviewError) { + reviewFailureError = getErrorMessage(reviewError); + core.error(`✗ Exception while submitting PR review for ${reviewRepo}#${reviewPrNum}: ${reviewFailureError}`); } - } catch (reviewError) { - reviewFailureError = getErrorMessage(reviewError); - core.error(`✗ Exception while submitting PR review: ${reviewFailureError}`); - } - // Roll back per-message success counts when the finalization POST failed. - // Both submit_pull_request_review and create_pull_request_review_comment handlers - // return success:true during message processing (they only buffer), so the failure - // must be reflected here to ensure the Processing Summary shows the correct counts. - if (reviewFailureError !== null) { - rollbackReviewResults(processingResult.results, reviewFailureError); + if (reviewFailureError !== null) { + rollbackReviewResultsForPR(processingResult.results, reviewRepo, reviewPrNum, reviewFailureError); + } } } @@ -1662,7 +1716,9 @@ module.exports = { processMessages, buildCommentMemoryMessagesFromFiles, rollbackReviewResults, + rollbackReviewResultsForPR, skipReviewResults, + skipReviewResultsForPR, logCreatedItemFromResult, isFailedProcessingResult, isReportOnlyFailureResult, diff --git a/actions/setup/js/safe_output_handler_manager.test.cjs b/actions/setup/js/safe_output_handler_manager.test.cjs index dc1a7dbf2b4..a5c5260fe45 100644 --- a/actions/setup/js/safe_output_handler_manager.test.cjs +++ b/actions/setup/js/safe_output_handler_manager.test.cjs @@ -9,7 +9,9 @@ import { processMessages, buildCommentMemoryMessagesFromFiles, rollbackReviewResults, + rollbackReviewResultsForPR, skipReviewResults, + skipReviewResultsForPR, logCreatedItemFromResult, isFailedProcessingResult, isReportOnlyFailureResult, @@ -1972,6 +1974,124 @@ describe("Safe Output Handler Manager", () => { }); }); + describe("rollbackReviewResultsForPR", () => { + it("rolls back only the matching PR result (nested result shape from processMessages)", () => { + const results = [ + { type: "submit_pull_request_review", success: true, result: { repo: "o/r", pull_request_number: 1 } }, + { type: "submit_pull_request_review", success: true, result: { repo: "o/r", pull_request_number: 2 } }, + ]; + rollbackReviewResultsForPR(results, "o/r", 1, "submission failed"); + expect(results[0].success).toBe(false); + expect(results[0].error).toBe("Review finalization failed: submission failed"); + // PR 2 result must not be touched + expect(results[1].success).toBe(true); + expect(results[1].error).toBeUndefined(); + }); + + it("rolls back create_pull_request_review_comment results for the matching PR only", () => { + const results = [ + { type: "create_pull_request_review_comment", success: true, result: { repo: "o/r", pull_request_number: 1 } }, + { type: "create_pull_request_review_comment", success: true, result: { repo: "o/r", pull_request_number: 2 } }, + { type: "submit_pull_request_review", success: true, result: { repo: "o/r", pull_request_number: 1 } }, + ]; + rollbackReviewResultsForPR(results, "o/r", 1, "finalize error"); + expect(results[0].success).toBe(false); + expect(results[2].success).toBe(false); + // PR 2 comment unchanged + expect(results[1].success).toBe(true); + }); + + it("falls back to rolling back all review results when no nested result matches", () => { + // Legacy shape: no r.result, no top-level repo/pull_request_number + const results = [ + { type: "submit_pull_request_review", success: true }, + { type: "create_pull_request_review_comment", success: true }, + { type: "add_comment", success: true }, + ]; + rollbackReviewResultsForPR(results, "o/r", 99, "fallback error"); + expect(results[0].success).toBe(false); + expect(results[1].success).toBe(false); + // non-review type must not be rolled back + expect(results[2].success).toBe(true); + }); + + it("does not modify results that already have success:false", () => { + const results = [{ type: "submit_pull_request_review", success: false, error: "already failed", result: { repo: "o/r", pull_request_number: 1 } }]; + rollbackReviewResultsForPR(results, "o/r", 1, "new error"); + // Falls back to global rollback path — but success is already false so no change + expect(results[0].error).toBe("already failed"); + }); + + it("does not modify results for a different repo", () => { + const results = [{ type: "submit_pull_request_review", success: true, result: { repo: "o/other", pull_request_number: 1 } }]; + rollbackReviewResultsForPR(results, "o/r", 1, "error"); + // No match → fallback path, but the only review result has a different repo + // Fallback rolls back all review results regardless of repo + // (existing behavior: warns and rolls everything back) + expect(results[0].success).toBe(false); + }); + + it("handles empty results array without throwing", () => { + expect(() => rollbackReviewResultsForPR([], "o/r", 1, "error")).not.toThrow(); + }); + }); + + describe("skipReviewResultsForPR", () => { + it("marks only the matching PR result as skipped (nested result shape from processMessages)", () => { + const results = [ + { type: "submit_pull_request_review", success: true, result: { repo: "o/r", pull_request_number: 1 } }, + { type: "submit_pull_request_review", success: true, result: { repo: "o/r", pull_request_number: 2 } }, + ]; + skipReviewResultsForPR(results, "o/r", 1, "PR is locked"); + expect(results[0].skipped).toBe(true); + expect(results[0].skipReason).toBe("PR is locked"); + // PR 2 must not be skipped + expect(results[1].skipped).toBeUndefined(); + }); + + it("marks create_pull_request_review_comment results for the matching PR only", () => { + const results = [ + { type: "create_pull_request_review_comment", success: true, result: { repo: "o/r", pull_request_number: 3 } }, + { type: "create_pull_request_review_comment", success: true, result: { repo: "o/r", pull_request_number: 5 } }, + { type: "submit_pull_request_review", success: true, result: { repo: "o/r", pull_request_number: 3 } }, + ]; + skipReviewResultsForPR(results, "o/r", 3, "locked"); + expect(results[0].skipped).toBe(true); + expect(results[2].skipped).toBe(true); + expect(results[1].skipped).toBeUndefined(); + }); + + it("does not modify results with success:false", () => { + const results = [{ type: "submit_pull_request_review", success: false, error: "already failed", result: { repo: "o/r", pull_request_number: 1 } }]; + skipReviewResultsForPR(results, "o/r", 1, "locked"); + expect(results[0].skipped).toBeUndefined(); + }); + + it("does not modify unrelated result types", () => { + const results = [ + { type: "add_comment", success: true, result: { repo: "o/r", pull_request_number: 1 } }, + { type: "create_issue", success: true }, + ]; + skipReviewResultsForPR(results, "o/r", 1, "locked"); + expect(results[0].skipped).toBeUndefined(); + expect(results[1].skipped).toBeUndefined(); + }); + + it("handles top-level repo/pull_request_number for backward compat", () => { + const results = [ + { type: "submit_pull_request_review", success: true, repo: "o/r", pull_request_number: 7 }, + { type: "submit_pull_request_review", success: true, repo: "o/r", pull_request_number: 8 }, + ]; + skipReviewResultsForPR(results, "o/r", 7, "locked"); + expect(results[0].skipped).toBe(true); + expect(results[1].skipped).toBeUndefined(); + }); + + it("handles empty results array without throwing", () => { + expect(() => skipReviewResultsForPR([], "o/r", 1, "locked")).not.toThrow(); + }); + }); + describe("buildCommentMemoryMessagesFromFiles", () => { it("loads comment-memory messages from markdown files when configured", () => { fs.mkdirSync("/tmp/gh-aw/comment-memory", { recursive: true }); diff --git a/actions/setup/js/submit_pr_review.cjs b/actions/setup/js/submit_pr_review.cjs index ba8ad25a50d..d9d8e8a01d8 100644 --- a/actions/setup/js/submit_pr_review.cjs +++ b/actions/setup/js/submit_pr_review.cjs @@ -20,17 +20,26 @@ const VALID_EVENTS = new Set(["APPROVE", "REQUEST_CHANGES", "COMMENT"]); /** * Main handler factory for submit_pull_request_review * Returns a message handler that stores review metadata (body and event) - * in the shared PR review buffer. The actual review submission happens - * during the handler manager's finalization step. + * in a PR review buffer. The actual review submission happens during the + * handler manager's finalization step. * - * The PR review buffer instance is passed via config._prReviewBuffer. + * Supports two buffer modes: + * - Registry mode (config._prReviewBufferRegistry): per-PR buffers managed by a registry. + * Target is resolved first; each distinct PR gets its own independent buffer. + * - Legacy mode (config._prReviewBuffer): a single shared buffer (backward compat). * * @type {HandlerFactoryFunction} */ async function main(config = {}) { const maxCount = config.max || 1; const targetConfig = config.target || "triggering"; - const buffer = config._prReviewBuffer; + // Registry mode takes precedence over legacy single-buffer mode. + // The two modes are mutually exclusive: when a registry is provided, _prReviewBuffer is ignored. + const registry = config._prReviewBufferRegistry || null; + const legacyBuffer = registry ? null : config._prReviewBuffer || null; + if (registry && config._prReviewBuffer) { + core.warning("submit_pull_request_review: Both _prReviewBufferRegistry and _prReviewBuffer were provided; registry mode takes precedence and _prReviewBuffer will be ignored."); + } const supersedeOlderReviews = parseBoolTemplatable(config.supersede_older_reviews, false); const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config); const githubClient = await createAuthenticatedGitHubClient(config); @@ -43,7 +52,7 @@ async function main(config = {}) { // Build the allowed events set from config (empty set means all events are allowed) const allowedEvents = new Set(Array.isArray(config.allowed_events) && config.allowed_events.length > 0 ? config.allowed_events.map(e => String(e).toUpperCase()) : []); - if (!buffer) { + if (!registry && !legacyBuffer) { core.warning("submit_pull_request_review: No PR review buffer provided in config"); return async function handleSubmitPRReview() { return { success: false, error: "No PR review buffer available" }; @@ -59,17 +68,19 @@ async function main(config = {}) { core.info(`Allowed review events: ${Array.from(allowedEvents).join(", ")}`); } - // Propagate per-handler staged flag to the shared 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 will be previewed without being submitted"); } if (supersedeOlderReviews) { core.warning("submit_pull_request_review: supersede-older-reviews is best-effort. Prefer allowed-events: [COMMENT] by default and use REQUEST_CHANGES only when merge-blocking is required."); - if (typeof buffer.setSupersedeOlderReviews === "function") { - buffer.setSupersedeOlderReviews(true); + if (registry) { + registry.setDefaultSupersedeOlderReviews(true); + } else if (typeof legacyBuffer.setSupersedeOlderReviews === "function") { + legacyBuffer.setSupersedeOlderReviews(true); } } @@ -124,21 +135,95 @@ async function main(config = {}) { // Only increment after validation passes processedCount++; + if (registry) { + return await handleWithRegistry(message, event, body); + } + return await handleWithLegacyBuffer(message, event, body); + }; + + /** + * Registry path: resolve target PR first, then obtain a per-PR buffer and store metadata. + * @param {Object} message + * @param {string} event + * @param {string} body + * @returns {Promise} + */ + async function handleWithRegistry(message, event, body) { + const targetResult = resolveTarget({ + targetConfig, + item: message, + context, + itemType: "PR review", + supportsPR: false, + supportsIssue: false, + }); + + if (!targetResult.success || !targetResult.number) { + const errMsg = (targetResult.success === false ? targetResult.error : undefined) || "Could not determine target PR"; + core.warning(`Could not resolve PR for review: ${errMsg}`); + return { success: false, error: errMsg }; + } + + const prNum = targetResult.number; + + const repoResult = resolveAndValidateRepo(message, defaultTargetRepo, allowedRepos, "PR review"); + if (!repoResult.success) { + core.warning(`Could not resolve repository for PR review: ${repoResult.error}`); + return { success: false, error: repoResult.error }; + } + + const { repo, repoParts } = repoResult; + + const buffer = registry.getOrCreate(repo, prNum); + if (!buffer) { + return { success: false, error: `Could not get review buffer for ${repo}#${prNum}` }; + } + + const filterResult = await checkRequiredFilter(githubClient, repoParts, prNum, requiredLabels, requiredTitlePrefix, "submit_pr_review"); + if (filterResult) return filterResult; + + if (buffer.hasReviewMetadata()) { + const errMsg = `PR ${repo}#${prNum} already has a pending review submission. Only one submit_pull_request_review per PR per run is allowed; use target: "*" with max: 1 so each PR gets its own invocation.`; + core.warning(`submit_pull_request_review: ${errMsg}`); + return { success: false, error: errMsg }; + } + + core.info(`Setting review metadata for ${repo}#${prNum}: event=${event}, bodyLength=${body.length}`); + buffer.setReviewMetadata(body, event); + + if (!buffer.getReviewContext()) { + await setReviewContextOnBuffer(buffer, prNum, repo, repoParts, message); + } + + return { + success: true, + event, + body_length: body.length, + pull_request_number: prNum, + repo, + deferred_manifest: true, + }; + } + + /** + * Legacy path: use the single shared buffer (backward compat for tests and older callers). + * @param {Object} message + * @param {string} event + * @param {string} body + * @returns {Promise} + */ + async function handleWithLegacyBuffer(message, event, body) { + const buffer = legacyBuffer; core.info(`Setting review metadata: event=${event}, bodyLength=${body.length}`); - // Apply required-labels/required-title-prefix filter if review context is already available const existingReviewCtx = buffer.getReviewContext(); if (existingReviewCtx) { const filterResult = await checkRequiredFilter(githubClient, existingReviewCtx.repoParts, existingReviewCtx.pullRequestNumber, requiredLabels, requiredTitlePrefix, "submit_pr_review"); if (filterResult) return filterResult; } - // Store the review metadata in the shared buffer buffer.setReviewMetadata(body, event); - // Ensure review context is set for body-only reviews (no inline comments). - // If create_pull_request_review_comment already set context, this is a no-op. - // Use target config as single source of truth (same as add_comment): resolveTarget first, then use payload PR only when it matches. if (!buffer.getReviewContext()) { const targetResult = resolveTarget({ targetConfig, @@ -155,59 +240,67 @@ async function main(config = {}) { } } else if (targetResult.number) { const prNum = targetResult.number; - - // Resolve and validate the target repository (supports cross-repo via target-repo config) const repoResult = resolveAndValidateRepo(message, defaultTargetRepo, allowedRepos, "PR review"); if (!repoResult.success) { - // Warn and leave context unset; submitReview() will subsequently fail - // with "No review context available" — this is not a silent failure. core.warning(`Could not resolve repository for PR review context: ${repoResult.error}`); } else { const { repo, repoParts } = repoResult; - const payloadPR = context.payload?.pull_request; - const usePayloadPR = payloadPR && payloadPR.number === prNum && payloadPR.head?.sha && repo === `${context.repo.owner}/${context.repo.repo}`; - - if (usePayloadPR) { - buffer.setReviewContext({ - repo, - repoParts, - pullRequestNumber: payloadPR.number, - pullRequest: payloadPR, - }); - core.info(`Set review context from triggering PR: ${repo}#${payloadPR.number}`); - } else { - try { - const { data: fetchedPR } = await githubClient.rest.pulls.get({ - owner: repoParts.owner, - repo: repoParts.repo, - pull_number: prNum, - }); - if (fetchedPR?.head?.sha) { - buffer.setReviewContext({ - repo, - repoParts, - pullRequestNumber: fetchedPR.number, - pullRequest: fetchedPR, - }); - core.info(`Set review context from target: ${repo}#${fetchedPR.number}`); - } else { - core.warning("Fetched PR missing head.sha - cannot set review context"); - } - } catch (fetchErr) { - core.warning(`Could not fetch PR #${prNum} for review context: ${getErrorMessage(fetchErr)}`); - } - } + await setReviewContextOnBuffer(buffer, prNum, repo, repoParts, message); } } } return { success: true, - event: event, + event, body_length: body.length, deferred_manifest: true, }; - }; + } + + /** + * Fetch (or use payload) PR details and set review context on a buffer. + * @param {Object} buffer + * @param {number} prNum + * @param {string} repo + * @param {Object} repoParts + * @param {Object} message + */ + async function setReviewContextOnBuffer(buffer, prNum, repo, repoParts, message) { + const payloadPR = context.payload?.pull_request; + const usePayloadPR = payloadPR && payloadPR.number === prNum && payloadPR.head?.sha && repo === `${context.repo.owner}/${context.repo.repo}`; + + if (usePayloadPR) { + buffer.setReviewContext({ + repo, + repoParts, + pullRequestNumber: payloadPR.number, + pullRequest: payloadPR, + }); + core.info(`Set review context from triggering PR: ${repo}#${payloadPR.number}`); + } else { + try { + const { data: fetchedPR } = await githubClient.rest.pulls.get({ + owner: repoParts.owner, + repo: repoParts.repo, + pull_number: prNum, + }); + if (fetchedPR?.head?.sha) { + buffer.setReviewContext({ + repo, + repoParts, + pullRequestNumber: fetchedPR.number, + pullRequest: fetchedPR, + }); + core.info(`Set review context from target: ${repo}#${fetchedPR.number}`); + } else { + core.warning("Fetched PR missing head.sha - cannot set review context"); + } + } catch (fetchErr) { + core.warning(`Could not fetch PR #${prNum} for review context: ${getErrorMessage(fetchErr)}`); + } + } + } } module.exports = { main }; diff --git a/actions/setup/js/submit_pr_review.test.cjs b/actions/setup/js/submit_pr_review.test.cjs index 1ec4e29c29b..6b019debebb 100644 --- a/actions/setup/js/submit_pr_review.test.cjs +++ b/actions/setup/js/submit_pr_review.test.cjs @@ -1,4 +1,4 @@ -import { describe, it, expect, beforeEach, vi } from "vitest"; +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; const mockCore = { debug: vi.fn(), @@ -38,7 +38,7 @@ global.github = { graphql: vi.fn().mockResolvedValue({}), }; -const { createReviewBuffer } = require("./pr_review_buffer.cjs"); +const { createReviewBuffer, createPrReviewBufferRegistry } = require("./pr_review_buffer.cjs"); describe("submit_pr_review (Handler Factory Architecture)", () => { let handler; @@ -772,3 +772,108 @@ describe("submit_pr_review (Handler Factory Architecture)", () => { delete global.context; }); }); + +describe("submit_pr_review multi-buffer (registry mode)", () => { + beforeEach(() => { + vi.clearAllMocks(); + global.context = { + eventName: "workflow_dispatch", + repo: { owner: "o", repo: "r" }, + payload: {}, + }; + global.github = { + rest: { + pulls: { + get: vi.fn().mockImplementation(({ pull_number }) => Promise.resolve({ data: { number: pull_number, head: { sha: `sha-${pull_number}` } } })), + }, + }, + }; + }); + + afterEach(() => { + delete global.context; + delete global.github; + }); + + it("two calls targeting distinct PRs each populate their own buffer", async () => { + const registry = createPrReviewBufferRegistry(); + const { main } = require("./submit_pr_review.cjs"); + const handler = await main({ max: 5, target: "*", _prReviewBufferRegistry: registry }); + + const result1 = await handler({ type: "submit_pull_request_review", body: "Body for PR 1", event: "COMMENT", pull_request_number: 1 }, {}); + const result2 = await handler({ type: "submit_pull_request_review", body: "Body for PR 2", event: "APPROVE", pull_request_number: 2 }, {}); + + expect(result1.success).toBe(true); + expect(result1.pull_request_number).toBe(1); + expect(result1.repo).toBe("o/r"); + + expect(result2.success).toBe(true); + expect(result2.pull_request_number).toBe(2); + expect(result2.repo).toBe("o/r"); + + const entries = registry.getAllEntries(); + expect(entries).toHaveLength(2); + + const [e1, e2] = entries; + expect(e1.prNumber).toBe(1); + expect(e1.buffer.hasReviewMetadata()).toBe(true); + + expect(e2.prNumber).toBe(2); + expect(e2.buffer.hasReviewMetadata()).toBe(true); + + // Buffers are independent — metadata does not leak between them + const ctx1 = e1.buffer.getReviewContext(); + const ctx2 = e2.buffer.getReviewContext(); + expect(ctx1.pullRequestNumber).toBe(1); + expect(ctx2.pullRequestNumber).toBe(2); + }); + + it("second call targeting the same PR is rejected with an error", async () => { + const registry = createPrReviewBufferRegistry(); + const { main } = require("./submit_pr_review.cjs"); + const handler = await main({ max: 5, target: "*", _prReviewBufferRegistry: registry }); + + const result1 = await handler({ body: "First call", event: "COMMENT", pull_request_number: 7 }, {}); + const result2 = await handler({ body: "Second call", event: "APPROVE", pull_request_number: 7 }, {}); + + expect(result1.success).toBe(true); + // Second call to the same PR must be rejected — only one submit per PR per run is allowed + expect(result2.success).toBe(false); + expect(result2.error).toMatch(/already has a pending review submission/); + + // Only one buffer entry created (PR 7) + const entries = registry.getAllEntries(); + expect(entries).toHaveLength(1); + expect(entries[0].prNumber).toBe(7); + }); + + it("returns success:false when target cannot be resolved in registry mode", async () => { + // workflow_dispatch with target:triggering => can't resolve PR + const registry = createPrReviewBufferRegistry(); + const { main } = require("./submit_pr_review.cjs"); + const handler = await main({ max: 1, target: "triggering", _prReviewBufferRegistry: registry }); + + const result = await handler({ body: "Review", event: "COMMENT" }, {}); + + expect(result.success).toBe(false); + expect(result.error).toBeTruthy(); + // Registry should be empty — nothing was buffered + expect(registry.getAllEntries()).toHaveLength(0); + }); + + it("staged defaults propagate to each new buffer via registry.setDefaultStaged", async () => { + const registry = createPrReviewBufferRegistry(); + const setDefaultStagedSpy = vi.spyOn(registry, "setDefaultStaged"); + const { main } = require("./submit_pr_review.cjs"); + await main({ max: 1, target: "*", staged: true, _prReviewBufferRegistry: registry }); + expect(setDefaultStagedSpy).toHaveBeenCalledWith(true); + }); + + it("supersede_older_reviews default propagates via registry.setDefaultSupersedeOlderReviews", async () => { + const registry = createPrReviewBufferRegistry(); + const setSuperSpy = vi.spyOn(registry, "setDefaultSupersedeOlderReviews"); + const { main } = require("./submit_pr_review.cjs"); + await main({ max: 1, target: "*", supersede_older_reviews: true, _prReviewBufferRegistry: registry }); + expect(setSuperSpy).toHaveBeenCalledWith(true); + }); +});