Require tools feature for uploading overlay DBs #3375
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a tools feature requirement to ensure overlay databases are only uploaded when using a CodeQL CLI that supports the bundleSupportsOverlay feature. This prevents uploading overlay databases with CLI versions that don't include a critical metadata file in the database bundle command.
Key changes:
- Adds
BundleSupportsOverlaytools feature enum value - Associates the feature flag
UploadOverlayDbToApiwith the new tools feature requirement - Refactors variable scoping to include database size in error reports
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/tools-features.ts | Adds new BundleSupportsOverlay enum value to the ToolsFeature enum |
| src/feature-flags.ts | Associates the UploadOverlayDbToApi feature flag with the BundleSupportsOverlay tools feature |
| src/database-upload.ts | Refactors bundledDbSize variable scope to capture size earlier and include it in error reports |
| lib/*.js | Auto-generated JavaScript files reflecting the TypeScript source changes |
mbg
left a comment
mbg
left a comment
There was a problem hiding this comment.
One question, otherwise this looks reasonable.
src/database-upload.ts
Outdated
| reports.push({ | ||
| language, | ||
| error: util.getErrorMessage(e), | ||
| ...(bundledDbSize ? { zipped_upload_size_bytes: bundledDbSize } : {}), |
There was a problem hiding this comment.
Is using the truthiness of bundledDbSize a problem here? E.g. do we care that this would be false if bundledDbSize was 0 rather than undefined?
There was a problem hiding this comment.
It shouldn't be 0, but worth correcting 👍
| ...(bundledDbSize !== undefined | ||
| ? { zipped_upload_size_bytes: bundledDbSize } | ||
| : {}), |
There was a problem hiding this comment.
Is this not equivalent to just
| ...(bundledDbSize !== undefined | |
| ? { zipped_upload_size_bytes: bundledDbSize } | |
| : {}), | |
| zipped_upload_size_bytes: bundledDbSize |
There was a problem hiding this comment.
➜ ~ node
Welcome to Node.js v25.2.1.
Type ".help" for more information.
> let x = {}
undefined
> x["a"] = undefined
undefined
> x
{ a: undefined }
There was a problem hiding this comment.
Yeah, but practically speaking there is not typically a difference in using an object with a key that has undefined as value vs a key that is absent. Asking for the value of the key gives undefined in both cases. The question is whether we care about the distinction that the key itself exists or not.
There was a problem hiding this comment.
That's fair. I'd prefer to leave this to the future and address it across the codebase.
mbg
left a comment
mbg
left a comment
There was a problem hiding this comment.
No strong feelings about the above conversation, so happy to approve.
The
database bundlecommand does not currently include a critical overlay database metadata file. Support is being added in conjunction with thebundleSupportsOverlaytools feature. This PR only uploads overlay DBs when using a CLI with that tool feature.Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Workflow types:
dynamicworkflows (Default Setup, CCR, ...).Products:
analysis-kinds: code-scanning.Environments:
github.comand/or GitHub Enterprise Cloud with Data Residency.How did/will you validate this change?
.test.tsfiles).pr-checks).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