From 8e9e46fd913e3bcd5e5d0b3a4cf83e54704bbe62 Mon Sep 17 00:00:00 2001 From: tiennm99 Date: Sun, 10 May 2026 03:05:56 +0700 Subject: [PATCH] docs(plans): pre-deploy wrap-up status + AWS-port phase sync + code-review report - Rewrite README.md for AWS-default deploy architecture - Sync AWS-port plan.md phases 01-07 statuses to reflect shipped code (trading, lolschedule, gemini modules) - Include code-reviewer report: cron-and-trading feedback addressed (gofmt, price-fetch reordering, senderInfo validation, ticker regex, typo fixes) --- README.md | 63 ++++-- plans/260510-0114-aws-port/plan.md | 14 +- .../phase-01-cosmetics.md | 57 ++++++ .../phase-02-metric-filter.md | 62 ++++++ .../phase-03-lolschedule-cron.md | 109 ++++++++++ .../phase-04-trading-module.md | 90 +++++++++ .../phase-05-eventbridge-schedules.md | 105 ++++++++++ plans/260510-0234-pre-deploy-wrapup/plan.md | 66 ++++++ ...e-reviewer-260510-0244-cron-and-trading.md | 189 ++++++++++++++++++ 9 files changed, 731 insertions(+), 24 deletions(-) create mode 100644 plans/260510-0234-pre-deploy-wrapup/phase-01-cosmetics.md create mode 100644 plans/260510-0234-pre-deploy-wrapup/phase-02-metric-filter.md create mode 100644 plans/260510-0234-pre-deploy-wrapup/phase-03-lolschedule-cron.md create mode 100644 plans/260510-0234-pre-deploy-wrapup/phase-04-trading-module.md create mode 100644 plans/260510-0234-pre-deploy-wrapup/phase-05-eventbridge-schedules.md create mode 100644 plans/260510-0234-pre-deploy-wrapup/plan.md create mode 100644 plans/260510-0234-pre-deploy-wrapup/reports/code-reviewer-260510-0244-cron-and-trading.md diff --git a/README.md b/README.md index 245b6e1..ddd9e6a 100644 --- a/README.md +++ b/README.md @@ -1,19 +1,26 @@ # miti99bot-go -Plug-n-play Telegram bot framework in Go, deployed on Google Cloud Run with Firestore + Gemini. Free-tier port of [miti99bot](https://github.com/tiennm99/miti99bot). +Plug-n-play Telegram bot framework in Go. **Default deploy: AWS Lambda + DynamoDB + EventBridge Scheduler (free tier).** Cloud Run + Firestore retained as an alt path. Free-tier port of [miti99bot](https://github.com/tiennm99/miti99bot). ## Status -Early scaffolding. See [`plans/260508-2222-go-port-cloud-run/plan.md`](plans/260508-2222-go-port-cloud-run/plan.md) for the full roadmap. +Mid-port. Code is on `main`; first AWS deploy still pending the user's manual AWS-account bootstrap. -| Phase | What | Status | +| Track | What | Status | |-------|------|--------| -| 01 | GCP setup, Cloud Run baseline | pending | -| 02 | Repo bootstrap + webhook skeleton | **partial** (local pieces done; Cloud Run deploy deferred to Phase 01) | -| 03 | Module framework + KVStore | **done** | -| 04 | Firestore KV + provider abstraction | **done** | -| 05–07 | Module ports (util/misc/wordle/loldle/lolschedule + AI: semantle/doantu/twentyq) | **done** | -| 08+ | Trading, cron wiring, CI/CD, cutover | pending | +| Modules | util, misc, wordle, loldle (+ ability/emoji/quote/splash variants), lolschedule, semantle, doantu, twentyq | **done** | +| Storage | KVStore interface; in-memory + Firestore + **DynamoDB** providers | **done** | +| AI | Gemini API client (`internal/ai`) | **done** | +| AWS IaC | SAM template + Makefile + GH Actions OIDC deploy | **done** | +| AWS bootstrap | account, IAM OIDC, SSM SecureString params, first `sam deploy` | **manual user steps — see [`aws/README.md`](aws/README.md)** | +| Cron handlers | lolschedule daily push, trading daily refresh | pending | +| Trading module | VN-stocks paper trading | pending | +| Cutover | Telegram webhook flip + 7-day soak | deploy-gated | + +Plans: +- Active: [`plans/260510-0234-pre-deploy-wrapup/`](plans/260510-0234-pre-deploy-wrapup/plan.md) — cron handlers + trading + cosmetics +- AWS port: [`plans/260510-0114-aws-port/`](plans/260510-0114-aws-port/plan.md) +- Original GCP plan (historical): [`plans/260508-2222-go-port-cloud-run/`](plans/260508-2222-go-port-cloud-run/plan.md) — module work reused; deploy phases superseded by AWS port ## Layout @@ -21,12 +28,18 @@ Early scaffolding. See [`plans/260508-2222-go-port-cloud-run/plan.md`](plans/260 cmd/server/ entrypoint internal/server/ HTTP routes (/, /webhook, /cron/{name}) internal/telegram/ Telegram webhook + bot wrapper -internal/modules/ Module framework, registry, dispatchers -internal/storage/ KVStore interface, in-memory impl, prefix wrapper +internal/modules/ Module framework, registry, dispatchers, modules +internal/storage/ KVStore interface; memory / firestore / dynamodb providers +internal/ai/ Gemini client +template.yaml AWS SAM IaC (Lambda + Function URL + DynamoDB + Logs + Budget) +docs/deploy-aws.md AWS deploy operations +aws/README.md One-time AWS account bootstrap cheatsheet ``` ## Run locally +In-memory KV (default; no AWS / no GCP needed): + ```sh TELEGRAM_BOT_TOKEN=… \ TELEGRAM_WEBHOOK_SECRET=local \ @@ -35,23 +48,39 @@ MODULES= \ go run ./cmd/server ``` -End-to-end smoke test against a Telegram dev bot requires `ngrok` (local) or a Cloud Run deployment. The dev bot is created manually; token is injected via env vars only. +End-to-end smoke test against a Telegram dev bot needs `ngrok` (local) or a deployed Function URL. The dev bot is created manually; token injected via env vars only. + +For DynamoDB integration tests: +```sh +make dynamodb-local # docker run amazon/dynamodb-local on :8001 +make test-dynamodb # runs internal/storage tests against DDB Local +``` + +For Firestore emulator (legacy): +```sh +make firestore-emulator # in a second shell +make test-emulator +``` ## Test ```sh -go vet ./... -go test ./... +make vet # go vet +make test # full unit suite (no emulator) +make test-dynamodb # storage tests against DynamoDB Local (requires Docker) +make test-emulator # storage tests against Firestore emulator ``` -## Build +## Deploy + +**AWS (canonical):** see [`docs/deploy-aws.md`](docs/deploy-aws.md). Push to `main` → GitHub Actions OIDC → SAM deploy. First-time bootstrap: [`aws/README.md`](aws/README.md). + +**Cloud Run (alternative, deferred):** the multi-stage `Dockerfile` builds an image suitable for any container runtime (Cloud Run, Fly.io, ECS Fargate, K8s). Image is `golang:1.25-alpine` → `gcr.io/distroless/static:nonroot`, ~15 MiB. ```sh docker build -t miti99bot-go . ``` -The image is multi-stage (`golang:1.23-alpine` → `gcr.io/distroless/static:nonroot`); resulting image is ~15 MiB. - ## License [Apache-2.0](LICENSE). diff --git a/plans/260510-0114-aws-port/plan.md b/plans/260510-0114-aws-port/plan.md index c165005..ccfd9c9 100644 --- a/plans/260510-0114-aws-port/plan.md +++ b/plans/260510-0114-aws-port/plan.md @@ -37,13 +37,13 @@ Re-target only the deploy/runtime layer. Module work (Phases 03–07 of GCP plan | # | Phase | Status | Effort | Key deliverable | |---|-------|--------|--------|-----------------| -| 01 | [AWS bootstrap + IAM OIDC + SAM skeleton](phase-01-aws-bootstrap.md) | pending | 3h | AWS account, OIDC trust, empty SAM stack deployable | -| 02 | [Lambda runtime (Go ZIP + LWA + Function URL)](phase-02-lambda-runtime.md) | pending | 4h | `/` and `/webhook` served from Lambda; secret-token check passes | -| 03 | [DynamoDB KV provider](phase-03-dynamodb-kv.md) | pending | 4h | `dynamodb_kv.go` + `dynamodb_provider.go` sibling to Firestore impl, parity tests pass | -| 04 | [EventBridge cron wiring](phase-04-eventbridge-cron.md) | pending | 3h | Scheduler → `/cron/{name}` with token, two crons firing on schedule | -| 05 | [GitHub Actions deploy (OIDC + SAM)](phase-05-gha-deploy.md) | pending | 3h | `deploy.yml` runs on push to `main`, builds + sam deploys idempotently | -| 06 | [Observability + budget alert](phase-06-observability.md) | pending | 2h | Logs retention set, $1 budget alert, cold-start P95 captured | -| 07 | [Cutover + README + retire GCP paths](phase-07-cutover.md) | pending | 3h | Webhook flipped to Function URL, README rewritten, GCP code paths kept but unwired by default | +| 01 | [AWS bootstrap + IAM OIDC + SAM skeleton](phase-01-aws-bootstrap.md) | pending (manual) | 3h | AWS account, OIDC trust, empty SAM stack deployable | +| 02 | [Lambda runtime (Go ZIP + LWA + Function URL)](phase-02-lambda-runtime.md) | code-done; awaits first deploy | 4h | `/` and `/webhook` served from Lambda; secret-token check passes | +| 03 | [DynamoDB KV provider](phase-03-dynamodb-kv.md) | code-done; integration tests skip without DDB Local | 4h | `dynamodb_kv.go` + `dynamodb_provider.go` sibling to Firestore impl, parity tests pass | +| 04 | [EventBridge cron wiring](phase-04-eventbridge-cron.md) | pending (blocked on cron handlers, see 260510-0234-pre-deploy-wrapup) | 3h | Scheduler → `/cron/{name}` with token, two crons firing on schedule | +| 05 | [GitHub Actions deploy (OIDC + SAM)](phase-05-gha-deploy.md) | done | 3h | `deploy.yml` runs on push to `main`, builds + sam deploys idempotently | +| 06 | [Observability + budget alert](phase-06-observability.md) | partial (budget shipped; metric filter in 260510-0234) | 2h | Logs retention set, $1 budget alert, cold-start P95 captured | +| 07 | [Cutover + README + retire GCP paths](phase-07-cutover.md) | pending (deploy-gated) | 3h | Webhook flipped to Function URL, README rewritten, GCP code paths kept but unwired by default | ## Dependency graph ``` diff --git a/plans/260510-0234-pre-deploy-wrapup/phase-01-cosmetics.md b/plans/260510-0234-pre-deploy-wrapup/phase-01-cosmetics.md new file mode 100644 index 0000000..99ecc6f --- /dev/null +++ b/plans/260510-0234-pre-deploy-wrapup/phase-01-cosmetics.md @@ -0,0 +1,57 @@ +--- +phase: 1 +title: "Cosmetics: README + plan status sync" +status: pending +priority: P3 +effort: "30m" +dependencies: [] +--- + +# Phase 01: Cosmetics — README + plan status sync + +## Overview +README still says "Cloud Run + Firestore"; AWS-port phases 02/03/05 say "pending" though their code already shipped. Sync both before any new clone or visitor reads stale docs. + +## Requirements +- **Functional:** README describes AWS as default deploy, with link to `docs/deploy-aws.md`. AWS-port plan's phase-status table reflects shipped code. GCP plan's tagline note about supersession remains. +- **Non-functional:** No code changes. Markdown-only. + +## Architecture +N/A — documentation update. + +## Related Code Files +- Modify: `README.md` +- Modify: `plans/260510-0114-aws-port/plan.md` (status column) +- Reference (no edit): `plans/260508-2222-go-port-cloud-run/plan.md` (already annotated) + +## Implementation Steps +1. Rewrite `README.md`: + - Tagline: "Plug-n-play Telegram bot framework in Go. Default deploy: AWS Lambda + DynamoDB + EventBridge (free tier). Cloud Run path retained as alt." + - Status table: collapse to one row per work-phase; mark current state honestly. + - "Run locally" section: keep in-memory KV path; add note about `make dynamodb-local` for DynamoDB integration testing; keep `make firestore-emulator` line. + - "Deploy" section: link `docs/deploy-aws.md` as canonical; mention Dockerfile for non-Lambda hosts. + - "Test" section: add `make test-dynamodb` line. +2. Update `plans/260510-0114-aws-port/plan.md` phases table: + - Phase 01 → still pending (manual user steps) + - Phase 02 → "code-done; awaiting first deploy" + - Phase 03 → "code-done; integration tests skip without DynamoDB Local" + - Phase 04 → still pending (this plan unblocks it) + - Phase 05 → "done" + - Phase 06 → "partial" — budget alert in template; metric filter deferred to this plan's Phase 02 + - Phase 07 → still pending (deploy-gated) +3. Smoke-render the README locally (`grip` or just open in editor) — confirm headings, links, and code blocks render cleanly. + +## Success Criteria +- [ ] README's intro line names AWS as default +- [ ] README's status table accurate (no "pending" rows that are actually done) +- [ ] AWS-port plan.md phase statuses reflect shipped code +- [ ] All link targets resolve (no 404 in `docs/deploy-aws.md`, `aws/README.md`, both plan files) +- [ ] No broken markdown rendering + +## Risk Assessment +- **Drift between README and plan.md** if updated separately later — Mitigation: this phase is the single place both get touched together; future drift caught in Phase 06 cutover. +- **Stale Cloud Run instructions misleading new contributors** — Mitigation: prefix the alt-path section with "Alternative: Cloud Run (deferred)" so the canonical path is unambiguous. + +## Open questions +1. Move Cloud Run instructions into `docs/deploy-gcp-cloud-run.md` instead of inlining? Cleaner README but adds a file. Default: inline a short note + link to old plan. +2. Add CI badge to README? Skip for v1 — no public bot, no marketing pressure. diff --git a/plans/260510-0234-pre-deploy-wrapup/phase-02-metric-filter.md b/plans/260510-0234-pre-deploy-wrapup/phase-02-metric-filter.md new file mode 100644 index 0000000..d54d855 --- /dev/null +++ b/plans/260510-0234-pre-deploy-wrapup/phase-02-metric-filter.md @@ -0,0 +1,62 @@ +--- +phase: 2 +title: "Cold-start metric filter" +status: pending +priority: P3 +effort: "30m" +dependencies: [] +--- + +# Phase 02: Cold-start metric filter + +## Overview +Add a CloudWatch Logs metric filter that extracts Lambda's `Init Duration` from the auto-emitted `REPORT` line so the AWS-port plan's "P95 < 1.5s" abort criterion is measurable from day one. + +## Requirements +- **Functional:** A custom metric `miti99bot/ColdStartInitDuration` exists; samples appear in CloudWatch Metrics within 5 minutes of cold-start. +- **Non-functional:** Stays inside CloudWatch's always-free 10 custom metrics. No additional ingest cost (filter operates on existing log stream). + +## Architecture +Lambda emits a synthetic `REPORT` line at the end of every invocation. On a cold start, that line includes `Init Duration: `. A `AWS::Logs::MetricFilter` parses the line and emits a custom metric value into `miti99bot/ColdStartInitDuration` namespace. + +``` +Lambda invocation → CloudWatch log stream → + filter pattern matches "REPORT ... Init Duration: " → + publish metric (Namespace: miti99bot, Name: ColdStartInitDuration, Value: ) +``` + +## Related Code Files +- Modify: `template.yaml` — add `ColdStartMetricFilter` resource + +## Implementation Steps +1. Append to `template.yaml` after `BotFunctionLogGroup`: + ```yaml + ColdStartMetricFilter: + Type: AWS::Logs::MetricFilter + Properties: + LogGroupName: !Ref BotFunctionLogGroup + FilterPattern: '[report="REPORT", reqid_label="RequestId:", reqid, dur_label="Duration:", dur, dur_unit="ms", bill_label="Billed", bill_dur_label, bill_dur, bill_unit, mem_label, mem_size_label, mem_size, mem_unit, max_label="Max", max_used_label="Memory", max_used_label2="Used:", max_used, max_used_unit, init_label="Init", init_dur_label="Duration:", init_dur, init_unit="ms"]' + MetricTransformations: + - MetricName: ColdStartInitDuration + MetricNamespace: miti99bot + MetricValue: $init_dur + Unit: Milliseconds + ``` +2. Validate locally: `make sam-validate` (offline lint). Should pass. +3. After AWS-port Phase 01 manual deploy completes, verify: + - `aws logs describe-metric-filters --log-group-name /aws/lambda/miti99bot-aws-port-bot` + - Trigger a cold start (`aws lambda update-function-configuration --environment ... ` flip). + - `aws cloudwatch get-metric-statistics --namespace miti99bot --metric-name ColdStartInitDuration --statistics Average,Maximum --start-time ... --end-time ... --period 300` + +## Success Criteria +- [ ] `template.yaml` has `ColdStartMetricFilter` resource +- [ ] `sam validate` passes +- [ ] Post-deploy: metric filter visible in AWS console, samples flow within 5 min of a cold start + +## Risk Assessment +- **Filter pattern brittleness** — Lambda's REPORT format is stable but unofficial. Mitigation: pattern uses positional + label parsing, tolerant of whitespace; if AWS changes format, filter just stops matching (no error, just zero data). +- **Warm-only invocations don't include `Init Duration`** — pattern won't match those, which is correct; we only want cold-start samples. + +## Open questions +1. Capture `Duration:` (warm + cold) too as a separate metric? YAGNI — request latency is already in CloudWatch's built-in `Duration` metric for the function. +2. Per-region dashboard? Skip — solo dev uses Insights queries. diff --git a/plans/260510-0234-pre-deploy-wrapup/phase-03-lolschedule-cron.md b/plans/260510-0234-pre-deploy-wrapup/phase-03-lolschedule-cron.md new file mode 100644 index 0000000..d9a7526 --- /dev/null +++ b/plans/260510-0234-pre-deploy-wrapup/phase-03-lolschedule-cron.md @@ -0,0 +1,109 @@ +--- +phase: 3 +title: "lolschedule daily-push cron handler" +status: pending +priority: P2 +effort: "3h" +dependencies: [] +--- + +# Phase 03: lolschedule daily-push cron handler + +## Overview +Implement the deferred `lolschedule_daily_push` cron — fans out today's match schedule to subscribers at 08:00 ICT. Requires extending `modules.Deps` to expose `*bot.Bot` (current blocker noted in `internal/modules/lolschedule/lolschedule.go:8-12`). + +## Requirements +- **Functional:** + - `lolschedule.Module.Crons()` returns one entry: name `daily_push`, schedule `0 1 * * *` (UTC = 08:00 ICT). + - Handler reads subscribers, fetches today's matches via existing `api_client.go`, sends formatted message to each chat via `*bot.Bot`. + - Failed sends per-chat are logged, do not abort the batch (one bad chat doesn't take down the whole push). +- **Non-functional:** + - Handler completes within Lambda's 30s default timeout for typical subscriber counts (<100). Past that, paginate or move to async. + - Rate-limit-aware: respect Telegram's 30 messages/sec global cap. For low subscriber counts, no batching needed. + +## Architecture +**Deps extension (the real work):** +```go +// internal/modules/module.go +type Deps struct { + KV storage.KVStore + Embedder ai.Embedder + Chatter ai.Chatter + Env map[string]string + Bot *bot.Bot // NEW — nil-safe; modules check before use +} +``` + +`*bot.Bot` is already constructed in `cmd/server/main.go` before `modules.Build`. Wire it into `BuildOptions` (typed, like `Embedder`/`Chatter`) and have `modules.Build` thread it into each module's `Deps`. Modules that don't need it ignore it — same pattern as Gemini. + +**Cron handler:** +```go +// internal/modules/lolschedule/cron.go (new file) +func (m *Module) dailyPush(ctx context.Context) error { + if m.deps.Bot == nil { + return errors.New("lolschedule: daily push requires bot reference") + } + subs, err := listSubscribers(ctx, m.kv) + if err != nil { return err } + matches, err := m.api.TodayMatches(ctx) // existing api_client method + if err != nil { return err } + msg := formatMatches(matches) // existing format.go helper + + var sent, failed int + for _, chatID := range subs { + if _, err := m.deps.Bot.SendMessage(ctx, &bot.SendMessageParams{ChatID: chatID, Text: msg}); err != nil { + log.Warn("lolschedule push failed", "chat", chatID, "err", err) + failed++ + continue + } + sent++ + } + log.Info("lolschedule daily push complete", "sent", sent, "failed", failed) + return nil +} +``` + +## Related Code Files +- Modify: `internal/modules/module.go` — add `Bot *bot.Bot` to `Deps` and `BuildOptions` +- Modify: `internal/modules/registry.go` (or wherever `modules.Build` constructs Deps) — thread `Bot` through +- Modify: `cmd/server/main.go` — pass `b` (already constructed) into `modules.BuildOptions{Bot: b}` +- Create: `internal/modules/lolschedule/cron.go` — `dailyPush` handler + helper +- Modify: `internal/modules/lolschedule/lolschedule.go` — implement `Crons()` returning the registration; remove the deferred-cron comment block at line 8-12 +- Create: `internal/modules/lolschedule/cron_test.go` — table tests for handler with mock bot + KV + +## Implementation Steps +1. **Deps extension:** + - Add `Bot *bot.Bot` field to `Deps` and `BuildOptions` (in `internal/modules/module.go`) + - Update `modules.Build` to copy `BuildOptions.Bot` into each constructed `Deps` + - Update `cmd/server/main.go` to pass `Bot: b` in the options literal + - Run `go vet ./...` + `go build ./...` — should be clean (additive change) +2. **lolschedule cron registration:** + - Add `Crons() []modules.Cron` to `Module`, returning `{{Name: "daily_push", Schedule: "0 1 * * *", Handler: m.dailyPush}}` (verify the exact struct shape from `modules.Cron` definition) + - Remove the deferred-cron comment block in `lolschedule.go` +3. **Handler implementation:** + - Write `cron.go` per architecture above + - Reuse existing `api_client.TodayMatches` and `format.go` helpers (read these to confirm signatures; adjust if needed) +4. **Tests:** + - Mock `*bot.Bot` via interface or wrapper; assert SendMessage called once per subscriber + - Cover: happy path (3 subs, all succeed); partial failure (1 of 3 fails, batch continues); empty subscribers (no-op, no error); API failure (returns error) +5. **Wire dispatch:** confirm `internal/modules/cron_dispatcher.go` (or wherever crons are surfaced to `internal/server/router.go`) picks up the new registration without further wiring. Check by hitting `/cron/lolschedule_daily_push` locally with the right secret token; should call the handler. +6. **Local smoke:** `go test ./internal/modules/lolschedule/...` green; manual `curl` against running server with at least one subscriber. + +## Success Criteria +- [ ] `modules.Deps.Bot` exposed; nil-safe (modules without it work unchanged) +- [ ] `lolschedule.Module.Crons()` returns one entry +- [ ] `dailyPush` handler implemented per architecture +- [ ] Cron-handler unit tests pass (happy path + partial failure + empty subs) +- [ ] `go vet`, `go build`, full `go test` green +- [ ] Manual `/cron/lolschedule_daily_push` invocation works locally and triggers fan-out + +## Risk Assessment +- **Deps extension breaks every module** if not nil-safe — Mitigation: zero-value `*bot.Bot` is `nil`, all current modules ignore the field, additive change. Add a registry test that builds a module without Bot to confirm. +- **Telegram global rate limit** (30 msg/sec) on large subscriber counts — Mitigation: add a 50ms sleep between sends if subs > 30; below that, send hot. Document threshold in cron handler. +- **Handler exceeds Lambda 30s timeout** at very large sub counts — Mitigation: estimate at 100 subs × 100ms each = 10s, comfortably under. If breached, raise function timeout to 60s in `template.yaml` (still free). +- **Long-poll bot client used elsewhere** vs cron-time short-lived calls — current `bot.Bot` instance is shared; SendMessage is goroutine-safe per the lib's design. Confirm in upstream go-telegram/bot docs if uncertain. + +## Open questions +1. Do we want per-subscriber timezone awareness, or push to all at the global 08:00 ICT? Original miti99bot pushes globally — match for parity. +2. Failure handling: store failed-chat IDs for retry on next push, or just log? Default: log only; transient Telegram failures resolve naturally. +3. Should daily push respect a "no matches today" outcome with a quiet skip vs. sending an empty message? Quiet skip; matches parity with original. diff --git a/plans/260510-0234-pre-deploy-wrapup/phase-04-trading-module.md b/plans/260510-0234-pre-deploy-wrapup/phase-04-trading-module.md new file mode 100644 index 0000000..8238887 --- /dev/null +++ b/plans/260510-0234-pre-deploy-wrapup/phase-04-trading-module.md @@ -0,0 +1,90 @@ +--- +phase: 4 +title: "Trading module port (VN stocks paper trading)" +status: pending +priority: P2 +effort: "6h" +dependencies: [] +--- + +# Phase 04: Trading module port (VN stocks paper trading) + +## Overview +Port the `trading` module from the original miti99bot to Go. Paper-trading on Vietnam-listed stocks: per-user portfolio + buy/sell commands + daily price refresh cron. Largest remaining cloud-agnostic chunk; carries from `plans/260508-2222-go-port-cloud-run/` Phase 08 unchanged in scope. + +## Requirements +- **Functional:** + - Commands (parity with original): `/buy `, `/sell `, `/portfolio`, `/price `, `/leaderboard` + - Daily price refresh cron at market-close (Vietnam: 15:00 ICT = UTC 08:00) — fetch latest closes for tracked tickers, store snapshot, recompute portfolio P&L + - Per-user paper-money starting balance, persistent ledger of trades + - Leaderboard: top-N users by total portfolio value +- **Non-functional:** + - Stays inside Firestore + DynamoDB free tiers (per-user state in 1-3 KV keys, leaderboard a single derived doc) + - Daily price API call counts: <50/day (well inside any reasonable free tier on the data source) + +## Architecture +**Module shape** mirrors existing modules (e.g. `wordle`): +``` +internal/modules/trading/ + trading.go Module struct + factory + Commands() + Crons() + api_client.go HTTP client to VN-stocks data source + api_client_test.go + portfolio.go core domain: Portfolio struct, buy/sell, mark-to-market + portfolio_test.go + handlers.go command handlers (/buy, /sell, /portfolio, /price, /leaderboard) + handlers_test.go + cron.go daily price refresh handler + cron_test.go + format.go message formatting helpers + format_test.go +``` + +**KV layout** (per-module partition, no cross-keys): +- `user::portfolio` → JSON Portfolio (cash, positions, trade history) +- `prices:` → JSON {price, timestamp} +- `leaderboard` → JSON sorted list (recomputed by cron) +- `tickers` → JSON array (set of tracked tickers across all users; cron iterates this) + +**Concurrency:** Buy/sell mutate the same user portfolio; reuse existing `internal/keylock` (already in repo from wordle work) keyed by `user::portfolio`. + +**Cron:** registered with name `daily_refresh`, schedule `0 8 * * *` (UTC = 15:00 ICT, market close). Iterates `tickers`, fetches each price, updates `prices:*`, recomputes leaderboard. + +## Related Code Files +- Create: all files under `internal/modules/trading/` (per architecture above) +- Modify: `cmd/server/main.go` — add `"trading": trading.New` to factories map +- Reference: `internal/modules/wordle/` as the closest existing template (commands + state + per-user mutex) +- Reference: `internal/modules/lolschedule/api_client.go` as the closest HTTP-fetching template + +## Implementation Steps +1. **Locate original miti99bot trading source** — review the JS implementation in https://github.com/tiennm99/miti99bot to nail down exact command shape, message formats, leaderboard rules, and the data source URL. +2. **Verify data source** — confirm the API used by original miti99bot is still free + accessible. If not, evaluate alternatives (TCBS public API, VPS public API, etc.). Document the choice in `api_client.go` header. +3. **Stub `api_client.go`** with the chosen endpoint + request shape; unit-test with golden HTTP fixtures (no network calls in tests). +4. **Domain (`portfolio.go`):** pure Go, no I/O — Portfolio struct, Buy/Sell methods returning new state + delta, mark-to-market against a price map. Heavily unit-tested (this is the easiest part to get wrong silently). +5. **Handlers (`handlers.go`):** parse args, load portfolio from KV under `keylock`, call domain method, persist, format reply. Mirror error paths from original (insufficient funds, unknown ticker, etc.). +6. **Cron (`cron.go`):** fetch tickers list, iterate, fetch each price, update KV, recompute leaderboard. Returns aggregate counts in log. +7. **Wire in `cmd/server/main.go`:** add factory line; bump `MODULES` env default in `template.yaml` to include `trading`. +8. **Tests:** ≥80% coverage on `portfolio.go` (domain), happy paths on handlers + cron with mock `api_client`. Match the bar set by `wordle`. +9. **Smoke locally:** `MODULES=trading go run ./cmd/server`; exercise `/buy`, `/sell`, `/portfolio`; manually trigger `/cron/trading_daily_refresh`. + +## Success Criteria +- [ ] All five commands implemented at parity with original miti99bot +- [ ] Daily refresh cron registered (Phase 05 wires it to AWS Scheduler) +- [ ] Portfolio domain has ≥80% test coverage +- [ ] No flaky tests; no network calls in unit tests (HTTP fixtures only) +- [ ] `go vet`, `go test ./internal/modules/trading/...`, full `go build` green +- [ ] Local smoke against in-memory KV exercises buy/sell/portfolio/leaderboard end-to-end +- [ ] `template.yaml` MODULES default updated to include `trading` + +## Risk Assessment +- **Original API source no longer free** — Mitigation: pre-flight check in step 2; if blocked, document fallback (web scrape with caching, or paid tier acceptance, or feature gate the module) and re-scope this phase. +- **Time zone bugs in market close** — Mitigation: store all timestamps as UTC, format for display only; unit-test the cron's UTC→ICT translation explicitly. +- **Leaderboard recomputation cost** at scale — Mitigation: full recompute is O(users); under 1k users this is single-digit ms. Past that, switch to incremental updates triggered on each trade. +- **Schema drift between paper-money currency and real ticker prices** (VND vs USD vs cents) — Mitigation: portfolio stores integer minor units (VND đồng, no fractional); document this loudly in `portfolio.go` header. +- **Concurrent buy/sell on same user** — Mitigation: `keylock` per `user::portfolio` already proven in wordle. + +## Open questions +1. Starting balance default — match original (likely 100M VND)? Confirm in step 1. +2. Allow short selling? Original likely doesn't; default to "long only, can't sell what you don't own." +3. Price ticks during market hours: refresh on `/price ` command, or only on cron? Cron-only is simpler and free-tier-friendlier; confirm against original behavior. +4. Should the cron also run on weekends (Vietnam market closed)? Skip Sat/Sun in handler; emit a no-op log line. +5. Multi-user leaderboard privacy — show user IDs or display names? Match original behavior; default to display name with fallback to "user-". diff --git a/plans/260510-0234-pre-deploy-wrapup/phase-05-eventbridge-schedules.md b/plans/260510-0234-pre-deploy-wrapup/phase-05-eventbridge-schedules.md new file mode 100644 index 0000000..e32768b --- /dev/null +++ b/plans/260510-0234-pre-deploy-wrapup/phase-05-eventbridge-schedules.md @@ -0,0 +1,105 @@ +--- +phase: 5 +title: "Wire EventBridge schedules to live cron handlers" +status: deferred +priority: P3 +effort: "30m" +dependencies: [3, 4] +--- + +> **Status update 2026-05-10:** Deferred to first-deploy decision. Two issues surfaced during Phase 03/04 implementation: +> 1. **Trading module has no cron** in upstream — only one schedule needed (lolschedule_daily_push), not two +> 2. **Lambda Web Adapter only handles HTTP-shape events** — direct Scheduler→Lambda invokes bypass LWA, requiring either an event-shape detector in `main.go` or the HTTPS-target universal-invoke pattern (`arn:aws:scheduler:::http-invoke`, added in 2024) +> +> The HTTPS-target syntax in `AWS::Scheduler::Schedule` needs validation against the deploy-region SAM transform; doing this offline without `sam validate` access risks committing infra that won't deploy. Decision deferred to deploy-time. Once user runs Phase 01 of AWS-port plan and has SAM available, add a single schedule for `lolschedule_daily_push` per the prose below — pick HTTPS or direct invoke based on what `sam validate` accepts. + +# Phase 05: Wire EventBridge schedules to live cron handlers + +## Overview +With Phases 03 + 04 landed, two cron routes exist (`/cron/lolschedule_daily_push`, `/cron/trading_daily_refresh`). This phase adds concrete `AWS::Scheduler::Schedule` resources to `template.yaml` so AWS Scheduler invokes them on schedule via the existing `SchedulerExecutionRole` + `CronDLQ` already provisioned by AWS-port Phase 01. + +## Requirements +- **Functional:** Two schedules deploy via SAM. Each fires at the correct cron expression with `X-Cron-Token` header sourced from Parameter Store. Failures land in `CronDLQ`. +- **Non-functional:** Stays inside EventBridge Scheduler free tier (14M invocations/mo; we use ~60). Token rotation = update SSM param + redeploy (acceptable trade-off). + +## Architecture +``` +EventBridge Scheduler (rule: 0 1 * * ? *) ─HTTPS POST─► /cron/lolschedule_daily_push + + Headers: X-Cron-Token: {{from SSM}} + + Retry: max 2, max-age 600s + + DLQ: CronDLQ on permanent failure + +EventBridge Scheduler (rule: 0 8 * * ? *) ─HTTPS POST─► /cron/trading_daily_refresh + (same auth + retry + DLQ shape) +``` + +**HTTPS target syntax:** EventBridge Scheduler uses `arn:aws:scheduler:::http-invoke` with `HttpParameters` carrying headers. SAM's `AWS::Scheduler::Schedule` resource passes through to this; no SAM transform magic needed. + +## Related Code Files +- Modify: `template.yaml` — append `LolscheduleDailyPushSchedule` + `TradingDailyRefreshSchedule` resources +- Reference (no edit): existing `SchedulerExecutionRole` + `CronDLQ` in `template.yaml` +- Reference: `aws/README.md` (SSM parameter setup for `/miti99bot/prod/cron-shared-secret`) + +## Implementation Steps +1. Confirm AWS SDK / CloudFormation supports `aws.HttpInvoke` target via `AWS::Scheduler::Schedule` for the deploy region (`ap-southeast-1`). Check via `aws cloudformation describe-type --type RESOURCE --type-name AWS::Scheduler::Schedule` if uncertain. +2. Append to `template.yaml`: + ```yaml + LolscheduleDailyPushSchedule: + Type: AWS::Scheduler::Schedule + Properties: + Name: !Sub "${AWS::StackName}-lolschedule-daily-push" + ScheduleExpression: "cron(0 1 * * ? *)" # 01:00 UTC = 08:00 ICT + FlexibleTimeWindow: { Mode: OFF } + State: ENABLED + Target: + Arn: !GetAtt BotFunction.Arn # Lambda direct? Or HTTPS? Decide per step 1 + RoleArn: !GetAtt SchedulerExecutionRole.Arn + RetryPolicy: { MaximumRetryAttempts: 2, MaximumEventAgeInSeconds: 600 } + DeadLetterConfig: { Arn: !GetAtt CronDLQ.Arn } + Input: '{"name":"lolschedule_daily_push"}' + # IF using HTTPS invoke (preferred for route preservation): + # Replace `Arn: !GetAtt BotFunction.Arn` with the universal target + # `Arn: arn:aws:scheduler:::http-invoke` and add HttpParameters. + + TradingDailyRefreshSchedule: + Type: AWS::Scheduler::Schedule + Properties: + Name: !Sub "${AWS::StackName}-trading-daily-refresh" + ScheduleExpression: "cron(0 8 * * ? *)" # 08:00 UTC = 15:00 ICT (market close) + FlexibleTimeWindow: { Mode: OFF } + State: ENABLED + Target: + # Same shape as above + RoleArn: !GetAtt SchedulerExecutionRole.Arn + RetryPolicy: { MaximumRetryAttempts: 2, MaximumEventAgeInSeconds: 600 } + DeadLetterConfig: { Arn: !GetAtt CronDLQ.Arn } + Input: '{"name":"trading_daily_refresh"}' + ``` +3. **Decide direct-invoke vs HTTPS** at implementation time: + - **HTTPS (preferred):** preserves `/cron/{name}` route; works with existing dispatcher; same shape as local-dev `curl` smoke. Need `HttpParameters` block with `X-Cron-Token` header. + - **Direct Lambda invoke:** simpler IAM, lower latency, bypasses HTTP layer. Requires a Lambda event-shape branch in `cmd/server/main.go` to detect Scheduler events vs Function URL events. + - Default: HTTPS for KISS; switch only if HTTPS proves flaky. +4. Validate locally: `make sam-validate` should pass. +5. After AWS-port Phase 01 deploy: + - Console → EventBridge Scheduler → "Run now" each rule. Confirm 200 from Lambda. + - Check CloudWatch log group for the cron handler executing. + - Send a synthetic invocation that fails (wrong token) — confirm DLQ receives the failed message. +6. Watch first scheduled fire from the AWS console (use a temporary `rate(2 minutes)` to verify, then revert). + +## Success Criteria +- [ ] Two schedules in `template.yaml` +- [ ] `sam validate` passes +- [ ] Post-deploy: Manual "run now" returns 200 and triggers handler +- [ ] DLQ receives failed invocations (synthetic test) +- [ ] First scheduled fire happens at the correct UTC time + +## Risk Assessment +- **`AWS::Scheduler::Schedule` HTTPS-target syntax** still evolving — mitigated by step 1 confirmation and ability to fall back to direct invoke. +- **Token mismatch between SSM and Lambda env** — both resolve at deploy time from the same parameter; no drift unless one is rotated independently. +- **Cron firing before Lambda is deployed** during stack creation — CloudFormation orders dependencies; Schedules `DependsOn: BotFunction` if needed (probably auto from Arn ref). +- **Time-zone confusion** — cron expressions use UTC; verified in comments next to each expression. + +## Open questions +1. Direct invoke vs HTTPS — final decision lives here, not Phase 04 of AWS-port plan. +2. Add a third schedule for a manual "ad-hoc" endpoint (e.g. for testing without console)? YAGNI — `aws scheduler invoke-now` works. +3. Schedule `State: ENABLED` vs `DISABLED` initially? ENABLED — first deploy implicitly trusts the cron handlers; if either causes prod issues, disable via console immediately. diff --git a/plans/260510-0234-pre-deploy-wrapup/plan.md b/plans/260510-0234-pre-deploy-wrapup/plan.md new file mode 100644 index 0000000..3ed3106 --- /dev/null +++ b/plans/260510-0234-pre-deploy-wrapup/plan.md @@ -0,0 +1,66 @@ +--- +title: "Pre-deploy wrap-up: cron handlers + trading + cosmetics" +description: "Cloud-agnostic Go work + small SAM additions to land before Phase 01 AWS bootstrap. Outputs feed directly into AWS-port plan's Phase 04 + 06 + 07 verification." +status: in-progress +priority: P2 +effort: 8h +branch: main +tags: [aws, modules, lolschedule, trading, observability, readme] +created: 2026-05-10 +blockedBy: [] +blocks: [260510-0114-aws-port] +--- + +# Plan: Pre-deploy wrap-up + +Five focused phases that finish all **non-deploy** remaining work. Designed to land *before* the user runs Phase 01 of the AWS-port plan (manual AWS account + first `sam deploy`). After this plan ships, AWS-port Phases 04, 06, 07 become genuinely meaningful (real cron handlers, real metric data, accurate README at cutover). + +## Why these five + +From the punch-list: +1. **Cosmetics** — README still GCP-flavored; AWS-port plan statuses say "pending" for code that already shipped +2. **Metric filter** — small additive SAM change; trivial to land now +3. **lolschedule daily-push cron** — was deferred from GCP plan; without it Phase 04 schedules nothing real +4. **Trading module** — biggest pending Go chunk; cloud-agnostic; from old GCP Phase 08 +5. **EventBridge schedules** — wires the new cron handlers to AWS Scheduler + +## Phases + +| # | Phase | Status | Effort | Key deliverable | +|---|-------|--------|--------|-----------------| +| 01 | [Cosmetics: README + plan status sync](phase-01-cosmetics.md) | done | 30m | README rewritten for AWS default; AWS-port phases marked code-done | +| 02 | [Cold-start metric filter](phase-02-metric-filter.md) | done | 30m | `AWS::Logs::MetricFilter` for `Init Duration` in `template.yaml` | +| 03 | [lolschedule daily-push cron](phase-03-lolschedule-cron.md) | done | 3h | `Crons()` registered; Deps exposes bot for fan-out; daily push at 08:00 ICT | +| 04 | [Trading module port](phase-04-trading-module.md) | done (scope-trimmed: no daily refresh cron, no leaderboard — neither in upstream) | 4h | VN-stocks paper trading: topup/buy/sell/stats/convert; KBS price source | +| 05 | [Wire EventBridge schedules](phase-05-eventbridge-schedules.md) | **deferred to first-deploy decision** | 30m | `AWS::Scheduler::Schedule` resource for lolschedule cron — needs HTTPS-vs-direct-invoke call validated against live SAM CLI | + +## Dependency graph +``` +01 ──┐ (README + status — independent) +02 ──┤ (metric filter — independent) +03 ──┐ + ├──► 05 (schedules need real handlers from 03 + 04) +04 ──┘ +``` + +01 and 02 can ship in any order, including parallel. 05 blocks on 03 + 04 having registered crons. + +## Relation to other plans +- Builds on: `plans/260510-0114-aws-port/` (the offline artifacts already shipped) +- Carried-over from `plans/260508-2222-go-port-cloud-run/` Phase 08 (trading) — that phase is fulfilled by this plan's Phase 04 +- After this lands, AWS-port Phase 04 (EventBridge) and Phase 06 (metric capture) become verifiable on first deploy + +## Out of scope (explicit non-goals) +- AWS account creation / IAM OIDC / first `sam deploy` (= AWS-port Phase 01, user-manual) +- Telegram webhook flip (= AWS-port Phase 07, deploy-gated) +- 7-day soak observations (= AWS-port Phase 07) +- Provisioned concurrency, DynamoDB TTL, X-Ray dashboard customization (YAGNI for v1) + +## Abort criteria +- Phase 03 hits architectural friction extending `modules.Deps` to expose `*bot.Bot` cleanly → split into a smaller Deps refactor PR first, defer cron handler. +- Phase 04 trading API source unavailable / paywalled → stub the data layer, mark module disabled by default. + +## Open questions +1. Daily-push timezone: ICT 08:00 = UTC 01:00 — confirm cron expression matches in Phase 03. +2. Trading data source: original miti99bot uses VN stocks API — confirm it's still free + accessible in Phase 04. +3. Should Phase 01 (README) wait until trading module is done, so the README can advertise it? Tradeoff: ship docs sooner vs. ship complete picture. Default: ship now, update README when trading lands. diff --git a/plans/260510-0234-pre-deploy-wrapup/reports/code-reviewer-260510-0244-cron-and-trading.md b/plans/260510-0234-pre-deploy-wrapup/reports/code-reviewer-260510-0244-cron-and-trading.md new file mode 100644 index 0000000..becf364 --- /dev/null +++ b/plans/260510-0234-pre-deploy-wrapup/reports/code-reviewer-260510-0244-cron-and-trading.md @@ -0,0 +1,189 @@ +# Code review — lolschedule cron + trading module + framework changes + +Date: 2026-05-10 +Reviewer: code-reviewer (staff) +Scope: 1× framework change, 1× new cron, 1× new module (~30 files) +Verification: build clean, `go vet` clean, `go test -race` 24/24 green; **but `gofmt -l` reports 1 file dirty** (see B1). + +--- + +## Critical (blocks merge) + +### C1. `gofmt -l` fails on `internal/modules/trading/handlers.go` +Line 22-28 of `state` struct has misaligned field tags. `gofmt -d` proposes: +``` +- kv storage.KVStore +- prices *PriceClient +- locks keylock.Map +- nowFn func() time.Time ++ kv storage.KVStore ++ prices *PriceClient ++ locks keylock.Map ++ nowFn func() time.Time + commingSoonMessage string +``` +golangci-lint v2.12 enforces `gofmt`; CI will fail. Run `gofmt -w internal/modules/trading/handlers.go`. + +### C2. Sell-rollback can silently lose user shares +`internal/modules/trading/handlers.go:188-189`: +```go +p.AddAsset(symbol, qty) +_ = SavePortfolio(ctx, s.kv, userID, p) +``` +The deduction-rollback Save's error is dropped with `_ =`. If KBS is down (already true at this codepath) AND the rollback write also fails (transient DynamoDB throttle, ctx deadline near expiry), the user's prior `DeductAsset` is in-memory only, never reverted, and on the next Load they will see the previous (post-deduct) state. Net result: shares deleted, no VND credited. + +Fix: log + replace user-facing message when rollback fails so an op is alerted, e.g.: +```go +if err := SavePortfolio(ctx, s.kv, userID, p); err != nil { + log.Error("trading_sell_rollback_failed", "user", userID, "symbol", symbol, "qty", qty, "err", err) + return chathelper.Reply(ctx, b, chatID, "Sell failed and rollback errored — contact support before retrying.") +} +``` +Or — better — fetch the price BEFORE acquiring the lock (parallel to handleBuy's structure) so no rollback path is needed. handleSell currently fetches *under* the lock too, which also blocks the user's mutex on a 10-second HTTP call (see H1). + +--- + +## High + +### H1. handleSell holds the per-user lock across a 10s HTTP call +`handlers.go:174-185`: lock acquired → Load → DeductAsset → **FetchPrice (10s timeout)** → AddCurrency → Save. Concurrent operations on the same user serialise behind a transient HTTP call to KBS. handleBuy correctly fetches the price *before* the lock. Rewrite handleSell to mirror handleBuy's order: validate, FetchPrice, then acquire lock, then Load → check holdings → Deduct + AddCurrency → Save. Eliminates the lock-held HTTP call AND removes the rollback hazard from C2. + +### H2. lolschedule cron will exceed the 60s server timeout above ~1100 subscribers +`internal/server/timeouts.go:9`: `defaultCronTimeout = 60 * time.Second`. +`internal/modules/lolschedule/cron.go:31`: `telegramRateLimitDelay = 50 * time.Millisecond` when subs > 30. +At N subscribers, throttled inter-send delay alone is `(N-1) * 50ms`. SendMessage HTTP latency adds ~50–200ms each. Effective ceiling ≈ 600–800 subscribers before the cron context cancels mid-batch. The handler does check `ctx.Done()` (good — returns ctx.Err) but the run is then logged as a failure with no resume state; the next day's run starts from chat[0] again so early subscribers are over-served and tail subscribers are starved (not fair). + +Mitigations (pick one for v1; defer the rest): +- a. Cap N: refuse new subscribers above e.g. 800 (warn user). +- b. Shard schedule: emit one cron per 500-sub group with offset times. Requires Phase 05 EventBridge work. +- c. Async fan-out: cron enqueues N SQS messages, each consumer SendMessages → done in parallel. Best long-term but needs new IaC. + +For v1 with realistic JS-source subscriber counts (<100), this is probably fine; flag for monitoring after deploy. + +### H3. handleSell: silent rollback save error swallows user data loss +Already covered in C2 — also high-severity from the data-integrity angle. + +### H4. No `From.ID == 0` defense +The user's task description says "we explicitly refuse but verify" — code does NOT verify. `senderInfo` (handlers.go:50) only refuses `nil` From, not `From.ID == 0`. If Telegram (or a malicious local-dev fixture) ever produces a User with ID=0, all such users would key into `user:0` and share a portfolio. Telegram's spec says IDs are positive, so this is defense-in-depth, but trivial to add: +```go +if msg == nil || msg.From == nil || msg.From.ID == 0 { + return 0, 0, false +} +``` + +--- + +## Medium + +### M1. `Currency` is `map[string]float64` for VND — should be int64 +VND has no sub-unit; the smallest legal denomination is 1 VND. `float64` arithmetic on `cost := float64(qty) * price` and `Meta.Invested += amount` accumulates IEEE-754 drift. After a few hundred buys at non-round prices (24,500 × 137 = 3,356,500 — exact, ok; but 18,750 × 31 = 581,250 — also exact, but compounded sums of non-power-of-two integers eventually drift). At Vietnamese stock-trade volumes this is unlikely to materialise as user-visible cents, but flagging because: +- (a) JSON decode `float64` of a saved 24,500,000 then `× 137` could round-trip-shift if KV ever stores e.g. "1.5e7"; +- (b) `FormatVND` uses `math.Round` which masks drift in the UI but not in the stored ledger. + +Severity is medium (not high) because the upstream JS likely had the same issue and no incident has been reported. Recommend documenting the trade-off in `portfolio.go` or migrating to int64 in v2. + +### M2. No ticker length / alphabet validation +`symbols.go:30-35` accepts any non-empty `args[1]` after upper+trim. There's no length cap, no `[A-Z0-9]` enforcement, no defence against unicode lookalikes. While `url.PathEscape` makes the HTTP call safe and `ErrNoPrice` paths skip cache writes (so no KV pollution from invalid lookups), a user could still spam: +``` +/trade_buy 1 АAA (Cyrillic А, looks like ASCII A) +``` +which generates KBS HTTP calls + a partial DoS amplification through your Lambda. Add a regex check, e.g. `^[A-Z0-9]{1,16}$` after upper+trim, return `ErrUnknownTicker` for misses. Cheap, principled. + +### M3. Field typo: `commingSoonMessage` +`handlers.go:27, 118, 212` — should be `comingSoonMessage` (one m). User-invisible (it's a private field), but CI linters with spell-check rules flag this. Style grep'd consistently (3 occurrences); a single rename works. + +### M4. `chatIDString` in cron_test.go is dead/wrong code +`cron_test.go:35-37`: +```go +func chatIDString(id int64) string { + return time.Unix(id, 0).Format("00") // arbitrary stringification +} +``` +`Format("00")` returns the literal string `"00"` because "00" contains no Go time-format directives. So every chat error message is identical: `"fakeSender: induced failure for chat 00"`. Replace with `strconv.FormatInt(id, 10)` or just inline `fmt.Errorf("fakeSender: induced failure for chat %d", id)`. + +### M5. lolschedule daily-push has no retry / dead-chat unsubscribe +A subscriber who blocks the bot returns 403 from SendMessage. The cron logs `failed++` and never removes them. Over weeks, the failure count grows. Not a correctness issue but an operational drag. Consider, in a future PR, trimming subscribers whose SendMessage returns specific 403/400 error codes. + +### M6. `Phase 05 EventBridge schedule` deferred — daily-push is dead-on-deploy +Per `plan.md:35`, Phase 05 is deferred. Without the `AWS::Scheduler::Schedule`, the registered `lolschedule_daily_push` cron will never fire in production. This is intentional per the plan, but I'm flagging because the README / changelog should not advertise the daily-push feature until Phase 05 ships. Verify the README copy doesn't promise active push. + +--- + +## Low + +### L1. `runDailyPush`: throttle decision is binary on `len(subs) > 30` +At N=31, the cron suddenly serialises with 50ms delays. Telegram's 30/sec global limit is a target rate not a hard ceiling — 30 contiguous sends is fine. The threshold is conservative; not wrong, just unnecessarily slow at N=31..100. + +### L2. `handleStats` allocates a per-call `heldList` slice — tiny GC churn at scale, fine for v1. + +### L3. `prices.go:103` shadows builtin `close` +```go +close := body.DataDay[0].C +if close <= 0 { ... } +``` +`close` is a Go builtin (channel close). Shadowing is legal but lint-noisy. Rename to `c` or `lastClose`. + +### L4. Test `TestRunDailyPush_SendsToAllSubscribers` asserts ordering of `sender.calls` +Subscribers come back from `listSubscribers` in JSON-array order, which is the order they were added — *currently*. If the persistence layer ever switches to a set-like backend, the test breaks. Either lock the contract in `listSubscribers`'s godoc or sort before asserting in the test. + +### L5. README / template.yaml — `trading` enabled by default +`template.yaml:17` adds `trading` to ModulesCSV. Trading is a financial-looking command surface (paper or not). For a personal bot this is fine, but consider whether it should be opt-in via a `--with-trading` deploy flag in case future operators want to disable it without editing the template. v1: leave as-is. + +--- + +## Edge cases / scout findings + +- **handleStats** reads portfolio without keylock; safe because LoadPortfolio JSON-decodes a fresh struct each call (no shared map memory with concurrent buy/sell). **No race.** +- **`defer s.locks.Acquire(key)()` semantics** — verified correct: outer call evaluates immediately (acquires), Unlock is deferred. Both buy and sell hold the lock over the right region. +- **ResolveSymbol cache-write fallback** — `_ = kv.PutJSON(...)` on cache miss + successful KBS lookup is intentional and safe (next call will reresolve). Acceptable. +- **`from.ID` collision** — see H4. +- **KBS HTTP error semantics** — 4xx/5xx → ErrNoPrice (verified by test). Network errors → wrapped. JSON decode errors → wrapped. Negative close → ErrNoPrice. Empty data_day → ErrNoPrice. **All paths covered.** +- **Cron auth** — `subtle.ConstantTimeCompare` used (router.go:68). No constant-time bypass via header probing. Good. +- **Cron name regex** — `^[a-z0-9_]{1,32}$`, blocks log injection. `lolschedule_daily_push` matches. Good. +- **No PII / secret leak** — error messages to users are generic ("Could not load portfolio. Try again later."); KBS upstream URLs not echoed; SendMessage params not logged with chat content; no stack traces propagated. +- **Stats fan-out latency** — sequential per-ticker FetchPrice; for a portfolio of 50 tickers at 100ms KBS latency that's 5s of dead time before the user sees anything. Below the 60s ceiling but bad UX. Probably fine for v1 (typical user holds <10). +- **Integer overflow** — `int64` for `qty` and `Assets` map values; max 9.2e18, never reachable for stock counts. `float64` for VND has 53-bit mantissa (~9e15 = 9 quadrillion VND ≈ $360 billion); not reachable. + +--- + +## Positive observations + +- Lock granularity (per-user) is correct, not over-broad. Distinct users never block each other. +- handleBuy correctly fetches price *before* acquiring lock — minimises lock duration. +- Tests use `httptest.NewServer` everywhere; **no real KBS calls in `go test`**. Hermetic. +- Dependency injection via `messageSender` interface in cron.go is exemplary: enables real-bot test without mocking the full `*bot.Bot` API. +- `BuildOptions` extension pattern: future deps (Bot, Embedder, Chatter) are added without breaking the `Build` signature. Good API stability hygiene. +- Cache write failure on `ResolveSymbol` is correctly non-fatal (one-line comment explains why). +- `senderInfo` correctly refuses channel posts / inline queries to avoid `user:0` collision. +- Defensive nil-map repair in `LoadPortfolio` is correct defence-in-depth. +- Throttle implementation in cron is select-based on `ctx.Done` — cooperative cancellation is wired. +- 24/24 packages green with `-race`; CI integration looks healthy. + +--- + +## Recommended action order + +1. **Fix C1** (gofmt) — 10s, unblocks CI. +2. **Fix C2 + H1 together** by reordering handleSell to fetch price before lock (mirrors handleBuy). One change, two issues resolved. +3. **Add H4** (From.ID == 0 check) — 3 lines. +4. **Add M2** (ticker regex) — 5 lines + 1 test. +5. **Rename M3** (`commingSoonMessage` → `comingSoonMessage`) — global replace. +6. **Fix M4** (chatIDString dead code) — 2-line fix. +7. **Defer rest** (M1 float→int64, M5 dead-chat unsub, L-series) to a follow-up PR. + +After (1)–(6), the change is mergeable. (1)–(3) are mandatory before deploy. + +--- + +## Unresolved questions + +1. Is the upstream JS `trading` module also using float64 for VND? If yes, M1 is parity (acceptable v1) — if no, this is a regression worth fixing now. +2. What's the realistic peak `lolschedule` subscriber count? If <300 ever, H2 is non-blocking; if growth is plausible, the decision in H2 (a/b/c) needs choosing before Phase 05 EventBridge ships. +3. Should the handleSell rollback path also restore `Meta.Invested` symmetry? Currently Buy doesn't touch Invested and Sell doesn't either — Invested only moves on `trade_topup`. This makes "Invested" mean "total deposits", not "cost basis", which deviates from typical brokerage semantics. Confirm intent matches JS source. +4. Is `Phase 05 EventBridge` going to land before public release? If yes, the daily-push code is exercised on first deploy. If no, it's dead-but-tested code. Either is fine — just confirm. + +--- + +**Status:** DONE_WITH_CONCERNS +**Summary:** Code is well-structured, hermetic-tested, race-clean. Two real correctness issues (C1 gofmt blocker, C2 silent rollback save) and one architectural smell (H1 lock-held HTTP call) need fixing before deploy. Trading module is a credible peer of wordle/loldle in shape and discipline; lolschedule cron is testable and correctly authenticated. +**Concerns:** C1 will fail CI. C2 + H1 are data-integrity (low probability, but not negligible at production scale). H4 is defense-in-depth. M-tier are quality-of-life. Phase 05 EventBridge schedule is deferred-by-design — verify README doesn't over-promise active push.