diff --git a/crates/fastskill-cli/src/commands/eval/scorecard/metrics.rs b/crates/fastskill-cli/src/commands/eval/scorecard/metrics.rs index 9af5f330..d44e3015 100644 --- a/crates/fastskill-cli/src/commands/eval/scorecard/metrics.rs +++ b/crates/fastskill-cli/src/commands/eval/scorecard/metrics.rs @@ -8,12 +8,12 @@ use crate::error::{CliError, CliResult}; use fastskill_evals::checks::load_checks_file; -use serde::Deserialize; +use serde::{Deserialize, Serialize}; use sha2::{Digest, Sha256}; use std::path::{Path, PathBuf}; /// What a metric measures, and the bar it has to clear. -#[derive(Debug, Deserialize)] +#[derive(Debug, Serialize, Deserialize)] #[serde(tag = "kind", rename_all = "snake_case")] pub enum MetricKind { /// Passing check results over observed check results, for the named check @@ -34,7 +34,7 @@ pub enum MetricKind { }, } -#[derive(Debug, Deserialize)] +#[derive(Debug, Serialize, Deserialize)] pub struct MetricSpec { pub name: String, /// Case-id patterns, `*` matching any run of characters. An empty list @@ -47,7 +47,7 @@ pub struct MetricSpec { } /// The metrics file as written. -#[derive(Debug, Deserialize)] +#[derive(Debug, Serialize, Deserialize)] pub struct MetricsFile { #[serde(rename = "metric", default)] pub metrics: Vec, @@ -105,13 +105,27 @@ pub fn load_metrics(path: &Path) -> CliResult { e )) })?; - let parsed: MetricsFile = toml::from_str(&text).map_err(|e| { + let parse_error = |e: toml::de::Error| { config_error(format!( "cannot parse metrics file '{}': {}", path.display(), e )) - })?; + }; + let parsed: MetricsFile = toml::from_str(&text).map_err(parse_error)?; + // Every optional key changes the question silently when misspelt: a lost + // `cases` gates every case, a lost `criterion` scores `overall`, and a lost + // `suites` leaves the benchmark unhashable. + let raw: toml::Value = toml::from_str(&text).map_err(parse_error)?; + let dropped = fastskill_core::core::unknown_keys::dropped_keys(&raw, &parsed, "metrics"); + if !dropped.is_empty() { + return Err(config_error( + fastskill_core::core::unknown_keys::unknown_keys_message( + &format!("metrics file '{}'", path.display()), + &dropped, + ), + )); + } if parsed.metrics.is_empty() { return Err(config_error(format!( "metrics file '{}' declares no [[metric]] entries", @@ -237,6 +251,40 @@ mod tests { assert!(err.contains("no [[metric]] entries"), "{err}"); } + #[test] + fn a_misspelt_metrics_key_is_refused() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("metrics.toml"); + std::fs::write( + &path, + "suite = [\"s\"]\n[[metric]]\nname = \"Judge\"\nkind = \"judge_score\"\n\ + judges = [\"j\"]\ncritera = \"tone\"\nmin_score = 0.5\n", + ) + .unwrap(); + let err = load_metrics(&path).unwrap_err().to_string(); + assert!(err.contains("EVAL_SCORECARD_CONFIG"), "{err}"); + assert!(err.contains("metrics.suite"), "{err}"); + assert!(err.contains("metrics.metric.0.critera"), "{err}"); + } + + #[test] + fn a_missing_metrics_file_is_a_config_error() { + let dir = tempfile::tempdir().unwrap(); + let err = load_metrics(&dir.path().join("metrics.toml")) + .unwrap_err() + .to_string(); + assert!(err.contains("cannot read metrics file"), "{err}"); + } + + #[test] + fn a_metrics_file_that_is_not_toml_is_a_config_error() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("metrics.toml"); + std::fs::write(&path, "[[metric]\nname = \"Judge\"\n").unwrap(); + let err = load_metrics(&path).unwrap_err().to_string(); + assert!(err.contains("cannot parse metrics file"), "{err}"); + } + #[test] fn metrics_parse_every_kind() { let dir = tempfile::tempdir().unwrap(); @@ -389,6 +437,19 @@ min_rate = 0.85 assert!(err.contains("suites/consultation"), "{err}"); } + #[test] + fn a_suite_without_its_checks_file_is_an_error() { + let dir = tempfile::tempdir().unwrap(); + let suite_dir = suite(dir.path(), "consultation", "Judge this."); + std::fs::remove_file(suite_dir.join("checks.toml")).unwrap(); + let path = metrics_with_suites(dir.path()); + let file = load_metrics(&path).unwrap(); + let err = benchmark_sha256(&path, &file.suites) + .unwrap_err() + .to_string(); + assert!(err.contains("has no checks.toml"), "{err}"); + } + #[test] fn a_judge_prompt_file_that_is_not_on_disk_is_an_error() { let dir = tempfile::tempdir().unwrap(); diff --git a/crates/fastskill-cli/tests/bundle_cli_test.rs b/crates/fastskill-cli/tests/bundle_cli_test.rs index 12451852..576c8f39 100644 --- a/crates/fastskill-cli/tests/bundle_cli_test.rs +++ b/crates/fastskill-cli/tests/bundle_cli_test.rs @@ -55,7 +55,7 @@ fn write_recipient(root: &Path) -> std::path::PathBuf { fs::create_dir_all(&storage).unwrap(); fs::write( root.join("skill-project.toml"), - "custom_root = \"keep\"\n\n[tool.fastskill]\nskills_directory = \".claude/skills\"\n\n[tool.fastskill.extension]\nflag = true\n\n[dependencies]\n", + "custom_root = \"keep\"\n\n[tool.fastskill]\nskills_directory = \".claude/skills\"\n\n[tool.other]\nflag = true\n\n[dependencies]\n", ) .unwrap(); storage @@ -537,10 +537,7 @@ fn adding_and_removing_an_unrelated_skill_preserves_bundle_and_extension_tables( let manifest = fs::read_to_string(recipient.path().join("skill-project.toml")).unwrap(); assert!(manifest.contains("[bundles.payments-team]"), "{manifest}"); assert!(manifest.contains("custom_root = \"keep\""), "{manifest}"); - assert!( - manifest.contains("[tool.fastskill.extension]"), - "{manifest}" - ); + assert!(manifest.contains("[tool.other]"), "{manifest}"); assert!(manifest.contains("flag = true"), "{manifest}"); } diff --git a/crates/fastskill-core/src/core/manifest.rs b/crates/fastskill-core/src/core/manifest.rs index 0aeff1ba..4653c8ce 100644 --- a/crates/fastskill-core/src/core/manifest.rs +++ b/crates/fastskill-core/src/core/manifest.rs @@ -595,6 +595,35 @@ impl SkillProjectToml { Self::from_toml_str(&content) } + /// Parse manifest content, then refuse unknown keys under `[tool.fastskill]`. + pub fn from_toml_str(content: &str) -> Result { + let project = Self::parse_any_schema(content)?; + project.reject_unknown_tool_keys(content)?; + Ok(project) + } + + /// `[tool.fastskill]` is FastSkill's own table, so a misspelt key there would + /// silently change behaviour; refuse it. The rest of the manifest stays + /// permissive: other tools' tables and sections this version does not model + /// are kept as they are. + fn reject_unknown_tool_keys(&self, content: &str) -> Result<(), ManifestError> { + let Some(config) = self.tool.as_ref().and_then(|tool| tool.fastskill.as_ref()) else { + return Ok(()); + }; + let raw: toml::Value = + toml::from_str(content).map_err(|error| ManifestError::Parse(error.to_string()))?; + let Some(raw) = raw.get("tool").and_then(|tool| tool.get("fastskill")) else { + return Ok(()); + }; + let dropped = crate::core::unknown_keys::dropped_keys(raw, config, "tool.fastskill"); + if dropped.is_empty() { + return Ok(()); + } + Err(ManifestError::Parse( + crate::core::unknown_keys::unknown_keys_message("skill-project.toml", &dropped), + )) + } + /// Parse manifest content, upgrading a legacy (pre-`Origin`) file in memory. /// /// Dispatch is driven by the `schema_version` key, read in a cheap first pass (the same @@ -614,7 +643,7 @@ impl SkillProjectToml { /// /// Nothing is written to disk here. The upgrade is persisted only when something saves /// the manifest for its own reasons — see [`SkillProjectToml::save_to_file`]. - pub fn from_toml_str(content: &str) -> Result { + fn parse_any_schema(content: &str) -> Result { /// First pass: read only `schema_version`, ignoring everything else. A legacy file /// does not parse as the current shape, so the version cannot be read from a full parse. #[derive(Deserialize)] diff --git a/crates/fastskill-core/src/core/manifest/tests.rs b/crates/fastskill-core/src/core/manifest/tests.rs index 662869a6..84ab8bdd 100644 --- a/crates/fastskill-core/src/core/manifest/tests.rs +++ b/crates/fastskill-core/src/core/manifest/tests.rs @@ -739,3 +739,36 @@ fn unserializable_local_paths_do_not_replace_existing_manifests() { )); assert_eq!(std::fs::read_to_string(&path).unwrap(), "original manifest"); } + +#[test] +fn unknown_keys_under_tool_fastskill_are_refused() { + let error = SkillProjectToml::from_toml_str( + "[dependencies]\n\n[tool.fastskill]\nskip_transitiv = true\n\n[tool.fastskill.eval]\nprompts = \"p.csv\"\ntrials = 3\n", + ) + .unwrap_err() + .to_string(); + assert!(error.contains("tool.fastskill.skip_transitiv"), "{error}"); + assert!(error.contains("tool.fastskill.eval.trials"), "{error}"); +} + +#[test] +fn unknown_keys_in_flattened_repository_entries_are_refused() { + let error = SkillProjectToml::from_toml_str( + "[dependencies]\n\n[[tool.fastskill.repositories]]\nname = \"team\"\ntype = \"git-marketplace\"\npriority = 0\nurl = \"https://example.test/skills.git\"\nbrnach = \"release\"\n", + ) + .unwrap_err() + .to_string(); + assert!( + error.contains("tool.fastskill.repositories.0.brnach"), + "{error}" + ); +} + +#[test] +fn tables_outside_tool_fastskill_stay_permissive() { + let project = SkillProjectToml::from_toml_str( + "future_key = 1\n[dependencies]\n\n[tool.other]\nanything = true\n\n[tool.fastskill]\nskills_directory = \".skills\"\nmanifests = {}\n\n[bundles.team]\nversion = \"1.0.0\"\n", + ) + .unwrap(); + assert!(project.tool.unwrap().fastskill.is_some()); +} diff --git a/crates/fastskill-core/src/core/mod.rs b/crates/fastskill-core/src/core/mod.rs index 9606caf7..90800958 100644 --- a/crates/fastskill-core/src/core/mod.rs +++ b/crates/fastskill-core/src/core/mod.rs @@ -42,6 +42,7 @@ pub mod skill_manager; pub(crate) mod skill_walk; pub mod sources; pub mod state_guard; +pub mod unknown_keys; pub mod update; pub mod validation; pub mod vector_index; diff --git a/crates/fastskill-core/src/core/project_state.rs b/crates/fastskill-core/src/core/project_state.rs index df751188..044a96ee 100644 --- a/crates/fastskill-core/src/core/project_state.rs +++ b/crates/fastskill-core/src/core/project_state.rs @@ -100,7 +100,7 @@ mod tests { let path = temporary.path().join("skill-project.toml"); std::fs::write( &path, - "[dependencies]\nold = \"1.0.0\"\n[bundles.team]\nversion = \"1.0.0\"\nartifact = \"team.zip\"\n[extension]\nvalue = \"keep\"\n[tool.fastskill]\nskills_directory = \"skills\"\n[tool.fastskill.extension]\nflag = true\n", + "[dependencies]\nold = \"1.0.0\"\n[bundles.team]\nversion = \"1.0.0\"\nartifact = \"team.zip\"\n[extension]\nvalue = \"keep\"\n[tool.fastskill]\nskills_directory = \"skills\"\n[tool.other]\nflag = true\n", ) .unwrap(); let mut project = SkillProjectToml::load_from_file(&path).unwrap(); @@ -117,7 +117,7 @@ mod tests { assert!(saved.contains("[extension]"), "{saved}"); assert!(saved.contains("value = \"keep\""), "{saved}"); assert!(saved.contains("[bundles.team]"), "{saved}"); - assert!(saved.contains("[tool.fastskill.extension]"), "{saved}"); + assert!(saved.contains("[tool.other]"), "{saved}"); assert!(saved.contains("flag = true"), "{saved}"); let value: toml::Value = toml::from_str(&saved).unwrap(); assert!(value["dependencies"].get("old").is_none()); diff --git a/crates/fastskill-core/src/core/unknown_keys.rs b/crates/fastskill-core/src/core/unknown_keys.rs new file mode 100644 index 00000000..74586479 --- /dev/null +++ b/crates/fastskill-core/src/core/unknown_keys.rs @@ -0,0 +1,111 @@ +//! Find configuration keys that parsed without error and then had no effect. +//! +//! serde skips unknown keys by default, and `deny_unknown_fields` cannot be +//! used on types with `#[serde(flatten)]` or on untagged and internally tagged +//! enums, which is where most of our configuration lives. Instead, serialize the +//! parsed value back and report every key the input has that the round trip +//! lost. That also catches keys dropped because an untagged enum picked a +//! variant that has no field for them. + +use serde::Serialize; + +/// Dotted paths (under `prefix`) of keys in `raw` that `parsed` does not keep. +/// +/// Array elements are addressed by index, as in `repositories.0.brnach`. An +/// empty table is not reported: fields such as `manifests = {}` are skipped on +/// serialization when empty and carry no setting either way. +pub fn dropped_keys(raw: &toml::Value, parsed: &T, prefix: &str) -> Vec { + let Ok(round_trip) = toml::Value::try_from(parsed) else { + return Vec::new(); + }; + let mut dropped = Vec::new(); + collect(raw, &round_trip, prefix, &mut dropped); + dropped +} + +fn collect(raw: &toml::Value, kept: &toml::Value, path: &str, dropped: &mut Vec) { + match (raw, kept) { + (toml::Value::Table(raw), toml::Value::Table(kept)) => { + for (key, value) in raw { + let child = format!("{path}.{key}"); + match kept.get(key) { + Some(kept) => collect(value, kept, &child, dropped), + None if value.as_table().is_some_and(|table| table.is_empty()) => {} + None => dropped.push(child), + } + } + } + (toml::Value::Array(raw), toml::Value::Array(kept)) if raw.len() == kept.len() => { + for (index, (raw, kept)) in raw.iter().zip(kept).enumerate() { + collect(raw, kept, &format!("{path}.{index}"), dropped); + } + } + _ => {} + } +} + +/// The error text for dropped keys in a file. +pub fn unknown_keys_message(file: &str, keys: &[String]) -> String { + format!( + "{file} has settings that FastSkill does not recognise: {}. Check the spelling, \ + or remove them if they are meant for a newer FastSkill.", + keys.join(", ") + ) +} + +#[cfg(test)] +#[allow(clippy::unwrap_used)] +mod tests { + use super::*; + use serde::{Deserialize, Serialize}; + + #[derive(Serialize, Deserialize)] + struct Outer { + #[serde(default)] + items: Vec, + #[serde(default)] + depth: u32, + } + + #[derive(Serialize, Deserialize)] + struct Item { + name: String, + #[serde(flatten)] + connection: Connection, + } + + #[derive(Serialize, Deserialize)] + #[serde(untagged)] + enum Connection { + Registry { index_url: String }, + Git { url: String }, + } + + fn dropped(text: &str) -> Vec { + let raw: toml::Value = toml::from_str(text).unwrap(); + let parsed: Outer = toml::from_str(text).unwrap(); + dropped_keys(&raw, &parsed, "root") + } + + #[test] + fn known_keys_and_defaults_are_not_reported() { + assert!(dropped("depth = 2\n[[items]]\nname = \"a\"\nurl = \"u\"\n").is_empty()); + assert!(dropped("").is_empty()); + } + + #[test] + fn misspelt_top_level_and_flattened_keys_are_reported() { + assert_eq!( + dropped("dpeth = 2\n[[items]]\nname = \"a\"\nurl = \"u\"\nbrnach = \"b\"\n"), + vec!["root.dpeth", "root.items.0.brnach"] + ); + } + + #[test] + fn keys_lost_to_an_untagged_variant_are_reported() { + assert_eq!( + dropped("[[items]]\nname = \"a\"\nindex_url = \"i\"\nurl = \"u\"\n"), + vec!["root.items.0.url"] + ); + } +} diff --git a/webdocs/skill-management/manifest-system.mdx b/webdocs/skill-management/manifest-system.mdx index ac3d1700..41c2a257 100644 --- a/webdocs/skill-management/manifest-system.mdx +++ b/webdocs/skill-management/manifest-system.mdx @@ -21,6 +21,10 @@ Use the [quickstart](/quickstart) to create this local source. `skill add` write dependency and installs it. Editing the manifest manually requires a subsequent `project install` to apply it. +FastSkill refuses a manifest with a key it does not recognise under +`[tool.fastskill]`, so a misspelt setting fails loudly instead of being ignored. +Other tables, such as `[tool.]`, are left alone and kept on save. + ## Manifest composition Reference a reusable team Manifest without copying its dependencies: