fix: skip marker-constrained uninstalled packages in Python requirements

[ruromero]

Summary

• Fix PEP 508 environment marker handling in Python requirements.txt scanning
• Packages with environment markers (e.g., pywin32==306 ; platform_system == "Windows") that are not installed because the marker didn't match are now silently skipped instead of throwing an error
• Marker-constrained packages that ARE installed continue to be included in the SBOM

Details

Uses tree-sitter marker_spec node detection in #parseRequirements() and checks against the installed packages cache (pip freeze) before processing each requirement. The fix follows the same decision logic as pip itself: if a package has a marker and pip didn't install it, skip it.

Implements TC-4043

Test plan

• New test suite for PEP 508 marker handling using mocked pip freeze/pip show output
• Verifies marker-constrained uninstalled packages are excluded from SBOM
• Verifies marker-constrained installed packages remain in SBOM
• ESLint passes with no errors
• Existing tests unaffected

🤖 Generated with Claude Code

[qodo-code-review]

Review Summary by Qodo

Skip marker-constrained uninstalled packages in Python requirements

🐞 Bug fix 🧪 Tests

Walkthroughs

Description

• Skip PEP 508 marker-constrained uninstalled packages in Python requirements
• Detect environment markers using tree-sitter marker_spec nodes
• Check installed packages cache before processing requirements
• Add comprehensive test suite for marker handling with mocked pip output

Diagram

flowchart LR
  A["requirements.txt with markers"] -->|tree-sitter parse| B["Detect marker_spec nodes"]
  B -->|hasMarker flag| C["Check pip freeze cache"]
  C -->|marker + not installed| D["Skip package"]
  C -->|marker + installed| E["Include in SBOM"]
  C -->|no marker| E

Loading

File Changes

1. src/providers/python_controller.js 🐞 Bug fix +7/-3

Add PEP 508 marker detection and skip logic

• Updated #parseRequirements() return type to include hasMarker boolean field
• Added tree-sitter detection of marker_spec nodes for each requirement
• Implemented skip logic in requirement processing loop to exclude marker-constrained uninstalled
 packages
• Packages with markers that ARE installed continue to be included in SBOM

src/providers/python_controller.js

---

2. test/providers/python_pip.test.js 🧪 Tests +38/-0

Add test suite for marker-constrained package handling

• Added new test suite for PEP 508 marker handling
• Test verifies marker-constrained uninstalled packages are excluded from SBOM
• Test verifies marker-constrained installed packages remain in SBOM
• Uses mocked pip freeze and pip show output via environment variables

test/providers/python_pip.test.js

---

3. test/providers/tst_manifests/pip/pip_requirements_txt_marker_skip/requirements.txt 🧪 Tests +3/-0

Test requirements.txt with PEP 508 markers

• Test manifest with three packages: two without markers and one Windows-only marker
• Includes six==1.16.0 (no marker), certifi==2023.7.22 (Python version marker), and
 pywin32==306 (Windows-only marker)

test/providers/tst_manifests/pip/pip_requirements_txt_marker_skip/requirements.txt

---

View more (1)

4. test/providers/tst_manifests/pip/pip_requirements_txt_marker_skip/expected_component_sbom.json 🧪 Tests +48/-0

Expected SBOM output for marker test case

• Expected SBOM output for marker test case containing only installed packages
• Includes certifi and six components but excludes pywin32 (Windows-only marker)
• Follows CycloneDX 1.4 format with proper component references and dependencies

test/providers/tst_manifests/pip/pip_requirements_txt_marker_skip/expected_component_sbom.json

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2)   📘 Rule violations (0)   📎 Requirement gaps (0)   🎨 UX Issues (0)

🐞\ ≡ Correctness (2)

1. Skips required marker deps 🐞 ≡

Description

In Python_controller#getDependenciesImpl, any requirement with a marker is silently skipped when not
found in the installed-packages cache, without evaluating whether the marker expression is false for
the current environment. This can drop required dependencies (marker evaluates true but package is
missing) and produce an incomplete SBOM instead of failing fast.

Code

src/providers/python_controller.js[R229-231]

+			if(hasMarker && CachedEnvironmentDeps[depName.toLowerCase()] === undefined) {
+				return
+			}

Evidence

The new logic treats hasMarker as equivalent to “not applicable”, but it never evaluates the
marker; it only checks installation presence. The repo’s own test manifest demonstrates a marker
that commonly evaluates true (python_version >= "3"), meaning “has marker” does not imply “optional
for this env”.

src/providers/python_controller.js[98-114]
src/providers/python_controller.js[228-231]
test/providers/tst_manifests/pip/pip_requirements_txt_marker_skip/requirements.txt[1-3]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Marker-constrained requirements are skipped solely based on absence from the installed package cache (`pip show`/`pipdeptree`), but the code never checks whether the marker actually evaluates to `false` for the current environment. This can silently omit dependencies that should apply (marker evaluates `true`) but are missing due to an incomplete/broken environment.

