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"})