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:

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):

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:

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.