Skip to main content

max / audiofiles

Content/Storage A+: seal SampleHash construction, unify loose-files location D1 — SampleHash construction seal: - `validated` is the sole validating constructor for untrusted input; `new` renamed `from_trusted` (trusted origins only: post-hash_file/import/DB) - drop the dead `From<&str>`/`From<String>` impls (the silent `.into()` bypass) - `Deserialize` now validates, so a SampleHash from a saved config/wire payload enforces the 64-lowercase-hex invariant; FromSql stays lax (DB is trusted) - `store::validate_hash` delegates to `SampleHash::validated` — one rule set, no more two validators that can drift on uppercase D2 — loose-files location unify: - `store::sample_location` + `store_blob_path`: single source of truth for where a sample's bytes live (loose `source_path` vs store blob); `resolve_file_path` rebuilt on it - `vfs_mirror` routes through `sample_location` instead of hand-rolling `store_root.join(hash)`, fixing dangling symlinks for loose-files samples (regression test added) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-06-22 21:42 UTC
Commit: 239870a192111d33c704e5b0c0d8bb07b0d5b05a
Parent: 445c39b
7 files changed, +198 insertions, -75 deletions
M crates/audiofiles-browser/src/state/import_workflow.rs +1 -1
@@ -607,7 +607,7 @@ impl BrowserState {
607 607
608 608 // Backfill: persist vectors + apply rules only — no review queue.
609 609 if !self.backfill_in_progress {
610 - let hash = audiofiles_core::SampleHash::new(result.hash.clone());
610 + let hash = audiofiles_core::SampleHash::from_trusted(result.hash.clone());
611 611 let name = self.backend.sample_original_name(&hash)
612 612 .unwrap_or_else(|_| hash.to_string());
613 613
M crates/audiofiles-browser/src/state/tests.rs +1 -1
@@ -772,7 +772,7 @@ mod import_and_analysis {
772 772 current_name: "test.wav".to_string(),
773 773 };
774 774 state.pending_review_items.push(ReviewItem {
775 - hash: audiofiles_core::SampleHash::new("abc"),
775 + hash: audiofiles_core::SampleHash::from_trusted("abc"),
776 776 name: "test.wav".to_string(),
777 777 result: audiofiles_core::analysis::AnalysisResult {
778 778 hash: "abc".to_string(),
M crates/audiofiles-core/src/export/mod.rs +4 -4
@@ -187,7 +187,7 @@ fn map_export_item(row: &rusqlite::Row) -> rusqlite::Result<Option<ExportItem>>
187 187 let source_path: Option<String> = row.get(8)?;
188 188
189 189 Ok(Some(ExportItem {
190 - hash: crate::SampleHash::new(hash),
190 + hash: crate::SampleHash::from_trusted(hash),
191 191 ext,
192 192 relative_path: PathBuf::from(path),
193 193 name,
@@ -732,7 +732,7 @@ mod tests {
732 732
733 733 fn make_item(name: &str, ext: &str) -> ExportItem {
734 734 ExportItem {
735 - hash: crate::SampleHash::new("deadbeef"),
735 + hash: crate::SampleHash::from_trusted("deadbeef"),
736 736 ext: ext.to_string(),
737 737 relative_path: PathBuf::from(name),
738 738 name: name.to_string(),
@@ -747,7 +747,7 @@ mod tests {
747 747
748 748 fn make_item_with_analysis(name: &str, ext: &str, bpm: f64, key: &str) -> ExportItem {
749 749 ExportItem {
750 - hash: crate::SampleHash::new("deadbeef"),
750 + hash: crate::SampleHash::from_trusted("deadbeef"),
751 751 ext: ext.to_string(),
752 752 relative_path: PathBuf::from(name),
753 753 name: name.to_string(),
@@ -969,7 +969,7 @@ mod tests {
969 969
970 970 // Create an item with a hash that doesn't exist in the store
971 971 let items = vec![ExportItem {
972 - hash: crate::SampleHash::new("nonexistent_hash_value"),
972 + hash: crate::SampleHash::from_trusted("nonexistent_hash_value"),
973 973 ext: "wav".to_string(),
974 974 relative_path: PathBuf::from("missing.wav"),
975 975 name: "missing.wav".to_string(),
M crates/audiofiles-core/src/export/resolve.rs +1 -1
@@ -127,7 +127,7 @@ mod tests {
127 127
128 128 fn item(name: &str) -> ExportItem {
129 129 ExportItem {
130 - hash: crate::SampleHash::new("a".repeat(64)),
130 + hash: crate::SampleHash::from_trusted("a".repeat(64)),
131 131 ext: "wav".to_string(),
132 132 relative_path: PathBuf::from(name),
133 133 name: name.to_string(),
M crates/audiofiles-core/src/id_types.rs +40 -22
@@ -74,17 +74,23 @@ define_i64_id!(VfsId, NodeId, SmartFolderId, CollectionId);
74 74 pub struct SampleHash(String);
75 75
76 76 impl SampleHash {
77 - /// Create from a trusted hash string (e.g. from the database or import pipeline).
78 - pub fn new(s: impl Into<String>) -> Self {
77 + /// Wrap a hash whose origin is already trusted to be a valid SHA-256 digest:
78 + /// the output of `hash_file`/the import pipeline, or a row read back from our
79 + /// own database. This skips validation by design — call it ONLY at those
80 + /// origins. Untrusted text (network, manifests, saved configs) must go through
81 + /// [`SampleHash::validated`] (or arrive via the validating `Deserialize` impl),
82 + /// which is the single gate that enforces the invariant. The name is the
83 + /// review smell: a `from_trusted` on a non-derived string is a bug.
84 + pub fn from_trusted(s: impl Into<String>) -> Self {
79 85 Self(s.into())
80 86 }
81 87
82 - /// Validate and create from untrusted input. Returns an error if the hash
83 - /// is not exactly 64 lowercase hex characters.
88 + /// The sole validating constructor for untrusted input. Returns an error if
89 + /// the hash is not exactly 64 lowercase hex characters.
84 90 ///
85 - /// Lowercase-only matches `store::validate_hash`: the app only ever produces
86 - /// lowercase hashes (`format!("{:x}")`), so the two validators agree on every
87 - /// input rather than diverging on uppercase.
91 + /// `store::validate_hash` delegates here, so there is one rule set rather than
92 + /// two that can drift. Lowercase-only matches how the app produces hashes
93 + /// (`format!("{:x}")`), so every real hash passes and uppercase is rejected.
88 94 pub fn validated(s: &str) -> std::result::Result<Self, &'static str> {
89 95 if s.len() != 64
90 96 || !s
@@ -136,17 +142,10 @@ impl PartialEq<&str> for SampleHash {
136 142 }
137 143 }
138 144
139 - impl From<String> for SampleHash {
140 - fn from(s: String) -> Self {
141 - Self(s)
142 - }
143 - }
144 -
145 - impl From<&str> for SampleHash {
146 - fn from(s: &str) -> Self {
147 - Self(s.to_string())
148 - }
149 - }
145 + // No blanket `From<String>`/`From<&str>`: those let any raw string become a
146 + // `SampleHash` via `.into()`, silently bypassing validation. Untrusted text goes
147 + // through `validated` (or the validating `Deserialize` below); trusted origins use
148 + // `from_trusted` explicitly.
150 149
151 150 impl serde::Serialize for SampleHash {
152 151 fn serialize<S: serde::Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
@@ -155,8 +154,12 @@ impl serde::Serialize for SampleHash {
155 154 }
156 155
157 156 impl<'de> serde::Deserialize<'de> for SampleHash {
157 + /// Validating: a `SampleHash` deserialized from a saved config / wire payload
158 + /// is the same untrusted boundary as `validated`, so it enforces the same
159 + /// 64-lowercase-hex invariant rather than trusting whatever bytes arrive.
158 160 fn deserialize<D: serde::Deserializer<'de>>(deserializer: D) -> Result<Self, D::Error> {
159 - String::deserialize(deserializer).map(Self)
161 + let s = String::deserialize(deserializer)?;
162 + SampleHash::validated(&s).map_err(serde::de::Error::custom)
160 163 }
161 164 }
162 165
@@ -167,6 +170,9 @@ impl rusqlite::types::ToSql for SampleHash {
167 170 }
168 171
169 172 impl rusqlite::types::FromSql for SampleHash {
173 + /// Lax by design: rows come from our own database, which only ever stores
174 + /// validated hashes, so this is a trusted origin (cf. `from_trusted`). The
175 + /// untrusted gates are `validated` and `Deserialize`.
170 176 fn column_result(value: rusqlite::types::ValueRef<'_>) -> rusqlite::types::FromSqlResult<Self> {
171 177 String::column_result(value).map(Self)
172 178 }
@@ -213,7 +219,7 @@ mod tests {
213 219
214 220 #[test]
215 221 fn sample_hash_deref() {
216 - let h = SampleHash::new("abc123");
222 + let h = SampleHash::from_trusted("abc123");
217 223 assert_eq!(&*h, "abc123");
218 224 assert_eq!(h.as_str(), "abc123");
219 225 }
@@ -244,15 +250,27 @@ mod tests {
244 250
245 251 #[test]
246 252 fn sample_hash_serde_roundtrip() {
247 - let h = SampleHash::new("test_hash");
253 + let h = SampleHash::from_trusted("a".repeat(64));
248 254 let json = serde_json::to_string(&h).unwrap();
249 255 let parsed: SampleHash = serde_json::from_str(&json).unwrap();
250 256 assert_eq!(h, parsed);
251 257 }
252 258
253 259 #[test]
260 + fn sample_hash_deserialize_rejects_invalid() {
261 + // The untrusted boundary enforces the invariant: a short/non-hex hash in
262 + // a saved config or wire payload fails to deserialize rather than landing
263 + // an invalid SampleHash in memory.
264 + assert!(serde_json::from_str::<SampleHash>("\"deadbeef\"").is_err());
265 + let upper = format!("\"{}\"", "A".repeat(64));
266 + assert!(serde_json::from_str::<SampleHash>(&upper).is_err());
267 + let ok = format!("\"{}\"", "a".repeat(64));
268 + assert!(serde_json::from_str::<SampleHash>(&ok).is_ok());
269 + }
270 +
271 + #[test]
254 272 fn sample_hash_eq_str() {
255 - let h = SampleHash::new("abc");
273 + let h = SampleHash::from_trusted("abc");
256 274 assert_eq!(h, "abc");
257 275 assert_eq!(h, "abc");
258 276 }
M crates/audiofiles-core/src/store.rs +86 -26
@@ -73,16 +73,18 @@ pub fn validate_extension(ext: &str) -> Result<()> {
73 73 }
74 74
75 75 /// Validate that a hash string is exactly 64 lowercase hex characters (SHA-256).
76 + ///
77 + /// Delegates to [`crate::SampleHash::validated`] so the rule lives in exactly one
78 + /// place — historically these were two parallel validators that could drift
79 + /// (e.g. on uppercase handling).
76 80 pub fn validate_hash(hash: &str) -> Result<()> {
77 - if hash.len() != 64 || !hash.bytes().all(|b| b.is_ascii_hexdigit() && !b.is_ascii_uppercase())
78 - {
79 - return Err(CoreError::HashInvalid(format!(
81 + crate::SampleHash::validated(hash).map(|_| ()).map_err(|_| {
82 + CoreError::HashInvalid(format!(
80 83 "expected 64 lowercase hex chars, got {:?} ({} chars)",
81 84 hash,
82 85 hash.len()
83 - )));
84 - }
85 - Ok(())
86 + ))
87 + })
86 88 }
87 89
88 90 /// Mark a stored blob read-only (best-effort, cross-platform). A failed chmod
@@ -243,11 +245,7 @@ impl SampleStore {
243 245 pub fn sample_path(&self, hash: &str, ext: &str) -> Result<PathBuf> {
244 246 validate_hash(hash)?;
245 247 validate_extension(ext)?;
246 - if ext.is_empty() {
247 - Ok(self.root.join(hash))
248 - } else {
249 - Ok(self.root.join(format!("{hash}.{ext}")))
250 - }
248 + Ok(store_blob_path(&self.root, hash, ext))
251 249 }
252 250
253 251 /// Remove a sample from store and database. CASCADE handles VFS/tag refs.
@@ -593,26 +591,88 @@ pub fn sample_source_path(db: &Database, hash: &str) -> Result<Option<String>> {
593 591 })
594 592 }
595 593
594 + /// Where a sample's bytes live on disk.
595 + ///
596 + /// `Loose` is a loose-files sample (imported by reference): the bytes stay at the
597 + /// user's original `source_path`, never copied into the store. `Store` is a normal
598 + /// content-addressed blob.
599 + #[derive(Debug, Clone, PartialEq, Eq)]
600 + pub enum SampleLocation {
601 + /// Content-addressed store blob at `{store_root}/{hash}[.ext]`.
602 + Store(PathBuf),
603 + /// Loose-files sample at the user's original path.
604 + Loose(PathBuf),
605 + }
606 +
607 + impl SampleLocation {
608 + /// The resolved on-disk path, regardless of which kind.
609 + pub fn path(&self) -> &Path {
610 + match self {
611 + SampleLocation::Store(p) | SampleLocation::Loose(p) => p,
612 + }
613 + }
614 +
615 + /// Consume into the resolved path.
616 + pub fn into_path(self) -> PathBuf {
617 + match self {
618 + SampleLocation::Store(p) | SampleLocation::Loose(p) => p,
619 + }
620 + }
621 + }
622 +
623 + /// Build the content-addressed store blob path for `hash`+`ext` under
624 + /// `store_root`. The single place `{store_root}/{hash}[.ext]` is constructed — no
625 + /// caller hand-rolls this join (it once drifted in the VFS mirror).
626 + pub fn store_blob_path(store_root: &Path, hash: &str, ext: &str) -> PathBuf {
627 + if ext.is_empty() {
628 + store_root.join(hash)
629 + } else {
630 + store_root.join(format!("{hash}.{ext}"))
631 + }
632 + }
633 +
634 + /// Single source of truth for "where does this sample's bytes live": the
635 + /// loose-files `source_path` if the sample has one, otherwise the store blob.
636 + ///
637 + /// Every consumer that needs a sample's file — playback, preview, export, and the
638 + /// VFS mirror — resolves through here (directly or via [`resolve_file_path`]), so a
639 + /// loose-files sample is never pointed at a non-existent store blob. This is a pure
640 + /// mapping (no filesystem access); [`resolve_file_path`] adds the runtime
641 + /// existence-fallback on top.
642 + pub fn sample_location(
643 + store_root: &Path,
644 + hash: &str,
645 + ext: &str,
646 + source_path: Option<&str>,
647 + ) -> SampleLocation {
648 + match source_path {
649 + Some(sp) => SampleLocation::Loose(PathBuf::from(sp)),
650 + None => SampleLocation::Store(store_blob_path(store_root, hash, ext)),
651 + }
652 + }
653 +
596 654 /// Resolve the actual file path for a sample, checking source_path first.
597 655 ///
598 - /// For loose-files mode samples (source_path is set), returns the source path if
599 - /// the file exists, otherwise falls back to the store path. For normal samples,
600 - /// returns the store path directly.
656 + /// Builds on [`sample_location`] (the canonical loose-vs-store decision) and adds
657 + /// a runtime existence fallback: for a loose-files sample whose original file has
658 + /// moved, falls back to the store blob if one happens to exist, else returns the
659 + /// source path so the caller surfaces a clean "not found".
601 660 pub fn resolve_file_path(store: &SampleStore, db: &Database, hash: &str, ext: &str) -> Result<PathBuf> {
602 - if let Some(sp) = sample_source_path(db, hash)? {
603 - let source = PathBuf::from(&sp);
604 - if source.exists() {
605 - return Ok(source);
606 - }
607 - // Fallback: maybe user re-imported in normal mode or placed file manually
608 - let store_path = store.sample_path(hash, ext)?;
609 - if store_path.exists() {
610 - return Ok(store_path);
661 + match sample_location(store.root(), hash, ext, sample_source_path(db, hash)?.as_deref()) {
662 + SampleLocation::Store(_) => store.sample_path(hash, ext),
663 + SampleLocation::Loose(source) => {
664 + if source.exists() {
665 + return Ok(source);
666 + }
667 + // Fallback: maybe user re-imported in normal mode or placed file manually.
668 + let store_path = store.sample_path(hash, ext)?;
669 + if store_path.exists() {
670 + return Ok(store_path);
671 + }
672 + // Return the source path anyway — caller will handle the "not found".
673 + Ok(source)
611 674 }
612 - // Return the source path anyway — caller will handle the "not found"
613 - return Ok(source);
614 675 }
615 - store.sample_path(hash, ext)
616 676 }
617 677
618 678 /// Update the source_path for a sample after verifying the new file's hash matches.
M crates/audiofiles-core/src/vfs_mirror.rs +65 -20
@@ -1,5 +1,7 @@
1 1 //! VFS symlink mirror: maintains a real directory tree mirroring the VFS
2 - //! with friendly-named symlinks pointing into the content-addressed store.
2 + //! with friendly-named symlinks pointing at each sample's real bytes — the
3 + //! content-addressed store blob for normal samples, the user's original
4 + //! `source_path` for loose-files samples (resolved via `store::sample_location`).
3 5 //!
4 6 //! DAWs and file managers can browse the mirror directory directly.
5 7 //! Unix only (macOS + Linux). Windows skipped (symlinks need Developer Mode).
@@ -11,7 +13,7 @@ use tracing::{debug, instrument, warn};
11 13
12 14 use crate::db::Database;
13 15 use crate::error::{io_err, Result};
14 - use crate::store::sample_extension;
16 + use crate::store::{sample_extension, sample_location, sample_source_path};
15 17 use crate::vfs::{list_full_tree, NodeType};
16 18
17 19 /// Configuration for the mirror directory.
@@ -45,12 +47,12 @@ pub fn sync_mirror(db: &Database, config: &MirrorConfig) -> Result<MirrorStats>
45 47 // Collect existing mirror entries so we can remove stale ones later.
46 48 let mut existing = collect_existing_entries(&config.mirror_root);
47 49
48 - // Build sample hash → extension map (batch query to avoid N+1).
50 + // Build sample hash → (extension, source_path) map (batch query to avoid N+1).
49 51 let hashes: Vec<&str> = tree
50 52 .iter()
51 53 .filter_map(|n| n.sample_hash.as_deref())
52 54 .collect();
53 - let extensions = batch_extensions(db, &hashes);
55 + let locations = batch_locations(db, &hashes);
54 56
55 57 for node in &tree {
56 58 let sanitized = sanitize_path(&node.path);
@@ -69,24 +71,26 @@ pub fn sync_mirror(db: &Database, config: &MirrorConfig) -> Result<MirrorStats>
69 71 }
70 72 NodeType::Sample => {
71 73 if let Some(ref hash) = node.sample_hash {
72 - let ext = extensions
74 + let (ext, source) = locations
73 75 .iter()
74 - .find(|(h, _)| h.as_str() == hash.as_str())
75 - .map(|(_, e)| e.as_str())
76 - .unwrap_or("");
77 -
78 - let store_file = if ext.is_empty() {
79 - config.store_root.join(hash.as_str())
80 - } else {
81 - config.store_root.join(format!("{}.{}", hash, ext))
82 - };
76 + .find(|(h, _, _)| h.as_str() == hash.as_str())
77 + .map(|(_, e, s)| (e.as_str(), s.as_deref()))
78 + .unwrap_or(("", None));
79 +
80 + // Resolve the real on-disk location via the shared primitive:
81 + // loose-files samples point at their source_path, normal
82 + // samples at the store blob. Hand-rolling `store_root.join`
83 + // here is exactly what produced dangling symlinks for
84 + // loose-files samples (the blob never exists for them).
85 + let target =
86 + sample_location(&config.store_root, hash, ext, source).into_path();
83 87
84 88 // Two distinct samples can sanitize to the same display
85 89 // path in one directory. The first wins the bare name; the
86 90 // rest get a disambiguating suffix so no placement is
87 - // silently dropped. A link that already points at our blob
91 + // silently dropped. A link that already points at our target
88 92 // is left as-is.
89 - let link_path = resolve_link_path(&full_path, &store_file);
93 + let link_path = resolve_link_path(&full_path, &target);
90 94
91 95 // Remove from stale set.
92 96 existing.remove(&link_path);
@@ -98,7 +102,7 @@ pub fn sync_mirror(db: &Database, config: &MirrorConfig) -> Result<MirrorStats>
98 102 std::fs::create_dir_all(parent)
99 103 .map_err(|e| io_err(parent, e))?;
100 104 }
101 - create_symlink(&store_file, &link_path)?;
105 + create_symlink(&target, &link_path)?;
102 106 stats.links_created += 1;
103 107 }
104 108 }
@@ -209,15 +213,20 @@ fn sanitize_component(name: &str) -> String {
209 213 s
210 214 }
211 215
212 - /// Batch-query file extensions for a set of sample hashes.
213 - fn batch_extensions(db: &Database, hashes: &[&str]) -> Vec<(String, String)> {
216 + /// Batch-query (extension, source_path) for a set of sample hashes.
217 + ///
218 + /// `source_path` is `Some` only for loose-files samples; the mirror needs it to
219 + /// point the symlink at the user's original file rather than a store blob that
220 + /// was never written.
221 + fn batch_locations(db: &Database, hashes: &[&str]) -> Vec<(String, String, Option<String>)> {
214 222 let mut result = Vec::with_capacity(hashes.len());
215 223 // Deduplicate to avoid redundant queries.
216 224 let mut seen = HashSet::new();
217 225 for &hash in hashes {
218 226 if seen.insert(hash)
219 227 && let Ok(ext) = sample_extension(db, hash) {
220 - result.push((hash.to_string(), ext));
228 + let source = sample_source_path(db, hash).ok().flatten();
229 + result.push((hash.to_string(), ext, source));
221 230 }
222 231 }
223 232 result
@@ -363,6 +372,42 @@ mod tests {
363 372
364 373 #[cfg(unix)]
365 374 #[test]
375 + fn loose_files_sample_symlinks_to_source_path() {
376 + // Regression: the mirror used to hand-roll `store_root.join(hash)` for
377 + // every sample, producing a dangling symlink for loose-files samples
378 + // (their bytes live at source_path, never in the store). It must now
379 + // resolve through `sample_location` and point at the source file.
380 + let (db, mirror_dir, store_dir) = setup();
381 + let src = TempDir::new().unwrap();
382 + let source_file = src.path().join("original_kick.wav");
383 + std::fs::write(&source_file, b"loose audio").unwrap();
384 +
385 + let hash = "a".repeat(64);
386 + insert_fake_sample(&db, &hash);
387 + db.conn()
388 + .execute(
389 + "UPDATE samples SET source_path = ?1 WHERE hash = ?2",
390 + rusqlite::params![source_file.to_string_lossy(), hash],
391 + )
392 + .unwrap();
393 + // Deliberately do NOT create a store blob — there isn't one for loose files.
394 +
395 + let vfs_id = create_vfs(&db, "Library").unwrap();
396 + create_sample_link(&db, vfs_id, None, "kick.wav", &hash).unwrap();
397 +
398 + let config = make_config(&mirror_dir, &store_dir);
399 + let stats = sync_mirror(&db, &config).unwrap();
400 + assert_eq!(stats.links_created, 1);
401 +
402 + let link = mirror_dir.path().join("Library/kick.wav");
403 + let target = std::fs::read_link(&link).unwrap();
404 + assert_eq!(target, source_file, "symlink must point at the loose source");
405 + // And the link must not dangle.
406 + assert!(link.exists(), "loose-files symlink should resolve, not dangle");
407 + }
408 +
409 + #[cfg(unix)]
410 + #[test]
366 411 fn stale_entries_removed() {
367 412 let (db, mirror_dir, store_dir) = setup();
368 413 insert_fake_sample(&db, "abc123");