fix: reject unsupported skill fields
This commit is contained in:
parent
62ef89a163
commit
4ddfccee2d
|
|
@ -5968,6 +5968,102 @@ mod build_summary_prompt_tests {
|
||||||
assert!(!prompt.contains("resident summary marker"));
|
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 {
|
fn minimal_manifest() -> WorkerManifest {
|
||||||
let toml_str = r#"
|
let toml_str = r#"
|
||||||
[worker]
|
[worker]
|
||||||
|
|
|
||||||
|
|
@ -287,7 +287,19 @@ fn parse_skill_source(source: SkillSource) -> Result<ParsedSkill, Vec<SkillDiagn
|
||||||
));
|
));
|
||||||
return Err(diagnostics);
|
return Err(diagnostics);
|
||||||
};
|
};
|
||||||
let frontmatter = match serde_yaml::from_str::<SkillFrontmatter>(frontmatter) {
|
let frontmatter_value = match serde_yaml::from_str::<serde_yaml::Value>(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::<SkillFrontmatter>(frontmatter_value) {
|
||||||
Ok(frontmatter) => frontmatter,
|
Ok(frontmatter) => frontmatter,
|
||||||
Err(error) => {
|
Err(error) => {
|
||||||
diagnostics.push(SkillDiagnostic::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<SkillDiagnostic>,
|
||||||
|
) {
|
||||||
|
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(
|
fn parse_allowed_tools(
|
||||||
value: serde_yaml::Value,
|
value: serde_yaml::Value,
|
||||||
provenance: &SkillProvenance,
|
provenance: &SkillProvenance,
|
||||||
|
|
@ -599,6 +684,64 @@ mod tests {
|
||||||
assert!(diagnostics.iter().any(|d| d.code == "name_parent_mismatch"));
|
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::<Vec<_>>();
|
||||||
|
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]
|
#[test]
|
||||||
fn workspace_skill_overrides_builtin() {
|
fn workspace_skill_overrides_builtin() {
|
||||||
let tmp = tempfile::tempdir().unwrap();
|
let tmp = tempfile::tempdir().unwrap();
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue
Block a user