mirror of
https://github.com/tiennm99/miti99bot.git
synced 2026-09-03 14:18:33 +00:00
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
This commit is contained in:
@@ -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.
|
||||
@@ -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 `<slug>_by_<bot_username>`, 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 (`<base> 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 `<slug>_by_<current username>`.
|
||||
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.
|
||||
@@ -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 <pack> <title...>` — 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/<name>`.
|
||||
|
||||
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 <pack> <title...>`
|
||||
|
||||
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 <pack>` — 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:<opaque id>` — 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: <CODE>`.
|
||||
|
||||
| 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_<bot_username>` 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.
|
||||
@@ -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 <pack> [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 <emoji...>`
|
||||
|
||||
`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 `<pack>` argument — see plan O1.
|
||||
|
||||
### `/ordersticker <position>`
|
||||
|
||||
`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`:
|
||||
`<pack> [emoji...]`, none, `<emoji...>`, `<position>`.
|
||||
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.
|
||||
@@ -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<TOKEN>/<path>`. **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).
|
||||
@@ -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 <position>.` 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.
|
||||
@@ -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 `<slug>_by_<bot_username>`, 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` | `<pack> <title...>` | yes (sticker/photo) | `GetStickerSet`, `UploadStickerFile`*, `CreateNewStickerSet` |
|
||||
| `/addsticker` | `<pack> [emoji...]` | yes (sticker/photo) | `UploadStickerFile`*, `AddStickerToSet` |
|
||||
| `/delsticker` | — | yes (sticker in own pack) | `DeleteStickerFromSet` |
|
||||
| `/editsticker` | `<emoji...>` | yes (sticker in own pack) | `SetStickerEmojiList` |
|
||||
| `/ordersticker` | `<position>` | yes (sticker in own pack) | `SetStickerPositionInSet` |
|
||||
| `/setpackicon` | — | yes (sticker in own pack) | `GetFile`, `SetStickerSetThumbnail` |
|
||||
| `/renamepack` | `<pack> <title...>` | no | `SetStickerSetTitle` |
|
||||
| `/delpack` | `<pack>` | 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_<bot_username>`
|
||||
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_<bot_username>` (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/<slug>_by_<bot>` 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_<bot>` 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 `<pack>` 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
|
||||
|
||||
<!-- slug: sticker-pack-module -->
|
||||
Reference in New Issue
Block a user