### Issue Context
- `#parseRequirements()` currently returns `{ name, version, hasMarker }`, but not the marker expression.
- `#getDependenciesImpl()` uses `hasMarker && notInstalled => return`.

### Fix Focus Areas
- src/providers/python_controller.js[98-114]
- src/providers/python_controller.js[228-246]

### Suggested fix
1. Extend `#parseRequirements()` to also return the marker text (e.g., locate the `marker_spec` child node and capture its `.text`).
2. When `hasMarker && notInstalled`, evaluate the marker against the current environment and:
  - If marker evaluates **false**: skip (current intended behavior).
  - If marker evaluates **true**: keep current behavior for missing deps (error out via existing flow), so SBOM isn’t silently incomplete.
3. If marker evaluation cannot be performed (e.g., missing evaluator), fail closed (do not skip) or gate skipping behind an explicit option.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Marker skip bypasses canonicalization 🐞 ≡

Description

The new marker skip decision uses depName.toLowerCase() keys, but python_controller does not
canonicalize package names per PEP 503 (collapsing [-_.]+), unlike other Python providers in this
repo. This can cause an installed marker-constrained package to be skipped (treated as “not
installed”) when the manifest name and installed distribution name differ only by canonicalization
(e.g., my.package vs my-package).

Code

src/providers/python_controller.js[R229-231]

+			if(hasMarker && CachedEnvironmentDeps[depName.toLowerCase()] === undefined) {
+				return
+			}

Evidence

python_controller’s cache and lookups use lowercasing plus limited one-off -/_ replacements, but
do not implement the repo’s existing canonicalization strategy used for Python ecosystems (PEP 503
normalization). Because the new behavior turns “not found in cache” into a silent skip for marker
requirements, any canonicalization mismatch can now silently drop dependencies instead of surfacing
as an error.

src/providers/python_controller.js[207-225]
src/providers/python_controller.js[228-231]
src/providers/base_pyproject.js[122-130]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`CachedEnvironmentDeps` keys and requirement lookups do not use consistent PEP 503 canonicalization (collapse `[-_.]+` to `-`). With the new marker-skip early return, a name mismatch can cause an installed, applicable marker dependency to be silently skipped.

### Issue Context
- Other Python providers already canonicalize names via `_canonicalize(name) { return name.toLowerCase().replace(/[-_.]+/g, '-') }`.
- python_controller currently uses `toLowerCase()` plus limited `replace('-', '_')` / `replace('_','-')` variants.

### Fix Focus Areas
- src/providers/python_controller.js[207-225]
- src/providers/python_controller.js[228-250]
- src/providers/base_pyproject.js[122-130]

### Suggested fix
1. Add a local `canonicalize(name)` helper in python_controller matching the repo’s PEP 503 logic (`toLowerCase().replace(/[-_.]+/g, '-')`).
2. Build `CachedEnvironmentDeps` primarily keyed by canonicalized names.
3. Lookup requirements using canonicalized `depName`.
4. (Optional/back-compat) Keep the existing `-`/`_` aliases if needed, but ensure canonical keys are always present and used for the marker skip check.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more

[qodo-code-review]

1. Skips required marker deps 🐞 Bug ≡ Correctness

In Python_controller#getDependenciesImpl, any requirement with a marker is silently skipped when not
found in the installed-packages cache, without evaluating whether the marker expression is false for
the current environment. This can drop required dependencies (marker evaluates true but package is
missing) and produce an incomplete SBOM instead of failing fast.

Agent Prompt

### Issue description
Marker-constrained requirements are skipped solely based on absence from the installed package cache (`pip show`/`pipdeptree`), but the code never checks whether the marker actually evaluates to `false` for the current environment. This can silently omit dependencies that should apply (marker evaluates `true`) but are missing due to an incomplete/broken environment.

### Issue Context
- `#parseRequirements()` currently returns `{ name, version, hasMarker }`, but not the marker expression.
- `#getDependenciesImpl()` uses `hasMarker && notInstalled => return`.

### Fix Focus Areas
- src/providers/python_controller.js[98-114]
- src/providers/python_controller.js[228-246]

### Suggested fix
1. Extend `#parseRequirements()` to also return the marker text (e.g., locate the `marker_spec` child node and capture its `.text`).
2. When `hasMarker && notInstalled`, evaluate the marker against the current environment and:
   - If marker evaluates **false**: skip (current intended behavior).
   - If marker evaluates **true**: keep current behavior for missing deps (error out via existing flow), so SBOM isn’t silently incomplete.
3. If marker evaluation cannot be performed (e.g., missing evaluator), fail closed (do not skip) or gate skipping behind an explicit option.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools