From a8af60457003a451acd494bc5387308a5e025d66 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Sun, 15 Feb 2026 19:33:08 +0000 Subject: [PATCH] fix: Require both 403 status AND locked message for error suppression This fixes the locked resource error check logic to properly require BOTH a 403 status code AND a locked message before suppressing the error. The previous logic was incorrectly simplified to ignore any error with a locked message, regardless of status code. This could mask legitimate errors that mention "locked" but aren't actually resource-locked errors. Changes: - Updated error check from `(is403Error && hasLockedMessage) || (!is403Error && hasLockedMessage)` to `is403Error && hasLockedMessage` - Updated comments to clarify that BOTH conditions are required - Updated tests to verify non-403 errors with locked messages are not suppressed Files modified: - actions/setup/js/add_reaction.cjs - actions/setup/js/add_reaction.test.cjs - actions/setup/js/add_reaction_and_edit_comment.cjs - actions/setup/js/add_reaction_and_edit_comment.test.cjs Fixes #15977 Co-Authored-By: Claude Sonnet 4.5 --- actions/setup/js/add_reaction.cjs | 4 ++-- actions/setup/js/add_reaction.test.cjs | 12 +++++++----- actions/setup/js/add_reaction_and_edit_comment.cjs | 4 ++-- .../setup/js/add_reaction_and_edit_comment.test.cjs | 12 +++++++----- 4 files changed, 18 insertions(+), 14 deletions(-) diff --git a/actions/setup/js/add_reaction.cjs b/actions/setup/js/add_reaction.cjs index 42636da87cc..e22bbe920c0 100644 --- a/actions/setup/js/add_reaction.cjs +++ b/actions/setup/js/add_reaction.cjs @@ -103,8 +103,8 @@ async function main() { const is403Error = error && typeof error === "object" && "status" in error && error.status === 403; const hasLockedMessage = errorMessage && (errorMessage.includes("locked") || errorMessage.includes("Lock conversation")); - // Only ignore the error if it's a 403 AND mentions locked, or if the message mentions locked - if ((is403Error && hasLockedMessage) || (!is403Error && hasLockedMessage)) { + // Only ignore the error if it's BOTH a 403 status code AND mentions locked + if (is403Error && hasLockedMessage) { // Silently ignore locked resource errors - just log for debugging core.info(`Cannot add reaction: resource is locked (this is expected and not an error)`); return; diff --git a/actions/setup/js/add_reaction.test.cjs b/actions/setup/js/add_reaction.test.cjs index 1b64719f40d..e5ff4ff9dcb 100644 --- a/actions/setup/js/add_reaction.test.cjs +++ b/actions/setup/js/add_reaction.test.cjs @@ -402,14 +402,16 @@ describe("add_reaction", () => { expect(mockCore.setFailed).not.toHaveBeenCalled(); }); - it("should silently ignore locked issue errors (message contains 'locked')", async () => { - mockGithub.request.mockRejectedValueOnce(new Error("Lock conversation is enabled")); + it("should fail for errors with 'locked' message but non-403 status", async () => { + // Errors mentioning "locked" should only be ignored if they have 403 status + const lockedError = new Error("Lock conversation is enabled"); + lockedError.status = 500; // Not 403 + mockGithub.request.mockRejectedValueOnce(lockedError); await runScript(); - expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("resource is locked")); - expect(mockCore.error).not.toHaveBeenCalled(); - expect(mockCore.setFailed).not.toHaveBeenCalled(); + expect(mockCore.error).toHaveBeenCalledWith(expect.stringContaining("Failed to add reaction")); + expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("Failed to add reaction")); }); it("should fail for 403 errors that don't mention locked", async () => { diff --git a/actions/setup/js/add_reaction_and_edit_comment.cjs b/actions/setup/js/add_reaction_and_edit_comment.cjs index 7b7471ef3b2..5dc2abde72a 100644 --- a/actions/setup/js/add_reaction_and_edit_comment.cjs +++ b/actions/setup/js/add_reaction_and_edit_comment.cjs @@ -161,8 +161,8 @@ async function main() { const is403Error = error && typeof error === "object" && "status" in error && error.status === 403; const hasLockedMessage = errorMessage && (errorMessage.includes("locked") || errorMessage.includes("Lock conversation")); - // Only ignore the error if it's a 403 AND mentions locked, or if the message mentions locked - if ((is403Error && hasLockedMessage) || (!is403Error && hasLockedMessage)) { + // Only ignore the error if it's BOTH a 403 status code AND mentions locked + if (is403Error && hasLockedMessage) { // Silently ignore locked resource errors - just log for debugging core.info(`Cannot add reaction: resource is locked (this is expected and not an error)`); return; diff --git a/actions/setup/js/add_reaction_and_edit_comment.test.cjs b/actions/setup/js/add_reaction_and_edit_comment.test.cjs index 76019444f49..e204f2cb002 100644 --- a/actions/setup/js/add_reaction_and_edit_comment.test.cjs +++ b/actions/setup/js/add_reaction_and_edit_comment.test.cjs @@ -248,15 +248,17 @@ const mockCore = { expect(mockCore.error).not.toHaveBeenCalled(), expect(mockCore.setFailed).not.toHaveBeenCalled()); }), - it("should silently ignore locked issue errors (message contains 'locked')", async () => { + it("should fail for errors with 'locked' message but non-403 status", async () => { + // Errors mentioning "locked" should only be ignored if they have 403 status + const lockedError = new Error("Lock conversation is enabled"); + lockedError.status = 500; // Not 403 ((process.env.GH_AW_REACTION = "eyes"), (global.context.eventName = "issues"), (global.context.payload = { issue: { number: 123 }, repository: { html_url: "https://github.com/testowner/testrepo" } }), - mockGithub.request.mockRejectedValueOnce(new Error("Lock conversation is enabled")), + mockGithub.request.mockRejectedValueOnce(lockedError), await eval(`(async () => { ${reactionScript}; await main(); })()`), - expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("resource is locked")), - expect(mockCore.error).not.toHaveBeenCalled(), - expect(mockCore.setFailed).not.toHaveBeenCalled()); + expect(mockCore.error).toHaveBeenCalledWith(expect.stringContaining("Failed to process reaction")), + expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("Failed to process reaction"))); }), it("should fail for 403 errors that don't mention locked", async () => { const forbiddenError = new Error("Forbidden: insufficient permissions");