Use json module for JSON validation in output-cache - #4096
Conversation
1a0c458 to
93d793c
Compare
There was a problem hiding this comment.
Pull request overview
Refactors output-cache validation to use shared JSON schema utilities.
Changes:
- Validates version information using JSON schemas.
- Validates cache entries before accessing the cached version.
Show a summary per file
| File | Description |
|---|---|
src/cli/output-cache.ts |
Replaces manual checks with shared JSON validators. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 1/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| json.validateSchema( | ||
| { | ||
| version: json.string, | ||
| features: json.optional(json.object({})), | ||
| overlayVersion: json.optional(json.number), | ||
| } as const satisfies json.Schema, | ||
| x, |
There was a problem hiding this comment.
Good catch, @mario-campos can you address this? You already have COMMAND_CACHE_FILENAME as a constant, export it and use it in the tests instead of duplicating the filename there.
mbg
left a comment
There was a problem hiding this comment.
Looks OK, but a good catch from Copilot there that we missed on the previous PR. I also added a comment about avoiding the disconnect between the schema and VersionInfo type. When using the json module for schemas, it is desirable to derive the corresponding type with FromSchema to avoid them going out of sync.
| json.validateSchema( | ||
| { | ||
| version: json.string, | ||
| features: json.optional(json.object({})), | ||
| overlayVersion: json.optional(json.number), | ||
| } as const satisfies json.Schema, | ||
| x, |
There was a problem hiding this comment.
Good catch, @mario-campos can you address this? You already have COMMAND_CACHE_FILENAME as a constant, export it and use it in the tests instead of duplicating the filename there.
| { | ||
| version: json.string, | ||
| features: json.optional(json.object({})), | ||
| overlayVersion: json.optional(json.number), | ||
| } as const satisfies json.Schema, |
There was a problem hiding this comment.
Minor: Lift the schemas out of the function definitions to make them named, top-level definitions in this file.
You might almost be able to derive VersionInfo from the schema then, which would be preferable over having a disjoint schema and type.
If the goal is not to diverge from the existing validation by making it stricter, then you could have e.g. something like:
export const versionInfoBaseSchema = {
version: json.string,
features: json.optional(json.object({})),
overlayVersion: json.optional(json.number),
} as const satisfies json.Schema;
export type VersionInfoBase = json.FromSchema<typeof versionInfoBaseSchema>;
export type VersionInfo = VersionInfoBase & {
// `features` remains optional, but the more specific type takes precedence
// over the `any` type derived by `FromSchema`.
features?: { [name: string]: boolean };
};
Use the included
jsonmodule to improve (make more readable) the validation of the output ofcodeql version.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, Code Quality, ...).Products:
analysis-kinds: code-scanning.analysis-kinds: code-quality.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