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 cannot assess whether the diff satisfies any specific requirements. The PR title "rebase" does not provide clarity on the intent or scope of the changes made. It would be beneficial to have a more descriptive title or a summary of the changes to understand the purpose of this PR.

2. Bugs and logic errors

• In backend/app.py, the change on line 90 introduces a new condition for the authentication check. The addition of cfg.disable_auth could lead to unintended access if not properly managed. Ensure that this configuration is well-documented and that its implications are clear.
• In backend/common/data_loader.py, the logic around repo_path has been modified. The new logic could potentially lead to issues if the repo_root is not set correctly. Ensure that the fallback logic is well-tested.
• In backend/common/portfolio_utils.py, the handling of price gaps has been modified. The introduction of _MAX_PRICE_GAP_FILL_DAYS on line 1668 is a good addition, but ensure that the logic for filling gaps does not inadvertently propagate stale prices beyond the intended limit.

3. API, data, and workflow safety

• The changes in the backend appear to maintain the existing API contracts, but without a clear understanding of the expected payloads, it's hard to guarantee that the frontend will remain aligned. Ensure that any changes to data structures are communicated to the frontend team.
• The modifications in frontend/src/ConfigContext.tsx on lines 135-191 change how the configuration is set. The removal of dependencies in the useEffect hook could lead to stale values being used. This should be tested to ensure that the configuration updates as expected.
• The changes in the test files, particularly in tests/backend/test_drawdown_reconciliation.py, introduce new tests that seem to cover edge cases. However, ensure that these tests are comprehensive enough to cover all scenarios, especially around price gap handling.

4. Test coverage

• The new tests added in tests/backend/test_drawdown_reconciliation.py for handling price gaps are a positive addition and help ensure that the new logic is validated.
• However, there is no indication that the changes in frontend/src/ConfigContext.tsx are covered by tests. It would be prudent to add tests to ensure that configuration changes are handled correctly and that the application behaves as expected when the configuration is updated.

5. Minor issues (optional)

• The PR title "rebase" is not descriptive. A more informative title would help reviewers understand the purpose of the changes.
• Consider adding comments in the code where the logic has changed significantly to aid future developers in understanding the rationale behind those changes.

Summary Verdict: REQUEST CHANGES. The PR requires a more descriptive title, additional tests for the frontend changes, and a review of the implications of the new authentication logic.

---

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 as instructed.

---

2. Bugs and Logic Errors

_active_instruments_dir() logic is broken (instruments.py, ~line 39)

def _active_instruments_dir() -> Path:
    if _INSTRUMENTS_DIR != _DEFAULT_INSTRUMENTS_DIR or _INSTRUMENTS_DIR.exists():
        return _INSTRUMENTS_DIR
    return _resolve_instruments_dir()

Since _DEFAULT_INSTRUMENTS_DIR = _INSTRUMENTS_DIR is assigned immediately after, these are always equal at module load time, so the condition _INSTRUMENTS_DIR != _DEFAULT_INSTRUMENTS_DIR is always False. The short-circuit then relies entirely on _INSTRUMENTS_DIR.exists(). If the directory doesn't exist, _resolve_instruments_dir() is called again — but that function just re-derives the same path. This is a no-op disguised as dynamic resolution. Either the intent was to support runtime patching of _INSTRUMENTS_DIR (in which case the comparison logic is backwards — you'd want != to mean "was patched"), or this whole mechanism solves nothing.

load_person_meta fallback logic change is semantically broader than intended (data_loader.py, ~line 1022)

if not explicit_local_fallback:
    return {}

Previously: if config.app_env == "aws" and not has_local_fallback. The new condition drops the app_env == "aws" guard entirely on S3 errors. In a local development environment where S3 isn't configured but data_root was not explicitly passed, an S3 exception will now silently return {} instead of falling through to local file loading. This could mask misconfigurations in non-AWS environments.

reports.py fallback logic is convoluted and potentially double-invokes _portfolio_snapshot (reports.py, ~line 663)

fallback_portfolio = self._portfolio
if fallback_portfolio is None and _DEFAULT_PORTFOLIO_SNAPSHOT is not None and _portfolio_snapshot is not _DEFAULT_PORTFOLIO_SNAPSHOT:
    fallback_portfolio = _portfolio_snapshot(self.owner, pricing_date=self.end) or None
    self._portfolio = fallback_portfolio or self._portfolio
self._owner_portfolio = fallback_portfolio or None

The condition _portfolio_snapshot is not _DEFAULT_PORTFOLIO_SNAPSHOT checks if the module-level function has been monkey-patched. This is fragile: it only works if tests/callers replace the module-level name, not if they inject via dependency. More critically, self._portfolio could already be None, making self._portfolio = fallback_portfolio or self._portfolio still None, and the or chaining on the final line then yields None anyway. The net effect: _owner_portfolio can still be None in all the same cases as before, but with more complexity.

_portfolio_value_series refactor — reporting_date vs effective_days boundary (portfolio_utils.py, ~line 1337)

values = values[values.index <= calc.reporting_date]

