Open the metric unit vocabulary

8 files changed, 256 insertions, 67 deletions

diff --git a/crates/fidget-spinner-cli/src/main.rs b/crates/fidget-spinner-cli/src/main.rs
index 2c026d1..7482794 100644
--- a/crates/fidget-spinner-cli/src/main.rs
+++ b/crates/fidget-spinner-cli/src/main.rs
@@ -493,8 +493,8 @@ struct MetricDefineArgs {
     project: ProjectArg,
     #[arg(long)]
     key: String,
-    #[arg(long, value_enum)]
-    unit: CliMetricUnit,
+    #[arg(long)]
+    unit: String,
     #[arg(long, value_enum)]
     objective: CliOptimizationObjective,
     #[arg(long, value_enum, default_value_t = CliMetricVisibility::Canonical)]
@@ -582,15 +582,6 @@ struct SkillShowArgs {
 }
 
 #[derive(Clone, Copy, Debug, Eq, PartialEq, ValueEnum)]
-enum CliMetricUnit {
-    Seconds,
-    Bytes,
-    Count,
-    Ratio,
-    Custom,
-}
-
-#[derive(Clone, Copy, Debug, Eq, PartialEq, ValueEnum)]
 enum CliOptimizationObjective {
     Minimize,
     Maximize,
@@ -995,7 +986,7 @@ fn run_metric_define(args: MetricDefineArgs) -> Result<(), StoreError> {
     let mut store = open_store(&args.project.project)?;
     print_json(&store.define_metric(DefineMetricRequest {
         key: NonEmptyText::new(args.key)?,
-        unit: args.unit.into(),
+        unit: MetricUnit::new(args.unit)?,
         objective: args.objective.into(),
         visibility: args.visibility.into(),
         description: args.description.map(NonEmptyText::new).transpose()?,
@@ -1398,18 +1389,6 @@ fn print_json(value: &impl Serialize) -> Result<(), StoreError> {
     Ok(())
 }
 
-impl From<CliMetricUnit> for MetricUnit {
-    fn from(value: CliMetricUnit) -> Self {
-        match value {
-            CliMetricUnit::Seconds => Self::Seconds,
-            CliMetricUnit::Bytes => Self::Bytes,
-            CliMetricUnit::Count => Self::Count,
-            CliMetricUnit::Ratio => Self::Ratio,
-            CliMetricUnit::Custom => Self::Custom,
-        }
-    }
-}
-
 impl From<CliOptimizationObjective> for OptimizationObjective {
     fn from(value: CliOptimizationObjective) -> Self {
         match value {
diff --git a/crates/fidget-spinner-cli/src/mcp/catalog.rs b/crates/fidget-spinner-cli/src/mcp/catalog.rs
index 9b486bc..d6c8171 100644
--- a/crates/fidget-spinner-cli/src/mcp/catalog.rs
+++ b/crates/fidget-spinner-cli/src/mcp/catalog.rs
@@ -601,9 +601,8 @@ fn tool_input_schema(name: &str) -> Value {
                 ("key", string_schema("Metric key.")),
                 (
                     "unit",
-                    enum_string_schema(
-                        &["seconds", "bytes", "count", "ratio", "custom"],
-                        "Metric unit.",
+                    string_schema(
+                        "Metric unit token. Builtins include `scalar`, `count`, `ratio`, `percent`, `bytes`, `nanoseconds`, `microseconds`, `milliseconds`, and `seconds`; custom lowercase ascii tokens are also allowed.",
                     ),
                 ),
                 (
diff --git a/crates/fidget-spinner-cli/src/ui.rs b/crates/fidget-spinner-cli/src/ui.rs
index 9a42411..0b05b29 100644
--- a/crates/fidget-spinner-cli/src/ui.rs
+++ b/crates/fidget-spinner-cli/src/ui.rs
@@ -10,7 +10,7 @@ use axum::routing::get;
 use camino::Utf8PathBuf;
 use fidget_spinner_core::{
     AttachmentTargetRef, ExperimentAnalysis, ExperimentOutcome, ExperimentStatus, FrontierRecord,
-    FrontierVerdict, MetricUnit, NonEmptyText, RunDimensionValue, Slug, VertexRef,
+    FrontierVerdict, KnownMetricUnit, MetricUnit, NonEmptyText, RunDimensionValue, Slug, VertexRef,
 };
 use fidget_spinner_store_sqlite::{
     ExperimentDetail, ExperimentSummary, FrontierMetricSeries, FrontierOpenProjection,
@@ -640,7 +640,7 @@ fn render_metric_series_section(
                                             (point.verdict.as_str())
                                         }
                                     }
-                                    td.nowrap { (format_metric_value(point.value, series.metric.unit)) }
+                                    td.nowrap { (format_metric_value(point.value, &series.metric.unit)) }
                                 }
                             }
                         }
@@ -1295,7 +1295,7 @@ fn render_metric_panel(
                     @for metric in metrics {
                         tr {
                             td { (metric.key) }
-                            td { (format_metric_value(metric.value, metric_unit_for(metric, outcome))) }
+                            td { (format_metric_value(metric.value, &metric_unit_for(metric, outcome))) }
                         }
                     }
                 }
@@ -1310,9 +1310,9 @@ fn metric_unit_for(
     outcome: &ExperimentOutcome,
 ) -> MetricUnit {
     if metric.key == outcome.primary_metric.key {
-        return MetricUnit::Custom;
+        return MetricUnit::scalar();
     }
-    MetricUnit::Custom
+    MetricUnit::scalar()
 }
 
 fn render_vertex_relation_sections(
@@ -1424,7 +1424,7 @@ fn render_experiment_card(experiment: &ExperimentSummary) -> Markup {
             div.meta-row {
                 span.metric-pill {
                     (metric.key) ": "
-                    (format_metric_value(metric.value, metric.unit))
+                    (format_metric_value(metric.value, &metric.unit))
                 }
             }
         }
@@ -1449,7 +1449,7 @@ fn render_experiment_summary_line(experiment: &ExperimentSummary) -> Markup {
         @if let Some(metric) = experiment.primary_metric.as_ref() {
             span.metric-pill {
                 (metric.key) ": "
-                (format_metric_value(metric.value, metric.unit))
+                (format_metric_value(metric.value, &metric.unit))
             }
         }
     }
@@ -1627,13 +1627,23 @@ fn render_dimension_value(value: &RunDimensionValue) -> String {
     }
 }
 
-fn format_metric_value(value: f64, unit: MetricUnit) -> String {
-    match unit {
-        MetricUnit::Bytes => format!("{} B", format_integerish(value)),
-        MetricUnit::Seconds => format!("{value:.3} s"),
-        MetricUnit::Count => format_integerish(value),
-        MetricUnit::Ratio => format!("{value:.4}"),
-        MetricUnit::Custom => format_float(value),
+fn format_metric_value(value: f64, unit: &MetricUnit) -> String {
+    match unit.known_kind() {
+        Some(KnownMetricUnit::Bytes) => format!("{} B", format_integerish(value)),
+        Some(KnownMetricUnit::Seconds) => format!("{value:.3} s"),
+        Some(KnownMetricUnit::Milliseconds) => format!("{value:.3} ms"),
+        Some(KnownMetricUnit::Microseconds) => format!("{} us", format_integerish(value)),
+        Some(KnownMetricUnit::Nanoseconds) => format!("{} ns", format_integerish(value)),
+        Some(KnownMetricUnit::Count) => format_integerish(value),
+        Some(KnownMetricUnit::Ratio) => format!("{value:.4}"),
+        Some(KnownMetricUnit::Percent) => format!("{value:.2}%"),
+        Some(KnownMetricUnit::Scalar) | None => {
+            if unit.as_str() == "scalar" {
+                format_float(value)
+            } else {
+                format!("{} {}", format_float(value), unit.as_str())
+            }
+        }
     }
 }
 
diff --git a/crates/fidget-spinner-cli/tests/mcp_hardening.rs b/crates/fidget-spinner-cli/tests/mcp_hardening.rs
index c086a45..86b6719 100644
--- a/crates/fidget-spinner-cli/tests/mcp_hardening.rs
+++ b/crates/fidget-spinner-cli/tests/mcp_hardening.rs
@@ -454,6 +454,64 @@ fn registry_and_history_surfaces_render_timestamps_as_strings() -> TestResult {
 }
 
 #[test]
+fn metric_define_accepts_builtin_and_custom_unit_tokens() -> TestResult {
+    let project_root = temp_project_root("metric_units")?;
+    init_project(&project_root)?;
+
+    let mut harness = McpHarness::spawn(Some(&project_root))?;
+    let _ = harness.initialize()?;
+    harness.notify_initialized()?;
+
+    let microseconds = harness.call_tool_full(
+        23,
+        "metric.define",
+        json!({
+            "key": "oracle_solve_wallclock_micros",
+            "unit": "micros",
+            "objective": "minimize",
+            "visibility": "canonical",
+        }),
+    )?;
+    assert_tool_ok(&microseconds);
+    assert_eq!(
+        tool_content(&microseconds)["record"]["unit"].as_str(),
+        Some("microseconds")
+    );
+
+    let custom = harness.call_tool_full(
+        24,
+        "metric.define",
+        json!({
+            "key": "root_lp_objective_last",
+            "unit": "objective",
+            "objective": "minimize",
+            "visibility": "canonical",
+        }),
+    )?;
+    assert_tool_ok(&custom);
+    assert_eq!(
+        tool_content(&custom)["record"]["unit"].as_str(),
+        Some("objective")
+    );
+
+    let placeholder = harness.call_tool(
+        25,
+        "metric.define",
+        json!({
+            "key": "bad_custom_placeholder",
+            "unit": "custom",
+            "objective": "minimize",
+        }),
+    )?;
+    assert_tool_error(&placeholder);
+    assert!(
+        must_some(tool_error_message(&placeholder), "metric unit error")?.contains("metric unit")
+    );
+
+    Ok(())
+}
+
+#[test]
 fn hypothesis_body_discipline_is_enforced_over_mcp() -> TestResult {
     let project_root = temp_project_root("single_paragraph")?;
     init_project(&project_root)?;
diff --git a/crates/fidget-spinner-core/src/error.rs b/crates/fidget-spinner-core/src/error.rs
index a095f57..67db8bd 100644
--- a/crates/fidget-spinner-core/src/error.rs
+++ b/crates/fidget-spinner-core/src/error.rs
@@ -4,6 +4,12 @@ use thiserror::Error;
 pub enum CoreError {
     #[error("text values must not be blank")]
     EmptyText,
+    #[error("metric units must not be blank")]
+    EmptyMetricUnit,
+    #[error(
+        "invalid metric unit `{0}`; expected a built-in unit like `microseconds` or a lowercase ascii token"
+    )]
+    InvalidMetricUnit(String),
     #[error("tag names must not be blank")]
     EmptyTagName,
     #[error(
diff --git a/crates/fidget-spinner-core/src/lib.rs b/crates/fidget-spinner-core/src/lib.rs
index 903e740..7089aa6 100644
--- a/crates/fidget-spinner-core/src/lib.rs
+++ b/crates/fidget-spinner-core/src/lib.rs
@@ -15,7 +15,7 @@ pub use crate::model::{
     ArtifactKind, ArtifactRecord, AttachmentTargetKind, AttachmentTargetRef, CommandRecipe,
     ExecutionBackend, ExperimentAnalysis, ExperimentOutcome, ExperimentRecord, ExperimentStatus,
     FieldValueType, FrontierBrief, FrontierRecord, FrontierRoadmapItem, FrontierStatus,
-    FrontierVerdict, HypothesisRecord, MetricDefinition, MetricUnit, MetricValue, MetricVisibility,
-    NonEmptyText, OptimizationObjective, RunDimensionDefinition, RunDimensionValue, Slug, TagName,
-    TagRecord, VertexKind, VertexRef,
+    FrontierVerdict, HypothesisRecord, KnownMetricUnit, MetricDefinition, MetricUnit, MetricValue,
+    MetricVisibility, NonEmptyText, OptimizationObjective, RunDimensionDefinition,
+    RunDimensionValue, Slug, TagName, TagRecord, VertexKind, VertexRef,
 };
diff --git a/crates/fidget-spinner-core/src/model.rs b/crates/fidget-spinner-core/src/model.rs
index cedd882..5f4bdeb 100644
--- a/crates/fidget-spinner-core/src/model.rs
+++ b/crates/fidget-spinner-core/src/model.rs
@@ -165,29 +165,129 @@ impl FrontierStatus {
     }
 }
 
-#[derive(Clone, Copy, Debug, Deserialize, Eq, Ord, PartialEq, PartialOrd, Serialize)]
-#[serde(rename_all = "snake_case")]
-pub enum MetricUnit {
-    Seconds,
-    Bytes,
+#[derive(Clone, Copy, Debug, Eq, Ord, PartialEq, PartialOrd)]
+pub enum KnownMetricUnit {
+    Scalar,
     Count,
     Ratio,
-    Custom,
+    Percent,
+    Bytes,
+    Nanoseconds,
+    Microseconds,
+    Milliseconds,
+    Seconds,
 }
 
-impl MetricUnit {
+impl KnownMetricUnit {
     #[must_use]
     pub const fn as_str(self) -> &'static str {
         match self {
-            Self::Seconds => "seconds",
-            Self::Bytes => "bytes",
+            Self::Scalar => "scalar",
             Self::Count => "count",
             Self::Ratio => "ratio",
-            Self::Custom => "custom",
+            Self::Percent => "percent",
+            Self::Bytes => "bytes",
+            Self::Nanoseconds => "nanoseconds",
+            Self::Microseconds => "microseconds",
+            Self::Milliseconds => "milliseconds",
+            Self::Seconds => "seconds",
+        }
+    }
+
+    fn parse_alias(raw: &str) -> Option<Self> {
+        match raw {
+            "1" | "scalar" | "unitless" | "dimensionless" => Some(Self::Scalar),
+            "count" | "counts" => Some(Self::Count),
+            "ratio" | "fraction" => Some(Self::Ratio),
+            "%" | "percent" | "percentage" | "pct" => Some(Self::Percent),
+            "bytes" | "byte" | "b" | "by" => Some(Self::Bytes),
+            "nanoseconds" | "nanosecond" | "ns" => Some(Self::Nanoseconds),
+            "microseconds" | "microsecond" | "us" | "µs" | "micros" => Some(Self::Microseconds),
+            "milliseconds" | "millisecond" | "ms" | "millis" => Some(Self::Milliseconds),
+            "seconds" | "second" | "s" | "sec" | "secs" => Some(Self::Seconds),
+            _ => None,
         }
     }
 }
 
+#[derive(Clone, Debug, Eq, Ord, PartialEq, PartialOrd, Serialize, Deserialize)]
+#[serde(try_from = "String", into = "String")]
+pub struct MetricUnit(String);
+
+impl MetricUnit {
+    pub fn new(value: impl Into<String>) -> Result<Self, CoreError> {
+        let raw = value.into();
+        let normalized = normalize_metric_unit(&raw)?;
+        Ok(Self(normalized))
+    }
+
+    #[must_use]
+    pub fn as_str(&self) -> &str {
+        &self.0
+    }
+
+    #[must_use]
+    pub fn known_kind(&self) -> Option<KnownMetricUnit> {
+        KnownMetricUnit::parse_alias(&self.0)
+    }
+
+    #[must_use]
+    pub fn scalar() -> Self {
+        Self(KnownMetricUnit::Scalar.as_str().to_owned())
+    }
+}
+
+impl TryFrom<String> for MetricUnit {
+    type Error = CoreError;
+
+    fn try_from(value: String) -> Result<Self, Self::Error> {
+        Self::new(value)
+    }
+}
+
+impl From<MetricUnit> for String {
+    fn from(value: MetricUnit) -> Self {
+        value.0
+    }
+}
+
+impl Display for MetricUnit {
+    fn fmt(&self, formatter: &mut Formatter<'_>) -> fmt::Result {
+        formatter.write_str(self.as_str())
+    }
+}
+
+fn normalize_metric_unit(raw: &str) -> Result<String, CoreError> {
+    let normalized = raw.trim().to_ascii_lowercase();
+    if normalized.is_empty() {
+        return Err(CoreError::EmptyMetricUnit);
+    }
+    if let Some(unit) = KnownMetricUnit::parse_alias(&normalized) {
+        return Ok(unit.as_str().to_owned());
+    }
+    if normalized == "custom" {
+        return Err(CoreError::InvalidMetricUnit(normalized));
+    }
+    let mut previous_was_separator = true;
+    let mut has_alphanumeric = false;
+    for character in normalized.chars() {
+        if character.is_ascii_lowercase() || character.is_ascii_digit() {
+            previous_was_separator = false;
+            has_alphanumeric = true;
+            continue;
+        }
+        if matches!(character, '-' | '_' | '/' | '.') && !previous_was_separator {
+            previous_was_separator = true;
+            continue;
+        }
+        return Err(CoreError::InvalidMetricUnit(normalized));
+    }
+    if !has_alphanumeric || previous_was_separator {
+        return Err(CoreError::InvalidMetricUnit(normalized));
+    }
+    Ok(normalized)
+}
+
 #[derive(Clone, Copy, Debug, Deserialize, Eq, Ord, PartialEq, PartialOrd, Serialize)]
 #[serde(rename_all = "snake_case")]
 pub enum OptimizationObjective {
@@ -358,6 +458,50 @@ impl RunDimensionDefinition {
     }
 }
 
+#[cfg(test)]
+mod tests {
+    use super::{KnownMetricUnit, MetricUnit};
+
+    #[test]
+    fn metric_unit_normalizes_known_aliases() {
+        let microseconds = MetricUnit::new("micros");
+        assert!(microseconds.is_ok());
+        let microseconds = match microseconds {
+            Ok(value) => value,
+            Err(_) => return,
+        };
+        assert_eq!(microseconds.as_str(), "microseconds");
+        assert_eq!(
+            microseconds.known_kind(),
+            Some(KnownMetricUnit::Microseconds)
+        );
+
+        let percent = MetricUnit::new("%");
+        assert!(percent.is_ok());
+        let percent = match percent {
+            Ok(value) => value,
+            Err(_) => return,
+        };
+        assert_eq!(percent.as_str(), "percent");
+        assert_eq!(percent.known_kind(), Some(KnownMetricUnit::Percent));
+    }
+
+    #[test]
+    fn metric_unit_accepts_real_custom_tokens_and_rejects_placeholder_custom() {
+        let objective = MetricUnit::new("objective");
+        assert!(objective.is_ok());
+        let objective = match objective {
+            Ok(value) => value,
+            Err(_) => return,
+        };
+        assert_eq!(objective.as_str(), "objective");
+        assert_eq!(objective.known_kind(), None);
+
+        let placeholder = MetricUnit::new("custom");
+        assert!(placeholder.is_err());
+    }
+}
+
 #[derive(Clone, Debug, Deserialize, PartialEq, Serialize)]
 pub struct MetricValue {
     pub key: NonEmptyText,
diff --git a/crates/fidget-spinner-store-sqlite/src/lib.rs b/crates/fidget-spinner-store-sqlite/src/lib.rs
index fbdbb32..253929e 100644
--- a/crates/fidget-spinner-store-sqlite/src/lib.rs
+++ b/crates/fidget-spinner-store-sqlite/src/lib.rs
@@ -3033,19 +3033,12 @@ fn parse_frontier_status(raw: &str) -> Result<FrontierStatus, rusqlite::Error> {
 }
 
 fn parse_metric_unit(raw: &str) -> Result<MetricUnit, rusqlite::Error> {
-    match raw {
-        "seconds" => Ok(MetricUnit::Seconds),
-        "bytes" => Ok(MetricUnit::Bytes),
-        "count" => Ok(MetricUnit::Count),
-        "ratio" => Ok(MetricUnit::Ratio),
-        "custom" => Ok(MetricUnit::Custom),
-        _ => Err(to_sql_conversion_error(StoreError::Json(
-            serde_json::Error::io(io::Error::new(
-                io::ErrorKind::InvalidData,
-                format!("invalid metric unit `{raw}`"),
-            )),
-        ))),
-    }
+    MetricUnit::new(raw).map_err(|error| {
+        to_sql_conversion_error(StoreError::Json(serde_json::Error::io(io::Error::new(
+            io::ErrorKind::InvalidData,
+            error.to_string(),
+        ))))
+    })
 }
 
 fn parse_optimization_objective(raw: &str) -> Result<OptimizationObjective, rusqlite::Error> {