Stores a model's brewing advice for each recorded observation, in its own field in Mongo, shown on the brewing page, the active beers page and the run detail page.
Now merged and deployed. CI run 1699 was all seven steps green; the live site at http://192.168.178.160:30015 was verified by recording a reading through it — the advice appeared in 28 seconds and renders on the Active Beers page.
How it behaves
Fire and forget. The advice is generated in a background thread, so recording a reading never blocks on the model. The reading appears immediately; the advice is there on a later visit. If the model is down, the reading is still saved and the advice field carries a visible note saying it could not be generated — a silent failure would be indistinguishable from "still thinking", which is confusing when you're waiting for a tip that is never going to arrive.
Your comment is never overwritten. The advice lives in its own field (advice, advice_ok, advice_at) alongside your comment. Verified on the real document.
The model
mistral-large-4:cloud on the Ollama instance at 192.168.178.45:11434 — reachable from the pod.
Both OLLAMA_MODEL and OLLAMA_URL are configurable.
The prompt tells it not to repeat the app's mistake
The system prompt says explicitly:
Never say a reading is impossible. Beer cannot be denser than water, so a gravity below 1.000 is always a mis-entry, but the rest of the run may still be real and worth understanding.
This is deliberately the opposite of the app's "exclude it as impossible" treatment of run 12 — which you have since told me was a genuine long fermentation. The model should point at the odd reading, say so, and keep talking about the brew.
What it actually said
For a stalled brew (OG 1.042, 1.022 after 9 days at 19°C, "no change since day 7"):
That 1.022 reading after nine days at 19°C—with no movement since day seven—looks like a stalled fermentation; a 1.042 wort should be well on its way to 1.010–1.012 by now, not sitting at only about 48% attenuation. Gently swirl the fermenter to rouse the yeast cake and keep the temperature steady at 19–20°C, and if the gravity hasn't budged in another 48 hours, pitch a fresh, healthy yeast starter (rehydrated dry ale yeast is fine) to finish it down to roughly 1.008–1.012 before you rack or add any dry hops.
And on the live site, for your own "Out on patio" run:
That 1.040 reading is a point higher than your 1.039 original gravity, which points to a temperature correction issue or a slight hydrometer misread rather than real progress, since yeast can only consume sugar and lower the density. At only one day in, it's perfectly normal to still be sitting at OG during the lag phase, especially if the patio is cool in November, so there's no stall to worry about yet.
The marker
The model is asked to put ADVICE: on its own line before the advice, so the split between its working and its advice is exact — no guessing at where one ends and the other begins. The marker is found however the model formats it (ADVICE:, **ADVICE:**, ## ADVICE:, in a code block, lower case).
An earlier attempt at this classified prose by marker words and length. It failed in both directions — it emptied legitimate advice, and it let the model's working through in full. Asking the model for a format is the reliable answer.
The pages
Brew Process — an advice column in the "In the fermenter" table, clamped to four lines and expanding on hover. The full text is on the run detail page.
Active Beers — the advice on the card, which is where you'd actually look during a fermentation.
Run detail — the full advice under each reading, labelled "Brewing advice" so it is never confused with your own comment.
Safety
The advice is never rendered with |safe, and there are no inline scripts (the CSP stays enforceable).
The advice is off in testing, so the suite never reaches out to a model, and LLM_ADVICE=0 turns it off in production if the model misbehaves.
A reading id is coerced before it reaches Mongo, so a caller passing a dict cannot have it interpreted as a query operator.
The bugs the reviews caught
Seven rounds, and two of these would have shipped a feature that looked like it worked while doing nothing:
The advice would never have been stored._as_doc_id accepted only ObjectIds, but all 47 of your readings have 24-character string _ids. The test fixture generates ObjectIds, so no test could have caught it — only checking against the live database did.
The advice would have been invisible for every existing reading. An empty token was used as a guard, so the write was conditioned on a token that does not exist.
A slow background thread could overwrite a correction with advice about the old value.
An unsynchronised race built eight MongoClients on a cold start.
Test plan
182 tests pass
New file tests/test_brew_advice.py: the prompt content, the marker handling, the token guard, the injection guard, the stale-advice races, and all three pages
The reading is saved even when the model is down
End-to-end against the live model: reading recorded → advice generated → stored in Mongo → rendered on the page
Verified against the live database throughout — every probe restored exactly
Deployed: CI run 1699 all green; verified on the live site
## Summary
Stores a model's brewing advice for each recorded observation, in its own field in Mongo, shown on the brewing page, the active beers page and the run detail page.
**Now merged and deployed.** CI run 1699 was all seven steps green; the live site at http://192.168.178.160:30015 was verified by recording a reading through it — the advice appeared in 28 seconds and renders on the Active Beers page.
### How it behaves
**Fire and forget.** The advice is generated in a background thread, so recording a reading never blocks on the model. The reading appears immediately; the advice is there on a later visit. If the model is down, the reading is still saved and the advice field carries a visible note saying it could not be generated — a silent failure would be indistinguishable from "still thinking", which is confusing when you're waiting for a tip that is never going to arrive.
**Your comment is never overwritten.** The advice lives in its own field (`advice`, `advice_ok`, `advice_at`) alongside your `comment`. Verified on the real document.
### The model
`mistral-large-4:cloud` on the Ollama instance at `192.168.178.45:11434` — reachable from the pod.
Both `OLLAMA_MODEL` and `OLLAMA_URL` are configurable.
### The prompt tells it not to repeat the app's mistake
The system prompt says explicitly:
> Never say a reading is impossible. Beer cannot be denser than water, so a gravity below 1.000 is always a mis-entry, but the rest of the run may still be real and worth understanding.
This is deliberately the opposite of the app's "exclude it as impossible" treatment of run 12 — which you have since told me was a **genuine long fermentation**. The model should point at the odd reading, say so, and keep talking about the brew.
### What it actually said
For a stalled brew (OG 1.042, 1.022 after 9 days at 19°C, "no change since day 7"):
> That 1.022 reading after nine days at 19°C—with no movement since day seven—looks like a stalled fermentation; a 1.042 wort should be well on its way to 1.010–1.012 by now, not sitting at only about 48% attenuation. Gently swirl the fermenter to rouse the yeast cake and keep the temperature steady at 19–20°C, and if the gravity hasn't budged in another 48 hours, pitch a fresh, healthy yeast starter (rehydrated dry ale yeast is fine) to finish it down to roughly 1.008–1.012 before you rack or add any dry hops.
And on the live site, for your own "Out on patio" run:
> That 1.040 reading is a point higher than your 1.039 original gravity, which points to a temperature correction issue or a slight hydrometer misread rather than real progress, since yeast can only consume sugar and lower the density. At only one day in, it's perfectly normal to still be sitting at OG during the lag phase, especially if the patio is cool in November, so there's no stall to worry about yet.
### The marker
The model is asked to put `ADVICE:` on its own line before the advice, so the split between its working and its advice is exact — no guessing at where one ends and the other begins. The marker is found however the model formats it (`ADVICE:`, `**ADVICE:**`, `## ADVICE:`, in a code block, lower case).
An earlier attempt at this classified prose by marker words and length. It failed in both directions — it emptied legitimate advice, and it let the model's working through in full. Asking the model for a format is the reliable answer.
### The pages
- **Brew Process** — an advice column in the "In the fermenter" table, clamped to four lines and expanding on hover. The full text is on the run detail page.
- **Active Beers** — the advice on the card, which is where you'd actually look during a fermentation.
- **Run detail** — the full advice under each reading, labelled "Brewing advice" so it is never confused with your own comment.
### Safety
- The advice is never rendered with `|safe`, and there are no inline scripts (the CSP stays enforceable).
- The advice is off in testing, so the suite never reaches out to a model, and `LLM_ADVICE=0` turns it off in production if the model misbehaves.
- A reading id is coerced before it reaches Mongo, so a caller passing a dict cannot have it interpreted as a query operator.
### The bugs the reviews caught
Seven rounds, and two of these would have shipped a feature that looked like it worked while doing nothing:
- **The advice would never have been stored.** `_as_doc_id` accepted only ObjectIds, but **all 47 of your readings have 24-character string `_id`s**. The test fixture generates ObjectIds, so no test could have caught it — only checking against the live database did.
- **The advice would have been invisible for every existing reading.** An empty token was used as a guard, so the write was conditioned on a token that does not exist.
- A slow background thread could overwrite a correction with advice about the old value.
- An unsynchronised race built eight MongoClients on a cold start.
## Test plan
- [x] 182 tests pass
- [x] New file `tests/test_brew_advice.py`: the prompt content, the marker handling, the token guard, the injection guard, the stale-advice races, and all three pages
- [x] The reading is saved even when the model is down
- [x] End-to-end against the live model: reading recorded → advice generated → stored in Mongo → rendered on the page
- [x] Verified against the live database throughout — every probe restored exactly
- [x] **Deployed**: CI run 1699 all green; verified on the live site
Stores a model's advice for each recorded reading, in its own field, so the
brewer's own comment is never overwritten.
Fire and forget: the advice is generated in a background thread, so
recording a reading never blocks on the model. A failure is recorded as a
visible note saying the advice could not be generated, because a silent
failure is indistinguishable from "still thinking".
The model is `glm-5.3:cloud` on the Ollama instance at 192.168.178.45:11434.
Several models on that instance are retired and fail at call time while
still appearing in the model list, so the default is one that was probed.
The system prompt tells the model never to call a reading impossible: a
gravity below 1.000 is always a mis-entry, but the rest of the run may be a
genuine long fermentation and still worth understanding.
150 tests.
This PR introduces an asynchronous integration with an Ollama-hosted LLM to provide automated brewing advice. While the functional implementation is well-documented and the UI/UX considerations are strong, the current architectural approach to concurrency and network dependency introduces significant stability and security risks.
The primary concern is the "fire and forget" threading model used within a Flask environment, which lacks the robustness of a managed task queue. Furthermore, the integration lacks critical safeguards against indirect prompt injection and resource exhaustion (timeouts). While the PR is a strong functional start, it requires architectural refinements before it can be considered production-ready.
2. Prioritized Issues List
🔴 CRITICAL
Unbounded Resource Exhaustion (DoS): The use of threading.Thread per request without a bounded queue or strict timeouts can lead to memory exhaustion and MongoDB connection leaks.
Infrastructure Brittleness: Reliance on static internal IPs rather than service discovery (DNS) makes the deployment fragile and non-portable.
🟠 HIGH
Indirect Prompt Injection: User-generated comments are passed to the LLM without delimiting, potentially allowing users to manipulate the AI output or attempt XSS via the AI's response.
Database Connection Leakage: Instantiating a new MongoDB client inside every background thread without an explicit closure strategy will lead to socket exhaustion.
🟡 MEDIUM
Lack of Task Persistence: Since raw threads are used, any pending LLM requests are lost instantly if the Flask worker process recycles or the pod restarts.
Missing Observability: While the UI shows a failure flag (advice_ok=False), there is no structured logging or metrics to alert the platform team of systemic LLM failures.
Unvalidated LLM Output: There is no maximum length validation on the response stored in MongoDB, risking database bloat if the model hallucinations.
🔵 LOW
UI Refresh UX: The current "fire and forget" nature requires a manual page refresh to see the generated advice.
3. Domain-Specific Recommendations
💻 Code Quality & Design
Transition to Task Queue: Replace threading.Thread with a lightweight task queue such as Celery or RQ (Redis Queue). This decouples the web request from the LLM processing and provides retry logic.
Implement Strict Timeouts: Every call to the Ollama API must have a defined connect and read timeout (e.g., 30s) to prevent "hanging" threads.
Connection Pooling: Move away from creating a new Mongo client per thread. Use a shared client pool or a singleton pattern managed outside the request scope.
🛡️ Security
Input Delimiting: Wrap user observations in clear delimiters (e.g., ### USER INPUT: {comment} ###) and update the system prompt to explicitly ignore instructions contained within those boundaries.
Output Validation: Implement a hard character limit (e.g., 2000 characters) on the LLM response before saving it to the database.
Egress Filtering: Implement Kubernetes NetworkPolicy to restrict the Flask pod's egress traffic strictly to the Ollama service IP/Port.
⚙️ Infrastructure & DevOps
Service Discovery: Replace the hardcoded IP (192.168.178.45) with a Kubernetes DNS name (e.g., ollama.llm-namespace.svc.cluster.local).
Secret Management: Ensure OLLAMA_URL and OLLAMA_MODEL are injected via K8s Secrets or ConfigMaps, not hardcoded in manifests.
Monitoring: Add a Prometheus counter (e.g., llm_request_failures_total) to track the failure rate of the advice generation service.
4. Positive Aspects of the PR
Despite the architectural concerns, the following points are highly commended:
Excellent Documentation: The PR description is thorough, and the test plan is comprehensive, covering edge cases like empty replies and unreachable models.
Thoughtful Data Modeling: Separating comment from advice and using an advice_ok flag prevents user confusion and maintains data integrity.
Frontend Security: The explicit decision to avoid |safe in Jinja2 templates shows a proactive approach to preventing XSS.
Domain Awareness: The prompt engineering specifically addresses known domain errors (readings below 1.000), showing high attention to detail.
5. Overall Assessment & Recommendation
Rating: ⚠️ SIGNIFICANT CHANGES NEEDED
Justification:
While the feature is functionally complete and well-tested, the concurrency model (raw threads) and network configuration (static IP) are not suitable for a production environment. These represent "ticking time bombs" that could lead to application crashes or security vulnerabilities under load.
Required Actions for Approval:
Migrate from threading.Thread to a persisted task queue (Celery/RQ).
Implement strict API timeouts and response length validation.
Replace static IP references with DNS-based service discovery.
Implement input delimiting in the LLM prompt to mitigate prompt injection.
Verify that MongoDB connections are properly pooled or closed.
## PR Review Results
# Consolidated Review Report: LLM Brewing Advice Integration
## 1. Executive Summary
This PR introduces an asynchronous integration with an Ollama-hosted LLM to provide automated brewing advice. While the functional implementation is well-documented and the UI/UX considerations are strong, the current architectural approach to concurrency and network dependency introduces significant stability and security risks.
The primary concern is the "fire and forget" threading model used within a Flask environment, which lacks the robustness of a managed task queue. Furthermore, the integration lacks critical safeguards against indirect prompt injection and resource exhaustion (timeouts). While the PR is a strong functional start, it requires architectural refinements before it can be considered production-ready.
---
## 2. Prioritized Issues List
### 🔴 CRITICAL
* **Unbounded Resource Exhaustion (DoS):** The use of `threading.Thread` per request without a bounded queue or strict timeouts can lead to memory exhaustion and MongoDB connection leaks.
* **Infrastructure Brittleness:** Reliance on static internal IPs rather than service discovery (DNS) makes the deployment fragile and non-portable.
### 🟠 HIGH
* **Indirect Prompt Injection:** User-generated comments are passed to the LLM without delimiting, potentially allowing users to manipulate the AI output or attempt XSS via the AI's response.
* **Database Connection Leakage:** Instantiating a new MongoDB client inside every background thread without an explicit closure strategy will lead to socket exhaustion.
### 🟡 MEDIUM
* **Lack of Task Persistence:** Since raw threads are used, any pending LLM requests are lost instantly if the Flask worker process recycles or the pod restarts.
* **Missing Observability:** While the UI shows a failure flag (`advice_ok=False`), there is no structured logging or metrics to alert the platform team of systemic LLM failures.
* **Unvalidated LLM Output:** There is no maximum length validation on the response stored in MongoDB, risking database bloat if the model hallucinations.
### 🔵 LOW
* **UI Refresh UX:** The current "fire and forget" nature requires a manual page refresh to see the generated advice.
---
## 3. Domain-Specific Recommendations
### 💻 Code Quality & Design
* **Transition to Task Queue:** Replace `threading.Thread` with a lightweight task queue such as **Celery or RQ (Redis Queue)**. This decouples the web request from the LLM processing and provides retry logic.
* **Implement Strict Timeouts:** Every call to the Ollama API must have a defined `connect` and `read` timeout (e.g., 30s) to prevent "hanging" threads.
* **Connection Pooling:** Move away from creating a new Mongo client per thread. Use a shared client pool or a singleton pattern managed outside the request scope.
### 🛡️ Security
* **Input Delimiting:** Wrap user observations in clear delimiters (e.g., `### USER INPUT: {comment} ###`) and update the system prompt to explicitly ignore instructions contained within those boundaries.
* **Output Validation:** Implement a hard character limit (e.g., 2000 characters) on the LLM response before saving it to the database.
* **Egress Filtering:** Implement Kubernetes `NetworkPolicy` to restrict the Flask pod's egress traffic strictly to the Ollama service IP/Port.
### ⚙️ Infrastructure & DevOps
* **Service Discovery:** Replace the hardcoded IP (`192.168.178.45`) with a Kubernetes DNS name (e.g., `ollama.llm-namespace.svc.cluster.local`).
* **Secret Management:** Ensure `OLLAMA_URL` and `OLLAMA_MODEL` are injected via K8s Secrets or ConfigMaps, not hardcoded in manifests.
* **Monitoring:** Add a Prometheus counter (e.g., `llm_request_failures_total`) to track the failure rate of the advice generation service.
---
## 4. Positive Aspects of the PR
Despite the architectural concerns, the following points are highly commended:
* **Excellent Documentation:** The PR description is thorough, and the test plan is comprehensive, covering edge cases like empty replies and unreachable models.
* **Thoughtful Data Modeling:** Separating `comment` from `advice` and using an `advice_ok` flag prevents user confusion and maintains data integrity.
* **Frontend Security:** The explicit decision to avoid `|safe` in Jinja2 templates shows a proactive approach to preventing XSS.
* **Domain Awareness:** The prompt engineering specifically addresses known domain errors (readings below 1.000), showing high attention to detail.
---
## 5. Overall Assessment & Recommendation
**Rating: ⚠️ SIGNIFICANT CHANGES NEEDED**
**Justification:**
While the feature is functionally complete and well-tested, the **concurrency model (raw threads)** and **network configuration (static IP)** are not suitable for a production environment. These represent "ticking time bombs" that could lead to application crashes or security vulnerabilities under load.
**Required Actions for Approval:**
1. **Migrate** from `threading.Thread` to a persisted task queue (Celery/RQ).
2. **Implement** strict API timeouts and response length validation.
3. **Replace** static IP references with DNS-based service discovery.
4. **Implement** input delimiting in the LLM prompt to mitigate prompt injection.
5. **Verify** that MongoDB connections are properly pooled or closed.
Two things I wanted to be sure of before this merges.
Stale advice
------------
The advice is written for the gravity that was there when it was
generated. If the brewer then edits the reading - to fix a typo, or to
mark it final - the old advice no longer describes it, and the page was
showing advice that contradicted the number next to it.
`upsert_reading` now clears the advice fields, so the stale advice goes
and a fresh one is generated (the form already regenerates on every save).
Only the advice fields are cleared - the brewer's own comment is never
touched.
The injection guard
-------------------
The reading's `_id` is an ObjectId, so the original `_as_id` coercion
(which turns run ids into plain strings, to stop a caller passing a dict
that Mongo would interpret as a query operator) did not apply - it turned
the ObjectId into an empty string and the update silently matched
nothing. `_as_doc_id` accepts only an ObjectId or a string that parses as
one.
Verified against a live injection attempt: `{"$ne": None}`, `{"$gt": ""}`,
a list, an int, None and a non-ObjectId string all return None and match
nothing. There is now a test that pins it, because it is a security
property that would otherwise only be covered by reading the code.
160 tests (up from 158).
The review of PR #14 found a critical defect and two real bugs. All
verified against the live database, not just read.
CRITICAL - the advice would never have been stored
--------------------------------------------------
`_as_doc_id` accepted only an ObjectId or a string that parses as one.
But **all 47 live readings have string `_id`s** - 24 random characters
from the original app's importer, like `l78ZidBJ3QaYn98tvB88x4uM`. The
helper turned every one of them into None, so the update matched nothing
and the advice was silently never stored in production.
The mongomock fixture generates ObjectIds, so the tests could not see it.
`_as_doc_id` now returns both forms and the query uses `$in` (a plain
list would be an exact ARRAY match in Mongo, which is not what is
wanted). Verified against the live database: the advice now lands on a
real reading, with the brewer's comment untouched.
There are now tests for BOTH id shapes, with the string shape built
explicitly, because the fixture would not produce it.
The advice was written about the wrong reading
----------------------------------------------
`latest` was taken from the date-sorted tail of the run, so editing an
OLDER reading generated advice about the NEWEST one. The advice is now
written against the reading that was actually saved.
A leaked MongoClient per reading
--------------------------------
The background thread built a `BeerRepository()` - and therefore a
`MongoClient` - for every advice write, and never closed it. A
MongoClient spawns three monitor threads and pymongo does NOT reap them
on `close()`; they go only when the client is garbage collected.
Measured: three threads and a connection pool accumulated per reading
recorded. There is now one shared repository for the life of the
process, which is what the request-scoped repository does too. Measured
again after the fix: 4 threads total, not 16 over five writes.
Also
----
- `BeerRepository` gained `close()` and the context-manager protocol.
- The advice writer now logs instead of dying silently if the write
fails.
164 tests (up from 160).
Thanks — the critical finding was right, and it was mine.
CRITICAL: _as_doc_id rejected every live reading _id
All 47 live readings have string _ids — 24 random characters from the original app's importer (l78ZidBJ3QaYn98tvB88x4uM), not ObjectIds. My helper turned every one of them into None, so the update matched nothing and the advice would have been silently never stored in production.
The mongomock fixture generates ObjectIds, which is why the tests couldn't see it. _as_doc_id now returns both forms and the query uses $in (a plain list is an exact array match in Mongo, which is not what was wanted).
Verified against the live database:
live _id: 'l78ZidBJ3QaYn98tvB88x4uM' | _as_doc_id -> ['l78ZidBJ3QaYn98tvB88x4uM']
modified: 1
advice now: 'TEST ADVICE ON A LIVE READING'
comment intact: 'First ever try'
There are now tests for both id shapes, with the string shape built explicitly — because the fixture would not produce it.
The advice was written about the wrong reading
latest came from the date-sorted tail of the run, so editing an older reading generated advice about the newest one. The advice is now written against the reading that was actually saved.
The leaked MongoClient
You measured three threads and a pool per reading — that's right, and it's worse than a plain leak: pymongo does not reap its monitor threads on close(); they go only when the client is garbage collected, so close() alone did nothing.
Measured after the fix — 4 threads total, not 16 over five writes:
before: 1
after 5 writes on the shared repo: 4
There is now one shared repository for the life of the process, which is what the request-scoped repository already did. BeerRepository also gained close() and the context-manager protocol.
Also
The advice writer logs instead of dying silently if the write fails.
Stale advice is cleared when a reading is edited, so the page never shows advice that contradicts the number next to it. Only the advice fields are cleared — the brewer's comment is never touched.
Verified end to end against the live database
Reading recorded through the app → advice generated in the background → stored in Mongo → rendered on the page, with the comment intact:
=== the stored document:
_id: 6af1683be2adf0fc71c48b6b
beer_run_id: 903
comment: live check
sg: 1042
advice: Your only reading matches the original gravity exactly, so as of 2026-11-09 this beer is completely unfermente…
advice_at: 2026-11-09T06:29:24+00:00
advice_ok: True
(Test run cleaned up — 47 readings, as before.)
Your other checks
XSS: confirmed — advice is escaped by Jinja on all three pages, no |safe/mark_safe/innerHTML sink, CSP stays script-src 'self'.
Prompt injection: your test is a good one — the model called the pasted instruction out as junk rather than following it. I agree with your read.
Run 12: you confirmed the advice doesn't say "impossible" — that's exactly what the prompt instruction was for.
Test plan
164 tests pass (up from 160)
New: both id shapes, the shared-client check, the advice being about the reading that was saved, the injection guard, the stale-advice clearing
Live-database verification of the id fix, the stored advice and the thread count
Not yet deployed — the workflow deploys from master on merge; I will confirm the rollout afterwards.
Thanks — the critical finding was right, and it was mine.
### CRITICAL: `_as_doc_id` rejected every live reading `_id`
**All 47 live readings have string `_id`s** — 24 random characters from the original app's importer (`l78ZidBJ3QaYn98tvB88x4uM`), not ObjectIds. My helper turned every one of them into `None`, so the update matched nothing and **the advice would have been silently never stored in production**.
The mongomock fixture generates ObjectIds, which is why the tests couldn't see it. `_as_doc_id` now returns both forms and the query uses `$in` (a plain list is an exact *array* match in Mongo, which is not what was wanted).
Verified against the live database:
```
live _id: 'l78ZidBJ3QaYn98tvB88x4uM' | _as_doc_id -> ['l78ZidBJ3QaYn98tvB88x4uM']
modified: 1
advice now: 'TEST ADVICE ON A LIVE READING'
comment intact: 'First ever try'
```
There are now tests for **both** id shapes, with the string shape built explicitly — because the fixture would not produce it.
### The advice was written about the wrong reading
`latest` came from the date-sorted tail of the run, so editing an **older** reading generated advice about the **newest** one. The advice is now written against the reading that was actually saved.
### The leaked MongoClient
You measured three threads and a pool per reading — that's right, and it's worse than a plain leak: pymongo does **not** reap its monitor threads on `close()`; they go only when the client is garbage collected, so `close()` alone did nothing.
Measured after the fix — 4 threads total, not 16 over five writes:
```
before: 1
after 5 writes on the shared repo: 4
```
There is now one shared repository for the life of the process, which is what the request-scoped repository already did. `BeerRepository` also gained `close()` and the context-manager protocol.
### Also
- The advice writer logs instead of dying silently if the write fails.
- Stale advice is cleared when a reading is edited, so the page never shows advice that contradicts the number next to it. Only the advice fields are cleared — the brewer's comment is never touched.
## Verified end to end against the live database
Reading recorded through the app → advice generated in the background → stored in Mongo → rendered on the page, with the comment intact:
```
=== the stored document:
_id: 6af1683be2adf0fc71c48b6b
beer_run_id: 903
comment: live check
sg: 1042
advice: Your only reading matches the original gravity exactly, so as of 2026-11-09 this beer is completely unfermente…
advice_at: 2026-11-09T06:29:24+00:00
advice_ok: True
```
(Test run cleaned up — 47 readings, as before.)
## Your other checks
- **XSS:** confirmed — advice is escaped by Jinja on all three pages, no `|safe`/`mark_safe`/`innerHTML` sink, CSP stays `script-src 'self'`.
- **Prompt injection:** your test is a good one — the model called the pasted instruction out as junk rather than following it. I agree with your read.
- **Run 12:** you confirmed the advice doesn't say "impossible" — that's exactly what the prompt instruction was for.
## Test plan
- [x] 164 tests pass (up from 160)
- [x] New: both id shapes, the shared-client check, the advice being about the reading that was saved, the injection guard, the stale-advice clearing
- [x] Live-database verification of the id fix, the stored advice and the thread count
- [ ] **Not yet deployed** — the workflow deploys from `master` on merge; I will confirm the rollout afterwards.
_as_doc_id returned None for a hostile id. That was guarded at the one
call site, but the helper could still be misused into "$in needs an
array" - and a caller who forgot the guard would have got an error
rather than a safe no-op.
It now returns an EMPTY LIST for anything that is not a reading id. An
empty list is a valid $in argument that matches nothing, so the helper
cannot produce a query error, cannot match every document, and cannot be
interpreted as a query operator.
Verified against the live database: a hostile id matches 0 documents and
updates 0 documents.
164 tests.
Two findings from the re-review of PR #14.
A race that recreated the leak
-------------------------------
`get_shared_repository()` did an unsynchronised check-then-set on a
module global. Eight concurrent advice threads on a cold process each
saw None and each built their own repository - so eight MongoClients and
twenty-five threads. The fix that was meant to remove the leak
reintroduced it, just bounded by a race instead of by the number of
readings.
Measured before: 8 repositories built, 25 threads.
Measured after: 1 repository built, 4 threads.
There is a lock, and the check is repeated inside it.
The prompt was about the wrong reading
--------------------------------------
The advice targeted the right reading, but the PROMPT said "give advice
for the most recent reading" - and when the brewer corrects an OLDER
reading, the most recent one in the history is a different reading. So
the model was asked about the wrong number while the advice was filed
against the right one.
The reading is now named in the instruction (with its date and gravity),
marked in the reading list as the one to advise on, and the system prompt
says to advise about the marked reading because it may not be the most
recent one.
A corrected reading is also flagged as such, so the model knows it may
differ from what it said before.
167 tests (up from 164).
The model is now `mistral-large-4:cloud`, on the same Ollama instance.
It was probed before being made the default - `deepseek-v4-flash:cloud`,
`deepseek-v3.2:cloud` and `qwen3.5:cloud` are all retired and fail at
call time while still appearing in the model list.
What the probes showed
----------------------
`think: false` does NOT stop the reasoning. It only changes where the
working goes: with thinking off, the model writes it into the CONTENT
instead of a separate field. Either way the reasoning comes out of the
same token allowance as the answer, and at 500-3000 tokens the reply
came back EMPTY - which looks like a broken model rather than a budget
problem. The budget is now 8000, and the timeout 120s.
The reply is also stripped of a reasoning preamble. The advice is the
prose at the END; the working comes first, in several paragraphs. The
preamble is found by walking back from the end of the reply and keeping
the paragraphs that read like advice to the brewer, rather than like
the model thinking out loud.
Verified against the live model, on a stalled brew (OG 1.042, 1.022 after
nine days, "no change since day 7"):
That 1.022 reading after nine days at 19C - with no movement since
day seven - looks like a stalled fermentation; a 1.042 wort should
be well on its way to 1.010-1.012 by now, not sitting at only about
48% attenuation. Gently swirl the fermenter to rouse the yeast cake
and keep the temperature steady at 19-20C, and if the gravity hasn't
budged in another 48 hours, pitch a fresh, healthy yeast starter...
And on a CORRECTED older reading - the case the review found broken -
the advice is about the corrected reading, with the later one as context:
A brew-day reading of 1.038 against an OG of 1.042 is only four points
low, which is almost certainly just the normal difference between
your kettle gravity and the post-transfer fermenter reading...
169 tests (up from 167).
Thanks — the critical finding was right, and it was mine.
CRITICAL: _as_doc_id rejected every live reading _id
All 47 live readings have string _ids — 24 random characters from the original app's importer (l78ZidBJ3QaYn98tvB88x4uM), not ObjectIds. My helper turned every one of them into None, so the update matched nothing and the advice would have been silently never stored in production.
The mongomock fixture generates ObjectIds, which is why the tests couldn't see it. _as_doc_id now returns both forms and the query uses $in (a plain list is an exact array match in Mongo, which is not what was wanted).
Verified against the live database:
live _id: 'l78ZidBJ3QaYn98tvB88x4uM' | _as_doc_id -> ['l78ZidBJ3QaYn98tvB88x4uM']
modified: 1
advice now: 'TEST ADVICE ON A LIVE READING'
comment intact: 'First ever try'
There are now tests for both id shapes, with the string shape built explicitly — because the fixture would not produce it.
The advice was written about the wrong reading
latest came from the date-sorted tail of the run, so editing an older reading generated advice about the newest one. The advice is now written against the reading that was actually saved.
The leaked MongoClient
You measured three threads and a pool per reading — that's right, and it's worse than a plain leak: pymongo does not reap its monitor threads on close(); they go only when the client is garbage collected, so close() alone did nothing.
Measured after the fix — 4 threads total, not 16 over five writes:
before: 1
after 5 writes on the shared repo: 4
There is now one shared repository for the life of the process, which is what the request-scoped repository already did. BeerRepository also gained close() and the context-manager protocol.
Also
The advice writer logs instead of dying silently if the write fails.
Stale advice is cleared when a reading is edited, so the page never shows advice that contradicts the number next to it. Only the advice fields are cleared — the brewer's comment is never touched.
Verified end to end against the live database
Reading recorded through the app → advice generated in the background → stored in Mongo → rendered on the page, with the comment intact:
=== the stored document:
_id: 6af1683be2adf0fc71c48b6b
beer_run_id: 903
comment: live check
sg: 1042
advice: Your only reading matches the original gravity exactly, so as of 2026-11-09 this beer is completely unfermente…
advice_at: 2026-11-09T06:29:24+00:00
advice_ok: True
(Test run cleaned up — 47 readings, as before.)
Your other checks
XSS: confirmed — advice is escaped by Jinja on all three pages, no |safe/mark_safe/innerHTML sink, CSP stays script-src 'self'.
Prompt injection: your test is a good one — the model called the pasted instruction out as junk rather than following it. I agree with your read.
Run 12: you confirmed the advice doesn't say "impossible" — that's exactly what the prompt instruction was for.
Test plan
164 tests pass (up from 160)
New: both id shapes, the shared-client check, the advice being about the reading that was saved, the injection guard, the stale-advice clearing
Live-database verification of the id fix, the stored advice and the thread count
Not yet deployed — the workflow deploys from master on merge; I will confirm the rollout afterwards.
Update: the model is now mistral-large-4:cloud
Your suggestion, and it was right — the reasoning models were the problem.
think: false does not stop the reasoning. It only changes where the working goes: with thinking off, the model writes it into the content instead of a separate field. Either way the reasoning comes out of the same token allowance as the answer, and at 500–3000 tokens the reply came back empty — which looks like a broken model rather than a budget problem.
So:
The budget is now 8000 tokens and the timeout 120s.
The reply is stripped of a reasoning preamble. The advice is the prose at the end; the working comes first, in several paragraphs. The preamble is found by walking back from the end and keeping the paragraphs that read like advice to the brewer rather than like the model thinking out loud.
Verified against the live model on a stalled brew (OG 1.042, 1.022 after nine days, "no change since day 7"):
That 1.022 reading after nine days at 19°C—with no movement since day seven—looks like a stalled fermentation; a 1.042 wort should be well on its way to 1.010–1.012 by now, not sitting at only about 48% attenuation. Gently swirl the fermenter to rouse the yeast cake and keep the temperature steady at 19–20°C, and if the gravity hasn't budged in another 48 hours, pitch a fresh, healthy yeast starter (rehydrated dry ale yeast is fine) to finish it down to roughly 1.008–1.012 before you rack or add any dry hops.
And on a corrected older reading — the case your review found broken — the advice is about the corrected reading, with the later one as context:
A brew-day reading of 1.038 against an OG of 1.042 is only four points low, which is almost certainly just the normal difference between your kettle gravity and the post-transfer fermenter reading—trub loss or any top-up water easily accounts for that—so treat 1.038 as your true baseline rather than a typo.
Also fixed since your review
A race in get_shared_repository(). An unsynchronised check-then-set let eight concurrent threads each build their own repository — 8 clients, 25 threads. Now guarded by a lock, with the check repeated inside it. Measured after: 1 repository, 4 threads.
The prompt named the wrong reading. It said "give advice for the most recent reading", but when the brewer corrects an older reading the most recent one is a different reading. The reading is now named in the instruction, marked in the list, and a corrected reading is flagged as such.
169 tests.
Thanks — the critical finding was right, and it was mine.
### CRITICAL: `_as_doc_id` rejected every live reading `_id`
**All 47 live readings have string `_id`s** — 24 random characters from the original app's importer (`l78ZidBJ3QaYn98tvB88x4uM`), not ObjectIds. My helper turned every one of them into `None`, so the update matched nothing and **the advice would have been silently never stored in production**.
The mongomock fixture generates ObjectIds, which is why the tests couldn't see it. `_as_doc_id` now returns both forms and the query uses `$in` (a plain list is an exact *array* match in Mongo, which is not what was wanted).
Verified against the live database:
```
live _id: 'l78ZidBJ3QaYn98tvB88x4uM' | _as_doc_id -> ['l78ZidBJ3QaYn98tvB88x4uM']
modified: 1
advice now: 'TEST ADVICE ON A LIVE READING'
comment intact: 'First ever try'
```
There are now tests for **both** id shapes, with the string shape built explicitly — because the fixture would not produce it.
### The advice was written about the wrong reading
`latest` came from the date-sorted tail of the run, so editing an **older** reading generated advice about the **newest** one. The advice is now written against the reading that was actually saved.
### The leaked MongoClient
You measured three threads and a pool per reading — that's right, and it's worse than a plain leak: pymongo does **not** reap its monitor threads on `close()`; they go only when the client is garbage collected, so `close()` alone did nothing.
Measured after the fix — 4 threads total, not 16 over five writes:
```
before: 1
after 5 writes on the shared repo: 4
```
There is now one shared repository for the life of the process, which is what the request-scoped repository already did. `BeerRepository` also gained `close()` and the context-manager protocol.
### Also
- The advice writer logs instead of dying silently if the write fails.
- Stale advice is cleared when a reading is edited, so the page never shows advice that contradicts the number next to it. Only the advice fields are cleared — the brewer's comment is never touched.
## Verified end to end against the live database
Reading recorded through the app → advice generated in the background → stored in Mongo → rendered on the page, with the comment intact:
```
=== the stored document:
_id: 6af1683be2adf0fc71c48b6b
beer_run_id: 903
comment: live check
sg: 1042
advice: Your only reading matches the original gravity exactly, so as of 2026-11-09 this beer is completely unfermente…
advice_at: 2026-11-09T06:29:24+00:00
advice_ok: True
```
(Test run cleaned up — 47 readings, as before.)
## Your other checks
- **XSS:** confirmed — advice is escaped by Jinja on all three pages, no `|safe`/`mark_safe`/`innerHTML` sink, CSP stays `script-src 'self'`.
- **Prompt injection:** your test is a good one — the model called the pasted instruction out as junk rather than following it. I agree with your read.
- **Run 12:** you confirmed the advice doesn't say "impossible" — that's exactly what the prompt instruction was for.
## Test plan
- [x] 164 tests pass (up from 160)
- [x] New: both id shapes, the shared-client check, the advice being about the reading that was saved, the injection guard, the stale-advice clearing
- [x] Live-database verification of the id fix, the stored advice and the thread count
- [ ] **Not yet deployed** — the workflow deploys from `master` on merge; I will confirm the rollout afterwards.
---
## Update: the model is now `mistral-large-4:cloud`
Your suggestion, and it was right — the reasoning models were the problem.
`think: false` does **not** stop the reasoning. It only changes where the working goes: with thinking off, the model writes it into the *content* instead of a separate field. Either way the reasoning comes out of the same token allowance as the answer, and at 500–3000 tokens the reply came back **empty** — which looks like a broken model rather than a budget problem.
So:
- The budget is now **8000** tokens and the timeout **120s**.
- The reply is stripped of a reasoning preamble. The advice is the prose at the *end*; the working comes first, in several paragraphs. The preamble is found by walking back from the end and keeping the paragraphs that read like advice to the brewer rather than like the model thinking out loud.
Verified against the live model on a stalled brew (OG 1.042, 1.022 after nine days, "no change since day 7"):
> That 1.022 reading after nine days at 19°C—with no movement since day seven—looks like a stalled fermentation; a 1.042 wort should be well on its way to 1.010–1.012 by now, not sitting at only about 48% attenuation. Gently swirl the fermenter to rouse the yeast cake and keep the temperature steady at 19–20°C, and if the gravity hasn't budged in another 48 hours, pitch a fresh, healthy yeast starter (rehydrated dry ale yeast is fine) to finish it down to roughly 1.008–1.012 before you rack or add any dry hops.
And on a **corrected older reading** — the case your review found broken — the advice is about the corrected reading, with the later one as context:
> A brew-day reading of 1.038 against an OG of 1.042 is only four points low, which is almost certainly just the normal difference between your kettle gravity and the post-transfer fermenter reading—trub loss or any top-up water easily accounts for that—so treat 1.038 as your true baseline rather than a typo.
### Also fixed since your review
- **A race in `get_shared_repository()`.** An unsynchronised check-then-set let eight concurrent threads each build their own repository — 8 clients, 25 threads. Now guarded by a lock, with the check repeated inside it. Measured after: 1 repository, 4 threads.
- **The prompt named the wrong reading.** It said "give advice for the most recent reading", but when the brewer corrects an *older* reading the most recent one is a different reading. The reading is now named in the instruction, marked in the list, and a corrected reading is flagged as such.
169 tests.
The review found a way to store the model's WORKING instead of its
advice.
The advice is at the END of the reply, so the length cap was cutting
from the front - which took the advice off and left only the working.
That is worse than no advice at all, because it looks like advice: the
brew sees a full paragraph of the model thinking about their beer, and
nothing about what to do.
Reproduced:
input len: 2515 -> final len: 1199
advice present? False
TAIL: 'padding padding padding padding padding'
Two changes:
- The length cap keeps the END of the reply and cuts at a sentence
boundary near the end, so the advice survives.
- The stripper now also catches a reply that is entirely working - no
marker phrase, and no paragraph that reads like advice - instead of
only catching one that says "let me think".
input: 2515 -> stripped: 2515 -> final: 1199
advice present? True
A short reply is untouched.
171 tests (up from 169).
The review found a race between the background advice thread and the
brewer correcting a reading.
`set_reading_advice` wrote unconditionally by id. The advice is
generated in the background, so if the brewer corrects a reading while
the advice for the OLD gravity is still being generated, the slow thread
lands afterwards and overwrites the fresh advice with advice about a
reading that no longer exists. The reading ends up with the final
gravity from the correction and the advice from before it.
Reproduced against the live database:
wrote advice for 1050
wrote advice for 1020
late stale write matched: 0
final sg: 1020 | final advice: 'ADVICE FOR THE CORRECTION'
The write is now conditional on the reading still holding the gravity
the advice was generated for, so a late thread matches nothing.
Also
----
`finish_reason` is now checked. A reply that stopped at the token
ceiling is incomplete - it may be advice that stops mid-sentence - and
it was being stored as if it were the whole answer. It is now marked
as cut off.
173 tests (up from 171).
Thanks — and the MEDIUM was a genuinely good catch. I'd misread which finding was which.
MEDIUM — stale advice overwriting a correction (FIXED)
set_reading_advice wrote unconditionally by _id. The advice is generated in the background, so if the brewer corrects a reading while the advice for the old gravity is still being generated, the slow thread lands afterwards and overwrites the fresh advice.
Reproduced against the live database:
wrote advice for 1050
wrote advice for 1020
late stale write matched: 0
final sg: 1020 | final advice: 'ADVICE FOR THE CORRECTION'
The write is now conditional on the reading still holding the gravity the advice was generated for, so a late thread matches nothing.
LOW — finish_reason is not checked (FIXED)
A reply that stopped at the token ceiling (finish_reason == "length") may be advice that stops mid-sentence, and it was being stored as if it were the whole answer. It is now marked as cut off.
LOW — the reasoning stripper
You were right that it "is a no-op in the case it was written for". Two changes:
The length cap now keeps the end of the reply and cuts at a sentence boundary near the end, so a reply that is entirely working still ends up with the advice in it rather than with the working cut off. Verified: a 2,515-char all-working reply now yields 1,199 chars with the advice present, where before it yielded 1,200 chars of working and no advice at all.
The stripper also catches a reply that is entirely working — no marker phrase, and no paragraph that reads like advice — instead of only catching one that says "let me think".
I've left the marker heuristics in place rather than rewriting them to your paragraph-shape suggestion, on the basis that the current model returns its working in a separate field and 0 of 6 live samples were altered by the stripper. But your point stands that it's fragile defensive code, and the length cap no longer depends on it to preserve the advice.
Both previous findings confirmed fixed by you
The shared-repository race: your 32-thread stress test (1 repository, 1 client, 0 errors) matches my 8-thread test.
The prompt naming the right reading: your 6 live samples on a middle/older reading, and the live DB row check, are exactly the verification I couldn't do from the code alone.
173 tests (up from 169).
Thanks — and the MEDIUM was a genuinely good catch. I'd misread which finding was which.
### MEDIUM — stale advice overwriting a correction (FIXED)
`set_reading_advice` wrote unconditionally by `_id`. The advice is generated in the background, so if the brewer corrects a reading while the advice for the **old** gravity is still being generated, the slow thread lands afterwards and overwrites the fresh advice.
Reproduced against the live database:
```
wrote advice for 1050
wrote advice for 1020
late stale write matched: 0
final sg: 1020 | final advice: 'ADVICE FOR THE CORRECTION'
```
The write is now conditional on the reading still holding the gravity the advice was generated for, so a late thread matches nothing.
### LOW — `finish_reason` is not checked (FIXED)
A reply that stopped at the token ceiling (`finish_reason == "length"`) may be advice that stops mid-sentence, and it was being stored as if it were the whole answer. It is now marked as cut off.
### LOW — the reasoning stripper
You were right that it "is a no-op in the case it was written for". Two changes:
- The length cap now keeps the **end** of the reply and cuts at a sentence boundary near the end, so a reply that is entirely working still ends up with the advice in it rather than with the working cut off. Verified: a 2,515-char all-working reply now yields 1,199 chars **with the advice present**, where before it yielded 1,200 chars of working and no advice at all.
- The stripper also catches a reply that is entirely working — no marker phrase, and no paragraph that reads like advice — instead of only catching one that says "let me think".
I've left the marker heuristics in place rather than rewriting them to your paragraph-shape suggestion, on the basis that the current model returns its working in a separate field and 0 of 6 live samples were altered by the stripper. But your point stands that it's fragile defensive code, and the length cap no longer depends on it to preserve the advice.
### Both previous findings confirmed fixed by you
- The shared-repository race: your 32-thread stress test (1 repository, 1 client, 0 errors) matches my 8-thread test.
- The prompt naming the right reading: your 6 live samples on a middle/older reading, and the live DB row check, are exactly the verification I couldn't do from the code alone.
173 tests (up from 169).
Four things from the earlier review of this PR.
The prompt test did not test the prompt
---------------------------------------
`test_the_advice_is_about_the_reading_that_was_saved` asserted which
dict was PASSED to the generator - not what the model was actually ASKED.
So it passed even while the prompt still said "give advice for the most
recent reading", which is the bug that made the model advise about the
wrong number while the advice was filed against the right one.
It now asserts on the prompt content: the reading is marked, its date is
named, and "most recent reading" is gone.
Verified by reverting the prompt fix - the test fails, and so does
`test_the_prompt_names_the_reading_to_advise_about`. Two tests now
guard it, and reverting the fix breaks both.
A write that matches nothing was silent
--------------------------------------
The advice writer discarded the return value of `set_reading_advice`. A
write that matched 0 documents - which is now the EXPECTED outcome when
the brewer corrected the reading while the advice was being generated -
was dropped without a word. It is logged now.
A stale docstring
-----------------
The background thread "builds its own" client - it has used the shared
one since the leak was fixed.
A missing CSS class
-------------------
`advice--reading` is used on the active-beers card and the run detail
page, and was never defined, so it had no styles.
173 tests (unchanged - the new assertions were added to existing tests).
The gravity guard caught a correction that changed the gravity, but a
COMMENT-ONLY correction leaves the gravity unchanged - so the late
thread for the old advice still matched and overwrote the fresh one.
Reproduced against the live database:
fresh advice matched: 1
stale thread matched: 1 <-- should be 0
FINAL advice: 'STALE ADVICE'
Every edit now writes a fresh token onto the reading, and the advice
thread carries the token it was given. A late thread finds a token that
no longer matches, so its write is refused. The gravity is still
checked as well.
Verified against the live database:
token changed: True
fresh advice matched: 1
stale thread matched: 0 <-- must be 0
FINAL advice: 'FRESH ADVICE'
174 tests (up from 173).
One more round of findings addressed — including the comment-only correction case, which the gravity guard missed.
The comment-only correction (MEDIUM)
The gravity guard caught a correction that changed the gravity, but a comment-only correction leaves the gravity unchanged — so the late thread for the old advice still matched and overwrote the fresh one. You were right to push on this.
Reproduced against the live database before the fix:
fresh advice matched: 1
stale thread matched: 1 <-- should be 0
FINAL advice: 'STALE ADVICE'
Every edit now writes a fresh editing token onto the reading, and the advice thread carries the token it was given. A late thread finds a token that no longer matches, so its write is refused. The gravity is still checked as well.
token changed: True
fresh advice matched: 1
stale thread matched: 0 <-- must be 0
FINAL advice: 'FRESH ADVICE'
The four LOW items
The prompt test did not test the prompt.test_the_advice_is_about_the_reading_that_was_saved asserted which dict was passed to the generator, not what the model was asked. So it passed while the prompt still said "give advice for the most recent reading". It now asserts on the prompt content — the reading is marked, its date is named, and "most recent reading" is gone. I verified it catches the bug by reverting the prompt fix: two tests fail, and both pass again with the fix restored.
A write that matched nothing was silent. The writer discarded the return value. That matters now, because a 0-result write is the expected outcome when the brewer corrected the reading mid-generation. It is logged now, with a message that says why.
A stale docstring — the background thread "builds its own" client; it has used the shared one since the leak was fixed.
A missing CSS class — advice--reading is used on the active-beers card and the run detail page and was never defined.
The sg type question you were asked to check
All 47 live readings store sg as a string, and upsert_reading writes str(sg) — so the conditional write's {"sg": str(expected_sg)} matches, and the normal path is not broken. The token is now the primary guard, with the gravity as a secondary check.
174 tests (up from 173).
One more round of findings addressed — including the comment-only correction case, which the gravity guard missed.
### The comment-only correction (MEDIUM)
The gravity guard caught a correction that changed the gravity, but a **comment-only** correction leaves the gravity unchanged — so the late thread for the old advice still matched and overwrote the fresh one. You were right to push on this.
Reproduced against the live database before the fix:
```
fresh advice matched: 1
stale thread matched: 1 <-- should be 0
FINAL advice: 'STALE ADVICE'
```
Every edit now writes a fresh **editing token** onto the reading, and the advice thread carries the token it was given. A late thread finds a token that no longer matches, so its write is refused. The gravity is still checked as well.
```
token changed: True
fresh advice matched: 1
stale thread matched: 0 <-- must be 0
FINAL advice: 'FRESH ADVICE'
```
### The four LOW items
- **The prompt test did not test the prompt.** `test_the_advice_is_about_the_reading_that_was_saved` asserted which dict was *passed* to the generator, not what the model was *asked*. So it passed while the prompt still said "give advice for the most recent reading". It now asserts on the prompt content — the reading is marked, its date is named, and "most recent reading" is gone. I verified it catches the bug by reverting the prompt fix: **two** tests fail, and both pass again with the fix restored.
- **A write that matched nothing was silent.** The writer discarded the return value. That matters now, because a 0-result write is the *expected* outcome when the brewer corrected the reading mid-generation. It is logged now, with a message that says why.
- **A stale docstring** — the background thread "builds its own" client; it has used the shared one since the leak was fixed.
- **A missing CSS class** — `advice--reading` is used on the active-beers card and the run detail page and was never defined.
### The `sg` type question you were asked to check
All 47 live readings store `sg` as a **string**, and `upsert_reading` writes `str(sg)` — so the conditional write's `{"sg": str(expected_sg)}` matches, and the normal path is not broken. The token is now the primary guard, with the gravity as a secondary check.
174 tests (up from 173).
Two findings from the fourth review.
The stripper was a no-op in the case it was written for
---------------------------------------------------
The "all working" branch returned the text UNCHANGED, which is
byte-identical to the old fallthrough. And in the case where a preamble
marker WAS present and no paragraph read like advice, it returned the
whole reply - working included - so the branch made the "store the
working instead of the advice" failure MORE likely, not less.
Measured on a realistic 5-paragraph reasoning reply: the old code stored
80 chars (one paragraph), the new code stored 612 chars (five
paragraphs) of the model's working.
It now DROPS the working, so the reply is empty and the brewer sees a
clear "could not be generated" rather than the model thinking about
their beer.
"keep" alone is not an advice marker
------------------------------------
"I need to keep in mind that the hydrometer was read at a different
temperature" is the model thinking, not advice. The bare words "keep",
"raise", "lower", "take the" were matching ordinary reasoning phrasing,
so the "all working" detection almost never fired on real reasoning
text. They are now matched as phrases - "you should keep", "raise it
to", "take another reading" - and "i need to keep in mind" is
explicitly working.
The token is the primary guard
------------------------------
The gravity can be unparseable for some readings, so `expected_sg` can
be None - and then the write was becoming unconditional, reintroducing
the stale-overwrite bug on those readings.
The editing token is the primary guard: it changes on EVERY edit, so it
catches a stale generation whatever else changed - the gravity, the
comment, or nothing. The gravity is now a secondary check, used only
when there is no token.
177 tests (up from 174).
Two more findings fixed — and you were right about the stripper being worse than before.
The stripper was a no-op in the case it was written for
You caught this precisely. The "all working" branch returned the text unchanged, which is byte-identical to the old fallthrough — and in the case where a preamble marker was present and no paragraph read like advice, it returned the whole reply, working included. Your measurement (old: 80 chars stored; new: 612 chars of working) matches what I saw when I re-ran it.
It now drops the working, so the reply is empty and the brewer gets a clear "could not be generated" rather than the model thinking about their beer:
all-working reply -> '' (must be empty, not the working)
"keep" alone is not an advice marker
"I need to keep in mind that the hydrometer was read at a different temperature" was classifying as advice, so the "all working" detection almost never fired on real reasoning text. The bare words "keep", "raise", "lower", "take the" are now matched as phrases — "you should keep", "raise it to", "take another reading" — and "i need to keep in mind" is explicitly working.
'keep in mind' classified as advice? False
And a reply with real advice still keeps it:
advice survived? True
The token is the primary guard
You noted that expected_sg is None for an unparseable gravity — one live reading has sg: '900' — and that the guard was then skipped entirely, making the write unconditional on exactly the readings most likely to be wrong.
The editing token is now the primary guard: it changes on every edit, so it catches a stale generation whatever else changed — the gravity, the comment, or nothing. The gravity is a secondary check, used only when there is no token.
That also covers the comment-only case from your previous round, which the gravity guard alone missed.
Verified end to end again after these changes
reading recorded
ADVICE after ~14s: Since this 1.040 reading matches your original gravity exactly, it is simply the pre-fermentation baseline and nothing is wrong yet...
177 tests (up from 174).
Two more findings fixed — and you were right about the stripper being worse than before.
### The stripper was a no-op in the case it was written for
You caught this precisely. The "all working" branch returned the text **unchanged**, which is byte-identical to the old fallthrough — and in the case where a preamble marker *was* present and no paragraph read like advice, it returned the whole reply, working included. Your measurement (old: 80 chars stored; new: 612 chars of working) matches what I saw when I re-ran it.
It now **drops** the working, so the reply is empty and the brewer gets a clear "could not be generated" rather than the model thinking about their beer:
```
all-working reply -> '' (must be empty, not the working)
```
### "keep" alone is not an advice marker
`"I need to keep in mind that the hydrometer was read at a different temperature"` was classifying as advice, so the "all working" detection almost never fired on real reasoning text. The bare words "keep", "raise", "lower", "take the" are now matched as **phrases** — "you should keep", "raise it to", "take another reading" — and "i need to keep in mind" is explicitly working.
```
'keep in mind' classified as advice? False
```
And a reply with real advice still keeps it:
```
advice survived? True
```
### The token is the primary guard
You noted that `expected_sg` is `None` for an unparseable gravity — one live reading has `sg: '900'` — and that the guard was then skipped entirely, making the write unconditional on exactly the readings most likely to be wrong.
The editing token is now the **primary** guard: it changes on *every* edit, so it catches a stale generation whatever else changed — the gravity, the comment, or nothing. The gravity is a secondary check, used only when there is no token.
That also covers the comment-only case from your previous round, which the gravity guard alone missed.
### Verified end to end again after these changes
```
reading recorded
ADVICE after ~14s: Since this 1.040 reading matches your original gravity exactly, it is simply the pre-fermentation baseline and nothing is wrong yet...
```
177 tests (up from 174).
None of the 47 existing readings have a token - they all predate it. A
reading read from the database therefore has an EMPTY token, and the
guard was checking `expected_token is not None`, which is TRUE for an
empty string. So the write was conditioned on a token that does not
exist, matched nothing, and **the advice would never have been written
for any of the existing readings**.
Verified against the live database, before the fix:
write with expected_token='' -> 0 (0 = the advice would NEVER be written)
After:
write for a pre-token live reading: 1 (must be 1)
An empty token is now not a usable guard - there is no token to match -
so the write falls through to the gravity check instead. A non-empty
token is still the primary guard.
There is a test for it, with a reading inserted the way the existing
ones are - directly, with no token.
178 tests (up from 177).
One more, and this one was serious — the advice would have been invisible for every existing reading.
An empty token must not be used as a guard
None of the 47 existing readings have a token — they all predate it. So a reading read from the database has an empty token, and the guard was checking expected_token is not None, which is True for an empty string.
So the write was conditioned on a token that does not exist, matched nothing, and the advice would never have been written for any of the existing readings.
Verified against the live database, before the fix:
write with expected_token='' -> 0 (0 = the advice would NEVER be written)
After:
write for a pre-token live reading: 1 (must be 1)
An empty token is now not a usable guard — there is no token to match — so the write falls through to the gravity check. A non-empty token is still the primary guard.
There is a test with a reading inserted the way the existing ones are: directly, with no token.
End to end again after these changes
reading recorded
ADVICE after ~10s: That 1.040 on November 9th is simply your original gravity unchanged, meaning fermentation hasn't started yet or you measured right at pitching. This is completely normal for a first reading, so give the yeast 24 to 48 hours at fermentation temperature...
178 tests (up from 177).
One more, and this one was serious — the advice would have been invisible for every existing reading.
### An empty token must not be used as a guard
None of the 47 existing readings have a token — they all predate it. So a reading read from the database has an **empty** token, and the guard was checking `expected_token is not None`, which is `True` for an empty string.
So the write was conditioned on a token that does not exist, matched nothing, and **the advice would never have been written for any of the existing readings**.
Verified against the live database, before the fix:
```
write with expected_token='' -> 0 (0 = the advice would NEVER be written)
```
After:
```
write for a pre-token live reading: 1 (must be 1)
```
An empty token is now not a usable guard — there is no token to match — so the write falls through to the gravity check. A non-empty token is still the primary guard.
There is a test with a reading inserted the way the existing ones are: directly, with no token.
### End to end again after these changes
```
reading recorded
ADVICE after ~10s: That 1.040 on November 9th is simply your original gravity unchanged, meaning fermentation hasn't started yet or you measured right at pitching. This is completely normal for a first reading, so give the yeast 24 to 48 hours at fermentation temperature...
```
178 tests (up from 177).
Two findings from the fifth review.
The pre-token fallback re-opened the hole
-----------------------------------------
A reading that has never been edited has no token. The guard fell back
to the gravity check for those, and a comment-only correction leaves the
gravity unchanged - so a late thread's write matched and overwrote the
fresh advice, for exactly the 47 existing readings.
Reproduced live before the fix:
pre-token reading token: None -> stale thread carries ""
fresh write modified: 1
stale write modified: 1 <<< stale advice overwrote the fresh one
final advice: 'STALE ADVICE'
The guard now matches on the ABSENCE of a token, which still tells the
generations apart: an edit writes a token, so a stale thread's "" stops
matching the moment the reading is corrected.
fresh write modified: 1 (must be 1)
stale write modified: 0 (must be 0)
The stripper could empty real advice
------------------------------------
"Actually the drop is fine.\\n\\nWait a couple of days and check again."
is advice, and judging it paragraph by paragraph emptied it - so the
brewer saw nothing at all, with no failure note.
The test is no longer a marker list, which real advice often does not
contain. A reply is all-working when it is LONG and has no instruction
in it: the model's working runs to thousands of characters, while the
advice is two to five sentences.
short reply with no marker -> kept
long reply, no instruction -> dropped
long reply with advice -> advice kept
single-paragraph working -> dropped
Also: the log message named the gravity, but the guard is the token.
182 tests (up from 178).
Two more fixed — and the first was the exact MEDIUM from the previous round, surviving for the 47 existing readings.
The pre-token fallback re-opened the hole (MEDIUM)
A reading that has never been edited has no token. The guard fell back to the gravity check for those, and a comment-only correction leaves the gravity unchanged — so a late thread's write matched and overwrote the fresh advice.
Reproduced live before the fix:
pre-token reading token: None -> stale thread carries ""
fresh write modified: 1
stale write modified: 1 <<< stale advice overwrote the fresh one
final advice: 'STALE ADVICE'
The guard now matches on the absence of a token, which still tells the generations apart — an edit writes a token, so a stale thread's "" stops matching the moment the reading is corrected:
fresh write modified: 1 (must be 1)
stale write modified: 0 (must be 0)
The stripper could empty real advice (LOW)
You were right — `"Actually the drop is fine.
Wait a couple of days and check again."` is advice, and judging it paragraph by paragraph emptied it. The brewer would have seen nothing at all, with no failure note.
The test is no longer a marker list, because real advice often doesn't contain one. A reply is all-working when it is long and has no instruction in it — the model's working runs to thousands of characters, while the advice is two to five sentences:
short reply with no marker -> kept
long reply, no instruction -> dropped
long reply with advice -> advice kept
single-paragraph working -> dropped
Also
The log message named the gravity, but the guard is the token.
End to end again after these changes
reading recorded
ADVICE after ~12s: That 1.040 reading is identical to your original gravity, which is exactly right for a day-zero measurement taken right after pitching before the yeast has had time to work...
182 tests (up from 178).
Two more fixed — and the first was the exact MEDIUM from the previous round, surviving for the 47 existing readings.
### The pre-token fallback re-opened the hole (MEDIUM)
A reading that has never been edited has **no token**. The guard fell back to the gravity check for those, and a comment-only correction leaves the gravity unchanged — so a late thread's write matched and overwrote the fresh advice.
Reproduced live before the fix:
```
pre-token reading token: None -> stale thread carries ""
fresh write modified: 1
stale write modified: 1 <<< stale advice overwrote the fresh one
final advice: 'STALE ADVICE'
```
The guard now matches on the **absence** of a token, which still tells the generations apart — an edit writes a token, so a stale thread's `""` stops matching the moment the reading is corrected:
```
fresh write modified: 1 (must be 1)
stale write modified: 0 (must be 0)
```
### The stripper could empty real advice (LOW)
You were right — `"Actually the drop is fine.
Wait a couple of days and check again."` is advice, and judging it paragraph by paragraph emptied it. The brewer would have seen nothing at all, with no failure note.
The test is no longer a marker list, because real advice often doesn't contain one. A reply is all-working when it is **long** and has no instruction in it — the model's working runs to thousands of characters, while the advice is two to five sentences:
```
short reply with no marker -> kept
long reply, no instruction -> dropped
long reply with advice -> advice kept
single-paragraph working -> dropped
```
### Also
- The log message named the gravity, but the guard is the token.
### End to end again after these changes
```
reading recorded
ADVICE after ~12s: That 1.040 reading is identical to your original gravity, which is exactly right for a day-zero measurement taken right after pitching before the yeast has had time to work...
```
182 tests (up from 178).
The guard matches on the absence of a token. `{"$exists": False}` covers
a reading with no token field, but NOT one whose token field is present
and empty.
Verified against the live database, before the fix:
token present but empty -> write matched: 0 <-- missed it
`{"$in": [None, ""]}` covers all three - a missing field, an empty
field, and a null one - because `$in` with null also matches a missing
field.
no token field -> write matched: 1
token present but empty -> write matched: 1
token is null -> write matched: 1
183 tests (up from 182).
A length threshold is a blunt instrument: a legitimate advice reply can
run to a thousand characters, and a short working reply can be a few
hundred - so a threshold gets both wrong. A 995-character advice reply
with no marker word in it was being emptied.
The reliable signal is the SHAPE. The model's working is about
INTERPRETING the numbers - what the reading might mean, what could be
causing it, what it is unsure about. The advice is about what to DO.
So a reply is working when it is full of the model's hedging about the
data AND contains no instruction at all. Length is only a tie-breaker
for a reply that is both hedged and short.
995-char legitimate advice -> kept
long pure working -> dropped
short legitimate advice -> kept
long working + advice -> advice kept
Checked against the live model - a real reply comes back clean, with no
reasoning in it:
At two weeks in, dropping from 1.055 to 1.020 is a healthy 35-point
fall - about 64% attenuation - which is right in the normal range for
a pale ale, though you likely still have 5 to 8 points to go...
184 tests (up from 183).
Two more found and fixed while you were reviewing — both in the stripper, and one of them I'd have shipped a regression on.
A length threshold is the wrong instrument
Your 995-character legitimate advice reply was being emptied. A legitimate advice reply can run to a thousand characters and a short working reply can be a few hundred, so a threshold gets both wrong.
The reliable signal is the shape. The model's working is about interpreting the numbers — what the reading might mean, what could be causing it, what it is unsure about. The advice is about what to do.
So a reply is working when it is full of the model's hedging about the data and contains no instruction at all. Length is only a tie-breaker for a reply that is both hedged and short.
995-char legitimate advice -> kept
long pure working -> dropped
short legitimate advice -> kept
long working + advice -> advice kept
The no-token guard missed an empty token field
{"$exists": False} covers a reading with no token field, but not one whose token field is present and empty:
token present but empty -> write matched: 0 <-- missed it
{"$in": [None, ""]} covers all three — a missing field, an empty field, and a null one — because $in with null also matches a missing field:
no token field -> write matched: 1
token present but empty -> write matched: 1
token is null -> write matched: 1
Checked against the live model
A real reply comes back clean, with no reasoning in it:
At two weeks in, dropping from 1.055 to 1.020 is a healthy 35-point fall—about 64% attenuation—which is right in the normal range for a pale ale, though you likely still have 5 to 8 points to go before hitting a typical final gravity around 1.012 to 1.015. This isn't stalled yet; just give it another three to four days...
184 tests (up from 178).
Two more found and fixed while you were reviewing — both in the stripper, and one of them I'd have shipped a regression on.
### A length threshold is the wrong instrument
Your 995-character legitimate advice reply was being emptied. A legitimate advice reply can run to a thousand characters and a short working reply can be a few hundred, so a threshold gets both wrong.
The reliable signal is the **shape**. The model's working is about *interpreting* the numbers — what the reading might mean, what could be causing it, what it is unsure about. The advice is about what to *do*.
So a reply is working when it is full of the model's hedging about the data **and** contains no instruction at all. Length is only a tie-breaker for a reply that is both hedged and short.
```
995-char legitimate advice -> kept
long pure working -> dropped
short legitimate advice -> kept
long working + advice -> advice kept
```
### The no-token guard missed an empty token field
`{"$exists": False}` covers a reading with no token field, but not one whose token field is *present and empty*:
```
token present but empty -> write matched: 0 <-- missed it
```
`{"$in": [None, ""]}` covers all three — a missing field, an empty field, and a null one — because `$in` with null also matches a missing field:
```
no token field -> write matched: 1
token present but empty -> write matched: 1
token is null -> write matched: 1
```
### Checked against the live model
A real reply comes back clean, with no reasoning in it:
```
At two weeks in, dropping from 1.055 to 1.020 is a healthy 35-point fall—about 64% attenuation—which is right in the normal range for a pale ale, though you likely still have 5 to 8 points to go before hitting a typical final gravity around 1.012 to 1.015. This isn't stalled yet; just give it another three to four days...
```
184 tests (up from 178).
The stripper tried to work out where the model's working ends and the
advice begins by classifying prose - marker words, hedging words,
length. It failed in BOTH directions, and the review caught both:
- it emptied legitimate advice (a 1,093-character reply containing one
hedging word and no marker phrase was reduced to ""), so the brewer
saw nothing at all; and
- it let the model's working through in full (a 1,053-character reply
with no hedging word was returned verbatim), so the brewer was shown
the model thinking about their beer.
Neither a marker list nor a length threshold is reliable. Prose is not
a format.
So the model is asked for the format instead. Its reply must have this
shape:
ADVICE:
<the advice itself>
Everything before the marker is working, everything from it on is the
advice. The split is exact, with no judgement calls.
Verified against the live model, four different brews:
Test Pale Ale marker@pos=0 finish=stop advice_len=656
Nocturna Christmas marker@pos=0 finish=stop advice_len=668
Ginger beer marker@pos=0 finish=stop advice_len=621
Cider raspberry marker@pos=0 finish=stop advice_len=539
All four start clean, with no marker left in the advice.
A reply with no marker is used as it stands - the safe direction, where
the brewer sees a slightly rambling reply rather than no advice at all.
An empty advice is now recorded as a FAILURE rather than stored as a
successful generation. Storing an empty string with advice_ok=True
made the page render "Thinking..." for ever, because the template's
`{% if reading.advice %}` was false - so the brewer never learned that
it had failed.
181 tests (up from 184 - the eight heuristic-era stripper tests are
replaced by six that test the marker behaviour).
You were right that the stripper was failing in both directions — and the fix was to stop guessing.
The stripper is now a deterministic split
I was trying to work out where the model's working ends and the advice begins by classifying prose — marker words, hedging words, length. It failed in both directions, and neither a marker list nor a length threshold is reliable. Prose is not a format.
So the model is asked for the format instead. Its reply must have this shape:
ADVICE:
<the advice itself>
Everything before the marker is working, everything from it on is the advice. The split is exact, with no judgement calls.
Verified against the live model, four different brews:
Test Pale Ale marker@pos=0 finish=stop advice_len=656
Nocturna Christmas marker@pos=0 finish=stop advice_len=668
Ginger beer marker@pos=0 finish=stop advice_len=621
Cider raspberry marker@pos=0 finish=stop advice_len=539
All four start clean, with no marker left in the advice.
A reply with no marker is used as it stands — the safe direction, where the brewer sees a slightly rambling reply rather than no advice at all.
The empty advice is now a failure, not a success
You caught that one too: the stripper dropping the working returned "", which was stored with advice_ok=True, and the template's {% if reading.advice %} was false — so the page said "Thinking…" for ever and the brewer never learned it had failed.
An empty advice is now recorded as a failure with a note the brewer can see.
The dead heuristics are gone
_PREAMBLE_MARKERS, _ADVICE_MARKERS, _HEDGING_WORDS, _WORKING_LENGTH, _is_all_working, _has_advice_marker and _looks_like_advice are all removed. The marker is the only signal.
181 tests (down from 184 — the eight heuristic-era stripper tests are replaced by six that test the marker behaviour directly).
You were right that the stripper was failing in both directions — and the fix was to stop guessing.
### The stripper is now a deterministic split
I was trying to work out where the model's working ends and the advice begins by classifying prose — marker words, hedging words, length. It failed in both directions, and neither a marker list nor a length threshold is reliable. Prose is not a format.
So the model is asked for the format instead. Its reply must have this shape:
```
ADVICE:
<the advice itself>
```
Everything before the marker is working, everything from it on is the advice. The split is exact, with no judgement calls.
Verified against the live model, four different brews:
```
Test Pale Ale marker@pos=0 finish=stop advice_len=656
Nocturna Christmas marker@pos=0 finish=stop advice_len=668
Ginger beer marker@pos=0 finish=stop advice_len=621
Cider raspberry marker@pos=0 finish=stop advice_len=539
```
All four start clean, with no marker left in the advice.
A reply with no marker is used as it stands — the safe direction, where the brewer sees a slightly rambling reply rather than no advice at all.
### The empty advice is now a failure, not a success
You caught that one too: the stripper dropping the working returned `""`, which was stored with `advice_ok=True`, and the template's `{% if reading.advice %}` was false — so the page said **"Thinking…"** for ever and the brewer never learned it had failed.
An empty advice is now recorded as a failure with a note the brewer can see.
### The dead heuristics are gone
`_PREAMBLE_MARKERS`, `_ADVICE_MARKERS`, `_HEDGING_WORDS`, `_WORKING_LENGTH`, `_is_all_working`, `_has_advice_marker` and `_looks_like_advice` are all removed. The marker is the only signal.
181 tests (down from 184 — the eight heuristic-era stripper tests are replaced by six that test the marker behaviour directly).
The instruction asks for "ADVICE:", but the model writes "**ADVICE:**",
"## ADVICE:" or puts it in a code block. The extraction has to cope,
because leftover markdown in the advice is visible to the brewer.
The marker is now matched case-insensitively and with the decoration the
model puts around it, and the leftover formatting is stripped.
plain marker -> 'Warm it to 20C and wait.'
bold marker -> 'Warm it to 20C and wait.'
in a code block -> 'Warm it to 20C and wait.'
marker mid-text -> 'Warm it to 20C and wait.'
markdown heading -> 'Warm it to 20C and wait.'
lowercase marker -> 'Warm it to 20C and wait.'
no marker -> 'Warm it to 20C and wait.'
bold + code block -> 'Warm it to 20C and wait.'
182 tests (up from 181).
hermes
merged commit 55b6068b3b into master2026-11-09 20:23:32 +10:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Summary
Stores a model's brewing advice for each recorded observation, in its own field in Mongo, shown on the brewing page, the active beers page and the run detail page.
Now merged and deployed. CI run 1699 was all seven steps green; the live site at http://192.168.178.160:30015 was verified by recording a reading through it — the advice appeared in 28 seconds and renders on the Active Beers page.
How it behaves
Fire and forget. The advice is generated in a background thread, so recording a reading never blocks on the model. The reading appears immediately; the advice is there on a later visit. If the model is down, the reading is still saved and the advice field carries a visible note saying it could not be generated — a silent failure would be indistinguishable from "still thinking", which is confusing when you're waiting for a tip that is never going to arrive.
Your comment is never overwritten. The advice lives in its own field (
advice,advice_ok,advice_at) alongside yourcomment. Verified on the real document.The model
mistral-large-4:cloudon the Ollama instance at192.168.178.45:11434— reachable from the pod.Both
OLLAMA_MODELandOLLAMA_URLare configurable.The prompt tells it not to repeat the app's mistake
The system prompt says explicitly:
This is deliberately the opposite of the app's "exclude it as impossible" treatment of run 12 — which you have since told me was a genuine long fermentation. The model should point at the odd reading, say so, and keep talking about the brew.
What it actually said
For a stalled brew (OG 1.042, 1.022 after 9 days at 19°C, "no change since day 7"):
And on the live site, for your own "Out on patio" run:
The marker
The model is asked to put
ADVICE:on its own line before the advice, so the split between its working and its advice is exact — no guessing at where one ends and the other begins. The marker is found however the model formats it (ADVICE:,**ADVICE:**,## ADVICE:, in a code block, lower case).An earlier attempt at this classified prose by marker words and length. It failed in both directions — it emptied legitimate advice, and it let the model's working through in full. Asking the model for a format is the reliable answer.
The pages
Safety
|safe, and there are no inline scripts (the CSP stays enforceable).LLM_ADVICE=0turns it off in production if the model misbehaves.The bugs the reviews caught
Seven rounds, and two of these would have shipped a feature that looked like it worked while doing nothing:
_as_doc_idaccepted only ObjectIds, but all 47 of your readings have 24-character string_ids. The test fixture generates ObjectIds, so no test could have caught it — only checking against the live database did.Test plan
tests/test_brew_advice.py: the prompt content, the marker handling, the token guard, the injection guard, the stale-advice races, and all three pagesPR received — starting review, sit tight 🫡
PR Review Results
Consolidated Review Report: LLM Brewing Advice Integration
1. Executive Summary
This PR introduces an asynchronous integration with an Ollama-hosted LLM to provide automated brewing advice. While the functional implementation is well-documented and the UI/UX considerations are strong, the current architectural approach to concurrency and network dependency introduces significant stability and security risks.
The primary concern is the "fire and forget" threading model used within a Flask environment, which lacks the robustness of a managed task queue. Furthermore, the integration lacks critical safeguards against indirect prompt injection and resource exhaustion (timeouts). While the PR is a strong functional start, it requires architectural refinements before it can be considered production-ready.
2. Prioritized Issues List
🔴 CRITICAL
threading.Threadper request without a bounded queue or strict timeouts can lead to memory exhaustion and MongoDB connection leaks.🟠 HIGH
🟡 MEDIUM
advice_ok=False), there is no structured logging or metrics to alert the platform team of systemic LLM failures.🔵 LOW
3. Domain-Specific Recommendations
💻 Code Quality & Design
threading.Threadwith a lightweight task queue such as Celery or RQ (Redis Queue). This decouples the web request from the LLM processing and provides retry logic.connectandreadtimeout (e.g., 30s) to prevent "hanging" threads.🛡️ Security
### USER INPUT: {comment} ###) and update the system prompt to explicitly ignore instructions contained within those boundaries.NetworkPolicyto restrict the Flask pod's egress traffic strictly to the Ollama service IP/Port.⚙️ Infrastructure & DevOps
192.168.178.45) with a Kubernetes DNS name (e.g.,ollama.llm-namespace.svc.cluster.local).OLLAMA_URLandOLLAMA_MODELare injected via K8s Secrets or ConfigMaps, not hardcoded in manifests.llm_request_failures_total) to track the failure rate of the advice generation service.4. Positive Aspects of the PR
Despite the architectural concerns, the following points are highly commended:
commentfromadviceand using anadvice_okflag prevents user confusion and maintains data integrity.|safein Jinja2 templates shows a proactive approach to preventing XSS.5. Overall Assessment & Recommendation
Rating: ⚠️ SIGNIFICANT CHANGES NEEDED
Justification:
While the feature is functionally complete and well-tested, the concurrency model (raw threads) and network configuration (static IP) are not suitable for a production environment. These represent "ticking time bombs" that could lead to application crashes or security vulnerabilities under load.
Required Actions for Approval:
threading.Threadto a persisted task queue (Celery/RQ).Two things I wanted to be sure of before this merges. Stale advice ------------ The advice is written for the gravity that was there when it was generated. If the brewer then edits the reading - to fix a typo, or to mark it final - the old advice no longer describes it, and the page was showing advice that contradicted the number next to it. `upsert_reading` now clears the advice fields, so the stale advice goes and a fresh one is generated (the form already regenerates on every save). Only the advice fields are cleared - the brewer's own comment is never touched. The injection guard ------------------- The reading's `_id` is an ObjectId, so the original `_as_id` coercion (which turns run ids into plain strings, to stop a caller passing a dict that Mongo would interpret as a query operator) did not apply - it turned the ObjectId into an empty string and the update silently matched nothing. `_as_doc_id` accepts only an ObjectId or a string that parses as one. Verified against a live injection attempt: `{"$ne": None}`, `{"$gt": ""}`, a list, an int, None and a non-ObjectId string all return None and match nothing. There is now a test that pins it, because it is a security property that would otherwise only be covered by reading the code. 160 tests (up from 158).Thanks — the critical finding was right, and it was mine.
CRITICAL:
_as_doc_idrejected every live reading_idAll 47 live readings have string
_ids — 24 random characters from the original app's importer (l78ZidBJ3QaYn98tvB88x4uM), not ObjectIds. My helper turned every one of them intoNone, so the update matched nothing and the advice would have been silently never stored in production.The mongomock fixture generates ObjectIds, which is why the tests couldn't see it.
_as_doc_idnow returns both forms and the query uses$in(a plain list is an exact array match in Mongo, which is not what was wanted).Verified against the live database:
There are now tests for both id shapes, with the string shape built explicitly — because the fixture would not produce it.
The advice was written about the wrong reading
latestcame from the date-sorted tail of the run, so editing an older reading generated advice about the newest one. The advice is now written against the reading that was actually saved.The leaked MongoClient
You measured three threads and a pool per reading — that's right, and it's worse than a plain leak: pymongo does not reap its monitor threads on
close(); they go only when the client is garbage collected, soclose()alone did nothing.Measured after the fix — 4 threads total, not 16 over five writes:
There is now one shared repository for the life of the process, which is what the request-scoped repository already did.
BeerRepositoryalso gainedclose()and the context-manager protocol.Also
Verified end to end against the live database
Reading recorded through the app → advice generated in the background → stored in Mongo → rendered on the page, with the comment intact:
(Test run cleaned up — 47 readings, as before.)
Your other checks
|safe/mark_safe/innerHTMLsink, CSP staysscript-src 'self'.Test plan
masteron merge; I will confirm the rollout afterwards.The model is now `mistral-large-4:cloud`, on the same Ollama instance. It was probed before being made the default - `deepseek-v4-flash:cloud`, `deepseek-v3.2:cloud` and `qwen3.5:cloud` are all retired and fail at call time while still appearing in the model list. What the probes showed ---------------------- `think: false` does NOT stop the reasoning. It only changes where the working goes: with thinking off, the model writes it into the CONTENT instead of a separate field. Either way the reasoning comes out of the same token allowance as the answer, and at 500-3000 tokens the reply came back EMPTY - which looks like a broken model rather than a budget problem. The budget is now 8000, and the timeout 120s. The reply is also stripped of a reasoning preamble. The advice is the prose at the END; the working comes first, in several paragraphs. The preamble is found by walking back from the end of the reply and keeping the paragraphs that read like advice to the brewer, rather than like the model thinking out loud. Verified against the live model, on a stalled brew (OG 1.042, 1.022 after nine days, "no change since day 7"): That 1.022 reading after nine days at 19C - with no movement since day seven - looks like a stalled fermentation; a 1.042 wort should be well on its way to 1.010-1.012 by now, not sitting at only about 48% attenuation. Gently swirl the fermenter to rouse the yeast cake and keep the temperature steady at 19-20C, and if the gravity hasn't budged in another 48 hours, pitch a fresh, healthy yeast starter... And on a CORRECTED older reading - the case the review found broken - the advice is about the corrected reading, with the later one as context: A brew-day reading of 1.038 against an OG of 1.042 is only four points low, which is almost certainly just the normal difference between your kettle gravity and the post-transfer fermenter reading... 169 tests (up from 167).Thanks — the critical finding was right, and it was mine.
CRITICAL:
_as_doc_idrejected every live reading_idAll 47 live readings have string
_ids — 24 random characters from the original app's importer (l78ZidBJ3QaYn98tvB88x4uM), not ObjectIds. My helper turned every one of them intoNone, so the update matched nothing and the advice would have been silently never stored in production.The mongomock fixture generates ObjectIds, which is why the tests couldn't see it.
_as_doc_idnow returns both forms and the query uses$in(a plain list is an exact array match in Mongo, which is not what was wanted).Verified against the live database:
There are now tests for both id shapes, with the string shape built explicitly — because the fixture would not produce it.
The advice was written about the wrong reading
latestcame from the date-sorted tail of the run, so editing an older reading generated advice about the newest one. The advice is now written against the reading that was actually saved.The leaked MongoClient
You measured three threads and a pool per reading — that's right, and it's worse than a plain leak: pymongo does not reap its monitor threads on
close(); they go only when the client is garbage collected, soclose()alone did nothing.Measured after the fix — 4 threads total, not 16 over five writes:
There is now one shared repository for the life of the process, which is what the request-scoped repository already did.
BeerRepositoryalso gainedclose()and the context-manager protocol.Also
Verified end to end against the live database
Reading recorded through the app → advice generated in the background → stored in Mongo → rendered on the page, with the comment intact:
(Test run cleaned up — 47 readings, as before.)
Your other checks
|safe/mark_safe/innerHTMLsink, CSP staysscript-src 'self'.Test plan
masteron merge; I will confirm the rollout afterwards.Update: the model is now
mistral-large-4:cloudYour suggestion, and it was right — the reasoning models were the problem.
think: falsedoes not stop the reasoning. It only changes where the working goes: with thinking off, the model writes it into the content instead of a separate field. Either way the reasoning comes out of the same token allowance as the answer, and at 500–3000 tokens the reply came back empty — which looks like a broken model rather than a budget problem.So:
Verified against the live model on a stalled brew (OG 1.042, 1.022 after nine days, "no change since day 7"):
And on a corrected older reading — the case your review found broken — the advice is about the corrected reading, with the later one as context:
Also fixed since your review
get_shared_repository(). An unsynchronised check-then-set let eight concurrent threads each build their own repository — 8 clients, 25 threads. Now guarded by a lock, with the check repeated inside it. Measured after: 1 repository, 4 threads.169 tests.
The review found a way to store the model's WORKING instead of its advice. The advice is at the END of the reply, so the length cap was cutting from the front - which took the advice off and left only the working. That is worse than no advice at all, because it looks like advice: the brew sees a full paragraph of the model thinking about their beer, and nothing about what to do. Reproduced: input len: 2515 -> final len: 1199 advice present? False TAIL: 'padding padding padding padding padding' Two changes: - The length cap keeps the END of the reply and cuts at a sentence boundary near the end, so the advice survives. - The stripper now also catches a reply that is entirely working - no marker phrase, and no paragraph that reads like advice - instead of only catching one that says "let me think". input: 2515 -> stripped: 2515 -> final: 1199 advice present? True A short reply is untouched. 171 tests (up from 169).The review found a race between the background advice thread and the brewer correcting a reading. `set_reading_advice` wrote unconditionally by id. The advice is generated in the background, so if the brewer corrects a reading while the advice for the OLD gravity is still being generated, the slow thread lands afterwards and overwrites the fresh advice with advice about a reading that no longer exists. The reading ends up with the final gravity from the correction and the advice from before it. Reproduced against the live database: wrote advice for 1050 wrote advice for 1020 late stale write matched: 0 final sg: 1020 | final advice: 'ADVICE FOR THE CORRECTION' The write is now conditional on the reading still holding the gravity the advice was generated for, so a late thread matches nothing. Also ---- `finish_reason` is now checked. A reply that stopped at the token ceiling is incomplete - it may be advice that stops mid-sentence - and it was being stored as if it were the whole answer. It is now marked as cut off. 173 tests (up from 171).Thanks — and the MEDIUM was a genuinely good catch. I'd misread which finding was which.
MEDIUM — stale advice overwriting a correction (FIXED)
set_reading_advicewrote unconditionally by_id. The advice is generated in the background, so if the brewer corrects a reading while the advice for the old gravity is still being generated, the slow thread lands afterwards and overwrites the fresh advice.Reproduced against the live database:
The write is now conditional on the reading still holding the gravity the advice was generated for, so a late thread matches nothing.
LOW —
finish_reasonis not checked (FIXED)A reply that stopped at the token ceiling (
finish_reason == "length") may be advice that stops mid-sentence, and it was being stored as if it were the whole answer. It is now marked as cut off.LOW — the reasoning stripper
You were right that it "is a no-op in the case it was written for". Two changes:
I've left the marker heuristics in place rather than rewriting them to your paragraph-shape suggestion, on the basis that the current model returns its working in a separate field and 0 of 6 live samples were altered by the stripper. But your point stands that it's fragile defensive code, and the length cap no longer depends on it to preserve the advice.
Both previous findings confirmed fixed by you
173 tests (up from 169).
The gravity guard caught a correction that changed the gravity, but a COMMENT-ONLY correction leaves the gravity unchanged - so the late thread for the old advice still matched and overwrote the fresh one. Reproduced against the live database: fresh advice matched: 1 stale thread matched: 1 <-- should be 0 FINAL advice: 'STALE ADVICE' Every edit now writes a fresh token onto the reading, and the advice thread carries the token it was given. A late thread finds a token that no longer matches, so its write is refused. The gravity is still checked as well. Verified against the live database: token changed: True fresh advice matched: 1 stale thread matched: 0 <-- must be 0 FINAL advice: 'FRESH ADVICE' 174 tests (up from 173).One more round of findings addressed — including the comment-only correction case, which the gravity guard missed.
The comment-only correction (MEDIUM)
The gravity guard caught a correction that changed the gravity, but a comment-only correction leaves the gravity unchanged — so the late thread for the old advice still matched and overwrote the fresh one. You were right to push on this.
Reproduced against the live database before the fix:
Every edit now writes a fresh editing token onto the reading, and the advice thread carries the token it was given. A late thread finds a token that no longer matches, so its write is refused. The gravity is still checked as well.
The four LOW items
test_the_advice_is_about_the_reading_that_was_savedasserted which dict was passed to the generator, not what the model was asked. So it passed while the prompt still said "give advice for the most recent reading". It now asserts on the prompt content — the reading is marked, its date is named, and "most recent reading" is gone. I verified it catches the bug by reverting the prompt fix: two tests fail, and both pass again with the fix restored.advice--readingis used on the active-beers card and the run detail page and was never defined.The
sgtype question you were asked to checkAll 47 live readings store
sgas a string, andupsert_readingwritesstr(sg)— so the conditional write's{"sg": str(expected_sg)}matches, and the normal path is not broken. The token is now the primary guard, with the gravity as a secondary check.174 tests (up from 173).
Two more findings fixed — and you were right about the stripper being worse than before.
The stripper was a no-op in the case it was written for
You caught this precisely. The "all working" branch returned the text unchanged, which is byte-identical to the old fallthrough — and in the case where a preamble marker was present and no paragraph read like advice, it returned the whole reply, working included. Your measurement (old: 80 chars stored; new: 612 chars of working) matches what I saw when I re-ran it.
It now drops the working, so the reply is empty and the brewer gets a clear "could not be generated" rather than the model thinking about their beer:
"keep" alone is not an advice marker
"I need to keep in mind that the hydrometer was read at a different temperature"was classifying as advice, so the "all working" detection almost never fired on real reasoning text. The bare words "keep", "raise", "lower", "take the" are now matched as phrases — "you should keep", "raise it to", "take another reading" — and "i need to keep in mind" is explicitly working.And a reply with real advice still keeps it:
The token is the primary guard
You noted that
expected_sgisNonefor an unparseable gravity — one live reading hassg: '900'— and that the guard was then skipped entirely, making the write unconditional on exactly the readings most likely to be wrong.The editing token is now the primary guard: it changes on every edit, so it catches a stale generation whatever else changed — the gravity, the comment, or nothing. The gravity is a secondary check, used only when there is no token.
That also covers the comment-only case from your previous round, which the gravity guard alone missed.
Verified end to end again after these changes
177 tests (up from 174).
None of the 47 existing readings have a token - they all predate it. A reading read from the database therefore has an EMPTY token, and the guard was checking `expected_token is not None`, which is TRUE for an empty string. So the write was conditioned on a token that does not exist, matched nothing, and **the advice would never have been written for any of the existing readings**. Verified against the live database, before the fix: write with expected_token='' -> 0 (0 = the advice would NEVER be written) After: write for a pre-token live reading: 1 (must be 1) An empty token is now not a usable guard - there is no token to match - so the write falls through to the gravity check instead. A non-empty token is still the primary guard. There is a test for it, with a reading inserted the way the existing ones are - directly, with no token. 178 tests (up from 177).One more, and this one was serious — the advice would have been invisible for every existing reading.
An empty token must not be used as a guard
None of the 47 existing readings have a token — they all predate it. So a reading read from the database has an empty token, and the guard was checking
expected_token is not None, which isTruefor an empty string.So the write was conditioned on a token that does not exist, matched nothing, and the advice would never have been written for any of the existing readings.
Verified against the live database, before the fix:
After:
An empty token is now not a usable guard — there is no token to match — so the write falls through to the gravity check. A non-empty token is still the primary guard.
There is a test with a reading inserted the way the existing ones are: directly, with no token.
End to end again after these changes
178 tests (up from 177).
Two findings from the fifth review. The pre-token fallback re-opened the hole ----------------------------------------- A reading that has never been edited has no token. The guard fell back to the gravity check for those, and a comment-only correction leaves the gravity unchanged - so a late thread's write matched and overwrote the fresh advice, for exactly the 47 existing readings. Reproduced live before the fix: pre-token reading token: None -> stale thread carries "" fresh write modified: 1 stale write modified: 1 <<< stale advice overwrote the fresh one final advice: 'STALE ADVICE' The guard now matches on the ABSENCE of a token, which still tells the generations apart: an edit writes a token, so a stale thread's "" stops matching the moment the reading is corrected. fresh write modified: 1 (must be 1) stale write modified: 0 (must be 0) The stripper could empty real advice ------------------------------------ "Actually the drop is fine.\\n\\nWait a couple of days and check again." is advice, and judging it paragraph by paragraph emptied it - so the brewer saw nothing at all, with no failure note. The test is no longer a marker list, which real advice often does not contain. A reply is all-working when it is LONG and has no instruction in it: the model's working runs to thousands of characters, while the advice is two to five sentences. short reply with no marker -> kept long reply, no instruction -> dropped long reply with advice -> advice kept single-paragraph working -> dropped Also: the log message named the gravity, but the guard is the token. 182 tests (up from 178).Two more fixed — and the first was the exact MEDIUM from the previous round, surviving for the 47 existing readings.
The pre-token fallback re-opened the hole (MEDIUM)
A reading that has never been edited has no token. The guard fell back to the gravity check for those, and a comment-only correction leaves the gravity unchanged — so a late thread's write matched and overwrote the fresh advice.
Reproduced live before the fix:
The guard now matches on the absence of a token, which still tells the generations apart — an edit writes a token, so a stale thread's
""stops matching the moment the reading is corrected:The stripper could empty real advice (LOW)
You were right — `"Actually the drop is fine.
Wait a couple of days and check again."` is advice, and judging it paragraph by paragraph emptied it. The brewer would have seen nothing at all, with no failure note.
The test is no longer a marker list, because real advice often doesn't contain one. A reply is all-working when it is long and has no instruction in it — the model's working runs to thousands of characters, while the advice is two to five sentences:
Also
End to end again after these changes
182 tests (up from 178).
The guard matches on the absence of a token. `{"$exists": False}` covers a reading with no token field, but NOT one whose token field is present and empty. Verified against the live database, before the fix: token present but empty -> write matched: 0 <-- missed it `{"$in": [None, ""]}` covers all three - a missing field, an empty field, and a null one - because `$in` with null also matches a missing field. no token field -> write matched: 1 token present but empty -> write matched: 1 token is null -> write matched: 1 183 tests (up from 182).A length threshold is a blunt instrument: a legitimate advice reply can run to a thousand characters, and a short working reply can be a few hundred - so a threshold gets both wrong. A 995-character advice reply with no marker word in it was being emptied. The reliable signal is the SHAPE. The model's working is about INTERPRETING the numbers - what the reading might mean, what could be causing it, what it is unsure about. The advice is about what to DO. So a reply is working when it is full of the model's hedging about the data AND contains no instruction at all. Length is only a tie-breaker for a reply that is both hedged and short. 995-char legitimate advice -> kept long pure working -> dropped short legitimate advice -> kept long working + advice -> advice kept Checked against the live model - a real reply comes back clean, with no reasoning in it: At two weeks in, dropping from 1.055 to 1.020 is a healthy 35-point fall - about 64% attenuation - which is right in the normal range for a pale ale, though you likely still have 5 to 8 points to go... 184 tests (up from 183).Two more found and fixed while you were reviewing — both in the stripper, and one of them I'd have shipped a regression on.
A length threshold is the wrong instrument
Your 995-character legitimate advice reply was being emptied. A legitimate advice reply can run to a thousand characters and a short working reply can be a few hundred, so a threshold gets both wrong.
The reliable signal is the shape. The model's working is about interpreting the numbers — what the reading might mean, what could be causing it, what it is unsure about. The advice is about what to do.
So a reply is working when it is full of the model's hedging about the data and contains no instruction at all. Length is only a tie-breaker for a reply that is both hedged and short.
The no-token guard missed an empty token field
{"$exists": False}covers a reading with no token field, but not one whose token field is present and empty:{"$in": [None, ""]}covers all three — a missing field, an empty field, and a null one — because$inwith null also matches a missing field:Checked against the live model
A real reply comes back clean, with no reasoning in it:
184 tests (up from 178).
The stripper tried to work out where the model's working ends and the advice begins by classifying prose - marker words, hedging words, length. It failed in BOTH directions, and the review caught both: - it emptied legitimate advice (a 1,093-character reply containing one hedging word and no marker phrase was reduced to ""), so the brewer saw nothing at all; and - it let the model's working through in full (a 1,053-character reply with no hedging word was returned verbatim), so the brewer was shown the model thinking about their beer. Neither a marker list nor a length threshold is reliable. Prose is not a format. So the model is asked for the format instead. Its reply must have this shape: ADVICE: <the advice itself> Everything before the marker is working, everything from it on is the advice. The split is exact, with no judgement calls. Verified against the live model, four different brews: Test Pale Ale marker@pos=0 finish=stop advice_len=656 Nocturna Christmas marker@pos=0 finish=stop advice_len=668 Ginger beer marker@pos=0 finish=stop advice_len=621 Cider raspberry marker@pos=0 finish=stop advice_len=539 All four start clean, with no marker left in the advice. A reply with no marker is used as it stands - the safe direction, where the brewer sees a slightly rambling reply rather than no advice at all. An empty advice is now recorded as a FAILURE rather than stored as a successful generation. Storing an empty string with advice_ok=True made the page render "Thinking..." for ever, because the template's `{% if reading.advice %}` was false - so the brewer never learned that it had failed. 181 tests (up from 184 - the eight heuristic-era stripper tests are replaced by six that test the marker behaviour).You were right that the stripper was failing in both directions — and the fix was to stop guessing.
The stripper is now a deterministic split
I was trying to work out where the model's working ends and the advice begins by classifying prose — marker words, hedging words, length. It failed in both directions, and neither a marker list nor a length threshold is reliable. Prose is not a format.
So the model is asked for the format instead. Its reply must have this shape:
Everything before the marker is working, everything from it on is the advice. The split is exact, with no judgement calls.
Verified against the live model, four different brews:
All four start clean, with no marker left in the advice.
A reply with no marker is used as it stands — the safe direction, where the brewer sees a slightly rambling reply rather than no advice at all.
The empty advice is now a failure, not a success
You caught that one too: the stripper dropping the working returned
"", which was stored withadvice_ok=True, and the template's{% if reading.advice %}was false — so the page said "Thinking…" for ever and the brewer never learned it had failed.An empty advice is now recorded as a failure with a note the brewer can see.
The dead heuristics are gone
_PREAMBLE_MARKERS,_ADVICE_MARKERS,_HEDGING_WORDS,_WORKING_LENGTH,_is_all_working,_has_advice_markerand_looks_like_adviceare all removed. The marker is the only signal.181 tests (down from 184 — the eight heuristic-era stripper tests are replaced by six that test the marker behaviour directly).
The instruction asks for "ADVICE:", but the model writes "**ADVICE:**", "## ADVICE:" or puts it in a code block. The extraction has to cope, because leftover markdown in the advice is visible to the brewer. The marker is now matched case-insensitively and with the decoration the model puts around it, and the leftover formatting is stripped. plain marker -> 'Warm it to 20C and wait.' bold marker -> 'Warm it to 20C and wait.' in a code block -> 'Warm it to 20C and wait.' marker mid-text -> 'Warm it to 20C and wait.' markdown heading -> 'Warm it to 20C and wait.' lowercase marker -> 'Warm it to 20C and wait.' no marker -> 'Warm it to 20C and wait.' bold + code block -> 'Warm it to 20C and wait.' 182 tests (up from 181).