diff --git a/plans/260824-1051-sticker-pack-module/phase-01-shared-prerequisites.md b/plans/260824-1051-sticker-pack-module/phase-01-shared-prerequisites.md index cf3de27..9123d2d 100644 --- a/plans/260824-1051-sticker-pack-module/phase-01-shared-prerequisites.md +++ b/plans/260824-1051-sticker-pack-module/phase-01-shared-prerequisites.md @@ -1,7 +1,7 @@ --- phase: 1 title: "Phase 1: Shared prerequisites" -status: todo +status: done priority: P1 effort: "4h" dependencies: [] @@ -122,26 +122,26 @@ is not entangled with sticker logic. ## Todo -- [ ] `recover()` in the command closure with `metrics.IncError("handler-panic")` -- [ ] `recover()` in the callback closure with guarded `AnswerCallbackQuery` -- [ ] Fix the stale `recover()` comment at `dispatcher.go:167` -- [ ] `RecordingBot.StubMethod(method, resultJSON)` -- [ ] `RecordingBot.FailMethodCode(method, errorCode, description)` -- [ ] Document that `FailMethod` produces a codeless failure -- [ ] `dispatcher_panic_test.go`: panicking command handler -- [ ] `dispatcher_panic_test.go`: panicking callback handler -- [ ] `recording_bot_test.go`: stubbed `getStickerSet` decodes into `models.StickerSet` -- [ ] `recording_bot_test.go`: `FailMethodCode` yields `bot.ErrorBadRequest` +- [x] `recover()` in the command closure with `metrics.IncError("handler-panic")` +- [x] `recover()` in the callback closure with guarded `AnswerCallbackQuery` +- [x] Fix the stale `recover()` comment at `dispatcher.go:167` +- [x] `RecordingBot.StubMethod(method, resultJSON)` +- [x] `RecordingBot.FailMethodCode(method, errorCode, description)` +- [x] Document that `FailMethod` produces a codeless failure +- [x] `dispatcher_panic_test.go`: panicking command handler +- [x] `dispatcher_panic_test.go`: panicking callback handler +- [x] `recording_bot_test.go`: stubbed `getStickerSet` decodes into `models.StickerSet` +- [x] `recording_bot_test.go`: `FailMethodCode` yields `bot.ErrorBadRequest` ## Success Criteria -- [ ] A command handler that panics is recovered; the test process survives and the error metric increments -- [ ] A callback handler that panics is recovered and the callback query is still answered -- [ ] `rg "recover\(\)" internal/modules/dispatcher.go` returns two hits -- [ ] No comment in the repo claims a `recover()` exists in `webhook.go` -- [ ] `rb.StubMethod("getStickerSet", ...)` lets `b.GetStickerSet` return a populated `*models.StickerSet` with a nil error -- [ ] `rb.FailMethodCode("getStickerSet", 400, "Bad Request: STICKERSET_INVALID")` produces an error satisfying `errors.Is(err, bot.ErrorBadRequest)` -- [ ] `go test ./...` passes with no changes to any existing test file other than additions +- [x] A command handler that panics is recovered; the test process survives and the error metric increments +- [x] A callback handler that panics is recovered and the callback query is still answered +- [x] `rg "recover\(\)" internal/modules/dispatcher.go` returns two hits +- [x] No comment in the repo claims a `recover()` exists in `webhook.go` +- [x] `rb.StubMethod("getStickerSet", ...)` lets `b.GetStickerSet` return a populated `*models.StickerSet` with a nil error +- [x] `rb.FailMethodCode("getStickerSet", 400, "Bad Request: STICKERSET_INVALID")` produces an error satisfying `errors.Is(err, bot.ErrorBadRequest)` +- [x] `go test ./...` passes with no changes to any existing test file other than additions ## Risk Assessment diff --git a/plans/260824-1051-sticker-pack-module/phase-02-store-setname-emoji.md b/plans/260824-1051-sticker-pack-module/phase-02-store-setname-emoji.md index 16a0f60..93b4335 100644 --- a/plans/260824-1051-sticker-pack-module/phase-02-store-setname-emoji.md +++ b/plans/260824-1051-sticker-pack-module/phase-02-store-setname-emoji.md @@ -1,7 +1,7 @@ --- phase: 2 title: "Phase 2: Store, set names, emoji parsing" -status: todo +status: done priority: P1 effort: "3h" dependencies: [1] @@ -146,29 +146,29 @@ from a replied sticker yields at most one element. ## Todo -- [ ] Define `Pack` incl. `Count` and `Pending`; assert no reserved-bson collision -- [ ] `packKey(ownerID)` and `getPack` returning a found flag -- [ ] `senderID` rejecting nil/zero/`IsBot`/`SenderChat` with an explanatory message -- [ ] `slugRe` validation incl. `__`, trailing `_`, 40-char cap -- [ ] `makeSetName` with 64-char guard and budget-reporting error -- [ ] `ownsSet` case-insensitive match against stored `Pack.Name` -- [ ] `usernameResolver` caching success but never failure, taking the handler's `b` -- [ ] `parseEmoji` cluster scanner with the 20-entry cap -- [ ] Four test files per the success criteria +- [x] Define `Pack` incl. `Count` and `Pending`; assert no reserved-bson collision +- [x] `packKey(ownerID)` and `getPack` returning a found flag +- [x] `senderID` rejecting nil/zero/`IsBot`/`SenderChat` with an explanatory message +- [x] `slugRe` validation incl. `__`, trailing `_`, 40-char cap +- [x] `makeSetName` with 64-char guard and budget-reporting error +- [x] `ownsSet` case-insensitive match against stored `Pack.Name` +- [x] `usernameResolver` caching success but never failure, taking the handler's `b` +- [x] `parseEmoji` cluster scanner with the 20-entry cap +- [x] Four test files per the success criteria ## Success Criteria -- [ ] `getPack` for owner A never returns owner B's pack -- [ ] `getPack` on an unknown owner returns `found == false` and a nil error -- [ ] No `List` call exists anywhere in the module -- [ ] Slug table rejects leading digit, `__`, trailing `_`, 2 chars, 41 chars -- [ ] `makeSetName` errors when `len(slug)+len("_by_"+username) > 64` -- [ ] `ownsSet` matches `MyPack_by_Bot` against a stored `mypack_by_bot` -- [ ] `ownsSet` returns false for a set name belonging to another bot -- [ ] A simulated bot username change does **not** break `ownsSet` for an existing pack -- [ ] `senderID` rejects `IsBot: true` and a non-nil `SenderChat`, each with zero store access -- [ ] `parseEmoji` handles joined input, ZWJ family, flag, keycap, skin tone; rejects plain text; errors above 20 -- [ ] `gofmt -l internal/modules/sticker` empty; `go test`/`go vet` clean +- [x] `getPack` for owner A never returns owner B's pack +- [x] `getPack` on an unknown owner returns `found == false` and a nil error +- [x] No `List` call exists anywhere in the module +- [x] Slug table rejects leading digit, `__`, trailing `_`, 2 chars, 41 chars +- [x] `makeSetName` errors when `len(slug)+len("_by_"+username) > 64` +- [x] `ownsSet` matches `MyPack_by_Bot` against a stored `mypack_by_bot` +- [x] `ownsSet` returns false for a set name belonging to another bot +- [x] A simulated bot username change does **not** break `ownsSet` for an existing pack +- [x] `senderID` rejects `IsBot: true` and a non-nil `SenderChat`, each with zero store access +- [x] `parseEmoji` handles joined input, ZWJ family, flag, keycap, skin tone; rejects plain text; errors above 20 +- [x] `gofmt -l internal/modules/sticker` empty; `go test`/`go vet` clean ## Risk Assessment diff --git a/plans/260824-1051-sticker-pack-module/phase-03-pack-lifecycle.md b/plans/260824-1051-sticker-pack-module/phase-03-pack-lifecycle.md index 75ad607..be70c00 100644 --- a/plans/260824-1051-sticker-pack-module/phase-03-pack-lifecycle.md +++ b/plans/260824-1051-sticker-pack-module/phase-03-pack-lifecycle.md @@ -1,7 +1,7 @@ --- phase: 3 title: "Phase 3: Pack lifecycle commands" -status: todo +status: done priority: P1 effort: "7h" dependencies: [1, 2] @@ -42,6 +42,18 @@ Registry key is `sticker`; command names stay unprefixed — the registry keys c `handlerTimeout = 10 * time.Second` is a package constant; every handler opens with it. +> **Superseded during implementation — see plan.md, "post-implementation review: +> global slug reservation".** Step 5 below adopts an existing set on the strength +> of a `Pending` record for this owner and slug. Review proved that insufficient: +> the pack record is keyed by owner, so a user with *no* pack who types someone +> else's slug produces identical evidence and took over their pack. The shipped +> code adds a create-only global reservation (`slug:` → ownerID) written +> before Telegram is touched, and adopts only when it names the caller. Steps 4-7 +> read as implemented **except** that a reservation check precedes step 4, and +> step 5's "abort, delete the pending record" on an unknown error is wrong for +> the same reason rule 4 exists — the shipped code keeps both the intent and the +> reservation unless the refusal is positively classified. + ### `/newpack ` — write-ahead intent `` appears here and nowhere else in the module. It fixes the permanent share URL, and @@ -209,39 +221,39 @@ into prose by the Bot API server; everything else arrives as `Bad Request: `", zero API calls. +This reply is deliberately **not** the uniform `resolveOwned` refusal, and the difference is +not an oversight: `resolveOwned` is uniform because it answers a question about a set the +caller named, which may be someone else's. `/addsticker` answers only "do *you* have a pack", +about the caller's own state, and discloses nothing about anyone else. Being helpful here +costs no privacy. A `Pending` record counts as no usable pack — same reply, plus the +`/mypack` re-run hint from Phase 3. + Every argument is an emoji — there is no pack token to disambiguate, so a stray word is caught by `parseEmoji` and reported as a usage error rather than silently read as a pack name. @@ -84,10 +110,27 @@ by `parseEmoji` and reported as a usage error rather than silently read as a pac call. Take the per-user keylock. On success, increment `Pack.Count` and commit under `WithoutCancel`. `STICKERS_TOO_MUCH` maps to "your pack is full (120 stickers)". +**Unverified premise — settle before writing this handler.** The whole sticker-source path +assumes `AddStickerToSet` accepts a `file_id` for a sticker that lives in a set this bot did +not create. The Bot API documents `InputSticker.sticker` as accepting "a file_id as a String +to send a file that already exists on the Telegram servers", and says nothing further — but +unlike C1–C11 this was never checked against the live API, and every other API assumption in +this plan was. If Telegram rejects cross-set reuse, this path collapses into Phase 5's +machinery (`GetFile` → download → `UploadStickerFile` → use the returned `file_id`), which +inverts the 4→5 dependency and is much better known before the handler is written than after. +One live call settles it (plan R12). + ### `/delsticker` No arguments. `resolveOwned`, then `DeleteStickerFromSet{Sticker: fileID}`. On success, -decrement `Pack.Count` (floor 0) and commit. +decrement `Pack.Count` (floor 0) and commit under `WithoutCancel`. + +**Take the per-user keylock**, exactly as `/addsticker` does. Both run the same +read-modify-write on `Pack.Count`, so locking one and not the other would be a half-measure +that only looks safe. `/editsticker` and `/ordersticker` write nothing and take no lock. +Under the lock, the commit uses plain `Put` — the lock is what makes the read-modify-write +safe, so the `PutVersioned` reasoning from Phase 3 (which guards *creation*, not updates) +does not apply. Whether removing the final sticker also destroys the set is **not documented** in the Bot API docs or the open-source Bot API server, so the plan depends on neither answer. @@ -104,6 +147,21 @@ Corrected: **do not probe.** Decrement the count and stop. If the set really is command returns `STICKERSET_INVALID`, and the shared handler for that (Phase 3) deletes the record then — a positive signal, per plan rule 4. Simpler and strictly safer. +**Aftermath of a `Count: 0` pack — state the recovery route.** Not probing means that if +Telegram *did* destroy the set, the user is left holding a record for a pack that no longer +exists. `/mypack` makes zero API calls by design, so it cannot notice; `/newpack` refuses +because a record exists. The user is not wedged — `/delpack` calls `DeleteStickerSet`, gets +`STICKERSET_INVALID`, and Phase 3's self-heal drops the record, freeing `/newpack` — but +nothing in the plan told them that, and this phase is what creates the situation. So: when +the decrement lands on 0, the reply says the pack is now empty, that Telegram may have +removed it, and that `/delpack` clears it if `/addsticker` reports the pack is gone. + +This is also why the success criterion below is scoped rather than absolute. `/delsticker` +must never delete the `Pack` record on a **transient or unknown** error — that is finding R7, +the whole reason the probe was removed. It must still delete it on a **positive** +`STICKERSET_INVALID`, which is Phase 3's cross-command self-heal and the mechanism that +unwedges the user. An unqualified "never deletes the record" would forbid the fix. + ### `/editsticker <emoji...>` `resolveOwned`, `parseEmoji` (at least one required — an empty `emoji_list` is invalid), then @@ -136,27 +194,35 @@ size and a local copy would go stale. Its error goes through `replyAPIError`. ## Todo -- [ ] `resolveSource` sticker branch (photo branch stubbed for Phase 5) -- [ ] `resolveOwned` using `getPack` + `ownsSet`, with the 6-step gate and uniform refusal -- [ ] `/addsticker` with emoji precedence and `Count` increment -- [ ] `/delsticker` with `Count` decrement and **no** probe -- [ ] `/editsticker` requiring at least one emoji -- [ ] `/ordersticker` rejecting negatives locally only -- [ ] Register all four with `Parameters` metadata -- [ ] `resolve_test.go`, `sticker_handlers_test.go` +- [x] Settle the `file_id`-reuse premise against the live API before writing `/addsticker` +- [x] `resolveSource` with the full `(ctx, b, ownerID, msg)` signature, sticker branch only +- [x] `resolveSource` static-only gate: `IsAnimated`, `IsVideo`, `Type != "regular"` +- [x] `resolveOwned` using `getPack` + `ownsSet`, with the 6-step gate and uniform refusal +- [x] `/addsticker` with emoji precedence and `Count` increment +- [x] `/addsticker` no-pack and pending-pack replies, zero API calls +- [x] `/delsticker` with `Count` decrement, keylock, and **no** probe +- [x] `/delsticker` empty-pack reply naming the `/delpack` recovery route +- [x] `/editsticker` requiring at least one emoji +- [x] `/ordersticker` rejecting negatives locally only +- [x] Register all four with `Parameters` metadata +- [x] `resolve_test.go`, `sticker_handlers_test.go` ## Success Criteria -- [ ] Each command's happy path asserts the expected method in `RecordingBot.Sent()` -- [ ] Missing reply, non-sticker reply, and empty `set_name` each produce a usage error with zero API calls -- [ ] "No pack yet" and "sticker from another bot's set" produce **byte-identical** reply text, asserted by comparing the two replies to each other -- [ ] A sticker whose `SetName` differs only in case from the stored `Pack.Name` resolves successfully -- [ ] `/addsticker` with a non-emoji argument is rejected by `parseEmoji`, not silently reinterpreted -- [ ] `/ordersticker -1` rejected locally; `/ordersticker 999` reaches the API -- [ ] `/editsticker` with no emoji rejected locally -- [ ] `/delsticker` makes exactly one API call and never deletes the `Pack` record -- [ ] `/addsticker` and `/delsticker` move `Count` by exactly one, floored at 0 -- [ ] Animated/video sticker reply rejected before any API call +- [x] Each command's happy path asserts the expected method in `RecordingBot.Sent()` +- [x] Missing reply, non-sticker reply, and empty `set_name` each produce a usage error with zero API calls +- [x] "No pack yet" and "sticker from another bot's set" produce **byte-identical** reply text, asserted by comparing the two replies to each other +- [x] A sticker whose `SetName` differs only in case from the stored `Pack.Name` resolves successfully +- [x] `/addsticker` with a non-emoji argument is rejected by `parseEmoji`, not silently reinterpreted +- [x] `/ordersticker -1` rejected locally; `/ordersticker 999` reaches the API +- [x] `/editsticker` with no emoji rejected locally +- [x] `/delsticker` makes exactly one API call, and keeps the `Pack` record on a transient or unknown error +- [x] `/delsticker` receiving `STICKERSET_INVALID` **does** delete the record (Phase 3 self-heal), unblocking `/newpack` +- [x] `/addsticker` and `/delsticker` move `Count` by exactly one, floored at 0 +- [x] A `/delsticker` that lands on `Count: 0` names `/delpack` in its reply +- [x] `/addsticker` with no pack, and with a `Pending` pack, each reply with zero API calls +- [x] `/addsticker` on a full pack maps `STICKERS_TOO_MUCH` to the "pack is full" reply (via Phase 1 `FailMethodCode`) +- [x] Animated, video, **and mask/custom-emoji** (`Type != "regular"`) replies rejected before any API call, on both the source and owned paths ## Risk Assessment diff --git a/plans/260824-1051-sticker-pack-module/phase-05-photo-pipeline.md b/plans/260824-1051-sticker-pack-module/phase-05-photo-pipeline.md index b3ca356..85df045 100644 --- a/plans/260824-1051-sticker-pack-module/phase-05-photo-pipeline.md +++ b/plans/260824-1051-sticker-pack-module/phase-05-photo-pipeline.md @@ -1,7 +1,7 @@ --- phase: 5 title: "Phase 5: Photo pipeline and pack icon" -status: todo +status: done priority: P1 effort: "8h" dependencies: [1, 2, 3, 4] @@ -50,6 +50,10 @@ must not be described as spec in code comments or user-facing text. ### Source selection +This is the photo branch of Phase 4's `resolveSource(ctx, b, ownerID, msg)`, which already +takes every parameter this branch needs — no call site changes, and no `photoRef` type: the +branch resolves to a `fileID` like the sticker branch, by uploading first. + From `msg.ReplyToMessage`: - `Photo []PhotoSize` — pick the largest by `FileSize`; do not rely on Telegram's ordering. @@ -159,28 +163,28 @@ which a 512px sticker's `file_id` does not meet. Confirm in Phase 6's smoke test ## Todo -- [ ] Add `golang.org/x/image`; verify `go mod tidy` produces no diff -- [ ] `download.go` with 2 MB `LimitReader`, own client timeout, sentinel conversion -- [ ] `classify(err)` returning a coarse label that cannot contain a URL -- [ ] `toStickerPNG` with DecodeConfig guard, CatmullRom scale, PNG size ladder -- [ ] `toThumbnailPNG` at exactly 100×100 with transparent padding -- [ ] Photo/document source selection with mime allowlist and 2 MB pre-check -- [ ] Wire photo branch into `/addsticker` and `/newpack` -- [ ] `/setpackicon` handler and registration -- [ ] `image_test.go` with in-test generated fixtures (no committed binaries) -- [ ] `download_test.go` asserting no token or URL in any returned error +- [x] Add `golang.org/x/image`; verify `go mod tidy` produces no diff +- [x] `download.go` with 2 MB `LimitReader`, own client timeout, sentinel conversion +- [x] `classify(err)` returning a coarse label that cannot contain a URL +- [x] `toStickerPNG` with DecodeConfig guard, CatmullRom scale, PNG size ladder +- [x] `toThumbnailPNG` at exactly 100×100 with transparent padding +- [x] Photo/document source selection with mime allowlist and 2 MB pre-check +- [x] Wire photo branch into `/addsticker` and `/newpack` +- [x] `/setpackicon` handler and registration +- [x] `image_test.go` with in-test generated fixtures (no committed binaries) +- [x] `download_test.go` asserting no token or URL in any returned error ## Success Criteria -- [ ] 1024×512 → 512×256; 300×900 → 171×512; 512×512 → 512×512 -- [ ] 1×5000 extreme aspect: short edge clamped to ≥1, no panic, no zero-dimension image -- [ ] Alpha channel preserved through the resize -- [ ] Source above 2 MB rejected with zero HTTP requests made -- [ ] Decoded dimensions above 4096×4096 rejected before pixel allocation -- [ ] Unsupported document mime rejected before download -- [ ] `toThumbnailPNG` output is exactly 100×100 -- [ ] **A forced transport failure against an `httptest` server yields an error whose text contains neither `"bot"` nor the URL** — asserted, not assumed -- [ ] `go mod tidy && git diff --exit-code go.mod go.sum` clean +- [x] 1024×512 → 512×256; 300×900 → 171×512; 512×512 → 512×512 +- [x] 1×5000 extreme aspect: short edge clamped to ≥1, no panic, no zero-dimension image +- [x] Alpha channel preserved through the resize +- [x] Source above 2 MB rejected with zero HTTP requests made +- [x] Decoded dimensions above 4096×4096 rejected before pixel allocation +- [x] Unsupported document mime rejected before download +- [x] `toThumbnailPNG` output is exactly 100×100 +- [x] **A forced transport failure against an `httptest` server yields an error whose text contains neither `"bot"` nor the URL** — asserted, not assumed +- [x] `go mod tidy && git diff --exit-code go.mod go.sum` clean ## Risk Assessment diff --git a/plans/260824-1051-sticker-pack-module/phase-06-wiring-docs.md b/plans/260824-1051-sticker-pack-module/phase-06-wiring-docs.md index 4ba2815..6ec2c7f 100644 --- a/plans/260824-1051-sticker-pack-module/phase-06-wiring-docs.md +++ b/plans/260824-1051-sticker-pack-module/phase-06-wiring-docs.md @@ -1,7 +1,7 @@ --- phase: 6 title: "Phase 6: Wiring, menu, docs" -status: todo +status: partial priority: P2 effort: "4h" dependencies: [1, 2, 3, 4, 5] @@ -100,6 +100,15 @@ modules' descriptions. `[emoji...]`, `delsticker` → ``, `editsticker` → `<emoji...>`, `ordersticker` → `<position>`, `setpackicon` → ``, `renamepack` → `<title...>`, `delpack` → ``, `mypack` → ``. +**`<name...>` is not yet a documented form.** `docs/command-parameter-conventions.md:15-20` +defines `<name>`, `<name,...>`, `[name]`, and `[name...]` — required *remaining text* appears +nowhere, and no existing command uses it (`rg "Parameters:"` across `internal/` confirms). +Three of the nine (`<title...>` twice, `<emoji...>`) need it. Add the row +`| Required remaining text | \`<name...>\` | \`<title...>\` |` to that table and an example +line, in the same change that registers the commands — the conventions doc is the authority +these registrations are validated against, so shipping an undocumented form silently +demotes it. + ### README and docs Module table row listing the nine commands, then a `### Sticker packs` section: **one pack per @@ -146,20 +155,21 @@ dispatcher hook (`dispatcher.go:80-84`). ## Todo - [ ] Set `MODULES` explicitly in the deployed environment and verify -- [ ] Update `.env.example` with the explicit list and a why-comment -- [ ] Confirm `mypack` is free in the registry alongside the other eight -- [ ] Draft nine descriptions within the measured budget and re-measure `RenderHelp` -- [ ] Add `"sticker": sticker.New` to `factories()` -- [ ] Add nine entries to `expectedParameters` -- [ ] README module-table row + `### Sticker packs` section -- [ ] `docs/sticker-packs.md` -- [ ] Full validation gate +- [x] Update `.env.example` with the explicit list and a why-comment +- [x] Confirm `mypack` is free in the registry alongside the other eight +- [x] Draft nine descriptions within the measured budget and re-measure `RenderHelp` +- [x] Add `"sticker": sticker.New` to `factories()` +- [x] Add nine entries to `expectedParameters` +- [x] Add the `<name...>` row + example to `docs/command-parameter-conventions.md` +- [x] README module-table row + `### Sticker packs` section +- [x] `docs/sticker-packs.md` +- [x] Full validation gate - [ ] Manual smoke sequence against a real token ## Success Criteria -- [ ] `gofmt -l .` empty; `go vet ./...` clean; `go test ./...` passes; `golangci-lint run` clean -- [ ] `RenderHelp` stays under 4096 runes with all nine commands registered +- [x] `gofmt -l .` empty; `go vet ./...` clean; `go test ./...` passes; `golangci-lint run` clean +- [x] `RenderHelp` stays under 4096 runes with all nine commands registered - [ ] With `MODULES` unset in a scratch environment, the module still loads — confirming C10 is understood rather than assumed away - [ ] With the explicit `MODULES` list and no `sticker` entry, none of the nine commands register - [ ] Smoke: reply to a sticker with `/newpack smoke_pack Smoke Pack`; the returned link opens diff --git a/plans/260824-1051-sticker-pack-module/plan.md b/plans/260824-1051-sticker-pack-module/plan.md index ccef672..6c710fa 100644 --- a/plans/260824-1051-sticker-pack-module/plan.md +++ b/plans/260824-1051-sticker-pack-module/plan.md @@ -1,7 +1,7 @@ --- title: "Sticker packs module" description: "internal/modules/sticker — public, one-pack-per-user Telegram sticker set management via single-shot reply commands using @Stickers command names" -status: pending +status: partial priority: P2 effort: "" tags: ["sticker", "telegram-bot", "module"] @@ -190,12 +190,12 @@ structs, so under the current harness they can only ever **error**. `FailMethod` | # | Phase | Status | |---|-------|--------| -| 1 | [Phase 1: Shared prerequisites](./phase-01-shared-prerequisites.md) | Pending | -| 2 | [Phase 2: Store, set names, emoji parsing](./phase-02-store-setname-emoji.md) | Pending | -| 3 | [Phase 3: Pack lifecycle commands](./phase-03-pack-lifecycle.md) | Pending | -| 4 | [Phase 4: Sticker commands (reply path)](./phase-04-sticker-commands.md) | Pending | -| 5 | [Phase 5: Photo pipeline and pack icon](./phase-05-photo-pipeline.md) | Pending | -| 6 | [Phase 6: Wiring, menu, docs](./phase-06-wiring-docs.md) | Pending | +| 1 | [Phase 1: Shared prerequisites](./phase-01-shared-prerequisites.md) | Done | +| 2 | [Phase 2: Store, set names, emoji parsing](./phase-02-store-setname-emoji.md) | Done | +| 3 | [Phase 3: Pack lifecycle commands](./phase-03-pack-lifecycle.md) | Done | +| 4 | [Phase 4: Sticker commands (reply path)](./phase-04-sticker-commands.md) | Done | +| 5 | [Phase 5: Photo pipeline and pack icon](./phase-05-photo-pipeline.md) | Done | +| 6 | [Phase 6: Wiring, menu, docs](./phase-06-wiring-docs.md) | Partial — code + docs done; live smoke and deployed MODULES pending | ## Dependencies @@ -261,6 +261,7 @@ Public module creating durable Telegram-side objects on a single-threaded dispat | R7 | A transient error deletes a live pack's record | `/mypack` reports no pack though the user can still open theirs via link | Rule 4 — destructive store deletes require positive `STICKERSET_INVALID` | | R8 | Bot username changes in BotFather | Every pack refuses as "not yours" after restart | Ownership matches stored `Pack.Name` against `Sticker.SetName`; username only builds *new* names (Phase 2) | | R9 | `golang.org/x/image` resampling disappoints | Visibly soft or aliased stickers in smoke | Swap `CatmullRom` for `ApproxBiLinear`; one line, one file | +| R12 | `AddStickerToSet` may reject a `file_id` from a set this bot did not create | `/addsticker` on a sticker from any other pack fails while the photo path works | The one API assumption in this plan never checked against the live API — the docs allow a `file_id` in `InputSticker.sticker` but are silent on provenance. Settle with one live call before writing Phase 4's handler. If rejected, `/addsticker`'s sticker path routes through Phase 5's `GetFile` → download → `UploadStickerFile`, which makes Phase 4 depend on Phase 5 rather than the reverse — cheap to know first, expensive to discover after | ## Accepted disclosure @@ -298,6 +299,42 @@ per-user pack limit is one (former O2). ## Design Revisions +### 2026-08-25 — three-lens review pass (security, correctness, tests) + +Three independent reviewers ran against the finished module. What they changed: + +- **Adoption no longer trusts the reservation alone.** The reservation proves + ownership only while it outlives the sets it guards, and it does not: it lives + in our store, the packs live at Telegram, and the in-memory backend is + selected silently whenever `MONGO_URL` is unset. After any wipe the original + takeover was reachable again. `createOrAdopt` now refuses to adopt when *this* + invocation first claimed the name — a genuine interrupted attempt always finds + its own reservation waiting, so there are no false negatives. +- **Post-action cleanup reads moved onto the detached context.** They wrote via + `commitContext` but read on the request context, so a cancelled request failed + the read and skipped the release while still deleting the pack record — + stranding a name permanently. Found independently by two reviewers. +- **Emoji clustering.** Nine valid emoji were refused outright; tag-sequence + flags shattered; and three inputs (trailing ZWJ, a joiner before a flag, an + odd regional-indicator count) passed validation and would have sent an + `emoji_list` Telegram rejects. +- **A reply tail is now reserved** from the handler budget via the existing + `chathelper.FetchContext`, which every other data-fetching module already + used. A slow photo `/newpack` could spend the whole 10s before Telegram was + called, and then send its error reply on a dead context — the user saw nothing. +- **The compression ladder resamples the scaled image, not the source.** It was + paying a full-size resample per rung: measured 1.99s versus 74ms for the three + rungs, on a dispatcher that runs handlers one at a time. +- **Tests.** Mutation testing refuted the previous round's non-vacuity claim in + three places. The `created`-flag release machinery had no coverage in either + direction; two emoji assertions passed against a handler that never called + Telegram; and nothing pinned module registration — the whole module could be + removed from `factories()` with the suite still green. + +Left open deliberately: the shutdown path never joins the polling goroutine, so +`context.WithoutCancel` does not actually survive process exit. That is +`cmd/server/main.go`, outside this module and affecting every module's commits. + ### 2026-08-25 — one pack per user The accepted scope originally chose "named packs, multiple per user, addressed by slug @@ -351,6 +388,86 @@ before an interruption. It now probes `GetStickerSet(oldName)` first and adopts overwrites when the old set exists. +### 2026-08-25 — post-implementation review: global slug reservation + +An independent review of the implemented module found a **pack takeover** in +`/newpack`, and it traces back to this plan, not only to the code. + +Phase 3 step 5 said: `GetStickerSet` succeeds → "we hold a `Pending` record for +this owner and slug, so this is our own interrupted attempt: adopt it". The +plan's own risk section stated the supporting invariant as *"a `Pending` record +for an owner means that owner asked for that name"*. That is true and **not +sufficient**. Asking is not creating. `PutVersioned` is create-only per *owner +key* (`packKey` is the owner ID alone); nothing reserved a name globally. So a +user with no pack who typed another user's slug produced byte-identical evidence +to a genuine resumed attempt — and adopted their pack, then could `/delpack` it. +Every share link is public, so slugs are trivially enumerable. + +The plan pre-authorised one response to this signal ("disable adoption — a +one-line change"), which would have closed the hole by dropping the +"an interrupted `/newpack` can be completed by re-running" success criterion. +The user chose the stronger fix instead: + +**A global slug reservation.** `SlugReservation{Slug, OwnerID}` is written +create-only under `slug:<slug>` *before* Telegram is touched. Adoption is +allowed only when the reservation names the caller. The first claimant of a name +is the only user who can ever adopt a set under it, so "this set is mine" became +a proven fact rather than an assumption — and both success criteria survive. + +Consequent changes: + +- `reserveSlug` runs before `claimSlug`; a name held by anyone else replies + "that pack name is taken" with **zero** API calls. +- `resolveStaleIntent` re-proves the *old* slug's reservation before adopting + under it, and releases it on a positive "no such set". +- `dropIntent` no longer fires on unknown errors (was a plan rule 4 violation in + its own right): on anything but a *classified* refusal both the intent and the + reservation survive, which is what lets a re-run recover. Only a positive + refusal releases them. +- `/delpack` keeps the reservation, so a deleted name stays recoverable by its + owner and unavailable to everyone else — which also matches Telegram's likely + behaviour for deleted short names (R11). + +Four further defects fixed in the same pass: + +- **A stale `/delpack` confirmation deleted a live pack's record.** Pending + actions used a random key per invocation, so two could be live at once, and + the delete path cleared the record *by owner* without checking it still named + the set being deleted — precisely the documented delete-then-recreate URL + change. Now keyed per user (matching `stock/pending_dividend.go`), superseded + by a newer prompt, and gated on `ownsSet` before clearing. +- **Unbounded storage from a public command.** The same random key meant a user + who ran `/delpack` and never tapped left a permanent document. +- **The panic barrier missed the detached hook goroutine**, so a panicking + `CommandHook` (the `stats` module ships one) still killed the process. C9's + "Phase 1 closes this" was true for handlers only. +- `/delpack`'s result message bypassed `chathelper.Reply`, so in a forum + supergroup it landed in General instead of the topic. + +### 2026-08-25 — Phase 4 review pass + +Ten findings applied before implementation began. Six changed what gets written: + +- `resolveSource` takes `(ctx, b, ownerID, msg)` from the start, and `photoRef` is gone — the + photo branch resolves to a `file_id` like every other path, so Phase 5 adds a branch instead + of rewriting call sites. +- The static-only gate moves onto `resolveSource`, the path that can actually receive a + non-static sticker, and now also rejects `Type != "regular"` — a mask sticker is static yet + invalid for a regular set, which `IsAnimated || IsVideo` does not catch. +- `/addsticker` gained its missing no-pack path, and the note on why its refusal is + deliberately *not* the uniform `resolveOwned` one. +- `/delsticker` takes the same per-user keylock as `/addsticker`; both do the same + read-modify-write on `Count`. +- `/delsticker`'s "never deletes the record" criterion was scoped to transient and unknown + errors. Unqualified, it forbade Phase 3's `STICKERSET_INVALID` self-heal — the very + mechanism that unwedges a user whose set Telegram removed. +- The animated/video check moved ahead of the store read in `resolveOwned`'s numbered gate. + +Also: the `Count: 0` aftermath now names its recovery route, `STICKERS_TOO_MUCH` and the +no-pack paths gained success criteria, R12 records the unverified `file_id`-reuse premise, and +Phase 6 must document `<name...>` in the conventions doc rather than shipping an undocumented +parameter form. + #### Consistency sweep — one-pack revision - Files reread: plan.md and all six phase files. diff --git a/plans/reports/correctness-review-260825-1515-sticker-module.md b/plans/reports/correctness-review-260825-1515-sticker-module.md new file mode 100644 index 0000000..d699976 --- /dev/null +++ b/plans/reports/correctness-review-260825-1515-sticker-module.md @@ -0,0 +1,423 @@ +# Correctness / Crash-safety / Concurrency Review — internal/modules/sticker + +Date: 2026-08-25 · Reviewer lens: correctness, crash-safety, concurrency. +Out of scope by assignment: cross-user security impact, test quality. +Scope: uncommitted `internal/modules/sticker/` (~1.9k LOC non-test) plus the +modified `cmd/server/main.go`, `internal/modules/dispatcher.go`, and the +storage/keylock contracts they rely on. + +`go build ./...` clean. `go vet` clean on sticker/storage/modules. +`go test ./internal/modules/sticker/` passes. + +## Verdict + +The write-ahead intent state machine is sound. Every /newpack interruption point +recovers or refuses; none wedges the user permanently on paths reachable in this +deployment. Two real findings (H-1, H-2) undermine the *durability* half of the +design rather than its logic. The rest is MEDIUM/LOW. + +--- + +## Findings + +### H-1 (HIGH) — `commitContext`'s SIGTERM protection is defeated by the shutdown path + +`cmd/server/main.go:214-228`, `internal/modules/sticker/state.go:53-59` + +`commitContext` uses `context.WithoutCancel(ctx)` + 5s so a post-action commit +survives SIGTERM. That only defends against *context* cancellation. The process +does not wait for it: + +``` +go func() { b.Start(rootCtx) }() // main.go:214 — return value never awaited +<-rootCtx.Done() // main.go:220 +srv.Shutdown(shutdownCtx) // HTTP only +} // main returns -> defer closeProvider() -> process exits +``` + +`b.Start` *would* drain correctly — with `WithNotAsyncHandlers` + 1 worker +(`telegram/client.go:28`, lib `defaultWorkers = 1`) its `wg.Wait()` blocks until +the inline handler returns — but `main` never joins that goroutine. On SIGTERM +`main` proceeds as soon as `srv.Shutdown` finishes (immediate with no in-flight +HTTP), runs `defer closeProvider()` (main.go:125), and exits. The detached +commit gets milliseconds, then the Mongo client is disconnected under it. + +Failure scenario: deploy lands while user runs `/newpack foo Bar`. +`CreateNewStickerSet` returns 200; `finishNewPack` → `commitPack` → detached +`Put` starts; SIGTERM arrives; process exits before the write lands. Record stays +`Pending:true`. Recoverable (re-run `/newpack foo …` adopts), so not data loss — +but the write-ahead recovery path is exercised routinely rather than rarely, and +the comment's claim ("must not be lost because the process is shutting down") is +false as built. + +Same exit kills `adjustCount`'s commit (count silently under-counts, never +self-corrects) and `renamePack`'s commit (stored title diverges from Telegram +permanently). + +Fix direction: have `main` wait for the polling goroutine before returning — +e.g. `pollDone := make(chan struct{}); go func(){ b.Start(rootCtx); close(pollDone) }()` +then `select { case <-pollDone: case <-time.After(shutdownGrace): }` before +`srv.Shutdown` / `closeProvider`. Grace must exceed `commitTimeout` (5s). + +### H-2 (HIGH) — every reply is sent on the same exhausted context the API work drained + +`internal/modules/sticker/state.go:62-65` and all nine handlers. + +`handlerContext` = 10s for the whole handler. Nothing reserves a tail for the +reply. `chathelper.Reply` sends with that same ctx, so once the budget is spent +the user gets **no message at all** — success or failure. + +Concrete: `/newpack mypack My Pack` replying to a **photo**. +`resolveSource` → `downloadFile` (own 8s client timeout, but also bounded by the +handler ctx) → `toStickerPNG` (CatmullRom on up to 4096×4096) → `UploadStickerFile`. +On a slow link that is 7-9s. `CreateNewStickerSet` then runs on <1-3s and +returns `context.DeadlineExceeded` — which is correctly *not* `createRefused`, +so intent + reservation are kept — and `replyAPIError` then calls +`reply(ctx, …)` on the dead context. User sees silence, has a `Pending` record +and a burned reservation, and no indication what happened. `/mypack` does show +the pending marker, so it is discoverable, not wedged. + +The repo already has the fix pattern and this module is the only one not using +it: `chathelper.FetchContext` reserves a 3s reply tail and is used by +`coin/views.go:35`, `gold/handlers.go:29,185`, `stock/stock_events.go:78`, +`monkeyd/tags_command.go:81`. + +Fix direction: derive `fetchCtx, cancel := chathelper.FetchContext(ctx)` for the +download/upload/Telegram calls and keep the outer `ctx` for `reply`. + +### M-1 (MEDIUM) — post-action cleanup helpers *read* on the cancellable context + +`pack_handlers.go:179` (`releaseSlug`), `:397` (`adjustCount`), `:432` +(`dropPackRecordIfSet`), `:464` (`dropPackRecord`). + +Each of these runs *after* a confirmed Telegram-side action, and each carefully +wraps its **write** in `commitContext` — but performs the **read** it depends on +with the caller's cancellable `ctx`. A cancelled/expired ctx therefore silently +skips the write. + +- `adjustCount:397` — ctx expired right after a successful `AddStickerToSet` → + `getPack` fails → count increment never persisted. The delta is lost forever + (the next adjust reads the stale base). Handler falls back to an in-memory + `pack.Count++` purely for the reply, so the user is told a number that was + never stored. +- `releaseSlug:179` — ctx expired in the `createRefused` branch + (`pack_handlers.go:351-354`) → reservation read fails → name stays reserved + with no pack behind it. The owner can re-reserve (owner check passes), so the + loss is only to other users' namespace, but it is permanent. +- `dropPackRecord:464` — read fails → record is still deleted (correct: keeping + it would block `/newpack`) but the slug can never be freed, because the record + was the only thing that knew the name. + +Fix direction: derive the commit context once at the top of each helper and use +it for both the read and the write. + +### M-2 (MEDIUM) — emoji: nine valid RGI emoji are refused outright + +`emoji.go:130-156` (`isEmojiRune`). Verified by running `parseEmoji` against +each codepoint: + +| Input | Result | +|---|---| +| `©️` U+00A9, `®️` U+00AE | refused | +| `〰️` U+3030, `〽️` U+303D | refused | +| `㊗️` U+3297, `㊙️` U+3299 | refused | +| `Ⓜ️` U+24C2 | refused | +| `⤴️` U+2934, `⤵️` U+2935 | refused | + +All are in Telegram's emoji keyboard. `™️` U+2122 and `ℹ️` U+2139 are special-cased +but their neighbours are not. `/editsticker ©️` fails with "is not an emoji". + +Fix direction: add U+00A9, U+00AE, U+2934-2935, U+3030, U+303D, U+3297, U+3299, +U+24C2 to the singleton/range list (2900-297F would also cover the arrows). + +### M-3 (MEDIUM) — emoji: tag-sequence flags shatter, and the refusal prints raw tag characters + +`emoji.go:69-102`. `isBinding` covers Mn/Me but tag characters (U+E0020-E007F) +are category **Cf**, so `🏴󠁧󠁢󠁥󠁮󠁧󠁿` (England/Scotland/Wales flags) splits into +`["🏴", "\U000e0067", "\U000e0062", …]`. Verified output: + +``` +in="🏴\U000e0067\U000e0062\U000e0065\U000e006e\U000e0067\U000e007f" +clusters=["🏴" "\U000e0067" … ] err="\U000e0067" is not an emoji. +``` + +Two problems: a legitimate emoji is rejected, and `%q` renders an invisible tag +char, so the user is told `"\U000e0067" is not an emoji`. + +Fix direction: treat U+E0020-U+E007F as binding, terminating the cluster at +U+E007F (cancel tag). + +### M-4 (MEDIUM) — emoji: three inputs pass validation and send an invalid `emoji_list` to Telegram + +`emoji.go:69-102`, `:117-128`. Verified: + +| Input | Clusters produced | Sent to Telegram | +|---|---|---| +| `😀‍` (trailing ZWJ) | `["😀‍"]` | yes → `STICKER_EMOJI_INVALID` | +| `😀‍🇻🇳` | `["😀‍🇻", "🇳"]` — the ZWJ branch (`emoji.go:83-88`) swallows the first regional indicator, orphaning the second | yes | +| `🇻🇳🇺` (odd RI count) | `["🇻🇳", "🇺"]` — lone RI passes `isEmojiRune` via `emoji.go:154` | yes | + +Not a crash: `apiRefusal` maps `STICKER_EMOJI_INVALID` to "Telegram rejected +those emoji", so the user gets a sane message after one wasted API round-trip. +Correctness bug, low impact. + +Fix direction: reject a cluster ending in ZWJ; do not classify a lone regional +indicator as emoji; do not let the ZWJ branch consume a regional indicator. + +### L-1 (LOW) — `/newpack` does all its expensive work before checking whether the caller already has a pack + +`pack_handlers.go:68-99`. `resolveSource` (photo path: GetFile + up to 2 MB +download + resize + `UploadStickerFile`) and `resolver.resolve` (GetMe) run +*before* the lock and before the "you already have a pack" pre-check. A user who +already owns a pack pays a full download+upload and creates a file on Telegram's +servers, then is refused. Wasted work only; no state divergence. Moving the +pre-check above `resolveSource` would also buy back budget for H-2 — but note +the pre-check must stay *after* `lockUser` and *before* `reserveSlug`, which is +the ordering the comment at `:85-91` is defending. + +### L-2 (LOW) — `dropPackRecord` is called both inside and outside `lockUser` + +Inside: `handleAddSticker` (`sticker_handlers.go:80`), `handleDelSticker` (`:129`), +`handleRenamePack` (`pack_handlers.go:544`). +Outside: `handleEditSticker` (`sticker_handlers.go:172`), `handleOrderSticker` +(`:210`), `handleSetPackIcon` (`setpackicon.go:45`). + +`dropPackRecord` is a read → delete → release-slug sequence. Unlocked call sites +could delete a record another handler just committed. + +**Not reachable in production today**: dispatch is inline with one worker +(`bot.WithNotAsyncHandlers()`, `defaultWorkers = 1`), the sticker module +registers no cron job, and the dispatcher's detached per-command stats hook +(`dispatcher.go:80-88`) writes only to the `stats` collection. Latent only — +flagging because the inconsistency reads as an oversight rather than a decision. + +Same class, same reachability: `handleAddSticker` resolves `pack` at +`sticker_handlers.go:41` *before* `defer s.lockUser(...)()` at `:66`, then uses +`pack.Name` for the API call. `adjustCount` correctly re-reads under the lock, +but the API call itself uses the pre-lock value. `handleRenamePack` likewise +reads at `pack_handlers.go:531` and `Put`s that whole stale record at `:550`, +which would clobber a concurrent `Count` change. + +### L-3 (LOW) — `/ordersticker` reports a position Telegram may not have honoured + +`sticker_handlers.go:196-222`. Upper bound is deliberately delegated to Telegram +(correct — a local `Count` is advisory). But if `SetStickerPositionInSet` clamps +an out-of-range position instead of erroring, the reply "Moved to position N" +states something false. Consider "Moved." or re-reading the set. + +### L-4 (LOW) — `photoFileID` picks by `FileSize`, which may be absent + +`photo.go:57-63`. `PhotoSize.FileSize` is optional in the Bot API. If Telegram +omits it for every size, all compare equal to 0 and `Photo[0]` — the *smallest* +thumbnail — is chosen, yielding a blurry sticker. Telegram populates it in +practice. Tie-break on `Width*Height` instead. + +### L-5 (LOW) — state divergence on a DB reset / collection drop + +`pack_handlers.go:313-320`. `createOrAdopt`'s `err == nil` branch adopts any +existing set under `<slug>_by_<bot>`, and its safety rests entirely on the +reservation table being authoritative. If the module's collection is dropped or +the same bot token is pointed at a fresh Mongo, the reservations vanish while the +Telegram sets do not: the next claimant of a previously-used slug reserves it +cleanly (`created == true`), then adopts the *previous* owner's set, and the +record's `Count = 1` (`:363-365`) will disagree with the real set. + +The cross-user impact belongs to the security reviewer; noting it here only as a +Count/ownership divergence and an operational constraint. Fix direction is +operational, not code: never drop this collection while packs exist, or gate +adoption on `created == false`. + +### Nit — `apiRefusal` hardcodes `120` instead of `maxStickersPerPack` + +`errors.go:76` says "Your pack is full (120 stickers)." while +`sticker_handlers.go:17` defines the const. The const is otherwise referenced +only from tests. + +--- + +## /newpack interruption-point table + +Notation: **R** = slug reservation (`slug:<slug>`), **P** = Pack record. +"Next `/newpack <same slug>`" and "Next `/newpack <other slug>`" are the two +recovery entry points. All rows traced against `handleNewPack` +(`pack_handlers.go:45-118`) and its four helpers. + +| # | Interruption point | State left behind | Next `/newpack` **same** slug | Next `/newpack` **different** slug | Wedged? | +|---|---|---|---|---|---| +| 1 | Before `reserveSlug` (arg/slug/title/source/GetMe failure, `:50-81`) | none | normal create | normal create | no | +| 2 | Between pre-check (`:92`) and `reserveSlug` write | none | normal create | normal create | no | +| 3 | After R written, before `claimSlug` (`:101-106`) | R only | `reserveSlug` conflict → own → resume (`created=false`), `claimSlug` creates P, create proceeds | R(old) orphaned; new R created; P created for new slug. Old R leaks (owner-held, re-reservable by owner) | no | +| 4 | `claimSlug` fails/answers with `created=true` (`:106-115`) | R released at `:112` | normal create | normal create | no | +| 5 | After P(pending) written, before `GetStickerSet` (`:106→313`) | R + P(pending) | `claimSlug` → `existing.Slug == slug` → resume → `GetStickerSet` missing → create | `resolveStaleIntent` (`:253`): R(old) held by caller, `GetStickerSet(old)` **missing** → `Put(new intent)`, `releaseSlug(old)` → create | no | +| 6 | `GetStickerSet` returns unknown error (`:325-330`) | R + P(pending) unchanged — **deliberately untouched** | retry; succeeds once Telegram answers | as row 5 | no | +| 7 | Between `GetStickerSet`(missing) and `CreateNewStickerSet` (`:337`) | R + P(pending) | resume → create | as row 5 | no | +| 8 | `CreateNewStickerSet` returns a `createRefused` code (`:351-354`) | R released, P dropped | clean retry (same refusal until the cause changes) | clean create | no | +| 9 | `CreateNewStickerSet` returns a non-refusal error (timeout/429/SIGTERM) — **set may or may not exist** (`:355`) | R + P(pending) kept | `GetStickerSet` decides: exists → adopt + commit; missing → create. Both correct | `resolveStaleIntent` probes old name: exists → **adopt old**, tell user "restored"; missing → release old, create new | no | +| 10 | Create succeeded server-side, process dies before `finishNewPack` | R + P(pending); set exists | `GetStickerSet` → exists → `finishNewPack(adopted=true)`, `Count=1` | `resolveStaleIntent` → old set exists → adopt, refuse the new name with "restored" | no | +| 11 | `commitPack` inside `finishNewPack` fails (`:366-369`) | R + P(pending); set exists | as row 10 → adopt + commit | as row 10 | no | +| 12 | Process exits during the detached `commitPack` (**H-1**) | identical to row 11 | as row 10 | as row 10 | no | +| 13 | `resolveStaleIntent` dies between `Put(new intent)` (`:296`) and `releaseSlug(old)` (`:300`) | R(old) orphaned + R(new) + P(new, pending) | resume new slug → create | probes new slug's set | no; old R leaks permanently to other users | +| 14 | P(pending) exists but its R is now held by someone else | P(pending) + foreign R | `claimSlug` → `existing.Slug == slug` → resume → `GetStickerSet` → **if the other holder created it, this adopts their set** | `resolveStaleIntent` → `held.OwnerID != caller` → drop dead intent, proceed cleanly | see L-5 | + +Row 14 is only reachable via the L-5 reservation-loss scenario; in normal +operation `reserveSlug` runs before `claimSlug`, so a pending P always implies +the caller held R at the moment P was written, and no code path transfers R +between users (`releaseSlug:187` re-verifies ownership). + +**Escape hatch verified**: `handleDelPack` (`delpack_callback.go:22`) does *not* +require `!Pending`, so a user stuck on a pending record can always clear it. +`DeleteStickerSet` on a never-created set returns `STICKERSET_INVALID`, which +`isStickerSetMissing` treats as success (`:180`), dropping P and releasing R. + +--- + +## Verified sound + +Checked and found correct; no action needed. + +**State machine / ordering** +- Pre-check "already has a finished pack" is inside `lockUser` and before + `reserveSlug` (`:83-99`). Reversing those two is the name-burning primitive the + comment describes; the order is right. +- `claimSlug` uses `PutVersioned(…, 0, …)` (create-only), never `Put`. Both + backends give exactly one winner: Mongo via the version-0/absent filter + + upsert + `_id` duplicate-key (`mongo_doc_store.go:85-105`); memory via + `ErrConflict` when the key exists (`memory_provider.go:107-110`). +- `created` is threaded correctly: `reserveSlug` returns `false` when it merely + resumes an existing reservation (`:165`), so the bail path at `:111` never + releases a reservation predating the invocation. +- `releaseSlug` re-verifies `held.OwnerID == ownerID` inside the operation + (`:187`), not at the call site. +- `dropPackRecord` reads before deleting so the slug is still known (`:464-480`), + and deletes the record before releasing the name — the safe ordering (the + reverse would free a name while a record still claims it). +- `dropPackRecordIfSet` guards on `ownsSet` (`:440`) so a stale `/delpack` + confirmation cannot erase a newer pack's record. +- `resolveStaleIntent` re-proves reservation ownership rather than inferring it + from the pending record (`:258-272`). + +**Error classification** +- `isStickerSetMissing` requires both `bot.ErrorBadRequest` **and** the + `STICKERSET_INVALID` substring (`errors.go:43-46`). `context.DeadlineExceeded`, + `context.Canceled`, 429, and transport errors all fall through — they never + authorise a record delete. Verified at all six call sites. +- `createRefused` (`errors.go:113-123`) lists only request-validation codes and + is kept separate from `apiRefusal` despite the overlap. Every code listed + genuinely proves nothing was created. +- No path infers absence from a generic failure. `createOrAdopt`'s `default` + branch (`:325-330`) and `resolveStaleIntent`'s (`:303-307`) both change nothing. + +**Count** +- `adjustCount` re-reads inside `lockUser` (`:397`) rather than trusting the + handler's pre-lock copy. +- Clamped at 0 (`:407-410`); cannot go negative. +- Missing record returns `storage.ErrNotFound` and does **not** recreate the + record (`:402-405`). +- A failed `AddStickerToSet`/`DeleteStickerFromSet` returns before `adjustCount`, + so a failed API call never moves the count. +- Pack-full (`STICKERS_TOO_MUCH`) returns via `replyAPIError` with no increment. + +**Context** +- `commitContext` used at every post-action commit: `commitPack:418`, + `dropIntent:381`, `dropPackRecord:471`, `releaseSlug:191`, + `dropPendingDelete:~200`, and the `/delpack` prompt persist. +- Not used where cancellation should apply: `reserveSlug`'s and `claimSlug`'s + pre-action writes, `resolveStaleIntent`'s intent replacement, and the pending + action consume in the callback all use the cancellable ctx. Correct. +- Every `context.WithTimeout` has a matching `cancel`, all `defer`red. No leaks; + `go vet` agrees. + +**Concurrency** +- `keylock.Map` zero value is usable; `defer s.lockUser(id)()` acquires eagerly + and defers only the unlock — correct idiom at all six call sites. +- `state`'s zero-value `usernameResolver` and `nowFn == nil` are both safe + (`state.go:47-51`, `setname.go:97-122`), so `New` not initialising them is fine. +- `usernameResolver.resolve` never caches a failure and holds no lock across the + `GetMe` call. +- Slug races between two *different* users are resolved by store atomicity, not + by `keylock` (which is per-user and could not help). Correct choice. +- `/delpack` double-press: the pending action is consumed *before* + `DeleteStickerSet` (`delpack_callback.go:~183`), so a second press finds + `ErrNotFound`. Serialized dispatch makes it moot anyway. + +**Callback safety** +- `models.CallbackQuery.Message` is a **value** (`MaybeInaccessibleMessage`), not + a pointer, in `go-telegram/bot v1.20.0`. `query.Message.Message` cannot nil-deref; + the inner `*Message` nil check is the right and sufficient guard. +- Binding check (`chat + message id + non-zero MessageID`) precedes every side + effect including `clearButton`. +- `parseDeleteCallback` bounds length to 64 and validates hex before use; the id + is a lookup key only, never an authorisation input. + +**Emoji (correct cases, verified by execution)** +ZWJ families `👨‍👩‍👧‍👦`; skin tone + ZWJ `👩🏽‍🚀`; VS16-then-ZWJ `🏳️‍🌈`; +ZWJ-then-VS16 `🏴‍☠️`; keycaps `1️⃣` `#️⃣`; regional-indicator pairs `🇻🇳`; +skin-tone modifiers `👍🏿`; `⭐` (the default emoji) classifies as emoji; +plain `A` is refused. `len(out) > 20` boundary matches Telegram's 1-20. +`parseEmoji` returns `(nil, nil)` for empty input and `/addsticker` falls back +correctly while `/editsticker` refuses — the right asymmetry. + +**Image pipeline** +- `decodeBounded` checks `DecodeConfig` before allocating pixels; rejects + >4096 per side and `<=0` dimensions as a `userError`. +- `scaleToLongEdge` clamps the short edge to ≥1, so 4096×1 → 512×1: no + zero-dimension image, no divide-by-zero. +- Compression ladder is a fixed 3-element slice — cannot loop forever. The + `data, err =` reassignment at `image.go:64` clobbers `data` on encode error, + but the loop unconditionally reassigns it afterwards, and a loop encode error + returns `(nil, encErr)`. No nil-with-nil-error return. +- `toThumbnailPNG` offsets are always ≥0 because both scaled dimensions are ≤100; + `draw.Draw`'s `sp` correctly maps `scaled.Bounds().Min`. +- `downloadFile` bounds by `maxSourceBytes+1` on the reader itself rather than + trusting `Content-Length`, and closes the body. + +**Boundaries** +- `slugRe` `^[a-z][a-z0-9_]{2,39}$` = 3-40 chars, matching `minSlugLen`/`maxSlugLen`; + `__` and trailing `_` are checked separately as the comment states. +- `makeSetName`'s budget cannot go negative for any legal Telegram username (≤32 + chars → suffix 36 → budget 28). +- No `List` calls anywhere in the module; every lookup is a keyed `Get`. No N+1. +- `senderID` rejects `SenderChat != nil`, so anonymous group admins cannot all + collapse onto `GroupAnonymousBot`'s single user id. + +**Error surfacing** +- `replyErr` shows `userError` verbatim and replaces everything else with + `genericFailure`; `downloadFile` discards the original error entirely rather + than wrapping, so the bot token in `FileDownloadLink` cannot reach a log via + `errors.Unwrap`/`%v`. `classify` inspects only error *types*. +- No path returns `nil` where the caller assumes success. The two "log and + continue" spots (`renamePack` commit `:550-554`, `adjustCount` fallback + `sticker_handlers.go:87-92`, `:135-141`) both follow a *confirmed* Telegram + success, so reporting success to the user is accurate about the pack even + though the stored count/title may lag. + +--- + +## Recommended actions + +1. **H-1** — join the polling goroutine in `main` with a grace period > 5s before + `srv.Shutdown`/`closeProvider`. Without this the whole `commitContext` design + is decorative. +2. **H-2** — adopt `chathelper.FetchContext` in the sticker handlers so a slow + photo pipeline cannot swallow the user's reply. +3. **M-1** — use the commit context for the *read* as well in `adjustCount`, + `releaseSlug`, `dropPackRecord`, `dropPackRecordIfSet`. +4. **M-2/M-3/M-4** — emoji table additions, tag-sequence binding, and the three + invalid-cluster rejections. +5. **L-1** — move the "already have a pack" pre-check above `resolveSource` + (keeping it inside `lockUser` and before `reserveSlug`). +6. **L-2** — make `lockUser` coverage uniform across the six `dropPackRecord` + call sites, and take the lock before the `getPack` whose value feeds the API + call in `handleAddSticker`/`handleRenamePack`. +7. L-3, L-4, L-5, nit — at author's discretion. + +## Unresolved questions + +1. Does Telegram permanently reserve a deleted set's short name? The code + (`pack_handlers.go:455-458`) documents this as unverified and degrades + gracefully either way, so it is not blocking — but it decides whether + `releaseSlug` after a `/delpack` is meaningful or purely local bookkeeping. +2. Does `SetStickerPositionInSet` error or clamp on an out-of-range position? + Determines whether L-3 is a false success message or a non-issue. +3. Is the sticker collection ever dropped or re-pointed in this deployment's + operational runbook? That is the sole trigger for L-5 / table row 14. diff --git a/plans/reports/security-review-260825-1515-sticker-module.md b/plans/reports/security-review-260825-1515-sticker-module.md new file mode 100644 index 0000000..02c4ea3 --- /dev/null +++ b/plans/reports/security-review-260825-1515-sticker-module.md @@ -0,0 +1,234 @@ +# Security review — sticker module (uncommitted) + +Lens: security / abuse resistance only. Style, naming, test coverage out of scope. +Method: traced attacker-controlled inputs (command args, replied message, callback payload, +image bytes) through every store write and every Telegram call that names a set. Third pass, +after the takeover and name-burning fixes. + +Verified environment facts used below: +- `internal/telegram/client.go:26-31` — `WithNotAsyncHandlers()`, single worker: updates are + processed strictly one at a time, inline on the polling goroutine. +- `cmd/server/main.go:266-280` — provider auto-detect: `MONGO_URL` unset ⇒ **memory backend**, + announced with `log.Warn` only. `"sticker": sticker.New` is registered unconditionally + (`cmd/server/main.go:95`). +- `go-telegram/bot@v1.21.0/raw_request.go:78-81` — the library redacts the token inside + `*url.Error.URL` for API-call failures. +- `models.CallbackQuery.Message` is a **value** `MaybeInaccessibleMessage` holding `*Message`, + so `query.Message.Message` cannot nil-panic. + +--- + +## HIGH — adoption is authorised by a record that is less durable than the object it protects + +`internal/modules/sticker/pack_handlers.go:313-321` (adopt branch), `:138-166` (reserveSlug), +`cmd/server/main.go:266-280` (backend selection). + +The reservation is the *only* evidence that an existing Telegram set belongs to the caller — +`GetStickerSet` exposes no owner. The reservation lives in the module's store; the sticker set +lives on Telegram forever. Any event that empties the store while the sets survive re-opens the +exact cross-user takeover the reservation was added to close, with no code change. + +Exploitation (memory backend variant — reachable by omitting `MONGO_URL`, which only logs a Warn): + +1. Victim V: `/newpack cool My Pack` → set `cool_by_<bot>` created, share link is public by design. +2. Bot restarts (deploy, OOM, VM reboot). Memory store is empty; Telegram set untouched. +3. Attacker A (any user, no pack) replies to any sticker with `/newpack cool Whatever`. + - `reserveSlug` → no reservation exists → created for A. + - `claimSlug` → no pack record for A → pending intent written. + - `createOrAdopt` → `GetStickerSet("cool_by_<bot>")` returns **nil error** → + `finishNewPack(..., adopted=true)` → A's record now owns V's set. +4. A gains, via calls that carry no owner scoping at all: + - `/delpack` → `DeleteStickerSet{Name}` — **destroys V's pack permanently** (no user_id param). + - `/delsticker` replying to any sticker from V's public pack — `resolveOwned` passes because + `pack.Name == st.SetName`; `DeleteStickerFromSet{Sticker}` takes only the file id. + - `/renamepack` → `SetStickerSetTitle{Name,Title}` — also unscoped. + V is simultaneously locked out: V's `/newpack cool` answers `slugTaken`, `/mypack` says no pack. + +Same primitive without the memory backend: collection dropped, `MONGO_DATABASE` changed, module +renamed, or a Mongo restore from a backup older than the newest sets. + +Why the existing guards do not stop it: every guard (reservation owner check, `created` scoping, +`ownsSet`, uniform refusals) reasons entirely inside the local store. When the store is empty the +guards are all *satisfied*, and the adopt branch is by design the path that turns "a set exists +under a name I hold" into ownership. + +Fix direction (cheap and precise, no new state): adoption should require that the reservation +**pre-dated this invocation**. `reserveSlug` already computes exactly that as `created`; plumb it +into `createOrAdopt` and, when `created == true` and `GetStickerSet` succeeds, refuse with +`slugTaken` (plus release the just-made reservation) instead of adopting. A genuine interrupted +attempt always re-enters with `created == false` (its reservation was written by the earlier run), +and a set cannot exist for a reservation first written microseconds ago in this same handler — so +this has no false negatives, and the store-wipe path can no longer adopt anything. +Secondly: refuse to build/register this module on a non-durable provider (or `log.Fatal` when +`KV_PROVIDER=memory` and sticker is enabled) — the module creates permanent, globally visible +Telegram objects and must not run on a store documented as "data lost on restart". + +## MEDIUM — cleanup helpers read on the request context but write on a detached one; slugs leak permanently + +`internal/modules/sticker/pack_handlers.go:179` (`releaseSlug` → `getSlugReservation(ctx, …)`), +`:464` (`dropPackRecord` → `getPack(ctx, …)`), `:432` (`dropPackRecordIfSet` → `getPack(ctx, …)`). + +`commitContext` (`state.go:59-61`) exists precisely because SIGTERM cancels `rootCtx` mid-handler. +It is applied to the `Delete`/`Put` calls in these helpers but **not** to the reads that decide what +to delete. The reads therefore fail exactly in the situation the detached write was designed for. + +Scenario (ordinary deploy, no attacker needed): user presses the `/delpack` confirm button; +`DeleteStickerSet` succeeds; SIGTERM lands (or the 10s `handlerTimeout` expires — the handler has +already made 2-4 API calls by then). +- In `dropPackRecordIfSet`, `getPack` fails → returns early → the record survives naming a set that + no longer exists. Self-heals on the next command via `STICKERSET_INVALID`, so this half is benign. +- In `dropPackRecord` (reached from any `isStickerSetMissing` path), `getPack` fails, the pack record + is deleted anyway on the detached context, and `pack.Slug` is never known, so the reservation is + never released. Result: a `slug:` document with no pack and no set behind it, held against every + other user **forever** — nothing in the module can free it (`releaseSlug` needs both owner and + slug, and the only record of the slug was just deleted). Manual DB surgery is the only recovery. +- `handleNewPack:111-113` has the same shape: a deadline-exceeded bail calls `releaseSlug` with the + dead context, so the "release only what this invocation created" repair silently no-ops. + +Fix direction: derive the commit context once at the top of `releaseSlug` / `dropPackRecord` / +`dropPackRecordIfSet` and use it for the read as well as the write. These reads are part of the +commit, not part of serving the request. + +## MEDIUM — uncancellable image work stalls every user of the bot + +`internal/modules/sticker/image.go:35` (`maxDecodeDimension = 4096`), `:63-79` (the fallback ladder, +which re-scales from the **full-size** source `img` on every rung), reached from `/addsticker`, +`/newpack` and `/setpackicon`. + +`toStickerPNG` takes no context and checks none, so `handlerTimeout` bounds nothing here, and +handlers are strictly serialized (one worker), so this is a whole-bot stall, not a per-user one. +Measured on this box (ARM64, Go 1.27) with a 4096×4096 PNG of random 8×8 blocks — 1,278,612 bytes, +comfortably under the 2 MiB `maxSourceBytes` cap, and its 512px downscale is per-pixel noise, so +every PNG encode overshoots `softMaxStickerBytes` and the full ladder runs: + +``` +decode 175 ms +512 DefaultCompression 553 ms → 787,362 B (> 512 KiB, ladder continues) +512 BestCompression 41 ms → 773,200 B +448 BestCompression 522 ms → 568,571 B +384 BestCompression 513 ms → 411,425 B +320 BestCompression 475 ms → 280,071 B +TOTAL 2.28 s of uninterruptible CPU per message +``` + +One user resending that image faster than every 2.3s keeps the single dispatch goroutine saturated; +all other users' commands queue behind it. Peak live memory is also ~64 MB for the decoded source +alone (as the comment at `image.go:29-34` acknowledges). + +Fix direction: scale the ladder rungs from the already-downscaled 512 image instead of `img` (drops +three of the four expensive 4096²→N CatmullRom passes); lower `maxDecodeDimension` to ~1536-2048 +(the target is 512px, so nothing above that adds quality); optionally take `ctx` and bail between +rungs. + +## LOW — `/newpack` pays for the image before the check that refuses the caller + +`internal/modules/sticker/pack_handlers.go:68` (`resolveSource`) runs before the lock, before the +"you already have a pack" pre-check at `:92`, and before `reserveSlug`. A user who already owns a +pack can make the bot download up to 2 MB, run the full conversion above, and call +`UploadStickerFile` on every `/newpack`, only to be refused by a single store read that could have +run first. Not a new primitive (the same work is legitimately available via `/addsticker`), and the +ordering is what keeps name-burning closed, so this is cost, not a hole. Moving the cheap +`getPack` pre-check above `resolveSource` preserves the reserve-after-precheck invariant and removes +the free work. + +## LOW — `handleRenamePack` commits a record it read before taking the lock + +`internal/modules/sticker/pack_handlers.go:531-551`: `getPack` runs at `:531`, the lock is taken at +`:540`, and `commitPack` at `:550` writes the whole document (`Count`, `Pending`, `Name`, …) from +that pre-lock read. `adjustCount:388-395` documents exactly why that is wrong and re-reads inside +the lock; rename does not. `handleDelPack:32-83` likewise reads and writes the pending record with +no lock at all. Neither is exploitable today — `WithNotAsyncHandlers` + one worker means no two +handlers ever interleave — so the locks are currently decorative and these are latent regressions +that surface the day async handlers or a second replica are introduced. + +--- + +## Attacked and held + +Callback path (`delpack_callback.go`), payload fully attacker-chosen: +- **Address someone else's confirmation** — held: the store lookup key is + `pendingDeleteKey(query.From.ID)` (`:108`); the payload id is never used to select *whose* action + loads, only compared for equality at `:144`. +- **Press a bystander's button in a group** — held: `msg.Chat.ID != action.ChatID || msg.ID != + action.MessageID` (`:137`) is evaluated *before* any side effect, so the bystander cannot even + strip the victim's keyboard; `clearButton` only runs after that binding passes. +- **Replay / double-press / two live confirmations** — held: deterministic per-user key + (`pending_delete.go:64`) so a second `/delpack` supersedes the first, and the action is consumed + with `pending.Delete` *before* `DeleteStickerSet` (`:159-167`). +- **Stale press after `/delpack` + `/newpack`** — held: `dropPackRecordIfSet` (`pack_handlers.go:431`) + re-checks `ownsSet` so a stale confirmation cannot erase the record of the *new* live pack. +- **Forwarded copy of the prompt / inaccessible message** — held: `query.Message.Message == nil` + guard at `:125` before any use, plus the message-id binding. +- **Malformed payload** — held: `parseDeleteCallback` bounds length to 64, requires the prefix, and + requires lowercase hex; ids are 12 random bytes from `crypto/rand`. +- **Anonymous / bot senders** — held for the command side: `senderID` rejects `IsBot` and + `SenderChat != nil` (`sender.go:28-38`), so the shared GroupAnonymousBot identity can never own or + delete a pack; `query.From.ID == 0` is rejected on the callback side. + +Reservation lifecycle — every `slug:` create/delete site enumerated: +create at `reserveSlug:139` only; delete at `handleNewPack:112` (only when `created`), +`resolveStaleIntent:300` (only after a positive `STICKERSET_INVALID` on the old name), +`createOrAdopt:353` (only when `createRefused`), `dropPackRecord:479` (only after a confirmed +`DeleteStickerSet` or a positive `STICKERSET_INVALID`). All four funnel through `releaseSlug`, +which re-verifies the holder itself (`:187`) rather than trusting the caller, so a delete-by-name +cross-user primitive does not exist. Unknown/transient errors change nothing +(`createOrAdopt:325-331`, `resolveStaleIntent:303-307`) — verified by +`TestNewPack_UnknownLookupErrorAborts` and `TestNewPack_ResumedReservationSurvivesABail`. +The only leak I could construct is the cancelled-context one filed as MEDIUM above. + +- **Name burning at zero API cost** — held: the existing-pack pre-check precedes `reserveSlug` + (`:92-101`) and the `created` flag stops a bail from releasing a resumed reservation. Sustained + burning also needs one Telegram account per name, since one pack per user is enforced by the + create-only `PutVersioned(packKey, 0, …)` and `/delpack` returns the name to the pool. +- **Key-space collision across the three views over one collection** — held: + `slugRe = ^[a-z][a-z0-9_]{2,39}$` cannot emit `:` or a leading digit, so `"slug:"+slug` is + disjoint from decimal `packKey` and from `"pending-delete:"+decimal`; `storage.validateKey` + additionally rejects `/`, `.`/`..` and `__ns__`, and `provider.Collection("sticker")` isolates the + module (`registry.go:151`). Callback prefixes are conflict-checked bidirectionally + (`registry.go:219`). +- **Mutating another user's stickers via a replied sticker** — held: `resolveOwned` compares the + *stored* `Pack.Name` against `Sticker.SetName`, which is authored by Telegram and not settable by + the sender; a copied sticker lands in the copier's own set with a new file id, and the original + message still carries the victim's `set_name`. +- **Enumeration** — held: `slugTaken` (`pack_handlers.go:23`) is byte-identical to the + `PACK_SHORT_NAME_OCCUPIED` mapping in `apiRefusal` (`errors.go:62`), so "reserved in this bot" and + "occupied on Telegram" are indistinguishable; `notOwnedRefusal` is the single answer for no-pack, + pending-pack, and foreign-set. Raw Telegram descriptions never reach a reply — `replyAPIError` + maps or genericises. +- **Bot-token leakage** — held on every path I could reach: the download path discards the original + error entirely (`errDownloadFailed`, `download.go:36-83`, no `%w` of the transport error) and logs + only `classify(err)`, which inspects types and never formats the error; API-call failures have the + token redacted inside `url.Error.URL` by the library itself; user replies echo only `userError` + text (`state.go:95-101`). Replies are sent with no `ParseMode`, so a 64-char attacker-chosen title + echoed in `/mypack` and the delete prompt cannot inject markup either. +- **Error classification** — held: `isStickerSetMissing` requires both `bot.ErrorBadRequest` and the + `STICKERSET_INVALID` code; `createRefused` is a separate positive-only list of + request-validation codes and is used only to authorise undoing an intent + reservation. No path + infers absence from a generic failure. +- **Bounds ordering** — held except as noted in LOW: MIME allowlist and `FileSize` are checked before + any byte is fetched (`photo.go:64,74`), `GetFile.FileSize` is re-checked server-side, the body is + read through `LimitReader(max+1)` with an explicit overflow check, `DecodeConfig` bounds dimensions + before any pixel buffer exists, and the 20-emoji cap is enforced before the API call. + +--- + +## Recommended actions + +1. Gate adoption on `created == false` (reservation pre-dated this `/newpack`), and refuse to run + this module on a non-durable store. — HIGH +2. Use `commitContext` for the reads inside `releaseSlug` / `dropPackRecord` / + `dropPackRecordIfSet`. — MEDIUM +3. Scale the fallback ladder from the 512px image and lower `maxDecodeDimension`. — MEDIUM +4. Move the existing-pack pre-check above `resolveSource` in `/newpack`. — LOW +5. Re-read inside the lock in `handleRenamePack` (match `adjustCount`), or state in the module doc + that serialization is guaranteed by the dispatcher and the locks are belt-and-braces. — LOW + +## Unresolved + +- `go test ./internal/modules/sticker/...` failed once on its first (uncached) run — tail showed two + `sticker_newpack_lookup … upstream is unhappy` ERROR lines then `FAIL` — and then passed 25+ + consecutive runs including `-count=8` and `-race`, and under 8-way CPU load. Not reproduced, not a + security finding, but flagging it for whoever owns the tests: the suspects are + `TestNewPack_UnknownLookupErrorAborts` / `TestNewPack_ResumedReservationSurvivesABail`. +- Whether Telegram permanently reserves the short name of a deleted set (plan R11) is still + unverified. The code handles both answers correctly, so this is an open fact, not a defect. diff --git a/plans/reports/test-review-260825-1515-sticker-module.md b/plans/reports/test-review-260825-1515-sticker-module.md new file mode 100644 index 0000000..e091d77 --- /dev/null +++ b/plans/reports/test-review-260825-1515-sticker-module.md @@ -0,0 +1,411 @@ +# Test / harness / wiring review — sticker module + +Reviewer lens: test quality, harness correctness, integration & wiring. +Method: read all sources, then **mutation-tested 22 production mutations** against the +suite. Security exploitation and deep state-machine correctness owned by other reviewers. + +Verdict: test quality is **high** — 19 of 22 mutations killed, and the ownership gates, +callback binding, self-heal, panic barrier and count bookkeeping are all genuinely pinned. +Two rounds previously claimed "all new tests non-vacuous"; that is **refuted in three +places**: the `created`/reservation-release machinery, two `/addsticker` emoji tests, and +the module-registration wiring. None is a bug in shipped behaviour today; all are real +holes that would let a future regression land green. + +--- + +## 1. Mutation-test results + +Every mutation applied to production source only, run, then reverted. Restoration verified +by md5 + `diff -r` (see §7). + +| # | Mutation applied | File | Guarding test(s) | Result | +|---|---|---|---|---| +| M1 | Delete the `ownsSet` gate from `resolveOwned` | resolve.go:116 | `TestResolveOwned_RefusalsAreIdentical` | **KILLED** | +| M2 | `!found \|\| pack.Pending` → `!found` | resolve.go:113 | `TestResolveOwned_PendingRefusesIdentically` | **KILLED** | +| M3 | Disable foreign-holder refusal in `reserveSlug` | pack_handlers.go:159 | `TestNewPack_CannotSeizeAnotherUsersPack`, `..._ForeignReservationRefusedBeforeAnyAPICall` | **KILLED** | +| M4 | Disable reservation-owner proof in `resolveStaleIntent` | pack_handlers.go:263 | `TestNewPack_StaleIntentCannotAdoptForeignName` | **KILLED** | +| **M5** | **`if created { releaseSlug }` → never release** | **pack_handlers.go:111** | *(none)* | **SURVIVED** | +| **M6** | **Delete the pre-reservation quota check entirely** | **pack_handlers.go:92-99** | *(none)* | **SURVIVED** | +| **M7** | **`reserveSlug` resumed path returns `created=true`** | **pack_handlers.go:165** | *(none)* | **SURVIVED** | +| M8 | `dropPackRecordIfSet` → `dropPackRecord` (blind by-owner delete) | delpack_callback.go:178 | `TestDelPackCallback_StalePressLeavesTheCurrentPackAlone` | **KILLED** | +| M9 | Disable chat/message binding check | delpack_callback.go:137 | `..._RejectsWrongBinding` (2 subtests), `..._BystanderCannotTouchAnotherUsersPrompt` | **KILLED** | +| M10 | Remove consume-before-destructive-call | delpack_callback.go:159 | `TestDelPackCallback_SecondPressIsInert` | **KILLED** | +| M11 | Disable expiry check | delpack_callback.go:149 | `TestDelPackCallback_RejectsExpired` | **KILLED** | +| M12b | `dropPackRecord` never releases the slug | pack_handlers.go:478 | `TestSelfHeal_ReleasesTheName`, `TestDelPackCallback_ReleasesTheName` | **KILLED** | +| M13 | `isStickerSetMissing` → `err != nil` (transient read as "gone") | errors.go:163 | 4 tests incl. both `_TransientErrorKeepsRecord` | **KILLED** | +| M14 | Remove the negative-count floor | pack_handlers.go:407 | `TestDelSticker_CountFlooredAtZero` | **KILLED** | +| M15 | Invert emoji precedence (replied beats explicit) | sticker_handlers.go:283 | `TestAddSticker_HappyPath`, `..._EmojiPrecedence/explicit_wins` | **KILLED** | +| **M16** | **Early-return before `AddStickerToSet` (no API call at all)** | **sticker_handlers.go:292** | *(none — see F2)* | **SURVIVED** | +| M17 | Drop the inherit-from-replied-sticker fallback | sticker_handlers.go:283 | `..._EmojiPrecedence/inherits_from_replied_sticker` | **KILLED** | +| M18 | Remove the command panic barrier | dispatcher.go:75 | `TestInstall_CommandPanicIsContained` (binary would die) | **KILLED** | +| M19 | Remove the command-hook panic barrier | dispatcher.go:83 | `TestInstall_CommandHookPanicIsContained` | **KILLED** | +| M20 | Callback barrier `onPanic` → nil (stop answering the query) | dispatcher.go:104 | `TestInstall_CallbackPanicIsContainedAndAnswered` | **KILLED** | +| M21 | `claimSlug` `PutVersioned(…,0,…)` → `Put` | pack_handlers.go:216 | `TestNewPack_DifferentSlugAdoptsExistingSet` | **KILLED** | +| M22 | `reserveSlug` `PutVersioned(…,0,…)` → `Put` | pack_handlers.go:139 | `TestNewPack_CannotSeize…`, `..._ForeignReservation…` | **KILLED** | +| **W1** | **Delete `"mypack": ""` from `expectedParameters`** | **cmd/server/command_menu_test.go:76** | *(none — see F4)* | **SURVIVED** | +| **W2** | **Unregister `sticker` from `factories()` (import removed too)** | **cmd/server/main.go:95** | *(none — see F5)* | **SURVIVED** | +| W3 | Corrupt a non-empty expectation (`"newpack": "WRONG"`) — control | command_menu_test.go:75 | `TestCommandDiscovery_AllPublicCommandsHaveSafeMetadata` | **KILLED** | + +Survivors M5/M6/M7 and W2 were each re-run against the **full** sticker suite / **full repo +suite** (`./...`), not just a `-run` subset. All still survived. + +--- + +## 2. Findings + +### F1 — HIGH — the `created` reservation-release machinery has zero test coverage + +`internal/modules/sticker/pack_handlers.go:106-115` and `:138-165`. + +Both directions of the `created` flag survive mutation: + +- **M5** (never release): survived the full suite. +- **M7** (always release, even a resumed reservation): survived the full suite. + +Both branches are reachable. I proved it with two temporary probe tests (since removed): + +- *Release-on-bail is real*: `/newpack newslug` while a pending record for `oldslug` + exists and the old set resolves → `reserveSlug` writes `newslug` (`created=true`), + `claimSlug`→`resolveStaleIntent` adopts `oldslug` and returns `done=true`, so + `releaseSlug(newslug)` must run. Under M5 the probe failed with + `newslug still reserved with no pack behind it - name burned`. Reservations are global + and permanent, so this is a per-invocation namespace leak. + **`TestNewPack_DifferentSlugAdoptsExistingSet` (pack_handlers_test.go:111) walks exactly + this path and asserts nothing about `newslug`.** One added line closes it. +- *Not-releasing-a-resumed-reservation is real*: reserve `oldslug` for the caller, pending + record under a different slug, `getStickerSet` fails unclassifiably → `resolveStaleIntent` + bails `done=true`. Under M7 that probe failed: the caller's pre-existing reservation was + destroyed, handing the name to the next asker while the set may still exist. + +**`TestNewPack_ResumedReservationSurvivesABail` (pack_handlers_test.go:573) does not test +what its name says.** In that test `claimSlug` returns `done=false` (same-slug resume), so +the `if created` branch is never reached; the bail happens later in `createOrAdopt`, which +never consults `created`. M7 survives it. It is a duplicate of +`TestNewPack_UnknownLookupErrorAborts` wearing a different name. + +Fix: assert `newslug` is unreserved in `TestNewPack_DifferentSlugAdoptsExistingSet`, and +re-point `TestNewPack_ResumedReservationSurvivesABail` at a bail inside +`claimSlug`/`resolveStaleIntent` (a pending record under a *different* slug + a 500 from +`getStickerSet` reaches it). + +### F2 — MEDIUM — two `/addsticker` emoji tests pass when no sticker is added at all + +`handlers_test.go:134-159` (`TestAddSticker_EmojiPrecedence`) and `:163-178` +(`TestAddSticker_FallsBackToDefaultEmoji`). + +Both use the pattern: + +```go +for _, call := range rb.Sent() { + if call.Method == "addStickerToSet" && !strings.Contains(call.Form["sticker"], tc.want) { + t.Errorf(...) + } +} +``` + +Zero matching calls ⇒ zero assertions ⇒ pass. **M16** confirms it: an early `return` placed +before `b.AddStickerToSet` leaves both tests green. They are half-live (M15/M17 kill +individual subtests via the emoji value) but they do not guard "a sticker was added". + +`TestAddSticker_HappyPath:108` has the right guard (`countMethod(...) != 1` + `Fatalf`). +Add the same two lines to both tests. Same latent shape at `resolve_test.go` — no, those +use explicit `countMethod` comparisons and are fine. + +### F3 — MEDIUM — `docs/sticker-packs.md` contradicts the reservation lifecycle it describes + +`docs/sticker-packs.md:99-101`: + +> "A name is claimed only when a pack is actually created, and it is released when that +> pack is deleted … A `/newpack` that is refused claims nothing." + +The first clause is false and inverts the module's central safety property. `reserveSlug` +writes the reservation **before Telegram is touched** — that write-ahead claim is precisely +what makes adoption safe, and `pack.go:236-239` plus `pack_handlers.go:120-137` say so at +length. A name is therefore held while a `/newpack` is merely *pending*, and +`TestNewPack_UnknownLookupErrorAborts:181` asserts the reservation **must survive** when no +pack was created. An interrupted `/newpack` holds its name indefinitely with no pack behind +it — the doc tells a reader the opposite. + +The second clause is also too strong: a *classified* refusal releases, but an unknown +`getStickerSet`/`createNewStickerSet` failure deliberately keeps both intent and +reservation (`pack_handlers.go:325-331, 347-356`). + +Everything else in the doc checks out against source: 512px long edge / 100×100 thumbnail +(`image.go:23,25`), 4096px cap (`maxDecodeDimension = 4096`), 2 MB (`maxSourceBytes`), +120 stickers, 1–20 emoji, 10-minute confirm TTL, 10-second handler deadline, slug rules, +`MODULES` semantics, uniform ownership refusals. + +### F4 — LOW — `command_menu_test` does not verify that a command has an expectation + +`cmd/server/command_menu_test.go:104`: `got != expectedParameters[command.Name]`. A missing +map key yields `""`, so any public command with empty `Parameters` that nobody added to the +map passes silently. **W1** confirms: deleting `"mypack": ""` changes nothing. Four of the +nine new entries (`mypack`, `delsticker`, `setpackicon`, `delpack`) are therefore +decorative. The test does not verify what its name ("AllPublicCommandsHaveSafeMetadata") +promises for parameterless commands. + +Fix: `want, ok := expectedParameters[command.Name]; if !ok { t.Errorf("no expectation for /%s") }`. +That also turns the test into the missing registration guard for F5. + +### F5 — MEDIUM — nothing pins that the sticker module is registered at all + +**W2**: with both the import and `"sticker": sticker.New` removed from `cmd/server/main.go`, +`go test ./...` is **fully green** — all 25 packages pass, including +`internal/modules/sticker` (its tests construct `state` directly and never go through +`factories()`). A bad merge or rebase that drops the factory line ships a bot with none of +the nine commands and a green CI. + +`command_menu_test.go` only iterates whatever `reg.PublicCommands()` returns, so an absent +module is invisible to it. Cheapest fix is F4's `ok` check, which makes the map an +inventory rather than a lookup. + +### F6 — LOW — `/newpack` documented but unregistered `MODULES` default changed silently + +`.env.example` flips `MODULES=` (empty ⇒ load everything) to an explicit 12-module list. +The list matches `factories()` exactly (verified key by key), so no module is dropped today. +But it is now a hand-maintained duplicate of `factories()` with no test tying the two +together — the next module added will be silently excluded for anyone starting from the +template. Worth a comment pointing at `factories()`, or a test. + +--- + +## 3. Harness review — `internal/testutil/recording_bot.go` + +All four questions checked empirically. **The harness changes are correct and +backwards-compatible.** + +- **`FailMethodCode` produces the real sentinels — confirmed.** The library switches on + `r.ErrorCode` decoded from the *body* and ignores the HTTP status + (`go-telegram/bot@v1.20.0/raw_request.go:102-131`). `FailMethodCode` marshals + `{"ok":false,"error_code":…,"description":…}`, so `errors.Is(err, bot.ErrorBadRequest)` + holds. `recording_bot_test.go:113` pins this, and `:130` pins the negative contrast for + bare `FailMethod`. The doc comment's `raw_request.go:103-125` citation is accurate. +- **`StubMethod` / `FailMethod` precedence is correct and tested.** `handle()` checks + `shouldFail` before `hasStub` (`:200-209`); `TestRecordingBot_FailureWinsOverStub` covers it. +- **`Reset()` is coherent and unchanged for existing callers.** It clears `calls` only — + which is exactly what it did before this changeset; the diff only adds a doc comment + explaining it. `nextMessageID` is deliberately not reset (IDs stay unique across a + Reset), which no caller depends on. All 13 other packages that use the harness call only + `Reset()`; **no module outside `sticker` uses `FailMethod`, `FailMethodCode` or + `StubMethod`**, so the new precedence rule cannot affect them. +- **Message IDs start at 1, not 0.** `handle()` increments *before* assigning + (`:192-195`), so the first `sendMessage` returns `message_id: 1`. This matters because + production rejects a binding with `action.MessageID == 0` (`delpack_callback.go:137`) — + I verified the full round trip (`/delpack` → press the button the handler itself + produced → `deleteStickerSet` fires exactly once). No collision. *(Observation only: + no test in the suite actually performs that round trip; the pieces are covered + separately.)* + +**One real harness concern (MEDIUM):** + +The multipart-parse tolerance (`:180`) is justified — I confirmed `getMe` genuinely fails +with `multipart: NextPart: EOF` and `ContentLength=-1`, so the old code made every +parameterless method untestable. But the tolerance is **wider than the justification**: I +posted a deliberately malformed multipart body (`Content-Type: multipart/form-data; +boundary=zzz` with non-multipart content) to `/sendMessage` and the harness answered +**HTTP 200** and recorded `{Method:sendMessage Form:map[]}`. A real Telegram would 400 it. + +Impact is bounded — the bot library always builds well-formed multipart, so production +cannot realistically emit garbage. The live risk is **masking**: any test asserting a form +field is *absent* would falsely pass if the whole form silently failed to parse. Such +assertions already exist outside this module, e.g. +`internal/modules/stock/dividend_flow_test.go:115,139` (`calls[1].Form["reply_markup"] != ""`). + +Suggested narrowing: tolerate only the empty-body case (`r.ContentLength <= 0`, or the +`NextPart: EOF` shape) and keep the 400 for genuinely malformed bodies; or record a +`ParseFailed bool` on `SentCall` so a masked parse is visible in `dumpCalls()`. + +--- + +## 4. Untested error branches + +Prioritised by blast radius if a bug landed there. `✗` = no test reaches the branch. + +**`pack_handlers.go` — state-corrupting or namespace-leaking:** + +- `:111` `if created { releaseSlug }` — ✗ **both directions** (F1). Name-burn / name-theft. +- `:178-196` `releaseSlug`'s own cross-user ownership guard (`held.OwnerID != ownerID`) — ✗. + Defence-in-depth against a bad call site, with zero coverage; a caller passing the wrong + owner would be caught only here. +- `:325-331` `createOrAdopt` default branch is covered, but **`:347-356` create failing with + an *unclassifiable* error** (intent + reservation must both survive) — ✗. This is the exact + mirror of `TestNewPack_UnknownLookupErrorAborts` and is the higher-risk half, since a + wrong answer here strands a slug whose set may exist. +- `:396-413` `adjustCount`'s `!found` → `storage.ErrNotFound` path, and both handler + fallbacks that consume it (`sticker_handlers.go:308-314`, `:352-359`) — ✗. These + synthesise a count for the reply; a bug shows the user a wrong number. +- `:147-158` `reserveSlug` conflict-but-unreadable (`getErr != nil || !found` → treat as + taken) — ✗. Comment calls out that guessing the other way *is* the takeover. +- `:220-229` `claimSlug` non-conflict store error / re-read failure — ✗. +- `:258-272` `resolveStaleIntent` reservation-read error, and both `Put` failures + (`:267`, `:296`) — ✗. +- `:283-286` adopt-commit failure — ✗. +- `:73-77` `resolver.resolve` (GetMe) failure, `:78-81` `makeSetName` failure through the + handler — ✗ (`makeSetName` is unit-tested, the handler branch is not). +- `:92-94` pre-check store error, `:495-499` `/mypack` store error, `:531-535` /`renamepack` + store error, `:542-547` `/renamepack`'s `isStickerSetMissing` self-heal — ✗. +- `:366-369`, `:550-554` commit failures — ✗ (both are best-effort by design). + +**`delpack_callback.go`:** + +- `:144-147` `action.ID != id` → clear button + "replaced by a newer /delpack" — ✗. + `TestDelPack_SecondPromptSupersedesTheFirst` is rejected earlier, at the *binding* check + (`:137`), so this branch and its `clearButton` side effect never execute in tests. +- `:97-100` malformed callback data through the handler — ✗ (`parseDeleteCallback` is + unit-tested at `delpack_callback_test.go:84` for the happy case only; no test feeds + over-length, non-hex or wrong-prefix data to `handleDelPackCallback`). +- `:105-107` `query.From.ID == 0`, `:118-120` `From.ID != action.OwnerID` — ✗ (both + unreachable given the owner-keyed lookup; defence in depth). +- `:113-116` pending-store read error, `:159-165` both consume-delete failure branches — ✗. +- `:32-36` `/delpack` store error and `:37-39` `!found` ("you don't have a pack") — ✗. +- `:70-73` `SendMessage` failure — ✗. Note this is the one path in the module that returns + a **raw** API error to the dispatcher rather than a `userError`/generic reply. +- `:80-83` pending `Put` failure — ✗. Leaves a live button with no server-side action. + +**`resolve.go`:** + +Well covered. Only `:108-111` `getPack` store error is ✗. `resolveSource`'s `replied == nil` +refusal (`:51-53`) is ✗ directly, though `resolveOwned`'s equivalent is tested. + +**Whole-feature gap — Phase 5 photo pipeline has no integrated coverage.** Every test +message is built by `stickerReply()`, which always sets `Sticker`. Grep confirms no test +constructs a `Photo:` / `Document:` reply and feeds it to a handler. Consequently +`resolvePhotoSource` (`photo.go:342`) is never executed, and **`handleSetPackIcon` +(`setpackicon.go:174`) is reached only by `TestHandlers_RefuseAnonymousSenders`, which +returns at the sender check before doing anything** — its download → resize → +`SetStickerSetThumbnail` → self-heal body is entirely untested. The pieces (`photoFileID`, +`downloadFile`, `toStickerPNG`, `toThumbnailPNG`) are individually well tested; the wiring +between them is not. `StubMethod("uploadStickerFile", …)` now makes this testable — that is +what the harness change was for. + +--- + +## 5. Test isolation, races, lint + +- `go test -race ./internal/modules/... ./internal/testutil/... ./cmd/server/...` — **clean, + exit 0, 0 `DATA RACE`**, all 17 packages ok. +- `golangci-lint run` on the four changed packages — **0 issues**. +- No shared mutable fixtures: every test builds its own `newTestState()` over a fresh + `storage.NewMemoryProvider()` and its own `RecordingBot`. No ordering dependence found. +- `syncBuffer` (dispatcher_panic_test.go:33) correctly mutex-guards the log sink for the + detached-goroutine test, and `waitForLog` polls rather than sleeping. Good. +- Two globals are mutated without isolation in `dispatcher_panic_test.go`: `log.SetDefault` + (restored via defer) and `metrics` counters (`metrics.Flush()` at :116, never reset). + Harmless today — nothing runs in parallel and no other test in `modules_test` asserts on + error counters — but the metrics assertion at :130 would become order-dependent if one + ever did. Worth a note, not a change. +- `seedPack` correctly seeds the slug reservation alongside the pack record + (handlers_test.go:45-50), and `seedInterrupted` does the same + (pack_handlers_test.go:387-398). Both carry a comment explaining that seeding the record + alone builds a state production cannot reach. This is the right instinct and it is why + M12b/M22 kill cleanly. + +## 6. Wiring & plan accuracy + +**Registered correctly:** all 9 commands appear in `sticker.go:22-82` with +`VisibilityPublic`, descriptions, and `Parameters` matching +`docs/command-parameter-conventions.md` (the new `<name...>` "required remaining text" row +is a genuine addition, used by `<title...>`). All 9 are menu-described and +parameter-documented. README table and `docs/sticker-packs.md` list the same 9. + +**Callback prefix is unique.** `callbackPrefix = "sticker_pack:"` vs the only other +callback in the repo, `stock`'s `"stock_div:"`. `registry.go:218-220` enforces this +**bidirectionally** (`HasPrefix` both ways), so the check is real, not nominal. + +**Plan accuracy** — `plan.md` status is honestly `partial`, and all 16 unchecked boxes are +live-Telegram smoke tests plus the unresolved R11 (does Telegram reserve deleted short +names). No inflated completion. Two checked boxes are contradicted by shipped code: + +- `phase-03:245` — "`/newpack` where `GetStickerSet` fails with a non-missing error aborts, + **deletes the pending record**, and never calls `CreateNewStickerSet`" is `[x]`, but the + shipped code deliberately **keeps** both intent and reservation + (`pack_handlers.go:325-331`), and `TestNewPack_UnknownLookupErrorAborts:174-183` asserts + that. The file's own superseding note at `phase-03:52-55` says this step is wrong — the + checkbox was ticked against the superseded text. +- `phase-03:225` — "`sticker.go` factory registering **4 commands** + callback prefix" is + `[x]`; the shipped factory registers 9. Stale phase-scoped wording, harmless. + +**`internal/modules/wordle/lookup_test.go`** — confirmed a pure `gofmt` alignment change. +The 8 map keys and all 8 values are byte-identical; only leading whitespace inside the +composite literal moved (the longest key `" crane "` no longer forces extra padding). +No behaviour change. + +## 7. Final state + +Every mutated file restored and verified byte-identical against a pre-review backup: + +``` +md5sum -c backup/md5.txt → all 31 files OK (no mismatches) +diff -r backup/cmdserver cmd/server → cmd/server IDENTICAL +diff -r backup/testutil internal/testutil → testutil IDENTICAL +diff -r backup/sticker internal/modules/sticker → sticker IDENTICAL +diff backup/dispatcher.go internal/modules/dispatcher.go → dispatcher IDENTICAL +``` + +Three temporary probe test files were created and removed (`zz_probe_test.go`, +`zz_probe2_test.go` in `sticker`; `zz_probe_test.go` in `testutil`); none remain. + +`git status --short`: + +``` + M .env.example + M README.md + M cmd/server/command_menu_test.go + M cmd/server/main.go + M docs/command-parameter-conventions.md + M go.mod + M go.sum + M internal/modules/dispatcher.go + M internal/modules/wordle/lookup_test.go + M internal/testutil/recording_bot.go + M internal/testutil/recording_bot_test.go + M plans/260824-1051-sticker-pack-module/phase-01-shared-prerequisites.md + M plans/260824-1051-sticker-pack-module/phase-02-store-setname-emoji.md + M plans/260824-1051-sticker-pack-module/phase-03-pack-lifecycle.md + M plans/260824-1051-sticker-pack-module/phase-04-sticker-commands.md + M plans/260824-1051-sticker-pack-module/phase-05-photo-pipeline.md + M plans/260824-1051-sticker-pack-module/phase-06-wiring-docs.md + M plans/260824-1051-sticker-pack-module/plan.md +?? docs/sticker-packs.md +?? internal/modules/dispatcher_panic_test.go +?? internal/modules/sticker/ +?? plans/reports/correctness-review-260825-1515-sticker-module.md +?? plans/reports/security-review-260825-1515-sticker-module.md +``` + +Identical to the state at review start, plus the two peer reviewers' reports and this file. + +`go test ./...` — **all 25 packages ok**, zero failures. +`go test -race` on all module + testutil + cmd/server packages — **ok, 0 data races**. +`golangci-lint run` on changed packages — **0 issues**. + +## 8. Recommended actions + +1. **(F1, high)** Assert `newslug` is released in `TestNewPack_DifferentSlugAdoptsExistingSet`; + re-point `TestNewPack_ResumedReservationSurvivesABail` at a bail inside + `claimSlug`/`resolveStaleIntent` so it kills M7. Two tests, ~6 lines. +2. **(F5 + F4, medium)** Add the `want, ok := expectedParameters[name]` presence check in + `command_menu_test.go`. Closes both the decorative-entry hole and the missing + registration guard in one edit. +3. **(F2, medium)** Add the `countMethod(rb, "addStickerToSet") != 1` + `Fatalf` guard to + `TestAddSticker_EmojiPrecedence` and `TestAddSticker_FallsBackToDefaultEmoji`. +4. **(F3, medium)** Correct `docs/sticker-packs.md:99-101` to describe the write-ahead + reservation: the name is claimed *before* Telegram is called and is held while an + attempt is pending; only a positively-classified refusal or a confirmed delete releases it. +5. **(§3, medium)** Narrow the multipart tolerance to the empty-body case, or surface a + `ParseFailed` flag on `SentCall`. +6. **(§4, medium)** Add one integrated photo test (`StubMethod("uploadStickerFile", …)`) + and one `handleSetPackIcon` happy path — the largest untested surface in the module. +7. **(§4, low)** Cover `createOrAdopt`'s unclassifiable-create-error branch and + `delpack_callback.go:144` (`action.ID != id`). +8. **(§6, low)** Untick or correct `phase-03:245`; fix the "4 commands" wording at `:225`. + +## 9. Unresolved questions + +- `phase-06` leaves R11 (does Telegram permanently reserve a deleted short name?) open, and + `dropPackRecord`'s comment reasons about it both ways. Nothing here can settle it without + a live token; the code's behaviour is safe under either answer, so it is correctly + deferred to the manual smoke list. +- Is the `.env.example` switch from empty-`MODULES` to an explicit list intended to change + deployed behaviour, or only to document intent? If deployments copy the template, adding + a future module will require an `.env` edit that nothing warns about.