This filter is applied per-series before ffill. The subsequent total = total[total.index <= calc.reporting_date] after the concat is now redundant but harmless. However, filtering before ffill means that if the last known price for a ticker falls exactly on reporting_date - 1 and there's a gap, ffill can still fill reporting_date — but if it falls before reporting_date - _MAX_PRICE_GAP_FILL_DAYS, it won't. The existing test test_portfolio_value_series_does_not_forward_fill_long_gaps tests idx[5] but the gap is indices 2–7 (6 days), which is > 5. The test asserts idx[5] is 520.0 (STEADY=100 + GAPPY forward-filled=420), but 420 would require filling 4 days from idx[1] — that's within the 5-day limit. idx[7] asserts 100.0 (STEADY only), which is 6 days out — correct. The test logic holds, but only by a narrow margin that should be documented.

resolve_paths fix has subtle regression (data_loader.py, ~line 147)

repo_path = Path(repo_root).expanduser() if repo_root else Path(__file__).resolve().parents[2]
if not repo_path.exists():
    repo_path = Path(__file__).resolve().parents[2]

Old code: if repo_root is provided but doesn't exist, fall back to parents[2]. New code: same final behaviour, but the intermediate expanduser() call now happens even on the non-existent path before the existence check. This is harmless but means expanduser is called twice on the fallback path (once returning a non-existent path, once via the fallback). More importantly, if someone passes a non-existent repo_root intentionally (e.g., in a test to trigger fallback), behaviour is preserved — fine.

_build_owner_summary removes "owner" from name key lookup (data_loader.py, ~line 297)

- for key in ("full_name", "display_name", "preferred_name", "owner", "name"):
+ for key in ("full_name", "display_name", "preferred_name", "name"):

Removing "owner" as a display name fallback key. If existing data files use owner as the display name field, this silently drops the display name. No migration note, no test coverage for this case.

---

3. API, Data, and Workflow Safety

app.py auth change — cfg.disable_auth bypass (app.py, line 93)

if allowlist_raw and not (cfg.disable_auth or os.getenv("TESTING")):

cfg.disable_auth is a config-file field that bypasses the email allowlist entirely. This is dangerous: if disable_auth is accidentally set to a truthy value in a production config (or through config inheritance/flattening bugs), the allowlist is silently ignored with no log warning. At minimum this should emit a loud warning log when disable_auth is active in non-TESTING environments. The _flatten_dict change in config.py (using setdefault) could interact with this — if a legacy config has disable_auth: true at the top level, setdefault now preserves it rather than being overwritten.

_flatten_dict setdefault change (config.py, ~line 200)

The semantics flip: previously the last value wins; now the first value wins. This is described as "section-based values parsed earlier take precedence over legacy top-level entries." This needs careful validation — any config file relying on top-level overrides to override section values will silently break. There's no test covering this priority inversion.

trail.json checked into repo without newline

-{"demo": ...}
+{"demo": ...}
\ No newline at end of file

A binary data file with test/demo state is tracked in the repo and modified. This is a workflow smell — this file should either be gitignored or managed separately. The missing newline is a minor formatting issue but indicates it may be written by code rather than edited by hand.

test_main.py fixture now uses yield but os.environ["TESTING"] is never cleaned up

os.environ["TESTING"] = "1"
previous = backend_config.offline_mode
backend_config.offline_mode = True
...
try:
    yield client
finally:
    backend_config.offline_mode = previous

offline_mode is restored, but TESTING=1 leaks into subsequent tests in the same process. Should use monkeypatch.setenv("TESTING", "1") or restore it in finally.

---

4. Test Coverage

Good additions:

• test_compute_max_drawdown_forward_fills_short_price_gaps and test_portfolio_value_series_does_not_forward_fill_long_gaps properly exercise the new ffill behaviour with _MAX_PRICE_GAP_FILL_DAYS.
• ConfigContext test for "does not refetch config when base currency changes locally" is a valuable regression guard.
• monkeypatch.setattr(portfolio_utils, "_PRICE_SNAPSHOT", ...) replacing direct mutation is correct.

Missing coverage:

• No test for cfg.disable_auth bypassing allowlist.
• No test for _flatten_dict priority inversion (section values win over top-level).
• No test for _build_owner_summary with a data file using owner as the display name key.
• No test for _active_instruments_dir() — the function's logic appears untestable as written.
• No test for load_person_meta new early-return behaviour outside AWS env.

---

5. Minor Issues

• ConfigContext.tsx: removing blank line between state initialization and setRelativeViewEnabled is cosmetic noise in the diff.
• test_allowed_emails_loaded_lowercase: the defensive raw if isinstance(raw, list) else [raw] handles a single-string config value, but the existing YAML likely always has a list — this suggests the config was once malformed or there's a real edge case that should be documented.
• _MAX_PRICE_GAP_FILL_DAYS = 5 is defined after it's used in _portfolio_value_series (line ~1340 vs ~1691). Python resolves this at call time not definition time, so it works, but it's confusing to read.

---

REQUEST CHANGES — The disable_auth bypass has no warning log and interacts dangerously with the setdefault config flattening change; _active_instruments_dir() logic is broken by construction; TESTING env var leaks from test fixture; and the removal of "owner" from display name keys is unguarded.

---

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