diff --git a/datasets/dataset_quality/example_run_config.yaml b/datasets/dataset_quality/example_run_config.yaml index 2984a520..d34d0c8f 100644 --- a/datasets/dataset_quality/example_run_config.yaml +++ b/datasets/dataset_quality/example_run_config.yaml @@ -1,10 +1,8 @@ ############################################################ ### Dataset Quality — example run config ############################################################ -# Grades the intrinsic quality of a CUJ dataset (no eval / agent run). Runs on the -# standard agent pipeline: a no-op generator does nothing, and the `dataset_quality` -# scorer grades the whole dataset holistically (each sub-scorer computes a 0-100 -# sub-score; grading rolls them into a weighted global score + letter grade). +# Grades the dataset's CUJs, not a model: no agent runs and nothing is executed. +# The product only supplies the tools and skills those CUJs are scored against. # Forced by dataset_format below; listed here only to document what runs. orchestrator: agent @@ -26,12 +24,9 @@ simulated_user_model_config: datasets/model_configs/noop_agent_model.yaml ############################################################ ### Scorers ############################################################ -# The `dataset_quality` scorer fetches the product's tool catalog live from its own -# model_config (each setup.mcp_servers entry with an httpUrl/url is queried via MCP -# tools/list; names are namespaced __ to match expected_trajectory). -# google_credentials servers use ADC, so this needs credentials + network (e.g. -# `gcloud auth application-default login`). Run the LLM judges with -# EVAL_GCP_PROJECT_REGION=global. +# Reads the product's capability surface from the model_config below: +# setup.mcp_servers for tools (named __, matching +# expected_trajectory) and setup.skills for skills. scorers: dataset_quality: model_config: datasets/model_configs/gemini_cli_model.yaml @@ -41,6 +36,8 @@ scorers: # required only by the judge sub-scorers; trajectory_coverage and # naming_distribution are static. sub_scorers: + # Counts MCP tools and skill scripts (matched against a CUJ's + # expected_trajectory) plus each skill's own name (expected_skills). trajectory_coverage: # weight: 20 (default) naming_distribution: @@ -71,8 +68,6 @@ scorers: ############################################################ ### Reporting ############################################################ -# The grade lands as one summary scoring row plus one row per category; uncomment -# bigquery to also append to the table. reporting: csv: output_directory: 'results' diff --git a/evalbench/generators/models/skills_catalog.py b/evalbench/generators/models/skills_catalog.py new file mode 100644 index 00000000..2b695733 --- /dev/null +++ b/evalbench/generators/models/skills_catalog.py @@ -0,0 +1,255 @@ +"""Reads a product's skill catalog from a model config's setup block. + +The skills counterpart of mcp_client. An entry that won't resolve raises instead +of being skipped: a half-resolved catalog is a shrunken denominator, which +inflates any coverage measured against it. +""" + +from dataclasses import dataclass +import logging +import os +import re +import shutil +import subprocess +import tempfile + +import yaml + + +_GIT_URL_PATTERN = re.compile(r"^(https?|git|ssh)://|^git@|\.git(#.*)?$") + +_CLONE_TIMEOUT_S = 120 + +# Leading `---` delimited YAML block at the top of a SKILL.md. +_FRONTMATTER_PATTERN = re.compile(r"\A---\s*\n(.*?)\n---\s*(?:\n|\Z)", re.DOTALL) + +# Languages a skill's scripts are written in. +_SCRIPT_SUFFIXES = frozenset({".js", ".cjs", ".mjs", ".ts", ".py", ".sh", ".bash"}) + + +class SkillCatalogError(Exception): + """A declared skills entry could not be resolved.""" + + +@dataclass(frozen=True) +class Skill: + """One skill in a product's catalog. + + `scripts` holds a skill's adjacent scripts/ directory — the layout MCP-toolbox + generated skills use, and what a CUJ trajectory names. A skill without one is + graded on its name alone. + """ + + name: str + description: str = "" + scripts: tuple[str, ...] = () + + +def resolve_skills(setup: dict) -> list[Skill]: + """Skills declared by a model config's setup block. + + Reads setup.skills and setup.skills_dir, returning an empty list when neither + is present. Each declared entry must yield at least one skill or it raises + SkillCatalogError, so "no skills" stays distinguishable from "skills failed + to resolve". + """ + setup = setup or {} + skills: list[Skill] = [] + + for entry in setup.get("skills") or []: + _collect_entry(entry, skills) + + skills_dir = setup.get("skills_dir") + if skills_dir: + _extend(skills, _scan_dir(skills_dir)) + + return skills + + +def _collect_entry(entry, into: list[Skill]) -> None: + """Resolves one setup.skills entry and appends it to the list.""" + if isinstance(entry, str): + _extend(into, _resolve_target(entry, wanted=None)) + return + if not isinstance(entry, dict): + raise SkillCatalogError(f"unusable skills entry {entry!r}") + + target = entry.get("url") or entry.get("path") + if not target: + raise SkillCatalogError(f"skills entry {entry!r} names no url or path") + + _extend(into, _resolve_target(target, wanted=_wanted_names(entry))) + + +def _wanted_names(entry: dict) -> set[str] | None: + """The subset of a source's skills an entry installs, if it narrows them. + + An entry may install only some of a repo's skills, in which case the rest are + not part of the product's surface and must not land in the denominator. + """ + names = entry.get("skills") or entry.get("skill_names") + if not names: + single = entry.get("skill") or entry.get("name") + names = [single] if single else None + return {str(name) for name in names} if names else None + + +def _resolve_target(target: str, wanted: set[str] | None) -> list[Skill]: + """Scan a local path, or clone a git URL to a temp dir and scan that.""" + if not _GIT_URL_PATTERN.search(target): + return _scan_dir(target, wanted) + + clone_dir = _clone(target) + try: + return _scan_dir(clone_dir, wanted) + finally: + shutil.rmtree(clone_dir, ignore_errors=True) + + +def _clone(url: str) -> str: + """Clones a skills repository into a temporary directory. + + A '#' ref is passed to --branch, so it must be a branch or tag; a + commit SHA fails rather than resolving. Retrying such a ref unpinned would + clone the default branch, grading the dataset against the wrong catalog. + """ + clone_url, _, ref = url.partition("#") + dest = tempfile.mkdtemp(prefix="skills_catalog_") + cmd = ["git", "clone", "--depth", "1"] + if ref: + cmd += ["--branch", ref] + try: + subprocess.run( + cmd + [clone_url, dest], check=True, capture_output=True, text=True, + timeout=_CLONE_TIMEOUT_S, + ) + except (subprocess.CalledProcessError, subprocess.TimeoutExpired, + OSError) as e: + shutil.rmtree(dest, ignore_errors=True) + raise SkillCatalogError(f"clone failed for {url}: {e}") from e + return dest + + +def _scan_dir(root: str, wanted: set[str] | None = None) -> list[Skill]: + """Reads every skill under a root, optionally narrowed to the wanted names.""" + if not os.path.isdir(root): + raise SkillCatalogError(f"skills path not found: {root}") + skills = [] + by_identity = {} + for path in _find_skill_dirs(root): + skill = _read_skill(path) + skills.append(skill) + # An entry may name a skill by its frontmatter name or by its directory. + by_identity[skill.name.lower()] = skill + by_identity.setdefault(os.path.basename(path).lower(), skill) + + if not skills: + raise SkillCatalogError(f"no SKILL.md found under {root}") + if not wanted: + return skills + + # Every wanted name must match. One that doesn't would go missing from the + # denominator and inflate coverage. + missing = sorted(name for name in wanted if name.lower() not in by_identity) + if missing: + raise SkillCatalogError(f"{root} holds no skill named: {', '.join(missing)}") + wanted_skills = {by_identity[name.lower()] for name in wanted} + return [skill for skill in skills if skill in wanted_skills] + + +def _find_skill_dirs(root: str) -> list[str]: + """Skill directories under a root, matching the generators' three layouts.""" + if os.path.exists(os.path.join(root, "SKILL.md")): + return [root] + + skills_root = os.path.join(root, "skills") + if os.path.isdir(skills_root): + return _child_skill_dirs(skills_root) + + return _child_skill_dirs(root) + + +def _child_skill_dirs(parent: str) -> list[str]: + try: + entries = sorted(os.listdir(parent)) + except OSError as e: + raise SkillCatalogError(f"cannot list {parent}: {e}") from e + return [ + os.path.join(parent, entry) + for entry in entries + if os.path.exists(os.path.join(parent, entry, "SKILL.md")) + ] + + +def _read_skill(skill_dir: str) -> Skill: + """One skill's name and description, from SKILL.md frontmatter. + + An agent activates a skill by its frontmatter name, so that wins over the + directory name, which is the fallback when the frontmatter is missing or + malformed. + """ + dir_name = os.path.basename(os.path.normpath(skill_dir)) + meta = _read_frontmatter(os.path.join(skill_dir, "SKILL.md")) + name = meta.get("name") or dir_name + description = meta.get("description") or "" + return Skill( + name=str(name).strip(), + description=str(description).strip(), + scripts=_read_scripts(skill_dir), + ) + + +def _read_scripts(skill_dir: str) -> tuple[str, ...]: + """The invocable scripts in a skill's scripts/ directory. + + Kept as filenames rather than stems because that is how a trajectory names + them ("list_instances.js"). + """ + scripts_dir = os.path.join(skill_dir, "scripts") + try: + entries = sorted(os.listdir(scripts_dir)) + except OSError: + return () + return tuple( + entry for entry in entries + if _is_script(entry) and os.path.isfile(os.path.join(scripts_dir, entry)) + ) + + +def _is_script(filename: str) -> bool: + """Whether a scripts/ entry is an invocable script a CUJ could name.""" + stem, suffix = os.path.splitext(filename) + if suffix.lower() not in _SCRIPT_SUFFIXES or stem == "__init__": + return False + return not (stem.startswith("test_") or stem.endswith("_test")) + + +def _read_frontmatter(skill_md: str) -> dict: + try: + with open(skill_md, "r", encoding="utf-8") as f: + text = f.read() + except OSError as e: + logging.warning("skills_catalog: cannot read %s: %s", skill_md, e) + return {} + + match = _FRONTMATTER_PATTERN.match(text) + if not match: + return {} + try: + meta = yaml.safe_load(match.group(1)) + except yaml.YAMLError as e: + logging.warning( + "skills_catalog: malformed frontmatter in %s: %s", skill_md, e + ) + return {} + return meta if isinstance(meta, dict) else {} + + +def _extend(skills: list[Skill], found: list[Skill]) -> None: + """Appends the found skills, dropping names already present (ignoring case).""" + seen = {skill.name.lower() for skill in skills} + for skill in found: + key = skill.name.lower() + if key and key not in seen: + seen.add(key) + skills.append(skill) diff --git a/evalbench/scorers/dataset_quality/composition.py b/evalbench/scorers/dataset_quality/composition.py index 711dafa7..c8731ff6 100644 --- a/evalbench/scorers/dataset_quality/composition.py +++ b/evalbench/scorers/dataset_quality/composition.py @@ -91,10 +91,10 @@ def run(self, context: DatasetQualityContext) -> SubScoreContribution: return SubScoreContribution( score=score, metrics={ - "dq_multitool_count": n_multi, - "dq_sequence_count": n_seq, - "dq_multitool_score": multitool_score, - "dq_sequencing_score": sequencing_score, + "multitool_cuj_count": n_multi, + "sequence_cuj_count": n_seq, + "multitool_score": multitool_score, + "sequencing_score": sequencing_score, }, suggestions=suggestions, evidence={KEY_MULTI_TOOL: multi_ids, KEY_SEQUENCE_DEPENDENCY: seq_ids}, diff --git a/evalbench/scorers/dataset_quality/context.py b/evalbench/scorers/dataset_quality/context.py index a1e4ebbd..204d0887 100644 --- a/evalbench/scorers/dataset_quality/context.py +++ b/evalbench/scorers/dataset_quality/context.py @@ -45,6 +45,12 @@ def expected_trajectory(scenario: dict) -> list: return trajectory if isinstance(trajectory, list) else [] +def expected_skills(scenario: dict) -> list: + """A scenario's expected_skills, or empty when it isn't a list.""" + skills = scenario.get("expected_skills") + return skills if isinstance(skills, list) else [] + + @dataclass class DatasetQualityContext: """Everything a scorer needs to grade one product's CUJ dataset.""" @@ -52,6 +58,9 @@ class DatasetQualityContext: product_name: str scenarios: list[dict] tools: list # list[mcp.types.Tool] + skills: list = field(default_factory=list) # list[skills_catalog.Skill] + # Why the declared skills could not be resolved, when they couldn't. + skills_error: str | None = None @property def n(self) -> int: @@ -66,6 +75,18 @@ def tool_names(self) -> list[str]: names = (self._tool_field(tool, "name") for tool in self.tools) return [name for name in names if name] + @property + def skill_names(self) -> list[str]: + return list(dict.fromkeys(skill.name for skill in self.skills if skill.name)) + + @property + def script_names(self) -> list[str]: + """Every script shipped by a catalog skill, de-duplicated across skills.""" + scripts = [] + for skill in self.skills: + scripts.extend(skill.scripts) + return list(dict.fromkeys(scripts)) + @property def tool_names_str(self) -> str: return ", ".join(self.tool_names) or "(none provided)" diff --git a/evalbench/scorers/dataset_quality/cuj_diversity.py b/evalbench/scorers/dataset_quality/cuj_diversity.py index 6546b6a6..2d61a1e1 100644 --- a/evalbench/scorers/dataset_quality/cuj_diversity.py +++ b/evalbench/scorers/dataset_quality/cuj_diversity.py @@ -77,8 +77,8 @@ def run(self, context: DatasetQualityContext) -> SubScoreContribution: return SubScoreContribution( score=score, metrics={ - "dq_paths_covered": len(covered), - "dq_paths_total": len(CUJ_PATHS), + "paths_covered": len(covered), + "paths_total": len(CUJ_PATHS), }, suggestions=suggestions, example_prompts=( diff --git a/evalbench/scorers/dataset_quality/error_recovery.py b/evalbench/scorers/dataset_quality/error_recovery.py index 0eb95ec6..baaf2899 100644 --- a/evalbench/scorers/dataset_quality/error_recovery.py +++ b/evalbench/scorers/dataset_quality/error_recovery.py @@ -76,8 +76,8 @@ def run(self, context: DatasetQualityContext) -> SubScoreContribution: return SubScoreContribution( score=score, metrics={ - "dq_error_modes_covered": len(covered), - "dq_error_modes_total": len(ERROR_RECOVERY_MODES), + "error_modes_covered": len(covered), + "error_modes_total": len(ERROR_RECOVERY_MODES), }, suggestions=suggestions, example_prompts=( diff --git a/evalbench/scorers/dataset_quality/naming_distribution.py b/evalbench/scorers/dataset_quality/naming_distribution.py index eef23578..56e4cc66 100644 --- a/evalbench/scorers/dataset_quality/naming_distribution.py +++ b/evalbench/scorers/dataset_quality/naming_distribution.py @@ -80,7 +80,7 @@ def run(self, context: DatasetQualityContext) -> SubScoreContribution: ) return SubScoreContribution( score=score, - metrics={"dq_tool_named_count": n_named}, + metrics={"tool_named_cuj_count": n_named}, suggestions=suggestions, evidence={"names_tool_ids": named_ids, "intent_based_ids": intent_ids}, ) diff --git a/evalbench/scorers/dataset_quality/parameter_coverage.py b/evalbench/scorers/dataset_quality/parameter_coverage.py index 3fdf7922..cd27b24f 100644 --- a/evalbench/scorers/dataset_quality/parameter_coverage.py +++ b/evalbench/scorers/dataset_quality/parameter_coverage.py @@ -97,8 +97,8 @@ def run(self, context: DatasetQualityContext) -> SubScoreContribution: return SubScoreContribution( score=score, metrics={ - "dq_param_covered": len(covered), - "dq_param_in_scope": len(params), + "params_covered": len(covered), + "params_in_scope": len(params), }, suggestions=suggestions, example_prompts=( diff --git a/evalbench/scorers/dataset_quality/scorer.py b/evalbench/scorers/dataset_quality/scorer.py index 618f1a76..822199a8 100644 --- a/evalbench/scorers/dataset_quality/scorer.py +++ b/evalbench/scorers/dataset_quality/scorer.py @@ -18,6 +18,7 @@ from generators.models import get_generator from generators.models.agent_cli import AgentCliGenerator from generators.models.mcp_client import McpToolsError +from generators.models.skills_catalog import SkillCatalogError, resolve_skills from scorers.comparator import Comparator from scorers.dataset_quality.composition import CompositionScorer from scorers.dataset_quality.context import ( @@ -125,43 +126,34 @@ def compare( ) -> list[Tuple[str, float | None, str]]: scenarios = self._extract_cujs(generated_eval_result) if not scenarios: - # Null score, not 0, so an empty/malformed all_cujs stays - # distinguishable from a genuine F on the trends page. - return [( - self.name, - None, - json.dumps( - { - "product_name": self.product_name, - "graded": False, - "error": "no CUJs to grade (missing or empty all_cujs)", - }, - default=str, - ), - )] + return self._ungraded("no CUJs to grade (missing or empty all_cujs)") + setup = load_yaml_config(self.model_config_path).get("setup") or {} try: - tools = self._fetch_tools() + tools = self._fetch_tools(setup) except McpToolsError as e: - # Tool discovery is infrastructure (network / ADC), not a property of - # the dataset, so an ungraded null row keeps a transient blip - # distinguishable from a genuine F. - logging.error("dataset_quality: tool discovery failed: %s", e) - return [( - self.name, - None, - json.dumps( - { - "product_name": self.product_name, - "graded": False, - "error": f"tool discovery failed: {e}", - }, - default=str, - ), - )] + # Every sub-scorer reads the tool catalog, so nothing is left to grade. + logging.error("dataset_quality: capability discovery failed: %s", e) + return self._ungraded(f"capability discovery failed: {e}") + try: + skills = resolve_skills(setup) + skills_error = None + except SkillCatalogError as e: + # Only trajectory_coverage reads the skills catalog, so only it drops. + logging.error("dataset_quality: skill resolution failed: %s", e) + skills, skills_error = [], str(e) + logging.info( + "dataset_quality: %d tools, %d skills declared", len(tools), len(skills) + ) + if not tools and not skills: + return self._ungraded( + f"{self.model_config_path} has no tools or skills configured" + ) context = DatasetQualityContext( product_name=self.product_name, scenarios=scenarios, tools=tools, + skills=skills, + skills_error=skills_error, ) contributions = self._run_scorers(context) @@ -241,6 +233,21 @@ def compare( )) return rows + def _ungraded(self, error: str) -> list[Tuple[str, float | None, str]]: + """One null-score row. Null, not 0, which would read as a genuine F.""" + return [( + self.name, + None, + json.dumps( + { + "product_name": self.product_name, + "graded": False, + "error": error, + }, + default=str, + ), + )] + def _run_scorers(self, context: DatasetQualityContext) -> dict: """Run every sub-scorer, returning ``{scorer: SubScoreContribution}``. @@ -301,10 +308,8 @@ def _extract_cujs(self, eval_results: Any) -> list[dict]: logging.warning("dataset_quality: no CUJs found in wrapper scenario") return scenarios - def _fetch_tools(self) -> list: + def _fetch_tools(self, setup: dict) -> list: """Query the product model config's MCP servers for the tool catalog.""" - model_config = load_yaml_config(self.model_config_path) - setup = model_config.get("setup") or {} if setup.get("extensions"): logging.warning( "dataset_quality: setup.extensions is not yet supported for " @@ -312,9 +317,9 @@ def _fetch_tools(self) -> list: ) mcp_servers = setup.get("mcp_servers") or {} if not mcp_servers: - raise McpToolsError( - f"{self.model_config_path} declares no setup.mcp_servers to query" - ) + # A skills-only product has no servers to query; the caller decides + # whether that leaves any capability surface to grade against. + return [] for attempt in range(1, _TOOL_FETCH_ATTEMPTS + 1): try: tools = AgentCliGenerator.fetch_mcp_tools( diff --git a/evalbench/scorers/dataset_quality/trajectory_coverage.py b/evalbench/scorers/dataset_quality/trajectory_coverage.py index 56b4ea78..9b18b00f 100644 --- a/evalbench/scorers/dataset_quality/trajectory_coverage.py +++ b/evalbench/scorers/dataset_quality/trajectory_coverage.py @@ -1,11 +1,12 @@ -"""Trajectory-coverage scorer: how many of the product's tools any CUJ exercises. +"""Trajectory-coverage scorer: how much of a product's surface any CUJ exercises. -Static (no judge). Tools referenced in a trajectory but absent from the schema -don't count toward coverage (that staleness is golden-validation's concern), so -the score is capped by the real catalog. +The catalog is every channel a product declares at once — MCP tools, the scripts +its skills ship, and the skills themselves — so a dataset written against one +channel isn't graded as though the others didn't exist. -Scope: the MCP tool channel only. Skills (expected_skills) have no static catalog -to score against, so skill coverage is a separate follow-up. +Names match verbatim, so one operation exposed through two channels is two +entries. A name no channel declares is ignored rather than flagged; stale +trajectories are golden-validation's concern. """ import logging @@ -15,44 +16,95 @@ DatasetQualityContext, SubScoreContribution, SubScorer, + expected_skills, expected_trajectory, ) +# Enough of a skill's description to say what a missing CUJ would cover, without +# a long gap list becoming a wall of text in the rendered report. +_DESCRIPTION_CHARS = 120 + + +def _describe(name: str, description: str) -> str: + """A gap entry, carrying the skill's description when the catalog has one. + + The synthesis pass may only reason from what the report states, so a bare + name would limit a recommendation to restating the name. + """ + # Frontmatter descriptions are routinely multi-line YAML blocks, but the gap + # list is one comma-joined line. + description = " ".join(description.split()) + if not description: + return name + if len(description) > _DESCRIPTION_CHARS: + description = description[:_DESCRIPTION_CHARS].rstrip() + "..." + return f"{name} ({description})" + + class TrajectoryCoverageScorer(SubScorer): - """Fraction of schema tools exercised by at least one expected_trajectory.""" + """Fraction of the product's capability catalog that some CUJ exercises.""" name = "trajectory_coverage" category = CATEGORY_TOOL_ACTIVATION default_weight = 20 def run(self, context: DatasetQualityContext) -> SubScoreContribution: - schema_tools = set(context.tool_names) - if not schema_tools: + if context.skills_error: + # Tools alone are a smaller denominator, which reads higher than truth. + logging.warning( + "trajectory_coverage: skipped; skills unresolved (%s)", + context.skills_error, + ) + return SubScoreContribution(applicable=False) + + operations = set(context.tool_names) | set(context.script_names) + skills = set(context.skill_names) + total = len(operations) + len(skills) + if not total: return SubScoreContribution(applicable=False) - trajectory_tools = set() + named_operations = set() + named_skills = set() for scenario in context.scenarios: - trajectory_tools.update(expected_trajectory(scenario)) + named_operations.update(expected_trajectory(scenario)) + named_skills.update(expected_skills(scenario)) - covered = schema_tools & trajectory_tools - uncovered = sorted(schema_tools - covered) - score = round(len(covered) / len(schema_tools) * 100) + covered_operations = operations & named_operations + covered_skills = skills & named_skills + covered = len(covered_operations) + len(covered_skills) + score = round(covered / total * 100) suggestions = [] - if uncovered: + if operations - covered_operations: + suggestions.append( + "No CUJ exercises these tools or scripts: " + + ", ".join(sorted(operations - covered_operations)) + ) + if skills - covered_skills: + descriptions = { + skill.name: skill.description for skill in context.skills + } suggestions.append( - "No CUJ exercises these tools: " + ", ".join(uncovered) + "No CUJ exercises these skills: " + + ", ".join( + _describe(name, descriptions.get(name, "")) + for name in sorted(skills - covered_skills) + ) ) logging.info( - "trajectory_coverage: \t%d/%d tools covered -> %d", - len(covered), len(schema_tools), score, + "trajectory_coverage: \t%d/%d covered (%d/%d operations, %d/%d " + "skills) -> %d", + covered, total, + len(covered_operations), len(operations), + len(covered_skills), len(skills), + score, ) return SubScoreContribution( score=score, metrics={ - "dq_covered_tools": len(covered), - "dq_total_tools": len(schema_tools), + "capabilities_covered": covered, + "capabilities_total": total, }, suggestions=suggestions, ) diff --git a/evalbench/scorers/dataset_quality/vague_examples.py b/evalbench/scorers/dataset_quality/vague_examples.py index 6a54bbfb..89f789cb 100644 --- a/evalbench/scorers/dataset_quality/vague_examples.py +++ b/evalbench/scorers/dataset_quality/vague_examples.py @@ -70,7 +70,7 @@ def run(self, context: DatasetQualityContext) -> SubScoreContribution: logging.info("vague_examples: \t%d/%d vague -> %d", n_vague, n, score) return SubScoreContribution( score=score, - metrics={"dq_vague_count": n_vague}, + metrics={"vague_cuj_count": n_vague}, suggestions=suggestions, evidence={KEY_VAGUE: vague_ids, "direct_ids": direct_ids}, ) diff --git a/evalbench/test/dataset_quality_test.py b/evalbench/test/dataset_quality_test.py index 0cffe864..871a3ba5 100644 --- a/evalbench/test/dataset_quality_test.py +++ b/evalbench/test/dataset_quality_test.py @@ -1,8 +1,9 @@ """Unit tests for the dataset-quality scoring flow. Covers the pieces whose contracts the rest of the flow depends on: the grading -rollup (``grading.py``), the judge-response parsing helpers (``llm.py``), the two -static sub-scorers, and the orchestrator's assembly of score rows +rollup (``grading.py``), the judge-response parsing helpers (``llm.py``), the +static sub-scorers and the skill catalog they grade against +(``skills_catalog.py``), and the orchestrator's assembly of score rows (``scorer.py``). Not yet covered: the five LLM judge sub-scorers, ``synthesis.py``, ``render.py``, @@ -13,7 +14,10 @@ import json import os +import shutil +import subprocess import sys +import tempfile import unittest from unittest.mock import patch @@ -23,10 +27,16 @@ sys.path.append(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) from generators.models.mcp_client import McpToolsError +from generators.models.skills_catalog import ( + Skill, + SkillCatalogError, + resolve_skills, +) from scorers.dataset_quality import llm from scorers.dataset_quality.composition import CompositionScorer from scorers.dataset_quality.context import ( CATEGORY_DISCOVERABILITY, + CATEGORY_TOOL_ACTIVATION, DatasetQualityContext, SubScoreContribution, SubScorer, @@ -43,7 +53,13 @@ KEY_SEQUENCE_DEPENDENCY, ) from scorers.dataset_quality.scorer import SCORER_REGISTRY, DatasetQualityScorer -from scorers.dataset_quality.trajectory_coverage import TrajectoryCoverageScorer +from scorers.dataset_quality.trajectory_coverage import ( + _DESCRIPTION_CHARS, + TrajectoryCoverageScorer, +) + + +_TRAJECTORY_WEIGHT = TrajectoryCoverageScorer.default_weight class _StubModel: @@ -69,9 +85,12 @@ def _tool(name, properties=None): ) -def _context(scenarios, tools): +def _context(scenarios, tools, skills=()): return DatasetQualityContext( - product_name="widget", scenarios=scenarios, tools=tools + product_name="widget", + scenarios=scenarios, + tools=tools, + skills=list(skills), ) @@ -287,13 +306,64 @@ def test_judges_return_none_on_an_objectless_array(self): class TrajectoryCoverageScorerTest(unittest.TestCase): - def test_no_schema_tools_is_inapplicable(self): + def test_no_tools_and_no_scripts_is_inapplicable(self): contribution = TrajectoryCoverageScorer({}, {}).run( _context([{"id": "c1", "expected_trajectory": ["alpha"]}], []) ) self.assertFalse(contribution.applicable) + def test_a_skills_only_product_is_scored_against_its_scripts(self): + # A skill groups operations rather than being one, so a trajectory names + # the scripts; without this the whole field goes ungraded. + context = _context( + [{"id": "c1", "expected_trajectory": ["list_instances.js"]}], + [], + [ + Skill("admin", scripts=("create_instance.js", "list_instances.js")), + Skill("data", scripts=("execute_sql.js", "list_instances.js")), + ], + ) + + contribution = TrajectoryCoverageScorer({}, {}).run(context) + + # list_instances.js ships in both skills but is one operation, so the + # catalog is 3 operations plus the 2 skills. + self.assertEqual(contribution.metrics["capabilities_total"], 5) + self.assertEqual(contribution.metrics["capabilities_covered"], 1) + self.assertEqual(contribution.score, 20) + self.assertIn("create_instance.js", contribution.suggestions[0]) + + def test_a_products_tools_and_skills_are_one_catalog(self): + # Both channels are installed together, so grading only the tools scores + # a skills-authored dataset against a surface it never names. + context = _context( + [{"id": "c1", "expected_trajectory": ["alpha"]}], + [_tool("alpha")], + [Skill("admin", scripts=("unrelated.js",))], + ) + + contribution = TrajectoryCoverageScorer({}, {}).run(context) + + self.assertEqual(contribution.metrics["capabilities_total"], 3) + self.assertEqual(contribution.score, 33) + + def test_a_skill_and_its_scripts_are_both_covered(self): + context = _context( + [{ + "id": "c1", + "expected_trajectory": ["list_instances.js"], + "expected_skills": ["admin"], + }], + [], + [Skill("admin", scripts=("list_instances.js",))], + ) + + contribution = TrajectoryCoverageScorer({}, {}).run(context) + + self.assertEqual(contribution.score, 100) + self.assertEqual(contribution.suggestions, []) + def test_tools_outside_the_schema_do_not_count_as_coverage(self): context = _context( [{"id": "c1", "expected_trajectory": ["alpha", "retired_tool"]}], @@ -303,8 +373,8 @@ def test_tools_outside_the_schema_do_not_count_as_coverage(self): contribution = TrajectoryCoverageScorer({}, {}).run(context) self.assertEqual(contribution.score, 25) - self.assertEqual(contribution.metrics["dq_covered_tools"], 1) - self.assertEqual(contribution.metrics["dq_total_tools"], 4) + self.assertEqual(contribution.metrics["capabilities_covered"], 1) + self.assertEqual(contribution.metrics["capabilities_total"], 4) self.assertIn("beta, delta, gamma", contribution.suggestions[0]) def test_a_string_trajectory_is_ignored_rather_than_split(self): @@ -335,6 +405,246 @@ def test_full_coverage_scores_100_without_suggestions(self): self.assertEqual(contribution.suggestions, []) +class ResolveSkillsTest(unittest.TestCase): + """The skill catalog trajectory_coverage scores against, read from setup.""" + + def setUp(self): + self.root = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, self.root, True) + + def _skill(self, parent, dir_name, frontmatter="", body="body", scripts=()): + path = os.path.join(parent, dir_name) + os.makedirs(path, exist_ok=True) + with open(os.path.join(path, "SKILL.md"), "w", encoding="utf-8") as f: + f.write(f"{frontmatter}{body}\n") + if scripts: + os.makedirs(os.path.join(path, "scripts"), exist_ok=True) + for script in scripts: + open(os.path.join(path, "scripts", script), "w").close() + return path + + @staticmethod + def _frontmatter(name, description): + return f"---\nname: {name}\ndescription: {description}\n---\n" + + def test_a_directory_holding_a_skill_md_is_one_skill(self): + self._skill(self.root, "solo", self._frontmatter("solo", "does solo")) + + skills = resolve_skills( + {"skills": [{"action": "link", "path": os.path.join(self.root, "solo")}]} + ) + + self.assertEqual(skills, [Skill("solo", "does solo")]) + + def test_a_skills_subdirectory_wins_over_the_root_children(self): + nested = os.path.join(self.root, "skills") + self._skill(nested, "alpha") + self._skill(self.root, "docs") + + skills = resolve_skills({"skills": [self.root]}) + + self.assertEqual([s.name for s in skills], ["alpha"]) + + def test_direct_children_are_scanned_without_a_skills_subdirectory(self): + self._skill(self.root, "alpha") + self._skill(self.root, "beta") + os.makedirs(os.path.join(self.root, "not-a-skill")) + + skills = resolve_skills({"skills_dir": self.root}) + + self.assertEqual([s.name for s in skills], ["alpha", "beta"]) + + def test_frontmatter_name_wins_over_the_directory_name(self): + self._skill( + self.root, "dir-name", self._frontmatter("declared-name", "d") + ) + + skills = resolve_skills({"skills_dir": self.root}) + + self.assertEqual(skills, [Skill("declared-name", "d")]) + + def test_absent_or_malformed_frontmatter_falls_back_to_the_directory(self): + self._skill(self.root, "bare") + self._skill(self.root, "broken", "---\nname: [unclosed\n---\n") + + skills = resolve_skills({"skills_dir": self.root}) + + self.assertEqual(skills, [Skill("bare", ""), Skill("broken", "")]) + + def test_an_entry_installing_a_subset_is_not_scored_against_the_rest(self): + for name in ("alpha", "beta", "gamma"): + self._skill(self.root, name) + + skills = resolve_skills( + {"skills": [{"path": self.root, "skills": ["alpha", "gamma"]}]} + ) + + self.assertEqual([s.name for s in skills], ["alpha", "gamma"]) + + def test_a_skills_scripts_are_read_as_its_operations(self): + self._skill( + self.root, "admin", scripts=("create_instance.js", "list_instances.js") + ) + + skills = resolve_skills({"skills_dir": self.root}) + + self.assertEqual( + skills[0].scripts, ("create_instance.js", "list_instances.js") + ) + + def test_an_undeclared_catalog_resolves_to_nothing(self): + self.assertEqual(resolve_skills({}), []) + + def test_a_narrowing_key_matching_nothing_is_fatal(self): + self._skill(self.root, "alpha") + + with self.assertRaises(SkillCatalogError): + resolve_skills({"skills": [{"path": self.root, "skill": "stale"}]}) + + def test_one_unmatched_name_is_fatal_even_when_the_others_match(self): + # Returning the two that matched would drop the third from the + # denominator and inflate coverage. + for name in ("alpha", "beta", "gamma"): + self._skill(self.root, name) + + with self.assertRaises(SkillCatalogError) as raised: + resolve_skills( + {"skills": [{"path": self.root, "skills": ["alpha", "typo", "gamma"]}]} + ) + + self.assertIn("typo", str(raised.exception)) + + def test_a_source_holding_no_skill_md_is_fatal(self): + os.makedirs(os.path.join(self.root, "docs")) + + with self.assertRaises(SkillCatalogError): + resolve_skills({"skills_dir": self.root}) + + def test_one_broken_entry_is_fatal_even_when_another_resolves(self): + # Checking only the combined total would pass here, leaving the catalog + # short of the product's real surface and inflating coverage. + working = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, working, True) + self._skill(working, "alpha") + + with self.assertRaises(SkillCatalogError): + resolve_skills({"skills": [working, os.path.join(self.root, "absent")]}) + + def test_an_entry_naming_no_source_is_fatal(self): + # Only a path or url resolves to a catalog; a bare name would have to be + # trusted rather than read, and no config declares skills that way. + for entry in ({"action": "install"}, {"action": "enable", "name": "a"}): + with self.subTest(entry=entry): + with self.assertRaises(SkillCatalogError): + resolve_skills({"skills": [entry]}) + + def test_the_same_skill_from_two_sources_is_listed_once(self): + self._skill(self.root, "alpha", self._frontmatter("Alpha", "first")) + other = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, other, True) + self._skill(other, "alpha", self._frontmatter("alpha", "second")) + + skills = resolve_skills({"skills": [self.root, other]}) + + self.assertEqual(skills, [Skill("Alpha", "first")]) + + @patch("generators.models.skills_catalog.subprocess.run") + def test_a_git_url_is_cloned_and_scanned(self, mock_run): + def clone(cmd, **kwargs): + self._skill(cmd[-1], "alpha", self._frontmatter("alpha", "cloned")) + + mock_run.side_effect = clone + + skills = resolve_skills({ + "skills": [{ + "action": "install_from_repo", + "url": "https://github.com/example/repo.git#main", + }] + }) + + self.assertEqual(skills, [Skill("alpha", "cloned")]) + self.assertIn("--branch", mock_run.call_args.args[0]) + + @patch("generators.models.skills_catalog.subprocess.run") + def test_a_failed_clone_is_fatal(self, mock_run): + mock_run.side_effect = subprocess.CalledProcessError(1, "git") + + with self.assertRaises(SkillCatalogError): + resolve_skills({"skills": ["https://github.com/example/repo.git"]}) + + @patch("generators.models.skills_catalog.subprocess.run") + def test_an_unresolvable_ref_is_not_retried_unpinned(self, mock_run): + # The retry would clone the default branch, silently grading against a + # revision nobody asked for. + mock_run.side_effect = subprocess.CalledProcessError(1, "git") + + with self.assertRaises(SkillCatalogError): + resolve_skills({"skills": ["https://github.com/example/repo.git#v2"]}) + self.assertEqual(mock_run.call_count, 1) + + +class ProseSkillCoverageTest(unittest.TestCase): + """Coverage for a skill catalog shipping no scripts, scored on expected_skills. + + Most skills in the wild are prose-only, so this is the common shape rather + than an edge case. + """ + + _CATALOG = [ + Skill("alpha", "runs alpha things"), + Skill("beta", "runs beta things"), + Skill("gamma"), + Skill("delta"), + ] + + def _run(self, scenarios, skills=None): + return TrajectoryCoverageScorer({}, {}).run( + _context(scenarios, [], self._CATALOG if skills is None else skills) + ) + + def test_skills_outside_the_catalog_do_not_count_as_coverage(self): + contribution = self._run( + [{"id": "c1", "expected_skills": ["alpha", "retired-skill"]}] + ) + + self.assertEqual(contribution.score, 25) + self.assertEqual(contribution.metrics["capabilities_covered"], 1) + self.assertEqual(contribution.metrics["capabilities_total"], 4) + + def test_a_gap_carries_the_catalog_description(self): + # Synthesis may only reason from the report, and for a prose-only skill + # the description is the sole record of what it does. + contribution = self._run([{"id": "c1", "expected_skills": ["alpha"]}]) + + self.assertIn("beta (runs beta things)", contribution.suggestions[0]) + self.assertIn("gamma", contribution.suggestions[0]) + + def test_a_long_multiline_description_stays_on_one_line(self): + # Frontmatter descriptions are routinely `description: |` blocks, which + # would otherwise break the comma-joined gap list across lines. + skill = Skill("beta", "runs\nbeta things " + "x" * _DESCRIPTION_CHARS) + + contribution = self._run([{"id": "c1", "expected_skills": []}], [skill]) + + gap = contribution.suggestions[0] + self.assertNotIn("\n", gap) + self.assertTrue(gap.endswith("...)")) + + def test_a_string_expected_skills_is_ignored_rather_than_split(self): + # Iterating a str registers each letter as a covered skill name. + contribution = self._run( + [{"id": "c1", "expected_skills": "alpha"}], [Skill("a")] + ) + + self.assertEqual(contribution.score, 0) + + def test_a_trajectory_does_not_cover_a_prose_skill(self): + # expected_trajectory names operations, which a prose skill has none of. + contribution = self._run([{"id": "c1", "expected_trajectory": ["alpha"]}]) + + self.assertEqual(contribution.score, 0) + + class CompositionScorerTest(unittest.TestCase): """Only the composable-surface gate; the judge itself is out of scope.""" @@ -417,7 +727,7 @@ def test_indirect_share_is_the_score(self): )) self.assertEqual(contribution.score, 67) - self.assertEqual(contribution.metrics["dq_tool_named_count"], 1) + self.assertEqual(contribution.metrics["tool_named_cuj_count"], 1) def test_only_the_starting_prompt_is_inspected(self): contribution = self._run([{ @@ -434,7 +744,7 @@ def test_no_suggestion_at_or_below_the_target_share(self): self._prompts("Use ls here", *["Find every config"] * 9) ) - self.assertEqual(at_target.metrics["dq_tool_named_count"], 1) + self.assertEqual(at_target.metrics["tool_named_cuj_count"], 1) self.assertEqual(at_target.suggestions, []) def test_suggestion_above_the_target_share(self): @@ -466,8 +776,14 @@ def _compare(scorer, generated_eval_result): ) -def _model_config(): - return {"setup": {"mcp_servers": {"widget": {"httpUrl": "https://x"}}}} +def _setup(**overrides): + setup = {"mcp_servers": {"widget": {"httpUrl": "https://x"}}} + setup.update(overrides) + return setup + + +def _model_config(**overrides): + return {"setup": _setup(**overrides)} def _config(**overrides): @@ -531,12 +847,17 @@ def test_sub_scorer_weight_overrides_the_default(self): self.assertEqual(scorer.scorers[0].weight, 42) - def test_registry_default_weights_are_a_budget_of_100(self): - # graded_weight/total_weight are only readable as a percentage of the - # rubric because the defaults sum to 100. + def test_registry_default_weights_are_a_fixed_budget(self): + # graded_weight/total_weight are only readable as a share of the rubric + # because the defaults sum to a fixed budget. total = sum(cls.default_weight for cls in SCORER_REGISTRY.values()) + activation = sum( + cls.default_weight for cls in SCORER_REGISTRY.values() + if cls.category == CATEGORY_TOOL_ACTIVATION + ) self.assertEqual(total, 100) + self.assertEqual(activation, _TRAJECTORY_WEIGHT) class ExtractCujsTest(unittest.TestCase): @@ -583,47 +904,86 @@ class FetchToolsTest(unittest.TestCase): def setUp(self): self.scorer = DatasetQualityScorer(_config(), {}) - @patch("scorers.dataset_quality.scorer.load_yaml_config") - def test_model_config_without_mcp_servers_raises(self, mock_load): - mock_load.return_value = {"setup": {}} - - with self.assertRaises(McpToolsError): - self.scorer._fetch_tools() + def test_a_setup_without_mcp_servers_has_nothing_to_query(self): + # Not an error: a skills-only product still has a surface to grade. + self.assertEqual(self.scorer._fetch_tools({}), []) @patch("scorers.dataset_quality.scorer.time.sleep") @patch("scorers.dataset_quality.scorer.AgentCliGenerator.fetch_mcp_tools") - @patch("scorers.dataset_quality.scorer.load_yaml_config") - def test_a_transient_failure_is_retried(self, mock_load, mock_fetch, _sleep): - mock_load.return_value = _model_config() + def test_a_transient_failure_is_retried(self, mock_fetch, _sleep): mock_fetch.side_effect = [McpToolsError("blip"), [_tool("alpha")]] - self.assertEqual(self.scorer._fetch_tools(), [_tool("alpha")]) + self.assertEqual(self.scorer._fetch_tools(_setup()), [_tool("alpha")]) self.assertEqual(mock_fetch.call_count, 2) @patch("scorers.dataset_quality.scorer.time.sleep") @patch("scorers.dataset_quality.scorer.AgentCliGenerator.fetch_mcp_tools") - @patch("scorers.dataset_quality.scorer.load_yaml_config") def test_repeated_failures_surface_after_the_last_attempt( - self, mock_load, mock_fetch, _sleep + self, mock_fetch, _sleep ): - mock_load.return_value = _model_config() mock_fetch.side_effect = McpToolsError("down") with self.assertRaises(McpToolsError): - self.scorer._fetch_tools() + self.scorer._fetch_tools(_setup()) self.assertEqual(mock_fetch.call_count, 3) @patch("scorers.dataset_quality.scorer.AgentCliGenerator.fetch_mcp_tools") - @patch("scorers.dataset_quality.scorer.load_yaml_config") - def test_an_empty_catalog_is_not_retried(self, mock_load, mock_fetch): - mock_load.return_value = _model_config() + def test_an_empty_catalog_is_not_retried(self, mock_fetch): mock_fetch.return_value = [] with self.assertRaises(McpToolsError): - self.scorer._fetch_tools() + self.scorer._fetch_tools(_setup()) self.assertEqual(mock_fetch.call_count, 1) +class ActivationCatalogTest(unittest.TestCase): + """End-to-end: a declared skills catalog reaches the scorer and is graded. + + The per-channel scoring itself is covered by the unit tests above; what runs + only here is the config -> resolve_skills -> context.skills wiring. + """ + + def setUp(self): + self.scorer = DatasetQualityScorer( + _config(sub_scorers={"trajectory_coverage": {}}), {} + ) + + @patch( + "scorers.dataset_quality.scorer.resolve_skills", + return_value=[Skill("s1"), Skill("s2")], + ) + @patch("scorers.dataset_quality.scorer.load_yaml_config") + def test_a_prose_only_skill_catalog_is_graded_on_the_skills( + self, mock_load, _skills + ): + # Most skills ship no scripts, so a product declaring only skills would + # otherwise go entirely ungraded on activation. + mock_load.return_value = {"setup": {"skills": ["repo"]}} + + rows = _compare( + self.scorer, _wrapper([{"id": "c1", "expected_skills": ["s1"]}]) + ) + + summary = json.loads(rows[0][2]) + self.assertEqual(summary["excluded_scorers"], []) + self.assertEqual(summary["dataset_quality_score"], 50) + + @patch("scorers.dataset_quality.scorer.resolve_skills", return_value=[]) + @patch("scorers.dataset_quality.scorer.load_yaml_config") + def test_a_product_declaring_neither_channel_scores_null( + self, mock_load, _skills + ): + mock_load.return_value = {"setup": {}} + + rows = _compare(self.scorer, _wrapper([{"id": "c1"}])) + + self.assertEqual(len(rows), 1) + self.assertIsNone(rows[0][1]) + self.assertIn( + "no tools or skills configured", json.loads(rows[0][2])["error"] + ) + + class DatasetQualityScorerCompareTest(unittest.TestCase): def test_an_empty_dataset_scores_null_rather_than_zero(self): @@ -653,7 +1013,33 @@ def test_tool_discovery_failure_scores_null_rather_than_zero( name, score, reason = rows[0] self.assertEqual(name, "dataset_quality") self.assertIsNone(score) - self.assertIn("tool discovery failed", json.loads(reason)["error"]) + self.assertIn("capability discovery failed", json.loads(reason)["error"]) + + @patch("scorers.dataset_quality.scorer.resolve_skills") + @patch("scorers.dataset_quality.scorer.AgentCliGenerator.fetch_mcp_tools") + @patch("scorers.dataset_quality.scorer.load_yaml_config") + def test_unresolvable_skills_drop_only_the_scorer_that_reads_them( + self, mock_load, mock_fetch, mock_skills + ): + mock_load.return_value = _model_config(skills=["gone"]) + mock_fetch.return_value = [_tool("alpha")] + mock_skills.side_effect = SkillCatalogError("skills path not found: gone") + scorer = DatasetQualityScorer( + _config( + sub_scorers={"trajectory_coverage": {}, "naming_distribution": {}} + ), + {}, + ) + + rows = _compare( + scorer, _wrapper([{"id": "c1", "starting_prompt": "make me a widget"}]) + ) + + summary = json.loads(rows[0][2]) + self.assertEqual(summary["excluded_scorers"], ["trajectory_coverage"]) + # naming_distribution never reads the catalog, so it still grades. + self.assertEqual(rows[0][1], 100.0) + self.assertEqual(summary["graded_weight"], 5) @patch.object( TrajectoryCoverageScorer,