GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Use correct `GitHubApiDetails` instance for `getApiClient` in `getCodeQLAssetDownloadURL` by mbg · Pull Request #4231 · github/codeql-action

Conversation

mbg commented Oct 13, 2026

Copy link

Copy Markdown

Member

Based on #4230 with some cherry-picked commits from #4198 along with a final, new commit that passes the correct GitHubApiDetails instance for getApiClient in getCodeQLAssetDownloadURL to address the point discussed in #4198 (comment).

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, Code Quality, ...).

Products:

Code Scanning - The changes impact analyses when analysis-kinds: code-scanning.
Code Quality - The changes impact analyses when analysis-kinds: code-quality.
Other first-party - The changes impact other first-party analyses.

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.
- Dashboards - I will watch relevant dashboards for issues after the release. Consider whether this requires this change to be released at a particular time rather than as part of a regular release.
- 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

github-actions Bot added the size/L May be hard to review label

Oct 13, 2026

mbg added this pull request to stack #4232

October 13, 2026 17:46

mbg marked this pull request as ready for review

October 14, 2026 09:33

mbg requested a review from a team as a code owner

October 14, 2026 09:33

Copilot AI balanced review requested due to automatic review settings

October 14, 2026 09:33

Copilot started reviewing on behalf of mbg

October 14, 2026 09:34

View session

Copilot AI reviewed

Oct 14, 2026

View reviewed changes

Copilot AI left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Warning

Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.

🟡 Changes recommended

The API-routing regression lacks direct coverage, and repository validation accepts malformed values.

2 open findings

What changed in this PR

Refactors CodeQL asset resolution so API requests use the correct GitHub instance and authentication details.

Changes:

Extracts repository and asset-resolution logic into a shared module.
Supports unauthenticated GitHub.com API details when running elsewhere.
Updates tests, helpers, generated output, and dependencies.

File	Description
`src/setup/repository.ts`	Adds shared release-asset resolution.
`src/setup/repository.test.ts`	Tests Action repository detection.
`src/api-client.ts`	Supports explicit and Dotcom API details.
`src/setup-codeql.ts`	Uses shared asset resolution.
`src/setup-codeql.test.ts`	Updates setup tests for API details.
`src/testing-utils.ts`	Adds reusable API-details mocking.
`src/codeql.test.ts`	Adopts the shared test helper.
`src/repository.ts`	Adds typed repository NWO strings.
`src/actions-util.ts`	Accepts read-only environments.
`package.json`	Removes a direct dependency.
`package-lock.json`	Updates dependency metadata.
`lib/entry-points.js`	Generated bundle; content excluded.

Files excluded by content exclusion policy (1)

lib/entry-points.js

🧠 Review effort: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/repository.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/setup/repository.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

mbg force-pushed the mbg/use-github-instance-for-getApiClient branch 2 times, most recently from 569b0a1 to 79f834f Compare

October 14, 2026 12:47

mbg force-pushed the mbg/use-github-instance-for-getApiClient branch from 79f834f to bd015c2 Compare

October 14, 2026 12:54

github-actions Bot added size/XL May be very hard to review and removed size/L May be hard to review labels

Oct 14, 2026

mbg added 4 commits

October 14, 2026 20:31


          Add getDotcomApiDetails

f728bb6


          Use instance in DownloadSource and pass it on to getApiClient

f2547e0


          Include apiDetails in CodeQLDownloadSource

b2bf750


          Add InstanceApiClient and incorporate getAuthorizationHeaderFor i…

8884b5b

…nto it

mbg force-pushed the mbg/use-github-instance-for-getApiClient branch from bd015c2 to 8884b5b Compare

October 14, 2026 19:50

github-actions Bot added size/L May be hard to review and removed size/XL May be very hard to review labels

Oct 14, 2026

henrymercer reviewed

Oct 15, 2026

View reviewed changes

henrymercer left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This broadly looks good. A couple of suggestions:

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/setup/repository.ts

Comment on lines +100 to +103

		// We construct the default GitHub.com source here, because `getDotcomApiDetails`
		// may retrieve the `token` input if we are currently running on GitHub.com and
		// we want to avoid doing that statically.
		const defaultDownloadSource: DownloadSource = [

henrymercer Oct 15, 2026

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

As far as I can tell, retrieving the token input here is what necessitates the changes to the tests including the mockApiDetails helper which you called out as not ideal.

Since we're only using these apiDetails in isSameDownloadSource, we could consider doing something like { auth: undefined, ...getDotcomInstance() } instead to avoid the need to get the token input. I think the mockApiDetails helper could then be removed.

Even if we want to use the token constructed here in the future, I'd argue it'd be more elegant to first migrate code paths to use ActionState, so we can avoid the need to stub getRequiredInput (resulting in serial tests).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

mbg Oct 16, 2026

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

As far as I can tell, retrieving the token input here is what necessitates the changes to the tests including the mockApiDetails helper which you called out as not ideal.

Just for the record, the mockApiDetails function and the comment in it aren't new -- I just moved the definition from codeql.test.ts to testing-utils.ts and updated it to return the stub so that I could reset it in one test where I had to use it in a loop.

Since we're only using these apiDetails in isSameDownloadSource, we could consider doing something like { auth: undefined, ...getDotcomInstance() } instead to avoid the need to get the token input. I think the mockApiDetails helper could then be removed.

I think that this would introduce another issue similar to the one that this PR and the previous one in the stack are trying to get rid of. If we explicitly set auth: undefined here now, it will be difficult to identify this issue in the future if this is ever used for something other than isSameDownloadSource here.

Even if we want to use the token constructed here in the future, I'd argue it'd be more elegant to first migrate code paths to use ActionState, so we can avoid the need to stub getRequiredInput (resulting in serial tests).

I'll have a look at making this change, but it depends a bit on how far through the call stack we need to thread it to get the tests to not need mockApiDetails anymore. I don't want to have to refactor all of setup-codeql for this right now.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/api-client.ts

		* are only used with the GitHub instance that is represented by the {@link GitHubApiCombinedDetails} value.
		*/
		export class InstanceApiClient {
		private readonly client: ApiClient;

henrymercer Oct 15, 2026

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Remove this for now since it's unused?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/api-client.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/testing-utils.ts

Comment on lines +64 to +65

		const getRequiredInput = sinon.stub(actionsUtil, "getRequiredInput");
		getRequiredInput.withArgs("token").returns(apiDetails.auth);

henrymercer Oct 15, 2026

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

See my other comment about this, but if we keep this then we should at least have the stub call through for other arguments.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Base automatically changed from mbg/auth/allow-unauthed to main

October 16, 2026 11:18

mbg added 3 commits

October 16, 2026 12:30


          Merge remote-tracking branch 'origin/main' into mbg/use-github-instan…

1772d44

…ce-for-getApiClient


          Address feedback from previous PR

1c350b1


          Explain when to use getAuthorizationHeaderFor

d9e3972

This branch has not been deployed

No deployments

Reviewers

henrymercer

henrymercer left review comments

Copilot code review Copilot

Copilot review effort

Applies to this pull request for everyone.Learn more about Copilot code review.

Copilot left review comments

At least 1 approving review is required to merge this pull request.

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

size/L

May be hard to review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.