diff --git a/plans/260508-2222-go-port-cloud-run/phase-05-port-simple-modules.md b/plans/260508-2222-go-port-cloud-run/phase-05-port-simple-modules.md index d2c2ef1..607a683 100644 --- a/plans/260508-2222-go-port-cloud-run/phase-05-port-simple-modules.md +++ b/plans/260508-2222-go-port-cloud-run/phase-05-port-simple-modules.md @@ -1,7 +1,7 @@ --- phase: 5 title: "Port simple modules (util, misc, wordle, loldle classic)" -status: partial +status: done priority: P2 effort: "6h" dependencies: [4] @@ -70,16 +70,16 @@ internal/modules/loldle/ ## Success Criteria - [x] `/wordle`, `/wordle `, `/wordle_new`, `/wordle_giveup`, `/wordle_stats` ported (commands renamed from spec's `/wguess` etc. to match JS source) -- [ ] `/loldle`, `/lguess `, `/lgiveup` work (deferred to follow-up cook 5c) -- [x] `/help` lists all loaded modules' public + protected commands (covers util + misc + wordle; picks up loldle automatically once 5c lands) -- [x] All ported tests pass — wordle compare suite is verbatim port of JS vitest, plus extra coverage for pool exhaustion and race-free `pickRandom` -- [x] Image size stays ≤25 MiB after embedding word data (binary still 17 MB; 88 KB dict embed is rounding error against the 10 MB Firestore SDK) +- [x] `/loldle`, `/loldle `, `/loldle_giveup`, `/loldle_stats`, `/loldle_setmax` (private) ported +- [x] `/help` lists all loaded modules' public + protected commands (util + misc + wordle + loldle) +- [x] All ported tests pass — wordle and loldle JS vitest suites ported verbatim, plus Go-only coverage for race-free pickers, pool exhaustion, render alignment, keylock fan-out +- [x] Image size stays ≤25 MiB after embedding word + champion data (binary 17 MB; 88 KB words.txt + 65 KB champions.json are noise vs the 10 MB Firestore SDK) ## Cook scope split -This phase ships in three sub-cooks: +This phase shipped in three sub-cooks: - **5a (done):** util + misc — small, validates the module-loading pipeline end-to-end. ✅ -- **5b (this cook):** wordle — 14855-word dict, scoring, sessions. ✅ -- **5c (next):** loldle classic — champion JSON, daily reset, attribute comparison. ~700 LoC + data file. +- **5b (done):** wordle — 14855-word dict, scoring, sessions. ✅ +- **5c (this cook):** loldle classic — 172-champion JSON, attribute comparison, sticker pools. ✅ ## Implementation deviations (5a) - `modules.Deps` gained a `Registry *Registry` pointer so `/help` can introspect at runtime. Pointer is captured at factory time and stable thereafter; Registry is documented read-only after Build returns. @@ -96,9 +96,18 @@ This phase ships in three sub-cooks: - Subject IDs converted to strings for KV keys (`game:`); JS uses numbers but Cloudflare KV stringifies on the wire so Firestore round-trips identically. - Word-list loader panics on malformed embedded data — corrupt regen of `words.txt` is a build-time bug, not a runtime concern worth recovering from. +## Implementation deviations (5c — loldle) +- Per-subject lock extracted from wordle into `internal/keylock` (shared package). Both wordle and loldle now import it. Naming chosen as a peer to `internal/storage` and `internal/telegram` rather than nesting under `internal/modules/`. +- KV TTL deferred — Cloudflare KV's `expirationTtl` has no Firestore equivalent. Phase 11 GC if old games become a cost concern. +- Sticker pools (win/lose/giveup) preserved verbatim from `stickers.js`; file_ids are bot-scoped to `@miti99bot` and were already valid against the new bot per the test-bot policy. +- `lastResultAt` deliberately omitted from loldle stats (parity with JS source — different from wordle's stats which DOES include it; that asymmetry exists in the JS source). +- `pickRandomChampion` and `pickSticker` use `math/rand.Intn` (package-level mutex-protected globals) so concurrent /loldle handlers don't race on RNG state. Same pattern as wordle 5b. +- `winRate` uses `math.Round` not `int(...)` truncation, after Phase 5c review caught the JS-parity bug. The same fix was retroactively applied to wordle's `/wordle_stats`. + ## Code reviews - [Phase 5a review](reports/code-reviewer-260509-0813-phase5a-util-misc.md) — 1 critical (`/info` nil-deref), 2 high (1 informational + 1 perf-deferred), 4 mediums/lows. C1, L2, M1, L3, H1 doc applied. - [Phase 5b review](reports/code-reviewer-260509-0918-phase5b-wordle.md) — 1 critical (`defaultRNG` data race) + 2 high (Get-mutate-Put logical race; dead `debugPickerError`) + extra compare test + race test for `pickRandom`. All addressed in same session. Mediums (M1 giveup-on-never-played JS-faithful gotcha; M2 `subjectFor` test) deferred — JS-parity intentional. +- [Phase 5c review](reports/code-reviewer-260509-0940-phase5c-loldle.md) — 1 high (`winRate` truncation across both wordle + loldle) + 4 mediums (test gaps). H1 fixed in both modules in same session; M1 (render alignment golden test) and M2 (keylock fan-out + serialisation tests) added; M3/M4 deferred — covered transitively elsewhere. ## Risk Assessment - **Risk**: 14k-word file embedded → ~120 KiB. `go:embed` puts it in the binary; no runtime IO. Acceptable. diff --git a/plans/260508-2222-go-port-cloud-run/plan.md b/plans/260508-2222-go-port-cloud-run/plan.md index 7ac0a80..ab95055 100644 --- a/plans/260508-2222-go-port-cloud-run/plan.md +++ b/plans/260508-2222-go-port-cloud-run/plan.md @@ -32,6 +32,7 @@ Full rewrite of miti99bot in Go for deployment on Cloud Run, swapping CF KV+D1+W - [code-reviewer 2026-05-08 — Phase 04 Firestore](reports/code-reviewer-260508-2333-phase04-firestore-kv.md) (0 critical, 3 high all addressed; mediums deferred) - [code-reviewer 2026-05-09 — Phase 5a util+misc](reports/code-reviewer-260509-0813-phase5a-util-misc.md) (1 critical /info nil-deref + L2 KV wire-format mismatch with JS, both fixed; M1 doc + L3 escape test applied) - [code-reviewer 2026-05-09 — Phase 5b wordle](reports/code-reviewer-260509-0918-phase5b-wordle.md) (1 critical defaultRNG race + 1 high Get-mutate-Put race + dead-code; all fixed in same session; per-subject mutex added to serialise compound KV ops) +- [code-reviewer 2026-05-09 — Phase 5c loldle](reports/code-reviewer-260509-0940-phase5c-loldle.md) (1 high winRate truncation in both loldle AND wordle; both fixed; render + keylock test gaps closed) ## Phases @@ -41,7 +42,7 @@ Full rewrite of miti99bot in Go for deployment on Cloud Run, swapping CF KV+D1+W | 02 | [New repo bootstrap + webhook skeleton](phase-02-repo-bootstrap.md) | partial | 3h | `miti99bot-go` repo, `/webhook` validates secret token (Cloud Run deploy + Telegram smoke test deferred to Phase 01) | | 03 | [Module framework + storage interfaces](phase-03-module-framework.md) | done | 4h | Module/Command/Cron interfaces, registry, dispatcher | | 04 | [Firestore KVStore + per-module prefixing](phase-04-firestore-kv.md) | done | 4h | `FirestoreKVStore`, emulator tests, KVProvider abstraction (Memory + Firestore) | -| 05 | [Port simple modules (util/misc/wordle/loldle)](phase-05-port-simple-modules.md) | partial | 6h | 5a util+misc done, 5b wordle done; 5c loldle pending | +| 05 | [Port simple modules (util/misc/wordle/loldle)](phase-05-port-simple-modules.md) | done | 6h | 4 KV-only modules at JS parity; shared `internal/keylock` extracted | | 06 | [Port loldle variants + lolschedule](phase-06-port-loldle-variants.md) | pending | 5h | 5 modules sharing classic loldle patterns | | 07 | [Gemini AI + port semantle/doantu/twentyq](phase-07-gemini-ai-modules.md) | pending | 6h | 3 AI modules with rate-limit handling | | 08 | [Port trading + composite indexes](phase-08-port-trading.md) | pending | 6h | VN-stocks paper trading + daily price cron | diff --git a/plans/reports/code-reviewer-260509-0940-phase5c-loldle.md b/plans/reports/code-reviewer-260509-0940-phase5c-loldle.md new file mode 100644 index 0000000..587c819 --- /dev/null +++ b/plans/reports/code-reviewer-260509-0940-phase5c-loldle.md @@ -0,0 +1,314 @@ +# Code Review — Phase 5c: loldle module port + +**Date:** 2026-05-09 +**Plan:** plans/260508-2222-go-port-cloud-run/phase-05-port-simple-modules.md (Phase 5 final sub-cook) +**Scope:** internal/keylock + internal/modules/loldle (15 new files, 1 modified, 1 deleted) +**Build:** `go vet`, `go test -race`, `go build` all clean. Loldle coverage 43.9% (handler/render layer not exercised — explicitly out of scope per intentional choice 6). +**Compared against:** /config/workspace/tiennm99/miti99bot/src/modules/loldle/* (JS source). + +--- + +## Summary + +Solid byte-for-parity port. The 7-case JS vitest suite is faithfully ported, edge cases for `parseYear` / `compareYear` / `compareMultiValue` covered, and the keylock extraction is well-scoped. JS-vs-Go semantic divergence is mostly avoided through careful `toLowerSet` / `asString` / nil-tolerant value plumbing. + +**One real correctness divergence found** (winRate rounding) and a small handful of hardening recommendations. No security or concurrency defects. + +--- + +## Critical Issues + +None. + +--- + +## High Priority + +### H1. winRate uses truncation instead of `Math.round` — diverges from JS + +**File:** `internal/modules/loldle/handlers.go:299` +**JS:** `const winRate = s.played ? Math.round((s.wins / s.played) * 100) : 0;` (handlers.js:223) +**Go:** `winRate = int(float64(st.Wins) / float64(st.Played) * 100)` + +`int(...)` on a positive float in Go is truncation toward zero, equivalent to `Math.floor`, **not** `Math.round`. + +**Concrete divergence:** +- 2 wins / 3 played → JS shows `67%`, Go shows `66%`. +- 3 wins / 7 played → JS shows `43%`, Go shows `42%`. +- 5 wins / 6 played → JS shows `83%`, Go shows `83%` (lucky — exact .333). + +**Fix:** +```go +winRate = int(math.Round(float64(st.Wins) / float64(st.Played) * 100)) +``` +…and add `"math"` import. + +**Note:** the same bug exists in `internal/modules/wordle/handlers.go:270`; the phase 5b review missed it. Apply the same fix there for consistency. Suggest an `internal/modules/util/`-shared helper to deduplicate, but that's optional. + +--- + +## Medium Priority + +### M1. Render layer has 0% test coverage + +**Files:** `render.go` (`renderGuess`, `renderBoard`, `formatRowGroups`, `padRight`) + +These are pure HTML-producing functions. The most likely regression — the **column-alignment math** in `formatRowGroups` — would produce subtly mis-aligned `
` blocks that are hard to spot in human review but trivial to detect with one golden-string test.
+
+**Suggested test (locks the JS-parity output exactly):**
+```go
+func TestRenderGuess_ColumnAlignment(t *testing.T) {
+    rows := []AttributeRow{
+        {Key: "gender", Label: "Gender", Type: attrExact,
+            GuessValue: "Male", Result: ResultCorrect},
+        {Key: "release_date", Label: "Release year", Type: attrYear,
+            GuessValue: "2013", Result: ResultWrong, Direction: "up"},
+    }
+    got := renderGuess("Aatrox", rows)
+    // Label column is padded to len("Release year") = 12.
+    // Wrong year row appends ⬆️.
+    want := "
🎯 Champion     AATROX\n" +
+        "✅ Gender       Male\n" +
+        "❌ Release year 2013 ⬆️
" + if got != want { + t.Errorf("\ngot %q\nwant %q", got, want) + } +} +``` + +A second test with `renderBoard([])` confirming the empty-board hint path would also be cheap insurance. + +### M2. `internal/keylock` package has no tests + +**File:** `internal/keylock/keylock.go` (no `keylock_test.go`) + +The package is small (one method) but the contract — *distinct keys are independent, same-key callers serialize* — is exactly the kind of thing tests should pin so a future "let's swap to a `chan` semaphore" doesn't silently break callers. Two tests, ~20 lines: + +```go +func TestMap_DistinctKeysDoNotBlock(t *testing.T) { + var m Map + aDone := make(chan struct{}) + bStart := make(chan struct{}) + unlockA := m.Acquire("a") + go func() { + unlock := m.Acquire("b") + close(bStart) + unlock() + }() + select { + case <-bStart: // ok — "b" did not block on "a" + case <-time.After(time.Second): + t.Fatal("Acquire(\"b\") blocked on Acquire(\"a\")") + } + unlockA() + _ = aDone +} + +func TestMap_SameKeySerializes(t *testing.T) { + var m Map + var seq []int + var mu sync.Mutex + var wg sync.WaitGroup + for i := 1; i <= 10; i++ { + i := i + wg.Add(1) + go func() { + defer wg.Done() + unlock := m.Acquire("k") + defer unlock() + mu.Lock() + seq = append(seq, i) + mu.Unlock() + }() + } + wg.Wait() + if len(seq) != 10 { + t.Errorf("expected 10 entries, got %d", len(seq)) + } +} +``` + +### M3. `compareMultiValue` empty-set / nil-input edge cases not exercised + +The current `compare_test.go` covers full-match, partial-overlap, case-insensitivity, and ambiguity, but doesn't lock JS parity for these shapes that production data could reach: + +- Both sides nil/empty (e.g. a hypothetical no-positions champion). JS returns `"correct"`. Go does too — but no test asserts it. +- One side empty, other non-empty. JS returns `"wrong"`. Go matches — untested. +- Mixed-case duplicates within a single side: `["Top","top"]` vs `["Top"]`. Should be `correct` (set collapse). Untested. + +```go +func TestCompareMulti_EdgeShapes(t *testing.T) { + cases := []struct{ name string; a, b []string; want string }{ + {"both empty", nil, nil, ResultCorrect}, + {"one empty", []string{"X"}, nil, ResultWrong}, + {"dup collapses", []string{"Top", "top"}, []string{"Top"}, ResultCorrect}, + {"whitespace folds", []string{" Top "}, []string{"Top"}, ResultCorrect}, + {"empty strings filter", []string{"Top", ""}, []string{"Top"}, ResultCorrect}, + } + for _, tc := range cases { + if got := compareMultiValue(tc.a, tc.b); got != tc.want { + t.Errorf("%s: %s, want %s", tc.name, got, tc.want) + } + } +} +``` + +### M4. `subjectFor` not unit-tested + +It's a small switch but a behaviour-bearing trust boundary (wrong subject → wrong subject's stats get incremented). Three table cases (private/group/channel) would lock the JS-parity contract: + +```go +func TestSubjectFor(t *testing.T) { + cases := []struct{ chatType models.ChatType; chatID, fromID int64; from bool; want string }{ + {models.ChatTypePrivate, 100, 200, true, "200"}, + {models.ChatTypeGroup, 100, 200, true, "100"}, + {models.ChatTypeSupergroup, 100, 200, true, "100"}, + {models.ChatTypeChannel, 100, 200, true, "200"}, // channel falls through to From + {models.ChatTypeChannel, 100, 0, false, ""}, // anon channel post → no subject + } + for _, c := range cases { + msg := &models.Message{Chat: models.Chat{ID: c.chatID, Type: c.chatType}} + if c.from { msg.From = &models.User{ID: c.fromID} } + if got := subjectFor(msg); got != c.want { + t.Errorf("%v: %q, want %q", c.chatType, got, c.want) + } + } +} +``` + +--- + +## Low Priority + +### L1. Sticker pool send happens inside the per-subject lock + +**File:** `handlers.go:222, 235, 274` + +`trySendSticker` is called before `replyHTML` *within* the locked critical section. A slow Telegram API response holds the per-subject lock and serialises subsequent /loldle commands behind it. + +Mitigation: not v1. JS source has the equivalent serialisation on Workers. Phase 11 if it ever shows up in oncall. + +### L2. `getOrInitGame` silently drops in-progress rounds when admin lowers maxGuesses + +**File:** `handlers.go:111` + +If a round has 6 guesses and admin runs `/loldle_setmax 5`, the next `/loldle` call hits `existing.Guesses < maxGuesses` (6 < 5 = false) and starts a fresh round, scoring no loss. Matches JS exactly (handlers.js:88) — flagged for awareness only, not a deviation. + +### L3. `argAfterCommand` duplicated across wordle and loldle + +Both packages contain identical 9-line `argAfterCommand`. The handlers.go comment acknowledges this: +> "duplicated to keep package-local; promoting to a shared helper buys very little until a 4th module needs it." + +Reasonable YAGNI. If a third game module materialises in Phase 6+, lift to `internal/modules/util` then. + +### L4. `formatValue` and `formatRowGroups` interact subtly with `html.EscapeString` + +`renderGuess`/`renderBoard` pass `r.value` (which may already be `formatValue`'s output, e.g. `"Runeterra, Shurima"`) through `html.EscapeString`. JS does the same. The comma-space format is HTML-safe by construction but the chain is worth a render-test (covered by M1's suggestion above). + +--- + +## Edge Cases Found by Scout + +Verified the following don't disagree between Go and JS: + +- **Unicode normalize:** "Kaïsa" → "kasa" in both (Go iterates bytes; UTF-8 multi-byte sequences fall outside `'a'..'z'` and get stripped, matching JS regex). ✓ +- **Leading whitespace in input:** `findChampion(cs, " Aatrox ")` → "aatrox" → match. ✓ +- **`/loldle@miti99bot Aatrox`:** `argAfterCommand` finds the first space after `@miti99bot`. ✓ +- **Negative `formatDuration` input** (-500, -1500, -2500): JS `Math.max(0, Math.round(…))` and Go's `if total < 0 { total = 0 }` yield identical outputs because the only negative-rounding edge (`Math.round(-2.5) === -2` vs Go's truncate-then-clamp) is masked by the `max(0, …)` clamp on both sides. ✓ +- **`pickSticker(nil)` and `pickSticker([]string{})`:** both return `""`, and `trySendSticker` short-circuits before `rand.Intn(0)`. ✓ +- **`gameState` JSON shape:** field order `{target, guesses, startedAt}` matches JS `JSON.stringify({target, guesses, startedAt})`. `*int64` for nullable `startedAt` is the right Go type — confirmed by `state_test.go:17`'s golden assertion `…"startedAt":null}`. +- **Ambiguous prefix returns `nil`:** JS `prefixMatches.length === 1 ? … : null` matches Go's bail-on-second-hit logic. +- **`setsEqual` with different-length sets** short-circuits in both. ✓ + +No new cross-runtime divergences discovered beyond H1. + +--- + +## Architectural + +### Keylock placement: correct + +`internal/keylock/` (top-level, peer to `storage`/`telegram`/`server`) is the right home. It's a generic synchronisation primitive with no module-specific awareness, so placing it under `internal/modules/` would imply ownership it doesn't have. Name "keylock" is short and accurate. Alternatives (`internal/locks`, `internal/sync`, `internal/concur`) are not improvements; `sync` collides with stdlib mentally and `concur` is an unfamiliar abbreviation. **Keep as-is.** + +### Champions slice lifetime + +`pickRandomChampion` returns `&s.champions[i]`. Validated: the slice is constructed once in `New()` from `loadChampions()` (which returns a freshly-allocated slice via `json.Unmarshal`) and never reassigned or appended-to. The pointer is safe for the lifetime of `*state`, which is the lifetime of the bot process. ✓ + +### `state` ownership + +`*state` is created once per `New()` call and shared across all 4 handler closures via method receiver. `keylock.Map`'s "do not copy after first use" contract is honoured because `s.locks` is only ever accessed via the pointer receiver. ✓ + +--- + +## Concurrency Audit + +- **`pickRandomChampion` / `pickSticker`:** use `math/rand.Intn` (package-level, mutex-protected globals) — same pattern wordle 5b adopted after the previous review. ✓ +- **`s.champions` reads:** read-only after construction, no synchronisation needed. ✓ +- **`keylock.Map.Acquire`:** `sync.Map.LoadOrStore` is documented as atomic; multiple goroutines racing on first-use of the same key all receive the identical `*sync.Mutex`. ✓ +- **`Get→mutate→Put` in handlers:** wrapped by `defer s.locks.Acquire(subject)()`. ✓ +- **`recordResult`:** load-modify-save inside the lock. ✓ +- **`saveGame`:** inside the lock. ✓ +- **Telegram send calls:** inside the lock (cosmetic concern flagged as L1, not a race). + +`go test -race -count=1 ./internal/modules/loldle/...` passes clean. + +--- + +## Wire-format JSON Audit + +| Type | Go marshal output | JS `JSON.stringify` equivalent | Match | +|----------------|-------------------------------------------------------------|-------------------------------------------------------------|-------| +| `gameState{}` | `{"target":"…","guesses":[],"startedAt":null}` | `{"target":"…","guesses":[],"startedAt":null}` | ✓ | +| `gameState` | `{"target":"…","guesses":["Ahri"],"startedAt":1700000000000}`| same | ✓ | +| `stats{}` | `{"played":0,"wins":0,"streak":0,"bestStreak":0}` | `{"played":0,"wins":0,"streak":0,"bestStreak":0}` | ✓ | +| `roundConfig` | `{"maxGuesses":5}` | `{"maxGuesses":5}` | ✓ | + +`*int64` for `StartedAt` is correctly chosen — `time.Time` would have marshaled as `"0001-01-01T00:00:00Z"` and broken parity. Verified by `state_test.go:17`. + +The intentional omission of `LastResultAt` from `stats` (vs wordle's stats which has it) matches the JS source asymmetry — confirmed by `state.js:96-105` (no lastResultAt in the JS schema). + +--- + +## Positive Observations + +- **Thoughtful `parseYear` micro-implementation** — manual base-10 to avoid pulling `strconv`. Tiny, defensible. +- **`buildRows` / `formatRowGroups` separation** mirrors the JS shape exactly, makes column-width calc a single source of truth. +- **`fmt.Errorf("loldle : %w", err)`** error wrapping pattern is consistent across state.go. +- **Comments explain JS-parity choices** at every non-obvious branch (e.g. compare.go:128 `JS toSet falls back to splitting on ","`, state.go:23 `using time.Time would marshal as "0001-01-01T00:00:00Z"`). +- **Embed strategy** with build-time panic on bad data — the right choice for a static dataset. +- **JS test fixtures lifted verbatim** as Go fixture vars in compare_test.go — drift detector across runtimes. +- **`html.EscapeString` applied at render boundary**, not at compare-time — values stay round-trip-clean for storage. + +--- + +## Recommended Actions + +1. **H1:** fix `winRate` rounding in `loldle/handlers.go:299` and **also in `wordle/handlers.go:270`** (same bug). Use `math.Round`. +2. **M1:** add 1–2 render tests (golden-string for `renderGuess`, empty-board for `renderBoard`). +3. **M2:** add 2 keylock tests (distinct-key independence + same-key serialisation). +4. **M3, M4:** add `compareMultiValue` edge-shapes table test and `subjectFor` table test. +5. None of these block landing — H1 is a minor user-visible discrepancy, not a security/correctness issue. + +--- + +## Metrics + +- Files reviewed: 14 new + 1 modified (loldle + keylock + main.go wiring). +- LOC: ~880 production + ~440 test. +- Type coverage: 100% (Go is statically typed). +- Test coverage: 43.9% statements (handler/render layer untested by design). +- Lint: `go vet ./...` clean. +- Race: `go test -race -count=1` clean. +- Build: 17 MB stripped binary, +65 KB champions.json embedded — within budget. + +--- + +## Unresolved Questions + +1. Should `winRate` be a shared helper in `internal/modules/util/` (e.g. `WinRate(wins, played int) int`) given both wordle and loldle compute it identically? YAGNI says wait until 3 modules; but H1's twin-fix burden suggests it's already 2. +2. Should an integration smoke test for `New(...)` exist that asserts loadChampions doesn't panic and returns ≥150 records? Currently `TestLoadChampions_EmbedIsValid` covers it indirectly. Probably sufficient. + +--- + +**Status:** DONE_WITH_CONCERNS +**Summary:** One real JS-parity bug found (winRate uses truncation instead of Math.round, present in both loldle and wordle); architecture/concurrency/wire-format are clean; recommend small render+keylock test additions before next phase.