Skip to content

Enforce configured targets in safe-output handlers #62447

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
Sep 21, 2026
Merged

Enforce configured targets in safe-output handlers #62447

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
c011c92
Enforce configured targets in safe output handlers
Copilot Sep 21, 2026
b16808f
Merge branch 'main' into copilot/ensure-target-triggering-enforcement
github-actions[bot] Sep 21, 2026
de83120
Address review feedback: fix target-enforcement gaps and add spec Tar…
Copilot Sep 21, 2026
7664e4e
Document safe-output target authorization
Copilot Sep 21, 2026
e5380ea
Harden safe-output target regression coverage
Copilot Sep 21, 2026
700589a
Fix safe-output target invariant type guard
Copilot Sep 21, 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
35 changes: 16 additions & 19 deletions actions/setup/js/add_comment.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,9 +610,8 @@ async function main(config = {}) {
commentIdToReuse = allowedCommentId.commentId;
}

// Check if item_number or issue_number was explicitly provided in the message.
// item_number takes precedence over issue_number when both are present.
// pr-number is accepted as an alias for item_number for robustness.
// Track whether the configured target selected an explicit item rather than the
// triggering context. This controls reply and retry behavior later in the handler.
/** @type {{ success: boolean, number?: number | null, deferred?: boolean, error?: string }} */
let itemTargetResult = { success: true, number: null };
if (hasExplicitCommentId) {
Expand Down Expand Up @@ -641,36 +640,31 @@ async function main(config = {}) {
};
}
} else {
itemTargetResult = resolveSafeOutputIssueTarget({ message, tempIdMap: temporaryIdMap, repoParts, handlerType: HANDLER_TYPE, aliases: ["item_number", "issue_number", "pr-number"] });
if (!itemTargetResult.success) return itemTargetResult;
}

