Give every BUGS.md entry an explicit fix and regression test

The first pass left 11 of the 24 entries with their remedy buried in prose and
only 7 with a named test, while the header claimed a regression test for each.
Every entry now follows one shape: symptom, root cause, *Proposed fix:*,
*Test:* — with the handful that genuinely cannot be unit-tested (Docker image
layout, browser behaviour) marked manual instead of left implied.

A few fixes gained detail while being written out: B-14 needs the empty-token
short-circuit kept ahead of compare_digest, B-15 breaks the current tests'
short JWT secret, B-19 must not update tip_header_hex from a losing header,
and B-20's column ends up holding the previous txid rather than the next one,
so its name is backwards.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

BUGS.md

@@ -6,8 +6,11 @@ deployment. The test suite was green at the time of the audit (79 passed), so **
 the findings below are caught by the existing tests** — every one of them needs a
 regression test alongside its fix.
 
-Findings are ordered by severity. Each entry is self-contained: symptom, root cause with
+Findings are ordered by severity. Each entry is self-contained and follows the same shape:
-file references, how to reproduce, and the proposed fix. Items already listed under
+symptom, root cause with `file:line` references, a `*Proposed fix:*` block, and a `*Test:*`
+block naming the regression test to add (a few are marked manual where an automated test
+would be testing the Docker image layout or the browser, not the code). Items already listed
+under
 "Known gaps / TODO" in `CLAUDE.md` are cross-referenced rather than repeated, except
 where this audit found the gap to be worse than documented.
 
@@ -300,6 +303,10 @@ the node's message in `params`. Add `error.broadcast_failed` to all 7 languages
 `app/static/i18n.js` (per the i18n contract in `CLAUDE.md`). Answer 502/503 rather than
 400, since the failure is not the client's fault.
 
+*Test:* `place_bet` / `request_withdrawal` with a client stub whose `broadcast` raises must
+raise `BetError`/`WithdrawalError` with code `broadcast_failed` — and must leave no
+`spent_txid`, no participant and no pending row behind.
+
 ---
 
 ### B-08
@@ -323,6 +330,10 @@ before broadcast but only committed after.
 to `broadcast` in a second commit. A crash between the two leaves a row the
 reconciliation task from [B-04](#b-04) can resolve against the chain in either direction.
 
+*Test:* with a client stub that broadcasts successfully but a session whose second commit
+raises, the `PendingTransaction` must still exist afterwards (in its pre-broadcast state)
+rather than the bet vanishing entirely.
+
 ---
 
 ### B-09
@@ -373,6 +384,10 @@ endpoints.
 `_CONFIG_FIELDS` (keep it in the response model) so the pause switch has exactly one
 audited path.
 
+*Test:* `PUT /admin/config` changing `fee_address` must produce exactly one
+`config_updated` audit row carrying both the old and the new value; `paused` must be
+rejected (or ignored) on that endpoint.
+
 ---
 
 ### B-11
@@ -409,6 +424,11 @@ actually receives", which is the opposite of the behaviour.
 subtract an estimated payout fee or relabel the field as an estimate in the UI. Keep
 `participant_count` as-is for display but consider exposing `confirmed_count` separately.
 
+*Test:* with two confirmed participants whose stored `bet_amount_sats` is below the
+configured `bet_amount_sats` (fee already deducted), `GET /rounds/current` must report a
+`jackpot_sats` derived from the stored amounts, and must not change when
+`RoundConfig.bet_amount_sats` is edited mid-round.
+
 ---
 
 ### B-12
@@ -433,6 +453,10 @@ registration) is therefore retried five times and finally reported as
 constraints in the HTML form. Inspect the `IntegrityError` and re-raise `username_taken`
 when it is the username constraint that failed.
 
+*Test:* `POST /auth/register` with an empty username and with a 3-character password must
+both be rejected (422/400) and create no user; a registration racing an existing username
+must answer `username_taken`, not `derivation_index_conflict`.
+
 ---
 
 ## Medium
@@ -445,8 +469,14 @@ when it is the username constraint that failed.
 
 `verify_password` catches only `VerifyMismatchError`. Argon2 raises `InvalidHashError` for
 a hash it cannot parse and `VerificationError` for other verification failures, both of
-which escape as an unhandled 500. Catch `argon2.exceptions.VerificationError` (the
+which escape as an unhandled 500.
-superclass of `VerifyMismatchError`) plus `InvalidHashError` and return `False`.
+
+*Proposed fix:* catch `argon2.exceptions.VerificationError` (the superclass of
+`VerifyMismatchError`) plus `InvalidHashError` and return `False`, so an unusable stored
+hash reads as "wrong password" rather than as a server fault.
+
+*Test:* `verify_password("x", "not-a-hash")` returns `False`; `POST /auth/login` against a
+user row with a corrupted `password_hash` answers 401.
 
 ### B-14
 
@@ -455,8 +485,14 @@ superclass of `VerifyMismatchError`) plus `InvalidHashError` and return `False`.
 *File:* `app/api/routes/admin.py:20-22`
 
 `x_admin_token != settings.admin_token` is a short-circuiting comparison. This token gates
