Address missing telemetry at the start of Actions by henrymercer · Pull Request #3415 · github/codeql-action · GitHub

The start of some of the Actions' entrypoints was not covered by a try/catch statement, resulting in missed telemetry if any of the code there errored.

Address it by pulling almost all the code into a try/catch statement for each of the Actions.

We could potentially also move this try/catch into the wrapper, but it can be useful to include other variables if they are set (like entries from the config) in the error telemetry.

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?

Workflow types:

Advanced setup - Impacts users who have custom CodeQL workflows.
Managed - Impacts users with dynamic workflows (Default Setup, CCR, ...).

Products:

Code Scanning - The changes impact analyses when analysis-kinds: code-scanning.
Code Quality - The changes impact analyses when analysis-kinds: code-quality.
CCR - The changes impact analyses for Copilot Code Reviews.
Third-party analyses - The changes affect the upload-sarif action.

Environments:

Dotcom - Impacts CodeQL workflows on github.com and/or GitHub Enterprise Cloud with Data Residency.
GHES - Impacts CodeQL workflows on 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.

Are there any special considerations for merging or releasing this change?

No special considerations - This change can be merged at any time.

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.

Pull request overview

This PR addresses missing telemetry coverage by wrapping initialization code in try/catch blocks across multiple action entry points. Previously, if errors occurred during early initialization (before the try/catch), they would not be captured in telemetry.

Changes:

Moved initialization code into existing try/catch blocks for five actions: upload-sarif, start-proxy, setup-codeql, init, and analyze
Updated variable declarations from const to let where needed to maintain proper scoping when variables are accessed in error handlers

Reviewed changes

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

Show a summary per file

File	Description
src/upload-sarif-action.ts	Moved initializeEnvironment, version checks, and starting status report inside try/catch
src/start-proxy-action.ts	Moved persistInputs inside try/catch
src/setup-codeql-action.ts	Moved initializeEnvironment, API details creation, and job UUID generation inside try/catch
src/init-action.ts	Moved initializeEnvironment, persistInputs, and early initialization inside try/catch; changed variable declarations to let for proper scoping
src/analyze-action.ts	Moved initializeEnvironment and persistInputs inside try/catch

Thank you for putting this together! This generally looks good. I have done some checking here to verify that the exceptions we get from the functions that are now in the try/catch blocks are of the types we'd want (see also my comment there).

Two higher-level questions:

Should we add comments at the start of the run functions to remind us that only variables which are shared between different scopes below should be declared outside the try block and should be initialised only using safe functions / ideally inside the try block?
Would it make sense to add try/catch blocks around the main run functions, where the catch block sends some status report if an exception does leak out?

getRequiredInput throws a ConfigurationError when the input is not supplied. That makes sense.

getRequiredEnvParam throws an Error rather than a ConfigurationError. We'd probably always expect these environment variables to be available, and if they're not it's probably nothing a user can do anything about, so Error probably makes sense for that.

Worth thinking about in general though, whether we want one or the other for a given input/env variable.

Observation: since this was previously outside the try/catch block, we also didn't get telemetry for failures here before.

I'd be surprised if these environment variables weren't set within the Actions environment, but agreed in general.

Would it make sense to add try/catch blocks around the main run functions, where the catch block sends some status report if an exception does leak out?

It's defensive, but I'd rather not just rely on the comments. Implemented.

Thank you for adding the extra error handling, and also spotting that one function call that wasn't in a try/catch block yet!

I spotted a few things that we should think about -- see the individual comments.

Few points here:

Should e be wrapped in getErrorMessage?
Change "error status report" to "unhandled error status report" or similar (possibly also in the function name)?
Re-throw the exception if isInTestMode() like in createStatusReportBase?

Yes
"unhandled" sounds better than "unexpected" 👍
Good idea

For start-proxy, we deliberately don't include the error to avoid leaking any sensitive information in the main handler in the (now) run function. We should probably omit the error here too.

This looks good now, thanks! Just one question and a couple of very minor points.

Intuitively this is unnecessary, because createStatusReportBase throws if isInTestMode() is true, so we should never have the statusReport === undefined case there.

Did you add this deliberately to catch a potential scenario where createStatusReportBase doesn't throw in test mode, but statusReport is undefined anyway?

Did you add this deliberately to catch a potential scenario where createStatusReportBase doesn't throw in test mode, but statusReport is undefined anyway?

