- Notifications
You must be signed in to change notification settings - Fork428
Overlay: Check database metadata for overlayBaseSpecifier#3340
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
35d5d53 to6391bd0Comparefd0d09f toaff036bComparekaspersv commentedDec 3, 2025
The failing |
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.
Pull request overview
This PR adds an additional validation check for overlay-base databases by runningcodeql resolve database and verifying the presence of anoverlayBaseSpecifier key in the database metadata. This check helps ensure that only valid overlay-base databases are uploaded to the cache or used for overlay analysis.
Key changes:
- New
resolveDatabasemethod added to the CodeQL CLI interface that runscodeql resolve database --format=json - Enhanced
checkOverlayBaseDatabasefunction to validate theoverlayBaseSpecifiermetadata for each language - Test coverage added for the new validation scenario where
overlayBaseSpecifieris missing
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/codeql.ts | AddedresolveDatabase method andResolveDatabaseOutput interface to support querying database metadata |
| src/overlay-database-utils.ts | Enhanced overlay-base database validation to check foroverlayBaseSpecifier usingcodeql resolve database |
| src/overlay-database-utils.test.ts | Added test case for missingoverlayBaseSpecifier and updated test infrastructure to mockresolveDatabase |
| src/testing-utils.ts | Changed default language increateTestConfig from empty array to[KnownLanguage.java] to support tests that iterate over languages |
| lib/*.js | Auto-generated JavaScript files reflecting the TypeScript changes |
Uh oh!
There was an error while loading.Please reload this page.
src/overlay-database-utils.ts Outdated
| resolveDatabaseOutput===undefined|| | ||
| !("overlayBaseSpecifier"inresolveDatabaseOutput) | ||
| ){ | ||
| logger.info(`${warningPrefix}: no overlayBaseSpecifier defined`); |
CopilotAIDec 3, 2025
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.
The logging level here should belogger.warning instead oflogger.info to be consistent with line 189 where a similar validation failure is logged as a warning. Validation failures that cause the function to return false should be warnings so they're visible in the logs.
| logger.info(`${warningPrefix}: no overlayBaseSpecifier defined`); | |
| logger.warning(`${warningPrefix}: no overlayBaseSpecifier defined`); |
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.
The info log-level is intentional. The extractor is allowed to decide not to produce an overlay-base. This will for instance happen for Go if the extractor is asked to produce an overlay-base for a traced build. In that case the Go extractor will decide not to produce an overlay-base and we shouldn't generate a warning for that case.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
c96efdb to505fdd0Compare505fdd0 toc4efbdaComparekaspersv commentedDec 3, 2025
Rebased on top of#3341. |
henrymercer left a comment
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.
A couple of suggestions otherwise this looks good 👍
Uh oh!
There was an error while loading.Please reload this page.
src/overlay-database-utils.ts Outdated
| logger.info(`${warningPrefix}: no overlayBaseSpecifier defined`); | ||
| returnfalse; | ||
| }else{ | ||
| logger.info( |
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.
Should this be a debug level log? I had a quick look and it looks like this is not within a log group.
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.
Yes, it makes sense to log at debug log-level in the else branch, to avoid logging unnecessarily in the default case where anoverlayBaseSpecifier is present. I would still like to keep the log that nooverlayBaseSpecifier was present at info level.
Co-authored-by: Henry Mercer <henrymercer@github.com>
c43362b intomainUh oh!
There was an error while loading.Please reload this page.
This PR adds an additional validity check for overlay-base databases before uploading an overlay-base to the actions cache or using an overlay-base for overlay analysis. The additional check consists of running
codeql resolve databaseand checking for the existence of anoverlayBaseSpecifierkey.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.analysis-kinds: code-quality.Environments:
github.com.How did/will you validate this change?
.test.tsfiles).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