diff --git a/src/flask/brew_advice.py b/src/flask/brew_advice.py index ecfc5bf..d0b0e31 100644 --- a/src/flask/brew_advice.py +++ b/src/flask/brew_advice.py @@ -276,7 +276,10 @@ def _strip_reasoning(text: str) -> str: # be a no-op, and the brewer would be shown the model thinking about # their beer instead of any advice at all. paragraphs = [p.strip() for p in text.split("\n\n") if p.strip()] - if len(paragraphs) > 1 and not any(_looks_like_advice(p) for p in paragraphs): + if paragraphs and _is_all_working(text, paragraphs): + # It is all working. It is dropped, so the brewer sees a clear + # "could not be generated" rather than the model thinking about + # their beer. return "" for marker in _PREAMBLE_MARKERS: @@ -291,6 +294,9 @@ def _strip_reasoning(text: str) -> str: # running text stops looking like working. paragraphs = [p.strip() for p in tail.split("\n\n") if p.strip()] if len(paragraphs) < 2: + # A single paragraph: the whole tail is one block of working + # with the advice inside it, so it is kept whole rather than + # thrown away. continue # Walk back from the end while the paragraph still reads like the @@ -342,6 +348,51 @@ _ADVICE_MARKERS = ( ) +#: A reply longer than this is the model's working, not the advice. +#: The advice is meant to be two to five sentences. +_WORKING_LENGTH = 600 + + +def _is_all_working(text: str, paragraphs: list[str]) -> bool: + """True when the reply is the model's working rather than advice. + + Two signals, because either alone is unreliable: + + - **Length.** The working runs to thousands of characters; the + advice is two to five sentences. + - **Shape.** The working interprets the numbers ("perhaps the yeast + is stressed, it could be that..."); the advice tells the brewer + what to do. + + A reply that is SHORT is kept even if it has no marker word - real + advice is often phrased without one ("Actually the drop is fine. + Wait a couple of days and check again."). + """ + if len(text) <= _WORKING_LENGTH: + # Too short to be the model's working. It is advice, whatever + # it is worded like. + return False + + if _has_advice_marker(text): + # It contains an instruction, so part of it is advice. + return False + + # Long, and nothing in it reads like an instruction. + return True + + +def _has_advice_marker(text: str) -> bool: + """True when the text contains any advice marker at all. + + Used on the WHOLE reply rather than paragraph by paragraph, because + advice can be phrased without any of the markers - "Actually the + drop is fine.\n\nWait a couple of days and check again." is advice, + and judging it paragraph by paragraph emptied it. + """ + lowered = text.lower() + return any(marker in lowered for marker in _ADVICE_MARKERS) + + def _looks_like_advice(paragraph: str) -> bool: """True when a paragraph reads like advice to the brewer. diff --git a/src/flask/mongo/repository.py b/src/flask/mongo/repository.py index 9d5ddec..e223f35 100644 --- a/src/flask/mongo/repository.py +++ b/src/flask/mongo/repository.py @@ -267,14 +267,20 @@ class BeerRepository: # 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. - # - # An EMPTY token is not a usable guard: it means the reading - # predates the token, so there is no token to match. - # Querying on it would match nothing, and the advice would - # never be written for the existing readings. query["advice_token"] = str(expected_token) + elif expected_token == "": + # The reading has NEVER been edited, so it has no token at + # all. Match on the ABSENCE of one, which still tells the + # generations apart: an edit writes a token, so a stale + # thread's "" stops matching the moment the reading is + # corrected. + # + # Falling back to the gravity here would re-open the hole + # this guard exists to close - a comment-only correction + # leaves the gravity unchanged. + query["advice_token"] = {"$exists": False} elif expected_sg is not None: - # No token - a direct call with a gravity to check against. + # A direct call with a gravity to check against. query["sg"] = str(expected_sg) # `$in` matches either form. A plain list would be an exact diff --git a/src/flask/views/brews.py b/src/flask/views/brews.py index c384365..c08a20c 100644 --- a/src/flask/views/brews.py +++ b/src/flask/views/brews.py @@ -145,7 +145,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 " - "no longer holds the gravity it was written for", + "has been edited since the advice was generated, so the " + "advice no longer matches it", 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 e27e95c..0a74a15 100644 --- a/tests/test_brew_advice.py +++ b/tests/test_brew_advice.py @@ -649,15 +649,23 @@ def test_a_reply_that_is_all_working_stores_no_working(): about their beer instead of any advice at all. """ working_only = ( - "Let me think about what this reading means.\n\n" + "Let me think about what this reading means. The original gravity is " + "1.055 and the current reading is 1.030, which is a drop of 25 points " + "over nine days at 19 degrees, and I need to work out what that means " + "for the expected attenuation.\n\n" "Perhaps the drop is smaller than expected, and it could be that the " - "yeast is stressed.\n\n" + "yeast is stressed by the temperature rather than actually done, " + "because most ale strains slow down considerably below 18 degrees.\n\n" "Wait, I need to keep in mind that the hydrometer was read at a " - "different temperature.\n\n" + "different temperature than the sample, which would make the reading " + "read lower than it really is, and I have no way of knowing from the " + "data alone what the correction factor was.\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." + "before the hops went in, and the reading on brew day was 1.055, so " + "the drop is twenty-five points against a potential of fifty-five.\n\n" + "So the question is what the expected attenuation should be for this " + "style at this temperature, and I am not sure without more data or a " + "temperature-corrected reading to compare against." ) stripped = brew_advice._strip_reasoning(working_only) @@ -667,6 +675,39 @@ def test_a_reply_that_is_all_working_stores_no_working(): ) +def test_a_short_reply_is_advice_even_without_a_marker_word(): + """Real advice is often phrased without any marker phrase. + + "Actually the drop is fine. Wait a couple of days and check again." + is advice - and judging it paragraph by paragraph emptied it, so the + brewer saw nothing at all. + """ + reply = ("Actually the drop is fine.\n\n" + "Wait a couple of days and check again.") + + assert brew_advice._strip_reasoning(reply) == reply + + +def test_a_long_reply_with_no_instruction_is_dropped(): + """Long, and nothing in it reads like an instruction - all working.""" + working = ("Let me think about this.\n\n" + + "Perhaps the yeast is stressed, and it could be that the " + "temperature is too low. " * 20) + + assert brew_advice._strip_reasoning(working) == "" + + +def test_a_long_reply_with_an_instruction_keeps_it(): + """The working is trimmed from it, but the advice stays.""" + mixed = ("Let me think about this.\n\n" + + "Perhaps the yeast is stressed. " * 15 + + "\n\nWarm the fermenter to 20C and give the yeast a gentle " + "swirl to rouse it.") + + out = brew_advice._strip_reasoning(mixed) + assert "Warm the fermenter" in out + + def test_a_paragraph_about_keeping_something_in_mind_is_not_advice(): """"Keep" alone is not an advice marker. @@ -745,6 +786,46 @@ def test_the_advice_is_written_for_a_reading_that_predates_the_token(repo): assert after["comment"] == "a comment" +def test_a_pre_token_reading_is_still_guarded_against_a_stale_write(repo): + """A reading that has never been edited has no token at all. + + The guard matches on the ABSENCE of a token, so an edit still tells + the generations apart. Falling back to the gravity here would + re-open the hole, because a comment-only correction leaves the + gravity unchanged. + """ + repo.readings.insert_one({ + "_id": "preToken0000000000000010", + "beer_run_id": "1", + "beer_type": "Ale", + "sg": "1050", + "date": "2026-11-01", + "final_reading": "False", + "comment": "first comment", + }) + + # The advice thread starts on this generation - no token. + assert repo.set_reading_advice("preToken0000000000000010", "FRESH ADVICE", + True, expected_sg=None, + expected_token="") == 1 + + # The brewer corrects the comment. The edit writes a token. + repo.upsert_reading(beer_run_id="1", beer_type="Ale", sg=1050, + reading_date="2026-11-01", final_reading=False, + comment="corrected comment") + + # The late thread for the OLD generation lands. It carries no token, + # and the reading now HAS one - so it must be refused. + stale = repo.set_reading_advice("preToken0000000000000010", "STALE ADVICE", + True, expected_sg=None, + expected_token="") + assert stale == 0, "the stale advice overwrote the correction" + + after = repo.readings.find_one({"beer_run_id": "1"}) + assert after.get("advice") is None + assert after["comment"] == "corrected comment" + + def test_a_reply_cut_off_at_the_token_ceiling_is_marked(repo): """A reply that stopped at the token limit is incomplete.