fix(resolver): exempt top-level pinned versions from transitive cooldown

[LalatenduMohanty]

Add exempt_versions field to Cooldown so that only the specific version(s) approved via a top-level == pin bypass the cooldown age check when resolved as transitive dependencies. Other versions remain subject to cooldown filtering.

Fixes: #1153 #1155

• PR follows CONTRIBUTING.md guidelines

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR makes Cooldown immutable and adds exempt_versions; resolver gains helpers to compute min_age/bootstrap_time and collect top-level == pinned versions; resolve_package_cooldown always returns a Cooldown populated with exempt_versions; BaseProvider normalizes cooldown to a Cooldown and short-circuits checks for exempt or disabled cooldowns; CLI/context/finder wiring and tests were updated to use the new always-present Cooldown behavior, and tests add extensive pinned/transitive scenarios.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title concisely and accurately describes the main change: adding exempt_versions field to Cooldown to bypass cooldown for top-level pinned versions when resolved transitively.
Description check	✅ Passed	The description clearly relates to the changeset by explaining the exempt_versions field purpose and references the fixed issues.
Linked Issues check	✅ Passed	The implementation fulfills issue #1153 by adding exempt_versions field to Cooldown and updating resolve_package_cooldown() to gather exempt versions from top-level exact pins and bypass cooldown checks for matching versions.
Out of Scope Changes check	✅ Passed	All changes align with the issue objectives: Cooldown becomes frozen, exempt_versions field is added, resolve_package_cooldown() returns Cooldown instances with exemptions, and related callers are updated accordingly.

