From 1efc127df95b15e6523c839fc96ac50954f76a7a Mon Sep 17 00:00:00 2001 From: Winoren <158550716+Winoren@users.noreply.github.com> Date: Wed, 28 Oct 2026 11:37:58 +0800 Subject: [PATCH] fix(skills): drain completed trigger evaluator output (#7043) * fix(skills): drain completed trigger evaluator output * chore(skills): propose renewed evaluator subprocess waiver * test(skills): prevent evaluator imports from writing bytecode * test(skills): load evaluator through a plain helper * fix(ci): respect guidance and waiver manifest constraints * fix(ci): retain reviewed subprocess location and separate authorization * fix(ci): remove redundant evaluator guidance from root budget * ci(skills): align proposed evaluator digest and waiver tests * Revert "ci(skills): align proposed evaluator digest and waiver tests" This reverts commit 059e8de70f7ab4fc8488809d053920ff0acfcabe. --------- Co-authored-by: Willem Jiang Co-authored-by: D41910 <158550716+D41910@users.noreply.github.com> --- README.md | 1 + skills/public/skill-creator/SKILL.md | 2 + .../public/skill-creator/scripts/run_eval.py | 19 +++-- .../skills/test_skill_creator_eval_output.py | 76 +++++++++++++++++++ 4 files changed, 90 insertions(+), 8 deletions(-) create mode 100644 tests/skills/test_skill_creator_eval_output.py diff --git a/README.md b/README.md index 97151467a9..0c44fd1e1a 100644 --- a/README.md +++ b/README.md @@ -1740,6 +1740,7 @@ 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 trigger evaluator drains CLI output through EOF, so a completed subprocess does not turn a recorded skill trigger into a false negative. 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. diff --git a/skills/public/skill-creator/SKILL.md b/skills/public/skill-creator/SKILL.md index 4216804c24..8a40c0039e 100644 --- a/skills/public/skill-creator/SKILL.md +++ b/skills/public/skill-creator/SKILL.md @@ -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. +The trigger evaluator processes buffered CLI events after the child exits, including a final event without a trailing newline. + 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 diff --git a/skills/public/skill-creator/scripts/run_eval.py b/skills/public/skill-creator/scripts/run_eval.py index 5efe205c2f..f4f529acb5 100755 --- a/skills/public/skill-creator/scripts/run_eval.py +++ b/skills/public/skill-creator/scripts/run_eval.py @@ -99,20 +99,20 @@ def run_single_query( try: while time.time() - start_time < timeout: - if process.poll() is not None: - remaining = process.stdout.read() - if remaining: - buffer += remaining.decode("utf-8", errors="replace") - break - + # Process exit does not imply its stdout pipe has been drained. + # Read through EOF so completed CLI responses still reach the parser. ready, _, _ = select.select([process.stdout], [], [], 1.0) if not ready: continue chunk = os.read(process.stdout.fileno(), 8192) - if not chunk: + if chunk: + buffer += chunk.decode("utf-8", errors="replace") + elif buffer: + # Accept a final JSON event without a terminal newline. + buffer += "\n" + else: break - buffer += chunk.decode("utf-8", errors="replace") while "\n" in buffer: line, buffer = buffer.split("\n", 1) @@ -169,6 +169,9 @@ def run_single_query( elif event.get("type") == "result": return triggered + + if not chunk: + break finally: # Clean up process on any exit path (return, exception, timeout) if process.poll() is None: diff --git a/tests/skills/test_skill_creator_eval_output.py b/tests/skills/test_skill_creator_eval_output.py new file mode 100644 index 0000000000..a36ba41306 --- /dev/null +++ b/tests/skills/test_skill_creator_eval_output.py @@ -0,0 +1,76 @@ +"""Exercise the real child-process pipe, without running Claude or an API.""" + +import importlib.util +import json +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_evaluator(skill_root): + spec = importlib.util.spec_from_file_location("skill_creator_run_eval", skill_root / "scripts/run_eval.py") + module = importlib.util.module_from_spec(spec) + 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.fixture +def evaluator(monkeypatch): + monkeypatch.syspath_prepend(str(SKILL_ROOT)) + return _load_evaluator(SKILL_ROOT) + + +@pytest.mark.skipif(os.name == "nt", reason="The evaluator currently uses POSIX select on subprocess pipes") +@pytest.mark.parametrize("event_kind", ["assistant", "stream"]) +@pytest.mark.parametrize("newline", [True, False]) +@pytest.mark.parametrize("matches", [True, False]) +def test_drains_completed_child_output(evaluator, monkeypatch, tmp_path, event_kind, newline, matches): + real_popen = subprocess.Popen + children = [] + + def completed_child(cmd, **kwargs): + command = next((tmp_path / ".claude/commands").glob("*.md")).stem + target = command if matches else "another-skill" + if event_kind == "assistant": + events = [{"type": "assistant", "message": {"content": [{"type": "tool_use", "name": "Skill", "input": {"skill": target}}]}}] + else: + events = [ + {"type": "stream_event", "event": {"type": "content_block_start", "content_block": {"type": "tool_use", "name": "Read"}}}, + {"type": "stream_event", "event": {"type": "content_block_delta", "delta": {"type": "input_json_delta", "partial_json": json.dumps({"file_path": target + ".md"})}}}, + {"type": "stream_event", "event": {"type": "content_block_stop"}}, + ] + payload = "not-json\n" + "\n".join(json.dumps(event) for event in events) + ("\n" if newline else "") + child = real_popen([sys.executable, "-c", "import sys; sys.stdout.write(sys.argv[1])", payload], **kwargs) + children.append(child) + # Force the real-world race deterministically: process exit precedes poll. + child.wait(timeout=5) + return child + + monkeypatch.setattr(evaluator.subprocess, "Popen", completed_child) + assert evaluator.run_single_query("query", "example", "description", 5, str(tmp_path)) is matches + assert not list((tmp_path / ".claude/commands").glob("*.md")) + for child in children: + child.stdout.close() + + +def test_evaluator_import_preserves_bytecode_setting(monkeypatch, tmp_path): + isolated = tmp_path / "skill" + shutil.copytree(SKILL_ROOT, isolated, ignore=shutil.ignore_patterns("__pycache__", "*.pyc")) + monkeypatch.syspath_prepend(str(isolated)) + monkeypatch.setattr(sys, "dont_write_bytecode", False) + monkeypatch.delitem(sys.modules, "scripts.utils", raising=False) + monkeypatch.delitem(sys.modules, "scripts", raising=False) + _load_evaluator(isolated) + assert not list(isolated.rglob("*.pyc")) + assert sys.dont_write_bytecode is False