From 9a6fa0f4c64a9a80da7e3555d111ab1667655d4c Mon Sep 17 00:00:00 2001 From: Colinho22 <48288595+Colinho22@users.noreply.github.com> Date: Mon, 14 Sep 2026 22:25:19 +0200 Subject: [PATCH 1/3] feat: canonical model registry with CI consistency check Introduces src/maestro/models.py as the single source of truth for model naming, exposed as MODEL_REGISTRY plus a get_model(internal_id) helper. ModelSpec (in schemas.py) carries internal_id, provider_id, display_name, provider_display_name, tier (frontier/efficiency), and an auto-derived snapshot_date so a dated snapshot is a distinct entry, not a mutable attribute. Migrates the viz theme to derive its provider/slot map from the registry so palette assignments cannot drift from pricing. Adds a reported-numbers dump (python -m maestro.analysis.reported_numbers) that writes the docs-referenced totals to a machine-readable JSON file, so transcribed numbers can be diffed against a generated source. A blocking pytest test enforces both shapes of drift: every MODELS entry and every provider dispatch target resolves through the registry, every model-shaped literal in tracked code and docs is a registered id (with a narrow, documented allowlist for vendor prose), and prose totals match reported_numbers.json when the file is present. --- docs/analysis.md | 23 +- src/maestro/analysis/reported_numbers.py | 140 +++++++++ src/maestro/experiment_config.py | 6 + src/maestro/models.py | 188 ++++++++++++ src/maestro/schemas.py | 49 ++- src/maestro/viz/theme.py | 29 +- tests/analysis/test_reported_numbers.py | 136 +++++++++ tests/test_model_registry_consistency.py | 362 +++++++++++++++++++++++ tests/test_models.py | 94 ++++++ 9 files changed, 1010 insertions(+), 17 deletions(-) create mode 100644 src/maestro/analysis/reported_numbers.py create mode 100644 src/maestro/models.py create mode 100644 tests/analysis/test_reported_numbers.py create mode 100644 tests/test_model_registry_consistency.py create mode 100644 tests/test_models.py diff --git a/docs/analysis.md b/docs/analysis.md index ce82365..c72afcb 100644 --- a/docs/analysis.md +++ b/docs/analysis.md @@ -249,7 +249,28 @@ them in `pyproject.toml` is what keeps historical numbers stable. --- -## 8. Related documentation +## 8. Reported-numbers dump + +A companion entry point, + +```bash +python -m maestro.analysis.reported_numbers +``` + +emits `output/analysis/reported_numbers.json`: the headline totals docs +and slide decks cite (total cell count, success / failure split, +aggregate cost). The file is the machine-readable source that +transcribed numbers in tracked prose must match, and the model-registry +consistency test in CI enforces that. Regenerate it after any run that +changes those totals, then update the docs from the file rather than +from a screenshot of the runner output. + +Empty database produces a valid `status: "empty"` payload so a fresh +checkout can still write the file and pass the consistency test. + +--- + +## 9. Related documentation - `docs/schema.md`: full database schema reference. - `docs/running.md`: how to produce the database in the first place. diff --git a/src/maestro/analysis/reported_numbers.py b/src/maestro/analysis/reported_numbers.py new file mode 100644 index 0000000..ac99a50 --- /dev/null +++ b/src/maestro/analysis/reported_numbers.py @@ -0,0 +1,140 @@ +""" +Canonical reported-numbers dump: the values docs quote and slide decks cite +are computed from the results database here and written to a +machine-readable JSON file. Downstream text (README, CHANGELOG, docs +markdown) is checked against this file by the consistency test, so a +transcribed number cannot silently disagree with the database. + +Scope is deliberately narrow: only headline totals that already appear in +the docs today (total cell count, success / failure split, aggregate +cost). Statistical results have their own richer artefacts under +``maestro.analysis.__main__``; this module is the join point where prose +meets truth. +""" + +from __future__ import annotations + +import argparse +import json +import sqlite3 +from pathlib import Path +from typing import Any + +from maestro.db.client import get_readonly_connection +from maestro.experiment_config import DB_PATH + +# Emitted schema version. Bump when a field is renamed or its meaning +# changes; consumers (docs, the consistency check) can then pin against a +# specific shape rather than a moving target. +SCHEMA_VERSION = "1.0" + +# Fixed relative location under the analysis output tree. Kept stable so +# the consistency check does not need a discovery step. +DEFAULT_OUTPUT_PATH = ( + Path(__file__).resolve().parents[3] + / "output" + / "analysis" + / "reported_numbers.json" +) + + +def compute_reported_numbers(conn: sqlite3.Connection) -> dict[str, Any]: + """ + Aggregate the docs-referenced totals from a results DB, read-only. + + Returns ``status="empty"`` when no runs have been recorded yet: every + numeric field is still present but set to 0 so a downstream diff still + works against a fresh (v2.0.0) checkout. + """ + totals_row = conn.execute( + """ + SELECT + COUNT(*) AS total_runs, + COALESCE(SUM(CASE WHEN error IS NULL AND output_diagram_code IS NOT NULL + AND TRIM(output_diagram_code) <> '' + THEN 1 ELSE 0 END), 0) AS successes, + COALESCE(SUM(CASE WHEN error IS NOT NULL OR output_diagram_code IS NULL + OR TRIM(COALESCE(output_diagram_code, '')) = '' + THEN 1 ELSE 0 END), 0) AS failures, + COALESCE(SUM(cost_usd), 0.0) AS total_cost_usd + FROM run_results + """ + ).fetchone() + + total_runs = int(totals_row["total_runs"]) + if total_runs == 0: + return { + "schema_version": SCHEMA_VERSION, + "status": "empty", + "total_runs": 0, + "successes": 0, + "failures": 0, + "total_cost_usd": 0.0, + } + + return { + "schema_version": SCHEMA_VERSION, + "status": "ok", + "total_runs": total_runs, + "successes": int(totals_row["successes"]), + "failures": int(totals_row["failures"]), + # Rounded to cents: docs quote e.g. USD 171.62, so writing extra + # digits would create a mismatch on the last decimal that is + # cosmetic, not real. Two-place rounding matches the reporting + # convention and keeps the diff meaningful. + "total_cost_usd": round(float(totals_row["total_cost_usd"]), 2), + } + + +def _parse_args(argv: list[str] | None = None) -> argparse.Namespace: + """Parse the CLI (``--db``, ``--out``).""" + parser = argparse.ArgumentParser( + prog="python -m maestro.analysis.reported_numbers", + description=( + "Dump the docs-referenced headline totals to a machine-readable " + "JSON file. Consumed by the model-registry consistency check." + ), + ) + parser.add_argument( + "--db", + type=Path, + default=DB_PATH, + help=f"Path to the experiment SQLite database (default: {DB_PATH}).", + ) + parser.add_argument( + "--out", + type=Path, + default=DEFAULT_OUTPUT_PATH, + help=( + "Destination file for the reported-numbers JSON " + f"(default: {DEFAULT_OUTPUT_PATH})." + ), + ) + return parser.parse_args(argv) + + +def main(argv: list[str] | None = None) -> int: + """ + Write the reported-numbers JSON file. Exit code 1 if the DB path does + not exist; 0 otherwise (including an empty DB, which still emits a + valid file so the docs pipeline can run against a fresh checkout). + """ + args = _parse_args(argv) + if not args.db.exists(): + print(f"ERROR: database not found: {args.db}") + return 1 + + with get_readonly_connection(args.db) as conn: + payload = compute_reported_numbers(conn) + + args.out.parent.mkdir(parents=True, exist_ok=True) + args.out.write_text( + json.dumps(payload, indent=2, sort_keys=False) + "\n", + encoding="utf-8", + ) + print(f"Wrote {args.out}") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/src/maestro/experiment_config.py b/src/maestro/experiment_config.py index 68c5f70..41c3b83 100644 --- a/src/maestro/experiment_config.py +++ b/src/maestro/experiment_config.py @@ -297,6 +297,12 @@ # April 2026 for the frozen main run. IDs are pinned to dated snapshots where # the provider offers one, so the run stays reproducible. # +# The model id on each row is the canonical internal_id from +# maestro.models.MODEL_REGISTRY (the single source of truth for model +# naming). A consistency test asserts every non-control entry here is +# registered, so a typo cannot land pricing under a name no other layer +# recognises. +# # Note: provider dispatch (run.py) is by substring (claude / gpt / mistral / # gemini / deepseek), so any new model id must contain its provider's needle. # tests/providers/test_provider_dispatch.py enforces this for every entry here. diff --git a/src/maestro/models.py b/src/maestro/models.py new file mode 100644 index 0000000..6b0a673 --- /dev/null +++ b/src/maestro/models.py @@ -0,0 +1,188 @@ +""" +Canonical model registry: the single source of truth for every model name +MAESTRO uses. + +Every place that names a model (pricing table, provider dispatch, viz +palettes, docs, tests) resolves through ``get_model`` or one of the +membership helpers. That eliminates the class of drift where the same +model appears under two spellings in two places, and it gives future +issues (pricing config, model-specific behaviour) a stable join key. + +A dated snapshot is a distinct registry entry, not a mutable attribute +(the ``ModelSpec`` docstring in ``schemas.py`` records why): swapping +``claude-haiku-4-5-20251001`` for a newer snapshot means adding a new +row here, not editing this one. +""" + +from __future__ import annotations + +import re + +from maestro.schemas import ModelSpec + +# Frontier == flagship; efficiency == smaller/cheaper. Two tiers per provider +# is the shape MAESTRO's paired matrix depends on; if that changes, this +# constant and viz theme's frontier/efficiency slot mapping move together. +TIER_FRONTIER = "frontier" +TIER_EFFICIENCY = "efficiency" + +# Regex over an internal id's terminal segment: 8 consecutive digits count as +# a snapshot date (e.g. the 20251001 in claude-haiku-4-5-20251001), 4 do not +# (a version number). Loose match is deliberate: a vendor's convention is +# not something the registry can normalise, only expose. +_SNAPSHOT_DATE_PATTERN = re.compile(r"-(\d{8})$") + + +def _snapshot_of(internal_id: str) -> str | None: + """Extract an 8-digit snapshot date from the tail of an internal id.""" + match = _SNAPSHOT_DATE_PATTERN.search(internal_id) + return match.group(1) if match else None + + +def _spec( + internal_id: str, + *, + provider_id: str, + display_name: str, + provider_display_name: str, + tier: str, +) -> ModelSpec: + """Build a ``ModelSpec`` and auto-derive its snapshot date if present.""" + return ModelSpec( + internal_id=internal_id, + provider_id=provider_id, + display_name=display_name, + provider_display_name=provider_display_name, + tier=tier, + snapshot_date=_snapshot_of(internal_id), + ) + + +# --------------------------------------------------------------------------- +# Canonical registry +# --------------------------------------------------------------------------- +# +# Order in this list is the order every downstream consumer displays models +# in (viz legends, printed tables, docs generation). Alphabetical by +# provider, frontier-then-efficiency within each provider: the same shape +# ``experiment_config.MODELS`` uses, kept in step so the two layers can be +# diffed at a glance. + +_REGISTRY_ENTRIES: tuple[ModelSpec, ...] = ( + _spec( + "claude-opus-4-8", + provider_id="anthropic", + display_name="Claude Opus 4.8", + provider_display_name="Claude", + tier=TIER_FRONTIER, + ), + _spec( + "claude-haiku-4-5-20251001", + provider_id="anthropic", + display_name="Claude Haiku 4.5", + provider_display_name="Claude", + tier=TIER_EFFICIENCY, + ), + _spec( + "gpt-5.5-2026-04-23", + provider_id="openai", + display_name="GPT-5.5", + provider_display_name="ChatGPT", + tier=TIER_FRONTIER, + ), + _spec( + "gpt-5.4-mini-2026-03-17", + provider_id="openai", + display_name="GPT-5.4 mini", + provider_display_name="ChatGPT", + tier=TIER_EFFICIENCY, + ), + _spec( + "mistral-medium-3-5", + provider_id="mistral", + display_name="Mistral Medium 3.5", + provider_display_name="Mistral", + tier=TIER_FRONTIER, + ), + _spec( + "mistral-small-2603", + provider_id="mistral", + display_name="Mistral Small", + provider_display_name="Mistral", + tier=TIER_EFFICIENCY, + ), + _spec( + "gemini-3.5-flash", + provider_id="gemini", + display_name="Gemini 3.5 Flash", + provider_display_name="Gemini", + tier=TIER_FRONTIER, + ), + _spec( + "gemini-3.1-flash-lite", + provider_id="gemini", + display_name="Gemini 3.1 Flash Lite", + provider_display_name="Gemini", + tier=TIER_EFFICIENCY, + ), + _spec( + "deepseek-v4-pro", + provider_id="deepseek", + display_name="DeepSeek V4 Pro", + provider_display_name="DeepSeek", + tier=TIER_FRONTIER, + ), + _spec( + "deepseek-v4-flash", + provider_id="deepseek", + display_name="DeepSeek V4 Flash", + provider_display_name="DeepSeek", + tier=TIER_EFFICIENCY, + ), +) + + +MODEL_REGISTRY: dict[str, ModelSpec] = { + spec.internal_id: spec for spec in _REGISTRY_ENTRIES +} + + +# --------------------------------------------------------------------------- +# Public accessors +# --------------------------------------------------------------------------- + + +def get_model(internal_id: str) -> ModelSpec: + """ + Resolve an internal id to its ``ModelSpec``. Raises ``KeyError`` with a + clear message on an unknown id: silently returning ``None`` would let a + typo propagate into a chart or a pricing lookup, which is exactly the + drift this registry exists to prevent. + """ + spec = MODEL_REGISTRY.get(internal_id) + if spec is None: + raise KeyError( + f"Unknown model internal_id: {internal_id!r}. " + f"Known: {', '.join(sorted(MODEL_REGISTRY))}" + ) + return spec + + +def all_internal_ids() -> set[str]: + """Set of every registered internal id (used by consistency scans).""" + return set(MODEL_REGISTRY) + + +def registered_models() -> list[ModelSpec]: + """Ordered list of every registered ``ModelSpec`` (registry order).""" + return list(_REGISTRY_ENTRIES) + + +def models_by_provider(provider_id: str) -> list[ModelSpec]: + """ + Every ``ModelSpec`` whose ``provider_id`` matches, in registry order. + Empty list on an unknown provider id: the caller decides whether that + is an error (a viz palette missing a provider) or a natural no-op (a + key-filtered run that excluded the whole vendor). + """ + return [spec for spec in _REGISTRY_ENTRIES if spec.provider_id == provider_id] diff --git a/src/maestro/schemas.py b/src/maestro/schemas.py index 31bff44..4ac2d4e 100644 --- a/src/maestro/schemas.py +++ b/src/maestro/schemas.py @@ -91,7 +91,8 @@ class RunConfig(BaseModel): run_id: UUID = Field(default_factory=uuid4) strategy: Strategy - model: str # e.g. "gpt-4o", "claude-3-5-sonnet" + # A registered internal_id from maestro.models.MODEL_REGISTRY. + model: str example_id: str # FK to InputFile.example_id tier: Tier run_number: int # Repeat index within same config (1-N) @@ -139,6 +140,52 @@ class RunEnvironment(BaseModel): captured_at: datetime = Field(default_factory=lambda: datetime.now(timezone.utc)) +# --------------------------------------------------------------------------- +# ModelSpec: canonical registry entry for a benchmarked model +# --------------------------------------------------------------------------- + + +class ModelSpec(BaseModel): + """ + Canonical identity of one benchmarked model, referenced everywhere a + model is named. The registry (``maestro.models.MODEL_REGISTRY``) is the + only place these fields are defined; pricing (``ModelPricing``), viz + palettes, provider dispatch and analysis all join back through + ``internal_id``. + + A dated snapshot is a distinct registry entry, not a mutable attribute: + each vendor-pinned snapshot id (for example the dated Haiku snapshot) + is its own row, and an undated alias would be another row entirely. + Cross-snapshot comparability is a research question, not a registry + concern. + """ + + # Stored verbatim in ``RunConfig.model`` and ``ModelPricing.model``, + # so this string IS the join key. Never rename an id once data has + # been collected under it: rows would silently unjoin. + internal_id: str + # Vendor / SDK identity, matching each provider's ``_PROVIDER_NAME`` + # (e.g. ``"anthropic"``, ``"openai"``). Distinct from the model-name + # needle in ``run.py:_PROVIDER_DISPATCH`` (``"claude"`` / ``"gpt"`` / + # ...) which is a dispatch-implementation detail, not a stable id. + provider_id: str + # Human display name of the specific model (e.g. ``"Claude Opus 4.8"``). + display_name: str + # Human display name of the vendor (e.g. ``"Claude"``). Kept beside + # ``display_name`` so the viz layer's provider-keyed palettes join + # directly against the registry. + provider_display_name: str + # Provider positioning within MAESTRO's paired experiment matrix: + # ``"frontier"`` (flagship) or ``"efficiency"`` (smaller/cheaper). + # Drives the viz slot mapping (frontier == slot 1, efficiency == 0). + tier: str + # Snapshot date embedded in the model id where the vendor pins one + # (an 8-digit tail like ``20251001``). ``None`` when the vendor rolls + # the id forward silently; that is a reproducibility risk the docs + # note, not a registry concern. + snapshot_date: str | None = None + + # --------------------------------------------------------------------------- # ModelPricing: lookup table for cost calculation # --------------------------------------------------------------------------- diff --git a/src/maestro/viz/theme.py b/src/maestro/viz/theme.py index dfbe3aa..a433bc6 100644 --- a/src/maestro/viz/theme.py +++ b/src/maestro/viz/theme.py @@ -21,6 +21,8 @@ import matplotlib as mpl import matplotlib.pyplot as plt +from maestro.models import TIER_FRONTIER, registered_models + # --------------------------------------------------------------------------- # Palettes: verbatim from the design guide (display-name keys). # --------------------------------------------------------------------------- @@ -78,8 +80,9 @@ # DB-value -> guide-display mappings. # # The database stores enum values and full model ids; the guide keys palettes -# by display name. These dicts are the bridge. Keep them in sync with -# maestro.schemas.Strategy and maestro.experiment_config.MODELS. +# by display name. These dicts are the bridge. Strategy values must stay in +# step with maestro.schemas.Strategy; model ids come from +# maestro.models.MODEL_REGISTRY (the canonical model registry). # --------------------------------------------------------------------------- # Strategy enum value (run_configs.strategy) -> guide display name. @@ -101,20 +104,16 @@ "ground_truth_control": "Ground Truth Control", } -# Model id (run_configs.model) -> (provider display name, slot) where slot is -# 0 for the efficiency model and 1 for the frontier model. Keep this in sync -# with experiment_config.MODELS (two ids per provider). +# Model id (run_configs.model) -> (provider display name, slot) where slot +# is 0 for the efficiency model and 1 for the frontier model. Derived from +# the canonical registry so palette assignments and provider dispatch +# cannot drift. _MODEL_TO_PROVIDER_SLOT: dict[str, tuple[str, int]] = { - "claude-opus-4-8": ("Claude", 1), - "claude-haiku-4-5-20251001": ("Claude", 0), - "gpt-5.5-2026-04-23": ("ChatGPT", 1), - "gpt-5.4-mini-2026-03-17": ("ChatGPT", 0), - "mistral-medium-3-5": ("Mistral", 1), - "mistral-small-2603": ("Mistral", 0), - "gemini-3.5-flash": ("Gemini", 1), - "gemini-3.1-flash-lite": ("Gemini", 0), - "deepseek-v4-pro": ("DeepSeek", 1), - "deepseek-v4-flash": ("DeepSeek", 0), + spec.internal_id: ( + spec.provider_display_name, + 1 if spec.tier == TIER_FRONTIER else 0, + ) + for spec in registered_models() } # Module-level guard so the rcParams update runs once per process even if diff --git a/tests/analysis/test_reported_numbers.py b/tests/analysis/test_reported_numbers.py new file mode 100644 index 0000000..831d697 --- /dev/null +++ b/tests/analysis/test_reported_numbers.py @@ -0,0 +1,136 @@ +""" +Reported-numbers dump: aggregate arithmetic and empty-DB behaviour. + +The whole point of this module is that the values docs quote are computed +from a fresh DB read, not maintained by hand. So the tests pin both +paths: an empty DB (fresh checkout) yields a valid empty-status payload, +and a populated DB reports the same totals the runner prints on exit. +""" + +from __future__ import annotations + +from pathlib import Path +from uuid import uuid4 + +import pytest + +from maestro.analysis.reported_numbers import ( + SCHEMA_VERSION, + compute_reported_numbers, +) +from maestro.db.client import get_connection, init_db +from maestro.schemas import RunConfig, RunResult, Strategy, Tier + + +@pytest.fixture() +def db_path(tmp_path: Path) -> Path: + """Fresh SQLite database with the MAESTRO schema, for one test.""" + path = tmp_path / "reported.db" + init_db(path) + return path + + +def _insert_result( + conn, + *, + strategy: Strategy, + example_id: str, + output: str | None, + cost_usd: float, + error: str | None, +) -> None: + """Persist a RunConfig / RunResult pair so the aggregate query can see it.""" + from maestro.db.queries import insert_run_config, insert_run_result + + config = RunConfig( + run_id=uuid4(), + strategy=strategy, + model="claude-opus-4-8", + example_id=example_id, + tier=Tier.SIMPLE, + run_number=1, + ) + insert_run_config(conn, config) + insert_run_result( + conn, + RunResult( + run_id=config.run_id, + output_diagram_code=output, + prompt_tokens=1, + completion_tokens=1, + duration_ms=1, + cost_usd=cost_usd, + error=error, + ), + ) + + +def test_empty_db_returns_zeroed_payload(db_path: Path) -> None: + with get_connection(db_path) as conn: + payload = compute_reported_numbers(conn) + + assert payload["schema_version"] == SCHEMA_VERSION + assert payload["status"] == "empty" + assert payload["total_runs"] == 0 + assert payload["successes"] == 0 + assert payload["failures"] == 0 + assert payload["total_cost_usd"] == 0.0 + + +def test_populated_db_reports_split_and_cost(db_path: Path) -> None: + with get_connection(db_path) as conn: + _insert_result( + conn, + strategy=Strategy.SINGLE_AGENT, + example_id="bpmn_1_01", + output="graph TD; a-->b", + cost_usd=0.01, + error=None, + ) + _insert_result( + conn, + strategy=Strategy.SOP_BASED, + example_id="bpmn_1_02", + output="graph TD; c-->d", + cost_usd=0.02, + error=None, + ) + _insert_result( + conn, + strategy=Strategy.CREW_AI, + example_id="bpmn_1_03", + output=None, + cost_usd=0.0, + error="RateLimitError: 429", + ) + + with get_connection(db_path) as conn: + payload = compute_reported_numbers(conn) + + assert payload["status"] == "ok" + assert payload["total_runs"] == 3 + assert payload["successes"] == 2 + assert payload["failures"] == 1 + # Cost is rounded to two decimals; matches how docs quote it. + assert payload["total_cost_usd"] == 0.03 + + +def test_empty_output_counts_as_failure(db_path: Path) -> None: + """A run with an empty-string diagram is not a valid success, so the + aggregate must not double-count it as one.""" + with get_connection(db_path) as conn: + _insert_result( + conn, + strategy=Strategy.SINGLE_AGENT, + example_id="bpmn_1_01", + output=" ", + cost_usd=0.0, + error=None, + ) + + with get_connection(db_path) as conn: + payload = compute_reported_numbers(conn) + + assert payload["total_runs"] == 1 + assert payload["successes"] == 0 + assert payload["failures"] == 1 diff --git a/tests/test_model_registry_consistency.py b/tests/test_model_registry_consistency.py new file mode 100644 index 0000000..1afd03e --- /dev/null +++ b/tests/test_model_registry_consistency.py @@ -0,0 +1,362 @@ +""" +Consistency checks for the canonical model registry. + +Two shapes of drift are blocked in CI here so the registry stays the single +source of truth its docstring claims: + +1. **Name drift.** A model referenced by a name not in ``MODEL_REGISTRY`` + (a typo in pricing, a lingering literal in the viz or docs) fails the + scan. The scan is deliberately strict: it walks tracked files under + ``src/`` (excluding the registry module), ``docs/``, ``README.md``, and + ``CHANGELOG.md``, and asserts every model-shaped literal it finds is a + registered internal id. + +2. **Number drift.** When ``output/analysis/reported_numbers.json`` exists + (produced by ``python -m maestro.analysis.reported_numbers``), any + docs-referenced total that appears verbatim in tracked prose must match + the value from the file. If the file does not exist yet (a fresh + checkout with an empty DB), the numeric check self-skips: the goal is + to prevent silent divergence, not to force a run before the DB has + data. +""" + +from __future__ import annotations + +import json +import re +from pathlib import Path + +import pytest + +from maestro.experiment_config import CONTROL_MODEL, MODELS +from maestro.models import MODEL_REGISTRY, all_internal_ids, get_model + +_REPO_ROOT = Path(__file__).resolve().parents[1] +_SRC_ROOT = _REPO_ROOT / "src" / "maestro" +_DOCS_ROOT = _REPO_ROOT / "docs" +_REPORTED_NUMBERS_PATH = _REPO_ROOT / "output" / "analysis" / "reported_numbers.json" + +# Files exempt from the model-literal scan. The registry module and its +# tests are the authoritative source; scanning them would just re-flag +# every entry. ``analysis_tables.ipynb`` and ``core-visualization.ipynb`` +# hold viz-layer display maps that should migrate to the registry once the +# notebook-to-figures extraction lands, but they are notebooks (JSON, not +# code review targets) and blocking on them right now would delay the +# blocking check itself. Tracked here explicitly so the exemption is +# reviewable. +_SCAN_EXEMPT_RELATIVE: frozenset[Path] = frozenset( + { + Path("src/maestro/models.py"), + Path("src/maestro/viz/analysis_tables.ipynb"), + Path("src/maestro/viz/core-visualization.ipynb"), + Path("tests/test_model_registry_consistency.py"), + } +) + +# Prose-scan file globs relative to the repo root. Deliberately narrow to +# code and human-authored docs; the DB, generated notebooks output, and +# vendor lock files are outside the review surface. +_SCAN_GLOBS: tuple[tuple[Path, str], ...] = ( + (_SRC_ROOT, "**/*.py"), + (_DOCS_ROOT, "**/*.md"), + (_REPO_ROOT, "README.md"), + (_REPO_ROOT, "CHANGELOG.md"), + (_REPO_ROOT / "tests", "**/*.py"), +) + +# Regex that matches a model-shaped internal id: a known provider needle +# followed by a hyphen and a version / snapshot tail. Same needles +# ``run.py`` dispatches on, so a legitimate identifier here always +# resolves. Requiring an explicit hyphen after the needle rules out python +# module paths (``mistralai``), CamelCase class names (``DeepSeekProvider``, +# excluded also by case-sensitive matching), snake_case identifiers +# (``deepseek_p``), and vendor domains (``mistral.ai``, ``deepseek.com``, +# ``deepseek.py``). Case sensitive: real model ids are lowercase. +_PROVIDER_NEEDLES = ("claude", "gpt", "mistral", "gemini", "deepseek") +_MODEL_LITERAL_PATTERN = re.compile( + r"\b(" + "|".join(_PROVIDER_NEEDLES) + r")-[a-z0-9][a-z0-9.\-]*" +) + +# Substrings whose match is deliberately not a target model id. Kept +# narrow on purpose: broadening it here would defeat the point of the +# scan. Each entry is a literal (lowercase, matches the pattern above). +_LITERAL_ALLOWLIST: frozenset[str] = frozenset( + { + # Generic version families referenced in provider docstrings. + # Not registered because they are examples of a lineage, not the + # pinned snapshot the matrix runs. + "gpt-4o", + "gpt-5", + "gpt-5-family", + "gpt-5.4-mini", + "gpt-5.5", + "mistral-large", + "mistral-small", + # Historical model literals mentioned in provider comments to + # document past behaviour; not part of the active matrix. + "claude-haiku-4-5", + # Test fixture used by legacy viz tests: a synthetic id that + # exercises the display path without needing a registry entry. + # Migration to a registered id is tracked separately from the + # registry rollout. + "gpt-4o-mini-2024-07-18", + # Design-guide palette label (docs/visualization_design_guide.md). + "claude-coral", + # URL fragment for Google's Gemini API documentation, not a model id. + "gemini-api", + } +) + + +def _tracked_files() -> list[Path]: + """Every reviewable file matched by ``_SCAN_GLOBS``, excluding exemptions.""" + seen: set[Path] = set() + files: list[Path] = [] + for root, pattern in _SCAN_GLOBS: + if not root.exists(): + continue + if pattern.startswith("**"): + candidates = root.glob(pattern) + else: + # Single-file glob, e.g. README.md. + candidate = root / pattern + candidates = [candidate] if candidate.exists() else [] + for path in candidates: + if not path.is_file(): + continue + rel = path.relative_to(_REPO_ROOT) + if rel in _SCAN_EXEMPT_RELATIVE: + continue + if rel in seen: + continue + seen.add(rel) + files.append(path) + return files + + +def _find_unregistered_literals(text: str) -> set[str]: + """ + Model-shaped literals in ``text`` that are neither in the registry + nor on the allowlist. Match is case-sensitive lowercase (the pattern + itself enforces it), which lets ``MistralProvider`` and other + CamelCase references pass without an explicit exemption. + """ + unregistered: set[str] = set() + for match in _MODEL_LITERAL_PATTERN.finditer(text): + raw = match.group(0) + # Strip trailing punctuation that regex does not eat (a comma, + # backtick, closing paren) so the compare is against the actual + # identifier the author wrote. + candidate = raw.rstrip(".,:;)\"'`") + if candidate in MODEL_REGISTRY: + continue + if candidate in _LITERAL_ALLOWLIST: + continue + unregistered.add(candidate) + return unregistered + + +# --------------------------------------------------------------------------- +# Registry integrity +# --------------------------------------------------------------------------- + + +def test_registry_is_non_empty(): + """A registry with no entries would silently pass every downstream check.""" + assert MODEL_REGISTRY, "MODEL_REGISTRY is empty" + + +def test_get_model_returns_matching_spec(): + for internal_id in all_internal_ids(): + spec = get_model(internal_id) + assert spec.internal_id == internal_id + + +def test_get_model_raises_keyerror_with_message_on_unknown(): + with pytest.raises(KeyError) as excinfo: + get_model("no-such-model-xyz") + # The error message names the offending id AND lists the registered + # ones; a bare KeyError would leave the user to grep. + assert "no-such-model-xyz" in str(excinfo.value) + assert "claude" in str(excinfo.value).lower() + + +# --------------------------------------------------------------------------- +# Pricing table and provider dispatch resolve through the registry +# --------------------------------------------------------------------------- + + +def test_every_pricing_entry_is_registered(): + """ + Every real ``ModelPricing`` row (all except the synthetic ``control`` + model) has a matching registry entry, so cost calculations and + scoring cannot resolve a row that no other layer recognises. + """ + for mp in MODELS: + if mp.model == CONTROL_MODEL.model: + continue + assert mp.model in MODEL_REGISTRY, ( + f"pricing entry '{mp.model}' has no matching registry row" + ) + + +def test_every_provider_dispatch_target_resolves_through_registry(): + """ + Each provider needle in ``run.py:_PROVIDER_DISPATCH`` claims at least + one registered model (so a needle without a live user gets caught). + """ + from maestro.run import _PROVIDER_DISPATCH, _dispatch_for_model + + needles = {needle for needle, _, _ in _PROVIDER_DISPATCH} + covered: set[str] = set() + for spec in MODEL_REGISTRY.values(): + dispatch = _dispatch_for_model(spec.internal_id) + assert dispatch is not None, ( + f"registered model '{spec.internal_id}' does not dispatch to a provider" + ) + needle, _, _ = dispatch + covered.add(needle) + assert needles == covered, ( + f"provider dispatch needles have no registered users: {needles - covered}" + ) + + +# --------------------------------------------------------------------------- +# Prose scan: no unregistered model literals in tracked files +# --------------------------------------------------------------------------- + + +def test_no_unregistered_model_literals_in_tracked_files(): + """ + Every model-shaped literal in tracked code and docs resolves through + the registry. Deliberately strict: the whole point of the registry is + that a rename touches one place, not five. + """ + offenders: dict[str, set[str]] = {} + for path in _tracked_files(): + try: + text = path.read_text(encoding="utf-8") + except UnicodeDecodeError: + continue + bad = _find_unregistered_literals(text) + if bad: + offenders[str(path.relative_to(_REPO_ROOT))] = bad + + assert not offenders, ( + "Unregistered model-shaped literals found. Add them to " + "maestro.models.MODEL_REGISTRY or, if they are not model ids, " + "extend _LITERAL_ALLOWLIST with a one-line justification. " + f"Offenders: {offenders}" + ) + + +# --------------------------------------------------------------------------- +# Numeric drift: docs match the machine-readable results dump +# --------------------------------------------------------------------------- + + +# Numbers we want the docs to keep in step with the generated file. Each +# entry is (json_field, list_of_literals_that_may_appear_in_docs). +# ``total_cost_usd`` is compared as a float; the count fields are compared +# with and without thousands separators (docs use both). +_TRANSCRIBED_NUMBERS: tuple[tuple[str, tuple[str, ...]], ...] = ( + ("total_runs", ("total_runs",)), + ("successes", ("successes",)), + ("failures", ("failures",)), + ("total_cost_usd", ("total_cost_usd",)), +) + + +def _load_reported_numbers() -> dict | None: + """Return the JSON payload if present, else ``None`` (skip signal).""" + if not _REPORTED_NUMBERS_PATH.exists(): + return None + return json.loads(_REPORTED_NUMBERS_PATH.read_text(encoding="utf-8")) + + +def _prose_files() -> list[Path]: + """Human-authored prose subset of ``_tracked_files``: docs + top-level.""" + return [ + path + for path in _tracked_files() + if path.suffix.lower() in {".md"} or path.name in {"README.md", "CHANGELOG.md"} + ] + + +def test_docs_totals_match_reported_numbers_file(): + """ + If a reported-numbers dump exists, docs must not contradict it. On a + fresh checkout (no DB, no dump) this self-skips: the check exists to + catch drift, not to require the DB be populated before merge. + """ + payload = _load_reported_numbers() + if payload is None: + pytest.skip( + "output/analysis/reported_numbers.json not present; " + "run `python -m maestro.analysis.reported_numbers` to enable." + ) + if payload.get("status") == "empty": + pytest.skip("reported_numbers.json reports an empty database") + + # Build the set of literal strings each field is allowed to appear as + # in prose. Integers accept a bare form and a thousands-separator form + # (docs use both), floats accept a two-decimal string. + allowed: dict[str, set[str]] = {} + for field, _labels in _TRANSCRIBED_NUMBERS: + value = payload.get(field) + if value is None: + continue + if isinstance(value, int): + allowed[field] = {str(value), f"{value:,}"} + elif isinstance(value, float): + allowed[field] = {f"{value:.2f}", f"{value:,.2f}"} + else: + allowed[field] = {str(value)} + + if not allowed: + pytest.skip("reported_numbers.json has no comparable fields") + + # Sanity: whenever a prose file mentions "USD ..." right before a + # numeric literal, that literal must match total_cost_usd. Similarly + # for the cell count. Kept pattern-driven rather than free-form so + # this can catch reworded prose that keeps the numbers. + mismatches: list[str] = [] + cost_pattern = re.compile(r"USD\s+(\d+(?:,\d{3})*(?:\.\d+)?)") + cell_pattern = re.compile(r"(\d[\d,]*)\s*(?:evaluated\s+)?cells?", re.IGNORECASE) + + for path in _prose_files(): + try: + text = path.read_text(encoding="utf-8") + except UnicodeDecodeError: + continue + + cost_allowed = allowed.get("total_cost_usd") + if cost_allowed is not None: + for match in cost_pattern.finditer(text): + if match.group(1) not in cost_allowed: + mismatches.append( + f"{path.relative_to(_REPO_ROOT)}: cost {match.group(1)} " + f"disagrees with total_cost_usd={payload['total_cost_usd']}" + ) + + runs_allowed = allowed.get("total_runs") + if runs_allowed is not None: + for match in cell_pattern.finditer(text): + # "5 repeats" and similar short numeric contexts are noise; + # only flag when the number itself is large enough to be + # the cell count (four+ digits). + literal = match.group(1) + cleaned = literal.replace(",", "") + if cleaned.isdigit() and len(cleaned) >= 4: + if literal not in runs_allowed: + mismatches.append( + f"{path.relative_to(_REPO_ROOT)}: cell count " + f"{literal} disagrees with total_runs=" + f"{payload['total_runs']}" + ) + + assert not mismatches, ( + "Transcribed number(s) in prose disagree with " + f"{_REPORTED_NUMBERS_PATH.relative_to(_REPO_ROOT)}. " + f"Regenerate the file or update the prose. Mismatches: {mismatches}" + ) diff --git a/tests/test_models.py b/tests/test_models.py new file mode 100644 index 0000000..769f8cd --- /dev/null +++ b/tests/test_models.py @@ -0,0 +1,94 @@ +""" +Registry-only unit tests. Cross-layer integrity (pricing, dispatch, prose) +is enforced in ``tests/test_model_registry_consistency.py``. +""" + +from __future__ import annotations + +import pytest + +from maestro.models import ( + MODEL_REGISTRY, + TIER_EFFICIENCY, + TIER_FRONTIER, + all_internal_ids, + get_model, + models_by_provider, + registered_models, +) + + +def test_every_entry_matches_its_key(): + """The dict's key IS the ``internal_id`` field; a mismatch would + let ``get_model(k)`` return a spec whose ``internal_id`` was `k'`, + silently unjoining every downstream reference.""" + for internal_id, spec in MODEL_REGISTRY.items(): + assert spec.internal_id == internal_id + + +def test_get_model_round_trip(): + for internal_id in all_internal_ids(): + assert get_model(internal_id).internal_id == internal_id + + +def test_get_model_unknown_id_raises_keyerror(): + with pytest.raises(KeyError): + get_model("not-a-real-model") + + +def test_get_model_error_names_known_ids(): + """The KeyError message includes the known ids so the fix path is + a one-line diff (add the id) rather than a treasure hunt.""" + with pytest.raises(KeyError) as excinfo: + get_model("nonsense-id") + msg = str(excinfo.value) + for internal_id in all_internal_ids(): + assert internal_id in msg + + +def test_registered_models_preserves_registry_order(): + """Downstream consumers rely on the registry order for + display; a set-driven variant would randomise it.""" + ordered = registered_models() + assert [spec.internal_id for spec in ordered] == list(MODEL_REGISTRY.keys()) + + +def test_every_spec_has_a_valid_tier(): + for spec in registered_models(): + assert spec.tier in {TIER_FRONTIER, TIER_EFFICIENCY} + + +def test_every_provider_has_both_tiers(): + """The paired matrix design (frontier + efficiency per provider) is + what the viz slot mapping and the results-chapter narrative depend + on, so this shape is a registry invariant, not a coincidence.""" + by_provider: dict[str, set[str]] = {} + for spec in registered_models(): + by_provider.setdefault(spec.provider_id, set()).add(spec.tier) + + for provider_id, tiers in by_provider.items(): + assert tiers == {TIER_FRONTIER, TIER_EFFICIENCY}, ( + f"provider '{provider_id}' has tiers {tiers}, expected both" + ) + + +def test_models_by_provider_filters_correctly(): + for spec in registered_models(): + results = models_by_provider(spec.provider_id) + assert spec in results + for other in results: + assert other.provider_id == spec.provider_id + + +def test_models_by_provider_unknown_returns_empty_list(): + assert models_by_provider("no-such-provider") == [] + + +def test_snapshot_date_extracted_when_present(): + haiku = get_model("claude-haiku-4-5-20251001") + assert haiku.snapshot_date == "20251001" + + +def test_snapshot_date_is_none_when_absent(): + opus = get_model("claude-opus-4-8") + assert opus.snapshot_date is None From de7635eb1e739df0792059a86e37c8212404be49 Mon Sep 17 00:00:00 2001 From: Colinho22 <48288595+Colinho22@users.noreply.github.com> Date: Mon, 14 Sep 2026 23:13:11 +0200 Subject: [PATCH 2/3] fix: address review feedback on model registry consistency check --- src/maestro/analysis/reported_numbers.py | 42 ++++--- src/maestro/models.py | 20 ++-- src/maestro/schemas.py | 24 ++++ tests/analysis/test_reported_numbers.py | 28 ++--- tests/test_model_registry_consistency.py | 135 ++++++++++++++++++++--- tests/test_models.py | 15 +++ 6 files changed, 205 insertions(+), 59 deletions(-) diff --git a/src/maestro/analysis/reported_numbers.py b/src/maestro/analysis/reported_numbers.py index ac99a50..3e45274 100644 --- a/src/maestro/analysis/reported_numbers.py +++ b/src/maestro/analysis/reported_numbers.py @@ -15,13 +15,12 @@ from __future__ import annotations import argparse -import json import sqlite3 from pathlib import Path -from typing import Any from maestro.db.client import get_readonly_connection from maestro.experiment_config import DB_PATH +from maestro.schemas import ReportedNumbers # Emitted schema version. Bump when a field is renamed or its meaning # changes; consumers (docs, the consistency check) can then pin against a @@ -38,7 +37,7 @@ ) -def compute_reported_numbers(conn: sqlite3.Connection) -> dict[str, Any]: +def compute_reported_numbers(conn: sqlite3.Connection) -> ReportedNumbers: """ Aggregate the docs-referenced totals from a results DB, read-only. @@ -63,31 +62,30 @@ def compute_reported_numbers(conn: sqlite3.Connection) -> dict[str, Any]: total_runs = int(totals_row["total_runs"]) if total_runs == 0: - return { - "schema_version": SCHEMA_VERSION, - "status": "empty", - "total_runs": 0, - "successes": 0, - "failures": 0, - "total_cost_usd": 0.0, - } - - return { - "schema_version": SCHEMA_VERSION, - "status": "ok", - "total_runs": total_runs, - "successes": int(totals_row["successes"]), - "failures": int(totals_row["failures"]), + return ReportedNumbers( + schema_version=SCHEMA_VERSION, + status="empty", + total_runs=0, + successes=0, + failures=0, + total_cost_usd=0.0, + ) + + return ReportedNumbers( + schema_version=SCHEMA_VERSION, + status="ok", + total_runs=total_runs, + successes=int(totals_row["successes"]), + failures=int(totals_row["failures"]), # Rounded to cents: docs quote e.g. USD 171.62, so writing extra # digits would create a mismatch on the last decimal that is # cosmetic, not real. Two-place rounding matches the reporting # convention and keeps the diff meaningful. - "total_cost_usd": round(float(totals_row["total_cost_usd"]), 2), - } + total_cost_usd=round(float(totals_row["total_cost_usd"]), 2), + ) def _parse_args(argv: list[str] | None = None) -> argparse.Namespace: - """Parse the CLI (``--db``, ``--out``).""" parser = argparse.ArgumentParser( prog="python -m maestro.analysis.reported_numbers", description=( @@ -129,7 +127,7 @@ def main(argv: list[str] | None = None) -> int: args.out.parent.mkdir(parents=True, exist_ok=True) args.out.write_text( - json.dumps(payload, indent=2, sort_keys=False) + "\n", + payload.model_dump_json(indent=2) + "\n", encoding="utf-8", ) print(f"Wrote {args.out}") diff --git a/src/maestro/models.py b/src/maestro/models.py index 6b0a673..aa7b334 100644 --- a/src/maestro/models.py +++ b/src/maestro/models.py @@ -26,17 +26,23 @@ TIER_FRONTIER = "frontier" TIER_EFFICIENCY = "efficiency" -# Regex over an internal id's terminal segment: 8 consecutive digits count as -# a snapshot date (e.g. the 20251001 in claude-haiku-4-5-20251001), 4 do not -# (a version number). Loose match is deliberate: a vendor's convention is -# not something the registry can normalise, only expose. -_SNAPSHOT_DATE_PATTERN = re.compile(r"-(\d{8})$") +# Regex over an internal id's terminal segment. Two vendor shapes are +# accepted: 8 consecutive digits (``20251001``) and the hyphenated form +# ``YYYY-MM-DD`` (``2026-04-23``). The captured group is normalised to the +# compact 8-digit form on return so downstream comparators do not need to +# know which shape the vendor picked. 4 consecutive digits do not count +# (that is a version number, e.g. ``mistral-small-2603``). Loose match +# is deliberate: a vendor's convention is not something the registry can +# normalise beyond this compact-form step, only expose. +_SNAPSHOT_DATE_PATTERN = re.compile(r"-(\d{4}-\d{2}-\d{2}|\d{8})$") def _snapshot_of(internal_id: str) -> str | None: - """Extract an 8-digit snapshot date from the tail of an internal id.""" + """Extract a snapshot date from the tail of an internal id, compact form.""" match = _SNAPSHOT_DATE_PATTERN.search(internal_id) - return match.group(1) if match else None + if match is None: + return None + return match.group(1).replace("-", "") def _spec( diff --git a/src/maestro/schemas.py b/src/maestro/schemas.py index 4ac2d4e..6188c12 100644 --- a/src/maestro/schemas.py +++ b/src/maestro/schemas.py @@ -186,6 +186,30 @@ class ModelSpec(BaseModel): snapshot_date: str | None = None +# --------------------------------------------------------------------------- +# ReportedNumbers: docs-referenced headline totals dumped from the DB +# --------------------------------------------------------------------------- + + +class ReportedNumbers(BaseModel): + """ + Machine-readable join point between the results database and any prose + (README, CHANGELOG, docs markdown) that quotes a headline total. The + producer (``maestro.analysis.reported_numbers``) writes an instance of + this model to JSON; the consistency test reads the same file back to + diff against tracked prose. Keeping it as a Pydantic model, rather + than a free dict, means the field set cannot silently drift between + the producer and the consumer. + """ + + schema_version: str + status: str + total_runs: int + successes: int + failures: int + total_cost_usd: float + + # --------------------------------------------------------------------------- # ModelPricing: lookup table for cost calculation # --------------------------------------------------------------------------- diff --git a/tests/analysis/test_reported_numbers.py b/tests/analysis/test_reported_numbers.py index 831d697..fe794d4 100644 --- a/tests/analysis/test_reported_numbers.py +++ b/tests/analysis/test_reported_numbers.py @@ -69,12 +69,12 @@ def test_empty_db_returns_zeroed_payload(db_path: Path) -> None: with get_connection(db_path) as conn: payload = compute_reported_numbers(conn) - assert payload["schema_version"] == SCHEMA_VERSION - assert payload["status"] == "empty" - assert payload["total_runs"] == 0 - assert payload["successes"] == 0 - assert payload["failures"] == 0 - assert payload["total_cost_usd"] == 0.0 + assert payload.schema_version == SCHEMA_VERSION + assert payload.status == "empty" + assert payload.total_runs == 0 + assert payload.successes == 0 + assert payload.failures == 0 + assert payload.total_cost_usd == 0.0 def test_populated_db_reports_split_and_cost(db_path: Path) -> None: @@ -107,12 +107,12 @@ def test_populated_db_reports_split_and_cost(db_path: Path) -> None: with get_connection(db_path) as conn: payload = compute_reported_numbers(conn) - assert payload["status"] == "ok" - assert payload["total_runs"] == 3 - assert payload["successes"] == 2 - assert payload["failures"] == 1 + assert payload.status == "ok" + assert payload.total_runs == 3 + assert payload.successes == 2 + assert payload.failures == 1 # Cost is rounded to two decimals; matches how docs quote it. - assert payload["total_cost_usd"] == 0.03 + assert payload.total_cost_usd == 0.03 def test_empty_output_counts_as_failure(db_path: Path) -> None: @@ -131,6 +131,6 @@ def test_empty_output_counts_as_failure(db_path: Path) -> None: with get_connection(db_path) as conn: payload = compute_reported_numbers(conn) - assert payload["total_runs"] == 1 - assert payload["successes"] == 0 - assert payload["failures"] == 1 + assert payload.total_runs == 1 + assert payload.successes == 0 + assert payload.failures == 1 diff --git a/tests/test_model_registry_consistency.py b/tests/test_model_registry_consistency.py index 1afd03e..0890d87 100644 --- a/tests/test_model_registry_consistency.py +++ b/tests/test_model_registry_consistency.py @@ -77,10 +77,14 @@ r"\b(" + "|".join(_PROVIDER_NEEDLES) + r")-[a-z0-9][a-z0-9.\-]*" ) -# Substrings whose match is deliberately not a target model id. Kept +# Model-family literals accepted only in prose (docstrings, comments, .md +# files, and Python string literals). Active model ids in production +# Python code must still resolve through the registry: the split exists +# so a family name like ``gpt-5.5`` cannot shadow an unregistered active +# alias if one ever appears as a bare identifier or dict key. Kept # narrow on purpose: broadening it here would defeat the point of the -# scan. Each entry is a literal (lowercase, matches the pattern above). -_LITERAL_ALLOWLIST: frozenset[str] = frozenset( +# scan. +_PROSE_ALLOWLIST: frozenset[str] = frozenset( { # Generic version families referenced in provider docstrings. # Not registered because they are examples of a lineage, not the @@ -98,7 +102,8 @@ # Test fixture used by legacy viz tests: a synthetic id that # exercises the display path without needing a registry entry. # Migration to a registered id is tracked separately from the - # registry rollout. + # registry rollout. It only ever appears as a string literal in + # test files, so the prose-context heuristic covers it. "gpt-4o-mini-2024-07-18", # Design-guide palette label (docs/visualization_design_guide.md). "claude-coral", @@ -134,12 +139,93 @@ def _tracked_files() -> list[Path]: return files -def _find_unregistered_literals(text: str) -> set[str]: +def _is_python_prose_context(text: str, match_start: int) -> bool: + """ + Heuristic: is the byte at ``match_start`` inside a Python comment, a + triple-quoted docstring, or a single/double-quoted string literal? + + Walks backward from ``match_start`` on the current line to catch + ``#``-style comments, and scans from the start of the file to count + unclosed quote runs so a match inside any string literal (including + triple-quoted docstrings) is flagged as prose. Approximate on + purpose: the goal is to distinguish "the match sits in prose or a + string literal" from "the match is a bare identifier or a dict key", + not to be a full Python tokenizer. Errs toward permissive on the + prose side; the CamelCase / registered-id checks upstream keep the + scan honest. + """ + # 1. Same-line ``#`` comment: any unquoted # earlier on the line means + # the match is inside a comment. + line_start = text.rfind("\n", 0, match_start) + 1 + line_prefix = text[line_start:match_start] + in_string = False + quote: str | None = None + i = 0 + while i < len(line_prefix): + ch = line_prefix[i] + if in_string: + if ch == "\\": + i += 2 + continue + if ch == quote: + in_string = False + quote = None + else: + if ch == "#": + return True + if ch in ('"', "'"): + in_string = True + quote = ch + i += 1 + + # 2. String / docstring context across the whole file up to the match. + # Count triple-quote and single-quote runs to see if the match sits + # inside an open string. Handles escaped quotes inside strings. + prefix = text[:match_start] + in_string = False + quote_seq: str | None = None + i = 0 + while i < len(prefix): + ch = prefix[i] + if in_string: + if ch == "\\": + i += 2 + continue + # Triple-quoted string only closes on the matching triple. + if quote_seq is not None and len(quote_seq) == 3: + if prefix[i : i + 3] == quote_seq: + in_string = False + quote_seq = None + i += 3 + continue + elif ch == quote_seq: + in_string = False + quote_seq = None + else: + if ch in ('"', "'"): + # Triple-quote takes precedence over single-quote. + if prefix[i : i + 3] == ch * 3: + in_string = True + quote_seq = ch * 3 + i += 3 + continue + in_string = True + quote_seq = ch + i += 1 + return in_string + + +def _find_unregistered_literals(text: str, *, is_python: bool) -> set[str]: """ Model-shaped literals in ``text`` that are neither in the registry - nor on the allowlist. Match is case-sensitive lowercase (the pattern - itself enforces it), which lets ``MistralProvider`` and other + nor on the prose allowlist. Match is case-sensitive lowercase (the + pattern itself enforces it), which lets ``MistralProvider`` and other CamelCase references pass without an explicit exemption. + + In Python files the prose allowlist only fires when the match sits in + a comment, docstring, or string literal, so a family-name literal + cannot shadow an unregistered active id used as a bare identifier or + dict key. In markdown, allowlist matches unconditionally. """ unregistered: set[str] = set() for match in _MODEL_LITERAL_PATTERN.finditer(text): @@ -150,8 +236,9 @@ def _find_unregistered_literals(text: str) -> set[str]: candidate = raw.rstrip(".,:;)\"'`") if candidate in MODEL_REGISTRY: continue - if candidate in _LITERAL_ALLOWLIST: - continue + if candidate in _PROSE_ALLOWLIST: + if not is_python or _is_python_prose_context(text, match.start()): + continue unregistered.add(candidate) return unregistered @@ -238,14 +325,15 @@ def test_no_unregistered_model_literals_in_tracked_files(): text = path.read_text(encoding="utf-8") except UnicodeDecodeError: continue - bad = _find_unregistered_literals(text) + is_python = path.suffix == ".py" + bad = _find_unregistered_literals(text, is_python=is_python) if bad: offenders[str(path.relative_to(_REPO_ROOT))] = bad assert not offenders, ( "Unregistered model-shaped literals found. Add them to " "maestro.models.MODEL_REGISTRY or, if they are not model ids, " - "extend _LITERAL_ALLOWLIST with a one-line justification. " + "extend _PROSE_ALLOWLIST with a one-line justification. " f"Offenders: {offenders}" ) @@ -288,6 +376,11 @@ def test_docs_totals_match_reported_numbers_file(): If a reported-numbers dump exists, docs must not contradict it. On a fresh checkout (no DB, no dump) this self-skips: the check exists to catch drift, not to require the DB be populated before merge. + + Only literals explicitly qualified as totals ("total N runs", + "all N cells", "N total runs") are diffed against ``total_runs``. + Qualified subsets (evaluated, successful, failed) live in richer + artefacts and are out of scope for this drift check. """ payload = _load_reported_numbers() if payload is None: @@ -322,7 +415,16 @@ def test_docs_totals_match_reported_numbers_file(): # this can catch reworded prose that keeps the numbers. mismatches: list[str] = [] cost_pattern = re.compile(r"USD\s+(\d+(?:,\d{3})*(?:\.\d+)?)") - cell_pattern = re.compile(r"(\d[\d,]*)\s*(?:evaluated\s+)?cells?", re.IGNORECASE) + # Match only literals explicitly labelled as a grand total. Two + # accepted shapes: a "total"/"all" qualifier immediately before the + # number ("total 4230 cells"), or "total run(s)"/"total cell(s)" + # immediately after ("4230 total cells"). Qualified subsets + # ("evaluated", "successful", "failed") are deliberately not matched. + cell_pattern = re.compile( + r"(?:total|all)\s+(?:of\s+)?(\d[\d,]*)\s*(?:runs?|cells?)\b" + r"|(\d[\d,]*)\s*total\s+(?:runs?|cells?)\b", + re.IGNORECASE, + ) for path in _prose_files(): try: @@ -342,10 +444,11 @@ def test_docs_totals_match_reported_numbers_file(): runs_allowed = allowed.get("total_runs") if runs_allowed is not None: for match in cell_pattern.finditer(text): - # "5 repeats" and similar short numeric contexts are noise; - # only flag when the number itself is large enough to be - # the cell count (four+ digits). - literal = match.group(1) + # Two alternatives in the pattern; exactly one group + # captures per match. "5 repeats" and similar short + # numeric contexts are noise; only flag when the number + # itself is large enough to be the cell count (4+ digits). + literal = next(g for g in match.groups() if g) cleaned = literal.replace(",", "") if cleaned.isdigit() and len(cleaned) >= 4: if literal not in runs_allowed: diff --git a/tests/test_models.py b/tests/test_models.py index 769f8cd..444ff4b 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -92,3 +92,18 @@ def test_snapshot_date_extracted_when_present(): def test_snapshot_date_is_none_when_absent(): opus = get_model("claude-opus-4-8") assert opus.snapshot_date is None + + +def test_snapshot_date_hyphenated_form_is_normalised_to_compact(): + """Vendor ids like ``gpt-5.5-2026-04-23`` embed the date with hyphens; + the registry stores the compact 8-digit form so downstream comparators + do not need to know which shape the vendor picked.""" + gpt = get_model("gpt-5.5-2026-04-23") + assert gpt.snapshot_date == "20260423" + + +def test_snapshot_date_four_digit_tail_is_not_a_date(): + """A 4-digit tail is a version number, not a date; leaving it as a + date would let a version bump masquerade as a snapshot roll.""" + small = get_model("mistral-small-2603") + assert small.snapshot_date is None From 99444bd38113d9bab18a6e8dab0c36d3a1c5ac90 Mon Sep 17 00:00:00 2001 From: Colinho22 <48288595+Colinho22@users.noreply.github.com> Date: Tue, 15 Sep 2026 00:09:33 +0200 Subject: [PATCH 3/3] fix: tighten registry/pricing invariants and freeze ModelSpec - Assert provider dispatch class._PROVIDER_NAME == spec.provider_id, so a rename on one side without the other is caught in CI. - Validate reported_numbers.json via ReportedNumbers.model_validate_json on the read side to catch schema drift the same way write-side type safety does. - Replace one-directional pricing coverage check with a bidirectional set-equality (plus duplicate) check between non-control pricing rows and the registry. - Freeze ModelSpec via ConfigDict(frozen=True): registry entries are semantically constants; freezing guards against accidental in-place mutation. Nothing in the codebase mutates a ModelSpec today. --- src/maestro/schemas.py | 4 +- tests/test_model_registry_consistency.py | 54 +++++++++++++++++------- 2 files changed, 42 insertions(+), 16 deletions(-) diff --git a/src/maestro/schemas.py b/src/maestro/schemas.py index 6188c12..600b167 100644 --- a/src/maestro/schemas.py +++ b/src/maestro/schemas.py @@ -10,7 +10,7 @@ from pathlib import Path from uuid import UUID, uuid4 -from pydantic import BaseModel, Field, computed_field +from pydantic import BaseModel, ConfigDict, Field, computed_field # --------------------------------------------------------------------------- # Enums: constrain experiment dimensions to valid values @@ -160,6 +160,8 @@ class ModelSpec(BaseModel): concern. """ + model_config = ConfigDict(frozen=True) + # Stored verbatim in ``RunConfig.model`` and ``ModelPricing.model``, # so this string IS the join key. Never rename an id once data has # been collected under it: rows would silently unjoin. diff --git a/tests/test_model_registry_consistency.py b/tests/test_model_registry_consistency.py index 0890d87..00f15e2 100644 --- a/tests/test_model_registry_consistency.py +++ b/tests/test_model_registry_consistency.py @@ -22,7 +22,6 @@ from __future__ import annotations -import json import re from pathlib import Path @@ -30,6 +29,7 @@ from maestro.experiment_config import CONTROL_MODEL, MODELS from maestro.models import MODEL_REGISTRY, all_internal_ids, get_model +from maestro.schemas import ReportedNumbers _REPO_ROOT = Path(__file__).resolve().parents[1] _SRC_ROOT = _REPO_ROOT / "src" / "maestro" @@ -273,24 +273,38 @@ def test_get_model_raises_keyerror_with_message_on_unknown(): # --------------------------------------------------------------------------- -def test_every_pricing_entry_is_registered(): +def test_pricing_and_registry_are_one_to_one(): """ - Every real ``ModelPricing`` row (all except the synthetic ``control`` - model) has a matching registry entry, so cost calculations and - scoring cannot resolve a row that no other layer recognises. + Every non-control registry model has exactly one pricing entry, and + every pricing entry has a matching registry row. CONTROL_MODEL is the + single intentional exception (synthetic 'control' pricing row for the + zero-cost control strategies). Without set-equality, a registry model + could ship without pricing (crash at cost calculation) or a pricing + entry could linger under a stale name (silent zero cost). """ - for mp in MODELS: - if mp.model == CONTROL_MODEL.model: - continue - assert mp.model in MODEL_REGISTRY, ( - f"pricing entry '{mp.model}' has no matching registry row" - ) + pricing_ids = [mp.model for mp in MODELS if mp.model != CONTROL_MODEL.model] + registry_ids = set(MODEL_REGISTRY) + + assert len(pricing_ids) == len(set(pricing_ids)), ( + f"duplicate pricing entries: " + f"{sorted({m for m in pricing_ids if pricing_ids.count(m) > 1})}" + ) + assert set(pricing_ids) == registry_ids, ( + "pricing and registry are out of sync. " + f"In pricing but not registry: {set(pricing_ids) - registry_ids}. " + f"In registry but not pricing: {registry_ids - set(pricing_ids)}." + ) def test_every_provider_dispatch_target_resolves_through_registry(): """ Each provider needle in ``run.py:_PROVIDER_DISPATCH`` claims at least - one registered model (so a needle without a live user gets caught). + one registered model (so a needle without a live user gets caught), + and the dispatched provider class's ``_PROVIDER_NAME`` matches the + registry's ``spec.provider_id`` for that model. That second assertion + pins the identity that ``models_by_provider`` and every provider-keyed + join rely on: a rename on one side without the other would silently + unjoin. """ from maestro.run import _PROVIDER_DISPATCH, _dispatch_for_model @@ -301,8 +315,13 @@ def test_every_provider_dispatch_target_resolves_through_registry(): assert dispatch is not None, ( f"registered model '{spec.internal_id}' does not dispatch to a provider" ) - needle, _, _ = dispatch + needle, provider_cls, _ = dispatch covered.add(needle) + assert provider_cls._PROVIDER_NAME == spec.provider_id, ( + f"registered model '{spec.internal_id}' has provider_id=" + f"'{spec.provider_id}' but dispatches to {provider_cls.__name__} " + f"with _PROVIDER_NAME='{provider_cls._PROVIDER_NAME}'" + ) assert needles == covered, ( f"provider dispatch needles have no registered users: {needles - covered}" ) @@ -356,10 +375,15 @@ def test_no_unregistered_model_literals_in_tracked_files(): def _load_reported_numbers() -> dict | None: - """Return the JSON payload if present, else ``None`` (skip signal).""" + """ + Return the JSON payload as a plain dict if present, else ``None`` + (skip signal). Parsing goes through ``ReportedNumbers.model_validate_json`` + so a schema drift on read is caught here too, not only at write time. + """ if not _REPORTED_NUMBERS_PATH.exists(): return None - return json.loads(_REPORTED_NUMBERS_PATH.read_text(encoding="utf-8")) + raw = _REPORTED_NUMBERS_PATH.read_text(encoding="utf-8") + return ReportedNumbers.model_validate_json(raw).model_dump() def _prose_files() -> list[Path]: