From 4ddfccee2d414a2dfe67a940906b505dd237132d Mon Sep 17 00:00:00 2001 From: Hare Date: Thu, 16 Jul 2026 09:06:45 +0900 Subject: [PATCH] fix: reject unsupported skill fields --- crates/worker/src/worker.rs | 96 +++++++++++++++++ crates/workspace-server/src/skills.rs | 145 +++++++++++++++++++++++++- 2 files changed, 240 insertions(+), 1 deletion(-) diff --git a/crates/worker/src/worker.rs b/crates/worker/src/worker.rs index 3b4f0d04..cd982986 100644 --- a/crates/worker/src/worker.rs +++ b/crates/worker/src/worker.rs @@ -5968,6 +5968,102 @@ mod build_summary_prompt_tests { assert!(!prompt.contains("resident summary marker")); } + #[test] + fn activate_skill_commits_and_appends_history_before_future_context_use() { + use std::io::{BufRead, BufReader, Write}; + use std::net::TcpListener; + use std::thread; + + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let addr = listener.local_addr().unwrap(); + let server = thread::spawn(move || { + let (mut stream, _) = listener.accept().unwrap(); + let mut reader = BufReader::new(stream.try_clone().unwrap()); + let mut request_line = String::new(); + reader.read_line(&mut request_line).unwrap(); + assert!( + request_line + .starts_with("GET /api/w/ws-skill/skills/triage-errors/activate HTTP/1.1") + ); + loop { + let mut line = String::new(); + reader.read_line(&mut line).unwrap(); + if line == "\r\n" || line.is_empty() { + break; + } + } + let body = serde_json::json!({ + "name": "triage-errors", + "provenance": { "kind": "workspace", "id": "workspace:triage-errors" }, + "diagnostics": [], + "body": "---\nname: triage-errors\ndescription: Use when testing activation history.\n---\n\n# Triage Errors\n\nCommitted Skill body." + }) + .to_string(); + write!( + stream, + "HTTP/1.1 200 OK\r\ncontent-type: application/json\r\ncontent-length: {}\r\n\r\n{}", + body.len(), + body + ) + .unwrap(); + }); + + let dir = tempfile::tempdir().unwrap(); + let manifest = minimal_manifest(); + let store = session_store::FsStore::new(dir.path().join("sessions")).unwrap(); + let cwd = dir.path().join("workspace"); + std::fs::create_dir_all(&cwd).unwrap(); + let scope = Scope::writable(&cwd).unwrap(); + let authority = WorkerFilesystemAuthority::local(cwd.clone(), cwd.clone()); + let mut worker = tokio::runtime::Runtime::new() + .unwrap() + .block_on(Worker::new( + manifest, + Engine::new(NoopClient), + store, + WorkerWorkspaceContext::with_client( + Some(WorkspaceId::new("ws-skill").unwrap()), + WorkspaceClient::http("ws-skill", format!("http://{addr}")), + ), + authority, + scope, + )) + .unwrap(); + + let activation = worker.activate_skill("triage-errors").unwrap(); + + assert_eq!(activation.name, "triage-errors"); + server.join().unwrap(); + let history = worker.history(); + assert_eq!(history.len(), 1); + let history_text = history[0].as_text().unwrap(); + assert!( + history_text + .contains("Agent Skill `triage-errors` activated from workspace:triage-errors") + ); + assert!(history_text.contains("# Triage Errors")); + assert!(history_text.contains("Committed Skill body.")); + + let entries = worker + .store + .read_all( + worker.segment_state.session_id(), + worker.segment_state.segment_id(), + ) + .unwrap(); + assert!(entries.iter().any(|entry| { + matches!( + entry, + LogEntry::SystemItem { + item: SystemItem::SkillActivation { name, body }, + .. + } if name == "triage-errors" + && body.contains("# Triage Errors") + && body == history_text + ) + })); + } + fn minimal_manifest() -> WorkerManifest { let toml_str = r#" [worker] diff --git a/crates/workspace-server/src/skills.rs b/crates/workspace-server/src/skills.rs index af4868ea..a55977ec 100644 --- a/crates/workspace-server/src/skills.rs +++ b/crates/workspace-server/src/skills.rs @@ -287,7 +287,19 @@ fn parse_skill_source(source: SkillSource) -> Result(frontmatter) { + let frontmatter_value = match serde_yaml::from_str::(frontmatter) { + Ok(value) => value, + Err(error) => { + diagnostics.push(SkillDiagnostic::error( + "invalid_frontmatter_yaml", + format!("SKILL.md frontmatter is invalid YAML: {error}"), + Some(provenance.id.clone()), + )); + return Err(diagnostics); + } + }; + diagnose_unsupported_frontmatter_keys(&frontmatter_value, &provenance, &mut diagnostics); + let frontmatter = match serde_yaml::from_value::(frontmatter_value) { Ok(frontmatter) => frontmatter, Err(error) => { diagnostics.push(SkillDiagnostic::error( @@ -442,6 +454,79 @@ fn validate_optional_string( } } +fn diagnose_unsupported_frontmatter_keys( + value: &serde_yaml::Value, + provenance: &SkillProvenance, + diagnostics: &mut Vec, +) { + let Some(mapping) = value.as_mapping() else { + diagnostics.push(SkillDiagnostic::error( + "invalid_frontmatter_shape", + "SKILL.md frontmatter must be a YAML mapping", + Some(provenance.id.clone()), + )); + return; + }; + + for key in mapping.keys() { + let Some(key) = key.as_str() else { + diagnostics.push(SkillDiagnostic::error( + "invalid_frontmatter_key", + "Skill frontmatter keys must be strings", + Some(provenance.id.clone()), + )); + continue; + }; + if !is_supported_frontmatter_key(key) { + let (code, message) = if is_workflow_projection_key(key) { + ( + "unsupported_workflow_frontmatter_field", + format!( + "Skill frontmatter field `{key}` is a removed Workflow projection/invocation field and is not accepted as Skill semantics" + ), + ) + } else { + ( + "unsupported_frontmatter_field", + format!( + "Skill frontmatter field `{key}` is not supported; supported fields are name, description, license, compatibility, metadata, and allowed-tools" + ), + ) + }; + diagnostics.push(SkillDiagnostic::error( + code, + message, + Some(provenance.id.clone()), + )); + } + } +} + +fn is_supported_frontmatter_key(key: &str) -> bool { + matches!( + key, + "name" | "description" | "license" | "compatibility" | "metadata" | "allowed-tools" + ) +} + +fn is_workflow_projection_key(key: &str) -> bool { + matches!( + key, + "model_invokation" + | "model_invocation" + | "user_invocable" + | "workflow" + | "workflow_record" + | "workflow_invoke" + | "invocation" + | "invocations" + | "graph" + | "nodes" + | "edges" + | "triggers" + ) +} + fn parse_allowed_tools( value: serde_yaml::Value, provenance: &SkillProvenance, @@ -599,6 +684,64 @@ mod tests { assert!(diagnostics.iter().any(|d| d.code == "name_parent_mismatch")); } + #[test] + fn workflow_projection_frontmatter_fields_are_rejected() { + let tmp = tempfile::tempdir().unwrap(); + write_skill( + tmp.path(), + "workflow-shaped", + "---\nname: workflow-shaped\ndescription: Use when proving workflow projection fields are rejected as Skills.\nmodel_invokation: old-typo\nuser_invocable: true\ngraph: {}\ninvocation:\n run: now\n---\n\n# Workflow Shaped", + ); + + let catalog = catalog(tmp.path()); + assert!( + catalog + .entries + .iter() + .all(|entry| entry.name != "workflow-shaped") + ); + let codes = catalog + .diagnostics + .iter() + .map(|diagnostic| diagnostic.code.as_str()) + .collect::>(); + assert!(codes.contains(&"unsupported_workflow_frontmatter_field")); + for unsupported in ["model_invokation", "user_invocable", "graph", "invocation"] { + assert!( + catalog.diagnostics.iter().any(|diagnostic| { + diagnostic.code == "unsupported_workflow_frontmatter_field" + && diagnostic.message.contains(unsupported) + }), + "missing unsupported-field diagnostic for {unsupported}" + ); + } + assert!(matches!( + detail(tmp.path(), "workflow-shaped"), + Err(SkillError::NotFound(_)) + )); + } + + #[test] + fn unknown_frontmatter_fields_are_rejected() { + let tmp = tempfile::tempdir().unwrap(); + write_skill( + tmp.path(), + "unknown-field", + "---\nname: unknown-field\ndescription: Use when proving unsupported Skill fields are rejected.\ncustom-authority: no\n---\n\n# Unknown", + ); + + let catalog = catalog(tmp.path()); + assert!( + catalog + .entries + .iter() + .all(|entry| entry.name != "unknown-field") + ); + assert!(catalog.diagnostics.iter().any(|diagnostic| diagnostic.code + == "unsupported_frontmatter_field" + && diagnostic.message.contains("custom-authority"))); + } + #[test] fn workspace_skill_overrides_builtin() { let tmp = tempfile::tempdir().unwrap();