mirror of
https://github.com/tiennm99/miti99bot.git
synced 2026-09-03 12:22:02 +00:00
docs(plans): revise sticker packs plan to one pack per user
Every command except /newpack drops its <pack> argument; the caller's single pack is resolved implicitly. /packlist becomes /mypack, and the store key is the user ID alone. Resolves rather than mitigates the worst red-team finding: /packlist's N+1 plus ten GetStickerSet calls under a 60s per-call ceiling is gone. /mypack is one Get and makes no API calls. resolveOwned collapses to a single Get. Both prior open questions are answered. The slug survives on /newpack alone, where it fixes the permanent share URL. Derived-from-user-id and opaque-id schemes were rejected: the first publishes the owner's Telegram ID forever, the second is unbrandable. /delpack reframed as the only way to change a pack URL, since Telegram exposes no rename-short-name method: - /renamepack's reply names the delete-and-recreate route instead of only stating the link cannot change - /delpack's confirm must state the title, the sticker count being destroyed, the link being surrendered, and that both are permanent - /repack migration rejected for this plan: up to ~121 sequential API calls exceeds handlerTimeout and stalls the bot for all users under C1. Viable only after the Phase 5 offload; recorded as a follow-up Fixes a bug in the write-ahead intent machinery: the different-slug pending branch overwrote unconditionally, permanently orphaning a set created before an interruption. It now probes GetStickerSet first and adopts when the old set exists. Adds R11 — whether a deleted slug can be reclaimed is undocumented and unresolvable without a live bot. Does not block the URL-change path, which needs a different name. Settled by a new Phase 6 smoke step. Phase 1 unchanged.
This commit is contained in:
@@ -3,7 +3,7 @@ phase: 2
|
||||
title: "Phase 2: Store, set names, emoji parsing"
|
||||
status: todo
|
||||
priority: P1
|
||||
effort: "4h"
|
||||
effort: "3h"
|
||||
dependencies: [1]
|
||||
---
|
||||
|
||||
@@ -12,15 +12,18 @@ dependencies: [1]
|
||||
## 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.
|
||||
user-chosen slug and a Telegram set name, sender validation, and emoji-argument parsing.
|
||||
Every function here is unit-testable without a bot.
|
||||
|
||||
One pack per user means the store layer is a single keyed record, not a collection scan.
|
||||
|
||||
## 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: persist at most one pack record per user, keyed so the caller's own user ID
|
||||
is the whole key — making the lookup itself the ownership check.
|
||||
- Functional: construct a Telegram set name `<slug>_by_<bot_username>` at creation, and match
|
||||
a sticker's `set_name` against the stored pack **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.
|
||||
@@ -30,32 +33,30 @@ Every function here is unit-testable without a bot.
|
||||
### Pack record
|
||||
|
||||
```go
|
||||
// Pack is one bot-created sticker set owned by a Telegram user.
|
||||
// Pack is the single 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
|
||||
Slug string `bson:"slug"` // chosen at creation, fixes the permanent URL
|
||||
Name string `bson:"name"` // Telegram set name, "<slug>_by_<botname>"
|
||||
Title string `bson:"title"` // display title, mutable
|
||||
OwnerID int64 `bson:"ownerId"` // Telegram user the set belongs to
|
||||
Count int `bson:"count"` // stickers in the set; keeps /packlist API-free
|
||||
Count int `bson:"count"` // stickers in the set; keeps /mypack 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.
|
||||
- **Key: `strconv.FormatInt(ownerID, 10)`** — the user ID alone. One pack per user makes the
|
||||
slug unnecessary as a key component, which removes the prefix scan entirely.
|
||||
- `getPack(ctx, ownerID) (Pack, bool, error)` — one `Get`; `storage.ErrNotFound` maps to
|
||||
`false, nil`. There is no `listPacks` and no `List` call anywhere in the module.
|
||||
|
||||
`Pending` implements write-ahead intent (plan C5). Phase 3 owns the state machine.
|
||||
This retires the worst red-team finding. The previous `listPacks` was a structural N+1
|
||||
(`mongo_doc_store.go:157-165` projects `_id` only, forcing a `Get` per key), and
|
||||
`/packlist` layered ten `GetStickerSet` calls on top of it under a 60s-per-call ceiling.
|
||||
|
||||
- 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.
|
||||
- `Count` keeps `/mypack` free of API calls. It is **advisory** — a user editing the pack
|
||||
through @Stickers desyncs it. Phase 3 refreshes it opportunistically.
|
||||
- `Pending` implements write-ahead intent (plan C5). Phase 3 owns the state machine.
|
||||
- `Pack`'s bson tags must not collide with `_id` / `version` / `updatedAt`; `storage.Typed`
|
||||
panics on collision (`internal/storage/doc_store.go:72`).
|
||||
|
||||
@@ -72,39 +73,39 @@ Rejects, in order: nil `msg`/`From`, zero ID, `From.IsBot`, and non-nil `msg.Sen
|
||||
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.
|
||||
share one pack — and under one-pack-per-user that is worse than it was before, because the
|
||||
first anonymous admin to run `/newpack` would block every other one and own the result.
|
||||
`rg "IsBot" internal/` returns zero hits today; `coin`, `gold`, and `stock` 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.
|
||||
The refusal must 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
|
||||
forbids consecutive underscores) and a trailing `_`. The cap is for link readability and to
|
||||
stay inside the 64-char set-name budget.
|
||||
- `makeSetName(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.
|
||||
- **`ownsSet(pack Pack, setName string) bool`** — case-insensitive comparison of `setName`
|
||||
against the stored `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).
|
||||
It deliberately replaces a `parseSlug(setName, botUsername)` design that re-derived the slug
|
||||
from the *live* username and discarded the persisted `Pack.Name`. Renaming the bot in
|
||||
BotFather — supported, and it leaves existing set names untouched — would have made every
|
||||
user's own pack refuse as "not yours" while `/mypack` still displayed it. Comparing the
|
||||
stored name also removes a case-sensitivity trap, since Telegram returns `SetName` with
|
||||
whatever casing the set was created with (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
|
||||
until the module asks. It is used **only** by `/newpack` to name a new set — 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`).
|
||||
nil-safe (`internal/modules/module.go:88`) and is nil under `BuildOptions{}`
|
||||
(`cmd/server/command_menu_test.go:55`).
|
||||
|
||||
### Emoji parsing
|
||||
|
||||
@@ -117,52 +118,54 @@ anonymous posting for this message"), not just deny.
|
||||
- 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.
|
||||
server's own message is the literal `too many emoji specified`). `defaultEmoji = "⭐"`.
|
||||
|
||||
Note `models.Sticker.Emoji` is a **single string** (`models/sticker.go:23`), so emoji
|
||||
inherited from a replied sticker yields at most one element.
|
||||
Because `/addsticker` now takes only `[emoji...]`, every one of its arguments is an emoji —
|
||||
there is no first-token disambiguation to perform, and a stray word fails loudly here rather
|
||||
than being mistaken for a pack name.
|
||||
|
||||
`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`
|
||||
- Create: matching `_test.go` files for each
|
||||
- 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)
|
||||
- Reference: `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`.
|
||||
1. `pack.go` — record, `packKey`, `getPack`.
|
||||
2. `sender.go` — `senderID` with the bot/anonymous refusals.
|
||||
3. `setname.go` — slug validation, `buildSetName`, `matchPack`, cached resolver interface.
|
||||
3. `setname.go` — slug validation, `makeSetName`, `ownsSet`, 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
|
||||
- [ ] `packKey(ownerID)` and `getPack` returning a found flag
|
||||
- [ ] `senderID` rejecting nil/zero/`IsBot`/`SenderChat` with an explanatory message
|
||||
- [ ] `slugRe` validation incl. `__`, trailing `_`, 40-char cap
|
||||
- [ ] `buildSetName` with 64-char guard and budget-reporting error
|
||||
- [ ] `matchPack` case-insensitive match against stored `Pack.Name`
|
||||
- [ ] `makeSetName` with 64-char guard and budget-reporting error
|
||||
- [ ] `ownsSet` case-insensitive match against stored `Pack.Name`
|
||||
- [ ] `usernameResolver` caching success but never failure, taking the handler's `b`
|
||||
- [ ] `parseEmoji` cluster scanner with the 20-entry cap
|
||||
- [ ] Four test files per the success criteria
|
||||
|
||||
## Success Criteria
|
||||
|
||||
- [ ] `listPacks` for owner A never returns owner B's packs
|
||||
- [ ] Quota boundary tested at 9, 10, 11 packs
|
||||
- [ ] `getPack` for owner A never returns owner B's pack
|
||||
- [ ] `getPack` on an unknown owner returns `found == false` and a nil error
|
||||
- [ ] No `List` call exists anywhere in the module
|
||||
- [ ] Slug table rejects leading digit, `__`, trailing `_`, 2 chars, 41 chars
|
||||
- [ ] `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
|
||||
- [ ] `makeSetName` errors when `len(slug)+len("_by_"+username) > 64`
|
||||
- [ ] `ownsSet` matches `MyPack_by_Bot` against a stored `mypack_by_bot`
|
||||
- [ ] `ownsSet` returns false for a set name belonging to another bot
|
||||
- [ ] A simulated bot username change does **not** break `ownsSet` for an existing pack
|
||||
- [ ] `senderID` rejects `IsBot: true` and a non-nil `SenderChat`, each with zero store access
|
||||
- [ ] `parseEmoji` handles joined input, ZWJ family, flag, keycap, skin tone; rejects plain text; errors above 20
|
||||
- [ ] `gofmt -l internal/modules/sticker` empty; `go test`/`go vet` clean
|
||||
@@ -174,12 +177,12 @@ dependency for this is disproportionate. Signal: a user reports an emoji split o
|
||||
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.
|
||||
**`Count` can drift.** Editing the pack through @Stickers changes the real count without the
|
||||
bot seeing it. Accepted: the field is advisory and feeds one display column. 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.
|
||||
|
||||
**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.
|
||||
**Keying on the user ID alone bakes in the one-pack limit.** Reversing to multiple packs later
|
||||
means a key migration, not just new commands — every existing record would need rewriting
|
||||
under a compound key. That is the real cost of plan R10, and it is why the limit belongs in
|
||||
the plan rather than living as a constant someone can bump.
|
||||
|
||||
@@ -3,7 +3,7 @@ phase: 3
|
||||
title: "Phase 3: Pack lifecycle commands"
|
||||
status: todo
|
||||
priority: P1
|
||||
effort: "8h"
|
||||
effort: "7h"
|
||||
dependencies: [1, 2]
|
||||
---
|
||||
|
||||
@@ -11,148 +11,178 @@ dependencies: [1, 2]
|
||||
|
||||
## 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.
|
||||
`/newpack`, `/mypack`, `/renamepack`, `/delpack` (+ confirm callback). These own creation and
|
||||
destruction of the user's single pack, 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: create the caller's pack and persist its record such that no interruption can
|
||||
permanently strand the slug.
|
||||
- Functional: show, rename, and delete that pack, never another user's.
|
||||
- Functional: a second `/newpack` while a pack exists is refused with a clear next step.
|
||||
- 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.
|
||||
- Functional: `/mypack` 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`.
|
||||
Mirrors `internal/modules/coin/coin.go`. `state` holds `store`, `pending` (a second typed view
|
||||
for delete confirmations — the pattern is idiomatic here; `loldle` and `lol` each build three
|
||||
views over one collection), `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` ships `/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.
|
||||
`<pack>` appears here and nowhere else in the module. It fixes the permanent share URL, and
|
||||
Telegram has no rename-short-name method, so it cannot be corrected later.
|
||||
|
||||
An earlier draft did "create on Telegram, then write the store", accepting that an interruption
|
||||
stranded 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 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
|
||||
3. `makeSetName(slug, username)`.
|
||||
4. **`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".
|
||||
`mongo_doc_store.go:87-105`). This *is* the one-pack quota — no separate counter exists.
|
||||
On `ErrConflict`, read the record:
|
||||
- confirmed → "you already have a pack (`<slug>`). Use /delpack first." Stop.
|
||||
- `Pending` with the **same** slug → this is our own interrupted attempt; resume at step 5.
|
||||
- `Pending` with a **different** slug → an earlier attempt was interrupted. **Probe
|
||||
`GetStickerSet(oldName)` before doing anything.**
|
||||
- The old set **exists** → the earlier attempt got as far as creating it. Adopt the old
|
||||
set, commit it, and tell the user they already have a pack (`<oldSlug>`) and must
|
||||
`/delpack` first if they want the new name. Do **not** overwrite.
|
||||
- The old set is **missing** (`isStickerSetMissing`) → nothing was created; overwrite the
|
||||
pending record with the new slug and continue.
|
||||
- **Any other error** → unknown; abort without touching the record.
|
||||
|
||||
Overwriting unconditionally would orphan a created-but-uncommitted set permanently: the
|
||||
set exists and is owned by the user, but adoption keys on the pending slug matching, so
|
||||
`/newpack <oldSlug>` would afterwards report "taken" with no route back. The probe is what
|
||||
makes the different-slug branch safe.
|
||||
|
||||
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.
|
||||
5. `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 7.
|
||||
- **`isStickerSetMissing`** → the slug is free; proceed to step 6.
|
||||
- **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}}}`.
|
||||
6. `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, …})`.
|
||||
On `PACK_SHORT_NAME_OCCUPIED`, another user of this bot holds the slug: delete the pending
|
||||
record and ask for a different one.
|
||||
7. 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.
|
||||
Re-running `/newpack` with the same slug after any interruption completes the operation instead
|
||||
of reporting it taken. That is the plan's "interrupted `/newpack` can be completed by re-running"
|
||||
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.
|
||||
### `/mypack` — zero API calls
|
||||
|
||||
### `/packlist` — zero API calls
|
||||
`getPack(senderID)`. One `Get`. Renders slug, title, `Count`, and the share link, or a short
|
||||
"you don't have a pack yet — `/newpack <name> <title>`" when absent. A `Pending` record renders
|
||||
with an "(incomplete — re-run /newpack)" marker rather than being hidden, so a stranded attempt
|
||||
is visible and fixable.
|
||||
|
||||
`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.
|
||||
This replaces the multi-pack `/packlist`, which 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 was a ~10-minute bot-wide
|
||||
freeze from one argument-free public command. That failure mode no longer exists.
|
||||
|
||||
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 <title...>`
|
||||
|
||||
### `/renamepack <pack> <title...>`
|
||||
`getPack(senderID)`; absent → "you don't have a pack yet". `SetStickerSetTitle{Name, Title}`,
|
||||
then commit the new `Title` under `WithoutCancel`.
|
||||
|
||||
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.
|
||||
The reply must state the share link is unchanged **and name the route to a different one**:
|
||||
`/delpack` then `/newpack <new-slug>`. This matters more now that the command takes no slug — a
|
||||
user typing "rename" with only a title is even likelier to expect the URL to follow, and it
|
||||
never can. Pointing at the real path turns a dead end into an answer.
|
||||
|
||||
**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.
|
||||
Required elements of the reply: the new title, the unchanged link, and the delete-and-recreate
|
||||
route with its cost stated (the stickers do not come along).
|
||||
|
||||
### `/delpack <pack>` — bound confirmation
|
||||
**Reverse gap:** if the API succeeds and the commit fails, `/mypack` shows a title Telegram no
|
||||
longer has. Cosmetic, self-heals on the next successful rename. Documented, not mitigated.
|
||||
|
||||
The earlier design put the slug in the callback data and re-checked ownership from
|
||||
### `/delpack` — no argument, bound confirmation
|
||||
|
||||
An earlier draft 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.
|
||||
else: the payload never expired, was not bound to a chat or message, and lived in scrollback
|
||||
forever. Three reviewers flagged it independently, and the `stock` module the draft cited as its
|
||||
model already solves it properly.
|
||||
|
||||
Follow `stock` fully, not half of it:
|
||||
With one pack per user the payload needs no slug at all — but it still needs everything else:
|
||||
|
||||
1. `/delpack` validates ownership, then writes a pending action:
|
||||
1. `/delpack` resolves the caller's pack, 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.
|
||||
`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.)
|
||||
|
||||
The confirm prompt must state all four consequences before the tap, since the command itself
|
||||
names nothing:
|
||||
|
||||
- the pack title being deleted;
|
||||
- **the sticker count that will be lost** (`Pack.Count`);
|
||||
- the exact share link that will stop working;
|
||||
- that both are permanent.
|
||||
|
||||
`/delpack` is the sanctioned way to change a pack URL, so this prompt is the last point at
|
||||
which a user learns the stickers do not survive the change. Understating it here is how
|
||||
someone loses 47 stickers expecting a rename.
|
||||
2. Callback data is `sticker_pack:d:<opaque id>` — comfortably inside the 64-byte cap (C3).
|
||||
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);
|
||||
- resolves the pending action; absent → "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;
|
||||
(`models/message.go:17-21`), 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 an 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`).
|
||||
- `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.
|
||||
**Reverse gap:** if `DeleteStickerSet` succeeds and `store.Delete` fails, a phantom record
|
||||
survives — and under one-pack-per-user that is worse than before, because it blocks `/newpack`
|
||||
entirely rather than consuming one of ten slots. Mitigation: any command receiving
|
||||
`isStickerSetMissing` from the API deletes the record on the spot, so the phantom clears on
|
||||
first contact and `/newpack` works again.
|
||||
|
||||
### 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>`.
|
||||
`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) |
|
||||
| `STICKERSET_INVALID` | your 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 |
|
||||
@@ -171,7 +201,7 @@ rewritten into prose by the Bot API server; everything else arrives as `Bad Requ
|
||||
|
||||
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.
|
||||
3. `/mypack` 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.
|
||||
@@ -182,43 +212,52 @@ rewritten into prose by the Bot API server; everything else arrives as `Bad Requ
|
||||
- [ ] `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
|
||||
- [ ] `/mypack` reading `Count`, marking a pending record, zero API calls
|
||||
- [ ] `/newpack` steps 1-7 incl. `PutVersioned` intent, three `ErrConflict` branches, adoption
|
||||
- [ ] Different-slug pending branch probes `GetStickerSet(oldName)` before overwriting
|
||||
- [ ] `/renamepack` reply: new title, unchanged link, and the /delpack + /newpack route
|
||||
- [ ] `/delpack` confirm prompt: title, sticker count, link, permanence
|
||||
- [ ] `pending_delete.go` with TTL, chat/message binding, opaque id
|
||||
- [ ] `/delpack` emitting the bound confirm keyboard
|
||||
- [ ] `/delpack` naming the pack in its confirm prompt
|
||||
- [ ] `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
|
||||
- [ ] `/mypack` records **zero** entries in `RecordingBot.Sent()`
|
||||
- [ ] A second `/newpack` with a confirmed pack present is refused, names the existing slug, and makes zero API calls
|
||||
- [ ] Interrupted `/newpack` (pending record, same slug, set exists) completes on re-run and does not report the slug taken
|
||||
- [ ] Interrupted `/newpack` with a *different* slug where the old set **exists** adopts the old set and refuses the new slug, leaving nothing orphaned
|
||||
- [ ] Interrupted `/newpack` with a *different* slug where the old set is **missing** replaces the pending record and proceeds
|
||||
- [ ] `/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
|
||||
- [ ] `/newpack` uses `PutVersioned(…, 0, …)`; the create path never calls `Put` for a new record
|
||||
- [ ] `/renamepack` with no pack replies "you don't have a pack yet" and makes zero API calls
|
||||
- [ ] `/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
|
||||
- [ ] A command receiving `STICKERSET_INVALID` deletes the stale record, unblocking `/newpack`
|
||||
- [ ] Title of 65 chars rejected locally, before any API call
|
||||
- [ ] `/delpack` confirm text contains the pack title, the sticker count, and the share link
|
||||
- [ ] `/renamepack` reply names the `/delpack` + `/newpack` route
|
||||
|
||||
## 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.
|
||||
**The write-ahead state machine is the most intricate logic in the plan**, and one-pack-per-user
|
||||
adds a branch rather than removing one: `ErrConflict` now means three different things
|
||||
(confirmed pack, own pending same-slug, own pending different-slug). Its correctness rests on
|
||||
one property — a `Pending` record for an owner means *that owner* asked for *that 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 5 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.
|
||||
**A stranded pending record now blocks the user entirely.** With ten slots it cost one; with one
|
||||
pack it blocks `/newpack` until resolved. Mitigated by making it visible in `/mypack` with a
|
||||
re-run hint, and by the different-slug overwrite branch in step 4 so a user is never wedged by
|
||||
a name they no longer want.
|
||||
|
||||
**`/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.
|
||||
|
||||
@@ -3,7 +3,7 @@ phase: 4
|
||||
title: "Phase 4: Sticker commands (reply path)"
|
||||
status: todo
|
||||
priority: P1
|
||||
effort: "5h"
|
||||
effort: "4h"
|
||||
dependencies: [1, 2, 3]
|
||||
---
|
||||
|
||||
@@ -11,16 +11,18 @@ dependencies: [1, 2, 3]
|
||||
|
||||
## Overview
|
||||
|
||||
Per-sticker operations on packs the caller owns: `/addsticker` (existing-sticker source),
|
||||
Per-sticker operations on the caller's pack: `/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.
|
||||
|
||||
None of these takes a pack argument. Under one-pack-per-user there is nothing to name.
|
||||
|
||||
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.
|
||||
- Functional: add an existing static sticker to the caller's pack.
|
||||
- Functional: remove, re-emoji, and reposition a sticker already in that pack.
|
||||
- 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.
|
||||
@@ -38,7 +40,7 @@ type stickerSource struct {
|
||||
}
|
||||
func (s *state) resolveSource(msg *models.Message) (stickerSource, error)
|
||||
|
||||
// an EXISTING sticker in one of the caller's packs
|
||||
// an EXISTING sticker in the caller's pack
|
||||
type ownedSticker struct {
|
||||
fileID string
|
||||
pack Pack
|
||||
@@ -46,37 +48,41 @@ type ownedSticker struct {
|
||||
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`:
|
||||
`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.
|
||||
3. `getPack(ownerID)` — **one `Get`**. Absent → the caller has no pack.
|
||||
4. `ownsSet(pack, sticker.SetName)` (Phase 2) — case-insensitive against the **stored**
|
||||
`Pack.Name`. False → the sticker is not from the caller's pack.
|
||||
5. **Steps 3 and 4 must produce byte-identical reply text.** Distinct messages would let a user
|
||||
probe whether a given set belongs to someone else. Pack *management* refusals stay uniform
|
||||
even though `/newpack` deliberately discloses slug occupancy (plan's Accepted disclosure
|
||||
section) — those 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.
|
||||
This replaced a `listPacks` + match design; with one pack it collapses to a single `Get` and a
|
||||
string comparison. It still compares against the stored `Pack.Name` rather than a slug
|
||||
re-derived from the live bot username — Phase 2 explains why (plan R8): a BotFather rename would
|
||||
otherwise make the user's own pack refuse as "not yours" while `/mypack` still displayed it.
|
||||
|
||||
`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...]`
|
||||
### `/addsticker [emoji...]`
|
||||
|
||||
Reply required. Slug from args, ownership from `store.Get`, source from `resolveSource`.
|
||||
Emoji precedence: explicit args → the replied sticker's emoji → `defaultEmoji`.
|
||||
Reply required. Pack from `getPack`, source from `resolveSource`. Emoji precedence: explicit
|
||||
args → the replied sticker's emoji → `defaultEmoji`.
|
||||
|
||||
Every argument is an emoji — there is no pack token to disambiguate, so a stray word is caught
|
||||
by `parseEmoji` and reported as a usage error rather than silently read as a pack name.
|
||||
|
||||
`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)".
|
||||
`WithoutCancel`. `STICKERS_TOO_MUCH` maps to "your pack is full (120 stickers)".
|
||||
|
||||
### `/delsticker`
|
||||
|
||||
@@ -84,32 +90,30 @@ No arguments. `resolveOwned`, then `DeleteStickerFromSet{Sticker: fileID}`. On s
|
||||
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.
|
||||
docs or the open-source Bot API server, so the plan depends on neither 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.
|
||||
An 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 pack. With one pack per user that is strictly
|
||||
worse than it was: the user loses their pack *and* is blocked from `/newpack` until the phantom
|
||||
clears.
|
||||
|
||||
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.
|
||||
command returns `STICKERSET_INVALID`, and the shared handler for that (Phase 3) deletes the
|
||||
record then — a positive signal, per plan rule 4. Simpler and strictly safer.
|
||||
|
||||
### `/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`.
|
||||
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}`.
|
||||
|
||||
@@ -127,13 +131,13 @@ set size and a local copy would go stale. Its error goes through `replyAPIError`
|
||||
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>`.
|
||||
`[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
|
||||
- [ ] `resolveOwned` using `getPack` + `ownsSet`, with the 6-step gate and uniform refusal
|
||||
- [ ] `/addsticker` with emoji precedence and `Count` increment
|
||||
- [ ] `/delsticker` with `Count` decrement and **no** probe
|
||||
- [ ] `/editsticker` requiring at least one emoji
|
||||
@@ -145,8 +149,9 @@ set size and a local copy would go stale. Its error goes through `replyAPIError`
|
||||
|
||||
- [ ] 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
|
||||
- [ ] "No pack yet" and "sticker from another bot's set" produce **byte-identical** reply text, asserted by comparing the two replies to each other
|
||||
- [ ] A sticker whose `SetName` differs only in case from the stored `Pack.Name` resolves successfully
|
||||
- [ ] `/addsticker` with a non-emoji argument is rejected by `parseEmoji`, not silently reinterpreted
|
||||
- [ ] `/ordersticker -1` rejected locally; `/ordersticker 999` reaches the API
|
||||
- [ ] `/editsticker` with no emoji rejected locally
|
||||
- [ ] `/delsticker` makes exactly one API call and never deletes the `Pack` record
|
||||
@@ -155,17 +160,15 @@ set size and a local copy would go stale. Its error goes through `replyAPIError`
|
||||
|
||||
## 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.
|
||||
**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.
|
||||
**`resolveOwned` now costs a single `Get`** — no `List`, no fan-out, no API call. This is the
|
||||
one place the one-pack revision made a correctness-critical path cheaper as well as simpler,
|
||||
and it removes the caching follow-up the multi-pack version needed.
|
||||
|
||||
**`Count` drift is user-visible but harmless.** Editing a pack through @Stickers desyncs it.
|
||||
**`Count` drift is user-visible but harmless.** Editing the 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.
|
||||
|
||||
@@ -127,7 +127,7 @@ matters. Do not restructure into upload-now-use-later.
|
||||
|
||||
### `/setpackicon`
|
||||
|
||||
No arguments; reply to a sticker in one of the caller's packs.
|
||||
No arguments; reply to a sticker in the caller's pack.
|
||||
|
||||
1. `resolveOwned` (Phase 4).
|
||||
2. `GetFile` + download that sticker's image, then `toThumbnailPNG`.
|
||||
|
||||
@@ -11,9 +11,9 @@ dependencies: [1, 2, 3, 4, 5]
|
||||
|
||||
## 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.
|
||||
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
|
||||
|
||||
@@ -31,7 +31,7 @@ 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
|
||||
write-capable module would go live on the next deploy with no operator decision. An earlier
|
||||
draft claimed the opposite in three places — goal, requirement, and a success criterion a
|
||||
reviewer would have ticked without testing.
|
||||
|
||||
@@ -44,36 +44,48 @@ Ordered fix, per the user's decision:
|
||||
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.
|
||||
it, that rollback means 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.
|
||||
not reintroduce the empty-means-everything trap.
|
||||
|
||||
### 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.
|
||||
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.
|
||||
(`registry.go:173`). All nine were verified free against the current registry; re-run registry
|
||||
tests to catch later additions. Note `mypack` replaced `packlist` in the one-pack revision —
|
||||
re-verify that name specifically, since it was not part of the original conflict check.
|
||||
|
||||
### The `/help` rune budget — the real Phase 6 blocker
|
||||
### The `/help` rune budget
|
||||
|
||||
`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.
|
||||
The one-pack revision helps here. Each help line is
|
||||
`InvocationSentence() + " " + SummarySentence()` (`internal/modules/command_presentation.go:16-24`),
|
||||
so `Parameters` counts against the budget — and four commands lost their `<pack>` token:
|
||||
|
||||
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.
|
||||
| Command | Was | Now |
|
||||
|---|---|---|
|
||||
| `/addsticker` | `<pack> [emoji...]` | `[emoji...]` |
|
||||
| `/renamepack` | `<pack> <title...>` | `<title...>` |
|
||||
| `/delpack` | `<pack>` | — |
|
||||
| `/packlist` → `/mypack` | — | — |
|
||||
|
||||
That is roughly 25 runes recovered across the module, leaving about **98 runes per command
|
||||
line** including the module header rather than ~85. Still not generous: `/ordersticker <position>.`
|
||||
alone is 24 runes, leaving ~74 for its description.
|
||||
|
||||
Write the nine descriptions against that budget *before* wiring, then re-measure — the figure
|
||||
above is derived, not measured post-change. If they do not fit, decide then whether `/help`
|
||||
needs pagination; that is a separate change, and it must not be "solved" by trimming other
|
||||
modules' descriptions.
|
||||
|
||||
### Test impact — which tests, and which only look related
|
||||
|
||||
@@ -84,28 +96,35 @@ Phase 6 afterthought, and it must not be "solved" by trimming other modules' des
|
||||
| `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 |
|
||||
|
||||
`expectedParameters` entries for the nine: `newpack` → `<pack> <title...>`, `addsticker` →
|
||||
`[emoji...]`, `delsticker` → ``, `editsticker` → `<emoji...>`, `ordersticker` → `<position>`,
|
||||
`setpackicon` → ``, `renamepack` → `<title...>`, `delpack` → ``, `mypack` → ``.
|
||||
|
||||
### 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.
|
||||
Module table row listing the nine commands, then a `### Sticker packs` section: **one pack per
|
||||
user**; the pack is created on behalf of the calling user; the bot manages only packs it
|
||||
created; the slug is chosen once at `/newpack` and fixes a permanent share link that
|
||||
`/renamepack` cannot change; static stickers and photos only; the 120-stickers-per-pack cap;
|
||||
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.
|
||||
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.
|
||||
usage text exactly (`docs/command-parameter-conventions.md` § Change Checklist); slug rules and
|
||||
the permanence of the resulting URL; 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; multiple packs per user — see plan R10;
|
||||
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`).
|
||||
No command is renamed or deleted **in the shipped bot** — `/packlist` never existed outside this
|
||||
plan, so its replacement by `/mypack` needs no migration. `AGENTS.md` § "Stats Compatibility"
|
||||
governs renames of live commands and does not apply. New commands accrue stats through the
|
||||
dispatcher hook (`dispatcher.go:80-84`).
|
||||
|
||||
## Related Code Files
|
||||
|
||||
@@ -118,7 +137,7 @@ governs renames and deletions. New commands accrue stats through the dispatcher
|
||||
## 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.
|
||||
2. Draft the nine descriptions against the ~98-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`.
|
||||
@@ -128,7 +147,8 @@ governs renames and deletions. New commands accrue stats through the dispatcher
|
||||
|
||||
- [ ] 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
|
||||
- [ ] Confirm `mypack` is free in the registry alongside the other eight
|
||||
- [ ] Draft nine descriptions within the measured budget and re-measure `RenderHelp`
|
||||
- [ ] Add `"sticker": sticker.New` to `factories()`
|
||||
- [ ] Add nine entries to `expectedParameters`
|
||||
- [ ] README module-table row + `### Sticker packs` section
|
||||
@@ -143,26 +163,31 @@ governs renames and deletions. New commands accrue stats through the dispatcher
|
||||
- [ ] 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: a second `/newpack` is refused and names the existing pack
|
||||
- [ ] Smoke: reply to a photo with `/addsticker 😂` — no pack named — and the sticker renders undistorted
|
||||
- [ ] Smoke: `/mypack`, `/renamepack New Title`, `/setpackicon`, `/ordersticker 0` all succeed
|
||||
- [ ] Smoke: `/renamepack` leaves the share link working and unchanged
|
||||
- [ ] Smoke: a **second** account replying to the first account's sticker with `/delsticker` is refused
|
||||
- [ ] 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: `/delpack` confirm prompt shows the title, sticker count, and link before confirming
|
||||
- [ ] Smoke: `/delpack` → confirm → link 404s; a second press reports already-used; `/newpack` then works again
|
||||
- [ ] Smoke: **after deleting, attempt `/newpack` with the same slug** — this settles plan R11. Record the outcome in `docs/sticker-packs.md` either way, and if the slug is reserved, add that to `/delpack`'s confirm text
|
||||
- [ ] Smoke: `/renamepack` reply names the delete-and-recreate route
|
||||
- [ ] 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.
|
||||
**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.
|
||||
**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.
|
||||
**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
|
||||
`/mypack` before anyone enabled it. Response: set `MODULES` immediately; the module is otherwise
|
||||
harmless until someone runs a command.
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
---
|
||||
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"
|
||||
description: "internal/modules/sticker — public, one-pack-per-user Telegram sticker set management via single-shot reply commands using @Stickers command names"
|
||||
status: pending
|
||||
priority: P2
|
||||
effort: ""
|
||||
@@ -15,10 +15,13 @@ blocks: []
|
||||
|
||||
## 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.
|
||||
New module `internal/modules/sticker` letting **any** user create and manage **one**
|
||||
personal Telegram sticker pack through the bot. The pack is created on behalf of the calling
|
||||
user (`user_id`), named `<slug>_by_<bot_username>`, and stays bot-manageable because the bot
|
||||
created it.
|
||||
|
||||
One pack per user is the central simplification: no command except `/newpack` takes a pack
|
||||
argument, because there is only ever one pack to act on.
|
||||
|
||||
Command names mirror @Stickers (`/newpack`, `/addsticker`, …) but each command is
|
||||
**single-shot**: one message carrying its arguments, optionally replying to a sticker or
|
||||
@@ -31,19 +34,20 @@ prerequisites, not incidental work.
|
||||
|
||||
| # | 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 |
|
||||
| 1 | Any user can create and fill their personal sticker pack without leaving the chat | P1 |
|
||||
| 2 | No command but `/newpack` requires naming a pack | P1 |
|
||||
| 3 | Ownership is enforced structurally — a user can never mutate another's pack | P1 |
|
||||
| 4 | No sticker command can stall the bot for other users beyond a bounded deadline | P1 |
|
||||
| 5 | A partial failure never permanently strands a user's pack | P1 |
|
||||
| 6 | Command names and semantics recognisable to @Stickers users | P2 |
|
||||
| 7 | 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 |
|
||||
| Pack model | **One pack per user.** The slug is chosen at creation and never used as an argument again |
|
||||
| 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 |
|
||||
@@ -55,23 +59,27 @@ All `VisibilityPublic`. `Parameters` follows `docs/command-parameter-conventions
|
||||
| Command | Parameters | Reply required | API calls |
|
||||
|---|---|---|---|
|
||||
| `/newpack` | `<pack> <title...>` | yes (sticker/photo) | `GetStickerSet`, `UploadStickerFile`*, `CreateNewStickerSet` |
|
||||
| `/addsticker` | `<pack> [emoji...]` | yes (sticker/photo) | `UploadStickerFile`*, `AddStickerToSet` |
|
||||
| `/addsticker` | `[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 |
|
||||
| `/renamepack` | `<title...>` | no | `SetStickerSetTitle` |
|
||||
| `/delpack` | — | no (inline confirm) | `DeleteStickerSet` |
|
||||
| `/mypack` | — | no | **none** — count comes from the store |
|
||||
|
||||
`*` only on the photo path.
|
||||
|
||||
### Deviation from the approved command preview
|
||||
### Slug is a name, not an address
|
||||
|
||||
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.
|
||||
`<pack>` survives on `/newpack` alone, where it fixes the permanent share URL
|
||||
`t.me/addstickers/<slug>_by_<bot>`. Telegram has no rename-short-name method, so that choice
|
||||
is unfixable afterwards — which is exactly why it stays user-chosen rather than derived from
|
||||
a user ID (which would publish the owner's numeric Telegram ID forever) or generated opaquely.
|
||||
|
||||
Every other command resolves the caller's single pack from the store, or from the replied
|
||||
sticker's `set_name` matched against it. This supersedes the earlier multi-pack design in
|
||||
which `<pack>` was an argument to `/addsticker`, `/renamepack`, and `/delpack`.
|
||||
|
||||
### Dropped from @Stickers
|
||||
|
||||
@@ -227,9 +235,11 @@ repo's first *direct* `golang.org/x/*` requirement; all current ones are indirec
|
||||
|
||||
Public module creating durable Telegram-side objects on a single-threaded dispatcher (C1):
|
||||
|
||||
- `maxPacksPerUser = 10`, enforced before `CreateNewStickerSet`.
|
||||
- One pack per user, enforced by a create-only write before `CreateNewStickerSet`. This is
|
||||
the quota; there is no separate counter to keep.
|
||||
- `handlerTimeout = 10s` on every handler — the primary bound (C2).
|
||||
- `/packlist` makes **zero** API calls; counts live on the `Pack` record.
|
||||
- `/mypack` makes **zero** API calls; the count lives on the `Pack` record. A single `Get`,
|
||||
no `List`, no per-pack fan-out.
|
||||
- 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,
|
||||
@@ -241,12 +251,14 @@ Public module creating durable Telegram-side objects on a single-threaded dispat
|
||||
| # | 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 |
|
||||
| R11 | A deleted slug may not be reclaimable | `/newpack <old-slug>` after `/delpack` reports the slug taken | Unknown at decision time; official docs are silent and community reports lean toward short names staying reserved. Does **not** block the URL-change path, which needs a *different* name. Settled empirically in Phase 6 smoke; if reserved, say so in `/delpack`'s confirm text |
|
||||
| R10 | A user wants two packs and cannot have one | Requests for a second pack | Accepted by design. Reversing it means restoring `<pack>` arguments across four commands — a deliberate, not incidental, change |
|
||||
| R2 | Partial failure strands a pack | User reports a slug reported taken that `/mypack` 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` |
|
||||
| R7 | A transient error deletes a live pack's record | `/mypack` reports no pack though the user can still open theirs via link | Rule 4 — destructive store deletes require positive `STICKERSET_INVALID` |
|
||||
| R8 | Bot username changes in BotFather | Every pack refuses as "not yours" after restart | Ownership matches stored `Pack.Name` against `Sticker.SetName`; username only builds *new* names (Phase 2) |
|
||||
| R9 | `golang.org/x/image` resampling disappoints | Visibly soft or aliased stickers in smoke | Swap `CatmullRom` for `ApproxBiLinear`; one line, one file |
|
||||
|
||||
@@ -262,11 +274,14 @@ 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
|
||||
- [ ] `/addsticker 😂` on a photo produces a valid static sticker with a 512px long edge and correct aspect ratio, with no pack named
|
||||
- [ ] 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
|
||||
- [ ] `/mypack` shows slug, title, count, and link for the caller's own pack, making no API calls
|
||||
- [ ] A second `/newpack` while a pack exists is refused, naming the existing pack and pointing at `/delpack`
|
||||
- [ ] `/delpack` takes no argument and requires inline confirmation bound to invoker, chat, message, and a TTL
|
||||
- [ ] `/delpack`'s confirm prompt states the title, sticker count, link, and permanence before the tap
|
||||
- [ ] `/renamepack`'s reply names the `/delpack` + `/newpack` route to a different URL
|
||||
- [ ] 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
|
||||
@@ -277,10 +292,79 @@ otherwise disclose which of the caller's own guesses correspond to real packs.
|
||||
|
||||
## 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?
|
||||
None. Both prior questions were resolved by the one-pack-per-user revision: `/editsticker`
|
||||
takes no pack argument because no command but `/newpack` does (former O1), and the
|
||||
per-user pack limit is one (former O2).
|
||||
|
||||
## Design Revisions
|
||||
|
||||
### 2026-08-25 — one pack per user
|
||||
|
||||
The accepted scope originally chose "named packs, multiple per user, addressed by slug
|
||||
argument". The user revised it to one pack per user, with every command operating on that
|
||||
pack implicitly.
|
||||
|
||||
Removed by the revision:
|
||||
|
||||
- `<pack>` arguments on `/addsticker`, `/renamepack`, and `/delpack`
|
||||
- `/packlist` (replaced by `/mypack`, singular) and its `List` + per-pack `Get` fan-out
|
||||
- `maxPacksPerUser` as a tunable — the limit is one
|
||||
- The separate default-pack command and per-user prefs record that a multi-pack default
|
||||
would have required
|
||||
|
||||
Red-team findings this revision resolves outright rather than mitigates:
|
||||
|
||||
- Finding 4 (`/packlist` unbounded: 60s client timeout x 10 calls plus an N+1) — `/mypack`
|
||||
is a single `Get` and makes no API calls at all
|
||||
- Former open question O2 (`maxPacksPerUser` sizing) — no longer a choice
|
||||
|
||||
Unaffected: Phase 1 (panic barrier, test harness) and Phase 5 (photo pipeline, token-leak
|
||||
sentinel) need no changes. The write-ahead intent machinery, anonymous-sender rule,
|
||||
`handlerTimeout`, and bot-rename resilience all carry over unchanged.
|
||||
|
||||
Trade-off accepted: a user who wants a memes pack and a reactions pack separately cannot
|
||||
have both (R10).
|
||||
|
||||
### 2026-08-25 — `/delpack` as the URL-change path
|
||||
|
||||
`/delpack` was reviewed as a destructive convenience. It is in fact the **only** mechanism for
|
||||
changing a pack's URL, because Telegram exposes no rename-short-name method. That reframing
|
||||
changed three things without adding a command:
|
||||
|
||||
- `/renamepack`'s reply now names the delete-and-recreate route instead of only stating that
|
||||
the link cannot change — turning a dead end into an answer.
|
||||
- `/delpack`'s confirm prompt must state the pack title, the sticker count being destroyed, the
|
||||
exact link being surrendered, and that both are permanent. It is the last point at which a
|
||||
user learns stickers do not survive a URL change.
|
||||
- A `/repack <newslug>` migration command was considered and **rejected for this plan**: copying
|
||||
a full pack is up to ~121 sequential API calls, which blows `handlerTimeout` and stalls the
|
||||
bot for every user under C1. It is viable only after Phase 5's goroutine offload lands, and is
|
||||
recorded here as a follow-up rather than scoped in.
|
||||
|
||||
The @Stickers command set was mapped exhaustively against the one-pack model; all nine of our
|
||||
commands are either direct adaptations or (for `/mypack`) a justified addition, and every
|
||||
dropped @Stickers command has a stated reason.
|
||||
|
||||
**Bug found during this pass** (Phase 3, `/newpack`): the different-slug pending branch
|
||||
overwrote the pending record unconditionally, which permanently orphans a set that was created
|
||||
before an interruption. It now probes `GetStickerSet(oldName)` first and adopts rather than
|
||||
overwrites when the old set exists.
|
||||
|
||||
|
||||
#### Consistency sweep — one-pack revision
|
||||
|
||||
- Files reread: plan.md and all six phase files.
|
||||
- Deltas checked: 9 (`<pack>` dropped from `/addsticker`, `/renamepack`, `/delpack`;
|
||||
`/packlist` -> `/mypack`; `listPacks` -> `getPack`; key `<ownerID>:<slug>` -> `<ownerID>`;
|
||||
`maxPacksPerUser` removed; `matchPack` -> `ownsSet`; `buildSetName` -> `makeSetName`).
|
||||
- Stale references reconciled: 4 (incl. the frontmatter description, which said
|
||||
"multi-pack-per-user" and is surfaced by every `ak plan list`). Two live risk signals in the R2/R7 rows still named
|
||||
`/packlist`; Phase 5's `/setpackicon` still said "one of the caller's packs". Phase 5 was
|
||||
therefore **not** untouched, contrary to the initial assessment.
|
||||
- Remaining `/packlist`, `listPacks`, and `maxPacksPerUser` mentions are all deliberate
|
||||
comparative or historical text ("this replaced X", the revision log, the red-team table).
|
||||
- Phases 1 and 5 confirmed free of multi-pack phrasing after the fix.
|
||||
- Unresolved contradictions: 0
|
||||
|
||||
## Red Team Review
|
||||
|
||||
|
||||
Reference in New Issue
Block a user