From 75f94d47e2b5c1bf4c131cf7a2614eeff0b12ca9 Mon Sep 17 00:00:00 2001 From: hermes Date: Mon, 9 Nov 2026 19:44:52 +1000 Subject: [PATCH] fix(advice): a deterministic split, not a guess at where the advice starts The stripper tried to work out where the model's working ends and the advice begins by classifying prose - marker words, hedging words, length. It failed in BOTH directions, and the review caught both: - it emptied legitimate advice (a 1,093-character reply containing one hedging word and no marker phrase was reduced to ""), so the brewer saw nothing at all; and - it let the model's working through in full (a 1,053-character reply with no hedging word was returned verbatim), so the brewer was shown the model thinking about their beer. Neither a marker list nor a length threshold is reliable. Prose is not a format. So the model is asked for the format instead. Its reply must have this shape: ADVICE: Everything before the marker is working, everything from it on is the advice. The split is exact, with no judgement calls. Verified against the live model, four different brews: Test Pale Ale marker@pos=0 finish=stop advice_len=656 Nocturna Christmas marker@pos=0 finish=stop advice_len=668 Ginger beer marker@pos=0 finish=stop advice_len=621 Cider raspberry marker@pos=0 finish=stop advice_len=539 All four start clean, with no marker left in the advice. A reply with no marker is used as it stands - the safe direction, where the brewer sees a slightly rambling reply rather than no advice at all. An empty advice is now recorded as a FAILURE rather than stored as a successful generation. Storing an empty string with advice_ok=True made the page render "Thinking..." for ever, because the template's `{% if reading.advice %}` was false - so the brewer never learned that it had failed. 181 tests (up from 184 - the eight heuristic-era stripper tests are replaced by six that test the marker behaviour). --- src/flask/brew_advice.py | 232 ++++++++------------------------------ tests/test_brew_advice.py | 180 +++++++++-------------------- 2 files changed, 99 insertions(+), 313 deletions(-) diff --git a/src/flask/brew_advice.py b/src/flask/brew_advice.py index d4fcc5b..31ce02b 100644 --- a/src/flask/brew_advice.py +++ b/src/flask/brew_advice.py @@ -78,6 +78,13 @@ temperature? - Should they rack, add more hops, raise the temperature, or wait? Rules: +- Your reply must have this exact shape: + + ADVICE: + + + The line "ADVICE:" comes FIRST, on its own line, with nothing before + it. Do not explain your reasoning and do not show your working. - Advise about the reading that is marked as the one to advise on. It may not be the most recent one - the brewer may be correcting an older reading - and the advice must be about the reading they asked about. @@ -213,9 +220,11 @@ def call_ollama(prompt: str, *, url: str | None = None, model: str | None = None truncated = finish_reason == "length" advice = _trim_advice(_strip_reasoning(content)) + if truncated: advice = _trim_advice(advice + " (the reply was cut off)") - return advice + + return advice, truncated def _trim_advice(text: str) -> str: @@ -239,199 +248,35 @@ def _trim_advice(text: str) -> str: return tail.strip() -#: The reasoning models sometimes put their working IN the content, even -#: with thinking turned off - "Let me look at this..." for several -#: thousand characters before the actual advice. The advice is the last -#: few paragraphs, so the preamble is cut off. -_PREAMBLE_MARKERS = ( - "let me look", - "let me think", - "let me work", - "okay, let me", - "first, let me", - "hmm,", - "wait,", - "let me analyze", - "let me consider", -) +#: The marker the model is asked to put at the start of its advice. +#: +#: Guessing where the advice begins by classifying prose is unreliable +#: in both directions - it emptied legitimate advice, and it let the +#: model's working through in full. A marker the model writes itself is +#: deterministic: everything before it is working, everything from it on +#: is the advice. +_ADVICE_MARKER = "ADVICE:" def _strip_reasoning(text: str) -> str: - """Cut a reasoning preamble off the front of a reply. + """Take the advice out of a reply. - The advice is the prose at the END - the working comes first, in - several paragraphs. So the preamble is removed by finding the last - paragraph break before the advice starts, not the first. + The model is asked to put ``ADVICE:`` on its own line at the start + of the advice, so the working before it can be dropped exactly, and + the advice kept exactly. - Only a preamble that looks like working is removed; a reply that - starts straight into the advice is left alone. + A reply with no marker is used as it stands. That is the safe + direction: the model was asked for the marker, so a reply without + one is one where it wrote the advice straight away - and if the + working did leak in, the brewer sees a slightly rambling reply + rather than no advice at all. """ - lowered = text.lower() + index = text.find(_ADVICE_MARKER) + if index == -1: + return text.strip() - # A reply that is ALL working - no advice paragraph at all - has to - # be caught too. The marker phrases are a hint, not the only signal: - # a reply can be pure working without ever saying "let me think". - # - # The working is DROPPED, not returned. Returning it unchanged would - # 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 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: - index = lowered.find(marker) - # index 0 is fine - a reply can START with "Let me look at this". - if index < 0: - continue - - tail = text[index:] - # The advice is the LAST paragraph of the tail. Split on the - # blank lines and take the paragraphs from the end, until the - # 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 - # model's working rather than the brewer's advice. - kept = [paragraphs[-1]] - for paragraph in reversed(paragraphs[:-1]): - if _looks_like_advice(paragraph): - kept.append(paragraph) - else: - break - kept.reverse() - - candidate = "\n\n".join(kept).strip() - if len(candidate) >= 80: - return candidate - return text - - -#: Words that mark a paragraph as the ADVICE rather than the model's -#: working. The advice talks about what the brewer should DO. -#: Phrases that mark a paragraph as the ADVICE rather than the model's -#: working. They are matched as PHRASES, not bare words - "keep" alone -#: matches "I need to keep in mind that the hydrometer was read at a -#: different temperature", which is working, not advice. -_ADVICE_MARKERS = ( - "you should", - "you could", - "you may want", - "you might", - "you will want", - "give it", - "let it sit", - "let it ferment", - "take another reading", - "take a reading", - "raise it to", - "raise the", - "lower the", - "rack it", - "you could repitch", - "i would suggest", - "i suggest", - "i recommend", - "in your case", - "here is", - "here's", - "don't", - "do not", -) - - -#: 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. - - The reliable signal is SHAPE, not length. A legitimate advice reply - can be a thousand characters, and a short working reply can be a - few hundred - so length alone gets both wrong. - - The model's working is about INTERPRETING the numbers: what the - reading might mean, what could be causing it, what it is unsure - about. The advice is about what to DO. - - So a reply is working when it is full of the model's hedging about - the data AND contains no instruction at all. - """ - if _has_advice_marker(text): - # It contains an instruction, so part of it is advice. - return False - - lowered = text.lower() - hedging = sum(lowered.count(word) for word in _HEDGING_WORDS) - - if hedging == 0: - # No instruction and no hedging about the data. This is advice, - # however long it is. - return False - - if len(text) <= _WORKING_LENGTH: - # Some hedging, but it is short - that is a brewer's note that - # happens to use a hedging word, not a page of working. - return False - - # Hedging, no instruction, and long: the working. - return True - - -#: The words the model uses when it is working through the data rather -#: than telling the brewer what to do. -_HEDGING_WORDS = ( - "perhaps", "maybe", "possibly", "could be", "might be", "might", - "i think", "i am not sure", "i'm not sure", "actually", "hmm", - "wait,", "let me", "i need to", "the question is", "unclear", - "i am unsure", "not sure", "unfortunately", -) - - -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. - - The model's working is full of hedging - "perhaps", "or maybe", "it - could be" - and is about interpreting the numbers. The advice is - about what to do. - """ - lowered = paragraph.lower() - if any(marker in lowered for marker in _ADVICE_MARKERS): - return True - # A paragraph that reads like working is not advice. The signs are - # hedging ("perhaps", "could be"), and the model talking to itself - # ("wait", "hmm", "so", "actually") rather than to the brewer. - working_words = ("perhaps", "maybe", "possibly", "could be", "might be", - "i think", "i am not sure", "actually", "hmm", "wait", - "let me", "so on brew day", "there is a discrepancy", - "that is a discrepancy", "which means", - "i need to keep in mind", "the question is", - "this could mean", "it could be", "or maybe") - if any(word in lowered for word in working_words): - return False - return True + advice = text[index + len(_ADVICE_MARKER):].strip() + return advice or "" def generate_advice(run: dict, reading: dict, history: list[dict], *, @@ -446,7 +291,18 @@ def generate_advice(run: dict, reading: dict, history: list[dict], *, """ prompt = build_prompt(run, reading, history, corrected=corrected) try: - return True, call_ollama(prompt, url=url, model=model, timeout=timeout) + advice, _truncated = call_ollama(prompt, url=url, model=model, + timeout=timeout) + if not advice: + # The reply had no advice in it - the model wrote only its + # working, or the marker was missing and nothing followed it. + # Storing an empty string with advice_ok=True would render + # as "Thinking..." for ever, so it is recorded as a failure + # with a note the brewer can see. + log.warning("the model returned no advice") + return False, ("Advice could not be generated - the model " + "returned only its reasoning.") + return True, advice except urllib.error.HTTPError as exc: # HTTPError is a subclass of URLError, so it has to be caught # FIRST or it would be swallowed by the unreachable branch. diff --git a/tests/test_brew_advice.py b/tests/test_brew_advice.py index d4bf319..2a8e52e 100644 --- a/tests/test_brew_advice.py +++ b/tests/test_brew_advice.py @@ -117,7 +117,7 @@ def test_a_successful_call_returns_the_text(monkeypatch): class Response: def read(self): return json.dumps({ - "choices": [{"message": {"content": " Leave it alone, it is still fermenting. "}}] + "choices": [{"message": {"content": "ADVICE: Leave it alone, it is still fermenting."}}] }).encode() def __enter__(self): @@ -226,38 +226,70 @@ def test_the_token_budget_leaves_room_for_the_reasoning(monkeypatch): assert captured["body"]["max_tokens"] >= 8000 -def test_a_reasoning_preamble_is_cut_off(): - """Some models put their working in the content even with thinking off. - The advice is the prose at the end; the working comes first, in - several paragraphs. The preamble is removed by finding the advice, - not by taking the text after the first blank line. + +def test_the_model_is_asked_for_an_advice_marker(): + """The marker is what makes the split deterministic. + + Guessing where the advice begins by classifying prose failed in both + directions - it emptied legitimate advice, and it let the model's + working through in full. A marker the model writes itself is exact. """ + assert "ADVICE:" in brew_advice.SYSTEM_PROMPT + + +def test_the_working_before_the_marker_is_dropped(): + """Everything before the marker is the model's working.""" reply = ( - "Let me look at this. The brewer has a Christmas beer, OG 1.042, " - "fermenting at 19C.\n\n" - "So on brew day, the reading is 1.038, but the OG is stated as 1.042. " - "That is a discrepancy of 4 points.\n\n" - "Wait, the reading on brew day is 1.038 but the recorded OG is 1.042.\n\n" - "On brew day a gravity of 1.038 against a recorded original of 1.042 " - "is a small discrepancy but not a problem. Take the OG from a " - "well-mixed sample, and start the fermentation at 18-20C." + "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, and I need to work out what that means for " + "the expected attenuation for this style at this temperature.\n\n" + "ADVICE:\n" + "Warm the fermenter to 20C and give the yeast a gentle swirl to " + "rouse it, then take another reading in three days." ) stripped = brew_advice._strip_reasoning(reply) - assert stripped.startswith("On brew day a gravity of 1.038") - assert "Let me look at this" not in stripped - assert "That is a discrepancy of 4 points" not in stripped + assert stripped.startswith("Warm the fermenter") + assert "Let me think" not in stripped + assert "1.055" not in stripped -def test_a_reply_that_starts_with_the_advice_is_untouched(): - """The stripper must not eat a good reply.""" - reply = ("The gravity looks fine for day nine. Give it three more days " - "before you do anything, and take another reading on Friday.") +def test_a_reply_with_the_marker_is_kept_whole(): + """A legitimate advice reply is never emptied. + + Whatever length it is, and whatever hedging words it happens to + contain, the advice after the marker is the advice. + """ + advice = ( + "Actually the drop is fine. Perhaps it will finish this week. Wait " + "a couple of days and check again, and if it has not moved, give " + "the yeast a gentle swirl and take another reading before you " + "consider repitching anything." + ) + reply = "ADVICE:\n" + advice + + assert brew_advice._strip_reasoning(reply) == advice + + +def test_a_reply_without_the_marker_is_used_as_it_stands(): + """The safe direction when the model does not use the marker. + + The brewer sees a slightly rambling reply rather than no advice at + all. + """ + reply = "The gravity looks fine for day nine. Give it three more days." + assert brew_advice._strip_reasoning(reply) == reply +def test_a_reply_with_only_working_after_the_marker_is_empty(): + """Nothing after the marker means there is no advice.""" + assert brew_advice._strip_reasoning("ADVICE:") == "" + + def test_an_over_long_reply_keeps_the_advice_at_the_end(): """The advice is at the END of the reply, so a plain truncation from the front would cut it off and leave only the model's working. @@ -287,7 +319,7 @@ def test_a_short_reply_is_not_trimmed(): def test_the_advice_is_truncated(): """A runaway reply must not fill the reading document.""" - long_text = "x" * 5000 + long_text = "ADVICE: " + ("x" * 5000) class Response: def read(self): @@ -640,112 +672,10 @@ 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. 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 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 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, 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) - - assert stripped == "", ( - f"the model's working was kept as advice: {stripped[:120]!r}" - ) -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_legitimate_advice_reply_is_kept(): - """Length alone is a blunt instrument. - - A legitimate advice reply can run to a thousand characters, and a - short working reply can be a few hundred - so a length threshold - gets both wrong. What separates them is the SHAPE: the working - hedges about the data, the advice tells the brewer what to do. - """ - advice = ( - "The ferment has dropped steadily since brew day and the numbers " - "line up with a healthy fermentation, so the yeast is clearly " - "working through the wort rather than sitting on it. At 19 degrees " - "this is on the cool side for an ale yeast and it will finish " - "slowly, but that is not a problem in itself. Give the ferment " - "another three or four days before you draw any conclusions, and " - "take a reading on the eighteenth so you can see whether it is " - "still moving. Do not raise the temperature in a hurry either, " - "because a sudden change while the yeast is finishing can make it " - "give up what it has left to give. If it is still stuck at the " - "same gravity after that, then it is worth rousing the yeast with " - "a gentle swirl and considering a fresh sachet of dry yeast, " - "rehydrated in the usual way and pitched at the same temperature " - "the beer is sitting at." - ) - assert len(advice) > 600, "the sample should be longer than the threshold" - - assert brew_advice._strip_reasoning(advice) == advice - - -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. - - "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): @@ -898,7 +828,7 @@ def test_a_reply_cut_off_at_the_token_ceiling_is_marked(repo): return json.dumps({ "choices": [{ "finish_reason": "length", - "message": {"content": "The gravity looks fine so far, but"}, + "message": {"content": "ADVICE: The gravity looks fine so far, but"}, }], }).encode()