chore: make snyk error when failing #1236
Conversation
|
Snyk is now (correctly) failing. We'd need to migrate off |
There was a problem hiding this comment.
Pull request overview
This PR enhances the Snyk test script to properly exit with an error code when vulnerabilities are detected, rather than silently failing. It also includes a new VSIX installation test script and refactors CI workflows to better separate build, test, and deployment stages.
Changes:
- Modified
snyk-test.tsto capture and report errors, exiting with code 1 when Snyk detects vulnerabilities - Added
test-vsix-install.shscript to verify VS Code extension installation across different operating systems - Restructured GitHub Actions workflows to separate build, test, and Snyk checks into distinct jobs with proper dependencies
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/snyk-test.ts | Updated to return error status and exit with failure code when vulnerabilities are found |
| scripts/test-vsix-install.sh | New script implementing cross-platform VSIX installation testing |
| package.json | Added test-install script and updated snyk-test to use .ts extension |
| .github/workflows/test-and-build.yaml | Restructured workflow into separate build-and-check, test, test-install, finalize, and test-snyk jobs |
| .github/workflows/test-and-build-from-fork.yaml | Simplified to use shared setup-node-environment action |
| .github/workflows/actions/test-and-build/action.yaml | Added VSIX install test step |
| .github/workflows/actions/setup-node-environment/action.yaml | New reusable action consolidating Node.js and environment setup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ); | ||
| } catch (err) { | ||
| console.warn(err); | ||
| error = err; |
There was a problem hiding this comment.
The caught error should be cast or validated as an Error type. The catch block receives 'unknown' type in TypeScript, and assigning it directly to 'error: Error | undefined' may cause type issues.
There was a problem hiding this comment.
eh.... doesn't matter terribly much
| "release-draft": "node ./scripts/release-draft.js", | ||
| "reformat": "eslint . --fix && prettier --write .", | ||
| "snyk-test": "node scripts/snyk-test.js", | ||
| "snyk-test": "node scripts/snyk-test.ts", |
There was a problem hiding this comment.
Running a TypeScript file directly with node will fail. This should either use ts-node or reference the compiled JavaScript file (snyk-test.js).
| "snyk-test": "node scripts/snyk-test.ts", | |
| "snyk-test": "ts-node scripts/snyk-test.ts", |
There was a problem hiding this comment.
We have erasable syntax!
gagik
added
the
no-title-validation
| @@ -185,7 +200,6 @@ jobs: | |||
| .sbom/snyk-test-result.json | |||
|
|
|||
| - name: Generate Vulnerability Report (Fail on >= High) | |||
There was a problem hiding this comment.
this is its own job now so it can fail
| continue-on-error: ${{ github.event_name == 'pull_request' }} | ||
| shell: bash | ||
| run: | | ||
| pnpm run generate-vulnerability-report > /dev/null |
There was a problem hiding this comment.
Was this previously sent to /dev/null to avoid logging potentially sensitive info?
There was a problem hiding this comment.
I suppose. I feel like this removes the whole point of the command... I guess the idea is that it's meant to fail above a certain level but the snyk command already fails... so really not sure what's going on there.
Out of abundance of caution I'll leave this to not print anything
2 participants
Stacked on #1235 and includes changes from #1201
It was silently failing before.