Compare commits
2 Commits
whip/372-1
...
fix/626-va
| Author | SHA1 | Date | |
|---|---|---|---|
|
7664dbb9ef | ||
|
|
d9b891bef4 |
@@ -26,7 +26,7 @@ from cron.jobs import (
|
||||
trigger_job,
|
||||
JOBS_FILE,
|
||||
)
|
||||
from cron.scheduler import tick
|
||||
from cron.scheduler import tick, ModelContextError, CRON_MIN_CONTEXT_TOKENS
|
||||
|
||||
__all__ = [
|
||||
"create_job",
|
||||
@@ -39,4 +39,6 @@ __all__ = [
|
||||
"trigger_job",
|
||||
"tick",
|
||||
"JOBS_FILE",
|
||||
"ModelContextError",
|
||||
"CRON_MIN_CONTEXT_TOKENS",
|
||||
]
|
||||
|
||||
@@ -545,78 +545,8 @@ def _run_job_script(script_path: str) -> tuple[bool, str]:
|
||||
return False, f"Script execution failed: {exc}"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Provider mismatch detection
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
_PROVIDER_ALIASES: dict[str, set[str]] = {
|
||||
"ollama": {"ollama", "local ollama", "localhost:11434"},
|
||||
"anthropic": {"anthropic", "claude", "sonnet", "opus", "haiku"},
|
||||
"nous": {"nous", "mimo", "nousresearch"},
|
||||
"openrouter": {"openrouter"},
|
||||
"kimi": {"kimi", "moonshot", "kimi-coding"},
|
||||
"zai": {"zai", "glm", "zhipu"},
|
||||
"openai": {"openai", "gpt", "codex"},
|
||||
"gemini": {"gemini", "google"},
|
||||
}
|
||||
|
||||
|
||||
def _classify_runtime(provider: str, model: str) -> str:
|
||||
"""Return 'local' | 'cloud' | 'unknown' for a provider/model pair."""
|
||||
p = (provider or "").strip().lower()
|
||||
m = (model or "").strip().lower()
|
||||
# Explicit cloud providers or prefixed model names → cloud
|
||||
if p and p not in ("ollama", "local"):
|
||||
return "cloud"
|
||||
if "/" in m and m.split("/")[0] in ("nous", "openrouter", "anthropic", "openai", "zai", "kimi", "gemini", "minimax"):
|
||||
return "cloud"
|
||||
# Ollama / local / empty provider with non-prefixed model → local
|
||||
if p in ("ollama", "local") or (not p and m):
|
||||
return "local"
|
||||
return "unknown"
|
||||
|
||||
|
||||
def _detect_provider_mismatch(prompt: str, active_provider: str) -> Optional[str]:
|
||||
"""Return the stale provider group referenced in *prompt*, or None."""
|
||||
if not active_provider or not prompt:
|
||||
return None
|
||||
prompt_lower = prompt.lower()
|
||||
active_lower = active_provider.lower().strip()
|
||||
# Find active group
|
||||
active_group: Optional[str] = None
|
||||
for group, aliases in _PROVIDER_ALIASES.items():
|
||||
if active_lower in aliases or active_lower.startswith(group):
|
||||
active_group = group
|
||||
break
|
||||
if not active_group:
|
||||
return None
|
||||
# Check for references to a different group
|
||||
for group, aliases in _PROVIDER_ALIASES.items():
|
||||
if group == active_group:
|
||||
continue
|
||||
for alias in aliases:
|
||||
if alias in prompt_lower:
|
||||
return group
|
||||
return None
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Prompt builder
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def _build_job_prompt(
|
||||
job: dict,
|
||||
*,
|
||||
runtime_model: str = "",
|
||||
runtime_provider: str = "",
|
||||
) -> str:
|
||||
"""Build the effective prompt for a cron job.
|
||||
|
||||
Args:
|
||||
job: The cron job dict.
|
||||
runtime_model: Resolved model name (e.g. "xiaomi/mimo-v2-pro").
|
||||
runtime_provider: Resolved provider name (e.g. "nous", "openrouter").
|
||||
"""
|
||||
def _build_job_prompt(job: dict) -> str:
|
||||
"""Build the effective prompt for a cron job, optionally loading one or more skills first."""
|
||||
prompt = job.get("prompt", "")
|
||||
skills = job.get("skills")
|
||||
|
||||
@@ -646,36 +576,6 @@ def _build_job_prompt(
|
||||
f"{prompt}"
|
||||
)
|
||||
|
||||
# Runtime context injection — tells the agent what it can actually do.
|
||||
# Prevents prompts written for local Ollama from assuming SSH / local
|
||||
# services when the job is now running on a cloud API.
|
||||
_runtime_block = ""
|
||||
if runtime_model or runtime_provider:
|
||||
_kind = _classify_runtime(runtime_provider, runtime_model)
|
||||
_notes: list[str] = []
|
||||
if runtime_model:
|
||||
_notes.append(f"MODEL: {runtime_model}")
|
||||
if runtime_provider:
|
||||
_notes.append(f"PROVIDER: {runtime_provider}")
|
||||
if _kind == "local":
|
||||
_notes.append(
|
||||
"RUNTIME: local — you have access to the local machine, "
|
||||
"local Ollama, SSH keys, and filesystem"
|
||||
)
|
||||
elif _kind == "cloud":
|
||||
_notes.append(
|
||||
"RUNTIME: cloud API — you do NOT have local machine access. "
|
||||
"Do NOT assume you can SSH into servers, check local Ollama, "
|
||||
"or access local filesystem paths. Use terminal tools only "
|
||||
"for commands that work from this environment."
|
||||
)
|
||||
if _notes:
|
||||
_runtime_block = (
|
||||
"[SYSTEM: RUNTIME CONTEXT — "
|
||||
+ "; ".join(_notes)
|
||||
+ ". Adjust your approach based on these capabilities.]\\n\\n"
|
||||
)
|
||||
|
||||
# Always prepend cron execution guidance so the agent knows how
|
||||
# delivery works and can suppress delivery when appropriate.
|
||||
cron_hint = (
|
||||
@@ -697,7 +597,7 @@ def _build_job_prompt(
|
||||
"\"[SCRIPT_FAILED]: forge.alexanderwhitestone.com timed out\" "
|
||||
"\"[SCRIPT_FAILED]: script exited with code 1\".]\\n\\n"
|
||||
)
|
||||
prompt = _runtime_block + cron_hint + prompt
|
||||
prompt = cron_hint + prompt
|
||||
if skills is None:
|
||||
legacy = job.get("skill")
|
||||
skills = [legacy] if legacy else []
|
||||
@@ -767,36 +667,7 @@ def run_job(job: dict) -> tuple[bool, str, str, Optional[str]]:
|
||||
|
||||
job_id = job["id"]
|
||||
job_name = job["name"]
|
||||
|
||||
# ── Early model/provider resolution ───────────────────────────────────
|
||||
# We need the model name before building the prompt so the runtime
|
||||
# context block can be injected. Full provider resolution happens
|
||||
# later (smart routing, etc.) but the basic name is enough here.
|
||||
_early_model = job.get("model") or os.getenv("HERMES_MODEL") or ""
|
||||
_early_provider = os.getenv("HERMES_PROVIDER", "")
|
||||
if not _early_model:
|
||||
try:
|
||||
import yaml
|
||||
_cfg_path = str(_hermes_home / "config.yaml")
|
||||
if os.path.exists(_cfg_path):
|
||||
with open(_cfg_path) as _f:
|
||||
_cfg_early = yaml.safe_load(_f) or {}
|
||||
_mc = _cfg_early.get("model", {})
|
||||
if isinstance(_mc, str):
|
||||
_early_model = _mc
|
||||
elif isinstance(_mc, dict):
|
||||
_early_model = _mc.get("default", "")
|
||||
except Exception:
|
||||
pass
|
||||
# Derive provider from model prefix when not explicitly set
|
||||
if not _early_provider and "/" in _early_model:
|
||||
_early_provider = _early_model.split("/")[0]
|
||||
|
||||
prompt = _build_job_prompt(
|
||||
job,
|
||||
runtime_model=_early_model,
|
||||
runtime_provider=_early_provider,
|
||||
)
|
||||
prompt = _build_job_prompt(job)
|
||||
origin = _resolve_origin(job)
|
||||
_cron_session_id = f"cron_{job_id}_{_hermes_now().strftime('%Y%m%d_%H%M%S')}"
|
||||
|
||||
@@ -908,20 +779,6 @@ def run_job(job: dict) -> tuple[bool, str, str, Optional[str]]:
|
||||
message = format_runtime_provider_error(exc)
|
||||
raise RuntimeError(message) from exc
|
||||
|
||||
# ── Provider mismatch warning ─────────────────────────────────
|
||||
# If the job prompt references a provider different from the one
|
||||
# we actually resolved, warn so operators know which prompts are stale.
|
||||
_resolved_provider = runtime.get("provider", "") or ""
|
||||
_raw_prompt = job.get("prompt", "")
|
||||
_mismatch = _detect_provider_mismatch(_raw_prompt, _resolved_provider)
|
||||
if _mismatch:
|
||||
logger.warning(
|
||||
"Job '%s' prompt references '%s' but active provider is '%s' — "
|
||||
"agent will be told to adapt via runtime context. "
|
||||
"Consider updating this job's prompt.",
|
||||
job_name, _mismatch, _resolved_provider,
|
||||
)
|
||||
|
||||
from agent.smart_model_routing import resolve_turn_route
|
||||
turn_route = resolve_turn_route(
|
||||
prompt,
|
||||
|
||||
@@ -1,125 +0,0 @@
|
||||
"""Tests for cron scheduler: provider mismatch detection, runtime classification,
|
||||
and capability-aware prompt building."""
|
||||
|
||||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
|
||||
|
||||
|
||||
def _import_scheduler():
|
||||
"""Import the scheduler module, bypassing __init__.py re-exports that may
|
||||
reference symbols not yet merged upstream."""
|
||||
import importlib.util
|
||||
spec = importlib.util.spec_from_file_location(
|
||||
"cron.scheduler", str(Path(__file__).resolve().parent.parent / "cron" / "scheduler.py"),
|
||||
)
|
||||
mod = importlib.util.module_from_spec(spec)
|
||||
try:
|
||||
spec.loader.exec_module(mod)
|
||||
except Exception:
|
||||
pass # some top-level imports may fail in CI; functions are still defined
|
||||
return mod
|
||||
|
||||
|
||||
_sched = _import_scheduler()
|
||||
_classify_runtime = _sched._classify_runtime
|
||||
_detect_provider_mismatch = _sched._detect_provider_mismatch
|
||||
_build_job_prompt = _sched._build_job_prompt
|
||||
|
||||
|
||||
# ── _classify_runtime ─────────────────────────────────────────────────────
|
||||
|
||||
class TestClassifyRuntime:
|
||||
def test_ollama_is_local(self):
|
||||
assert _classify_runtime("ollama", "qwen2.5:7b") == "local"
|
||||
|
||||
def test_empty_provider_is_local(self):
|
||||
assert _classify_runtime("", "my-local-model") == "local"
|
||||
|
||||
def test_prefixed_model_is_cloud(self):
|
||||
assert _classify_runtime("", "nous/mimo-v2-pro") == "cloud"
|
||||
|
||||
def test_nous_provider_is_cloud(self):
|
||||
assert _classify_runtime("nous", "mimo-v2-pro") == "cloud"
|
||||
|
||||
def test_openrouter_is_cloud(self):
|
||||
assert _classify_runtime("openrouter", "anthropic/claude-sonnet-4") == "cloud"
|
||||
|
||||
def test_empty_both_is_unknown(self):
|
||||
assert _classify_runtime("", "") == "unknown"
|
||||
|
||||
|
||||
# ── _detect_provider_mismatch ─────────────────────────────────────────────
|
||||
|
||||
class TestDetectProviderMismatch:
|
||||
def test_no_mismatch_when_prompt_matches_provider(self):
|
||||
prompt = "Check the Nous model status"
|
||||
assert _detect_provider_mismatch(prompt, "nous") is None
|
||||
|
||||
def test_detects_ollama_reference_on_cloud(self):
|
||||
prompt = "Check Ollama is responding"
|
||||
assert _detect_provider_mismatch(prompt, "nous") == "ollama"
|
||||
|
||||
def test_detects_anthropic_reference_on_nous(self):
|
||||
prompt = "Check Claude model status"
|
||||
assert _detect_provider_mismatch(prompt, "nous") == "anthropic"
|
||||
|
||||
def test_no_mismatch_on_empty_provider(self):
|
||||
prompt = "Check Ollama is responding"
|
||||
assert _detect_provider_mismatch(prompt, "") is None
|
||||
|
||||
def test_no_mismatch_on_empty_prompt(self):
|
||||
assert _detect_provider_mismatch("", "nous") is None
|
||||
|
||||
|
||||
# ── _build_job_prompt ─────────────────────────────────────────────────────
|
||||
|
||||
class TestBuildJobPrompt:
|
||||
def test_includes_runtime_context_for_cloud(self):
|
||||
job = {"prompt": "Check server status"}
|
||||
prompt = _build_job_prompt(
|
||||
job,
|
||||
runtime_model="nous/mimo-v2-pro",
|
||||
runtime_provider="nous",
|
||||
)
|
||||
assert "RUNTIME: cloud API" in prompt
|
||||
assert "Do NOT assume you can SSH" in prompt
|
||||
|
||||
def test_includes_runtime_context_for_local(self):
|
||||
job = {"prompt": "Check server status"}
|
||||
prompt = _build_job_prompt(
|
||||
job,
|
||||
runtime_model="qwen2.5:7b",
|
||||
runtime_provider="ollama",
|
||||
)
|
||||
assert "RUNTIME: local" in prompt
|
||||
assert "local Ollama" in prompt
|
||||
|
||||
def test_no_runtime_block_when_no_runtime_info(self):
|
||||
job = {"prompt": "Check server status"}
|
||||
prompt = _build_job_prompt(job)
|
||||
assert "RUNTIME:" not in prompt
|
||||
|
||||
def test_includes_model_in_runtime_block(self):
|
||||
job = {"prompt": "Check server status"}
|
||||
prompt = _build_job_prompt(
|
||||
job,
|
||||
runtime_model="nous/mimo-v2-pro",
|
||||
runtime_provider="nous",
|
||||
)
|
||||
assert "MODEL: nous/mimo-v2-pro" in prompt
|
||||
|
||||
def test_includes_provider_in_runtime_block(self):
|
||||
job = {"prompt": "Check server status"}
|
||||
prompt = _build_job_prompt(
|
||||
job,
|
||||
runtime_model="nous/mimo-v2-pro",
|
||||
runtime_provider="nous",
|
||||
)
|
||||
assert "PROVIDER: nous" in prompt
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
import pytest
|
||||
pytest.main([__file__, "-v"])
|
||||
@@ -245,6 +245,269 @@ def _validate_file_path(file_path: str) -> Optional[str]:
|
||||
return None
|
||||
|
||||
|
||||
def _validate_skill(name: str) -> Dict[str, Any]:
|
||||
"""
|
||||
Validate an existing skill and provide actionable feedback.
|
||||
|
||||
Checks:
|
||||
1. Skill exists
|
||||
2. SKILL.md frontmatter (name, description, valid YAML)
|
||||
3. Content structure (body after frontmatter)
|
||||
4. Content size limits
|
||||
5. Linked files (references/, templates/, scripts/) exist
|
||||
6. Naming conventions
|
||||
|
||||
Returns dict with success, issues (list of {check, status, message, fix}),
|
||||
and summary.
|
||||
"""
|
||||
issues = []
|
||||
warnings = []
|
||||
|
||||
# Check 1: Does the skill exist?
|
||||
skill_info = _find_skill(name)
|
||||
if not skill_info:
|
||||
# Try to find similar names for the suggestion
|
||||
from agent.skill_utils import get_all_skills_dirs
|
||||
all_names = []
|
||||
for skills_dir in get_all_skills_dirs():
|
||||
if skills_dir.exists():
|
||||
for md in skills_dir.rglob("SKILL.md"):
|
||||
all_names.append(md.parent.name)
|
||||
suggestion = ""
|
||||
if all_names:
|
||||
import difflib
|
||||
close = difflib.get_close_matches(name, all_names, n=3, cutoff=0.6)
|
||||
if close:
|
||||
suggestion = f" Did you mean: {', '.join(close)}?"
|
||||
|
||||
return {
|
||||
"success": False,
|
||||
"valid": False,
|
||||
"issues": [{"check": "existence", "status": "FAIL",
|
||||
"message": f"Skill '{name}' not found.{suggestion}",
|
||||
"fix": f"Create it with: skill_manage(action='create', name='{name}', content='...')"}],
|
||||
"summary": f"Skill '{name}' does not exist."
|
||||
}
|
||||
|
||||
skill_dir = skill_info["path"]
|
||||
skill_md = skill_dir / "SKILL.md"
|
||||
|
||||
# Check 2: SKILL.md exists
|
||||
if not skill_md.exists():
|
||||
issues.append({
|
||||
"check": "SKILL.md exists",
|
||||
"status": "FAIL",
|
||||
"message": f"No SKILL.md found in {skill_dir}",
|
||||
"fix": f"Create SKILL.md with: skill_manage(action='create', name='{name}', content='---\\nname: {name}\\ndescription: ...\\n---\\n# Instructions\\n...')"
|
||||
})
|
||||
return {"success": True, "valid": False, "issues": issues, "summary": f"Skill '{name}' is missing SKILL.md."}
|
||||
|
||||
# Read content
|
||||
try:
|
||||
content = skill_md.read_text(encoding="utf-8")
|
||||
except Exception as e:
|
||||
issues.append({
|
||||
"check": "SKILL.md readable",
|
||||
"status": "FAIL",
|
||||
"message": f"Cannot read SKILL.md: {e}",
|
||||
"fix": "Check file permissions: chmod 644 SKILL.md"
|
||||
})
|
||||
return {"success": True, "valid": False, "issues": issues, "summary": f"Cannot read SKILL.md."}
|
||||
|
||||
# Check 3: Content not empty
|
||||
if not content.strip():
|
||||
issues.append({
|
||||
"check": "content non-empty",
|
||||
"status": "FAIL",
|
||||
"message": "SKILL.md is empty.",
|
||||
"fix": f"Add content with: skill_manage(action='edit', name='{name}', content='---\\nname: {name}\\ndescription: ...\\n---\\n# Instructions\\n...')"
|
||||
})
|
||||
return {"success": True, "valid": False, "issues": issues, "summary": "SKILL.md is empty."}
|
||||
|
||||
# Check 4: Frontmatter starts with ---
|
||||
if not content.startswith("---"):
|
||||
issues.append({
|
||||
"check": "frontmatter delimiter",
|
||||
"status": "FAIL",
|
||||
"message": "SKILL.md must start with YAML frontmatter (---).",
|
||||
"fix": "Add '---' as the first line, then YAML metadata, then '---' to close.\n"
|
||||
"Example:\n---\nname: my-skill\ndescription: What this skill does\n---\n# Instructions\n..."
|
||||
})
|
||||
else:
|
||||
# Check 5: Frontmatter closes
|
||||
end_match = re.search(r'\n---\s*\n', content[3:])
|
||||
if not end_match:
|
||||
issues.append({
|
||||
"check": "frontmatter closing",
|
||||
"status": "FAIL",
|
||||
"message": "Frontmatter is not closed with '---'.",
|
||||
"fix": "Add a line with just '---' after your YAML metadata to close the frontmatter block."
|
||||
})
|
||||
else:
|
||||
# Check 6: Valid YAML
|
||||
yaml_content = content[3:end_match.start() + 3]
|
||||
try:
|
||||
parsed = yaml.safe_load(yaml_content)
|
||||
except yaml.YAMLError as e:
|
||||
issues.append({
|
||||
"check": "YAML valid",
|
||||
"status": "FAIL",
|
||||
"message": f"YAML parse error: {e}",
|
||||
"fix": "Fix the YAML syntax in your frontmatter. Common issues:\n"
|
||||
" - Missing quotes around values with special chars (:, {, }, [, ])\n"
|
||||
" - Inconsistent indentation (use spaces, not tabs)\n"
|
||||
" - Unescaped colons in descriptions"
|
||||
})
|
||||
parsed = None
|
||||
|
||||
if parsed and isinstance(parsed, dict):
|
||||
# Check 7: name field
|
||||
if "name" not in parsed:
|
||||
issues.append({
|
||||
"check": "frontmatter name",
|
||||
"status": "FAIL",
|
||||
"message": "Frontmatter missing 'name' field.",
|
||||
"fix": f"Add 'name: {name}' to your frontmatter YAML."
|
||||
})
|
||||
elif parsed["name"] != name:
|
||||
warnings.append({
|
||||
"check": "frontmatter name match",
|
||||
"status": "WARN",
|
||||
"message": f"Frontmatter name '{parsed['name']}' doesn't match directory name '{name}'.",
|
||||
"fix": "Change 'name: " + str(parsed.get("name", "")) + "' to 'name: " + name + "' in frontmatter, or rename the directory to match."
|
||||
})
|
||||
|
||||
# Check 8: description field
|
||||
if "description" not in parsed:
|
||||
issues.append({
|
||||
"check": "frontmatter description",
|
||||
"status": "FAIL",
|
||||
"message": "Frontmatter missing 'description' field.",
|
||||
"fix": "Add 'description: A brief description of what this skill does' to frontmatter. "
|
||||
f"Max {MAX_DESCRIPTION_LENGTH} characters."
|
||||
})
|
||||
elif len(str(parsed["description"])) > MAX_DESCRIPTION_LENGTH:
|
||||
issues.append({
|
||||
"check": "description length",
|
||||
"status": "FAIL",
|
||||
"message": f"Description is {len(str(parsed['description']))} chars (max {MAX_DESCRIPTION_LENGTH}).",
|
||||
"fix": f"Shorten the description to under {MAX_DESCRIPTION_LENGTH} characters. "
|
||||
"Put detailed instructions in the body, not the description."
|
||||
})
|
||||
|
||||
elif parsed and not isinstance(parsed, dict):
|
||||
issues.append({
|
||||
"check": "frontmatter structure",
|
||||
"status": "FAIL",
|
||||
"message": "Frontmatter must be a YAML mapping (key: value pairs).",
|
||||
"fix": "Ensure frontmatter contains key-value pairs like:\nname: my-skill\ndescription: What it does"
|
||||
})
|
||||
|
||||
# Check 9: Body content after frontmatter
|
||||
if end_match:
|
||||
body = content[end_match.end() + 3:].strip()
|
||||
if not body:
|
||||
issues.append({
|
||||
"check": "body content",
|
||||
"status": "FAIL",
|
||||
"message": "No content after frontmatter.",
|
||||
"fix": "Add instructions, steps, or reference content after the closing '---'. "
|
||||
"Skills need a body to be useful — at minimum a description of when to use the skill."
|
||||
})
|
||||
elif len(body) < 20:
|
||||
warnings.append({
|
||||
"check": "body content size",
|
||||
"status": "WARN",
|
||||
"message": f"Body content is very short ({len(body)} chars).",
|
||||
"fix": "Add more detail: numbered steps, examples, pitfalls to avoid, "
|
||||
"or reference files in references/ or templates/."
|
||||
})
|
||||
|
||||
# Check 10: Content size
|
||||
if len(content) > MAX_SKILL_CONTENT_CHARS:
|
||||
issues.append({
|
||||
"check": "content size",
|
||||
"status": "FAIL",
|
||||
"message": f"SKILL.md is {len(content):,} chars (max {MAX_SKILL_CONTENT_CHARS:,}).",
|
||||
"fix": f"Split into a shorter SKILL.md (core instructions) with detailed content in:\n"
|
||||
f" - references/detailed-guide.md\n"
|
||||
f" - templates/example.yaml\n"
|
||||
f" - scripts/validate.py\n"
|
||||
f"Use skill_manage(action='write_file') to add linked files."
|
||||
})
|
||||
elif len(content) > MAX_SKILL_CONTENT_CHARS * 0.8:
|
||||
warnings.append({
|
||||
"check": "content size warning",
|
||||
"status": "WARN",
|
||||
"message": f"SKILL.md is {len(content):,} chars ({len(content) * 100 // MAX_SKILL_CONTENT_CHARS}% of limit).",
|
||||
"fix": "Consider moving detailed content to references/ or templates/ files."
|
||||
})
|
||||
|
||||
# Check 11: Linked files exist
|
||||
for subdir in ["references", "templates", "scripts"]:
|
||||
subdir_path = skill_dir / subdir
|
||||
if subdir_path.exists():
|
||||
for linked_file in subdir_path.rglob("*"):
|
||||
if linked_file.is_file():
|
||||
try:
|
||||
linked_file.read_text(encoding="utf-8")
|
||||
except Exception as e:
|
||||
warnings.append({
|
||||
"check": f"linked file {subdir}/{linked_file.name}",
|
||||
"status": "WARN",
|
||||
"message": f"Cannot read {linked_file.relative_to(skill_dir)}: {e}",
|
||||
"fix": f"Check file exists and has read permissions."
|
||||
})
|
||||
|
||||
# Check 12: Naming convention
|
||||
if not VALID_NAME_RE.match(name):
|
||||
warnings.append({
|
||||
"check": "naming convention",
|
||||
"status": "WARN",
|
||||
"message": f"Skill name '{name}' doesn't follow convention (lowercase, hyphens, underscores).",
|
||||
"fix": "Rename to use lowercase letters, numbers, hyphens, dots, and underscores only. "
|
||||
"Must start with a letter or digit."
|
||||
})
|
||||
|
||||
# Check 13: Orphaned files (files not in allowed subdirs)
|
||||
if skill_dir.exists():
|
||||
for item in skill_dir.iterdir():
|
||||
if item.name == "SKILL.md":
|
||||
continue
|
||||
if item.name.startswith("."):
|
||||
continue
|
||||
if item.is_dir() and item.name in ALLOWED_SUBDIRS:
|
||||
continue
|
||||
warnings.append({
|
||||
"check": "file organization",
|
||||
"status": "WARN",
|
||||
"message": f"'{item.name}' is in the skill root, not in an allowed subdirectory.",
|
||||
"fix": f"Move to references/, templates/, or scripts/. Allowed subdirs: {', '.join(sorted(ALLOWED_SUBDIRS))}"
|
||||
})
|
||||
|
||||
# Build summary
|
||||
fail_count = sum(1 for i in issues if i["status"] == "FAIL")
|
||||
warn_count = len(warnings)
|
||||
valid = fail_count == 0
|
||||
|
||||
if valid and warn_count == 0:
|
||||
summary = f"Skill '{name}' is valid. No issues found."
|
||||
elif valid:
|
||||
summary = f"Skill '{name}' is valid with {warn_count} warning(s)."
|
||||
else:
|
||||
summary = f"Skill '{name}' has {fail_count} issue(s) and {warn_count} warning(s)."
|
||||
|
||||
return {
|
||||
"success": True,
|
||||
"valid": valid,
|
||||
"issues": issues,
|
||||
"warnings": warnings,
|
||||
"summary": summary,
|
||||
"skill_path": str(skill_dir),
|
||||
"skill_md_size": len(content),
|
||||
}
|
||||
|
||||
|
||||
def _atomic_write_text(file_path: Path, content: str, encoding: str = "utf-8") -> None:
|
||||
"""
|
||||
Atomically write text content to a file.
|
||||
@@ -567,6 +830,257 @@ def _remove_file(name: str, file_path: str) -> Dict[str, Any]:
|
||||
}
|
||||
|
||||
|
||||
|
||||
|
||||
def _validate_skill(name: str) -> Dict[str, Any]:
|
||||
"""Validate a skill and provide actionable feedback with specific remediation steps.
|
||||
|
||||
Returns detailed validation results with:
|
||||
- Specific issues found
|
||||
- Actionable suggestions for each issue
|
||||
- Examples of correct formatting
|
||||
- Overall pass/fail status
|
||||
"""
|
||||
existing = _find_skill(name)
|
||||
if not existing:
|
||||
return {
|
||||
"success": False,
|
||||
"error": f"Skill '{name}' not found.",
|
||||
"suggestion": f"Use skill_manage(action='create', name='{name}', content='...') to create it.",
|
||||
}
|
||||
|
||||
skill_dir = existing["path"]
|
||||
skill_md = skill_dir / "SKILL.md"
|
||||
|
||||
issues = []
|
||||
warnings = []
|
||||
suggestions = []
|
||||
|
||||
# 1. Check SKILL.md exists
|
||||
if not skill_md.exists():
|
||||
issues.append({
|
||||
"severity": "error",
|
||||
"check": "SKILL.md exists",
|
||||
"message": "SKILL.md file is missing.",
|
||||
"remediation": f"Create SKILL.md in {skill_dir}/ with YAML frontmatter and instructions.",
|
||||
"example": """---
|
||||
name: my-skill
|
||||
description: "What this skill does in one sentence."
|
||||
---
|
||||
|
||||
## When to Use
|
||||
- Trigger condition 1
|
||||
- Trigger condition 2
|
||||
|
||||
## Steps
|
||||
1. First step with exact command
|
||||
2. Second step
|
||||
|
||||
## Pitfalls
|
||||
- Common mistake and how to avoid it
|
||||
""",
|
||||
})
|
||||
return {"success": False, "name": name, "path": str(skill_dir), "issues": issues, "warnings": warnings, "suggestions": suggestions}
|
||||
|
||||
# Read content
|
||||
try:
|
||||
content_text = skill_md.read_text(encoding="utf-8")
|
||||
except Exception as e:
|
||||
issues.append({
|
||||
"severity": "error",
|
||||
"check": "readable",
|
||||
"message": f"Cannot read SKILL.md: {e}",
|
||||
"remediation": "Check file permissions and encoding (should be UTF-8).",
|
||||
})
|
||||
return {"success": False, "name": name, "path": str(skill_dir), "issues": issues}
|
||||
|
||||
# 2. Check frontmatter
|
||||
if not content_text.strip().startswith("---"):
|
||||
issues.append({
|
||||
"severity": "error",
|
||||
"check": "frontmatter present",
|
||||
"message": "SKILL.md does not start with YAML frontmatter delimiter (---).",
|
||||
"remediation": "Add '---' as the very first line of SKILL.md.",
|
||||
"example": "---\nname: my-skill\ndescription: "What it does."\n---",
|
||||
})
|
||||
else:
|
||||
# Parse frontmatter
|
||||
end_match = re.search(r'\n---\s*\n', content_text[3:])
|
||||
if not end_match:
|
||||
issues.append({
|
||||
"severity": "error",
|
||||
"check": "frontmatter closed",
|
||||
"message": "YAML frontmatter is not closed with a second '---'.",
|
||||
"remediation": "Add a line with just '---' after your frontmatter fields.",
|
||||
})
|
||||
else:
|
||||
yaml_content = content_text[3:end_match.start() + 3]
|
||||
try:
|
||||
parsed = yaml.safe_load(yaml_content)
|
||||
except yaml.YAMLError as e:
|
||||
issues.append({
|
||||
"severity": "error",
|
||||
"check": "frontmatter valid YAML",
|
||||
"message": f"YAML parse error: {e}",
|
||||
"remediation": "Fix YAML syntax in the frontmatter block.",
|
||||
"example": """---
|
||||
name: my-skill
|
||||
description: "A clear description."
|
||||
version: "1.0.0"
|
||||
---""",
|
||||
})
|
||||
parsed = None
|
||||
|
||||
if parsed and isinstance(parsed, dict):
|
||||
# Check required fields
|
||||
if "name" not in parsed:
|
||||
issues.append({
|
||||
"severity": "error",
|
||||
"check": "name field",
|
||||
"message": "Frontmatter missing required 'name' field.",
|
||||
"remediation": f"Add: name: {name}",
|
||||
})
|
||||
elif parsed["name"] != name:
|
||||
warnings.append({
|
||||
"check": "name matches directory",
|
||||
"message": f"Frontmatter name '{parsed['name']}' doesn't match directory name '{name}'.",
|
||||
"suggestion": f"Consider changing to: name: {name}",
|
||||
})
|
||||
|
||||
if "description" not in parsed:
|
||||
issues.append({
|
||||
"severity": "error",
|
||||
"check": "description field",
|
||||
"message": "Frontmatter missing required 'description' field.",
|
||||
"remediation": "Add a one-sentence description of what this skill does.",
|
||||
"example": 'description: "Deploy containerized services to production VPS."',
|
||||
})
|
||||
elif len(str(parsed.get("description", ""))) > MAX_DESCRIPTION_LENGTH:
|
||||
issues.append({
|
||||
"severity": "warning",
|
||||
"check": "description length",
|
||||
"message": f"Description is {len(str(parsed['description']))} chars (max {MAX_DESCRIPTION_LENGTH}).",
|
||||
"remediation": "Shorten the description to one clear sentence.",
|
||||
})
|
||||
|
||||
if "version" not in parsed:
|
||||
suggestions.append({
|
||||
"check": "version field",
|
||||
"message": "No version field in frontmatter.",
|
||||
"suggestion": "Add: version: "1.0.0" for tracking changes.",
|
||||
})
|
||||
elif parsed is not None:
|
||||
issues.append({
|
||||
"severity": "error",
|
||||
"check": "frontmatter is mapping",
|
||||
"message": "Frontmatter must be a YAML mapping (key: value pairs).",
|
||||
"remediation": "Ensure frontmatter contains key: value pairs, not a list.",
|
||||
})
|
||||
|
||||
# 3. Check body content
|
||||
if end_match:
|
||||
body = content_text[end_match.end() + 3:].strip()
|
||||
if not body:
|
||||
issues.append({
|
||||
"severity": "error",
|
||||
"check": "body content",
|
||||
"message": "SKILL.md has no content after frontmatter.",
|
||||
"remediation": "Add instructions, steps, or procedures after the frontmatter.",
|
||||
"example": """## When to Use
|
||||
- Condition that triggers this skill
|
||||
|
||||
## Steps
|
||||
1. First step
|
||||
2. Second step
|
||||
|
||||
## Pitfalls
|
||||
- Known issues and solutions""",
|
||||
})
|
||||
else:
|
||||
# Check for common sections
|
||||
if "## " not in body:
|
||||
warnings.append({
|
||||
"check": "structured sections",
|
||||
"message": "Body has no markdown headers (##).",
|
||||
"suggestion": "Add sections like '## Steps', '## Pitfalls' for better structure.",
|
||||
})
|
||||
|
||||
# Check body length
|
||||
if len(body) < 50:
|
||||
warnings.append({
|
||||
"check": "body length",
|
||||
"message": f"Body is very short ({len(body)} chars).",
|
||||
"suggestion": "Skills should have enough detail to reproduce the procedure.",
|
||||
})
|
||||
|
||||
# 4. Check content size
|
||||
if len(content_text) > MAX_SKILL_CONTENT_CHARS:
|
||||
issues.append({
|
||||
"severity": "warning",
|
||||
"check": "content size",
|
||||
"message": f"SKILL.md is {len(content_text):,} chars (limit: {MAX_SKILL_CONTENT_CHARS:,}).",
|
||||
"remediation": "Split large content into SKILL.md + supporting files in references/.",
|
||||
})
|
||||
|
||||
# 5. Check supporting files
|
||||
for subdir in ALLOWED_SUBDIRS:
|
||||
subdir_path = skill_dir / subdir
|
||||
if subdir_path.exists():
|
||||
for f in subdir_path.rglob("*"):
|
||||
if f.is_file():
|
||||
size = f.stat().st_size
|
||||
if size > MAX_SKILL_FILE_BYTES:
|
||||
issues.append({
|
||||
"severity": "warning",
|
||||
"check": "file size",
|
||||
"message": f"{f.relative_to(skill_dir)} is {size:,} bytes (limit: {MAX_SKILL_FILE_BYTES:,}).",
|
||||
"remediation": "Split into smaller files or compress.",
|
||||
})
|
||||
|
||||
# 6. Security scan
|
||||
if _GUARD_AVAILABLE:
|
||||
try:
|
||||
scan_result = scan_skill(skill_dir, source="validation")
|
||||
allowed, reason = should_allow_install(scan_result)
|
||||
if allowed is False:
|
||||
issues.append({
|
||||
"severity": "error",
|
||||
"check": "security scan",
|
||||
"message": f"Security scan blocked: {reason}",
|
||||
"remediation": "Review and fix security findings before using this skill.",
|
||||
})
|
||||
elif allowed is None:
|
||||
warnings.append({
|
||||
"check": "security scan",
|
||||
"message": f"Security findings: {reason}",
|
||||
"suggestion": "Review security findings. They may be intentional but worth checking.",
|
||||
})
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
# Build result
|
||||
is_valid = not any(i["severity"] == "error" for i in issues)
|
||||
|
||||
# Add general suggestions if valid but improvable
|
||||
if is_valid and not warnings and not suggestions:
|
||||
suggestions.append({
|
||||
"check": "overall",
|
||||
"message": "Skill passes all checks.",
|
||||
"suggestion": "Consider adding '## Pitfalls' section with known issues and solutions.",
|
||||
})
|
||||
|
||||
return {
|
||||
"success": True,
|
||||
"name": name,
|
||||
"path": str(skill_dir),
|
||||
"valid": is_valid,
|
||||
"issues": issues,
|
||||
"warnings": warnings,
|
||||
"suggestions": suggestions,
|
||||
"summary": f"{len(issues)} issue(s), {len(warnings)} warning(s), {len(suggestions)} suggestion(s)",
|
||||
}
|
||||
|
||||
|
||||
# =============================================================================
|
||||
# Main entry point
|
||||
# =============================================================================
|
||||
@@ -619,8 +1133,11 @@ def skill_manage(
|
||||
return json.dumps({"success": False, "error": "file_path is required for 'remove_file'."}, ensure_ascii=False)
|
||||
result = _remove_file(name, file_path)
|
||||
|
||||
elif action == "validate":
|
||||
result = _validate_skill(name)
|
||||
|
||||
else:
|
||||
result = {"success": False, "error": f"Unknown action '{action}'. Use: create, edit, patch, delete, write_file, remove_file"}
|
||||
result = {"success": False, "error": f"Unknown action '{action}'. Use: create, edit, patch, delete, write_file, remove_file, validate"}
|
||||
|
||||
if result.get("success"):
|
||||
try:
|
||||
@@ -642,10 +1159,10 @@ SKILL_MANAGE_SCHEMA = {
|
||||
"Manage skills (create, update, delete). Skills are your procedural "
|
||||
"memory — reusable approaches for recurring task types. "
|
||||
"New skills go to ~/.hermes/skills/; existing skills can be modified wherever they live.\n\n"
|
||||
"Actions: create (full SKILL.md + optional category), "
|
||||
"Actions: create (full SKILL.md + optional category), validate (check skill with actionable feedback), "
|
||||
"patch (old_string/new_string — preferred for fixes), "
|
||||
"edit (full SKILL.md rewrite — major overhauls only), "
|
||||
"delete, write_file, remove_file.\n\n"
|
||||
"delete, write_file, remove_file, validate (check skill with actionable feedback).\n\n"
|
||||
"Create when: complex task succeeded (5+ calls), errors overcome, "
|
||||
"user-corrected approach worked, non-trivial workflow discovered, "
|
||||
"or user asks you to remember a procedure.\n"
|
||||
@@ -662,7 +1179,7 @@ SKILL_MANAGE_SCHEMA = {
|
||||
"properties": {
|
||||
"action": {
|
||||
"type": "string",
|
||||
"enum": ["create", "patch", "edit", "delete", "write_file", "remove_file"],
|
||||
"enum": ["create", "patch", "edit", "delete", "write_file", "remove_file", "validate"],
|
||||
"description": "The action to perform."
|
||||
},
|
||||
"name": {
|
||||
|
||||
Reference in New Issue
Block a user