rebase

[leonarduk]

No description provided.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[github-actions]

GPT AI Code Review

1. Acceptance criteria

The diff does not reference any specific acceptance criteria since there is no linked issue. However, the changes made in the code appear to enhance the handling of currency conversions, particularly for pence-denominated currencies (GBX, GBXP, GBp) and ensure that prices are consistently returned in GBP. The modifications to the load_latest_prices and _close_on functions align with the goal of preserving portfolio and compliance correctness by ensuring accurate currency conversions. Overall, the changes seem to meet the implicit acceptance criteria of maintaining correctness and consistency in the handling of financial data.

2. Bugs and logic errors

There are no apparent bugs or logic errors in the changes. The introduction of the _is_pence_currency function is a clear improvement, allowing for better handling of pence-denominated currencies. The logic for converting prices to GBP is well-structured, and the checks for NaN and infinite values are appropriately handled. The code also avoids double-conversion of currencies, which is crucial for maintaining accuracy in financial calculations.

3. API, data, and workflow safety

The changes made in the backend should not break the existing frontend/backend contracts, as the output structure of the load_latest_prices and _close_on functions remains consistent. The modifications do not introduce any new fields or alter the expected data shapes, which is essential for maintaining alignment between the backend and frontend. There are no changes that would affect local smoke tests, deployment workflows, or repository scripts. The handling of secrets and permissions appears to be unaffected by these changes.

4. Test coverage

The test coverage has been improved with the addition of new tests that validate the behavior of the modified functions. The tests for load_latest_prices and _close_on ensure that the new currency conversion logic is exercised. Specifically, the test test_load_latest_prices_converts_native_close_to_gbp and test_close_on_converts_native_currency_to_gbp validate that native currency prices are correctly converted to GBP, which is a critical aspect of the changes. However, it would be beneficial to have additional tests that cover edge cases, such as handling unexpected currency codes or missing data.

5. Minor issues (optional)

The code style is consistent with existing practices, and the naming conventions are clear. The comments added to the code enhance readability and understanding of the logic, particularly around currency conversion. There are no significant minor issues that would cause future confusion.

Summary Verdict: APPROVE

---

Reviewed by GPT via gpt-pr-review.yml. Advisory only.

[github-actions]

Claude AI Code Review

Code Review: rebase PR

1. Acceptance criteria

No linked issue. Reviewing on merits: this appears to be a refactor/cleanup pass over holding_utils.py, prices.py, and associated tests, extracting _is_pence_currency, fixing load_live_prices to apply the pence-conversion logic (previously only load_latest_prices did it), correcting a docstring bug (misplaced code before docstring), and adding FX conversion to _close_on.

Scope looks coherent. Nothing obviously missing relative to what the code claims to do.

---

2. Bugs and Logic Errors

load_live_prices — pence conversion is new behaviour, not just a refactor

In load_latest_prices, pence conversion was already present (inline). In load_live_prices, the old code only applied scale * price and then FX conversion. The new code additionally applies the pence-to-GBP /100 for GBX/GBXP/GBp instruments when scale == 1. This is likely correct, but it is a behaviour change, not a no-op cleanup. Any live-price consumer (e.g. snapshot polling) will now return different values for pence-denominated instruments that previously weren't divided by 100. There are no tests for load_live_prices covering this case — only load_latest_prices and _close_on get the new pence tests.

_close_on — FX cache is discarded every call

value *= _fx_to_base(currency, "GBP", {})

A fresh empty dict {} is passed on every call, so FX rates are re-fetched for every ticker. This was already the pattern in the old code for this function and may be intentional, but now that _close_on is used more heavily (it's called inside get_price_snapshot loops), it's worth noting the repeated network cost.

_close_on — adj close / adj_close fallback could silently return an adjusted price without pence conversion

The column preference order is:

name_map.get("close_gbp") or name_map.get("close") or name_map.get("adj close") or name_map.get("adj_close")

If close_gbp is absent, and the feed returns only Adj Close, the pence conversion is applied because close_col.lower() != "close_gbp". That's correct. No issue here — just worth noting it's intentional.

enrich_holding — units variable removed mid-function

# old code at line ~516:
units = float(out.get(UNITS, 0) or 0)

This line is deleted in the diff. units is used below (e.g., mv = round(units * float(px), 2) at ~line 608). Where is units defined now? Searching the context: units is assigned earlier in enrich_holding (not shown in the diff), and the removed line was a re-assignment at that point in the function. If nothing between the earlier assignment and the usage mutates out[UNITS], this is safe. But the removal is silent and needs verification that no mutation of out[UNITS] happens between the original assignment and the point where this re-read was occurring. This is a latent correctness risk.

Missing newline at end of file

-    return f"Cash ({account_ccy})"
+    return f"Cash ({account_ccy})"
\ No newline at end of file

This actively introduces a missing-newline warning and will cause pre-commit/linting failures if EOL checks are enabled.

_get_price_for_date_scaled — docstring/code ordering fix is correct

The old code had executable statements before the docstring (a real bug — the early-return if "CASH" in parts ran but the docstring was unreachable as a string literal). The fix moves the docstring before the code. This is a correctness fix, not cosmetic.

---

3. API, Data, and Workflow Safety

Frontend contract: no shape changes observed. The fields price, current_price_gbp, market_value_gbp, gain_gbp, unrealised_gain_gbp, unrealized_gain_gbp, gain_pct, day_change_gbp, cost_basis_source are all still emitted. Safe.

resp.raise_for_status() added to load_live_prices

resp.raise_for_status()

This is inside a try/except Exception block, so HTTP 4xx/5xx errors are now caught and logged as warnings rather than silently returning partial data from a malformed response. Improvement, but it means a Yahoo Finance API hiccup that previously returned an empty payload (and silently produced no prices) now logs a warning. Verify that callers tolerate an empty dict return without raising.

# type: ignore removed from _fx_to_base imports

Both load_latest_prices and load_live_prices previously suppressed type errors on these imports. Removing the suppression is fine if the type stubs are now correct, but if CI runs mypy in strict mode this could introduce new failures.

---

4. Test Coverage

Missing: load_live_prices pence conversion test. The new pence-conversion logic in load_live_prices has no test. Given that this is a behaviour change (not present in old code), a test like the one added for load_latest_prices is needed.

New test test_close_on_converts_native_currency_to_gbp: correct and valuable.

New test test_load_latest_prices_converts_native_close_to_gbp: correct.

test_last_close_fallback_snapshot_does_not_double_convert_fx: The name suggests it tests double-conversion prevention, but the monkeypatched _close_on returns 80.0 directly (already GBP), so it doesn't actually exercise

---

Reviewed by Claude via claude-pr-review.yml. Advisory only.