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

There are no linked issues or acceptance criteria provided, so I will assess the changes based on their own merits. The changes primarily involve enhancements to the load_latest_prices and load_live_prices functions in holding_utils.py, particularly around handling currency conversions and scaling. The addition of the _is_pence_currency function improves clarity and correctness in determining pence-denominated currencies. Overall, the changes appear to enhance the functionality without introducing any regressions.

2. Bugs and logic errors

The logic for handling pence-denominated currencies has been encapsulated in the _is_pence_currency function, which is a good improvement. However, I would recommend adding unit tests specifically for this function to ensure it behaves as expected across all edge cases. The changes to the load_latest_prices and load_live_prices functions seem logically sound, but it’s crucial to ensure that the handling of currency conversion is thoroughly tested, especially since financial applications are sensitive to such calculations.

3. API, data, and workflow safety

The changes do not appear to break the existing backend/frontend contracts. The output structure of the load_latest_prices and load_live_prices functions remains consistent with what the frontend expects. The addition of the get_instrument_meta mock in the tests suggests that the new logic will be validated against expected behavior. However, it would be prudent to run integration tests to ensure that the entire workflow remains intact, especially since financial data is involved.

4. Test coverage

The test coverage has been improved with the addition of tests for the load_latest_prices function, specifically checking for the new currency handling logic. However, it would be beneficial to add dedicated tests for the _is_pence_currency function to ensure it handles all expected inputs correctly. Additionally, tests should cover edge cases, such as unexpected currency codes or missing data.

5. Minor issues (optional)

The code style is consistent with existing conventions, and the comments are clear and informative. However, the comment about preserving the raw metadata string for mixed-case pence detection could be more explicit about why this is necessary. This would help future maintainers understand the rationale behind the implementation.

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. Evaluating on its own merits. The PR appears to be a cleanup/refactor: extracting _is_pence_currency, removing dead/duplicate code, fixing a type annotation, improving live price pence handling, adding resp.raise_for_status(), and patching a docstring-after-code bug.

---

2. Bugs and logic errors

Serious: units variable removed before use

In enrich_holding, the line:

units = float(out.get(UNITS, 0) or 0)  # removed at ~line 517

is deleted. However, units is used later at lines ~607–608:

mv = round(units * float(px), 2)

and earlier in the cash branch at line ~403. The cash branch assignment is earlier and still present, but for non-cash holdings the units local variable was set right before px = px_source = .... If there was a pre-existing units binding earlier in enrich_holding from before the diff context, this may be fine — but if that was the only assignment for non-cash paths, this is a silent NameError or stale-value bug. This needs verification against the full function body.

load_live_prices: pence detection now applied but load_latest_prices had it guarded differently

In load_latest_prices, the pence /100 only applies when scale == 1. The same guard is correctly applied in the new load_live_prices path — good. But note: the old load_live_prices code only did price *= scale and then FX-converted if ccy != "GBP", meaning GBX instruments were not being divided by 100 unless get_scaling_override returned 0.01. The new code adds the /100 path, which is a behaviour change. If any instruments have scale == 0.01 and currency GBX, they'll now be divided by 100 twice (once via price *= 0.01, then... no wait — _is_pence_currency guard is if scale == 1, so the /100 only fires when scale is 1). Actually this is consistent. But it's a silent behaviour change for live prices that isn't tested.

get_instrument_meta fallback added silently

meta = get_instrument_meta(full_ticker) or get_instrument_meta(ticker) or {}

The fallback to bare ticker (no exchange) is new. If get_instrument_meta is keyed differently, this could return a wrong instrument's metadata (e.g., "BARC" matching a different exchange's record). No test covers this path.

Missing newline at EOF

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

The diff introduces a missing newline. This will cause a linting failure in most CI pipelines and is the opposite of cleanup.

Docstring after code (fixed correctly)

The swap of the docstring and the CASH guard in _get_price_for_date_scaled is a real bug fix — the docstring was unreachable. Good catch.

---

3. API, data, and workflow safety

Payload shapes: out["price"] = px loses the comment # legacy name used in parts of UI — functionally identical, no regression.

resp.raise_for_status(): Good addition. However, Yahoo Finance's v7 endpoint frequently returns 200 with an error payload. The raise_for_status() only catches HTTP-level errors; malformed payloads are already handled by the outer except Exception. No regression here.

_is_pence_currency is module-level but untested directly: The logic raw.upper() in {"GBX", "GBXP"} or raw == "GBp" is correct per the docstring. However, raw.upper() in {"GBX", "GBXP"} already handles "GBp" via uppercasing... wait, no: "GBp".upper() == "GBP", not "GBX". The raw == "GBp" special case is correctly necessary and the logic is sound.

Import comment change: # type: ignore → # local import to avoid circular is fine and more informative.

---

4. Test coverage

New test covers FX conversion path (USD.L → 80.0): Good addition. The _fx_to_base monkeypatch returning 0.8 and asserting 100.0 * 0.8 = 80.0 is correct.

Missing test coverage:

• _is_pence_currency has no direct unit test (trivial but worth having given the mixed-case subtlety).
• Live prices pence behaviour change has no test.
• get_instrument_meta fallback-to-ticker path has no test.
• The units removal in enrich_holding has no regression test.

Existing test test_load_latest_prices_resolution_scaling_and_missing: The scaling_override for "USD" returns 1.0 and Close_gbp is absent, so the FX path fires. The assertion USD.L: 80.0 is correct. However, the test doesn't cover the case where get_instrument_meta(full_ticker) returns None and the fallback get_instrument_meta(ticker) is needed — i.e., the new fallback logic is untested.

---

5. Minor issues

• Missing newline at EOF (line ~639): introduced by this PR — must be fixed.
• Removed comments explaining why certain things exist (e.g., # legacy name used in parts of UI, # keep both spellings, # last resort). These had real documentation value for future readers.
• _lower_name_map is unchanged but the PR removes the blank line between imports in

---

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