feat: further error re-classification #2833

NlightNFotis · 2025-03-31T11:24:27Z

Description

This PR adds some further error classification for issues we observe in telemetry, along with a re-classification of InvalidSarifUploadErrors as user-errors.

Merge / deployment checklist

NOTE: This is an internal/telemetry change, and doesn't affect users in any way that would induce the need to update either the readme or the changelog file.

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.

…tatus report is produced

Copilot

Pull Request Overview

This PR refines how error classification is handled in telemetry by re-classifying InvalidSarifUploadError as a user error and improving error message matching for configuration and upload errors. Key changes include:

Updating error classification functions (shouldConsiderConfigurationError and shouldConsiderInvalidRequest) to use substring matching.
Modifying the getActionsStatus logic to treat InvalidSarifUploadError as a user-error.
Extending API client error handling to cover additional error message conditions.

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/upload-lib.ts	Updated error classification logic and exported the helper functions.
src/upload-lib.test.ts	Added tests validating new error classification behavior for configuration and invalid request errors.
src/status-report.ts & lib/status-report.js	Modified getActionsStatus to include InvalidSarifUploadError handling.
src/api-client.ts & lib/api-client.js	Extended error message matching in wrapApiConfigurationError.
Other test files	Added corresponding unit tests verifying updated error handling.

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

Copilot · 2025-03-31T11:24:54Z

src/upload-lib.ts

+  const expectedConfigErrors = [
+    "CodeQL analyses from advanced configurations cannot be processed when the default setup is enabled",
+    "rejecting delivery as the repository has too many logical alerts",
+  ];
+
  return (
    processingErrors.length === 1 &&
-    processingErrors[0] ===
-      "CodeQL analyses from advanced configurations cannot be processed when the default setup is enabled"
+    expectedConfigErrors.some((msg) => processingErrors[0].includes(msg))
  );


Using .includes for matching error messages can potentially lead to false positives if an error string contains an expected substring unintentionally. Consider using stricter matching or regex patterns to ensure only the intended error messages are classified.

NlightNFotis · 2025-03-31T11:25:51Z

Hi @henrymercer, would it be possible to get a review on this one?

I'd like some further eyes on f21cf0b, as I'm fairly confident the functionality works, but I'm not sure if we are to consider InvalidSarifUploadErrors as user-errors (I've seen discussions leaning both ways).

henrymercer · 2025-03-31T15:05:23Z

src/api-client.ts

+      e.message.includes("Bad credentials") ||
+      e.message.includes("Not Found") ||
+      e.message.includes("Resource not accessible by integration") ||


There's an opportunity to provide a better error message here too, for example for bad credentials / not found we could ask whether the job has contents: read and security-events: write permissions.

Thanks for the suggestion, I have implemented this at 55ee663.

henrymercer · 2025-03-31T15:08:27Z

src/status-report.ts

