fix(questions): a position's cost reaches the stem in its native currency, and ranking compares one base currency (closes #664)

[atomchung]

Summary

On a mixed TW/US book, trade_recap.build_state's per-position holdings dict never carried a currency field at all, so review._initial_thesis_question's pos.get("currency") or "USD" fallback silently relabeled a raw TWD cost basis as USD -- roughly a 30x overstatement as the user reads it. The same raw, unconverted cost also drove _ticker_importance's "largest cost" ranking, so on a mixed book a TWD position's face value could outrank a genuinely larger USD position purely because TWD amounts carry more zeros.

Per the maintainer disposition (2026-07-31 comment on #664): the question surface gets a deterministic position-cost record -- native amount + native currency for the stem, plus a single-base-currency ranking value from the shared valuation projection. No FX arithmetic in the agent/copy layer; initial_thesis selection ranks only on the normalized value; missing FX omits the candidate from the ranking rather than falling back to raw units.

Update: an earlier version of this PR's _normalized_position_cost had its own bug, found in review -- see "Correction" below.

What changed

• skills/fomo-kernel/engine/trade_recap.py: build_state now takes cur_map and threads it onto each held position's currency field ((cur_map or {}).get(t, "USD")) -- the same per-ticker currency source portfolio_basis.build_valuation_frame's positions= already reads in main(). Backward compatible: an omitted cur_map keeps the same USD default build_state always produced.
• skills/fomo-kernel/engine/review.py: added _normalized_position_cost, which converts a native-currency amount into the card's aggregate currency through currency_meta.fx -- the exact conversion result [defect·P1] A currency that appears only in a cash-flow row is added to the USD total at 1.0 — #612's refusal has held currencies as its domain, and no rate is ever requested #649 (aggregate_currencies/fetch_fx/usd_view) already resolved and froze onto the card. _ticker_importance's position_cost fallback now uses it instead of comparing raw units. When the rate is unavailable, it returns (None, "fx_unavailable") rather than falling back to the raw amount, and the initial_thesis selection loop drops that candidate from ranking (recording a fx_unavailable rejection) instead of comparing it on an unconverted magnitude.
• tests/test_engine_units.py: unit test proving build_state threads cur_map onto state.holdings.positions[ticker]["currency"], and that an omitted cur_map stays backward compatible.
• tests/test_review_v2.py: (1) two direct _ticker_importance unit tests, each covering both currency_meta shapes (no aggregate_currency key, and the explicit key) -- one for correct normalization, one for the missing-FX refusal; (2) a full-CLI end-to-end test (real CSV ingestion -> build_state -> _question_queue) reproducing the book shape from the issue; (3) a direct _question_queue test proving a candidate with no FX rate is refused from ranking, never compared raw.

The stem construction itself already read the position's own currency field correctly; only the write side (build_state never populated it) and the ranking read (_ticker_importance's raw fallback) were wrong.

Correction (post-review)

Review found a real defect in the first version: _normalized_position_cost's own aggregate-currency fallback --
aggregate = str(meta.get("aggregate_currency") or currency).upper() -- silently defaulted the aggregate to the
position's own currency whenever aggregate_currency was absent from currency_meta, making currency == aggregate
trivially true and disabling normalization for the whole book. Reproduction supplied and confirmed:

meta_real = {"mixed": True, "currencies": ["TWD", "USD"], "fx": {"TWD": 0.031}}   # no aggregate_currency key
_normalized_position_cost(900000, "TWD", {"currency_meta": meta_real})  # -> 900000.0 (raw), should be 27900.0

Investigation into whether this is reachable through the two real production producers:

• trade_recap.build_state's currency_meta literal (trade_recap.py) always sets aggregate_currency explicitly.
• snapshot_adapter.py's currency_meta literal also always sets it explicitly.
• No class CurrencyMeta exists anywhere in the engine; both producers are plain dict literals.
• The [defect·P1] A TWD position's cost basis reaches the question surface labeled USD, and the "largest cost" ranking compares raw units across currencies #664 E2E test's real CLI-produced card was re-checked and does carry aggregate_currency (verified: plan["engine_card"]["currency_meta"]["aggregate_currency"] == "USD" on that fixture, now asserted in the test itself).

So the specific claim "production never writes this key" does not hold for either of the two current documented ingestion routes (CSV and snapshot). It is reachable through the --card-json/--state-json adapter/testing lane (review.py's own help text calls those flags "adapter/testing"), and defensively through any future producer -- which is exactly why none of the original unit tests (which all hand-built a currency_meta with the key present) caught it. The underlying defensive-correctness bug is real and worth fixing regardless of today's reachability, per this repo's own "the identity factor is removed wherever currencies differ, not merely guarded upstream" discipline (#649's mirrored-surfaces row).

Fix: _normalized_position_cost now calls card_renderer._currency(card) -- the single already-existing reader for "this card's aggregate currency," with its own documented or "USD" fallback (card_renderer.py:842) -- instead of re-deriving the aggregate with a second, divergent default. _exit_importance has the identical latent pattern (aggregate = str(meta.get("aggregate_currency") or currency).upper()); left untouched here as a separate, pre-existing surface out of #664's scope (noted as an adjacent finding).

On the "E2E contradiction": the E2E test's card comes from the real CLI (trade_recap.main()), which always populates aggregate_currency -- so the E2E test never exercised the missing-key shape either before or after this correction, and mutation (a) (raw-unit ranking restored) reddening it is unrelated to this specific bug; it reddens because that mutation removes normalization entirely, not because of the aggregate-currency key. Confirmed empirically both ways: plan["engine_card"].get("ticker_diagnosis") on the E2E fixture lists only ["RTA", "RTC", "RTB"] -- none of the four test tickers -- so the position_cost fallback this fix touches is genuinely exercised, and raw vs. normalized orderings genuinely differ (900000 > 600000 > 30000 > 22000 raw vs. 30000 > 28530 > 22000 > 19020 normalized). Both facts are now asserted in the test itself, not left to a comment.

