Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 2 additions & 7 deletions actions/setup/js/add_reaction.cjs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// @ts-check
/// <reference types="@actions/github-script" />

const { getErrorMessage } = require("./error_helpers.cjs");
const { getErrorMessage, isLockedError } = require("./error_helpers.cjs");

/**
* Add a reaction to the triggering item (issue, PR, comment, or discussion).
Expand Down Expand Up @@ -99,12 +99,7 @@ async function main() {
const errorMessage = getErrorMessage(error);

// Check if the error is due to a locked issue/PR/discussion
// GitHub API returns 403 with specific messages for locked resources
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 BOTH a 403 status code AND mentions locked
if (is403Error && hasLockedMessage) {
if (isLockedError(error)) {
// 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;
Expand Down
25 changes: 24 additions & 1 deletion actions/setup/js/error_helpers.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,27 @@ function getErrorMessage(error) {
return String(error);
}

module.exports = { getErrorMessage };
/**
* Check if an error is due to a locked issue/PR/discussion.
* GitHub API returns 403 with specific messages for locked resources.
* This helper is used to determine if an operation should be silently ignored.
*
* @param {unknown} error - The error value to check
* @returns {boolean} True if error is due to locked resource, false otherwise
*/
function isLockedError(error) {
// Check if the error has a 403 status code
const is403Error = error && typeof error === "object" && "status" in error && error.status === 403;
if (!is403Error) {
return false;
}

// Check if the error message mentions "locked"
const errorMessage = getErrorMessage(error);
const hasLockedMessage = Boolean(errorMessage && (errorMessage.includes("locked") || errorMessage.includes("Lock conversation")));

// Only return true if it's BOTH a 403 status code AND mentions locked
return hasLockedMessage;
}

module.exports = { getErrorMessage, isLockedError };
52 changes: 51 additions & 1 deletion actions/setup/js/error_helpers.test.cjs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { describe, it, expect } from "vitest";
import { getErrorMessage } from "./error_helpers.cjs";
import { getErrorMessage, isLockedError } from "./error_helpers.cjs";

describe("error_helpers", () => {
describe("getErrorMessage", () => {
Expand Down Expand Up @@ -39,4 +39,54 @@ describe("error_helpers", () => {
expect(getErrorMessage(error)).toBe("[object Object]");
});
});

describe("isLockedError", () => {
it("should return true for 403 error with 'locked' in message", () => {
const error = new Error("Issue is locked");
error.status = 403;
expect(isLockedError(error)).toBe(true);
});

it("should return true for 403 error with 'Lock conversation' in message", () => {
const error = new Error("Lock conversation is enabled");
error.status = 403;
expect(isLockedError(error)).toBe(true);
});

it("should return false for 403 error without 'locked' in message", () => {
const error = new Error("Forbidden: insufficient permissions");
error.status = 403;
expect(isLockedError(error)).toBe(false);
});

it("should return false for non-403 error with 'locked' in message", () => {
const error = new Error("Issue is locked");
error.status = 500;
expect(isLockedError(error)).toBe(false);
});

it("should return false for error without status property", () => {
const error = new Error("Issue is locked");
expect(isLockedError(error)).toBe(false);
});

it("should return false for null error", () => {
expect(isLockedError(null)).toBe(false);
});

it("should return false for undefined error", () => {
expect(isLockedError(undefined)).toBe(false);
});

it("should handle object errors with status and message", () => {
const error = { status: 403, message: "This resource is locked" };
expect(isLockedError(error)).toBe(true);
});

it("should return false for 403 error with only partial match", () => {
const error = { status: 403, message: "This issue has been unlocked" };
// Contains "unlocked" which includes "locked" substring
expect(isLockedError(error)).toBe(true);
Copy link

Copilot AI Feb 15, 2026

Choose a reason for hiding this comment

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

Test description and expectation are contradictory. The test is named "should return false for 403 error with only partial match" and the comment on line 88 states "Contains 'unlocked' which includes 'locked' substring", suggesting this should return false (not a locked error). However, the expectation on line 89 is expect(isLockedError(error)).toBe(true), which expects true.

The message "This issue has been unlocked" contains "locked" as a substring but indicates the resource is NOT locked. The function should return false for this case to avoid false positives. Change the expectation to toBe(false) to match the test description.

Suggested change
expect(isLockedError(error)).toBe(true);
expect(isLockedError(error)).toBe(false);

Copilot uses AI. Check for mistakes.
});
});
});
20 changes: 17 additions & 3 deletions actions/setup/js/notify_comment_error.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
const { loadAgentOutput } = require("./load_agent_output.cjs");
const { getRunSuccessMessage, getRunFailureMessage, getDetectionFailureMessage } = require("./messages_run_status.cjs");
const { getMessages } = require("./messages_core.cjs");
const { getErrorMessage } = require("./error_helpers.cjs");
const { getErrorMessage, isLockedError } = require("./error_helpers.cjs");
const { sanitizeContent } = require("./sanitize_content.cjs");

/**
Expand Down Expand Up @@ -246,7 +246,14 @@ async function main() {
if (response?.data?.html_url) core.info(`Comment URL: ${response.data.html_url}`);
return;
} catch (error) {
// Don't fail the workflow if we can't create the comment
// Check if the error is due to a locked issue/PR/discussion
if (isLockedError(error)) {
// Silently ignore locked resource errors - just log for debugging
core.info(`Cannot create append-only comment: resource is locked (this is expected and not an error)`);
return;
}

// Don't fail the workflow if we can't create the comment (for other errors)
core.warning(`Failed to create append-only comment: ${getErrorMessage(error)}`);
return;
}
Expand Down Expand Up @@ -300,7 +307,14 @@ async function main() {
core.info(`Comment URL: ${response.data.html_url}`);
}
} catch (error) {
// Don't fail the workflow if we can't update the comment
// Check if the error is due to a locked issue/PR/discussion
if (isLockedError(error)) {
// Silently ignore locked resource errors - just log for debugging
core.info(`Cannot update comment: resource is locked (this is expected and not an error)`);
return;
}
Comment on lines 249 to 315
Copy link

Copilot AI Feb 15, 2026

Choose a reason for hiding this comment

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

The new locked error handling in notify_comment_error.cjs (lines 250-254 and 311-315) is not covered by tests. The file notify_comment_error.test.cjs only has a generic error handling test that doesn't specifically test locked resource errors.

Consider adding test cases similar to those in add_reaction.test.cjs (lines 393-426) to verify:

  1. Locked errors (403 + "locked" message) are silently ignored with info logging
  2. Non-locked 403 errors still produce warnings
  3. Locked messages with non-403 status still produce warnings

Copilot uses AI. Check for mistakes.

// Don't fail the workflow if we can't update the comment (for other errors)
core.warning(`Failed to update comment: ${getErrorMessage(error)}`);
}
}
Expand Down
Loading