ci: check dependency licenses#825
Conversation
Greptile SummaryThis PR adds a reproducible CI gate that validates every locked Python runtime dependency against an explicit Apache-2.0 compatibility policy across Python 3.10–3.14. All previous review findings have been addressed:
|
| Filename | Overview |
|---|---|
| scripts/check_dependency_licenses.py | Core scanner logic: SPDX expression parser, MIT text recognizer, semicolon-separated alias normalization, and policy evaluation. Logic is correct and well-exercised by the test suite. |
| dependency-license-policy.toml | Policy allowlist and 8 exact-version/license exceptions; no email-validator entry and email-validator is absent from uv.lock, so no stale-exception concern. |
| tests_e2e/test_dependency_license_check.py | 12 unit tests cover permissive/composite licenses, AND/OR semantics, WITH-exception fail-closed, empty semicolons, bundled MIT text, and stale exceptions; coverage is thorough. |
| .github/workflows/ci.yml | Adds dependency-licenses matrix job (Python 3.10-3.14) pinned at commit SHAs with fail-fast: false; correctly passes LICENSE_PYTHON_VERSION from the matrix. |
| pyproject.toml | Adds license-check optional group with tomli conditional on Python < 3.11; correctly structured so --no-dev --group license-check includes tomli only when needed. |
| Makefile | New check-dependency-licenses target uses --isolated --no-dev --group license-check --locked to build an accurate production slice before invoking the script. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[make check-dependency-licenses] --> B[uv run --isolated --no-dev --group license-check --locked]
B --> C[check_dependency_licenses.py]
C --> D[uv tool run pip-licenses --python sys.executable]
D --> E[JSON report: name, version, license, license_text]
E --> F[evaluate_report]
F --> G{package in exceptions?}
G -- yes --> H{exact version+license match?}
H -- yes --> I[reviewed]
H -- no --> J[exception mismatch violation]
G -- no --> K[normalized_license_alternatives]
K --> L{license == UNKNOWN?}
L -- yes --> M{looks_like_mit_license text?}
M -- yes --> N[frozenset MIT]
M -- no --> O[unknown violation]
L -- no --> P[alias lookup / SPDX parser]
P --> Q{any alternative subset of allowed_licenses?}
Q -- yes --> R[passes]
Q -- no --> S[disallowed violation]
F --> T{stale exceptions?}
T -- yes --> U[stale exception violation]
T -- no --> V[all clear]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[make check-dependency-licenses] --> B[uv run --isolated --no-dev --group license-check --locked]
B --> C[check_dependency_licenses.py]
C --> D[uv tool run pip-licenses --python sys.executable]
D --> E[JSON report: name, version, license, license_text]
E --> F[evaluate_report]
F --> G{package in exceptions?}
G -- yes --> H{exact version+license match?}
H -- yes --> I[reviewed]
H -- no --> J[exception mismatch violation]
G -- no --> K[normalized_license_alternatives]
K --> L{license == UNKNOWN?}
L -- yes --> M{looks_like_mit_license text?}
M -- yes --> N[frozenset MIT]
M -- no --> O[unknown violation]
L -- no --> P[alias lookup / SPDX parser]
P --> Q{any alternative subset of allowed_licenses?}
Q -- yes --> R[passes]
Q -- no --> S[disallowed violation]
F --> T{stale exceptions?}
T -- yes --> U[stale exception violation]
T -- no --> V[all clear]
Reviews (4): Last reviewed commit: "fix: declare license checker fallback" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| tokens = [token.strip() for token in EXPRESSION_OPERATOR.split(stripped) if token.strip()] | ||
| if not tokens: | ||
| return frozenset() | ||
| identifiers.update(tokens) |
There was a problem hiding this comment.
stripped is empty (e.g. from a trailing semicolon such as "Apache Software License; " or consecutive semicolons), tokens is [] and the function immediately returns frozenset(), throwing away any identifiers accumulated in earlier loop iterations. The package is then flagged as "unknown or unrecognized" even though at least one valid segment was already parsed. The correct fix is to skip empty segments with continue rather than exiting the loop early.
| tokens = [token.strip() for token in EXPRESSION_OPERATOR.split(stripped) if token.strip()] | |
| if not tokens: | |
| return frozenset() | |
| identifiers.update(tokens) | |
| tokens = [token.strip() for token in EXPRESSION_OPERATOR.split(stripped) if token.strip()] | |
| if not tokens: | |
| continue | |
| identifiers.update(tokens) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/check_dependency_licenses.py
Line: 90-93
Comment:
Premature early return on empty semicolon segment discards already-collected identifiers. When `stripped` is empty (e.g. from a trailing semicolon such as `"Apache Software License; "` or consecutive semicolons), `tokens` is `[]` and the function immediately returns `frozenset()`, throwing away any identifiers accumulated in earlier loop iterations. The package is then flagged as "unknown or unrecognized" even though at least one valid segment was already parsed. The correct fix is to skip empty segments with `continue` rather than exiting the loop early.
```suggestion
tokens = [token.strip() for token in EXPRESSION_OPERATOR.split(stripped) if token.strip()]
if not tokens:
continue
identifiers.update(tokens)
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in 42748e4. Empty semicolon-separated segments are now skipped without discarding valid identifiers, and test_evaluate_report_ignores_empty_semicolon_segments covers trailing and consecutive delimiters. The focused suite and full CI pass.
| [exceptions.email-validator] | ||
| license = "The Unlicense (Unlicense)" | ||
| reason = "Existing unmodified validation library installed as a separate runtime dependency." |
There was a problem hiding this comment.
email-validator exception is for a package that PR #824 removes. The stale-exception check added in this very PR will then flag it as "email-validator: stale exception; package is not present in the scanned environment" and fail the dependency-licenses job. The PR description already calls this out, but the exception must be removed from this file before (or at the same time as) merge whenever #824 lands first.
Prompt To Fix With AI
This is a comment left during a code review.
Path: dependency-license-policy.toml
Line: 27-29
Comment:
**Stale exception will break CI if PR #824 merges first.** The `email-validator` exception is for a package that PR #824 removes. The stale-exception check added in this very PR will then flag it as `"email-validator: stale exception; package is not present in the scanned environment"` and fail the `dependency-licenses` job. The PR description already calls this out, but the exception must be removed from this file before (or at the same time as) merge whenever #824 lands first.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Agreed. PR #824 remains open, so the current runtime closure still contains email-validator and this exception is currently required. The resolution is to merge #825 before #824; if the order changes, this exception will be removed from #825 before merge. The dependency is also called out in the PR description.
Code Review: PR #825 — ci: check dependency licensesSummaryThis PR adds a reproducible CI check that validates the locked runtime dependency
The design is clean and well-factored: pure Findings1. (High)
|
|
Thanks for putting this together, @nabinchha — the package-scoped policy and regression coverage are a solid foundation. SummaryThis adds a repository-owned dependency-license policy, a pinned FindingsCritical — Let's fix these before merge
Warnings — Worth addressing
What Looks Good
VerdictNeeds changes. Before merge, cover the Python 3.10 lock slice, preserve and validate This review was generated by an AI assistant. |
Add a locked runtime dependency license scan with an explicit permissive allowlist and package-specific reviewed exceptions. Fail on unknown licenses, exception drift, and stale exceptions. Closes #822 Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Preserve SPDX OR, AND, and WITH semantics during policy evaluation. Cover malformed and delimited metadata, recognize the alternate MIT heading, and declare the Python 3.10 TOML fallback. Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Scan isolated runtime environments across every supported Python version. Require exact version and license exception reports, preserve SPDX WITH terms, and accept only complete canonical MIT text. Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
| "pytest-env>=1.2.0,<2", | ||
| "pytest-httpx>=0.36.0,<1", | ||
| "ruff>=0.14.10,<1", | ||
| "tomli>=2.0.1,<3; python_version < '3.11'", |
There was a problem hiding this comment.
tomli placed in dev group causes ModuleNotFoundError on Python 3.10 CI run
The check-dependency-licenses make target runs uv run --all-packages --no-dev --locked. Because --no-dev excludes the dev dependency group, tomli is not installed in that environment. On Python 3.10 the script reaches import tomli as tomllib (line 18 of scripts/check_dependency_licenses.py) and immediately crashes, making the dependency-licenses CI job always fail for the 3.10 matrix slot.
A fix is to move tomli out of the dev group — either into a dedicated non-dev group (e.g. scripts) and add --group scripts to the make target, or into the root project's dependencies with the same python_version < '3.11' marker.
Prompt To Fix With AI
This is a comment left during a code review.
Path: pyproject.toml
Line: 45
Comment:
`tomli` placed in `dev` group causes `ModuleNotFoundError` on Python 3.10 CI run
The `check-dependency-licenses` make target runs `uv run --all-packages --no-dev --locked`. Because `--no-dev` excludes the `dev` dependency group, `tomli` is not installed in that environment. On Python 3.10 the script reaches `import tomli as tomllib` (line 18 of `scripts/check_dependency_licenses.py`) and immediately crashes, making the `dependency-licenses` CI job always fail for the `3.10` matrix slot.
A fix is to move `tomli` out of the `dev` group — either into a dedicated non-dev group (e.g. `scripts`) and add `--group scripts` to the make target, or into the root project's `dependencies` with the same `python_version < '3.11'` marker.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in 322f72a. tomli is now declared in a dedicated license-check dependency group, and the isolated Make target explicitly requests that group while retaining --no-dev. The Python 3.10 locked license scan passes (90 packages checked), along with 12 focused tests and make check-all.
Remove the reviewed exception after PR #824 dropped email-validator from the locked runtime dependency graph. Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
d9a1cb8 to
b048ee4
Compare
Move the Python 3.10 tomli fallback into a dedicated dependency group and request it explicitly from the isolated license-check target. Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
|
@johnnygreco Thanks for the detailed review. All four findings are addressed in
PR #824 has also merged. This branch is rebased onto it, and the stale Verification: 12 focused tests pass, |
johnnygreco
left a comment
johnnygreco
left a comment
There was a problem hiding this comment.
@nabinchha Thanks for addressing this so thoroughly. I independently verified the updated head: all four findings are resolved, the 12 focused tests and full formatting/lint checks pass, the lock is current, and isolated locked license scans pass on Python 3.10–3.14. No remaining concerns from my review. Approved.
2 participants
📋 Summary
Add a reproducible CI gate that validates every supported Python locked runtime dependency slice against an explicit Apache-2.0 compatibility policy. Unknown or unapproved licenses, changed exact exception reports, stale exceptions, and unreviewed SPDX exception terms fail closed.
🔗 Related Issue
Closes #822
🔄 Changes
🧪 Testing
make check-alluv lock --check✅ Checklist
Description updated with AI