From c47bcf2c7fb0084b47aba04640f66f1bfdbddc47 Mon Sep 17 00:00:00 2001 From: tiennm99 Date: Tue, 15 Sep 2026 13:48:34 +0700 Subject: [PATCH] feat(blacklist): show the changed list after every add and remove /blacklist_add, /blacklist_del, /whitelist_add and /whitelist_del now answer with the current contents of the list they touched, so the sender sees the result without following up with /blacklist_rules. All four outcomes end the same way, including the two that change nothing: text already present, and text that was not there to remove. Those are exactly when someone wants to see what the list holds, and "every add and remove shows the list" is a simpler rule than one conditional on whether a write landed. The confirmation line shares the listing's byte budget, so a long list cannot push the combined reply past Telegram's message limit. --- docs/blacklist.md | 7 ++ internal/modules/blacklist/handlers.go | 37 +++++++++- internal/modules/blacklist/handlers_test.go | 73 ++++++++++++++++++- ...cklist-module-from-brainstorm-to-deploy.md | 68 +++++++++++++++++ 4 files changed, 180 insertions(+), 5 deletions(-) create mode 100644 plans/journals/2026-09-15-blacklist-module-from-brainstorm-to-deploy.md diff --git a/docs/blacklist.md b/docs/blacklist.md index ec2c486..4f21c2e 100644 --- a/docs/blacklist.md +++ b/docs/blacklist.md @@ -77,6 +77,13 @@ Matching is by substring, after the text is normalized: ## Listing +Every `/blacklist_add`, `/blacklist_del`, `/whitelist_add` and `/whitelist_del` answers with +the current contents of the list it touched, so you see the result without running anything +else. That includes the two outcomes that change nothing — text already present, or text that +was not there to remove — since those are exactly the moments you want to see what the list +actually holds. A whitelist command shows the whitelist; a blacklist command shows the +blacklist. + `/blacklist_rules` prints both lists in one message, each with its entry count, showing entries as they were typed rather than in the normalized form. Each entry is tappable to copy, ready to paste into a `_del` command. diff --git a/internal/modules/blacklist/handlers.go b/internal/modules/blacklist/handlers.go index 14fbfbf..a001c69 100644 --- a/internal/modules/blacklist/handlers.go +++ b/internal/modules/blacklist/handlers.go @@ -147,6 +147,33 @@ func (s *state) get(ctx context.Context, key string) (Entry, bool, error) { return entry, true, nil } +// replyWithList answers an add or a remove with its confirmation followed by +// the current contents of the list, so the sender sees the result of what they +// just did without running /blacklist_rules. +// +// Every outcome of those four commands ends here, including the two that change +// nothing: "already present" and "not there" are exactly when someone wants to +// see what the list actually holds, and "every add and remove shows the list" +// is a simpler rule to rely on than one conditional on whether a write landed. +// +// The confirmation goes into the builder first, so renderSection's byte budget +// already accounts for it and the combined message still fits one reply. +// +// A failed read here is logged but not reported: the change itself succeeded, +// and answering a completed write with a generic failure would be a lie. +func (s *state) replyWithList(ctx context.Context, b *bot.Bot, msg *models.Message, list, prefix, confirmation string) error { + var sb strings.Builder + sb.WriteString(confirmation) + + docs, err := s.store.Scan(ctx, prefix) + if err != nil { + log.Error("blacklist_mutation_scan", "list", list, "err", err) + return chathelper.ReplyHTML(ctx, b, msg, sb.String()) + } + renderSection(&sb, list, prefix, docs) + return chathelper.ReplyHTML(ctx, b, msg, sb.String()) +} + // handleAdd stores text in one of the thread's two lists. func (s *state) handleAdd(list string) modules.CommandHandler { command := listName(list) + "_add" @@ -165,6 +192,7 @@ func (s *state) handleAdd(list string) modules.CommandHandler { } chatID, threadID := threadOf(msg) + prefix := scopePrefix(chatID, threadID, list) key := entryKey(chatID, threadID, list, normText) // Read before writing purely to word the reply. The write is @@ -176,7 +204,7 @@ func (s *state) handleAdd(list string) modules.CommandHandler { return chathelper.Reply(ctx, b, msg, genericFailure) } if found { - return chathelper.ReplyHTML(ctx, b, msg, fmt.Sprintf( + return s.replyWithList(ctx, b, msg, list, prefix, fmt.Sprintf( "%s is already in this topic's %s.", html.EscapeString(existing.Text), listName(list))) } @@ -189,7 +217,7 @@ func (s *state) handleAdd(list string) modules.CommandHandler { log.Error("blacklist_add", "list", list, "err", err) return chathelper.Reply(ctx, b, msg, genericFailure) } - return chathelper.ReplyHTML(ctx, b, msg, fmt.Sprintf( + return s.replyWithList(ctx, b, msg, list, prefix, fmt.Sprintf( "Added %s to this topic's %s.", html.EscapeString(raw), listName(list))) } @@ -217,6 +245,7 @@ func (s *state) handleDel(list string) modules.CommandHandler { } chatID, threadID := threadOf(msg) + prefix := scopePrefix(chatID, threadID, list) key := entryKey(chatID, threadID, list, normText) // Read first so absent text is reported as such. Delete on a missing @@ -227,7 +256,7 @@ func (s *state) handleDel(list string) modules.CommandHandler { log.Error("blacklist_del_lookup", "list", list, "err", err) return chathelper.Reply(ctx, b, msg, genericFailure) } else if !found { - return chathelper.ReplyHTML(ctx, b, msg, fmt.Sprintf( + return s.replyWithList(ctx, b, msg, list, prefix, fmt.Sprintf( "%s is not in this topic's %s.", html.EscapeString(raw), listName(list))) } @@ -236,7 +265,7 @@ func (s *state) handleDel(list string) modules.CommandHandler { log.Error("blacklist_del", "list", list, "err", err) return chathelper.Reply(ctx, b, msg, genericFailure) } - return chathelper.ReplyHTML(ctx, b, msg, fmt.Sprintf( + return s.replyWithList(ctx, b, msg, list, prefix, fmt.Sprintf( "Removed %s from this topic's %s.", html.EscapeString(raw), listName(list))) } diff --git a/internal/modules/blacklist/handlers_test.go b/internal/modules/blacklist/handlers_test.go index 75cc858..373e8cd 100644 --- a/internal/modules/blacklist/handlers_test.go +++ b/internal/modules/blacklist/handlers_test.go @@ -2,6 +2,7 @@ package blacklist_test import ( "context" + "fmt" "strings" "testing" @@ -210,7 +211,7 @@ func TestRules_TrimsToOneMessage(t *testing.T) { const entries = 400 for i := range entries { send(t, rb, testutil.NewPrivateMessage(7, - "/blacklist_add "+strings.Repeat("x", 30)+string(rune('a'+i%26))+strings.Repeat("y", i%7))) + fmt.Sprintf("/blacklist_add %s%03d", strings.Repeat("x", 30), i))) } got := send(t, rb, testutil.NewPrivateMessage(7, "/blacklist_rules")) @@ -388,3 +389,73 @@ func TestReplyChainThreadIsNotATopic(t *testing.T) { t.Fatalf("rules reply = %q; want the entry listed", got) } } + +// A mutation answers with the current contents of the list it changed, so the +// sender never has to follow it with /blacklist_rules. +func TestAddAndDel_ShowTheChangedList(t *testing.T) { + rb := installBlacklist(t) + + first := send(t, rb, testutil.NewPrivateMessage(7, "/blacklist_add cat")) + if !strings.Contains(first, "Blacklist (1)") || !strings.Contains(first, "cat") { + t.Fatalf("add reply = %q; want the list appended", first) + } + + second := send(t, rb, testutil.NewPrivateMessage(7, "/blacklist_add dog")) + if !strings.Contains(second, "Blacklist (2)") { + t.Fatalf("add reply = %q; want both entries counted", second) + } + if !strings.Contains(second, "cat") || !strings.Contains(second, "dog") { + t.Fatalf("add reply = %q; want the whole list, not just the new entry", second) + } + + removed := send(t, rb, testutil.NewPrivateMessage(7, "/blacklist_del cat")) + if !strings.Contains(removed, "Blacklist (1)") { + t.Fatalf("del reply = %q; want the list after removal", removed) + } + // Only the listing may be checked for absence: the confirmation line above + // it echoes the removed text by design. + _, listing, _ := strings.Cut(removed, "Blacklist") + if strings.Contains(listing, "cat") { + t.Fatalf("del reply = %q; the removed entry is still listed", removed) + } + if !strings.Contains(listing, "dog") { + t.Fatalf("del reply = %q; want the surviving entry listed", removed) + } + + empty := send(t, rb, testutil.NewPrivateMessage(7, "/blacklist_del dog")) + if !strings.Contains(empty, "Blacklist (0)") || !strings.Contains(empty, "nothing yet") { + t.Fatalf("del reply = %q; want an empty list shown", empty) + } +} + +// Each list answers with itself: a whitelist change shows the whitelist, not +// the blacklist. +func TestWhitelistMutation_ShowsOnlyTheWhitelist(t *testing.T) { + rb := installBlacklist(t) + send(t, rb, testutil.NewPrivateMessage(7, "/blacklist_add cat")) + + got := send(t, rb, testutil.NewPrivateMessage(7, "/whitelist_add exception")) + if !strings.Contains(got, "Whitelist (1)") { + t.Fatalf("whitelist add reply = %q; want the whitelist", got) + } + if strings.Contains(got, "Blacklist") { + t.Fatalf("whitelist add reply = %q; want only the changed list", got) + } +} + +// The confirmation shares the list's byte budget, so a long list cannot push +// the combined reply past Telegram's limit. +func TestAdd_ReplyStaysWithinOneMessage(t *testing.T) { + rb := installBlacklist(t) + var last string + for i := range 400 { + last = send(t, rb, testutil.NewPrivateMessage(7, + fmt.Sprintf("/blacklist_add %s%03d", strings.Repeat("x", 30), i))) + } + if n := len([]rune(last)); n > 4096 { + t.Fatalf("add reply is %d characters, over Telegram's limit", n) + } + if !strings.Contains(last, "more.") { + t.Fatalf("add reply = %q; want a trim notice", last) + } +} diff --git a/plans/journals/2026-09-15-blacklist-module-from-brainstorm-to-deploy.md b/plans/journals/2026-09-15-blacklist-module-from-brainstorm-to-deploy.md new file mode 100644 index 0000000..2d98983 --- /dev/null +++ b/plans/journals/2026-09-15-blacklist-module-from-brainstorm-to-deploy.md @@ -0,0 +1,68 @@ +--- +title: "Blacklist module: from brainstorm to deploy" +date: 2026-09-15 +summary: "Shipped a passive per-topic blacklist/whitelist module; review caught a /help rune ceiling, four unpinned escaping sites, and a reply-chain scoping trap" +--- + +# Blacklist module: from brainstorm to deploy + +## What happened + +Delivered `internal/modules/blacklist` end to end in one session: brainstorm, plan +(`plans/260915-1108-blacklist-module/`), four phases, review, and push to `main` as `e4412d5`. + +Six public commands over two per-topic lists. The module is passive by explicit decision — +it never scans chat traffic, because enforcement would need a dispatcher message hook, +BotFather privacy mode off, and per-group admin delete rights. + +Four owner decisions shaped it, each settled before any code: passive registry over +auto-moderation; whitelist as an exception layer rather than an independent list; per-thread +and world-writable scope; and diacritic-sensitive matching (`ma`, `má`, `mà` are three +entries). + +## What the review caught + +Four things a passing test suite did not: + +1. **`/help` was 9 runes from its 4096-rune pin.** Measured directly: the module cost 480 of + the 489 runes that were left. `RenderHelp` HTML-escapes both halves of every line, so each + `'` costs 5 runes and each `` costs 14. Shortening six command descriptions + restored headroom to 76. The ceiling is structural — `/help` sends one un-chunked message — + and the next module anywhere in the repo will hit it. +2. **Four surviving mutants.** Three HTML-escaping sites and the post-normalization byte cap + were correct in code but pinned by no test. Fixed, and each new test was verified to fail + with its guard removed rather than assumed to be load-bearing. +3. **Usage strings contradicted their own `Parameters` metadata** (`` vs ``), + against `docs/command-parameter-conventions.md` item 3. +4. **Reply-chain thread ids.** `threadOf` trusted `msg.MessageThreadID` unconditionally. + Telegram associates a thread id with any reply chain in a supergroup, not only with a + forum topic, so a reply-form `/blacklist_add` in a plain supergroup would have written to a + scope no standalone `/blacklist_rules` could read back. Now gated on `IsTopicMessage`. + +## DocStore.Scan landed mid-work + +The push was rejected: four commits had landed on `main` meanwhile, one adding +`DocStore.Scan` specifically to kill the List-then-Get-per-key pattern. The phase-3 design +had copied exactly that pattern from `alias.renderNames`, and `/blacklist_rules` paid it +twice per call. Rebased and adopted `Scan` before pushing. `/blacklist_check` still uses +`List`, needing only entry names. + +Worth noting for next time: a plan written against a contract can be overtaken by that +contract while the plan is being executed. The rebase was the moment to re-read what changed, +not just to resolve conflicts. + +## Decisions + +- Per-thread entry count stays unbounded, matching `alias`. Entries remain capped at 200 + bytes each. +- `internal/modules/module.go` shows a one-space gofmt drift under go1.27.1 but is untouched + by this work and accepted by `golangci-lint`. Left alone rather than adding unrelated churn. + +## Next steps + +- Watch `/help` — 76 runes of headroom is one command away from red. Chunking it is a shared + surface change nobody has scoped yet. +- Confirm on the live bot that reply-form `/blacklist_add` and a later `/blacklist_rules` + agree in a non-forum supergroup. + +> Historical work record — not durable authority. Prefer docs/specs/ADRs for current decisions.