From 6f016dccb867f79fe93083b7f15987356e3470bb Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Thu, 10 Sep 2026 17:45:41 +0000 Subject: [PATCH 1/2] Implement spec 003: the answering model comes from config.yml Stages 1 and 2 of the plan. The model is an optional `llm:` section in config.yml; how it must be CALLED stays derived in code. Precedence is LLM_MODEL, then config.yml, then the built-in default -- the reverse of util/secrets.py, where a mounted Docker secret beats the environment. Both are the more specific source winning, seen from different sides, and resolve_llm_model says so, because the inconsistency looks like a bug until you see which way each points. A configured temperature the model is known to refuse now stops startup naming the model, the value and the fix. Without it the mistake is a 400 on a user's first question -- visible to a user, attributed to the chatbot, and diagnosable only from logs. A model the table has not met is deliberately not validated: the table is empirical and always behind, so an unknown model must not block startup. Writing the tests found a real defect. Pydantic ignores unknown keys by default, so `embedding_model: text-embedding-3-large` in an `llm:` section would have been accepted, discarded, and left an operator believing they had set it. LLMConfig now forbids extras, which Config.from_yaml turns into a refusal to start -- the honest answer to a setting that cannot be honoured, and the guard that makes FR-004 real rather than aspirational. The shape is @AaryanCode69's from #112: named fields rather than #151's "provider/model" string, because base_url has nowhere to live in a flat string and Plant Reactome needs it. Verified against the quickstart, not just the unit tests: no llm section resolves exactly as before, luna resolves and derives temperature 1.0, LLM_MODEL overrides the file, a contradictory pair raises SystemExit, an unknown model still starts, and an embedding model in config.yml is fatal. The guard was perturbation-checked -- deleting it fails the test that covers it. Co-Authored-By: Claude Opus 5 --- .config.schema.yaml | 24 ++++++ bin/chat-chainlit.py | 2 +- config_default.yml | 11 +++ specs/003-model-configuration/tasks.md | 44 +++++----- src/agent/graph.py | 72 ++++++++++++++-- src/util/config_yml/__init__.py | 6 ++ src/util/config_yml/models.py | 45 ++++++++++ tests/agent/test_model_configuration.py | 109 ++++++++++++++++++++++++ tests/agent/test_model_temperature.py | 43 ++++++++++ 9 files changed, 325 insertions(+), 31 deletions(-) create mode 100644 src/util/config_yml/models.py create mode 100644 tests/agent/test_model_configuration.py diff --git a/.config.schema.yaml b/.config.schema.yaml index 9fca799..d2f1205 100644 --- a/.config.schema.yaml +++ b/.config.schema.yaml @@ -14,6 +14,30 @@ properties: enum: ["all", "logged_in"] required: ["enabled"] required: ["postprocessing"] + llm: + # The answering model. Every field is optional; an absent `llm:` section + # leaves behaviour exactly as it was. There is deliberately no embedding + # model here -- it is derived from the installed bundle, because a query + # embedded with a different model than built the vectors returns nonsense + # rather than an error. + type: object + properties: + provider: + type: string + enum: ["openai", "ollama"] + model: + type: string + description: "e.g. gpt-4o-mini, gpt-5.6-luna. LLM_MODEL overrides this." + base_url: + type: string + description: "OpenAI-compatible endpoint, for self-hosted models." + temperature: + type: number + description: >- + Almost always leave unset: the value a model requires is derived. Set + it only for a model the derived table has not met. A value the model + refuses stops the server at startup. + additionalProperties: false messages: type: object additionalProperties: diff --git a/bin/chat-chainlit.py b/bin/chat-chainlit.py index 70b9d4a..fbbbae8 100644 --- a/bin/chat-chainlit.py +++ b/bin/chat-chainlit.py @@ -47,7 +47,7 @@ config: Config | None = Config.from_yaml() profiles: list[ProfileName] = config.profiles if config else [ProfileName.React_to_Me] -llm_graph = AgentGraph(profiles) +llm_graph = AgentGraph(profiles, llm_config=config.llm if config else None) POSTGRES_CHAINLIT_DB = os.getenv("POSTGRES_CHAINLIT_DB") S3_BUCKET = os.getenv("S3_BUCKET") diff --git a/config_default.yml b/config_default.yml index e53055a..5db5e33 100644 --- a/config_default.yml +++ b/config_default.yml @@ -3,6 +3,17 @@ profiles: - React-to-Me +# The answering model. Commented out on purpose: with no `llm:` section the +# built-in default is used, which is what every existing deployment expects. +# LLM_MODEL in the environment overrides whatever is set here. +# +#llm: +# provider: openai +# model: gpt-4o-mini +# +# Do not add an embedding model here. It comes from the bundle that built the +# vectors; setting it to anything else makes retrieval silently meaningless. + features: postprocessing: # external web search feature enabled: true diff --git a/specs/003-model-configuration/tasks.md b/specs/003-model-configuration/tasks.md index f6a2836..99d6662 100644 --- a/specs/003-model-configuration/tasks.md +++ b/specs/003-model-configuration/tasks.md @@ -16,17 +16,17 @@ test asserts the failure. They are written with the code they cover, not after. ## Phase 1: Setup -- [ ] T001 Create branch `feat/model-configuration` from `origin/main` -- [ ] T002 Re-read #112 and #151 with `gh pr diff`, to credit them accurately in the commits that land their idea +- [x] T001 Create branch `feat/model-configuration` from `origin/main` +- [x] T002 Re-read #112 and #151 with `gh pr diff`, to credit them accurately in the commits that land their idea ## Phase 2: Foundational **Blocking: every user story below depends on the config field existing.** -- [ ] T003 Create `LLMConfig` (`provider: str = "openai"`, `model: str | None = None`, `base_url: str | None = None`, `temperature: float | None = None`) in `src/util/config_yml/models.py`, after the shape in #112 and crediting @AaryanCode69 -- [ ] T004 Add `llm: LLMConfig | None = None` to `Config` in `src/util/config_yml/__init__.py` — optional, so a config without it is unchanged (FR-002) -- [ ] T005 [P] Add the matching `llm` object to `.config.schema.yaml`, with **no** embedding field (FR-004) -- [ ] T006 [P] Document the section, commented out, in `config_default.yml` +- [x] T003 Create `LLMConfig` (`provider: str = "openai"`, `model: str | None = None`, `base_url: str | None = None`, `temperature: float | None = None`) in `src/util/config_yml/models.py`, after the shape in #112 and crediting @AaryanCode69 +- [x] T004 Add `llm: LLMConfig | None = None` to `Config` in `src/util/config_yml/__init__.py` — optional, so a config without it is unchanged (FR-002) +- [x] T005 [P] Add the matching `llm` object to `.config.schema.yaml`, with **no** embedding field (FR-004) +- [x] T006 [P] Document the section, commented out, in `config_default.yml` ## Phase 3: User Story 1 — A deployment names its model beside its other settings (P1) @@ -35,13 +35,13 @@ test asserts the failure. They are written with the code they cover, not after. **Independent test**: set a model in `config.yml`, start the server, ask a question, confirm from the log which model answered. Quickstart steps 1–3. -- [ ] T007 [US1] Add `resolve_llm_model(config)` to `src/agent/graph.py`: `LLM_MODEL` beats `config.llm.model` beats the current default, and document why the precedence is the reverse of `util/secrets.py` (both are "the more specific wins") -- [ ] T008 [US1] Wire `AgentGraph.__init__` to it, passing `base_url` and `provider` from the config when present, in `src/agent/graph.py` -- [ ] T009 [US1] Log the effective model at startup in `src/agent/graph.py` (FR-008) — the name only, never a key -- [ ] T010 [P] [US1] Test in `tests/agent/test_model_configuration.py`: no `llm` section behaves exactly as today (FR-002) -- [ ] T011 [P] [US1] Test in `tests/agent/test_model_configuration.py`: a configured model is the one selected -- [ ] T012 [P] [US1] Test in `tests/agent/test_model_configuration.py`: `LLM_MODEL` overrides `config.yml` (FR-003) -- [ ] T013 [US1] Run quickstart steps 1–3 against a real bundle and confirm the log names the expected model each time (constitution Article I) +- [x] T007 [US1] Add `resolve_llm_model(config)` to `src/agent/graph.py`: `LLM_MODEL` beats `config.llm.model` beats the current default, and document why the precedence is the reverse of `util/secrets.py` (both are "the more specific wins") +- [x] T008 [US1] Wire `AgentGraph.__init__` to it, passing `base_url` and `provider` from the config when present, in `src/agent/graph.py` +- [x] T009 [US1] Log the effective model at startup in `src/agent/graph.py` (FR-008) — the name only, never a key +- [x] T010 [P] [US1] Test in `tests/agent/test_model_configuration.py`: no `llm` section behaves exactly as today (FR-002) +- [x] T011 [P] [US1] Test in `tests/agent/test_model_configuration.py`: a configured model is the one selected +- [x] T012 [P] [US1] Test in `tests/agent/test_model_configuration.py`: `LLM_MODEL` overrides `config.yml` (FR-003) +- [x] T013 [US1] Run quickstart steps 1–3 against a real bundle and confirm the log names the expected model each time (constitution Article I) ## Phase 4: User Story 2 — An unusable model stops the server, not the conversation (P1) @@ -50,12 +50,12 @@ confirm from the log which model answered. Quickstart steps 1–3. **Independent test**: `gpt-5.6-luna` with `temperature: 0` must refuse to start. Quickstart steps 4–5. -- [ ] T014 [US2] Extend `resolve_temperature` in `src/agent/graph.py` to accept a configured temperature and raise `SystemExit` naming model, value and fix when the model refuses it (FR-006) -- [ ] T015 [P] [US2] Test in `tests/agent/test_model_temperature.py`: luna + `temperature: 0` exits, and the message contains all three of model, value and remedy -- [ ] T016 [P] [US2] Test in `tests/agent/test_model_temperature.py`: a model absent from the table starts normally (FR-007) -- [ ] T017 [P] [US2] Test in `tests/agent/test_model_temperature.py`: `LLM_TEMPERATURE` still wins over the configured value -- [ ] T018 [US2] Perturbation check: delete the guard and confirm T015 fails — a test that cannot fail is not a tripwire (Article III) -- [ ] T019 [US2] Run quickstart steps 4–5 and confirm the server refuses to start rather than failing on the first question +- [x] T014 [US2] Extend `resolve_temperature` in `src/agent/graph.py` to accept a configured temperature and raise `SystemExit` naming model, value and fix when the model refuses it (FR-006) +- [x] T015 [P] [US2] Test in `tests/agent/test_model_temperature.py`: luna + `temperature: 0` exits, and the message contains all three of model, value and remedy +- [x] T016 [P] [US2] Test in `tests/agent/test_model_temperature.py`: a model absent from the table starts normally (FR-007) +- [x] T017 [P] [US2] Test in `tests/agent/test_model_temperature.py`: `LLM_TEMPERATURE` still wins over the configured value +- [x] T018 [US2] Perturbation check: delete the guard and confirm T015 fails — a test that cannot fail is not a tripwire (Article III) +- [x] T019 [US2] Run quickstart steps 4–5 and confirm the server refuses to start rather than failing on the first question ## Phase 5: User Story 3 — Surfaces choose their own model (P2) @@ -68,9 +68,9 @@ Quickstart steps 4–5. ## Phase 6: Polish & Cross-Cutting -- [ ] T022 [P] Verify `grep -rn embedding .config.schema.yaml config_default.yml` finds no embedding model field (SC-004), and add a test asserting it -- [ ] T023 [P] Confirm `tests/util/test_config.py` passes **untouched** — adding a section must not change what an invalid config does (Article III) -- [ ] T024 Run `ruff check`, `ruff format --check`, `mypy`, `pytest` +- [x] T022 [P] Verify `grep -rn embedding .config.schema.yaml config_default.yml` finds no embedding model field (SC-004), and add a test asserting it +- [x] T023 [P] Confirm `tests/util/test_config.py` passes **untouched** — adding a section must not change what an invalid config does (Article III) +- [x] T024 Run `ruff check`, `ruff format --check`, `mypy`, `pytest` - [ ] T025 Close #112 with credit to @AaryanCode69, stating plainly that the LLM half is harvested and the embedding half rejected because it bypasses `resolve_embedding_model()` and would silently break Plant Reactome - [ ] T026 Close #151 with credit to @bhavyakeerthi3, noting the flat-string shape was reasonable but `base_url` has nowhere to live in it - [ ] T027 Update `specs/003-model-configuration/spec.md` with the outcome, and record D1 as taken-as-recommended diff --git a/src/agent/graph.py b/src/agent/graph.py index e1d19dc..561e983 100644 --- a/src/agent/graph.py +++ b/src/agent/graph.py @@ -17,6 +17,7 @@ from agent.models import get_embedding, get_llm from agent.profiles import ProfileName, create_profile_graphs from agent.profiles.base import InputState, OutputState +from util.config_yml.models import LLMConfig from util.embedding_environment import EmbeddingEnvironment from util.logging import logging from util.secrets import get_db_uri @@ -126,11 +127,22 @@ def resolve_embedding_model() -> str: _SNAPSHOT_SUFFIX = re.compile(r"-\d{4}-\d{2}-\d{2}$") -def resolve_temperature(model: str) -> float: +def refuses_zero(model: str) -> bool: + """Whether `model` accepts only its own default temperature.""" + return _SNAPSHOT_SUFFIX.sub("", model) in FIXED_TEMPERATURE_MODELS + + +def resolve_temperature(model: str, *, configured: float | None = None) -> float: """The temperature to send for `model`. Returning 1.0 for the models above trades determinism for being able to use them at all. That trade is made here, once, rather than at each call site. + + A `configured` value that the model is known to refuse stops the process. + Without that check the mistake surfaces as a 400 on a user's first question + -- visible to a user, attributed to the chatbot, and diagnosable only from + logs. A model the table has not met is not validated at all: the table is + empirical and always behind, so an unknown model must not block startup. """ override = os.getenv("LLM_TEMPERATURE") if override is not None and override.strip() != "": @@ -138,26 +150,70 @@ def resolve_temperature(model: str) -> float: return float(override) except ValueError: raise SystemExit(f"LLM_TEMPERATURE={override!r} is not a number.") from None - if _SNAPSHOT_SUFFIX.sub("", model) in FIXED_TEMPERATURE_MODELS: - return FIXED_TEMPERATURE - return 0.0 + + if configured is not None: + if refuses_zero(model) and configured != FIXED_TEMPERATURE: + raise SystemExit( + f"config.yml sets llm.temperature={configured} for {model!r}, " + f"which accepts only {FIXED_TEMPERATURE}. Remove the temperature " + "and it will be derived, or set LLM_TEMPERATURE to override." + ) + return configured + + return FIXED_TEMPERATURE if refuses_zero(model) else 0.0 + + +DEFAULT_LLM_MODEL = "gpt-4o-mini" + + +def resolve_llm_model(llm_config: "LLMConfig | None") -> tuple[str, str, str | None]: + """Pick the answering model: environment, then config.yml, then the default. + + Returns (provider, model, base_url). + + Environment beats file here, which is the reverse of util/secrets.py, where a + mounted Docker secret beats the environment. The two are the same rule seen + from different sides -- the more specific source wins. A secret is mounted BY + a deployment and should beat a file committed to the repository; LLM_MODEL is + how an operator overrides a committed config.yml for one container without + editing it. Stating this because the inconsistency looks like a bug until you + see which way each one points. + """ + provider = "openai" + model = DEFAULT_LLM_MODEL + base_url = os.getenv("LLM_BASE_URL") + + if llm_config is not None: + provider = llm_config.provider + model = llm_config.model or model + base_url = base_url or llm_config.base_url + + return provider, os.getenv("LLM_MODEL", model), base_url class AgentGraph: def __init__( self, profiles: list[ProfileName], + llm_config: "LLMConfig | None" = None, ) -> None: # Get base models embedding_model = resolve_embedding_model() - llm_model = os.getenv("LLM_MODEL", "gpt-4o-mini") - llm_base_url = os.getenv("LLM_BASE_URL", None) + llm_provider, llm_model, llm_base_url = resolve_llm_model(llm_config) + temperature = resolve_temperature( + llm_model, configured=llm_config.temperature if llm_config else None + ) + # The name only. A model id is not a secret, but this is the line a key + # would end up on if anyone ever widened it. + logging.info( + f"Answering with {llm_provider}/{llm_model} at temperature {temperature}" + ) llm: BaseChatModel = get_llm( - "openai", + llm_provider, llm_model, base_url=llm_base_url, request_timeout=360.0, - temperature=resolve_temperature(llm_model), + temperature=temperature, ) embedding_base_url = os.getenv("OPENAI_BASE_URL", None) embedding: Embeddings = get_embedding( diff --git a/src/util/config_yml/__init__.py b/src/util/config_yml/__init__.py index cccded1..eea29b5 100644 --- a/src/util/config_yml/__init__.py +++ b/src/util/config_yml/__init__.py @@ -7,6 +7,7 @@ from agent.profile_names import ProfileName from util.config_yml.features import Feature, Features from util.config_yml.messages import Message, TriggerEvent +from util.config_yml.models import LLMConfig from util.config_yml.usage_limits import MessageRate, UsageLimits from util.config_yml.user_matching import match_user from util.logging import logging @@ -21,6 +22,11 @@ class Config(BaseModel): features: Features + # Optional, and None rather than a default instance: a config.yml with no + # `llm:` section must behave exactly as it did before this field existed + # (spec 003 FR-002), and "absent" has to be distinguishable from "present + # and empty" for that to hold. + llm: LLMConfig | None = None messages: dict[str, Message] profiles: list[ProfileName] usage_limits: UsageLimits diff --git a/src/util/config_yml/models.py b/src/util/config_yml/models.py new file mode 100644 index 0000000..6e04d0d --- /dev/null +++ b/src/util/config_yml/models.py @@ -0,0 +1,45 @@ +"""Which model a deployment answers with. + +The shape is @AaryanCode69's from #112 -- named fields rather than a +"provider/model" string to re-parse, which is also where `base_url` can live. +Plant Reactome needs that: it serves its embedding model from a self-hosted +OpenAI-compatible endpoint. + +What is deliberately absent is an embedding model. It is derived from the bundle +that built the vectors (`agent.graph.resolve_embedding_model`), because a query +embedded with a different model than the stored vectors returns nonsense rather +than an error. Both #112 and #151 made it configurable; that is the one part of +them not taken. See specs/003-model-configuration/spec.md FR-004. +""" + +from pydantic import BaseModel, ConfigDict + + +class LLMConfig(BaseModel): + """The answering model. Every field is optional, so an `llm:` section may set + only what it wants to change and inherit the rest.""" + + # extra="forbid" so an unknown key is a validation error, which Config.from_yaml + # treats as fatal. Pydantic's default is to ignore extras silently -- meaning a + # config.yml saying `embedding_model: text-embedding-3-large` would be accepted, + # discarded, and leave an operator believing they had set it. Refusing to start + # is the only honest answer to a setting that cannot be honoured. + model_config = ConfigDict(extra="forbid") + + provider: str = "openai" + + # None means "not configured here", which leaves LLM_MODEL and then the + # built-in default in charge. A default of "gpt-4o-mini" would instead make + # every config.yml silently pin that model, which is the opposite of + # FR-002's promise that an absent section changes nothing. + model: str | None = None + + base_url: str | None = None + + # Almost always leave unset. The value a model requires is derived in + # agent.graph.resolve_temperature from a measured table, because it is a + # property of the model rather than a preference: the gpt-5.5/5.6 families, + # o3 and o4-mini accept only 1.0, while gpt-5.1/5.2/5.4 accept 0.0. Setting + # it here to something the model refuses now stops startup rather than + # failing on a user's first question (FR-006). + temperature: float | None = None diff --git a/tests/agent/test_model_configuration.py b/tests/agent/test_model_configuration.py new file mode 100644 index 0000000..71526c1 --- /dev/null +++ b/tests/agent/test_model_configuration.py @@ -0,0 +1,109 @@ +"""Which model answers, and where that choice comes from. + +Precedence is LLM_MODEL, then config.yml, then the built-in default. That is the +reverse of util/secrets.py, where a mounted Docker secret beats the environment -- +the same rule from opposite sides: the more specific source wins. A secret is +mounted BY a deployment and should beat a committed file; LLM_MODEL is how one +container overrides a committed config.yml. +""" + +import pytest + +pytest.importorskip("langchain_openai", reason="LLM stack not installed") + +from pydantic import ValidationError # noqa: E402 + +from agent.graph import DEFAULT_LLM_MODEL, resolve_llm_model # noqa: E402 +from util.config_yml.models import LLMConfig # noqa: E402 + + +@pytest.fixture(autouse=True) +def _no_env(monkeypatch: pytest.MonkeyPatch) -> None: + """A developer's LLM_MODEL must not decide what these tests assert.""" + monkeypatch.delenv("LLM_MODEL", raising=False) + monkeypatch.delenv("LLM_BASE_URL", raising=False) + + +def test_no_configuration_behaves_exactly_as_before() -> None: + """FR-002, and the scenario every existing deployment is in. + + The most important test here: adding this feature must be invisible to a + config.yml that does not use it. + """ + assert resolve_llm_model(None) == ("openai", DEFAULT_LLM_MODEL, None) + + +def test_an_empty_llm_section_also_changes_nothing() -> None: + """`llm: {}` is a section that sets nothing, not a request for something.""" + assert resolve_llm_model(LLMConfig()) == ("openai", DEFAULT_LLM_MODEL, None) + + +def test_a_configured_model_is_the_one_selected() -> None: + provider, model, base_url = resolve_llm_model(LLMConfig(model="gpt-5.6-luna")) + assert (provider, model, base_url) == ("openai", "gpt-5.6-luna", None) + + +def test_the_environment_overrides_the_file(monkeypatch: pytest.MonkeyPatch) -> None: + """FR-003. How one container is pointed elsewhere without editing a file.""" + monkeypatch.setenv("LLM_MODEL", "gpt-4o-mini") + _, model, _ = resolve_llm_model(LLMConfig(model="gpt-5.6-luna")) + assert model == "gpt-4o-mini" + + +def test_the_environment_overrides_the_base_url_too( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setenv("LLM_BASE_URL", "https://from-the-environment/v1") + _, _, base_url = resolve_llm_model(LLMConfig(base_url="https://from-the-file/v1")) + assert base_url == "https://from-the-environment/v1" + + +def test_a_self_hosted_endpoint_can_be_configured() -> None: + """Plant Reactome serves a model from its own OpenAI-compatible endpoint. + + This is why the config is named fields rather than a "provider/model" + string, as #151 proposed -- base_url has nowhere to live in a flat string. + """ + provider, model, base_url = resolve_llm_model( + LLMConfig(provider="ollama", model="bge-m3", base_url="http://localhost:11434") + ) + assert (provider, model, base_url) == ( + "ollama", + "bge-m3", + "http://localhost:11434", + ) + + +def test_the_config_model_cannot_name_an_embedding_model() -> None: + """FR-004, the one part of #112 and #151 deliberately not taken. + + The embedding model is derived from the bundle that built the vectors. A + query embedded with a different model returns nonsense rather than an error, + so there is no safe way to configure it. + """ + assert "embedding" not in LLMConfig.model_fields + + # And naming one is fatal rather than ignored. Pydantic's default is to drop + # unknown keys silently, which would leave an operator believing they had set + # something; Config.from_yaml turns this ValidationError into a refusal to start. + with pytest.raises(ValidationError, match="embedding_model"): + LLMConfig(embedding_model="text-embedding-3-large") # type: ignore[call-arg] + + +def test_no_configuration_file_can_name_an_embedding_model() -> None: + """SC-004, checked against the files an operator actually edits. + + The schema and the shipped default are what a person copies from. If either + showed an embedding model, someone would set it. + """ + from pathlib import Path + + repo = Path(__file__).parent.parent.parent + for name in (".config.schema.yaml", "config_default.yml"): + text = repo.joinpath(name).read_text() + offending = [ + line + for line in text.splitlines() + if "embedding" in line.lower() and not line.lstrip().startswith("#") + ] + assert not offending, f"{name} offers an embedding setting: {offending}" diff --git a/tests/agent/test_model_temperature.py b/tests/agent/test_model_temperature.py index 1fd5c88..9881127 100644 --- a/tests/agent/test_model_temperature.py +++ b/tests/agent/test_model_temperature.py @@ -154,3 +154,46 @@ def test_get_llm_still_defaults_to_zero(monkeypatch: pytest.MonkeyPatch) -> None """Callers that never heard of this change keep the old behaviour.""" monkeypatch.setenv("OPENAI_API_KEY", "sk-not-a-real-key") assert _built("gpt-4o-mini").temperature == 0.0 + + +# --- configured temperature, from config.yml (spec 003, US2) ------------------- + + +def test_a_configured_temperature_a_model_refuses_stops_startup() -> None: + """FR-006, and the reason spec 003 is a specification and not a bump. + + Without this the mistake surfaces as a 400 on a user's first question -- + visible to a user, attributed to the chatbot, diagnosable only from logs. + """ + with pytest.raises(SystemExit) as exc: + resolve_temperature("gpt-5.6-luna", configured=0.0) + + message = str(exc.value) + assert "gpt-5.6-luna" in message, "name the model" + assert "0.0" in message, "name the value" + assert "LLM_TEMPERATURE" in message, "name the fix" + + +def test_a_configured_temperature_a_model_accepts_is_used() -> None: + assert resolve_temperature("gpt-4o-mini", configured=0.7) == 0.7 + + +def test_the_only_supported_value_is_accepted_when_configured() -> None: + """Setting 1.0 explicitly for luna is redundant but not wrong.""" + assert resolve_temperature("gpt-5.6-luna", configured=1.0) == FIXED_TEMPERATURE + + +def test_an_unknown_model_is_not_validated(monkeypatch: pytest.MonkeyPatch) -> None: + """FR-007. The table is empirical and always behind; an unknown model must + not block startup. It fails on the first request with OpenAI's own 404, + which is unambiguous.""" + assert resolve_temperature("gpt-7-unreleased", configured=0.0) == 0.0 + + +def test_the_environment_still_wins_over_a_configured_value( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """LLM_TEMPERATURE is the escape hatch, so it must outrank config.yml -- + including the guard above, which is what makes it an escape hatch.""" + monkeypatch.setenv("LLM_TEMPERATURE", "1") + assert resolve_temperature("gpt-5.6-luna", configured=0.0) == 1.0 From ce7cfef6c2eaf8c271bdac1e39d895a81ad6738a Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Thu, 10 Sep 2026 17:55:43 +0000 Subject: [PATCH 2/2] Forbid unknown top-level config keys too Adversarial review of the LLMConfig guard: it only covered keys inside the llm section. A typo in the section NAME -- llmm: for llm: -- still loaded cleanly, did nothing, and left the operator believing they had configured a model. Same failure, one level up. Checked config.yml and config_default.yml before turning it on; neither carries an unknown key, so this refuses nothing that works today. Co-Authored-By: Claude Opus 5 --- src/util/config_yml/__init__.py | 10 +++++++++- tests/util/test_config.py | 19 +++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/util/config_yml/__init__.py b/src/util/config_yml/__init__.py index eea29b5..54b92a8 100644 --- a/src/util/config_yml/__init__.py +++ b/src/util/config_yml/__init__.py @@ -2,7 +2,7 @@ from typing import Self import yaml -from pydantic import BaseModel, ValidationError +from pydantic import BaseModel, ConfigDict, ValidationError from agent.profile_names import ProfileName from util.config_yml.features import Feature, Features @@ -21,6 +21,14 @@ class Config(BaseModel): + # extra="forbid" for the same reason LLMConfig does it, one level up. Without + # it a typo in a section name -- `llmm:` for `llm:`, or a key at the wrong + # indentation -- loads cleanly, does nothing, and leaves the operator + # believing they configured something. Checked against config.yml and + # config_default.yml before turning on: neither carries an unknown key, so + # this refuses nothing that works today. + model_config = ConfigDict(extra="forbid") + features: Features # Optional, and None rather than a default instance: a config.yml with no # `llm:` section must behave exactly as it did before this field existed diff --git a/tests/util/test_config.py b/tests/util/test_config.py index 1f011b3..66a81f6 100644 --- a/tests/util/test_config.py +++ b/tests/util/test_config.py @@ -135,3 +135,22 @@ def test_config_paths_do_not_depend_on_the_working_directory( assert CONFIG_DEFAULT_YML.is_absolute() monkeypatch.chdir(tmp_path) assert Config.from_yaml(CONFIG_DEFAULT_YML) is not None + + +def test_an_unknown_top_level_key_is_fatal(tmp_path: Path) -> None: + """A typo in a section name must not load cleanly and do nothing. + + `llmm:` for `llm:`, or a key at the wrong indentation, used to be accepted + and silently dropped -- so the operator saw a server that started fine and a + setting that had no effect. That is the failure spec 003 exists to prevent, + one level above where it was being prevented. + """ + path = tmp_path / "config.yml" + path.write_text(VALID + "\nllmm:\n model: gpt-4o-mini\n") + with pytest.raises(SystemExit, match="Invalid config"): + Config.from_yaml(path) + + +def test_the_shipped_configs_carry_no_unknown_keys() -> None: + """Turning on extra="forbid" must not refuse a config that works today.""" + assert Config.from_yaml(CONFIG_DEFAULT_YML) is not None