Skip to content

Conversation

@henrymercer
Copy link
Contributor

Update generic API configuration error handling to handle HTTP errors with the httpStatusCode property instead of the statusCode property, and update the CodeQL setup logic to use this error handling.

Risk assessment

For internal use only. Please select the risk level of this change:

  • Low risk: Changes are fully under feature flags, or have been fully tested and validated in pre-production environments and are highly observable, or are documentation or test only.

Which use cases does this change impact?

  • Advanced setup - Impacts users who have custom workflows.
  • Default setup - Impacts users who use default setup.
  • Code Scanning - Impacts Code Scanning (i.e. analysis-kinds: code-scanning).
  • Code Quality - Impacts Code Quality (i.e. analysis-kinds: code-quality).
  • Third-party analyses - Impacts third-party analyses (i.e. upload-sarif).
  • GHES - Impacts GitHub Enterprise Server.

How did/will you validate this change?

  • Unit tests - I am depending on unit test coverage (i.e. tests in .test.ts files).
  • End-to-end tests - I am depending on PR checks (i.e. tests in pr-checks).

If something goes wrong after this change is released, what are the mitigation and rollback strategies?

  • Rollback - Change can only be disabled by rolling back the release or releasing a new version with a fix.

How will you know if something goes wrong after this change is released?

  • Telemetry - I rely on existing telemetry or have made changes to the telemetry.
    • Alerts - New or existing monitors will trip if something goes wrong with this change.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Consider adding a changelog entry for this change.
  • Confirm the readme and docs have been updated if necessary.

@henrymercer henrymercer requested a review from a team as a code owner October 20, 2025 14:03
Copilot AI review requested due to automatic review settings October 20, 2025 14:03
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances error handling for API configuration errors by updating the HTTP error detection mechanism and extending error handling coverage to the CodeQL setup logic. The main improvement is replacing the simple isHTTPError type check with a more robust asHTTPError function that can handle different HTTP error formats.

Key Changes

  • Replaced isHTTPError boolean function with asHTTPError converter function that normalizes HTTP errors
  • Extended API configuration error wrapping to handle rate limiting (429 status) as configuration errors
  • Updated CodeQL setup logic to use the improved error handling wrapper

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/util.ts Replaced isHTTPError with asHTTPError function to handle both status and httpStatusCode properties
src/api-client.ts Enhanced wrapApiConfigurationError to handle 429 rate limit errors and use new HTTP error detection
src/codeql.ts Updated CodeQL setup to use API configuration error wrapping and removed specific RequestError handling
src/upload-lib.ts Updated to use new asHTTPError function for consistent error handling
src/trap-caching.ts Updated import and usage to use new HTTP error detection method
src/status-report.ts Updated import and usage to use new HTTP error detection method
src/feature-flags.ts Updated to use new HTTP error detection method
lib/*.js Generated JavaScript files reflecting the TypeScript changes

Comment on lines 286 to 289
httpError.message.includes("API rate limit exceeded") ||
httpError.message.includes("commit not found") ||
httpError.message.includes("Resource not accessible by integration") ||
/ref .* not found in this repository/.test(httpError.message)
Copy link

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

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

[nitpick] The error message check has been made more generic by removing 'for installation', but this could potentially match unintended error messages. Consider using a more specific pattern or combining with status code checks for better accuracy.

Suggested change
httpError.message.includes("API rate limit exceeded") ||
httpError.message.includes("commit not found") ||
httpError.message.includes("Resource not accessible by integration") ||
/ref .* not found in this repository/.test(httpError.message)
((httpError.message.includes("API rate limit exceeded") &&
(httpError.status === 403 || httpError.status === 429)) ||
httpError.message.includes("commit not found") ||
httpError.message.includes("Resource not accessible by integration") ||
/ref .* not found in this repository/.test(httpError.message))

Copilot uses AI. Check for mistakes.
} catch (rawError) {
const e = api.wrapApiConfigurationError(rawError);
const ErrorClass =
e instanceof util.ConfigurationError ||
Copy link

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

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

[nitpick] The removal of the RequestError status 429 check from the ErrorClass condition logic appears intentional since this is now handled by wrapApiConfigurationError, but the comment should be updated to reflect that rate limiting is now handled upstream.

Suggested change
e instanceof util.ConfigurationError ||
e instanceof util.ConfigurationError ||
// Rate limiting (HTTP 429) is now handled upstream by wrapApiConfigurationError.

Copilot uses AI. Check for mistakes.
mbg
mbg previously approved these changes Oct 20, 2025
Copy link
Member

@mbg mbg left a comment

Choose a reason for hiding this comment

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

LGTM. Just a couple of small suggestions I thought of while reviewing.

@henrymercer henrymercer force-pushed the henrymercer/http-error-handling branch from a0c5447 to e6e649a Compare October 21, 2025 09:32
@github-actions github-actions bot added the size/S Should be easy to review label Oct 21, 2025
@henrymercer henrymercer requested a review from mbg October 21, 2025 10:31
Copy link
Member

@mbg mbg left a comment

Choose a reason for hiding this comment

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

Thanks for implementing those suggestions, and even making the checks in asHTTPError more thorough than suggested!

@henrymercer henrymercer merged commit 8e53c48 into main Oct 21, 2025
243 checks passed
@henrymercer henrymercer deleted the henrymercer/http-error-handling branch October 21, 2025 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Should be easy to review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants