mirror of
https://github.com/we-promise/sure.git
synced 2026-09-05 14:51:15 +00:00
3d6a8d8b6e7c5cfeafbdffbde5b8b6b26eeb3e2b
1121
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
3d6a8d8b6e |
feat(budgets): move money between envelopes in one gesture (#3164)
* feat(budgets): carry a category's unspent budget into the next month
A budget category resets to zero every month, so anything non-monthly
(annual insurance, a holiday fund, car servicing) has no place to
accumulate. Two columns on budget_categories turn a category into a real
envelope: `rollover_enabled`, opt-in per category and off by default, and
`rolled_over_amount`, the surplus carried in from the previous month.
rolled_over(n) = rollover_enabled
? max(0, budgeted(n-1) + rolled_over(n-1) - actual(n-1))
: 0
v1 floors at zero: only a surplus carries, never an overspend.
The amount is materialized, not derived. March depends on February which
depends on January, so computing it on read would walk the whole chain on
every budget render. Budget::RolloverCalculator recomputes it in a single
forward pass and writes once via upsert_all, from Budget.find_or_bootstrap
and from BudgetCategoriesController#update -- allocations and the toggle
being the only inputs. No Transaction hook: a past month's actuals can
change after the fact, and the page load is a fine moment to catch up.
Scope kept deliberately narrow. `Budget#budgeted_spending`,
`#allocated_spending` and `#available_to_allocate` are untouched -- the top
of the budget page still answers "I planned to spend X, I've allocated Y".
The carry is per-envelope information, surfaced as `Budget#total_rolled_over`
and never folded into those totals.
What the carry does change is consumption: `available_to_spend`,
`percent_of_budget_spent` and `budgeted?` all count it, or a category funded
entirely by rollover would read as unbudgeted and get an alert pill while it
still had money left. `display_budgeted_spending` stays the month's
allocation alone -- the card shows the two figures side by side.
Details worth knowing:
- A parent's carry is net of its ring-fenced subcategories'. A parent's
allocation already contains theirs and its actuals already contain their
spending; those subcategories carry their own surplus, so counting the
parent's raw leftover would roll the same money over twice.
- Chains never mix: household with household, a member's personal budgets
with their own. A missing month is a gap the carry crosses, not a month
budgeted at zero.
- The carry stops at a currency change. sync_budget_categories stamps
categories with family.currency at sync time while a budget freezes its
own at creation, so the guard is on budget_category.currency -- the unit
the amount is actually denominated in.
- upsert_all writes with `update_only`, so a concurrent request that moves
an allocation between our read and our write doesn't get it clobbered by
the stale value we loaded.
- copy_from! copies the toggle, never the amount.
Cost for families that never turn it on: one EXISTS query per budget page
load, measured, including on the reports page which also bootstraps a
budget. With rollover on, the walk starts at the first month that uses it
rather than at the two-year history bound.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CyD26wsXjsYfpTgAGL1n1Z
* fix(budgets): pin the household rollover chain to a viewer-independent scope
Addresses review feedback on #3143.
The household budget (user_id NULL) has no owner to scope actuals by, and
`IncomeStatement` falls back to `Current.user` when nobody says otherwise.
The calculator therefore computed one shared `rolled_over_amount` through
whichever member happened to load the page, and each viewer overwrote the
other's number -- last one wins, and a member could infer spending in
accounts they cannot see. `Budget#income_statement_accounts` can now be
overridden, and the calculator pins the household chain to the whole
family so the shared row holds one number. Personal chains are untouched:
they already scope to their owner's accounts and were always deterministic.
`copy_from!` runs after `find_or_bootstrap` has already recomputed the
chain, so copying `rollover_enabled` left the target sitting on a zero carry
until the next page load. It now recomputes before its transaction commits.
The toggle tooltip described the wrong direction. `incoming_carry` checks
the flag of the month being computed, so the toggle governs what that month
*receives* from the previous one, not what it sends forward. Reworded in
English and French.
The concurrency regression test now drives its concurrent write through
`Budget#budget_category_actual_spending`, a public seam, instead of stubbing
a private method of the calculator from another class's test suite.
Each guard was confirmed load-bearing by reverting it and watching its test
fail. bin/rails test: 6939 runs, 0 failures.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CyD26wsXjsYfpTgAGL1n1Z
* fix(budgets): let the rollover choice stand instead of resetting each month
`rollover_enabled` lives on budget_categories, one row per (budget,
category), so a month created by `find_or_bootstrap` was born with the flag
off. Switching rollover on for Vacations in January and simply opening
February dropped January's surplus on the floor -- the user had to re-arm
the toggle every month, or go through "copy from previous budget". The
feature's headline case, a category funded 50/month accumulating over a
year, did not work as shipped.
New rows now inherit the flag from the last initialized budget of the same
owner, the same chain the carry itself walks. Turning the toggle off on a
given month still overrides it from there on, so the per-month escape hatch
survives.
The flag stays on budget_categories rather than moving to Category, which is
where comparable products (Monarch, Copilot, Lunch Money) put it. Categories
here are family-wide while budgets are per owner, so a category-level flag
would force one member's rollover choice onto everyone's personal budget and
onto the household budget. budget_categories is the only table carrying both
the category and the owner. A regression test covers that isolation.
Naming follows the same products: the toggle reads "Rollover", the noun, not
"Roll over", the verb -- which also matches `rollover_enabled` and the
calculator. Both tooltips now describe the property rather than a direction
("keep this category's unspent money from one month to the next"). The
previous wording named the direction the flag actually gates, incoming,
which is accurate but the opposite of the mental model every comparable
product installs; describing the property is true under either reading. The
French card string switched to "+%{amount} de report" so it no longer has to
agree in number with a currency noun it cannot see.
bin/rails test: 6942 runs, 0 failures. The inheritance was confirmed
load-bearing by removing it and watching its tests fail.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CyD26wsXjsYfpTgAGL1n1Z
* fix(budgets): make a rollover opt-out stop the money in both directions
`incoming_carry` gates what a month receives, but `leftover_for` computed
what it sends regardless of the toggle. So switching rollover off for one
month and back on the next handed the opted-out month's whole allocation to
the month after: the surplus the user meant to forfeit reappeared a month
later. Reproduced at 100, where 0 was expected.
The outgoing carry is now gated on the same flag, which also skips the
actuals lookup for opted-out rows. "Off" now means this envelope does not
roll over, in either direction -- the reading the standing toggle and the
tooltip both promise.
Found by CodeRabbit on #3143. It only became wrong with the standing-choice
inheritance in
|
||
|
buzzromainandClaude Opus 5
|
1fddb4d97c |
feat(budgets): carry a category's unspent budget into the next month (#3143)
* feat(budgets): carry a category's unspent budget into the next month
A budget category resets to zero every month, so anything non-monthly
(annual insurance, a holiday fund, car servicing) has no place to
accumulate. Two columns on budget_categories turn a category into a real
envelope: `rollover_enabled`, opt-in per category and off by default, and
`rolled_over_amount`, the surplus carried in from the previous month.
rolled_over(n) = rollover_enabled
? max(0, budgeted(n-1) + rolled_over(n-1) - actual(n-1))
: 0
v1 floors at zero: only a surplus carries, never an overspend.
The amount is materialized, not derived. March depends on February which
depends on January, so computing it on read would walk the whole chain on
every budget render. Budget::RolloverCalculator recomputes it in a single
forward pass and writes once via upsert_all, from Budget.find_or_bootstrap
and from BudgetCategoriesController#update -- allocations and the toggle
being the only inputs. No Transaction hook: a past month's actuals can
change after the fact, and the page load is a fine moment to catch up.
Scope kept deliberately narrow. `Budget#budgeted_spending`,
`#allocated_spending` and `#available_to_allocate` are untouched -- the top
of the budget page still answers "I planned to spend X, I've allocated Y".
The carry is per-envelope information, surfaced as `Budget#total_rolled_over`
and never folded into those totals.
What the carry does change is consumption: `available_to_spend`,
`percent_of_budget_spent` and `budgeted?` all count it, or a category funded
entirely by rollover would read as unbudgeted and get an alert pill while it
still had money left. `display_budgeted_spending` stays the month's
allocation alone -- the card shows the two figures side by side.
Details worth knowing:
- A parent's carry is net of its ring-fenced subcategories'. A parent's
allocation already contains theirs and its actuals already contain their
spending; those subcategories carry their own surplus, so counting the
parent's raw leftover would roll the same money over twice.
- Chains never mix: household with household, a member's personal budgets
with their own. A missing month is a gap the carry crosses, not a month
budgeted at zero.
- The carry stops at a currency change. sync_budget_categories stamps
categories with family.currency at sync time while a budget freezes its
own at creation, so the guard is on budget_category.currency -- the unit
the amount is actually denominated in.
- upsert_all writes with `update_only`, so a concurrent request that moves
an allocation between our read and our write doesn't get it clobbered by
the stale value we loaded.
- copy_from! copies the toggle, never the amount.
Cost for families that never turn it on: one EXISTS query per budget page
load, measured, including on the reports page which also bootstraps a
budget. With rollover on, the walk starts at the first month that uses it
rather than at the two-year history bound.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CyD26wsXjsYfpTgAGL1n1Z
* fix(budgets): pin the household rollover chain to a viewer-independent scope
Addresses review feedback on #3143.
The household budget (user_id NULL) has no owner to scope actuals by, and
`IncomeStatement` falls back to `Current.user` when nobody says otherwise.
The calculator therefore computed one shared `rolled_over_amount` through
whichever member happened to load the page, and each viewer overwrote the
other's number -- last one wins, and a member could infer spending in
accounts they cannot see. `Budget#income_statement_accounts` can now be
overridden, and the calculator pins the household chain to the whole
family so the shared row holds one number. Personal chains are untouched:
they already scope to their owner's accounts and were always deterministic.
`copy_from!` runs after `find_or_bootstrap` has already recomputed the
chain, so copying `rollover_enabled` left the target sitting on a zero carry
until the next page load. It now recomputes before its transaction commits.
The toggle tooltip described the wrong direction. `incoming_carry` checks
the flag of the month being computed, so the toggle governs what that month
*receives* from the previous one, not what it sends forward. Reworded in
English and French.
The concurrency regression test now drives its concurrent write through
`Budget#budget_category_actual_spending`, a public seam, instead of stubbing
a private method of the calculator from another class's test suite.
Each guard was confirmed load-bearing by reverting it and watching its test
fail. bin/rails test: 6939 runs, 0 failures.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CyD26wsXjsYfpTgAGL1n1Z
* fix(budgets): let the rollover choice stand instead of resetting each month
`rollover_enabled` lives on budget_categories, one row per (budget,
category), so a month created by `find_or_bootstrap` was born with the flag
off. Switching rollover on for Vacations in January and simply opening
February dropped January's surplus on the floor -- the user had to re-arm
the toggle every month, or go through "copy from previous budget". The
feature's headline case, a category funded 50/month accumulating over a
year, did not work as shipped.
New rows now inherit the flag from the last initialized budget of the same
owner, the same chain the carry itself walks. Turning the toggle off on a
given month still overrides it from there on, so the per-month escape hatch
survives.
The flag stays on budget_categories rather than moving to Category, which is
where comparable products (Monarch, Copilot, Lunch Money) put it. Categories
here are family-wide while budgets are per owner, so a category-level flag
would force one member's rollover choice onto everyone's personal budget and
onto the household budget. budget_categories is the only table carrying both
the category and the owner. A regression test covers that isolation.
Naming follows the same products: the toggle reads "Rollover", the noun, not
"Roll over", the verb -- which also matches `rollover_enabled` and the
calculator. Both tooltips now describe the property rather than a direction
("keep this category's unspent money from one month to the next"). The
previous wording named the direction the flag actually gates, incoming,
which is accurate but the opposite of the mental model every comparable
product installs; describing the property is true under either reading. The
French card string switched to "+%{amount} de report" so it no longer has to
agree in number with a currency noun it cannot see.
bin/rails test: 6942 runs, 0 failures. The inheritance was confirmed
load-bearing by removing it and watching its tests fail.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CyD26wsXjsYfpTgAGL1n1Z
* fix(budgets): make a rollover opt-out stop the money in both directions
`incoming_carry` gates what a month receives, but `leftover_for` computed
what it sends regardless of the toggle. So switching rollover off for one
month and back on the next handed the opted-out month's whole allocation to
the month after: the surplus the user meant to forfeit reappeared a month
later. Reproduced at 100, where 0 was expected.
The outgoing carry is now gated on the same flag, which also skips the
actuals lookup for opted-out rows. "Off" now means this envelope does not
roll over, in either direction -- the reading the standing toggle and the
tooltip both promise.
Found by CodeRabbit on #3143. It only became wrong with the standing-choice
inheritance in
|
||
|
Sure Admin (bot)
|
3bdfe9521e |
Fix AI health check for OpenAI-compatible endpoints (#3184)
* Fix AI health probe for OpenAI-compatible endpoints * Stabilize accounts sync system test * Address AI health probe review feedback |
||
|
Juan José MataandClaude Opus 5
|
d6462f5fc9 |
feat(snaptrade): add device-flow OAuth alongside the browser redirect (#3126)
* feat(snaptrade): add device-flow OAuth alongside the browser redirect SnapTrade could only be connected through the authorization-code + PKCE flow, which needs a confidential OAuth client: SNAPTRADE_OAUTH_CLIENT_SECRET and a redirect URI registered on the OAuth app. A deployment that cannot register one had no path at all. Add the device grant (RFC 8628) as a second way to obtain the same token, so people can pick the flow that suits their deployment. Both grants end at SnaptradeItem#apply_oauth_tokens!, so a device-authorized item is indistinguishable from a redirect-authorized one from there on -- same Bearer data calls, refresh, revocation and sync. Nothing about existing authorized items changes: no schema change, no migration, and the PKCE path is untouched. - Provider::Snaptrade gains start_device_authorization and poll_device_token, with endpoints read from SnapTrade's OAuth metadata document (cached). - oauth_configured? now means "some flow is available" (public client id), which is what gates syncing and the provider panel; the new authorization_code_configured? gates the redirect flow specifically. - Token and revocation requests authenticate as a public client when no secret is configured -- client_id in the body instead of HTTP Basic. Without this a device-authorized item would authorize fine and then fail at its first token rotation. - The settings panel offers both when both are available; every other entry point picks one through SnaptradeItemsHelper#snaptrade_authorize_path. - The device page carries a failed attempt's code back into the form, so "not confirmed yet" is a retry rather than a restart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk * fix(snaptrade): keep the provider panel's setup-step keys and cover both flows Two test_unit failures from the panel change. The setup steps were reordered and their keys renamed, which orphaned the translations twelve locales already had for them and broke the test asserting `oauth_setup_step_3`. The rename bought nothing: reword the steps in place instead, leaving the callback URL on step 2 where the interpolation lives. The panel tests stubbed `oauth_configured?`, which no longer decides which buttons render -- that is now `authorization_code_configured?`. Stub both, so the "configured" cases test the deployment they name, and add the device-only case that was previously unreachable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk * fix(snaptrade): address device-flow review findings Two real bugs from the bot reviews, plus consistency work. The completion form posts into the `drawer` frame so errors re-render in place, but a successful redirect was then followed as a frame navigation. Both destinations carry the layout's empty `drawer` frame, so Turbo swapped that in and merely closed the dialog: the notice was lost and `return_to=setup_accounts` never advanced. Success now breaks out with a redirect stream action, the same mechanism holdings and categorizes already use, while errors keep rendering in the drawer. RFC 8628 §3.1 requires a confidential client to authenticate its device authorization request, and the panel offers the device code on deployments that configured a secret. That request now carries the same client authentication as the token request. Token endpoint resolution is now shared by all three grants, since whatever issued a token has to be what refreshes it. It reads the discovery document only when already cached and never fetches it, so the browser flow keeps working off the constant it has always used -- no new network call on refresh and no new way for an existing authorized item to fail. Also: the drawer no longer asks the provider whether it is configured, the controller tells it; and the test helpers restore the previous OAuth config rather than clearing it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk * fix(snaptrade): reject a device authorization response that cannot drive the flow A 2xx missing device_code, user_code or a verification URI was passed straight to the drawer, which then rendered a blank code and a link to nowhere -- a dead end the user could only abandon. Every one of those fields is load-bearing, and a response without them is partial or schema-changed, so fail with a message instead. Same reasoning as the results-array check in get_positions. verification_uri_complete substitutes for verification_uri when present, since the drawer prefers it for the link anyway. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk * fix(snaptrade): filter device-flow codes from request logs complete_oauth_device_flow receives the device code as a request parameter, and none of the existing filter_parameters patterns is a substring of "device_code" -- ParameterFilter matches on substrings, and "token", "_key", "secret", "code_verifier" and "code_challenge" all miss it. So Rails' default "Processing by ... Parameters: {...}" line was writing it in plaintext. That matters more here than ordinary log hygiene: the device code is the only capability check on redemption. Unlike the redirect flow's state, nothing binds a device code to the family that requested it, so anyone who can read the logs could redeem another family's in-flight authorization into their own item and pick up a token for that family's brokerage data. Adds :device_code, :user_code and :verification_uri_complete (which embeds the user code) to the filter list, with a regression test in the style of the existing Sophtron credential-filtering test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk * fix(snaptrade): bind a pending device authorization to its session The device code was posted back from the drawer as a form field, so the request body was the only thing deciding which item a pending authorization redeemed into. Nothing tied a code to the family that asked for it -- the guarantee `state` gives the redirect flow -- so a code recovered from anywhere could be redeemed into an item belonging to someone else, handing them a token for the victim's brokerage data. Hold the pending authorization in the session instead, where oauth_callback already keeps its code_verifier and state: - start_oauth_device_flow records the code, what the page displays, the family, the item and the return_to context under :snaptrade_device_flow. - complete_oauth_device_flow reads the code from there and refuses unless the flow was started by this session for this family and this item. A device_code parameter is no longer read at all, so there is no longer a way to inject one. - return_to and accountable_type come from the session too, so completion needs nothing from the form to find its way back. The code now never reaches the browser, which also makes the previous commit's log filtering a second line of defence rather than the only one. A failed attempt keeps the code only while it is still redeemable: expired_token and access_denied clear it so the page offers a fresh start, while a transient failure leaves it in place to retry. expires_in and interval are no longer carried anywhere, since nothing ever read them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk * fix(snaptrade): use one token endpoint for every grant poll_device_token resolved the token endpoint from the cached discovery document while exchange_code and refresh_tokens used TOKEN_URL, so which URL a device-issued token was refreshed at depended on whether the 12h metadata cache was still warm. If the discovered endpoint ever differed from the constant, a device-authorized item would work until the cache lapsed and then fail its first rotation -- and fail invisibly, since a refresh failure marks the connection requires_update. Resolve it by removing the choice rather than by making refresh depend on discovery. RFC 8628 §3.4 redeems a device code at the authorization server's token endpoint, the same one the authorization code grant uses: there is one token endpoint, not one per grant, and nothing to keep in sync between issuing a token and refreshing it. TOKEN_URL is also the endpoint the browser flow has been using in production, so it is the one with evidence behind it. Discovery is still consulted, but only for device_authorization_endpoint, which has no hardcoded equivalent. This also keeps refresh free of any network dependency it did not already have: reintroducing discovery there would have put a fetch, with retries and backoff, in front of every token rotation on items that never needed one. Also restore the previous OAuth configuration in the missing-client-id test instead of leaving the client id nil, which made it order-dependent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
|
0aa43de10a |
Add experimental Swift-native Sure Insights app (#3134)
* Add Swift-native Sure app * Fix push subscriptions schema for CI * Address native app review feedback * Address remaining native app review feedback * Use Flutter app logo for native icon * Honor insight notification preferences and locale --------- Co-authored-by: Juan Jose Mata <2v8shcb6pz@privaterelay.appleid.com> Co-authored-by: sure-admin <sure-admin@splashblot.com> |
||
|
Sure Admin (bot)andJuan José Mata
|
311f06e404 |
Fix budget cache invalidation after transaction deletion (#2808)
Signed-off-by: Juan José Mata <juanjo.mata@gmail.com> Co-authored-by: Juan José Mata <juanjo.mata@gmail.com> |
||
|
|
3e79de5b64 |
perf: memoize Family#balance_sheet/investment_statement, cache transactions-index side queries (#3058)
* perf: memoize Family#balance_sheet/investment_statement, cache transactions-index side queries The account sidebar renders on every page (mobile + desktop, 3 tabs each) and calls Family#balance_sheet multiple times per render; neither it nor Family#investment_statement/InvestmentStatement#current_holdings were memoized, so each call rebuilt the underlying query from scratch. Memoize both per-user (family sharing means different users must not share a cached BalanceSheet/InvestmentStatement). Also cache TransactionsController#index's uncategorized_count and projected_recurring lookups, which run unconditionally on every request regardless of whether the underlying data changed, using the same entries_cache_version-keyed pattern already used elsewhere in the codebase (e.g. Transaction::Search#totals). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: address PR #3058 review feedback on cache invalidation and query reuse - Invalidate transactions-index caches when the current user's AccountShare access changes, not just on entries/recurring updates (CodeRabbit/Codex flagged revoked users could see stale data for up to a day). - Use full-precision timestamps instead of to_i in the cache keys so same-second updates aren't missed. - Reuse the already-memoized investment_account_ids in InvestmentStatement#current_holdings instead of an extra any? query. - Assert the rendered response instead of a controller instance variable in the uncategorized_count test, per CodeRabbit nitpick. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: assert rendered response, not implementation details, in PR #3058 tests Two CodeRabbit nitpicks from the second review round: the uncategorized-count cache-reuse assertion matched a scope name that never appears in generated SQL (making it vacuous), and the recurring-cache revocation test read the controller's private @projected_recurring ivar instead of the rendered page. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: bust transactions-index caches on entry/recurring deletion and account status change jjmata's PR review flagged two invalidation gaps: hard-deleting an uncategorized entry or recurring transaction left the previous max updated_at unchanged (cache never busted), and toggling an account's active status doesn't touch entries/AccountShare at all. Fold in counts (like account_share_version already did) and a new Family#accounts_status_version, and move the version helpers onto Family/Current per the "fat models, skinny controllers" nit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: include merchant version in projected_recurring cache key Editing or deleting a FamilyMerchant doesn't touch recurring_transactions, so the cached projected-recurring list (rendered with merchant name/logo, expires_in: 1.day) could show stale merchant data for up to a day. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(transactions): version projected-recurring cache by referenced merchants, not just FamilyMerchant recurring_transactions.merchant_id can point at a shared ProviderMerchant (recurring detection copies transaction.merchant_id), not just a family-owned FamilyMerchant. merchants_version only tracked Family#merchants (FamilyMerchant), so a ProviderMerchant update (e.g. ProviderMerchant::Enhancer setting name/logo) left the cached projected recurring list stale for up to a day. Replace Family#merchants_version with #recurring_transaction_merchants_version, scoped to the merchant records actually referenced by the family's recurring transactions (both FamilyMerchant and ProviderMerchant), and bump the cache key version. Also fix a flaky test assertion that matched all recurring_transactions-table queries instead of the actual projection query, since computing the cache key itself still runs small COUNT/MAX queries against that table on a cache hit. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Gerald <248542187+gfr-free@users.noreply.github.com> |
||
|
|
00d35993eb |
Exclude pending transactions from balances (#2897)
* Exclude pending transactions from balances * Fix balance regression assertions * Add pending balance review regressions * Fix merge conflict commit --------- Signed-off-by: Juan José Mata <juanjo.mata@gmail.com> Co-authored-by: Juan José Mata <juanjo.mata@gmail.com> Co-authored-by: Juan José Mata <jjmata@jjmata.com> |
||
|
Jeffandjeffrey701
|
12eb9e15fb |
fix(lunchflow): mark sync unhealthy when importer reports fetch failures (#1796) (#1873)
* fix(lunchflow): mark sync unhealthy when importer reports fetch failures (#1796) `LunchflowItem::Syncer#perform_sync` called `lunchflow_item.import_latest_lunchflow_data` but threw the result away and then ran `collect_health_stats(sync, errors: nil)`. The importer already catches per-account 429/500 fetch errors, bumps a `transactions_failed` counter, and returns `success: false` — but the syncer never inspected the return value, so the parent sync was marked completed/green even when zero transactions were imported because every fetch had been rate-limited. Capture the importer result and translate any `accounts_failed` / `transactions_failed` / `error` fields into the `{ message:, category: }` error shape `collect_health_stats` expects. The exception-path `rescue` branch is unchanged. Closes #1796 * test(lunchflow): i18n the new health messages + add syncer invariant tests (#1796) Two pieces of follow-up feedback: - @coderabbitai + @JSONbored: the three new operator-facing strings should go through I18n.t. Add keys under provider_warnings.lunchflow_* (matching the existing provider_warnings.limited_investment_data shape) and use Rails pluralization for the count-bearing entries. Other locales follow the repo's normal translation flow. - @jjmata + @JSONbored: add tests for the invariants. New LunchflowItem::SyncerTest covers: * successful import → sync healthy * accounts_failed positive → sync unhealthy with localized message * transactions_failed positive → sync unhealthy with localized message * both counters positive → both error entries recorded in order * sync raises → sync_error category + reraise (existing rescue branch) @jjmata also asked to confirm the importer contract: LunchflowItem::Importer#import returns 'success: accounts_failed == 0 && transactions_failed == 0' (see importer.rb), so the early 'return [] if import_result[:success]' guard is safe — success is never true while either counter is positive. --------- Co-authored-by: jeffrey701 <jeffrey701@users.noreply.github.com> |
||
|
Juan José Mata
|
fd6f4ff078 |
Add live AI checks to system health (#3155)
* Add live AI checks to system health Give super admins a dedicated AI status view with bounded liveness probes for LLMs, vector stores, pgvector, and embedding endpoints. Record sanitized failures in both the system debug log and Rails logger, and document the recommended local configuration.\n\nCloses #3145 * Fix AI health CI checks * Address AI health review feedback * Correct Ollama model preload guidance * Distinguish OpenAI-compatible providers * Make Ollama startup readiness explicit * Recognize Cloudflare AI endpoints |
||
|
buzzromainandClaude Opus 5
|
c26b16d36f |
chore(goals): drop copy-pasted duplicates and an unused lock-key helper (#3159)
`Account#goal_earmarked_total` and `Account#free_to_earmark` were each defined twice, byte for byte, with no `private` boundary or singleton class between the blocks — a copy-paste artifact where the second definition silently overwrote the first. Keep one copy of each. `Goal.advisory_lock_key_for` had no caller anywhere in app/; the only reference was a test asserting the dead helper was deterministic. Remove both. No behavior change. Claude-Session: https://claude.ai/code/session_01DJ1npaGEHr6t2HW1rYZdt4 Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
buzzromainandClaude Opus 5
|
721fc394ec |
fix(securities): give prefixed crypto tickers their logo back (#3151)
Security#crypto_base_asset only stripped a fiat suffix — "BTCUSD" gave "BTC" — so it answered nil for the "CRYPTO:BTC" form, which is the form every crypto integration writes: the on-chain wallets, Kraken, CoinStats and Binance all store the prefix. display_logo_url feeds that nil to the crypto branch, so none of those securities carried a logo at all. Provider::BinancePublic already parses every shape it accepts — the pair form, the prefixed pair, the bare base asset and the USD stablecoins — so this delegates instead of growing a second parser next to it. The parsing moves to a class method for that; the private instance method stays as a delegator, so its own callers and their tests are untouched. Verified across the four documented forms: CRYPTO:BTC and BTCUSD both give BTC, CRYPTO:ETH gives ETH, CRYPTO:USDT gives USDT. A logo still needs BRAND_FETCH_CLIENT_ID configured — this fixes the parsing, not the hosting requirement. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
GFRandClaude Sonnet 5
|
0252787dc0 |
fix(enable-banking): use real merchant instead of POS terminal line for name (#2968)
* fix(enable-banking): use real merchant instead of POS terminal line for name
Some ASPSPs (e.g. BankDirekt/Raiffeisen in Austria) return
remittance_information as a multi-element array where the first line is a
generic card terminal descriptor (\"POS 45,13 AT D6 31.07. 10:27\")
and a later line holds the real merchant. EnableBankingEntry::Processor
always used the first array element, so transaction names showed the
terminal string instead of the merchant.
primary_remittance_information now skips lines that look like a technical
terminal booking (POS/ATM + amount, or a trailing date+time stamp) and
prefers the first descriptive line, falling back to the original element
when nothing better is available. It also strips known small-merchant
payment-processor prefixes (SumUp, Square, iZettle, PayPal) from the
selected line.
Fixes #2935
Disclosure: this fix was written by Claude Code, verified against the
reporter's real (decrypted) Enable Banking payload and against test-stack
Rails test / RuboCop / Brakeman runs.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix(enable-banking): require both technical-line signals together
Address CodeRabbit/Codex review feedback on #2968: the POS/ATM+amount
prefix and the trailing date+time suffix were OR'd, so either alone
could misclassify a legitimate line as technical. A line embedding the
merchant right after the amount (e.g. "POS 45,13 BILLA DANKT ...") or
a legitimate descriptor that happens to end in a timestamp (e.g.
"Invoice paid 31.07. 10:27") would have been wrongly skipped.
Both signals are now required together in a single anchored pattern,
matching every real technical line observed in production while no
longer misclassifying either of the scenarios above.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix(enable-banking): tighten date-stamp regex and clean up the merchant line
Addresses two review threads on this PR:
1. jjmata (PR review): technical_remittance_line?'s date-stamp check required
exactly 2-digit day/month (\d{2}[./]\d{2}), so an un-padded date ("1.07."
instead of "01.07.") wasn't recognized as technical and the line would
resurface as the transaction name -- reproducing the original #2935 bug for
that date shape. Now accepts 1-2 digits for both.
2. john-frandsen (issue #2935 comment): suggested cleaning up the merchant line
further (e.g. "BILLA DANKT 0007114 SIEGENDORF 7011" -> "Billa"). Checked
point 1 (structured remittance fields) against Enable Banking's own API
docs -- no such field exists there, not applicable. Points 2/3/5 already
match current behavior. Point 4 (loyalty-marker cleanup) implemented as two
layers:
- Primary: match the line against merchants the family already knows
(Family#known_merchant_names) -- self-maintaining, no pattern-guessing,
and now also assigns the transaction's merchant when matched (previously
out of scope for blank-counterparty EB transactions). Case-insensitive,
regex-escaped, longest-match-wins, with a minimum length guard against
spurious short-name matches.
- Fallback (no known merchant yet): remove only the "DANKT"/"DANKE"
thank-you marker word itself, not a directional truncation -- the marker
can precede or follow the merchant name depending on phrasing ("X DANKT"
vs. "DANKE ... bei X"), so truncating at it risked deleting the real
merchant name in one of the two phrasings.
Verified against the full test suite, RuboCop, and Brakeman on a test-stack
Rails instance (0 RuboCop offenses, 0 Brakeman warnings; full-suite failures
present on that instance are pre-existing/environmental and unrelated to
these files).
Disclosure: this fix (investigation, implementation, and tests) was written
by Claude Code.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix(enable-banking): address CodeRabbit/Codex review on merchant-line cleanup
Follow-up to
|
||
|
buzzromainandClaude Opus 5
|
0d0003b032 |
fix(onchain-wallets): show linked wallets on the accounts page, and drop the tracking row on unlink (#3136)
Two gaps in #3081 after it merged, both raised there by @jjmata's review of #2191 — credit to that PR for identifying them. **Linked wallets were invisible on /accounts.** The feature creates real Sure accounts and they count towards net worth, but the accounts page never showed them: `Account.manual` excludes anything carrying a provider link, and unlike the twenty other providers there was no on-chain section to claim them. A family whose only connection was a wallet saw the empty state on the one page meant to list their accounts. The controller now loads the items, the view renders them, and the empty-state condition counts them. The card is rendered by name rather than as a collection, because the model's default partial slot is already the provider settings row; renaming it would be the tidier convention but costs eight locale files of churn for a follow-up fix. **A generic unlink left the tracking row behind.** The dedicated disconnect flow is careful, but `AccountsController#unlink` destroys the AccountProvider directly, and an OnchainWalletAccount holds that link rather than being held by it. Orphaned, it stops syncing — the syncer only reads linked rows — while its partial unique index still holds the (item, chain, address, asset) slot, so linking that same asset again would collide with a row nothing displays. It is destroyed with its link now, following the CoinStats precedent in the same model, with a guard against the recursion that precedent lacks. The regression test is the one asked for explicitly: the Sure account and its holdings survive as manual while the tracking row and the provider link go. **The card only renders accounts the viewer may see.** Found in review of this branch. An item is surfaced as soon as ONE of its accounts is accessible, so rendering them all showed a member given access to one wallet account the names and balances of the others. Reproduced before fixing, with two real addresses under one item and a member shared into only one: the unshared account's name appeared on /accounts. Non-admins now get the accessible subset and the address count derives from it; admins keep the whole item, which is the rule visible_provider_items already applies. That pattern is not specific to this card — six existing providers pass `item.accounts` unfiltered to the same partial, which filters nothing. In the same reproduction the unshared name appeared twice, once from a CoinStats card over the same accounts. Raised separately for the maintainers; only this card is changed here. Both figures are computed on the item off the preloaded associations and prepared per card by the controller, the way `_coinstats_sync_stats_map` already is: a first attempt did it in the template with a `linked` scope, which opened a fresh relation and cost a query per row. Measured on /accounts with 1 then 5 wallet items: 52->79 queries became 50->69, so 6.75 per extra item became 4.75. Verified beyond the suite: a real Bitcoin address linked, then both flows driven over real HTTP — `GET /accounts` renders the card, and `DELETE /accounts/:id/unlink` leaves the account behind reporting `manual: true`. Integration parity was checked rather than assumed: on-chain now appears everywhere CoinStats does, the nightly family sync picks it up through its own reflection over `*_items` associations, and it is already exposed to the mobile client via /api/v1/accounts and to /api/v1/provider_connections. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Sure Admin (bot)
|
81c613f64a |
Improve async AI rule error reporting (#2271)
* Improve async AI rule error reporting * fix(rule-runs): preserve failures on job completion * fix(rule-runs): deduplicate failure messages |
||
|
Josh
|
ca66346dc5 |
feat: add safe admin user removal (#3131)
* feat: add safe admin user removal * fix: address user removal review findings * fix: close remaining user removal review gaps * fix: handle deleted users during session creation * fix: fail closed when session creation fails * fix: reject token issuance for inactive users |
||
|
|
735b62d9c7 |
Track self-custody wallets natively: Bitcoin, EVM and Solana (#3081)
* feat(onchain-wallets): foundation for self-custody wallet tracking
Adds the schema, models and the normalised contract every chain will
produce, with no chain implemented yet.
The central constraint of a multi-chain integration is that `case chain`
must not spread through the importer, processor, controller and views.
So a chain adapter's only job is to turn an address into an
Onchain::Snapshot — a list of Onchain::Assets and Onchain::Movements —
and Onchain::Chains is the single source of truth for which chains exist,
how their addresses are validated, what their native asset is, and which
adapter to instantiate. Everything downstream is written once.
Two tables:
- onchain_wallet_items: the family-level connection. Keyless by
default; the only credential is an optional Etherscan key, encrypted
via `encrypts`.
- onchain_wallet_accounts: one row per asset, per address, per chain.
Uniqueness uses three partial unique indexes, one per asset kind, because
the identity of an asset depends on its kind: a native coin is identified
by its address alone while a token is identified by its contract/mint. A
single index over all columns would treat two native rows with a NULL
contract_address as distinct and let duplicates through — the model test
proves the rejection comes from the database by saving with
`validate: false`.
The schema also leaves room for extended-key (xpub) wallets later: an HD
wallet is a set of derived addresses under one item, which the current
uniqueness key already allows, so adding it needs no destructive change.
db/schema.rb is hand-edited to add only the two new tables: regenerating
it with the Rails version now in the Gemfile reorders every column in the
file, which is out of scope for this change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): chain-agnostic importer, processor and syncer
The whole pipeline is written once here and never branches on chain: it
consumes Onchain::Snapshots, so a chain is only ever a registry key and an
adapter.
Importer: refreshes every tracked address and records a digest of what it
saw (quantity plus movements, no timestamps) on each row. It deliberately
never creates rows — real wallets are full of spam airdrops, so a newly
seen token becomes trackable only when the user ticks it. An asset that
disappears from a wallet goes to zero rather than going stale.
Syncer: reprocesses only the rows whose digest changed. Two consecutive
syncs of an idle wallet write nothing at all — no row updates, no
holdings, no entries, and no queued account syncs. Both the importer and
syncer tests for that were checked against the pre-fix behaviour: with the
content_hash guard removed they fail.
Processor: writes the holding, the account balance and the movements.
Movements materialise two ways. When that day's price is known, a signed
trade (positive = Buy, negative = Sell) so cost basis and the value chart
reconstruct back to acquisition. Otherwise a display-only entry with
amount 0 and excluded: true, raw movement preserved in `extra` — visible,
but not inventing a value that would distort the account's history. Prices
are matched on the exact day for trades, because valuing a two-year-old
transfer at today's price would fabricate a cost basis.
Onchain::SecurityResolver binds a "CRYPTO:<SYMBOL>" ticker straight to the
crypto price provider instead of going through provider search, where
"USDC" can come back as a EUR-quoted pair and then need FX to repair. It
reuses an existing Security for the ticker whatever its MIC, so one asset
never splits into two records. Symbol normalisation for bridged
stablecoins and the price backfill follow in the next commit.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): canonical asset symbols and price backfill
Two things stood between a linked wallet and a correct valuation.
Bridged and wrapped variants. The same dollar is USDC on Ethereum, USDC.e
on Arbitrum and USDbC on Base; the same ether is ETH natively and WETH
once wrapped. Left alone each variant becomes its own Security, so one
gets priced and the others sit at zero, and the same asset held on two
chains reports two different values. Onchain::AssetSymbol maps the
variants that are redeemable 1:1 onto the canonical asset, so pricing them
as that asset is exact rather than approximate.
Missing price history. A Security created at link time has none, so on the
first sync every movement would fall back to a display-only entry and the
cost basis would never reconstruct — the feature would look broken exactly
when the user first looks at it. The processor now backfills the window in
one batched provider call rather than one call per movement date, includes
today so a wallet whose movements all predate the sync window still gets a
current valuation, and treats a provider failure as a logged warning: the
holding and the quantity are still written.
All of it is a no-op when no crypto price provider is enabled, which is
the case the settings panel has to warn about before linking.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): Bitcoin adapter
Bitcoin has no account balances: an address owns unspent outputs, so the
balance is everything ever paid to it minus everything spent from it, and
mempool totals count — a broadcast-but-unconfirmed spend has already left
the wallet as far as its owner is concerned. Movements are the net effect
of each transaction on the address, so a self-transfer nets to zero and
produces no entry.
All three address formats are accepted (Base58 P2PKH/P2SH, bech32
segwit, bech32m taproot) with the character-set exclusions each encoding
actually has, and a malformed address is rejected before any request is
made — the test relies on WebMock failing the run if a request escapes.
Single address, not extended keys. A Bitcoin wallet is normally an HD
wallet: one xpub derives thousands of addresses and change goes to derived
ones, so tracking a single address under-reports such a wallet. Extended
keys would need BIP32 derivation (a dependency this codebase does not
want) or a descriptor-indexing backend. The limitation is stated in the
adapter, will be stated in the linking UI, and an xpub is rejected as an
address rather than silently treated as one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): EVM adapter for six networks, two backends
One adapter class serves every EVM network: a network's identity — label,
native coin, explorer URL, whether a family-supplied Etherscan key applies
— is data in the chain registry, so adding a network is an entry there
rather than a branch here.
The unit tracked is the (chain, address) couple, carried by the unique
index. A 0x address is valid on all six networks and holds different
balances on each, so detection asks each candidate network whether the
address is worth tracking there. That probe is exactly one request and
never reads paginated history: Blockscout's address summary carries the
coin balance and the token/transfer flags together, which is also why a
wallet holding only ERC-20 tokens with zero native balance is still found.
An explorer being down means "not detected here", not an error the user has
to interpret — a dead indexer must not break linking.
Two interchangeable backends behind Provider::EvmExplorer: keyless
Blockscout by default for every network, and Etherscan when the family
configured a key on a network the registry enables it for (today Ethereum
only), where a key buys nothing but a higher rate limit. Etherscan
deliberately does not implement the activity probe: its one-request answer
is the native balance, which reports "nothing here" for a token-only
wallet, so detection stays on the indexer that can answer correctly.
Zero-balance token rows are dropped — real wallets are full of spam dust —
while the native asset is always reported, even at zero, because a wallet
that spent everything still has a history worth keeping.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): Solana adapter
A Solana wallet does not hold its tokens. Each SPL token sits in its own
token account, owned by the wallet but addressed separately, so balances
come from enumerating those accounts across both token programs rather
than from reading the wallet address — and because one wallet can own
several accounts for the same mint, they are summed into one position.
Emptied token accounts are left behind on chain by design and are dropped.
RPC gives mints, not metadata. Well-known mints get their real symbol;
anything else is labelled with its mint in a form that deliberately cannot
pass for a ticker, so security resolution declines it and the asset is
tracked by quantity rather than priced as some unrelated coin that happens
to share a name.
Activity is one request. Bitcoin's Base58 addresses fall inside Solana's
address shape, so the two are told apart by asking the node: it rejects a
non-32-byte key, which reads as "not here" rather than an error.
The Snapshot it returns has the same shape as Bitcoin's and the EVM
adapter's — asserted in the test — so nothing downstream changed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): settings panel and linking flow
Linking is three steps: paste an address, confirm which network, choose
what to track.
The network step exists because address formats are not unique to a chain.
Candidates come from the address shape, and when there is more than one each
is asked whether the address is worth tracking there — one bounded request
each. When several answer yes, or none does, the user picks from a list that
marks which ones showed activity. Silently keeping the first match would
link the wrong network and surface later as a sync bug.
The token step imports nothing that was not ticked. Assets whose symbol a
price provider can quote are pre-checked; spam airdrops, whose "symbol" is
usually an advertisement, are listed unticked and can still be tracked by
quantity. Quantities and metadata are re-read from the chain when the
selection is applied, so a tampered selection can only change which assets
are tracked, never what they claim to hold. Previewing an address creates
no connection record, so an abandoned flow leaves nothing behind.
The panel and both modal steps carry a price-provider warning: with no
crypto market data enabled every wallet is valued at zero, which users
report as a broken sync rather than a missing setting, so it is said before
linking and links to where to fix it. The Bitcoin single-address limit and
"never enter a seed phrase" are stated in the linking UI too.
Errors are separated by kind. A rate limit or an unreachable explorer gets
its own localized message. Anything else is a bug: the user sees a generic
message and the class and message go to DebugLogEntry, never into the
response — asserted in the tests, which also check the exception text does
not appear in the body.
Adapters now translate their data source's errors into Onchain::Chains
errors, so the controller rescues two chain-agnostic types instead of
carrying a list of every explorer's error classes — which would have put
per-chain knowledge back into the controller.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): manage, review tokens, change address, disconnect
Four actions, because two would make the token choice irrevocable.
Review tokens reopens the selection screen with the address left alone —
deliberately without an address field. Without it the only way to untick a
token would be to change the address, which is a different operation
entirely. Assets the chain no longer reports stay listed and ticked, so
they can be dropped once they are gone.
Disconnect one asset is a per-row action with a button next to every asset.
A destroy route no view calls is dead code: the feature does not exist
until it has a button, so the test asserts one form per asset rather than
one per wallet.
Change address updates the existing rows instead of recreating them, so the
accounts, holdings, entries and balance history all survive — verified by
asserting a pre-existing balance is still there afterwards, and checked
against the recreate-instead-of-update behaviour, which fails it. The
content digest is cleared so the next sync reprocesses even if the new
address happens to hold the same amount, and an account still carrying its
generated name is renamed to match.
Disconnect wallet drops every asset at one address and leaves other
addresses alone.
Disconnecting never destroys an account: the provider link goes, holdings
are detached, and what the user can see stays as a manual account that
stops updating — the same contract every other provider's unlink has here.
The duplicate-address guard covers initial linking as well as address
changes. Without it, re-linking an already-tracked address would become the
unofficial way to add a token, quietly creating a second set of rows for
the same wallet; both guards are checked against that pre-fix behaviour.
Every lookup and mutation is scoped through Current.family, including the
per-row disconnect, which cannot reach a row belonging to another
connection.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs(onchain-wallets): hosting guide for self-custody wallet tracking
docs/hosting/onchain-wallets.md covers what the feature reads, which
endpoint serves each network and how to point it at your own instance, the
optional Etherscan key and why it is optional, the per-sync request cost and
where history is capped, and the management actions.
Two things get stated plainly because they generate the support traffic:
prices come from a separate market data setting, so with no crypto-capable
provider enabled every wallet is tracked by quantity and valued at zero; and
Bitcoin is one address at a time, which under-reports an HD wallet whose
funds are spread across derived addresses.
Every locale key the feature uses is checked to resolve, including the
pluralised ones.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): keep EVM balances on the keyless indexer when a key is set
Configuring an Etherscan key moved every read onto Etherscan, including
balances. That was wrong: Etherscan has no free endpoint that enumerates an
address's tokens, so its token balances are summed from transfer history —
which cannot see a rebasing token's current balance, and silently
under-reports any wallet whose history exceeds the page cap. A user adding a
key to fix a rate limit would have quietly traded it for wrong balances.
The two reads are now separated by what each backend can actually answer.
Balances and activity detection always go to the keyless indexer, whose
address summary answers both in one request — so adding a key buys nothing
there and cannot cost anything either. History, the paginated and
rate-limited half, is where a key helps and is the only thing it changes.
Provider::Etherscan drops its balance methods and refuses token_balances
with the reason, rather than offering an approximation that reads as a fact.
The chain-agnostic error mapping now accepts several error families per role,
since one snapshot can involve both backends.
Checked against the pre-fix behaviour: with balances routed through the
keyed backend, the new test reports 1 USDC held instead of the 7 the indexer
sees, and Etherscan raises on the balance call.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): name verified SPL tokens so they can be priced
Solana RPC returns mints, not names, so every SPL token outside the
hard-coded handful showed up as "SPL:abcd…wxyz" — which security resolution
correctly declines, leaving the asset tracked by quantity and valued at zero.
For a wallet holding anything beyond USDC that was most of its value.
Names now come from Jupiter's keyless token search, asked once per snapshot
for every mint at once, cached 24 hours per mint.
Only mints the list reports as *verified* are trusted. Anyone can mint a
token calling itself USDC; naming an unverified one would hand it the real
dollar's price and value dust at thousands. Unverified and unknown mints keep
the placeholder, and the test for that asserts the spam token is not named
USDC. Misses are cached as well, because spam wallets hold many mints that
will still be unvouched-for tomorrow.
Metadata is a naming nicety, not the wallet: a list that is down or rate
limiting degrades to placeholders instead of failing the snapshot, and the
hard-coded mints stay as an offline floor. Assets and movements resolve from
the same lookup, so a token cannot be called two different things within one
snapshot.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): enable crypto pricing in one click, and log zero valuations
The warning said what was wrong and where to fix it, but still made the user
navigate to another settings page and pick the right provider out of a list.
On a self-hosted instance an admin can now fix it from the warning itself:
the crypto provider is appended to the enabled securities providers, leaving
the others alone — enabling crypto prices must not turn off whatever prices
the user's equities, and the test fails if it does. On a managed instance the
providers are the operator's setting, so the button is not offered and the
action refuses.
The other half is diagnosis. Until now a holding valued at zero for this
reason looked exactly like a holding whose price simply had not been fetched
yet: nothing recorded it, so support had to infer it from a screenshot. The
processor now records it via DebugLogEntry — but only for this cause, since a
price merely missing for today is ordinary and already covered by the
backfill.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): upgrade display-only movements once their price is known
A wallet linked before market data covered its history got the worst of both
worlds: its transfers landed as zero-amount excluded entries, and nothing ever
brought them back. The syncer only reprocesses assets whose on-chain state
changed, and a two-year-old transfer never changes — so the cost basis stayed
broken for exactly the wallets that were linked earliest.
perform_post_sync now runs a repair over every linked asset, not just the ones
that moved: what changed is the price history, which no chain read can report.
It reads prices from the database only, makes no network call, and does
nothing when there is nothing to upgrade.
An upgraded entry keeps its external_id, so it is the same transfer rather
than a duplicate. The entry is destroyed and rewritten because an Entry cannot
change entryable type in place and import_trade refuses an id already held by
a Transaction. Entries from other providers are never touched — matched by
source and by this asset's own id prefix.
Checked against the pre-fix behaviour: with perform_post_sync empty again, the
syncer test fails.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): make history truncation visible and its depth configurable
History was capped silently. A wallet with more transfers than one sync reads
looked exactly like a wallet whose history was fully imported — the only trace
was a Rails log line on Bitcoin, and nothing at all on the EVM and Solana
paths. A user reconciling their cost basis had no way to tell the difference
between "this is all there was" and "we stopped reading".
Each source now reports whether it stopped on the budget or on the end of the
history. That travels on the Snapshot, so it stays chain-agnostic; the flag is
recorded on the affected rows, the manage screen says the history is
incomplete for that address, and the importer records it once per address per
sync — only when something changed, so an idle wallet with deep history does
not log the same line every night. The message also states what is not
affected: balances come from an address summary, never from history.
The depth itself is a hosting decision, not a property of a chain, so it moves
out of the providers into Onchain::HistoryBudget and is settable with
ONCHAIN_HISTORY_MAX_PAGES (default 10, clamped to 200). Adapters inject it, so
the providers stay unaware of the policy and the budget is testable without
touching the environment.
What this does not do is resume where it stopped. Reading older history across
syncs needs a per-wallet cursor and changes what the content digest means —
which is what guarantees an idle wallet writes nothing — so it is a change of
its own rather than a rider on this one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): read Blockscout token balances the way the API accepts
Verified against the live API: /api/v2/addresses/{address}/token-balances
rejects a `type` filter with HTTP 422 — the filter exists on token-transfers,
not here. Every EVM snapshot was therefore failing in production and
surfacing as "the explorer could not be reached", while the tests passed
because the stub encoded the same wrong assumption. No unit test can catch a
mistaken API contract; only asking the API can.
Without the filter the endpoint answers with every token standard the address
holds — ERC-20, ERC-721, ERC-1155, ERC-404 — so ERC-20 is now selected
client-side. An NFT row carries value "1" and no decimals, so it would have
been imported as a fungible balance of one token, priced by whatever its
symbol happened to resemble.
The tests now stub the endpoint the way it really answers and assert we ask
without a query, so re-adding the filter fails the run. Each token's
market-cap signal is carried through for the next commit, which has to decide
what to do about an address holding thousands of tokens.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): bound how many tokens one address can surface
Probing the live APIs turned up the other thing the stubs were hiding: real
addresses are airdrop dumping grounds. A well-known Ethereum address returns
7,924 ERC-20 balances (3.2 MB), and a comparable Solana one 2,801 token
accounts. Nothing bounded either. That meant a review screen with thousands of
rows nobody can use, and on Solana — where names are looked up in batches of
50 — around 56 extra requests per sync just to label an airdrop dump.
One read now surfaces at most 200 tokens per address
(ONCHAIN_MAX_TOKENS_PER_ADDRESS, clamped to 5,000). The native coin is never
affected, and anything already tracked keeps syncing regardless of the cap.
What survives the cap has to be both sensible and stable. On EVM the tokens
are ranked by the market cap the indexer already reports, so real assets stay
and airdrops fall off the end. Solana RPC gives no such signal, so the order
is the mint address: arbitrary, but identical between two reads of an unchanged
address — an unstable order would reshuffle the wallet, change the content
digest, and rewrite holdings every night. There is a test for that on both
chains. On Solana the cap is applied before metadata lookup, so it bounds the
requests as well as the rows.
The cap is not silent: it is recorded on the affected rows, stated in the token
review screen and in Manage wallets, and reported alongside history truncation
in the debug log with the limit that applied.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(onchain-wallets): walk the linking and management flow in a browser
The linking flow stacks three Turbo frame navigations — the provider drawer,
the linking modal on top of it, then the token review in that same modal —
and no controller test exercises any of that. Driving it in a real browser
found two things worth keeping:
The panel is reached two different ways. With nothing linked yet it is a card
under "Available" that opens the drawer; once a wallet exists the provider
moves to "Your connections", where the panel renders inline inside a
disclosure and there is no link to click at all. Only the first path had ever
been exercised.
Both paths now are, along with the parts of the flow that only exist in a
browser: assets arriving pre-ticked or not according to whether they can be
priced, review tokens reopening the selection with no address field present,
and disconnecting one asset leaving its account behind. One test per flow —
the branching cases stay in the controller test, where they cost a fraction
of the time.
Verified visually at each step: the warning banner, the Bitcoin
single-address note, the three-asset review with the spam token unticked, the
four management actions, and the accounts landing under Crypto with the wallet
subtype.
Full system suite green the documented way (DISABLE_PARALLELIZATION=true):
94 runs, 385 assertions, 0 failures.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): report a timed-out data source as unreachable
Reading a real Solana wallet turned up the hole: the public RPC timed out,
Net::ReadTimeout escaped every layer untranslated, and the user was told
something had gone wrong with Sure — the generic message reserved for our own
bugs — with a DebugLogEntry filed as if it were one. A public endpoint being
slow is the most ordinary failure this feature has.
All five clients now translate transport failures into their own ApiError:
timeouts, refused or reset connections, unreachable hosts, TLS errors, and a
response that is not JSON. The adapters already map a provider's own errors
onto Onchain::Chains::UnreachableError, so a timeout now reads as "the public
explorer could not be reached", the message that tells the user to retry,
while genuine bugs keep the generic one. During chain detection it goes back
to meaning "not detected here", so a slow explorer still cannot break linking.
The translated message carries only the error class, never the original
message, because a transport error's message contains the full URL and these
get logged.
Checked against the pre-fix behaviour on Bitcoin: with the translation
removed, the timeout test fails with the raw exception instead of the chain
error.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): name on-chain trades after the asset, not "shares"
Running a real Bitcoin address through the app showed every imported transfer
as "Buy 0.000003 shares of CRYPTO:BTC". That name comes from the shared trade
helper, which is written for equities; a wallet does not hold shares, and the
internal ticker is not what the user calls the coin. Trades are now named
"Buy 0.000003 BTC", through i18n like the display-only entries already were.
Also drops the sync status_text calls. Sync has no such attribute in this
schema, so every one of them was a guarded no-op and the four locale strings
they referenced could never render — dead weight copied from another
provider's syncer.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): keep balances when a source refuses history
Reading real wallets on the free Solana endpoint exposed two problems, one of
which I introduced.
The budget. Making history depth configurable scaled Solana's transaction cap
from 25 to 250, and on Solana one transaction is one RPC call rather than one
page — so the same nominal depth became an order of magnitude more expensive
and a sync went from seconds to minutes. The per-transaction budget is now its
own constant, scaled proportionally off the page knob so one setting still
moves both, with a test that pins it far below the paginated row count.
The bigger one: a failure while reading history threw away the balances too.
A balance is one bounded request and is what a wallet fundamentally is; history
is paginated, far more expensive, and the first thing a throttled endpoint
refuses. On the free Solana endpoint, which routinely throttles getTransaction,
that meant a wallet showed nothing at all rather than showing what it holds —
permanently, not transiently.
History is now best effort: when the data source refuses it, the balances are
still recorded and the history is marked incomplete, which the manage screen
already surfaces. Anything that is not the data source failing still raises, so
a bug here cannot be swallowed. Verified live: the same Solana wallet that
failed entirely now reports 52.06 SOL and its SPL tokens with the history
flagged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): pre-tick what is worth something, not what parses as a ticker
Linking a real airdropped address showed 200 of its 201 assets arriving
pre-ticked, one click away from 200 accounts — the opposite of what reviewing
tokens before import is for. The pre-tick rule was "the symbol looks like a
ticker", and airdrops use perfectly plausible short symbols (0XBTC, 4CHAN), so
it selected nearly everything.
Assets now carry whether the data source treats them as notable, and only those
are pre-ticked. Two attempts at that signal, decided by measuring the real
address rather than guessing:
- Blockscout's `reputation` is "ok" for all 6,669 tokens it holds. Useless.
- Market-cap presence looks strong on the full list (365 of 6,669) but is
useless after the cap, which already ranks by market cap — hence every
surfaced token having one.
- Holding value discriminates: of the 365 priced tokens, 112 are worth more
than a dollar.
So on EVM networks a token is notable when the indexer can price it and the
holding is worth more than a dollar; on Solana when the verified token list
vouches for the mint; the native coin always. Pre-ticked count on that address
drops from 200 to 72, and everything else stays one click away.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): warn when nothing can convert USD prices into the family currency
A family whose currency is not USD needs two settings, not one, and only the
first was covered. The single provider that prices bare crypto symbols quotes in
USD, so valuing a wallet in EUR needs an exchange rate on top — and Sure's
default exchange rate provider requires an API key, so a self-hosted install
without one has no FX at all.
Tested against a real address in a EUR family: every wallet came out at zero,
with 275 transfers recorded as unpriced, and nothing anywhere said why. That is
the same support ticket the crypto-provider banner exists to prevent, arriving
through the other door.
Onchain::Pricing answers "can an on-chain asset be valued in this currency, and
if not, why" with the two reasons separately. The linking UI states whichever
applies — naming the currency for the FX one, and pointing at Frankfurter, which
needs no key — and a USD family never sees that warning because it needs no
conversion. The processor records the reasons when a holding lands at zero, so
support sees "exchange_rate" rather than guessing.
Verified live afterwards: with EXCHANGE_RATE_PROVIDER=frankfurter, the same EUR
family values the wallet at 416,198,342.25 EUR at 0.86363 USD/EUR, with all 275
transfers priced in EUR.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): let a movement become a trade after being display-only
Syncing a real wallet twice — once before its prices were reachable, once after
— raised ArgumentError from the shared importer: an Entry cannot change
entryable type in place, and the display-only Transaction already held the
external_id the trade needed.
That is the ordinary case, not an edge one. A wallet linked before market data
covers its history gets display-only entries on the first sync, and the first
sync that can price them dies. Worse, it dies inside perform_sync, so the repair
pass that exists precisely to upgrade those entries — and which runs in
perform_post_sync, afterwards — was never reached. The account stayed stuck.
Both paths now go through one writer that discards a stale display-only entry
before the trade takes over its identity, so the entry keeps being the same
transfer rather than becoming a duplicate. Checked against the pre-fix
behaviour: without the discard, the new test raises the original ArgumentError.
Found by running a EUR family through two syncs on real data, which is the only
way the two price states occur in order.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): write a trade and drop its display-only entry atomically
Replacing a display-only entry with the trade it became is one change, but it
was two writes outside a transaction. A failure between them left the account
with neither: the transfer disappeared until some later sync happened to rewrite
it, which for an idle wallet could be never.
The repair pass already wrapped this; the sync path did not, and that is the one
that runs first.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): tell apart two transfers of one token in one transaction
A token transfer was identified by its transaction hash and contract, which are
not unique together: a swap router or a batch payout routinely emits several
transfers of the same token involving the same address within one transaction.
The second overwrote the first, so a transfer disappeared from the account
without a trace.
Both EVM backends report the log index — the field that makes an event unique
inside a transaction — and neither was using it. It is now part of the
identifier, with the contract kept only as a fallback for an instance that does
not report one.
Reported by review on #3081; confirmed against the live Blockscout payload,
which carries log_index. The test fails against the previous identifier.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): canonicalise an address per chain before anything keys off it
Addresses were only stripped, never canonicalised, and what counts as the same
address differs by chain — so the duplicate guard could be walked straight past
and one Bitcoin case was worse than a duplicate.
On EVM, hex is hex: 0xABC… and 0xabc… are one wallet, but they linked as two,
each with its own accounts and holdings for the same balance.
On Bitcoin, bech32 is case-insensitive and canonically lowercase, and the API
reports outputs that way. An uppercase bech32 address passed validation, gave a
correct balance from the address summary, and matched no output at all — so the
wallet silently had zero movements and no cost basis, with nothing anywhere
saying why.
Canonicalisation is now the adapter's answer, since only the chain knows whether
case carries identity: EVM and bech32 fold, Base58 and Solana are left exactly
as given. The controller applies it as soon as the chain is known and before the
duplicate check, and on an address change too.
Reported by review on #3081. Both tests fail without the folding.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): stop folding the case of Solana mints
Contract identifiers were downcased everywhere on the assumption that a contract
address is hex. That holds for ERC-20 but not for an SPL mint, which is a Base58
public key where case is part of the value: the stored mint was an unusable copy
of the real one, and two distinct mints could collide once folded into the same
string — one wallet's balance landing on the other's row.
Whether case carries identity is a property of the token kind, so it is now
answered in one place and applied consistently: by the asset's identity key, by
the column, and by the two comparisons in Onchain::Snapshot. A Movement no longer
folds anything on its own, because a movement does not know its asset's kind.
That also removes the duplication review flagged: the asset key was derived in
three places and only one of them folded, so the review screen and the linker
could disagree about what identifies an asset. There is one definition now,
OnchainWalletAccount#asset_key.
Reported by review on #3081. The test fails with the unconditional fold.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): isolate a failing address, and let a late symbol land
Two problems in the importer, both reported by review on #3081.
One unreachable address took the whole connection down with it: the loop over a
family's addresses had no rescue, so a Bitcoin explorer being throttled left an
untouched Ethereum wallet unsynced as well. Each address is now recorded and
skipped on its own — a row we failed to read keeps its previous quantity rather
than being zeroed, since we did not learn that it holds nothing — and only a
connection whose every address failed is reported as a failed sync rather than a
quiet success.
The content digest covered quantity and movements but not the metadata written
alongside them, so a Solana mint that later gained a real symbol from the token
list produced the same digest as before: the row was never rewritten and its
placeholder label became permanent. That silently undid the token-naming work.
The digest now covers everything the update writes.
Both tests fail against the previous behaviour.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* refactor(onchain-wallets): use DS::Button for the wallet actions
Five hand-built button_to controls carried their own utility-class strings for
what DS::Button already does — sync, disconnect a connection, stop tracking one
asset, disconnect a wallet, enable crypto prices. They drifted from the design
system on size, hover and destructive treatment, and the confirm prompts were
wired by hand.
The browser test walks the two that matter, so the behaviour is unchanged; this
is the styling and the confirm handling moving to the component that owns them.
Reported by review on #3081, along with the raw `bg-amber-600` on the provider
badge — left as it is, deliberately: all 23 other providers set a raw palette
class for their badge, there is no functional token for a brand colour, and
changing one entry would make it the only different one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): four smaller findings from the review on #3081
**A failed read no longer leaves an empty connection.** `link_wallet` created the
family's connection before fetching the address, so an explorer being down left a
connection with no wallets showing in the panel as connected. Reading needs no
saved connection — which is exactly why previewing uses an unsaved one — so it is
now created only once the read succeeds.
**Assets that could not be tracked are named instead of blamed on the user.**
`success?` is `created.positive?`, and the only failure message was "pick at least
one asset to track" — so a user who ticked three assets and hit three failed
creates was told to tick something, sending them back to tick the same three.
The linker already collected which assets failed; the message now says so, and a
partial failure is reported alongside what did get tracked instead of being
dropped silently. Same fix in the token revision action, which had the same shape.
**A malformed stored amount costs its movement, not the asset.** `BigDecimal()`
on a stored payload raises, and one unparseable row would fail the whole asset's
processing — including the repair pass. These payloads are written by this code,
so it takes a row that survived a format change, but the containment is two lines.
**No dead-end link.** The warning offered "open market data settings" to
everyone, while that page is gated to self-hosted admins — the same gate the
enable button already had. Both controls are now behind it, and the warning text
stands on its own for everyone else.
Each has a test that fails against the previous behaviour.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): reject non-finite amounts and quantities
The guard added an hour ago for malformed stored amounts was incomplete for the
very case it was written for: BigDecimal parses "NaN", "Infinity" and
"-Infinity", and none of them is zero, so all three sailed through
`parse_amount` into trade materialisation. Verified rather than assumed — the
test fails without the check with PG::NumericValueOutOfRange, so the infinity
reached the insert.
Fixed at both layers that write numbers. Movements: a non-finite amount skips its
movement, like any other unparseable one. Quantities: `normalize` treats
non-finite as unknown and writes zero, because Postgres numeric stores NaN
happily and one NaN quantity would turn every total that reads it into NaN.
Reported by review on #3081.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): derive the content digest from what actually gets written
Third time the digest missed a field it was supposed to cover: first the symbol,
so a Solana placeholder that gained a real name was never rewritten; now the
truncation flags, so a wallet whose history became complete — or started being
capped — kept showing the old completeness in the UI, since the early return
fires before `extra` is touched.
Rather than add a third field to a hand-picked list, the digest is now taken from
exactly the attribute hash that is about to be written. The two cannot drift
apart, which is what kept going wrong.
That needs two things to hold. Movements are sorted before the payload is built,
so the order a source happened to list them in is not mistaken for a change. And
the hash is canonicalised before hashing, because jsonb does not preserve key
order: `extra` written as {history, assets} comes back as {assets, history}, and
hashing that raw made an idle wallet's digest flip every other sync — the tests
caught it, and it is now covered by one asserting that reordered movements are
not a change.
Reported by review on #3081. Both halves checked against the pre-fix behaviour:
without the flags two tests fail, without the canonicalisation three.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs(onchain-wallets): state the pricing coverage limit, and that DeFi is unseen
The hosting guide explained how to configure prices but never said what the
crypto provider actually covers. It quotes by symbol, and a symbol is not a
token's identity — measured on a real Ethereum address, two of its ten largest
token positions were quoted and the other eight, including holdings worth
roughly $406k, $141k and $74k, showed zero with their quantities tracked
correctly.
That reads as a broken sync unless it is written down, so it is now the first
limitation in the list, with the practical rule a user needs: a zero next to a
token you know is worth something means the provider does not list it, not that
the balance is wrong. Troubleshooting gains the matching entry, separating it
from the two configuration causes that produce a zero across every wallet.
Also records that DeFi positions — staked ETH, LP tokens, lending, Solana stake
accounts — are invisible, which was missing from the list entirely. A wallet
holding most of its value in a staking protocol reports a fraction of it.
Docs only; no code touched, so the suite was not re-run — the last run on this
tree was green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): do not zero an asset the token cap never reached
A tracked asset missing from a snapshot was always read as "the wallet no
longer holds it" and set to zero. That is right for a complete read, and wrong
for a capped one: at most 200 tokens per address are surfaced, so a tracked
token can be absent simply because the read stopped before reaching it. Its
balance was then wiped — a real holding removed from the user's net worth, with
a holding of zero written and the account balance reduced to match.
It is reachable: on Solana the surfaced set is ordered by mint address, which is
arbitrary, so a genuinely held USDC position on an airdropped wallet can fall
outside the cap and be zeroed on the next sync.
Absence of evidence is not evidence of absence — the same distinction this code
already makes for an address it could not read, where the row keeps its previous
quantity. When the snapshot is capped and the asset is not in it, the row now
keeps what was last known and only its completeness flags are refreshed.
Reported by review on #3081. The test fails against the previous behaviour, and
the "an asset that disappeared is set to zero" case still passes: a complete read
that no longer lists an asset still zeroes it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* refactor(onchain-wallets): rename the local `token` variables the secret scan trips on
Pipelock failed the PR with eleven "Credential in URL (high)" findings, all on
the same shape: a local variable named `token` being assigned. `token = ...`
is what a leaked credential looks like to a scanner, and the rule is a
reasonable one to keep.
Renamed rather than excluded. Excluding paths or adding `# pipelock:ignore`
would have kept the pattern and blunted the check for everyone; the new names
are at least as clear — `token_data` for the raw hash from an indexer,
`metadata` for the resolved symbol/name pair, `token_asset` for an
Onchain::Asset in tests.
Verified with the scanner itself rather than by inference: the same pipelock
2.8.0 the workflow pins, run locally over the branch diff, now reports "No
secrets found in diff". It also caught one site the CI log had truncated away
(the system test), which is why the local run was worth setting up.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): bound detection latency, and three review findings
Detection runs on the request thread but asked each candidate chain with a
sync's patience: a 30s timeout, three retries and exponential backoff, per
chain. A 0x address is a candidate on six networks, so one rate-limited
explorer could hold the page for minutes. Detection now reads with its own
budget - one short attempt, no retry, ONCHAIN_DETECTION_TIMEOUT to raise it -
while syncs keep the patient one. A chain that cannot answer in time is
reported as "no activity", which is the screen an ambiguous answer already
produces.
Also from review:
- A full page of Solana signatures is now reported as incomplete history.
The adapter passes no cursor, so a page that fills means the read stopped
short of the address's history; counting only against the transaction
budget called that complete.
- A reused CRYPTO: security with no price provider is bound to the crypto
one. A blank provider falls back to whichever is enabled first, and only
the crypto provider quotes a bare coin symbol, so the holding valued at
zero and read as a broken sync. A provider another integration chose is
left alone: the CRYPTO: prefix is shared with the exchange integrations.
- The chain select's label reached the form builder as an HTML attribute
instead of a label option, so no <label> was associated with the field.
Each regression test was checked against the pre-fix code: the two detection
tests fail with four requests instead of one, and the truncation test fails
by reporting complete history.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): let the table refuse a token row with no contract
The partial unique indexes key a token on its contract address, and NULLs
are distinct to Postgres, so a token row that reached the table without one
would slip past its index and duplicate freely. The model already refuses
it, but a direct write does not go through the model, and this repo puts
simple guarantees like this in the database.
Added to the existing migration rather than a new one: the table is created
by this branch, so the constraint belongs with it, and this leaves the
schema version untouched.
The regression test writes with validate: false, which is how the sibling
tests prove a guarantee comes from the table rather than from the model
above it. Without the constraint it fails with "expected but nothing was
raised".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): detect a token-only Solana wallet, and pin asset_kind
Two findings from the review of the previous commits.
Detection read only the wallet's lamports, so a wallet emptied of SOL but
still holding SPL tokens answered "nothing here". Each token account carries
its own rent, so an empty wallet address is not an empty wallet. The token
accounts are asked only once the balance comes back zero, so the ordinary
case still costs the one request this probe is meant to be, and accounts
left behind empty do not count as activity.
The narrow blast radius is worth stating: has_activity? only runs when an
address matches more than one chain, and when no candidate answers the user
is asked to choose rather than turned away. So this was a worse screen, not
a rejected wallet.
Separately, the check constraint accepted any asset_kind that carried a
contract address. Each partial unique index names its kind, so a row with
any other one is keyed by nothing and duplicates freely. Adding a token kind
already means adding its index here, so pinning the three in the table adds
no coupling that the indexes did not already have.
Both regression tests fail on the previous code. The third test - emptied
token accounts are not activity - passes either way by design: it guards the
new branch rather than testing it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(onchain-wallets): cover the asset_kind constraint that shipped without it
The constraint landed in
|
||
|
|
4e010493c7 |
feat(up): map Up category slugs to Sure categories on import (#2487)
* feat(up): map Up category slugs to Sure categories on import UpEntry::Processor captured Up's category slug into extra but never applied it, so Up transactions imported uncategorised even though the user had already tagged them in the Up app. Add UpAccount::Transactions::CategoryTaxonomy + CategoryMatcher, mirroring PlaidAccount::Transactions::CategoryMatcher: map Up's child category slugs onto the family's existing/default Sure categories by alias, and wire the matcher through UpAccount::Transactions::Processor into UpEntry::Processor. The category is applied via the adapter's enrich_attribute, so a category the user has set or locked is preserved on re-sync. High-confidence mappings only. Up-specific categories with no honest Sure default (Booze, Pets, Apps & Games, Life Admin, Technology, ...) intentionally stay uncategorised for the user's own rules / AI, since a wrong auto-category is worse than none. Adds a matcher unit test and processor wiring tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(up): match category slugs as strings in CategoryMatcher Up category ids are string slugs; compare them against the taxonomy keys as strings so the lookup does not depend on the keys being symbols. No behaviour change (the "slug": hash syntax already produces symbol keys that matched the symbolized input, covered by the matcher unit test), but it removes a subtle footgun and reads clearer. Flagged by the Codex review on the PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(up): make category import non-destructive; word-boundary the alias match Per review feedback: do not bootstrap Sure's default categories during a sync. family_categories now returns the family's existing categories without creating defaults, so a family that has none (deliberately cleared, or pre-onboarding) gets uncategorised transactions rather than having the full default set silently created. Matching resumes once the user sets up categories through the normal UI flow. Also word-boundary the "and" stripping in the matcher normalization so it strips only the standalone conjunction, not "and" inside a word (e.g. errand). Adds a processor test for the non-destructive guarantee. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Gavin Matthews <matthews.gav@gmail.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Juan José MataandClaude
|
4d1d33f91d |
perf: memoize Family#balance_sheet and sync status lookups per request (#2553)
Family#balance_sheet built a new BalanceSheet on every call. The application layout renders the account sidebar twice per page (desktop + mobile) with three tab panels each, and each panel asks the family for its balance sheet - so the account, sync-status and exchange-rate queries behind it ran up to six times per request. - Memoize Family#balance_sheet per user id (Current.family is the same instance for the whole request, and jobs/controllers use short-lived Family objects, so staleness is not a concern). - Memoize BalanceSheet::SyncStatusMonitor#syncing_account_ids in the instance: it is called once per account row, and each call was a Rails.cache round-trip (or a full re-query with the test null store). Measured on the test suite probes: ReportsController#index view time 230ms -> 155ms, AccountsController#show 299ms -> 233ms. Claude-Session: https://claude.ai/code/session_01RpZe2ajeGkPRRBHfaJTfUB Co-authored-by: Claude <noreply@anthropic.com> |
||
|
super
|
1a5d04d527 |
feat(rules): add a Transaction tag condition filter (#2558)
* feat(rules): add a Transaction tag condition filter Rules could already set tags via the set_transaction_tags action but had no way to match transactions by an existing tag. Add a select-type transaction_tag condition filter (mirroring transaction_category) with the standard "Equal to" and "Is empty" operators, registered on the transaction rule resource so it surfaces in the rule builder automatically. Also remap the tag UUID<->name in family data export/import for the new condition, matching how the transaction_category/transaction_merchant conditions and the set_transaction_tags action are already handled, so tag-based rules survive an export/import round-trip. Closes #2557 * fix(rules): match transaction_tag via EXISTS so compound tag conditions work Addresses review feedback on #2558: the tag filter joined transactions.tags and predicated on tags.id, which broke two compound cases: - two ANDed tag conditions collapsed to `tags.id = a AND tags.id = b` on the same joined alias and could never match, even when the transaction had both tags; - OR / multi-tag matches returned a transaction once per tagging row, inflating counts and making rule actions iterate duplicate transactions. Use a correlated EXISTS subquery per condition instead. Each condition is independent (fixes the AND case) and no join is added, so rows are never multiplied (fixes the OR duplication) and prepare adds nothing, keeping branches structurally compatible inside a compound OR. Add tests for both. |
||
|
|
6439a731ab |
feat(self-hosting): surface Sidekiq-unhealthy nudge + admin system health page (#1906)
* feat(self-hosting): surface Sidekiq-unhealthy nudge + admin system health page (#1481) When the Sidekiq worker container isn't running — the most common Docker Compose misconfiguration in self-hosted setups — every background job silently never executes. Balance calculations, net-worth updates, and account syncs stall. The UI shows zeros and "No balance data available for this date" without explaining why (#1481, #1047). Per jjmata's resolution on the issue, this PR ships both halves of the fix in one pass: 1. A user-facing nudge banner that appears on every authenticated page when Sidekiq isn't processing jobs. Tells the user their data may be stale; doesn't pretend zeros are real. 2. An admin-only deep link from that banner into a new `/settings/admin/system_health` page (super-admin gated, matching the existing admin namespace contract) showing live Sidekiq state: process count, last heartbeat, max queue latency, job counters, and per-queue depth. ## What changed - New `SidekiqHealth` PORO (`app/models/sidekiq_health.rb`) eagerly loads ProcessSet + Queue + Stats in one pass and exposes `healthy?` plus a stable `reason` symbol (`:redis_unreachable`, `:no_worker_processes`, `:stale_heartbeat`, `:queue_backed_up`). Any Redis/Sidekiq failure during the eager load is caught and surfaced as `:redis_unreachable` so a degraded broker never crashes the layout. - `ApplicationController#current_sidekiq_health` memoizes a single instance per request via `helper_method` so the layout, banner partial, and any controller checks share one Redis round-trip. - New `app/views/shared/_sidekiq_health_banner.html.erb` rendered from `_htmldoc.html.erb` when `Current.user` is present and the health check is failing. Banner shows the user-facing message to everyone; the "View system health" CTA + reason detail are gated on `Current.user&.super_admin?`. - New `Admin::SystemHealthController#show` (inherits the existing `Admin::BaseController`, so super-admin gating is enforced for free) + view rendering status, counters, and per-queue breakdown. - Routes: `resource :system_health, only: :show` inside the existing `namespace :admin`. - Settings nav: new "System health" entry under the Advanced section, gated on `super_admin?` to match `sso_providers_label` and `users_label`. - i18n: new `shared.sidekiq_health_banner.*` keys (title, body, CTA, per-reason explanations) and a full `admin.system_health.show.*` namespace for the new admin page. English-only, matching how `ds.pill.*` and other DS keys are scoped. ## Why - jjmata: "Let's take both approaches ... a nudge about 'data unavailable' which hyperlinks to the admin UI if you are an admin only (not for other types of users) sounds like the best path forward. **Any takers for the PR?**" (#1481) - smurfpandey: "We can add a section in Settings for superadmins to see 'health' of the application/host." - The detection signal is conservative on purpose: - `PROCESS_HEARTBEAT_TIMEOUT = 2.minutes` tolerates deploy restarts and brief Redis blips without flapping. - `LATENCY_THRESHOLD = 5.minutes` is well above the sync-job tail under default `config/sidekiq.yml` concurrency. ## Validation This worktree runs on Windows without a local Ruby toolchain, so I could not run `bin/rubocop`, `bundle exec erb_lint`, `bin/brakeman`, or `bin/rails test` locally. CI will run the full matrix on the PR: - `lint` — `bin/rubocop -f github` - `lint_js` — `npm run lint` (no JS touched, should be green) - `scan_ruby` — `bin/brakeman --no-pager` - `scan_js` — `bin/importmap audit` - `test_unit` — `bin/rails test` (includes 7 new tests under `test/models/sidekiq_health_test.rb` and 4 new under `test/controllers/admin/system_health_controller_test.rb`) - `test_system` — `DISABLE_PARALLELIZATION=true bin/rails test:system` - `pipelock` — secret + agent-security diff scan Manual checks done in this worktree: - Re-read `CONTRIBUTING.md` and `.cursor/rules/project-conventions.mdc`. PORO under `app/models/` per Convention 2. No new gem dependency per Convention 1. Banner uses semantic tokens (`bg-warning/10`, `text-warning`) per the design-system rules. No `lucide_icon` direct call — uses the `icon` helper per CLAUDE.md. - Confirmed `Sidekiq::ProcessSet` / `Sidekiq::Queue` / `Sidekiq::Stats` are the same APIs Sidekiq 7+ exposes (we're on Sidekiq 8.x per the `Gemfile.lock` comment in `config/initializers/sidekiq.rb`). - Tests stub `Sidekiq::ProcessSet.new` / `Sidekiq::Queue.all` / `Sidekiq::Stats.new` so the suite doesn't need Redis populated. - The admin route lives inside the existing `namespace :admin` so `Admin::BaseController#require_super_admin!` enforces auth — no new authorization surface added. ## Notes - No public API endpoints, no rswag specs, no OpenAPI changes. - No migrations, no model changes outside the new PORO. - No background jobs touched. - English-only locale entry, mirroring the `ds.*` / `admin.invitations.*` precedent in this repo. Other locales fall back to English. - Detection thresholds are constants on `SidekiqHealth` so they're easy to tune from a follow-up PR if the defaults turn out to flap on any real-world deployment. - The banner positions itself at `top-20` (below the impersonation / super-admin bars) and uses `z-40` (below the `z-50` notification tray). Single-screen overlap with mobile flash toasts is acceptable for V1. Refs: #1481, #1047 * fix(self-hosting): address review on Sidekiq health PR (#1481) - `Admin::SystemHealthController#show` now reads from the request-memoized `current_sidekiq_health` instead of building a fresh `SidekiqHealth.new`, so the controller and the layout banner share one Redis round-trip. - `SidekiqHealth#reason` now treats `last_heartbeat_at.nil?` the same as a stale beat: a registered process that hasn't published a heartbeat is not "healthy". Previously the check short-circuited on the nil guard and silently fell through to the queue-latency branch. Added a unit test covering the `ProcessSet` entry with `"beat" => nil` case. - Settings nav: switched the "System health" entry's icon from `activity` to `heart-pulse` so it no longer duplicates the LLM Usage icon. - Routes: dropped the redundant `controller: "system_health"` option from the `resource :system_health` declaration — Rails infers `Admin::SystemHealthController` from the namespace, matching the style of the sibling `:sso_providers`, `:users`, `:invitations`, and `:families` admin resources. * fix(self-hosting): scope + cache Sidekiq health, admin-only banner (#1481) Addresses the second round of maintainer review on the Sidekiq health PR. - Skip the check entirely in managed mode. `current_sidekiq_health` returns `nil` unless `Rails.application.config.app_mode.self_hosted?`, so authenticated requests in managed deployments add zero Redis round-trips for this feature. - Cache the snapshot across requests via `SidekiqHealth.current` (Rails.cache, TTL `CACHE_TTL` = 60s default, env-overridable). The per-request memoization on `ApplicationController` is preserved on top, so even back-to-back self-hosted pages share one fetch. - Make thresholds operator-tunable. `PROCESS_HEARTBEAT_TIMEOUT`, `LATENCY_THRESHOLD`, and the new `CACHE_TTL` read from `SIDEKIQ_HEALTH_HEARTBEAT_TIMEOUT`, `SIDEKIQ_HEALTH_LATENCY_THRESHOLD`, and `SIDEKIQ_HEALTH_CACHE_TTL` env vars (seconds), with the previous values as defaults. Comments now explain the tuning rationale. - Gate the banner on `Current.user&.super_admin?` at the layout level rather than rendering a vague warning to family members who can't act on it. The partial no longer carries an internal admin check since the call site does it; non-admins see nothing. - Replace the hard-coded `top-20` offset with a computed offset based on which impersonation bars are visible (`top-4` / `top-20` / `top-36`) so the banner doesn't collide with the super-admin or approval bars when both are stacked above it. - `Admin::SystemHealthController#show` now bypasses the cache (`SidekiqHealth.expire_cache!` + `SidekiqHealth.new`) so an operator who just restarted the worker sees fresh state instead of a stale 60-second snapshot. Also lets the page render in managed mode where `current_sidekiq_health` is nil. - Tests: add coverage for `.current` cache reuse and `.expire_cache!` forcing a re-query, swapping `Rails.cache` to a MemoryStore since the test env defaults to `:null_store`. * fix(self-hosting): route singular resource + drop assert_same on cached snapshot (#1481) Two CI failures surfaced once the full pipeline ran on this branch for the first time (it was gated on contributor approval until d04b78e): - Admin system-health controller tests returned 404. Singular `resource :system_health` in `config/routes.rb` makes Rails infer `Admin::SystemHealthsController` (it pluralizes the controller name even for singular resources), but the controller file is named `system_health_controller.rb` / `Admin::SystemHealthController`. Restore the explicit `controller: "system_health"` override that the previous "address review" commit dropped on the (mistaken) premise that Rails would infer it from the namespace — the sibling admin routes all use plural `resources` so they round-trip cleanly, this one doesn't. Comment now spells the gotcha out so the next reviewer doesn't try to "simplify" it again. - `SidekiqHealthTest#test_current_memoizes_across_calls_inside_the_cache_TTL` used `assert_same` on the two returns from `SidekiqHealth.current`. `ActiveSupport::Cache::MemoryStore` defaults to `dup_values: true` and Marshals on read, so a cache hit returns an `==`-equal but `equal?`-different instance. Replace the identity check with the behavioral assertion we actually care about: re-stub `ProcessSet` to raise on the second call, then assert the second `current` return is still healthy (proving Redis was not re-queried). * fix(i18n): drop redundant inline default on system_health nav label (#1481) `system_health_label` is already defined in config/locales/views/settings/en.yml, so the inline `default: "System health"` was a hard-coded English string in the template (DS Drift Patrol Rule 5). Use the bare locale lookup like the sibling nav entries. --------- Co-authored-by: John Baillie <johnbaillie2007@gmail.com> Co-authored-by: Khaostica <256858950+Khaostica@users.noreply.github.com> |
||
|
|
5a2bf02b13 |
fix(simplefin): stop repair_stale_linkages from hijacking a live linkage on a same-name twin (#3116)
* fix(simplefin): stop repair_stale_linkages from hijacking a live linkage on a same-name twin repair_stale_linkages matched purely on case-insensitive display name, so two distinct upstream accounts sharing a name (e.g. two "CHECKING (0001)" accounts at the same institution) caused the unlinked twin to silently steal the linked account's AccountProvider, merge in its transactions, and overwrite its balance on every subsequent sync. Thread the upstream account_id set already computed during account discovery through to repair_stale_linkages so it only treats a linked account as stale when its account_id is actually absent upstream, and skip ambiguous multi-way name matches instead of picking the first one. Fixes #2852 * fix(simplefin): clear stale upstream_account_ids and log skipped repairs - Clear simplefin_item.upstream_account_ids at the start of each perform_account_discovery run so a later discovery that finds zero accounts can't reuse IDs from a prior run on the same SimplefinItem instance (CodeRabbit review finding). - Capture skipped stale-linkage repairs via DebugLogEntry so operators can see them in /settings/debug, not just the raw Rails log (Codex review finding). - Fix two pre-existing SimplefinAccount::Transactions::ProcessorInvestmentTest tests broken by the new upstream_account_ids nil-guard: they called process_accounts directly without going through the Importer, so they now set upstream_account_ids explicitly to simulate a legitimate "old account_id genuinely absent upstream" repair. - Add regression coverage for both fixes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Gerald <248542187+gfr-free@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
|
b0ecb919b0 |
fix(lunchflow): refresh stored transaction on pending to posted (#2778)
* fix(lunchflow): refresh stored transaction on pending to posted LunchflowItem::Importer keyed stored raw transactions but treated them as immutable snapshots. When Lunchflow flipped a transaction from pending to posted under a stable ID, fetch_and_store_transactions skipped it as a duplicate and kept the stale pending snapshot. The processor then re-imported it with isPending:true, so ProviderImportAdapter#import_transaction never reached its pending-clearing branch and the entry stayed stuck with a "Pending" badge. Index stored transactions by key (the Lunchflow ID, or a content hash for the blank IDs Lunchflow returns for some pendings) and refresh the stored snapshot in place when the upstream payload actually changed, while still deduplicating by key to prevent unbounded growth. Fixes #2735 * fix(lunchflow): preserve identical same-response blank-ID transactions The stored snapshot was keyed with a Hash of key -> transaction, so two rows in one sync response that share a content hash (Lunchflow returns blank IDs for some pending transactions, and two genuinely distinct identical purchases hash the same) collapsed into a single entry. The second row hit the existing-key branch as though it were a duplicate, dropping a real transaction before LunchflowEntry::Processor could apply its collision suffix. Pool the existing snapshot into one bucket per key and match incoming rows one-for-one (shift), so same-response collisions each claim their own slot or count as new. This preserves every real transaction while still deduplicating across syncs (re-syncing the same pair stays at two, not four) and refreshing pending -> posted transitions. Adds a regression test for two identical blank-ID rows in the same response. --------- Co-authored-by: agentloop <agentloop@localhost> Co-authored-by: pro3958 <pro3958@users.noreply.github.com> |
||
|
|
29c0a369d0 |
fix(transfers): isolate concurrent transfer match in a savepoint (#2769)
* fix(transfers): isolate concurrent transfer match in a savepoint auto_match_transfers! opens one Transfer.transaction and, per candidate, calls Transfer.find_or_create_by! while rescuing RecordNotUnique. The transfers table has a composite unique index on (inflow_transaction_id, outflow_transaction_id), so when two syncs of the same family run at once the losing insert raises the unique violation. On PostgreSQL a failed statement aborts the whole surrounding transaction. Rescuing the Ruby exception does not clear that state, so the next update! raises PG::InFailedSqlTransaction and every following candidate is dropped. Run the per-candidate insert in its own savepoint via Transfer.transaction(requires_new: true), extracted into a private find_or_create_transfer! helper. A lost race now rolls back only to the savepoint; the outer transaction stays healthy and the loop keeps matching. The same race surfacing through the uniqueness validation (RecordInvalid with :taken) is treated as already-created; any other validation failure is re-raised. Fixes #2471 * fix(transfers): only swallow the uniqueness race for the exact pair The rescue treated a :taken on inflow_transaction_id or outflow_transaction_id as proof that this candidate's transfer was created. But the uniqueness validations are per-column, so two same-amount candidates racing (one committing (inflow, outflow_a) while another tries (inflow, outflow_b)) raise :taken on inflow_transaction_id even though no Transfer exists for (inflow, outflow_b). The caller then marked outflow_b as matched with no Transfer behind it. Confirm the exact (inflow_transaction_id, outflow_transaction_id) row exists before accepting the race; otherwise return nil and skip the candidate. Non-:taken validation failures still re-raise. The RecordNotUnique path (composite index) already implies the exact pair — it now returns that row for the same reason. * test(transfers): assert matching continues past a skipped collision The concurrent-race test had no surviving candidate, so a regression that stopped processing after the skipped collision would still pass. Add a second, non-conflicting candidate and assert its transfer is created and both entries are marked. * test(transfers): pass insert! attributes as an explicit hash Ruby 3 treats insert!(inflow_transaction_id: ..., outflow_transaction_id: ...) as keyword arguments, so ActiveRecord's insert!(attributes) got zero positional args and raised ArgumentError (given 0, expected 1). Wrap the attributes in { } so they are the positional attributes hash. --------- Co-authored-by: agentloop <agentloop@localhost> Co-authored-by: pro3958 <pro3958@users.noreply.github.com> |
||
|
|
d57c4301f2 |
fix(import): tolerate null Rule names and orphaned rejected transfers (#2775)
* fix(import): tolerate null Rule names and orphaned rejected transfers Importing a full all.ndjson export aborted on data that is actually valid. The preflight listed name as a required field for Rule, but rules.name is nullable and the model allows it, so a single rule with "name": null blocked the entire import. Separately, a RejectedTransfer whose referenced transaction had been deleted raised a hard missing_reference error in preflight and a MissingReferenceError in strict mode, even though the importer already had a skip path for it. Require Rule.id instead of Rule.name in preflight, matching the field the importer actually needs. Treat RejectedTransfer references as advisory: a missing referenced transaction becomes a warning, and the importer resolves the references with required: false so the orphaned row is skipped and counted instead of raising. Fixes #2721 * fix(import): keep SureImport preflight warnings as strings at the API boundary The orphaned-RejectedTransfer path emits warnings as {code, message} hashes via add_warning, but the published OpenAPI contract documents /api/v1/imports/preflight warnings as strings. sure_import_preflight_payload copied them through unchanged, so the endpoint returned a heterogeneous array once that path was exercised and contract-generated clients could fail to deserialize. Map warnings to their human-readable message at the API boundary so the array stays homogeneous strings, matching the documented schema. The internal {code, message} shape is unchanged. Adds a regression test. * i18n(import): localize missing-reference preflight messages The missing-reference warning and error are user-facing (returned in Result#payload[:warnings]/[:errors]) but were hard-coded. Move the full templates to config/locales/models/sure_import/preflight/en.yml and interpolate line, type, field, and value, per the i18n coding guideline. Warning key and error behavior are unchanged, and the rendered text is identical. --------- Co-authored-by: agentloop <agentloop@localhost> Co-authored-by: pro3958 <pro3958@users.noreply.github.com> |
||
|
GFRandGerald
|
07ce130405 |
fix: respect SURE_IMPORT_MAX_NDJSON_SIZE_MB in Sure import GUI upload (#3111)
The GUI upload paths (imports_controller#create_sure_import and Import::UploadsController#update_sure_import_upload) checked file size against the hardcoded SureImport::MAX_NDJSON_SIZE constant instead of SureImport.max_ndjson_size, so self-hosted admins raising SURE_IMPORT_MAX_NDJSON_SIZE_MB had no effect on the GUI — only the API upload paths honored it. Removes the now-unused constant. Fixes #3010. Co-authored-by: Gerald <248542187+gfr-free@users.noreply.github.com> |
||
|
|
f0a0013da9 |
fix(charts): round series values to the currency's display precision (#3091)
* fix(charts): round chart values to the currency's display precision Charts are drawn from the serialized amounts, not from the formatted strings, so a sub-unit residue plotted as a visible move between two points that both read $0.00, and the trend between them reported a change. Round in `Series#as_json` so the series values stay exact for insights, goals and the assistant. Also hide the percentage when the previous value is zero, since that makes it infinite. * fix(charts): round the two payloads that bypass Series#as_json `NetWorthBreakdownSeriesBuilder` builds its payload by hand, so the reports chart never went through the rounding added in `Series#as_json` and still plotted raw amounts: adjacent points printing the same value rendered a visible move, and the tooltip it inherits reported a change between them. `Series#trend` had the same gap on the server side. It is rendered right above the chart by `UI::Account::Chart` and by the reports summary, so an account going from 0 to a sub-cent residue showed a coloured $0.00 change next to a flat line. Rounding the trend can make a previously finite percentage infinite, so guard the three views that render `percent_formatted` without checking, as `shared/_trend_change` already does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(charts): address net worth review comments * fix(charts): tighten rounded trend handling * fix(reports): restore positive sign in print trend --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: sure-admin <sure-admin@splashblot.com> |
||
|
Abhinav Dhiman
|
aafefab65a |
fix(process-pdf-job): discard on RuntimeError and guard failed status (#2453)
* fix(process-pdf-job): discard on RuntimeError and guard failed status Add discard_on(RuntimeError) to drop the job immediately instead of exhausting 25 Sidekiq retries on deterministic errors. The block logs job_id and message for observability. Widen the early-return guard from status == "complete" to also cover "failed", preventing re-processing of already-failed imports. * fix(process-pdf-job): discard on Provider::Error instead of RuntimeError * fix(process-pdf-job): log error class name instead of message in discard handler |
||
|
GFRandGerald
|
fa8769f792 |
fix: support DD/MM/YY date format for CSV imports (#3110)
Adds "DD/MM/YY" as a CSV-only date format for transaction and account balance imports, addressing #1530. Kept out of Family::DATE_FORMATS (the global date preference) since a prior PR (#531) adding it there was rejected by a maintainer: 2-digit years are ambiguous (Ruby's %y assumes 1969-2068) and could silently misparse historical or future-dated transactions. Restricting it to Import::CSV_ONLY_DATE_FORMATS keeps it available where the user can see and verify a parsed preview against their own CSV data, per the maintainer's suggested approach in that PR's discussion. Co-authored-by: Gerald <248542187+gfr-free@users.noreply.github.com> |
||
|
Juan José MataandClaude Opus 5
|
8ca65ffd27 |
Reconcile PDF statement imports against transactions that already exist (#3105)
* Reset to main, keeping only the account matcher improvements Backs out the LLM-driven reconciliation work (PR #1382's approach and the two commits hardening it). That approach compares whole-statement aggregates, which is all-or-nothing: when 18 of 20 transactions have already synced the totals disagree, the import proceeds, and 18 duplicates are created. Reconciliation is a row-level problem and belongs in the import path, where TransactionImport already solves it via Account::ProviderImportAdapter. Kept from that work, because it stands on its own: - AccountMatcher gains a hint-based class-level entry point so callers without an AccountStatement row can score against the same rules. The instance path used by AccountStatement#assign_account_match is unchanged. - It also refuses to guess between equally-confident candidates rather than letting max_by take whichever the scan reached first. Account names are not unique within a family, so that tie was reachable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KvWSJotDiqP6qSR7qEiC8Q * Reconcile PDF statement imports against transactions that already exist Reconciliation is a row-level problem. Comparing whole-statement totals is all-or-nothing: when 18 of 20 transactions have already synced the totals disagree, the import proceeds, and 18 duplicates are created. This matches each extracted transaction against what the account already holds, so only genuinely new transactions are ever offered for import and the rest are marked reconciled. TransactionImport#import! already does this for CSV via Account::ProviderImportAdapter. PdfImport#import! called it zero times and built one Transaction per row unconditionally -- the only import path in the repo with no duplicate protection. Reconciliation state follows Quicken's uncleared / cleared / reconciled, but only the last state is stored: - "Cleared" means the institution acknowledged the transaction, which is exactly what entries.source and entries.external_id already record. It stays accurate on its own, because the adapter stamps both onto a manual entry when a provider transaction claims it, so a hand-entered transaction that later appears in a download becomes cleared with no extra bookkeeping. Deriving it also keeps it non-editable, which is right: it is a fact about provenance, not an opinion. - "Reconciled" means a statement was matched against the transaction. Nothing can derive that, so entries gains reconciled_at and reconciled_by_statement_id. It is a judgement, so it can be set and unset, and it survives the statement being deleted (the FK nullifies, the timestamp stays). Matching: - find_duplicate_transaction grows include_provider_entries, which is what makes this work for Provider-backed accounts -- the existing where(external_id: nil) filter hid synced transactions from every import path, so this gap affected CSV imports equally. Default stays false: provider sync must not claim another provider's entry. - It also grows date_window, because a statement's posting date routinely differs by a day or two from the date a provider recorded. Nearest date wins. - Name is deliberately not matched on: statement descriptions and provider names for the same transaction rarely agree. The adapter makes the same choice for sync. - Candidates are built as real Import::Row objects so matching uses the same signed_amount and date_iso the import itself would write, rather than a second interpretation of signage that could drift. - Matching is per-account, so with no account assigned every row is offered and re-judged on assignment; reassigning also releases the previous account's reconciliations. - A row whose date or amount will not parse is offered for import rather than dropped, so nothing disappears silently. - import! re-checks at publish, since a sync can land between review and publish, and new transactions are born reconciled: the statement is their evidence. Provider-backed accounts are now offered in the import target picker. The manual-only restriction existed because importing into a synced account would duplicate what sync brought in, which is precisely what this removes. Also fixes a bug this uncovered on main: extract_transactions stored the extractor's symbol-keyed hash, while every reader digs with strings. jsonb keeps the hash as assigned until reload and ProcessPdfJob never reloads, so has_extracted_transactions? was false and PDF imports generated zero rows. The existing tests missed it -- one uses a YAML fixture, the other stubs the extractor with string keys. Supersedes #1382. Refs #1379. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KvWSJotDiqP6qSR7qEiC8Q * Fix three review findings in statement reconciliation All three confirmed against the code before fixing. Publish-time recheck consumed the same entry twice. import! started its exclusion list empty, so a statement carrying two same-amount transactions against an account holding only one would re-match the surviving row against the entry row generation had already consumed -- silently dropping a genuinely new transaction instead of creating it. Seed the exclusions with the entries this statement already reconciled; newly synced entries are still caught, since only already-reconciled ones are excluded. A regeneration that emptied the row set left the import stranded. In the normal upload flow the account is assigned after extraction, so assign_account! regenerates -- and if everything then reconciled, rows_count went to zero while status stayed pending. _pdf_import.html.erb renders pending-with-no-rows as the processing screen, and process_with_ai_later cannot restart because ai_processed? is already true, so the import was stuck with no way forward. Status now follows the same rule ProcessPdfJob applies after initial processing, in both directions: no rows completes it, rows returning sends it back to pending. Guarded by data_committed? so a published import is never reopened. Unevaluatable rows went only to the Rails log. AGENTS.md asks for DebugLogEntry.capture on recoverable import failures so they surface in /settings/debug with structured context. Capture family, account, import, statement, row number and the raw date/amount that would not parse. Adds regression coverage for each. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KvWSJotDiqP6qSR7qEiC8Q * Fix row regeneration collision and over-broad reconciliation release test_unit caught one error in 6462 tests, and it was real. Row regeneration collided on the second call. insert_all! bypasses ActiveRecord, so the rows association is never populated with what it wrote. Calling generate_rows_from_extracted_data twice on the same in-memory record -- which assign_account! now does after ProcessPdfJob has already generated once -- made rows.destroy_all clear a stale empty collection, delete nothing, and then collide on (import_id, source_row_number). Reload before destroying, and reset the association after inserting so sync_mappings and the view read what was actually written. Releasing reconciliations was scoped to the statement, not the account. A statement is evidence for exactly one account at a time but can back more than one import, so reassigning an account cleared reconciliations another account still relied on. Scoped to the account being moved away from; a blank scope releases nothing, which is correct because nothing is reconciled while no account is assigned. The second finding was raised by CodeRabbit. Its other flagged risk -- entries being marked reconciled before the import is published -- is deliberate and stays: the statement is the evidence, reconciling is reversible via unmark_reconciled!, and deferring it to publish would leave a fully reconciled statement with nothing to publish and therefore nothing ever marked. Adds regression coverage for both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KvWSJotDiqP6qSR7qEiC8Q * Address review nitpicks: model validation, scope placement, mock style, lock-safe DDL All four checked against the repo's own conventions before applying. Mirror the check constraint as a model validation. Assigning reconciled_by_statement without reconciled_at raised StatementInvalid rather than a validation error. CLAUDE.md Convention 5 asks for exactly this pairing -- the constraint in the database, an ActiveRecord validation for form-friendly errors. Stop hijacking the pending-scope comment. The reconciliation scopes were inserted directly under "Pending transaction scopes", so that header read as documentation for them and the provider note below read as a continuation of reconciled_by. Given the reconciliation scopes their own header. Use OpenStruct for the provider response double, per "Always prefer OpenStruct when creating mock instances". Verified OpenStruct.new(success?: true) responds to success?, and ostruct is already a dependency used elsewhere in test/. Make the migration lock-safe. entries is the largest table in the app: both indexes now build concurrently, and the check constraint is added unvalidated then validated separately so VALIDATE takes only SHARE UPDATE EXCLUSIVE instead of holding ACCESS EXCLUSIVE for a full scan. This follows existing practice -- 13 migrations already use disable_ddl_transaction! and 11 use algorithm: :concurrently, with add_offline_reason_to_securities combining add_column and a concurrent index in one migration exactly like this. The suggested follow-up migration for validation was not needed: validating in the same non- transactional migration gets the same lock behavior without a second file, and the repo has no validate: false precedent in 400 migrations. schema.rb is unchanged: a validated constraint and a concurrently-built index dump identically. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KvWSJotDiqP6qSR7qEiC8Q * Add the two review-requested regression tests Covers the invalid-amount half of "malformed rows are offered, not dropped" -- the existing test only exercised an unparseable date. Asserts the raw value is stored verbatim rather than coerced to 0, so the review step shows the user what the statement actually said. Also covers the Entry validation added in |
||
|
GFRandGerald
|
ecf00f8a0f |
fix(enable_banking): recognize N26's PERIOD_INVALID error shape as a retryable period rejection (#3112)
N26 (via Enable Banking) rejects an out-of-range transaction period with
{"code": "PERIOD_INVALID", "detail": "dateFrom=...,dateTo=..."} instead of
the {"error": "WRONG_TRANSACTIONS_PERIOD"} shape the retry ladder from
#2992 already handles. wrong_transactions_period? never matched, so the
sync failed outright instead of retrying with a shorter window. Also
guard corrected_date_from against a non-hash detail payload, which would
otherwise raise NoMethodError for this ASPSP's plain-string detail.
Fixes #1262
Co-authored-by: Gerald <248542187+gfr-free@users.noreply.github.com>
|
||
|
|
9d225a21e6 |
fix: disable Mark as Recurring button when a manual recurring transaction already exists (#3103)
* fix: disable "Mark as Recurring" button when a manual recurring transaction already exists Previously the button was always clickable and only failed after a POST, showing "A manual recurring transaction already exists for this pattern". Extract the lookup into Transaction#existing_manual_recurring_transaction (reused by the controller guard) so the view can disable the button ahead of time and show the reason inline. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: address CodeRabbit review feedback on PR #3103 Move the existing_manual_recurring lookup out of the show view and into the controller so rendering no longer runs an Active Record query in-template, and strengthen the "no match" model test with near-match recurring transactions that individually differ by account, merchant, amount, currency, and manual flag. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: move mark-recurring presentation state fully into controller, fix stale state on failed update Address CodeRabbit follow-up on PR #3103: - Compute the mark-recurring button's subtitle text/class, href, disabled state, title, and class entirely in TransactionsController (via a shared assign_mark_recurring_state helper) instead of deriving them with ternaries in the view. - Populate that state before TransactionsController#update re-renders :show on a failed entry update, so the button doesn't incorrectly appear enabled when a matching manual recurring transaction exists. - Add a controller test covering the failed-update render path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: use blank name instead of blank date to trigger validation failure in mark-recurring test The CI test_unit run flagged a real bug in the test itself: TransactionsController#entry_params strips blank :date/:amount before update, so date: "" never reached model validation and the update succeeded (302) instead of failing (422) as the test expected. Use a blank :name instead, which isn't stripped, and add DOM assertions (disabled button, no mark_as_recurring form action) per CodeRabbit's follow-up review. Verified against a live NAS Rails console reproduction (bypassing the test stack's broken fixtures) that the failed-update render now correctly shows the button as disabled with the "already exists" message and no action link. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: use transaction id instead of entry id in mark-recurring route assertion mark_as_recurring is a member route on the transactions resource keyed by the Transaction's id, not the Entry's id (Entry uses delegated_type, so Entry and its Transaction entryable have distinct ids). The prior assertion built the path from `entry`, which could produce a different URL than the one actually rendered, so the "no href" check could pass even if the button leaked a link. Use entry.entryable so the assertion matches the real route. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: refresh mark-recurring state on turbo_stream update, avoid unconditional query, keep DS::Button href Addresses jjmata's review on PR #3103: - Extract the "Mark as Recurring" block into a dom_id-wrapped partial and replace it in the successful update turbo_stream response, so inline edits that change whether the transaction matches an existing manual recurring transaction are reflected immediately instead of only on the next full page render. - Skip the existing_manual_recurring_transaction lookup entirely when the block won't be rendered (no edit permission, or split-child entry), avoiding an unconditional extra query on every transaction show/failed update. - Keep href present on the DS::Button and only toggle disabled, matching the established pattern elsewhere in the app, instead of nulling href (which flips the component to a bare <button> and leaks a stray method="post" attribute). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Gerald <248542187+gfr-free@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
Brandon
|
25a9011f14 |
feat(ai): analytical tool-set upgrade for the builtin assistant (#3064)
* fix(assistant): survive tool failures with error and hint results instead of aborting the turn
A tool exception used to raise FunctionExecutionError out of the responder
loop, turning the whole turn into a generic chat error banner. An unknown
tool name was worse: the rescue block itself crashed (fn.name on nil).
Tool failures now come back to the model as data ({error, hint}) so the
conversation survives and the model can retry once with corrected
arguments. The catch-all branch logs and tells the model not to retry.
FunctionExecutionError remains defined for API compatibility.
* fix(assistant): strict schemas declare every property as required
get_categories and get_tags declared an optional page property while
inheriting strict mode, which is invalid under strict function calling
(every property must be listed in required). Both now opt out of strict
mode like every other paginated tool, and gain a page_size param
(1..100) while being touched.
A registry-walking test asserts the invariant for every current and
future tool, preview tools included.
* fix(assistant): HistoryTrimmer always keeps the newest turn
Trimming iterates newest-first and stopped at the first group over
budget. When the newest group alone exceeded the budget, everything was
dropped, including the user message the model was being asked to
answer, and the provider received only the system prompt. The newest
group now always survives.
* perf(assistant): compact AI time series and make account history opt-in
get_accounts shipped a 5-year monthly series for every account on every
call, roughly 60 formatted money strings per account, which dominates
the tool payload for multi-account families and swamps small
self-hosted context windows. The series is now opt-in
(include_balance_series) and bounded by a named period (series_period,
default last_365_days).
to_ai_time_series states the currency once and emits numeric values
instead of formatting every point; the system prompt already tells the
model how to render currency.
get_accounts also now returns account ids (they are what other tools
accept as account_ids filters) and respects the visible scope, so
hidden accounts no longer leak into responses.
* refactor(assistant): id and name filters replace user-data enums in get_transactions
The schema inlined every account, category, merchant, and tag name as
enum values on every request. That grows without bound with family
data, defeats provider prompt caching (definitions change whenever a
name does), and is the pattern that made empty-enum pruning necessary
in the first place.
Filters are now plain string arrays documented as exact names from the
sibling get_* tools, which Transaction::Search already resolves
server-side, plus an account_ids UUID filter. New params: page_size
(1..100), sort_by amount, types (income/expense/transfer, the way to
exclude transfers), and statuses (pending/confirmed).
The three now-unused enum helpers are removed from the base class;
family_tag_names stays for update_tag, which still identifies tags by
name.
* feat(assistant): add get_merchants and get_recurring_transactions
Merchants were unreachable: names appeared nowhere and
update_transaction's merchant_id had no source of ids, making it
unusable. get_merchants lists id, exact name, and source, scoped
through available_merchants_for so merchants seen only in accounts
hidden from the user never leak.
Recurring transactions had a model, an Upcoming view, and no assistant
reach. get_recurring_transactions lists detected and manual recurring
items (status filter defaulting to active, optional
upcoming_within_days window) with per-currency totals of active
non-transfer items, answering subscription and upcoming-bill questions
directly instead of via transaction paging.
* feat(assistant): flexible periods on get_balance_sheet and trends on get_income_statement
get_balance_sheet was hard-wired to five years of monthly history with
no parameters, although Period supports arbitrary ranges and the chart
builder takes any interval. It now accepts a named period or custom
dates plus an interval, with a 400-point cap so a day-granularity
request over a decade returns an error instead of a giant series. The
default call is byte-compatible with the old shape. The balance sheet
object is also memoized; it was being constructed four times per call.
get_income_statement gains the analysis surface the assistant lacked:
group_by month for a monthly income/expenses/net series (capped at 36
buckets), compare_previous_period for an equal-length prior window
with absolute and percent deltas, and account_ids to scope totals to
specific accounts via IncomeStatement#totals_for. Category breakdowns
are family-wide by construction, so the account-filtered view omits
them and says why. Unknown or inaccessible account ids come back as a
soft failure naming the ids so the model can correct itself.
* feat(assistant): preview reads for insights and valuations
The Insights feed is generated nightly with pre-computed numbers, and
the chat assistant could not read a word of it. get_insights returns
the visible feed (type filter, acknowledged toggle, limit) without
marking anything read; an assistant read is not the user viewing the
feed. It sits in PREVIEW_FUNCTION_CLASSES because the feature itself is
preview-gated, which also keeps it off the default /mcp surface.
record_valuation was write-only: an agent recording provenance-cited
valuations had no way to audit what it wrote or find dates already
carrying a value. get_valuations lists valuation entries newest first
with kind and the citation notes, scoped to accessible visible
accounts.
* feat(assistant): cache-stable system prompt with session context
The prompt interpolated currency mid-text and the date near the end, so
no two requests shared a cacheable prefix, and it told the model
nothing about the family: not one account name, not a single category.
Models opened most conversations blind, either wandering through tools
or answering without data.
The prompt is now STATIC_INSTRUCTIONS, a frozen constant that is
byte-identical for every request (providers discount an
exactly-repeated prefix; tool definitions are also stable now that
schemas carry no user data), followed by a trailing Session context
block holding everything volatile: date, date format, currency details,
an account roster with balances, and category names.
The static half gains a request-classification rule (CHAT / LOOKUP /
ANALYSIS), a reuse-what-you-have rule with an explicit re-fetch
carve-out, specific-tool preference, and the error/hint retry-once
rule that pairs with the tool soft-fail contract.
Context stays cheap by construction: the roster collapses to per-type
counts beyond 25 accounts, categories to a count beyond 60 names, and
both collapse whenever the configured context window is under 4096
(the self-hosted default is 2048), via the new Assistant::TokenBudget
helper. Intro chats are untouched.
* feat(assistant): raise tool-round cap to 8 with a no-tools grace turn; instructions-aware history budget
Five rounds was tight for a tool surface that now supports real
analysis chains, and hitting the cap raised ToolCallLimitError, which
surfaced to the user as a dead chat with an error banner. The default
is now eight rounds (env override unchanged), and on the final
permitted round the follow-up request offers no tools, so the model
must answer in text with whatever it gathered. The limit error remains
as a defensive backstop.
The generic-path history budget reserved a flat 256 tokens for a
system prompt that already estimates well past that; the trimmer now
budgets against the actual instructions when available.
LLM_MAX_RESPONSE_TOKENS was reserved in budget math but never sent to
the provider. It is now sent (max_tokens on chat completions,
max_output_tokens on the Responses API) only when explicitly
configured via ENV or a stored Setting; stock installs keep today's
uncapped behavior.
* test(evals): chat golden v2 exercising the real prompt and registry
The eval runner scored a fiction: hardcoded instructions and four fake
permissive tool schemas, so a prompt or registry regression could
sail through green. It now runs STATIC_INSTRUCTIONS plus a fixed
synthetic session context and builds definitions from
Assistant.function_classes against a reference user (classes whose
schema cannot build are skipped with a log line, never faked).
chat_golden_v2 adds routing scenarios the upgrade cares about: CHAT
classification must use no tools, aggregates route to
get_income_statement / get_balance_sheet rather than transaction
paging, and the new analytical tools are selected with sensible
params. The dataset header documents the harness's single-shot
limitation.
* docs(ai,mcp): current tool tables, responder loop, prompt structure, timeout math
Both docs listed 7 tools against a registry of 19, in three separate
drift-prone copies. mcp.md now carries the canonical tables (default +
preview); ai.md links to them from the MCP section, keeps one grouped
functions list for the architecture chapter, and replaces its stale
hardcoded registry snippet with a pointer to assistant.rb.
The architecture section gains the contracts contributors need when
adding a function: the responder loop (rounds vs calls, cap 8, the
no-tools grace turn) and the error/hint soft-failure convention, plus
the prompt's static/session-context split and its collapse gates.
Timeout guidance is recomputed for the new default cap.
* fix(assistant): address automated review findings
Codex and CodeRabbit findings on the initial push, all verified before
changing anything:
- AI time series rounded every value to two decimals, which turns
0.001 BTC into 0.0; values now round to the currency's own precision
(BTC 8, CLF 4, OMR 3).
- get_income_statement validated account_ids against all visible
accounts, but totals_for excludes hidden, excluded-from-reports and
tax-advantaged accounts, so those ids produced silent zeros. Ids now
validate against income_statement.eligible_accounts and the soft
failure explains eligibility.
- get_recurring_transactions computed totals from the displayed rows,
so past the 200-row cap the value labeled a total was partial. Totals
now aggregate over the full filtered scope in SQL, and the response
carries total_results and a truncated flag. The upcoming_within_days
window also starts at today, matching its documentation; overdue
items appear in unwindowed calls.
- get_valuations silently dropped a malformed date filter and presented
unfiltered data as filtered; malformed dates now return invalid_date.
- get_balance_sheet returned a generic failure for a reversed custom
range because Period's own validation raises past the Date::Error
rescue; it now returns the structured invalid_date error.
- get_insights documents that its family-wide scope matches the web
feed exactly (InsightsController serves Current.family.insights to
every member), so the tool exposes nothing the /insights page does
not already show the same user.
- Tests: limit clamp proven against more insights than the cap,
Setting fallbacks stubbed in the provider budget tests, currency
precision and reversed-range regression tests added.
* refactor(assistant): apply reviewer nitpicks
- order declares type alongside its enum, matching sort_by
- page-size clamp deduplicated into the base class (MAX_PAGE_SIZE +
shared resolved_page_size); dead per-tool copies removed
- get_accounts preloads balance rows only when the series is requested
- get_income_statement validates the bucket count before running any
aggregation work
Deliberately unchanged: the balance sheet's monthly_history key. The
default response shape stays byte-compatible for existing MCP
consumers, and the nested series already states its interval.
* fix(assistant): second-round review findings on get_valuations
- A reversed date range (start after end) now returns the structured
invalid_date error instead of presenting an empty result as filtered
data, matching get_balance_sheet's handling.
- Page numbers are normalized before pagination: Pagy raises on zero,
negative or non-numeric pages. The fix lands as a shared
resolved_page helper on the base class and applies to every
paginated tool (categories, tags, merchants, transactions, holdings,
valuations), since all shared the same page-or-1 pattern; schemas
declare minimum: 1.
* fix(assistant): round series amounts as BigDecimal before Float conversion
Converting to Float first can perturb the value at the requested
precision; round the exact decimal, then convert for JSON.
* fix(assistant): address maintainer review findings
- get_accounts no longer fails the whole listing when one account's
start date lies beyond the requested period (start_date derives from
the first entry, which can be future-dated); that account simply has
no series. The unrescued Period.custom was reachable exactly there.
- The balances preload is gone: the series goes through
Balance::ChartSeriesBuilder, which runs its own query keyed by
account ids, so the eager-loaded rows were loaded and discarded.
- Provider::Openai#context_window now delegates to
Assistant::TokenBudget, removing the duplicated ENV > Setting >
default precedence so prompt assembly and the provider can never
disagree about the window.
* fix(ai): final no-tools round uses tool_choice none instead of dropping tools
Anthropic rejects requests whose messages contain tool_use blocks when
no tools are defined, so re-requesting with an empty tool list made the
final-round grace die in a provider 400 on Anthropic models. The final
round now sends the real tool definitions with tool_choice none, which
both providers accept, and the model answers in prose as intended.
* fix(assistant): scope every income statement read to the requesting user
get_income_statement validated account_ids against the user-scoped statement
but computed every total from an unscoped one. IncomeStatement falls back to
Current.user, which is nil in the assistant job and the MCP endpoint, so the
unscoped reads dropped the included_in_finances_for filter and reported
family-wide totals next to ids that had been checked against a narrower set.
Route all reads through one memoized user-scoped statement, the idiom
get_balance_sheet already uses. Also lets the per-instance memoization in
IncomeStatement apply across the eligibility check and the totals.
Adds a regression test that fails without the change, plus a companion test
asserting eligibility and totals agree on scope. Guard the strictness walk
against an empty registry so it cannot silently assert nothing.
|
||
|
Ellion BlessanandClaude Sonnet 5
|
e69894adb9 |
fix(providers): capture API error response body in PDF processor span output (#2937)
* fix(providers): capture API error response body in PDF processor span output Anthropic and OpenAI PDF processing errors only logged the exception message, dropping the parsed response body that usually explains the failure. Add safe_error_body to both providers' UsageRecorder concerns and include it in the langfuse span output on failure, with tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013SYp89xEYfTkrxw8HcCUQ8 * fix(providers): allowlist PDF processor error fields sent to Langfuse safe_error_body forwarded the entire upstream error body into the langfuse span output. For custom OpenAI-compatible providers/proxies (and the analogous Anthropic path), that body can echo request content from the financial document being processed. Replace it with safe_error_detail, which extracts only type/message/code/request_id instead of the raw body. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013SYp89xEYfTkrxw8HcCUQ8 * test(openai): cover request_id extraction in PDF processor error_detail The safe_error_detail request_id path (error.response_headers) had no test coverage. Stub response_headers with x-request-id and assert it appears in error_detail. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013SYp89xEYfTkrxw8HcCUQ8 --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
Abhinav Dhiman
|
e3d46021c2 |
fix: memory leak in sidekiq (#1940)
* perf(sync): reduce per-job memory peak in Balance/Holding materialization
Profiling of SyncJob (StackProf object mode + Sidekiq memory middleware)
showed peaks of 600k-1.1M live heap slots per job and ~196k retained
ActiveModel::Attribute::FromUser objects post-GC, driven by full-history
in-memory accumulation in the balance/holding sync pipeline.
Changes:
- Replace Holding.new / Balance.new in calculators with lightweight
Struct-based HoldingData / BalanceData. Skips AR attribute sets,
belongs_to proxies, dirty tracking, type casting, and callbacks
that were never used (upsert_all bypasses validations/callbacks
anyway). Eliminates ~30% of allocations and the bulk of retained
ActiveModel::Attribute::* instances.
- Build upsert payloads directly from struct fields instead of
Holding/Balance#attributes.slice(...).
- Batch upsert_all in PERSIST_BATCH_SIZE (2,000) slices in both
Balance::Materializer and Holding::Materializer so the intermediate
attribute-hash array is bounded instead of holding the full
multi-year history alongside the calculator output.
- Replace account.holdings.reload with account.holdings.reset in
Holding::Materializer. Same cache invalidation, no eager re-query;
the next consumer (Balance::SyncCache) loads on demand.
- Mutate entries in place in Balance::SyncCache#converted_entries
instead of Entry#dup. The instances are scoped to the throwaway
sync-cache and never persisted, so dup'ing was producing tens of
thousands of unused FromUser/FromDatabase attribute wrappers per
sync.
All persist paths run inside the existing Balance.transaction wrapper,
so batched upserts retain transactional atomicity. No production caller
of Balance::SyncCache or Holding::Materializer reuses the affected
instances outside the materializer's lifetime.
Test coverage: balance/{sync_cache,materializer,forward_calculator,
reverse_calculator} and holding/{materializer,forward_calculator,
reverse_calculator} plus account/syncer and sync (82 runs, 2,548
assertions, 0 failures).
* refactor(holding): stream materializer upserts to bound peak memory
Replace full-array accumulation + each_slice in Materializer#persist_holdings
with two flush-on-fill buffers (holdings_buffer_to_upsert_with_cost /
holdings_buffer_to_upsert_without_cost) that upsert and clear at
PERSIST_BATCH_SIZE, keeping peak RSS bounded to ~2x batch size.
Also add assert_not_nil guards in ReverseCalculatorTest before
dereferencing calculated.find results to surface clear failures
instead of NoMethodError.
* refactor(balance): promote BalanceData to Balance namespace and document sync mutation safety
- Extract Balance::BalanceData struct into its own file (app/models/balance/balance_data.rb)
so it is discoverable without knowing it lived inside BaseCalculator
- Remove inline Struct definition from Balance::BaseCalculator; update build_balance
to reference Balance::BalanceData explicitly (required because class Foo::Bar syntax
does not nest Foo in constant lookup)
- Add comment to SyncCache#converted_entries clarifying that to_a materialises
independent AR instances with no identity map active, making in-place mutation safe
- Update all Balance::BaseCalculator::BalanceData references in materializer_test
* test(balance): use to_h instead of attributes on BalanceData struct in waypoint test
* perf(balance,market_data): replace sort_by + first/last with minmax_by and push account-entry join to SQL
- Balance::Materializer#purge_stale_balances: replace sort_by(&:date) + first/last with minmax_by(&:date) to avoid full sort when only min/max are needed
- MarketDataImporter: replace Entry.group(:account_id).minimum(:date) (which loads all account IDs into a Hash) with a LEFT JOIN subquery that computes MIN(date) per account in SQL and exposes it as first_entry_date on the Account relation
* test(balance): use BalanceData struct in materializer purge test
* fix(holding): carry cost_basis forward onto gap-filled dates
|
||
|
Atlasandsure-admin
|
eb382b8899 |
Fix onboarding country and currency defaults (#3070)
* fix: derive onboarding currency from country * feat: use browser locale for onboarding defaults * Fix onboarding currency defaults * Preserve saved onboarding currency during hydration --------- Co-authored-by: sure-admin <sure-admin@splashblot.com> |
||
|
|
fd01a5b3dc |
fix(accounts): remember transaction page size across account navigation (#3084)
* fix(accounts): remember transaction page size across account navigation per_page was only ever read from the current request's query string, so switching accounts always reset the activity feed back to 10 entries. Reuses TransactionsController's existing prev_transaction_page_params session store so the chosen page size applies consistently on both the account detail page and the global transactions page. Closes #3082 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(accounts): validate stored per_page and preserve it across filtered requests Addresses review feedback on #3084: safe_per_page now validates the stored default against the allowed values (a raw stored value like "1" was previously passed through unchecked), TransactionsController no longer wipes the remembered per_page when a request supplies filters but omits per_page, and Session#prev_transaction_page_params normalizes a NULL value to an empty hash. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(transactions): use stored per_page as pagy fallback on filtered requests store_params! already preserved the previously-selected per_page in the session when a filtered request (e.g. dashboard money-flow links) omitted it, but TransactionsController#index still called safe_per_page with its hardcoded default of 10, so the stored preference was never applied to the actual page rendered. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Gerald <248542187+gfr-free@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
sentry[bot]andsentry[bot] <39604003+sentry[bot]@users.noreply.github.com>
|
3c299a76f0 |
fix(demo): Scope budget auto-fill to family entries to prevent FK violations (#3078)
Co-authored-by: sentry[bot] <39604003+sentry[bot]@users.noreply.github.com> |
||
|
|
e5750a6c09 |
feat(budgets): add per-user personal budgets with strict isolation (#2891)
* feat(budgets): add per-user personal budgets with strict isolation Families can now opt into personal budgets (toggleable via family settings): each family member gets their own budget for a given period instead of sharing a single family-wide budget. - Add families.personal_budgets flag and budgets.user_id, with partial unique indexes so shared budgets (user_id IS NULL) and personal budgets (user_id IS NOT NULL) can't collide. - Budget.find_or_bootstrap scopes lookup/creation by user when the family has personal_budgets enabled. - Scope most_recent_initialized_budget (used to seed a new budget from the prior period) by user_id so one user's copy-forward never bleeds into another user's budget. - budgets.user_id cascades on user deletion so personal budgets don't outlive their owner. * feat(budgets): enforce user-specific budget ownership and cascade deletion * feat(budgets): display user name for personal budgets in budget card on the plan section * feat(budgets): enhance personal budgets display for admins with preview feature indication * feat(budgets): enforce user-specific budget and category visibility for personal budgets * feat(budgets): create budget section titles and add translations notice in preferences * feat(budgets): let household and personal budgets coexist with sharing Previously enabling personal_budgets made the shared household budget unreachable. Budget.find_or_bootstrap now takes an explicit household: flag so both can be resolved independently for the same period, with a new household_budget_enabled family setting to opt out of the household side and keep personal budgets only. Adds a BudgetShare model (read_only/read_write) so a member can grant another family member access to their personal budget, enforced via Budget#viewable_by?/editable_by? across BudgetsController, BudgetCategoriesController, PlansController, and the read-only API. Preferences gains a Budget sharing card (gated on preview access like the rest of the personal budgets UI) and an owner switcher pill ( Household / mine / shared-with-me) appears on the budget page and the Plan hub card. Also fixes personal budgets showing the same "actual spending" as the household budget: actual spending/income now scope to the budget owner's own accounts instead of the viewer's full accessible set, via a new accounts: override on IncomeStatement. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * feat(budgets): enhance budget switcher with icons and improved styling * feat(budgets): remove user name display from budget card and header * feat(budgets): remove unique index on taggable_type and taggable_id in taggings * feat(budgets): enhance budget sharing functionality and improve UI elements * Collapse personal budget migrations --------- Signed-off-by: JulienGourmet <69808509+jubbakka@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: sure-admin <sure-admin@splashblot.com> |
||
|
Scott HughesandClaude Opus 5
|
5bba880e66 |
Fix SnapTrade holdings by using the /positions/all endpoint (#3043)
* Fix SnapTrade holdings by using the /positions/all endpoint SnapTrade returns HTTP 410 Gone on /positions, /holdings and /options for apps registered after their 2026 cutoff, so newly connected accounts import their balance but never any holdings. The importer rescues the error and the sync still reports success, which makes it look like the brokerage simply holds nothing. Switch get_positions to the documented replacement, /positions/all, and teach the shared payload helpers to read its flat `instrument` object alongside the legacy nested `symbol.symbol` shape. Fixes #3029 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Filter unsupported instrument kinds before they reach the payload Derivatives skipped by HoldingsProcessor still landed in raw_holdings_payload, where calculate_holdings_value sums units * price over every entry. Any non-zero sum then displaces SnapTrade's own figure in calculate_total_balance, so an options-only account could report a balance derived from per-contract units against a per-share price. Move the denylist to Provider::Snaptrade#get_positions so unsupported kinds never enter the payload at all, keeping holdings, balance, currency detection and cash-equivalent handling consistent with one filter. Also raise when the positions response carries no results array, so a partial or schema-changed response leaves the previous snapshot in place instead of overwriting it with nothing. An empty results array remains a legitimately empty account. Note that tax_lots[].cost_basis is documented as a whole-lot total, unlike the per-share field this reads. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Sure Admin (bot)
|
b0edf99262 | Use moniker interpolation for user family labels (#3060) | ||
|
GFRandClaude Sonnet 5
|
d0bb1a31e8 |
fix(recurring): include amount in manual recurring duplicate check (#2972)
* fix(recurring): include amount in manual recurring duplicate check TransactionsController#mark_as_recurring blocked a second manual recurring transaction whenever an existing one shared the same account + payee name/merchant + currency, even when the amount differed -- stricter than the DB unique indexes (idx_recurring_txns_acct_name / idx_recurring_txns_acct_merchant), RecurringTransaction::Identifier's own grouping key, and the equivalent check already used in TransfersController#mark_as_recurring. Add amount to the duplicate lookup so two distinct recurring payments to the same payee at different amounts are both allowed, while an exact duplicate is still blocked. Also rescue ActiveRecord::RecordNotUnique around the create call so a race between the pre-check and the DB constraint (e.g. a double-submit) surfaces the same friendly "already exists" message instead of a generic error, mirroring the existing race-handling pattern in RecurringTransaction::Identifier. Fixes #2936 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(recurring): don't blend distinct charge amounts into variance band Once two manual recurring rows with the same payee/different amounts can coexist (this PR), RecurringTransaction.create_from_transaction's variance-band discovery still matched historical entries only by account/payee/currency/day-window -- never by amount -- so it could blend genuinely unrelated charges (e.g. a fee + a due from the same merchant, same day) into one row's expected_amount_min/max/avg. Flagged by Codex review on this PR. Confirmed this is not hypothetical: two real production transactions (3.00 and 19.68, same merchant, same day) got blended into a single recurring row showing a fabricated "11.34" projected amount that matches neither real transaction. The same unfiltered matching independently exists in RecurringTransaction::Identifier#manual_recurring_matches_entry?, which periodically re-derives every manual recurring row's variance after each sync (via IdentifyRecurringTransactionsJob). Both call sites needed the fix together, or the job would silently re-blend amounts on the next sync. Add RecurringTransaction.amount_within_variance_band?(candidate, anchor, ratio: 2) -- a candidate only counts as "the same fluctuating payment" if it's within 2x (double/half) of the anchor. Anchored on the target amount (not pairwise) so unrelated charges can't chain together; ratio-based (not %-of-target-with-floor) so it's scale-invariant and handles signed (expense) amounts correctly. Threshold checked against real data: existing variance test fixtures sit at ~1.2-1.3x (must stay included), the real corrupted case sits at ~6.6x (must be excluded) -- 2x leaves comfortable margin on both sides. Wire this into find_matching_transaction_entries/ find_matching_transaction_amounts (SQL-level filter, same pattern as the existing day-of-month bounds) and into manual_recurring_matches_entry?. amount_window_scope/ matching_transactions and create_from_transfer need no changes -- confirmed by reading: the former only consumes an already-computed band, the latter never does variance discovery at all. Does not touch any already-corrupted production data -- deliberately out of scope, discussed separately. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
Guillem Arias FausteandJuan José Mata
|
acf4cb2010 |
fix(goals): allow deleting a goal without archiving it first (#2963)
* fix(goals): allow deleting a goal without archiving it first
Goals could only be deleted after being archived. `GoalsController#destroy`
redirected with "Archive the goal before deleting it." unless the goal was
already archived, and the Delete item in the show-page kebab was wrapped in
`if @goal.archived?`. Nothing in the archive confirm copy hinted that
archiving was the prerequisite, so in practice an active goal had no delete
affordance anywhere in the UI.
The gate bought no safety. Destroying a goal cascades only to its own
`goal_accounts` and `goal_pledges`, and `GoalPledge#clear_matched_transaction_extra`
unstamps `extra["goal"]["pledge_id"]` from any transaction a matched pledge
claimed. No account, balance, entry or transaction is touched. Every other
resource in Sure (accounts, categories, rules, family merchants) deletes in
one step.
Drop the gate, render Delete unconditionally, and shorten the label from
"Delete permanently" to "Delete" now that it no longer needs to contrast
with an archive-first step.
The confirm copy moves to `Goal#deletion_confirm` and spells out what
survives. The generic `CustomConfirm.for_resource_deletion` only says "This
is not reversible", which overstates it for a goal.
Index cards deliberately keep no actions — the card stays a single click
target, and the show-page kebab is one click away.
* fix(goals): escape the goal name in the delete confirmation
`confirm_dialog_controller` assigns the confirm `body` to `innerHTML` — bodies
such as the accounts' `confirm_body_html` legitimately carry markup — so a goal
named "<img src=x onerror=…>" ran as soon as a family member opened the delete
confirmation. Verified in a browser: parsing the rendered `data-turbo-confirm`
and assigning its body produced a live `<img>` element with a working `onerror`
handler.
Escape the interpolated name. Only `body` needs it; the dialog sets its title
and button label with `textContent`.
`CustomConfirm.for_resource_deletion` interpolates a record name into the same
HTML-rendered body and was already reachable from accounts, categories, rules
and family merchants, so it is escaped here too rather than left as a known
hole next to the fixed one.
Also add the three `confirm_delete_*` keys to every locale that ships goal
translations. Fallbacks meant these silently rendered English rather than
breaking, so this is untranslated copy rather than a fault — ru is included,
which the review list omitted.
* i18n(confirm): move the resource-deletion copy to locale keys
`for_resource_deletion` built its title, body and button label as English
string interpolation, against the project's rule that user-facing strings go
through `t()`. It backs ~39 call sites — accounts, rules, tags, chats, every
provider item — so all of them were English-only.
Moved to `shared.custom_confirm.resource_deletion_*`, alongside the
`default_*` keys the same class already used.
`titleize` / `downcase` stay applied to the record name so the English output
is byte-identical to what the hardcoded strings produced; a locale needing
different casing can absorb it in its own string. Pinned by a test, along with
the escaping of the one field the dialog renders as HTML.
* i18n(confirm): translate the resource-deletion copy
The keys added when this copy moved out of hardcoded English only landed in
en.yml, leaving ~40 call sites falling back to English in every other locale.
Added to the eight other shared locale files that already carry the sibling
`custom_confirm.default_*` strings: ca, fr, hu, it, ru, tr, vi, zh-CN. Each
body reuses that locale's own "this is not reversible" sentence, so the
generic and resource-specific confirmations read the same, and each follows
the register its `default_title` already set (vous / siz / Вы, tu for ca).
The remaining shared locale files (de, es, nb, nl, pl, pt-BR, ro, zh-TW) have
no `custom_confirm` block at all, so they are left alone — adding one would
invent structure they have not adopted, and fallbacks already cover them. The
test derives its locale list from which files define the sibling key rather
than hardcoding it, so it follows that set as it grows.
* test(goals): restore the active-goal destroy test lost in the merge
Merging main into this branch hit a conflict in
`test/controllers/goals_controller_test.rb`: main had added two tests
immediately above the destroy block, and the resolution took main's side
wholesale for that hunk. That resurrected `destroy on non-archived is
rejected` — the test this PR replaces — and dropped its replacement.
The resurrected test failed against the new controller, since destroy no
longer gates on `archived?`:
GoalsControllerTest#test_destroy_on_non-archived_is_rejected
`Goal.count` didn't change by 0, but by -1.
Swap it back for `destroy deletes an active goal and cascades to its
links and pledges`. Main's two new tests stay.
* i18n(goals): finish the delete copy in de and zh-TW
Nine locales ship goals translations, not seven. `de.yml` and `zh-TW.yml`
were left behind: both still carried the dead `goals.destroy.archive_first`
key, still labelled the kebab item "Delete permanently" (Endgültig löschen
/ 永久刪除) after it was shortened elsewhere, and had none of the
`confirm_delete_*` keys, so a German or Traditional Chinese family saw the
new delete dialog in English.
Add the three confirm keys using each file's existing vocabulary — Zusagen
for pledges in German (informal du, matching the rest of the file), 投入 in
Traditional Chinese — drop `archive_first`, and shorten the label.
`confirm_delete copy resolves in every locale that ships goal translations`
could not have caught this. It hardcoded the seven locales, and its
assertions went through plain `I18n.t`: the backend has
I18n::Backend::Fallbacks mixed in, so a missing German key resolved to the
English string and `.present?` passed anyway. Verified — deleting
`confirm_delete_title` from `de.yml` left the test green.
Derive the locale list from the goals YAMLs and look the keys up with
`fallback: false, default: nil`. The same deletion now fails with
"de is missing goals.show.confirm_delete_title".
---------
Signed-off-by: Juan José Mata <juanjo.mata@gmail.com>
Co-authored-by: Juan José Mata <juanjo.mata@gmail.com>
|
||
|
Juan José MataandClaude Opus 5
|
da483746e2 |
Add comprehensive debug logging to AI cache reset job (#3046)
* Trace "Reset AI cache" runs in the debug log The /rules "Reset AI cache" button fired a background job whose only output was Rails.logger, so there was no way to tell from the app whether a reset ran, partially failed, or never started. Every stage now writes to DebugLogEntry under the new "ai_cache_reset" category, so a whole run is filterable in /settings/debug: - info when the request is enqueued from the rules page, and info again when the job starts (a request with no matching start means the job never reached a worker) - error when a scope fails outright, or when the enqueue itself fails - warn (capped at 5 per scope) for individual records that could not be cleared, plus warn when the job is handed no family - info on completion with the number of AI cache entries removed, broken down by scope, with failures and skipped records in the metadata The completion count needed fixing to be worth reporting: the class-level Enrichable.clear_ai_cache counted records visited, not cache entries removed, so it reported every transaction in the family regardless of whether anything was cleared. It now sums the enrichments actually deleted, and takes an optional block so a single unclearable record warns and is counted instead of aborting the sweep and discarding the tally of everything already cleared. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YWwaCxjhSxBpvvgNLmCw67 * Treat a false perform_later result as an enqueue failure perform_later turns an ActiveJob::EnqueueError — or an enqueue aborted by a callback — into a false return rather than raising it, so the previous rescue-only check missed those cases entirely: the controller logged the reset as requested and redirected with a success notice while nothing had been queued, which is exactly the blind spot this branch set out to close. Branch on the return value and raise the job's own enqueue_error when it carries one, so both failure modes route through the same error entry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YWwaCxjhSxBpvvgNLmCw67 * Cover the yielded enqueue_error path and assert the job argument The false-return test stubs perform_later without yielding, so it only exercised the fallback error. The branch that re-raises the job's own enqueue_error — the one that carries the adapter's underlying cause into the debug entry, which is the point of surfacing it at all — had no coverage. Add a test that yields a job carrying an EnqueueError and asserts the cause reaches both the raised error and the entry metadata. Also assert the family is what gets enqueued, in all three tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YWwaCxjhSxBpvvgNLmCw67 * Scope the enqueue rescue to the enqueue The rescue reports "could not be enqueued", but it also covered the request log that runs after the job is safely queued. That was harmless in practice — DebugLogEntry.capture rescues internally and returns nil, so it cannot raise — but the guarantee rested on the internals of a different class rather than on the shape of this method. Split the enqueue into its own method so the rescue covers only what it reports on. Nothing after a successful enqueue can now be recorded as an enqueue failure and retried, regardless of what those later steps call. No behavior change on any of the four paths already covered by tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YWwaCxjhSxBpvvgNLmCw67 --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Sure Admin (bot)
|
75aa16e4e2 |
Log async rule run failures to debug log (#3045)
* Log async rule run failures to debug log * Propagate auto-categorize provider failures |
||
|
Andrew B
|
c9fbfd9f71 |
fix(chat): make the assistant response timeout configurable (#2910)
* fix(chat): make the assistant response timeout configurable (#2893)
Self-hosted users running a local model report the chat failing with
"assistant not available" after 90 seconds even though the model
generates a reply and tokens are billed.
Three timeouts are involved and only one was configurable:
- OPENAI_REQUEST_TIMEOUT (60s) — already settable, not the blocker.
- The browser watchdog in chat_controller.js (90s) — hardcoded, and
this is what actually fires.
- Chat::UNDELIVERED_RESPONSE_TIMEOUT (60s) — a constant, so raising
the client value alone would not have helped.
The watchdog cannot be avoided by streaming here: custom
OpenAI-compatible providers route through generic_chat_response, which
forces synchronous calls, so nothing renders until the whole generation
finishes. Time-to-last-token has to beat the deadline.
Adds AI_RESPONSE_TIMEOUT (ENV > Setting > 90s default, floored at 30s),
exposed on the Self-Hosting settings page and passed to the Stimulus
controller at all three mount points — show, index and the sidebar in
the application layout, each of which declares data-controller="chat"
independently.
The server floor is derived from the same value but kept 10s below it.
report_timeout answers 200 whether or not it acted and the client only
retries on a non-ok response, so a floor at or above the client value
would let clock skew strand a pending bubble permanently.
Also guards AssistantMessage#append_text!. The watchdog runs in the web
process while the job holds its own copy of the message, so a job
finishing after the bubble was destroyed or demoted would silently
resurrect it alongside the error the user was already shown.
* fix(chat): let the watchdog retry when report_timeout declines
`report_timeout` answered 200 whether or not `handle_undelivered_response!`
acted. The Stimulus watchdog only stops retrying a URL once it sees a 2xx, so
a declined report was treated as final.
That stranded the bubble whenever the client's clock ran more than
SERVER_TIMEOUT_GRACE ahead of the server's: the watchdog posts at its own
timeout, the server sees a message younger than its floor and no-ops, and
nothing ever retries. The bubble spins forever with no error and no Retry.
Answering 409 instead lets the next 5s tick try again, so any amount of skew
costs retries rather than a stuck chat. The grace window stays as an
optimisation to keep those retries rare, not as the correctness mechanism.
* docs(chat): correct the AI_RESPONSE_TIMEOUT ordering guidance
The docs, locale string and examples all said to keep OPENAI_REQUEST_TIMEOUT at
or above AI_RESPONSE_TIMEOUT. That is backwards.
The two limits span different things. OPENAI_REQUEST_TIMEOUT bounds each HTTP
call to the model on its own; AI_RESPONSE_TIMEOUT covers the whole turn and its
clock starts when the message is queued, so it also absorbs Sidekiq queue time
and, for a tool-using turn, two model calls plus the tool run between them.
Keeping the chat timeout the larger of the two means a slow model surfaces the
specific HTTP timeout error rather than a generic "no response", and the job
stops instead of running on after the chat has given up. The shipped 60/90
defaults already had this ordering; only the guidance was wrong.
compose.example.ai.yml gets 300/660 so the Ollama example can actually complete
a tool-using turn.
* fix(chat): claim the pending bubble atomically before appending
append_text! read the row's status and then saved, leaving a window in which
the watchdog could demote the row to `failed` between the two. The late
content would then land on a bubble the user had already been told failed,
flipping it back to `complete`.
Replaces the read with a conditional UPDATE that only succeeds while the row is
still pending, so the check and the state change cannot be separated.
Uses a conditional UPDATE rather than with_lock because append_text! is called
once per chunk on the streaming path; a row lock and transaction per chunk would
be far more expensive. The claim only runs on the first append, since later ones
are no longer pending.
* test(chat): isolate AI_RESPONSE_TIMEOUT from the environment
Chat.response_timeout reads ENV ahead of Setting, so stubbing only the Setting
left the assertions at the mercy of the environment they run in. With
AI_RESPONSE_TIMEOUT=45 exported, five of these tests failed — the default,
floor and grace assertions were all silently measuring the env value.
Adds a with_setting_timeout helper that stubs the Setting and clears the
variable together, and switches the controller tests to stub
Chat.undelivered_response_timeout directly, since what they care about is the
resolved floor rather than how it was configured.
Both files now pass with or without AI_RESPONSE_TIMEOUT set.
* docs(chat): size AI_RESPONSE_TIMEOUT for chained tool calls
The guidance assumed a tool-using turn costs two model calls. #2767 landed
after this branch was opened and made tool calls iterative: `Assistant::Responder`
now loops until `iteration > max_tool_call_iterations`, so a turn runs to
1 + ASSISTANT_MAX_TOOL_CALL_ITERATIONS calls — six by default — with tool
execution in between. At the default 60s per-call timeout that is up to 360s of
model time against a 90s watchdog.
Streaming does not rescue this either. `emit(:output_text)` only fires for a
response that carries text, and tool-call-only rounds carry none, so the bubble
stays on "Thinking…" through every round regardless of provider.
Documents ASSISTANT_MAX_TOOL_CALL_ITERATIONS as the cheaper lever: dropping it to
2 halves the worst case instead of demanding a half-hour timeout, at the cost of
failing long tool chains earlier with a clear limit error. compose.example.ai.yml
now shows that combination rather than a timeout sized for six calls it never had.
* docs(chat): state the whole-turn timeout as a sum, not a maximum
The guidance said to keep AI_RESPONSE_TIMEOUT "above" or "the larger of"
OPENAI_REQUEST_TIMEOUT. That understates it: the watchdog covers the entire turn,
so the bound is
(1 + ASSISTANT_MAX_TOOL_CALL_ITERATIONS) * OPENAI_REQUEST_TIMEOUT
+ tool execution + queue wait
Merely exceeding the per-call limit can still leave the chat reporting failure
while the worker keeps going.
One phrasing was outright wrong: "keep AI_RESPONSE_TIMEOUT the largest of the
three" compared a duration against ASSISTANT_MAX_TOOL_CALL_ITERATIONS, which is a
count, not seconds.
Resizes the examples against the formula — compose.example.ai.yml 1000 -> 1200 and
the Ollama doc example 600 -> 720, both now showing the arithmetic — and states
plainly that the 90s default is sized for typical cloud latency rather than the
worst-case bound, with the formula being what matters once per-call latency
approaches the timeout.
* docs(chat): list the AI settings fields and tag the formula fence
The Settings UI walkthrough listed three of the eight fields on the AI Provider
form. JSON Mode, the three Token Budget fields and the new Chat Response Timeout
were all missing, so the timeout was only discoverable from the troubleshooting
section. Rewrites the list to follow the form's own grouping and uses the labels
the form actually renders.
Also tags the whole-turn formula fence as `text` (markdownlint MD040).
* fix(compose): forward ASSISTANT_MAX_TOOL_CALL_ITERATIONS in standard compose
This file enumerates container environment explicitly — there is no env_file — so
a variable absent from the x-rails-env anchor never reaches web or worker.
The tool-call cap was only named in a comment here, while the docs added in
|
||
|
Sure Admin (bot)
|
73d43bc1d0 |
Fix SSO JIT new family creator role (#3024)
* Fix SSO JIT new family creator role * Preserve super admin SSO creator defaults * Update new family creator role test |
||
|
Guillem Arias Fauste
|
7f0a6fb6dc |
chore: drop dead transactions-section preferences (#3021)
`transactions_section_controller.js` was added by #454 for the upcoming recurring-transactions section. #771 moved recurring transactions to a dedicated tab and removed the only mount, so the controller and its whole persistence chain have been dead since then: `data-controller="transactions-section"` appears nowhere in app/views or app/components, and `transactions_section_collapsed?` had no callers outside its own definition. Removed: - app/javascript/controllers/transactions_section_controller.js - User#update_transactions_preferences - User#transactions_section_collapsed? - TransactionsController#update_preferences + #preferences_params - the `patch :update_preferences` route on the transactions collection The only caller of /transactions/update_preferences was the dead controller itself. No tests referenced any of it — the `update_preferences` cases in pages_controller_test cover the dashboard's route — which is how it stayed dead unnoticed. No migration: users who collapsed a section before #771 keep a stale `transactions_collapsed_sections` key in `users.preferences`, and nothing reads it after this. The dashboard and reports section-layout preferences are untouched. |
||
|
Brandon
|
1973c557e5 |
fix(ai): drop empty data-driven enums from assistant function schemas (#3016)
* fix(ai): drop empty data-driven enums from assistant function schemas
Enum values in tool schemas are built from family data (account names,
categories, merchants, tags, tickers). A family with none of these gets
enum: [], which is invalid JSON Schema. OpenAI tolerates it, but strict
OpenAI-compatible providers reject the entire request, breaking chat for
fresh families until they create a tag or merchant.
Prune empty enums in build_schema, falling back to a plain string. One
choke point covers every function and both consumers: chat tool
definitions for all providers, and the /mcp endpoint's tools/list.
* fix(ai): address review feedback on enum pruning
Stop recursion at populated enum values: enum members are literal
values, not subschemas, so a literal like enum: [{ enum: [] }] must be
preserved verbatim rather than rewritten.
Also cover PREVIEW_FUNCTION_CLASSES in the registry regression test by
enabling the preview preference on the test user, with a guard assertion
so the test fails if preview functions ever silently drop out.
|
||
|
GFRandClaude Sonnet 5
|
746d56c4bd |
fix: gracefully handle invalid family timezone instead of crashing (#2821)
* fix: gracefully handle invalid family timezone instead of crashing Family#timezone is a free-text IANA zone name with no validation on write. If it becomes stale (e.g. tzdata renames a zone, like the historical Europe/Kiev -> Europe/Kyiv switch) or a migration meant to remap legacy names never ran, Localize#switch_timezone passed the raw string straight to Time.use_zone, which raises ArgumentError for any unrecognized zone. Since switch_timezone runs as an around_action on every request, this crashed the entire app for the affected family, including the login page. Now validates the zone via ActiveSupport::TimeZone[] first and falls back to the app default (logging a DebugLogEntry) instead of raising. The log write is debounced per (family, bad value) via Rails.cache (once per day) so an affected family doesn't write one DebugLogEntry row per page view indefinitely. Fixes #390 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: address review feedback on timezone fallback - Make the invalid-timezone debounce lease atomic. Rails.cache.fetch is read-then-write, not atomic, so two concurrent requests could both observe a cache miss and both log before either write landed. Rails.cache.write(unless_exist: true) maps to Redis's atomic SET NX in production, so only one request ever wins the lease. (via CodeRabbit) - Stop using "Europe/Kiev" as the invalid-timezone value in tests. Whether ActiveSupport::TimeZone still resolves that legacy alias depends on the host's installed tzdata version (tzinfo-data is Windows/JRuby-only per Gemfile), so the test's pass/fail behavior wasn't deterministic across machines/CI. Use a deliberately nonexistent name instead. (via Codex) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: validate Family#timezone on write to address root cause of #390 The previous commit made the *crash* graceful, but left the actual defect in place: nothing stopped an unrecognized IANA zone name from being written to Family#timezone in the first place (direct DB/API access, an old dump predating a tzdata rename, or a future rename of a currently-valid zone). Add a Family-level validation using the same ActiveSupport::TimeZone[] lookup Localize#resolved_timezone uses at request time, so "valid at save" and "valid when rendering" can't drift apart. Deliberately not `inclusion: { in: ActiveSupport::TimeZone.all.map(&:name) }`, matching the neighboring locale/date_format validations: verified empirically that the settings form submits `tz.tzinfo.identifier` (e.g. "America/New_York"), not `tz.name` (e.g. "Eastern Time (US & Canada)"), and those differ for all 150 zones Rails ships. An inclusion check against `.name` would have rejected every legitimate value the form submits. The validation only runs when timezone is actually being changed (if: :timezone_changed?). A family with a pre-existing bad value (the exact #390 scenario) must still be able to save unrelated changes -- otherwise this would turn a previously-harmless bad value into a blocker for any other settings update or background job touching that family's record. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |