Plan and task out spec 003, model configuration

specs/003-model-configuration/plan.md

@@ -0,0 +1,157 @@
+# Implementation Plan: Model Configuration
+
+**Branch**: `feat/model-configuration` | **Date**: 2026-09-10 | **Spec**: [spec.md](./spec.md)
+
+**Input**: Feature specification from `/specs/003-model-configuration/spec.md`
+
+## Summary
+
+Move the answering model into `config.yml`, where a deployment already declares its
+profiles, quotas, features and messages. Keep the model's *call requirements* —
+temperature, structured-output method — derived in code, and refuse at startup a
+configuration that contradicts them.
+
+Harvests the LLM half of #112 (@AaryanCode69) and #151 (@bhavyakeerthi3), and
+rejects the embedding half of both.
+
+## Technical Context
+
+**Language/Version**: Python 3.12
+
+**Primary Dependencies**: pydantic v2 (`Config` is already a `BaseModel`),
+`langchain-openai` 1.6 via `agent.models.get_llm`.
+
+**Storage**: `config.yml` at the repo root, bind-mounted read-only into the
+container; `config_default.yml` shipped in the image as the fallback.
+
+**Testing**: pytest. `tests/util/test_config.py` already pins the loader's failure
+behaviour, including that an invalid file is fatal rather than silently defaulted.
+`tests/agent/test_model_temperature.py` pins the derived table.
+
+**Target Platform**: Linux container, and developer machines.
+
+**Project Type**: Library within a single application repository.
+
+**Performance Goals**: None. This is a startup-time concern; it runs once.
+
+**Constraints**: A deployment with no model configured must behave exactly as it
+does today (FR-002). `resolve_embedding_model()` must remain the only source of the
+embedding model (FR-004).
+
+**Scale/Scope**: One new config section, one validation, four call sites at most.
+
+## Constitution Check
+
+*GATE: must pass before implementation, re-checked after.*
+
+| Article | How this plan satisfies it |
+|---|---|
+| I — verify the user path | The exit criterion is the server starting with a model set in `config.yml` and answering a question with it — not a unit test asserting the field parses. |
+| II — measure, don't argue | Nothing here changes retrieval, so no baseline is owed. The model *choice* is spec 002's measurement, and this plan deliberately does not make it. |
+| III — characterization tests pin behaviour | `test_config.py`'s existing tests must pass untouched: adding a section must not change what an invalid config does. |
+| IV — fail loudly | The whole point of Stage 2. A contradictory model/temperature pair stops startup; it does not get silently corrected. |
+| V — derive from the source of truth | FR-004 and FR-005 are this article restated: the embedding model comes from the bundle, the temperature from the measured table. Neither becomes configurable. |
+| VI — bias to doing over filing | Two contributed PRs are harvested here rather than left open another six months. |
+
+**No violations.** The one judgement call is D1 (validate against the table, not the
+API), taken as recommended in the spec: no network call at startup.
+
+## Project Structure
+
+### Documentation (this feature)
+
+```text
+specs/003-model-configuration/
+├── spec.md          # what and why, with D1 recommended
+├── plan.md          # this file
+└── checklists/
+    └── requirements.md
+```
+
+No `research.md`: there are no unresolved unknowns. The two contributed PRs are the
+research, the temperature behaviour was measured empirically in #189, and D1 is
+decided. No `data-model.md` or `contracts/` — the "model" is three optional fields
+on an existing pydantic class, described below in full, and the only external
+contract is `config.yml`, whose schema file is edited in Stage 1.
+
+### Source Code
+
+```text
+src/util/config_yml/
+├── __init__.py       # Config gains an optional `llm` section
+└── models.py         # new: LLMConfig, after #112's shape
+
+src/agent/
+├── graph.py          # AgentGraph reads the config; resolve_temperature validates
+└── models.py         # unchanged -- get_llm already takes model and temperature
+
+.config.schema.yaml   # the editor-facing schema gains the same section
+config_default.yml    # documents the section without setting it
+```
+
+## Implementation Stages
+
+### Stage 1 — The model becomes configurable, with today's behaviour as the default
+
+Add `LLMConfig` (`provider`, `model`, `base_url`) and an optional `llm` field on
+`Config`. `AgentGraph` prefers it, falling back to `LLM_MODEL`, then to the current
+default.
+
+Precedence is **environment over file** (FR-003), the opposite of Docker secrets in
+`util/secrets.py`, and the difference is worth stating: a secret is mounted *by* the
+deployment and should beat a checked-in file, whereas `LLM_MODEL` is how an operator
+overrides a committed `config.yml` for one container. Both rules are "the more
+specific thing wins"; they only look contradictory.
+
+Nothing is required. A `config.yml` with no `llm:` section behaves exactly as today,
+which is what keeps every existing deployment working (FR-002).
+
+**Exit criteria**: `test_config.py` passes untouched; a `config.yml` naming
+`gpt-5.6-luna` starts the server and answers a question with it; the effective model
+appears in the startup log (FR-008).
+
+### Stage 2 — A contradictory configuration stops startup
+
+`resolve_temperature` already knows which models refuse `0.0`. If `config.yml` (or
+`LLM_TEMPERATURE`) sets a temperature a model will reject, startup fails naming the
+model, the value, and the fix.
+
+This is the reason the feature is a specification rather than a bump. Today that
+mistake is a 400 on a user's first question — visible to a user, attributed to the
+chatbot, and diagnosable only from logs.
+
+Per D1 the check is against the table alone. No API call, so it is instant, works
+offline, and cannot make startup depend on OpenAI being reachable. A model the table
+has not met does not block startup (FR-007).
+
+**Exit criteria**: `gpt-5.6-luna` with `temperature: 0` refuses to start and names
+all three of model, value and fix; an unknown model starts normally; the existing
+`LLM_TEMPERATURE` escape hatch still wins.
+
+### Stage 3 — Close #112 and #151
+
+Both are superseded by Stages 1–2. Close them courteously, crediting each
+contributor in the commit that lands their idea, and say plainly why the embedding
+half was rejected — it bypasses `resolve_embedding_model()` and would silently break
+Plant Reactome retrieval.
+
+**Exit criteria**: both PRs closed with credit; `.config.schema.yaml` and
+`config_default.yml` document the new section; no configuration file can name an
+embedding model (SC-004).
+
+## Complexity Tracking
+
+| Decision | Simpler alternative rejected | Why |
+|---|---|---|
+| A nested `llm:` section | A flat `llm: "openai/gpt-4o-mini"` string, as #151 does | Re-parsing a string to recover provider and model is work the loader can do once. `base_url` has nowhere to live in a flat string, and Plant Reactome needs it. |
+| Validate at startup | Validate on first use | First use is a user's first question. The whole point is that the operator learns before a user does. |
+| Table only, no probe | One API call at startup | D1. It catches the mistake actually being made, for free and offline. A mistyped model name fails immediately and unmistakably anyway. |
+| Leave the embedding model alone | Make it configurable "for symmetry", as both PRs do | It is derived from the bundle that built the vectors. Configuring it is how you get silent nonsense instead of an error. |
+
+## Out of Scope
+
+- **Which model becomes the default.** [Spec 002](../002-default-llm-choice/spec.md),
+  gated on an answer-quality run. This plan makes the choice expressible, not made.
+- Per-surface model selection (spec 003 User Story 3). Only chat exists today; the
+  nesting introduced in Stage 1 is what makes it cheap later.
+- The embedding model, permanently.

