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
28 changes: 26 additions & 2 deletions actions/setup/js/add_comment.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,13 @@ async function main(config = {}) {
return { success: true, commentId: comment.id, url: comment.html_url, itemNumber, repo: itemRepo, isDiscussion: isDiscussionFlag };
};

// Normalize reply_to_id once so both the main discussion path and the
// 404 discussion fallback path use the same validated value.
const normalizedExplicitReplyToId = message.reply_to_id === undefined || message.reply_to_id === null ? null : String(message.reply_to_id).trim();
if (message.reply_to_id !== undefined && message.reply_to_id !== null && !normalizedExplicitReplyToId) {
core.warning("Ignoring empty discussion reply_to_id after normalization");
}

try {
// Hide older comments if enabled AND append-only-comments is not enabled
// When append-only-comments is true, we want to keep all comments visible
Expand All @@ -590,7 +597,18 @@ async function main(config = {}) {
// GitHub Discussions only supports two nesting levels, so if the triggering comment is
// itself a reply, we resolve the top-level parent's node ID to use as replyToId.
const hasExplicitItemNumber = message.item_number !== undefined && message.item_number !== null;
const replyToId = context.eventName === "discussion_comment" && !hasExplicitItemNumber ? await resolveTopLevelDiscussionCommentId(githubClient, context.payload?.comment?.node_id) : null;
let replyToId;
if (context.eventName === "discussion_comment" && !hasExplicitItemNumber) {
// When triggered by a discussion_comment event, thread the reply under the triggering comment.
replyToId = await resolveTopLevelDiscussionCommentId(githubClient, context.payload?.comment?.node_id);
} else if (normalizedExplicitReplyToId) {
// Allow the agent to explicitly specify a reply_to_id (e.g. for workflow_dispatch-triggered
// workflows that know the target comment node ID). Apply resolveTopLevelDiscussionCommentId
// to handle cases where the caller passes a reply node ID instead of a top-level one.
replyToId = await resolveTopLevelDiscussionCommentId(githubClient, normalizedExplicitReplyToId);
} else {
replyToId = null;
}
if (replyToId) {
core.info(`Replying as threaded comment to discussion comment node ID: ${replyToId}`);
}
Expand Down Expand Up @@ -621,7 +639,13 @@ async function main(config = {}) {

try {
core.info(`Trying #${itemNumber} as discussion...`);
const comment = await commentOnDiscussion(githubClient, repoParts.owner, repoParts.repo, itemNumber, processedBody, null);
// When retrying as discussion, honour the normalized reply_to_id from the message.
// Apply resolveTopLevelDiscussionCommentId to handle nested reply node IDs.
const fallbackReplyToId = normalizedExplicitReplyToId ? await resolveTopLevelDiscussionCommentId(githubClient, normalizedExplicitReplyToId) : null;
if (fallbackReplyToId) {
core.info(`Replying as threaded comment to discussion comment node ID: ${fallbackReplyToId}`);
}
const comment = await commentOnDiscussion(githubClient, repoParts.owner, repoParts.repo, itemNumber, processedBody, fallbackReplyToId);

core.info(`Created comment on discussion: ${comment.html_url}`);
return recordComment(comment, true);
Expand Down
305 changes: 305 additions & 0 deletions actions/setup/js/add_comment.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -594,6 +594,311 @@ describe("add_comment", () => {
// Should NOT be threaded since item_number was explicitly provided
expect(capturedReplyToId).toBeUndefined();
});

it("should use reply_to_id from message when not triggered by discussion_comment event", async () => {
const addCommentScript = fs.readFileSync(path.join(__dirname, "add_comment.cjs"), "utf8");

// workflow_dispatch trigger (not discussion_comment); item_number refers to a discussion
mockContext.eventName = "workflow_dispatch";
mockContext.payload = {};

// Make REST API return 404 so the code falls back to the discussion path
mockGithub.rest.issues.createComment = async () => {
const err = new Error("Not Found");
// @ts-expect-error - Simulating GitHub REST API error
err.status = 404;
throw err;
};

let capturedReplyToId = undefined;
mockGithub.graphql = async (query, variables) => {
if (query.includes("addDiscussionComment")) {
capturedReplyToId = variables?.replyToId;
return {
addDiscussionComment: {
comment: {
id: "DC_kwDOTest456",
body: variables?.body,
createdAt: "2024-01-01",
url: "https://github.com/owner/repo/discussions/10#discussioncomment-456",
},
},
};
}
// Mock replyTo resolution: the provided reply_to_id is already top-level (no parent)
if (query.includes("replyTo")) {
return { node: { replyTo: null } };
}
return {
repository: {
discussion: {
id: "D_kwDOTest123",
url: "https://github.com/owner/repo/discussions/10",
},
},
};
};

const handler = await eval(`(async () => { ${addCommentScript}; return await main({}); })()`);

const message = {
type: "add_comment",
item_number: 10,
body: "🔄 Updated finding...",
reply_to_id: "DC_kwDOParentComment456",
};

const result = await handler(message, {});

expect(result.success).toBe(true);
expect(result.isDiscussion).toBe(true);
// Should use the reply_to_id from the message
expect(capturedReplyToId).toBe("DC_kwDOParentComment456");
});

it("should resolve top-level parent when reply_to_id points to a nested reply", async () => {
const addCommentScript = fs.readFileSync(path.join(__dirname, "add_comment.cjs"), "utf8");

// workflow_dispatch trigger (not discussion_comment); item_number refers to a discussion
mockContext.eventName = "workflow_dispatch";
mockContext.payload = {};

// Make REST API return 404 so the code falls back to the discussion path
mockGithub.rest.issues.createComment = async () => {
const err = new Error("Not Found");
// @ts-expect-error - Simulating GitHub REST API error
err.status = 404;
throw err;
};

let capturedReplyToId = undefined;
mockGithub.graphql = async (query, variables) => {
if (query.includes("addDiscussionComment")) {
capturedReplyToId = variables?.replyToId;
return {
addDiscussionComment: {
comment: {
id: "DC_kwDOTest456",
body: variables?.body,
createdAt: "2024-01-01",
url: "https://github.com/owner/repo/discussions/10#discussioncomment-456",
},
},
};
}
// Mock replyTo resolution: the provided reply_to_id is itself a reply, return its parent
if (query.includes("replyTo")) {
return { node: { replyTo: { id: "DC_kwDOTopLevelComment123" } } };
}
return {
repository: {
discussion: {
id: "D_kwDOTest123",
url: "https://github.com/owner/repo/discussions/10",
},
},
};
};

const handler = await eval(`(async () => { ${addCommentScript}; return await main({}); })()`);

const message = {
type: "add_comment",
item_number: 10,
body: "🔄 Updated finding...",
reply_to_id: "DC_kwDONestedReply789",
};

const result = await handler(message, {});

expect(result.success).toBe(true);
expect(result.isDiscussion).toBe(true);
// Should resolve to the top-level parent, not the nested reply
expect(capturedReplyToId).toBe("DC_kwDOTopLevelComment123");
});

it("should ignore reply_to_id when triggered by discussion_comment event without explicit item_number", async () => {
const addCommentScript = fs.readFileSync(path.join(__dirname, "add_comment.cjs"), "utf8");

// discussion_comment trigger takes precedence over reply_to_id
mockContext.eventName = "discussion_comment";
mockContext.payload = {
discussion: {
number: 10,
},
comment: {
node_id: "DC_kwDOTriggeringComment123",
},
};

let capturedReplyToId = undefined;
mockGithub.graphql = async (query, variables) => {
if (query.includes("addDiscussionComment")) {
capturedReplyToId = variables?.replyToId;
return {
addDiscussionComment: {
comment: {
id: "DC_kwDOTest456",
body: variables?.body,
createdAt: "2024-01-01",
url: "https://github.com/owner/repo/discussions/10#discussioncomment-456",
},
},
};
}
// Mock replyTo resolution: triggering comment is top-level (no parent)
if (query.includes("replyTo")) {
return { node: { replyTo: null } };
}
return {
repository: {
discussion: {
id: "D_kwDOTest123",
url: "https://github.com/owner/repo/discussions/10",
},
},
};
};

const handler = await eval(`(async () => { ${addCommentScript}; return await main({}); })()`);

const message = {
type: "add_comment",
body: "Reply that provides reply_to_id but should use triggering comment instead",
reply_to_id: "DC_kwDOShouldBeIgnored999",
};

const result = await handler(message, {});

expect(result.success).toBe(true);
expect(result.isDiscussion).toBe(true);
// Should use the triggering comment node_id, not the reply_to_id from the message
expect(capturedReplyToId).toBe("DC_kwDOTriggeringComment123");
});

it("should ignore and warn when reply_to_id is a whitespace-only string", async () => {
const addCommentScript = fs.readFileSync(path.join(__dirname, "add_comment.cjs"), "utf8");

mockContext.eventName = "workflow_dispatch";
mockContext.payload = {};

// Capture warning calls
const warningCalls = [];
mockCore.warning = msg => {
warningCalls.push(msg);
};

// Make REST API return 404 so the code falls back to the discussion path
mockGithub.rest.issues.createComment = async () => {
const err = new Error("Not Found");
// @ts-expect-error - Simulating GitHub REST API error
err.status = 404;
throw err;
};

let capturedReplyToId = undefined;
mockGithub.graphql = async (query, variables) => {
if (query.includes("addDiscussionComment")) {
capturedReplyToId = variables?.replyToId;
return {
addDiscussionComment: {
comment: {
id: "DC_kwDOTest456",
body: variables?.body,
createdAt: "2024-01-01",
url: "https://github.com/owner/repo/discussions/10#discussioncomment-456",
},
},
};
}
return {
repository: {
discussion: {
id: "D_kwDOTest123",
url: "https://github.com/owner/repo/discussions/10",
},
},
};
};

const handler = await eval(`(async () => { ${addCommentScript}; return await main({}); })()`);

const message = {
type: "add_comment",
item_number: 10,
body: "Comment with blank reply_to_id",
reply_to_id: " ",
};

const result = await handler(message, {});

expect(result.success).toBe(true);
expect(result.isDiscussion).toBe(true);
// Whitespace-only reply_to_id should be ignored (post top-level)
expect(capturedReplyToId).toBeUndefined();
expect(warningCalls).toContain("Ignoring empty discussion reply_to_id after normalization");
});

it("should coerce numeric reply_to_id to string before use", async () => {
const addCommentScript = fs.readFileSync(path.join(__dirname, "add_comment.cjs"), "utf8");

mockContext.eventName = "workflow_dispatch";
mockContext.payload = {};

// Make REST API return 404 so the code falls back to the discussion path
mockGithub.rest.issues.createComment = async () => {
const err = new Error("Not Found");
// @ts-expect-error - Simulating GitHub REST API error
err.status = 404;
throw err;
};

let capturedReplyToId = undefined;
mockGithub.graphql = async (query, variables) => {
if (query.includes("addDiscussionComment")) {
capturedReplyToId = variables?.replyToId;
return {
addDiscussionComment: {
comment: {
id: "DC_kwDOTest456",
body: variables?.body,
createdAt: "2024-01-01",
url: "https://github.com/owner/repo/discussions/10#discussioncomment-456",
},
},
};
}
// Mock replyTo resolution: node ID is top-level (no parent)
if (query.includes("replyTo")) {
return { node: { replyTo: null } };
}
return {
repository: {
discussion: {
id: "D_kwDOTest123",
url: "https://github.com/owner/repo/discussions/10",
},
},
};
};

const handler = await eval(`(async () => { ${addCommentScript}; return await main({}); })()`);

const message = {
type: "add_comment",
item_number: 10,
body: "Comment with numeric reply_to_id",
// @ts-expect-error - intentionally passing a number to test coercion
reply_to_id: 12345,
};

const result = await handler(message, {});

expect(result.success).toBe(true);
expect(result.isDiscussion).toBe(true);
// Numeric reply_to_id should be coerced to "12345"
expect(capturedReplyToId).toBe("12345");
});
});

describe("regression test for wrong PR bug", () => {
Expand Down
Loading
Loading