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