feat(resolver): enforce PyPI quarantine check for all resolver types#1102
feat(resolver): enforce PyPI quarantine check for all resolver types#1102pavank63 wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThe changes extract PyPI quarantine status checking (PEP 792) into a standalone Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/fromager/resolver.py (1)
377-397: 💤 Low value
QUARANTINEDfalls through to catch-all, logging misleading "unknown status" warning.If
get_project_from_pypiqueries PyPI directly (not via entry points) or if a custom index mirrors PyPI's quarantine status,QUARANTINEDhitscase _:and logs as "unknown status." Add explicit handling:♻️ Suggested fix
case pypi_simple.ProjectStatus.DEPRECATED | pypi_simple.ProjectStatus.ARCHIVED: logger.warning( "project %r is no longer active: %r: %s", project, package.status, package.status_reason, ) + case pypi_simple.ProjectStatus.QUARANTINED: + # Quarantine is enforced at resolution entry points (resolve/resolve_source). + # If we reach here, the check either passed or was bypassed. + logger.debug( + "project %r has quarantine status in index response (checked at entry point)", + project, + ) case _: logger.warning(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fromager/resolver.py` around lines 377 - 397, The match on package.status in get_project_from_pypi currently falls through QUARANTINED to the catch-all and logs "unknown status"; add an explicit case for pypi_simple.ProjectStatus.QUARANTINED in the match block (near the existing cases for ACTIVE, DEPRECATED, ARCHIVED) and log a clear warning (e.g., "project %r is quarantined: %s") using project and package.status_reason so quarantined packages are not misreported as unknown; keep check_pypi_quarantine_status semantics unchanged at the resolution entry points.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/fromager/resolver.py`:
- Around line 377-397: The match on package.status in get_project_from_pypi
currently falls through QUARANTINED to the catch-all and logs "unknown status";
add an explicit case for pypi_simple.ProjectStatus.QUARANTINED in the match
block (near the existing cases for ACTIVE, DEPRECATED, ARCHIVED) and log a clear
warning (e.g., "project %r is quarantined: %s") using project and
package.status_reason so quarantined packages are not misreported as unknown;
keep check_pypi_quarantine_status semantics unchanged at the resolution entry
points.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 72cf8f67-18ca-4c6c-8848-8044e83d0c4d
📒 Files selected for processing (3)
src/fromager/resolver.pysrc/fromager/sources.pytests/test_resolver.py
|
I see two potential issues with this approach:
|
|
@tiran thanks for flagging these.
This one should be handled already. The except Exception block in
I can think of two options to handle this.
|
|
Added a per-package opt-out: |
|
This pull request has merge conflicts that must be resolved before it can be merged. |
|
There is a similar option |
|
I think they serve different purposes. |
The PEP 792 quarantine check previously only ran inside get_project_from_pypi(), which is only called by PyPIProvider. Packages resolved via GitHubTagProvider, GitLabTagProvider, or PyPIProvider with a custom index URL bypassed the check entirely. Add a standalone check_pypi_quarantine_status() function that queries pypi.org for quarantine status and call it unconditionally from both resolve() and resolve_source() entry points. Remove the quarantine check from get_project_from_pypi() to avoid split responsibility. Signed-off-by: Pavan Kalyan Reddy Cherupally <pcherupa@redhat.com> Co-Authored-By: Claude <claude@anthropic.com>
Packages resolved from non-PyPI sources (GitHub, GitLab) may share a name with an unrelated PyPI project. If that PyPI project gets quarantined, it would incorrectly block the legitimate package. Add `resolver_dist.skip_pypi_quarantine` (default false) to allow maintainers to disable the PEP 792 quarantine check for specific packages. Co-Authored-By: Claude <claude@anthropic.com> Signed-off-by: Pavan Kalyan Reddy Cherupally <pcherupa@redhat.com>
pavank63
force-pushed
the
enforce-quarantine-check-non-pypi-resolvers
branch
from
e5f6d30 to
b092d8e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/fromager/packagesettings/_pbi.py`:
- Around line 264-267: This new public property resolver_skip_pypi_quarantine
needs a Sphinx version marker in its docstring: update the property docstring on
resolver_skip_pypi_quarantine to include a versionadded directive (e.g.
:versionadded: X.Y) right after the one-line description so the new API is
documented properly; ensure you edit the docstring for the
resolver_skip_pypi_quarantine `@property` in the packagesettings/_pbi module.
In `@tests/test_resolver.py`:
- Around line 1341-1364: The test is tautological because it calls the mock
directly instead of exercising the resolver; update
test_check_pypi_quarantine_skipped_with_per_package_setting to call the real
resolver entrypoint (e.g., resolver.resolve or resolver.resolve_source) with the
mocked context/package_build_info and a Requirement("testpkg"), while patching
resolver.check_pypi_quarantine_status to a mock and using requests_mock to seed
the PyPI response; then assert the patched
check_pypi_quarantine_status.assert_not_called() after resolve returns to verify
the resolver respected
mock_ctx.package_build_info(...).resolver_skip_pypi_quarantine. Ensure you
reference the existing mocks (mock_ctx, mock_pbi) and the patched symbol
resolver.check_pypi_quarantine_status so the test exercises production wiring
rather than an inline conditional.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0d20fa96-3815-49d2-b0d6-37fb3affa866
📒 Files selected for processing (6)
src/fromager/packagesettings/_models.pysrc/fromager/packagesettings/_pbi.pysrc/fromager/resolver.pysrc/fromager/sources.pytests/test_packagesettings.pytests/test_resolver.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_packagesettings.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/fromager/sources.py
- src/fromager/resolver.py
| @property | ||
| def resolver_skip_pypi_quarantine(self) -> bool: | ||
| """Skip PyPI quarantine status check for this package?""" | ||
| return self._ps.resolver_dist.skip_pypi_quarantine |
There was a problem hiding this comment.
Add a Sphinx version marker for the new public property.
resolver_skip_pypi_quarantine is a new public API but its docstring lacks a versionadded directive.
Suggested patch
`@property`
def resolver_skip_pypi_quarantine(self) -> bool:
- """Skip PyPI quarantine status check for this package?"""
+ """Skip PyPI quarantine status check for this package?
+
+ .. versionadded:: 0.86
+ """
return self._ps.resolver_dist.skip_pypi_quarantineAs per coding guidelines: "**/*.{rst,py}: Use Sphinx versionadded, versionremoved, versionchanged directives for user-facing changes".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @property | |
| def resolver_skip_pypi_quarantine(self) -> bool: | |
| """Skip PyPI quarantine status check for this package?""" | |
| return self._ps.resolver_dist.skip_pypi_quarantine | |
| `@property` | |
| def resolver_skip_pypi_quarantine(self) -> bool: | |
| """Skip PyPI quarantine status check for this package? | |
| .. versionadded:: 0.86 | |
| """ | |
| return self._ps.resolver_dist.skip_pypi_quarantine |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/fromager/packagesettings/_pbi.py` around lines 264 - 267, This new public
property resolver_skip_pypi_quarantine needs a Sphinx version marker in its
docstring: update the property docstring on resolver_skip_pypi_quarantine to
include a versionadded directive (e.g. :versionadded: X.Y) right after the
one-line description so the new API is documented properly; ensure you edit the
docstring for the resolver_skip_pypi_quarantine `@property` in the
packagesettings/_pbi module.
| def test_check_pypi_quarantine_skipped_with_per_package_setting() -> None: | ||
| """Quarantine check is skipped when skip_pypi_quarantine is True.""" | ||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| mock_ctx = MagicMock() | ||
| mock_pbi = MagicMock() | ||
| mock_pbi.resolver_skip_pypi_quarantine = True | ||
| mock_ctx.package_build_info.return_value = mock_pbi | ||
|
|
||
| req = Requirement("testpkg") | ||
|
|
||
| with ( | ||
| requests_mock.Mocker() as m, | ||
| patch.object(resolver, "check_pypi_quarantine_status") as mock_check, | ||
| ): | ||
| m.get( | ||
| "https://pypi.org/simple/testpkg/", | ||
| text=_quarantined_simple_response, | ||
| ) | ||
| skip = mock_ctx.package_build_info(req).resolver_skip_pypi_quarantine | ||
| if not skip: | ||
| resolver.check_pypi_quarantine_status(req.name) | ||
|
|
||
| mock_check.assert_not_called() |
There was a problem hiding this comment.
This test is tautological and doesn’t validate resolver wiring.
The current test manually checks skip and conditionally calls the mocked function, so assert_not_called() can pass even if production resolve() / resolve_source() ignores resolver_skip_pypi_quarantine.
Please invoke the real resolver entrypoint and assert check_pypi_quarantine_status was skipped there.
As per coding guidelines: "tests/**: Verify test actually tests the intended behavior."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_resolver.py` around lines 1341 - 1364, The test is tautological
because it calls the mock directly instead of exercising the resolver; update
test_check_pypi_quarantine_skipped_with_per_package_setting to call the real
resolver entrypoint (e.g., resolver.resolve or resolver.resolve_source) with the
mocked context/package_build_info and a Requirement("testpkg"), while patching
resolver.check_pypi_quarantine_status to a mock and using requests_mock to seed
the PyPI response; then assert the patched
check_pypi_quarantine_status.assert_not_called() after resolve returns to verify
the resolver respected
mock_ctx.package_build_info(...).resolver_skip_pypi_quarantine. Ensure you
reference the existing mocks (mock_ctx, mock_pbi) and the patched symbol
resolver.check_pypi_quarantine_status so the test exercises production wiring
rather than an inline conditional.
2 participants
The PEP 792 quarantine check previously only ran inside get_project_from_pypi(), which is only called by PyPIProvider. Packages resolved via GitHubTagProvider, GitLabTagProvider, or PyPIProvider with a custom index URL bypassed the check entirely.
Add a standalone check_pypi_quarantine_status() function that queries pypi.org for quarantine status and call it unconditionally from both resolve() and resolve_source() entry points. Remove the quarantine check from get_project_from_pypi() to avoid split responsibility.
Closes: #1101