From 11c3116e93e0bb9d33657adaf3746825930935a4 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Thu, 10 Sep 2026 18:46:28 +0000 Subject: [PATCH 1/3] Close out spec 003: implemented, #112 and #151 closed with credit T025-T027. Both contributed PRs are closed as implemented, each with a comment saying specifically what was taken from it and why the embedding half was not: a query embedded with a different model than built the vectors returns confident nonsense rather than an error, so the model comes from the bundle path and config.yml deliberately cannot name one. Records the two defects writing it found -- pydantic silently ignoring unknown keys, and the same guard being one level too low so a typo in the section name loaded cleanly and did nothing -- and the pattern behind both: the guard was placed on the thing being built rather than on the seam beside it. User Story 3 remains unbuilt on purpose; only chat exists. Co-Authored-By: Claude Opus 5 --- specs/003-model-configuration/spec.md | 42 +++++++++++++++++++++++++- specs/003-model-configuration/tasks.md | 4 +-- 2 files changed, 43 insertions(+), 3 deletions(-) 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..ea793a7 100644 --- a/specs/003-model-configuration/tasks.md +++ b/specs/003-model-configuration/tasks.md @@ -71,8 +71,8 @@ Quickstart steps 4–5. - [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 +- [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 - [ ] T027 Update `specs/003-model-configuration/spec.md` with the outcome, and record D1 as taken-as-recommended ## Dependencies From fb270c1d25aaa315031088b2d112271c6b1a9969 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Thu, 10 Sep 2026 18:47:24 +0000 Subject: [PATCH 2/3] Record what a second surface would add (T020, T021) All 27 tasks of spec 003 complete. User Story 3 stays unbuilt -- only chat exists -- but the plan now shows the shape a second surface takes and why one would want to differ: 22.5s per question against 41.2s is a real gap between a search-results panel and a background summarisation. Deliberate this time, rather than discovered later as the context budget was in spec 001. Co-Authored-By: Claude Opus 5 --- specs/003-model-configuration/plan.md | 19 +++++++++++++++++++ specs/003-model-configuration/tasks.md | 4 ++-- 2 files changed, 21 insertions(+), 2 deletions(-) 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/tasks.md b/specs/003-model-configuration/tasks.md index ea793a7..a80b253 100644 --- a/specs/003-model-configuration/tasks.md +++ b/specs/003-model-configuration/tasks.md @@ -63,8 +63,8 @@ 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 From 432d9c27755b9806208e9647ca92a7d178d2410e Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Thu, 10 Sep 2026 18:47:58 +0000 Subject: [PATCH 3/3] Tick T027; the spec outcome was written but the box was not checked Co-Authored-By: Claude Opus 5 --- specs/003-model-configuration/tasks.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specs/003-model-configuration/tasks.md b/specs/003-model-configuration/tasks.md index a80b253..4ec4288 100644 --- a/specs/003-model-configuration/tasks.md +++ b/specs/003-model-configuration/tasks.md @@ -73,7 +73,7 @@ Quickstart steps 4–5. - [x] T024 Run `ruff check`, `ruff format --check`, `mypy`, `pytest` - [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 -- [ ] T027 Update `specs/003-model-configuration/spec.md` with the outcome, and record D1 as taken-as-recommended +- [x] T027 Update `specs/003-model-configuration/spec.md` with the outcome, and record D1 as taken-as-recommended ## Dependencies