trim verbose comments and docstrings

app/frontend_management.py

@@ -207,19 +207,16 @@ def download_release_asset_zip(release: Release, destination_path: str) -> None:
 class FrontendManager:
     CUSTOM_FRONTENDS_ROOT = str(Path(__file__).parents[1] / "web_custom_versions")
 
-    # Version specifiers that resolve to a moving target on each invocation.
+    # Specifiers that resolve to a moving target. Downloads made via these are
-    # Versions downloaded via these specifiers are tracked in the per-provider
+    # tracked so old copies can be pruned; explicit pins (@1.46.0, @v1.46.0)
-    # metadata file so that stale copies can be pruned when a new release
+    # are left alone.
-    # becomes the current one. Explicitly pinned versions (e.g. ``@1.46.0`` or
-    # ``@v1.46.0``) are left alone so users can keep them around indefinitely
-    # for things like bisecting frontend regressions.
     AUTO_MANAGED_VERSION_SPECIFIERS = ("latest", "prerelease")
-
-    # File written next to per-provider version folders that records which
-    # versions were downloaded via an auto-managed specifier. Hidden so it does
-    # not show up as a sibling release in casual ``ls`` output.
     AUTO_MANAGED_METADATA_FILENAME = ".auto_managed.json"
 
+    # Allowlist for version directory names. Prevents a tampered metadata
+    # file from steering cleanup at paths like "../etc".
+    _VERSION_DIRNAME_PATTERN = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]*$")
+
     @classmethod
     def _provider_dir(cls, repo_owner: str, repo_name: str) -> Path:
         return Path(cls.CUSTOM_FRONTENDS_ROOT) / f"{repo_owner}_{repo_name}"
@@ -228,12 +225,6 @@ class FrontendManager:
     def _auto_managed_metadata_path(cls, repo_owner: str, repo_name: str) -> Path:
         return cls._provider_dir(repo_owner, repo_name) / cls.AUTO_MANAGED_METADATA_FILENAME
 
-    # A version directory name must look like a simple semver-ish token. We
-    # use this as a defensive allowlist when interpreting metadata so a
-    # malformed or tampered ``.auto_managed.json`` cannot point cleanup at
-    # paths outside the provider directory (e.g. ``../somewhere``).
-    _VERSION_DIRNAME_PATTERN = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]*$")
-
     @classmethod
     def _is_safe_version_dirname(cls, name: str) -> bool:
         if not isinstance(name, str):
@@ -244,10 +235,8 @@ class FrontendManager:
 
     @classmethod
     def _read_auto_managed_versions(cls, repo_owner: str, repo_name: str) -> list[str]:
-        """Return the list of versions previously downloaded under an
+        """Versions tracked as auto-managed for this provider. Any error or
-        auto-managed specifier for this provider. Missing, unreadable, or
+        malformed content yields an empty list."""
-        otherwise malformed metadata is treated as an empty list so a bad
-        file never blocks startup or directs cleanup at unrelated paths."""
         metadata_path = cls._auto_managed_metadata_path(repo_owner, repo_name)
         if not metadata_path.exists():
             return []
@@ -270,24 +259,17 @@ class FrontendManager:
         versions = data.get("auto_managed", [])
         if not isinstance(versions, list):
             return []
-        # Filter out anything that doesn't look like a safe version dirname
-        # so a tampered file can't point us at, say, ``../../etc``.
         return [v for v in versions if cls._is_safe_version_dirname(v)]
 
     @classmethod
     def _write_auto_managed_versions(
         cls, repo_owner: str, repo_name: str, versions: list[str]
     ) -> None:
-        """Persist the auto-managed version list atomically. Deduped and
-        sorted for stability so the file is friendly to diffs. Any entry that
-        doesn't look like a safe version dirname is dropped before write so
-        the on-disk metadata always contains valid values."""
         metadata_path = cls._auto_managed_metadata_path(repo_owner, repo_name)
         metadata_path.parent.mkdir(parents=True, exist_ok=True)
         safe_versions = [v for v in versions if cls._is_safe_version_dirname(v)]
         payload = {"auto_managed": sorted(set(safe_versions))}
