From ab3050194d9787e28e0a0a6bea4318c07366db1f Mon Sep 17 00:00:00 2001 From: hermes Date: Mon, 9 Nov 2026 15:52:26 +1000 Subject: [PATCH 01/17] feat(advice): LLM brewing advice on every observation (work in progress) 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. --- src/flask/brew_advice.py | 211 +++++++++++++++++ src/flask/brewing.py | 31 +++ src/flask/mongo/repository.py | 43 ++++ src/flask/static/css/app.css | 62 +++++ src/flask/templates/active.html | 7 + src/flask/templates/brews.html | 8 + src/flask/templates/run_detail.html | 6 + src/flask/views/brews.py | 97 +++++++- tests/test_brew_advice.py | 338 ++++++++++++++++++++++++++++ 9 files changed, 799 insertions(+), 4 deletions(-) create mode 100644 src/flask/brew_advice.py create mode 100644 tests/test_brew_advice.py diff --git a/src/flask/brew_advice.py b/src/flask/brew_advice.py new file mode 100644 index 0000000..222c21a --- /dev/null +++ b/src/flask/brew_advice.py @@ -0,0 +1,211 @@ +"""Brewing advice from an LLM, generated for each recorded observation. + +The advice is a *comment on the reading*, not a replacement for the +brewer's own comment. Every reading gets its own advice text, stored in +its own field, so nothing the brewer wrote is ever overwritten. + +Design notes +------------ +- **The LLM is never in the request path.** Recording a reading must not + block on a model that may be slow or down, so the advice is generated in + a background thread and written when it is ready. The page shows the + reading immediately and the advice appears on a later visit. +- **Failures are recorded, not raised.** If the model is unreachable the + advice field gets a short note saying so. A silent failure would look + identical to "no advice yet", which is confusing when you are waiting + for a tip that is never going to arrive. +- **The model is configurable**, with a default that is known to work on + this homelab. Several models on the Ollama instance have been retired + and fail at call time while still appearing in the model list, so the + default is verified rather than assumed. +""" + +from __future__ import annotations + +import json +import logging +import os +import urllib.error +import urllib.request + +log = logging.getLogger(__name__) + +#: The Ollama instance on the homelab. Reachable from the pod. +DEFAULT_OLLAMA_URL = os.getenv("OLLAMA_URL", "http://192.168.178.45:11434") + +#: A model that is known to work. `deepseek-v4-flash:cloud`, +#: `deepseek-v3.2:cloud` and `qwen3.5:cloud` are all retired and fail at +#: call time, so they are not safe defaults. +DEFAULT_MODEL = os.getenv("OLLAMA_MODEL", "glm-5.3:cloud") + +#: How long to wait for the model before giving up. The advice is +#: generated in the background, so this only bounds the background thread. +DEFAULT_TIMEOUT = float(os.getenv("LLM_TIMEOUT", "45")) + +#: The advice is advice, not instruction. Keep it short - it has to be +#: readable on the brewing page next to the brewer's own comment. +MAX_ADVICE_CHARS = 1200 + +SYSTEM_PROMPT = """\ +You are an experienced home brewer giving practical advice to a single \ +brewer about their own beer. You know brewing science: specific gravity, \ +attenuation, yeast health, temperature, timing, and what a stalled or \ +struck-out fermentation looks like. + +You will be given one brew's readings so far: its name, the original \ +gravity, each reading's date and specific gravity, and any comment the \ +brewer wrote. + +Give the brewer useful, specific, practical advice about THIS reading and \ +this brew. Consider things like: + +- Is the fermentation progressing at a normal rate for the style and \ +temperature? +- Has it stalled, and if so what should they do about it? +- Is the yeast likely stressed, unhealthy, or done? +- Is the gravity drop plausible for the original gravity? +- Should they rack, add more hops, raise the temperature, or wait? + +Rules: +- If everything looks normal, say so briefly and say what to expect next. +- If something looks wrong, say what it is and what to do about it. +- Be specific to the numbers given. Do not give generic advice. +- If a reading looks like a typo or a mis-entry, say so and suggest \ +checking it, but do not assume it - a slow or long fermentation can be \ +genuine. +- Never say a reading is impossible. Beer cannot be denser than water, so \ +a gravity below 1.000 is always a mis-entry or a typo, but the rest of the \ +run may still be real and worth understanding. +- No preamble, no headings, no bullet-point templates, no "Great question!". \ +Write two to five sentences of plain prose. +""" + + +def _reading_lines(readings: list[dict]) -> str: + """The readings, as the compact list the model reads.""" + lines = [] + for r in readings: + parts = [f"- {r.get('date')}: gravity {r.get('sg_decimal')} ({r.get('point_scale')})"] + if r.get("comment"): + parts.append(f"brewer's comment: {r['comment']}") + if r.get("final"): + parts.append("(marked as the FINAL reading)") + lines.append(" ".join(parts)) + return "\n".join(lines) + + +def build_prompt(run: dict, reading: dict, history: list[dict]) -> str: + """The user-message content for one observation.""" + name = run.get("canonical_name") or run.get("name") or "this brew" + og = run.get("og_decimal") + if og is None and run.get("og_points"): + og = f"{run['og_points'] / 1000:.3f}" + + parts = [ + f"Beer: {name}", + ] + if og: + parts.append(f"Original gravity: {og}") + if run.get("tap_number"): + parts.append(f"Fermenter tap: {run['tap_number']}") + if run.get("notes"): + parts.append(f"Brewer's notes: {run['notes']}") + + parts.append("") + parts.append("Readings so far (oldest first):") + parts.append(_reading_lines(history)) + + parts.append("") + parts.append("Give advice for the most recent reading.") + return "\n".join(parts) + + +def call_ollama(prompt: str, *, url: str | None = None, model: str | None = None, + timeout: float | None = None) -> str: + """Ask the model for advice. Raises on failure.""" + base = (url or DEFAULT_OLLAMA_URL).rstrip("/") + chosen_model = model or DEFAULT_MODEL + wait = timeout if timeout is not None else DEFAULT_TIMEOUT + + payload = json.dumps({ + "model": chosen_model, + "messages": [ + {"role": "system", "content": SYSTEM_PROMPT}, + {"role": "user", "content": prompt}, + ], + "temperature": 0.4, + # The reasoning models emit their thinking first, and it comes out + # of the same budget. At 500 tokens the reasoning used the whole + # allowance and the reply came back empty - which looked like a + # broken model rather than a budget problem. 2000 leaves room for + # the reasoning AND the advice. + "max_tokens": 2000, + "stream": False, + }).encode("utf-8") + + request = urllib.request.Request( + f"{base}/v1/chat/completions", + data=payload, + headers={"Content-Type": "application/json"}, + method="POST", + ) + with urllib.request.urlopen(request, timeout=wait) as response: + body = json.loads(response.read().decode("utf-8")) + + choices = body.get("choices") or [] + if not choices: + raise RuntimeError("the model returned no choices") + content = (choices[0].get("message") or {}).get("content") or "" + content = content.strip() + if not content: + raise RuntimeError("the model returned an empty response") + return content[:MAX_ADVICE_CHARS] + + +def generate_advice(run: dict, reading: dict, history: list[dict], *, + url: str | None = None, model: str | None = None, + timeout: float | None = None) -> tuple[bool, str]: + """Generate the advice for one reading. + + Returns ``(ok, text)``. On failure ``ok`` is False and ``text`` is a + short note explaining what went wrong, so the reading carries a + visible reason instead of silently having no advice. + """ + prompt = build_prompt(run, reading, history) + try: + return True, call_ollama(prompt, url=url, model=model, timeout=timeout) + except urllib.error.HTTPError as exc: + # HTTPError is a subclass of URLError, so it has to be caught + # FIRST or it would be swallowed by the unreachable branch. + detail = "" + try: + detail = exc.read().decode("utf-8", "replace")[:300] + except Exception: + pass + log.warning("the model returned HTTP %s: %s", exc.code, detail) + return False, (f"Advice could not be generated - the model returned " + f"HTTP {exc.code}. {detail}".strip()) + except urllib.error.URLError as exc: + log.warning("the model was unreachable: %s", exc) + return False, "Advice could not be generated - the model was unreachable." + except TimeoutError: + log.warning("the model timed out") + return False, "Advice could not be generated - the model timed out." + except Exception as exc: # noqa: BLE001 - the caller must never crash + log.warning("advice generation failed: %s", exc) + return False, f"Advice could not be generated - {exc}" + + +def generate_advice_in_background(run: dict, reading: dict, + history: list[dict], writer) -> None: + """Generate the advice and hand it to *writer* when it is ready. + + *writer* is called as ``writer(reading_id, ok, text)``. It runs in a + background thread, so it must not touch the request context - the + caller is responsible for giving it a way to reach Mongo. + """ + ok, text = generate_advice(run, reading, history) + try: + writer(reading.get("_id"), ok, text) + except Exception: # noqa: BLE001 - never let the thread die noisily + log.exception("could not store the generated advice") diff --git a/src/flask/brewing.py b/src/flask/brewing.py index 65ec584..8332f01 100644 --- a/src/flask/brewing.py +++ b/src/flask/brewing.py @@ -152,6 +152,10 @@ class Reading: comment: str = "" beer_type: str = "" _id: str = "" + # The LLM's advice for this reading. It lives in its own field in + # Mongo, so the brewer's own comment is never overwritten. + advice: str = "" + advice_ok: bool = True @property def sg_decimal(self) -> float | None: @@ -171,6 +175,8 @@ class Reading: comment=(doc.get("comment") or "").strip(), beer_type=(doc.get("beer_type") or "").strip(), _id=str(doc.get("_id") or ""), + advice=(doc.get("advice") or "").strip(), + advice_ok=bool(doc.get("advice_ok", True)), ) @@ -189,6 +195,25 @@ class Run: # -- aggregate accessors ------------------------------------------------ # + def latest_advice(self) -> str: + """The advice for the most recent reading that has any. + + The advice is generated in the background, so the newest reading + may not have any yet - in which case the newest one that does is + used, rather than showing nothing. + """ + for reading in reversed(self.readings): + if reading.advice: + return reading.advice + return "" + + def latest_advice_ok(self) -> bool: + """False when the advice is a note saying it could not be generated.""" + for reading in reversed(self.readings): + if reading.advice: + return reading.advice_ok + return True + def sg_values(self) -> list[int]: """Gravity points of every *usable* reading, in chronological order.""" return [r.sg_points for r in self.readings if r.sg_points is not None] @@ -622,6 +647,10 @@ def run_summary(run: Run, today: date | None = None, "days_in_fermenter": run.days_in_fermenter(today), "comments": run.comments(), "last_comment": run.comments()[-1] if run.comments() else "", + # The advice the LLM gave for the most recent reading, if it has + # arrived yet. + "advice": run.latest_advice(), + "advice_ok": run.latest_advice_ok(), "attenuation": attenuation_percent(run), "curve": fermentation_curve(run), "outliers": [r.sg_points for r in run.outlier_readings()], @@ -633,6 +662,8 @@ def run_summary(run: Run, today: date | None = None, "final_reading": r.final_reading, "comment": r.comment, "beer_type": r.beer_type, + "advice": r.advice, + "advice_ok": r.advice_ok, } for r in run.readings ], diff --git a/src/flask/mongo/repository.py b/src/flask/mongo/repository.py index 7f74d7f..0ca5649 100644 --- a/src/flask/mongo/repository.py +++ b/src/flask/mongo/repository.py @@ -188,6 +188,27 @@ class BeerRepository: existing = self.readings.find_one(query) return str(existing.get("_id")) if existing else "" + def set_reading_advice(self, reading_id: str, advice: str, + ok: bool = True) -> int: + """Attach the LLM's advice to a reading. + + The advice lives in its own field, so the brewer's own comment is + never overwritten. ``ok`` says whether the advice was actually + generated or is a note explaining that it could not be. + """ + doc_id = _as_doc_id(reading_id) + if doc_id is None: + return 0 + result = self.readings.update_one( + {"_id": doc_id}, + {"$set": { + "advice": str(advice), + "advice_ok": bool(ok), + "advice_at": _now_iso(), + }}, + ) + return result.modified_count + def set_final_reading(self, beer_run_id: str, reading_date: str, final: bool) -> int: """Flag (or unflag) a reading as the final reading of its run.""" @@ -314,6 +335,28 @@ def _as_id(value: object) -> str: return "" +def _as_doc_id(value: object): + """Coerce a reading document id to something Mongo will match. + + The ``_id`` of a reading is an ``ObjectId``, not a run id, so the + ``_as_id`` coercion does not apply - it would turn the ObjectId into + an empty string and the update would silently match nothing. + + Only an ObjectId or a plain string is accepted, so a dict can never + be injected as a query operator. + """ + from bson import ObjectId + + if isinstance(value, ObjectId): + return value + if isinstance(value, str): + try: + return ObjectId(value) + except Exception: + return None + return None + + def _as_date(value: object) -> str: """Coerce a reading date to a plain ``YYYY-MM-DD`` string.""" if isinstance(value, str): diff --git a/src/flask/static/css/app.css b/src/flask/static/css/app.css index 6bc8a98..f99a72f 100644 --- a/src/flask/static/css/app.css +++ b/src/flask/static/css/app.css @@ -689,3 +689,65 @@ table.data .comment { color: var(--text-muted); font-size: .84rem; } .reading-list__sg { font-family: var(--mono); font-weight: 650; flex: 0 0 4rem; } .reading-list__body { flex: 1; min-width: 0; } .reading-list__comment { color: var(--text-muted); font-size: .84rem; } + +/* ------------------------------------------------------------- advice */ + +/* The LLM's advice. It sits next to the brewer's own comment, never + replaces it, and is visually distinct so the two are never confused. */ +.advice { + display: block; + margin-top: .35rem; + padding: .5rem .65rem; + border-left: 2px solid var(--amber); + border-radius: 0 4px 4px 0; + background: var(--amber-dim); + color: var(--text); + font-size: .8125rem; + line-height: 1.5; +} + +.advice__label { + display: block; + margin-bottom: .15rem; + font-size: .625rem; + font-weight: 600; + letter-spacing: .08em; + text-transform: uppercase; + color: var(--amber-text); +} + +/* Advice that could not be generated - it says why. */ +.advice--failed { + border-left-color: var(--danger); + background: var(--danger-dim); + color: var(--text-muted); +} + +.advice--failed .advice__label { + color: var(--danger); +} + +.advice-cell { + max-width: 32rem; +} + +/* In the table the advice is clamped to a few lines - the full text is + on the run detail page, where there is room to read it. */ +.advice-cell .advice { + margin-top: 0; + font-size: .75rem; + display: -webkit-box; + -webkit-line-clamp: 4; + line-clamp: 4; + -webkit-box-orient: vertical; + overflow: hidden; +} + +/* The clamped text opens up on hover, so the whole thing is readable + without leaving the page. */ +.advice-cell:hover .advice, +.advice-cell:focus-within .advice { + -webkit-line-clamp: unset; + line-clamp: unset; + overflow: visible; +} diff --git a/src/flask/templates/active.html b/src/flask/templates/active.html index d100863..3180111 100644 --- a/src/flask/templates/active.html +++ b/src/flask/templates/active.html @@ -100,6 +100,13 @@ {% endif %} + {% if run.advice %} +
+ Brewing advice + {{ run.advice }} +
+ {% endif %} +
Open run diff --git a/src/flask/templates/brews.html b/src/flask/templates/brews.html index 526b0fa..2ee2efa 100644 --- a/src/flask/templates/brews.html +++ b/src/flask/templates/brews.html @@ -83,6 +83,7 @@ Gravity Est. ABV Comment + Brewing advice @@ -111,6 +112,13 @@ — {% endif %} + + {% if run.advice %} + {{ run.advice }} + {% else %} + Thinking… + {% endif %} + Open diff --git a/src/flask/templates/run_detail.html b/src/flask/templates/run_detail.html index f1b2163..8127e85 100644 --- a/src/flask/templates/run_detail.html +++ b/src/flask/templates/run_detail.html @@ -144,6 +144,12 @@ {% else %}
No comment
{% endif %} + {% if reading.advice %} +
+ Brewing advice + {{ reading.advice }} +
+ {% endif %}
{% if reading.final_reading %} Final diff --git a/src/flask/views/brews.py b/src/flask/views/brews.py index e1c4d30..7aea1c6 100644 --- a/src/flask/views/brews.py +++ b/src/flask/views/brews.py @@ -2,15 +2,22 @@ from __future__ import annotations -from flask import (Blueprint, flash, jsonify, redirect, render_template, - request, session, url_for) +import logging +import os +import threading + +from flask import (Blueprint, current_app, flash, jsonify, redirect, + render_template, request, session, url_for) from flask_wtf import FlaskForm from wtforms import (BooleanField, DateField, IntegerField, StringField, SubmitField, TextAreaField) from wtforms.validators import DataRequired, Length, NumberRange, Optional +import brew_advice from brewing import implausible_runs, is_plausible_sg, run_summary +log = logging.getLogger(__name__) + bp = Blueprint("brews", __name__) @@ -34,6 +41,82 @@ def login_required(fn): return wrapper +def advice_enabled() -> bool: + """Whether the LLM advice should be generated. + + It is off in testing, so the suite never reaches out to a model, and + it can be turned off in production with ``LLM_ADVICE=0`` if the model + is misbehaving. + """ + if current_app.config.get("TESTING"): + return False + return os.getenv("LLM_ADVICE", "1").strip().lower() not in {"0", "false", "no"} + + +def _start_advice(run_id: str, reading_id: str) -> None: + """Generate the LLM's advice for a reading, in the background. + + The advice is a separate field on the reading, so the brewer's own + comment is never touched. + + The background thread cannot use the request-scoped repository - that + client is closed when the request ends - so it builds its own. + """ + if not reading_id: + return + if not advice_enabled(): + return + + try: + repository = repo() + run = repository.run(run_id) + if run is None: + return + meta = repository.run_metadata(run_id) + summary = run_summary(run, og_override=meta.get("og_points")) + run_info = { + "canonical_name": meta.get("name") or run.display_name(), + "name": run.display_name(), + "og_points": summary.get("og_points"), + "og_decimal": summary.get("og_decimal"), + "tap_number": meta.get("tap_number"), + "notes": meta.get("notes", ""), + } + + # The model's view of the run: every reading, with the gravity in + # both forms a brewer would recognise. + readings = [] + for reading in run.readings: + points = reading.sg_points + readings.append({ + "_id": reading._id, + "date": reading.date.isoformat() if reading.date else "", + "sg": points, + "sg_decimal": reading.sg_decimal, + "point_scale": f"{points} points" if points is not None else "", + "comment": reading.comment or "", + "final": bool(reading.final_reading), + }) + + latest = dict(readings[-1]) if readings else {"date": "", "sg": None} + latest["_id"] = reading_id + + except Exception: # noqa: BLE001 - advice must never break the save + log.exception("could not gather the reading for the advice") + return + + def write(read_id: str, ok: bool, text: str) -> None: + from mongo.repository import BeerRepository + BeerRepository().set_reading_advice(read_id, text, ok) + + thread = threading.Thread( + target=brew_advice.generate_advice_in_background, + args=(run_info, latest, readings, write), + daemon=True, + ) + thread.start() + + def _last_reading_date(run) -> str | None: """The ISO date of the most recent reading in a run.""" last = run.last_reading() @@ -104,8 +187,9 @@ def index(): "(990-1200). It was saved anyway — double-check the reading.", "warning", ) - repo().upsert_reading( - beer_run_id=str(form.beer_run_id.data), + run_id = str(form.beer_run_id.data) + reading_id = repo().upsert_reading( + beer_run_id=run_id, beer_type=(form.beer_run_type.data or "").strip(), sg=sg_points, reading_date=form.Date.data.isoformat(), @@ -113,6 +197,11 @@ def index(): comment=(form.comment.data or "").strip(), ) flash("Reading saved.", "success") + + # The advice is generated in the background: recording a reading + # must not block on the model. The page shows the reading straight + # away and the advice is there on a later visit. + _start_advice(run_id, reading_id) return redirect(url_for("brews.index")) runs = repo().runs() diff --git a/tests/test_brew_advice.py b/tests/test_brew_advice.py new file mode 100644 index 0000000..7d807c3 --- /dev/null +++ b/tests/test_brew_advice.py @@ -0,0 +1,338 @@ +"""Tests for the LLM brewing advice: the prompt, the storage and the display.""" + +from __future__ import annotations + +import json +from datetime import date + +import pytest + +import brew_advice +from conftest import seed_readings + + +# ------------------------------------------------------------ the prompt # + +def test_the_prompt_gives_the_model_the_brew_and_its_readings(): + """The model needs the name, the OG and every reading.""" + run = { + "canonical_name": "Nocturna Christmas beer", + "og_decimal": 1.042, + "tap_number": 3, + "notes": "dark malt, 19C", + } + reading = {"date": "2026-01-10", "sg_decimal": 1.022, + "point_scale": "1022 points", "comment": "no change since day 7", + "final": False} + history = [ + {"date": "2026-01-01", "sg_decimal": 1.042, "point_scale": "1042 points", + "comment": "", "final": False}, + reading, + ] + + prompt = brew_advice.build_prompt(run, reading, history) + + assert "Nocturna Christmas beer" in prompt + assert "1.042" in prompt + assert "Fermenter tap: 3" in prompt + assert "dark malt" in prompt + # Both readings, oldest first. + assert "2026-01-01" in prompt + assert "2026-01-10" in prompt + assert "no change since day 7" in prompt + assert "1.022" in prompt + + +def test_the_prompt_tells_the_model_not_to_call_a_reading_impossible(): + """A reading below 1.000 is a mis-entry, but the run may still be real. + + This matters: the brewer's own run 12 was a genuine long fermentation + that the app flagged as impossible. The model must not repeat that + mistake - it should point at the odd reading and keep talking about + the brew. + """ + assert "Never say a reading is impossible" in brew_advice.SYSTEM_PROMPT + assert "still be real" in brew_advice.SYSTEM_PROMPT + + +def test_the_prompt_marks_the_final_reading(): + run = {"canonical_name": "Ale"} + reading = {"date": "2026-01-20", "sg_decimal": 1.010, "point_scale": "1010 points", + "comment": "", "final": True} + prompt = brew_advice.build_prompt(run, reading, [reading]) + assert "FINAL reading" in prompt + + +# ---------------------------------------------------------- the LLM call # + +def test_a_successful_call_returns_the_text(monkeypatch): + """A normal reply comes back as the advice.""" + captured = {} + + def fake_urlopen(request, timeout=None): + captured["url"] = request.full_url + captured["body"] = json.loads(request.data.decode()) + + class Response: + def read(self): + return json.dumps({ + "choices": [{"message": {"content": " Leave it alone, it is still fermenting. "}}] + }).encode() + + def __enter__(self): + return self + + def __exit__(self, *args): + return False + + return Response() + + monkeypatch.setattr(brew_advice.urllib.request, "urlopen", fake_urlopen) + + ok, text = brew_advice.generate_advice( + {"canonical_name": "Ale"}, + {"date": "2026-01-01", "sg_decimal": 1.042, "point_scale": "1042 points", + "comment": "", "final": False}, + [{"date": "2026-01-01", "sg_decimal": 1.042, "point_scale": "1042 points", + "comment": "", "final": False}], + model="glm-5.3:cloud", + ) + + assert ok is True + assert text == "Leave it alone, it is still fermenting." + assert captured["url"].endswith("/v1/chat/completions") + assert captured["body"]["model"] == "glm-5.3:cloud" + + +def test_an_unreachable_model_does_not_raise(monkeypatch): + """A failure is recorded, not raised. + + The reading has to be saved either way, and a silent failure would be + indistinguishable from "the advice is still being generated". + """ + def boom(request, timeout=None): + raise brew_advice.urllib.error.URLError("connection refused") + + monkeypatch.setattr(brew_advice.urllib.request, "urlopen", boom) + + ok, text = brew_advice.generate_advice( + {"canonical_name": "Ale"}, + {"date": "2026-01-01", "sg_decimal": 1.042, "point_scale": "1042 points", + "comment": "", "final": False}, + [], + ) + + assert ok is False + assert "could not be generated" in text + + +def test_an_empty_reply_is_a_failure(monkeypatch): + """A model that returns nothing has not given advice.""" + + class Response: + def read(self): + return json.dumps({"choices": [{"message": {"content": " "}}]}).encode() + + def __enter__(self): + return self + + def __exit__(self, *args): + return False + + monkeypatch.setattr(brew_advice.urllib.request, "urlopen", + lambda request, timeout=None: Response()) + + ok, text = brew_advice.generate_advice({"canonical_name": "Ale"}, + {"date": "2026-01-01"}, []) + assert ok is False + assert "could not be generated" in text + + +def test_the_token_budget_leaves_room_for_the_reasoning(monkeypatch): + """The reasoning models emit their thinking first, out of the same budget. + + At 500 tokens the reasoning used the whole allowance and the reply + came back EMPTY - which looks like a broken model rather than a + budget problem. The budget has to cover the reasoning AND the advice. + """ + captured = {} + + def fake_urlopen(request, timeout=None): + captured["body"] = json.loads(request.data.decode()) + + class Response: + def read(self): + return json.dumps({ + "choices": [{"message": {"content": "Rouse the yeast."}}] + }).encode() + + def __enter__(self): + return self + + def __exit__(self, *args): + return False + + return Response() + + monkeypatch.setattr(brew_advice.urllib.request, "urlopen", fake_urlopen) + + brew_advice.generate_advice({"canonical_name": "Ale"}, + {"date": "2026-01-01"}, []) + + assert captured["body"]["max_tokens"] >= 2000 + + +def test_the_advice_is_truncated(): + """A runaway reply must not fill the reading document.""" + long_text = "x" * 5000 + + class Response: + def read(self): + return json.dumps({"choices": [{"message": {"content": long_text}}]}).encode() + + def __enter__(self): + return self + + def __exit__(self, *args): + return False + + import brew_advice as ba + original = ba.urllib.request.urlopen + ba.urllib.request.urlopen = lambda request, timeout=None: Response() + try: + ok, text = ba.generate_advice({"canonical_name": "Ale"}, + {"date": "2026-01-01"}, []) + finally: + ba.urllib.request.urlopen = original + + assert ok is True + assert len(text) <= ba.MAX_ADVICE_CHARS + + +# ------------------------------------------------------------- storage # + +def test_the_advice_is_stored_in_its_own_field(repo): + """The brewer's comment is never overwritten.""" + seed_readings(repo, [("1", "Ale", "1042", "2026-01-01", False, "my own comment")]) + + doc = repo.readings.find_one({"beer_run_id": "1"}) + repo.set_reading_advice(str(doc["_id"]), "The advice", ok=True) + + updated = repo.readings.find_one({"beer_run_id": "1"}) + assert updated["advice"] == "The advice" + assert updated["advice_ok"] is True + # The brewer's own comment is untouched. + assert updated["comment"] == "my own comment" + + +def test_a_failed_generation_is_stored_as_a_failure(repo): + """The note says it could not be generated, and is flagged as such.""" + seed_readings(repo, [("1", "Ale", "1042", "2026-01-01", False, "")]) + + doc = repo.readings.find_one({"beer_run_id": "1"}) + repo.set_reading_advice(str(doc["_id"]), "Advice could not be generated.", ok=False) + + updated = repo.readings.find_one({"beer_run_id": "1"}) + assert updated["advice_ok"] is False + + +def test_the_latest_advice_comes_from_the_newest_reading_with_one(): + """The advice for the newest reading wins. + + The advice is generated in the background, so the newest reading may + not have one yet - in which case the newest that does is used. + """ + from brewing import Run + + run = Run.from_docs("1", [ + {"_id": "a", "beer_run_id": "1", "sg": "1042", "date": "2026-01-01", + "final_reading": "False", "comment": "", "advice": "first advice"}, + {"_id": "b", "beer_run_id": "1", "sg": "1030", "date": "2026-01-05", + "final_reading": "False", "comment": "", "advice": "second advice"}, + {"_id": "c", "beer_run_id": "1", "sg": "1020", "date": "2026-01-10", + "final_reading": "False", "comment": "", "advice": ""}, + ]) + + assert run.latest_advice() == "second advice" + assert run.latest_advice_ok() is True + + +# ------------------------------------------------------------- the pages # + +def test_recording_a_reading_saves_it_even_when_the_model_is_down(client, repo): + """The model being down must never stop a reading being saved.""" + repo.users.insert_one({"username": "beer", "password": "beer_pass"}) + client.post("/login", data={"username": "beer", "password": "beer_pass"}, + follow_redirects=True) + + response = client.post("/brews", data={ + "beer_run_id": "99", + "beer_run_type": "Test ale", + "Date": "2026-01-01", + "sg": "1042", + "comment": "a comment", + }, follow_redirects=True) + + assert response.status_code == 200 + assert repo.readings.count_documents({"beer_run_id": "99"}) == 1 + + +def test_the_advice_is_shown_on_the_brewing_page(client, repo): + """The advice appears next to the comment, clearly labelled.""" + repo.users.insert_one({"username": "beer", "password": "beer_pass"}) + client.post("/login", data={"username": "beer", "password": "beer_pass"}, + follow_redirects=True) + + seed_readings(repo, [("1", "Ale", "1042", "2026-01-01", False, "my comment")]) + doc = repo.readings.find_one({"beer_run_id": "1"}) + repo.set_reading_advice(str(doc["_id"]), "Rouse the yeast and wait.", ok=True) + + response = client.get("/brews") + body = response.get_data(as_text=True) + + assert "Rouse the yeast and wait." in body + # The brewer's own comment is still there. + assert "my comment" in body + # And it is labelled as advice, so the two are not confused. + assert "Brewing advice" in body + + +def test_the_advice_is_shown_on_the_active_beers_page(client, repo): + """The active page is where the brewer looks during a fermentation.""" + repo.users.insert_one({"username": "beer", "password": "beer_pass"}) + client.post("/login", data={"username": "beer", "password": "beer_pass"}, + follow_redirects=True) + + seed_readings(repo, [("1", "Ale", "1042", "2026-01-01", False, "my comment")]) + doc = repo.readings.find_one({"beer_run_id": "1"}) + repo.set_reading_advice(str(doc["_id"]), "Warm it to 21C and wait.", ok=True) + + response = client.get("/active") + body = response.get_data(as_text=True) + + assert "Warm it to 21C and wait." in body + assert "Brewing advice" in body + + +def test_a_failed_advice_is_marked_as_such(client, repo): + """A generation failure reads differently from real advice.""" + repo.users.insert_one({"username": "beer", "password": "beer_pass"}) + client.post("/login", data={"username": "beer", "password": "beer_pass"}, + follow_redirects=True) + + seed_readings(repo, [("1", "Ale", "1042", "2026-01-01", False, "")]) + doc = repo.readings.find_one({"beer_run_id": "1"}) + repo.set_reading_advice(str(doc["_id"]), "Advice could not be generated.", + ok=False) + + response = client.get("/brews") + body = response.get_data(as_text=True) + assert "advice--failed" in body + + +def test_advice_is_not_generated_in_tests(client, repo): + """The suite must never reach out to a real model.""" + from views.brews import advice_enabled + + with client.application.app_context(): + assert advice_enabled() is False From d0aef7a787da822fe4727c2fae073877a9ab8d23 Mon Sep 17 00:00:00 2001 From: hermes Date: Mon, 9 Nov 2026 16:05:59 +1000 Subject: [PATCH 02/17] fix(advice): clear stale advice on edit, and pin the injection guard 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). --- src/flask/mongo/repository.py | 8 +++++- tests/test_brew_advice.py | 46 +++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/src/flask/mongo/repository.py b/src/flask/mongo/repository.py index 0ca5649..1de2779 100644 --- a/src/flask/mongo/repository.py +++ b/src/flask/mongo/repository.py @@ -180,7 +180,13 @@ class BeerRepository: "date": _as_date(reading_date), "final_reading": "True" if final_reading else "False", "comment": str(comment), - } + }, + # The advice was written for the gravity that was there when + # it was generated. If the reading has been edited, that + # advice no longer describes it, so it is cleared and a fresh + # one is generated. (Advice is only ever cleared here - never + # the brewer's comment.) + "$unset": {"advice": "", "advice_ok": "", "advice_at": ""}, } result = self.readings.update_one(query, update, upsert=True) if result.upserted_id is not None: diff --git a/tests/test_brew_advice.py b/tests/test_brew_advice.py index 7d807c3..3485594 100644 --- a/tests/test_brew_advice.py +++ b/tests/test_brew_advice.py @@ -225,6 +225,52 @@ def test_the_advice_is_stored_in_its_own_field(repo): assert updated["comment"] == "my own comment" +def test_editing_a_reading_clears_its_stale_advice(repo): + """The advice was written for the gravity that was there before. + + If the reading is edited, that advice no longer describes it, so it + is cleared and a fresh one is generated. The brewer's own comment is + never cleared. + """ + seed_readings(repo, [("1", "Ale", "1042", "2026-01-01", False, "my comment")]) + + doc = repo.readings.find_one({"beer_run_id": "1"}) + repo.set_reading_advice(str(doc["_id"]), "THE ADVICE", True) + + # Edit the reading. + repo.upsert_reading(beer_run_id="1", beer_type="Ale", sg=1030, + reading_date="2026-01-01", final_reading=False, + comment="my comment") + + after = repo.readings.find_one({"beer_run_id": "1"}) + assert after.get("advice") is None, "the stale advice was kept" + assert after["comment"] == "my comment", "the brewer's comment was touched" + assert after["sg"] == "1030" + + +def test_the_reading_id_cannot_be_used_to_inject_a_query(repo): + """The reading id is coerced before it reaches Mongo. + + Every value that reaches a Mongo query is coerced first, because a + caller passing a dict would have it interpreted as a query operator + rather than a value - the classic NoSQL injection route. A reading + id that is not a plain ObjectId or string must match nothing. + """ + seed_readings(repo, [ + ("1", "Ale", "1042", "2026-01-01", False, ""), + ("2", "Ale", "1030", "2026-01-01", False, ""), + ]) + + for hostile in [{"$ne": None}, {"$gt": ""}, ["a"], 123, None, + "not-an-objectid", ""]: + modified = repo.set_reading_advice(hostile, "INJECTED", True) + assert modified == 0, f"{hostile!r} matched something" + + # And no document was touched. + for doc in repo.readings.find({}): + assert doc.get("advice") is None + + def test_a_failed_generation_is_stored_as_a_failure(repo): """The note says it could not be generated, and is flagged as such.""" seed_readings(repo, [("1", "Ale", "1042", "2026-01-01", False, "")]) From 6dd0b7b046438fcaa4362b3d2891e6b04cd9036c Mon Sep 17 00:00:00 2001 From: hermes Date: Mon, 9 Nov 2026 16:29:39 +1000 Subject: [PATCH 03/17] fix(advice): the review findings - the ids, the client leak, the wrong reading 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). --- src/flask/mongo/repository.py | 78 ++++++++++++++++-- src/flask/views/brews.py | 28 ++++++- tests/test_brew_advice.py | 148 ++++++++++++++++++++++++++++++++++ 3 files changed, 242 insertions(+), 12 deletions(-) diff --git a/src/flask/mongo/repository.py b/src/flask/mongo/repository.py index 1de2779..0044244 100644 --- a/src/flask/mongo/repository.py +++ b/src/flask/mongo/repository.py @@ -46,6 +46,33 @@ class BeerRepository: self.run_meta = self.db["beer_runs"] self._ensure_indexes() + def close(self) -> None: + """Close the underlying Mongo client and release it. + + A ``MongoClient`` keeps a connection pool and spawns three + monitoring threads. ``close()`` alone does NOT reap them - they + go when the client object is garbage collected - so the + reference is dropped as well. + + The request-scoped repository is closed by the app's teardown, but + a caller that builds its own - the advice background thread, for + instance - has to close it itself or the threads and the pool + accumulate, three per reading. + """ + client = self.client + try: + client.close() + except Exception: + pass + # Drop the reference so the monitor threads can be reaped. + self.client = None + + def __enter__(self) -> "BeerRepository": + return self + + def __exit__(self, *exc_info) -> None: + self.close() + def _ensure_indexes(self) -> None: """Make sure the lookup keys are indexed. @@ -205,8 +232,10 @@ class BeerRepository: doc_id = _as_doc_id(reading_id) if doc_id is None: return 0 + # `$in` matches either form. A plain list would be an exact + # ARRAY match in Mongo, which is not what is wanted here. result = self.readings.update_one( - {"_id": doc_id}, + {"_id": {"$in": doc_id}}, {"$set": { "advice": str(advice), "advice_ok": bool(ok), @@ -341,12 +370,42 @@ def _as_id(value: object) -> str: return "" +#: A process-wide repository, for the code that runs outside a request. +#: +#: A ``MongoClient`` spawns three monitor threads and pymongo does not +#: reap them on ``close()`` - they go only when the client is garbage +#: collected. So building a client per advice write accumulated three +#: threads and a connection pool for every reading recorded. One shared +#: client for the life of the process is what the request-scoped +#: repository does too. +_shared_repository: "BeerRepository | None" = None + + +def get_shared_repository() -> "BeerRepository": + """The process-wide repository, built on first use.""" + global _shared_repository + + if _shared_repository is None: + _shared_repository = BeerRepository() + return _shared_repository + + def _as_doc_id(value: object): """Coerce a reading document id to something Mongo will match. - The ``_id`` of a reading is an ``ObjectId``, not a run id, so the - ``_as_id`` coercion does not apply - it would turn the ObjectId into - an empty string and the update would silently match nothing. + The ``_id`` of a reading is not a run id, so the ``_as_id`` coercion + does not apply - it would turn the id into an empty string and the + update would silently match nothing. + + Two shapes exist in this database: + + - the ORIGINAL app's ids are 24 random CHARACTERS stored as a string + (``l78ZidBJ3QaYn98tvB88x4uM``), and + - ids written by Mongo itself are ``ObjectId`` objects. + + So the id is returned in BOTH forms and the query matches either. + Accepting only the ObjectId form would mean the advice was never + stored for any of the existing readings. Only an ObjectId or a plain string is accepted, so a dict can never be injected as a query operator. @@ -354,12 +413,15 @@ def _as_doc_id(value: object): from bson import ObjectId if isinstance(value, ObjectId): - return value - if isinstance(value, str): + return [value] + if isinstance(value, str) and value: try: - return ObjectId(value) + # A 24-character hex string may or may not be the stored + # ObjectId or the stored string, so try both. + return [ObjectId(value), value] except Exception: - return None + # Not hex - it can only be the original app's string id. + return [value] return None diff --git a/src/flask/views/brews.py b/src/flask/views/brews.py index 7aea1c6..bb85916 100644 --- a/src/flask/views/brews.py +++ b/src/flask/views/brews.py @@ -89,7 +89,7 @@ def _start_advice(run_id: str, reading_id: str) -> None: for reading in run.readings: points = reading.sg_points readings.append({ - "_id": reading._id, + "_id": str(reading._id), "date": reading.date.isoformat() if reading.date else "", "sg": points, "sg_decimal": reading.sg_decimal, @@ -98,7 +98,14 @@ def _start_advice(run_id: str, reading_id: str) -> None: "final": bool(reading.final_reading), }) - latest = dict(readings[-1]) if readings else {"date": "", "sg": None} + # The reading that is actually being advised about. It is not + # necessarily the newest one: the brewer may be editing an older + # reading, and advice about the newest reading would be about the + # wrong number. + saved = next((r for r in readings if r.get("_id") == str(reading_id)), None) + if saved is None: + saved = dict(readings[-1]) if readings else {"date": "", "sg": None} + latest = dict(saved) latest["_id"] = reading_id except Exception: # noqa: BLE001 - advice must never break the save @@ -106,8 +113,21 @@ def _start_advice(run_id: str, reading_id: str) -> None: return def write(read_id: str, ok: bool, text: str) -> None: - from mongo.repository import BeerRepository - BeerRepository().set_reading_advice(read_id, text, ok) + # The client is SHARED, not built per reading. A MongoClient + # spawns three monitor threads and pymongo does not reap them on + # close(), so building one per advice write accumulated three + # threads and a connection pool for every reading recorded. + # + # The shared client is created on first use and lives for the + # life of the process, which is what the request-scoped + # repository does too. + from mongo.repository import BeerRepository, get_shared_repository + + try: + shared = get_shared_repository() + shared.set_reading_advice(read_id, text, ok) + except Exception: # noqa: BLE001 - never let the thread die noisily + log.exception("could not store the generated advice") thread = threading.Thread( target=brew_advice.generate_advice_in_background, diff --git a/tests/test_brew_advice.py b/tests/test_brew_advice.py index 3485594..f67deae 100644 --- a/tests/test_brew_advice.py +++ b/tests/test_brew_advice.py @@ -3,6 +3,7 @@ from __future__ import annotations import json +from contextlib import contextmanager from datetime import date import pytest @@ -248,6 +249,153 @@ def test_editing_a_reading_clears_its_stale_advice(repo): assert after["sg"] == "1030" +def test_the_advice_is_stored_on_a_reading_with_a_string_id(repo): + """The original app's readings have 24-character STRING ids. + + This is the shape every existing reading in the live database has - + the ids were generated by the old app as random characters, not by + Mongo. A helper that only accepted an ObjectId would have matched + nothing, and the advice would silently never be stored. + + The mongomock fixture generates ObjectIds, so this has to be + constructed explicitly or the bug is invisible in the tests. + """ + repo.readings.insert_one({ + "_id": "l78ZidBJ3QaYn98tvB88x4uM", + "beer_run_id": "1", + "beer_type": "Ale", + "sg": "1042", + "date": "2026-01-01", + "final_reading": "False", + "comment": "First ever try", + }) + + modified = repo.set_reading_advice("l78ZidBJ3QaYn98tvB88x4uM", + "THE ADVICE", True) + assert modified == 1, "the advice was not stored on a string-id reading" + + after = repo.readings.find_one({"_id": "l78ZidBJ3QaYn98tvB88x4uM"}) + assert after["advice"] == "THE ADVICE" + assert after["comment"] == "First ever try" + + +def test_the_advice_is_stored_on_a_reading_with_an_object_id(repo): + """The other shape: a reading written by Mongo itself.""" + seed_readings(repo, [("1", "Ale", "1042", "2026-01-01", False, "a comment")]) + doc = repo.readings.find_one({"beer_run_id": "1"}) + + modified = repo.set_reading_advice(str(doc["_id"]), "THE ADVICE", True) + assert modified == 1 + + after = repo.readings.find_one({"beer_run_id": "1"}) + assert after["advice"] == "THE ADVICE" + + +def test_writing_the_advice_does_not_leak_a_client_per_reading(): + """A MongoClient spawns three monitor threads and pymongo does not + reap them on close() - they go only when the client is collected. + + So building a client per advice write accumulated three threads and a + connection pool for every reading recorded. The advice writer shares + one client for the life of the process instead. + """ + import inspect + + from views import brews as brews_module + + # The writer must NOT build a repository of its own - it has to use + # the shared one. A MongoClient spawns three monitor threads and + # pymongo does not reap them on close(), so a client per advice write + # accumulated three threads and a connection pool for every reading + # recorded. + source = inspect.getsource(brews_module._start_advice) + + assert "BeerRepository()" not in source, ( + "the advice writer builds its own repository - that leaks a Mongo " + "client (three monitor threads) per reading" + ) + assert "get_shared_repository" in source, ( + "the advice writer must use the shared repository" + ) + + +@contextmanager +def _app_with_repo(repo): + """An application context whose repository is *repo*. + + The view reads the repository from ``app.extensions``, so pointing + that at a fixture is enough to drive the view's code directly. + """ + from app_factory import create_app + + app = create_app({ + "TESTING": True, + "WTF_CSRF_ENABLED": False, + "SECRET_KEY": "test-secret", + "MONGO_CLIENT": repo.client, + }) + # The advice must be generated even though this is a test app. + app.config["TESTING"] = False + + original = app.extensions["beer_repo"] + app.extensions["beer_repo"] = lambda: repo + with app.app_context(): + try: + yield app + finally: + app.extensions["beer_repo"] = original + + +def test_the_advice_is_about_the_reading_that_was_saved(): + """Editing an OLDER reading must not generate advice about the newest. + + The advice is written against a specific reading, so the reading being + saved has to be the one the advice is about. + """ + import time + + import mongomock + from mongo.repository import BeerRepository + from views import brews as brews_module + from views.brews import _start_advice + + captured = {} + + def fake_generate(run, reading, history, writer=None): + captured["reading"] = reading + captured["history"] = history + return True, "THE ADVICE" + + original = brews_module.brew_advice.generate_advice_in_background + brews_module.brew_advice.generate_advice_in_background = fake_generate + try: + # Two readings; the OLDER one is the one being saved. + repo = BeerRepository(client=mongomock.MongoClient()) + old_id = repo.upsert_reading(beer_run_id="1", beer_type="Ale", sg=1042, + reading_date="2026-01-01", final_reading=False, + comment="first") + repo.upsert_reading(beer_run_id="1", beer_type="Ale", sg=1030, + reading_date="2026-01-10", final_reading=False, + comment="second") + + # The advice is only generated when it is enabled, and it is off + # in testing. + with _app_with_repo(repo): + _start_advice("1", old_id) + for _ in range(40): + time.sleep(0.05) + if captured: + break + finally: + brews_module.brew_advice.generate_advice_in_background = original + + assert captured, "the advice was never generated" + assert captured["reading"]["_id"] == old_id + assert captured["reading"]["sg"] == 1042, ( + "the advice was generated about the wrong reading" + ) + + def test_the_reading_id_cannot_be_used_to_inject_a_query(repo): """The reading id is coerced before it reaches Mongo. From 21fa0d9a12828b47b7e3fdbe1263b73a1d42bd53 Mon Sep 17 00:00:00 2001 From: hermes Date: Mon, 9 Nov 2026 16:31:05 +1000 Subject: [PATCH 04/17] fix(advice): make the id helper unusable rather than merely rejected _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. --- src/flask/mongo/repository.py | 8 ++++++-- tests/test_brew_advice.py | 8 ++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/flask/mongo/repository.py b/src/flask/mongo/repository.py index 0044244..5aceb89 100644 --- a/src/flask/mongo/repository.py +++ b/src/flask/mongo/repository.py @@ -230,7 +230,7 @@ class BeerRepository: generated or is a note explaining that it could not be. """ doc_id = _as_doc_id(reading_id) - if doc_id is None: + if not doc_id: return 0 # `$in` matches either form. A plain list would be an exact # ARRAY match in Mongo, which is not what is wanted here. @@ -422,7 +422,11 @@ def _as_doc_id(value: object): except Exception: # Not hex - it can only be the original app's string id. return [value] - return None + # Anything else - a dict, a list, an int, None - is not a reading + # id. An EMPTY LIST is returned rather than None: it is a valid + # `$in` argument that matches nothing, so the helper cannot be + # misused into a query error or into matching every document. + return [] def _as_date(value: object) -> str: diff --git a/tests/test_brew_advice.py b/tests/test_brew_advice.py index f67deae..7098057 100644 --- a/tests/test_brew_advice.py +++ b/tests/test_brew_advice.py @@ -414,6 +414,14 @@ def test_the_reading_id_cannot_be_used_to_inject_a_query(repo): modified = repo.set_reading_advice(hostile, "INJECTED", True) assert modified == 0, f"{hostile!r} matched something" + # The helper itself must return something that is safe to put in a + # query - an empty list matches nothing. Returning None would raise + # "$in needs an array" if it were ever used without a guard. + from mongo.repository import _as_doc_id + + for hostile in [{"$ne": None}, {"$gt": ""}, ["a"], 123, None, ""]: + assert _as_doc_id(hostile) == [], f"{hostile!r} produced a usable id" + # And no document was touched. for doc in repo.readings.find({}): assert doc.get("advice") is None From 0011f30e599774205b048ca05154b5ec1cdacdea Mon Sep 17 00:00:00 2001 From: hermes Date: Mon, 9 Nov 2026 17:04:57 +1000 Subject: [PATCH 05/17] fix(advice): a race in the shared repository, and the prompt naming the wrong reading 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). --- src/flask/brew_advice.py | 50 +++++++++++++++---- src/flask/mongo/repository.py | 22 ++++++++- src/flask/views/brews.py | 14 ++++-- tests/test_brew_advice.py | 90 ++++++++++++++++++++++++++++++++++- 4 files changed, 160 insertions(+), 16 deletions(-) diff --git a/src/flask/brew_advice.py b/src/flask/brew_advice.py index 222c21a..da914dc 100644 --- a/src/flask/brew_advice.py +++ b/src/flask/brew_advice.py @@ -67,6 +67,9 @@ temperature? - Should they rack, add more hops, raise the temperature, or wait? Rules: +- Advise about the reading that is marked as the one to advise on. It + may not be the most recent one - the brewer may be correcting an older + reading - and the advice must be about the reading they asked about. - If everything looks normal, say so briefly and say what to expect next. - If something looks wrong, say what it is and what to do about it. - Be specific to the numbers given. Do not give generic advice. @@ -81,8 +84,13 @@ Write two to five sentences of plain prose. """ -def _reading_lines(readings: list[dict]) -> str: - """The readings, as the compact list the model reads.""" +def _reading_lines(readings: list[dict], subject_id: str | None = None) -> str: + """The readings, as the compact list the model reads. + + The reading being advised about is marked, so the model does not have + to work out which one from the dates - the brewer may be correcting + an older reading. + """ lines = [] for r in readings: parts = [f"- {r.get('date')}: gravity {r.get('sg_decimal')} ({r.get('point_scale')})"] @@ -90,12 +98,19 @@ def _reading_lines(readings: list[dict]) -> str: parts.append(f"brewer's comment: {r['comment']}") if r.get("final"): parts.append("(marked as the FINAL reading)") + if subject_id is not None and str(r.get("_id")) == str(subject_id): + parts.append("<<< THIS IS THE READING TO ADVISE ON") lines.append(" ".join(parts)) return "\n".join(lines) -def build_prompt(run: dict, reading: dict, history: list[dict]) -> str: - """The user-message content for one observation.""" +def build_prompt(run: dict, reading: dict, history: list[dict], + corrected: bool = False) -> str: + """The user-message content for one observation. + + *corrected* says the reading has just been edited, so the advice + should take that into account. + """ name = run.get("canonical_name") or run.get("name") or "this brew" og = run.get("og_decimal") if og is None and run.get("og_points"): @@ -113,10 +128,23 @@ def build_prompt(run: dict, reading: dict, history: list[dict]) -> str: parts.append("") parts.append("Readings so far (oldest first):") - parts.append(_reading_lines(history)) + parts.append(_reading_lines(history, subject_id=reading.get("_id"))) parts.append("") - parts.append("Give advice for the most recent reading.") + # The reading being advised about is NAMED, not just described as + # "the most recent one". The brewer may be editing an older reading, + # and the advice is about THAT reading - so the prompt has to say + # which one, or the model advises about the newest instead. + parts.append( + f"Give advice about the reading dated {reading.get('date') or 'unknown'}" + f" (gravity {reading.get('sg_decimal')})," + + (" which the brewer has just corrected." if corrected else "") + ) + if corrected: + parts.append( + "It is the reading they have just corrected, so it may differ " + "from what it said before." + ) return "\n".join(parts) @@ -139,7 +167,7 @@ def call_ollama(prompt: str, *, url: str | None = None, model: str | None = None # allowance and the reply came back empty - which looked like a # broken model rather than a budget problem. 2000 leaves room for # the reasoning AND the advice. - "max_tokens": 2000, + "max_tokens": 3000, "stream": False, }).encode("utf-8") @@ -163,6 +191,7 @@ def call_ollama(prompt: str, *, url: str | None = None, model: str | None = None def generate_advice(run: dict, reading: dict, history: list[dict], *, + corrected: bool = False, url: str | None = None, model: str | None = None, timeout: float | None = None) -> tuple[bool, str]: """Generate the advice for one reading. @@ -171,7 +200,7 @@ def generate_advice(run: dict, reading: dict, history: list[dict], *, short note explaining what went wrong, so the reading carries a visible reason instead of silently having no advice. """ - prompt = build_prompt(run, reading, history) + prompt = build_prompt(run, reading, history, corrected=corrected) try: return True, call_ollama(prompt, url=url, model=model, timeout=timeout) except urllib.error.HTTPError as exc: @@ -197,14 +226,15 @@ def generate_advice(run: dict, reading: dict, history: list[dict], *, def generate_advice_in_background(run: dict, reading: dict, - history: list[dict], writer) -> None: + history: list[dict], writer, + corrected: bool = False) -> None: """Generate the advice and hand it to *writer* when it is ready. *writer* is called as ``writer(reading_id, ok, text)``. It runs in a background thread, so it must not touch the request context - the caller is responsible for giving it a way to reach Mongo. """ - ok, text = generate_advice(run, reading, history) + ok, text = generate_advice(run, reading, history, corrected=corrected) try: writer(reading.get("_id"), ok, text) except Exception: # noqa: BLE001 - never let the thread die noisily diff --git a/src/flask/mongo/repository.py b/src/flask/mongo/repository.py index 5aceb89..e0d31e8 100644 --- a/src/flask/mongo/repository.py +++ b/src/flask/mongo/repository.py @@ -19,6 +19,7 @@ from __future__ import annotations import logging import os +import threading from typing import Any from pymongo import MongoClient @@ -380,13 +381,30 @@ def _as_id(value: object) -> str: #: repository does too. _shared_repository: "BeerRepository | None" = None +#: Guards the check-then-set above. +#: +#: Without it, concurrent advice threads each saw ``None`` and each +#: built their own repository - so the fix reintroduced the very leak it +#: was meant to remove, just bounded by a race instead of by the number +#: of readings. Eight threads on a cold process built eight clients and +#: twenty-five threads. +_shared_repository_lock = threading.Lock() + def get_shared_repository() -> "BeerRepository": - """The process-wide repository, built on first use.""" + """The process-wide repository, built on first use. + + Thread-safe: the first caller builds it, the rest wait and get the + same one. + """ global _shared_repository if _shared_repository is None: - _shared_repository = BeerRepository() + with _shared_repository_lock: + # Checked again inside the lock - another thread may have + # built it while this one was waiting. + if _shared_repository is None: + _shared_repository = BeerRepository() return _shared_repository diff --git a/src/flask/views/brews.py b/src/flask/views/brews.py index bb85916..6b00c07 100644 --- a/src/flask/views/brews.py +++ b/src/flask/views/brews.py @@ -53,7 +53,7 @@ def advice_enabled() -> bool: return os.getenv("LLM_ADVICE", "1").strip().lower() not in {"0", "false", "no"} -def _start_advice(run_id: str, reading_id: str) -> None: +def _start_advice(run_id: str, reading_id: str, corrected: bool = False) -> None: """Generate the LLM's advice for a reading, in the background. The advice is a separate field on the reading, so the brewer's own @@ -131,7 +131,7 @@ def _start_advice(run_id: str, reading_id: str) -> None: thread = threading.Thread( target=brew_advice.generate_advice_in_background, - args=(run_info, latest, readings, write), + args=(run_info, latest, readings, write, corrected), daemon=True, ) thread.start() @@ -208,6 +208,14 @@ def index(): "warning", ) run_id = str(form.beer_run_id.data) + # Whether the reading was already there: a re-submitted reading + # is a CORRECTION, and the advice should say so. + existing = repo().reading_docs_for_run(run_id) + was_existing = any( + str(doc.get("date")) == form.Date.data.isoformat() + for doc in existing + ) + reading_id = repo().upsert_reading( beer_run_id=run_id, beer_type=(form.beer_run_type.data or "").strip(), @@ -221,7 +229,7 @@ def index(): # The advice is generated in the background: recording a reading # must not block on the model. The page shows the reading straight # away and the advice is there on a later visit. - _start_advice(run_id, reading_id) + _start_advice(run_id, reading_id, corrected=was_existing) return redirect(url_for("brews.index")) runs = repo().runs() diff --git a/tests/test_brew_advice.py b/tests/test_brew_advice.py index 7098057..3a12d0f 100644 --- a/tests/test_brew_advice.py +++ b/tests/test_brew_advice.py @@ -56,6 +56,46 @@ def test_the_prompt_tells_the_model_not_to_call_a_reading_impossible(): assert "still be real" in brew_advice.SYSTEM_PROMPT +def test_the_prompt_names_the_reading_to_advise_about(): + """The reading being advised about must be NAMED. + + The brewer may be correcting an OLDER reading, so "the most recent + reading" is the wrong one - and the advice would be about the wrong + number. The reading is marked in the list and named in the + instruction. + """ + run = {"canonical_name": "Ale"} + older = {"_id": "old", "date": "2026-01-01", "sg_decimal": 1.042, + "point_scale": "1042 points", "comment": "", "final": False} + newer = {"_id": "new", "date": "2026-01-10", "sg_decimal": 1.030, + "point_scale": "1030 points", "comment": "", "final": False} + + prompt = brew_advice.build_prompt(run, older, [older, newer]) + + # The older one is the subject. + assert "THIS IS THE READING TO ADVISE ON" in prompt + # And it is named, with its date and gravity. + assert "2026-01-01" in prompt + assert "1.042" in prompt + # And NOT the newer one. + assert "1.03" in prompt # it is in the history, which is right + assert prompt.count("THIS IS THE READING TO ADVISE ON") == 1 + + +def test_the_prompt_says_when_a_reading_has_been_corrected(): + """A corrected reading may differ from what it said before.""" + run = {"canonical_name": "Ale"} + reading = {"_id": "r1", "date": "2026-01-01", "sg_decimal": 1.012, + "point_scale": "1012 points", "comment": "", "final": False} + + plain = brew_advice.build_prompt(run, reading, [reading]) + assert "just corrected" not in plain + + corrected = brew_advice.build_prompt(run, reading, [reading], corrected=True) + assert "just corrected" in corrected + assert "may differ" in corrected + + def test_the_prompt_marks_the_final_reading(): run = {"canonical_name": "Ale"} reading = {"date": "2026-01-20", "sg_decimal": 1.010, "point_scale": "1010 points", @@ -361,9 +401,10 @@ def test_the_advice_is_about_the_reading_that_was_saved(): captured = {} - def fake_generate(run, reading, history, writer=None): + def fake_generate(run, reading, history, writer=None, corrected=False): captured["reading"] = reading captured["history"] = history + captured["corrected"] = corrected return True, "THE ADVICE" original = brews_module.brew_advice.generate_advice_in_background @@ -396,6 +437,53 @@ def test_the_advice_is_about_the_reading_that_was_saved(): ) +def test_the_shared_repository_is_built_only_once(): + """Concurrent advice threads must not each build their own client. + + An unsynchronised check-then-set on a module global let eight threads + on a cold process each build their own repository - so the fix + reintroduced the very leak it was meant to remove, just bounded by a + race instead of by the number of readings. + """ + import threading + + import mongomock + import mongo.repository as R + + original_shared = R._shared_repository + built = [] + original_init = R.BeerRepository.__init__ + + def counting_init(self, *args, **kwargs): + built.append(1) + original_init(self, *args, **kwargs) + + R._shared_repository = None + R.BeerRepository.__init__ = counting_init + original_build = R._build_client + R._build_client = lambda: mongomock.MongoClient() + try: + instances = [] + barrier = threading.Barrier(8) + + def go(): + barrier.wait() # release all eight at once + instances.append(id(R.get_shared_repository())) + + threads = [threading.Thread(target=go) for _ in range(8)] + for t in threads: + t.start() + for t in threads: + t.join() + finally: + R._shared_repository = original_shared + R.BeerRepository.__init__ = original_init + R._build_client = original_build + + assert len(built) == 1, f"built {len(built)} repositories, expected 1" + assert len(set(instances)) == 1, "the threads got different repositories" + + def test_the_reading_id_cannot_be_used_to_inject_a_query(repo): """The reading id is coerced before it reaches Mongo. From cf08b751e48a55ea23158a35a2f85d4e8ef37706 Mon Sep 17 00:00:00 2001 From: hermes Date: Mon, 9 Nov 2026 17:10:24 +1000 Subject: [PATCH 06/17] feat(advice): use mistral-large-4, and cope with models that show their working 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). --- src/flask/brew_advice.py | 144 ++++++++++++++++++++++++++++++++++---- tests/test_brew_advice.py | 37 +++++++++- 2 files changed, 168 insertions(+), 13 deletions(-) diff --git a/src/flask/brew_advice.py b/src/flask/brew_advice.py index da914dc..eeec114 100644 --- a/src/flask/brew_advice.py +++ b/src/flask/brew_advice.py @@ -33,14 +33,25 @@ log = logging.getLogger(__name__) #: The Ollama instance on the homelab. Reachable from the pod. DEFAULT_OLLAMA_URL = os.getenv("OLLAMA_URL", "http://192.168.178.45:11434") -#: A model that is known to work. `deepseek-v4-flash:cloud`, -#: `deepseek-v3.2:cloud` and `qwen3.5:cloud` are all retired and fail at -#: call time, so they are not safe defaults. -DEFAULT_MODEL = os.getenv("OLLAMA_MODEL", "glm-5.3:cloud") +#: The model to use. Override with the ``OLLAMA_MODEL`` environment +#: variable. +#: +#: A model that does NOT reason is much better here. The reasoning +#: models emit their thinking out of the same token allowance as the +#: answer, and several of them burned the whole budget on thinking and +#: returned an EMPTY reply - which looks like a broken model rather than +#: a budget problem. With `think: false` the model still shows its +#: working, but inline in the content, so the answer can be taken from +#: the end of it. +#: +#: `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, so the default is one that was probed. +DEFAULT_MODEL = os.getenv("OLLAMA_MODEL", "mistral-large-4:cloud") #: How long to wait for the model before giving up. The advice is #: generated in the background, so this only bounds the background thread. -DEFAULT_TIMEOUT = float(os.getenv("LLM_TIMEOUT", "45")) +DEFAULT_TIMEOUT = float(os.getenv("LLM_TIMEOUT", "120")) #: The advice is advice, not instruction. Keep it short - it has to be #: readable on the brewing page next to the brewer's own comment. @@ -163,11 +174,14 @@ def call_ollama(prompt: str, *, url: str | None = None, model: str | None = None ], "temperature": 0.4, # The reasoning models emit their thinking first, and it comes out - # of the same budget. At 500 tokens the reasoning used the whole - # allowance and the reply came back empty - which looked like a - # broken model rather than a budget problem. 2000 leaves room for - # the reasoning AND the advice. - "max_tokens": 3000, + # of the SAME budget as the answer. At 500 tokens the reasoning + # used the whole allowance and the reply came back empty - which + # looks like a broken model rather than a budget problem. + # + # `think: false` does NOT help: the models still think, they just + # put the working in the content instead of a separate field. + # So the budget has to cover the reasoning AND the advice. + "max_tokens": 8000, "stream": False, }).encode("utf-8") @@ -183,11 +197,117 @@ def call_ollama(prompt: str, *, url: str | None = None, model: str | None = None choices = body.get("choices") or [] if not choices: raise RuntimeError("the model returned no choices") - content = (choices[0].get("message") or {}).get("content") or "" + message = choices[0].get("message") or {} + content = message.get("content") or "" content = content.strip() if not content: raise RuntimeError("the model returned an empty response") - return content[:MAX_ADVICE_CHARS] + return _strip_reasoning(content)[:MAX_ADVICE_CHARS] + + +#: The reasoning models sometimes put their working IN the content, even +#: with thinking turned off - "Let me look at this..." for several +#: thousand characters before the actual advice. The advice is the last +#: few paragraphs, so the preamble is cut off. +_PREAMBLE_MARKERS = ( + "let me look", + "let me think", + "let me work", + "okay, let me", + "first, let me", + "hmm,", + "wait,", + "let me analyze", + "let me consider", +) + + +def _strip_reasoning(text: str) -> str: + """Cut a reasoning preamble off the front of a reply. + + The advice is the prose at the END - the working comes first, in + several paragraphs. So the preamble is removed by finding the last + paragraph break before the advice starts, not the first. + + Only a preamble that looks like working is removed; a reply that + starts straight into the advice is left alone. + """ + lowered = text.lower() + for marker in _PREAMBLE_MARKERS: + index = lowered.find(marker) + # index 0 is fine - a reply can START with "Let me look at this". + if index < 0: + continue + + tail = text[index:] + # The advice is the LAST paragraph of the tail. Split on the + # blank lines and take the paragraphs from the end, until the + # running text stops looking like working. + paragraphs = [p.strip() for p in tail.split("\n\n") if p.strip()] + if len(paragraphs) < 2: + continue + + # Walk back from the end while the paragraph still reads like the + # model's working rather than the brewer's advice. + kept = [paragraphs[-1]] + for paragraph in reversed(paragraphs[:-1]): + if _looks_like_advice(paragraph): + kept.append(paragraph) + else: + break + kept.reverse() + + candidate = "\n\n".join(kept).strip() + if len(candidate) >= 80: + return candidate + return text + + +#: Words that mark a paragraph as the ADVICE rather than the model's +#: working. The advice talks about what the brewer should DO. +_ADVICE_MARKERS = ( + "you should", + "you could", + "you may want", + "you might", + "give it", + "let it", + "take another", + "take the", + "raise", + "lower", + "rack", + "keep", + "suggest", + "recommend", + "in your case", + "here is", + "here's", + "don't", + "do not", +) + + +def _looks_like_advice(paragraph: str) -> bool: + """True when a paragraph reads like advice to the brewer. + + The model's working is full of hedging - "perhaps", "or maybe", "it + could be" - and is about interpreting the numbers. The advice is + about what to do. + """ + lowered = paragraph.lower() + if any(marker in lowered for marker in _ADVICE_MARKERS): + return True + # A paragraph that reads like working is not advice. The signs are + # hedging ("perhaps", "could be"), and the model talking to itself + # ("wait", "hmm", "so", "actually") rather than to the brewer. + working_words = ("perhaps", "maybe", "possibly", "could be", "might be", + "i think", "i am not sure", "actually", "hmm", "wait", + "let me", "so on brew day", "there is a discrepancy", + "that is a discrepancy", "but the", "which means") + if any(word in lowered for word in working_words): + return False + return True def generate_advice(run: dict, reading: dict, history: list[dict], *, diff --git a/tests/test_brew_advice.py b/tests/test_brew_advice.py index 3a12d0f..516890f 100644 --- a/tests/test_brew_advice.py +++ b/tests/test_brew_advice.py @@ -220,7 +220,42 @@ def test_the_token_budget_leaves_room_for_the_reasoning(monkeypatch): brew_advice.generate_advice({"canonical_name": "Ale"}, {"date": "2026-01-01"}, []) - assert captured["body"]["max_tokens"] >= 2000 + # The reasoning comes out of the same budget as the answer, and + # `think: false` does not stop it - the models just move the working + # into the content. The budget has to cover both. + assert captured["body"]["max_tokens"] >= 8000 + + +def test_a_reasoning_preamble_is_cut_off(): + """Some models put their working in the content even with thinking off. + + The advice is the prose at the end; the working comes first, in + several paragraphs. The preamble is removed by finding the advice, + not by taking the text after the first blank line. + """ + reply = ( + "Let me look at this. The brewer has a Christmas beer, OG 1.042, " + "fermenting at 19C.\n\n" + "So on brew day, the reading is 1.038, but the OG is stated as 1.042. " + "That is a discrepancy of 4 points.\n\n" + "Wait, the reading on brew day is 1.038 but the recorded OG is 1.042.\n\n" + "On brew day a gravity of 1.038 against a recorded original of 1.042 " + "is a small discrepancy but not a problem. Take the OG from a " + "well-mixed sample, and start the fermentation at 18-20C." + ) + + stripped = brew_advice._strip_reasoning(reply) + + assert stripped.startswith("On brew day a gravity of 1.038") + assert "Let me look at this" not in stripped + assert "That is a discrepancy of 4 points" not in stripped + + +def test_a_reply_that_starts_with_the_advice_is_untouched(): + """The stripper must not eat a good reply.""" + reply = ("The gravity looks fine for day nine. Give it three more days " + "before you do anything, and take another reading on Friday.") + assert brew_advice._strip_reasoning(reply) == reply def test_the_advice_is_truncated(): From b414240bb534274b6ade266aa736654703601c4d Mon Sep 17 00:00:00 2001 From: hermes Date: Mon, 9 Nov 2026 17:36:13 +1000 Subject: [PATCH 07/17] fix(advice): keep the advice when the reply is over-long 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). --- src/flask/brew_advice.py | 33 ++++++++++++++++++++++++++++++++- tests/test_brew_advice.py | 27 +++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/src/flask/brew_advice.py b/src/flask/brew_advice.py index eeec114..35adea7 100644 --- a/src/flask/brew_advice.py +++ b/src/flask/brew_advice.py @@ -202,7 +202,28 @@ def call_ollama(prompt: str, *, url: str | None = None, model: str | None = None content = content.strip() if not content: raise RuntimeError("the model returned an empty response") - return _strip_reasoning(content)[:MAX_ADVICE_CHARS] + return _trim_advice(_strip_reasoning(content)) + + +def _trim_advice(text: str) -> str: + """Cut the advice down to the readable length, keeping the END. + + The advice is at the end of the reply, so a plain truncation from the + front would cut the advice off and leave only the model's working - + which is worse than no advice at all, because it looks like advice. + So an over-long reply is cut at a sentence boundary near the end. + """ + if len(text) <= MAX_ADVICE_CHARS: + return text + + tail = text[-MAX_ADVICE_CHARS:] + # Start at the first sentence break in the tail, so it does not begin + # mid-word. + for separator in (". ", "? ", "! "): + index = tail.find(separator) + if index != -1 and index < 200: + return tail[index + 2:].strip() + return tail.strip() #: The reasoning models sometimes put their working IN the content, even @@ -233,6 +254,16 @@ def _strip_reasoning(text: str) -> str: starts straight into the advice is left alone. """ lowered = text.lower() + + # A reply that is ALL working - no advice paragraph at all - has to + # be caught too. The marker phrases are a hint, not the only signal: + # a reply can be pure working without ever saying "let me think". + paragraphs = [p.strip() for p in text.split("\n\n") if p.strip()] + if len(paragraphs) > 1 and not any(_looks_like_advice(p) for p in paragraphs): + # No paragraph reads like advice, so the whole thing is working. + # There is nothing to keep - the caller stores what is left. + return text + for marker in _PREAMBLE_MARKERS: index = lowered.find(marker) # index 0 is fine - a reply can START with "Let me look at this". diff --git a/tests/test_brew_advice.py b/tests/test_brew_advice.py index 516890f..99cee4c 100644 --- a/tests/test_brew_advice.py +++ b/tests/test_brew_advice.py @@ -258,6 +258,33 @@ def test_a_reply_that_starts_with_the_advice_is_untouched(): assert brew_advice._strip_reasoning(reply) == reply +def test_an_over_long_reply_keeps_the_advice_at_the_end(): + """The advice is at the END of the reply, so a plain truncation from + the front would cut it off and leave only the model's working. + + That is worse than no advice at all, because it looks like advice. + """ + working = ( + "I need to work through what this reading means. The original gravity " + "is 1.055 and the current reading is 1.030. " + ("padding " * 260) + + "Leave the fermenter alone, take another reading in three days and " + "only then consider repitching." + ) + + final = brew_advice._trim_advice(working) + + assert len(final) <= brew_advice.MAX_ADVICE_CHARS + assert "Leave the fermenter alone" in final, ( + "the advice was cut off and only the model's working was left" + ) + + +def test_a_short_reply_is_not_trimmed(): + """The trim must not touch a reply that already fits.""" + reply = "The gravity looks fine for day nine. Give it three more days." + assert brew_advice._trim_advice(reply) == reply + + def test_the_advice_is_truncated(): """A runaway reply must not fill the reading document.""" long_text = "x" * 5000 From 3e1e4f42b7888776837ae59a91a49be8941efe39 Mon Sep 17 00:00:00 2001 From: hermes Date: Mon, 9 Nov 2026 17:45:16 +1000 Subject: [PATCH 08/17] fix(advice): stale advice must not overwrite a correction 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). --- src/flask/brew_advice.py | 15 ++++++- src/flask/mongo/repository.py | 20 ++++++++- src/flask/views/brews.py | 13 ++++-- tests/test_brew_advice.py | 81 +++++++++++++++++++++++++++++++++++ 4 files changed, 122 insertions(+), 7 deletions(-) diff --git a/src/flask/brew_advice.py b/src/flask/brew_advice.py index 35adea7..1304614 100644 --- a/src/flask/brew_advice.py +++ b/src/flask/brew_advice.py @@ -198,11 +198,24 @@ def call_ollama(prompt: str, *, url: str | None = None, model: str | None = None if not choices: raise RuntimeError("the model returned no choices") message = choices[0].get("message") or {} + + # Some models put their working in a separate field. It is ignored - + # the advice is the content. content = message.get("content") or "" content = content.strip() if not content: raise RuntimeError("the model returned an empty response") - return _trim_advice(_strip_reasoning(content)) + + # A reply cut off at the token ceiling is INCOMPLETE. It may be + # advice that stops mid-sentence, so it is marked as truncated rather + # than stored as if it were the whole answer. + finish_reason = choices[0].get("finish_reason") or body.get("done_reason") + truncated = finish_reason == "length" + + advice = _trim_advice(_strip_reasoning(content)) + if truncated: + advice = _trim_advice(advice + " (the reply was cut off)") + return advice def _trim_advice(text: str) -> str: diff --git a/src/flask/mongo/repository.py b/src/flask/mongo/repository.py index e0d31e8..c36c8e1 100644 --- a/src/flask/mongo/repository.py +++ b/src/flask/mongo/repository.py @@ -223,20 +223,36 @@ class BeerRepository: return str(existing.get("_id")) if existing else "" def set_reading_advice(self, reading_id: str, advice: str, - ok: bool = True) -> int: + ok: bool = True, + expected_sg: int | None = None) -> int: """Attach the LLM's advice to a reading. The advice lives in its own field, so the brewer's own comment is never overwritten. ``ok`` says whether the advice was actually generated or is a note explaining that it could not be. + + ``expected_sg`` is the gravity the advice was generated FOR. The + write only happens if the reading still holds that gravity. + + This matters because the advice is generated in the background: if + the brewer corrects a reading while the advice for the old value + is still being generated, the slow thread would land afterwards + and overwrite the fresh advice with advice about a reading that + no longer exists. Without the guard the run ends up with the + final gravity from the correction and the advice from before it. """ doc_id = _as_doc_id(reading_id) if not doc_id: return 0 + + query: dict = {"_id": {"$in": doc_id}} + if expected_sg is not None: + query["sg"] = str(expected_sg) + # `$in` matches either form. A plain list would be an exact # ARRAY match in Mongo, which is not what is wanted here. result = self.readings.update_one( - {"_id": {"$in": doc_id}}, + query, {"$set": { "advice": str(advice), "advice_ok": bool(ok), diff --git a/src/flask/views/brews.py b/src/flask/views/brews.py index 6b00c07..9c92aa9 100644 --- a/src/flask/views/brews.py +++ b/src/flask/views/brews.py @@ -112,20 +112,25 @@ def _start_advice(run_id: str, reading_id: str, corrected: bool = False) -> None log.exception("could not gather the reading for the advice") return + # The gravity the advice is about. The write is conditional on the + # reading still holding it, so a slow advice thread cannot overwrite + # a correction with advice about the old value. + advice_sg = latest.get("sg") + def write(read_id: str, ok: bool, text: str) -> None: # The client is SHARED, not built per reading. A MongoClient - # spawns three monitor threads and pymongo does not reap them on - # close(), so building one per advice write accumulated three + # spawns three monitor threads and pymongo does not reap them + # on close(), so building one per advice write accumulated three # threads and a connection pool for every reading recorded. # # The shared client is created on first use and lives for the # life of the process, which is what the request-scoped # repository does too. - from mongo.repository import BeerRepository, get_shared_repository + from mongo.repository import get_shared_repository try: shared = get_shared_repository() - shared.set_reading_advice(read_id, text, ok) + shared.set_reading_advice(read_id, text, ok, expected_sg=advice_sg) except Exception: # noqa: BLE001 - never let the thread die noisily log.exception("could not store the generated advice") diff --git a/tests/test_brew_advice.py b/tests/test_brew_advice.py index 99cee4c..4222718 100644 --- a/tests/test_brew_advice.py +++ b/tests/test_brew_advice.py @@ -546,6 +546,87 @@ def test_the_shared_repository_is_built_only_once(): assert len(set(instances)) == 1, "the threads got different repositories" +def test_stale_advice_cannot_overwrite_a_correction(repo): + """A slow advice thread must not overwrite a correction. + + The advice is generated in the background. If the brewer corrects a + reading while the advice for the old gravity is still being + generated, the slow thread lands afterwards - and without a guard the + reading ends up with the final gravity from the correction and the + advice from before it. + + The write is conditional on the reading still holding the gravity the + advice was generated for. + """ + seed_readings(repo, [("1", "Ale", "1050", "2026-01-01", False, "a comment")]) + doc = repo.readings.find_one({"beer_run_id": "1"}) + reading_id = str(doc["_id"]) + + # The advice for the ORIGINAL 1050 arrives late... + modified = repo.set_reading_advice(reading_id, "ADVICE FOR THE 1050", + True, expected_sg=1050) + assert modified == 1 + + # ...but the brewer has already corrected it to 1020, and the fresh + # advice has landed. + repo.upsert_reading(beer_run_id="1", beer_type="Ale", sg=1020, + reading_date="2026-01-01", final_reading=False, + comment="a comment") + repo.set_reading_advice(reading_id, "ADVICE FOR THE 1020", True, + expected_sg=1020) + + # Now the late thread for the old 1050 finally lands. + repo.set_reading_advice(reading_id, "STALE ADVICE FOR THE 1050", True, + expected_sg=1050) + + after = repo.readings.find_one({"beer_run_id": "1"}) + assert after["sg"] == "1020" + assert after["advice"] == "ADVICE FOR THE 1020", ( + f"the stale advice overwrote the correction: {after['advice']!r}" + ) + + +def test_a_reply_cut_off_at_the_token_ceiling_is_marked(repo): + """A reply that stopped at the token limit is incomplete. + + It may be advice that stops mid-sentence, so it must not be stored + as if it were the whole answer. + """ + captured = {} + + def fake_urlopen(request, timeout=None): + captured["body"] = json.loads(request.data.decode()) + + class Response: + def read(self): + return json.dumps({ + "choices": [{ + "finish_reason": "length", + "message": {"content": "The gravity looks fine so far, but"}, + }], + }).encode() + + def __enter__(self): + return self + + def __exit__(self, *args): + return False + + return Response() + + import brew_advice as ba + original = ba.urllib.request.urlopen + ba.urllib.request.urlopen = fake_urlopen + try: + ok, text = ba.generate_advice({"canonical_name": "Ale"}, + {"date": "2026-01-01"}, []) + finally: + ba.urllib.request.urlopen = original + + assert ok is True + assert "cut off" in text + + def test_the_reading_id_cannot_be_used_to_inject_a_query(repo): """The reading id is coerced before it reaches Mongo. From 8da3601941e50ba1a8c748a5f47c25f6d06f9e45 Mon Sep 17 00:00:00 2001 From: hermes Date: Mon, 9 Nov 2026 17:47:14 +1000 Subject: [PATCH 09/17] fix(advice): the remaining review findings 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). --- src/flask/static/css/app.css | 6 ++++++ src/flask/views/brews.py | 16 ++++++++++++++-- tests/test_brew_advice.py | 14 ++++++++++++++ 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/src/flask/static/css/app.css b/src/flask/static/css/app.css index f99a72f..fb54866 100644 --- a/src/flask/static/css/app.css +++ b/src/flask/static/css/app.css @@ -716,6 +716,12 @@ table.data .comment { color: var(--text-muted); font-size: .84rem; } color: var(--amber-text); } +/* The advice on the active-beers card and the run detail page - the + full text, not the clamped table cell. */ +.advice--reading { + margin-top: .5rem; +} + /* Advice that could not be generated - it says why. */ .advice--failed { border-left-color: var(--danger); diff --git a/src/flask/views/brews.py b/src/flask/views/brews.py index 9c92aa9..5c6045b 100644 --- a/src/flask/views/brews.py +++ b/src/flask/views/brews.py @@ -60,7 +60,8 @@ def _start_advice(run_id: str, reading_id: str, corrected: bool = False) -> None comment is never touched. The background thread cannot use the request-scoped repository - that - client is closed when the request ends - so it builds its own. + client is closed when the request ends - so it uses the shared + process-wide one. """ if not reading_id: return @@ -130,7 +131,18 @@ def _start_advice(run_id: str, reading_id: str, corrected: bool = False) -> None try: shared = get_shared_repository() - shared.set_reading_advice(read_id, text, ok, expected_sg=advice_sg) + written = shared.set_reading_advice(read_id, text, ok, + expected_sg=advice_sg) + if not written: + # The reading was corrected while the advice was being + # generated, so the advice no longer matches it. That is + # the guard working - the fresh advice for the corrected + # reading will land in its own turn. + log.info( + "the advice for reading %s was not stored: the reading " + "no longer holds the gravity it was written for", + read_id, + ) except Exception: # noqa: BLE001 - never let the thread die noisily log.exception("could not store the generated advice") diff --git a/tests/test_brew_advice.py b/tests/test_brew_advice.py index 4222718..f5db183 100644 --- a/tests/test_brew_advice.py +++ b/tests/test_brew_advice.py @@ -498,6 +498,20 @@ def test_the_advice_is_about_the_reading_that_was_saved(): "the advice was generated about the wrong reading" ) + # And the PROMPT must ask about that reading - not just pass it + # through. Asserting only which dict was passed would not have + # caught the prompt still saying "give advice for the most recent + # reading", which is the bug that made the model advise about the + # wrong number. + prompt = brew_advice.build_prompt( + {"canonical_name": "Ale"}, + captured["reading"], + captured["history"], + ) + assert "THIS IS THE READING TO ADVISE ON" in prompt + assert "2026-01-01" in prompt + assert "most recent reading" not in prompt + def test_the_shared_repository_is_built_only_once(): """Concurrent advice threads must not each build their own client. From a78aabe3eaa9f27da664102704ae5145e7e517a6 Mon Sep 17 00:00:00 2001 From: hermes Date: Mon, 9 Nov 2026 18:14:24 +1000 Subject: [PATCH 10/17] fix(advice): an editing token, so a comment-only correction is safe too 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). --- src/flask/brewing.py | 5 +++++ src/flask/mongo/repository.py | 38 +++++++++++++++++++++++++++------ src/flask/views/brews.py | 15 ++++++++----- tests/test_brew_advice.py | 40 +++++++++++++++++++++++++++++++++++ 4 files changed, 87 insertions(+), 11 deletions(-) diff --git a/src/flask/brewing.py b/src/flask/brewing.py index 8332f01..470e130 100644 --- a/src/flask/brewing.py +++ b/src/flask/brewing.py @@ -156,6 +156,10 @@ class Reading: # Mongo, so the brewer's own comment is never overwritten. advice: str = "" advice_ok: bool = True + # Marks this generation of the reading. Every edit writes a fresh + # one, so an advice thread working on an earlier version of the + # reading can be told apart from the current one. + advice_token: str = "" @property def sg_decimal(self) -> float | None: @@ -177,6 +181,7 @@ class Reading: _id=str(doc.get("_id") or ""), advice=(doc.get("advice") or "").strip(), advice_ok=bool(doc.get("advice_ok", True)), + advice_token=str(doc.get("advice_token") or ""), ) diff --git a/src/flask/mongo/repository.py b/src/flask/mongo/repository.py index c36c8e1..931eb1c 100644 --- a/src/flask/mongo/repository.py +++ b/src/flask/mongo/repository.py @@ -209,13 +209,20 @@ class BeerRepository: "final_reading": "True" if final_reading else "False", "comment": str(comment), }, - # The advice was written for the gravity that was there when + # The advice was written for the reading that was there when # it was generated. If the reading has been edited, that # advice no longer describes it, so it is cleared and a fresh # one is generated. (Advice is only ever cleared here - never # the brewer's comment.) "$unset": {"advice": "", "advice_ok": "", "advice_at": ""}, } + # A fresh token marks this generation of the reading. The advice + # thread carries the token it was given, so a thread that is + # still working on an EARLIER version of the reading finds a + # token that no longer matches and its write is refused. The + # gravity alone is not enough - a comment-only correction leaves + # the gravity unchanged. + update["$set"]["advice_token"] = _new_token() result = self.readings.update_one(query, update, upsert=True) if result.upserted_id is not None: return str(result.upserted_id) @@ -224,22 +231,32 @@ class BeerRepository: def set_reading_advice(self, reading_id: str, advice: str, ok: bool = True, - expected_sg: int | None = None) -> int: + expected_sg: int | None = None, + expected_token: str | None = None) -> int: """Attach the LLM's advice to a reading. The advice lives in its own field, so the brewer's own comment is never overwritten. ``ok`` says whether the advice was actually generated or is a note explaining that it could not be. - ``expected_sg`` is the gravity the advice was generated FOR. The - write only happens if the reading still holds that gravity. + The write is CONDITIONAL, so a slow advice thread cannot + overwrite a correction: + + - ``expected_token`` is the editing token of the reading at the + moment the advice was generated. Every edit writes a fresh + token, so a late thread's token no longer matches. + - ``expected_sg`` is the gravity the advice was generated for, + as a second check. This matters because the advice is generated in the background: if the brewer corrects a reading while the advice for the old value is still being generated, the slow thread would land afterwards and overwrite the fresh advice with advice about a reading that - no longer exists. Without the guard the run ends up with the - final gravity from the correction and the advice from before it. + no longer exists. + + The token is needed as well as the gravity, because a + comment-only correction leaves the gravity unchanged - the token + is what tells the two generations apart. """ doc_id = _as_doc_id(reading_id) if not doc_id: @@ -248,6 +265,8 @@ class BeerRepository: query: dict = {"_id": {"$in": doc_id}} if expected_sg is not None: query["sg"] = str(expected_sg) + if expected_token is not None: + query["advice_token"] = str(expected_token) # `$in` matches either form. A plain list would be an exact # ARRAY match in Mongo, which is not what is wanted here. @@ -485,6 +504,13 @@ def hash_password(password: str) -> str: return generate_password_hash(password) +def _new_token() -> str: + """A fresh token, to mark one generation of a reading from the next.""" + import secrets + + return secrets.token_hex(8) + + def _now_iso() -> str: from datetime import datetime, timezone diff --git a/src/flask/views/brews.py b/src/flask/views/brews.py index 5c6045b..c384365 100644 --- a/src/flask/views/brews.py +++ b/src/flask/views/brews.py @@ -91,6 +91,7 @@ def _start_advice(run_id: str, reading_id: str, corrected: bool = False) -> None points = reading.sg_points readings.append({ "_id": str(reading._id), + "advice_token": reading.advice_token, "date": reading.date.isoformat() if reading.date else "", "sg": points, "sg_decimal": reading.sg_decimal, @@ -113,10 +114,11 @@ def _start_advice(run_id: str, reading_id: str, corrected: bool = False) -> None log.exception("could not gather the reading for the advice") return - # The gravity the advice is about. The write is conditional on the - # reading still holding it, so a slow advice thread cannot overwrite - # a correction with advice about the old value. + # The gravity and the editing token of the reading being advised + # about. The write is conditional on both, so a slow advice thread + # cannot overwrite a correction with advice about the old value. advice_sg = latest.get("sg") + advice_token = latest.get("advice_token") def write(read_id: str, ok: bool, text: str) -> None: # The client is SHARED, not built per reading. A MongoClient @@ -131,8 +133,11 @@ def _start_advice(run_id: str, reading_id: str, corrected: bool = False) -> None try: shared = get_shared_repository() - written = shared.set_reading_advice(read_id, text, ok, - expected_sg=advice_sg) + written = shared.set_reading_advice( + read_id, text, ok, + expected_sg=advice_sg, + expected_token=advice_token, + ) if not written: # The reading was corrected while the advice was being # generated, so the advice no longer matches it. That is diff --git a/tests/test_brew_advice.py b/tests/test_brew_advice.py index f5db183..960f47c 100644 --- a/tests/test_brew_advice.py +++ b/tests/test_brew_advice.py @@ -600,6 +600,46 @@ def test_stale_advice_cannot_overwrite_a_correction(repo): ) +def test_a_comment_only_correction_cannot_be_overwritten(repo): + """A comment-only correction leaves the gravity unchanged. + + So the gravity alone cannot tell the two generations apart - a late + thread for the old advice would still match and overwrite the fresh + one. The editing token is what tells them apart. + """ + # Written through upsert_reading, so it has an editing token. + reading_id = repo.upsert_reading(beer_run_id="1", beer_type="Ale", sg=1050, + reading_date="2026-01-01", + final_reading=False, comment="first comment") + doc = repo.readings.find_one({"beer_run_id": "1"}) + token = doc.get("advice_token") + assert token, "the reading has no editing token" + + # The advice thread starts on this generation. + # The brewer corrects the COMMENT only - the gravity is unchanged. + repo.upsert_reading(beer_run_id="1", beer_type="Ale", sg=1050, + reading_date="2026-01-01", final_reading=False, + comment="corrected comment") + assert repo.readings.find_one({"beer_run_id": "1"}).get("advice_token") != token, ( + "the correction did not write a fresh token" + ) + + # The fresh advice lands. + fresh_token = repo.readings.find_one({"beer_run_id": "1"}).get("advice_token") + assert repo.set_reading_advice(reading_id, "FRESH ADVICE", True, + expected_sg=1050, + expected_token=fresh_token) == 1 + + # The late thread for the OLD generation finally lands. + stale = repo.set_reading_advice(reading_id, "STALE ADVICE", True, + expected_sg=1050, expected_token=token) + assert stale == 0, "the stale advice overwrote the correction" + + after = repo.readings.find_one({"beer_run_id": "1"}) + assert after["advice"] == "FRESH ADVICE" + assert after["comment"] == "corrected comment" + + def test_a_reply_cut_off_at_the_token_ceiling_is_marked(repo): """A reply that stopped at the token limit is incomplete. From 27a28b5e58f3a53b72b020a70c6186b3e6b15d9a Mon Sep 17 00:00:00 2001 From: hermes Date: Mon, 9 Nov 2026 18:22:51 +1000 Subject: [PATCH 11/17] fix(advice): the stripper must DROP the working, and the token is the guard 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). --- src/flask/brew_advice.py | 38 ++++++++++++------- src/flask/mongo/repository.py | 8 +++- tests/test_brew_advice.py | 70 +++++++++++++++++++++++++++++++++++ 3 files changed, 101 insertions(+), 15 deletions(-) diff --git a/src/flask/brew_advice.py b/src/flask/brew_advice.py index 1304614..ecfc5bf 100644 --- a/src/flask/brew_advice.py +++ b/src/flask/brew_advice.py @@ -271,11 +271,13 @@ def _strip_reasoning(text: str) -> str: # A reply that is ALL working - no advice paragraph at all - has to # be caught too. The marker phrases are a hint, not the only signal: # a reply can be pure working without ever saying "let me think". + # + # The working is DROPPED, not returned. Returning it unchanged would + # be a no-op, and the brewer would be shown the model thinking about + # their beer instead of any advice at all. paragraphs = [p.strip() for p in text.split("\n\n") if p.strip()] if len(paragraphs) > 1 and not any(_looks_like_advice(p) for p in paragraphs): - # No paragraph reads like advice, so the whole thing is working. - # There is nothing to keep - the caller stores what is left. - return text + return "" for marker in _PREAMBLE_MARKERS: index = lowered.find(marker) @@ -309,21 +311,29 @@ def _strip_reasoning(text: str) -> str: #: Words that mark a paragraph as the ADVICE rather than the model's #: working. The advice talks about what the brewer should DO. +#: Phrases that mark a paragraph as the ADVICE rather than the model's +#: working. They are matched as PHRASES, not bare words - "keep" alone +#: matches "I need to keep in mind that the hydrometer was read at a +#: different temperature", which is working, not advice. _ADVICE_MARKERS = ( "you should", "you could", "you may want", "you might", + "you will want", "give it", - "let it", - "take another", - "take the", - "raise", - "lower", - "rack", - "keep", - "suggest", - "recommend", + "let it sit", + "let it ferment", + "take another reading", + "take a reading", + "raise it to", + "raise the", + "lower the", + "rack it", + "you could repitch", + "i would suggest", + "i suggest", + "i recommend", "in your case", "here is", "here's", @@ -348,7 +358,9 @@ def _looks_like_advice(paragraph: str) -> bool: working_words = ("perhaps", "maybe", "possibly", "could be", "might be", "i think", "i am not sure", "actually", "hmm", "wait", "let me", "so on brew day", "there is a discrepancy", - "that is a discrepancy", "but the", "which means") + "that is a discrepancy", "which means", + "i need to keep in mind", "the question is", + "this could mean", "it could be", "or maybe") if any(word in lowered for word in working_words): return False return True diff --git a/src/flask/mongo/repository.py b/src/flask/mongo/repository.py index 931eb1c..a0f0505 100644 --- a/src/flask/mongo/repository.py +++ b/src/flask/mongo/repository.py @@ -263,10 +263,14 @@ class BeerRepository: return 0 query: dict = {"_id": {"$in": doc_id}} - if expected_sg is not None: - query["sg"] = str(expected_sg) if expected_token is not None: + # 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. query["advice_token"] = str(expected_token) + elif expected_sg is not None: + # No token - a direct call with a gravity to check against. + query["sg"] = str(expected_sg) # `$in` matches either form. A plain list would be an exact # ARRAY match in Mongo, which is not what is wanted here. diff --git a/tests/test_brew_advice.py b/tests/test_brew_advice.py index 960f47c..314c9a4 100644 --- a/tests/test_brew_advice.py +++ b/tests/test_brew_advice.py @@ -640,6 +640,76 @@ def test_a_comment_only_correction_cannot_be_overwritten(repo): assert after["comment"] == "corrected comment" +def test_a_reply_that_is_all_working_stores_no_working(): + """A reply with no advice paragraph in it must not be stored. + + The old code returned the text unchanged, which was a no-op - and in + the case where a preamble marker WAS present it returned the whole + reply, working included. So the brewer was shown the model thinking + about their beer instead of any advice at all. + """ + working_only = ( + "Let me think about what this reading means.\n\n" + "Perhaps the drop is smaller than expected, and it could be that the " + "yeast is stressed.\n\n" + "Wait, I need to keep in mind that the hydrometer was read at a " + "different temperature.\n\n" + "I need to keep in mind that the original gravity here was measured " + "before the hops went in.\n\n" + "So the question is what the expected attenuation should be, and I " + "am not sure without more data." + ) + + stripped = brew_advice._strip_reasoning(working_only) + + assert stripped == "", ( + f"the model's working was kept as advice: {stripped[:120]!r}" + ) + + +def test_a_paragraph_about_keeping_something_in_mind_is_not_advice(): + """"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 to the brewer. + """ + working = ("I need to keep in mind that the hydrometer was read at a " + "different temperature, which explains the discrepancy.") + assert brew_advice._looks_like_advice(working) is False + + +def test_the_token_is_the_primary_guard(repo): + """The token changes on EVERY edit, so it catches a stale generation + whatever else changed. + + The gravity alone is not enough - a comment-only correction leaves it + unchanged - and it can be unparseable for some readings, so + `expected_sg` can be None. The token is what the guard relies on. + """ + reading_id = repo.upsert_reading(beer_run_id="1", beer_type="Ale", sg=1050, + reading_date="2026-01-01", + final_reading=False, comment="first") + token = repo.readings.find_one({"beer_run_id": "1"})["advice_token"] + + # A direct call with no generation context is not conditional - it is + # a deliberate write, so it goes through. + assert repo.set_reading_advice(reading_id, "DIRECT ADVICE", True) == 1 + + # The brewer edits the reading, so the generation changes. + repo.upsert_reading(beer_run_id="1", beer_type="Ale", sg=1050, + reading_date="2026-01-01", final_reading=False, + comment="edited") + + # A write for the generation that no longer exists is refused, even + # with no gravity to check against. + assert repo.set_reading_advice(reading_id, "STALE ADVICE", True, + expected_sg=None, + expected_token=token) == 0 + after = repo.readings.find_one({"beer_run_id": "1"}) + # The edit cleared the advice, and the stale write did not put it back. + assert after.get("advice") is None + + def test_a_reply_cut_off_at_the_token_ceiling_is_marked(repo): """A reply that stopped at the token limit is incomplete. From 7d9d718b4161ce48dcce0d9fb7f0c6d7251fae97 Mon Sep 17 00:00:00 2001 From: hermes Date: Mon, 9 Nov 2026 18:24:57 +1000 Subject: [PATCH 12/17] fix(advice): an empty token must not be used as a guard 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). --- src/flask/mongo/repository.py | 7 ++++++- tests/test_brew_advice.py | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/src/flask/mongo/repository.py b/src/flask/mongo/repository.py index a0f0505..9d5ddec 100644 --- a/src/flask/mongo/repository.py +++ b/src/flask/mongo/repository.py @@ -263,10 +263,15 @@ class BeerRepository: return 0 query: dict = {"_id": {"$in": doc_id}} - if expected_token is not None: + if expected_token: # 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. + # + # An EMPTY token is not a usable guard: it means the reading + # predates the token, so there is no token to match. + # Querying on it would match nothing, and the advice would + # never be written for the existing readings. query["advice_token"] = str(expected_token) elif expected_sg is not None: # No token - a direct call with a gravity to check against. diff --git a/tests/test_brew_advice.py b/tests/test_brew_advice.py index 314c9a4..e27e95c 100644 --- a/tests/test_brew_advice.py +++ b/tests/test_brew_advice.py @@ -710,6 +710,41 @@ def test_the_token_is_the_primary_guard(repo): assert after.get("advice") is None +def test_the_advice_is_written_for_a_reading_that_predates_the_token(repo): + """None of the existing readings have a token. + + They all predate it, so a reading read from the database has an EMPTY + token. An empty token is not a usable guard - there is no token to + match - so the write must not be conditioned on it, or the advice + would never be written for any of them. + """ + # Inserted directly, the way the readings already in the database + # are - no token. + repo.readings.insert_one({ + "_id": "l78ZidBJ3QaYn98tvB88x4uM", + "beer_run_id": "1", + "beer_type": "Ale", + "sg": "1042", + "date": "2024-09-01", + "final_reading": "False", + "comment": "a comment", + }) + + run = repo.run("1") + assert run.readings[0].advice_token == "", "the reading should have no token" + + # The advice flow passes the token it read, which is empty. + written = repo.set_reading_advice("l78ZidBJ3QaYn98tvB88x4uM", + "THE ADVICE", True, + expected_sg=None, + expected_token=run.readings[0].advice_token) + assert written == 1, "the advice was never written for a pre-token reading" + + after = repo.readings.find_one({"beer_run_id": "1"}) + assert after["advice"] == "THE ADVICE" + assert after["comment"] == "a comment" + + def test_a_reply_cut_off_at_the_token_ceiling_is_marked(repo): """A reply that stopped at the token limit is incomplete. From d18d037bb9a9981ca9f9c5079ed285a83bcf6626 Mon Sep 17 00:00:00 2001 From: hermes Date: Mon, 9 Nov 2026 19:08:28 +1000 Subject: [PATCH 13/17] fix(advice): the pre-token guard, and a stripper that cannot empty real advice 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). --- src/flask/brew_advice.py | 53 +++++++++++++++++++- src/flask/mongo/repository.py | 18 ++++--- src/flask/views/brews.py | 3 +- tests/test_brew_advice.py | 93 ++++++++++++++++++++++++++++++++--- 4 files changed, 153 insertions(+), 14 deletions(-) diff --git a/src/flask/brew_advice.py b/src/flask/brew_advice.py index ecfc5bf..d0b0e31 100644 --- a/src/flask/brew_advice.py +++ b/src/flask/brew_advice.py @@ -276,7 +276,10 @@ def _strip_reasoning(text: str) -> str: # be a no-op, and the brewer would be shown the model thinking about # their beer instead of any advice at all. paragraphs = [p.strip() for p in text.split("\n\n") if p.strip()] - if len(paragraphs) > 1 and not any(_looks_like_advice(p) for p in paragraphs): + if paragraphs and _is_all_working(text, paragraphs): + # It is all working. It is dropped, so the brewer sees a clear + # "could not be generated" rather than the model thinking about + # their beer. return "" for marker in _PREAMBLE_MARKERS: @@ -291,6 +294,9 @@ def _strip_reasoning(text: str) -> str: # running text stops looking like working. paragraphs = [p.strip() for p in tail.split("\n\n") if p.strip()] if len(paragraphs) < 2: + # A single paragraph: the whole tail is one block of working + # with the advice inside it, so it is kept whole rather than + # thrown away. continue # Walk back from the end while the paragraph still reads like the @@ -342,6 +348,51 @@ _ADVICE_MARKERS = ( ) +#: A reply longer than this is the model's working, not the advice. +#: The advice is meant to be two to five sentences. +_WORKING_LENGTH = 600 + + +def _is_all_working(text: str, paragraphs: list[str]) -> bool: + """True when the reply is the model's working rather than advice. + + Two signals, because either alone is unreliable: + + - **Length.** The working runs to thousands of characters; the + advice is two to five sentences. + - **Shape.** The working interprets the numbers ("perhaps the yeast + is stressed, it could be that..."); the advice tells the brewer + what to do. + + A reply that is SHORT is kept even if it has no marker word - real + advice is often phrased without one ("Actually the drop is fine. + Wait a couple of days and check again."). + """ + if len(text) <= _WORKING_LENGTH: + # Too short to be the model's working. It is advice, whatever + # it is worded like. + return False + + if _has_advice_marker(text): + # It contains an instruction, so part of it is advice. + return False + + # Long, and nothing in it reads like an instruction. + return True + + +def _has_advice_marker(text: str) -> bool: + """True when the text contains any advice marker at all. + + Used on the WHOLE reply rather than paragraph by paragraph, because + advice can be phrased without any of the markers - "Actually the + drop is fine.\n\nWait a couple of days and check again." is advice, + and judging it paragraph by paragraph emptied it. + """ + lowered = text.lower() + return any(marker in lowered for marker in _ADVICE_MARKERS) + + def _looks_like_advice(paragraph: str) -> bool: """True when a paragraph reads like advice to the brewer. diff --git a/src/flask/mongo/repository.py b/src/flask/mongo/repository.py index 9d5ddec..e223f35 100644 --- a/src/flask/mongo/repository.py +++ b/src/flask/mongo/repository.py @@ -267,14 +267,20 @@ class BeerRepository: # 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. - # - # An EMPTY token is not a usable guard: it means the reading - # predates the token, so there is no token to match. - # Querying on it would match nothing, and the advice would - # never be written for the existing readings. query["advice_token"] = str(expected_token) + elif expected_token == "": + # The reading has NEVER been edited, so it has no token at + # all. Match on the ABSENCE of one, which still tells the + # generations apart: an edit writes a token, so a stale + # thread's "" stops matching the moment the reading is + # corrected. + # + # Falling back to the gravity here would re-open the hole + # this guard exists to close - a comment-only correction + # leaves the gravity unchanged. + query["advice_token"] = {"$exists": False} elif expected_sg is not None: - # No token - a direct call with a gravity to check against. + # A direct call with a gravity to check against. query["sg"] = str(expected_sg) # `$in` matches either form. A plain list would be an exact diff --git a/src/flask/views/brews.py b/src/flask/views/brews.py index c384365..c08a20c 100644 --- a/src/flask/views/brews.py +++ b/src/flask/views/brews.py @@ -145,7 +145,8 @@ def _start_advice(run_id: str, reading_id: str, corrected: bool = False) -> None # reading will land in its own turn. log.info( "the advice for reading %s was not stored: the reading " - "no longer holds the gravity it was written for", + "has been edited since the advice was generated, so the " + "advice no longer matches it", read_id, ) except Exception: # noqa: BLE001 - never let the thread die noisily diff --git a/tests/test_brew_advice.py b/tests/test_brew_advice.py index e27e95c..0a74a15 100644 --- a/tests/test_brew_advice.py +++ b/tests/test_brew_advice.py @@ -649,15 +649,23 @@ def test_a_reply_that_is_all_working_stores_no_working(): about their beer instead of any advice at all. """ working_only = ( - "Let me think about what this reading means.\n\n" + "Let me think about what this reading means. The original gravity is " + "1.055 and the current reading is 1.030, which is a drop of 25 points " + "over nine days at 19 degrees, and I need to work out what that means " + "for the expected attenuation.\n\n" "Perhaps the drop is smaller than expected, and it could be that the " - "yeast is stressed.\n\n" + "yeast is stressed by the temperature rather than actually done, " + "because most ale strains slow down considerably below 18 degrees.\n\n" "Wait, I need to keep in mind that the hydrometer was read at a " - "different temperature.\n\n" + "different temperature than the sample, which would make the reading " + "read lower than it really is, and I have no way of knowing from the " + "data alone what the correction factor was.\n\n" "I need to keep in mind that the original gravity here was measured " - "before the hops went in.\n\n" - "So the question is what the expected attenuation should be, and I " - "am not sure without more data." + "before the hops went in, and the reading on brew day was 1.055, so " + "the drop is twenty-five points against a potential of fifty-five.\n\n" + "So the question is what the expected attenuation should be for this " + "style at this temperature, and I am not sure without more data or a " + "temperature-corrected reading to compare against." ) stripped = brew_advice._strip_reasoning(working_only) @@ -667,6 +675,39 @@ def test_a_reply_that_is_all_working_stores_no_working(): ) +def test_a_short_reply_is_advice_even_without_a_marker_word(): + """Real advice is often phrased without any marker phrase. + + "Actually the drop is fine. Wait a couple of days and check again." + is advice - and judging it paragraph by paragraph emptied it, so the + brewer saw nothing at all. + """ + reply = ("Actually the drop is fine.\n\n" + "Wait a couple of days and check again.") + + assert brew_advice._strip_reasoning(reply) == reply + + +def test_a_long_reply_with_no_instruction_is_dropped(): + """Long, and nothing in it reads like an instruction - all working.""" + working = ("Let me think about this.\n\n" + + "Perhaps the yeast is stressed, and it could be that the " + "temperature is too low. " * 20) + + assert brew_advice._strip_reasoning(working) == "" + + +def test_a_long_reply_with_an_instruction_keeps_it(): + """The working is trimmed from it, but the advice stays.""" + mixed = ("Let me think about this.\n\n" + + "Perhaps the yeast is stressed. " * 15 + + "\n\nWarm the fermenter to 20C and give the yeast a gentle " + "swirl to rouse it.") + + out = brew_advice._strip_reasoning(mixed) + assert "Warm the fermenter" in out + + def test_a_paragraph_about_keeping_something_in_mind_is_not_advice(): """"Keep" alone is not an advice marker. @@ -745,6 +786,46 @@ def test_the_advice_is_written_for_a_reading_that_predates_the_token(repo): assert after["comment"] == "a comment" +def test_a_pre_token_reading_is_still_guarded_against_a_stale_write(repo): + """A reading that has never been edited has no token at all. + + The guard matches on the ABSENCE of a token, so an edit still tells + the generations apart. Falling back to the gravity here would + re-open the hole, because a comment-only correction leaves the + gravity unchanged. + """ + repo.readings.insert_one({ + "_id": "preToken0000000000000010", + "beer_run_id": "1", + "beer_type": "Ale", + "sg": "1050", + "date": "2026-11-01", + "final_reading": "False", + "comment": "first comment", + }) + + # The advice thread starts on this generation - no token. + assert repo.set_reading_advice("preToken0000000000000010", "FRESH ADVICE", + True, expected_sg=None, + expected_token="") == 1 + + # The brewer corrects the comment. The edit writes a token. + repo.upsert_reading(beer_run_id="1", beer_type="Ale", sg=1050, + reading_date="2026-11-01", final_reading=False, + comment="corrected comment") + + # The late thread for the OLD generation lands. It carries no token, + # and the reading now HAS one - so it must be refused. + stale = repo.set_reading_advice("preToken0000000000000010", "STALE ADVICE", + True, expected_sg=None, + expected_token="") + assert stale == 0, "the stale advice overwrote the correction" + + after = repo.readings.find_one({"beer_run_id": "1"}) + assert after.get("advice") is None + assert after["comment"] == "corrected comment" + + def test_a_reply_cut_off_at_the_token_ceiling_is_marked(repo): """A reply that stopped at the token limit is incomplete. From 4bedd23c2487b3f0fc548ff1dd2b1ffb535ac408 Mon Sep 17 00:00:00 2001 From: hermes Date: Mon, 9 Nov 2026 19:10:16 +1000 Subject: [PATCH 14/17] fix(advice): the no-token guard must cover all three shapes of "no token" 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). --- src/flask/mongo/repository.py | 5 ++++- tests/test_brew_advice.py | 27 +++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/src/flask/mongo/repository.py b/src/flask/mongo/repository.py index e223f35..f781ada 100644 --- a/src/flask/mongo/repository.py +++ b/src/flask/mongo/repository.py @@ -278,7 +278,10 @@ class BeerRepository: # Falling back to the gravity here would re-open the hole # this guard exists to close - a comment-only correction # leaves the gravity unchanged. - query["advice_token"] = {"$exists": False} + # `$in` with null also matches a MISSING field, so this + # covers a reading with no token AND one whose token field + # is present but empty. + query["advice_token"] = {"$in": [None, ""]} elif expected_sg is not None: # A direct call with a gravity to check against. query["sg"] = str(expected_sg) diff --git a/tests/test_brew_advice.py b/tests/test_brew_advice.py index 0a74a15..3879299 100644 --- a/tests/test_brew_advice.py +++ b/tests/test_brew_advice.py @@ -826,6 +826,33 @@ def test_a_pre_token_reading_is_still_guarded_against_a_stale_write(repo): assert after["comment"] == "corrected comment" +def test_the_advice_is_written_for_a_reading_with_no_token_field(repo): + """All three shapes of "no token" must work. + + A reading read from the database has no token at all; one written by + an older version could have the field present but empty, or null. + The guard matches on the absence of a token, so it has to cover all + three - otherwise the advice is silently never written. + """ + for index, extra in enumerate(({}, {"advice_token": ""}, {"advice_token": None})): + reading_id = f"noToken{index:04d}" + repo.readings.insert_one({ + "_id": reading_id, + "beer_run_id": str(index), + "beer_type": "Ale", + "sg": "1050", + "date": "2026-01-01", + "final_reading": "False", + "comment": "c", + **extra, + }) + + written = repo.set_reading_advice(reading_id, "THE ADVICE", True, + expected_sg=None, + expected_token="") + assert written == 1, f"the advice was not written for {extra!r}" + + def test_a_reply_cut_off_at_the_token_ceiling_is_marked(repo): """A reply that stopped at the token limit is incomplete. From cbc71716e8ffefd02bf650209547bb3909618b46 Mon Sep 17 00:00:00 2001 From: hermes Date: Mon, 9 Nov 2026 19:27:08 +1000 Subject: [PATCH 15/17] fix(advice): judge the working by its shape, not by its length 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). --- src/flask/brew_advice.py | 47 ++++++++++++++++++++++++++------------- tests/test_brew_advice.py | 29 ++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 15 deletions(-) diff --git a/src/flask/brew_advice.py b/src/flask/brew_advice.py index d0b0e31..d4fcc5b 100644 --- a/src/flask/brew_advice.py +++ b/src/flask/brew_advice.py @@ -356,31 +356,48 @@ _WORKING_LENGTH = 600 def _is_all_working(text: str, paragraphs: list[str]) -> bool: """True when the reply is the model's working rather than advice. - Two signals, because either alone is unreliable: + The reliable signal is SHAPE, not length. A legitimate advice reply + can be a thousand characters, and a short working reply can be a + few hundred - so length alone gets both wrong. - - **Length.** The working runs to thousands of characters; the - advice is two to five sentences. - - **Shape.** The working interprets the numbers ("perhaps the yeast - is stressed, it could be that..."); the advice tells the brewer - what to do. + 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. - A reply that is SHORT is kept even if it has no marker word - real - advice is often phrased without one ("Actually the drop is fine. - Wait a couple of days and check again."). + So a reply is working when it is full of the model's hedging about + the data AND contains no instruction at all. """ - if len(text) <= _WORKING_LENGTH: - # Too short to be the model's working. It is advice, whatever - # it is worded like. - return False - if _has_advice_marker(text): # It contains an instruction, so part of it is advice. return False - # Long, and nothing in it reads like an instruction. + lowered = text.lower() + hedging = sum(lowered.count(word) for word in _HEDGING_WORDS) + + if hedging == 0: + # No instruction and no hedging about the data. This is advice, + # however long it is. + return False + + if len(text) <= _WORKING_LENGTH: + # Some hedging, but it is short - that is a brewer's note that + # happens to use a hedging word, not a page of working. + return False + + # Hedging, no instruction, and long: the working. return True +#: The words the model uses when it is working through the data rather +#: than telling the brewer what to do. +_HEDGING_WORDS = ( + "perhaps", "maybe", "possibly", "could be", "might be", "might", + "i think", "i am not sure", "i'm not sure", "actually", "hmm", + "wait,", "let me", "i need to", "the question is", "unclear", + "i am unsure", "not sure", "unfortunately", +) + + def _has_advice_marker(text: str) -> bool: """True when the text contains any advice marker at all. diff --git a/tests/test_brew_advice.py b/tests/test_brew_advice.py index 3879299..d4bf319 100644 --- a/tests/test_brew_advice.py +++ b/tests/test_brew_advice.py @@ -688,6 +688,35 @@ def test_a_short_reply_is_advice_even_without_a_marker_word(): assert brew_advice._strip_reasoning(reply) == reply +def test_a_long_legitimate_advice_reply_is_kept(): + """Length alone 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 length threshold + gets both wrong. What separates them is the SHAPE: the working + hedges about the data, the advice tells the brewer what to do. + """ + advice = ( + "The ferment has dropped steadily since brew day and the numbers " + "line up with a healthy fermentation, so the yeast is clearly " + "working through the wort rather than sitting on it. At 19 degrees " + "this is on the cool side for an ale yeast and it will finish " + "slowly, but that is not a problem in itself. Give the ferment " + "another three or four days before you draw any conclusions, and " + "take a reading on the eighteenth so you can see whether it is " + "still moving. Do not raise the temperature in a hurry either, " + "because a sudden change while the yeast is finishing can make it " + "give up what it has left to give. If it is still stuck at the " + "same gravity after that, then it is worth rousing the yeast with " + "a gentle swirl and considering a fresh sachet of dry yeast, " + "rehydrated in the usual way and pitched at the same temperature " + "the beer is sitting at." + ) + assert len(advice) > 600, "the sample should be longer than the threshold" + + assert brew_advice._strip_reasoning(advice) == advice + + def test_a_long_reply_with_no_instruction_is_dropped(): """Long, and nothing in it reads like an instruction - all working.""" working = ("Let me think about this.\n\n" + From 75f94d47e2b5c1bf4c131cf7a2614eeff0b12ca9 Mon Sep 17 00:00:00 2001 From: hermes Date: Mon, 9 Nov 2026 19:44:52 +1000 Subject: [PATCH 16/17] fix(advice): a deterministic split, not a guess at where the advice starts 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: 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). --- src/flask/brew_advice.py | 232 ++++++++------------------------------ tests/test_brew_advice.py | 180 +++++++++-------------------- 2 files changed, 99 insertions(+), 313 deletions(-) diff --git a/src/flask/brew_advice.py b/src/flask/brew_advice.py index d4fcc5b..31ce02b 100644 --- a/src/flask/brew_advice.py +++ b/src/flask/brew_advice.py @@ -78,6 +78,13 @@ temperature? - Should they rack, add more hops, raise the temperature, or wait? Rules: +- Your reply must have this exact shape: + + ADVICE: + + + The line "ADVICE:" comes FIRST, on its own line, with nothing before + it. Do not explain your reasoning and do not show your working. - Advise about the reading that is marked as the one to advise on. It may not be the most recent one - the brewer may be correcting an older reading - and the advice must be about the reading they asked about. @@ -213,9 +220,11 @@ def call_ollama(prompt: str, *, url: str | None = None, model: str | None = None truncated = finish_reason == "length" advice = _trim_advice(_strip_reasoning(content)) + if truncated: advice = _trim_advice(advice + " (the reply was cut off)") - return advice + + return advice, truncated def _trim_advice(text: str) -> str: @@ -239,199 +248,35 @@ def _trim_advice(text: str) -> str: return tail.strip() -#: The reasoning models sometimes put their working IN the content, even -#: with thinking turned off - "Let me look at this..." for several -#: thousand characters before the actual advice. The advice is the last -#: few paragraphs, so the preamble is cut off. -_PREAMBLE_MARKERS = ( - "let me look", - "let me think", - "let me work", - "okay, let me", - "first, let me", - "hmm,", - "wait,", - "let me analyze", - "let me consider", -) +#: The marker the model is asked to put at the start of its advice. +#: +#: Guessing where the advice begins by classifying prose is unreliable +#: in both directions - it emptied legitimate advice, and it let the +#: model's working through in full. A marker the model writes itself is +#: deterministic: everything before it is working, everything from it on +#: is the advice. +_ADVICE_MARKER = "ADVICE:" def _strip_reasoning(text: str) -> str: - """Cut a reasoning preamble off the front of a reply. + """Take the advice out of a reply. - The advice is the prose at the END - the working comes first, in - several paragraphs. So the preamble is removed by finding the last - paragraph break before the advice starts, not the first. + The model is asked to put ``ADVICE:`` on its own line at the start + of the advice, so the working before it can be dropped exactly, and + the advice kept exactly. - Only a preamble that looks like working is removed; a reply that - starts straight into the advice is left alone. + A reply with no marker is used as it stands. That is the safe + direction: the model was asked for the marker, so a reply without + one is one where it wrote the advice straight away - and if the + working did leak in, the brewer sees a slightly rambling reply + rather than no advice at all. """ - lowered = text.lower() + index = text.find(_ADVICE_MARKER) + if index == -1: + return text.strip() - # A reply that is ALL working - no advice paragraph at all - has to - # be caught too. The marker phrases are a hint, not the only signal: - # a reply can be pure working without ever saying "let me think". - # - # The working is DROPPED, not returned. Returning it unchanged would - # be a no-op, and the brewer would be shown the model thinking about - # their beer instead of any advice at all. - paragraphs = [p.strip() for p in text.split("\n\n") if p.strip()] - if paragraphs and _is_all_working(text, paragraphs): - # It is all working. It is dropped, so the brewer sees a clear - # "could not be generated" rather than the model thinking about - # their beer. - return "" - - for marker in _PREAMBLE_MARKERS: - index = lowered.find(marker) - # index 0 is fine - a reply can START with "Let me look at this". - if index < 0: - continue - - tail = text[index:] - # The advice is the LAST paragraph of the tail. Split on the - # blank lines and take the paragraphs from the end, until the - # running text stops looking like working. - paragraphs = [p.strip() for p in tail.split("\n\n") if p.strip()] - if len(paragraphs) < 2: - # A single paragraph: the whole tail is one block of working - # with the advice inside it, so it is kept whole rather than - # thrown away. - continue - - # Walk back from the end while the paragraph still reads like the - # model's working rather than the brewer's advice. - kept = [paragraphs[-1]] - for paragraph in reversed(paragraphs[:-1]): - if _looks_like_advice(paragraph): - kept.append(paragraph) - else: - break - kept.reverse() - - candidate = "\n\n".join(kept).strip() - if len(candidate) >= 80: - return candidate - return text - - -#: Words that mark a paragraph as the ADVICE rather than the model's -#: working. The advice talks about what the brewer should DO. -#: Phrases that mark a paragraph as the ADVICE rather than the model's -#: working. They are matched as PHRASES, not bare words - "keep" alone -#: matches "I need to keep in mind that the hydrometer was read at a -#: different temperature", which is working, not advice. -_ADVICE_MARKERS = ( - "you should", - "you could", - "you may want", - "you might", - "you will want", - "give it", - "let it sit", - "let it ferment", - "take another reading", - "take a reading", - "raise it to", - "raise the", - "lower the", - "rack it", - "you could repitch", - "i would suggest", - "i suggest", - "i recommend", - "in your case", - "here is", - "here's", - "don't", - "do not", -) - - -#: A reply longer than this is the model's working, not the advice. -#: The advice is meant to be two to five sentences. -_WORKING_LENGTH = 600 - - -def _is_all_working(text: str, paragraphs: list[str]) -> bool: - """True when the reply is the model's working rather than advice. - - The reliable signal is SHAPE, not length. A legitimate advice reply - can be a thousand characters, and a short working reply can be a - few hundred - so length alone gets both wrong. - - 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. - """ - if _has_advice_marker(text): - # It contains an instruction, so part of it is advice. - return False - - lowered = text.lower() - hedging = sum(lowered.count(word) for word in _HEDGING_WORDS) - - if hedging == 0: - # No instruction and no hedging about the data. This is advice, - # however long it is. - return False - - if len(text) <= _WORKING_LENGTH: - # Some hedging, but it is short - that is a brewer's note that - # happens to use a hedging word, not a page of working. - return False - - # Hedging, no instruction, and long: the working. - return True - - -#: The words the model uses when it is working through the data rather -#: than telling the brewer what to do. -_HEDGING_WORDS = ( - "perhaps", "maybe", "possibly", "could be", "might be", "might", - "i think", "i am not sure", "i'm not sure", "actually", "hmm", - "wait,", "let me", "i need to", "the question is", "unclear", - "i am unsure", "not sure", "unfortunately", -) - - -def _has_advice_marker(text: str) -> bool: - """True when the text contains any advice marker at all. - - Used on the WHOLE reply rather than paragraph by paragraph, because - advice can be phrased without any of the markers - "Actually the - drop is fine.\n\nWait a couple of days and check again." is advice, - and judging it paragraph by paragraph emptied it. - """ - lowered = text.lower() - return any(marker in lowered for marker in _ADVICE_MARKERS) - - -def _looks_like_advice(paragraph: str) -> bool: - """True when a paragraph reads like advice to the brewer. - - The model's working is full of hedging - "perhaps", "or maybe", "it - could be" - and is about interpreting the numbers. The advice is - about what to do. - """ - lowered = paragraph.lower() - if any(marker in lowered for marker in _ADVICE_MARKERS): - return True - # A paragraph that reads like working is not advice. The signs are - # hedging ("perhaps", "could be"), and the model talking to itself - # ("wait", "hmm", "so", "actually") rather than to the brewer. - working_words = ("perhaps", "maybe", "possibly", "could be", "might be", - "i think", "i am not sure", "actually", "hmm", "wait", - "let me", "so on brew day", "there is a discrepancy", - "that is a discrepancy", "which means", - "i need to keep in mind", "the question is", - "this could mean", "it could be", "or maybe") - if any(word in lowered for word in working_words): - return False - return True + advice = text[index + len(_ADVICE_MARKER):].strip() + return advice or "" def generate_advice(run: dict, reading: dict, history: list[dict], *, @@ -446,7 +291,18 @@ def generate_advice(run: dict, reading: dict, history: list[dict], *, """ prompt = build_prompt(run, reading, history, corrected=corrected) try: - return True, call_ollama(prompt, url=url, model=model, timeout=timeout) + advice, _truncated = call_ollama(prompt, url=url, model=model, + timeout=timeout) + if not advice: + # The reply had no advice in it - the model wrote only its + # working, or the marker was missing and nothing followed it. + # Storing an empty string with advice_ok=True would render + # as "Thinking..." for ever, so it is recorded as a failure + # with a note the brewer can see. + log.warning("the model returned no advice") + return False, ("Advice could not be generated - the model " + "returned only its reasoning.") + return True, advice except urllib.error.HTTPError as exc: # HTTPError is a subclass of URLError, so it has to be caught # FIRST or it would be swallowed by the unreachable branch. diff --git a/tests/test_brew_advice.py b/tests/test_brew_advice.py index d4bf319..2a8e52e 100644 --- a/tests/test_brew_advice.py +++ b/tests/test_brew_advice.py @@ -117,7 +117,7 @@ def test_a_successful_call_returns_the_text(monkeypatch): class Response: def read(self): return json.dumps({ - "choices": [{"message": {"content": " Leave it alone, it is still fermenting. "}}] + "choices": [{"message": {"content": "ADVICE: Leave it alone, it is still fermenting."}}] }).encode() def __enter__(self): @@ -226,38 +226,70 @@ def test_the_token_budget_leaves_room_for_the_reasoning(monkeypatch): assert captured["body"]["max_tokens"] >= 8000 -def test_a_reasoning_preamble_is_cut_off(): - """Some models put their working in the content even with thinking off. - The advice is the prose at the end; the working comes first, in - several paragraphs. The preamble is removed by finding the advice, - not by taking the text after the first blank line. + +def test_the_model_is_asked_for_an_advice_marker(): + """The marker is what makes the split deterministic. + + Guessing where the advice begins by classifying prose failed in both + directions - it emptied legitimate advice, and it let the model's + working through in full. A marker the model writes itself is exact. """ + assert "ADVICE:" in brew_advice.SYSTEM_PROMPT + + +def test_the_working_before_the_marker_is_dropped(): + """Everything before the marker is the model's working.""" reply = ( - "Let me look at this. The brewer has a Christmas beer, OG 1.042, " - "fermenting at 19C.\n\n" - "So on brew day, the reading is 1.038, but the OG is stated as 1.042. " - "That is a discrepancy of 4 points.\n\n" - "Wait, the reading on brew day is 1.038 but the recorded OG is 1.042.\n\n" - "On brew day a gravity of 1.038 against a recorded original of 1.042 " - "is a small discrepancy but not a problem. Take the OG from a " - "well-mixed sample, and start the fermentation at 18-20C." + "Let me think about what this reading means. The original gravity " + "is 1.055 and the current reading is 1.030, which is a drop of 25 " + "points over nine days, and I need to work out what that means for " + "the expected attenuation for this style at this temperature.\n\n" + "ADVICE:\n" + "Warm the fermenter to 20C and give the yeast a gentle swirl to " + "rouse it, then take another reading in three days." ) stripped = brew_advice._strip_reasoning(reply) - assert stripped.startswith("On brew day a gravity of 1.038") - assert "Let me look at this" not in stripped - assert "That is a discrepancy of 4 points" not in stripped + assert stripped.startswith("Warm the fermenter") + assert "Let me think" not in stripped + assert "1.055" not in stripped -def test_a_reply_that_starts_with_the_advice_is_untouched(): - """The stripper must not eat a good reply.""" - reply = ("The gravity looks fine for day nine. Give it three more days " - "before you do anything, and take another reading on Friday.") +def test_a_reply_with_the_marker_is_kept_whole(): + """A legitimate advice reply is never emptied. + + Whatever length it is, and whatever hedging words it happens to + contain, the advice after the marker is the advice. + """ + advice = ( + "Actually the drop is fine. Perhaps it will finish this week. Wait " + "a couple of days and check again, and if it has not moved, give " + "the yeast a gentle swirl and take another reading before you " + "consider repitching anything." + ) + reply = "ADVICE:\n" + advice + + assert brew_advice._strip_reasoning(reply) == advice + + +def test_a_reply_without_the_marker_is_used_as_it_stands(): + """The safe direction when the model does not use the marker. + + The brewer sees a slightly rambling reply rather than no advice at + all. + """ + reply = "The gravity looks fine for day nine. Give it three more days." + assert brew_advice._strip_reasoning(reply) == reply +def test_a_reply_with_only_working_after_the_marker_is_empty(): + """Nothing after the marker means there is no advice.""" + assert brew_advice._strip_reasoning("ADVICE:") == "" + + def test_an_over_long_reply_keeps_the_advice_at_the_end(): """The advice is at the END of the reply, so a plain truncation from the front would cut it off and leave only the model's working. @@ -287,7 +319,7 @@ def test_a_short_reply_is_not_trimmed(): def test_the_advice_is_truncated(): """A runaway reply must not fill the reading document.""" - long_text = "x" * 5000 + long_text = "ADVICE: " + ("x" * 5000) class Response: def read(self): @@ -640,112 +672,10 @@ def test_a_comment_only_correction_cannot_be_overwritten(repo): assert after["comment"] == "corrected comment" -def test_a_reply_that_is_all_working_stores_no_working(): - """A reply with no advice paragraph in it must not be stored. - - The old code returned the text unchanged, which was a no-op - and in - the case where a preamble marker WAS present it returned the whole - reply, working included. So the brewer was shown the model thinking - about their beer instead of any advice at all. - """ - working_only = ( - "Let me think about what this reading means. The original gravity is " - "1.055 and the current reading is 1.030, which is a drop of 25 points " - "over nine days at 19 degrees, and I need to work out what that means " - "for the expected attenuation.\n\n" - "Perhaps the drop is smaller than expected, and it could be that the " - "yeast is stressed by the temperature rather than actually done, " - "because most ale strains slow down considerably below 18 degrees.\n\n" - "Wait, I need to keep in mind that the hydrometer was read at a " - "different temperature than the sample, which would make the reading " - "read lower than it really is, and I have no way of knowing from the " - "data alone what the correction factor was.\n\n" - "I need to keep in mind that the original gravity here was measured " - "before the hops went in, and the reading on brew day was 1.055, so " - "the drop is twenty-five points against a potential of fifty-five.\n\n" - "So the question is what the expected attenuation should be for this " - "style at this temperature, and I am not sure without more data or a " - "temperature-corrected reading to compare against." - ) - - stripped = brew_advice._strip_reasoning(working_only) - - assert stripped == "", ( - f"the model's working was kept as advice: {stripped[:120]!r}" - ) -def test_a_short_reply_is_advice_even_without_a_marker_word(): - """Real advice is often phrased without any marker phrase. - - "Actually the drop is fine. Wait a couple of days and check again." - is advice - and judging it paragraph by paragraph emptied it, so the - brewer saw nothing at all. - """ - reply = ("Actually the drop is fine.\n\n" - "Wait a couple of days and check again.") - - assert brew_advice._strip_reasoning(reply) == reply -def test_a_long_legitimate_advice_reply_is_kept(): - """Length alone 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 length threshold - gets both wrong. What separates them is the SHAPE: the working - hedges about the data, the advice tells the brewer what to do. - """ - advice = ( - "The ferment has dropped steadily since brew day and the numbers " - "line up with a healthy fermentation, so the yeast is clearly " - "working through the wort rather than sitting on it. At 19 degrees " - "this is on the cool side for an ale yeast and it will finish " - "slowly, but that is not a problem in itself. Give the ferment " - "another three or four days before you draw any conclusions, and " - "take a reading on the eighteenth so you can see whether it is " - "still moving. Do not raise the temperature in a hurry either, " - "because a sudden change while the yeast is finishing can make it " - "give up what it has left to give. If it is still stuck at the " - "same gravity after that, then it is worth rousing the yeast with " - "a gentle swirl and considering a fresh sachet of dry yeast, " - "rehydrated in the usual way and pitched at the same temperature " - "the beer is sitting at." - ) - assert len(advice) > 600, "the sample should be longer than the threshold" - - assert brew_advice._strip_reasoning(advice) == advice - - -def test_a_long_reply_with_no_instruction_is_dropped(): - """Long, and nothing in it reads like an instruction - all working.""" - working = ("Let me think about this.\n\n" + - "Perhaps the yeast is stressed, and it could be that the " - "temperature is too low. " * 20) - - assert brew_advice._strip_reasoning(working) == "" - - -def test_a_long_reply_with_an_instruction_keeps_it(): - """The working is trimmed from it, but the advice stays.""" - mixed = ("Let me think about this.\n\n" + - "Perhaps the yeast is stressed. " * 15 + - "\n\nWarm the fermenter to 20C and give the yeast a gentle " - "swirl to rouse it.") - - out = brew_advice._strip_reasoning(mixed) - assert "Warm the fermenter" in out - - -def test_a_paragraph_about_keeping_something_in_mind_is_not_advice(): - """"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 to the brewer. - """ - working = ("I need to keep in mind that the hydrometer was read at a " - "different temperature, which explains the discrepancy.") - assert brew_advice._looks_like_advice(working) is False def test_the_token_is_the_primary_guard(repo): @@ -898,7 +828,7 @@ def test_a_reply_cut_off_at_the_token_ceiling_is_marked(repo): return json.dumps({ "choices": [{ "finish_reason": "length", - "message": {"content": "The gravity looks fine so far, but"}, + "message": {"content": "ADVICE: The gravity looks fine so far, but"}, }], }).encode() From 8de3d011dbe34d220840a6d7bff99ef913ecb620 Mon Sep 17 00:00:00 2001 From: hermes Date: Mon, 9 Nov 2026 19:46:13 +1000 Subject: [PATCH 17/17] fix(advice): find the marker however the model formats it 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). --- src/flask/brew_advice.py | 35 +++++++++++++++++++++++++++++++---- tests/test_brew_advice.py | 22 ++++++++++++++++++++++ 2 files changed, 53 insertions(+), 4 deletions(-) diff --git a/src/flask/brew_advice.py b/src/flask/brew_advice.py index 31ce02b..0dce4b2 100644 --- a/src/flask/brew_advice.py +++ b/src/flask/brew_advice.py @@ -25,6 +25,7 @@ from __future__ import annotations import json import logging import os +import re import urllib.error import urllib.request @@ -271,12 +272,38 @@ def _strip_reasoning(text: str) -> str: working did leak in, the brewer sees a slightly rambling reply rather than no advice at all. """ - index = text.find(_ADVICE_MARKER) - if index == -1: + index = _find_marker(text) + if index is None: return text.strip() - advice = text[index + len(_ADVICE_MARKER):].strip() - return advice or "" + advice = text[index:].strip() + + # The model may wrap the marker in markdown or a code block. Strip + # the leftover formatting so the advice reads as prose. + advice = re.sub(r"^\*\*|\*\*$", "", advice).strip() + if advice.startswith("```"): + advice = advice[3:] + if advice.endswith("```"): + advice = advice[:-3] + if advice.startswith("##"): + advice = advice.lstrip("#").strip() + + return advice.strip() or "" + + +def _find_marker(text: str) -> int | None: + """Where the advice starts, or ``None`` if there is no marker. + + The marker is matched case-insensitively and with the markdown + decoration the model might put around it - ``**ADVICE:**``, + ``## ADVICE:`` - because the instruction asks for "ADVICE:" but the + model formats it its own way often enough to matter. + """ + match = re.search(r"^[\s#>\*]*advice\s*:[\s\*>]*", text, + re.IGNORECASE | re.MULTILINE) + if match: + return match.end() + return None def generate_advice(run: dict, reading: dict, history: list[dict], *, diff --git a/tests/test_brew_advice.py b/tests/test_brew_advice.py index 2a8e52e..12dbf57 100644 --- a/tests/test_brew_advice.py +++ b/tests/test_brew_advice.py @@ -274,6 +274,28 @@ def test_a_reply_with_the_marker_is_kept_whole(): assert brew_advice._strip_reasoning(reply) == advice +def test_the_marker_is_found_however_the_model_formats_it(): + """The model formats the marker its own way often enough to matter. + + The instruction asks for "ADVICE:", but the model writes + "**ADVICE:**", "## ADVICE:" or puts it in a code block. Each of + those has to extract the advice cleanly, with no leftover markdown. + """ + advice = "Warm it to 20C and wait." + + for reply in ( + f"ADVICE:\n{advice}", + f"**ADVICE:**\n{advice}", + f"## ADVICE:\n{advice}", + f"```\nADVICE:\n{advice}\n```", + f"**ADVICE:**\n```\n{advice}\n```", + f"advice:\n{advice}", + ): + assert brew_advice._strip_reasoning(reply) == advice, ( + f"the marker was not extracted cleanly from {reply!r}" + ) + + def test_a_reply_without_the_marker_is_used_as_it_stands(): """The safe direction when the model does not use the marker.