Skip to content

An idea for testing fixture-based eval harness for skill steps - #158

Merged
potiuk merged 19 commits into
apache:mainfrom
justinmclean:testing
May 17, 2026
Merged

An idea for testing fixture-based eval harness for skill steps#158
potiuk merged 19 commits into
apache:mainfrom
justinmclean:testing

Conversation

@justinmclean

Copy link
Copy Markdown
Member

What this adds

A small, model-agnostic eval harness under tools/skill-evals/ for
verifying that skill steps produce the expected structured output.

The runner prints the exact system prompt + user prompt for each fixture
case so you can paste it into any model and compare the response against
expected.json so as to be vendor-neutral.

This is an early idea; I'm sharing it for feedback before we go further.

  • Should fixture cases live next to their skill rather than under tools/skill-evals/evals/?
  • Is manual paste-and-compare enough, or do we want automated scoring via the API? If so, how do we make that vendor-neutral?
  • Is there a simpler approach entirely?

Happy to rework or close in favour of a better idea.

@choo121600 choo121600 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this looks really great!!!

For (1), I think tools/skill-evals/evals/ looks like the better location. Putting it under the agent path does look cleaner structurally, but I worry there’s a risk of eval fixtures accidentally mixing into agent context at some point, so keeping them separated seems safer.

For (2), I think manual paste-and-compare is sufficient for now. If we eventually automate model execution, I’d prefer a CLI-style approach (e.g. claude -p) over direct API integration.

@potiuk

potiuk commented May 15, 2026

Copy link
Copy Markdown
Member

Really cool . I love how clean and nice it is.. I would probably use Yaml not json ;) (but this is just my eastethics looking at all this extra {} :) .

For (1), I think tools/skill-evals/evals/ looks like the better location. Putting it under the agent path does look cleaner structurally, but I worry there’s a risk of eval fixtures accidentally mixing into agent context at some point, so keeping them separated seems safer.

Agree.

For (2), I think manual paste-and-compare is sufficient for now. If we eventually automate model execution, I’d prefer a CLI-style approach (e.g. claude -p) over direct API integration.

I think it's good to start. We could even add a SKILL to run all such evals locally (asking the agent to essentially copy & paste) things - and have us to do periodic re-run with our own agents (We could even have this skill written in the way that it would setup some reminders to do it from time to time) . I think that should do for now.

@justinmclean

Copy link
Copy Markdown
Member Author

I updated and expanded on this for the cover two skills.

runner.py now assembles the system prompt by extracting the relevant section directly from SKILL.md at run time (via a step-config.json pointer in each fixtures directory), rather than relying on a static copy. A change to the skill rules is immediately visible in the prompt — if it would cause the model to produce different output, the test fails.

External tool calls (GitHub CLI, Gmail MCP, canned-response scan) are never executed during evals. Their outputs are pre-rendered as structured text in each case's report.md and injected into the user turn as mock data. This keeps inputs fully deterministic and requires no network access or API credentials to run.

Removed the hardcoded SYSTEM_PROMPT fallback in runner.py.

65/65 cases pass (32 import, 33 triage).

@justinmclean

Copy link
Copy Markdown
Member Author

BTW, there's no need for a manual copy and paste unless you want to. You can ask Claude Desktop to run the tests, and it will, no SKILL file is needed.

@potiuk

potiuk commented May 16, 2026

Copy link
Copy Markdown
Member

Wowowowoowow! That looks super cool :)

@potiuk

potiuk commented May 16, 2026

Copy link
Copy Markdown
Member

Some results of those agentic skils are terrifying :) .... But we can also use agents to review those changes now:

Screenshot 2026-05-16 at 13 17 42

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this up early — the fixture corpus is impressively thorough, and the model-agnostic paste-and-compare framing is a sensible MVP. A few things to flag before going further; left some specifics inline.

Direction / answers to your questions

Fixtures next to the skill vs. under tools/skill-evals/?
Keep under tools/skill-evals/. Co-locating 620 fixture files inside .claude/skills/<name>/ would bloat the agent's view of "what is the skill" and the skill loader has to walk past them. A one-line pointer from each SKILL.md to the eval suite is enough cross-linking.

Vendor-neutral automation?
Paste-and-compare collapses at 206 cases. eval_runner.py already proves the wiring exists — generalise it behind a call_model(system, user) -> str interface keyed by --provider {anthropic,openai,bedrock,vertex}, each a ~30-line adapter. JSON extraction + the comparator stay vendor-neutral.

