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..3e45274 --- /dev/null +++ b/src/maestro/analysis/reported_numbers.py @@ -0,0 +1,138 @@ +""" +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 sqlite3 +from pathlib import Path + +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 +# 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) -> ReportedNumbers: + """ + 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 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), + ) + + +def _parse_args(argv: list[str] | None = None) -> argparse.Namespace: + 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( + payload.model_dump_json(indent=2) + "\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..aa7b334 --- /dev/null +++ b/src/maestro/models.py @@ -0,0 +1,194 @@ +""" +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. 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 a snapshot date from the tail of an internal id, compact form.""" + match = _SNAPSHOT_DATE_PATTERN.search(internal_id) + if match is None: + return None + return match.group(1).replace("-", "") + + +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..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 @@ -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,78 @@ 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. + """ + + 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. + 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 + + +# --------------------------------------------------------------------------- +# 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/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..fe794d4 --- /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..00f15e2 --- /dev/null +++ b/tests/test_model_registry_consistency.py @@ -0,0 +1,489 @@ +""" +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 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 +from maestro.schemas import ReportedNumbers + +_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.\-]*" +) + +# 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. +_PROSE_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. 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", + # 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 _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 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): + 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 _PROSE_ALLOWLIST: + if not is_python or _is_python_prose_context(text, match.start()): + 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_pricing_and_registry_are_one_to_one(): + """ + 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). + """ + 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), + 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 + + 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, 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}" + ) + + +# --------------------------------------------------------------------------- +# 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 + 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 _PROSE_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 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 + raw = _REPORTED_NUMBERS_PATH.read_text(encoding="utf-8") + return ReportedNumbers.model_validate_json(raw).model_dump() + + +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. + + 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: + 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+)?)") + # 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: + 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): + # 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: + 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..444ff4b --- /dev/null +++ b/tests/test_models.py @@ -0,0 +1,109 @@ +""" +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 + + +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