Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

32 changes: 23 additions & 9 deletions actions/setup/js/safe_output_handler_manager.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -363,8 +363,10 @@ async function processMessages(messageHandlers, messages, onItemCreated = null)
continue;
}

// Fail-fast: if a previous code-push operation failed, cancel non-code-push messages
if (codePushFailures.length > 0 && !CODE_PUSH_TYPES.has(messageType)) {
// Fail-fast: if a previous code-push operation failed, cancel non-code-push messages.
// Exception: add_comment messages are allowed through so the status comment still reaches
// the user — they will be annotated with a failure note (see effectiveMessage logic below).
if (codePushFailures.length > 0 && !CODE_PUSH_TYPES.has(messageType) && messageType !== "add_comment") {
const cancelReason = `Cancelled: code push operation failed (${codePushFailures[0].type}: ${codePushFailures[0].error})`;
core.info(`⏭ Message ${i + 1} (${messageType}) cancelled — ${cancelReason}`);
results.push({
Expand Down Expand Up @@ -450,14 +452,26 @@ async function processMessages(messageHandlers, messages, onItemCreated = null)
// Record the temp ID map size before processing to detect new IDs
const tempIdMapSizeBefore = temporaryIdMap.size;

// If a previous code-push operation fell back to a review issue, prepend a correction
// note to add_comment bodies so the posted comment accurately reflects the outcome.
// The note is placed before the AI-generated body so users see the clarification immediately.
// For add_comment messages: prepend any relevant correction notes before the AI-generated
// body so users see the clarification immediately.
let effectiveMessage = message;
if (messageType === "add_comment" && codePushFallbackInfo) {
const fallbackNote = `\n\n---\n> [!NOTE]\n> The pull request was not created — a fallback review issue was created instead due to protected file changes: [#${codePushFallbackInfo.issueNumber}](${codePushFallbackInfo.issueUrl})\n\n`;
effectiveMessage = { ...message, body: fallbackNote + (message.body || "") };
core.info(`Prepending fallback correction note to add_comment body (fallback issue: #${codePushFallbackInfo.issueNumber})`);
if (messageType === "add_comment") {
// If a previous code-push operation fell back to a review issue, prepend a correction note
// so the posted comment accurately reflects the outcome.
if (codePushFallbackInfo) {
const fallbackNote = `\n\n---\n> [!NOTE]\n> The pull request was not created — a fallback review issue was created instead due to protected file changes: [#${codePushFallbackInfo.issueNumber}](${codePushFallbackInfo.issueUrl})\n\n`;
effectiveMessage = { ...effectiveMessage, body: fallbackNote + (effectiveMessage.body || "") };
core.info(`Prepending fallback correction note to add_comment body (fallback issue: #${codePushFallbackInfo.issueNumber})`);
}
// If a previous code-push operation failed outright (e.g. patch application error),
// prepend a failure warning so the status comment accurately reflects that the
// code changes were not applied.
if (codePushFailures.length > 0) {
const failure = codePushFailures[0];
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

add_comment failure annotation always uses codePushFailures[0]. If multiple code-push operations occur (they’re explicitly not cancelled) or a later code-push succeeds, the warning can become stale/misleading (e.g., it may report an earlier failure even though a later create_pull_request succeeded). Consider tracking the most recent code-push outcome (e.g., use the last failure, and/or clear the failure state on subsequent code-push success) so the comment reflects the final state users care about.

Suggested change
const failure = codePushFailures[0];
const failure = codePushFailures[codePushFailures.length - 1];

Copilot uses AI. Check for mistakes.
const failureNote = `\n\n---\n> [!WARNING]\n> The \`${failure.type}\` operation failed: ${failure.error}. The code changes were not applied.\n\n`;
effectiveMessage = { ...effectiveMessage, body: failureNote + (effectiveMessage.body || "") };
core.info(`Prepending code push failure note to add_comment body (${failure.type}: ${failure.error})`);
Comment on lines +471 to +473
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

failure.error is interpolated directly into a GitHub callout blockquote. If the error message contains newlines, only the first line will be inside the > [!WARNING] blockquote and the remaining lines will render outside the callout (and can disrupt formatting). Consider normalizing multi-line errors for markdown (e.g., prefix each line consistently within the quote, or wrap details in a <details> / code block) before prepending.

Suggested change
const failureNote = `\n\n---\n> [!WARNING]\n> The \`${failure.type}\` operation failed: ${failure.error}. The code changes were not applied.\n\n`;
effectiveMessage = { ...effectiveMessage, body: failureNote + (effectiveMessage.body || "") };
core.info(`Prepending code push failure note to add_comment body (${failure.type}: ${failure.error})`);
const failureErrorText = typeof failure.error === "string" ? failure.error : String(failure.error);
const normalizedFailureError = failureErrorText.replace(/\r?\n/g, "\n> ");
const failureNote = `\n\n---\n> [!WARNING]\n> The \`${failure.type}\` operation failed: ${normalizedFailureError}. The code changes were not applied.\n\n`;
effectiveMessage = { ...effectiveMessage, body: failureNote + (effectiveMessage.body || "") };
core.info(`Prepending code push failure note to add_comment body (${failure.type}: ${normalizedFailureError})`);

Copilot uses AI. Check for mistakes.
}
}

// Call the message handler with the individual message and resolved temp IDs
Expand Down
83 changes: 71 additions & 12 deletions actions/setup/js/safe_output_handler_manager.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -964,11 +964,11 @@ describe("Safe Output Handler Manager", () => {
});

describe("code-push fail-fast behaviour", () => {
it("should cancel subsequent messages when push_to_pull_request_branch fails", async () => {
it("should cancel subsequent non-add_comment messages when push_to_pull_request_branch fails", async () => {
const messages = [{ type: "push_to_pull_request_branch" }, { type: "add_comment", body: "Success!" }, { type: "create_issue", title: "Issue" }];

const codePushHandler = vi.fn().mockResolvedValue({ success: false, error: "Branch not found" });
const commentHandler = vi.fn();
const commentHandler = vi.fn().mockResolvedValue([{ _tracking: null }]);
const issueHandler = vi.fn();

const handlers = new Map([
Expand All @@ -987,22 +987,24 @@ describe("Safe Output Handler Manager", () => {
// First result: code-push failed
expect(result.results[0].success).toBe(false);
expect(result.results[0].error).toBe("Branch not found");
// Subsequent results: cancelled
expect(result.results[1].success).toBe(false);
expect(result.results[1].cancelled).toBe(true);
expect(result.results[1].reason).toContain("Cancelled");
// add_comment is NOT cancelled — it should be called with a failure note prepended
expect(result.results[1].cancelled).toBeUndefined();
expect(commentHandler).toHaveBeenCalledTimes(1);
const calledMessage = commentHandler.mock.calls[0][0];
expect(calledMessage.body).toContain("push_to_pull_request_branch");
expect(calledMessage.body).toContain("Branch not found");
// create_issue IS still cancelled (non-add_comment non-code-push type)
expect(result.results[2].success).toBe(false);
expect(result.results[2].cancelled).toBe(true);
// Subsequent handlers were NOT called
expect(commentHandler).not.toHaveBeenCalled();
// create_issue handler was NOT called
expect(issueHandler).not.toHaveBeenCalled();
});

it("should cancel subsequent messages when create_pull_request fails via exception", async () => {
it("should allow add_comment through when create_pull_request fails via exception", async () => {
const messages = [{ type: "create_pull_request" }, { type: "add_comment", body: "PR created!" }];

const codePushHandler = vi.fn().mockRejectedValue(new Error("API error"));
const commentHandler = vi.fn();
const commentHandler = vi.fn().mockResolvedValue([{ _tracking: null }]);

const handlers = new Map([
["create_pull_request", codePushHandler],
Expand All @@ -1015,8 +1017,13 @@ describe("Safe Output Handler Manager", () => {
expect(result.codePushFailures).toHaveLength(1);
expect(result.codePushFailures[0].type).toBe("create_pull_request");
expect(result.codePushFailures[0].error).toBe("API error");
expect(result.results[1].cancelled).toBe(true);
expect(commentHandler).not.toHaveBeenCalled();
// add_comment is NOT cancelled — handler is called with failure note
expect(result.results[1].cancelled).toBeUndefined();
expect(commentHandler).toHaveBeenCalledTimes(1);
const calledMessage = commentHandler.mock.calls[0][0];
expect(calledMessage.body).toContain("create_pull_request");
expect(calledMessage.body).toContain("API error");
expect(calledMessage.body).toContain("PR created!");
});

it("should NOT cancel subsequent code-push messages after a code-push failure", async () => {
Expand Down Expand Up @@ -1213,6 +1220,58 @@ describe("Safe Output Handler Manager", () => {
expect(calledMessage.body).toBe("A fix PR has been created.");
expect(calledMessage.body).not.toContain("pull request was not created");
});

it("should prepend failure note to add_comment body when create_pull_request fails (e.g. patch application error)", async () => {
const messages = [
{ type: "create_pull_request", title: "My Fix PR" },
{ type: "add_comment", body: "The agent has completed its work." },
];

const prHandler = vi.fn().mockResolvedValue({ success: false, error: "Failed to apply patch" });
const commentHandler = vi.fn().mockResolvedValue([{ _tracking: null }]);

const handlers = new Map([
["create_pull_request", prHandler],
["add_comment", commentHandler],
]);

await processMessages(handlers, messages);

// add_comment handler must have been called (not cancelled)
expect(commentHandler).toHaveBeenCalledTimes(1);
const calledMessage = commentHandler.mock.calls[0][0];
// Failure note should be prepended
expect(calledMessage.body).toContain("create_pull_request");
expect(calledMessage.body).toContain("Failed to apply patch");
expect(calledMessage.body).toContain("The agent has completed its work.");
// Note should appear before the original body
const noteIndex = calledMessage.body.indexOf("create_pull_request");
const bodyIndex = calledMessage.body.indexOf("The agent has completed its work.");
expect(noteIndex).toBeLessThan(bodyIndex);
});

it("should prepend failure note to add_comment body when push_to_pull_request_branch fails", async () => {
const messages = [
{ type: "push_to_pull_request_branch", branch: "fix-branch" },
{ type: "add_comment", body: "Changes have been pushed." },
];

const pushHandler = vi.fn().mockResolvedValue({ success: false, error: "Branch not found" });
const commentHandler = vi.fn().mockResolvedValue([{ _tracking: null }]);

const handlers = new Map([
["push_to_pull_request_branch", pushHandler],
["add_comment", commentHandler],
]);

await processMessages(handlers, messages);

expect(commentHandler).toHaveBeenCalledTimes(1);
const calledMessage = commentHandler.mock.calls[0][0];
expect(calledMessage.body).toContain("push_to_pull_request_branch");
expect(calledMessage.body).toContain("Branch not found");
expect(calledMessage.body).toContain("Changes have been pushed.");
});
});

describe("output emission via emitSafeOutputActionOutputs", () => {
Expand Down
Loading