fix(advice): guard the advice write on the comment as well as the token #21

Merged
hermes merged 1 commits from fix/advice-comment-guard into master 2026-11-09 22:27:01 +10:00
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"})