From d9ec817999b02f5a2534b538a6b2abc2cbc6b554 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 6 Nov 2026 08:53:50 +0000 Subject: [PATCH 1/5] Initial plan From ca1fef1659ab6d41ef9cc82e3c10a5a7009da5cc Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 6 Nov 2026 09:17:47 +0000 Subject: [PATCH 2/5] Fix dependencies waiting on buffered PR review submission Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- actions/setup/js/safe_output_dependencies.cjs | 2 +- .../js/safe_output_dependencies.test.cjs | 42 ++++++- .../setup/js/safe_output_handler_manager.cjs | 107 +++++++++--------- actions/setup/js/safe_output_outcomes.cjs | 1 + .../setup/js/safe_output_outcomes.test.cjs | 51 +++++++++ .../content/docs/reference/safe-outputs.md | 6 + 6 files changed, 156 insertions(+), 53 deletions(-) diff --git a/actions/setup/js/safe_output_dependencies.cjs b/actions/setup/js/safe_output_dependencies.cjs index d31ea22cb62..c9f70a3e4f7 100644 --- a/actions/setup/js/safe_output_dependencies.cjs +++ b/actions/setup/js/safe_output_dependencies.cjs @@ -150,7 +150,7 @@ function checkSafeOutputDependencies(plan, index, results, allowWaiting) { if (records.length > 1) return { state: "failed", errorCode: "safe_output_dependency_failed", error: "Ambiguous safe-output prerequisite result" }; const record = records[0]; if (isConfirmedDependencySuccess(record)) continue; - if (allowWaiting && (!record || classifySafeOutputResult(record) === "deferred")) { + if (allowWaiting && (!record || classifySafeOutputResult(record) === "deferred" || (record.success === true && pickOutcomeField(record, "deferred_manifest")))) { waiting = true; continue; } diff --git a/actions/setup/js/safe_output_dependencies.test.cjs b/actions/setup/js/safe_output_dependencies.test.cjs index 2e95aec8206..ace7140cb7d 100644 --- a/actions/setup/js/safe_output_dependencies.test.cjs +++ b/actions/setup/js/safe_output_dependencies.test.cjs @@ -6,7 +6,7 @@ import { buildSafeOutputDependencyPlan, isConfirmedDependencySuccess } from "./s import { main as createActionHandler } from "./safe_output_action_handler.cjs"; import { main as createCallWorkflowHandler } from "./call_workflow.cjs"; import { createHandlers } from "./safe_outputs_handlers.cjs"; -import { buildSafeOutputOutcomes, projectSafeOutputData } from "./safe_output_outcomes.cjs"; +import { buildSafeOutputOutcomes, finalizeBufferedReviewOutcomes, projectSafeOutputData } from "./safe_output_outcomes.cjs"; import { createSessionValidator } from "./scripts/validate_session.cjs"; const id = index => `so_${String(index).padStart(12, "0")}`; @@ -163,6 +163,46 @@ describe("safe-output runtime dependencies", () => { expect(result.results.every(item => item.success)).toBe(true); }); + it.each(["submit_pull_request_review", "create_pull_request_review_comment"])("waits for buffered %s submission before running a dependent check and its dependent", async type => { + const calls = []; + const review = vi.fn(async () => { + calls.push("buffer"); + return { success: true, repo: "owner/repo", pull_request_number: 42, deferred_manifest: true, ...(type === "create_pull_request_review_comment" ? { buffered: true, metadata: { path: "file.js", line: 1, side: "RIGHT" } } : {}) }; + }); + const check = vi.fn(async () => { + calls.push("check"); + return { success: true }; + }); + const comment = vi.fn(async () => { + calls.push("comment"); + return { success: true }; + }); + const finalize = vi.fn(async results => { + calls.push("submit"); + expect(check).not.toHaveBeenCalled(); + finalizeBufferedReviewOutcomes(results, "owner/repo", 42, { + success: true, + review_url: "https://github.com/owner/repo/pull/42#pullrequestreview-1", + comment_count: 1, + review_comments: [{ metadata: { path: "file.js", line: 1, side: "RIGHT" } }], + }); + }); + const result = await processMessages( + new Map([ + [type, review], + ["create_check_run", check], + ["add_comment", comment], + ]), + [message(1, undefined, type), message(2, id(1), "create_check_run"), message(3, id(2))], + null, + finalize + ); + expect(calls).toEqual(["buffer", "submit", "check", "comment"]); + expect(finalize).toHaveBeenCalledOnce(); + expect(result.results.every(item => item.success)).toBe(true); + expect(result.results[0].result.deferred_manifest).toBe(false); + }); + it("retries a waiting dependent that itself defers, before releasing its dependent", async () => { const calls = []; const attempts = new Map(); diff --git a/actions/setup/js/safe_output_handler_manager.cjs b/actions/setup/js/safe_output_handler_manager.cjs index aa2b8e3e178..68220af5ef7 100644 --- a/actions/setup/js/safe_output_handler_manager.cjs +++ b/actions/setup/js/safe_output_handler_manager.cjs @@ -994,9 +994,10 @@ function sortMessagesByTemporaryIdDependencies(messages) { * @param {Map} messageHandlers - Map of message handler functions * @param {Array} messages - Array of safe output messages * @param {((item: {type: string, url?: string, number?: number, repo?: string, temporaryId?: string}) => void)|null} [onItemCreated] - Optional callback invoked after each successful create operation (for manifest logging) + * @param {((results: Array) => Promise)|null} [finalizeBufferedOutputs] - Finalize buffered effects before resolving waiting dependencies * @returns {Promise<{success: boolean, results: Array, temporaryIdMap: Object, artifactUrlMap: Map, outputsWithUnresolvedIds: Array, missings: Object, codePushFailures: Array<{type: string, error: string}>}>} */ -async function processMessages(messageHandlers, messages, onItemCreated = null) { +async function processMessages(messageHandlers, messages, onItemCreated = null, finalizeBufferedOutputs = null) { if (readClaimScopeContext() && !currentClaimHandle()) { throw new Error("Queue safe outputs must execute in a trusted per-Claim handler context; use the scoped manager"); } @@ -1656,6 +1657,7 @@ async function processMessages(messageHandlers, messages, onItemCreated = null) } await retryDeferredMessages(); + if (finalizeBufferedOutputs) await finalizeBufferedOutputs(results); for (const i of waitingMessages) { await processMessage(i, false); } @@ -2289,64 +2291,67 @@ async function main(options = {}) { // even if no individual write fails. The check is best-effort – failures are non-fatal. await checkRateLimitHeadroom(github, "safe_outputs_pre_check"); - // Process all messages in order of appearance - const processingResult = await processMessages(messageHandlers, allMessages, logCreatedItem); - outcomeResults = processingResult.results; - await finalizeLedgerAppend(config, messageHandlers); - - // 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 { - await assertClaimAuthorized({ type: "submit_pull_request_review", claim_handle: currentClaimHandle(), repo: reviewRepo, pull_request_number: reviewPrNum }); - const stalePRReason = await getStalePullRequestReason(github, context); - if (stalePRReason) { - core.info(stalePRReason); - skipReviewResultsForPR(processingResult.results, reviewRepo, reviewPrNum, stalePRReason); - continue; + // Finalize buffered PR reviews before releasing their dependent outputs. + /** @param {Array} results */ + async function finalizeBufferedReviews(results) { + 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)`); } - const reviewResult = await reviewBuffer.submitReview(); - if (reviewResult.success && !reviewResult.skipped) { - finalizeBufferedReviewOutcomes(processingResult.results, reviewRepo, reviewPrNum, reviewResult); - logCreatedItemFromResult(logCreatedItem, "submit_pull_request_review", reviewResult); - logCreatedItemFromResult( - logCreatedItem, - "create_pull_request_review_comment", - reviewResult.review_comments?.map(comment => (comment && typeof comment === "object" ? { ...comment, success: true } : comment)) - ); - 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"); + /** @type {any} */ + let reviewFailureError = null; + try { + await assertClaimAuthorized({ type: "submit_pull_request_review", claim_handle: currentClaimHandle(), repo: reviewRepo, pull_request_number: reviewPrNum }); + const stalePRReason = await getStalePullRequestReason(github, context); + if (stalePRReason) { + core.info(stalePRReason); + skipReviewResultsForPR(results, reviewRepo, reviewPrNum, stalePRReason); + continue; + } + const reviewResult = await reviewBuffer.submitReview(); + if (reviewResult.success && !reviewResult.skipped) { + finalizeBufferedReviewOutcomes(results, reviewRepo, reviewPrNum, reviewResult); + logCreatedItemFromResult(logCreatedItem, "submit_pull_request_review", reviewResult); + logCreatedItemFromResult( + logCreatedItem, + "create_pull_request_review_comment", + reviewResult.review_comments?.map(comment => (comment && typeof comment === "object" ? { ...comment, success: true } : comment)) + ); + 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(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}`); } - 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}`); + } 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 for ${reviewRepo}#${reviewPrNum}: ${reviewFailureError}`); - } - if (reviewFailureError !== null) { - rollbackReviewResultsForPR(processingResult.results, reviewRepo, reviewPrNum, reviewFailureError); + if (reviewFailureError !== null) { + rollbackReviewResultsForPR(results, reviewRepo, reviewPrNum, reviewFailureError); + } } } } + // Process all messages in order of appearance + const processingResult = await processMessages(messageHandlers, allMessages, logCreatedItem, finalizeBufferedReviews); + outcomeResults = processingResult.results; + await finalizeLedgerAppend(config, messageHandlers); + // Store collected missings in helper module for handlers to access if (processingResult.missings) { setCollectedMissings(processingResult.missings); diff --git a/actions/setup/js/safe_output_outcomes.cjs b/actions/setup/js/safe_output_outcomes.cjs index 617e1ab20f0..86a27276ce3 100644 --- a/actions/setup/js/safe_output_outcomes.cjs +++ b/actions/setup/js/safe_output_outcomes.cjs @@ -129,6 +129,7 @@ function finalizeBufferedReviewOutcomes(results, repo, prNumber, review) { if (index >= 0) { const [comment] = comments.splice(index, 1); result.deferred_manifest = false; + result.buffered = false; result.url = comment.url || review.review_url; } else if (completeComments) { record.skipped = true; diff --git a/actions/setup/js/safe_output_outcomes.test.cjs b/actions/setup/js/safe_output_outcomes.test.cjs index 1e8e877bd6b..9076f194145 100644 --- a/actions/setup/js/safe_output_outcomes.test.cjs +++ b/actions/setup/js/safe_output_outcomes.test.cjs @@ -287,6 +287,57 @@ describe("safe-output application outcomes", () => { expect(JSON.stringify(outcome)).not.toContain("PRIVATE"); }); + it.each(["success", "failed", "skipped", "staged"])("resolves a check depending on a buffered review after %s submission", async scenario => { + const pullRequest = { number: 1, state: "open", head: { sha: "abc123" }, user: { login: "review-author" } }; + const summary = { addRaw: vi.fn().mockReturnThis(), write: vi.fn().mockResolvedValue(undefined) }; + vi.stubGlobal("core", { info: vi.fn(), debug: vi.fn(), warning: vi.fn(), error: vi.fn(), setOutput: vi.fn(), setFailed: vi.fn(), summary }); + vi.stubGlobal("context", { eventName: "pull_request", repo: { owner: "org", repo: "repo" }, payload: { pull_request: pullRequest } }); + const calls = []; + const createReview = vi.fn(async () => { + calls.push("review"); + if (scenario === "failed") throw Object.assign(new Error("Review rejected"), { status: 422 }); + return { data: { id: 5, html_url: "https://github.com/org/repo/pull/1#pullrequestreview-5" } }; + }); + const createCheck = vi.fn(async () => { + calls.push("check"); + return { data: { id: 6, html_url: "https://github.com/org/repo/check-runs/6" } }; + }); + vi.stubGlobal("github", { + rest: { + rateLimit: { get: vi.fn().mockResolvedValue({ data: { resources: { core: { remaining: 5000, limit: 5000, reset: 0 } } } }) }, + pulls: { get: vi.fn().mockResolvedValue({ data: { ...pullRequest, ...(scenario === "skipped" ? { state: "closed" } : {}) } }), listReviews: vi.fn().mockResolvedValue({ data: [] }), createReview }, + checks: { create: createCheck }, + }, + }); + vi.stubEnv("GH_AW_SAFE_OUTPUTS_STAGED", "false"); + vi.stubEnv("GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG", JSON.stringify({ submit_pull_request_review: { staged: scenario === "staged" }, create_check_run: {} })); + vi.spyOn(fs, "mkdirSync").mockImplementation(() => undefined); + vi.spyOn(fs, "writeFileSync").mockImplementation(() => undefined); + const append = vi.spyOn(fs, "appendFileSync").mockImplementation(() => undefined); + await handlerManagerMain({ + messages: [ + { type: "submit_pull_request_review", output_id: "so_000000000001", pull_request_number: 1, body: "PRIVATE_REVIEW_BODY", event: "COMMENT" }, + { type: "create_check_run", output_id: "so_000000000002", "depends-on": "so_000000000001", conclusion: "success", title: "Review complete", summary: "PRIVATE_CHECK_BODY" }, + ], + }); + const outcomesCall = append.mock.calls.find(([file]) => String(file).endsWith("safe-output-outcomes.jsonl")); + expect(outcomesCall).toBeDefined(); + const outcomes = String(outcomesCall[1]) + .trim() + .split("\n") + .map(line => JSON.parse(line)); + if (scenario === "success") { + expect(calls).toEqual(["review", "check"]); + expect(global.core.setFailed).not.toHaveBeenCalled(); + expect(outcomes.map(outcome => outcome.status)).toEqual(["success", "success"]); + } else { + expect(createCheck).not.toHaveBeenCalled(); + expect(global.core.setFailed).toHaveBeenCalled(); + expect(outcomes.find(outcome => outcome.type === "create_check_run").status).toBe("failed"); + } + expect(JSON.stringify(outcomes)).not.toContain("PRIVATE"); + }); + it("persists real processing results and collects root and Claim outcomes into the unified session", async () => { const root = fs.mkdtempSync(path.join(process.cwd(), ".safe-output-outcomes-test-")); vi.stubGlobal("core", { info: vi.fn(), debug: vi.fn(), warning: vi.fn(), error: vi.fn(), setOutput: vi.fn(), setFailed: vi.fn() }); diff --git a/docs/src/content/docs/reference/safe-outputs.md b/docs/src/content/docs/reference/safe-outputs.md index 86721e0d2a8..4434405e84f 100644 --- a/docs/src/content/docs/reference/safe-outputs.md +++ b/docs/src/content/docs/reference/safe-outputs.md @@ -84,6 +84,12 @@ Skipped, cancelled, staged, deferred, or delegated results do not establish success. Missing, ambiguous, or cyclic dependencies also fail closed. Independent outputs retain their existing behavior and policy checks. +`submit_pull_request_review` and `create_pull_request_review_comment` can be +prerequisites. Inline comments and the review body are buffered and submitted +together at the end of message processing, before dependent outputs run. +Buffering alone does not confirm success: dependents wait for submission and +run only if their prerequisite was actually published. + Dependencies apply within a single safe-output processing pass (and within the same Claim in queue workflows). Custom safe jobs, `call_workflow`, `push_repo_memory`, `upload_asset`, `noop`, and out-of-band diagnostics From 4b47d0d6ec2d9c0bc094508ed015c81df66f15dd Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 6 Nov 2026 09:21:02 +0000 Subject: [PATCH 3/5] Handle chained buffered review dependencies without replaying effects Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- actions/setup/js/pr_review_buffer.cjs | 10 +++++++ actions/setup/js/pr_review_buffer.test.cjs | 13 ++++++++ .../js/safe_output_dependencies.test.cjs | 22 +++++++++++++- .../setup/js/safe_output_handler_manager.cjs | 14 +++++++-- .../setup/js/safe_output_outcomes.test.cjs | 30 +++++++++++++------ actions/setup/js/submit_pr_review.cjs | 5 +++- actions/setup/js/submit_pr_review.test.cjs | 3 +- .../content/docs/reference/safe-outputs.md | 1 + 8 files changed, 83 insertions(+), 15 deletions(-) diff --git a/actions/setup/js/pr_review_buffer.cjs b/actions/setup/js/pr_review_buffer.cjs index 0d6c7934d08..fa6d2a74b6d 100644 --- a/actions/setup/js/pr_review_buffer.cjs +++ b/actions/setup/js/pr_review_buffer.cjs @@ -945,6 +945,15 @@ function createPrReviewBufferRegistry() { return insertionOrder; } + /** + * Consume the current batch so dependent outputs can buffer a fresh review. + * @returns {{repo: string, prNumber: number, buffer: Object}[]} + */ + function drainEntries() { + bufferMap.clear(); + return insertionOrder.splice(0); + } + /** * Returns true if any buffer has buffered comments or review metadata. * @returns {boolean} @@ -983,6 +992,7 @@ function createPrReviewBufferRegistry() { return { getOrCreate, getAllEntries, + drainEntries, hasAnyContent, setDefaultFooterMode, setDefaultFooterContext, diff --git a/actions/setup/js/pr_review_buffer.test.cjs b/actions/setup/js/pr_review_buffer.test.cjs index faa74af0ae4..7ff22235a2d 100644 --- a/actions/setup/js/pr_review_buffer.test.cjs +++ b/actions/setup/js/pr_review_buffer.test.cjs @@ -82,6 +82,19 @@ describe("createPrReviewBufferRegistry", () => { expect(entries.map(e => e.prNumber)).toEqual([3, 1, 2]); }); + it("drains each batch once and creates a fresh buffer for the same PR", () => { + const registry = createPrReviewBufferRegistry(); + const buffer = registry.getOrCreate("o/r", 1); + buffer.setReviewMetadata("First batch", "APPROVE"); + expect(registry.drainEntries()).toEqual([{ repo: "o/r", prNumber: 1, buffer }]); + expect(registry.drainEntries()).toEqual([]); + expect(registry.hasAnyContent()).toBe(false); + const next = registry.getOrCreate("o/r", 1); + expect(next).not.toBe(buffer); + expect(next.hasReviewMetadata()).toBe(false); + expect(next.hasBufferedComments()).toBe(false); + }); + it("hasAnyContent returns false when all buffers are empty", () => { const registry = createPrReviewBufferRegistry(); registry.getOrCreate("o/r", 1); diff --git a/actions/setup/js/safe_output_dependencies.test.cjs b/actions/setup/js/safe_output_dependencies.test.cjs index ace7140cb7d..ca37a5722a9 100644 --- a/actions/setup/js/safe_output_dependencies.test.cjs +++ b/actions/setup/js/safe_output_dependencies.test.cjs @@ -178,6 +178,7 @@ describe("safe-output runtime dependencies", () => { return { success: true }; }); const finalize = vi.fn(async results => { + if (!results.some(result => result.result?.deferred_manifest)) return; calls.push("submit"); expect(check).not.toHaveBeenCalled(); finalizeBufferedReviewOutcomes(results, "owner/repo", 42, { @@ -198,7 +199,7 @@ describe("safe-output runtime dependencies", () => { finalize ); expect(calls).toEqual(["buffer", "submit", "check", "comment"]); - expect(finalize).toHaveBeenCalledOnce(); + expect(finalize).toHaveBeenCalled(); expect(result.results.every(item => item.success)).toBe(true); expect(result.results[0].result.deferred_manifest).toBe(false); }); @@ -217,6 +218,25 @@ describe("safe-output runtime dependencies", () => { expect(result.results.every(item => item.success)).toBe(true); }); + it("retries deferred dependents after review publication without replaying completed retries", async () => { + const calls = []; + const attempts = new Map(); + const handler = vi.fn(async msg => { + calls.push(msg.output_id); + const count = attempts.get(msg.output_id) || 0; + attempts.set(msg.output_id, count + 1); + if (msg.output_id === id(2)) return { success: true, deferred_manifest: true }; + return count === 0 && msg.output_id !== id(4) ? { deferred: true } : { success: true }; + }); + const result = await processMessages(new Map([["add_comment", handler]]), [message(1), message(2, id(1)), message(3, id(2)), message(4, id(3))], null, async results => { + for (const record of results) { + if (record.result?.deferred_manifest) record.result.deferred_manifest = false; + } + }); + expect(calls).toEqual([id(1), id(1), id(2), id(3), id(3), id(4)]); + expect(result.results.every(item => item.success)).toBe(true); + }); + it("waits for a prerequisite's temporary-ID resolution on retry", async () => { const calls = []; const link = vi.fn(async (_message, temporaryIds) => { diff --git a/actions/setup/js/safe_output_handler_manager.cjs b/actions/setup/js/safe_output_handler_manager.cjs index 68220af5ef7..ed13c4e8b4c 100644 --- a/actions/setup/js/safe_output_handler_manager.cjs +++ b/actions/setup/js/safe_output_handler_manager.cjs @@ -1517,7 +1517,7 @@ async function processMessages(messageHandlers, messages, onItemCreated = null, core.info(`\n=== Retrying Deferred Messages ===`); core.info(`Found ${deferredMessages.length} deferred message(s) to retry`); - let pending = [...deferredMessages]; + let pending = deferredMessages.filter(deferred => results.some(result => result.messageIndex === deferred.messageIndex && result.deferred)); for (let round = 0; pending.length > 0 && round <= messages.length; round++) { const roundStartTemporaryIdCount = temporaryIdMap.size; const attempted = new Set(pending.map(deferred => deferred.messageIndex)); @@ -1657,7 +1657,15 @@ async function processMessages(messageHandlers, messages, onItemCreated = null, } await retryDeferredMessages(); - if (finalizeBufferedOutputs) await finalizeBufferedOutputs(results); + if (finalizeBufferedOutputs) { + let previousResultCount; + do { + await finalizeBufferedOutputs(results); + previousResultCount = results.length; + await drainWaitingMessages(); + await retryDeferredMessages(); + } while (results.length > previousResultCount); + } for (const i of waitingMessages) { await processMessage(i, false); } @@ -2294,7 +2302,7 @@ async function main(options = {}) { // Finalize buffered PR reviews before releasing their dependent outputs. /** @param {Array} results */ async function finalizeBufferedReviews(results) { - const registryEntries = prReviewBufferRegistry.getAllEntries(); + const registryEntries = prReviewBufferRegistry.drainEntries(); for (const { repo: reviewRepo, prNumber: reviewPrNum, buffer: reviewBuffer } of registryEntries) { if (reviewBuffer.hasBufferedComments() || reviewBuffer.hasReviewMetadata()) { core.info(`\n=== Finalizing PR Review for ${reviewRepo}#${reviewPrNum} ===`); diff --git a/actions/setup/js/safe_output_outcomes.test.cjs b/actions/setup/js/safe_output_outcomes.test.cjs index 9076f194145..b288a7d8ad6 100644 --- a/actions/setup/js/safe_output_outcomes.test.cjs +++ b/actions/setup/js/safe_output_outcomes.test.cjs @@ -287,16 +287,16 @@ describe("safe-output application outcomes", () => { expect(JSON.stringify(outcome)).not.toContain("PRIVATE"); }); - it.each(["success", "failed", "skipped", "staged"])("resolves a check depending on a buffered review after %s submission", async scenario => { + it.each(["success", "chained", "chained-comment", "failed", "skipped", "staged"])("resolves a check depending on a buffered review after %s submission", async scenario => { const pullRequest = { number: 1, state: "open", head: { sha: "abc123" }, user: { login: "review-author" } }; const summary = { addRaw: vi.fn().mockReturnThis(), write: vi.fn().mockResolvedValue(undefined) }; vi.stubGlobal("core", { info: vi.fn(), debug: vi.fn(), warning: vi.fn(), error: vi.fn(), setOutput: vi.fn(), setFailed: vi.fn(), summary }); vi.stubGlobal("context", { eventName: "pull_request", repo: { owner: "org", repo: "repo" }, payload: { pull_request: pullRequest } }); const calls = []; - const createReview = vi.fn(async () => { + const createReview = vi.fn(async ({ pull_number }) => { calls.push("review"); if (scenario === "failed") throw Object.assign(new Error("Review rejected"), { status: 422 }); - return { data: { id: 5, html_url: "https://github.com/org/repo/pull/1#pullrequestreview-5" } }; + return { data: { id: pull_number + 4, html_url: `https://github.com/org/repo/pull/${pull_number}#pullrequestreview-${pull_number + 4}` } }; }); const createCheck = vi.fn(async () => { calls.push("check"); @@ -305,19 +305,26 @@ describe("safe-output application outcomes", () => { vi.stubGlobal("github", { rest: { rateLimit: { get: vi.fn().mockResolvedValue({ data: { resources: { core: { remaining: 5000, limit: 5000, reset: 0 } } } }) }, - pulls: { get: vi.fn().mockResolvedValue({ data: { ...pullRequest, ...(scenario === "skipped" ? { state: "closed" } : {}) } }), listReviews: vi.fn().mockResolvedValue({ data: [] }), createReview }, + pulls: { + get: vi.fn().mockResolvedValue({ data: { ...pullRequest, ...(scenario === "skipped" ? { state: "closed" } : {}) } }), + listReviews: vi.fn().mockResolvedValue({ data: [] }), + listFiles: vi.fn().mockResolvedValue({ data: [{ filename: "file.js" }] }), + createReview, + }, checks: { create: createCheck }, }, }); vi.stubEnv("GH_AW_SAFE_OUTPUTS_STAGED", "false"); - vi.stubEnv("GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG", JSON.stringify({ submit_pull_request_review: { staged: scenario === "staged" }, create_check_run: {} })); + vi.stubEnv("GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG", JSON.stringify({ submit_pull_request_review: { max: 2, target: "*", staged: scenario === "staged" }, create_pull_request_review_comment: {}, create_check_run: {} })); vi.spyOn(fs, "mkdirSync").mockImplementation(() => undefined); vi.spyOn(fs, "writeFileSync").mockImplementation(() => undefined); const append = vi.spyOn(fs, "appendFileSync").mockImplementation(() => undefined); await handlerManagerMain({ messages: [ { type: "submit_pull_request_review", output_id: "so_000000000001", pull_request_number: 1, body: "PRIVATE_REVIEW_BODY", event: "COMMENT" }, - { type: "create_check_run", output_id: "so_000000000002", "depends-on": "so_000000000001", conclusion: "success", title: "Review complete", summary: "PRIVATE_CHECK_BODY" }, + ...(scenario === "chained" ? [{ type: "submit_pull_request_review", output_id: "so_000000000003", "depends-on": "so_000000000001", pull_request_number: 2, body: "PRIVATE_SECOND_REVIEW_BODY", event: "COMMENT" }] : []), + ...(scenario === "chained-comment" ? [{ type: "create_pull_request_review_comment", output_id: "so_000000000003", "depends-on": "so_000000000001", path: "file.js", line: 1, body: "PRIVATE_INLINE_COMMENT" }] : []), + { type: "create_check_run", output_id: "so_000000000002", "depends-on": scenario.startsWith("chained") ? "so_000000000003" : "so_000000000001", conclusion: "success", title: "Review complete", summary: "PRIVATE_CHECK_BODY" }, ], }); const outcomesCall = append.mock.calls.find(([file]) => String(file).endsWith("safe-output-outcomes.jsonl")); @@ -326,10 +333,15 @@ describe("safe-output application outcomes", () => { .trim() .split("\n") .map(line => JSON.parse(line)); - if (scenario === "success") { - expect(calls).toEqual(["review", "check"]); + if (scenario === "success" || scenario.startsWith("chained")) { + expect(calls).toEqual(scenario.startsWith("chained") ? ["review", "review", "check"] : ["review", "check"]); expect(global.core.setFailed).not.toHaveBeenCalled(); - expect(outcomes.map(outcome => outcome.status)).toEqual(["success", "success"]); + expect(outcomes.every(outcome => outcome.status === "success")).toBe(true); + if (scenario === "chained-comment") { + expect(createReview.mock.calls[0][0]).not.toHaveProperty("comments"); + expect(createReview.mock.calls[1][0].comments).toHaveLength(1); + expect(createReview.mock.calls[1][0].event).toBe("COMMENT"); + } } else { expect(createCheck).not.toHaveBeenCalled(); expect(global.core.setFailed).toHaveBeenCalled(); diff --git a/actions/setup/js/submit_pr_review.cjs b/actions/setup/js/submit_pr_review.cjs index 2626a5c4233..107078116b4 100644 --- a/actions/setup/js/submit_pr_review.cjs +++ b/actions/setup/js/submit_pr_review.cjs @@ -108,6 +108,7 @@ async function main(config = {}) { } let processedCount = 0; + const reviewTargets = new Set(); /** * Message handler that stores review metadata @@ -210,7 +211,8 @@ async function main(config = {}) { const filterResult = await checkRequiredFilter(githubClient, repoParts, prNum, requiredLabels, requiredTitlePrefix, "submit_pr_review"); if (filterResult) return filterResult; - if (buffer.hasReviewMetadata()) { + const reviewTarget = `${repo}#${prNum}`; + if (buffer.hasReviewMetadata() || reviewTargets.has(reviewTarget)) { 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 }; @@ -218,6 +220,7 @@ async function main(config = {}) { core.info(`Setting review metadata for ${repo}#${prNum}: event=${event}, bodyLength=${body.length}`); buffer.setReviewMetadata(body, event); + reviewTargets.add(reviewTarget); if (!buffer.getReviewContext()) { await setReviewContextOnBuffer(buffer, prNum, repo, repoParts, message); diff --git a/actions/setup/js/submit_pr_review.test.cjs b/actions/setup/js/submit_pr_review.test.cjs index 0e4e773d6bb..18bef5c0fa0 100644 --- a/actions/setup/js/submit_pr_review.test.cjs +++ b/actions/setup/js/submit_pr_review.test.cjs @@ -828,12 +828,13 @@ describe("submit_pr_review multi-buffer (registry mode)", () => { expect(ctx2.pullRequestNumber).toBe(2); }); - it("second call targeting the same PR is rejected with an error", async () => { + it.each([false, true])("second call targeting the same PR is rejected even after draining: %s", async drained => { 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 }, {}); + if (drained) registry.drainEntries(); const result2 = await handler({ body: "Second call", event: "APPROVE", pull_request_number: 7 }, {}); expect(result1.success).toBe(true); diff --git a/docs/src/content/docs/reference/safe-outputs.md b/docs/src/content/docs/reference/safe-outputs.md index 4434405e84f..10e789007ad 100644 --- a/docs/src/content/docs/reference/safe-outputs.md +++ b/docs/src/content/docs/reference/safe-outputs.md @@ -89,6 +89,7 @@ prerequisites. Inline comments and the review body are buffered and submitted together at the end of message processing, before dependent outputs run. Buffering alone does not confirm success: dependents wait for submission and run only if their prerequisite was actually published. +Review comments released by a dependency are submitted in a subsequent batch. Dependencies apply within a single safe-output processing pass (and within the same Claim in queue workflows). Custom safe jobs, `call_workflow`, From cbe5c63024ac5caec6f3c61c0f74e0a7e8d63d7b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 6 Nov 2026 09:30:15 +0000 Subject: [PATCH 4/5] Preserve published review outcomes across dependent batch failures Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- actions/setup/js/safe_output_handler_manager.cjs | 11 +++++++---- actions/setup/js/safe_output_outcomes.test.cjs | 13 +++++++++---- docs/src/content/docs/reference/safe-outputs.md | 7 ++++--- 3 files changed, 20 insertions(+), 11 deletions(-) diff --git a/actions/setup/js/safe_output_handler_manager.cjs b/actions/setup/js/safe_output_handler_manager.cjs index ed13c4e8b4c..92540ed3314 100644 --- a/actions/setup/js/safe_output_handler_manager.cjs +++ b/actions/setup/js/safe_output_handler_manager.cjs @@ -2300,8 +2300,11 @@ async function main(options = {}) { await checkRateLimitHeadroom(github, "safe_outputs_pre_check"); // Finalize buffered PR reviews before releasing their dependent outputs. + const finalizedReviewResults = new Set(); /** @param {Array} results */ async function finalizeBufferedReviews(results) { + const reviewResults = results.filter(record => record.result?.deferred_manifest && !finalizedReviewResults.has(record)); + for (const record of reviewResults) finalizedReviewResults.add(record); const registryEntries = prReviewBufferRegistry.drainEntries(); for (const { repo: reviewRepo, prNumber: reviewPrNum, buffer: reviewBuffer } of registryEntries) { if (reviewBuffer.hasBufferedComments() || reviewBuffer.hasReviewMetadata()) { @@ -2319,12 +2322,12 @@ async function main(options = {}) { const stalePRReason = await getStalePullRequestReason(github, context); if (stalePRReason) { core.info(stalePRReason); - skipReviewResultsForPR(results, reviewRepo, reviewPrNum, stalePRReason); + skipReviewResultsForPR(reviewResults, reviewRepo, reviewPrNum, stalePRReason); continue; } const reviewResult = await reviewBuffer.submitReview(); if (reviewResult.success && !reviewResult.skipped) { - finalizeBufferedReviewOutcomes(results, reviewRepo, reviewPrNum, reviewResult); + finalizeBufferedReviewOutcomes(reviewResults, reviewRepo, reviewPrNum, reviewResult); logCreatedItemFromResult(logCreatedItem, "submit_pull_request_review", reviewResult); logCreatedItemFromResult( logCreatedItem, @@ -2338,7 +2341,7 @@ async function main(options = {}) { if (reviewResult.pr_locked) { core.setOutput("pr_locked", "true"); } - skipReviewResultsForPR(results, reviewRepo, reviewPrNum, skipReason); + skipReviewResultsForPR(reviewResults, 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}`); @@ -2349,7 +2352,7 @@ async function main(options = {}) { } if (reviewFailureError !== null) { - rollbackReviewResultsForPR(results, reviewRepo, reviewPrNum, reviewFailureError); + rollbackReviewResultsForPR(reviewResults, reviewRepo, reviewPrNum, reviewFailureError); } } } diff --git a/actions/setup/js/safe_output_outcomes.test.cjs b/actions/setup/js/safe_output_outcomes.test.cjs index b288a7d8ad6..b5587569da9 100644 --- a/actions/setup/js/safe_output_outcomes.test.cjs +++ b/actions/setup/js/safe_output_outcomes.test.cjs @@ -287,7 +287,7 @@ describe("safe-output application outcomes", () => { expect(JSON.stringify(outcome)).not.toContain("PRIVATE"); }); - it.each(["success", "chained", "chained-comment", "failed", "skipped", "staged"])("resolves a check depending on a buffered review after %s submission", async scenario => { + it.each(["success", "chained", "chained-comment", "chained-comment-failed", "failed", "skipped", "staged"])("resolves a check depending on a buffered review after %s submission", async scenario => { const pullRequest = { number: 1, state: "open", head: { sha: "abc123" }, user: { login: "review-author" } }; const summary = { addRaw: vi.fn().mockReturnThis(), write: vi.fn().mockResolvedValue(undefined) }; vi.stubGlobal("core", { info: vi.fn(), debug: vi.fn(), warning: vi.fn(), error: vi.fn(), setOutput: vi.fn(), setFailed: vi.fn(), summary }); @@ -295,7 +295,7 @@ describe("safe-output application outcomes", () => { const calls = []; const createReview = vi.fn(async ({ pull_number }) => { calls.push("review"); - if (scenario === "failed") throw Object.assign(new Error("Review rejected"), { status: 422 }); + if (scenario === "failed" || (scenario === "chained-comment-failed" && calls.length === 2)) throw Object.assign(new Error("Review rejected"), { status: 422 }); return { data: { id: pull_number + 4, html_url: `https://github.com/org/repo/pull/${pull_number}#pullrequestreview-${pull_number + 4}` } }; }); const createCheck = vi.fn(async () => { @@ -323,7 +323,7 @@ describe("safe-output application outcomes", () => { messages: [ { type: "submit_pull_request_review", output_id: "so_000000000001", pull_request_number: 1, body: "PRIVATE_REVIEW_BODY", event: "COMMENT" }, ...(scenario === "chained" ? [{ type: "submit_pull_request_review", output_id: "so_000000000003", "depends-on": "so_000000000001", pull_request_number: 2, body: "PRIVATE_SECOND_REVIEW_BODY", event: "COMMENT" }] : []), - ...(scenario === "chained-comment" ? [{ type: "create_pull_request_review_comment", output_id: "so_000000000003", "depends-on": "so_000000000001", path: "file.js", line: 1, body: "PRIVATE_INLINE_COMMENT" }] : []), + ...(scenario.startsWith("chained-comment") ? [{ type: "create_pull_request_review_comment", output_id: "so_000000000003", "depends-on": "so_000000000001", path: "file.js", line: 1, body: "PRIVATE_INLINE_COMMENT" }] : []), { type: "create_check_run", output_id: "so_000000000002", "depends-on": scenario.startsWith("chained") ? "so_000000000003" : "so_000000000001", conclusion: "success", title: "Review complete", summary: "PRIVATE_CHECK_BODY" }, ], }); @@ -333,7 +333,7 @@ describe("safe-output application outcomes", () => { .trim() .split("\n") .map(line => JSON.parse(line)); - if (scenario === "success" || scenario.startsWith("chained")) { + if (scenario === "success" || scenario === "chained" || scenario === "chained-comment") { expect(calls).toEqual(scenario.startsWith("chained") ? ["review", "review", "check"] : ["review", "check"]); expect(global.core.setFailed).not.toHaveBeenCalled(); expect(outcomes.every(outcome => outcome.status === "success")).toBe(true); @@ -346,6 +346,11 @@ describe("safe-output application outcomes", () => { expect(createCheck).not.toHaveBeenCalled(); expect(global.core.setFailed).toHaveBeenCalled(); expect(outcomes.find(outcome => outcome.type === "create_check_run").status).toBe("failed"); + if (scenario === "chained-comment-failed") { + expect(calls).toEqual(["review", "review"]); + expect(outcomes.find(outcome => outcome.type === "submit_pull_request_review").status).toBe("success"); + expect(outcomes.find(outcome => outcome.type === "create_pull_request_review_comment").status).toBe("failed"); + } } expect(JSON.stringify(outcomes)).not.toContain("PRIVATE"); }); diff --git a/docs/src/content/docs/reference/safe-outputs.md b/docs/src/content/docs/reference/safe-outputs.md index 10e789007ad..e397cb4cb38 100644 --- a/docs/src/content/docs/reference/safe-outputs.md +++ b/docs/src/content/docs/reference/safe-outputs.md @@ -95,9 +95,10 @@ Dependencies apply within a single safe-output processing pass (and within the same Claim in queue workflows). Custom safe jobs, `call_workflow`, `push_repo_memory`, `upload_asset`, `noop`, and out-of-band diagnostics (`report_incomplete`, `report_missing_tool`, `report_missing_data`) reject -`depends-on`. The effects of `upload_code_coverage` run in a later job, so -staging it cannot establish a successful prerequisite. Dependencies do not -provide rollback for operations that have already succeeded. +`depends-on`. `upload_code_coverage`, `dispatch_workflow`, `dispatch_repository`, +and custom action outputs delegate effects outside this manager, so they cannot +establish successful prerequisites. Dependencies do not provide rollback for +operations that have already succeeded. ## Available Safe Output Types From 57ffa160d38bad6caf5728a8639b67eed3c6dc8a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 6 Nov 2026 14:51:34 +0000 Subject: [PATCH 5/5] Fix buffered review retry progress and unify dependency outcomes Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- actions/setup/js/safe_output_dependencies.cjs | 7 +-- .../js/safe_output_dependencies.test.cjs | 49 ++++++++++++++++++- .../setup/js/safe_output_handler_manager.cjs | 24 ++++++--- actions/setup/js/safe_output_outcomes.cjs | 3 +- .../setup/js/safe_output_outcomes.test.cjs | 12 ++++- actions/setup/js/safe_outputs_status.cjs | 2 +- actions/setup/js/submit_pr_review.cjs | 2 +- .../docs/specs/safe-outputs-specification.md | 21 +++++++- 8 files changed, 102 insertions(+), 18 deletions(-) diff --git a/actions/setup/js/safe_output_dependencies.cjs b/actions/setup/js/safe_output_dependencies.cjs index c9f70a3e4f7..30f80c4ed40 100644 --- a/actions/setup/js/safe_output_dependencies.cjs +++ b/actions/setup/js/safe_output_dependencies.cjs @@ -126,7 +126,7 @@ function isConfirmedDependencySuccess(record) { // @safe-outputs-exempt USE-003: reads the staged flag only to reject unconfirmed success; renders no staged preview if (process.env.GH_AW_SAFE_OUTPUTS_STAGED === "true") return false; if (!record || record.success !== true || classifySafeOutputResult(record) !== "success") return false; - if (pickOutcomeField(record, "staged") || pickOutcomeField(record, "queued") || pickOutcomeField(record, "buffered") || pickOutcomeField(record, "deferred_manifest")) return false; + if (pickOutcomeField(record, "staged") || pickOutcomeField(record, "queued") || pickOutcomeField(record, "buffered")) return false; const result = record.result; if (Array.isArray(result)) { return result.length > 0 && result.every(item => item && typeof item === "object" && isConfirmedDependencySuccess({ success: item.success !== false, result: item })); @@ -150,11 +150,12 @@ function checkSafeOutputDependencies(plan, index, results, allowWaiting) { if (records.length > 1) return { state: "failed", errorCode: "safe_output_dependency_failed", error: "Ambiguous safe-output prerequisite result" }; const record = records[0]; if (isConfirmedDependencySuccess(record)) continue; - if (allowWaiting && (!record || classifySafeOutputResult(record) === "deferred" || (record.success === true && pickOutcomeField(record, "deferred_manifest")))) { + if (allowWaiting && (!record || classifySafeOutputResult(record) === "deferred")) { waiting = true; continue; } - return { state: "failed", errorCode: "safe_output_dependency_failed", error: "Safe-output prerequisite did not complete successfully" }; + const skipReason = pickOutcomeField(record, "skipReason") || (pickOutcomeField(record, "skipped") && pickOutcomeField(record, "reason")); + return { state: "failed", errorCode: "safe_output_dependency_failed", error: `Safe-output prerequisite did not complete successfully${skipReason ? `: ${skipReason}` : ""}` }; } return { state: waiting ? "waiting" : "ready" }; } diff --git a/actions/setup/js/safe_output_dependencies.test.cjs b/actions/setup/js/safe_output_dependencies.test.cjs index ca37a5722a9..63584889248 100644 --- a/actions/setup/js/safe_output_dependencies.test.cjs +++ b/actions/setup/js/safe_output_dependencies.test.cjs @@ -2,7 +2,8 @@ import { beforeEach, afterEach, describe, expect, it, vi } from "vitest"; import { processMessages, sortMessageIndicesByTemporaryIdDependencies } from "./safe_output_handler_manager.cjs"; -import { buildSafeOutputDependencyPlan, isConfirmedDependencySuccess } from "./safe_output_dependencies.cjs"; +import { buildSafeOutputDependencyPlan, checkSafeOutputDependencies, isConfirmedDependencySuccess } from "./safe_output_dependencies.cjs"; +import { classifySafeOutputResult, computeSafeOutputsStatus } from "./safe_outputs_status.cjs"; import { main as createActionHandler } from "./safe_output_action_handler.cjs"; import { main as createCallWorkflowHandler } from "./call_workflow.cjs"; import { createHandlers } from "./safe_outputs_handlers.cjs"; @@ -237,6 +238,52 @@ describe("safe-output runtime dependencies", () => { expect(result.results.every(item => item.success)).toBe(true); }); + it("finalizes a review buffered by an in-place retry without result-array growth", async () => { + const calls = []; + let published = false; + const handler = vi.fn(async msg => { + calls.push(msg.output_id); + if (msg.output_id === id(1)) return { success: true, deferred_manifest: true }; + if (msg.output_id === id(2)) return published ? { success: true, deferred_manifest: true } : { deferred: true }; + return { success: true }; + }); + const finalize = vi.fn(async results => { + published = true; + for (const record of results) { + if (record.result?.deferred_manifest) record.result.deferred_manifest = false; + } + }); + const result = await processMessages(new Map([["add_comment", handler]]), [message(1), message(2), message(3, id(2))], null, finalize); + expect(calls).toEqual([id(1), id(2), id(2), id(2), id(3)]); + expect(finalize).toHaveBeenCalledTimes(2); + expect(result.results.every(item => classifySafeOutputResult(item) === "success")).toBe(true); + }); + + it("does not retry unresolved handlers again when finalization makes no resolution progress", async () => { + const handler = vi.fn(async () => ({ deferred: true })); + const finalize = vi.fn(async () => {}); + const result = await processMessages(new Map([["add_comment", handler]]), [message(1)], null, finalize); + expect(handler).toHaveBeenCalledTimes(2); + expect(result.results[0]).toMatchObject({ deferred: true }); + }); + + it("classifies buffered review publication consistently across consumers", () => { + const record = { messageIndex: 0, success: true, result: { deferred_manifest: true } }; + expect(classifySafeOutputResult(record)).toBe("deferred"); + expect(computeSafeOutputsStatus([record])).toMatchObject({ itemsApplied: 0, itemsDeferred: 1, status: "deferred" }); + expect(buildSafeOutputOutcomes([record])[0].status).toBe("deferred"); + }); + + it("fails a skipped review prerequisite immediately with its skip reason", () => { + const plan = buildSafeOutputDependencyPlan([message(1), message(2, id(1))]); + const record = { messageIndex: 0, success: true, skipped: true, skipReason: "PR is closed", result: { deferred_manifest: true } }; + expect(checkSafeOutputDependencies(plan, 1, [record], true)).toEqual({ + state: "failed", + errorCode: "safe_output_dependency_failed", + error: "Safe-output prerequisite did not complete successfully: PR is closed", + }); + }); + it("waits for a prerequisite's temporary-ID resolution on retry", async () => { const calls = []; const link = vi.fn(async (_message, temporaryIds) => { diff --git a/actions/setup/js/safe_output_handler_manager.cjs b/actions/setup/js/safe_output_handler_manager.cjs index 92540ed3314..3c580bebbb9 100644 --- a/actions/setup/js/safe_output_handler_manager.cjs +++ b/actions/setup/js/safe_output_handler_manager.cjs @@ -836,7 +836,7 @@ function rollbackReviewResultsForPR(results, repo, prNumber, errorMessage) { * the skip must be back-propagated here so the Processing Summary reflects the actual * outcome (skipped) rather than a misleading success count. * - * @param {Array<{type: string, success: boolean, skipped?: boolean, skipReason?: string}>} results - Processing results to mutate + * @param {Array<{type: string, success: boolean, skipped?: boolean, skipReason?: string, deferred_manifest?: boolean, result?: {deferred_manifest?: boolean}}>} results - Processing results to mutate * @param {string} skipReason - Human-readable reason for the skip */ function skipReviewResults(results, skipReason) { @@ -844,6 +844,8 @@ function skipReviewResults(results, skipReason) { if ((r.type === "submit_pull_request_review" || r.type === "create_pull_request_review_comment") && r.success === true) { r.skipped = true; r.skipReason = skipReason; + r.deferred_manifest = false; + if (r.result) r.result.deferred_manifest = false; } } } @@ -851,7 +853,7 @@ 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 {Array<{type: string, success: boolean, skipped?: boolean, skipReason?: string, deferred_manifest?: boolean, repo?: string, pull_request_number?: number, result?: {repo?: string, pull_request_number?: number, deferred_manifest?: boolean}}>} results * @param {string} repo - Repository slug (owner/repo) * @param {number} prNumber - Pull request number * @param {string} skipReason - Human-readable reason for the skip @@ -864,6 +866,8 @@ function skipReviewResultsForPR(results, repo, prNumber, skipReason) { 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; + r.deferred_manifest = false; + if (r.result) r.result.deferred_manifest = false; } } } @@ -1658,13 +1662,19 @@ async function processMessages(messageHandlers, messages, onItemCreated = null, await retryDeferredMessages(); if (finalizeBufferedOutputs) { - let previousResultCount; - do { + for (let pass = 0; pass <= messages.length; pass++) { + const pendingPublications = results.filter(record => record.success && !record.skipped && record.result?.deferred_manifest); + const pendingResults = new Set(pendingPublications.map(record => record.result)); + const temporaryIdCount = temporaryIdMap.size; + const deferredCount = deferredMessages.length; await finalizeBufferedOutputs(results); - previousResultCount = results.length; + const publicationProgress = pendingPublications.some(record => classifySafeOutputResult(record) !== "deferred"); await drainWaitingMessages(); - await retryDeferredMessages(); - } while (results.length > previousResultCount); + if (publicationProgress || temporaryIdMap.size !== temporaryIdCount || deferredMessages.length !== deferredCount) { + await retryDeferredMessages(); + } + if (!results.some(record => record.success && !record.skipped && record.result?.deferred_manifest && !pendingResults.has(record.result))) break; + } } for (const i of waitingMessages) { await processMessage(i, false); diff --git a/actions/setup/js/safe_output_outcomes.cjs b/actions/setup/js/safe_output_outcomes.cjs index 86a27276ce3..f5b2c5a16ef 100644 --- a/actions/setup/js/safe_output_outcomes.cjs +++ b/actions/setup/js/safe_output_outcomes.cjs @@ -85,8 +85,7 @@ function buildSafeOutputOutcomes(results, staged = false) { const items = fanout ? (record.result.length ? record.result : [undefined]) : [record.result]; return items.map(result => { const item = fanout ? { ...record, result, success: record.success === false ? false : (result?.success ?? record.success), ...(record.result.length ? {} : { skipped: true }) } : record; - let status = classifySafeOutputResult(item); - if (status === "success" && pickOutcomeField(item, "deferred_manifest")) status = "deferred"; + const status = classifySafeOutputResult(item); const isStaged = staged || pickOutcomeField(item, "staged") === true; const url = !isStaged && (status === "success" || status === "warning") ? RESULT_URL_FIELDS.map(field => canonicalSafeOutputUrl(result?.[field])).find(Boolean) : undefined; return { diff --git a/actions/setup/js/safe_output_outcomes.test.cjs b/actions/setup/js/safe_output_outcomes.test.cjs index b5587569da9..3182351b7c9 100644 --- a/actions/setup/js/safe_output_outcomes.test.cjs +++ b/actions/setup/js/safe_output_outcomes.test.cjs @@ -42,6 +42,8 @@ describe("safe-output application outcomes", () => { const skipped = { ...result, success: true }; skipReviewResultsForPR([skipped], "org/repo", 1, "PRIVATE_REASON"); expect(buildSafeOutputOutcomes([skipped])[0]).toMatchObject({ status: "skipped" }); + expect(skipped.result.deferred_manifest).toBe(false); + expect(skipped.skipReason).toBe("PRIVATE_REASON"); expect(buildSafeOutputOutcomes([{ success: true }], true)[0]).toMatchObject({ status: "staged", staged: true }); }); @@ -287,7 +289,7 @@ describe("safe-output application outcomes", () => { expect(JSON.stringify(outcome)).not.toContain("PRIVATE"); }); - it.each(["success", "chained", "chained-comment", "chained-comment-failed", "failed", "skipped", "staged"])("resolves a check depending on a buffered review after %s submission", async scenario => { + it.each(["success", "chained", "chained-comment", "chained-comment-failed", "failed", "skipped", "skipped-after-buffering", "staged"])("resolves a check depending on a buffered review after %s submission", async scenario => { const pullRequest = { number: 1, state: "open", head: { sha: "abc123" }, user: { login: "review-author" } }; const summary = { addRaw: vi.fn().mockReturnThis(), write: vi.fn().mockResolvedValue(undefined) }; vi.stubGlobal("core", { info: vi.fn(), debug: vi.fn(), warning: vi.fn(), error: vi.fn(), setOutput: vi.fn(), setFailed: vi.fn(), summary }); @@ -306,7 +308,9 @@ describe("safe-output application outcomes", () => { rest: { rateLimit: { get: vi.fn().mockResolvedValue({ data: { resources: { core: { remaining: 5000, limit: 5000, reset: 0 } } } }) }, pulls: { - get: vi.fn().mockResolvedValue({ data: { ...pullRequest, ...(scenario === "skipped" ? { state: "closed" } : {}) } }), + get: vi.fn(async () => ({ + data: { ...pullRequest, ...(scenario === "skipped" || (scenario === "skipped-after-buffering" && global.core.info.mock.calls.some(([text]) => String(text).includes("=== Finalizing PR Review"))) ? { state: "closed" } : {}) }, + })), listReviews: vi.fn().mockResolvedValue({ data: [] }), listFiles: vi.fn().mockResolvedValue({ data: [{ filename: "file.js" }] }), createReview, @@ -346,6 +350,10 @@ describe("safe-output application outcomes", () => { expect(createCheck).not.toHaveBeenCalled(); expect(global.core.setFailed).toHaveBeenCalled(); expect(outcomes.find(outcome => outcome.type === "create_check_run").status).toBe("failed"); + if (scenario.startsWith("skipped")) { + expect(outcomes.find(outcome => outcome.type === "submit_pull_request_review").status).toBe("skipped"); + expect(global.core.setFailed).toHaveBeenCalledWith(expect.stringContaining("Safe-output prerequisite did not complete successfully: Pull request #1 closed after this workflow was triggered")); + } if (scenario === "chained-comment-failed") { expect(calls).toEqual(["review", "review"]); expect(outcomes.find(outcome => outcome.type === "submit_pull_request_review").status).toBe("success"); diff --git a/actions/setup/js/safe_outputs_status.cjs b/actions/setup/js/safe_outputs_status.cjs index 7e43d5c2938..f6a789d6c04 100644 --- a/actions/setup/js/safe_outputs_status.cjs +++ b/actions/setup/js/safe_outputs_status.cjs @@ -36,7 +36,7 @@ function classifySafeOutputResult(result) { // A skipped operation did not apply a mutation, even when it carries a warning. if (pickOutcomeField(result, "skipped")) return "skipped"; if (pickOutcomeField(result, "warning")) return "warning"; - if (pickOutcomeField(result, "success")) return "success"; + if (pickOutcomeField(result, "success")) return pickOutcomeField(result, "deferred_manifest") ? "deferred" : "success"; return "failed"; } diff --git a/actions/setup/js/submit_pr_review.cjs b/actions/setup/js/submit_pr_review.cjs index 107078116b4..3d8b5c5495f 100644 --- a/actions/setup/js/submit_pr_review.cjs +++ b/actions/setup/js/submit_pr_review.cjs @@ -212,7 +212,7 @@ async function main(config = {}) { if (filterResult) return filterResult; const reviewTarget = `${repo}#${prNum}`; - if (buffer.hasReviewMetadata() || reviewTargets.has(reviewTarget)) { + if (reviewTargets.has(reviewTarget)) { 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 }; diff --git a/docs/src/content/docs/specs/safe-outputs-specification.md b/docs/src/content/docs/specs/safe-outputs-specification.md index 16d05f6879f..2e80ad09671 100644 --- a/docs/src/content/docs/specs/safe-outputs-specification.md +++ b/docs/src/content/docs/specs/safe-outputs-specification.md @@ -7,7 +7,7 @@ sidebar: # Safe Outputs MCP Gateway Specification -**Version**: 1.29.18
+**Version**: 1.29.19
**Status**: Working Draft
**Publication Date**: 2026-11-06
**Editor**: GitHub Agentic Workflows Team
@@ -2502,6 +2502,18 @@ If an optional advisory or enrichment field is declared with `x-strip-on-error: Fields used for privileged transport metadata, including patch anchoring and upload asset file metadata, MUST be derived by trusted workflow steps or privileged processors rather than accepted from agent-controlled NDJSON. +### 7.0.4 Dependency and Batch Processing Semantics + +**Requirement DSB1 (Eligible Review Prerequisites):** Implementations MUST permit `submit_pull_request_review` and `create_pull_request_review_comment` outputs to serve as `depends-on` prerequisites. + +**Requirement DSB2 (Review Buffer Drain):** Implementations MUST drain and finalize the applicable buffered review batch before releasing outputs that depend on it. + +**Requirement DSB3 (Publication-Based Success):** Buffering MUST NOT establish prerequisite success; success requires confirmed publication. Failed, skipped, staged, queued, or otherwise unconfirmed outputs MUST NOT establish prerequisite success. Unsupported or delegated outputs, including custom jobs and standalone steps, MUST NOT establish prerequisite success. + +**Requirement DSB4 (Deferred-Dependent Retry):** Newly eligible deferred dependents MUST be retried without replaying completed handlers. Implementations MUST finalize newly buffered batches, including batches created by in-place retries, before releasing their dependents. Retries MUST be bounded and contingent on resolution progress; unresolved prerequisites MUST fail closed. + +**Requirement DSB5 (Single Explicit Review Submission):** Implementations MUST preserve at most one explicit `submit_pull_request_review` per pull request per run across drained batches. + ### 7.1 Core Issue Operations #### Type: create_issue @@ -4217,6 +4229,7 @@ or review bodies to returned or persisted decision evidence. **Notes**: - Comments buffered via PR review buffer for batch submission +- Publication and dependent-output release MUST follow [§7.0.4](#704-dependency-and-batch-processing-semantics). - Higher default max (10) enables comprehensive code review - When `safe-outputs.create-pull-request-review-comment.target` is `"*"`, requests MUST include `pull_request_number`. @@ -4244,6 +4257,7 @@ or review bodies to returned or persisted decision evidence. **Notes**: - Submits all buffered review comments from `create_pull_request_review_comment` +- Publication, dependent-output release, and the per-run explicit-review limit MUST follow [§7.0.4](#704-dependency-and-batch-processing-semantics). - Review status affects PR merge requirements - **Target**: `target` accepts `"triggering"` (default), `"*"` (request MUST include `pull_request_number`), or an explicit PR number (e.g. `${{ github.event.inputs.pr_number }}`). Required when the workflow is not triggered by a pull request (e.g. `workflow_dispatch`). - **Cross-Repository**: `target-repo` specifies a repository in `owner/repo` format to submit reviews on PRs in another repo. Use `allowed-repos` to permit additional repositories. @@ -6745,6 +6759,11 @@ This specification revision aligns with directly relevant `CHANGELOG.md` entries - **Earlier changelog entry**: status comments were decoupled from default AI reaction behavior; explicit `on.status-comment` configuration is required when status comments are desired. - **Earlier changelog entry**: `command` trigger was renamed to `slash_command` with deprecation compatibility. +**Version 1.29.19** (2026-11-06): + +- **Added**: §7.0.4 specifies review prerequisites, publication-confirmed dependency success, bounded batch finalization and deferred retries without replay, and one explicit review per PR per run. +- **Preserved**: Fail-closed dependency handling and rejection of unsupported, delegated, staged, queued, or otherwise unconfirmed prerequisites. + **Version 1.29.18** (2026-11-06): - **Added**: Issue artifact schemas, count and human-friendly UTF-8 size limits, sanitization, authenticated publication, staged previews, typed contracts, and partial-failure requirements for `create_issue`.