_{✏️ 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (2)

src/fromager/candidate.py (1)

36-36: ⚡ Quick win

Add Sphinx versionadded directive for the new field.

The exempt_versions field is a user-facing change to a public API. As per coding guidelines, use the versionadded directive to document when this field was introduced.

📝 Suggested documentation addition

     exempt_versions bypasses the age check for specific versions that were
     already approved via a top-level exact pin.
+
+    .. versionadded:: <version>
+       The `exempt_versions` field.
     """

🤖 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/candidate.py` at line 36, The new public field exempt_versions
(dataclass field in Candidate) needs a Sphinx versionadded directive in the
class docstring; update the Candidate class docstring to add a "..
versionadded:: X.Y.Z" entry describing exempt_versions and when it was
introduced (replace X.Y.Z with the release version), e.g., note that
exempt_versions is a frozenset[Version] used to skip certain versions, so
documentation consumers see when this public API appeared.

src/fromager/resolver.py (1)

196-218: ⚡ Quick win

Add Sphinx versionchanged directive for the behavior change.

The function now populates exempt_versions to allow top-level pinned versions to bypass cooldown when encountered as transitive dependencies. This is a significant user-facing behavior change that should be documented.

📝 Suggested documentation addition

     Otherwise a ``Cooldown`` is returned with:
 
     * *min_age* from the per-package override (if set) or the global cooldown.
     * *bootstrap_time* inherited from the global cooldown (for a consistent
       cutoff across the entire run) or the current time.
     * *exempt_versions* populated from top-level exact-pinned entries in the
       dependency graph, so transitive resolutions of the same package honour
       the user's explicit pin.
+
+    .. versionchanged:: <version>
+       Added support for exempting top-level pinned versions from cooldown
+       when encountered as transitive dependencies via the `exempt_versions` field.
     """

🤖 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 196 - 218, Add a Sphinx
"versionchanged" directive to the docstring of resolve_package_cooldown
documenting that it now populates exempt_versions from top-level exact-pinned
entries so transitive dependencies can bypass cooldown; update the docstring
near the explanatory bullet points to include a short versionchanged note
(mentioning the new behavior, version number or "since X.Y" placeholder, and a
one-line rationale) and ensure formatting matches existing Sphinx directives
used elsewhere in the project.

🤖 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.

Nitpick comments:
In `@src/fromager/candidate.py`:
- Line 36: The new public field exempt_versions (dataclass field in Candidate)
needs a Sphinx versionadded directive in the class docstring; update the
Candidate class docstring to add a ".. versionadded:: X.Y.Z" entry describing
exempt_versions and when it was introduced (replace X.Y.Z with the release
version), e.g., note that exempt_versions is a frozenset[Version] used to skip
certain versions, so documentation consumers see when this public API appeared.

In `@src/fromager/resolver.py`:
- Around line 196-218: Add a Sphinx "versionchanged" directive to the docstring
of resolve_package_cooldown documenting that it now populates exempt_versions
from top-level exact-pinned entries so transitive dependencies can bypass
cooldown; update the docstring near the explanatory bullet points to include a
short versionchanged note (mentioning the new behavior, version number or "since
X.Y" placeholder, and a one-line rationale) and ensure formatting matches
existing Sphinx directives used elsewhere in the project.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 08b06f2d-b90a-423b-b7d4-a5b322a32805

📥 Commits

Reviewing files that changed from the base of the PR and between ef797e4 and 400382b.

📒 Files selected for processing (3)

• src/fromager/candidate.py
• src/fromager/resolver.py
• tests/test_cooldown.py

[rd4398]

I have a nit but it looks overall okay. I see Doug has left comments as well, so I will wait for him to take a look again

[rd4398]

Non blocker: I think this is dead code and can be removed. The final else: return None is unreachable: if min_age_override is None and ctx.cooldown is None, we already returned None in the first guard.

[LalatenduMohanty]

Good catch, fixed it in a followup commit.

[dhellmann]

This is still doing lots of conditional checks to try to return None when we're not going to apply a cooldown.

Why not just say that the cooldown period is 0 (or -1), every age of every package will be more than that, and you return a real value. Then everywhere else in the code you don't have to check for None at all.

Why is ctx.cooldown ever None? Why don't we enforce that that thing has a value when the process starts?

[LalatenduMohanty]

Agree the code can be simplified further.

[LalatenduMohanty]

@dhellmann I have added another commit for the refactor. PTAL

[coderabbitai]

🧹 Nitpick comments (2)

src/fromager/__main__.py (1)

286-286: ⚡ Quick win

Use Cooldown.disabled() for the zero-day case.

This is now the only path in the diff that materializes a disabled cooldown via the raw constructor instead of the classmethod. Routing min_release_age == 0 through disabled() keeps a single canonical disabled representation and avoids drifting from any future defaults on the policy object.

♻️ Proposed change

-        cooldown=candidate.Cooldown(min_age=datetime.timedelta(days=min_release_age)),
+        cooldown=(
+            candidate.Cooldown.disabled()
+            if min_release_age == 0
+            else candidate.Cooldown(
+                min_age=datetime.timedelta(days=min_release_age)
+            )
+        ),

🤖 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/__main__.py` at line 286, Replace the direct construction of a
disabled cooldown with the canonical classmethod: when evaluating cooldown in
the code path that uses
candidate.Cooldown(min_age=datetime.timedelta(days=min_release_age)), detect the
zero-day case (min_release_age == 0) and call candidate.Cooldown.disabled()
instead of constructing via the raw constructor; update the logic around
creating cooldown to branch so non-zero min_release_age still uses
candidate.Cooldown(min_age=...) while zero uses candidate.Cooldown.disabled() to
ensure a single canonical disabled representation.

tests/test_cooldown.py (1)

866-872: ⚡ Quick win

Make this cutoff assertion deterministic.

This still races the wall clock by comparing against a fresh now(). Reuse the Cooldown.disabled() instance you assign to tmp_context and assert against its captured bootstrap_time instead.

♻️ Proposed change

-    tmp_context.cooldown = candidate.Cooldown.disabled()
+    disabled = candidate.Cooldown.disabled()
+    tmp_context.cooldown = disabled
     tmp_context.set_max_release_age(30)
     cutoff = resolver._compute_max_age_cutoff(tmp_context)
     assert cutoff is not None
-    expected = datetime.datetime.now(datetime.UTC) - datetime.timedelta(days=30)
-    assert abs((cutoff - expected).total_seconds()) < 2
+    expected = disabled.bootstrap_time - datetime.timedelta(days=30)
+    assert cutoff == expected

🤖 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 - 872, The test races the clock by
comparing cutoff to datetime.now(); instead, obtain the disabled cooldown
instance you assigned to tmp_context (via candidate.Cooldown.disabled()), read
its bootstrap_time, call tmp_context.set_max_release_age(30) and
resolver._compute_max_age_cutoff(tmp_context), then assert the cutoff equals
bootstrap_time - datetime.timedelta(days=30) (or assert the absolute seconds
difference is within a tiny tolerance) so the expectation is deterministic;
reference tmp_context, candidate.Cooldown.disabled(), bootstrap_time,
set_max_release_age, and resolver._compute_max_age_cutoff when making the
change.

🤖 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.

Nitpick comments:
In `@src/fromager/__main__.py`:
- Line 286: Replace the direct construction of a disabled cooldown with the
canonical classmethod: when evaluating cooldown in the code path that uses
candidate.Cooldown(min_age=datetime.timedelta(days=min_release_age)), detect the
zero-day case (min_release_age == 0) and call candidate.Cooldown.disabled()
instead of constructing via the raw constructor; update the logic around
creating cooldown to branch so non-zero min_release_age still uses
candidate.Cooldown(min_age=...) while zero uses candidate.Cooldown.disabled() to
ensure a single canonical disabled representation.

In `@tests/test_cooldown.py`:
- Around line 866-872: The test races the clock by comparing cutoff to
datetime.now(); instead, obtain the disabled cooldown instance you assigned to
tmp_context (via candidate.Cooldown.disabled()), read its bootstrap_time, call
tmp_context.set_max_release_age(30) and
resolver._compute_max_age_cutoff(tmp_context), then assert the cutoff equals
bootstrap_time - datetime.timedelta(days=30) (or assert the absolute seconds
difference is within a tiny tolerance) so the expectation is deterministic;
reference tmp_context, candidate.Cooldown.disabled(), bootstrap_time,
set_max_release_age, and resolver._compute_max_age_cutoff when making the
change.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 84820027-5235-44c9-b942-8e72b6df7fdd

📥 Commits

Reviewing files that changed from the base of the PR and between 400382b and 40bf323.

📒 Files selected for processing (6)

• src/fromager/__main__.py
• src/fromager/candidate.py
• src/fromager/context.py
• src/fromager/resolver.py
• tests/test_cooldown.py
• tests/test_finders.py

🚧 Files skipped from review as they are similar to previous changes (2)

• src/fromager/candidate.py
• src/fromager/resolver.py

[coderabbitai]

Actionable comments posted: 1

🤖 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 720-721: Replace checks that rely on timedelta truthiness for
disabling cooldown with explicit comparisons against zero: wherever code checks
"if not self.cooldown.min_age" (and the other occurrence using the same
truthiness) change it to "if self.cooldown.min_age <= timedelta(0)" (or
equivalent numeric-zero comparison) so negative min_age values are treated as
disabled per the Cooldown contract; update both places referencing
self.cooldown.min_age (the checks in the resolver methods that gate cooldown
logic) to use an explicit <= 0 check and import/construct timedelta(0) if
needed.

🪄 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: 6ade1dae-fdd0-41c6-b5e1-a99e0c6bd79e

📥 Commits

Reviewing files that changed from the base of the PR and between 40bf323 and 59e698b.

📒 Files selected for processing (7)

• src/fromager/__main__.py
• src/fromager/candidate.py
• src/fromager/context.py
• src/fromager/finders.py
• src/fromager/resolver.py
• tests/test_cooldown.py
• tests/test_finders.py

💤 Files with no reviewable changes (1)

• src/fromager/finders.py

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

tests/test_cooldown.py (1)

866-873: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Assert against tmp_context.cooldown.bootstrap_time instead of wall-clock now().

This assertion is time-sensitive and can flake in slower CI, and it doesn’t directly prove _compute_max_age_cutoff uses the cooldown bootstrap timestamp.

Suggested change

 def test_compute_max_age_cutoff_without_cooldown(
     tmp_context: context.WorkContext,
 ) -> None:
     """_compute_max_age_cutoff uses disabled cooldown's bootstrap_time."""
     tmp_context.cooldown = candidate.Cooldown.disabled()
     tmp_context.set_max_release_age(30)
     cutoff = resolver._compute_max_age_cutoff(tmp_context)
-    assert cutoff is not None
-    expected = datetime.datetime.now(datetime.UTC) - datetime.timedelta(days=30)
-    assert abs((cutoff - expected).total_seconds()) < 2
+    assert cutoff is not None
+    expected = tmp_context.cooldown.bootstrap_time - datetime.timedelta(days=30)
+    assert cutoff == expected

As per coding guidelines: "tests/**: Verify test actually tests the intended behavior. Check for missing edge cases."

🤖 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 - 873, Replace the flaky wall-clock
assertion with one that computes the expected cutoff from the cooldown bootstrap
timestamp: use tmp_context.cooldown.bootstrap_time combined with
datetime.timedelta(days=30) (after calling tmp_context.set_max_release_age(30))
to form expected, then compare that to the result of
resolver._compute_max_age_cutoff(tmp_context); this ensures the test asserts
that _compute_max_age_cutoff uses tmp_context.cooldown.bootstrap_time rather
than datetime.now().

🧹 Nitpick comments (1)

tests/test_cooldown.py (1)

354-377: ⚡ Quick win

Normalize helper cooldown defaults to Cooldown.disabled() to match the new contract.

Both helpers default cooldown to None, which weakens coverage of the “cooldown is always a Cooldown object” runtime contract introduced by this stack.

Suggested change

 def _make_ctx(
     tmp_path: pathlib.Path,
     *,
     cooldown: candidate.Cooldown | None = None,
     min_release_age: int | None = None,
 ) -> context.WorkContext:
     """Build a WorkContext with an optional per-package min_release_age setting."""
+    if cooldown is None:
+        cooldown = candidate.Cooldown.disabled()
     settings_dir = tmp_path / "settings"
     settings_dir.mkdir(exist_ok=True)
@@
 def _make_gitlab_provider(
     cooldown: candidate.Cooldown | None = None,
 ) -> resolver.GitLabTagProvider:
+    if cooldown is None:
+        cooldown = candidate.Cooldown.disabled()
     return resolver.GitLabTagProvider(
         project_path="test/pkg",
         server_url="https://gitlab.com",
         matcher=re.compile(r"^v(.*)$"),
         cooldown=cooldown,
         use_resolver_cache=False,
     )

Also applies to: 535-543

🤖 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 354 - 377, The helper currently defaults
the cooldown parameter to None which breaks the new contract that cooldown must
be a Cooldown object; change the default parameter value from None to
candidate.Cooldown.disabled() (and do the same in the other helper at 535-543),
and ensure when instantiating context.WorkContext you pass that Cooldown object
(i.e., no additional None checks required) so WorkContext.cooldown is always a
candidate.Cooldown instance.

🤖 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.

Outside diff comments:
In `@tests/test_cooldown.py`:
- Around line 866-873: Replace the flaky wall-clock assertion with one that
computes the expected cutoff from the cooldown bootstrap timestamp: use
tmp_context.cooldown.bootstrap_time combined with datetime.timedelta(days=30)
(after calling tmp_context.set_max_release_age(30)) to form expected, then
compare that to the result of resolver._compute_max_age_cutoff(tmp_context);
this ensures the test asserts that _compute_max_age_cutoff uses
tmp_context.cooldown.bootstrap_time rather than datetime.now().

---

Nitpick comments:
In `@tests/test_cooldown.py`:
- Around line 354-377: The helper currently defaults the cooldown parameter to
None which breaks the new contract that cooldown must be a Cooldown object;
change the default parameter value from None to candidate.Cooldown.disabled()
(and do the same in the other helper at 535-543), and ensure when instantiating
context.WorkContext you pass that Cooldown object (i.e., no additional None
checks required) so WorkContext.cooldown is always a candidate.Cooldown
instance.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 23ff3087-0412-43be-aafb-fe017250ac22

📥 Commits

Reviewing files that changed from the base of the PR and between 59e698b and 12b8635.

📒 Files selected for processing (7)

• src/fromager/__main__.py
• src/fromager/candidate.py
• src/fromager/context.py
• src/fromager/finders.py
• src/fromager/resolver.py
• tests/test_cooldown.py
• tests/test_finders.py

💤 Files with no reviewable changes (1)

• src/fromager/finders.py