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
77 changes: 76 additions & 1 deletion actions/setup/js/resolve_pr_review_thread.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,57 @@ function isIntegrationAccessError(error) {
return messages.some(message => message.toLowerCase().includes(integrationErrorFragment));
}

/**
* Check whether an error indicates the referenced GraphQL node no longer exists.
* Review thread node IDs can become stale between the agent turn and the safe-outputs
* replay (thread already resolved, deleted, or superseded), which surfaces as a
* "Could not resolve to a node" or "Not Found" error. These are treated as skippable.
* @param {unknown} error
* @returns {boolean}
*/
function isMissingNodeError(error) {
/** @type {string[]} */
const messages = [getErrorMessage(error)];
/** @type {Array<{type?: unknown, message?: unknown, path?: unknown}>} */
const graphQLErrors = [];

if (error && typeof error === "object" && "errors" in error && Array.isArray(error.errors)) {
for (const graphQLError of error.errors) {
graphQLErrors.push(graphQLError);
if (typeof graphQLError?.message === "string") {
messages.push(graphQLError.message);
}
}
}

const hasNodeScopedNotFoundType = graphQLErrors.some(graphQLError => {
if (typeof graphQLError?.type !== "string" || graphQLError.type.toUpperCase() !== "NOT_FOUND") {
return false;
}
const hasNodeScopedMessage = typeof graphQLError?.message === "string" && graphQLError.message.toLowerCase().includes("could not resolve to a node");
const hasNodeScopedPath =
Array.isArray(graphQLError?.path) &&
graphQLError.path.some(pathPart => {
if (typeof pathPart !== "string") return false;
const normalized = pathPart.toLowerCase();
// GraphQL paths for stale-thread mutation failures are rooted at "resolveReviewThread".
return normalized === "node" || normalized === "resolvereviewthread";
});
return hasNodeScopedMessage || hasNodeScopedPath;
});
if (hasNodeScopedNotFoundType) {
return true;
}

return messages.some(message => {
const normalized = message.trim().toLowerCase();
// Match the stale-node GraphQL error, or Octokit's bare "Not Found" 404 message.
// Deliberately avoid a loose "not found" substring match so unrelated errors
// (e.g. "Repository not found") still surface as real failures.
return normalized.includes("could not resolve to a node") || normalized === "not found";
});
}

/**
* Main handler factory for resolve_pull_request_review_thread
* Returns a message handler function that processes individual resolve messages.
Expand Down Expand Up @@ -323,7 +374,22 @@ async function main(config = {}) {
}

// Look up the thread's PR number and repository
const threadInfo = await getThreadPullRequestInfo(githubClient, threadId);
/** @type {Awaited<ReturnType<typeof getThreadPullRequestInfo>>} */
let threadInfo;
try {
threadInfo = await getThreadPullRequestInfo(githubClient, threadId);
} catch (error) {
if (isMissingNodeError(error)) {
core.info(`Review thread ${threadId} could not be resolved (${getErrorMessage(error)}) — already resolved or stale; skipping`);
return {
success: true,
thread_id: threadId,
is_resolved: true,
skipped: true,
};
}
throw error;
}
if (threadInfo.status === "missing") {
core.info(`Review thread ${threadId} not found — already resolved or stale; skipping`);
return {
Expand Down Expand Up @@ -478,6 +544,15 @@ async function main(config = {}) {
try {
resolveResult = await resolveReviewThreadAPI(githubClient, resolvedThreadId);
} catch (error) {
if (isMissingNodeError(error)) {
core.info(`Review thread ${resolvedThreadId} could not be resolved (${getErrorMessage(error)}) — already resolved or stale; skipping`);
return {
success: true,
thread_id: resolvedThreadId,
is_resolved: true,
skipped: true,
};
}
if (isIntegrationAccessError(error)) {
const warningMessage =
`Skipping resolve_pull_request_review_thread for ${resolvedThreadId}: configuration mismatch ` +
Expand Down
102 changes: 100 additions & 2 deletions actions/setup/js/resolve_pr_review_thread.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,104 @@ describe("resolve_pr_review_thread", () => {
expect(mockGraphql).toHaveBeenCalledTimes(1);
});

it("should succeed as a no-op when the lookup throws a stale-node GraphQL error", async () => {
mockGraphql.mockImplementation(() => {
const error = new Error("Request failed");
error.errors = [{ type: "NOT_FOUND", message: "Could not resolve to a node with the global id of 'PRRT_kwDOPc1QR87fJc0o'" }];
return Promise.reject(error);
});

const { main } = require("./resolve_pr_review_thread.cjs");
const freshHandler = await main({ max: 10 });

const result = await freshHandler({ type: "resolve_pull_request_review_thread", thread_id: "PRRT_kwDOPc1QR87fJc0o" }, {});

expect(result.success).toBe(true);
expect(result.skipped).toBe(true);
expect(result.is_resolved).toBe(true);
expect(mockCore.error).not.toHaveBeenCalled();
});

it("should succeed as a no-op when the lookup throws a plain Not Found error", async () => {
mockGraphql.mockImplementation(() => Promise.reject(new Error("Not Found")));

const { main } = require("./resolve_pr_review_thread.cjs");
const freshHandler = await main({ max: 10 });

const result = await freshHandler({ type: "resolve_pull_request_review_thread", thread_id: "PRRT_stale" }, {});

expect(result.success).toBe(true);
expect(result.skipped).toBe(true);
expect(mockCore.error).not.toHaveBeenCalled();
});

it("should succeed as a no-op when the resolve mutation reports a stale node", async () => {
mockGraphql.mockImplementation(query => {
if (query.includes("resolveReviewThread")) {
const error = new Error("Could not resolve to a node with the global id of 'PRRT_kwDOABCD123456'");
return Promise.reject(error);
}
return Promise.resolve({
node: {
__typename: "PullRequestReviewThread",
id: "PRRT_kwDOABCD123456",
isResolved: false,
pullRequest: { number: 42, repository: { nameWithOwner: "test-owner/test-repo" } },
},
});
});

const { main } = require("./resolve_pr_review_thread.cjs");
const freshHandler = await main({ max: 10 });

const result = await freshHandler({ type: "resolve_pull_request_review_thread", thread_id: "PRRT_kwDOABCD123456" }, {});

expect(result.success).toBe(true);
expect(result.skipped).toBe(true);
expect(result.is_resolved).toBe(true);
expect(mockCore.error).not.toHaveBeenCalled();
});

it("should still fail for unrelated lookup errors", async () => {
mockGraphql.mockImplementation(() => Promise.reject(new Error("Internal server error")));

const { main } = require("./resolve_pr_review_thread.cjs");
const freshHandler = await main({ max: 10 });

const result = await freshHandler({ type: "resolve_pull_request_review_thread", thread_id: "PRRT_kwDOABCD123456" }, {});

expect(result.success).toBe(false);
expect(result.error).toContain("Internal server error");
});

it("should still fail for unrelated 'not found' errors such as a missing repository", async () => {
mockGraphql.mockImplementation(() => Promise.reject(new Error("Repository not found")));

const { main } = require("./resolve_pr_review_thread.cjs");
const freshHandler = await main({ max: 10 });

const result = await freshHandler({ type: "resolve_pull_request_review_thread", thread_id: "PRRT_kwDOABCD123456" }, {});

expect(result.success).toBe(false);
expect(result.error).toContain("Repository not found");
});

it("should still fail for structured NOT_FOUND errors unrelated to stale thread nodes", async () => {
mockGraphql.mockImplementation(() => {
const error = new Error("Repository not found");
error.errors = [{ type: "NOT_FOUND", message: "Repository not found", path: ["repository"] }];
return Promise.reject(error);
});

const { main } = require("./resolve_pr_review_thread.cjs");
const freshHandler = await main({ max: 10 });

const result = await freshHandler({ type: "resolve_pull_request_review_thread", thread_id: "PRRT_kwDOABCD123456" }, {});

expect(result.success).toBe(false);
expect(result.error).toContain("Repository not found");
});

it("should resolve a review comment node ID by finding its parent thread", async () => {
mockGraphql.mockImplementation(query => {
if (query.includes("resolveReviewThread")) {
Expand Down Expand Up @@ -582,7 +680,7 @@ describe("resolve_pr_review_thread", () => {
});

it("should handle API errors gracefully", async () => {
mockGraphql.mockRejectedValue(new Error("Could not resolve. Thread not found."));
mockGraphql.mockRejectedValue(new Error("Internal server error while loading thread"));

const message = {
type: "resolve_pull_request_review_thread",
Expand All @@ -592,7 +690,7 @@ describe("resolve_pr_review_thread", () => {
const result = await handler(message, {});

expect(result.success).toBe(false);
expect(result.error).toContain("Could not resolve");
expect(result.error).toContain("Internal server error");
});

it("should soft-skip when resolve mutation returns integration access error", async () => {
Expand Down
Loading