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.
2026-06-08 02:15:15 +00:00 · 2026-05-09 12:19:27 +07:00
parent d9cafdcb30
commit c89aa1c0dc
3 changed files with 228 additions and 7 deletions
@@ -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.
@@ -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 |
@@ -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 <bad>` 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 <dup>` already guessed | HTML reply, **guess NOT counted, NOT saved** | same — duplicate-check returns before `game.Guesses = append(...)` and before any save | ✓ |
+| `/loldle_emoji <correct>` 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).