10000 fix: change regex matching for API error to not contain regex boundaries by NlightNFotis · Pull Request #2832 · github/codeql-action · GitHub
[go: up one dir, main page]

Skip to content

fix: change regex matching for API error to not contain regex boundaries #2832

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.

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
merged 3 commits into from
Mar 28, 2025

Conversation

NlightNFotis
Copy link
Member

Description

This fixes an issue with a regex pattern match on API errors failing due to regex boundaries on the pattern, which would affect telemetry.

Merge / deployment checklist

Note: This change doesn't affect users and doesn't require any changes to the readme or the changelog.

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

@Copilot Copilot AI review requested due to automatic review settings March 28, 2025 14:40
@NlightNFotis NlightNFotis requested a review from a team as a code owner March 28, 2025 14:40
Copy link
Contributor
@Copilot 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 fixes an issue with the regex used to match API error messages by removing the regex boundaries so that error messages containing extra context can be detected for proper telemetry reporting. It also adds additional tests to verify the behavior of getActionsStatus and wrapApiConfigurationError functions across both TypeScript and JavaScript implementations.

  • Updated the regex pattern in API client files to remove boundaries.
  • Added test cases for getActionsStatus and API error wrapping behavior.
  • Extended imports and helper functions in test files to support the new error handling logic.

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/status-report.test.ts Added tests for various error types and updated imports
src/api-client.ts Modified regex to remove boundaries for improved API error matching
src/api-client.test.ts Added tests for the updated API error wrapping behavior
lib/status-report.test.js Added JavaScript tests mirroring TypeScript tests for error handling
lib/api-client.test.js Added JavaScript tests for API error wrapping functionality
lib/api-client.js Modified regex to remove boundaries in API error matching
Comments suppressed due to low confidence (3)

src/status-report.test.ts:214

  • The test description for a ConfigurationError with an additional failure cause states it should return 'failure', but the expected result is 'user-error'. Please update the description to accurately reflect the expected outcome.
t.is(getActionsStatus(new ConfigurationError("exit code 1"), "multiple things went wrong"), "user-error", "getActionsStatus should return failure if passed a configuration error and an additional failure cause");

src/api-client.ts:248

  • Removing the regex boundaries could allow unintended partial matches; please confirm that this broader matching is intended and consider adding comments or tests to ensure it doesn't capture unwanted error messages.
/ref .* not found in this repository/.test(e.message)

lib/api-client.js:209

  • Similar to the TypeScript version, removing the regex boundaries here may lead to broader than intended matches. Confirm this change with appropriate documentation or tests to avoid potential false positives.
/ref .* not found in this repository/.test(e.message)

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

Copy link
Contributor
@henrymercer henrymercer 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 adding tests!

@NlightNFotis NlightNFotis merged commit 9f45e74 into main Mar 28, 2025
286 of 288 checks passed
@NlightNFotis NlightNFotis deleted the NlightNFotis/fix_config_error_classification branch March 28, 2025 15:18
@github-actions github-actions bot mentioned this pull request Apr 7, 2025
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0