GeezerVison

Caching these invocations makes a lot of sense! I have a high level comment and a couple of lower level comments.

The main point is that now that we're caching multiple invocations, it might be a good opportunity to generalise the design. For instance, you could imagine something like:

const versionCache = createPersistedCliCache({ envVar: EnvVar.CODEQL_VERSION_INFO, validate: isVersionInfo });
const resolveLanguagesCache = createPersistedCliCache({ envVar: EnvVar.CODEQL_RESOLVE_LANGUAGES, validate: isResolveLanguagesOutput });

where createPersistedCliCache handles memoising in the Action and persisting between Actions steps with an environment variable.

Some smaller things:

• Ideally the cache entry would also depend on getExtraOptionsFromEnv(["resolve","languages"])
• We should remove the cache in testing-utils.ts like we do for the CodeQL version cache

I agree with @henrymercer's comments regarding a more generalised design for this. I am wondering about the use of environment variables here vs using a file on disk. I don't know if you have already considered this, but we store e.g. the Action configuration on disk as a file. Perhaps that would make sense for these cached CLI results as well.

A general point: could we also make sure to add doc comments for new top-level definitions before merging?

Pull request overview

This PR introduces a cross-step cache for selected CodeQL CLI command outputs (notably codeql version and codeql resolve languages) to reduce repeated JVM startups and improve performance across GitHub Actions steps. It also refactors extractor resolution to derive extractor roots from resolve languages (reusing the cached output) and extends the internal JSON validation helpers to support stronger runtime validation of CLI JSON output.

Changes:

• Add a new 2-tier command-output cache (in-memory + temp-file) and wire it into codeql.ts for version and resolve languages.
• Refactor resolveExtractor() to use resolveLanguages() rather than invoking codeql resolve extractor.
• Extend src/json validation helpers (number/object validators and undefinable) and add unit tests; remove now-obsolete util-based version cache.

Copilot's findings

• Files reviewed: 10/11 changed files
• Comments generated: 3

Thanks for implementing the generic cache mechanism, that's a nice simplification! To make this PR easier to review and get merged, what do you think about splitting this PR into two: the first would implement the generic cache mechanism just for codeql version, and the second would use it to cache codeql resolve languages?

I haven't finished reviewing everything (particularly the new tests as well as codeql.ts), but I already have a bunch of comments that I think should be addressed. So to not end up with too much noise in one go, I am submitting my review without reviewing those last bits.

The main thing to look at is that the on-disk cache is repeatedly written to and read from, which obviously has unnecessary overheads. It would be better if the on-disk cache was only read once (if it exists) to initialise the in-memory cache. It should then only be written to when the Action is about to exit.

Secondly, it would be nice (but is not essential) if we could avoid having the in-memory cache as global state and instead propagate it between the relevant use sites. Since we already have a CodeQL object, we could probably make the command cache part of it. That would make the data flow more explicit and simplify the tests, which then don't have to clear the global cache and can additionally run in parallel.

Thanks for continuing to work on this, and addressing earlier feedback.

Unfortunately, I still don't think this is ready. There are a few things going on which suggest that the changes aren't finished yet, such as that the caching is not used for resolveLanguages and that only the init action stores the on-disk cache.

Further, there continue to be two high-level design points that I am concerned about:

1. The cache is only restored from disk on a per-command basis in getCachedCommandOutput. Restoring all cache entries from the on-disk cache would make sense. This could reasonably be done in either runInActions or the first time that readCommandCacheFile is called and successfully reads the on-disk cache.
2. Cache entries are keyed by the command and CLI path, but not the arguments. This probably doesn't matter for version and maybe not even for resolve languages, but would make sense to avoid potential surprises now or later. To make this change, entries in the cache shouldn't primarily be indexed by CommandCacheKey values, but by keys derived (i.e. hashes computed) from the CLI path, command, arguments, and possibly even other factors such as environment variables.

A concern is also self-hosted runners, where the on-disk cache may persist between workflows. We should make sure that this either doesn't happen (by explicitly deleting the file in e.g. init-post) or storing e.g. the analysis UUID in the file and deleting it if it doesn't match.

I'd also like us to be careful with the new validators you've added. These shouldn't accidentally end up in the path of direct CLI output, because that would make this a high-risk change.

To get on with things and have something that can be merged sooner-than-later, perhaps it would be good to break this down into at least two parts (each as separate PR):

1. An implementation of only "Tier 1" and "Tier 3". I.e. no on-disk cache at all, but addressing my concern about the cache keys.
2. Adding "Tier 2" back in, and addressing my concerns about reading/writing the on-disk cache and potential issues on self-hosted runners.