diff --git a/src/flask/brew_advice.py b/src/flask/brew_advice.py new file mode 100644 index 0000000..0dce4b2 --- /dev/null +++ b/src/flask/brew_advice.py @@ -0,0 +1,368 @@ +"""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 re +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") + +#: 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", "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. +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: +- 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. +- 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], 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')})"] + if r.get("comment"): + 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], + 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"): + 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, subject_id=reading.get("_id"))) + + parts.append("") + # 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) + + +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 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") + + 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") + 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") + + # 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, truncated + + +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 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: + """Take the advice out of a reply. + + 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. + + 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. + """ + index = _find_marker(text) + if index is None: + return text.strip() + + 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], *, + corrected: bool = False, + 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, corrected=corrected) + try: + 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. + 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, + 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, corrected=corrected) + 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..470e130 100644 --- a/src/flask/brewing.py +++ b/src/flask/brewing.py @@ -152,6 +152,14 @@ 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 + # 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: @@ -171,6 +179,9 @@ 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)), + advice_token=str(doc.get("advice_token") or ""), ) @@ -189,6 +200,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 +652,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 +667,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..f781ada 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 @@ -46,6 +47,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. @@ -180,14 +208,96 @@ class BeerRepository: "date": _as_date(reading_date), "final_reading": "True" if final_reading else "False", "comment": str(comment), - } + }, + # 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) 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, + 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. + + 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. + + 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: + return 0 + + query: dict = {"_id": {"$in": doc_id}} + 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. + 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. + # `$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) + + # `$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( + query, + {"$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 +424,82 @@ 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 + +#: 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. + + Thread-safe: the first caller builds it, the rest wait and get the + same one. + """ + global _shared_repository + + if _shared_repository is None: + 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 + + +def _as_doc_id(value: object): + """Coerce a reading document id to something Mongo will match. + + 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. + """ + from bson import ObjectId + + if isinstance(value, ObjectId): + return [value] + if isinstance(value, str) and value: + try: + # 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: + # Not hex - it can only be the original app's string id. + return [value] + # 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: """Coerce a reading date to a plain ``YYYY-MM-DD`` string.""" if isinstance(value, str): @@ -336,6 +522,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/static/css/app.css b/src/flask/static/css/app.css index 6bc8a98..fb54866 100644 --- a/src/flask/static/css/app.css +++ b/src/flask/static/css/app.css @@ -689,3 +689,71 @@ 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); +} + +/* 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); + 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..c08a20c 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,125 @@ 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, 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 + comment is never touched. + + The background thread cannot use the request-scoped repository - that + client is closed when the request ends - so it uses the shared + process-wide one. + """ + 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": str(reading._id), + "advice_token": reading.advice_token, + "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), + }) + + # 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 + log.exception("could not gather the reading for the advice") + return + + # 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 + # 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 get_shared_repository + + try: + shared = get_shared_repository() + 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 + # 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 " + "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 + log.exception("could not store the generated advice") + + thread = threading.Thread( + target=brew_advice.generate_advice_in_background, + args=(run_info, latest, readings, write, corrected), + 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 +230,17 @@ 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) + # 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(), sg=sg_points, reading_date=form.Date.data.isoformat(), @@ -113,6 +248,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, 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 new file mode 100644 index 0000000..12dbf57 --- /dev/null +++ b/tests/test_brew_advice.py @@ -0,0 +1,1019 @@ +"""Tests for the LLM brewing advice: the prompt, the storage and the display.""" + +from __future__ import annotations + +import json +from contextlib import contextmanager +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_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", + "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": "ADVICE: 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"}, []) + + # 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_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 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("Warm the fermenter") + assert "Let me think" not in stripped + assert "1.055" not in stripped + + +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_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. + + 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. + + 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 = "ADVICE: " + ("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_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_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, corrected=False): + captured["reading"] = reading + captured["history"] = history + captured["corrected"] = corrected + 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" + ) + + # 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. + + 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_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_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_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_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_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_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. + + 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": "ADVICE: 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. + + 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" + + # 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 + + +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