Skip to main content

max / audiofiles

Normalise detected musical key to "<note> minor" spelling detect_bpm_key stored stratum-dsp's compact DJ-style name ("Am", "C#m", "C") straight through, but every consumer expects "<note> major" / "<note> minor". search::compatible_keys matches on ends_with("minor") against a table written that way, so KeyFilterMode::Compatible returned nothing usable for any key the pipeline had ever written, and analysis::suggest emitted "key.am" instead of "key.a-minor". Normalise at the detector rather than at each consumer: one spelling in the database, one place to change it. Migration 034 rewrites rows written before this. The rename {key} token now renders the compact form, so generated filenames keep producing "Am" rather than gaining a space. Every existing test built a musical_key by hand, which is why the mismatch survived a passing suite. Add a test that drives the real detector and asserts the spelling contract plus that compatible_keys resolves the result.
Co-Authored-By
Claude Opus 5 (1M context) <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-07-29 16:04 UTC
Signed with PGP, not checked
Commit: 69a616d93ca82c4faeffef78786287fa28d3634d
Parent: 5963710
3 files changed, +191 insertions, -10 deletions
M crates/audiofiles-core/src/db.rs +83 -6
@@ -1618,6 +1618,33 @@
1618 1618 END;
1619 1619 ";
1620 1620
1621 + const MIGRATION_034: &str = r"
1622 + -- Normalise musical_key to the '<note> major' / '<note> minor' spelling.
1623 + --
1624 + -- detect_bpm_key stored stratum_dsp's compact DJ-style name ('Am', 'C#m', 'C')
1625 + -- straight through with no normalisation, while search::compatible_keys matches
1626 + -- on ends_with('minor') against a table of '<note> minor' strings and
1627 + -- analysis::suggest builds its tag by replacing a space that was never there.
1628 + -- Key-compatible search therefore matched nothing and the tag came out as
1629 + -- 'key.am'. The detector now normalises at the boundary; this rewrites rows
1630 + -- written before that.
1631 + --
1632 + -- substr drops only the trailing minor marker; replace(.., 'm', '') would be
1633 + -- equivalent today only because no note name contains an 'm', which is not a
1634 + -- property worth depending on. Rows already canonical contain a space and are
1635 + -- excluded by the WHERE clause, so re-running is a no-op.
1636 + UPDATE audio_analysis
1637 + SET musical_key = CASE
1638 + WHEN musical_key LIKE '%m'
1639 + THEN substr(musical_key, 1, length(musical_key) - 1) || ' minor'
1640 + ELSE musical_key || ' major'
1641 + END
1642 + WHERE musical_key IS NOT NULL
1643 + AND musical_key NOT LIKE '% major'
1644 + AND musical_key NOT LIKE '% minor'
1645 + AND musical_key != '';
1646 + ";
1647 +
1621 1648 /// Register `hash_row_id(salt, key) -> TEXT` as a deterministic SQLite
1622 1649 /// function on the given connection. Used by the M018 sync triggers so the
1623 1650 /// `sync_changelog.row_id` field never carries cleartext content (tag strings,
@@ -1805,6 +1832,7 @@
1805 1832 MIGRATION_031,
1806 1833 MIGRATION_032,
1807 1834 MIGRATION_033,
1835 + MIGRATION_034,
1808 1836 ];
1809 1837
1810 1838 for (i, sql) in MIGRATIONS.iter().enumerate() {
@@ -1998,6 +2026,55 @@
1998 2026 mod tests {
1999 2027 use super::*;
2000 2028
2029 + #[test]
2030 + fn migration_034_normalises_legacy_key_spellings() {
2031 + let db = Database::open_in_memory().unwrap();
2032 + // Rows written before the detector normalised its output. Inserted
2033 + // post-migration and re-run explicitly, since an in-memory DB starts
2034 + // empty and the migration would otherwise have nothing to rewrite.
2035 + db.conn()
2036 + .execute_batch(
2037 + "INSERT INTO samples
2038 + (hash, original_name, file_extension, file_size, import_date, last_modified)
2039 + VALUES ('a', 'a.wav', 'wav', 1, 0, 0), ('b', 'b.wav', 'wav', 1, 0, 0),
2040 + ('c', 'c.wav', 'wav', 1, 0, 0), ('d', 'd.wav', 'wav', 1, 0, 0);
2041 + INSERT INTO audio_analysis
2042 + (hash, musical_key, duration, sample_rate, channels, analyzed_at)
2043 + VALUES ('a', 'Am', 1.0, 44100, 2, 0), ('b', 'C#m', 1.0, 44100, 2, 0),
2044 + ('c', 'F#', 1.0, 44100, 2, 0), ('d', 'A minor', 1.0, 44100, 2, 0);",
2045 + )
2046 + .unwrap();
2047 + db.conn().execute_batch(MIGRATION_034).unwrap();
2048 +
2049 + let mut stmt = db
2050 + .conn()
2051 + .prepare("SELECT hash, musical_key FROM audio_analysis ORDER BY hash")
2052 + .unwrap();
2053 + let got: Vec<(String, String)> = stmt
2054 + .query_map([], |r| Ok((r.get(0)?, r.get(1)?)))
2055 + .unwrap()
2056 + .map(Result::unwrap)
2057 + .collect();
2058 +
2059 + assert_eq!(got[0].1, "A minor");
2060 + assert_eq!(got[1].1, "C# minor");
2061 + assert_eq!(got[2].1, "F# major");
2062 + // Already canonical, must not be rewritten to "A minor major".
2063 + assert_eq!(got[3].1, "A minor");
2064 +
2065 + // Idempotent: a second application changes nothing.
2066 + db.conn().execute_batch(MIGRATION_034).unwrap();
2067 + let after: String = db
2068 + .conn()
2069 + .query_row(
2070 + "SELECT musical_key FROM audio_analysis WHERE hash = 'a'",
2071 + [],
2072 + |r| r.get(0),
2073 + )
2074 + .unwrap();
2075 + assert_eq!(after, "A minor");
2076 + }
2077 +
2001 2078 #[test]
2002 2079 fn file_db_applies_performance_pragmas() {
2003 2080 let dir = tempfile::tempdir().unwrap();
@@ -2073,7 +2150,7 @@
2073 2150 .conn()
2074 2151 .query_row("PRAGMA user_version", [], |row| row.get(0))
2075 2152 .unwrap();
2076 - assert_eq!(version, 33);
2153 + assert_eq!(version, 34);
2077 2154 }
2078 2155
2079 2156 #[test]
@@ -2084,7 +2161,7 @@
2084 2161 .conn()
2085 2162 .query_row("PRAGMA user_version", [], |row| row.get(0))
2086 2163 .unwrap();
2087 - assert_eq!(version, 33);
2164 + assert_eq!(version, 34);
2088 2165 }
2089 2166
2090 2167 #[test]
@@ -2233,7 +2310,7 @@
2233 2310 .conn()
2234 2311 .query_row("PRAGMA user_version", [], |row| row.get(0))
2235 2312 .unwrap();
2236 - assert_eq!(version, 33);
2313 + assert_eq!(version, 34);
2237 2314 }
2238 2315
2239 2316 /// Simulates the worst-case recovery path: a prior partial migration left
@@ -2277,7 +2354,7 @@
2277 2354 .conn()
2278 2355 .query_row("PRAGMA user_version", [], |row| row.get(0))
2279 2356 .unwrap();
2280 - assert_eq!(version, 33);
2357 + assert_eq!(version, 34);
2281 2358 }
2282 2359
2283 2360 /// M018 contract: the `sync_changelog.row_id` for sensitive tables must
@@ -2507,7 +2584,7 @@
2507 2584 let initial_version: i32 = conn
2508 2585 .query_row("PRAGMA user_version", [], |row| row.get(0))
2509 2586 .unwrap();
2510 - assert_eq!(initial_version, 33);
2587 + assert_eq!(initial_version, 34);
2511 2588
2512 2589 let batch = format!("BEGIN;\n{bad_sql}\nPRAGMA user_version = 999;\nCOMMIT;");
2513 2590 let first_err = conn.execute_batch(&batch).unwrap_err();
@@ -2572,7 +2649,7 @@
2572 2649 .conn()
2573 2650 .query_row("PRAGMA user_version", [], |row| row.get(0))
2574 2651 .unwrap();
2575 - assert_eq!(version, 33);
2652 + assert_eq!(version, 34);
2576 2653 }
2577 2654
2578 2655 #[test]
M crates/audiofiles-core/src/rename.rs +36 -3
@@ -4,6 +4,24 @@
4 4
5 5 use crate::error::{CoreError, Result};
6 6
7 + /// Render a stored key in the compact form used in filenames.
8 + ///
9 + /// Keys are stored canonically as "A minor" / "C major" so that
10 + /// `search::compatible_keys` and the tag builder have one spelling to parse.
11 + /// A filename wants "Am", not "A minor" -- a space in the middle of a generated
12 + /// name is worse than the abbreviation, and "Am" is what the rename token
13 + /// produced before keys were normalised, so this keeps existing patterns
14 + /// yielding the same names.
15 + fn compact_key(key: &str) -> String {
16 + match key.rsplit_once(' ') {
17 + Some((note, "minor")) => format!("{note}m"),
18 + Some((note, "major")) => note.to_string(),
19 + // Anything else (already compact, or an unexpected spelling) passes
20 + // through rather than being mangled.
21 + _ => key.to_string(),
22 + }
23 + }
24 +
7 25 /// Context values used to resolve tokens in a rename pattern.
8 26 pub struct RenameContext {
9 27 pub name: String,
@@ -106,7 +124,11 @@
106 124 .bpm
107 125 .map(|b| format!("{}", b.round() as i64))
108 126 .unwrap_or_default(),
109 - Token::Key => ctx.musical_key.clone().unwrap_or_default(),
127 + Token::Key => ctx
128 + .musical_key
129 + .as_deref()
130 + .map(compact_key)
131 + .unwrap_or_default(),
110 132 Token::Class => ctx
111 133 .classification
112 134 .as_ref()
@@ -194,7 +216,8 @@
194 216 name: name.to_string(),
195 217 extension: "wav".to_string(),
196 218 bpm: Some(120.5),
197 - musical_key: Some("Cm".to_string()),
219 + // Canonical stored spelling; {key} renders it compactly.
220 + musical_key: Some("C minor".to_string()),
198 221 classification: Some("Kick".to_string()),
199 222 duration: Some(1.234),
200 223 index,
@@ -216,6 +239,16 @@
216 239 assert_eq!(result, "005_hit_121_Cm_kick_1.2s_wav");
217 240 }
218 241
242 + #[test]
243 + fn key_token_renders_compactly() {
244 + assert_eq!(compact_key("A minor"), "Am");
245 + assert_eq!(compact_key("C# minor"), "C#m");
246 + assert_eq!(compact_key("C major"), "C");
247 + assert_eq!(compact_key("F# major"), "F#");
248 + // Pre-normalisation values pass through rather than being mangled.
249 + assert_eq!(compact_key("Am"), "Am");
250 + }
251 +
219 252 #[test]
220 253 fn reject_path_separators() {
221 254 assert!(RenamePattern::parse("foo/{name}").is_err());
@@ -254,7 +287,7 @@
254 287 name: "sample".to_string(),
255 288 extension: "wav".to_string(),
256 289 bpm: None,
257 - musical_key: Some("Am".to_string()),
290 + musical_key: Some("A minor".to_string()),
258 291 classification: None,
259 292 duration: None,
260 293 index: 0,
M crates/audiofiles-core/src/analysis/bpm.rs +72 -1
@@ -10,6 +10,32 @@
10 10 pub key_confidence: Option<f32>,
11 11 }
12 12
13 + /// Convert a `stratum_dsp` key name to the canonical stored spelling.
14 + ///
15 + /// stratum emits a compact DJ-style name: "C" and "F#" for major, "Am" and
16 + /// "C#m" for minor. Everything downstream expects `"<note> major"` /
17 + /// `"<note> minor"`: [`crate::search::compatible_keys`] matches on
18 + /// `ends_with("minor")` against a table written that way, and
19 + /// `analysis::suggest` builds its `key.a-minor` tag by lowercasing and
20 + /// replacing the space. Storing the compact form made both silently no-op --
21 + /// key-compatible search matched nothing and the tag came out as `key.am`.
22 + ///
23 + /// Normalising here rather than at each consumer keeps one spelling in the
24 + /// database and one place to change it.
25 + pub(crate) fn canonical_key(name: &str) -> Option<String> {
26 + let name = name.trim();
27 + // No natural or sharp note name ends in 'm', so a trailing 'm' is
28 + // unambiguously the minor marker.
29 + let (note, mode) = match name.strip_suffix('m') {
30 + Some(note) => (note, "minor"),
31 + None => (name, "major"),
32 + };
33 + if note.is_empty() {
34 + return None;
35 + }
36 + Some(format!("{note} {mode}"))
37 + }
38 +
13 39 /// Detect BPM and musical key using stratum-dsp's `analyze_audio`.
14 40 ///
15 41 /// Skips samples shorter than `min_duration` seconds (caller typically passes 2.0)
@@ -61,7 +87,7 @@
61 87 if name.is_empty() || name == "Unknown" {
62 88 None
63 89 } else {
64 - Some(name)
90 + canonical_key(&name)
65 91 }
66 92 };
67 93
@@ -79,6 +105,51 @@
79 105 mod tests {
80 106 use super::*;
81 107
108 + #[test]
109 + fn canonical_key_converts_stratum_spelling() {
110 + // Exactly the strings stratum_dsp::Key::name() produces.
111 + assert_eq!(canonical_key("C").as_deref(), Some("C major"));
112 + assert_eq!(canonical_key("F#").as_deref(), Some("F# major"));
113 + assert_eq!(canonical_key("Am").as_deref(), Some("A minor"));
114 + assert_eq!(canonical_key("C#m").as_deref(), Some("C# minor"));
115 + assert_eq!(canonical_key(""), None);
116 + }
117 +
118 + #[test]
119 + fn detector_emits_canonical_key_format() {
120 + // The gap that let the format mismatch ship: every other test builds a
121 + // musical_key by hand, so nothing asserted against what the detector
122 + // actually returns. This drives the real detector and checks the
123 + // spelling contract rather than a specific key, which would be brittle.
124 + let sr = 44100;
125 + let mut samples = Vec::with_capacity(sr as usize * 4);
126 + // A minor triad (A3/C4/E4) so there is real tonal content to analyse.
127 + for i in 0..sr * 4 {
128 + let t = f64::from(i) / f64::from(sr);
129 + let v = (t * 220.0 * std::f64::consts::TAU).sin()
130 + + (t * 261.63 * std::f64::consts::TAU).sin()
131 + + (t * 329.63 * std::f64::consts::TAU).sin();
132 + samples.push((v / 3.0) as f32);
133 + }
134 +
135 + let result = detect_bpm_key(&samples, sr, 2.0);
136 + // Asserted rather than `if let`: a conditional check would pass
137 + // vacuously the day the detector stops returning keys, which is exactly
138 + // the regression this test exists to catch.
139 + let key = result
140 + .key
141 + .expect("detector returned no key for a sustained triad");
142 + assert!(
143 + key.ends_with(" major") || key.ends_with(" minor"),
144 + "detector returned {key:?}, which search::compatible_keys and \
145 + analysis::suggest cannot parse"
146 + );
147 + assert!(
148 + crate::search::compatible_keys(&key).len() > 1,
149 + "compatible_keys returned nothing usable for {key:?}"
150 + );
151 + }
152 +
82 153 #[test]
83 154 fn short_sample_returns_none() {
84 155 let short = vec![0.0f32; 44100]; // 1 second