Skip to content

fix(USE-001): add standardized error codes to pr_review_buffer and set_issue_type handlers #41692

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 2 commits into from
Jun 26, 2026
Merged

fix(USE-001): add standardized error codes to pr_review_buffer and set_issue_type handlers #41692

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
de81259
Initial plan
Copilot Jun 26, 2026
664915e
fix(USE-001): add standardized error codes to pr_review_buffer and se…
Copilot Jun 26, 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
3 changes: 2 additions & 1 deletion actions/setup/js/pr_review_buffer.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ const { isStagedMode } = require("./safe_output_helpers.cjs");
const { generateWorkflowCallIdMarker, matchesWorkflowId } = require("./generate_footer.cjs");
const { attachExecutionState, fetchPullRequestReviewState } = require("./safe_output_execution_metadata.cjs");
const { withRetry, RATE_LIMIT_RETRY_CONFIG, isTransientError, sleep } = require("./error_recovery.cjs");
const { ERR_API } = require("./error_codes.cjs");

const SUPERSEDE_REVIEW_MESSAGE = "Superseded by updated review from same workflow.";
const MAX_SUPERSEDE_REVIEW_PAGES = 10;
Expand Down Expand Up @@ -126,7 +127,7 @@ function createReviewBuffer() {
return await fetchPullRequestReviewState(github, repoParts, pullRequestNumber);
} catch (error) {
if (!isTransientError(error)) {
throw new Error(`Failed to capture ${phase} PR review state for #${pullRequestNumber}: ${getErrorMessage(error)} (non-transient)`, { cause: error });
throw new Error(`${ERR_API}: Failed to capture ${phase} PR review state for #${pullRequestNumber}: ${getErrorMessage(error)} (non-transient)`, { cause: error });

@github-actions github-actions Bot Jun 26, 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.

[/tdd] The non-transient error branch (the one just fixed) isn't exercised by any test in pr_review_buffer.test.cjs — so the ERR_API: prefix could regress silently.

💡 Suggested test 
it("fetchReviewStateBestEffort rethrows non-transient errors with ERR_API prefix", async () => {
  const nonTransientErr = new Error("quota exceeded");
  mockFetchPullRequestReviewState.mockRejectedValue(nonTransientErr);
  mockIsTransientError.mockReturnValue(false);

  await expect(buffer.fetchReviewStateBestEffort(repoParts, 123, "before"))
    .rejects.toThrow(/^ERR_API:/);
});

Pairs naturally with the existing transient-error path test to give full branch coverage.

}
core.warning(`Failed to capture ${phase} PR review state for #${pullRequestNumber}: ${getErrorMessage(error)}. Continuing without execution-state metadata.`);
return null;
Expand Down
3 changes: 2 additions & 1 deletion actions/setup/js/set_issue_type.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ const { isStagedMode, checkRequiredFilter } = require("./safe_output_helpers.cjs
const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs");
const { resolveSafeOutputIssueTarget } = require("./temporary_id.cjs");
const { hasIssueIntentsRuntimeFeature, normalizeIssueIntentMetadata } = require("./issue_intents.cjs");
const { ERR_VALIDATION } = require("./error_codes.cjs");

/** @type {string} Safe output type handled by this module */
const HANDLER_TYPE = "set_issue_type";
Expand Down Expand Up @@ -108,7 +109,7 @@ function toRestIssueIntentMetadata(intentMetadata) {
restMetadata.confidence = "high";
break;
default:
throw new Error(`Invalid confidence ${JSON.stringify(intentMetadata.confidence)}. Expected one of: LOW, MEDIUM, HIGH.`);
throw new Error(`${ERR_VALIDATION}: Invalid confidence ${JSON.stringify(intentMetadata.confidence)}. Expected one of: LOW, MEDIUM, HIGH.`);

@github-actions github-actions Bot Jun 26, 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.

[/tdd] This fixed error path has no regression test — a future refactor could silently drop the ERR_VALIDATION: prefix and the conformance checker would only catch it in the next daily run.

💡 Suggested test 
it("throws ERR_VALIDATION for an unrecognised confidence value", () => {
  const intentMetadata = { confidence: "UNKNOWN" };
  expect(() => toRestIssueIntentMetadata(intentMetadata))
    .toThrow(/^ERR_VALIDATION:/);
});

This keeps the conformance guarantee self-verifying and avoids another USE-001 regression.

}
}
return restMetadata;
Expand Down
Loading