Proof

1. Mixed TW/US fixture that emits initial_thesis questions: test_initial_thesis_native_currency_label_and_normalized_cost_ranking_on_mixed_book runs a real CSV (3 closed round trips for the behavioral tier + 2 TWD / 2 USD open positions) through prepare and confirms 3 initial_thesis questions are queued.
2. Native currency truthful in every stem: same test asserts the TWD position's stem contains "TWD 900,000" and never the literal "USD"; the USD position's currency field reads "USD".
3. Order matches normalized cost, including the exact TW-raw-bigger-but-normalized-smaller shape: normalized order is USOPEN1 (30,000) > 3001.TW (~28,530) > USOPEN2 (22,000) > 3002.TW (~19,020, trimmed) -- raw order would have put both TWD positions first. Verified selected == ["USOPEN1", "3001.TW", "USOPEN2"] and 3002.TW rejected with initial_thesis_limit. The test now also asserts ticker_diagnosis omits all four tickers (proving the position_cost fallback is genuinely exercised) and that this fixture's own currency_meta does carry aggregate_currency (proving what this specific test can and cannot catch).
4. Missing FX never falls back to raw-unit ordering: test_ticker_importance_refuses_rather_than_falls_back_to_raw_units_on_missing_fx and test_initial_thesis_selection_refuses_a_candidate_it_cannot_normalize_instead_of_ranking_it_raw construct a mixed=True card with an incomplete fx map directly (the real pipeline cannot reach this state -- trade_recap.usd_view already refuses the whole review upstream) and confirm the TWD candidate is dropped with reason fx_unavailable, never entered into the ranked queue.
5. Aggregate currency resolved through one reader, both currency_meta shapes: test_ticker_importance_ranking_uses_engine_fx_for_mixed_currency_amounts and the missing-FX test above both loop over "no aggregate_currency key" and "explicit key" and assert identical, correct results for each.

Mutation dance (verbatim red output, then restored)

(a) Restored the raw-unit ranking (reverted _ticker_importance's fallback to abs(float(pos.get("cost") or 0)), "position_cost"), run against the corrected tests:

FAIL test_ticker_importance_ranking_uses_engine_fx_for_mixed_currency_amounts :: AssertionError(('no aggregate_currency key', 900000.0))
FAIL test_ticker_importance_refuses_rather_than_falls_back_to_raw_units_on_missing_fx :: AssertionError(('no aggregate_currency key', 900000.0, 'position_cost'))
FAIL test_initial_thesis_native_currency_label_and_normalized_cost_ranking_on_mixed_book :: AssertionError("selection must rank by normalized cost, not raw face value: ['3001.TW', '3002.TW', 'USOPEN1']")
FAIL test_initial_thesis_selection_refuses_a_candidate_it_cannot_normalize_instead_of_ranking_it_raw :: AssertionError(...TWX entered the queue...)

(b) Restored the relabel (reverted build_state's holdings dict to a constant "currency": "USD", never cur_map):

FAIL test_build_state_threads_cur_map_onto_each_position_currency :: AssertionError({'shares': 900.0, 'cost': 900000.0, ..., 'currency': 'USD', ...})
FAIL test_initial_thesis_native_currency_label_and_normalized_cost_ranking_on_mixed_book :: AssertionError({..., 'currency': 'USD', ..., 'question': 'You are holding 3001.TW at a cost basis of about USD 900,000. ...'})

(c) Restored the divergent aggregate-currency fallback (_normalized_position_cost's aggregate line reverted to str(meta.get("aggregate_currency") or currency).upper()) -- the specific bug review found:

FAIL test_ticker_importance_ranking_uses_engine_fx_for_mixed_currency_amounts :: AssertionError(('no aggregate_currency key', 900000.0))
FAIL test_ticker_importance_refuses_rather_than_falls_back_to_raw_units_on_missing_fx :: AssertionError(('no aggregate_currency key', 900000.0, 'position_cost'))
PASS test_initial_thesis_native_currency_label_and_normalized_cost_ranking_on_mixed_book   (expected -- its real card always carries the key)
PASS test_initial_thesis_selection_refuses_a_candidate_it_cannot_normalize_instead_of_ranking_it_raw   (expected, same reason)

All three mutations restored from backup; full suite re-verified green after each.

Test plan

• python3 tests/run_all.py -- all 49 suites pass
• Mutation (a) reddens the 4 ranking-dependent tests; restored
• Mutation (b) reddens the 2 currency-label-dependent tests; restored
• Mutation (c) reddens exactly the 2 direct _ticker_importance tests' "no key" cases and leaves the E2E/queue tests green, as expected; restored
• Rebased twice onto origin/main as it advanced during this session (fix(add-cash): the drift gate compares frozen source facts, so the card-beat cash answer is recordable (closes #665) #684/[defect·P1] add-cash refuses the session it exists for: the drift gate compares question_queue and state_snapshot whole, so the card-beat cash answer is unrecordable #665, then fix(questions): planned_entry declares and collects its thesis capture in the same exchange (closes #667) #692/[defect] planned_entry declares no answer requirement, then preview refuses without a captured thesis — a dead end reachable only by following the contract #667) -- neither touches the _ticker_importance/_initial_thesis_question/initial_candidates code path this PR changes

🤖 Generated with Claude Code