GmRelayBot/docs/review-report.md

# Code review — feat/issue-112-wizard-refactor (issue #112)

**Reviewer:** Verifier (mvs_86868b01387b492aae27ce6f77aca4cb)
**Branch:** `feat/issue-112-wizard-refactor` (base `origin/main`)
**Commits reviewed:** `8f0f2ef`, `b81d865`, `f095209`, `7cfb196`, `c4a77d3`
**Build:** ✅ `dotnet build GM-Relay.slnx` — 0 warnings, 0 errors
**Tests:** 580 passed / 2 skipped / 1 failed. 1 failure is the pre-existing
`DiscordProjectStructureTests.Version_ShouldBeSynchronizedForDiscordFeatureRelease`
(uncommitted release work in working tree, not part of this branch).

## VERDICT: REQUEST_CHANGES

The branch is **NOT shippable in its current state.** Every choice button
and every "Другое…" button in the wizard is silently broken at runtime
due to a wire-format mismatch between the renderer and the dispatcher.
A user who clicks "D&D 5e", "Pathfinder 2e", "Waitlist вкл", "Опубликовать",
or any "Другое… ✏️" button will see "⚠️ Неизвестная кнопка" instead of
the wizard advancing. The 12 source-level smoke tests don't catch this
because they only check string presence in source code, not the actual
button-click → dispatch flow.

The architecture is otherwise sound: no Telegram.Bot/NetCord leak into
Shared, single state-machine source, all DI wired, AOT-safe, parameterized
SQL, owner/co-GM permission check with null-safety, SecretRedactor on the
connection string. The fix is small and surgical.

---

## Critical findings

### C-1. Choice-button custom-id is missing the `choice:` segment — wizard is unusable end-to-end

**Files:**
- `src/GmRelay.DiscordBot/Features/Sessions/Wizard/DiscordWizardStep.cs:79-80`
- `src/GmRelay.DiscordBot/Features/Sessions/Wizard/DiscordWizardInteractionModule.cs:174-226`

**What's wrong.** The dispatcher's button handler matches `parts[1]`
against `"choice"`, `"back"`, `"cancel"`, `"create"`, `"resume"`, and
falls through to "default → Неизвестная кнопка" for anything else. The
dispatcher's own documentation and the deliverable's wire-format table
both agree the canonical choice-button format is
`wizard:btn:choice:<step>:<value>`. But `ButtonCustomId` emits
`$"wizard:btn:{step}:{value}"` — **the literal `choice:` segment is
missing**. So clicking "D&D 5e" on the System step produces
`wizard:btn:System:Dnd5e`, which NetCord strips the `[ComponentInteraction("wizard")]`
prefix from, arriving at the dispatcher as `args = "btn:System:Dnd5e"` →
`parts = ["btn", "System", "Dnd5e"]` → `parts[1] = "System"` → default
branch → "⚠️ Неизвестная кнопка".

The same bug hits:
- `RenderType` — "Одну игру" / "Пул игр" buttons (emits
  `wizard:btn:Type:single`, `wizard:btn:Type:pool`)
- `RenderSystem` — D&D/Pathfinder/CoC/GURPS/Fate ("wizard:btn:System:Dnd5e" etc.)
  **and** the "⏭ Пропустить" button (emits `wizard:btn:System:_skip`)
- `RenderDuration` — "3 часа" / "4 часа" / "5 часов" / "6 часов" and
  "⏭ Пропустить"
- `RenderCapacity` / `RenderPoolSlotCapacity` — "Waitlist вкл" / "Без waitlist"
  (emits `wizard:btn:Capacity:waitlist:on` etc.)
- `RenderPublish` — "Опубликовать" / "Только в чате"
- `RenderPoolAddSlots` — "Добавить слот" / "Готово, к превью"
- `RenderPickClub` — back/cancel still work (parts[1] = "back"/"cancel")

Only back, cancel, create, resume, and the "Другое… ✏️" → modal buttons
are unaffected by *this specific* bug (see C-2 for modal buttons).

The smoke test `Dispatcher_ShouldParseAllWizardActionKinds`
(`tests/GmRelay.Bot.Tests/Discord/DiscordWizardInteractionModuleSourceTests.cs:85-97`)
checks that the strings `"choice"`, `"back"`, `"cancel"`, `"create"`,
`"resume"` appear in `DiscordWizardInteractionModule.cs`. It doesn't
check the renderer's output, so the bug is invisible to the test suite.
The same file's comment at line 69 documents the *expected* format as
`"btn:choice:Type:single"` — which would be the correct fix.

**How to fix.** Change `ButtonCustomId` in `DiscordWizardStep.cs`:

```csharp
public static string ButtonCustomId(string step, string value) =>
    $"wizard:btn:choice:{step}:{value}";
```

This brings the renderer into alignment with the dispatcher's switch
case `"choice"` (line 209) and with the deliverable's table at line 83.
Re-verify with a manual click-through of every button on every step, or
add a parser-side test (see I-3 below).

### C-2. "Другое… ✏️" modal trigger buttons route to "default" instead of opening a modal

**File:** `src/GmRelay.DiscordBot/Features/Sessions/Wizard/DiscordWizardStep.cs:190, 206, 314`
**Read against:** `src/GmRelay.DiscordBot/Features/Sessions/Wizard/DiscordWizardInteractionModule.cs:207-226`

**What's wrong.** The renderer emits modal triggers as
`wizard:btn:modal:SystemFreeText` (the "Другое… ✏️" button on the System
step), `wizard:btn:modal:DurationFreeText` (Duration step), and
`wizard:btn:modal:PoolSystemDurationFreeText` (PoolSystemDuration step).
The dispatcher's button switch handles `"choice"`, `"back"`, `"cancel"`
but not `"modal"`. The user's click on "Другое…" hits the default branch
and returns "⚠️ Неизвестная кнопка" — no modal pops up, the wizard
doesn't advance. The open question in `deliverable.md:125-132` ("Modal
handler's free-text mapping is a hack") implicitly assumes these buttons
*work* in production, so the design intent is clear but the implementation
didn't deliver it.

**How to fix.** Add a `"modal"` case in the dispatcher's switch (between
`"create"` and the existing branches, mirroring the "create" / "resume"
special-case pattern):

```csharp
if (parts[1] == "modal" && parts.Length >= 3)
{
    var modal = DiscordWizardStep.BuildModal(parts[2], draft.ChatId);
    if (modal is not null)
    {
        await context.Interaction.SendResponseAsync(InteractionCallback.Modal(modal));
    }
    else
    {
        await AckWithErrorAsync(context.Interaction, "Модал недоступен");
    }
    return;
}
```

This bypasses the wizard's state machine entirely (the user's intent is
"open a modal for free-text input", not "advance the wizard"). When the
user submits the modal, `HandleModalAsync` will run, which already knows
how to map `SystemFreeText` → `WizardStepNames.System` (line 453-462).
Add a click-through test for at least one of the three steps.

### C-3. `ex.Message` from `CreateSessionHandler` is shipped to the user's Discord

**File:** `src/GmRelay.DiscordBot/Features/Sessions/Wizard/DiscordWizardSubmitter.cs:104`

**What's wrong.** On submit failure the submitter edits the draft
message with `$"💥 Ошибка: {ex.Message}\nПопытка {payload.RetryCount}/{MaxRetries}."`
and ships it to the user-visible draft embed. The exception originates
in `CreateSessionHandler` which talks to PostgreSQL via Dapper. Postgres
exception messages routinely include the constraint name, the conflicting
key value, and sometimes the full SQL text. Even the connection-string
DSN could leak if an `NpgsqlException` wraps a connection failure. A
malicious user who can submit many sessions can probe DB schema and
state by reading the error strings.

**How to fix.** Log the full `ex` to the server-side log (already done
on line 86) but show the user a generic error:

```csharp
await EditDraftMessageAsync(
    draft,
    $"💥 Ошибка при создании сессии. Попытка {payload.RetryCount}/{MaxRetries}. "
    + "Попробуйте повторить или обратитесь к администратору.",
    RetryCancelActions(),
    ct);
```

If you want to preserve a per-error recovery hint (e.g. "Duplicate
title — pick a different name"), map known exception types to localized
strings; never embed the raw `ex.Message`.

---

## Important findings

### I-1. `Owner`/`CoGm` permission lookup runs on every wizard invocation, no cache

**File:** `src/GmRelay.DiscordBot/Features/Sessions/Wizard/DiscordWizardCommand.cs:85-87`

The slash command issues an `await DiscordPermissionLookup.LoadManagerUserIdsAsync(...)`
on every `/newsession-wizard` invocation. This is a 3-table join that
scales linearly with the number of clubs the user manages. With a 24-hour
draft lifetime and a single draft per owner, the same query repeats
frequently. Not critical for the v3.8.0 release, but a 30-second in-memory
cache would cut DB load noticeably during heavy wizard use. Same query
shape lives in `DiscordNewSessionHandler` already (per the file comment),
so a shared cache would benefit both.

### I-2. `_skip` sentinel bypasses the wizard's own validation

**File:** `src/GmRelay.DiscordBot/Features/Sessions/Wizard/DiscordWizardStep.cs:191, 207`
**Read against:** `src/GmRelay.Shared/Features/Sessions\CreateSession\Wizard\GameCreationWizard.cs:287-290`

`GameCreationWizard.ApplySystemChoice` matches `"_skip"` and accepts it
without further checks. The renderer emits `wizard:btn:System:_skip`.
This is correct *if* the choice-button wire format gets fixed (C-1); the
"`_skip`" string is hard-coded in the wizard and the renderer uses the
same constant. But there's no central constant — both files have their
own copies of the magic string. A future refactor that renames the
sentinel in one place will silently break the other. Suggest
`public const string SkipSentinel = "_skip"` on a shared class.

### I-3. Smoke tests are string-matching only — no behaviour coverage of the adapter

**File:** `tests/GmRelay.Bot.Tests/Discord/DiscordWizardInteractionModuleSourceTests.cs`

All 12 smoke tests in this file are `Assert.Contains` against the
source text. They would all pass against a file full of `// choice`
comments and dead code. The test class header at line 8-17 acknowledges
this ("smoke gate"), and the broader Wizard test suite
(`tests/GmRelay.Bot.Tests/Features/Sessions/CreateSession/Wizard/`) does
exercise the platform-neutral state machine — but the *adapter* (the
mapping from `ButtonInteractionContext` → `_wizard.HandleInteractionAsync`)
has zero behavioural coverage. C-1 and C-2 both bypass the entire
smoke-test surface.

Minimum bar to add: a parser-roundtrip test that takes the renderer's
output for each `RenderX()` step and feeds it through the dispatcher's
button handler to verify it doesn't fall into the default branch. Even
a hand-rolled `ButtonInteractionContext` fake (or a helper that mimics
the dispatcher's `args.Split(':', 4)` parser) would catch both bugs.
Cost: ~50 lines; payoff: catches the entire class of "renderer and
dispatcher disagree on the wire format" regressions.

### I-4. `AddComponentInteractions<TInteraction, TContext>` is called for Modal but the renderer relies on `Label → TextInput` layout

**File:** `src/GmRelay.DiscordBot/Features/Sessions/Wizard/DiscordWizardStep.cs:436-444`
**Read against:** `DiscordWizardInteractionModule.cs:440-451`

`BuildModal` always wraps a single `TextInput` in a `Label` (Discord's
`IModalComponentProperties` API requires labels). The dispatcher reads
`Components[0]` and assumes it is a `Label` (line 446). This is
consistent *today*, but if a future step needs two inputs in one
modal, the extraction logic needs to walk all components, not just
`[0]`. Document the constraint on the dispatcher's
`ExtractModalText` method ("current contract: exactly one Label,
one TextInput") and the renderer's `BuildModal` ("emits one
Label+TextInput, no exceptions").

### I-5. The 3-retry counter is bound to the in-memory `WizardPayload`, not the DB row

**File:** `src/GmRelay.DiscordBot/Features/Sessions/Wizard/DiscordWizardSubmitter.cs:86-89`

`payload.RetryCount += 1; SavePayload(draft, payload);` is called *before*
the `if (payload.RetryCount >= MaxRetries)` check. The counter is
serialized into `draft.PayloadJson` and re-loaded on the next click, so
the bound is correct across bot restarts. However, the in-memory
`draft` object is shared with the dispatcher's `_wizard` after
`HandleInteractionAsync` returns, and a future refactor that pulls the
payload from the DB instead of the in-memory copy could see a stale
count. Document the invariant: "RetryCount is read from the in-memory
payload after this line; do not re-load from DB before the comparison."

### I-6. `BuildResumeRow` re-uses the same customId suffix scheme as the in-wizard buttons

**File:** `src/GmRelay.DiscordBot/Features/Sessions/Wizard/DiscordWizardCommand.cs:193-209`

`BuildResumeRow` emits three buttons:
- "▶️ Продолжить" → `wizard:btn:resume:continue` ✓ (works)
- "🔄 Заново" → `wizard:btn:resume:restart` ✓ (works)
- "❌ Отмена" → `wizard:btn:cancel:1` (uses `DiscordWizardStep.ButtonCustomId("cancel", "1")`)

The cancel button relies on the dispatcher's `parts[1] == "cancel"`
match. With C-1 fixed, this still works because cancel is in the
switch, not the new "choice" path. But the `ButtonCustomId` signature
will change semantics after C-1: it will become
`$"wizard:btn:choice:{step}:{value}"`. `BuildResumeRow` passing
`"cancel"` as the step will then produce `wizard:btn:choice:cancel:1`,
which the dispatcher's switch will not match (no `"choice"` in the
parts). Fix C-1 must update `BuildResumeRow` to emit
`wizard:btn:cancel:1` directly (not via `ButtonCustomId`).

---

## Nits

- `DiscordWizardInteractionModule.cs:308, 357` — the select and modal
  handlers split args with `Split(':', 2)` (max 2 parts), while the
  button handler uses `Split(':', 4)` (max 4 parts). Inconsistent. The
  net effect is correct (select has 2 segments, modal has 2, button has
  2–4), but a comment explaining the max-count rationale would help.
- `DiscordWizardStep.cs:74` — `throw new InvalidOperationException` on
  unknown step is fine for now, but a future maintainer adding a step
  to `WizardStepNames` will get a runtime exception. A `switch` exhaustiveness
  check (e.g. a private static assert in tests) would catch this at
  build time.
- `DiscordPermissionLookup.cs:28-30` — `g.platform = 'Discord'` is
  hard-coded in the SQL. A `g.platform = @Platform` parameter would
  mirror the dispatcher's parameterized style and make the helper
  reusable for any future platform. Not blocking.
- `DiscordWizardCommand.cs:72-81` — fetching the guild via REST inside
  the slash command costs an extra round-trip. The
  `resolvedPermissions` from the interaction already includes
  `Administrator`; only the "guild owner" case needs the REST call.
  Consider short-circuiting when `(resolvedPermissions & Administrator)
  != 0`.
- `DiscordWizardMessenger.cs:188-192` — hard-coded
  `new Color(0x5865F2)` (Discord blurple). Extracting to a const
  `WizardEmbedColor` would let the Web/Telegram versions use the same
  brand color if they ever render wizards.

---

## Migration V032 sanity check

**File:** `src/GmRelay.Bot/Migrations/V032__wizard_drafts_platform.sql`

- Line 8-9 `ADD COLUMN platform TEXT NOT NULL DEFAULT 'Telegram'`
  — DEFAULT literal makes this O(1) on PostgreSQL ≥ 11. Safe on
  existing rows.
- Lines 13-30 — `ALTER COLUMN ... TYPE TEXT USING ...::TEXT` on the
  `chat_id`, `message_thread_id`, `draft_message_id`, and renamed
  `owner_id` columns. All conversions are lossless
  (BIGINT → decimal-string, INT → decimal-string). Safe.
- Line 26-27 `RENAME COLUMN owner_telegram_id TO owner_id` — the
  rename happens mid-migration. Any DML hitting the table
  concurrently that uses the old name will fail. For a bot that
  processes both Telegram and Discord traffic, this is a brief
  exclusivity lock. Consider splitting into two migrations
  (rename + new index, then type change) so each lock is shorter.
  Not blocking for v3.8.0, but document the brief lock window.

No DEFAULT-cascade issue, no NOT NULL on existing-row failure. The
deliverable's "Will this fail on existing rows?" question gets a
"no, but plan a maintenance window" answer.

---

## Architecture sanity (re-confirmed)

- `src/GmRelay.Shared/` — only references to Telegram.Bot/NetCord are
  in doc comments warning the developer not to add them. csproj has
  no Telegram.Bot or NetCord package references. `GameCreationWizard`,
  `IWizardMessenger`, `WizardCallbackData`, `WizardStepLimits`,
  `WizardStepNames`, `WizardPayload`, `WizardDraft` are all in exactly
  one place (Shared). ✓
- `src/GmRelay.DiscordBot/Program.cs:87-102` — all 7 wizard services
  registered as singleton. All 3 `AddComponentInteractions<...>`
  calls present (Button, StringMenu, Modal). All 4 module/dispatcher
  classes (`WizardInteractionDispatcher`, `DiscordWizardButtonModule`,
  `DiscordWizardStringMenuModule`, `DiscordWizardModalModule`)
  registered. ✓
- AOT-safety: no `System.Reflection`, no `dynamic`, no
  `Activator.CreateInstance`, no `Type.GetType` in the new Discord
  or Shared code. ✓
- `DiscordPermissionLookup.cs:23-31` and
  `DiscordWizardMessenger.cs:154-165` and the inline
  `WizardClubLookup` in `DiscordWizardInteractionModule.cs:508-519`
  all use parameterized queries (`@GuildId`, `@Platform`,
  `@ExternalId`, `@OwnerId`). No SQL string interpolation. ✓
- `Program.cs:54` — `SecretRedactor.RedactConnectionString` on the
  startup log. ✓
- `DiscordWizardCommand.cs:51-94` — DM invocations rejected
  (`GuildId` null check), channel null-checked, member type-checked
  via `as GuildInteractionUser`, owner/admin/DB-manager permission
  check via `DiscordPermissionChecker.CanManageSchedule`. No NRE
  on `Context.User`. ✓

---

## Summary

Strong foundation: the platform-neutral refactor is well-executed, the
state machine has solid test coverage, the Discord adapter's DI graph
is clean, and security primitives (parameterized SQL, permission check,
secret redaction) are in place. But the Discord adapter's runtime path
is untested, and a single oversight in the renderer's button custom-id
format (missing the `choice:` segment) breaks every choice button in
the wizard at click time. The "Другое… ✏️" modal triggers are also
unrouted in the dispatcher, leaving the free-text input path
unreachable. The 3-attempt finalize loop works but leaks `ex.Message`
to the user. After fixing C-1 / C-2 / C-3, adding I-3 (behavioural
test of the adapter), and re-running the manual click-through checklist
(System → Duration → DateTime → Capacity → Visibility → Publish →
Confirm for Single; full pool flow), this branch is ready to merge.