refactor(hm-vm): SnapshotId exposes a public String field, allowing arbitrary IDs to be minted anywhere (#119)

markovejnovic · web-flow · commit f1368ca32883 · 2026-06-10T13:12:26.000-07:00
diff --git a/crates/hm-exec/src/local/runner/vm.rs b/crates/hm-exec/src/local/runner/vm.rs
@@ -61,7 +61,7 @@ async fn run_step_vm(vm: &HmVm, ctx: &StepContext, input: ExecutorInput) -> Resu
     };
 
     let source = if let Some(ref snap) = input.parent_snapshot {
-        ImageSource::Snapshot(SnapshotId(snap.0.clone()))
+        ImageSource::Snapshot(SnapshotId::new(snap.0.clone()))
     } else {
         ImageSource::Image(
             input
@@ -137,13 +137,13 @@ async fn run_step_vm(vm: &HmVm, ctx: &StepContext, input: ExecutorInput) -> Resu
             tag: result
                 .snapshot
                 .as_ref()
-                .map_or_else(String::new, |s| s.0.clone()),
+                .map_or_else(String::new, ToString::to_string),
         });
     }
 
     Ok(StepResult {
         exit_code: result.exit_code,
-        committed_snapshot: result.snapshot.map(|s| SnapshotRef(s.0)),
+        committed_snapshot: result.snapshot.map(|s| SnapshotRef(s.to_string())),
         artifacts: vec![],
     })
 }
