Repository navigation
Fix skill-creator trigger detection reporting 0% recall - #1769
ChiFungHillmanChan wants to merge 2 commits into
Conversation
run_single_query concluded that a skill had not triggered before the evidence could arrive, so every eval reported recall=0% and run_loop then "improved" descriptions against measurements that were all false. Three paths ended the judgement early: another tool opening a block was treated as proof of failure, the first content_block_stop decided the whole query, and the non-streaming fallback returned inside its own loop after one tool_use. Detection now keeps scanning until the result event, resetting per-block state instead of concluding from it. Adds tests that replay recorded streams through a real pipe, so no API calls.
|
Reviewed this PR against current Two-state verification (macOS, system python3, stdlib only):
Non-blocking observations:
Heads-up for maintainers: Overall: looks good to merge from a code standpoint. |
The documented module path could never resolve: the directory is skill-creator, with a hyphen, so skills.skill_creator.scripts is not an importable path. Name the working directory and use the path that works from it.
|
Thanks for re-running it independently, and you are right about the docstring.
On the test file living inside the skill directory: that is intentional, and #1744 asks for test coverage here. Happy to move it if maintainers would rather it sat elsewhere. On the collision, your read matches mine, and it is worth being plain about it. #1757 also closes #1721, two days before this one, and addresses the same three causes: the early If maintainers prefer #1757, I am glad to close this one, just say so. The outcome I would rather avoid is four PRs on the same function continuing to sit while the harness still reports 0% recall. |
|
Hi @ChiFungHillmanChan — verified the follow-up commit (73cf998) on the current head, macOS system python3, run from
The docstring now documents both working invocations, and the non-importable |
2 participants
The issue
Fixes #1721.
skill-creator's trigger evaluation reportsprecision=100% recall=0%for every skill, whatever its description says.run_loopthen tunes the description against evidence that every positive query failed, and reports that as an optimisation. Nothing errors, so the output reads like a real measurement of a bad description.The diagnosis is the reporter's. This implements the three paths they identified.
How it works now, and why that is wrong
run_single_query()inskills/skill-creator/scripts/run_eval.pydecides whether the skill fired by watching theclaude -pevent stream. Three paths reach a verdict before the evidence can arrive.Any other tool opening a block is treated as proof of failure:
A concrete query makes the model orient itself with
BashorGrepfirst, which is exactly the shape of query the skill-creator guide asks authors to write.The first
content_block_stopthen decides the whole query, so aReadof an unrelated file before theSkillcall settles it against that block's accumulated input:And the non-streaming fallback returns inside its own
for, so only the firsttool_usein an assistant message is ever examined.Tested before the fix
At the merge base,
34040c9, withscripts/test_run_eval.pyfrom this branch:In each of those three the skill was invoked and the harness scored it as not triggered. The fourth test is the negative case, a query where the skill never runs, and it passes here.
Tested after the fix
Same command, same file, at this PR's tip
b7a840e:The negative case passes in both states, so the change does not buy recall with false positives.
What the fix does
Detection keeps scanning until the
resultevent. Another tool opening a block clears the pending state rather than ending the query,content_block_stopresets per-block state instead of concluding from it, and the fallback walks the whole assistant message before moving on.scripts/test_run_eval.pyis new. It replays recorded streams through a real OS pipe, soselect()andos.read()behave as they do against a live child and only the bytes are scripted. No API calls, and nothing beyond the standard library.What it does not do
--num-workers > 1behaviour the reporter mentions as possibly separate. That is untouched here.claude -phas already exited at the next poll, its output is read intobufferand the loop breaks without parsing it. I will raise that separately.claude -psession.