feat(resolver): add release-age cooldown to protect against supply-chain attacks#1018
feat(resolver): add release-age cooldown to protect against supply-chain attacks#1018ryanpetrello wants to merge 1 commit intopython-wheel-build:mainfrom
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 release-age cooldown that excludes sdist candidates newer than a run-scoped cutoff (bootstrap_time − min_release_age). Exposes CLI flag Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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
release-age-cooldown
branch
from
6e36963 to
bf50e2c
Compare
ryanpetrello
force-pushed
the
release-age-cooldown
branch
from
bf50e2c to
66643e7
Compare
ryanpetrello
force-pushed
the
release-age-cooldown
branch
from
66643e7 to
4efda94
Compare
ryanpetrello
force-pushed
the
release-age-cooldown
branch
from
a634f2f to
8c6aee9
Compare
ryanpetrello
force-pushed
the
release-age-cooldown
branch
from
8c6aee9 to
f1bf70c
Compare
ryanpetrello
force-pushed
the
release-age-cooldown
branch
from
927027b to
97eadc0
Compare
![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 the current code and only fix it if needed.
Inline comments:
In `@e2e/test_bootstrap_cooldown_gitlab.sh`:
- Line 33: The test currently installs the package via pip install
"$SCRIPTDIR/gitlab_override_example" without removing it afterwards; add a
teardown step that uninstalls that package and ensure it always runs by adding a
trap handler (e.g., trap cleanup EXIT) and a cleanup function that runs pip
uninstall -y gitlab_override_example (or uses the same install target/venv if
one is used) so the entry points are removed between tests; update the script to
call this cleanup in normal exit and on failure to avoid cross-test
contamination.
- Around line 65-68: The test currently only checks for
'python_gitlab-5.[1-9]*.whl' and will miss 6.x+ wheels; update the pipeline
after find (the pipeline using find ... | grep -q .) to use an extended regex
that matches any wheel with minor >=1 or any higher major (for example replace
the simple grep with grep -E 'python_gitlab-(5\.[1-9]|[6-9][0-9]*)\..*\.whl'),
ensuring the match covers 5.1+ and all 6.x+ versions so the failing branch (echo
"FAIL: ..."; pass=false) triggers for those wheels.
🪄 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: 93f07c80-a652-486a-a8c9-a63795674b76
📒 Files selected for processing (8)
docs/how-tos/release-age-cooldown.rste2e/ci_bootstrap_suite.she2e/gitlab_override_example/pyproject.tomle2e/gitlab_override_example/src/package_plugins/__init__.pye2e/gitlab_override_example/src/package_plugins/python_gitlab.pye2e/test_bootstrap_cooldown.she2e/test_bootstrap_cooldown_gitlab.she2e/test_bootstrap_cooldown_transitive.sh
✅ Files skipped from review due to trivial changes (1)
- e2e/gitlab_override_example/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/ci_bootstrap_suite.sh
- docs/how-tos/release-age-cooldown.rst
| if find "$OUTDIR/wheels-repo/downloads/" -name 'python_gitlab-5.[1-9]*.whl' | grep -q .; then | ||
| echo "FAIL: a python_gitlab wheel newer than 5.0.0 was selected despite the cooldown" 1>&2 | ||
| pass=false | ||
| fi |
There was a problem hiding this comment.
Broaden the “newer wheel” assertion to catch 6.x+ too.
The current glob only detects 5.1+; a regression selecting 6.x would not fail this test.
Proposed fix
-# Confirm newer versions were rejected by the cooldown.
-if find "$OUTDIR/wheels-repo/downloads/" -name 'python_gitlab-5.[1-9]*.whl' | grep -q .; then
- echo "FAIL: a python_gitlab wheel newer than 5.0.0 was selected despite the cooldown" 1>&2
+# Confirm only the expected fallback version was selected.
+if find "$OUTDIR/wheels-repo/downloads/" -name 'python_gitlab-*.whl' ! -name 'python_gitlab-5.0.0*.whl' | grep -q .; then
+ echo "FAIL: a python_gitlab wheel other than 5.0.0 was selected despite the cooldown" 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_gitlab.sh` around lines 65 - 68, The test
currently only checks for 'python_gitlab-5.[1-9]*.whl' and will miss 6.x+
wheels; update the pipeline after find (the pipeline using find ... | grep -q .)
to use an extended regex that matches any wheel with minor >=1 or any higher
major (for example replace the simple grep with grep -E
'python_gitlab-(5\.[1-9]|[6-9][0-9]*)\..*\.whl'), ensuring the match covers 5.1+
and all 6.x+ versions so the failing branch (echo "FAIL: ..."; pass=false)
triggers for those wheels.
97eadc0 to
7a50176
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_github.sh`:
- Around line 44-45: Update the stale comment that mentions a hardcoded
"9999-day cooldown" to reflect the actual dynamic calculation using MIN_AGE;
locate the comment near the test for GitHub cooldown in
e2e/test_bootstrap_cooldown_github.sh and change wording to indicate the
cooldown is derived from MIN_AGE (computed from the anchor date) and therefore
skipped because GitHub timestamps aren't supported, rather than referencing a
fixed 9999-day value.
- Around line 28-29: Add a cleanup trap to uninstall the plugin installed by the
pip install "$SCRIPTDIR/github_override_example" line: define a small cleanup
function that runs pip uninstall -y "$(basename
"$SCRIPTDIR/github_override_example")" (or pip uninstall -y
github_override_example) and register it with trap 'cleanup' EXIT so the plugin
is removed on script exit; update the area around the pip install invocation to
call the cleanup function and ensure the trap is set immediately after
successful install.
🪄 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: 0e017856-5853-4a8e-a3a3-aa2349d3ee4a
📒 Files selected for processing (12)
docs/how-tos/release-age-cooldown.rste2e/ci_bootstrap_suite.she2e/github_override_example/pyproject.tomle2e/github_override_example/src/package_plugins/__init__.pye2e/github_override_example/src/package_plugins/stevedore_github.pye2e/gitlab_override_example/pyproject.tomle2e/gitlab_override_example/src/package_plugins/__init__.pye2e/gitlab_override_example/src/package_plugins/python_gitlab.pye2e/test_bootstrap_cooldown.she2e/test_bootstrap_cooldown_github.she2e/test_bootstrap_cooldown_gitlab.she2e/test_bootstrap_cooldown_transitive.sh
✅ Files skipped from review due to trivial changes (4)
- e2e/gitlab_override_example/pyproject.toml
- e2e/github_override_example/pyproject.toml
- e2e/gitlab_override_example/src/package_plugins/python_gitlab.py
- docs/how-tos/release-age-cooldown.rst
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/ci_bootstrap_suite.sh
ryanpetrello
force-pushed
the
release-age-cooldown
branch
3 times, most recently
from
ccab88a to
42bdbdb
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
e2e/test_bootstrap_cooldown_github.sh (1)
28-29:⚠️ Potential issue | 🟠 MajorAdd teardown trap for the installed override plugin.
pip install "$SCRIPTDIR/github_override_example"is not cleaned up, so this plugin can leak into later e2e tests in the same runner.As per coding guidelines "e2e/**/*.sh: Check for proper cleanup and teardown (trap handlers)."Proposed fix
# Install the override plugin that routes stevedore through GitHubTagProvider. +trap 'pip uninstall -y github_override_example >/dev/null 2>&1 || true' EXIT pip install "$SCRIPTDIR/github_override_example"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/test_bootstrap_cooldown_github.sh` around lines 28 - 29, The installed override plugin via pip install "$SCRIPTDIR/github_override_example" can leak into other tests; add a teardown trap that uninstalls the package on script exit/failure. Modify the script around the pip install call to register a trap (e.g., trap 'pip uninstall -y github_override_example || true' EXIT) or a named cleanup function that runs pip uninstall -y github_override_example and is invoked on EXIT/ERR, making sure to reference the same package name and SCRIPTDIR variable so the plugin is removed regardless of how the script exits.e2e/test_bootstrap_cooldown_gitlab.sh (2)
32-33:⚠️ Potential issue | 🟠 MajorInstall cleanup is missing; add an EXIT trap.
The override plugin installation is persistent for the process environment and can affect subsequent tests.
As per coding guidelines "e2e/**/*.sh: Check for proper cleanup and teardown (trap handlers)."Proposed fix
# Install the override plugin that routes python-gitlab through GitLabTagProvider. +trap 'python3 -m pip uninstall -y gitlab_override_example >/dev/null 2>&1 || true' EXIT pip install "$SCRIPTDIR/gitlab_override_example"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/test_bootstrap_cooldown_gitlab.sh` around lines 32 - 33, Add a cleanup EXIT trap around the pip install of the override plugin so the installed package is removed when the script exits: define a trap before the pip install that runs pip uninstall -y gitlab_override_example (or pip uninstall -y "$SCRIPTDIR/gitlab_override_example" if that is the package name) and restores any modified environment; locate the pip install "$SCRIPTDIR/gitlab_override_example" line and insert the trap registration (trap '...cleanup command...' EXIT) to guarantee teardown even on failure.
64-66:⚠️ Potential issue | 🟠 MajorBroaden the “newer version selected” check to include 6.x+.
The current glob only catches
5.1+. If resolver regresses to6.x, this test would still pass.Proposed fix
-# Confirm newer versions were rejected by the cooldown. -if find "$OUTDIR/wheels-repo/downloads/" -name 'python_gitlab-5.[1-9]*.whl' | grep -q .; then - echo "FAIL: a python_gitlab wheel newer than 5.0.0 was selected despite the cooldown" 1>&2 +# Confirm only the expected fallback version was selected. +if find "$OUTDIR/wheels-repo/downloads/" -name 'python_gitlab-*.whl' ! -name 'python_gitlab-5.0.0*.whl' | grep -q .; then + echo "FAIL: a python_gitlab wheel other than 5.0.0 was selected despite the cooldown" 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_gitlab.sh` around lines 64 - 66, The find pattern currently checks for 'python_gitlab-5.[1-9]*.whl' and only catches 5.1+, so update the test to detect any wheel with major version >=5 (e.g., 5.x, 6.x, 7.x, etc.) by replacing that glob with a broader match; for example, use find with a regex like the existing find invocation but edited to use -regextype posix-extended -regex '.*/python_gitlab-(5|6|7|8|9)[0-9]*\..*\.whl' (or another equivalent regex that matches major versions >=5) so the check fails if any newer major version beyond 5.0.0 is selected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@e2e/test_bootstrap_cooldown_github.sh`:
- Around line 28-29: The installed override plugin via pip install
"$SCRIPTDIR/github_override_example" can leak into other tests; add a teardown
trap that uninstalls the package on script exit/failure. Modify the script
around the pip install call to register a trap (e.g., trap 'pip uninstall -y
github_override_example || true' EXIT) or a named cleanup function that runs pip
uninstall -y github_override_example and is invoked on EXIT/ERR, making sure to
reference the same package name and SCRIPTDIR variable so the plugin is removed
regardless of how the script exits.
In `@e2e/test_bootstrap_cooldown_gitlab.sh`:
- Around line 32-33: Add a cleanup EXIT trap around the pip install of the
override plugin so the installed package is removed when the script exits:
define a trap before the pip install that runs pip uninstall -y
gitlab_override_example (or pip uninstall -y
"$SCRIPTDIR/gitlab_override_example" if that is the package name) and restores
any modified environment; locate the pip install
"$SCRIPTDIR/gitlab_override_example" line and insert the trap registration (trap
'...cleanup command...' EXIT) to guarantee teardown even on failure.
- Around line 64-66: The find pattern currently checks for
'python_gitlab-5.[1-9]*.whl' and only catches 5.1+, so update the test to detect
any wheel with major version >=5 (e.g., 5.x, 6.x, 7.x, etc.) by replacing that
glob with a broader match; for example, use find with a regex like the existing
find invocation but edited to use -regextype posix-extended -regex
'.*/python_gitlab-(5|6|7|8|9)[0-9]*\..*\.whl' (or another equivalent regex that
matches major versions >=5) so the check fails if any newer major version beyond
5.0.0 is selected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 80d00ed7-c5ea-4d1b-8dd5-309a563b10f1
📒 Files selected for processing (14)
docs/how-tos/release-age-cooldown.rste2e/ci_bootstrap_suite.she2e/cooldown_override_settings/stevedore.yamle2e/github_override_example/pyproject.tomle2e/github_override_example/src/package_plugins/__init__.pye2e/github_override_example/src/package_plugins/stevedore_github.pye2e/gitlab_override_example/pyproject.tomle2e/gitlab_override_example/src/package_plugins/__init__.pye2e/gitlab_override_example/src/package_plugins/python_gitlab.pye2e/test_bootstrap_cooldown.she2e/test_bootstrap_cooldown_github.she2e/test_bootstrap_cooldown_gitlab.she2e/test_bootstrap_cooldown_override.she2e/test_bootstrap_cooldown_transitive.sh
✅ Files skipped from review due to trivial changes (4)
- e2e/cooldown_override_settings/stevedore.yaml
- e2e/github_override_example/pyproject.toml
- e2e/gitlab_override_example/pyproject.toml
- docs/how-tos/release-age-cooldown.rst
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/ci_bootstrap_suite.sh
- e2e/gitlab_override_example/src/package_plugins/python_gitlab.py
ryanpetrello
force-pushed
the
release-age-cooldown
branch
3 times, most recently
from
e0acc7d to
a11daf3
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
e2e/test_bootstrap_cooldown_github.sh (1)
46-47:⚠️ Potential issue | 🟡 MinorUpdate stale “9999-day cooldown” wording.
This comment no longer matches the code path (MIN_AGE is dynamically computed), which can mislead readers during triage.
Proposed fix
-# Resolution must succeed despite the 9999-day cooldown — GitHub timestamps +# Resolution must succeed despite the configured cooldown — GitHub timestamps # are not yet supported, so the cooldown is skipped rather than enforced.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/test_bootstrap_cooldown_github.sh` around lines 46 - 47, The comment about a "9999-day cooldown" is stale because MIN_AGE is computed dynamically; update the comment near the MIN_AGE computation and the cooldown-handling block in the e2e/test_bootstrap_cooldown_github.sh script to describe that MIN_AGE is computed dynamically and that the cooldown is skipped because GitHub timestamps are not supported (rather than hardcoding 9999 days). Locate references to MIN_AGE and the cooldown skip branch and revise the wording to accurately reflect dynamic computation and the reason for skipping enforcement.e2e/test_bootstrap_cooldown_gitlab.sh (1)
67-70:⚠️ Potential issue | 🟠 MajorBroaden the “blocked newer versions” assertion to include 6.x+.
The current pattern only catches
5.1–5.9; a6.x+selection would not fail this test.Proposed fix
-# Confirm newer versions were rejected by the cooldown. -if find "$OUTDIR/wheels-repo/downloads/" -name 'python_gitlab-5.[1-9]*.whl' | grep -q .; then - echo "FAIL: a python_gitlab wheel newer than 5.0.0 was selected despite the cooldown" 1>&2 +# Confirm only the expected fallback version was selected. +if find "$OUTDIR/wheels-repo/downloads/" -name 'python_gitlab-*.whl' ! -name 'python_gitlab-5.0.0*.whl' | grep -q .; then + echo "FAIL: a python_gitlab wheel other than 5.0.0 was selected despite the cooldown" 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_gitlab.sh` around lines 67 - 70, The test currently only looks for wheels matching 'python_gitlab-5.[1-9]*.whl' so a 6.x or higher wheel would slip through; update the conditional that uses find+grep to use extended regex matching any python_gitlab wheel with minor >=1 or any major >5, e.g. replace the grep with grep -E (or grep -Eq) and the pattern 'python_gitlab-(5\.[1-9]|[6-9][0-9]*\.)' so the if-statement that uses find "$OUTDIR/wheels-repo/downloads/" ... | grep -q . is changed to use the extended regex to catch 5.1+ and all 6.x+ wheels.
🧹 Nitpick comments (1)
tests/test_cooldown.py (1)
534-568: Add an assertion for one-time warning emission.This test validates warning content, but not the “one-time warning” behavior. Call
validate_candidate()twice and assert only one matching warning record is emitted.Suggested test hardening
def test_github_cooldown_skips_with_warning( caplog: pytest.LogCaptureFixture, ) -> None: @@ with caplog.at_level(logging.WARNING, logger="fromager.resolver"): - result = provider.validate_candidate( + first = provider.validate_candidate( "test-pkg", requirements, incompatibilities, candidate ) + second = provider.validate_candidate( + "test-pkg", requirements, incompatibilities, candidate + ) - assert result is True + assert first is True + assert second is True assert "cooldown cannot be enforced" in caplog.text assert "not yet implemented" in caplog.text + warning_records = [ + r for r in caplog.records + if r.levelno == logging.WARNING and "cooldown cannot be enforced" in r.message + ] + assert len(warning_records) == 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cooldown.py` around lines 534 - 568, Update test_github_cooldown_skips_with_warning to exercise the one-time warning behavior by calling provider.validate_candidate(...) twice with the same inputs (use the existing provider, candidate, requirements, incompatibilities) and then assert that exactly one warning matching the expected substrings ("cooldown cannot be enforced" and/or "not yet implemented") appears in caplog.records or caplog.text; reference the GitHubTagProvider.validate_candidate call in the test and use caplog to count matching warning records to ensure only a single warning was emitted across both invocations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fromager/packagesettings/_models.py`:
- Around line 106-107: Replace the placeholder Sphinx directive `..
versionadded:: 0.XX` in the module docstring (the `.. versionadded:: 0.XX` line
in src/fromager/packagesettings/_models.py) with the actual release version
string for this change (e.g., 1.2.0) so the documentation metadata is accurate;
ensure the format matches other `versionadded` entries in the repo (no extra
text) and run the docs build or linter to confirm no formatting issues.
In `@src/fromager/resolver.py`:
- Around line 477-484: Add an instance-level latch so the "cooldown cannot be
enforced" warning is logged only once per provider instance: introduce a boolean
attribute on the resolver/provider (e.g., self._cooldown_warning_emitted = False
in the __init__ where self.cooldown is set), then in the code paths that
currently log the warning when candidates lack timestamps (the blocks that emit
"cooldown cannot be enforced" around the candidate/timestamp checks) wrap the
logger call with "if not self._cooldown_warning_emitted: log warning;
self._cooldown_warning_emitted = True". Apply the same check to the other
location mentioned so both places use the same instance flag.
- Around line 594-629: The cooldown logic currently runs for every Candidate;
restrict it to source distributions only by adding a guard at the top of the
block (before checking candidate.upload_time): only enforce the cooldown when
the candidate is an sdist (e.g. check candidate.is_sdist or candidate.filetype
== "sdist" depending on the Candidate API); if the candidate is not an sdist,
skip the cooldown check and allow the candidate (return True / continue as the
surrounding control flow expects). Update the block that references
self.cooldown, candidate.upload_time, and supports_upload_time so the sdist
check is evaluated first.
- Around line 140-167: The function _resolve_package_cooldown currently uses
datetime.datetime.now(datetime.UTC) inside bootstrap_time, causing different
packages resolved later to get different cutoffs; change it to use a single
stable fallback timestamp computed once per run (e.g., a resolver-level or
module-level STABLE_BOOTSTRAP_TIME) instead of calling now() on each invocation.
Modify _resolve_package_cooldown to use that stable timestamp when
global_cooldown is None (or accept a bootstrap_time parameter propagated from
the caller), keeping the existing logic for inheriting bootstrap_time from
global_cooldown and returning Cooldown(min_age=..., bootstrap_time=stable_time).
Ensure the unique symbols to update are _resolve_package_cooldown,
bootstrap_time, global_cooldown, per_package_days, and the Cooldown construction
so all packages in a run share the same cutoff.
---
Duplicate comments:
In `@e2e/test_bootstrap_cooldown_github.sh`:
- Around line 46-47: The comment about a "9999-day cooldown" is stale because
MIN_AGE is computed dynamically; update the comment near the MIN_AGE computation
and the cooldown-handling block in the e2e/test_bootstrap_cooldown_github.sh
script to describe that MIN_AGE is computed dynamically and that the cooldown is
skipped because GitHub timestamps are not supported (rather than hardcoding 9999
days). Locate references to MIN_AGE and the cooldown skip branch and revise the
wording to accurately reflect dynamic computation and the reason for skipping
enforcement.
In `@e2e/test_bootstrap_cooldown_gitlab.sh`:
- Around line 67-70: The test currently only looks for wheels matching
'python_gitlab-5.[1-9]*.whl' so a 6.x or higher wheel would slip through; update
the conditional that uses find+grep to use extended regex matching any
python_gitlab wheel with minor >=1 or any major >5, e.g. replace the grep with
grep -E (or grep -Eq) and the pattern 'python_gitlab-(5\.[1-9]|[6-9][0-9]*\.)'
so the if-statement that uses find "$OUTDIR/wheels-repo/downloads/" ... | grep
-q . is changed to use the extended regex to catch 5.1+ and all 6.x+ wheels.
---
Nitpick comments:
In `@tests/test_cooldown.py`:
- Around line 534-568: Update test_github_cooldown_skips_with_warning to
exercise the one-time warning behavior by calling
provider.validate_candidate(...) twice with the same inputs (use the existing
provider, candidate, requirements, incompatibilities) and then assert that
exactly one warning matching the expected substrings ("cooldown cannot be
enforced" and/or "not yet implemented") appears in caplog.records or
caplog.text; reference the GitHubTagProvider.validate_candidate call in the test
and use caplog to count matching warning records to ensure only a single warning
was emitted across both invocations.
🪄 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: 105e1f96-e6ef-4b4c-b9a4-53891902b741
📒 Files selected for processing (23)
docs/how-tos/index.rstdocs/how-tos/release-age-cooldown.rste2e/ci_bootstrap_suite.she2e/cooldown_override_settings/stevedore.yamle2e/github_override_example/pyproject.tomle2e/github_override_example/src/package_plugins/__init__.pye2e/github_override_example/src/package_plugins/stevedore_github.pye2e/gitlab_override_example/pyproject.tomle2e/gitlab_override_example/src/package_plugins/__init__.pye2e/gitlab_override_example/src/package_plugins/python_gitlab.pye2e/test_bootstrap_cooldown.she2e/test_bootstrap_cooldown_github.she2e/test_bootstrap_cooldown_gitlab.she2e/test_bootstrap_cooldown_override.she2e/test_bootstrap_cooldown_transitive.shsrc/fromager/__main__.pysrc/fromager/context.pysrc/fromager/packagesettings/_models.pysrc/fromager/packagesettings/_pbi.pysrc/fromager/resolver.pytests/test_bootstrap_requirement_resolver.pytests/test_cooldown.pytests/test_packagesettings.py
✅ Files skipped from review due to trivial changes (6)
- e2e/cooldown_override_settings/stevedore.yaml
- docs/how-tos/index.rst
- tests/test_packagesettings.py
- e2e/ci_bootstrap_suite.sh
- e2e/gitlab_override_example/pyproject.toml
- e2e/github_override_example/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (4)
- src/fromager/packagesettings/_pbi.py
- src/fromager/main.py
- e2e/gitlab_override_example/src/package_plugins/python_gitlab.py
- docs/how-tos/release-age-cooldown.rst
| .. versionadded:: 0.XX | ||
| """ |
There was a problem hiding this comment.
Replace placeholder versionadded value before merge.
.. versionadded:: 0.XX is still a placeholder and should be set to the real release version to keep docs/changelog metadata accurate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fromager/packagesettings/_models.py` around lines 106 - 107, Replace the
placeholder Sphinx directive `.. versionadded:: 0.XX` in the module docstring
(the `.. versionadded:: 0.XX` line in src/fromager/packagesettings/_models.py)
with the actual release version string for this change (e.g., 1.2.0) so the
documentation metadata is accurate; ensure the format matches other
`versionadded` entries in the repo (no extra text) and run the docs build or
linter to confirm no formatting issues.
| cooldown: Cooldown | None = None, | ||
| ): | ||
| super().__init__() | ||
| self.constraints = constraints or Constraints() | ||
| self.req_type = req_type | ||
| self.use_cache_candidates = use_resolver_cache | ||
| self.cooldown = cooldown | ||
|
|
There was a problem hiding this comment.
Emit the “cooldown cannot be enforced” warning once per provider instance.
Right now the warning is emitted for every candidate lacking timestamps. That is noisy and doesn’t match the one-time warning contract.
Suggested one-time warning latch
@@
def __init__(
@@
self.use_cache_candidates = use_resolver_cache
self.cooldown = cooldown
+ self._warned_missing_upload_time_support = False
@@
if candidate.upload_time is None:
if not self.supports_upload_time:
@@
- logger.warning(
- "%s: release-age cooldown cannot be enforced — upload "
- "timestamp support is not yet implemented for %s; "
- "cooldown check skipped",
- requirement.name,
- self.get_provider_description(),
- )
+ if not self._warned_missing_upload_time_support:
+ logger.warning(
+ "%s: release-age cooldown cannot be enforced — upload "
+ "timestamp support is not yet implemented for %s; "
+ "cooldown check skipped",
+ requirement.name,
+ self.get_provider_description(),
+ )
+ self._warned_missing_upload_time_support = True
return TrueAlso applies to: 597-607
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fromager/resolver.py` around lines 477 - 484, Add an instance-level latch
so the "cooldown cannot be enforced" warning is logged only once per provider
instance: introduce a boolean attribute on the resolver/provider (e.g.,
self._cooldown_warning_emitted = False in the __init__ where self.cooldown is
set), then in the code paths that currently log the warning when candidates lack
timestamps (the blocks that emit "cooldown cannot be enforced" around the
candidate/timestamp checks) wrap the logger call with "if not
self._cooldown_warning_emitted: log warning; self._cooldown_warning_emitted =
True". Apply the same check to the other location mentioned so both places use
the same instance flag.
There was a problem hiding this comment.
I like this feedback (and think we should incorporate it into this PR).
There was a problem hiding this comment.
@ryanpetrello, glad you agree! Here's the plan to implement the one-time warning latch:
- Add
self._warned_missing_upload_time_support = FalsetoBaseProvider.__init__ - Gate the warning in
is_satisfied_by()with the latch and flip it after logging
🧠 Learnings used
Learnt from: LalatenduMohanty
Repo: python-wheel-build/fromager PR: 1000
File: docs/proposals/release-cooldown.md:63-66
Timestamp: 2026-03-31T20:39:21.688Z
Learning: In the fromager project (python-wheel-build/fromager), the `--min-release-age` CLI option intentionally uses `click.IntRange(min=0)` to reject negative values with an error. Negative values are considered a user mistake and should not be silently coerced to 0 via `clamp=True`. Do not suggest adding `clamp=True` to this option.
- 📌 Commit the changes to this branch
…ain attacks Adds a configurable minimum release age policy that rejects package versions published fewer than N days ago. This protects automated builds from supply-chain attacks where a malicious version is published and immediately pulled in before it can be reviewed. When active, any sdist candidate whose upload-time is more recent than the cutoff is not considered a valid option during constraint resolution. The cutoff is fixed at the start of each run so all resolutions share the same boundary. Key behaviors: - --min-release-age flag (envvar FROMAGER_MIN_RELEASE_AGE) sets the global minimum age in days; 0 (default) disables the check entirely - Enforcement lives in BaseProvider.is_satisfied_by() so it applies to PyPI, GitLab, and any future providers uniformly - Providers that can currently provide release timestamps (PyPI, GitLab) are fail-closed: a candidate with no upload-time metadata is rejected when a cooldown is active; providers without timestamp support yet (GitHub) emit a one-time warning and skip the check instead - resolver_dist.min_release_age in package settings overrides the global flag per-package (None = inherit, 0 = disable, positive int = override) Co-Authored-By: Claude <claude@anthropic.com>
ryanpetrello
force-pushed
the
release-age-cooldown
branch
from
a11daf3 to
f1ee8af
Compare
|
I used AI to do a deep analysis of the PR and I have found a few issues. I list only the most relevant ones : Cooldown leaks into informational commands (Confirmed with manual testing) Problem: default_resolver_provider now reads ctx.cooldown unconditionally. Every call site that goes through overrides.find_and_invoke("get_resolver_provider", resolver.default_resolver_provider, ...) picks up the cooldown. This includes informational commands that are not performing a build: In commands/package.py, list_versions at line ~146 and commands/find_updates.py, _find_newer_versions at line ~195 If a user runs fromager --min-release-age 7 package list-versions some-pkg, recently published versions are silently hidden from the output. Worse, fromager --min-release-age 7 find-updates would not report recently published versions as available updates — defeating the purpose of the command. Suggestion: Either scope cooldown to bootstrap-only commands (by not setting ctx.cooldown for informational commands), or have list_versions/find_updates explicitly create providers without cooldown. Double PBI lookup — design inconsistency Problem: default_resolver_provider() now calls ctx.package_build_info(req) internally to read resolver_min_release_age. But all its callers (sources.get_source_provider, wheels.get_prebuilt_wheel_provider, commands/package.py, etc.) already call ctx.package_build_info(req) themselves to extract the other resolver settings (include_sdists, include_wheels, ignore_platform, sdist_server_url) and pass them as explicit arguments. This breaks the existing pattern where default_resolver_provider is a stateless function of its arguments. Cooldown is the only resolver setting that's read inside the callee instead of being passed by the caller. Suggestion: Either pass the resolved cooldown as an explicit argument (consistent with how all other PBI-derived settings flow), or add a comment acknowledging the deviation. Warning spam for GitHub provider (Confirmed with manual testing) Problem: The PR description says "providers without timestamp support (GitHub) emit a one-time warning." In practice, the warning fires once per candidate, not once per provider instance. A GitHub repo with 50 tags produces 50 identical "release-age cooldown cannot be enforced" warnings. I confirmed this by running a test: 10 candidates through a single GitHubTagProvider with a cooldown active produces 10 identical warnings instead of 1. Root cause: The warning in is_satisfied_by() has no deduplication. It fires every time the method is called for a candidate with upload_time=None on a provider with supports_upload_time=False. Fix: Add a boolean latch on the provider instance. |
|
Besides the above I tested the PR manually and everything else plus the e2e tests seem to be working fine! |
That feels like a feature. It lets someone manually reproduce the list of packages, varying that age number with different calls, to find when a version they expected to see in a build would show up in a future build.
Why not have the resolver look up all of the settings itself? Do any of the callers modify those arguments?
That feels like a bug. |
3 participants
Adds a configurable minimum release age policy that rejects package versions published fewer than N days ago. This protects automated builds from supply-chain attacks where a malicious version is published and immediately pulled in before it can be reviewed.
When active, any sdist candidate whose upload-time is more recent than the cutoff is not considered a valid option during constraint resolution. The cutoff is fixed at the start of each run so all resolutions share the same boundary.
Key behaviors:
related: #877