-
Notifications
You must be signed in to change notification settings - Fork 401
Handle user errors for invalid UserConfig
s and missing query files
#3203
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
- Throws a `ConfigurationError` if parsing the YAML fails - Add a couple of tests for it
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 improves error handling for CodeQL configuration by classifying invalid config files and missing query files as ConfigurationError
s. The changes introduce JSON schema validation for config files and better error classification to provide users with clearer feedback when their configurations are malformed.
- Adds JSON schema validation for user config files with structured error reporting
- Expands CLI error classification to include missing query file scenarios
- Updates type definitions to better handle query input formats
Reviewed Changes
Copilot reviewed 17 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/error-messages.ts |
Adds new error message functions for config file parsing and validation errors |
src/db-config-schema.json |
Defines comprehensive JSON schema for CodeQL database configuration validation |
src/config/db-config.ts |
Implements parseUserConfig function with JSON schema validation and updates type definitions |
src/config/db-config.test.ts |
Adds tests for the new parseUserConfig function covering valid YAML and error cases |
src/config-utils.ts |
Updates config loading to use the new parseUserConfig function for better error handling |
src/cli-errors.ts |
Expands error pattern matching to include missing query file scenarios |
src/cli-errors.test.ts |
Adds test case for unknown query file error classification |
src/status-report.ts |
Updates query rendering to use new renderQueryInput helper function |
lib/* |
Generated JavaScript files reflecting the TypeScript changes |
), | ||
); | ||
} | ||
|
Copilot
AI
Oct 12, 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 function should validate that the parsed document conforms to the UserConfig interface before casting. Consider adding runtime type checking or at least documenting the assumption that schema validation guarantees type safety.
// Assumption: The JSON schema used above is kept in sync with the UserConfig TypeScript interface. | |
// As long as schema validation passes, it is safe to cast to UserConfig. |
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <[email protected]>
6aa2711
to
c71b45c
Compare
throw new ConfigurationError( | ||
errorMessages.getInvalidConfigFileMessage( | ||
pathInput, | ||
`The configuration file contained ${result.errors.length} error(s)`, | ||
), | ||
); |
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.
Could we list the errors?
name?: string; | ||
"disable-default-queries"?: boolean; | ||
queries?: QuerySpec[]; | ||
queries?: QueryInput[]; |
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 config file only accepts query specs. I double checked this here: https://github.com/github/codeql-action/actions/runs/18463233951
"type": "array", | ||
"description": "List of additional queries to run", | ||
"items": { | ||
"$ref": "#/definitions/QueryInput" |
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 should be just QuerySpec
}, | ||
"paths-ignore": { | ||
"type": "array", | ||
"description": "Paths to ignore during analysis", |
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.
"description": "Paths to ignore during analysis", | |
"description": "Paths to exclude from analysis", |
"description": "Query filter that can either include or exclude queries", | ||
"oneOf": [ | ||
{ | ||
"$ref": "#/definitions/ExcludeQueryFilter" | ||
}, | ||
{ | ||
"$ref": "#/definitions/IncludeQueryFilter" | ||
} | ||
] |
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.
It's possible to specify neither include
nor exclude
, and it's allowable, although unusual, to specify both an include
and an exclude
. I think we should just specify that the include
and exclude
properties are both optional.
Classifies two additional types of errors as
ConfigurationError
s:Best reviewed commit-by-commit.
Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
analysis-kinds: code-scanning
).analysis-kinds: code-quality
).How did/will you validate this change?
.test.ts
files).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?
Merge / deployment checklist