specs/003-model-configuration/quickstart.md

@@ -0,0 +1,90 @@
+# Quickstart: verifying model configuration
+
+How to prove this feature works, end to end, the way a deployment uses it. Every
+step here is a thing that can be observed failing, not a unit test.
+
+## Prerequisites
+
+- an installed reactome bundle (`./bin/embeddings_manager which`)
+- `OPENAI_API_KEY` in `.env`
+
+## 1. Nothing configured behaves exactly as before (FR-002)
+
+The most important scenario, because it is what every existing deployment does.
+
+```bash
+grep -c '^llm:' config.yml || echo "no llm section — good"
+poetry run chainlit run bin/chat-chainlit.py
+```
+
+**Expect**: the server starts and the startup log names `gpt-4o-mini`. Ask a
+question; it answers.
+
+## 2. A model set in config.yml is the one that answers (FR-001, FR-008)
+
+```yaml
+# config.yml
+llm:
+  provider: openai
+  model: gpt-5.6-luna
+```
+
+**Expect**: the startup log names `gpt-5.6-luna`, and `resolve_temperature` has
+silently sent `1.0` — that model refuses `0.0` and nobody had to know.
+
+## 3. The environment overrides the file (FR-003)
+
+```bash
+LLM_MODEL=gpt-4o-mini poetry run chainlit run bin/chat-chainlit.py
+```
+
+**Expect**: `gpt-4o-mini` in the log, despite `config.yml` naming luna. This is how
+one container is overridden without editing a committed file.
+
+## 4. A contradictory pair stops startup (FR-006, SC-003)
+
+```yaml
+llm:
+  model: gpt-5.6-luna
+  temperature: 0
+```
+
+**Expect**: the server **refuses to start**, naming the model, the value, and the
+fix. It must not start and fail later — that is the entire point of Stage 2.
+
+Compare with what happens without this feature: the server starts, and the first
+user to ask a question gets an error that reads as the chatbot being broken.
+
+## 5. A model the table has not met still starts (FR-007)
+
+```yaml
+llm:
+  model: gpt-7-whatever
+```
+
+**Expect**: the server starts. An unknown model is not an error — the table is
+empirical and always behind. It will fail on the first request with OpenAI's own
+404, which is unambiguous, and `LLM_TEMPERATURE` remains the escape hatch.
+
+## 6. No configuration file can name an embedding model (FR-004, SC-004)
+
+```bash
+grep -rn "embedding" .config.schema.yaml config_default.yml
+```
+
+**Expect**: no `embedding` model field anywhere. The embedding model is read from
+the bundle path by `resolve_embedding_model()`, because a query embedded with a
+different model than built the vectors returns nonsense rather than an error.
+
+```bash
+poetry run pytest tests/agent/test_embedding_model_resolution.py -q
+```
+
+## 7. The gates
+
+```bash
+poetry run ruff check . && poetry run mypy . && poetry run pytest
+```
+
+`tests/util/test_config.py` must pass **untouched**: adding a section must not
+change what the loader does with an invalid file.

