From 145a7261f5ae4657c4f4e3ef617844c2ca8c9d43 Mon Sep 17 00:00:00 2001 From: tiennm99 Date: Sat, 16 May 2026 13:35:11 +0700 Subject: [PATCH] =?UTF-8?q?refactor(plans):=20code-review=20audit=20report?= =?UTF-8?q?s=20=E2=80=94=20modules=20+=20infra?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Captures the two parallel code-reviewer reports that drove the audit-driven hygiene pass in the previous commit. Kept for future reviewers and as the trail of which findings were applied vs. deferred as cosmetic. --- ...-reviewer-infra-260516-1231-infra-audit.md | 96 +++++++++++++++++++ ...iewer-modules-260516-1232-modules-audit.md | 89 +++++++++++++++++ 2 files changed, 185 insertions(+) create mode 100644 plans/reports/code-reviewer-infra-260516-1231-infra-audit.md create mode 100644 plans/reports/code-reviewer-modules-260516-1232-modules-audit.md diff --git a/plans/reports/code-reviewer-infra-260516-1231-infra-audit.md b/plans/reports/code-reviewer-infra-260516-1231-infra-audit.md new file mode 100644 index 0000000..96ed7f3 --- /dev/null +++ b/plans/reports/code-reviewer-infra-260516-1231-infra-audit.md @@ -0,0 +1,96 @@ +# Infra audit — 260516-1231 + +## Summary +- 14 findings: 12 safe, 2 I/O-changing +- Overall health: solid. Hot paths (webhook, cron, KV) are well-bounded with timeouts, constant-time secret checks, panic recovery, and strong reads. The biggest themes are (a) log-middleware loses the `req` line on panics and (b) a few error-handling/cleanup gaps in the migration CLI. No data-loss or auth-bypass bugs spotted in the in-scope code. + +## Safe findings (behavior-preserving — assistant will apply) + +- **[high] `LogRequests` drops `req` line on downstream panic** — `internal/server/log_middleware.go:39-51` + - What: `next.ServeHTTP` is called inline; if it panics, the trailing `log.Info("req", …)` never runs. + - Why it matters: 5xx-rate alert filter parses `msg=req status>=500`; panics in cron handlers (no recover at that layer) silently disappear from the dashboard. + - Fix sketch: move the `log.Info("req", …)` into a `defer` and add a `recover()` that re-panics after logging (or sets status=500). Status downstream tooling parses is preserved. + +- **[high] cron handler has no panic recovery** — `internal/server/router.go:56-94` + - What: `modules.DispatchScheduled` runs without a `defer recover()`. Webhook recovers (`telegram/webhook.go:77-83`); cron does not. + - Why it matters: a panic in a cron handler tears down the goroutine, `http.Server` writes nothing, the `req` line above is lost, and EventBridge sees a connection reset → SQS DLQ entry without a useful log. + - Fix sketch: mirror the webhook's `func(){ defer recover() … }()` block around the dispatch call. Log panic+stack, return 500. + +- **[high] `kv-import` skips already-imported keys without explicit guard once a partition has any item** — `internal/migration/dynamodb_writer.go:39-61` (verify) + - What: I initially flagged `attribute_not_exists(pk)` as wrong because pk is shared per module. **Re-verified against AWS docs**: DynamoDB evaluates `ConditionExpression` against the specific item identified by the request's `Key` (pk, sk). The condition is correct as written. + - Why it matters: no bug; documenting the verification so a future reviewer doesn't "fix" it. + - Fix sketch: add a one-liner comment near the condition stating "evaluated per (pk,sk) item, not partition-wide". + +- **[medium] `runTradingAuditDump` leaks `f.Close` error and silently writes incomplete files** — `cmd/migrate_cf_data/main.go:194-206` + - What: `defer func(){ _ = f.Close() }()` ignores Close's error; if disk fills mid-write `enc.Encode` returns but Close's flush error never surfaces. + - Why it matters: audit dumps are evidence; an aborted Close yields a truncated JSONL the operator believes is complete. + - Fix sketch: shadow `err` in a deferred closer, return `errors.Join(returnedErr, closeErr)`. + +- **[medium] `logDispatch` slices mid-UTF-8 codepoint** — `internal/telegram/webhook.go:106-108` + - What: `text[:dispatchTextPreview]` cuts at byte 64; Vietnamese / emoji messages produce invalid UTF-8 in the `text` log attr. + - Why it matters: log line still ships (slog escapes), but the preview is unreadable garbage for the most common bot audience. + - Fix sketch: walk back to the previous rune boundary; `utf8.RuneStart` check or `utf8.DecodeLastRuneInString` until valid. + +- **[medium] webhook panic-recover may double-WriteHeader** — `internal/telegram/webhook.go:76-86` + - What: after `recover()`, handler writes 200 unconditionally. If the panicking handler already called `w.WriteHeader`/`w.Write`, the second `WriteHeader(200)` logs a `superfluous response.WriteHeader` warning. + - Why it matters: log noise + the actual response code Telegram sees may not be 200. + - Fix sketch: gate the trailing `w.WriteHeader(http.StatusOK)` behind a `statusRecorder`-style check (set a flag inside recover), OR document that handlers must not touch the http response. + +- **[medium] `PerUserLimiter` map grows unbounded per Lambda instance** — `internal/ai/ratelimit.go:19-53` + - What: `buckets[subject]` is never evicted. Distinct user IDs across the lifetime of a Lambda instance accumulate `*rate.Limiter` entries (~120 B each). + - Why it matters: Lambda recycles long before this matters in practice (similar logic to `keylock`'s memo), but it's worth a one-line comment matching the keylock rationale so a future reviewer doesn't re-flag it. + - Fix sketch: copy the eviction-deferred rationale from `keylock/keylock.go` package comment into ratelimit.go. + +- **[medium] migration CLI uses `context.Background()` with no SIGTERM handling** — `cmd/migrate_cf_data/main.go:89,131,189` & `cmd/migrate_cf_data/convert_value.go:31` + - What: every subcommand runs against `context.Background()`. Operator Ctrl-C kills mid-paginate; a partial Scan in `convert-value-to-string` leaves the table in a half-converted state visible to the runtime. + - Why it matters: idempotent on retry, but the operator has no clean-stop affordance. + - Fix sketch: `signal.NotifyContext(context.Background(), SIGINT, SIGTERM)` at top of each runner. + +- **[low] `subtle.ConstantTimeCompare` leaks length** — `internal/server/router.go:69`, `internal/telegram/webhook.go:49` + - What: when lengths differ, function returns 0 immediately without scanning bytes. Length itself is observable via timing. + - Why it matters: secrets are fixed-length (SSM-managed), so length is not actually attacker-controlled. Not exploitable; noting it so the docstring around constant-time checks reflects reality. + - Fix sketch: no code change; one comment line at the call site clarifies "constant-time within same-length inputs; length is a constant of the deploy". + +- **[low] `DynamoDBKVStore.List` returns `nil` (not empty slice) when no items match** — `internal/storage/dynamodb_kv.go:139-177` + - What: declared `var keys []string`; never initialized to `[]string{}`. Empty result is nil. + - Why it matters: `FirestoreKVStore.List` (firestore_kv.go:184) has the same nil-return shape, so it's consistent. But callers that do `if keys == nil` get the same as `len == 0`, so semantics are fine; flag only because callers using `keys[:]` on a nil get a nil — usually fine in Go but worth a doc line in `KVStore.List` saying "may return nil; len(keys)==0 is the canonical empty check". + - Fix sketch: docstring tweak on `KVStore.List` in `kv_store.go:13-20`. + +- **[low] `Report.Format` ignores writer errors** — `internal/migration/report.go:41-66` + - What: every `fmt.Fprintln`/`fmt.Fprintf` return is discarded with `_`. + - Why it matters: comment at line 39-40 says "callers pass os.Stdout or *bytes.Buffer" — for stdout, a broken-pipe (operator piped to `head`) silently swallows the rest of the report. + - Fix sketch: short-circuit on first write error; return that error from `Format`. Caller in `main.go` can `os.Exit(1)` on report-write failure. + +- **[low] `prefixedStore.List` allocates `make([]string, len(keys))` even when `keys == nil`** — `internal/storage/prefix.go:45-55` + - What: harmless (`make([]string, 0)` returns non-nil empty slice). Diverges from raw store return shape (nil vs empty). Tests pass; just an inconsistency. + - Why it matters: cosmetic; flag only if you want backend-vs-prefixed parity. + - Fix sketch: early-return `nil, nil` when inner returned `nil, nil`. + +## I/O-changing suggestions (need user approval — DO NOT apply) + +- **[medium] `cmd/server/main.go` writes `WriteTimeout: 6 * time.Minute` while `template.yaml` sets Lambda Timeout=30s** — `cmd/server/main.go:141` + - What: HTTP server allows 6-minute writes; Lambda kills the container at 30s. + - Why it matters: under Lambda this never fires; under non-Lambda (local server) it allows 6-min slow-loris writes. Comment says "6 min accommodates /cron/{name}" but cron is already capped at 60s in `internal/server/timeouts.go:9`. + - Fix sketch: align HTTP `WriteTimeout` with the cron timeout (~75s) + slack. + - User/operator impact: changes the server's slow-client behavior on the non-Lambda path; doesn't affect prod Lambda. Worth confirming before tightening. + +- **[medium] webhook 413 / 400 / 401 responses leak internal error strings to Telegram** — `internal/telegram/webhook.go:45,50,62,65` + - What: `http.Error(w, "method not allowed", ...)`, `"unauthorized"`, etc. Telegram retries on non-2xx, so these strings are not user-visible. **But** Lambda Function URL is `AuthType: NONE` per `template.yaml:129` — anyone on the internet can probe `/webhook`. Strings reveal "this is a real Telegram bot endpoint." + - Why it matters: trivial fingerprinting, not a real risk. Mention only because deeper hardening (always return 200 with empty body on auth fail, log the rejection) would reduce noise from random scanners. + - Fix sketch: collapse all rejection paths to a single bare 401 with empty body. + - User/operator impact: changes response shape ops dashboards may rely on; the 413 distinction in particular is documented as deliberate ("Telegram + ops dashboards can distinguish body-too-big"). Worth user call. + +## Architecture observations (no immediate change) + +- Storage layer cross-provider value parity (string vs binary) is deliberate per recent commit and exercised by the `convert-value-to-string` migration tool. Validated, not undone. +- `MemoryProvider` shares one underlying `MemoryKVStore` across modules with `Prefixed` isolation; `DynamoDBProvider` uses partition-per-module. Both bypass the prefix layer's empty-prefix panic by validating module name through `collectionNameRe` before construction. Solid. +- `keylock.Map` and `ai.PerUserLimiter` both rely on Lambda recycle frequency to bound memory. Same rationale, different files — worth a one-paragraph note in `docs/system-architecture.md` so future reviewers find the pattern documented once. +- `firestoreInitTimeout` (10s) lives in main.go but the package comment in `internal/storage/firestore_client.go` is silent on cold-start budget. Cohesion is fine; mention only if you ever move this to a constants file. + +## Unresolved questions + +- Should the webhook handler's panic-recover path emit a 5xx so Telegram retries? Current behavior is 200 (per code comment, intentional — "do not trigger 24-hour retry storm"). Verify with the team that the trade-off (drop the poisoned update vs retry forever) still holds for AI handlers where a transient panic may be recoverable. +- `Default` package-level `metrics.Registry` is shared across goroutines; tests reset it via `Default.snapshot()`. Is the global ergonomics still desired, or should `cmd/server/main.go` construct and inject a registry? Current state has implicit shared state that's hard to swap in tests other than the test that owns the metrics package. + +**Status:** DONE +**Summary:** 14 findings on the in-scope infra layer (12 behavior-preserving, 2 needing operator nod). Highest-leverage fixes: deferred req-log in middleware, panic recovery in cron handler, UTF-8-safe truncation in dispatch logging. diff --git a/plans/reports/code-reviewer-modules-260516-1232-modules-audit.md b/plans/reports/code-reviewer-modules-260516-1232-modules-audit.md new file mode 100644 index 0000000..41715ba --- /dev/null +++ b/plans/reports/code-reviewer-modules-260516-1232-modules-audit.md @@ -0,0 +1,89 @@ +# Modules audit — 260516-1232 + +## Summary +- 14 findings: 11 safe, 3 I/O-changing +- Overall health: framework is clean, well-tested, idiomatic. Most modules show consistent KV+keylock discipline. Two real correctness gaps: (1) `lolschedule` subscriber list mutated without a lock, (2) `twentyq` `_ = clearGame` swallows error paths. Several DRY/style nits. + +## Safe findings (behavior-preserving — assistant will apply) + +- **[high] lolschedule subscriber list — unprotected Get→mutate→Put** — internal/modules/lolschedule/subscribers.go:33-48, 52-72 + - What: `addSubscriber`/`removeSubscriber` load → modify slice → save, but lolschedule has no `keylock.Map`. Two concurrent `/lolschedule_subscribe` from different chats lose one write (the second overwrites the first's append). + - Why it matters: silent data loss on every concurrent subscription burst, which is exactly when daily push wiring is most likely (multiple groups subscribing on a launch announcement). + - Fix sketch: add `keylock.Map` to `state`, wrap addSubscriber/removeSubscriber with `defer locks.Acquire(subscribersKey)()`, or take a single module-wide lock since the slot is one global key. + +- **[med] twentyq drops error on auto-clear of solved-but-lingering games** — internal/modules/twentyq/handlers.go:96 + - What: `_ = clearGame(ctx, s.kv, subject)` silently discards KV errors. If the delete fails, every subsequent `/twentyq` call repeats the same failing delete forever (game stays Solved → branch re-enters). + - Why it matters: KV error during recovery turns into permanent module deadlock for the subject; no log trail. + - Fix sketch: log on error; consider returning the error so the user sees `upstreamFail` rather than silently looping. + +- **[med] misc /mstats returns error to dispatcher instead of replying** — internal/modules/misc/misc.go:78-80 + - What: KV read failure path returns wrapped error; dispatcher logs but user sees no reply. + - Why it matters: inconsistent with trading/wordle/loldle where load-failure paths reply with a user-visible hint. Silent timeout on the user side. + - Fix sketch: log + `chathelper.Reply(...,"Could not load stats. Try again later.")`, return nil. + +- **[med] PriceClient builds a fresh `*http.Client` on every call** — internal/modules/trading/prices.go:31-36 + - What: zero-value `PriceClient.httpClient()` returns `&http.Client{Timeout: kbsHTTPTimeout}` on every `FetchPrice`. New client → new Transport → no connection reuse across calls (matters in `handleStats`'s loop over holdings). + - Why it matters: TLS handshake per stock during /trade_stats on cold cache. + - Fix sketch: cache the default client on the receiver (sync.Once) or initialise in `newState`. + +- **[low] `findChampion` recomputes `normalizeName` for every champion on every guess** — internal/modules/loldle/lookup.go:36, 42 + - What: O(N) per guess, each call allocs a new lowercase byte slice for the same names. + - Why it matters: champions.json ≈170 entries × 2 normalize calls × every guess. Tiny but pure waste; a built-once `map[normalized]int` would also simplify the prefix-ambiguity loop. + - Fix sketch: precompute `normalizedNames []string` in `loadChampions` once; lookup runs over precomputed strings. + +- **[low] `RenderHelp.ownerOf` is O(modules × commands) per command** — internal/modules/util/help.go:42, 47, 80 + - What: For each public+protected command, a linear scan over every module's commands. + - Why it matters: cold-path, but DRY: registry already knows the owner (`owners` map in `Build`). Easy to expose `Registry.OwnerOf(name)`. + - Fix sketch: store `owner map[string]string` on Registry; have ownerOf consult it. + +- **[low] loldle/handlers.go does not record a loss on data-refresh round drop** — internal/modules/loldle/handlers.go:144-152 + - What: target gone from champions.json → `clearGame` only, no `recordResult(false)`. Consistent with JS-parity comment but means a guess that hit the dead-target branch consumes nothing. + - Why it matters: arguably correct (not the user's fault), but inconsistent with /loldle_giveup which records loss when target is gone (handlers.go:220-224 records before target lookup). Worth one line of doc. + - Fix sketch: leave behavior, add a code comment cross-referencing /loldle_giveup so the asymmetry is intentional, not an oversight. + +- **[low] `lolschedule.cacheKey` uses unstable RFC3339 with monotonic risk only in test paths** — internal/modules/lolschedule/lolschedule.go:227-229 + - What: not a real bug today (RFC3339 strips monotonic), but `time.Now().UTC().UnixMilli()` vs `cached.Ts` could go negative under clock skew between Lambda instances → unintended cache hit on backwards-clocked data. + - Why it matters: theoretical; on Lambda the clock skew is small. Worth `if diff := now-cached.Ts; diff >= 0 && diff < cacheTTL.Ms()` guard. + - Fix sketch: change comparison to require non-negative age. + +- **[low] `lolschedule.truncate` byte-slices UTF-8** — internal/modules/lolschedule/lolschedule.go:264-269 + - What: `s[:maxLen]` slices bytes; can split a multibyte rune. Logs only, so user-invisible, but log JSON encoders may emit replacement chars. + - Fix sketch: switch to rune-aware slicing (`[]rune(s)[:n]`) since this is cold path. + +- **[low] `prices.go` shadows builtin `close`** — internal/modules/trading/prices.go:103 + - What: `close := body.DataDay[0].C` shadows `close` (builtin for channels). No bug; readability nit. + - Fix sketch: rename to `c` or `lastClose`. + +- **[low] `formatStats` parens math for round-half-up is opaque** — internal/modules/twentyq/render.go:85, 89 + - What: `(s.Solved*200 + s.Played) / (2 * s.Played)` is integer round-half-up of solveRate. Works but is unobvious; tests don't probe boundary (e.g. solved=1, played=3 → solveRate=33; solved=1, played=2 → 50). + - Fix sketch: add a one-line comment naming the formula, or extract `roundPct(num, den int)` helper. + +## I/O-changing suggestions (need user approval — DO NOT apply) + +- **[med] /lolschedule unknown subscribers should be pruned on 403** — internal/modules/lolschedule/cron.go:91-98 + - What: a chat that blocked the bot returns `Forbidden: bot was blocked by the user`. The cron logs and continues but never removes the dead chat from the subscriber list. Forever-retried. + - Why it matters: subscriber list grows monotonically with dead chats; daily push wastes API calls; on Telegram global ratelimit (30 msg/s) one bad apple lengthens the throttled window for everyone. + - User impact: dead subscribers stop receiving anything (already true); user-visible change is auto-removal — a subscribed user who blocks then unblocks the bot would have to /lolschedule_subscribe again to restore. + - Fix sketch: inspect error from SendMessage for 403 / "bot was blocked" / "chat not found", call `removeSubscriber`. Document in the unsubscribe message. + +- **[low] twentyq seed pool is small (51) — fast repeats** — internal/modules/twentyq/seeds.go + - What: per-subject, after ~20 plays a repeat is near-certain. JS-parity, but JS may have been small too. + - User impact: noticeably repetitive games for power users. + - Fix sketch: grow the seed list, or maintain per-subject "recent seeds" exclusion window in KV. + +- **[low] /info exposes thread/chat/sender IDs publicly** — internal/modules/util/info.go + - What: `VisibilityPublic`. Anyone in a group can see the chat ID. Low sensitivity, but in some moderation setups admins prefer not to expose internal IDs. + - User impact: shifting to `VisibilityProtected` would deny non-admins. + - Fix sketch: flip visibility OR keep as-is and document the choice. + +## Architecture observations (no immediate change) + +- Module factory closure pattern is consistent and clean across all six modules — `state{kv, ...}` built in `New`, closures bound to handlers. +- `chathelper` consolidates the obvious helpers; nothing else duplicated across modules looks worth folding (the trading argsAfterCommand is genuinely different from chathelper.ArgAfterCommand — chathelper returns a single trimmed string, trading needs the tokenized slice). +- `keylock.Map` usage discipline is right: `defer locks.Acquire(subject)()` everywhere mutation happens. Only gap: lolschedule subscribers. +- Cron-vs-bot decoupling via `messageSender` interface in lolschedule/cron.go is the right call; mirror it if another module ever adds a cron that sends. +- Visibility enforcement is correct: dispatcher gates BEFORE handler runs, denies are silent — no leak. + +## Unresolved questions +- Is the JS source's lolschedule subscribe also unlocked? If yes, the lock omission may be parity-deliberate; either way the Go runtime needs it (Lambda concurrent invocations on same chat possible). +- Should `/loldle` ambiguous-prefix behaviour (`findChampion` returns nil → "Champion not found") be improved to list candidates? UX gap, but I/O-changing.