Repository navigation
MVP frontend wiring, Azure deployment, and CI/CD pipeline - #7
Merged
Merged
Conversation
Design for closing the six mocked frontend domains, the missing S8 admin/lots/ export routes, the unbuilt ADR 20 real-time push fan-out, and real Stripe Elements billing capture — plus exhaustive Vitest/RTL coverage per frontend/CLAUDE.md's one-test-per-component rule.
…backend - portfoliosApi.js now calls GET/POST /portfolios/models and /assignment instead of mockClient; drops the fabricated securities/price table. - useSecurities derives the orderable universe from the real models response (deduped target_weights) instead of a mock quote endpoint. - OrderForm collects the reference price from the customer directly, since no quote endpoint exists. - ModelCard/PortfolioPage updated to the real field names (weight_pct, model_portfolio_id) and a static per-model description (no description field in the real response). - chatApi/useChatStream replaced the fake word-splitting reveal with a real fetch + ReadableStream consumer of the existing chat SSE endpoint.
- feesApi.js now calls GET /fees and POST /payment-methods instead of mockClient; drops the invented month_to_date_gain and payment_method fields (neither exists on the real response). - PaymentMethodForm sends one of Stripe's fixed test-mode payment method tokens (pm_card_visa, etc.) to the real endpoint rather than a brand/last-4 string pair — a genuinely live sandbox call once STRIPE_SECRET_KEY (test mode) is configured, no Elements UI needed. - useCustomerFees/CustomerFeesPanel (admin) now call feesApi.getForCustomer instead of the adminCustomersApi mock, which drops getFees entirely. - Fixes two regressions this surfaced: HoldingsTable and DashboardPage still read the old target_weight/model_id field names the mock used; LotTable depended on useSecurities' now-removed reference_price, so lotsApi's own mock seed now carries its own current_price per lot.
New SimulatedBadge (design-system.md §8.3's neutral provenance-tag pattern, generalized past its one named case) placed on the customer directory, customer detail's profile/KYC-override sections, tax lots, and the statement export button — the three domains this pass didn't wire to a backend (no route exists yet: S8's /lots, /admin/customers*, /admin/kyc-overrides, /statements/<period>/export). Honest real-vs-simulated labelling, not new hardening.
nginx.conf hardcoded proxy_pass to http://backend:8000, which only
resolves inside docker-compose's own network. Renamed to
nginx.conf.template and switched to \${BACKEND_URL}, rendered by
nginx's own built-in template mechanism
(/docker-entrypoint.d/20-envsubst-on-templates.sh) at container start
- Dockerfile sets a default of http://backend:8000 so the local
docker-compose flow is unaffected; a deployed environment (Azure
Container Apps) overrides it to the backend's actual reachable FQDN.
Verified locally: built the image, confirmed the template renders with
a custom BACKEND_URL and nginx starts and serves the SPA correctly.
Three real bugs found deploying to Azure Container Apps, all reproducible and fixed independently of the platform quirk that surfaced them: - Missing proxy_http_version 1.1 + an unforwarded Connection header caused nginx's own proxy module to return 426 Upgrade Required whenever the ingress-to-container hop set Connection/Upgrade headers nginx then blindly forwarded to a plain HTTP/1.1 upstream that never completed the requested upgrade. - proxy_buffering was never disabled for /api/, so the chat SSE endpoint would have buffered its entire response instead of streaming tokens incrementally. - BACKEND_URL should reference the backend Container App's cluster-internal short name (http://backend), not its public FQDN -- routing east-west traffic back out through the environment's shared external ingress gateway was consistently unreliable (intermittent TLS resets), while the internal service name is stable. Verified against a live deployment: 10/10 consecutive requests succeeded, plus a full register -> login -> portfolios -> fees -> chat SSE smoke test through the public frontend URL.
…token Stripe's fixed test-mode tokens (pm_card_visa etc.) each materialize a new PaymentMethod object on .attach() -- the real id differs from the token passed in. StripeBillingAdapter reused the input id for the follow-up Customer.modify() call, which fails Stripe's own "must be attached to the customer" check whenever a fixed test token is used (as opposed to a genuine pre-existing pm_... id). Found live: deploying the MVP with a real Stripe test-mode key and exercising POST /payment-methods end-to-end surfaced a real 500 that no prior test caught, because the contract test's own assertion (stripe_payment_method_id == the input token) only held for the fake adapter and silently encoded the same wrong assumption. Loosened that assertion to what the port actually guarantees; the real-adapter contract test (requires_credentials) now passes against the live sandbox.
README gets a Deployment section with the live Azure Container Apps URLs and an explicit list of what's deferred; CHANGELOG and DECISION-LOG record the MVP-first pivot, the nginx and Stripe fixes found only by actually deploying, and why Key Vault was set up for real rather than deferred (FLASK_ENV=production refuses to boot without it, and dropping to development mode would have disabled HSTS/forced-HTTPS/the session cookie's Secure flag).
Phase 1 of closing the gap audit: root CLAUDE.md requires lint, unit + integration + API tests, coverage thresholds, dependency + secret scanning, SAST, Docker build + image scan, and an Alembic migration check on every PR, plus CD to Azure Container Apps gated on green CI and manual approval -- none of this existed. - .github/workflows/ci.yml: backend (ruff, mypy --strict, lint-imports, full pytest suite with the 77% coverage floor against real Postgres/Redis service containers, pip-audit, bandit, Alembic migration-drift check) and frontend (oxlint, build, npm audit) jobs, plus Gitleaks secret scanning and Trivy image scans on both Docker builds. - .github/workflows/cd.yml: on green CI on main, build + push commit-SHA-tagged images (never :latest -- Container Apps does not reliably re-pull a moved tag), run migrations via the existing backend-migrate Container Apps Job, then update both Container Apps. Gated on the `production` GitHub Environment's manual approval. - .github/dependabot.yml: uv, npm, GitHub Actions, and both Dockerfiles. Fixed while proving the backend job actually goes green: - test_cash_lock_serialization.py's concurrency test compared which of two independent Postgres connections' commit/unblock notifications a Python thread noticed first -- a genuine two-connection race, not a thread-scheduling one, so no amount of thread synchronization fixed it (confirmed by trying). Rewrote the assertion to measure the withdrawal thread's own blocking duration against its own monotonic clock instead; verified deterministic across 20 consecutive runs. - bandit flagged two pre-existing, reviewed B608 false positives (static owner-authored DDL and an already-validated dynamic-SQL wrapper). Suppressed precisely at both lines with justification -- the DDL one needed splitting its assignment from its .format() call first, since the flagged line was mid-string and a same-line comment there would have corrupted the generated SQL; verified the generated SQL is byte-identical and the chat security-perimeter tests still pass. Verified: 687 backend tests green (0 failed), coverage 82.82% against the 77% floor, ruff/mypy/import-linter clean, bandit clean, migration drift check clean, frontend lint/build/audit clean, both Docker images build on linux/amd64. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
The CI workflow from the previous commit had never actually run against a truly clean checkout. It surfaced five genuine, pre-existing issues -- none introduced by this change, all previously masked by every local dev machine having a backend/.env: - ci.yml: mypy/ruff steps ran without `working-directory: backend`, silently dropping backend/pyproject.toml's [tool.mypy] overrides (the pydantic plugin, the flask_*/alpaca/plaid ignore-missing-imports list) -- 34 spurious errors. Fixed by adding the working directory consistently across all backend-quality steps. - ci.yml: the drift-check step's `git status --porcelain` reports paths relative to the repo root even when invoked from a `working-directory: backend` step -- stripped the prefix back off so the following grep/rm resolve the same file. - ci.yml: `npm audit` was missing `working-directory: frontend`, so it ran at the repo root with no lockfile (ENOLOCK). - ci.yml: aquasecurity/trivy-action@0.24.0 doesn't exist (tags are v-prefixed) -- pinned to the real v0.36.0. - ci.yml: the migration-check job passed only DATABASE_URL_OWNER, but `app.config.Settings` validates all 7 required fields eagerly at construction even though alembic/env.py only reads sqlalchemy_url_owner -- added the full (non-secret, ephemeral-DB) env block. - The deepest one: ~20 call sites across every webhook, funding, fees, chat, identity, orders, and admin/rebalance controller plus every scheduled job call the module-level get_settings() singleton directly instead of the app's injected test_settings, so its first construction still needs real env vars -- with no .env present, 22 tests across 5 files failed with "7 validation errors for Settings" the moment CI actually ran clean. Fixed centrally with an autouse `_default_settings_env` fixture in tests/conftest.py (monkeypatch. setenv, reverted per test -- not a session-wide or import-time os.environ mutation, which was tried first and broke test_config.py's own deliberately env-isolated "missing setting" tests). Those tests were in turn hardened to explicitly delenv every COMPLETE key so they no longer depend on ambient env being clean -- matching what their own docstring already claimed. Verified: 687 backend tests green across 3 consecutive runs with a fully clean environment (env -i, no .env, no shell exports) and separately with .env present; ruff/mypy/lint-imports clean. Confirmed live against the actual GitHub Actions run (PR #7), not just locally: this fixes the failures that run surfaced in lint/types, tests, migration-check, dependency-scan, and docker-build-and-scan. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
…ing results Docker build + Trivy scan (PR #7) failed once real vulnerability data was involved: nginx:1.27-alpine's libssl3/libcrypto3 carried CVE-2026-31789 (CRITICAL, fixed upstream in 3.3.7-r0, not yet in that tag's build). Bumped to nginx:1.29-alpine and confirmed via `apk info` it ships 3.5.6-r0 -- but a second local scan still found 34 HIGH findings across libssl3/libuuid/libxml2/nghttp2-libs, all with fixes already released in Alpine's package repo, just not yet folded into Docker Hub's rebuild of that tag. Added `apk update && apk upgrade --no-cache` in the runtime stage so the image tracks Alpine's own (faster) patch cadence instead of waiting on the upstream image rebuild; verified 0 findings with `trivy image` locally, `nginx -t` still validates the rendered config, and the backend image (Debian slim, apt-get) was independently confirmed already clean. Also added `fail-fast: false` to the backend/frontend build matrix -- the frontend leg's failure had silently cancelled the backend leg mid-scan ("The operation was canceled"), hiding its actual result. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
… fetch failure BalanceCard/ReturnCard only branched on status 'loading'/'idle' before falling through to the value-rendering branch -- any other status, including 'error', called formatMoney(null)/formatPercent(null), which construct a Decimal from null and throw (verified directly: `new Decimal(null)` -> "DecimalError: Invalid argument: null"). With no error boundary anywhere in the app, a single failed GET /valuation/balance or /valuation/returns took down the whole /dashboard route. Added the explicit status === 'error' branch both components were missing, rendering the same ErrorState-with-retry pattern already used elsewhere (LotTable.jsx's precedent), and threaded error/refetch from useBalance/useReturns through DashboardPage into both cards. Verified: lint and build clean; the DecimalError is unreachable now that 'error' status never reaches formatMoney/formatPercent. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
…vent dispatcher
An independent multi-agent audit (backend-audit) found three blocker-
class gaps: written, unit-tested code that nothing in the running
application ever actually invokes. Verified each by hand before
fixing, per this project's own established discipline.
- register_cli(app) existed with the exact right implementation and
docstring ("Called by the application factory when CLI wiring is
enabled") but create_app() never called it -- `flask jobs <name>`,
the entrypoint Makefile's `job` target and every deployed Container
Apps Job use, had no commands registered at all. One-line fix;
verified `flask jobs --help` now lists all 8 commands.
- No `outbox-worker` CLI command existed. Without it, nothing ever
drains `job_outbox` -- an order placed via POST /orders enqueues
`submit_order_to_broker` and simply sits there forever; same for
`charge_fee`. Added the command, routing each of the three task
types this codebase's services actually enqueue
(submit_order_to_broker, charge_fee, process_inbound_event) through
a new OutboxTaskRouter (app/services/ops/outbox_task_router.py,
unit-tested in isolation -- pure routing logic, no DB/provider
needed, unlike the credential-gated CLI wiring around it).
- `InboundEventDispatcher.register(...)` was never called anywhere --
its own module docstring already said so ("not yet wired end-to-
end... tracked as owed operational work"). This is the one gap that
silently breaks money-moving correctness even with the outbox worker
running: Alpaca fills arrive over a websocket (ADR 22), not a
request/response webhook, so TradeUpdatesConsumer.handle_message
only records the InboundEvent -- applying the fill to the order/
ledger/lot-consumption has no other path. Registered
AlpacaTradeUpdateHandler for InboundEventSource.ALPACA in the new
outbox-worker command. STRIPE/PLAID already apply their effect
synchronously inside their webhook controllers, so their own
process_inbound_event row is a durability record, not a required
second application -- registered as explicit no-ops so the
dispatcher's "unregistered source is a wiring gap" check doesn't
retry-then-dead-letter every Stripe/Plaid webhook's outbox row.
Two more real bugs surfaced only by actually constructing these
objects together for the first time:
- OutboxRepository/OutboxUnitOfWork's Protocols (app/workers/outbox.py)
didn't structurally match the concrete OpsUnitOfWork/JobOutboxRepository
they were written to accept -- outbox_id: object vs the real
uuid.UUID, a bare `outbox: OutboxRepository` attribute vs a real
@cached_property, and a loose `__exit__(*args: object)` vs
UnitOfWork's real 3-argument signature. Fixed to match the exact,
already-proven pattern event_intake.py's equivalent Protocol uses.
Never caught before because nothing had ever type-checked a real
UnitOfWork against these Protocols.
Added `outbox_max_attempts` (Settings, default 5) since OutboxWorker
needs one and no generic setting existed. Added a `worker` service to
docker-compose.yml (same image as `backend`, `flask jobs outbox-worker`
as its command) so local dev actually has a running worker, matching
Makefile's `worker` target which previously had no command to invoke.
Verified: 691 backend tests green (4 new), ruff/mypy --strict/
lint-imports clean. Live smoke test of `flask jobs outbox-worker`
against real local Alpaca paper + Stripe test credentials: clean
startup, correctly constructs real adapters, blocks on LISTEN with no
errors. Repeated with credentials unset: falls back to FakeBrokerAdapter
with a loud warning rather than crashing or silently looking identical
to production. Also verified running as the actual `worker` container
via `docker compose --profile app up worker` -- same clean result.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
S12 §3's own stated rule: every FK gets an explicit supporting index (Postgres does not create one automatically) and every named WHERE/ORDER BY access pattern gets a composite index added at the time the table is created -- never bolted on ad hoc later. An independent audit found 8 of the 11 named indexes missing entirely (verified: none of the seven affected model classes declared them, so this needed adding to the ORM models before autogenerating, not just a hand- written migration -- otherwise the next autogenerate would see them in the DB but not the model metadata and want to drop them again). - account(customer_id, role) -- every cash-policy/balance query, S1 §5 - posting(account_id) -- S1's balance-summing queries - journal_entry(effective_date), journal_entry(recorded_at) -- S1/ADR 1's bitemporal range queries - order(customer_id, status) -- S3 §6's order list/status queries - tax_lot(customer_id, security_id, acquired_at) -- S5 §4's FIFO ordering - reconciliation_break(status, opened_at) -- S7 §7's aged-break list - chat_message(session_id, created_at) -- S11 §6's message history daily_close, order_event, and fee_accrual were already covered by existing UNIQUE-backed indexes; admin_audit_log already had its own. Verified: migration applies clean against real Postgres, all 8 indexes confirmed present via pg_indexes, a second autogenerate against the now-migrated database produces an empty diff (no drift), 691 tests green, ruff/mypy --strict clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
Non-negotiable #5 requires "a working MCP surface, not a design for one" -- minimum three read tools and one write tool landing in a human approval queue, plus a written list of operations never handed to an autonomous agent. Nothing satisfies this today; S11's chat assistant is an HTTP+SSE endpoint, not an MCP server, and has no write path. ADR 24 records the decision and its rationale: read tools reuse ADR 19's proven curated-view/least-privilege-role boundary directly rather than re-deriving it; all three write tools (resolve a reconciliation break, override a KYC verdict, propose a rebalance) only ever insert a pending agent_action_request row -- approval, not the tool call, is what executes the underlying already-tested service method. Includes the required written list of operations never handed to an agent. S13 is the implementation-level spec: the agent_action_request and staff_api_token schemas, the six tool contracts, the four approval routes and their execution mapping back to existing services, edge cases (concurrent approval, revoked token mid-session, a resolved-out- from-under-you break), and the testing strategy including a real MCP client hitting a running server end-to-end. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
- TaxLotRepository.list_for_customer / LotConsumptionRepository.list_for_lots back the new read, matching the unpaginated per-customer list shape valuation history already uses. - Response always reads TaxLot.adjusted_basis / LotConsumption.realized_gain_loss (S5's mutate-in-place fields), never original_cost_basis for gain math, so a wash-sale-adjusted lot can never surface its pre-adjustment figure. - is_provisional aggregates true if any of the lot's own consumptions are still provisional (S5 §7 edge case 1). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
- CSV of lot_consumption rows realized in the as-published period, always the lot's own wash-sale-adjusted basis and each consumption's own is_provisional flag -- never a raw pre-adjustment figure. - An unpublished period returns 404 statement_not_published rather than falling back to a live-derived export (S8 §6 case 1). - LotConsumptionRepository.list_realized_in_period, TaxLotRepository.list_by_ids back the new read. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
- GET /api/v1/admin/customers: case-insensitive email search, cursor-paginated via app/core/pagination.py (Customer carries no name field yet, so email is the only match target). - GET /api/v1/admin/customers/<id>: aggregated view calling the same ValuationService.value_book path GET /valuation/balance uses, and surfacing any open reconciliation break prominently (S8 §6 case 2). - GET /api/v1/admin/customers/<id>/fees: thin re-export of the same FeeSummaryService.summarize GET /fees now calls, extracted from fees.py so both routes can never independently drift on the same customer's figures (S8 §6 case 4's no-drift requirement, applied to fees). - New AdminUnitOfWork (app/services/admin/uow.py) composes FeesUnitOfWork with S7's reconciliation model mixin for this cross-aggregate read. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
- KycService.start_verification now blocks with KycLockedError once kyc_status is rejected AND the latest attempt has exhausted KYC_MAX_ATTEMPTS (S2 §9's own deferred gate) -- a single canceled attempt well below the cap still allows a normal retry (S8 §6 case 3). - POST /identity/kyc-sessions surfaces the block as 422 kyc_locked. - New POST /api/v1/admin/kyc-overrides/<customer_id> (adviser/admin, @Audited) resets customer.kyc_status to pending without touching the append-only kyc_session table, per S2 §9's own text on how reopening works. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VwHwPi8sLxxcmbJ1ez8u2p
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
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.
Summary
Two pieces of work landed together since they're tightly coupled (the deploy config the MVP needed is what CI/CD now also verifies):
MVP (already deployed and live)
mockClient.jsfor those four domains. Lots, admin-customers, and statement export stay honestly labelled "Simulated" (no backend routes yet).StripeBillingAdapter.attach_payment_methodwas reusing the caller-supplied token instead of the attachedPaymentMethod's own id, which Stripe's fixed test tokens require.proxy_http_version 1.1,Connectionheader hygiene, and SSE buffering — all found and fixed against the live deployment.trueup-mvp-rg,centralus): Postgres Flexible Server, Key Vault-backed envelope encryption via managed identity, Redis, backend + frontend apps. Live athttps://frontend.proudmeadow-36949862.centralus.azurecontainerapps.io.CI/CD (this session, Phase 1 of a broader gap-closure plan)
.github/workflows/ci.yml: backend (lint, mypy --strict, import-linter, full test suite + 77% coverage floor against real Postgres/Redis, pip-audit, bandit, Alembic drift check) and frontend (lint, build, npm audit) jobs, plus Gitleaks secret scanning and Trivy image scans..github/workflows/cd.yml: on green CI onmain, build + push commit-SHA-tagged images, run migrations via thebackend-migrateContainer Apps Job, deploy both apps — gated on theproductionGitHub Environment's manual approval..github/dependabot.ymlfor uv/npm/Actions/Docker.test_cash_lock_serialization.py) discovered while proving the new CI job goes green reliably — it compared which of two independent Postgres connections' notifications a Python thread noticed first, which is not orderable; rewrote it to measure blocking duration on its own monotonic clock instead. Verified deterministic across 20 runs.Test plan
make checkequivalent run locally: ruff, mypy --strict, lint-imports all cleanbandit -r app -ll: 0 findingsnpm auditall cleanlinux/amd64locallyproductionGitHub Environment (manual-approval reviewers) andAZURE_CLIENT_ID/AZURE_TENANT_ID/AZURE_SUBSCRIPTION_IDOIDC secrets still need to be configured in repo settings beforecd.ymlcan run — flagging explicitly rather than silently, this needs repo-admin access I don't have from hereSecurity / DB / API impact
SECRET_KEY) currently exist only as local plaintext files — flagged to the user for rotation into a proper secret store, not fixed in this PR.🤖 Generated with Claude Code