specs/003-model-configuration/tasks.md

@@ -0,0 +1,119 @@
+---
+description: "Task list for model configuration"
+---
+
+# Tasks: Model Configuration
+
+**Input**: Design documents from `/specs/003-model-configuration/`
+
+**Prerequisites**: [plan.md](./plan.md), [spec.md](./spec.md), [quickstart.md](./quickstart.md)
+
+**Tests**: Included. This repository's constitution (Article III) makes tests the
+tripwire for behaviour, and Article IV's "fail loudly" requirement is only real if a
+test asserts the failure. They are written with the code they cover, not after.
+
+**Branch**: `feat/model-configuration`
+
+## 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
+
+## 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`
+
+## Phase 3: User Story 1 — A deployment names its model beside its other settings (P1)
+
+**Goal**: the model comes from `config.yml`.
+
+**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)
+
+## Phase 4: User Story 2 — An unusable model stops the server, not the conversation (P1)
+
+**Goal**: a contradictory model/temperature pair is refused at startup.
+
+**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
+
+## Phase 5: User Story 3 — Surfaces choose their own model (P2)
+
+**Goal**: not built now; make sure Stage 1's shape does not preclude it.
+
+**Independent test**: none — this phase ships no behaviour.
+
+- [ ] T020 [US3] Confirm `LLMConfig` is nestable per surface without a schema break, and record in `specs/003-model-configuration/plan.md` what a second surface would add
+- [ ] T021 [US3] Cross-reference spec 002's latency table (22.5s vs 41.2s per question) in the spec as the reason surfaces will want to differ
+
+## 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`
+- [ ] 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
+
+## Dependencies
+
+```text
+Phase 1 (T001-T002)
+      |
+Phase 2 (T003-T006)   <- blocking; nothing below works without the field
+      |
+      +-- Phase 3 US1 (T007-T013)   P1
+      |         |
+      +-- Phase 4 US2 (T014-T019)   P1, needs US1's resolution to validate
+      |
+      +-- Phase 5 US3 (T020-T021)   P2, documentation only
+                |
+Phase 6 (T022-T027)
+```
+
+US2 depends on US1: there is nothing to validate until the model is configurable.
+US3 depends on neither and ships no code.
+
+## Parallel Opportunities
+
+- **T005, T006** — different files, no shared state
+- **T010, T011, T012** — three tests in one new file; write together, they share fixtures
+- **T015, T016, T017** — same, in the existing temperature test file
+- **T022, T023** — independent checks
+
+`T025` and `T026` are deliberately **not** parallel with the rest: close a
+contributor's PR only once the thing that replaces it has actually landed and its
+gates are green.
+
+## Implementation Strategy
+
+**MVP is Phase 2 + Phase 3 (US1).** That alone delivers FR-001 through FR-003 and
+FR-008 — a deployment can name its model — and is independently shippable.
+
+**Phase 4 (US2) is the reason this is a specification.** It is P1 alongside US1
+rather than after it, because shipping US1 alone adds two new ways to configure a
+model wrongly without adding any way to find out before a user does.
+
+Phase 5 ships nothing and could be dropped without loss; it exists so the nesting
+decision in T003 is made deliberately rather than discovered later, which is the
+mistake the context budget made in spec 001.