From 3a523c3c8ac1bf9094dbe65a6f53b71085438c0c Mon Sep 17 00:00:00 2001 From: main Date: Fri, 20 Mar 2026 23:31:29 -0400 Subject: Open the metric unit vocabulary --- crates/fidget-spinner-cli/src/main.rs | 27 +--- crates/fidget-spinner-cli/src/mcp/catalog.rs | 5 +- crates/fidget-spinner-cli/src/ui.rs | 38 ++++-- crates/fidget-spinner-cli/tests/mcp_hardening.rs | 58 ++++++++ crates/fidget-spinner-core/src/error.rs | 6 + crates/fidget-spinner-core/src/lib.rs | 6 +- crates/fidget-spinner-core/src/model.rs | 164 +++++++++++++++++++++-- crates/fidget-spinner-store-sqlite/src/lib.rs | 19 +-- 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)] @@ -581,15 +581,6 @@ struct SkillShowArgs { name: Option, } -#[derive(Clone, Copy, Debug, Eq, PartialEq, ValueEnum)] -enum CliMetricUnit { - Seconds, - Bytes, - Count, - Ratio, - Custom, -} - #[derive(Clone, Copy, Debug, Eq, PartialEq, ValueEnum)] enum CliOptimizationObjective { Minimize, @@ -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 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 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 @@ -453,6 +453,64 @@ fn registry_and_history_surfaces_render_timestamps_as_strings() -> TestResult { Ok(()) } +#[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(µseconds); + assert_eq!( + tool_content(µseconds)["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")?; 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 { + 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) -> Result { + 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::parse_alias(&self.0) + } + + #[must_use] + pub fn scalar() -> Self { + Self(KnownMetricUnit::Scalar.as_str().to_owned()) + } +} + +impl TryFrom for MetricUnit { + type Error = CoreError; + + fn try_from(value: String) -> Result { + Self::new(value) + } +} + +impl From 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 { + 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 { } fn parse_metric_unit(raw: &str) -> Result { - 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 { -- cgit v1.2.3