-the private-key export endpoint, so it deserves `secrets.compare_digest`. Keep the
+the private-key export endpoint, so it deserves a constant-time comparison.
-existing "empty token means always deny" behaviour.
+
+*Proposed fix:* `secrets.compare_digest(x_admin_token, settings.admin_token)`, keeping the
+existing "empty configured token means always deny" short-circuit *before* it (compare_digest
+on two empty strings returns `True`).
+
+*Test:* the existing admin auth tests still pass, plus one asserting an empty
+`ADMIN_TOKEN` setting denies an empty `X-Admin-Token` header.
 
 ### B-15
 
@@ -467,9 +503,16 @@ existing "empty token means always deny" behaviour.
 `jwt_secret` and `xprv_encryption_key` both default to `""`. With an empty `JWT_SECRET`,
 PyJWT raises `InvalidKeyError: HMAC key must not be empty` on every login and
 registration — a 500 with no hint about the real cause (verified locally). With an empty
-`XPRV_ENCRYPTION_KEY`, Fernet fails on the first key derivation instead. Both should fail
+`XPRV_ENCRYPTION_KEY`, Fernet fails on the first key derivation instead. Either way the
-fast: a Pydantic validator that rejects empty values (and, for `jwt_secret`, a minimum
+container starts up healthy and only fails once a user touches the broken path.
-length) so the container refuses to start rather than half-working.
+
+*Proposed fix:* a Pydantic `field_validator` (or `model_validator`) on `Settings` rejecting
+empty `jwt_secret`/`xprv_encryption_key`, with a minimum length on `jwt_secret` (32 bytes,
+per the `InsecureKeyLengthWarning` PyJWT already emits in the test suite). The process must
+refuse to boot instead of half-working. Note the tests currently rely on a short secret, so
+they need updating alongside.
+
+*Test:* constructing `Settings(jwt_secret="")` raises `ValidationError`.
 
 ### B-16
 
@@ -480,8 +523,16 @@ length) so the container refuses to start rather than half-working.
 The image copies `pyproject.toml`, `app/`, `migrations/`, `alembic.ini` and `scripts/`.
 `docs/` is never copied (it is not in `.dockerignore` — it is simply not `COPY`-ed), so
 `FileResponse("docs/guida-utente.md")` raises inside the container and the help link in
-the navbar returns a 500 in every real deployment. Add `COPY docs ./docs` to the
+the navbar returns a 500 in every real deployment.
-Dockerfile (and treat the guide as a shipped asset), or serve the guide from `app/static/`.
+
+*Proposed fix:* add `COPY docs ./docs` to the `Dockerfile` (treating the guide as a shipped
+asset), or move the guide under `app/static/` and serve it from there. Whichever is chosen,
+`GET /guida` should degrade to a 404 with a clear message rather than an unhandled
+exception when the file is absent.
+
+*Test:* not unit-testable as-is (it depends on the image layout) — verify with
+`docker compose up -d --build && curl -k https://localhost/guida`, and add that check to
+`docs/running-the-server.md`'s smoke list.
 
 ### B-17
 
@@ -501,6 +552,10 @@ distinct error code — it is a user mistake, not a system limit), and/or identi
 change output by index rather than by address, recording it on `PendingTransaction` at
 build time.
 
+*Test:* `request_withdrawal` to `user.address` is rejected; and given a hand-built tx with
+two outputs to the same address, the bump reduces the *change* one (the last), leaving the
+recipient amount untouched.
+
 ### B-18
 
 **DB session held open across Electrum network calls during payout**
