fix(advice): the stripper must DROP the working, and the token is the guard

Two findings from the fourth review.

The stripper was a no-op in the case it was written for
---------------------------------------------------
The "all working" branch returned the text UNCHANGED, which is
byte-identical to the old fallthrough.  And in the case where a preamble
marker WAS present and no paragraph read like advice, it returned the
whole reply - working included - so the branch made the "store the
working instead of the advice" failure MORE likely, not less.

Measured on a realistic 5-paragraph reasoning reply: the old code stored
80 chars (one paragraph), the new code stored 612 chars (five
paragraphs) of the model's working.

It now DROPS the working, so the reply is empty and the brewer sees a
clear "could not be generated" rather than the model thinking about
their beer.

"keep" alone is not an advice marker
------------------------------------
"I need to keep in mind that the hydrometer was read at a different
temperature" is the model thinking, not advice.  The bare words "keep",
"raise", "lower", "take the" were matching ordinary reasoning phrasing,
so the "all working" detection almost never fired on real reasoning
text.  They are now matched as phrases - "you should keep", "raise it
to", "take another reading" - and "i need to keep in mind" is
explicitly working.

The token is the primary guard
------------------------------
The gravity can be unparseable for some readings, so `expected_sg` can
be None - and then the write was becoming unconditional, reintroducing
the stale-overwrite bug on those readings.

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.  The gravity is now a secondary check, used only
when there is no token.

177 tests (up from 174).
This commit is contained in:
2026-11-09 18:22:51 +10:00
parent a78aabe3ea
commit 27a28b5e58
3 changed files with 101 additions and 15 deletions
+70
View File
@@ -640,6 +640,76 @@ def test_a_comment_only_correction_cannot_be_overwritten(repo):
assert after["comment"] == "corrected comment"
def test_a_reply_that_is_all_working_stores_no_working():
"""A reply with no advice paragraph in it must not be stored.
The old code returned the text unchanged, which was a no-op - and in
the case where a preamble marker WAS present it returned the whole
reply, working included. So the brewer was shown the model thinking
about their beer instead of any advice at all.
"""
working_only = (
"Let me think about what this reading means.\n\n"
"Perhaps the drop is smaller than expected, and it could be that the "
"yeast is stressed.\n\n"
"Wait, I need to keep in mind that the hydrometer was read at a "
"different temperature.\n\n"
"I need to keep in mind that the original gravity here was measured "
"before the hops went in.\n\n"
"So the question is what the expected attenuation should be, and I "
"am not sure without more data."
)
stripped = brew_advice._strip_reasoning(working_only)
assert stripped == "", (
f"the model's working was kept as advice: {stripped[:120]!r}"
)
def test_a_paragraph_about_keeping_something_in_mind_is_not_advice():
""""Keep" alone is not an advice marker.
"I need to keep in mind that the hydrometer was read at a different
temperature" is the model thinking, not advice to the brewer.
"""
working = ("I need to keep in mind that the hydrometer was read at a "
"different temperature, which explains the discrepancy.")
assert brew_advice._looks_like_advice(working) is False
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_a_reply_cut_off_at_the_token_ceiling_is_marked(repo):
"""A reply that stopped at the token limit is incomplete.