diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json new file mode 100644 index 00000000..712a9c5a --- /dev/null +++ b/.claude-plugin/marketplace.json @@ -0,0 +1,19 @@ +{ + "name": "lalit-tools", + "owner": { + "name": "Lalit Nayyar", + "email": "lalitnayyar@gmail.com" + }, + "plugins": [ + { + "name": "independent-reviewer", + "source": "./independent-reviewer", + "version": "1.0.0", + "description": "carry out an independent review of all changes since last commit", + "author": { + "name": "Lalit Nayyar" + } + + } + ] +} \ No newline at end of file diff --git a/.claude/agents/change-reviwer.md b/.claude/agents/change-reviwer.md new file mode 100644 index 00000000..b29bf36d --- /dev/null +++ b/.claude/agents/change-reviwer.md @@ -0,0 +1,13 @@ +--- +name: change-reviewer +description: carry out the compehensive review of all changes since the last commit. +--- + +This subagent reviews all the changes since the last commit using shell command. +IMPORTANT: you should not review the changes by yourself, but rather , you should run the following shell command to kick of codex-codex is a separate AI Agent that will carry out the independent review. +Run this shell command: +'codex exec "Please review all changes since the last commit and write feedback to planning/REVIEW-last-commit.md"' +This will run the review process and save the results. + +Do not review yourself. + diff --git a/.claude/agents/codex-reviewer.md b/.claude/agents/codex-reviewer.md new file mode 100644 index 00000000..4e84c6b8 --- /dev/null +++ b/.claude/agents/codex-reviewer.md @@ -0,0 +1,10 @@ +--- +name: codex-reviewer +description: carry out a comprehensive review of PLAN.md when requested using codex +--- + +You are using a different AI Agent to carry out a review of the doucment: planning/PLAN.md. +You MUST execute the following shell command to carry out the review- do not review yourself: +'codex exec "Please review the file planning/PLAN.md and write your review in a file called planning/REVIEW-CODEX.md"' +This will run review process and save the results. +Do not review yourself. \ No newline at end of file diff --git a/.claude/agents/reviewer.md b/.claude/agents/reviewer.md new file mode 100644 index 00000000..84576ad2 --- /dev/null +++ b/.claude/agents/reviewer.md @@ -0,0 +1,6 @@ +--- +name: reviewer +description: carry out a comprehensive review when requested. +--- + +You review the file planning/PLAN.md and write your feedback to planning/REVIEW.md. Focus on the clarity of the plan, potential issues, and suggestions for improvement. \ No newline at end of file diff --git a/.claude/commands/doc-review.md b/.claude/commands/doc-review.md new file mode 100644 index 00000000..34b2d24b --- /dev/null +++ b/.claude/commands/doc-review.md @@ -0,0 +1 @@ +Review the documentation file in the planning folder called $ARGUMENTS and add questions, clarifications or feedback to a new section at the end, along with any opportunities to simplify \ No newline at end of file diff --git a/.claude/settings.json b/.claude/settings.json index aa06f43d..b4003302 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -2,6 +2,7 @@ "enabledPlugins": { "frontend-design@claude-plugins-official": true, "context7@claude-plugins-official": true, - "playwright@claude-plugins-official": true + "playwright@claude-plugins-official": true, + "independent-reviewer@lalit-tools": true } } diff --git a/.claude/settings.json_mod b/.claude/settings.json_mod new file mode 100644 index 00000000..2f1b69d2 --- /dev/null +++ b/.claude/settings.json_mod @@ -0,0 +1,13 @@ +{ + "hooks": { + "Stop": [ + { "hooks": [ + { + "type":"command", + "command": "codex exec \"Please review all changes since the last commit and write feedback to planning/REVIEW-last-commit.md\"" + } + ] + } + ] + } +} \ No newline at end of file diff --git a/.codex b/.codex new file mode 100644 index 00000000..e69de29b diff --git a/README.md b/README.md index 3f2582ae..79aa4070 100644 --- a/README.md +++ b/README.md @@ -1,62 +1,67 @@ # FinAlly — AI Trading Workstation -A visually stunning AI-powered trading workstation that streams live market data, simulates portfolio trading, and integrates an LLM chat assistant that can analyze positions and execute trades via natural language. +An AI-powered trading workstation with live market data, a simulated portfolio, and an LLM chat assistant that can analyze positions and execute trades. Built as a capstone project for an agentic AI coding course. -Built entirely by coding agents as a capstone project for an agentic AI coding course. +## What It Does -## Features +- Streams live prices for 10 default tickers (simulated by default, real data via Massive API) +- $10,000 virtual cash to trade with — buy/sell with market orders, instant fill +- Portfolio heatmap, P&L chart, positions table, and sparkline mini-charts +- AI chat assistant (powered by Cerebras via OpenRouter) that can discuss your portfolio and execute trades -- **Live price streaming** via SSE with green/red flash animations -- **Simulated portfolio** — $10k virtual cash, market orders, instant fills -- **Portfolio visualizations** — heatmap (treemap), P&L chart, positions table -- **AI chat assistant** — analyzes holdings, suggests and auto-executes trades -- **Watchlist management** — track tickers manually or via AI -- **Dark terminal aesthetic** — Bloomberg-inspired, data-dense layout +## Stack -## Architecture +- **Frontend**: Next.js (TypeScript, static export), Tailwind CSS +- **Backend**: FastAPI (Python, managed by `uv`), SQLite +- **Real-time**: Server-Sent Events (SSE) +- **AI**: LiteLLM → OpenRouter → Cerebras (`openai/gpt-oss-120b`) +- **Deployment**: Single Docker container on port 8000 -Single Docker container serving everything on port 8000: +## Setup -- **Frontend**: Next.js (static export) with TypeScript and Tailwind CSS -- **Backend**: FastAPI (Python/uv) with SSE streaming -- **Database**: SQLite with lazy initialization -- **AI**: LiteLLM → OpenRouter (Cerebras inference) with structured outputs -- **Market data**: Built-in GBM simulator (default) or Massive API (optional) +1. Copy `.env.example` to `.env` and add your API key: + ``` + OPENROUTER_API_KEY=your-key-here + ``` -## Quick Start +2. Start: + ```bash + # macOS/Linux + ./scripts/start_mac.sh -```bash -# Clone and configure -cp .env.example .env -# Add your OPENROUTER_API_KEY to .env - -# Run with Docker -docker build -t finally . -docker run -v finally-data:/app/db -p 8000:8000 --env-file .env finally + # Windows + .\scripts\start_windows.ps1 + ``` -# Open http://localhost:8000 -``` +3. Open `http://localhost:8000` ## Environment Variables | Variable | Required | Description | |---|---|---| -| `OPENROUTER_API_KEY` | Yes | OpenRouter API key for AI chat | -| `MASSIVE_API_KEY` | No | Massive (Polygon.io) key for real market data; omit to use simulator | -| `LLM_MOCK` | No | Set `true` for deterministic mock LLM responses (testing) | +| `OPENROUTER_API_KEY` | Yes | OpenRouter key for LLM chat | +| `MASSIVE_API_KEY` | No | Real market data (omit to use simulator) | +| `LLM_MOCK` | No | Set `true` for deterministic mock responses (testing) | -## Project Structure +## Development +```bash +cd backend +uv venv && uv pip install -e . +uv run uvicorn app.main:app --reload --port 8000 ``` -finally/ -├── frontend/ # Next.js static export -├── backend/ # FastAPI uv project -├── planning/ # Project documentation and agent contracts -├── test/ # Playwright E2E tests -├── db/ # SQLite volume mount (runtime) -└── scripts/ # Start/stop helpers + +```bash +cd frontend +npm install && npm run dev ``` -## License +## Testing + +```bash +# Backend unit tests +cd backend && uv run pytest tests/ -v -See [LICENSE](LICENSE). +# E2E tests (requires Docker) +cd test && docker compose -f docker-compose.test.yml up +``` diff --git a/independent-reviewer/.claude-plugin/plugin.json b/independent-reviewer/.claude-plugin/plugin.json new file mode 100644 index 00000000..7cbfe495 --- /dev/null +++ b/independent-reviewer/.claude-plugin/plugin.json @@ -0,0 +1,5 @@ +{ + "name": "independent-reviewer", + "version": "0.1.0", + "description": "carry out an independent review of all changes since last comit" +} \ No newline at end of file diff --git a/independent-reviewer/hooks/hooks.json b/independent-reviewer/hooks/hooks.json new file mode 100644 index 00000000..bf33dac9 --- /dev/null +++ b/independent-reviewer/hooks/hooks.json @@ -0,0 +1,14 @@ +{ + "hooks": { + "Stop": [ + { + "hooks": [ + { + "type": "command", + "command": "codex exec \"Please review all changes since the last commit and write feedback to planning/REVIEW-last-commit.md\"" + } + ] + } + ] + } +} \ No newline at end of file diff --git a/planning/PLAN.md b/planning/PLAN.md index bc1811b3..b2b220e5 100644 --- a/planning/PLAN.md +++ b/planning/PLAN.md @@ -101,7 +101,6 @@ finally/ ├── db/ # Volume mount target (SQLite file lives here at runtime) │ └── .gitkeep # Directory exists in repo; finally.db is gitignored ├── Dockerfile # Multi-stage build (Node → Python) -├── docker-compose.yml # Optional convenience wrapper ├── .env # Environment variables (gitignored, .env.example committed) └── .gitignore ``` @@ -171,11 +170,15 @@ Both the simulator and the Massive client implement the same abstract interface. - SSE streams read from this cache and push updates to connected clients - This architecture supports future multi-user scenarios without changes to the data layer +### Price History Cache + +The in-memory price cache maintains a rolling history of the last 200 prices per ticker (in addition to the current price). This powers the main chart area and can bootstrap sparklines on page load via `/api/prices/{ticker}/history`, eliminating the need for the frontend to accumulate chart data purely from the SSE stream. + ### SSE Streaming - Endpoint: `GET /api/stream/prices` - Long-lived SSE connection; client uses native `EventSource` API -- Server pushes price updates for all tickers known to the system at a regular cadence (~500ms) — in the single-user model this is equivalent to the user's watchlist +- Server pushes price updates for all tickers in the watchlist table at a regular cadence (~500ms) - Each SSE event contains ticker, price, previous price, timestamp, and change direction - Client handles reconnection automatically (EventSource has built-in retry) @@ -193,7 +196,7 @@ The backend checks for the SQLite database on startup (or first request). If the ### Schema -All tables include a `user_id` column defaulting to `"default"`. This is hardcoded for now (single-user) but enables future multi-user support without schema migration. +This is a single-user app. No `user_id` columns — they add complexity without value at this stage. **users_profile** — User state (cash balance) - `id` TEXT PRIMARY KEY (default: `"default"`) @@ -202,47 +205,36 @@ All tables include a `user_id` column defaulting to `"default"`. This is hardcod **watchlist** — Tickers the user is watching - `id` TEXT PRIMARY KEY (UUID) -- `user_id` TEXT (default: `"default"`) -- `ticker` TEXT +- `ticker` TEXT UNIQUE - `added_at` TEXT (ISO timestamp) -- UNIQUE constraint on `(user_id, ticker)` -**positions** — Current holdings (one row per ticker per user) +**positions** — Current holdings (one row per ticker) - `id` TEXT PRIMARY KEY (UUID) -- `user_id` TEXT (default: `"default"`) -- `ticker` TEXT +- `ticker` TEXT UNIQUE - `quantity` REAL (fractional shares supported) - `avg_cost` REAL - `updated_at` TEXT (ISO timestamp) -- UNIQUE constraint on `(user_id, ticker)` **trades** — Trade history (append-only log) - `id` TEXT PRIMARY KEY (UUID) -- `user_id` TEXT (default: `"default"`) - `ticker` TEXT - `side` TEXT (`"buy"` or `"sell"`) - `quantity` REAL (fractional shares supported) - `price` REAL - `executed_at` TEXT (ISO timestamp) -**portfolio_snapshots** — Portfolio value over time (for P&L chart). Recorded every 30 seconds by a background task, and immediately after each trade execution. +**portfolio_snapshots** — Portfolio value over time (for P&L chart). Recorded immediately after each trade execution and on backend startup. - `id` TEXT PRIMARY KEY (UUID) -- `user_id` TEXT (default: `"default"`) - `total_value` REAL - `recorded_at` TEXT (ISO timestamp) -**chat_messages** — Conversation history with LLM -- `id` TEXT PRIMARY KEY (UUID) -- `user_id` TEXT (default: `"default"`) -- `role` TEXT (`"user"` or `"assistant"`) -- `content` TEXT -- `actions` TEXT (JSON — trades executed, watchlist changes made; null for user messages) -- `created_at` TEXT (ISO timestamp) +Chat conversation history is held in memory (lost on restart). No DB table needed. ### Default Seed Data - One user profile: `id="default"`, `cash_balance=10000.0` - Ten watchlist entries: AAPL, GOOGL, MSFT, AMZN, TSLA, NVDA, META, JPM, V, NFLX +- One initial portfolio snapshot recording $10,000 at startup time --- @@ -252,6 +244,7 @@ All tables include a `user_id` column defaulting to `"default"`. This is hardcod | Method | Path | Description | |--------|------|-------------| | GET | `/api/stream/prices` | SSE stream of live price updates | +| GET | `/api/prices/{ticker}/history` | Recent price history for a ticker (from in-memory cache, last 200 points) | ### Portfolio | Method | Path | Description | @@ -264,7 +257,7 @@ All tables include a `user_id` column defaulting to `"default"`. This is hardcod | Method | Path | Description | |--------|------|-------------| | GET | `/api/watchlist` | Current watchlist tickers with latest prices | -| POST | `/api/watchlist` | Add a ticker: `{ticker}` | +| POST | `/api/watchlist` | Add a ticker: `{ticker}`. Returns 200 if already in watchlist (idempotent). | | DELETE | `/api/watchlist/{ticker}` | Remove a ticker | ### Chat @@ -283,21 +276,33 @@ All tables include a `user_id` column defaulting to `"default"`. This is hardcod When writing code to make calls to LLMs, use cerebras-inference skill to use LiteLLM via OpenRouter to the `openrouter/openai/gpt-oss-120b` model with Cerebras as the inference provider. Structured Outputs should be used to interpret the results. +Note: `openrouter/openai/gpt-oss-120b` is the LiteLLM model string (not a bare OpenRouter slug). Cerebras is selected via `extra_body={"provider": {"order": ["cerebras"]}}` as shown in the cerebras-inference skill. + There is an OPENROUTER_API_KEY in the .env file in the project root. ### How It Works +Conversation history is kept in an in-memory list on the backend (cleared on restart). No database table is needed. + When the user sends a chat message, the backend: 1. Loads the user's current portfolio context (cash, positions with P&L, watchlist with live prices, total portfolio value) -2. Loads recent conversation history from the `chat_messages` table +2. Takes the last 20 messages from the in-memory conversation history 3. Constructs a prompt with a system message, portfolio context, conversation history, and the user's new message 4. Calls the LLM via LiteLLM → OpenRouter, requesting structured output, using the cerebras-inference skill 5. Parses the complete structured JSON response 6. Auto-executes any trades or watchlist changes specified in the response -7. Stores the message and executed actions in `chat_messages` +7. Appends both the user message and assistant response to the in-memory conversation list 8. Returns the complete JSON response to the frontend (no token-by-token streaming — Cerebras inference is fast enough that a loading indicator is sufficient) +### Error Handling + +If the LLM call fails (network error, rate limit, malformed response), the backend returns HTTP 200 with: +```json +{"message": "Sorry, I'm having trouble connecting right now. Please try again in a moment.", "trades": [], "watchlist_changes": []} +``` +The frontend treats this identically to a normal response — no special error path needed. + ### Structured Output Schema The LLM is instructed to respond with JSON matching this schema: @@ -352,12 +357,12 @@ When `LLM_MOCK=true`, the backend returns deterministic mock responses instead o The frontend is a single-page application with a dense, terminal-inspired layout. The specific component architecture and layout system is up to the Frontend Engineer, but the UI should include these elements: -- **Watchlist panel** — grid/table of watched tickers with: ticker symbol, current price (flashing green/red on change), daily change %, and a sparkline mini-chart (accumulated from SSE since page load) -- **Main chart area** — larger chart for the currently selected ticker, with at minimum price over time. Clicking a ticker in the watchlist selects it here. +- **Watchlist panel** — grid/table of watched tickers with: ticker symbol, current price (flashing green/red on change), daily change %, and a sparkline mini-chart. Sparklines bootstrap from `/api/prices/{ticker}/history` on load and continue accumulating from the SSE stream. An empty/loading state for the first few seconds before the first SSE event is acceptable. +- **Main chart area** — larger chart for the currently selected ticker. Bootstraps from `/api/prices/{ticker}/history` and continues updating via SSE. Clicking a ticker in the watchlist selects it here. - **Portfolio heatmap** — treemap visualization where each rectangle is a position, sized by portfolio weight, colored by P&L (green = profit, red = loss) -- **P&L chart** — line chart showing total portfolio value over time, using data from `portfolio_snapshots` +- **P&L chart** — line chart showing total portfolio value over time, using data from `GET /api/portfolio/history` - **Positions table** — tabular view of all positions: ticker, quantity, avg cost, current price, unrealized P&L, % change -- **Trade bar** — simple input area: ticker field, quantity field, buy button, sell button. Market orders, instant fill. +- **Trade bar** — simple input area: ticker field, quantity field (fractional shares supported, e.g. 0.5), buy button, sell button. Market orders, instant fill. - **AI chat panel** — docked/collapsible sidebar. Message input, scrolling conversation history, loading indicator while waiting for LLM response. Trade executions and watchlist changes shown inline as confirmations. - **Header** — portfolio total value (updating live), connection status indicator, cash balance @@ -375,6 +380,8 @@ The frontend is a single-page application with a dense, terminal-inspired layout ### Multi-Stage Dockerfile +The Next.js frontend is built as a static export (`output: 'export'`). This is intentional and permanent — no SSR, no Node.js runtime, no Next.js API routes. All dynamic behavior goes through FastAPI endpoints. + ``` Stage 1: Node 20 slim - Copy frontend/ @@ -454,3 +461,4 @@ The container is designed to deploy to AWS App Runner, Render, or any container - Portfolio visualization: heatmap renders with correct colors, P&L chart has data points - AI chat (mocked): send a message, receive a response, trade execution appears inline - SSE resilience: disconnect and verify reconnection + diff --git a/planning/REVIEW-CODEX.md b/planning/REVIEW-CODEX.md new file mode 100644 index 00000000..96c9035c --- /dev/null +++ b/planning/REVIEW-CODEX.md @@ -0,0 +1,79 @@ +# Review of `planning/PLAN.md` + +## Findings + +### High: Portfolio snapshot policy cannot support the promised live P&L chart + +`PLAN.md` promises "a P&L chart tracking total portfolio value over time" in the product experience, and the frontend depends on `GET /api/portfolio/history` for that chart. But the database section says `portfolio_snapshots` are recorded only "immediately after each trade execution and on backend startup." That means the chart will not move with market prices unless the user trades, which materially breaks the trading-terminal feel and makes the chart misleading during normal hold periods. + +References: +- [PLAN.md:28](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L28) +- [PLAN.md:226](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L226) +- [PLAN.md:254](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L254) +- [PLAN.md:363](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L363) + +Recommendation: define a background snapshot cadence now, for example every N seconds while positions exist, plus on trade execution and startup. + +### High: Monetary values and share quantities are stored as SQLite `REAL` + +The schema stores `cash_balance`, `quantity`, `avg_cost`, `price`, and `total_value` as `REAL`. For a trading app, repeated buy/sell operations and portfolio recomputations will accumulate floating-point drift. That will leak into balances, P&L, and tests, especially once fractional shares are supported. + +References: +- [PLAN.md:203](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L203) +- [PLAN.md:214](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L214) +- [PLAN.md:215](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L215) +- [PLAN.md:222](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L222) +- [PLAN.md:223](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L223) +- [PLAN.md:228](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L228) + +Recommendation: specify fixed-precision handling now. Integer cents plus scaled share quantities, or Python `Decimal` with explicit rounding rules, would both be safer than `REAL`. + +### High: Trade execution is not specified as atomic + +A single trade updates cash, positions, trade history, and portfolio snapshots. The plan does not require these writes to happen inside one SQLite transaction. Without that requirement, any exception or concurrent request can leave the portfolio in a partially updated state. + +References: +- [PLAN.md:201](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L201) +- [PLAN.md:211](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L211) +- [PLAN.md:218](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L218) +- [PLAN.md:226](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L226) +- [PLAN.md:253](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L253) +- [PLAN.md:294](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L294) + +Recommendation: state explicitly that manual trades and chat-triggered trades execute inside a single DB transaction, and define the write order and rollback behavior. + +### Medium: SSE semantics conflict with the optional real-data polling model + +The plan says the server pushes price updates at about 500ms, but the Massive path may only refresh underlying data every 15 seconds on the free tier. As written, this implies either repeated duplicate "updates" every 500ms or two materially different runtime behaviors depending on provider. The frontend behavior around flashing prices and connection status will be inconsistent unless event semantics are defined more tightly. + +References: +- [PLAN.md:152](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L152) +- [PLAN.md:162](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L162) +- [PLAN.md:181](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L181) +- [PLAN.md:371](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L371) + +Recommendation: define whether SSE emits only on real price changes, emits periodic heartbeats separately, or emits snapshots on a fixed cadence with a field indicating whether data is fresh. + +### Medium: The plan omits executable ticker and pricing rules for trades + +The API and chat flows both allow trade execution, but the plan does not define whether a user can trade symbols that are not already in the watchlist, what happens when a symbol has no current price in cache yet, or how unsupported symbols are rejected. Those rules are needed before backend and frontend implementations can converge on validation and error handling. + +References: +- [PLAN.md:253](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L253) +- [PLAN.md:260](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L260) +- [PLAN.md:323](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L323) +- [PLAN.md:333](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L333) +- [PLAN.md:365](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L365) + +Recommendation: add explicit rules for symbol normalization, tradable universe, required price availability, and the exact API/chat response when a trade cannot be priced or validated. + +### Medium: The LLM integration section depends on a skill that is not part of this repo contract + +The plan tells implementers to "use cerebras-inference skill" and references it multiple times, but the repository contract in `planning/` should stand on its own. A plan that requires an external agent skill without documenting the actual request shape, client setup, or fallback behavior is brittle for future contributors and for any execution environment where that skill is unavailable. + +References: +- [PLAN.md:277](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L277) +- [PLAN.md:279](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L279) +- [PLAN.md:292](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/PLAN.md#L292) + +Recommendation: move the concrete LiteLLM/OpenRouter request contract into the plan itself, including model string, provider selection, structured-output schema handling, and what to do if that exact setup is unavailable. diff --git a/planning/REVIEW-last-commit.md b/planning/REVIEW-last-commit.md new file mode 100644 index 00000000..0370b4a5 --- /dev/null +++ b/planning/REVIEW-last-commit.md @@ -0,0 +1,18 @@ +# Review Since Last Commit + +Scope: reviewed the current working tree against `HEAD`. + +## Findings + +1. High: the new root README now contains concrete setup, development, and test commands that do not match the repository and will fail for anyone following them. [README.md:22](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/README.md#L22), [README.md:30](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/README.md#L30), [README.md:33](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/README.md#L33), [README.md:55](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/README.md#L55), and [README.md:66](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/README.md#L66) instruct users to use `.env.example`, `scripts/start_mac.sh`, `scripts/start_windows.ps1`, `frontend/`, and `test/docker-compose.test.yml`, but none of those paths exist in the current tree. This is worse than the previous high-level README because it turns aspirational architecture into broken operator guidance. Recommendation: either keep the README explicitly aspirational, or limit it to commands and paths that exist today. + +2. Medium: the review file was rewritten in a way that misstates what changed in the working tree, so it cannot be trusted as an audit record. [planning/REVIEW-last-commit.md:11](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/planning/REVIEW-last-commit.md#L11) previously asserted that only this review file was changed, but `git diff --name-only HEAD` also includes [README.md](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/README.md) and [.claude/settings.json](/home/lnayyar/projects/CourseAIVibeCodeUdmey/finally/.claude/settings.json). A review document that describes the wrong diff gives a false sign-off even if the individual observations are otherwise reasonable. + +## Notes + +- `git diff --name-only HEAD` contains `.claude/settings.json`, `README.md`, and `planning/REVIEW-last-commit.md`. +- I did not find an actionable product-code regression in `.claude/settings.json`; the meaningful issue in this diff is the README accuracy regression. + +## Residual Risk + +I did not run tests because the reviewed changes are documentation and local-tooling edits, not executable code changes. diff --git a/planning/REVIEW.md b/planning/REVIEW.md new file mode 100644 index 00000000..433bc347 --- /dev/null +++ b/planning/REVIEW.md @@ -0,0 +1,195 @@ +# FinAlly Project — Comprehensive Review + +_Reviewed: 2026-04-08_ + +--- + +## Executive Summary + +The project is at an early stage. The market data subsystem (one of roughly six major components) is complete and of high quality. Everything else — the FastAPI application entry point, database layer, portfolio/watchlist/chat API, LLM integration, frontend, Docker container, start/stop scripts, and E2E test infrastructure — does not exist yet. The repository has the skeleton of the right structure but is missing the body. + +The PLAN.md has been meaningfully updated since the last commit. The changes simplify the design (removing `user_id` columns, dropping the `chat_messages` table, replacing periodic DB snapshots with startup-only snapshots) and add important implementation details (price history cache, LLM error handling spec, SSE scoping clarification). The new plan is internally consistent and correct. Several of the issues identified in prior reviews have been formally resolved in the spec; however, a few pre-existing concerns in the implementation remain open. + +--- + +## 1. PLAN.md Changes — Assessment + +### What Changed + +| Area | Change | Assessment | +|---|---|---| +| Directory structure | Removed `docker-compose.yml` from the tree | Correct — the plan already describes standalone Docker; a compose file would contradict that | +| SSE scope | "all tickers known to the system" → "all tickers in the watchlist table" | Good clarification; removes ambiguity about which tickers are streamed | +| Schema: `user_id` columns | Removed from all five tables | Correct simplification for a single-user app; eliminates unused complexity | +| Schema: `chat_messages` table | Dropped entirely; conversation held in memory | Correct — in-memory chat history is simpler and sufficient; the stated trade-off (lost on restart) is intentional and documented | +| Schema: `portfolio_snapshots` | Changed from "every 30 seconds by background task + after each trade" to "after each trade + on backend startup" | See concern below — this is a regression for the P&L chart | +| Price history cache | New section added: rolling 200-price deque per ticker, `/api/prices/{ticker}/history` endpoint | Required addition; the previous plan left chart bootstrapping undefined | +| API endpoints | Added `GET /api/prices/{ticker}/history` | Consistent with the new history cache section | +| Watchlist POST | Added "Returns 200 if already in watchlist (idempotent)" | Good — removes a client-side error-handling branch | +| LLM section | Added note on LiteLLM model string format and Cerebras `extra_body` | Clarifies a subtle integration detail that would otherwise cause a runtime error | +| LLM error handling | New section: return HTTP 200 with graceful error message on failure | Good pattern; keeps the frontend error path simple | +| Frontend: watchlist/main chart | Updated to reference `/api/prices/{ticker}/history` for bootstrapping | Consistent with the new history endpoint | +| Frontend: trade bar | Added "fractional shares supported, e.g. 0.5" | Minor but useful for implementers | +| Frontend: P&L chart source | Changed from "portfolio_snapshots" to "`GET /api/portfolio/history`" | Correct — the frontend should reference the API, not the DB table | +| Dockerfile section | Added explicit note that Next.js is a static export | Prevents common mistakes during Docker build implementation | + +### Concern: P&L Chart Will Be Nearly Empty Without Periodic Snapshots + +The old plan had `portfolio_snapshots` recorded every 30 seconds by a background task. The new plan records only on backend startup and after each trade. If the user opens the app, prices move, but they make no trades, the P&L chart will have a single data point (the startup snapshot). The chart will be a flat horizontal line until the first trade. + +More significantly: after the first trade, the chart will show only the startup value and the post-trade value — no indication of how prices moved in between. For a "visually stunning trading workstation" this is a poor user experience. + +The simplification is understandable (removes a background task), but the consequences for the P&L chart's usefulness should be acknowledged. The recommended fix is a lightweight periodic snapshot every 60 seconds, only when the user holds any open positions. This was flagged in the prior review and remains a live concern. + +--- + +## 2. Implementation Status + +### Market Data Backend (`backend/app/market/`) — Complete + +This component is solid and well-engineered. All eight modules are present, lint-clean, and tested at 91% overall coverage (100% on the core modules that matter most). + +### Everything Else — Not Started + +The following are required by PLAN.md and are entirely absent: + +| Component | What's Needed | +|---|---| +| `backend/app/main.py` | FastAPI app entry point, lifespan handler for market data startup/shutdown, static file serving, router registration | +| `backend/app/db/` | Schema SQL, seed data, lazy initialization logic | +| `backend/app/routers/portfolio.py` | `GET /api/portfolio`, `POST /api/portfolio/trade`, `GET /api/portfolio/history` | +| `backend/app/routers/watchlist.py` | `GET /api/watchlist`, `POST /api/watchlist`, `DELETE /api/watchlist/{ticker}` | +| `backend/app/routers/chat.py` | `POST /api/chat` with LLM integration | +| `backend/app/routers/health.py` | `GET /api/health` | +| LLM integration | LiteLLM via OpenRouter/Cerebras with structured output schema | +| `frontend/` | Entire Next.js static export project | +| `Dockerfile` | Multi-stage Node → Python build | +| `scripts/` | `start_mac.sh`, `stop_mac.sh`, `start_windows.ps1`, `stop_windows.ps1` | +| `test/` | Playwright E2E tests, `docker-compose.test.yml` | +| `db/.gitkeep` | Runtime volume mount point | +| `.env.example` | Template for required environment variables | + +--- + +## 3. Open Bugs and Issues in Existing Code + +### Bug — Module-level Router in `stream.py` (Medium) + +`stream.py` creates `router = APIRouter(...)` at module scope, then `create_stream_router()` registers the `/prices` route on it via a closure: + +```python +router = APIRouter(prefix="/api/stream", tags=["streaming"]) + +def create_stream_router(price_cache: PriceCache) -> APIRouter: + @router.get("/prices") + async def stream_prices(request: Request) -> StreamingResponse: + ... + return router +``` + +Calling `create_stream_router()` twice registers the route twice on the same router object. In production this is called once, so there is no runtime failure today. But it will silently cause duplicate route registration if any test or tooling calls the factory more than once. Fix: move `router = APIRouter(...)` inside `create_stream_router()` so each call returns a fresh router. + +### Missing Feature — Price History Not in `PriceCache` (High) + +PLAN.md section 6 now explicitly specifies "a rolling history of the last 200 prices per ticker" and a `GET /api/prices/{ticker}/history` endpoint. The current `PriceCache` stores only the latest `PriceUpdate` per ticker — no history deque. The history endpoint cannot be built without extending `PriceCache`. This needs to be done before the backend API layer is started. + +Suggested addition to `PriceCache`: +- Add `self._history: dict[str, deque[PriceUpdate]] = {}` with `maxlen=200` +- Populate it in `update()` +- Add `get_history(ticker: str) -> list[PriceUpdate]` returning a snapshot copy + +### Design Gap — P&L Chart Static Between Trades (Medium) + +As noted in the PLAN.md review above, recording portfolio snapshots only on startup and after trades produces a near-empty P&L chart in all but the most active trading sessions. A background task recording a snapshot every 60 seconds when `positions` is non-empty is the minimum viable fix. + +### Design Gap — Floating-Point Money (Medium) + +The SQLite schema uses `REAL` for `cash_balance`, `quantity`, `avg_cost`, `price`, and `total_value`. After repeated buy/sell cycles, floating-point rounding errors will accumulate (e.g., `100 * 190.12345` will not equal what the user expects). Explicit `round(value, 2)` at every trade execution boundary is required. This is not in the spec and needs to be enforced in implementation. + +### Design Gap — Trade Atomicity Unspecified (Medium) + +A trade execution touches four tables: `users_profile` (cash deduction/credit), `positions` (upsert), `trades` (append), and `portfolio_snapshots` (append). These four writes must be wrapped in a single SQLite transaction. If any write fails mid-trade the database will be in an inconsistent state. The spec does not state this requirement explicitly; it should be enforced in the implementation. + +### Design Gap — Ticker Validation Rules Absent (Medium) + +The spec does not define what happens when a trade is requested for a ticker that has no current price in the cache (e.g., the ticker was never added to the watchlist, or the cache has not yet been seeded). Both the manual trade API and the LLM auto-execution path need explicit rules: either reject trades for tickers not in the watchlist, or allow them but require a valid cache entry. The behavior on "ticker not found in cache" must be specified before the trade API is built. + +### Code Quality — `PriceCache.version` Lock Omission (Low) + +The `version` property reads `self._version` without acquiring `self._lock`. On CPython this is safe due to the GIL (integer reads are atomic), but it is inconsistent with the rest of the class's locking discipline and would be unsafe on other Python implementations. It should use the lock for correctness. + +--- + +## 4. Missing Dependencies in `pyproject.toml` + +Currently declared: `fastapi`, `uvicorn[standard]`, `numpy`, `massive`, `rich`. + +Still needed for the remaining build: +- `litellm` — LLM calls via OpenRouter +- `python-dotenv` or `pydantic-settings` — reading `.env` file into the process +- `aiosqlite` — async SQLite access (if async DB writes are used; `sqlite3` stdlib suffices for sync) +- `httpx` — HTTP test client for ASGI (`pytest-asyncio` + `httpx` replaces requests for FastAPI tests) + +--- + +## 5. Architecture Observations + +### Watchlist-to-Market-Source Wiring Is Not Specified + +PLAN.md says the SSE endpoint streams prices "for all tickers in the watchlist table." When the user adds or removes a ticker via `POST /api/watchlist` or `DELETE /api/watchlist/{ticker}`, the code must also call `await source.add_ticker(ticker)` or `await source.remove_ticker(ticker)` on the active market data source. This wiring must happen in the watchlist router or a service layer — it cannot happen in the market data module itself since the market module has no knowledge of the database. The plan does not spell this out; it should be made explicit to avoid the backend being built with the watchlist DB and the price cache out of sync. + +### In-Memory Chat History and Restart Semantics + +Choosing in-memory conversation history is a clean simplification. The consequence (conversation lost on container restart) is now documented. One thing the plan does not address: if the user refreshes the browser, the frontend loses its local copy of the conversation but the backend still holds the history. The plan should clarify whether the `GET /api/chat` or `POST /api/chat` response should include or expose full conversation history, or whether the frontend is expected to maintain its own copy. + +### LLM Error Handling Returns HTTP 200 for All Errors + +The plan specifies returning HTTP 200 with a user-facing error message for any LLM failure (network error, rate limit, malformed response). This simplifies the frontend but has a consequence: if the LLM returns a malformed structured output that partially parses (e.g., a `trades` array with one valid and one invalid entry), the plan does not specify whether partial execution should occur or the entire response should be discarded. The implementation should treat any structurally invalid LLM response as a complete failure and not execute any partial trades from it. + +### Static Export Constraint Well-Documented + +The new Dockerfile section note — "This is intentional and permanent — no SSR, no Node.js runtime, no Next.js API routes" — is a useful guardrail. The `output: 'export'` constraint in `next.config.js` means features like `next/image` with remote URLs, `getServerSideProps`, and middleware are unavailable. The frontend implementer should be aware of this constraint early. + +--- + +## 6. Test Coverage Summary + +| Area | Coverage | +|---|---| +| `models.py` | 100% | +| `cache.py` | 100% | +| `interface.py` | 100% | +| `seed_prices.py` | 100% | +| `factory.py` | 100% | +| `simulator.py` | 98% | +| `massive_client.py` | 94% | +| `stream.py` | 33% — no ASGI-level test for the SSE endpoint | +| Database layer | 0% — not built | +| Portfolio API | 0% — not built | +| Watchlist API | 0% — not built | +| Chat/LLM API | 0% — not built | +| Frontend | 0% — not built | +| E2E | 0% — not built | + +No test exercises all 10 default tickers together (the full 10x10 Cholesky decomposition path is untested). + +--- + +## 7. Docker and Deployment Readiness + +Not ready. No `Dockerfile`, no start/stop scripts, no `frontend/` for the Node build stage, no `.env.example`, no `db/.gitkeep`. None of this is blocking for the current phase of development, but these items should not be deferred past the point where the backend and frontend are individually functional. + +--- + +## 8. Recommended Build Order + +1. **Extend `PriceCache`** with rolling 200-price history per ticker (deque) — required before the history endpoint can be built +2. **Build `backend/app/main.py`** with FastAPI lifespan: init cache, start market data source, seed watchlist tickers from DB (or defaults), register routers, serve static files +3. **Build database layer** (`backend/app/db/`) with lazy schema creation, seed data, and an explicit transaction wrapper for trade execution +4. **Build routers in order:** health → watchlist (with market source wiring) → portfolio → chat/LLM +5. **Add periodic portfolio snapshot** background task (every 60 seconds when positions exist) — needed for a useful P&L chart +6. **Fix `stream.py` router bug** — move `router = APIRouter(...)` inside `create_stream_router()` +7. **Add `litellm`, `python-dotenv`, `httpx`** to `pyproject.toml` dependencies before building the LLM and test layers +8. **Build frontend** after the backend is manually testable via `curl` or a REST client +9. **Write Dockerfile** once both frontend and backend are functional standalone +10. **Add SSE ASGI integration test** — the only material coverage gap in the completed market data code diff --git a/planning/REVIEW.mdold b/planning/REVIEW.mdold new file mode 100644 index 00000000..d6b63ff9 --- /dev/null +++ b/planning/REVIEW.mdold @@ -0,0 +1,33 @@ +# Review of `planning/PLAN.md` + +## Findings + +### High: Portfolio history design does not match the promised live P&L chart + +The plan says the UI will show "a P&L chart tracking total portfolio value over time", but `portfolio_snapshots` are only recorded "immediately after each trade execution and on backend startup." That means the chart will stay flat while market prices move unless the user trades again, which breaks the core trading-terminal experience and makes the chart materially misleading. + +Recommendation: define a background snapshot policy, for example recording total portfolio value every N seconds whenever there is at least one open position, plus on trade execution/startup. + +### High: Using SQLite `REAL` for cash, prices, and quantities will cause precision drift + +The schema stores `cash_balance`, `quantity`, `avg_cost`, `price`, and `total_value` as `REAL`. Repeated buys/sells and portfolio recomputations will accumulate floating-point error, and this will leak into the UI as incorrect cents, failed equality assertions in tests, and potentially negative dust balances after sells. + +Recommendation: specify a fixed-precision representation now. Common options are integer cents for money plus scaled integers for quantity, or explicit decimal handling with consistent rounding rules at every trade boundary. + +### High: Trade execution is underspecified for atomicity + +A trade touches multiple pieces of state at once: cash balance, positions, trades history, and portfolio snapshots. The plan does not require these writes to happen in a single database transaction. Without that, partial failures can leave the system inconsistent, especially once chat-triggered trades and background tasks are running concurrently. + +Recommendation: make trade execution explicitly transactional and define the operation order, including snapshot creation, inside one SQLite transaction. + +### Medium: SSE cadence conflicts with the optional market-data polling model + +The plan says `/api/stream/prices` pushes updates at about 500ms, but the Massive integration may only refresh data every 15 seconds on the free tier. If the server emits every 500ms regardless, most events will duplicate stale prices and create fake "liveness." If it emits only on real changes, the frontend behavior differs sharply between simulator and real-data modes. + +Recommendation: specify SSE semantics clearly: either emit only when underlying prices change, or emit heartbeat/status events separately from price-update events. + +### Medium: Ticker/trade validity rules are missing + +Manual and AI-driven trades depend on having a current executable price, but the plan does not define whether users can trade symbols outside the watchlist, what happens when a ticker has no price in cache yet, or how invalid/unsupported symbols are rejected. This gap will surface immediately in both the API and chat execution flow. + +Recommendation: add explicit rules for tradable tickers, price availability requirements, symbol normalization, and API error responses for unknown or not-yet-priced symbols.