Yes, but this is probably overly defensive. I'll remove it.

Minor: Perhaps add "unhandled" before "error status report", so it's more obvious what this log message relates to.

I'd originally held off since this is customer facing, but it's probably not confusing in retrospect.

Minor: Maybe change this to "Unhandled CodeQL Action error" or "Unhandled ${actionName} error" so it's a less generic prefix / less likely to clash with some error message we might get from elsewhere.

Expand try-catch to cover more of Actions

7c72e12

henrymercer requested a review from a team as a code owner January 22, 2026 10:49

Copilot AI review requested due to automatic review settings January 22, 2026 10:49

github-actions Bot added the size/S Should be easy to review label Jan 22, 2026

Copilot started reviewing on behalf of henrymercer January 22, 2026 10:50 View session

Copilot AI reviewed Jan 22, 2026

View reviewed changes

mbg previously approved these changes Jan 22, 2026

View reviewed changes

henrymercer added 3 commits January 22, 2026 11:27

Move config saving within try-catch

b715292

Add reminder to minimise code outside try/catch

14bd767

Add catch-all error reporting for errors that slip through run

229e0cd

henrymercer dismissed mbg’s stale review via 229e0cd January 22, 2026 13:21

github-actions Bot added size/M Should be of average difficulty to review and removed size/S Should be easy to review labels Jan 22, 2026

mbg reviewed Jan 22, 2026

View reviewed changes

henrymercer added 5 commits January 23, 2026 10:34

Rename to "unhandled"

6bd84b6

Use getErrorMessage in more places

529c266

Differentiate unhandled errors in telemetry

edebb78

Throw if in test mode

91f3460

Omit error from start-proxy Action

28bfb7b

henrymercer requested a review from mbg January 23, 2026 10:43

henrymercer changed the title Address missing telemetry for start of Actions Address missing telemetry at the start of Actions Jan 23, 2026

mbg previously approved these changes Jan 23, 2026

View reviewed changes

Address review comments

fba7872

henrymercer dismissed mbg’s stale review via fba7872 January 23, 2026 12:22

mbg approved these changes Jan 23, 2026

View reviewed changes

henrymercer enabled auto-merge January 23, 2026 12:25

henrymercer merged commit 1601acf into main Jan 23, 2026
250 checks passed

henrymercer deleted the henrymercer/address-telemetry-gap branch January 23, 2026 12:51

github-actions Bot mentioned this pull request Jan 23, 2026

Merge main into releases/v4 #3417

Merged

8 tasks

github-actions Bot mentioned this pull request Jan 23, 2026

Merge releases/v4 into releases/v3 #3419

Merged

8 tasks

+                    auth: getRequiredInput("token"),
+                    externalRepoAuth: getOptionalInput("external-repository-token"),
+                    url: getRequiredEnvParam("GITHUB_SERVER_URL"),
+                    apiURL: getRequiredEnvParam("GITHUB_API_URL"),

+                  await sendUnexpectedErrorStatusReport(
+                    ActionName.StartProxy,
+                    startedAt,
+                    error,

+                  } else if (isInTestMode()) {
+                    throw new Error(
+                      "Failed to create status report for unhandled error in test mode.",
+                    );

                     logger,
-                    undefined,
-                    getErrorMessage(error),
+                    `Unhandled error: ${getErrorMessage(error)}`,

+                  }
+                } catch (e) {
+                  logger.warning(
+                    `Caught an exception while sending the error status report: ${e}.`,

Address missing telemetry at the start of Actions by henrymercer · Pull Request #3415 · github/codeql-action · GitHub

Uh oh!

Address missing telemetry at the start of Actions - #3415

Address missing telemetry at the start of Actions#3415henrymercer merged 10 commits intomaingithub/codeql-action:mainfrom henrymercer/address-telemetry-gapgithub/codeql-action:henrymercer/address-telemetry-gapCopy head branch name to clipboard

Conversation

henrymercer commented Jan 22, 2026

Risk assessment

Which use cases does this change impact?

How did/will you validate this change?

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

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

Are there any special considerations for merging or releasing this change?

Merge / deployment checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

mbg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henrymercer commented Jan 22, 2026

Uh oh!

mbg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Address missing telemetry at the start of Actions#3415
henrymercer merged 10 commits into
mainfrom
henrymercer/address-telemetry-gap