fix(skills): parse optimizer metadata as YAML (#7049)

* fix(skills): parse optimizer metadata as YAML

* fix(skills): address optimizer review feedback

* fix(ci): retain only upstream preapproved waiver hashes

* fix(ci): retain reviewed subprocess location and separate authorization

---------

Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
This commit is contained in:
Winoren 2026-10-27 14:44:27 +08:00 committed by GitHub
parent e2e8ba7aec
commit fd2d1de424
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
8 changed files with 198 additions and 92 deletions

View File

@ -226,6 +226,8 @@ Repo-wide conventions:
- **Test-driven development** — features and bug fixes ship with tests. Backend tests live
in `backend/tests/` (TDD is mandatory there; see [backend/AGENTS.md](backend/AGENTS.md));
frontend tests live in `frontend/tests/`.
Skill-creator tests must preserve YAML values/body, restore bytecode settings,
and check invalid metadata through offline CLI subprocesses.
Public skill script regressions live in `tests/skills/` and run in Skill Review CI
independently of package review. Exercise dependency fallbacks with the optional
import unavailable and mock HTTP transport to keep tests offline.

View File

@ -1719,6 +1719,8 @@ validated or converted, and provider-specific format restrictions still apply.
The [music-generation skill](skills/public/music-generation/SKILL.md#output-handling) preserves existing output when the provider returns malformed, empty or unrecognized audio.
The bundled skill-creator description optimizer reads YAML frontmatter consistently with skill validation, preserving quoted strings and multiline descriptions. Its scripts require PyYAML, as does the skill package validator. Invalid skill metadata is reported on stderr with a nonzero exit status rather than a traceback.
Skills are loaded progressively — only when the task needs them, not all at once. This keeps the context window lean and makes DeerFlow work well even with token-sensitive models.
The bundled video-generation command reports failures on stderr and exits with

View File

@ -442,6 +442,8 @@ While it runs, periodically tail the output to give the user updates on which it
This handles the full optimization loop automatically. It splits the eval set into 60% train and 40% held-out test, evaluates the current description (running each query 3 times to get a reliable trigger rate), then calls Claude to propose improvements based on what failed. It re-evaluates each new description on both train and test, iterating up to 5 times. When it's done, it opens an HTML report in the browser showing the results per iteration and returns JSON with `best_description` — selected by test score rather than train score to avoid overfitting.
Description optimization requires PyYAML (also used by the package validator). Name and description fields are parsed as YAML strings, including escaped quotes, comments and block scalars. Invalid metadata stops the evaluator, optimizer or description-improvement command with an `Error:` on stderr and exit status 1.
### How skill triggering works
Understanding the triggering mechanism helps design better eval queries. Skills appear in Claude's `available_skills` list with their name + description, and Claude decides whether to consult a skill based on that description. The important thing to know is that Claude only consults skills for tasks it can't easily handle on its own — simple, one-step queries like "read this PDF" may not trigger a skill even if the description matches perfectly, because Claude can handle them directly with basic tools. Complex, multi-step, or specialized queries reliably trigger skills when the description matches.

View File

@ -41,9 +41,7 @@ def _call_claude(prompt: str, model: str | None, timeout: int = 300) -> str:
timeout=timeout,
)
if result.returncode != 0:
raise RuntimeError(
f"claude -p exited {result.returncode}\nstderr: {result.stderr}"
)
raise RuntimeError(f"claude -p exited {result.returncode}\nstderr: {result.stderr}")
return result.stdout
@ -59,14 +57,8 @@ def improve_description(
iteration: int | None = None,
) -> str:
"""Call Claude to improve the description based on eval results."""
failed_triggers = [
r for r in eval_results["results"]
if r["should_trigger"] and not r["pass"]
]
false_triggers = [
r for r in eval_results["results"]
if not r["should_trigger"] and not r["pass"]
]
failed_triggers = [r for r in eval_results["results"] if r["should_trigger"] and not r["pass"]]
false_triggers = [r for r in eval_results["results"] if not r["should_trigger"] and not r["pass"]]
# Build scores summary
train_score = f"{eval_results['summary']['passed']}/{eval_results['summary']['total']}"
@ -76,9 +68,12 @@ def improve_description(
else:
scores_summary = f"Train: {train_score}"
prompt = f"""You are optimizing a skill description for a Claude Code skill called "{skill_name}". A "skill" is sort of like a prompt, but with progressive disclosure -- there's a title and description that Claude sees when deciding whether to use the skill, and then if it does use the skill, it reads the .md file which has lots more details and potentially links to other resources in the skill folder like helper files and scripts and additional documentation or examples.
prompt = f"""You are optimizing a skill description for a Claude Code skill called "{skill_name}". A "skill" is sort of like a prompt, but with progressive disclosure -- there's a title and description that Claude \
sees when deciding whether to use the skill, and then if it does use the skill, it reads the .md file which has lots more details and potentially links to other resources in the skill folder like helper files and \
scripts and additional documentation or examples.
The description appears in Claude's "available_skills" list. When a user sends a query, Claude decides whether to invoke the skill based solely on the title and on this description. Your goal is to write a description that triggers for relevant queries, and doesn't trigger for irrelevant ones.
The description appears in Claude's "available_skills" list. When a user sends a query, Claude decides whether to invoke the skill based solely on the title and on this description. Your goal is to write a description \
that triggers for relevant queries, and doesn't trigger for irrelevant ones.
Here's the current description:
<current_description>
@ -104,9 +99,9 @@ Current scores ({scores_summary}):
prompt += "PREVIOUS ATTEMPTS (do NOT repeat these — try something structurally different):\n\n"
for h in history:
train_s = f"{h.get('train_passed', h.get('passed', 0))}/{h.get('train_total', h.get('total', 0))}"
test_s = f"{h.get('test_passed', '?')}/{h.get('test_total', '?')}" if h.get('test_passed') is not None else None
test_s = f"{h.get('test_passed', '?')}/{h.get('test_total', '?')}" if h.get("test_passed") is not None else None
score_str = f"train={train_s}" + (f", test={test_s}" if test_s else "")
prompt += f'<attempt {score_str}>\n'
prompt += f"<attempt {score_str}>\n"
prompt += f'Description: "{h["description"]}"\n'
if "results" in h:
prompt += "Train results:\n"
@ -114,7 +109,7 @@ Current scores ({scores_summary}):
status = "PASS" if r["pass"] else "FAIL"
prompt += f' [{status}] "{r["query"][:80]}" (triggered {r["triggers"]}/{r["runs"]})\n'
if h.get("note"):
prompt += f'Note: {h["note"]}\n'
prompt += f"Note: {h['note']}\n"
prompt += "</attempt>\n\n"
prompt += f"""</scores_summary>
@ -124,7 +119,9 @@ Skill content (for context on what the skill does):
{skill_content}
</skill_content>
Based on the failures, write a new and improved description that is more likely to trigger correctly. When I say "based on the failures", it's a bit of a tricky line to walk because we don't want to overfit to the specific cases you're seeing. So what I DON'T want you to do is produce an ever-expanding list of specific queries that this skill should or shouldn't trigger for. Instead, try to generalize from the failures to broader categories of user intent and situations where this skill would be useful or not useful. The reason for this is twofold:
Based on the failures, write a new and improved description that is more likely to trigger correctly. When I say "based on the failures", it's a bit of a tricky line to walk because we don't want to overfit to the \
specific cases you're seeing. So what I DON'T want you to do is produce an ever-expanding list of specific queries that this skill should or shouldn't trigger for. Instead, try to generalize from the failures to broader \
categories of user intent and situations where this skill would be useful or not useful. The reason for this is twofold:
1. Avoid overfitting
2. The list might get loooong and it's injected into ALL queries and there might be a lot of skills, so we don't want to blow too much space on any given description.
@ -210,7 +207,11 @@ def main():
if args.history:
history = json.loads(Path(args.history).read_text(encoding="utf-8"))
name, _, content = parse_skill_md(skill_path)
try:
name, _, content = parse_skill_md(skill_path)
except ValueError as exc:
print(f"Error: {exc}", file=sys.stderr)
sys.exit(1)
current_description = eval_results["description"]
if args.verbose:
@ -232,13 +233,16 @@ def main():
# Output as JSON with both the new description and updated history
output = {
"description": new_description,
"history": history + [{
"description": current_description,
"passed": eval_results["summary"]["passed"],
"failed": eval_results["summary"]["failed"],
"total": eval_results["summary"]["total"],
"results": eval_results["results"],
}],
"history": history
+ [
{
"description": current_description,
"passed": eval_results["summary"]["passed"],
"failed": eval_results["summary"]["failed"],
"total": eval_results["summary"]["total"],
"results": eval_results["results"],
}
],
}
print(json.dumps(output, indent=2))

View File

@ -57,20 +57,15 @@ def run_single_query(
project_commands_dir.mkdir(parents=True, exist_ok=True)
# Use YAML block scalar to avoid breaking on quotes in description
indented_desc = "\n ".join(skill_description.split("\n"))
command_content = (
f"---\n"
f"description: |\n"
f" {indented_desc}\n"
f"---\n\n"
f"# {skill_name}\n\n"
f"This skill handles: {skill_description}\n"
)
command_content = f"---\ndescription: |\n {indented_desc}\n---\n\n# {skill_name}\n\nThis skill handles: {skill_description}\n"
command_file.write_text(command_content, encoding="utf-8")
cmd = [
"claude",
"-p", query,
"--output-format", "stream-json",
"-p",
query,
"--output-format",
"stream-json",
"--verbose",
"--include-partial-messages",
]
@ -82,6 +77,11 @@ def run_single_query(
# programmatic subprocess usage is safe.
env = {k: v for k, v in os.environ.items() if k != "CLAUDECODE"}
# Keep this reviewed CLI invocation as a fixed executable plus argv.
# Query/model values must never be interpolated into a shell command.
# Capture stdout for evaluation rather than inheriting a terminal.
# The surrounding finally block owns termination after early exits.
# Environment filtering above permits nested CLI evaluation.
process = subprocess.Popen(
cmd,
stdout=subprocess.PIPE,
@ -232,14 +232,16 @@ def run_eval(
did_pass = trigger_rate >= trigger_threshold
else:
did_pass = trigger_rate < trigger_threshold
results.append({
"query": query,
"should_trigger": should_trigger,
"trigger_rate": trigger_rate,
"triggers": sum(triggers),
"runs": len(triggers),
"pass": did_pass,
})
results.append(
{
"query": query,
"should_trigger": should_trigger,
"trigger_rate": trigger_rate,
"triggers": sum(triggers),
"runs": len(triggers),
"pass": did_pass,
}
)
passed = sum(1 for r in results if r["pass"])
total = len(results)
@ -276,7 +278,11 @@ def main():
print(f"Error: No SKILL.md found at {skill_path}", file=sys.stderr)
sys.exit(1)
name, original_description, content = parse_skill_md(skill_path)
try:
name, original_description, content = parse_skill_md(skill_path)
except ValueError as exc:
print(f"Error: {exc}", file=sys.stderr)
sys.exit(1)
description = args.description or original_description
project_root = find_project_root()

View File

@ -78,10 +78,10 @@ def run_loop(
for iteration in range(1, max_iterations + 1):
if verbose:
print(f"\n{'='*60}", file=sys.stderr)
print(f"\n{'=' * 60}", file=sys.stderr)
print(f"Iteration {iteration}/{max_iterations}", file=sys.stderr)
print(f"Description: {current_description}", file=sys.stderr)
print(f"{'='*60}", file=sys.stderr)
print(f"{'=' * 60}", file=sys.stderr)
# Evaluate train + test together in one batch for parallelism
all_queries = train_set + test_set
@ -118,23 +118,25 @@ def run_loop(
test_results = None
test_summary = None
history.append({
"iteration": iteration,
"description": current_description,
"train_passed": train_summary["passed"],
"train_failed": train_summary["failed"],
"train_total": train_summary["total"],
"train_results": train_results["results"],
"test_passed": test_summary["passed"] if test_summary else None,
"test_failed": test_summary["failed"] if test_summary else None,
"test_total": test_summary["total"] if test_summary else None,
"test_results": test_results["results"] if test_results else None,
# For backward compat with report generator
"passed": train_summary["passed"],
"failed": train_summary["failed"],
"total": train_summary["total"],
"results": train_results["results"],
})
history.append(
{
"iteration": iteration,
"description": current_description,
"train_passed": train_summary["passed"],
"train_failed": train_summary["failed"],
"train_total": train_summary["total"],
"train_results": train_results["results"],
"test_passed": test_summary["passed"] if test_summary else None,
"test_failed": test_summary["failed"] if test_summary else None,
"test_total": test_summary["total"] if test_summary else None,
"test_results": test_results["results"] if test_results else None,
# For backward compat with report generator
"passed": train_summary["passed"],
"failed": train_summary["failed"],
"total": train_summary["total"],
"results": train_results["results"],
}
)
# Write live report if path provided
if live_report_path:
@ -151,6 +153,7 @@ def run_loop(
live_report_path.write_text(generate_html(partial_output, auto_refresh=True, skill_name=name), encoding="utf-8")
if verbose:
def print_eval_stats(label, results, elapsed):
pos = [r for r in results if r["should_trigger"]]
neg = [r for r in results if not r["should_trigger"]]
@ -164,7 +167,7 @@ def run_loop(
precision = tp / (tp + fp) if (tp + fp) > 0 else 1.0
recall = tp / (tp + fn) if (tp + fn) > 0 else 1.0
accuracy = (tp + tn) / total if total > 0 else 0.0
print(f"{label}: {tp+tn}/{total} correct, precision={precision:.0%} recall={recall:.0%} accuracy={accuracy:.0%} ({elapsed:.1f}s)", file=sys.stderr)
print(f"{label}: {tp + tn}/{total} correct, precision={precision:.0%} recall={recall:.0%} accuracy={accuracy:.0%} ({elapsed:.1f}s)", file=sys.stderr)
for r in results:
status = "PASS" if r["pass"] else "FAIL"
rate_str = f"{r['triggers']}/{r['runs']}"
@ -192,10 +195,7 @@ def run_loop(
t0 = time.time()
# Strip test scores from history so improvement model can't see them
blinded_history = [
{k: v for k, v in h.items() if not k.startswith("test_")}
for h in history
]
blinded_history = [{k: v for k, v in h.items() if not k.startswith("test_")} for h in history]
new_description = improve_description(
skill_name=name,
skill_content=content,
@ -265,7 +265,11 @@ def main():
print(f"Error: No SKILL.md found at {skill_path}", file=sys.stderr)
sys.exit(1)
name, _, _ = parse_skill_md(skill_path)
try:
name, _, _ = parse_skill_md(skill_path)
except ValueError as exc:
print(f"Error: {exc}", file=sys.stderr)
sys.exit(1)
# Set up live report path
if args.report != "none":

View File

@ -2,6 +2,7 @@
from pathlib import Path
import yaml
def parse_skill_md(skill_path: Path) -> tuple[str, str, str]:
@ -23,27 +24,17 @@ def parse_skill_md(skill_path: Path) -> tuple[str, str, str]:
if end_idx is None:
raise ValueError("SKILL.md missing frontmatter (no closing ---)")
name = ""
description = ""
frontmatter_lines = lines[1:end_idx]
i = 0
while i < len(frontmatter_lines):
line = frontmatter_lines[i]
if line.startswith("name:"):
name = line[len("name:"):].strip().strip('"').strip("'")
elif line.startswith("description:"):
value = line[len("description:"):].strip()
# Handle YAML multiline indicators (>, |, >-, |-)
if value in (">", "|", ">-", "|-"):
continuation_lines: list[str] = []
i += 1
while i < len(frontmatter_lines) and (frontmatter_lines[i].startswith(" ") or frontmatter_lines[i].startswith("\t")):
continuation_lines.append(frontmatter_lines[i].strip())
i += 1
description = " ".join(continuation_lines)
continue
else:
description = value.strip('"').strip("'")
i += 1
# Use the same YAML semantics as quick_validate.py and the runtime loader.
# Stripping quotes manually changes escaped strings, comments and block scalars.
try:
frontmatter = yaml.safe_load("\n".join(lines[1:end_idx]))
except yaml.YAMLError as exc:
raise ValueError(f"Invalid YAML in SKILL.md frontmatter: {exc}") from exc
if not isinstance(frontmatter, dict):
raise ValueError("SKILL.md frontmatter must be a YAML dictionary")
return name, description, content
name = frontmatter.get("name", "")
description = frontmatter.get("description", "")
if not isinstance(name, str) or not isinstance(description, str):
raise ValueError("SKILL.md name and description must be strings")
return name.strip(), description.strip(), content

View File

@ -0,0 +1,95 @@
"""Description optimization must evaluate the same YAML scalars as the loader."""
import importlib.util
import os
import shutil
import subprocess
import sys
from pathlib import Path
import pytest
SKILL_ROOT = Path(__file__).resolve().parents[2] / "skills/public/skill-creator"
def load_script(name, skill_root=SKILL_ROOT):
spec = importlib.util.spec_from_file_location("skill_creator_" + name, skill_root / "scripts" / (name + ".py"))
module = importlib.util.module_from_spec(spec)
sys.modules[spec.name] = module
previous = sys.dont_write_bytecode
sys.dont_write_bytecode = True
try:
spec.loader.exec_module(module)
finally:
sys.dont_write_bytecode = previous
return module
@pytest.mark.parametrize(
("description_yaml", "expected"),
[
('"Use \\"quoted\\" examples"', 'Use "quoted" examples'),
("'Write the user''s report'", "Write the user's report"),
("Research papers # maintainer note", "Research papers"),
("|-\n First line\n Second line", "First line\nSecond line"),
(">-\n First paragraph\n\n Second paragraph", "First paragraph\nSecond paragraph"),
("|2-\n Keep this line\n And this one", "Keep this line\nAnd this one"),
("Plain description", "Plain description"),
],
)
@pytest.mark.parametrize("name_yaml", ["example", "example # comment"])
def test_optimizer_reads_validated_yaml_values(tmp_path, description_yaml, expected, name_yaml):
content = "---\nname: " + name_yaml + "\ndescription: " + description_yaml + "\n---\n# Body\n"
(tmp_path / "SKILL.md").write_text(content, encoding="utf-8")
assert load_script("quick_validate").validate_skill(tmp_path)[0]
name, description, body = load_script("utils").parse_skill_md(tmp_path)
assert name == "example"
assert description == expected
assert body == content
@pytest.mark.parametrize("frontmatter", ["- item", "name: example\ndescription: [not, text]", "name: true\ndescription: text", "name: example\ndescription: ["])
def test_optimizer_rejects_invalid_metadata(tmp_path, frontmatter):
(tmp_path / "SKILL.md").write_text("---\n" + frontmatter + "\n---\nbody", encoding="utf-8")
with pytest.raises(ValueError):
load_script("utils").parse_skill_md(tmp_path)
@pytest.mark.parametrize("name", ["utils", "quick_validate"])
def test_script_loader_preserves_bytecode_setting(monkeypatch, tmp_path, name):
isolated = tmp_path / "skill"
shutil.copytree(SKILL_ROOT, isolated, ignore=shutil.ignore_patterns("__pycache__", "*.pyc"))
monkeypatch.setattr(sys, "dont_write_bytecode", False)
module = load_script(name, isolated)
assert not list(isolated.rglob("*.pyc"))
assert sys.dont_write_bytecode is False
assert sys.modules[module.__name__] is module
@pytest.mark.parametrize("command", ["run_eval", "run_loop", "improve_description"])
@pytest.mark.parametrize("metadata", ["name: example\ndescription: [", "name: true\ndescription: example", "missing frontmatter"])
def test_cli_reports_invalid_frontmatter_without_traceback(tmp_path, command, metadata):
skill = tmp_path / "input-skill"
skill.mkdir()
(skill / "SKILL.md").write_text("---\n" + metadata + "\n---\nBody", encoding="utf-8")
inputs = tmp_path / "eval.json"
inputs.write_text("{}" if command == "improve_description" else "[]", encoding="utf-8")
args = ["--skill-path", str(skill)]
if command == "improve_description":
args += ["--eval-results", str(inputs), "--model", "offline-test"]
else:
args += ["--eval-set", str(inputs)]
if command == "run_loop":
args += ["--model", "offline-test", "--report", "none"]
result = subprocess.run(
[sys.executable, "-B", "-m", "scripts." + command, *args],
cwd=SKILL_ROOT,
env={**os.environ, "PATH": "", "PYTHONDONTWRITEBYTECODE": "1"},
capture_output=True,
text=True,
timeout=10,
)
assert result.returncode == 1
assert result.stderr.startswith("Error: ")
assert "Traceback" not in result.stderr
assert result.stdout == ""