Skip to content

Skip add_labels when no triggering issue or PR exists #65619

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
Oct 4, 2026
Merged

Skip add_labels when no triggering issue or PR exists #65619

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e8b104a
Initial plan
Copilot Oct 4, 2026
dee393b
Skip add_labels without triggering issue context
Copilot Oct 4, 2026
45b8bd3
Fail closed on label target resolution
Copilot Oct 4, 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
6 changes: 6 additions & 0 deletions actions/setup/js/add_labels.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ const HANDLER_TYPE = "add_labels";
const { validateLabels } = require("./safe_output_validator.cjs");
const { getErrorMessage } = require("./error_helpers.cjs");
const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs");
const { resolveTarget } = require("./safe_output_helpers.cjs");
const { tryEnforceArrayLimit } = require("./limit_enforcement_helpers.cjs");
const { logStagedPreviewInfo } = require("./staged_preview.cjs");
const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs");
Expand Down Expand Up @@ -293,6 +294,11 @@ const main = createCountGatedHandler({
if (!targetResult.success) return targetResult;
itemNumber = targetResult.number ?? triggeringItemNumber;
} else if (target === "triggering") {
const targetResult = resolveTarget({ targetConfig: target, item: message, context, itemType: HANDLER_TYPE, supportsPR: true });

@github-actions github-actions Bot Oct 4, 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.

[/codebase-design] resolveInvocationContext(context) is already called two lines above (line 287) to get triggeringItemNumber, and resolveTarget calls it again internally. Besides the redundant work, resolveInvocationContext can throw (e.g. checkAllowedRepo validation failures for workflow_dispatch/repository_dispatch overrides) — the first call at line 287 is unguarded, so that failure mode predates this PR, but adding a second call site here doubles the chance of an uncaught throw bubbling out of the handler instead of being converted into a clean {success:false} result.

💡 Suggested simplification

Since effectiveContext from line 287 and the context resolveTarget recomputes internally should be equivalent, consider passing the already-resolved effectiveContext/triggeringItemNumber into the skip check directly (e.g. a lighter-weight isTriggeringContextAvailable(effectiveContext, { supportsPR: true }) check) rather than invoking the full resolveTarget resolution path a second time just to read shouldFail.

@copilot please address this.

Copilot AI Oct 4, 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.

Fixed in 45b8bd3: the triggering path now uses resolveTarget's validated number and context directly, removing the duplicate invocation-context resolution. The same skip behavior now covers remove_labels and replace_label.

if (!targetResult.success && targetResult.shouldFail === false) {
core.warning(targetResult.error);
return { success: false, skipped: true, reason: targetResult.error, error: targetResult.error };
}
itemNumber = triggeringItemNumber;

Copilot AI Oct 4, 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.

Fixed in 45b8bd3: add_labels now returns hard resolveTarget failures and uses its validated issue/PR number, preventing forwarded string-number contexts from reaching the API.

} else {
itemNumber = Number(target);
Expand Down
8 changes: 6 additions & 2 deletions actions/setup/js/add_labels.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ describe("add_labels", () => {
};

mockContext = {
eventName: "issues",
repo: {
owner: "test-owner",
repo: "test-repo",
Expand Down Expand Up @@ -702,6 +703,7 @@ describe("add_labels", () => {
});

it("should add labels to a pull request from context", async () => {
mockContext.eventName = "pull_request";
mockContext.payload = {
pull_request: {
number: 789,
Expand Down Expand Up @@ -743,7 +745,8 @@ describe("add_labels", () => {
expect(result.error.includes("Invalid item number")).toBe(true);
});

it("should handle missing item_number and no context", async () => {
it("should skip when the triggering context has no issue or pull request", async () => {
mockContext.eventName = "workflow_dispatch";
mockContext.payload = {};

const handler = await main({ max: 10 });
Expand All @@ -756,7 +759,8 @@ describe("add_labels", () => {
);

expect(result.success).toBe(false);
expect(result.error.includes("No issue/PR number available")).toBe(true);
expect(result.skipped).toBe(true);
expect(result.error).toContain('Target is "triggering" but not running in issue or pull request context');
});

it("should respect max count limit", async () => {
Expand Down
12 changes: 12 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 @@ -411,6 +411,18 @@ describe("Safe Output Handler Manager", () => {
expect(fatalFailures).toEqual([{ type: "create_issue", success: false, error: "Validation failed" }]);
});

it("does not fail the job when a successful write has an unrelated add_labels skip", () => {
const results = [
{ type: "add_labels", success: false, skipped: true, error: 'Target is "triggering" but not running in issue or pull request context' },
{ type: "create_discussion", success: true },
];
const { fatalFailures, reportOnlyFailures } = partitionFailureResults(results);

expect(fatalFailures).toEqual([]);
expect(reportOnlyFailures).toEqual([]);
expect(computeSafeOutputsStatus(results).status).toBe("completed_with_skips");
});

it("computes partial success item status from mixed successful and failed results", () => {
const status = computeSafeOutputsStatus([
{ type: "create_issue", success: true },
Expand Down
Loading