Skip to content

[jsweep] Clean add_comment.cjs #32949

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 2 commits into from
May 18, 2026
+79 −18
Merged

[jsweep] Clean add_comment.cjs #32949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5579b7f
jsweep: clean add_comment.cjs
github-actions[bot] May 18, 2026
fc28feb
test: assert alias allowlist logs for dedupe/order
Copilot May 18, 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
34 changes: 16 additions & 18 deletions actions/setup/js/add_comment.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,21 @@ const { resolveInvocationContext } = require("./invocation_context_helpers.cjs")
/** @type {string} Safe output type handled by this module */
const HANDLER_TYPE = "add_comment";

/**
* Deduplicate an array of strings using case-insensitive comparison, preserving original casing and order.
* @param {string[]} aliases
* @returns {string[]}
*/
function deduplicateCaseInsensitive(aliases) {
const seen = new Set();
return aliases.filter(alias => {
const key = alias.toLowerCase();
if (seen.has(key)) return false;
seen.add(key);
return true;
});
}

/**
* Resolve effective event name/payload for native and forwarded contexts.
* Supports:
Expand Down Expand Up @@ -566,24 +581,7 @@ async function main(config = {}) {
}
}
}
const allowedMentionAliases = [];
const seenAllowedMentionAliases = new Set();
for (const alias of parentAuthors) {
const key = alias.toLowerCase();
if (seenAllowedMentionAliases.has(key)) {
continue;
}
seenAllowedMentionAliases.add(key);
allowedMentionAliases.push(alias);
}
for (const alias of configuredMentionAliases) {
const key = alias.toLowerCase();
if (seenAllowedMentionAliases.has(key)) {
continue;
}
seenAllowedMentionAliases.add(key);
allowedMentionAliases.push(alias);
}
const allowedMentionAliases = deduplicateCaseInsensitive([...parentAuthors, ...configuredMentionAliases]);

if (allowedMentionAliases.length > 0) {
core.info(`[MENTIONS] Allowing aliases in comment: ${allowedMentionAliases.join(", ")}`);
Expand Down
63 changes: 63 additions & 0 deletions actions/setup/js/add_comment.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2775,6 +2775,69 @@ describe("add_comment", () => {
// but the key test is that the handler succeeds
expect(result.isDiscussion).toBe(true);
});

it("should deduplicate allowed aliases case-insensitively across parentAuthors and configured mentions", async () => {
const addCommentScript = fs.readFileSync(path.join(__dirname, "add_comment.cjs"), "utf8");

// Issue author is "Alice"; configured mentions also include "alice" (different casing)
mockContext.eventName = "issues";
mockContext.payload = {
issue: {
number: 42,
user: { login: "Alice", type: "User" },
},
};

const infoMessages = [];
mockCore.info = msg => infoMessages.push(msg);

let capturedBody = null;
mockGithub.rest.issues.createComment = async ({ body }) => {
capturedBody = body;
return { data: { id: 12345, html_url: "https://github.com/owner/repo/issues/42#issuecomment-12345" } };
};

// configured mentions includes "alice" (lowercase dup) and "Bob" (unique)
const handler = await eval(`(async () => { ${addCommentScript}; return await main({ mentions: { allowed: ["alice", "Bob"] } }); })()`);

const result = await handler({ type: "add_comment", body: "@Alice @Bob check this out" }, {});

expect(result.success).toBe(true);
expect(capturedBody).toContain("@Alice");
expect(capturedBody).toContain("@Bob");
Comment on lines +2806 to +2807
expect(infoMessages).toContain("[MENTIONS] Allowing aliases in comment: Alice, Bob");
});

it("should preserve order: parentAuthors first, then configuredMentionAliases", async () => {
const addCommentScript = fs.readFileSync(path.join(__dirname, "add_comment.cjs"), "utf8");

mockContext.eventName = "issues";
mockContext.payload = {
issue: {
number: 42,
user: { login: "Charlie", type: "User" },
},
};

const infoMessages = [];
mockCore.info = msg => infoMessages.push(msg);

let capturedBody = null;
mockGithub.rest.issues.createComment = async ({ body }) => {
capturedBody = body;
return { data: { id: 12345, html_url: "https://github.com/owner/repo/issues/42#issuecomment-12345" } };
};

const handler = await eval(`(async () => { ${addCommentScript}; return await main({ mentions: { allowed: ["Dave", "Eve"] } }); })()`);

const result = await handler({ type: "add_comment", body: "@Charlie @Dave @Eve thanks!" }, {});

expect(result.success).toBe(true);
expect(capturedBody).toContain("@Charlie");
expect(capturedBody).toContain("@Dave");
expect(capturedBody).toContain("@Eve");
Copy link
Copy Markdown
Contributor Author

@github-actions github-actions Bot May 18, 2026

Choose a reason for hiding this comment

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

[/tdd] The test name promises ordering verification (parentAuthors first, then configuredMentionAliases) but the assertions only check presence — they do not verify that Charlie appears before Dave/Eve in the final allowedMentionAliases list.

If the ordering guarantee matters (e.g. parentAuthors always take precedence), consider asserting order directly via the core.info log message:

// Capture info logs to assert ordering
const infos = [];
mockCore.info = msg => infos.push(msg);
// ... run handler ...
const infoLog = infos.find(m => m.startsWith('[MENTIONS] Allowing aliases'));
const aliases = infoLog.replace('[MENTIONS] Allowing aliases in comment: ', '').split(', ');
expect(aliases.indexOf('Charlie')).toBeLessThan(aliases.indexOf('Dave'));
expect(aliases.indexOf('Charlie')).toBeLessThan(aliases.indexOf('Eve'));

As written the test passes even if configuredMentionAliases were accidentally prepended before parentAuthors, making it a weaker specification than its name implies.

expect(infoMessages).toContain("[MENTIONS] Allowing aliases in comment: Charlie, Dave, Eve");
});
});

describe("staged mode", () => {
Expand Down