Skip to content

fix: Parse .whl files without file existence check#6689

Open
james-snyk wants to merge 1 commit intomainfrom
fix/whl-check
Open

fix: Parse .whl files without file existence check#6689
james-snyk wants to merge 1 commit intomainfrom
fix/whl-check

Conversation

@james-snyk
Copy link
Copy Markdown
Contributor

Pull Request Submission Checklist

  • Follows CONTRIBUTING guidelines
  • Commit messages
    are release-note ready, emphasizing
    what was changed, not how.
  • Includes detailed description of changes
  • Contains risk assessment (Low | Medium | High)
  • Highlights breaking API changes (if applicable)
  • Links to automated tests covering new functionality
  • Includes manual testing instructions (if necessary)
  • Updates relevant GitBook documentation (PR link: ___)
  • Includes product update to be announced in the next stable release notes

What does this PR do?

Fix issue scanning python projects that ref .whl files when scanning with --all-projects

Where should the reviewer start?

snyk/snyk-python-plugin#291

How should this be manually tested?

Run cli against new test fixture

What's the product update that needs to be communicated to CLI users?

Successful scan python projects when using --all-projects and multiple files ref to .whl packages.

@snyk-io
Copy link
Copy Markdown

snyk-io bot commented Mar 27, 2026

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@james-snyk james-snyk force-pushed the fix/whl-check branch 2 times, most recently from 8372fd3 to 34f4bd9 Compare March 27, 2026 15:48
const baseApi = '/api/v1';
env = {
...process.env,
SNYK_API: 'http://localhost:' + port + baseApi,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: please use getFirstIPv4Address() instead of localhost, using localhost causes the tests to not cover the full stack

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: this is still using localhost. @james-snyk here's an example usage of getFirstIPv4Address().

Copy link
Copy Markdown
Contributor

@PeterSchafer PeterSchafer Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@james-snyk I think this is the last open comment here. Do you want to adapt it? Or is there a reason why not?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this change but at some point accidentally reverted it. Have added it back now

@james-snyk james-snyk marked this pull request as ready for review March 27, 2026 17:01
@james-snyk james-snyk requested review from a team as code owners March 27, 2026 17:01
@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.


// Exit code should be 0 (success) or 2 (missing packages)
// Missing packages is acceptable if pip install failed on the CI system
expect([0, 2]).toContain(code);
Copy link
Copy Markdown
Contributor

@PeterSchafer PeterSchafer Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: why is 0 or 2 okay? shouldn't this behaviour reproducible?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah was being too lenient with the ci but have updated this now as the catch i have for the install should make sure it gets installed correctly

@james-snyk james-snyk force-pushed the fix/whl-check branch 2 times, most recently from 6f05953 to 96b7e73 Compare March 30, 2026 08:40
@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

@james-snyk james-snyk force-pushed the fix/whl-check branch 2 times, most recently from f42f1d0 to 81edaa6 Compare March 30, 2026 10:01
@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

@james-snyk james-snyk force-pushed the fix/whl-check branch 2 times, most recently from cb60432 to 6990742 Compare March 30, 2026 13:17
@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

@james-snyk james-snyk force-pushed the fix/whl-check branch 2 times, most recently from 7816dee to a6b8cec Compare March 31, 2026 08:36
@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.


// Individual scanning should work correctly with .whl files
// No parse errors should occur
expect(stderr).not.toContain('Unparsable requirement line');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: asserting against stderr and stdout (human readable) ,are quite brittle assertions. Better would be to use json output or some other machine readable contract.

);

// Should successfully test both projects
expect(stdout).toContain('project-a');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

@james-snyk james-snyk force-pushed the fix/whl-check branch 3 times, most recently from 28c5106 to 3a4444b Compare April 2, 2026 11:08
@snyk-pr-review-bot

This comment has been minimized.

1 similar comment
@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

@james-snyk james-snyk force-pushed the fix/whl-check branch 2 times, most recently from 0db36e7 to 8e5a54a Compare April 7, 2026 10:17
@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot
Copy link
Copy Markdown

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Unsafe Test Cleanup 🟡 [minor]

The afterAll block calls server.close() without checking if server has been initialized. If an error occurs in beforeAll prior to the assignment of server (e.g., in getServerPort or getFirstIPv4Address), the afterAll hook will throw a TypeError: Cannot read property 'close' of undefined, which can obscure the original error in CI logs.

server.close(() => {
Inconsistent Test Environment 🟡 [minor]

The logic for setting PYTHONPATH to include the locally installed site-packages is restricted to win32. Since Snyk often uses the Python interpreter to resolve dependencies, omitting this on Linux/macOS means the test might run against an incomplete environment where the dependencies installed via pip install --target are not discoverable. This limits the effectiveness of the test on non-Windows platforms.

  testEnv.PYTHONPATH = pythonPaths.join(';');
}
Metadata Regression 🟡 [minor]

The update to snyk-python-plugin version 3.2.1 resulted in the removal of the license field from its entry in package-lock.json. This could affect automated license compliance scanning tools that rely on the lockfile for metadata. Ensure the published package includes the license field in its manifest.

"version": "3.2.1",
"resolved": "https://registry.npmjs.org/snyk-python-plugin/-/snyk-python-plugin-3.2.1.tgz",
"integrity": "sha512-OQhwWTKkWQjdQZFRNsfToeP1hJUDnqTnh7xm0xPUQQqaEx0qnQR+0Kda7YiwtyiWU5ZQbCkvcv2wZ1Y7+q28Mw==",
📚 Repository Context Analyzed

This review considered 6 relevant code sections from 2 files (average relevance: 0.72)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants