Address PR review cleanup

Author: ayhammouda

src/mcp_server_python_docs/__main__.py

@@ -232,15 +232,20 @@ def build_index(versions: str, skip_content: bool) -> None:
                     pip_path = os.path.join(scripts_dir, "pip")
 
                     # Install Sphinx with the version pin for this CPython branch.
-                    # Older Sphinx releases still import pkg_resources, which
-                    # modern venvs do not always seed by default.
+                    bootstrap_requirements = build_sphinx_bootstrap_requirements(
+                        config["sphinx_pin"]
+                    )
+                    if len(bootstrap_requirements) > 1:
+                        logger.info(
+                            "Installing Sphinx bootstrap packages for Python %s: %s",
+                            version,
+                            ", ".join(bootstrap_requirements[:-1]),
+                        )
                     subprocess.run(
                         [
                             pip_path,
                             "install",
-                            *build_sphinx_bootstrap_requirements(
-                                config["sphinx_pin"]
-                            ),
+                            *bootstrap_requirements,
                         ],
                         check=True,
                         capture_output=True,

src/mcp_server_python_docs/ingestion/publish.py

@@ -14,6 +14,7 @@
 from collections.abc import Iterable
 from datetime import datetime
 from pathlib import Path
+from typing import Final
 
 from mcp_server_python_docs.storage.db import (
     get_cache_dir,
@@ -23,7 +24,7 @@
 
 logger = logging.getLogger(__name__)
 
-SMOKE_SENTINEL_SYMBOL = "asyncio.run"
+SMOKE_SENTINEL_SYMBOL: Final[str] = "asyncio.run"
 
 
 def _version_sort_key(version: str) -> tuple[int, ...]:

src/mcp_server_python_docs/ingestion/sphinx_json.py

@@ -63,13 +63,14 @@ def _mcp_json_default(self, obj):
     jsonimpl.SphinxJSONEncoder.default = _mcp_json_default
 '''
 
-_IMGHDR_COMPAT_MODULE = '''"""Compatibility shim for old Sphinx on Python 3.13+."""
+_IMGHDR_COMPAT_MODULE = '''"""Compatibility shim for Sphinx builds that import imghdr."""
 
 from __future__ import annotations
 
 import os
 
 
+# Preserve the stdlib imghdr extension hook for old Sphinx-related extensions.
 tests = []
 
 
@@ -144,6 +145,8 @@ def write_sphinx_json_sitecustomize(output_dir: Path) -> Path:
     output_dir.mkdir(parents=True, exist_ok=True)
     sitecustomize_path = output_dir / "sitecustomize.py"
     sitecustomize_path.write_text(_SPHINX_JSON_SITECUSTOMIZE, encoding="utf-8")
+    # PYTHONPATH prepending makes this shim shadow stdlib imghdr on Python 3.12.
+    # The detected formats match the Sphinx usage we need for CPython docs builds.
     imghdr_path = output_dir / "imghdr.py"
     imghdr_path.write_text(_IMGHDR_COMPAT_MODULE, encoding="utf-8")
     return sitecustomize_path
@@ -182,17 +185,30 @@ def build_sphinx_json_command(
     ]
 
 
+def _sphinx_pin_needs_pkg_resources(sphinx_pin: str) -> bool:
+    normalized = sphinx_pin.strip().lower().replace(" ", "")
+    return normalized.startswith(
+        (
+            "sphinx==3.",
+            "sphinx==4.",
+            "sphinx~=3.",
+            "sphinx~=4.",
+            "sphinx<5",
+            "sphinx<=4",
+        )
+    )
+
+
 def build_sphinx_bootstrap_requirements(sphinx_pin: str) -> list[str]:
     """Return packages needed before installing CPython Doc requirements.
 
-    setuptools<70 keeps ``pkg_resources`` available, which old Sphinx
+    setuptools<70 keeps ``pkg_resources`` available when old Sphinx
     releases (e.g. the 3.4.x line pinned for the Python 3.10 docs build)
     still import at startup.
     """
-    return [
-        "setuptools<70",
-        sphinx_pin,
-    ]
+    if _sphinx_pin_needs_pkg_resources(sphinx_pin):
+        return ["setuptools<70", sphinx_pin]
+    return [sphinx_pin]
 
 
 def parse_fjson(filepath: Path) -> dict:

tests/test_ci_workflows.py

@@ -4,6 +4,8 @@
 from pathlib import Path
 
 PROJECT_ROOT = Path(__file__).parent.parent
+# Keep this literal independent of SUPPORTED_DOC_VERSIONS_CSV so the test catches
+# workflow drift even if the application constant changes at the same time.
 SUPPORTED_VERSION_ARGS = "3.10,3.11,3.12,3.13,3.14"
 
 

tests/test_ingestion.py

@@ -7,6 +7,7 @@
 """
 from __future__ import annotations
 
+import io
 import os
 import runpy
 import shutil
@@ -117,9 +118,31 @@ def test_writes_imghdr_compat_module(self, tmp_path):
 
         content = (output_dir / "imghdr.py").read_text(encoding="utf-8")
         assert "tests = []" in content
+        assert "stdlib imghdr extension hook" in content
         assert "def what" in content
         assert "jpeg" in content
 
+    def test_imghdr_compat_module_detects_sphinx_image_formats(self, tmp_path):
+        output_dir = tmp_path / "compat"
+        write_sphinx_json_sitecustomize(output_dir)
+        namespace = runpy.run_path(str(output_dir / "imghdr.py"))
+
+        what = namespace["what"]
+
+        assert what(io.BytesIO(b"\xff\xd8\xff\xe0")) == "jpeg"
+        assert what(io.BytesIO(b"\x89PNG\r\n\x1a\nextra")) == "png"
+        assert what(io.BytesIO(b"GIF89aextra")) == "gif"
+
+    def test_imghdr_compat_module_preserves_tests_hook(self, tmp_path):
+        output_dir = tmp_path / "compat"
+        write_sphinx_json_sitecustomize(output_dir)
+        namespace = runpy.run_path(str(output_dir / "imghdr.py"))
+
+        tests = namespace["tests"]
+        tests.append(lambda header, _file: "bmp" if header.startswith(b"BM") else None)
+
+        assert namespace["what"](io.BytesIO(b"BMfake")) == "bmp"
+
     def test_translation_proxy_patch_stringifies_proxy_objects(
         self, tmp_path, monkeypatch
     ):
@@ -180,6 +203,16 @@ def test_bootstrap_requirements_include_setuptools_before_sphinx(self):
 
         assert requirements == ["setuptools<70", "sphinx==3.4.3"]
 
+    def test_bootstrap_requirements_include_setuptools_for_sphinx_4(self):
+        requirements = build_sphinx_bootstrap_requirements("Sphinx < 5")
+
+        assert requirements == ["setuptools<70", "Sphinx < 5"]
+
+    def test_bootstrap_requirements_skip_setuptools_for_modern_sphinx(self):
+        requirements = build_sphinx_bootstrap_requirements("sphinx~=8.2.0")
+
+        assert requirements == ["sphinx~=8.2.0"]
+
     def test_build_command_uses_json_builder_and_classic_theme(self, tmp_path):
         sphinx_build = tmp_path / "bin" / "sphinx-build"
         doc_dir = tmp_path / "cpython" / "Doc"