-    return error instanceof ConfigurationError ? "user-error" : "failure";
+    if (
+      error instanceof ConfigurationError ||
+      error instanceof InvalidSarifUploadError


This should only be a user error if a third-party tool generated the SARIF. See the handling of InvalidSarifUploadError in src/upload-sarif-action.ts.

I put some thought into trying to come up with a schema for identifying how to handle this case, and I came to realisation that perhaps the best way to implement this is by passing getActionsStatus an extra parameter indicating whether the analysis is a third party one or not. This would also make it easier to extend it in the future based on this information.

I have implemented this approach in 55ee663.

I'd prefer to keep getActionsStatus simple and delegate the task of deciding whether errors are user errors or not to the part of the code where the relevant logic is implemented.

For example, the upload-sarif Action we should catch any InvalidSarifUploadErrors and rethrow them as a ConfigurationError if it's a third-party run. So by the time we reach the top-level status reporting logic, we should only be dealing with InvalidSarifUploadErrors that aren't user errors.

If we're seeing InvalidSarifUploadErrors from third-party runs, there's a bug in the upload-sarif Action — we should fix that to make sure it's wrapping those errors in ConfigurationErrors.

Thank you, I have re-implemented it this way, managing to repurpose some of the code already written to simplify the code and its handling: 498c7f3

henrymercer · 2025-03-31T15:09:53Z

src/upload-lib.ts

  return processingErrors.every(
    (error) =>
      error.startsWith("rejecting SARIF") ||
      error.startsWith("an invalid URI was provided as a SARIF location") ||
      error.startsWith("locationFromSarifResult: expected artifact location") ||
+      error.startsWith("SyntaxError: Unexpected end of JSON input") ||


How's this possible if we're validating the SARIF against the JSON schema before we try to upload it? I'd be surprised if this was going wrong, but is there a bug in the way we combine the SARIF files?

Are we getting this error when we're doing the validation locally? And if so, does this function catch the error?

If we're getting this error from the server, I would say this is a bug.

Henry is correct, I misunderstood the error location and thought this was coming from the backend. Judging from the stack trace I've seen, this appears to be coming from the uploadFiles function calling validateSarifFileSchema on each input file, which then fails the validation.

In this case, the upload-sarif-action.ts would be the best location for that, and we also probably want to categorise as a user-error the case where we ourselves are not the producers of a SARIF file with syntax errors.

I have now corrected this at b53826d

…er designating if the analysis is third-party

aeisenberg · 2025-04-01T17:20:05Z

src/resolve-environment-action.ts

+      const isThirdPartyAnalysis = !isFirstPartyAnalysis(
+        ActionName.ResolveEnvironment,
+      );


Can you convert this to a function in status-report.ts? You are duplicating and negating the call to isFirstPartyAnalysis in enough places that it might be cleaner to just extract it to a separate function.

henrymercer

(posting this review so Slack doesn't keep reminding me)

See #2833 (comment)

…as ConfigurationError if in known error category

henrymercer

Thanks for addressing the feedback! Looks good, just one suggestion.

henrymercer · 2025-04-02T17:09:11Z

src/upload-sarif-action.ts

+      // There was a problem validating the JSON (SARIF) file.
+      unwrappedError instanceof SyntaxError;


There's a small chance that by catching any SyntaxError here we miss something genuine. If we put a try/catch around the JSON parse block specifically, and throw an InvalidSarifUploadError if there was invalid JSON, we can avoid that situation.

Thanks, this is a great suggestion! Added in 676a422

…better discriminate error thrown

NlightNFotis added 4 commits March 31, 2025 11:51

fix: update comment for test to state correct expected outcome

3c42562

feat: classify more HTTP errors as configuration errors in api-client

a022653

feat: classify some observed SARIF errors as InvalidSarifUploadError

72a2b12

feat: reclassify InvalidSarifUploadError as a user-error when final s…

f21cf0b

…tatus report is produced

Copilot bot review requested due to automatic review settings March 31, 2025 11:24

NlightNFotis requested a review from a team as a code owner March 31, 2025 11:24

NlightNFotis requested a review from henrymercer March 31, 2025 11:24

Copilot AI reviewed Mar 31, 2025

View reviewed changes

henrymercer reviewed Mar 31, 2025

View reviewed changes

NlightNFotis added 3 commits March 31, 2025 16:29

Merge branch 'main' into NlightNFotis/reclassify_upload_sarif_issues

01f1a1f

review-comments: refactor getActionsStatus to accept an extra paramet…

55ee663

…er designating if the analysis is third-party

review-comments: remove syntax-error handling for SARIF from upload-lib

b53826d

NlightNFotis requested a review from henrymercer April 1, 2025 14:15

aeisenberg reviewed Apr 1, 2025

View reviewed changes

henrymercer reviewed Apr 2, 2025

View reviewed changes

NlightNFotis added 2 commits April 2, 2025 15:13

refactor: revert getActionsStatus taking an extra argument

efd29be

review-comments: unwrap error in upload-sarif-action and re-classify …

498c7f3

…as ConfigurationError if in known error category

NlightNFotis requested a review from henrymercer April 2, 2025 14:23

henrymercer reviewed Apr 2, 2025

View reviewed changes

NlightNFotis and others added 2 commits April 2, 2025 19:06

review-comments: nest validateSariFileSchema into try-catch block to …

676a422

…better discriminate error thrown

Merge branch 'main' into NlightNFotis/reclassify_upload_sarif_issues

06703ce

henrymercer approved these changes Apr 2, 2025

View reviewed changes

NlightNFotis merged commit e13fe0d into main Apr 2, 2025
270 checks passed

NlightNFotis deleted the NlightNFotis/reclassify_upload_sarif_issues branch April 2, 2025 19:09

github-actions bot mentioned this pull request Apr 7, 2025

Merge main into releases/v3 #2844

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: further error re-classification #2833

feat: further error re-classification #2833

NlightNFotis commented Mar 31, 2025

Copilot AI left a comment

Copilot AI Mar 31, 2025

NlightNFotis commented Mar 31, 2025

henrymercer Mar 31, 2025

NlightNFotis Apr 1, 2025

henrymercer Mar 31, 2025

NlightNFotis Apr 1, 2025

henrymercer Apr 1, 2025

NlightNFotis Apr 2, 2025

henrymercer Mar 31, 2025

aeisenberg Mar 31, 2025

NlightNFotis Apr 1, 2025

aeisenberg Apr 1, 2025

henrymercer left a comment

henrymercer left a comment

henrymercer Apr 2, 2025

NlightNFotis Apr 2, 2025

		// There was a problem validating the JSON (SARIF) file.
		unwrappedError instanceof SyntaxError;

feat: further error re-classification #2833

feat: further error re-classification #2833

Conversation

NlightNFotis commented Mar 31, 2025

Description

Merge / deployment checklist

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Copilot AI Mar 31, 2025

Choose a reason for hiding this comment

NlightNFotis commented Mar 31, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henrymercer left a comment

Choose a reason for hiding this comment

henrymercer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment