diff --git a/datasets/mcp_readability/run_config.yaml b/datasets/mcp_readability/run_config.yaml index b8200197..8f6879de 100644 --- a/datasets/mcp_readability/run_config.yaml +++ b/datasets/mcp_readability/run_config.yaml @@ -29,6 +29,31 @@ scorers: model_config: datasets/model_configs/gemini_3.1_pro_model.yaml style_guide: datasets/mcp_readability/style_guide.md +# Carry-forward baselines (optional; omit the whole block to keep today's +# behaviour, which re-judges every endpoint on every run). +# +# The LLM judge is a thinking model: even at temperature 0 an unchanged tool +# surface can produce different findings, so issue counts drift with no +# explanation. With a baseline store configured, each endpoint's tool surface +# and judge inputs are fingerprinted; when nothing changed the previous findings +# are reused verbatim and the model is not called at all. Every deviation is +# reported in `mcp_readability_change_reason` (unchanged / tools_changed / +# style_guide_changed / model_changed / ...), so numbers move only for tools a +# team actually touched. +# +# Waiving a rule in `exceptions.yaml` changes the judge fingerprint and so +# re-judges that whole endpoint. +# baseline: +# store: bigquery # none (default) | local | bigquery +# # store: local # reads results//evals.csv from earlier runs; +# # results_dir: results # must match reporting.csv.output_directory +# max_age_days: 90 # re-derive a baseline older than this; 0 disables +# # expiry. Staggered per endpoint so they don't all +# # flip on the same day. Reported as baseline_expired. +# force_refresh: false # re-judge everything once (also +# # EVALBENCH_MCP_FORCE_REFRESH=1 for one-offs) +# # force_refresh_products: ["AlloyDB"] # or just these products + # Inputs (paths relative to the repo root / run cwd). endpoints_config: datasets/mcp_readability/endpoints.yaml exceptions_config: datasets/mcp_readability/exceptions.yaml # optional diff --git a/evalbench/evaluator/mcp_readability/baseline.py b/evalbench/evaluator/mcp_readability/baseline.py new file mode 100644 index 00000000..2c249d37 --- /dev/null +++ b/evalbench/evaluator/mcp_readability/baseline.py @@ -0,0 +1,314 @@ +"""Where the previous run's readability judgement is read back from. + +A baseline is the most recent persisted judgement for an endpoint. The scorer +compares this run's fingerprints against it and skips the model entirely when +nothing changed (see scorers.mcp_carry_forward). + +There are three stores. NullBaselineStore is the default and never finds one, +so every run is a full judge, exactly as today. LocalResultsBaselineStore scans +results/*/evals.csv, bounded by max_runs because that directory grows without +limit. BigQueryBaselineStore queries the shared results table; it is the repo's +first BigQuery read path and reuses the project and dataset resolution that +reporting.bqstore uses for writes. + +Two rules hold for all of them. Stores prime once and are then read from +memory, because _check_endpoint runs in a thread pool and a per-endpoint query +would be N round trips and racy against the store. And failure degrades rather +than aborts: a store error is logged and treated as "no baseline", costing a +re-judge and nothing else. That is a deliberate exception to the orchestrator's +fail-fast handling, since a baseline is an optimisation rather than a +measurement, and the identity writing results may have no read permission on +the dataset. +""" + +from abc import ABC, abstractmethod +import csv +import datetime +import glob +import hashlib +import json +import logging +import os + +from scorers.mcp_carry_forward import Baseline + + +# Result columns a baseline is reconstructed from. +_KEY = "mcp_readability_endpoint_key" +_TOOL_FINGERPRINTS = "mcp_readability_tool_fingerprints_json" +_JUDGE_FINGERPRINT = "mcp_readability_judge_fingerprint" +_JUDGE_COMPONENTS = "mcp_readability_judge_components_json" +_FEEDBACK = "mcp_readability_llm_feedback_json" +_SCORE = "mcp_readability_score" +_PROVENANCE = "mcp_readability_feedback_provenance_json" +_TIMESTAMP_UTC = "mcp_readability_check_timestamp_utc" +_TIMESTAMP_NAIVE = "mcp_readability_check_timestamp" +_JOB_ID = "job_id" + +# Newest-first scan cap for the local store: results/ is never pruned, and a +# baseline older than this many runs is not worth the file I/O. +_DEFAULT_MAX_RUNS = 200 + +# How far back the BigQuery store looks. Anything older than the longest +# sensible max_age_days would be discarded by the expiry check anyway. +_LOOKBACK_DAYS = 400 + + +class BaselineStore(ABC): + """Prefetch baselines for a run, then serve them from memory.""" + + @abstractmethod + def prime(self, endpoint_keys: list[str]) -> None: + """Fetch the baseline for every endpoint in this run, once.""" + pass + + @abstractmethod + def load(self, endpoint_key: str) -> Baseline | None: + """Return the primed baseline for one endpoint, or None if absent.""" + pass + + +class NullBaselineStore(BaselineStore): + """No baselines: every endpoint is judged in full.""" + + def prime(self, endpoint_keys: list[str]) -> None: + return None + + def load(self, endpoint_key: str) -> Baseline | None: + return None + + +class _RowBaselineStore(BaselineStore): + """Shared reconstruction of baselines from result rows. + + Abstract; subclasses supply prime(), which decides where the rows come from. + """ + + def __init__(self): + self._baselines: dict[str, Baseline] = {} + + def load(self, endpoint_key: str) -> Baseline | None: + return self._baselines.get(endpoint_key) + + def _absorb(self, row: dict, wanted: set[str]) -> None: + """Keep row as the baseline for its endpoint if it is the newest.""" + key = (row.get(_KEY) or "").strip() + if not key or key not in wanted: + return + timestamp = _row_timestamp(row) + existing = self._baselines.get(key) + if existing is not None and existing.check_timestamp >= timestamp: + return + self._baselines[key] = Baseline( + endpoint_key=key, + job_id=(row.get(_JOB_ID) or "").strip(), + check_timestamp=timestamp, + judge_fingerprint=(row.get(_JUDGE_FINGERPRINT) or "").strip(), + judge_components=_load_json(row.get(_JUDGE_COMPONENTS), {}), + tool_fingerprints=_load_json(row.get(_TOOL_FINGERPRINTS), {}), + feedback=_load_json(row.get(_FEEDBACK), {}), + readability_score=_safe_int(row.get(_SCORE)), + provenance=_load_json(row.get(_PROVENANCE), {}), + ) + + +class LocalResultsBaselineStore(_RowBaselineStore): + """Baselines read from /*/evals.csv.""" + + def __init__(self, results_dir: str = "results", + max_runs: int = _DEFAULT_MAX_RUNS): + super().__init__() + self.results_dir = results_dir + self.max_runs = max(1, int(max_runs)) + + def prime(self, endpoint_keys: list[str]) -> None: + wanted = {k for k in endpoint_keys if k} + if not wanted: + return + pattern = os.path.join(self.results_dir, "*", "evals.csv") + # Newest mtime first, capped: the newest run holding a given endpoint + # wins, and _absorb's timestamp comparison settles ties. + paths = sorted( + glob.glob(pattern), key=_safe_mtime, reverse=True + )[: self.max_runs] + for path in paths: + try: + with open(path, "r", encoding="utf-8", newline="") as f: + for row in csv.DictReader(f): + self._absorb(row, wanted) + except (OSError, csv.Error) as e: + logging.warning( + "mcp_readability: could not read baseline %s: %s", path, e + ) + + +class BigQueryBaselineStore(_RowBaselineStore): + """Baselines read from the shared ..results table.""" + + def __init__(self, gcp_project_id: str = "", dataset_id: str = "evalbench"): + super().__init__() + self.gcp_project_id = gcp_project_id + self.dataset_id = dataset_id or "evalbench" + + def prime(self, endpoint_keys: list[str]) -> None: + wanted = {k for k in endpoint_keys if k} + if not wanted: + return + from google.cloud import bigquery + from util.gcp import get_gcp_project + + project = get_gcp_project(self.gcp_project_id) + client = bigquery.Client(project=project) + columns = ", ".join( + [ + _KEY, + _TIMESTAMP_UTC, + _TIMESTAMP_NAIVE, + _JUDGE_FINGERPRINT, + _JUDGE_COMPONENTS, + _TOOL_FINGERPRINTS, + _FEEDBACK, + _SCORE, + _PROVENANCE, + _JOB_ID, + ] + ) + # One row per endpoint: the newest by UTC timestamp, falling back to the + # older naive local-time column for rows written before it existed. + # + # The lookback filters on the readability timestamp, not on the shared + # run_time column: readability rows do not populate run_time, so a + # predicate on it would silently match nothing. Both timestamp columns + # are ISO-8601 strings, which order lexicographically by date. + query = f""" + SELECT * EXCEPT(row_num) FROM ( + SELECT {columns}, ROW_NUMBER() OVER ( + PARTITION BY {_KEY} + ORDER BY COALESCE({_TIMESTAMP_UTC}, {_TIMESTAMP_NAIVE}) DESC + ) AS row_num + FROM `{project}.{self.dataset_id}.results` + WHERE {_KEY} IN UNNEST(@keys) + AND COALESCE({_TIMESTAMP_UTC}, {_TIMESTAMP_NAIVE}) >= @cutoff + ) WHERE row_num = 1 + """ + cutoff = ( + datetime.datetime.now(datetime.timezone.utc) + - datetime.timedelta(days=_LOOKBACK_DAYS) + ).isoformat() + job_config = bigquery.QueryJobConfig( + query_parameters=[ + bigquery.ArrayQueryParameter("keys", "STRING", sorted(wanted)), + bigquery.ScalarQueryParameter("cutoff", "STRING", cutoff), + ] + ) + for row in client.query(query, job_config=job_config).result(): + self._absorb(dict(row), wanted) + + +def build_store(config: dict) -> BaselineStore: + """Build the store named by the run config's baseline block. + + An absent or empty block yields a NullBaselineStore, so a config that has + not opted in behaves exactly as it does today. An unknown store name raises, + matching the orchestrator's fail-fast handling of unknown config values. + """ + config = config or {} + store = str(config.get("store") or "none").strip().lower() + if store in ("", "none", "off", "disabled"): + return NullBaselineStore() + if store == "local": + return LocalResultsBaselineStore( + results_dir=config.get("results_dir") or "results", + max_runs=int(config.get("max_runs") or _DEFAULT_MAX_RUNS), + ) + if store == "bigquery": + return BigQueryBaselineStore( + gcp_project_id=config.get("gcp_project_id") or "", + dataset_id=config.get("dataset_id") or "evalbench", + ) + raise ValueError( + f"mcp_readability: unknown baseline store {store!r}; " + "allowed: none, local, bigquery" + ) + + +def is_expired(baseline: Baseline, max_age_days: int, now=None) -> bool: + """Whether a baseline is too old to reuse. + + Expiry is staggered by the endpoint key so a fleet of endpoints primed on + the same day does not all re-judge on the same later day. This does break + the strict invariant, which is acceptable only because the resulting run + reports baseline_expired, making it an announced re-judge rather than an + unexplained count change. + + max_age_days of 0 disables expiry entirely. now defaults to wall-clock UTC + and is injected by tests. + """ + if max_age_days <= 0: + return False + stamp = _parse_timestamp(baseline.check_timestamp) + if stamp is None: + # An unparseable timestamp cannot prove freshness. + return True + now = now or datetime.datetime.now(datetime.timezone.utc) + offset = _stagger(baseline.endpoint_key, max_age_days) + return (now - stamp).days >= max_age_days + offset + + +def _stagger(endpoint_key: str, max_age_days: int) -> int: + """Return a deterministic per-endpoint offset in [0, max_age_days). + + Hashed rather than read as hex, because an endpoint may carry an explicit + id: from endpoints.yaml, which is arbitrary text. + """ + if not endpoint_key: + return 0 + digest = hashlib.sha256(endpoint_key.encode("utf-8")).hexdigest() + return int(digest[:8], 16) % max_age_days + + +def _row_timestamp(row: dict) -> str: + return ( + (row.get(_TIMESTAMP_UTC) or "").strip() + or (row.get(_TIMESTAMP_NAIVE) or "").strip() + ) + + +def _parse_timestamp(value: str) -> datetime.datetime | None: + if not value: + return None + try: + stamp = datetime.datetime.fromisoformat(str(value).replace("Z", "+00:00")) + except ValueError: + return None + if stamp.tzinfo is None: + # Pre-existing rows recorded naive local time; assume UTC rather than + # discard them, since the only use is a coarse age comparison. + stamp = stamp.replace(tzinfo=datetime.timezone.utc) + return stamp + + +def _load_json(value, default): + if value in (None, "", "null"): + return default + if isinstance(value, (dict, list)): + return value + try: + parsed = json.loads(value) + except (TypeError, ValueError): + return default + return parsed if isinstance(parsed, type(default)) else default + + +def _safe_int(value) -> int: + try: + return int(float(value)) + except (TypeError, ValueError): + return 0 + + +def _safe_mtime(path: str) -> float: + try: + return os.path.getmtime(path) + except OSError: + return 0.0 diff --git a/evalbench/evaluator/mcp_readability/orchestrator.py b/evalbench/evaluator/mcp_readability/orchestrator.py index 97b084b5..19bde6d5 100644 --- a/evalbench/evaluator/mcp_readability/orchestrator.py +++ b/evalbench/evaluator/mcp_readability/orchestrator.py @@ -28,18 +28,23 @@ import concurrent.futures import datetime +import hashlib import json import logging +import os import tempfile import threading from evaluator.orchestrator import Orchestrator from generators.models import get_generator +from scorers import mcp_carry_forward as cf +from scorers.mcp_fingerprint import tool_fingerprints, toolset_fingerprint from scorers.mcp_readability_scoring import EndpointContext from scorers.mcp_style_readability import McpStyleReadabilityScorer from scorers.mcp_tool_metrics import McpToolMetricsScorer from util.config import load_yaml_config +from evaluator.mcp_readability import baseline as baseline_mod from evaluator.mcp_readability import exceptions as exceptions_mod @@ -70,6 +75,16 @@ "mcp_readability_source_url", "mcp_readability_endpoint_type", "mcp_readability_check_timestamp", + # Unambiguous run ordering. The column above is naive local time, so rows + # written from google3 and from OSS interleave wrongly when sorted; the + # baseline store orders on this one and falls back to the naive column only + # for rows written before it existed. + "mcp_readability_check_timestamp_utc", + # Carry-forward identity: which endpoint this row is about, and the exact + # tool surface it was judged against. + "mcp_readability_endpoint_key", + "mcp_readability_toolset_fingerprint", + "mcp_readability_tool_fingerprints_json", "job_id", ] @@ -112,6 +127,23 @@ def __init__(self, config, db_configs, setup_config, report_progress=False): config.get("exceptions_config") ) + # Carry-forward baselines. An absent baseline block yields the null + # store, so every endpoint is judged in full exactly as today and the + # google3-mirrored run config keeps working until it opts in. + baseline_config = config.get("baseline") or {} + self.baseline_store = baseline_mod.build_store(baseline_config) + self.baseline_max_age_days = int( + baseline_config.get("max_age_days", 90) + ) + self.force_refresh = _force_refresh(baseline_config) + self.force_refresh_products = { + str(p).strip().lower() + for p in (baseline_config.get("force_refresh_products") or []) + } + # Set when priming the store fails, so every endpoint reports + # baseline_unavailable rather than silently looking like a first run. + self.baseline_unavailable = False + # Optional endpoint_type filter (validated against the allowed set). type_filter = config.get("endpoint_types") or [] self.endpoint_type_filter = { @@ -176,6 +208,10 @@ def evaluate(self, dataset=None): self.score_rows = [] return + # Prefetch every baseline on one thread before the pool starts: the + # per-endpoint alternative is N round trips racing each other. + self._prime_baselines(endpoints) + workers = max(1, int(self.endpoint_runners)) results = [] # list[(row, score_rows)] with concurrent.futures.ThreadPoolExecutor(max_workers=workers) as pool: @@ -231,23 +267,40 @@ def _check_endpoint(self, endpoint: dict): row = self._base_row(product_name, endpoint_url, endpoint_type) row["job_id"] = self.job_id + endpoint_key = _endpoint_key( + endpoint, product_name, endpoint_url, endpoint_type + ) + row["mcp_readability_endpoint_key"] = endpoint_key try: # 1. Fetch tools + render man-page markup. tools, man_page = self.tools_generator.fetch_tools(endpoint) + # 2. Fingerprint the tool surface. Written on every row regardless + # of whether carry-forward is enabled, so turning it on later finds + # usable baselines already in place. + fingerprints = tool_fingerprints(tools) + row["mcp_readability_toolset_fingerprint"] = toolset_fingerprint( + fingerprints + ) + row["mcp_readability_tool_fingerprints_json"] = json.dumps( + fingerprints, sort_keys=True + ) - # 2. Exceptions (waivers) for this endpoint. + # 3. Exceptions (waivers) for this endpoint. applicable = exceptions_mod.applicable_exceptions( endpoint, self.all_exceptions ) - # 3. Run every configured scorer against the shared context. + # 4. Run every configured scorer against the shared context. context = EndpointContext( product_name=product_name, endpoint=endpoint, tools=tools, man_page=man_page, exceptions=applicable, + baseline=self._baseline_context( + endpoint, endpoint_key, fingerprints + ), ) score_rows = [] for scorer in self.scorers: @@ -272,6 +325,80 @@ def _check_endpoint(self, endpoint: dict): return row, score_rows + # ------------------------------------------------------------------ + # Carry-forward baselines + # ------------------------------------------------------------------ + def _prime_baselines(self, endpoints: list[dict]) -> None: + """Prefetch the previous judgement for every endpoint in this run. + + A store failure is logged and downgraded to "no baselines", which costs + a full re-judge and nothing else. That is a deliberate exception to this + orchestrator's fail-fast ethos: a baseline is an optimisation, not a + measurement, and the identity writing results may lack read permission. + """ + keys = [ + _endpoint_key( + ep, + ep.get("product_name", ""), + self._endpoint_ref(ep), + _validate_endpoint_type(ep.get("endpoint_type")), + ) + for ep in endpoints + ] + try: + self.baseline_store.prime(keys) + except Exception: + self.baseline_unavailable = True + logging.exception( + "mcp_readability: could not load baselines; re-judging every " + "endpoint in full." + ) + + def _baseline_context( + self, endpoint: dict, endpoint_key: str, fingerprints: dict + ): + context = cf.BaselineContext( + endpoint_key=endpoint_key, + job_id=self.job_id, + tool_fingerprints=fingerprints, + toolset_fingerprint=toolset_fingerprint(fingerprints), + ) + if self.baseline_unavailable: + context.override_reason = cf.BASELINE_UNAVAILABLE + return context + try: + found = self.baseline_store.load(endpoint_key) + except Exception: + logging.exception( + "mcp_readability: could not load the baseline for %s; " + "re-judging in full.", + endpoint_key, + ) + context.override_reason = cf.BASELINE_UNAVAILABLE + return context + # Attached even when it must not be reused: an expired or force-refreshed + # baseline is still what "since the previous review" is measured against. + # The override reason is what stops its findings being carried. + context.baseline = found + if self._forced(endpoint): + context.override_reason = cf.FORCED_REFRESH + elif found is not None and baseline_mod.is_expired( + found, self.baseline_max_age_days + ): + context.override_reason = cf.BASELINE_EXPIRED + return context + + def _forced(self, endpoint: dict) -> bool: + """Whether this endpoint must be re-judged regardless of its baseline. + + Carry-forward otherwise entrenches the first run's draw: a hallucinated + P0 could only be cleared by editing the tool. This is the escape hatch. + """ + if self.force_refresh: + return True + product = str(endpoint.get("product_name", "")).strip().lower() + return bool(product) and product in self.force_refresh_products + # ------------------------------------------------------------------ # Helpers # ------------------------------------------------------------------ @@ -314,5 +441,48 @@ def _base_row(product_name, endpoint_url, endpoint_type) -> dict: "mcp_readability_check_timestamp": ( datetime.datetime.now().isoformat() ), + "mcp_readability_check_timestamp_utc": ( + datetime.datetime.now(datetime.timezone.utc) + .isoformat() + .replace("+00:00", "Z") + ), "job_id": "", } + + +def _endpoint_key( + endpoint: dict, + product_name: str, + endpoint_url: str, + endpoint_type: str, +) -> str: + """Return the stable identity a baseline is looked up by. + + Derived from the same identity fields the row already reports, so no config + change is needed to start using carry-forward. The cost is that renaming a + product orphans its history. Set an explicit id: on the endpoint to survive + renames; the rename then surfaces as endpoint_identity_changed rather than + silently looking like a brand-new endpoint. + """ + explicit = endpoint.get("id") + if explicit: + return str(explicit).strip() + raw = "|".join( + [product_name or "", endpoint_url or "", endpoint_type or ""] + ) + return hashlib.sha256(raw.encode("utf-8")).hexdigest() + + +def _force_refresh(baseline_config: dict) -> bool: + """Whether every endpoint must be re-judged this run. + + The environment variable exists for Guitar one-offs, where editing the + mirrored run config is not practical. + """ + if os.environ.get("EVALBENCH_MCP_FORCE_REFRESH", "").strip().lower() in ( + "1", + "true", + "yes", + ): + return True + return bool(baseline_config.get("force_refresh")) diff --git a/evalbench/generators/models/mcp_tool_formatter.py b/evalbench/generators/models/mcp_tool_formatter.py index 9912a560..e5e23f50 100644 --- a/evalbench/generators/models/mcp_tool_formatter.py +++ b/evalbench/generators/models/mcp_tool_formatter.py @@ -234,6 +234,37 @@ def _format_props( return lines +def format_tool_section(tool: mcp_types.Tool) -> str: + """Formats a single tool as its man-page section. + + Exposed separately from format_tools_to_man_page so callers can fingerprint + the exact bytes the judge sees for one tool. Fingerprinting the rendered + section rather than the Tool fields makes "fingerprints are equal" and "the + judge saw identical text" the same statement, instead of relying on a + hand-maintained field list staying in sync with this renderer. + + The returned section is not stripped: the man page is the sections joined + with newlines, so leading/trailing blank lines are the join's concern. + """ + lines = [ + "=" * 80, + f"TOOL: {tool.name}", + "=" * 80, + "DESCRIPTION:", + ] + + desc = tool.description or "No description provided." + lines.extend(_wrap_text(desc, " ")) + + lines.append("\nPARAMETERS:") + if tool.inputSchema and tool.inputSchema.get("properties"): + lines.extend(_format_props(tool.inputSchema)) + else: + lines.append(" None\n") + + return "\n".join(lines) + + def format_tools_to_man_page(tools: Sequence[mcp_types.Tool]) -> str: """Formats a sequence of MCP tools into a man-page style string. @@ -247,21 +278,4 @@ def format_tools_to_man_page(tools: Sequence[mcp_types.Tool]) -> str: if not tools: return "No tools available." - lines = [] - for tool in tools: - lines.append("=" * 80) - lines.append(f"TOOL: {tool.name}") - lines.append("=" * 80) - lines.append("DESCRIPTION:") - - desc = tool.description or "No description provided." - lines.extend(_wrap_text(desc, " ")) - - lines.append("\nPARAMETERS:") - if tool.inputSchema and tool.inputSchema.get("properties"): - schema_lines = _format_props(tool.inputSchema) - lines.extend(schema_lines) - else: - lines.append(" None\n") - - return "\n".join(lines).strip() + return "\n".join(format_tool_section(tool) for tool in tools).strip() diff --git a/evalbench/scorers/mcp_carry_forward.py b/evalbench/scorers/mcp_carry_forward.py new file mode 100644 index 00000000..7381ef5e --- /dev/null +++ b/evalbench/scorers/mcp_carry_forward.py @@ -0,0 +1,554 @@ +"""Deciding what to re-judge, and merging carried findings with fresh ones. + +The invariant: given the same endpoint, the same per-tool fingerprints and the +same judge fingerprint, the emitted feedback is byte-identical to the previous +run and zero model calls are made. Every departure from it is reported with a +named reason (see CHANGE_REASONS), derived from an exact set-difference over +fingerprint components rather than inferred. + +The judge already groups its findings per tool, so the skip is per tool too: +only tools whose rendered surface changed are re-judged, and a team's numbers +move for the tools they actually touched. + +This sits under scorers/ rather than evaluator/mcp_readability/ because that +package's __init__ imports the orchestrator, which imports the scorers, so a +scorer importing from it would be a circular import. The baseline store, which +only the orchestrator touches, does live there and imports Baseline from here. +""" + +import copy +from dataclasses import dataclass, field +import hashlib +import re +from typing import Any + +from scorers.mcp_fingerprint import component_diff + + +# How this run's feedback was produced. +MODE_FULL_JUDGE = "full_judge" # every tool judged +MODE_PARTIAL = "partial" # some tools judged, the rest carried +MODE_CARRIED = "carried" # nothing judged, no model call + + +# Why this run's feedback may differ from the previous one. Exactly one is +# reported per endpoint, in mcp_readability_change_reason. +UNCHANGED = "unchanged" +TOOLS_CHANGED = "tools_changed" +STYLE_GUIDE_CHANGED = "style_guide_changed" +MODEL_CHANGED = "model_changed" +WAIVERS_CHANGED = "waivers_changed" +PROMPT_CHANGED = "prompt_changed" +BASELINE_EXPIRED = "baseline_expired" +FORCED_REFRESH = "forced_refresh" +ENDPOINT_IDENTITY_CHANGED = "endpoint_identity_changed" +BASELINE_UNAVAILABLE = "baseline_unavailable" +NO_BASELINE = "no_baseline" + +CHANGE_REASONS = ( + UNCHANGED, + TOOLS_CHANGED, + STYLE_GUIDE_CHANGED, + MODEL_CHANGED, + WAIVERS_CHANGED, + PROMPT_CHANGED, + BASELINE_EXPIRED, + FORCED_REFRESH, + ENDPOINT_IDENTITY_CHANGED, + BASELINE_UNAVAILABLE, + NO_BASELINE, +) + + +# Which judge component maps to which reported reason. Ordered: when several +# components change at once the earliest match wins, so the most consequential +# (and most likely to explain a count swing) is the one reported. +_COMPONENT_REASONS = ( + ("judge_model", MODEL_CHANGED), + ("style_guide_sha", STYLE_GUIDE_CHANGED), + ("style_guide_path", STYLE_GUIDE_CHANGED), + ("prompt_version", PROMPT_CHANGED), + ("prompt_sha", PROMPT_CHANGED), + ("scorer_name", PROMPT_CHANGED), + ("exceptions", WAIVERS_CHANGED), + ("product_name", ENDPOINT_IDENTITY_CHANGED), +) + +# The judge's entry for issues that belong to no individual tool. +GENERAL = "general" + + +@dataclass +class Baseline: + """The previous run's judgement for one endpoint, as loaded from a store.""" + + endpoint_key: str + job_id: str = "" + # Prefers the UTC _check_timestamp_utc column, falling back to the older + # naive local-time column when reading pre-existing rows. + check_timestamp: str = "" + judge_fingerprint: str = "" + judge_components: dict = field(default_factory=dict) + tool_fingerprints: dict = field(default_factory=dict) + # The public feedback dict (findings_by_tool / waived / summary): the score + # is stripped from that column, so it is carried separately. + feedback: dict = field(default_factory=dict) + readability_score: int = 0 + provenance: dict = field(default_factory=dict) + + +@dataclass +class BaselineContext: + """What the orchestrator hands the scorer so it can skip work. + + Carried on EndpointContext as a single baseline field. override_reason is + set when the orchestrator already knows the baseline must not be used + (expiry, a forced refresh, or a store that could not be read); the scorer + then re-judges in full and reports that reason instead of guessing at one. + """ + + endpoint_key: str = "" + job_id: str = "" + tool_fingerprints: dict = field(default_factory=dict) + toolset_fingerprint: str = "" + baseline: Baseline | None = None + override_reason: str = "" + + +@dataclass +class Decision: + """What to judge for one endpoint, and how to explain the outcome.""" + + mode: str = MODE_FULL_JUDGE + change_reason: str = NO_BASELINE + rejudged_tools: list[str] = field(default_factory=list) + carried_tools: list[str] = field(default_factory=list) + added_tools: list[str] = field(default_factory=list) + removed_tools: list[str] = field(default_factory=list) + component_diff: list[str] = field(default_factory=list) + judge_components: dict = field(default_factory=dict) + baseline: Baseline | None = None + + @property + def needs_model_call(self) -> bool: + return self.mode != MODE_CARRIED + + @property + def names_changed(self) -> bool: + """Whether the set of tool names changed, not just their contents. + + A rename, addition or removal is what creates a cross-tool + inconsistency, so this decides whether the general entry is re-derived + or carried. + """ + return bool(self.added_tools or self.removed_tools) + + @property + def tools_to_report(self) -> list[str]: + """What the judge is asked to report on in a partial run. + + Includes general when the tool-name set changed. The focus clause tells + the judge to emit entries only for the names listed here, so omitting it + would suppress the cross-tool findings a rename or removal is most + likely to create, and _merge_partial would then source general from an + empty judged response and lose the baseline's copy too. + """ + names = set(self.rejudged_tools) | set(self.added_tools) + if self.names_changed: + names.add(GENERAL) + return sorted(names) + + @property + def accepted_tools(self) -> set[str]: + """Per-tool entries the judge's output is trusted for. + + Excludes general, which _merge_partial decides on separately. + """ + return set(self.rejudged_tools) | set(self.added_tools) + + +def decide(context: BaselineContext | None, judge_fingerprint: str) -> Decision: + """Choose full / partial / carried for one endpoint.""" + if context is None: + return Decision(mode=MODE_FULL_JUDGE, change_reason=NO_BASELINE) + + current = context.tool_fingerprints or {} + if context.override_reason: + # The baseline stays attached even though its findings are not reused: + # it is still the reference for "what changed since last time". + # Dropping it would report every finding as new and blank the date in + # the banner, on exactly the runs (expiry, forced refresh) where the + # reader most needs to know what moved. + return Decision( + mode=MODE_FULL_JUDGE, + change_reason=context.override_reason, + rejudged_tools=list(current), + baseline=context.baseline, + ) + + baseline = context.baseline + if baseline is None: + return Decision( + mode=MODE_FULL_JUDGE, + change_reason=NO_BASELINE, + rejudged_tools=list(current), + ) + + if judge_fingerprint != baseline.judge_fingerprint: + # A changed judge invalidates every finding, not just the ones for + # changed tools: the same tool can be judged differently by a new model + # or against a new guide. decide_with_components names which input + # changed; on its own decide() can only say that one did. + return Decision( + mode=MODE_FULL_JUDGE, + change_reason=PROMPT_CHANGED, + rejudged_tools=list(current), + baseline=baseline, + ) + + previous = baseline.tool_fingerprints or {} + added = [name for name in current if name not in previous] + removed = [name for name in previous if name not in current] + changed = [ + name + for name in current + if name in previous and current[name] != previous[name] + ] + unchanged = [ + name + for name in current + if name in previous and current[name] == previous[name] + ] + + if not added and not removed and not changed: + return Decision( + mode=MODE_CARRIED, + change_reason=UNCHANGED, + carried_tools=unchanged, + baseline=baseline, + ) + + return Decision( + mode=MODE_PARTIAL, + change_reason=TOOLS_CHANGED, + rejudged_tools=changed, + carried_tools=unchanged, + added_tools=added, + removed_tools=removed, + baseline=baseline, + ) + + +def decide_with_components( + context: BaselineContext | None, + judge_fingerprint: str, + judge_components: dict, +) -> Decision: + """decide(), with the component diff computed against context. + + Split out so decide() stays testable with a fingerprint alone. This is what + the scorer calls. + """ + decision = decide(context, judge_fingerprint) + decision.judge_components = dict(judge_components or {}) + baseline = context.baseline if context else None + if ( + baseline is not None + and decision.mode == MODE_FULL_JUDGE + and not context.override_reason + ): + if not baseline.judge_components: + # A baseline written before components were recorded, or one whose + # component JSON did not parse. Diffing against {} would report + # every key as changed and blame whichever happens to be first in + # _COMPONENT_REASONS, so say plainly that it could not be compared. + decision.component_diff = [] + decision.change_reason = BASELINE_UNAVAILABLE + else: + decision.component_diff = component_diff( + baseline.judge_components, judge_components + ) + decision.change_reason = _reason_for_components( + decision.component_diff + ) + return decision + + +def _reason_for_components(diff: list[str]) -> str: + for component, reason in _COMPONENT_REASONS: + if component in diff: + return reason + if diff: + # Some component changed that predates this mapping; "the prompt inputs + # changed" is the accurate superset. + return PROMPT_CHANGED + # The fingerprint differs but no individual component does. A baseline + # exists, so this is not a first run -- it simply cannot be compared. + return BASELINE_UNAVAILABLE + + +def merge_feedback( + decision: Decision, + judged: dict | None, + tool_order: list[str], +) -> dict: + """Combine carried baseline findings with the judge's fresh ones. + + judged is None in carried mode, where no model call was made. Merged entries + follow the current man-page order with the general entry first, so the + output ordering does not depend on which tools happened to be re-judged. + + Returns findings_by_tool, waived and summary. Counts are left out; the + caller recomputes them over the merged findings. + """ + baseline = decision.baseline + baseline_feedback = (baseline.feedback if baseline else {}) or {} + + if decision.mode == MODE_CARRIED: + merged = { + "findings_by_tool": _entries(baseline_feedback), + "waived": baseline_feedback.get("waived") or [], + "summary": baseline_feedback.get("summary", ""), + } + elif decision.mode == MODE_PARTIAL: + merged = _merge_partial(decision, judged or {}, baseline_feedback) + else: + merged = { + "findings_by_tool": _entries(judged or {}), + "waived": (judged or {}).get("waived") or [], + "summary": (judged or {}).get("summary", ""), + } + + merged["findings_by_tool"] = _ordered(merged["findings_by_tool"], tool_order) + return merged + + +def _merge_partial( + decision: Decision, judged: dict, baseline_feedback: dict +) -> dict: + """Accept judge entries only for changed or added tools; carry the rest. + + The judge is shown the whole man page in a partial run, because its severity + calibration is relative to the full surface and judging a lone tool inflates + severity. It is merely asked to report on the changed ones. Filtering here, + not the prompt, is what provides the guarantee; the prompt clause only trims + output tokens. + """ + accept = decision.accepted_tools + carry = set(decision.carried_tools) + + merged_entries = [] + for entry in _entries(judged): + if entry["tool"] in accept: + merged_entries.append(entry) + for entry in _entries(baseline_feedback): + if entry["tool"] in carry: + merged_entries.append(entry) + + # A fresh "general" entry is only trustworthy when the set of tool names + # changed, since a rename, addition or removal is what creates a cross-tool + # inconsistency. A description-only edit that introduces one is a known + # false negative. + # + # When names did change, "general" is on the focus list, so the judge + # omitting it means there is no cross-tool issue, the same reading given to + # any re-judged tool that returns nothing. The dropped finding shows up in + # the provenance block's resolved list rather than vanishing quietly. + source = judged if decision.names_changed else baseline_feedback + merged_entries.extend(e for e in _entries(source) if e["tool"] == GENERAL) + + return { + "findings_by_tool": merged_entries, + "waived": judged.get("waived") or baseline_feedback.get("waived") or [], + "summary": judged.get("summary", "") or baseline_feedback.get( + "summary", "" + ), + } + + +def _entries(feedback: dict) -> list[dict]: + """Return the usable per-tool entries, deep-copied. + + The copy matters: a Baseline loaded from the store is shared by every + caller, and the merged findings are mutated afterwards when ids are minted + into them. Returning the store's own dicts would let one endpoint's merge + write into another's baseline. + + Entries with no findings are dropped, matching the renderer's filter, so the + JSON column cannot carry an entry the HTML never shows. + """ + raw = (feedback or {}).get("findings_by_tool") + if not isinstance(raw, list): + return [] + entries = [] + for entry in raw: + if not isinstance(entry, dict): + continue + tool = str(entry.get("tool", "")).strip() + findings = entry.get("findings") + if not tool or not isinstance(findings, list): + continue + findings = [f for f in findings if isinstance(f, dict)] + if findings: + entries.append({"tool": tool, "findings": copy.deepcopy(findings)}) + return entries + + +def _ordered(entries: list[dict], tool_order: list[str]) -> list[dict]: + """Order entries: general first, then man-page order, then anything else.""" + rank = {name: i for i, name in enumerate(tool_order)} + fallback = len(rank) + + def key(item): + index, entry = item + if entry["tool"] == GENERAL: + return (-1, index) + return (rank.get(entry["tool"], fallback), index) + + return [entry for _, entry in sorted(enumerate(entries), key=key)] + + +def mint_finding_ids(entries: list[dict]) -> None: + """Assign a stable finding_id to every finding, in place. + + (tool, rule_id) is not unique, since the prompt invites the same rule to be + reported several times for one tool, so the optional locator (a parameter + name, description, name) and then the title disambiguate, with an ordinal + suffix as the last resort. + + Ids are minted before the delta view that consumes them ships. Carried + findings keep whatever id they arrived with, so identity is only stable if + ids exist from the first generation that gets carried; retrofitting later + leaves every existing baseline without them. + """ + seen: dict[str, int] = {} + for entry in entries: + tool = entry.get("tool", "") + for finding in entry.get("findings", []): + if not isinstance(finding, dict) or finding.get("finding_id"): + continue + base = _finding_id(tool, finding) + count = seen.get(base, 0) + seen[base] = count + 1 + finding["finding_id"] = base if not count else f"{base}-{count}" + + +def _finding_id(tool: str, finding: dict) -> str: + discriminator = finding.get("locator") or finding.get("title") or "" + raw = "|".join( + [ + str(tool), + str(finding.get("rule_id", "")), + _normalize(str(discriminator)), + ] + ) + return hashlib.sha1(raw.encode("utf-8")).hexdigest()[:12] + + +def _normalize(text: str) -> str: + return re.sub(r"\s+", " ", text).strip().lower() + + +def finding_ids(entries: list[dict]) -> set[str]: + """Return every finding_id present in a list of per-tool entries.""" + return { + finding["finding_id"] + for entry in entries + for finding in entry.get("findings", []) + if isinstance(finding, dict) and finding.get("finding_id") + } + + +def build_provenance( + decision: Decision, merged_entries: list[dict], job_id: str = "" +) -> dict[str, Any]: + """Build the provenance block stored with the feedback and shown in HTML. + + Records where each tool's findings came from, which findings are new versus + resolved, and how old the oldest carried judgement is, so a finding carried + for 200 days without ever being re-derived is visible rather than silently + authoritative. + + job_id is this run's, used as the origin when findings were re-derived. The + result is stored both inside the feedback JSON column and in + mcp_readability_feedback_provenance_json. + """ + baseline = decision.baseline + previous_entries = _entries(baseline.feedback if baseline else {}) + previous_ids = finding_ids(previous_entries) + current_ids = finding_ids(merged_entries) + + # Per-tool markers only mean something when some tools were spared: in a + # full judge every tool was re-judged for a reason the banner already + # states, and labelling them all "schema changed" would be a lie. + tool_provenance = {} + if decision.mode != MODE_FULL_JUDGE: + for name in decision.carried_tools: + tool_provenance[name] = "carried" + for name in decision.rejudged_tools: + tool_provenance[name] = "rejudged" + for name in decision.added_tools: + tool_provenance[name] = "new" + + return { + "component_changes": _component_changes(decision), + "mode": decision.mode, + "change_reason": decision.change_reason, + "baseline_job_id": baseline.job_id if baseline else "", + "baseline_origin_job_id": _origin_job_id(baseline, decision, job_id), + "baseline_timestamp": baseline.check_timestamp if baseline else "", + "rejudged_tools": sorted(decision.rejudged_tools), + "carried_tools": sorted(decision.carried_tools), + "added_tools": sorted(decision.added_tools), + "removed_tools": sorted(decision.removed_tools), + "component_diff": decision.component_diff, + "tool_provenance": tool_provenance, + "new_finding_ids": sorted(current_ids - previous_ids), + "resolved_finding_ids": sorted(previous_ids - current_ids), + "previous_finding_count": _count(previous_entries), + "finding_count": _count(merged_entries), + } + + +def _component_changes(decision: Decision) -> dict[str, dict]: + """Return before/after values for the scalar judge components that changed. + + The diff alone gives "the model changed"; the values give + "gemini-2.5-pro -> gemini-3.1-pro-preview", which is the difference between + a reader trusting the explanation and going looking for it. Non-scalar + components such as the waiver list are reported as changed without their + contents. + """ + baseline = decision.baseline + previous = (baseline.judge_components if baseline else {}) or {} + current = decision.judge_components or {} + changes = {} + for name in decision.component_diff: + before, after = previous.get(name), current.get(name) + if isinstance(before, (str, int, float, type(None))) and isinstance( + after, (str, int, float, type(None)) + ): + changes[name] = {"from": before, "to": after} + else: + changes[name] = {} + return changes + + +def _origin_job_id(baseline, decision: Decision, job_id: str) -> str: + """Return the oldest generation still represented in these findings. + + A fully carried run inherits whatever the baseline recorded, so the chain + points transitively back at the first full judge. A partial run inherits it + too, because some of its findings really are that old, and reporting the + current job would hide the entrenchment this column exists to expose. Only a + run that re-derived everything becomes its own origin. + """ + if baseline is None or not decision.carried_tools: + return job_id + return (baseline.provenance or {}).get("baseline_origin_job_id") or ( + baseline.job_id + ) + + +def _count(entries: list[dict]) -> int: + return sum(len(entry.get("findings", [])) for entry in entries) diff --git a/evalbench/scorers/mcp_fingerprint.py b/evalbench/scorers/mcp_fingerprint.py new file mode 100644 index 00000000..5de0c03d --- /dev/null +++ b/evalbench/scorers/mcp_fingerprint.py @@ -0,0 +1,101 @@ +"""Fingerprints over every input that can change MCP readability feedback. + +The readability judge is a thinking model: even at temperature 0 an unchanged +tool surface can yield different findings from one run to the next, so counts +move with no stated reason. The model cannot be made repeatable, so its inputs +are hashed instead and unchanged ones are not re-judged. + +A tool is hashed by its rendered man-page section rather than its Tool fields, +which makes "the fingerprints match" and "the judge would read identical text" +the same statement. Hashing fields would need a list of which fields matter, +kept in sync with the renderer by hand. Everything else feeding the prompt -- +style guide, prompt text, model, product name, waivers -- is hashed separately +as the judge fingerprint. + +No concrete-scorer imports here, so both the scorers and the orchestrator can +import this without a cycle. +""" + +from collections.abc import Sequence +import hashlib +import json +from typing import Any + +from generators.models.mcp_tool_formatter import format_tool_section + + +def sha256_text(text: str) -> str: + """Return the sha256 hex digest of text, UTF-8 encoded.""" + return hashlib.sha256((text or "").encode("utf-8")).hexdigest() + + +def tool_fingerprints(tools: Sequence[Any]) -> dict[str, str]: + """Map each tool name to the sha256 of its rendered man-page section. + + A later duplicate of a tool name is ignored, matching the first-declaration- + wins merge the tools generator applies across sources. + """ + fingerprints: dict[str, str] = {} + for tool in tools or []: + name = getattr(tool, "name", "") or "" + if not name or name in fingerprints: + continue + fingerprints[name] = sha256_text(format_tool_section(tool)) + return fingerprints + + +def toolset_fingerprint(fingerprints: dict[str, str]) -> str: + """Return a single hash over the whole tool surface. + + Computed over (name, fingerprint) pairs sorted by name, so reordering the + tools does not change it. Order is excluded because the per-tool + fingerprints already carry the guarantee, and making it significant would + force a global re-judge whenever a source is re-declared. + """ + payload = sorted((name, fp) for name, fp in (fingerprints or {}).items()) + return sha256_text(json.dumps(payload, separators=(",", ":"))) + + +def canonical_exceptions(exceptions: list | None) -> list[dict]: + """Reduce waivers to their prompt-visible fields, sorted for stability. + + Only rule_id and reason reach the prompt, and the orchestrator's match order + follows the exceptions file's layout rather than anything semantic, so + reordering the file must not count as a judge change. + """ + canonical = [] + for exc in exceptions or []: + if not isinstance(exc, dict): + continue + canonical.append( + { + "rule_id": str(exc.get("rule_id", "")), + "reason": str(exc.get("reason", "")), + } + ) + return sorted(canonical, key=lambda e: (e["rule_id"], e["reason"])) + + +def judge_fingerprint(components: dict[str, Any]) -> tuple[str, dict[str, Any]]: + """Hash every judge input other than the tools themselves. + + Components are the style guide sha and path, prompt version and sha, model, + scorer name, product name, and canonicalised waivers. They are returned + alongside the hash because a mismatch has to be explained to a human: + diffing them turns an unexplained count change into "the style guide + changed". + """ + canonical = json.dumps(components, sort_keys=True, separators=(",", ":"), + default=str) + return sha256_text(canonical), dict(components) + + +def component_diff(previous: dict | None, current: dict | None) -> list[str]: + """Return the names of the judge components that differ between two runs.""" + previous = previous or {} + current = current or {} + return sorted( + key + for key in set(previous) | set(current) + if previous.get(key) != current.get(key) + ) diff --git a/evalbench/scorers/mcp_readability_scoring.py b/evalbench/scorers/mcp_readability_scoring.py index c1d8f494..0b6d92db 100644 --- a/evalbench/scorers/mcp_readability_scoring.py +++ b/evalbench/scorers/mcp_readability_scoring.py @@ -35,6 +35,11 @@ class EndpointContext: tools: list # list[mcp.types.Tool] man_page: str exceptions: list # applicable waivers for this endpoint + # scorers.mcp_carry_forward.BaselineContext, or None when carry-forward is + # not configured. Typed loosely and defaulted last so existing positional + # construction keeps working, and so this module stays free of scorer + # imports. + baseline: Any = None @dataclass diff --git a/evalbench/scorers/mcp_style_readability.py b/evalbench/scorers/mcp_style_readability.py index b361e93e..4bf5eee3 100644 --- a/evalbench/scorers/mcp_style_readability.py +++ b/evalbench/scorers/mcp_style_readability.py @@ -18,12 +18,19 @@ import re from generators.models import get_generator +from scorers import mcp_carry_forward as cf +from scorers.mcp_fingerprint import ( + canonical_exceptions, + judge_fingerprint, + sha256_text, +) from scorers.mcp_readability_scoring import ( EndpointContext, SEVERITY_BADGES, ScoreContribution, severity_tally, ) +from util.config import load_yaml_config # Output-token ceiling for the JSON-mode judge call. Gemini 3.x is a *thinking* @@ -52,6 +59,8 @@ class TruncatedResponseError(Exception): {{"tool": "", "findings": [ {{"severity": "P0|P1|P2", "rule_id": "", + "locator": "", "title": "", "message": "", "suggestion": ""}} ]}} @@ -143,10 +152,34 @@ class TruncatedResponseError(Exception): ) +# Bumped by hand whenever a prompt edit should invalidate carried findings. The +# sha of PROMPT_TEMPLATE is fingerprinted alongside it as a backstop, so +# forgetting to bump this is safe. The constant exists to let a semantic change +# be declared even when the text is untouched. +PROMPT_VERSION = "1" + + +# Appended only for a partial run. The filter in scorers.mcp_carry_forward is +# what guarantees unchanged tools keep their findings; this clause only stops +# the judge spending output tokens re-describing them. The full man page is +# still supplied above, because the prompt's severity calibration ("don't be +# pedantic when architectural blockers exist") is relative to the whole surface. +_FOCUS_CLAUSE = """ +### SCOPE OF THIS REVIEW +Only these tools have changed since the last review: {focus_tools} +Read the whole man page above for context -- your severity calibration must +account for the entire tool surface -- but emit "findings_by_tool" entries ONLY +for the tools listed on this line. Findings for every other tool are carried +over from the previous review and will be discarded if you repeat them. +""" + + class McpStyleReadabilityScorer: """Scores a tools spec against the MCP style guide using an LLM.""" - # Result-row columns this scorer contributes. + # Result-row columns this scorer contributes. The trailing block records + # what the judge was given and how much of the feedback it actually + # produced, so a count change can be attributed without re-running anything. COLUMNS = [ "mcp_readability_p0_issues", "mcp_readability_p1_issues", @@ -154,6 +187,15 @@ class McpStyleReadabilityScorer: "mcp_readability_score", "mcp_readability_llm_feedback_json", "mcp_readability_llm_feedback_html", + "mcp_readability_judge_fingerprint", + "mcp_readability_judge_components_json", + "mcp_readability_style_guide_sha", + "mcp_readability_style_guide_path", + "mcp_readability_prompt_version", + "mcp_readability_judge_model", + "mcp_readability_feedback_mode", + "mcp_readability_change_reason", + "mcp_readability_feedback_provenance_json", ] def __init__(self, config: dict, global_models): @@ -171,25 +213,79 @@ def __init__(self, config: dict, global_models): "style_guide is required for the mcp_style_readability scorer" ) self.style_guide = _read_text(style_guide_path) + self.style_guide_path = style_guide_path + # Only the hash is persisted, never the guide text: detection is a hash + # comparison, and the text would be duplicated into every endpoint row + # on every run. The path travels with it because the + # style_guide.*.local.md overrides mean two runs can legitimately use + # different guides, which a hash change alone cannot tell apart from an + # edit to the same guide. + self.style_guide_sha = sha256_text(self.style_guide) self.max_output_tokens = int( config.get("max_output_tokens", _MAX_OUTPUT_TOKENS) ) self.model = get_generator(global_models, self.model_config) + # Read the model name from the config rather than the generator object: + # GeminiGenerator exposes vertex_model but ClaudeGenerator exposes + # model_id, and reading the config is uniform. It also keeps the + # google3-vs-OSS model split from letting one environment reuse the + # other's baselines. + self.judge_model = _judge_model_name(self.model_config) def run(self, context: EndpointContext) -> ScoreContribution: - """Evaluate one endpoint: judge the man page, pass iff no P0 findings.""" - feedback = self.evaluate( - tools_markup=context.man_page, - style_guide=self.style_guide, - product_name=context.product_name, - exceptions=context.exceptions, + """Evaluate one endpoint: judge the man page, pass iff no P0 findings. + + Re-judges only what changed. When the tool surface and every judge input + are unchanged since the baseline, the previous findings are returned + verbatim and the model is never called; see scorers.mcp_carry_forward + for the invariant this upholds. + """ + baseline_context = getattr(context, "baseline", None) + components = self._judge_components(context) + fingerprint, components = judge_fingerprint(components) + decision = cf.decide_with_components( + baseline_context, fingerprint, components + ) + tool_order = list( + (baseline_context.tool_fingerprints if baseline_context else {}) + or _tool_names(context.tools) ) - p0 = int(feedback.get("p0_issues", 0)) + + judged = None + if decision.needs_model_call: + judged = self.evaluate( + tools_markup=context.man_page, + style_guide=self.style_guide, + product_name=context.product_name, + exceptions=context.exceptions, + focus_tools=( + decision.tools_to_report + if decision.mode == cf.MODE_PARTIAL + else None + ), + ) + + feedback = self._merged_feedback( + decision, + judged, + tool_order, + job_id=baseline_context.job_id if baseline_context else "", + ) + logging.info( + "mcp_readability: %s judged %s (%s): %d findings, %d model call(s)", + context.product_name, + decision.mode, + decision.change_reason, + feedback["p0_issues"] + feedback["p1_issues"] + feedback["p2_issues"], + 1 if decision.needs_model_call else 0, + ) + + p0 = feedback["p0_issues"] return ScoreContribution( row_fields={ "mcp_readability_p0_issues": p0, - "mcp_readability_p1_issues": int(feedback.get("p1_issues", 0)), - "mcp_readability_p2_issues": int(feedback.get("p2_issues", 0)), + "mcp_readability_p1_issues": feedback["p1_issues"], + "mcp_readability_p2_issues": feedback["p2_issues"], "mcp_readability_score": int(feedback.get("readability_score", 0)), # Both feedback columns omit the readability score on purpose; # only the numeric metric column above carries it. @@ -199,20 +295,86 @@ def run(self, context: EndpointContext) -> ScoreContribution: "mcp_readability_llm_feedback_html": self.to_html( feedback, context.product_name ), + "mcp_readability_judge_fingerprint": fingerprint, + "mcp_readability_judge_components_json": json.dumps( + components, sort_keys=True + ), + "mcp_readability_style_guide_sha": self.style_guide_sha, + "mcp_readability_style_guide_path": self.style_guide_path, + "mcp_readability_prompt_version": PROMPT_VERSION, + "mcp_readability_judge_model": self.judge_model, + "mcp_readability_feedback_mode": decision.mode, + "mcp_readability_change_reason": decision.change_reason, + "mcp_readability_feedback_provenance_json": json.dumps( + feedback.get("provenance") or {}, sort_keys=True + ), }, score=100 if p0 == 0 else 0, logs=( f"p0_issues={p0}, " - f"readability_score={feedback.get('readability_score', 0)}" + f"readability_score={feedback.get('readability_score', 0)}, " + f"mode={decision.mode}, reason={decision.change_reason}" ), ) + def _judge_components(self, context: EndpointContext) -> dict: + """Every judge input other than the tools themselves. + + Kept as a dict rather than a bare hash so a mismatch can name the + component that changed, which is what turns an unexplained count swing + into "the style guide changed". + """ + return { + "scorer_name": self.name, + "prompt_version": PROMPT_VERSION, + "prompt_sha": sha256_text(PROMPT_TEMPLATE), + "style_guide_sha": self.style_guide_sha, + "style_guide_path": self.style_guide_path, + "judge_model": self.judge_model, + # Interpolated into the prompt, so it is a judge input. + "product_name": context.product_name or "", + "exceptions": canonical_exceptions(context.exceptions), + } + + def _merged_feedback( + self, decision, judged, tool_order: list[str], job_id: str = "" + ) -> dict: + """Merge carried and fresh findings, then recount from the result. + + Counts are always recomputed over the merged findings, so the P0/P1/P2 + arithmetic stays consistent with what is rendered no matter how the + findings were sourced. + """ + merged = cf.merge_feedback(decision, judged, tool_order) + entries = merged["findings_by_tool"] + # Carried findings already have ids and keep them; only fresh ones are + # minted, which is what makes finding identity stable across runs. + cf.mint_finding_ids(entries) + counts = _severity_counts([f for e in entries for f in e["findings"]]) + if decision.mode == cf.MODE_CARRIED and decision.baseline is not None: + # _public_feedback strips the score from the JSON column, so it is + # restored from the numeric metric column instead. + score = decision.baseline.readability_score + else: + score = _safe_int((judged or {}).get("readability_score")) + return { + "readability_score": score, + "p0_issues": counts["P0"], + "p1_issues": counts["P1"], + "p2_issues": counts["P2"], + "findings_by_tool": entries, + "waived": merged["waived"], + "summary": merged["summary"], + "provenance": cf.build_provenance(decision, entries, job_id), + } + def evaluate( self, tools_markup: str, style_guide: str, product_name: str, exceptions: list[dict] | None = None, + focus_tools: list[str] | None = None, ) -> dict: """Run the LLM readability check and return a normalized feedback dict.""" prompt = PROMPT_TEMPLATE.format( @@ -221,6 +383,8 @@ def evaluate( tools_markup=tools_markup or "(no tools)", exceptions=json.dumps(exceptions or [], indent=2), ) + if focus_tools: + prompt += _FOCUS_CLAUSE.format(focus_tools=", ".join(focus_tools)) raw = self._generate(prompt) return self._parse(raw) @@ -299,6 +463,10 @@ def _parse(self, raw: str) -> dict: """Normalize the model output into a stable feedback dict.""" data = self._extract_json(raw) by_tool = _clean_findings_by_tool(data.get("findings_by_tool")) + # Mint ids here so a finding is identifiable the moment it exists: a + # carried finding keeps the id it was born with, which is only stable if + # every generation that can be carried has one. + cf.mint_finding_ids(by_tool) counts = _severity_counts( [f for entry in by_tool for f in entry["findings"]] ) @@ -316,11 +484,13 @@ def _parse(self, raw: str) -> dict: def to_html(feedback: dict, product_name: str = "") -> str: """Render feedback as a human-readable HTML fragment. - Leads with the overall summary, renders the judge's per-tool findings - lists in the order it returned them, and ends with the allowed exceptions - (waived rules) and their reasons. It deliberately omits any numeric - readability score -- the intent is review notes an engineer can act on, - not a grade. + Leads with a provenance banner and the overall summary, renders the + per-tool findings in man-page order with the cross-tool general entry + first, and ends with the allowed exceptions (waived rules) and their + reasons. That order is the one the judge is asked for, now enforced + rather than assumed, since a partial run assembles entries from two + sources. No numeric readability score is shown: the intent is review + notes an engineer can act on, not a grade. HTML (rather than Markdown) because this column is surfaced in a dashboard that renders it as HTML. All model-supplied text is escaped. @@ -335,17 +505,32 @@ def to_html(feedback: dict, product_name: str = "") -> str: f"

MCP Tool Readability Review — {title}

", ] + # Immediately after the title, before the summary: the reader's first + # question about a changed number is "why", and an explicit "identical + # to the previous run" builds as much trust as an explanation does. + provenance = feedback.get("provenance") or {} + banner = _provenance_banner(feedback) + if banner: + parts.append( + "

Change tracking: " + f"{esc(banner)}

" + ) + summary = str(feedback.get("summary", "")).strip() if summary: parts.append(f"

Summary: {esc(summary)}

") + tool_provenance = provenance.get("tool_provenance") or {} by_tool = _clean_findings_by_tool(feedback.get("findings_by_tool")) if not by_tool: parts.append("

No findings

") for entry in by_tool: items = entry["findings"] + marker = _TOOL_MARKERS.get(tool_provenance.get(entry["tool"]), "") + suffix = f" · {esc(marker)}" if marker else "" parts.append( - f"

{esc(entry['tool'])} — {severity_tally(items)}

" + f"

{esc(entry['tool'])} — {severity_tally(items)}" + f"{suffix}

" ) parts.append("
    ") for f in items: @@ -388,6 +573,151 @@ def to_html(feedback: dict, product_name: str = "") -> str: return "".join(parts) +# Per-tool heading suffix. An untouched tool says so explicitly, which is what +# lets a reader tell "not re-judged" apart from "re-judged and happened to +# score the same". +_TOOL_MARKERS = { + "carried": "unchanged since the previous review", + "rejudged": "re-judged (schema changed)", + "new": "new tool", +} + + +def _provenance_banner(feedback: dict) -> str: + """One sentence explaining why this report differs from the previous one.""" + provenance = feedback.get("provenance") or {} + mode = provenance.get("mode") + if not mode: + return "" + + reason = provenance.get("change_reason", "") + since = _date_of(provenance.get("baseline_timestamp", "")) + since_clause = f" since {since}" if since else "" + previous = provenance.get("previous_finding_count", 0) + current = provenance.get("finding_count", 0) + new = len(provenance.get("new_finding_ids") or []) + resolved = len(provenance.get("resolved_finding_ids") or []) + net = ( + f" Net {previous} → {current} ({new} new, {resolved} resolved)." + if previous or current + else "" + ) + + if mode == cf.MODE_CARRIED: + tally = severity_tally( + [ + f + for entry in _clean_findings_by_tool( + feedback.get("findings_by_tool") + ) + for f in entry["findings"] + ] + ) + return ( + f"Unchanged{since_clause}. No tool schema changes; findings carried " + f"forward verbatim. {tally} — identical to the previous run." + ) + + if mode == cf.MODE_PARTIAL: + rejudged = provenance.get("rejudged_tools") or [] + added = provenance.get("added_tools") or [] + removed = provenance.get("removed_tools") or [] + carried = provenance.get("carried_tools") or [] + changed = rejudged + added + total = len(changed) + len(carried) + detail = [] + if rejudged: + detail.append(f"Re-judged: {', '.join(rejudged)}.") + if added: + detail.append(f"Added: {', '.join(added)}.") + if removed: + detail.append(f"Removed: {', '.join(removed)}.") + return ( + f"{len(changed)} of {total} tools changed{since_clause}. " + + " ".join(detail) + + f" The other {len(carried)} tools are carried forward unchanged." + + net + ) + + if reason == cf.NO_BASELINE: + return ( + "No previous review to compare against; this is the first recorded " + "run for this endpoint." + ) + + caveat = ( + " All tools were re-judged, so count changes below may reflect the " + "judge rather than your tools." + ) + changes = provenance.get("component_changes") or {} + if reason == cf.MODEL_CHANGED: + return ( + f"Judge model changed{since_clause}" + f"{_transition(changes.get('judge_model'))}.{caveat}{net}" + ) + if reason == cf.STYLE_GUIDE_CHANGED: + return ( + f"Style guide changed{since_clause}" + f"{_transition(changes.get('style_guide_path'))}.{caveat}{net}" + ) + if reason == cf.WAIVERS_CHANGED: + return ( + f"Waived rules changed{since_clause}.{caveat}{net}" + ) + if reason == cf.PROMPT_CHANGED: + return f"The review prompt changed{since_clause}.{caveat}{net}" + if reason == cf.BASELINE_EXPIRED: + return ( + f"The previous review{since_clause} has aged out and was " + f"re-derived from scratch.{caveat}{net}" + ) + if reason == cf.FORCED_REFRESH: + return f"A full re-review was requested for this run.{caveat}{net}" + if reason == cf.ENDPOINT_IDENTITY_CHANGED: + return ( + "This endpoint's identity changed, so its previous review could " + "not be matched to it." + ) + if reason == cf.BASELINE_UNAVAILABLE: + return ( + f"The previous review{since_clause} could not be read or compared, " + f"so every tool was re-judged.{caveat}{net}" + ) + return f"Every tool was re-judged ({reason})." + + +def _transition(change: dict | None) -> str: + """Render a recorded component change as " (a → b)", else an empty string.""" + if not change or change.get("from") in (None, "") or change.get("to") in ( + None, + "", + ): + return "" + return f" ({change['from']} → {change['to']})" + + +def _date_of(timestamp: str) -> str: + return str(timestamp or "").split("T")[0] + + +def _tool_names(tools) -> list[str]: + return [name for name in (getattr(t, "name", "") for t in tools or []) if name] + + +def _judge_model_name(model_config_path: str) -> str: + """The judge's model id, read from its model config. + + Falls back to the config path when the file cannot be read: a stable string + is all the fingerprint needs, and an unreadable model config would already + have failed at generator construction. + """ + try: + config = load_yaml_config(model_config_path) or {} + except Exception: + return str(model_config_path) + return str(config.get("vertex_model") or model_config_path) + + def _clean_findings_by_tool(by_tool) -> list[dict]: """The judge's per-tool findings, with unusable entries dropped. diff --git a/evalbench/test/mcp_baseline_test.py b/evalbench/test/mcp_baseline_test.py new file mode 100644 index 00000000..a85dca28 --- /dev/null +++ b/evalbench/test/mcp_baseline_test.py @@ -0,0 +1,433 @@ +"""Unit tests for the readability baseline stores. + +Two behaviours matter most: picking the *newest* previous run for an endpoint +(picking an older one would resurrect stale findings), and degrading to "no +baseline" on any read failure rather than aborting the job, since a baseline is +an optimisation and not a measurement. +""" + +import csv +import datetime +import os +import tempfile +import unittest + +from evaluator.mcp_readability import baseline as baseline_mod +from evaluator.mcp_readability import orchestrator as orchestrator_mod +from evaluator.mcp_readability.orchestrator import McpReadabilityOrchestrator +from evaluator.mcp_readability.baseline import ( + BigQueryBaselineStore, + LocalResultsBaselineStore, + NullBaselineStore, + build_store, + is_expired, +) +from scorers import mcp_carry_forward as cf +from scorers.mcp_carry_forward import Baseline + + +_COLUMNS = [ + "mcp_readability_endpoint_key", + "mcp_readability_check_timestamp_utc", + "mcp_readability_check_timestamp", + "mcp_readability_judge_fingerprint", + "mcp_readability_judge_components_json", + "mcp_readability_tool_fingerprints_json", + "mcp_readability_llm_feedback_json", + "mcp_readability_feedback_provenance_json", + "mcp_readability_score", + "job_id", +] + + +def _write_run(root, job_id, rows): + directory = os.path.join(root, job_id) + os.makedirs(directory, exist_ok=True) + path = os.path.join(directory, "evals.csv") + with open(path, "w", encoding="utf-8", newline="") as f: + writer = csv.DictWriter(f, fieldnames=_COLUMNS) + writer.writeheader() + for row in rows: + writer.writerow({column: row.get(column, "") for column in _COLUMNS}) + return path + + +def _row(key, timestamp, job_id, score="70", feedback='{"summary": "s"}'): + return { + "mcp_readability_endpoint_key": key, + "mcp_readability_check_timestamp_utc": timestamp, + "mcp_readability_judge_fingerprint": "jf", + "mcp_readability_judge_components_json": '{"judge_model": "m"}', + "mcp_readability_tool_fingerprints_json": '{"a": "fp"}', + "mcp_readability_llm_feedback_json": feedback, + "mcp_readability_feedback_provenance_json": '{"baseline_origin_job_id": "j0"}', + "mcp_readability_score": score, + "job_id": job_id, + } + + +class NullStoreTest(unittest.TestCase): + + def test_never_finds_a_baseline(self): + store = NullBaselineStore() + store.prime(["key"]) + self.assertIsNone(store.load("key")) + + +class LocalResultsStoreTest(unittest.TestCase): + + def setUp(self): + self._tmp = tempfile.TemporaryDirectory() + self.root = self._tmp.name + + def tearDown(self): + self._tmp.cleanup() + + def test_picks_the_newest_run(self): + _write_run( + self.root, "job-old", + [_row("key", "2026-09-01T00:00:00Z", "job-old", score="10")], + ) + _write_run( + self.root, "job-new", + [_row("key", "2026-09-09T00:00:00Z", "job-new", score="90")], + ) + store = LocalResultsBaselineStore(results_dir=self.root) + store.prime(["key"]) + found = store.load("key") + self.assertEqual(found.job_id, "job-new") + self.assertEqual(found.readability_score, 90) + + def test_parses_the_json_columns(self): + _write_run(self.root, "job", [_row("key", "2026-09-09T00:00:00Z", "job")]) + store = LocalResultsBaselineStore(results_dir=self.root) + store.prime(["key"]) + found = store.load("key") + self.assertEqual(found.tool_fingerprints, {"a": "fp"}) + self.assertEqual(found.judge_components, {"judge_model": "m"}) + self.assertEqual(found.feedback, {"summary": "s"}) + self.assertEqual(found.provenance["baseline_origin_job_id"], "j0") + + def test_only_requested_endpoints_are_kept(self): + _write_run( + self.root, "job", + [ + _row("wanted", "2026-09-09T00:00:00Z", "job"), + _row("other", "2026-09-09T00:00:00Z", "job"), + ], + ) + store = LocalResultsBaselineStore(results_dir=self.root) + store.prime(["wanted"]) + self.assertIsNotNone(store.load("wanted")) + self.assertIsNone(store.load("other")) + + def test_falls_back_to_the_naive_timestamp_column(self): + row = _row("key", "", "job") + row["mcp_readability_check_timestamp"] = "2026-09-09T12:00:00" + _write_run(self.root, "job", [row]) + store = LocalResultsBaselineStore(results_dir=self.root) + store.prime(["key"]) + self.assertEqual(store.load("key").check_timestamp, "2026-09-09T12:00:00") + + def test_rows_without_an_endpoint_key_are_ignored(self): + _write_run(self.root, "job", [_row("", "2026-09-09T00:00:00Z", "job")]) + store = LocalResultsBaselineStore(results_dir=self.root) + store.prime(["key"]) + self.assertIsNone(store.load("key")) + + def test_missing_results_directory_is_not_an_error(self): + store = LocalResultsBaselineStore(results_dir=os.path.join(self.root, "x")) + store.prime(["key"]) + self.assertIsNone(store.load("key")) + + def test_unreadable_run_is_skipped_not_raised(self): + # An evals.csv that cannot be opened (here: it is a directory) must cost + # a re-judge, not abort the job. + os.makedirs(os.path.join(self.root, "bad", "evals.csv")) + _write_run( + self.root, "good", [_row("key", "2026-09-09T00:00:00Z", "good")] + ) + store = LocalResultsBaselineStore(results_dir=self.root) + store.prime(["key"]) # must not raise + self.assertEqual(store.load("key").job_id, "good") + + def test_scan_is_capped(self): + for i in range(5): + _write_run( + self.root, f"job-{i}", + [_row(f"key-{i}", "2026-09-09T00:00:00Z", f"job-{i}")], + ) + store = LocalResultsBaselineStore(results_dir=self.root, max_runs=1) + store.prime([f"key-{i}" for i in range(5)]) + found = [i for i in range(5) if store.load(f"key-{i}") is not None] + self.assertEqual(len(found), 1) + + +class BuildStoreTest(unittest.TestCase): + + def test_absent_block_is_the_null_store(self): + self.assertIsInstance(build_store({}), NullBaselineStore) + self.assertIsInstance(build_store(None), NullBaselineStore) + + def test_explicit_none_is_the_null_store(self): + self.assertIsInstance(build_store({"store": "none"}), NullBaselineStore) + + def test_local_and_bigquery(self): + self.assertIsInstance( + build_store({"store": "local"}), LocalResultsBaselineStore + ) + self.assertIsInstance( + build_store({"store": "bigquery"}), BigQueryBaselineStore + ) + + def test_unknown_store_fails_fast(self): + with self.assertRaises(ValueError): + build_store({"store": "redis"}) + + +class ExpiryTest(unittest.TestCase): + + def _baseline(self, days_ago, key="00000000"): + stamp = datetime.datetime( + 2026, 9, 14, tzinfo=datetime.timezone.utc + ) - datetime.timedelta(days=days_ago) + return Baseline(endpoint_key=key, check_timestamp=stamp.isoformat()) + + def _now(self): + return datetime.datetime(2026, 9, 14, tzinfo=datetime.timezone.utc) + + def test_fresh_baseline_is_not_expired(self): + self.assertFalse(is_expired(self._baseline(1), 90, now=self._now())) + + def test_old_baseline_is_expired(self): + self.assertTrue(is_expired(self._baseline(200), 90, now=self._now())) + + def test_zero_max_age_disables_expiry(self): + self.assertFalse(is_expired(self._baseline(9999), 0, now=self._now())) + + def test_unparseable_timestamp_expires(self): + stale = Baseline(endpoint_key="k", check_timestamp="not a date") + self.assertTrue(is_expired(stale, 90, now=self._now())) + + def test_expiry_is_staggered_across_endpoints(self): + # Same age, different endpoints: they must not all flip on one day, or + # a whole fleet re-judges at once and every number moves together. + verdicts = { + is_expired(self._baseline(95, key=f"endpoint-{i}"), 90, + now=self._now()) + for i in range(200) + } + self.assertEqual(verdicts, {True, False}) + + def test_stagger_stays_inside_the_window(self): + for key in ("", "endpoint-1", "a" * 100, "alloydb-prod"): + with self.subTest(key=key): + self.assertLess(baseline_mod._stagger(key, 90), 90) + + def test_a_very_old_baseline_expires_whatever_the_stagger(self): + for i in range(20): + with self.subTest(i=i): + self.assertTrue( + is_expired( + self._baseline(1000, key=f"endpoint-{i}"), 90, + now=self._now(), + ) + ) + + def test_naive_timestamps_are_treated_as_utc(self): + stale = Baseline(endpoint_key="00000000", + check_timestamp="2020-01-01T00:00:00") + self.assertTrue(is_expired(stale, 90, now=self._now())) + + +class BigQueryStoreTest(unittest.TestCase): + + def test_prime_with_no_keys_does_not_touch_bigquery(self): + # No import of google.cloud.bigquery, no client, no query. + store = BigQueryBaselineStore() + store.prime([]) + self.assertIsNone(store.load("key")) + + def test_rows_are_absorbed_like_csv_rows(self): + store = BigQueryBaselineStore() + store._absorb(_row("key", "2026-09-09T00:00:00Z", "job"), {"key"}) + self.assertEqual(store.load("key").job_id, "job") + + +class _ExplodingStore: + def prime(self, endpoint_keys): + raise RuntimeError("no read permission on the dataset") + + def load(self, endpoint_key): + raise RuntimeError("no read permission on the dataset") + + +def _orchestrator(store, **attrs): + """A bare orchestrator with only the carry-forward state populated.""" + orchestrator = McpReadabilityOrchestrator.__new__(McpReadabilityOrchestrator) + orchestrator.job_id = "job-under-test" + orchestrator.baseline_store = store + orchestrator.baseline_max_age_days = 90 + orchestrator.force_refresh = False + orchestrator.force_refresh_products = set() + orchestrator.baseline_unavailable = False + for key, value in attrs.items(): + setattr(orchestrator, key, value) + return orchestrator + + +class StoreFailureDegradesTest(unittest.TestCase): + """A baseline is an optimisation, so a read failure must never abort a run.""" + + def test_prime_failure_is_swallowed_and_recorded(self): + orchestrator = _orchestrator(_ExplodingStore()) + endpoint = { + "product_name": "AlloyDB", + "endpoint_type": "PROD", + "tools_source": {"url": "http://x"}, + } + orchestrator._prime_baselines([endpoint]) # must not raise + self.assertTrue(orchestrator.baseline_unavailable) + context = orchestrator._baseline_context(endpoint, "key", {"a": "fp"}) + self.assertEqual(context.override_reason, cf.BASELINE_UNAVAILABLE) + self.assertIsNone(context.baseline) + + def test_load_failure_is_swallowed(self): + orchestrator = _orchestrator(_ExplodingStore()) + context = orchestrator._baseline_context({}, "key", {"a": "fp"}) + self.assertEqual(context.override_reason, cf.BASELINE_UNAVAILABLE) + + def test_expired_baseline_is_an_announced_rejudge(self): + class _Store: + def prime(self, keys): + pass + + def load(self, key): + return Baseline( + endpoint_key=key, check_timestamp="2020-01-01T00:00:00Z" + ) + + context = _orchestrator(_Store())._baseline_context({}, "k", {}) + self.assertEqual(context.override_reason, cf.BASELINE_EXPIRED) + # Still attached: the override stops its findings being carried, but it + # remains the reference for "what changed since the previous review". + self.assertIsNotNone(context.baseline) + self.assertEqual( + cf.decide(context, "any-fingerprint").mode, cf.MODE_FULL_JUDGE + ) + + def test_force_refresh_still_keeps_the_baseline_for_comparison(self): + class _Store: + def prime(self, keys): + pass + + def load(self, key): + return Baseline( + endpoint_key=key, check_timestamp="2026-09-09T00:00:00Z" + ) + + orchestrator = _orchestrator(_Store(), force_refresh=True) + context = orchestrator._baseline_context({}, "k", {}) + self.assertEqual(context.override_reason, cf.FORCED_REFRESH) + self.assertIsNotNone(context.baseline) + self.assertEqual( + cf.decide(context, "any-fingerprint").mode, cf.MODE_FULL_JUDGE + ) + + def test_force_refresh_skips_the_baseline(self): + orchestrator = _orchestrator(NullBaselineStore(), force_refresh=True) + context = orchestrator._baseline_context({}, "k", {}) + self.assertEqual(context.override_reason, cf.FORCED_REFRESH) + + def test_force_refresh_products_is_per_product(self): + orchestrator = _orchestrator( + NullBaselineStore(), force_refresh_products={"alloydb"} + ) + forced = orchestrator._baseline_context( + {"product_name": "AlloyDB"}, "k", {} + ) + other = orchestrator._baseline_context( + {"product_name": "Cloud SQL"}, "k", {} + ) + self.assertEqual(forced.override_reason, cf.FORCED_REFRESH) + self.assertEqual(other.override_reason, "") + + def test_null_store_yields_no_override_and_no_baseline(self): + context = _orchestrator(NullBaselineStore())._baseline_context( + {}, "k", {"a": "fp"} + ) + self.assertEqual(context.override_reason, "") + self.assertIsNone(context.baseline) + + +class EndpointKeyTest(unittest.TestCase): + + def test_explicit_id_wins(self): + self.assertEqual( + orchestrator_mod._endpoint_key( + {"id": "alloydb-prod"}, "AlloyDB", "http://x", "PROD" + ), + "alloydb-prod", + ) + + def test_derived_key_is_stable(self): + args = ({}, "AlloyDB", "http://x", "PROD") + self.assertEqual( + orchestrator_mod._endpoint_key(*args), + orchestrator_mod._endpoint_key(*args), + ) + + def test_each_identity_field_changes_the_key(self): + base = orchestrator_mod._endpoint_key( + {}, "AlloyDB", "http://x", "PROD" + ) + variants = [ + ({}, "Cloud SQL", "http://x", "PROD"), + ({}, "AlloyDB", "http://y", "PROD"), + ({}, "AlloyDB", "http://x", "STAGING"), + ] + for variant in variants: + with self.subTest(variant=variant): + self.assertNotEqual( + base, orchestrator_mod._endpoint_key(*variant) + ) + + def test_explicit_id_survives_a_product_rename(self): + self.assertEqual( + orchestrator_mod._endpoint_key( + {"id": "pinned"}, "AlloyDB", "http://x", "PROD" + ), + orchestrator_mod._endpoint_key( + {"id": "pinned"}, "AlloyDB Omni", "http://x", "PROD" + ), + ) + + +class ForceRefreshEnvTest(unittest.TestCase): + + def tearDown(self): + os.environ.pop("EVALBENCH_MCP_FORCE_REFRESH", None) + + def test_env_var_forces_a_refresh(self): + os.environ["EVALBENCH_MCP_FORCE_REFRESH"] = "1" + self.assertTrue(orchestrator_mod._force_refresh({})) + + def test_config_flag_forces_a_refresh(self): + self.assertTrue(orchestrator_mod._force_refresh({"force_refresh": True})) + + def test_default_is_no_refresh(self): + self.assertFalse(orchestrator_mod._force_refresh({})) + + +class TimestampParsingTest(unittest.TestCase): + + def test_trailing_z_is_accepted(self): + parsed = baseline_mod._parse_timestamp("2026-09-09T00:00:00Z") + self.assertEqual(parsed.tzinfo, datetime.timezone.utc) + + def test_empty_is_none(self): + self.assertIsNone(baseline_mod._parse_timestamp("")) + + +if __name__ == "__main__": + unittest.main() diff --git a/evalbench/test/mcp_carry_forward_test.py b/evalbench/test/mcp_carry_forward_test.py new file mode 100644 index 00000000..e5032575 --- /dev/null +++ b/evalbench/test/mcp_carry_forward_test.py @@ -0,0 +1,827 @@ +"""Unit tests for readability carry-forward. + +The load-bearing cases are the invariant ones: an unchanged endpoint must make +zero model calls and emit byte-identical feedback, and a partial run must +discard the judge's findings for tools it was not asked about even when the +model volunteers them. Everything else here guards the explanation -- the +change reason a human reads to understand why a number moved. +""" + +import json +import unittest + +from mcp import types as mcp_types + +from scorers import mcp_carry_forward as cf +from scorers.mcp_fingerprint import tool_fingerprints +from scorers.mcp_readability_scoring import EndpointContext +from scorers.mcp_style_readability import McpStyleReadabilityScorer + + +def _tool(name, description="Does a thing."): + return mcp_types.Tool( + name=name, description=description, inputSchema={"type": "object"} + ) + + +def _entry(tool, *findings): + return {"tool": tool, "findings": list(findings)} + + +def _finding(severity="P1", rule_id="Tool Names", title="", locator=""): + finding = {"severity": severity, "rule_id": rule_id} + if title: + finding["title"] = title + if locator: + finding["locator"] = locator + return finding + + +class _RecordingModel: + """A judge that counts calls and returns a canned response.""" + + def __init__(self, payload): + self.payload = payload + self.calls = 0 + self.prompts = [] + + def generate(self, prompt): + self.calls += 1 + self.prompts.append(prompt) + return json.dumps(self.payload) + + +def _focus_lines(model): + """The prompt's scope line, naming the tools the judge must report on.""" + return [ + line + for line in model.prompts[0].splitlines() + if "have changed" in line + ] + + +def _scorer(model): + scorer = McpStyleReadabilityScorer.__new__(McpStyleReadabilityScorer) + scorer.name = "mcp_style_readability" + scorer.style_guide = "guide" + scorer.style_guide_sha = "guide-sha" + scorer.style_guide_path = "guide.md" + scorer.judge_model = "fake-model" + scorer.model = model + return scorer + + +def _run(scorer, tools, baseline=None, override_reason="", exceptions=None): + fingerprints = tool_fingerprints(tools) + context = EndpointContext( + product_name="AlloyDB", + endpoint={}, + tools=tools, + man_page="man page", + exceptions=exceptions or [], + baseline=cf.BaselineContext( + endpoint_key="key", + tool_fingerprints=fingerprints, + baseline=baseline, + override_reason=override_reason, + ), + ) + return scorer.run(context) + + +def _baseline_from(contribution, tools, **overrides): + """The Baseline a next run would load from this run's result row.""" + fields = contribution.row_fields + baseline = cf.Baseline( + endpoint_key="key", + job_id="job-1", + check_timestamp="2026-09-09T00:00:00Z", + judge_fingerprint=fields["mcp_readability_judge_fingerprint"], + judge_components=json.loads( + fields["mcp_readability_judge_components_json"] + ), + tool_fingerprints=tool_fingerprints(tools), + feedback=json.loads(fields["mcp_readability_llm_feedback_json"]), + readability_score=fields["mcp_readability_score"], + provenance=json.loads( + fields["mcp_readability_feedback_provenance_json"] + ), + ) + for key, value in overrides.items(): + setattr(baseline, key, value) + return baseline + + +class UnchangedEndpointTest(unittest.TestCase): + """The invariant: same inputs => same bytes, no model call.""" + + def setUp(self): + self.tools = [_tool("list_instances"), _tool("create_instance")] + self.payload = { + "readability_score": 72, + "findings_by_tool": [ + _entry("create_instance", _finding("P0", "Avoid complex params")), + _entry("list_instances", _finding("P2", "Concise Descriptions")), + ], + "summary": "Mostly fine.", + } + + def test_no_model_call_and_identical_feedback(self): + first_model = _RecordingModel(self.payload) + first = _run(_scorer(first_model), self.tools) + self.assertEqual(first_model.calls, 1) + + second_model = _RecordingModel(self.payload) + second = _run( + _scorer(second_model), + self.tools, + baseline=_baseline_from(first, self.tools), + ) + + self.assertEqual(second_model.calls, 0) + self.assertEqual( + second.row_fields["mcp_readability_feedback_mode"], cf.MODE_CARRIED + ) + self.assertEqual( + second.row_fields["mcp_readability_change_reason"], cf.UNCHANGED + ) + # Byte-identical findings, counts and score. + self.assertEqual( + _findings_json(second), _findings_json(first) + ) + for column in ( + "mcp_readability_p0_issues", + "mcp_readability_p1_issues", + "mcp_readability_p2_issues", + "mcp_readability_score", + ): + self.assertEqual( + second.row_fields[column], first.row_fields[column], column + ) + + def test_reordering_tools_still_carries(self): + first = _run(_scorer(_RecordingModel(self.payload)), self.tools) + model = _RecordingModel(self.payload) + reordered = list(reversed(self.tools)) + second = _run( + _scorer(model), reordered, baseline=_baseline_from(first, self.tools) + ) + self.assertEqual(model.calls, 0) + self.assertEqual( + second.row_fields["mcp_readability_change_reason"], cf.UNCHANGED + ) + + def test_banner_states_the_report_is_identical(self): + first = _run(_scorer(_RecordingModel(self.payload)), self.tools) + second = _run( + _scorer(_RecordingModel(self.payload)), + self.tools, + baseline=_baseline_from(first, self.tools), + ) + html = second.row_fields["mcp_readability_llm_feedback_html"] + self.assertIn("Unchanged since 2026-09-09", html) + self.assertIn("identical to the previous run", html) + self.assertIn("unchanged since the previous review", html) + + +class OriginJobIdTest(unittest.TestCase): + """The origin must expose findings that have never been re-derived. + + Without this, a hallucinated P0 carried for months looks as authoritative + as one the judge produced today. + """ + + def setUp(self): + self.tools = [_tool("list_instances")] + self.payload = { + "readability_score": 70, + "findings_by_tool": [_entry("list_instances", _finding("P1"))], + "summary": "s", + } + + def _run(self, job_id, baseline=None, tools=None): + fingerprints = tool_fingerprints(tools or self.tools) + context = EndpointContext( + product_name="AlloyDB", + endpoint={}, + tools=tools or self.tools, + man_page="man page", + exceptions=[], + baseline=cf.BaselineContext( + endpoint_key="key", + job_id=job_id, + tool_fingerprints=fingerprints, + baseline=baseline, + ), + ) + return _scorer(_RecordingModel(self.payload)).run(context) + + def test_first_run_is_its_own_origin(self): + first = self._run("job-1") + self.assertEqual(_provenance(first)["baseline_origin_job_id"], "job-1") + + def test_origin_survives_a_chain_of_carried_runs(self): + first = self._run("job-1") + baseline = _baseline_from(first, self.tools) + baseline.job_id = "job-1" + second = self._run("job-2", baseline=baseline) + self.assertEqual(_provenance(second)["baseline_origin_job_id"], "job-1") + + third_baseline = _baseline_from(second, self.tools) + third_baseline.job_id = "job-2" + third = self._run("job-3", baseline=third_baseline) + # Still job-1: nothing has been re-derived since. + self.assertEqual(_provenance(third)["baseline_origin_job_id"], "job-1") + + def test_a_full_rejudge_resets_the_origin(self): + first = self._run("job-1") + baseline = _baseline_from(first, self.tools) + baseline.job_id = "job-1" + baseline.judge_fingerprint = "stale" # forces a full re-judge + second = self._run("job-2", baseline=baseline) + self.assertEqual(_provenance(second)["baseline_origin_job_id"], "job-2") + + def test_a_partial_rejudge_keeps_the_older_origin(self): + # Some findings really are that old, so reporting job-2 would hide them. + first = self._run("job-1", tools=[_tool("a"), _tool("b")]) + baseline = _baseline_from(first, [_tool("a"), _tool("b")]) + baseline.job_id = "job-1" + second = self._run( + "job-2", baseline=baseline, tools=[_tool("a"), _tool("b", "Edited.")] + ) + self.assertEqual( + second.row_fields["mcp_readability_feedback_mode"], cf.MODE_PARTIAL + ) + self.assertEqual(_provenance(second)["baseline_origin_job_id"], "job-1") + + +class PartialRunTest(unittest.TestCase): + """Only changed tools are re-judged; the rest keep their findings.""" + + def setUp(self): + self.tools = [_tool("list_instances"), _tool("create_instance")] + self.baseline_payload = { + "readability_score": 60, + "findings_by_tool": [ + _entry( + "create_instance", + _finding("P0", "Avoid complex params", title="nested config"), + _finding("P1", "Tool Names", title="verb missing"), + ), + _entry("list_instances", _finding("P2", "Concise Descriptions")), + ], + "summary": "Baseline summary.", + } + self.first = _run( + _scorer(_RecordingModel(self.baseline_payload)), self.tools + ) + + def _changed_tools(self): + return [_tool("list_instances"), _tool("create_instance", "Rewritten.")] + + def test_unchanged_tool_findings_are_not_taken_from_the_judge(self): + # The judge volunteers findings for the unchanged tool too; they must be + # discarded, because the filter -- not the prompt -- is the guarantee. + model = _RecordingModel( + { + "readability_score": 90, + "findings_by_tool": [ + _entry("create_instance", _finding("P2", "Tool Names")), + _entry( + "list_instances", + _finding("P0", "Hallucinated"), + ), + ], + "summary": "New summary.", + } + ) + contribution = _run( + _scorer(model), + self._changed_tools(), + baseline=_baseline_from(self.first, self.tools), + ) + self.assertEqual(model.calls, 1) + self.assertEqual( + contribution.row_fields["mcp_readability_feedback_mode"], + cf.MODE_PARTIAL, + ) + by_tool = _by_tool(contribution) + self.assertEqual( + [f["rule_id"] for f in by_tool["list_instances"]], + ["Concise Descriptions"], # carried, not the hallucinated P0 + ) + self.assertEqual( + [f["rule_id"] for f in by_tool["create_instance"]], ["Tool Names"] + ) + self.assertEqual(contribution.row_fields["mcp_readability_p0_issues"], 0) + + def test_prompt_names_the_tools_to_report_on(self): + model = _RecordingModel({"readability_score": 90, "findings_by_tool": []}) + _run( + _scorer(model), + self._changed_tools(), + baseline=_baseline_from(self.first, self.tools), + ) + prompt = model.prompts[0] + self.assertIn("SCOPE OF THIS REVIEW", prompt) + self.assertIn("create_instance", prompt) + # The whole man page is still supplied, for severity calibration. + self.assertIn("man page", prompt) + + def test_removed_tool_findings_are_dropped_and_counts_recomputed(self): + model = _RecordingModel( + {"readability_score": 90, "findings_by_tool": [], "summary": "s"} + ) + contribution = _run( + _scorer(model), + [_tool("list_instances")], # create_instance removed + baseline=_baseline_from(self.first, self.tools), + ) + by_tool = _by_tool(contribution) + self.assertNotIn("create_instance", by_tool) + self.assertEqual(contribution.row_fields["mcp_readability_p0_issues"], 0) + self.assertEqual(contribution.row_fields["mcp_readability_p1_issues"], 0) + self.assertEqual(contribution.row_fields["mcp_readability_p2_issues"], 1) + provenance = _provenance(contribution) + self.assertEqual(provenance["removed_tools"], ["create_instance"]) + + def test_added_tool_is_marked_new(self): + model = _RecordingModel( + { + "readability_score": 80, + "findings_by_tool": [_entry("delete_instance", _finding("P1"))], + "summary": "s", + } + ) + contribution = _run( + _scorer(model), + self.tools + [_tool("delete_instance")], + baseline=_baseline_from(self.first, self.tools), + ) + provenance = _provenance(contribution) + self.assertEqual(provenance["added_tools"], ["delete_instance"]) + self.assertEqual(provenance["tool_provenance"]["delete_instance"], "new") + self.assertEqual( + provenance["tool_provenance"]["list_instances"], "carried" + ) + + def test_carried_findings_keep_their_finding_id(self): + before = _by_tool(self.first)["list_instances"][0]["finding_id"] + contribution = _run( + _scorer( + _RecordingModel({"readability_score": 90, "findings_by_tool": []}) + ), + self._changed_tools(), + baseline=_baseline_from(self.first, self.tools), + ) + after = _by_tool(contribution)["list_instances"][0]["finding_id"] + self.assertEqual(before, after) + + def test_general_entry_is_carried_when_only_descriptions_changed(self): + first = _run( + _scorer( + _RecordingModel( + { + "readability_score": 60, + "findings_by_tool": [ + _entry("general", _finding("P1", "Tool Count Limits")), + _entry("list_instances", _finding("P2")), + ], + "summary": "s", + } + ) + ), + self.tools, + ) + contribution = _run( + _scorer( + _RecordingModel( + { + "readability_score": 90, + "findings_by_tool": [ + _entry("general", _finding("P0", "Invented")), + ], + "summary": "s", + } + ) + ), + self._changed_tools(), + baseline=_baseline_from(first, self.tools), + ) + self.assertEqual( + [f["rule_id"] for f in _by_tool(contribution)["general"]], + ["Tool Count Limits"], + ) + + def test_general_entry_is_refreshed_when_the_tool_set_changes(self): + first = _run( + _scorer( + _RecordingModel( + { + "readability_score": 60, + "findings_by_tool": [ + _entry("general", _finding("P1", "Tool Count Limits")) + ], + "summary": "s", + } + ) + ), + self.tools, + ) + contribution = _run( + _scorer( + _RecordingModel( + { + "readability_score": 90, + "findings_by_tool": [ + _entry("general", _finding("P1", "Inconsistent Naming")) + ], + "summary": "s", + } + ) + ), + self.tools + [_tool("delete_instance")], + baseline=_baseline_from(first, self.tools), + ) + self.assertEqual( + [f["rule_id"] for f in _by_tool(contribution)["general"]], + ["Inconsistent Naming"], + ) + + def test_general_entry_is_rendered_first(self): + first = _run( + _scorer( + _RecordingModel( + { + "readability_score": 60, + "findings_by_tool": [ + _entry("list_instances", _finding("P2")), + _entry("general", _finding("P1")), + ], + "summary": "s", + } + ) + ), + self.tools, + ) + contribution = _run( + _scorer( + _RecordingModel({"readability_score": 90, "findings_by_tool": []}) + ), + self._changed_tools(), + baseline=_baseline_from(first, self.tools), + ) + tools = [e["tool"] for e in _findings(contribution)] + self.assertEqual(tools[0], "general") + + +class CrossToolFindingsTest(unittest.TestCase): + """The ``general`` entry must survive a partial run. + + The focus clause tells the judge to emit entries ONLY for the tools it + lists. If ``general`` is not on that list, an obedient judge omits it, and + sourcing ``general`` from that response loses the baseline's copy too -- + silently dropping cross-tool findings exactly when a rename or removal + makes them most likely. + """ + + def setUp(self): + self.tools = [_tool("a"), _tool("b")] + self.first = _run( + _scorer( + _RecordingModel( + { + "readability_score": 60, + "findings_by_tool": [ + _entry("general", _finding("P1", "Tool Count Limits")), + _entry("a", _finding("P2")), + ], + "summary": "s", + } + ) + ), + self.tools, + ) + + def _obedient_judge(self): + """A judge that emits nothing it was not asked for.""" + return _RecordingModel( + {"readability_score": 60, "findings_by_tool": [], "summary": "s"} + ) + + def test_adding_a_tool_asks_the_judge_for_general(self): + model = self._obedient_judge() + _run( + _scorer(model), + self.tools + [_tool("c")], + baseline=_baseline_from(self.first, self.tools), + ) + focus = _focus_lines(model) + self.assertTrue(focus) + self.assertIn("general", focus[0]) + + def test_removing_a_tool_asks_the_judge_for_general(self): + model = self._obedient_judge() + _run( + _scorer(model), + [_tool("a")], + baseline=_baseline_from(self.first, self.tools), + ) + focus = _focus_lines(model) + self.assertTrue(focus, "a removal-only run must still scope the prompt") + self.assertIn("general", focus[0]) + + def test_description_only_edit_does_not_ask_for_general(self): + # No name change => the baseline's general entry is carried, so there is + # nothing to ask for and no output tokens to spend. + model = self._obedient_judge() + _run( + _scorer(model), + [_tool("a"), _tool("b", "Rewritten.")], + baseline=_baseline_from(self.first, self.tools), + ) + focus = _focus_lines(model) + self.assertNotIn("general", focus[0]) + + def test_an_omitted_general_counts_as_resolved_not_as_a_silent_drop(self): + # Once general is on the focus list, the judge's silence about it means + # "no cross-tool issue" -- the same reading applied to any re-judged + # tool. The finding must therefore disappear *and* be reported as + # resolved, so the count movement is explained rather than mysterious. + before = _by_tool(self.first)["general"][0]["finding_id"] + contribution = _run( + _scorer(self._obedient_judge()), + self.tools + [_tool("c")], + baseline=_baseline_from(self.first, self.tools), + ) + self.assertNotIn("general", _by_tool(contribution)) + self.assertIn(before, _provenance(contribution)["resolved_finding_ids"]) + + def test_a_fresh_general_still_wins_when_the_judge_provides_one(self): + model = _RecordingModel( + { + "readability_score": 60, + "findings_by_tool": [ + _entry("general", _finding("P1", "Inconsistent Naming")) + ], + "summary": "s", + } + ) + contribution = _run( + _scorer(model), + self.tools + [_tool("c")], + baseline=_baseline_from(self.first, self.tools), + ) + self.assertEqual( + [f["rule_id"] for f in _by_tool(contribution)["general"]], + ["Inconsistent Naming"], + ) + + +class BaselineIsolationTest(unittest.TestCase): + """Merging must not write into the shared Baseline the store handed out.""" + + def test_carried_findings_do_not_mutate_the_baseline(self): + tools = [_tool("a")] + first = _run( + _scorer( + _RecordingModel( + { + "readability_score": 60, + "findings_by_tool": [_entry("a", _finding("P1"))], + "summary": "s", + } + ) + ), + tools, + ) + baseline = _baseline_from(first, tools) + # Strip the ids a previous generation minted, as an old baseline would. + for entry in baseline.feedback["findings_by_tool"]: + for finding in entry["findings"]: + finding.pop("finding_id", None) + snapshot = json.dumps(baseline.feedback, sort_keys=True) + + _run(_scorer(_RecordingModel({})), tools, baseline=baseline) + + self.assertEqual( + json.dumps(baseline.feedback, sort_keys=True), + snapshot, + "the store's Baseline was mutated in place", + ) + + def test_two_endpoints_sharing_a_baseline_do_not_interfere(self): + tools = [_tool("a")] + first = _run( + _scorer( + _RecordingModel( + { + "readability_score": 60, + "findings_by_tool": [_entry("a", _finding("P1"))], + "summary": "s", + } + ) + ), + tools, + ) + shared = _baseline_from(first, tools) + one = _run(_scorer(_RecordingModel({})), tools, baseline=shared) + two = _run(_scorer(_RecordingModel({})), tools, baseline=shared) + self.assertEqual(_findings_json(one), _findings_json(two)) + + +class ChangeReasonTest(unittest.TestCase): + """Each judge input that changes names itself in the reason column.""" + + def setUp(self): + self.tools = [_tool("list_instances")] + self.payload = { + "readability_score": 70, + "findings_by_tool": [_entry("list_instances", _finding("P1"))], + "summary": "s", + } + self.first = _run(_scorer(_RecordingModel(self.payload)), self.tools) + + def _reason_after(self, **scorer_changes): + scorer = _scorer(_RecordingModel(self.payload)) + for key, value in scorer_changes.items(): + setattr(scorer, key, value) + contribution = _run( + scorer, self.tools, baseline=_baseline_from(self.first, self.tools) + ) + return contribution.row_fields["mcp_readability_change_reason"] + + def test_style_guide_change(self): + self.assertEqual( + self._reason_after(style_guide_sha="other"), cf.STYLE_GUIDE_CHANGED + ) + + def test_model_change(self): + self.assertEqual( + self._reason_after(judge_model="gemini-2.5-pro"), cf.MODEL_CHANGED + ) + + def test_product_rename_is_an_identity_change(self): + scorer = _scorer(_RecordingModel(self.payload)) + context = EndpointContext( + product_name="AlloyDB Omni", # renamed, explicit id kept the baseline + endpoint={}, + tools=self.tools, + man_page="man page", + exceptions=[], + baseline=cf.BaselineContext( + endpoint_key="key", + tool_fingerprints=tool_fingerprints(self.tools), + baseline=_baseline_from(self.first, self.tools), + ), + ) + self.assertEqual( + scorer.run(context).row_fields["mcp_readability_change_reason"], + cf.ENDPOINT_IDENTITY_CHANGED, + ) + + def test_waiver_change(self): + contribution = _run( + _scorer(_RecordingModel(self.payload)), + self.tools, + baseline=_baseline_from(self.first, self.tools), + exceptions=[{"rule_id": "Tool Names", "reason": "legacy"}], + ) + self.assertEqual( + contribution.row_fields["mcp_readability_change_reason"], + cf.WAIVERS_CHANGED, + ) + + def test_expiry_still_reports_the_delta_against_the_old_review(self): + # An expired baseline must not supply findings, but it is still the + # reference for what moved -- otherwise every finding reads as new. + contribution = _run( + _scorer(_RecordingModel(self.payload)), + self.tools, + baseline=_baseline_from(self.first, self.tools), + override_reason=cf.BASELINE_EXPIRED, + ) + provenance = _provenance(contribution) + self.assertEqual(provenance["previous_finding_count"], 1) + self.assertEqual(provenance["new_finding_ids"], []) + self.assertEqual(provenance["resolved_finding_ids"], []) + html = contribution.row_fields["mcp_readability_llm_feedback_html"] + self.assertIn("2026-09-09", html) + + def test_a_baseline_without_components_is_not_called_a_first_run(self): + baseline = _baseline_from(self.first, self.tools) + baseline.judge_components = {} # written before components were recorded + baseline.judge_fingerprint = "stale" + contribution = _run( + _scorer(_RecordingModel(self.payload)), self.tools, baseline=baseline + ) + self.assertEqual( + contribution.row_fields["mcp_readability_change_reason"], + cf.BASELINE_UNAVAILABLE, + ) + html = contribution.row_fields["mcp_readability_llm_feedback_html"] + self.assertNotIn("first recorded run", html) + self.assertIn("could not be read or compared", html) + + def test_override_reasons_force_a_full_judge(self): + for reason in ( + cf.FORCED_REFRESH, + cf.BASELINE_EXPIRED, + cf.BASELINE_UNAVAILABLE, + ): + with self.subTest(reason=reason): + model = _RecordingModel(self.payload) + contribution = _run( + _scorer(model), + self.tools, + baseline=_baseline_from(self.first, self.tools), + override_reason=reason, + ) + self.assertEqual(model.calls, 1) + self.assertEqual( + contribution.row_fields["mcp_readability_change_reason"], + reason, + ) + + def test_model_change_banner_names_both_models(self): + scorer = _scorer(_RecordingModel(self.payload)) + scorer.judge_model = "gemini-2.5-pro" + contribution = _run( + scorer, self.tools, baseline=_baseline_from(self.first, self.tools) + ) + html = contribution.row_fields["mcp_readability_llm_feedback_html"] + self.assertIn("Judge model changed", html) + self.assertIn("fake-model", html) + self.assertIn("gemini-2.5-pro", html) + + def test_first_run_says_there_is_no_previous_review(self): + html = self.first.row_fields["mcp_readability_llm_feedback_html"] + self.assertIn("No previous review", html) + + +class FindingIdTest(unittest.TestCase): + + def test_same_rule_twice_on_one_tool_gets_distinct_ids(self): + entries = [ + _entry( + "create_instance", + _finding("P1", "Tool Names", locator="database_id"), + _finding("P1", "Tool Names", locator="instance_id"), + ) + ] + cf.mint_finding_ids(entries) + ids = [f["finding_id"] for f in entries[0]["findings"]] + self.assertEqual(len(set(ids)), 2) + + def test_identical_findings_collide_into_an_ordinal_suffix(self): + entries = [ + _entry("a", _finding("P1", "Tool Names"), _finding("P1", "Tool Names")) + ] + cf.mint_finding_ids(entries) + first, second = (f["finding_id"] for f in entries[0]["findings"]) + self.assertEqual(second, f"{first}-1") + + def test_existing_ids_are_preserved(self): + entries = [_entry("a", {"severity": "P1", "finding_id": "keepme"})] + cf.mint_finding_ids(entries) + self.assertEqual(entries[0]["findings"][0]["finding_id"], "keepme") + + def test_id_is_stable_across_runs(self): + def mint(): + entries = [_entry("a", _finding("P1", "Tool Names", title="Fix it"))] + cf.mint_finding_ids(entries) + return entries[0]["findings"][0]["finding_id"] + + self.assertEqual(mint(), mint()) + + def test_title_whitespace_and_case_do_not_change_the_id(self): + def mint(title): + entries = [_entry("a", _finding("P1", "Tool Names", title=title))] + cf.mint_finding_ids(entries) + return entries[0]["findings"][0]["finding_id"] + + self.assertEqual(mint("Fix it"), mint("fix it ")) + + +def _findings(contribution): + return json.loads( + contribution.row_fields["mcp_readability_llm_feedback_json"] + )["findings_by_tool"] + + +def _findings_json(contribution): + return json.dumps(_findings(contribution), sort_keys=True) + + +def _by_tool(contribution): + return {e["tool"]: e["findings"] for e in _findings(contribution)} + + +def _provenance(contribution): + return json.loads( + contribution.row_fields["mcp_readability_feedback_provenance_json"] + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/evalbench/test/mcp_fingerprint_test.py b/evalbench/test/mcp_fingerprint_test.py new file mode 100644 index 00000000..d87b18e6 --- /dev/null +++ b/evalbench/test/mcp_fingerprint_test.py @@ -0,0 +1,165 @@ +"""Unit tests for the MCP readability input fingerprints. + +The whole carry-forward guarantee rests on these: a fingerprint that is stable +when it should not be would silently freeze stale findings, and one that churns +would re-judge everything and defeat the point. +""" + +import unittest + +from mcp import types as mcp_types + +from scorers.mcp_fingerprint import ( + canonical_exceptions, + component_diff, + judge_fingerprint, + tool_fingerprints, + toolset_fingerprint, +) + + +def _tool(name, description="Does a thing.", schema=None): + return mcp_types.Tool( + name=name, + description=description, + inputSchema=schema or {"type": "object"}, + ) + + +class ToolFingerprintsTest(unittest.TestCase): + + def test_identical_tools_fingerprint_identically(self): + self.assertEqual( + tool_fingerprints([_tool("a")]), tool_fingerprints([_tool("a")]) + ) + + def test_description_change_changes_the_fingerprint(self): + before = tool_fingerprints([_tool("a", "Old.")]) + after = tool_fingerprints([_tool("a", "New.")]) + self.assertNotEqual(before["a"], after["a"]) + + def test_schema_change_changes_the_fingerprint(self): + before = tool_fingerprints([_tool("a")]) + after = tool_fingerprints( + [_tool("a", schema={"type": "object", + "properties": {"x": {"type": "string"}}})] + ) + self.assertNotEqual(before["a"], after["a"]) + + def test_other_tools_do_not_affect_a_tool_fingerprint(self): + alone = tool_fingerprints([_tool("a")]) + together = tool_fingerprints([_tool("a"), _tool("b")]) + self.assertEqual(alone["a"], together["a"]) + + def test_duplicate_names_keep_the_first_declaration(self): + fps = tool_fingerprints([_tool("a", "First."), _tool("a", "Second.")]) + self.assertEqual(list(fps), ["a"]) + self.assertEqual(fps["a"], tool_fingerprints([_tool("a", "First.")])["a"]) + + +class ToolsetFingerprintTest(unittest.TestCase): + + def test_reordering_tools_does_not_change_the_toolset_fingerprint(self): + forward = tool_fingerprints([_tool("a"), _tool("b")]) + backward = tool_fingerprints([_tool("b"), _tool("a")]) + self.assertNotEqual(list(forward), list(backward)) # order differs + self.assertEqual( + toolset_fingerprint(forward), toolset_fingerprint(backward) + ) + + def test_adding_a_tool_changes_the_toolset_fingerprint(self): + self.assertNotEqual( + toolset_fingerprint(tool_fingerprints([_tool("a")])), + toolset_fingerprint(tool_fingerprints([_tool("a"), _tool("b")])), + ) + + def test_renaming_a_tool_changes_the_toolset_fingerprint(self): + self.assertNotEqual( + toolset_fingerprint(tool_fingerprints([_tool("a")])), + toolset_fingerprint(tool_fingerprints([_tool("z")])), + ) + + +class CanonicalExceptionsTest(unittest.TestCase): + + def test_reordering_waivers_is_not_a_change(self): + one = [{"rule_id": "b", "reason": "y"}, {"rule_id": "a", "reason": "x"}] + two = [{"rule_id": "a", "reason": "x"}, {"rule_id": "b", "reason": "y"}] + self.assertEqual(canonical_exceptions(one), canonical_exceptions(two)) + + def test_extra_keys_are_dropped(self): + self.assertEqual( + canonical_exceptions([{"rule_id": "a", "reason": "x", "note": "z"}]), + [{"rule_id": "a", "reason": "x"}], + ) + + def test_non_dict_entries_are_ignored(self): + self.assertEqual(canonical_exceptions(["nope", None]), []) + + +class JudgeFingerprintTest(unittest.TestCase): + + def _components(self, **overrides): + components = { + "scorer_name": "mcp_style_readability", + "prompt_version": "1", + "style_guide_sha": "abc", + "judge_model": "gemini-3.1-pro-preview", + "product_name": "AlloyDB", + "exceptions": [], + } + components.update(overrides) + return components + + def test_same_components_same_fingerprint(self): + first, _ = judge_fingerprint(self._components()) + second, _ = judge_fingerprint(self._components()) + self.assertEqual(first, second) + + def test_key_order_does_not_matter(self): + components = self._components() + reversed_order = dict(reversed(list(components.items()))) + self.assertEqual( + judge_fingerprint(components)[0], + judge_fingerprint(reversed_order)[0], + ) + + def test_each_component_change_changes_the_fingerprint(self): + baseline, _ = judge_fingerprint(self._components()) + for key, value in [ + ("style_guide_sha", "def"), + ("judge_model", "gemini-2.5-pro"), + ("prompt_version", "2"), + ("product_name", "Cloud SQL"), + ("exceptions", [{"rule_id": "a", "reason": "x"}]), + ]: + with self.subTest(component=key): + changed, _ = judge_fingerprint(self._components(**{key: value})) + self.assertNotEqual(baseline, changed) + + def test_components_are_returned_for_diffing(self): + _, components = judge_fingerprint(self._components()) + self.assertEqual(components["judge_model"], "gemini-3.1-pro-preview") + + +class ComponentDiffTest(unittest.TestCase): + + def test_names_only_the_changed_components(self): + self.assertEqual( + component_diff( + {"judge_model": "a", "style_guide_sha": "s"}, + {"judge_model": "b", "style_guide_sha": "s"}, + ), + ["judge_model"], + ) + + def test_added_and_removed_components_count_as_changed(self): + self.assertEqual(component_diff({}, {"judge_model": "a"}), ["judge_model"]) + self.assertEqual(component_diff({"judge_model": "a"}, {}), ["judge_model"]) + + def test_identical_components_have_no_diff(self): + self.assertEqual(component_diff({"a": 1}, {"a": 1}), []) + + +if __name__ == "__main__": + unittest.main() diff --git a/evalbench/test/mcp_readability_test.py b/evalbench/test/mcp_readability_test.py index 7ed9eda3..f52e5b81 100644 --- a/evalbench/test/mcp_readability_test.py +++ b/evalbench/test/mcp_readability_test.py @@ -268,6 +268,9 @@ def test_readability_scorer_run(): scorer = McpStyleReadabilityScorer.__new__(McpStyleReadabilityScorer) scorer.name = "mcp_style_readability" scorer.style_guide = "guide" + scorer.style_guide_sha = "sha" + scorer.style_guide_path = "guide.md" + scorer.judge_model = "fake-model" scorer.model = _FakeLLM() # one P1 finding, no P0 ctx = EndpointContext( product_name="p", endpoint={}, tools=[], man_page="mp", exceptions=[] @@ -276,6 +279,9 @@ def test_readability_scorer_run(): assert contrib.row_fields["mcp_readability_p1_issues"] == 1 assert contrib.row_fields["mcp_readability_score"] == 80 assert contrib.score == 100 # no P0 -> pass + # With no baseline configured the endpoint is judged in full, as before. + assert contrib.row_fields["mcp_readability_feedback_mode"] == "full_judge" + assert contrib.row_fields["mcp_readability_change_reason"] == "no_baseline" def test_readability_scorer_requires_style_guide(): @@ -415,6 +421,248 @@ def test_orchestrator_end_to_end(): assert by_comp["mcp_style_readability"]["comparison_error"] is None +class _CountingLLM(_FakeLLM): + """_FakeLLM that records how many times the judge was actually called.""" + + def __init__(self): + self.calls = 0 + + def generate(self, prompt): + self.calls += 1 + return super().generate(prompt) + + +def _persist_evals(rows, output_dir, job_id): + """Write evals.csv exactly as the CSV reporter does. + + Going through ``report.get_dataframe`` matters: it builds the frame with + ``dtype="string"``, so every value the baseline store reads back is a + string. A round trip that skipped it would not catch a column the store + parses wrongly. + """ + from reporting import report + + directory = os.path.join(output_dir, job_id) + os.makedirs(directory, exist_ok=True) + path = os.path.join(directory, "evals.csv") + report.get_dataframe(rows).to_csv(path, index=False) + return path + + +def _feedback_without_provenance(row): + feedback = json.loads(row["mcp_readability_llm_feedback_json"]) + feedback.pop("provenance", None) + return json.dumps(feedback, sort_keys=True) + + +def _run_once(config, output_dir): + """One full orchestrator run; returns ``(row, judge_call_count)``.""" + from evaluator import get_orchestrator + + llm = _CountingLLM() + with patch( + "scorers.mcp_style_readability.get_generator", return_value=llm + ): + orch = get_orchestrator(config, [], {}) + orch.evaluate([]) + job_id, _, results_tf, _, _ = orch.process() + with open(results_tf) as f: + rows = json.load(f) + _persist_evals(rows, output_dir, job_id) + return rows[0], llm.calls + + +def test_carry_forward_round_trip_through_evals_csv(): + """The invariant, end to end: unchanged endpoint => no model call, same bytes. + + This is the only test that exercises the full persistence round trip, so it + is what catches a result column the baseline store reads under the wrong + name -- a failure that would otherwise look like "the baseline never + matches" and silently re-judge forever. + """ + with tempfile.TemporaryDirectory() as d: + results_dir = os.path.join(d, "results") + ep_path = os.path.join(d, "endpoints.yaml") + _write_endpoints( + ep_path, + "Sample", + { + "type": "file", + "path": _ds("datasets/mcp_readability/sample_tools.json"), + }, + ) + config = _base_config(ep_path, results_dir) + config["baseline"] = {"store": "local", "results_dir": results_dir} + + first, first_calls = _run_once(config, results_dir) + assert first_calls == 1 + assert first["mcp_readability_feedback_mode"] == "full_judge" + assert first["mcp_readability_change_reason"] == "no_baseline" + + second, second_calls = _run_once(config, results_dir) + assert second_calls == 0 # the judge was never invoked + assert second["mcp_readability_feedback_mode"] == "carried" + assert second["mcp_readability_change_reason"] == "unchanged" + # Byte-identical findings. The provenance block is excluded because it + # describes *this* run's sourcing, so it necessarily differs -- that is + # the explanation, not the feedback. + assert _feedback_without_provenance(second) == ( + _feedback_without_provenance(first) + ) + for column in ( + "mcp_readability_p0_issues", + "mcp_readability_p1_issues", + "mcp_readability_p2_issues", + "mcp_readability_score", + "mcp_readability_toolset_fingerprint", + "mcp_readability_endpoint_key", + ): + assert second[column] == first[column], column + assert "identical to the previous run" in ( + second["mcp_readability_llm_feedback_html"] + ) + + +def test_carry_forward_style_guide_change_forces_a_full_rejudge(): + with tempfile.TemporaryDirectory() as d: + results_dir = os.path.join(d, "results") + ep_path = os.path.join(d, "endpoints.yaml") + _write_endpoints( + ep_path, + "Sample", + { + "type": "file", + "path": _ds("datasets/mcp_readability/sample_tools.json"), + }, + ) + guide_path = os.path.join(d, "style_guide.md") + with open(guide_path, "w") as f: + f.write("# guide\nRule one.\n") + + config = _base_config(ep_path, results_dir) + config["scorers"]["mcp_style_readability"]["style_guide"] = guide_path + config["baseline"] = {"store": "local", "results_dir": results_dir} + + _run_once(config, results_dir) + with open(guide_path, "w") as f: + f.write("# guide\nRule one.\nRule two.\n") + + second, calls = _run_once(config, results_dir) + assert calls == 1 + assert second["mcp_readability_feedback_mode"] == "full_judge" + assert second["mcp_readability_change_reason"] == "style_guide_changed" + assert "Style guide changed" in ( + second["mcp_readability_llm_feedback_html"] + ) + + +def test_carry_forward_rejudges_only_the_edited_tool(): + """Editing one tool must move that tool's findings and nothing else.""" + with tempfile.TemporaryDirectory() as d: + results_dir = os.path.join(d, "results") + tools_path = os.path.join(d, "tools.json") + with open(_ds("datasets/mcp_readability/sample_tools.json")) as f: + tools = json.load(f) + with open(tools_path, "w") as f: + json.dump(tools, f) + + ep_path = os.path.join(d, "endpoints.yaml") + _write_endpoints(ep_path, "Sample", {"type": "file", "path": tools_path}) + config = _base_config(ep_path, results_dir) + config["baseline"] = {"store": "local", "results_dir": results_dir} + + first, _ = _run_once(config, results_dir) + + edited = tools["tools"][1]["name"] + tools["tools"][1]["description"] = "A rewritten description." + with open(tools_path, "w") as f: + json.dump(tools, f) + + second, calls = _run_once(config, results_dir) + assert calls == 1 + assert second["mcp_readability_feedback_mode"] == "partial" + assert second["mcp_readability_change_reason"] == "tools_changed" + + provenance = json.loads( + second["mcp_readability_feedback_provenance_json"] + ) + assert provenance["rejudged_tools"] == [edited] + assert edited not in provenance["carried_tools"] + + # Every untouched tool keeps exactly the findings it had in run 1. + before = _findings_by_tool(first) + after = _findings_by_tool(second) + for tool in provenance["carried_tools"]: + assert after.get(tool) == before.get(tool), tool + + +def _findings_by_tool(row): + feedback = json.loads(row["mcp_readability_llm_feedback_json"]) + return {e["tool"]: e["findings"] for e in feedback["findings_by_tool"]} + + +def test_baseline_failure_does_not_abort_the_run(): + """A store that raises must cost a re-judge, not the whole job. + + Exercised through the real ThreadPoolExecutor rather than by calling the + helper directly: the orchestrator is otherwise strictly fail-fast, and an + exception escaping a worker would abort every endpoint. + """ + from evaluator import get_orchestrator + + class _ExplodingStore: + def prime(self, endpoint_keys): + raise RuntimeError("no read permission on the dataset") + + def load(self, endpoint_key): + raise RuntimeError("no read permission on the dataset") + + with tempfile.TemporaryDirectory() as d: + ep_path = os.path.join(d, "endpoints.yaml") + _write_endpoints( + ep_path, + "Sample", + { + "type": "file", + "path": _ds("datasets/mcp_readability/sample_tools.json"), + }, + ) + config = _base_config(ep_path, d) + config["baseline"] = {"store": "local", "results_dir": d} + with patch( + "scorers.mcp_style_readability.get_generator", + return_value=_CountingLLM(), + ): + orch = get_orchestrator(config, [], {}) + orch.baseline_store = _ExplodingStore() + orch.evaluate([]) # must not raise + _, _, results_tf, _, _ = orch.process() + with open(results_tf) as f: + row = json.load(f)[0] + assert row["mcp_readability_feedback_mode"] == "full_judge" + assert row["mcp_readability_change_reason"] == "baseline_unavailable" + + +def test_carry_forward_is_off_unless_configured(): + """No baseline block => every run is a full judge, exactly as before.""" + with tempfile.TemporaryDirectory() as d: + results_dir = os.path.join(d, "results") + ep_path = os.path.join(d, "endpoints.yaml") + _write_endpoints( + ep_path, + "Sample", + { + "type": "file", + "path": _ds("datasets/mcp_readability/sample_tools.json"), + }, + ) + config = _base_config(ep_path, results_dir) + _run_once(config, results_dir) + second, calls = _run_once(config, results_dir) + assert calls == 1 + assert second["mcp_readability_feedback_mode"] == "full_judge" + + def test_orchestrator_fetch_error_aborts_run(): """Fail-fast: a fetch failure propagates and aborts the run (nothing stored). diff --git a/evalbench/test/mcp_tool_formatter_test.py b/evalbench/test/mcp_tool_formatter_test.py index d52faad6..86faef05 100644 --- a/evalbench/test/mcp_tool_formatter_test.py +++ b/evalbench/test/mcp_tool_formatter_test.py @@ -9,7 +9,10 @@ from mcp import types as mcp_types -from generators.models.mcp_tool_formatter import format_tools_to_man_page +from generators.models.mcp_tool_formatter import ( + format_tool_section, + format_tools_to_man_page, +) def _tool(name, description, schema): @@ -147,5 +150,54 @@ def test_multiple_tools_all_rendered(self): self.assertIn("TOOL: b", out) +class FormatToolSectionTest(unittest.TestCase): + """The per-tool section is exactly the slice the whole man page is built of. + + The readability check fingerprints these sections to decide what to + re-judge, so any divergence between a section and its place in the man page + would silently re-judge (or fail to re-judge) the wrong tools. + """ + + def _tools(self): + return [ + _tool( + "search", + "Search things.", + { + "type": "object", + "properties": { + "query": {"type": "string", "description": "Query."}, + "limit": {"type": "integer", "enum": [10, 20]}, + }, + "required": ["query"], + }, + ), + _tool("ping", "Health check.", {"type": "object"}), + ] + + def test_man_page_is_the_join_of_its_sections(self): + tools = self._tools() + joined = "\n".join(format_tool_section(t) for t in tools).strip() + self.assertEqual(format_tools_to_man_page(tools), joined) + + def test_each_section_appears_verbatim_in_the_man_page(self): + tools = self._tools() + man_page = format_tools_to_man_page(tools) + for tool in tools: + self.assertIn(format_tool_section(tool).strip(), man_page) + + def test_section_is_independent_of_the_other_tools(self): + first, second = self._tools() + self.assertEqual( + format_tool_section(first), + format_tool_section(first), + ) + # Rendering alone vs. alongside another tool must not differ. + alone = format_tools_to_man_page([first]) + self.assertEqual(alone, format_tool_section(first).strip()) + self.assertNotIn("ping", alone) + self.assertIn("TOOL: ping", format_tool_section(second)) + + if __name__ == "__main__": unittest.main()