-        # Atomic write via temp file + rename so a crashed process can't leave
+        # Atomic write: tmp + rename so a crash can't leave a half-written file.
-        # a half-written metadata file behind.
         tmp_path = metadata_path.with_suffix(metadata_path.suffix + ".tmp")
         try:
             with open(tmp_path, "w", encoding="utf-8") as fh:
@@ -309,12 +291,8 @@ class FrontendManager:
     def _prune_auto_managed_versions(
         cls, repo_owner: str, repo_name: str, keep_version: str
     ) -> None:
-        """Remove all auto-managed version folders for this provider other
+        """Remove tracked auto-managed folders other than ``keep_version``
-        than ``keep_version`` and update the metadata to only list it.
+        and rewrite the metadata to list only it."""
-
-        Folders that aren't currently tracked as auto-managed (i.e. explicitly
-        pinned downloads) are never touched.
-        """
         tracked = cls._read_auto_managed_versions(repo_owner, repo_name)
         if not tracked and keep_version is None:
             return
@@ -333,11 +311,8 @@ class FrontendManager:
         for stale_version in tracked:
             if stale_version == keep_version:
                 continue
-            # ``_read_auto_managed_versions`` already filters tracked entries
+            # Defense in depth: read already filters, but re-check here so
-            # through ``_is_safe_version_dirname``, but re-check here so that
+            # the rmtree target is provably under the provider dir.
-            # this helper is also safe when called with externally-supplied
-            # version lists (and so a defense-in-depth audit can confirm the
-            # rmtree target lives under the provider directory).
             if not cls._is_safe_version_dirname(stale_version):
                 logging.warning(
                     "Refusing to clean up suspicious frontend version name: %r",
@@ -356,8 +331,6 @@ class FrontendManager:
                     exc,
                 )
                 continue