if (itemTargetResult.number != null) {
itemNumber = itemTargetResult.number;
core.info(`Using explicitly provided target number (item_number/issue_number/pr-number): #${itemNumber}`);
} else if (!hasExplicitCommentId) {
// Check if this is a discussion context
const isDiscussionContext = effectiveContext.eventName === "discussion" || effectiveContext.eventName === "discussion_comment";

if (isDiscussionContext) {
// For discussions, always use the discussion context
if (commentTarget === "triggering" && isDiscussionContext) {
isDiscussion = true;
itemNumber = effectiveContext.payload?.discussion?.number;

if (!itemNumber) {
core.warning("Discussion context detected but no discussion number found");
return {
success: false,
error: "No discussion number available",
};
}

core.info(`Using discussion context: #${itemNumber}`);
} else {
// For issues/PRs, use the resolveTarget helper which respects target configuration
let targetItem = message;
if (commentTarget === "*") {
itemTargetResult = resolveSafeOutputIssueTarget({ message, tempIdMap: temporaryIdMap, repoParts, handlerType: HANDLER_TYPE, aliases: ["item_number", "issue_number", "pr-number"] });
if (!itemTargetResult.success) return itemTargetResult;
if (itemTargetResult.number != null) {
targetItem = { ...message, item_number: itemTargetResult.number };
}
}

const targetResult = resolveTarget({
targetConfig: commentTarget,
item: message,
item: targetItem,
context: effectiveContext,
itemType: "add_comment",
supportsPR: true, // add_comment supports both issues and PRs
Expand Down Expand Up @@ -710,6 +704,9 @@ async function main(config = {}) {
}

itemNumber = targetResult.number;
if (commentTarget !== "triggering") {
itemTargetResult = { success: true, number: itemNumber };
}
core.info(`Resolved target ${targetResult.contextType} #${itemNumber} (target config: ${commentTarget})`);
}
}
Expand Down
42 changes: 21 additions & 21 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 @@ -283,7 +283,7 @@ describe("add_comment", () => {
expect(result.error).toBeTruthy();
});

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

/** @type {any} */
Expand All @@ -310,8 +310,8 @@ describe("add_comment", () => {
const result = await handler(message, {});

expect(result.success).toBe(true);
expect(capturedIssueNumber).toBe(777);
expect(result.itemNumber).toBe(777);
expect(capturedIssueNumber).toBe(8535);
expect(result.itemNumber).toBe(8535);
});

it("should resolve from context when item_number is not provided", async () => {
Expand Down Expand Up @@ -545,7 +545,7 @@ describe("add_comment", () => {
};
};

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

const message = {
type: "add_comment",
Expand Down Expand Up @@ -1055,7 +1055,7 @@ describe("add_comment", () => {
throw err;
};

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

// Explicit item_number targeting a different discussion (via 404 fallback)
const message = {
Expand Down Expand Up @@ -1117,7 +1117,7 @@ describe("add_comment", () => {
};
};

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

const message = {
type: "add_comment",
Expand Down Expand Up @@ -1179,7 +1179,7 @@ describe("add_comment", () => {
};
};

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

const message = {
type: "add_comment",
Expand Down Expand Up @@ -1302,7 +1302,7 @@ describe("add_comment", () => {
};
};

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

const message = {
type: "add_comment",
Expand Down Expand Up @@ -1364,7 +1364,7 @@ describe("add_comment", () => {
};
};

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

const message = {
type: "add_comment",
Expand Down Expand Up @@ -1997,7 +1997,7 @@ describe("add_comment", () => {
}
};

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

const message = {
type: "add_comment",
Expand Down Expand Up @@ -2198,7 +2198,7 @@ describe("add_comment", () => {
}
};

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

const message = {
type: "add_comment",
Expand Down Expand Up @@ -2257,7 +2257,7 @@ describe("add_comment", () => {
}
};

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

const message = {
type: "add_comment",
Expand Down Expand Up @@ -2292,7 +2292,7 @@ describe("add_comment", () => {
};
};

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

const message = {
type: "add_comment",
Expand All @@ -2315,7 +2315,7 @@ describe("add_comment", () => {
it("should defer when temporary ID is not yet resolved", async () => {
const addCommentScript = fs.readFileSync(path.join(__dirname, "add_comment.cjs"), "utf8");

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

const message = {
type: "add_comment",
Expand Down Expand Up @@ -2348,7 +2348,7 @@ describe("add_comment", () => {
};
};

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

const message = {
type: "add_comment",
Expand All @@ -2370,7 +2370,7 @@ describe("add_comment", () => {
it("should handle invalid temporary ID format", async () => {
const addCommentScript = fs.readFileSync(path.join(__dirname, "add_comment.cjs"), "utf8");

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

const message = {
type: "add_comment",
Expand Down Expand Up @@ -2439,7 +2439,7 @@ describe("add_comment", () => {
};
};

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

const message = {
type: "add_comment",
Expand Down Expand Up @@ -2472,7 +2472,7 @@ describe("add_comment", () => {
};
};

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

const message = {
type: "add_comment",
Expand All @@ -2495,7 +2495,7 @@ describe("add_comment", () => {
it("should defer when issue_number has unresolved temporary ID", async () => {
const addCommentScript = fs.readFileSync(path.join(__dirname, "add_comment.cjs"), "utf8");

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

const message = {
type: "add_comment",
Expand Down Expand Up @@ -3079,7 +3079,7 @@ describe("add_comment", () => {
};
};

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

const message = {
type: "add_comment",
Expand Down Expand Up @@ -3124,7 +3124,7 @@ describe("add_comment", () => {
};
};

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

const message = {
type: "add_comment",
Expand Down
25 changes: 12 additions & 13 deletions actions/setup/js/add_reviewer.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,8 @@ const HANDLER_TYPE = "add_reviewer";

const { processItems } = require("./safe_output_processor.cjs");
const { getErrorMessage } = require("./error_helpers.cjs");
const { getPullRequestNumber } = require("./pr_helpers.cjs");
const { logStagedPreviewInfo } = require("./staged_preview.cjs");
const { isStagedMode, checkRequiredFilter } = require("./safe_output_helpers.cjs");
const { isStagedMode, checkRequiredFilter, resolveTarget } = require("./safe_output_helpers.cjs");
const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs");
const { attachExecutionState, extractReviewStateFromData, fetchPullRequestReviewState } = require("./safe_output_execution_metadata.cjs");
const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs");
Expand Down Expand Up @@ -85,6 +84,7 @@ async function main(config = {}) {
const allowedReviewers = config.allowed ?? [];
const allowedTeamReviewers = config.allowed_team_reviewers ?? [];
const maxCount = config.max ?? 10;
const targetConfig = config.target || "triggering";
const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config);
const githubClient = await createAuthenticatedGitHubClient(config);
const isStaged = isStagedMode(config);
Expand Down Expand Up @@ -152,21 +152,20 @@ async function main(config = {}) {

processedCount++;

const { prNumber, error } = getPullRequestNumber(message, context);

if (error) {
core.warning(error);
return {
success: false,
error,
};
}
if (prNumber === null) {
const targetResult = resolveTarget({
targetConfig,
item: message,
context,
itemType: HANDLER_TYPE,
});
if (!targetResult.success) {
core.warning(targetResult.error);
return {
success: false,
error: "Pull request number is required",
error: targetResult.error,
};
}
const prNumber = targetResult.number;

const repoResult = resolveAndValidateRepo(message, defaultTargetRepo, allowedRepos, "pull request reviewer");
if (!repoResult.success) {
Expand Down
8 changes: 6 additions & 2 deletions actions/setup/js/add_reviewer.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,8 @@ describe("add_reviewer (Handler Factory Architecture)", () => {
});

it("should use explicit PR number from message", async () => {
const { main } = require("./add_reviewer.cjs");
handler = await main({ max: 10, allowed: ["user1"], target: "*" });
const message = {
type: "add_reviewer",
reviewers: ["user1"],
Expand Down Expand Up @@ -310,7 +312,7 @@ describe("add_reviewer (Handler Factory Architecture)", () => {
const result = await handler(message, {});

expect(result.success).toBe(false);
expect(result.error).toContain("No pull_request_number provided and not in pull request context");
expect(result.error).toContain("not running in pull request context");
expect(mockGithub.rest.pulls.requestReviewers).not.toHaveBeenCalled();
});

Expand Down Expand Up @@ -510,6 +512,8 @@ describe("add_reviewer (Handler Factory Architecture)", () => {
});

it("should return error for invalid pull_request_number", async () => {
const { main } = require("./add_reviewer.cjs");
handler = await main({ max: 10, allowed: ["user1"], target: "*" });
const invalidValues = ["not-a-number", null, "abc123"];

for (const invalidValue of invalidValues) {
Expand All @@ -523,7 +527,7 @@ describe("add_reviewer (Handler Factory Architecture)", () => {
const result = await handler(message, {});

expect(result.success).toBe(false);
expect(result.error).toContain("Invalid pull_request_number");
expect(result.error).toMatch(/Invalid pull_request_number|no pull_request_number/);
expect(mockGithub.rest.pulls.requestReviewers).not.toHaveBeenCalled();
}
});
Expand Down
Loading
Loading