One pass over the LOW-tier review items plus two folded issues:
- Delete validate_journal_entry (dead since the Fava migration; it
validated the pre-string-amount model) with its exports, unused
crud imports, and tests. Beancount validates entries now.
- Migration m006: UNIQUE index on user_roles(user_id, role_id) after
deduping; assign_user_role inserts with ON CONFLICT DO NOTHING and
returns the existing assignment — closes the auto-assign
check-then-act race on concurrent logins.
- Extract _get_username_from_user_id (110 lines in views_api, fresh
LNbits Database per call inside per-row hot paths) into
user_lookup.py with one shared core-DB handle, a 60s TTL cache and
a batch get_usernames API (review #18).
- Receivable-entry responses report CLEARED, matching the flag the
formatter actually writes; PENDING misled the UI (libra-#35).
- Replace the remaining print() calls in tasks.py with logger.
- get_all_accounts derives valid roots from
account_utils.ACCOUNT_TYPE_ROOTS instead of a hardcoded tuple, and
the no-op per-test rate-limit reset is gone (libra-#54).
- Delete migrations_old.py.bak, MIGRATION_SQUASH_SUMMARY.md,
docs/PHASE*_COMPLETE.md and the rendered .html; .gitignore data/
(it holds the runtime .lnbits_auth_key secret).
- Track docs/CODE-REVIEW-2026-06.md with finding statuses updated for
the PR #55-#59 + chore/hygiene series.
- CLAUDE.md notes LNbits pins Pydantic v1: keep .dict(), don't
"modernize" to .model_dump().
Note: format_payment_entry's is_payable docstring (flagged in review
follow-up) turned out to be consistent with the body — no change.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Auth + input-validation cluster (CODE-REVIEW-2026-06 #5, #6, #16, #17
+ libra-#36, libra-#51, libra-#52):
- can_access_user_data compares full user ids only. The 8-char prefix
comparison was a 32-bit space: any prefix collision (or a crafted
short target id) let one user read another's data.
- can_access_account matches the User-{short} SEGMENT exactly; the
substring test also matched accounts merely containing it
(Expenses:Misc-User-deadbeef).
- Manual-payment approve/reject are status-guarded
(UPDATE ... WHERE status='pending' + rowcount): concurrent admins
can't double-book. The approve endpoint claims the request BEFORE
writing the ledger entry and reverts the claim if the write fails,
so at most one journal entry can exist per request.
- Account-name validation centralized into
account_utils.validate_account_name (libra-#51) — called from
crud.create_account (the choke point for every creation path,
virtual parents allowed a bare root), the admin add-account
endpoint, and fava_client.add_account at the writer boundary
(libra-#52).
- crud.create_account translates backend unique-violations into
AccountExistsError instead of leaking sqlalchemy internals
(libra-#36); POST /accounts returns 409 on duplicates and 400 on
malformed names. get_or_create_user_account catches the domain
error instead of string-matching the SQLite message (which never
matched on Postgres).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fava-client hardening cluster (CODE-REVIEW-2026-06 #7, #14, #15, #19
+ libra-#23, libra-#53):
- New FavaClient.transform_source_line does the whole
read-checksum-modify-write under the global write lock and maps
Fava 409/412 to ChecksumConflictError. The approve and reject
endpoints used to do this dance with raw httpx and no lock — two
concurrent mutations raced each other and every other ledger
writer (libra-#23). They now route through the new method and
translate conflicts to HTTP 409.
- update_entry_source / delete_entry raise ChecksumConflictError on
409/412 instead of leaking raw HTTPStatusError.
- One shared httpx.AsyncClient per FavaClient (12 per-call
instantiations removed — no more TCP handshake per request);
closed via libra_stop. Health probes keep their 2s timeout
per-request.
- Account names/patterns are validated against ^[A-Za-z0-9:_-]+$
before interpolation into BQL string literals.
- The posting amount regexes are consolidated into module-level
compiled patterns, all decimal-tolerant — the old integer-only
SATS pattern silently dropped decimal-SATS postings (Fava's @@->@
normalisation emits them) from balances.
- add-account no longer verifies its own write with a second
serialized get_all_accounts round-trip (libra-#53):
sync_single_account_from_beancount grows an assume_exists path.
New test: concurrent approve+reject must both land (was
lost-update/412 before the lock).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
format_balance returned a Beancount source string, but fava.add_entry
feeds PUT /add_entries whose deserialiser expects
{"t": "Balance", "amount": {"number", "currency"}, ...} — every
assertion create 500'd. Returns the dict shape now.
The seven strict-xfail reconciliation tests tracking this flip to
regular passing tests.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Settlement correctness cluster from CODE-REVIEW-2026-06 (#2, #8, #13)
plus libra-#38:
- format_net_settlement_entry now enforces the same inline balance
constraint as the fiat formatter (payment = receivable - payable
+ credit) and grows an optional credit leg. An unbalanced
settlement raises instead of reaching the ledger.
- on_invoice_paid settles only what the payment covers: a partial
payment clears that much receivable; excess (or a payment with
nothing owed) becomes user credit. Previously the full prior
balance was cleared against a smaller payment, shipping unbalanced
postings. Settlement links are attached only when the payment
clears the full open balance, and only for same-currency entries.
- get_unsettled_entries_bql returns each entry's real posting
currency (was hardcoded "EUR") and exact Decimal amount strings
(was float). /receivables/settle nets only entries denominated in
the settlement currency.
- fiat_rate/btc_rate metadata computed via Decimal (new
fiat_rate_metadata helper) instead of float division — cost-basis
records no longer carry float drift.
- format_posting_at_average_cost omits the cost braces when
cost_currency is unset ("SATS {}" is invalid Beancount).
- Underpay error payload serializes amounts as exact Decimal strings.
- validate_metadata catches decimal.InvalidOperation so bad
fiat_amount input becomes ValidationError (libra-#38); flipped the
tracking xfail.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two fixes to POST /api/v1/record-payment:
- The Fava duplicate check caught every exception and proceeded to
write, so a transient Fava blip produced double entries. It now
fails closed: transport errors return 503 and the client retries.
While here: the check queried {base_url}/api/journal, but base_url
already ends in /api — the doubled path 404'd, meaning the
duplicate check has silently never run.
- The endpoint now goes through the same processed_payments claim
gate as the background invoice listener, so the webhook+poller pair
can't both record the same payment_hash: a 'done' claim replays as
"already recorded", an in-flight claim returns 409.
Addresses CODE-REVIEW-2026-06 finding #10.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Fava-side duplicate checks (add_entry_idempotent, journal-link
scan) are read-then-write races: on restart with a persisted invoice
queue, or webhook + poller firing together, both callers pass the
"not present" check and both insert.
New processed_payments table (m005) keyed on payment_hash; exactly
one claimant wins the INSERT ... ON CONFLICT DO NOTHING. Lifecycle:
'processing' while the write is in flight, 'done' after; failed
recordings release the claim so redelivery retries, and 'processing'
rows from a crashed process are cleared at listener startup.
Also wraps the invoice-listener loop body in try/except so one poison
payment can't kill payment recording for the process lifetime.
Addresses CODE-REVIEW-2026-06 findings #4 and #9.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The migration version bump lands in the core dbversions table while
the DDL lands in ext_libra — the two writes are not atomic. A failed
bump re-runs the whole migration on next boot; bare
CREATE/ALTER/INSERT then crashes the extension until manual
dbversions surgery.
- CREATE TABLE / CREATE INDEX -> IF NOT EXISTS
- ALTER TABLE ADD COLUMN -> _alter_add_column_safe (same swallow
pattern as the events/withdraw fork migrations)
- seed INSERTs (default accounts, virtual parents, default roles) ->
ON CONFLICT (name) DO NOTHING
Tests run the chain twice against a fresh SQLite DB (full-chain rerun
and per-migration rerun); both fail against the previous migrations.
Addresses CODE-REVIEW-2026-06 findings #3 and #12.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
_open_directive_exists hardcoded '^YYYY-MM-DD open ' (dash-only, 2-digit,
single-space), but Beancount's DATE token (parser/lexer.l) is
(17|18|19|20)[0-9]{2}[-/][0-9]+[-/][0-9]+ and inter-token whitespace is any
[ \t\r] run. So a validly-formatted existing Open written as '2024/3/5 open X'
or '2020-01-01 open X' escaped detection → duplicate Open appended →
bean-check rejects the file. Anchor on Beancount's actual date pattern and
[ \t]+ separators. Adds parametrized coverage for slash/single-digit/multi-
space/tab variants.
Found in a coherence pass over the Beancount source.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When add_account reported the Open already existed, the endpoint raised
409 before the DB-mirror step — so an account present in the ledger but
missing from libra's DB (a prior sync failure with no cross-DB atomicity,
or an out-of-band open) was stranded: invisible to permissions with no
recovery path. Now 409 only when the account is already in the DB too;
otherwise sync it and return success. Adds a recovery test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The existence check matched 'open <name>' anywhere in the chart source,
so a prior account's description metadata or a comment mentioning the
name produced a false 409, while a real directive with an inline comment
and no space ('open X;legacy') was missed → a duplicate Open was appended
and bean-check then rejected the file, breaking every later /api/source
write. Extract the check into a pure _open_directive_exists() anchored to
'^YYYY-MM-DD open <name>' with an account-boundary negative-lookahead, and
unit-test both failure directions plus prefix/child non-matches.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The test harness was never updated to the post-server-deploy#4 split ledger
layout, so libra's per-user account opens (routed to accounts/users.beancount
by fava_client._infer_target_file) 500'd as a 'non-source file' and fell back
to DB-only — breaking the balance test and contributing to settlement errors.
Make the harness ledger a faithful split (root includes accounts/chart.beancount
+ accounts/users.beancount; title stays in root so the slug still matches).
Also raise lnbits_rate_limit_no for the session: the full suite fires >200
req/min and the default limiter 429'd fixture setup intermittently (10-11
errors). The limiter is built once at app creation, so setting it in the
session settings fixture (before the app fixture) disables it suite-wide.
Net: full suite goes from 1 failed / ~10 errors to fully green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The suite targets the lnbits dev worktree (needs lnbits.core.signers)
and trips on three non-obvious environment requirements, each of which
cost a failed run today: LNBITS_EXTENSIONS_PATH is the parent of an
extensions/ dir, the data folder must be a fresh temp dir per run, and
lnbits dev mandates LNBITS_KEY_MASTER at boot.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Approving a pending entry created with a reference (e.g. invoice
"42-144") 404'd with "Pending entry unknown not found": the list
endpoints recovered the entry id by parsing links for a libra- prefix,
but reference-bearing entries displace that link with the fused
"{reference}-{entry_id}" form, so the id surfaced as the literal
"unknown" and the approve call round-tripped it.
Make the entry-id transaction metadata the single canonical identity:
- _extract_entry_id() resolves metadata-first (libra- link parsing kept
only for pre-dfdcc44 ledger history); used by /entries/user,
/entries/pending, approve, and reject.
- Creation endpoints no longer fuse the reference with the entry id —
the user reference becomes its own sanitized link and round-trips
verbatim in API responses. Typed exp-/rcv-/inc- links stay as the
settlement-tracking handles.
- format_revenue_entry now writes entry-id metadata like its siblings
and sanitizes its reference link (was appended raw); generic
POST /entries sanitizes its reference link too.
- User-journal reference extraction skips all system link prefixes
(typed links used to leak into the reference field).
Contract documented in CLAUDE.md (Data Integrity → Entry Identity &
Links), pinned by tests/test_entry_identity_api.py and formatter
contract tests in test_unit.py.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When the caller omits settled_entry_links (the default), the endpoint
auto-detects open entries across both directions for the user and writes
a single transaction that:
- Zeros every per-user account that has an open balance, not just the
net (the libra-#33 bug — previously the 2-leg form left both Payable
and Receivable carrying non-zero balances after a complete cash
settlement, while only netting the cash side).
- Routes any cash above the net obligation to Liabilities:Credit:User-X
(libra-#41), so over-payment lands on a real liability account
instead of silently drifting.
- Attaches every reconciled source entry's link
(exp-..., rcv-...) so a reader scanning the settlement transaction
can trace what it cleared.
Cash less than the net obligation, with no explicit links, returns 400
with a structured diff (cash_paid, net_obligation, receivable_total,
payable_total). The operator either pays the exact net or passes
settled_entry_links to settle a specific subset; partial settlement
without a coherent target is not silently absorbed.
The legacy explicit-links code path is unchanged — callers that pass
settled_entry_links keep the 2-leg shape with no auto-detection. None
of the callers in libra or aiolabs/webapp currently use that field, but
the contract is preserved for the partial-settle-of-specific-entries
flow.
format_fiat_net_settlement_entry is the new helper for the 2/3/4-leg
shape; it enforces the cash-balance constraint inline so callers can't
accidentally produce an unbalanced transaction.
tests/test_settlement_api.py (6 tests) locks in:
- Nancy's #33 scenario: receivable 100 + payable 50 + cash 50
zeros both per-user accounts, links both source entries
- Overpay: cash 70 against net 50 → credit balance 20
- Pure receivable overpay → credit appears
- Underpay without explicit links → 400 with diff
- No open receivables → 400 with hint pointing at /payables/pay
- Explicit settled_entry_links uses legacy 2-leg path
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
113 passing tests + 3 skipped + 8 xfailed across 10 files, covering user
expense and income flow, admin receivable/revenue, settings + auth gates,
void/reject, manual payment requests, balance display, Lightning auth
paths, reconciliation API, and pure-function units. Runs against a real
Fava subprocess and full LNbits app via asgi_lifespan; the harness
captures the auth-flow / settings / env-var disciplines surfaced during
build-out (see tests/README.md and tests/conftest.py docstring).
Eight xfailed/skipped tests carry full implementations gated behind issues
#38, #39, #40 — they flip back on automatically when those land.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
