-
Notifications
You must be signed in to change notification settings - Fork 434
Support ignoring generated files #3318
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
2fac308 to
b4db382
Compare
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 experimental support for excluding generated files (marked with linguist-generated=true in .gitattributes) from CodeQL analysis. The feature is controlled by the ignore_generated_files feature flag and is automatically enabled for Copilot Code Reviews (CCR).
Key Changes
- New git utility functions (
listFiles,getGeneratedFiles) to identify generated files via git attributes - New
isCCR()helper function to detect CCR execution context - Integration into config initialization to add generated files to
paths-ignore
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/git-utils.ts | Adds functions to list tracked files and identify files marked as generated via linguist-generated attribute |
| src/git-utils.test.ts | Adds unit tests for the new git utility functions |
| src/feature-flags.ts | Defines the IgnoreGeneratedFiles feature flag with environment variable CODEQL_ACTION_IGNORE_GENERATED_FILES |
| src/config-utils.ts | Integrates generated file detection into config initialization, adding them to paths-ignore when feature is enabled or in CCR |
| src/actions-util.ts | Adds isCCR() detection function and modifies isDefaultSetup() to exclude CCR scenarios |
| src/actions-util.test.ts | Adds tests for the new workflow detection functions |
| lib/*.js | Auto-generated JavaScript code mirroring TypeScript changes (not reviewed per guidelines) |
| ["ls-files"], | ||
| "Unable to list tracked files.", | ||
| ); | ||
| return stdout.split(os.EOL); |
Copilot
AI
Nov 19, 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.
Splitting on os.EOL will include an empty string at the end of the array if the git output ends with a newline (which is typical). This empty string will then be passed to git check-attr in getGeneratedFiles. Consider filtering out empty strings: return stdout.split(os.EOL).filter((line) => line.length > 0);
| return stdout.split(os.EOL); | |
| return stdout.split(os.EOL).filter((line) => line.length > 0); |
|
|
||
| const generatedFiles: string[] = []; | ||
| const regex = /^([^:]+): linguist-generated: true$/; | ||
| for (const result of stdout.split(os.EOL)) { |
Copilot
AI
Nov 19, 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.
Splitting on os.EOL will include an empty string at the end of the array if the git output ends with a newline (which is typical). The regex won't match empty strings, but it's cleaner to filter them out explicitly: for (const result of stdout.split(os.EOL).filter((line) => line.length > 0)). This pattern is used elsewhere in the codebase (e.g., line 284 checks if (line) before processing).
| for (const result of stdout.split(os.EOL)) { | |
| for (const result of stdout.split(os.EOL).filter((line) => line.length > 0)) { |
| } | ||
|
|
||
| /** Determines whether we are running in CCR. */ | ||
| export function isCCR(): boolean { |
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.
What about introducing an environment variable we set in CCR, rather than relying on the analysis key?
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 discussed elsewhere, that is sort of what we are doing here already (and we decide on whether are in CCR in other places in the same way currently).
A better solution would be to add a new analysis kind (or similar) for CCR, but that would be more work and currently the same as code-quality in essentially everything but name.
I'd suggest we should stick with this for the moment (since we also use the same approach elsewhere) and look at improving it longer-term.
| export async function getGeneratedFiles( | ||
| workingDirectory: string, | ||
| ): Promise<string[]> { | ||
| const files = await listFiles(workingDirectory); |
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 could potentially be a very large number of files, too many to pass on the command line.
If we're mainly interested in CCR, could we filter down to just the diff here?
Alternatively, we could parse globs from the .gitattributes file rather than finding all files that match. That would be more likely to contain one entry for a large directory rather than potentially hundreds.
Or we could add a limit on the number of files on which we'll run check-attr.
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 could potentially be a very large number of files, too many to pass on the command line.
True. The files could be passed in via stdin as well I think, which might be a more robust solution.
If we're mainly interested in CCR, could we filter down to just the diff here?
Potentially. We should first look at whether there is actually enough of a performance hit for large repos for this to make sense though.
Alternatively, we could parse globs from the .gitattributes file rather than finding all files that match. That would be more likely to contain one entry for a large directory rather than potentially hundreds.
I considered this, but I'd like to avoid it if we can get the information from git itself so we don't have to try and duplicate what it does.
| export async function getGeneratedFiles( | ||
| workingDirectory: string, | ||
| ): Promise<string[]> { |
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.
Have you measured how long this operation takes overall on a large mono-repo?
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.
I believe it should be relatively quick, since Git caches this information IIRC. That said, I haven't explicitly tested it on a large repo, but I can do that.
The analysis key already tells us this under normal conditions
This PR adds experimental support for excluding files that are marked as
linguist-generated=truein a.gitattributesfile from analysis.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:
Environments:
github.com.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