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.
This commit is contained in:
2026-11-09 22:26:29 +10:00
parent a38febaca8
commit e603f23928
3 changed files with 77 additions and 3 deletions
+28 -1
View File
@@ -18,6 +18,7 @@ Two collections are used:
from __future__ import annotations from __future__ import annotations
import logging import logging
import re
import os import os
import threading import threading
from typing import Any from typing import Any
@@ -232,7 +233,8 @@ class BeerRepository:
def set_reading_advice(self, reading_id: str, advice: str, def set_reading_advice(self, reading_id: str, advice: str,
ok: bool = True, ok: bool = True,
expected_sg: int | None = None, 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. """Attach the LLM's advice to a reading.
The advice lives in its own field, so the brewer's own comment is 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. # A direct call with a gravity to check against.
query["sg"] = str(expected_sg) 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 # `$in` matches either form. A plain list would be an exact
# ARRAY match in Mongo, which is not what is wanted here. # ARRAY match in Mongo, which is not what is wanted here.
result = self.readings.update_one( result = self.readings.update_one(
+8 -2
View File
@@ -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. # cannot overwrite a correction with advice about the old value.
advice_sg = latest.get("sg") advice_sg = latest.get("sg")
advice_token = latest.get("advice_token") 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: def write(read_id: str, ok: bool, text: str) -> None:
# The client is SHARED, not built per reading. A MongoClient # 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, read_id, text, ok,
expected_sg=advice_sg, expected_sg=advice_sg,
expected_token=advice_token, expected_token=advice_token,
expected_comment=advice_comment,
) )
if not written: if not written:
# The reading was corrected while the advice was being # 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. # reading will land in its own turn.
log.info( log.info(
"the advice for reading %s was not stored: the reading " "the advice for reading %s was not stored: the reading "
"has been edited since the advice was generated, so the " "has been edited since the advice was generated, so its "
"advice no longer matches it", "editing token or comment no longer matches the advice",
read_id, read_id,
) )
except Exception: # noqa: BLE001 - never let the thread die noisily except Exception: # noqa: BLE001 - never let the thread die noisily
+41
View File
@@ -1121,6 +1121,47 @@ def test_stale_advice_cannot_overwrite_a_comment_only_correction(repo):
assert doc["comment"] == "corrected comment" 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): def test_a_failed_advice_is_marked_as_such(client, repo):
"""A generation failure reads differently from real advice.""" """A generation failure reads differently from real advice."""
repo.users.insert_one({"username": "beer", "password": "beer_pass"}) repo.users.insert_one({"username": "beer", "password": "beer_pass"})