diff --git a/src/valcore/api/main.py b/src/valcore/api/main.py index 8cc0437..a98ae16 100644 --- a/src/valcore/api/main.py +++ b/src/valcore/api/main.py @@ -3,6 +3,7 @@ import importlib from importlib.resources import files as _package_files from pathlib import Path +from typing import Any from fastapi import FastAPI, Request from fastapi.middleware.cors import CORSMiddleware @@ -24,7 +25,7 @@ ValcoreError, ) from valcore.models import VALID_CAPABILITIES -from valcore.settings import MODEL_CATALOG +from valcore.settings import get_settings, model_catalog from valcore.tools import tool_names from valcore.tracing import configure_tracing, instrument_app @@ -159,10 +160,17 @@ async def health() -> dict[str, str]: return {"status": "ok"} @app.get("/api/config") - async def config() -> dict[str, list[str]]: - """Return the pickers the SPA needs: models, tools, and capabilities.""" + async def config() -> dict[str, Any]: + """Return the pickers the SPA needs: models, tools, and capabilities. + + ``models`` is a suggestion list for type-ahead, not a closed set -- the SPA + accepts any well-formed ``gateway/:`` string. ``default_model`` + is sent separately because the catalog is sorted alphabetically, so its first + entry is arbitrary and not a sensible default. + """ return { - "models": list(MODEL_CATALOG), + "models": list(model_catalog()), + "default_model": get_settings().default_model, "tools": tool_names(), "capabilities": sorted(VALID_CAPABILITIES), } diff --git a/src/valcore/settings.py b/src/valcore/settings.py index c6462e7..d6e09b3 100644 --- a/src/valcore/settings.py +++ b/src/valcore/settings.py @@ -29,13 +29,25 @@ "gateway/groq", ) -MODEL_CATALOG: list[str] = [ - "gateway/anthropic:claude-sonnet-5", - "gateway/anthropic:claude-opus-4-5", - "gateway/anthropic:claude-haiku-4-5", - "gateway/openai:gpt-5", - "gateway/google:gemini-2.5-pro", -] + +@functools.lru_cache +def model_catalog() -> tuple[str, ...]: + """Return the gateway-routable model names known to the pinned ``pydantic-ai``. + + These are type-ahead suggestions, not a whitelist -- ``validate_model_string`` + accepts any well-formed ``gateway/:`` string and leaves it to the + Gateway to reject names it does not serve. Filtering through ``GATEWAY_ROUTES`` + (rather than a bare ``gateway/`` prefix) keeps the two in step: every suggestion + is a string ``validate_model_string`` accepts. + + ``pydantic_ai.models`` is imported here rather than at module scope because it + costs ~235ms to import, and every CLI invocation loads this module while only the + API's config endpoint needs the catalog. + """ + from pydantic_ai.models import known_model_names + + prefixes = tuple(f"{route}:" for route in GATEWAY_ROUTES) + return tuple(sorted(n for n in known_model_names() if n.startswith(prefixes))) class _TomlConfigSource(PydanticBaseSettingsSource): diff --git a/tests/test_api_skeleton.py b/tests/test_api_skeleton.py index 12f9c03..b37cfb4 100644 --- a/tests/test_api_skeleton.py +++ b/tests/test_api_skeleton.py @@ -17,6 +17,7 @@ ReferencedError, ValcoreError, ) +from valcore.settings import get_settings @pytest.fixture @@ -58,6 +59,23 @@ async def test_config_populated() -> None: assert payload["capabilities"] +@pytest.mark.anyio +async def test_config_default_model_is_not_merely_the_head_of_the_catalog() -> None: + """The SPA seeds new versions from ``default_model``, never ``models[0]``. + + The catalog is sorted alphabetically, so its first entry is an arbitrary Bedrock + model. This pins the two apart so a regression to ``models[0]`` is caught here + rather than by a user wondering why their evaluator picked a model they never chose. + """ + app = create_app() + async with _client(app) as client: + payload = (await client.get("/api/config")).json() + + assert payload["default_model"] == get_settings().default_model + assert payload["default_model"] in payload["models"] + assert payload["default_model"] != payload["models"][0] + + @pytest.mark.anyio @pytest.mark.parametrize( ("error", "status", "type_name"), diff --git a/tests/test_config.py b/tests/test_config.py index 0c51e86..538bf27 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -362,3 +362,51 @@ def test_logfire_extra_is_present_in_dev_environment() -> None: failing loudly here. """ assert importlib.util.find_spec("logfire") is not None + + +# --- model catalog ----------------------------------------------------------------- + + +def test_model_catalog_is_derived_and_covers_every_gateway_route() -> None: + """The catalog comes from pydantic-ai, not a hand-maintained literal. + + The old five-entry list went stale silently. Asserting a floor well above it, + plus coverage of every route, means a future pydantic-ai bump that drops a + provider fails here instead of quietly shrinking the picker. + """ + catalog = settings.model_catalog() + + assert len(catalog) > 100 + routes_seen = {name.split(":", 1)[0] for name in catalog} + assert routes_seen == set(settings.GATEWAY_ROUTES) + + +def test_every_catalog_entry_passes_model_validation() -> None: + """The catalog and the validator cannot drift apart. + + `model_catalog` filters through GATEWAY_ROUTES precisely so that suggesting a + model the validator would then reject is impossible. + """ + for name in settings.model_catalog(): + settings.validate_model_string(name) + + +def test_default_model_is_in_the_catalog() -> None: + assert settings.Settings().default_model in settings.model_catalog() + + +def test_validate_model_string_accepts_names_absent_from_the_catalog() -> None: + """Shape is the only gate; the Gateway is the authority on what exists. + + The Gateway serves more models than any pinned pydantic-ai knows about, so an + unrecognised-but-well-formed name must pass rather than block the user. + """ + unknown = "gateway/anthropic:some-model-released-tomorrow" + assert unknown not in settings.model_catalog() + settings.validate_model_string(unknown) # must not raise + + +def test_validate_model_string_still_rejects_malformed_names() -> None: + for bad in ("claude-sonnet-5", "gateway/nope:x", "gateway/anthropic:"): + with pytest.raises(ConfigError): + settings.validate_model_string(bad) diff --git a/web/src/components/VersionEditor.test.tsx b/web/src/components/VersionEditor.test.tsx index a3d0923..3b3625e 100644 --- a/web/src/components/VersionEditor.test.tsx +++ b/web/src/components/VersionEditor.test.tsx @@ -22,6 +22,9 @@ vi.mock("../api/client", () => ({ const config: AppConfig = { models: ["gateway/anthropic:claude-sonnet-5", "gateway/openai:gpt-5"], + // Deliberately not `models[0]`, so the defaulting assertions below would fail if the + // editor went back to seeding new versions from the head of the catalog. + default_model: "gateway/openai:gpt-5", tools: ["row_get"], capabilities: ["FileSystem", "Shell"], }; @@ -186,11 +189,89 @@ describe("VersionEditor: draft mode", () => { expect((screen.getByLabelText("Version name") as HTMLInputElement).value).toBe(""); expect((screen.getByLabelText("Instructions") as HTMLTextAreaElement).value).toBe(""); expect((screen.getByLabelText("Prompt template") as HTMLTextAreaElement).value).toBe(""); - expect((screen.getByLabelText("Model") as HTMLSelectElement).value).toBe(config.models[0]); + expect((screen.getByLabelText("Model") as HTMLInputElement).value).toBe(config.default_model); expect(screen.getByRole("button", { name: "Create version" })).not.toBeNull(); expect(screen.queryByRole("button", { name: "Save changes" })).toBeNull(); }); + it("prefills every field from seedFrom, so a new version starts as an edit", async () => { + const source = makeVersion({ + id: "v1", + version_name: "tone-check", + notes: "the prior notes", + model: "gateway/openai:gpt-5", + instructions: "Judge the tone.", + prompt_template: "Rate {answer}", + required_columns: ["answer"], + tools: ["row_get"], + }); + render( + , + ); + + expect((screen.getByLabelText("Version name") as HTMLInputElement).value).toBe("tone-check"); + expect((screen.getByLabelText("Instructions") as HTMLTextAreaElement).value).toBe( + "Judge the tone.", + ); + expect((screen.getByLabelText("Prompt template") as HTMLTextAreaElement).value).toBe( + "Rate {answer}", + ); + expect((screen.getByLabelText("Model") as HTMLInputElement).value).toBe( + "gateway/openai:gpt-5", + ); + + // Seeded, not adopted: this is still a create, so it saves via createVersion. + expect(screen.getByRole("button", { name: "Create version" })).not.toBeNull(); + }); + + it("seeds a copy, so editing the draft cannot mutate the source version", async () => { + const source = makeVersion({ required_columns: ["answer"], tools: ["row_get"] }); + const before = JSON.stringify(source); + const user = userEvent.setup(); + render( + , + ); + + await user.clear(screen.getByLabelText("Instructions")); + await user.type(screen.getByLabelText("Instructions"), "Different."); + + expect(JSON.stringify(source)).toBe(before); + }); + + it("falls back to a blank form when there is no seed", () => { + render(); + + expect((screen.getByLabelText("Version name") as HTMLInputElement).value).toBe(""); + expect((screen.getByLabelText("Instructions") as HTMLTextAreaElement).value).toBe(""); + }); + + it("accepts a model the catalog does not list", async () => { + // The Gateway serves more models than the pinned pydantic-ai knows about, so the + // field must take a well-formed name that is absent from the suggestions. + const user = userEvent.setup(); + render(); + + const field = screen.getByLabelText("Model") as HTMLInputElement; + const unlisted = "gateway/groq:llama-4-maverick"; + expect(config.models).not.toContain(unlisted); + + await user.clear(field); + await user.type(field, unlisted); + + expect(field.value).toBe(unlisted); + }); + + it("offers the catalog as suggestions without constraining the field", () => { + render(); + + const field = screen.getByLabelText("Model") as HTMLInputElement; + expect(field.tagName).toBe("INPUT"); + expect(field.getAttribute("list")).toBeTruthy(); + + const options = document.querySelectorAll(`#${field.getAttribute("list")} option`); + expect([...options].map((o) => o.getAttribute("value"))).toEqual(config.models); + }); + it("disables Save and shows an inline error for an incomplete draft", () => { render(); @@ -307,7 +388,10 @@ describe("VersionEditor: sectioned layout", () => { expect(screen.getByText(/read-only/i)).not.toBeNull(); expect((screen.getByLabelText("Version name") as HTMLInputElement).readOnly).toBe(true); expect((screen.getByLabelText("Instructions") as HTMLTextAreaElement).readOnly).toBe(true); - expect((screen.getByLabelText("Model") as HTMLSelectElement).disabled).toBe(true); + // `readOnly`, not `disabled`: the old control was a `: the +// Gateway serves far more models than any list we ship, so the catalog is type-ahead only and +// an unrecognised-but-well-formed name must still be enterable. `MODEL_TEMPLATE` seeds an empty +// field on focus so the suggestion list opens on the route prefix instead of all ~160 entries. +const MODEL_OPTIONS_ID = "model-catalog-options"; +const MODEL_TEMPLATE = "gateway/"; + // A field label paired with its optional info affordance. The tooltip trigger is a // `type="button"`, so it never carries `aria-expanded` and stays distinct from the one // collapsible section's disclosure. A ` @@ -58,7 +62,7 @@ vi.mock("../api/client", async () => { }; }); -const config = { models: ["model-a"], tools: [], capabilities: [] }; +const config = { models: ["model-a"], default_model: "model-a", tools: [], capabilities: [] }; function makeVersion(overrides: Partial = {}): EvaluatorVersion { return { @@ -163,6 +167,38 @@ describe("EvaluatorDetail: draft editor", () => { expect(screen.getByRole("button", { name: "Run" })).toBeDisabled(); }); + it("seeds a new draft from the version currently on screen", async () => { + vi.mocked(api).mockResolvedValue(config); + vi.mocked(evaluators.get).mockResolvedValue( + makeDetail({ + active_version_id: "v1", + versions: [makeVersion({ id: "v1" }), makeVersion({ id: "v2", version_name: "v2" })], + }), + ); + const user = userEvent.setup(); + renderDetail(); + + expect(await screen.findByText("Editing version v1")).toBeTruthy(); + + // Switch versions first, so this pins "the one on screen" rather than "the first one". + await user.selectOptions(screen.getByRole("combobox", { name: "Version" }), "v2"); + await user.click(screen.getByRole("button", { name: "New version" })); + + expect(screen.getByText("Draft editor")).toBeTruthy(); + expect(screen.getByText("editor-seed: v2")).toBeTruthy(); + }); + + it("seeds nothing when the evaluator has no versions yet", async () => { + vi.mocked(api).mockResolvedValue(config); + vi.mocked(evaluators.get).mockResolvedValue( + makeDetail({ active_version_id: null, versions: [] }), + ); + renderDetail(); + + expect(await screen.findByText("Draft editor")).toBeTruthy(); + expect(screen.getByText("editor-seed: none")).toBeTruthy(); + }); + it("selects the newly created version after a draft is saved", async () => { vi.mocked(api).mockResolvedValue(config); vi.mocked(evaluators.get) diff --git a/web/src/pages/EvaluatorDetail.tsx b/web/src/pages/EvaluatorDetail.tsx index ed1f720..14bc0d4 100644 --- a/web/src/pages/EvaluatorDetail.tsx +++ b/web/src/pages/EvaluatorDetail.tsx @@ -286,6 +286,7 @@ export default function EvaluatorDetail({ id }: EvaluatorDetailProps) { = {}): UseSetupResul }; } -const config = { models: ["model-a", "model-b"], tools: [], capabilities: [] }; +const config = { + models: ["model-a", "model-b"], + default_model: "model-b", + tools: [], + capabilities: [], +}; function makeEvaluator(overrides: Partial = {}): Evaluator { return { diff --git a/web/src/pages/EvaluatorsPage.tsx b/web/src/pages/EvaluatorsPage.tsx index fd64c30..7b30d21 100644 --- a/web/src/pages/EvaluatorsPage.tsx +++ b/web/src/pages/EvaluatorsPage.tsx @@ -102,7 +102,7 @@ function EvaluatorsList() { } const draft = await evaluators.generate({ criteria }); const evaluator = await evaluators.create({ name }); - const model = config.models[0] ?? ""; + const model = config.default_model; await evaluators.createVersion(evaluator.id, draftToVersion(draft, model)); navigate(`/evaluators/${evaluator.id}`); }