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.cjs b/actions/setup/js/safe_output_dependencies.cjs index d31ea22cb62..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 })); @@ -154,7 +154,8 @@ function checkSafeOutputDependencies(plan, index, results, allowWaiting) { 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 2e95aec8206..63584889248 100644 --- a/actions/setup/js/safe_output_dependencies.test.cjs +++ b/actions/setup/js/safe_output_dependencies.test.cjs @@ -2,11 +2,12 @@ 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"; -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 +164,47 @@ 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 => { + if (!results.some(result => result.result?.deferred_manifest)) return; + 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).toHaveBeenCalled(); + 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(); @@ -177,6 +219,71 @@ 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("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 aa2b8e3e178..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; } } } @@ -994,9 +998,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"); } @@ -1516,7 +1521,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)); @@ -1656,6 +1661,21 @@ async function processMessages(messageHandlers, messages, onItemCreated = null) } await retryDeferredMessages(); + if (finalizeBufferedOutputs) { + 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); + const publicationProgress = pendingPublications.some(record => classifySafeOutputResult(record) !== "deferred"); + await drainWaitingMessages(); + 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); } @@ -2289,64 +2309,70 @@ 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. + 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()) { + 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(reviewResults, reviewRepo, reviewPrNum, stalePRReason); + continue; + } + const reviewResult = await reviewBuffer.submitReview(); + if (reviewResult.success && !reviewResult.skipped) { + finalizeBufferedReviewOutcomes(reviewResults, 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(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}`); } - 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(reviewResults, 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..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 { @@ -129,6 +128,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..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,6 +289,80 @@ describe("safe-output application outcomes", () => { expect(JSON.stringify(outcome)).not.toContain("PRIVATE"); }); + 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 }); + vi.stubGlobal("context", { eventName: "pull_request", repo: { owner: "org", repo: "repo" }, payload: { pull_request: pullRequest } }); + const calls = []; + const createReview = vi.fn(async ({ pull_number }) => { + calls.push("review"); + 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 () => { + 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(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, + }, + 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: { 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" }, + ...(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.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" }, + ], + }); + 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" || 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); + 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(); + 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"); + expect(outcomes.find(outcome => outcome.type === "create_pull_request_review_comment").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/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 2626a5c4233..3d8b5c5495f 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 (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 86721e0d2a8..e397cb4cb38 100644 --- a/docs/src/content/docs/reference/safe-outputs.md +++ b/docs/src/content/docs/reference/safe-outputs.md @@ -84,13 +84,21 @@ 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. +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`, `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 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`.