From cf620a257e84de168621b0f6d27991ccb532dd1a Mon Sep 17 00:00:00 2001 From: tiennm99 Date: Wed, 30 Sep 2026 14:19:01 +0700 Subject: [PATCH] docs: fix stale code comments and add missing package docs Correct comments that described the retired webhook transport, a removed /cron route, the old KV store and wrapper types, and behaviour that has since changed; drop plan and review labels; reword two startup log lines that overstated or misnamed what they report. --- cmd/server/main.go | 23 ++++++++++---- internal/deploynotify/deploy_notify.go | 4 +-- internal/deploynotify/deploy_notify_test.go | 2 +- internal/keylock/keylock.go | 14 ++++----- internal/log/log.go | 6 ++-- internal/log/log_test.go | 2 +- internal/metrics/counters.go | 17 ++++++----- internal/modules/alias/alias.go | 6 ++-- internal/modules/alias/fallback_test.go | 2 +- internal/modules/alias/handlers.go | 9 +++--- internal/modules/alias/handlers_test.go | 8 ++--- internal/modules/blacklist/blacklist.go | 2 +- internal/modules/blacklist/handlers.go | 7 +++-- internal/modules/blacklist/handlers_test.go | 7 +++-- internal/modules/coin/coin.go | 9 ++++-- internal/modules/coin/format.go | 7 +++++ internal/modules/coin/portfolio.go | 23 ++++++++++++++ internal/modules/coin/price_providers.go | 8 +++++ internal/modules/coin/prices.go | 6 ++++ internal/modules/coin/symbols.go | 8 +++++ internal/modules/coin/trade_args.go | 3 ++ internal/modules/coin/views.go | 11 +++++-- .../modules/coin/views_reply_budget_test.go | 2 +- internal/modules/cron_dispatcher.go | 4 +-- internal/modules/gold/format.go | 7 +++++ internal/modules/gold/gold.go | 5 +++- internal/modules/gold/handlers.go | 6 ++-- internal/modules/gold/portfolio.go | 11 +++++++ internal/modules/gold/vnappmob_client.go | 4 +-- internal/modules/lol/api_client.go | 9 +++--- internal/modules/lol/api_client_test.go | 2 +- internal/modules/lol/cron.go | 16 +++++----- internal/modules/lol/format.go | 10 +++---- internal/modules/lol/format_test.go | 7 ++--- internal/modules/lol/parse_date.go | 4 +-- internal/modules/loldle/compare.go | 13 ++++---- internal/modules/loldle/handlers.go | 12 ++++---- internal/modules/loldle/lookup.go | 3 ++ internal/modules/loldle/state.go | 2 +- internal/modules/misc/misc.go | 22 +++++++------- internal/modules/module.go | 19 ++++++------ internal/modules/modules.go | 16 +++++----- internal/modules/monkeyd/handlers_test.go | 4 +-- internal/modules/monkeyd/monkeyd.go | 10 ++++--- internal/modules/monkeyd/tags_command.go | 7 +++-- internal/modules/registry.go | 30 +++++++++---------- internal/modules/registry_test.go | 7 +++-- internal/modules/stats/startup.go | 18 ++++++++++- internal/modules/stats/stats.go | 7 +++++ internal/modules/stats/stats_test.go | 9 +++--- internal/modules/stats/usage_store.go | 14 +++++++++ internal/modules/stats/views.go | 7 ++++- .../modules/sticker/addsticker_command.go | 3 +- .../sticker/addsticker_command_test.go | 2 +- .../modules/sticker/sticker_emoji_test.go | 9 +++--- .../modules/sticker/sticker_image_test.go | 2 +- internal/modules/sticker/sticker_pack.go | 8 ++--- internal/modules/sticker/sticker_video.go | 6 ++-- internal/modules/stock/dividend_events_ssi.go | 21 ++++++++----- .../modules/stock/dividend_notifications.go | 22 ++++++++++++++ internal/modules/stock/format.go | 2 -- internal/modules/stock/handlers.go | 17 ++++++----- internal/modules/stock/pending_dividend.go | 5 ++++ internal/modules/stock/portfolio.go | 20 +++++++++++++ internal/modules/stock/prices.go | 29 +++++++++++------- internal/modules/stock/prices_kbs.go | 2 ++ internal/modules/stock/prices_vci.go | 2 ++ internal/modules/stock/stock.go | 14 ++++++++- internal/modules/stock/symbols.go | 5 ++-- .../modules/util/chathelper/chathelper.go | 28 +++++++++-------- internal/modules/util/info.go | 2 +- internal/modules/util/util.go | 4 +-- internal/modules/wordle/compare.go | 10 +++---- internal/modules/wordle/handlers_test.go | 2 +- internal/modules/wordle/lookup.go | 5 ++-- internal/modules/wordle/lookup_test.go | 2 +- internal/modules/wordle/pick_random.go | 8 ++--- internal/modules/wordle/state.go | 4 +-- internal/modules/wordle/words.go | 4 +-- internal/server/log_middleware.go | 19 ++++++------ internal/server/router.go | 3 ++ internal/storage/doc_store.go | 9 ++++-- internal/systemstate/systemstate.go | 9 ++++++ internal/telegram/client.go | 12 +++++--- internal/telegram/webhook_test.go | 2 +- internal/testutil/recording_bot.go | 15 ++-------- internal/testutil/update_builders.go | 4 +-- 87 files changed, 515 insertions(+), 267 deletions(-) diff --git a/cmd/server/main.go b/cmd/server/main.go index 86e37b8..cba4e56 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -1,3 +1,7 @@ +// Command server runs miti99bot: it loads configuration from the environment, +// opens the storage backend, builds the module registry, and then serves +// Telegram updates by long polling while an in-process scheduler fires module +// crons. A small HTTP server answers the container health check. package main import ( @@ -43,6 +47,8 @@ import ( // (resolveCommitSHA prefers it). var gitSHA = buildCommitSHA() +// buildCommitSHA returns the short vcs.revision embedded by go build, or "" +// when the binary carries no VCS metadata. func buildCommitSHA() string { info, ok := debug.ReadBuildInfo() if !ok { @@ -176,7 +182,7 @@ func main() { defer stopCron() if cfg.BotOwnerID == 0 { - log.Warn("OWNER_ID unset; all Private + Protected commands will be denied") + log.Warn("OWNER_ID unset; Private (owner-only) commands will be denied; Protected commands still work for ADMIN_IDS") } // Clear any existing webhook at startup before the owner DM and before @@ -202,7 +208,7 @@ func main() { Handler: handler, ReadHeaderTimeout: 10 * time.Second, ReadTimeout: 30 * time.Second, - // The only route is GET / (health). It responds instantly, so a tight + // The only route is GET / (health). It responds instantly, so this // write deadline is ample and bounds any slow-loris write. WriteTimeout: 30 * time.Second, IdleTimeout: 120 * time.Second, @@ -296,7 +302,7 @@ func buildProvider(ctx context.Context, cfg config) (storage.Provider, func(), e switch backend { case "memory": - log.Warn("KV backend: in-memory (data lost on restart)") + log.Warn("storage backend: in-memory (data lost on restart)") return storage.NewMemoryProvider(), func() {}, nil case "mongodb": @@ -331,6 +337,7 @@ func buildProvider(ctx context.Context, cfg config) (storage.Provider, func(), e } } +// config is the process configuration read from the environment by loadConfig. type config struct { Port string TelegramBotToken string @@ -343,6 +350,9 @@ type config struct { MongoDatabase string // required when KVProvider=mongodb } +// loadConfig reads config from the environment. PORT defaults to 8080 and an +// invalid PORT is fatal; malformed OWNER_ID / ADMIN_IDS entries are logged and +// ignored. func loadConfig() config { envMap := make(map[string]string, len(os.Environ())) for _, kv := range os.Environ() { @@ -354,9 +364,9 @@ func loadConfig() config { if port == "" { port = "8080" } - // PORT must be numeric — http.Server constructs ":" verbatim, so a - // junk value would surface only at ListenAndServe time. Fail fast here - // instead. Range check is delegated to http.Server (it handles 0/65535). + // PORT must be a number in 0..65535. http.Server uses ":" verbatim, + // so a junk value would otherwise surface only at ListenAndServe time; + // fail fast here instead. if n, err := strconv.Atoi(port); err != nil || n < 0 || n > 65535 { log.Fatal("invalid PORT", "value", port) } @@ -373,6 +383,7 @@ func loadConfig() config { } } +// splitCSV splits a comma-separated list, trimming entries and dropping empty ones. func splitCSV(s string) []string { if s == "" { return nil diff --git a/internal/deploynotify/deploy_notify.go b/internal/deploynotify/deploy_notify.go index 5889a10..6919fd1 100644 --- a/internal/deploynotify/deploy_notify.go +++ b/internal/deploynotify/deploy_notify.go @@ -68,8 +68,8 @@ func skipReason(cfg Config) string { return "" } -// renderMessage is exposed for tests; keep the format stable enough that the -// owner can grep their Telegram history by SHA. +// renderMessage builds the DM text. Keep the format stable so the owner can +// search their Telegram history by SHA. func renderMessage(sha string) string { return fmt.Sprintf("🚀 miti99bot deployed: %s", sha) } diff --git a/internal/deploynotify/deploy_notify_test.go b/internal/deploynotify/deploy_notify_test.go index 8f829c1..78b6dd4 100644 --- a/internal/deploynotify/deploy_notify_test.go +++ b/internal/deploynotify/deploy_notify_test.go @@ -42,7 +42,7 @@ func TestRun_SendsOnStartup(t *testing.T) { } func TestRun_SendsEveryStartupNoDedup(t *testing.T) { - // Unlike the old dedup behaviour, the same SHA must notify on every boot. + // No dedup: the same SHA must notify on every boot. for i := 0; i < 2; i++ { rec := &recorder{} Run(context.Background(), Config{OwnerID: 42, GitSHA: "abc123", Sender: rec.send}) diff --git a/internal/keylock/keylock.go b/internal/keylock/keylock.go index 6a0f981..203295c 100644 --- a/internal/keylock/keylock.go +++ b/internal/keylock/keylock.go @@ -1,11 +1,11 @@ // Package keylock serialises compound operations that target the same key // (typically a chat / user / subject identifier) across goroutines. // -// Why a separate package: every game module needs a per-subject mutex to -// turn the store's single-op atomicity into safe Get→mutate→Put. The bot -// dispatcher runs each Telegram update in its own goroutine, so without -// explicit per-subject serialisation two updates to the same game could -// race and drop one write. +// Why a separate package: several modules need a per-subject mutex to turn +// the store's single-op atomicity into safe Get→mutate→Put. Telegram updates +// are handled one at a time, but crons fire on scheduler goroutines alongside +// them, so without explicit per-subject serialisation a cron and a handler +// writing the same subject could race and drop a write. // // Trade-off: the underlying sync.Map grows unboundedly with distinct keys // (~32 B each). At the current bot scale, that is acceptable; add eviction if @@ -23,8 +23,8 @@ type Map struct { // Acquire locks the per-key mutex and returns its Unlock as a func so the // caller can `defer m.Acquire(key)()` at the top of a critical section. // -// Distinct keys never block each other; same-key callers run serially in the -// order Acquire was called. +// Distinct keys never block each other; same-key callers run one at a time. +// Like sync.Mutex, it does not guarantee FIFO order among waiters. func (m *Map) Acquire(key string) func() { v, _ := m.m.LoadOrStore(key, &sync.Mutex{}) mu := v.(*sync.Mutex) diff --git a/internal/log/log.go b/internal/log/log.go index 2fb1d89..197c336 100644 --- a/internal/log/log.go +++ b/internal/log/log.go @@ -10,11 +10,11 @@ // Usage: // // log.Info("server starting", "port", 8080) -// log.Error("kv write failed", "module", "misc", "command", "ping", "err", err) +// log.Error("store write failed", "module", "misc", "command", "ping", "err", err) // log.Fatal("missing required env", "key", "TELEGRAM_BOT_TOKEN") // -// slog escapes newlines and quotes in field values, which closes the -// log-injection class (J3 in the 2026-05-09 review). +// slog escapes newlines and quotes in field values, so user-controlled text in +// a field cannot forge an extra log record. package log import ( diff --git a/internal/log/log_test.go b/internal/log/log_test.go index 0af99c4..29c8339 100644 --- a/internal/log/log_test.go +++ b/internal/log/log_test.go @@ -87,7 +87,7 @@ func TestError_AttachesErrField(t *testing.T) { } func TestNewlineEscaping_NoLogInjection(t *testing.T) { - // Closes J3 (log-injection class) — slog must escape \n inside field + // Log-injection guard — slog must escape \n inside field // values so an attacker controlled string can't synthesise a fake log // record on the next line. buf, restore := captureLogger(t, slog.LevelInfo) diff --git a/internal/metrics/counters.go b/internal/metrics/counters.go index 2d97cfc..c26c63f 100644 --- a/internal/metrics/counters.go +++ b/internal/metrics/counters.go @@ -51,14 +51,15 @@ var Default = New() func (r *Registry) IncCommand(name string) { r.inc(r.commandsMap(), name) } // IncError bumps the counter for an error category — small, stable kinds -// like "ai-429", "kv-unavailable", "telegram-403". +// like "handler-error" or "handler-panic". func (r *Registry) IncError(kind string) { r.inc(r.errorsMap(), kind) } func (r *Registry) commandsMap() map[string]*atomic.Int64 { return r.commands } func (r *Registry) errorsMap() map[string]*atomic.Int64 { return r.errors } -// inc bumps the counter for name in m, allocating on first use. Allocates -// only when the name is new, so steady-state increments are mutex-free. +// inc bumps the counter for name in m, allocating on first use. Only a new +// name takes the write lock; steady-state increments share the read lock and +// bump the atomic. func (r *Registry) inc(m map[string]*atomic.Int64, name string) { r.mu.RLock() c, ok := m[name] @@ -116,7 +117,7 @@ func drain(m map[string]*atomic.Int64) map[string]int64 { // // {"msg":"metrics","commands":{"wordle":3,"loldle":1},"errors":{"handler-error":1}} // -// CloudWatch Logs filters on `jsonPayload.msg=metrics` for dashboards. +// Keep msg=metrics stable; log-based dashboards filter on it. // Empty categories appear as null (slog's default for nil maps). func (r *Registry) Flush() { cmds, errs := r.snapshot() @@ -130,10 +131,10 @@ func (r *Registry) Flush() { log.Info("metrics", "commands", cmds, "errors", errs) } -// Run starts a goroutine that flushes counters every DefaultFlushInterval -// until ctx is cancelled. It does one final Flush on exit so a SIGTERM -// shutdown captures the trailing window. Returns immediately; the -// goroutine runs in the background. +// Run flushes counters every DefaultFlushInterval until ctx is cancelled, +// then does one final Flush so a SIGTERM shutdown captures the trailing +// window. It blocks until ctx is done, so callers run it in its own +// goroutine. // // Idiomatic usage: // diff --git a/internal/modules/alias/alias.go b/internal/modules/alias/alias.go index 33c7c3d..3936acd 100644 --- a/internal/modules/alias/alias.go +++ b/internal/modules/alias/alias.go @@ -1,5 +1,7 @@ -// Package alias implements /alias and /insert: a shared, bot-wide dictionary -// mapping a short name to any Telegram message the bot has seen. +// Package alias implements /alias, /insert, /aliases and /unalias: a shared, +// bot-wide dictionary mapping a short name to any Telegram message the bot has +// seen. A saved name is also invocable directly as /name (through the command +// fallback) and from inline mode as "@botname ". // // The namespace is global on purpose — a name assigned in any chat works in // every chat, for everyone, the same way the sticker pack /addsticker writes to diff --git a/internal/modules/alias/fallback_test.go b/internal/modules/alias/fallback_test.go index 064c1c9..42c45f4 100644 --- a/internal/modules/alias/fallback_test.go +++ b/internal/modules/alias/fallback_test.go @@ -13,7 +13,7 @@ import ( "github.com/tiennm99/miti99bot/internal/testutil" ) -// The headline of path B: a saved name becomes its own command. +// The headline of the fallback: a saved name becomes its own command. func TestFallback_SavedNameWorksAsItsOwnCommand(t *testing.T) { rb := installAlias(t) rb.Bot.ProcessUpdate(context.Background(), diff --git a/internal/modules/alias/handlers.go b/internal/modules/alias/handlers.go index 5e9267e..262bb4a 100644 --- a/internal/modules/alias/handlers.go +++ b/internal/modules/alias/handlers.go @@ -18,9 +18,9 @@ import ( ) const ( - // handlerTimeout bounds both handlers. The bot dispatches updates inline on - // a single worker with no deadline of its own, so without this the - // library's 60s per-call HTTP ceiling is the only bound. + // handlerTimeout bounds every command handler and the fallback. The bot + // dispatches updates inline on a single worker with no deadline of its own, + // so without this the library's 60s per-call HTTP ceiling is the only bound. handlerTimeout = 10 * time.Second // maxNameLen matches Telegram's own username cap, which is the format these @@ -51,7 +51,8 @@ const genericFailure = "Something went wrong. Try again in a moment." // errUnknownKind marks a stored record this build cannot send. var errUnknownKind = errors.New("alias: unknown kind") -// parseName validates the single argument both commands take. +// parseName validates the single name argument /alias, /insert and /unalias +// take, and the bare command name the fallback resolves. // // A leading "@" is stripped rather than rejected: the names are username-shaped // and typing one with the sigil is a natural mistake, not a different request. diff --git a/internal/modules/alias/handlers_test.go b/internal/modules/alias/handlers_test.go index fc5153d..32dc8e3 100644 --- a/internal/modules/alias/handlers_test.go +++ b/internal/modules/alias/handlers_test.go @@ -14,8 +14,8 @@ import ( "github.com/tiennm99/miti99bot/internal/testutil" ) -// installAlias builds a registry holding only the alias module. Both commands -// are public, so no auth is needed for them to dispatch. +// installAlias builds a registry holding only the alias module. Every command +// it registers is public, so no auth is needed for them to dispatch. func installAlias(t *testing.T) *testutil.RecordingBot { t.Helper() rb := testutil.NewRecordingBot(t) @@ -173,8 +173,8 @@ func TestAlias_AnimationBeatsDocument(t *testing.T) { } } -// Global namespace, last assignment wins — and the reply says so, since there -// is no /unalias to undo a mistake with. +// Global namespace, last assignment wins — and the reply says so, because an +// overwrite silently discards whatever the name held before. func TestAlias_OverwriteAnnouncesReplacement(t *testing.T) { rb := installAlias(t) rb.Bot.ProcessUpdate(context.Background(), diff --git a/internal/modules/blacklist/blacklist.go b/internal/modules/blacklist/blacklist.go index 5e4c1b9..a32ab2b 100644 --- a/internal/modules/blacklist/blacklist.go +++ b/internal/modules/blacklist/blacklist.go @@ -3,7 +3,7 @@ // // The module is passive on purpose. It never reads ordinary chat messages and // never deletes, warns or restricts anyone: the lists are inert until -// /blacklist_check asks about a specific text. Enforcement would need a +// /blacklist_check (or /blacklist with an argument) asks about a specific text. Enforcement would need a // message-level hook the dispatcher does not have, privacy mode disabled in // BotFather, and group-admin delete rights — all deliberately out of scope. // Check is a pure function, so a future hook could call it unchanged. diff --git a/internal/modules/blacklist/handlers.go b/internal/modules/blacklist/handlers.go index f82770f..209a423 100644 --- a/internal/modules/blacklist/handlers.go +++ b/internal/modules/blacklist/handlers.go @@ -26,9 +26,10 @@ const ( // only touch storage, so the budget is generous. handlerTimeout = 10 * time.Second - // maxListBytes keeps /blacklist_rules inside Telegram's 4096-character - // sendMessage limit, with room for the second heading and a trim notice - // after the budget is spent. + // maxListBytes keeps every list reply — /blacklist_rules, a bare + // /blacklist, and the listing appended to each add and remove — inside + // Telegram's 4096-character sendMessage limit, with room for the second + // heading and a trim notice after the budget is spent. // // The budget counts the markup, not only the entries: Telegram // measures the message it is sent, and at 13 bytes a pair the tags outweigh diff --git a/internal/modules/blacklist/handlers_test.go b/internal/modules/blacklist/handlers_test.go index 66759c4..0746e6d 100644 --- a/internal/modules/blacklist/handlers_test.go +++ b/internal/modules/blacklist/handlers_test.go @@ -290,8 +290,8 @@ func TestCheck_DiacriticsAreSignificant(t *testing.T) { } } -// An entry containing the characters that cannot appear literally in a storage -// key must survive the round trip through the store. +// An entry containing '/' (which cannot appear literally in a storage key) and +// '%' (the escape marker) must survive the round trip through the store. func TestEntry_WithKeyHazardsRoundTrips(t *testing.T) { rb := installBlacklist(t) const hazard = "50%2F/off" @@ -309,7 +309,8 @@ func TestEntry_WithKeyHazardsRoundTrips(t *testing.T) { } // Replies use parse_mode HTML, so every site that echoes user text must escape -// it. These pin the three sites outside /blacklist_rules. +// it. These pin the four add and remove confirmations: added, already present, +// removed, and not there. func TestAddAndDel_EscapeUserText(t *testing.T) { rb := installBlacklist(t) diff --git a/internal/modules/coin/coin.go b/internal/modules/coin/coin.go index 9224c4e..fa3e547 100644 --- a/internal/modules/coin/coin.go +++ b/internal/modules/coin/coin.go @@ -1,3 +1,7 @@ +// Package coin is a crypto paper-trading module: users top up a virtual USD +// balance, buy and sell coins at live prices, and review a portfolio with +// unrealized and account P&L. Prices come from Binance, then Coinbase, then +// CoinGecko, with a short in-process cache (see PriceClient). package coin import ( @@ -5,8 +9,9 @@ import ( "github.com/tiennm99/miti99bot/internal/storage" ) -// New is the coin paper-trading module factory. It is opt-in through MODULES -// and keeps its portfolio state separate from stock and gold modules. +// New is the coin paper-trading module factory. Like every module it is +// selected through MODULES, and it keeps its portfolio state separate from the +// stock and gold modules. func New(deps modules.Deps) modules.Module { s := newState(storage.Typed[Portfolio](deps.Store)) return modules.Module{ diff --git a/internal/modules/coin/format.go b/internal/modules/coin/format.go index 4a0c689..bd804ae 100644 --- a/internal/modules/coin/format.go +++ b/internal/modules/coin/format.go @@ -6,6 +6,8 @@ import ( "strings" ) +// FormatUSD renders n as "$1,234.56" (a leading "-" for negatives), or +// "invalid USD" for NaN and infinities. func FormatUSD(n float64) string { if math.IsNaN(n) || math.IsInf(n, 0) { return "invalid USD" @@ -59,6 +61,8 @@ func formatCompactUSD(n float64) string { return sign + amount + suffixes[suffixIndex] } +// FormatCoinQty renders a coin quantity with up to eight decimals and no +// trailing zeros. func FormatCoinQty(n float64) string { s := strconv.FormatFloat(n, 'f', 8, 64) s = strings.TrimRight(s, "0") @@ -69,6 +73,9 @@ func FormatCoinQty(n float64) string { return s } +// FormatPnLUSD renders the gain of currentValue over invested as a signed +// amount plus percentage, e.g. "+$12.00 (+1.20%)". The percentage is 0 when +// nothing was invested. func FormatPnLUSD(currentValue, invested float64) string { return formatPnLUSD(currentValue, invested, FormatUSD) } diff --git a/internal/modules/coin/portfolio.go b/internal/modules/coin/portfolio.go index c828a53..7cefb11 100644 --- a/internal/modules/coin/portfolio.go +++ b/internal/modules/coin/portfolio.go @@ -10,23 +10,35 @@ import ( "github.com/tiennm99/miti99bot/internal/storage" ) +// coinDustEpsilon is the tolerance below which balances and quantities are +// treated as zero, absorbing float rounding left over by trades. const coinDustEpsilon = 1e-9 + +// portfolioUpdateAttempts bounds optimistic-write retries in UpdatePortfolio. const portfolioUpdateAttempts = 5 + +// CollectionName is the module name and store collection for coin portfolios. const CollectionName = "coin" +// Store is the coin module's typed portfolio store. type Store = storage.DocStore[Portfolio] +// AssetPosition is one held coin: Quantity in coin units and Base, the USD +// cost basis still attributed to that quantity. type AssetPosition struct { Quantity float64 `json:"quantity" bson:"quantity"` Base float64 `json:"base" bson:"base"` } +// Portfolio is one user's coin account, keyed by upper-case ticker in Assets. type Portfolio struct { USD float64 `json:"usd" bson:"usd"` Assets map[string]AssetPosition `json:"assets" bson:"assets"` Meta PortfolioMeta `json:"meta" bson:"meta"` } +// PortfolioMeta holds account-level totals: Invested is the sum of all USD +// top-ups and CreatedAt is ms since epoch. type PortfolioMeta struct { Invested float64 `json:"invested" bson:"invested"` CreatedAt int64 `json:"createdAt" bson:"createdAt"` @@ -57,6 +69,9 @@ func SavePortfolio(ctx context.Context, store Store, userID int64, p Portfolio) return nil } +// UpdatePortfolio loads the user's portfolio, applies mutate, validates it, and +// writes it back with a versioned put, retrying on write conflicts. An error +// returned by mutate aborts without saving and is passed through unwrapped. func UpdatePortfolio(ctx context.Context, store Store, userID int64, now int64, mutate func(*Portfolio) error) (Portfolio, error) { key := portfolioKey(userID) for attempt := 0; attempt < portfolioUpdateAttempts; attempt++ { @@ -101,6 +116,8 @@ func loadPortfolioForUpdate(ctx context.Context, store Store, key string, now in } } +// Validate rejects a negative or non-finite USD balance and any position whose +// key is not a canonical ticker or whose quantity or basis is not positive. func (p Portfolio) Validate() error { if math.IsNaN(p.USD) || math.IsInf(p.USD, 0) || p.USD < 0 { return fmt.Errorf("coin: invalid USD balance") @@ -120,6 +137,9 @@ func (p *Portfolio) AddUSD(amount float64) { p.normalize() } +// DeductUSD withdraws amount when the balance covers it within dust +// tolerance. It reports whether it did, plus the resulting (or unchanged) +// balance. func (p *Portfolio) DeductUSD(amount float64) (ok bool, balance float64) { p.normalize() if p.USD+coinDustEpsilon < amount { @@ -146,6 +166,9 @@ func (p *Portfolio) BuyTicker(symbol string, quantity, base float64) error { return nil } +// SellTicker removes quantity of symbol and the proportional share of its cost +// basis, returned as soldBase. Selling the whole position (within dust +// tolerance) deletes it. ok is false when the position cannot cover quantity. func (p *Portfolio) SellTicker(symbol string, quantity float64) (remaining, soldBase float64, ok bool, err error) { position, exists := p.Assets[symbol] if !exists || !isPositiveFinite(quantity) || position.Quantity+coinDustEpsilon < quantity { diff --git a/internal/modules/coin/price_providers.go b/internal/modules/coin/price_providers.go index 685bd1d..fc97b3d 100644 --- a/internal/modules/coin/price_providers.go +++ b/internal/modules/coin/price_providers.go @@ -13,16 +13,22 @@ import ( var errProviderRateLimited = errors.New("coin: provider rate limited") +// BinanceProvider quotes the coin's USDT pair, then its USD pair. URL +// overrides the public market-data endpoint for tests. type BinanceProvider struct { HTTP *http.Client URL string } +// CoinbaseProvider reads the coin's USD rate from Coinbase exchange rates. type CoinbaseProvider struct { HTTP *http.Client URL string } +// CoinGeckoProvider looks up the coin by its known CoinGecko ID or, failing +// that, by the lower-case ticker and then the best market-cap match from +// CoinGecko search. type CoinGeckoProvider struct { HTTP *http.Client URL string @@ -55,6 +61,8 @@ type coinGeckoSearchCoin struct { MarketCapRank *int `json:"market_cap_rank"` } +// FetchUSD stops at the first rate-limit response instead of trying the USD +// pair, so the caller can move on to the next provider. func (p *BinanceProvider) FetchUSD(ctx context.Context, coin CoinSymbol) (CoinPrice, error) { for _, quote := range []string{"USDT", "USD"} { price, err := p.fetchPair(ctx, coin.Symbol, quote) diff --git a/internal/modules/coin/prices.go b/internal/modules/coin/prices.go index 1ad8711..f0dc11b 100644 --- a/internal/modules/coin/prices.go +++ b/internal/modules/coin/prices.go @@ -21,8 +21,10 @@ const ( coinPriceCacheTTL = 30 * time.Second ) +// ErrNoCoinPrice reports that no provider returned a usable price. var ErrNoCoinPrice = errors.New("coin: no price available") +// CoinPrice is a USD quote for Symbol and the provider name it came from. type CoinPrice struct { Symbol string USD float64 @@ -33,6 +35,8 @@ type PriceProvider interface { FetchUSD(ctx context.Context, coin CoinSymbol) (CoinPrice, error) } +// PriceClient tries each provider in order and returns the first positive +// quote, caching it per symbol for CacheTTL (no caching when CacheTTL <= 0). type PriceClient struct { Providers []PriceProvider CacheTTL time.Duration @@ -47,6 +51,8 @@ type cachedPrice struct { expiry time.Time } +// NewPriceClient builds the production client: Binance, then Coinbase, then +// CoinGecko, sharing one HTTP client and a 30-second cache. func NewPriceClient() *PriceClient { httpClient := &http.Client{Timeout: coinHTTPTimeout} return &PriceClient{ diff --git a/internal/modules/coin/symbols.go b/internal/modules/coin/symbols.go index 02f0298..1c4d83d 100644 --- a/internal/modules/coin/symbols.go +++ b/internal/modules/coin/symbols.go @@ -6,8 +6,11 @@ import ( "unicode" ) +// ErrUnsupportedCoin reports a ticker that fails validation. var ErrUnsupportedCoin = errors.New("coin: unsupported coin") +// CoinSymbol is a validated upper-case ticker plus its CoinGecko ID when the +// ticker is in knownCoinGeckoIDs. type CoinSymbol struct { Symbol string CoinGeckoID string @@ -15,6 +18,8 @@ type CoinSymbol struct { const maxCoinSymbolLength = 20 +// knownCoinGeckoIDs pins major tickers to their CoinGecko IDs, since +// CoinGecko IDs are names ("bitcoin"), not tickers. var knownCoinGeckoIDs = map[string]string{ "BTC": "bitcoin", "ETH": "ethereum", @@ -26,6 +31,9 @@ var knownCoinGeckoIDs = map[string]string{ "TON": "the-open-network", } +// ResolveCoinSymbol normalizes input to an upper-case ticker of 1-20 ASCII +// letters and digits with at least one letter. It does not check that the +// coin exists; providers decide that. func ResolveCoinSymbol(input string) (CoinSymbol, error) { symbol := strings.ToUpper(strings.TrimSpace(input)) if !validCoinSymbol(symbol) { diff --git a/internal/modules/coin/trade_args.go b/internal/modules/coin/trade_args.go index 2669be3..c975682 100644 --- a/internal/modules/coin/trade_args.go +++ b/internal/modules/coin/trade_args.go @@ -9,6 +9,9 @@ type coinValueArgs struct { value float64 } +// parseCoinValueArgs accepts " " or " ". When +// neither order parses, it returns ErrUnsupportedCoin if the second argument +// is a number (the coin was the bad part) and invalidValueErr otherwise. func parseCoinValueArgs(args []string, validValue func(float64) bool, invalidValueErr error) (coinValueArgs, error) { if len(args) != 2 { return coinValueArgs{}, invalidValueErr diff --git a/internal/modules/coin/views.go b/internal/modules/coin/views.go index 5ad8a0f..e2adddb 100644 --- a/internal/modules/coin/views.go +++ b/internal/modules/coin/views.go @@ -29,9 +29,9 @@ func (s *state) handleStats(ctx context.Context, b *bot.Bot, update *models.Upda // Fetch sequentially (not concurrently) so the price client's keep-alive // connection pool is reused across coins rather than opening N simultaneous - // TLS handshakes. The reply-reserved sub-context bounds the whole loop so the final - // Reply keeps its budget; a slow/failed provider degrades to "(price - // unavailable)" instead of failing the summary. + // TLS handshakes. The reply-reserved sub-context bounds the whole loop so the + // final reply keeps its budget; a slow or failed provider degrades that row + // to "N/A" and the summary to partial totals instead of failing the reply. fetchCtx, cancel := chathelper.FetchContext(ctx) defer cancel() for _, symbol := range sortedAssetSymbols(p.Assets) { @@ -92,8 +92,13 @@ func sortedAssetSymbols(assets map[string]AssetPosition) []string { return symbols } +// portfolioReplyLimit keeps the rendered reply under Telegram's 4096-character +// message limit with room to spare. const portfolioReplyLimit = 4000 +// portfolioTableReply renders the position and summary tables, dropping +// positions from the end (and noting how many) until the reply fits +// portfolioReplyLimit. func portfolioTableReply(title string, positions, summary [][]string) string { omitted := 0 for { diff --git a/internal/modules/coin/views_reply_budget_test.go b/internal/modules/coin/views_reply_budget_test.go index 20d253a..6cbfa01 100644 --- a/internal/modules/coin/views_reply_budget_test.go +++ b/internal/modules/coin/views_reply_budget_test.go @@ -22,7 +22,7 @@ func (blockingPriceFetcher) FetchUSD(ctx context.Context, _ CoinSymbol) (CoinPri // TestHandleStatsDeliversReplyWhenUpstreamHangs proves the reply-reserve fix: // even when the price upstream hangs for the entire fetch budget, handleStats -// still delivers a summary (with a "price unavailable" line) on the original +// still delivers a summary (with "N/A" price cells) on the original // context instead of failing the whole reply with "context deadline exceeded". func TestHandleStatsDeliversReplyWhenUpstreamHangs(t *testing.T) { // Seed a holding using a fast fetcher, then swap in the hanging upstream. diff --git a/internal/modules/cron_dispatcher.go b/internal/modules/cron_dispatcher.go index 7cef0b0..a4285be 100644 --- a/internal/modules/cron_dispatcher.go +++ b/internal/modules/cron_dispatcher.go @@ -9,8 +9,8 @@ import ( // ErrCronNotFound is returned when name addresses an unregistered cron. var ErrCronNotFound = errors.New("cron not found") -// DispatchScheduled runs the cron registered under name with the per-module -// prefixed Deps the registry stored at Build time. Returns ErrCronNotFound if +// DispatchScheduled runs the cron registered under name with the owning +// module's Deps the registry stored at Build time. Returns ErrCronNotFound if // no module owns that name — a scheduler firing an unknown name is a // configuration bug worth surfacing to the caller. // diff --git a/internal/modules/gold/format.go b/internal/modules/gold/format.go index 3976811..4bf6d86 100644 --- a/internal/modules/gold/format.go +++ b/internal/modules/gold/format.go @@ -6,6 +6,9 @@ import ( "strings" ) +// FormatVND rounds n to whole dong and renders it with dot thousands +// separators, e.g. "1.234.567 VND", or "invalid VND" when n is not finite or +// does not fit an int64. func FormatVND(n float64) string { if math.IsNaN(n) || math.IsInf(n, 0) || n > float64(math.MaxInt64) || n < float64(math.MinInt64) { return "invalid VND" @@ -26,6 +29,8 @@ func FormatVND(n float64) string { return sb.String() } +// FormatLuong renders a lượng quantity with up to four decimals and no +// trailing zeros. func FormatLuong(n float64) string { s := strconv.FormatFloat(n, 'f', 4, 64) s = strings.TrimRight(s, "0") @@ -36,6 +41,8 @@ func FormatLuong(n float64) string { return s } +// FormatPnL renders the gain of currentValue over invested as a signed VND +// amount plus percentage. The percentage is 0 when nothing was invested. func FormatPnL(currentValue, invested float64) string { diff := currentValue - invested pct := 0.0 diff --git a/internal/modules/gold/gold.go b/internal/modules/gold/gold.go index 11cef46..be45ff6 100644 --- a/internal/modules/gold/gold.go +++ b/internal/modules/gold/gold.go @@ -1,3 +1,6 @@ +// Package gold is an SJC gold paper-trading module: users top up a virtual VND +// balance and buy or sell gold by the lượng at live SJC quotes from VNAppMob, +// buying at the SJC sell price and selling at the SJC buy price. package gold import ( @@ -5,7 +8,7 @@ import ( ) // New is the gold paper-trading module factory. It keeps its portfolio state -// separate from the stock module. +// separate from the stock and coin modules. func New(deps modules.Deps) modules.Module { s := newState(deps.Store) return modules.Module{ diff --git a/internal/modules/gold/handlers.go b/internal/modules/gold/handlers.go index 3f3de94..2208adf 100644 --- a/internal/modules/gold/handlers.go +++ b/internal/modules/gold/handlers.go @@ -23,9 +23,9 @@ func (s *state) handlePrice(ctx context.Context, b *bot.Bot, update *models.Upda if len(args) != 0 { return chathelper.Reply(ctx, b, update.Message, "Usage: /gold_price") } - // Fetch under a reply-reserved sub-context (the composite fetcher may try - // providers sequentially); reply on the original ctx so delivery keeps its - // budget headroom. + // Fetch under a reply-reserved sub-context (VNAppMob may refresh its API + // key and retry, costing several round trips); reply on the original ctx so + // delivery keeps its budget headroom. fetchCtx, cancel := chathelper.FetchContext(ctx) defer cancel() p, err := s.prices.FetchPrice(fetchCtx) diff --git a/internal/modules/gold/portfolio.go b/internal/modules/gold/portfolio.go index d3c9b83..42baaaf 100644 --- a/internal/modules/gold/portfolio.go +++ b/internal/modules/gold/portfolio.go @@ -10,18 +10,26 @@ import ( "github.com/tiennm99/miti99bot/internal/storage" ) +// goldDustEpsilon is the tolerance below which balances and quantities are +// treated as zero, absorbing float rounding left over by trades. const goldDustEpsilon = 1e-9 + +// portfolioUpdateAttempts bounds optimistic-write retries in UpdatePortfolio. const portfolioUpdateAttempts = 5 // PortfolioStore is the gold module's typed portfolio store. type PortfolioStore = storage.DocStore[Portfolio] +// Portfolio is one user's gold account: a VND cash balance and gold held in +// lượng. type Portfolio struct { VND float64 `json:"vnd" bson:"vnd"` Luong float64 `json:"luong" bson:"luong"` Meta PortfolioMeta `json:"meta" bson:"meta"` } +// PortfolioMeta holds account-level totals: Invested is the sum of all VND +// top-ups and CreatedAt is ms since epoch. type PortfolioMeta struct { Invested float64 `json:"invested" bson:"invested"` CreatedAt int64 `json:"createdAt" bson:"createdAt"` @@ -51,6 +59,9 @@ func SavePortfolio(ctx context.Context, store PortfolioStore, userID int64, p Po return nil } +// UpdatePortfolio loads the user's portfolio, applies mutate, and writes it +// back with a versioned put, retrying on write conflicts. An error returned by +// mutate aborts without saving and is passed through unwrapped. func UpdatePortfolio(ctx context.Context, store PortfolioStore, userID int64, now int64, mutate func(*Portfolio) error) (Portfolio, error) { key := portfolioKey(userID) for attempt := 0; attempt < portfolioUpdateAttempts; attempt++ { diff --git a/internal/modules/gold/vnappmob_client.go b/internal/modules/gold/vnappmob_client.go index 899f2bd..bdb2350 100644 --- a/internal/modules/gold/vnappmob_client.go +++ b/internal/modules/gold/vnappmob_client.go @@ -33,7 +33,7 @@ type apiKeyCache struct { // VNAppMobClient fetches Vietnam SJC gold prices from api.vnappmob.com. // It self-manages a free JWT API key, caching it in the typed store and -// refreshing it before expiry or when the SJC endpoint returns 403. +// refreshing it before expiry or when the SJC endpoint returns 401 or 403. type VNAppMobClient struct { HTTP *http.Client BaseURL string // explicit test override; production uses https://api.vnappmob.com @@ -76,7 +76,7 @@ func (c *VNAppMobClient) httpClient() *http.Client { } // FetchSJCPrice returns the VNAppMob SJC buy/sell price per lượng in VND. -// On 403 it refreshes the API key once and retries. +// On 401 or 403 it refreshes the API key once and retries. func (c *VNAppMobClient) FetchSJCPrice(ctx context.Context) (buy, sell float64, err error) { key, err := c.getKey(ctx) if err != nil { diff --git a/internal/modules/lol/api_client.go b/internal/modules/lol/api_client.go index aefa1e2..30044a8 100644 --- a/internal/modules/lol/api_client.go +++ b/internal/modules/lol/api_client.go @@ -21,8 +21,9 @@ // on (lck, lpl, …) via leagueSlugMap; unmapped leagues pass through and the // major-league filter drops them naturally. // -// Cache strategy: live-first fetches with a KV-backed 60-minute stale -// fallback for current schedule windows. +// Cache strategy: live-first fetches with a document-store-backed 60-minute +// stale fallback for current schedule windows (today, tomorrow, this and next +// week, and the daily push). Explicit-date /lol lookups are always live. package lol import ( @@ -63,7 +64,7 @@ const ( // leagueSlugMap canonicalizes PandaScore league slugs to the slugs the // formatters were built on (format.go's majorLeagueSlugs / leagueOrder). -// Discovered live from /lol/leagues — see the plan's phase-01 findings. +// The PandaScore slugs were taken from the live /lol/leagues listing. // lta-north also maps to lcs: it is the LTA-era NA top flight; only the // slug is canonicalized, the display name still passes through. var leagueSlugMap = map[string]string{ @@ -408,7 +409,7 @@ func (c *Client) GetEventsWithFallback(ctx context.Context, cache CacheStore, fr // <= maxLen, appending "..." if cut. Keeps log output bounded — upstream // error pages can be large, and team names mix in Korean/Chinese characters // that a raw byte slice would split mid-codepoint (producing replacement -// glyphs in CloudWatch). +// glyphs in the logs). func truncate(s string, maxLen int) string { if len(s) <= maxLen { return s diff --git a/internal/modules/lol/api_client_test.go b/internal/modules/lol/api_client_test.go index f2ba904..74c8ff2 100644 --- a/internal/modules/lol/api_client_test.go +++ b/internal/modules/lol/api_client_test.go @@ -135,7 +135,7 @@ func TestGetEventsWithFallback_StaleFallback(t *testing.T) { staleEvents := []ScheduleEvent{ {StartTime: "2026-05-09T05:00:00Z", League: League{Slug: "lck", Name: "LCK"}}, } - // 10 minutes ago — past the 120s fresh window but well inside 60-min stale. + // 10 minutes ago — well inside the 60-minute stale window. staleTs := time.Now().UTC().Add(-10 * time.Minute).UnixMilli() if err := cache.Put(context.Background(), cacheKey(from, to), cacheRecord{Ts: staleTs, Events: staleEvents}); err != nil { t.Fatal(err) diff --git a/internal/modules/lol/cron.go b/internal/modules/lol/cron.go index b3f9530..f4551d2 100644 --- a/internal/modules/lol/cron.go +++ b/internal/modules/lol/cron.go @@ -77,16 +77,16 @@ func classifyTerminal(err error) terminalKind { return terminalNone } -// dailyPushCronName is the cron route segment + in-process scheduler key. -// Must match the regex in internal/server/router.go (^[a-z0-9_]{1,32}$). +// dailyPushCronName is the cron's registry and in-process scheduler key; it +// must be unique across all modules' crons. const dailyPushCronName = "lol_daily_push" // dailyPushSchedule drives the in-process scheduler (internal/cron). Cron // expression is UTC; 01:00 UTC == 08:00 ICT. const dailyPushSchedule = "0 1 * * *" -// lastPushDateKey records the ICT date (YYYY-MM-DD) of the most recent -// completed daily push. The handler claims this key before fanning out and +// lastPushDateKey records the ICT date (YYYY-MM-DD) of the most recently +// claimed daily push. The handler claims this key before fanning out and // no-ops if it is already today's schedule day, making the push idempotent per // ICT date. This defends against double-fire windows from rolling deploys that // briefly run two containers or operator misconfiguration. @@ -116,7 +116,8 @@ type lastPushDoc struct { // PushDateStore is the typed store for last-push date documents. type PushDateStore = storage.DocStore[lastPushDoc] -// dailyPushCron returns the cron registration. Schedule is documentation only. +// dailyPushCron returns the cron registration; the in-process scheduler fires +// the handler on Schedule. func (s *state) dailyPushCron() modules.Cron { return modules.Cron{ Name: dailyPushCronName, @@ -238,9 +239,8 @@ func runDailyPush(ctx context.Context, s *state, sender messageSender) error { sent++ } - // Best-effort prune. Failure here just leaves the dead chats in the list - // for tomorrow's push — same behaviour as before this code existed, so - // strictly an improvement even when the writes fail. + // Best-effort prune. A failed write just leaves the dead chats in the list, + // and tomorrow's push fails on them again and retries the prune. pruned := pruneDeadSubscribers(ctx, s, deadChats, deadTopics) log.Info("lol daily push complete", diff --git a/internal/modules/lol/format.go b/internal/modules/lol/format.go index 09bec64..95c921d 100644 --- a/internal/modules/lol/format.go +++ b/internal/modules/lol/format.go @@ -23,8 +23,8 @@ var leagueOrder = []string{ } // majorLeagueSlugs filters the upstream schedule down to the headline -// tournaments most viewers care about. Without this filter the API -// returns 135+ events/week and replies blow past Telegram's 4096-char limit. +// tournaments most viewers care about. Without this filter PandaScore returns +// ~270 matches/week and replies blow past Telegram's 4096-char limit. var majorLeagueSlugs = map[string]bool{ "lck": true, "lpl": true, @@ -81,9 +81,9 @@ func teamLabel(t Team) string { } // declaredOutcome returns a team's upstream-declared series outcome ("win" or -// "loss"), or "" when upstream has not published one. Two distinct upstream -// shapes collapse to "": a missing `result` object, and the far more common -// `{"outcome": null, "gameWins": 0}`. +// "loss"), or "" when upstream has not published one. Two shapes collapse to +// "": a nil Result, and the far more common Result with no Outcome that +// toScheduleEvent builds until PandaScore commits a winner. func declaredOutcome(t Team) string { if t.Result == nil { return "" diff --git a/internal/modules/lol/format_test.go b/internal/modules/lol/format_test.go index 6a52451..cadb5fc 100644 --- a/internal/modules/lol/format_test.go +++ b/internal/modules/lol/format_test.go @@ -66,10 +66,9 @@ func TestFormatEventLine_Completed_BoldsWinner(t *testing.T) { } } -// Upstream flips state to "completed" when the broadcast window closes, but -// fills gameWins/outcome from a separate per-game ingestion path. In the gap it -// sends {"outcome": null, "gameWins": 0} for both teams — a shape that must not -// be reported as a real 0–0 draw. +// Upstream can mark a series finished before committing a winner. In that gap +// both teams carry a Result with no outcome and zero gameWins — a shape that +// must not be reported as a real 0–0 draw. func TestFormatEventLine_CompletedWithoutResults_OmitsScore(t *testing.T) { pending := &TeamResult{} // json `{"outcome": null, "gameWins": 0}` e := ScheduleEvent{ diff --git a/internal/modules/lol/parse_date.go b/internal/modules/lol/parse_date.go index 556666a..2c83884 100644 --- a/internal/modules/lol/parse_date.go +++ b/internal/modules/lol/parse_date.go @@ -59,8 +59,8 @@ func addDays(date time.Time, days int) time.Time { } // splitParts breaks the trimmed input into [dd, mm?, yyyy?] string parts. -// Accepts dash- or slash-separated values, or a 1/2/4/8-digit unbroken -// run (today, this-month, this-year, full ddmmyyyy). +// Accepts dash- or slash-separated values, or an unbroken digit run: 1-2 +// digits (dd, current month), 4 (ddmm, current year), or 8 (ddmmyyyy). func splitParts(trimmed string) ([]string, string) { if strings.ContainsAny(trimmed, "-/") { // Replace both delimiters with a single one, then split. diff --git a/internal/modules/loldle/compare.go b/internal/modules/loldle/compare.go index a14f214..0a73443 100644 --- a/internal/modules/loldle/compare.go +++ b/internal/modules/loldle/compare.go @@ -14,8 +14,8 @@ const ( attrYear AttrType = "year" ) -// Result categories. handlers/render rely on these literals via the marker -// maps, so renaming a constant requires updating those maps in lockstep. +// Result categories stored in AttributeRow.Result. render.go's markerFor +// maps each one to its on-screen marker; an unknown value renders as wrong. const ( ResultCorrect = "correct" ResultPartial = "partial" @@ -24,8 +24,8 @@ const ( // AttributeRow describes one attribute's comparison output as one row of // the render board: key/label identify the row, type drives the comparison -// algorithm, result is the rendered marker, direction is set only for -// wrong year-type rows ("up"/"down"). +// algorithm, result is the category that render maps to a marker, direction +// is set only for wrong year-type rows ("up"/"down"). type AttributeRow struct { Key string Label string @@ -206,13 +206,12 @@ func yearOrPlaceholder(y int) string { if y == 0 { return "?" } - // strconv would pull in another import; for a 4-digit positive int the - // Sprintf path is fine and zero-allocs after warmup. return fmtYear(y) } +// fmtYear renders y as exactly four digits, zero-padded. Manual base-10 +// keeps strconv out of this file; parseYear never yields more than 4 digits. func fmtYear(y int) string { - // Manual base-10 to avoid strconv import; year is always 4 digits here. return string([]byte{ byte('0' + (y/1000)%10), byte('0' + (y/100)%10), diff --git a/internal/modules/loldle/handlers.go b/internal/modules/loldle/handlers.go index d26526d..859fa7d 100644 --- a/internal/modules/loldle/handlers.go +++ b/internal/modules/loldle/handlers.go @@ -37,9 +37,10 @@ func (s *state) findByName(name string) *Champion { } // rehydrateGuesses recomputes board rows from the stored championNames. -// Champions removed from champions.json since the round started are skipped -// (returns the surviving prefix) so a data refresh never breaks an active -// round. +// Guesses naming champions removed from champions.json since the round +// started are skipped, so a data refresh never breaks an active round. A +// missing target yields an empty board; handleLoldle clears that round on +// the next guess. func (s *state) rehydrateGuesses(g *gameState) []boardEntry { target := s.findByName(g.Target) if target == nil { @@ -273,8 +274,9 @@ func (s *state) handleStats(ctx context.Context, b *bot.Bot, update *models.Upda scope, st.Played, st.Wins, chathelper.WinRate(st.Wins, st.Played), st.Streak, st.BestStreak)) } -// handleSetMax is /loldle_setmax — private command, sets the per-subject -// MaxGuesses override (1..MaxGuessesCap). Takes effect on the next round. +// handleSetMax is /loldle_setmax — owner-only (VisibilityPrivate), sets +// the per-subject MaxGuesses override (1..MaxGuessesCap). Takes effect on the +// next round because an active round keeps its frozen MaxGuesses. func (s *state) handleSetMax(ctx context.Context, b *bot.Bot, update *models.Update) error { msg := update.Message if msg == nil { diff --git a/internal/modules/loldle/lookup.go b/internal/modules/loldle/lookup.go index 8da8aba..34fe963 100644 --- a/internal/modules/loldle/lookup.go +++ b/internal/modules/loldle/lookup.go @@ -31,6 +31,9 @@ func findChampion(pool []Champion, input string) *Champion { return champion } +// findChampionMatch implements findChampion and additionally reports whether +// a nil result came from an ambiguous prefix, so handlers can ask for the +// full name instead of saying "not found". func findChampionMatch(pool []Champion, input string) (*Champion, bool) { q := normalizeName(input) if q == "" { diff --git a/internal/modules/loldle/state.go b/internal/modules/loldle/state.go index c9ec87c..36df0b6 100644 --- a/internal/modules/loldle/state.go +++ b/internal/modules/loldle/state.go @@ -22,7 +22,7 @@ const ( // and lose that distinction. // // Guesses is just championNames; comparison rows are recomputed at render -// time against current champions.json so a weekly data refresh updates +// time against current champions.json so a data refresh updates // historical board displays without migrating saved rounds. type gameState struct { Target string `json:"target" bson:"target"` diff --git a/internal/modules/misc/misc.go b/internal/modules/misc/misc.go index 9ac1461..44c90d3 100644 --- a/internal/modules/misc/misc.go +++ b/internal/modules/misc/misc.go @@ -1,10 +1,10 @@ -// Package misc is a small stub module that proves the framework end-to-end: -// /ping (public, exercises KV write), /ping_stats (protected, exercises KV -// read), /random (public random picker), /wheelofnames (public wheel picker -// with optional GIF), /ff (protected give-up-on-T1 rant), /xlt1 (public +// Package misc is a small module that proves the framework end-to-end: +// /ping (public, exercises a store write), /ping_stats (protected, exercises a +// store read), /random (public random picker), /wheelofnames (public wheel +// picker with optional GIF), /ff (protected give-up-on-T1 rant), /xlt1 (public // apologise-to-T1 petition, the sequel to /ff), /giaxang (public Petrolimex -// retail fuel prices), /the_answer (private easter egg), and small public -// disclaimer commands. +// retail fuel prices), /the_answer (private easter egg), and the public +// disclaimer commands /trongtruonghop, /tth, /trongtruonghopvng, and /tthvng. package misc import ( @@ -31,17 +31,17 @@ const lastPingKey = "last_ping" // commands. Three %s slots: target (escaped), sender mention, sender mention. const trongTruongHopTemplate = "Trong trường hợp nhóm này bị điều tra bởi %s, %s khẳng định không liên quan tới nhóm hoặc những cá nhân khác trong nhóm này. %s không rõ tại sao lại có mặt ở đây vào thời điểm này, có lẽ tài khoản đã được thêm bởi một bên thứ ba." -// defaultTarget is the substituted "investigator" name when /trongtruonghop is -// invoked without an argument. The command keeps a custom-arg override. +// defaultTarget is the substituted "investigator" name when /trongtruonghop or +// /tth is invoked without an argument. Both accept a custom target. const defaultTarget = "các cơ quan trực thuộc Bộ CA hoặc các tổ chức chính trị tương tự phục vụ cho nhà nước CHXHCNVN" // vngTarget is the fixed substituted "investigator" name for -// /trongtruonghopvng. That command intentionally ignores custom args. +// /trongtruonghopvng and /tthvng. Both intentionally ignore custom args. const vngTarget = "công ty cổ phần tập đoàn VNG nói chung và công ty 2morebits nói riêng" // lastPing is the value stored at the `last_ping` key: { at: }. // int64 ms-epoch (not time.Time → RFC3339) keeps the on-disk shape compact -// and consistent with every other timestamp field in the bot's KV. +// and consistent with every other timestamp field the bot stores. type lastPing struct { At int64 `json:"at" bson:"at"` } @@ -131,6 +131,8 @@ func senderMention(u *models.User) string { return fmt.Sprintf(`%s`, u.ID, html.EscapeString(name)) } +// disclaimerCommand builds one disclaimer command. With allowCustomTarget the +// command argument, when present, replaces defaultTarget. func disclaimerCommand(name, description, defaultTarget string, allowCustomTarget bool) modules.Command { parameters := "" if allowCustomTarget { diff --git a/internal/modules/module.go b/internal/modules/module.go index 81964db..7052a68 100644 --- a/internal/modules/module.go +++ b/internal/modules/module.go @@ -22,9 +22,10 @@ const ( ) // CommandHandler runs in response to a Telegram command. Returning an error -// causes the dispatcher to log the failure. Telegram retries are governed by -// the webhook HTTP status (200), not handler errors — so the error return is -// purely for logging/metrics, not flow control. +// causes the dispatcher to log the failure and count it in metrics. Updates +// arrive by long polling and are consumed either way — Telegram never +// redelivers on a handler error — so the error return is purely for +// logging/metrics, not flow control. type CommandHandler func(ctx context.Context, b *bot.Bot, update *models.Update) error // CallbackHandler runs in response to inline-keyboard callback data. Callback @@ -39,11 +40,9 @@ type Callback struct { Handler CallbackHandler } -// CronHandler runs when a cron fires — driven by the in-process scheduler -// (internal/cron) on self-host, or by a POST to /cron/{name} for manual -// triggers. Crons receive the per-module-prefixed Deps via the registry; -// handlers should not capture the base Deps from the factory closure or KV -// writes will collide across modules. +// CronHandler runs when a cron fires, driven by the in-process scheduler +// (internal/cron). The handler receives the owning module's Deps — the same +// bundle its Factory got, including its own storage Collection. type CronHandler func(ctx context.Context, deps Deps) error // Command is a single Telegram bot command exposed by a module. @@ -74,8 +73,8 @@ type Module struct { Callbacks []Callback Crons []Cron CommandHook func(ctx context.Context, name string, update *models.Update) // optional; called by dispatcher after each authorized command invocation. update carries the originating Telegram update so hooks can attribute usage to a user. - Fallback *CommandFallback // optional; handles a /command no module registered. At most one across all modules. - Inline *InlineQuery // optional; handles inline-mode queries. At most one across all modules. + Fallback *CommandFallback // optional; handles a /command no module registered. At most one across all modules. + Inline *InlineQuery // optional; handles inline-mode queries. At most one across all modules. } // CommandFallback handles a /command that no module registered. diff --git a/internal/modules/modules.go b/internal/modules/modules.go index 7ee4e9c..7b7756d 100644 --- a/internal/modules/modules.go +++ b/internal/modules/modules.go @@ -1,8 +1,10 @@ -package modules - -// This file used to hold a static `Factories` catalog. With concrete modules -// now living in subpackages (internal/modules/util, /misc, …), keeping the -// catalog here would create an import cycle (modules → util → modules). +// Package modules is the bot's module framework: the Module, Command, Callback +// and Cron types a feature package declares, the Registry that Build assembles +// from the MODULES selection, and the dispatcher that Install wires into the +// Telegram bot with visibility-based authorization. // -// The composition root in cmd/server owns the catalog instead. Tests pass -// their own catalog into Build, exercising only the modules they care about. +// The module catalog (name → Factory) does not live here. Concrete modules are +// subpackages (internal/modules/util, /misc, …) that import this package, so +// keeping the catalog here would create an import cycle. The composition root +// in cmd/server owns it instead, and tests pass their own catalog into Build. +package modules diff --git a/internal/modules/monkeyd/handlers_test.go b/internal/modules/monkeyd/handlers_test.go index 14b1439..86dd504 100644 --- a/internal/modules/monkeyd/handlers_test.go +++ b/internal/modules/monkeyd/handlers_test.go @@ -23,8 +23,8 @@ const testNovelURL = "https://monkeydd.com/tro-lai-nam-thang-cu.html" // behind the registered command, so tests can substitute its exporter and run // the export synchronously instead of on a detached goroutine. // -// ownerID is permitted, which /monkeyd_crawl requires: the command is -// Protected, and the dispatcher drops unauthorized calls silently. +// ownerID becomes the bot owner. Both commands are public, so it grants no +// extra access; the public-access tests below rely on a non-owner sender. func install(t *testing.T, ownerID int64) (*testutil.RecordingBot, *runner) { t.Helper() return installWith(t, ownerID, func(context.Context, string) ([]string, error) { diff --git a/internal/modules/monkeyd/monkeyd.go b/internal/modules/monkeyd/monkeyd.go index d801974..ef7bb40 100644 --- a/internal/modules/monkeyd/monkeyd.go +++ b/internal/modules/monkeyd/monkeyd.go @@ -1,7 +1,9 @@ // Package monkeyd exports a monkeydd.com novel as a PDF and sends it back as a -// Telegram document. The crawling and rendering live in the monkeyd-crawler -// submodule (third_party/monkeyd-crawler); this module is the Telegram surface -// around it: argument validation, one-at-a-time scheduling, and delivery. +// Telegram document (/monkeyd_crawl), and reports a novel's genre tags as a +// copyable hashtag line (/monkeyd_tags). The crawling and rendering live in the +// monkeyd-crawler submodule (third_party/monkeyd-crawler); this module is the +// Telegram surface around it: argument validation, one-at-a-time scheduling, +// and delivery. package monkeyd import ( @@ -19,7 +21,7 @@ import ( "github.com/tiennm99/miti99bot/internal/modules/util/chathelper" ) -// commandName is the single command this module exposes. +// commandName is the PDF export command; tagsCommandName is the other one. const commandName = "monkeyd_crawl" // parameters is the display syntax shared by the command menu, /help, and the diff --git a/internal/modules/monkeyd/tags_command.go b/internal/modules/monkeyd/tags_command.go index a54942b..b800441 100644 --- a/internal/modules/monkeyd/tags_command.go +++ b/internal/modules/monkeyd/tags_command.go @@ -30,7 +30,7 @@ const tagsParameters = "" const tagsUsage = "Usage: /" + tagsCommandName + " " + tagsParameters const ( - // tagsFetchTimeout bounds the whole command. Unlike an export this is one + // tagsFetchTimeout bounds the tag fetch. Unlike an export this is one // request and runs inline, and handlers are dispatched one at a time, so a // stalled fetch would hold up every other command until it gives up. tagsFetchTimeout = 30 * time.Second @@ -40,8 +40,9 @@ const ( tagsRetries = 1 ) -// tagsFetcher returns a novel's tags. A field on the module rather than a -// direct call so tests can exercise the command without network access. +// tagsFetcher returns a novel's tags. It is injected through newModule rather +// than called directly so tests can exercise the command without network +// access. type tagsFetcher func(ctx context.Context, novelURL string) ([]string, error) func tagsCommand(fetch tagsFetcher) modules.Command { diff --git a/internal/modules/registry.go b/internal/modules/registry.go index 39b0ff8..b80c0af 100644 --- a/internal/modules/registry.go +++ b/internal/modules/registry.go @@ -14,13 +14,13 @@ import ( ) // moduleNameRe is intentionally looser than commandNameRe — it allows hyphen -// so module names can carry hyphenated suffixes (e.g. "loldle-classic"). The -// crucial constraint is "no `:`" so the storage Prefixed wrapper's `:` -// delimiter cannot be subverted; everything else is style. +// because a module name is only a catalog key and a collection name, never a +// Telegram command. It must stay identical to storage's collectionNameRe: the +// providers re-validate the name and hand back an always-failing store for +// anything outside that alphabet. // // Telegram command names still need the stricter [a-z0-9_]{1,32} alphabet -// (commandNameRe in validate.go). Cron route segments use their own regex in -// internal/server/router.go and stay strict for log-injection safety. +// (commandNameRe in validate.go). var moduleNameRe = regexp.MustCompile(`^[a-z0-9_-]{1,32}$`) // Registry holds the resolved set of modules selected by the MODULES env var. @@ -36,7 +36,7 @@ type Registry struct { protected map[string]Command private map[string]Command crons map[string]Cron // name → Cron, unique across modules - cronDeps map[string]Deps // cron name → owning module's prefixed Deps + cronDeps map[string]Deps // cron name → owning module's Deps callbacks map[string]Callback commandHooks []func(ctx context.Context, name string, update *models.Update) fallback *CommandFallback // at most one; owner tracked in Build @@ -127,8 +127,8 @@ func (r *Registry) Cron(name string) (Cron, bool) { return c, ok } -// CronDeps returns the per-module-prefixed Deps the cron's owning module -// received. The cron dispatcher uses this to pass scoped Deps to the handler. +// CronDeps returns the Deps the cron's owning module received from Build. +// The cron dispatcher passes them to the handler. func (r *Registry) CronDeps(name string) (Deps, bool) { d, ok := r.cronDeps[name] return d, ok @@ -155,6 +155,13 @@ func (r *Registry) Crons() []Cron { return out } +// BuildOptions bundles the optional dependencies threaded into every Module +// Factory's Deps. Adding new optional deps here keeps Build's signature +// stable as the dep list grows. +type BuildOptions struct { + Bot *bot.Bot +} + // Build constructs a Registry from the requested module names. The Provider // supplies a per-module-isolated Collection (one MongoDB collection per module; // MemoryProvider keeps one map per module). Build validates every command/cron @@ -163,13 +170,6 @@ func (r *Registry) Crons() []Cron { // Names not present in factories are reported as a single error so a typo in // MODULES does not silently load a smaller bot than intended. Duplicate names // in MODULES are also a hard error to keep startup deterministic. -// BuildOptions bundles the optional dependencies threaded into every Module -// Factory's Deps. Adding new optional deps here keeps Build's signature -// stable as the dep list grows. -type BuildOptions struct { - Bot *bot.Bot -} - func Build(enabled []string, factories map[string]Factory, provider storage.Provider, opts BuildOptions) (*Registry, error) { if provider == nil { return nil, fmt.Errorf("modules: storage Provider is required") diff --git a/internal/modules/registry_test.go b/internal/modules/registry_test.go index 58d6b29..aa05b54 100644 --- a/internal/modules/registry_test.go +++ b/internal/modules/registry_test.go @@ -161,7 +161,7 @@ func TestBuild_RejectsInvalidCallback(t *testing.T) { func TestBuild_RequiresProvider(t *testing.T) { _, err := Build(nil, map[string]Factory{}, nil, BuildOptions{}) if err == nil { - t.Error("expected error when KVProvider is nil") + t.Error("expected error when the storage provider is nil") } } @@ -256,8 +256,9 @@ func TestDispatchScheduled_PassesScopedDeps(t *testing.T) { func TestBuild_RejectsInvalidModuleName(t *testing.T) { // `-` is intentionally allowed so modules can carry hyphenated names. `:` - // must stay rejected — it's the storage prefix delimiter and a - // hyphen-allowing regex must not let it through. + // must stay rejected — the module alphabet mirrors storage's + // collection-name alphabet, and loosening one without the other would + // hand a module an always-failing store. for _, name := range []string{"BadName", "a:b", "", "with space", "with.dot", "with/slash"} { t.Run(name, func(t *testing.T) { _, err := Build([]string{name}, map[string]Factory{}, newProvider(), BuildOptions{}) diff --git a/internal/modules/stats/startup.go b/internal/modules/stats/startup.go index 9fa2046..576b0d3 100644 --- a/internal/modules/stats/startup.go +++ b/internal/modules/stats/startup.go @@ -14,6 +14,9 @@ import ( "github.com/tiennm99/miti99bot/internal/systemstate" ) +// The retired stock_dividend command's usage rows are soft-deleted (marked +// deleted: true) rather than removed, and the run is recorded under +// deletedStockDividendMarkerKey in the system collection. const ( statsCommandUsersIndexName = "stats_cmd_n_user" statsUserCommandsIndexName = "stats_uid_n_cmd" @@ -24,7 +27,8 @@ const ( ) // InitStore performs stats collection startup maintenance. MongoDB index -// creation and the legacy-command migration are both safe to run every boot. +// creation and the retired-command migration are both idempotent and run on +// every boot. func InitStore(ctx context.Context, statsColl, systemColl storage.Collection) error { if mongoColl, ok := storage.MongoCollection(statsColl); ok { if err := ensureUsageIndexes(ctx, mongoColl); err != nil { @@ -34,6 +38,14 @@ func InitStore(ctx context.Context, statsColl, systemColl storage.Collection) er return markDeletedCommand(ctx, statsColl, systemColl, deletedStockDividendCommand, deletedStockDividendMarkerKey) } +// markDeletedCommand soft-deletes every usage row of a retired command and +// records the outcome in the system-state marker under markerKey. +// +// It runs on every boot rather than stopping once the marker exists: an older +// build still running alongside this one (during a rolling deploy, say) can +// write fresh rows for the command after the first run, and each later boot +// sweeps those up. CompletedAt keeps the first run's time; Count and UpdatedAt +// reflect the latest one. func markDeletedCommand(ctx context.Context, statsColl, systemColl storage.Collection, command, markerKey string) error { system := systemstate.New(systemColl) marker, exists, err := system.Get(ctx, markerKey) @@ -103,6 +115,10 @@ func markDocUsageEntriesDeleted(ctx context.Context, docs storage.DocStore[usage return matched, nil } +// markUsageEntryDeleted flags one row as deleted with a versioned write, +// retrying on conflict so a concurrent writer's update is never overwritten. It +// reports whether the row belongs to command at all, since the key prefix +// alone also matches longer command names. func markUsageEntryDeleted(ctx context.Context, docs storage.DocStore[usageEntry], key, command string) (bool, error) { for attempt := 0; attempt < deletedCommandMigrationRetries; attempt++ { entry, version, err := docs.Get(ctx, key) diff --git a/internal/modules/stats/stats.go b/internal/modules/stats/stats.go index b5ff6cc..00f7690 100644 --- a/internal/modules/stats/stats.go +++ b/internal/modules/stats/stats.go @@ -1,5 +1,11 @@ // Package stats tracks command usage and exposes /stats subcommands sorted by // popularity. +// +// Counts are recorded through a CommandHook, so every authorized command +// invocation of every module is counted without the modules knowing. Storage +// goes through Mongo aggregations when the collection is MongoDB-backed and +// falls back to reading the whole collection through the document store +// otherwise (the in-memory provider). package stats import ( @@ -12,6 +18,7 @@ import ( "github.com/tiennm99/miti99bot/internal/storage" ) +// topK caps the rows every /stats view shows. const topK = 20 // counter owns the stats repository used by the command hook and render views. diff --git a/internal/modules/stats/stats_test.go b/internal/modules/stats/stats_test.go index b2ded2c..5da96be 100644 --- a/internal/modules/stats/stats_test.go +++ b/internal/modules/stats/stats_test.go @@ -286,7 +286,7 @@ func seedFixture(t *testing.T, c *counter) { ctx := context.Background() // alice (id=1): /ping x3, /wordle x1 // bob (id=2): /ping x1, /wordle x2 - // carol (id=3): /ping x1 (username later cleared to test skip) + // carol (id=3): /ping x1 for i := 0; i < 3; i++ { c.Inc(ctx, "ping", updateFrom(1, "alice")) } @@ -356,10 +356,9 @@ func TestRenderStats_UserCommands(t *testing.T) { } } -// Pins the leading-colon disambiguation in viewUserCommands: a user ID -// suffix like ":2" must not falsely match a pair key for a different user -// whose ID happens to end in "2" (e.g. 12, 22, 42, 142). Both reviewers -// flagged this as a potential bug; this test proves the absence of the bug. +// A user's view is resolved by the stored user ID, not by matching the ":" +// key suffix, so user 2's view must not pick up rows belonging to a user whose +// ID merely ends in "2" (e.g. 12, 22, 42, 142). func TestRenderStats_UserCommands_IDSuffixDoesNotFalseMatch(t *testing.T) { ctx := context.Background() c := newStatsCounter() diff --git a/internal/modules/stats/usage_store.go b/internal/modules/stats/usage_store.go index 84c2d21..1913011 100644 --- a/internal/modules/stats/usage_store.go +++ b/internal/modules/stats/usage_store.go @@ -48,6 +48,8 @@ func newUsageStore(coll storage.Collection) usageStore { return &docUsageStore{docs: storage.Typed[usageEntry](coll)} } +// usageKey names a usage row: the bare command for the anonymous bucket (a +// sender without a public username), or ":" for a per-user row. func usageKey(cmd string, userID int64) string { if userID == 0 { return cmd @@ -55,6 +57,11 @@ func usageKey(cmd string, userID int64) string { return cmd + ":" + strconv.FormatInt(userID, 10) } +// docUsageStore implements usageStore over a plain DocStore, for the in-memory +// provider. Every aggregate reads the whole collection, which is fine at the +// sizes that backend sees. mu serialises the read-modify-write in Increment, +// since Put is unconditional and two concurrent increments would otherwise +// lose one. type docUsageStore struct { mu sync.Mutex docs storage.DocStore[usageEntry] @@ -242,6 +249,8 @@ func (s *docUsageStore) loadEntriesLocked(ctx context.Context) ([]usageEntry, er return entries, nil } +// mongoUsageStore implements usageStore with native MongoDB updates and +// aggregations, backed by the indexes ensureUsageIndexes creates. type mongoUsageStore struct { coll *mongo.Collection } @@ -337,6 +346,8 @@ func (s *mongoUsageStore) TopUsers(ctx context.Context, limit int) ([]row, error bsonField("user", bson.D{bsonField("$type", "string"), bsonField("$ne", "")}), bsonField("deleted", bson.D{bsonField("$ne", true)}), })}, + // Newest row first, so $first labels each user with their most recent + // username. bson.D{bsonField("$sort", bson.D{bsonField("updatedAt", -1)})}, bson.D{bsonField("$group", bson.D{ bsonField("_id", "$uid"), @@ -464,6 +475,9 @@ func (s *mongoUsageStore) userIDByUsername(ctx context.Context, username string) return doc.UserID, true, nil } +// isRetiredCommand reports whether cmd has been removed from the bot. Its rows +// are never incremented or shown, even ones written before the migration +// marked them deleted. func isRetiredCommand(cmd string) bool { return cmd == deletedStockDividendCommand } diff --git a/internal/modules/stats/views.go b/internal/modules/stats/views.go index 26a1446..a64dc3a 100644 --- a/internal/modules/stats/views.go +++ b/internal/modules/stats/views.go @@ -14,7 +14,10 @@ import ( "github.com/tiennm99/miti99bot/internal/modules/util/chathelper" ) -const telegramMaxLen = 4000 // leave margin below Telegram's 4096-byte hard limit +// telegramMaxLen leaves margin below Telegram's 4096-character message limit. +// It is compared against the byte length, which is never shorter than the +// character count, so the check errs on the safe side. +const telegramMaxLen = 4000 const statsUsage = `Usage: /stats @@ -22,6 +25,8 @@ const statsUsage = `Usage: /stats user /stats cmd ` +// row is one rendered line of a /stats view: a "/command" or "@username" +// label and its count. type row struct { display string n int64 diff --git a/internal/modules/sticker/addsticker_command.go b/internal/modules/sticker/addsticker_command.go index a035258..039d14f 100644 --- a/internal/modules/sticker/addsticker_command.go +++ b/internal/modules/sticker/addsticker_command.go @@ -50,8 +50,7 @@ type stickerSource struct { // Every caller writes to the same shared pack, named by STICKER_PACK_NAME. The // caller's own identity is not used anywhere: AddStickerToSet takes the *set // owner's* user ID, so there is nothing per-user to store, key, or lock, and -// no ownership to check. That is what lets this live in util as one stateless -// command rather than as a module. +// no ownership to check. That is what keeps the command stateless. // // The resolver is built here and captured by the handler so the bot's username // is fetched at most once per process rather than once per invocation. diff --git a/internal/modules/sticker/addsticker_command_test.go b/internal/modules/sticker/addsticker_command_test.go index e2605ed..f37e483 100644 --- a/internal/modules/sticker/addsticker_command_test.go +++ b/internal/modules/sticker/addsticker_command_test.go @@ -28,7 +28,7 @@ func installSticker(t *testing.T) *testutil.RecordingBot { return rb } -// installAddSticker builds the util module with the shared pack configured and +// installAddSticker builds the sticker module with the shared pack configured and // getMe stubbed. // // getMe must be stubbed explicitly: the bot runs with WithSkipGetMe(), and diff --git a/internal/modules/sticker/sticker_emoji_test.go b/internal/modules/sticker/sticker_emoji_test.go index 8d1b1b7..2e28b77 100644 --- a/internal/modules/sticker/sticker_emoji_test.go +++ b/internal/modules/sticker/sticker_emoji_test.go @@ -42,7 +42,7 @@ func TestParseEmoji(t *testing.T) { } } -// A stray word must fail loudly. With no pack argument left on /addsticker, +// A stray word must fail loudly. /addsticker takes no argument but emoji, so // there is nothing else an argument could have meant. func TestParseEmoji_RejectsPlainText(t *testing.T) { for _, arg := range []string{"mypack", "hello 😂", "a"} { @@ -74,9 +74,10 @@ func TestParseEmoji_RefusalIsUserFacing(t *testing.T) { } // TestParseEmoji_ClusterEdgeCases pins the emoji-clustering rules that a -// hand-rolled segmenter gets wrong. Every case here failed before the -// clustering fix: the first group was refused outright, the second silently -// produced an emoji_list Telegram rejects. +// hand-rolled segmenter gets wrong. Most cases broke an earlier version: it +// refused or mis-split the accepted group, and let the refused group through +// as an emoji_list Telegram rejects. Sequences that already worked sit +// alongside so a fix cannot regress them. func TestParseEmoji_ClusterEdgeCases(t *testing.T) { accepted := []struct { name string diff --git a/internal/modules/sticker/sticker_image_test.go b/internal/modules/sticker/sticker_image_test.go index fbd2811..5a8ecc5 100644 --- a/internal/modules/sticker/sticker_image_test.go +++ b/internal/modules/sticker/sticker_image_test.go @@ -66,7 +66,7 @@ func TestToStickerPNG_Geometry(t *testing.T) { // An extreme aspect ratio must not round the short edge down to zero, which // would produce an invalid image rather than an error. // -// 1x4000 rather than the plan's 1x5000: 5000 is past maxDecodeDimension, so +// 1x4000 rather than 1x5000: 5000 is past maxDecodeDimension, so // that case never reaches the scaler at all — it is rejected by the guard // below. The clamp still needs exercising, and this is the most extreme ratio // that actually gets there (512/4000 rounds to 0 before clamping). diff --git a/internal/modules/sticker/sticker_pack.go b/internal/modules/sticker/sticker_pack.go index b7092b5..f366231 100644 --- a/internal/modules/sticker/sticker_pack.go +++ b/internal/modules/sticker/sticker_pack.go @@ -17,10 +17,10 @@ import ( ) const ( - // stickerPackNameEnv overrides which set /addsticker writes to. The set - // must already exist and must have been created by this bot, which is the - // only thing that makes it bot-manageable — there is no command to create - // one, by design. + // stickerPackNameEnv overrides which set /addsticker writes to. The name + // must end in "_by_", the only thing that makes a set + // bot-manageable; packTitle checks it before any upload. A set that does + // not exist yet is created by the first successful /addsticker. stickerPackNameEnv = "STICKER_PACK_NAME" // defaultStickerPackName is the shared pack used when the env is unset. diff --git a/internal/modules/sticker/sticker_video.go b/internal/modules/sticker/sticker_video.go index 44c6d6b..4aa6fe2 100644 --- a/internal/modules/sticker/sticker_video.go +++ b/internal/modules/sticker/sticker_video.go @@ -111,8 +111,8 @@ func runFFmpeg(ctx context.Context, in, out string, crf int) ([]byte, error) { // #nosec G204 — no part of argv is user input. The binary is a package // constant, the flags are literals, the numbers come from constants and the - // CRF ladder, the filter is built from constants, and in/out are paths this - // function made under its own MkdirTemp directory. The caller's bytes reach + // CRF ladder, the filter is built from constants, and in/out are paths + // toStickerWEBM made under its own MkdirTemp directory. The caller's bytes reach // ffmpeg as the *contents* of `in`, never as an argument. cmd := exec.CommandContext(ctx, ffmpegBinary, "-hide_banner", "-loglevel", "error", @@ -144,7 +144,7 @@ func runFFmpeg(ctx context.Context, in, out string, crf int) ([]byte, error) { } // #nosec G304 — `out` is not a caller-supplied path: it is filepath.Join of - // this function's own MkdirTemp directory and a fixed file name. + // toStickerWEBM's own MkdirTemp directory and a fixed file name. data, err := os.ReadFile(out) if err != nil { return nil, fmt.Errorf("sticker video: read output: %w", err) diff --git a/internal/modules/stock/dividend_events_ssi.go b/internal/modules/stock/dividend_events_ssi.go index 0981fa2..614c3fc 100644 --- a/internal/modules/stock/dividend_events_ssi.go +++ b/internal/modules/stock/dividend_events_ssi.go @@ -32,8 +32,8 @@ var ssiProviderIDPattern = regexp.MustCompile(`^[A-Za-z0-9_-]{1,64}$`) // public endpoint. It is isolated behind DividendEventProvider because SSI does // not publish a compatibility or availability guarantee for this endpoint. type SSIDividendProvider struct { - HTTP *http.Client - BaseURL string + HTTP *http.Client // Optional; nil uses a shared client with ssiDividendTimeout. + BaseURL string // Optional SSI iBoard API base override. defaultOnce sync.Once defaultClient *http.Client @@ -108,8 +108,9 @@ func (p *SSIDividendProvider) endpoint() string { } // FetchDividendEvents returns only validated cash dividends and explicitly -// described share dividends. SSI is queried with a one-calendar-day overlap in -// Asia/Saigon, then results are filtered by publication time to (after, through]. +// described share dividends, sorted by publication time. SSI filters by +// calendar day, so the query and the result both keep a one-day overlap before +// after (from the start of the previous Asia/Saigon day) and end at through. func (p *SSIDividendProvider) FetchDividendEvents(ctx context.Context, symbol string, after, through time.Time) ([]DividendEvent, error) { symbol, from, to, err := prepareSSIEventFetch( symbol, @@ -251,6 +252,9 @@ func (p *SSIDividendProvider) copyStockEvent(raw ssiDividendEvent, requestedSymb }, true } +// ssiStockEventCursor picks the timestamp /stock_events filters and sorts by: +// publicDate when present, otherwise the first parseable ex-right, record, or +// issue date. func ssiStockEventCursor(raw ssiDividendEvent) (time.Time, bool) { if strings.TrimSpace(raw.PublicDate) != "" { return parseSSIDate(raw.PublicDate) @@ -340,9 +344,10 @@ func (p *SSIDividendProvider) normalizeEvent(raw ssiDividendEvent, requestedSymb return event, true } -// SSI normally supplies publicDate, the correct cursor timestamp. Older rows -// can omit it, so ex-right, record, then issue/payment date are used as a -// deterministic fallback. Any supplied but malformed date invalidates the row. +// parseSSIDividendDates parses an SSI row's dates. SSI normally supplies +// publicDate, which becomes PublishedAt. Older rows can omit it, so ex-right, +// record, then issue/payment date are used as a deterministic fallback. Any +// supplied but malformed date invalidates the row. func parseSSIDividendDates(raw ssiDividendEvent) (publishedAt, exDate, recordDate, paymentDate time.Time, ok bool) { var valid bool if exDate, valid = parseSSIOptionalDate(raw.ExrightDate); !valid { @@ -397,6 +402,8 @@ func positiveWholeNumber(value string) (int64, bool) { return ratio.Num().Int64(), true } +// exactShareRatio reads SSI's ratio as new shares per owned share and returns +// it as an exact owned:new pair in lowest terms, e.g. "0.13" -> 100:13. func exactShareRatio(value string) (owned, newShares int64, ok bool) { ratio, parsed := new(big.Rat).SetString(strings.TrimSpace(value)) if !parsed || ratio.Sign() <= 0 || !ratio.Num().IsInt64() || !ratio.Denom().IsInt64() { diff --git a/internal/modules/stock/dividend_notifications.go b/internal/modules/stock/dividend_notifications.go index d7ac212..4915086 100644 --- a/internal/modules/stock/dividend_notifications.go +++ b/internal/modules/stock/dividend_notifications.go @@ -20,6 +20,8 @@ import ( ) const ( + // dividendFetchTimeout bounds all provider fetches for one /stock_portfolio + // run, independent of the SSI client's per-request timeout. dividendFetchTimeout = 12 * time.Second dividendFetchWorkers = 4 ) @@ -29,6 +31,10 @@ type dividendRef struct { eventID string } +// dividendFetchJob is one provider query. A recent job discovers every event +// in the discovery window for a held ticker; a historical job re-fetches one +// Asia/Saigon publication day to refresh only targetIDs: unprocessed events +// published before that window whose Record date is unknown or already reached. type dividendFetchJob struct { symbol string after time.Time @@ -43,6 +49,8 @@ type dividendCheckResult struct { err error } +// includes drops events outside the job's own range, including the provider's +// day-granularity overlap, and restricts historical jobs to their targets. func (job dividendFetchJob) includes(event DividendEvent) bool { if event.Symbol != job.symbol || event.PublishedAt.Before(job.after) || event.PublishedAt.After(job.through) { return false @@ -54,6 +62,10 @@ func (job dividendFetchJob) includes(event DividendEvent) bool { return wanted } +// notifyDividendEvents syncs dividend history, then posts one message per +// unprocessed event on a held ticker: a notice before Record date, or an +// "Apply dividend" suggestion once it is due. Failed tickers are summarized in +// a single reply instead of failing the portfolio command. func (s *state) notifyDividendEvents(ctx context.Context, b *bot.Bot, msg *models.Message, userID int64, snapshot Portfolio, checkedThrough time.Time) error { if s.pending != nil { s.cleanupExpiredDividends(ctx, checkedThrough.UnixMilli()) @@ -103,6 +115,10 @@ func (s *state) notifyDividendEvents(ctx context.Context, b *bot.Bot, msg *model return nil } +// syncDividendHistory fetches events without the user lock, then reloads the +// portfolio under it to prune expired history and merge the results, so a +// concurrent trade is never overwritten by the pre-fetch snapshot. It returns +// the tickers whose fetch failed. func (s *state) syncDividendHistory(ctx context.Context, userID int64, snapshot Portfolio, now time.Time) ([]string, error) { jobs := dividendFetchJobs(snapshot, now) results := s.fetchDividendJobs(ctx, jobs) @@ -247,6 +263,10 @@ func sortedDividendRefs(p Portfolio) []dividendRef { return refs } +// sendFutureDividendNotice posts a non-actionable notice for an event whose +// Record date has not arrived. Like sendDividendSuggestion, it re-checks the +// event and position under the user lock; expectedOpenedAt skips a position +// that was closed and reopened since the history sync. func (s *state) sendFutureDividendNotice(ctx context.Context, b *bot.Bot, msg *models.Message, userID, expectedOpenedAt int64, ref dividendRef) error { defer s.locks.Acquire(strconv.FormatInt(userID, 10))() @@ -274,6 +294,8 @@ func (s *state) sendFutureDividendNotice(ctx context.Context, b *bot.Bot, msg *m return nil } +// sendDividendSuggestion posts the actionable message and then binds its +// pending action to the sent message ID, so only that message's button works. func (s *state) sendDividendSuggestion(ctx context.Context, b *bot.Bot, msg *models.Message, userID, expectedOpenedAt int64, ref dividendRef) error { defer s.locks.Acquire(strconv.FormatInt(userID, 10))() diff --git a/internal/modules/stock/format.go b/internal/modules/stock/format.go index 9d8741a..eed8e2a 100644 --- a/internal/modules/stock/format.go +++ b/internal/modules/stock/format.go @@ -1,5 +1,3 @@ -// Package stock is a paper-stock module for VN stocks. It keeps a per-user -// live portfolio in KV and prices positions at the current market price. package stock import ( diff --git a/internal/modules/stock/handlers.go b/internal/modules/stock/handlers.go index f05b4bc..ea45021 100644 --- a/internal/modules/stock/handlers.go +++ b/internal/modules/stock/handlers.go @@ -17,9 +17,10 @@ import ( "github.com/tiennm99/miti99bot/internal/modules/util/chathelper" ) -// state is the per-module runtime. store is module-scoped (the framework -// prefixes/partitions). PriceClient is reused across calls; nowFn allows -// tests to inject a deterministic clock for portfolio CreatedAt. +// state is the per-module runtime. store and pending are typed views of the +// module's own collection. prices is reused across calls so its HTTP +// connection pool survives; locks serializes each user's portfolio +// read-modify-write; nowFn lets tests inject a deterministic clock. type state struct { store Store pending PendingDividendStore @@ -389,9 +390,10 @@ func (s *state) handleShareDividend(ctx context.Context, b *bot.Bot, update *mod "\nHolding: "+formatShareQuantity(held)+" → "+formatShareQuantity(finalHolding)) } -// handleStats renders the portfolio first, then synchronizes dividend history -// for each held ticker. Network calls never hold the user lock, while history -// merging and notification state updates reload under it. +// handleStats serves /stock_portfolio. It renders the portfolio first, then +// synchronizes dividend history for each held ticker. Price and SSI dividend +// fetches never hold the user lock; history merging and each dividend message +// reload the portfolio under it. func (s *state) handleStats(ctx context.Context, b *bot.Bot, update *models.Update) error { userID, ok := senderInfo(update) if !ok { @@ -410,7 +412,8 @@ func (s *state) handleStats(ctx context.Context, b *bot.Bot, update *models.Upda totalBasis := 0.0 missingPrice := false - // Filter out zero-balance assets (DeductAsset removes them, but defensive). + // Skip zero-quantity positions. SellTicker deletes a fully sold position and + // Validate rejects them on load, so this is only a defensive guard. type held struct { symbol string qty int64 diff --git a/internal/modules/stock/pending_dividend.go b/internal/modules/stock/pending_dividend.go index af92b0f..2931ead 100644 --- a/internal/modules/stock/pending_dividend.go +++ b/internal/modules/stock/pending_dividend.go @@ -17,6 +17,8 @@ const ( maxDividendCallbackBytes = 64 ) +// PendingDividendStore is the typed view of the module collection that holds +// pending dividend actions under pendingDividendPrefix keys. type PendingDividendStore = storage.DocStore[PendingDividendAction] // PendingDividendAction is the server-side half of an inline button. Financial @@ -77,6 +79,9 @@ func parseDividendCallback(data string) (int64, string, bool) { return ownerID, eventID, true } +// cleanupExpiredDividends deletes pending actions past ExpiresAt. It is +// best-effort: list and delete failures are ignored because every button press +// re-checks expiry anyway. func (s *state) cleanupExpiredDividends(ctx context.Context, now int64) { if s.pending == nil { return diff --git a/internal/modules/stock/portfolio.go b/internal/modules/stock/portfolio.go index 6bc6e0b..1442a13 100644 --- a/internal/modules/stock/portfolio.go +++ b/internal/modules/stock/portfolio.go @@ -10,8 +10,11 @@ import ( "github.com/tiennm99/miti99bot/internal/storage" ) +// Store is the typed view of the module collection that holds portfolios. type Store = storage.DocStore[Portfolio] +// CollectionName is the storage collection shared by portfolios and pending +// dividend actions; the two are told apart by key prefix. const CollectionName = "stock" // AssetPosition keeps the complete persisted state for one open stock ticker. @@ -42,6 +45,9 @@ type DividendRecord struct { Processed bool `json:"processed" bson:"processed"` } +// Portfolio is one user's persisted paper account. Assets holds only open +// positions, and Dividends maps ticker to SSI event ID to retained history, +// which outlives a closed position until it ages out. type Portfolio struct { VND float64 `json:"vnd" bson:"vnd"` Assets map[string]AssetPosition `json:"assets" bson:"assets"` @@ -49,11 +55,14 @@ type Portfolio struct { Meta PortfolioMeta `json:"meta" bson:"meta"` } +// PortfolioMeta tracks account-level totals. Invested is the sum of all +// top-ups and is the baseline for the account-wide P&L. type PortfolioMeta struct { Invested float64 `json:"invested" bson:"invested"` CreatedAt int64 `json:"createdAt" bson:"createdAt"` } +// NewPortfolio returns an empty portfolio created at now (Unix milliseconds). func NewPortfolio(now int64) Portfolio { return Portfolio{ Assets: map[string]AssetPosition{}, @@ -66,6 +75,9 @@ func portfolioKey(userID int64) string { return "user:" + strconv.FormatInt(userID, 10) } +// LoadPortfolio reads userID's portfolio, returning a fresh one when none is +// stored. Nil maps and a missing CreatedAt are filled in before validation, so +// callers can mutate the result directly. func LoadPortfolio(ctx context.Context, store Store, userID int64, now int64) (Portfolio, error) { p, _, err := store.Get(ctx, portfolioKey(userID)) switch { @@ -90,6 +102,9 @@ func LoadPortfolio(ctx context.Context, store Store, userID int64, now int64) (P } } +// SavePortfolio validates p and overwrites userID's stored portfolio. The write +// is unversioned; handlers serialize read-modify-write cycles with the per-user +// lock instead. func SavePortfolio(ctx context.Context, store Store, userID int64, p Portfolio) error { if err := p.Validate(); err != nil { return fmt.Errorf("stock: save portfolio %d: %w", userID, err) @@ -100,6 +115,9 @@ func SavePortfolio(ctx context.Context, store Store, userID int64, p Portfolio) return nil } +// Validate rejects a portfolio that must never be persisted: a negative or +// non-finite balance, non-canonical tickers, empty or corrupt positions, and +// malformed dividend records. func (p Portfolio) Validate() error { if math.IsNaN(p.VND) || math.IsInf(p.VND, 0) || p.VND < 0 { return fmt.Errorf("stock: invalid VND balance") @@ -131,6 +149,8 @@ func (p *Portfolio) AddVND(amount float64) { p.VND += amount } +// DeductVND debits amount when the balance covers it. On failure the balance +// is left unchanged and returned so the caller can report the shortfall. func (p *Portfolio) DeductVND(amount float64) (ok bool, balance float64) { if p.VND < amount { return false, p.VND diff --git a/internal/modules/stock/prices.go b/internal/modules/stock/prices.go index c1fd4fc..2de9194 100644 --- a/internal/modules/stock/prices.go +++ b/internal/modules/stock/prices.go @@ -10,17 +10,20 @@ import ( "time" ) -// stockPriceHTTPTimeout caps a stock quote request. Kept under the handler -// deadline so a slow upstream cannot starve the Telegram reply budget. +// stockPriceHTTPTimeout caps each quote request made by the default HTTP +// client. FetchPrice and FetchPrices try providers in sequence, so the bound is +// per provider attempt; a slow upstream fails over instead of stalling the reply. const stockPriceHTTPTimeout = 3 * time.Second const stockBrowserUserAgent = "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 Chrome/120.0.0.0 Safari/537.36" -// PriceClient fetches VN stock quotes. Zero value uses KBS current price-board -// quotes first, then VCI current quotes, then SSI direct quotes. +// PriceClient fetches VN stock quotes. The zero value tries KBS current +// price-board quotes first, then VCI current quotes, then SSI direct quotes. +// Setting URL moves SSI to the front and keeps KBS or VCI as a fallback only +// when its own URL is also set. type PriceClient struct { HTTP *http.Client - URL string // SSI direct quote endpoint base override. + URL string // SSI direct quote endpoint base override; see the ordering above. KBSURL string // KBS current quote endpoint override. VCIURL string // VCI current quote endpoint override. @@ -40,8 +43,9 @@ func (c *PriceClient) httpClient() *http.Client { return c.defaultClient } -// FetchPrice returns the current VND price for ticker, or ErrNoPrice if all -// configured providers return no usable quote. +// FetchPrice returns the current VND price for ticker from the first provider +// that yields a usable quote. The error wraps ErrNoPrice only when every +// attempted provider reported no price; transport or decode failures do not. func (c *PriceClient) FetchPrice(ctx context.Context, ticker string) (float64, error) { ticker = strings.ToUpper(strings.TrimSpace(ticker)) if ticker == "" { @@ -80,9 +84,10 @@ func (c *PriceClient) FetchPrice(ctx context.Context, ticker string) (float64, e return 0, combineProviderErrors(ticker, errs...) } -// FetchPrices returns current prices for the requested tickers. Missing or -// invalid quotes are omitted from the returned map; callers can degrade those -// symbols individually. +// FetchPrices returns current prices for the requested tickers from the first +// provider that yields at least one usable quote. Missing or invalid quotes are +// omitted from the returned map rather than retried on another provider; +// callers degrade those symbols individually. func (c *PriceClient) FetchPrices(ctx context.Context, tickers []string) (map[string]float64, error) { requested := normalizeTickers(tickers) if len(requested) == 0 { @@ -127,6 +132,7 @@ func (c *PriceClient) FetchPrices(ctx context.Context, tickers []string) (map[st return nil, combineProviderErrors(strings.Join(requested, ","), errs...) } +// ssiFirst reports whether an explicit SSI URL puts SSI ahead of KBS and VCI. func (c *PriceClient) ssiFirst() bool { return strings.TrimSpace(c.URL) != "" } @@ -145,11 +151,14 @@ func normalizeTickers(tickers []string) []string { // ErrNoPrice means no provider returned a usable price for the ticker. var ErrNoPrice = errors.New("stock: no price available") +// providerError records one failed provider attempt for combineProviderErrors. type providerError struct { name string err error } +// combineProviderErrors joins every attempt into one message, wrapping +// ErrNoPrice only when all attempts were no-price outcomes. func combineProviderErrors(ticker string, errs ...providerError) error { allNoPrice := true parts := make([]string, 0, len(errs)) diff --git a/internal/modules/stock/prices_kbs.go b/internal/modules/stock/prices_kbs.go index 235d5be..c5b6973 100644 --- a/internal/modules/stock/prices_kbs.go +++ b/internal/modules/stock/prices_kbs.go @@ -21,6 +21,8 @@ type kbsQuote struct { Price float64 `json:"CP"` } +// kbsFallbackEnabled reports whether FetchPrice and FetchPrices try KBS: always +// in the default order, and only with an explicit KBSURL once SSI goes first. func (c *PriceClient) kbsFallbackEnabled() bool { return strings.TrimSpace(c.KBSURL) != "" || !c.ssiFirst() } diff --git a/internal/modules/stock/prices_vci.go b/internal/modules/stock/prices_vci.go index 255b537..cbde1cd 100644 --- a/internal/modules/stock/prices_vci.go +++ b/internal/modules/stock/prices_vci.go @@ -25,6 +25,8 @@ type vciQuote struct { } `json:"matchPrice"` } +// vciFallbackEnabled reports whether FetchPrice and FetchPrices try VCI: always +// in the default order, and only with an explicit VCIURL once SSI goes first. func (c *PriceClient) vciFallbackEnabled() bool { return strings.TrimSpace(c.VCIURL) != "" || !c.ssiFirst() } diff --git a/internal/modules/stock/stock.go b/internal/modules/stock/stock.go index 6588fa6..cb2f7c4 100644 --- a/internal/modules/stock/stock.go +++ b/internal/modules/stock/stock.go @@ -1,3 +1,14 @@ +// Package stock is a paper-trading module for Vietnamese stocks. Each Telegram +// user gets a virtual VND account: /stock_topup adds cash, /stock_buy and +// /stock_sell trade at the current market price, and /stock_portfolio shows +// positions with P&L. Prices come from KBS, then VCI, then SSI iBoard (see +// PriceClient); /stock_info and /stock_events read SSI directly. +// +// Each portfolio is one document keyed "user:" in the module's collection +// (MongoDB or in-memory). It also retains recent SSI dividend events: +// /stock_portfolio syncs them for held tickers and offers each one that is due +// behind an inline "Apply dividend" button, while /stock_cash_dividend and +// /stock_share_dividend record a dividend manually. package stock import ( @@ -5,7 +16,8 @@ import ( "github.com/tiennm99/miti99bot/internal/storage" ) -// New is the stock module Factory. Nine user-facing commands. +// New is the stock module Factory. It registers nine public commands plus the +// callback handler behind the "Apply dividend" button. func New(deps modules.Deps) modules.Module { s := newState( storage.Typed[Portfolio](deps.Store), diff --git a/internal/modules/stock/symbols.go b/internal/modules/stock/symbols.go index 9520cac..eb1ab2c 100644 --- a/internal/modules/stock/symbols.go +++ b/internal/modules/stock/symbols.go @@ -13,8 +13,9 @@ var tickerRe = regexp.MustCompile(`^[A-Z0-9]{1,16}$`) // ErrUnknownTicker means the user input is not a valid stock ticker shape. var ErrUnknownTicker = errors.New("stock: unknown ticker") -// The empty-input case returns ErrUnknownTicker to keep the caller's branch -// shape simple (one error path covers both empty + unknown). +// normalizeStockSymbol trims and upper-cases ticker and checks it against +// tickerRe. Empty input also returns ErrUnknownTicker, so callers need only one +// error path for both empty and malformed tickers. func normalizeStockSymbol(ticker string) (string, error) { ticker = strings.ToUpper(strings.TrimSpace(ticker)) if !tickerRe.MatchString(ticker) { diff --git a/internal/modules/util/chathelper/chathelper.go b/internal/modules/util/chathelper/chathelper.go index bffe9e9..d1679c4 100644 --- a/internal/modules/util/chathelper/chathelper.go +++ b/internal/modules/util/chathelper/chathelper.go @@ -1,7 +1,6 @@ -// Package chathelper consolidates per-module Telegram helpers (SubjectFor, -// ArgAfterCommand, NowMillis, Reply, ReplyHTML, WinRate) that would -// otherwise be duplicated across every module. Single source here; modules -// import. +// Package chathelper holds the small Telegram helpers shared by every module: +// state scoping (SubjectFor), argument parsing, topic-preserving replies and +// edits, fetch-deadline budgeting, and text formatting. package chathelper import ( @@ -91,19 +90,22 @@ func ArgAfterCommand(text string) string { // NowMillis returns current UTC ms-since-epoch. func NowMillis() int64 { return time.Now().UTC().UnixMilli() } -// replyReserve is the slice of the handler's deadline kept aside for delivering -// the Telegram reply. The whole update handler runs under one bounded context -// (see internal/telegram/webhook.go); if upstream price fetches consume all of -// it, the final SendMessage fails with "context deadline exceeded" and the user -// sees no response. Reserving a fixed tail guarantees delivery headroom. +// replyReserve is the slice of a caller's deadline kept aside for delivering +// the Telegram reply. When the context carries a deadline (for example a +// cron's per-fire timeout) and upstream fetches consume all of it, the final +// SendMessage fails with "context deadline exceeded" and the user sees no +// response; reserving a fixed tail keeps delivery headroom. Update handlers +// under long polling currently receive a context with no deadline, so there +// is nothing to reserve on that path. const replyReserve = 3 * time.Second // FetchContext derives a child of ctx for upstream data fetches, leaving // replyReserve of the parent's deadline for the subsequent Reply (which must be -// called with the original ctx, not this child). If ctx has no deadline, or -// less than replyReserve remains, the child gets a small positive floor so a -// fetch still attempts rather than failing instantly. Callers must call the -// returned cancel. +// called with the original ctx, not this child). If ctx has no deadline, the +// child is a plain cancelable context with no deadline of its own — the usual +// case for update handlers today. If less than replyReserve (plus a second) +// remains, the child gets a one-second floor so a fetch still attempts rather +// than failing instantly. Callers must call the returned cancel. func FetchContext(ctx context.Context) (context.Context, context.CancelFunc) { dl, ok := ctx.Deadline() if !ok { diff --git a/internal/modules/util/info.go b/internal/modules/util/info.go index 07fc2d9..a532fc5 100644 --- a/internal/modules/util/info.go +++ b/internal/modules/util/info.go @@ -20,7 +20,7 @@ func infoCommand() modules.Command { // routing IDs — chat id, thread id, sender id. Useful for admins // debugging group/topic routing; not something every group member // should be able to enumerate. Non-admins see no response at all - // (Visibility denies are silent — see dispatcher.go:31). + // (Visibility denies are silent — see modules.Auth.Permits). Visibility: modules.VisibilityProtected, Description: "Show chat id, thread id, and sender id (debug helper)", Handler: func(ctx context.Context, b *bot.Bot, update *models.Update) error { diff --git a/internal/modules/util/util.go b/internal/modules/util/util.go index a1ee669..97d51c2 100644 --- a/internal/modules/util/util.go +++ b/internal/modules/util/util.go @@ -7,8 +7,8 @@ import ( "github.com/tiennm99/miti99bot/internal/modules" ) -// New is the module Factory. Closes over Deps so each handler has access to -// the registry (for /help) and to the bot framework (for sending replies). +// New is the module Factory. /help closes over deps.Registry so it renders +// the fully built registry at call time; the other handlers need no Deps. func New(deps modules.Deps) modules.Module { return modules.Module{ Commands: []modules.Command{ diff --git a/internal/modules/wordle/compare.go b/internal/modules/wordle/compare.go index 1472312..de2f3bd 100644 --- a/internal/modules/wordle/compare.go +++ b/internal/modules/wordle/compare.go @@ -6,17 +6,17 @@ package wordle // without magic numbers. const WordLength = 5 -// LetterResult labels a single guessed letter's state. Values are part of -// the stored game's JSON shape: "correct" | "partial" | "wrong". +// Letter result categories stored in LetterScore.Result. The values are part +// of the stored game document's shape, so changing one orphans saved rounds. const ( ResultCorrect = "correct" ResultPartial = "partial" ResultWrong = "wrong" ) -// LetterScore is the shape stored per guess (nested in GameState). bson tags -// mirror the json names so migrated docs (which keep the original JSON keys) -// read back verbatim. +// LetterScore is one scored letter, stored per guess (nested in GameState). +// bson tags mirror the json names so the document's field names are the same +// in either encoding. type LetterScore struct { Letter string `json:"letter" bson:"letter"` Result string `json:"result" bson:"result"` diff --git a/internal/modules/wordle/handlers_test.go b/internal/modules/wordle/handlers_test.go index 1b61998..9ff5326 100644 --- a/internal/modules/wordle/handlers_test.go +++ b/internal/modules/wordle/handlers_test.go @@ -166,7 +166,7 @@ func TestWordleStats_AfterWin(t *testing.T) { } } -// Group chats key by chat id, so two private users both writing /wordle +// Group chats key by chat id, so two different users both writing /wordle // in the same group must mutate the same game. func TestWordle_GroupSubjectIsChatID(t *testing.T) { rb, games := installWordle(t, 0, "-100", "crane") diff --git a/internal/modules/wordle/lookup.go b/internal/modules/wordle/lookup.go index 7decc47..e1daad1 100644 --- a/internal/modules/wordle/lookup.go +++ b/internal/modules/wordle/lookup.go @@ -15,9 +15,8 @@ func normalizeWord(input string) string { return string(out) } -// rejectReason classifies why validateGuess returned not-ok. The user-facing -// reply mapping in handlers branches on these values, so renaming a constant -// here requires updating that mapping. +// rejectReason classifies why validateGuess returned not-ok. rejectMessage in +// handlers.go maps each reason to its user-facing reply. type rejectReason string const ( diff --git a/internal/modules/wordle/lookup_test.go b/internal/modules/wordle/lookup_test.go index 75a0653..87a6faa 100644 --- a/internal/modules/wordle/lookup_test.go +++ b/internal/modules/wordle/lookup_test.go @@ -9,7 +9,7 @@ func TestNormalizeWord(t *testing.T) { "CRANE": "crane", " crane ": "crane", "c-r-a-n-e": "crane", - "héllo": "hllo", // strips non a-z (including the é and accented o-equivalent) + "héllo": "hllo", // strips non a-z bytes, including both UTF-8 bytes of é "!@#$%": "", "42 crane": "crane", } diff --git a/internal/modules/wordle/pick_random.go b/internal/modules/wordle/pick_random.go index 1f514b3..0bf84b5 100644 --- a/internal/modules/wordle/pick_random.go +++ b/internal/modules/wordle/pick_random.go @@ -5,12 +5,12 @@ import ( "math/rand" ) -// errEmptyWordList is returned by pickers when the dictionary is empty — -// callers (Init) should fail fast rather than spin a game with no answers. +// errEmptyWordList is returned by pickRandom when the dictionary is empty. +// startFresh propagates it so the handler fails instead of saving a round +// with no answer. var errEmptyWordList = errors.New("wordle: word list is empty") -// pickRandom is the picker handlers actually use today. Uniform random pick. -// rng allows tests to inject a deterministic source. When rng is nil we fall +// pickRandom returns a uniformly random word. rng allows tests to inject a deterministic source. When rng is nil we fall // through to math/rand's package-level Intn, which IS goroutine-safe via an // internal mutex on the global Source — important because the bot dispatcher // runs each Telegram update in its own goroutine and concurrent /wordle_new diff --git a/internal/modules/wordle/state.go b/internal/modules/wordle/state.go index 0e27b6d..b5be2d5 100644 --- a/internal/modules/wordle/state.go +++ b/internal/modules/wordle/state.go @@ -25,7 +25,7 @@ type GuessRecord struct { // GameState is the per-subject record for an in-progress (or finished) round. // -// `giveup` is always emitted (initialized to false on /wordle_new). Do NOT +// `giveup` is always emitted (initialized to false on every fresh round). Do NOT // add omitempty — the field is part of the stored document's shape, so // emitting it unconditionally keeps already-saved games self-describing // when inspected via raw dumps. @@ -34,7 +34,7 @@ type GameState struct { Guesses []GuessRecord `json:"guesses" bson:"guesses"` Solved bool `json:"solved" bson:"solved"` Giveup bool `json:"giveup" bson:"giveup"` - StartedAt int64 `json:"startedAt" bson:"startedAt"` // ms-since-epoch (Date.now()) + StartedAt int64 `json:"startedAt" bson:"startedAt"` // ms-since-epoch (chathelper.NowMillis) } // Stats is the lifetime score record. lastResultAt is *int64 so an unplayed diff --git a/internal/modules/wordle/words.go b/internal/modules/wordle/words.go index 307bddb..9a710ac 100644 --- a/internal/modules/wordle/words.go +++ b/internal/modules/wordle/words.go @@ -12,8 +12,8 @@ import ( var rawWords string // loadWords parses the embedded list into a slice plus a membership set. Both -// outputs share the same backing strings, so memory is roughly the dict size -// (≈90 KiB) — well under the binary-size budget. +// outputs share the same backing strings, so the word bytes (≈90 KiB) are +// held once rather than duplicated. // // Words are validated to be exactly WordLength a-z; any malformed line panics // at startup so a bad regen of the data file is caught immediately, not on diff --git a/internal/server/log_middleware.go b/internal/server/log_middleware.go index 153fe59..31f8189 100644 --- a/internal/server/log_middleware.go +++ b/internal/server/log_middleware.go @@ -33,15 +33,14 @@ func (r *statusRecorder) effectiveStatus() int { // LogRequests wraps an http.Handler with a request log line: // -// {"msg":"req","method":"POST","path":"/webhook","status":200,"ms":12} +// {"msg":"req","method":"GET","path":"/","status":200,"ms":0} // -// CloudWatch Logs filters on `jsonPayload.msg=req AND jsonPayload.status>=500` -// for 5xx-rate alerting — keep the field names stable or the alarm goes dark. +// Keep the field names stable: log-based dashboards and alerts may filter on +// msg=req and status>=500. // // The req line is emitted from a deferred closure so a panic in a downstream -// handler still produces an observable log entry — without this, a cron -// panic would disappear silently (http.Server does its own recover but never -// runs middleware again on the way out). +// handler still produces an observable log entry — http.Server's own recover +// only logs to stderr and never runs middleware again on the way out. func LogRequests(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { start := time.Now() @@ -59,10 +58,10 @@ func LogRequests(next http.Handler) http.Handler { }) } -// recoverPanicStatus folds a recovered panic into the status to log: returns -// 500 if a panic was recovered (and re-panics nothing — http.Server will -// terminate the connection cleanly while the deferred req log still runs), -// otherwise returns the original status untouched. +// recoverPanicStatus folds a recovered panic into the status to log: it +// returns 500 if a panic was recovered, otherwise the original status +// untouched. The panic is logged with its stack and absorbed, not re-raised, +// so the deferred req line always runs. // // Re-panicking would lose the deferred log line in some recover-order edge // cases; absorbing the panic here matches the webhook handler's posture of diff --git a/internal/server/router.go b/internal/server/router.go index 2a77172..2505309 100644 --- a/internal/server/router.go +++ b/internal/server/router.go @@ -1,3 +1,6 @@ +// Package server is the bot's HTTP surface: a single health route for the +// container monitor, wrapped in structured request logging. Telegram updates +// and crons never arrive over HTTP. package server import "net/http" diff --git a/internal/storage/doc_store.go b/internal/storage/doc_store.go index e10c383..b10e8fd 100644 --- a/internal/storage/doc_store.go +++ b/internal/storage/doc_store.go @@ -1,3 +1,8 @@ +// Package storage is the bot's document store. Each module gets its own +// Collection from a Provider — one MongoDB collection per module in +// production, or an in-process map for tests and local runs — and builds typed +// DocStore views over it with Typed. Values are stored as native documents +// with an optimistic-locking version per key. package storage import ( @@ -86,8 +91,8 @@ type Collection interface { // point that binds a Collection to its backend's typed store. // // It panics if T is a struct whose BSON field names collide with a reserved root -// field (_id, version, updatedAt) — a programmer error caught at startup, in the -// same spirit as Prefixed panicking on an empty prefix. +// field (_id, version, updatedAt) — a programmer error, so it fails loudly at +// startup rather than producing duplicate document keys at write time. func Typed[T any](c Collection) DocStore[T] { if err := checkReservedFields[T](); err != nil { panic(err) diff --git a/internal/systemstate/systemstate.go b/internal/systemstate/systemstate.go index 6ecc482..f2c17fd 100644 --- a/internal/systemstate/systemstate.go +++ b/internal/systemstate/systemstate.go @@ -1,3 +1,6 @@ +// Package systemstate stores app-level process metadata — chiefly completion +// markers for one-time startup tasks such as data migrations — in a shared +// "system" collection that belongs to no feature module. package systemstate import ( @@ -22,14 +25,19 @@ type Record struct { UpdatedAt int64 `json:"updated_at" bson:"updated_at"` } +// Store is a typed view over the system collection, keyed by a stable, +// caller-chosen string per task. type Store struct { docs storage.DocStore[Record] } +// New returns a Store over coll, normally the collection named CollectionName. func New(coll storage.Collection) Store { return Store{docs: storage.Typed[Record](coll)} } +// Get returns the record stored under key. A missing key is not an error: it +// reports false with a zero Record. func (s Store) Get(ctx context.Context, key string) (Record, bool, error) { rec, _, err := s.docs.Get(ctx, key) if err != nil { @@ -41,6 +49,7 @@ func (s Store) Get(ctx context.Context, key string) (Record, bool, error) { return rec, true, nil } +// Put overwrites the record stored under key. func (s Store) Put(ctx context.Context, key string, rec Record) error { return s.docs.Put(ctx, key, rec) } diff --git a/internal/telegram/client.go b/internal/telegram/client.go index 8f4dbc0..e5d4910 100644 --- a/internal/telegram/client.go +++ b/internal/telegram/client.go @@ -1,3 +1,6 @@ +// Package telegram constructs the go-telegram bot client used for long +// polling and holds the few raw Bot API calls the library does not make +// reliably (see DeleteWebhook). package telegram import ( @@ -20,10 +23,11 @@ var pollingAllowedUpdates = bot.AllowedUpdates{"message", "callback_query", "inl // // - WithSkipGetMe: avoid a blocking GetMe call at startup. Token validity // surfaces on the first outgoing API call instead. -// - WithNotAsyncHandlers: handlers run synchronously inside the dispatch -// goroutine. Module handlers take their own ctx (not r.Context()), so this -// is safe; it also bounds in-flight work to one update at a time, which -// suits the single-replica polling deployment. +// - WithNotAsyncHandlers: handlers run synchronously on the library's single +// update worker instead of one goroutine per update. That bounds in-flight +// work to one update at a time, which suits the single-replica polling +// deployment; the cost is that a slow handler delays every update queued +// behind it. // - WithAllowedUpdates: only request the update kinds the bot handles. // // Callers may pass extra options that override these defaults. diff --git a/internal/telegram/webhook_test.go b/internal/telegram/webhook_test.go index 929316c..d79494e 100644 --- a/internal/telegram/webhook_test.go +++ b/internal/telegram/webhook_test.go @@ -27,7 +27,7 @@ func TestDeleteWebhookAt_OK(t *testing.T) { func TestDeleteWebhookAt_EmptyBody(t *testing.T) { // Reproduces the failing environment: 200 with an empty body. Must surface - // as an error so the caller retries / warns rather than assuming success. + // as an error so the caller warns rather than assuming success. srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) // no body })) diff --git a/internal/testutil/recording_bot.go b/internal/testutil/recording_bot.go index f4767bd..3665c12 100644 --- a/internal/testutil/recording_bot.go +++ b/internal/testutil/recording_bot.go @@ -129,7 +129,7 @@ func (rb *RecordingBot) StubMethod(method string, resultJSON string) { // (bot.ErrorBadRequest for 400, bot.ErrorForbidden for 403, and so on). // // This is the difference from FailMethod: the library switches on the -// error_code *in the response body* (raw_request.go:103-125), not the HTTP +// error_code *in the response body* (raw_request.go), not the HTTP // status, so a codeless failure never takes a sentinel shape. Handlers that // classify errors with errors.Is must be tested through this method. // @@ -172,15 +172,6 @@ func (rb *RecordingBot) FailMethod(method string, status int, body string) { func (rb *RecordingBot) handle(w http.ResponseWriter, r *http.Request) { method := apiMethodFromPath(r.URL.Path) - // Parameterless methods (getMe) send no body at all, so a parse failure is - // not an error there — it just means there are no form fields to record. - // Failing the request would make those methods untestable no matter what - // the test registered. - // - // That tolerance is scoped to requests that carry no multipart body. A - // request that claims to be multipart and then fails to parse is a real - // fault, and answering it 200 with an empty Form would quietly satisfy - // every test that asserts a field is *absent*. // Read the body before parsing, because an empty body and a corrupt one are // otherwise indistinguishable: multipart reports both as "no parts". // @@ -261,8 +252,8 @@ func apiMethodFromPath(p string) string { } // okResponseFor returns a minimal `{ok:true, result:...}` payload that the -// bot library will accept for the named API method. SendMessage / SendSticker -// expect a Message; most others accept a bool. +// bot library will accept for the named API method. Message-producing methods +// (see isMessageProducingMethod) expect a Message; most others accept a bool. func okResponseFor(method string, messageID int) string { if isMessageProducingMethod(method) { // Minimal shape: id, date, chat. Bot library decodes via json so diff --git a/internal/testutil/update_builders.go b/internal/testutil/update_builders.go index 49510d2..fe9060e 100644 --- a/internal/testutil/update_builders.go +++ b/internal/testutil/update_builders.go @@ -70,8 +70,8 @@ func NewChannelMessage(chatID int64, text string) *models.Update { } // botCommandEntity is what Telegram attaches when a message starts with `/`. -// The dispatcher's MatchTypeCommand uses it to extract the command name, so -// every fixture command-bearing message must include one. +// The dispatcher's matchCommand reads the command name from it, so every +// fixture command-bearing message must include one. func botCommandEntity(text string) models.MessageEntity { if len(text) == 0 || text[0] != '/' { return models.MessageEntity{Type: models.MessageEntityTypeBotCommand}