Skip to content

Make add_comment wildcard target misses non-fatal in safe outputs #37041

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 6 commits into from
Jun 5, 2026
+33 −1
Merged

Make add_comment wildcard target misses non-fatal in safe outputs #37041

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
32d9baf
Initial plan
Copilot Jun 5, 2026
79129bc
Make targetless add_comment wildcard non-fatal
Copilot Jun 5, 2026
59681e4
Harden wildcard add_comment skip detection
Copilot Jun 5, 2026
8ba5273
Refine add_comment wildcard target skip handling
Copilot Jun 5, 2026
07064c4
Address review feedback for add_comment wildcard handling
Copilot Jun 5, 2026
eb3ac1e
Merge branch 'main' into copilot/aw-failures-fix-add-comment-safety
pelikhan Jun 5, 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
13 changes: 13 additions & 0 deletions actions/setup/js/add_comment.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ const { resolveInvocationContext } = require("./invocation_context_helpers.cjs")

/** @type {string} Safe output type handled by this module */
const HANDLER_TYPE = "add_comment";
// Keep the full list of accepted explicit wildcard target fields (including aliases
// pre-handled by resolveSafeOutputIssueTarget) to preserve a defensive boundary check.
const WILDCARD_TARGET_FIELDS = ["item_number", "issue_number", "pull_request_number", "pr_number", "pr", "pull_number"];
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Jun 5, 2026

Choose a reason for hiding this comment

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

[/diagnose] item_number and issue_number in WILDCARD_TARGET_FIELDS are pre-handled by resolveSafeOutputIssueTarget at line 471 (using the same aliases). By the time resolveTarget fails, those two fields are already absent or null in the message — making their presence in this list technically dead weight.

The fields that can actually reach this code path unaided are pull_request_number, pr_number, pr, and pull_number. A short JSDoc comment would make this intent explicit and prevent future drift.

📋 Reasoning
  • resolveSafeOutputIssueTarget (line 471) uses aliases ["item_number", "issue_number", "pr-number"]
  • If any of these are non-null, it resolves before reaching resolveTarget — we never enter the shouldFail branch
  • If it succeeds with number: null, those fields are absent/null in the message
  • So at the wildcard check site, message.item_number != null and message.issue_number != null are always false

Consider narrowing and documenting:

// Fields recognized by resolveTarget (pull-only path, supportsIssue: false).
// item_number/issue_number/pr-number are handled earlier by resolveSafeOutputIssueTarget.
const WILDCARD_TARGET_FIELDS = ["pull_request_number", "pr_number", "pr", "pull_number"];

Or keep the full list and add a comment explaining it serves as a belt-and-suspenders guard.


/**
* Deduplicate an array of strings using case-insensitive comparison, preserving original casing and order.
Expand Down Expand Up @@ -504,6 +507,16 @@ async function main(config = {}) {

if (!targetResult.success) {
if (targetResult.shouldFail) {
const hasExplicitWildcardTargetField = WILDCARD_TARGET_FIELDS.some(field => message[field] != null);
const missingWildcardTarget = commentTarget === "*" && !hasExplicitWildcardTargetField;
if (missingWildcardTarget) {
core.info(targetResult.error);
return {
success: false,
skipped: true,
error: targetResult.error,
};
}
core.warning(targetResult.error);
return {
success: false,
Expand Down
21 changes: 20 additions & 1 deletion actions/setup/js/add_comment.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ describe("add_comment", () => {
expect(result.itemNumber).toBe(28912);
});

it("should fail when target is '*' but no item_number provided", async () => {
it("should skip (not fail) when target is '*' but no item_number provided", async () => {
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Jun 5, 2026

Choose a reason for hiding this comment

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

[/tdd] Missing regression test for the "strict behavior preserved" invariant — when target: "*" fires with an explicit PR field present, the handler should still hard-fail.

The updated test only locks in the skip path. Without a complementary test for the hard-fail path, the !hasExplicitWildcardTargetField condition can be accidentally inverted without breaking the suite.

💡 Suggested additional test

Add a test adjacent to the current one:

it("should hard-fail (not skip) when target is * but explicit field resolves nothing meaningful", async () => {
  // Tests that having a recognized field still triggers hard-fail
  // when resolveTarget returns shouldFail (strict behavior preserved)
  const addCommentScript = fs.readFileSync(path.join(__dirname, "add_comment.cjs"), "utf8");

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

  const message = {
    type: "add_comment",
    pull_request_number: 42, // explicit field present — should still hard-fail
    body: "Test comment",
  };

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

  expect(result.success).toBe(false);
  expect(result.skipped).toBeUndefined(); // NOT a skip — explicit field was provided
});

This makes the skip-vs-fail boundary explicit and protects against accidental widening of the skip condition.

const addCommentScript = fs.readFileSync(path.join(__dirname, "add_comment.cjs"), "utf8");

const handler = await eval(`(async () => { ${addCommentScript}; return await main({ target: '*' }); })()`);
Expand All @@ -235,9 +235,28 @@ describe("add_comment", () => {
const result = await handler(message, {});

expect(result.success).toBe(false);
expect(result.skipped).toBe(true);
expect(result.error).toMatch(/no.*item_number/i);
});

it("should hard-fail (not skip) when target is '*' and explicit pull_request_number is invalid", async () => {
const addCommentScript = fs.readFileSync(path.join(__dirname, "add_comment.cjs"), "utf8");

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

const message = {
type: "add_comment",
pull_request_number: "invalid",
body: "Test comment with invalid explicit pull_request_number",
};

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

expect(result.success).toBe(false);
expect(result.skipped).toBeUndefined();
expect(result.error).toBeTruthy();
});

it("should use explicit item_number even with triggering target", async () => {
const addCommentScript = fs.readFileSync(path.join(__dirname, "add_comment.cjs"), "utf8");

Expand Down
Loading