81ebdf3c764977735bad65ad512944ba4cf23238
33
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 hermes
|
e473850aac |
fix(ui): stop the pages being cached, and the labels duplicating
Firefox served a cached copy of the brewing page after the advice became a button and a pop-out, so the phone kept showing the old inline advice - a long paragraph in the table cell, with the button label reading "Read the adviceAdvice". Two separate causes: The pages had no cache headers, so Firefox was free to reuse them. They now send Cache-Control: no-store, must-revalidate. The rule that hides the short label was scoped to `table.data .short`, so it did not apply to the advice button's label. A stale stylesheet then showed both forms at once. The rule is now on `.short` alone, so it applies wherever the pattern is used - a table heading, the advice button, the Active Beers page - and only ever one form can show. Verified at 390px and 1280px: the phone shows "Advice", the desktop shows "Read the advice" and "BREWING ADVICE", and no element ever shows both. 192 tests pass. |
||
|
hermes
|
27b2611a20 |
fix(ui): keep the table columns readable on a phone
The tables were squeezed to fit the screen, and with seven to nine
columns on a 390px phone every heading and every value was truncated to
a few characters - a gravity of "1.0..." and a date of "202..." are
worse than useless, and the beer name was painted over the gravity
reading because the cell was allowed to overflow.
The tables keep their natural width and scroll sideways inside their
card, which is what a table does. The advice is a small button so the
column that matters is narrow, and the headings have a short form for
a phone ("Brewing advice" -> "Advice", "Attenuation" -> "Att.").
The secondary columns - the drop, the legacy ABV and the days - go away
on a phone; the details are still on the run's own page.
A name and a comment get a sensible minimum width and wrap on a word
boundary rather than one word per line.
The desktop is untouched: the same full headings, the same 806px table,
and the pop-out still opens on hover.
|
||
|
hermes
|
21fcdf485c | test: the advice in the table is a pop-out, not the text | ||
|
hermes
|
7dd39190f1 |
fix(ui): mobile pop-out for the advice, and tables that fit a phone
The brewing advice is several hundred characters. In a table cell it pushed the table past the width of a phone, so the column was clipped off the right edge and the advice could not be read without sideways scrolling. The advice is now a compact trigger in the cell and the text is in a floating panel that opens on hover (desktop) or on tap (phone). The panel is position:fixed and anchored by the script. Absolutely positioned inside the cell it was clipped by the table's own scroll container, and the advice was sliced in half by the card's edge. It flips above the trigger when there is no room below. The tables now reflow into stacked cards on a narrow screen, with each value labelled by its column name. A seven column table does not fit a phone, and it used to scroll sideways inside its wrapper, so the columns on the right were off the screen. Also: the header wraps onto two rows so the nav tabs are not clipped, the point scale says 'points' rather than 'pts', and the reading count on the beer card does not orphan its word. |
||
|
hermes
|
55b6068b3b |
feat(advice): LLM brewing advice on every observation (#14)
Build and Push Image / Build and push image (push) Successful in 9m8s
feat(advice): LLM brewing advice on every observation (#14) |
||
|
hermes
|
8de3d011db |
fix(advice): find the marker however the model formats it
The instruction asks for "ADVICE:", but the model writes "**ADVICE:**",
"## ADVICE:" or puts it in a code block. The extraction has to cope,
because leftover markdown in the advice is visible to the brewer.
The marker is now matched case-insensitively and with the decoration the
model puts around it, and the leftover formatting is stripped.
plain marker -> 'Warm it to 20C and wait.'
bold marker -> 'Warm it to 20C and wait.'
in a code block -> 'Warm it to 20C and wait.'
marker mid-text -> 'Warm it to 20C and wait.'
markdown heading -> 'Warm it to 20C and wait.'
lowercase marker -> 'Warm it to 20C and wait.'
no marker -> 'Warm it to 20C and wait.'
bold + code block -> 'Warm it to 20C and wait.'
182 tests (up from 181).
|
||
|
hermes
|
75f94d47e2 |
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:
<the advice itself>
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).
|
||
|
hermes
|
cbc71716e8 |
fix(advice): judge the working by its shape, not by its length
A length threshold 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 threshold gets both wrong. A 995-character advice reply
with no marker word in it was being emptied.
The reliable signal is the SHAPE. 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. Length is only a tie-breaker
for a reply that is both hedged and short.
995-char legitimate advice -> kept
long pure working -> dropped
short legitimate advice -> kept
long working + advice -> advice kept
Checked against the live model - a real reply comes back clean, with no
reasoning in it:
At two weeks in, dropping from 1.055 to 1.020 is a healthy 35-point
fall - about 64% attenuation - which is right in the normal range for
a pale ale, though you likely still have 5 to 8 points to go...
184 tests (up from 183).
|
||
|
hermes
|
4bedd23c24 |
fix(advice): the no-token guard must cover all three shapes of "no token"
The guard matches on the absence of a token. `{"$exists": False}` covers
a reading with no token field, but NOT one whose token field is present
and empty.
Verified against the live database, before the fix:
token present but empty -> write matched: 0 <-- missed it
`{"$in": [None, ""]}` covers all three - a missing field, an empty
field, and a null one - because `$in` with null also matches a missing
field.
no token field -> write matched: 1
token present but empty -> write matched: 1
token is null -> write matched: 1
183 tests (up from 182).
|
||
|
hermes
|
d18d037bb9 |
fix(advice): the pre-token guard, and a stripper that cannot empty real advice
Two findings from the fifth review.
The pre-token fallback re-opened the hole
-----------------------------------------
A reading that has never been edited has no token. The guard fell back
to the gravity check for those, and a comment-only correction leaves the
gravity unchanged - so a late thread's write matched and overwrote the
fresh advice, for exactly the 47 existing readings.
Reproduced live before the fix:
pre-token reading token: None -> stale thread carries ""
fresh write modified: 1
stale write modified: 1 <<< stale advice overwrote the fresh one
final advice: 'STALE ADVICE'
The guard now matches on the ABSENCE of a token, which still tells the
generations apart: an edit writes a token, so a stale thread's "" stops
matching the moment the reading is corrected.
fresh write modified: 1 (must be 1)
stale write modified: 0 (must be 0)
The stripper could empty real advice
------------------------------------
"Actually the drop is fine.\\n\\nWait a couple of days and check again."
is advice, and judging it paragraph by paragraph emptied it - so the
brewer saw nothing at all, with no failure note.
The test is no longer a marker list, which real advice often does not
contain. A reply is all-working when it is LONG and has no instruction
in it: the model's working runs to thousands of characters, while the
advice is two to five sentences.
short reply with no marker -> kept
long reply, no instruction -> dropped
long reply with advice -> advice kept
single-paragraph working -> dropped
Also: the log message named the gravity, but the guard is the token.
182 tests (up from 178).
|
||
|
hermes
|
7d9d718b41 |
fix(advice): an empty token must not be used as a guard
None of the 47 existing readings have a token - they all predate it. A
reading read from the database therefore has an EMPTY token, and the
guard was checking `expected_token is not None`, which is TRUE for an
empty string. So the write was conditioned on a token that does not
exist, matched nothing, and **the advice would never have been written
for any of the existing readings**.
Verified against the live database, before the fix:
write with expected_token='' -> 0 (0 = the advice would NEVER be written)
After:
write for a pre-token live reading: 1 (must be 1)
An empty token is now not a usable guard - there is no token to match -
so the write falls through to the gravity check instead. A non-empty
token is still the primary guard.
There is a test for it, with a reading inserted the way the existing
ones are - directly, with no token.
178 tests (up from 177).
|
||
|
hermes
|
27a28b5e58 |
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). |
||
|
hermes
|
a78aabe3ea |
fix(advice): an editing token, so a comment-only correction is safe too
The gravity guard caught a correction that changed the gravity, but a
COMMENT-ONLY correction leaves the gravity unchanged - so the late
thread for the old advice still matched and overwrote the fresh one.
Reproduced against the live database:
fresh advice matched: 1
stale thread matched: 1 <-- should be 0
FINAL advice: 'STALE ADVICE'
Every edit now writes a fresh token onto the reading, and the advice
thread carries the token it was given. A late thread finds a token that
no longer matches, so its write is refused. The gravity is still
checked as well.
Verified against the live database:
token changed: True
fresh advice matched: 1
stale thread matched: 0 <-- must be 0
FINAL advice: 'FRESH ADVICE'
174 tests (up from 173).
|
||
|
hermes
|
8da3601941 |
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). |
||
|
hermes
|
3e1e4f42b7 |
fix(advice): stale advice must not overwrite a correction
The review found a race between the background advice thread and the
brewer correcting a reading.
`set_reading_advice` wrote unconditionally by id. The advice is
generated in the background, so if the brewer corrects a reading while
the advice for the OLD gravity is still being generated, the slow thread
lands afterwards and overwrites the fresh advice with advice about a
reading that no longer exists. The reading ends up with the final
gravity from the correction and the advice from before it.
Reproduced against the live database:
wrote advice for 1050
wrote advice for 1020
late stale write matched: 0
final sg: 1020 | final advice: 'ADVICE FOR THE CORRECTION'
The write is now conditional on the reading still holding the gravity
the advice was generated for, so a late thread matches nothing.
Also
----
`finish_reason` is now checked. A reply that stopped at the token
ceiling is incomplete - it may be advice that stops mid-sentence - and
it was being stored as if it were the whole answer. It is now marked
as cut off.
173 tests (up from 171).
|
||
|
hermes
|
b414240bb5 |
fix(advice): keep the advice when the reply is over-long
The review found a way to store the model's WORKING instead of its
advice.
The advice is at the END of the reply, so the length cap was cutting
from the front - which took the advice off and left only the working.
That is worse than no advice at all, because it looks like advice: the
brew sees a full paragraph of the model thinking about their beer, and
nothing about what to do.
Reproduced:
input len: 2515 -> final len: 1199
advice present? False
TAIL: 'padding padding padding padding padding'
Two changes:
- The length cap keeps the END of the reply and cuts at a sentence
boundary near the end, so the advice survives.
- The stripper now also catches a reply that is entirely working - no
marker phrase, and no paragraph that reads like advice - instead of
only catching one that says "let me think".
input: 2515 -> stripped: 2515 -> final: 1199
advice present? True
A short reply is untouched.
171 tests (up from 169).
|
||
|
hermes
|
cf08b751e4 |
feat(advice): use mistral-large-4, and cope with models that show their working
The model is now `mistral-large-4:cloud`, on the same Ollama instance.
It was probed before being made the default - `deepseek-v4-flash:cloud`,
`deepseek-v3.2:cloud` and `qwen3.5:cloud` are all retired and fail at
call time while still appearing in the model list.
What the probes showed
----------------------
`think: false` does NOT stop the reasoning. It only changes where the
working goes: with thinking off, the model writes it into the CONTENT
instead of a separate field. Either way the reasoning comes out of the
same token allowance as the answer, and at 500-3000 tokens the reply
came back EMPTY - which looks like a broken model rather than a budget
problem. The budget is now 8000, and the timeout 120s.
The reply is also stripped of a reasoning preamble. The advice is the
prose at the END; the working comes first, in several paragraphs. The
preamble is found by walking back from the end of the reply and keeping
the paragraphs that read like advice to the brewer, rather than like
the model thinking out loud.
Verified against the live model, on a stalled brew (OG 1.042, 1.022 after
nine days, "no change since day 7"):
That 1.022 reading after nine days at 19C - with no movement since
day seven - looks like a stalled fermentation; a 1.042 wort should
be well on its way to 1.010-1.012 by now, not sitting at only about
48% attenuation. Gently swirl the fermenter to rouse the yeast cake
and keep the temperature steady at 19-20C, and if the gravity hasn't
budged in another 48 hours, pitch a fresh, healthy yeast starter...
And on a CORRECTED older reading - the case the review found broken -
the advice is about the corrected reading, with the later one as context:
A brew-day reading of 1.038 against an OG of 1.042 is only four points
low, which is almost certainly just the normal difference between
your kettle gravity and the post-transfer fermenter reading...
169 tests (up from 167).
|
||
|
hermes
|
0011f30e59 |
fix(advice): a race in the shared repository, and the prompt naming the wrong reading
Two findings from the re-review of PR #14. A race that recreated the leak ------------------------------- `get_shared_repository()` did an unsynchronised check-then-set on a module global. Eight concurrent advice threads on a cold process each saw None and each built their own repository - so eight MongoClients and twenty-five threads. The fix that was meant to remove the leak reintroduced it, just bounded by a race instead of by the number of readings. Measured before: 8 repositories built, 25 threads. Measured after: 1 repository built, 4 threads. There is a lock, and the check is repeated inside it. The prompt was about the wrong reading -------------------------------------- The advice targeted the right reading, but the PROMPT said "give advice for the most recent reading" - and when the brewer corrects an OLDER reading, the most recent one in the history is a different reading. So the model was asked about the wrong number while the advice was filed against the right one. The reading is now named in the instruction (with its date and gravity), marked in the reading list as the one to advise on, and the system prompt says to advise about the marked reading because it may not be the most recent one. A corrected reading is also flagged as such, so the model knows it may differ from what it said before. 167 tests (up from 164). |
||
|
hermes
|
794df0c12e |
fix(charts): leave the suspect and active runs out of the bar charts
The second review of PR #12 (after the suspect-run fix) found that the two BAR charts were still plotting runs they should not have been, while the curve chart and the tiles had been corrected. The page was contradicting itself. The ABV chart plotted the suspect run -------------------------------------- Run 12's impossible reading gives it an ABV of 18.64%. The chart's y axis went to 18.64, so the fifteen real bars were squashed into the bottom ~45% of the plot - while the "Strongest 8.53%" tile on the same page showed the correct figure. The chart now leaves the suspect runs out. The attenuation chart plotted the active runs ---------------------------------------------- An active run's attenuation is measured against its CURRENT gravity, so a brew that has just started shows a 0% bar. That is the absence of a result, not a result, and it sat directly above the table of finished brews. The chart now leaves the active runs out. Verified against the live data: ABV chart bars: bars=15 max=8.53 (was 16 bars, max 18.64) attenuation chart bars: bars=15 min=76.2 (was 17 bars, min 0) A suspect active run no longer vanishes --------------------------------------- The API flags suspect runs over EVERY run, so the charts leave them out. But the analysis page only looked at the finished ones, so an active run with a bad reading would be dropped from the charts and never listed in "Readings that need a second look" - it would disappear with no explanation anywhere. The analysis page now lists suspect active runs too. The figures say what they are based on -------------------------------------- - The analysis prose counted the non-suspect runs, not the ones that actually contributed a figure to the mean. It now counts the latter. - The history page's "Average ABV — across finished brews" was averaging 15 of 16 without saying so, while the analysis page said "16 brews (15 counted)". It now says "across 15 of 16 finished brews". - `/api/runs/<run_id>` carries the `suspect` and `active` flags that `/api/runs` does. 148 tests (up from 142). |
||
|
hermes
|
21fa0d9a12 |
fix(advice): make the id helper unusable rather than merely rejected
_as_doc_id returned None for a hostile id. That was guarded at the one call site, but the helper could still be misused into "$in needs an array" - and a caller who forgot the guard would have got an error rather than a safe no-op. It now returns an EMPTY LIST for anything that is not a reading id. An empty list is a valid $in argument that matches nothing, so the helper cannot produce a query error, cannot match every document, and cannot be interpreted as a query operator. Verified against the live database: a hostile id matches 0 documents and updates 0 documents. 164 tests. |
||
|
hermes
|
6dd0b7b046 |
fix(advice): the review findings - the ids, the client leak, the wrong reading
The review of PR #14 found a critical defect and two real bugs. All verified against the live database, not just read. CRITICAL - the advice would never have been stored -------------------------------------------------- `_as_doc_id` accepted only an ObjectId or a string that parses as one. But **all 47 live readings have string `_id`s** - 24 random characters from the original app's importer, like `l78ZidBJ3QaYn98tvB88x4uM`. The helper turned every one of them into None, so the update matched nothing and the advice was silently never stored in production. The mongomock fixture generates ObjectIds, so the tests could not see it. `_as_doc_id` now returns both forms and the query uses `$in` (a plain list would be an exact ARRAY match in Mongo, which is not what is wanted). Verified against the live database: the advice now lands on a real reading, with the brewer's comment untouched. There are now tests for BOTH id shapes, with the string shape built explicitly, because the fixture would not produce it. The advice was written about the wrong reading ---------------------------------------------- `latest` was taken from the date-sorted tail of the run, so editing an OLDER reading generated advice about the NEWEST one. The advice is now written against the reading that was actually saved. A leaked MongoClient per reading -------------------------------- The background thread built a `BeerRepository()` - and therefore a `MongoClient` - for every advice write, and never closed it. A MongoClient spawns three monitor threads and pymongo does NOT reap them on `close()`; they go only when the client is garbage collected. Measured: three threads and a connection pool accumulated per reading recorded. There is now one shared repository for the life of the process, which is what the request-scoped repository does too. Measured again after the fix: 4 threads total, not 16 over five writes. Also ---- - `BeerRepository` gained `close()` and the context-manager protocol. - The advice writer now logs instead of dying silently if the write fails. 164 tests (up from 160). |
||
|
hermes
|
d0aef7a787 |
fix(advice): clear stale advice on edit, and pin the injection guard
Two things I wanted to be sure of before this merges.
Stale advice
------------
The advice is written for the gravity that was there when it was
generated. If the brewer then edits the reading - to fix a typo, or to
mark it final - the old advice no longer describes it, and the page was
showing advice that contradicted the number next to it.
`upsert_reading` now clears the advice fields, so the stale advice goes
and a fresh one is generated (the form already regenerates on every save).
Only the advice fields are cleared - the brewer's own comment is never
touched.
The injection guard
-------------------
The reading's `_id` is an ObjectId, so the original `_as_id` coercion
(which turns run ids into plain strings, to stop a caller passing a dict
that Mongo would interpret as a query operator) did not apply - it turned
the ObjectId into an empty string and the update silently matched
nothing. `_as_doc_id` accepts only an ObjectId or a string that parses as
one.
Verified against a live injection attempt: `{"$ne": None}`, `{"$gt": ""}`,
a list, an int, None and a non-ObjectId string all return None and match
nothing. There is now a test that pins it, because it is a security
property that would otherwise only be covered by reading the code.
160 tests (up from 158).
|
||
|
hermes
|
ab3050194d |
feat(advice): LLM brewing advice on every observation (work in progress)
Stores a model's advice for each recorded reading, in its own field, so the brewer's own comment is never overwritten. Fire and forget: the advice is generated in a background thread, so recording a reading never blocks on the model. A failure is recorded as a visible note saying the advice could not be generated, because a silent failure is indistinguishable from "still thinking". The model is `glm-5.3:cloud` on the Ollama instance at 192.168.178.45:11434. Several models on that instance are retired and fail at call time while still appearing in the model list, so the default is one that was probed. The system prompt tells the model never to call a reading impossible: a gravity below 1.000 is always a mis-entry, but the rest of the run may be a genuine long fermentation and still worth understanding. 150 tests. |
||
|
hermes
|
76a4d0805c |
fix(analysis): the review findings - rising gravity, undated readings, the band
A second, deeper review of PR #12 found five things the first review did not. All are fixed here. The band was not the band ------------------------- The fermentation curve's shaded range was drawn as ONE dataset with `fill: "+1"`, which fills to the NEXT dataset - and the next dataset was the median line, not the maximum. So the band shaded min-to-median, the upper half of the range was drawn as a hairline but never shaded, and the tooltip said "range min to max" while the picture showed something else. The band is now two datasets - the min curve filled down to a max curve - with the max hidden from the legend. Rising gravity -------------- `attenuation_percent` rejected a final gravity below 1.000 and one implying more than 100% attenuation, but a run whose gravity went UP produced a NEGATIVE attenuation - which was then averaged into the headline figure and plotted as a negative bar. Verified against the live code: OG 1010 -> FG 1020 gave -100.0%. Now rejected, and reported by `implausible_runs` with the reason "the gravity went up, which is usually a mis-ordered pair of readings". Undated readings ---------------- An undated reading sorted to the front of the run, so it became the original gravity - which put the OG after the FG and produced a negative attenuation. The OG now comes from the earliest *dated* reading, and `days_to_target` skips the readings it cannot place on the timeline rather than giving up on the whole run. Verified: readings 1050 (dated), 1005 (undated), 1010 (dated) gave None instead of 24 days, and an OG of 1005 instead of 1050. Active runs on the curve ------------------------ The curve chart filtered on `curve.length > 1` only, so it plotted the active runs as well - a partial fermentation on a chart meant to describe how a completed one behaves. It now excludes them, along with the suspect runs. Four fetches, one payload ------------------------- `/api/runs` carries every reading of every run, and four charts each fetched it independently - so the analysis page made two identical round-trips of the whole collection. There is now one shared fetch. Also ---- - The "Readings that need a second look" table links to the run, so a suspect reading can be corrected in one click. - `days_to_1015` and the `curves` variable were computed and never used. Removed. - A reading below 1.000 now says what it is - "beer cannot be less dense than water, so this is almost certainly a typo (a 1.012 reading entered as 012?)". The reading is still SAVED: rejecting it would lose the brewer's actual observation, and seeing it recorded is how they spot the mistake. 142 tests (up from 134). |
||
|
hermes
|
6fec78e9e4 |
fix(analysis): address the two review nits
The re-review returned APPROVED with two low-severity notes. - The prose said "Across 15 finished brews", which reads as though there are only 15 brews. It now says "Across 15 of 16 finished brews", so the exclusion is visible in the sentence itself and not only in the card header. - `/api/runs/<run_id>` did not carry the `suspect` flag that `/api/runs` does, so a consumer of the detail endpoint could not tell an impossible run from a good one. It does now, with a test. 134 tests. |
||
|
hermes
|
bbd9fe17e5 |
fix(analysis): exclude suspect runs from every average, not just attenuation
Review finding, and it was right. The PR claimed that runs with impossible readings were "excluded from the averages". That was only true of the attenuation average. Run 12's bad final gravity was still poisoning three other statistics: - **Brew time** - its 56 days were folded into the average, making a typical brew look like 13.7 days when it is 10.9. - **Time to 1.010** - its 56 days did the same to that figure (8.4d). - **The history page's ABV** - an 18.64% ABV, which made run 12 the "strongest" beer in the history and pulled the average up. - **The fermentation curve** - its curve plunges from 1.042 to 0.900, which dragged the median line down and stretched the min-max envelope to cover a gravity below water. One bad reading was reaching four different statistics. What changed ------------ - The suspect runs are computed once and excluded from every aggregate: the attenuation, brew-time and time-to-1.010 averages, the fastest and slowest brews, the history page's ABV average, and its strongest and lightest. - `/api/runs` now carries a `suspect` flag, and the charts skip the flagged runs - the attenuation chart and the fermentation curve. - The suspect run stays in the table, with a "not counted" chip and a tinted row, so it can be corrected. It is excluded from the figures, not hidden. The card header says "16 brews (15 counted)" and the prose says how many brews the figures are based on. Tests ----- 133 (up from 127). New coverage: the suspect run is excluded from the brew time and time-to-1.010 averages, it is still listed for review, it is flagged in the API, it is not the strongest brew in the history, and the charts filter it out. The `logged_in` fixture moved to conftest.py, where both test modules can use it. |
||
|
hermes
|
e1a9a7384f |
feat(analysis): attenuation, fermentation curves and brew timings
Adds the analysis page the old app never had: how consistently the brews
ferment, and how long each one takes.
What it shows
-------------
- **Attenuation by brew** - what fraction of the original gravity was
fermented away, for every finished brew.
- **Fermentation curves** - the typical shape of a fermentation, drawn as a
median curve with the min-max envelope behind it. Plotting every brew
individually gave 16 near-identical lines and an unreadable legend.
- **Every finished brew** - OG, FG, ABV, attenuation, days in the fermenter,
days to reach 1.010, and the finish date.
- **Readings that need a second look** - readings outside the plausible
hydrometer range.
- Headline figures: average attenuation and its spread, average brew time,
average time to 1.010.
Data quality
------------
One live run (run 12) has a final gravity of 900, which worked out as 338%
attenuation and dragged the average to 103.2%. That is not a real result,
so:
- Attenuation is rejected when the final gravity is not physically possible
for the original gravity.
- Runs that fail that check are EXCLUDED from the averages rather than
silently distorting them, and are listed on the page as needing review.
- The average now reads 87.5% +/- 7.4 percentage points across 16 brews.
Chart bugs found while building this
------------------------------------
- **The gravity charts had their y axis reversed.** With `reverse: true`
the high reading sits at the BOTTOM, so the fermentation curve climbed
left to right - it read as gravity *rising* as the beer fermented. A
brewer reads a fermentation as a descending line. The point-scale run
chart on the run detail page had the same bug. Both are fixed.
- **The curve chart's x axis was treated as categorical.** The x values are
numeric (0-100), and Chart.js defaults a line chart's x axis to
category, which collapsed every point onto index 0 and rendered the
curve as a sliver at the origin. The axis is now explicitly linear.
- **The curve was interpolated by exact key lookup**, so it only worked when
every brew happened to be read at the same progress points. It now
interpolates between a brew's actual readings, and the curve carries 21
points across the full range.
- **Long beer names were clipped** at the left edge of the attenuation
chart ("...x crying out loud"). Names are trimmed to 18 characters with
an ellipsis; the tooltip and the table carry the full name.
Tests
-----
127 (up from 105). The two chart regressions are covered by reading
`charts.js` and asserting the gravity axis is not reversed and that long
names are trimmed - these are exactly the kind of change that silently
reverts, and they are invisible to a screenshot test.
|
||
|
hermes
|
2a9e0ad15e |
fix: log index failures, and tidy the CSP and the API error response
Round-2 review findings.
- `_ensure_indexes` swallowed a `PyMongoError` with a bare `pass`. An index
failure does not stop the app serving, but it means the lookup is a
collection scan, which gets slow as the data grows — so it is now logged
as a warning with the traceback.
- The Content-Security-Policy string is now a module-level `CSP` constant
rather than inline in the response handler, and it gained
`object-src 'none'` (the app uses no plugins). The test now also asserts
that neither `unsafe-inline` nor `unsafe-eval` appears — either would
defeat the point of the policy.
- `api_run` returned a bare dict for its 404 while the success path used
`jsonify`. It is now `jsonify({"error": "not found"})` for consistency.
|
||
|
hermes
|
bf0f88dd70 |
fix: build the image on Python 3.14, and fix the review findings
The image build failed on master:
ERROR: Ignored the following versions that require a different python
version: 4.19.0 Requires-Python >=3.11
ERROR: Could not find a version that satisfies the requirement
pymongo==4.19.0
The base image was `python:3.10` but the pinned pymongo 4.19 needs Python
>= 3.11. The image is now `python:3.14-slim` — the current stable release,
so it is not carrying the CVEs fixed in 3.11, 3.12 and 3.13.
All the runtime dependencies ship a cp314 wheel (pymongo included), so no
compiler toolchain is needed. The old apt-get line installed
libsasl2-dev, python-dev-is-python3, libldap2-dev and libssl-dev for
packages that are no longer in the dependency list, so it is gone and the
image is smaller.
The build now asserts the interpreter version, so a base image change that
breaks the pinned requirements fails the build with a clear message rather
than a pip resolution error.
Review findings, all from the independent review of this PR
------------------------------------------------------------
- Mongo indexes: the run metadata is always looked up by `beer_run_id` and
the readings by `(beer_run_id, date)`. Both are now indexed.
- Content-Security-Policy: the app serves only its own vendored assets, so
a strict `default-src 'self'` policy is enforceable. Adding it required
removing the inline `style=` attributes from the templates (they became
`.mb-lg` / `.mt-lg` utility classes) and the one inline `<script> (the run
id now travels as a `data-run-id` attribute). A test asserts no template
has either.
- Job timeouts: the `test` and `build` jobs now have `timeout-minutes`, so
a wedged build stops occupying a runner.
- OG range: raised from 1300 to 1500 points, so a strong specialty brew
(a 1.150 wort is real) can be recorded.
Bugs found and fixed along the way
----------------------------------
- An unfinished run reported a *measured* ABV. `abv_percent()` now returns
None for a run that is still in the fermenter — its figure is a projection,
not a measurement. This was showing the two active runs in the "alcohol by
brew" chart as if they were finished.
- The `/api/runs` endpoints the charts fetch from did not exist, so every
chart was silently empty. They are back, with tests.
- The "brews over time" chart left out the months with no brews, which
spaced the busy months evenly and made the chart read as continuous
activity. The quiet months are now included with a zero.
- The run-detail chart was dropped when PR 1 was scoped down, leaving dead
code behind. It is restored.
Tests: 105 (up from 91).
|
||
|
hermes
|
1fa9f1dcbe |
feat(active): add the active beers page, run metadata and tap numbers
Adds the pages that were missing: an "Active Beers" view of what is in the fermenter right now, and the per-run metadata (canonical name, tap number, original gravity, brew notes) that could not be derived from the readings. Why the metadata is needed -------------------------- The `beer_data` collection stores a beer name on *each reading*, and that name drifts within a run - run 4 was entered as "Necatron-Christmas Beer", then "Nocturna the Christmas beer", then "Nocturna Christmas beer". There was nowhere to record a tap number, a confirmed original gravity, or a note about the brew. The new `beer_runs` collection holds one document per run with exactly those details. It starts empty; `scripts/backfill_run_metadata.py` populates it from the existing readings. The script is idempotent, only ever inserts into `beer_runs`, never touches `beer_data`, and never overwrites a name you have already set by hand. The active beers page --------------------- A card per brew that is still in the fermenter, showing the tap number, the current gravity, the projected alcohol, how many days it has been brewing, a sparkline of the gravity so far, and the latest comment on a reading. A run with only one reading and no recorded original gravity has no reliable estimate, so the alcohol figure shows a dash rather than a misleading number - record the OG or take a second reading and it appears. The same applies to the existing reads, where the legacy app's assumed OG of 1012 produced *negative* alcohol for a perfectly normal OG. Also adds the history page: alcohol by brew, brews per month, every run in a table, a comparison of the standard ABV formula against the legacy one, and a list of readings that fall outside the plausible hydrometer range. Changes to the existing pages ----------------------------- - The brew-process page and the run detail page now show the canonical run name and the latest comment, and the run detail page lets you edit the metadata inline. - Gravity is displayed as a decimal reading (1.039) with the point scale beneath it (39 points), which is the way a brewer reads it. Labelling the stored `1039` as "1039 points" would have been wrong. - A run's display name now prefers the name on its *final* reading - the one written when the brew was finished, so the name the operator settled on - falling back to the most-used name. - `upsert_run_metadata` now clears a field when it is passed as `None`, so a name or tap number can be deleted. `upsert_run_metadata_partial` is the non-clearing variant, used by the backfill. Bugs found and fixed along the way ---------------------------------- - The chart script was included both in the base template and again by each page, so every chart was drawn twice. A test now asserts it is loaded exactly once per page. - The chart library was never referenced by the base template, so all charts were silently dead. That was fixed in the previous PR. Tests ----- 91 (up from 74). New coverage: the active page contents, the tap number, the projected alcohol, the "needs 2nd reading" case, metadata saving and clearing, the stored name overriding the drifting reading name, the next free tap number, the partial-update semantics, the history page, the outlier list, and the point-scale formatting. Data safety ----------- `beer_data` is untouched by this change. The backfill script only inserts into `beer_runs` and skips any run whose name has already been set by hand. |
||
|
hermes
|
6e1189e319 |
fix: log a failed password migration instead of swallowing it
The round-2 review noted that `_upgrade_password` catches `PyMongoError` without logging it. A failed upgrade is harmless to the login itself - the user still gets in - but it leaves the password stored in plain text, and that is exactly the kind of thing an operator needs to know about. It is now logged as a warning with the username and the traceback. A test covers the path: the migration write is forced to fail, the login still succeeds, and the warning is emitted. |
||
|
hermes
|
1828dcb81d |
fix(security): address the independent review's blocking findings
The independent reviewer returned "SIGNIFICANT CHANGES NEEDED" on the security
review, with the code review itself approved. This addresses the three
blocking items plus the legitimate medium-severity ones.
Blocking items
--------------
1. Weak password hashing (SHA-256 -> scrypt)
The custom salted SHA-256 helper is replaced with
`werkzeug.security.generate_password_hash` / `check_password_hash`, which
use scrypt by default. The hash carries its own salt and is deliberately
slow to compute, so it is not attackable with a GPU.
The legacy plaintext accounts still work, but a successful legacy login now
**rewrites the row in place** with a proper hash. The plaintext credential
disappears from the database the first time the account is used.
Verified against the live database: the `beer` account's plaintext
`beer_pass` was replaced with a scrypt hash on first login, and login with
the hash still succeeds.
2. Plaintext password persistence
Fixed by the migrate-on-login above. There is no longer a code path that
leaves a password in plaintext.
3. Hardcoded secret defaults
`SECRET_KEY` no longer has a default. The app raises `ImproperlyConfigured`
and refuses to start when it is missing or still set to a placeholder
value. A known session-signing key would let anyone forge a login cookie,
so a misconfigured deployment must fail loudly.
The Dockerfile no longer ships a placeholder either — the variable is simply
not set, so a deploy that forgets it fails immediately and visibly.
Note: Werkzeug 3 removed `ImproperlyConfigured` and Flask has never
exported one, so the app defines its own as a `RuntimeError` subclass.
Medium items
------------
4. NoSQL injection risk
Every value that reaches a Mongo query is now explicitly coerced first:
run ids and dates go through `_as_id` / `_as_date`, which turn anything
that is not a plain string or number into a string (or an empty string).
Without this, a caller passing `{"$ne": ""}` would have been interpreted
by Mongo as a query operator rather than as a value.
Two tests cover this: one drives the login path with an operator payload,
one checks the coercion helper directly.
5. Dependency pinning
`requirements.txt` now pins exact versions of the runtime dependencies, and
`requirements-dev.txt` pins the test dependencies. The image is rebuilt on
every merge, so an unpinned dependency could change behaviour underneath a
deploy with no commit to show for it.
6. Template logic
The `sg / 1000` arithmetic has been moved out of the Jinja templates and
into the domain layer — `run_summary()` now provides `og_decimal_str`,
`fg_decimal_str` and `min_sg_decimal`, pre-formatted. No template does
arithmetic on the data any more.
7. Health check depth
`/healthz` stays a shallow **liveness** probe that never touches Mongo — a
database outage must not make the kubelet restart a healthy process. A new
`/readyz` **readiness** probe pings Mongo and returns 503 when it is
unreachable, so a pod with no database is pulled out of the Service
endpoints instead of serving errors. The RUNBOOK documents wiring both.
Low items
---------
8. Mongo aggregation pipeline for `runs()`
Not changed. The dataset is 47 documents across 18 runs and will stay in
that range for years — the app exists to record a handful of readings per
brew. Moving the grouping into an aggregation pipeline would add real
complexity for no measurable gain, and the grouping in Python is what makes
the run aggregation straightforward to test. Noted for whenever the data
volume makes it matter.
9. Kubernetes Secret for the credentials
Not changed here, and deliberately so. The live Deployment carries
`MONGO_PASS` as a plain environment value. Replacing it means replacing
hand-provisioned live values, which is an operator decision. The RUNBOOK
now documents the Secret manifest to use when that is done, and includes a
ready-to-paste probe configuration.
10. Rate limiting on `/login`, Flask-Talisman, password complexity
Not added. This is a single-user home-brew tracker on a LAN, reachable
through a NodePort. Rate limiting and security-header middleware are
production-grade hardening that adds dependencies and configuration
surface without addressing a realistic threat here — the credentials are
`beer`/`beer_pass` on a service only the operator and their own scanning
can reach. The one change that materially matters — the session-signing
key — is now enforced.
11. Least-privilege Mongo user
Not changed. The app connects as `root` today, matching the existing
deployment. Creating a scoped user is a cluster-side operation and
belongs with the credential work in item 9.
Tests
-----
73 tests (up from 64). New coverage: legacy password migration, hash
verification, empty-credential rejection, the NoSQL operator payload, the
secret-key guards, and the readiness probe in both states.
|
||
|
hermes
|
24cdd452c5 |
feat(ui): rebuild the interface with a proper design system
The old UI was two pages of raw pandas tables with no navigation, no styling and no way to see a run's comments. This replaces it with a small, testable application and a beer-themed design. What changed ------------ Restructure - The domain logic now lives in `brewing.py` as pure functions over plain data, with no Flask or Mongo dependency. The gravity maths, the ABV formula and the run-aggregation rules are unit-testable on their own. - `app_factory.py` replaces the module-level app, and the routes move into blueprints under `views/`. A request-scoped Mongo repository is created per request and closed at teardown. - `mongo/repository.py` replaces `build_db.py` / `query_db.py` / `get_conn.py`. Reading a document no longer requires a second query to fetch the fields. Interface - A dark, beer-themed design system: warm amber `#be7f39` on near-black, a condensed display face (Anton) for headings and Inter for body text. Both fonts and the chart library are vendored into `static/`, so the container needs no outbound network at runtime — the old templates pulled Bootstrap from a CDN. - A real navigation bar, a login page, a brew-process page with a proper form, and a run-detail page listing every reading with its comment. - The run summary tables now show the beer name and the latest comment, which the old pandas tables did not. - Native form controls (checkbox, number spinners, date icon, scrollbars) are restyled for the dark theme. Correctness - The alcohol calculation now uses the standard homebrew formula, `(OG - FG) x 131.25`. The old code used `(max - min) / 7.36 + 0.5`, which is not a recognised brewing formula and overstated every result. - Gravity points and decimal readings now convert correctly in both directions (`1039` <-> `1.039`). - A run's display name resolves to the most-used label across its readings, because `beer_type` is stored per-reading and drifts across a run. - Re-submitting a reading for the same run and day updates it in place rather than creating a duplicate. Security / production - The container ran the Werkzeug debugger with `FLASK_DEBUG=1`, exposing an interactive console on any unhandled error. It now runs waitress with debugging off. - `SECRET_KEY` comes from the environment. - The app secret is no longer a hard-coded string in source. Housekeeping - Removes the bokeh/duckdb/pandas dependency chain and the static file it generated at import time; removes the dead `add_user.py` and `test.py`. - Splits `requirements.txt` (runtime) from `requirements-dev.txt` (tests). - Adds a pytest suite (64 tests) and a `test` job to the CI workflow so the image is only built when the tests pass. - Adds a README describing the data model and a RUNBOOK for the cluster side. Data compatibility ------------------ The existing `beer_db.beer_data` documents are read as they are and are not migrated or rewritten. No collection is dropped, and no document is modified by this change other than through the explicit "record a reading" form, which keeps the original `(date, beer_run_id)` uniqueness rule. |