Repository navigation
fix(scanner): isolate custom cliScannerSource binaries and test oldest supported scanner - #65
Merged
Conversation
…t supported scanner
mateobur
previously approved these changes
Sep 24, 2026
tembleking
enabled auto-merge (squash)
September 24, 2026 15:23
|
The custom-source cache isolation is a clean, well-tested fix. Before merging, please confirm the new scannerCompatibility test does not download a binary at CI time (flaky risk), and that CI and contributors are on Node 22+ given the test-tooling and mocha upgrades now require it. |
Member
Author
|
Thanks!
|
airadier
approved these changes
Oct 7, 2026
airadier
left a comment
Contributor
There was a problem hiding this comment.
LGTM, approving. CI is green on all platforms. Nothing blocking; a few non-blocking follow-ups:
configScanner.test.ts: the new test callsconfiguration.update(...)withoutawaitand never resetscliScannerSourcein the shared test workspace. This can be order-dependent or flaky. Suggest awaiting the updates and resetting toundefinedin afinally, or stubbinggetConfigurationasscannerCompatibility.test.tsdoes.package.json:overrides.mocha ^12.0.2goes beyond what@vscode/test-cli0.0.15 declares (mocha ^11.7.6). Worth confirming this is intentional. test-cli 0.0.15 and c8 11 also now require Node >=22 / >=20, so Node 18 environments can no longer install or run the tests.justfileoldest-cli-scanner: any failed or non-200 HEAD request counts as a missing version, and two consecutive misses end the patch scan. A transient network or CDN error could therefore silently write a wrong oldest version. Consider distinguishing 404 from other errors and retrying. It also picks the first release inside the window rather than the release that was current at the cutoff, so the "oldest supported" guarantee is slightly weaker than documented.scannerCompatibility.test.ts: the suite downloads two real binaries from download.sysdig.com on every run, so it fails offline. Consider gating it behind an env var or a dedicated CI job. The flag check (${flag}\b) can also match prose in the help text; a stricter pattern like(^|\s)${flag}(\s|=|,|$)would be tighter.configScanner.ts: the custom-source cache key hashes only the URL, so a mutable URL (e.g.latest) is never re-downloaded once cached. This is not a regression, since the old key was just SCANNER_VERSION, but it may be worth documenting that the URL should be immutable.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Users can point
cliScannerSourceat any scanner build, including older ones. That path was broken and untested: the cached binary was always named afterSCANNER_VERSION, so a custom binary and the default one overwrote each other. Custom sources are now cached under a name derived from a hash of their URL.A new compatibility test downloads both the newest and the oldest supported scanner (the oldest release inside the 365-day window) through
cliScannerSource. For each one it checks--version, and checks that every flag the VM and IaC runners pass is still accepted. It needs no token, so it runs in the existing CI. Both versions are tagged with markers and bumped byjust update(update-cli-scanner/update-oldest-cli-scanner;update-scanner-versionis kept as an alias), the same approach used in scan-action and the Jenkins plugin. This also bumps the default scanner from 1.27.1 to 1.30.1.just testwas broken before this change. VS Code 1.139 renamed its bundled binary fromElectrontoCode, and@vscode/test-electron2.x still looks forElectron. This PR bumps@vscode/test-electronto 3.1.0 and@vscode/test-clito 0.0.15. With 0.0.15,npm auditflagged themochait pulls in (serialize-javascript,diff) as fixable, which failedjust lint.mochais now forced to 12 throughoverrides, andnpm audit fixtook care of the rest (js-yaml,fast-uri), so the audit is clean.