feat(resolver): add PyPI cooldown policy to support configurable rejection of recently-published sdists#989
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a PyPI "cooldown" policy to Fromager: new CLI flag Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
ryanpetrello
force-pushed
the
pypi-simple-cooldown
branch
2 times, most recently
from
683927a to
3e4feee
Compare
![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)
734-761: Minor: RedundantNonecheck in list comprehension.The
if c.upload_time is not Noneguard at line 755 is unnecessary sincecooldown_blocked(lines 740-744) already filters to only candidates whereupload_time is not None.♻️ Suggested simplification
if cooldown_blocked: oldest_days = min( (self.cooldown.bootstrap_time - c.upload_time).days for c in cooldown_blocked - if c.upload_time is not None )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fromager/resolver.py` around lines 734 - 761, In the cooldown-checking block inside the resolver (where self.cooldown is handled and candidates are gathered via self._find_cached_candidates), remove the redundant guard in the min(...) comprehension that checks "if c.upload_time is not None" because cooldown_blocked is already filtered to only include candidates with upload_time not None; update the computation of oldest_days to directly compute (self.cooldown.bootstrap_time - c.upload_time).days for c in cooldown_blocked to simplify and avoid the unnecessary None check.
🤖 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 734-761: In the cooldown-checking block inside the resolver (where
self.cooldown is handled and candidates are gathered via
self._find_cached_candidates), remove the redundant guard in the min(...)
comprehension that checks "if c.upload_time is not None" because
cooldown_blocked is already filtered to only include candidates with upload_time
not None; update the computation of oldest_days to directly compute
(self.cooldown.bootstrap_time - c.upload_time).days for c in cooldown_blocked to
simplify and avoid the unnecessary None check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99fcf663-540b-47dd-a94d-eac3a8ec7e29
📒 Files selected for processing (8)
docs/how-tos/index.rstdocs/how-tos/pypi-cooldown.rstsrc/fromager/__main__.pysrc/fromager/bootstrapper.pysrc/fromager/context.pysrc/fromager/resolver.pysrc/fromager/wheels.pytests/test_cooldown.py
✅ Files skipped from review due to trivial changes (4)
- docs/how-tos/index.rst
- docs/how-tos/pypi-cooldown.rst
- tests/test_cooldown.py
- src/fromager/context.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/fromager/wheels.py
- src/fromager/bootstrapper.py
ryanpetrello
force-pushed
the
pypi-simple-cooldown
branch
from
3e4feee to
baa72bc
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/fromager/__main__.py (1)
157-177: 🛠️ Refactor suggestion | 🟠 MajorAdd a docstring to
main(public function).Line 157 defines a public function that was modified, but it still has no docstring.
Proposed fix
def main( @@ pypi_min_age: int, ) -> None: + """Configure CLI runtime state and initialize the shared WorkContext.""" # Save the debug flag so invoke_main() can use it.As per coding guidelines, "src/**/*.py: Add docstrings on all public functions and classes".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fromager/__main__.py` around lines 157 - 177, Add a docstring for the public function main describing its purpose and behavior, and document the parameters and return value: briefly state what main does (CLI entrypoint), describe key parameters like ctx (click.Context), verbose/debug, log_file/error_log_file, sdists_repo/wheels_repo, build_wheel_server_url, work_dir, patches_dir, settings_file/settings_dir, constraints_file, cleanup, variant, jobs, network_isolation, and pypi_min_age, plus note that it returns None; keep it concise, use triple-quoted string directly under the def main(...) signature in the src/fromager/__main__.py module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/fromager/__main__.py`:
- Around line 157-177: Add a docstring for the public function main describing
its purpose and behavior, and document the parameters and return value: briefly
state what main does (CLI entrypoint), describe key parameters like ctx
(click.Context), verbose/debug, log_file/error_log_file,
sdists_repo/wheels_repo, build_wheel_server_url, work_dir, patches_dir,
settings_file/settings_dir, constraints_file, cleanup, variant, jobs,
network_isolation, and pypi_min_age, plus note that it returns None; keep it
concise, use triple-quoted string directly under the def main(...) signature in
the src/fromager/__main__.py module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 413b409d-2ef6-4d63-a216-f0744ff6048d
📒 Files selected for processing (8)
docs/how-tos/index.rstdocs/how-tos/pypi-cooldown.rstsrc/fromager/__main__.pysrc/fromager/bootstrapper.pysrc/fromager/context.pysrc/fromager/resolver.pysrc/fromager/wheels.pytests/test_cooldown.py
✅ Files skipped from review due to trivial changes (4)
- docs/how-tos/index.rst
- src/fromager/bootstrapper.py
- docs/how-tos/pypi-cooldown.rst
- tests/test_cooldown.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/fromager/wheels.py
- src/fromager/resolver.py
|
Aside from the unit tests in the PR, here's some details on how I tested this in an end-to-end manner (I used Claude Code to automate and step through several https://pypi.org/project/pecan/has a small, well-understood transitive dependency graph with a natural spread of upload ages and historical pinned constraints, making it easy to straddle cooldown thresholds precisely. Package ages at time of testingExperiment 1: Cooldown boundary testFour separate At 20 days: pecan 1.8.0 (14d) and setuptools 82.0.1 (18d) are within the cooldown window and rejected. The resolver falls back to pecan 1.7.0 and setuptools 82.0.0. All other packages are older than 20 days and resolve to their latest version. At 450 days: every package that has had a release in the past 15 months is forced onto an older version. packaging has no release old enough to satisfy the threshold and drops out of the build entirely. At 9999 days: resolution fails immediately on pecan with an informative error: found 51 candidate(s) for pecan but all are within the 9999-day cooldown window (oldest is 14 day(s) old). The bootstrap exits non-zero without building anything. Experiment 2: Cache interactionBootstrapped with no cooldown to seed a local wheel server, then re-ran bootstrap with With a 30-day cooldown, the primary resolver selects pecan 1.7.0 and setuptools 82.0.0 (their latest-published versions pass the threshold). The cache holds 1.8.0 and 82.0.1 — a different version — so those two packages miss the cache and are rebuilt from source. The remaining six packages resolve to the same version in both the cache and the primary resolver, and are served from cache without being rebuilt. |
ryanpetrello
force-pushed
the
pypi-simple-cooldown
branch
2 times, most recently
from
5841d7a to
684bdf2
Compare
ryanpetrello
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/fromager/resolver.py (1)
736-740: Redundant filter inmin()comprehension.The
if c.upload_time is not Noneguard on line 739 is unnecessary becausecooldown_blocked(line 724-728) already only contains candidates wherec.upload_time is not None.♻️ Proposed simplification
if cooldown_blocked: oldest_days = min( (self.cooldown.bootstrap_time - c.upload_time).days for c in cooldown_blocked - if c.upload_time is not None )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fromager/resolver.py` around lines 736 - 740, The comprehension computing oldest_days contains a redundant filter; remove the "if c.upload_time is not None" clause in the min() generator since cooldown_blocked (constructed earlier) already only includes candidates with c.upload_time set—update the expression that assigns oldest_days (which references self.cooldown.bootstrap_time and cooldown_blocked) to iterate directly over cooldown_blocked without the extra guard so the logic remains identical but simpler.
🤖 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 736-740: The comprehension computing oldest_days contains a
redundant filter; remove the "if c.upload_time is not None" clause in the min()
generator since cooldown_blocked (constructed earlier) already only includes
candidates with c.upload_time set—update the expression that assigns oldest_days
(which references self.cooldown.bootstrap_time and cooldown_blocked) to iterate
directly over cooldown_blocked without the extra guard so the logic remains
identical but simpler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc6276e3-35be-44e7-837a-eb6ab65300b8
📒 Files selected for processing (6)
docs/how-tos/index.rstdocs/how-tos/pypi-cooldown.rstsrc/fromager/__main__.pysrc/fromager/context.pysrc/fromager/resolver.pytests/test_cooldown.py
✅ Files skipped from review due to trivial changes (3)
- docs/how-tos/index.rst
- docs/how-tos/pypi-cooldown.rst
- tests/test_cooldown.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/fromager/main.py
- src/fromager/context.py
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
I analyzed the PR with Claude and I got the following concerns. I do not know how critical these are for the current implementation: Plugin bypass risk: The overrides.find_and_invoke() pattern means a custom get_resolver_provider plugin receives ctx and could read ctx.pypi_cooldown, but is not required to. The invoke() function in overrides.py silently drops kwargs that the override function doesn't accept. If a plugin creates its own PyPIProvider without forwarding the cooldown, it silently bypasses the policy with no warning. The same risk exists if a plugin overrides resolve_source entirely. Recommendation: Add a warning log in resolve() or default_resolver_provider() when a cooldown is configured but the returned provider doesn't have one. Or document this prominently for plugin authors. Upload-time trust: The cooldown relies on upload_time from the PyPI Simple JSON API (PEP 691). This metadata is provided by the index server itself and is not cryptographically signed. A compromised or malicious index could report a fake upload time. However, for PyPI.org specifically, this is reasonable — the timestamp comes from PyPI's own records. |
|
I manually tested the PR and can confirm that it works as expected (as the 1st phase of #877) |
|
@iangelak my take on this feedback:
I think this could be a valid gotcha for plugin authors. A custom That said, does any of our existing tooling extend
As the comments called out, I think this entire trust model we're discussing sort of relies on the notion that the index overall has not been maliciously compromised in such a way that fake upload times could be reported. |
|
Is a granularity of one day okay or should we support strings like "4d", "12h", and "1d6h" ? |
|
@tiran I think that's a great question. Maybe we should support "days" or "hours"? It feels like those are the most likely values you'd want for cooldowns. |
Our plugins follow a pattern of checking for whether we're resolving source or other and returning a custom provider or returning |
There is actually a plugin for a package in our downstream tool which directly constructs PyPIProvider This means the plugin will silently bypass cooldown enforcement when --pypi-min-age is set, because it never passes cooldown=ctx.pypi_cooldown to the provider. |
|
@iangelak okay, that's good feedback. I see few potential paths forward, and this is an area I might need to lean on @python-wheel-build/fromager-maintainers a bit for feedback on the correct approach: A few options, roughly in order of increasing enforcement strength:
When I wrote the original version of this MR, I actually went down path #2, but I realized that putting enforcement in |
|
@ryanpetrello How does the new system handle edge cases for transient dependencies? Example:
Does new system build 1.0, 1.1, or 1.2? |
(assuming we're not running in test mode) I'd expect it to build none of these, and I think that's what we want if the goal is to enforce package cooldowns (even in transitive dependencies).
The example you gave (a top-level dependency that has a transitive dependency that is in the future) isn't a thing I've explicitly tested (it's a little odd - in your example, I have, however, validated that transitive dependencies are subject to the cooldown logic in ~/dev/fromager/testing fromager -v --pypi-min-age=30 bootstrap requests 2>&1 | grep cooldown
2026-03-30 07:43:05,458 DEBUG:fromager.resolver:688: requests: pypi-cooldown: skipping requests 2.33.0 uploaded 4 days, 20:32:23.744346 ago (cooldown: 30 days, 0:00:00)
2026-03-30 07:43:05,736 DEBUG:fromager.resolver:688: setuptools: pypi-cooldown: skipping setuptools 82.0.1 uploaded 20 days, 22:55:48.109174 ago (cooldown: 30 days, 0:00:00)
2026-03-30 07:43:06,739 DEBUG:fromager.resolver:688: setuptools: pypi-cooldown: skipping setuptools 82.0.1 uploaded 20 days, 22:55:48.109174 ago (cooldown: 30 days, 0:00:00)
2026-03-30 07:43:07,615 DEBUG:fromager.resolver:688: charset_normalizer: pypi-cooldown: skipping charset-normalizer 3.4.5 uploaded 24 days, 5:39:45.870766 ago (cooldown: 30 days, 0:00:00)
2026-03-30 07:43:07,615 DEBUG:fromager.resolver:688: charset_normalizer: pypi-cooldown: skipping charset-normalizer 3.4.6 uploaded 14 days, 16:49:39.852183 ago (cooldown: 30 days, 0:00:00)
2026-03-30 07:43:12,971 DEBUG:fromager.resolver:688: setuptools: pypi-cooldown: skipping setuptools 82.0.1 uploaded 20 days, 22:55:48.109174 ago (cooldown: 30 days, 0:00:00)
2026-03-30 07:43:13,007 DEBUG:fromager.resolver:688: setuptools-scm: pypi-cooldown: skipping setuptools-scm 10.0.2 uploaded 4 days, 19:12:37.597960 ago (cooldown: 30 days, 0:00:00)
2026-03-30 07:43:13,007 DEBUG:fromager.resolver:688: setuptools-scm: pypi-cooldown: skipping setuptools-scm 10.0.3 uploaded 3 days, 22:54:38.922040 ago (cooldown: 30 days, 0:00:00)
2026-03-30 07:43:13,007 DEBUG:fromager.resolver:688: setuptools-scm: pypi-cooldown: skipping setuptools-scm 10.0.4 uploaded 2 days, 21:32:25.110897 ago (cooldown: 30 days, 0:00:00)
2026-03-30 07:43:13,007 DEBUG:fromager.resolver:688: setuptools-scm: pypi-cooldown: skipping setuptools-scm 10.0.5 uploaded 2 days, 19:45:59.579162 ago (cooldown: 30 days, 0:00:00)
... |
ryanpetrello
force-pushed
the
pypi-simple-cooldown
branch
from
684bdf2 to
683fad3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_cooldown.py (1)
123-136: Test name doesn't match implementation.The test uses
include_sdists=Falsewhich implicitly disables cooldown (per resolver.py line 637). The name "cooldown_disabled" suggests explicitly testingcooldown=None. Consider either:
- Rename to
test_wheel_only_mode_selects_latest- Or add a separate test with
include_sdists=True, cooldown=Noneto explicitly test the disabled pathSuggested rename
-def test_cooldown_disabled_selects_latest() -> None: - """Without a cooldown the resolver selects the latest version as normal.""" +def test_no_cooldown_selects_latest() -> None: + """Wheel-only resolution (include_sdists=False) selects the latest version.""" with requests_mock.Mocker() as r:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cooldown.py` around lines 123 - 136, The test function test_cooldown_disabled_selects_latest currently sets include_sdists=False (wheel-only mode) which implicitly disables the cooldown; either rename the test to reflect that behavior (e.g., test_wheel_only_mode_selects_latest) or add a new explicit test that constructs resolver.PyPIProvider with include_sdists=True and passes cooldown=None into resolvelib.Resolver (or the appropriate resolver constructor) to assert the resolver still selects version "2.0.0" for Requirement("test-pkg"); update the test name or add the new test function accordingly and keep the same request mocking and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_cooldown.py`:
- Around line 123-136: The test function test_cooldown_disabled_selects_latest
currently sets include_sdists=False (wheel-only mode) which implicitly disables
the cooldown; either rename the test to reflect that behavior (e.g.,
test_wheel_only_mode_selects_latest) or add a new explicit test that constructs
resolver.PyPIProvider with include_sdists=True and passes cooldown=None into
resolvelib.Resolver (or the appropriate resolver constructor) to assert the
resolver still selects version "2.0.0" for Requirement("test-pkg"); update the
test name or add the new test function accordingly and keep the same request
mocking and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9110ea04-1e9d-4a6f-aaff-84e68223f680
📒 Files selected for processing (6)
docs/how-tos/index.rstdocs/how-tos/pypi-cooldown.rstsrc/fromager/__main__.pysrc/fromager/context.pysrc/fromager/resolver.pytests/test_cooldown.py
✅ Files skipped from review due to trivial changes (2)
- docs/how-tos/index.rst
- docs/how-tos/pypi-cooldown.rst
ryanpetrello
force-pushed
the
pypi-simple-cooldown
branch
from
683fad3 to
9509c29
Compare
rd4398
left a comment
rd4398
left a comment
There was a problem hiding this comment.
This looks good to me and a solid first step in rolling this out to all providers. I had a few suggestions / comments
| @click.option( | ||
| "--pypi-min-age", | ||
| type=click.IntRange(min=0), | ||
| default=0, |
There was a problem hiding this comment.
I don't see FROMAGER_PYPI_MIN_AGE environment variable added to click option.
Shouldn't we add something like envvar="FROMAGER_PYPI_MIN_AGE" here or am I missing anything?
There was a problem hiding this comment.
(addressed, and added a test)
| self.sdist_server_url = sdist_server_url | ||
| self.ignore_platform = ignore_platform | ||
| self.override_download_url = override_download_url | ||
|
|
There was a problem hiding this comment.
The cache_key property doesn't include the cooldown configuration. This could cause cached candidates from a non-cooldown run to be reused in a
cooldown run (or vice versa), bypassing the policy. Is this intentional?
Can we at least do one of the below:
- Add it cache_key:
if self.cooldown is not None:
key = f"{key}+cooldown_{self.cooldown.min_age.days}"
- Add a comment explaining why it's intentionally excluded
- Add documentation that cooldown filtering happens during validation, not candidate discovery
There was a problem hiding this comment.
(forgive me if I've got these details wrong - I'm still learning how fromager works internally)
Assuming I understand how discovery + validation actually works, I think the cache_key only needs to vary on things that affect discovery i.e., things which affect what candidates come back from PyPI?
Validation of each candidate is where the cooldown validation currently is implemented in this PR.
It sounds like you might be describing a scenario where two separate fromager invocations - one with a cooldown, one without - somehow share cached candidates. Is that possible?
There was a problem hiding this comment.
also @rd4398 (assuming I'm understanding properly ☝️ )
This is what I have in mind:
diff --git a/src/fromager/resolver.py b/src/fromager/resolver.py
index 57b58d0..a9b65b8 100644
--- a/src/fromager/resolver.py
+++ b/src/fromager/resolver.py
@@ -649,6 +649,11 @@ class PyPIProvider(BaseProvider):
key = f"{key}+{self.override_download_url}"
if self.ignore_platform:
key = f"{key}+ignore_platform"
+ # The cache_key needs to vary on things that affect candidate
+ # discovery (find_candidates). Cooldown filtering is part of validation
+ # (is_satisfied_by) and runs against the cached candidate list, so it
+ # does not need its own cache bucket. See is_satisfied_by() for the
+ # cooldown enforcement logic.
return key| index does not provide that metadata, candidates will be rejected under the | ||
| fail-closed policy described below. | ||
|
|
||
| Explicit version pins (``package==1.2.3``) are subject to the same cooldown as |
There was a problem hiding this comment.
Can we add an example for how to override it? Maybe something like:
If a pinned version is blocked::
$ fromager --pypi-min-age 7 bootstrap package==2.0.0
ERROR: found 1 candidate for package==2.0.0 but all are within the 7-day cooldown window
To override temporarily::
$ fromager --pypi-min-age 0 bootstrap package==2.0.0
There was a problem hiding this comment.
I think it should be covered by the usage examples above?
ryanpetrello
force-pushed
the
pypi-simple-cooldown
branch
from
9509c29 to
a08153d
Compare
ryanpetrello
force-pushed
the
pypi-simple-cooldown
branch
from
a08153d to
8174b5e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
e2e/test_bootstrap_cooldown_constraint_conflict.sh (1)
33-35: Use single quotes in trap to defer variable expansion.The variable
$constraints_fileis expanded when the trap is set. While this works because the variable is assigned first, using single quotes is the idiomatic pattern and avoids subtle bugs if code is reordered.Fix
constraints_file=$(mktemp) -trap "rm -f $constraints_file" EXIT +trap 'rm -f "$constraints_file"' EXIT echo "pbr==7.0.3" > "$constraints_file"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/test_bootstrap_cooldown_constraint_conflict.sh` around lines 33 - 35, The trap currently expands $constraints_file at trap registration time which can be fragile; change the trap command that references constraints_file so the variable is expanded when the trap runs (use single quotes around the trap body or otherwise defer expansion) — update the trap "rm -f $constraints_file" EXIT to a form that defers expansion (so the cleanup of constraints_file happens on EXIT using the runtime value).tests/test_gitutils.py (1)
17-26: Add docstring to helper function.Per coding guidelines, public functions should have docstrings.
Suggested docstring
def setuptools_scm_version(root_dir: pathlib.Path) -> Version: + """Run setuptools_scm in the given directory and return the detected version.""" out = subprocess.check_output(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_gitutils.py` around lines 17 - 26, Add a clear docstring to the helper function setuptools_scm_version that explains its purpose (run setuptools_scm in the given root_dir and return a packaging.version.Version parsed from the last line of subprocess.check_output output), its parameters (root_dir: pathlib.Path) and return type (Version), and note any raised exceptions (e.g., subprocess.CalledProcessError) or side effects (changes to cwd via subprocess). Place the docstring immediately under the def line for setuptools_scm_version so tools and linters recognize it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/test_bootstrap_cooldown_transitive.sh`:
- Around line 71-79: The file contains tests using "[ ! -f
"$OUTDIR/wheels-repo/downloads/stevedore-5.1.0"*.whl ]" and similar for pbr
which misuse -f with globs; replace those checks with a glob-aware existence
check (e.g., use compgen -G "$OUTDIR/wheels-repo/downloads/stevedore-5.1.0*.whl"
>/dev/null || ... or iterate with for ... in ...; do break; done) to correctly
detect whether any matching wheel exists and set pass=false when none are found;
update both the stevedore and pbr checks (the two conditional blocks that echo
"FAIL: ... wheel not found in wheels-repo") to use this approach.
In `@e2e/test_bootstrap_cooldown.sh`:
- Around line 55-58: The shell test currently uses [ ! -f
"$OUTDIR/wheels-repo/downloads/stevedore-5.1.0"*.whl ] which fails because -f
does not expand globs; change the existence check to use glob-aware matching
(e.g., run compgen -G "$OUTDIR/wheels-repo/downloads/stevedore-5.1.0"*.whl
>/dev/null && handle the success case, otherwise set pass=false) so the presence
of any matching wheel file is detected reliably; update the conditional that
references the glob "$OUTDIR/wheels-repo/downloads/stevedore-5.1.0"*.whl
accordingly.
---
Nitpick comments:
In `@e2e/test_bootstrap_cooldown_constraint_conflict.sh`:
- Around line 33-35: The trap currently expands $constraints_file at trap
registration time which can be fragile; change the trap command that references
constraints_file so the variable is expanded when the trap runs (use single
quotes around the trap body or otherwise defer expansion) — update the trap "rm
-f $constraints_file" EXIT to a form that defers expansion (so the cleanup of
constraints_file happens on EXIT using the runtime value).
In `@tests/test_gitutils.py`:
- Around line 17-26: Add a clear docstring to the helper function
setuptools_scm_version that explains its purpose (run setuptools_scm in the
given root_dir and return a packaging.version.Version parsed from the last line
of subprocess.check_output output), its parameters (root_dir: pathlib.Path) and
return type (Version), and note any raised exceptions (e.g.,
subprocess.CalledProcessError) or side effects (changes to cwd via subprocess).
Place the docstring immediately under the def line for setuptools_scm_version so
tools and linters recognize it.
🪄 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: d45e26d6-5927-4512-81b5-1aed8506a1bd
📒 Files selected for processing (34)
.coderabbit.yaml.git-blame-ignore-revs.github/workflows/check.yaml.github/workflows/test.yaml.markdownlint-config.yaml.mdformat.toml.mergify.yml.pre-commit-config.yamlAGENTS.mdCONTRIBUTING.mdREADME.mddocs/develop.mddocs/how-tos/index.rstdocs/how-tos/pypi-cooldown.rstdocs/http-retry.mddocs/reference/files.mddocs/using.mde2e/ci_bootstrap_suite.she2e/flit_core_override/src/package_plugins/flit_core.pye2e/fromager_hooks/src/package_plugins/hooks.pye2e/mergify_lint.pye2e/setup_coverage.pye2e/stevedore_override/src/package_plugins/stevedore.pye2e/test_bootstrap_cooldown.she2e/test_bootstrap_cooldown_constraint_conflict.she2e/test_bootstrap_cooldown_transitive.shpyproject.tomlsrc/fromager/__main__.pysrc/fromager/context.pysrc/fromager/resolver.pytests/conftest.pytests/test_cooldown.pytests/test_gitutils.pytests/test_resolver.py
💤 Files with no reviewable changes (1)
- .markdownlint-config.yaml
✅ Files skipped from review due to trivial changes (16)
- e2e/setup_coverage.py
- README.md
- e2e/stevedore_override/src/package_plugins/stevedore.py
- docs/how-tos/index.rst
- docs/using.md
- .mergify.yml
- e2e/fromager_hooks/src/package_plugins/hooks.py
- e2e/flit_core_override/src/package_plugins/flit_core.py
- .mdformat.toml
- CONTRIBUTING.md
- docs/develop.md
- e2e/ci_bootstrap_suite.sh
- .git-blame-ignore-revs
- AGENTS.md
- .coderabbit.yaml
- docs/how-tos/pypi-cooldown.rst
🚧 Files skipped from review as they are similar to previous changes (1)
- src/fromager/context.py
e2e/test_bootstrap_cooldown.sh
Outdated
| if [ ! -f "$OUTDIR/wheels-repo/downloads/stevedore-5.1.0"*.whl ]; then | ||
| echo "FAIL: stevedore-5.1.0 wheel not found in wheels-repo" 1>&2 | ||
| pass=false | ||
| fi |
There was a problem hiding this comment.
-f doesn't work with globs.
Same issue as the transitive test — use compgen -G for glob matching.
🔧 Proposed fix
-if [ ! -f "$OUTDIR/wheels-repo/downloads/stevedore-5.1.0"*.whl ]; then
+if ! compgen -G "$OUTDIR/wheels-repo/downloads/stevedore-5.1.0"*.whl > /dev/null; then
echo "FAIL: stevedore-5.1.0 wheel not found in wheels-repo" 1>&2
pass=false
fi📝 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.
| if [ ! -f "$OUTDIR/wheels-repo/downloads/stevedore-5.1.0"*.whl ]; then | |
| echo "FAIL: stevedore-5.1.0 wheel not found in wheels-repo" 1>&2 | |
| pass=false | |
| fi | |
| if ! compgen -G "$OUTDIR/wheels-repo/downloads/stevedore-5.1.0"*.whl > /dev/null; then | |
| echo "FAIL: stevedore-5.1.0 wheel not found in wheels-repo" 1>&2 | |
| pass=false | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[error] 55-55: -f doesn't work with globs. Use a for loop.
(SC2144)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/test_bootstrap_cooldown.sh` around lines 55 - 58, The shell test
currently uses [ ! -f "$OUTDIR/wheels-repo/downloads/stevedore-5.1.0"*.whl ]
which fails because -f does not expand globs; change the existence check to use
glob-aware matching (e.g., run compgen -G
"$OUTDIR/wheels-repo/downloads/stevedore-5.1.0"*.whl >/dev/null && handle the
success case, otherwise set pass=false) so the presence of any matching wheel
file is detected reliably; update the conditional that references the glob
"$OUTDIR/wheels-repo/downloads/stevedore-5.1.0"*.whl accordingly.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/test_bootstrap_cooldown_constraint_conflict.sh (1)
33-34: Minor: Use single quotes in trap to expand at signal time.While the current code works (the variable is assigned before the trap), single quotes are idiomatic for traps:
🔧 Proposed fix
constraints_file=$(mktemp) -trap "rm -f $constraints_file" EXIT +trap 'rm -f "$constraints_file"' EXIT🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/test_bootstrap_cooldown_constraint_conflict.sh` around lines 33 - 34, The trap currently uses double quotes which expands $constraints_file at trap definition time; change the trap to use single quotes and quote the variable so it is expanded at signal time (update the trap invocation from trap "rm -f $constraints_file" EXIT to use single quotes and quoted variable so the temporary filename in constraints_file is removed correctly on EXIT).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/test_bootstrap_cooldown_constraint_conflict.sh`:
- Around line 33-34: The trap currently uses double quotes which expands
$constraints_file at trap definition time; change the trap to use single quotes
and quote the variable so it is expanded at signal time (update the trap
invocation from trap "rm -f $constraints_file" EXIT to use single quotes and
quoted variable so the temporary filename in constraints_file is removed
correctly on EXIT).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7bf2f168-608b-4841-a107-3ed91724e44b
📒 Files selected for processing (10)
docs/how-tos/index.rstdocs/how-tos/pypi-cooldown.rste2e/ci_bootstrap_suite.she2e/test_bootstrap_cooldown.she2e/test_bootstrap_cooldown_constraint_conflict.she2e/test_bootstrap_cooldown_transitive.shsrc/fromager/__main__.pysrc/fromager/context.pysrc/fromager/resolver.pytests/test_cooldown.py
✅ Files skipped from review due to trivial changes (4)
- docs/how-tos/index.rst
- e2e/ci_bootstrap_suite.sh
- docs/how-tos/pypi-cooldown.rst
- tests/test_cooldown.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/fromager/main.py
ryanpetrello
force-pushed
the
pypi-simple-cooldown
branch
from
8174b5e to
972e21d
Compare
… versions Adds --pypi-min-age (FROMAGER_PYPI_MIN_AGE) to reject package versions published fewer than N days ago, protecting against supply-chain attacks. Enforcement is automatic for all PyPI resolutions including custom plugins, and fail-closed when upload_time metadata is missing. Co-Authored-By: Claude <claude@anthropic.com> Related: python-wheel-build#877
ryanpetrello
force-pushed
the
pypi-simple-cooldown
branch
from
972e21d to
7698f04
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
e2e/test_bootstrap_cooldown_transitive.sh (1)
71-79:⚠️ Potential issue | 🟡 Minor
-fdoesn't work with globs — usecompgen -G.Same issue as the other cooldown test script. The
[ -f ... ]test with glob patterns doesn't behave as expected.🔧 Proposed fix
-if [ ! -f "$OUTDIR/wheels-repo/downloads/stevedore-5.1.0"*.whl ]; then +if ! compgen -G "$OUTDIR/wheels-repo/downloads/stevedore-5.1.0"*.whl > /dev/null; then echo "FAIL: stevedore-5.1.0 wheel not found in wheels-repo" 1>&2 pass=false fi -if [ ! -f "$OUTDIR/wheels-repo/downloads/pbr-5.11.1"*.whl ]; then +if ! compgen -G "$OUTDIR/wheels-repo/downloads/pbr-5.11.1"*.whl > /dev/null; then echo "FAIL: pbr-5.11.1 wheel not found in wheels-repo" 1>&2 pass=false fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/test_bootstrap_cooldown_transitive.sh` around lines 71 - 79, The file uses [ -f "$OUTDIR/wheels-repo/downloads/stevedore-5.1.0"*.whl ] and similar for pbr which fails with glob patterns; replace each -f glob check with a compgen-based existence check (e.g. matches=$(compgen -G "$OUTDIR/wheels-repo/downloads/stevedore-5.1.0"*.whl); if [ -z "$matches" ]; then echo "FAIL: …" 1>&2; pass=false; fi) and do the same change for the pbr-5.11.1 check so the script correctly detects files when using globs.
🧹 Nitpick comments (1)
e2e/test_bootstrap_cooldown_constraint_conflict.sh (1)
33-35: Use single quotes in trap to defer variable expansion.With double quotes,
$constraints_fileexpands at trap definition time. Single quotes defer expansion until trap execution, which is safer if the variable assignment were to fail.🔧 Proposed fix
constraints_file=$(mktemp) -trap "rm -f $constraints_file" EXIT +trap 'rm -f "$constraints_file"' EXIT echo "pbr==7.0.3" > "$constraints_file"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/test_bootstrap_cooldown_constraint_conflict.sh` around lines 33 - 35, The trap currently uses double quotes causing $constraints_file to expand at trap definition time; change the trap invocation in e2e/test_bootstrap_cooldown_constraint_conflict.sh to use single quotes so the constraint_file variable (constraints_file) is expanded only when the trap runs — update the trap command that references constraints_file accordingly to defer expansion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/test_bootstrap_cooldown.sh`:
- Around line 56-59: The file's file-existence checks use [ -f ... ] with a glob
which doesn't expand; replace those conditionals (e.g., the stevedore wheel
check and the similar envvar-related check around the 79-82 region) to use
compgen -G "pattern" >/dev/null || ... (or test its exit status) so the glob is
expanded properly; update the if/then logic for the functions/conditionals that
reference the patterns (the stevedore wheel check and the envvar wheel-check) to
rely on compgen's success/failure instead of [ -f ... ].
---
Duplicate comments:
In `@e2e/test_bootstrap_cooldown_transitive.sh`:
- Around line 71-79: The file uses [ -f
"$OUTDIR/wheels-repo/downloads/stevedore-5.1.0"*.whl ] and similar for pbr which
fails with glob patterns; replace each -f glob check with a compgen-based
existence check (e.g. matches=$(compgen -G
"$OUTDIR/wheels-repo/downloads/stevedore-5.1.0"*.whl); if [ -z "$matches" ];
then echo "FAIL: …" 1>&2; pass=false; fi) and do the same change for the
pbr-5.11.1 check so the script correctly detects files when using globs.
---
Nitpick comments:
In `@e2e/test_bootstrap_cooldown_constraint_conflict.sh`:
- Around line 33-35: The trap currently uses double quotes causing
$constraints_file to expand at trap definition time; change the trap invocation
in e2e/test_bootstrap_cooldown_constraint_conflict.sh to use single quotes so
the constraint_file variable (constraints_file) is expanded only when the trap
runs — update the trap command that references constraints_file accordingly to
defer expansion.
🪄 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: f3cfe493-6e2a-4e06-aedf-8ee196d57f78
📒 Files selected for processing (10)
docs/how-tos/index.rstdocs/how-tos/pypi-cooldown.rste2e/ci_bootstrap_suite.she2e/test_bootstrap_cooldown.she2e/test_bootstrap_cooldown_constraint_conflict.she2e/test_bootstrap_cooldown_transitive.shsrc/fromager/__main__.pysrc/fromager/context.pysrc/fromager/resolver.pytests/test_cooldown.py
✅ Files skipped from review due to trivial changes (2)
- docs/how-tos/index.rst
- docs/how-tos/pypi-cooldown.rst
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/ci_bootstrap_suite.sh
- src/fromager/resolver.py
e2e/test_bootstrap_cooldown.sh
Outdated
| if [ ! -f "$OUTDIR/wheels-repo/downloads/stevedore-5.1.0"*.whl ]; then | ||
| echo "FAIL (flag): stevedore-5.1.0 wheel not found in wheels-repo" 1>&2 | ||
| pass=false | ||
| fi |
There was a problem hiding this comment.
-f doesn't work with globs — use compgen -G.
The shell [ -f ... ] test doesn't expand globs; it treats the pattern as a literal filename. When the glob matches zero or multiple files, this test fails silently or incorrectly.
🔧 Proposed fix
-if [ ! -f "$OUTDIR/wheels-repo/downloads/stevedore-5.1.0"*.whl ]; then
+if ! compgen -G "$OUTDIR/wheels-repo/downloads/stevedore-5.1.0"*.whl > /dev/null; then
echo "FAIL (flag): stevedore-5.1.0 wheel not found in wheels-repo" 1>&2
pass=false
fiAnd similarly for the envvar check:
-if [ ! -f "$OUTDIR/wheels-repo/downloads/stevedore-5.1.0"*.whl ]; then
+if ! compgen -G "$OUTDIR/wheels-repo/downloads/stevedore-5.1.0"*.whl > /dev/null; then
echo "FAIL (envvar): stevedore-5.1.0 wheel not found in wheels-repo" 1>&2
pass=false
fiAlso applies to: 79-82
🧰 Tools
🪛 Shellcheck (0.11.0)
[error] 56-56: -f doesn't work with globs. Use a for loop.
(SC2144)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/test_bootstrap_cooldown.sh` around lines 56 - 59, The file's
file-existence checks use [ -f ... ] with a glob which doesn't expand; replace
those conditionals (e.g., the stevedore wheel check and the similar
envvar-related check around the 79-82 region) to use compgen -G "pattern"
>/dev/null || ... (or test its exit status) so the glob is expanded properly;
update the if/then logic for the functions/conditionals that reference the
patterns (the stevedore wheel check and the envvar wheel-check) to rely on
compgen's success/failure instead of [ -f ... ].
|
I believe having a global cooldown period and a per package cool-down period is a simple and easier UX and it can be extended further for edge cases. For example the for repositories which uses github and do not have releases we can track individual commits. I sent a proposal #1000 as we discussed. Please take a look. If folks like per provider solution I will be happy to close the proposal. The intent is to understand these options and choose the better solution. |
6 participants
What
Adds a
--pypi-min-ageflag (andFROMAGER_PYPI_MIN_AGEenv var) that rejects PyPI package versions published fewer than N days ago. A value of 0 (the default) disables the check entirely.Why
Partially implements #877
Supply-chain attacks often work by publishing a malicious package version and relying on automated builds picking it up immediately. A configurable minimum age gives collections a window to detect and respond before a bad version is pulled in.