feat: Scheduled Execution (Task 1.1)

[digital-thinking]

Changes

SchedulerService (src/fin_trade/services/scheduler.py) — 425 lines

• APScheduler BackgroundScheduler with thread-safe singleton pattern
• Auto-runs portfolios on configured cadence (daily/weekly/monthly)
• Two modes per portfolio: auto-apply (trades execute immediately) or queue for review (pending trades)
• Persists state to data/state/scheduler.json (enabled portfolios, last run times)
• Robust error handling: catches all exceptions, logs them, continues scheduling
• Per-portfolio execution locks to prevent concurrent runs

Model Changes

• PortfolioConfig: added scheduler_enabled: bool and auto_apply_trades: bool
• PortfolioService: added save_config() method, empty state file guard

UI

• Portfolio Detail: scheduling section with enable/disable toggle, auto-apply toggle, run now button, next/last run display
• Dashboard: scheduler status indicator, active schedule count, upcoming runs

Tests (tests/test_scheduler.py) — 7 tests

• start/stop, enable/disable, frequency scheduling, auto-apply, queue mode, error handling, state persistence

+895 lines across 11 files

[digital-thinking]

Technical Review of PR #6 - Scheduled Execution

Assumption: PR #6 compares feature/scheduled-execution against origin/master.

🚨 Critical Issues Found

1. Singleton conflicts with st.cache_resource

Evidence: SchedulerService singleton pattern in src/fin_trade/services/scheduler.py:38-85 plus st.cache_resource in src/fin_trade/app.py:38-52.

Problem: The cached get_services() can rebuild SecurityService/PortfolioService/AgentService, but SchedulerService.__init__ short-circuits on _initialized, so it never picks up the new instances. This is a real risk in Streamlit re-runs/hot reloads and in tests that expect fresh service graphs.

Fix: Pick one singleton mechanism. Remove the singleton from SchedulerService (remove __new__, _instance, _initialized, reset_instance_for_tests()) and rely on st.cache_resource.

Tests: Update tests/test_scheduler.py to stop calling SchedulerService.reset_instance_for_tests().

2. Non-atomic file writes in _save_state_locked

Evidence: Direct write with open(..., "w") and json.dump in src/fin_trade/services/scheduler.py:195-204.

Problem: If the process dies mid-write, the file can be truncated or empty, losing all enabled/last-run state.

Fix: Write to temp file and atomically replace:

tmp = state_path.with_suffix(".tmp")
with open(tmp, "w") as f:
    json.dump(data, f)
    f.flush()
    os.fsync(f.fileno())
os.replace(tmp, state_path)

Tests: Add unit test simulating partial write with pre-created empty file.

3. Missing error logging for debate/langgraph failures

Evidence: In src/fin_trade/services/scheduler.py:275-301, the except path only logs to ExecutionLogService when agent_mode == "simple".

Problem: If DebateAgentService.execute() or LangGraphAgentService.execute() raises, no execution log entry is recorded for analytics/UI.

Fix: Log failures for all agent modes: agent_mode=config.agent_mode, success=False, error_message=str(exc), num_trades=0.

Tests: Add tests for agent_mode="debate" and agent_mode="langgraph" with execute() raising, assert execution_log_service.log_execution called with success=False.

Summary

These issues affect data integrity, dependency injection consistency, and error visibility. All should be addressed before merge.

Tokens used: 30,067 (Codex analysis)

[digital-thinking]

File "...fin_trade\pages\dashboard.py", line 144, in render_dashboard_page
elif item.next_run <= now:
^^^^^^^^^^^^^^^^^^^^
TypeError: can't compare offset-naive and offset-aware datetimes