Skip to content

Fix temporary ID substitution in safe-output comments #57955

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 3 commits into from
Sep 2, 2026
+101 −8
Merged

Fix temporary ID substitution in safe-output comments #57955

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7e38026
Fix temporary ID updates in comments
Copilot Sep 2, 2026
5a6bff2
Preserve add-comment metadata during ID updates
Copilot Sep 2, 2026
6d81653
Refine legacy comment tracking metadata
Copilot Sep 2, 2026
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
26 changes: 18 additions & 8 deletions actions/setup/js/safe_output_handler_manager.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1166,7 +1166,21 @@ async function processMessages(messageHandlers, messages, onItemCreated = null)
// Check if this output was created with unresolved temporary IDs
// For create_issue, create_discussion, add_comment - check if body has unresolved IDs

// Handle add_comment which returns an array of comments
// Handle the current add_comment result shape.
if (messageType === "add_comment" && result?.commentId && result?.repo) {

@pelikhan pelikhan Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@copilot review all other safe outputs for different shapes

Copilot AI Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reviewed the safe-output result conventions and updated add_comment tracking to use the current object body while preserving its metadata, with legacy array support retaining tracking metadata and optional body. Regression coverage now exercises the synthetic update request. Changes are in 5a6bff2 and 6d81653.

@github-actions github-actions Bot Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

actions/setup/js/safe_output_handler_manager.cjs:L1170: shrink: two near-duplicate add_comment branches for object and array results. One normalized branch or helper handles both shapes once.

const contentToCheck = getContentToCheck(messageType, message, result);
if (contentToCheck && hasUnresolvedTemporaryIds(contentToCheck, temporaryIdMap, artifactUrlMap)) {
core.info(`Comment ${result.commentId} on ${result.repo}#${result.itemNumber} was created with unresolved temporary IDs - tracking for update`);
outputsWithUnresolvedIds.push({
type: messageType,
message,
result,
originalTempIdMapSize: tempIdMapSizeBefore,
});
}
}

// Handle the legacy add_comment result shape.
if (messageType === "add_comment" && Array.isArray(result)) {

@github-actions github-actions Bot Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Now that add_comment always returns a single object (never an array — confirmed in add_comment.cjs, which never returns createdComments as an array to the caller), this Array.isArray(result) legacy branch appears to be dead code for the add_comment handler. Worth confirming whether any other handler registered for add_comment-type messages can still return an array; if not, consider removing this branch (and the corresponding _tracking bookkeeping in add_comment.cjs) in a follow-up to avoid maintaining an unreachable code path. Not blocking this fix. @copilot please address this.

const contentToCheck = getContentToCheck(messageType, message, result);
if (contentToCheck && hasUnresolvedTemporaryIds(contentToCheck, temporaryIdMap, artifactUrlMap)) {
Expand All @@ -1177,12 +1191,7 @@ async function processMessages(messageHandlers, messages, onItemCreated = null)
outputsWithUnresolvedIds.push({
type: messageType,
message: message,
result: {
commentId: comment._tracking.commentId,
itemNumber: comment._tracking.itemNumber,
repo: comment._tracking.repo,
isDiscussion: comment._tracking.isDiscussion,
},
result: { ...comment._tracking, ...(comment.body ? { body: comment.body } : {}) },
originalTempIdMapSize: tempIdMapSizeBefore,
});
}
Expand Down Expand Up @@ -1384,7 +1393,7 @@ function getContentToCheck(messageType, message, result) {
case "create_discussion":
return message.body || "";
case "add_comment":
return message.body || "";
return result?.body || message.body || "";
case "comment_memory":
return result?.managedBody || message.body || "";
case "create_pull_request":
Expand Down Expand Up @@ -2026,4 +2035,5 @@ module.exports = {
partitionFailureResults,
computeSafeOutputsStatus,
setSafeOutputsStatusOutputs,
processSyntheticUpdates,
};
83 changes: 83 additions & 0 deletions actions/setup/js/safe_output_handler_manager.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
partitionFailureResults,
computeSafeOutputsStatus,
setSafeOutputsStatusOutputs,
processSyntheticUpdates,
} from "./safe_output_handler_manager.cjs";

const require = createRequire(import.meta.url);
Expand Down Expand Up @@ -153,6 +154,88 @@ describe("Safe Output Handler Manager", () => {

expect(sortMessageIndicesByTemporaryIdDependencies([dependent, producer, unrelated])).toEqual([1, 0, 2]);
});

it("tracks a comment emitted before its temporary-ID producer", async () => {
const callOrder = [];
const handlers = new Map([
[
"add_comment",
vi.fn(async (_message, resolvedTemporaryIds) => {
callOrder.push("add_comment");
expect(resolvedTemporaryIds).toEqual({});
return {
success: true,
commentId: 123,
itemNumber: 42,
repo: "owner/repo",
isDiscussion: false,
body: "Tracking issue: #aw_track1\n\nHandler footer marker",
};
}),
],
[
"create_issue",
vi.fn(async () => {
callOrder.push("create_issue");
return { success: true, temporaryId: "aw_track1", repo: "owner/tracker", number: 99 };
}),
],
]);
const messages = [
{ type: "add_comment", item_number: 42, body: "Tracking issue: #aw_track1" },
{ type: "create_issue", temporary_id: "aw_track1", title: "Tracking issue" },
];

const result = await processMessages(handlers, messages);

expect(callOrder).toEqual(["add_comment", "create_issue"]);
expect(result.temporaryIdMap.aw_track1).toEqual({ repo: "owner/tracker", number: 99 });
expect(result.outputsWithUnresolvedIds).toEqual([
{
type: "add_comment",
message: { type: "add_comment", item_number: 42, body: "Tracking issue: #aw_track1" },
result: {
success: true,
commentId: 123,
itemNumber: 42,
repo: "owner/repo",
isDiscussion: false,
body: "Tracking issue: #aw_track1\n\nHandler footer marker",
},
originalTempIdMapSize: 0,
},
]);
});

it("updates the posted comment body while retaining handler metadata", async () => {
const updateComment = vi.fn().mockResolvedValue({});
const github = { rest: { issues: { updateComment } } };
const trackedOutputs = [
{
type: "add_comment",
message: { type: "add_comment", body: "Tracking issue: #aw_track1" },
result: {
success: true,
commentId: 123,
itemNumber: 42,
repo: "owner/repo",
isDiscussion: false,
body: "Tracking issue: #aw_track1\n\nHandler footer marker",
},
originalTempIdMapSize: 0,
},
];

const updateCount = await processSyntheticUpdates(github, {}, trackedOutputs, new Map([["aw_track1", { repo: "owner/tracker", number: 99 }]]), new Map());

expect(updateCount).toBe(1);
expect(updateComment).toHaveBeenCalledWith({
owner: "owner",
repo: "repo",
comment_id: 123,
body: "Tracking issue: owner/tracker#99\n\nHandler footer marker",
});
});
});

describe("logCreatedItemFromResult", () => {
Expand Down
Loading