diff --git a/specs/003-model-configuration/plan.md b/specs/003-model-configuration/plan.md index 1f348be..2c0482c 100644 --- a/specs/003-model-configuration/plan.md +++ b/specs/003-model-configuration/plan.md @@ -154,4 +154,23 @@ embedding model (SC-004). 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. + + Confirmed nestable without a schema break (T020): a second surface adds a key + beside `llm:` holding the same `LLMConfig` shape -- + + ```yaml + llm: # the deployment default + model: gpt-4o-mini + surfaces: + analysis_summary: # slower is fine; nobody is watching a cursor + model: gpt-5.6-luna + ``` + + `LLMConfig` needs no change for that, and `resolve_llm_model` takes the config + object rather than reading globals, so a caller can pass a different one. + + Why surfaces will want to differ (T021): spec 002 measured 22.5s per question + for gpt-4o-mini against 41.2s for gpt-5.6-luna. A search-results panel and a + background summarisation have opposite tolerances for that, so forcing them to + agree is a choice with a real cost. - The embedding model, permanently. diff --git a/specs/003-model-configuration/spec.md b/specs/003-model-configuration/spec.md index 0d550f4..e407667 100644 --- a/specs/003-model-configuration/spec.md +++ b/specs/003-model-configuration/spec.md @@ -4,7 +4,9 @@ **Created**: 2026-09-09 -**Status**: Draft. Two contributed PRs to harvest; one decision (D1) for the team. +**Status**: **Implemented** 2026-09-10 (#200 plan, #201 implementation). D1 taken as +recommended — validation against the table, no API call at startup. #112 and #151 +closed with credit. Outcome recorded at the end of this file. **Input**: Make it possible to switch between models — `gpt-4o-mini` and `gpt-5.6-luna` in particular — when per-model settings such as temperature differ. Does this need something to configure it with? @@ -243,3 +245,41 @@ on the first request, and no one is confused about the cause. specification makes the choice expressible; it does not make it. - Per-request model selection by an end user. - The embedding model, permanently. FR-004. + + +--- + +## Outcome (2026-09-10) + +Implemented in #201, planned in #200. The answering model is an optional `llm:` +section in `config.yml`; how it must be *called* stays derived in code. + +D1 was taken as recommended: startup validates a configured temperature against +the measured table, with no API call. Instant, works offline, and startup cannot +come to depend on OpenAI being reachable. + +### What writing it found + +**Pydantic ignores unknown keys by default.** `embedding_model:` inside an `llm:` +section would have been accepted, silently discarded, and left an operator +believing they had set it — the exact failure FR-004 exists to prevent, arriving +through the door FR-004 was meant to lock. `LLMConfig` now forbids extras. + +**And the guard was one level too low.** An adversarial review found that a typo in +the *section name* — `llmm:` for `llm:` — still loaded cleanly and did nothing. +`Config` forbids extras too now. Both were checked against the real `config.yml` +and `config_default.yml` first: neither carries an unknown key, so nothing that +works today is refused. + +The pattern in both: the guard was placed on the thing being built rather than on +the seam beside it. + +### Still open + +**User Story 3, per-surface model selection**, ships nothing. Only chat exists. The +nesting chosen in Stage 1 is what makes it cheap when a second surface appears — +which is the mistake spec 001 made with the context budget, made deliberately this +time rather than discovered. + +Spec 002's decision — *whether* luna becomes the default — remains open. This +specification made the choice expressible, not made. diff --git a/specs/003-model-configuration/tasks.md b/specs/003-model-configuration/tasks.md index 99d6662..4ec4288 100644 --- a/specs/003-model-configuration/tasks.md +++ b/specs/003-model-configuration/tasks.md @@ -63,17 +63,17 @@ Quickstart steps 4–5. **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 +- [x] 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 +- [x] 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 - [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 +- [x] 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 +- [x] T026 Close #151 with credit to @bhavyakeerthi3, noting the flat-string shape was reasonable but `base_url` has nowhere to live in it +- [x] T027 Update `specs/003-model-configuration/spec.md` with the outcome, and record D1 as taken-as-recommended ## Dependencies