Simpler approach?
The harness itself is small (~250 lines) — not the place to simplify. Where you can: cut the duplication between runner.py and eval_runner.py by extracting prepare_case(case_dir, fixtures_dir, _cache) -> (label, system, user, expected).

Two blocking things not anchorable to a single line

  1. No pyproject.toml. Neither uv run --project tools/skill-evals skill-eval ... (README) nor python3 -m skill_evals.eval_runner ... (eval_runner docstring) resolves without a pyproject.toml under tools/skill-evals/ declaring the skill_evals src-layout package and the skill-eval console script. The PR ships none.

  2. README's "All five cases passed" is unverifiable. runner.py doesn't compare, eval_runner.py isn't wired to CI in this PR. Either drop the claim or land a regenerable summary.

Review ergonomics

620 files in one PR is brutal to review. The framework (runners + 1 suite) could land first; the rest stack as follow-ups — would let the runner bugs (see inline) get caught before the fixture corpus is locked in around them.

Risk summary

Risk Severity
extract_skill_section truncates Step 2 of security-cve-allocate today high — silently passes a wrong test
Missing pyproject.toml — neither documented runner invocation works high
extract_json MULTILINE regex strips internal fences too medium
README oversells "structural assertions" and "all five passed" low (docs)

Comment thread tools/skill-evals/src/skill_evals/runner.py Outdated
Comment thread tools/skill-evals/src/skill_evals/runner.py
Comment thread tools/skill-evals/src/skill_evals/runner.py Outdated
Comment thread tools/skill-evals/src/skill_evals/eval_runner.py Outdated
Comment thread tools/skill-evals/src/skill_evals/eval_runner.py Outdated
Comment thread tools/skill-evals/README.md

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at SHA 1142fb5 (3 commits since my first review: 1c74f9c, 2076710, 1142fb5)

Status of my prior comments

# Topic Status
1 extract_skill_section heading bug Fixed. New code tracks ```` / ~~~ fence state and uses `re.match(r"^(#{1,6}) ", …)` instead of the `lstrip("#")` trick. Verified concretely: the line-309 `# Leading…` in `security-cve-allocate/SKILL.md` is inside a fenced block, so Step 2 extraction is no longer truncated.
2 rglob("fixtures") double-count Fixed. seen_fixtures set + is_relative_to check (runner.py:213-219).
3 .format() fragility Fixed. Slot contract documented at runner.py:48-50, plus an actionable error wrapped around the .format() call (runner.py:263-268). Reasonable choice to keep str.format rather than string.Template.
4 extract_json MULTILINE regex Moot. eval_runner.py removed — when automation lands as a follow-up, please re-check this.
5 compare vs README structural-assertions claim Moot (also lifted by eval_runner.py removal). The remaining README paragraph now correctly describes a fixture convention rather than runner behavior.
6 Missing pyproject.toml Fixed. Hatchling backend, [project.scripts] console-script entry, requires-python >= 3.11 matches the PEP-604 type hints, dependencies = [] matches stdlib-only imports. uv run --project tools/skill-evals skill-eval … should resolve cleanly.

The "All five cases passed" claim is also gone from the README.

Direction

Cutting eval_runner.py was the right call for an MVP — the harness is now self-contained, has no third-party deps, and the print-and-paste workflow is the right primitive to land first. Automation is a clean follow-up.

Minor follow-ups (non-blocking)

  • pyproject.toml declares dev = ["pytest>=8.0", …] but ships no tests/. extract_skill_section, find_cases, and the fence-state tracker are exactly the kind of small pure-function logic that benefits most from a handful of unit tests — would have caught the original truncation bug before it ever shipped. Worth a follow-up PR.
  • README still describes the suite as "currently manual — it prints prompts for human review" (line 70). Accurate as of this PR; just flagging that the "CI considerations" paragraph below it (lines 71-74) reads as deferred-but-planned, which is appropriate.

Blockers cleared from my side.

@justinmclean
justinmclean marked this pull request as ready for review May 17, 2026 05:35
@justinmclean justinmclean changed the title An idea for testing fixture-based eval harness for skill steps [DRAFT] An idea for testing fixture-based eval harness for skill steps May 17, 2026
@potiuk

potiuk commented May 17, 2026

Copy link
Copy Markdown
Member

Let's get it in :)

@potiuk
potiuk merged commit a3db638 into apache:main May 17, 2026
12 checks passed
@choo121600

Copy link
Copy Markdown
Member

COOOL🙌

@justinmclean
justinmclean deleted the testing branch May 28, 2026 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants