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.
This commit is contained in:
tiennm99 committed 2026-09-15 13:48:34 +07:00
1 parent e5125fcd93
commit c47bcf2c7f
4 files changed
+180 -5

No files matched your search

+7
View File
@@ -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.
+33 -4
View File
@@ -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(
"<code>%s</code> 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 <code>%s</code> 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(
"<code>%s</code> 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 <code>%s</code> from this topic's %s.",
html.EscapeString(raw), listName(list)))
}
+72 -1
View File
@@ -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, "<b>Blacklist</b> (1)") || !strings.Contains(first, "<code>cat</code>") {
t.Fatalf("add reply = %q; want the list appended", first)
}
second := send(t, rb, testutil.NewPrivateMessage(7, "/blacklist_add dog"))
if !strings.Contains(second, "<b>Blacklist</b> (2)") {
t.Fatalf("add reply = %q; want both entries counted", second)
}
if !strings.Contains(second, "<code>cat</code>") || !strings.Contains(second, "<code>dog</code>") {
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, "<b>Blacklist</b> (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, "<b>Blacklist</b>")
if strings.Contains(listing, "<code>cat</code>") {
t.Fatalf("del reply = %q; the removed entry is still listed", removed)
}
if !strings.Contains(listing, "<code>dog</code>") {
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, "<b>Blacklist</b> (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, "<b>Whitelist</b> (1)") {
t.Fatalf("whitelist add reply = %q; want the whitelist", got)
}
if strings.Contains(got, "<b>Blacklist</b>") {
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)
}
}
@@ -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 `<text...>` 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** (`<text>` vs `<text...>`),
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.