From a8d404ef94d3b039dbd5640508914b977386891a Mon Sep 17 00:00:00 2001 From: test Date: Sat, 1 Aug 2026 01:42:51 +0800 Subject: [PATCH 1/2] fix(questions): a position's cost reaches the stem in its native currency, and ranking compares one base currency (closes #664) On a mixed TW/US book, `build_state`'s per-position holdings dict never carried a currency field at all, so `_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 a TWD position's face value could outrank a genuinely larger USD position purely because TWD amounts carry more zeros. - trade_recap.build_state now threads the caller's `cur_map` (the same per-ticker currency source `portfolio_basis.build_valuation_frame` already reads) onto each position, so the native currency is a stored fact instead of a reader's assumption. - review._ticker_importance's `position_cost` fallback now normalizes through `card.currency_meta.fx` -- the resolved conversion #649 already froze onto the card -- exactly like the sibling `_exit_importance` already does for exit-notional ranking. When the required rate is unavailable the candidate is refused from the ranked selection (reason `fx_unavailable`) rather than compared on its raw, unconverted magnitude. The stem itself already read the position's own currency field; only the write side and the ranking read were wrong. Co-Authored-By: Claude Fable 5 --- skills/fomo-kernel/engine/review.py | 55 ++++++++- skills/fomo-kernel/engine/trade_recap.py | 9 +- tests/test_engine_units.py | 35 ++++++ tests/test_review_v2.py | 151 +++++++++++++++++++++++ 4 files changed, 245 insertions(+), 5 deletions(-) diff --git a/skills/fomo-kernel/engine/review.py b/skills/fomo-kernel/engine/review.py index 1f500690..90536e39 100644 --- a/skills/fomo-kernel/engine/review.py +++ b/skills/fomo-kernel/engine/review.py @@ -2906,15 +2906,53 @@ def _exit_question(item, language, card=None, prior=None): } +def _normalized_position_cost(cost, currency, card): + """A native-currency cost basis converted into the card's aggregate + currency (#664), or ``None`` when the review's own resolved FX map has no + rate for it. + + Reads the same frozen ``currency_meta`` (#649's shared conversion result) + ``_exit_importance`` already reads, rather than a second acquisition of + the aggregate lookup. Never an identity/raw fallback: a book the engine + reports ``mixed`` already has a complete rate for every held currency — + ``trade_recap.usd_view`` fails the whole review closed otherwise — so + ``None`` here only guards a caller whose card/state did not go through + that gate, which is exactly the shape a unit test exercises directly. + """ + meta = (card or {}).get("currency_meta") or {} + currency = str(currency or "USD").upper() + aggregate = str(meta.get("aggregate_currency") or currency).upper() + if not meta.get("mixed") or currency == aggregate: + return abs(float(cost or 0)) + factor = (meta.get("fx") or {}).get(currency) + if factor is None: + return None + try: + return abs(float(cost) * float(factor)) + except (TypeError, ValueError): + return None + + def _ticker_importance(card, state, ticker): for row in card.get("ticker_diagnosis") or []: if row.get("ticker") == ticker and row.get("impact") is not None: return abs(float(row["impact"])), "pnl_impact" pos = (_active_positions(state).get(ticker) or {}) try: - return abs(float(pos.get("cost") or 0)), "position_cost" + cost = float(pos.get("cost") or 0) except (TypeError, ValueError): return 0.0, "unknown" + # #664: the position_cost fallback used to compare this raw native-currency + # magnitude directly against other tickers' raw magnitudes -- silently + # wrong on a mixed-currency book, where the same face value in TWD and USD + # differ by the exchange rate. Normalize it exactly like the ticker_diagnosis + # branch above already is (its rts/held/last_px are usd_view's own output). + normalized = _normalized_position_cost(cost, pos.get("currency"), card) + if normalized is None: + # Never fall back to the raw amount: refuse the ranking for this + # candidate rather than let an unconverted magnitude compete. + return None, "fx_unavailable" + return normalized, "position_cost" def _initial_thesis_id(cycle_id): @@ -3147,9 +3185,20 @@ def _question_queue(card, state, active, previous_state, language, recent_exits= cost = card_renderer._finite_number(pos.get("cost")) if cost is None or cost <= 0: continue # cannot ground the stem in a concrete magnitude - initial_candidates.append(_initial_thesis_question( + candidate = _initial_thesis_question( ticker, pos, cost, card, state, language, - recalled=_recalled_entry_statement(evaluation_recall, ticker, cycle_id))) + recalled=_recalled_entry_statement(evaluation_recall, ticker, cycle_id)) + if candidate.get("_importance") is None: + # #664: the "largest cost" ranking compares normalized amounts + # only. When the aggregate FX map has no rate for this + # position's currency, refuse to rank it rather than let its + # raw native-currency magnitude compete against one already + # normalized -- the same fail-closed posture #649 gives the + # aggregate reader this candidate's importance came from. + rejected.append(_rejection(candidate["id"], "initial_thesis", + "fx_unavailable", cycle_id=cycle_id)) + continue + initial_candidates.append(candidate) initial_candidates.sort(key=lambda row: (-float(row.get("_importance") or 0), str(row.get("id")))) candidates.extend(initial_candidates[:INITIAL_THESIS_LIMIT]) initial_overflow = initial_candidates[INITIAL_THESIS_LIMIT:] diff --git a/skills/fomo-kernel/engine/trade_recap.py b/skills/fomo-kernel/engine/trade_recap.py index b56e6184..b202ee5b 100644 --- a/skills/fomo-kernel/engine/trade_recap.py +++ b/skills/fomo-kernel/engine/trade_recap.py @@ -2206,7 +2206,7 @@ def position_observables(holdings, last_px, date_end, prices=None, markets=None) def build_state(rows, rts, held, dims, overview, ab, rx, currency_meta=None, - avg_down=None, last_px=None, prev_end=None, cash=None, + cur_map=None, avg_down=None, last_px=None, prev_end=None, cash=None, portfolio_structure=None, price_snapshot=None, market_context=None, max_pos_override=None, price_provenance=None, price_request=None, prices=None, valuation_frame=None, splits=None, @@ -2271,6 +2271,11 @@ def build_state(rows, rts, held, dims, overview, ab, rx, currency_meta=None, add_cursors = current_cycle_add_cursors(rows) holdings = {t: {"shares": round(sh, 4), "cost": round(c, 2), "avg_cost": round(c / sh, 4) if sh > 1e-9 else None, + # #664: the position's own native currency (same reader as + # portfolio_basis.build_valuation_frame's `positions=` above), + # so a downstream cost-basis question can label its native + # amount correctly instead of a reader defaulting it to USD. + "currency": (cur_map or {}).get(t, "USD"), "cycle_start": cyc.get(t, {}).get("start"), # 算不出開倉（CSV 缺期初持倉）→ 標 #unknown，不 fallback 裸 ticker（雙審 codex#4） # ⚠️ 格式契約 = 頂部 CYCLE_ID_RE / CYCLE_ID_UNKNOWN_RE(#61):改這裡必先改常數,契約測試會抓 @@ -3009,7 +3014,7 @@ def main(): fx_provenance=("engine_fetch" if not fx_err else "unavailable"), ).to_dict() state = build_state(rows, rts, held, dims, overview, ab, rx, - currency_meta=currency_meta, + currency_meta=currency_meta, cur_map=cur_map, avg_down=avg_down, last_px=last_px, prev_end=prev_end, cash=cash_data, portfolio_structure=portfolio_structure, diff --git a/tests/test_engine_units.py b/tests/test_engine_units.py index aa904ab3..2a16357d 100644 --- a/tests/test_engine_units.py +++ b/tests/test_engine_units.py @@ -757,6 +757,41 @@ def test_build_state_echoes_position_cap_override(): max_pos_override=5)["max_position_pct"] is None, "壞值 fail-closed → None" +def test_build_state_threads_cur_map_onto_each_position_currency(): + """#664: a held position's own native currency must reach + `state["holdings"]["positions"][ticker]["currency"]`, not default silently + to USD. Before this, `build_state`'s per-ticker dict carried no currency + field at all regardless of the caller's `cur_map` -- so a downstream + reader (the initial-thesis question stem) had nothing but its own "USD" + fallback, and a TWD position's raw cost was relabeled as if it were USD. + + `cur_map=None` (the parameter's default, and every call site before #664) + must keep resolving to "USD" -- the same default `cur_map.get(t, "USD")` + already uses elsewhere (e.g. `portfolio_basis.build_valuation_frame`'s + `positions=` in `main()`), so this is additive rather than a behavior + change for a single-currency book. + """ + rows = [_R("2330.TW", "buy", 900, 1000.0, "2024-03-01"), + _R("AAPL", "buy", 100, 150.0, "2024-03-02")] + rows[0]["currency"] = "TWD" + rows[1]["currency"] = "USD" + ab = dict(note="無價格") + cur_map, _currencies, _conflicts = tr.currency_map(rows) + assert cur_map == {"2330.TW": "TWD", "AAPL": "USD"}, cur_map + + with_map = _state_from(rows, ab, cur_map=cur_map)["holdings"]["positions"] + assert with_map["2330.TW"]["currency"] == "TWD", with_map["2330.TW"] + assert with_map["AAPL"]["currency"] == "USD", with_map["AAPL"] + + # Backward compatible: an existing caller that never learned about + # `cur_map` (every call site before #664) still gets the same USD default + # `build_state` always produced -- just now on an explicit field instead + # of a downstream reader assuming it. + no_map = _state_from(rows, ab)["holdings"]["positions"] + assert no_map["2330.TW"]["currency"] == "USD" + assert no_map["AAPL"]["currency"] == "USD" + + # ─────── J3. current_book_projection():dim_size × ticker_diagnosis 分母合一(#477) ─────── # # #324/#481 已把診斷/處方的觸發「線」對齊到同一個 effective_oversize_trigger;這裡收的是 diff --git a/tests/test_review_v2.py b/tests/test_review_v2.py index de7c87db..38450899 100644 --- a/tests/test_review_v2.py +++ b/tests/test_review_v2.py @@ -6760,6 +6760,44 @@ def test_exit_question_ranking_uses_engine_fx_for_mixed_currency_amounts(): "raw TWD notional must not outrank a larger aggregate-currency exit" +def test_ticker_importance_ranking_uses_engine_fx_for_mixed_currency_amounts(): + """#664: the initial-thesis "largest cost" ranking must hold the same + discipline the sibling test above already holds for exit-notional ranking. + The `position_cost` fallback used to compare `pos["cost"]` -- a raw + native-currency face value -- directly across tickers, so a TWD position + could outrank a larger USD one purely because TWD amounts carry more + zeros. It must read `currency_meta.fx` (#649's already-resolved rate) + exactly like `_exit_importance` does, never the raw amount.""" + card = {"currency_meta": {"mixed": True, "aggregate_currency": "USD", + "fx": {"TWD": 0.0317}}, "ticker_diagnosis": []} + state = {"holdings": {"positions": { + "TWX": {"cost": 900000.0, "currency": "TWD"}, + "USX": {"cost": 30000.0, "currency": "USD"}, + }}} + tw_importance, tw_basis = review_engine._ticker_importance(card, state, "TWX") + us_importance, us_basis = review_engine._ticker_importance(card, state, "USX") + assert tw_basis == us_basis == "position_cost" + assert abs(tw_importance - 28530.0) < 1e-6, tw_importance + assert us_importance == 30000.0 + assert us_importance > tw_importance, \ + "a smaller USD position must outrank a larger raw-TWD one once normalized" + + +def test_ticker_importance_refuses_rather_than_falls_back_to_raw_units_on_missing_fx(): + """#664: a missing rate must never resolve to an identity factor of 1.0 -- + the same "the identity factor is removed wherever currencies differ, not + merely guarded upstream" discipline #649 holds for the aggregate reader. + This is exercised directly because the real pipeline cannot reach this + state: a book the engine reports `mixed` already carries a complete `fx` + map for every held currency, since `trade_recap.usd_view` refuses the + whole review before a card/state with a gap could ever be built.""" + card = {"currency_meta": {"mixed": True, "aggregate_currency": "USD", "fx": {}}, + "ticker_diagnosis": []} + state = {"holdings": {"positions": {"TWX": {"cost": 900000.0, "currency": "TWD"}}}} + importance, basis = review_engine._ticker_importance(card, state, "TWX") + assert importance is None and basis == "fx_unavailable", (importance, basis) + + def test_custom_exit_reason_requires_the_users_words(): question = review_engine._exit_question( {"revisit_id": "A#2026-07-01#1#2026-07-10#1.0", "ticker": "A", @@ -9681,6 +9719,119 @@ def test_initial_thesis_dedup_skips_a_position_with_an_existing_thesis(): for q in plan["question_queue"]), "the un-thesised holding is still asked" +_664_MIXED_BOOK_ROWS = [ + # Three closed US round trips -- only their count matters, so the tier + # classifier (>= MIN_ROUND_TRIPS) reaches "behavioral" and the first-review + # question band is not suppressed as a thin/structural file (#306). Dated + # well before the open positions below so they never register as recent + # exits and compete for a capture slot. + "RTA,BUY,100,50.00,2024-01-05,Trade,US,USD", + "RTA,SELL,100,60.00,2024-02-05,Trade,US,USD", + "RTB,BUY,50,40.00,2024-01-06,Trade,US,USD", + "RTB,SELL,50,45.00,2024-02-06,Trade,US,USD", + "RTC,BUY,80,30.00,2024-01-07,Trade,US,USD", + "RTC,SELL,80,35.00,2024-02-07,Trade,US,USD", + # Four open, un-thesised positions -- two TW/TWD, two US/USD -- with no + # existing thesis, so every one is an initial_thesis candidate. + "3001.TW,BUY,900,1000.00,2024-03-01,Trade,TW,TWD", # raw 900,000 TWD; normalized ~28,530 + "3002.TW,BUY,600,1000.00,2024-03-02,Trade,TW,TWD", # raw 600,000 TWD; normalized ~19,020 (smallest) + "USOPEN1,BUY,300,100.00,2024-03-03,Trade,US,USD", # 30,000 USD (largest normalized) + "USOPEN2,BUY,220,100.00,2024-03-04,Trade,US,USD", # 22,000 USD +] +_664_MIXED_BOOK_CLOSES = [ + # Close == avg cost on every open position, so realized+unrealized impact + # is ~0 and `ticker_diagnosis` (which drops |impact| < 1) never reports + # them -- forcing `_ticker_importance`'s `position_cost` fallback, the + # exact path #664 fixes, rather than the already-normalized pnl_impact + # branch fed by usd_view's own `_u` arrays. + {"ticker": "3001.TW", "close": 1000.0, "date": "2026-07-30", "currency": "TWD"}, + {"ticker": "3002.TW", "close": 1000.0, "date": "2026-07-30", "currency": "TWD"}, + {"ticker": "USOPEN1", "close": 100.0, "date": "2026-07-30", "currency": "USD"}, + {"ticker": "USOPEN2", "close": 100.0, "date": "2026-07-30", "currency": "USD"}, +] + + +def test_initial_thesis_native_currency_label_and_normalized_cost_ranking_on_mixed_book(): + """#664, driven through the real CLI (CSV ingestion -> `trade_recap.main`'s + `build_state` -> `review.py`'s question queue) rather than injected + artifacts, so a regression in either the write side (build_state + threading `cur_map` onto each position) or the read side + (`_ticker_importance`/`_initial_thesis_question`) is caught. + + On this mixed TW/US first review, a TWD position's cost basis must reach + the initial-thesis stem in its own currency -- never relabeled USD -- and + the "largest cost" selection must rank on the FX-normalized value, not the + raw face value. Reproduces the book shape from the issue: 3001.TW's raw + TWD cost (900,000) is larger than 3002.TW's (600,000) and both dwarf the + USD positions' raw figures (30,000 / 22,000), but at the review's own + resolved rate (0.0317 USD per TWD) they convert to ~28,530 and ~19,020 -- + both inside the USD positions' range. The normalized ranking must + interleave them (USOPEN1 > 3001.TW > USOPEN2 > 3002.TW), never group + every TWD position ahead of every USD one by raw magnitude alone. + """ + with tempfile.TemporaryDirectory() as tmp: + env = _offline_engine_env(tmp) + csv_path, prices = _fx_case(tmp, "initial_thesis_664", _664_MIXED_BOOK_ROWS, + fx={"TWD": 0.0317}, prices=_664_MIXED_BOOK_CLOSES) + root = pathlib.Path(tmp) / "coach" + run = _run("prepare", csv_path, "--root", root, "--language", "en", + "--prices", prices, env=env) + assert run.returncode == 0, run.stdout + run.stderr + plan = _fx_plan(root) + assert plan["route"] == "first_review" + assert plan["engine_state"]["review_tier"]["tier"] == "behavioral", \ + "this fixture must actually clear the thin-file suppression (#306)" + + initial = [q for q in plan["question_queue"] if q["kind"] == "initial_thesis"] + assert initial, "a mixed-currency first review must still emit initial-thesis questions" + + by_ticker = {q["ticker"]: q for q in initial} + assert by_ticker["3001.TW"]["currency"] == "TWD", by_ticker["3001.TW"] + assert "TWD 900,000" in by_ticker["3001.TW"]["question"], by_ticker["3001.TW"]["question"] + assert "USD" not in by_ticker["3001.TW"]["question"], \ + "a TWD position's stem must never relabel its native amount as USD" + assert by_ticker["USOPEN1"]["currency"] == "USD" + + # Normalized order: USOPEN1 (30,000) > 3001.TW (~28,530) > USOPEN2 + # (22,000) > 3002.TW (~19,020, the smallest, trimmed by INITIAL_THESIS_LIMIT). + assert [q["ticker"] for q in initial] == ["USOPEN1", "3001.TW", "USOPEN2"], \ + (f"selection must rank by normalized cost, not raw face value: " + f"{[q['ticker'] for q in initial]}") + rejected = plan["card_plan"]["question_selection"]["rejected"] + assert {"id": review_engine._initial_thesis_id("3002.TW#2024-03-02#1"), + "kind": "initial_thesis", "cycle_id": "3002.TW#2024-03-02#1", + "reason": "initial_thesis_limit"} in rejected, \ + "3002.TW has the smallest normalized cost and must be the one trimmed, not by raw units" + + +def test_initial_thesis_selection_refuses_a_candidate_it_cannot_normalize_instead_of_ranking_it_raw(): + """#664: when the aggregate FX map has no rate for a held currency, the + candidate must be refused from the ranked selection rather than compared + on its raw native-currency magnitude. Exercised through `_question_queue` + directly (the same pattern `test_weekly_review_quiet_week_backfills_...` + above uses) because the real CLI cannot construct this state -- a book + the engine reports `mixed` already has a complete `fx` map by the time a + card exists at all.""" + card = {"currency_meta": {"mixed": True, "aggregate_currency": "USD", "fx": {}}, + "ticker_diagnosis": [], "thesis_questions": []} + positions = { + "TWX": {"cost": 900000.0, "currency": "TWD", "cycle_id": "TWX#2026-01-01#1"}, + "USX": {"cost": 30000.0, "currency": "USD", "cycle_id": "USX#2026-01-01#1"}, + } + state = {"holdings": {"positions": positions}} + missing = [{"ticker": t, "cycle_id": p["cycle_id"]} for t, p in positions.items()] + queue, report = review_engine._question_queue( + card, state, {}, None, "en", route="first_review", + missing_thesis_positions=missing, tier="behavioral") + initial = [q for q in queue if q["kind"] == "initial_thesis"] + assert [q["ticker"] for q in initial] == ["USX"], \ + f"the TWD candidate with no FX rate must never enter the ranked queue: {initial}" + assert {"id": review_engine._initial_thesis_id("TWX#2026-01-01#1"), + "kind": "initial_thesis", "cycle_id": "TWX#2026-01-01#1", + "reason": "fx_unavailable"} in report["rejected"] + assert all(set(row) == {"id", "kind", "cycle_id", "reason"} for row in report["rejected"]) + + def _evaluation_row(evaluation_id, ticker, created, reason=None, decision="open"): """A trade_evaluations.jsonl row shaped like consider's own writer. From 7faa3fb5dc0642f0716f46a48a83601d67f31db0 Mon Sep 17 00:00:00 2001 From: test Date: Sat, 1 Aug 2026 02:07:57 +0800 Subject: [PATCH 2/2] fix(questions): resolve the aggregate currency through the single existing reader, not a second divergent default Review found that _normalized_position_cost's own fallback -- `aggregate = str(meta.get("aggregate_currency") or currency).upper()` -- silently defaults the aggregate currency to the *position's own* currency whenever `aggregate_currency` is absent from `currency_meta`. That makes `currency == aggregate` trivially true for every position, disabling normalization for the whole book without any signal: the same identity-factor failure #649 removed from the aggregate reader itself, reintroduced one layer up. Confirmed with the exact reproduction supplied: `_normalized_position_cost(900000, "TWD", {"currency_meta": {"mixed": True, "currencies": ["TWD", "USD"], "fx": {"TWD": 0.031}}})` returned `900000.0` (raw) instead of `27900.0` (converted). Both of the engine's two current real producers (`trade_recap.build_state`, `snapshot_adapter`) always populate `aggregate_currency` explicitly, and the #664 E2E test's real CLI card carries it too (verified: `plan["engine_card"] ["currency_meta"]["aggregate_currency"] == "USD"` on that fixture) -- so this gap was not reachable through either documented production route. It is reachable through the `--card-json`/`--state-json` adapter/testing lane (review.py's own help text for those flags), and defensively through any future producer, which is why the existing unit tests -- which all supplied `aggregate_currency` explicitly -- never exercised it. card_renderer._currency(card) is already the single reader for "what is this card's aggregate currency," with its own documented `or "USD"` fallback (card_renderer.py:842). _normalized_position_cost now calls it instead of re-deriving the aggregate with a different, divergent default, collapsing to one reader per docs/development-guide.md's "two readers, one fact" discipline for this specific value. _exit_importance has the same latent divergent-fallback pattern; left untouched here as a separate, pre-existing surface out of this issue's scope. Test changes: - test_ticker_importance_ranking_uses_engine_fx_for_mixed_currency_amounts and test_ticker_importance_refuses_rather_than_falls_back_to_raw_units_on_missing_fx now each cover two currency_meta shapes -- no aggregate_currency key, and the explicit key -- rather than only the explicit-key shape. - The E2E test now asserts, rather than assumes, that ticker_diagnosis omits all four test tickers (so the position_cost fallback this fix touches is actually exercised) and that its own currency_meta does carry the explicit key (so its docstring's claim about what it can and cannot catch is verified, not asserted in prose alone). Mutation-verified: reverting the aggregate lookup to the divergent fallback reddens exactly the two direct _ticker_importance tests' "no key" cases and leaves the E2E/queue tests green -- confirming those are the only tests that exercise this specific shape, matching the reasoning above. Co-Authored-By: Claude Fable 5 --- skills/fomo-kernel/engine/review.py | 11 +++- tests/test_review_v2.py | 79 +++++++++++++++++++++++------ 2 files changed, 73 insertions(+), 17 deletions(-) diff --git a/skills/fomo-kernel/engine/review.py b/skills/fomo-kernel/engine/review.py index 90536e39..c6cfc48d 100644 --- a/skills/fomo-kernel/engine/review.py +++ b/skills/fomo-kernel/engine/review.py @@ -2918,10 +2918,19 @@ def _normalized_position_cost(cost, currency, card): ``trade_recap.usd_view`` fails the whole review closed otherwise — so ``None`` here only guards a caller whose card/state did not go through that gate, which is exactly the shape a unit test exercises directly. + + The aggregate currency itself has exactly one reader, + ``card_renderer._currency`` -- its ``or "USD"`` fallback is the review's + own convention for "no aggregate declared," not this function's caller's + currency. Resolving a missing ``aggregate_currency`` key to the position's + *own* currency would make every position trivially "already the + aggregate" and silently turn off normalization for the whole book instead + of naming the gap -- the same identity-factor failure #649 removed from + the aggregate reader itself, reintroduced one layer up. """ meta = (card or {}).get("currency_meta") or {} currency = str(currency or "USD").upper() - aggregate = str(meta.get("aggregate_currency") or currency).upper() + aggregate = card_renderer._currency(card or {}) if not meta.get("mixed") or currency == aggregate: return abs(float(cost or 0)) factor = (meta.get("fx") or {}).get(currency) diff --git a/tests/test_review_v2.py b/tests/test_review_v2.py index 38450899..7201c83b 100644 --- a/tests/test_review_v2.py +++ b/tests/test_review_v2.py @@ -6767,35 +6767,61 @@ def test_ticker_importance_ranking_uses_engine_fx_for_mixed_currency_amounts(): native-currency face value -- directly across tickers, so a TWD position could outrank a larger USD one purely because TWD amounts carry more zeros. It must read `currency_meta.fx` (#649's already-resolved rate) - exactly like `_exit_importance` does, never the raw amount.""" - card = {"currency_meta": {"mixed": True, "aggregate_currency": "USD", - "fx": {"TWD": 0.0317}}, "ticker_diagnosis": []} - state = {"holdings": {"positions": { + exactly like `_exit_importance` does, never the raw amount. + + Two `currency_meta` shapes, both must produce the identical answer. The + first carries no `aggregate_currency` key at all -- reachable through the + `--card-json`/`--state-json` adapter lane, or any future/differently + shaped producer. `_normalized_position_cost` must resolve the aggregate + through the one shared reader, `card_renderer._currency` (its own + documented `or "USD"` fallback), never by defaulting to the *position's + own* currency -- that would make every position trivially "already the + aggregate" and silently turn normalization off for the whole book, the + identity-factor failure #649 removed from the aggregate reader itself, + reintroduced one layer up. The second shape is the explicit key both of + the engine's two current real producers (`trade_recap.build_state`, + `snapshot_adapter`) always populate it with, pinned so the common case + stays covered too. + """ + positions = { "TWX": {"cost": 900000.0, "currency": "TWD"}, "USX": {"cost": 30000.0, "currency": "USD"}, - }}} - tw_importance, tw_basis = review_engine._ticker_importance(card, state, "TWX") - us_importance, us_basis = review_engine._ticker_importance(card, state, "USX") - assert tw_basis == us_basis == "position_cost" - assert abs(tw_importance - 28530.0) < 1e-6, tw_importance - assert us_importance == 30000.0 - assert us_importance > tw_importance, \ - "a smaller USD position must outrank a larger raw-TWD one once normalized" + } + state = {"holdings": {"positions": positions}} + for label, meta in [ + ("no aggregate_currency key", {"mixed": True, "currencies": ["TWD", "USD"], + "fx": {"TWD": 0.0317}}), + ("explicit aggregate_currency", {"mixed": True, "aggregate_currency": "USD", + "currencies": ["TWD", "USD"], "fx": {"TWD": 0.0317}}), + ]: + card = {"currency_meta": meta, "ticker_diagnosis": []} + tw_importance, tw_basis = review_engine._ticker_importance(card, state, "TWX") + us_importance, us_basis = review_engine._ticker_importance(card, state, "USX") + assert tw_basis == us_basis == "position_cost", (label, tw_basis, us_basis) + assert abs(tw_importance - 28530.0) < 1e-6, (label, tw_importance) + assert us_importance == 30000.0, (label, us_importance) + assert us_importance > tw_importance, \ + f"{label}: a smaller USD position must outrank a larger raw-TWD one once normalized" def test_ticker_importance_refuses_rather_than_falls_back_to_raw_units_on_missing_fx(): """#664: a missing rate must never resolve to an identity factor of 1.0 -- the same "the identity factor is removed wherever currencies differ, not merely guarded upstream" discipline #649 holds for the aggregate reader. + Covers both `currency_meta` shapes -- no `aggregate_currency` key and the + explicit key -- since both must fail the same way once a rate is missing. This is exercised directly because the real pipeline cannot reach this state: a book the engine reports `mixed` already carries a complete `fx` map for every held currency, since `trade_recap.usd_view` refuses the whole review before a card/state with a gap could ever be built.""" - card = {"currency_meta": {"mixed": True, "aggregate_currency": "USD", "fx": {}}, - "ticker_diagnosis": []} state = {"holdings": {"positions": {"TWX": {"cost": 900000.0, "currency": "TWD"}}}} - importance, basis = review_engine._ticker_importance(card, state, "TWX") - assert importance is None and basis == "fx_unavailable", (importance, basis) + for label, meta in [ + ("no aggregate_currency key", {"mixed": True, "fx": {}}), + ("explicit aggregate_currency", {"mixed": True, "aggregate_currency": "USD", "fx": {}}), + ]: + card = {"currency_meta": meta, "ticker_diagnosis": []} + importance, basis = review_engine._ticker_importance(card, state, "TWX") + assert importance is None and basis == "fx_unavailable", (label, importance, basis) def test_custom_exit_reason_requires_the_users_words(): @@ -9768,6 +9794,18 @@ def test_initial_thesis_native_currency_label_and_normalized_cost_ranking_on_mix both inside the USD positions' range. The normalized ranking must interleave them (USOPEN1 > 3001.TW > USOPEN2 > 3002.TW), never group every TWD position ahead of every USD one by raw magnitude alone. + + This is the shape a real review actually produces: `trade_recap.main`'s + `currency_meta` literal always carries an explicit `aggregate_currency` + (verified below), so this test cannot by itself catch a card missing that + key -- `test_ticker_importance_ranking_uses_engine_fx_for_mixed_currency_amounts` + covers that shape directly. What only the real CLI proves is that + `build_state` actually threads `cur_map` end to end and that the + `position_cost` fallback -- not the already-normalized `ticker_diagnosis` + branch -- is the one doing the work: `_664_MIXED_BOOK_CLOSES` prices every + open position at exactly its own average cost, so realized+unrealized + impact is ~0 and `ticker_diagnosis` (which drops `abs(impact) < 1`) omits + all four of them, forced and asserted below rather than assumed. """ with tempfile.TemporaryDirectory() as tmp: env = _offline_engine_env(tmp) @@ -9781,6 +9819,15 @@ def test_initial_thesis_native_currency_label_and_normalized_cost_ranking_on_mix assert plan["route"] == "first_review" assert plan["engine_state"]["review_tier"]["tier"] == "behavioral", \ "this fixture must actually clear the thin-file suppression (#306)" + assert plan["engine_card"]["currency_meta"].get("aggregate_currency") == "USD", \ + "this fixture's own currency_meta carries the key -- the missing-key shape is covered separately" + + diagnosed = {row["ticker"] for row in plan["engine_card"].get("ticker_diagnosis") or []} + for ticker in ("3001.TW", "3002.TW", "USOPEN1", "USOPEN2"): + assert ticker not in diagnosed, \ + (f"{ticker} must be absent from ticker_diagnosis, or this test exercises the " + f"already-normalized pnl_impact branch instead of the position_cost fallback " + f"#664 fixes: {diagnosed}") initial = [q for q in plan["question_queue"] if q["kind"] == "initial_thesis"] assert initial, "a mixed-currency first review must still emit initial-thesis questions"