FinAlly v1.0 — complete AI trading workstation (5 phases, all requirements met) - #2
Open
hendroriyadi wants to merge 114 commits into
Open
FinAlly v1.0 — complete AI trading workstation (5 phases, all requirements met)#2hendroriyadi wants to merge 114 commits into
hendroriyadi wants to merge 114 commits into
Conversation
Two independent roadmaps existed for milestone v1.0: this worktree's backend-first 6-phase plan (Persistence & Trade Engine -> ... -> Docker) and a vertical-slice 5-phase MVP roadmap already drafted in the main checkout (Live Market Terminal -> Manual Trading -> Portfolio Visualization -> AI Copilot -> One-Command Ship), each phase ending in a working browser-visible slice. User chose the vertical-slice roadmap. Adopts ROADMAP.md, REQUIREMENTS.md (traceability), and STATE.md from the main checkout as source of truth; removes the now-superseded Phase 1 (persistence-trade-engine) CONTEXT/RESEARCH/PLAN artifacts planned against the old phase numbering. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- git rm --cached db/finally.db and delete working-tree file + sidecars - add db/.gitkeep so the runtime volume-mount directory survives a fresh clone - add finally.db/-shm/-wal/-journal patterns to .gitignore - add httpx>=0.28.1 to backend dev deps (required for fastapi.testclient.TestClient)
…watchlist - backend/app/db/: WAL+busy_timeout connection factory (run_db seam), idempotent schema init + seed from SEED_PRICES, watchlist read access - backend/app/routes/watchlist.py: GET /api/watchlist with server-side ticker normalization helper for later write endpoints - backend/app/main.py: create_app() factory - lifespan lazily inits the db, starts the market data source from the persisted watchlist, mounts the frozen SSE stream router plus the watchlist router, dev CORS allowlist restricted to http://localhost:3000 - tests: temp_db/client fixtures running the real lifespan; seeded watchlist test; SSE mount test driven via raw ASGI scope/receive/send (httpx's ASGITransport buffers the whole app call and cannot signal a mid-stream disconnect, which would hang forever against stream.py's intentionally infinite generator)
…ed live - app/db/watchlist.py: add_watchlist_ticker/remove_watchlist_ticker/ count_watchlist, all through run_db with ? placeholders; duplicate detection via the (user_id, ticker) unique constraint's IntegrityError keeps it race-free - app/routes/watchlist.py: POST validates shape, checks MAX_WATCHLIST_SIZE, persists then calls market_source.add_ticker(); DELETE validates the path param, removes the row, then calls market_source.remove_ticker() - persist-then-track ordering so a DB failure never desyncs the stream - tests: WAL/busy_timeout pragma test, schema/seed/idempotency tests, and seven route tests (add/duplicate/malformed/at-cap/remove/unknown/ malformed-delete) using a spy market source to assert the call-through
… shell - create-next-app scaffold (TypeScript, App Router, ESLint) + lucide-react - Tailwind v4 CSS-first theme with locked FinAlly color tokens (canvas/panel/edge/accent/primary/submit/positive/destructive) - next.config.ts set to output: 'export' with images.unoptimized for static export - Root layout: Inter font, dark canvas shell, AppHeader mounted above page content - AppHeader: title + optional status-dot slot for Phase 3, no other content - Fixed .gitignore's overly broad .env* pattern to .env*.local + negation so .env.local.example can be committed while .env.local stays ignored - Pinned tailwindcss to ^4.3.3 (was bare ^4) to match the resolved lockfile version and satisfy the major-version verify gate
…te real - lib/types.ts: WatchlistItem, PriceUpdate, PriceMap, ConnectionStatus mirroring the backend JSON contracts - lib/api.ts: API_BASE, ApiError, fetchWatchlist/addWatchlistTicker/removeWatchlistTicker typed fetch wrappers - components/WatchlistRow.tsx: presentational row (ticker/price/chg%/sparkline slot), em-dash placeholder until live data exists - components/WatchlistPanel.tsx: fetch-on-mount grid owning loading (skeleton), error, empty, populated, and bounded-overflow-scroll states, copy verbatim from the UI-SPEC - app/page.tsx: renders WatchlistPanel as the page's only content region - fix(.gitignore): root Python-template `lib/` pattern was unanchored and silently ignored frontend/lib/, which would have permanently blocked api.ts and types.ts from ever being tracked; added a scoped `!/frontend/lib/` negation rather than anchoring the original pattern, so unrelated nested lib/ directories (e.g. tooling build output) stay ignored
SUMMARY.md for 01-02, plus STATE.md/ROADMAP.md progress updates (2/4 plans complete in Phase 1). Task code was already committed (99ac631, 6bea9c8) by an executor session that hit two consecutive transient API connection drops right at the finish line; this commit completes the bookkeeping the crashed session left undone, after independently re-verifying Task 3's automated acceptance criteria (tsc, build, eslint, copy-string and export checks) pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… tells the truth - lib/useSseStream.ts: single EventSource lifecycle, session baselines, capped per-ticker history (MAX_SPARKLINE_POINTS=60), error handler only sets status (never rebuilds the connection, avoiding a reconnect storm) - components/PriceStreamProvider.tsx: React context sharing one stream between the header and the watchlist grid - components/ConnectionStatusDot.tsx: fixed 8px three-state indicator - app/layout.tsx wraps AppHeader + children in PriceStreamProvider; AppHeader.tsx becomes a client component rendering the dot from context Deviation: accumulator state is published via a shallow-copied useState snapshot after each frame rather than read directly from ref.current during render — this repo's Next.js 16 / eslint-config-next ships the react-hooks/refs lint rule, which forbids reading ref.current in the render path. Refs still own the mutation/accumulation; state exists purely to be a render-safe read.
…essive sparklines - components/Sparkline.tsx: hand-written inline SVG polyline; flat-baseline placeholder under two points, zero-range guard for a perfectly flat series - components/WatchlistRow.tsx: flash state on price change (500ms fade, restarts on rapid ticks rather than stacking timers), CHG% coloured by the sign of the session-baseline change percent - components/WatchlistPanel.tsx: sources price/history/baselines from usePriceStreamContext(), computes CHG% as (price - sessionBaseline) / sessionBaseline * 100 per this plan's operative definition; watchlist REST fetch is unchanged and never re-triggered by stream events
…ong-text states - AddTickerForm.tsx: uppercase/cap-10 controlled input, submit disabled while empty or in-flight, spinner during submit, inline error naming the submitted ticker on 400/409/422, input value preserved on failure - WatchlistPanel.tsx: renders AddTickerForm in the panel header (outside the scrolling row area), addItem() appends the new row so it renders immediately via WatchlistRow's existing em-dash/flat-sparkline placeholder path
- RemoveTickerButton.tsx: per-row remove control (Trash2/Loader2 icon, 44px tablet hit target via inverted min-w/min-h breakpoint), no confirmation dialog. Row removed only after DELETE succeeds; a failed delete leaves the row present with a briefly-shown inline error naming the ticker (auto-clears after ~4s). - WatchlistRow.tsx: fixed-width fifth cell for the remove control so other columns never reflow. - WatchlistPanel.tsx: wires RemoveTickerButton per row; removeItem() filters the ticker out of state on success. Emptying the watchlist falls through to the existing items.length===0 branch with no new empty-state code, per the plan's explicit instruction. Completes WATCH-03 in the browser (backend contract shipped in Plan 01). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…s done SUMMARY.md for 01-04, plus STATE.md/ROADMAP.md progress updates. Task 2's code was already committed by an executor session that stalled during its own verification step; this commit completes the bookkeeping after independently re-verifying all of Task 2's acceptance criteria pass (tsc, build, eslint, copy/wiring greps, backend test suite still 86/86 green). Phase 1 (Live Market Terminal) now has all 4 plans complete across all 11 requirements. Next: code review, UI review, and phase-level verification. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Core security asks hold: parameterized SQL throughout, exact-origin CORS with no credentials, no dangerouslySetInnerHTML/eval/innerHTML anywhere ticker strings flow. Findings are logic/race bugs: watchlist size-cap TOCTOU, no compensation when market-source call fails after a DB mutation, overly-broad IntegrityError catch, a seed race in init_db() that contradicts its own idempotency doc comment, an SSE connection-status state machine that can't distinguish "still retrying" from "permanently closed", and unhandled non-ApiError promise rejections in the add/remove ticker forms. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- WR-01: watchlist size cap now enforced inside the same atomic INSERT
as the row insert (INSERT ... SELECT ... WHERE COUNT(*) < max_size),
closing the check-then-act race between concurrent POSTs. Raises
WatchlistCapReachedError on a blocked insert.
- WR-02: add_ticker/remove_ticker now compensate (delete/re-add the
watchlist row) if the market_source call fails after the DB mutation
already committed, so DB and live-stream state can't diverge. Both
return 502 on that failure path.
- WR-03: add_watchlist_ticker now distinguishes the expected
(user_id, ticker) UNIQUE violation from any other IntegrityError via
a message substring match, logging unexpected integrity failures at
error level instead of silently reporting them as "duplicate ticker."
- WR-04: init_db()'s seed step uses INSERT OR IGNORE as a single atomic
statement instead of a separate SELECT COUNT(*) + INSERT, so two
concurrent init_db() calls can no longer both observe "unseeded" and
race to an unhandled IntegrityError.
- IN-02: TICKER_PATTERN now requires a leading alphanumeric character,
rejecting bare-punctuation shapes like "-" or "--".
- IN-03: DELETE /api/watchlist/{ticker} path parameter now declares the
same min_length=1/max_length=10 bound as the POST body field.
8 new/updated tests (concurrent-cap, compensation, seed-race,
IntegrityError-logging coverage). Backend suite: 94/94 passing
(was 86), ruff clean.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…d history - app/routes/chat.py: SYSTEM_PROMPT (persona, constant) + build_chat_messages() (pure) render a volatile context message from get_portfolio_state() + value_portfolio() (the same pair the portfolio route and snapshot writer use) and list_watchlist() priced via price_cache.get_price(). A missing price renders an explicit "unavailable" marker, never zero. - POST handler now loads portfolio/watchlist fresh on every request (no caching on app.state) and passes the already-read history plus the new user message through build_chat_messages() instead of the prior minimal inline list. - tests/routes/test_chat.py: pure-function coverage for build_chat_messages plus a recorder-based freshness proof showing a trade and a watchlist addition made between two turns are visible in the second turn's context.
apply_watchlist_add/apply_watchlist_remove now own the only copy of the insert-then-start-feed (and delete-then-stop-feed) sequence, including the compensating rollback that keeps the stored list and the live stream from diverging when the market-source call fails. They raise purpose-named errors (DuplicateTickerError, TickerNotOnWatchlistError, MarketSourceSyncError) rather than HTTPException, so the chat executor in the next task never has to unwrap a web-framework type to build a transcript message. The two HTTP handlers are now thin exception-to-status translators with every status code and detail string preserved character for character -- proven by the existing 11 route tests passing with no test function edited (only the import block changed). 7 new helper-level tests cover both compensation branches by asserting on the stored list after an injected failure, not just on the exception type.
_execute_watchlist_action mirrors _execute_trade_action's shape and calls
the shared apply_watchlist_add/apply_watchlist_remove helpers, so an
AI-initiated change runs the identical persist-then-track-then-compensate
sequence the form runs -- including the market-source sync that makes an
added ticker actually stream a price. Every failure (duplicate, cap,
not-found, sync, shape-invalid, unanticipated) returns HTTP 200 with a
per-action error carrying the exact sentence the manual UI shows.
Both loops now feed one ordered action list, so a reply doing both a trade
and a watchlist change reports both, and a watchlist failure never blocks
a trade beside it.
9 new tests. The load-bearing one asserts a newly added ticker acquires a
live price, not just a row -- verified non-vacuous by a mutation
spot-check: stubbing out the market_source.add_ticker call makes it fail
with exactly the symptom 04-RESEARCH.md's Pitfall 2 describes.
Deviation: the plan's acceptance criterion "grep for app.db.watchlist
imports in chat.py returns 0" was already unsatisfiable before this task
-- Plan 04-02 legitimately imports list_watchlist there for CHAT-02
context grounding (a read, not a mutation). Satisfied the criterion's
stated intent instead ("the chat route reaches the watchlist only through
the shared helpers"): WatchlistCapReachedError now comes from the route
module alongside the other three helper errors, chat.py imports no
watchlist mutation function, and the surviving list_watchlist import is
documented inline as read-only.
…ions ChatPanel is a 384px right-hand dock at xl collapsing to a 56px rail, and stacking full-width below the dashboard on narrower viewports -- an ordinary flex sibling of the page content, so collapsing lets the dashboard reclaim the width with no layout change. app/page.tsx is untouched. Transcript covers all five states (history error, skeleton, empty, populated, thinking) with UI-SPEC copy verbatim. The user's own message renders immediately and survives a failed send; no optimistic assistant content is ever shown -- the thinking row is a status indicator, not a bubble. Conversation survives a reload via GET /api/chat/history on mount. A reply containing a succeeded action calls the shared portfolio context's refresh(), so the header, positions table, and heatmap move from the same source rather than a second fetch path. ChatActionCard renders one card per executed action -- the entire transparency mitigation for this project's deliberate no-confirmation- dialog design. Failed actions render the backend's own sentence verbatim rather than a frontend restatement free to drift. A successful sell is styled exactly like a successful buy; profitability has its own surfaces. All state is component-local: no new context, no localStorage. Collapse hides rather than unmounts, so an unsent draft and the scroll position survive it.
CR-01 (critical, real bug): ChatPanel branched on historyError first and unconditionally, and the flag was never cleared -- so one failed mount fetch permanently masked the transcript, and a user's later successful sends updated state behind a banner they could never get past. Messages now win whenever any exist; the error only renders when there is genuinely nothing else to show, and a successful send also clears the stale flag. WR-01: journal_mode=WAL takes an exclusive lock on a not-yet-WAL file and can return SQLITE_BUSY *without* invoking the busy handler, so ordering the pragma after busy_timeout is necessary but not sufficient. Set the timeout first AND retry the switch. Measured with 8 threads racing connect() on a fresh file, 40 trials: original 3 failures, reorder-only 2, reorder+retry 0. The previously-flaky test_concurrent_init_db_calls_do_not_raise now passes 25/25. WR-02: 30s timeout on the LiteLLM call -- a hung upstream previously stranded the user on "FinAlly is thinking..." forever and held a worker thread. WR-03: reset the textarea height after send, so an expanded empty box doesn't remain. WR-04: a 4xx now says the message was rejected rather than telling the user to check a connection that is fine. IN-01: length bounds on Trade.ticker/WatchlistChange.ticker, matching the HTTP-facing model. IN-02: type the extracted helpers' market_source as MarketDataSource. IN-03: ChatActionResult's optional fields arrive as explicit null, not as absent keys (verified against both endpoints) -- typed `| null` and switched the card's guards to `!= null`, which closes a latent crash path where a null would have reached .toFixed().
All 8 code review findings fixed. Verification: human_needed, 0 code-level gaps, 9/9 requirements (CHAT-01..07, UI-04, TEST-02), 209/209 backend tests. Four items need a human, one of which is the only external dependency never exercised here: a live chat round trip with a real OPENROUTER_API_KEY (the key resolves empty in this sandbox, so LLM_MOCK=true carries the entire automated suite).
Four plans covering DEPLOY-01/02/03, TEST-03, TEST-04 across three waves.
Wave 1 (parallel, zero file overlap):
05-01 Docker tracer — multi-stage image, directory-gated StaticFiles mount
added after the API routers, plus the two-lifecycle persistence proof
05-03 Vitest + RTL, price flash / watchlist CRUD / positions math, and the
CR-01 chat regression with a mutation spot-check
Wave 2:
05-02 .env.example and four idempotent start/stop scripts, proven by
running each twice against real containers
Wave 3:
05-04 Playwright E2E over the shipped image under LLM_MOCK=true
Unlike Phases 1-4, this phase's central claims are shell-assertable, so they
are proven here rather than deferred: one-port serving is curled against a
running container, persistence is an actual stop/remove/restart cycle on the
same volume, and script idempotence is measured by repetition. The only
manual item is the Windows .ps1 pair (no Windows runner available).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Multi-stage Dockerfile: node:24-slim builds the static export, then Astral's uv python3.13-trixie-slim image installs the backend via the documented two-sync split and copies the export to /app/static. backend/app/main.py mounts StaticFiles(html=True) at / as the LAST statement in create_app(), guarded on the directory existing. Both halves matter: Starlette matches in declaration order so a mount at / registered any earlier would shadow every /api/* route, and StaticFiles' check_dir default raises at construction time so the guard is what keeps a bare `uv run uvicorn` dev session (no built frontend) from failing at import. Proven on a container that actually ran, not asserted: /api/health returns JSON, / returns the terminal's HTML, /api/watchlist returns seeded tickers, an unknown path 404s, and the process runs as appuser. The image carries no .env and no developer database; /app/db is created and chowned before dropping privileges so a fresh named volume is writable for the SQLite file and its WAL sidecars. Two version traps encoded as gates: the uv image is -trixie (NOT the stale -bookworm tag family), and FastAPI stays at 0.128.7 — its native app.frontend() needs >=0.138.0, and this app's single-page export doesn't need the SPA fallback that feature exists for. 7 new unit tests prove the mount can't shadow /api/*, including a structural assertion that the mount is last in the route table so a future appended route fails here rather than 404ing mysteriously in production.
test/verify-persistence.sh runs two real container lifecycles against one
named volume: writes cash, a position, a trade, a watchlist entry, and a
chat turn through the real HTTP API; REMOVES container one (not just stops
it -- a stopped container still owns its writable layer, so restarting it
would prove nothing); starts a differently-named container on the same
volume; and compares four separately-reported dimensions.
Verified non-vacuous by mutation spot-check, per the discipline used in
Phases 2/3/4. Pointing FINALLY_DB_PATH outside the mount makes all four
dimensions fail with the exact diagnosis they exist to give:
FAIL cash before=8589.91 after=10000.00
FAIL positions before=[AAPL 3, MSFT 2] after=[]
FAIL watchlist before=[...PYPL...] after=[no PYPL]
FAIL chat before=2 after=0
Restoring the path makes it pass again. The mutation was never committed.
The script never reads the SQLite file and never inspects a docker run
command string -- a check that asserted a -v flag is present would pass
against an image that persists nothing, which is the whole failure mode.
It uses its own finally-verify-data volume (a comment-stripped grep
asserts finally-data never appears) so it can never delete real data, and
a trap removes both containers on every exit path.
start_mac.sh: resolves the repo root from its own path, reports "Docker not installed" and "daemon not running" differently (different fixes), creates .env from .env.example when absent (--env-file fails hard otherwise, and .env is gitignored so no fresh clone has one), builds only when the image is missing or --build is passed, and force-removes any same-named container before running so a previously crashed one can't turn the next run into a name conflict. stop_mac.sh exits 0 whether or not anything was running and contains no volume-removal verb at all (enforced by a comment-stripped negative grep) -- a stop that silently deleted a portfolio would be the worst bug this phase could ship. Both use ANCHORED name filters (name=^finally$), so finally-verify-1 from the persistence script can never be mistaken for the app. Verified: start twice -> exactly one container answering /api/health with JSON and / with HTML; stop twice -> zero containers, volume intact. The fresh-clone path was exercised by moving the real .env aside; it was restored byte-identically, confirmed by sha256 comparison.
Direct translations of the shell pair -- same variables, control flow, messages, and anchored name filters -- so the two can be diffed side by side. No Windows runner exists in this environment, so these are verified structurally here (mirror-parity greps) and by a human at the phase gate. Three PowerShell specifics that each produce a script that looks correct and misbehaves: the param block is the first executable statement (only comments may precede it); $ErrorActionPreference="Stop" does NOT make a failing native command throw, so every docker call whose failure matters checks $LASTEXITCODE (5 checks); and the repo root comes from $PSScriptRoot rather than the caller's cwd. README: added a Quick Start naming all four scripts, and replaced the Status section, which still claimed only the market data backend was built -- badly stale as of Phase 4.
The project's first frontend test framework. All nine dev deps re-verified against the live registry before install (version resolves, repository is the official project repo, no install-time script) and pinned exactly; npm audit shows only the pre-existing next/postcss/sharp advisories. 7 tests on WatchlistRow's flash, using fake timers rather than real waits: no flash on first price, uptick green, downtick red, fade completes at 500ms, a repeated price is not a tick, undefined price renders the placeholder -- and the one behavior a naive implementation gets wrong, that a second tick RESTARTS the fade instead of the first tick's stale timer clearing it. Verified non-vacuous by mutation: removing the timer-clearing logic fails exactly that test and only that test. Two setup findings the plan did not anticipate: - tsconfig's `exclude` (added so `next build` skips test files) is ALSO honoured by vite-tsconfig-paths, so it refused to map @/ inside the very files needing it. Added an explicit resolve.alias; both concerns hold. - RTL only auto-registers cleanup when Vitest globals are enabled, and this config runs without them. Without an explicit afterEach(cleanup) the DOM accumulated and queries failed with "found multiple elements" for reasons unrelated to the component.
27 frontend tests across 4 files. TEST-03's four named areas are covered: price flash (prior commit), watchlist CRUD, portfolio display calculations, and chat message rendering. The centrepiece is the ChatPanel CR-01 regression. Phase 4's review found that bug as CRITICAL and noted the component had zero coverage: a single failed history load permanently masked the transcript, so later successful sends updated state behind a banner the user could never get past. The test fails the history fetch, sends a message, and asserts the REPLY IS VISIBLE -- a test that only checked the error appears would have passed against the bug. Mutation-checked, and the first attempt was instructive: reverting only the render-branch priority did NOT fail the test, because the fix has two independent mechanisms (branch priority AND clearing the flag on a successful send). Removing both -- i.e. reconstructing the original Phase 4 bug -- fails exactly that one test. Worth knowing the fix is defense-in-depth rather than assuming one mechanism carries it. PositionsTable tests assert the table follows the LIVE stream price rather than the server's snapshot P&L, which is the specific staleness bug that derivation exists to prevent. vi.mock with importOriginal throughout, never vi.spyOn: ES module namespace objects are sealed, and spreading the original keeps the real ApiError available for the components' instanceof branches.
THE IMPORTANT PART: the E2E suite caught a real production bug on its first successful run. `.dockerignore` excluded `.env*` only at the repo root, so `frontend/.env.local` stayed in the build context -- and Next.js reads it at BUILD time, baking NEXT_PUBLIC_API_URL=http://localhost:8000 into the static bundle. The shipped image therefore served a frontend that asked the BROWSER's localhost for its API: works by accident on the developer's machine, ERR_CONNECTION_REFUSED anywhere else. Confirmed by grepping the built image (http://localhost:8000 present in a JS chunk), fixed with **/.env patterns, and confirmed gone after rebuild. Every check before this missed it. curl against the container passed because curl doesn't run the bundle; the persistence script passed for the same reason; unit tests never build an image. Only a real browser against the real image could see it -- which is the entire argument for TEST-04. Also found: the compose service could not be named `app`. Chromium's HSTS preload list contains the whole `.app` gTLD, so http://app:8000 is force-upgraded to HTTPS and every navigation dies with ERR_SSL_PROTOCOL_ERROR. Renamed to finally-app, documented inline. Rig: compose builds the REAL production image (context .., root Dockerfile), gates Playwright on its healthcheck, passes LLM_MOCK=true and never OPENROUTER_API_KEY, publishes no host port, and mounts no volume so spec 01 gets a freshly seeded database. Specs are numerically prefixed because Playwright collects alphabetically and 01-fresh-start asserts the untouched $10,000 balance every later spec spends. Chat specs use the mock's exact trigger phrases. Status: 3 of 10 specs green. The rig itself is proven end to end; the remaining failures are locator refinements, tracked honestly in 05-VERIFICATION.md rather than claimed as done.
Phase 5 set out to be the first phase that PROVES its claims rather than deferring them, and it delivered: DEPLOY-01, DEPLOY-02 and DEPLOY-03 were all verified against real containers, two of them additionally mutation-checked. TEST-03 shipped 27 component tests. TEST-04 is PARTIAL and recorded as such (rig proven end to end, 3/10 specs green) rather than rounded up -- the remaining failures are locator refinements plus one genuine test-design issue in the SSE spec. Three real bugs were found and fixed during the phase, the most important by the E2E suite itself: NEXT_PUBLIC_API_URL baked into the shipped bundle, which every curl-based check had missed. Also untracks test/artifacts (Playwright output that was committed by an earlier session and is now gitignored).
Two bookkeeping gaps caught by /gsd-resume-work's incomplete-work scan: - 05-03-PLAN.md had no SUMMARY. The work itself shipped correctly in two commits (framework + 27 tests, all passing), but the summary was never written -- so the plan read as incomplete to any resume. Backfilled from the actual commits, including the three deviations and the mutation-check lesson (the first mutation attempt passed because the CR-01 fix has two independent mechanisms; only reconstructing the original bug failed it). - STATE.md frontmatter said total_phases: 4 with completed_phases: 5, a stale value from an earlier scripted edit that matched the wrong field.
TEST-04 now passes end to end (9 specs, ~46s). Getting there required
fixing one real product bug and four test-harness mistakes of my own.
PRODUCT BUG (found by the suite, now fixed): an AI-initiated watchlist
REMOVE updated the database but not the grid -- the API reported the
ticker gone while the row lingered until a reload, directly violating
ROADMAP Phase 4 criterion 4 ("updates the watchlist grid"). Proven with a
probe: API_HAS_SHOP_AFTER_REMOVE=false, GRID_HAS_SHOP_AFTER_REMOVE=1.
WatchlistPanel owns local state with no provider, and ChatPanel lives in a
different subtree, so ChatPanel now dispatches a window event that the
panel listens for. A context for one signal would be more machinery than
the problem needs.
HARNESS FIXES, each diagnosed by probe rather than guessed:
- getByRole(name:) is a SUBSTRING match by default, so "Positions" also
matched the heatmap's "No open positions" heading and resolved two
sections. Added exact:true.
- Plain .click() never satisfies Playwright's "stable" check on a page
re-rendering every 500ms from the price stream: measured 12s timeout vs
29ms for dispatchEvent. Added clickLive(), which asserts enabled first
since dispatchEvent bypasses the disabled check.
- dispatchEvent does not perform an element's DEFAULT ACTION, so it cannot
submit a type="submit" button. Chat sends now use Enter, the real gesture.
- sendChat counted assistant labels before the mount history fetch settled,
racing its own baseline. Now waits for the skeleton to clear.
- Watchlist row accessible names are "AAPL190.13+0.02%" with no separator,
so a \b word boundary could never match.
SSE spec rewritten: route.abort() only intercepts NEW requests and
setOffline does not tear down an established EventSource (verified: still
"Connected" after 10s offline). It now breaks the stream before the page
opens it, then releases -- exercising the same native-retry recovery with
no reload.
One known flake remains, documented rather than hidden: the first chat send
against a fresh container occasionally renders no reply while later sends
take ~1s. Ruled out slowness (90s ceiling still failed), hydration, and the
submit-vs-dispatch issue. The configured retry covers it; root cause open.
Updates 05-04-SUMMARY, 05-VERIFICATION, STATE and ROADMAP now that the E2E suite passes end to end. Phase 5 closes 5/5 success criteria proven, with the Windows .ps1 pair the single remaining human check. Records the third real bug the suite caught (AI watchlist removal not re-syncing the grid), the five harness mistakes and their diagnoses, and the one known flake with the three hypotheses that were ruled out by experiment rather than assumed.
hendroriyadi
changed the title
hendroriyadi
marked this pull request as ready for review
August 5, 2026 00:01
…s off
It was never flaky. The first chat test failed on attempt 1 in every single
full-suite run (four for four) and passed on retry. The configured retry
turned a 100%-reproducible assertion bug into something that looked
probabilistic, and I accepted "passes on retry" instead of investigating.
Root cause found by instrumenting the request rather than theorising: the
server was never involved. POST /api/chat returned 200 OK with a successful
trade every time. The bug was in the test helper --
chatPanel(page).getByText("FINALLY") // case-insensitive SUBSTRING
getByText(string) matches case-insensitively on substrings, so "FINALLY"
also matched the empty-state copy "Start chatting with FinAlly". On an empty
conversation the baseline count was 1, not 0. Sending then REPLACED the
empty state with one assistant label, so the count stayed at 1 while the
assertion waited for 2. On retry the conversation was no longer empty, the
empty state was gone, and the arithmetic happened to work.
Fixed with { exact: true }. Suite now passes 10/10 with retries DISABLED,
twice, in 14-22s -- faster because nothing burns 30s against a timeout.
Second time in this plan that Playwright's default substring matching caused
a failure that looked like something else (the first: getByRole(name:)
matching two panels). Both are now exact.
playwright.config.ts keeps one CI retry for genuine infrastructure noise but
now says a retry-only pass is a bug report, not a pass -- the check that
would have caught this immediately.
Earlier hypotheses, all ruled out by experiment and all wrong: slowness (a
90s ceiling still failed), hydration, submit-vs-dispatch. No app code
changed by this fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Builds the FinAlly AI trading workstation end to end, per
planning/PLAN.md. All five roadmap phases are implemented, reviewed, and verified, and all 37 v1 requirements are met.What's here
./scripts/start_mac.sh→ http://localhost:8000Verification
253 automated tests — 216 backend (pytest), 27 frontend (Vitest/RTL), 10 E2E (Playwright, against the real production image). The E2E suite passes with retries disabled. Lint clean, static export builds.
Phase 5's deployment claims were proven against real containers, not deferred:
curl /api/health→ JSON andcurl /→ the terminal's HTML on one port, as a non-root user.envpath exercised with the real file restored byte-identicallyFive mutation spot-checks confirm the important tests aren't vacuous — each deliberately breaks the behaviour and confirms the specific test fails:
Bugs found and fixed along the way
Two were caught by the E2E suite and were invisible to every other check:
NEXT_PUBLIC_API_URLbaked into the shipped bundle —.dockerignoreexcluded.env*only at the repo root, sofrontend/.env.localreached the build context and Next.js read it at build time. The image served a frontend asking the browser's localhost for its API: fine on a developer machine, broken everywhere else. curl-based checks can't see this, because curl never executes the bundle.Others, from code review and verification:
ChatPaneldead end (critical) — a never-cleared error flag permanently masked the transcript after one failed history load, even as later sends succeededjournal_mode=WALcan returnSQLITE_BUSYwithout invoking the busy handler, so reordering the pragmas wasn't enough. Fix chosen by measurement across 320 racing connections: 3 → 2 → 0 failures.toFixed()crash on a null, and a silent watchlist reset on restartOne correction worth flagging
An earlier revision of this PR listed a "known E2E flake" as an open gap. That was a misdiagnosis, now fixed. It was never flaky: the first chat test failed on attempt 1 in every run and passed on retry, and the retry made a 100%-reproducible bug look probabilistic.
The server was never involved —
POST /api/chatreturned200 OKthroughout. The bug was in the test helper: Playwright'sgetByTextmatches case-insensitive substrings, so"FINALLY"also matched the empty-state copy "Start chatting with FinAlly", making the baseline count 1 instead of 0. Fixed with{ exact: true }; the suite now passes with retries disabled.playwright.config.tskeeps one CI retry for real infrastructure noise, but now carries a note that a retry-only pass is a bug report rather than a pass.Known gaps — deliberately visible, not rounded up
OPENROUTER_API_KEYresolves empty in this sandbox, soLLM_MOCK=truecarried the whole automated suite. Request mechanics are validated; real model output needs a key..ps1scripts need one human run (no Windows runner here). They're verified structurally — mirror-parity greps, param-block-first,$LASTEXITCODEchecks, no volume-deletion verb.All tracked in
.planning/STATE.md.🤖 Generated with Claude Code