fix(fx): every currency entering account aggregation is fetched, and a missing rate fails closed (closes #649) - #685
Merged
Conversation
…a missing rate fails closed (closes #649) `cur_map` is filled by `load()`, which keeps BUY/SELL rows only, while `load_cash_flows` reads a separate per-row `Currency`. #612's refusal and the `mixed_ccy` fetch decision both read `cur_map`, so a currency present only in a cash-flow row was invisible to both: on a USD stock book with one TWD interest row no rate was ever requested, and `cash_position` summed raw TWD into a USD total at 1.0 — a balance 28x its real size, reported under `currency_meta.mixed: false`, `fx: "not_needed"`, `basis.fx_approx: false`, `unanchored: []` and `cash_source: "anchored"`. Remedy 2 from the maintainer disposition: fetch every currency that can enter the aggregate, then fail closed only when acquisition genuinely fails. - `aggregate_currencies` is the one domain builder (held positions plus cash-flow rows) and `missing_aggregate_fx_rates` the one predicate over whatever domain a lane built. `held_currency_fx_gaps` becomes their composition over the holdings domain instead of a second walk of `cur_map`, so `usd_view` and `consequence` are unchanged. - `main()` composes the universe before `market_request`, so the same set drives acquisition and the gate, and refuses upstream of the card, the state write and the ledger ingest when a required rate did not arrive. - The identity fallback is removed wherever currencies differ rather than only guarded upstream: `cash_position` refuses two buckets it has no rate for, and `account_perf` gates `missing_fx` instead of letting `_fx_getter` return `[1.0] * len(px_index)`. That getter's domain drops the unconditional union with USD, which had made a pure TWD book indistinguishable from a mixed one missing a rate. - Accepted cash anchors need no term of their own: `cash_position` buckets by cash-flow currency, so an anchor in a currency no flow created is dropped rather than converted, and refusing over it would block a book on an amount nothing reads. - `MissingHeldCurrencyRate` is now `MissingAggregateCurrencyRate`; its sentence names the aggregate's span rather than claiming every currency in it is held. Single-currency books are byte-stable: a pure USD book, a pure TWD book and #612's own mixed-holdings shape each produce a plan identical to `origin/main`'s, compared field by field. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
atomchung
marked this pull request as ready for review
July 31, 2026 17:08
This was referenced Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #649. Implements the maintainer disposition (candidate remedy 2): fetch every currency that can enter a cross-currency aggregate, then fail closed only when a required rate is genuinely unavailable.
The defect
cur_mapis filled byload(), which keeps BUY/SELL rows only.load_cash_flowsreads a separate per-rowCurrencyforDEPOSIT/WITHDRAWAL/DIVIDEND/INTEREST/FEE. #612's refusal and themixed_ccyfetch decision both readcur_map, so a currency present only in a cash-flow row was invisible to both — no rate was ever requested, and nothing was ever reported missing.Reproduced on
main@e58fef9with the issue's synthetic CSV (a USD stock book with one TWD interest row),--cashanchors in both currencies, an isolated--rootandTR_LEDGER=/dev/null:cash.balancecash.weightcurrency_meta.mixedfalsetruecurrency_meta.currencies["USD"]["TWD", "USD"]price_provenance.fx"not_needed""feed"honesty_ledgercurrency_mixentrycurrency_mixpresentWhat changed
skills/fomo-kernel/engine/trade_recap.pyaggregate_currenciesis the one domain builder (held positions + cash-flow rows);missing_aggregate_fx_ratesis the one predicate over whatever domain a lane built;held_currency_fx_gapsbecomes their composition over the holdings domain.main()composes the universe beforemarket_request, so acquisition and the gate read one set, and refuses upstream of the card/state/ledger.cash_positionrefuses two buckets it has no rate for.MissingHeldCurrencyRate→MissingAggregateCurrencyRate, with a sentence that names the aggregate's span rather than claiming every currency in it is held.skills/fomo-kernel/engine/perf.py_fx_getterreturns the currencies it could not cover instead of silently emitting[1.0] * len(px_index), andaccount_perfwithholds the whole account pillar behind amissing_fxgate. Its domain drops the unconditional union withUSD, which had made a pure TWD book indistinguishable from a mixed one missing a rate.docs/maintainer-guide.mddocs/prd-ledger.mdskills/fomo-kernel/references/price-feed.md,schemas/price-feed.schema.jsontests/test_engine_units.pycash_position's refuse/convert pair, and its two compatibility halves.tests/test_price_paths.pyaccount_perf's refuse/convert pair and the pure-TWD compatibility half; the two existing mixed-currency fixtures now carry thefx_spotthe production path always supplies.tests/test_review_v2.pyTwo decisions worth reviewing
Accepted cash anchors get no term of their own in the universe. The disposition names them, but
cash_positionbuckets by cash-flow currency and_anchor_forpairs anchors into those buckets, so an anchor in a currency no flow created is dropped rather than converted. Including it would refuse a review over an amount nothing reads. Asserted, not reasoned:test_cash_position_single_bucket_and_flowless_anchor_keep_the_identity.The universe is built from
load_cash_flows(paths)withouttrade_rows. The estimate that argument unlocks needs split-adjusted amounts, and splits cannot be applied before the market request is composed. Omitting it cannot narrow the answer, because those rows take their currency from a trade row, which is already acur_mapvalue — pinned by an equality assertion intest_aggregate_currencies_spans_cash_flow_rows_not_only_holdingsrather than left as a comment.The four proofs
1. The fixture requests TWD FX and converts correctly when the rate resolves.
test_a_currency_only_a_cash_flow_row_carries_is_fetched_and_refused_when_absent(second half).cash.balanceis 35,849.00 (1,000,000 × 0.030849 + 5,000) against the 1,005,000 it reported before,currency_meta.fx == {"TWD": 0.030849},mixed is True,price_provenance.fx == "feed". The request half is proven by the code, not the value:market_data._from_feediteratesrequest["currencies"], so afx_unavailablegap for TWD (asserted in the first half) exists only if TWD entered the acquisition request.2. The same fixture fails closed before account-level metrics when the rate does not resolve. Same test, first half —
returncode == 2, one controlled CLI error, no traceback, and.pending/ledger.jsonlabsent from the refused root:3. Pure-USD and pure-TWD books are unchanged. Two receipts.
test_a_book_whose_cash_rows_share_the_stock_currency_is_byte_stabledrives both through the CLI and assertsmixed is False,fx is None,price_provenance.fx == "not_needed"and nocurrency_mixentry. Out of band, the same books plus #612's own mixed-holdings shape were run throughorigin/main's engine and this branch's and the persistedplan.jsoncompared field by field, normalizing only the two fields that are functions of the environment (engine_version, from git-describe, and themkdtemppath insideengine_meta):4. Mutation proof. Each mutation applied to a
cpbackup,__pycache__cleared between runs, restored by checksum afterward.(a1)
cash_position's 1.0 fallback restored — delete theif gaps: raise, leavingfx.get(c, 1.0):(a2)
_fx_getter's 1.0 fallback restored — delete themissing.append(c)guard:The failure output is the defect itself: 300,000 TWD read as 300,000 USD gives
acct_twr30,050% andcash_drag30,000%, withfx_approx: falseandunanchored: []still claiming nothing is wrong — the same shape the issue measured at 2945% / 2903%.(b) held-currency-only universe restored —
aggregate_currencies(cur_map, load_cash_flows(paths))→aggregate_currencies(cur_map):It reddens on the
fx_unavailableassertion specifically, which is the intended kill: with the narrow universe the book is still refused, but bycash_positionrather than by acquisition, because TWD was never requested and so was never reported missing. The single-currency regression stays green under this mutation, as a compatibility test should.Suite
Run from the repository root with pandas 3.0.3, yfinance 1.3.0 and rich importable, so nothing skipped silently.
Adjacent findings, deliberately not folded in
account_perfcanKeyErroron a residual currency.fx_at(r["currency"], n - 1)at thecash_residualgate indexescols, which is built from the value-carrying currencies. A ledger cash snapshot declaring a currency with no flow and no holding produces a residual outside that set. Pre-existing, unchanged here, and unreachable frommain()today.currency_meta.alpha_beta_noteis written and never read — a [design·P1] Two questions collect an answer nothing reads: initial_thesis and exit_consistency (proved by a byte-identical card) #429-class dead field. It also carries a hard-coded Traditional Chinese sentence onto an English card whenever a book is mixed, which this change makes reachable for one more book shape._anchor_for). The user declares a balance and the engine ignores it without a word. [feat·design] Pre-review data-sanity check: confirm cash anchor (and flag known-incomplete data) before the motive questions #357-adjacent; the correct fix is a disclosure or a bucket, not a refusal.perf.account_perf'smissing_fxgate has noaccount_gatecopy key. It degrades to the genericannualizedsentence by the renderer's documented fallback. Left uncovered on purpose: the gate is defence in depth and cannot reach a card whilemain()refuses upstream, so a copy key would be a string with no producer.Note for the merge queue
PR #684 touches
docs/maintainer-guide.mdandtests/test_review_v2.pyas well. Different row and different region — text conflicts are possible, semantic overlap is not.🤖 Generated with Claude Code