rebase

[leonarduk]

No description provided.

[github-actions]

GPT AI Code Review

1. Acceptance criteria

The PR does not have any linked issue or acceptance criteria, but the changes made in the code appear to be well-scoped and complete. The new sections for portfolio reporting (overview, sectors, regions, concentration, and VaR) are clearly defined and implemented. The addition of these sections aligns with the goal of enhancing the reporting capabilities of the application.

2. Bugs and logic errors

The code appears to be logically sound, with appropriate error handling for cases where the portfolio or risk modules are not available. However, there are a few areas to consider:

• In the _build_portfolio_var_section function, if risk_mod is None, it returns an empty list. This is acceptable, but it would be beneficial to log a warning or error message to indicate that the risk module is not available, which could help in debugging.
• The calculation of weight_pct in the _build_portfolio_sectors_section and _build_portfolio_regions_section functions could potentially lead to a division by zero if total_value is zero. While the current implementation handles this by checking if total_value is truthy, it may be clearer to explicitly handle this case with a conditional statement.

3. API, data, and workflow safety

The changes made in the backend should not break the existing frontend/backend contracts, as the new report sections are added without altering existing data structures. The new sections are well-defined and should integrate smoothly with the existing reporting framework. However, it is essential to ensure that the frontend is updated to handle these new sections correctly, as there are no changes in the payload shapes that would cause issues.

There are no apparent risks to local smoke tests, deployment workflows, or repository scripts. The code does not expose any secrets or permissions issues.

4. Test coverage

The new test case test_portfolio_section_builders thoroughly exercises the new functionality added in the PR. It covers the construction of various portfolio sections and checks for expected values, which is excellent. However, it would be beneficial to add tests for edge cases, such as:

• What happens when the portfolio has no accounts or holdings?
• How does the system behave if the build_owner_portfolio function raises an unexpected exception?

5. Minor issues (optional)

• The naming conventions are consistent and clear, which aids in readability. However, consider adding docstrings to the new functions to describe their purpose and parameters, as this will improve maintainability.
• The use of or "Unknown" in several places is a good practice for handling missing data, but it might be worth considering a more structured approach to handle defaults, such as using a constants file for default values.

Summary Verdict: APPROVE

---

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

[github-actions]

Claude AI Code Review

PR Review: "rebase"

1. Acceptance criteria

No linked issue. Reviewing on merits. The PR bundles three distinct concerns:

1. CASH.GBP synthetic close fix in portfolio_utils.py and risk.py
2. IAM least-privilege refactor in backend_lambda_stack.py
3. Test updates/additions for the above

All three appear internally consistent and the changes are self-described. No obvious gaps relative to the stated intent.

---

2. Bugs and logic errors

portfolio_utils.py — compute_owner_performance (lines ~967–987)

The NaN-dropping behavior for CASH.GBP creates an index alignment hazard. For non-cash tickers, closes.index = df["Date"] is assigned after closes = pd.to_numeric(...), giving closes a RangeIndex at assignment time but then overwriting it. For CASH.GBP, closes = pd.Series(1.0, index=df["Date"]) is correct. However, the closes.dropna() call on the CASH.GBP branch drops nothing (all 1.0), but the non-cash branch drops NaNs from a series indexed by date — the total.add(values, fill_value=0) then aligns by date index, which is correct. This works but is fragile: if apply_scaling ever returns a frame with a non-default index, the closes.index = df["Date"] assignment silently misaligns if lengths differ (e.g. scaling drops rows). Consider asserting len(closes) == len(df) before the index assignment.

risk.py — ordering of null checks (lines ~167–184)

ts = portfolio_utils.load_meta_timeseries(sym, exch, days)
if ts is None or ts.empty:
    continue
scale = portfolio_utils.get_scaling_override(sym, exch, requested_scaling=None)
ts = portfolio_utils.apply_scaling(ts, scale)
if ticker.strip().upper() == "CASH.GBP":
    ts = ts.copy()
    ts["Close"] = 1.0
var_single = portfolio_utils.compute_var(ts, confidence=confidence)
is_cash = ticker.strip().upper() == "CASH.GBP"

ticker.strip().upper() == "CASH.GBP" is evaluated twice — no bug, but is_cash could be hoisted before the first use. Minor.

More importantly: apply_scaling is called on CASH.GBP before the ts["Close"] = 1.0 override, which is correct (scaling is irrelevant since it's immediately overwritten), but the ordering is misleading. A future maintainer might assume scaling is applied to the synthetic series. The comment doesn't flag this.

risk.py — closes computed from already-overridden ts

After ts["Close"] = 1.0 for CASH.GBP, closes = pd.to_numeric(ts["Close"], errors="coerce").dropna() will produce an all-1.0 series. compute_var on a flat series returns None (no variance), which is then caught by if var_single is None and is_cash: var_single = 0.0. This is correct. But the market_value_gbp lookup that follows:

closes = pd.to_numeric(ts["Close"], errors="coerce").dropna()

...this closes is used to compute current_price = float(closes.iloc[-1]) and then contribution = var_single * current_price * units. For CASH.GBP with var_single = 0.0, contribution is 0.0 * 1.0 * units = 0.0, which is correct. ✓

test_portfolio_utils_returns.py — CASH.GBP value assertion

The new expectation [11.0, 12.0] (line ~290): with units=1, CASH.GBP synthetic close is 1.0, so CASH contributes 1.0/day. NORM.L contributes 10.0 and 11.0. Sum = 11.0, 12.0. ✓ Consistent with the implementation.

test_compute_owner_performance_drops_partial_close_nans (new test)

The test asserts dates ["2024-01-01", "2024-01-03"] and values [21.0, 23.0]. Date 2024-01-02 is dropped because NAN.L has a NaN close on that date, making its contribution NaN, so after total.add with fill_value=0, date 2024-01-02 would be 0 + 1.0 (cash) = 1.0 — wait, that's not dropped.

Actually, closes.dropna() on NAN.L drops the 2024-01-02 entry entirely from that series before values = closes * units. Then total.add(values, fill_value=0) will include 2024-01-02 with only the cash contribution of 1.0. But the test expects 2024-01-02 to be absent from output. This needs verification — does the final filtering step drop single-day partial sums? Looking at the broader context: there's a test_compute_owner_performance_filters_single_day_zero which suggests there's a post-processing filter. If the filter only drops zero-value days, day 2024-01-02 with value 1.0 (from cash alone) would not be filtered, and the test would fail.

This is a potential bug in the test or the implementation. The test asserts 2024-01-02 is absent but the cash synthetic close of 1.0 × 1 unit = 1.0 should appear for that date unless the post-processing specifically requires all holdings to contribute on a given date. Needs investigation before merge.

---

3. API, data, and workflow safety

IAM refactor — backend_lambda_stack.py

The old code called `data_bucket

---

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