@@ -510,8 +565,16 @@ build time.
 `_trigger_payout` opens a session and, inside it, awaits `client.listunspent(...)` and
 `client.broadcast(...)` before committing. On SQLite that holds the write lock for the
 duration of two network round-trips (unbounded, per [B-01](#b-01)); on Postgres it becomes
-a long-running transaction. Restructure as: read config/winner and close the session →
+a long-running transaction.
-do the network work → reopen a session to persist the result.
+
+*Proposed fix:* restructure into three phases — read config/winner and close the session →
+do the network work (`listunspent`, build, `broadcast`) → reopen a session to persist the
+result and the `PendingTransaction`. This also makes the [B-05](#b-05) error handling
+easier to place, since the failure-prone part is no longer inside a transaction.
+
+*Test:* the existing payout tests must still pass; add one asserting no session is open
+while the stub client's `broadcast` is being awaited (e.g. by having the stub attempt a
+write through a second session).
 
 ### B-19
 
@@ -521,9 +584,16 @@ do the network work → reopen a session to persist the result.
 
 `self.tip_height = header["height"]` accepts a lower height on a reorg. Since
 `_wait_for_next_block` compares `self._listener.tip_height <= tip_at_close`, a regression
-silently extends the wait by a block. Reorg handling is out of scope for v1 by explicit
+silently extends the wait by a block.
-design decision, but the assignment should at least be `max(...)`, or log a warning when
+
-the height goes backwards.
+*Proposed fix:* full reorg handling is out of scope for v1 by explicit design decision, but
+the assignment should be guarded: keep `max(self.tip_height, header["height"])` and log a
+warning when a header arrives with a lower height, so the condition is at least visible in
+the logs when it happens. Do not update `tip_header_hex` from a header that loses this
+comparison, or height and hash would describe different blocks.
+
+*Test:* feeding the consumer a header at height N then N-1 leaves `tip_height == N` and the
+hash unchanged.
 
 ## Low / hygiene
 
@@ -536,8 +606,15 @@ the height goes backwards.
 
 `bump_fee` overwrites `current_txid` in place and never records what the old txid was. The
 column exists, is selected, and is rendered in the admin "Transazioni pendenti" table,
-where it therefore always shows `—`. Set it in `bump_fee` (part of the [B-02](#b-02) fix)
+where it therefore always shows `—`.
-so the replacement chain is auditable.
+
+*Proposed fix:* set it in `bump_fee` (naturally part of the [B-02](#b-02) fix) so the
+replacement chain is auditable. Note the column semantics are the reverse of what the name
+suggests for an in-place update — it will hold the *previous* txid, so either rename it
+(`previous_txid`, via a migration) or document the direction on the model.
+
+*Test:* after a bump, the row's `replaced_by_txid` holds the pre-bump txid and
+`current_txid` the new one.
 
 ### B-21
 
@@ -548,9 +625,14 @@ so the replacement chain is auditable.
 `pending_ids` (line 32) is computed and never used. Line 35 rebuilds the same tuples from
 ORM objects whose session has already been closed; it works today only because
 `expire_on_commit=False` and no commit intervened, so flipping that engine setting would
-break it silently. Select the plain columns in the query
+break it silently.
-(`select(PendingTransaction.id, ..., PendingTransaction.kind)`) instead of hydrating
+
-entities.
+*Proposed fix:* drop `pending_ids` and select the plain columns
+(`select(PendingTransaction.id, PendingTransaction.current_txid, PendingTransaction.kind)`)
+instead of hydrating entities, so nothing outlives the session.
+
+*Test:* covered by the existing `tests/unit/test_confirmation.py` — it must stay green with
+`expire_on_commit=True` forced on the test session factory.
 
 ### B-22
 
@@ -560,9 +642,15 @@ entities.
 `app/static/admin.js` (table cells)
 
 Every amount is displayed as `sats / SATS_PER_PLM` with no formatting, so values like
-`0.7000000000000001` are reachable in the balance, the jackpot and the admin tables. Add a
+`0.7000000000000001` are reachable in the balance, the jackpot and the admin tables.
-single `formatPlm(sats)` helper (fixed decimals, locale-aware grouping) and route every
+
-display through it.
+*Proposed fix:* one `formatPlm(sats)` helper (fixed decimals, locale-aware grouping via
+`Intl.NumberFormat` with the language already resolved by `i18n.js`), used by every display
+site. Amounts sent *to* the server must keep going through `Math.round(x * SATS_PER_PLM)` —
+the formatter is for display only.
+
+*Test:* manual — with a 0.7 PLM jackpot and a 12345678.9 PLM balance, no artefacts and no
+locale mismatch against the selected language.
 
 ### B-23
 
@@ -574,8 +662,16 @@ display through it.
 invoked from the SSE handler, the poll chain, `placeBet`, `withdraw` and `showDashboard`,
 all sharing `#refresh-btn`. Two overlapping calls make the second snapshot the *loading*
 label, which it then restores permanently — leaving the button stuck on
-"Aggiornamento…". Guard with a per-button in-flight flag (bail out, or await the existing
+"Aggiornamento…".
-promise).
+
+*Proposed fix:* keep the in-flight promise on the element itself (e.g. a
+`button._loadingPromise` / `WeakMap`); a second call either awaits the existing one or
+returns immediately, so only the outermost call restores the markup. Also worth
+de-duplicating the SSE burst: `onRoundServerEvent` fires three fetches per event, and the
+broadcaster is generic, so every client reacts to every event.
+
+*Test:* manual — trigger a bet while an SSE-driven `refreshMe()` is in flight and confirm
+the refresh button returns to its icon+label state.
 
 ### B-24
 
@@ -586,8 +682,14 @@ promise).
 It answers `{"detail": "internal server error"}` — a bare string, while
 `app/api/errors.py` documents `detail` as `{"code", "message", "params"}`. The frontend
 tolerates a string (`apiErrorMessage` handles that case), but the contract should be
-uniform: return `ApiError("internal_error", "internal server error").as_detail()` and add
+uniform.
-`error.internal_error` to the i18n table.
+
+*Proposed fix:* return `ApiError("internal_error", "internal server error").as_detail()` and
+add `error.internal_error` to all 7 languages in `app/static/i18n.js`. Keep the response
+body free of exception details — the traceback belongs in `logs/app.log` only.
+
+*Test:* an endpoint stubbed to raise answers 500 with `detail.code == "internal_error"` and
+no exception text in the body.
 
 ---