From 93a32beba3c4e8ef35f32f20661cfed908834b0b Mon Sep 17 00:00:00 2001 From: tiennm99 Date: Sat, 9 May 2026 12:19:27 +0700 Subject: [PATCH] chore(plans): mark Phase 6a partial; log code-review findings Update phase status to reflect loldle-emoji port completion. Document code-review findings in dedicated report, noting per-subject keylock strategy and JS-wire-format decode verification as key validation gates. --- .../phase-06-port-loldle-variants.md | 31 ++- plans/260508-2222-go-port-cloud-run/plan.md | 3 +- ...viewer-260509-1206-phase6a-loldle-emoji.md | 201 ++++++++++++++++++ 3 files changed, 228 insertions(+), 7 deletions(-) create mode 100644 plans/reports/code-reviewer-260509-1206-phase6a-loldle-emoji.md diff --git a/plans/260508-2222-go-port-cloud-run/phase-06-port-loldle-variants.md b/plans/260508-2222-go-port-cloud-run/phase-06-port-loldle-variants.md index d5c598d..4101e3d 100644 --- a/plans/260508-2222-go-port-cloud-run/phase-06-port-loldle-variants.md +++ b/plans/260508-2222-go-port-cloud-run/phase-06-port-loldle-variants.md @@ -1,7 +1,7 @@ --- phase: 6 title: "Port loldle variants + lolschedule" -status: pending +status: partial priority: P2 effort: "5h" dependencies: [5] @@ -65,12 +65,31 @@ A small shared package would help, but keep modules independent (KISS) until dup 8. Wire factories. 9. Smoke each command against dev bot. +## Cook scope split +This phase ships in five sub-cooks (one per module — each is large enough to risk context exhaustion): +- **6a (this cook):** loldle-emoji — 172-record emoji clue dict, binary scoring, simplest variant. ✅ +- **6b (next):** loldle-quote — same shape as emoji, quote pool. **Prep work:** extract `normalize`, `subjectFor`, `argAfterCommand`, `findChampion` to a shared package; classic loldle and 6a both already duplicate them, 6b would be the third caller — past the YAGNI extraction threshold. +- **6c (next):** loldle-ability — DDragon ability-icon URL builder, sendPhoto. +- **6d (next):** loldle-splash — DDragon splash URL builder, sendPhoto. +- **6e (next):** lolschedule — HTTP client to lolesports/leaguepedia API; no game state, different shape entirely. + ## Success Criteria -- [ ] All 5 modules respond to their commands -- [ ] Ability + splash images render in Telegram (no broken-image markers) -- [ ] `/lolschedule today` matches JS behavior -- [ ] All variants share consistent guess-count limits matching JS recent revert (`commit 29e558b`) -- [ ] Ported tests pass +- [x] loldle-emoji responds to `/loldle_emoji`, `/loldle_emoji_giveup`, `/loldle_emoji_stats`, `/loldle_emoji_setmax` +- [ ] Ability + splash images render in Telegram (no broken-image markers) — deferred to 6c/6d +- [ ] `/lolschedule today` matches JS behavior — deferred to 6e +- [ ] All variants share consistent guess-count limits matching JS recent revert (`commit 29e558b`) — partial: emoji at 5, others pending +- [x] Ported tests pass for loldle-emoji (lookup, state, render, JS-wire-format decode) + +## Implementation deviations (6a — loldle-emoji) +- `moduleNameRe` relaxed from `^[a-z0-9_]{1,32}$` to `^[a-z0-9_-]{1,32}$` so JS-source module names like `loldle-emoji` pass validation. The storage prefix delimiter (`:`) remains rejected; tests cover both shapes. +- Go package directory + package name use `loldleemoji` (no separator) per Go convention; the registered MODULE name is `loldle-emoji` (hyphenated, byte-identical to JS) for KV-prefix migration parity. +- `normalize`, `subjectFor`, `argAfterCommand` duplicated from classic loldle. Marked for extraction at the start of cook 6b — three callers will exist by then, past the YAGNI threshold. +- `winRate` uses `math.Round` from day one (lesson from Phase 5c review). +- KV TTL deferred — Cloudflare KV's `expirationTtl` has no Firestore equivalent. +- No sticker pools — JS source has none for emoji mode. + +## Code reviews (6a) +- [Phase 6a review](reports/code-reviewer-260509-1206-phase6a-loldle-emoji.md) — 0 critical, 0 high. Concerns: F#1 (JS-wire-format decode test) added in same session; F#2 (`getOrInitGame` cap-reduction edge case) deferred — defensive branch only; E (extract shared helpers) earmarked as 6b prep work. ## Risk Assessment - **Risk**: Riot Data Dragon version pinning — JS version may use different ddragon version than fresh fetch. **Mitigation**: pin version in env or fetch latest at cold start; document in README. diff --git a/plans/260508-2222-go-port-cloud-run/plan.md b/plans/260508-2222-go-port-cloud-run/plan.md index ab95055..0109d50 100644 --- a/plans/260508-2222-go-port-cloud-run/plan.md +++ b/plans/260508-2222-go-port-cloud-run/plan.md @@ -33,6 +33,7 @@ Full rewrite of miti99bot in Go for deployment on Cloud Run, swapping CF KV+D1+W - [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) +- [code-reviewer 2026-05-09 — Phase 6a loldle-emoji](reports/code-reviewer-260509-1206-phase6a-loldle-emoji.md) (0 critical/high; JS-wire-format decode test added; shared-helper extraction queued for 6b) ## Phases @@ -43,7 +44,7 @@ Full rewrite of miti99bot in Go for deployment on Cloud Run, swapping CF KV+D1+W | 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) | 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 | +| 06 | [Port loldle variants + lolschedule](phase-06-port-loldle-variants.md) | partial | 5h | 6a loldle-emoji done; quote/ability/splash/lolschedule pending | | 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 | | 09 | [Cloud Scheduler cron wiring](phase-09-cloud-scheduler.md) | pending | 2h | 2 jobs → `/cron/{name}` with OIDC | diff --git a/plans/reports/code-reviewer-260509-1206-phase6a-loldle-emoji.md b/plans/reports/code-reviewer-260509-1206-phase6a-loldle-emoji.md new file mode 100644 index 0000000..c02f5ab --- /dev/null +++ b/plans/reports/code-reviewer-260509-1206-phase6a-loldle-emoji.md @@ -0,0 +1,201 @@ +# code-reviewer · phase 6a · loldle-emoji port + +Date: 2026-05-09 +Scope: first sub-cook of Phase 6 — port `loldle-emoji` from JS to Go. +Verdict: **DONE_WITH_CONCERNS** — all flagged items are non-blocking. JS-parity solid; build/test green. + +--- + +## Scope reviewed + +- `internal/modules/loldleemoji/{loldleemoji,champions,normalize,lookup,state,render,handlers}.go` (+ tests) +- `internal/modules/loldleemoji/data/emojis.json` (172 records, verbatim from JS) +- `internal/modules/registry.go` — moduleNameRe relaxation +- `internal/modules/registry_test.go` — updated rejection cases +- `cmd/server/main.go` — factory wiring +- Cross-checked against JS source `src/modules/loldle-emoji/{index,handlers,state,render,lookup}.js` + and `src/util/normalize-name.js` + +go vet ✓ · go test -race -count=1 ./... ✓ on the loldleemoji package. + +--- + +## A. JS-parity correctness + +Walked every player-visible flow side-by-side with `handlers.js`: + +| Flow | JS | Go | Match | +|---|---|---|---| +| `/loldle_emoji` no arg, no game | render empty board | same | ✓ | +| `/loldle_emoji` no arg, mid-round | render board with N guesses | same | ✓ | +| `/loldle_emoji` no arg, target gone from refreshed pool | clearGame + "data was updated" reply (HTML) | same | ✓ | +| `/loldle_emoji ` champion not found | plain-text reply, no guess counted | same (note Go uses `%q` → ASCII-only diff for non-ASCII args; matches classic loldle) | ✓ | +| `/loldle_emoji ` already guessed | HTML reply, **guess NOT counted, NOT saved** | same — duplicate-check returns before `game.Guesses = append(...)` and before any save | ✓ | +| `/loldle_emoji ` win | recordResult(true) + clearGame + sticker — wait no, **emoji has no stickers** (correctly omitted) | same | ✓ | +| Last-guess loss | recordResult(false) + clearGame, render full board | same | ✓ | +| Mid-round wrong guess | save game with appended guess | same | ✓ | +| `startedAt` stamping | first **actual** guess (not empty-arg view) starts the clock | same — `if game.StartedAt == nil { now := nowMillis(); game.StartedAt = &now }` is **after** the no-arg early-return and **after** the duplicate-guess early-return | ✓ | +| `/loldle_emoji_giveup` no game | "No active round" hint | same | ✓ | +| `/loldle_emoji_giveup` active | recordResult(false) + clearGame + reveal | same | ✓ | +| `/loldle_emoji_stats` win-rate rounding | `Math.round(wins/played * 100)` | `math.Round(...)` (lesson from Phase 5c) | ✓ | +| `/loldle_emoji_setmax` private | strconv.Atoi range-check 1..10, persisted via configKey | same | ✓ | + +**One JS-Go behavioural divergence found** — non-blocking, present in classic loldle too: + +> `Champion not found: %q.` — Go's `%q` escape format differs from JS's `${arg}` interpolation for non-ASCII inputs (e.g. `"中文"` → `"中文"` in Go vs literal in JS). Affects only the user-visible error text reflecting their own typo. Already shipped in classic loldle (Phase 5c). Not worth fixing here unless paired with a same-PR fix in classic loldle. + +`getOrInitGame` correctly handles the mid-round-cap-reduction edge case: if a previous round saved with N guesses and the operator then `/loldle_emoji_setmax`'d below N, the existing-with-overflow check `len(existing.Guesses) < maxGuesses` is **false** so a fresh round is started. Matches JS. + +--- + +## B. moduleNameRe relaxation safety + +Tracked `loldle-emoji` end-to-end through the storage layer: + +1. `Build()` validates against `^[a-z0-9_-]{1,32}$` — accepts. +2. **Memory backend**: `MemoryProvider.For(name)` calls `Prefixed(base, name)` → prefix becomes `"loldle-emoji:"`. `Prefixed.k()` does plain string concat. Hyphen passes through cleanly. +3. **Firestore backend**: `FirestoreProvider.For(name)` creates `NewFirestoreKVStore(client, name)` → collection name `loldle-emoji`. Firestore collection name rules: not `__.*__`, no `/`, valid UTF-8, ≤1500 bytes. `loldle-emoji` is fine. +4. **`:` rejected**: confirmed by `TestBuild_RejectsInvalidModuleName` adding `"a:b"` to the rejection set. The storage prefix delimiter integrity is preserved. +5. **Cron name regex** (`internal/server/router.go`) is independent of moduleNameRe. Module-with-hyphen names are not used in URL paths; cron names live in `Cron.Name`, validated separately. +6. **Telegram BotFather command names** — irrelevant; module names are not commands. Commands still use the strict `commandNameRe = ^[a-z0-9_]{1,32}$` (validate.go), so command names with hyphens would still be rejected. Verified handler command names: `loldle_emoji`, `loldle_emoji_giveup`, etc. — underscore-only, no hyphen leak. Good. + +**Verdict**: relaxation is safe and well-scoped. The added test +`TestBuild_AcceptsHyphenatedModuleName` plus the expanded +rejection list locks the contract. + +One minor nit: `moduleNameRe` would also accept reserved Firestore patterns like `__abc__` (starts/ends with `__`). Not introduced by this change — the prior regex `^[a-z0-9_]{1,32}$` allowed it too. Out of scope. + +--- + +## C. Wire-format JSON shape parity + +Round-tripped each persisted shape against the JS-written form: + +``` +gameState (StartedAt nil): {"target":"Aatrox","guesses":[],"startedAt":null} ✓ +gameState (StartedAt set): {"target":"Aatrox","guesses":["Ahri"],"startedAt":N} ✓ +stats: {"played":N,"wins":N,"streak":N,"bestStreak":N} ✓ +roundConfig: {"maxGuesses":N} ✓ +``` + +Critical detail re-verified: `startFreshGame` initialises `Guesses: []string{}` (not `nil`), so the marshaled output has `"guesses":[]` not `"guesses":null`. JS-side `existing.guesses.length` survives the round-trip. Confirmed via empirical test. + +**Gap**: there's no test that asserts a JS-written record (i.e. raw JSON bytes +from a hypothetical KV migration source) decodes into the Go struct correctly. +The existing tests only round-trip Go → KV → Go. See section F for a suggested +test. + +--- + +## D. Concurrency + +| Surface | Verdict | +|---|---| +| `keylock.Map` per subject | Standard pattern, identical to classic loldle. Get→mutate→Put critical sections are wrapped at the top of every mutating handler. ✓ | +| `handleStats` not locked | Same as classic loldle. Single read; no lost-update risk. ✓ | +| `math/rand.Intn` global | Mutex-protected globals (lesson from Phase 5b). ✓ | +| `s.pool` slice | Built once in `loadPool()` (factory time), never mutated. `findByExactName`/`findChampion` return `&s.pool[i]` — safe because backing array is immutable for the life of the registry. Handlers only read `.ChampionName` and `.Emojis` (both strings, immutable). ✓ | +| Pool pointer outliving handler | Handler returns drop the pointer; pool persists. No GC concern. ✓ | +| `s.locks` zero-value | `keylock.Map` documents zero-value usability. ✓ | +| `defer s.locks.Acquire(subject)()` idiom | Inner call evaluated immediately (lock acquired); outer call deferred (unlock on return). Idiomatic. ✓ | + +No race detected by `-race` test runs. No mutable shared state outside KV. + +--- + +## E. Accepted-duplication tradeoff + +`normalize`, `subjectFor`, `argAfterCommand` are duplicated across loldle and +loldleemoji. Three more variants planned (loldle-quote, loldle-ability, +loldle-splash). At 5 callers the case for extraction is overwhelming. + +**Recommendation**: extract NOW, before phase 6b adds the 3rd duplicate. The +deferred-extraction note in `normalize.go` says "once a third variant lands" +— phase 6b is exactly that, so the natural extraction point is **phase 6b's +prep step**, not later. Suggested package: `internal/loldlecommon` or +`internal/champname` (for `normalize`+`findChampion` together; the lookup +function itself is also duplicated and is the most useful extraction target). + +This is a non-blocking observation; the current code is fine. Just a +flag-it-for-the-next-cook so drift doesn't compound (e.g. someone fixes a bug +in one copy and not the others — the `winRate` truncation lesson from 5c is +exactly this kind of bug). + +--- + +## F. Test gaps (highest-leverage additions) + +The existing 25-ish cases cover the in-Go logic well. Two additional tests +would meaningfully tighten the migration story: + +1. **JS-wire-format decode test** — put raw bytes + `{"target":"Ahri","guesses":["Aatrox"],"startedAt":1700000000000}` into the + KV via `kv.Put`, then call `loadGame`, then assert all three fields. This + locks the migration contract. Equivalent for `stats` and `roundConfig`. One + `t.Run`-table per shape, ~30 lines total. + +2. **`getOrInitGame` cap-reduction case** — pre-populate a game with + `len(Guesses) == 5`, set MaxGuesses to 3 via setMaxGuesses, then call + `getOrInitGame` and assert a fresh round was started (target may differ, + but `len(Guesses) == 0`). Defends the defensive branch on + `state.go:saveGame` overwriting a stale round. + +Optional third: **`getMaxGuesses` with out-of-range stored value** falls back +to default. Currently relied on but not directly tested. ~5 lines. + +(Telegram-side handler tests skipped per intentional choice — agreed; the +return-path of SendMessage isn't worth a fake HTTP server.) + +--- + +## Issues by priority + +### Critical +- None. + +### High +- None. + +### Medium +- **Extract `normalize`, `subjectFor`, `argAfterCommand`, `findChampion` into a shared package as part of phase 6b prep** (E). Three more variants are imminent; deferring extraction past 6b means three more copies must be edited every time the helper changes. Not blocking 6a; flagging for 6b's plan. + +### Low +- **`%q` escape divergence for non-ASCII args** in "Champion not found: %q." (A). Already shipped in classic loldle. If addressed, do both modules in one PR. +- **README does not mention `loldle-emoji` in MODULES example** (cmd/server/main.go review). Trivial — `/ck:docs` material. +- **`moduleNameRe` would accept `__abc__`** (Firestore-reserved). Not introduced here; pre-existing latent issue. Out of scope. + +### Nice-to-have tests +- JS-wire-format decode test for `gameState` / `stats` / `roundConfig` (F). +- `getOrInitGame` cap-reduction case (F). + +--- + +## Positive observations + +- `winRate = math.Round(...)` correctly applied without prompting (5c lesson stuck). +- `startFreshGame` writes `Guesses: []string{}` (not nil) — wire-format-correct without explicit comment, but the `state_test.go` `TestGameState_StartedAtNullByDefault` locks it. +- `loadPool` panics on bad data (build-time bug) but filters empty-emoji records in normal flow — matches JS exactly. +- `defer s.locks.Acquire(subject)()` is in every mutating handler. Easy to forget; consistent here. +- Registry test additions (`TestBuild_RejectsInvalidModuleName` with explicit + `"a:b"` entry, `TestBuild_AcceptsHyphenatedModuleName`) clearly document + the regex-relaxation contract — exactly the kind of test that catches a + later "tighten the regex" regression. +- Module-name-vs-package-name mismatch (`loldle-emoji` registered, `loldleemoji` package) is documented in code comments. Good. +- All handler command names use underscores (`loldle_emoji`, etc.) — correctly stricter than module names, matching Telegram BotFather command rules. + +--- + +## Unresolved questions + +1. Phase 6b will add `loldle-quote`. Should the helper-extraction work be + counted as part of 6a (touch the just-shipped code) or 6b (carve out + helpers as the very first commit of 6b)? My recommendation: 6b. The + current 6a code is correct and shipping. +2. Should the `%q`-vs-JS-interpolation divergence be tracked as a follow-up + ticket against both loldle modules, or accepted indefinitely? Trivial + either way. + +--- + +**Status:** DONE_WITH_CONCERNS +**Summary:** loldle-emoji port is JS-parity-correct, race-clean, and wire-format-stable; concerns are non-blocking — extract shared helpers in 6b prep, add a JS-wire-format decode test, and the pre-existing `%q` divergence remains (consistent with classic loldle).