Fix existing bug and add test coverage

This commit attempts to fix the existing bug
#956

and add test coverage for the refactor as sugegsted in the
MR

Signed-off-by: Rohan Devasthale <rdevasth@redhat.com>

Author: rd4398

src/fromager/bootstrapper.py

@@ -189,15 +189,18 @@ def resolve_version(
                 )
 
             # Check cache first to avoid re-resolving
-            cached_result = self._resolver.get_cached_resolution(req)
+            # Git URLs are always source (not prebuilt)
+            cached_result = self._resolver.get_cached_resolution(req, pre_built=False)
             if cached_result is not None:
                 logger.debug(f"resolved {req} from cache")
                 return cached_result
 
             logger.info("resolving source via URL, ignoring any plugins")
             source_url, resolved_version = self._resolve_version_from_git_url(req=req)
-            # Cache the git URL resolution
-            self._resolver.cache_resolution(req, (source_url, resolved_version))
+            # Cache the git URL resolution (always source, not prebuilt)
+            self._resolver.cache_resolution(
+                req, pre_built=False, result=(source_url, resolved_version)
+            )
             return source_url, resolved_version
 
         # Delegate to RequirementResolver

src/fromager/requirement_resolver.py

@@ -50,7 +50,8 @@ def __init__(
         self.ctx = ctx
         self.prev_graph = prev_graph
         # Session-level resolution cache to avoid re-resolving same requirements
-        self._resolved_requirements: dict[str, tuple[str, Version]] = {}
+        # Key: (requirement_string, pre_built) to distinguish source vs prebuilt
+        self._resolved_requirements: dict[tuple[str, bool], tuple[str, Version]] = {}
 
     def resolve(
         self,
@@ -67,7 +68,7 @@ def resolve(
         3. PyPI resolution (source or prebuilt based on package build info)
 
         Args:
-            req: Package requirement (must NOT have URL)
+            req: Package requirement
             req_type: Type of requirement
             parent_req: Parent requirement from dependency chain
             pre_built: Optional override to force prebuilt (True) or source (False).
@@ -77,15 +78,24 @@ def resolve(
             Tuple of (url, resolved_version)
 
         Raises:
-            ValueError: If req contains a URL (must use Bootstrapper for git URLs)
+            ValueError: If req contains a git URL and pre_built is False
+                (git URL source resolution must be handled by Bootstrapper)
         """
-        if req.url:
+        # Determine pre_built if not specified (needed for cache key and URL guard)
+        if pre_built is None:
+            pbi = self.ctx.package_build_info(req)
+            pre_built = pbi.pre_built
+
+        # Git URL source resolution must be handled by Bootstrapper.
+        # But git URL prebuilt resolution is allowed - we look for wheels on PyPI
+        # (test mode fallback uses this path).
+        if req.url and not pre_built:
             raise ValueError(
                 f"Git URL requirements must be handled by Bootstrapper: {req}"
             )
 
-        # Check session cache first
-        cached_result = self.get_cached_resolution(req)
+        # Check session cache (keyed by requirement + pre_built)
+        cached_result = self.get_cached_resolution(req, pre_built)
         if cached_result is not None:
             logger.debug(f"resolved {req} from cache")
             return cached_result
@@ -95,15 +105,15 @@ def resolve(
 
         # Cache the result
         result = (url, resolved_version)
-        self.cache_resolution(req, result)
+        self.cache_resolution(req, pre_built, result)
         return url, resolved_version
 
     def _resolve(
         self,
         req: Requirement,
         req_type: RequirementType,
         parent_req: Requirement | None,
-        pre_built: bool | None,
+        pre_built: bool,
     ) -> tuple[str, Version]:
         """Internal resolution logic without caching.
 
@@ -115,17 +125,11 @@ def _resolve(
             req: Package requirement
             req_type: Type of requirement
             parent_req: Parent requirement from dependency chain
-            pre_built: Optional override to force prebuilt (True) or source (False).
-                If None, uses package build info to determine.
+            pre_built: Whether to resolve prebuilt (True) or source (False)
 
         Returns:
             Tuple of (url, resolved_version)
         """
-        # Determine whether to resolve prebuilt or source
-        if pre_built is None:
-            pbi = self.ctx.package_build_info(req)
-            pre_built = pbi.pre_built
-
         # Try graph
         cached_resolution = self._resolve_from_graph(
             req=req,
@@ -162,20 +166,24 @@ def _resolve(
     def get_cached_resolution(
         self,
         req: Requirement,
+        pre_built: bool,
     ) -> tuple[str, Version] | None:
         """Get a cached resolution result if it exists.
 
         Args:
             req: Package requirement to look up in cache
+            pre_built: Whether looking for prebuilt or source resolution
 
         Returns:
             Tuple of (source_url, resolved_version) if cached, None otherwise
         """
-        return self._resolved_requirements.get(str(req))
+        cache_key = (str(req), pre_built)
+        return self._resolved_requirements.get(cache_key)
 
     def cache_resolution(
         self,
         req: Requirement,
+        pre_built: bool,
         result: tuple[str, Version],
     ) -> None:
         """Cache a resolution result.
@@ -185,9 +193,11 @@ def cache_resolution(
 
         Args:
             req: Package requirement to cache
+            pre_built: Whether this is a prebuilt or source resolution
             result: Tuple of (source_url, resolved_version)
         """
-        self._resolved_requirements[str(req)] = result
+        cache_key = (str(req), pre_built)
+        self._resolved_requirements[cache_key] = result
 
     def _resolve_from_graph(
         self,

tests/test_requirement_resolver.py

@@ -1,5 +1,7 @@
 """Tests for requirement_resolver module."""
 
+from unittest.mock import MagicMock, patch
+
 import pytest
 from packaging.requirements import Requirement
 from packaging.utils import canonicalize_name
@@ -248,8 +250,8 @@ def test_resolve_from_graph_no_previous_graph(tmp_context: WorkContext) -> None:
     )
 
 
-def test_resolve_rejects_git_urls(tmp_context: WorkContext) -> None:
-    """RequirementResolver.resolve() rejects git URLs."""
+def test_resolve_rejects_git_urls_for_source(tmp_context: WorkContext) -> None:
+    """RequirementResolver.resolve() rejects git URLs when pre_built=False."""
     resolver = RequirementResolver(tmp_context)
 
     with pytest.raises(
@@ -261,3 +263,170 @@ def test_resolve_rejects_git_urls(tmp_context: WorkContext) -> None:
             pre_built=False,
             parent_req=None,
         )
+
+
+@patch("fromager.requirement_resolver.wheels.resolve_prebuilt_wheel")
+@patch("fromager.requirement_resolver.wheels.get_wheel_server_urls")
+def test_resolve_allows_git_urls_for_prebuilt(
+    mock_get_servers: MagicMock,
+    mock_resolve_wheel: MagicMock,
+    tmp_context: WorkContext,
+) -> None:
+    """RequirementResolver.resolve() allows git URLs when pre_built=True (test mode fallback)."""
+    resolver = RequirementResolver(tmp_context)
+    req = Requirement("mypkg @ git+https://github.com/example/repo.git")
+
+    # Mock wheel resolution to return expected result
+    mock_get_servers.return_value = ["https://pypi.org/simple"]
+    mock_resolve_wheel.return_value = (
+        "https://files.pythonhosted.org/mypkg-1.0-py3-none-any.whl",
+        Version("1.0"),
+    )
+
+    # Should NOT raise - git URLs are allowed when explicitly requesting prebuilt
+    url, version = resolver.resolve(
+        req=req,
+        req_type=RequirementType.INSTALL,
+        pre_built=True,
+        parent_req=None,
+    )
+
+    # Verify it routed to wheel resolution
+    mock_resolve_wheel.assert_called_once()
+    assert url == "https://files.pythonhosted.org/mypkg-1.0-py3-none-any.whl"
+    assert version == Version("1.0")
+
+
+@patch("fromager.requirement_resolver.wheels.resolve_prebuilt_wheel")
+@patch("fromager.requirement_resolver.wheels.get_wheel_server_urls")
+def test_resolve_auto_routes_to_prebuilt(
+    mock_get_servers: MagicMock,
+    mock_resolve_wheel: MagicMock,
+    tmp_context: WorkContext,
+) -> None:
+    """resolve(pre_built=None) with pbi.pre_built=True routes to wheels.resolve_prebuilt_wheel."""
+    req = Requirement("setuptools>=40")
+
+    # Mock package build info to return pre_built=True
+    mock_pbi = MagicMock()
+    mock_pbi.pre_built = True
+
+    with patch.object(tmp_context, "package_build_info", return_value=mock_pbi):
+        resolver = RequirementResolver(tmp_context)
+
+        # Mock wheel resolution to return expected result
+        mock_get_servers.return_value = ["https://pypi.org/simple"]
+        mock_resolve_wheel.return_value = (
+            "https://files.pythonhosted.org/setuptools-1.0-py3-none-any.whl",
+            Version("1.0"),
+        )
+
+        # Call resolve with pre_built=None (should auto-detect)
+        url, version = resolver.resolve(
+            req=req,
+            req_type=RequirementType.INSTALL,
+            parent_req=None,
+            pre_built=None,
+        )
+
+        # Verify it routed to wheel resolution
+        mock_resolve_wheel.assert_called_once()
+        assert url == "https://files.pythonhosted.org/setuptools-1.0-py3-none-any.whl"
+        assert version == Version("1.0")
+
+
+@patch("fromager.requirement_resolver.sources.resolve_source")
+def test_resolve_auto_routes_to_source(
+    mock_resolve_source: MagicMock,
+    tmp_context: WorkContext,
+) -> None:
+    """resolve(pre_built=None) with pbi.pre_built=False routes to sources.resolve_source."""
+    req = Requirement("mypackage>=1.0")
+
+    # Mock package build info to return pre_built=False
+    mock_pbi = MagicMock()
+    mock_pbi.pre_built = False
+
+    with patch.object(tmp_context, "package_build_info", return_value=mock_pbi):
+        resolver = RequirementResolver(tmp_context)
+
+        # Mock source resolution to return expected result
+        mock_resolve_source.return_value = (
+            "https://files.pythonhosted.org/mypackage-2.0.tar.gz",
+            Version("2.0"),
+        )
+
+        # Call resolve with pre_built=None (should auto-detect)
+        url, version = resolver.resolve(
+            req=req,
+            req_type=RequirementType.INSTALL,
+            parent_req=None,
+            pre_built=None,
+        )
+
+        # Verify it routed to source resolution
+        mock_resolve_source.assert_called_once()
+        assert url == "https://files.pythonhosted.org/mypackage-2.0.tar.gz"
+        assert version == Version("2.0")
+
+
+@patch("fromager.requirement_resolver.wheels.resolve_prebuilt_wheel")
+@patch("fromager.requirement_resolver.wheels.get_wheel_server_urls")
+@patch("fromager.requirement_resolver.sources.resolve_source")
+def test_resolve_prebuilt_after_source_uses_separate_cache(
+    mock_resolve_source: MagicMock,
+    mock_get_servers: MagicMock,
+    mock_resolve_wheel: MagicMock,
+    tmp_context: WorkContext,
+) -> None:
+    """resolve(pre_built=True) after same req resolved as source uses separate cache."""
+    req = Requirement("testpkg==1.5")
+
+    # Mock package build info to return pre_built=False initially
+    mock_pbi = MagicMock()
+    mock_pbi.pre_built = False
+
+    with patch.object(tmp_context, "package_build_info", return_value=mock_pbi):
+        resolver = RequirementResolver(tmp_context)
+
+        # Mock source resolution
+        mock_resolve_source.return_value = (
+            "https://files.pythonhosted.org/testpkg-1.5.tar.gz",
+            Version("1.5"),
+        )
+
+        # First call: resolve as source (pre_built=None, auto-detects to False)
+        url1, version1 = resolver.resolve(
+            req=req,
+            req_type=RequirementType.INSTALL,
+            parent_req=None,
+            pre_built=None,
+        )
+
+        assert url1 == "https://files.pythonhosted.org/testpkg-1.5.tar.gz"
+        assert version1 == Version("1.5")
+        assert mock_resolve_source.call_count == 1
+
+        # Mock wheel resolution for second call
+        mock_get_servers.return_value = ["https://pypi.org/simple"]
+        mock_resolve_wheel.return_value = (
+            "https://files.pythonhosted.org/testpkg-1.5-py3-none-any.whl",
+            Version("1.5"),
+        )
+
+        # Second call: resolve same req as prebuilt (explicit pre_built=True)
+        # This should NOT return the cached source result
+        url2, version2 = resolver.resolve(
+            req=req,
+            req_type=RequirementType.INSTALL,
+            parent_req=None,
+            pre_built=True,
+        )
+
+        # Verify it called wheel resolution (not cached)
+        assert mock_resolve_wheel.call_count == 1
+        assert url2 == "https://files.pythonhosted.org/testpkg-1.5-py3-none-any.whl"
+        assert version2 == Version("1.5")
+
+        # Verify source was only called once (first time, not second)
+        assert mock_resolve_source.call_count == 1