chore(plans): update phase 02-03 status and record code review findings

Phase 02 (repo bootstrap, partial) and Phase 03 (module framework)
completed and reviewed. Cloud Run deployment deferred to Phase 01.
Updates plan progress, phase completion notes, and code review report
documenting security hardening and test coverage decisions.

plans/260508-2222-go-port-cloud-run/phase-02-repo-bootstrap.md

@@ -1,7 +1,7 @@
 ---
 phase: 2
 title: "New repo bootstrap + webhook skeleton"
-status: pending
+status: partial
 priority: P1
 effort: "3h"
 dependencies: [1]
@@ -62,11 +62,17 @@ Choice of `github.com/go-telegram/bot` (not `go-telegram-bot-api/v5`) — active
 10. Manual deploy to Cloud Run for end-to-end check: `gcloud run deploy miti99bot-go --source=. --region=asia-southeast1 --set-env-vars=… --set-secrets=TELEGRAM_BOT_TOKEN=…,TELEGRAM_WEBHOOK_SECRET=…`. Point dev bot's webhook at the Cloud Run URL. Verify `/ping`.
 
 ## Success Criteria
-- [ ] Repo exists, CI green
-- [ ] `/ping` works against dev bot via Cloud Run URL
-- [ ] Secret-token mismatch returns 401
-- [ ] Image size ≤20 MiB
-- [ ] No-secrets-in-git audit clean (gitleaks or manual scan)
+- [x] Repo exists, CI workflow defined (`go vet` + `go test -race` + `go build`)
+- [ ] `/ping` works against dev bot via Cloud Run URL — **deferred until Phase 01 (GCP setup) lands**
+- [x] Secret-token mismatch returns 401 (constant-time compare; covered by `internal/telegram/webhook_test.go`)
+- [x] Image size ≤20 MiB (binary 6.4 MB; distroless image ≈9 MB)
+- [x] No-secrets-in-git audit clean (no creds present; secrets stripped from `Deps.Env`)
+
+## Implementation deviations
+- Step 1 (`gh repo create`) skipped — repo already exists at `github.com/tiennm99/miti99bot-go`.
+- Steps 9–10 (ngrok smoke test, Cloud Run deploy) deferred to Phase 01.
+- Step 4's direct `/ping` registration replaced by the Phase 03 module dispatcher; no example module is shipped yet (`MODULES=""` boots cleanly).
+- Webhook + cron handlers carry hardenings beyond spec: constant-time secret compare, `MaxBytesReader`, shared-secret bridge for `/cron/{name}` (env: `CRON_SHARED_SECRET`; absent → endpoint disabled), bounded handler context via `bot.WithNotAsyncHandlers`, `bot.WithSkipGetMe` to avoid 5s cold-start blocking call. Code review recommendations C1–C3, H1–H7, M1, M4, L5 applied; remaining M-class items tracked in [report](reports/code-reviewer-260508-2254-phase02-03-bootstrap.md).
 
 ## Risk Assessment
 - **Risk**: `gcloud run deploy --source` uses Cloud Build, which has its own free tier (120 build-min/day). **Mitigation**: small Go build is ~30s; well within. CI/CD in Phase 10 may move builds to GHA to keep Cloud Build for fallback.

plans/260508-2222-go-port-cloud-run/phase-03-module-framework.md

@@ -1,7 +1,7 @@
 ---
 phase: 3
 title: "Module framework + storage interfaces"
-status: pending
+status: done
 priority: P1
 effort: "4h"
 dependencies: [2]
