From e603f239288000f3a58872e0020ccbe5b5069e72 Mon Sep 17 00:00:00 2001 From: hermes Date: Mon, 9 Nov 2026 22:26:29 +1000 Subject: [PATCH] fix(advice): guard the advice write on the comment as well as the token The editing token catches every edit the application makes, because upsert_reading writes a fresh token on every write. A document edited outside the application - by a script, or by hand - keeps its empty token, so a stale thread's write would still match on the gravity alone and could put advice about the old comment back. The advice is generated from the comment as well as the gravity, so the comment is now a second, independent guard on the write. A stale write can only land if BOTH the gravity and the comment are unchanged. The stored comment and the one the view sees can differ by trailing whitespace - two of the live readings are padded - so the match is a stripped, anchored regex rather than an exact one. Also: the log message said the reading 'no longer holds the gravity it was written for', but the guard is the token and the comment. It now says what actually happened. 194 tests pass. --- src/flask/mongo/repository.py | 29 ++++++++++++++++++++++++- src/flask/views/brews.py | 10 +++++++-- tests/test_brew_advice.py | 41 +++++++++++++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 3 deletions(-) diff --git a/src/flask/mongo/repository.py b/src/flask/mongo/repository.py index d502d65..6e15c8a 100644 --- a/src/flask/mongo/repository.py +++ b/src/flask/mongo/repository.py @@ -18,6 +18,7 @@ Two collections are used: from __future__ import annotations import logging +import re import os import threading from typing import Any @@ -232,7 +233,8 @@ class BeerRepository: def set_reading_advice(self, reading_id: str, advice: str, ok: bool = True, expected_sg: int | None = None, - expected_token: str | None = None) -> int: + expected_token: str | None = None, + expected_comment: 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 @@ -286,6 +288,31 @@ class BeerRepository: # A direct call with a gravity to check against. query["sg"] = str(expected_sg) + if expected_comment is not None: + # The comment the advice was written for. + # + # This is a SECOND, independent guard. The token covers + # every edit the application makes, but a document edited + # outside the application - by a script, or by hand in a + # database tool - would keep its empty token and a stale + # thread's write would still match on the gravity alone. + # + # The advice is generated from the comment as well as the + # gravity, so the comment is part of what the advice + # describes. Matching on it means a stale write can only + # land if BOTH the gravity and the comment are unchanged. + # + # The stored comment and the one the view sees can differ by + # trailing whitespace, so a regex is used to match either. + # + # `^comment\s*$` matches the stored value with any trailing + # whitespace, and the exact value too. It is anchored, so it + # cannot match a longer comment that merely starts the same + # way. + query["comment"] = { + "$regex": "^" + re.escape(expected_comment.strip()) + r"\s*$" + } + # `$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( diff --git a/src/flask/views/brews.py b/src/flask/views/brews.py index 29d268c..3ed5f45 100644 --- a/src/flask/views/brews.py +++ b/src/flask/views/brews.py @@ -127,6 +127,11 @@ def _start_advice(run_id: str, reading_id: str, corrected: bool = False) -> None # cannot overwrite a correction with advice about the old value. advice_sg = latest.get("sg") advice_token = latest.get("advice_token") + # The comment is part of what the advice describes, so it is a + # second, independent guard on the write. The token covers every + # edit the application makes; this also covers a document edited + # outside it. + advice_comment = latest.get("comment") or "" def write(read_id: str, ok: bool, text: str) -> None: # The client is SHARED, not built per reading. A MongoClient @@ -145,6 +150,7 @@ def _start_advice(run_id: str, reading_id: str, corrected: bool = False) -> None read_id, text, ok, expected_sg=advice_sg, expected_token=advice_token, + expected_comment=advice_comment, ) if not written: # The reading was corrected while the advice was being @@ -153,8 +159,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 " - "has been edited since the advice was generated, so the " - "advice no longer matches it", + "has been edited since the advice was generated, so its " + "editing token or comment no longer matches the advice", 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 4eb5860..6b50828 100644 --- a/tests/test_brew_advice.py +++ b/tests/test_brew_advice.py @@ -1121,6 +1121,47 @@ def test_stale_advice_cannot_overwrite_a_comment_only_correction(repo): assert doc["comment"] == "corrected comment" +def test_stale_advice_cannot_overwrite_a_comment_edited_outside_the_app(repo): + """The comment is a guard in its own right, not just the token. + + The token covers every edit the application makes. A document + edited outside it - by a script, or by hand - keeps its empty + token, so a stale thread's write would still match on the gravity + alone. Matching the comment as well closes that. + """ + repo.readings.insert_one({ + "_id": "outsideedit01", + "beer_run_id": "1", + "date": "2026-01-01", + "sg": "1040", + "comment": "original comment", + "final_reading": "False", + # No advice_token - a legacy reading. + }) + + # The advice is written for the original comment. + assert repo.set_reading_advice( + "outsideedit01", "FRESH ADVICE", ok=True, + expected_token="", expected_comment="original comment") == 1 + + # Somebody edits the comment directly in the database, leaving the + # token empty - so the token cannot tell the generations apart. + repo.readings.update_one( + {"_id": "outsideedit01"}, + {"$set": {"comment": "edited outside the app"}}, + ) + + # The slow thread lands. It must be refused, even though the token + # still matches. + assert repo.set_reading_advice( + "outsideedit01", "STALE ADVICE", ok=True, + expected_token="", expected_comment="original comment") == 0 + + doc = repo.readings.find_one({"_id": "outsideedit01"}) + assert doc["advice"] == "FRESH ADVICE" + assert doc["comment"] == "edited outside the app" + + 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"})