feat(resolver): exempt top-level == pins from release cooldown#1124
Draft
LalatenduMohanty wants to merge 1 commit intopython-wheel-build:mainfrom
Draft
feat(resolver): exempt top-level == pins from release cooldown#1124LalatenduMohanty wants to merge 1 commit intopython-wheel-build:mainfrom
LalatenduMohanty wants to merge 1 commit intopython-wheel-build:mainfrom
Conversation
Closes: python-wheel-build#1123 Co-Authored-By: Claude <claude@anthropic.com> Signed-off-by: Lalatendu Mohanty <lmohanty@redhat.com>
📝 WalkthroughWalkthroughThis PR implements automatic release-age cooldown bypass for top-level requirements using exact version pins ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
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/resolver.py`:
- Around line 140-142: The helper _has_equality_pin currently treats any "=="
specifier as an exact pin; update it to only return True for true exact pins by
checking both spec.operator == "==" and that the spec's version contains no
wildcard characters (e.g., no '*' or patterns like '1.*'). In other words,
inside _has_equality_pin (working with the Requirement and its specifier
members), ensure you inspect spec.version (or str(spec)) and exclude specifiers
where '*' appears (or the version ends with '.*') so wildcard pins like "==1.*"
do not count as exact equality pins.
In `@tests/test_cooldown.py`:
- Around line 866-907: Add a new test that exercises the boundary case where a
top-level wildcard equality like Requirement("test-pkg==1.*") should NOT be
treated as an exact-pin exemption: call resolver.resolve_package_cooldown with
ctx from _make_ctx(tmp_path, cooldown=_COOLDOWN),
req_type=RequirementType.TOP_LEVEL and Requirement("test-pkg==1.*"), and assert
the returned value equals _COOLDOWN; place the test alongside the other
test_resolve_package_cooldown_* functions and name it something like
test_resolve_package_cooldown_toplevel_wildcard_equality_not_exempt to mirror
existing naming.
🪄 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: 44fe1182-608d-4129-aaa2-29b43592632d
📒 Files selected for processing (6)
docs/how-tos/release-age-cooldown.rstdocs/proposals/release-cooldown.mdsrc/fromager/resolver.pysrc/fromager/sources.pysrc/fromager/wheels.pytests/test_cooldown.py
Comment on lines
+140
to
+142
| def _has_equality_pin(req: Requirement) -> bool: | ||
| """Return True if the requirement contains an ``==`` version specifier.""" | ||
| return any(spec.operator == "==" for spec in req.specifier) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that wildcard equality is parsed and currently matches operator "=="
python - <<'PY'
from packaging.requirements import Requirement
r = Requirement("test-pkg==1.*")
for s in r.specifier:
print("operator=", s.operator, "version=", s.version)
PYRepository: python-wheel-build/fromager
Length of output: 97
Exclude wildcard pins from == operator exemption
The function _has_equality_pin() currently exempts any requirement with == operator, including wildcards like ==1.*. This widens the cooldown bypass beyond exact pins and weakens the protection model.
Fix
def _has_equality_pin(req: Requirement) -> bool:
"""Return True if the requirement contains an ``==`` version specifier."""
- return any(spec.operator == "==" for spec in req.specifier)
+ return any(
+ spec.operator == "==" and "*" not in spec.version
+ for spec in req.specifier
+ )🤖 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/resolver.py` around lines 140 - 142, The helper
_has_equality_pin currently treats any "==" specifier as an exact pin; update it
to only return True for true exact pins by checking both spec.operator == "=="
and that the spec's version contains no wildcard characters (e.g., no '*' or
patterns like '1.*'). In other words, inside _has_equality_pin (working with the
Requirement and its specifier members), ensure you inspect spec.version (or
str(spec)) and exclude specifiers where '*' appears (or the version ends with
'.*') so wildcard pins like "==1.*" do not count as exact equality pins.
Comment on lines
+866
to
+907
| def test_resolve_package_cooldown_exempt_toplevel_equality_pin( | ||
| tmp_path: pathlib.Path, | ||
| ) -> None: | ||
| """Top-level == pin bypasses cooldown.""" | ||
| ctx = _make_ctx(tmp_path, cooldown=_COOLDOWN) | ||
| result = resolver.resolve_package_cooldown( | ||
| ctx, Requirement("test-pkg==1.3.2"), req_type=RequirementType.TOP_LEVEL | ||
| ) | ||
| assert result is None | ||
|
|
||
|
|
||
| def test_resolve_package_cooldown_enforced_transitive_equality_pin( | ||
| tmp_path: pathlib.Path, | ||
| ) -> None: | ||
| """Transitive == pin does NOT bypass cooldown.""" | ||
| ctx = _make_ctx(tmp_path, cooldown=_COOLDOWN) | ||
| result = resolver.resolve_package_cooldown( | ||
| ctx, Requirement("test-pkg==1.3.2"), req_type=RequirementType.INSTALL | ||
| ) | ||
| assert result is _COOLDOWN | ||
|
|
||
|
|
||
| def test_resolve_package_cooldown_enforced_toplevel_no_pin( | ||
| tmp_path: pathlib.Path, | ||
| ) -> None: | ||
| """Top-level requirement without == still gets cooldown.""" | ||
| ctx = _make_ctx(tmp_path, cooldown=_COOLDOWN) | ||
| result = resolver.resolve_package_cooldown( | ||
| ctx, Requirement("test-pkg>=1.0"), req_type=RequirementType.TOP_LEVEL | ||
| ) | ||
| assert result is _COOLDOWN | ||
|
|
||
|
|
||
| def test_resolve_package_cooldown_none_req_type_not_exempt( | ||
| tmp_path: pathlib.Path, | ||
| ) -> None: | ||
| """Unknown req_type (None) with == does NOT bypass cooldown.""" | ||
| ctx = _make_ctx(tmp_path, cooldown=_COOLDOWN) | ||
| result = resolver.resolve_package_cooldown( | ||
| ctx, Requirement("test-pkg==1.3.2"), req_type=None | ||
| ) | ||
| assert result is _COOLDOWN |
There was a problem hiding this comment.
Add a boundary test for wildcard equality (==1.*)
These tests cover the core matrix well, but they don’t guard the exact-pin boundary. Add one case asserting top-level test-pkg==1.* does not bypass cooldown.
Suggested test
+def test_resolve_package_cooldown_toplevel_wildcard_equality_not_exempt(
+ tmp_path: pathlib.Path,
+) -> None:
+ """Top-level wildcard equality is not an exact pin, so cooldown remains active."""
+ ctx = _make_ctx(tmp_path, cooldown=_COOLDOWN)
+ result = resolver.resolve_package_cooldown(
+ ctx, Requirement("test-pkg==1.*"), req_type=RequirementType.TOP_LEVEL
+ )
+ assert result is _COOLDOWN🤖 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_cooldown.py` around lines 866 - 907, Add a new test that exercises
the boundary case where a top-level wildcard equality like
Requirement("test-pkg==1.*") should NOT be treated as an exact-pin exemption:
call resolver.resolve_package_cooldown with ctx from _make_ctx(tmp_path,
cooldown=_COOLDOWN), req_type=RequirementType.TOP_LEVEL and
Requirement("test-pkg==1.*"), and assert the returned value equals _COOLDOWN;
place the test alongside the other test_resolve_package_cooldown_* functions and
name it something like
test_resolve_package_cooldown_toplevel_wildcard_equality_not_exempt to mirror
existing naming.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
==version pins now bypass release cooldown enforcement — the operator has explicitly chosen that version==pins remain subject to cooldown to prevent malicious packages from bypassing the check via pinned dependenciesreq_typestill get full cooldown enforcementCloses #1123