@@ -111,10 +111,18 @@ type KVStore interface {
 10. Unit tests: `registry_test.go` (conflict detection, validation errors), `prefix_test.go` (round-trip).
 
 ## Success Criteria
-- [ ] Empty `MODULES=""` boots cleanly, only `/start` works (grammY default fallback)
-- [ ] Two modules with same command name → startup fails with clear error
-- [ ] Per-module KVStore prefix isolation verified by test
-- [ ] `go vet ./...` + `go test ./...` green
+- [x] Empty `MODULES=""` boots cleanly (no fallback handler today; grammY's `/start` parity deferred to a future phase)
+- [x] Two modules with same command name → startup fails with clear error (`TestBuild_DetectsCommandConflict`)
+- [x] Per-module KVStore prefix isolation verified by test (`TestBuild_PerModulePrefixedKV`, `TestPrefixed_RoundTrip`, `TestDispatchScheduled_PassesPrefixedDeps`)
+- [x] `go vet ./...` + `go test -race -count=1 ./...` green
+
+## Implementation deviations
+- `Factory func() Module` → `Factory func(deps Deps) Module`: handler closures capture deps directly. Eliminates a separate `Module.Init` lifecycle step.
+- `Factories []Factory` → `Factories map[string]Factory`: required for `MODULES`-env name lookup; prevents duplicate names at compile-load.
+- `Deps` ships only `KV` + `Env` today. `Firestore` + `Gemini` fields land in Phases 04 / 07 (YAGNI).
+- Cron uniqueness enforced across modules (registry-level), instead of "concurrent errgroup of all matches" — simpler and matches the one-cron-per-name reality.
+- `cmd/server` strips `TELEGRAM_BOT_TOKEN`, `TELEGRAM_WEBHOOK_SECRET`, `CRON_SHARED_SECRET` from `Deps.Env` to prevent accidental leakage.
+- Module names validated against `^[a-z0-9_]{1,32}$` (same regex as commands) so KV prefix isolation cannot be subverted by a `:` in the name.
 
 ## Risk Assessment
 - **Risk**: `go-telegram/bot` `RegisterHandler` is more general than grammY's `bot.command`. Need to confirm behavior on `/cmd@botname` (group chats). **Mitigation**: library docs say `MatchTypeCommand` strips `@botname`; verify with a group chat test before Phase 05.

plans/260508-2222-go-port-cloud-run/plan.md

@@ -1,7 +1,7 @@
 ---
 title: "Go port of miti99bot to Google Cloud Run (free tier)"
 description: "Full rewrite of the grammY/Cloudflare Worker bot in Go, deployed to Cloud Run with Firestore + Gemini + Cloud Scheduler — all free-tier."
-status: planning
+status: in-progress
 priority: P2
 effort: 5-7d
 branch: main
@@ -28,15 +28,15 @@ Full rewrite of miti99bot in Go for deployment on Cloud Run, swapping CF KV+D1+W
 - **Cutover**: dual-run on test bot → flip prod webhook → 7-day overlap → decommission CF Worker.
 
 ## Reports
-- _(none yet — research phase optional, only triggered if cold-start / quota assumptions need validation)_
+- [code-reviewer 2026-05-08 — Phase 02-03 bootstrap](reports/code-reviewer-260508-2254-phase02-03-bootstrap.md) (3 critical + 7 high addressed in same session; 2 medium + nits deferred)
 
 ## Phases
 
 | # | Phase | Status | Effort | Key deliverable |
 |---|-------|--------|--------|-----------------|
 | 01 | [GCP setup + free-tier baseline](phase-01-gcp-setup.md) | pending | 3h | Hello-world Go on Cloud Run, cold-start P95 captured |
-| 02 | [New repo bootstrap + webhook skeleton](phase-02-repo-bootstrap.md) | pending | 3h | `miti99bot-go` repo, `/webhook` validates secret token |
-| 03 | [Module framework + storage interfaces](phase-03-module-framework.md) | pending | 4h | Module/Command/Cron interfaces, registry, dispatcher |
+| 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) | pending | 4h | `FirestoreKVStore`, emulator tests, in-memory fake |
 | 05 | [Port simple modules (util/misc/wordle/loldle)](phase-05-port-simple-modules.md) | pending | 6h | 4 KV-only modules at parity with JS version |
 | 06 | [Port loldle variants + lolschedule](phase-06-port-loldle-variants.md) | pending | 5h | 5 modules sharing classic loldle patterns |
@@ -83,6 +83,7 @@ If Firestore reads cap is hit → enable Cloud Run instance-level cache (warm-in
 Per-phase rollback documented in each phase file. Phase 12 is the only irreversible step; until then, the CF Worker continues to serve prod via existing webhook.
 
 ## Open questions
-1. Scheduler cron names — keep `0 17 * * *` (UTC) = midnight Saigon? Confirm timezone target.
-2. Migrate KV/D1 data to Firestore (one-shot export) or start fresh on cutover? Trading has user-balance state worth preserving; loldle/wordle session state can reset.
-3. Test Telegram bot — reuse an existing dev bot, or create new `@miti99bot_dev`?
+_Resolved 2026-05-08:_
+1. ~~Scheduler cron names~~ → **Keep `0 17 * * *` UTC** (= midnight Saigon). Cloud Scheduler stays UTC, no behavior change vs. JS Worker.
+2. ~~Migrate KV/D1 data~~ → **Migrate everything**. One-shot export of D1 + KV → Firestore on cutover. Phase 12 owns the migration script.
+3. ~~Test Telegram bot~~ → **User creates the bot manually**, token + webhook secret injected via Cloud Run env vars (`TELEGRAM_BOT_TOKEN`, `TELEGRAM_WEBHOOK_SECRET`).

plans/reports/code-reviewer-260508-2254-phase02-03-bootstrap.md

@@ -0,0 +1,327 @@
+# Code Review: Phase 02 (partial) + Phase 03 — Repo Bootstrap & Module Framework
+
+**Date:** 2026-05-08
+**Scope:** `cmd/server/`, `internal/{server,telegram,modules,storage}/`, Dockerfile, CI, README
+**Spec:** `plans/260508-2222-go-port-cloud-run/phase-{02,03}.md`
+**Build status:** `go vet ./...` green, `go test -race -count=1 ./...` green
+
+---
+
+## TL;DR
+
+Solid scaffolding. Test coverage on registry + prefix wrapper is good. **Three real bugs** that will bite Phase 04+:
+
+1. `bot.Start(ctx)` is wrong for webhook mode (will spam Telegram with 409s).
+2. Cron handler receives **unprefixed** Deps, breaking per-module KV isolation for crons.
+3. Webhook secret-token compare is not constant-time.
+
+Plus several things to harden before any module ships in Phase 05.
+
+---
+
+## Critical
+
+### C1 — `bot.Start(ctx)` runs long-polling against an active webhook
+**File:** `cmd/server/main.go:64` and comment at lines 61–63.
+
+The comment claims `Start` "enables long-polling-style internals the library uses for handler matching cleanup." That is wrong. Reading `vendor/github.com/go-telegram/bot/bot.go:137` and `get_updates.go`:
+
+- `Start` spawns `getUpdates` (a long-poll loop calling Telegram's `getUpdates` API) and `waitUpdates` workers.
+- When a webhook is registered, Telegram returns **HTTP 409 Conflict** for every `getUpdates` call. The library will log the error via `defaultErrorsHandler` and retry with exponential backoff capped at 5s. **Forever.** Visible noise in Cloud Run logs and wasted egress.
+- `waitUpdates` only services the `b.updates` channel. Our webhook calls `b.ProcessUpdate(...)` directly, bypassing the channel entirely, so the workers idle.
+
+`Start` is doing nothing useful in webhook mode. Remove it. Either:
+- Drop the goroutine altogether (handler dispatch via `ProcessUpdate` does not require workers).
+- Or use `bot.WithWebhookSecretToken(...)` + `b.StartWebhook(ctx)` + `b.WebhookHandler()` and feed via the library's channel (more idiomatic, but requires reworking our handler).
+
+Smallest fix: delete line 64.
+
+### C2 — Cron dispatcher hands handlers the **base** Deps, not per-module prefixed Deps
+**Files:** `internal/modules/cron_dispatcher.go:11–17`, `internal/server/router.go:56`, `cmd/server/main.go:35,48`.
+
+The flow:
+1. `main.go` builds `deps := modules.Deps{KV: kv, ...}` with the **unprefixed** in-memory KV.
+2. `Build()` constructs each module with a *prefixed* `moduleDeps` (registry.go:76–80). Good.
+3. The cron handler (a closure inside the factory) **may** capture the prefixed `moduleDeps.KV` — fine for closures.
+4. But `DispatchScheduled(ctx, name, reg, deps)` is called with the **base, unprefixed** `deps` (router.go:56), and it passes this to `cron.Handler(ctx, deps)`.
+
+So a Phase 05 module that writes the idiomatic way:
+
+```go
+Cron{Handler: func(ctx context.Context, deps modules.Deps) error {
+    return deps.KV.Put(ctx, "last_run", []byte(time.Now().String()))
+}}
+```
+
+…writes to the **bare** key `last_run`, colliding with every other module's cron. Two modules running daily crons will silently overwrite each other.
+
+The signature *implies* `deps` is the channel by which the handler gets its dependencies. Either:
+- `DispatchScheduled` reconstructs prefixed deps from the cron owner (use `cronOwners` map, currently thrown away after Build).
+- Or remove `Deps` from `CronHandler` signature and document "capture deps via factory closure". Risky footgun.
+
+Recommend the first option: add `cronOwners` to `Registry` and have `DispatchScheduled` re-prefix.
+
+### C3 — Webhook secret compare is not constant-time
+**File:** `internal/telegram/webhook.go:35`.
+
+```go
+if r.Header.Get(secretTokenHeader) != secret || secret == "" {
+```
+
+Go's `!=` on strings short-circuits on first byte mismatch. An attacker observing response timing across many requests can recover the secret byte-by-byte (classic CWE-208).
+
+Fix:
+```go
+got := r.Header.Get(secretTokenHeader)
+if secret == "" || subtle.ConstantTimeCompare([]byte(got), []byte(secret)) != 1 {
+    http.Error(w, "unauthorized", http.StatusUnauthorized)
+    return
+}
+```
+
+Telegram secrets are also rate-limited at the network edge, so the practical risk is moderate, but the fix is one line and `crypto/subtle` is stdlib.
+
+---
+
+## High
+
+### H1 — `/cron/{name}` is fully unauthenticated
+**File:** `internal/server/router.go:36–66`.
+
+Phase 09 plans OIDC auth, but until then this endpoint is **public on the internet**. Anyone who knows a cron name (and they're guessable: `daily`, `wordle_reset`, etc.) can:
+- Trigger billable side effects (Firestore writes, Gemini API calls in later phases).
+- DoS the instance: each request locks an instance for up to 5 minutes (`defaultCronTimeout`). Cloud Run free tier scales to ≤1 concurrent instance per the plan; a handful of attacker requests pin the bot.
+
+Until Phase 09 lands, at minimum:
+- Require a shared secret header (e.g. `X-Cron-Token` env var) and reject if absent. Cheap, removes the public-discovery risk.
+- Or gate behind an `ENABLE_CRON_ENDPOINT` env flag (default false) so dev environments don't expose it.
+
+### H2 — Webhook `r.Context()` cancels mid-handler (async dispatch + immediate 200)
+**Files:** `internal/telegram/webhook.go:49–50`; library `process_update.go:31` confirms async-by-default.
+
+`b.ProcessUpdate(r.Context(), &update)` runs the handler in a fresh goroutine (`go r(ctx, b, upd)`). The webhook then writes 200 and returns, after which the HTTP server cancels `r.Context()`. Any handler that does network I/O (Telegram API calls, Firestore reads) will get `context.Canceled` from its `ctx`.
+
+This is invisible today (no commands shipped), but the moment Phase 05 lands a real handler doing `b.SendMessage(ctx, ...)`, calls will fail intermittently — and worse, only when the webhook returns *fast* (the race is tighter for cheap handlers).
+
+Fix: pass a detached context to `ProcessUpdate`. Go 1.21+ has `context.WithoutCancel`:
+```go
+ctx := context.WithoutCancel(r.Context())
+ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
+defer cancel() // will not be hit before goroutine — see note below
+b.ProcessUpdate(ctx, &update)
+```
+
+Note: because `ProcessUpdate` spawns a goroutine, you cannot `defer cancel()` here — the goroutine outlives the request. Better: `bot.WithNotAsyncHandlers()` so the handler runs synchronously *and* the webhook returns 200 only after dispatch completes (acceptable since Telegram allows up to 60s before retry). Or wrap in our own goroutine with a timeout that lives in main's context tree.
+
+### H3 — Webhook body read is unbounded
+**File:** `internal/telegram/webhook.go:41`.
+
+`json.NewDecoder(r.Body).Decode(...)` reads until the body closes. A malicious client (we already 401'd unauthorized requests, but the body is read… wait, no — body is read *after* the auth check, so this is mostly an authorized-Telegram concern). Still, defensive:
+
+```go
+r.Body = http.MaxBytesReader(w, r.Body, 1<<20) // 1 MiB
+```
+
+Telegram updates with media are bounded; 1 MiB is plenty.
+
+### H4 — Webhook secret WARN should be a fatal at startup
+**File:** `internal/telegram/webhook.go:27–28`.
+
+If `TELEGRAM_WEBHOOK_SECRET` is empty, the handler logs once and rejects all requests. The bot is broken but the process keeps running, returning 401 to every Telegram update. Telegram retries fail. Better: `log.Fatal` at startup if secret is empty (force-fail-fast) — `cmd/server/main.go:22-24` already has the same shape for the bot token.
+
+### H5 — All env vars (including secrets) leak to every module via `Deps.Env`
+**File:** `cmd/server/main.go:91–106`, `internal/modules/module.go:62`.
+
+`loadConfig` builds `envMap` from the entire process environment, then stuffs it into `deps.Env`. `TELEGRAM_BOT_TOKEN` and `TELEGRAM_WEBHOOK_SECRET` are in there. Every module factory receives the full bag. A future module that logs `deps.Env` (for debugging) — or worse, reflects it into a Telegram message — leaks the bot token. With Firestore creds and Gemini keys arriving in Phase 04/07, this surface only grows.
+
+Fix: redact known-sensitive keys before passing to modules, or restructure `Deps.Env` to be per-module-allowlisted (`MODULES` env names a module; that module gets only its own `MODULE_<NAME>_*` vars).
+
+Minimum acceptable today: hardcode a deny-list:
+```go
+for _, k := range []string{"TELEGRAM_BOT_TOKEN", "TELEGRAM_WEBHOOK_SECRET"} {
+    delete(envMap, k)
+}
+```
+
+### H6 — Module names from `MODULES` env are not validated; `:` in a name breaks KV isolation
+**Files:** `cmd/server/main.go:110–122`, `internal/storage/prefix.go:11–16`, `internal/modules/registry.go:69`.
+
+`splitCSV` only trims whitespace. `Prefixed(s, "a:b")` produces prefix `"a:b:"`. A separate module `"a"` writes to key `"b:foo"` → underlying key `"a:b:foo"` — the same as module `"a:b"` writing to `"foo"`. Two modules sharing storage breaks the isolation invariant the framework otherwise enforces.
+
+Either:
+- Validate module names at `Build` start (same regex as commands: `^[a-z0-9_]{1,32}$`).
+- Or assert factory map keys conform at package init (every key in `Factories` is checked).
+
+### H7 — `bot.New` blocks 5s on Telegram API at startup
+**File:** `internal/telegram/client.go:10`.
+
+`bot.New(token)` calls `GetMe` synchronously (library `bot.go:91–99`, 5s default timeout) unless `WithSkipGetMe()` is passed. On a Cloud Run cold start with transient network blip, the bot **fails to start**, the container exits, and Cloud Run retries — a self-DoS during Telegram API outages. Cold-start budget is 500ms (per phase 02 spec). 5s eats that 10x over.
+
+Pass `bot.WithSkipGetMe()` in production. Token validity is independently asserted on the first outgoing call.
+
+---
+
+## Medium
+
+### M1 — Duplicate names in `MODULES` env produce confusing "command conflict" error
+**File:** `internal/modules/registry.go:69–113`.
+
+`MODULES="alpha,alpha"` calls `factories["alpha"]` twice. Second iteration's commands collide with first → `command conflict: /a1 defined in "alpha" and "alpha"`. The user sees the same name twice and has to deduce the cause.
+
+Fix: dedupe `enabled` first, or fail fast with `fmt.Errorf("duplicate module %q in MODULES", name)`.
+
+### M2 — Spec deviation in `DispatchScheduled` (concurrent errgroup) not documented
+**Files:** spec phase-03 step 6 (errgroup; multiple modules can claim same name); `cron_dispatcher.go:11`.
+
+Phase 03 spec says crons run "across all modules concurrently (errgroup)". Current implementation enforces cron-name uniqueness across modules (`cronOwners` in registry.go:106), then runs the single matched handler. This is a stronger invariant — fine — but worth a comment in `cron_dispatcher.go` so the next reader doesn't reintroduce errgroup. (The user's deviation list called out factory shape and module map; cron uniqueness is undocumented.)
+
+### M3 — Single-handler `WriteTimeout` straddles webhook (fast) and cron (slow) routes
+**File:** `cmd/server/main.go:57`.
+
+`WriteTimeout: 6 * time.Minute` accommodates the cron path. The webhook path inherits this 6-minute leash; if a handler hangs (nil deref + library async dispatch), the request stays open 6 minutes, holding a Cloud Run instance against its concurrency cap (free tier: 1 inflight request per instance per the plan).
+
+Use per-handler `http.TimeoutHandler` wrapping just `/webhook` at, say, 10s. Cron stays at the server-level 6-minute cap.
+
+### M4 — Log injection via cron name (and update fields)
+**Files:** `internal/server/router.go:52,61`.
+
+`name` comes from `r.URL.Path` (already %-decoded by net/http). Validation only blocks `/`. Newlines, ANSI escapes, etc. would let an attacker forge log lines. Log shippers like Cloud Logging treat each line as a JSON record; a smuggled `\n{"severity":"CRITICAL",...}` line would forge a fake event.
+
+Fix: validate `name` against `^[a-z0-9_]{1,32}$` (same regex as commands). 404 anything else. Bonus: fail-fast on attempts to dispatch unregistered crons before any logging.
+
+### M5 — Dead code: `errors.Is(err, http.ErrBodyReadAfterClose)`
+**File:** `internal/telegram/webhook.go:42–44`.
+
+`http.ErrBodyReadAfterClose` is documented as "returned by Read after the body has been closed", which only happens if the handler reads `r.Body` post-return. `json.Decoder` reads synchronously inside the handler; this branch is unreachable. Remove it (or replace with a generic decode-error log if observability matters).
+
+### M6 — `CommandHandler` returning `error` is decorative
+**Files:** `internal/modules/module.go:27`, `internal/modules/dispatcher.go:22–26`.
+
+The library's `bot.HandlerFunc` is `func(ctx, *Bot, *Update)` — no error return. Our wrapper logs `cmdCopy.Handler` errors and that's it. The `error` return offers no flow control: no retry, no metrics, no Telegram-side signal. It's a comfortable pattern for handler authors but creates the illusion of error handling.
+
+Either:
+- Document explicitly in `Command.Handler` godoc: "returned errors are logged and otherwise ignored; do not rely on them to retry or alert".
+- Or hook in a metrics counter / error reporter at the central log site so the field has teeth.
+
+### M7 — `Registry.AllCommands` is a public mutable map
+**File:** `internal/modules/registry.go:14`.
+
+Build returns a `*Registry` whose `AllCommands` field is a `map[string]Command`. Callers can mutate it post-construction. The dispatcher relies on Build-time validation; a downstream `reg.AllCommands["evil"] = Command{...}` after `Install(b, reg)` is silent because handlers are already registered with `bot`, but anyone reading `reg.AllCommands` for a `/help` listing would see ghost commands. Either lock fields private + expose accessor, or document the invariant.
+
+---
+
+## Low
+
+### L1 — `mod.Name` is overwritten after factory returns
+**File:** `internal/modules/registry.go:81`.
+
+`mod.Name = name` clobbers whatever the factory set. Today factories set `Name: name` themselves (`registry_test.go:34`); the assignment is defensive but undocumented. The factory's `Module.Name` field becomes effectively meaningless. Either drop the field from `Module` (it's the registry key, not a module property) or document the override.
+
+### L2 — `MemoryKVStore.List` on empty prefix returns *all* keys
+**File:** `internal/storage/memory_kv.go:68–79`.
+
+`strings.HasPrefix(k, "")` is always true. Through `Prefixed(_, "modA").List(ctx, "")` this reduces to `inner.List("modA:")` which is correct, but raw `MemoryKVStore.List(ctx, "")` returns the whole keyspace. If a test or smoke runner uses the bare store, it will see everything. Document or refuse empty prefix.
+
+### L3 — `Registry.Cron` returns `Cron` by value but also stores by value
+**File:** `internal/modules/registry.go:31–34`.
+
+Fine functionally. If `Cron` ever grows to hold mutable state (rate limiter, last-run timestamp), value-copy semantics will surprise the next author. Worth a comment that `Cron` is value-typed deliberately.
+
+### L4 — `splitCSV` empty-vs-empty edge: `MODULES=,` returns `nil`, `MODULES=` returns `nil`, `MODULES=alpha,,beta` returns `["alpha","beta"]`
+**File:** `cmd/server/main.go:110–122`.
+
+Behaviorally fine (skip blanks), but no test exercises any of these paths. One table test would lock it down.
+
+### L5 — `ErrCronNotFound` declared with `fmt.Errorf` instead of `errors.New`
+**File:** `internal/modules/cron_dispatcher.go:20`.
+
+`fmt.Errorf("cron not found")` works but doesn't use a format directive. `errors.New` is the more idiomatic choice and signals "this is a sentinel".
+
+---
+
+## Nit
+
+### N1 — `Module.Init` field referenced in spec but absent from code
+The spec (phase-03.md:64) lists `Init` as an optional field on `Module`. Implementation drops it (factory-takes-deps pattern eliminates the need). Already covered in user's deviation list (#1) — no action needed, but cross-reference in `module.go` godoc would help future readers.
+
+### N2 — `ErrCronNotFound` could include the registry's known cron names in the error
+For diagnostics, errors at the dispatcher layer could list available names: `cron %q not found; available: [a, b, c]`. Phase 09 will add proper observability; minor today.
+
+### N3 — `cmd/server/main.go:42–43` log line reads cleanly but accesses `reg.Crons()` which sorts on every call
+Currently called once at startup, no perf concern. Worth a comment that `Crons()` allocates a sorted copy if downstream code starts calling it per-request.
+
+### N4 — Dockerfile has no healthcheck
+`distroless/static:nonroot` has no shell, so `HEALTHCHECK CMD wget …` won't work without adding a busybox layer. Cloud Run uses HTTP startup probes anyway, so this is fine; no action.
+
+---
+
+## Test Gaps (would catch a regression)
+
+Existing tests cover registry conflict + validation, prefix round-trip, prefix-list-strip. Missing:
+
+1. **`TestWebhookHandler_*`** (no test file for `internal/telegram/`):
+   - Rejects non-POST → 405.
+   - Rejects missing/wrong secret → 401.
+   - Rejects malformed JSON → 400.
+   - Accepts valid update → 200, `b.ProcessUpdate` invoked exactly once.
+   - Constant-time compare verified by parametric byte-flip table (won't *prove* timing-safe but locks the API choice).
+
+2. **`TestCronHandler_*`** (no test for `internal/server/router.go`):
+   - 405 on GET, 404 on `/cron/`, 404 on `/cron/foo/bar`, 404 on unknown name (`ErrCronNotFound` → 404 mapping).
+   - Path-traversal smoke: `/cron/../etc` cleaned by net/http to `/etc`, never reaches handler. Worth a test that confirms.
+   - Log-injection guard: `/cron/foo%0Abar` → 404 (after M4 fix).
+
+3. **`TestMemoryKVStore_Concurrent`** — race-free when many goroutines Put/Get simultaneously. Trivial; pairs with `-race` in CI.
+
+4. **`TestPrefixedStore_PrefixCollision`** — module `"foo"` writing key `"o:bar"` does *not* show up in `Prefixed(_, "foo:o").List("bar")`. Locks the H6 invariant once names are validated.
+
+5. **`TestBuild_DedupesModules`** — once H6/M1 fixed, assert the chosen behavior (error vs silent dedupe).
+
+6. **`TestDispatchScheduled_PrefixedDeps`** — once C2 fixed, write a cron handler that puts to `deps.KV` and assert the underlying base store contains the prefixed key.
+
+7. **`TestInstall_RegistersAllCommands`** — assert `b.RegisterHandler` is called for each command. Library's `Bot` exposes `handlers` only privately; can wrap in a small test helper that registers a deterministic-match handler and feeds an update through `ProcessUpdate`.
+
+---
+
+## Positive Observations
+
+- Clean package layout, idiomatic small interfaces (`KVStore` is the right shape).
+- `Prefixed` wrapper is a tidy 50-line solution; tests prove round-trip + isolation.
+- Validation error messages mention the offending module — saves debugging time.
+- `splitCSV` handles whitespace + empty entries gracefully without external libs.
+- Dockerfile is correct: distroless + nonroot + `-s -w` + `CGO_ENABLED=0`. 6.4 MB binary is well under the 15 MiB target.
+- CI runs `-race -count=1` from day one.
+- Spec deviations are documented in code comments (factory shape, factory map). Easy for next reviewer.
+
+---
+
+## Recommended Action Order
+
+| # | Severity | Action |
+|---|----------|--------|
+| 1 | C1 | Delete `go b.Start(rootCtx)` in main.go:64 |
+| 2 | C2 | Fix cron dispatcher to pass per-module-prefixed Deps |
+| 3 | C3 | Replace webhook secret compare with `subtle.ConstantTimeCompare` |
+| 4 | H1 | Gate `/cron/{name}` behind shared-secret env check until Phase 09 |
+| 5 | H2 | Detach context for `ProcessUpdate` (or switch to `WithNotAsyncHandlers`) |
+| 6 | H5 | Strip secrets from `Deps.Env` |
+| 7 | H6 | Validate module names with the command regex |
+| 8 | H7 | `bot.New(token, bot.WithSkipGetMe())` |
+| 9 | H3 / H4 | `MaxBytesReader` + `log.Fatal` on empty secret |
+| 10 | M-class | Address as time allows; M2/M5 are doc-only |
+
+C1, C2, C3 are blocking — they will manifest as production incidents the moment Phase 05 ships any real module.
+
+---
+
+## Unresolved Questions
+
+1. **OIDC vs shared-secret for cron:** Phase 09 plans OIDC. Is a temporary shared-secret bridge acceptable, or should `/cron/*` be entirely disabled (404'd at the router) until Phase 09 lands? The latter is safer; the former lets Phase 05 modules write crons that can be smoke-tested locally.
+2. **Async vs sync handler dispatch:** the library defaults async, we want bounded handler execution. Is `bot.WithNotAsyncHandlers()` the agreed pattern, or do we accept the goroutine-per-update model and need a separate concurrency cap? Affects H2's resolution.
+3. **`Module.Name` field:** keep for symmetry with JS port, or drop now that it's unused? Affects L1 + future module authoring docs.
+4. **`Deps.Env` policy:** redact known-sensitive keys (cheap), or restructure to per-module allowlist (correct)? H5's resolution depends on this.
+
+---
+
+**Status:** DONE_WITH_CONCERNS
+**Summary:** Scaffolding is sound; three real bugs (long-poll-vs-webhook, unprefixed cron Deps, non-constant-time secret compare) must land before Phase 05 ships any module.