From 8da3601941e50ba1a8c748a5f47c25f6d06f9e45 Mon Sep 17 00:00:00 2001 From: hermes Date: Mon, 9 Nov 2026 17:47:14 +1000 Subject: [PATCH] fix(advice): the remaining review findings Four things from the earlier review of this PR. The prompt test did not test the prompt --------------------------------------- `test_the_advice_is_about_the_reading_that_was_saved` asserted which dict was PASSED to the generator - not what the model was actually ASKED. So it passed even while the prompt still said "give advice for the most recent reading", which is the bug that made the model advise about the wrong number while the advice was filed against the right one. It now asserts on the prompt content: the reading is marked, its date is named, and "most recent reading" is gone. Verified by reverting the prompt fix - the test fails, and so does `test_the_prompt_names_the_reading_to_advise_about`. Two tests now guard it, and reverting the fix breaks both. A write that matches nothing was silent -------------------------------------- The advice writer discarded the return value of `set_reading_advice`. A write that matched 0 documents - which is now the EXPECTED outcome when the brewer corrected the reading while the advice was being generated - was dropped without a word. It is logged now. A stale docstring ----------------- The background thread "builds its own" client - it has used the shared one since the leak was fixed. A missing CSS class ------------------- `advice--reading` is used on the active-beers card and the run detail page, and was never defined, so it had no styles. 173 tests (unchanged - the new assertions were added to existing tests). --- src/flask/static/css/app.css | 6 ++++++ src/flask/views/brews.py | 16 ++++++++++++++-- tests/test_brew_advice.py | 14 ++++++++++++++ 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/src/flask/static/css/app.css b/src/flask/static/css/app.css index f99a72f..fb54866 100644 --- a/src/flask/static/css/app.css +++ b/src/flask/static/css/app.css @@ -716,6 +716,12 @@ table.data .comment { color: var(--text-muted); font-size: .84rem; } color: var(--amber-text); } +/* The advice on the active-beers card and the run detail page - the + full text, not the clamped table cell. */ +.advice--reading { + margin-top: .5rem; +} + /* Advice that could not be generated - it says why. */ .advice--failed { border-left-color: var(--danger); diff --git a/src/flask/views/brews.py b/src/flask/views/brews.py index 9c92aa9..5c6045b 100644 --- a/src/flask/views/brews.py +++ b/src/flask/views/brews.py @@ -60,7 +60,8 @@ def _start_advice(run_id: str, reading_id: str, corrected: bool = False) -> None comment is never touched. The background thread cannot use the request-scoped repository - that - client is closed when the request ends - so it builds its own. + client is closed when the request ends - so it uses the shared + process-wide one. """ if not reading_id: return @@ -130,7 +131,18 @@ def _start_advice(run_id: str, reading_id: str, corrected: bool = False) -> None try: shared = get_shared_repository() - shared.set_reading_advice(read_id, text, ok, expected_sg=advice_sg) + written = shared.set_reading_advice(read_id, text, ok, + expected_sg=advice_sg) + if not written: + # The reading was corrected while the advice was being + # generated, so the advice no longer matches it. That is + # the guard working - the fresh advice for the corrected + # 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", + read_id, + ) except Exception: # noqa: BLE001 - never let the thread die noisily log.exception("could not store the generated advice") diff --git a/tests/test_brew_advice.py b/tests/test_brew_advice.py index 4222718..f5db183 100644 --- a/tests/test_brew_advice.py +++ b/tests/test_brew_advice.py @@ -498,6 +498,20 @@ def test_the_advice_is_about_the_reading_that_was_saved(): "the advice was generated about the wrong reading" ) + # And the PROMPT must ask about that reading - not just pass it + # through. Asserting only which dict was passed would not have + # caught the prompt still saying "give advice for the most recent + # reading", which is the bug that made the model advise about the + # wrong number. + prompt = brew_advice.build_prompt( + {"canonical_name": "Ale"}, + captured["reading"], + captured["history"], + ) + assert "THIS IS THE READING TO ADVISE ON" in prompt + assert "2026-01-01" in prompt + assert "most recent reading" not in prompt + def test_the_shared_repository_is_built_only_once(): """Concurrent advice threads must not each build their own client.