From 9c3c5457f985a6312849c875bc09a2d5a504cd0b Mon Sep 17 00:00:00 2001 From: tiennm99 Date: Tue, 25 Aug 2026 09:32:22 +0700 Subject: [PATCH] docs(plans): add sticker packs module plan Plan for internal/modules/sticker: public, multi-pack-per-user Telegram sticker set management using @Stickers command names, single-shot reply+args instead of a conversational flow. Six phases, 117 tasks. Phase 1 covers two shared-code prerequisites the module would otherwise expose: no panic barrier on the update path, and a test harness that cannot return structured API results. Researched against the live Bot API and red-teamed by three adversarial reviewers; 16 findings accepted, recorded in plan.md. Notable corrections: - MODULES is not opt-in; an empty value loads every module, so the factories() entry is itself the enablement - getStickerSet exposing no owner does not force "orphans cannot be adopted"; a write-ahead intent record makes recovery sound - the file-download URL embeds the bot token and reaches the dispatcher log through url.Error, which logging file_id does not prevent - /packlist ran ten API calls under a 60s per-call ceiling, a worse stall than the photo pipeline the plan had been guarding - the /delsticker probe deleted a live pack's record on any transient error - /help has 884 runes of headroom for nine new commands --- .../phase-01-shared-prerequisites.md | 160 +++++++++ .../phase-02-store-setname-emoji.md | 185 ++++++++++ .../phase-03-pack-lifecycle.md | 224 ++++++++++++ .../phase-04-sticker-commands.md | 171 +++++++++ .../phase-05-photo-pipeline.md | 208 +++++++++++ .../phase-06-wiring-docs.md | 168 +++++++++ plans/260824-1051-sticker-pack-module/plan.md | 335 ++++++++++++++++++ 7 files changed, 1451 insertions(+) create mode 100644 plans/260824-1051-sticker-pack-module/phase-01-shared-prerequisites.md create mode 100644 plans/260824-1051-sticker-pack-module/phase-02-store-setname-emoji.md create mode 100644 plans/260824-1051-sticker-pack-module/phase-03-pack-lifecycle.md create mode 100644 plans/260824-1051-sticker-pack-module/phase-04-sticker-commands.md create mode 100644 plans/260824-1051-sticker-pack-module/phase-05-photo-pipeline.md create mode 100644 plans/260824-1051-sticker-pack-module/phase-06-wiring-docs.md create mode 100644 plans/260824-1051-sticker-pack-module/plan.md 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 new file mode 100644 index 0000000..cf3de27 --- /dev/null +++ b/plans/260824-1051-sticker-pack-module/phase-01-shared-prerequisites.md @@ -0,0 +1,160 @@ +--- +phase: 1 +title: "Phase 1: Shared prerequisites" +status: todo +priority: P1 +effort: "4h" +dependencies: [] +--- + +# Phase 1: Shared prerequisites + +## Overview + +Two gaps in shared code that the sticker module would otherwise expose. Both live outside +`internal/modules/sticker` and both block every later phase, so they land first and merge +independently of the feature. + +1. **No panic barrier on the update path** (plan C9) — a handler panic terminates the + process, and Phase 5 proposes decoding attacker-supplied images in a handler. +2. **`RecordingBot` cannot return structured API results** (plan C11) — three later phases + have success criteria that are unimplementable against the current harness. + +## Requirements + +- Functional: a panic in any command or callback handler is recovered, logged with the same + context as a handler error, and does not terminate the process. +- Functional: tests can make any Bot API method return a chosen JSON result, and can make a + method fail with a Telegram-shaped error carrying an `error_code`. +- Non-functional: no behaviour change for existing modules; every existing test passes + unmodified. +- Non-functional: the recovery path must not swallow the failure silently — it increments an + error metric and logs at ERROR. + +## Architecture + +### Panic barrier in `modules.Install` + +`internal/modules/dispatcher.go:65-111` registers two closures per module — one for commands, +one for callback data. Neither recovers. With `bot.WithNotAsyncHandlers()` (plan C1) the +handler runs inline on the single polling goroutine, so an unrecovered panic ends the +process for every user. + +The repo already has the exact pattern to copy at `internal/cron/scheduler.go:66-74`, which +recovers around cron handlers. Mirror it: + +```go +defer func() { + if rec := recover(); rec != nil { + metrics.IncError("handler-panic") + log.Error("command panic", "command", cmdCopy.Name, "recovered", rec, + "stack", string(debug.Stack())) + } +}() +``` + +Apply to both closures. The callback variant should also attempt `AnswerCallbackQuery` so +the user's client stops showing a spinner, guarded so a failure there cannot panic again. + +**Also fix the stale comment at `internal/modules/dispatcher.go:167`,** which claims +"would panic the goroutine before our `recover()` in webhook.go". +`internal/telegram/webhook.go` contains only `DeleteWebhook` — there has been no +webhook-served handler since the move to long polling. The comment currently tells a reader +a protection exists that does not. + +### `RecordingBot` structured responses + +`internal/testutil/recording_bot.go:178-195` (`okResponseFor`) returns +`{"ok":true,"result":true}` for every method not in `isMessageProducingMethod` +(`:156-165`). Methods decoding into a struct therefore always error: + +| Method | Decodes into | Current test behaviour | +|---|---|---| +| `getStickerSet` | `*models.StickerSet` | `json: cannot unmarshal bool` | +| `getFile` | `*models.File` | same | +| `uploadStickerFile` | `*models.File` | same | +| `getMe` | `*models.User` | same | + +Add two capabilities, both additive: + +```go +// StubMethod makes method return the given JSON as its "result" field. +func (rb *RecordingBot) StubMethod(method string, resultJSON string) + +// FailMethodCode makes method fail with a Telegram-shaped error carrying an +// error_code, so library errors take the same ErrorBadRequest / ErrorForbidden +// shape production emits. +func (rb *RecordingBot) FailMethodCode(method string, errorCode int, description string) +``` + +`FailMethod` (`:99-112`) stays as-is for existing callers, but its doc comment must state +that it produces a **codeless** failure that does **not** take the `ErrorBadRequest` shape — +that distinction is what plan rule 4 depends on, and a future reader must not confuse the two. + +Precedence when both a stub and a failure are registered for one method: the failure wins, +so a test can override a stubbed happy path without unregistering it. + +### Why this is a separate phase + +Both changes touch files every other module's tests depend on +(`internal/modules/dispatcher.go`, `internal/testutil/recording_bot.go`). Landing them +alone, with the full suite green, keeps the blast radius reviewable and means a problem here +is not entangled with sticker logic. + +## Related Code Files + +- Modify: `internal/modules/dispatcher.go` (recover in both closures; fix the stale comment) +- Modify: `internal/testutil/recording_bot.go` (`StubMethod`, `FailMethodCode`, doc fix) +- Create: `internal/modules/dispatcher_panic_test.go` +- Modify: `internal/testutil/recording_bot_test.go` +- Reference: `internal/cron/scheduler.go:66-74` (the pattern to mirror) +- Reference: `internal/metrics` (`IncError`), `internal/log` (`Error`) + +## Implementation Steps + +1. Add the recover to the command closure in `Install`, with metric + structured log. +2. Add the recover to the callback closure, including a guarded `AnswerCallbackQuery`. +3. Correct the stale comment at `dispatcher.go:167`. +4. Add `StubMethod` and `FailMethodCode` to `RecordingBot`; document `FailMethod`'s codeless + shape. +5. Tests per the Todo list. +6. Run the full suite — every existing test must pass untouched. + +## 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` + +## 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 + +## Risk Assessment + +**Recovering a panic can mask a real bug.** A handler that panics on every invocation would +now fail quietly per-request instead of crashing loudly. Mitigation: log at ERROR with the +full stack and increment a distinct `handler-panic` metric, so the condition is visible +rather than silent. This is the same trade the cron scheduler already made +(`cron/scheduler.go:66-74`); consistency with it is worth more than a second opinion here. + +**Changing shared test infrastructure can break other modules' tests.** Both additions are +new methods; no existing signature or default behaviour changes. The success criterion +"no changes to any existing test file other than additions" is what proves it. + +**Scope note.** The panic barrier is a pre-existing repo-wide gap, not one this module +introduces — the module only makes it far easier to reach. It is included here on an +explicit user decision rather than as silent scope expansion. 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 new file mode 100644 index 0000000..05b4ae1 --- /dev/null +++ b/plans/260824-1051-sticker-pack-module/phase-02-store-setname-emoji.md @@ -0,0 +1,185 @@ +--- +phase: 2 +title: "Phase 2: Store, set names, emoji parsing" +status: todo +priority: P1 +effort: "4h" +dependencies: [1] +--- + +# Phase 2: Store, set names, emoji parsing + +## Overview + +Pure, Telegram-free foundation: the persisted pack record, the mapping between a +user-facing slug and a Telegram set name, sender validation, and emoji-argument parsing. +Every function here is unit-testable without a bot. + +## Requirements + +- Functional: persist one record per pack, keyed so a lookup by the caller's own user ID is + itself the ownership check. +- Functional: build Telegram set names of the form `_by_`, and match a + sticker's `set_name` against stored packs **without** re-deriving it from the live username. +- Functional: reject senders that are bots or anonymous chat surrogates. +- Functional: split an emoji argument run into individual emoji, accepting `😂 🔥` and `😂🔥`. +- Non-functional: no network calls and no Telegram API types in the store layer. + +## Architecture + +### Pack record + +```go +// Pack is one bot-created sticker set owned by a Telegram user. +type Pack struct { + Slug string `bson:"slug"` // user-facing id, e.g. "my_memes" + Name string `bson:"name"` // Telegram set name, "my_memes_by_botname" + Title string `bson:"title"` // display title + OwnerID int64 `bson:"ownerId"` // Telegram user the set belongs to + Count int `bson:"count"` // stickers in the set; keeps /packlist API-free + Pending bool `bson:"pending"` // write-ahead intent; see Phase 3 + CreatedAt int64 `bson:"createdAt"` // unix millis +} +``` + +`Count` exists so `/packlist` makes zero API calls (plan C2 / R1). It is maintained by +`/addsticker` and `/delsticker` and is explicitly **advisory** — it can drift if a user edits +the pack through @Stickers. Phase 3 documents how it self-heals. + +`Pending` implements write-ahead intent (plan C5). Phase 3 owns the state machine. + +- Key: `packKey(ownerID int64, slug string)` → `strconv.FormatInt(ownerID,10) + ":" + slug`. + `:` is legal — `internal/storage/keys.go:24-41` forbids only `/`, `.`, `..`, `__…__`, empty, + and >1500 bytes. +- `listPacks(ctx, ownerID)` → `store.List(ctx, ownerID+":")` then `Get` each; sort by slug. + The N+1 is structural: `mongo_doc_store.go:157-165` projects `_id` only. +- Ownership is structural: a command derives the key from the *caller's* ID, so a hit means + the caller owns the pack. +- `maxPacksPerUser = 10` (plan O2), checked before create. +- `Pack`'s bson tags must not collide with `_id` / `version` / `updatedAt`; `storage.Typed` + panics on collision (`internal/storage/doc_store.go:72`). + +### Sender validation + +```go +// senderID returns the personal Telegram user behind msg, or an error when the +// message has no usable personal identity. +func senderID(msg *models.Message) (int64, error) +``` + +Rejects, in order: nil `msg`/`From`, zero ID, `From.IsBot`, and non-nil `msg.SenderChat`. + +The last two are not theoretical. Telegram substitutes a single global `GroupAnonymousBot` +user for **every** anonymous group-admin message and puts the real origin in `SenderChat` +(`models/message.go:86-87`). Without this check, all anonymous admins across all groups would +share one pack namespace and one quota, and `CreateNewStickerSet` with a bot `user_id` would +fail with an unmapped error. `rg "IsBot" internal/` returns zero hits today — `coin`, `gold`, +and `stock` all check only `From != nil && From.ID != 0`, which is safe for paper-trading +state but not for durable Telegram objects. + +The refusal text should explain the fix ("sticker packs need a personal account — turn off +anonymous posting for this message"), not just deny. + +### Set names + +- `slugRe = ^[a-z][a-z0-9_]{2,39}$` — 3 to 40 chars. Additionally reject `__` (Telegram + forbids consecutive underscores) and a trailing `_`. The 40-char cap is for link + readability and title budget; it is **no longer** coupled to callback-payload size, since + Phase 3 puts an opaque id in the callback rather than a slug. +- `buildSetName(slug, botUsername) (string, error)` → `slug + "_by_" + botUsername`, erroring + above 64 chars and reporting the remaining slug budget so the reply can say "max N + characters". +- **`matchPack(packs []Pack, setName string) (Pack, bool)`** — case-insensitive comparison of + `setName` against each `pack.Name`. This is the ownership resolver used by Phase 4. + + It deliberately replaces the earlier `parseSlug(setName, botUsername)` design. That version + re-derived the slug from the *live* username and discarded the persisted `Pack.Name` + entirely — so renaming the bot in BotFather (a supported operation that leaves existing set + names untouched) would make every user's own packs refuse as "not yours", while `/packlist` + still listed them. It also let a case variant in `SetName` miss the key. Matching the stored + name fixes both (plan R8). + +- `usernameResolver` caches `GetMe` and **must not cache failures**. The bot starts with + `bot.WithSkipGetMe()` (`internal/telegram/client.go:26`), so nothing populates a username + until the module asks. It is used **only** to build names for new packs — never for + ownership. It takes the handler's `b *bot.Bot`, **not** `deps.Bot`, which is documented + nil-safe (`internal/modules/module.go:88`) and is nil under + `modules.Build(nil, factories(), …, BuildOptions{})` (`cmd/server/command_menu_test.go:55`). + +### Emoji parsing + +`parseEmoji(args []string) ([]string, error)` — join arguments, then split into clusters: + +- keep ZWJ (`U+200D`) sequences together; +- absorb variation selectors (`U+FE0F`/`U+FE0E`), skin-tone modifiers (`U+1F3FB`–`U+1F3FF`), + and combining marks into the preceding cluster; +- pair regional indicators (`U+1F1E6`–`U+1F1FF`); +- keep keycap sequences (` U+FE0F U+20E3`) together. + +Reject non-emoji text with a usage error. Cap at 20 (`emoji_list` is documented 1–20; the +server's own message is the literal `too many emoji specified`). `defaultEmoji = "⭐"` for +sources carrying none. + +Note `models.Sticker.Emoji` is a **single string** (`models/sticker.go:23`), so emoji +inherited from a replied sticker yields at most one element. + +## Related Code Files + +- Create: `internal/modules/sticker/pack.go`, `setname.go`, `sender.go`, `emoji.go` +- Create: `internal/modules/sticker/pack_test.go`, `setname_test.go`, `sender_test.go`, + `emoji_test.go` +- Reference: `internal/storage/doc_store.go:38` (DocStore contract), `keys.go:24-41` +- Reference: `internal/modules/coin/handlers_test.go:36` (memory-store test pattern) +- Reference: `go-telegram/bot@v1.20.0` `models/message.go:86-87` (`SenderChat`), + `models/user.go:12` (`IsBot`), `models/sticker.go:23` (`Emoji` is one string) + +## Implementation Steps + +1. `pack.go` — record, key helpers, `listPacks`, `maxPacksPerUser`. +2. `sender.go` — `senderID` with the bot/anonymous refusals. +3. `setname.go` — slug validation, `buildSetName`, `matchPack`, cached resolver interface. +4. `emoji.go` — cluster scanner and `defaultEmoji`. +5. Tests per the Todo list. + +## Todo + +- [ ] Define `Pack` incl. `Count` and `Pending`; assert no reserved-bson collision +- [ ] `packKey` and `listPacks` with owner-prefix scan +- [ ] `maxPacksPerUser` quota helper +- [ ] `senderID` rejecting nil/zero/`IsBot`/`SenderChat` with an explanatory message +- [ ] `slugRe` validation incl. `__`, trailing `_`, 40-char cap +- [ ] `buildSetName` with 64-char guard and budget-reporting error +- [ ] `matchPack` 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 + +## Success Criteria + +- [ ] `listPacks` for owner A never returns owner B's packs +- [ ] Quota boundary tested at 9, 10, 11 packs +- [ ] Slug table rejects leading digit, `__`, trailing `_`, 2 chars, 41 chars +- [ ] `buildSetName` errors when `len(slug)+len("_by_"+username) > 64` +- [ ] `matchPack` matches `MyPack_by_Bot` against a stored `mypack_by_bot` +- [ ] `matchPack` returns false for a set name belonging to another bot +- [ ] A simulated bot username change does **not** break `matchPack` for existing packs +- [ ] `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 + +## Risk Assessment + +**Emoji cluster scanning is hand-rolled.** Go has no stdlib grapheme segmentation and a +dependency for this is disproportionate. Signal: a user reports an emoji split or rejected. +Response: extend the table-driven test with the failing sequence. If failures accumulate +across many scripts, take a segmentation dependency. + +**`Count` can drift.** A user editing their pack through @Stickers changes the real count +without the bot seeing it. Accepted: the field is advisory and only feeds a display column. +Phase 3 refreshes it opportunistically whenever a command already holds a `GetStickerSet` +response, so it self-heals without any command paying for a lookup it did not need. + +**The bot username is still a single point of failure for `/newpack`.** `matchPack` protects +existing packs from a rename (R8), but creation still builds `_by_`. +After a rename, old packs keep working and new ones use the new suffix — correct behaviour, +but worth stating so nobody "fixes" it later by rewriting stored names. 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 new file mode 100644 index 0000000..07be8e1 --- /dev/null +++ b/plans/260824-1051-sticker-pack-module/phase-03-pack-lifecycle.md @@ -0,0 +1,224 @@ +--- +phase: 3 +title: "Phase 3: Pack lifecycle commands" +status: todo +priority: P1 +effort: "8h" +dependencies: [1, 2] +--- + +# Phase 3: Pack lifecycle commands + +## Overview + +`/newpack`, `/packlist`, `/renamepack`, `/delpack` (+ confirm callback). These own pack +creation and destruction, and they carry the plan's two hardest correctness problems: +surviving partial failure, and making an irreversible delete safe to confirm. + +All handlers follow the plan's cross-cutting rules (explicit deadline, `WithoutCancel` +commits, `senderID`, positive error classification). + +## Requirements + +- Functional: create a sticker set on behalf of the caller and persist its record such that + no interruption can permanently strand the slug. +- Functional: list, rename, and delete the caller's own packs, never another user's. +- Functional: `/delpack` confirmation is bound to invoker, chat, message, and a TTL. +- Functional: `/packlist` makes zero API calls. +- Non-functional: a store record must never claim a pack that does not exist, and a live + pack must never lose its record because of a transient error. + +## Architecture + +### Factory + +Mirrors `internal/modules/coin/coin.go`. `state` holds `store`, `pending` (a second typed +view for delete confirmations), `resolver`, `locks keylock.Map`, `nowFn`. Registry key is +`sticker`; command names stay unprefixed — the registry keys commands by `cmd.Name` +independent of module name (`registry.go:176`), which is why `misc` can ship `/ff`. + +`handlerTimeout = 10 * time.Second` is a package constant; every handler opens with it. + +### `/newpack ` — write-ahead intent + +The earlier draft did "create on Telegram, then write the store", and accepted that an +interruption strands the slug forever. Two review findings killed that: the commit ran on +`rootCtx`, which SIGTERM cancels, so **every deploy** during a `/newpack` stranded a slug; +and the "cannot adopt" rule was not actually forced by the API's missing owner field (plan +C5). The bot does not need the API to name the owner — it needs its own record of who asked. + +1. `senderID(msg)`; `defer s.locks.Acquire(...)()`. +2. Parse slug and title (1–64 chars). +3. Quota check via `listPacks`. +4. `buildSetName(slug, username)`. +5. **`PutVersioned(ctx, key, 0, Pack{Pending: true, …})`** — the create-only primitive + (`doc_store.go:33-37`; Mongo gives a linearizable single-winner via duplicate-key, + `mongo_doc_store.go:87-105`). `ErrConflict` means this owner already has a record for + this slug: if it is `Pending`, resume at step 6 (this is the retry path); if confirmed, + reply "you already have that pack". + Use `Put` nowhere here — it is a 5-attempt Get→PutVersioned loop + (`mongo_doc_store.go:120-142`) that silently overwrites. +6. `GetStickerSet(name)`: + - **succeeds** → the set exists. We hold a `Pending` record for this owner and slug, so + this is our own interrupted attempt: **adopt it**, jump to step 8. + If we did *not* hold a pending record we would not have reached here — step 5 would + have created one — so adoption is only ever of our own intent. + - **`isStickerSetMissing`** → the slug is free; proceed to step 7. + - **any other error** → unknown. Abort, delete the pending record, reply generic failure. + Never guess (plan rule 4). +7. `CreateNewStickerSet{UserID, Name, Title, Stickers: []InputSticker{{Sticker: fileID, Format: "static", EmojiList: emoji}}}`. + No top-level `sticker_format` — it moved to `InputSticker.Format` in Bot API 7.2 (C7). + On `PACK_SHORT_NAME_OCCUPIED`, another user of this bot holds the slug: delete the + pending record and ask for a different slug. +8. Commit: `Put(context.WithoutCancel(ctx), key, Pack{Pending: false, Count: 1, …})`. + Reply with the title and `https://t.me/addstickers/`. + +Re-running `/newpack` with the same slug after any interruption now completes the operation +instead of reporting it taken. That is the plan's "interrupted `/newpack` can be completed by +re-running" success criterion. + +A pending record does consume a quota slot until resolved. Acceptable at +`maxPacksPerUser = 10`; a stale-pending sweep is a follow-up, not this phase. + +### `/packlist` — zero API calls + +`listPacks(senderID)` rendered with `chathelper.MonospaceTable` (slug, title, `Count`, link). +Pending records render with a "(incomplete — re-run /newpack)" marker rather than being +hidden, so a user can see and fix a stranded attempt. + +The earlier design issued one `GetStickerSet` per pack. Under plan C2 each call is bounded +only by the library's 60s `http.Client` timeout (`bot.go:17-18,75-77`), so ten of them on a +serialized dispatcher is a ~10-minute bot-wide freeze from one argument-free public command — +strictly worse than the photo pipeline the plan had been treating as its main risk. Counts +now come from `Pack.Count` (Phase 2). + +### `/renamepack ` + +Ownership via `store.Get(packKey(sender, slug))`. `SetStickerSetTitle{Name, Title}`, then +commit the new `Title` under `WithoutCancel`. The reply must state the share link is +unchanged — the Telegram short name is permanent. + +**Reverse gap (was undocumented):** if the API succeeds and the commit fails, `/packlist` +shows a title Telegram no longer has. Cosmetic only, self-heals on the next successful +rename. Documented rather than mitigated. + +### `/delpack ` — bound confirmation + +The earlier design put the slug in the callback data and re-checked ownership from +`CallbackQuery.From.ID`. That defends against *other* users pressing the button and nothing +else: the payload never expires, is not bound to a chat or message, and lives in scrollback +forever. Alice cancels, recreates `memes` months later with 100 stickers, taps the stale +button, and it is destroyed with no fresh intent. Three reviewers flagged it independently, +and the `stock` module the draft cited as its model already solves this properly. + +Follow `stock` fully, not half of it: + +1. `/delpack` validates ownership, then writes a pending action: + `pendingDelete{ID, OwnerID, Slug, ChatID, MessageID, ExpiresAt}` with + `pendingDeleteTTL = 10 * time.Minute` (`stock/pending_dividend.go:12-16,26-34` uses 24h + for a non-destructive action; a destructive one earns a shorter window). +2. Callback data is `sticker_pack:d:` — an id, not a slug. Well under the 64-byte + cap (C3), and it decouples the payload from slug length entirely. +3. The callback handler: + - returns early when `update.CallbackQuery == nil`; + - resolves the pending action; absent → answer "this confirmation expired or was already + used" (`stock/dividend_callback.go:66-80` is the model); + - checks `query.From.ID == action.OwnerID` — identity from `From.ID`, **never** the payload; + - checks the chat/message binding and `ExpiresAt`; + - guards `query.Message.Message` for nil — it is a `MaybeInaccessibleMessage` + (`models/message.go:17-21`) and is nil for messages Telegram marks inaccessible. + `stock/dividend_callback.go:82-83` already guards exactly this. Phase 1's panic barrier + is the backstop, not the excuse to skip the guard; + - deletes the pending action **before** calling `DeleteStickerSet` (single-use); + - `DeleteStickerSet{Name}`, then `store.Delete` under `WithoutCancel`; + - `AnswerCallbackQuery` and clear the button via `EditMessageReplyMarkup` with empty + markup, using `action.ChatID`/`action.MessageID` (`dividend_callback.go:26-33`). + +**Reverse gap (was undocumented):** if `DeleteStickerSet` succeeds and `store.Delete` fails, +a phantom record survives — consuming a quota slot, rendering in `/packlist`, and erroring on +every operation. This is worse than an orphan set, because the user cannot see why they are +at their limit. Mitigation: `/packlist` and every command that receives +`isStickerSetMissing` from the API deletes the offending record on the spot, so a phantom +self-heals on first contact. + +### Error mapping + +`replyAPIError` matches **MTProto code substrings**, never human text (plan rule 4 / R3). +Only `PACK_SHORT_NAME_OCCUPIED`, `PACK_SHORT_NAME_INVALID`, and `STICKER_EMOJI_INVALID` are +rewritten into prose by the Bot API server; everything else arrives as `Bad Request: `. + +| Match | Reply | +|---|---| +| `PACK_SHORT_NAME_OCCUPIED` / "already occupied" | slug taken, pick another | +| `PACK_SHORT_NAME_INVALID` / "invalid sticker set name" | slug rejected by Telegram | +| `PACK_TITLE_INVALID` | title rejected by Telegram | +| `STICKERSET_INVALID` | that pack no longer exists (and delete the record) | +| `STICKERS_TOO_MUCH` | pack is full (120 stickers) | +| `STICKER_EMOJI_INVALID` / "invalid sticker emojis" | emoji rejected | +| `too many emoji specified` | at most 20 emoji per sticker | +| anything else | generic failure; raw error to the dispatcher log | + +## Related Code Files + +- Create: `internal/modules/sticker/sticker.go`, `state.go`, `pack_handlers.go`, + `pending_delete.go`, `delpack_callback.go`, `errors.go`, and their tests +- Reference: `internal/modules/coin/coin.go`, `coin/handlers.go:51` (keylock idiom) +- Reference: `internal/modules/stock/pending_dividend.go:12-34,47-78`, + `stock/dividend_callback.go:17-33,66-83,99-102`, `stock/dividend_notifications.go:303-320` +- Reference: `internal/storage/doc_store.go:33-41`, `mongo_doc_store.go:87-142` + +## Implementation Steps + +1. `state.go`, `sticker.go` wiring four commands + the `sticker_pack:` callback. +2. `errors.go` with `replyAPIError` and `isStickerSetMissing`. +3. `/packlist` first — no mutations, no API calls, easiest to verify. +4. `/newpack` with the write-ahead state machine. +5. `/renamepack`. +6. `pending_delete.go`, `/delpack`, and the callback. +7. Tests per the Todo list, using Phase 1's `StubMethod` / `FailMethodCode`. + +## Todo + +- [ ] `state.go` with store, pending view, resolver, locks, nowFn, `handlerTimeout` +- [ ] `sticker.go` factory registering 4 commands + callback prefix +- [ ] `errors.go`: `replyAPIError` code table + `isStickerSetMissing` +- [ ] `/packlist` reading `Count`, marking pending records, zero API calls +- [ ] `/newpack` steps 1-8 incl. `PutVersioned` intent and adoption +- [ ] `/renamepack` with permanent-link note and `WithoutCancel` commit +- [ ] `pending_delete.go` with TTL, chat/message binding, opaque id +- [ ] `/delpack` emitting the bound confirm keyboard +- [ ] `delpack_callback.go` with expiry, binding, nil-message guard, single-use +- [ ] Record self-heal on `isStickerSetMissing` across commands +- [ ] Tests per the success criteria + +## Success Criteria + +- [ ] `/packlist` records **zero** entries in `RecordingBot.Sent()` +- [ ] Interrupted `/newpack` (pending record present, set exists) completes on re-run and does not report the slug taken +- [ ] `/newpack` where `GetStickerSet` fails with a non-missing error aborts, deletes the pending record, and never calls `CreateNewStickerSet` +- [ ] `/newpack` uses `PutVersioned(…, 0, …)`; a second create attempt surfaces `ErrConflict` rather than overwriting +- [ ] Foreign-pack access replies with the ownership error and records zero API calls +- [ ] Quota exceeded at 11 packs blocks before any API call +- [ ] `/delpack` confirm after `ExpiresAt` is refused as expired, with no `DeleteStickerSet` +- [ ] `/delpack` confirm from a different `From.ID` is refused, with no `DeleteStickerSet` +- [ ] `/delpack` confirm with a nil `CallbackQuery.Message.Message` is handled without panic +- [ ] Pressing the same confirm twice deletes once; the second press reports already-used +- [ ] Callback data is asserted ≤ 64 bytes +- [ ] A command receiving `STICKERSET_INVALID` deletes the stale record +- [ ] Title of 65 chars rejected locally, before any API call + +## Risk Assessment + +**The write-ahead state machine is the most intricate logic in the plan.** Its correctness +rests on one property: a `Pending` record for `(owner, slug)` means *this owner* asked for +*this name*, and only this bot can create `*_by_` names. If either half stops +holding, adoption becomes unsafe. Signal: a user reports adopting a pack they did not create. +Response: disable adoption (step 6 becomes "slug taken") and fall back to the documented +orphan gap — a one-line change, deliberately. + +**A pending record consumes quota until resolved.** A user who abandons an interrupted +`/newpack` sees 9 usable slots. Visible in `/packlist` with its marker, and re-running the +command clears it. A sweep for pending records older than an hour is a follow-up. + +**`/delpack` remains irreversible on Telegram's side.** The TTL and bindings reduce accidental +confirmation; they cannot undo a deliberate one. Do not add a `--force` bypass. diff --git a/plans/260824-1051-sticker-pack-module/phase-04-sticker-commands.md b/plans/260824-1051-sticker-pack-module/phase-04-sticker-commands.md new file mode 100644 index 0000000..822c35d --- /dev/null +++ b/plans/260824-1051-sticker-pack-module/phase-04-sticker-commands.md @@ -0,0 +1,171 @@ +--- +phase: 4 +title: "Phase 4: Sticker commands (reply path)" +status: todo +priority: P1 +effort: "5h" +dependencies: [1, 2, 3] +--- + +# Phase 4: Sticker commands (reply path) + +## Overview + +Per-sticker operations on packs the caller owns: `/addsticker` (existing-sticker source), +`/delsticker`, `/editsticker`, `/ordersticker`. All are driven by replying to a sticker. The +photo source arrives in Phase 5 through the same `/addsticker` handler. + +All handlers follow the plan's cross-cutting rules. + +## Requirements + +- Functional: add an existing static sticker to one of the caller's packs. +- Functional: remove, re-emoji, and reposition a sticker already in one of those packs. +- Non-functional: ownership is checked before any API call, and the refusal text is identical + for "another user's pack" and "another bot's pack". +- Non-functional: no transient error may delete a live pack's record. + +## Architecture + +### Shared resolution + +```go +// source of a NEW sticker: whatever the replied message carries +type stickerSource struct { + fileID string // static sticker file_id, or "" when a photo needs uploading + photo *photoRef // Phase 5; nil on the sticker path + emoji []string // from the replied sticker; at most one element +} +func (s *state) resolveSource(msg *models.Message) (stickerSource, error) + +// an EXISTING sticker in one of the caller's packs +type ownedSticker struct { + fileID string + pack Pack +} +func (s *state) resolveOwned(ctx context.Context, msg *models.Message, ownerID int64) (ownedSticker, error) +``` + +`resolveOwned` is the single ownership gate for `/delsticker`, `/editsticker`, +`/ordersticker`, and (Phase 5) `/setpackicon`: + +1. Require `msg.ReplyToMessage.Sticker`; else usage error. +2. Require a non-empty `Sticker.SetName`. +3. `listPacks(ownerID)`, then `matchPack(packs, sticker.SetName)` (Phase 2) — a + case-insensitive comparison against the **stored** `Pack.Name`. +4. No match → the caller does not own it. +5. **Steps 3 and 4 must produce byte-identical reply text.** Distinct messages would let a + user probe which slugs exist under other accounts. Pack *management* refusals stay uniform + even though `/newpack` deliberately discloses slug occupancy (see the plan's Accepted + disclosure section) — the two are different questions. +6. Reject stickers with `IsAnimated` or `IsVideo`. Static-only module; defence in depth. + +Note this uses `matchPack`, not a slug re-derived from the live bot username. Phase 2 +explains why (plan R8): a BotFather rename would otherwise make every user's own packs refuse +as "not yours" while `/packlist` still listed them. + +`models.Sticker.Emoji` is a single string (`models/sticker.go:23`), so `stickerSource.emoji` +from a replied sticker holds at most one element. + +### `/addsticker [emoji...]` + +Reply required. Slug from args, ownership from `store.Get`, source from `resolveSource`. +Emoji precedence: explicit args → the replied sticker's emoji → `defaultEmoji`. + +`AddStickerToSet{UserID: ownerID, Name: pack.Name, Sticker: InputSticker{Sticker: fileID, Format: "static", EmojiList: emoji}}`. + +`UserID` is the pack owner, always the caller — the module never lets a non-owner reach this +call. Take the per-user keylock. On success, increment `Pack.Count` and commit under +`WithoutCancel`. `STICKERS_TOO_MUCH` maps to "this pack is full (120 stickers)". + +### `/delsticker` + +No arguments. `resolveOwned`, then `DeleteStickerFromSet{Sticker: fileID}`. On success, +decrement `Pack.Count` (floor 0) and commit. + +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 does not depend on either answer. + +The earlier draft probed with `GetStickerSet` afterwards and deleted the local record when +the probe "reported not-found" — but never defined how not-found differs from a failed call, +and its own success criterion (`FailMethod` → "record confirmed removed") specified the +destructive reading. Under that design a 429, a DNS blip, or a SIGTERM-cancelled context +during a routine delete would erase the only record of a live 49-sticker pack, which plan C5 +makes unrecoverable without operator help. + +Corrected: **do not probe.** Decrement the count and stop. If the set really is gone, the next +command against it returns `STICKERSET_INVALID`, and the shared handler for that (Phase 3) +deletes the record then — a positive signal, per plan rule 4. This is simpler and strictly +safer than probing. + +### `/editsticker ` + +`resolveOwned`, `parseEmoji` (at least one required — an empty `emoji_list` is invalid), then +`SetStickerEmojiList{Sticker: fileID, EmojiList: emoji}`. + +Deviates from the approved command preview, which included a `` argument — see plan O1. + +### `/ordersticker ` + +`resolveOwned`, parse a non-negative integer, reject negatives locally. 0-based, stated in the +usage text. Do **not** bound the upper end locally — Telegram validates against the current +set size and a local copy would go stale. Its error goes through `replyAPIError`. + +`SetStickerPositionInSet{Sticker: fileID, Position: pos}`. + +## Related Code Files + +- Create: `internal/modules/sticker/resolve.go`, `sticker_handlers.go`, and their tests +- Modify: `internal/modules/sticker/sticker.go` (register four commands) +- Reference: `internal/modules/util/handlers_test.go:108-118` — an existing test synthesizing + `ReplyToMessage` with a `models.Sticker{FileID, FileUniqueID, SetName, Emoji}`. Exactly the + fixture shape every test here needs. +- Reference: `internal/testutil/update_builders.go` (`NewPrivateMessage`, `NewGroupMessage`) + +## Implementation Steps + +1. `resolve.go` with both helpers and the deliberately uniform not-owned reply. +2. The four handlers in `sticker_handlers.go`, each opening with `handlerTimeout`. +3. Register with `Parameters` per `docs/command-parameter-conventions.md`: + ` [emoji...]`, none, ``, ``. +4. Tests per the Todo list. + +## Todo + +- [ ] `resolveSource` sticker branch (photo branch stubbed for Phase 5) +- [ ] `resolveOwned` using `matchPack`, 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` + +## 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 +- [ ] Foreign-bot set and another user's slug 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 +- [ ] `/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 + +## Risk Assessment + +**The uniform-refusal requirement is easy to regress.** A later contributor improving the +error copy could split the two messages and reintroduce the disclosure. Mitigation: the test +asserts equality *between the two paths' replies* rather than asserting two fixed strings, so +the intent survives a rewrite of the copy. + +**`resolveOwned` costs a `listPacks` per invocation** — one `List` plus up to ten `Get`s +against storage, no API calls. Bounded by `maxPacksPerUser` and cheap relative to the +network round trip these commands already make. Signal it matters: storage latency shows up +in command timings. Response: cache the caller's packs for the duration of one handler, which +is a local change since every command resolves at most once. + +**`Count` drift is user-visible but harmless.** Editing a pack through @Stickers desyncs it. +Phase 3 refreshes it whenever a command already holds a `GetStickerSet` response, so it +self-heals without any command paying for a lookup it did not otherwise need. 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 new file mode 100644 index 0000000..cd23008 --- /dev/null +++ b/plans/260824-1051-sticker-pack-module/phase-05-photo-pipeline.md @@ -0,0 +1,208 @@ +--- +phase: 5 +title: "Phase 5: Photo pipeline and pack icon" +status: todo +priority: P1 +effort: "8h" +dependencies: [1, 2, 3, 4] +--- + +# Phase 5: Photo pipeline and pack icon + +## Overview + +Turn a replied-to photo (or image document) into a valid static sticker, and add +`/setpackicon`, which needs the same resizing machinery at a different output size. This is +the only phase doing network I/O and CPU work, and the only one handling the bot token. + +## Requirements + +- Functional: a replied photo or image document becomes a sticker with a 512px long edge and + a preserved aspect ratio. +- Functional: `/setpackicon` sets a pack's thumbnail from a sticker already in that pack. +- Non-functional: bounded in bytes and time — it blocks every other user while it runs (C1). +- Non-functional (**security**): the download URL embeds the bot token and must never reach a + log, a reply, or a returned error. + +## Architecture + +### Bounds + +Plan rule 1 already puts `handlerTimeout = 10s` on every handler, which is the outer bound. +This phase adds: + +| Bound | Value | Why | +|---|---|---| +| Source file size | reject above **2 MB** before downloading | Telegram-compressed `photo` sizes are typically well under 500 KB | +| Decoded dimensions | reject above 4096×4096 via `DecodeConfig` | Bounds peak allocation before any pixel buffer exists | +| HTTP client | explicit per-request timeout, not `http.DefaultClient` | The library's shared client is 60s (`bot.go:17-18`) — too long to inherit | + +Worst case is a ~10s bot-wide stall (C1). Bounded and observable, not zero. + +### Telegram's static-sticker format + +PNG or WEBP; **one side exactly 512px**, the other ≤512px. Pack thumbnails differ: PNG or +WEBP, exactly **100×100**, ≤128 KB. + +There is no documented file-size limit for static stickers — the widely-repeated 512 KB +figure appears in no current official page (plan R4). It is a client-side ceiling only and +must not be described as spec in code comments or user-facing text. + +### Source selection + +From `msg.ReplyToMessage`: + +- `Photo []PhotoSize` — pick the largest by `FileSize`; do not rely on Telegram's ordering. +- `Document` — accept only `MimeType` of `image/png`, `image/jpeg`, `image/webp`. Reject + anything else *before* downloading. + +Reject when `FileSize > 2<<20`. + +### Download — and the token-leak trap + +`GetFile{FileID}` → `b.FileDownloadLink(f)` (`bot.go:180-182`) → `http.Get`. + +`FileDownloadLink` returns `https://api.telegram.org/file/bot/`. **Every +transport failure from `http.Client.Do` returns a `*url.Error` whose `Error()` embeds the +full URL**, and `internal/modules/dispatcher.go:136-138` logs a handler's returned error +verbatim. A timeout mid-transfer — trivially reachable — would therefore print the bot token +to stdout, the Coolify log store, and any log shipper. + +The earlier draft's mitigation ("log the `file_id` instead") covered only deliberate logging +and missed this path entirely; its success criterion would have passed while the leak shipped. + +Correct handling, per plan rule 5 — **no error from this package may escape raw**: + +```go +var errDownloadFailed = errors.New("sticker: download failed") + +// ... +if err != nil { + log.Error("sticker download", "file_id", fileID, "reason", classify(err)) + return nil, fmt.Errorf("file_id=%s: %w", fileID, errDownloadFailed) +} +``` + +The original error is discarded, never wrapped — wrapping would keep the URL reachable +through `errors.Unwrap` and `%v`. `classify(err)` maps to a coarse label (`timeout`, +`transport`, `status`) that cannot contain a URL. + +Other rules: + +- `io.LimitReader(body, 2<<20)`; never trust `Content-Length`. +- Read fully into memory — bounded at 2 MB, so no temp files. + +### Decode / resize / encode + +`toStickerPNG(src []byte) ([]byte, error)`: + +1. `image.DecodeConfig` first — reject above 4096×4096 before allocating pixels. +2. `image.Decode` with `image/jpeg`, `image/png`, `image/gif` registered, plus + `golang.org/x/image/webp` (read-only decoder, same module). +3. Scale so the long edge is exactly 512 and the short edge is `round(short*512/long)`, + clamped to ≥1. A square input yields 512×512. +4. `draw.CatmullRom.Scale` into a fresh `*image.NRGBA` — preserves alpha. +5. `png.Encode`. Above the 512 KB client-side ceiling, retry with + `png.Encoder{CompressionLevel: png.BestCompression}`; if still over, step the long edge + down (448, 384, 320). Give up after 320. + +`toThumbnailPNG(src []byte) ([]byte, error)` runs the same pipeline to exactly 100×100, +padding the short edge with transparency to preserve aspect ratio. + +Both are pure functions over `[]byte` so they test without a network. + +### Upload + +`UploadStickerFile{UserID: ownerID, Sticker: &models.InputFileUpload{Filename: "sticker.png", Data: bytes.NewReader(png)}, StickerFormat: "static"}` +→ use the returned `File.FileID` as `InputSticker.Sticker`. + +Two steps, not stylistic: plan C6 shows the form builder honours `attach://` only for +`[]models.InputSticker`, so the single `InputSticker` in `AddStickerToSetParams` cannot carry +raw bytes; `*models.InputFileUpload` *is* handled (`build_request_form.go:87`). +`uploadStickerFile` still takes `sticker_format` even though `createNewStickerSet` lost its +top-level equivalent in Bot API 7.2. + +The returned `file_id` is consumed immediately, so its undocumented validity window never +matters. Do not restructure into upload-now-use-later. + +### `/setpackicon` + +No arguments; reply to a sticker in one of the caller's packs. + +1. `resolveOwned` (Phase 4). +2. `GetFile` + download that sticker's image, then `toThumbnailPNG`. +3. `SetStickerSetThumbnail{Name: pack.Name, UserID: ownerID, Thumbnail: &models.InputFileUpload{...}, Format: "static"}`. + +The API does accept a `file_id` string for `thumbnail` — the only documented restriction bars +HTTP URLs for animated/video. The reason to resize is the documented 100×100 requirement, +which a 512px sticker's `file_id` does not meet. Confirm in Phase 6's smoke test; if a raw +`file_id` is accepted and auto-resized, this collapses to one call. + +## Related Code Files + +- Modify: `go.mod`, `go.sum` — add `golang.org/x/image` +- Create: `internal/modules/sticker/download.go`, `image.go`, `setpackicon.go`, + `download_test.go`, `image_test.go` +- Modify: `internal/modules/sticker/resolve.go` (photo branch), `sticker_handlers.go` + (`/addsticker` photo path), `pack_handlers.go` (`/newpack` photo path), `sticker.go` +- Reference: `go-telegram/bot@v1.20.0` `bot.go:180-182`, `build_request_form.go:87,105` +- Reference: `internal/modules/dispatcher.go:136-138` (the log path the sentinel protects) + +## Implementation Steps + +1. `go get golang.org/x/image`; confirm a direct require and a clean `go mod tidy`. +2. `download.go` — bounded fetch, own client timeout, sentinel error conversion. +3. `image.go` — `toStickerPNG`, `toThumbnailPNG`. +4. Wire the photo branch into `resolveSource`, then `/addsticker` and `/newpack`. +5. `setpackicon.go` + registration. +6. Tests per the Todo list. + +## 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 + +## 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 + +## Risk Assessment + +**R1 — bot-wide stall.** Under C1 every photo request blocks all users for up to +`handlerTimeout`. Tight bounds cap the damage but do not remove it; a user in a loop can keep +the bot substantially stalled. + +- Signal: reply latency for unrelated commands spikes with image traffic. +- Response, in order: (a) lower `handlerTimeout` and the 2 MB cap; (b) offload the pipeline + to a detached goroutine that acks immediately and replies on completion, mirroring + `dispatcher.go:80-84`; (c) if neither suffices, revisit the Public visibility decision with + the user — their call, not a unilateral change. +- **If (b) is ever taken, C1's single-in-flight guarantee disappears**, and two things become + mandatory together: a package-level semaphore around image decoding, and a real look at the + `/newpack` quota check, which becomes genuinely racy rather than merely lock-protected. The + two are linked deliberately so neither is done without the other. + +**Untrusted image decoding.** Bounded by size and dimension checks; Go's decoders are +memory-safe. Peak allocation is ~64 MB per conversion at the 4096² cap, and C1 guarantees one +at a time. Phase 1's panic barrier is the backstop for a decoder panic — but it is a backstop, +not a licence to skip the dimension guard. + +**New dependency.** `golang.org/x/image` is the only one in the plan and the repo's first +*direct* `golang.org/x/*` requirement. Confined to `image.go`. If resampling disappoints, +swapping `CatmullRom` for `ApproxBiLinear` is one line (plan R9). 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 new file mode 100644 index 0000000..7b6640f --- /dev/null +++ b/plans/260824-1051-sticker-pack-module/phase-06-wiring-docs.md @@ -0,0 +1,168 @@ +--- +phase: 6 +title: "Phase 6: Wiring, menu, docs" +status: todo +priority: P2 +effort: "4h" +dependencies: [1, 2, 3, 4, 5] +--- + +# Phase 6: Wiring, menu, docs + +## Overview + +Register the module, make enabling it an explicit operator decision, and bring the +user-facing surfaces named in `AGENTS.md` § "Command Changes" into line. Nothing from Phases +2–5 is reachable by a user until this phase lands. + +## Requirements + +- Functional: the module is enabled only by an explicit `MODULES` entry. +- Functional: all nine commands appear in `/help` and the native menu with correct metadata. +- Non-functional: `/help` stays under the 4096-rune ceiling the test suite enforces. +- Non-functional: no stats migration — nothing is renamed or deleted. + +## Architecture + +### Enablement must come before registration + +`internal/modules/registry.go:107-116` expands an empty `MODULES` to **every** registered +factory, and its own comment calls that the documented contract. The repo ships +`.env.example:16` as `MODULES=` (empty) and `compose.yml:14` documents "empty = all modules". + +So adding `"sticker": sticker.New` to `factories()` **is** the enablement: a public, +write-capable module would go live on the next deploy with no operator decision. The earlier +draft claimed the opposite in three places — goal, requirement, and a success criterion a +reviewer would have ticked without testing. + +Ordered fix, per the user's decision: + +1. **First**, set `MODULES` explicitly in the deployed environment to the current eleven + modules, and verify the bot restarts with an unchanged command set: + `util,misc,amlich,monkeyd,wordle,loldle,lol,stock,gold,coin,stats` +2. **Then** add the `factories()` entry and merge. +3. Add `sticker` to `MODULES` when the operator chooses to turn it on. + +Only after step 1 is "remove `sticker` from `MODULES`" a genuine zero-deploy rollback. Before +it, that rollback requires enumerating eleven module names into an empty variable under +pressure. + +Update `.env.example` with the explicit list and a comment stating why, so a fresh clone does +not reintroduce the empty-means-everything trap for this module. + +### Registration + +One line in `factories()` (`cmd/server/main.go:83`). Plain string key, matching `util`, +`misc`, and `gold`; the `CollectionName` constant form in `lol`/`coin`/`stock` exists because +those packages reuse the name elsewhere, which this one does not. + +`Build` validates names against `^[a-z0-9_]{1,32}$` (`validate.go:10`) and rejects duplicates +(`registry.go:173`). All nine were verified free; re-run registry tests to catch later additions. + +### The `/help` rune budget — the real Phase 6 blocker + +`cmd/server/command_menu_test.go:110-113` renders the full `/help` body and `t.Fatalf`s above +`telegramMessageMaxRunesForTest = 4096` (`:116-119`). + +Measured at HEAD: **3212 runes, 45 public commands, 11 modules — 884 runes of headroom.** + +Nine commands plus a module header must fit in 884 runes: about **85 runes per command line**, +and that line is `InvocationSentence() + " " + SummarySentence()` +(`internal/modules/command_presentation.go:16-24`), so `Parameters` counts too. Worked +example: `/ordersticker .` is 24 runes, leaving ~61 for its description. + +Write the nine descriptions against that budget *before* wiring, and re-measure. If they do +not fit, decide then whether `/help` needs pagination — that is a separate change, not a +Phase 6 afterthought, and it must not be "solved" by trimming other modules' descriptions. + +### Test impact — which tests, and which only look related + +| Test | Uses real `factories()`? | Action | +|---|---|---| +| `main_test.go:111` `TestFactoriesIncludesExpectedModules` | No — builds only `{"gold","coin"}` | **No change needed** | +| `command_menu_test.go:19` `TestBotCommandMenu_...ModuleOrder` | No — synthetic `alpha`/`beta` | **No change needed** | +| `command_menu_test.go:54` `TestCommandDiscovery_AllPublicCommandsHaveSafeMetadata` | **Yes** — `modules.Build(nil, factories(), …)` | **Add all nine to `expectedParameters`**; also enforces ≤256-rune descriptions, no `Eg:`, no newlines, and the 4096-rune `/help` ceiling | +| `command_menu_test.go:121` `TestBotCommandMenu_StockDividendContracts` | Stock-specific | No change | + +### README and docs + +Module table row listing the nine commands, then a `### Sticker packs` section: packs are +created on behalf of the calling user; the bot manages only packs it created; the share link +is permanent even after `/renamepack`; static stickers and photos only; the 10-packs-per-user +and 120-stickers-per-pack caps; anonymous group admins are not supported and why. + +Note the relationship to `/stickerid` in `util` — it stays put. It is a private debug helper +for reading a `file_id`, not pack management. + +`docs/sticker-packs.md` carries: the full command reference with `Parameters` matching handler +usage text exactly (`docs/command-parameter-conventions.md` § Change Checklist); slug rules; +the image contract (512px long edge, PNG; thumbnails 100×100); what is deliberately absent +and why (usage-statistics commands — no Bot API support; animated/video/emoji packs — out of +scope; conversational flow — no message hook at `internal/modules/dispatcher.go:66`); the +accepted slug-occupancy disclosure; and the `Count` drift note. + +### Stats compatibility + +No command is renamed or deleted, so `AGENTS.md` § "Stats Compatibility" does not apply — it +governs renames and deletions. New commands accrue stats through the dispatcher hook +(`dispatcher.go:80-84`). + +## Related Code Files + +- Modify: deployed environment `MODULES` (**before** the code change), `.env.example` +- Modify: `cmd/server/main.go` (factory entry + import) +- Modify: `cmd/server/command_menu_test.go` (`expectedParameters`, +9) +- Modify: `README.md` +- Create: `docs/sticker-packs.md` + +## Implementation Steps + +1. Set `MODULES` explicitly in the deployed environment; verify an unchanged command set. +2. Draft the nine descriptions against the 85-rune budget; measure `RenderHelp` locally. +3. Add the factory entry and import. +4. Update `expectedParameters`; run `go test ./cmd/server/...`. +5. README row + section; `docs/sticker-packs.md`. +6. Full gate, then the manual smoke sequence. + +## Todo + +- [ ] Set `MODULES` explicitly in the deployed environment and verify +- [ ] Update `.env.example` with the explicit list and a why-comment +- [ ] Draft nine descriptions within the ~85-rune budget and measure +- [ ] 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 +- [ ] 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 +- [ ] 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 +- [ ] Smoke: reply to a photo with `/addsticker smoke_pack 😂`; sticker renders undistorted +- [ ] Smoke: `/packlist`, `/renamepack`, `/setpackicon`, `/ordersticker 0` all succeed +- [ ] Smoke: a **second** account is refused on `/addsticker smoke_pack` +- [ ] Smoke: an anonymous group admin is refused with the explanatory message +- [ ] Smoke: `/delpack smoke_pack` → confirm → link 404s; a second press reports already-used +- [ ] Smoke: observed error strings for an occupied slug and a full pack match `replyAPIError`, or the table is corrected + +## Risk Assessment + +**The error-code table is unverified until this phase.** Only three MTProto codes are +rewritten into prose by the Bot API server; the rest were inferred from the open-source +server's rewrite table, not a live reproduction (plan R3). The smoke sequence deliberately +includes an occupied-slug and a full-pack case. A mismatch is a polish gap, not a blocker — +the generic fallback means users see a sane message either way. + +**Manual smoke is the only Telegram-side coverage.** Every automated test uses +`RecordingBot`, which never contacts Telegram. CI cannot catch a wrong parameter name or a +rejected image format. The smoke sequence is mandatory before announcing the feature. + +**Enablement ordering is a process risk, not a code one.** If step 1 is skipped and the +factory entry merges first, the module goes live unannounced. Signal: the deployed bot +answers `/packlist` before anyone enabled it. Response: set `MODULES` immediately; the module +is otherwise harmless until someone runs a command. diff --git a/plans/260824-1051-sticker-pack-module/plan.md b/plans/260824-1051-sticker-pack-module/plan.md new file mode 100644 index 0000000..86d588f --- /dev/null +++ b/plans/260824-1051-sticker-pack-module/plan.md @@ -0,0 +1,335 @@ +--- +title: "Sticker packs module" +description: "internal/modules/sticker — public, multi-pack-per-user Telegram sticker set management via single-shot reply+args commands using @Stickers command names" +status: pending +priority: P2 +effort: "" +tags: ["sticker", "telegram-bot", "module"] +created: 2026-08-24 +branch: main +blockedBy: [] +blocks: [] +--- + +# Sticker packs module + +## Overview + +New module `internal/modules/sticker` letting **any** user create and manage their own +Telegram sticker packs through the bot. Packs are created on behalf of the calling user +(`user_id`), named `_by_`, and stay bot-manageable because the bot +created them. + +Command names mirror @Stickers (`/newpack`, `/addsticker`, …) but each command is +**single-shot**: one message carrying its arguments, optionally replying to a sticker or +photo. Commands are **unprefixed**, like the `misc` module (`/ff`, `/random`). + +Phase 1 fixes two shared-code gaps this module would otherwise expose. They are +prerequisites, not incidental work. + +## Goals + +| # | Goal | Priority | +|---|------|----------| +| 1 | Any user can create and fill a personal sticker pack without leaving the chat | P1 | +| 2 | Ownership is enforced structurally — a user can never mutate another's pack | P1 | +| 3 | No sticker command can stall the bot for other users beyond a bounded deadline | P1 | +| 4 | A partial failure never permanently strands a user's pack | P1 | +| 5 | Command names and semantics recognisable to @Stickers users | P2 | +| 6 | Enabling the module is an explicit operator decision | P2 | + +## Accepted scope + +| Decision | Value | +|---|---| +| Visibility | `VisibilityPublic` — every user manages their own packs | +| Pack model | Named packs, multiple per user, addressed by slug argument | +| Inputs | Existing static stickers (reply) + photos/image documents (reply) | +| Interaction | Single-shot reply + args; no conversation state, no `/cancel` | +| Naming | @Stickers command names, no module prefix | + +## Command surface + +All `VisibilityPublic`. `Parameters` follows `docs/command-parameter-conventions.md`. + +| Command | Parameters | Reply required | API calls | +|---|---|---|---| +| `/newpack` | ` ` | yes (sticker/photo) | `GetStickerSet`, `UploadStickerFile`*, `CreateNewStickerSet` | +| `/addsticker` | ` [emoji...]` | yes (sticker/photo) | `UploadStickerFile`*, `AddStickerToSet` | +| `/delsticker` | — | yes (sticker in own pack) | `DeleteStickerFromSet` | +| `/editsticker` | `` | yes (sticker in own pack) | `SetStickerEmojiList` | +| `/ordersticker` | `` | yes (sticker in own pack) | `SetStickerPositionInSet` | +| `/setpackicon` | — | yes (sticker in own pack) | `GetFile`, `SetStickerSetThumbnail` | +| `/renamepack` | ` ` | no | `SetStickerSetTitle` | +| `/delpack` | `` | no (inline confirm) | `DeleteStickerSet` | +| `/packlist` | — | no | **none** — counts come from the store | + +`*` only on the photo path. + +### Deviation from the approved command preview + +The preview showed `/editsticker my_memes 😂🔥` with an explicit pack argument. This plan +drops it: the replied-to sticker carries `set_name`, which is matched against the caller's +stored packs. `/delsticker`, `/editsticker`, `/ordersticker`, `/setpackicon` all resolve the +pack that way. Open question O1. + +### Dropped from @Stickers + +`/stats`, `/top`, `/packstats`, `/packtop`, `/topbypack`, `/packusagetop` report sticker +**usage counts**, which the Bot API does not expose. `/stats` is also already owned by the +`stats` module. `/newanimated`, `/newvideo`, `/newemojipack`, `/newmasks` are outside the +accepted static-only scope. `/cancel` is meaningless without conversation state. + +## Architecture constraints (verified against this repo and the live API) + +Each was checked against source, not assumed. C1–C8 survived adversarial review; C5 was +corrected. + +### C1 — Handlers are globally serialized + +`internal/telegram/client.go:27` passes `bot.WithNotAsyncHandlers()`, and the library's +`defaultWorkers = 1` (`bot.go:20`) with no `WithWorkers` override. `process_update.go:26-28` +runs the handler inline. One slow handler stalls **every** user. + +Caveat found in review: "one update at a time" is not "one goroutine". The cron scheduler +(`cmd/server/main.go:163`) and the detached per-command stats hook +(`internal/modules/dispatcher.go:80-84`) both run concurrently with handlers. Neither +touches pack state today, but the per-user keylock is therefore **not** redundant. + +### C2 — No per-update deadline, and the library's own ceiling is 60s + +Handler ctx is `rootCtx` (`cmd/server/main.go:107,214`), which has no deadline, so +`chathelper.FetchContext` (`chathelper.go:107-113`) returns a bare `WithCancel` that bounds +nothing. The only remaining ceiling is the library's shared +`http.Client{Timeout: time.Minute}` (`bot.go:17-18,75-77`). + +Consequence: **every** handler needs its own explicit deadline, not just the photo path. +Ten sequential API calls under a 60s per-call ceiling is a ~10-minute bot-wide freeze. + +### C3 — Callback data caps at 64 bytes + +`internal/modules/stock/pending_dividend.go:16-17` enforces `maxDividendCallbackBytes = 64` +against Telegram's limit. `/delpack` carries an opaque pending-action id, not a slug, so the +budget is comfortable. + +### C4 — Callback prefix conflicts are checked bidirectionally + +`internal/modules/registry.go:216-219`. `sticker_pack:` does not overlap `stock_div:`, the +only existing prefix. + +### C5 — The API cannot prove ownership, so the bot must record intent before acting + +`getStickerSet` returns only `name`, `title`, `sticker_type`, `stickers`, `thumbnail` +(`models/sticker_set.go:4-10`) — no owner field. + +The earlier draft concluded "therefore orphaned sets can never be adopted". Review showed +that does not follow: the bot does not need the API to name the owner, it needs **its own +record of who asked for that name**. Since only this bot can create `*_by_` +sets, a write-ahead intent record makes an existing set attributable. See R2 and Phase 3. + +### C6 — Raw bytes cannot ride on `AddStickerToSet` + +`build_request_form.go:105` handles `attach://` only for `[]models.InputSticker`. The single +`InputSticker` in `AddStickerToSetParams` (`methods_params.go:905-909`) falls through to +`addFormFieldDefault`, and `StickerAttachment` is `json:"-"` (`models/sticker.go:40`), so it +is silently dropped. Photo path must be `UploadStickerFile` (which accepts +`*models.InputFileUpload`, `build_request_form.go:87`) → use the returned `File.FileID`. + +### C7 — Library is current for stickers + +Bot API is at 10.3 (2026-08-24); last sticker changes were Bot API 7.2 (2024-03-31). +`v1.20.0` has `InputSticker.Format`, no top-level `sticker_format` on +`CreateNewStickerSetParams`, and `ReplaceStickerInSet`. No known gap. + +### C8 — Telegram limits (confirmed against official docs) + +| Limit | Value | +|---|---| +| Set name | 1–64 chars, letters/digits/underscore, begins with a letter, no consecutive underscores, ends `_by_` (case-insensitive) | +| Static sticker image | PNG or WEBP; one side **exactly** 512px, other ≤512px | +| Static sticker file size | **Not documented.** The widely-repeated 512 KB figure appears in no current official page (R4) | +| Set thumbnail | PNG/WEBP, exactly 100×100, ≤128 KB | +| Stickers per set | 120 regular/mask, 200 custom emoji | +| Emoji per sticker | 1–20 | +| Set title | 1–64 chars | +| Sets per bot | **Not documented** — no known ceiling | + +### C9 — There is no panic barrier on the update path + +`rg "recover()"` finds four sites: `testutil/mongotest`, `server/log_middleware.go:50`, +`monkeyd/export_job.go:52`, `cron/scheduler.go:67`. **None on the command or callback path.** +`internal/modules/dispatcher.go:167` carries a stale comment promising "our `recover()` in +webhook.go"; `internal/telegram/webhook.go` contains only `DeleteWebhook`. + +With C1, a panic in any handler terminates the process. Phase 1 closes this. + +### C10 — `MODULES` is not opt-in + +`internal/modules/registry.go:107-116` expands an empty list to every registered factory, +and its own comment calls that the documented contract (`.env.example:16` ships `MODULES=` +empty; `compose.yml:14` says "empty = all modules"). Adding a `factories()` entry **is** the +enablement. Phase 6 sets `MODULES` explicitly before merging. + +### C11 — `RecordingBot` cannot return structured results + +`internal/testutil/recording_bot.go:178-195` answers every non-message-producing method with +`{"ok":true,"result":true}`. `GetStickerSet`, `GetFile`, and `UploadStickerFile` decode into +structs, so under the current harness they can only ever **error**. `FailMethod` +(`:99-112`) emits no `error_code`, so library errors in tests never take the +`ErrorBadRequest` shape production emits. Phase 1 extends the harness. + +## Phases + +| # | 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 | + +## Dependencies + +Phase 1 blocks everything (shared code + test harness). Phase 2 blocks 3, 4, 5. Phase 3 +blocks 4 and 5. Phase 5 depends on Phase 4's `/addsticker` handler. Phase 6 last. No +cross-plan dependencies — the only other plan is completed and touches disjoint files. + +## Cross-cutting rules + +These apply to every handler in Phases 3–5. Stated once here rather than repeated. + +1. **Explicit deadline.** Every handler opens with + `ctx, cancel := context.WithTimeout(ctx, handlerTimeout)` (`handlerTimeout = 10s`, + package constant). Required by C2 — nothing else bounds a call. +2. **Durable writes survive shutdown.** Store writes that commit a completed Telegram-side + action use `context.WithoutCancel(ctx)` plus a short timeout, mirroring the existing + idiom at `dispatcher.go:78-84`. `rootCtx` is cancelled by SIGTERM mid-handler, so a + plain `ctx` write fails on every deploy (R2). +3. **One sender helper.** `senderID(msg) (int64, error)` rejects a nil `From`, `From.IsBot`, + and any message carrying `SenderChat`. Anonymous group admins share a single + `GroupAnonymousBot` id, so without this all anonymous admins across all groups share one + pack namespace and one quota (R6). +4. **Positive error classification only.** `isStickerSetMissing(err)` is + `errors.Is(err, bot.ErrorBadRequest) && strings.Contains(err.Error(), "STICKERSET_INVALID")`. + Any other error is "unknown — abort with no side effects". Never infer "absent" from a + generic failure (R3, R7). +5. **Errors from the download path never escape raw.** They are converted to a sentinel at + the boundary, discarding the original (R5). + +## New dependency + +`golang.org/x/image` — for `draw.CatmullRom` resampling. Stdlib decodes JPEG/PNG and encodes +PNG but ships no scaler, and stickers need an exact 512px long edge. Note it becomes the +repo's first *direct* `golang.org/x/*` requirement; all current ones are indirect. + +## Abuse surface + +Public module creating durable Telegram-side objects on a single-threaded dispatcher (C1): + +- `maxPacksPerUser = 10`, enforced before `CreateNewStickerSet`. +- `handlerTimeout = 10s` on every handler — the primary bound (C2). +- `/packlist` makes **zero** API calls; counts live on the `Pack` record. +- Photo source rejected above 2 MB; decoded dimensions capped at 4096×4096. +- Slug alphabet `^[a-z][a-z0-9_]{2,39}$`, no `__`, no trailing `_`. +- `internal/keylock` per-user serialization — genuinely load-bearing, not decorative, + because crons and the detached stats hook run concurrently with handlers (C1 caveat). +- Anonymous/bot senders refused outright (cross-cutting rule 3). + +## Risks + +| # | Risk | Signal it broke | Response | +|---|---|---|---| +| R1 | A handler stalls the bot for all users (C1) | Reply latency spikes for unrelated commands | `handlerTimeout` caps it at 10s; if still felt, lower it, then consider offloading the photo path (Phase 5 R1) | +| R2 | Partial failure strands a pack | User reports a slug reported taken that `/packlist` does not show | Write-ahead intent + `WithoutCancel` commits (Phase 3). Reverse gaps for `/delpack` and `/renamepack` documented in Phase 3 | +| R3 | Error-code matching drifts | Users see the generic reply where a specific one was expected | Match MTProto **codes**, never human text; confirm empirically in Phase 6 smoke | +| R4 | The 512 KB static-sticker ceiling may not be real | Uploads succeed above it, or fail below it | Client-side ceiling only; never stated as spec in user-facing text | +| R5 | Bot token leaks through a transport error | Any log line containing `api.telegram.org/file/bot` | Sentinel conversion at the download boundary + a test asserting the error text is clean (Phase 5) | +| R6 | Ownership collapses for anonymous senders | Two users see each other's packs | Cross-cutting rule 3 refuses them before any store access | +| R7 | A transient error deletes a live pack's record | `/packlist` loses an entry the user can still open 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 | + +## Accepted disclosure + +`/newpack` answers "is this slug taken?" for any slug, which reveals that *some* user of this +bot owns it. This is accepted, not solved: `t.me/addstickers/_by_` is publicly +probeable without the bot, so the command adds no information an attacker lacks. The earlier +draft claimed no disclosure while shipping this probe — the claim was wrong and is removed. +Pack *management* refusals remain deliberately uniform (Phase 4), because those would +otherwise disclose which of the caller's own guesses correspond to real packs. + +## Success Criteria + +- [ ] A non-admin user can reply to a sticker with `/newpack mypack My Pack` and receive a working `t.me/addstickers/mypack_by_` link +- [ ] `/addsticker mypack 😂` on a photo produces a valid static sticker with a 512px long edge and correct aspect ratio +- [ ] Managing a pack the caller does not own fails with an ownership error and makes zero API calls +- [ ] The not-owned reply text is byte-identical whether the set is another user's or another bot's +- [ ] `/packlist` shows slug, title, count, and link for the caller's packs only, making no API calls +- [ ] `/delpack` requires inline confirmation bound to invoker, chat, message, and a TTL +- [ ] Every handler is bounded by an explicit deadline; no handler can exceed `handlerTimeout` +- [ ] A panic in any module handler is contained and logged, and does not terminate the process +- [ ] An interrupted `/newpack` can be completed by re-running the same command +- [ ] Anonymous group admins and bot senders are refused before any store or API access +- [ ] No log line or error string contains the file-download URL +- [ ] Module is enabled only by an explicit `MODULES` entry, verified in a deployed environment +- [ ] `go test ./...`, `go vet ./...`, `gofmt -l .`, and `golangci-lint run` all clean + +## Open Questions + +- **O1** — Should `/editsticker` keep the explicit `` argument from the approved + command preview, or resolve the pack from the replied sticker's `set_name` as planned? +- **O2** — `maxPacksPerUser = 10` no longer drives `/packlist` cost now that counts are + stored, so it is a pure product choice. Keep 10? + +## Red Team Review + +### Session — 2026-08-25 +**Findings:** 28 raw from 3 reviewers → 16 after dedup (16 accepted, 0 rejected) +**Severity breakdown:** 7 Critical, 6 High, 3 Medium +**Reviewers:** Security Adversary (Fact Checker), Failure Mode Analyst (Flow Tracer), +Assumption Destroyer (Scope Auditor). All findings carried `file:line` evidence, so none +were filtered. Four of the seven Critical findings were independently corroborated by all +three reviewers. + +| # | Finding | Severity | Disposition | Applied To | +|---|---------|----------|-------------|------------| +| 1 | Empty `MODULES` loads all modules; "opt-in" false in 3 places | Critical | Accept | C10, Phase 6, Goals | +| 2 | No `recover()` on the update path; a handler panic kills the process | Critical | Accept | C9, Phase 1 | +| 3 | Bot token leaks via `*url.Error` into the dispatcher log | Critical | Accept | Rule 5, R5, Phase 5 | +| 4 | `/packlist` unbounded: 60s client timeout × 10 calls + N+1 | Critical | Accept | C2, Rule 1, Phase 3 | +| 5 | `/delsticker` probe deletes the record on any error | Critical | Accept | Rule 4, R7, Phase 4 | +| 6 | `/delpack` button is a permanent replayable delete capability | Critical | Accept | Phase 3 | +| 7 | `RecordingBot` cannot return structs; 3 phases untestable | Critical | Accept | C11, Phase 1 | +| 8 | Anonymous group admins share one `From.ID` | High | Accept | Rule 3, R6 | +| 9 | Bot rename orphans every pack; `Pack.Name` written never read | High | Accept | R8, Phase 2, Phase 4 | +| 10 | `GetStickerSet` not-found is an undefined branch | High | Accept | Rule 4, Phase 3 | +| 11 | Commit writes on `rootCtx`; deploy is the normal orphan path | High | Accept | Rule 2, R2, Phase 3 | +| 12 | `/help` 4096-rune ceiling; 884 runes headroom measured | High | Accept | Phase 6 | +| 13 | C5's no-adoption conclusion not forced by its premise | High | Accept | C5, R2, Phase 3 | +| 14 | `Put` used where create-only `PutVersioned` exists | Medium | Accept | Phase 3 | +| 15 | `parseSlug` case-preserving value reaches the storage key | Medium | Accept | Folded into #9 | +| 16 | `/newpack` oracle contradicts the plan's own no-disclosure claim | Medium | Accept | Accepted disclosure section | + +**User decisions taken during adjudication:** explicit `MODULES` before Phase 6; panic +barrier in `modules.Install` as Phase 1; module-wide deadline plus persisted counts; +write-ahead intent record for orphan recovery. + +### Whole-Plan Consistency Sweep +- Files reread: plan.md, phase-01-shared-prerequisites.md, phase-02-store-setname-emoji.md, + phase-03-pack-lifecycle.md, phase-04-sticker-commands.md, phase-05-photo-pipeline.md, + phase-06-wiring-docs.md +- Decision deltas checked: 14 (phase renumber 1-5 -> 1-6; `parseSlug` -> `matchPack`; + `/delsticker` probe removed; `/packlist` API-free via `Pack.Count`; `Pack.Pending` + write-ahead intent; callback payload slug -> opaque id; C5 rewritten; opt-in claims + removed; `handlerTimeout` cross-cutting rule; `senderID` rule; `isStickerSetMissing`; + download sentinel; slug cap decoupled from C3; `Put` -> `PutVersioned` for create) +- Reconciled stale references: 0 remaining. `parseSlug` survives only in phase-02 as the + named superseded design and in the red-team table as finding #15 — both deliberate + historical references, not live claims. "opt-in" survives only in C10's negative heading + and the finding that corrected it. +- Link integrity: all 6 phase links resolve; no orphan phase files. +- Cross-phase references verified consistent under the new numbering. +- Unresolved contradictions: 0 + +