Use correct GitHubApiDetails instance for getApiClient in getCodeQLAssetDownloadURL - #4231
mbg wants to merge 7 commits into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
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 findingsReject repository paths without exactly two non-empty components · New
Test getApiClient receives download source API details · New
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. |
- 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.
henrymercer
left a comment
There was a problem hiding this comment.
This broadly looks good. A couple of suggestions:
Sorry, something went wrong.
| // 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 = [ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| * are only used with the GitHub instance that is represented by the {@link GitHubApiCombinedDetails} value. | ||
| */ | ||
| export class InstanceApiClient { | ||
| private readonly client: ApiClient; |
There was a problem hiding this comment.
Remove this for now since it's unused?
Sorry, something went wrong.
| const getRequiredInput = sinon.stub(actionsUtil, "getRequiredInput"); | ||
| getRequiredInput.withArgs("token").returns(apiDetails.auth); |
There was a problem hiding this comment.
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.
This branch has not been deployed
Reviewers
At least 1 approving review is required to merge this pull request.
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:
Which use cases does this change impact?
Workflow types:
Products:
Environments:
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