From e56f5c5d7d6eb31ed247e8b6a4da26eff373ebb5 Mon Sep 17 00:00:00 2001 From: tiennm99 Date: Tue, 25 Aug 2026 11:08:22 +0700 Subject: [PATCH] docs(plans): revise sticker packs plan to one pack per user MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every command except /newpack drops its 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. --- .../phase-02-store-setname-emoji.md | 147 ++++++----- .../phase-03-pack-lifecycle.md | 249 ++++++++++-------- .../phase-04-sticker-commands.md | 99 +++---- .../phase-05-photo-pipeline.md | 2 +- .../phase-06-wiring-docs.md | 127 +++++---- plans/260824-1051-sticker-pack-module/plan.md | 148 ++++++++--- 6 files changed, 463 insertions(+), 309 deletions(-) diff --git a/plans/260824-1051-sticker-pack-module/phase-02-store-setname-emoji.md b/plans/260824-1051-sticker-pack-module/phase-02-store-setname-emoji.md index 05b4ae1..16a0f60 100644 --- a/plans/260824-1051-sticker-pack-module/phase-02-store-setname-emoji.md +++ b/plans/260824-1051-sticker-pack-module/phase-02-store-setname-emoji.md @@ -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 `_by_`, 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 `_by_` 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, "_by_" + 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 (` 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 `_by_`. -After a rename, old packs keep working and new ones use the new suffix β€” correct behaviour, -but worth stating so nobody "fixes" it later by rewriting stored names. +**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. diff --git a/plans/260824-1051-sticker-pack-module/phase-03-pack-lifecycle.md b/plans/260824-1051-sticker-pack-module/phase-03-pack-lifecycle.md index 07be8e1..75ad607 100644 --- a/plans/260824-1051-sticker-pack-module/phase-03-pack-lifecycle.md +++ b/plans/260824-1051-sticker-pack-module/phase-03-pack-lifecycle.md @@ -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 ` β€” 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. +`` 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 (``). 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 (``) 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 ` 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/`. -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 `" 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. diff --git a/plans/260824-1051-sticker-pack-module/phase-04-sticker-commands.md b/plans/260824-1051-sticker-pack-module/phase-04-sticker-commands.md index 822c35d..1cb3f3e 100644 --- a/plans/260824-1051-sticker-pack-module/phase-04-sticker-commands.md +++ b/plans/260824-1051-sticker-pack-module/phase-04-sticker-commands.md @@ -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. diff --git a/plans/260824-1051-sticker-pack-module/phase-05-photo-pipeline.md b/plans/260824-1051-sticker-pack-module/phase-05-photo-pipeline.md index cd23008..b3ca356 100644 --- a/plans/260824-1051-sticker-pack-module/phase-05-photo-pipeline.md +++ b/plans/260824-1051-sticker-pack-module/phase-05-photo-pipeline.md @@ -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`. diff --git a/plans/260824-1051-sticker-pack-module/phase-06-wiring-docs.md b/plans/260824-1051-sticker-pack-module/phase-06-wiring-docs.md index 7b6640f..4ba2815 100644 --- a/plans/260824-1051-sticker-pack-module/phase-06-wiring-docs.md +++ b/plans/260824-1051-sticker-pack-module/phase-06-wiring-docs.md @@ -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. diff --git a/plans/260824-1051-sticker-pack-module/plan.md b/plans/260824-1051-sticker-pack-module/plan.md index 86d588f..ccef672 100644 --- a/plans/260824-1051-sticker-pack-module/plan.md +++ b/plans/260824-1051-sticker-pack-module/plan.md @@ -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