diff --git a/crates/hm-vm/src/docker.rs b/crates/hm-vm/src/docker.rs
@@ -112,7 +112,7 @@ impl VmBackend for DockerBackend {
 
     #[instrument(skip(self, _config))]
     async fn restore(&self, snapshot: &SnapshotId, _config: &VmConfig) -> Result<Box<dyn Vm>> {
-        let container_id = self.start_container(&snapshot.0).await?;
+        let container_id = self.start_container(snapshot.as_ref()).await?;
         Ok(Box::new(DockerVm {
             client: self.client.clone(),
             container_id: Some(container_id),
@@ -121,22 +121,22 @@ impl VmBackend for DockerBackend {
 
     #[instrument(skip(self))]
     async fn snapshot_exists(&self, snapshot: &SnapshotId) -> Result<bool> {
-        self.image_exists_by_tag(&snapshot.0).await
+        self.image_exists_by_tag(snapshot.as_ref()).await
     }
 
     #[instrument(skip(self))]
     async fn remove_snapshot(&self, snapshot: &SnapshotId) -> Result<()> {
         self.client
             .remove_image(
-                &snapshot.0,
+                snapshot.as_ref(),
                 Some(RemoveImageOptions {
                     force: true,
                     noprune: false,
                 }),
                 None,
             )
             .await
-            .with_context(|| format!("removing image '{}'", snapshot.0))?;
+            .with_context(|| format!("removing image '{snapshot}'"))?;
         Ok(())
     }
 }
@@ -346,7 +346,7 @@ impl Vm for DockerVm {
             .await
             .context("commit container")?;
         let full_tag = format!("{repo}:{tag}");
-        Ok(SnapshotId(full_tag))
+        Ok(SnapshotId::new(full_tag))
     }
 
     #[instrument(skip(self))]
diff --git a/crates/hm-vm/src/registry.rs b/crates/hm-vm/src/registry.rs
@@ -102,7 +102,7 @@ impl ImageRegistry {
         }
 
         drop(conn);
-        snapshot.map(SnapshotId)
+        snapshot.map(SnapshotId::new)
     }
 
     /// Insert or update a cache entry.
@@ -118,10 +118,11 @@ impl ImageRegistry {
         };
 
         // INSERT OR REPLACE handles both new and updated entries.
+        let snapshot_id: &str = snapshot.as_ref();
         let _result = conn.execute(
             "INSERT OR REPLACE INTO snapshots (key, snapshot_id, accessed_at)
              VALUES (?1, ?2, ?3)",
-            rusqlite::params![key, snapshot.0, now],
+            rusqlite::params![key, snapshot_id, now],
         );
 
         drop(conn);
@@ -149,7 +150,7 @@ impl ImageRegistry {
         }
 
         drop(conn);
-        snapshot.map(SnapshotId)
+        snapshot.map(SnapshotId::new)
     }
 
     /// Returns the number of cached entries.
@@ -193,7 +194,9 @@ impl ImageRegistry {
         };
 
         let evicted: Vec<SnapshotId> = stmt
-            .query_map([overflow], |row| row.get::<_, String>(0).map(SnapshotId))
+            .query_map([overflow], |row| {
+                row.get::<_, String>(0).map(SnapshotId::new)
+            })
             .ok()
             .map(|rows| rows.filter_map(Result::ok).collect())
             .unwrap_or_default();
@@ -234,25 +237,25 @@ mod tests {
     #[test]
     fn put_then_get_returns_snapshot() {
         let (reg, _dir) = open_temp(10);
-        let snap = SnapshotId("snap-abc".into());
+        let snap = SnapshotId::new("snap-abc");
         let evicted = reg.put("my-key", &snap);
         assert!(evicted.is_empty());
 
         let got = reg.get("my-key");
-        assert_eq!(got, Some(SnapshotId("snap-abc".into())));
+        assert_eq!(got, Some(SnapshotId::new("snap-abc")));
     }
 
     #[test]
     fn get_updates_access_time() {
         let (reg, _dir) = open_temp(2);
 
         // Insert a, then b. "a" is older by insertion order.
-        reg.put("a", &SnapshotId("snap-a".into()));
+        reg.put("a", &SnapshotId::new("snap-a"));
 
         // Tiny sleep so timestamps differ.
         std::thread::sleep(std::time::Duration::from_secs(1));
 
-        reg.put("b", &SnapshotId("snap-b".into()));
+        reg.put("b", &SnapshotId::new("snap-b"));
 
         // Touch "a" so it becomes the most recently accessed.
         std::thread::sleep(std::time::Duration::from_secs(1));
@@ -261,10 +264,10 @@ mod tests {
         // Now insert "c" -- capacity is 2, so one must be evicted.
         // "b" should be evicted since "a" was touched more recently.
         std::thread::sleep(std::time::Duration::from_secs(1));
-        let evicted = reg.put("c", &SnapshotId("snap-c".into()));
+        let evicted = reg.put("c", &SnapshotId::new("snap-c"));
 
         assert_eq!(evicted.len(), 1);
-        assert_eq!(evicted[0], SnapshotId("snap-b".into()));
+        assert_eq!(evicted[0], SnapshotId::new("snap-b"));
 
         // "a" should still be present.
         assert!(reg.get("a").is_some());
@@ -276,16 +279,16 @@ mod tests {
     fn eviction_returns_overflow_entries() {
         let (reg, _dir) = open_temp(2);
 
-        reg.put("x", &SnapshotId("snap-x".into()));
+        reg.put("x", &SnapshotId::new("snap-x"));
         std::thread::sleep(std::time::Duration::from_secs(1));
-        reg.put("y", &SnapshotId("snap-y".into()));
+        reg.put("y", &SnapshotId::new("snap-y"));
         std::thread::sleep(std::time::Duration::from_secs(1));
 
         // This third insert should evict the oldest ("x").
-        let evicted = reg.put("z", &SnapshotId("snap-z".into()));
+        let evicted = reg.put("z", &SnapshotId::new("snap-z"));
 
         assert_eq!(evicted.len(), 1);
-        assert_eq!(evicted[0], SnapshotId("snap-x".into()));
+        assert_eq!(evicted[0], SnapshotId::new("snap-x"));
         assert_eq!(reg.len(), 2);
     }
 
@@ -296,25 +299,25 @@ mod tests {
 
         {
             let reg = ImageRegistry::open(&db_path, 10).expect("open");
-            reg.put("persistent", &SnapshotId("snap-persist".into()));
+            reg.put("persistent", &SnapshotId::new("snap-persist"));
             assert_eq!(reg.len(), 1);
             // reg is dropped here, closing the connection.
         }
 
         let reg2 = ImageRegistry::open(&db_path, 10).expect("reopen");
         assert_eq!(reg2.len(), 1);
         let got = reg2.get("persistent");
-        assert_eq!(got, Some(SnapshotId("snap-persist".into())));
+        assert_eq!(got, Some(SnapshotId::new("snap-persist")));
     }
 
     #[test]
     fn invalidate_returns_removed_snapshot() {
         let (reg, _dir) = open_temp(10);
-        let snap = SnapshotId("snap-rm".into());
+        let snap = SnapshotId::new("snap-rm");
         reg.put("to-remove", &snap);
 
         let removed = reg.invalidate("to-remove");
-        assert_eq!(removed, Some(SnapshotId("snap-rm".into())));
+        assert_eq!(removed, Some(SnapshotId::new("snap-rm")));
         assert!(reg.get("to-remove").is_none());
         assert_eq!(reg.len(), 0);
 
diff --git a/crates/hm-vm/src/types.rs b/crates/hm-vm/src/types.rs
@@ -50,9 +50,35 @@ pub enum SnapshotLabel {
 }
 
 /// Opaque snapshot handle. Backend-specific contents.
+///
+/// The inner representation is private so a snapshot id can only be minted
+/// through [`SnapshotId::new`]; read access goes through the `AsRef<str>` /
+/// `Deref<Target = str>` impls or the `Display` impl. This keeps the handle a
+/// distinct domain newtype rather than an interchangeable `String`.
 #[derive(Clone, Debug, Hash, PartialEq, Eq, derive_more::Display)]
 #[display("{_0}")]
-pub struct SnapshotId(pub String);
+pub struct SnapshotId(String);
+
+impl SnapshotId {
+    /// Construct a snapshot handle from a backend-specific id.
+    pub fn new(id: impl Into<String>) -> Self {
+        Self(id.into())
+    }
+}
+
+impl AsRef<str> for SnapshotId {
+    fn as_ref(&self) -> &str {
+        &self.0
+    }
+}
+
+impl std::ops::Deref for SnapshotId {
+    type Target = str;
+
+    fn deref(&self) -> &str {
+        &self.0
+    }
+}
 
 /// Result of executing an action.
 #[derive(Clone, Debug)]
diff --git a/crates/hm-vm/src/vm.rs b/crates/hm-vm/src/vm.rs
@@ -118,7 +118,7 @@ impl HmVm {
                 let evicted = self.registry.put(key, &snap);
                 for old in &evicted {
                     if let Err(e) = self.backend.remove_snapshot(old).await {
-                        warn!(snapshot = %old.0, error = %e, "failed to remove evicted snapshot");
+                        warn!(snapshot = %old, error = %e, "failed to remove evicted snapshot");
                     }
                 }
             }
@@ -186,7 +186,7 @@ mod tests {
         async fn restore(&self, snapshot: &SnapshotId, _config: &VmConfig) -> Result<Box<dyn Vm>> {
             self.calls
                 .lock()
-                .map_or_else(|_| {}, |mut c| c.push(format!("restore:{}", snapshot.0)));
+                .map_or_else(|_| {}, |mut c| c.push(format!("restore:{snapshot}")));
             Ok(Box::new(MockVm {
                 calls: Arc::clone(&self.calls),
                 exit_code: self.exit_code,
@@ -196,15 +196,15 @@ mod tests {
         async fn snapshot_exists(&self, snapshot: &SnapshotId) -> Result<bool> {
             self.calls.lock().map_or_else(
                 |_| {},
-                |mut c| c.push(format!("snapshot_exists:{}", snapshot.0)),
+                |mut c| c.push(format!("snapshot_exists:{snapshot}")),
             );
             Ok(self.snapshot_exists)
         }
 
         async fn remove_snapshot(&self, snapshot: &SnapshotId) -> Result<()> {
             self.calls.lock().map_or_else(
                 |_| {},
-                |mut c| c.push(format!("remove_snapshot:{}", snapshot.0)),
+                |mut c| c.push(format!("remove_snapshot:{snapshot}")),
             );
             Ok(())
         }
@@ -246,7 +246,7 @@ mod tests {
             self.calls
                 .lock()
                 .map_or_else(|_| {}, |mut c| c.push(format!("snapshot:{label}")));
-            Ok(SnapshotId(format!("snap-{label}")))
+            Ok(SnapshotId::new(format!("snap-{label}")))
         }
 
         async fn destroy(&mut self) -> Result<()> {
@@ -322,7 +322,7 @@ mod tests {
         let (registry, _dir) = open_temp_registry(10);
 
         // Pre-populate the registry.
-        registry.put("step-1", &SnapshotId("cached-snap".into()));
+        registry.put("step-1", &SnapshotId::new("cached-snap"));
 
         let hm = HmVm::new(Arc::new(backend.clone()), registry, VmConfig::default());
 
@@ -339,7 +339,7 @@ mod tests {
 
         assert_eq!(result.exit_code, 0);
         assert!(result.cached);
-        assert_eq!(result.snapshot, Some(SnapshotId("cached-snap".into())));
+        assert_eq!(result.snapshot, Some(SnapshotId::new("cached-snap")));
 
         let log = calls(&backend);
         // Only snapshot_exists should have been called -- no create, exec, etc.
