Fix unused tool versions not removed in prek cache gc (#1436)

Author: j178

crates/prek/src/cli/cache_gc.rs

@@ -4,6 +4,7 @@ use std::ops::AddAssign;
 use std::path::Path;
 
 use anyhow::Result;
+use clap::ValueEnum;
 use owo_colors::OwoColorize;
 use rustc_hash::FxHashMap;
 use rustc_hash::FxHashSet;
@@ -252,7 +253,6 @@ pub(crate) async fn cache_gc(
     )?;
 
     // Sweep tools/<bucket>/<version>
-    // Only do this when we can positively identify used versions (otherwise be conservative).
     let removed_tool_versions = sweep_tool_versions(store, &used_tool_versions, dry_run, verbose)?;
 
     let mut removed_tools = removed_tool_buckets;
@@ -437,13 +437,9 @@ fn sweep_tool_versions(
 ) -> Result<Removal> {
     let mut total = Removal::new(RemovalKind::Tools);
 
-    for (bucket, keep_versions) in used_tool_versions {
-        // If we don't have any positively-identified versions, be conservative.
-        if keep_versions.is_empty() {
-            continue;
-        }
-
+    for bucket in ToolBucket::value_variants() {
         let bucket_root = store.tools_path(*bucket);
+        let keep_versions = used_tool_versions.get(bucket);
         let removed =
             sweep_tool_bucket_versions(*bucket, &bucket_root, keep_versions, dry_run, verbose)?;
         total += removed;
@@ -455,7 +451,7 @@ fn sweep_tool_versions(
 fn sweep_tool_bucket_versions(
     bucket: ToolBucket,
     bucket_root: &Path,
-    keep_versions: &FxHashSet<String>,
+    keep_versions: Option<&FxHashSet<String>>,
     dry_run: bool,
     collect_names: bool,
 ) -> Result<Removal> {
@@ -486,7 +482,11 @@ fn sweep_tool_bucket_versions(
         let Some(version_name) = path.file_name().and_then(|n| n.to_str()) else {
             continue;
         };
-        if keep_versions.contains(version_name) {
+        // Skip hidden/system dirs.
+        if version_name.starts_with('.') {
+            continue;
+        }
+        if keep_versions.is_some_and(|keep| keep.contains(version_name)) {
             continue;
         }
 
@@ -554,6 +554,10 @@ fn sweep_dir_by_name(
         let Some(name) = path.file_name().and_then(|n| n.to_str()) else {
             continue;
         };
+        // Skip hidden/system dirs.
+        if name.starts_with('.') {
+            continue;
+        }
         if keep_names.contains(name) {
             continue;
         }

crates/prek/src/store.rs

@@ -301,7 +301,7 @@ impl Store {
     }
 }
 
-#[derive(Copy, Clone, Eq, Hash, PartialEq)]
+#[derive(Copy, Clone, Eq, Hash, PartialEq, clap::ValueEnum)]
 pub(crate) enum ToolBucket {
     Uv,
     Python,

crates/prek/tests/cache.rs

@@ -399,6 +399,89 @@ fn cache_gc_prunes_unused_tool_versions() -> anyhow::Result<()> {
     Ok(())
 }
 
+#[test]
+fn cache_gc_prunes_tool_versions_without_positive_identification() -> anyhow::Result<()> {
+    let context = TestContext::new();
+
+    context.write_pre_commit_config(indoc::indoc! {r#"
+        repos:
+          - repo: local
+            hooks:
+              - id: local-python
+                name: Local Python Hook
+                entry: "python -c \"print(1)\""
+                language: python
+    "#});
+
+    let home = context.home_dir();
+
+    // Track the config so GC has something to mark from.
+    let config_path = context.work_dir().child(CONFIG_FILE);
+    write_config_tracking_file(home, &[config_path.path()])?;
+
+    // Seed a matching installed hook env marker, but use a toolchain path that is *not* inside
+    // PREK_HOME/tools. This means we cannot positively identify a used tool version, so all
+    // tool versions under the bucket are unused and should be pruned.
+    let env_py = home.child("hooks/python-keep");
+    env_py.create_dir_all()?;
+    let marker_py = json!({
+        "language": "python",
+        "language_version": "3.12.0",
+        "dependencies": [],
+        "env_path": env_py.path(),
+        "toolchain": "/usr/bin/python3",
+        "extra": {},
+    });
+    env_py
+        .child(".prek-hook.json")
+        .write_str(&serde_json::to_string_pretty(&marker_py)?)?;
+
+    // Seed tool versions that should be removed.
+    let py_312 = home.child("tools/python/3.12.0");
+    let py_311 = home.child("tools/python/3.11.0");
+    py_312.create_dir_all()?;
+    py_311.create_dir_all()?;
+
+    // Add a temp dir to ensure it is not removed.
+    home.child("repos/.temp").create_dir_all()?;
+    home.child("tools/.temp").create_dir_all()?;
+
+    cmd_snapshot!(
+        context.filters(),
+        context.command().args(["cache", "gc", "--dry-run", "-v"]),
+        @r"
+    success: true
+    exit_code: 0
+    ----- stdout -----
+    Would remove 2 tools ([SIZE])
+
+    Would remove 2 tools:
+    - python/3.11.0
+      path: [HOME]/tools/python/3.11.0
+    - python/3.12.0
+      path: [HOME]/tools/python/3.12.0
+
+    ----- stderr -----
+    "
+    );
+
+    cmd_snapshot!(context.filters(), context.command().args(["cache", "gc"]), @r"
+    success: true
+    exit_code: 0
+    ----- stdout -----
+    Removed 2 tools ([SIZE])
+
+    ----- stderr -----
+    ");
+
+    py_312.assert(predicates::path::missing());
+    py_311.assert(predicates::path::missing());
+    home.child("tools/python")
+        .assert(predicates::path::is_dir());
+
+    Ok(())
+}
+
 #[test]
 fn cache_gc_keeps_local_hook_env() -> anyhow::Result<()> {
     let context = TestContext::new();