-            # Ensure the resolved target lives strictly under the resolved
-            # provider directory (so symlinks / path tricks can't escape).
             if (
                 stale_resolved == provider_dir_resolved
                 or provider_dir_resolved not in stale_resolved.parents
@@ -388,10 +361,8 @@ class FrontendManager:
     def _untrack_auto_managed_version(
         cls, repo_owner: str, repo_name: str, version: str
     ) -> None:
-        """Drop ``version`` from the auto-managed list without deleting its
+        """Drop ``version`` from auto-managed tracking without touching its
-        folder. Used when a user explicitly pins a version that previously
+        folder, so a subsequent explicit pin survives later @latest cleanups."""
-        had been downloaded under ``@latest`` / ``@prerelease`` so the next
-        auto cleanup pass doesn't wipe it out."""
         tracked = cls._read_auto_managed_versions(repo_owner, repo_name)
         if version not in tracked:
             return
@@ -577,9 +548,6 @@ comfyui-workflow-templates is not installed.
                 logging.info(
                     f"Using existing copy of specific frontend version tag: {repo_owner}/{repo_name}@{version}"
                 )
-                # User explicitly pinned this exact version: promote it out of
-                # the auto-managed set so future @latest cleanups won't wipe
-                # it out.
                 cls._untrack_auto_managed_version(
                     repo_owner, repo_name, version.lstrip("v")
                 )
@@ -617,17 +585,10 @@ comfyui-workflow-templates is not installed.
 
         if download_succeeded:
             if is_auto_managed:
-                # Wipe out previously-tracked auto-managed versions and record
-                # the current one. This is what keeps disk usage bounded when
-                # users run with ``--front-end-version <repo>@latest`` over a
-                # long period of time (CORE-285).
                 cls._prune_auto_managed_versions(
                     repo_owner, repo_name, semantic_version
                 )
             else:
-                # An explicit version request matched a folder that had been
-                # downloaded under @latest previously. Promote it so it is no
-                # longer subject to auto-cleanup.
                 cls._untrack_auto_managed_version(
                     repo_owner, repo_name, semantic_version
                 )

tests-unit/app_test/frontend_manager_test.py

@@ -291,14 +291,11 @@ def test_get_installed_templates_version_not_installed():
     assert version is None
 
 
-# ---------------------------------------------------------------------------
 # Auto-managed @latest / @prerelease cleanup (CORE-285)
-# ---------------------------------------------------------------------------
 
 
 @pytest.fixture
 def custom_frontends_root(tmp_path, monkeypatch):
-    """Point ``FrontendManager.CUSTOM_FRONTENDS_ROOT`` at a fresh tmp dir."""
     root = tmp_path / "web_custom_versions"
     root.mkdir()
     monkeypatch.setattr(FrontendManager, "CUSTOM_FRONTENDS_ROOT", str(root))
@@ -306,7 +303,6 @@ def custom_frontends_root(tmp_path, monkeypatch):
 
 
 def _make_version_dir(root, owner, repo, version):
-    """Create ``<root>/<owner>_<repo>/<version>/index.html`` and return path."""
     folder = root / f"{owner}_{repo}" / version
     folder.mkdir(parents=True, exist_ok=True)
     (folder / "index.html").write_text("<html></html>")
@@ -333,8 +329,6 @@ def test_read_auto_managed_versions_corrupt(custom_frontends_root):
 
 @pytest.mark.parametrize("payload", ["[]", "null", '"oops"', "42"])
 def test_read_auto_managed_versions_non_dict_root(custom_frontends_root, payload):
-    """Valid JSON whose root isn't a dict must not raise — bug pointed out in
-    Oracle review: ``data.get(...)`` would throw on non-dict roots."""
     provider_dir = custom_frontends_root / "o_r"
     provider_dir.mkdir()
     (provider_dir / FrontendManager.AUTO_MANAGED_METADATA_FILENAME).write_text(payload)
@@ -342,8 +336,6 @@ def test_read_auto_managed_versions_non_dict_root(custom_frontends_root, payload
 
 
 def test_read_auto_managed_versions_drops_unsafe_names(custom_frontends_root):
-    """Tampered metadata containing path-traversal-y names must be ignored
-    at read time so cleanup never even sees them."""
     provider_dir = custom_frontends_root / "o_r"
     provider_dir.mkdir()
     (provider_dir / FrontendManager.AUTO_MANAGED_METADATA_FILENAME).write_text(
@@ -353,8 +345,6 @@ def test_read_auto_managed_versions_drops_unsafe_names(custom_frontends_root):
 
 
 def test_write_auto_managed_versions_drops_unsafe_names(custom_frontends_root):
-    """Even if a caller passes a tainted list, the file we persist must
-    only contain safe names."""
     FrontendManager._write_auto_managed_versions(
         "o", "r", ["1.0.0", "../escape", "/etc/passwd", "..", "."]
     )
@@ -364,10 +354,6 @@ def test_write_auto_managed_versions_drops_unsafe_names(custom_frontends_root):
 def test_prune_refuses_to_delete_outside_provider_dir(
     custom_frontends_root, monkeypatch
 ):
-    """Defense in depth: even if a hostile name slips past the read/write
-    filters somehow, ``_prune_auto_managed_versions`` must refuse to rmtree
-    anything outside the provider directory."""
-    # Set up a sibling directory that must NOT be touched.
     sibling = custom_frontends_root / "do-not-touch"
     sibling.mkdir()
     (sibling / "marker").write_text("keep me")
@@ -376,8 +362,6 @@ def test_prune_refuses_to_delete_outside_provider_dir(
     provider_dir.mkdir()
     _make_version_dir(custom_frontends_root, "o", "r", "1.0.0")
 
-    # Bypass the read filter to simulate a corrupt-but-parseable list slipping
-    # through (e.g. older code wrote it before this hardening landed).
     monkeypatch.setattr(
         FrontendManager,
         "_read_auto_managed_versions",
@@ -386,24 +370,19 @@ def test_prune_refuses_to_delete_outside_provider_dir(
 
     FrontendManager._prune_auto_managed_versions("o", "r", keep_version="2.0.0")
 
-    # Sibling untouched.
     assert sibling.exists()
     assert (sibling / "marker").read_text() == "keep me"
-    # In-bounds folder still gets cleaned.
     assert not (provider_dir / "1.0.0").exists()
 
 
 def test_prune_auto_managed_versions_removes_stale_and_keeps_pinned(
     custom_frontends_root,
 ):
-    # Two versions previously fetched via @latest, plus an explicitly pinned one.
     _make_version_dir(custom_frontends_root, "o", "r", "1.0.0")
     _make_version_dir(custom_frontends_root, "o", "r", "1.1.0")
     pinned = _make_version_dir(custom_frontends_root, "o", "r", "1.2.0")
     FrontendManager._write_auto_managed_versions("o", "r", ["1.0.0", "1.1.0"])
 
-    # User runs @latest again and it resolves to 1.1.0 — older auto-managed
-    # 1.0.0 should be deleted, pinned 1.2.0 should remain untouched.
     FrontendManager._prune_auto_managed_versions("o", "r", keep_version="1.1.0")
 
     provider_dir = custom_frontends_root / "o_r"
@@ -426,8 +405,6 @@ def test_untrack_auto_managed_version_does_not_delete_folder(custom_frontends_ro
 def test_init_frontend_latest_prunes_previous_auto_managed_versions(
     custom_frontends_root, mock_provider, mock_releases
 ):
-    # Pre-existing folders: 1.0.0 was previously downloaded via @latest, 1.1.5
-    # was explicitly pinned by the user. Now @latest resolves to 2.0.0.
     _make_version_dir(custom_frontends_root, "test-owner", "test-repo", "1.0.0")
     pinned = _make_version_dir(
         custom_frontends_root, "test-owner", "test-repo", "1.1.5"
@@ -436,7 +413,6 @@ def test_init_frontend_latest_prunes_previous_auto_managed_versions(
         "test-owner", "test-repo", ["1.0.0"]
     )
 
-    # Stub out the actual download so we just create the destination dir.
     def fake_download(release, destination_path):
         Path(destination_path).mkdir(parents=True, exist_ok=True)
         (Path(destination_path) / "index.html").write_text("<html></html>")
@@ -450,55 +426,43 @@ def test_init_frontend_latest_prunes_previous_auto_managed_versions(
         )
 
     provider_dir = custom_frontends_root / "test-owner_test-repo"
-    # 2.0.0 was downloaded and tracked.
     assert Path(result) == provider_dir / "2.0.0"
     assert (provider_dir / "2.0.0").exists()
     assert FrontendManager._read_auto_managed_versions(
         "test-owner", "test-repo"
     ) == ["2.0.0"]
-    # Old auto-managed 1.0.0 was pruned.
     assert not (provider_dir / "1.0.0").exists()
-    # Pinned 1.1.5 was left alone.
     assert pinned.exists()
 
 
 def test_init_frontend_explicit_version_promotes_out_of_auto_managed(
     custom_frontends_root, mock_provider
 ):
-    # 1.0.0 was previously downloaded via @latest.
     _make_version_dir(custom_frontends_root, "test-owner", "test-repo", "1.0.0")
     FrontendManager._write_auto_managed_versions(
         "test-owner", "test-repo", ["1.0.0"]
     )
 
-    # User now explicitly pins it. The `v`-prefixed early-return path runs.
     result = FrontendManager.init_frontend_unsafe(
         "test-owner/test-repo@v1.0.0", mock_provider
     )
 
     provider_dir = custom_frontends_root / "test-owner_test-repo"
     assert Path(result) == provider_dir / "1.0.0"
-    # It should no longer be tracked as auto-managed, so a future @latest run
-    # won't sweep it away.
     assert FrontendManager._read_auto_managed_versions(
         "test-owner", "test-repo"
     ) == []
-    # The folder is still on disk.
     assert (provider_dir / "1.0.0").exists()
 
 
-def test_init_frontend_explicit_version_no_v_prefix_promotes_out_of_auto_managed(
+def test_init_frontend_explicit_no_v_prefix_promotes_out_of_auto_managed(
     custom_frontends_root, mock_provider
 ):
-    # 1.0.0 was previously downloaded via @latest, and is also already on
-    # disk so the GitHub-resolution path is a no-op for download.
     _make_version_dir(custom_frontends_root, "test-owner", "test-repo", "1.0.0")
     FrontendManager._write_auto_managed_versions(
         "test-owner", "test-repo", ["1.0.0"]
     )
 
-    # No `v` prefix → goes through the GitHub release lookup path. The folder
-    # already exists, so download is skipped.
     with patch(
         "app.frontend_management.download_release_asset_zip"
     ) as mock_download_zip:
@@ -507,7 +471,6 @@ def test_init_frontend_explicit_version_no_v_prefix_promotes_out_of_auto_managed
         )
         mock_download_zip.assert_not_called()
 
-    # It should be promoted out of auto-managed even when the folder is reused.
     assert FrontendManager._read_auto_managed_versions(
         "test-owner", "test-repo"
     ) == []